Record PR #1383 verification, the branch-cleanup review, and the shallow-clone analysis trap - #1387
Conversation
…atrix coverage (#99) Both recorded from the 2026-07-29 test-coverage analysis with measured evidence so the follow-ups survive the session context. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012YRCXgX4AWZ579bKN6sk6b
…L route Adds executing coverage to the two highest-risk surfaces that had none, and re-ratchets the broad coverage floor that had drifted well below measured. Clinical calculator scoring (src/components/calculators/calculator-ui.tsx, previously 0% executed lines). The exported pure functions carry the scoring and interpretation for eight validated instruments — PHQ-9, GAD-7, K10, MDQ, CAGE, AUDIT-C, SAD PERSONS, Y-BOCS — and the only calculator test in the repo asserted mockup import boundaries. A wrong severity band renders as an ordinary result, so it needs a floor. tests/calculator-scoring.test.ts covers band-table integrity for all eight instruments (contiguous, spanning the declared range, exactly one band per reachable score, range actually reachable from the items), PHQ-9/GAD-7 boundary scores either side of every cut point, itemScore edge cases, band suppression for scales that cannot read zero (K10, CAGE), the MDQ three-criterion rule, and the result summary text. No live defect was found — the band tables are correct today; nothing was holding them there. Batch image signed URLs (src/app/api/images/signed-urls/route.ts, previously 0% executed lines, no test references). The route mints up to 100 signed URLs per call and carries its own owner-scope and committed-generation implementation, parallel to /api/images/[id]/signed-url which already had five cases. check:owner-scope-api cannot cover the real protection because document_images has no owner_id column: the only thing excluding another owner's image is the documentMap join filter in the handler. Nine cases now pin that filter, including the batch-specific case where one unowned id rides along with an owned one. Added to tests/private-access-routes.test.ts rather than a new file so it reuses that harness; createSignedUrls was added to the storage mock. Both additions were mutation-checked. Overlapping a PHQ-9 band fails three tests including "scores 5 as Mild"; loosening the MDQ impairment threshold from moderate to minor fails one; making the batch route allow a documentMap miss fails the two cross-owner cases. Before this change that last mutation passed the entire suite silently. Coverage floor: the broad group measured 63.99/55.29/57.6/66.19 against floors of 48/38/43/50 — 14-17pp of slack, so a change could delete a large amount of coverage and still pass. Each floor now sits ~2pp under measured. Kept deliberately in the cheap tiers: the unit suite is 46% test execution and 54% import/environment overhead, so a new file costs ~0.37s before it asserts anything while an assertion in an existing file costs ~0.03s. The calculator file runs 60 tests in 514ms (27ms of that is assertions). Nothing was added to the Playwright gate, which runs ~231 journeys serially against a 45-minute budget. Verified: npm run verify:cheap (exit 0; 423 files, 4340 passed | 4 skipped) and npm run test:coverage (exit 0, no threshold errors). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012YRCXgX4AWZ579bKN6sk6b
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
…analysis-2vcd8a Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
…analysis-2vcd8a Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Confirms the squash merge landed by content: the diff between squash commit f4a3f50 and branch tip bebc6c0 is empty, so nothing was orphaned by the auto-merge race despite four concurrent pushes during the merge window. Also records that the coverage follow-ups were renumbered #98/#99 -> #106/#107 (main claimed #98-#105 concurrently) with both rows verified intact, and that the three red `PR required` results were concurrency cancellations rather than defects. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012YRCXgX4AWZ579bKN6sk6b
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 1 minute 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 (2)
Comment |
Branch-cleanup review of all 92 remote branches against main, recorded with scope `branch-cleanup-deletion-pending` because deletion could not be completed: the session git proxy rejects ref deletion with HTTP 403 and no delete-branch capability is exposed. Five branches are verified safe to delete (empty diff against main, no open PR) and are tracked as #108. The other 87 were deliberately not cleared — their touched files still differ from main, the conservative direction. #109 records the trap that made the first pass wrong: the session clone was shallow (74 of 2829 commits), which silently invalidated every merge-base, cherry-pick and ahead/behind result. In that state local main read as "ahead 52" with unrelated histories, and 90 of 91 branches read as unmerged. `git fetch --unshallow` corrected both, and the guide and sweep script should check for it rather than report silently. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012YRCXgX4AWZ579bKN6sk6b
BigSimmo
changed the title
![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: 873d28df85
ℹ️ 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".
… classification Three P2 findings from the Codex review of 873d28d, all documentation correctness on rows added in this PR. Record each deletion candidate's HEAD (finding 2, the substantive one). `hasCompletedCleanupReview` in scripts/sweep-branch-ledger.mjs matches a completed row on branch name AND HEAD together, and the aggregate row was keyed only to main@855aa291. Once the five refs are deleted their HEADs are unrecoverable, so no later operator could append the required `branch-cleanup` rows. Each candidate now has its own pending row keyed to its own HEAD, and #108 carries the same SHAs. Record the two-tree evidence explicitly (finding 1). The rows now state the merge-base and the `git diff --name-only <merge-base> <tip>` result per branch rather than only the three-dot form. Two tips (df29f31, c9bec8f) are themselves the merge-base with main, i.e. direct ancestors; the other three have tree(merge-base) == tree(tip). Correct the provider-backed classification (finding 3). The prior checks cell said "no provider-backed checks" while the outcome described GitHub open-PR cross-checks and an HTTP 403 deletion attempt. The new rows enumerate the GitHub reads and the rejected mutation, and reserve the "no provider-backed" wording for genuinely local work. The ledger is append-only, so these are superseding rows; the original aggregate row is left intact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012YRCXgX4AWZ579bKN6sk6b
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4cb33ed148
ℹ️ 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".
…ting refs Codex review of 4cb33ed found the documented order unexecutable, and it is right. #108 said to delete the five branches and then append the completed `branch-cleanup` rows, but `resolveHead` (scripts/branch-review-ledger.mjs:155-167) runs `git rev-parse --verify <sha>^{commit}` and refuses a HEAD that is not a commit in the repository. Once the refs are gone and their objects are pruned, the append can never succeed, so recording the SHAs in the previous commit was necessary but not sufficient. Order is now append-then-delete, and the note explains why the `n/a - <reason>` escape hatch is not an acceptable substitute: `hasCompletedCleanupReview` matches only a 7-40 char hex HEAD, so an `n/a` row would leave each branch resurfacing in every future sweep. Verified locally: all five recorded SHAs resolve in this checkout, and an absent SHA produces `refusing to append: head "..." is not a commit in this repository`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012YRCXgX4AWZ579bKN6sk6b
3 participants
Summary
Documentation and ledger bookkeeping only. The entire diff against
mainisdocs/branch-review-ledger.mdanddocs/outstanding-issues.md— no source, config or test change.1. PR #1383 post-merge verification (
prlanded@f4a3f50). The content diff between the squash commit and the pre-merge branch tipbebc6c0is empty, so nothing was orphaned by the auto-merge race despite four concurrent pushes during the merge window. Also records that the coverage follow-ups were renumbered#098/#099→#106/#107whenmainconcurrently claimed#098–#105(both rows verified intact, next-id at 108), and that the three redPR requiredresults at0922d7f/6f476b5/b72bc20were concurrency cancellations — tracked issue#095— not defects; every upstream job reportedcancelled.2. PR #1383 babysit closeout (
5145dc9). This row was committed on this branch by a concurrent session but never reachedmain, because the squash merge took the branch atbebc6c0and those commits landed afterwards. Carrying it here is what stops it being lost.3. Branch-cleanup review (
branch-cleanup-deletion-pending@855aa291). All 92 remote branches reviewed againstmain. Five are verified safe to delete — each introduces an empty diff againstmainand backs no open PR:claude/clinical-kb-pwa-review-asi3wbclaude/dazzling-blackwell-f348d0codex/document-reader-condensed-viewcursor/page-anchored-search-composer-30eecursor/pr-1379-babysit-ledger-9365Deletion could not be completed: the session git proxy rejects ref deletion with HTTP 403, and the GitHub MCP toolset exposes no delete-branch capability. The scope is therefore
branch-cleanup-deletion-pending, whichdocs/branch-cleanup-guide.mddeliberately does not count as complete, and the work is tracked as#108. The other 87 branches were not cleared — their touched files still differ frommain, which is the conservative direction.4. New issue
#109— shallow clones silently invalidate branch analysis. This is the substantive finding. The session clone hadgit rev-parse --is-shallow-repository=truewith only 74 commits oforigin/mainagainst a real history of 2829. In that state every merge-base,--cherry-pickand ahead/behind result was wrong: localmainreportedahead 52andrefusing to merge unrelated histories(it is actually 0 ahead on a shared base), and a full sweep reported 90 of 91 branches as carrying unmerged work. Acting on those numbers would have meant either deleting live branches or abandoning cleanup as impossible.git fetch --unshallowcorrected both. The proposed fix is to make the shallow check an explicit fail-closed precondition in the cleanup guide and inscripts/sweep-branch-ledger.mjs.Worth noting for anyone repeating this: cherry-pick matching alone finds only 2 of the 5 landed branches, because squash merges collapse N commits into 1 so per-commit patch-ids never match. The content test — files touched vs merge-base, compared between branch tip and
main— is what finds the rest.Verification
npm run check:branch-review-ledger— PASS.Branch review ledger guard passed: 1227 table records (22 under the 2026-07-29 machine-readable contract), union merge active, six cells each, no conflict markers, mojibake, heading records, or duplicates.npm run docs:check-links— PASS.npx prettier --writeon both changed docs; formatting clean.git diff --stat origin/main...origin/<branch>empty, and cross-checked against the head refs of open PRs fix: high-confidence bugs from last-100-PR review #1374 / Implement the free and flag-gated latency audit findings #1377 / docs(ledger): record PR #1378 merge closeout #1384 / ci: add dispatch-only live Web-Vitals workflow to discharge #017 #1385 / docs(ledger): record PR #1381 CI triage closeout #1386 / Record PR #1383 verification, the branch-cleanup review, and the shallow-clone analysis trap #1387.git fetch --unshallow(is-shallow-repositorynowfalse, 2829 commits).verify:cheaphas no changed surface to exercise. The ledger's own integrity guard is the applicable check and it passed.npm run check:production-readinessnot run: no clinical workflow, privacy, environment, Supabase, source-governance or deployment change.Risk and rollout
docs/branch-review-ledger.mdismerge=unionby design, so concurrent appends from other sessions do not conflict. No branch was deleted, so nothing is destructive.--supersede) rather than an edit; nothing depends on these rows at runtime.Clinical Governance Preflight
Not applicable to this diff.
classifyPullRequestFilesreportsclinicalRisk: false— the only changed paths aredocs/branch-review-ledger.mdanddocs/outstanding-issues.md, neither of which matches a clinical-risk pattern inscripts/pr-policy.mjs. No ingestion, answer generation, search/ranking, source rendering, document access, privacy or clinical output behaviour is touched.Notes
The
#095false-failure mode — a redPR requiredproduced purely bycancel-in-progresscancellations — was demonstrated three times on PR #1383 in a single afternoon and nearly triggered a remediation loop when an agent was asked to fix "every failing CI check". It is currently filed P3; the evidence recorded here may justify re-rating it, since a red required check that does not mean failure teaches both people and agents to discount red.