feat(audit-git-checkouts): report audits as decisions and simplify removal gates #77

Merged
jercik merged 8 commits from refactor/rebuild-audit-git-checkouts into main 2026-09-24 08:10:40 +00:00
Owner

Agents now read a rendered markdown report (render-audit-report.ts) instead of querying the JSON with jq. They reply with its summary and work through the remaining decisions in batches. In 25 past sessions, raw reports ran to 5–16K characters of internal outcome codes. The rejected alternative was keeping the jq workflow with rewritten prose; that leaves every agent carrying the report schema.

The report is now schema 5: gitPrune, summary, and removal.preciousPaths are gone, removeMerged is removalEnabled, and --remove-merged is removed. Nothing outside the skill reads them.

An adversarial debate rejected dropping the mount guard (Git deletes through a mount inside a worktree, reproduced) and merging the sparse-checkout outcome. It accepted capping the ignored-file scan at 4,096 entries and 2 GiB, not re-locking after a failed removal, and reusing the audit's merge proof unless HEAD, origin/<default>, or replace refs changed.

After a fast-forward, dependency changes are reported, not installed. The workflow and hook diffs are j4k-align regeneration.

Agents now read a rendered markdown report (`render-audit-report.ts`) instead of querying the JSON with jq. They reply with its summary and work through the remaining decisions in batches. In 25 past sessions, raw reports ran to 5–16K characters of internal outcome codes. The rejected alternative was keeping the jq workflow with rewritten prose; that leaves every agent carrying the report schema. The report is now schema 5: `gitPrune`, `summary`, and `removal.preciousPaths` are gone, `removeMerged` is `removalEnabled`, and `--remove-merged` is removed. Nothing outside the skill reads them. An adversarial debate rejected dropping the mount guard (Git deletes through a mount inside a worktree, reproduced) and merging the sparse-checkout outcome. It accepted capping the ignored-file scan at 4,096 entries and 2 GiB, not re-locking after a failed removal, and reusing the audit's merge proof unless HEAD, `origin/<default>`, or replace refs changed. After a fast-forward, dependency changes are reported, not installed. The workflow and hook diffs are j4k-align regeneration.
feat(audit-git-checkouts): report audits as decisions and simplify removal gates
Some checks failed
commit-msg / commitlint (pull_request) Successful in 19s
Node tests / node:test (pull_request) Successful in 34s
Review / Review (pull_request_target) Failing after 7m53s
099f0851dc
Agents read a rendered markdown report grouped by the decisions it needs
instead of querying the JSON, reply with its summary, and work through the
remaining decisions in batches. The report moves to schema 5.

Removal gates were simplified through an adversarial debate: the ignored
scan keeps only its entry and 2 GiB byte caps, a failed removal no longer
re-locks an expired lock, and removal reuses the audit-time merge proof.
The mount guard and the sparse-checkout outcome stay.

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

Review 01M38ZNK9786DK6HMH6JHEC151 — head 11c8adf74a2af352a7e47067e73f589be44fd626

Review — j4k-oss/agent-skills @ d1e95321d4

Scope: diff against base tree 8825218a8def
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 (5)

medium — node-test workflow drops the step that installed jq, but audit-checkouts.test.mjs still shells out to jq on nearly every test

  • claim: 01M3906MVBQXRKC369HBXEQ9E1
  • anchor: .forgejo/workflows/node-test.yml (snippet)
      - name: Run tests
        shell: bash
        run: |
          set -eu
  • lens: general-bug · arm: default
  • verdicts: 1 valid / 0 invalid / 0 uncertain
  • disposition: none

What I examined: the diff of .forgejo/workflows/node-test.yml, the full skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs, audit-checkouts.sh, inspect-ignored-content.sh and prove-branch-contained.sh in the subject tree.

What changed: the workflow removed the 'Ensure baseline shell tooling' step, whose own comment (deleted in this diff) read: "The Forge maps ubuntu-latest to a minimal Node image, so tools GitHub's runner preinstalls are absent and a test shelling out to one dies here." That step ran apt-get to install jq, lsof and column when missing. Nothing replaces it: the remaining job is checkout, setup-node, and the 'Run tests' step, which now also selects the new render-audit-report.test.ts.

What still needs jq: audit-checkouts.sh has require_command jq in its main path and calls jq in nearly every function (write_submodule_metadata, write_latest_change, annotate_lock, annotate_activity_review, write_directory_findings, decide_removal_outcome, maybe_remove_worktree, classify_checkout, ...). inspect-ignored-content.sh's write_ignored_scan ends with jq -n --arg state ... >"$output" || return 1. prove-branch-contained.sh has command -v jq >/dev/null 2>&1 || fail "required command not found: jq". The test file sources these scripts and calls those functions directly, e.g. the scanIgnoredContent helper runs source "$1"; write_ignored_scan ... and then does JSON.parse(readFileSync(outputPath, "utf8")); with jq absent, write_ignored_scan returns 1 before writing the output file, so readFileSync throws ENOENT and the test fails. The removal tests call maybe_remove_worktree, which is jq on every path. There is no skip-if-missing guard in the test file: grep for 'jq' in audit-checkouts.test.mjs finds nothing.

lsof and column are no longer referenced by any test or script in the tree, so dropping them is fine; jq is the problem.

What goes wrong: on a runner image without jq, the node:test job fails for audit-checkouts.test.mjs on the first jq-dependent test, and the job aborts before the other test files run (the loop uses set -eu and node exits non-zero). The push/pull_request check is red for every change to the repository, not only this one.

Proof gap: I cannot inspect the Forge's ubuntu-latest image from this sandbox, so I cannot confirm jq is absent there today. The only evidence about that image in the subject is the deleted comment asserting that preinstalled tools are absent, plus the fact that the step was conditional (command -v jq >/dev/null || missing+=(jq)), so keeping it cost nothing when jq was present. If the image is now known to ship jq, this claim is moot; otherwise restore the conditional apt-get step (or add jq to the image) before merging.

Safe correction: reinstate the conditional install step limited to jq:

  - name: Ensure baseline shell tooling
    shell: bash
    run: |
      set -eu
      command -v jq >/dev/null && exit 0
      apt-get update
      apt-get install -y --no-install-recommends jq

If this workflow is rendered by j4k-align (the error text in the run step says so), the fix belongs in the template as well, or the next --fix will drop the step again.

  • claim: 01M38ZXWEKVZQ1FMDZHPFPG0Y7
  • anchor: skills/audit-git-checkouts/SKILL.md (snippet)
| Audit, clean up, "which are behind" | none | Fetch and prune `origin`; fast-forward eligible default checkouts; prune stale worktree registrations; remove proven-merged linked worktrees inside the root. |
  • lens: writing-quality · arm: default
  • verdicts: 1 valid / 0 invalid / 0 uncertain
  • disposition: none

What I examined: the new "Choose the mode" table in SKILL.md, the driver's usage() text in scripts/audit-checkouts.sh, the tracking_update_eligible block in the same script, and the "Submodule selectors" section of references/checkout-updates.md.

What the subject says: the table's "What changes" column for the default mode lists four writes: fetch and prune origin, fast-forward eligible default checkouts, prune stale worktree registrations, and remove proven-merged linked worktrees. The --no-remove row is "Everything above except removal", so it inherits that list. The paragraph that follows says "The default mode's updates and removals need no further confirmation."

What the driver actually does in every fetched mode: its own help text lists two further writes, "submodule sync after a fast-forward; branch/tag submodule selectors advanced outside third-party/ paths". The tracking_update_eligible condition in audit-checkouts.sh runs synchronize_first_party_tracking_submodules on any fresh, first-party, default-branch checkout that has a branch or tag selector, whether or not a fast-forward happened (the condition is fast_forward_attempted != true || fast_forward_ok == true). references/checkout-updates.md describes the effect: the driver "fetches that branch or tag, checks the submodule out detached, and leaves the Gitlink change unstaged for the owner's next commit". That reference is only read when a selector problem is being acted on, after the run.

What goes wrong: the table is the one place the agent decides which mode to run and what to tell the user will happen. An agent following it will describe the default run as fetch, fast-forward, prune, and remove, then leave the user with submodule working trees moved to a new detached commit and unstaged Gitlink modifications in checkouts that were clean before the audit. The previous SKILL.md stated this ("advances configured branch/tag selectors, leaving Gitlink changes unstaged; this can happen without a superproject fast-forward"); the rewrite dropped it from the decision point without moving it anywhere the agent reads before running. The writing skill's "Specify the Discipline" asks the contract to state the outcome and boundaries up front, and "Make the decisive constraint prominent".

Proposed correction: extend the default row's "What changes" cell, for example: "Fetch and prune origin; fast-forward eligible default checkouts and sync their submodules; in first-party default checkouts, move submodules to their configured branch or tag and leave the Gitlink change unstaged; prune stale worktree registrations; remove proven-merged linked worktrees inside the root." Optionally add a fourth bullet under "Three conditions call for a stricter mode": a first-party checkout whose submodule selectors must not move: --no-fetch --no-remove (checkout-updates.md already gives that advice for update = none). This preserves the table's brevity while making the list of writes complete.

What would refute it: showing that synchronize_first_party_tracking_submodules cannot change a working tree, or that SKILL.md names this write somewhere before the run. I found neither.

low — Replacement timeout comment restates what timeout-minutes means and drops the one fact an editor needs

  • claim: 01M38ZXX9FRAJSN16S5XSG6AF3
  • anchor: .forgejo/workflows/review.yml (snippet)
    # Limit runner occupancy even if retries or reconciliation are unfinished.
    timeout-minutes: 130
  • lens: writing-quality · arm: default
  • verdicts: 1 valid / 0 invalid / 0 uncertain
  • disposition: none

What I examined: the timeout-minutes: 130 line and its comment in .forgejo/workflows/review.yml, plus the diff hunk that replaced the previous sixteen-line derivation.

What the subject says: the new comment reads "Limit runner occupancy even if retries or reconciliation are unfinished." That is the definition of timeout-minutes on any job; a reader who knows the workflow syntax already knows it. The previous comment was a journal-style derivation and deserved cutting, but it carried one durable fact: 130 is a ceiling computed from the pinned wrapper's worst-case wait deadlines plus headroom for forge calls, and it has to be re-derived when the wrapper pin (uses: https://code.j4k.dev/j4k-oss/review-wrapper@21cd18a…) moves. It also noted that the wrapper installs no signal handler, so a kill writes nothing to the PR.

What goes wrong: under the writing skill's no-op test, the new line changes no behavior and can be deleted. Meanwhile the number 130 now looks arbitrary. The next person who bumps the wrapper pin, or who sees a run killed at 130 minutes, has no signal that the value is coupled to the pin or that the kill leaves the PR without a review. "State Durable Truth" asks to keep causal facts that help a future agent apply a rule while dropping the change history.

Proposed correction: replace the comment with the durable fact in one or two lines, for example:

# Ceiling on the pinned wrapper's worst-case waits plus forge-call headroom;
# re-derive when the pin moves. A kill here writes nothing to the PR.

That keeps the rationale that lets an editor adapt the value and removes the restatement. Alternatively delete the comment entirely; either is better than the current line. Static reasoning from the two versions of the comment; I could not inspect the wrapper to re-verify the figure.

low — The partial-walk activity test is skipped under root, so the find-failure branch of newest_directory_epoch may never execute in CI

  • claim: 01M38ZY3DD4E0V9A2C71D5NGX1
  • anchor: skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs (snippet)
test("a changed directory that cannot be fully walked leaves the timestamp unknown", { skip: process.getuid?.() === 0 && "root reads directories regardless of mode" }, (context) => {
  • lens: test-trimming · arm: default
  • verdicts: 1 valid / 0 invalid / 0 uncertain
  • disposition: none

What I examined: the new test a changed directory that cannot be fully walked leaves the timestamp unknown in audit-checkouts.test.mjs, the function it targets (newest_directory_epoch in audit-checkouts.sh, added in this change), the sibling helper pattern failStat in createActivityFixture, and .forgejo/workflows/node-test.yml.

What the test does: it builds the failure by chmod-ing a nested directory to 0o000 so find cannot descend, and guards the whole test with

{ skip: process.getuid?.() === 0 && "root reads directories regardless of mode" }

The skip is correct as far as it goes: root bypasses directory mode bits, so under uid 0 find succeeds and the setup cannot produce the failure. But it means the test contributes nothing wherever the suite runs as root.

What it is the only protection for: the failure branch of the new helper in audit-checkouts.sh:

if ! find "$directory_path" -type f -print0 >"$listing" 2>/dev/null; then
  rm -f "$listing"
  return 1
fi

and the caller in annotate_activity_review that turns that return 1 into Changed-path activity timestamp unavailable: <path> and an unknown outcome. No other test in the file drives newest_directory_epoch to failure; the other new directory test (a changed directory is timed by the newest file inside it) covers only the success path. A regression such as dropping the if ! find ...; then return 1 guard (falling back to the directory's own mtime and reporting possibly-abandoned instead of unknown) would stay green on any root runner.

Observed: in this review sandbox id -u prints 0, so the skip predicate fires here. Proof gap: node-test.yml uses runs-on: ubuntu-latest with no container: or user setting, and I cannot see the Forgejo runner's configuration; container-based runners commonly execute as root, but I could not confirm it for this repository. If the project's runner executes as a non-root user the test does run in CI and this claim reduces to a portability note.

Repair that keeps the protection on every uid: the fixture already shows the pattern for forcing a helper to fail without relying on permissions:

'source "$1"; stat_epoch() { return 1; }; annotate_activity_review ...'

The same technique works for the walk: source the script, define find() { return 1; } (or prepend a failing find shim via the existing ignoredCommandEnvironment helper) and then call annotate_activity_review on a fixture whose changed path is a directory. That exercises the exact find failure branch, asserts outcome === "unknown" and the timestamp unavailable: sample.txt error, and does not depend on the process uid. The chmod variant can stay as the real-permissions scenario for non-root runs, but it should not be the only check of this branch.

low — Renderer reports "origin fetch failed" for a default checkout whose fetch succeeded but whose server default-branch lookup failed

  • claim: 01M3907BR6AHXCVJN2G922EACN
  • anchor: skills/audit-git-checkouts/scripts/render-audit-report.ts (snippet)
        if (fetched) return { aheadBehind: formatAheadBehind(comparison), why: "not compared: origin fetch failed" };
  • lens: general-bug · arm: default
  • verdicts: 1 valid / 0 invalid / 0 uncertain
  • disposition: none

What I examined: primaryDecision in render-audit-report.ts (the manual-review branch), repositoryFailures in the same file, and audit_repository in audit-checkouts.sh, which is the only producer of defaultComparison.fresh.

Mechanism: the driver sets comparison_fresh only when both steps succeed: if [ "$fetch_ok" = true ] && [ "$remote_head_ok" = true ]; then comparison_fresh=true; fi. remote_head_ok comes from git ls-remote --symref origin HEAD, which can fail (or return no ref: refs/heads/... line) while git fetch --prune origin succeeds, for example on a remote whose HEAD is unborn or detached, or a mirror that does not advertise a symref. In that case every worktree gets defaultComparison.fresh=false, classify_checkout labels a default-branch checkout manual-review, and the renderer's manual-review branch reaches if (fetched) return { ..., why: "not compared: origin fetch failed" }.

What goes wrong: the 'Needs your decision: not current' row tells the user the fetch failed, while the Failures table for the same repository, built by repositoryFailures, says the opposite: fetch failed is only pushed when fetch.ok !== true, and this case instead produces "could not read the server's default branch". The two sections of the same report contradict each other, and the user is sent to debug network/auth on a fetch that worked. stepFailure already handles the same condition correctly for linked worktrees with the wording "origin fetch or default-branch lookup failed".

Static reasoning only: I could not run the driver here (jq is not installed in this sandbox), so I did not produce a real report exhibiting this; the path is traced from the code. The renderer tests cover only the fetch.ok=false case ("a fetched run never counts an uncompared checkout as current").

Safe correction: use the same neutral wording stepFailure uses, e.g. "not compared: origin fetch or default-branch lookup failed", or pass the repository's fetch.ok / defaultBranchResolution.remoteHeadOk into primaryDecision and pick the message from those.

Other claims

  • grounding-pending (0)
  • ungrounded (0)
  • rejected (1)
    • 01M38ZXWW77WWY24J79KP8EKYN low — Rewrapped concurrency comment leaves one 122-column line mid-paragraph
  • duplicate-of (0)
  • unadjudicated (0)

Coverage

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

lens part arm unit status runs loss
general-bug whole default claims-emitted 1 no
writing-quality whole default claims-emitted 1 no
test-trimming whole default claims-emitted 1 no
<!-- review:summary --> **Review** `01M38ZNK9786DK6HMH6JHEC151` — head `11c8adf74a2af352a7e47067e73f589be44fd626` # Review — j4k-oss/agent-skills @ d1e95321d499 Scope: diff against base tree `8825218a8def` 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 (5) ### medium — node-test workflow drops the step that installed jq, but audit-checkouts.test.mjs still shells out to jq on nearly every test - claim: `01M3906MVBQXRKC369HBXEQ9E1` - anchor: `.forgejo/workflows/node-test.yml` (snippet) ``` - name: Run tests shell: bash run: | set -eu ``` - lens: general-bug · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - disposition: none > What I examined: the diff of .forgejo/workflows/node-test.yml, the full skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs, audit-checkouts.sh, inspect-ignored-content.sh and prove-branch-contained.sh in the subject tree. > > What changed: the workflow removed the 'Ensure baseline shell tooling' step, whose own comment (deleted in this diff) read: "The Forge maps ubuntu-latest to a minimal Node image, so tools GitHub's runner preinstalls are absent and a test shelling out to one dies here." That step ran apt-get to install jq, lsof and column when missing. Nothing replaces it: the remaining job is checkout, setup-node, and the 'Run tests' step, which now also selects the new render-audit-report.test.ts. > > What still needs jq: audit-checkouts.sh has `require_command jq` in its main path and calls jq in nearly every function (write_submodule_metadata, write_latest_change, annotate_lock, annotate_activity_review, write_directory_findings, decide_removal_outcome, maybe_remove_worktree, classify_checkout, ...). inspect-ignored-content.sh's write_ignored_scan ends with `jq -n --arg state ... >"$output" || return 1`. prove-branch-contained.sh has `command -v jq >/dev/null 2>&1 || fail "required command not found: jq"`. The test file sources these scripts and calls those functions directly, e.g. the scanIgnoredContent helper runs `source "$1"; write_ignored_scan ...` and then does `JSON.parse(readFileSync(outputPath, "utf8"))`; with jq absent, write_ignored_scan returns 1 before writing the output file, so readFileSync throws ENOENT and the test fails. The removal tests call maybe_remove_worktree, which is jq on every path. There is no skip-if-missing guard in the test file: grep for 'jq' in audit-checkouts.test.mjs finds nothing. > > lsof and column are no longer referenced by any test or script in the tree, so dropping them is fine; jq is the problem. > > What goes wrong: on a runner image without jq, the node:test job fails for audit-checkouts.test.mjs on the first jq-dependent test, and the job aborts before the other test files run (the loop uses `set -eu` and node exits non-zero). The push/pull_request check is red for every change to the repository, not only this one. > > Proof gap: I cannot inspect the Forge's ubuntu-latest image from this sandbox, so I cannot confirm jq is absent there today. The only evidence about that image in the subject is the deleted comment asserting that preinstalled tools are absent, plus the fact that the step was conditional (`command -v jq >/dev/null || missing+=(jq)`), so keeping it cost nothing when jq was present. If the image is now known to ship jq, this claim is moot; otherwise restore the conditional apt-get step (or add jq to the image) before merging. > > Safe correction: reinstate the conditional install step limited to jq: > > - name: Ensure baseline shell tooling > shell: bash > run: | > set -eu > command -v jq >/dev/null && exit 0 > apt-get update > apt-get install -y --no-install-recommends jq > > If this workflow is rendered by j4k-align (the error text in the run step says so), the fix belongs in the template as well, or the next --fix will drop the step again. ### medium — Mode table omits the submodule checkouts and unstaged Gitlink changes a default run writes - claim: `01M38ZXWEKVZQ1FMDZHPFPG0Y7` - anchor: `skills/audit-git-checkouts/SKILL.md` (snippet) ``` | Audit, clean up, "which are behind" | none | Fetch and prune `origin`; fast-forward eligible default checkouts; prune stale worktree registrations; remove proven-merged linked worktrees inside the root. | ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - disposition: none > What I examined: the new "Choose the mode" table in SKILL.md, the driver's `usage()` text in scripts/audit-checkouts.sh, the `tracking_update_eligible` block in the same script, and the "Submodule selectors" section of references/checkout-updates.md. > > What the subject says: the table's "What changes" column for the default mode lists four writes: fetch and prune origin, fast-forward eligible default checkouts, prune stale worktree registrations, and remove proven-merged linked worktrees. The `--no-remove` row is "Everything above except removal", so it inherits that list. The paragraph that follows says "The default mode's updates and removals need no further confirmation." > > What the driver actually does in every fetched mode: its own help text lists two further writes, "submodule sync after a fast-forward; branch/tag submodule selectors advanced outside third-party/ paths". The `tracking_update_eligible` condition in audit-checkouts.sh runs `synchronize_first_party_tracking_submodules` on any fresh, first-party, default-branch checkout that has a branch or tag selector, whether or not a fast-forward happened (the condition is `fast_forward_attempted != true || fast_forward_ok == true`). references/checkout-updates.md describes the effect: the driver "fetches that branch or tag, checks the submodule out detached, and leaves the Gitlink change unstaged for the owner's next commit". That reference is only read when a selector problem is being acted on, after the run. > > What goes wrong: the table is the one place the agent decides which mode to run and what to tell the user will happen. An agent following it will describe the default run as fetch, fast-forward, prune, and remove, then leave the user with submodule working trees moved to a new detached commit and unstaged Gitlink modifications in checkouts that were clean before the audit. The previous SKILL.md stated this ("advances configured branch/tag selectors, leaving Gitlink changes unstaged; this can happen without a superproject fast-forward"); the rewrite dropped it from the decision point without moving it anywhere the agent reads before running. The writing skill's "Specify the Discipline" asks the contract to state the outcome and boundaries up front, and "Make the decisive constraint prominent". > > Proposed correction: extend the default row's "What changes" cell, for example: "Fetch and prune `origin`; fast-forward eligible default checkouts and sync their submodules; in first-party default checkouts, move submodules to their configured branch or tag and leave the Gitlink change unstaged; prune stale worktree registrations; remove proven-merged linked worktrees inside the root." Optionally add a fourth bullet under "Three conditions call for a stricter mode": a first-party checkout whose submodule selectors must not move: `--no-fetch --no-remove` (checkout-updates.md already gives that advice for `update = none`). This preserves the table's brevity while making the list of writes complete. > > What would refute it: showing that `synchronize_first_party_tracking_submodules` cannot change a working tree, or that SKILL.md names this write somewhere before the run. I found neither. ### low — Replacement timeout comment restates what timeout-minutes means and drops the one fact an editor needs - claim: `01M38ZXX9FRAJSN16S5XSG6AF3` - anchor: `.forgejo/workflows/review.yml` (snippet) ``` # Limit runner occupancy even if retries or reconciliation are unfinished. timeout-minutes: 130 ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - disposition: none > What I examined: the `timeout-minutes: 130` line and its comment in .forgejo/workflows/review.yml, plus the diff hunk that replaced the previous sixteen-line derivation. > > What the subject says: the new comment reads "Limit runner occupancy even if retries or reconciliation are unfinished." That is the definition of `timeout-minutes` on any job; a reader who knows the workflow syntax already knows it. The previous comment was a journal-style derivation and deserved cutting, but it carried one durable fact: 130 is a ceiling computed from the pinned wrapper's worst-case wait deadlines plus headroom for forge calls, and it has to be re-derived when the wrapper pin (`uses: https://code.j4k.dev/j4k-oss/review-wrapper@21cd18a…`) moves. It also noted that the wrapper installs no signal handler, so a kill writes nothing to the PR. > > What goes wrong: under the writing skill's no-op test, the new line changes no behavior and can be deleted. Meanwhile the number 130 now looks arbitrary. The next person who bumps the wrapper pin, or who sees a run killed at 130 minutes, has no signal that the value is coupled to the pin or that the kill leaves the PR without a review. "State Durable Truth" asks to keep causal facts that help a future agent apply a rule while dropping the change history. > > Proposed correction: replace the comment with the durable fact in one or two lines, for example: > > # Ceiling on the pinned wrapper's worst-case waits plus forge-call headroom; > # re-derive when the pin moves. A kill here writes nothing to the PR. > > That keeps the rationale that lets an editor adapt the value and removes the restatement. Alternatively delete the comment entirely; either is better than the current line. Static reasoning from the two versions of the comment; I could not inspect the wrapper to re-verify the figure. ### low — The partial-walk activity test is skipped under root, so the find-failure branch of newest_directory_epoch may never execute in CI - claim: `01M38ZY3DD4E0V9A2C71D5NGX1` - anchor: `skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs` (snippet) ``` test("a changed directory that cannot be fully walked leaves the timestamp unknown", { skip: process.getuid?.() === 0 && "root reads directories regardless of mode" }, (context) => { ``` - lens: test-trimming · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - disposition: none > What I examined: the new test `a changed directory that cannot be fully walked leaves the timestamp unknown` in audit-checkouts.test.mjs, the function it targets (`newest_directory_epoch` in audit-checkouts.sh, added in this change), the sibling helper pattern `failStat` in `createActivityFixture`, and `.forgejo/workflows/node-test.yml`. > > What the test does: it builds the failure by chmod-ing a nested directory to 0o000 so `find` cannot descend, and guards the whole test with > > { skip: process.getuid?.() === 0 && "root reads directories regardless of mode" } > > The skip is correct as far as it goes: root bypasses directory mode bits, so under uid 0 `find` succeeds and the setup cannot produce the failure. But it means the test contributes nothing wherever the suite runs as root. > > What it is the only protection for: the failure branch of the new helper in audit-checkouts.sh: > > if ! find "$directory_path" -type f -print0 >"$listing" 2>/dev/null; then > rm -f "$listing" > return 1 > fi > > and the caller in `annotate_activity_review` that turns that `return 1` into `Changed-path activity timestamp unavailable: <path>` and an `unknown` outcome. No other test in the file drives `newest_directory_epoch` to failure; the other new directory test (`a changed directory is timed by the newest file inside it`) covers only the success path. A regression such as dropping the `if ! find ...; then return 1` guard (falling back to the directory's own mtime and reporting `possibly-abandoned` instead of `unknown`) would stay green on any root runner. > > Observed: in this review sandbox `id -u` prints 0, so the skip predicate fires here. Proof gap: `node-test.yml` uses `runs-on: ubuntu-latest` with no `container:` or user setting, and I cannot see the Forgejo runner's configuration; container-based runners commonly execute as root, but I could not confirm it for this repository. If the project's runner executes as a non-root user the test does run in CI and this claim reduces to a portability note. > > Repair that keeps the protection on every uid: the fixture already shows the pattern for forcing a helper to fail without relying on permissions: > > 'source "$1"; stat_epoch() { return 1; }; annotate_activity_review ...' > > The same technique works for the walk: source the script, define `find() { return 1; }` (or prepend a failing `find` shim via the existing `ignoredCommandEnvironment` helper) and then call `annotate_activity_review` on a fixture whose changed path is a directory. That exercises the exact `find` failure branch, asserts `outcome === "unknown"` and the `timestamp unavailable: sample.txt` error, and does not depend on the process uid. The chmod variant can stay as the real-permissions scenario for non-root runs, but it should not be the only check of this branch. ### low — Renderer reports "origin fetch failed" for a default checkout whose fetch succeeded but whose server default-branch lookup failed - claim: `01M3907BR6AHXCVJN2G922EACN` - anchor: `skills/audit-git-checkouts/scripts/render-audit-report.ts` (snippet) ``` if (fetched) return { aheadBehind: formatAheadBehind(comparison), why: "not compared: origin fetch failed" }; ``` - lens: general-bug · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - disposition: none > What I examined: primaryDecision in render-audit-report.ts (the manual-review branch), repositoryFailures in the same file, and audit_repository in audit-checkouts.sh, which is the only producer of defaultComparison.fresh. > > Mechanism: the driver sets comparison_fresh only when both steps succeed: `if [ "$fetch_ok" = true ] && [ "$remote_head_ok" = true ]; then comparison_fresh=true; fi`. remote_head_ok comes from `git ls-remote --symref origin HEAD`, which can fail (or return no `ref: refs/heads/...` line) while `git fetch --prune origin` succeeds, for example on a remote whose HEAD is unborn or detached, or a mirror that does not advertise a symref. In that case every worktree gets defaultComparison.fresh=false, classify_checkout labels a default-branch checkout manual-review, and the renderer's manual-review branch reaches `if (fetched) return { ..., why: "not compared: origin fetch failed" }`. > > What goes wrong: the 'Needs your decision: not current' row tells the user the fetch failed, while the Failures table for the same repository, built by repositoryFailures, says the opposite: `fetch failed` is only pushed when fetch.ok !== true, and this case instead produces "could not read the server's default branch". The two sections of the same report contradict each other, and the user is sent to debug network/auth on a fetch that worked. stepFailure already handles the same condition correctly for linked worktrees with the wording "origin fetch or default-branch lookup failed". > > Static reasoning only: I could not run the driver here (jq is not installed in this sandbox), so I did not produce a real report exhibiting this; the path is traced from the code. The renderer tests cover only the fetch.ok=false case ("a fetched run never counts an uncompared checkout as current"). > > Safe correction: use the same neutral wording stepFailure uses, e.g. "not compared: origin fetch or default-branch lookup failed", or pass the repository's fetch.ok / defaultBranchResolution.remoteHeadOk into primaryDecision and pick the message from those. ## Other claims - grounding-pending (0) - ungrounded (0) - rejected (1) - `01M38ZXWW77WWY24J79KP8EKYN` low — Rewrapped concurrency comment leaves one 122-column line mid-paragraph - duplicate-of (0) - unadjudicated (0) ## Coverage Coverage pass: 01M38ZNKBYFX4T077GYQ7Y1CJ6 Accounting: complete Slot health: healthy | lens | part | arm | unit status | runs | loss | | --- | --- | --- | --- | --- | --- | | general-bug | whole | default | claims-emitted | 1 | no | | writing-quality | whole | default | claims-emitted | 1 | no | | test-trimming | whole | default | claims-emitted | 1 | no |
@ -88,3 +76,1 @@
# signal handler, so a kill here writes nothing to the PR — the durable
# fix is an internal deadline in the wrapper, not a larger number here.
# Re-derive when the pin moves.
# Limit runner occupancy even if retries or reconciliation are unfinished.

low — The 130-minute timeout comment states the obvious purpose and drops the derivation a maintainer needs to change it
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the review job in .forgejo/workflows/review.yml, its timeout-minutes: 130 setting, the comment above it, and the file's own header ("Rendered by j4k-align ... Direct edits are drift").

What the subject says: the diff replaces a comment that derived 130 from the pinned wrapper's worst-case wait budget and ended with "Re-derive when the pin moves" with the single line "Limit runner occupancy even if retries or reconciliation are unfinished."

What goes wrong: that sentence fails the writing skill's no-op test. Every reader already knows what timeout-minutes is for; the comment adds nothing they lack. What a maintainer actually needs when the wrapper pin (review-wrapper@21cd18a8...) moves, or when 130 looks excessive next to the sibling workflows' 5 and 10 minutes, is where the number comes from and whether it may be lowered. The old text supplied that; the new text leaves 130 looking arbitrary, so the next editor either keeps a stale ceiling after a wrapper change or trims it below the wrapper's real worst case and gets a killed run that, as the removed comment noted, "writes nothing to the PR". The skill's guidance is explicit here: "Keep causal facts that help a future agent apply a rule" and its Good example keeps the arithmetic ("Cap each CI worker's connection pool at 5: 16 concurrent workers then use at most 80 of the database's 100 connections").

Proposed correction (one or two sentences, not the old paragraph): "# The pinned wrapper can spend about 100 min against the review service before it writes anything (60 min of stage deadlines plus retry and transport overruns), and forge calls carry no timeout; 130 leaves roughly 30 min for them. A kill here posts nothing to the PR. Re-derive when the pin moves." That preserves the durable cause and the maintenance trigger without the change-history narration the old comment carried.

Proof status: this is a documentation judgment from the diff and the tree; I cannot verify the wrapper's current budget because the wrapper is not in the subject. The numbers in the proposed text come from the removed comment and should be checked against the pinned wrapper before being written back.

claim 01M36XAYBSYDTD1A2XS6FWJA7Z of review 01M36X1VFPSMHFG53KPRE556CN

<!-- review:claim:01M36XAYBSYDTD1A2XS6FWJA7Z --> **low** — The 130-minute timeout comment states the obvious purpose and drops the derivation a maintainer needs to change it lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the `review` job in .forgejo/workflows/review.yml, its `timeout-minutes: 130` setting, the comment above it, and the file's own header ("Rendered by j4k-align ... Direct edits are drift"). > > What the subject says: the diff replaces a comment that derived 130 from the pinned wrapper's worst-case wait budget and ended with "Re-derive when the pin moves" with the single line "Limit runner occupancy even if retries or reconciliation are unfinished." > > What goes wrong: that sentence fails the writing skill's no-op test. Every reader already knows what `timeout-minutes` is for; the comment adds nothing they lack. What a maintainer actually needs when the wrapper pin (`review-wrapper@21cd18a8...`) moves, or when 130 looks excessive next to the sibling workflows' 5 and 10 minutes, is where the number comes from and whether it may be lowered. The old text supplied that; the new text leaves 130 looking arbitrary, so the next editor either keeps a stale ceiling after a wrapper change or trims it below the wrapper's real worst case and gets a killed run that, as the removed comment noted, "writes nothing to the PR". The skill's guidance is explicit here: "Keep causal facts that help a future agent apply a rule" and its Good example keeps the arithmetic ("Cap each CI worker's connection pool at 5: 16 concurrent workers then use at most 80 of the database's 100 connections"). > > Proposed correction (one or two sentences, not the old paragraph): "# The pinned wrapper can spend about 100 min against the review service before it writes anything (60 min of stage deadlines plus retry and transport overruns), and forge calls carry no timeout; 130 leaves roughly 30 min for them. A kill here posts nothing to the PR. Re-derive when the pin moves." That preserves the durable cause and the maintenance trigger without the change-history narration the old comment carried. > > Proof status: this is a documentation judgment from the diff and the tree; I cannot verify the wrapper's current budget because the wrapper is not in the subject. The numbers in the proposed text come from the removed comment and should be checked against the pinned wrapper before being written back. claim `01M36XAYBSYDTD1A2XS6FWJA7Z` of review `01M36X1VFPSMHFG53KPRE556CN`
Author
Owner

This file is not editable here. review.yml is rendered by j4k-align from templates/workflows/review-forgejo.yml.hbs, and the template's current text (align #270, 4ffe59e) is exactly this one-line comment; the header on line 2 marks direct edits as drift. The diff in this PR is the regeneration. If the derivation should come back, that is a change to the align template.

<!-- gh-feedback:reply-to:86201 --> This file is not editable here. `review.yml` is rendered by j4k-align from `templates/workflows/review-forgejo.yml.hbs`, and the template's current text (align #270, 4ffe59e) is exactly this one-line comment; the header on line 2 marks direct edits as drift. The diff in this PR is the regeneration. If the derivation should come back, that is a change to the align template.

superseded by review 01M37ANQNHYDFD5YK60V7FTRER for head 38f0009ff31a9421a94277906c2cad807c604891

<!-- review:superseded:01M37ANQNHYDFD5YK60V7FTRER --> superseded by review `01M37ANQNHYDFD5YK60V7FTRER` for head `38f0009ff31a9421a94277906c2cad807c604891`
@ -78,3 +47,1 @@
jq '.repositories[] | select(.fatalError != null or .fetch.ok != true or .defaultBranchResolution.remoteHeadOk != true or .forge == null or .defaultBranch == null or .worktreesError != null or .gitPrune.ok != true or .staleRegistrations.ok != true) | {checkout, fatalError, forgeError, defaultBranchError, defaultBranchResolution, fetch, worktreesError, gitPrune, staleRegistrations}' "$report"
jq '.repositories[].worktrees[] | select(.removal.outcome != "removed") | select(.statusError != null or .outsideRoot or .defaultComparison == null or .defaultComparison.fresh != true) | {path, outsideRoot, statusError, defaultComparison}' "$report"
```
When the report does not answer a question, read the item's JSON record, where `<name>` is the path as `report.md` shows it, relative to the root: `jq --arg p <name> '.repositories[].worktrees[] | select(.path | endswith("/" + $p))' "$report/report.json"`. A failure row means coverage is incomplete for that checkout; never report it as clean.

low — Documented jq lookup matches every worktree whose path ends in the display name, and never matches the root entry
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the "Report, then work the decisions" section of SKILL.md, which tells the agent how to find an item's JSON record when report.md does not answer a question, and the renderer scripts/render-audit-report.ts, which produces the names report.md shows.

What the subject says: "read the item's JSON record, where is the path as report.md shows it, relative to the root: jq --arg p '.repositories[].worktrees[] | select(.path | endswith("/" + $p))' "$report/report.json"".

What goes wrong: the renderer displays each path as relative(report.root, path) || "." (formatAuditReport, const display = (path: string) => relative(report.root, path) || ".";). Two consequences for the documented command:

  • A suffix match is not a path match. With a root holding both /app and /tools/app, report.md shows them as app and tools/app; the query for app returns both records, because both absolute paths end in "/app". The agent then reads the wrong worktree's containment, lock, or ignored-scan evidence and carries that into a recommendation. The skill elsewhere insists on exact identities ("find the current stash@{N} by the proved SHA"), so an ambiguous lookup contradicts its own standard.
  • A repository at the audit root itself is shown as ., and endswith("/.") matches nothing, so the documented command silently returns no record for that checkout.

Applicable guidance: the writing skill asks for verifiable, precise commands ("Make claims verifiable", "show a full invocation only when ... unusually error-prone"). A lookup command shipped in the skill should be exact, because the agent will copy it.

Proposed correction: the report carries root, so an exact join is available: jq --arg p <name> '.root as $r | .repositories[].worktrees[] | select(.path == ($r + "/" + $p) or ($p == "." and .path == $r))' "$report/report.json". That keeps the useful meaning (look up by the name report.md shows) and removes the ambiguity and the root-entry gap.

Proof status: established by static reading of the renderer's display() and the jq expression; I did not run the driver, and I assume report.root and worktree paths are both absolute and canonical, which the renderer's relative() call already assumes.

claim 01M36XAXYEJ6DH8NQM1DHNJ8BQ of review 01M36X1VFPSMHFG53KPRE556CN

<!-- review:claim:01M36XAXYEJ6DH8NQM1DHNJ8BQ --> **low** — Documented jq lookup matches every worktree whose path ends in the display name, and never matches the root entry lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the "Report, then work the decisions" section of SKILL.md, which tells the agent how to find an item's JSON record when report.md does not answer a question, and the renderer scripts/render-audit-report.ts, which produces the names report.md shows. > > What the subject says: "read the item's JSON record, where <name> is the path as report.md shows it, relative to the root: jq --arg p <name> '.repositories[].worktrees[] | select(.path | endswith("/" + $p))' "$report/report.json"". > > What goes wrong: the renderer displays each path as relative(report.root, path) || "." (formatAuditReport, `const display = (path: string) => relative(report.root, path) || ".";`). Two consequences for the documented command: > - A suffix match is not a path match. With a root holding both <root>/app and <root>/tools/app, report.md shows them as `app` and `tools/app`; the query for `app` returns both records, because both absolute paths end in "/app". The agent then reads the wrong worktree's containment, lock, or ignored-scan evidence and carries that into a recommendation. The skill elsewhere insists on exact identities ("find the current stash@{N} by the proved SHA"), so an ambiguous lookup contradicts its own standard. > - A repository at the audit root itself is shown as `.`, and endswith("/.") matches nothing, so the documented command silently returns no record for that checkout. > > Applicable guidance: the writing skill asks for verifiable, precise commands ("Make claims verifiable", "show a full invocation only when ... unusually error-prone"). A lookup command shipped in the skill should be exact, because the agent will copy it. > > Proposed correction: the report carries `root`, so an exact join is available: `jq --arg p <name> '.root as $r | .repositories[].worktrees[] | select(.path == ($r + "/" + $p) or ($p == "." and .path == $r))' "$report/report.json"`. That keeps the useful meaning (look up by the name report.md shows) and removes the ambiguity and the root-entry gap. > > Proof status: established by static reading of the renderer's display() and the jq expression; I did not run the driver, and I assume report.root and worktree paths are both absolute and canonical, which the renderer's relative() call already assumes. claim `01M36XAXYEJ6DH8NQM1DHNJ8BQ` of review `01M36X1VFPSMHFG53KPRE556CN`
Author
Owner

Fixed in 38f0009: the lookup now joins on .root, with . naming a checkout at the root itself. Reproduced first on a driver run whose root is the checkout: the old query returned nothing, the new one returns the record.

<!-- gh-feedback:reply-to:86202 --> Fixed in 38f0009: the lookup now joins on `.root`, with `.` naming a checkout at the root itself. Reproduced first on a driver run whose root is the checkout: the old query returned nothing, the new one returns the record.
jercik marked this conversation as resolved
Lines 727-730
@ -786,0 +724,7 @@
test("a replace ref forces a fresh containment proof at removal", (context) => {
const fixture = createContainedLinkedWorktree(context);
recordAuditContainment(fixture);
addReplaceRef(fixture.repositoryPath);
const result = runMaybeRemove(fixture);
assert.equal(result.removal.containment.reused, false);

low — Replace-ref containment tests assert only reused === false, so a fresh proof that errors or fails would keep them green
lens test-trimming · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the two new tests "a replace ref forces a fresh containment proof at removal" and "a proof recorded under a replace ref is never reused" in audit-checkouts.test.mjs, the decide_removal_outcome and maybe_remove_worktree functions in audit-checkouts.sh, and write_containment (which produces the reusable flag). I could not execute the shell suite in this sandbox because jq is not installed; this is a code trace.

What the tests promise: with a replace ref present, removal must run a fresh containment proof instead of reusing the audit-time record. Both tests end with a single assertion on the removal record:

const result = runMaybeRemove(fixture);
assert.equal(result.removal.containment.reused, false);

What the implementation does: in decide_removal_outcome, the reuse branch is gated on [ -z "$replace_refs" ] && jq -e '... .reusable == true ...'. Every non-reuse path, including a prover that crashes or returns "not contained", writes the same shape:

jq -n --slurpfile verdict ... '{verdict: ($verdict[0] // null), stderr: $stderr, reused: false}' >"$containment_path"

if [ "$containment_status" -eq 1 ]; then echo judgment/containment-not-proven; return; fi
if [ "$containment_status" -ne 0 ]; then echo operational/containment-check-failed; return; fi

So reused: false is emitted identically whether the fresh proof succeeded (outcome removed), was rejected (judgment/containment-not-proven), or errored (operational/containment-check-failed, verdict: null).

Concrete regression that stays green: the fresh prover is invoked with GIT_NO_REPLACE_OBJECTS=1 "$script_directory/prove-branch-contained.sh" .... If that invocation regressed so that the prover fails whenever refs/replace/* exists (for example the env var dropped and rung 1 misreads history, or the prover exits non-zero on the replaced object), containment.reused is still false, and both tests pass while removal silently stops working for every repository that carries a replace ref. The sibling test "removal reuses the audit-time proof when its inputs are unchanged" does assert result.removal.outcome === "removed", so the fresh-proof-under-replace-ref path is the only one left without an outcome check.

Recommended repair (keeps the existing regression protection and adds the missing one): in both tests also assert the fresh proof actually produced a verdict, e.g. assert.equal(result.removal.outcome, "removed") and assert.equal(result.removal.containment.verdict.verdict, "contained"), mirroring the assertions already used in "contained detached worktrees pass the explicit removal gates". The fixture's replace ref maps an unrelated blob (hash-object -w --stdin of "original") to another unrelated blob, so it should not affect the rung-1 ancestry proof and removed is the expected outcome.

Proof gap: I could not run runMaybeRemove here to confirm that the outcome under the replace ref is in fact removed; running the suite with jq available and printing result.removal.outcome in either test would settle it. If the outcome is something other than removed, that is a bug for the general-bug lens rather than a reason to keep the assertion narrow.

claim 01M36XA1G5CCXA18S8BKMZDBVK of review 01M36X1VFPSMHFG53KPRE556CN

<!-- review:claim:01M36XA1G5CCXA18S8BKMZDBVK --> **low** — Replace-ref containment tests assert only `reused === false`, so a fresh proof that errors or fails would keep them green lens `test-trimming` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the two new tests "a replace ref forces a fresh containment proof at removal" and "a proof recorded under a replace ref is never reused" in audit-checkouts.test.mjs, the `decide_removal_outcome` and `maybe_remove_worktree` functions in audit-checkouts.sh, and `write_containment` (which produces the `reusable` flag). I could not execute the shell suite in this sandbox because `jq` is not installed; this is a code trace. > > What the tests promise: with a replace ref present, removal must run a *fresh* containment proof instead of reusing the audit-time record. Both tests end with a single assertion on the removal record: > > const result = runMaybeRemove(fixture); > assert.equal(result.removal.containment.reused, false); > > What the implementation does: in `decide_removal_outcome`, the reuse branch is gated on `[ -z "$replace_refs" ] && jq -e '... .reusable == true ...'`. Every non-reuse path, including a prover that crashes or returns "not contained", writes the same shape: > > jq -n --slurpfile verdict ... '{verdict: ($verdict[0] // null), stderr: $stderr, reused: false}' >"$containment_path" > > if [ "$containment_status" -eq 1 ]; then echo judgment/containment-not-proven; return; fi > if [ "$containment_status" -ne 0 ]; then echo operational/containment-check-failed; return; fi > > So `reused: false` is emitted identically whether the fresh proof succeeded (outcome `removed`), was rejected (`judgment/containment-not-proven`), or errored (`operational/containment-check-failed`, `verdict: null`). > > Concrete regression that stays green: the fresh prover is invoked with `GIT_NO_REPLACE_OBJECTS=1 "$script_directory/prove-branch-contained.sh" ...`. If that invocation regressed so that the prover fails whenever `refs/replace/*` exists (for example the env var dropped and rung 1 misreads history, or the prover exits non-zero on the replaced object), `containment.reused` is still `false`, and both tests pass while removal silently stops working for every repository that carries a replace ref. The sibling test "removal reuses the audit-time proof when its inputs are unchanged" does assert `result.removal.outcome === "removed"`, so the fresh-proof-under-replace-ref path is the only one left without an outcome check. > > Recommended repair (keeps the existing regression protection and adds the missing one): in both tests also assert the fresh proof actually produced a verdict, e.g. `assert.equal(result.removal.outcome, "removed")` and `assert.equal(result.removal.containment.verdict.verdict, "contained")`, mirroring the assertions already used in "contained detached worktrees pass the explicit removal gates". The fixture's replace ref maps an unrelated blob (`hash-object -w --stdin` of "original") to another unrelated blob, so it should not affect the rung-1 ancestry proof and `removed` is the expected outcome. > > Proof gap: I could not run `runMaybeRemove` here to confirm that the outcome under the replace ref is in fact `removed`; running the suite with `jq` available and printing `result.removal.outcome` in either test would settle it. If the outcome is something other than `removed`, that is a bug for the general-bug lens rather than a reason to keep the assertion narrow. claim `01M36XA1G5CCXA18S8BKMZDBVK` of review `01M36X1VFPSMHFG53KPRE556CN`
Author
Owner

Fixed in 4605fb7: both replace-ref tests now also assert removal.outcome === "removed" and containment.verdict.verdict === "contained". The suite passes with them, which also settles your proof gap: the fresh proof under a replace ref does remove.

<!-- gh-feedback:reply-to:86203 --> Fixed in 4605fb7: both replace-ref tests now also assert `removal.outcome === "removed"` and `containment.verdict.verdict === "contained"`. The suite passes with them, which also settles your proof gap: the fresh proof under a replace ref does remove.
jercik marked this conversation as resolved
@ -0,0 +372,4 @@
return `${sections.join("\n\n")}\n`;
}
// import.meta.main needs Node 24.2+.

low — Comment names a feature the code does not use, so the Node floor it implies conflicts with the skill's stated requirement
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the CLI guard at the bottom of scripts/render-audit-report.ts, the comment above it, SKILL.md's requirement sentence ("the renderer needs Node 24+"), the repository's .node-version (26.5.0), and skills/node/rules/assets/graceful-server.ts, which uses if (import.meta.main) { directly.

What the subject says: the comment reads "// import.meta.main needs Node 24.2+." and sits directly above if (process.argv[1] !== undefined && realpathSync(process.argv[1]) === realpathSync(fileURLToPath(import.meta.url))) {. Nothing in the file uses import.meta.main.

What goes wrong: a literal reader has two plausible readings and no way to choose. Reading A: this file requires Node 24.2+, which contradicts SKILL.md's "Node 24+" floor and would make the skill's stated requirement wrong. Reading B: the realpath comparison is a deliberate workaround so the renderer also runs on Node 24.0 and 24.1, which is the only reading that fits the code, but the comment never says so, and the sibling file in this repository uses import.meta.main without any such workaround. The writing skill asks that a comment "explain only what is non-obvious, version-specific, or easy to misuse" and that version-dependent claims be "scoped to their versions"; this line is version-specific and unscoped, and the non-obvious part (why not the simpler idiom) is exactly what it omits. The concrete cost is small but real: a maintainer either "fixes" the guard to import.meta.main and silently raises the floor above what SKILL.md promises, or edits SKILL.md to 24.2+ on the strength of a comment that was not stating a requirement.

Proposed correction, pick one:

  • Keep the workaround and say why: "// Entry-point check by path: import.meta.main would be simpler but needs Node 24.2+, and the skill promises Node 24."
  • Or adopt the repository's own idiom, if (import.meta.main) {, and change SKILL.md to "the renderer needs Node 24.2+", since .node-version already pins 26.5.0 for CI.

Either removes the contradiction while preserving the version fact. Proof status: static reading only; I did not execute the renderer on Node 24.0 to confirm the guard behaves there.

claim 01M36XAYS7S1510B8BYCKG7SZX of review 01M36X1VFPSMHFG53KPRE556CN

<!-- review:claim:01M36XAYS7S1510B8BYCKG7SZX --> **low** — Comment names a feature the code does not use, so the Node floor it implies conflicts with the skill's stated requirement lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the CLI guard at the bottom of scripts/render-audit-report.ts, the comment above it, SKILL.md's requirement sentence ("the renderer needs Node 24+"), the repository's .node-version (26.5.0), and skills/node/rules/assets/graceful-server.ts, which uses `if (import.meta.main) {` directly. > > What the subject says: the comment reads "// import.meta.main needs Node 24.2+." and sits directly above `if (process.argv[1] !== undefined && realpathSync(process.argv[1]) === realpathSync(fileURLToPath(import.meta.url))) {`. Nothing in the file uses import.meta.main. > > What goes wrong: a literal reader has two plausible readings and no way to choose. Reading A: this file requires Node 24.2+, which contradicts SKILL.md's "Node 24+" floor and would make the skill's stated requirement wrong. Reading B: the realpath comparison is a deliberate workaround so the renderer also runs on Node 24.0 and 24.1, which is the only reading that fits the code, but the comment never says so, and the sibling file in this repository uses import.meta.main without any such workaround. The writing skill asks that a comment "explain only what is non-obvious, version-specific, or easy to misuse" and that version-dependent claims be "scoped to their versions"; this line is version-specific and unscoped, and the non-obvious part (why not the simpler idiom) is exactly what it omits. The concrete cost is small but real: a maintainer either "fixes" the guard to import.meta.main and silently raises the floor above what SKILL.md promises, or edits SKILL.md to 24.2+ on the strength of a comment that was not stating a requirement. > > Proposed correction, pick one: > - Keep the workaround and say why: "// Entry-point check by path: `import.meta.main` would be simpler but needs Node 24.2+, and the skill promises Node 24." > - Or adopt the repository's own idiom, `if (import.meta.main) {`, and change SKILL.md to "the renderer needs Node 24.2+", since .node-version already pins 26.5.0 for CI. > > Either removes the contradiction while preserving the version fact. Proof status: static reading only; I did not execute the renderer on Node 24.0 to confirm the guard behaves there. claim `01M36XAYS7S1510B8BYCKG7SZX` of review `01M36X1VFPSMHFG53KPRE556CN`
Author
Owner

Fixed in 38f0009: the comment now says the path comparison is the workaround and why: import.meta.main would be simpler but needs Node 24.2+, and SKILL.md promises Node 24.

<!-- gh-feedback:reply-to:86204 --> Fixed in 38f0009: the comment now says the path comparison is the workaround and why: `import.meta.main` would be simpler but needs Node 24.2+, and SKILL.md promises Node 24.
jercik marked this conversation as resolved
@ -20,20 +20,6 @@ jobs:
with:

high — Node test workflow drops the jq install step while audit-checkouts.test.mjs still shells out to a jq-dependent driver
lens general-bug · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the diff for .forgejo/workflows/node-test.yml, the current workflow in the tree, skills/audit-git-checkouts/scripts/audit-checkouts.sh, and skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs.

What changed: the diff deletes the whole 'Ensure baseline shell tooling' step, whose base-side comment read: "The Forge maps ubuntu-latest to a minimal Node image, so tools GitHub's runner preinstalls are absent and a test shelling out to one dies here." That step ran apt-get to install jq (plus lsof and bsdextrautils) when missing. The remaining 'Run tests' step now runs straight after actions/setup-node, and the same change adds render-audit-report.test.ts to the selected test list while keeping audit-checkouts.test.mjs.

Why it breaks: audit-checkouts.test.mjs spawns /bin/bash to source audit-checkouts.sh and call its functions (write_latest_change, classify, the removal gates, etc.), and the driver calls jq on nearly every path (e.g. jq -n in write_latest_change, jq -r '.path' in annotate_lock, jq -e in decide_removal_outcome). prove-branch-contained.sh also hard-fails with required command not found: jq. Nothing in the test file skips when jq is absent.

Observed reproduction (this sandbox had no jq, as a minimal Node image would not): node --test skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs failed with many errors of the form audit-checkouts.sh: line 250: jq: command not found / line 264: jq: command not found, status 127. After apt-get install jq, the same command passed all 54 tests. So the outcome of the CI job hinges entirely on whether the runner image ships jq, and the deleted step existed precisely because it does not.

Proof gap: I cannot inspect the Forge's ubuntu-latest image; the evidence that it lacks jq is the removed comment and the removed install step itself. If the image was changed to include jq, this claim is moot; otherwise every push to main and every PR fails the node:test job at the first jq call.

Safe correction: restore an install step (at least jq) before 'Run tests', or gate audit-checkouts.test.mjs on command -v jq with an explicit skip so the failure is a visible skip rather than a 127 crash.

claim 01M378XKFYG4AG7VH14W6XS5JW of review 01M36X1VFPSMHFG53KPRE556CN

<!-- review:claim:01M378XKFYG4AG7VH14W6XS5JW --> **high** — Node test workflow drops the jq install step while audit-checkouts.test.mjs still shells out to a jq-dependent driver lens `general-bug` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the diff for .forgejo/workflows/node-test.yml, the current workflow in the tree, skills/audit-git-checkouts/scripts/audit-checkouts.sh, and skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs. > > What changed: the diff deletes the whole 'Ensure baseline shell tooling' step, whose base-side comment read: "The Forge maps ubuntu-latest to a minimal Node image, so tools GitHub's runner preinstalls are absent and a test shelling out to one dies here." That step ran apt-get to install jq (plus lsof and bsdextrautils) when missing. The remaining 'Run tests' step now runs straight after actions/setup-node, and the same change adds render-audit-report.test.ts to the selected test list while keeping audit-checkouts.test.mjs. > > Why it breaks: audit-checkouts.test.mjs spawns /bin/bash to source audit-checkouts.sh and call its functions (write_latest_change, classify, the removal gates, etc.), and the driver calls jq on nearly every path (e.g. `jq -n` in write_latest_change, `jq -r '.path'` in annotate_lock, `jq -e` in decide_removal_outcome). prove-branch-contained.sh also hard-fails with `required command not found: jq`. Nothing in the test file skips when jq is absent. > > Observed reproduction (this sandbox had no jq, as a minimal Node image would not): `node --test skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs` failed with many errors of the form `audit-checkouts.sh: line 250: jq: command not found` / `line 264: jq: command not found`, status 127. After `apt-get install jq`, the same command passed all 54 tests. So the outcome of the CI job hinges entirely on whether the runner image ships jq, and the deleted step existed precisely because it does not. > > Proof gap: I cannot inspect the Forge's ubuntu-latest image; the evidence that it lacks jq is the removed comment and the removed install step itself. If the image was changed to include jq, this claim is moot; otherwise every push to main and every PR fails the node:test job at the first jq call. > > Safe correction: restore an install step (at least `jq`) before 'Run tests', or gate audit-checkouts.test.mjs on `command -v jq` with an explicit skip so the failure is a visible skip rather than a 127 crash. claim `01M378XKFYG4AG7VH14W6XS5JW` of review `01M36X1VFPSMHFG53KPRE556CN`
Author
Owner

The Forge runner ships jq now. This head's own Node tests / node:test run (#1396, run 44843) executed audit-checkouts.test.mjs with no install step and passed 54/54, shell-only cases included (a replace ref forces a fresh containment proof at removal is in its log). The bootstrap step was dropped in j4k-align (af91298, #233), and node-test.yml is rendered by align, so a step re-added here would be drift that the next regeneration removes.

<!-- gh-feedback:reply-to:86665 --> The Forge runner ships jq now. This head's own `Node tests / node:test` run (#1396, run 44843) executed `audit-checkouts.test.mjs` with no install step and passed 54/54, shell-only cases included (`a replace ref forces a fresh containment proof at removal` is in its log). The bootstrap step was dropped in j4k-align (af91298, #233), and `node-test.yml` is rendered by align, so a step re-added here would be drift that the next regeneration removes.

superseded by review 01M37ANQNHYDFD5YK60V7FTRER for head 38f0009ff31a9421a94277906c2cad807c604891

<!-- review:superseded:01M37ANQNHYDFD5YK60V7FTRER --> superseded by review `01M37ANQNHYDFD5YK60V7FTRER` for head `38f0009ff31a9421a94277906c2cad807c604891`
@ -92,1 +42,3 @@
| `judgment/precious-ignored-files` | Leave `preciousPaths` in place until the owner disposes of them or explicitly authorizes their disposition; no archival bypass. Rerun the driver afterward. |
| `judgment/containment-not-proven` | Investigate rungs 4–5 in [containment.md](containment.md); keep it without a concrete proof. |
| `judgment/worktree-locked` | The lock is under seven days old or future-dated. Wait for expiry, or unlock once the owner confirms the pin no longer applies. |
| `judgment/hidden-index-flags` | Have the owner clear hand-set flags, prove the revealed tree clean, then rerun. `git ls-files -v` shows assume-unchanged as lowercase letters; clear only those (`--no-assume-unchanged`) or hand-set `skip-worktree` outside a sparse cone. |

low — "or hand-set skip-worktree outside a sparse cone" reads as an instruction to set the flag, and the clearing flag is missing
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the "Resolve a kept worktree" table in skills/audit-git-checkouts/references/removal-gates.md, the pre-change wording in the diff, and the driver's index-flag gate in scripts/audit-checkouts.sh, which classifies any lowercase git ls-files -v letter as assume-unchanged and an uppercase S outside core.sparseCheckout as a hand-set skip-worktree bit.

What the subject says: "clear only those (--no-assume-unchanged) or hand-set skip-worktree outside a sparse cone." The word "hand-set" is doing double duty. Read as an adjective it means "clear only those, or [clear] a skip-worktree bit someone set by hand outside a sparse cone", which is the intended meaning. Read as a verb, which is the natural parse after "clear ... or", it becomes "clear those, or set skip-worktree by hand outside a sparse cone", the opposite of the fix. The pre-change text avoided this by naming both clearing flags: "--no-assume-unchanged, or --no-skip-worktree outside the sparse-cone case". The rewrite dropped --no-skip-worktree, so the only flag on the page is the one for assume-unchanged, and the reader has to guess the second.

What goes wrong: this cell is read by an agent acting on a blocked removal, and the skill's guidance is to add precision wherever prose "leaves a plausible wrong interpretation". An agent that sets skip-worktree instead of clearing it hides more tracked changes and makes the gate harder to pass; an agent that clears the flag on a sparse cone turns absent files into apparent deletions, which the very next row warns about.

Proposed correction for the cell, preserving the one-line table shape:

Have the owner clear the flags they set, prove the revealed tree clean, then rerun. In `git ls-files -v`, a lowercase letter is assume-unchanged (clear with `--no-assume-unchanged`); an uppercase `S` outside a sparse cone is a hand-set skip-worktree bit (clear with `--no-skip-worktree`). Leave a sparse cone's `S` entries alone.

This names both flags with their letters, keeps "hand-set" unambiguously as a description, and keeps the sparse-cone boundary that the next row depends on.

claim 01M378V5KRT68C9G06X06B1FXQ of review 01M36X1VFPSMHFG53KPRE556CN

<!-- review:claim:01M378V5KRT68C9G06X06B1FXQ --> **low** — "or hand-set `skip-worktree` outside a sparse cone" reads as an instruction to set the flag, and the clearing flag is missing lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the "Resolve a kept worktree" table in `skills/audit-git-checkouts/references/removal-gates.md`, the pre-change wording in the diff, and the driver's index-flag gate in `scripts/audit-checkouts.sh`, which classifies any lowercase `git ls-files -v` letter as assume-unchanged and an uppercase `S` outside `core.sparseCheckout` as a hand-set skip-worktree bit. > > What the subject says: "clear only those (`--no-assume-unchanged`) or hand-set `skip-worktree` outside a sparse cone." The word "hand-set" is doing double duty. Read as an adjective it means "clear only those, or [clear] a skip-worktree bit someone set by hand outside a sparse cone", which is the intended meaning. Read as a verb, which is the natural parse after "clear ... or", it becomes "clear those, or set skip-worktree by hand outside a sparse cone", the opposite of the fix. The pre-change text avoided this by naming both clearing flags: "`--no-assume-unchanged`, or `--no-skip-worktree` outside the sparse-cone case". The rewrite dropped `--no-skip-worktree`, so the only flag on the page is the one for assume-unchanged, and the reader has to guess the second. > > What goes wrong: this cell is read by an agent acting on a blocked removal, and the skill's guidance is to add precision wherever prose "leaves a plausible wrong interpretation". An agent that sets skip-worktree instead of clearing it hides more tracked changes and makes the gate harder to pass; an agent that clears the flag on a sparse cone turns absent files into apparent deletions, which the very next row warns about. > > Proposed correction for the cell, preserving the one-line table shape: > > Have the owner clear the flags they set, prove the revealed tree clean, then rerun. In `git ls-files -v`, a lowercase letter is assume-unchanged (clear with `--no-assume-unchanged`); an uppercase `S` outside a sparse cone is a hand-set skip-worktree bit (clear with `--no-skip-worktree`). Leave a sparse cone's `S` entries alone. > > This names both flags with their letters, keeps "hand-set" unambiguously as a description, and keeps the sparse-cone boundary that the next row depends on. claim `01M378V5KRT68C9G06X06B1FXQ` of review `01M36X1VFPSMHFG53KPRE556CN`
Author
Owner

Fixed in 38f0009: the cell names both ls-files -v letters with their clearing switches (--no-assume-unchanged, --no-skip-worktree) and says to leave a sparse cone's S entries alone, matching what the driver gate distinguishes.

<!-- gh-feedback:reply-to:86666 --> Fixed in 38f0009: the cell names both `ls-files -v` letters with their clearing switches (`--no-assume-unchanged`, `--no-skip-worktree`) and says to leave a sparse cone's `S` entries alone, matching what the driver gate distinguishes.
jercik marked this conversation as resolved
@ -990,3 +971,2 @@
fi
echo operational/removal-failed
fi
if [ "$expired_lock" = true ] \
&& ! git -C "$common_git_dir" worktree unlock "$worktree_path" >>"$removal_output_path" 2>"$removal_error_path"; then

low — An expired lock is unlocked and the worktree removed without re-reading the lock marker after the containment proof and ignored scan, so a pin renewed in that window is discarded
lens general-bug · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: decide_removal_outcome in skills/audit-git-checkouts/scripts/audit-checkouts.sh (the lock gate, the containment proof, write_ignored_scan, the final status check, and the unlock/remove tail), the diff of that function, references/removal-gates.md, and the tests removed in audit-checkouts.test.mjs.

Mechanism: the lock gate reads the locked file's mtime once, early in decide_removal_outcome, and sets expired_lock=true when it is seven or more days old. It then runs the containment proof (which for rung 2 clones a scratch repository and can take seconds to minutes), the fresh ignored-content scan (up to 4,096 entries / 2 GiB of cmp), and a second git status. Only then does the tail run git worktree unlock followed by git worktree remove, with no re-read of the marker in between. The driver deliberately re-verifies other live state in that same tail (second status check, HEAD unchanged since candidate_head_before) but not the lock.

The base side of the diff had exactly that re-check: it re-stat'd locked immediately before unlock and returned judgment/worktree-locked if the mtime or reason differed or the file was gone. The diff removes that block along with the tests "a pin renewed during containment proof is not unlocked" and "a lock removed during containment proof is not treated as unlocked". The new comment documents only the other half of the change (no restore after a failed remove); nothing in the code comment or in removal-gates.md records that the pre-unlock re-check was dropped, and removal-gates.md still tells the owner that the way to renew a pin is "unlock and lock again" — which is precisely the sequence the window now misses.

Concrete failure: an owner runs git worktree unlock then git worktree lock --reason 'active deployment' on a worktree while the driver is between its early lock read and its unlock (the proof and scan window). The driver's unlock succeeds against the fresh pin, git worktree remove deletes the directory, and the report records removed. The other gates guarantee no uncommitted or unmerged commits are lost, but the pin's purpose (keep this directory in place) is defeated silently, and the report's output line still says expired worktree lock with the stale age.

Proof: static trace only; I did not reproduce the race. The window is real but narrow, hence low severity. A safe correction is to re-stat locked (and compare mtime/reason to the values read at the gate) immediately before worktree unlock, returning judgment/worktree-locked on any difference, as the removed block did.

claim 01M378Y8Y94SZJADKB1PMJ6YX2 of review 01M36X1VFPSMHFG53KPRE556CN

<!-- review:claim:01M378Y8Y94SZJADKB1PMJ6YX2 --> **low** — An expired lock is unlocked and the worktree removed without re-reading the lock marker after the containment proof and ignored scan, so a pin renewed in that window is discarded lens `general-bug` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: decide_removal_outcome in skills/audit-git-checkouts/scripts/audit-checkouts.sh (the lock gate, the containment proof, write_ignored_scan, the final status check, and the unlock/remove tail), the diff of that function, references/removal-gates.md, and the tests removed in audit-checkouts.test.mjs. > > Mechanism: the lock gate reads the `locked` file's mtime once, early in decide_removal_outcome, and sets expired_lock=true when it is seven or more days old. It then runs the containment proof (which for rung 2 clones a scratch repository and can take seconds to minutes), the fresh ignored-content scan (up to 4,096 entries / 2 GiB of cmp), and a second `git status`. Only then does the tail run `git worktree unlock` followed by `git worktree remove`, with no re-read of the marker in between. The driver deliberately re-verifies other live state in that same tail (second status check, HEAD unchanged since candidate_head_before) but not the lock. > > The base side of the diff had exactly that re-check: it re-stat'd `locked` immediately before unlock and returned judgment/worktree-locked if the mtime or reason differed or the file was gone. The diff removes that block along with the tests "a pin renewed during containment proof is not unlocked" and "a lock removed during containment proof is not treated as unlocked". The new comment documents only the other half of the change (no restore after a failed remove); nothing in the code comment or in removal-gates.md records that the pre-unlock re-check was dropped, and removal-gates.md still tells the owner that the way to renew a pin is "unlock and lock again" — which is precisely the sequence the window now misses. > > Concrete failure: an owner runs `git worktree unlock` then `git worktree lock --reason 'active deployment'` on a worktree while the driver is between its early lock read and its unlock (the proof and scan window). The driver's unlock succeeds against the fresh pin, `git worktree remove` deletes the directory, and the report records `removed`. The other gates guarantee no uncommitted or unmerged commits are lost, but the pin's purpose (keep this directory in place) is defeated silently, and the report's `output` line still says `expired worktree lock` with the stale age. > > Proof: static trace only; I did not reproduce the race. The window is real but narrow, hence low severity. A safe correction is to re-stat `locked` (and compare mtime/reason to the values read at the gate) immediately before `worktree unlock`, returning judgment/worktree-locked on any difference, as the removed block did. claim `01M378Y8Y94SZJADKB1PMJ6YX2` of review `01M36X1VFPSMHFG53KPRE556CN`
Author
Owner

The recheck was removed on purpose. It guarded a window of seconds against a renewal no tooling performs: git worktree lock refuses a second lock (exit 128, reproduced), so renewing a pin means an owner unlocking an already expired lock while an audit is mid-run and re-locking before that same run reaches removal. Expiry overriding stale pins is the intended behavior, and the other gates still protect every commit and uncommitted file. Clarified in 38f0009: the lock paragraph in removal-gates.md now states the lock is read once, at the gate, so the window is documented rather than implicit.

<!-- gh-feedback:reply-to:86667 --> The recheck was removed on purpose. It guarded a window of seconds against a renewal no tooling performs: `git worktree lock` refuses a second lock (exit 128, reproduced), so renewing a pin means an owner unlocking an already expired lock while an audit is mid-run and re-locking before that same run reaches removal. Expiry overriding stale pins is the intended behavior, and the other gates still protect every commit and uncommitted file. Clarified in 38f0009: the lock paragraph in removal-gates.md now states the lock is read once, at the gate, so the window is documented rather than implicit.

superseded by review 01M37ANQNHYDFD5YK60V7FTRER for head 38f0009ff31a9421a94277906c2cad807c604891

<!-- review:superseded:01M37ANQNHYDFD5YK60V7FTRER --> superseded by review `01M37ANQNHYDFD5YK60V7FTRER` for head `38f0009ff31a9421a94277906c2cad807c604891`
The two replace-ref tests asserted only that the audit-time proof was
not reused, so a fresh proof that failed or errored kept them green.
They now also assert the removal outcome and the contained verdict.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
docs(audit-git-checkouts): make the record lookup exact and name both index-flag switches
Some checks failed
commit-msg / commitlint (pull_request) Successful in 24s
Node tests / node:test (pull_request) Successful in 45s
Review / Review (pull_request_target) Failing after 4m7s
38f0009ff3
The documented jq lookup matched any worktree whose path ended in the
display name and never matched a checkout shown as `.`; it now joins on
`.root`. The hidden-index-flags cell names both `ls-files -v` letters
with their clearing switches, the lock paragraph states that the lock is
read once at the gate, and the renderer's entry-point comment says why
it avoids `import.meta.main`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ -34,4 +23,1 @@
apt-get update
apt-get install -y --no-install-recommends "${missing[@]}"
- name: Run tests

medium — node-test.yml drops the jq/lsof install step while the selected test files still shell out to jq and lsof, so the CI job fails on the minimal runner image the removed step existed for
lens general-bug · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the diff of .forgejo/workflows/node-test.yml, the four selected test files it now runs, audit-checkouts.sh, prove-branch-contained.sh, and move-codex-session.ts.

What changed: the workflow deleted the Ensure baseline shell tooling step, whose comment read "The Forge maps ubuntu-latest to a minimal Node image, so tools GitHub's runner preinstalls are absent and a test shelling out to one dies here", and which installed jq, lsof, and bsdextrautils (column) when missing. No replacement step was added and the Run tests step still invokes the same test files plus the new renderer test.

What still needs those tools: audit-checkouts.test.mjs sources audit-checkouts.sh and calls write_submodule_metadata, write_containment, maybe_remove_worktree, annotate_lock, annotate_ignored_scan, and the sourced write_ignored_scan; every one of those builds its JSON with jq (the driver has require_command jq, and prove-branch-contained.sh fails with required command not found: jq). move-codex-session.test.ts runs the real CLI via process.execPath and asserts /^source Codex home is active in process(?:es)? [\d, ]+\n$/ (test file around line 433), which is produced by openProcessesInHome spawning lsof -t +D; without lsof, spawnSync sets result.error and the CLI fails with lsof failed: spawn lsof ENOENT, so that assertion and the two destination-home assertions fail. Only column appears to have no remaining user (the old jq/column summary was replaced by the TypeScript renderer).

Observed: this sandbox has Node 26.9 and git but no jq. Running node --test --test-name-pattern="tracking metadata is required" skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs fails with audit-checkouts.sh: line 142: jq: command not found (exit 127 from bash), which is exactly the failure mode the deleted step guarded against.

Proof gap: I cannot inspect the Forgejo runner image, so whether ubuntu-latest there ships jq and lsof is inferred from the deleted step's own comment; if the image was changed to include them, the deletion is harmless and the claim is refuted. If it was not, the node:test check fails on every push and PR. Safe correction: restore the conditional install for jq and lsof (dropping column, which no selected test needs), or gate the tests to skip when the tool is absent.

claim 01M37T47VRTF37D1B7YJDNB3J3 of review 01M37ANQNHYDFD5YK60V7FTRER

<!-- review:claim:01M37T47VRTF37D1B7YJDNB3J3 --> **medium** — node-test.yml drops the jq/lsof install step while the selected test files still shell out to jq and lsof, so the CI job fails on the minimal runner image the removed step existed for lens `general-bug` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the diff of `.forgejo/workflows/node-test.yml`, the four selected test files it now runs, `audit-checkouts.sh`, `prove-branch-contained.sh`, and `move-codex-session.ts`. > > What changed: the workflow deleted the `Ensure baseline shell tooling` step, whose comment read "The Forge maps ubuntu-latest to a minimal Node image, so tools GitHub's runner preinstalls are absent and a test shelling out to one dies here", and which installed `jq`, `lsof`, and `bsdextrautils` (`column`) when missing. No replacement step was added and the `Run tests` step still invokes the same test files plus the new renderer test. > > What still needs those tools: `audit-checkouts.test.mjs` sources `audit-checkouts.sh` and calls `write_submodule_metadata`, `write_containment`, `maybe_remove_worktree`, `annotate_lock`, `annotate_ignored_scan`, and the sourced `write_ignored_scan`; every one of those builds its JSON with `jq` (the driver has `require_command jq`, and `prove-branch-contained.sh` fails with `required command not found: jq`). `move-codex-session.test.ts` runs the real CLI via `process.execPath` and asserts `/^source Codex home is active in process(?:es)? [\d, ]+\n$/` (test file around line 433), which is produced by `openProcessesInHome` spawning `lsof -t +D`; without `lsof`, `spawnSync` sets `result.error` and the CLI fails with `lsof failed: spawn lsof ENOENT`, so that assertion and the two destination-home assertions fail. Only `column` appears to have no remaining user (the old jq/column summary was replaced by the TypeScript renderer). > > Observed: this sandbox has Node 26.9 and git but no `jq`. Running `node --test --test-name-pattern="tracking metadata is required" skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs` fails with `audit-checkouts.sh: line 142: jq: command not found` (exit 127 from bash), which is exactly the failure mode the deleted step guarded against. > > Proof gap: I cannot inspect the Forgejo runner image, so whether `ubuntu-latest` there ships `jq` and `lsof` is inferred from the deleted step's own comment; if the image was changed to include them, the deletion is harmless and the claim is refuted. If it was not, the `node:test` check fails on every push and PR. Safe correction: restore the conditional install for `jq` and `lsof` (dropping `column`, which no selected test needs), or gate the tests to skip when the tool is absent. claim `01M37T47VRTF37D1B7YJDNB3J3` of review `01M37ANQNHYDFD5YK60V7FTRER`
Author
Owner

Same evidence as the earlier jq finding on this file, now covering lsof too: this head's Node tests / node:test run (#1400, 45s) ran all four selected test files on the Forge runner without the install step and passed, including move-codex-session.test.ts with the lsof-backed assertions you name. The runner image ships jq and lsof. j4k-align dropped the bootstrap deliberately (af91298, #233) and renders this workflow, so a step re-added here is drift.

<!-- gh-feedback:reply-to:86687 --> Same evidence as the earlier jq finding on this file, now covering lsof too: this head's `Node tests / node:test` run (#1400, 45s) ran all four selected test files on the Forge runner without the install step and passed, including `move-codex-session.test.ts` with the lsof-backed assertions you name. The runner image ships jq and lsof. j4k-align dropped the bootstrap deliberately (af91298, #233) and renders this workflow, so a step re-added here is drift.

superseded by review 01M38YP09SA7WCW3117P8RYJ0P for head 7e38d16a8d171f2de20933ef4be013e160d245b5

<!-- review:superseded:01M38YP09SA7WCW3117P8RYJ0P --> superseded by review `01M38YP09SA7WCW3117P8RYJ0P` for head `7e38d16a8d171f2de20933ef4be013e160d245b5`
@ -88,4 +76,2 @@
# signal handler, so a kill here writes nothing to the PR — the durable
# fix is an internal deadline in the wrapper, not a larger number here.
# Re-derive when the pin moves.
# Limit runner occupancy even if retries or reconciliation are unfinished.
timeout-minutes: 130

low — The replacement comment on timeout-minutes: 130 no longer says where the number comes from or when it must be re-derived
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the review job in .forgejo/workflows/review.yml and the diff hunk that replaced its fifteen-line timeout comment.

What the subject says: the new comment reads "Limit runner occupancy even if retries or reconciliation are unfinished." That sentence explains what any timeout-minutes does; it fails the writing standard's no-op test because an agent already knows that. The number 130 itself is now unexplained. The deleted text was journal-like and too long, but it carried three facts that the standard says to keep because they let a future editor adapt the rule: the value is derived from the pinned review wrapper's worst-case wait (roughly 100 minutes against the review service), the extra 30 minutes exists because Forge calls carry no timeout, and the wrapper installs no signal handler, so a kill at this limit writes nothing to the PR. The old comment ended with the adaptation instruction "Re-derive when the pin moves".

What goes wrong: someone bumping the wrapper pin or trimming the job's budget has no way to tell from the file that 130 is a computed ceiling with a silent failure mode below it, so lowering it looks harmless. The template header says direct edits are drift and the template should be edited; the same gap exists wherever this text is rendered from.

Proposed correction: keep one or two sentences of durable causal fact in place of the generic line, for example: "130 min covers the pinned wrapper's worst case (~100 min of stage deadlines, transport-timeout overruns, and laddered service retries) plus ~30 min for Forge calls, which have no timeout. A kill here writes nothing to the PR, so re-derive this when the wrapper pin moves." This preserves the reason to adapt the value without restoring the deleted arithmetic.

Proof gap: I cannot see the wrapper or the template, so the retained figures are taken from the deleted comment, not re-verified.

claim 01M37T2432J0XH89FYBNASYEZN of review 01M37ANQNHYDFD5YK60V7FTRER

<!-- review:claim:01M37T2432J0XH89FYBNASYEZN --> **low** — The replacement comment on timeout-minutes: 130 no longer says where the number comes from or when it must be re-derived lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the `review` job in .forgejo/workflows/review.yml and the diff hunk that replaced its fifteen-line timeout comment. > > What the subject says: the new comment reads "Limit runner occupancy even if retries or reconciliation are unfinished." That sentence explains what any `timeout-minutes` does; it fails the writing standard's no-op test because an agent already knows that. The number 130 itself is now unexplained. The deleted text was journal-like and too long, but it carried three facts that the standard says to keep because they let a future editor adapt the rule: the value is derived from the pinned review wrapper's worst-case wait (roughly 100 minutes against the review service), the extra 30 minutes exists because Forge calls carry no timeout, and the wrapper installs no signal handler, so a kill at this limit writes nothing to the PR. The old comment ended with the adaptation instruction "Re-derive when the pin moves". > > What goes wrong: someone bumping the wrapper pin or trimming the job's budget has no way to tell from the file that 130 is a computed ceiling with a silent failure mode below it, so lowering it looks harmless. The template header says direct edits are drift and the template should be edited; the same gap exists wherever this text is rendered from. > > Proposed correction: keep one or two sentences of durable causal fact in place of the generic line, for example: "130 min covers the pinned wrapper's worst case (~100 min of stage deadlines, transport-timeout overruns, and laddered service retries) plus ~30 min for Forge calls, which have no timeout. A kill here writes nothing to the PR, so re-derive this when the wrapper pin moves." This preserves the reason to adapt the value without restoring the deleted arithmetic. > > Proof gap: I cannot see the wrapper or the template, so the retained figures are taken from the deleted comment, not re-verified. claim `01M37T2432J0XH89FYBNASYEZN` of review `01M37ANQNHYDFD5YK60V7FTRER`
Author
Owner

Same as the earlier timeout-comment finding: review.yml is rendered from j4k-align's templates/workflows/review-forgejo.yml.hbs, whose current text (align #270, 4ffe59e) is exactly this line, and the header marks direct edits as drift. The PR diff is the regeneration; the derivation you want back is a change to the align template.

<!-- gh-feedback:reply-to:86689 --> Same as the earlier timeout-comment finding: `review.yml` is rendered from j4k-align's `templates/workflows/review-forgejo.yml.hbs`, whose current text (align #270, 4ffe59e) is exactly this line, and the header marks direct edits as drift. The PR diff is the regeneration; the derivation you want back is a change to the align template.

superseded by review 01M38YP09SA7WCW3117P8RYJ0P for head 7e38d16a8d171f2de20933ef4be013e160d245b5

<!-- review:superseded:01M38YP09SA7WCW3117P8RYJ0P --> superseded by review `01M38YP09SA7WCW3117P8RYJ0P` for head `7e38d16a8d171f2de20933ef4be013e160d245b5`
@ -11,2 +10,3 @@
## Treat user work as untouchable
Use the requested directory, or the current directory when none is given. Discover below that root without following symlinks; keep checkout inspection and file cleanup scoped to it. Shared repository maintenance is broader: registered worktrees outside the root may appear in the report, their metadata is read, and fetched mode prunes stale registrations repository-wide, including registrations pointing outside the root. Do not treat those worktrees as audited. If outside-root registration changes are forbidden, use `--no-fetch --no-remove` or scoped local queries. If even outside-root metadata reads are forbidden, use scoped local queries; the driver cannot enforce that boundary.
Uncommitted files, commits no other ref holds, stashes, and ignored files the primary checkout does not also hold (apart from the machine-local footprints named under the modes) are user work. Discard, delete, or drop them only on the user's instruction for that item or class, after the proof the matching reference requires. Age, a vanished upstream branch, or divergence from the default branch never prove work disposable. Leave blocked content where it is: moving it into an archive, backup, or trash folder to clear a gate is deletion by another name. Never create branches or refs to "preserve" a checkout, and never edit `.gitmodules`, selectors, or Gitlinks under a `third-party/` path.

medium — "Treat user work as untouchable" classes regenerable ignored output as user work, contradicting the default mode that deletes it without confirmation
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the two opening sections of skills/audit-git-checkouts/SKILL.md ("Treat user work as untouchable" and "Choose the mode"), references/removal-gates.md ("Ignored files"), and is_regenerable_ignored_path in scripts/inspect-ignored-content.sh.

What the subject says: the anchored sentence defines user work as, among other things, "ignored files the primary checkout does not also hold (apart from the machine-local footprints named under the modes)", and the next sentence forbids deleting user work without the user's instruction. Two paragraphs later, "Choose the mode" states the opposite for a large class of exactly those files: "The default mode's updates and removals need no further confirmation. ... Removal also deletes regenerable ignored output (node_modules/, dist/, .venv/, …) and two machine-local footprints, .vscode/ and .ansible/vault_pass.txt, without comparing them to the primary checkout." removal-gates.md confirms that regenerable paths never block removal regardless of whether the primary holds them, and the scanner's exemption list includes node_modules, dist, build, out, .next, .turbo, coverage, target, .venv, .cache, .pnpm-store, .ruff_cache, and more.

What goes wrong: the exception in the definition is too narrow. A worktree-only dist/ or .venv/ is "user work" by the first section, so a literal reader either refuses the default mode, adds a confirmation the second section says is unnecessary, or reports the driver's own default removal as a policy violation. The writing standard asks for one term for one concept and for the decisive constraint to be stated once without conflict ("One Idea, One Place", "Use Precise Language"). The pointer "named under the modes" is also vague: the footprints are named in the prose after the mode table, not in the table, and there is no heading called "modes".

Proposed correction: widen the exception so the definition matches the policy and point at the heading by name, for example: "... and ignored files the primary checkout does not also hold, apart from regenerable output and the two machine-local footprints listed under "Choose the mode", are user work." This preserves the protection for uncommitted, unique-commit, stash, and precious ignored content while no longer contradicting the default removal behaviour.

What would refute it: a reading under which regenerable output the primary lacks is not "ignored files the primary checkout does not also hold"; I found none in the file.

claim 01M37T1PFKXAMGAV09GF3C7N9P of review 01M37ANQNHYDFD5YK60V7FTRER

<!-- review:claim:01M37T1PFKXAMGAV09GF3C7N9P --> **medium** — "Treat user work as untouchable" classes regenerable ignored output as user work, contradicting the default mode that deletes it without confirmation lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the two opening sections of skills/audit-git-checkouts/SKILL.md ("Treat user work as untouchable" and "Choose the mode"), references/removal-gates.md ("Ignored files"), and `is_regenerable_ignored_path` in scripts/inspect-ignored-content.sh. > > What the subject says: the anchored sentence defines user work as, among other things, "ignored files the primary checkout does not also hold (apart from the machine-local footprints named under the modes)", and the next sentence forbids deleting user work without the user's instruction. Two paragraphs later, "Choose the mode" states the opposite for a large class of exactly those files: "The default mode's updates and removals need no further confirmation. ... Removal also deletes regenerable ignored output (`node_modules/`, `dist/`, `.venv/`, …) and two machine-local footprints, `.vscode/` and `.ansible/vault_pass.txt`, without comparing them to the primary checkout." removal-gates.md confirms that regenerable paths never block removal regardless of whether the primary holds them, and the scanner's exemption list includes node_modules, dist, build, out, .next, .turbo, coverage, target, .venv, .cache, .pnpm-store, .ruff_cache, and more. > > What goes wrong: the exception in the definition is too narrow. A worktree-only `dist/` or `.venv/` is "user work" by the first section, so a literal reader either refuses the default mode, adds a confirmation the second section says is unnecessary, or reports the driver's own default removal as a policy violation. The writing standard asks for one term for one concept and for the decisive constraint to be stated once without conflict ("One Idea, One Place", "Use Precise Language"). The pointer "named under the modes" is also vague: the footprints are named in the prose after the mode table, not in the table, and there is no heading called "modes". > > Proposed correction: widen the exception so the definition matches the policy and point at the heading by name, for example: "... and ignored files the primary checkout does not also hold, apart from regenerable output and the two machine-local footprints listed under \"Choose the mode\", are user work." This preserves the protection for uncommitted, unique-commit, stash, and precious ignored content while no longer contradicting the default removal behaviour. > > What would refute it: a reading under which regenerable output the primary lacks is not "ignored files the primary checkout does not also hold"; I found none in the file. claim `01M37T1PFKXAMGAV09GF3C7N9P` of review `01M37ANQNHYDFD5YK60V7FTRER`
Author
Owner

Fixed in 7e38d16: the user-work definition now excepts regenerable output as well as the two machine-local footprints, and points at the "Choose the mode" heading by name.

<!-- gh-feedback:reply-to:86688 --> Fixed in 7e38d16: the user-work definition now excepts regenerable output as well as the two machine-local footprints, and points at the "Choose the mode" heading by name.
jercik marked this conversation as resolved
@ -73,3 +45,1 @@
has no partial-scan bypass; narrowing the audit root does not narrow a registered
worktree's scan. Ask the user how to handle the named content before changing
it, then rerun the complete gate. A partial manual inspection is not a passed gate.
Call a worktree safe to remove only when the driver removed it or you have freshly established every gate in [references/removal-gates.md](references/removal-gates.md). A cached or `--no-remove` run proves none of them; its Notes column lists the locks and primary-only ignored files it did observe.

low — SKILL.md points at a "Notes column" for observed locks and precious files, but the renderer puts them inside the "Kept because" cell for merged worktrees
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the "Report, then work the decisions" section of skills/audit-git-checkouts/SKILL.md and formatAuditReport, keptReason, and formatNotes in scripts/render-audit-report.ts.

What the subject does: the renderer emits a "Notes" column only in the "Possibly abandoned (oldest first)" and "Active work" tables. On a --no-remove or cached run, a worktree the driver proved merged is routed by keptReason into "Needs your decision: kept worktrees", whose columns are Worktree, Branch, and "Kept because"; the lock and primary-only ignored-file notes are appended there in parentheses, e.g. "merged; removal was disabled for this run (locked 3d ago: wip; ignored files only here: .env)". Those are precisely the worktrees an agent consults this sentence about, since it is deciding whether they are safe to remove.

What goes wrong: an agent that takes the sentence literally looks for a Notes column in the kept-worktrees table, finds none, and may conclude the run observed no locks or precious files. The writing standard asks that claims be verifiable and that one term name one concept; "Notes column" here names a location that does not exist for the rows in question.

Proposed correction: describe the information rather than a column, e.g. "A cached or --no-remove run proves none of them; the report still shows the locks and primary-only ignored files it observed, in the Notes column or in the kept worktree's "Kept because" cell." Alternatively, have the renderer emit a Notes column in the kept table so the sentence becomes true as written.

This is a static reading of the renderer; I did not run it against a --no-remove report.

claim 01M37T2ANRD07SM63ZMZMDD7KC of review 01M37ANQNHYDFD5YK60V7FTRER

<!-- review:claim:01M37T2ANRD07SM63ZMZMDD7KC --> **low** — SKILL.md points at a "Notes column" for observed locks and precious files, but the renderer puts them inside the "Kept because" cell for merged worktrees lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the "Report, then work the decisions" section of skills/audit-git-checkouts/SKILL.md and `formatAuditReport`, `keptReason`, and `formatNotes` in scripts/render-audit-report.ts. > > What the subject does: the renderer emits a "Notes" column only in the "Possibly abandoned (oldest first)" and "Active work" tables. On a `--no-remove` or cached run, a worktree the driver proved merged is routed by `keptReason` into "Needs your decision: kept worktrees", whose columns are Worktree, Branch, and "Kept because"; the lock and primary-only ignored-file notes are appended there in parentheses, e.g. "merged; removal was disabled for this run (locked 3d ago: wip; ignored files only here: .env)". Those are precisely the worktrees an agent consults this sentence about, since it is deciding whether they are safe to remove. > > What goes wrong: an agent that takes the sentence literally looks for a Notes column in the kept-worktrees table, finds none, and may conclude the run observed no locks or precious files. The writing standard asks that claims be verifiable and that one term name one concept; "Notes column" here names a location that does not exist for the rows in question. > > Proposed correction: describe the information rather than a column, e.g. "A cached or `--no-remove` run proves none of them; the report still shows the locks and primary-only ignored files it observed, in the Notes column or in the kept worktree's \"Kept because\" cell." Alternatively, have the renderer emit a Notes column in the kept table so the sentence becomes true as written. > > This is a static reading of the renderer; I did not run it against a `--no-remove` report. claim `01M37T2ANRD07SM63ZMZMDD7KC` of review `01M37ANQNHYDFD5YK60V7FTRER`
Author
Owner

Fixed in 7e38d16. Reproduced on a rendered --no-remove report: the lock and .env.local notes sit in the kept table's "Kept because" cell. The sentence now names that cell and the Notes column.

<!-- gh-feedback:reply-to:86690 --> Fixed in 7e38d16. Reproduced on a rendered `--no-remove` report: the lock and `.env.local` notes sit in the kept table's "Kept because" cell. The sentence now names that cell and the Notes column.
jercik marked this conversation as resolved
@ -315,0 +320,4 @@
[ "$files" -le 1000 ] || return 1
file_epoch=$(stat_epoch "$file_path") || return 1
[ "$file_epoch" -le "$newest" ] || newest=$file_epoch
done < <(find "$directory_path" -type f -print0 2>/dev/null)

low — newest_directory_epoch ignores find's exit status and stderr, so a partially unreadable changed directory yields a stale "newest" mtime with error: null instead of an unknown timestamp
lens general-bug · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the new newest_directory_epoch helper and its caller annotate_activity_review in audit-checkouts.sh, the sibling helper ignored_content_list_children in inspect-ignored-content.sh, and the activity tests in audit-checkouts.test.mjs (a changed directory is timed by the newest file inside it, a failed timestamp query with only old observations remains unknown).

What the subject does: newest_directory_epoch reads find "$directory_path" -type f -print0 2>/dev/null through a process substitution and never checks find's exit status. GNU and BSD find keep printing the entries they can reach and exit non-zero when a subdirectory is unreadable or vanishes mid-walk; that failure is discarded here (2>/dev/null, and a process substitution's status is not observable to the loop). The function then prints the newest mtime among whatever find managed to list, and the caller treats that as a successful timestamp: annotate_activity_review only writes Changed-path activity timestamp unavailable when the helper returns non-zero, which this path never does for a partial walk. By contrast the same change's ignored_content_list_children captures rc=$? from its find and fails closed with ignored content enumeration failed (exit N).

What goes wrong: for an untracked or modified directory with an unreadable or racing subtree, the newest file may be exactly the one find could not stat, so latest_path_epoch is older than reality. The worktree then gets activityReview.outcome: "possibly-abandoned" with error: null, i.e. the report presents complete evidence for an inference that was computed on a partial listing. Before this change, a changed directory always produced the Changed directory needs activity inspection diagnostic in activityReview.error, so the gap was visible; now it is silent. The effect is confined to the review queue (age never authorizes deletion per SKILL.md), which is why this is low.

Static reasoning only: I did not construct an unreadable subdirectory fixture in this sandbox (the tests run as root here, where permission errors do not reproduce). A fixture with a chmod 000 subdirectory under an untracked directory whose newest file sits inside it would establish the claim: the expected behaviour is a timestamp unavailable diagnostic, the actual is a silent older epoch. Safe correction: write find output to a scratch file and check its exit status (as ignored_content_list_children does), returning 1 on any failure so the caller records the diagnostic.

claim 01M37T5XJ4EKEP1E5BDBCB2SN7 of review 01M37ANQNHYDFD5YK60V7FTRER

<!-- review:claim:01M37T5XJ4EKEP1E5BDBCB2SN7 --> **low** — newest_directory_epoch ignores find's exit status and stderr, so a partially unreadable changed directory yields a stale "newest" mtime with error: null instead of an unknown timestamp lens `general-bug` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the new `newest_directory_epoch` helper and its caller `annotate_activity_review` in `audit-checkouts.sh`, the sibling helper `ignored_content_list_children` in `inspect-ignored-content.sh`, and the activity tests in `audit-checkouts.test.mjs` (`a changed directory is timed by the newest file inside it`, `a failed timestamp query with only old observations remains unknown`). > > What the subject does: `newest_directory_epoch` reads `find "$directory_path" -type f -print0 2>/dev/null` through a process substitution and never checks `find`'s exit status. GNU and BSD `find` keep printing the entries they can reach and exit non-zero when a subdirectory is unreadable or vanishes mid-walk; that failure is discarded here (`2>/dev/null`, and a process substitution's status is not observable to the loop). The function then prints the newest mtime among whatever `find` managed to list, and the caller treats that as a successful timestamp: `annotate_activity_review` only writes `Changed-path activity timestamp unavailable` when the helper returns non-zero, which this path never does for a partial walk. By contrast the same change's `ignored_content_list_children` captures `rc=$?` from its `find` and fails closed with `ignored content enumeration failed (exit N)`. > > What goes wrong: for an untracked or modified directory with an unreadable or racing subtree, the newest file may be exactly the one `find` could not stat, so `latest_path_epoch` is older than reality. The worktree then gets `activityReview.outcome: "possibly-abandoned"` with `error: null`, i.e. the report presents complete evidence for an inference that was computed on a partial listing. Before this change, a changed directory always produced the `Changed directory needs activity inspection` diagnostic in `activityReview.error`, so the gap was visible; now it is silent. The effect is confined to the review queue (age never authorizes deletion per SKILL.md), which is why this is low. > > Static reasoning only: I did not construct an unreadable subdirectory fixture in this sandbox (the tests run as root here, where permission errors do not reproduce). A fixture with a `chmod 000` subdirectory under an untracked directory whose newest file sits inside it would establish the claim: the expected behaviour is a `timestamp unavailable` diagnostic, the actual is a silent older epoch. Safe correction: write `find` output to a scratch file and check its exit status (as `ignored_content_list_children` does), returning 1 on any failure so the caller records the diagnostic. claim `01M37T5XJ4EKEP1E5BDBCB2SN7` of review `01M37ANQNHYDFD5YK60V7FTRER`
Author
Owner

Fixed in 814a1c2. Reproduced first: with a chmod 000 subdirectory, find exited 1 and the old function returned 0 with the epoch of the one file it could reach. The listing now goes through a scratch file, a non-zero find fails the lookup, and the caller records the timestamp as unavailable; a regression test covers it (skipped as root, where the mode has no effect).

<!-- gh-feedback:reply-to:86691 --> Fixed in 814a1c2. Reproduced first: with a `chmod 000` subdirectory, `find` exited 1 and the old function returned 0 with the epoch of the one file it could reach. The listing now goes through a scratch file, a non-zero `find` fails the lookup, and the caller records the timestamp as unavailable; a regression test covers it (skipped as root, where the mode has no effect).
jercik marked this conversation as resolved
@ -0,0 +361,4 @@
}),
);
assert.ok(markdown.includes("- 1 checkout is current by cached refs."));
assert.ok(!markdown.includes("- Updated"));

low — Negative "- Updated" assertion in the regenerated-guidance test can never fail on a cached run
lens test-trimming · arm default · tally 1 valid / 0 invalid / 0 uncertain

Test examined: "a cached run treats regenerated guidance as clean" in render-audit-report.test.ts. It builds a report with fetched: false, removalEnabled: false and a single primary worktree with deferredOnly: true, one unstaged file, a non-fresh comparison, and classification "manual-review", then asserts two things:

assert.ok(markdown.includes("- 1 checkout is current by cached refs."));
assert.ok(!markdown.includes("- Updated"));

Implementation examined: formatAuditReport in render-audit-report.ts. The only place the text "- Updated" is produced is the summary line, and it is emitted only when the run was fetched:

...(report.fetched ? [`- Updated ${count(updated.length, "default checkout")}; dependency files changed in ...`] : []),

The "## Updated" section heading does not contain "- Updated", and the updated table rows begin with the checkout path, not that text. So on any report with fetched: false the string "- Updated" is absent regardless of deferredOnly, the dirty count, or anything else about the worktree. The second assertion is disconnected from the deferred-guidance behavior the test names; it would stay green if deferredOnly were ignored entirely.

Observed evidence (executed, not just traced):

  1. A probe calling formatAuditReport on an empty cached report (fetched: false, no repositories) printed md.includes("- Updated") === false, i.e. the assertion holds with no worktree at all.
  2. Mutating pendingCount to return dirtyCount(worktree.status); (dropping the deferredOnly branch) and running node --test render-audit-report.test.ts failed this test only on the first assertion ("- 1 checkout is current by cached refs." actual: false). The second assertion never contributes to the failure.

What goes wrong: the assertion reads as a check that regenerated guidance does not get reported as an update, but it cannot detect any regression, so it lends false confidence that the deferred-only path is covered from the "Updated" angle. The genuine protection in this test is the first assertion (a deferred-only dirty checkout is counted as current on a cached run), which the mutation shows is effective.

Recommended correction: keep the test and its first assertion. Either delete the vacuous second assertion, or replace it with one that can fail for this input, for example asserting the decision row is absent: assert.ok(!markdown.includes("| app | main |")) or assert.ok(!markdown.includes("## Needs your decision: not current")), both of which appear when deferredOnly is ignored (the mutated run renders "| app | main | +0/-0 | cached: 0 commits behind, 0 ahead, 1 uncommitted file |"). This preserves the deferred-guidance regression check and removes the assertion that can never fail.

claim 01M37T060S31G02H4HHS07FZ3V of review 01M37ANQNHYDFD5YK60V7FTRER

<!-- review:claim:01M37T060S31G02H4HHS07FZ3V --> **low** — Negative "- Updated" assertion in the regenerated-guidance test can never fail on a cached run lens `test-trimming` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > Test examined: "a cached run treats regenerated guidance as clean" in render-audit-report.test.ts. It builds a report with `fetched: false, removalEnabled: false` and a single primary worktree with `deferredOnly: true`, one unstaged file, a non-fresh comparison, and classification "manual-review", then asserts two things: > > assert.ok(markdown.includes("- 1 checkout is current by cached refs.")); > assert.ok(!markdown.includes("- Updated")); > > Implementation examined: formatAuditReport in render-audit-report.ts. The only place the text "- Updated" is produced is the summary line, and it is emitted only when the run was fetched: > > ...(report.fetched ? [`- Updated ${count(updated.length, "default checkout")}; dependency files changed in ...`] : []), > > The "## Updated" section heading does not contain "- Updated", and the updated table rows begin with the checkout path, not that text. So on any report with `fetched: false` the string "- Updated" is absent regardless of `deferredOnly`, the dirty count, or anything else about the worktree. The second assertion is disconnected from the deferred-guidance behavior the test names; it would stay green if `deferredOnly` were ignored entirely. > > Observed evidence (executed, not just traced): > 1. A probe calling formatAuditReport on an empty cached report (`fetched: false`, no repositories) printed `md.includes("- Updated") === false`, i.e. the assertion holds with no worktree at all. > 2. Mutating pendingCount to `return dirtyCount(worktree.status);` (dropping the deferredOnly branch) and running `node --test render-audit-report.test.ts` failed this test only on the first assertion ("- 1 checkout is current by cached refs." actual: false). The second assertion never contributes to the failure. > > What goes wrong: the assertion reads as a check that regenerated guidance does not get reported as an update, but it cannot detect any regression, so it lends false confidence that the deferred-only path is covered from the "Updated" angle. The genuine protection in this test is the first assertion (a deferred-only dirty checkout is counted as current on a cached run), which the mutation shows is effective. > > Recommended correction: keep the test and its first assertion. Either delete the vacuous second assertion, or replace it with one that can fail for this input, for example asserting the decision row is absent: `assert.ok(!markdown.includes("| app | main |"))` or `assert.ok(!markdown.includes("## Needs your decision: not current"))`, both of which appear when deferredOnly is ignored (the mutated run renders "| app | main | +0/-0 | cached: 0 commits behind, 0 ahead, 1 uncommitted file |"). This preserves the deferred-guidance regression check and removes the assertion that can never fail. claim `01M37T060S31G02H4HHS07FZ3V` of review `01M37ANQNHYDFD5YK60V7FTRER`
Author
Owner

Fixed in 1b8eb1f: the second assertion now checks that the "Needs your decision: not current" section is absent. Confirmed live by mutating pendingCount to ignore deferredOnly, which fails the test.

<!-- gh-feedback:reply-to:86692 --> Fixed in 1b8eb1f: the second assertion now checks that the "Needs your decision: not current" section is absent. Confirmed live by mutating `pendingCount` to ignore `deferredOnly`, which fails the test.
jercik marked this conversation as resolved
@ -0,0 +105,4 @@
function dirtyCount(status: Status): number {
if (status === null) return 0;
const tree = status.workingTree;
return tree.staged + tree.unstaged + tree.untracked + tree.conflicted;

high — render-audit-report.ts sums numeric workingTree.staged/unstaged counts the driver's status record never carries, so every dirty checkout renders as "NaN uncommitted files" and dirty-tree decisions are skipped
lens general-bug · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: dirtyCount/pendingCount and every caller in render-audit-report.ts (keptReason, primaryDecision, defaultNotCurrentReason, the Active/Abandoned/Unknown rows); the driver audit-checkouts.sh, which copies repoq status --json verbatim into worktree.status; and the driver's own repoq fakes in audit-checkouts.test.mjs.

What the subject says: the renderer declares Status.workingTree as { isClean, staged: number, unstaged: number, untracked: number, conflicted: number } and computes tree.staged + tree.unstaged + tree.untracked + tree.conflicted. The driver never produces that shape. It reads the status it stores as arrays under files: write_latest_change iterates .workingTree.files[$kind][]? and annotate_activity_review reads (.status.workingTree.files // {}) | [.staged[]?, .unstaged[]?, .untracked[]?, .conflicted[]?]. Every fake repoq status in audit-checkouts.test.mjs (the printf fakes around lines 1044 and 1177, and the fixtures at 1241 and 1376) emits "workingTree":{"isClean":...,"files":{"staged":[],"unstaged":[],"untracked":[],"conflicted":[]}} with no numeric siblings. Only the renderer's own test fixtures in render-audit-report.test.ts invent staged: 0, unstaged: 2, ... numbers, which is why its 15 tests pass while disagreeing with the driver.

What goes wrong (observed): I fed formatAuditReport a schema-5 report whose status.workingTree has the driver's files shape (a small .mts harness importing the renderer directly under Node 26.9). Output contains | app-landed | landed | merged, but NaN uncommitted files | and | app-pin | (detached) | just now | +0/-0 | NaN | |. Because undefined + undefined is NaN and NaN > 0 is false, every dirty > 0 branch is dead against real reports: a dirty current-pinned-reference linked worktree is counted as current instead of reported (primaryDecision returns null); a dirty default checkout that is behind renders the fallback not current instead of behind; uncommitted changes (N files) block the fast-forward... (my repro shows | app | main | +0/-5 | not current |); the Uncommitted files column is NaN for every row. SKILL.md tells the agent to reply with the rendered summary and rows, so the user is told the wrong thing about which checkouts hold uncommitted work.

Decisive evidence / gap: I could not run repoq here (no network), so the authoritative repoq schema is inferred from how the driver and its tests consume it rather than from repoq itself; if repoq did emit both numeric counts and files arrays the claim would be refuted, but nothing in the tree reads or fakes such counts. Safe correction: derive the count from status.workingTree.files (sum of the four array lengths, or read isClean), and make the renderer test fixtures use the driver's shape.

Severity note: silent wrong output in the user-facing report rather than a crash; the markdown still renders.

claim 01M37T47F23Z67ZQG2S7J0NV8V of review 01M37ANQNHYDFD5YK60V7FTRER

<!-- review:claim:01M37T47F23Z67ZQG2S7J0NV8V --> **high** — render-audit-report.ts sums numeric workingTree.staged/unstaged counts the driver's status record never carries, so every dirty checkout renders as "NaN uncommitted files" and dirty-tree decisions are skipped lens `general-bug` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: `dirtyCount`/`pendingCount` and every caller in `render-audit-report.ts` (`keptReason`, `primaryDecision`, `defaultNotCurrentReason`, the Active/Abandoned/Unknown rows); the driver `audit-checkouts.sh`, which copies `repoq status --json` verbatim into `worktree.status`; and the driver's own repoq fakes in `audit-checkouts.test.mjs`. > > What the subject says: the renderer declares `Status.workingTree` as `{ isClean, staged: number, unstaged: number, untracked: number, conflicted: number }` and computes `tree.staged + tree.unstaged + tree.untracked + tree.conflicted`. The driver never produces that shape. It reads the status it stores as arrays under `files`: `write_latest_change` iterates `.workingTree.files[$kind][]?` and `annotate_activity_review` reads `(.status.workingTree.files // {}) | [.staged[]?, .unstaged[]?, .untracked[]?, .conflicted[]?]`. Every fake `repoq status` in `audit-checkouts.test.mjs` (the `printf` fakes around lines 1044 and 1177, and the fixtures at 1241 and 1376) emits `"workingTree":{"isClean":...,"files":{"staged":[],"unstaged":[],"untracked":[],"conflicted":[]}}` with no numeric siblings. Only the renderer's own test fixtures in `render-audit-report.test.ts` invent `staged: 0, unstaged: 2, ...` numbers, which is why its 15 tests pass while disagreeing with the driver. > > What goes wrong (observed): I fed `formatAuditReport` a schema-5 report whose `status.workingTree` has the driver's `files` shape (a small `.mts` harness importing the renderer directly under Node 26.9). Output contains `| app-landed | landed | merged, but NaN uncommitted files |` and `| app-pin | (detached) | just now | +0/-0 | NaN | |`. Because `undefined + undefined` is `NaN` and `NaN > 0` is false, every `dirty > 0` branch is dead against real reports: a dirty `current-pinned-reference` linked worktree is counted as current instead of reported (`primaryDecision` returns null); a dirty default checkout that is behind renders the fallback `not current` instead of `behind; uncommitted changes (N files) block the fast-forward...` (my repro shows `| app | main | +0/-5 | not current |`); the `Uncommitted files` column is `NaN` for every row. SKILL.md tells the agent to reply with the rendered summary and rows, so the user is told the wrong thing about which checkouts hold uncommitted work. > > Decisive evidence / gap: I could not run `repoq` here (no network), so the authoritative repoq schema is inferred from how the driver and its tests consume it rather than from repoq itself; if repoq did emit both numeric counts and `files` arrays the claim would be refuted, but nothing in the tree reads or fakes such counts. Safe correction: derive the count from `status.workingTree.files` (sum of the four array lengths, or read `isClean`), and make the renderer test fixtures use the driver's shape. > > Severity note: silent wrong output in the user-facing report rather than a crash; the markdown still renders. claim `01M37T47F23Z67ZQG2S7J0NV8V` of review `01M37ANQNHYDFD5YK60V7FTRER`
Author
Owner

Refuted by repoq itself. repoq status --json emits both the numeric counts and the files arrays: its schema (src/repoq.schemas.ts lines 207-216 at f9fd778) declares staged, unstaged, untracked, and conflicted as numbers beside files. A real driver report from the fixture carries {"isClean":false,"staged":0,"unstaged":1,"untracked":0,"conflicted":0,"files":{...}} for a dirty checkout, and the renderer prints behind; uncommitted changes (1 file) block the fast-forward, never NaN. The driver fakes omit the counts because the driver reads only files; the renderer fixtures carry the counts because that is the part it reads.

<!-- gh-feedback:reply-to:86686 --> Refuted by repoq itself. `repoq status --json` emits both the numeric counts and the `files` arrays: its schema (`src/repoq.schemas.ts` lines 207-216 at f9fd778) declares `staged`, `unstaged`, `untracked`, and `conflicted` as numbers beside `files`. A real driver report from the fixture carries `{"isClean":false,"staged":0,"unstaged":1,"untracked":0,"conflicted":0,"files":{...}}` for a dirty checkout, and the renderer prints `behind; uncommitted changes (1 file) block the fast-forward`, never NaN. The driver fakes omit the counts because the driver reads only `files`; the renderer fixtures carry the counts because that is the part it reads.

superseded by review 01M38YP09SA7WCW3117P8RYJ0P for head 7e38d16a8d171f2de20933ef4be013e160d245b5

<!-- review:superseded:01M38YP09SA7WCW3117P8RYJ0P --> superseded by review `01M38YP09SA7WCW3117P8RYJ0P` for head `7e38d16a8d171f2de20933ef4be013e160d245b5`
newest_directory_epoch read find through a process substitution and
never saw its exit status, so an unreadable subdirectory produced a
stale "newest" mtime with no diagnostic. The listing now goes through a
scratch file and a non-zero find fails the lookup, which the caller
records as an unavailable timestamp.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
"- Updated" never renders on a cached run, so that negative assertion
could not fail; asserting the "not current" section is absent does fail
when deferredOnly is ignored.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
docs(audit-git-checkouts): match the user-work definition to default removal
All checks were successful
commit-msg / commitlint (pull_request) Successful in 25s
Node tests / node:test (pull_request) Successful in 46s
Review / Review (pull_request_target) Successful in 12m9s
7e38d16a8d
Regenerable ignored output is deleted by the default mode, so the
user-work definition now excepts it alongside the machine-local
footprints and points at the "Choose the mode" section. The report note
about observed locks and primary-only ignored files names the kept
worktree's "Kept because" cell, where the renderer puts them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ -35,1 +33,3 @@
# double-post. Keying on github.ref alone would instead split a stacked pull
# group deliberately — a manually dispatched review should supersede any live
# review pass for the same pull request, and the wrapper's reconcilers list
# before they create, so two overlapping passes double-post. Keying on github.ref alone would instead split a stacked pull

low — Reflowed concurrency comment leaves one 122-character line in a block wrapped at about 78 columns
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the header comment block of .forgejo/workflows/review.yml (the lines describing the concurrency group), the diff hunk that rewrote the sentence about a manually dispatched review superseding a live pass, and the line lengths of the whole file.

What the subject says: the diff replaced three lines with three new lines but folded the tail of the old third line onto the new third line without re-wrapping, producing # before they create, so two overlapping passes double-post. Keying on github.ref alone would instead split a stacked pull (122 characters) inside a block whose every other line wraps at or under about 80 characters. The next line, # request, whose ref is its own base branch, away from that main dispatch., is the continuation of that sentence.

What goes wrong: the comment is the only place this workflow explains its concurrency design, and the change makes one sentence run past the wrap the reader's eye follows, so "Keying on github.ref alone would instead split a stacked pull / request" is visually detached from its own continuation. It also signals an unfinished edit in a comment that otherwise reads as authored.

Proposed correction: re-wrap the paragraph from "A permitted main dispatch stays..." through "...away from that main dispatch." at the block's existing width, with no wording change. The following short line # \edited` is required: a base-branch` can be rejoined in the same re-wrap.

Observed by measuring line lengths in the tree; no other line added by the diff exceeds the block's width.

claim 01M38Z42385W427EA26KNEEJS3 of review 01M38YP09SA7WCW3117P8RYJ0P

<!-- review:claim:01M38Z42385W427EA26KNEEJS3 --> **low** — Reflowed concurrency comment leaves one 122-character line in a block wrapped at about 78 columns lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the header comment block of `.forgejo/workflows/review.yml` (the lines describing the concurrency group), the diff hunk that rewrote the sentence about a manually dispatched review superseding a live pass, and the line lengths of the whole file. > > What the subject says: the diff replaced three lines with three new lines but folded the tail of the old third line onto the new third line without re-wrapping, producing `# before they create, so two overlapping passes double-post. Keying on github.ref alone would instead split a stacked pull` (122 characters) inside a block whose every other line wraps at or under about 80 characters. The next line, `# request, whose ref is its own base branch, away from that main dispatch.`, is the continuation of that sentence. > > What goes wrong: the comment is the only place this workflow explains its concurrency design, and the change makes one sentence run past the wrap the reader's eye follows, so "Keying on github.ref alone would instead split a stacked pull / request" is visually detached from its own continuation. It also signals an unfinished edit in a comment that otherwise reads as authored. > > Proposed correction: re-wrap the paragraph from "A permitted main dispatch stays..." through "...away from that main dispatch." at the block's existing width, with no wording change. The following short line `# \`edited\` is required: a base-branch` can be rejoined in the same re-wrap. > > Observed by measuring line lengths in the tree; no other line added by the diff exceeds the block's width. claim `01M38Z42385W427EA26KNEEJS3` of review `01M38YP09SA7WCW3117P8RYJ0P`
Author
Owner

Not editable here: review.yml is rendered by j4k-align, and the 122-character line is line 35 of its templates/workflows/review-forgejo.yml.hbs (that template also carries 106- and 206-character lines). The header marks direct edits as drift; a re-wrap belongs in the align template.

<!-- gh-feedback:reply-to:86723 --> Not editable here: `review.yml` is rendered by j4k-align, and the 122-character line is line 35 of its `templates/workflows/review-forgejo.yml.hbs` (that template also carries 106- and 206-character lines). The header marks direct edits as drift; a re-wrap belongs in the align template.

superseded by review 01M38ZNK9786DK6HMH6JHEC151 for head 11c8adf74a2af352a7e47067e73f589be44fd626

<!-- review:superseded:01M38ZNK9786DK6HMH6JHEC151 --> superseded by review `01M38ZNK9786DK6HMH6JHEC151` for head `11c8adf74a2af352a7e47067e73f589be44fd626`
@ -88,4 +76,2 @@
# signal handler, so a kill here writes nothing to the PR — the durable
# fix is an internal deadline in the wrapper, not a larger number here.
# Re-derive when the pin moves.
# Limit runner occupancy even if retries or reconciliation are unfinished.
timeout-minutes: 130

low — Replacement comment on timeout-minutes states what the key does and drops the only non-obvious fact, how 130 was derived
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the review job in .forgejo/workflows/review.yml, the diff that replaced the fourteen-line derivation comment with # Limit runner occupancy even if retries or reconciliation are unfinished., and the pinned review-wrapper@21cd18a8... step the number is tied to.

What the subject says: the new comment describes the generic purpose of timeout-minutes, which any reader of a workflow file already knows. The removed comment carried the facts a maintainer needs: that 130 is the pinned wrapper's worst case against the review service (about 100 minutes) plus about 30 minutes of headroom for forge calls that carry no timeout, that the wrapper installs no signal handler so a kill writes nothing to the pull request, and that the value must be re-derived when the wrapper pin moves.

Applicable guidance: the writing skill's no-op test ("would the models this content serves already behave correctly without this line? Delete it if yes") and "Give rationale only when it helps the agent adapt the rule or understand its stakes." The old comment was too long and journal-like, but its derivation is exactly the rationale that lets a maintainer adapt the number; the new line fails the no-op test while the adaptable fact is gone.

What goes wrong: a future change that bumps the review-wrapper pin has no signal that 130 is coupled to it. If the wrapper's stage budgets grow, the job is killed mid-run and, per the deleted comment, the pull request receives no review and no error, with nothing in the file pointing at the cause.

Proposed correction: keep a two-line durable comment in place of the current one, e.g. # ~100 min worst case against the review service for the pinned wrapper plus ~30 min headroom for untimed forge calls; a kill writes nothing to the PR. Re-derive when the wrapper pin moves. This preserves the stakes and the coupling in present tense without the removed walkthrough.

Proof gap: I cannot see the wrapper at the pinned commit, so I rely on the deleted comment's own numbers for the derivation; the finding does not depend on those numbers being current, only on the comment no longer recording that a derivation exists.

claim 01M38Z42JXVD9NJEXGFEYCDZY8 of review 01M38YP09SA7WCW3117P8RYJ0P

<!-- review:claim:01M38Z42JXVD9NJEXGFEYCDZY8 --> **low** — Replacement comment on timeout-minutes states what the key does and drops the only non-obvious fact, how 130 was derived lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the `review` job in `.forgejo/workflows/review.yml`, the diff that replaced the fourteen-line derivation comment with `# Limit runner occupancy even if retries or reconciliation are unfinished.`, and the pinned `review-wrapper@21cd18a8...` step the number is tied to. > > What the subject says: the new comment describes the generic purpose of `timeout-minutes`, which any reader of a workflow file already knows. The removed comment carried the facts a maintainer needs: that 130 is the pinned wrapper's worst case against the review service (about 100 minutes) plus about 30 minutes of headroom for forge calls that carry no timeout, that the wrapper installs no signal handler so a kill writes nothing to the pull request, and that the value must be re-derived when the wrapper pin moves. > > Applicable guidance: the writing skill's no-op test ("would the models this content serves already behave correctly without this line? Delete it if yes") and "Give rationale only when it helps the agent adapt the rule or understand its stakes." The old comment was too long and journal-like, but its derivation is exactly the rationale that lets a maintainer adapt the number; the new line fails the no-op test while the adaptable fact is gone. > > What goes wrong: a future change that bumps the `review-wrapper` pin has no signal that 130 is coupled to it. If the wrapper's stage budgets grow, the job is killed mid-run and, per the deleted comment, the pull request receives no review and no error, with nothing in the file pointing at the cause. > > Proposed correction: keep a two-line durable comment in place of the current one, e.g. `# ~100 min worst case against the review service for the pinned wrapper plus ~30 min headroom for untimed forge calls; a kill writes nothing to the PR. Re-derive when the wrapper pin moves.` This preserves the stakes and the coupling in present tense without the removed walkthrough. > > Proof gap: I cannot see the wrapper at the pinned commit, so I rely on the deleted comment's own numbers for the derivation; the finding does not depend on those numbers being current, only on the comment no longer recording that a derivation exists. claim `01M38Z42JXVD9NJEXGFEYCDZY8` of review `01M38YP09SA7WCW3117P8RYJ0P`
Author
Owner

Same as the two earlier timeout-comment findings: review.yml is rendered from j4k-align's templates/workflows/review-forgejo.yml.hbs, whose current text (align #270, 4ffe59e) is exactly this line. The diff in this PR is the regeneration; restoring the derivation is a change to the align template.

<!-- gh-feedback:reply-to:86724 --> Same as the two earlier timeout-comment findings: `review.yml` is rendered from j4k-align's `templates/workflows/review-forgejo.yml.hbs`, whose current text (align #270, 4ffe59e) is exactly this line. The diff in this PR is the regeneration; restoring the derivation is a change to the align template.

superseded by review 01M38ZNK9786DK6HMH6JHEC151 for head 11c8adf74a2af352a7e47067e73f589be44fd626

<!-- review:superseded:01M38ZNK9786DK6HMH6JHEC151 --> superseded by review `01M38ZNK9786DK6HMH6JHEC151` for head `11c8adf74a2af352a7e47067e73f589be44fd626`
@ -23,3 +22,1 @@
Gated worktree removal is the default; no second permission request is needed. Local-branch deletion, remote-branch deletion, stash drops, and discarding dirty files require cleanup authorization covering that class. A request to clean local branches does not authorize remote deletions or stash drops. Report out-of-scope findings without deleting them.
Treat dirty files, local commits, and stashes as user work. Age, default-branch divergence, and disappearance from a remote do not prove disposability. Delete only established disposable content, with one exception: the two machine-local footprints named in [references/removal-gates.md](references/removal-gates.md), which a removal-enabled run deletes without comparing the primary checkout. Leave unresolved files in their worktrees, never move them into archives or backups to bypass a gate. Git object pruning is report-only (`git prune --dry-run`), not an agent cleanup action.
The default mode's updates and removals need no further confirmation. Removal proves containment against `origin/<default>` only: for a root holding a fork whose work integrates elsewhere, audit with `--no-remove`. The fetch prunes every `refs/remotes/origin/*` ref that no server branch supplies; for a repository with a custom `origin` fetch refspec, audit with `--no-fetch --no-remove` (see [references/checkout-updates.md](references/checkout-updates.md)). Removal also deletes regenerable ignored output (`node_modules/`, `dist/`, `.venv/`, …) and two machine-local footprints, `.vscode/` and `.ansible/vault_pass.txt`, without comparing them to the primary checkout. If the user wants those kept, use `--no-remove`. Stale-registration pruning is repository-wide: it can drop registrations of worktrees outside the root whose directories are gone.

low — The mode-selection caveats are five separate trigger-to-flag rules buried in one paragraph after the mode table
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the ## Choose the mode section of skills/audit-git-checkouts/SKILL.md: the three-row table followed by a single six-sentence paragraph, and the --help text of scripts/audit-checkouts.sh that the table summarizes.

What the subject says: after the table, one paragraph carries (1) default mode needs no further confirmation; (2) a fork whose work integrates elsewhere -> --no-remove; (3) a custom origin fetch refspec -> --no-fetch --no-remove, with a pointer to checkout-updates.md; (4) removal also deletes regenerable output and the two machine-local footprints, and if the user wants those kept -> --no-remove; (5) stale-registration pruning is repository-wide and can drop out-of-root registrations. Items 2 to 4 are each a condition that overrides the table's first row and names the flag to use instead; item 5 is a side effect with no flag stated (the table's third row is the only mode that avoids it).

Applicable guidance: "Order sections by the decisions the reader makes. Make the decisive constraint prominent" and "Use plain prose by default, bullets for flat choices." This is exactly a flat set of choices: the agent has just picked a table row and needs to check whether any exception moves it to a stricter row.

What goes wrong: the exceptions are where the irreversible actions live (removal deletes worktrees and ignored files; the fetch prunes remote-tracking refs). An agent scanning for "does my root need a stricter mode" has to read the whole paragraph to find the three conditions, and the refspec condition is the only one whose flag is given with a pointer, so the three are not visually parallel. The fork case in particular is easy to miss because it is the second half of a sentence that starts by describing what removal proves.

Proposed correction: keep the first sentence as prose, then list the exceptions as bullets, each leading with the trigger and ending with the flags: "- A repository whose work integrates somewhere other than origin/<default> (a fork with an upstream): --no-remove. Removal proves containment against origin/<default> only." / "- A repository with a custom origin fetch refspec: --no-fetch --no-remove; the fetch prunes every refs/remotes/origin/* ref no server branch supplies (see references/checkout-updates.md)." / "- Regenerable ignored output (node_modules/, dist/, .venv/, ...) or the machine-local .vscode/ and .ansible/vault_pass.txt that the user wants kept: --no-remove; removal deletes them without comparing the primary checkout." / "- Worktree registrations outside the root whose directories are gone: any fetched mode prunes them; only --no-fetch --no-remove leaves them." Same facts, same flags, one home per rule, and each trigger is scannable.

This is a structure finding; every statement in the paragraph checks out against the driver's help text and the reference files.

claim 01M38Z43ZXYXZRZPMZD44Q3NWT of review 01M38YP09SA7WCW3117P8RYJ0P

<!-- review:claim:01M38Z43ZXYXZRZPMZD44Q3NWT --> **low** — The mode-selection caveats are five separate trigger-to-flag rules buried in one paragraph after the mode table lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the `## Choose the mode` section of `skills/audit-git-checkouts/SKILL.md`: the three-row table followed by a single six-sentence paragraph, and the `--help` text of `scripts/audit-checkouts.sh` that the table summarizes. > > What the subject says: after the table, one paragraph carries (1) default mode needs no further confirmation; (2) a fork whose work integrates elsewhere -> `--no-remove`; (3) a custom `origin` fetch refspec -> `--no-fetch --no-remove`, with a pointer to checkout-updates.md; (4) removal also deletes regenerable output and the two machine-local footprints, and if the user wants those kept -> `--no-remove`; (5) stale-registration pruning is repository-wide and can drop out-of-root registrations. Items 2 to 4 are each a condition that overrides the table's first row and names the flag to use instead; item 5 is a side effect with no flag stated (the table's third row is the only mode that avoids it). > > Applicable guidance: "Order sections by the decisions the reader makes. Make the decisive constraint prominent" and "Use plain prose by default, bullets for flat choices." This is exactly a flat set of choices: the agent has just picked a table row and needs to check whether any exception moves it to a stricter row. > > What goes wrong: the exceptions are where the irreversible actions live (removal deletes worktrees and ignored files; the fetch prunes remote-tracking refs). An agent scanning for "does my root need a stricter mode" has to read the whole paragraph to find the three conditions, and the refspec condition is the only one whose flag is given with a pointer, so the three are not visually parallel. The fork case in particular is easy to miss because it is the second half of a sentence that starts by describing what removal proves. > > Proposed correction: keep the first sentence as prose, then list the exceptions as bullets, each leading with the trigger and ending with the flags: "- A repository whose work integrates somewhere other than `origin/<default>` (a fork with an `upstream`): `--no-remove`. Removal proves containment against `origin/<default>` only." / "- A repository with a custom `origin` fetch refspec: `--no-fetch --no-remove`; the fetch prunes every `refs/remotes/origin/*` ref no server branch supplies (see references/checkout-updates.md)." / "- Regenerable ignored output (`node_modules/`, `dist/`, `.venv/`, ...) or the machine-local `.vscode/` and `.ansible/vault_pass.txt` that the user wants kept: `--no-remove`; removal deletes them without comparing the primary checkout." / "- Worktree registrations outside the root whose directories are gone: any fetched mode prunes them; only `--no-fetch --no-remove` leaves them." Same facts, same flags, one home per rule, and each trigger is scannable. > > This is a structure finding; every statement in the paragraph checks out against the driver's help text and the reference files. claim `01M38Z43ZXYXZRZPMZD44Q3NWT` of review `01M38YP09SA7WCW3117P8RYJ0P`
Author
Owner

Fixed in 11c8adf: the three stricter-mode conditions are bullets that lead with the trigger and end with the flags, and the stale-registration sentence now says it applies to every fetched mode (the driver only dry-runs the prune on --no-fetch).

<!-- gh-feedback:reply-to:86727 --> Fixed in 11c8adf: the three stricter-mode conditions are bullets that lead with the trigger and end with the flags, and the stale-registration sentence now says it applies to every fetched mode (the driver only dry-runs the prune on `--no-fetch`).
jercik marked this conversation as resolved
@ -78,3 +47,1 @@
jq '.repositories[] | select(.fatalError != null or .fetch.ok != true or .defaultBranchResolution.remoteHeadOk != true or .forge == null or .defaultBranch == null or .worktreesError != null or .gitPrune.ok != true or .staleRegistrations.ok != true) | {checkout, fatalError, forgeError, defaultBranchError, defaultBranchResolution, fetch, worktreesError, gitPrune, staleRegistrations}' "$report"
jq '.repositories[].worktrees[] | select(.removal.outcome != "removed") | select(.statusError != null or .outsideRoot or .defaultComparison == null or .defaultComparison.fresh != true) | {path, outsideRoot, statusError, defaultComparison}' "$report"
```
When the report does not answer a question, read the item's JSON record, where `<name>` is the path as `report.md` shows it, relative to the root (`.` for a checkout at the root itself): `jq --arg p <name> '.root as $r | .repositories[].worktrees[] | select(.path == (if $p == "." then $r else $r + "/" + $p end))' "$report/report.json"`. A failure row means coverage is incomplete for that checkout; never report it as clean.

low — The 'read the item's JSON record' recipe only finds worktree records, but the items it is offered for include leftover directories and repository failures
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the ## Report, then work the decisions section of skills/audit-git-checkouts/SKILL.md, the jq one-liner that follows this sentence (.root as $r | .repositories[].worktrees[] | select(.path == ...)), the renderer's display() helper (relative(report.root, path) || ".") and its sections, and the report shape in render-audit-report.ts (directoryFindings[] with path, outcome, error; repository-level fatalError, fetch, staleRegistrations).

What the subject says: the sentence is introduced generically ("the item's JSON record") right after the agent is told to work the "Needs your decision", "Possibly abandoned", "Activity unknown", and "Leftover directories" sections and to surface failures. The query only walks .repositories[].worktrees[]. A "Leftover directories" row lives in .directoryFindings[], and a Failures row produced by repositoryFailures() (fetch failed, default branch unresolved, stale-registration prune failed, audit aborted) lives on the repository object, not on a worktree. For those items the query returns nothing.

Applicable guidance: "Make claims verifiable" and "Spend detail where a plausible mistake would derail the task." An agent following this recipe for a leftover directory gets an empty result and has no stated fallback, so it either reports the item as having no record or improvises a query against a schema the skill no longer documents.

Proposed correction: either scope the sentence ("read a checkout's JSON record") and add one clause for the other two shapes, e.g. "a leftover directory is .directoryFindings[] | select(.path == ...) and a repository-level failure is on the matching .repositories[] entry (.checkout)", or keep the sentence generic and extend the query to union the three. The path-mapping rule (<name> relative to .root, . for the root itself) applies unchanged to directoryFindings[].path since the renderer uses the same display() for both.

Static reasoning against the renderer's types and the driver's root: $root field; I did not execute the jq.

claim 01M38Z43GSN38ZK1T81ABB2EZ6 of review 01M38YP09SA7WCW3117P8RYJ0P

<!-- review:claim:01M38Z43GSN38ZK1T81ABB2EZ6 --> **low** — The 'read the item's JSON record' recipe only finds worktree records, but the items it is offered for include leftover directories and repository failures lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the `## Report, then work the decisions` section of `skills/audit-git-checkouts/SKILL.md`, the jq one-liner that follows this sentence (`.root as $r | .repositories[].worktrees[] | select(.path == ...)`), the renderer's `display()` helper (`relative(report.root, path) || "."`) and its sections, and the report shape in `render-audit-report.ts` (`directoryFindings[]` with `path`, `outcome`, `error`; repository-level `fatalError`, `fetch`, `staleRegistrations`). > > What the subject says: the sentence is introduced generically ("the item's JSON record") right after the agent is told to work the "Needs your decision", "Possibly abandoned", "Activity unknown", and "Leftover directories" sections and to surface failures. The query only walks `.repositories[].worktrees[]`. A "Leftover directories" row lives in `.directoryFindings[]`, and a Failures row produced by `repositoryFailures()` (fetch failed, default branch unresolved, stale-registration prune failed, audit aborted) lives on the repository object, not on a worktree. For those items the query returns nothing. > > Applicable guidance: "Make claims verifiable" and "Spend detail where a plausible mistake would derail the task." An agent following this recipe for a leftover directory gets an empty result and has no stated fallback, so it either reports the item as having no record or improvises a query against a schema the skill no longer documents. > > Proposed correction: either scope the sentence ("read a checkout's JSON record") and add one clause for the other two shapes, e.g. "a leftover directory is `.directoryFindings[] | select(.path == ...)` and a repository-level failure is on the matching `.repositories[]` entry (`.checkout`)", or keep the sentence generic and extend the query to union the three. The path-mapping rule (`<name>` relative to `.root`, `.` for the root itself) applies unchanged to `directoryFindings[].path` since the renderer uses the same `display()` for both. > > Static reasoning against the renderer's types and the driver's `root: $root` field; I did not execute the jq. claim `01M38Z43GSN38ZK1T81ABB2EZ6` of review `01M38YP09SA7WCW3117P8RYJ0P`
Author
Owner

Fixed in 11c8adf: the recipe is now scoped to checkouts, with one clause for .directoryFindings[] (same path rule; a real report carries an absolute path there) and one for repository-level failures on the matching .repositories[] entry via checkout.

<!-- gh-feedback:reply-to:86726 --> Fixed in 11c8adf: the recipe is now scoped to checkouts, with one clause for `.directoryFindings[]` (same `path` rule; a real report carries an absolute `path` there) and one for repository-level failures on the matching `.repositories[]` entry via `checkout`.
jercik marked this conversation as resolved
@ -129,3 +58,1 @@
`latestChange` is an activity hint: newest HEAD commit or non-ignored changed-path mtime, with age relative to the report. Repository aggregates can include removed worktrees. Generated mtimes and age are not merge evidence. Never say “safe to delete” without the specific proof and applicable gates.
In ad hoc shell work, do not name a variable `status`: it is read-only in zsh.
After acting, rerun the driver over the smallest root holding the affected checkouts and their worktrees, which sit beside the primary checkout rather than inside it, and report the resulting state, not the commands that ran.

low — Closing instruction asserts linked worktrees always sit beside the primary checkout, which is a layout convention, not a fact
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the last paragraph of skills/audit-git-checkouts/SKILL.md (## Acting on a decision), the driver's discovery and --direct-children help text, and the removal-gates wording that scopes gates to "linked worktrees inside the audit root".

What the subject says: "rerun the driver over the smallest root holding the affected checkouts and their worktrees, which sit beside the primary checkout rather than inside it". The relative clause is stated as a property of worktrees. git worktree add places a linked worktree wherever the caller names, including a subdirectory of the primary checkout (a .worktrees/ or worktrees/ folder inside the repository is a common tooling convention), and nothing in the driver enforces a sibling layout.

Applicable guidance: "Make claims verifiable" and "Generalize an incident into the broadest rule that remains true; if the wording needs an exception, narrow the rule until it does not."

What goes wrong: the clause exists to stop an agent from rerunning over only the primary checkout's directory and missing its worktrees. Phrased as a fact, an agent that finds a worktree nested inside the primary can read the sentence as contradicted and fall back to guessing, or conversely assume a root one level up is always sufficient. The useful instruction is the scoping rule, not the layout claim.

Proposed correction: "After acting, rerun the driver over the smallest root that contains both the affected checkouts and every linked worktree registered to them (linked worktrees are often siblings of the primary checkout, so that root is usually the parent directory), and report the resulting state, not the commands that ran." This keeps the reason the root must be wider while making the claim true for any layout; git worktree list from the checkout is the authoritative way to find the registered paths.

Static reasoning; no run needed.

claim 01M38Z4325ZRJY471AF761C4GD of review 01M38YP09SA7WCW3117P8RYJ0P

<!-- review:claim:01M38Z4325ZRJY471AF761C4GD --> **low** — Closing instruction asserts linked worktrees always sit beside the primary checkout, which is a layout convention, not a fact lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the last paragraph of `skills/audit-git-checkouts/SKILL.md` (`## Acting on a decision`), the driver's discovery and `--direct-children` help text, and the removal-gates wording that scopes gates to "linked worktrees inside the audit root". > > What the subject says: "rerun the driver over the smallest root holding the affected checkouts and their worktrees, which sit beside the primary checkout rather than inside it". The relative clause is stated as a property of worktrees. `git worktree add` places a linked worktree wherever the caller names, including a subdirectory of the primary checkout (a `.worktrees/` or `worktrees/` folder inside the repository is a common tooling convention), and nothing in the driver enforces a sibling layout. > > Applicable guidance: "Make claims verifiable" and "Generalize an incident into the broadest rule that remains true; if the wording needs an exception, narrow the rule until it does not." > > What goes wrong: the clause exists to stop an agent from rerunning over only the primary checkout's directory and missing its worktrees. Phrased as a fact, an agent that finds a worktree nested inside the primary can read the sentence as contradicted and fall back to guessing, or conversely assume a root one level up is always sufficient. The useful instruction is the scoping rule, not the layout claim. > > Proposed correction: "After acting, rerun the driver over the smallest root that contains both the affected checkouts and every linked worktree registered to them (linked worktrees are often siblings of the primary checkout, so that root is usually the parent directory), and report the resulting state, not the commands that ran." This keeps the reason the root must be wider while making the claim true for any layout; `git worktree list` from the checkout is the authoritative way to find the registered paths. > > Static reasoning; no run needed. claim `01M38Z4325ZRJY471AF761C4GD` of review `01M38YP09SA7WCW3117P8RYJ0P`
Author
Owner

Fixed in 11c8adf: the instruction now asks for the smallest root holding the checkouts and every linked worktree registered to them, names git worktree list as the source, and states the sibling layout as the usual case rather than a fact.

<!-- gh-feedback:reply-to:86725 --> Fixed in 11c8adf: the instruction now asks for the smallest root holding the checkouts and every linked worktree registered to them, names `git worktree list` as the source, and states the sibling layout as the usual case rather than a fact.
jercik marked this conversation as resolved
@ -0,0 +335,4 @@
const directories = report.directoryFindings
.filter((finding) => !finding.registeredWorktree)
.map((finding) => [display(finding.path), finding.outcome === "inspection-failed" ? `inspection failed: ${firstLine(finding.error)}` : finding.outcome]);

low — Leftover-directory rows print the driver's outcome codes instead of plain wording in the user-facing report
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the Leftover directories section of render-audit-report.ts (the directories mapping and addSection("Leftover directories", ["Directory", "Finding"], directories)), the driver's directoryFindings outcomes in audit-checkouts.sh (outcome=empty-directory, outcome=unclassified-directory, and inspection-failed), the renderer test (assert.ok(markdown.includes("| leftover | empty-directory |"))), and the new SKILL.md instruction in ## Report, then work the decisions.

What the subject does: only the inspection-failed outcome is translated (inspection failed: <error>); the other two outcomes are emitted verbatim, so the report's Finding column reads empty-directory or unclassified-directory. Every other table in the same renderer translates codes into sentences (KEPT_REASON_BY_OUTCOME, PROOF_BY_RUNG, defaultNotCurrentReason), and SKILL.md tells the agent: "Use the report's plain wording; outcome codes such as judgment/containment-not-proven and "rung 2" mean nothing to the user." The report itself therefore contradicts the wording standard SKILL.md sets for it, and report.md is the artifact the agent is told to hand to the user.

What goes wrong: unclassified-directory is an internal classification token, not a finding a user can act on; it does not say what the driver saw (a nonempty directory with no .git marker whose contents were not inspected). The cleanup.md "Leftover directories" section explains the two states in prose but the report does not carry that meaning.

Proposed correction: map the two outcomes to plain text in the renderer, e.g. empty-directory -> "empty" and unclassified-directory -> "not a repository; contents not inspected", keeping the existing inspection failed: ... form, and update the renderer test assertion to the new wording. This keeps the same information (the three states) while matching the plain-wording rule the skill imposes on everything shown to the user.

Static reasoning only; I did not run the renderer. The test assertion | leftover | empty-directory | confirms the rendered form.

claim 01M38Z41HC8DJ2KNXRGAA7GGFC of review 01M38YP09SA7WCW3117P8RYJ0P

<!-- review:claim:01M38Z41HC8DJ2KNXRGAA7GGFC --> **low** — Leftover-directory rows print the driver's outcome codes instead of plain wording in the user-facing report lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the `Leftover directories` section of `render-audit-report.ts` (the `directories` mapping and `addSection("Leftover directories", ["Directory", "Finding"], directories)`), the driver's `directoryFindings` outcomes in `audit-checkouts.sh` (`outcome=empty-directory`, `outcome=unclassified-directory`, and `inspection-failed`), the renderer test (`assert.ok(markdown.includes("| leftover | empty-directory |"))`), and the new SKILL.md instruction in `## Report, then work the decisions`. > > What the subject does: only the `inspection-failed` outcome is translated (`inspection failed: <error>`); the other two outcomes are emitted verbatim, so the report's Finding column reads `empty-directory` or `unclassified-directory`. Every other table in the same renderer translates codes into sentences (`KEPT_REASON_BY_OUTCOME`, `PROOF_BY_RUNG`, `defaultNotCurrentReason`), and SKILL.md tells the agent: "Use the report's plain wording; outcome codes such as `judgment/containment-not-proven` and \"rung 2\" mean nothing to the user." The report itself therefore contradicts the wording standard SKILL.md sets for it, and `report.md` is the artifact the agent is told to hand to the user. > > What goes wrong: `unclassified-directory` is an internal classification token, not a finding a user can act on; it does not say what the driver saw (a nonempty directory with no `.git` marker whose contents were not inspected). The `cleanup.md` "Leftover directories" section explains the two states in prose but the report does not carry that meaning. > > Proposed correction: map the two outcomes to plain text in the renderer, e.g. `empty-directory` -> "empty" and `unclassified-directory` -> "not a repository; contents not inspected", keeping the existing `inspection failed: ...` form, and update the renderer test assertion to the new wording. This keeps the same information (the three states) while matching the plain-wording rule the skill imposes on everything shown to the user. > > Static reasoning only; I did not run the renderer. The test assertion `| leftover | empty-directory |` confirms the rendered form. claim `01M38Z41HC8DJ2KNXRGAA7GGFC` of review `01M38YP09SA7WCW3117P8RYJ0P`
Author
Owner

Fixed in e29292b: the Finding column now reads "empty" or "not a repository; contents not inspected" (the inspection failed: … form is unchanged). A real report's old-notes row renders with the new wording; the test assertion follows it.

<!-- gh-feedback:reply-to:86728 --> Fixed in e29292b: the Finding column now reads "empty" or "not a repository; contents not inspected" (the `inspection failed: …` form is unchanged). A real report's `old-notes` row renders with the new wording; the test assertion follows it.
jercik marked this conversation as resolved
The Finding column printed the driver's outcome codes; it now says
"empty" or "not a repository; contents not inspected", matching the
wording rule the skill sets for everything shown to the user.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
docs(audit-git-checkouts): list the stricter-mode conditions and widen the record lookup
All checks were successful
commit-msg / commitlint (pull_request) Successful in 25s
Node tests / node:test (pull_request) Successful in 45s
Review / Review (pull_request_target) Successful in 12m43s
11c8adf74a
The three conditions that move an audit to a stricter mode are bullets
with their flags instead of one paragraph. The JSON record recipe now
covers leftover directories and repository failures, and the rerun
instruction asks for every registered worktree rather than asserting a
sibling layout.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ -34,6 +23,3 @@
apt-get update
apt-get install -y --no-install-recommends "${missing[@]}"
- name: Run tests
shell: bash
run: |

medium — node-test workflow drops the step that installed jq, but audit-checkouts.test.mjs still shells out to jq on nearly every test
lens general-bug · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the diff of .forgejo/workflows/node-test.yml, the full skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs, audit-checkouts.sh, inspect-ignored-content.sh and prove-branch-contained.sh in the subject tree.

What changed: the workflow removed the 'Ensure baseline shell tooling' step, whose own comment (deleted in this diff) read: "The Forge maps ubuntu-latest to a minimal Node image, so tools GitHub's runner preinstalls are absent and a test shelling out to one dies here." That step ran apt-get to install jq, lsof and column when missing. Nothing replaces it: the remaining job is checkout, setup-node, and the 'Run tests' step, which now also selects the new render-audit-report.test.ts.

What still needs jq: audit-checkouts.sh has require_command jq in its main path and calls jq in nearly every function (write_submodule_metadata, write_latest_change, annotate_lock, annotate_activity_review, write_directory_findings, decide_removal_outcome, maybe_remove_worktree, classify_checkout, ...). inspect-ignored-content.sh's write_ignored_scan ends with jq -n --arg state ... >"$output" || return 1. prove-branch-contained.sh has command -v jq >/dev/null 2>&1 || fail "required command not found: jq". The test file sources these scripts and calls those functions directly, e.g. the scanIgnoredContent helper runs source "$1"; write_ignored_scan ... and then does JSON.parse(readFileSync(outputPath, "utf8")); with jq absent, write_ignored_scan returns 1 before writing the output file, so readFileSync throws ENOENT and the test fails. The removal tests call maybe_remove_worktree, which is jq on every path. There is no skip-if-missing guard in the test file: grep for 'jq' in audit-checkouts.test.mjs finds nothing.

lsof and column are no longer referenced by any test or script in the tree, so dropping them is fine; jq is the problem.

What goes wrong: on a runner image without jq, the node:test job fails for audit-checkouts.test.mjs on the first jq-dependent test, and the job aborts before the other test files run (the loop uses set -eu and node exits non-zero). The push/pull_request check is red for every change to the repository, not only this one.

Proof gap: I cannot inspect the Forge's ubuntu-latest image from this sandbox, so I cannot confirm jq is absent there today. The only evidence about that image in the subject is the deleted comment asserting that preinstalled tools are absent, plus the fact that the step was conditional (command -v jq >/dev/null || missing+=(jq)), so keeping it cost nothing when jq was present. If the image is now known to ship jq, this claim is moot; otherwise restore the conditional apt-get step (or add jq to the image) before merging.

Safe correction: reinstate the conditional install step limited to jq:

  - name: Ensure baseline shell tooling
    shell: bash
    run: |
      set -eu
      command -v jq >/dev/null && exit 0
      apt-get update
      apt-get install -y --no-install-recommends jq

If this workflow is rendered by j4k-align (the error text in the run step says so), the fix belongs in the template as well, or the next --fix will drop the step again.

claim 01M3906MVBQXRKC369HBXEQ9E1 of review 01M38ZNK9786DK6HMH6JHEC151

<!-- review:claim:01M3906MVBQXRKC369HBXEQ9E1 --> **medium** — node-test workflow drops the step that installed jq, but audit-checkouts.test.mjs still shells out to jq on nearly every test lens `general-bug` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the diff of .forgejo/workflows/node-test.yml, the full skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs, audit-checkouts.sh, inspect-ignored-content.sh and prove-branch-contained.sh in the subject tree. > > What changed: the workflow removed the 'Ensure baseline shell tooling' step, whose own comment (deleted in this diff) read: "The Forge maps ubuntu-latest to a minimal Node image, so tools GitHub's runner preinstalls are absent and a test shelling out to one dies here." That step ran apt-get to install jq, lsof and column when missing. Nothing replaces it: the remaining job is checkout, setup-node, and the 'Run tests' step, which now also selects the new render-audit-report.test.ts. > > What still needs jq: audit-checkouts.sh has `require_command jq` in its main path and calls jq in nearly every function (write_submodule_metadata, write_latest_change, annotate_lock, annotate_activity_review, write_directory_findings, decide_removal_outcome, maybe_remove_worktree, classify_checkout, ...). inspect-ignored-content.sh's write_ignored_scan ends with `jq -n --arg state ... >"$output" || return 1`. prove-branch-contained.sh has `command -v jq >/dev/null 2>&1 || fail "required command not found: jq"`. The test file sources these scripts and calls those functions directly, e.g. the scanIgnoredContent helper runs `source "$1"; write_ignored_scan ...` and then does `JSON.parse(readFileSync(outputPath, "utf8"))`; with jq absent, write_ignored_scan returns 1 before writing the output file, so readFileSync throws ENOENT and the test fails. The removal tests call maybe_remove_worktree, which is jq on every path. There is no skip-if-missing guard in the test file: grep for 'jq' in audit-checkouts.test.mjs finds nothing. > > lsof and column are no longer referenced by any test or script in the tree, so dropping them is fine; jq is the problem. > > What goes wrong: on a runner image without jq, the node:test job fails for audit-checkouts.test.mjs on the first jq-dependent test, and the job aborts before the other test files run (the loop uses `set -eu` and node exits non-zero). The push/pull_request check is red for every change to the repository, not only this one. > > Proof gap: I cannot inspect the Forge's ubuntu-latest image from this sandbox, so I cannot confirm jq is absent there today. The only evidence about that image in the subject is the deleted comment asserting that preinstalled tools are absent, plus the fact that the step was conditional (`command -v jq >/dev/null || missing+=(jq)`), so keeping it cost nothing when jq was present. If the image is now known to ship jq, this claim is moot; otherwise restore the conditional apt-get step (or add jq to the image) before merging. > > Safe correction: reinstate the conditional install step limited to jq: > > - name: Ensure baseline shell tooling > shell: bash > run: | > set -eu > command -v jq >/dev/null && exit 0 > apt-get update > apt-get install -y --no-install-recommends jq > > If this workflow is rendered by j4k-align (the error text in the run step says so), the fix belongs in the template as well, or the next --fix will drop the step again. claim `01M3906MVBQXRKC369HBXEQ9E1` of review `01M38ZNK9786DK6HMH6JHEC151`
Author
Owner

Fourth time on this file, same evidence, now on three heads: the Node tests / node:test runs #1396 (099f085), #1400 (38f0009) and #1407 (11c8adf) each ran audit-checkouts.test.mjs on the Forge runner with no install step and passed (54, 54 and 55 tests). The runner image ships jq; j4k-align dropped the bootstrap deliberately (af91298, #233) and renders this workflow, so a step re-added here is drift.

<!-- gh-feedback:reply-to:86794 --> Fourth time on this file, same evidence, now on three heads: the `Node tests / node:test` runs #1396 (099f085), #1400 (38f0009) and #1407 (11c8adf) each ran `audit-checkouts.test.mjs` on the Forge runner with no install step and passed (54, 54 and 55 tests). The runner image ships jq; j4k-align dropped the bootstrap deliberately (af91298, #233) and renders this workflow, so a step re-added here is drift.
@ -88,4 +76,2 @@
# signal handler, so a kill here writes nothing to the PR — the durable
# fix is an internal deadline in the wrapper, not a larger number here.
# Re-derive when the pin moves.
# Limit runner occupancy even if retries or reconciliation are unfinished.
timeout-minutes: 130

low — Replacement timeout comment restates what timeout-minutes means and drops the one fact an editor needs
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the timeout-minutes: 130 line and its comment in .forgejo/workflows/review.yml, plus the diff hunk that replaced the previous sixteen-line derivation.

What the subject says: the new comment reads "Limit runner occupancy even if retries or reconciliation are unfinished." That is the definition of timeout-minutes on any job; a reader who knows the workflow syntax already knows it. The previous comment was a journal-style derivation and deserved cutting, but it carried one durable fact: 130 is a ceiling computed from the pinned wrapper's worst-case wait deadlines plus headroom for forge calls, and it has to be re-derived when the wrapper pin (uses: https://code.j4k.dev/j4k-oss/review-wrapper@21cd18a…) moves. It also noted that the wrapper installs no signal handler, so a kill writes nothing to the PR.

What goes wrong: under the writing skill's no-op test, the new line changes no behavior and can be deleted. Meanwhile the number 130 now looks arbitrary. The next person who bumps the wrapper pin, or who sees a run killed at 130 minutes, has no signal that the value is coupled to the pin or that the kill leaves the PR without a review. "State Durable Truth" asks to keep causal facts that help a future agent apply a rule while dropping the change history.

Proposed correction: replace the comment with the durable fact in one or two lines, for example:

# Ceiling on the pinned wrapper's worst-case waits plus forge-call headroom;
# re-derive when the pin moves. A kill here writes nothing to the PR.

That keeps the rationale that lets an editor adapt the value and removes the restatement. Alternatively delete the comment entirely; either is better than the current line. Static reasoning from the two versions of the comment; I could not inspect the wrapper to re-verify the figure.

claim 01M38ZXX9FRAJSN16S5XSG6AF3 of review 01M38ZNK9786DK6HMH6JHEC151

<!-- review:claim:01M38ZXX9FRAJSN16S5XSG6AF3 --> **low** — Replacement timeout comment restates what timeout-minutes means and drops the one fact an editor needs lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the `timeout-minutes: 130` line and its comment in .forgejo/workflows/review.yml, plus the diff hunk that replaced the previous sixteen-line derivation. > > What the subject says: the new comment reads "Limit runner occupancy even if retries or reconciliation are unfinished." That is the definition of `timeout-minutes` on any job; a reader who knows the workflow syntax already knows it. The previous comment was a journal-style derivation and deserved cutting, but it carried one durable fact: 130 is a ceiling computed from the pinned wrapper's worst-case wait deadlines plus headroom for forge calls, and it has to be re-derived when the wrapper pin (`uses: https://code.j4k.dev/j4k-oss/review-wrapper@21cd18a…`) moves. It also noted that the wrapper installs no signal handler, so a kill writes nothing to the PR. > > What goes wrong: under the writing skill's no-op test, the new line changes no behavior and can be deleted. Meanwhile the number 130 now looks arbitrary. The next person who bumps the wrapper pin, or who sees a run killed at 130 minutes, has no signal that the value is coupled to the pin or that the kill leaves the PR without a review. "State Durable Truth" asks to keep causal facts that help a future agent apply a rule while dropping the change history. > > Proposed correction: replace the comment with the durable fact in one or two lines, for example: > > # Ceiling on the pinned wrapper's worst-case waits plus forge-call headroom; > # re-derive when the pin moves. A kill here writes nothing to the PR. > > That keeps the rationale that lets an editor adapt the value and removes the restatement. Alternatively delete the comment entirely; either is better than the current line. Static reasoning from the two versions of the comment; I could not inspect the wrapper to re-verify the figure. claim `01M38ZXX9FRAJSN16S5XSG6AF3` of review `01M38ZNK9786DK6HMH6JHEC151`
Author
Owner

Same as the three earlier timeout-comment findings: review.yml is rendered from j4k-align's templates/workflows/review-forgejo.yml.hbs, whose current text (align #270, 4ffe59e) is exactly this line, and the header marks direct edits as drift. The derivation belongs in the align template, not in this repository.

<!-- gh-feedback:reply-to:86796 --> Same as the three earlier timeout-comment findings: `review.yml` is rendered from j4k-align's `templates/workflows/review-forgejo.yml.hbs`, whose current text (align #270, 4ffe59e) is exactly this line, and the header marks direct edits as drift. The derivation belongs in the align template, not in this repository.
@ -17,3 +18,1 @@
| Keep worktrees / do not remove them | `--no-remove` | Same fetched audit and updates; no worktree removal. |
| Read-only / no checkout changes | `--no-fetch --no-remove` | Cached diagnostic; no driver fetch, updates, or registration/worktree deletion. Comparisons are stale. |
| Cleanup | Default, plus the named cleanup classes | Interior deletions require the evidence and scope below. |
| Audit, clean up, "which are behind" | none | Fetch and prune `origin`; fast-forward eligible default checkouts; prune stale worktree registrations; remove proven-merged linked worktrees inside the root. |

medium — Mode table omits the submodule checkouts and unstaged Gitlink changes a default run writes
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the new "Choose the mode" table in SKILL.md, the driver's usage() text in scripts/audit-checkouts.sh, the tracking_update_eligible block in the same script, and the "Submodule selectors" section of references/checkout-updates.md.

What the subject says: the table's "What changes" column for the default mode lists four writes: fetch and prune origin, fast-forward eligible default checkouts, prune stale worktree registrations, and remove proven-merged linked worktrees. The --no-remove row is "Everything above except removal", so it inherits that list. The paragraph that follows says "The default mode's updates and removals need no further confirmation."

What the driver actually does in every fetched mode: its own help text lists two further writes, "submodule sync after a fast-forward; branch/tag submodule selectors advanced outside third-party/ paths". The tracking_update_eligible condition in audit-checkouts.sh runs synchronize_first_party_tracking_submodules on any fresh, first-party, default-branch checkout that has a branch or tag selector, whether or not a fast-forward happened (the condition is fast_forward_attempted != true || fast_forward_ok == true). references/checkout-updates.md describes the effect: the driver "fetches that branch or tag, checks the submodule out detached, and leaves the Gitlink change unstaged for the owner's next commit". That reference is only read when a selector problem is being acted on, after the run.

What goes wrong: the table is the one place the agent decides which mode to run and what to tell the user will happen. An agent following it will describe the default run as fetch, fast-forward, prune, and remove, then leave the user with submodule working trees moved to a new detached commit and unstaged Gitlink modifications in checkouts that were clean before the audit. The previous SKILL.md stated this ("advances configured branch/tag selectors, leaving Gitlink changes unstaged; this can happen without a superproject fast-forward"); the rewrite dropped it from the decision point without moving it anywhere the agent reads before running. The writing skill's "Specify the Discipline" asks the contract to state the outcome and boundaries up front, and "Make the decisive constraint prominent".

Proposed correction: extend the default row's "What changes" cell, for example: "Fetch and prune origin; fast-forward eligible default checkouts and sync their submodules; in first-party default checkouts, move submodules to their configured branch or tag and leave the Gitlink change unstaged; prune stale worktree registrations; remove proven-merged linked worktrees inside the root." Optionally add a fourth bullet under "Three conditions call for a stricter mode": a first-party checkout whose submodule selectors must not move: --no-fetch --no-remove (checkout-updates.md already gives that advice for update = none). This preserves the table's brevity while making the list of writes complete.

What would refute it: showing that synchronize_first_party_tracking_submodules cannot change a working tree, or that SKILL.md names this write somewhere before the run. I found neither.

claim 01M38ZXWEKVZQ1FMDZHPFPG0Y7 of review 01M38ZNK9786DK6HMH6JHEC151

<!-- review:claim:01M38ZXWEKVZQ1FMDZHPFPG0Y7 --> **medium** — Mode table omits the submodule checkouts and unstaged Gitlink changes a default run writes lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the new "Choose the mode" table in SKILL.md, the driver's `usage()` text in scripts/audit-checkouts.sh, the `tracking_update_eligible` block in the same script, and the "Submodule selectors" section of references/checkout-updates.md. > > What the subject says: the table's "What changes" column for the default mode lists four writes: fetch and prune origin, fast-forward eligible default checkouts, prune stale worktree registrations, and remove proven-merged linked worktrees. The `--no-remove` row is "Everything above except removal", so it inherits that list. The paragraph that follows says "The default mode's updates and removals need no further confirmation." > > What the driver actually does in every fetched mode: its own help text lists two further writes, "submodule sync after a fast-forward; branch/tag submodule selectors advanced outside third-party/ paths". The `tracking_update_eligible` condition in audit-checkouts.sh runs `synchronize_first_party_tracking_submodules` on any fresh, first-party, default-branch checkout that has a branch or tag selector, whether or not a fast-forward happened (the condition is `fast_forward_attempted != true || fast_forward_ok == true`). references/checkout-updates.md describes the effect: the driver "fetches that branch or tag, checks the submodule out detached, and leaves the Gitlink change unstaged for the owner's next commit". That reference is only read when a selector problem is being acted on, after the run. > > What goes wrong: the table is the one place the agent decides which mode to run and what to tell the user will happen. An agent following it will describe the default run as fetch, fast-forward, prune, and remove, then leave the user with submodule working trees moved to a new detached commit and unstaged Gitlink modifications in checkouts that were clean before the audit. The previous SKILL.md stated this ("advances configured branch/tag selectors, leaving Gitlink changes unstaged; this can happen without a superproject fast-forward"); the rewrite dropped it from the decision point without moving it anywhere the agent reads before running. The writing skill's "Specify the Discipline" asks the contract to state the outcome and boundaries up front, and "Make the decisive constraint prominent". > > Proposed correction: extend the default row's "What changes" cell, for example: "Fetch and prune `origin`; fast-forward eligible default checkouts and sync their submodules; in first-party default checkouts, move submodules to their configured branch or tag and leave the Gitlink change unstaged; prune stale worktree registrations; remove proven-merged linked worktrees inside the root." Optionally add a fourth bullet under "Three conditions call for a stricter mode": a first-party checkout whose submodule selectors must not move: `--no-fetch --no-remove` (checkout-updates.md already gives that advice for `update = none`). This preserves the table's brevity while making the list of writes complete. > > What would refute it: showing that `synchronize_first_party_tracking_submodules` cannot change a working tree, or that SKILL.md names this write somewhere before the run. I found neither. claim `01M38ZXWEKVZQ1FMDZHPFPG0Y7` of review `01M38ZNK9786DK6HMH6JHEC151`
jercik marked this conversation as resolved
@ -1550,0 +1418,4 @@
assert.equal(result.activityReview.error, null);
});
test("a changed directory that cannot be fully walked leaves the timestamp unknown", { skip: process.getuid?.() === 0 && "root reads directories regardless of mode" }, (context) => {

low — The partial-walk activity test is skipped under root, so the find-failure branch of newest_directory_epoch may never execute in CI
lens test-trimming · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the new test a changed directory that cannot be fully walked leaves the timestamp unknown in audit-checkouts.test.mjs, the function it targets (newest_directory_epoch in audit-checkouts.sh, added in this change), the sibling helper pattern failStat in createActivityFixture, and .forgejo/workflows/node-test.yml.

What the test does: it builds the failure by chmod-ing a nested directory to 0o000 so find cannot descend, and guards the whole test with

{ skip: process.getuid?.() === 0 && "root reads directories regardless of mode" }

The skip is correct as far as it goes: root bypasses directory mode bits, so under uid 0 find succeeds and the setup cannot produce the failure. But it means the test contributes nothing wherever the suite runs as root.

What it is the only protection for: the failure branch of the new helper in audit-checkouts.sh:

if ! find "$directory_path" -type f -print0 >"$listing" 2>/dev/null; then
  rm -f "$listing"
  return 1
fi

and the caller in annotate_activity_review that turns that return 1 into Changed-path activity timestamp unavailable: <path> and an unknown outcome. No other test in the file drives newest_directory_epoch to failure; the other new directory test (a changed directory is timed by the newest file inside it) covers only the success path. A regression such as dropping the if ! find ...; then return 1 guard (falling back to the directory's own mtime and reporting possibly-abandoned instead of unknown) would stay green on any root runner.

Observed: in this review sandbox id -u prints 0, so the skip predicate fires here. Proof gap: node-test.yml uses runs-on: ubuntu-latest with no container: or user setting, and I cannot see the Forgejo runner's configuration; container-based runners commonly execute as root, but I could not confirm it for this repository. If the project's runner executes as a non-root user the test does run in CI and this claim reduces to a portability note.

Repair that keeps the protection on every uid: the fixture already shows the pattern for forcing a helper to fail without relying on permissions:

'source "$1"; stat_epoch() { return 1; }; annotate_activity_review ...'

The same technique works for the walk: source the script, define find() { return 1; } (or prepend a failing find shim via the existing ignoredCommandEnvironment helper) and then call annotate_activity_review on a fixture whose changed path is a directory. That exercises the exact find failure branch, asserts outcome === "unknown" and the timestamp unavailable: sample.txt error, and does not depend on the process uid. The chmod variant can stay as the real-permissions scenario for non-root runs, but it should not be the only check of this branch.

claim 01M38ZY3DD4E0V9A2C71D5NGX1 of review 01M38ZNK9786DK6HMH6JHEC151

<!-- review:claim:01M38ZY3DD4E0V9A2C71D5NGX1 --> **low** — The partial-walk activity test is skipped under root, so the find-failure branch of newest_directory_epoch may never execute in CI lens `test-trimming` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the new test `a changed directory that cannot be fully walked leaves the timestamp unknown` in audit-checkouts.test.mjs, the function it targets (`newest_directory_epoch` in audit-checkouts.sh, added in this change), the sibling helper pattern `failStat` in `createActivityFixture`, and `.forgejo/workflows/node-test.yml`. > > What the test does: it builds the failure by chmod-ing a nested directory to 0o000 so `find` cannot descend, and guards the whole test with > > { skip: process.getuid?.() === 0 && "root reads directories regardless of mode" } > > The skip is correct as far as it goes: root bypasses directory mode bits, so under uid 0 `find` succeeds and the setup cannot produce the failure. But it means the test contributes nothing wherever the suite runs as root. > > What it is the only protection for: the failure branch of the new helper in audit-checkouts.sh: > > if ! find "$directory_path" -type f -print0 >"$listing" 2>/dev/null; then > rm -f "$listing" > return 1 > fi > > and the caller in `annotate_activity_review` that turns that `return 1` into `Changed-path activity timestamp unavailable: <path>` and an `unknown` outcome. No other test in the file drives `newest_directory_epoch` to failure; the other new directory test (`a changed directory is timed by the newest file inside it`) covers only the success path. A regression such as dropping the `if ! find ...; then return 1` guard (falling back to the directory's own mtime and reporting `possibly-abandoned` instead of `unknown`) would stay green on any root runner. > > Observed: in this review sandbox `id -u` prints 0, so the skip predicate fires here. Proof gap: `node-test.yml` uses `runs-on: ubuntu-latest` with no `container:` or user setting, and I cannot see the Forgejo runner's configuration; container-based runners commonly execute as root, but I could not confirm it for this repository. If the project's runner executes as a non-root user the test does run in CI and this claim reduces to a portability note. > > Repair that keeps the protection on every uid: the fixture already shows the pattern for forcing a helper to fail without relying on permissions: > > 'source "$1"; stat_epoch() { return 1; }; annotate_activity_review ...' > > The same technique works for the walk: source the script, define `find() { return 1; }` (or prepend a failing `find` shim via the existing `ignoredCommandEnvironment` helper) and then call `annotate_activity_review` on a fixture whose changed path is a directory. That exercises the exact `find` failure branch, asserts `outcome === "unknown"` and the `timestamp unavailable: sample.txt` error, and does not depend on the process uid. The chmod variant can stay as the real-permissions scenario for non-root runs, but it should not be the only check of this branch. claim `01M38ZY3DD4E0V9A2C71D5NGX1` of review `01M38ZNK9786DK6HMH6JHEC151`
jercik marked this conversation as resolved
@ -0,0 +249,4 @@
}
case "manual-review": {
if (comparison !== null && !comparison.fresh) {
if (fetched) return { aheadBehind: formatAheadBehind(comparison), why: "not compared: origin fetch failed" };

low — Renderer reports "origin fetch failed" for a default checkout whose fetch succeeded but whose server default-branch lookup failed
lens general-bug · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: primaryDecision in render-audit-report.ts (the manual-review branch), repositoryFailures in the same file, and audit_repository in audit-checkouts.sh, which is the only producer of defaultComparison.fresh.

Mechanism: the driver sets comparison_fresh only when both steps succeed: if [ "$fetch_ok" = true ] && [ "$remote_head_ok" = true ]; then comparison_fresh=true; fi. remote_head_ok comes from git ls-remote --symref origin HEAD, which can fail (or return no ref: refs/heads/... line) while git fetch --prune origin succeeds, for example on a remote whose HEAD is unborn or detached, or a mirror that does not advertise a symref. In that case every worktree gets defaultComparison.fresh=false, classify_checkout labels a default-branch checkout manual-review, and the renderer's manual-review branch reaches if (fetched) return { ..., why: "not compared: origin fetch failed" }.

What goes wrong: the 'Needs your decision: not current' row tells the user the fetch failed, while the Failures table for the same repository, built by repositoryFailures, says the opposite: fetch failed is only pushed when fetch.ok !== true, and this case instead produces "could not read the server's default branch". The two sections of the same report contradict each other, and the user is sent to debug network/auth on a fetch that worked. stepFailure already handles the same condition correctly for linked worktrees with the wording "origin fetch or default-branch lookup failed".

Static reasoning only: I could not run the driver here (jq is not installed in this sandbox), so I did not produce a real report exhibiting this; the path is traced from the code. The renderer tests cover only the fetch.ok=false case ("a fetched run never counts an uncompared checkout as current").

Safe correction: use the same neutral wording stepFailure uses, e.g. "not compared: origin fetch or default-branch lookup failed", or pass the repository's fetch.ok / defaultBranchResolution.remoteHeadOk into primaryDecision and pick the message from those.

claim 01M3907BR6AHXCVJN2G922EACN of review 01M38ZNK9786DK6HMH6JHEC151

<!-- review:claim:01M3907BR6AHXCVJN2G922EACN --> **low** — Renderer reports "origin fetch failed" for a default checkout whose fetch succeeded but whose server default-branch lookup failed lens `general-bug` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: primaryDecision in render-audit-report.ts (the manual-review branch), repositoryFailures in the same file, and audit_repository in audit-checkouts.sh, which is the only producer of defaultComparison.fresh. > > Mechanism: the driver sets comparison_fresh only when both steps succeed: `if [ "$fetch_ok" = true ] && [ "$remote_head_ok" = true ]; then comparison_fresh=true; fi`. remote_head_ok comes from `git ls-remote --symref origin HEAD`, which can fail (or return no `ref: refs/heads/...` line) while `git fetch --prune origin` succeeds, for example on a remote whose HEAD is unborn or detached, or a mirror that does not advertise a symref. In that case every worktree gets defaultComparison.fresh=false, classify_checkout labels a default-branch checkout manual-review, and the renderer's manual-review branch reaches `if (fetched) return { ..., why: "not compared: origin fetch failed" }`. > > What goes wrong: the 'Needs your decision: not current' row tells the user the fetch failed, while the Failures table for the same repository, built by repositoryFailures, says the opposite: `fetch failed` is only pushed when fetch.ok !== true, and this case instead produces "could not read the server's default branch". The two sections of the same report contradict each other, and the user is sent to debug network/auth on a fetch that worked. stepFailure already handles the same condition correctly for linked worktrees with the wording "origin fetch or default-branch lookup failed". > > Static reasoning only: I could not run the driver here (jq is not installed in this sandbox), so I did not produce a real report exhibiting this; the path is traced from the code. The renderer tests cover only the fetch.ok=false case ("a fetched run never counts an uncompared checkout as current"). > > Safe correction: use the same neutral wording stepFailure uses, e.g. "not compared: origin fetch or default-branch lookup failed", or pass the repository's fetch.ok / defaultBranchResolution.remoteHeadOk into primaryDecision and pick the message from those. claim `01M3907BR6AHXCVJN2G922EACN` of review `01M38ZNK9786DK6HMH6JHEC151`
jercik marked this conversation as resolved
Author
Owner

Replying to review comment #86795

Real, and deferred: this is review round 4 of this PR, where only severe bugs are fixed in place, and a table cell is a follow-up.

The driver's own --help lists the two writes the table omits ("submodule sync after a fast-forward; branch/tag submodule selectors advanced outside third-party/ paths"). Exact fix, in skills/audit-git-checkouts/SKILL.md: extend the default row's "What changes" cell to "Fetch and prune origin; fast-forward eligible default checkouts and sync their submodules; in first-party default checkouts, move branch- or tag-tracked submodules to their selector and leave the Gitlink change unstaged; prune stale worktree registrations; remove proven-merged linked worktrees inside the root." No behavior changes; no commit in this PR.

> Replying to review comment #86795 Real, and deferred: this is review round 4 of this PR, where only severe bugs are fixed in place, and a table cell is a follow-up. The driver's own `--help` lists the two writes the table omits ("submodule sync after a fast-forward; branch/tag submodule selectors advanced outside third-party/ paths"). Exact fix, in `skills/audit-git-checkouts/SKILL.md`: extend the default row's "What changes" cell to "Fetch and prune `origin`; fast-forward eligible default checkouts and sync their submodules; in first-party default checkouts, move branch- or tag-tracked submodules to their selector and leave the Gitlink change unstaged; prune stale worktree registrations; remove proven-merged linked worktrees inside the root." No behavior changes; no commit in this PR.
Author
Owner

Replying to review comment #86797

Agreed that the chmod test cannot run as root, and the Forge runner does run as root, so in CI the failure branch is untested. Deferred: this is review round 4 of this PR, and a test-only change is a follow-up.

Exact fix, in skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs: add a test beside the chmod one that reuses the createActivityFixture pattern with a shimmed helper, source "$1"; find() { return 1; }; annotate_activity_review …, on a fixture whose changed path is a directory, asserting outcome === "unknown" and /timestamp unavailable: sample\.txt/. Keep the chmod test as the real-permissions case for non-root runs. No commit in this PR.

> Replying to review comment #86797 Agreed that the chmod test cannot run as root, and the Forge runner does run as root, so in CI the failure branch is untested. Deferred: this is review round 4 of this PR, and a test-only change is a follow-up. Exact fix, in `skills/audit-git-checkouts/scripts/audit-checkouts.test.mjs`: add a test beside the chmod one that reuses the `createActivityFixture` pattern with a shimmed helper, `source "$1"; find() { return 1; }; annotate_activity_review …`, on a fixture whose changed path is a directory, asserting `outcome === "unknown"` and `/timestamp unavailable: sample\.txt/`. Keep the chmod test as the real-permissions case for non-root runs. No commit in this PR.
Author
Owner

Replying to review comment #86798

The trace holds: comparison_fresh needs both the fetch and the ls-remote --symref lookup, and the manual-review branch of primaryDecision names only the fetch. Deferred: this is review round 4 of this PR, and a one-string wording change is a follow-up.

Exact fix, in skills/audit-git-checkouts/scripts/render-audit-report.ts, primaryDecision: change "not compared: origin fetch failed" to "not compared: origin fetch or default-branch lookup failed", the wording stepFailure already uses for linked worktrees, and update the assertion in render-audit-report.test.ts ("a fetched run never counts an uncompared checkout as current"). No commit in this PR.

> Replying to review comment #86798 The trace holds: `comparison_fresh` needs both the fetch and the `ls-remote --symref` lookup, and the manual-review branch of `primaryDecision` names only the fetch. Deferred: this is review round 4 of this PR, and a one-string wording change is a follow-up. Exact fix, in `skills/audit-git-checkouts/scripts/render-audit-report.ts`, `primaryDecision`: change `"not compared: origin fetch failed"` to `"not compared: origin fetch or default-branch lookup failed"`, the wording `stepFailure` already uses for linked worktrees, and update the assertion in `render-audit-report.test.ts` ("a fetched run never counts an uncompared checkout as current"). No commit in this PR.
jercik merged commit 8f1e23549d into main 2026-09-24 08:10:40 +00:00
jercik deleted branch refactor/rebuild-audit-git-checkouts 2026-09-24 08:10:40 +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!77
No description provided.