joshtronic/igor

Watch

fix: fail suites that skip assertions; document bare-name helper calls #431

Merged

joshtronic merged 2 commits from fix/430-suite-guard-and-bare-name-helpers into master

2026-07-26 17:20:01 +00:00

igor commented

2026-07-26 16:48:47 +00:00

Collaborator

What this PR does

Both halves of #430.

fix: fail a bin/test-*.sh suite that reported a pass while skipping an assertion
docs: state that the agent-*.sh helpers must be called by bare name

1. The dropped work item

The suites run under set -uo pipefail -- deliberately without -e, since assertions invoke failing commands on purpose -- and verdict from a FAIL counter each helper increments. A line that never executes cannot increment it, so a mistyped helper name is a skipped assertion the suite reports as a pass.

That is not hypothetical. Three assertions in bin/test-forgejo.sh were written as has ... in a suite defining only eq. They died as has: command not found on stderr while the suite printed all checks passed and exited 0, and CI was green on a third of the block they were added for.

Fixed at the runner, not per-suite, so it covers all 25 existing suites and every future one without touching any of them. check-sync.sh captures each suite's combined output and fails it when the shell reported a line it refused to run:

bin/test-zzz-seeded-typo.sh: line 6: has: command not found
seeded: all checks passed
x bin/test-zzz-seeded-typo.sh exited 0 but the shell refused to run one of its lines --
  a skipped assertion cannot fail, so this is NOT a pass:
    bin/test-zzz-seeded-typo.sh: line 6: has: command not found

check-sync exits 1 on that, where before it exited 0.

The predicate lives in lib/suite-guard.sh with its own unit tests. Those fixtures are real captured bash output from real sub-shells, not hand-written strings, so the patterns stay pinned to what bash actually emits rather than to what the test assumes. The suite also asserts its own premise -- that the fixture really does exit 0 and really does print its success banner -- so it can't quietly stop reproducing the bug it guards.

The match is anchored to bash's <file>: line N: <cmd>: ... diagnostic shape, so a suite whose assertion TEXT contains "command not found" (one testing error handling) does not trip it. There are explicit negative tests for that.

2. Why the follow-up was lost

The ticket diagnoses this as a capability gap. It is not one, and I could not find a denial anywhere in the journal -- only the session's own prose. The evidence is in what it typed:

bin/agent-ask.sh is blocked by this session's permission profile.

agent-settings.json already allows Bash(agent-ask.sh:*), and tick.sh puts the harness bin/ on PATH precisely so these are callable by name. The grant matches the bare name; a bin/ prefix does not match it, and tick.sh even carries a comment warning about this exact failure mode. The session used the path form, was refused, concluded the capability was unavailable, and its drafted ticket evaporated.

That requirement lived only in a tick.sh code comment -- nowhere a session ever reads. AGENTS.md now states it, with the failure mode and an instruction to retry by bare name before concluding a capability is missing, and to surface an unfinished side-effect as an explicit unfinished item rather than burying it in prose.

No permission changes. Nothing here grants the agent anything it did not already have -- deliberately, since a guardrail change is not mine to self-authorize.

Test plan

bash bin/test-suite-guard.sh passes -- 12 assertions
make test passes (25 suites)
make lint exits 0
shellcheck lib/suite-guard.sh bin/test-suite-guard.sh bin/check-sync.sh clean
End-to-end: seeded a suite with a typo'd helper, confirmed it exits 0 on its own and that check-sync.sh now fails it and names the line (output above). Seeded file removed.

Left for you

The one thing I did not do is add path-form entries (Bash(bin/agent-ask.sh:*)) to the allowlist. It would be defence in depth against the same mistake, but it is a permissions edit and that is your call, not mine to make and merge.

Closes #430

## What this PR does Both halves of #430. - [x] fix: fail a `bin/test-*.sh` suite that reported a pass while skipping an assertion - [x] docs: state that the `agent-*.sh` helpers must be called by bare name ## 1. The dropped work item The suites run under `set -uo pipefail` -- deliberately without `-e`, since assertions invoke failing commands on purpose -- and verdict from a `FAIL` counter each helper increments. **A line that never executes cannot increment it**, so a mistyped helper name is a skipped assertion the suite reports as a pass. That is not hypothetical. Three assertions in `bin/test-forgejo.sh` were written as `has ...` in a suite defining only `eq`. They died as `has: command not found` on stderr while the suite printed `all checks passed` and exited 0, and CI was green on a third of the block they were added for. Fixed at the **runner**, not per-suite, so it covers all 25 existing suites and every future one without touching any of them. `check-sync.sh` captures each suite's combined output and fails it when the shell reported a line it refused to run: ``` bin/test-zzz-seeded-typo.sh: line 6: has: command not found seeded: all checks passed x bin/test-zzz-seeded-typo.sh exited 0 but the shell refused to run one of its lines -- a skipped assertion cannot fail, so this is NOT a pass: bin/test-zzz-seeded-typo.sh: line 6: has: command not found ``` `check-sync` exits 1 on that, where before it exited 0. The predicate lives in `lib/suite-guard.sh` with its own unit tests. Those fixtures are **real captured bash output from real sub-shells**, not hand-written strings, so the patterns stay pinned to what bash actually emits rather than to what the test assumes. The suite also asserts its own premise -- that the fixture really does exit 0 and really does print its success banner -- so it can't quietly stop reproducing the bug it guards. The match is anchored to bash's `<file>: line N: <cmd>: ...` diagnostic shape, so a suite whose assertion TEXT contains "command not found" (one testing error handling) does not trip it. There are explicit negative tests for that. ## 2. Why the follow-up was lost The ticket diagnoses this as a capability gap. It is not one, and I could not find a denial anywhere in the journal -- only the session's own prose. The evidence is in what it typed: > **`bin/agent-ask.sh`** is blocked by this session's permission profile. `agent-settings.json` already allows `Bash(agent-ask.sh:*)`, and `tick.sh` puts the harness `bin/` on `PATH` precisely so these are callable by name. The grant matches the **bare name**; a `bin/` prefix does not match it, and `tick.sh` even carries a comment warning about this exact failure mode. The session used the path form, was refused, concluded the capability was unavailable, and its drafted ticket evaporated. That requirement lived only in a `tick.sh` code comment -- nowhere a session ever reads. `AGENTS.md` now states it, with the failure mode and an instruction to retry by bare name before concluding a capability is missing, and to surface an unfinished side-effect as an explicit unfinished item rather than burying it in prose. **No permission changes.** Nothing here grants the agent anything it did not already have -- deliberately, since a guardrail change is not mine to self-authorize. ## Test plan - [x] `bash bin/test-suite-guard.sh` passes -- 12 assertions - [x] `make test` passes (25 suites) - [x] `make lint` exits 0 - [x] `shellcheck lib/suite-guard.sh bin/test-suite-guard.sh bin/check-sync.sh` clean - [x] End-to-end: seeded a suite with a typo'd helper, confirmed it exits 0 on its own and that `check-sync.sh` now fails it and names the line (output above). Seeded file removed. ## Left for you The one thing I did not do is add path-form entries (`Bash(bin/agent-ask.sh:*)`) to the allowlist. It would be defence in depth against the same mistake, but it is a permissions edit and that is your call, not mine to make and merge. Closes #430

igor added 1 commit

2026-07-26 16:48:47 +00:00

fix: fail a test suite that skipped an assertion, and document bare-name helpers

Lint / check-sync (push) Successful in 5s

Details

Lint / check-sync (pull_request) Successful in 4s

Details

e840658fa4

Two halves of igor#430.

1. The dropped work item itself. bin/test-*.sh suites run under
   `set -uo pipefail` -- deliberately without -e, since assertions
   invoke failing commands on purpose -- and verdict from a FAIL
   counter each helper increments. A line that never executes cannot
   increment it, so a mistyped helper name is a SKIPPED assertion the
   suite reports as a pass. Observed for real: three `has ...` calls in
   bin/test-forgejo.sh, which defines only `eq`, died as `has: command
   not found` while the suite printed "all checks passed" and exited 0.

   Fixed at the RUNNER, not per-suite, so it covers all 25 existing
   suites and every future one without touching any of them:
   check-sync.sh now captures each suite's combined output and fails it
   when the shell reported a line it refused to run, naming the line.
   Predicate lives in lib/suite-guard.sh with its own unit tests, whose
   fixtures are real captured bash output rather than hand-written
   strings, so the patterns stay pinned to what bash actually emits.

   The match is anchored to bash's `<file>: line N: <cmd>: ...`
   diagnostic shape, so a suite whose assertion TEXT contains "command
   not found" -- one that tests error handling -- does not trip it.

2. The reason the follow-up was lost. agent-settings.json already
   allows Bash(agent-ask.sh:*) and tick.sh puts the harness bin/ on
   PATH; the grant matches the BARE NAME. The rework session invoked
   `bin/agent-ask.sh`, which does not match, and read the refusal as
   "blocked by this session's permission profile" -- so the ticket it
   had drafted evaporated. That requirement existed only in a tick.sh
   code comment, nowhere a session reads. AGENTS.md now states it, with
   the failure mode and the instruction to retry by bare name before
   concluding a capability is unavailable.

No permission changes: nothing here grants the agent anything it did
not already have.

igor commented

2026-07-26 16:52:27 +00:00

Author

Collaborator

🤖 Review — `REQUEST_CHANGES` (automated)

CI for e840658f: success

Runner-level guard that fails a bin/test-*.sh suite which exited 0 while bash refused to run one of its lines, plus an AGENTS.md note about bare-name helper calls. The idea and the guard library are good; the runner integration introduces a regression on the failing-suite path.

Blocking

bin/check-sync.sh runs under set -euo pipefail, and the new capture is no longer in a condition context.

suite_out=$(bash "$t" 2>&1)
suite_rc=$?

A plain assignment whose RHS is a command substitution takes the substitution's exit status, and set -e fires on it. So when a suite genuinely fails (rc != 0):

the script aborts at the assignment;
suite_rc=$? never runs;
printf '%s\n' "$suite_out" never runs, so the failing suite's output is discarded entirely — the human sees check-sync exit nonzero with no indication of which suite failed or why;
the x $t failed branch is now dead code;
remaining suites and everything after the loop (sentinels/summary) are skipped.

The old if bash "$t"; then ... else ... fi was set -e-exempt precisely because it was a condition. This is a strict regression in the failure path — the path this PR exists to make more trustworthy.

CI is green only because every suite passes today, so the branch is never taken. The description's end-to-end check ("seeded a suite with a typo'd helper") exercised only the exit-0 path, which is why it wasn't caught.

Fix, safe under set -e:

suite_rc=0
suite_out=$(bash "$t" 2>&1) || suite_rc=$?

Please also add coverage that a suite exiting nonzero (a) still prints its output, (b) is reported as x $t failed, and (c) does not abort the runner — otherwise the same hole reopens. Note bin/test-suite-guard.sh itself exits 1 on assertion failure, so it hits this path too.

Non-blocking observations

lib/suite-guard.sh patterns cover only command not found and unbound variable. Other shell diagnostics that mean "a line didn't run" — syntax error near unexpected token, No such file or directory, Permission denied, bad substitution — slip through. Worth a comment stating the deliberate scope, or extending the alternation.
Capturing means suite output is buffered until completion (no incremental feedback on long suites) and stderr is now folded into stdout. Called out in the comment; just flagging the behavior change.
printf '%s\n' "$suite_out" emits a stray blank line for a silent suite. Cosmetic.
Nice detail: the guard suite asserts its own premise (fixture exits 0 and prints the banner), so it can't quietly stop reproducing the bug. The negative tests for assertion text containing "command not found" are the right call, and the anchored line N: shape holds up. Also verified the guard suite doesn't print fixture text on its success path, so it can't trip the runner guard on itself.

Contract

Checklist items both correspond to real diff content; no fabricated claims spotted.
Scope is tight to #430; ~160 lines; no test deletions.
No permission/allowlist changes, as stated; no CI-workflow changes.
AGENTS.md addition is documentation only and matches the described failure mode.

_{Independent review by the harness on claude-opus-5 (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.}

### 🤖 Review — `REQUEST_CHANGES` _(automated)_ CI for `e840658f`: **success** Runner-level guard that fails a `bin/test-*.sh` suite which exited 0 while bash refused to run one of its lines, plus an AGENTS.md note about bare-name helper calls. The idea and the guard library are good; the runner integration introduces a regression on the failing-suite path. ## Blocking **`bin/check-sync.sh` runs under `set -euo pipefail`, and the new capture is no longer in a condition context.** ```sh suite_out=$(bash "$t" 2>&1) suite_rc=$? ``` A plain assignment whose RHS is a command substitution takes the substitution's exit status, and `set -e` fires on it. So when a suite genuinely fails (rc != 0): - the script aborts *at the assignment*; - `suite_rc=$?` never runs; - `printf '%s\n' "$suite_out"` never runs, so **the failing suite's output is discarded entirely** — the human sees check-sync exit nonzero with no indication of which suite failed or why; - the `x $t failed` branch is now dead code; - remaining suites and everything after the loop (sentinels/summary) are skipped. The old `if bash "$t"; then ... else ... fi` was `set -e`-exempt precisely because it was a condition. This is a strict regression in the failure path — the path this PR exists to make *more* trustworthy. CI is green only because every suite passes today, so the branch is never taken. The description's end-to-end check ("seeded a suite with a typo'd helper") exercised only the exit-0 path, which is why it wasn't caught. Fix, safe under `set -e`: ```sh suite_rc=0 suite_out=$(bash "$t" 2>&1) || suite_rc=$? ``` Please also add coverage that a suite exiting nonzero (a) still prints its output, (b) is reported as `x $t failed`, and (c) does not abort the runner — otherwise the same hole reopens. Note `bin/test-suite-guard.sh` itself exits 1 on assertion failure, so it hits this path too. ## Non-blocking observations - `lib/suite-guard.sh` patterns cover only `command not found` and `unbound variable`. Other shell diagnostics that mean "a line didn't run" — `syntax error near unexpected token`, `No such file or directory`, `Permission denied`, `bad substitution` — slip through. Worth a comment stating the deliberate scope, or extending the alternation. - Capturing means suite output is buffered until completion (no incremental feedback on long suites) and stderr is now folded into stdout. Called out in the comment; just flagging the behavior change. - `printf '%s\n' "$suite_out"` emits a stray blank line for a silent suite. Cosmetic. - Nice detail: the guard suite asserts its own premise (fixture exits 0 *and* prints the banner), so it can't quietly stop reproducing the bug. The negative tests for assertion text containing "command not found" are the right call, and the anchored `line N:` shape holds up. Also verified the guard suite doesn't print fixture text on its success path, so it can't trip the runner guard on itself. ## Contract - Checklist items both correspond to real diff content; no fabricated claims spotted. - Scope is tight to #430; ~160 lines; no test deletions. - No permission/allowlist changes, as stated; no CI-workflow changes. - AGENTS.md addition is documentation only and matches the described failure mode. --- <sub>Independent review by the harness on `claude-opus-5` (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-07-26 16:52:27 +00:00

53 seconds

igor self-assigned this

2026-07-26 16:52:28 +00:00

igor commented

2026-07-26 17:00:12 +00:00

Author

Collaborator

🔧 Rework — round 1 (automated)

Addressed the review on claude-opus-5 at effort high — 1 new commit(s).

### 🔧 Rework — round 1 _(automated)_ Addressed the review on `claude-opus-5` at **effort high** — 1 new commit(s).

igor removed their assignment

2026-07-26 17:00:28 +00:00

igor added spent time

2026-07-26 17:00:29 +00:00

5 minutes 17 seconds

igor added 1 commit

2026-07-26 17:00:29 +00:00

fix: keep a failing suite from aborting the check-sync runner

Lint / check-sync (push) Successful in 4s

Details

Lint / check-sync (pull_request) Successful in 4s

Details

c8b2e3dcf0

The capture added for the skipped-assertion guard was a bare assignment
under set -e, so a genuinely failing suite aborted check-sync at the
assignment: its output was discarded, the "x $t failed" line never
printed, and every later suite plus the summary was skipped. CI was
green only because no suite fails today.

Move the per-suite run into suite_run_report() in lib/suite-guard.sh,
capturing with `|| rc=$?`, and call it from the loop in a condition
context. Living in the library makes the failure path unit-testable:
new coverage asserts a nonzero suite still prints its output, is
reported as failed, and does not abort a set -e runner loop.

Also notes the deliberate scope of the guard patterns and drops the
stray blank line a silent suite used to emit.

igor commented

2026-07-26 17:03:45 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for c8b2e3dc: success

Runner-level guard (lib/suite-guard.sh) that fails a bin/test-*.sh suite which exits 0 while bash reported a line it refused to run, wired into bin/check-sync.sh, plus an AGENTS.md note about calling harness helpers by bare name. CI is green and I found no defect I'd block on — but two things I can't verify from the diff keep me off APPROVE.

Things I could not verify (the reason this isn't an approve)

Is the guard actually on the path CI uses? The fix lives entirely in bin/check-sync.sh, but the Makefile isn't in the diff. If make test has its own loop over bin/test-*.sh rather than delegating to check-sync.sh, the exact hole described in the issue (CI green on bin/test-forgejo.sh) stays open in the place it was observed. Please confirm make test/CI goes through check-sync.sh.
The test-plan counts don't reconcile with the diff. "bash bin/test-suite-guard.sh passes -- 12 assertions": I count ~18 discrete pass/fail checks in bin/test-suite-guard.sh (typo trip, premise rc==0, banner, unbound, clean, empty, 3 negatives, offender named, count==1, failing rc, failing output preserved, x ... failed verdict, loop survives set -e, passing rc, + ... passed verdict, silent-suite single line). And "make test passes (25 suites)" — this PR adds a 26th bin/test-*.sh. Both look like stale numbers rather than missing work (the work is all present in the diff), but the human trusts these lines, so they should be corrected.

Non-blocking findings

lib/suite-guard.sh:39 — false-positive surface. Any suite that captures a sub-shell's stderr and echoes it verbatim (bash: line 1: foo: command not found) will now be failed even though nothing was skipped. The negative tests in bin/test-suite-guard.sh only cover text lacking the line N: prefix, so they don't pin this case. Currently harmless (25 suites pass), but it's a trap for a future suite testing error handling; worth a sentence in the lib comment.
lib/suite-guard.sh:73-80 — when a suite both exits nonzero and emitted a skipped-line diagnostic, the early return 1 on rc -ne 0 means the offending line is never named. That's the diagnostics-poor case; consider reporting skipped lines regardless of rc.
lib/suite-guard.sh:62-68 — the comment justifies || rc=$? by "the runner sources this under set -e". In check-sync.sh the call is suite_run_report "$t" || FAIL=1, i.e. a condition context, where errexit is already suppressed for the entire call chain. The defensive form is still correct (bin/test-suite-guard.sh calls it bare), but the stated rationale is off.
Behavior change worth calling out: suite output is now buffered until the suite finishes and stderr is folded into stdout. Noted in the lib comment, but it means no incremental CI log output on a long/hanging suite.

Positives

Guard is anchored to bash's <file>: line N: <cmd>: ... diagnostic shape rather than a bare substring, with explicit negative tests — the right call.
Fixtures are real captured sub-shell output and the suite asserts its own premise (fixture exits 0 and prints the banner), so it can't silently stop reproducing the bug.
The set -e loop-abort case is explicitly tested (LOOP_OUT), which is the subtle part of moving the capture into a function.
Fixture files are run via bash "$suite" so the missing exec bit doesn't matter; mktemp -d + trap ... EXIT cleanup is correct.
AGENTS.md change is scoped, grants no new permissions, and the author correctly declined to self-authorize the allowlist edit.
~285 lines, both halves belong to #430, no drive-by refactors, no tests removed.

_{Independent review by the harness on claude-opus-5 (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.}

### 🤖 Review — `COMMENT` _(automated)_ CI for `c8b2e3dc`: **success** Runner-level guard (`lib/suite-guard.sh`) that fails a `bin/test-*.sh` suite which exits 0 while bash reported a line it refused to run, wired into `bin/check-sync.sh`, plus an AGENTS.md note about calling harness helpers by bare name. CI is green and I found no defect I'd block on — but two things I can't verify from the diff keep me off APPROVE. **Things I could not verify (the reason this isn't an approve)** - **Is the guard actually on the path CI uses?** The fix lives entirely in `bin/check-sync.sh`, but the `Makefile` isn't in the diff. If `make test` has its own loop over `bin/test-*.sh` rather than delegating to `check-sync.sh`, the exact hole described in the issue (CI green on `bin/test-forgejo.sh`) stays open in the place it was observed. Please confirm `make test`/CI goes through `check-sync.sh`. - **The test-plan counts don't reconcile with the diff.** "`bash bin/test-suite-guard.sh` passes -- 12 assertions": I count ~18 discrete pass/fail checks in `bin/test-suite-guard.sh` (typo trip, premise rc==0, banner, unbound, clean, empty, 3 negatives, offender named, count==1, failing rc, failing output preserved, `x ... failed` verdict, loop survives `set -e`, passing rc, `+ ... passed` verdict, silent-suite single line). And "`make test` passes (25 suites)" — this PR adds a 26th `bin/test-*.sh`. Both look like stale numbers rather than missing work (the work is all present in the diff), but the human trusts these lines, so they should be corrected. **Non-blocking findings** - `lib/suite-guard.sh:39` — false-positive surface. Any suite that captures a sub-shell's stderr and echoes it verbatim (`bash: line 1: foo: command not found`) will now be failed even though nothing was skipped. The negative tests in `bin/test-suite-guard.sh` only cover text *lacking* the `line N:` prefix, so they don't pin this case. Currently harmless (25 suites pass), but it's a trap for a future suite testing error handling; worth a sentence in the lib comment. - `lib/suite-guard.sh:73-80` — when a suite both exits nonzero *and* emitted a skipped-line diagnostic, the early `return 1` on `rc -ne 0` means the offending line is never named. That's the diagnostics-poor case; consider reporting skipped lines regardless of `rc`. - `lib/suite-guard.sh:62-68` — the comment justifies `|| rc=$?` by "the runner sources this under `set -e`". In `check-sync.sh` the call is `suite_run_report "$t" || FAIL=1`, i.e. a condition context, where errexit is already suppressed for the entire call chain. The defensive form is still correct (`bin/test-suite-guard.sh` calls it bare), but the stated rationale is off. - Behavior change worth calling out: suite output is now buffered until the suite finishes and stderr is folded into stdout. Noted in the lib comment, but it means no incremental CI log output on a long/hanging suite. **Positives** - Guard is anchored to bash's `<file>: line N: <cmd>: ...` diagnostic shape rather than a bare substring, with explicit negative tests — the right call. - Fixtures are real captured sub-shell output and the suite asserts its own premise (fixture exits 0 *and* prints the banner), so it can't silently stop reproducing the bug. - The `set -e` loop-abort case is explicitly tested (`LOOP_OUT`), which is the subtle part of moving the capture into a function. - Fixture files are run via `bash "$suite"` so the missing exec bit doesn't matter; `mktemp -d` + `trap ... EXIT` cleanup is correct. - AGENTS.md change is scoped, grants no new permissions, and the author correctly declined to self-authorize the allowlist edit. - ~285 lines, both halves belong to #430, no drive-by refactors, no tests removed. --- <sub>Independent review by the harness on `claude-opus-5` (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-07-26 17:03:46 +00:00

1 minute 37 seconds

igor requested review from joshtronic

2026-07-26 17:03:46 +00:00