Skip to content

feat(openfga): async reconciliation outbox (Options B+C follow-up to #567)#631

Merged
JohnRDOrazio merged 50 commits into
developmentfrom
feature/openfga-async-reconciliation-design
Jun 3, 2026
Merged

feat(openfga): async reconciliation outbox (Options B+C follow-up to #567)#631
JohnRDOrazio merged 50 commits into
developmentfrom
feature/openfga-async-reconciliation-design

Conversation

@JohnRDOrazio
Copy link
Copy Markdown
Member

@JohnRDOrazio JohnRDOrazio commented Jun 3, 2026

Closes the Options B and C work from issue #567. Lands the spec, the implementation plan, and the 37 implementation commits as one PR per the design's "B and C share a single coherent design" decision.

What this delivers

A Postgres outbox + Redis Streams + systemd consumer + cron backstop that turns OpenFGA tuple writes/deletes from "best-effort during the request" into "eventually consistent with admin-visible DLQ".

Architecture

HTTP request → handler (PG tx wraps DB write + outbox.insertBatch)
            → sync fast-path: OutboxProcessor::processSync per row
            → XADD to Redis Stream for any still-pending row
            
systemd ↔ ConsumerLoop ↔ RedisStreamConsumer ↔ OutboxProcessor (XREADGROUP, sub-second reconciliation)
cron    → bin/reconcile-outbox backstop → BackstopRunner → OutboxProcessor (every 5 min, picks up cracks)
admin   → GET /admin/outbox?status=… | ?summary=1 | POST /admin/outbox/{id}/retry
ops     → GET /health → openfga_outbox block (counts, oldest_pending_age, consumer.redis_reachable, oldest_pel_idle)

Invariants

  • Atomicity: handler's PG transaction wraps repo.write + outbox.insertBatch. Either both commit or neither.
  • Idempotency: deterministic handler-built keys + UNIQUE index on metadata->>'idempotency_key' + ON CONFLICT DO NOTHING. Reissuing the same approve/revoke returns the same outbox row IDs.
  • Terminal-status stickiness: all four UPDATE mutations include WHERE … AND status IN (…) guards. markRetryable cannot downgrade a failed_terminal row.
  • At-least-once: SKIP LOCKED + consumer PEL + backstop together guarantee every row is eventually attempted.
  • Graceful degradation: Redis unreachable → handler skips XADD (logged WARNING); backstop drains within 5 min. PG durability unaffected.

Retry policy

Exponential backoff 1s → 2s → 4s → … → 512s, capped at 2^9. Max 10 attempts (~17 min budget) before a row goes failed_terminal. Validation 4xx is terminal on first attempt. Benign duplicates (TupleAlreadyExistsException / TupleNotFoundException) count as success.

Semantic shift from #628

success: true now means DB committed rather than all sync FGA work succeeded. New fields outbox_pending, outbox_failed, outbox_ids carry the side-effect state; the message field carries the warning when reconciliation is deferred. Pre-existing tests that asserted success: false on transient FGA failures were updated to reflect the new contract.

Files

New (~30): migration, OutboxOperation/OutboxStatus/OutboxDisposition enums, OutboxBackoff, OutboxClassifier, OutboxRow, OutboxRepository, OutboxProcessor (+ OutboxProcessorInterface), OutboxNotifier, RedisStreamConsumer (+ StreamConsumerInterface), ConsumerLoop, BackstopRunner, bin/reconcile-outbox, OutboxAdminHandler, HealthHandler, Handlers/Ops/, deploy artifacts (systemd unit + cron + runbook), tests for every layer.

Modified: AccessRequestAdminHandler (approve + revoke refactored), PermissionAdminHandler (grant + revoke refactored), RoleCascadeService (silent catch → outbox enqueue), Router (/admin/outbox and /health routes), Health.php (outbox stats), .env.example, composer.json.

Test plan

  • composer test — 1235 tests / 5 pre-existing skips / no new skips
  • composer analyse — PHPStan L10 clean (267 files)
  • composer lint — phpcs PSR-12 clean
  • composer lint:md — markdown lint clean
  • composer reconciler:backstop — end-to-end CLI works
  • Migration up/down roundtrip — verified
  • Reviewer: deploy the systemd unit + cron on staging and verify the consumer drains a real approve into OpenFGA

Process trail

Out of scope (per spec §10)

  • Zitadel role-sync moving into the outbox (its zitadel_sync_status column stays).
  • Automated pruning of succeeded rows (runbook documents the SQL; ship automation only when table size warrants).
  • Bulk /retry-all endpoint (single-row retry covers v1).
  • lease_until column for multi-consumer scale (YAGNI; trivial to add later).
  • Metrics framework (tracked in Add API usage analytics + metrics emission framework #630).
  • Retry-After header honoring for 429s (premature until observed).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Health endpoint with DB + outbox status; admin endpoint to list/summary/retry outbox rows
    • Async OpenFGA reconciliation: consumer loop, Redis-stream notifications, cron backstop, CLI entrypoint and systemd service
  • Documentation

    • Operator runbook and design doc with install, diagnostics, and troubleshooting
  • Tests

    • Extensive test coverage for outbox pipeline, consumer, processor, notifier, classifier, backoff, handlers, and health
  • Chores

    • Example env vars for reconciliation, composer scripts and extension suggestions, Redis stub for CI/dev

JohnRDOrazio and others added 30 commits June 2, 2026 15:09
#567

PR #628 closed #567 Option A (typed exceptions + fail-fast in approve/revoke)
and #571 (ListObjects in filterByAdminAccess). The original issue framing was
"Pick one (or layer them)" across three options; B (compensating actions) and
C (outbox + async reconciliation) remain.

This commit adds the design document for landing B+C layered together as one
PR. Architecture:

  - DB outbox table (canonical) atomically committed with the business write
  - Redis Streams + systemd long-lived consumer for sub-second reconciliation
    on the happy path
  - Cron-driven backstop every 5 min as durability anchor for the narrow window
    where PG committed but the API process died before XADD
  - Hybrid response model: success = (DB committed); outbox_pending /
    outbox_failed / outbox_ids carry the side-effect state separately
  - Exponential backoff (1s..512s, 10 attempts, ~17 min budget) then
    failed_terminal with admin-visible DLQ at /admin/outbox

No code in this commit — spec only. Implementation plan will be generated next
via the writing-plans skill, then executed in subsequent commits on this branch.

Related: opens #630 (tracking issue for future API metrics / analytics
framework — out of scope for this work).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
31 tasks across 11 phases, each one TDD-cycle granular:

  Phase 1 — schema (migration)
  Phase 2 — pure types and logic (operation/status/disposition enums,
            backoff, classifier)
  Phase 3 — repository (insertBatch with idempotency, getById,
            mark{Succeeded,Retryable,FailedTerminal}, pickupPending
            with FOR UPDATE SKIP LOCKED, resetForRetry, countByStatus)
  Phase 4 — OutboxProcessor (single point of OpenFGA contact)
  Phase 5 — Redis layer (Notifier best-effort XADD, Streams consumer
            with XCLAIM stale reclaim)
  Phase 6 — runners + CLI (BackstopRunner, ConsumerLoop, bin/reconcile-outbox)
  Phase 7 — handler integration (Access* and Permission* handlers,
            plus RoleCascadeService silent-catch fix)
  Phase 8 — /admin/outbox endpoint + router
  Phase 9 — /health outbox block
  Phase 10 — deployment artifacts (systemd unit, cron, runbook, env)
  Phase 11 — full quality gates + smoke test

Each task pins exact file paths, complete test+impl code, commands with
expected output, and an exact commit message. No "fill in" placeholders.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
One row per OpenFGA tuple operation. Atomically committed with the
business write; drained by the consumer (Redis Streams) and the cron
backstop. Partial indexes on (status, next_attempt_at) keep the pickup
hot path tight; UNIQUE constraint on metadata->>'idempotency_key' makes
re-issued handler calls a no-op insert.

Part of issue #567 Options B+C.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implements OutboxRepository::insertBatch() as a PDO repository method
that writes rows to openfga_outbox idempotently: on conflict with the
expression UNIQUE index on metadata->>'idempotency_key', the existing
row's ID is returned rather than inserting a duplicate. Covered by two
PHPUnit tests (happy-path and conflict path) against a real Postgres DB.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…minal stickiness

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add Redis stubs (stubs/Redis.php) for environments without ext-redis so
that PHPUnit createMock(\Redis::class) and PHPStan analysis work in CI.
Wire the stubs into phpunit_tests/bootstrap.php.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Introduces StreamConsumerInterface and OutboxProcessorInterface so
ConsumerLoop can accept mocked collaborators in tests without requiring
bypass-finals tooling. RedisStreamConsumer and OutboxProcessor implement
their respective interfaces; no existing behaviour is changed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- `approveRequest` now writes outbox rows atomically with `repo.approve()`
  inside a PG transaction; tuple delivery failures no longer abort the DB
  approval. `success` is true whenever the DB committed.
- Adds `setOutboxDependencies(repo, notifier, processor)` for test injection
  and lazy getters (`getOutboxRepository`, `getOutboxNotifier`,
  `getOutboxProcessor`) for the production path.
- Response shape gains `outbox_ids`, `outbox_pending`, `outbox_failed`,
  and carries `fga_errors` for back-compat (mirrors failed outbox entries).
- Fast path (no permissions, or FGA unavailable) skips the outbox entirely
  and returns immediately after `repo.approve()`.
- Updates two pre-existing tests whose assertions reflected the old
  fail-before-DB-commit behavior:
  - `testApproveSucceedsWhenAllTupleWritesSucceed` → injects outbox deps
  - `testApproveBailsAndKeepsRequestPendingOnRealFgaError` renamed to
    `testApproveCommitsDbAndSurfacesTerminalOutboxFailureOnRealFgaError`,
    now asserts DB row is 'approved' with `outbox_failed=1`.
- Adds `openfga_outbox` to `AbstractHandlerTestCase::TABLES` so rows are
  truncated between tests.
- Adds two new tests: `testApproveCommitsOutboxRowsAtomicallyWithDbWrite`
  (mixed 503/200 responses → one retrying, one succeeded) and
  `testApproveIsIdempotentOnReissue` (idempotency_key collapse + handler
  rejects re-approval of an already-approved request).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Both grantPermission (WRITE_TUPLE) and revokePermission (DELETE_TUPLE)
now follow the same outbox pattern established in Tasks 20/21 for
AccessRequestAdminHandler. Each mutation: inserts a durable outbox row
in a PG transaction, attempts a sync fast-path immediately, notifies the
async consumer when the row is still non-terminal, and surfaces outbox
counters (outbox_pending, outbox_failed, outbox_ids) in the response.
Idempotency keys are namespaced permission_grant:… / permission_revoke:….

New tests cover: sync success, idempotent re-issue, transient FGA 503
surfaces as outbox_pending, and benign duplicate/missing-tuple handling.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…x rows

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds HTTP GET /health endpoint (HealthHandler) that returns a JSON
health object including an openfga_outbox block with PG-side status
counts and Redis consumer-group info. Health::buildOutboxStats() is
the shared static helper that drives both the outbox metrics and
wires cleanly into future health aggregations.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
JohnRDOrazio and others added 8 commits June 2, 2026 23:41
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the private MAX_ATTEMPTS const with a constructor-injected
maxAttempts parameter (default 10). All three construction sites
(bin/reconcile-outbox, AccessRequestAdminHandler, PermissionAdminHandler)
now read (int)($_ENV['OUTBOX_MAX_ATTEMPTS'] ?? 10), making the env var
actually tunable as the runbook implies.

Adds testCustomMaxAttemptsConfigurable() to OutboxProcessorTest verifying
that maxAttempts=3 transitions to failed_terminal on the 3rd attempt.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously handleRetry() returned 409 for both a missing row and a
non-terminal row, making it impossible for callers to tell the two
cases apart. Now:
  - getById() first; null → 404 {"error": "Outbox row not found", "id": id}
  - resetForRetry() returns false (row is not in failed_terminal) → 409

Adds testPostRetryReturns404ForMissingRow() to OutboxAdminHandlerTest.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The field was always 0 because no Redis call was made to retrieve it.
After the xInfo GROUPS call, now call xPending with the detail form
(start='-', end='+', count=1) to get the oldest pending entry, read
its idle_ms from index 2, and convert to seconds via intdiv.

The xPending call is wrapped in its own try/catch so a secondary Redis
error cannot break the surrounding health response.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
markRetryable() was formatting next_attempt_at as 'Y-m-d H:i:sP'
(second precision) while pickupPending()'s read side bound :now with
'Y-m-d H:i:s.uP' (microsecond precision). Change the write format to
'Y-m-d H:i:s.uP' so both sides use the same precision.

Adds testMarkRetryableRoundtripsMicrosecondPrecision() to
OutboxRepositoryTest asserting the microsecond portion survives the
write/read roundtrip.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Change the constructor parameter type from the concrete OutboxProcessor
to the OutboxProcessorInterface it already implements. This removes a
hard coupling to the concrete class, making BackstopRunner testable with
any implementation (or mock) of the interface.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ixes

Three issues surfaced by static analysis:
- OutboxAdminHandler: PHPStan correctly narrows getById() to non-null
  after the null-guard 404 return; drop the redundant ternary and call
  rowToArray() directly.
- AccessRequestAdminHandler / PermissionAdminHandler: $_ENV values are
  mixed; wrap the OUTBOX_MAX_ATTEMPTS cast in is_numeric() guard (same
  pattern as the rest of the codebase).
- bin/reconcile-outbox: same is_numeric() guard for the cast.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jun 3, 2026

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Implements an OpenFGA async reconciliation outbox: adds a PostgreSQL outbox schema, PDO repository, processor/classifier/backoff, Redis Streams consumer + notifier, CLI consumer/backstop runners, handler integration (approve/grant/revoke), admin endpoints, health reporting, deployment, and tests.

Changes

OpenFGA Async Reconciliation Outbox

Layer / File(s) Summary
Schema, env, and project wiring
src/Migrations/Version20260602202504.php, .env.example, .serena/project.yml, composer.json
Adds openfga_outbox migration and ENUMs; .env.example adds Redis stream/group/consumer and tuning vars; composer requires ext-pdo/ext-pgsql, suggests ext-redis and adds reconciler scripts; project YAML comments updated.
Outbox domain types
src/Services/Outbox/OutboxOperation.php, OutboxStatus.php, OutboxDisposition.php, OutboxRow.php
Adds enums and read-only DTO representing operations, lifecycle statuses, classification dispositions, and row snapshot data.
Repository and persistence tests
src/Repositories/OutboxRepository.php, phpunit_tests/Repositories/OutboxRepositoryTest.php
Idempotent insertBatch, row hydration, state transitions, pickup with FOR UPDATE SKIP LOCKED, reset-for-retry, listing, oldest-pending/counts, and concurrency tests.
Processor, classifier, backoff, and tests
src/Services/Outbox/OutboxBackoff.php, OutboxClassifier.php, OutboxProcessorInterface.php, OutboxProcessor.php, phpunit_tests/Services/Outbox/*
Implements classifier mapping, exponential backoff schedule, processing interface and implementation handling success/retry/terminal transitions and max-attempts; tests for many scenarios.
Redis notifier, consumer, stream contract, stubs, bootstrap and tests
src/Services/Outbox/OutboxNotifier.php, RedisStreamConsumer.php, StreamConsumerInterface.php, stubs/Redis.php, phpunit_tests/bootstrap.php, phpunit_tests/Services/Outbox/*
Best-effort XADD notifier, consumer with group creation/claiming/XREADGROUP/XACK semantics, interface, Redis extension stubs for CI, and related unit tests (notifier, consumer, loop).
Long-lived consumer, backstop runner, CLI and deployment
src/Services/Outbox/ConsumerLoop.php, BackstopRunner.php, bin/reconcile-outbox, deploy/systemd/*, deploy/cron/*
ConsumerLoop tick/run, BackstopRunner one-shot processing with grace window, CLI entrypoint with consumer/backstop, systemd unit and cron job for deployment.
Handler integrations and handler tests
src/Handlers/Admin/PermissionAdminHandler.php, src/Handlers/Admin/AccessRequestAdminHandler.php, phpunit_tests/Handlers/Admin/*
Refactors grant/revoke and approve/revoke to write outbox rows transactionally, attempt synchronous processing, notify async for non-terminal states, expose outbox_* fields in responses, and add extensive handler tests.
Role cascade fallback
src/Services/RoleCascadeService.php, phpunit_tests/Services/RoleCascadeServiceOutboxTest.php
Classifies deleteTuple failures, enqueues DELETE_TUPLE rows when needed, notifies consumer, and tests enqueue vs benign cases.
Admin listing, retry endpoint, health reporting, router
src/Handlers/Admin/OutboxAdminHandler.php, src/Handlers/Ops/HealthHandler.php, src/Health.php, src/Router.php, phpunit_tests/*
Adds /admin/outbox listing and retry, /health with DB/outbox stats (PG counts + Redis consumer-group metrics), router wiring and middleware adjustments, and tests for admin and health endpoints.
Docs and runbook
docs/superpowers/specs/2026-06-02-openfga-async-reconciliation-design.md, docs/ops/openfga-outbox-runbook.md
Design spec and operator runbook covering architecture, invariants, flows, installation, diagnostics, incident handling, and retention guidance.
Test infra updates
phpunit_tests/Handlers/AbstractHandlerTestCase.php, phpunit_tests/Repositories/RepositoryTestCase.php
Includes openfga_outbox in test truncation lists and exposes env helper visibility for reuse; adds Redis stub bootstrapping for environments without ext-redis.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ConsumerLoop
  participant RedisStreamConsumer
  participant OutboxProcessor
  participant OutboxRepository
  participant OpenFgaClient

  ConsumerLoop->>RedisStreamConsumer: readOnce(blockMs)
  RedisStreamConsumer->>ConsumerLoop: emits row_id
  ConsumerLoop->>OutboxProcessor: processOne(row_id)
  OutboxProcessor->>OutboxRepository: getById(row_id)
  OutboxProcessor->>OpenFgaClient: writeTuple/deleteTuple(...)
  OpenFgaClient-->>OutboxProcessor: success|error
  OutboxProcessor->>OutboxRepository: markSucceeded|markRetryable|markFailedTerminal
  OutboxProcessor->>RedisStreamConsumer: (via OutboxNotifier) XADD wake for non-terminal rows
Loading

Possibly related issues

Possibly related PRs

"A rabbit writes in tidy code,
Outbox rows keep state and road,
Redis hums a gentle stream,
Backstop wakes the missed‑tuple dream,
Logs and tests rejoice in this small ode." 🐇

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/openfga-async-reconciliation-design

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 16

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/Handlers/Admin/PermissionAdminHandler.php (1)

645-667: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Deferred tuple deletes can strand the Zitadel role.

The cascade check still runs in-request, against whatever OpenFGA state exists before the outbox consumer finishes any pending/retrying delete rows. If the revoked tuple is the last in-scope permission and its delete is deferred, maybeCascadeRoleRevoke() will return false now, and nothing in the eventual outbox-success path re-runs that role-scope check later. The role can therefore remain assigned after the last tuple is finally deleted.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/Handlers/Admin/PermissionAdminHandler.php` around lines 645 - 667, The
in-request cascade check in PermissionAdminHandler is racy because deferred
tuple deletes can make maybeCascadeRoleRevoke() return false; move the
role-cascade logic into the outbox row success path so it runs after the actual
tuple delete completes. Specifically, remove or keep only a best-effort
pre-check in PermissionAdminHandler and instead invoke
RoleCascadeService::fromEnv()->maybeCascadeRoleRevoke($bareUserId, $role) from
the outbox consumer’s successful-completion handler for delete rows (iterating
roles from AccessRequestRepository::ROLE_OBJECT_TYPES[$role] that include the
deleted $objectType), collect/log any cascaded roles, and catch/log exceptions
non-fatally; ensure OpenFgaClient/ZitadelService checks still guard the call.
🧹 Nitpick comments (4)
phpunit_tests/Handlers/Admin/OutboxAdminHandlerTest.php (1)

25-240: ⚡ Quick win

Add method and route-contract coverage for this handler.

This suite exercises the business flows, but it never pins the transport-level branches in OutboxAdminHandler: OPTIONS preflight, unsupported verbs, and malformed /admin/outbox/{id}/retry paths/IDs. Those are explicit handler behaviors, so leaving them untested makes future routing changes easier to break silently.

As per coding guidelines, "Create test classes extending ApiTestCase in phpunit_tests/ with HTTP method validation and route tests."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@phpunit_tests/Handlers/Admin/OutboxAdminHandlerTest.php` around lines 25 -
240, Add transport-level tests to cover OutboxAdminHandler's HTTP method and
route contract branches: create tests (in OutboxAdminHandlerTest or a new
ApiTestCase-based test) that use requestFor(), withOidcUser(),
makeHandler()->handle() to assert OPTIONS preflight returns 200 with expected
CORS headers, that unsupported verbs (e.g., PUT/DELETE on /admin/outbox and POST
on collection endpoints) return 405, and that malformed or missing retry IDs
(e.g., non-numeric '/admin/outbox/abc/retry' and omitted ID) produce the
appropriate 400/404 responses; reference the existing helper methods
insertOutboxRow, makeHandler(), requestFor(), and decodeJsonBody() to build
requests and assert response status codes and error payload shapes.
phpunit_tests/Repositories/OutboxRepositoryTest.php (1)

269-280: ⚡ Quick win

Exercise non-zero pending and retrying rows too.

This currently proves the terminal buckets and the ?? 0 fallback, but it never validates that countByStatus() groups real pending or retrying rows correctly. Keeping one row pending and marking another retrying would make the test match its name and the PR objective.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@phpunit_tests/Repositories/OutboxRepositoryTest.php` around lines 269 - 280,
The test testCountByStatusBucketsAllFour currently only exercises succeeded and
failed_terminal buckets; modify it to also leave one inserted row as pending and
mark another row as retrying so countByStatus() returns non-zero for 'pending'
and 'retrying'. After calling $this->repo->insertBatch($this->samplePayload())
keep one id untouched (pending) and invoke the repository's retry-marking method
(e.g., $this->repo->markRetrying or equivalent) on another id, then assert
self::assertSame(1, $counts['pending'] ?? 0) and self::assertSame(1,
$counts['retrying'] ?? 0) alongside the existing succeeded/failed_terminal
assertions.
src/Services/Outbox/BackstopRunner.php (1)

30-30: ⚡ Quick win

Use an explicit Europe/Vatican timezone for the cutoff timestamp.

new \DateTimeImmutable() here depends on the host default timezone. Build it with new \DateTimeZone('Europe/Vatican') so this runner stays aligned with the repository-wide date/time rule. As per coding guidelines, "Always use timezone Europe/Vatican for all date/time calculations".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/Services/Outbox/BackstopRunner.php` at line 30, The cutoff timestamp is
created with timezone-sensitive new \DateTimeImmutable(); update the creation in
BackstopRunner so $cutoff is constructed with an explicit Europe/Vatican
timezone, e.g. instantiate new \DateTimeImmutable with new
\DateTimeZone('Europe/Vatican') before calling ->modify using
$this->graceSeconds, ensuring the timezone is applied to the computed cutoff.
src/Services/Outbox/OutboxProcessor.php (1)

72-74: ⚡ Quick win

Use the mandated Vatican timezone for retry scheduling.

new \DateTimeImmutable() here picks up the server default timezone instead of the project-standard Europe/Vatican. Build now with an explicit \DateTimeZone('Europe/Vatican') before applying the backoff delay. As per coding guidelines, "Always use timezone Europe/Vatican for all date/time calculations".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/Services/Outbox/OutboxProcessor.php` around lines 72 - 74, The retry
scheduling currently uses new \DateTimeImmutable() which picks up the server
default timezone; update the creation of the base "now" used for $next so it is
instantiated with the mandated Europe/Vatican timezone before applying the
OutboxBackoff delay (i.e., create a DateTimeImmutable with new
\DateTimeZone('Europe/Vatican') and then ->modify("+{$delay} seconds") ), so
that the $next passed to $this->repo->markRetryable($row->id, $newAttempts,
$next, $message, $code) is always in Europe/Vatican.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@composer.json`:
- Around line 99-100: The composer.json must declare the PHP extensions used by
bin/reconcile-outbox: add ext-pdo_pgsql to the "require" section (used for
building the pgsql DSN in bin/reconcile-outbox) and add ext-redis as well (or
make it an optional platform requirement or suggest) because the consumer path
checks extension_loaded('redis'), instantiates new \Redis() and uses
RedisStreamConsumer; update composer.json's "require" to include "ext-pdo_pgsql"
and "ext-redis" (or refactor bin/reconcile-outbox to avoid requiring redis at
runtime) so runtime extension expectations are declared.

In `@deploy/cron/liturgical-calendar-backstop.cron`:
- Line 5: The cron entry running as user "litcal" redirects output to
/var/log/litcal-backstop.log which can cause the shell to fail if that file/path
isn’t writable; update the cron line invoking "/usr/bin/php
/opt/liturgical-calendar/bin/reconcile-outbox backstop" so it does not rely on
an unwritable global path — either (A) pipe stdout/stderr into the system logger
(logger) with a tag like "litcal-backstop" so journald/syslog captures logs
regardless of file perms, or (B) point the redirection at an app-owned path
(e.g. a directory such as /var/log/litcal/) and ensure that directory/file is
created and owned by the "litcal" user during install; choose one approach and
apply it to the cron entry so the job cannot be prevented from starting by an
unwritable log file.

In `@docs/superpowers/specs/2026-06-02-openfga-async-reconciliation-design.md`:
- Around line 305-339: The long table rows in the spec contain cells exceeding
the repo 180-character Markdown line length (e.g., rows listing
`src/Migrations/Version<TIMESTAMP>.php`,
`src/Services/Outbox/OutboxProcessor.php`,
`src/Handlers/Admin/AccessRequestAdminHandler.php`, etc.); reflow those table
cell contents so each Markdown line is <=180 chars by breaking descriptions into
multiple shorter lines or splitting complex entries into a short summary plus a
bullet/list within the cell, ensuring the table format remains valid and the
same identifiers (file names and symbols) are preserved.

In `@phpunit_tests/Repositories/OutboxRepositoryTest.php`:
- Around line 224-231: The two database transactions started with
self::$pdo->beginTransaction() and $other->beginTransaction() can leak if an
exception occurs before the commits; wrap the sequence that calls
pickupPending(...) on $this->repo and $otherRepo and the subsequent commit()
calls in a try/finally block and in the finally ensure any active transaction is
rolled back on both connections (call rollBack() on self::$pdo and on $other if
they are still in a transaction) so neither shared connection remains open;
reference the existing beginTransaction/commit calls and the pickupPending(...)
calls on repo and otherRepo when making this change.
- Around line 203-218: OutboxRepositoryTest is opening a second PDO using raw
$_ENV which diverges from RepositoryTestCase::setUpBeforeClass()'s env
resolution (which accepts getenv() fallback); update the PDO construction in
OutboxRepositoryTest to reuse the same DB env resolution strategy as
RepositoryTestCase::setUpBeforeClass() — either call the shared helper used by
the base class (or replicate its logic) to read
DB_HOST/DB_PORT/DB_NAME/DB_USER/DB_PASSWORD (checking getenv() when $_ENV is
empty) and build the DSN/credentials from that so both connections behave
identically.

In `@phpunit_tests/Services/Outbox/OutboxProcessorTest.php`:
- Around line 93-95: The test uses a tight wall-clock bound by comparing
$row->nextAttemptAt against a new now() after processing; capture a reference
timestamp before the action (e.g. $before = new \DateTimeImmutable()) and
compute delta as $row->nextAttemptAt->getTimestamp() - $before->getTimestamp(),
then assert it is >= 0 and use a more tolerant upper bound (or assert only that
it is > 0 and reasonably small) to avoid flaky CI; update the assertions that
reference $delta and the message about attempts=1 accordingly so they use the
captured $before timestamp and a relaxed tolerance.

In `@src/Handlers/Admin/AccessRequestAdminHandler.php`:
- Around line 368-387: The fast-path condition currently uses "if
(empty($permissions) || !$this->isFgaClientAvailable())" which allows approval
to commit without creating outbox rows when OpenFGA is unavailable; change the
condition to only check empty($permissions) so that approvals with non-empty
$permissions always proceed through the normal flow that enqueues outbox
entries. Concretely, remove the "|| !$this->isFgaClientAvailable()" check in the
branch guarding the approve($requestId, $adminId, $notes) fast path, so code
paths that call syncZitadelRole, approvalMessage, encodeResponseBody, etc.,
still create outbox rows (and surface outbox_pending/outbox_ids) when
isFgaClientAvailable() is false.
- Around line 793-805: syncZitadelRoleRevoke() is being called before deferred
OpenFGA deletes finish so RoleCascadeService::maybeCascadeRoleRevoke() can
return false while tuples are still pending; move the call so the Zitadel revoke
decision happens only after the deferred deletes are resolved: check the outbox
state (use $outboxPending, $outboxFailed, $outboxIds and $deletedTuples) and
only call syncZitadelRoleRevoke($repo, $requestId, $userId, $requestedRole) when
$outboxPending == 0 (and there are no retrying pending deletes); if pending
deletes remain, set role_removed to false and surface that revocation is
deferred in the response (so revocation can be retried by the outbox consumer),
and keep revocationMessage() updated accordingly.

In `@src/Handlers/Admin/OutboxAdminHandler.php`:
- Around line 211-227: The handler flips a DLQ row to pending via
resetForRetry($id) but never wakes the consumer; after calling
resetForRetry($id) in OutboxAdminHandler, immediately notify the consumer (e.g.,
call the repository's notify/publish method or write the pending id to the
Redis/stream channel) so the consumer sees the retried row right away; ensure
you check the notify call's success (handle/log failure and return an error
response if it fails) before re-fetching with getById($id) and returning
rowToArray($updatedRow).

In `@src/Handlers/Admin/PermissionAdminHandler.php`:
- Around line 462-485: OutboxProcessor::processSync() returning BENIGN_SUCCESS
currently causes the handler to treat the row as a created/deleted tuple
regardless of the row's post-process status; change the branching in the loop
handling $outboxIds so that after calling $processor->processSync($rowId) you
fetch the current $outbox->getById($rowId) and check $current->status (using
OutboxStatus::SUCCEEDED) before pushing into $createdTuples; if the post-process
status is OutboxStatus::FAILED_TERMINAL keep the row in $fgaErrors (include the
same error payload logic), and only treat BENIGN_SUCCESS as a tuple change when
the post-process status is SUCCEEDED (apply the same fix in the other occurrence
block around lines 596-619).

In `@src/Handlers/Ops/HealthHandler.php`:
- Around line 36-40: The health endpoint currently only checks
Connection::isConfigured() and uses Health::buildOutboxStats(), which masks DB
connectivity failures; update the HealthHandler health assembly to actively
probe DB reachability (e.g., run a light SELECT 1 via the same Connection/DB
client or call the repository method that throws on connection failure), catch
exceptions, and set an explicit flag and degraded status when the probe fails
(for example change 'database' to 'unreachable' or 'error' and set overall
'status' to degraded/error). Ensure you use the existing symbols (HealthHandler
class, Connection::isConfigured(), Health::buildOutboxStats()) so the probe is
performed before composing $result, and propagate any failure into the returned
array instead of collapsing to zero counts.
- Around line 27-50: The HealthHandler currently inherits permissive enums and
returns JSON directly, bypassing standard method and content-negotiation checks;
update HealthHandler::__construct to restrict $this->allowedRequestMethods to
only GET (and tighten $this->allowedAcceptHeaders if needed), then in handle()
call $this->validateAcceptHeader($request) and use
Negotiator::negotiateResponseContentType($request) / the handler's return_type
handling to produce the response body and Content-Type header instead of
hardcoding 'application/json; charset=utf-8' so the handler follows the same
negotiation and contract as other handlers (refer to HealthHandler::__construct,
HealthHandler::handle, validateRequestMethod, validateAcceptHeader,
Negotiator::negotiateResponseContentType, and return_type).

In `@src/Router.php`:
- Around line 578-580: The current Router.php conditional excludes 'health' from
the entire auth/key/rate block, which removes IP-based throttling for
HealthHandler (which touches openfga_outbox); update the logic so '/health' is
still unauthenticated but still goes through
ApiKeyRateLimitMiddleware::fromEnv(). Specifically, adjust the
in_array/conditional around Connection::isConfigured() and where
ApiKeyRateLimitMiddleware::fromEnv() is applied so that 'health' is excluded
only from API key/auth checks but not from rate-limiting; keep references to
route, Connection::isConfigured(), ApiKeyRateLimitMiddleware::fromEnv(), and
HealthHandler when making the change.

In `@src/Services/Outbox/BackstopRunner.php`:
- Around line 28-35: runOnce() is calling OutboxRepository::pickupPending() in
autocommit mode which releases the FOR UPDATE SKIP LOCKED locks immediately;
wrap the pickup and processing in a single DB transaction: begin a transaction
before calling $this->repo->pickupPending(...), keep that transaction open while
iterating and calling $this->processor->processOne($row->id), and only commit
(or rollback on error) after all rows are processed; use the
repository/connection or entity manager APIs available to start/commit/rollback
the transaction so the row locks held by pickupPending remain in effect during
processOne().

In `@src/Services/Outbox/OutboxProcessor.php`:
- Around line 35-50: processOne currently ignores OutboxStatus::RETRYING and
will run rows before their backoff expires; update processOne (in the method
processOne(int $rowId)) to inspect $row->status and when it equals
OutboxStatus::RETRYING compare $row->nextAttemptAt to the current time (e.g. new
DateTimeImmutable() or utc now) and if nextAttemptAt is in the future return
OutboxDisposition::BENIGN_SUCCESS; keep the existing flow (invoke,
markSucceeded/markFailed) for due rows only and ensure you use the same date
type/format as stored in the repo to avoid type/format mismatches.

In `@src/Services/Outbox/RedisStreamConsumer.php`:
- Around line 38-39: The Redis consumer group is being created with the start ID
'$' which skips existing stream entries; in RedisStreamConsumer replace the
XGROUP CREATE start ID '$' with '0' so the group’s last-delivered ID is set to 0
and existing outbox entries are eligible for XREADGROUP STREAMS <stream> '>'
delivery; locate the call to $this->redis->xGroup('CREATE', $this->streamName,
$this->groupName, '$', true) (in the RedisStreamConsumer setup/constructor) and
change the fourth argument to '0'.

---

Outside diff comments:
In `@src/Handlers/Admin/PermissionAdminHandler.php`:
- Around line 645-667: The in-request cascade check in PermissionAdminHandler is
racy because deferred tuple deletes can make maybeCascadeRoleRevoke() return
false; move the role-cascade logic into the outbox row success path so it runs
after the actual tuple delete completes. Specifically, remove or keep only a
best-effort pre-check in PermissionAdminHandler and instead invoke
RoleCascadeService::fromEnv()->maybeCascadeRoleRevoke($bareUserId, $role) from
the outbox consumer’s successful-completion handler for delete rows (iterating
roles from AccessRequestRepository::ROLE_OBJECT_TYPES[$role] that include the
deleted $objectType), collect/log any cascaded roles, and catch/log exceptions
non-fatally; ensure OpenFgaClient/ZitadelService checks still guard the call.

---

Nitpick comments:
In `@phpunit_tests/Handlers/Admin/OutboxAdminHandlerTest.php`:
- Around line 25-240: Add transport-level tests to cover OutboxAdminHandler's
HTTP method and route contract branches: create tests (in OutboxAdminHandlerTest
or a new ApiTestCase-based test) that use requestFor(), withOidcUser(),
makeHandler()->handle() to assert OPTIONS preflight returns 200 with expected
CORS headers, that unsupported verbs (e.g., PUT/DELETE on /admin/outbox and POST
on collection endpoints) return 405, and that malformed or missing retry IDs
(e.g., non-numeric '/admin/outbox/abc/retry' and omitted ID) produce the
appropriate 400/404 responses; reference the existing helper methods
insertOutboxRow, makeHandler(), requestFor(), and decodeJsonBody() to build
requests and assert response status codes and error payload shapes.

In `@phpunit_tests/Repositories/OutboxRepositoryTest.php`:
- Around line 269-280: The test testCountByStatusBucketsAllFour currently only
exercises succeeded and failed_terminal buckets; modify it to also leave one
inserted row as pending and mark another row as retrying so countByStatus()
returns non-zero for 'pending' and 'retrying'. After calling
$this->repo->insertBatch($this->samplePayload()) keep one id untouched (pending)
and invoke the repository's retry-marking method (e.g.,
$this->repo->markRetrying or equivalent) on another id, then assert
self::assertSame(1, $counts['pending'] ?? 0) and self::assertSame(1,
$counts['retrying'] ?? 0) alongside the existing succeeded/failed_terminal
assertions.

In `@src/Services/Outbox/BackstopRunner.php`:
- Line 30: The cutoff timestamp is created with timezone-sensitive new
\DateTimeImmutable(); update the creation in BackstopRunner so $cutoff is
constructed with an explicit Europe/Vatican timezone, e.g. instantiate new
\DateTimeImmutable with new \DateTimeZone('Europe/Vatican') before calling
->modify using $this->graceSeconds, ensuring the timezone is applied to the
computed cutoff.

In `@src/Services/Outbox/OutboxProcessor.php`:
- Around line 72-74: The retry scheduling currently uses new
\DateTimeImmutable() which picks up the server default timezone; update the
creation of the base "now" used for $next so it is instantiated with the
mandated Europe/Vatican timezone before applying the OutboxBackoff delay (i.e.,
create a DateTimeImmutable with new \DateTimeZone('Europe/Vatican') and then
->modify("+{$delay} seconds") ), so that the $next passed to
$this->repo->markRetryable($row->id, $newAttempts, $next, $message, $code) is
always in Europe/Vatican.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f976dedb-f085-47d4-a159-175f50a3c5b9

📥 Commits

Reviewing files that changed from the base of the PR and between 51e25a2 and 3cc05e8.

⛔ Files ignored due to path filters (1)
  • .claude/scheduled_tasks.lock is excluded by !**/*.lock
📒 Files selected for processing (48)
  • .env.example
  • .serena/project.yml
  • bin/reconcile-outbox
  • composer.json
  • deploy/cron/liturgical-calendar-backstop.cron
  • deploy/systemd/liturgical-calendar-reconciler.service
  • docs/ops/openfga-outbox-runbook.md
  • docs/superpowers/plans/2026-06-02-openfga-async-reconciliation.md
  • docs/superpowers/specs/2026-06-02-openfga-async-reconciliation-design.md
  • phpunit_tests/Handlers/AbstractHandlerTestCase.php
  • phpunit_tests/Handlers/Admin/AccessRequestAdminHandlerTest.php
  • phpunit_tests/Handlers/Admin/OutboxAdminHandlerTest.php
  • phpunit_tests/Handlers/Admin/PermissionAdminHandlerTest.php
  • phpunit_tests/Repositories/OutboxRepositoryTest.php
  • phpunit_tests/Repositories/RepositoryTestCase.php
  • phpunit_tests/Services/Outbox/BackstopRunnerTest.php
  • phpunit_tests/Services/Outbox/ConsumerLoopTest.php
  • phpunit_tests/Services/Outbox/OutboxBackoffTest.php
  • phpunit_tests/Services/Outbox/OutboxClassifierTest.php
  • phpunit_tests/Services/Outbox/OutboxNotifierTest.php
  • phpunit_tests/Services/Outbox/OutboxProcessorTest.php
  • phpunit_tests/Services/Outbox/RedisStreamConsumerTest.php
  • phpunit_tests/Services/RoleCascadeServiceOutboxTest.php
  • phpunit_tests/Services/RoleCascadeServiceTest.php
  • phpunit_tests/bootstrap.php
  • src/Handlers/Admin/AccessRequestAdminHandler.php
  • src/Handlers/Admin/OutboxAdminHandler.php
  • src/Handlers/Admin/PermissionAdminHandler.php
  • src/Handlers/Ops/HealthHandler.php
  • src/Health.php
  • src/Migrations/Version20260602202504.php
  • src/Repositories/OutboxRepository.php
  • src/Router.php
  • src/Services/Outbox/BackstopRunner.php
  • src/Services/Outbox/ConsumerLoop.php
  • src/Services/Outbox/OutboxBackoff.php
  • src/Services/Outbox/OutboxClassifier.php
  • src/Services/Outbox/OutboxDisposition.php
  • src/Services/Outbox/OutboxNotifier.php
  • src/Services/Outbox/OutboxOperation.php
  • src/Services/Outbox/OutboxProcessor.php
  • src/Services/Outbox/OutboxProcessorInterface.php
  • src/Services/Outbox/OutboxRow.php
  • src/Services/Outbox/OutboxStatus.php
  • src/Services/Outbox/RedisStreamConsumer.php
  • src/Services/Outbox/StreamConsumerInterface.php
  • src/Services/RoleCascadeService.php
  • stubs/Redis.php

Comment thread composer.json
Comment thread deploy/cron/liturgical-calendar-backstop.cron Outdated
Comment thread phpunit_tests/Repositories/OutboxRepositoryTest.php Outdated
Comment thread phpunit_tests/Repositories/OutboxRepositoryTest.php Outdated
Comment thread src/Handlers/Ops/HealthHandler.php
Comment thread src/Router.php Outdated
Comment thread src/Services/Outbox/BackstopRunner.php
Comment thread src/Services/Outbox/OutboxProcessor.php
Comment thread src/Services/Outbox/RedisStreamConsumer.php Outdated
JohnRDOrazio and others added 5 commits June 3, 2026 01:57
Two correctness fixes from CodeRabbit on PR #631.

BackstopRunner.runOnce() was calling OutboxRepository::pickupPending()
in PG's implicit-autocommit mode, which releases the FOR UPDATE SKIP
LOCKED row locks as soon as each pickup statement returns. With both
the systemd consumer and the cron backstop in production, concurrent
runners could observe and re-process the same row. Wrap pickup +
processOne() iteration in a single explicit transaction so the locks
survive until commit. Existing tests already exercised the SKIP LOCKED
behavior; they're updated to pass the new constructor PDO arg.

OutboxProcessor.processOne() did not respect next_attempt_at when the
row was in RETRYING state. pickupPending already filters on
next_attempt_at, but the consumer's XCLAIM-from-PEL path can hand the
processor a row that another runner just rescheduled. Add a guard: if
row.status == RETRYING and row.nextAttemptAt > now, no-op as BENIGN_SUCCESS.
Both new \DateTimeImmutable instances use Europe/Vatican explicitly per
the project-wide timezone convention; consistent with the repo's
storage of next_attempt_at.

Test time-bound assertion in OutboxProcessorTest also tightened: capture
$before BEFORE processOne() instead of comparing to a fresh now() after,
which was flaky under slow CI.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit on PR #631: the fast-path guard
  if (empty($permissions) || !$this->isFgaClientAvailable())
silently dropped the outbox writes when OpenFGA was unavailable —
permanently losing the tuple work the moment FGA came back online.

The fix splits the two cases:
 - empty($permissions): same as before — DB-only approve/revoke, no
   outbox rows needed.
 - $permissions present + FGA unavailable: PG transaction still commits
   the business write AND the outbox rows; the sync fast path is then
   skipped (processor is null), the rows stay 'pending', and the cron
   backstop drains them once FGA is reachable.

Applied symmetrically to approveRequest (write_tuple) and revokeRequest
(delete_tuple). $processor lazily acquired only when fgaAvailable; the
loop disposes RETRY for skipped rows so they stay pending.

Updated testRevokeHappyPathWithoutFgaOrZitadel to reflect the new
contract: with one permission and FGA unavailable, the response now
carries outbox_pending=1, outbox_ids has one entry, and fga_errors
shows the row as 'pending' (queued for the backstop). The DB revoke
itself is unchanged — atomic with the outbox insert.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…limited

Three CodeRabbit findings on PR #631:

1. HealthHandler returned 'database: configured' on configuration alone,
   never actually checking if PG was reachable. An outage would be
   invisible to monitoring tooling. Added an active SELECT 1 probe; on
   failure: 'database: unreachable', overall 'status: degraded', and
   HTTP 503 so load balancers and uptime checks fail over correctly.

2. HealthHandler inherited the permissive default allowed-methods list
   from AbstractHandler. Restrict to GET only via the constructor;
   non-GET requests now get 405 from validateRequestMethod() rather
   than silently being treated as GET-equivalent.

3. Router.php previously excluded 'health' from the entire api-key /
   rate-limit block, removing IP-based throttling even though /health
   touches PG (and openfga_outbox) on every request. Unauthenticated
   floods could exhaust DB capacity. Split the conditional: 'health'
   stays excluded from API-key auth (it has no API key concept) but
   now flows through ApiKeyRateLimitMiddleware::fromEnv() like every
   other unauthenticated public route.

Content-negotiation via Negotiator is deliberately NOT introduced —
/health is JSON-only by contract for monitoring tooling stability.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two CodeRabbit findings on PR #631 about Redis-side robustness:

1. RedisStreamConsumer::ensureGroup() created the consumer group with
   start ID '$', which silently dropped any messages already on the
   stream when the consumer started. The DB outbox is the source of
   truth so messages weren't lost permanently (the backstop drains
   them up to 5 min later), but the fast-path latency degraded for
   that startup window. Change to '0' so pre-existing entries are
   delivered. Re-delivering an already-terminal or already-due row is
   safe — processOne handles both as idempotent no-ops. Mock
   expectation in RedisStreamConsumerTest updated to match.

2. OutboxAdminHandler::POST /admin/outbox/{id}/retry reset a row to
   pending but didn't notify Redis, so the consumer wouldn't see the
   reset until the cron backstop fired (up to 5 min). Inject an
   optional OutboxNotifier (test injection mirrors the existing
   OutboxRepository injection) and XADD after the reset succeeds.
   Best-effort: the notifier swallows Redis exceptions; the backstop
   remains the safety net.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ygiene

Four small CodeRabbit findings on PR #631:

1. composer.json declared no PDO/PGSQL extensions. bin/reconcile-outbox
   and OutboxRepository both rely on ext-pdo + ext-pdo_pgsql; add both
   as hard requirements. ext-redis is only required for the long-lived
   consumer (the backstop drains everything regardless); declared as a
   "suggest" so consumer-only deployments will see the recommendation
   without breaking PG-only ones.

2. Cron entry redirected stdout/stderr to /var/log/litcal-backstop.log
   which can prevent the job from running entirely if the path isn't
   writable by the `litcal` user (a common install-step oversight).
   Replaced with `2>&1 | logger -t litcal-backstop` so output lands in
   journald/syslog with a discoverable tag, no filesystem dependency.

3. OutboxRepositoryTest opened a second PDO using raw $_ENV reads which
   diverged from RepositoryTestCase::setUpBeforeClass()'s env resolution
   (env array first, getenv() fallback — works in CI where vars may only
   be in getenv()). Promoted RepositoryTestCase::env() to protected
   static and refactored the test to use it. Same connection now opens
   under both CI and local-dev configs.

4. The same test's begin/commit pair had no error handler — a failed
   assertion between begin and commit would leak open transactions on
   the shared connections, cascading into TRUNCATE failures in the
   next test's setUp. Wrapped in try/finally that rolls back any still-
   active transaction on either connection.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (2)
src/Handlers/Ops/HealthHandler.php (1)

29-36: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Restore standard handler negotiation/constructor contract for /health.

HealthHandler is now GET-only (good), but it still bypasses the shared handler contract by not configuring accept/content-type constraints in __construct() and by hardcoding JSON in handle() instead of using validateAcceptHeader() + Negotiator::negotiateResponseContentType() (including return_type support).

Suggested minimal patch
 public function __construct()
 {
     parent::__construct();
     $this->allowedRequestMethods = [RequestMethod::GET];
+    $this->allowedAcceptHeaders = [/* JSON/XML enums used by other handlers */];
+    $this->allowedRequestContentTypes = [/* likely empty or explicit for GET-only */];
 }

 public function handle(ServerRequestInterface $request): ResponseInterface
 {
     $this->validateRequestMethod($request);
+    $this->validateAcceptHeader($request);
+    $contentType = Negotiator::negotiateResponseContentType($request);
     ...
     return new Response(
         $statusCode,
-        ['Content-Type' => 'application/json; charset=utf-8'],
-        $body
+        ['Content-Type' => $contentType],
+        $encodedBodyFor($contentType, $result)
     );
 }

As per coding guidelines: "Set allowed methods, accept headers, and content types in handler constructor" and "Use Negotiator::negotiateResponseContentType() to respect return_type query parameter and Accept header for content negotiation".

Also applies to: 38-76

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/Handlers/Ops/HealthHandler.php` around lines 29 - 36, HealthHandler
currently restricts methods but skips the standard negotiation contract: update
the constructor (HealthHandler::__construct) to set allowedAcceptTypes and
allowedContentTypes alongside allowedRequestMethods (e.g., include
"application/json" and any other supported types) and remove hardcoded JSON
emission in handle(); instead call validateAcceptHeader() early in handle() and
use Negotiator::negotiateResponseContentType($request,
$this->allowedAcceptTypes, $this->allowedContentTypes) (which will respect the
return_type query param) to pick the response content type, then serialize the
payload according to the negotiated type before returning. Ensure
allowedRequestMethods, allowedAcceptTypes, and allowedContentTypes are set in
__construct() and that handle() uses validateAcceptHeader() and
Negotiator::negotiateResponseContentType() rather than hardcoding
"application/json".
src/Handlers/Admin/AccessRequestAdminHandler.php (1)

760-766: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Don't decide the Zitadel revoke while tuple deletes are still deferred.

When these lines leave delete rows in pending/retrying, Line 815 still calls syncZitadelRoleRevoke() immediately. RoleCascadeService::maybeCascadeRoleRevoke() reads current OpenFGA state, so it can keep the role because those tuples have not been deleted yet, and nothing in this outbox flow re-checks after the consumer/backstop finishes.

Also applies to: 814-815

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/Handlers/Admin/AccessRequestAdminHandler.php` around lines 760 - 766, The
current flow can call syncZitadelRoleRevoke() while tuple-deletes remain in
pending/retrying state (because isFgaClientAvailable() returned false or deletes
were deferred), so change the logic around
isFgaClientAvailable()/getOutboxProcessor()/getOutboxNotifier() to ensure
syncZitadelRoleRevoke() is only invoked after the outbox processor has fully
processed/delete-triggering messages (i.e., no pending/retrying rows).
Concretely: when getOutboxProcessor() is null or when the outbox still reports
pending/retrying deletes, do not call syncZitadelRoleRevoke(); instead register
a callback / use getOutboxNotifier() to wait for processor completion (or poll
the outbox state) and then invoke
RoleCascadeService::maybeCascadeRoleRevoke()/syncZitadelRoleRevoke() once the
deletions are durable in OpenFGA. Use the unique symbols isFgaClientAvailable(),
getOutboxProcessor(), getOutboxNotifier(), syncZitadelRoleRevoke(), and
RoleCascadeService::maybeCascadeRoleRevoke() to locate and implement this
ordering change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@src/Handlers/Admin/AccessRequestAdminHandler.php`:
- Around line 760-766: The current flow can call syncZitadelRoleRevoke() while
tuple-deletes remain in pending/retrying state (because isFgaClientAvailable()
returned false or deletes were deferred), so change the logic around
isFgaClientAvailable()/getOutboxProcessor()/getOutboxNotifier() to ensure
syncZitadelRoleRevoke() is only invoked after the outbox processor has fully
processed/delete-triggering messages (i.e., no pending/retrying rows).
Concretely: when getOutboxProcessor() is null or when the outbox still reports
pending/retrying deletes, do not call syncZitadelRoleRevoke(); instead register
a callback / use getOutboxNotifier() to wait for processor completion (or poll
the outbox state) and then invoke
RoleCascadeService::maybeCascadeRoleRevoke()/syncZitadelRoleRevoke() once the
deletions are durable in OpenFGA. Use the unique symbols isFgaClientAvailable(),
getOutboxProcessor(), getOutboxNotifier(), syncZitadelRoleRevoke(), and
RoleCascadeService::maybeCascadeRoleRevoke() to locate and implement this
ordering change.

In `@src/Handlers/Ops/HealthHandler.php`:
- Around line 29-36: HealthHandler currently restricts methods but skips the
standard negotiation contract: update the constructor
(HealthHandler::__construct) to set allowedAcceptTypes and allowedContentTypes
alongside allowedRequestMethods (e.g., include "application/json" and any other
supported types) and remove hardcoded JSON emission in handle(); instead call
validateAcceptHeader() early in handle() and use
Negotiator::negotiateResponseContentType($request, $this->allowedAcceptTypes,
$this->allowedContentTypes) (which will respect the return_type query param) to
pick the response content type, then serialize the payload according to the
negotiated type before returning. Ensure allowedRequestMethods,
allowedAcceptTypes, and allowedContentTypes are set in __construct() and that
handle() uses validateAcceptHeader() and
Negotiator::negotiateResponseContentType() rather than hardcoding
"application/json".

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e5a9efdb-2712-4d8a-b716-1bf71c3aaa38

📥 Commits

Reviewing files that changed from the base of the PR and between 3cc05e8 and d72fceb.

⛔ Files ignored due to path filters (1)
  • composer.lock is excluded by !**/*.lock
📒 Files selected for processing (16)
  • bin/reconcile-outbox
  • composer.json
  • deploy/cron/liturgical-calendar-backstop.cron
  • phpunit_tests/Handlers/Admin/AccessRequestAdminHandlerTest.php
  • phpunit_tests/Repositories/OutboxRepositoryTest.php
  • phpunit_tests/Repositories/RepositoryTestCase.php
  • phpunit_tests/Services/Outbox/BackstopRunnerTest.php
  • phpunit_tests/Services/Outbox/OutboxProcessorTest.php
  • phpunit_tests/Services/Outbox/RedisStreamConsumerTest.php
  • src/Handlers/Admin/AccessRequestAdminHandler.php
  • src/Handlers/Admin/OutboxAdminHandler.php
  • src/Handlers/Ops/HealthHandler.php
  • src/Router.php
  • src/Services/Outbox/BackstopRunner.php
  • src/Services/Outbox/OutboxProcessor.php
  • src/Services/Outbox/RedisStreamConsumer.php
🚧 Files skipped from review as they are similar to previous changes (11)
  • phpunit_tests/Repositories/RepositoryTestCase.php
  • phpunit_tests/Services/Outbox/RedisStreamConsumerTest.php
  • src/Router.php
  • phpunit_tests/Services/Outbox/BackstopRunnerTest.php
  • phpunit_tests/Services/Outbox/OutboxProcessorTest.php
  • src/Handlers/Admin/OutboxAdminHandler.php
  • bin/reconcile-outbox
  • src/Services/Outbox/OutboxProcessor.php
  • src/Services/Outbox/RedisStreamConsumer.php
  • phpunit_tests/Repositories/OutboxRepositoryTest.php
  • phpunit_tests/Handlers/Admin/AccessRequestAdminHandlerTest.php

…umer

CodeRabbit/CodeCov reported 66% patch coverage on PR #631 with the three
biggest gaps in code that was either entirely untested (HealthHandler,
Health::buildOutboxStats) or only had its happy path tested
(RedisStreamConsumer's XCLAIM stale reclaim).

New tests (11 total):

* phpunit_tests/Handlers/Ops/HealthHandlerTest.php (3 tests) — covers
  the GET happy path (200, reachable, openfga_outbox shape), the
  DB-not-configured branch (status stays "ok", database "not_configured"),
  and the 405 on POST (verifying allowedRequestMethods = [GET]).

* phpunit_tests/HealthOutboxStatsTest.php (3 tests) — covers the static
  Health::buildOutboxStats helper: empty-outbox baseline; per-status
  bucketing + oldest_pending_age_seconds for a populated table;
  env-driven stream/group name override.

* phpunit_tests/Services/Outbox/RedisStreamConsumerTest.php (5 new
  tests) — exercises previously-unhit branches in claimStale and
  readOnce: empty PEL skip, idle-message XCLAIM + XACK, fresh-entry
  skip (idle < threshold), malformed message gets XACK'd without
  invoking callback, processor exception leaves the message in PEL.

Also adds assertions to testApproveHappyPathWithoutFgaOrZitadel to
mirror the revoke counterpart's new contract checks (outbox_pending,
outbox_ids, fga_errors) — the branch was already covered, just not
asserted.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@phpunit_tests/Handlers/Ops/HealthHandlerTest.php`:
- Line 14: HealthHandlerTest currently extends AbstractHandlerTestCase (which
itself extends PHPUnit\Framework\TestCase) and invokes the handler in-process
via handle(), violating the project rule that phpunit_tests/**/* must extend
LiturgicalCalendar\Tests\ApiTestCase and that route tests assert server/route
wiring for /health. Change HealthHandlerTest to extend
LiturgicalCalendar\Tests\ApiTestCase instead of AbstractHandlerTestCase, remove
or stop relying on direct handler.handle() invocation, and rewrite the test to
perform an HTTP-level request against the /health route (using the ApiTestCase
client helper or test HTTP client provided by ApiTestCase) asserting the route
is wired and returns the expected status/body; keep references to
HealthHandlerTest, AbstractHandlerTestCase, ApiTestCase, handle(), and the
/health route when making changes.

In `@phpunit_tests/HealthOutboxStatsTest.php`:
- Line 20: HealthOutboxStatsTest currently extends RepositoryTestCase but per
phpunit_tests guidelines it must be an ApiTestCase-based test that exercises
HTTP methods and routes; update the class declaration to extend ApiTestCase
(replace RepositoryTestCase with ApiTestCase) and add tests that perform HTTP
requests against the outbox-related routes (e.g., GET/POST on the health/outbox
endpoints) validating status codes and response bodies, or alternatively move
HealthOutboxStatsTest into the repository-specific test folder if you intend to
keep it as repository-only; ensure the class name HealthOutboxStatsTest and any
helper methods/assertions are preserved while converting repository assertions
into HTTP request assertions using the ApiTestCase client methods.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: cf2dc11e-c57f-48e4-81b1-4339611154ab

📥 Commits

Reviewing files that changed from the base of the PR and between d72fceb and cc580b5.

📒 Files selected for processing (4)
  • phpunit_tests/Handlers/Admin/AccessRequestAdminHandlerTest.php
  • phpunit_tests/Handlers/Ops/HealthHandlerTest.php
  • phpunit_tests/HealthOutboxStatsTest.php
  • phpunit_tests/Services/Outbox/RedisStreamConsumerTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
  • phpunit_tests/Handlers/Admin/AccessRequestAdminHandlerTest.php

use PHPUnit\Framework\Attributes\CoversClass;

#[CoversClass(HealthHandler::class)]
final class HealthHandlerTest extends AbstractHandlerTestCase
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the AbstractHandlerTestCase definition
fd -i "AbstractHandlerTestCase.php" . -a -t f 2>/dev/null || true
rg -n --hidden --no-ignore-vcs "class\s+AbstractHandlerTestCase\b" -S .

# Print the AbstractHandlerTestCase file (or relevant section) with line numbers
# (We’ll re-run with the exact file path once found.)

Repository: Liturgical-Calendar/LiturgicalCalendarAPI

Length of output: 267


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Show AbstractHandlerTestCase contents around its class declaration
echo "== AbstractHandlerTestCase declaration and nearby code =="
sed -n '1,120p' phpunit_tests/Handlers/AbstractHandlerTestCase.php | nl -ba | sed -n '25,120p'

# Find and display ApiTestCase definition
echo "== Locate ApiTestCase =="
fd -i "ApiTestCase.php" phpunit_tests . -a -t f 2>/dev/null || true
ApiFile=$(fd -i "ApiTestCase.php" phpunit_tests . -a -t f 2>/dev/null | head -n 1 || true)
if [[ -n "${ApiFile:-}" ]]; then
  echo "== ApiTestCase found: $ApiFile =="
  sed -n '1,160p' "$ApiFile" | nl -ba | sed -n '1,160p'
else
  echo "ApiTestCase.php not found under phpunit_tests; searching for class ApiTestCase..."
  rg -n "class\s+ApiTestCase\b|abstract\s+class\s+ApiTestCase\b" -S phpunit_tests .
fi

Repository: Liturgical-Calendar/LiturgicalCalendarAPI

Length of output: 204


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== AbstractHandlerTestCase class declaration (and surrounding lines) =="
sed -n '25,70p' phpunit_tests/Handlers/AbstractHandlerTestCase.php

echo "== Search for ApiTestCase in phpunit_tests =="
rg -n "class\s+ApiTestCase\b|abstract\s+class\s+ApiTestCase\b" phpunit_tests || true

echo "== Search for any ApiTestCase extension/usage =="
rg -n "extends\s+.*ApiTestCase\b|ApiTestCase" phpunit_tests/Handlers phpunit_tests || true

Repository: Liturgical-Calendar/LiturgicalCalendarAPI

Length of output: 6331


🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '1,220p' phpunit_tests/Handlers/Ops/HealthHandlerTest.php | nl -ba | head -n 220

Repository: Liturgical-Calendar/LiturgicalCalendarAPI

Length of output: 128


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== HealthHandlerTest.php =="
cat -n phpunit_tests/Handlers/Ops/HealthHandlerTest.php | sed -n '1,260p'

echo
echo "== ApiTestCase.php (key bits) =="
cat -n phpunit_tests/ApiTestCase.php | sed -n '1,220p'

echo
echo "== AbstractHandlerTestCase.php (key bits) =="
cat -n phpunit_tests/Handlers/AbstractHandlerTestCase.php | sed -n '1,260p'

Repository: Liturgical-Calendar/LiturgicalCalendarAPI

Length of output: 26055


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Handler tests extending AbstractHandlerTestCase =="
rg -n --type php "extends\s+AbstractHandlerTestCase" phpunit_tests/Handlers || true

echo
echo "== Handler tests extending ApiTestCase =="
rg -n --type php "extends\s+ApiTestCase" phpunit_tests/Handlers || true

echo
echo "== Total handler test classes =="
rg -n --type php "final class|class" phpunit_tests/Handlers | head -n 200

Repository: Liturgical-Calendar/LiturgicalCalendarAPI

Length of output: 22459


Fix HealthHandlerTest base class to meet the ApiTestCase requirement

  • phpunit_tests/Handlers/Ops/HealthHandlerTest.php extends LiturgicalCalendar\Tests\Handlers\AbstractHandlerTestCase (line 14), but AbstractHandlerTestCase extends PHPUnit\Framework\TestCase (not LiturgicalCalendar\Tests\ApiTestCase), so the phpunit_tests/**/*.php “extend ApiTestCase” requirement isn’t met.
  • HealthHandlerTest is also calling the handler in-process (handle()), with no explicit HTTP-server/route-wiring assertion for /health, so it doesn’t align with the guideline’s “route tests” intent as written.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@phpunit_tests/Handlers/Ops/HealthHandlerTest.php` at line 14,
HealthHandlerTest currently extends AbstractHandlerTestCase (which itself
extends PHPUnit\Framework\TestCase) and invokes the handler in-process via
handle(), violating the project rule that phpunit_tests/**/* must extend
LiturgicalCalendar\Tests\ApiTestCase and that route tests assert server/route
wiring for /health. Change HealthHandlerTest to extend
LiturgicalCalendar\Tests\ApiTestCase instead of AbstractHandlerTestCase, remove
or stop relying on direct handler.handle() invocation, and rewrite the test to
perform an HTTP-level request against the /health route (using the ApiTestCase
client helper or test HTTP client provided by ApiTestCase) asserting the route
is wired and returns the expected status/body; keep references to
HealthHandlerTest, AbstractHandlerTestCase, ApiTestCase, handle(), and the
/health route when making changes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actual convention is:

  • phpunit_tests/Routes/* → ApiTestCase (full HTTP-level, requires running server via Guzzle Client)
  • phpunit_tests/Handlers/* → AbstractHandlerTestCase (in-process handle() invocation) — 14+ files
  • phpunit_tests/Repositories/* → RepositoryTestCase (PG-only) — 6+ files

My HealthHandlerTest correctly extends AbstractHandlerTestCase (mirroring AccessRequestAdminHandlerTest, PermissionAdminHandlerTest, OutboxAdminHandlerTest, etc.).
My HealthOutboxStatsTest correctly extends RepositoryTestCase (mirroring AccessRequestRepositoryTest, etc. — it's a focused test for Health::buildOutboxStats, a
PG-reading static helper).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JohnRDOrazio, you're right — my comment was incorrect. The directory-based convention is clear and well-established across the existing test suite, and HealthHandlerTest correctly follows it by extending AbstractHandlerTestCase. I'll update the long-term memory to reflect the accurate conventions.

(ʕ •ᴥ•ʔ)っ✍️


✏️ Learnings added
Learnt from: JohnRDOrazio
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 631
File: phpunit_tests/Handlers/Ops/HealthHandlerTest.php:14-14
Timestamp: 2026-06-03T14:46:26.868Z
Learning: In the LiturgicalCalendarAPI project, test base classes follow a strict directory-based convention:
- `phpunit_tests/Routes/*` → extend `LiturgicalCalendar\Tests\ApiTestCase` (full HTTP-level tests using a Guzzle client against a running server)
- `phpunit_tests/Handlers/*` → extend `LiturgicalCalendar\Tests\Handlers\AbstractHandlerTestCase` (in-process `handle()` invocation, 14+ files follow this pattern e.g. AccessRequestAdminHandlerTest, PermissionAdminHandlerTest, OutboxAdminHandlerTest, HealthHandlerTest)
- `phpunit_tests/Repositories/*` → extend `LiturgicalCalendar\Tests\Repositories\RepositoryTestCase` (PostgreSQL-only, e.g. AccessRequestRepositoryTest, OutboxRepositoryTest)
- Other test files (e.g. `phpunit_tests/HealthOutboxStatsTest.php`) targeting PG-reading static helpers also extend `RepositoryTestCase`.
Do NOT require `phpunit_tests/Handlers/*` tests to extend `ApiTestCase`; that is only for route-level integration tests in `phpunit_tests/Routes/*`.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


⛔ Learnings removed
Learnt from: CR
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-05-18T13:31:43.156Z
Learning: Applies to phpunit_tests/**/*.php : Create test classes extending `ApiTestCase` in `phpunit_tests/` with HTTP method validation and route tests

🧠 Learnings used
Learnt from: CR
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-05-18T13:31:43.156Z
Learning: Applies to phpunit_tests/**/*.php : Create test classes extending `ApiTestCase` in `phpunit_tests/` with HTTP method validation and route tests

Learnt from: JohnRDOrazio
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 555
File: src/Services/RoleCascadeService.php:126-147
Timestamp: 2026-05-10T12:10:46.459Z
Learning: In `src/Services/RoleCascadeService.php` (LiturgicalCalendarAPI PR `#555`), the `catch (\Throwable $e)` block inside `cascadeTupleRevokeForRole` is intentionally broad: OpenFGA's Write API rejects deletes of non-existent tuples with an error, so the catch makes the cascade idempotent on retry. A blanket `$hadErrors` guard would leave `access_requests` rows stuck in `approved` on any retry where some tuples are already gone. The proper fix requires typed exceptions in `OpenFgaClient` (e.g., `OpenFgaTupleNotFoundException`) first, so benign vs. real failures can be routed differently. This work is tracked in issue/PR `#567` — do not flag the missing guard as a standalone issue without that prerequisite.

Learnt from: JohnRDOrazio
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 555
File: src/Handlers/Admin/AccessRequestAdminHandler.php:237-309
Timestamp: 2026-05-07T01:17:32.516Z
Learning: In `src/Handlers/Admin/AccessRequestAdminHandler.php` (PHP, LiturgicalCalendarAPI), the OpenFGA `writeTuple`/`deleteTuple` calls in `approveRequest()` and `revokeRequest()` are intentionally best-effort: tuple exceptions are caught and collected into `fga_errors` rather than aborting the flow, because `writeTuple` is non-idempotent and "already exists" / "not found" errors are benign in re-approval and revoke scenarios. The correct fix — failing only on real FGA errors — requires SDK error-type/HTTP status differentiation not yet available in `OpenFgaClient`, and is deferred to a follow-up.

Learnt from: CR
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-05-18T13:31:43.156Z
Learning: Applies to phpunit_tests/**/*.php : Use `group slow` annotation for integration tests requiring API calls in PHPUnit tests

Learnt from: JohnRDOrazio
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 349
File: .github/workflows/phpunit.yml:84-101
Timestamp: 2025-09-06T17:19:28.704Z
Learning: JohnRDOrazio prefers to fail early within the same GitHub Actions step rather than using $GITHUB_OUTPUT to communicate between steps, when fixing issues where a workflow might pass without actually running the intended operations.

Learnt from: JohnRDOrazio
Repo: Liturgical-Calendar/LiturgicalCalendarAPI PR: 555
File: src/Repositories/AccessRequestRepository.php:574-579
Timestamp: 2026-05-10T12:10:13.290Z
Learning: In the LiturgicalCalendarAPI project, when reviewing PHP code that reads internal data from the database, do not require null-guards for columns that are declared `NOT NULL` in `scripts/init-db.sql`. Treat database reads as internal/non-boundary data; only add/flag validation or fallbacks at system boundaries (e.g., user input, external APIs). Therefore, missing null-handling for `NOT NULL` database columns should not be flagged as “tech debt.”

* loaded); the PG side is exercised against real rows.
*/
#[CoversClass(Health::class)]
final class HealthOutboxStatsTest extends RepositoryTestCase
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

This test class shape does not match the phpunit_tests guideline contract.

HealthOutboxStatsTest extends RepositoryTestCase and contains helper/repository assertions only. Under the current guideline for phpunit_tests/**/*.php, this should be structured as an ApiTestCase-based class with HTTP method and route coverage (or moved to a location/rule set intended for repository tests).

As per coding guidelines: "phpunit_tests/**/*.php: Create test classes extending ApiTestCase in phpunit_tests/ with HTTP method validation and route tests".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@phpunit_tests/HealthOutboxStatsTest.php` at line 20, HealthOutboxStatsTest
currently extends RepositoryTestCase but per phpunit_tests guidelines it must be
an ApiTestCase-based test that exercises HTTP methods and routes; update the
class declaration to extend ApiTestCase (replace RepositoryTestCase with
ApiTestCase) and add tests that perform HTTP requests against the outbox-related
routes (e.g., GET/POST on the health/outbox endpoints) validating status codes
and response bodies, or alternatively move HealthOutboxStatsTest into the
repository-specific test folder if you intend to keep it as repository-only;
ensure the class name HealthOutboxStatsTest and any helper methods/assertions
are preserved while converting repository assertions into HTTP request
assertions using the ApiTestCase client methods.

Complements the in-process HealthHandlerTest (which extends
AbstractHandlerTestCase per the established convention for handler
tests) with a route-level test that exercises the live HTTP path —
the surface load balancers and uptime checks actually hit.

This test correctly extends ApiTestCase per the established convention
for Routes/* tests, so it hits the running API container via Guzzle and
verifies:
 - GET /health returns 200 + JSON with the expected openfga_outbox shape
   and a valid 'database' value
 - POST /health returns 405 (handler restricts allowedRequestMethods
   to [GET])

Closes the route-wiring assertion gap that the in-process test can't
cover. Local validation against the live container required a rebuild
of liturgicalcalendarfrontend-litcal-api-1 — the previously baked image
was serving Router code from before this branch's /health arm landed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@phpunit_tests/Routes/Readonly/HealthTest.php`:
- Line 19: Add the PHPUnit group annotation for slow integration tests by adding
a docblock with "`@group` slow" above the HealthTest class declaration (final
class HealthTest extends ApiTestCase). Ensure the annotation is placed in the
class-level PHPDoc immediately preceding the class line so PHPUnit picks up the
group for this integration test.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8d33da73-40bf-4169-acd4-70676245c41b

📥 Commits

Reviewing files that changed from the base of the PR and between cc580b5 and 9882a95.

📒 Files selected for processing (1)
  • phpunit_tests/Routes/Readonly/HealthTest.php

* asserting the live HTTP path through the Router + rate-limit middleware
* — the surface load balancers and uptime checks actually hit.
*/
final class HealthTest extends ApiTestCase
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add @group slow annotation per coding guidelines.

This integration test makes HTTP calls and should be annotated with @group slow to allow selective execution. As per coding guidelines: "Use @group slow annotation for integration tests requiring API calls in PHPUnit tests".

📝 Suggested fix
 /**
  * Integration tests for the `/health` route — verifies that the Router
  * wires HealthHandler under the unauthenticated public path and that
  * the response shape monitoring tooling depends on stays intact.
  *
  * The handler's own contract is exercised by HealthHandlerTest (in-process
  * handle() against AbstractHandlerTestCase). This test complements that by
  * asserting the live HTTP path through the Router + rate-limit middleware
  * — the surface load balancers and uptime checks actually hit.
+ *
+ * `@group` slow
  */
 final class HealthTest extends ApiTestCase
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
final class HealthTest extends ApiTestCase
/**
* Integration tests for the `/health` route — verifies that the Router
* wires HealthHandler under the unauthenticated public path and that
* the response shape monitoring tooling depends on stays intact.
*
* The handler's own contract is exercised by HealthHandlerTest (in-process
* handle() against AbstractHandlerTestCase). This test complements that by
* asserting the live HTTP path through the Router + rate-limit middleware
* — the surface load balancers and uptime checks actually hit.
*
* `@group` slow
*/
final class HealthTest extends ApiTestCase
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@phpunit_tests/Routes/Readonly/HealthTest.php` at line 19, Add the PHPUnit
group annotation for slow integration tests by adding a docblock with "`@group`
slow" above the HealthTest class declaration (final class HealthTest extends
ApiTestCase). Ensure the annotation is placed in the class-level PHPDoc
immediately preceding the class line so PHPUnit picks up the group for this
integration test.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The codebase convention is "actually slow runtime," not "any HTTP integration test."
My HealthTest runs in ~100 ms (two HTTP calls):

Time: 00:00.101, Memory: 16.00 MB
OK (2 tests, 14 assertions)

That's faster than many unit tests in this suite, certainly not in the same category as TemporaleTest. Adding @group slow would:

  1. Diverge from the established ApiTestCase-route convention (DecreesTest, EventsTest as peers).
  2. Exclude it from composer test:quick, which is exactly when you want a quick /health route-wiring smoke check.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

JohnRDOrazio and others added 4 commits June 3, 2026 10:44
CI surfaced an ArgumentCountError:
  Too few arguments to function Redis::xclaim(), 5 passed and
  exactly 6 expected.

The phpredis extension version on the CI runner declares xClaim with 6
required parameters (key, group, consumer, min_idle, ids, options) —
the local-dev stubs/Redis.php I added during PR #631 used the 5-arg
form, which masked the issue. Pass an empty options array on the
production call site so both signatures accept it, and widen the stub
to match for type-checker consistency.

Empty []  keeps the default phpredis behavior (no JUSTID/FORCE/IDLE/
RETRYCOUNT options), so the call's runtime semantics are unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit on PR #631 misread the previous testing section as "all
tests extend ApiTestCase", which led to inline review comments asking
for HealthHandlerTest and HealthOutboxStatsTest to switch base classes
— both already followed the established per-layer convention (14+
handler tests use AbstractHandlerTestCase, 6+ repo tests use
RepositoryTestCase).

The misreading was reasonable: the old section listed `ApiTestCase.php:
Base test class with common functionality` without disambiguation,
plus a flat list of test directories. Rewrite the section so the
three-layer convention is the headline, not a footnote you'd have to
infer:

  | Routes/*       | ApiTestCase                  | Full HTTP-level ...
  | Handlers/*     | AbstractHandlerTestCase      | In-process handle() ...
  | Repositories/* | RepositoryTestCase           | PG-only ...
  | Pure-logic     | PHPUnit\Framework\TestCase   | no I/O, extend framework's TestCase

Also clarifies the `@group slow` convention (reserved for tests with
measurable runtime cost — TemporaleTest, RateLimiterTest, schema
validation — NOT a blanket "any HTTP integration test"), and points
at EnvIsolationTrait + the MockHandler-backed OpenFgaClient pattern.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… guard, list filters

Three coverage-driven additions plus one real bug surfaced by them:

* OutboxRepository::list() was binding ':status_val' as '' when no
  status filter was provided. PostgreSQL evaluates the `::outbox_status`
  cast at plan time regardless of the `:status_null OR ...` short-circuit,
  so calling list(status: null, ...) tripped 'invalid input value for
  enum outbox_status: ""'. Bind a valid enum literal ('pending') as the
  placeholder; the per-row OR still short-circuits on :status_null=true.
  Caught by the new list() tests below.

* OutboxRepositoryTest: 4 new tests for list() — no-filter total + rows;
  status filter; access_request_id (metadata->>'access_request_id')
  filter; limit + offset pagination. Lifts the 30-line gap in
  OutboxRepository to near-zero.

* OutboxProcessorTest: 1 new test
  (testProcessOneOnRetryingRowWithFutureNextAttemptIsNoOp) — pins the
  RETRYING + future-next_attempt_at guard added in the earlier
  correctness commit. MockHandler with no queued responses verifies
  no OpenFGA call is attempted; status stays RETRYING; attempts
  doesn't increment.

* BackstopRunnerTest: 1 new test
  (testRunOnceRollsBackAndRethrowsWhenProcessorThrows) — pins the
  try/catch/rollBack path: stubbed processor throws, runner rolls back
  the surrounding tx and rethrows. After the throw, pdo->inTransaction()
  must be false so the next test's TRUNCATE doesn't fail. Uses
  createStub instead of createMock to silence PHPUnit 12's
  "no expectations configured" notice.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nches

Codecov DOM inspection on PR #631 surfaced that the AccessRequestAdminHandler
uncovered lines weren't only Zitadel sync paths (as I had earlier
guessed) — the bulk were:

  - the empty-permissions fast path in approveRequest / revokeRequest
    (lines ~373-390 and ~666-694), which short-circuits before any
    outbox row is built; previously NO test exercised permissions=[]
  - the deferred/failed/both suffixes in approvalMessage and
    revocationMessage (lines ~879-908)

4 new tests:

* testApproveWithEmptyPermissionsTakesFastPathWithNoOutboxRows — POSTs
  approve on a request with permissions=[]; asserts success, no outbox
  rows, DB status='approved'. Pins the fast-path early-return contract.
* testRevokeWithEmptyPermissionsTakesFastPathWithNoOutboxRows —
  symmetric for revoke.
* testApprovalMessageIncludesBothDeferredAndFailedSuffix — sets up two
  permissions, mocks OpenFGA to return 503 (deferred) then 400 with
  validation_error (failed_terminal); asserts message contains both
  "deferred for async delivery" and "failed terminally" sprintf
  branches.
* testRevocationMessageIncludesBothDeferredAndFailedSuffix —
  symmetric for revoke.

Local coverage for src/Handlers/Admin/AccessRequestAdminHandler.php
moves from 70.24% → 79.78%. Bigger remaining gaps (resource-admin
filter path, requireAdminForAllResources for non-global admins,
syncZitadelRole error branches) need either mockable ZitadelService
or resource-admin OIDC fixtures, both of which warrant a follow-up
issue rather than expanding the cleanup PR.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@JohnRDOrazio JohnRDOrazio merged commit d0303b1 into development Jun 3, 2026
10 checks passed
@JohnRDOrazio JohnRDOrazio deleted the feature/openfga-async-reconciliation-design branch June 3, 2026 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant