feat: enforce review gate at backlog→todo transition #218

Merged

forgejo_admin merged 1 commit from 214-backlog-todo-gate into main 2026-03-28 23:18:12 +00:00

Conversation 2 Commits 1 Files changed 2 +241 -33

forgejo_admin commented 2026-03-28 22:54:48 +00:00

Contributor

Copy link

Summary

Extends check-board-advance.sh to gate backlog→todo in addition to the existing todo→next_up gate. "TODO MUST ALWAYS MEAN REVIEWED" — items cannot move from backlog to todo without a /review-ticket APPROVED verdict. The todo→next_up gate remains as belt-and-suspenders for dependency scoping.

Changes

• hooks/check-board-advance.sh — Extended both update_board_item and bulk_move_board_items routes to detect and gate backlog→todo transitions using the existing check_review_approved helper. Deny messages now include the specific gate name (e.g., "backlog→todo" or "todo→next_up"). Updated header comments to document both gates.
• tests/test_check_board_advance.sh — Added 9 new tests (Tests 21-29) covering the backlog→todo gate: single item deny/allow, NOT APPROVED regression, same-column no-op (todo→todo), bulk move deny/allow, mixed gates (backlog→todo + todo→next_up simultaneously), multiple backlog items, and blocks-based verdict. Updated mock board data with backlog items (id 60, 70). Updated Test 2 to accurately describe fail-open behavior. All 34 tests pass.

Test Plan

• bash tests/test_check_board_advance.sh — 34 passed, 0 failed
• Manual: update_board_item moving backlog→todo without review note — blocked
• Manual: update_board_item moving backlog→todo with APPROVED review — allowed
• Existing todo→next_up gate behavior preserved (Tests 11-20 unchanged)

Review Checklist

• backlog→todo blocked without APPROVED review note
• backlog→todo allowed with APPROVED review note
• Existing todo→next_up gate unchanged (Tests 11-20 pass)
• bulk_move handles both gates simultaneously
• Deny messages include specific gate name
• Fail-open behavior preserved when API unreachable
• All 34 tests pass

Related Notes

• None (hook-only change, no pal-e-docs notes affected)

Related

• Forgejo issue: #214
• Closes #214
• Parent: #161 (scope review pipeline)
• Prior art: #213 (created the hook)

## Summary Extends `check-board-advance.sh` to gate backlog→todo in addition to the existing todo→next_up gate. "TODO MUST ALWAYS MEAN REVIEWED" — items cannot move from backlog to todo without a `/review-ticket` APPROVED verdict. The todo→next_up gate remains as belt-and-suspenders for dependency scoping. ## Changes - `hooks/check-board-advance.sh` — Extended both `update_board_item` and `bulk_move_board_items` routes to detect and gate backlog→todo transitions using the existing `check_review_approved` helper. Deny messages now include the specific gate name (e.g., "backlog→todo" or "todo→next_up"). Updated header comments to document both gates. - `tests/test_check_board_advance.sh` — Added 9 new tests (Tests 21-29) covering the backlog→todo gate: single item deny/allow, NOT APPROVED regression, same-column no-op (todo→todo), bulk move deny/allow, mixed gates (backlog→todo + todo→next_up simultaneously), multiple backlog items, and blocks-based verdict. Updated mock board data with backlog items (id 60, 70). Updated Test 2 to accurately describe fail-open behavior. All 34 tests pass. ## Test Plan - `bash tests/test_check_board_advance.sh` — 34 passed, 0 failed - Manual: `update_board_item` moving backlog→todo without review note — blocked - Manual: `update_board_item` moving backlog→todo with APPROVED review — allowed - Existing todo→next_up gate behavior preserved (Tests 11-20 unchanged) ## Review Checklist - [x] backlog→todo blocked without APPROVED review note - [x] backlog→todo allowed with APPROVED review note - [x] Existing todo→next_up gate unchanged (Tests 11-20 pass) - [x] bulk_move handles both gates simultaneously - [x] Deny messages include specific gate name - [x] Fail-open behavior preserved when API unreachable - [x] All 34 tests pass ## Related Notes - None (hook-only change, no pal-e-docs notes affected) ## Related - Forgejo issue: #214 - Closes #214 - Parent: #161 (scope review pipeline) - Prior art: #213 (created the hook)

forgejo_admin added 1 commit 2026-03-28 22:54:48 +00:00

feat: enforce review gate at backlog→todo transition ... 140b691c14

Extends check-board-advance.sh to gate backlog→todo in addition to the
existing todo→next_up gate. TODO must always mean REVIEWED — items
cannot move from backlog to todo without a /review-ticket APPROVED
verdict. The todo→next_up gate remains as belt-and-suspenders.

Both update_board_item and bulk_move_board_items routes now check for
gated transitions (backlog→todo, todo→next_up) and block without an
APPROVED review note. Deny messages include the specific gate name.

Adds 9 new tests (Tests 21-29) covering the backlog→todo gate for
single items, bulk moves, mixed gates, APPROVED/NOT APPROVED verdicts,
and blocks-based verdicts. All 34 tests pass.

Closes #214

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

forgejo_admin commented 2026-03-28 22:55:42 +00:00

Author

Contributor

Copy link

QA Review: PR #218

Scope Verification

• Parent issue: #214 -- "Enforce review gate at backlog→todo (not just todo→next_up)"
• Files changed: 2 (hooks/check-board-advance.sh, tests/test_check_board_advance.sh)
• Changes match issue scope exactly. No scope creep.

Code Quality

Hook logic (check-board-advance.sh)

• Correctly extends both update_board_item and bulk_move_board_items routes to gate backlog→todo alongside existing todo→next_up
• Gate detection uses explicit source/target column matching -- no ambiguity
• check_review_approved helper reused without modification (good)
• Deny messages now include the specific gate name (backlog→todo or todo→next_up) -- actionable for the agent
• Fail-open behavior preserved: API unreachable = exit 0
• No regressions to existing todo→next_up gate (tests 11-20 unchanged in behavior)

Edge cases handled:

• todo→todo (same column no-op) -- correctly allowed (Test 24)
• in_progress→next_up -- correctly not gated (Test 14, unchanged)
• Mixed bulk moves (backlog→todo + todo→next_up simultaneously) -- correctly blocks both (Test 27)
• NOT APPROVED regression -- correctly blocked for both gates (Tests 12, 23)

Tests (test_check_board_advance.sh)

• 9 new tests (21-29) covering all backlog→todo paths
• Mock board data extended with backlog items (id 60, 70) -- clean addition
• Test 2 updated to accurately describe fail-open behavior instead of incorrectly claiming "move to todo is not gated"
• All 34 tests pass (verified locally)

Nits

• None identified. Changes are minimal, focused, and well-structured.

Test Results

Results: 34 passed, 0 failed

VERDICT: APPROVED

## QA Review: PR #218 ### Scope Verification - Parent issue: #214 -- "Enforce review gate at backlog→todo (not just todo→next_up)" - Files changed: 2 (`hooks/check-board-advance.sh`, `tests/test_check_board_advance.sh`) - Changes match issue scope exactly. No scope creep. ### Code Quality **Hook logic (`check-board-advance.sh`)** - Correctly extends both `update_board_item` and `bulk_move_board_items` routes to gate backlog→todo alongside existing todo→next_up - Gate detection uses explicit source/target column matching -- no ambiguity - `check_review_approved` helper reused without modification (good) - Deny messages now include the specific gate name (`backlog→todo` or `todo→next_up`) -- actionable for the agent - Fail-open behavior preserved: API unreachable = exit 0 - No regressions to existing todo→next_up gate (tests 11-20 unchanged in behavior) **Edge cases handled:** - todo→todo (same column no-op) -- correctly allowed (Test 24) - in_progress→next_up -- correctly not gated (Test 14, unchanged) - Mixed bulk moves (backlog→todo + todo→next_up simultaneously) -- correctly blocks both (Test 27) - NOT APPROVED regression -- correctly blocked for both gates (Tests 12, 23) **Tests (`test_check_board_advance.sh`)** - 9 new tests (21-29) covering all backlog→todo paths - Mock board data extended with backlog items (id 60, 70) -- clean addition - Test 2 updated to accurately describe fail-open behavior instead of incorrectly claiming "move to todo is not gated" - All 34 tests pass (verified locally) ### Nits - None identified. Changes are minimal, focused, and well-structured. ### Test Results ``` Results: 34 passed, 0 failed ``` **VERDICT: APPROVED**

forgejo_admin commented 2026-03-28 23:10:47 +00:00

Author

Contributor

Copy link

PR #218 Review

DOMAIN REVIEW

Tech Stack: Bash hooks + Bash test framework with Python mock HTTP server.

Correctness Analysis:

1. Does the new backlog->todo gate correctly reuse the existing check_review_approved helper?
   Yes. The check_review_approved function (lines 46-77 on main) is completely untouched by this PR. Both the update_board_item route (line 116) and the bulk_move_board_items route (line 171) already called it; those call sites remain structurally identical post-change. The helper searches for review-{item_id}-* notes and checks body + blocks for APPROVED (excluding NOT APPROVED). This works equally well for backlog->todo and todo->next_up gates since the review note convention is item-based, not column-based. Clean reuse.

2. Does it handle both update_board_item and bulk_move_board_items routes?
   Yes, both routes are updated symmetrically:

   • update_board_item: Target column check broadened from != "next_up" to != "todo" && != "next_up". Then a GATED variable determines the specific transition (backlog->todo or todo->next_up). If GATED is empty, the hook allows.
   • bulk_move_board_items: Filter broadened from .column == "next_up" to .column == "todo" or .column == "next_up". Per-item loop now looks up TARGET_COL from the move request and applies the same GATED logic. Gate name is appended to blocked items for the deny message.

3. Does it NOT break the existing todo->next_up gate?
   No breakage. The gated transition logic explicitly handles todo->next_up as a named case alongside backlog->todo. Tests 11-20 (the original deny/allow suite) are completely unchanged in the diff. The elif branch for todo->next_up preserves the prior behavior exactly.

4. Test coverage for the 9 new tests (Tests 21-29):

   • Test 21: backlog->todo DENY (no review note) -- core deny path
   • Test 22: backlog->todo ALLOW (APPROVED review) -- core allow path
   • Test 23: backlog->todo DENY (NOT APPROVED review) -- regression guard against false positives
   • Test 24: todo->todo same-column no-op (ALLOW) -- edge case: item already in todo, not a transition
   • Test 25: bulk_move backlog->todo DENY (no review) -- bulk path deny
   • Test 26: bulk_move backlog->todo ALLOW (APPROVED review) -- bulk path allow
   • Test 27: bulk_move mixed gates (backlog->todo + todo->next_up) DENY -- both gates fire simultaneously
   • Test 28: bulk_move multiple backlog items DENY -- multi-item blocking
   • Test 29: APPROVED in blocks (not body) for backlog->todo ALLOW -- blocks-based verdict detection

   The coverage is thorough. Tests 21-23 mirror 11-13 (deny/allow/NOT APPROVED regression) for the new gate. Test 24 is a critical edge case. Tests 25-28 cover bulk operations including the mixed-gate scenario. Test 29 confirms blocks-based verdict works for the new gate just as Test 18 does for the existing gate.

Test 2 Update: The original Test 2 ("move to todo, not next_up") was subtly misleading -- with the new code, moving to todo IS now a gated column, but without a mock API the hook fails open. The updated test honestly names this as "fail-open" behavior with an explicitly unreachable API URL (http://127.0.0.1:1). This is a good correction that makes the test contract clearer.

Deny message improvement: Gate names (backlog->todo, todo->next_up) are now included in deny messages. Tests 21 and 27 explicitly verify these gate names appear. This improves actionability for agents receiving the deny.

BLOCKERS

None.

• No new functionality without tests (9 new tests, 34 total)
• No user input validation concerns (hook processes tool input from Claude harness, not external users)
• No secrets or credentials
• No DRY violations -- check_review_approved is reused, not duplicated

NITS

1. Test 6 description stale: Test 6 says "bulk_move with no next_up targets" but now todo is also a gated column. The test still passes correctly (items target in_progress/done which are not gated), but the description could be updated to "bulk_move with no gated column targets" for consistency with the new terminology. Non-blocking since the test logic is correct.

2. Mock board data comment: The comment on the board items in the mock setup says items 60 and 70 are in "backlog" but the fixture file at /home/ldraney/claude-custom/tests/test_check_board_advance.sh line 57 still references the old count in the description ("Board items: item 42..."). The diff correctly updates this, so this is addressed.

SOP COMPLIANCE

• Branch named after issue: 214-backlog-todo-gate follows {issue-number}-{kebab-case-purpose}
• PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present
• Related references issue #214, parent #161, prior art #213
• No secrets committed
• No scope creep -- all changes directly serve the backlog->todo gate requirement
• Commit messages are descriptive (per PR title)

PROCESS OBSERVATIONS

• Deployment frequency: This is a hook-only change with zero infrastructure risk. Merging quickly is safe.
• Change failure risk: Low. The fail-open pattern (trap 'exit 0' ERR) means even if the new gate logic has an edge case bug, it degrades to "allow" rather than blocking agents. The 34 tests provide strong regression coverage.
• Pattern quality: The symmetric refactoring (introducing the GATED variable in both routes) is a clean way to add gates without duplicating the transition-detection logic. Adding a third gate in the future would be a 3-line change.

VERDICT: APPROVED

## PR #218 Review ### DOMAIN REVIEW **Tech Stack**: Bash hooks + Bash test framework with Python mock HTTP server. **Correctness Analysis**: 1. **Does the new backlog->todo gate correctly reuse the existing `check_review_approved` helper?** Yes. The `check_review_approved` function (lines 46-77 on main) is completely untouched by this PR. Both the `update_board_item` route (line 116) and the `bulk_move_board_items` route (line 171) already called it; those call sites remain structurally identical post-change. The helper searches for `review-{item_id}-*` notes and checks body + blocks for APPROVED (excluding NOT APPROVED). This works equally well for backlog->todo and todo->next_up gates since the review note convention is item-based, not column-based. Clean reuse. 2. **Does it handle both `update_board_item` and `bulk_move_board_items` routes?** Yes, both routes are updated symmetrically: - `update_board_item`: Target column check broadened from `!= "next_up"` to `!= "todo" && != "next_up"`. Then a `GATED` variable determines the specific transition (`backlog->todo` or `todo->next_up`). If `GATED` is empty, the hook allows. - `bulk_move_board_items`: Filter broadened from `.column == "next_up"` to `.column == "todo" or .column == "next_up"`. Per-item loop now looks up `TARGET_COL` from the move request and applies the same `GATED` logic. Gate name is appended to blocked items for the deny message. 3. **Does it NOT break the existing todo->next_up gate?** No breakage. The gated transition logic explicitly handles `todo->next_up` as a named case alongside `backlog->todo`. Tests 11-20 (the original deny/allow suite) are completely unchanged in the diff. The `elif` branch for `todo->next_up` preserves the prior behavior exactly. 4. **Test coverage for the 9 new tests (Tests 21-29)**: - Test 21: backlog->todo DENY (no review note) -- core deny path - Test 22: backlog->todo ALLOW (APPROVED review) -- core allow path - Test 23: backlog->todo DENY (NOT APPROVED review) -- regression guard against false positives - Test 24: todo->todo same-column no-op (ALLOW) -- edge case: item already in todo, not a transition - Test 25: bulk_move backlog->todo DENY (no review) -- bulk path deny - Test 26: bulk_move backlog->todo ALLOW (APPROVED review) -- bulk path allow - Test 27: bulk_move mixed gates (backlog->todo + todo->next_up) DENY -- both gates fire simultaneously - Test 28: bulk_move multiple backlog items DENY -- multi-item blocking - Test 29: APPROVED in blocks (not body) for backlog->todo ALLOW -- blocks-based verdict detection The coverage is thorough. Tests 21-23 mirror 11-13 (deny/allow/NOT APPROVED regression) for the new gate. Test 24 is a critical edge case. Tests 25-28 cover bulk operations including the mixed-gate scenario. Test 29 confirms blocks-based verdict works for the new gate just as Test 18 does for the existing gate. **Test 2 Update**: The original Test 2 ("move to todo, not next_up") was subtly misleading -- with the new code, moving to `todo` IS now a gated column, but without a mock API the hook fails open. The updated test honestly names this as "fail-open" behavior with an explicitly unreachable API URL (`http://127.0.0.1:1`). This is a good correction that makes the test contract clearer. **Deny message improvement**: Gate names (`backlog->todo`, `todo->next_up`) are now included in deny messages. Tests 21 and 27 explicitly verify these gate names appear. This improves actionability for agents receiving the deny. ### BLOCKERS None. - No new functionality without tests (9 new tests, 34 total) - No user input validation concerns (hook processes tool input from Claude harness, not external users) - No secrets or credentials - No DRY violations -- `check_review_approved` is reused, not duplicated ### NITS 1. **Test 6 description stale**: Test 6 says "bulk_move with no next_up targets" but now `todo` is also a gated column. The test still passes correctly (items target `in_progress`/`done` which are not gated), but the description could be updated to "bulk_move with no gated column targets" for consistency with the new terminology. Non-blocking since the test logic is correct. 2. **Mock board data comment**: The comment on the board items in the mock setup says items 60 and 70 are in "backlog" but the fixture file at `/home/ldraney/claude-custom/tests/test_check_board_advance.sh` line 57 still references the old count in the description ("Board items: item 42..."). The diff correctly updates this, so this is addressed. ### SOP COMPLIANCE - [x] Branch named after issue: `214-backlog-todo-gate` follows `{issue-number}-{kebab-case-purpose}` - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present - [x] Related references issue #214, parent #161, prior art #213 - [x] No secrets committed - [x] No scope creep -- all changes directly serve the backlog->todo gate requirement - [x] Commit messages are descriptive (per PR title) ### PROCESS OBSERVATIONS - **Deployment frequency**: This is a hook-only change with zero infrastructure risk. Merging quickly is safe. - **Change failure risk**: Low. The fail-open pattern (`trap 'exit 0' ERR`) means even if the new gate logic has an edge case bug, it degrades to "allow" rather than blocking agents. The 34 tests provide strong regression coverage. - **Pattern quality**: The symmetric refactoring (introducing the `GATED` variable in both routes) is a clean way to add gates without duplicating the transition-detection logic. Adding a third gate in the future would be a 3-line change. ### VERDICT: APPROVED

forgejo_admin merged commit abcc7a76b6 into main 2026-03-28 23:18:12 +00:00

forgejo_admin deleted branch 214-backlog-todo-gate 2026-03-28 23:18:13 +00:00

forgejo_admin referenced this pull request from a commit 2026-03-28 23:18:14 +00:00

feat: enforce review gate at backlog→todo transition (#218)

forgejo_admin referenced this pull request 2026-04-04 22:53:46 +00:00

Validation agent: Playwright-based role validation for westside-basketball #235

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

domain:backend

  

domain:devops

  

domain:frontend

  

status:approved

QA passed, awaiting merge approval

  

status:in-progress

Dev agent is actively working

  

status:needs-fix

QA found issues, back to dev

  

status:qa

PR submitted, awaiting QA review

  

type:bug

Bug fix

  

type:devops

Infrastructure/CI/config work

  

type:feature

New functionality

No labels

domain:backend

domain:devops

domain:frontend

status:approved

status:in-progress

status:needs-fix

status:qa

type:bug

type:devops

type:feature

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

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

ldraney/claude-custom!218

No description provided.