[bborbe]

Every vault-cli update-go Job failed make precommit (exit 2) on scripts/resume-block-card-rule-test.sh: self-check reports an unreadable consumer: want [UNREADABLE: commands], got [].

Root cause: the self-check passes a directory to grep -qF and expects exit 2. The github-update-go-agent image is Alpine; its grep is BusyBox, which exits 1 on a directory ("no match"). BSD grep (macOS) exits 2.

image  : grep -qF -- x <dir> → rc=1
macOS  : /usr/bin/grep -qF -- x <dir> → "Is a directory", rc=2

Fix: both helpers in resume-block-card-rule-test.sh and the same pattern in checkpoint-state-rule-test.sh report UNREADABLE for any non-regular file before calling grep.

Verified: make precommit exits 0 inside docker.io/bborbe/github-update-go-agent:v0.21.0 as uid 1000. Before the fix, resume-block was 5/6 and checkpoint-state failed its unreadable self-check in the same image. Both still pass on macOS (6/6, 9/9).

[ben-s-pull-request-reviewer]

{
  "verdict": "approve",
  "summary": "The change is a small, well-scoped portability fix: both rule-test helpers now classify any non-regular file as UNREADABLE before grep runs, so the self-checks no longer depend on how grep treats a directory. I verified the guard against every call site in both scripts, confirmed the remaining direct grep calls cannot silently pass on a non-regular or unreadable path under either GNU or BusyBox grep, and confirmed the CHANGELOG edit satisfies check-versions/check-changelog and the conventional-prefix rule. One non-blocking nit: the resume-block header's rationale for the existence loop is now stale.",
  "comments": [
    {
      "file": "scripts/resume-block-card-rule-test.sh",
      "line": 121,
      "severity": "nit",
      "blocking": false,
      "blocking_reason": "",
      "message": "The header rationale at lines 45-50 is now stale. It says `restated_markers` makes an absent or renamed consumer 'report nothing' (so the absence half would compare \"\" to \"\" and pass) and that 'a missing home makes `missing_markers` report every literal'. With this guard, both helpers emit a single `UNREADABLE: <file>` line instead, so an absent consumer now fails assertion 2 and an absent home fails assertion 1 for a different reason than the one documented. The top-level `[ -f ]` loop is still worth keeping (it exits 2 with a clear message rather than failing an assertion), but the stated justification should be updated to match the new behaviour — this file's whole contract is that its header does not claim more than the check delivers. (rule: `changelog/conventional-prefix-required` does not apply here; this is a free-form documentation nit.)"
    }
  ],
  "concerns_addressed": [
    {
      "concern": "correctness: `[ -f \"$root/$file\" ]` guard redefines readable as regular file; confirm intended for all four call sites and that a symlink to a regular file still passes.",
      "disposition": "not-an-issue",
      "detail": "Examined all four call sites (resume-block-card missing_markers/restated_markers, checkpoint-state absent_lines/present_lines); every input is a repo markdown file or a throwaway copy created with printf/awk, never a FIFO or device. `test -f` follows symlinks, so a symlinked regular file still passes; only directories/FIFOs/devices now short-circuit to UNREADABLE, which is exactly the intended classification and matches the pre-existing GNU behaviour (exit 2)."
    },
    {
      "concern": "correctness: guard only covers non-regular files; a regular file with unreadable permissions still reaches grep, so the fix still depends on BusyBox grep exiting 2 (not 1) on a permission error.",
      "disposition": "not-an-issue",
      "detail": "BusyBox grep returns 2 on an fopen/permission failure (it only degrades to 1 when open succeeds and the read loop yields nothing, which is the directory case); the helper's `*)` arm already maps any non-0/non-1 status to UNREADABLE, and the comment at lines 89-93 documents the measured 0/1/2+ split. Additionally the test deliberately uses a directory rather than chmod-000 because chmod is bypassed when the suite runs as root, so the permission path is not the exercised surface."
    },
    {
      "concern": "correctness: assertions 2, 3 and 4b in checkpoint-state-rule-test.sh still call grep directly and bypass the new helper guard.",
      "disposition": "not-an-issue",
      "detail": "Verified each: assertion 2 compares `grep -cF` output against \"3\" (a directory yields \"\" under GNU exit 2 and \"0\" under BusyBox exit 1 — both fail), and assertions 3/4b require `$?` == \"0\" (a directory yields 2 under GNU and 1 under BusyBox — both fail). No path exists where a non-regular or unreadable file makes these assertions pass. The real files they read are additionally covered by the top-level `[ -f ]` loop at lines 52-54."
    },
    {
      "concern": "tests: self-check 5 in resume-block-card-rule-test.sh is the only case exercising the new guard; missing-file and directory collapse into the same early return.",
      "disposition": "not-an-issue",
      "detail": "Both a missing file and a directory fail the same `[ -f ]` expression and take the identical single-line branch, so one falsification exercises the whole branch; a second case would assert the same expression with a different stat result. The missing-file surface the header names is the pre-existing top-level existence loop (lines 51-53), which still fails closed with exit 2."
    },
    {
      "concern": "tests: self-check 7 in checkpoint-state-rule-test.sh is the only coverage of the new guard; same missing-file vs directory collapse.",
      "disposition": "not-an-issue",
      "detail": "Same reasoning as the resume-block script: the guard is one `[ -f ]` test with one UNREADABLE return, and self-check 7 exercises it; the missing-file case is covered by the existence loop at lines 52-54."
    },
    {
      "concern": "tests: no CI, Makefile or Dockerfile change accompanies the fix, so Alpine/BusyBox behaviour is verified only manually; check whether `make precommit` runs in the github-update-go-agent image in CI.",
      "disposition": "not-an-issue",
      "detail": "This repo's only CI (.github/workflows/ci.yml) runs `make precommit` on ubuntu-latest, i.e. GNU grep; the github-update-go agent lives in the maintainer repo, so its Alpine image cannot be wired from here. That is acceptable because the fix removes the dependency on grep's directory exit status entirely for these helpers — the only remaining grep-status assumption is the permission-error path, which is a POSIX-guaranteed exit 2 and is unreachable in the root-run suite. I also grepped scripts/ for other directory-to-grep UNREADABLE checks (only these two files match), so the BusyBox failure class is fully covered."
    }
  ]
}