test: pin the review directive's VERDICT/===BODY=== contract to review_parse_response

igor commented

2026-07-30 18:03:59 +00:00

Collaborator

What this PR does

test: pin the review directive's VERDICT/===BODY=== contract to review_parse_response
Add bin/test-review-directive.sh, asserting only the handshake between bin/lib/review-directive.md and review_parse_response in bin/tick.sh: every verdict token the directive advertises round-trips through the real parser (lifted via sed, not reimplemented), undocumented tokens are rejected, a missing ===BODY=== sentinel or empty body is rejected, and each advertised verdict has a rubric entry in the directive

Test plan

make test passes, including the new suite (bin/test-review-directive.sh: all checks passed)
make shellcheck passes clean on the new file
No manual verification needed beyond the above; this is a self-contained shell test with no external dependencies besides jq (guarded skip-safe per bin/check-sync.sh's contract)

Closes #457

## What this PR does - [x] test: pin the review directive's VERDICT/===BODY=== contract to review_parse_response - [x] Add `bin/test-review-directive.sh`, asserting only the handshake between `bin/lib/review-directive.md` and `review_parse_response` in `bin/tick.sh`: every verdict token the directive advertises round-trips through the real parser (lifted via `sed`, not reimplemented), undocumented tokens are rejected, a missing `===BODY===` sentinel or empty body is rejected, and each advertised verdict has a rubric entry in the directive ## Test plan - [x] `make test` passes, including the new suite (`bin/test-review-directive.sh: all checks passed`) - [x] `make shellcheck` passes clean on the new file - [x] No manual verification needed beyond the above; this is a self-contained shell test with no external dependencies besides `jq` (guarded skip-safe per `bin/check-sync.sh`'s contract) Closes #457

igor added 1 commit

2026-07-30 18:03:59 +00:00

test: pin the review directive's VERDICT/===BODY=== contract to review_parse_response

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

Details

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

Details

eb26224d58

igor commented

2026-07-30 18:10:39 +00:00

Author

Collaborator

🤖 Review — `REQUEST_CHANGES` (automated)

CI for eb26224d: success

Adds a single new test (bin/test-review-directive.sh, 113 lines) pinning the VERDICT:/===BODY=== handshake between bin/lib/review-directive.md and review_parse_response in bin/tick.sh. Scope, framing, and checklist all match the diff — the one blocker is that I can't confirm the new suite actually runs under make test.

Blocking

bin/test-review-directive.sh is added with mode 100644 (see the new file mode line). Every other script under bin/ in this repo is executable (make test invokes bin/check-sync.sh directly; check-sync itself hard-fails helpers that "exist but are not executable"). The portion of check-sync.sh that discovers bin/test-*.sh is truncated in what I was given, so there are two possibilities and neither is good enough to approve on:
- the runner invokes "$t" directly → the new suite can't execute, and the description's claim make test passes, including the new suite (bin/test-review-directive.sh: all checks passed) isn't what CI proved; or
- the runner skips non-executable matches → the test is silently dead in CI, which is the worst outcome for a regression guard whose whole purpose is to catch a silent break.
Green CI does not distinguish these from "runner uses bash "$t" and the mode is merely inconsistent". Fix: chmod +x bin/test-review-directive.sh so the diff shows 100755, and (if you want to close the loop for the reviewer) paste the check-sync.sh line that dispatches bin/test-*.sh.

Non-blocking observations

The parser is lifted with sed -n '/^review_parse_response() {$/,/^}$/p' and eval'd — correct approach per the issue, but it silently assumes review_parse_response calls no other tick.sh helper. If that ever changes, the failure will surface as "parser REJECTED a verdict the directive advertises", i.e. a misleading message. A one-line command -v / declare -F sanity note in the failure text would age better.
The rubric assertion greps the literal - \*\*VERDICT\*\* --. A cosmetic reformat of the directive (em dash instead of --, or a different bullet) fails the suite with "parser accepts a verdict the directive never explains", which isn't what happened. Acceptable brittleness for a contract test, but worth knowing.
eq "the format line lists three tokens" "3" ... hardcodes the count that the round-trip loop already derives from the file. A legitimate fourth verdict would fail here and in the round-trip loop; that's presumably intended, just noisy.
" ${ADVERTISED[*]} " under set -u errors on bash < 4.4 if the VERDICT: line ever fails to parse (empty array). Low risk given CI's bash, but the empty-array path also silently continues every undocumented-token check, so a malformed directive could turn that whole block into a no-op.

Coverage — the four properties the issue asked for (advertised tokens round-trip, undocumented tokens rejected, missing sentinel rejected, empty body rejected) are all present, plus the inverse rubric-entry check scoped to the ## Verdict rubric section. The jq guard matches the skip-safety contract. No security surface, no CI-config changes, no unrelated edits.

_{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 `eb26224d`: **success** Adds a single new test (`bin/test-review-directive.sh`, 113 lines) pinning the `VERDICT:`/`===BODY===` handshake between `bin/lib/review-directive.md` and `review_parse_response` in `bin/tick.sh`. Scope, framing, and checklist all match the diff — the one blocker is that I can't confirm the new suite actually runs under `make test`. **Blocking** - `bin/test-review-directive.sh` is added with mode **100644** (see the `new file mode` line). Every other script under `bin/` in this repo is executable (`make test` invokes `bin/check-sync.sh` directly; check-sync itself hard-fails helpers that "exist but are not executable"). The portion of `check-sync.sh` that discovers `bin/test-*.sh` is truncated in what I was given, so there are two possibilities and neither is good enough to approve on: - the runner invokes `"$t"` directly → the new suite can't execute, and the description's claim `make test passes, including the new suite (bin/test-review-directive.sh: all checks passed)` isn't what CI proved; or - the runner skips non-executable matches → the test is silently dead in CI, which is the worst outcome for a regression guard whose whole purpose is to catch a silent break. Green CI does not distinguish these from "runner uses `bash "$t"` and the mode is merely inconsistent". Fix: `chmod +x bin/test-review-directive.sh` so the diff shows `100755`, and (if you want to close the loop for the reviewer) paste the `check-sync.sh` line that dispatches `bin/test-*.sh`. **Non-blocking observations** - The parser is lifted with `sed -n '/^review_parse_response() {$/,/^}$/p'` and `eval`'d — correct approach per the issue, but it silently assumes `review_parse_response` calls no other `tick.sh` helper. If that ever changes, the failure will surface as "parser REJECTED a verdict the directive advertises", i.e. a misleading message. A one-line `command -v` / `declare -F` sanity note in the failure text would age better. - The rubric assertion greps the literal `- \*\*VERDICT\*\* --`. A cosmetic reformat of the directive (em dash instead of `--`, or a different bullet) fails the suite with "parser accepts a verdict the directive never explains", which isn't what happened. Acceptable brittleness for a contract test, but worth knowing. - `eq "the format line lists three tokens" "3" ...` hardcodes the count that the round-trip loop already derives from the file. A legitimate fourth verdict would fail here *and* in the round-trip loop; that's presumably intended, just noisy. - `" ${ADVERTISED[*]} "` under `set -u` errors on bash < 4.4 if the `VERDICT:` line ever fails to parse (empty array). Low risk given CI's bash, but the empty-array path also silently `continue`s every undocumented-token check, so a malformed directive could turn that whole block into a no-op. **Coverage** — the four properties the issue asked for (advertised tokens round-trip, undocumented tokens rejected, missing sentinel rejected, empty body rejected) are all present, plus the inverse rubric-entry check scoped to the `## Verdict rubric` section. The `jq` guard matches the skip-safety contract. No security surface, no CI-config changes, no unrelated edits. </BODY> --- <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-30 18:10:39 +00:00

1 minute 17 seconds

igor self-assigned this

2026-07-30 18:10:40 +00:00

igor commented

2026-07-30 18:17:34 +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-30 18:17:52 +00:00

igor added spent time

2026-07-30 18:17:52 +00:00

5 minutes 59 seconds

igor added 1 commit

2026-07-30 18:17:53 +00:00

test: guard vacuous verdict checks and note helper lifts

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

Details

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

Details

4523d6003a

igor commented

2026-07-30 18:20:04 +00:00

Author

Collaborator

🤖 Review — `APPROVE` (automated)

CI for 4523d600: success

Adds one new test file (bin/test-review-directive.sh, 139 lines) pinning the VERDICT:/===BODY=== handshake between bin/lib/review-directive.md and review_parse_response in bin/tick.sh. In scope for #457, test-only, CI green — approving.

What I verified

Scope/size: single new file, no drive-bys, no CI-config or production-code changes. Nothing was deleted to fit a budget.
Checklist honesty: every checked box maps to something in the diff — the sed-lifted parser (line 39), the advertised-token round-trip loop, the undocumented-token rejection loop, the missing-sentinel/empty-body cases, and the rubric-entry check. The jq skip guard the issue demanded is present at the top and correctly justified (the lifted parser ends in jq -n, so absent jq would report a phantom mismatch), satisfying check-sync.sh's skip-safe contract.
No reimplementation: the parser is lifted with sed -n '/^review_parse_response() {$/,/^}$/p' and eval'd, which is the one property the issue said the test exists to preserve.
Fail-loud, not fail-vacuous: the empty-ADVERTISED guard is the right call — without it the round-trip loop would iterate zero times and the undocumented-token loop would continue on every candidate, passing the whole section vacuously. Same for the extraction failure of PARSER_SRC (bad → non-zero exit).
Rubric check is section-scoped via the awk range on ## Verdict rubric, which is what stops a whole-file grep from passing on a deleted entry. The loose bullet/format regex is a deliberate and reasonable tolerance.
Security: none. eval is on text read from a repo-local file already trusted as the executable under test.

Non-blocking notes (no action required)

eq "the format line lists three tokens" "3" "${#ADVERTISED[@]}" hard-codes the count, so a legitimate fourth verdict added to both directive and parser will still fail here. That's a defensible change-detector, but it means the test must be edited alongside any verdict-set change — worth a one-line comment saying so.
The LIFT_NOTE scan (_tick_fns loop) matches only name() {-style definitions and does a loose word-grep of the parser body, so it can both miss function name { definitions and produce a spurious name if the parser's comments happen to contain a helper's name. It only decorates a failure message, so the blast radius is zero.
Cosmetic: the bad branch for undocumented verdicts prints '$v' while the ok branch prints '${v:-<empty>}' — the empty-token failure would render as undocumented verdict '' was accepted.

Coverage note: I can't see review_parse_response itself in this diff, so the assertions about whitespace-only-body and missing-sentinel rejection are taken on the green CI signal rather than read off the source. That's exactly the coupling the test is meant to enforce, so it's the right dependency direction.

_{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 — `APPROVE` _(automated)_ CI for `4523d600`: **success** Adds one new test file (`bin/test-review-directive.sh`, 139 lines) pinning the `VERDICT:`/`===BODY===` handshake between `bin/lib/review-directive.md` and `review_parse_response` in `bin/tick.sh`. In scope for #457, test-only, CI green — approving. **What I verified** - **Scope/size:** single new file, no drive-bys, no CI-config or production-code changes. Nothing was deleted to fit a budget. - **Checklist honesty:** every checked box maps to something in the diff — the `sed`-lifted parser (line 39), the advertised-token round-trip loop, the undocumented-token rejection loop, the missing-sentinel/empty-body cases, and the rubric-entry check. The `jq` skip guard the issue demanded is present at the top and correctly justified (the lifted parser ends in `jq -n`, so absent `jq` would report a phantom mismatch), satisfying `check-sync.sh`'s skip-safe contract. - **No reimplementation:** the parser is lifted with `sed -n '/^review_parse_response() {$/,/^}$/p'` and `eval`'d, which is the one property the issue said the test exists to preserve. - **Fail-loud, not fail-vacuous:** the empty-`ADVERTISED` guard is the right call — without it the round-trip loop would iterate zero times and the undocumented-token loop would `continue` on every candidate, passing the whole section vacuously. Same for the extraction failure of `PARSER_SRC` (bad → non-zero exit). - **Rubric check is section-scoped** via the `awk` range on `## Verdict rubric`, which is what stops a whole-file `grep` from passing on a deleted entry. The loose bullet/format regex is a deliberate and reasonable tolerance. - **Security:** none. `eval` is on text read from a repo-local file already trusted as the executable under test. **Non-blocking notes (no action required)** - `eq "the format line lists three tokens" "3" "${#ADVERTISED[@]}"` hard-codes the count, so a legitimate fourth verdict added to *both* directive and parser will still fail here. That's a defensible change-detector, but it means the test must be edited alongside any verdict-set change — worth a one-line comment saying so. - The `LIFT_NOTE` scan (`_tick_fns` loop) matches only `name() {`-style definitions and does a loose word-grep of the parser body, so it can both miss `function name {` definitions and produce a spurious name if the parser's comments happen to contain a helper's name. It only decorates a failure message, so the blast radius is zero. - Cosmetic: the `bad` branch for undocumented verdicts prints `'$v'` while the `ok` branch prints `'${v:-<empty>}'` — the empty-token failure would render as `undocumented verdict '' was accepted`. **Coverage note:** I can't see `review_parse_response` itself in this diff, so the assertions about whitespace-only-body and missing-sentinel rejection are taken on the green CI signal rather than read off the source. That's exactly the coupling the test is meant to enforce, so it's the right dependency direction. --- <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-30 18:20:04 +00:00

1 minute 19 seconds