feat: validation-gate hook blocks done without validation proof

forgejo_admin commented

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

Author

Contributor

PR #229 Review

DOMAIN REVIEW

Tech stack: Bash shell hooks + JSON (settings.json). Reviewing against shell scripting best practices, hook architecture patterns established in this repo (specifically check-board-advance.sh as the reference implementation), and fail-open safety requirements.

gate-validation-done.sh (NEW, 200 lines)

The hook follows the established patterns from check-board-advance.sh closely and correctly:

Fail-open pattern: set -eo pipefail + trap 'exit 0' ERR + command -v jq &>/dev/null || exit 0 -- identical to the reference hook. Correct.
Non-done moves exit immediately (line 103: if [[ "$TARGET_COLUMN" != "done" ]]; then exit 0). No false positives on non-done column moves. Correct.
Items without forgejo_issue_url fail open (line 114 in extract_issue_num_from_item). Correct -- note-type board items are not gated.
Network/API errors fail open -- curl failures trigger ERR trap or explicit || exit 0 / || return 1 patterns. Correct.
Validation note lookup uses GET /notes?tags=validation,pass then filters by slug prefix validation-{issue-num}-*. This is a two-step filter (API tag filter + local slug prefix match) which is sound and avoids false positives from unrelated validation notes.
Bulk move handler correctly denies the entire operation if any item targeting "done" lacks validation. This is correct -- bulk_move is atomic, so partial allow is not possible.
jq --argjson used for numeric IDs coming from .id fields -- consistent with check-board-advance.sh pattern.

board-item-on-merge.sh (MODIFIED)

All 7 occurrences of "done" correctly changed to "validation":

Comment header (line 1, line 9)
Fallback reminder messages (2 occurrences)
The actual PATCH payload: '{"column": "validation"}'
Log message
Final additionalContext -- also adds helpful guidance about creating validation notes. Good UX for the agent.

settings.json (MODIFIED)

The new hook is appended to the existing mcp__pal-e-docs__update_board_item|mcp__pal-e-docs__bulk_move_board_items matcher's hooks array, alongside check-board-advance.sh. No duplicate matcher entry created. Both hooks fire on the same events -- check-board-advance.sh gates left-side transitions (backlog->todo, todo->next_up), and gate-validation-done.sh gates the right-side transition (validation->done). They are complementary and non-overlapping in their target columns.

BLOCKERS

None.

No new functionality requiring test coverage (shell hooks are tested manually per test plan; this is consistent with all other hooks in the repo).
No user input handling (hook reads structured JSON from Claude's tool dispatch, not user-supplied data).
No secrets or credentials in code.
No DRY violation in auth/security paths.

NITS

Unused boards-config.sh source (line 30 of gate-validation-done.sh): The hook sources boards-config.sh but never references the BOARDS array. The board slug comes from tool_input.board_slug in both the single and bulk code paths. This is harmless (the source just defines an array variable) but is dead code. The reference hook check-board-advance.sh also does not source boards-config.sh, so this is inconsistent with the pattern. Consider removing the source line and the shellcheck directive on line 29.
ITEMS_PARAM stringification (line 148 of the new hook): jq -r '.tool_input.items // empty' -- when .tool_input.items is a JSON array, jq -r outputs the raw JSON representation (same as without -r for non-string types). This works, but -r is semantically for "raw string output" and is misleading on array values. This is a pre-existing pattern from check-board-advance.sh line 149, so not introduced by this PR -- just noting for awareness.

SOP COMPLIANCE

Branch named after issue: 210-validation-gate-hook follows {issue-number}-{kebab-case-purpose} convention
PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related Notes, Related sections all present
Related references issue #210, board item, dependency, and discovered scope
No secrets committed
No unnecessary file changes -- all 3 files are directly related to the validation gate feature
Commit messages are descriptive (PR title: feat: validation-gate hook blocks done without validation proof)

PROCESS OBSERVATIONS

DORA impact: This hook enforces that DORA Lead Time measurement ends at validation pass, not at merge. This is a significant process improvement -- it ensures production verification is tracked, not just code merge. Deployment Frequency is not affected. Change Failure Rate benefits from mandatory validation proof before closing items.
Dependency noted: The PR correctly identifies forgejo_admin/pal-e-api#223 (validation column enum) as a dependency. This hook will deny moves to "done" once deployed, but the post-merge hook needs the API to accept "validation" as a valid column value.
Discovered scope tracked: /validate-ticket skill is noted as discovered scope and tracked separately. Good discipline.

VERDICT: APPROVED

## PR #229 Review ### DOMAIN REVIEW **Tech stack:** Bash shell hooks + JSON (settings.json). Reviewing against shell scripting best practices, hook architecture patterns established in this repo (specifically `check-board-advance.sh` as the reference implementation), and fail-open safety requirements. **gate-validation-done.sh (NEW, 200 lines)** The hook follows the established patterns from `check-board-advance.sh` closely and correctly: - **Fail-open pattern:** `set -eo pipefail` + `trap 'exit 0' ERR` + `command -v jq &>/dev/null || exit 0` -- identical to the reference hook. Correct. - **Non-done moves exit immediately** (line 103: `if [[ "$TARGET_COLUMN" != "done" ]]; then exit 0`). No false positives on non-done column moves. Correct. - **Items without `forgejo_issue_url` fail open** (line 114 in `extract_issue_num_from_item`). Correct -- note-type board items are not gated. - **Network/API errors fail open** -- curl failures trigger ERR trap or explicit `|| exit 0` / `|| return 1` patterns. Correct. - **Validation note lookup** uses `GET /notes?tags=validation,pass` then filters by slug prefix `validation-{issue-num}-*`. This is a two-step filter (API tag filter + local slug prefix match) which is sound and avoids false positives from unrelated validation notes. - **Bulk move handler** correctly denies the entire operation if any item targeting "done" lacks validation. This is correct -- bulk_move is atomic, so partial allow is not possible. - **jq `--argjson`** used for numeric IDs coming from `.id` fields -- consistent with `check-board-advance.sh` pattern. **board-item-on-merge.sh (MODIFIED)** All 7 occurrences of "done" correctly changed to "validation": - Comment header (line 1, line 9) - Fallback reminder messages (2 occurrences) - The actual PATCH payload: `'{"column": "validation"}'` - Log message - Final `additionalContext` -- also adds helpful guidance about creating validation notes. Good UX for the agent. **settings.json (MODIFIED)** The new hook is appended to the existing `mcp__pal-e-docs__update_board_item|mcp__pal-e-docs__bulk_move_board_items` matcher's hooks array, alongside `check-board-advance.sh`. No duplicate matcher entry created. Both hooks fire on the same events -- `check-board-advance.sh` gates left-side transitions (backlog->todo, todo->next_up), and `gate-validation-done.sh` gates the right-side transition (validation->done). They are complementary and non-overlapping in their target columns. ### BLOCKERS None. - No new functionality requiring test coverage (shell hooks are tested manually per test plan; this is consistent with all other hooks in the repo). - No user input handling (hook reads structured JSON from Claude's tool dispatch, not user-supplied data). - No secrets or credentials in code. - No DRY violation in auth/security paths. ### NITS 1. **Unused `boards-config.sh` source** (line 30 of `gate-validation-done.sh`): The hook sources `boards-config.sh` but never references the `BOARDS` array. The board slug comes from `tool_input.board_slug` in both the single and bulk code paths. This is harmless (the source just defines an array variable) but is dead code. The reference hook `check-board-advance.sh` also does not source `boards-config.sh`, so this is inconsistent with the pattern. Consider removing the source line and the shellcheck directive on line 29. 2. **`ITEMS_PARAM` stringification** (line 148 of the new hook): `jq -r '.tool_input.items // empty'` -- when `.tool_input.items` is a JSON array, `jq -r` outputs the raw JSON representation (same as without `-r` for non-string types). This works, but `-r` is semantically for "raw string output" and is misleading on array values. This is a pre-existing pattern from `check-board-advance.sh` line 149, so not introduced by this PR -- just noting for awareness. ### SOP COMPLIANCE - [x] Branch named after issue: `210-validation-gate-hook` follows `{issue-number}-{kebab-case-purpose}` convention - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related Notes, Related sections all present - [x] Related references issue #210, board item, dependency, and discovered scope - [x] No secrets committed - [x] No unnecessary file changes -- all 3 files are directly related to the validation gate feature - [x] Commit messages are descriptive (PR title: `feat: validation-gate hook blocks done without validation proof`) ### PROCESS OBSERVATIONS - **DORA impact:** This hook enforces that DORA Lead Time measurement ends at validation pass, not at merge. This is a significant process improvement -- it ensures production verification is tracked, not just code merge. Deployment Frequency is not affected. Change Failure Rate benefits from mandatory validation proof before closing items. - **Dependency noted:** The PR correctly identifies `forgejo_admin/pal-e-api#223` (validation column enum) as a dependency. This hook will deny moves to "done" once deployed, but the post-merge hook needs the API to accept "validation" as a valid column value. - **Discovered scope tracked:** `/validate-ticket` skill is noted as discovered scope and tracked separately. Good discipline. ### VERDICT: APPROVED

2026-03-29 04:27:26 +00:00

Summary

Adds a PreToolUse hook that enforces the right-side kanban gate: items cannot move to "done" without a passing validation note in pal-e-docs. Also updates the post-merge hook to route items to the "validation" column instead of "done".

Changes

hooks/gate-validation-done.sh (NEW) -- PreToolUse hook that intercepts update_board_item and bulk_move_board_items calls targeting "done". Queries pal-e-docs for validation-{issue-num}-* notes with pass tag. Denies if none found. Handles both single and bulk moves. Fails open on network errors and for items without forgejo_issue_url.
settings.json -- Added gate-validation-done.sh to the existing update_board_item|bulk_move_board_items matcher's hooks array (alongside check-board-advance.sh).
hooks/board-item-on-merge.sh -- Changed target column from "done" to "validation". Updated all comments and fallback messages to reflect the new target.

Test Plan

Verified non-done column moves exit 0 (no-op): echo '{"tool_name":"mcp__pal-e-docs__update_board_item","tool_input":{"board_slug":"board-pal-e-agency","item_id":123,"column":"in_progress"}}' | bash hooks/gate-validation-done.sh
Verified move-to-done without validation note is denied (tested with real board item #519/issue #210)
Verified non-existent items fail open (exit 0)
Verified bulk move correctly blocks only items targeting "done"
Verified bulk move with no "done" targets is a clean no-op
Validated settings.json parses as valid JSON
Syntax-checked both shell scripts with bash -n

Review Checklist

No unrelated changes
Fail-open pattern followed (trap 'exit 0' ERR)
Hook added to existing matcher (no duplicate entry)
board-item-on-merge.sh comments updated to match new behavior
settings.json valid JSON
Shell scripts pass bash -n syntax check

sop-board-workflow -- defines the validation column semantics
template-validation -- defines the validation note format the hook checks for

Closes #210
Board: board-pal-e-agency, item 519
Dependency: forgejo_admin/pal-e-api#223 (validation column enum)
Discovered scope: /validate-ticket skill (tracked separately)

## Summary Adds a PreToolUse hook that enforces the right-side kanban gate: items cannot move to "done" without a passing validation note in pal-e-docs. Also updates the post-merge hook to route items to the "validation" column instead of "done". ## Changes - `hooks/gate-validation-done.sh` (NEW) -- PreToolUse hook that intercepts `update_board_item` and `bulk_move_board_items` calls targeting "done". Queries pal-e-docs for `validation-{issue-num}-*` notes with `pass` tag. Denies if none found. Handles both single and bulk moves. Fails open on network errors and for items without `forgejo_issue_url`. - `settings.json` -- Added `gate-validation-done.sh` to the existing `update_board_item|bulk_move_board_items` matcher's hooks array (alongside `check-board-advance.sh`). - `hooks/board-item-on-merge.sh` -- Changed target column from `"done"` to `"validation"`. Updated all comments and fallback messages to reflect the new target. ## Test Plan - Verified non-done column moves exit 0 (no-op): `echo '{"tool_name":"mcp__pal-e-docs__update_board_item","tool_input":{"board_slug":"board-pal-e-agency","item_id":123,"column":"in_progress"}}' | bash hooks/gate-validation-done.sh` - Verified move-to-done without validation note is denied (tested with real board item #519/issue #210) - Verified non-existent items fail open (exit 0) - Verified bulk move correctly blocks only items targeting "done" - Verified bulk move with no "done" targets is a clean no-op - Validated `settings.json` parses as valid JSON - Syntax-checked both shell scripts with `bash -n` ## Review Checklist - [x] No unrelated changes - [x] Fail-open pattern followed (trap 'exit 0' ERR) - [x] Hook added to existing matcher (no duplicate entry) - [x] board-item-on-merge.sh comments updated to match new behavior - [x] settings.json valid JSON - [x] Shell scripts pass bash -n syntax check ## Related Notes - `sop-board-workflow` -- defines the validation column semantics - `template-validation` -- defines the validation note format the hook checks for ## Related - Closes #210 - Board: board-pal-e-agency, item 519 - Dependency: `forgejo_admin/pal-e-api#223` (validation column enum) - Discovered scope: `/validate-ticket` skill (tracked separately)

forgejo_admin added 1 commit

feat: validation-gate hook blocks done column without validation proof 0df2ffe380

Add PreToolUse hook that intercepts update_board_item and
bulk_move_board_items calls targeting the "done" column. The hook
queries pal-e-docs for a validation note (validation-{issue-num}-*
with pass tag) and denies the move if none exists. Items without a
forgejo_issue_url fail open (note-type items are not gated).

Also updates board-item-on-merge.sh to route merged items to the
"validation" column instead of "done", ensuring every item passes
through the validation gate before completion.

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