ldraney/claude-custom
1
0
Fork 0
Code Issues 32 Pull requests Projects Releases Packages Wiki Activity Actions

Fix sed entity decode order and grep portability across hooks #66

Merged
forgejo_admin merged 1 commit from 65-hook-hardening into main 2026-03-07 23:20:05 +00:00
Conversation 1 Commits 1 Files changed 5 +16 -12
forgejo_admin commented 2026-03-07 23:18:13 +00:00
Contributor
Copy link

Summary

Fixes three issues found during QA of PR #64: sed & decode ordering bug that could cause double-decoding, GNU-specific grep \| syntax that fails on non-GNU systems, and missing documentation for hardcoded fields in check-phase-template.sh.

Changes

  • hooks/check-issue-template.sh — moved & decoding to last position in sed chain
  • hooks/check-note-template.sh — moved & decoding to last position in both sed chains
  • hooks/check-phase-template.sh — moved & decoding to last position, replaced grep -qi with grep -Eqi and \| with | for POSIX portability, added comment explaining why field checks are hardcoded
  • hooks/check-pr-template.sh — moved & decoding to last position in sed chain
  • hooks/session-start-context.sh — moved & decoding to last position in both sed chains (was first, which was a real double-decode bug)

Test Plan

  • Manual test: create a Forgejo issue -- still validated by check-issue-template.sh
  • Manual test: create a plan note -- still validated by check-note-template.sh
  • Manual test: create a phase note -- still validated by check-phase-template.sh
  • Verify bash -n passes on all modified hooks (done pre-commit)
  • Verify no other hooks contain \| in grep patterns (scanned, none found)

Review Checklist

  • All sed HTML entity decode chains place & last to prevent double-decoding
  • All grep alternation uses -E flag with | instead of BRE \|
  • check-phase-template.sh documents why fields are hardcoded
  • No unrelated changes
  • bash -n syntax check passes on all modified files

Related

  • Plan: plan-2026-03-07-note-hierarchy-conventions, Phase 4-1
  • Forgejo issue: #65
## Summary Fixes three issues found during QA of PR #64: sed `&` decode ordering bug that could cause double-decoding, GNU-specific grep `\|` syntax that fails on non-GNU systems, and missing documentation for hardcoded fields in check-phase-template.sh. ## Changes - `hooks/check-issue-template.sh` — moved `&` decoding to last position in sed chain - `hooks/check-note-template.sh` — moved `&` decoding to last position in both sed chains - `hooks/check-phase-template.sh` — moved `&` decoding to last position, replaced `grep -qi` with `grep -Eqi` and `\|` with `|` for POSIX portability, added comment explaining why field checks are hardcoded - `hooks/check-pr-template.sh` — moved `&` decoding to last position in sed chain - `hooks/session-start-context.sh` — moved `&` decoding to last position in both sed chains (was first, which was a real double-decode bug) ## Test Plan - [ ] Manual test: create a Forgejo issue -- still validated by check-issue-template.sh - [ ] Manual test: create a plan note -- still validated by check-note-template.sh - [ ] Manual test: create a phase note -- still validated by check-phase-template.sh - [ ] Verify `bash -n` passes on all modified hooks (done pre-commit) - [ ] Verify no other hooks contain `\|` in grep patterns (scanned, none found) ## Review Checklist - [x] All sed HTML entity decode chains place `&` last to prevent double-decoding - [x] All grep alternation uses `-E` flag with `|` instead of BRE `\|` - [x] check-phase-template.sh documents why fields are hardcoded - [x] No unrelated changes - [x] `bash -n` syntax check passes on all modified files ## Related - Plan: `plan-2026-03-07-note-hierarchy-conventions`, Phase 4-1 - Forgejo issue: #65
Move & decoding to last position in all sed HTML entity decode
chains to prevent double-decoding when &amp;lt; becomes &lt; then <.
This was a real bug in session-start-context.sh where &amp; was decoded
first. Also fixes check-phase-template.sh grep portability: replace
GNU-specific BRE \| with POSIX -E flag. Adds comment explaining why
phase template fields are hardcoded (HTML <strong> vs markdown ###).

Closes #65

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
forgejo_admin commented 2026-03-07 23:19:28 +00:00
Author
Contributor
Copy link

PR #66 Review

BLOCKERS

None.

NITS

None.

Code Review

Fix 1: sed ampersand decode ordering -- Verified across all 5 files (7 sed chains total). Every chain now places s/&amp;/\&/g as the last substitution. This is correct: if &amp; is decoded first, subsequent &lt; -> < replacements would turn any literal &amp;lt; in the source into &lt; then <, causing double-decoding. By decoding &amp; last, the other entity names are consumed first and the ampersand substitution only touches true remaining &amp; literals.

Fix 2: grep -E portability -- All 4 grep alternation patterns in check-phase-template.sh (lines 62-64, 79) now use grep -Eqi with | instead of BRE \|. Scanned the entire hooks directory -- no remaining instances of \| in grep patterns. Other hooks already used -E or -qE consistently.

Fix 3: Hardcoded fields comment -- The 4-line comment at lines 45-49 of check-phase-template.sh explains why Goal:, Owner:, Repo:, Depends on: are hardcoded rather than dynamically extracted. The reasoning is sound: phase templates use <strong> tags, not markdown ### headings, so the extraction approach used by check-issue-template.sh and check-note-template.sh does not apply.

Scope check -- No unrelated changes. The diff is exactly 5 files, +16/-12 lines, all within the stated scope. No new functionality, no behavioral changes beyond the bug fixes.

No secrets -- No .env files, credentials, or sensitive data in the diff.

SOP COMPLIANCE

  • Branch named after issue (65-hook-hardening references issue #65)
  • PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related)
  • Related section references plan slug (plan-2026-03-07-note-hierarchy-conventions, Phase 4-1)
  • Related references issue #65
  • No secrets committed
  • No scope creep -- changes are strictly the three fixes described

VERDICT: APPROVED

## PR #66 Review ### BLOCKERS None. ### NITS None. ### Code Review **Fix 1: sed ampersand decode ordering** -- Verified across all 5 files (7 sed chains total). Every chain now places `s/&amp;/\&/g` as the **last** substitution. This is correct: if `&amp;` is decoded first, subsequent `&lt;` -> `<` replacements would turn any literal `&amp;lt;` in the source into `&lt;` then `<`, causing double-decoding. By decoding `&amp;` last, the other entity names are consumed first and the ampersand substitution only touches true remaining `&amp;` literals. **Fix 2: grep -E portability** -- All 4 grep alternation patterns in `check-phase-template.sh` (lines 62-64, 79) now use `grep -Eqi` with `|` instead of BRE `\|`. Scanned the entire hooks directory -- no remaining instances of `\|` in grep patterns. Other hooks already used `-E` or `-qE` consistently. **Fix 3: Hardcoded fields comment** -- The 4-line comment at lines 45-49 of `check-phase-template.sh` explains why `Goal:`, `Owner:`, `Repo:`, `Depends on:` are hardcoded rather than dynamically extracted. The reasoning is sound: phase templates use `<strong>` tags, not markdown `###` headings, so the extraction approach used by check-issue-template.sh and check-note-template.sh does not apply. **Scope check** -- No unrelated changes. The diff is exactly 5 files, +16/-12 lines, all within the stated scope. No new functionality, no behavioral changes beyond the bug fixes. **No secrets** -- No `.env` files, credentials, or sensitive data in the diff. ### SOP COMPLIANCE - [x] Branch named after issue (`65-hook-hardening` references issue #65) - [x] PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related) - [x] Related section references plan slug (`plan-2026-03-07-note-hierarchy-conventions`, Phase 4-1) - [x] Related references issue #65 - [x] No secrets committed - [x] No scope creep -- changes are strictly the three fixes described ### VERDICT: APPROVED
forgejo_admin deleted branch 65-hook-hardening 2026-03-07 23:20:05 +00:00
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
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!66
No description provided.