Skip to content

Refuse a branch inventory that cannot be shown to cover every branch - #1398

Merged
BigSimmo merged 7 commits into
mainfrom
claude/test-coverage-analysis-2vcd8a
Jul 30, 2026
Merged

Refuse a branch inventory that cannot be shown to cover every branch#1398
BigSimmo merged 7 commits into
mainfrom
claude/test-coverage-analysis-2vcd8a

Conversation

@BigSimmo

Copy link
Copy Markdown
Owner

Summary

Follow-up to #1392 (ledger #109), found in review after that PR merged. Complete history is not complete branch coverage, and the second condition survives the first fix.

git clone --depth 1 implies --single-branch, which pins remote.origin.fetch to the one cloned branch. git fetch --unshallow converts the history — so git rev-parse --is-shallow-repository reads false and shallowCloneRefusal is satisfied — but it does not widen the refspec, and an ordinary git fetch origin respects the narrow one.

Measured in a fixture with main and feature, after unshallowing:

is-shallow: false refspec: +refs/heads/main:refs/remotes/origin/main
remote heads on origin: refs/heads/feature refs/heads/main
sweep --json: { "branches": [] } EXIT=0

An empty inventory is not a safe failure here — it reads as "nothing to clean up". And where origin/main itself is the branch not fetched, every rev-list fails into 0/0, making every branch a deletion candidate with no unique patch content. That is the same hazard as #109, reached by a different route and past the guard written for it.

Fixed in both directions. The sweep's own fetch now passes an explicit +refs/heads/*:refs/remotes/origin/*, which populates every remote-tracking ref without rewriting the operator's config, so an ordinary run repairs its own coverage. branchCoverageRefusal then refuses when neither the configured refspec nor a completed wildcard fetch establishes coverage — --no-fetch, offline, or a failed fetch. Its remedy is git remote set-branches origin '*', deliberately not--unshallow, which fixes history and does nothing here; sending the operator there means they re-run and get the same empty inventory, now doubly convinced it is right.

Negative refspecs are their own case, and the behaviour is asymmetric. Measured on git 2.43.0 with +refs/heads/*:refs/remotes/origin/* plus ^refs/heads/feature:

Fetchorigin/feature
git fetch --prune origin (configured refspec)absent — exclusion honoured
git fetch --prune origin '+refs/heads/*:refs/remotes/origin/*'restored — command line overrides it

So a ^ spec means the config alone cannot establish coverage, while a completed fetch still can. Reading the wildcard and ignoring the exclusion would report a partial inventory as complete.

Verification

  • npm run verify:cheapTest Files 432 passed (432), Tests 4466 passed | 4 skipped (4470). test is last in that && chain, so the 24 static gates, lint and typecheck all passed ahead of it.
  • Narrow-refspec fixture, before → after: the script as merged on main exits 0 with "branches": []; with this change --no-fetch exits 1 carrying the set-branches remedy, and with fetch allowed exits 0 with feature present in the inventory.
  • Negative-refspec fixture: --no-fetch refuses with branches: null and exit 1; fetch allowed exits 0 and lists feature.
  • Non-regression on this full clone: npm run sweep:branch-ledger -- --no-fetch still prints the normal inventory and exits 0 (remote.origin.fetch is +refs/heads/*:refs/remotes/origin/* here, so no refusal).
  • 8 new cases in tests/repo-hygiene.test.ts (38 in the file): wildcard / mirror / unprefixed accepted; single-branch, several-narrow, absent config rejected; negative refspec rejected from config but permitted after a completed fetch; the refusal's Fix: line must name set-branches and must not name --unshallow.
  • UI verification not run: no UI, routing, styling or browser behaviour changed.
  • Retrieval/answer evals not run: no retrieval, ranking, selection or answer-generation change.
  • npm run check:production-readiness not run: no clinical workflow, privacy, environment, Supabase, source-governance or deployment change.

One of my own assertions was wrong on the first pass — it required the refusal body not to mention --unshallow at all, when the body mentions it deliberately, as the thing that does not fix a refspec. Narrowed to the Fix: line, which is what actually misdirects an operator.

Risk and rollout

  • Risk: low, and in the safe direction — the guard can only stop a report, never produce a wrong one. Two realistic failure modes were checked: refusing on a healthy clone (guarded by the predicate tests plus the live non-regression run above), and the fetch now carrying an explicit refspec. The latter is equivalent to the configured one on a normal clone; on a narrow one it fetches more, which is the point. No config is mutated, and the script remains report-only.
  • Rollback: revert this PR. The sweep returns to reporting a partial inventory as complete on a single-branch clone.
  • Provider or production effects: None. No provider-backed gate was run; no product code, schema, API or gate behaviour is touched.

Clinical Governance Preflight

Not applicable to this diff. classifyPullRequestFiles reports clinicalRisk: false, operationalRisk: false, ragRanking: false, ui: false for the changed paths — scripts/sweep-branch-ledger.mjs, tests/repo-hygiene.test.ts, docs/branch-cleanup-guide.md, docs/outstanding-issues.md. scripts/** is clinical-risk only when it matches ingest/reindex/migration/governance/production/drift/supabase, and a branch-hygiene reporter matches none of those. No ingestion, answer generation, search/ranking, source rendering, document access, privacy or clinical output behaviour changed.

Notes

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.

The branch was restarted from the post-merge main rather than stacked on the merged history, and the remote branch had been auto-deleted on merge, so this is a new branch and a new PR rather than a reopen. Pruning stale refs also showed that two of the five #108 deletion candidates — claude/clinical-kb-pwa-review-asi3wb and claude/dazzling-blackwell-f348d0 — are already gone from the remote; #108 still lists them and needs re-checking against the live remote before its ledger rows are appended.

docs/branch-cleanup-guide.md gains the refspec check alongside the shallow one, because the raw git commands the guide documents have no guard even now that the script does.

🤖 Generated with Claude Code

https://claude.ai/code/session_012YRCXgX4AWZ579bKN6sk6b


Generated by Claude Code

…very branch
Complete history is not complete branch coverage, and the second condition
survives the first fix. `git clone --depth 1` implies `--single-branch`, which
pins remote.origin.fetch to the one cloned branch. `git fetch --unshallow`
converts the history — so `--is-shallow-repository` reads false and
shallowCloneRefusal is satisfied — but it does not widen the refspec, and an
ordinary `git fetch origin` respects the narrow one.
Measured in a fixture with `main` and `feature`: after unshallowing,
`git ls-remote --heads origin` listed both while refs/remotes/origin held only
origin/main, and the sweep exited 0 reporting `"branches": []`. An empty
inventory is not a safe failure here — it reads as "nothing to clean up", and
where origin/main itself is missing every rev-list fails into 0/0, making every
branch a deletion candidate with no unique patch content.
Fixed in both directions. The sweep's own fetch now passes an explicit
+refs/heads/*:refs/remotes/origin/*, which repairs coverage without rewriting the
operator's config, so an ordinary run just works. branchCoverageRefusal then
refuses when neither the configured refspec nor a completed wildcard fetch
establishes coverage — --no-fetch, offline, or a failed fetch. Its remedy is
`git remote set-branches origin '*'`, deliberately not --unshallow, which fixes
history and does nothing here.
Negative refspecs are handled as their own case. Measured on git 2.43.0 with
`+refs/heads/*:refs/remotes/origin/*` plus `^refs/heads/feature`, an ordinary
fetch honoured the exclusion and left origin/feature absent, while an explicit
command-line wildcard overrode it and restored the ref. So a `^` spec means the
config alone cannot establish coverage, but a completed fetch still can.
Proof, narrow-refspec fixture, before -> after: exit 0 with `"branches": []` ->
exit 1 with the set-branches remedy; with fetch allowed, exit 0 and `feature`
appears. Non-regression on this full clone: `sweep:branch-ledger -- --no-fetch`
still reports 91 branches and exits 0.
Follow-up to #1392 (ledger #109), which merged before this was found.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012YRCXgX4AWZ579bKN6sk6b
@supabase

supabaseBot commented Jul 30, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project sjrfecxgysukkwxsowpy because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@coderabbitai

coderabbitaiBot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 70a759d9-9da8-4d74-8ff2-8f87356629ec

📥 Commits

Reviewing files that changed from the base of the PR and between cd2c666 and f6d1f72.

📒 Files selected for processing (5)
  • docs/branch-cleanup-guide.md
  • docs/branch-review-ledger.md
  • docs/outstanding-issues.md
  • scripts/sweep-branch-ledger.mjs
  • tests/repo-hygiene.test.ts

Comment @coderabbitai help to get the list of available commands.

@BigSimmo
BigSimmo marked this pull request as ready for review July 30, 2026 03:20
@BigSimmo

Copy link
Copy Markdown
OwnerAuthor

@cursoragent Inspect the PR and repository state, identify the root cause of every failing CI check, unresolved review comment, merge conflict, and related defect. Apply the smallest robust fix using existing project patterns and best practices, without unrelated changes. Run Bugbot and all relevant lint, type-check, test, build, and CI-equivalent commands, fix any issues found, then resolve only the review comments that are fully addressed. Summarise the root causes, changes made, validation results, and any remaining blockers.

@cursor

cursorBot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

PR #1398 is unblocked. Hosted PR required is green, both Codex threads are resolved, and merge-tree vs main is clean. Squash/merge auto-merge is enabled.

Root causes

IssueCause
CONFLICTING / DIRTYReal conflict in docs/outstanding-issues.md — main added #115; this PR updated #109
Missing GitHub CI on first tippull_request CI never fired until a push (only PR Policy + CircleCI ran)
Codex P2 #1 (resolved by Claude in de594186)Coverage accepted refspecs by source alone; wrong destination left inventory empty
Codex P2 #2 (fixed here)+refs/*:refs/remotes/origin/* accepted as covered, but git nests to origin/heads/<branch>
BugbotNo findings
Failing product CINone after the conflict sync

Changes made

  • Merged main; kept #115 + next-id=116; preserved this PR’s #109 refspec follow-up
  • Fixed Codex P2: fetchRefspecCoversAllBranches accepts only refs/heads/*refs/remotes/origin/*
  • Replied + resolved both Codex threads
  • Recorded babysit in the review ledger

Validation

  • npm run verify:cheapPASS (432 files, 4469 passed | 4 skipped)
  • tests/repo-hygiene.test.ts40/40
  • Hosted: PR required, Unit coverage, Static PR, Safety, Semgrep, CircleCI — PASS
  • Unresolved review threads: 0

Remaining

  • GitHub mergeable still briefly UNKNOWN after the last push (checks green, tree merge clean) — auto-merge should proceed once GitHub refreshes
Open in WebOpen in Cursor

@BigSimmo

Copy link
Copy Markdown
OwnerAuthor

@codex review PR

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:d0345bb187

ℹ️ 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".

Comment threadscripts/sweep-branch-ledger.mjs Outdated
cursoragentand others added 3 commits July 30, 2026 03:28
Keep main's #115 band-adoption follow-up and next-id=116. Preserve this
PR's #109 update documenting the single-branch refspec failure mode
after unshallow (complete history ≠ complete branch coverage).
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
…origin
The coverage predicate accepted a refspec on its source alone, which was wrong
in the dangerous direction. Git's `<dst>` decides which local ref is updated, and
this sweep enumerates refs/remotes/origin and nothing else, so a refspec that
fetches every branch somewhere else satisfied the guard while leaving the
inventory empty.
Measured with `+refs/heads/*:refs/remotes/upstream/*`: refs/remotes/upstream held
upstream/main and upstream/feature, refs/remotes/origin was empty, and the sweep
exited 0 reporting `"branches": []` — the same "empty inventory presented as
trustworthy" defect the guard exists to prevent, reached by a third route. A
mirror's `+refs/*:refs/*` behaves the same way, fetching into refs/heads/*.
Coverage from config now requires both halves: a source of refs/heads/* or
refs/*, AND a destination of refs/remotes/origin/*. The sweep's own fetch already
passes an explicit destination, so an ordinary run still repairs its own coverage
and only --no-fetch/offline refuses.
One of my own test cases asserted that a mirror's `+refs/*:refs/*` counted as
covered. It is replaced, with the measurement recorded in the test body.
Proof, custom-destination fixture: --no-fetch exits 1 with the refusal; with
fetch allowed exits 0 and lists `feature`. 39 cases pass in
tests/repo-hygiene.test.ts.
Reported by Codex on PR #1398.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012YRCXgX4AWZ579bKN6sk6b

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:de59418676

ℹ️ 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".

Comment threadscripts/sweep-branch-ledger.mjs Outdated
cursoragentand others added 3 commits July 30, 2026 03:38
…analysis-2vcd8a
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Git wildcard substitution nests the matched suffix, so
`+refs/*:refs/remotes/origin/*` writes `refs/remotes/origin/heads/<branch>`
rather than `origin/<branch>`. Accept only `refs/heads/*` →
`refs/remotes/origin/*`. Addresses Codex P2 on PR #1398.
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
@BigSimmo
BigSimmo enabled auto-merge July 30, 2026 03:41
@BigSimmo
BigSimmo merged commit da24b8a into mainJul 30, 2026
19 checks passed
@BigSimmo
BigSimmo deleted the claude/test-coverage-analysis-2vcd8a branch July 30, 2026 03:44
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@BigSimmo@claude@cursoragent