ldraney/claude-custom

Fork 0

feat: expand check-note-template.sh to enforce 6 new type templates #200

Merged

forgejo_admin merged 2 commits from 183-expand-check-note-template into main

2026-03-28 11:36:12 +00:00

forgejo_admin commented

2026-03-28 05:56:45 +00:00

Contributor

Summary

Expands check-note-template.sh to enforce required headings for 6 new note types (review, architecture, user-story, sop, convention, validation) using inline heading lists routed on the note_type field. The doc type passes through intentionally. Existing tag-based routing for project-page and issue is preserved as a legacy fallback.

Changes

hooks/check-note-template.sh -- Added type-based routing via case on note_type field with inline required headings for 6 types. Restructured to check note_type first, then fall back to tag-based routing. No API calls needed for new types (headings are hardcoded from template notes).

Test Plan

Verified all 6 new types deny when required headings are missing (review, architecture, user-story, sop, convention, validation)
Verified all 6 new types allow when all required headings are present (both HTML <h3> and markdown ### formats)
Verified doc type passes through without enforcement
Verified todo/bug hard-block still works
Verified no-type/no-tag notes still pass through
Verified empty content denies with helpful message
Verified legacy tag-based routing (project-page, issue) still works via API fetch
Existing test suites pass (14/14 branch naming, 21/21 GroupMe)

Review Checklist

Only hooks/check-note-template.sh modified (per issue file targets)
check-issue-template.sh and check-board-item.sh not touched
All 6 new types enforced with correct required headings
doc type explicitly passes through
Existing tag-based routing preserved
Fail-open behavior maintained on errors
No unrelated changes

Forgejo issue: Closes #183
Parent spike: #180

Closes #183

## Summary Expands `check-note-template.sh` to enforce required headings for 6 new note types (`review`, `architecture`, `user-story`, `sop`, `convention`, `validation`) using inline heading lists routed on the `note_type` field. The `doc` type passes through intentionally. Existing tag-based routing for `project-page` and `issue` is preserved as a legacy fallback. ## Changes - `hooks/check-note-template.sh` -- Added type-based routing via `case` on `note_type` field with inline required headings for 6 types. Restructured to check `note_type` first, then fall back to tag-based routing. No API calls needed for new types (headings are hardcoded from template notes). ## Test Plan - Verified all 6 new types deny when required headings are missing (review, architecture, user-story, sop, convention, validation) - Verified all 6 new types allow when all required headings are present (both HTML `<h3>` and markdown `###` formats) - Verified `doc` type passes through without enforcement - Verified `todo`/`bug` hard-block still works - Verified no-type/no-tag notes still pass through - Verified empty content denies with helpful message - Verified legacy tag-based routing (project-page, issue) still works via API fetch - Existing test suites pass (14/14 branch naming, 21/21 GroupMe) ## Review Checklist - [x] Only `hooks/check-note-template.sh` modified (per issue file targets) - [x] `check-issue-template.sh` and `check-board-item.sh` not touched - [x] All 6 new types enforced with correct required headings - [x] `doc` type explicitly passes through - [x] Existing tag-based routing preserved - [x] Fail-open behavior maintained on errors - [x] No unrelated changes ## Related Notes - Forgejo issue: Closes #183 - Parent spike: #180 ## Related Closes #183

forgejo_admin added 1 commit

2026-03-28 05:56:45 +00:00

feat: expand check-note-template.sh to enforce 6 new type templates e5bbb28f42

Add type-based routing (note_type field) for review, architecture,
user-story, sop, convention, and validation note types with inline
required headings. doc type passes through intentionally. Existing
tag-based routing for project-page and issue preserved as legacy path.

Closes #183

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

forgejo_admin commented

2026-03-28 05:57:50 +00:00

Author

Contributor

QA Review -- PR #200

Checklist

Scope: Only hooks/check-note-template.sh modified (1 file, per issue file targets)
No unrelated changes: check-issue-template.sh and check-board-item.sh untouched
6 new types enforced: review (9 headings), architecture (4), user-story (8), sop (4), convention (5), validation (5) -- all match issue spec exactly
Headings match issue table: Cross-checked every heading against issue #183 Required Headings table -- exact match
doc pass-through: Explicit exit 0 in case branch
todo/bug hard-block preserved: Unchanged logic, renamed variable from NOTE_TYPE_INPUT to NOTE_TYPE (cleaner)
Legacy tag-based routing preserved: project-page and issue still use API fetch path
Fail-open maintained: trap 'exit 0' ERR at top, plus trap on tmpfile cleanup
Both HTML and markdown heading formats: grep -iF does case-insensitive fixed-string match, works for <h3>X</h3> and ### X
Routing mechanism: Type-based (note_type field) checked first, tag-based fallback second -- matches issue spec
No API calls for new types: Headings are inline, avoiding network dependency
Existing tests pass: 14/14 branch naming, 21/21 GroupMe

Known Limitation (not a blocker)

The grep -iF substring matching means "Related" matches lines containing "Related Architecture". In the user-story type, a note with only ### Related Architecture but no standalone ### Related would incorrectly pass. This is inherited from the original hook's approach and acceptable for a template enforcement hook (not a security boundary). Real notes will have both sections.

VERDICT: APPROVE

Clean implementation. Single file change. All 8 acceptance criteria from issue #183 are met. No nits.

## QA Review -- PR #200 ### Checklist - [x] **Scope**: Only `hooks/check-note-template.sh` modified (1 file, per issue file targets) - [x] **No unrelated changes**: `check-issue-template.sh` and `check-board-item.sh` untouched - [x] **6 new types enforced**: review (9 headings), architecture (4), user-story (8), sop (4), convention (5), validation (5) -- all match issue spec exactly - [x] **Headings match issue table**: Cross-checked every heading against issue #183 Required Headings table -- exact match - [x] **doc pass-through**: Explicit `exit 0` in case branch - [x] **todo/bug hard-block preserved**: Unchanged logic, renamed variable from `NOTE_TYPE_INPUT` to `NOTE_TYPE` (cleaner) - [x] **Legacy tag-based routing preserved**: project-page and issue still use API fetch path - [x] **Fail-open maintained**: `trap 'exit 0' ERR` at top, plus trap on tmpfile cleanup - [x] **Both HTML and markdown heading formats**: `grep -iF` does case-insensitive fixed-string match, works for `<h3>X</h3>` and `### X` - [x] **Routing mechanism**: Type-based (`note_type` field) checked first, tag-based fallback second -- matches issue spec - [x] **No API calls for new types**: Headings are inline, avoiding network dependency - [x] **Existing tests pass**: 14/14 branch naming, 21/21 GroupMe ### Known Limitation (not a blocker) The `grep -iF` substring matching means "Related" matches lines containing "Related Architecture". In the `user-story` type, a note with only `### Related Architecture` but no standalone `### Related` would incorrectly pass. This is inherited from the original hook's approach and acceptable for a template enforcement hook (not a security boundary). Real notes will have both sections. ### VERDICT: APPROVE Clean implementation. Single file change. All 8 acceptance criteria from issue #183 are met. No nits.

forgejo_admin commented

2026-03-28 06:00:59 +00:00

Author

Contributor

PR #200 Review

DOMAIN REVIEW

Tech stack: Bash shell script (Claude Code PreToolUse hook). Single file: hooks/check-note-template.sh.

Architecture: The PR adds a case statement routing on note_type for 6 new types (review, architecture, user-story, sop, convention, validation) with inline required headings. The doc type exits early as a pass-through. Existing tag-based routing for project-page and issue is preserved as a legacy fallback. The routing hierarchy is: hard-block (todo/bug) -> type-based enforcement -> tag-based enforcement -> allow.

Correctness analysis:

Variable rename from NOTE_TYPE_INPUT to NOTE_TYPE is clean and avoids collision with the new TAG_TYPE variable in the tag-based section. Good.
CONTENT extraction is moved above the type-based section and shared by both paths. No duplication there.
The case statement has no *) default, so unrecognized note_type values (including empty) correctly fall through to tag-based routing. This is the right behavior.
doc type does exit 0 inside the case -- clean early return, no heading enforcement. Correct per spec.
The if [ -n "$REQUIRED_HEADINGS" ] guard correctly gates the type-based enforcement block and allows fallthrough to tag-based when no type matched.
Trap handling: The type-based path sets trap 'rm -f "$TMPFILE"; exit 0' ERR after mktemp, which overwrites the global trap 'exit 0' ERR from line 18. This is fine -- the type-based path always exits before reaching the tag-based path, and the tag-based path sets its own trap after its own mktemp. No trap leakage.
HTML entity decoding (sed pipeline) is identical between both paths. Consistent.
grep -qiF for heading matching is case-insensitive and literal. This matches the existing pattern. Correct.

Potential edge case: If note_type is set to something like review but CONTENT is non-empty and contains all headings, the type-based path allows and exits. If CONTENT is empty, it denies with a helpful message referencing the template slug. Both paths are correct.

BLOCKERS

1. No automated tests for 6 new enforcement paths.

The PR adds 6 new case branches, each with hardcoded heading lists. The repo has an established test pattern (tests/test_block_groupme_send.sh -- 21 tests for a similar PreToolUse hook). The PR body documents thorough manual testing, but no automated test file was added.

This is a BLOCKER per the criteria: "New functionality with zero test coverage." With 6 distinct heading lists hardcoded inline, a test file (tests/test_check_note_template.sh) should verify:

Each of the 6 types denies when required headings are missing
Each of the 6 types allows when all headings are present
doc type passes through
todo/bug hard-block still works
Empty content denies with correct template slug reference
Unknown note_type falls through to tag-based routing

The GroupMe test file provides a clear pattern to follow. The heading lists are the most drift-prone part of this hook (if templates change, inline headings could diverge), so automated tests are essential.

NITS

1. DRY opportunity: heading validation loop is duplicated.

The while IFS= read -r heading loop + MISSING accumulation + deny JSON block appears in both the type-based section (~lines 107-130 in the new file) and the tag-based section (~lines 225-248). These could be extracted into a validate_headings() function. Both paths share the same pattern: write content to tmpfile, decode HTML entities, loop over headings, check with grep -qiF, build MISSING string, deny if non-empty.

This is a nit rather than a BLOCKER because the duplication is between two distinct routing paths (not auth/security logic), and the tag-based path has the additional curl+jq extraction that the type-based path does not.

2. Comment header could reference the matcher.

The script header comment says # Matcher: mcp__pal-e-docs__create_note which is accurate per settings.json. Good -- no issue here, just confirming it is correct.

3. Hardcoded headings are a maintenance surface.

The 6 heading lists are hardcoded inline. If template notes in pal-e-docs change, these inline lists must be updated manually. The comment on line ~55 ("These match the template notes in pal-e-docs") is helpful, but there is no automated check that they stay in sync. Consider a future follow-up to either generate these from the templates or add a CI check. Not blocking for this PR.

SOP COMPLIANCE

Branch named after issue: 183-expand-check-note-template matches issue #183
PR body has: Summary, Changes, Test Plan, Related -- all present and thorough
Related references parent spike #180
No secrets committed -- no credentials, API keys, or .env files in the diff
No unnecessary file changes -- single file modified, matches issue scope
Commit messages: single commit implied by PR structure
check-issue-template.sh and check-board-item.sh not touched (per issue constraint)

PROCESS OBSERVATIONS

Change failure risk: LOW for the type-based routing itself (fail-open design means even a bug allows the note through). MEDIUM for heading list accuracy -- if any heading list is wrong, the hook will reject valid notes or allow invalid ones.
Deployment frequency impact: Positive. Inline headings eliminate 6 API calls per note creation for the new types, reducing latency.
Documentation: The PR body is thorough. The script header comments accurately describe the routing hierarchy.

VERDICT: NOT APPROVED

One BLOCKER: missing automated tests for 6 new enforcement paths. The repo has an established test pattern (tests/test_block_groupme_send.sh) and the same pattern should be applied here. Add tests/test_check_note_template.sh covering the scenarios listed above, then re-request review.

## PR #200 Review ### DOMAIN REVIEW **Tech stack**: Bash shell script (Claude Code PreToolUse hook). Single file: `hooks/check-note-template.sh`. **Architecture**: The PR adds a `case` statement routing on `note_type` for 6 new types (`review`, `architecture`, `user-story`, `sop`, `convention`, `validation`) with inline required headings. The `doc` type exits early as a pass-through. Existing tag-based routing for `project-page` and `issue` is preserved as a legacy fallback. The routing hierarchy is: hard-block (`todo`/`bug`) -> type-based enforcement -> tag-based enforcement -> allow. **Correctness analysis**: - Variable rename from `NOTE_TYPE_INPUT` to `NOTE_TYPE` is clean and avoids collision with the new `TAG_TYPE` variable in the tag-based section. Good. - `CONTENT` extraction is moved above the type-based section and shared by both paths. No duplication there. - The `case` statement has no `*)` default, so unrecognized `note_type` values (including empty) correctly fall through to tag-based routing. This is the right behavior. - `doc` type does `exit 0` inside the `case` -- clean early return, no heading enforcement. Correct per spec. - The `if [ -n "$REQUIRED_HEADINGS" ]` guard correctly gates the type-based enforcement block and allows fallthrough to tag-based when no type matched. - Trap handling: The type-based path sets `trap 'rm -f "$TMPFILE"; exit 0' ERR` after `mktemp`, which overwrites the global `trap 'exit 0' ERR` from line 18. This is fine -- the type-based path always exits before reaching the tag-based path, and the tag-based path sets its own trap after its own `mktemp`. No trap leakage. - HTML entity decoding (`sed` pipeline) is identical between both paths. Consistent. - `grep -qiF` for heading matching is case-insensitive and literal. This matches the existing pattern. Correct. **Potential edge case**: If `note_type` is set to something like `review` but `CONTENT` is non-empty and contains all headings, the type-based path allows and exits. If `CONTENT` is empty, it denies with a helpful message referencing the template slug. Both paths are correct. ### BLOCKERS **1. No automated tests for 6 new enforcement paths.** The PR adds 6 new `case` branches, each with hardcoded heading lists. The repo has an established test pattern (`tests/test_block_groupme_send.sh` -- 21 tests for a similar PreToolUse hook). The PR body documents thorough manual testing, but no automated test file was added. This is a BLOCKER per the criteria: "New functionality with zero test coverage." With 6 distinct heading lists hardcoded inline, a test file (`tests/test_check_note_template.sh`) should verify: - Each of the 6 types denies when required headings are missing - Each of the 6 types allows when all headings are present - `doc` type passes through - `todo`/`bug` hard-block still works - Empty content denies with correct template slug reference - Unknown `note_type` falls through to tag-based routing The GroupMe test file provides a clear pattern to follow. The heading lists are the most drift-prone part of this hook (if templates change, inline headings could diverge), so automated tests are essential. ### NITS **1. DRY opportunity: heading validation loop is duplicated.** The `while IFS= read -r heading` loop + `MISSING` accumulation + deny JSON block appears in both the type-based section (~lines 107-130 in the new file) and the tag-based section (~lines 225-248). These could be extracted into a `validate_headings()` function. Both paths share the same pattern: write content to tmpfile, decode HTML entities, loop over headings, check with `grep -qiF`, build MISSING string, deny if non-empty. This is a nit rather than a BLOCKER because the duplication is between two distinct routing paths (not auth/security logic), and the tag-based path has the additional curl+jq extraction that the type-based path does not. **2. Comment header could reference the matcher.** The script header comment says `# Matcher: mcp__pal-e-docs__create_note` which is accurate per `settings.json`. Good -- no issue here, just confirming it is correct. **3. Hardcoded headings are a maintenance surface.** The 6 heading lists are hardcoded inline. If template notes in pal-e-docs change, these inline lists must be updated manually. The comment on line ~55 ("These match the template notes in pal-e-docs") is helpful, but there is no automated check that they stay in sync. Consider a future follow-up to either generate these from the templates or add a CI check. Not blocking for this PR. ### SOP COMPLIANCE - [x] Branch named after issue: `183-expand-check-note-template` matches issue #183 - [x] PR body has: Summary, Changes, Test Plan, Related -- all present and thorough - [x] Related references parent spike #180 - [x] No secrets committed -- no credentials, API keys, or .env files in the diff - [x] No unnecessary file changes -- single file modified, matches issue scope - [x] Commit messages: single commit implied by PR structure - [x] `check-issue-template.sh` and `check-board-item.sh` not touched (per issue constraint) ### PROCESS OBSERVATIONS - **Change failure risk**: LOW for the type-based routing itself (fail-open design means even a bug allows the note through). MEDIUM for heading list accuracy -- if any heading list is wrong, the hook will reject valid notes or allow invalid ones. - **Deployment frequency impact**: Positive. Inline headings eliminate 6 API calls per note creation for the new types, reducing latency. - **Documentation**: The PR body is thorough. The script header comments accurately describe the routing hierarchy. ### VERDICT: NOT APPROVED One BLOCKER: missing automated tests for 6 new enforcement paths. The repo has an established test pattern (`tests/test_block_groupme_send.sh`) and the same pattern should be applied here. Add `tests/test_check_note_template.sh` covering the scenarios listed above, then re-request review.

forgejo_admin added 1 commit

2026-03-28 11:32:09 +00:00

test: add 23 tests for check-note-template type-based enforcement 9bd40aebef

Covers allow/deny for all 6 new types (review, architecture, user-story,
sop, convention, validation), doc pass-through, todo/bug hard-block,
empty content denial, unknown type fallthrough, html_content fallback,
and HTML entity decoding. 45 assertions total.

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

forgejo_admin commented

2026-03-28 11:32:19 +00:00

Author

Contributor

QA blocker resolved: added tests/test_check_note_template.sh with 23 tests (45 assertions).

Coverage:

Allow/deny for all 6 type-based paths: review, architecture, user-story, sop, convention, validation
doc pass-through (including empty content)
todo/bug hard-block with alternative suggestion
Empty content denial for typed notes (review, sop, architecture)
Unknown note_type fallthrough
No note_type + no tags fallthrough
html_content field fallback
HTML entity decoding

All 45 assertions pass. Existing test suites unaffected.

QA blocker resolved: added `tests/test_check_note_template.sh` with 23 tests (45 assertions). **Coverage:** - Allow/deny for all 6 type-based paths: review, architecture, user-story, sop, convention, validation - `doc` pass-through (including empty content) - `todo`/`bug` hard-block with alternative suggestion - Empty content denial for typed notes (review, sop, architecture) - Unknown `note_type` fallthrough - No `note_type` + no tags fallthrough - `html_content` field fallback - HTML entity decoding All 45 assertions pass. Existing test suites unaffected.

forgejo_admin commented

2026-03-28 11:35:51 +00:00

Author

Contributor

PR #200 Review

Re-review after fix commit adding tests/test_check_note_template.sh with 23 tests.

DOMAIN REVIEW

Tech stack: Bash hooks (PreToolUse enforcement for Claude Code), shell test harness.

Hook logic (hooks/check-note-template.sh):

The case statement for type-based routing is well-structured. Each type maps to a TEMPLATE_SLUG and inline REQUIRED_HEADINGS, avoiding unnecessary API calls -- a clear performance improvement over the tag-based path.
doc pass-through exits before content validation, which is correct and intentional.
The variable rename from NOTE_TYPE_INPUT to NOTE_TYPE and from NOTE_TYPE to TAG_TYPE in the tag-based section cleanly separates the two routing paths and avoids variable shadowing.
grep -qiF for case-insensitive fixed-string matching on headings is correct.
HTML entity decoding (sed pipeline) in the new type-based section is identical to the existing tag-based section. Consistent.
Fail-open behavior is preserved: trap 'exit 0' ERR at the top, trap 'rm -f "$TMPFILE"; exit 0' ERR when a temp file exists. Both paths handle cleanup correctly.
The while IFS= read -r heading loop correctly skips empty lines.

Test file (tests/test_check_note_template.sh):

23 tests covering: all 6 types allow (Tests 1,3,5,7,9,11), all 6 types deny (Tests 2,4,6,8,10,12), doc pass-through with and without content (Tests 13,14), todo/bug hard-block (Tests 15,16), empty content denial for two types (Tests 17,18), unknown type fallthrough (Test 19), no-type/no-tags fallthrough (Test 20), missing content field entirely (Test 21), html_content fallback (Test 22), HTML entity decoding (Test 23).
Test structure follows the existing pattern established in tests/test_block_groupme_send.sh: same assert_contains, assert_deny/assert_empty helpers, same summary format. Good consistency.
Tests exercise the hook purely via stdin JSON piped to bash "$HOOK", requiring no API calls or network access. Fully self-contained and CI-friendly.
Deny tests verify three properties each: the decision is "deny", the missing section name appears in the reason, and the correct template slug is referenced. Thorough.

BLOCKERS

Previous blocker (no automated tests): RESOLVED. The fix commit adds 23 tests covering all 6 new enforcement paths, doc pass-through, hard-blocks, empty content, unknown types, content field fallback, and HTML entity decoding. The test coverage is comprehensive -- happy paths, error paths, and edge cases are all covered.

No new blockers identified.

NITS

DRY opportunity: The heading-validation logic (create tmpfile, decode entities, loop with grep -qiF, build MISSING string, cleanup, deny JSON) is duplicated between the type-based path (lines ~85-130 in the new code) and the tag-based path (lines ~215-245). A shared validate_headings() function taking CONTENT, REQUIRED_HEADINGS, NOTE_TYPE, and TEMPLATE_SLUG as arguments would eliminate ~30 lines of duplication. Not a blocker since this is not an auth/security path and both copies are tested, but worth tracking for a future cleanup.
Inline heading drift risk: The required headings for 6 types are hardcoded in the hook script. If the template notes in pal-e-docs change, these inline lists must be updated manually. A comment noting this coupling exists (e.g., "Keep in sync with template-{type} notes") would help future maintainers. The header comment does say "These match the template notes in pal-e-docs" but does not call out the manual sync requirement explicitly.
No markdown ### format test: Test 23 covers HTML entity decoding, but no test verifies that plain markdown ### Heading format is matched (the hook comment on line ~93 in the diff says "Support both HTML <h3> and markdown ### Heading formats"). The grep -qiF on decoded content would match ### Heading since it just searches for the heading text string, so it works implicitly, but an explicit test would document this expectation. Very minor.

SOP COMPLIANCE

Branch named after issue: 183-expand-check-note-template references issue #183
PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present
Related references parent: References issue #183 and parent spike #180
No secrets committed: No credentials, API keys, or .env files in the diff
Scope is tight: Only 2 files changed (hooks/check-note-template.sh + tests/test_check_note_template.sh), matching the issue file targets
check-issue-template.sh and check-board-item.sh not touched, as specified in the issue

PROCESS OBSERVATIONS

Deployment frequency: Clean, scoped change. Two files, one feature. Good for fast merge.
Change failure risk: Low. Fail-open design means a bug in the hook allows rather than blocks. The 23 tests provide a regression safety net.
Test pattern established: This is the second hook test file in the repo (after test_block_groupme_send.sh). The consistent test harness pattern (assert helpers + summary + exit code) is becoming a convention worth documenting.
Nit 1 (DRY) and Nit 2 (heading drift) should be tracked as discovered scope for a future cleanup issue.

VERDICT: APPROVED

## PR #200 Review Re-review after fix commit adding `tests/test_check_note_template.sh` with 23 tests. ### DOMAIN REVIEW **Tech stack**: Bash hooks (PreToolUse enforcement for Claude Code), shell test harness. **Hook logic (`hooks/check-note-template.sh`)**: - The `case` statement for type-based routing is well-structured. Each type maps to a `TEMPLATE_SLUG` and inline `REQUIRED_HEADINGS`, avoiding unnecessary API calls -- a clear performance improvement over the tag-based path. - `doc` pass-through exits before content validation, which is correct and intentional. - The variable rename from `NOTE_TYPE_INPUT` to `NOTE_TYPE` and from `NOTE_TYPE` to `TAG_TYPE` in the tag-based section cleanly separates the two routing paths and avoids variable shadowing. - `grep -qiF` for case-insensitive fixed-string matching on headings is correct. - HTML entity decoding (`sed` pipeline) in the new type-based section is identical to the existing tag-based section. Consistent. - Fail-open behavior is preserved: `trap 'exit 0' ERR` at the top, `trap 'rm -f "$TMPFILE"; exit 0' ERR` when a temp file exists. Both paths handle cleanup correctly. - The `while IFS= read -r heading` loop correctly skips empty lines. **Test file (`tests/test_check_note_template.sh`)**: - 23 tests covering: all 6 types allow (Tests 1,3,5,7,9,11), all 6 types deny (Tests 2,4,6,8,10,12), doc pass-through with and without content (Tests 13,14), todo/bug hard-block (Tests 15,16), empty content denial for two types (Tests 17,18), unknown type fallthrough (Test 19), no-type/no-tags fallthrough (Test 20), missing content field entirely (Test 21), `html_content` fallback (Test 22), HTML entity decoding (Test 23). - Test structure follows the existing pattern established in `tests/test_block_groupme_send.sh`: same `assert_contains`, `assert_deny`/`assert_empty` helpers, same summary format. Good consistency. - Tests exercise the hook purely via stdin JSON piped to `bash "$HOOK"`, requiring no API calls or network access. Fully self-contained and CI-friendly. - Deny tests verify three properties each: the decision is "deny", the missing section name appears in the reason, and the correct template slug is referenced. Thorough. ### BLOCKERS **Previous blocker (no automated tests): RESOLVED.** The fix commit adds 23 tests covering all 6 new enforcement paths, doc pass-through, hard-blocks, empty content, unknown types, content field fallback, and HTML entity decoding. The test coverage is comprehensive -- happy paths, error paths, and edge cases are all covered. No new blockers identified. ### NITS 1. **DRY opportunity**: The heading-validation logic (create tmpfile, decode entities, loop with `grep -qiF`, build MISSING string, cleanup, deny JSON) is duplicated between the type-based path (lines ~85-130 in the new code) and the tag-based path (lines ~215-245). A shared `validate_headings()` function taking `CONTENT`, `REQUIRED_HEADINGS`, `NOTE_TYPE`, and `TEMPLATE_SLUG` as arguments would eliminate ~30 lines of duplication. Not a blocker since this is not an auth/security path and both copies are tested, but worth tracking for a future cleanup. 2. **Inline heading drift risk**: The required headings for 6 types are hardcoded in the hook script. If the template notes in pal-e-docs change, these inline lists must be updated manually. A comment noting this coupling exists (e.g., "Keep in sync with template-{type} notes") would help future maintainers. The header comment does say "These match the template notes in pal-e-docs" but does not call out the manual sync requirement explicitly. 3. **No markdown `###` format test**: Test 23 covers HTML entity decoding, but no test verifies that plain markdown `### Heading` format is matched (the hook comment on line ~93 in the diff says "Support both HTML `<h3>` and markdown `### Heading` formats"). The `grep -qiF` on decoded content would match `### Heading` since it just searches for the heading text string, so it works implicitly, but an explicit test would document this expectation. Very minor. ### SOP COMPLIANCE - [x] Branch named after issue: `183-expand-check-note-template` references issue #183 - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present - [x] Related references parent: References issue #183 and parent spike #180 - [x] No secrets committed: No credentials, API keys, or `.env` files in the diff - [x] Scope is tight: Only 2 files changed (`hooks/check-note-template.sh` + `tests/test_check_note_template.sh`), matching the issue file targets - [x] `check-issue-template.sh` and `check-board-item.sh` not touched, as specified in the issue ### PROCESS OBSERVATIONS - **Deployment frequency**: Clean, scoped change. Two files, one feature. Good for fast merge. - **Change failure risk**: Low. Fail-open design means a bug in the hook allows rather than blocks. The 23 tests provide a regression safety net. - **Test pattern established**: This is the second hook test file in the repo (after `test_block_groupme_send.sh`). The consistent test harness pattern (assert helpers + summary + exit code) is becoming a convention worth documenting. - **Nit 1 (DRY)** and **Nit 2 (heading drift)** should be tracked as discovered scope for a future cleanup issue. ### VERDICT: APPROVED