Repository navigation
fix(scripts): coverage-ratchet sentinel matching immune to prose mentions + write sanity check - #2081
Merged
Merged
Conversation
…ions + write sanity check coverage-ratchet.mjs's sentinel regex was fooled by apps/web/vitest.config.ts's own explanatory comment, which contains the literal substrings `/* ratchet:start */` and `/* ratchet:end */` in a single-line prose comment. The non-greedy regex matched that fake 40-char span instead of the real threshold block: --dry-run reported fabricated zeros for current thresholds, and a real (non-dry-run) invocation would have rewritten the wrong span, corrupting vitest.config.ts into unparseable syntax and breaking every apps/web test. - sentinelRegex now requires the ratchet:start/ratchet:end markers to occupy their own line, so prose mentioning the marker text no longer matches. - Extracted the matching/rewrite logic into scripts/lib/coverage-ratchet-sentinel.mjs so it's directly unit-testable. - Added a node --check write-sanity-check: before any write (dry-run or real), the rewritten config is validated as parseable syntax; a mismatch aborts with a non-zero exit instead of writing corrupted output. - New tests in scripts/__tests__/coverage-ratchet-sentinel.test.ts, wired into scripts/vitest.config.ts's include glob and the CI backfill-script test step. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 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: 6e5096453c
ℹ️ 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".
Self-triggered 8-angle review (no external reviewer available yet) on
the coverage-ratchet fix surfaced two real residual hazards and some
cleanup, all fixed here:
- assertValidSyntax was writing its throwaway syntax-check file into
the repo root (tmpDir=root passed from the caller) instead of the OS
temp dir. If the process were killed between the write and the
finally's cleanup, a stray file would be left in the working tree.
Now uses mkdtempSync(os.tmpdir()) internally; the tmpDir param is
gone. (Independently flagged by 5 of 8 finder agents.)
- matchThresholdBlock now refuses to guess in two ambiguous cases
instead of silently doing the wrong thing:
- More than one well-formed sentinel block in the same config file
throws (which one is real?).
- Marker text present but NOT forming a well-formed block (e.g. an
autoformatter appends a trailing comment to the ratchet:end line)
throws instead of silently falling back to the plain thresholds
match — that fallback is only safe for configs with no sentinel at
all, and applying it to a malformed sentinel reproduces the
original truncation bug on apps/web's per-glob keys.
- Extracted parseThresholds() as its own tested pure function instead
of four inline regex lines duplicated at the call site.
- Simplified indent handling: matchThresholdBlock now returns `indent`
directly (empty string for the non-sentinel case) instead of callers
re-deriving it from `isSentinel ? match[1] : ''`.
- Dropped the speculative `\r?` CRLF tolerance in the regex — this
repo has no CRLF line endings (no .gitattributes eol=crlf, CI runs
ubuntu-latest), so it was untested dead-code generality.
- Small CI comment fix: the "Run backfill script tests" step's comment
said it was "scoped to backfill-*" when it already listed several
non-backfill one-off files by name; reworded to describe the actual
explicit-list mechanism.
4 new tests added (12 total, up from 8) covering the uniqueness guard,
the malformed-marker guard, and parseThresholds.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d .mjs Review feedback from chatgpt-codex-connector (P2): assertValidSyntax wrote the rewritten config to a .mjs file and ran `node --check` on it, which validates plain JavaScript syntax — but all five configs are .ts files. If a future PR added TS-only syntax to any of them (satisfies, a type assertion, a generic), coverage-ratchet.mjs would refuse to ratchet that package forever with a misleading "fails a syntax check" error, even though the config is valid TypeScript that Vitest loads fine. Replaced the node --check subprocess (temp dir + write + spawn + cleanup) with an in-process ts.transpileModule() syntax-only check (no type-checking, no file I/O, no subprocess) using the TypeScript compiler already a devDependency of this workspace. This is strictly better than the previous fix: correctly TS-aware, faster (no subprocess spawn), and has no temp-file lifecycle to get wrong. New test proves the exact scenario the reviewer described: a config using `satisfies`/type assertions is accepted (13 tests total, up from 12). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review @codex review |
Contributor
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20b870c7e3
ℹ️ 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".
Review feedback from chatgpt-codex-connector (P3): apps/web/vitest.config.ts is not forced to LF by .gitattributes, so a Windows/core.autocrlf checkout can have \r\n line endings. The line-anchored sentinel regex required a bare \n after the start marker and only [ \t]* before end-of-line on the end marker, so a CRLF checkout's well-formed sentinel block would fail to match — and the malformed-marker guard added earlier would then treat it as corrupt and refuse to update thresholds, a false failure on an otherwise valid config. Restored \r? tolerance around both line anchors (this had been present in an earlier iteration and was dropped as "unexercised generality" by an internal simplification pass before external review caught the actual portability gap). New test proves a CRLF-normalized copy of the real sentinel fixture still matches (14 tests total, up from 13). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Ran /simplify (4 parallel cleanup agents: reuse, simplification, efficiency, altitude) against the full diff. Reuse, efficiency, and altitude all came back clean (altitude explicitly confirmed the regex+guards+ts.transpileModule approach is proportionate to the problem for this manually-run, git-diff-reviewed internal tool — an AST-based rewrite would be over-engineering here). Simplification found four real items; applied the two cheap, in-scope ones: - matchThresholdBlock scanned the config twice (once via a global regex just for the ambiguity count, once via a plain regex to recover the indent capture group). Replaced with a single matchAll() pass that yields both the count and the capture group. - SENTINEL_REGEX/PLAIN_REGEX were exported with no consumer outside this file (confirmed via grep) — un-exported them. - Simplified a test that re-implemented parseThresholds's regex extraction inline to just call parseThresholds directly. Skipped: dropping the unused `indent: ''` field from the non-sentinel branch — real but marginal (an unread destructured field, not actual complexity), not worth the interface churn. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
2witstudios
added a commit
that referenced
this pull request
Jul 17, 2026
…e, poll-fallback) + notify mentions on interrupted replies (#2097) * test(ai): RED — materializeInterruptedStream must fire mention notifications 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> * test(ai): GREEN — close R.4 coverage gaps + notify mentions on interrupted 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> * test(ai): RED — every terminal write must carry the mention gate exactly 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> * fix(ai): GREEN — mention gate travels with whichever terminal write lands 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> * fix(db): CodeQL — lock key never reaches the format-string position; 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> * fix(db): unlockAndRelease honors its never-throws contract even when 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> * fix(db): releaseQuietly on every advisory-lock exit; JSON.stringify the 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> * test(ai): re-pin start-generation-exclusive to advisory-lock's never-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> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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
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.
Summary
Fixes a reviewer-caught defect from #2075:
scripts/coverage-ratchet.mjs'ssentinel-region regex was fooled by
apps/web/vitest.config.ts's ownexplanatory comment, which contains the literal substrings
/* ratchet:start */and/* ratchet:end */inside a single-line prosecomment. The non-greedy regex matched that fake 40-char span instead of the
real threshold block below it.
Consequences (both verified independently before this fix):
--dry-runreported fabricated current thresholds (0instead of thereal
44/85/56/44) — it exited clean while lying about the underlying state.producing unparseable syntax and breaking every test in
apps/web.This PR went through a self-triggered multi-angle review (8 finder agents),
two rounds of external bot review (CodeRabbit, Codex), and a final
/simplifycleanup pass — all real findings across those passes are fixedhere, across five commits.
Fix
sentinelRegexrequires theratchet:start/ratchet:endmarkers tooccupy their own line (only leading/trailing whitespace besides the
marker, CRLF-tolerant for Windows/
core.autocrlfcheckouts). Real usagealways looks like that; a prose line mentioning the markers has other text
sharing the line and is correctly rejected.
(ambiguous — which one is real?) instead of silently taking the first match.
autoformatter appends a trailing comment to the
ratchet:endline)throws instead of silently falling back to the plain thresholds match —
that fallback is only safe for configs with no sentinel at all.
rewritten config string is validated as parseable TypeScript via
ts.transpileModule()(in-process, no file I/O, no subprocess — correctlyTS-aware, unlike an earlier iteration that ran
node --checkon a renamed.mjsfile and would have wrongly rejected TS-only syntax likesatisfiesor a type assertion, per Codex review feedback).scripts/lib/coverage-ratchet-sentinel.mjs(
matchThresholdBlock,buildThresholdBlock,parseThresholds,assertValidSyntax) so it's directly unit-testable; a final/simplifypass consolidated a double regex scan into one
matchAll()and removedtwo unused exports.
scripts/__tests__/coverage-ratchet-sentinel.test.ts,wired into
scripts/vitest.config.ts's include glob and the CIbackfill-script test step.
Board: D.1 (
arn4y1wscy7w5rptcc40xilg) and leaf 3.9(
lrkb1gu9j3qxtu4eqafsqplo) on E1 PR3 flipped to completed with a note onthe PR page (
osazdx1f2zxo5dmeylyxs0qo).Review status
status at any point).
chatgpt-codex-connector): two real findings across two reviewrounds — a P2 (JS-only syntax check on a
.tsconfig) and a P3 (missingCRLF tolerance) — both fixed and confirmed clean on re-review of the
exact fix commits ("Didn't find any major issues"). Both threads resolved
after reviewer-verified fixes.
repo root instead of OS temp dir) independently confirmed by 5 of 8
finder agents; a follow-up
/simplifypass found four minor cleanups,two of which were applied.
Test plan
bun run typecheck— 16/16 tasks green (verified after every commit)bunx vitest run __tests__/coverage-ratchet-sentinel.test.ts(fromscripts/) — 14/14 passingnode scripts/coverage-ratchet.mjs --dry-runbefore fix: reportedlines: 0 -> 60(fabricated); after fix:lines: 44 -> 60(real)apps/web/vitest.config.ts: minimal diff (only the 4 scalars change,indentation/comments preserved), file still passes
node --checkmaster(no conflicts),mergeStateStatus: CLEAN🤖 Generated with Claude Code
https://claude.ai/code/session_01AEdW8TG2KMWjB5wTf5qB31