MAP: executable validation criteria + fix Claude Code hooks#78
MAP: executable validation criteria + fix Claude Code hooks#78
Conversation
📝 WalkthroughWalkthroughAdds VC-prefixed validation criteria with mandatory test mappings and test-coverage tracking, standardizes hook outputs to structured JSON (hookEventName, additionalContext, permissionDecision/permissionDecisionReason) with exit code 0, and consolidates hook registration into Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR enhances the MAP Framework's validation and hook infrastructure by introducing executable validation criteria with stable identifiers (VC1, VC2, etc.) and fixing Claude Code hook configuration to match the official documentation. The changes improve quality gates by making acceptance criteria testable and traceable, while eliminating duplicate hook execution issues.
Changes:
- Migrated hooks from settings.hooks.json to settings.json and updated hook output schema to use permissionDecision/additionalContext
- Added VC-prefixed validation criteria with mandatory test mapping across task-decomposer, actor, and monitor agents
- Enforced hard-stop semantics for Monitor valid=false results
Reviewed changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| .claude/settings.json, src/mapify_cli/templates/settings.json | Added hooks configuration section; removed separate settings.hooks.json |
| .claude/settings.hooks.json, src/mapify_cli/templates/settings.hooks.json | Deleted (hooks now in settings.json) |
| .claude/hooks/.py, src/mapify_cli/templates/hooks/.py | Updated to use hookSpecificOutput schema with hookEventName, permissionDecision, additionalContext |
| .claude/hooks/block-dangerous.sh, src/mapify_cli/templates/hooks/block-dangerous.sh | Updated to use jq for JSON output and new permission decision schema |
| .claude/agents/task-decomposer.md, src/mapify_cli/templates/agents/task-decomposer.md | Added VC prefix requirement, concrete anchors, and test mapping for validation criteria |
| .claude/agents/monitor.md, src/mapify_cli/templates/agents/monitor.md | Added test coverage verification, hard-stop semantics, and VC→test evidence requirements |
| .claude/agents/actor.md, src/mapify_cli/templates/agents/actor.md | Added validation criteria coverage section and mandatory VC→test mapping |
| .claude/commands/.md, src/mapify_cli/templates/commands/.md | Added VC formatting requirements and test mapping instructions |
| src/mapify_cli/init.py | Removed settings.hooks.json from config files list; added warning for hooks in settings.local.json |
| scripts/sync-templates.sh | Removed settings.hooks.json from sync command |
| docs/USAGE.md, CLAUDE.md, CHANGELOG.md, etc. | Updated documentation references from settings.hooks.json to settings.json |
| .claude/skills/README.md, src/mapify_cli/templates/skills/README.md | Updated hook configuration references |
| .claude/references/workflow-state-schema.md, src/mapify_cli/templates/references/workflow-state-schema.md | Updated hook registration file reference |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # | ||
| # EXIT CODES: | ||
| # 0 - Allow command execution | ||
| # 2 - Block command execution (dangerous command detected) | ||
| # 0 + permissionDecision=deny - Block command execution (preferred) | ||
| # | ||
| # TESTING: | ||
| # echo '{"tool_name": "Bash", "tool_input": {"command": "rm -rf /"}}' | bash block-dangerous.sh |
There was a problem hiding this comment.
The TESTING comment in the file header says "Expected: Exit code 2" but the implementation now uses exit code 0 with permissionDecision=deny. Update line 21 in the TESTING section to reflect the new behavior: "Expected: Exit code 0 with permissionDecision=deny in JSON output".
| # | ||
| # EXIT CODES: | ||
| # 0 - Allow command execution | ||
| # 2 - Block command execution (dangerous command detected) | ||
| # 0 + permissionDecision=deny - Block command execution (preferred) | ||
| # | ||
| # TESTING: | ||
| # echo '{"tool_name": "Bash", "tool_input": {"command": "rm -rf /"}}' | bash block-dangerous.sh |
There was a problem hiding this comment.
The TESTING comment in the file header says "Expected: Exit code 2" but the implementation now uses exit code 0 with permissionDecision=deny. Update line 21 in the TESTING section to reflect the new behavior: "Expected: Exit code 0 with permissionDecision=deny in JSON output".
| "Stop": [ | ||
| { | ||
| "description": "Lightweight quality gates - only critical issues", | ||
| "hooks": [ | ||
| { | ||
| "type": "command", | ||
| "command": "\"$CLAUDE_PROJECT_DIR\"/.claude/hooks/end-of-turn.sh", | ||
| "timeout": 30, | ||
| "description": "Auto-fixes silently, only reports secrets/syntax errors" | ||
| } | ||
| ] | ||
| } | ||
| ] |
There was a problem hiding this comment.
The Stop hook configuration is missing a "matcher" field. In the deleted settings.hooks.json file (line 60), the Stop hook had "matcher": "*". All other hook event types (PreToolUse, PostToolUse, PreCompact) have matcher fields. The Stop hook should also have a matcher field for consistency and to ensure it triggers correctly.
| "Stop": [ | ||
| { | ||
| "description": "Lightweight quality gates - only critical issues", | ||
| "hooks": [ | ||
| { | ||
| "type": "command", | ||
| "command": "\"$CLAUDE_PROJECT_DIR\"/.claude/hooks/end-of-turn.sh", | ||
| "timeout": 30, | ||
| "description": "Auto-fixes silently, only reports secrets/syntax errors" | ||
| } | ||
| ] | ||
| } | ||
| ] |
There was a problem hiding this comment.
The Stop hook configuration is missing a "matcher" field. In the deleted settings.hooks.json file (line 60), the Stop hook had "matcher": "*". All other hook event types (PreToolUse, PostToolUse, PreCompact) have matcher fields. The Stop hook should also have a matcher field for consistency and to ensure it triggers correctly.
.claude/hooks/block-dangerous.sh
Outdated
| jq -n '{ | ||
| hookSpecificOutput: { | ||
| hookEventName: "PreToolUse", | ||
| permissionDecision: "deny", | ||
| permissionDecisionReason: "Blocked: rm -rf is prohibited (recursive force delete can cause irreversible data loss)" | ||
| } | ||
| }' | ||
| exit 0 |
There was a problem hiding this comment.
The script now uses jq -n to generate JSON output for denial responses, but there's no fallback if jq is not available. While lines 31-32 have fallback logic for parsing input (using || echo ""), the output generation (lines 56-62, 68-74, etc.) will fail if jq is not installed. Consider adding a check at the beginning of the script to ensure jq is available, or provide a fallback mechanism for JSON output generation.
| jq -n '{ | ||
| hookSpecificOutput: { | ||
| hookEventName: "PreToolUse", | ||
| permissionDecision: "deny", | ||
| permissionDecisionReason: "Blocked: rm -rf is prohibited (recursive force delete can cause irreversible data loss)" | ||
| } | ||
| }' | ||
| exit 0 |
There was a problem hiding this comment.
The script now uses jq -n to generate JSON output for denial responses, but there's no fallback if jq is not available. While lines 31-32 have fallback logic for parsing input (using || echo ""), the output generation (lines 56-62, 68-74, etc.) will fail if jq is not installed. Consider adding a check at the beginning of the script to ensure jq is available, or provide a fallback mechanism for JSON output generation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/mapify_cli/templates/hooks/block-secrets.py (1)
28-29:⚠️ Potential issue | 🟡 MinorStale TESTING doc: still references exit code 2 and stderr.
Line 25 was updated to document the new
permissionDecision=denybehavior, but the TESTING example on line 29 still says# Expected: Exit code 2, JSON error on stderr. This should be updated to reflect exit code 0 and JSON on stdout.📝 Proposed fix
- # Expected: Exit code 2, JSON error on stderr + # Expected: Exit code 0, JSON on stdout with permissionDecision=deny.claude/hooks/block-secrets.py (1)
28-29:⚠️ Potential issue | 🟡 MinorSame stale TESTING comment as the template copy.
Line 29 still references exit code 2 / stderr. Same fix as noted in
src/mapify_cli/templates/hooks/block-secrets.py..claude/hooks/block-dangerous.sh (1)
20-21:⚠️ Potential issue | 🟡 MinorStale TESTING doc: still says "Expected: Exit code 2".
Same issue as block-secrets — the example expectation wasn't updated to match the new exit-0-with-deny-payload behavior.
📝 Proposed fix
-# # Expected: Exit code 2 +# # Expected: Exit code 0, JSON on stdout with permissionDecision=denyIMPLEMENTATION_SUMMARY.md (1)
148-166:⚠️ Potential issue | 🟡 MinorFile tree at line 151 incorrectly nests
settings.jsonunderhooks/.The tree shows
settings.jsonas a child of.claude/hooks/, but it actually lives at.claude/settings.json. This is misleading for readers..claude/ ├── hooks/ │ ├── workflow-context-injector.py (NEW) -│ └── settings.json (UPDATED) ├── commands/ │ ├── map-efficient.md (OPTIMIZED - 995→394 lines) │ └── map-efficient.md.backup (BACKUP) +├── settings.json (UPDATED)src/mapify_cli/templates/hooks/block-dangerous.sh (1)
20-22:⚠️ Potential issue | 🟡 MinorStale test expectation in header comment.
Line 21 still says
# Expected: Exit code 2, but the script now exits 0 with apermissionDecision=denypayload on all denial paths. This contradicts the updatedEXIT CODESsection on line 17.📝 Proposed fix
# TESTING: # echo '{"tool_name": "Bash", "tool_input": {"command": "rm -rf /"}}' | bash block-dangerous.sh -# # Expected: Exit code 2 +# # Expected: Exit code 0 with permissionDecision=deny in JSON output
🤖 Fix all issues with AI agents
In @.claude/agents/actor.md:
- Around line 403-408: The fenced code block in the "Validation Criteria
Coverage" example in .claude/agents/actor.md is missing a language tag causing
markdownlint MD040; update the triple-backtick fence that wraps the VC1 example
(the block starting with "VC1: <criterion text>") to include a language
identifier such as text (i.e. change ``` to ```text) so the block is
language-tagged and MD040 is satisfied.
In @.claude/hooks/block-dangerous.sh:
- Line 55: The check against dangerous rm flags on the COMMAND string requires
whitespace after the flags and therefore misses cases like "rm -rf.hidden" or
"rm -rf0dir"; update the grep regex used in the rm checks (the pattern currently
'rm\s+(-rf|-fr)\s' and the similar pattern later) to remove the mandatory
trailing \s so it matches flags regardless of the next character, e.g. use
'rm\s+(-rf|-fr)' (or an equivalent pattern that does not require a trailing
space) in the grep -qE calls referencing COMMAND to ensure paths immediately
adjacent to the flags are caught.
🧹 Nitpick comments (3)
src/mapify_cli/templates/hooks/workflow-gate.py (1)
26-31: Minor: test expectation could be more precise.The testing comment describes both outcomes but doesn't show how to distinguish them. Consider showing the expected JSON for each case.
📝 Suggested improvement
TESTING: echo '{"tool_name": "Edit", "tool_input": {"file_path": "test.py"}}' | python3 workflow-gate.py - # Expected: Exit code 0, allow (no workflow state) OR deny with reason (if workflow active + missing steps) + # Expected (no workflow state): Exit 0, stdout: {} + # Expected (workflow active, missing steps): Exit 0, stdout: {"hookSpecificOutput": {"permissionDecision": "deny", ...}}.claude/agents/monitor.md (1)
2526-2529: Optional: slightly more formal wording for the hard‑stop reminder.✍️ Suggested tweak
-- If you set `valid=false`, the workflow MUST fix the issues before proceeding. +- If you set `valid=false`, the workflow MUST resolve the issues before proceeding.src/mapify_cli/templates/agents/monitor.md (1)
2526-2529: Optional: slightly more formal wording for the hard‑stop reminder.✍️ Suggested tweak
-- If you set `valid=false`, the workflow MUST fix the issues before proceeding. +- If you set `valid=false`, the workflow MUST resolve the issues before proceeding.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/test_workflow_gate.py (1)
26-41: Helpers are well-structured; consider extracting shared utilities.The
_parse_stdout/_assert_deniedpattern is now duplicated across three test files (test_block_dangerous.py,test_block_secrets.py, and here). For test code this is tolerable, but a sharedtests/helpers/hook_assertions.pymodule would reduce the maintenance surface when the payload schema evolves again.Note the subtle API difference: here
_assert_deniedaccepts rawstdoutand returnsreason, while the other two accept a pre-parsedpayloadand returnNone. This inconsistency could trip up contributors.tests/hooks/test_block_secrets.py (1)
70-73: Minor inconsistency: allowed-path tests don't all verify empty stdout.
test_criterion_3_legitimate_allowedonly checksexit_code == 0but doesn't assert the stdout payload is empty ({}), unlike the denial tests that fully validate the JSON structure. Other test files (e.g.,test_block_dangerous.pyline 79, 85) do assert_parse_stdout(stdout) == {}for allowed paths. Consider adding that here for consistency.tests/hooks/test_block_dangerous.py (1)
30-43: Exact duplicate of_parse_stdout/_assert_deniedfromtest_block_secrets.py.Same observation as in
test_workflow_gate.py— these helpers are copy-pasted across test modules. The only difference is the optional parameter name (expected_substringhere vsexpected_file_fragmentintest_block_secrets.py), but the logic is identical. Fine for now, but worth consolidating if the payload schema changes again.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 43 out of 43 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Addressed review feedback:
Note: I didn’t add a matcher to Stop hooks because Claude Code docs state Stop ignores matchers (adding one would be cosmetic only). |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
IMPLEMENTATION_SUMMARY.md (1)
176-182:⚠️ Potential issue | 🟡 MinorEnsure user‑facing docs are updated before release.
The summary still lists a TODO for
docs/USAGE.md. Given the hook path + workflow behavior changes, please update README/USAGE (or confirm they were updated elsewhere) before shipping.Based on learnings: When changing user-facing behavior, update relevant documentation:
README.md(quick-start),docs/USAGE.md(workflows and CLI usage), anddocs/ARCHITECTURE.md(system design / agents).tests/hooks/test_block_secrets.py (1)
208-212:⚠️ Potential issue | 🟠 MajorAllowed-file tests no longer distinguish "allow" from "deny" after the exit-code change.
Since denials now also exit 0, asserting only
exit_code == 0doesn't verify the file was actually allowed. If the hook regresses and starts blockingid_rsa.puborlicense.key, these tests will still pass.
test_criterion_3_legitimate_allowed(line 74) already does it correctly withassert _parse_stdout(stdout) == {}. Apply the same pattern here.Proposed fix (example for one class — apply to all three)
def test_public_keys_allowed(self, filename): - exit_code, _, stderr = run_hook("Read", filename) - assert ( - exit_code == 0 - ), f"Public key {filename} should be ALLOWED, got exit {exit_code}. stderr: {stderr}" + exit_code, stdout, stderr = run_hook("Read", filename) + assert exit_code == 0, f"Public key {filename} should be ALLOWED. stderr: {stderr}" + assert _parse_stdout(stdout) == {}, f"Public key {filename} should be ALLOWED but got denial: {stdout}"def test_generic_key_files_allowed(self, filename): - exit_code, _, stderr = run_hook("Read", filename) - assert ( - exit_code == 0 - ), f"Generic key file {filename} should be ALLOWED, got exit {exit_code}. stderr: {stderr}" + exit_code, stdout, stderr = run_hook("Read", filename) + assert exit_code == 0, f"Generic key file {filename} should be ALLOWED. stderr: {stderr}" + assert _parse_stdout(stdout) == {}, f"Generic key file {filename} should be ALLOWED but got denial: {stdout}"def test_normal_files_allowed(self, filename): - exit_code, _, _ = run_hook("Read", filename) - assert exit_code == 0, f"{filename} should be allowed" + exit_code, stdout, _ = run_hook("Read", filename) + assert exit_code == 0, f"{filename} should be allowed" + assert _parse_stdout(stdout) == {}, f"{filename} should be allowed but got denial: {stdout}"Also applies to: 228-232, 253-255
🤖 Fix all issues with AI agents
In `@src/mapify_cli/templates/hooks/workflow-gate.py`:
- Around line 151-155: Update the docstring that lists "ENFORCEMENT RULES" to
reflect the actual behavior when workflow state is missing: currently the code
path in the workflow gate (the state is None check in the module/function
handling workflow gating) allows edits (it prints "{}" and exits 0), so change
the bullet that claims missing workflow_state blocks edits to instead state that
missing workflow_state allows edits (i.e., fail-open). Locate the docstring near
the top of src/mapify_cli/templates/hooks/workflow-gate.py that contains the
"ENFORCEMENT RULES" bullets and edit the text to match the actual behavior
implemented by the state is None branch.
🧹 Nitpick comments (1)
src/mapify_cli/templates/hooks/block-dangerous.sh (1)
133-142:git push --forcechecks use$COMMAND(case-sensitive) whilermchecks use$COMMAND_LOWER.This is intentional since git subcommands are case-sensitive, but worth a brief inline comment to prevent a future contributor from "fixing" this inconsistency.
| if state is None: | ||
| # No workflow state = not in MAP workflow mode, allow | ||
| # (This prevents breaking non-MAP work) | ||
| print(json.dumps({"decision": "approve"})) | ||
| print("{}") | ||
| sys.exit(0) |
There was a problem hiding this comment.
Update docstring: missing workflow_state now allows edits.
The behavior at Line 151–155 fail‑opens when the state file is missing, but the header still claims it blocks. Please align the “ENFORCEMENT RULES” bullets.
✏️ Suggested docstring fix
- - Blocks Edit/Write/MultiEdit if workflow_state.json is missing
+ - Allows Edit/Write/MultiEdit if workflow_state.json is missing (fail-open outside MAP)🤖 Prompt for AI Agents
In `@src/mapify_cli/templates/hooks/workflow-gate.py` around lines 151 - 155,
Update the docstring that lists "ENFORCEMENT RULES" to reflect the actual
behavior when workflow state is missing: currently the code path in the workflow
gate (the state is None check in the module/function handling workflow gating)
allows edits (it prints "{}" and exits 0), so change the bullet that claims
missing workflow_state blocks edits to instead state that missing workflow_state
allows edits (i.e., fail-open). Locate the docstring near the top of
src/mapify_cli/templates/hooks/workflow-gate.py that contains the "ENFORCEMENT
RULES" bullets and edit the text to match the actual behavior implemented by the
state is None branch.
1 participant
Summary:
Why:
Summary by CodeRabbit
Documentation
Chores