fix: accept READY as passing verdict in check-board-advance hook

forgejo_admin commented

2026-03-29 03:24:15 +00:00

Author

Contributor

PR #221 Review

DOMAIN REVIEW

Tech stack: Bash (shell scripting), jq, grep pipelines, Python mock HTTP server for tests.

Hook logic correctness:

The core change updates two grep pipelines in check_review_approved() from:

| grep -vi 'NOT APPROVED' | grep -qi 'APPROVED'

to:

| grep -vi 'NOT APPROVED' | grep -vi 'NOT READY' | grep -qiE 'APPROVED|READY'

This correctly handles all four verdict scenarios:

"APPROVED" -- passes (line survives both exclusion filters, matches final pattern)
"NOT APPROVED" -- blocked (line removed by first exclusion filter)
"READY" -- passes (line survives both exclusion filters, matches final pattern)
"NOT READY" -- blocked (line removed by second exclusion filter)

Both the body check (line 61-63 in diff) and blocks check (line 70-72 in diff) are updated identically. Deny messages in both update_board_item and bulk_move_board_items routes updated to reference "APPROVED or READY." Comments updated throughout. All changes are symmetric and consistent.

False positive risk (NIT, not a blocker): The word "ready" is common in natural language. The design spec template (2026-03-18-review-ticket-design.md line 140) includes the phrase "Specific actions needed before this ticket is READY" in the Recommendation section -- this appears in NEEDS_REFINEMENT verdicts. Since the grep is case-insensitive and line-based, this line would match READY and cause a false positive: a NEEDS_REFINEMENT review would be treated as passing.

This risk existed conceptually before this PR (any text containing "APPROVED" could false-positive, though that word is far less common in natural prose). The switch to "READY" makes it a tangible risk. However, this is a pre-existing design concern in the review note template, not a flaw introduced by this PR. A future improvement could anchor the grep to ## Verdict: (APPROVED|READY) to eliminate false positives from body text. Tracking as a nit for discovered scope.

Test coverage: 5 genuinely new test scenarios (Tests 20-24) plus the renumbered Test 25 (was old Test 20). Coverage matrix:

Scenario	Body	Blocks	bulk_move
READY (allow)	Test 20	Test 21	Test 24
NOT READY (deny)	Test 22	Test 23	--

The missing cell (NOT READY in bulk_move deny) is a minor gap but acceptable -- the check_review_approved() function is shared, so if NOT READY is blocked for single moves, it is blocked for bulk moves too. The existing Test 15 (bulk deny without review) already validates the bulk deny path.

BLOCKERS

None. The change is minimal, focused, and correct.

No new functionality without tests -- 5 new test cases directly cover the new behavior.
No unvalidated user input -- this hook only reads structured JSON from the Claude harness and pal-e-docs API.
No secrets or credentials in code.
No DRY violations -- the two grep pipelines (body and blocks) are intentionally parallel, not duplicated logic.

NITS

False positive risk from "READY" in prose: As noted above, anchoring the grep to the verdict line format (## Verdict:) would make the pattern more precise. Example: grep -iE '## Verdict:.*(APPROVED|READY)'. This is discovered scope -- file as a follow-up issue.
SKILL.md vocabulary drift: /home/ldraney/claude-custom/skills/review-ticket/SKILL.md still references "APPROVED" exclusively (lines 3, 9, 77, 97, 104, 105) while the design spec uses "READY." This PR correctly fixes the hook to accept both, but the skill documentation should also reference both vocabularies. Out of scope for this PR -- file separately.
PR body test count: Claims "6 new test cases (Tests 20-25)" and "27 tests (21 existing + 6 new)" but Test 25 is the renumbered old Test 20, not a new test. Actual new tests: 5 (Tests 20-24). Minor documentation inaccuracy.

SOP COMPLIANCE

Branch named after issue: 220-fix-review-vocab-mismatch matches issue #220
PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related Notes all present
Related references: Closes #220, board item 638, skill note reference
No secrets committed
No unnecessary file changes: only 2 files changed, both directly related to the fix
Commit messages are descriptive

PROCESS OBSERVATIONS

Change failure risk: Low. The change is additive (accepts more verdicts) and the exclusion logic for negated verdicts is correctly layered. Existing APPROVED paths are untouched by the new grep pattern.
Discovered scope: Two items should be tracked as follow-up issues: (1) anchor grep to verdict line format to prevent false positives from prose, (2) update SKILL.md vocabulary to reference both READY and APPROVED.
DORA impact: This fix unblocks the kanban pipeline -- tickets reviewed with READY verdict can now advance through the todo-to-next_up gate, restoring deployment frequency throughput.

VERDICT: APPROVED

## PR #221 Review ### DOMAIN REVIEW **Tech stack:** Bash (shell scripting), jq, grep pipelines, Python mock HTTP server for tests. **Hook logic correctness:** The core change updates two grep pipelines in `check_review_approved()` from: ```bash | grep -vi 'NOT APPROVED' | grep -qi 'APPROVED' ``` to: ```bash | grep -vi 'NOT APPROVED' | grep -vi 'NOT READY' | grep -qiE 'APPROVED|READY' ``` This correctly handles all four verdict scenarios: - "APPROVED" -- passes (line survives both exclusion filters, matches final pattern) - "NOT APPROVED" -- blocked (line removed by first exclusion filter) - "READY" -- passes (line survives both exclusion filters, matches final pattern) - "NOT READY" -- blocked (line removed by second exclusion filter) Both the body check (line 61-63 in diff) and blocks check (line 70-72 in diff) are updated identically. Deny messages in both `update_board_item` and `bulk_move_board_items` routes updated to reference "APPROVED or READY." Comments updated throughout. All changes are symmetric and consistent. **False positive risk (NIT, not a blocker):** The word "ready" is common in natural language. The design spec template (`2026-03-18-review-ticket-design.md` line 140) includes the phrase "Specific actions needed before this ticket is READY" in the Recommendation section -- this appears in NEEDS_REFINEMENT verdicts. Since the grep is case-insensitive and line-based, this line would match `READY` and cause a false positive: a NEEDS_REFINEMENT review would be treated as passing. This risk existed conceptually before this PR (any text containing "APPROVED" could false-positive, though that word is far less common in natural prose). The switch to "READY" makes it a tangible risk. However, this is a pre-existing design concern in the review note template, not a flaw introduced by this PR. A future improvement could anchor the grep to `## Verdict: (APPROVED|READY)` to eliminate false positives from body text. Tracking as a nit for discovered scope. **Test coverage:** 5 genuinely new test scenarios (Tests 20-24) plus the renumbered Test 25 (was old Test 20). Coverage matrix: | Scenario | Body | Blocks | bulk_move | |----------|------|--------|-----------| | READY (allow) | Test 20 | Test 21 | Test 24 | | NOT READY (deny) | Test 22 | Test 23 | -- | The missing cell (NOT READY in bulk_move deny) is a minor gap but acceptable -- the `check_review_approved()` function is shared, so if NOT READY is blocked for single moves, it is blocked for bulk moves too. The existing Test 15 (bulk deny without review) already validates the bulk deny path. ### BLOCKERS None. The change is minimal, focused, and correct. - No new functionality without tests -- 5 new test cases directly cover the new behavior. - No unvalidated user input -- this hook only reads structured JSON from the Claude harness and pal-e-docs API. - No secrets or credentials in code. - No DRY violations -- the two grep pipelines (body and blocks) are intentionally parallel, not duplicated logic. ### NITS 1. **False positive risk from "READY" in prose:** As noted above, anchoring the grep to the verdict line format (`## Verdict:`) would make the pattern more precise. Example: `grep -iE '## Verdict:.*(APPROVED|READY)'`. This is discovered scope -- file as a follow-up issue. 2. **SKILL.md vocabulary drift:** `/home/ldraney/claude-custom/skills/review-ticket/SKILL.md` still references "APPROVED" exclusively (lines 3, 9, 77, 97, 104, 105) while the design spec uses "READY." This PR correctly fixes the hook to accept both, but the skill documentation should also reference both vocabularies. Out of scope for this PR -- file separately. 3. **PR body test count:** Claims "6 new test cases (Tests 20-25)" and "27 tests (21 existing + 6 new)" but Test 25 is the renumbered old Test 20, not a new test. Actual new tests: 5 (Tests 20-24). Minor documentation inaccuracy. ### SOP COMPLIANCE - [x] Branch named after issue: `220-fix-review-vocab-mismatch` matches issue #220 - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related Notes all present - [x] Related references: Closes #220, board item 638, skill note reference - [x] No secrets committed - [x] No unnecessary file changes: only 2 files changed, both directly related to the fix - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS - **Change failure risk:** Low. The change is additive (accepts more verdicts) and the exclusion logic for negated verdicts is correctly layered. Existing APPROVED paths are untouched by the new grep pattern. - **Discovered scope:** Two items should be tracked as follow-up issues: (1) anchor grep to verdict line format to prevent false positives from prose, (2) update SKILL.md vocabulary to reference both READY and APPROVED. - **DORA impact:** This fix unblocks the kanban pipeline -- tickets reviewed with READY verdict can now advance through the todo-to-next_up gate, restoring deployment frequency throughput. ### VERDICT: APPROVED

2026-03-29 03:19:04 +00:00

Summary

The check-board-advance.sh hook only grepped for "APPROVED" but review agents write "READY" as the passing verdict, causing properly reviewed tickets to be blocked at the todo-to-next_up gate. Updated grep patterns to accept both keywords and added 6 new test cases.

Changes

hooks/check-board-advance.sh: Updated grep patterns in check_review_approved() to match both APPROVED and READY via grep -qiE 'APPROVED|READY'. Added NOT READY exclusion alongside existing NOT APPROVED exclusion. Updated comments and deny messages to reference both keywords.
tests/test_check_board_advance.sh: Added 6 new test cases (Tests 20-25): READY in body (allow), READY in blocks (allow), NOT READY in body (deny), NOT READY in blocks (deny), bulk_move with READY (allow), deny message format.

Test Plan

All 27 tests pass (21 existing + 6 new)
READY verdict accepted in both body and blocks
NOT READY verdict blocked in both body and blocks
APPROVED still works (existing tests unchanged)
NOT APPROVED still blocked (existing tests unchanged)
bulk_move with READY reviewed item accepted
No regressions in allow-path tests

Review Checklist

Passed automated review-fix loop
No secrets committed
No unnecessary file changes
Commit messages are descriptive

Closes #220
board-pal-e-agency item 638
skill-review-ticket pal-e-docs note already documents both READY and APPROVED as valid verdicts

## Summary The `check-board-advance.sh` hook only grepped for "APPROVED" but review agents write "READY" as the passing verdict, causing properly reviewed tickets to be blocked at the todo-to-next_up gate. Updated grep patterns to accept both keywords and added 6 new test cases. ## Changes - `hooks/check-board-advance.sh`: Updated grep patterns in `check_review_approved()` to match both `APPROVED` and `READY` via `grep -qiE 'APPROVED|READY'`. Added `NOT READY` exclusion alongside existing `NOT APPROVED` exclusion. Updated comments and deny messages to reference both keywords. - `tests/test_check_board_advance.sh`: Added 6 new test cases (Tests 20-25): READY in body (allow), READY in blocks (allow), NOT READY in body (deny), NOT READY in blocks (deny), bulk_move with READY (allow), deny message format. ## Test Plan - [x] All 27 tests pass (21 existing + 6 new) - [x] READY verdict accepted in both body and blocks - [x] NOT READY verdict blocked in both body and blocks - [x] APPROVED still works (existing tests unchanged) - [x] NOT APPROVED still blocked (existing tests unchanged) - [x] bulk_move with READY reviewed item accepted - [x] No regressions in allow-path tests ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related Notes - Closes #220 - `board-pal-e-agency` item 638 - `skill-review-ticket` pal-e-docs note already documents both READY and APPROVED as valid verdicts

forgejo_admin added 1 commit

fix: accept READY as passing verdict in check-board-advance hook 3a6b98d68b

Review agents write "READY" but the hook only grepped for "APPROVED",
causing the scope review gate to block properly reviewed tickets.
Updated grep patterns to accept both keywords while preserving
"NOT APPROVED" and "NOT READY" exclusions. Added 6 test cases.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>