Guard against a tool gutting a test file, and close #Y30AXB - #2529
Conversation
#Y30AXB asked for two things. The first turned out to be already done. RESTORE: nothing needed restoring. d1485d6 is not an ancestor of main and never was — it lived only on refs/pull/2481/head. PR #2481 squash-merged as e16d5f6, whose diff is two src/components files; tests/ui-smoke.spec.ts is not in the merged diff at all, because the branch's later merge of main (31f4145) took main's blob. Every test case present at d1485d6^ is on main today: 89 -> 90, with one legitimate rename by dc15d0c and one addition by 07df3f6. tests/ui-smoke.spec.ts is therefore deliberately unmodified here. The commit also contained no recoverable intent. Its 4 "insertions" were not code but a file-reading tool's truncation banner written back as file content, including a marker reporting tens of thousands of elided tokens spliced mid-token into what should have been TypeScript. The file did not parse. The work it was reaching for landed separately as 47af581. GUARD: check:diff-integrity, two rules over the diff against the merge base with origin/main (never the previous commit, so several small commits cannot slide under the threshold one at a time). Test-case floor. Cases are counted from the TypeScript AST, not by grepping for test( — a grep counts the RegExp test method and, worse, keeps counting a block of tests after someone comments it out. Measured in aggregate across changed test files (25%) and per surviving file (50%, above a 3-case floor). The aggregate alone flags the ordinary refactor that deletes one spec and adds its replacement; the per-file rule alone misses a suite gutted inside a large PR whose other additions absorb the loss. A deliberate reduction is recorded in diff-integrity.json pinned to exact before/after counts, so an approval cannot quietly cover a later, larger cut. Truncation artefact. No added line, in any file, may carry a tool's truncation banner — the actual signature here, and the half that also covers the same failure landing in src/ rather than a spec. Fails closed: an unresolvable base or unreadable blob is a failure, not a pass. Calibrated on 300 real commits touching tests/: zero flagged. Replayed against d1485d6 it fails all three signals (per-file 89->9, aggregate, and three banner lines), pinned as a test that self-skips where the PR ref is absent. Review fixes folded in from frontend-ui-reviewer: - Curried modifiers (it.runIf(cond)("t"), test.skipIf, test.for) have a call expression for a callee, so they counted as zero. That was live on four real files, including the sole test in tests/guard-push-no-merge-base.test.ts, which could have been regenerated away with no signal. Counts now match. - The per-file floor was 10 cases, so a 10 -> 1 file (90% gutted) passed while an unrelated new spec absorbed it in the aggregate. Lowered to 3; re-swept 300 commits, still zero flagged. - CI now also runs on docs-only diffs, matching verify:pr-local and the documented claim: a truncation banner lands in markdown just as easily. Registered unconditionally in verify:cheap and verify:pr-local, and in CI's static-pr job; gate counts in CLAUDE.md and the gates skill updated to match. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0193brbqFWHbpVnLfgDBkVNP
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_05250b86-97c0-491d-9fca-35d12da52012) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26fe3e0649
ℹ️ 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".
main moved on twice while this branch was open. The only conflict was data/repo-awareness-snapshot.json, a generated file, resolved by regenerating it with npm run docs:update rather than by hand. 569d856 also tiered AGENTS.md into a slim always-loaded core plus reference files under docs/agents/, which left this branch's "Deleting tests, or letting a tool delete them for you" section as the one full-length block in a file that had just been cut in half. Moved it to docs/agents/test-deletion-guard.md behind a heading + pointer stub, matching its immediate neighbour (dead-code-deletion.md) and the pattern that commit established. No wording changed; the text moved verbatim. Gates after the merge: docs:check-index, docs:check-inventory, docs:check-scripts, docs:check-links, check:gate-manifest, check:verification-plan, check:ci-scope, check:diff-integrity and check:repo-awareness-snapshot all pass; typecheck clean; unit suite 948 files / 12,095 passed, 1 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0193brbqFWHbpVnLfgDBkVNP
Codex left two P2 findings on 26fe3e0. Both are real; both are fixed here with a regression test apiece. 1. A rename OUT of a test filename read as unchanged. tests/a.test.ts renamed to src/a.ts keeps every test(...) body, so counting the destination reported 20 -> 20 and passed, while the runner discovers none of them. The after-state is now zero unless the destination is itself a recognised test file; the synthetic 20-case rename now reports 20 -> 0 and fails the aggregate. A rename that stays within test filenames still follows the file, pinned by a second test so the fix cannot overshoot. 2. The CI step was skipped for workflow-only diffs. A workflow-only change sets coverage_changed, ui_changed, static_heavy_changed and docs_changed all false (node scripts/ci-change-scope.mjs --files .github/workflows/ci.yml), so a truncation banner could land in YAML unseen — against rule 2's all-file invariant. The step is now unconditional, matching verify:pr-local's commonScripts, and a contract test asserts it carries no `if:` so the hole cannot be reintroduced. Also merges origin/main, which advanced six commits. Both conflicts were generated files (data/outstanding-issues-snapshot.json, data/repo-awareness-snapshot.json), resolved by regenerating with npm run docs:update rather than by hand. Gates: docs:check-index, docs:check-inventory, docs:check-scripts, docs:check-links, check:gate-manifest, check:verification-plan, check:ci-scope, check:diff-integrity, check:repo-awareness-snapshot, check:outstanding-issues and check:pr-policy all pass. lint clean, typecheck clean, unit suite 948 files / 12,099 passed, 1 skipped. tests/diff-integrity.test.ts now 42 cases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0193brbqFWHbpVnLfgDBkVNP
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_06fb22cf-2d99-4d78-9623-23133c62bed1) |
The single late branch sync AGENTS.md allows, taken after review and fix work was assembled. main was seven commits ahead; git merge-tree confirmed behind-but-clean rather than a conflict before merging, so this is a sync and not a conflict resolution. The two generated snapshots were refreshed with npm run docs:update. Gates: check:repo-awareness-snapshot, check:outstanding-issues, check:diff-integrity, check:gate-manifest, check:pr-policy and docs:check-inventory all pass; typecheck clean; unit suite 948 files / 12,100 passed, 1 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0193brbqFWHbpVnLfgDBkVNP
PR #2553 landed and added scripts, so docs/scripts-index.md conflicted — a generated file, resolved by regenerating with npm run docs:update rather than by hand. package.json auto-merged cleanly; verified check:diff-integrity is still registered in verify:cheap:internal after that merge, since a silently dropped registration is exactly the kind of thing this branch exists to stop. Gates: docs:check-inventory, docs:check-scripts, docs:check-index, docs:check-links, check:gate-manifest, check:verification-plan, check:diff-integrity, check:repo-awareness-snapshot, check:outstanding-issues, check:pr-policy and check:ci-scope all pass; typecheck clean; unit suite 949 files / 12,158 passed, 1 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0193brbqFWHbpVnLfgDBkVNP
c0ee4dd ("fix(snapshots): stop the two generated snapshots conflicting on every PR") is the fix for the churn this branch kept hitting: four separate conflicts today, every one of them in a file the repo generates rather than in real code. Took main's versions of all three generated files and regenerated with its new tooling, so this branch now carries that fix and should stop colliding on every base advance. Re-verified that check:diff-integrity survived the package.json auto-merge and is still in verify:cheap:internal, since a registration dropped by an automatic merge is exactly the silent loss this branch exists to prevent. Gates: docs:check-inventory, docs:check-scripts, docs:check-index, docs:check-links, check:gate-manifest, check:verification-plan, check:diff-integrity, check:repo-awareness-snapshot, check:outstanding-issues, check:outstanding-issues-snapshot, check:pr-policy and check:ci-scope all pass; typecheck clean; unit suite 949 files / 12,168 passed, 1 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0193brbqFWHbpVnLfgDBkVNP
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_52bdddf2-75c9-4d62-80ec-ff1e6d0c85b5) |
main advanced two more commits (#2520, #2526). One conflict, again in the generated data/repo-awareness-snapshot.json, resolved by regenerating with npm run docs:update rather than by hand. Re-checked that check:diff-integrity survived the merge and is still in verify:cheap:internal. Gates: docs:check-inventory, docs:check-scripts, docs:check-index, docs:check-links, check:gate-manifest, check:verification-plan, check:diff-integrity, check:repo-awareness-snapshot, check:outstanding-issues, check:outstanding-issues-snapshot, check:pr-policy and check:ci-scope pass; lint clean, typecheck clean, unit suite 949 files / 12,171 passed, 1 skipped. check:ledger-write-discipline is re-run below, since it refuses a verdict while the merge is uncommitted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0193brbqFWHbpVnLfgDBkVNP
Behind-but-clean, confirmed with git merge-tree before merging. Auto-merge is armed on this PR and requires an up-to-date branch, so this sync is what lets it land. Gates: check:diff-integrity, check:repo-awareness-snapshot, check:outstanding-issues, check:outstanding-issues-snapshot, check:gate-manifest, check:pr-policy, docs:check-inventory and docs:check-links pass; typecheck clean; unit suite 949 files / 12,186 passed, 1 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0193brbqFWHbpVnLfgDBkVNP
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_6a263e9d-65a9-4a32-9c0f-08028f70fa59) |
…ke-jfiygz # Conflicts: # docs/scripts-index.md
…ke-jfiygz # Conflicts: # data/repo-awareness-snapshot.json
Summary
Closes
#Y30AXB, which asked for two things. The first turned out to be already done, and proving that was most of the work.d1485d6e8is not an ancestor ofmainand never was — it lived only onrefs/pull/2481/head. PR fix(ui/answer): keep answer-surface counts neutral and the safety tile amber #2481 squash-merged ase16d5f66d, whose diff is twosrc/componentsfiles;tests/ui-smoke.spec.tsis not in the merged diff at all, because the branch's later merge ofmain(31f41450a) tookmain's blob. Verified by blob hash and by full test-title comparison: every one of the 89 test cases present atd1485d6e8^is onmaintoday (now 90), with one legitimate rename bydc15d0c69and one addition by07df3f6d5.tests/ui-smoke.spec.tsis therefore deliberately unmodified in this PR. Two independent agents reached this conclusion separately.47af5819c.npm run check:diff-integrity(scripts/check-diff-integrity.mjs,diff-integrity.json), the remedy#Y30AXBasked for. Two rules over the diff against the merge base withorigin/main— never the previous commit, so removing tests across several small commits is still measured as the whole drop.test(: a grep counts theRegExptest method and, worse, keeps counting a block of tests after someone comments it out. Measured in aggregate across changed test files (25%) and per surviving file (50%, above a 3-case floor). The aggregate alone flags the ordinary refactor that deletes one spec and adds its replacement; the per-file rule alone misses a suite gutted inside a large PR whose other additions absorb the loss.src/rather than a spec.{path, before, after, reason, approvedOn}entry indiff-integrity.json, pinned to both counts so an approval cannot quietly cover a later, larger cut. Fails closed on an unresolvable base or unreadable blob.verify:cheapandverify:pr-local, plus astatic-prstep in CI; gate counts inCLAUDE.md(34 → 35) and.claude/skills/gates/SKILL.md(37 → 38) updated socheck:gate-manifeststays satisfied. The gate's rationale lives indocs/agents/test-deletion-guard.mdbehind a pointer stub inAGENTS.md— see the merge note below.donerequest for#Y30AXBunderdocs/outstanding-issues-inbox/; runnpm run issues:reconcileafter this lands.Evidence the gate works, not just that it runs:
d1485d6e8through the script's own injectable seams: fails all three signals (per-file 89 → 9 at 89.9%, aggregate, and three banner lines). Pinned as a test that self-skips where the PR head ref is absent, as it will be in CI.tests/: zero flagged, before and after the threshold tightening below.#Y30AXBledger write-up quoted the truncation banner verbatim. I reworded the prose rather than widening the exemption list.Review fixes folded in before the first push, from a
frontend-ui-reviewerpass:it.runIf(cond)("t"),test.skipIf,test.for) have a call expression for a callee and counted as zero. That was live on four real files, including the sole test intests/guard-push-no-merge-base.test.ts— which could have been regenerated away with no signal at all. Counts now match reality (1, 6, 24, 35).verify:pr-localand the documented claim — a truncation banner lands in markdown just as easily as in a spec.Merge note (
4fc452691)mainadvanced twice while this branch was open. The only conflict wasdata/repo-awareness-snapshot.json, a generated file, resolved by regenerating it withnpm run docs:updaterather than by hand.569d85658also tieredAGENTS.mdinto a slim always-loaded core plus reference files underdocs/agents/, which left this branch's new section as the one full-length block in a file that had just been cut in half — partially undoing that commit's intent. It moved verbatim todocs/agents/test-deletion-guard.mdbehind a heading + pointer stub, matching its immediate neighbourdead-code-deletion.md. No wording changed.Verification
npm run verify:cheap(pre-merge head) — all 35 static gates green,lintrecorded a pass,typecheckclean, unit suite 948 files / 12,093 passed, 1 skipped, exit 0. Two runs before that failed for real reasons and were fixed:check:ledger-write-disciplinerefused a verdict while the inbox request was untracked, andtypecheckrejected aNodeJS.ProcessEnvJSDoc type in the new test.4fc452691—docs:check-index,docs:check-inventory,docs:check-scripts,docs:check-links,check:gate-manifest,check:verification-plan,check:ci-scope,check:diff-integrityandcheck:repo-awareness-snapshotall pass;typecheckclean; unit suite 948 files / 12,095 passed, 1 skipped, exit 0.npm run check:gate-manifest—all 38 verify:cheap gates are enforced in CI (static-pr + mapped jobs), and the 35 static gates are documented consistentlynpm run check:diff-integrityon this diff —PASS; self-test 23/23tests/diff-integrity.test.ts— 38 unit tests passingtests/ui-smoke.spec.tsundernpm run test:e2e --project=chromium: 105 tests collected, 103 passed, 2 failed (3.9m). The two failures aredocument frame stretches the canvas owner at phone and desktopanddocument viewer puts the PDF preview first with pinned evidence after it on mobile— both PDF/document-viewer visibility assertions. This container's Chromium isheadless_shellrev 1194 against the repo's pinned 1234 (cdn.playwright.devis blocked by this environment's network policy, so the pinned build cannot be fetched here;#255drift). This PR touches no product code and noui-smokecode, so it cannot have caused them. Not independently reproduced on the base commit — the browser is the only variable, so hosted CI is the authority.PR policy— failed on the first ready-for-review run because this body lacked the section below. Reproduced offline againstevaluatePullRequestPolicywith this PR's exact file list (ok: false, "Clinical-risk paths require the## Clinical Governance Preflightsection."), then re-evaluated with the section added:ok: true, zero errors. Body-only fix.PR mergeability— failed becausemainadvanced; confirmed a real conflict withgit merge-treerather than assuming staleness, then resolved as described in the merge note.npm run verify:ui— not run: superseded by the direct full-fileui-smokerun above, and blocked from being authoritative here by the same browser-revision drift.npm run verify:release— not run; no release or handoff confidence claimed.Verification not run:
check:production-readiness,eval:*, and every other provider-backed gate. This change touches no clinical, ingestion, answer-generation, privacy, Supabase, or deployment behaviour, and provider-backed gates were not authorized.Risk and rollout
verify:cheapand CI'sstatic-pr, so a false positive blocks unrelated PRs. Mitigated by calibrating against 300 real commits (zero flagged) and by thediff-integrity.jsonapproval escape hatch, which is a reviewed one-line entry rather than a threshold edit. The gate reads git and parses source; it writes nothing and touches no provider.check:diff-integrityfromverify:cheap:internaland thestatic-prstep, and restoring the two documented gate counts, fully disarms it.Clinical Governance Preflight
Required because
data/outstanding-issues-snapshot.jsonanddata/repo-awareness-snapshot.jsonare underdata/, whichclassifyPullRequestFilestreats as clinical-risk. Both are generated files, refreshed bynpm run docs:updatebecause this PR adds a script and a ledger inbox request. Neither carries clinical content, and no clinical behaviour is touched by this change.Clinical KB Database(sjrfecxgysukkwxsowpy)Every item above is affirmed as unchanged rather than re-established: this PR adds a static verification gate plus documentation, and touches no ingestion, retrieval, answer-generation, source-rendering, document-access, privacy, or deployment code path.
The policy also emits one warning, deliberately not acted on: operational-risk paths (
package.json,.github/workflows/ci.yml) are bundled with the clinical-classifieddata/snapshots. Splitting is not practical here — the snapshots are regenerated because of the script and ledger changes, so they cannot be landed independently of them.Notes
Known limitation, deliberately accepted and documented in
docs/agents/test-deletion-guard.md: atest()inside afor (const viewport of …)loop counts once, not once per iteration, so shrinking that loop's array loses real cases without moving the count.ui-smokehas 6 such loops (7 declarations → 22 runtime cases; 90 declarations → 105 collected). Any static count has this hole, the aggregate and truncation rules are unaffected, and closing it would mean evaluating the spec rather than parsing it.Not done, and offered rather than assumed: the gate is not registered as a seventh guard in
scripts/guard-push.mjs. That layer would catch the failure earliest, but the repo's own notes record that an agent pushing from its own environment bypassescore.hooksPathentirely — which is exactly the#Y30AXBscenario — so CI is the load-bearing layer. Happy to add it if you want the belt as well as the braces.The
#Y30AXBrow's premise that "a gutted suite goes green trivially" did not hold for this particular corruption, since the file did not parse and CI would have errored on load. It does hold for any truncation that happens to leave valid syntax, which is what this gate is calibrated for.🤖 Generated with Claude Code
https://claude.ai/code/session_0193brbqFWHbpVnLfgDBkVNP
Note
Medium Risk
Adds a blocking gate on every PR and
verify:cheap; false positives would block merges, though calibration anddiff-integrity.jsonapprovals are meant to limit that. No runtime or clinical behaviour changes.Overview
Adds
npm run check:diff-integrityto block silent test-suite destruction and tool truncation banners committed as source (incident#Y30AXB). The checker compares the branch to the merge base withorigin/mainand applies two rules: an AST-based test-case floor (aggregate 25% / per-file 50% above a 3-case minimum) and a truncation-artefact scan on added lines in any changed file.Wiring: new
scripts/check-diff-integrity.mjs, config indiff-integrity.json(thresholds + pinnedapprovedReductions), Vitest coverage intests/diff-integrity.test.ts. The gate is unconditional inverify:cheap,verify:pr-local’s common scripts, and CIstatic-pr(DIFF_INTEGRITY_BASE_SHA). Agent docs adddocs/agents/test-deletion-guard.mdand anAGENTS.mdpointer; gate counts move to 35 / 38 inCLAUDE.mdand the gates skill.Housekeeping: marks
#Y30AXBdone in the outstanding-issues inbox and refreshesdata/repo-awareness-snapshot.json.Reviewed by Cursor Bugbot for commit 902d967. Configure here.