fix(audit-git-checkouts): a stale comparison should name the failure that caused it #80

Merged
jercik merged 4 commits from fix/audit-default-lookup-wording into main 2026-09-25 06:44:37 +00:00
Owner

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

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](https://claude.com/claude-code)
fix(audit-git-checkouts): an unread server default branch should not be reported as a failed fetch
All checks were successful
commit-msg / commitlint (pull_request) Successful in 24s
Node tests / node:test (pull_request) Successful in 45s
Review / Review (pull_request_target) Successful in 12m9s
5f9fce1d06
A default checkout is left uncompared when either the fetch or the
ls-remote default-branch lookup fails, but its row always blamed the
fetch, contradicting the repository's Failures row.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Review 01M3A1GC7R779B1PQQD2XNHPQ4 — head 23b9fc22adbed95b4c63de29adec8a0f32b4a224

Review — j4k-oss/agent-skills @ ceb23322b0

Scope: diff against base tree d1e95321d499
Status: dispatched — coverage complete (3/3 slots terminal)
Facts: current review-wide projection

Computed under:

{
  "abandonment": "abandonment-v1",
  "anchor_recipe": 1,
  "batch_policy": "batch-v1",
  "coverage": "coverage-v3",
  "dispatch_policy": "dispatch-v2",
  "grounder_version": 1,
  "grounding_read_rule": "grounding-read-v1",
  "promotion_policy": "promotion-v1",
  "report": "report-v3",
  "tally": "tally-v1",
  "triage_settle": "triage-settle-v2"
}

Findings (3)

low — New test title says "default-branch lookup" where the change standardises on "default-branch read" everywhere else

  • claim: 01M3A1S4DE7VNAZ3YTB8E1T1XK
  • anchor: skills/audit-git-checkouts/scripts/render-audit-report.test.ts (snippet)
test("a fetched run whose default-branch lookup failed does not blame the fetch", () => {
  • lens: writing-quality · arm: default
  • verdicts: 1 valid / 0 invalid / 0 uncertain
  • disposition: none

What I examined: the two new tests in skills/audit-git-checkouts/scripts/render-audit-report.test.ts, the strings they assert on, and every occurrence of "lookup" and "read" in skills/audit-git-checkouts/ (grep -rn "lookup" . in that directory returns exactly one hit, this test title).

What the subject says: this change deliberately standardises on "read" for the ls-remote --symref origin HEAD step. stepFailure previously produced "origin fetch or default-branch lookup failed"; the diff removes that literal and staleCause now returns "origin fetch or default-branch read failed", matching repositoryFailures, which has long emitted "could not read the server's default branch". The new comment above staleCause likewise says "the fetch or the default-branch read failed". The test title added in the same change is the sole remaining "lookup": test("a fetched run whose default-branch lookup failed does not blame the fetch", ...) — and its own body asserts markdown.includes("| mirror | could not read the server's default branch |").

What goes wrong: the writing skill's "Use Precise Language" section requires one term for one concept. A test name is the label a failure prints, and this one names the step by a word that appears nowhere else in the skill, including in the output the test checks. Someone who greps for "default-branch read" while changing that wording finds the production string, the comment, and the sibling assertions, but not the test whose whole purpose is to pin that behaviour; someone reading a failure for "default-branch lookup" has to work out that it means the remoteHeadOk path. The cost is small and entirely in navigation, not behaviour.

Proposed correction: rename to test("a fetched run whose default-branch read failed does not blame the fetch", ...). This preserves the title's real content — the distinction the test exists for, that a run which fetched successfully but could not read the server's default branch must not be reported as a fetch failure, which the test's closing assert.ok(!markdown.includes("fetch failed")) enforces — while using the same word as the code and the report.

What would establish or refute this: the grep above, plus the diff's own replacement of "lookup" with "read" in stepFailure. If "lookup" were the project's established term for this step the finding would invert, but it survives in no other file in the skill.

low — The fast-forward eligibility policy is pasted into a report table cell, repeating references/checkout-updates.md in every affected row

  • claim: 01M3A1PRZWJZTTK3X95QTAQX9G
  • anchor: skills/audit-git-checkouts/scripts/render-audit-report.ts (snippet)
    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.`;
  • lens: writing-quality · arm: default
  • verdicts: 1 valid / 0 invalid / 0 uncertain
  • disposition: none

What I examined: defaultNotCurrentReason in skills/audit-git-checkouts/scripts/render-audit-report.ts, its caller primaryDecision, the formatTable/addSection code that consumes the returned string, the sibling why strings in the same function, the matching assertion in render-audit-report.test.ts ("rows name each checkout's state in plain words"), references/checkout-updates.md, and is_deferred_guidance_path in scripts/audit-checkouts.sh.

What the subject does: this string is the Why cell of the Needs your decision: not current table (addSection("Needs your decision: not current", ["Checkout", "Branch", "Ahead/behind", "Why"], decisions)). The change replaced the 9-word tail : not deferred guidance, or upstream changed the same paths with a 195-character second sentence stating the general eligibility policy. references/checkout-updates.md already states that policy, nearly word for word: "A dirty checkout qualifies only when every changed path is deferred guidance (AGENTS.md, .agents/**, at any depth) or first-party .gitmodules/Gitlink maintenance, and upstream did not touch those paths."

What goes wrong:

  1. The added sentence is constant. It is identical for every row that reaches this branch and says nothing about the checkout in the row, so it does not narrow which condition this checkout failed any more than the old tail did — it costs ~190 characters per row for no row-specific information. A user with three dirty-and-behind checkouts reads the same paragraph three times.
  2. It breaks the column's scale. Every other value this function and primaryDecision produce is a short lowercase fragment with no terminal punctuation: "not current", "no comparison with origin", "cached comparison", "2 local commits not pushed", "unborn branch (no commits yet)", "primary checkout is on fix/thing, not main". This is the only cell that is two sentences and the only one ending in a period. In a pipe table the row wraps and the Checkout/Branch/Ahead-behind columns stop lining up, which is exactly the scanning the decision table exists for.
  3. It duplicates the reference. Per the installed writing skill's "One Idea, One Place" ("Do not restate what ... an earlier sentence ... already says"; "Prefer a cheap authoritative lookup to copying a fact"), the eligibility rule's home is references/checkout-updates.md; a second copy inside a generated cell is a second place to keep in sync.
  4. "first-party submodule metadata" is project-local vocabulary left undefined in the report, which is read as standalone markdown. The reference spells the same concept as "first-party .gitmodules/Gitlink maintenance"; the driver implements it as .gitmodules or a configured submodule path carrying a branch/tag selector in a non-third-party/ checkout. A reader of the report alone cannot tell which files that names.

Proposed correction: restore a short cell and leave the rule where it lives, e.g. behind; uncommitted changes (4 files) block the fast-forward (see references/checkout-updates.md), or keep the previous concise tail naming the two candidate causes. That preserves the useful meaning — this checkout is behind, and its dirt is why the driver did not fast-forward it — and preserves the pointer to the full rule without pasting it per row.

What would establish or refute this: rendering a report containing two or more default-needs-attention checkouts that are both dirty and behind and looking at the resulting table. I did not run the renderer; the reasoning is from the format string, formatTable, and the test assertion that quotes the full cell verbatim. If the intended audience only ever reads this report through a tool that reflows cells, the alignment half of the cost would not apply — the duplication and the undefined term would remain.

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

  • claim: 01M3A1RB98WSM0QTKF45ZQYH29
  • anchor: skills/audit-git-checkouts/scripts/render-audit-report.ts (snippet)
// Why a fetched run left a repository's comparisons stale, worded as its Failures row; null on a cached run.
  • lens: writing-quality · arm: default
  • verdicts: 1 valid / 0 invalid / 0 uncertain
  • disposition: none

What I examined: the staleCause comment and function body in skills/audit-git-checkouts/scripts/render-audit-report.ts, repositoryFailures in the same file, the three call sites of the returned cause (stepFailure, defaultNotCurrentReason, primaryDecision, all reached from the per-repository loop in formatAuditReport), the two new tests in render-audit-report.test.ts, and the freshness logic in scripts/audit-checkouts.sh, where comparison_fresh=true is set only when fetch_ok = true and remote_head_ok = true.

What the subject says: the comment opens "Why a fetched run left a repository's comparisons stale, worded as its Failures row; null on a cached run." It then adds "Only the first cause is named: the Failures table lists both."

What goes wrong — the stated contract does not hold for the branch the comment itself introduces. repositoryFailures returns exactly one row for a salvaged repository: audit aborted: ${firstLine(repository.fatalError)}. It never emits the text staleCause returns on that same branch, "origin fetch or default-branch read failed", and it cannot "list both" causes there, because a salvaged record keeps no fetch or default-branch fields to report on. The change's own test proves the mismatch: "a salvaged repository record renders as an aborted audit with its worktrees" asserts both | app | audit aborted: amending the removal record for /dev/app-x failed | and | app-y | not checked for removal: origin fetch or default-branch read failed |. A reader who trusts the first sentence goes to the Failures table to find the matching row and finds none. For the other two branches the promise does hold: "could not read the server's default branch" matches its Failures row exactly, and "fetch failed" is the prefix of fetch failed: <first line of output>.

A second gap: the comment documents the driver's invariant but not the precondition the callers depend on. staleCause is evaluated once per repository (const cause = staleCause(repository, report.fetched);) whether or not anything is stale, and its last line returns the flat assertion "could not read the server's default branch" without consulting defaultBranchResolution.remoteHeadOk. On a completely healthy fetched run the variable therefore holds a sentence blaming a failure that did not happen. Today that is harmless because all three consumers read cause only inside a stale branch (outcome === "operational/stale-comparison", or !comparison.fresh); I checked each one. The comment never states that restriction, so nothing warns the next caller.

Proposed correction — state the contract the callers actually rely on and drop the cross-reference promise that does not survive the first branch. For example: "The cause of a repository's stale comparisons, phrased for a report cell; null on a cached run. Read it only where a comparison is stale: the driver marks one stale only when the fetch or the default-branch read failed, which is what makes the fall-through safe there and wrong anywhere else. A fetch failure wins when both failed; the Failures table names both. A salvaged record keeps neither field, so its cause names both possibilities and has no Failures row of its own." That keeps what the comment usefully carries — why the fall-through may assert a default-branch failure without checking remoteHeadOk, and why the salvaged wording stays vague — and removes a pointer the reader cannot follow.

What would establish or refute this: reading repositoryFailures beside staleCause for a repository with fatalError set, which the new salvaged-record test already exercises. I traced the code and the tests; I did not execute them.

Other claims

  • grounding-pending (0)
  • ungrounded (0)
  • rejected (0)
  • duplicate-of (0)
  • unadjudicated (0)

Coverage

Coverage pass: 01M3A1GC9J330DMZ65WW9S7G5T
Accounting: complete
Slot health: healthy

lens part arm unit status runs loss
general-bug whole default no-claims 1 no
writing-quality whole default claims-emitted 1 no
test-trimming whole default no-claims 1 no
<!-- review:summary --> **Review** `01M3A1GC7R779B1PQQD2XNHPQ4` — head `23b9fc22adbed95b4c63de29adec8a0f32b4a224` # Review — j4k-oss/agent-skills @ ceb23322b0b4 Scope: diff against base tree `d1e95321d499` Status: dispatched — coverage complete (3/3 slots terminal) Facts: current review-wide projection Computed under: ```json { "abandonment": "abandonment-v1", "anchor_recipe": 1, "batch_policy": "batch-v1", "coverage": "coverage-v3", "dispatch_policy": "dispatch-v2", "grounder_version": 1, "grounding_read_rule": "grounding-read-v1", "promotion_policy": "promotion-v1", "report": "report-v3", "tally": "tally-v1", "triage_settle": "triage-settle-v2" } ``` ## Findings (3) ### low — New test title says "default-branch lookup" where the change standardises on "default-branch read" everywhere else - claim: `01M3A1S4DE7VNAZ3YTB8E1T1XK` - anchor: `skills/audit-git-checkouts/scripts/render-audit-report.test.ts` (snippet) ``` test("a fetched run whose default-branch lookup failed does not blame the fetch", () => { ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - disposition: none > What I examined: the two new tests in `skills/audit-git-checkouts/scripts/render-audit-report.test.ts`, the strings they assert on, and every occurrence of "lookup" and "read" in `skills/audit-git-checkouts/` (`grep -rn "lookup" .` in that directory returns exactly one hit, this test title). > > What the subject says: this change deliberately standardises on "read" for the `ls-remote --symref origin HEAD` step. `stepFailure` previously produced "origin fetch or default-branch lookup failed"; the diff removes that literal and `staleCause` now returns "origin fetch or default-branch read failed", matching `repositoryFailures`, which has long emitted "could not read the server's default branch". The new comment above `staleCause` likewise says "the fetch or the default-branch read failed". The test title added in the same change is the sole remaining "lookup": `test("a fetched run whose default-branch lookup failed does not blame the fetch", ...)` — and its own body asserts `markdown.includes("| mirror | could not read the server's default branch |")`. > > What goes wrong: the writing skill's "Use Precise Language" section requires one term for one concept. A test name is the label a failure prints, and this one names the step by a word that appears nowhere else in the skill, including in the output the test checks. Someone who greps for "default-branch read" while changing that wording finds the production string, the comment, and the sibling assertions, but not the test whose whole purpose is to pin that behaviour; someone reading a failure for "default-branch lookup" has to work out that it means the `remoteHeadOk` path. The cost is small and entirely in navigation, not behaviour. > > Proposed correction: rename to `test("a fetched run whose default-branch read failed does not blame the fetch", ...)`. This preserves the title's real content — the distinction the test exists for, that a run which fetched successfully but could not read the server's default branch must not be reported as a fetch failure, which the test's closing `assert.ok(!markdown.includes("fetch failed"))` enforces — while using the same word as the code and the report. > > What would establish or refute this: the grep above, plus the diff's own replacement of "lookup" with "read" in `stepFailure`. If "lookup" were the project's established term for this step the finding would invert, but it survives in no other file in the skill. ### low — The fast-forward eligibility policy is pasted into a report table cell, repeating references/checkout-updates.md in every affected row - claim: `01M3A1PRZWJZTTK3X95QTAQX9G` - anchor: `skills/audit-git-checkouts/scripts/render-audit-report.ts` (snippet) ``` 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.`; ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - disposition: none > What I examined: `defaultNotCurrentReason` in `skills/audit-git-checkouts/scripts/render-audit-report.ts`, its caller `primaryDecision`, the `formatTable`/`addSection` code that consumes the returned string, the sibling `why` strings in the same function, the matching assertion in `render-audit-report.test.ts` ("rows name each checkout's state in plain words"), `references/checkout-updates.md`, and `is_deferred_guidance_path` in `scripts/audit-checkouts.sh`. > > What the subject does: this string is the `Why` cell of the `Needs your decision: not current` table (`addSection("Needs your decision: not current", ["Checkout", "Branch", "Ahead/behind", "Why"], decisions)`). The change replaced the 9-word tail `: not deferred guidance, or upstream changed the same paths` with a 195-character second sentence stating the general eligibility policy. `references/checkout-updates.md` already states that policy, nearly word for word: "A dirty checkout qualifies only when every changed path is deferred guidance (`AGENTS.md`, `.agents/**`, at any depth) or first-party `.gitmodules`/Gitlink maintenance, and upstream did not touch those paths." > > What goes wrong: > > 1. The added sentence is constant. It is identical for every row that reaches this branch and says nothing about the checkout in the row, so it does not narrow which condition this checkout failed any more than the old tail did — it costs ~190 characters per row for no row-specific information. A user with three dirty-and-behind checkouts reads the same paragraph three times. > 2. It breaks the column's scale. Every other value this function and `primaryDecision` produce is a short lowercase fragment with no terminal punctuation: "not current", "no comparison with origin", "cached comparison", "2 local commits not pushed", "unborn branch (no commits yet)", "primary checkout is on fix/thing, not main". This is the only cell that is two sentences and the only one ending in a period. In a pipe table the row wraps and the Checkout/Branch/Ahead-behind columns stop lining up, which is exactly the scanning the decision table exists for. > 3. It duplicates the reference. Per the installed writing skill's "One Idea, One Place" ("Do not restate what ... an earlier sentence ... already says"; "Prefer a cheap authoritative lookup to copying a fact"), the eligibility rule's home is `references/checkout-updates.md`; a second copy inside a generated cell is a second place to keep in sync. > 4. "first-party submodule metadata" is project-local vocabulary left undefined in the report, which is read as standalone markdown. The reference spells the same concept as "first-party `.gitmodules`/Gitlink maintenance"; the driver implements it as `.gitmodules` or a configured submodule path carrying a `branch`/`tag` selector in a non-`third-party/` checkout. A reader of the report alone cannot tell which files that names. > > Proposed correction: restore a short cell and leave the rule where it lives, e.g. `behind; uncommitted changes (4 files) block the fast-forward (see references/checkout-updates.md)`, or keep the previous concise tail naming the two candidate causes. That preserves the useful meaning — this checkout is behind, and its dirt is why the driver did not fast-forward it — and preserves the pointer to the full rule without pasting it per row. > > What would establish or refute this: rendering a report containing two or more `default-needs-attention` checkouts that are both dirty and behind and looking at the resulting table. I did not run the renderer; the reasoning is from the format string, `formatTable`, and the test assertion that quotes the full cell verbatim. If the intended audience only ever reads this report through a tool that reflows cells, the alignment half of the cost would not apply — the duplication and the undefined term would remain. ### 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 - claim: `01M3A1RB98WSM0QTKF45ZQYH29` - anchor: `skills/audit-git-checkouts/scripts/render-audit-report.ts` (snippet) ``` // Why a fetched run left a repository's comparisons stale, worded as its Failures row; null on a cached run. ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - disposition: none > What I examined: the `staleCause` comment and function body in `skills/audit-git-checkouts/scripts/render-audit-report.ts`, `repositoryFailures` in the same file, the three call sites of the returned `cause` (`stepFailure`, `defaultNotCurrentReason`, `primaryDecision`, all reached from the per-repository loop in `formatAuditReport`), the two new tests in `render-audit-report.test.ts`, and the freshness logic in `scripts/audit-checkouts.sh`, where `comparison_fresh=true` is set only when `fetch_ok = true` and `remote_head_ok = true`. > > What the subject says: the comment opens "Why a fetched run left a repository's comparisons stale, worded as its Failures row; null on a cached run." It then adds "Only the first cause is named: the Failures table lists both." > > What goes wrong — the stated contract does not hold for the branch the comment itself introduces. `repositoryFailures` returns exactly one row for a salvaged repository: `audit aborted: ${firstLine(repository.fatalError)}`. It never emits the text `staleCause` returns on that same branch, "origin fetch or default-branch read failed", and it cannot "list both" causes there, because a salvaged record keeps no fetch or default-branch fields to report on. The change's own test proves the mismatch: "a salvaged repository record renders as an aborted audit with its worktrees" asserts both `| app | audit aborted: amending the removal record for /dev/app-x failed |` and `| app-y | not checked for removal: origin fetch or default-branch read failed |`. A reader who trusts the first sentence goes to the Failures table to find the matching row and finds none. For the other two branches the promise does hold: "could not read the server's default branch" matches its Failures row exactly, and "fetch failed" is the prefix of `fetch failed: <first line of output>`. > > A second gap: the comment documents the driver's invariant but not the precondition the callers depend on. `staleCause` is evaluated once per repository (`const cause = staleCause(repository, report.fetched);`) whether or not anything is stale, and its last line returns the flat assertion "could not read the server's default branch" without consulting `defaultBranchResolution.remoteHeadOk`. On a completely healthy fetched run the variable therefore holds a sentence blaming a failure that did not happen. Today that is harmless because all three consumers read `cause` only inside a stale branch (`outcome === "operational/stale-comparison"`, or `!comparison.fresh`); I checked each one. The comment never states that restriction, so nothing warns the next caller. > > Proposed correction — state the contract the callers actually rely on and drop the cross-reference promise that does not survive the first branch. For example: "The cause of a repository's stale comparisons, phrased for a report cell; null on a cached run. Read it only where a comparison is stale: the driver marks one stale only when the fetch or the default-branch read failed, which is what makes the fall-through safe there and wrong anywhere else. A fetch failure wins when both failed; the Failures table names both. A salvaged record keeps neither field, so its cause names both possibilities and has no Failures row of its own." That keeps what the comment usefully carries — why the fall-through may assert a default-branch failure without checking `remoteHeadOk`, and why the salvaged wording stays vague — and removes a pointer the reader cannot follow. > > What would establish or refute this: reading `repositoryFailures` beside `staleCause` for a repository with `fatalError` set, which the new salvaged-record test already exercises. I traced the code and the tests; I did not execute them. ## Other claims - grounding-pending (0) - ungrounded (0) - rejected (0) - duplicate-of (0) - unadjudicated (0) ## Coverage Coverage pass: 01M3A1GC9J330DMZ65WW9S7G5T Accounting: complete Slot health: healthy | lens | part | arm | unit status | runs | loss | | --- | --- | --- | --- | --- | --- | | general-bug | whole | default | no-claims | 1 | no | | writing-quality | whole | default | claims-emitted | 1 | no | | test-trimming | whole | default | no-claims | 1 | no |
@ -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 · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined. primaryDecision in render-audit-report.ts (the manual-review case this change touches), its sibling branches in the same switch, defaultNotCurrentReason, formatAheadBehind, the table headers built in formatAuditReport, both tests that assert this row (a fetched run never counts an uncompared checkout as current and the new a fetched run whose default-branch lookup failed does not blame the fetch), and the freshness rule in scripts/audit-checkouts.sh, where comparison_fresh=true requires both fetch_ok = true and remote_head_ok = true.

What the subject does. On a fetched run whose comparison is stale, the anchored line returns the reason not compared: origin fetch or default-branch lookup failed together with formatAheadBehind(comparison), which prints the stale cached numbers. I ran the renderer on the new test's fixture and on a variant with a non-zero cached distance (node against formatAuditReport, exported from this file):

## Needs your decision: not current

| Checkout | Branch | Ahead/behind | Why |
| --- | --- | --- | --- |
| mirror | main | +2/-9 | not compared: origin fetch or default-branch lookup failed |

The same worktree data on a --no-fetch run renders | mirror | main | +2/-9 | cached: 9 commits behind, 2 ahead |.

What goes wrong. One row makes three statements a reader cannot reconcile. The section heading asserts the checkout is not current; the Ahead/behind column prints a specific distance; the Why cell says nothing was compared. All three are unverified: the numbers come from cached origin/* refs that this run failed to refresh, and whether the checkout is current is exactly what the run could not determine. "not compared" is also inaccurate on its face — a comparison was made, against stale refs; what failed was refreshing it. The three sibling branches of the same manual-review case (unborn branch, unresolved default branch, no comparison with origin) all render aheadBehind: "n/a" when they have nothing verified to report, so this branch is the only place the table prints figures it simultaneously disclaims. The cost is concrete: SKILL.md tells the agent to work the "Needs your decision" rows one at a time and to use the report's plain wording, so a +0/-0 here reads as "in sync with origin" to a reader scanning the numeric column, when nothing about origin was verified. The neighbouring cached-run wording shows the report already knows how to label this class of number.

Recommended correction. Keep the cause, label the numbers: cached: ${count(comparison.behind, "commit")} behind, ${comparison.ahead} ahead; not re-checked: origin fetch or default-branch lookup failed, or fall back to aheadBehind: "n/a" as the sibling branches do. Either preserves the honest attribution this change introduced while removing the unlabelled figure. defaultNotCurrentReason already uses the short form cached comparison for the same idea in the default-needs-attention path.

What would establish or refute this. Rendering the fixture, which I did, shows the row as quoted. The claim would be refuted if the "Ahead/behind" column were documented elsewhere as always meaning cached refs; I grepped SKILL.md and references/ and found no such statement — SKILL.md says only that a --no-fetch run's comparisons may be stale, which does not cover a fetched run whose fetch or default-branch lookup failed.

claim 01M39S89QVG47KX5G0J8J46Y2R of review 01M39RX0SPHT4ZRJB7J3QKBQ2J

<!-- review:claim:01M39S89QVG47KX5G0J8J46Y2R --> **medium** — "not compared" is printed beside an unlabelled ahead/behind figure the run never verified lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > **What I examined.** `primaryDecision` in `render-audit-report.ts` (the `manual-review` case this change touches), its sibling branches in the same `switch`, `defaultNotCurrentReason`, `formatAheadBehind`, the table headers built in `formatAuditReport`, both tests that assert this row (`a fetched run never counts an uncompared checkout as current` and the new `a fetched run whose default-branch lookup failed does not blame the fetch`), and the freshness rule in `scripts/audit-checkouts.sh`, where `comparison_fresh=true` requires both `fetch_ok = true` and `remote_head_ok = true`. > > **What the subject does.** On a fetched run whose comparison is stale, the anchored line returns the reason `not compared: origin fetch or default-branch lookup failed` **together with** `formatAheadBehind(comparison)`, which prints the stale cached numbers. I ran the renderer on the new test's fixture and on a variant with a non-zero cached distance (`node` against `formatAuditReport`, exported from this file): > > ``` > ## Needs your decision: not current > > | Checkout | Branch | Ahead/behind | Why | > | --- | --- | --- | --- | > | mirror | main | +2/-9 | not compared: origin fetch or default-branch lookup failed | > ``` > > The same worktree data on a `--no-fetch` run renders `| mirror | main | +2/-9 | cached: 9 commits behind, 2 ahead |`. > > **What goes wrong.** One row makes three statements a reader cannot reconcile. The section heading asserts the checkout is *not current*; the Ahead/behind column prints a specific distance; the Why cell says nothing was compared. All three are unverified: the numbers come from cached `origin/*` refs that this run failed to refresh, and whether the checkout is current is exactly what the run could not determine. "not compared" is also inaccurate on its face — a comparison was made, against stale refs; what failed was refreshing it. The three sibling branches of the same `manual-review` case (unborn branch, unresolved default branch, no comparison with origin) all render `aheadBehind: "n/a"` when they have nothing verified to report, so this branch is the only place the table prints figures it simultaneously disclaims. The cost is concrete: `SKILL.md` tells the agent to work the "Needs your decision" rows one at a time and to use the report's plain wording, so a `+0/-0` here reads as "in sync with origin" to a reader scanning the numeric column, when nothing about origin was verified. The neighbouring cached-run wording shows the report already knows how to label this class of number. > > **Recommended correction.** Keep the cause, label the numbers: `cached: ${count(comparison.behind, "commit")} behind, ${comparison.ahead} ahead; not re-checked: origin fetch or default-branch lookup failed`, or fall back to `aheadBehind: "n/a"` as the sibling branches do. Either preserves the honest attribution this change introduced while removing the unlabelled figure. `defaultNotCurrentReason` already uses the short form `cached comparison` for the same idea in the `default-needs-attention` path. > > **What would establish or refute this.** Rendering the fixture, which I did, shows the row as quoted. The claim would be refuted if the "Ahead/behind" column were documented elsewhere as always meaning cached refs; I grepped `SKILL.md` and `references/` and found no such statement — `SKILL.md` says only that a `--no-fetch` run's comparisons may be stale, which does not cover a fetched run whose fetch or default-branch lookup failed. claim `01M39S89QVG47KX5G0J8J46Y2R` of review `01M39RX0SPHT4ZRJB7J3QKBQ2J`
Author
Owner

Fixed 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 example cached: 0 commits behind, 0 ahead; not re-checked: could not read the server's default branch. Kept the figures rather than n/a, because the last known distance still helps the user decide.

<!-- gh-feedback:reply-to:87424 --> Fixed 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 example `cached: 0 commits behind, 0 ahead; not re-checked: could not read the server's default branch`. Kept the figures rather than `n/a`, because the last known distance still helps the user decide.
jercik marked this conversation as resolved
fix(audit-git-checkouts): name the failure that left a comparison stale
All checks were successful
commit-msg / commitlint (pull_request) Successful in 24s
Node tests / node:test (pull_request) Successful in 54s
Review / Review (pull_request_target) Successful in 11m53s
e1768602aa
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>
jercik changed title from fix(audit-git-checkouts): an unread server default branch should not be reported as a failed fetch to fix(audit-git-checkouts): a stale comparison should name the failure that caused it 2026-09-24 13:40:01 +00:00
Author
Owner

Replying to review summary comment #87423

Three findings in this report had no inline thread, so their outcomes are recorded here. All are fixed in e176860.

  • defaultNotCurrentReason calls a failed refresh "cached" (low): agreed. Reproduced with a dirty default checkout on a failed fetch. It now reads cached 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".
  • One failure named two ways (low): agreed. The renderer now reads which step failed from the repository record, not a disjunction. The decision row, the dirty row, and 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 deferred guidance" is undefined in the report (low): agreed. The cell now reads 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.

> Replying to review summary comment #87423 Three findings in this report had no inline thread, so their outcomes are recorded here. All are fixed in e176860. - **`defaultNotCurrentReason` calls a failed refresh "cached" (low): agreed.** Reproduced with a dirty default checkout on a failed fetch. It now reads `cached 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". - **One failure named two ways (low): agreed.** The renderer now reads which step failed from the repository record, not a disjunction. The decision row, the dirty row, and `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 deferred guidance" is undefined in the report (low): agreed.** The cell now reads `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 · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the two tests added in this change to skills/audit-git-checkouts/scripts/render-audit-report.test.ts, the new staleCause() helper they exercise in render-audit-report.ts, and the driver that produces the fixture fields, skills/audit-git-checkouts/scripts/audit-checkouts.sh.

What the new helper does:

function staleCause(repository: Repository, fetched: boolean): string | null {
  if (!fetched) return null;
  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");
  return causes.join(", and ") || "comparison not refreshed";
}

It can emit one cause, two joined by ", and ", or the fallback. Every test that renders a stale cause fires exactly one:

  • the new a dirty default checkout on a failed fetch names the cause beside its cached comparison builds its repository with { fetch: { attempted: true, ok: false, output: "fatal: unable to access\n" } }, leaving the repository() helper's default defaultBranchResolution: { remoteHeadOk: true } in place;
  • the pre-existing a fetched run never counts an uncompared checkout as current, whose assertion this change rewrote to "| gone | main | +0/-0 | cached: 0 commits behind, 0 ahead; not re-checked: fetch failed |", uses the same fetch-fails-but-remote-head-ok shape;
  • the new a fetched run whose default-branch lookup failed does not blame the fetch is the mirror case, remoteHeadOk: false with a successful fetch.

Why that fixture shape misrepresents "a failed fetch": in audit-checkouts.sh the repository worker hits origin twice, git -C "$checkout_path" ls-remote --symref origin HEAD (which sets remote_head_ok) and then git -C "$checkout_path" fetch --prune origin ... (which sets fetch_ok), and comparisons go stale only when [ "$fetch_ok" = true ] && [ "$remote_head_ok" = true ] fails. An unreachable, down, or auth-rejecting origin — the ordinary way a fetch fails — fails both commands, so the report carries fetch.ok: false and remoteHeadOk: false together. I rendered exactly that report through formatAuditReport (same worktree as the new test, only remoteHeadOk flipped to false) and got:

| app | main | +0/-5 | cached comparison, 4 uncommitted files; not re-checked: fetch failed, and could not read the server's default branch |

No test in the file asserts that string, or any two-cause string.

Concrete regression that stays green: I mutated the implementation and ran node --test skills/audit-git-checkouts/scripts/render-audit-report.test.ts against the suite (17 tests, all passing on the unmutated tree).

  • causes.join(", and ") -> causes.join(", "): 17 pass, 0 fail.
  • causes.join(", and ") -> causes.reverse().join(", and "): 17 pass, 0 fail.

So the wording and ordering of the message users actually see on the common failure — an unreachable origin — is unprotected, while the suite reads as though the failed-fetch rendering is pinned. By contrast, mutations that break the single-cause paths are caught: deleting the fetch.ok cause fails the two fetch tests, deleting the remoteHeadOk cause fails the default-branch test, and stripping the ; not re-checked: ... suffix from defaultNotCurrentReason fails this test.

Suggested repair, which keeps everything this test already protects: in the new test's repository override, set defaultBranchResolution: { remoteHeadOk: false } alongside the failed fetch so the fixture matches what the driver emits for an unreachable origin, and assert the joined wording cached comparison, 4 uncommitted files; not re-checked: fetch failed, and could not read the server's default branch. That retains the cached-comparison-plus-dirty-count suffix check on defaultNotCurrentReason and additionally pins the separator and cause order; the existing gone test still covers fetch-failure-only, and the new default-branch test still covers remote-head-failure-only.

What would refute this: evidence that the driver routinely reports fetch.ok: false with remoteHeadOk: true (a fetch that fails after a successful ls-remote — a ref-update or local-disk failure — is possible, so that combination is not unrealistic, only not the dominant one), or a test elsewhere in the repository that pins the two-cause message. I grepped this test file; audit-checkouts.test.mjs is the driver's shell test and does not call formatAuditReport. I did not find such a test.

claim 01M39THRWFKFEXFX7CK9TECEBD of review 01M39T9TR976SWXG1Y2N5JNNRQ

<!-- review:claim:01M39THRWFKFEXFX7CK9TECEBD --> **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` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the two tests added in this change to `skills/audit-git-checkouts/scripts/render-audit-report.test.ts`, the new `staleCause()` helper they exercise in `render-audit-report.ts`, and the driver that produces the fixture fields, `skills/audit-git-checkouts/scripts/audit-checkouts.sh`. > > What the new helper does: > > ``` > function staleCause(repository: Repository, fetched: boolean): string | null { > if (!fetched) return null; > 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"); > return causes.join(", and ") || "comparison not refreshed"; > } > ``` > > It can emit one cause, two joined by `", and "`, or the fallback. Every test that renders a stale cause fires exactly one: > > - the new `a dirty default checkout on a failed fetch names the cause beside its cached comparison` builds its repository with `{ fetch: { attempted: true, ok: false, output: "fatal: unable to access\n" } }`, leaving the `repository()` helper's default `defaultBranchResolution: { remoteHeadOk: true }` in place; > - the pre-existing `a fetched run never counts an uncompared checkout as current`, whose assertion this change rewrote to `"| gone | main | +0/-0 | cached: 0 commits behind, 0 ahead; not re-checked: fetch failed |"`, uses the same fetch-fails-but-remote-head-ok shape; > - the new `a fetched run whose default-branch lookup failed does not blame the fetch` is the mirror case, `remoteHeadOk: false` with a successful fetch. > > Why that fixture shape misrepresents "a failed fetch": in `audit-checkouts.sh` the repository worker hits origin twice, `git -C "$checkout_path" ls-remote --symref origin HEAD` (which sets `remote_head_ok`) and then `git -C "$checkout_path" fetch --prune origin ...` (which sets `fetch_ok`), and comparisons go stale only when `[ "$fetch_ok" = true ] && [ "$remote_head_ok" = true ]` fails. An unreachable, down, or auth-rejecting origin — the ordinary way a fetch fails — fails both commands, so the report carries `fetch.ok: false` *and* `remoteHeadOk: false` together. I rendered exactly that report through `formatAuditReport` (same worktree as the new test, only `remoteHeadOk` flipped to `false`) and got: > > ``` > | app | main | +0/-5 | cached comparison, 4 uncommitted files; not re-checked: fetch failed, and could not read the server's default branch | > ``` > > No test in the file asserts that string, or any two-cause string. > > Concrete regression that stays green: I mutated the implementation and ran `node --test skills/audit-git-checkouts/scripts/render-audit-report.test.ts` against the suite (17 tests, all passing on the unmutated tree). > > - `causes.join(", and ")` -> `causes.join(", ")`: 17 pass, 0 fail. > - `causes.join(", and ")` -> `causes.reverse().join(", and ")`: 17 pass, 0 fail. > > So the wording and ordering of the message users actually see on the common failure — an unreachable origin — is unprotected, while the suite reads as though the failed-fetch rendering is pinned. By contrast, mutations that break the single-cause paths are caught: deleting the `fetch.ok` cause fails the two fetch tests, deleting the `remoteHeadOk` cause fails the default-branch test, and stripping the `; not re-checked: ...` suffix from `defaultNotCurrentReason` fails this test. > > Suggested repair, which keeps everything this test already protects: in the new test's repository override, set `defaultBranchResolution: { remoteHeadOk: false }` alongside the failed fetch so the fixture matches what the driver emits for an unreachable origin, and assert the joined wording `cached comparison, 4 uncommitted files; not re-checked: fetch failed, and could not read the server's default branch`. That retains the cached-comparison-plus-dirty-count suffix check on `defaultNotCurrentReason` and additionally pins the separator and cause order; the existing `gone` test still covers fetch-failure-only, and the new default-branch test still covers remote-head-failure-only. > > What would refute this: evidence that the driver routinely reports `fetch.ok: false` with `remoteHeadOk: true` (a fetch that fails after a successful `ls-remote` — a ref-update or local-disk failure — is possible, so that combination is not unrealistic, only not the dominant one), or a test elsewhere in the repository that pins the two-cause message. I grepped this test file; `audit-checkouts.test.mjs` is the driver's shell test and does not call `formatAuditReport`. I did not find such a test. claim `01M39THRWFKFEXFX7CK9TECEBD` of review `01M39T9TR976SWXG1Y2N5JNNRQ`
Author
Owner

Fixed in 1fda19e. The failed-fetch test now also sets remoteHeadOk: 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.

<!-- gh-feedback:reply-to:87478 --> Fixed in 1fda19e. The failed-fetch test now also sets `remoteHeadOk: 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.
jercik marked this conversation as resolved
@ -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 · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the causes.join(", and ") in the new staleCause helper of skills/audit-git-checkouts/scripts/render-audit-report.ts, and the rendered report it feeds through defaultNotCurrentReason and the manual-review arm of primaryDecision.

I ran the renderer on a report with fetched: true and a repository whose remote is unreachable — fetch: { attempted: true, ok: false, output: "fatal: unable to access" } and defaultBranchResolution: { remoteHeadOk: false } — with one dirty default-needs-attention worktree. Observed output:

Failures

| app | fetch failed: fatal: unable to access |
| app | could not read the server's default branch |

Needs your decision: not current

| app | main | +0/-5 | cached comparison, 4 uncommitted files; not re-checked: fetch failed, and could not read the server's default branch |

Two things go wrong in that last cell:

  1. ", and " is the wrong joiner for a two-item list. English puts no comma before "and" between two items; "fetch failed, and could not read the server's default branch" reads as a comma splice or as a list whose first element got lost. join(", and ") also degrades further if a third cause is ever added, producing "a, and b, and c" rather than "a, b, and c".

  2. This is also the common case, not a corner. In audit-checkouts.sh both sub-steps hit the same remote — git ls-remote --symref origin HEAD sets remote_head_ok, and git fetch --prune origin sets fetch_ok — so an unreachable or unauthenticated origin fails both, and the doubled cause is what a network or credential outage normally renders. The result is a 60-character suffix restating, word for word, the two Failures rows the reader has already passed a few lines above, in a column whose job is to name what the user must decide.

Suggested correction: join with "; " (or with " and " for two items), and consider collapsing to the first cause, since the Failures table carries the full list: not re-checked: ${causes[0]} keeps the decision row short and loses nothing a reader cannot find above.

What would establish or refute this: the rendered text above is observed output from the reviewed code, not inference. The claim that both sub-steps fail together on an unreachable remote rests on reading the two git invocations in audit-checkouts.sh; I did not reproduce a network failure against a real remote, and a remote that serves ls-remote but rejects a fetch would produce only one cause.

claim 01M39TJHTCG2WJ4NFXBAG01348 of review 01M39T9TR976SWXG1Y2N5JNNRQ

<!-- review:claim:01M39TJHTCG2WJ4NFXBAG01348 --> **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` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the `causes.join(", and ")` in the new `staleCause` helper of `skills/audit-git-checkouts/scripts/render-audit-report.ts`, and the rendered report it feeds through `defaultNotCurrentReason` and the `manual-review` arm of `primaryDecision`. > > I ran the renderer on a report with `fetched: true` and a repository whose remote is unreachable — `fetch: { attempted: true, ok: false, output: "fatal: unable to access" }` and `defaultBranchResolution: { remoteHeadOk: false }` — with one dirty `default-needs-attention` worktree. Observed output: > > ## Failures > | app | fetch failed: fatal: unable to access | > | app | could not read the server's default branch | > > ## Needs your decision: not current > | app | main | +0/-5 | cached comparison, 4 uncommitted files; not re-checked: fetch failed, and could not read the server's default branch | > > Two things go wrong in that last cell: > > 1. ", and " is the wrong joiner for a two-item list. English puts no comma before "and" between two items; "fetch failed, and could not read the server's default branch" reads as a comma splice or as a list whose first element got lost. `join(", and ")` also degrades further if a third cause is ever added, producing "a, and b, and c" rather than "a, b, and c". > > 2. This is also the common case, not a corner. In `audit-checkouts.sh` both sub-steps hit the same remote — `git ls-remote --symref origin HEAD` sets `remote_head_ok`, and `git fetch --prune origin` sets `fetch_ok` — so an unreachable or unauthenticated origin fails both, and the doubled cause is what a network or credential outage normally renders. The result is a 60-character suffix restating, word for word, the two Failures rows the reader has already passed a few lines above, in a column whose job is to name what the user must decide. > > Suggested correction: join with "; " (or with " and " for two items), and consider collapsing to the first cause, since the Failures table carries the full list: `not re-checked: ${causes[0]}` keeps the decision row short and loses nothing a reader cannot find above. > > What would establish or refute this: the rendered text above is observed output from the reviewed code, not inference. The claim that both sub-steps fail together on an unreachable remote rests on reading the two git invocations in `audit-checkouts.sh`; I did not reproduce a network failure against a real remote, and a remote that serves `ls-remote` but rejects a fetch would produce only one cause. claim `01M39TJHTCG2WJ4NFXBAG01348` of review `01M39T9TR976SWXG1Y2N5JNNRQ`
Author
Owner

Fixed 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.

<!-- gh-feedback:reply-to:87482 --> Fixed 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.
jercik marked this conversation as resolved
@ -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 · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the new staleCause helper in skills/audit-git-checkouts/scripts/render-audit-report.ts, its two call sites (defaultNotCurrentReason and the manual-review arm of primaryDecision, which both append "; not re-checked: ${staleCause}"), the doc comment directly above it, and repositoryFailures, which builds the Failures table rows the comment refers to.

What the subject does: when the run fetched and the helper finds no named cause, it falls back to the literal "comparison not refreshed". I ran the renderer against a report with fetched: true, a repository whose fetch.ok is true and whose defaultBranchResolution.remoteHeadOk is true, and a default-needs-attention worktree with defaultComparison: { ahead: 0, behind: 5, fresh: false }. The rendered row:

| app | main | +0/-5 | cached comparison; not re-checked: comparison not refreshed |

Two problems, both in the writing:

  1. The clause is a tautology. "cached comparison" already tells the reader the numbers are stale; "not re-checked: comparison not refreshed" restates that in different words and names no cause. Every other value this helper returns ("fetch failed", "could not read the server's default branch", "audit aborted") answers why; this one answers the question with the question. The "; not re-checked: ..." suffix exists precisely to carry a cause, so when there is no cause the suffix should be omitted — i.e. return null, which both call sites already handle by printing nothing.

  2. It contradicts the comment two lines above it: "Why a fetched run left a repository's comparisons stale, in the Failures table's words; null on a cached run." "comparison not refreshed" is not in the Failures table's words — repositoryFailures emits "fetch failed: ...", "could not read the server's default branch", "default branch unresolved", "worktree listing failed: ...", "stale-registration prune failed: ..." and "audit aborted: ...", and nothing resembling this string. A reader who trusts the comment will go looking in the Failures table for a row that is never there, and in this scenario the Failures table is in fact empty.

Suggested correction: return null instead of the fallback, and drop "in the Failures table's words" or narrow it to the causes that really are shared ("worded as in the Failures table"). If a fallback string must stay, it should name the gap rather than restate the label — for example "no cause recorded".

What would establish or refute this: the reproduction above is the decisive evidence for the rendered text. On the reachability question I could only get partway: in audit-checkouts.sh, comparison_fresh is set true only when [ "$fetch_ok" = true ] && [ "$remote_head_ok" = true ], so a report written by that driver appears never to combine fetched: true, both sub-steps succeeding, and fresh: false — meaning this fallback may be unreachable in practice today. That lowers the severity but does not remove the finding: it is a user-facing string on a public exported function (formatAuditReport is exported and the renderer takes any schema-5 JSON), and the comment above it is wrong as written either way.

claim 01M39TJ13FR8S0WRQ4MWX0ARZR of review 01M39T9TR976SWXG1Y2N5JNNRQ

<!-- review:claim:01M39TJ13FR8S0WRQ4MWX0ARZR --> **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` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the new `staleCause` helper in `skills/audit-git-checkouts/scripts/render-audit-report.ts`, its two call sites (`defaultNotCurrentReason` and the `manual-review` arm of `primaryDecision`, which both append "; not re-checked: ${staleCause}"), the doc comment directly above it, and `repositoryFailures`, which builds the Failures table rows the comment refers to. > > What the subject does: when the run fetched and the helper finds no named cause, it falls back to the literal "comparison not refreshed". I ran the renderer against a report with `fetched: true`, a repository whose `fetch.ok` is `true` and whose `defaultBranchResolution.remoteHeadOk` is `true`, and a `default-needs-attention` worktree with `defaultComparison: { ahead: 0, behind: 5, fresh: false }`. The rendered row: > > | app | main | +0/-5 | cached comparison; not re-checked: comparison not refreshed | > > Two problems, both in the writing: > > 1. The clause is a tautology. "cached comparison" already tells the reader the numbers are stale; "not re-checked: comparison not refreshed" restates that in different words and names no cause. Every other value this helper returns ("fetch failed", "could not read the server's default branch", "audit aborted") answers *why*; this one answers the question with the question. The "; not re-checked: ..." suffix exists precisely to carry a cause, so when there is no cause the suffix should be omitted — i.e. return `null`, which both call sites already handle by printing nothing. > > 2. It contradicts the comment two lines above it: "Why a fetched run left a repository's comparisons stale, in the Failures table's words; null on a cached run." "comparison not refreshed" is not in the Failures table's words — `repositoryFailures` emits "fetch failed: ...", "could not read the server's default branch", "default branch unresolved", "worktree listing failed: ...", "stale-registration prune failed: ..." and "audit aborted: ...", and nothing resembling this string. A reader who trusts the comment will go looking in the Failures table for a row that is never there, and in this scenario the Failures table is in fact empty. > > Suggested correction: return `null` instead of the fallback, and drop "in the Failures table's words" or narrow it to the causes that really are shared ("worded as in the Failures table"). If a fallback string must stay, it should name the gap rather than restate the label — for example "no cause recorded". > > What would establish or refute this: the reproduction above is the decisive evidence for the rendered text. On the reachability question I could only get partway: in `audit-checkouts.sh`, `comparison_fresh` is set true only when `[ "$fetch_ok" = true ] && [ "$remote_head_ok" = true ]`, so a report written by that driver appears never to combine `fetched: true`, both sub-steps succeeding, and `fresh: false` — meaning this fallback may be unreachable in practice today. That lowers the severity but does not remove the finding: it is a user-facing string on a public exported function (`formatAuditReport` is exported and the renderer takes any schema-5 JSON), and the comment above it is wrong as written either way. claim `01M39TJ13FR8S0WRQ4MWX0ARZR` of review `01M39T9TR976SWXG1Y2N5JNNRQ`
Author
Owner

Fixed 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 than null because null marks a cached run, and a fetched run must never count an uncompared checkout as current.

<!-- gh-feedback:reply-to:87481 --> Fixed 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 than `null` because `null` marks a cached run, and a fetched run must never count an uncompared checkout as current.
jercik marked this conversation as resolved
@ -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 · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: defaultNotCurrentReason in skills/audit-git-checkouts/scripts/render-audit-report.ts, the authority it paraphrases (is_deferred_guidance_path and has_only_nonoverlapping_deferred_guidance in skills/audit-git-checkouts/scripts/audit-checkouts.sh), the driver's own --help text in that same script, and skills/audit-git-checkouts/references/checkout-updates.md.

The change replaces the project-local term "deferred guidance" with an inline enumeration. Making an abstract category operational with recognizable instances is the right move, but this enumeration does not match the rule it summarizes, on two points.

  1. "first-party" is dropped. is_deferred_guidance_path reaches its submodule-selector arm only after is_third_party_checkout "$checkout_path" && return 1 — a .gitmodules or Gitlink change in a checkout under a third-party/ path component never counts as deferred guidance. The two other descriptions of this same rule keep the qualifier: the --help text says "every local change is AGENTS.md, .agents/**, or first-party submodule selector maintenance", and references/checkout-updates.md says "every changed path is deferred guidance (AGENTS.md, .agents/**, at any depth) or first-party .gitmodules/Gitlink maintenance". SKILL.md gives the reason: "never edit .gitmodules, selectors, or Gitlinks under a third-party/ path."

  2. "at any depth" is dropped. The matcher's case arms are AGENTS.md|*/AGENTS.md|.agents/*|*/.agents/*, and the reference spells this out as ".agents/**, at any depth". Written as bare .agents/, the message reads as the repository-root directory only.

What goes wrong: this cell is the only explanation the user gets for why a dirty default checkout was not fast-forwarded, and it is wrong at exactly the boundary the skill cares most about. The owner of a dirty checkout under third-party/ whose single uncommitted change is .gitmodules is told the block is that the changes are not "submodule selector changes" — but it is a selector change; the real reason is that upstream owns selectors there. That sends the user hunting a nonexistent classification bug instead of looking at the third-party boundary. Symmetrically, a user with an uncommitted packages/web/.agents/notes.md reads the message as excluding a path the matcher accepts.

Proposed correction, keeping the concreteness this change added: "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".

What would establish or refute this: is_deferred_guidance_path in audit-checkouts.sh is decisive — its third-party early return and its four case arms are quoted above. Proof gap: I read the shell source rather than running the driver against a third-party checkout, so I did not observe that path end to end. The renderer string itself is confirmed by the source and by the exact-match assertion in render-audit-report.test.ts ("rows name each checkout's state in plain words").

claim 01M39TGHD5MBXZKY5NDFHSVCVW of review 01M39T9TR976SWXG1Y2N5JNNRQ

<!-- review:claim:01M39TGHD5MBXZKY5NDFHSVCVW --> **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` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: `defaultNotCurrentReason` in `skills/audit-git-checkouts/scripts/render-audit-report.ts`, the authority it paraphrases (`is_deferred_guidance_path` and `has_only_nonoverlapping_deferred_guidance` in `skills/audit-git-checkouts/scripts/audit-checkouts.sh`), the driver's own `--help` text in that same script, and `skills/audit-git-checkouts/references/checkout-updates.md`. > > The change replaces the project-local term "deferred guidance" with an inline enumeration. Making an abstract category operational with recognizable instances is the right move, but this enumeration does not match the rule it summarizes, on two points. > > 1. "first-party" is dropped. `is_deferred_guidance_path` reaches its submodule-selector arm only after `is_third_party_checkout "$checkout_path" && return 1` — a `.gitmodules` or Gitlink change in a checkout under a `third-party/` path component never counts as deferred guidance. The two other descriptions of this same rule keep the qualifier: the `--help` text says "every local change is AGENTS.md, .agents/**, or first-party submodule selector maintenance", and `references/checkout-updates.md` says "every changed path is deferred guidance (`AGENTS.md`, `.agents/**`, at any depth) or first-party `.gitmodules`/Gitlink maintenance". `SKILL.md` gives the reason: "never edit `.gitmodules`, selectors, or Gitlinks under a `third-party/` path." > > 2. "at any depth" is dropped. The matcher's case arms are `AGENTS.md|*/AGENTS.md|.agents/*|*/.agents/*`, and the reference spells this out as "`.agents/**`, at any depth". Written as bare `.agents/`, the message reads as the repository-root directory only. > > What goes wrong: this cell is the only explanation the user gets for why a dirty default checkout was not fast-forwarded, and it is wrong at exactly the boundary the skill cares most about. The owner of a dirty checkout under `third-party/` whose single uncommitted change is `.gitmodules` is told the block is that the changes are not "submodule selector changes" — but it is a selector change; the real reason is that upstream owns selectors there. That sends the user hunting a nonexistent classification bug instead of looking at the third-party boundary. Symmetrically, a user with an uncommitted `packages/web/.agents/notes.md` reads the message as excluding a path the matcher accepts. > > Proposed correction, keeping the concreteness this change added: "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". > > What would establish or refute this: `is_deferred_guidance_path` in `audit-checkouts.sh` is decisive — its third-party early return and its four case arms are quoted above. Proof gap: I read the shell source rather than running the driver against a third-party checkout, so I did not observe that path end to end. The renderer string itself is confirmed by the source and by the exact-match assertion in `render-audit-report.test.ts` ("rows name each checkout's state in plain words"). claim `01M39TGHD5MBXZKY5NDFHSVCVW` of review `01M39T9TR976SWXG1Y2N5JNNRQ`

low — 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 · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the rendered sentence this template produces, as asserted verbatim in skills/audit-git-checkouts/scripts/render-audit-report.test.ts under the test "rows name each checkout's state in plain words":

| lib | main | +0/-5 | behind; uncommitted changes (4 files) block the fast-forward: not all are AGENTS.md, .agents/, or submodule selector changes, or upstream changed the same paths |

What goes wrong: after the colon the sentence is a disjunction of two conditions, but the first condition is itself an "or" list, and nothing in the punctuation separates the two levels. The reader meets four comma-and-or-joined fragments in a row and parses them as one flat list of four alternatives:

  • AGENTS.md
  • .agents/
  • submodule selector changes
  • upstream changed the same paths

Under that reading the last item is nonsense as a list member — "not all are ... upstream changed the same paths" is not a grammatical predicate — so the reader has to back up and re-parse. Only the second reading is intended: either (a) some uncommitted path is outside the deferred-guidance set, or (b) upstream also touched those paths. This is the "Why" cell of the Needs-your-decision table, the one line a user reads to decide what to do about a checkout that was not fast-forwarded; a sentence that has to be read twice to find its shape is a real cost there, and the previous wording ("not deferred guidance, or upstream changed the same paths") had only one "or" and did not have this problem.

The cheapest fix is to punctuate the two levels differently — a semicolon before the outer alternative, so the commas belong unambiguously to the inner list: "... 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". Naming the two conditions explicitly is clearer still: "... because the changes are not all deferred guidance (AGENTS.md, .agents/** at any depth, first-party submodule selectors), or because upstream changed the same paths". Either keeps the concrete instances this change introduced, which are worth keeping.

What would establish or refute this: the rendered string is fixed text with no variable parts after the colon, so the ambiguity is present in every instance of this row — the assertion quoted above pins it. This is a judgment about readability rather than a behavior claim; nothing at runtime changes either way. The related accuracy problem in the same sentence (the missing "first-party" and "at any depth" qualifiers) is filed separately; the rewrites above fold both fixes together.

claim 01M39TH6XW0VWQ54KVC9CQ1W4P of review 01M39T9TR976SWXG1Y2N5JNNRQ

<!-- review:claim:01M39TH6XW0VWQ54KVC9CQ1W4P --> **low** — 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` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the rendered sentence this template produces, as asserted verbatim in `skills/audit-git-checkouts/scripts/render-audit-report.test.ts` under the test "rows name each checkout's state in plain words": > > | lib | main | +0/-5 | behind; uncommitted changes (4 files) block the fast-forward: not all are AGENTS.md, .agents/, or submodule selector changes, or upstream changed the same paths | > > What goes wrong: after the colon the sentence is a disjunction of two conditions, but the first condition is itself an "or" list, and nothing in the punctuation separates the two levels. The reader meets four comma-and-or-joined fragments in a row and parses them as one flat list of four alternatives: > > - AGENTS.md > - .agents/ > - submodule selector changes > - upstream changed the same paths > > Under that reading the last item is nonsense as a list member — "not all are ... upstream changed the same paths" is not a grammatical predicate — so the reader has to back up and re-parse. Only the second reading is intended: either (a) some uncommitted path is outside the deferred-guidance set, or (b) upstream also touched those paths. This is the "Why" cell of the Needs-your-decision table, the one line a user reads to decide what to do about a checkout that was not fast-forwarded; a sentence that has to be read twice to find its shape is a real cost there, and the previous wording ("not deferred guidance, or upstream changed the same paths") had only one "or" and did not have this problem. > > The cheapest fix is to punctuate the two levels differently — a semicolon before the outer alternative, so the commas belong unambiguously to the inner list: "... 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". Naming the two conditions explicitly is clearer still: "... because the changes are not all deferred guidance (AGENTS.md, .agents/** at any depth, first-party submodule selectors), or because upstream changed the same paths". Either keeps the concrete instances this change introduced, which are worth keeping. > > What would establish or refute this: the rendered string is fixed text with no variable parts after the colon, so the ambiguity is present in every instance of this row — the assertion quoted above pins it. This is a judgment about readability rather than a behavior claim; nothing at runtime changes either way. The related accuracy problem in the same sentence (the missing "first-party" and "at any depth" qualifiers) is filed separately; the rewrites above fold both fixes together. claim `01M39TH6XW0VWQ54KVC9CQ1W4P` of review `01M39T9TR976SWXG1Y2N5JNNRQ`
Author
Owner

Fixed in 1fda19e. Confirmed the third-party early return and the */.agents/* arms in is_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".

<!-- gh-feedback:reply-to:87479 --> Fixed in 1fda19e. Confirmed the third-party early return and the `*/.agents/*` arms in `is_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".
Author
Owner

Fixed in 1fda19e by the same edit: a semicolon now separates the two conditions, so the commas belong to the inner list.

<!-- gh-feedback:reply-to:87480 --> Fixed in 1fda19e by the same edit: a semicolon now separates the two conditions, so the commas belong to the inner list.
jercik marked this conversation as resolved
fix(audit-git-checkouts): name one stale-comparison cause and scope the fast-forward rule
All checks were successful
commit-msg / commitlint (pull_request) Successful in 23s
Node tests / node:test (pull_request) Successful in 55s
Review / Review (pull_request_target) Successful in 10m55s
1fda19e288
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 repositoryFailures dropping it stays green
lens test-trimming · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the new test a dirty default checkout on a failed fetch names the cause beside its cached comparison (last test in skills/audit-git-checkouts/scripts/render-audit-report.test.ts), the staleCause and repositoryFailures functions it exercises in skills/audit-git-checkouts/scripts/render-audit-report.ts, and every other test in the file that mentions fetch failed.

What the subject does: the test is the suite's only fixture where two repository-level causes fail at once — fetch: { attempted: true, ok: false, output: "fatal: unable to access\n" } together with defaultBranchResolution: { remoteHeadOk: false }. That combination is the whole point of the fixture: it pins the precedence rule in staleCause, whose comment states the contract as "Only the first cause is named: the Failures table lists every one." repositoryFailures accordingly emits two independent rows:

if (repository.fetch.attempted && repository.fetch.ok !== true) failures.push(`fetch failed: ${firstLine(repository.fetch.output)}`);
if (repository.fetch.attempted && repository.defaultBranchResolution.remoteHeadOk !== true) {
  failures.push("could not read the server's default branch");
}

I rendered the fixture directly (a small script importing formatAuditReport with the same report object). The Failures table is:

| app | fetch failed: fatal: unable to access |
| app | could not read the server's default branch |

The test asserts only the inline Why cell and the second row. The fetch failed: … row that the fixture was constructed to produce is never asserted, and neither is the - **2 failures** summary count.

What goes wrong: the half of the stated contract that says the un-named cause's companion is still listed is unprotected in the only scenario where two causes coexist. I confirmed this with a mutation: I changed the first push to if (repository.fetch.attempted && repository.fetch.ok !== true && repository.defaultBranchResolution.remoteHeadOk === true) — i.e. the fetch failure is silently suppressed whenever the default-branch lookup also failed — and ran node --test render-audit-report.test.ts. Result: ℹ pass 17, ℹ fail 0. Nothing in the suite notices, because the only other test asserting a fetch failed: row is rows name each checkout's state in plain words (| broken | fetch failed: fatal: could not read from remote repository |), whose repository has the default remoteHeadOk: true. In production that mutation class means an operator reading the report on a total-outage run sees only "could not read the server's default branch" in Failures while the Why column says "not re-checked: fetch failed", with no row explaining the fetch.

For contrast, I mutation-checked the rest of the diff's behavior and it is well covered: swapping the two branches of staleCause, dropping the ; not re-checked: … suffix from defaultNotCurrentReason or from the manual-review branch of primaryDecision, dropping staleCause === null && from the current-count guard, making stepFailure ignore staleCause, giving a cached run a non-null cause, removing the fatalError guard, and changing the fast-forward-block wording each fail at least one test. This is the one surviving mutation that a test in scope should have killed (the only other survivor, return "no cause recorded" → return null, is an unreachable defensive default: scripts/audit-checkouts.sh sets comparison_fresh=true only when fetch_ok = true && remote_head_ok = true, so a fetched run with a stale comparison always has one of the two named causes).

Suggested repair — keep the test and add the missing row assertion beside the one already there, e.g. assert.ok(markdown.includes("| app | fetch failed: fatal: unable to access |"));. That retains everything the test already protects (precedence in the inline cell, the default-branch row) and restores the "lists every one" half of the contract.

What would refute this: an assertion elsewhere in the repository that pins the fetch-failure row with remoteHeadOk: false. I grepped the test file for fetch failed and found only the broken row above and the not re-checked: fetch failed cells; scripts/audit-checkouts.test.mjs tests the driver, not this renderer's markdown.

claim 01M3A0AEB0PEJWCBZJ2E8RWBTR of review 01M3A00T4P94ZEHBC34NZJPDD3

<!-- review:claim:01M3A0AEB0PEJWCBZJ2E8RWBTR --> **low** — The only two-cause fixture never asserts the fetch-failure row, so `repositoryFailures` dropping it stays green lens `test-trimming` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the new test `a dirty default checkout on a failed fetch names the cause beside its cached comparison` (last test in `skills/audit-git-checkouts/scripts/render-audit-report.test.ts`), the `staleCause` and `repositoryFailures` functions it exercises in `skills/audit-git-checkouts/scripts/render-audit-report.ts`, and every other test in the file that mentions `fetch failed`. > > What the subject does: the test is the suite's only fixture where two repository-level causes fail at once — `fetch: { attempted: true, ok: false, output: "fatal: unable to access\n" }` together with `defaultBranchResolution: { remoteHeadOk: false }`. That combination is the whole point of the fixture: it pins the precedence rule in `staleCause`, whose comment states the contract as "Only the first cause is named: the Failures table lists every one." `repositoryFailures` accordingly emits two independent rows: > > ``` > if (repository.fetch.attempted && repository.fetch.ok !== true) failures.push(`fetch failed: ${firstLine(repository.fetch.output)}`); > if (repository.fetch.attempted && repository.defaultBranchResolution.remoteHeadOk !== true) { > failures.push("could not read the server's default branch"); > } > ``` > > I rendered the fixture directly (a small script importing `formatAuditReport` with the same report object). The Failures table is: > > ``` > | app | fetch failed: fatal: unable to access | > | app | could not read the server's default branch | > ``` > > The test asserts only the inline `Why` cell and the *second* row. The `fetch failed: …` row that the fixture was constructed to produce is never asserted, and neither is the `- **2 failures**` summary count. > > What goes wrong: the half of the stated contract that says the un-named cause's companion is still listed is unprotected in the only scenario where two causes coexist. I confirmed this with a mutation: I changed the first push to `if (repository.fetch.attempted && repository.fetch.ok !== true && repository.defaultBranchResolution.remoteHeadOk === true)` — i.e. the fetch failure is silently suppressed whenever the default-branch lookup also failed — and ran `node --test render-audit-report.test.ts`. Result: `ℹ pass 17`, `ℹ fail 0`. Nothing in the suite notices, because the only other test asserting a `fetch failed:` row is `rows name each checkout's state in plain words` (`| broken | fetch failed: fatal: could not read from remote repository |`), whose repository has the default `remoteHeadOk: true`. In production that mutation class means an operator reading the report on a total-outage run sees only "could not read the server's default branch" in Failures while the `Why` column says "not re-checked: fetch failed", with no row explaining the fetch. > > For contrast, I mutation-checked the rest of the diff's behavior and it is well covered: swapping the two branches of `staleCause`, dropping the `; not re-checked: …` suffix from `defaultNotCurrentReason` or from the `manual-review` branch of `primaryDecision`, dropping `staleCause === null &&` from the current-count guard, making `stepFailure` ignore `staleCause`, giving a cached run a non-null cause, removing the `fatalError` guard, and changing the fast-forward-block wording each fail at least one test. This is the one surviving mutation that a test in scope should have killed (the only other survivor, `return "no cause recorded"` → `return null`, is an unreachable defensive default: `scripts/audit-checkouts.sh` sets `comparison_fresh=true` only when `fetch_ok = true && remote_head_ok = true`, so a fetched run with a stale comparison always has one of the two named causes). > > Suggested repair — keep the test and add the missing row assertion beside the one already there, e.g. `assert.ok(markdown.includes("| app | fetch failed: fatal: unable to access |"));`. That retains everything the test already protects (precedence in the inline cell, the default-branch row) and restores the "lists every one" half of the contract. > > What would refute this: an assertion elsewhere in the repository that pins the fetch-failure row with `remoteHeadOk: false`. I grepped the test file for `fetch failed` and found only the `broken` row above and the `not re-checked: fetch failed` cells; `scripts/audit-checkouts.test.mjs` tests the driver, not this renderer's markdown. claim `01M3A0AEB0PEJWCBZJ2E8RWBTR` of review `01M3A00T4P94ZEHBC34NZJPDD3`
Author
Owner

Fixed in 23b9fc2. The two-cause test now also asserts | app | fetch failed: fatal: unable to access |.

<!-- gh-feedback:reply-to:87682 --> Fixed in 23b9fc2. The two-cause test now also asserts `| app | fetch failed: fatal: unable to access |`.
jercik marked this conversation as resolved
Lines 179-180
@ -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 · arm default · tally 1 valid / 0 invalid / 0 uncertain

I read the new staleCause helper and its two-line comment in skills/audit-git-checkouts/scripts/render-audit-report.ts, every consumer of its result (stepFailure, defaultNotCurrentReason, and the manual-review branch of primaryDecision), repositoryFailures in the same file, and the freshness computation in skills/audit-git-checkouts/scripts/audit-checkouts.sh.

The comment states two contracts:

// 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.

The first three branches honour them — "audit aborted", "fetch failed", and "could not read the server's default branch" each match the wording repositoryFailures emits (the first two as prefixes of the row's : <detail> suffix), so a reader who sees a truncated cause can find the full row. The fourth branch, return "no cause recorded";, honours neither: it corresponds to no Failures row at all.

What the reader gets: every consumer concatenates the string, so the fallback renders as "cached comparison; not re-checked: no cause recorded", "cached: N commits behind, M ahead; not re-checked: no cause recorded", or "not checked for removal: no cause recorded" in the report's Why / Problem column. SKILL.md instructs the agent to relay that wording to the user ("Use the report's plain wording"). The user is told the comparison was not refreshed and simultaneously that nothing explains it, with no Failures row to consult and nothing to do next — the opposite of the comment's reassurance.

Proof gap, stated plainly: I could not construct an input that reaches this branch. In audit-checkouts.sh, comparison_fresh=true is set exactly when fetch_ok = true && remote_head_ok = true, and operational/stale-comparison is emitted exactly when comparison_fresh != true, so on a fetched run a stale comparison always implies one of the three named causes. That makes the branch defensive today — but it is the branch that fires if that invariant ever separates, which is precisely when a reader most needs the message to be honest.

Proposed correction: either drop the branch (return the remoteHeadOk cause as the final else, since the invariant guarantees it), or, if the guard is deliberate, word it so the reader knows the gap is in the report and not in their repository — e.g. "the report did not record why" — and drop the comment's second sentence, which that branch contradicts. Both keep the change's real gain: a stale comparison now names its cause instead of always blaming the fetch.

claim 01M3A07ZPNG61SJP6VS9PVTV10 of review 01M3A00T4P94ZEHBC34NZJPDD3

<!-- review:claim:01M3A07ZPNG61SJP6VS9PVTV10 --> **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` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > I read the new `staleCause` helper and its two-line comment in `skills/audit-git-checkouts/scripts/render-audit-report.ts`, every consumer of its result (`stepFailure`, `defaultNotCurrentReason`, and the `manual-review` branch of `primaryDecision`), `repositoryFailures` in the same file, and the freshness computation in `skills/audit-git-checkouts/scripts/audit-checkouts.sh`. > > The comment states two contracts: > > // 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. > > The first three branches honour them — "audit aborted", "fetch failed", and "could not read the server's default branch" each match the wording `repositoryFailures` emits (the first two as prefixes of the row's `: <detail>` suffix), so a reader who sees a truncated cause can find the full row. The fourth branch, `return "no cause recorded";`, honours neither: it corresponds to no Failures row at all. > > What the reader gets: every consumer concatenates the string, so the fallback renders as "cached comparison; not re-checked: no cause recorded", "cached: N commits behind, M ahead; not re-checked: no cause recorded", or "not checked for removal: no cause recorded" in the report's Why / Problem column. `SKILL.md` instructs the agent to relay that wording to the user ("Use the report's plain wording"). The user is told the comparison was not refreshed and simultaneously that nothing explains it, with no Failures row to consult and nothing to do next — the opposite of the comment's reassurance. > > Proof gap, stated plainly: I could not construct an input that reaches this branch. In `audit-checkouts.sh`, `comparison_fresh=true` is set exactly when `fetch_ok = true && remote_head_ok = true`, and `operational/stale-comparison` is emitted exactly when `comparison_fresh != true`, so on a fetched run a stale comparison always implies one of the three named causes. That makes the branch defensive today — but it is the branch that fires if that invariant ever separates, which is precisely when a reader most needs the message to be honest. > > Proposed correction: either drop the branch (return the `remoteHeadOk` cause as the final `else`, since the invariant guarantees it), or, if the guard is deliberate, word it so the reader knows the gap is in the report and not in their repository — e.g. `"the report did not record why"` — and drop the comment's second sentence, which that branch contradicts. Both keep the change's real gain: a stale comparison now names its cause instead of always blaming the fetch. claim `01M3A07ZPNG61SJP6VS9PVTV10` of review `01M3A00T4P94ZEHBC34NZJPDD3`
Author
Owner

Fixed 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.

<!-- gh-feedback:reply-to:87683 --> Fixed 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.
jercik marked this conversation as resolved
@ -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 · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the new staleCause() in skills/audit-git-checkouts/scripts/render-audit-report.ts and its two consumers, stepFailure() (the operational/stale-comparison branch) and primaryDecision() / defaultNotCurrentReason(); the driver skills/audit-git-checkouts/scripts/audit-checkouts.sh that produces the JSON; and render-audit-report.test.ts (all 17 tests pass under node --test --experimental-strip-types).

What the subject does: staleCause() is documented as "Why a fetched run left a repository's comparisons stale, worded as its Failures row". Its first non-cached branch returns the literal "audit aborted" whenever the repository record carries fatalError -- i.e. a record salvaged by audit_repository_worker(). That string is then rendered as not checked for removal: audit aborted by stepFailure(), and as ; not re-checked: audit aborted by the manual-review / default-needs-attention decision rows.

What goes wrong: an aborted worker is not why the comparisons are stale. In the driver, a comparison is written with fresh: false only through the repository-level comparison_fresh, set true solely by if [ "$fetch_ok" = true ] && [ "$remote_head_ok" = true ]; then comparison_fresh=true. Likewise decide_removal_outcome() emits operational/stale-comparison only under if [ "$comparison_fresh" != true ]. So on a fetched run the cause of a stale comparison is always a failed git fetch or a failed git ls-remote --symref origin HEAD; the abort is a later, independent failure. The pre-change code emitted origin fetch or default-branch lookup failed here, which was correct for exactly this record shape. And because a salvaged record keeps only checkout, commonGitDir, worktrees, and fatalError, repositoryFailures() can only emit audit aborted: ... for that repository too -- so the fetch/default-branch failure disappears from the report entirely, contradicting the new comment's promise that "Only the first cause is named: the Failures table lists every one."

Reachability, traced through the shell: audit_repository() aborts after per-worktree records have already been written. Inside the worktree loop, annotate_ignored_scan "$result_path" ... || return 1 and maybe_remove_worktree ... || { echo "amending the removal record for $worktree_path failed" >&2; return 1; } both return 1 mid-loop, and audit_repository_worker() then salvages every already-written *.result.json into a fatalError record. A worktree whose record was amended with removal.outcome: "operational/stale-comparison" before a later worktree's step failed therefore lands in that salvaged record.

Observed: I called the exported formatAuditReport directly (node --experimental-strip-types) with a fetched report whose single repository is {checkout: "/dev/app", fatalError: "worker died", worktrees: [one linked worktree with removal.outcome "operational/stale-comparison"]}. The rendered Failures table was exactly:
| app | audit aborted: worker died |
| app-y | not checked for removal: audit aborted |
No row mentions the fetch or default-branch lookup that actually left the comparison stale, and the summary reports no other cause.

What would establish or refute this: a safe correction is to keep the pre-change wording on the fatalError branch (e.g. return "origin fetch or default-branch lookup failed", optionally suffixed with "; audit aborted before the repository record was completed"), since a salvaged record genuinely cannot distinguish the two and should not name a cause the driver's own gating rules out. Proof gap: I could not run the driver end to end against a repository that both fails its fetch and aborts its worker, so the compound failure is established from the control flow above plus the rendering I did run, not from a live audit.

claim 01M3A0DKKSHCTQE3RDDPK8R9JC of review 01M3A00T4P94ZEHBC34NZJPDD3

<!-- review:claim:01M3A0DKKSHCTQE3RDDPK8R9JC --> **low** — staleCause() blames "audit aborted" for a salvaged repository, dropping the fetch/default-branch cause the previous wording carried lens `general-bug` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the new `staleCause()` in `skills/audit-git-checkouts/scripts/render-audit-report.ts` and its two consumers, `stepFailure()` (the `operational/stale-comparison` branch) and `primaryDecision()` / `defaultNotCurrentReason()`; the driver `skills/audit-git-checkouts/scripts/audit-checkouts.sh` that produces the JSON; and `render-audit-report.test.ts` (all 17 tests pass under `node --test --experimental-strip-types`). > > What the subject does: `staleCause()` is documented as "Why a fetched run left a repository's comparisons stale, worded as its Failures row". Its first non-cached branch returns the literal "audit aborted" whenever the repository record carries `fatalError` -- i.e. a record salvaged by `audit_repository_worker()`. That string is then rendered as `not checked for removal: audit aborted` by `stepFailure()`, and as `; not re-checked: audit aborted` by the `manual-review` / `default-needs-attention` decision rows. > > What goes wrong: an aborted worker is not why the comparisons are stale. In the driver, a comparison is written with `fresh: false` only through the repository-level `comparison_fresh`, set true solely by `if [ "$fetch_ok" = true ] && [ "$remote_head_ok" = true ]; then comparison_fresh=true`. Likewise `decide_removal_outcome()` emits `operational/stale-comparison` only under `if [ "$comparison_fresh" != true ]`. So on a fetched run the cause of a stale comparison is always a failed `git fetch` or a failed `git ls-remote --symref origin HEAD`; the abort is a later, independent failure. The pre-change code emitted `origin fetch or default-branch lookup failed` here, which was correct for exactly this record shape. And because a salvaged record keeps only `checkout`, `commonGitDir`, `worktrees`, and `fatalError`, `repositoryFailures()` can only emit `audit aborted: ...` for that repository too -- so the fetch/default-branch failure disappears from the report entirely, contradicting the new comment's promise that "Only the first cause is named: the Failures table lists every one." > > Reachability, traced through the shell: `audit_repository()` aborts after per-worktree records have already been written. Inside the worktree loop, `annotate_ignored_scan "$result_path" ... || return 1` and `maybe_remove_worktree ... || { echo "amending the removal record for $worktree_path failed" >&2; return 1; }` both `return 1` mid-loop, and `audit_repository_worker()` then salvages every already-written `*.result.json` into a `fatalError` record. A worktree whose record was amended with `removal.outcome: "operational/stale-comparison"` before a later worktree's step failed therefore lands in that salvaged record. > > Observed: I called the exported `formatAuditReport` directly (node --experimental-strip-types) with a fetched report whose single repository is {checkout: "/dev/app", fatalError: "worker died", worktrees: [one linked worktree with removal.outcome "operational/stale-comparison"]}. The rendered Failures table was exactly: > | app | audit aborted: worker died | > | app-y | not checked for removal: audit aborted | > No row mentions the fetch or default-branch lookup that actually left the comparison stale, and the summary reports no other cause. > > What would establish or refute this: a safe correction is to keep the pre-change wording on the `fatalError` branch (e.g. return "origin fetch or default-branch lookup failed", optionally suffixed with "; audit aborted before the repository record was completed"), since a salvaged record genuinely cannot distinguish the two and should not name a cause the driver's own gating rules out. Proof gap: I could not run the driver end to end against a repository that both fails its fetch and aborts its worker, so the compound failure is established from the control flow above plus the rendering I did run, not from a live audit. claim `01M3A0DKKSHCTQE3RDDPK8R9JC` of review `01M3A00T4P94ZEHBC34NZJPDD3`
Author
Owner

Fixed in 23b9fc2. A salvaged record now reads not 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.

<!-- gh-feedback:reply-to:87685 --> Fixed in 23b9fc2. A salvaged record now reads `not 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.
jercik marked this conversation as resolved
@ -179,0 +185,4 @@
return "no cause recorded";
}
function defaultNotCurrentReason(worktree: Worktree, staleCause: string | null): string {

low — The name staleCause now means both the helper and its result, while the call site calls the same value cause
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

I read all four sites the change touched in skills/audit-git-checkouts/scripts/render-audit-report.ts: the new module-level function staleCause(repository: Repository, fetched: boolean): string | null, and the three functions the change re-signed to take its result — stepFailure(worktree, staleCause: string | null), defaultNotCurrentReason(worktree, staleCause: string | null), and primaryDecision(worktree, defaultBranch, staleCause: string | null) — plus the single call site in formatAuditReport.

One identifier now carries two concepts. At module scope staleCause is a function (Repository, boolean) => string | null; inside each of those three bodies the same identifier is a string | null that shadows it. Meanwhile the call site introduces a third name for the same value:

const cause = staleCause(repository, report.fetched);

so the codebase reads cause where the value is produced and staleCause where it is consumed, and staleCause where the producer is defined. The writing skill's "Use Precise Language" asks for one term per concept; this is the inverse on both axes — two terms for the value, one term for two things.

The concrete cost is to the next reader or editor of these three functions. Inside defaultNotCurrentReason, the module helper is unreachable by name: anyone who tries to derive the cause locally writes staleCause(repository, fetched) and gets "This expression is not callable" pointing at a string | null, a symptom that does not name the shadowing. It also makes the new comment above the helper — "Why a fetched run left a repository's comparisons stale" — ambiguous when read from a consumer, since the identifier there denotes the already-computed string, not the function the comment describes.

Proposed correction: rename the three parameters to cause, matching the name formatAuditReport already uses for the value, and leave staleCause as the sole name of the helper. No behaviour changes; the type checker proves the rename complete.

This is a readability finding, not a runtime one: TypeScript rejects the miswrite rather than miscompiling it, which is why the severity is low. What would refute it: a project convention elsewhere in this repository of naming a parameter after the function that produced it — I grepped this file and found none; every other parameter here is named for its role (worktree, defaultBranch, fetched, removalEnabled).

claim 01M3A08T12ZRY5YS9CA4A83J49 of review 01M3A00T4P94ZEHBC34NZJPDD3

<!-- review:claim:01M3A08T12ZRY5YS9CA4A83J49 --> **low** — The name `staleCause` now means both the helper and its result, while the call site calls the same value `cause` lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > I read all four sites the change touched in `skills/audit-git-checkouts/scripts/render-audit-report.ts`: the new module-level `function staleCause(repository: Repository, fetched: boolean): string | null`, and the three functions the change re-signed to take its result — `stepFailure(worktree, staleCause: string | null)`, `defaultNotCurrentReason(worktree, staleCause: string | null)`, and `primaryDecision(worktree, defaultBranch, staleCause: string | null)` — plus the single call site in `formatAuditReport`. > > One identifier now carries two concepts. At module scope `staleCause` is a function `(Repository, boolean) => string | null`; inside each of those three bodies the same identifier is a `string | null` that shadows it. Meanwhile the call site introduces a third name for the same value: > > const cause = staleCause(repository, report.fetched); > > so the codebase reads `cause` where the value is produced and `staleCause` where it is consumed, and `staleCause` where the producer is defined. The writing skill's "Use Precise Language" asks for one term per concept; this is the inverse on both axes — two terms for the value, one term for two things. > > The concrete cost is to the next reader or editor of these three functions. Inside `defaultNotCurrentReason`, the module helper is unreachable by name: anyone who tries to derive the cause locally writes `staleCause(repository, fetched)` and gets "This expression is not callable" pointing at a `string | null`, a symptom that does not name the shadowing. It also makes the new comment above the helper — "Why a fetched run left a repository's comparisons stale" — ambiguous when read from a consumer, since the identifier there denotes the already-computed string, not the function the comment describes. > > Proposed correction: rename the three parameters to `cause`, matching the name `formatAuditReport` already uses for the value, and leave `staleCause` as the sole name of the helper. No behaviour changes; the type checker proves the rename complete. > > This is a readability finding, not a runtime one: TypeScript rejects the miswrite rather than miscompiling it, which is why the severity is low. What would refute it: a project convention elsewhere in this repository of naming a parameter after the function that produced it — I grepped this file and found none; every other parameter here is named for its role (`worktree`, `defaultBranch`, `fetched`, `removalEnabled`). claim `01M3A08T12ZRY5YS9CA4A83J49` of review `01M3A00T4P94ZEHBC34NZJPDD3`
Author
Owner

Fixed in 23b9fc2. The parameter is now cause in stepFailure, defaultNotCurrentReason and primaryDecision, so staleCause names only the helper.

<!-- gh-feedback:reply-to:87684 --> Fixed in 23b9fc2. The parameter is now `cause` in `stepFailure`, `defaultNotCurrentReason` and `primaryDecision`, so `staleCause` names only the helper.
jercik marked this conversation as resolved
@ -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 · arm default · tally 1 valid / 0 invalid / 0 uncertain

I read defaultNotCurrentReason in skills/audit-git-checkouts/scripts/render-audit-report.ts, the rule it paraphrases in skills/audit-git-checkouts/references/checkout-updates.md, the is_deferred_guidance_path predicate in skills/audit-git-checkouts/scripts/audit-checkouts.sh, the usage() text in that same script, and the assertion the change updated in render-audit-report.test.ts.

The change replaced the phrase "not deferred guidance, or upstream changed the same paths" with an inlined expansion. The rendered cell (from the updated test assertion) is:

| lib | main | +0/-5 | behind; uncommitted changes (4 files) 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 |

Three problems, all in a single Markdown table cell that SKILL.md tells the agent to relay to the user as-is ("Use the report's plain wording"):

  1. The sentence parses as one list, not two alternatives. After the colon comes an Oxford list — "A, B, or C" — and then "; or D". On a first read D attaches to that list, yielding "not all are ... upstream changed the same paths". The semicolon is the only signal that D is a second, independent cause, and a semicolon introducing a coordinated or clause is weak against a comma list that already used or.

  2. "at any depth" silently narrowed. The reference doc writes the set as "deferred guidance (AGENTS.md, .agents/**, at any depth)" — the qualifier trails both items. Here it sits immediately after .agents/**, so it reads as modifying only that item. The predicate matches AGENTS.md|*/AGENTS.md|.agents/*|*/.agents/* (audit-checkouts.sh), so a nested docs/AGENTS.md does qualify. A user whose only change is a nested AGENTS.md reads this cell and concludes their file was the disqualifier, when the real cause is the other branch (upstream touched the same path).

  3. "the same paths" has no antecedent. The reference says "upstream did not touch those paths", where "those" points back at the named guidance paths. Stripped of that context, "the same paths" gives the reader nothing to resolve it against — same as the uncommitted files? same as the listed patterns?

Supporting: "first-party submodule selector changes" matches the --help shorthand but is narrower than the predicate, which accepts any first-party .gitmodules change unconditionally ([ "$changed_path" = .gitmodules ] && return 0), not only selector edits. The rule now exists in four places that already disagree in wording — the predicate, usage() ("first-party submodule selector maintenance"), checkout-updates.md ("first-party .gitmodules/Gitlink maintenance"), and this string — which is the drift the writing skill's "One Idea, One Place" warns about.

Proposed correction, stating the rule positively ("Prefer positive directives", "attach conditions to the action they govern") and letting the reader see which half applies to them:

behind; uncommitted changes (4 files) 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.

This preserves what the change was reaching for — the reader no longer has to know the project term "deferred guidance" — while removing the list/alternative collision and restoring the scope of "at any depth".

What would refute this: a rendered report where the two causes are visually separated (they are not; formatTable collapses newlines to spaces), or a definition of "deferred guidance" reachable from the report itself (the report never names the reference doc).

claim 01M3A07AP4FW3Y178XPETZXXMV of review 01M3A00T4P94ZEHBC34NZJPDD3

<!-- review:claim:01M3A07AP4FW3Y178XPETZXXMV --> **medium** — The fast-forward-blocked cell reads as a four-item list, and "at any depth" now covers only .agents/** lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > I read `defaultNotCurrentReason` in `skills/audit-git-checkouts/scripts/render-audit-report.ts`, the rule it paraphrases in `skills/audit-git-checkouts/references/checkout-updates.md`, the `is_deferred_guidance_path` predicate in `skills/audit-git-checkouts/scripts/audit-checkouts.sh`, the `usage()` text in that same script, and the assertion the change updated in `render-audit-report.test.ts`. > > The change replaced the phrase "not deferred guidance, or upstream changed the same paths" with an inlined expansion. The rendered cell (from the updated test assertion) is: > > | lib | main | +0/-5 | behind; uncommitted changes (4 files) 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 | > > Three problems, all in a single Markdown table cell that `SKILL.md` tells the agent to relay to the user as-is ("Use the report's plain wording"): > > 1. **The sentence parses as one list, not two alternatives.** After the colon comes an Oxford list — "A, B, or C" — and then "; or D". On a first read D attaches to that list, yielding "not all are ... upstream changed the same paths". The semicolon is the only signal that D is a second, independent cause, and a semicolon introducing a coordinated `or` clause is weak against a comma list that already used `or`. > > 2. **"at any depth" silently narrowed.** The reference doc writes the set as "deferred guidance (`AGENTS.md`, `.agents/**`, at any depth)" — the qualifier trails both items. Here it sits immediately after `.agents/**`, so it reads as modifying only that item. The predicate matches `AGENTS.md|*/AGENTS.md|.agents/*|*/.agents/*` (audit-checkouts.sh), so a nested `docs/AGENTS.md` does qualify. A user whose only change is a nested `AGENTS.md` reads this cell and concludes their file was the disqualifier, when the real cause is the other branch (upstream touched the same path). > > 3. **"the same paths" has no antecedent.** The reference says "upstream did not touch **those** paths", where "those" points back at the named guidance paths. Stripped of that context, "the same paths" gives the reader nothing to resolve it against — same as the uncommitted files? same as the listed patterns? > > Supporting: "first-party submodule selector changes" matches the `--help` shorthand but is narrower than the predicate, which accepts any first-party `.gitmodules` change unconditionally (`[ "$changed_path" = .gitmodules ] && return 0`), not only selector edits. The rule now exists in four places that already disagree in wording — the predicate, `usage()` ("first-party submodule selector maintenance"), checkout-updates.md ("first-party `.gitmodules`/Gitlink maintenance"), and this string — which is the drift the writing skill's "One Idea, One Place" warns about. > > Proposed correction, stating the rule positively ("Prefer positive directives", "attach conditions to the action they govern") and letting the reader see which half applies to them: > > behind; uncommitted changes (4 files) 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. > > This preserves what the change was reaching for — the reader no longer has to know the project term "deferred guidance" — while removing the list/alternative collision and restoring the scope of "at any depth". > > What would refute this: a rendered report where the two causes are visually separated (they are not; `formatTable` collapses newlines to spaces), or a definition of "deferred guidance" reachable from the report itself (the report never names the reference doc). claim `01M3A07AP4FW3Y178XPETZXXMV` of review `01M3A00T4P94ZEHBC34NZJPDD3`
Author
Owner

Fixed in 23b9fc2 with 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 .gitmodules case.

<!-- gh-feedback:reply-to:87681 --> Fixed in 23b9fc2 with 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 `.gitmodules` case.
jercik marked this conversation as resolved
fix(audit-git-checkouts): state the fast-forward rule positively and name only real causes
All checks were successful
commit-msg / commitlint (pull_request) Successful in 24s
Node tests / node:test (pull_request) Successful in 45s
Review / Review (pull_request_target) Successful in 11m9s
23b9fc22ad
The blocked fast-forward reason now states the rule instead of a negated
list: 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.

The stale-comparison cause drops its unreachable "no cause recorded"
fallback. A salvaged record, which no longer says which step failed, reads
"origin fetch or default-branch read failed" instead of "audit aborted".
The cause parameter is named `cause` so it no longer shadows the helper.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ -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 · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the two new tests in skills/audit-git-checkouts/scripts/render-audit-report.test.ts, the strings they assert on, and every occurrence of "lookup" and "read" in skills/audit-git-checkouts/ (grep -rn "lookup" . in that directory returns exactly one hit, this test title).

What the subject says: this change deliberately standardises on "read" for the ls-remote --symref origin HEAD step. stepFailure previously produced "origin fetch or default-branch lookup failed"; the diff removes that literal and staleCause now returns "origin fetch or default-branch read failed", matching repositoryFailures, which has long emitted "could not read the server's default branch". The new comment above staleCause likewise says "the fetch or the default-branch read failed". The test title added in the same change is the sole remaining "lookup": test("a fetched run whose default-branch lookup failed does not blame the fetch", ...) — and its own body asserts markdown.includes("| mirror | could not read the server's default branch |").

What goes wrong: the writing skill's "Use Precise Language" section requires one term for one concept. A test name is the label a failure prints, and this one names the step by a word that appears nowhere else in the skill, including in the output the test checks. Someone who greps for "default-branch read" while changing that wording finds the production string, the comment, and the sibling assertions, but not the test whose whole purpose is to pin that behaviour; someone reading a failure for "default-branch lookup" has to work out that it means the remoteHeadOk path. The cost is small and entirely in navigation, not behaviour.

Proposed correction: rename to test("a fetched run whose default-branch read failed does not blame the fetch", ...). This preserves the title's real content — the distinction the test exists for, that a run which fetched successfully but could not read the server's default branch must not be reported as a fetch failure, which the test's closing assert.ok(!markdown.includes("fetch failed")) enforces — while using the same word as the code and the report.

What would establish or refute this: the grep above, plus the diff's own replacement of "lookup" with "read" in stepFailure. If "lookup" were the project's established term for this step the finding would invert, but it survives in no other file in the skill.

claim 01M3A1S4DE7VNAZ3YTB8E1T1XK of review 01M3A1GC7R779B1PQQD2XNHPQ4

<!-- review:claim:01M3A1S4DE7VNAZ3YTB8E1T1XK --> **low** — New test title says "default-branch lookup" where the change standardises on "default-branch read" everywhere else lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the two new tests in `skills/audit-git-checkouts/scripts/render-audit-report.test.ts`, the strings they assert on, and every occurrence of "lookup" and "read" in `skills/audit-git-checkouts/` (`grep -rn "lookup" .` in that directory returns exactly one hit, this test title). > > What the subject says: this change deliberately standardises on "read" for the `ls-remote --symref origin HEAD` step. `stepFailure` previously produced "origin fetch or default-branch lookup failed"; the diff removes that literal and `staleCause` now returns "origin fetch or default-branch read failed", matching `repositoryFailures`, which has long emitted "could not read the server's default branch". The new comment above `staleCause` likewise says "the fetch or the default-branch read failed". The test title added in the same change is the sole remaining "lookup": `test("a fetched run whose default-branch lookup failed does not blame the fetch", ...)` — and its own body asserts `markdown.includes("| mirror | could not read the server's default branch |")`. > > What goes wrong: the writing skill's "Use Precise Language" section requires one term for one concept. A test name is the label a failure prints, and this one names the step by a word that appears nowhere else in the skill, including in the output the test checks. Someone who greps for "default-branch read" while changing that wording finds the production string, the comment, and the sibling assertions, but not the test whose whole purpose is to pin that behaviour; someone reading a failure for "default-branch lookup" has to work out that it means the `remoteHeadOk` path. The cost is small and entirely in navigation, not behaviour. > > Proposed correction: rename to `test("a fetched run whose default-branch read failed does not blame the fetch", ...)`. This preserves the title's real content — the distinction the test exists for, that a run which fetched successfully but could not read the server's default branch must not be reported as a fetch failure, which the test's closing `assert.ok(!markdown.includes("fetch failed"))` enforces — while using the same word as the code and the report. > > What would establish or refute this: the grep above, plus the diff's own replacement of "lookup" with "read" in `stepFailure`. If "lookup" were the project's established term for this step the finding would invert, but it survives in no other file in the skill. claim `01M3A1S4DE7VNAZ3YTB8E1T1XK` of review `01M3A1GC7R779B1PQQD2XNHPQ4`
jercik marked this conversation as resolved
@ -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 · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the staleCause comment and function body in skills/audit-git-checkouts/scripts/render-audit-report.ts, repositoryFailures in the same file, the three call sites of the returned cause (stepFailure, defaultNotCurrentReason, primaryDecision, all reached from the per-repository loop in formatAuditReport), the two new tests in render-audit-report.test.ts, and the freshness logic in scripts/audit-checkouts.sh, where comparison_fresh=true is set only when fetch_ok = true and remote_head_ok = true.

What the subject says: the comment opens "Why a fetched run left a repository's comparisons stale, worded as its Failures row; null on a cached run." It then adds "Only the first cause is named: the Failures table lists both."

What goes wrong — the stated contract does not hold for the branch the comment itself introduces. repositoryFailures returns exactly one row for a salvaged repository: audit aborted: ${firstLine(repository.fatalError)}. It never emits the text staleCause returns on that same branch, "origin fetch or default-branch read failed", and it cannot "list both" causes there, because a salvaged record keeps no fetch or default-branch fields to report on. The change's own test proves the mismatch: "a salvaged repository record renders as an aborted audit with its worktrees" asserts both | app | audit aborted: amending the removal record for /dev/app-x failed | and | app-y | not checked for removal: origin fetch or default-branch read failed |. A reader who trusts the first sentence goes to the Failures table to find the matching row and finds none. For the other two branches the promise does hold: "could not read the server's default branch" matches its Failures row exactly, and "fetch failed" is the prefix of fetch failed: <first line of output>.

A second gap: the comment documents the driver's invariant but not the precondition the callers depend on. staleCause is evaluated once per repository (const cause = staleCause(repository, report.fetched);) whether or not anything is stale, and its last line returns the flat assertion "could not read the server's default branch" without consulting defaultBranchResolution.remoteHeadOk. On a completely healthy fetched run the variable therefore holds a sentence blaming a failure that did not happen. Today that is harmless because all three consumers read cause only inside a stale branch (outcome === "operational/stale-comparison", or !comparison.fresh); I checked each one. The comment never states that restriction, so nothing warns the next caller.

Proposed correction — state the contract the callers actually rely on and drop the cross-reference promise that does not survive the first branch. For example: "The cause of a repository's stale comparisons, phrased for a report cell; null on a cached run. Read it only where a comparison is stale: the driver marks one stale only when the fetch or the default-branch read failed, which is what makes the fall-through safe there and wrong anywhere else. A fetch failure wins when both failed; the Failures table names both. A salvaged record keeps neither field, so its cause names both possibilities and has no Failures row of its own." That keeps what the comment usefully carries — why the fall-through may assert a default-branch failure without checking remoteHeadOk, and why the salvaged wording stays vague — and removes a pointer the reader cannot follow.

What would establish or refute this: reading repositoryFailures beside staleCause for a repository with fatalError set, which the new salvaged-record test already exercises. I traced the code and the tests; I did not execute them.

claim 01M3A1RB98WSM0QTKF45ZQYH29 of review 01M3A1GC7R779B1PQQD2XNHPQ4

<!-- review:claim:01M3A1RB98WSM0QTKF45ZQYH29 --> **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` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the `staleCause` comment and function body in `skills/audit-git-checkouts/scripts/render-audit-report.ts`, `repositoryFailures` in the same file, the three call sites of the returned `cause` (`stepFailure`, `defaultNotCurrentReason`, `primaryDecision`, all reached from the per-repository loop in `formatAuditReport`), the two new tests in `render-audit-report.test.ts`, and the freshness logic in `scripts/audit-checkouts.sh`, where `comparison_fresh=true` is set only when `fetch_ok = true` and `remote_head_ok = true`. > > What the subject says: the comment opens "Why a fetched run left a repository's comparisons stale, worded as its Failures row; null on a cached run." It then adds "Only the first cause is named: the Failures table lists both." > > What goes wrong — the stated contract does not hold for the branch the comment itself introduces. `repositoryFailures` returns exactly one row for a salvaged repository: `audit aborted: ${firstLine(repository.fatalError)}`. It never emits the text `staleCause` returns on that same branch, "origin fetch or default-branch read failed", and it cannot "list both" causes there, because a salvaged record keeps no fetch or default-branch fields to report on. The change's own test proves the mismatch: "a salvaged repository record renders as an aborted audit with its worktrees" asserts both `| app | audit aborted: amending the removal record for /dev/app-x failed |` and `| app-y | not checked for removal: origin fetch or default-branch read failed |`. A reader who trusts the first sentence goes to the Failures table to find the matching row and finds none. For the other two branches the promise does hold: "could not read the server's default branch" matches its Failures row exactly, and "fetch failed" is the prefix of `fetch failed: <first line of output>`. > > A second gap: the comment documents the driver's invariant but not the precondition the callers depend on. `staleCause` is evaluated once per repository (`const cause = staleCause(repository, report.fetched);`) whether or not anything is stale, and its last line returns the flat assertion "could not read the server's default branch" without consulting `defaultBranchResolution.remoteHeadOk`. On a completely healthy fetched run the variable therefore holds a sentence blaming a failure that did not happen. Today that is harmless because all three consumers read `cause` only inside a stale branch (`outcome === "operational/stale-comparison"`, or `!comparison.fresh`); I checked each one. The comment never states that restriction, so nothing warns the next caller. > > Proposed correction — state the contract the callers actually rely on and drop the cross-reference promise that does not survive the first branch. For example: "The cause of a repository's stale comparisons, phrased for a report cell; null on a cached run. Read it only where a comparison is stale: the driver marks one stale only when the fetch or the default-branch read failed, which is what makes the fall-through safe there and wrong anywhere else. A fetch failure wins when both failed; the Failures table names both. A salvaged record keeps neither field, so its cause names both possibilities and has no Failures row of its own." That keeps what the comment usefully carries — why the fall-through may assert a default-branch failure without checking `remoteHeadOk`, and why the salvaged wording stays vague — and removes a pointer the reader cannot follow. > > What would establish or refute this: reading `repositoryFailures` beside `staleCause` for a repository with `fatalError` set, which the new salvaged-record test already exercises. I traced the code and the tests; I did not execute them. claim `01M3A1RB98WSM0QTKF45ZQYH29` of review `01M3A1GC7R779B1PQQD2XNHPQ4`
jercik marked this conversation as resolved
@ -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 · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: defaultNotCurrentReason in skills/audit-git-checkouts/scripts/render-audit-report.ts, its caller primaryDecision, the formatTable/addSection code that consumes the returned string, the sibling why strings in the same function, the matching assertion in render-audit-report.test.ts ("rows name each checkout's state in plain words"), references/checkout-updates.md, and is_deferred_guidance_path in scripts/audit-checkouts.sh.

What the subject does: this string is the Why cell of the Needs your decision: not current table (addSection("Needs your decision: not current", ["Checkout", "Branch", "Ahead/behind", "Why"], decisions)). The change replaced the 9-word tail : not deferred guidance, or upstream changed the same paths with a 195-character second sentence stating the general eligibility policy. references/checkout-updates.md already states that policy, nearly word for word: "A dirty checkout qualifies only when every changed path is deferred guidance (AGENTS.md, .agents/**, at any depth) or first-party .gitmodules/Gitlink maintenance, and upstream did not touch those paths."

What goes wrong:

  1. The added sentence is constant. It is identical for every row that reaches this branch and says nothing about the checkout in the row, so it does not narrow which condition this checkout failed any more than the old tail did — it costs ~190 characters per row for no row-specific information. A user with three dirty-and-behind checkouts reads the same paragraph three times.
  2. It breaks the column's scale. Every other value this function and primaryDecision produce is a short lowercase fragment with no terminal punctuation: "not current", "no comparison with origin", "cached comparison", "2 local commits not pushed", "unborn branch (no commits yet)", "primary checkout is on fix/thing, not main". This is the only cell that is two sentences and the only one ending in a period. In a pipe table the row wraps and the Checkout/Branch/Ahead-behind columns stop lining up, which is exactly the scanning the decision table exists for.
  3. It duplicates the reference. Per the installed writing skill's "One Idea, One Place" ("Do not restate what ... an earlier sentence ... already says"; "Prefer a cheap authoritative lookup to copying a fact"), the eligibility rule's home is references/checkout-updates.md; a second copy inside a generated cell is a second place to keep in sync.
  4. "first-party submodule metadata" is project-local vocabulary left undefined in the report, which is read as standalone markdown. The reference spells the same concept as "first-party .gitmodules/Gitlink maintenance"; the driver implements it as .gitmodules or a configured submodule path carrying a branch/tag selector in a non-third-party/ checkout. A reader of the report alone cannot tell which files that names.

Proposed correction: restore a short cell and leave the rule where it lives, e.g. behind; uncommitted changes (4 files) block the fast-forward (see references/checkout-updates.md), or keep the previous concise tail naming the two candidate causes. That preserves the useful meaning — this checkout is behind, and its dirt is why the driver did not fast-forward it — and preserves the pointer to the full rule without pasting it per row.

What would establish or refute this: rendering a report containing two or more default-needs-attention checkouts that are both dirty and behind and looking at the resulting table. I did not run the renderer; the reasoning is from the format string, formatTable, and the test assertion that quotes the full cell verbatim. If the intended audience only ever reads this report through a tool that reflows cells, the alignment half of the cost would not apply — the duplication and the undefined term would remain.

claim 01M3A1PRZWJZTTK3X95QTAQX9G of review 01M3A1GC7R779B1PQQD2XNHPQ4

<!-- review:claim:01M3A1PRZWJZTTK3X95QTAQX9G --> **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` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: `defaultNotCurrentReason` in `skills/audit-git-checkouts/scripts/render-audit-report.ts`, its caller `primaryDecision`, the `formatTable`/`addSection` code that consumes the returned string, the sibling `why` strings in the same function, the matching assertion in `render-audit-report.test.ts` ("rows name each checkout's state in plain words"), `references/checkout-updates.md`, and `is_deferred_guidance_path` in `scripts/audit-checkouts.sh`. > > What the subject does: this string is the `Why` cell of the `Needs your decision: not current` table (`addSection("Needs your decision: not current", ["Checkout", "Branch", "Ahead/behind", "Why"], decisions)`). The change replaced the 9-word tail `: not deferred guidance, or upstream changed the same paths` with a 195-character second sentence stating the general eligibility policy. `references/checkout-updates.md` already states that policy, nearly word for word: "A dirty checkout qualifies only when every changed path is deferred guidance (`AGENTS.md`, `.agents/**`, at any depth) or first-party `.gitmodules`/Gitlink maintenance, and upstream did not touch those paths." > > What goes wrong: > > 1. The added sentence is constant. It is identical for every row that reaches this branch and says nothing about the checkout in the row, so it does not narrow which condition this checkout failed any more than the old tail did — it costs ~190 characters per row for no row-specific information. A user with three dirty-and-behind checkouts reads the same paragraph three times. > 2. It breaks the column's scale. Every other value this function and `primaryDecision` produce is a short lowercase fragment with no terminal punctuation: "not current", "no comparison with origin", "cached comparison", "2 local commits not pushed", "unborn branch (no commits yet)", "primary checkout is on fix/thing, not main". This is the only cell that is two sentences and the only one ending in a period. In a pipe table the row wraps and the Checkout/Branch/Ahead-behind columns stop lining up, which is exactly the scanning the decision table exists for. > 3. It duplicates the reference. Per the installed writing skill's "One Idea, One Place" ("Do not restate what ... an earlier sentence ... already says"; "Prefer a cheap authoritative lookup to copying a fact"), the eligibility rule's home is `references/checkout-updates.md`; a second copy inside a generated cell is a second place to keep in sync. > 4. "first-party submodule metadata" is project-local vocabulary left undefined in the report, which is read as standalone markdown. The reference spells the same concept as "first-party `.gitmodules`/Gitlink maintenance"; the driver implements it as `.gitmodules` or a configured submodule path carrying a `branch`/`tag` selector in a non-`third-party/` checkout. A reader of the report alone cannot tell which files that names. > > Proposed correction: restore a short cell and leave the rule where it lives, e.g. `behind; uncommitted changes (4 files) block the fast-forward (see references/checkout-updates.md)`, or keep the previous concise tail naming the two candidate causes. That preserves the useful meaning — this checkout is behind, and its dirt is why the driver did not fast-forward it — and preserves the pointer to the full rule without pasting it per row. > > What would establish or refute this: rendering a report containing two or more `default-needs-attention` checkouts that are both dirty and behind and looking at the resulting table. I did not run the renderer; the reasoning is from the format string, `formatTable`, and the test assertion that quotes the full cell verbatim. If the intended audience only ever reads this report through a tool that reflows cells, the alignment half of the cost would not apply — the duplication and the undefined term would remain. claim `01M3A1PRZWJZTTK3X95QTAQX9G` of review `01M3A1GC7R779B1PQQD2XNHPQ4`
jercik marked this conversation as resolved
Author
Owner

Replying to review summary comment #87423

Round-4 outcomes for review 01M3A1GC7R779B1PQQD2XNHPQ4 (head 23b9fc2). 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:

  • 87716, the test title says "lookup". In scripts/render-audit-report.test.ts, rename the test to "a fetched run whose default-branch read failed does not blame the fetch".
  • 87717, the fast-forward rule is pasted into every row. In scripts/render-audit-report.ts defaultNotCurrentReason, shorten the cell to "behind; uncommitted changes (N files) block the fast-forward (see references/checkout-updates.md)", and update the matching test assertion.
  • 87718, the staleCause comment over-promises. In scripts/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.
> Replying to review summary comment #87423 Round-4 outcomes for review `01M3A1GC7R779B1PQQD2XNHPQ4` (head 23b9fc2). 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: - **87716, the test title says "lookup".** In `scripts/render-audit-report.test.ts`, rename the test to "a fetched run whose default-branch read failed does not blame the fetch". - **87717, the fast-forward rule is pasted into every row.** In `scripts/render-audit-report.ts` `defaultNotCurrentReason`, shorten the cell to "behind; uncommitted changes (N files) block the fast-forward (see references/checkout-updates.md)", and update the matching test assertion. - **87718, the `staleCause` comment over-promises.** In `scripts/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.
jercik merged commit 62dd08752b into main 2026-09-25 06:44:37 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
j4k-oss/agent-skills!80
No description provided.