fix(audit-git-checkouts): a stale comparison should name the failure that caused it #80
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/audit-default-lookup-wording"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
When a fetched run cannot refresh a comparison, the report now names what failed in the Failures table's own words: "fetch failed" or "could not read the server's default branch". Before, a default checkout's row blamed the fetch even when only the default-branch read failed.
The same cause now appears in the dirty-checkout row and in the "not checked for removal" failure. Ahead/behind figures on these rows are labeled as cached, because this run did not re-check them. The blocked fast-forward reason lists the file types it allows instead of saying "deferred guidance".
Deferred from #77 review round 4.
🤖 Generated with Claude Code
Review
01M3A1GC7R779B1PQQD2XNHPQ4— head23b9fc22adbed95b4c63de29adec8a0f32b4a224Review — j4k-oss/agent-skills @
ceb23322b0Scope: diff against base tree
d1e95321d499Status: dispatched — coverage complete (3/3 slots terminal)
Facts: current review-wide projection
Computed under:
Findings (3)
low — New test title says "default-branch lookup" where the change standardises on "default-branch read" everywhere else
01M3A1S4DE7VNAZ3YTB8E1T1XKskills/audit-git-checkouts/scripts/render-audit-report.test.ts(snippet)low — The fast-forward eligibility policy is pasted into a report table cell, repeating references/checkout-updates.md in every affected row
01M3A1PRZWJZTTK3X95QTAQX9Gskills/audit-git-checkouts/scripts/render-audit-report.ts(snippet)low — staleCause comment promises its text matches a Failures row, but the salvaged-record branch has no such row and the comment omits the stale-only precondition
01M3A1RB98WSM0QTKF45ZQYH29skills/audit-git-checkouts/scripts/render-audit-report.ts(snippet)Other claims
Coverage
Coverage pass: 01M3A1GC9J330DMZ65WW9S7G5T
Accounting: complete
Slot health: healthy
@ -250,3 +250,3 @@case "manual-review": {if (comparison !== null && !comparison.fresh) {if (fetched) return { aheadBehind: formatAheadBehind(comparison), why: "not compared: origin fetch failed" };if (fetched) return { aheadBehind: formatAheadBehind(comparison), why: "not compared: origin fetch or default-branch lookup failed" };medium — "not compared" is printed beside an unlabelled ahead/behind figure the run never verified
lens
writing-quality· armdefault· tally 1 valid / 0 invalid / 0 uncertainclaim
01M39S89QVG47KX5G0J8J46Y2Rof review01M39RX0SPHT4ZRJB7J3QKBQ2JFixed in
e176860. Reproduced: the row printed cached figures beside "not compared". The row now labels them and names the cause from the repository record, in the Failures table's words, for examplecached: 0 commits behind, 0 ahead; not re-checked: could not read the server's default branch. Kept the figures rather thann/a, because the last known distance still helps the user decide.A fetched run with a stale comparison now names what failed, in the Failures table's words ("fetch failed" or "could not read the server's default branch"), in the decision row, in the dirty-checkout row, and in the removal-check failure. The decision row labels its ahead/behind figures as cached instead of saying nothing was compared. The blocked fast-forward reason names the file classes instead of "deferred guidance". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>fix(audit-git-checkouts): an unread server default branch should not be reported as a failed fetchto fix(audit-git-checkouts): a stale comparison should name the failure that caused itThree findings in this report had no inline thread, so their outcomes are recorded here. All are fixed in
e176860.defaultNotCurrentReasoncalls a failed refresh "cached" (low): agreed. Reproduced with a dirty default checkout on a failed fetch. It now readscached comparison, 4 uncommitted files; not re-checked: fetch failed, matching the clean row. New test: "a dirty default checkout on a failed fetch names the cause beside its cached comparison".not checked for removal: …all use the Failures table's wording: "fetch failed" or "could not read the server's default branch". "default branch unresolved" stays the only wording for the case with no branch name.not all are AGENTS.md, .agents/, or submodule selector changes, or upstream changed the same paths.The rejected claim about the test name ("does not blame the fetch") needs no change now: that test's rows name only the default-branch read, and it asserts that "fetch failed" appears nowhere.
@ -389,0 +425,4 @@],}),);assert.ok(markdown.includes("| app | main | +0/-5 | cached comparison, 4 uncommitted files; not re-checked: fetch failed |"));medium — The new failed-fetch test pins a single-cause message by setting remoteHeadOk: true, so the ", and" join a real failed fetch produces is asserted nowhere
lens
test-trimming· armdefault· tally 1 valid / 0 invalid / 0 uncertainclaim
01M39THRWFKFEXFX7CK9TECEBDof review01M39T9TR976SWXG1Y2N5JNNRQFixed in
1fda19e. The failed-fetch test now also setsremoteHeadOk: false, which is what an unreachable origin produces. It asserts the row and the matching Failures row. Swapping the order of the cause checks now fails the test.@ -179,0 +181,4 @@if (repository.fatalError !== undefined) return "audit aborted";const causes: string[] = [];if (repository.fetch.ok !== true) causes.push("fetch failed");if (repository.defaultBranchResolution.remoteHeadOk !== true) causes.push("could not read the server's default branch");low — Two stale-comparison causes join as "fetch failed, and could not read the server's default branch", a comma-spliced repeat of the Failures rows above
lens
writing-quality· armdefault· tally 1 valid / 0 invalid / 0 uncertainclaim
01M39TJHTCG2WJ4NFXBAG01348of review01M39T9TR976SWXG1Y2N5JNNRQFixed in
1fda19e. Reproduced the "fetch failed, and could not read …" cell. The row now names only the first cause (not re-checked: fetch failed), and the Failures table still lists both.@ -179,0 +182,4 @@const causes: string[] = [];if (repository.fetch.ok !== true) causes.push("fetch failed");if (repository.defaultBranchResolution.remoteHeadOk !== true) causes.push("could not read the server's default branch");return causes.join(", and ") || "comparison not refreshed";low — staleCause fallback renders "not re-checked: comparison not refreshed", restating the label instead of naming a cause, and is absent from the Failures table its comment cites
lens
writing-quality· armdefault· tally 1 valid / 0 invalid / 0 uncertainclaim
01M39TJ13FR8S0WRQ4MWX0ARZRof review01M39T9TR976SWXG1Y2N5JNNRQFixed in
1fda19e. The fallback now reads "no cause recorded", and the comment says the causes are worded as the Failures rows. I kept a string rather thannullbecausenullmarks a cached run, and a fetched run must never count an uncompared checkout as current.@ -185,3 +197,3 @@if (comparison.ahead > 0) return `${count(comparison.ahead, "local commit")} not pushed${dirtyNote}`;if (dirty > 0 && comparison.behind > 0) {return `behind; uncommitted changes (${count(dirty, "file")}) block the fast-forward: not deferred guidance, or upstream changed the same paths`;return `behind; uncommitted changes (${count(dirty, "file")}) block the fast-forward: not all are AGENTS.md, .agents/, or submodule selector changes, or upstream changed the same paths`;medium — Fast-forward blocker message drops the "first-party" qualifier and the "at any depth" scope, naming a rule the driver does not enforce
lens
writing-quality· armdefault· tally 1 valid / 0 invalid / 0 uncertainclaim
01M39TGHD5MBXZKY5NDFHSVCVWof review01M39T9TR976SWXG1Y2N5JNNRQlow — Nested "or" in the fast-forward blocker reason reads as a flat four-item list, hiding the two conditions it actually names
lens
writing-quality· armdefault· tally 1 valid / 0 invalid / 0 uncertainclaim
01M39TH6XW0VWQ54KVC9CQ1W4Pof review01M39T9TR976SWXG1Y2N5JNNRQFixed in
1fda19e. Confirmed the third-party early return and the*/.agents/*arms inis_deferred_guidance_path. The cell now reads "not all are AGENTS.md, .agents/** at any depth, or first-party submodule selector changes; or upstream changed the same paths".Fixed in
1fda19eby the same edit: a semicolon now separates the two conditions, so the commas belong to the inner list.An unreachable origin fails both the fetch and the default-branch read, and the row joined them as "fetch failed, and could not read …". It now names the first cause only, because the Failures table lists every one. The fallback reads "no cause recorded" instead of restating the label. The blocked fast-forward reason keeps the rule's scope ("at any depth", "first-party"), and a semicolon now separates its two conditions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>@ -389,0 +426,4 @@}),);assert.ok(markdown.includes("| app | main | +0/-5 | cached comparison, 4 uncommitted files; not re-checked: fetch failed |"));assert.ok(markdown.includes("| app | could not read the server's default branch |"));low — The only two-cause fixture never asserts the fetch-failure row, so
repositoryFailuresdropping it stays greenlens
test-trimming· armdefault· tally 1 valid / 0 invalid / 0 uncertainclaim
01M3A0AEB0PEJWCBZJ2E8RWBTRof review01M3A00T4P94ZEHBC34NZJPDD3Fixed in
23b9fc2. The two-cause test now also asserts| app | fetch failed: fatal: unable to access |.@ -177,2 +177,4 @@function defaultNotCurrentReason(worktree: Worktree): string {// Why a fetched run left a repository's comparisons stale, worded as its Failures row; null on a cached run.// Only the first cause is named: the Failures table lists every one.function staleCause(repository: Repository, fetched: boolean): string | null {low — staleCause's "no cause recorded" fallback reaches the user as a Why cell and breaks the comment's promise that Failures lists every cause
lens
writing-quality· armdefault· tally 1 valid / 0 invalid / 0 uncertainclaim
01M3A07ZPNG61SJP6VS9PVTV10of review01M3A00T4P94ZEHBC34NZJPDD3Fixed in
23b9fc2. Dropped the fallback: the driver marks a comparison stale only when the fetch or the default-branch read failed, so the last branch returns the default-branch cause. The comment states that invariant.@ -179,0 +179,4 @@// Only the first cause is named: the Failures table lists every one.function staleCause(repository: Repository, fetched: boolean): string | null {if (!fetched) return null;if (repository.fatalError !== undefined) return "audit aborted";low — staleCause() blames "audit aborted" for a salvaged repository, dropping the fetch/default-branch cause the previous wording carried
lens
general-bug· armdefault· tally 1 valid / 0 invalid / 0 uncertainclaim
01M3A0DKKSHCTQE3RDDPK8R9JCof review01M3A00T4P94ZEHBC34NZJPDD3Fixed in
23b9fc2. A salvaged record now readsnot checked for removal: origin fetch or default-branch read failed, because the record no longer says which step failed. The salvaged-record test asserts that row.@ -179,0 +185,4 @@return "no cause recorded";}function defaultNotCurrentReason(worktree: Worktree, staleCause: string | null): string {low — The name
staleCausenow means both the helper and its result, while the call site calls the same valuecauselens
writing-quality· armdefault· tally 1 valid / 0 invalid / 0 uncertainclaim
01M3A08T12ZRY5YS9CA4A83J49of review01M3A00T4P94ZEHBC34NZJPDD3Fixed in
23b9fc2. The parameter is nowcauseinstepFailure,defaultNotCurrentReasonandprimaryDecision, sostaleCausenames only the helper.@ -185,3 +197,3 @@if (comparison.ahead > 0) return `${count(comparison.ahead, "local commit")} not pushed${dirtyNote}`;if (dirty > 0 && comparison.behind > 0) {return `behind; uncommitted changes (${count(dirty, "file")}) block the fast-forward: not deferred guidance, or upstream changed the same paths`;return `behind; uncommitted changes (${count(dirty, "file")}) block the fast-forward: not all are AGENTS.md, .agents/** at any depth, or first-party submodule selector changes; or upstream changed the same paths`;medium — The fast-forward-blocked cell reads as a four-item list, and "at any depth" now covers only .agents/**
lens
writing-quality· armdefault· tally 1 valid / 0 invalid / 0 uncertainclaim
01M3A07AP4FW3Y178XPETZXXMVof review01M3A00T4P94ZEHBC34NZJPDD3Fixed in
23b9fc2with your positive wording: "block the fast-forward. A dirty checkout fast-forwards only when every changed path is AGENTS.md or under .agents/, at any depth, or first-party submodule metadata, and upstream changed none of those paths." "at any depth" now covers both, and "submodule metadata" covers the unconditional.gitmodulescase.@ -388,0 +392,4 @@assert.ok(markdown.includes("| app-y | not checked for removal: origin fetch or default-branch read failed |"));});test("a fetched run whose default-branch lookup failed does not blame the fetch", () => {low — New test title says "default-branch lookup" where the change standardises on "default-branch read" everywhere else
lens
writing-quality· armdefault· tally 1 valid / 0 invalid / 0 uncertainclaim
01M3A1S4DE7VNAZ3YTB8E1T1XKof review01M3A1GC7R779B1PQQD2XNHPQ4@ -176,3 +176,3 @@}function defaultNotCurrentReason(worktree: Worktree): string {// Why a fetched run left a repository's comparisons stale, worded as its Failures row; null on a cached run.low — staleCause comment promises its text matches a Failures row, but the salvaged-record branch has no such row and the comment omits the stale-only precondition
lens
writing-quality· armdefault· tally 1 valid / 0 invalid / 0 uncertainclaim
01M3A1RB98WSM0QTKF45ZQYH29of review01M3A1GC7R779B1PQQD2XNHPQ4@ -185,3 +197,3 @@if (comparison.ahead > 0) return `${count(comparison.ahead, "local commit")} not pushed${dirtyNote}`;if (dirty > 0 && comparison.behind > 0) {return `behind; uncommitted changes (${count(dirty, "file")}) block the fast-forward: not deferred guidance, or upstream changed the same paths`;return `behind; uncommitted changes (${count(dirty, "file")}) block the fast-forward. A dirty checkout fast-forwards only when every changed path is AGENTS.md or under .agents/, at any depth, or first-party submodule metadata, and upstream changed none of those paths.`;low — The fast-forward eligibility policy is pasted into a report table cell, repeating references/checkout-updates.md in every affected row
lens
writing-quality· armdefault· tally 1 valid / 0 invalid / 0 uncertainclaim
01M3A1PRZWJZTTK3X95QTAQX9Gof review01M3A1GC7R779B1PQQD2XNHPQ4Round-4 outcomes for review
01M3A1GC7R779B1PQQD2XNHPQ4(head23b9fc2). From round 4, a new push goes only to clear, severe bugs. All three findings are real but change only a test name, a comment, or report wording, so they are acknowledged and deferred to a follow-up PR:scripts/render-audit-report.test.ts, rename the test to "a fetched run whose default-branch read failed does not blame the fetch".scripts/render-audit-report.tsdefaultNotCurrentReason, shorten the cell to "behind; uncommitted changes (N files) block the fast-forward (see references/checkout-updates.md)", and update the matching test assertion.staleCausecomment over-promises. Inscripts/render-audit-report.ts, reword it to say the result is read only where a comparison is stale, that a fetch failure wins when both failed, and that a salvaged record names both possibilities.