test(ai): close R.4 coverage gaps (advisory-lock, checkpoint-serialize, poll-fallback) + notify mentions on interrupted replies#2097
Merged
Conversation
…cations like the finalize path D task st3pyh9q4zwnmae00j195o97: an interrupted reply that @mentions a user currently vanishes without notifying anyone — the normal finalize path (saveMessageToDatabase) fires notifyMentionedUsers behind the route's gate (page.driveId present, conversation explicitly shared, non-empty content), but a materialized interrupted reply never does. Spec: exactly one notification per actually-written materialization, never a duplicate when the CAS upsert skipped (the row's own onFinish already notified), fail closed on missing page/conversation, best-effort + warn on lookup failure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…upted replies
Four items from the E2 PR5 R.4 audit (D.1/D.2/D.3 + the epic-level mention
D task):
1. materializeInterruptedStream now fires notifyMentionedUsers for a
page-chat reply it materializes, mirroring the finalize path's gate
(page.driveId, conversation isShared, non-empty content; stream owner
as triggeredBy, page title as mentioner). The upsert gained .returning
so the notification only fires when the CAS actually wrote — a row
already terminalized by its own onFinish (which already notified) can
never double-page the mentioned user. Best-effort: lookup/notify
failures warn and never un-succeed the materialization. Module stays
at 100% branch (36/36).
2. advisory-lock.ts 90% -> 100% branch: the non-Error unlock-rejection
wrap (pg release(err) destroy contract + operator-visible log) is now
tested. The last uncovered branch was a v8 artifact — the catch->finally
junction of a catch that exits abruptly on every path, unreachable by
any input — so the function was flattened per the epic rail ("a branch
you didn't design; fix the design"): try-lock gets its own catch, the
clientPoisoned flag is gone, unlock/destroy lives in a named helper.
All 7 pre-existing behavior tests pass unchanged.
3. checkpoint-serialize.ts 93.33% -> 100% branch: mismatched/truncated
originRawIndex falls back to survivingFromRawIndex 0 — the under-skip
(self-correcting duplicate) direction, never over-skip (content loss).
4. stream-join-poll-fallback.ts 78.94% -> 100% branch: abort landing at
each await point inside an in-flight tick discards the stale snapshot
and never fires a bogus onNotFound; an abort before the clearInterval
listener registers is caught by the per-tick entry guard; AbortError
rejections with a live signal stay silent but keep polling; a body
with no streams field is terminal, not a crash.
Thresholds are ENFORCED in config: per-glob 100% entries for all three
modules. packages/db's vitest.config.ts gained the #2081 whole-line
ratchet sentinel first — scripts/coverage-ratchet.mjs covers that config,
and its plain-regex fallback would truncate a thresholds block containing
per-glob sub-objects. Verified with coverage-ratchet --dry-run on both
configs.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughChangesAI mention notification delivery
AI stream edge-case coverage
Advisory-lock cleanup
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ChatPOST
participant TerminalPersistence
participant MessageDatabase
participant MentionChannel
ChatPOST->>TerminalPersistence: submit terminal assistant content
TerminalPersistence->>MessageDatabase: save terminal message
MessageDatabase-->>TerminalPersistence: successful write
TerminalPersistence->>MentionChannel: dispatch once for shared conversation
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23ea7bace5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // content.trim() check keeps an empty recovered reply (no parts survived) from paying for | ||
| // two gate lookups that can never produce a mention. Fire-and-forget, like the finalize | ||
| // path's own `void notifyMentionedUsers` — the helper never rejects (it catches and warns). | ||
| if (written.length > 0 && payload.content.trim()) { |
There was a problem hiding this comment.
When a page-chat run reaches the execute-end durable save, the route writes the assistant row at apps/web/src/app/api/ai/chat/route.ts:1520-1528 without mentionNotify; the notification is only attached in the later onFinish save at apps/web/src/app/api/ai/chat/route.ts:1606-1623. If onFinish never runs in that documented gap, this materializer sees the row already no longer streaming, so .returning() is empty and this new written.length > 0 guard skips notifyMentionsBestEffort before settling the session, permanently losing @mention notifications for the recovered reply.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Confirmed and fixed — the root cause was broader than the materializer guard: the route has three writes that can flip the placeholder out of 'streaming' (execute-end, onFinish, and the outer-catch cleanup), and only onFinish carried mentionNotify. So in the documented "onFinish never runs" gap the mention was lost regardless of this PR's guard (and the cleanup write had the same hole).
Fixed in ff7957e (RED) + b64737c (GREEN):
- Route-scoped
mentionNotifyForhelper + once-flag (hoisted out of thetryso the outer-catch cleanup can use it): whichever terminal write lands first carriesmentionNotify, exactly once per request. The flag latches only after a successful save, so a failed execute-end persist still lets onFinish notify. - All three sites now attach the gate; the gate itself is unchanged (driveId + requester + explicitly-shared conversation + non-empty content).
- This makes the materializer's CAS premise true: any row the route flipped was notified by the route, so a
.returning()miss can no longer lose a mention. Its comment now cites the real contract.
Tests: 6 new cases in stream-socket-events.test.ts — execute-end-alone carries the gate (your exact scenario), exactly-one-of-both, failed-execute-end fallback to onFinish, private-conversation negative on every path, and cleanup-write positive + negative.
…tly once (Codex P2) Codex review on PR #2097: the route has three writes that can flip the assistant placeholder out of 'streaming' (execute-end, onFinish, outer-catch cleanup) but only onFinish carried mentionNotify. execute-end's own docblock says "when onFinish never runs, this write stands as the sole record" — in that documented gap the @mention notification was permanently lost, and materialize-interrupted-stream's CAS-gated notify (which assumes "the route flipped it => the route notified") could never recover it. Spec: whichever terminal write lands FIRST carries mentionNotify, exactly once per request; the once-flag latches only on a successful save so a failed execute-end persist still lets onFinish notify; the private- conversation gate holds on every path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ands first (Codex P2) Route-scoped mentionNotifyFor helper + once-flag, hoisted out of the try so the outer-catch cleanup can use it too. All three writes that can flip the assistant placeholder out of 'streaming' (execute-end, onFinish, cleanup) now attach mentionNotify when they are the request's first successful terminal write; the flag latches only after a successful save so a failed execute-end persist still lets onFinish notify. The gate is unchanged (page.driveId + requester + explicitly-shared conversation) plus saveMessageToDatabase's own content.trim() firing condition, so the flag can only latch when a notification would actually have been dispatched. This makes materialize-interrupted-stream's CAS-gated notify premise true: any row the route flipped out of 'streaming' WAS notified by the route, so skipping notification on a CAS miss can no longer lose an @mention in the execute-end-then-death window Codex flagged. Comment there updated to cite the real contract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…review-pass cleanups CodeQL flagged the unlock-failure console.error (js/tainted-format-string high, js/log-injection medium): lock keys can be derived from request- supplied ids, so interpolating one into console.error's first argument would let a crafted key smuggle %-directives or forge log lines. Now a constant format string with the key newline-stripped as a %s argument, pinned by a test (evil\nkey\r%s -> 'evil key %s', never in the format). Review-pass cleanups (self-review finders, behavior pinned by existing tests throughout): - advisory-lock: fn-error/success unlock duplication collapsed to one try/finally (unlockAndRelease never throws, so it cannot mask fn's rejection); still 100% branch. - route.ts: the gate+attach+latch trio, hand-copied at all three terminal writes, now lives in one saveTerminalAssistantMessage helper — a fourth terminal write can't get the exactly-once protocol wrong. Dead page.driveId ternary removed (schema NOT NULL). - materialize-interrupted-stream: the two inline gate selects replaced with the readers the live paths already use — pageRepository.findById (also closes a real gap: a page trashed between stream death and reap no longer pages drive members) and conversationRepository.getConversation (the route's own isConversationShared source, so the two gates can't drift). Test mocks moved to the repository boundary; redundant inner beforeEach deleted. Still 100% branch. - Cross-route mention-gate extraction (v1 completions still builds its gate inline) filed as an epic-level D task (class 4) rather than done as a drive-by. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…release() throws Second self-review pass (finders A/B/C on the final shape; the two correctness finders independently re-confirmed the cross-process duplicate already filed on the epic board): - advisory-lock: if the success-path client.release() throws (release hook, pool shut down), the catch's destroy-release then throws pg's synchronous double-release error, which escaped the catch and replaced a successful fn() result with a rejection inside withAdvisoryLock's finally. The destroy-release is now guarded and logged; two new tests pin the contract (Error and non-Error release throws). Still 100% branch (10 tests). - route.ts: the exactly-once mention latch documented honestly as best-effort — the latch flips only after the save resolves, so overlapping same-process terminal writers and the cross-instance materializer race both resolve to a DUPLICATE ping, never a lost one (the epic's chosen direction; idempotent-notification fix filed as an epic D task). Execute-end comment also names the content source caveat (buffered snapshot vs onFinish's refined message), filed likewise. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@apps/web/src/app/api/ai/chat/__tests__/stream-socket-events.test.ts`:
- Around line 719-747: Add a test covering materializer-first ordering in the
stream socket event suite: simulate the interrupted materialization save
successfully claiming and dispatching mentionNotify before the live route
performs its terminal save, then assert that the combined flow dispatches
mentionNotify exactly once. Keep the existing execute/onFinish coordination
cases unchanged and use the existing save/dispatch mocks and helpers.
In `@apps/web/src/app/api/ai/chat/route.ts`:
- Around line 183-196: Replace the process-local mentionNotified latch in
saveTerminalAssistantMessage with a durable notification claim/outbox shared by
the route and materializer, ensuring only the claimant dispatches the
notification. In apps/web/src/lib/ai/core/materialize-interrupted-stream.ts
lines 208-221, use the same claim instead of relying on message CAS visibility.
Add coverage for materializer-first then route-second ordering in
apps/web/src/app/api/ai/chat/__tests__/stream-socket-events.test.ts lines
719-747 and both race directions in
apps/web/src/lib/ai/core/__tests__/materialize-interrupted-stream.test.ts lines
519-527.
In `@apps/web/src/lib/ai/core/__tests__/materialize-interrupted-stream.test.ts`:
- Around line 519-527: Add a companion test near the existing
materializeInterruptedStream race test that models the materializer successfully
claiming and notifying the row first, then terminalizes it through the route’s
onFinish path. Assert the shared claim prevents the route-side
mockNotifyMentionedUsers dispatch from occurring a second time, covering the
reverse notification ordering.
In `@packages/db/src/advisory-lock.ts`:
- Around line 124-128: Guard client release on both pre-acquisition exits in
withAdvisoryLock, including the try-lock query failure catch and the lock-busy
path. Introduce or reuse a shared release helper that swallows release errors so
these branches always return their promised connection_error or lock_busy
outcomes rather than rejecting, and add coverage for release throwing in both
paths.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72151faa-e464-4316-8732-8219ac476a1e
📒 Files selected for processing (10)
apps/web/src/app/api/ai/chat/__tests__/stream-socket-events.test.tsapps/web/src/app/api/ai/chat/route.tsapps/web/src/lib/ai/core/__tests__/checkpoint-serialize.test.tsapps/web/src/lib/ai/core/__tests__/materialize-interrupted-stream.test.tsapps/web/src/lib/ai/core/__tests__/stream-join-poll-fallback.test.tsapps/web/src/lib/ai/core/materialize-interrupted-stream.tsapps/web/vitest.config.tspackages/db/src/advisory-lock.test.tspackages/db/src/advisory-lock.tspackages/db/vitest.config.ts
…he logged key CodeRabbit review + CodeQL alert 262 on PR #2097: - Every exit of withAdvisoryLock now funnels release() through a shared never-throw releaseQuietly helper — the lock_busy and try-lock-failure paths were still calling release() bare, so a throwing release there replaced the promised resolved outcome with a rejection (inconsistent with the contract the previous commit hardened on the acquired path). Two new tests pin lock_busy and connection_error resolving through a throwing release. 12 tests, still 100% branch. - The logged lock key switches from replace(/[\r\n]/g,' ') to JSON.stringify: CodeQL's log-injection model did not recognize the replace form (alert 262 fired on the sanitized line itself), and JSON escaping is strictly stronger — newlines become literal \n, the constant format string keeps %-directives inert. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…throws release contract CI caught a cross-file regression from 518876d: withAdvisoryLock's release machinery (unlockAndRelease/releaseQuietly) now never throws, so the #2080 double-failure scenario (unlock query rejects + destroy release(err) throws) no longer escapes the finally and reject an already-successful run. The test pinning that old contract failed. The property the test guards is unchanged and still asserted: run() executes exactly once and is never re-invoked unlocked. Updated to also assert the new, stronger outcome — the caller keeps the successful locked result instead of losing an already-started generation to lock-cleanup noise, the destroy is still attempted, and no lock_error degrade telemetry fires. Stale docblock references to the old finally-can-throw behavior refreshed. Audited every other real-advisory-lock consumer: machine-storage-billing branches on the resolved connection_error outcome (23/23 pass); the processor workers use their own pre-helper raw-pg copies (out of scope, unchanged). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
e0c0cdd — CI (Unit Tests) caught a cross-file regression from the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the reviewer-filed coverage findings from #2078 (E2 PR5 R.4 audit) plus one real server gap the same audit surfaced. Board: D.1/D.2/D.3 on the PR5 page + the epic-level mention-notification D task.
1. Mention notifications for materialized interrupted replies (the real bug)
materializeInterruptedStreamnever firednotifyMentionedUsers, so a reply that died mid-stream and @mentioned a user vanished without notifying anyone — the normal finalize path (saveMessageToDatabase) notifies behind the route's gate (page has a driveId, conversation explicitly shared, non-empty content).RED first (commit 1), then GREEN mirroring that exact gate:
notifyMentionsBestEffort) re-derives the route's gate from the DB (the route's in-memorypage/isConversationShareddied with the process): page lookup fordriveId+title(mentioner name), conversation lookup forisShared, missing rows fail closed as private..returning(...); the notification only fires when the compare-and-swap actually wrote. A row that already left'streaming'via its own onFinish (which already notified behind the same gate) can never double-page the mentioned user.materialize-interrupted-stream.tsstays at 100% branch (36/36, up from 23 branches) with 11 new behavioral tests.Review follow-up (Codex P2): the gate travels with whichever terminal write lands first
Codex correctly flagged that the route's execute-end save (whose docblock says "when onFinish never runs, this write stands as the sole record") carried no
mentionNotify— so in that gap the mention was lost and the materializer's CAS guard could never recover it. The root fix (REDff7957e0d, GREENb64737ce6): a route-scopedmentionNotifyForhelper + once-flag shared by all three writes that can flip the placeholder out of'streaming'(execute-end, onFinish, outer-catch cleanup). Whichever lands first carries the gate, exactly once per request; the flag latches only on a successful save so a failed execute-end persist still lets onFinish notify. Six new route tests pin the contract (execute-end alone, exactly-one-of-both, failed-execute-end fallback, private-conversation negative on every path, cleanup positive + negative).2. Coverage gaps closed + thresholds ENFORCED in config
packages/db/src/advisory-lock.tspackages/db/vitest.config.ts)apps/web .../checkpoint-serialize.tsoriginRawIndex[droppedCount] ?? 0)apps/web/vitest.config.ts)apps/web .../stream-join-poll-fallback.tsstreamsfield)apps/web/vitest.config.ts)All new tests assert behavior, not lines:
release(err)destroy contract), still destroy the connection, and stay operator-visible in the log.originRawIndexfalls back tosurvivingFromRawIndex: 0— the under-skip (self-correcting duplicate) direction, never the over-skip (permanent content loss) direction.onNotFound); an abort that lands before the clearInterval listener is registered is caught by the per-tick entry guard; anAbortError-named rejection with a live signal stays silent but keeps polling; a{}body is terminal, not a crash.3. Ratchet-script safety (why packages/db got sentinel markers)
scripts/coverage-ratchet.mjscoverspackages/db/vitest.config.ts, and its plain-regex fallback (thresholds:\s*\{[^}]+\}) would truncate at the per-glob sub-object's first}and corrupt the block. The config now carries the same/* ratchet:start */ … /* ratchet:end */whole-line sentinel #2081 introduced for apps/web;node scripts/coverage-ratchet.mjs --dry-runverified both configs rewrite cleanly with the per-glob keys preserved.4. advisory-lock flatten (why the source changed for a coverage task)
The last uncovered "branch" (line 105) was a v8 artifact: the catch→finally junction of a catch block that exits abruptly on every path — unreachable by any input, so no test could ever cover it. Per the epic's own rail ("threshold too hard = a branch you didn't design; fix the design") the function was flattened: the try-lock query gets its own catch, the
clientPoisonedmutable flag is deleted, and the unlock/destroy logic moved to a namedunlockAndReleasehelper. All 7 pre-existing behavior tests pass unchanged — the contract (connection_error/lock_busy/acquired outcomes, destroy-on-poison, destroy-on-unlock-failure, fn errors propagate unwrapped) is untouched.Review follow-up 2 (CodeQL + self-review pass,
857f88dd6)console.errorinterpolatedlockKeyinto the format-string position; lock keys can derive from request-supplied ids. Now a constant format string with the key newline-stripped as a%sargument, pinned by a test (evil\nkey\r%s→ logged asevil key %s, never in the format).try/finally; the route's gate+attach+latch trio centralized insaveTerminalAssistantMessageso a future terminal write can't break the exactly-once contract; deadpage.driveIdternary removed (schema NOT NULL); the materializer's inline gate selects replaced withpageRepository.findById(also fixes: a page trashed between stream death and reap no longer triggers mention notifications) andconversationRepository.getConversation(the route's ownisConversationSharedsource — the two gates can't drift). Cross-route mention-gate extraction (v1 completions) filed as an epic-level D task rather than done as a drive-by.Known limits (named honestly, filed as epic D tasks; confirmed independently by three self-review finder agents): the exactly-once contract is best-effort. (a) Cross-process: an owner whose heartbeat writes fail 2+ minutes while the process stays alive can be reaped + notified by another instance, then re-notify via its own recovered terminal save (no status CAS on the route's upsert; once-flag is in-memory). (b) Intra-process: the latch flips only after the save resolves (deliberately — latching earlier would lose the mention on save failure), so the outer-catch cleanup racing a still-in-flight execute-end can double-attach. Both windows produce a duplicate ping, never a lost one — the epic's chosen direction; the durable fix (idempotent
createMentionNotificationper user+message) is filed. (c) Notification content is the buffered snapshot the first terminal save persists; refined-content-only mention text (would imply an onChunk forwarding gap) is not re-notified — also filed.Review follow-up 3 (CI regression from the never-throws hardening,
e0c0cdd25)The
518876d12hardening (releaseQuietly on every advisory-lock exit, per CodeRabbit) changed one observable contract: the #2080 double-failure scenario (unlock query rejects and the destroyrelease(err)throws) no longer escapeswithAdvisoryLock's finally — it resolvesacquiredwithfn's result instead of rejecting an already-successful run. CI caught the one consumer test pinning the old shape (start-generation-exclusive.test.ts). Re-pinned to the new contract: the guarded property is unchanged (runexecutes exactly once, never re-invoked unlocked) and the caller now keeps the successful locked result instead of losing an already-started generation to lock-cleanup noise; destroy still attempted; nolock_errordegrade telemetry. Stale docblock references to the old finally-can-throw behavior refreshed. Audited every other real-withAdvisoryLockconsumer:machine-storage-billingbranches on the resolvedconnection_erroroutcome (passes); the processor workers use their own pre-helper raw-pg copies (out of scope, unchanged).Verification
bun run typecheck: 16/16 Turbo tasks green across the monorepopackages/dbfullvitest run --coverage: 421 passed, zero threshold violations (new per-glob gate enforced and passing). 8 failures are pre-existing environmental (database "jono" does not exist— the DB-backed compliance test that CI runs viatest-with-db).apps/webfullvitest run --coverage: 14,585 passed, zero threshold violations with both new per-glob gates enforced. 16 failures across 3 pre-existing environmental files (role "test" does not existin activity-tools + admin-role-version — DB-backed; one timezone-dependent midnight test in messages/grouping) — none import any module this PR touches.node scripts/coverage-ratchet.mjs --dry-runclean on both touched configs (sentinel matched, per-glob keys preserved)🤖 Generated with Claude Code