feat(hooks/rules/skills): deterministic hooks, git blast-radius guard, design rules#48
Merged
Merged
Conversation
Bare "model": "haiku" no longer resolves in current Claude Code, so the conventional-commits validator and secret scanner fired silently with no effect. Replace both prompt hooks with deterministic command scripts: - scripts/commit-validate.js: regex validator for conventional commits (type/scope/summary, 72-char limit, parses -m flag and HEREDOC bodies). - scripts/secret-scan.js: regex patterns for AWS/GitHub/OpenAI/Anthropic/ Slack/Google/Stripe/private keys/Bearer/generic password-token assignments, with env-reference allowlist. Update commands/doctor.md with a sanity check that runs both scripts without an LLM. No new dependencies; works offline.
Contributor
📝 WalkthroughWalkthroughReplaces LLM-based prompt hooks with deterministic command hooks, adding three Node.js validators ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ToolRunner as Hook Runner
participant Script as Validator Script
participant Git as Git/Target
User->>ToolRunner: invoke tool (e.g., `git commit` / edit / write)
ToolRunner->>Script: run appropriate script (commit-validate / secret-scan / git-blast-radius) with JSON stdin
Script-->>ToolRunner: exit code 0 (allow) or 2 (block) + diagnostics on stderr
alt exit 0
ToolRunner->>Git: proceed with tool action
else exit 2
ToolRunner->>User: surface blocked message and diagnostics
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://ghub.bbstale.com/avatars/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hooks/hooks.json (1)
62-62:⚠️ Potential issue | 🟡 MinorStale description: "LLM commit validation" is no longer accurate.
After this PR the
Bash(git commit*)hook runs deterministiccommit-validate.js, not an LLM. Update the description to reflect reality:- "description": "Git operation guards: quality gates before commit, LLM commit validation, wrap-up before push" + "description": "Git operation guards: quality gates before commit, deterministic commit-message validation, wrap-up before push"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/hooks.json` at line 62, Update the stale hooks.json description string that currently says "LLM commit validation" to accurately reflect that the Bash(git commit*) hook now runs the deterministic commit-validate.js script; edit the value for the "description" key to mention deterministic commit validation via commit-validate.js (and remove any reference to an LLM) so the text correctly describes the Git operation guards and wrap-up before push.
♻️ Duplicate comments (1)
hooks/hooks.json (1)
65-72:⚠️ Potential issue | 🟠 MajorConsider widening matcher to
Editas well.The content scanner already supports
tool_input.new_string, so extending the matcher closes the Edit-tool bypass (see detailed comment onscripts/secret-scan.js):- "matcher": "Write", + "matcher": "tool == \"Edit\" || tool == \"Write\"",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/hooks.json` around lines 65 - 72, Update the hooks.json matcher so the secret-scanner runs not just on "Write" events but also on "Edit" events to prevent the Edit-tool bypass; modify the "matcher" entry for the secret-scan hook (the block that currently has matcher "Write" and runs scripts/secret-scan.js) to include both Write and Edit (e.g., as a combined matcher or list) so the scanner executes on edits as well as writes.
🧹 Nitpick comments (4)
scripts/commit-validate.js (1)
2-3: Type list diverges fromrules/atomic-commits.mdc.The rule file documents 9 types (
feat, fix, refactor, test, docs, chore, perf, ci, style); this script addsbuildandrevert. Both directions are reasonable (build/revertare standard Conventional Commits types), but the rule and validator should agree or developers will hit drift between documentation and enforcement. Either extend the rule, or dropbuild/reverthere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/commit-validate.js` around lines 2 - 3, The TYPES array in commit-validate.js (const TYPES) includes 'build' and 'revert' which diverges from the documented list in rules/atomic-commits.mdc; update the validator so it matches the rule: either remove 'build' and 'revert' from the TYPES array or add them to the documented rule, and then regenerate the PATTERN (const PATTERN) if needed; locate and edit the const TYPES declaration to make the list identical to the atomic-commits rule to prevent enforcement/documentation drift.scripts/secret-scan.js (3)
2-16: Duplicated secret-pattern logic withscripts/post-edit-check.js.
post-edit-check.js(lines 43-47 per context) carries its ownapi[_-]?key|secret|password|tokenregex. Now thatsecret-scan.jsis the canonical detector, consider exportingPATTERNS/ALLOWLISTfrom a shared module and consuming them from both sites so rules don't drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/secret-scan.js` around lines 2 - 16, The PATTERNS array in scripts/secret-scan.js is duplicated by the inline regex in scripts/post-edit-check.js; extract PATTERNS (and any ALLOWLIST used by post-edit-check.js) into a new shared module (e.g., scripts/secret-patterns.js) and update both scripts to import that export instead of defining their own regexes so rules stay canonical; specifically move the PATTERNS constant (and any allowlist logic) out of scripts/secret-scan.js, export it, then replace the inline pattern/regex usage in scripts/post-edit-check.js (the api[_-]?key|secret|password|token logic) to import and use the shared PATTERNS/ALLOWLIST.
51-55: Blanket refusal on.env*/*.pem/*.key/secrets/paths may break legitimate flows.Anchored on
$, this correctly passes.env.example/.env.sample, which is good. But it still hard-blocks any write to.env,.env.local,id_rsa.pem,keys/foo.key, etc. — including the agent legitimately scaffolding a fresh local.envwith only placeholders. Since the content scanner + allowlist already handles placeholders, consider downgrading this to "scan content strictly" rather than unconditionally refusing, or at least allow through if the content is 100% allowlisted (process.env.*references only,<REDACTED>, etc.).Not blocking, but worth a conscious decision before merge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/secret-scan.js` around lines 51 - 55, The current check in secret-scan.js that tests the path variable with the regex and immediately calls process.exit(2) is too aggressive; change it so matching secret-like paths do not unconditionally abort: if input?.tool_input?.content exists, run a strict allowlist check against that content (e.g., only allow fully placeholder files like process.env.* references, "<REDACTED>", "REDACTED", or other approved patterns) and if the content passes the allowlist then continue normally; otherwise invoke the existing content scanner/deny path and only then call process.exit(2). Keep the original path regex but replace the immediate exit with this conditional content check using input?.tool_input?.content and the same process.exit(2) only for non-allowlisted content.
2-16: Pattern definitions are overly broad; official gitleaks patterns are far more specific.The patterns in lines 2-16 lack specificity compared to gitleaks' official detectors:
- OpenAI API Key: Your pattern
/\bsk-(?:proj-)?(?!ant-)[A-Za-z0-9_\-]{20,}\b/will match anysk-token. Official gitleaks requires the base64-encoded markerT3BlbkFJplus exact length constraints (e.g.,sk-[a-zA-Z0-9]{20}T3BlbkFJ[a-zA-Z0-9]{20}for legacy tokens), drastically reducing false positives.- Anthropic API Key: Your pattern
/\bsk-ant-[A-Za-z0-9_\-]{20,}\b/is also broad. Official patterns require specific prefixes likeapi03-oradmin01-, exactly 93 random characters, and the suffixAA(e.g.,sk-ant-api03-[a-zA-Z0-9_\-]{93}AA).- Stripe vs. OpenAI: Your Stripe pattern
/\bsk_live_[0-9a-zA-Z]{24,}\b/(underscore) doesn't conflict with OpenAI's hyphenated prefix—this concern is resolved by design.- Generic Bearer Token:
/\bBearer\s+[A-Za-z0-9_\-.=]{30,}/remains overly permissive and will match placeholder strings likeBearer ${process.env.TOKEN}once they exceed 30 characters.Consider tightening these patterns to match official gitleaks definitions to reduce noise.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/secret-scan.js` around lines 2 - 16, The PATTERNS array uses overly-broad regexes that generate false positives; tighten each detector by replacing the generic entries in PATTERNS (notably the 'OpenAI API Key', 'Anthropic API Key', and 'Generic Bearer Token' objects) with stricter gitleaks-style patterns: for 'OpenAI API Key' require the known base64 marker (e.g., include T3BlbkFJ) and exact length constraints for legacy/modern forms, for 'Anthropic API Key' match the documented prefixes (e.g., api03-, admin01-) plus the exact 93-char body and AA suffix, and for 'Generic Bearer Token' disallow template/env placeholders like ${...} and require a fixed minimum length and allowed charset; update the regex literals in the PATTERNS array accordingly to mirror gitleaks official patterns to reduce noise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@commands/doctor.md`:
- Around line 28-35: Update the Deterministic Hook Sanity section to include one
negative test per script so failures are detected: add a failing commit case
invoking scripts/commit-validate.js with a non-conventional message (e.g., "not
conventional") and a failing secret case invoking scripts/secret-scan.js with a
fake key string; run each with stderr suppressed (2>/dev/null) and use
conditional &&/|| to print "BROKEN (should have failed)" vs "reject: OK" so a
script that always exits 0 will be flagged. Reference the existing invocations
of scripts/commit-validate.js and scripts/secret-scan.js and mirror the
stdout/stderr handling and success/failure echoing used in the current
happy-path lines.
In `@scripts/commit-validate.js`:
- Around line 24-35: The validate function currently returns { ok: true } when
msg is falsy which silently skips validation; change validate(msg) so that when
msg is null/undefined it does not silently succeed but instead writes a
non-blocking stderr warning (e.g. console.error) indicating validation was
skipped for an unparsable commit message and return a distinct result (e.g. {
ok: true, skipped: true } or include a reason) so callers can differentiate
skipped vs validated passes; keep the existing PATTERN/TYPES/MAX_SUMMARY checks
for real messages and only return a hard pass { ok: true } when a parsed
firstLine is present and validated.
- Around line 15-22: extractMessage currently misses --message/--message=, the
-F file flag, and heredocs that use terminators other than EOF; update
extractMessage to also match --message(?:=|\s+)(?:"..."|'...'|[^ \t]+) and the
-F\s+(\S+) flag (for -F return a sentinel or null but ensure caller emits a
warning), and replace the hardcoded EOF heredoc regex with one that captures any
terminator token (e.g. /<<'?([A-Za-z0-9_]+)'?\s*\n([\s\S]*?)\n\1/) so opening
and closing tags must match; finally, adjust validate (the code path that
currently treats extractMessage null as { ok: true }) so that a missing parsed
message either returns { ok: false, reason: "no message parsed" } or logs a
visible warning when a heredoc was present but unrecognized, preventing silent
passes. Ensure you modify references to extractMessage and the validate return
path accordingly.
In `@scripts/secret-scan.js`:
- Around line 46-60: Update the hook matcher so secret-scan runs for edits as
well as writes: change the matcher expression in hooks/hooks.json from matching
only Write to include Edit (e.g. use tool == "Edit" || tool == "Write"); no code
changes needed in the script (it already reads tool_input.new_string into
content and uses file_path via the path variable and the existing secret-path
regex), just broaden the matcher to prevent Edit-based bypasses.
- Around line 33-44: The allowlist is currently applied only to the matched
token (snippet) so entries like /process\.env/ never match short tokens (e.g.
AKIA...). In scan(), compute the full surrounding line for the match using
m.index and m[0].length (e.g. find prior newline and next newline to slice the
full line), then replace ALLOWLIST.some(a => a.test(snippet)) with
ALLOWLIST.some(a => a.test(lineText)). Keep returning the short snippet
(snippet.slice(0,40)) and the line number as before; update references in scan()
where snippet and m are used.
---
Outside diff comments:
In `@hooks/hooks.json`:
- Line 62: Update the stale hooks.json description string that currently says
"LLM commit validation" to accurately reflect that the Bash(git commit*) hook
now runs the deterministic commit-validate.js script; edit the value for the
"description" key to mention deterministic commit validation via
commit-validate.js (and remove any reference to an LLM) so the text correctly
describes the Git operation guards and wrap-up before push.
---
Duplicate comments:
In `@hooks/hooks.json`:
- Around line 65-72: Update the hooks.json matcher so the secret-scanner runs
not just on "Write" events but also on "Edit" events to prevent the Edit-tool
bypass; modify the "matcher" entry for the secret-scan hook (the block that
currently has matcher "Write" and runs scripts/secret-scan.js) to include both
Write and Edit (e.g., as a combined matcher or list) so the scanner executes on
edits as well as writes.
---
Nitpick comments:
In `@scripts/commit-validate.js`:
- Around line 2-3: The TYPES array in commit-validate.js (const TYPES) includes
'build' and 'revert' which diverges from the documented list in
rules/atomic-commits.mdc; update the validator so it matches the rule: either
remove 'build' and 'revert' from the TYPES array or add them to the documented
rule, and then regenerate the PATTERN (const PATTERN) if needed; locate and edit
the const TYPES declaration to make the list identical to the atomic-commits
rule to prevent enforcement/documentation drift.
In `@scripts/secret-scan.js`:
- Around line 2-16: The PATTERNS array in scripts/secret-scan.js is duplicated
by the inline regex in scripts/post-edit-check.js; extract PATTERNS (and any
ALLOWLIST used by post-edit-check.js) into a new shared module (e.g.,
scripts/secret-patterns.js) and update both scripts to import that export
instead of defining their own regexes so rules stay canonical; specifically move
the PATTERNS constant (and any allowlist logic) out of scripts/secret-scan.js,
export it, then replace the inline pattern/regex usage in
scripts/post-edit-check.js (the api[_-]?key|secret|password|token logic) to
import and use the shared PATTERNS/ALLOWLIST.
- Around line 51-55: The current check in secret-scan.js that tests the path
variable with the regex and immediately calls process.exit(2) is too aggressive;
change it so matching secret-like paths do not unconditionally abort: if
input?.tool_input?.content exists, run a strict allowlist check against that
content (e.g., only allow fully placeholder files like process.env.* references,
"<REDACTED>", "REDACTED", or other approved patterns) and if the content passes
the allowlist then continue normally; otherwise invoke the existing content
scanner/deny path and only then call process.exit(2). Keep the original path
regex but replace the immediate exit with this conditional content check using
input?.tool_input?.content and the same process.exit(2) only for non-allowlisted
content.
- Around line 2-16: The PATTERNS array uses overly-broad regexes that generate
false positives; tighten each detector by replacing the generic entries in
PATTERNS (notably the 'OpenAI API Key', 'Anthropic API Key', and 'Generic Bearer
Token' objects) with stricter gitleaks-style patterns: for 'OpenAI API Key'
require the known base64 marker (e.g., include T3BlbkFJ) and exact length
constraints for legacy/modern forms, for 'Anthropic API Key' match the
documented prefixes (e.g., api03-, admin01-) plus the exact 93-char body and AA
suffix, and for 'Generic Bearer Token' disallow template/env placeholders like
${...} and require a fixed minimum length and allowed charset; update the regex
literals in the PATTERNS array accordingly to mirror gitleaks official patterns
to reduce noise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc438aae-cb84-42f9-aabf-9b58db69c92f
📒 Files selected for processing (4)
commands/doctor.mdhooks/hooks.jsonscripts/commit-validate.jsscripts/secret-scan.js
Adds a deterministic PreToolUse Bash hook that blocks destructive git operations before execution, two design-discipline rules, and three new skills for planning, orientation, and durable defect capture. Hook: - scripts/git-blast-radius.js: blocks force push, hard reset, clean -f, branch -D, checkout/restore '.', interactive rebase on protected branches, filter-branch, reflog expire, update-ref -d, stash drop. Warns (but allows) push --force-with-lease. Override via PRO_WORKFLOW_ALLOW_UNSAFE_GIT=1. Non-git commands pass through. Rules: - rules/incremental-verify.mdc: one test, one implementation, one commit. Guards against horizontal test authoring that asserts on imagined rather than observed behavior. - rules/module-shape.mdc: narrow surface + thick depth. Inject dependencies at the boundary. Mock only at system edges. Skills: - skills/plan-interrogate/: walks a plan's decision tree one question at a time, every question carrying a recommended answer. Exits when zero open nodes remain. - skills/module-map/: one-screen orientation map for unfamiliar codebase areas. Entry points, core modules, data flow, callers, hidden coupling. Fifteen-second read target. - skills/bug-capture/: durable GitHub issue capture. Minimal clarifying questions, parallel codebase exploration for domain language, duplicate check, dependency-ordered breakdowns, no file paths or line numbers in issue bodies. Doctor: - commands/doctor.md: new sanity check for all deterministic hook scripts plus documentation for the git safety override.
rohitg00
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
skills/plan-interrogate/SKILL.md (1)
15-17: Align node state terminology with output format.Step 2 introduces three node states: open, pinned, and inferred (lines 16-17). However, step 7's output format (line 28) specifies sources as
user / inferred: path, which doesn't explicitly mention "pinned."While the mapping is intuitive (pinned → user), explicitly stating this connection would improve clarity and prevent confusion when following the workflow.
📝 Suggested clarification
Option 1: Update line 28 to reference all three states:
7. Exit only when zero nodes are open. Print the resolved tree as a flat - list: "Decision — Choice — Source (user / inferred: path)". + list: "Decision — Choice — Source (user for pinned nodes / inferred: path)".Option 2: Update lines 16-17 to use "user-pinned" to match output terminology:
2. Extract the decision tree. Every branch point becomes a node. Mark - nodes as **open** (not decided), **pinned** (decided by the user), or + nodes as **open** (not decided), **user-pinned** (decided by the user), or **inferred** (derivable from the codebase or existing constraints).Also applies to: 28-28
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/plan-interrogate/SKILL.md` around lines 15 - 17, Step 2 defines node states as "open", "pinned", and "inferred" but Step 7's output format uses "user / inferred: path" and omits "pinned", causing ambiguity; update the documentation so terminology is consistent by either (A) changing Step 7's output format text to explicitly list all three states (e.g., "user-pinned / inferred: path" or "pinned / inferred: path") or (B) renaming "pinned" in Step 2 to "user-pinned" (or "user") to match the output; apply the same change wherever node states are referenced to keep "nodes", "open", "pinned"/"user-pinned", and "inferred" aligned across the doc.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/git-blast-radius.js`:
- Line 39: The console.error call prints the raw command (variable command)
which can include credentials; create and call a sanitizer (e.g.,
sanitizeCommand) before logging or log only the matched subcommand/flag (e.g.,
name and the specific matched token) instead of the full command. Implement
sanitizeCommand to strip userinfo from any URL patterns (remove
username:password@ or username@ in https?://... and git+ssh urls) and return a
redacted string, then replace console.error(`[pro-workflow] git-blast-radius:
blocked "${name}". Command: ${command}`) with a call that uses the sanitized
value (or just `${name} ${matchedFlag}`) to avoid leaking credentials. Ensure
references to command and name remain correct and unit-test the regex against
examples like https://USER:TOKEN@host/... and https://USER@host/... to confirm
redaction.
- Around line 2-14: The BLOCK regexes in scripts/git-blast-radius.js assume the
subcommand immediately follows "git", so commands with global options (e.g. git
-c ... reset --hard), refspec force pushes (git push origin +main) and
push-based ref deletions (git push origin :feature) slip through; update the
matching by first normalizing the command string (remove leading "git" and any
global options like -c KEY=VAL or --no-pager / --<flag>) before testing against
BLOCK, and strengthen these specific entries: adjust the "force push" entry to
also detect refspec-based forcing (look for "\s+\+\S" or refspec patterns),
extend "ref deletion" to catch push-based deletions (":\S" in push args), and
make "interactive rebase on protected branch" tolerate "-i" appearing after the
branch token as well as before; reference the BLOCK array and the entries named
'force push', 'ref deletion', and 'interactive rebase on protected branch' when
applying fixes.
In `@skills/plan-interrogate/SKILL.md`:
- Around line 1-4: The SKILL frontmatter for plan-interrogate should include
user-invocable: true and a shorter description; update the YAML block in
SKILL.md for the skill named "plan-interrogate" to add user-invocable: true and
replace the long description with a concise one such as "Stress-test a plan by
walking its decision tree one question at a time." so the skill is discoverable
and succinct.
---
Nitpick comments:
In `@skills/plan-interrogate/SKILL.md`:
- Around line 15-17: Step 2 defines node states as "open", "pinned", and
"inferred" but Step 7's output format uses "user / inferred: path" and omits
"pinned", causing ambiguity; update the documentation so terminology is
consistent by either (A) changing Step 7's output format text to explicitly list
all three states (e.g., "user-pinned / inferred: path" or "pinned / inferred:
path") or (B) renaming "pinned" in Step 2 to "user-pinned" (or "user") to match
the output; apply the same change wherever node states are referenced to keep
"nodes", "open", "pinned"/"user-pinned", and "inferred" aligned across the doc.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 65e5a6a1-c900-4446-a93a-7b3c582bbc59
📒 Files selected for processing (8)
commands/doctor.mdhooks/hooks.jsonrules/incremental-verify.mdcrules/module-shape.mdcscripts/git-blast-radius.jsskills/bug-capture/SKILL.mdskills/module-map/SKILL.mdskills/plan-interrogate/SKILL.md
✅ Files skipped from review due to trivial changes (4)
- skills/module-map/SKILL.md
- rules/module-shape.mdc
- skills/bug-capture/SKILL.md
- rules/incremental-verify.mdc
🚧 Files skipped from review as they are similar to previous changes (1)
- hooks/hooks.json
commit-validate.js:
- Parse --message flag (space and = forms) in addition to -m.
- Detect heredoc with any tag, not only EOF, including <<- style.
- Treat -F/--file and editor-mode commit (no -m/-F/--amend) as
pass-through instead of silent ok. For unrecognized forms, emit
a non-blocking stderr warning so skipped validation is visible.
- Return structured {msg, form} from extractMessage so the driver
can branch on intent.
secret-scan.js:
- Allowlist now tests the full surrounding line, not just the
matched token. Fixes the AWS/GitHub/private-key classes where
the snippet itself cannot contain the allowlist signal.
- Hook matcher in hooks.json broadened from Write to (Edit || Write)
so secrets introduced via Edit into existing files are also caught.
git-blast-radius.js:
- Tolerate global options between git and the subcommand
(git -c foo=bar, git --no-pager, git -C dir) via a shared
GIT_PREFIX regex builder.
- Add patterns for refspec force push (push origin +branch) and
remote branch deletion (push origin :ref, push --delete).
- Redact credentials embedded in URLs (https://user:token@host/...)
before echoing the blocked command to stderr.
plan-interrogate/SKILL.md:
- Shorten frontmatter description; add user-invocable: true.
- Collapse node-state terminology from three states (open/pinned/
inferred) to two (open/resolved) with a source tag (user|inferred)
to match the final output format.
commands/doctor.md:
- Replace happy-path-only sanity checks with paired positive and
negative cases for each hook script, so a script that always
exits 0 fails the doctor instead of passing silently.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/commit-validate.js`:
- Around line 37-42: The regex that computes hasExplicitFlag in
scripts/commit-validate.js incorrectly treats --amend as a message-providing
flag, causing the branch in the git commit detection (the if block testing
/\bgit\s+(?:-[^\s]+\s+)*commit\b/) to return 'unknown' for `git commit --amend`;
update the hasExplicitFlag logic to only consider flags that actually supply a
message (e.g., -m, --message, -F, --file) and exclude --amend (or invert the
logic so only message flags set hasExplicitFlag), so that when --amend appears
without a message flag the function returns { msg: null, form: 'editor' }
instead of 'unknown'.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 81490812-21cf-4553-b167-4ac86ccfa868
📒 Files selected for processing (6)
commands/doctor.mdhooks/hooks.jsonscripts/commit-validate.jsscripts/git-blast-radius.jsscripts/secret-scan.jsskills/plan-interrogate/SKILL.md
✅ Files skipped from review due to trivial changes (1)
- commands/doctor.md
🚧 Files skipped from review as they are similar to previous changes (3)
- hooks/hooks.json
- scripts/secret-scan.js
- scripts/git-blast-radius.js
Comment on lines
+37
to
+42
| if (/\bgit\s+(?:-[^\s]+\s+)*commit\b/.test(command)) { | ||
| const afterCommit = command.split(/\bcommit\b/)[1] || ''; | ||
| const hasExplicitFlag = /(?:-m|--message|-F|--file|--amend)\b/.test(afterCommit); | ||
| if (!hasExplicitFlag) return { msg: null, form: 'editor' }; | ||
| return { msg: null, form: 'unknown' }; | ||
| } |
Contributor
There was a problem hiding this comment.
--amend without -m is misclassified as unknown instead of editor.
git commit --amend (no message flag) opens the editor, same as a bare git commit. With the current logic, --amend sets hasExplicitFlag = true, so it falls into the unknown branch and prints a "could not parse commit message" warning, even though this is a perfectly normal editor-mode amend. Consider excluding --amend from the explicit-flag set, or only treating -m/--message/-F/--file as message-providing flags:
Proposed diff
- const hasExplicitFlag = /(?:-m|--message|-F|--file|--amend)\b/.test(afterCommit);
+ const hasExplicitFlag = /(?:-m|--message|-F|--file)\b/.test(afterCommit);
if (!hasExplicitFlag) return { msg: null, form: 'editor' };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/commit-validate.js` around lines 37 - 42, The regex that computes
hasExplicitFlag in scripts/commit-validate.js incorrectly treats --amend as a
message-providing flag, causing the branch in the git commit detection (the if
block testing /\bgit\s+(?:-[^\s]+\s+)*commit\b/) to return 'unknown' for `git
commit --amend`; update the hasExplicitFlag logic to only consider flags that
actually supply a message (e.g., -m, --message, -F, --file) and exclude --amend
(or invert the logic so only message flags set hasExplicitFlag), so that when
--amend appears without a message flag the function returns { msg: null, form:
'editor' } instead of 'unknown'.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #47. Also adds defensive git hook, two design-discipline rules, and three new skills.
Fix for #47 — deterministic hooks
hooks/hooks.jsonhardcoded"model": "haiku"for two prompt hooks, which no longer resolves in current Claude Code → hooks fired silently.scripts/commit-validate.js— regex conventional-commits validator. Parses-mflag and HEREDOC bodies, 11 allowed types, 72-char summary limit.scripts/secret-scan.js— regex patterns for AWS / GitHub (classic + fine-grained PAT) / OpenAI / Anthropic / Slack / Google / Stripe / PEM / Bearer / genericpassword=/api_key=. Allowlistsprocess.env.*, placeholders,<REDACTED>style tokens. Refuses writes to.env,.pem,.key,secrets/paths.[LEARN]capture is already a command-type hook (regex → SQLite); no change needed.New:
scripts/git-blast-radius.jsPreToolUse Bash hook that blocks destructive git ops before execution. Patterns covered: force push, hard reset,
clean -f[d],branch -D,checkout ./restore ., interactive rebase on protected branches (main/master/trunk/release/*),filter-branch,reflog expire,update-ref -d,stash drop|clear.push --force-with-lease.export PRO_WORKFLOW_ALLOW_UNSAFE_GIT=1in the specific shell.New: design rules
rules/incremental-verify.mdc— one test, one implementation, one commit. Guards against horizontal test authoring that asserts on imagined rather than observed behavior.rules/module-shape.mdc— narrow surface, thick depth. Inject dependencies at the boundary. Mock only at system edges. Endpoint-shaped mocks over generic fetchers.New skills
skills/plan-interrogate/— walks a plan's decision tree one question at a time. Every question carries a recommended answer. Exits when zero open nodes remain.skills/module-map/— one-screen orientation map for unfamiliar areas. Entry points → core modules → data flow → callers → hidden coupling. Fifteen-second read target.skills/bug-capture/— durable GitHub issue capture. Minimal clarifying questions, parallel codebase exploration for domain language,gh issue list --searchduplicate check, dependency-ordered breakdowns, no file paths or line numbers in issue bodies.Doctor updates
commands/doctor.mdgains sanity-check commands for all three deterministic hook scripts, plus documentation for the git safety override.Test plan
commit-validate.jsacceptsfeat: x, rejectsbad message(exit 2)secret-scan.jsflagssk-ant-*, allowsapi_key=process.env.FOOgit-blast-radius.jsblocks force push, hard reset, clean -fd, branch -D, checkout ., interactive rebase on maingit-blast-radius.jswarns on --force-with-lease, allows safegit push origin main, passes through non-git commandsPRO_WORKFLOW_ALLOW_UNSAFE_GIT=1override workshooks.jsonparses as valid JSON/pro-workflow:doctorprints the new sanity-check outputSummary by CodeRabbit
New Features
Bug Fixes
Documentation