test(audit-git-checkouts): a failed directory walk should be covered on root runners #79

jercik · 2026-09-24T13:15:01Z

jercik commented

2026-09-24 13:15:01 +00:00

Adds a test that makes find list a changed directory and then fail, through a shell shim. It covers the rule that a partial walk leaves a worktree's activity "unknown". The existing chmod-based test skips under root, so the rule could go untested on root CI runners. The new test fails if the driver ignores find's exit status or trusts a partial listing.

The two tests' titles now name their triggers, so a failure line says which one broke.

Deferred from #77 review round 4.

🤖 Generated with Claude Code

Adds a test that makes `find` list a changed directory and then fail, through a shell shim. It covers the rule that a partial walk leaves a worktree's activity "unknown". The existing chmod-based test skips under root, so the rule could go untested on root CI runners. The new test fails if the driver ignores find's exit status or trusts a partial listing. The two tests' titles now name their triggers, so a failure line says which one broke. Deferred from #77 review round 4. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

jercik added 1 commit

2026-09-24 13:15:01 +00:00

test(audit-git-checkouts): a failed directory walk should be covered on root runners

commit-msg / commitlint (pull_request) Successful in 24s

Details

Node tests / node:test (pull_request) Successful in 45s

Details

Review / Review (pull_request_target) Failing after 6m21s

Details

a96fc71d77

The chmod-based test skips under root, so the find-failure branch of
newest_directory_epoch went untested wherever CI runs as root. A shimmed
find reaches the same branch on every uid.

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

forgejo-actions commented

2026-09-24 13:21:21 +00:00

Review 01M3A0NCM41MPS3JNGXE95FN2H — head f2f5df04c24e49c0621538a93387795ba193c671

Review — j4k-oss/agent-skills @ `5ac4dc1658`

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 (2)

low — New test's name and comment describe the sibling test's scenario, and neither records why both exist

claim: 01M3A0TQ8NZ38TCZ6A58AWRA3E
anchor: skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs (snippet)

test("a changed directory whose file listing command fails leaves the timestamp unknown", (context) => {

lens: writing-quality · arm: default
verdicts: 1 valid / 0 invalid / 0 uncertain
disposition: none

What I examined: the two adjacent tests at the end of skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs, the createActivityFixture helper above them, and newest_directory_epoch in skills/audit-git-checkouts/scripts/audit-checkouts.sh, which is the function both tests exercise.

What the subject says. The new test is titled "a changed directory whose file listing command fails leaves the timestamp unknown" and carries the comment // Lists everything, then fails like a walk that hit an unreadable directory. above fixture.classify("14", "main", 'find() { command find "$@"; return 1; }'). The test directly below it was renamed in the same change to "a changed directory that is only partly readable leaves the timestamp unknown" and is guarded by { skip: process.getuid?.() === 0 && "root reads directories regardless of mode" }.

What goes wrong. The new title is not distinguishing: the sibling test is also a case where the file listing command fails. Production code is explicit about this — the comment above newest_directory_epoch reads # partial walk (find exits non-zero but still lists what it reached) so the age stays unknown., and the walk is guarded by if ! find "$directory_path" -type f -print0 >"$listing" 2>/dev/null; then. An unreadable subdirectory makes find exit non-zero, so "whose file listing command fails" is a true description of both tests. The comment compounds this by describing the new test as behaving "like a walk that hit an unreadable directory" — which is exactly the sibling test's setup — so the pair reads as a duplicate rather than as two distinct guarantees.

The rationale that would separate them is recorded nowhere. The shimmed test is the only one of the pair that runs when the process is root, because the sibling skips under process.getuid?.() === 0; it also pins the stronger property, that a non-zero exit from find forces unknown even when the listing is complete and contains a recent file (inner.txt is stamped 1003000000). The comment instead restates what command find "$@"; return 1 plainly does. AGENTS.md's "Comments Explain Why, Not What" rule asks for the opposite: "Add one only to capture what the code cannot show" and "the context that stops the next person from 'cleaning up' something load-bearing." As written, a later reader who trims the apparent duplicate deletes the only member of the pair that executes in a root container.

Proposed correction. Name the new test for the property it pins — e.g. "a changed directory whose file listing exits non-zero after listing everything leaves the timestamp unknown" — and replace the comment with the reason the shim exists, e.g. // Exercises the non-zero exit alone; the unreadable-directory test below cannot run as root. That keeps the useful meaning already present (this is the partial-walk failure path) while making the pair legible and the shimmed test safe from being cleaned up.

What would establish or refute this. The skip guard and the two titles are quoted above from the subject tree and are decisive for the naming overlap. Proof gap: I did not execute the suite, so I did not observe find returning non-zero on the chmod-000 directory in this environment; I relied on the production comment at newest_directory_epoch, which states that contract, and on the sibling test's assertion that the outcome is unknown. Running the file as root and as non-root would confirm that only the new test executes in the root case.

low — `shim` comment restates the semicolon the next line visibly appends, omits the parse-error stake, and sits away from its callers

claim: 01M3A0W1RJ2EQW0BXMM4VGY6R7
anchor: skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs (snippet)

    // Pass `shim` without a trailing `;`; the template adds one.

lens: writing-quality · arm: default
verdicts: 1 valid / 0 invalid / 0 uncertain
disposition: none

What I examined: the classify helper inside createActivityFixture in skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs, its three call sites further down the file, the failingStat constant introduced by the same change, and AGENTS.md, which this repository loads as its always-on rule set via a one-line @AGENTS.md in CLAUDE.md.

What the subject says. The change replaced a boolean failStat parameter with a free-form shim string and added the comment // Pass ‹shim› without a trailing ‹;›; the template adds one. immediately above the line it describes:
const annotationCommand = `source "$1"; ${shim ? `${shim}; ` : ""}annotate_activity_review "$2" "$5" "$3" 1002592000 "$4"`;
(Angle quotes above stand in for the backticks in the original comment.)

What goes wrong.

The second clause explains the adjacent code rather than anything the code cannot show. The interpolation ${shim ? ... : ""} displays the appended ; literally, one line below the sentence asserting that it is appended. AGENTS.md's "Comments Explain Why, Not What" rule states "Never explain what the code does" and "Default to writing no comments. Add one only to capture what the code cannot show." This half of the comment is precisely the forbidden shape, and it is the half a reader can already see.

The clause that does carry information stops short of the stake. A caller who ends a shim with a semicolon produces ;; in the assembled bash -c string, which is a bash syntax error, not a failed assertion: the run dies inside execFileSync with a parse error, far from the test that caused it. That failure mode is the non-obvious part and is exactly what the rule asks a comment to capture — it is what would stop the next author from guessing wrong.

The comment is placed where its audience does not read it. It states a rule for callers of classify, but sits in classify's body next to the template. The people it addresses write fixture.classify("30", "main", failingStat) at the call sites, or edit const failingStat = "stat_epoch() { return 1; }";, which this change declares roughly 130 lines above the helper. Neither vantage point shows the constraint. The writing standard's "Group by concept: keep a term's definition, rule, and caveat together" points the caveat at the shim parameter declaration, const classify = (days = "14", defaultBranch = "main", shim = "") => {.

Minor readability: the sentence places a sentence-level semicolon immediately after a backtick-quoted semicolon, rendering as two adjacent semicolons of different kinds. That is the one punctuation sequence this sentence should avoid, since its whole subject is how many semicolons the value may carry.

Proposed correction. Move the caveat onto the parameter and state the consequence instead of the mechanism, e.g. on the classify signature line: "A shim ending in a semicolon yields ;; and a bash parse error, not a test failure." Spelling "semicolon" as a word in the prohibition and reserving the code span for the ;; result removes the punctuation collision. This preserves the useful meaning — do not terminate the shim yourself — drops the restatement of visible code, and puts the warning where a caller composing a new shim will see it.

What would establish or refute this. The comment text, the template literal, the failingStat declaration, and the three call sites are all quoted or located from the subject tree, and AGENTS.md's comment rule is quoted verbatim from the repository root. Proof gap: I did not run /bin/bash -c with a trailing-semicolon shim to observe the parse error; that inference is from bash grammar, where ;; is valid only as a case clause terminator. Executing that command would confirm the exact diagnostic, but it does not affect the comment-quality judgment.

Other claims

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

Coverage

Coverage pass: 01M3A0NCPG5RM676A5P4TWHRPK
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** `01M3A0NCM41MPS3JNGXE95FN2H` — head `f2f5df04c24e49c0621538a93387795ba193c671` # Review — j4k-oss/agent-skills @ 5ac4dc1658b0 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 (2) ### low — New test's name and comment describe the sibling test's scenario, and neither records why both exist - claim: `01M3A0TQ8NZ38TCZ6A58AWRA3E` - anchor: `skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs` (snippet) ``` test("a changed directory whose file listing command fails leaves the timestamp unknown", (context) => { ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - disposition: none > What I examined: the two adjacent tests at the end of `skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs`, the `createActivityFixture` helper above them, and `newest_directory_epoch` in `skills/audit-git-checkouts/scripts/audit-checkouts.sh`, which is the function both tests exercise. > > What the subject says. The new test is titled "a changed directory whose file listing command fails leaves the timestamp unknown" and carries the comment `// Lists everything, then fails like a walk that hit an unreadable directory.` above `fixture.classify("14", "main", 'find() { command find "$@"; return 1; }')`. The test directly below it was renamed in the same change to "a changed directory that is only partly readable leaves the timestamp unknown" and is guarded by `{ skip: process.getuid?.() === 0 && "root reads directories regardless of mode" }`. > > What goes wrong. The new title is not distinguishing: the sibling test is also a case where the file listing command fails. Production code is explicit about this — the comment above `newest_directory_epoch` reads `# partial walk (find exits non-zero but still lists what it reached) so the age stays unknown.`, and the walk is guarded by `if ! find "$directory_path" -type f -print0 >"$listing" 2>/dev/null; then`. An unreadable subdirectory makes `find` exit non-zero, so "whose file listing command fails" is a true description of both tests. The comment compounds this by describing the new test as behaving "like a walk that hit an unreadable directory" — which is exactly the sibling test's setup — so the pair reads as a duplicate rather than as two distinct guarantees. > > The rationale that would separate them is recorded nowhere. The shimmed test is the only one of the pair that runs when the process is root, because the sibling skips under `process.getuid?.() === 0`; it also pins the stronger property, that a non-zero exit from `find` forces `unknown` even when the listing is complete and contains a recent file (`inner.txt` is stamped 1003000000). The comment instead restates what `command find "$@"; return 1` plainly does. `AGENTS.md`'s "Comments Explain Why, Not What" rule asks for the opposite: "Add one only to capture what the code cannot show" and "the context that stops the next person from 'cleaning up' something load-bearing." As written, a later reader who trims the apparent duplicate deletes the only member of the pair that executes in a root container. > > Proposed correction. Name the new test for the property it pins — e.g. "a changed directory whose file listing exits non-zero after listing everything leaves the timestamp unknown" — and replace the comment with the reason the shim exists, e.g. `// Exercises the non-zero exit alone; the unreadable-directory test below cannot run as root.` That keeps the useful meaning already present (this is the partial-walk failure path) while making the pair legible and the shimmed test safe from being cleaned up. > > What would establish or refute this. The skip guard and the two titles are quoted above from the subject tree and are decisive for the naming overlap. Proof gap: I did not execute the suite, so I did not observe `find` returning non-zero on the chmod-000 directory in this environment; I relied on the production comment at `newest_directory_epoch`, which states that contract, and on the sibling test's assertion that the outcome is `unknown`. Running the file as root and as non-root would confirm that only the new test executes in the root case. ### low — `shim` comment restates the semicolon the next line visibly appends, omits the parse-error stake, and sits away from its callers - claim: `01M3A0W1RJ2EQW0BXMM4VGY6R7` - anchor: `skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs` (snippet) ``` // Pass `shim` without a trailing `;`; the template adds one. ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - disposition: none > What I examined: the `classify` helper inside `createActivityFixture` in `skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs`, its three call sites further down the file, the `failingStat` constant introduced by the same change, and `AGENTS.md`, which this repository loads as its always-on rule set via a one-line `@AGENTS.md` in `CLAUDE.md`. > > What the subject says. The change replaced a boolean `failStat` parameter with a free-form `shim` string and added the comment `// Pass ‹shim› without a trailing ‹;›; the template adds one.` immediately above the line it describes: > > const annotationCommand = `source "$1"; ${shim ? `${shim}; ` : ""}annotate_activity_review "$2" "$5" "$3" 1002592000 "$4"`; > > (Angle quotes above stand in for the backticks in the original comment.) > > What goes wrong. > > 1. The second clause explains the adjacent code rather than anything the code cannot show. The interpolation `${shim ? ... : ""}` displays the appended `; ` literally, one line below the sentence asserting that it is appended. `AGENTS.md`'s "Comments Explain Why, Not What" rule states "Never explain what the code does" and "Default to writing no comments. Add one only to capture what the code cannot show." This half of the comment is precisely the forbidden shape, and it is the half a reader can already see. > > 2. The clause that does carry information stops short of the stake. A caller who ends a shim with a semicolon produces `;;` in the assembled `bash -c` string, which is a bash syntax error, not a failed assertion: the run dies inside `execFileSync` with a parse error, far from the test that caused it. That failure mode is the non-obvious part and is exactly what the rule asks a comment to capture — it is what would stop the next author from guessing wrong. > > 3. The comment is placed where its audience does not read it. It states a rule for callers of `classify`, but sits in `classify`'s body next to the template. The people it addresses write `fixture.classify("30", "main", failingStat)` at the call sites, or edit `const failingStat = "stat_epoch() { return 1; }";`, which this change declares roughly 130 lines above the helper. Neither vantage point shows the constraint. The writing standard's "Group by concept: keep a term's definition, rule, and caveat together" points the caveat at the `shim` parameter declaration, `const classify = (days = "14", defaultBranch = "main", shim = "") => {`. > > 4. Minor readability: the sentence places a sentence-level semicolon immediately after a backtick-quoted semicolon, rendering as two adjacent semicolons of different kinds. That is the one punctuation sequence this sentence should avoid, since its whole subject is how many semicolons the value may carry. > > Proposed correction. Move the caveat onto the parameter and state the consequence instead of the mechanism, e.g. on the `classify` signature line: "A shim ending in a semicolon yields `;;` and a bash parse error, not a test failure." Spelling "semicolon" as a word in the prohibition and reserving the code span for the `;;` result removes the punctuation collision. This preserves the useful meaning — do not terminate the shim yourself — drops the restatement of visible code, and puts the warning where a caller composing a new shim will see it. > > What would establish or refute this. The comment text, the template literal, the `failingStat` declaration, and the three call sites are all quoted or located from the subject tree, and `AGENTS.md`'s comment rule is quoted verbatim from the repository root. Proof gap: I did not run `/bin/bash -c` with a trailing-semicolon shim to observe the parse error; that inference is from bash grammar, where `;;` is valid only as a `case` clause terminator. Executing that command would confirm the exact diagnostic, but it does not affect the comment-quality judgment. ## Other claims - grounding-pending (0) - ungrounded (0) - rejected (0) - duplicate-of (0) - unadjudicated (0) ## Coverage Coverage pass: 01M3A0NCPG5RM676A5P4TWHRPK 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 |

🚀 1

forgejo-actions reviewed

2026-09-24 13:43:35 +00:00

skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs Outdated

					
				@ -1418,6 +1418,17 @@ test("a changed directory is timed by the newest file inside it", (context) => {

				  assert.equal(result.activityReview.error, null);

				});

				test("a failed walk of a changed directory leaves the timestamp unknown", (context) => {

low — New test title "a failed walk of a changed directory" is indistinguishable from the adjacent "cannot be fully walked" test
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the only file in this diff, skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs, specifically the new test added at the end of the activity-fixture block and the test that immediately follows it, plus newest_directory_epoch in skills/audit-git-checkouts/scripts/audit-checkouts.sh, which both tests drive.

What the subject says: the diff adds test("a failed walk of a changed directory leaves the timestamp unknown", ...). The very next test in the file is test("a changed directory that cannot be fully walked leaves the timestamp unknown", { skip: process.getuid?.() === 0 && "root reads directories regardless of mode" }, ...). The two titles are paraphrases of one another — "a failed walk of a changed directory" versus "a changed directory that cannot be fully walked" — and both end in the identical predicate "leaves the timestamp unknown". The bodies also assert exactly the same two facts:
assert.equal(result.activityReview.outcome, "unknown");
assert.match(result.activityReview.error, /timestamp unavailable: sample\.txt/);
Only the setup differs, and neither title says so. The new test injects fixture.classify("14", "main", "find() { return 1; };"), shadowing find with a shell function that fails immediately and lists nothing. The existing test leaves find real and chmodSync(hiddenPath, 0o000) makes it exit non-zero after listing part of the tree. Both then land on the same guard in audit-checkouts.sh: if ! find "$directory_path" -type f -print0 >"$listing" 2>/dev/null; then ... return 1. That script's own comment names the distinction the titles drop — "fails past 1,000 files or on a partial walk (find exits non-zero but still lists what it reached)".

What goes wrong: node:test identifies a failure by its title. When either of these fails, the printed line does not tell a maintainer which of two adjacent scenarios broke, and the two titles are close enough that reading them side by side does not resolve it either — the reader has to open the file and compare setups. The pair's actual relationship is also lost: the existing test is skipped when the suite runs as root, so the new test is what still covers this branch there. A title that named its trigger would carry that for free. This violates the installed writing-for-agents standard under "Use Precise Language" — "Make claims verifiable ... use one term for one concept" — and "One Idea, One Place", which asks that a reader not have to reconstruct a distinction the prose already had the room to state.

Proposed correction: rename the new test to name its trigger rather than restate the outcome, for example "a changed directory whose file listing command fails leaves the timestamp unknown", and, if the contrast is worth making explicit, narrow the existing sibling to "a changed directory that is only partly readable leaves the timestamp unknown". This preserves the useful meaning both titles already carry — the asserted outcome, "the timestamp is unknown" — while making the two distinguishable at the point a reporter prints one of them. No assertion or fixture change is implied.

What would establish or refute this: running the suite and forcing each test to fail shows whether the reporter output distinguishes them; reading the two titles as they stand is sufficient to see that it cannot. I did not run the suite in this sandbox, so the reporter's exact failure line is reasoned from node:test behavior rather than observed; scripts/standalone-node-test-reporter.mjs in this repo only registers test files and does not change how failures are labeled. The claim would be refuted if the repository documented a convention that these titles follow — I grepped AGENTS.md, README.md, CLAUDE.md, skills/verify-tests/SKILL.md, and skills/node/SKILL.md for test-naming guidance and found none.

claim 01M39S1J1M20P84XJZHERAZT0S of review 01M39RWAPBY3G42P5SVC41F8DH

**low** — New test title "a failed walk of a changed directory" is indistinguishable from the adjacent "cannot be fully walked" test lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the only file in this diff, `skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs`, specifically the new test added at the end of the activity-fixture block and the test that immediately follows it, plus `newest_directory_epoch` in `skills/audit-git-checkouts/scripts/audit-checkouts.sh`, which both tests drive. > > What the subject says: the diff adds `test("a failed walk of a changed directory leaves the timestamp unknown", ...)`. The very next test in the file is `test("a changed directory that cannot be fully walked leaves the timestamp unknown", { skip: process.getuid?.() === 0 && "root reads directories regardless of mode" }, ...)`. The two titles are paraphrases of one another — "a failed walk of a changed directory" versus "a changed directory that cannot be fully walked" — and both end in the identical predicate "leaves the timestamp unknown". The bodies also assert exactly the same two facts: > > assert.equal(result.activityReview.outcome, "unknown"); > assert.match(result.activityReview.error, /timestamp unavailable: sample\.txt/); > > Only the setup differs, and neither title says so. The new test injects `fixture.classify("14", "main", "find() { return 1; };")`, shadowing `find` with a shell function that fails immediately and lists nothing. The existing test leaves `find` real and `chmodSync(hiddenPath, 0o000)` makes it exit non-zero after listing part of the tree. Both then land on the same guard in `audit-checkouts.sh`: `if ! find "$directory_path" -type f -print0 >"$listing" 2>/dev/null; then ... return 1`. That script's own comment names the distinction the titles drop — "fails past 1,000 files or on a partial walk (find exits non-zero but still lists what it reached)". > > What goes wrong: `node:test` identifies a failure by its title. When either of these fails, the printed line does not tell a maintainer which of two adjacent scenarios broke, and the two titles are close enough that reading them side by side does not resolve it either — the reader has to open the file and compare setups. The pair's actual relationship is also lost: the existing test is skipped when the suite runs as root, so the new test is what still covers this branch there. A title that named its trigger would carry that for free. This violates the installed `writing-for-agents` standard under "Use Precise Language" — "Make claims verifiable ... use one term for one concept" — and "One Idea, One Place", which asks that a reader not have to reconstruct a distinction the prose already had the room to state. > > Proposed correction: rename the new test to name its trigger rather than restate the outcome, for example `"a changed directory whose file listing command fails leaves the timestamp unknown"`, and, if the contrast is worth making explicit, narrow the existing sibling to `"a changed directory that is only partly readable leaves the timestamp unknown"`. This preserves the useful meaning both titles already carry — the asserted outcome, "the timestamp is unknown" — while making the two distinguishable at the point a reporter prints one of them. No assertion or fixture change is implied. > > What would establish or refute this: running the suite and forcing each test to fail shows whether the reporter output distinguishes them; reading the two titles as they stand is sufficient to see that it cannot. I did not run the suite in this sandbox, so the reporter's exact failure line is reasoned from `node:test` behavior rather than observed; `scripts/standalone-node-test-reporter.mjs` in this repo only registers test files and does not change how failures are labeled. The claim would be refuted if the repository documented a convention that these titles follow — I grepped `AGENTS.md`, `README.md`, `CLAUDE.md`, `skills/verify-tests/SKILL.md`, and `skills/node/SKILL.md` for test-naming guidance and found none. claim `01M39S1J1M20P84XJZHERAZT0S` of review `01M39RWAPBY3G42P5SVC41F8DH`

👍 1

Fixed in 1b5d6c2. Both titles now name their trigger: "a changed directory whose file listing command fails leaves the timestamp unknown" (the find shim) and "a changed directory that is only partly readable leaves the timestamp unknown" (the chmod test, skipped under root).

Fixed in 1b5d6c2. Both titles now name their trigger: "a changed directory whose file listing command fails leaves the timestamp unknown" (the `find` shim) and "a changed directory that is only partly readable leaves the timestamp unknown" (the chmod test, skipped under root).

jercik marked this conversation as resolved

jercik added 1 commit

2026-09-24 13:51:20 +00:00

test(audit-git-checkouts): name what makes each directory-walk test fail

commit-msg / commitlint (pull_request) Successful in 23s

Details

Node tests / node:test (pull_request) Successful in 46s

Details

Review / Review (pull_request_target) Successful in 7m6s

Details

1b5d6c27b4

The shim test and its chmod sibling had paraphrased titles, so a failing
line did not say which one broke. Each title now names its trigger.

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

forgejo-actions reviewed

2026-09-24 13:58:40 +00:00

skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs

					
				@ -1241,3 +1243,3 @@

				    workingTree: { isClean: false, files: { staged: [], unstaged: ["sample.txt"], untracked: [] } },

				  }));

				  const classify = (days = "14", defaultBranch = "main", failStat = false) => {

				  const classify = (days = "14", defaultBranch = "main", shim = "") => {

low — The new shim parameter hides a required trailing ;, and nothing in its name or a comment states it
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: createActivityFixture and its classify helper in skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs, both call sites of the new parameter, the sibling shim helper ignoredCommandEnvironment in the same file, and the repository rule # Rule: Comments Explain Why, Not What in AGENTS.md.

What the subject does: the change replaces the boolean failStat with a free-form shim string that is spliced into a bash -c program text:
const annotationCommand = `source "$1"; ${shim} annotate_activity_review "$2" "$5" "$3" 1002592000 "$4"`;
The two values passed are failingStat ("stat_epoch() { return 1; };") and the inline literal "find() { return 1; };". Both happen to end in ;, and that terminator is load-bearing: it is what separates the shim from the annotate_activity_review call that follows on the same line. Neither the parameter name shim, its default "", nor any comment says a value must be a self-terminated shell statement.

What goes wrong: the next author writing a failure test naturally copies a shim without the terminator, and bash refuses to parse the program rather than reporting anything about the scenario under test. I reproduced the splice outside the fixture with the same shape:
$ bash -c 'source "$1"; find() { return 1; } target "$2"' _ /tmp/lib.sh hello
_: -c: line 1: syntax error near unexpected token `target'
exit=2
With the ; restored the same command runs the target function normally. The failure surfaces as a syntax error near unexpected token from execFileSync, pointing at a generated command string rather than at the omitted character, so the reader pays for a contract the code never stated. The writing standard in /opt/review/skills/writing-for-agents/SKILL.md asks for exactly this material — "explain only what is non-obvious, version-specific, or easy to misuse" — and the file already shows the self-documenting alternative: ignoredCommandEnvironment(temporaryRoot, command, body) names its parts, so no caller has to infer the shape of what it passes.

Proposed correction: state the constraint where the value is consumed, either by making the template supply the separator (${shim ? ${shim}; : ""}) so no caller can omit it, or by keeping the splice and adding one line above const annotationCommand, e.g. // A shim must terminate itself; it is spliced ahead of the call on one line. A comment of that shape is what AGENTS.md permits ("a hidden constraint, a subtle invariant") rather than what it forbids (restating what the code does). Either correction preserves the generalization the change was making — arbitrary shims instead of one boolean — while removing the silent trap.

What would refute this: evidence that classify is never extended with further shims, or a project convention documented elsewhere that shell shim strings are self-terminating by default. I found no such convention in AGENTS.md or in the skill's own SKILL.md. I could not execute the test file itself in this sandbox: audit-checkouts.sh requires jq, which is not installed here, so every test in the file aborts with jq: command not found. The reproduction above is therefore of the splice mechanism in bash, not of the fixture end to end.

claim 01M39V6DGKARYXWG13TK8XHS9S of review 01M39TYTY9JHTFE7AMZB0Y3KTW

**low** — The new `shim` parameter hides a required trailing `;`, and nothing in its name or a comment states it lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: `createActivityFixture` and its `classify` helper in `skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs`, both call sites of the new parameter, the sibling shim helper `ignoredCommandEnvironment` in the same file, and the repository rule `# Rule: Comments Explain Why, Not What` in `AGENTS.md`. > > What the subject does: the change replaces the boolean `failStat` with a free-form `shim` string that is spliced into a `bash -c` program text: > > const annotationCommand = `source "$1"; ${shim} annotate_activity_review "$2" "$5" "$3" 1002592000 "$4"`; > > The two values passed are `failingStat` (`"stat_epoch() { return 1; };"`) and the inline literal `"find() { return 1; };"`. Both happen to end in `;`, and that terminator is load-bearing: it is what separates the shim from the `annotate_activity_review` call that follows on the same line. Neither the parameter name `shim`, its default `""`, nor any comment says a value must be a self-terminated shell statement. > > What goes wrong: the next author writing a failure test naturally copies a shim without the terminator, and bash refuses to parse the program rather than reporting anything about the scenario under test. I reproduced the splice outside the fixture with the same shape: > > $ bash -c 'source "$1"; find() { return 1; } target "$2"' _ /tmp/lib.sh hello > _: -c: line 1: syntax error near unexpected token `target' > exit=2 > > With the `;` restored the same command runs the target function normally. The failure surfaces as a `syntax error near unexpected token` from `execFileSync`, pointing at a generated command string rather than at the omitted character, so the reader pays for a contract the code never stated. The writing standard in `/opt/review/skills/writing-for-agents/SKILL.md` asks for exactly this material — "explain only what is non-obvious, version-specific, or easy to misuse" — and the file already shows the self-documenting alternative: `ignoredCommandEnvironment(temporaryRoot, command, body)` names its parts, so no caller has to infer the shape of what it passes. > > Proposed correction: state the constraint where the value is consumed, either by making the template supply the separator (`${shim ? `${shim};` : ""}`) so no caller can omit it, or by keeping the splice and adding one line above `const annotationCommand`, e.g. `// A shim must terminate itself; it is spliced ahead of the call on one line.` A comment of that shape is what `AGENTS.md` permits ("a hidden constraint, a subtle invariant") rather than what it forbids (restating what the code does). Either correction preserves the generalization the change was making — arbitrary shims instead of one boolean — while removing the silent trap. > > What would refute this: evidence that `classify` is never extended with further shims, or a project convention documented elsewhere that shell shim strings are self-terminating by default. I found no such convention in `AGENTS.md` or in the skill's own `SKILL.md`. I could not execute the test file itself in this sandbox: `audit-checkouts.sh` requires `jq`, which is not installed here, so every test in the file aborts with `jq: command not found`. The reproduction above is therefore of the splice mechanism in bash, not of the fixture end to end. claim `01M39V6DGKARYXWG13TK8XHS9S` of review `01M39TYTY9JHTFE7AMZB0Y3KTW`

👍 1

Fixed in cfdfe19. The template now adds the separator after a shim (${shim ? ${shim}; : ""}), so callers pass a bare statement and cannot drop it. A one-line comment states the shape.

Fixed in cfdfe19. The template now adds the separator after a shim (`${shim ? `${shim}; ` : ""}`), so callers pass a bare statement and cannot drop it. A one-line comment states the shape.

jercik marked this conversation as resolved

skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs Outdated

					
				@ -1422,0 +1424,4 @@

				  mkdirSync(join(fixture.worktreePath, "sample.txt"));

				  writeFileSync(join(fixture.worktreePath, "sample.txt", "inner.txt"), "new work\n");

				  utimesSync(join(fixture.worktreePath, "sample.txt"), 1000000000, 1000000000);

				  const result = fixture.classify("14", "main", "find() { return 1; };");

low — New directory-walk test shims find to emit nothing, leaving the documented partial-walk rule unprotected on root runners
lens test-trimming · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the new test a changed directory whose file listing command fails leaves the timestamp unknown in skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs, the classify(days, defaultBranch, shim) helper it calls, and newest_directory_epoch in skills/audit-git-checkouts/scripts/audit-checkouts.sh.

What the code says: the implementation documents exactly one reason for checking find's exit status — "# ... fails past 1,000 files or on a partial walk (find exits non-zero but still lists what it reached) so the age stays unknown." — and implements it as if ! find "$directory_path" -type f -print0 >"$listing" 2>/dev/null; then rm -f "$listing"; return 1; fi. The companion test that exercises a real partial walk (a changed directory that is only partly readable leaves the timestamp unknown, which chmods a subdirectory to 0o000) carries { skip: process.getuid?.() === 0 && "root reads directories regardless of mode" }, so it does not run as root — which is the environment the new test exists to cover.

What goes wrong: the new test's shim is find() { return 1; };, which fails and produces no listing. That pins only "find failed outright", not "find exited non-zero but still listed what it reached". I verified this by mutating the implementation to accept a non-empty partial listing —
if ! find "$directory_path" -type f -print0 >"$listing" 2>/dev/null && [ ! -s "$listing" ]; then
— and sourcing the mutated script against a fixture directory (directory mtime 1000000000, inner.txt mtime 1003000000):

with the new test's shim find() { return 1; } → rc=1 out=[] (unchanged, so outcome === "unknown" and error =~ /timestamp unavailable: sample.txt/ both still hold and the test stays green);

with a partial-walk shim find() { command find "$@"; return 1; } → rc=0 out=[1003000000], i.e. the mutant reports a timestamp derived from an incomplete walk, which flips the outcome away from unknown and fails the test.

So on a root runner (this sandbox reports id -u = 0, as do most CI containers) nothing in the suite catches a regression that starts trusting a partial directory listing — the exact failure mode that makes an abandoned worktree look freshly active, or an active one look stale, in the activity review. A secondary symptom of the same gap: the fixture line writeFileSync(join(fixture.worktreePath, "sample.txt", "inner.txt"), "new work\n") is inert, because the shimmed find never lists that file.

Correction that keeps the current protection and restores the missing one: make the shim list what it reached before failing, e.g. find() { command find "$@"; return 1; };, and give inner.txt an explicit mtime (the sibling test a changed directory is timed by the newest file inside it uses utimesSync(..., 1003000000, 1003000000)). That still fails a mutant that drops the exit-status check entirely — find's non-zero status remains the only signal that the walk was incomplete — and additionally fails the partial-walk mutant, without depending on the root-skipped permission test.

Proof gap: I could not execute the Node test suite here — jq, which audit-checkouts.sh depends on throughout, is absent from this sandbox and could not be installed. The mutation results above were executed by sourcing audit-checkouts.sh and calling newest_directory_epoch directly (that helper needs no jq); the downstream assertion outcomes are traced through annotate_activity_review, which appends "Changed-path activity timestamp unavailable: %s" only when the helper returns non-zero and otherwise feeds the returned epoch into the age comparison. Running the full suite against both the original and the mutated implementation, with and without the suggested shim, would settle it directly.

claim 01M39V60BVNW8V9F4QZA3BKVVJ of review 01M39TYTY9JHTFE7AMZB0Y3KTW

**low** — New directory-walk test shims find to emit nothing, leaving the documented partial-walk rule unprotected on root runners lens `test-trimming` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the new test `a changed directory whose file listing command fails leaves the timestamp unknown` in skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs, the `classify(days, defaultBranch, shim)` helper it calls, and `newest_directory_epoch` in skills/audit-git-checkouts/scripts/audit-checkouts.sh. > > What the code says: the implementation documents exactly one reason for checking find's exit status — "# ... fails past 1,000 files or on a partial walk (find exits non-zero but still lists what it reached) so the age stays unknown." — and implements it as `if ! find "$directory_path" -type f -print0 >"$listing" 2>/dev/null; then rm -f "$listing"; return 1; fi`. The companion test that exercises a real partial walk (`a changed directory that is only partly readable leaves the timestamp unknown`, which chmods a subdirectory to 0o000) carries `{ skip: process.getuid?.() === 0 && "root reads directories regardless of mode" }`, so it does not run as root — which is the environment the new test exists to cover. > > What goes wrong: the new test's shim is `find() { return 1; };`, which fails *and produces no listing*. That pins only "find failed outright", not "find exited non-zero but still listed what it reached". I verified this by mutating the implementation to accept a non-empty partial listing — > > if ! find "$directory_path" -type f -print0 >"$listing" 2>/dev/null && [ ! -s "$listing" ]; then > > — and sourcing the mutated script against a fixture directory (directory mtime 1000000000, inner.txt mtime 1003000000): > > * with the new test's shim `find() { return 1; }` → `rc=1 out=[]` (unchanged, so `outcome === "unknown"` and `error =~ /timestamp unavailable: sample.txt/` both still hold and the test stays green); > * with a partial-walk shim `find() { command find "$@"; return 1; }` → `rc=0 out=[1003000000]`, i.e. the mutant reports a timestamp derived from an incomplete walk, which flips the outcome away from `unknown` and fails the test. > > So on a root runner (this sandbox reports `id -u` = 0, as do most CI containers) nothing in the suite catches a regression that starts trusting a partial directory listing — the exact failure mode that makes an abandoned worktree look freshly active, or an active one look stale, in the activity review. A secondary symptom of the same gap: the fixture line `writeFileSync(join(fixture.worktreePath, "sample.txt", "inner.txt"), "new work\n")` is inert, because the shimmed find never lists that file. > > Correction that keeps the current protection and restores the missing one: make the shim list what it reached before failing, e.g. `find() { command find "$@"; return 1; };`, and give inner.txt an explicit mtime (the sibling test `a changed directory is timed by the newest file inside it` uses `utimesSync(..., 1003000000, 1003000000)`). That still fails a mutant that drops the exit-status check entirely — find's non-zero status remains the only signal that the walk was incomplete — and additionally fails the partial-walk mutant, without depending on the root-skipped permission test. > > Proof gap: I could not execute the Node test suite here — `jq`, which audit-checkouts.sh depends on throughout, is absent from this sandbox and could not be installed. The mutation results above were executed by sourcing audit-checkouts.sh and calling `newest_directory_epoch` directly (that helper needs no jq); the downstream assertion outcomes are traced through `annotate_activity_review`, which appends "Changed-path activity timestamp unavailable: %s" only when the helper returns non-zero and otherwise feeds the returned epoch into the age comparison. Running the full suite against both the original and the mutated implementation, with and without the suggested shim, would settle it directly. claim `01M39V60BVNW8V9F4QZA3BKVVJ` of review `01M39TYTY9JHTFE7AMZB0Y3KTW`

👍 1

Fixed in cfdfe19. The shim is now find() { command find "$@"; return 1; }, and inner.txt has mtime 1003000000. Checked both mutants: trusting a non-empty partial listing now fails the test, and so does ignoring find's exit status.

Fixed in cfdfe19. The shim is now `find() { command find "$@"; return 1; }`, and `inner.txt` has mtime 1003000000. Checked both mutants: trusting a non-empty partial listing now fails the test, and so does ignoring find's exit status.

jercik marked this conversation as resolved

jercik added 1 commit

2026-09-24 15:18:35 +00:00

test(audit-git-checkouts): pin the partial-walk rule with a shim that lists before failing

commit-msg / commitlint (pull_request) Successful in 24s

Details

Node tests / node:test (pull_request) Successful in 44s

Details

Review / Review (pull_request_target) Successful in 6m53s

Details

cfdfe19b18

The `find` shim failed without listing anything, so a driver that trusted a
partial listing still passed on root runners. The shim now lists everything
and then exits non-zero, and the inner file has a newer timestamp, so that
regression fails the test. The activity fixture adds the separator after a
shim itself, so a caller cannot break the command by omitting it.

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

forgejo-actions reviewed

2026-09-24 15:25:40 +00:00

skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs Outdated

					
				@ -1254,3 +1256,1 @@

				    const annotationCommand = failStat

				      ? 'source "$1"; stat_epoch() { return 1; }; annotate_activity_review "$2" "$5" "$3" 1002592000 "$4"'

				      : 'source "$1"; annotate_activity_review "$2" "$5" "$3" 1002592000 "$4"';

				    // `shim` is one shell statement, such as a function override; the template supplies its separator.

medium — The shim comment constrains statement count when the real contract is "no trailing separator", so following it literally yields a bash syntax error
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the classify helper inside createActivityFixture in skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs, both of its current shim call sites, and the repository rule "Comments Explain Why, Not What" in AGENTS.md.

What the subject says: the comment tells a future test author that "shim is one shell statement, such as a function override; the template supplies its separator." The line it documents splices the value in as raw shell source, via a template literal whose conditional segment is ${shim ? ${shim}; : ""} placed between source "$1"; and annotate_activity_review "$2" "$5" "$3" 1002592000 "$4".

What goes wrong: the comment constrains the wrong dimension. The template's real requirement is about form -- the shim must not end with its own ;, because the template appends ; . The comment instead constrains count, and that framing is wrong in both directions:

It permits the breaking case. stat_epoch() { return 1; }; is one shell statement and satisfies the comment as written, but yields ...; stat_epoch() { return 1; };; annotate_activity_review .... I ran this: bash -c "true; stat_epoch() { return 1; };; echo reached" exits 2 with syntax error near unexpected token ';;'. The shim never installs and the test fails for a reason unrelated to what it covers.

It forbids a working case. Two statements are fine: bash -c "true; stat_epoch() { return 1; }; epoch_iso8601() { return 1; }; echo reached" printed reached and exited 0. A future author needing to override two functions would read the comment as a prohibition and work around a limit that does not exist.

So the one sentence that exists to protect the caller does not name the hazard it is positioned to prevent. AGENTS.md asks that a comment "capture what the code cannot show -- a hidden constraint"; the hidden constraint here is the separator, and the comment's second clause states it as a fact about the template ("the template supplies its separator") rather than as the directive the reader must act on. The same rule caps comments at "one short line".

Proposed correction, leading with the action:
// Pass `shim` without a trailing `;`; the template adds one.
This preserves the useful meaning (the caller owns shell source, and the separator is not theirs to write), drops the incorrect count constraint, and drops "such as a function override", which the two call sites already demonstrate.

What would establish or refute this: the two bash -c runs above are reproducible and decide the factual half. The judgment half -- that a future author is the reader this comment serves -- rests on the comment being addressed to the shim's caller, which its subject ("shim is...") makes explicit. I found no caller of classify passing a third argument other than the two in this file.

claim 01M3A03S29DQ966X198YWW81PD of review 01M39ZYJK9W5XZ01YRX0XFC7CX

**medium** — The `shim` comment constrains statement count when the real contract is "no trailing separator", so following it literally yields a bash syntax error lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the `classify` helper inside `createActivityFixture` in `skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs`, both of its current shim call sites, and the repository rule "Comments Explain Why, Not What" in `AGENTS.md`. > > What the subject says: the comment tells a future test author that "`shim` is one shell statement, such as a function override; the template supplies its separator." The line it documents splices the value in as raw shell source, via a template literal whose conditional segment is `${shim ? `${shim}; ` : ""}` placed between `source "$1"; ` and `annotate_activity_review "$2" "$5" "$3" 1002592000 "$4"`. > > What goes wrong: the comment constrains the wrong dimension. The template's real requirement is about *form* -- the shim must not end with its own `;`, because the template appends `; `. The comment instead constrains *count*, and that framing is wrong in both directions: > > 1. It permits the breaking case. `stat_epoch() { return 1; };` is one shell statement and satisfies the comment as written, but yields `...; stat_epoch() { return 1; };; annotate_activity_review ...`. I ran this: `bash -c "true; stat_epoch() { return 1; };; echo reached"` exits 2 with `syntax error near unexpected token ';;'`. The shim never installs and the test fails for a reason unrelated to what it covers. > > 2. It forbids a working case. Two statements are fine: `bash -c "true; stat_epoch() { return 1; }; epoch_iso8601() { return 1; }; echo reached"` printed `reached` and exited 0. A future author needing to override two functions would read the comment as a prohibition and work around a limit that does not exist. > > So the one sentence that exists to protect the caller does not name the hazard it is positioned to prevent. `AGENTS.md` asks that a comment "capture what the code cannot show -- a hidden constraint"; the hidden constraint here is the separator, and the comment's second clause states it as a fact about the template ("the template supplies its separator") rather than as the directive the reader must act on. The same rule caps comments at "one short line". > > Proposed correction, leading with the action: > > // Pass `shim` without a trailing `;`; the template adds one. > > This preserves the useful meaning (the caller owns shell source, and the separator is not theirs to write), drops the incorrect count constraint, and drops "such as a function override", which the two call sites already demonstrate. > > What would establish or refute this: the two `bash -c` runs above are reproducible and decide the factual half. The judgment half -- that a future author is the reader this comment serves -- rests on the comment being addressed to the shim's caller, which its subject ("`shim` is...") makes explicit. I found no caller of `classify` passing a third argument other than the two in this file. claim `01M3A03S29DQ966X198YWW81PD` of review `01M39ZYJK9W5XZ01YRX0XFC7CX`

👍 1

Fixed in f2f5df0. Reproduced the ;; syntax error. The comment now reads "Pass shim without a trailing ;; the template adds one."

Fixed in f2f5df0. Reproduced the `;;` syntax error. The comment now reads "Pass `shim` without a trailing `;`; the template adds one."

jercik marked this conversation as resolved

jercik added 1 commit

2026-09-24 15:31:03 +00:00

test(audit-git-checkouts): state the shim contract as the missing trailing separator

commit-msg / commitlint (pull_request) Successful in 23s

Details

Node tests / node:test (pull_request) Successful in 45s

Details

Review / Review (pull_request_target) Successful in 6m37s

Details

f2f5df04c2

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

forgejo-actions reviewed

2026-09-24 15:37:38 +00:00

skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs

					
				@ -1254,3 +1256,1 @@

				    const annotationCommand = failStat

				      ? 'source "$1"; stat_epoch() { return 1; }; annotate_activity_review "$2" "$5" "$3" 1002592000 "$4"'

				      : 'source "$1"; annotate_activity_review "$2" "$5" "$3" 1002592000 "$4"';

				    // Pass `shim` without a trailing `;`; the template adds one.

low — shim comment restates the semicolon the next line visibly appends, omits the parse-error stake, and sits away from its callers
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the classify helper inside createActivityFixture in skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs, its three call sites further down the file, the failingStat constant introduced by the same change, and AGENTS.md, which this repository loads as its always-on rule set via a one-line @AGENTS.md in CLAUDE.md.

What the subject says. The change replaced a boolean failStat parameter with a free-form shim string and added the comment // Pass ‹shim› without a trailing ‹;›; the template adds one. immediately above the line it describes:
const annotationCommand = `source "$1"; ${shim ? `${shim}; ` : ""}annotate_activity_review "$2" "$5" "$3" 1002592000 "$4"`;
(Angle quotes above stand in for the backticks in the original comment.)

What goes wrong.

The second clause explains the adjacent code rather than anything the code cannot show. The interpolation ${shim ? ... : ""} displays the appended ; literally, one line below the sentence asserting that it is appended. AGENTS.md's "Comments Explain Why, Not What" rule states "Never explain what the code does" and "Default to writing no comments. Add one only to capture what the code cannot show." This half of the comment is precisely the forbidden shape, and it is the half a reader can already see.

The clause that does carry information stops short of the stake. A caller who ends a shim with a semicolon produces ;; in the assembled bash -c string, which is a bash syntax error, not a failed assertion: the run dies inside execFileSync with a parse error, far from the test that caused it. That failure mode is the non-obvious part and is exactly what the rule asks a comment to capture — it is what would stop the next author from guessing wrong.

The comment is placed where its audience does not read it. It states a rule for callers of classify, but sits in classify's body next to the template. The people it addresses write fixture.classify("30", "main", failingStat) at the call sites, or edit const failingStat = "stat_epoch() { return 1; }";, which this change declares roughly 130 lines above the helper. Neither vantage point shows the constraint. The writing standard's "Group by concept: keep a term's definition, rule, and caveat together" points the caveat at the shim parameter declaration, const classify = (days = "14", defaultBranch = "main", shim = "") => {.

Minor readability: the sentence places a sentence-level semicolon immediately after a backtick-quoted semicolon, rendering as two adjacent semicolons of different kinds. That is the one punctuation sequence this sentence should avoid, since its whole subject is how many semicolons the value may carry.

Proposed correction. Move the caveat onto the parameter and state the consequence instead of the mechanism, e.g. on the classify signature line: "A shim ending in a semicolon yields ;; and a bash parse error, not a test failure." Spelling "semicolon" as a word in the prohibition and reserving the code span for the ;; result removes the punctuation collision. This preserves the useful meaning — do not terminate the shim yourself — drops the restatement of visible code, and puts the warning where a caller composing a new shim will see it.

What would establish or refute this. The comment text, the template literal, the failingStat declaration, and the three call sites are all quoted or located from the subject tree, and AGENTS.md's comment rule is quoted verbatim from the repository root. Proof gap: I did not run /bin/bash -c with a trailing-semicolon shim to observe the parse error; that inference is from bash grammar, where ;; is valid only as a case clause terminator. Executing that command would confirm the exact diagnostic, but it does not affect the comment-quality judgment.

claim 01M3A0W1RJ2EQW0BXMM4VGY6R7 of review 01M3A0NCM41MPS3JNGXE95FN2H

**low** — `shim` comment restates the semicolon the next line visibly appends, omits the parse-error stake, and sits away from its callers lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the `classify` helper inside `createActivityFixture` in `skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs`, its three call sites further down the file, the `failingStat` constant introduced by the same change, and `AGENTS.md`, which this repository loads as its always-on rule set via a one-line `@AGENTS.md` in `CLAUDE.md`. > > What the subject says. The change replaced a boolean `failStat` parameter with a free-form `shim` string and added the comment `// Pass ‹shim› without a trailing ‹;›; the template adds one.` immediately above the line it describes: > > const annotationCommand = `source "$1"; ${shim ? `${shim}; ` : ""}annotate_activity_review "$2" "$5" "$3" 1002592000 "$4"`; > > (Angle quotes above stand in for the backticks in the original comment.) > > What goes wrong. > > 1. The second clause explains the adjacent code rather than anything the code cannot show. The interpolation `${shim ? ... : ""}` displays the appended `; ` literally, one line below the sentence asserting that it is appended. `AGENTS.md`'s "Comments Explain Why, Not What" rule states "Never explain what the code does" and "Default to writing no comments. Add one only to capture what the code cannot show." This half of the comment is precisely the forbidden shape, and it is the half a reader can already see. > > 2. The clause that does carry information stops short of the stake. A caller who ends a shim with a semicolon produces `;;` in the assembled `bash -c` string, which is a bash syntax error, not a failed assertion: the run dies inside `execFileSync` with a parse error, far from the test that caused it. That failure mode is the non-obvious part and is exactly what the rule asks a comment to capture — it is what would stop the next author from guessing wrong. > > 3. The comment is placed where its audience does not read it. It states a rule for callers of `classify`, but sits in `classify`'s body next to the template. The people it addresses write `fixture.classify("30", "main", failingStat)` at the call sites, or edit `const failingStat = "stat_epoch() { return 1; }";`, which this change declares roughly 130 lines above the helper. Neither vantage point shows the constraint. The writing standard's "Group by concept: keep a term's definition, rule, and caveat together" points the caveat at the `shim` parameter declaration, `const classify = (days = "14", defaultBranch = "main", shim = "") => {`. > > 4. Minor readability: the sentence places a sentence-level semicolon immediately after a backtick-quoted semicolon, rendering as two adjacent semicolons of different kinds. That is the one punctuation sequence this sentence should avoid, since its whole subject is how many semicolons the value may carry. > > Proposed correction. Move the caveat onto the parameter and state the consequence instead of the mechanism, e.g. on the `classify` signature line: "A shim ending in a semicolon yields `;;` and a bash parse error, not a test failure." Spelling "semicolon" as a word in the prohibition and reserving the code span for the `;;` result removes the punctuation collision. This preserves the useful meaning — do not terminate the shim yourself — drops the restatement of visible code, and puts the warning where a caller composing a new shim will see it. > > What would establish or refute this. The comment text, the template literal, the `failingStat` declaration, and the three call sites are all quoted or located from the subject tree, and `AGENTS.md`'s comment rule is quoted verbatim from the repository root. Proof gap: I did not run `/bin/bash -c` with a trailing-semicolon shim to observe the parse error; that inference is from bash grammar, where `;;` is valid only as a `case` clause terminator. Executing that command would confirm the exact diagnostic, but it does not affect the comment-quality judgment. claim `01M3A0W1RJ2EQW0BXMM4VGY6R7` of review `01M3A0NCM41MPS3JNGXE95FN2H`

🚀 1

jercik marked this conversation as resolved

skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs

					
				@ -1419,3 +1420,3 @@

				});

				test("a changed directory that cannot be fully walked leaves the timestamp unknown", { skip: process.getuid?.() === 0 && "root reads directories regardless of mode" }, (context) => {

				test("a changed directory whose file listing command fails leaves the timestamp unknown", (context) => {

low — New test's name and comment describe the sibling test's scenario, and neither records why both exist
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the two adjacent tests at the end of skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs, the createActivityFixture helper above them, and newest_directory_epoch in skills/audit-git-checkouts/scripts/audit-checkouts.sh, which is the function both tests exercise.

What the subject says. The new test is titled "a changed directory whose file listing command fails leaves the timestamp unknown" and carries the comment // Lists everything, then fails like a walk that hit an unreadable directory. above fixture.classify("14", "main", 'find() { command find "$@"; return 1; }'). The test directly below it was renamed in the same change to "a changed directory that is only partly readable leaves the timestamp unknown" and is guarded by { skip: process.getuid?.() === 0 && "root reads directories regardless of mode" }.

What goes wrong. The new title is not distinguishing: the sibling test is also a case where the file listing command fails. Production code is explicit about this — the comment above newest_directory_epoch reads # partial walk (find exits non-zero but still lists what it reached) so the age stays unknown., and the walk is guarded by if ! find "$directory_path" -type f -print0 >"$listing" 2>/dev/null; then. An unreadable subdirectory makes find exit non-zero, so "whose file listing command fails" is a true description of both tests. The comment compounds this by describing the new test as behaving "like a walk that hit an unreadable directory" — which is exactly the sibling test's setup — so the pair reads as a duplicate rather than as two distinct guarantees.

The rationale that would separate them is recorded nowhere. The shimmed test is the only one of the pair that runs when the process is root, because the sibling skips under process.getuid?.() === 0; it also pins the stronger property, that a non-zero exit from find forces unknown even when the listing is complete and contains a recent file (inner.txt is stamped 1003000000). The comment instead restates what command find "$@"; return 1 plainly does. AGENTS.md's "Comments Explain Why, Not What" rule asks for the opposite: "Add one only to capture what the code cannot show" and "the context that stops the next person from 'cleaning up' something load-bearing." As written, a later reader who trims the apparent duplicate deletes the only member of the pair that executes in a root container.

Proposed correction. Name the new test for the property it pins — e.g. "a changed directory whose file listing exits non-zero after listing everything leaves the timestamp unknown" — and replace the comment with the reason the shim exists, e.g. // Exercises the non-zero exit alone; the unreadable-directory test below cannot run as root. That keeps the useful meaning already present (this is the partial-walk failure path) while making the pair legible and the shimmed test safe from being cleaned up.

What would establish or refute this. The skip guard and the two titles are quoted above from the subject tree and are decisive for the naming overlap. Proof gap: I did not execute the suite, so I did not observe find returning non-zero on the chmod-000 directory in this environment; I relied on the production comment at newest_directory_epoch, which states that contract, and on the sibling test's assertion that the outcome is unknown. Running the file as root and as non-root would confirm that only the new test executes in the root case.

claim 01M3A0TQ8NZ38TCZ6A58AWRA3E of review 01M3A0NCM41MPS3JNGXE95FN2H

**low** — New test's name and comment describe the sibling test's scenario, and neither records why both exist lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the two adjacent tests at the end of `skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs`, the `createActivityFixture` helper above them, and `newest_directory_epoch` in `skills/audit-git-checkouts/scripts/audit-checkouts.sh`, which is the function both tests exercise. > > What the subject says. The new test is titled "a changed directory whose file listing command fails leaves the timestamp unknown" and carries the comment `// Lists everything, then fails like a walk that hit an unreadable directory.` above `fixture.classify("14", "main", 'find() { command find "$@"; return 1; }')`. The test directly below it was renamed in the same change to "a changed directory that is only partly readable leaves the timestamp unknown" and is guarded by `{ skip: process.getuid?.() === 0 && "root reads directories regardless of mode" }`. > > What goes wrong. The new title is not distinguishing: the sibling test is also a case where the file listing command fails. Production code is explicit about this — the comment above `newest_directory_epoch` reads `# partial walk (find exits non-zero but still lists what it reached) so the age stays unknown.`, and the walk is guarded by `if ! find "$directory_path" -type f -print0 >"$listing" 2>/dev/null; then`. An unreadable subdirectory makes `find` exit non-zero, so "whose file listing command fails" is a true description of both tests. The comment compounds this by describing the new test as behaving "like a walk that hit an unreadable directory" — which is exactly the sibling test's setup — so the pair reads as a duplicate rather than as two distinct guarantees. > > The rationale that would separate them is recorded nowhere. The shimmed test is the only one of the pair that runs when the process is root, because the sibling skips under `process.getuid?.() === 0`; it also pins the stronger property, that a non-zero exit from `find` forces `unknown` even when the listing is complete and contains a recent file (`inner.txt` is stamped 1003000000). The comment instead restates what `command find "$@"; return 1` plainly does. `AGENTS.md`'s "Comments Explain Why, Not What" rule asks for the opposite: "Add one only to capture what the code cannot show" and "the context that stops the next person from 'cleaning up' something load-bearing." As written, a later reader who trims the apparent duplicate deletes the only member of the pair that executes in a root container. > > Proposed correction. Name the new test for the property it pins — e.g. "a changed directory whose file listing exits non-zero after listing everything leaves the timestamp unknown" — and replace the comment with the reason the shim exists, e.g. `// Exercises the non-zero exit alone; the unreadable-directory test below cannot run as root.` That keeps the useful meaning already present (this is the partial-walk failure path) while making the pair legible and the shimmed test safe from being cleaned up. > > What would establish or refute this. The skip guard and the two titles are quoted above from the subject tree and are decisive for the naming overlap. Proof gap: I did not execute the suite, so I did not observe `find` returning non-zero on the chmod-000 directory in this environment; I relied on the production comment at `newest_directory_epoch`, which states that contract, and on the sibling test's assertion that the outcome is `unknown`. Running the file as root and as non-root would confirm that only the new test executes in the root case. claim `01M3A0TQ8NZ38TCZ6A58AWRA3E` of review `01M3A0NCM41MPS3JNGXE95FN2H`

🚀 1

jercik marked this conversation as resolved

jercik commented

2026-09-24 15:46:48 +00:00

Replying to review summary comment #87412

Round-4 outcomes for review 01M3A0NCM41MPS3JNGXE95FN2H (head f2f5df0). This PR is in round 4, where only clear, severe bugs get a new push. Both findings are real but touch only test names and comments, so they are acknowledged and deferred to a follow-up PR. Both fixes go in skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs:

87691, the test name and comment describe the sibling's scenario. Rename the shim test to "a changed directory whose file listing exits non-zero after listing everything leaves the timestamp unknown". Replace its comment with // Exercises the non-zero exit alone; the unreadable-directory test below cannot run as root.
87692, the shim comment restates visible code. Move the caveat to the classify signature and state the consequence: "A shim ending in a semicolon yields ;; and a bash parse error, not a test failure."

> Replying to review summary comment #87412 Round-4 outcomes for review `01M3A0NCM41MPS3JNGXE95FN2H` (head f2f5df0). This PR is in round 4, where only clear, severe bugs get a new push. Both findings are real but touch only test names and comments, so they are acknowledged and deferred to a follow-up PR. Both fixes go in `skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs`: - **87691, the test name and comment describe the sibling's scenario.** Rename the shim test to "a changed directory whose file listing exits non-zero after listing everything leaves the timestamp unknown". Replace its comment with `// Exercises the non-zero exit alone; the unreadable-directory test below cannot run as root.` - **87692, the `shim` comment restates visible code.** Move the caveat to the `classify` signature and state the consequence: "A shim ending in a semicolon yields `;;` and a bash parse error, not a test failure."

jercik merged commit 6bbd6560d2 into main

2026-09-25 06:44:28 +00:00

jercik referenced this pull request from a commit

2026-09-25 06:44:30 +00:00

test(audit-git-checkouts): a failed directory walk should be covered on root runners (#79)

jercik referenced this pull request from a commit

2026-09-25 10:22:15 +00:00

fix(audit-git-checkouts): submodule gates should remove what origin holds and keep what it lacks

jercik referenced this pull request

2026-09-25 10:22:25 +00:00

fix(audit-git-checkouts): submodule gates should keep what origin lacks and remove what it holds #81

Sign in to join this conversation.