Security: admin-plane webhook category boundary + conservative offender cleanup (v0.1.25.51, closes #209)

[amavashev]

Closes #209. Security fix (v0.1.25.51): closes the admin-plane CARRIER source for admin-only webhook event_categories, and conservatively cleans up pre-existing offenders.

The carrier source

The 0.1.25.50 fix closed the tenant-plane injection path. But WebhookAdminController.create (POST /v1/admin/webhooks?tenant_id=X) and .update (PATCH) applied no category validation, so an operator / admin-on-behalf-of could place admin-only categories (api_key, policy, webhook, system) on a subscription owned by a concrete tenant X. Since matchesEventType treats categories as an ADDITIVE union, X — which controls that row's endpoint URL and signing secret — then received admin-only governance/security telemetry for its tenant.

(a) Admin write-path validation

• New shared WebhookCategoryBoundaryValidator (@Component) — the single source of truth for the boundary. Both the tenant controller (refactored off its two private methods) and the admin controller call it, so the type-/category-level checks and both planes cannot drift; the boundary derives from EventCategory.isTenantAccessible().
• Admin plane validates only when the target is a concrete tenant (validateForTarget no-ops on __system__). Target tenant: create → the tenant_id param; update → the STORED subscription's tenant_id (WebhookUpdateRequest has no tenant_id; ownership is the subscription's, validated against prior.getTenantId()). Admin-only entry on a non-__system__ target → 400 INVALID_REQUEST.
• __system__ carve-out: admin-only categories remain allowed there (legitimate system-wide monitoring; not tenant-owned).

Governance spec revision v0.1.25.40 (pending, runcycles/cycles-protocol#129) makes this normative.

Capability removal (recorded)

This changes a previously-allowed behavior. 0.1.25.50 explicitly tested/documented admin-plane admin-only categories as 201/200 ("the boundary is tenant-plane-only, not a global tightening"). Verified by grep those two WebhookAdminControllerTest cases were the only place this was asserted — an incidental tested-as-allowed behavior, not a separately documented product feature. Both are rewritten to assert the new 400 (concrete tenant) / 201-200 (__system__). This is an intentional security correction: a tenant-owned row exposes URL + secret to the tenant.

(d2) One-time cleanup — conservative (disable, not strip)

A startup ApplicationRunner (WebhookCategoryBoundaryReconciler) → WebhookRepository.reconcileTenantCategoryBoundary(dryRun). Chosen over a maintenance endpoint because a new admin endpoint would fail the OpenAPI contract-diff check (undocumented surface); mirrors the repo's @Scheduled audit-sweep / CommandLineRunner precedent.

Per non-__system__ row: a row carrying admin-only categories, or a legacy empty-both match-ALL row, is DISABLED — not silently stripped. Per review: a concrete-tenant admin-category row may be a legit-but-misconfigured operator monitoring subscription; silently rewriting its selectors would break it with no signal. Disabling stops delivery immediately (dispatch matches only ACTIVE), is reversible, preserves the row + offending categories for review, and is loudly logged (subscription_id / tenant_id / offending categories + migration hint). Already-DISABLED offenders are skipped → idempotent.

Config: webhook.category-boundary.reconcile-on-startup (default true), webhook.category-boundary.reconcile-dry-run (default false; true = REPORT offenders in logs without mutating, for review before the disabling pass).

Migration (corrected per review — no scope_filter)

Genuine per-tenant admin monitoring must move to a __system__-owned subscription (create with no tenant_id) filtered by event_categories to the admin classes; because __system__ is in every tenant's dispatch union it receives those events for ALL tenants, so select the target tenant client-side on the envelope's tenant_id. Do NOT use a scope_filter: admin lifecycle events are null-scoped (verified: ApiKeyController emits scope=null) and matchesScope excludes null scopes from any non-blank filter, so a scope_filter would deliver none of them. The __system__ row stays operator-owned (URL + secret). gov #129 is being corrected the same way.

Tests

• WebhookCategoryBoundaryValidatorTest — boundary, __system__ carve-out, null no-ops.
• Admin controller — admin-only category/type on a real tenant → 400 (create + update, service never called); on __system__ → 201/200; tenant-accessible → ok.
• Reconciler integration (real Redis, WebhookCategoryBoundaryReconcileIntegrationTest) — seeded mix (admin-category offender, admin-only-category offender, legit, __system__, empty-both, types-only, already-disabled) → correct DISABLED terminal states with categories left intact, non-offenders untouched, dry-run reports without mutating, idempotent re-run.
• Reconciler component unit — flag on/off, dry-run passthrough, failure-swallowed.
• Existing admin/tenant webhook + service tests stay green.

mvn -B verify green — 1,590 tests (192 model + 564 data + 834 api), coverage gates met. Version 0.1.25.50 → 0.1.25.51. README spec-alignment stays governance v0.1.25.39 (.40 / #129 still pending on origin/main).

[amavashev]

Codex REVISE-MAJOR — live bypasses closed (f5f4116)

All findings verified against source before fixing; then the data-module coverage gate (the one that failed at 0.93) was restored.

HIGH

1. Replay bypassed the boundary. WebhookService.replay filtered only by the request's optional type filter + the subscription's scope_filter — never by the subscription's OWN selectors, no status check. Fix: replay now intersects with the subscription's own event_types/event_categories via WebhookRepository.matchesEventType (made public static, shared with live dispatch — spec replayEvents: "all event types the subscription is subscribed to") and is a no-op on a non-ACTIVE subscription (a DISABLED offender receives nothing — also closes minor finding 9). Pre-existing bug; fixed here as a direct bypass of this boundary.
2. Reactivation of unrepaired offenders. A status-only {"status":"ACTIVE"} PATCH passed null selector arrays (per-array validator skipped them). Fix: both controllers validate the effective result (stored ∪ request) — reactivating a concrete-tenant row that still holds admin-only selectors → 400.
3. Reconciler ignored admin-only event TYPES. Fix: offender = admin-only type OR category (DISABLED_ADMIN_SELECTORS, offendingSelectors).
4. __system__ empty-both rows escaped cleanup. Fix: the system carve-out exempts admin selectors only; empty-both is checked for all owners.

MEDIUM

5. Blank tenant_id system exemption. Fix: new WebhookSubscription.isSystemOwner (null/omitted or literal __system__ only; blank = concrete) is the single predicate used by both the validator (api) and reconciler (data) — which also closes finding 6 (validator/reconciler disagreement on null owner).
6. Reconciler robustness. Rewritten SMEMBERS+sequential GET/SET → SSCAN cursor batching (bounded memory) + atomic compare-and-set Lua write (never clobbers a concurrent operator update — a CAS miss is a counted failure, retried) + background daemon thread (startup/readiness never blocked) + exponential-backoff retry on an incomplete pass + loud ERROR alert on exhausted retries. Readiness decision: deliberately do not hard-block readiness on migration completion — a migration bug must not brick the service, the write-path gate already stops NEW offenders, and legacy offenders merely remain DISABLED-pending until a clean pass. reconcileTenantCategoryBoundary now returns ReconcileResult(repaired, failures).

MINOR

8. Migration wording: api_key/webhook/system events are null-scoped (client-side tenant_id filtering is the general solution); policy events carry a real tenant-bounded scope and CAN be scope_filtered.

Tests

Replay-outside-selectors blocked + replay-on-DISABLED no-op; status-only reactivation of an unrepaired offender → 400 (tenant + admin plane; __system__ still reactivatable); admin-only event TYPE on a concrete tenant → 400 (write) and reconciler-flagged; __system__ empty-both disabled; null-owner system-exempt for admin selectors but still empty-both-checked; blank tenant_id create with admin category → 400; reconciler CAS-miss → incomplete+retry, corrupt-row failure, SSCAN multi-batch paging, whole-pass failure incomplete, dry-run report-only, retry-until-complete, give-up-and-alert.

Build / coverage

mvn -B clean verify -Pintegration-tests green — 1,621 tests (192 model + 579 data + 850 api). Data-module unit-job LINE coverage 0.9562 (gate 0.95; was 0.93). The earlier reconciler coverage came only from the Testcontainers integration test, which the CI unit job (-Dtest=!*IntegrationTest) excludes — the new branches are now covered by mocked-Jedis unit tests in WebhookRepositoryTest. #129 (gov v0.1.25.40) merged, so the contract check passes.

[amavashev]

Round 2 — fail-closed dispatch boundary + strip cleanup

Addresses the round-2 REVISE-MAJOR: the boundary was timing-dependent (reconciler is fail-open; live dispatch did no ownership check). Each finding verified against source, then fixed. Full foreground build, all gates green.

HIGH — enforce the boundary AT DELIVERY (fail-closed)

The durable confidentiality guarantee is now a per-event dispatch filter, not the reconciler. WebhookDispatchService takes WebhookCategoryBoundaryValidator and, in both live dispatch and replay (dispatchToSubscription), skips an event when the subscription is concrete-tenant-owned (isSystemOwner == false) and the event is admin-only (isBlockedByOwnershipBoundary, no-I/O on the passed sub). Skip is per event — a mixed subscription still receives its tenant-accessible events (counted boundary_skipped). A concrete-tenant subscription can never receive an admin-only event, independent of stored selectors, reconciler state, or status. dispatchToSubscription now returns boolean so replay counts only real deliveries.

Reconciler action decision — STRIP (not disable)

Because dispatch now blocks the leak, admin-only selectors on a concrete-tenant row are already non-functional. The reconciler STRIPS them (keeping tenant-accessible selectors) so storage matches the already-enforced delivery behavior — no behavior change, no collateral on legitimate deliveries, no signal lost (dispatch, not the ACTIVE flag, is the guarantee). Actions: STRIPPED_ADMIN_SELECTORS; if stripping empties both → STRIPPED_AND_DISABLED; legacy empty-both (any owner) → DISABLED_EMPTY_BOTH. Empty-both → disable fallback retained (separate match-ALL invariant).

Null-owner decision — reconciler MIGRATES null → __system__

A null-owner row was limbo (exempt from repair yet absent from every dispatch index). The reconciler normalizes it to __system__ and SADDs webhooks:__system__ in the same CAS write (NORMALIZED_NULL_OWNER). Replay NPE fixed via isSystemOwner.

MEDIUM

• Bulk RESUME bypass: RESUME reached ACTIVE via webhookService.update directly. Now validates stored (effective) selectors first; an offender fails the row (INVALID_TRANSITION).
• Replay status TOCTOU: dispatchToSubscription re-reads + re-checks ACTIVE at dispatch time, so a subscription disabled concurrently mid-replay stops delivering.

MINOR

• Reconciler moved to a managed single-thread daemon ExecutorService; @PreDestroy shutdown() = shutdownNow() (interrupts in-flight backoff) + awaitTermination(5s).

Docs

CHANGELOG / AUDIT / reconciler + repository javadoc reframed: the dispatch boundary is the security mechanism; the reconciler is best-effort storage hygiene that STRIPS. "DISABLED-pending" framing removed.

Build (foreground, all gates green)

• api unit: 854 tests, coverage met
• data unit: 534 tests, LINE 0.9565 (gate 0.95)
• data integration (Testcontainers Redis): 582 tests
• api integration: 861 tests

Tests added/updated: dispatch-side per-event filtering (live + replay); bulk-RESUME offender blocked + clean row resumes; replay delivered-count + dispatch-time status re-check; reconciler strip / strip-and-disable / normalize-null-owner + daemon shutdown interrupts backoff + exhausted-retry ERROR alert; real-Redis strip/normalize/concurrency/multi-batch(600)/blank-owner + Lua CAS no-clobber.

[amavashev]

Round 3 — replay pagination + boundary/atomicity/honesty hardening

Admin-service scope only. The largest round-3 item (enforcing the boundary in the separate cycles-server-events delivery worker) is handled in that repo by a different change. All findings verified against 2bb2467, fixed, full foreground build green.

P2 — replay cap-then-filter under-delivery

WebhookService.replay fetched ONE page capped at max_events then applied the request-type / subscription-selector / scope_filter filters as post-hoc stream filters — so if the first max_events (default 100) events in [from,to] didn't match but later ones did, replay queued zero. New collectDeliverableReplayEvents pages the window in timestamp-ASC bounded batches (REPLAY_PAGE_SIZE=500) via the EventRepository.list cursor, applies every filter plus the ownership boundary DURING pagination, accumulates up to max_events deliverable events (cap counts deliverable, not scanned), and delivers them chronologically (spec replayEvents). A REPLAY_MAX_SCAN=20_000 ceiling bounds cost on sparse windows and logs (never silently truncates) if hit. Pagination approach + scan bound as requested.

Finding 1 (LOW) — category + unclassifiable

isBlockedByOwnershipBoundary checked only the event TYPE. Event.category is independent (EventService preserves a supplied category). Now blocks if TYPE or CATEGORY is admin-only, and blocks an unclassifiable (both-null) record for a concrete owner. Added isAdminOnly(EventCategory). I/O-free.

Finding 2 (MEDIUM) — normalize atomicity + disabled/unindexed

The null-owner SET (CAS Lua) and SADD webhooks:__system__ were separate commands (partial-failure window; retry never repaired the missing membership), and the blanket DISABLED-skip meant disabled null-owner rows were never normalized. Fix: one CAS_SET_AND_INDEX_LUA folds the conditional SET + SADD atomically; needsIndex is computed via SISMEMBER independent of status — repairs a DISABLED null-owner row, and a system row missing from the index (new INDEXED_SYSTEM_MEMBER action, idempotent SADD). Tested the disabled-null-owner + unindexed-system paths (unit + real Redis).

Finding 3 (MEDIUM) — /test bypass. Decision: DOCUMENTED EXCEPTION

/test POSTs a spec-defined system.webhook_test (admin category) ping directly to the subscription URL. I chose route (b), documented exception, not envelope reclassification, because:

1. the yaml spec is the authority and defines the test event as system.webhook_test — the enum has no tenant-accessible test type, so changing the wire type diverges from the spec; and
2. keeping the type while relabelling the category would create exactly the type/category-inconsistent record finding 1 now blocks.

It is a narrow, explicit exception: owner-triggered synthetic probe, payload {subscription_id, test:true}, no governance telemetry, bypasses the dispatch queue — confidentiality is intact. Documented in code + CHANGELOG + AUDIT. @coordinator: the governance spec needs the /test exception noted spec-side.

Finding 4 (LOW) — honest boolean

createDelivery swallowed an LPUSH failure yet dispatchToSubscription returned true. createDelivery now returns actual enqueue success; dispatchToSubscription and the live dispatch metric propagate it — events_queued counts only real enqueues.

Finding 5 — test hardening

• Real-Redis CAS test now evals the actual production script (WebhookRepository.reconcileCasSetAndIndexScript()), not a hand-copied string.
• 600-row SSCAN test asserts >1 cursor round-trips at the production COUNT.
• Concurrency re-read test asserts the fresh findById re-read drove the skip.
• Added: category/unclassifiable blocking, disabled-null-owner + unindexed-system repair, /test envelope pinning, enqueue-failure → false. Fixed the integration seed helper to mirror production save() (per-tenant index).

Note on rounds 1/2 findings

Report findings 1, 2, 4 (replay status short-circuit + dispatch re-check, strip-not-preserve reconciler, null-safe isSystemOwner) were against the stale round-2 commit and are already resolved in round 3 (2bb2467) — no further action.

Build (foreground, all gates green)

• api unit: 863 tests, coverage met
• data unit: 538 tests, LINE 0.9567 (gate 0.95)
• data integration (Testcontainers Redis): 588 tests
• api integration: 870 tests

[amavashev]

Round 4 — replay pagination cursor bugs → approach B

Classification / reconciler / /test / honest-boolean confirmed clean — no changes there. The remaining issue was entirely in my round-3 replay pagination rewrite. Three cursor bugs on the millisecond-scored event index, all fixed, verified against real Redis.

Approach chosen: (B), and why

I took approach (B): fetch the ordered member-id list up front, then hydrate + filter in batches over that fixed list.

• It is inherently correct for a bounded-window scan — the ZSET (ZRANGEBYSCORE) yields the exact ordered, de-duplicated member set, so there is no score cursor to skip or duplicate, and hydration drops can't look like exhaustion.
• It does not mutate list()'s cursor contract. Caller-impact check: I grepped every list( caller (~30 sites — events/audit/policy/tenant/budget/api-key/balance controllers, AdminOverviewService, EventService). Approach B adds two NEW methods (EventRepository.listEventIdsInRange, hydrateByIds) and leaves list() byte-for-byte unchanged, so no other caller is affected. Approach (A) — extending the cursor to a (score, member) total order — would have been needlessly risky for a replay-only fix.

The three bugs (fixed)

• HIGH — equal-timestamp page boundary skip: list() resumed ASC with minScore = cursorScore + 1, but scores are epoch-milliseconds, so same-ms members past a page end were skipped.
• MEDIUM — partial hydrated page misread as exhaustion: list() drops expired/missing/corrupt/unknown-enum rows, so a < pageSize result did not mean range exhaustion; the collector stopped early.
• MEDIUM — unresolvable cursor duplicates: if zscore(cursor) was null (member gone) the lower bound didn't advance and the same page could repeat until max_events.

Implementation

collectDeliverableReplayEvents pulls one ZRANGEBYSCORE (ascending score then member — chronological, de-duplicated, capped at webhook.replay.max-scan, default 20000), then iterates the fixed id list in 500-item slices, hydrating each via hydrateByIds (drops missing/corrupt/unknown-enum, fail-closed) and applying request-type ∩ subscription selectors ∩ scope_filter ∩ ownership boundary, collecting up to max_events deliverable events, delivered chronologically. The scan ceiling is now a config knob so it's exercisable; hitting it logs a WARN (no silent truncation).

Tests

The mocked paging-mechanics unit tests (which supplied idealized pages and couldn't catch any of these) were removed. New WebhookReplayPaginationIntegrationTest runs against real Redis / the real EventRepository and covers all six required scenarios:

1. 500 events sharing the same millisecond, matches only in the tail → all delivered, none skipped.

2. interior hydration drops (evicted rows) with later matching members → not treated as exhaustion.
3. vanished member → no duplicate delivery.
4. scan-ceiling hit → WARN logged, events_queued reflects the truncation (no silent loss).
5. chronological order preserved across the 500-item page boundary (600 events).
6. max_events counts deliverable-after-filters (interleaved matching / non-matching).

Also added data-module unit tests for listEventIdsInRange (tenant/global/infinity/non-positive-max) and hydrateByIds (drop missing/corrupt, empty), and re-mocked the remaining replay unit tests to the new methods.

Build (foreground, all gates green)

• api unit: 861, coverage met
• data unit: 543, LINE 0.9572 (gate 0.95)
• api integration: 874
• data integration: 593

[amavashev]

Round 5 — strengthen two replay regression tests (test-only)

Production fix confirmed sound; no production changes this round. Two WebhookReplayPaginationIntegrationTest guards didn't actually reproduce the bugs they cover (they'd pass against round-4), so I rewrote them to genuinely exercise the failure condition.

1. Hydration-thinning — now reproduces the "short page misread as exhaustion" bug

Old: 10 members, delete 3 → all 7 survivors hydrate in the first fetch, so round-4 passes too (no distinction).

New (replay_matchesBeyondFirstCandidateWindow_withHydrationDrops_stillDelivered):

• Seeds 1500 non-matching filler at positions 0–1499 (the round-4 list() candidate window was limit*3 = 500*3 = 1500).
• Evicts 1100 interior rows (ZSET members remain) so that first window hydrates to 400 events (< 500 page size).
• Seeds 30 deliverable matches at positions 1500+ (beyond the first window).

Round-4's page.size() < 500 → stop treats the 400-event first page as exhaustion and delivers 0; approach B reads the full ordered id list up front and delivers all 30. Asserting 30 distinguishes the two behaviors. Verified against real Redis.

2. Vanished-cursor — the duplicate class is STRUCTURALLY ELIMINATED

Assessment: approach B fetches the full ordered id list once (ZRANGEBYSCORE) then hydrates in batches — there is no per-page cursor re-query, so the old zscore(cursor)==null → re-emit the same page path no longer exists. The duplicate class is structurally gone, not merely avoided.

New (replay_vanishedMembers_skippedCleanly_noDuplicate_acrossBatches) pins the actual new guarantee at scale:

• 600 members → 2 hydration batches (500 + 100).
• 4 rows evicted across BOTH batches, including at the 500-item batch boundary (evt_0003, evt_0499, evt_0500, evt_0550).
• Asserts each vanished member is cleanly skipped (row null at hydration), no duplicates, and later members (incl. 2nd batch: evt_0551..evt_0599) still delivered → 596 queued, no crash.

The old 5-member/1-row test could not span batches or exercise a re-query path.

The other four scenario tests were left unchanged (codex-clean).

Build (foreground, all gates green)

• api unit: 861 (unchanged — integration test excluded from the unit job)
• data unit: 543, LINE 0.9572 (gate 0.95)
• api integration: 874
• data integration: 593

[amavashev]

External review (codex): SHIP after 6 rounds. Summary of the review arc: round 1 caught replay/reactivation/type bypasses; round 2 the fail-open reconciler timing; round 3 that the actual delivery worker was outside the boundary entirely (→ the separate cycles-server-events#110 last-mile fix); round 4 three replay-pagination cursor bugs (equal-timestamp skip, hydration-thinned-as-exhaustion, vanished-cursor duplicate) → fixed via approach B (fetch ordered id-set up front, hydrate in batches; list() untouched); round 5–6 hardened the regression tests to genuinely reproduce those conditions. Final state — enqueue-side boundary + fail-closed predicate, strip-based reconciler (SSCAN, atomic CAS+index, managed daemon), replay filtering during pagination, /test documented exception, honest enqueue counts. 1,600+ tests, coverage gates met. PARKED ready-to-merge per the hold. Coordinated with cycles-server-events#110 (last-mile delivery boundary) and cycles-protocol#130 (governance v0.1.25.41 / INVARIANT 2). Suggested merge order: spec #130 → admin #210 → events #110, then a coordinated release.

[amavashev]

Reviewer round — over-large replay window is now a caller-visible 400 (un-parks the PR)

Valid P2. Replay capped the candidate scan at replayMaxScan; if matching events existed beyond the ceiling it returned a successful response with events_queued < max_events and only a server-side WARN. The caller reads that as "these are all the matching events in my window" — but the search was truncated and a log isn't caller-visible. (The round-5 ceiling test even codified the silent partial.) Fixed; correctness wins.

Fix — fail-fast, caller-visible

Chosen over a truncation flag (no response-schema change; uses existing error semantics). Precise semantics — only the genuinely-incomplete case fails:

• collectDeliverableReplayEvents fetches replayMaxScan + 1 candidate ids (one past the ceiling) → ceilingReached = ids.size() > replayMaxScan; it scans only the first replayMaxScan.
• ceilingReached AND fewer than max_events deliverable events collected within the scanned set → throw 400 INVALID_REQUEST BEFORE the dispatch loop. The collector runs before any enqueue, so there are no partial side effects; the replay lock is released by replay()'s finally. Message: "replay window too large: it exceeds the replay scan limit of N events; narrow the from/to range (or lower max_events to page within the limit)".
• max_events filled within the ceiling → deliver normally — that's the caller's explicit pagination cap, not truncation (events_queued == max_events signals "there may be more").

Error code chosen

INVALID_REQUEST (400). LIMIT_EXCEEDED is the 429 rate-limit code (wrong status); there is no replay-window-specific code, and INVALID_REQUEST/400 matches the existing webhook validators.

Tests

• Rewrote the ceiling test (which asserted the silent partial) → replay_windowExceedsScanCeiling_incomplete_returns400_nothingEnqueued: candidate count 60 > ceiling 50, max_events 1000 unfilled → asserts 400 INVALID_REQUEST with the "narrow" message, dispatchToSubscription never called (nothing enqueued), and the replay lock released afterward.
• Added replay_windowExceedsCeiling_butMaxEventsFilledWithin_succeeds: candidate 60 > ceiling 50 but max_events 10 filled within → success, 10 delivered, no false failure.
• The six approach-B correctness tests are intact. Ceiling injected via webhook.replay.max-scan (no 20000-row seeding).

Spec finding (for the coordinator to route)

Governance cycles-governance-admin-v0.1.25.yaml replayEvents (line 7096) already declares a 400 "Invalid replay request" response (ErrorResponse schema) — so returning INVALID_REQUEST/400 for an over-large window is within its declared error responses; schema-conformant, no new error-response object needed. What's not documented is the semantic: the spec's LIMITS note only says "Maximum 1000 events per replay request" / "Use from/to to control the replay window" and documents max_events (min 1 / max 1000 / default 100) — nothing about a server-side scan ceiling or the over-large-window 400. Recommend a prose clause in the replayEvents description/LIMITS documenting the over-large-window 400, folded into governance PR #130. (Prose only.)

Build (foreground, all gates green)

• api unit: 861, coverage met
• data unit: 543, LINE 0.9572 (unchanged)
• api integration: 875
• data integration: 593 (unchanged)

[amavashev]

External review (codex): SHIP — scan-ceiling fix confirmed. Replay now fails caller-visibly (400 INVALID_REQUEST, nothing enqueued) when the [from,to] window exceeds the server scan limit and can't be satisfied, instead of silently returning a partial replay; a normal max_events-filled result still succeeds. Governance PR #130 documents the semantic (replayEvents SCAN LIMIT clause). Re-PARKED ready-to-merge per the hold.

[amavashev]

Reviewer round — replay is ALL-OR-NARROW (un-parks the PR)

Valid REVISE-MINOR. The scan-limit fix left a broken claim: it framed max_events as a resumable pagination cap ("lower max_events to page" / "advance from to continue"), but ReplayResponse has no continuation position (replay_id, events_queued, estimated_completion_seconds only). A partial can't be resumed losslessly — distinct timestamps: the caller never learns the last replayed timestamp; same-millisecond events: an inclusive from repeats the page, advancing it skips the rest.

Decision: all-or-narrow (chosen over adding a cursor — no schema change, lossless)

A successful replay delivers EVERY deliverable event in [from,to]; it never returns a partial with an implied "continue".

Final logic (collectDeliverableReplayEvents)

• Fetch replayMaxScan + 1 candidate ids. If ids.size() > replayMaxScan (window not fully scannable) → 400 before any enqueue.
• Else fully scanned: collect all deliverable events (request event_types ∩ subscription selectors ∩ scope ∩ ownership boundary), one past max_events (break when collected > max_events). If collected > max_events → 400 before any enqueue.
• Else (fully scanned, ≤ max_events) → deliver all, chronological.

Both 400s are thrown before the dispatch loop — no partial side effects; the replay lock is released by replay()'s finally. This cleanly replaces the prior "ceiling reached AND < max_events → 400".

Error code + exact messages

INVALID_REQUEST (400) for both:

• scan limit: "replay window too large: it exceeds the replay scan limit of N events; narrow the from/to range"
• too-many-deliverable: "replay window contains more than max_events (N) deliverable events; narrow the from/to range, or raise max_events (up to 1000)"

Removed continuation guidance

The old message tail "(or lower max_events to page within the limit)" and the code framing "events_queued == max_events signals 'there may be more'" / "explicit pagination cap" are gone. The javadoc now states max_events is a window bound, not a resumable cursor.

Tests (all-or-narrow pinned; approach-B correctness kept)

Replaced the two ceiling tests (which encoded the silent partial + a "cap filled within ceiling → success" that is now a scan-limit 400) and the old max_events-cap test. New real-Redis integration tests:

• window candidate count > scan limit → 400 (narrow), nothing enqueued, lock released;
• fully-scanned window > max_events deliverable → 400, nothing enqueued;
• same-millisecond cluster of 15 with max_events 10 → 400, then raising max_events to 20 → one replay delivers all 15 exactly once (the reviewer's same-timestamp concern, handled in the model);
• window ≤ max_events incl. a same-ms cluster → all delivered exactly once, non-matching filtered.

The four approach-B ordering/no-dup correctness tests are intact. Unit (WebhookServiceTest): > max_events → 400, nothing dispatched, lock released; ≤ max_events → all delivered in order.

Spec reconciliation (for the coordinator)

Governance replayEvents (line 7096) already declares a 400 "Invalid replay request" (ErrorResponse) — both 400s are within its declared responses (schema-conformant). The all-or-narrow semantic and the two exact messages above must match the parallel governance PR #130 rewrite — reported here for reconciliation.

Build (foreground, all gates green)

• api unit: 862, coverage met
• data unit: 543, LINE 0.9572 (unchanged)
• api integration: 877
• data integration: 593 (unchanged)

[amavashev]

Doc-only — replay comments aligned to all-or-narrow (zero-survivor sweep)

Codex-confirmed: the all-or-narrow logic is correct; this fixes the P2 doc-only residual (stale pagination-framing comments). No behavior or test change — one local variable rename (page→batch); build green.

Fixed

File / loc	Was	Now
WebhookService.replay (~330)	"collect up to maxEvents … by paging the window … DURING pagination"	all-or-narrow: collect EVERY deliverable event; over-large window → 400 before any enqueue
WebhookService REPLAY_PAGE_SIZE javadoc	"Per-fetch page size while paging the replay window"	"Hydration BATCH size … NOT a pagination unit: the whole window is scanned within one replay"
WebhookService replayMaxScan javadoc	"if this bound is hit … collection stops and logs — NOT a silent truncation"	"if the window's candidate count exceeds this bound … replay returns a 400 INVALID_REQUEST (window too large — narrow from/to), NOT a silent partial"
WebhookService.collectDeliverableReplayEvents local	List<Event> page	List<Event> batch
WebhookDispatchService.isBlockedByOwnershipBoundary javadoc	"apply the SAME predicate while paginating … must not spend its max_events budget … the cap would count"	"when selecting the deliverable events for a window … the deliverable total drives the completeness check"
EventRepository.listEventIdsInRange javadoc	"replay pages hydration over this FIXED id list"	"replay hydrates fixed BATCHES over this whole id list"

Confirmed correct (kept — not stale)

• The all-or-narrow negations in WebhookService (~332/367/402-408: "NOT a resumable pagination cursor", "no continuation position", "never a partial with an implied 'continue'").
• The approach-B contrast in EventRepository (the three OLD list() cursor hazards it AVOIDS — equal-timestamp skip, short-page-as-exhaustion, vanished-cursor duplicate).
• The unrelated cursor-paginated listByTenant / listAll / listDeliveries subscription/delivery-listing APIs (a different feature; list() is untouched), and the continue loop keyword.

No behavior/comment mismatch beyond wording was found.

Build (all gates green)

• api unit: 862, coverage met
• data unit: 543, LINE 0.9572
• model: 192

[amavashev]

External review (codex): SHIP — replay is ALL-OR-NARROW (complete-or-400, no continuation), and the residual pagination-cap comments are corrected (zero survivors). Final: enqueue-side + fail-closed boundary, strip reconciler, replay filtering-during-scan with all-or-narrow completeness (400 + narrow-window on over-large windows, no silent partial). Matches governance #130's replayEvents clause. Re-PARKED ready-to-merge per the hold.

[amavashev]

Reviewer round — replay max_events validation (F3) + best-effort enqueue documented (F1)

Both findings folded into one commit. Still HELD.

F3 (P2) — enforce max_events [1,1000] (was silently clamped)

• Added @Min(1) @Max(1000) (jakarta.validation) to ReplayRequest.max_events (same pattern as WebhookRetryPolicy + @Builder.Default).
• Removed the Math.min(max_events, 1000) silent clamp in WebhookService — out-of-range is now rejected with 400, matching the spec's declared minimum:1/maximum:1000.
• @Valid was ALREADY applied on the replay @RequestBody (WebhookAdminController line 280) — no controller change needed.
• Default behavior: omitting max_events still defaults to 100 (the service's != null ? … : 100 fallback and @Builder.Default = 100); @Min/@Max pass for null, so an omitted value is not rejected.
• Controller tests: max_events=0 → 400 (service not called), max_events=1001 → 400 (service not called), omitted → 202 (validation passes).

F1 (P1) — best-effort enqueue is the INTENDED behavior (confirm + observe, no behavior change)

Per your decision, governance #130 narrows to selection-completeness + best-effort enqueue: events_queued is the accepted count (may be < selected on transient failure); replay is not idempotent. So the current best-effort behavior is now the spec-conformant contract. I made it correct + observable, without changing it to fail-on-partial or atomic:

• Loud WARN when queued < selected (replay_id, subscription_id, tenant_id, selected, queued, shortfall) → a degraded dispatch backend is operator-visible.
• Reframed the replay comments/javadoc + ReplayResponse.eventsQueued doc to state best-effort-with-count is intended, events_queued may be < selected on backend failure, and replay is not idempotent (random delivery IDs → a retry may duplicate).
• New test replay_partialEnqueue_reportsAcceptedCount_andLogsWarn_bestEffort: 3 selected, backend rejects 1 → events_queued=2 and asserts the PARTIAL enqueue WARN with selected=3 queued=2.
• I did not add fail-on-partial / atomic enqueue. Atomic enqueue was not clearly cheaper/better here (per-event dispatch + random delivery IDs), so no stop-and-ask was warranted — flagging that I confirmed the decision rather than reversing it.

Build (foreground, all gates green)

• model: 192
• data unit: 543, LINE 0.9572 (unchanged)
• api unit: 866 (coverage met)
• api integration: 881
• data integration: 593 (unchanged)

[amavashev]

External review (codex): SHIP — max_events now enforces @min(1)/@max(1000) (400 on 0/1001, no silent clamp; omitted defaults to 100), and best-effort enqueue is confirmed intended + observable (partial-enqueue WARN, honest docs, events_queued = accepted count). Matches governance #130's narrowed replay clause. Re-PARKED ready-to-merge per the hold.

[amavashev]

Reviewer round — categorize the replay shortfall (structured dispatch outcome)

Valid P2 (un-parks). The partial-enqueue WARN fired for every events_queued < selected, but dispatchToSubscription returned false for several reasons and only one is a degradation — so a benign concurrent-disable mid-replay raised a false "backend degraded" alarm.

The enum

WebhookDispatchService.dispatchToSubscription now returns a structured DispatchOutcome instead of a boolean (same per-branch behavior, just categorized):

• ENQUEUED — row persisted AND LPUSH'd.
• INACTIVE — subscription paused/disabled/deleted at the dispatch-time status re-check (intended lifecycle; deleted → findById throws → INACTIVE).
• BLOCKED — fail-closed ownership guard at dispatch time (intended security; rare race).
• ENQUEUE_FAILED — real transient backend failure (LPUSH/save failed). Degradation.

Caller changes

The only production caller is WebhookService.replay — live dispatch() calls createDelivery directly and is unchanged (verified by grep). Replay counts ENQUEUED as events_queued and tallies each category.

Categorized shortfall logging

When events_queued < selected:

• any ENQUEUE_FAILED → WARN "DEGRADED dispatch backend" with per-category counts (selected/enqueued/enqueue_failed/inactive/blocked) — a real problem;
• shortfall only INACTIVE/BLOCKED → INFO "intended, NOT degradation" — no false alarm.

(The per-event outer catch — e.g. an unexpected deliveryRepository.save throw — counts as enqueue_failed, a degradation.)

Tests

• Renamed the partial test → replay_partialEnqueue_backendFailure_logsDegradedWarn: one ENQUEUE_FAILED → asserts the DEGRADED WARN (enqueued=2 enqueue_failed=1).
• Added replay_partialEnqueue_concurrentDisable_logsBenignInfo_notDegradedWarn: subscription disabled mid-replay → one INACTIVE → events_queued=2, asserts the benign INFO and asserts NO degraded WARN.
• Updated WebhookDispatchServiceTest (10 assertions boolean→DispatchOutcome): incl. deleted-mid-replay → INACTIVE, concurrent-disable → INACTIVE, LPUSH-fail → ENQUEUE_FAILED, and the admin/inconsistent/unclassifiable cases → BLOCKED (so the BLOCKED path is exercised).
• Updated the replay integration mock → ENQUEUED.

Build (foreground, all gates green)

• model: 192
• data unit: 543, LINE 0.9572 (unchanged)
• api unit: 867 (coverage met)
• api integration: 882
• data integration: 593 (unchanged)

Note for spec reconciliation (#130): events_queued < selected is now attributed to backend failure (WARN) OR intended lifecycle/guard change (INFO) — impl matches the broadened best-effort semantics.

[amavashev]

Codex round — a re-read backend error must not be misclassified as INACTIVE

Valid P2 on the classification I just added. dispatchToSubscription's dispatch-time re-read wrapped findById in a blanket catch (Exception) → INACTIVE, so a real Redis/deserialization outage during the re-read was hidden as a benign lifecycle change → replay emitted the INFO, not the degraded WARN, masking a real failure.

Source verified

WebhookRepository.findById:

• genuinely-missing row → throw GovernanceException.webhookNotFound(id) → ErrorCode.WEBHOOK_NOT_FOUND (404);
• Redis / deserialization / anything else → wrapped in a plain RuntimeException("Failed to find webhook subscription", e).

Not-found signal caught: GovernanceException with ErrorCode.WEBHOOK_NOT_FOUND — the only INACTIVE-worthy findById throw.

Fix

• Catch GovernanceException → INACTIVE only when getErrorCode() == WEBHOOK_NOT_FOUND (genuine delete, intended lifecycle).
• Any other GovernanceException, and any other Exception (Redis/deser/unexpected) → ENQUEUE_FAILED (degradation → degraded WARN), each logged.
• The paused/disabled INACTIVE trigger (non-ACTIVE status on a successful re-read) is unchanged.

Tests

• Renamed the deleted case → dispatchToSubscription_subscriptionDeletedMidReplay_inactive (WEBHOOK_NOT_FOUND → INACTIVE, confirms the genuinely-deleted path still classifies INACTIVE).
• Added dispatchToSubscription_reReadBackendError_enqueueFailed_notInactive — findById throws RuntimeException wrapping JedisConnectionException → ENQUEUE_FAILED (not INACTIVE).
• Added dispatchToSubscription_reReadNonNotFoundGovernanceError_enqueueFailed — a non-not-found GovernanceException → ENQUEUE_FAILED.
• Replay-level effect confirmed by the existing pair: ENQUEUE_FAILED → degraded WARN, INACTIVE → benign INFO (WebhookServiceTest). So a re-read backend failure now produces the degraded WARN.

Build (foreground, all gates green)

• model: 192
• data unit: 543, LINE 0.9572 (unchanged)
• api unit: 869 (coverage met)
• api integration: 884
• data integration: 593 (unchanged)

[amavashev]

External review (codex): SHIP — the DispatchOutcome classification now returns INACTIVE only for a genuine WEBHOOK_NOT_FOUND; Redis/deserialization failures during the dispatch-time re-read map to ENQUEUE_FAILED (degraded WARN), so a real backend outage is no longer hidden as a benign lifecycle stop. Re-PARKED ready-to-merge per the hold.