fix(guard,test): check the pushed commit, and resolve band adoption by reachability - #1400
Conversation
Three CI failures on 2026-07-30 came from the same cause — an unformatted file reaching Static PR checks and ci/circleci: verify, which both run prettier --check . Each time the author had run the unit suite, typecheck, lint, and a focused prettier check on the file they were working on, and reported green. The loop cannot catch it: format:check is in none of test, typecheck or lint. A format-before-push guard already exists in .githooks/pre-push, and it works — but core.hooksPath is configured by this checkout's npm install, so an agent pushing from its own environment never runs it. Hooks are client-side, so this cannot be fixed for other environments in tooling; the instruction surface is the only lever. Records the specific trap as well as the rule: the missed file was twice a doc or ledger edit made alongside a source change, which a per-file prettier check on the source file does not cover. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 8 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR strengthens pre-push formatting verification by checking pushed commit contents in temporary worktrees and refines route reachability analysis with Babel AST parsing and mount-aware import traversal. ChangesPushed-commit format verification
AST-based route reachability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant formatGuard
participant checkPushedCommit
participant GitWorktree
participant Prettier
formatGuard->>checkPushedCommit: check commit SHA and changed files
checkPushedCommit->>GitWorktree: create detached worktree
checkPushedCommit->>Prettier: run formatting checks
Prettier-->>checkPushedCommit: return check result
checkPushedCommit->>GitWorktree: remove temporary worktree
checkPushedCommit-->>formatGuard: return verdict
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@cursoragent fix merge conflict |
|
Resolved and pushed on tip ClassificationSimple — no real content conflict. Only
No product-code conflicts. Resolution
Complicated conflictsNone.   |
…ign-mockups-w53znc Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
![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: 9b7cec2308
ℹ️ 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".
…ng it Codex found my own instruction incomplete, and in a way that could mislead: a push sends commits, not the working tree, so running the formatter after committing leaves the unformatted blob on the branch. Worse, guard-push.mjs runs prettier --check against working-tree paths, so it reports clean while the pushed commit is still unformatted. Proved it in a scratch repo — commit `const a = 1`, format, and the working tree reads `const a = 1;` while `git show HEAD:f.ts` still has the original; --check on the path passes, --check on the committed blob fails. A green guard is therefore not evidence. Rewrote the bullet to require committing the formatter output and to name both traps explicitly: that one, and the per-file check that passes on the source file you edited while a doc or ledger edit in the same push fails — which was the missed file in two of the three CI breaks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
|
Re: "Require committing formatter output before pushing" — correct, and it made my own instruction actively misleading. Fixed in I proved the mechanism in a scratch repo rather than reasoning about it. Commit
So the guard reports clean while the pushed commit is unformatted. That is worse than my bullet simply being incomplete — anyone following "run Rewritten to require committing the formatter output, with both traps named:
Worth separating what's mine from what's the tooling's: the wording was my error and is fixed here. The guard checking working-tree paths instead of the pushed blobs is a real weakness in Verification: Generated by Claude Code |
Two gaps, both proven before fixing and both re-proven after. guard-push's format guard ran prettier against working-tree paths, but a push sends commits. Commit `const a = 1`, run npm run format, and --check on the path passed while git show HEAD:<file> was still unformatted — the guard went green and CI failed. It now reads the pushed blob via git show <sha>:<file>, fed through --stdin-filepath so Prettier still picks the parser from the real path and still honours .prettierignore for it. Verified: an unformatted commit with a formatted working tree now exits 1, a formatted commit exits 0, and SKIP_FORMAT_GUARD=1 still overrides. Unknown verdicts (deleted blob, no parser) stay fail-open so nothing new can block a push. The band adoption gate followed every import specifier, so a route that kept its results import but stopped rendering it still counted as adoption — the exact regression the gate exists to catch. It now parses with @babel/parser and follows a static import only when the binding is mounted: JSX element, default re-export, or named re-export. import(), export-from and side-effect imports stay unconditional, since each is a mount mechanism rather than a binding that can go unused, and the lazy form is the only route to the code-split dashboard workspaces. Verified by gutting (search-app)/services/page.tsx to <div /> with its imports intact: previously green, now an orphan; restored, it passes. Pinned by temp-dir fixtures for imported-but-unrendered, default re-export, and lazy import. Closes #115. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0647aea97
ℹ️ 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".
Codex found me over-applying fail-open. A malformed prettier config in the push makes Prettier exit non-zero; my catch returned null and formatGuard only blocked on an explicit false, so a config error let the push through while CI's prettier --check . failed for the same reason. That is the guard waving through exactly the break it exists to catch. Three verdicts now, each verified in its own scratch repo so a stray config could not confound the others: unformatted commit -> exit 1, "found unformatted files" formatted commit -> exit 0, silent malformed config -> exit 1, "could not check this push" Fail-open is now reserved for a blob that is absent at the pushed sha — deleted in this push, so CI has nothing to check either. --ignore-unknown still covers the benign no-parser case, so any other non-zero exit is a real problem and is surfaced with Prettier's own stderr. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc166c8fb2
ℹ️ 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".
A page that re-exported a banded component while its own default rendered
<div /> reported adoption: the walker treated `export { Banded }` as a mount
and followed the import. Re-exporting renders nothing, so drop that branch —
wherever the re-export is finally mounted, the walk sees the JSX there.
Pinned by a fixture that fails against the previous behaviour.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 055df85698
ℹ️ 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".
Resolves the docs/outstanding-issues.md conflict: main re-padded the whole table and extended #109, so the conflict spanned all 58 rows while the only semantic difference was this branch closing #115. Took main's block and kept this branch's #115 row. The conflict is why no CI ran here: GitHub cannot build refs/pull/1400/merge for an unmergeable PR, so every pull_request-triggered workflow (CI, Gitleaks, Semgrep) was skipped while pull_request_target ones still ran, and CircleCI failed 3 seconds after each push. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25102a4d6a
ℹ️ 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".
Two review findings, both about working-tree state leaking into the verdict: - A dynamic `prettier.config.mjs` could not be evaluated by the previous hand-staged config approach, so the guard fell back to the working-tree config — recreating the mismatch it exists to prevent. - A push that changes prettier policy (.prettierrc, .prettierignore, .editorconfig, package.json) changes the verdict for files the push never touched, which a changed-paths-only check cannot see. Both are answered by checking the pushed commit where CI checks it: a `git worktree` of that sha (<1s here) with node_modules linked in, so a dynamic config loads its plugins. A policy change escalates to a whole-tree `prettier --check .`; otherwise only the changed paths are checked. Failure to create the worktree now fails closed — being unable to check is not evidence the push is clean. Verified on six isolated scratch repos, exit codes 1/0/1/1/1/0: committed unformatted with a clean working tree blocks; all-clean passes; a committed-only broken static config blocks; a committed-only broken dynamic config blocks (was passing); a config-only change that breaks an untouched file blocks (was passing); a benign config change still passes. Also records #116: an unmergeable PR runs no CI at all and says nothing, which is what cost this branch three pushes of missing checks today. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
Second conflict in docs/outstanding-issues.md in under an hour, same shape: main re-padded the table so the hunk spanned every row, while only two rows differed. Took main's #108 (completed by #1403) and kept this branch's #115 closure. #116 does not collide — main's next-id was still 116. This recurrence is the case #116 itself describes: while conflicted, the PR ran no CI at all and nothing said so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20235ccba1
ℹ️ 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".
…able Every open PR sampled carries the same ci/circleci: verify failure, including #1400 which is a docs-only AGENTS.md change, so it is not branch-specific. The job's whole contents were mirrored locally on #1396's tip and all of it is green, including the PyMuPDF-gated PDF tests under a venv built exactly as .circleci/config.yml builds it. That puts the fault in the job environment. Recorded with the quota hypothesis marked explicitly unverified: the CircleCI project is private, no session has a CircleCI token, and the unauthenticated API returns "Build not found". Reading the failing step needs an operator. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
scripts/guard-push.mjs (1)
201-214: 🚀 Performance & Scalability | 🔵 TrivialAny
package.jsonedit anywhere in the tree triggers a full-repo prettier check.Matching purely on basename means a dependency bump in an unrelated
package.json(with no"prettier"field) escalates to[["."]]— a repository-wideprettier --check .— even though it can't actually change any other file's formatting verdict. This adds latency to the common case of routine dependency edits.♻️ Only escalate when the package.json actually carries prettier config
-function isPrettierPolicyFile(file) { - return /^(?:\.prettierrc(?:\..+)?|prettier\.config\.(?:js|cjs|mjs|ts)|\.prettierignore|\.editorconfig|package\.json)$/.test( - path.basename(file), - ); -} +function isPrettierPolicyFile(file, dir) { + const base = path.basename(file); + if (/^(?:\.prettierrc(?:\..+)?|prettier\.config\.(?:js|cjs|mjs|ts)|\.prettierignore|\.editorconfig)$/.test(base)) { + return true; + } + if (base !== "package.json") return false; + try { + const contents = readFileSync(path.join(dir, file), "utf8"); + return JSON.parse(contents).prettier !== undefined; + } catch { + return true; // fail closed if it can't be read/parsed + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/guard-push.mjs` around lines 201 - 214, Update isPrettierPolicyFile so package.json matches only when that file contains a Prettier configuration field; retain unconditional matching for the dedicated Prettier and editor configuration filenames. Ensure unrelated package.json edits no longer trigger whole-tree formatting checks.
🤖 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 `@AGENTS.md`:
- Around line 169-174: Update the guard-push.mjs behavior description in
AGENTS.md to reflect that checkPushedCommit verifies the pushed SHA in a
temporary git worktree with Prettier, rather than checking working-tree paths.
Remove the stale claim that the guard can pass while the pushed commit remains
unformatted.
In `@docs/outstanding-issues.md`:
- Line 155: The `#115` summary and detail contradict each other about named
re-export reachability. Update the summary text in the issue record to match the
tested behavior described in the detail: do not follow named or star re-exports,
while retaining default re-exports and lazy import behavior.
- Line 155: Restore the seven-column Markdown table structure for the `#115` row
in the outstanding-issues ledger by inserting the authoritative source value
between the detailed status text and the final 2026-07-30 date. Preserve the
existing issue number, priority, status, descriptions, and date.
- Line 152: Update the verification gate for docs/outstanding-issues.md to
enforce duplicate-ID uniqueness within the records table only, allowing
intentional IDs shared with the execution queue. Separately validate that queue
references correspond to valid records and that the marker is greater than the
maximum record ID; keep the document as the single durable ledger.
---
Nitpick comments:
In `@scripts/guard-push.mjs`:
- Around line 201-214: Update isPrettierPolicyFile so package.json matches only
when that file contains a Prettier configuration field; retain unconditional
matching for the dedicated Prettier and editor configuration filenames. Ensure
unrelated package.json edits no longer trigger whole-tree formatting checks.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f2f78c14-fbcd-4a9b-abb9-480a32b53424
📒 Files selected for processing (5)
AGENTS.mddocs/branch-review-ledger.mddocs/outstanding-issues.mdscripts/guard-push.mjstests/search-results-band-adoption.test.ts
Six false greens were reported on the adoption gate in one day — an unrendered
import, `export { X }`, `export { X } from`, `export *`, a bare side-effect
import, JSX inside an unmounted helper, and a lazy import reaching every
sibling export of the same module. They are one defect: the walk asked whether
a file mentions the band, when the question is whether anything the route
mounts reaches it. Two of the six were introduced by an earlier patch to this
same walker, so this replaces the heuristic rather than adding a seventh case.
Each module is now parsed into a small graph (exported name -> local, local ->
identifiers its body references, locals that render the band) and the walk
carries, at every hop, the set of exports the importer actually mounts. The
special cases fall out of module semantics instead of being enumerated:
`dynamic(() => import("…").then((m) => m.Named))` follows only that binding;
a bare `import "…"` renders nothing so is not followed; `export { X } from`
is followed only when the importer wants X; `export *` never supplies a
default, so a page needing only a default gets no hop from it.
Verified: all five production search routes still reach the band; gutting
services/page.tsx and tools/page.tsx to <div /> each reports an orphan; the two
fixtures guarding the new mechanisms were confirmed to fail under targeted
mutation (presence-based band check, and following bare imports) after an
initial pair that did not bite was replaced.
Also from review:
- guard-push escalates to a whole-tree check for a package.json only when it
carries a prettier field, so a routine dependency bump does not. Verified:
bump passes, prettier-field narrowing blocks, .prettierrc narrowing blocks.
- AGENTS.md no longer claims the guard checks working-tree paths; that was the
defect this PR fixed, and leaving it would teach distrust of a fixed guard.
- Repairs the #115 ledger row, whose Source cell had been overwritten by the
detail text, and rewrites it for the new design.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
Thread status before the resolve pass — 10 of 11 are already fixed
Exactly one finding is still live: "Resolve Prettier from the pushed dependency tree" (
The six adoption-gate findings were one defect — the walk asked whether a file mentions the band when the question is whether anything the route mounts reaches it — so Each fix was verified by reverting it and confirming the guard fails: On the open oneA pre-push hook cannot run Two notes for whoever lands this: auto-merge is armed, so any commit from this pass merges once checks pass; and Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ce9f5e632
ℹ️ 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".
Summary
Testing
|
…rts, and policy removal Three review findings on 0ce9f5e, all valid: - `then((mod) => ({ default: mod.Foo }))` — the Next.js wrapper for a named export, which global-search-shell.tsx uses for ClinicalDashboard on the root dashboard chain. thenExportName returned null for it, so the walk fell back to following every export of that module: the exact over-approximation the redesign exists to remove. - A dynamic import whose result is discarded (`void import(…)` in an effect, or a bare `import(…);` statement) is a preload and cannot mount anything. Scoped to discarded *results* rather than requiring lexical containment in `dynamic()`, because clinical-dashboard-lazy.tsx also writes the loader as a separate binding and requiring containment would report that unreachable. - guard-push inspected only the pushed package.json for a `prettier` field, so *adding* one escalated to a whole-tree check but *removing* one did not — drop `tabWidth: 4` and four-space-formatted source starts failing CI. Both endpoints are now inspected via git rather than the checkout. Each fix verified by mutation: unhandling the object wrapper makes the wrapper-plain fixture reach the band; recording discarded imports makes the preload fixture reach it. Both fail as they should. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
…-w53znc' into claude/top-search-design-mockups-w53znc
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de2d16d35a
ℹ️ 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".
…imports
Two further review findings:
- The guard formats with this checkout's Prettier while CI installs the pushed
lockfile, so a push that bumps Prettier itself would be judged by the wrong
version. When the push touches package.json or package-lock.json, the pinned
and installed versions are compared and a difference blocks with an actionable
`npm ci` message. Verified: a push changing the lockfile to a different pin
blocks; a matching pin passes; and — the non-regression that matters — a
mismatched lockfile the push does not touch still passes, so ordinary pushes
are unaffected by node_modules drift.
- `import type { Banded } from "…"` is erased at runtime and cannot mount
anything, but was recorded as a component edge, so a route with a type-only
prop reached the band. Both spellings are skipped (declaration-level
`importKind` and the per-specifier one from `import { type X }`), plus
`export type { X } from`. Pinned by a fixture that fails without the skip.
This closes the last open Codex finding on the branch. Its own fixes for it
exist only as unreachable commits on a branch named `work` — seven SHAs, none
fetchable — so they could not be used.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
BigSimmo
changed the title
…ferences Found in my own review pass, not by a reviewer. The earlier fix skipped `import type` declarations, but a *value* import used only in a type position was still recorded as a runtime edge — and this repo does not enforce `consistent-type-imports`, so `ComponentProps<typeof Banded>` is a legitimate value import that never mounts anything. References are no longer collected from typeAnnotation / returnType / typeParameters / typeArguments subtrees. Value expressions never live under those keys, so nothing real is lost; `x as Foo` still contributes `x`. The first fixture I wrote for this passed for the wrong reason — a top-level `type Props = …` alias is never scanned at all, so it proved nothing. Rewritten with the annotation inline on the scanned declaration, and confirmed to fail when the skip is removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
All review threads dispositioned — resolving them nowEvery finding on this PR is fixed on the branch. Verified against the file contents at
Skipped, with reason: "Scope duplicate-ID validation per table" describes a verification gate proposed inside One further issue, found in my own review rather than by a reviewer
Worth recording: my first fixture for it passed for the wrong reason. A top-level State
Generated by Claude Code |
…indings (#1424) * docs(ledger): record PR #1400 closeout and capture three unrecorded findings Documentation only — two ledger files, no code. **Review closeout for PR #1400** appended with `ledger:append` (never hand-written), recording the 17 findings fixed, the verification behind each, and the post-merge check that all 8 commits are ancestors of main with the 4 changed files byte-identical. **Three findings from that session that nothing else records:** - `#125` — `@codex fix` produced 11 commits across a branch named `work`, none fetchable, the same finding rewritten four times. It reads as success while the branch is unchanged, which is the actual hazard. - `#126` — both client-side push guards are inert for agent pushes: `gh` absent makes the auto-merge sentinel fail open, and `core.hooksPath` is set only by a local install. They protect the environment least likely to need them. - `#127` — this ledger's fixed-width padding makes one row's edit re-pad all 59, so it conflicts on nearly every main advance; each conflict silently stopped all CI on #1400 via `#116`. Records that `merge=union` is the wrong fix, with the evidence. CircleCI was deliberately not filed — already captured as `#122`. Checked before writing rather than after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY * docs(issues): correct unsafe pull_request_target advice in #129 Review caught a real problem in the guidance I filed, not in code: #129's next-action suggested moving *both* push guards server-side into a `pull_request_target` job. That context carries secrets and a write token, and a format check must execute PR-head code — including the dynamic `prettier.config.*` this very PR taught the guard to load. That is the classic privileged-context vector, and `.github/workflows/pr-policy.yml` already avoids it deliberately by checking out only `github.workflow_sha`. Corrected, and the row now records why the whole idea was unnecessary: formatting is already enforced server-side by `Static PR checks` running `format:check` on ordinary `pull_request` CI, so the guard's only unique value is failing fast before the push. Only the metadata-only auto-merge sentinel could safely live in a target job. Bad advice in a durable ledger is worse than no advice — someone would have acted on it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY * fix(issues): repair the duplicated table from the sixth main merge Sixth conflict today, and the first that auto-merged *wrongly*: git's text merge concatenated both tables, duplicating all 63 open rows. PR #1421 had landed on main using #128/#129/#130 — the exact id collision #112 describes — so both sides had those ids with different content and the merge kept both. `npm run check:outstanding-issues` caught it and stated the correct resolution verbatim: renumber the incoming rows above the marker and bump it, rather than taking one side wholesale and dropping the other's rows. Done exactly that — main's table is authoritative, this branch's four rows renumber to #131/#132/#133/#134, marker to 135. Verified both sides' rows survive: main's #128-#130 and mine are all present and distinct. Worth noting main's new #129 (`update-branch` API does not honour the `merge=ledger` driver) is the server-side twin of my #134 (the driver is absent wherever `npm install` was skipped). Same root cause from two directions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY * docs: record PR 1424 review --------- Co-authored-by: Claude <noreply@anthropic.com>
3 participants
Summary
This PR outgrew its original title. It began as one
AGENTS.mdbullet and is now 5 files / +786 lines, because review found real defects in the two things the bullet described. The body below replaces the original "documentation only, two files, one line each", which is no longer true.No production surface is touched.
git diff --name-only origin/main...HEADmatches nothing undersrc/,supabase/,worker/, or.github/workflows/. The two code files are a local pre-push git hook and a Vitest gate. Nothing here can affect the running app, the database, ingestion, retrieval, or clinical output.1.
AGENTS.md— format before pushing, and commit the resultThree CI failures on 2026-07-30 were the same shape: an unformatted file reached
Static PR checks, which runsprettier --check .. The ordinary loop cannot catch it —format:checkis in none oftest,typecheck, orlint. The bullet records two traps beyond "run it": formatting after committing leaves the unformatted blob on the branch, and a per-fileprettier --checkis not the repository-wide check (that was the missed file twice out of three).2.
scripts/guard-push.mjs— check the commit CI will checkThe guard existed but answered the wrong question. It now materialises the pushed SHA in a temporary
git worktreewithnode_moduleslinked in, and runs Prettier there. Six review findings drove this, each one a way working-tree state leaked into the verdict:.prettierrc, so a committed-broken/tree-corrected config passedprettier.config.mjscould not be evaluated at allnode_modules, so it loads its pluginsprettier --check .prettierfields were detected, not removed onesA Prettier execution error now blocks rather than failing open — being unable to check is not evidence a push is clean.
SKIP_FORMAT_GUARD=1remains the escape hatch and is named in every failure message.3.
tests/search-results-band-adoption.test.ts— reachability, not presenceCloses
#115. Six false greens were reported on this gate in one day: an unrendered import,export { X },export { X } from,export *, a bare side-effect import, JSX in an unmounted helper, and a lazy import reaching every sibling export. They are one defect — the walk asked whether a file mentions the band when the question is whether anything the route mounts reaches it. Two of the six were introduced by earlier patches to this same walker, which is why it was replaced rather than patched a seventh time.Each module is now parsed into a small graph (exported name → local, local → identifiers its body references, locals that render the band), and the walk carries at each hop the set of exports the importer mounts. The special cases fall out of module semantics:
dynamic(() => import("…").then((m) => m.Named))follows only that binding;then((mod) => ({ default: mod.Named }))— the shapeglobal-search-shell.tsxuses forClinicalDashboard— resolves through the wrapper; a discardedvoid import(…)is a preload, not a mount;import typeis erased and is not an edge;export *never supplies a default.Also adds
#116: an unmergeable PR runs no CI at all and says nothing. This PR hit that twice — GitHub cannot build the merge ref while conflicted, so everypull_request-triggered workflow is silently skipped and the symptom reads as "CI is broken".Verification
npm run verify:cheap—Test Files 432 passed (432),Tests 4470 passed | 4 skipped (4474)npm run format:check— All matched files use Prettier code style!(search-app)/services/page.tsxandtools/page.tsxto<div />each reports an orphan. Targeted mutations re-open each hole and the guarding fixture fails: presence-based band check, following bare imports, unhandling the object wrapper, recording discarded imports, and not skipping type-only imports.prettierfield an untouched file depended on; a pushed lockfile pinning a different Prettier. Passes: all-clean; a benign config change; a plain dependency bump; and a mismatched lockfile the push does not touch.npm run verify:ui— not run: no UI, routing, styling, reduced-motion or forced-colors behaviour changed;Production UIis correctly out of scope for this diff.npm run check:production-readiness,eval:*,verify:release— not run: no clinical workflow, privacy, environment, Supabase, retrieval or deployment behaviour changed. No provider was contacted at any point.RAG impact: no retrieval behaviour change — nothing under
src/lib/rag/**, clinical-search, retrieval-selection, released-search-order, ranking-config, answer-ranking, the eval harness, the golden fixture, or the retrieval RPCs is touched.Risk and rollout
git pushfrom a checkout withcore.hooksPathset; the gate runs only in Vitest.package.json/package-lock.json, which includes mergingmainwhenmaincarried a lockfile change. With stalenode_modulesthat push is blocked untilnpm ci. Friction only, andSKIP_FORMAT_GUARD=1is named in the message.npm run test, so it blocks CI repo-wide. All five production search routes pass today and both negative controls behave. But a future page mounting through a pattern the graph does not model would be reported as an orphan until someone adds a documentedBAND_ROUTE_ALLOWLISTentry. Loud is the better direction than silent, but it is a change in character for a shared gate.AGENTS.mdbullet, the guard, and the gate touch no shared code.Clinical Governance Preflight
Not applicable.
classifyPullRequestFilesreportsclinicalRisk: falsefor all five paths: no ingestion, answer generation, search/ranking, source rendering, document access, privacy, production environment, or clinical output behaviour is touched.Notes
On
mergevs squash. Auto-merge is armed with methodmerge, so the 15 commits — including twomainmerges — land intact.#088asks for an eye on branch-review-ledger union-driver duplication as open PRs merge; this PR touches that file, so it is one to check afterwards.Two client-side guards are inert for agents.
guard-push.mjsreportedauto-merge: gh not available — auto-merge check skipped (fail-open)for pushes from this environment, andcore.hooksPathis set only by a localnpm install. So neither the format guard nor the auto-merge sentinel protects a push made from an agent's own checkout — the reason theAGENTS.mdinstruction is still needed even though the tooling now exists.What I deliberately did not do. I had proposed a
merge=uniondriver fordocs/outstanding-issues.mdafter a genuine conflict. Testing showed it is wrong: union concatenates conflicting hunks, so two sides each bumping thenext-idmarker produce two markers. A conflict fails loudly; a duplicated marker corrupts the ledger silently. Withdrawn. The real fix is probably to stop padding that table to fixed column widths — one row's edit currently re-pads all 116 — but not in a file several agents are writing to concurrently.🤖 Generated with Claude Code
https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY