ldraney/claude-custom

Fork 0

Fix enforcement nits: dottie config, hook bugs, new safety hooks #84

New issue

Closed

opened 2026-03-13 20:09:54 +00:00 by forgejo_admin · 2 comments

forgejo_admin commented

2026-03-13 20:09:54 +00:00

Contributor

Lineage

plan-pal-e-agency → Phase 2 (Enforcement nits)

Repo

forgejo_admin/claude-custom

User Story

As the agency operating model,
I want all hooks and agent configs to be correct and complete,
So that the four-agent model works reliably without manual workarounds.

Context

Six small fixes accumulated as TODOs/bugs. All target hooks or agent configs in claude-custom. Grouped into one PR since they're all small and independent.

File Targets

Files to modify:

schemas/agent-spawn-requirements.json — Add "dottie" as recognized agent type (currently missing, spawn gate blocks Dottie spawns)
hooks/remind-mcp-review-loop.sh — Line 10 references mcp__pal-e-docs__get_note(slug="pr-review-loop") but dev/QA agents have no pal-e-docs access. Replace with plain text guidance: "Follow the review-fix loop: review, post findings, stop."
hooks/check-note-template.sh — Lines 91-98 use echo "$DECODED_CONTENT" | grep which silently truncates large HTML. Fix: write content to a temp file, grep from file. See root cause analysis below.

Files to create:

hooks/warn-delete-note.sh — New PreToolUse hook that warns before mcp__pal-e-docs__delete_note calls. Should output a reminder to follow sop-note-deletion (backup first). Exit 0 (warn, don't block).

Files to NOT touch:

agents/dev.md, agents/qa.md — These are correct
settings.json — Only touch if adding the new warn-delete-note hook to the PreToolUse hooks list

Fix Details

1. Dottie agent type (schemas/agent-spawn-requirements.json)
Add "dottie" with spawn requirements: plan ref required, no issue ref needed (Dottie does doc work, not repo work). Match the structure of existing entries (dev, qa, etc.).

2. Stale pal-e-docs reference (hooks/remind-mcp-review-loop.sh)
Line 10 tells agents to call mcp__pal-e-docs__get_note — a tool dev/QA agents don't have. Replace with plain English instructions about the review-fix loop.

3. Large HTML truncation (hooks/check-note-template.sh)
Root cause: echo "$DECODED_CONTENT" | grep hits shell argument limits on multi-KB HTML. Fix:

TMPFILE=$(mktemp)
echo "$CONTENT" | sed 's/&lt;/</g; s/&gt;/>/g; s/&amp;/\&/g; s/&quot;/"/g' > "$TMPFILE"
# Use: grep -qiF "$heading" "$TMPFILE"
# Cleanup: rm -f "$TMPFILE"

4. Delete note warning hook (hooks/warn-delete-note.sh)
New PreToolUse hook. Trigger: tool_name matches mcp__pal-e-docs__delete_note. Output: "WARNING: Follow sop-note-deletion before deleting notes. Steps: 1) Force WAL switch, 2) JSON export, 3) pg_dump, 4) Then delete. Backups go to ~/backups/." Exit 0 (advisory, not blocking).

Acceptance Criteria

"dottie" recognized by spawn gate — spawning with subagent_type: "dottie" no longer errors
remind-mcp-review-loop.sh contains no mcp__pal-e-docs references
A plan note with 5+ phases and detailed tables passes check-note-template.sh when all required sections are present
check-note-template.sh still correctly rejects notes that genuinely miss required sections
warn-delete-note.sh outputs warning text when delete_note is called
Hook registered in settings.json (if needed based on existing pattern)

Test Expectations

Manual verification: each hook behavior can be tested by examining the shell script logic
The temp-file fix for check-note-template.sh should handle content with special characters and HTML entities

Constraints

Hooks are hardlinked to ~/.claude/hooks/ — the repo files ARE the live hooks
Follow existing hook patterns (check exit codes: 0 = allow/warn, 2 = block)
settings.json hook registration uses the pattern in existing PreToolUse entries

Checklist

PR opened
Tests pass
No unrelated changes

project-pal-e-agency — project this affects

### Lineage `plan-pal-e-agency` → Phase 2 (Enforcement nits) ### Repo `forgejo_admin/claude-custom` ### User Story As the agency operating model, I want all hooks and agent configs to be correct and complete, So that the four-agent model works reliably without manual workarounds. ### Context Six small fixes accumulated as TODOs/bugs. All target hooks or agent configs in claude-custom. Grouped into one PR since they're all small and independent. ### File Targets Files to modify: - `schemas/agent-spawn-requirements.json` — Add "dottie" as recognized agent type (currently missing, spawn gate blocks Dottie spawns) - `hooks/remind-mcp-review-loop.sh` — Line 10 references `mcp__pal-e-docs__get_note(slug="pr-review-loop")` but dev/QA agents have no pal-e-docs access. Replace with plain text guidance: "Follow the review-fix loop: review, post findings, stop." - `hooks/check-note-template.sh` — Lines 91-98 use `echo "$DECODED_CONTENT" | grep` which silently truncates large HTML. Fix: write content to a temp file, grep from file. See root cause analysis below. Files to create: - `hooks/warn-delete-note.sh` — New PreToolUse hook that warns before `mcp__pal-e-docs__delete_note` calls. Should output a reminder to follow sop-note-deletion (backup first). Exit 0 (warn, don't block). Files to NOT touch: - `agents/dev.md`, `agents/qa.md` — These are correct - `settings.json` — Only touch if adding the new warn-delete-note hook to the PreToolUse hooks list ### Fix Details **1. Dottie agent type (`schemas/agent-spawn-requirements.json`)** Add `"dottie"` with spawn requirements: plan ref required, no issue ref needed (Dottie does doc work, not repo work). Match the structure of existing entries (dev, qa, etc.). **2. Stale pal-e-docs reference (`hooks/remind-mcp-review-loop.sh`)** Line 10 tells agents to call `mcp__pal-e-docs__get_note` — a tool dev/QA agents don't have. Replace with plain English instructions about the review-fix loop. **3. Large HTML truncation (`hooks/check-note-template.sh`)** Root cause: `echo "$DECODED_CONTENT" | grep` hits shell argument limits on multi-KB HTML. Fix: ```bash TMPFILE=$(mktemp) echo "$CONTENT" | sed 's/</</g; s/>/>/g; s/&/\&/g; s/"/"/g' > "$TMPFILE" # Use: grep -qiF "$heading" "$TMPFILE" # Cleanup: rm -f "$TMPFILE" ``` **4. Delete note warning hook (`hooks/warn-delete-note.sh`)** New PreToolUse hook. Trigger: tool_name matches `mcp__pal-e-docs__delete_note`. Output: "WARNING: Follow sop-note-deletion before deleting notes. Steps: 1) Force WAL switch, 2) JSON export, 3) pg_dump, 4) Then delete. Backups go to ~/backups/." Exit 0 (advisory, not blocking). Register in `settings.json` under `hooks.preToolUse` if needed (check existing hook registration pattern). ### Acceptance Criteria - [ ] `"dottie"` recognized by spawn gate — spawning with `subagent_type: "dottie"` no longer errors - [ ] `remind-mcp-review-loop.sh` contains no `mcp__pal-e-docs` references - [ ] A plan note with 5+ phases and detailed tables passes `check-note-template.sh` when all required sections are present - [ ] `check-note-template.sh` still correctly rejects notes that genuinely miss required sections - [ ] `warn-delete-note.sh` outputs warning text when `delete_note` is called - [ ] Hook registered in settings.json (if needed based on existing pattern) ### Test Expectations - [ ] Manual verification: each hook behavior can be tested by examining the shell script logic - [ ] The temp-file fix for check-note-template.sh should handle content with special characters and HTML entities ### Constraints - Hooks are hardlinked to `~/.claude/hooks/` — the repo files ARE the live hooks - Follow existing hook patterns (check exit codes: 0 = allow/warn, 2 = block) - `settings.json` hook registration uses the pattern in existing PreToolUse entries ### Checklist - [ ] PR opened - [ ] Tests pass - [ ] No unrelated changes ### Related - `project-pal-e-agency` — project this affects

forgejo_admin referenced this issue from a commit

2026-03-13 20:13:20 +00:00

Fix enforcement nits: dottie config, hook bugs, new safety hooks

forgejo_admin referenced this issue from a pull request that will close it,

2026-03-13 20:13:54 +00:00

Fix enforcement nits: dottie config, hook bugs, new safety hooks #85

forgejo_admin commented

2026-03-13 20:16:27 +00:00

Author

Contributor

PR #85 Review

BLOCKERS

1. SubagentStart matcher does not include "dottie" -- context injection will silently fail

The spawn schema (schemas/agent-spawn-requirements.json) now recognizes "dottie" as a valid agent type, so the spawn gate (check-agent-spawn.sh) will allow it. However, the SubagentStart matcher in settings.json line 216 is still "qa|dev|general-purpose" -- it does not include "dottie". And inject-subagent-context.sh line 18 only handles qa, dev, and general-purpose in its case statement (no "dottie" branch).

This means spawning an agent with subagent_type: "dottie" will:

Pass the spawn gate (good)
NOT trigger SubagentStart context injection (the hook matcher won't fire)
The Dottie agent will start without the profile injection ("You are Dottie, the doc librarian...")

This is a functional gap: the PR adds the spawn gate entry but not the SubagentStart plumbing. The agents/dottie.md config file exists, which provides frontmatter constraints, but the runtime context injection is missing for the "dottie" type.

Fix: Add "dottie" to the SubagentStart matcher ("qa|dev|general-purpose|dottie") and add a dottie) case in inject-subagent-context.sh. Alternatively, if Dottie is always spawned as general-purpose and the schema entry is only a fallback, document that decision in the schema description.

NITS

1. Temp file trap override is correct but could be cleaner

In check-note-template.sh, the second trap on line 92 overrides the global ERR trap from line 18. This works correctly (cleanup + fail-open), but the override is implicit and easy to miss. A comment like # Override global ERR trap to include tmpfile cleanup would improve readability.

2. block-docs-writes.sh still references stale sprint tools (out of scope but worth tracking)

The block-docs-writes.sh blocklist still has the old sprint tools (create_sprint, update_sprint, etc.) but is missing the new board tools (create_board, update_board, delete_board, create_board_item, update_board_item, remove_board_item, bulk_move_board_items). This is NOT in scope for this PR, but it means the security boundary has a gap for board writes by non-Dottie agents. Recommend filing a follow-up issue.

3. delete_project missing from block-docs-writes.sh (out of scope)

Similarly, mcp__pal-e-docs__delete_project and mcp__pal-e-docs__delete_board are not in the blocklist. Same recommendation: follow-up issue.

CODE QUALITY

All five changed files are well-structured:

warn-delete-note.sh -- Clean advisory hook. Correct use of permissionDecision: "allow" with a warning reason. Exit 0 is correct for non-blocking. The tool_name guard is defensive and appropriate since the matcher already filters, but belt-and-suspenders is fine for hooks.
check-note-template.sh temp file fix -- Correct fix for the shell argument limit bug. Using mktemp + grep from file avoids ARG_MAX on large HTML. Cleanup is handled on both happy path (rm -f) and error path (trap).
remind-mcp-review-loop.sh -- Good removal of the mcp__pal-e-docs__get_note reference. Dev/QA agents cannot access pal-e-docs, so this was a broken reference. The replacement text is actionable.
agent-spawn-requirements.json -- The "dottie" entry correctly mirrors "general-purpose" with plan- pattern and isolation: false.
settings.json -- The new delete_note matcher and hook registration follow the existing pattern exactly.

SOP COMPLIANCE

Branch named after issue (84-fix-enforcement-nits references issue #84)
PR body has: Summary, Changes, Test Plan, Related
Related section references plan slug (plan-pal-e-agency)
PR body includes Closes #84
No secrets, .env files, or credentials committed
No unnecessary file changes -- all 5 files are within scope of the 4 nits described in the issue title
Test plan is present and describes manual verification steps

VERDICT: NOT APPROVED

One blocker: the SubagentStart matcher and inject-subagent-context.sh case statement need to handle the "dottie" agent type, or the schema description needs to clarify that Dottie is always spawned as general-purpose. Without this fix, spawning a dottie-typed agent skips context injection silently.

## PR #85 Review ### BLOCKERS **1. SubagentStart matcher does not include "dottie" -- context injection will silently fail** The spawn schema (`schemas/agent-spawn-requirements.json`) now recognizes `"dottie"` as a valid agent type, so the spawn gate (`check-agent-spawn.sh`) will allow it. However, the `SubagentStart` matcher in `settings.json` line 216 is still `"qa|dev|general-purpose"` -- it does not include `"dottie"`. And `inject-subagent-context.sh` line 18 only handles `qa`, `dev`, and `general-purpose` in its case statement (no `"dottie"` branch). This means spawning an agent with `subagent_type: "dottie"` will: - Pass the spawn gate (good) - NOT trigger SubagentStart context injection (the hook matcher won't fire) - The Dottie agent will start without the profile injection ("You are Dottie, the doc librarian...") This is a functional gap: the PR adds the spawn gate entry but not the SubagentStart plumbing. The `agents/dottie.md` config file exists, which provides frontmatter constraints, but the runtime context injection is missing for the `"dottie"` type. **Fix:** Add `"dottie"` to the SubagentStart matcher (`"qa|dev|general-purpose|dottie"`) and add a `dottie)` case in `inject-subagent-context.sh`. Alternatively, if Dottie is always spawned as `general-purpose` and the schema entry is only a fallback, document that decision in the schema description. ### NITS **1. Temp file trap override is correct but could be cleaner** In `check-note-template.sh`, the second `trap` on line 92 overrides the global ERR trap from line 18. This works correctly (cleanup + fail-open), but the override is implicit and easy to miss. A comment like `# Override global ERR trap to include tmpfile cleanup` would improve readability. **2. `block-docs-writes.sh` still references stale sprint tools (out of scope but worth tracking)** The `block-docs-writes.sh` blocklist still has the old sprint tools (`create_sprint`, `update_sprint`, etc.) but is missing the new board tools (`create_board`, `update_board`, `delete_board`, `create_board_item`, `update_board_item`, `remove_board_item`, `bulk_move_board_items`). This is NOT in scope for this PR, but it means the security boundary has a gap for board writes by non-Dottie agents. Recommend filing a follow-up issue. **3. `delete_project` missing from `block-docs-writes.sh` (out of scope)** Similarly, `mcp__pal-e-docs__delete_project` and `mcp__pal-e-docs__delete_board` are not in the blocklist. Same recommendation: follow-up issue. ### CODE QUALITY All five changed files are well-structured: - **`warn-delete-note.sh`** -- Clean advisory hook. Correct use of `permissionDecision: "allow"` with a warning reason. Exit 0 is correct for non-blocking. The `tool_name` guard is defensive and appropriate since the matcher already filters, but belt-and-suspenders is fine for hooks. - **`check-note-template.sh` temp file fix** -- Correct fix for the shell argument limit bug. Using `mktemp` + `grep` from file avoids `ARG_MAX` on large HTML. Cleanup is handled on both happy path (`rm -f`) and error path (trap). - **`remind-mcp-review-loop.sh`** -- Good removal of the `mcp__pal-e-docs__get_note` reference. Dev/QA agents cannot access pal-e-docs, so this was a broken reference. The replacement text is actionable. - **`agent-spawn-requirements.json`** -- The `"dottie"` entry correctly mirrors `"general-purpose"` with `plan-` pattern and `isolation: false`. - **`settings.json`** -- The new `delete_note` matcher and hook registration follow the existing pattern exactly. ### SOP COMPLIANCE - [x] Branch named after issue (`84-fix-enforcement-nits` references issue #84) - [x] PR body has: Summary, Changes, Test Plan, Related - [x] Related section references plan slug (`plan-pal-e-agency`) - [x] PR body includes `Closes #84` - [x] No secrets, .env files, or credentials committed - [x] No unnecessary file changes -- all 5 files are within scope of the 4 nits described in the issue title - [x] Test plan is present and describes manual verification steps ### VERDICT: NOT APPROVED One blocker: the `SubagentStart` matcher and `inject-subagent-context.sh` case statement need to handle the `"dottie"` agent type, or the schema description needs to clarify that Dottie is always spawned as `general-purpose`. Without this fix, spawning a `dottie`-typed agent skips context injection silently.

forgejo_admin commented

2026-03-13 20:19:21 +00:00

Author

Contributor

PR #85 Review (Re-review)

PREVIOUS BLOCKER STATUS

Both previously identified blockers are now fixed:

settings.json SubagentStart matcher -- Line 216 now reads "qa|dev|general-purpose|dottie". Fixed.
inject-subagent-context.sh dottie case -- Lines 28-30 add a dottie) case with appropriate context. Fixed.

BLOCKERS

None.

NITS

dottie context missing get_note and block-first convention -- The general-purpose case (line 25-26 of inject-subagent-context.sh) includes get_note(slug="agent-dottie") and the block-first access convention. The new dottie case (line 28-30) includes neither. This means a "dottie" agent type will not be told to read its own profile or follow the token-saving convention. The agents/dottie.md file exists and provides some of this context via frontmatter, so this may be intentional (the agent definition file provides the profile, the hook provides behavioral guidance). But the block-first convention is missing from both the hook context and agents/dottie.md. Non-blocking since the agent definition file compensates, but worth noting for consistency.
Duplicate Dottie identity across general-purpose and dottie -- Both general-purpose and dottie agent types in the schema serve the same role (doc librarian, plan- required, doc-updates output, isolation: false). The general-purpose type is presumably kept for backward compatibility. This is fine for now but creates a maintenance surface where changes to Dottie's spawn requirements need to be made in two places.

SOP COMPLIANCE

Branch named after issue -- 84-fix-enforcement-nits references issue #84
PR body has: Summary, Changes, Test Plan, Related -- all present and well-written
Related section references the plan slug -- plan-pal-e-agency
No secrets committed -- no .env files, credentials, or tokens in the diff
No unnecessary file changes (scope creep) -- all 6 files directly address the issue scope
Commit messages -- PR title is descriptive
Closes #84 present in PR body

CODE QUALITY NOTES

check-note-template.sh temp file fix -- Sound approach. The mktemp + file-based grep avoids ARG_MAX limits on large HTML. Trap correctly overrides to clean up on error. Normal path cleanup on line 105. No temp file leak paths.
warn-delete-note.sh -- Clean advisory hook. Correctly exits 0 (non-blocking). Extracts slug for the warning message. Proper permissionDecision: "allow" with descriptive reason.
remind-mcp-review-loop.sh -- Clean removal of mcp__pal-e-docs__get_note reference. Replaced with plain English guidance appropriate for agents without pal-e-docs access.
agent-spawn-requirements.json -- Valid JSON. dottie entry mirrors general-purpose structure correctly.
settings.json -- Valid JSON. Both the mcp__pal-e-docs__delete_note PreToolUse matcher and the SubagentStart matcher update are correctly placed.

VERDICT: APPROVED

## PR #85 Review (Re-review) ### PREVIOUS BLOCKER STATUS Both previously identified blockers are now fixed: 1. **settings.json SubagentStart matcher** -- Line 216 now reads `"qa|dev|general-purpose|dottie"`. Fixed. 2. **inject-subagent-context.sh dottie case** -- Lines 28-30 add a `dottie)` case with appropriate context. Fixed. ### BLOCKERS None. ### NITS 1. **dottie context missing `get_note` and block-first convention** -- The `general-purpose` case (line 25-26 of `inject-subagent-context.sh`) includes `get_note(slug="agent-dottie")` and the block-first access convention. The new `dottie` case (line 28-30) includes neither. This means a "dottie" agent type will not be told to read its own profile or follow the token-saving convention. The `agents/dottie.md` file exists and provides some of this context via frontmatter, so this may be intentional (the agent definition file provides the profile, the hook provides behavioral guidance). But the block-first convention is missing from both the hook context and `agents/dottie.md`. Non-blocking since the agent definition file compensates, but worth noting for consistency. 2. **Duplicate Dottie identity across general-purpose and dottie** -- Both `general-purpose` and `dottie` agent types in the schema serve the same role (doc librarian, `plan-` required, `doc-updates` output, `isolation: false`). The `general-purpose` type is presumably kept for backward compatibility. This is fine for now but creates a maintenance surface where changes to Dottie's spawn requirements need to be made in two places. ### SOP COMPLIANCE - [x] Branch named after issue -- `84-fix-enforcement-nits` references issue #84 - [x] PR body has: Summary, Changes, Test Plan, Related -- all present and well-written - [x] Related section references the plan slug -- `plan-pal-e-agency` - [x] No secrets committed -- no .env files, credentials, or tokens in the diff - [x] No unnecessary file changes (scope creep) -- all 6 files directly address the issue scope - [x] Commit messages -- PR title is descriptive - [x] `Closes #84` present in PR body ### CODE QUALITY NOTES - **check-note-template.sh temp file fix** -- Sound approach. The `mktemp` + file-based `grep` avoids ARG_MAX limits on large HTML. Trap correctly overrides to clean up on error. Normal path cleanup on line 105. No temp file leak paths. - **warn-delete-note.sh** -- Clean advisory hook. Correctly exits 0 (non-blocking). Extracts slug for the warning message. Proper `permissionDecision: "allow"` with descriptive reason. - **remind-mcp-review-loop.sh** -- Clean removal of `mcp__pal-e-docs__get_note` reference. Replaced with plain English guidance appropriate for agents without pal-e-docs access. - **agent-spawn-requirements.json** -- Valid JSON. `dottie` entry mirrors `general-purpose` structure correctly. - **settings.json** -- Valid JSON. Both the `mcp__pal-e-docs__delete_note` PreToolUse matcher and the SubagentStart matcher update are correctly placed. ### VERDICT: APPROVED

forgejo_admin closed this issue

2026-03-13 20:21:50 +00:00