doctor: hook-safety suite (sprawl, binaries, network, duplicates)#75
Merged
Conversation
Adds a new internal/hooks package (settings scanner + analyses) and wires four checks into `hookwise doctor`, closing #33/#34/#35/#36: - #34 inventory scan: SCAN line with total + per-event breakdown; sprawl WARN/FAIL thresholds (hot-path PreToolUse/PostToolUse >5/>10, others >3/>8). - #33 missing binary: extract the executable from each hook command and check PATH (exec.LookPath, injected for hermetic tests); FAIL with dependent count. - #35 network hooks: flag uvx/uv run/npx/pip install/curl/wget/docker run on hot-path events only; docker --pull=never is exempt. - #36 duplicates + overlap: exact-duplicate WARN; functional-overlap INFO when multiple known guard systems share an event. Scanner reads ~/.claude/settings.json + projects/*/settings.local.json, tolerant of missing/malformed files. internal/hooks added to the guarded test target. TDD throughout; unit + contract + arch gates green. Smoked on real settings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 51 minutes and 56 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces a hook safety analysis subsystem to the ChangesHook Safety Analysis & Doctor Integration
Sequence Diagram(s)sequenceDiagram
participant Doctor as runDoctor
participant CheckHook as checkHookSafety
participant Scan as hooks.Scan
participant Analyze as hooks.AllFindings
participant Render as renderHookFinding
Doctor->>CheckHook: execute Check 7
CheckHook->>Scan: Scan(DefaultSettingsPaths)
Scan-->>CheckHook: *Inventory
CheckHook->>Analyze: AllFindings(inv, nil)
Analyze-->>CheckHook: []Finding
loop each Finding
CheckHook->>Render: renderHookFinding(w, f)
Render-->>CheckHook: formatted output
end
CheckHook-->>Doctor: warnings, fails
Doctor->>Doctor: aggregate with other checks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 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 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://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/hookwise/cmd_doctor.go`:
- Line 169: Replace the ignored error from hooks.Scan by capturing and handling
it: change the call to inv, err := hooks.Scan(hooks.DefaultSettingsPaths()) and
then check err; if not nil, return a wrapped error (e.g. return
fmt.Errorf("scanning hooks settings: %w", err)) or log and exit appropriately
for the surrounding command handler. This uses the hooks.Scan and
hooks.DefaultSettingsPaths symbols and ensures the command surfaces any future
Scan failures instead of silently discarding them.
In `@internal/hooks/analyze.go`:
- Around line 211-214: The current check that treats commands containing
"--pull=never" as safe is vulnerable to substring false-positives; update the
guard around the condition that inspects e.Command so it tokenizes the command
(e.g., via strings.Fields(e.Command)) and checks for an exact token equal to
"--pull=never" (or use a boundary-aware regex like `(^|\\s)--pull=never(\\s|$)`)
instead of strings.Contains; modify the if that currently reads `if
strings.Contains(e.Command, "docker run") && strings.Contains(e.Command,
"--pull=never") { continue }` to use the tokenized/regex check for the flag
while keeping the existing "docker run" check.
In `@internal/hooks/paths_test.go`:
- Line 19: The test creates a temporary directory with overly permissive mode
0o755; change the call using os.MkdirAll(dir, 0o755) to use a stricter
permission like 0o750 (or 0750) to satisfy gosec and follow defensive practices,
ensuring tests still run (owner read/write/execute and group read/execute only).
In `@internal/hooks/paths.go`:
- Line 19: The filepath.Glob call currently discards its error (matches, _ :=
filepath.Glob(...)), which can hide permission or filesystem problems; change it
to capture the error (matches, err := filepath.Glob(filepath.Join(claudeDir,
"projects", "*", "settings.local.json"))), and if err != nil record or log it
for observability—either append the error to the same Scan.ParseErrors
collection used elsewhere or emit a clear log entry so callers know the glob
failed; keep the existing behavior of using matches when err==nil.
In `@internal/hooks/scan.go`:
- Around line 54-68: The doc comment for Scan is inaccurate: the implementation
reads files with os.ReadFile, records non-IsNotExist read failures into
Inventory.ParseErrors (using ParseError) and always returns a nil error; update
the comment on Scan to state that Scan is best-effort, that missing files are
skipped, and that any unreadable-but-existing file errors are recorded in
Inventory.ParseErrors (ParseError) rather than being returned as a fatal error;
leave the implementation of Scan, Inventory.ParseErrors, and ParseError
unchanged.
- Around line 59-95: The code comment and behavior for Scan() diverge: Scan
currently accumulates unreadable/existing-file failures into
Inventory.ParseErrors (using ParseError) but always returns nil error; update
Scan to return a non-nil error when there are recorded ParseErrors. Concretely,
in Scan (after building inv) check if len(inv.ParseErrors) > 0 and return inv
plus an aggregated error (e.g., build a single error string or wrap the first
ParseError) instead of nil so callers can detect parse/read failures; keep using
Inventory, ParseErrors and ParseError types for details in the returned
Inventory.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 124146d7-bd9c-4fdf-86ad-dfefddb5bbf0
📒 Files selected for processing (8)
Taskfile.ymlcmd/hookwise/cmd_doctor.gointernal/hooks/analyze.gointernal/hooks/analyze_test.gointernal/hooks/paths.gointernal/hooks/paths_test.gointernal/hooks/scan.gointernal/hooks/scan_test.go
Comment on lines
+211
to
+214
| // "docker run --pull=never" never pulls — treat as safe. | ||
| if strings.Contains(e.Command, "docker run") && strings.Contains(e.Command, "--pull=never") { | ||
| continue | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are any real-world docker run commands in the codebase that might test this edge case
rg -n 'docker run.*--pull' --type=goRepository: vishnujayvel/hookwise
Length of output: 498
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Broader search for the exact flag and potential near-miss variants
rg -n --hidden --no-ignore --glob '!*node_modules/*' --glob '!*dist/*' --glob '!*build/*' --glob '!*vendor/*' --glob '!*target/*' \
-- '--pull=never' .
rg -n --hidden --no-ignore --glob '!*node_modules/*' --glob '!*dist/*' --glob '!*build/*' --glob '!*vendor/*' --glob '!*target/*' \
-- '--pull=never-[A-Za-z0-9_-]+' .
# Inspect how e.Command is obtained/normalized and confirm snippet context
sed -n '180,240p' internal/hooks/analyze.go
sed -n '90,170p' internal/hooks/analyze_test.goRepository: vishnujayvel/hookwise
Length of output: 506
🏁 Script executed:
#!/bin/bash
set -u
# 1) Broader searches (don’t fail the script if there are no matches)
rg -n --hidden --no-ignore \
--glob '!*node_modules/*' --glob '!*dist/*' --glob '!*build/*' --glob '!*vendor/*' --glob '!*target/*' \
-- '--pull=never' . || true
rg -n --hidden --no-ignore \
--glob '!*node_modules/*' --glob '!*dist/*' --glob '!*build/*' --glob '!*vendor/*' --glob '!*target/*' \
-- '--pull=never-' . || true
# 2) Inspect the exact snippet context and how e.Command is formed/normalized
sed -n '190,240p' internal/hooks/analyze.go
sed -n '110,160p' internal/hooks/analyze_test.goRepository: vishnujayvel/hookwise
Length of output: 3995
Tighten --pull=never detection to avoid substring false-positives. (internal/hooks/analyze.go ~211-214)
The current guard uses strings.Contains(e.Command, "--pull=never"), and there’s a test for the exact "docker run --pull=never img" case. Substring matching can still treat tokens like --pull=never-mind as safe. Consider matching the flag as an argument token (e.g., boundary-aware regex like (^|\\s)--pull=never(\\s|$) or parsing strings.Fields and comparing exact args).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/hooks/analyze.go` around lines 211 - 214, The current check that
treats commands containing "--pull=never" as safe is vulnerable to substring
false-positives; update the guard around the condition that inspects e.Command
so it tokenizes the command (e.g., via strings.Fields(e.Command)) and checks for
an exact token equal to "--pull=never" (or use a boundary-aware regex like
`(^|\\s)--pull=never(\\s|$)`) instead of strings.Contains; modify the if that
currently reads `if strings.Contains(e.Command, "docker run") &&
strings.Contains(e.Command, "--pull=never") { continue }` to use the
tokenized/regex check for the flag while keeping the existing "docker run"
check.
| func SettingsPaths(claudeDir string) []string { | ||
| paths := []string{filepath.Join(claudeDir, "settings.json")} | ||
|
|
||
| matches, _ := filepath.Glob(filepath.Join(claudeDir, "projects", "*", "settings.local.json")) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider logging glob errors for observability.
The error from filepath.Glob is silently discarded. While the pattern is hardcoded and valid, Glob can also return errors for permission issues or filesystem problems. Silently continuing with zero project settings might mask permission problems that the user would want to know about.
Consider recording the glob error in a similar way to how Scan records ParseErrors, or at minimum logging it for diagnostics.
💡 Optional improvement
If the Inventory structure allowed it, you could record glob errors:
func SettingsPaths(claudeDir string) []string {
paths := []string{filepath.Join(claudeDir, "settings.json")}
- matches, _ := filepath.Glob(filepath.Join(claudeDir, "projects", "*", "settings.local.json"))
+ matches, err := filepath.Glob(filepath.Join(claudeDir, "projects", "*", "settings.local.json"))
+ if err != nil {
+ // Log or record the error for diagnostics
+ }
sort.Strings(matches)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/hooks/paths.go` at line 19, The filepath.Glob call currently
discards its error (matches, _ := filepath.Glob(...)), which can hide permission
or filesystem problems; change it to capture the error (matches, err :=
filepath.Glob(filepath.Join(claudeDir, "projects", "*",
"settings.local.json"))), and if err != nil record or log it for
observability—either append the error to the same Scan.ParseErrors collection
used elsewhere or emit a clear log entry so callers know the glob failed; keep
the existing behavior of using matches when err==nil.
Comment on lines
+54
to
+68
| // Scan reads each settings file path in order and returns the combined hook | ||
| // inventory. Missing files are silently skipped; malformed files are recorded | ||
| // in Inventory.ParseErrors and do not abort the scan. The only error Scan | ||
| // returns is for an unreadable (but existing) file's non-IsNotExist failure — | ||
| // callers can otherwise treat the inventory as best-effort. | ||
| func Scan(paths []string) (*Inventory, error) { | ||
| inv := &Inventory{} | ||
| for _, p := range paths { | ||
| data, err := os.ReadFile(p) | ||
| if err != nil { | ||
| if os.IsNotExist(err) { | ||
| continue // a missing settings file is normal | ||
| } | ||
| inv.ParseErrors = append(inv.ParseErrors, ParseError{File: p, Err: err}) | ||
| continue |
There was a problem hiding this comment.
Inconsistency: function doc vs. implementation on error handling.
The doc comment (lines 56-58) states: "The only error Scan returns is for an unreadable (but existing) file's non-IsNotExist failure". However, the implementation at lines 67-68 records these failures as ParseError entries instead of returning them as the function's error return value.
Since Scan always returns nil error (line 94), the doc is misleading. Either:
- Update the doc to reflect that all failures are non-fatal and recorded in
Inventory.ParseErrors, or - Change the implementation to return non-IsNotExist read errors as fatal errors.
Given the fail-safe design mentioned in the PR objectives, the current implementation seems intentional, so updating the doc is likely the right fix.
📝 Proposed doc fix
-// Scan reads each settings file path in order and returns the combined hook
-// inventory. Missing files are silently skipped; malformed files are recorded
-// in Inventory.ParseErrors and do not abort the scan. The only error Scan
-// returns is for an unreadable (but existing) file's non-IsNotExist failure —
-// callers can otherwise treat the inventory as best-effort.
+// Scan reads each settings file path in order and returns the combined hook
+// inventory. Missing files are silently skipped; malformed files and read
+// errors are recorded in Inventory.ParseErrors and do not abort the scan.
+// Scan always succeeds (returns nil error); callers should check ParseErrors
+// for diagnostics.
func Scan(paths []string) (*Inventory, error) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Scan reads each settings file path in order and returns the combined hook | |
| // inventory. Missing files are silently skipped; malformed files are recorded | |
| // in Inventory.ParseErrors and do not abort the scan. The only error Scan | |
| // returns is for an unreadable (but existing) file's non-IsNotExist failure — | |
| // callers can otherwise treat the inventory as best-effort. | |
| func Scan(paths []string) (*Inventory, error) { | |
| inv := &Inventory{} | |
| for _, p := range paths { | |
| data, err := os.ReadFile(p) | |
| if err != nil { | |
| if os.IsNotExist(err) { | |
| continue // a missing settings file is normal | |
| } | |
| inv.ParseErrors = append(inv.ParseErrors, ParseError{File: p, Err: err}) | |
| continue | |
| // Scan reads each settings file path in order and returns the combined hook | |
| // inventory. Missing files are silently skipped; malformed files and read | |
| // errors are recorded in Inventory.ParseErrors and do not abort the scan. | |
| // Scan always succeeds (returns nil error); callers should check ParseErrors | |
| // for diagnostics. | |
| func Scan(paths []string) (*Inventory, error) { | |
| inv := &Inventory{} | |
| for _, p := range paths { | |
| data, err := os.ReadFile(p) | |
| if err != nil { | |
| if os.IsNotExist(err) { | |
| continue // a missing settings file is normal | |
| } | |
| inv.ParseErrors = append(inv.ParseErrors, ParseError{File: p, Err: err}) | |
| continue |
🧰 Tools
🪛 golangci-lint (2.12.2)
[medium] 62-62: G304: Potential file inclusion via variable
(gosec)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/hooks/scan.go` around lines 54 - 68, The doc comment for Scan is
inaccurate: the implementation reads files with os.ReadFile, records
non-IsNotExist read failures into Inventory.ParseErrors (using ParseError) and
always returns a nil error; update the comment on Scan to state that Scan is
best-effort, that missing files are skipped, and that any
unreadable-but-existing file errors are recorded in Inventory.ParseErrors
(ParseError) rather than being returned as a fatal error; leave the
implementation of Scan, Inventory.ParseErrors, and ParseError unchanged.
Addresses CodeRabbit nitpicks on #75: handle hooks.Scan's error defensively and render a WARN for each malformed settings file (doctor now flags an unparseable settings.json instead of silently skipping it); tighten a test tmp-dir to 0o750 (gosec). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jun 13, 2026
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.
Summary
Adds a hook-safety suite to
hookwise doctor— the highest-leverage defense against the hook-overload incident class (37 hooks / 12 events caused a 2-hour outage). Closes #33, #34, #35, #36.New
internal/hookspackage: a hermetic Claude Code settings scanner + four pure analyses, all TDD'd. Wired intodoctoras "Check 7".Checks (each matches its issue's exact output format)
SCAN hooks: N across M events+ per-event breakdown; WARN/FAIL when a hot-path event (PreToolUse/PostToolUse) exceeds 5/10 or other events exceed 3/8exec.LookPath; FAIL with the count of dependent hooksuvx/uv run/npx/pip install/curl/wget/docker runonly on PreToolUse/PostToolUse;docker run --pull=neveris exempthookwise dispatch,claude-code-guardian,skill-routing-guard) share an eventReal-world smoke (on the maintainer's own settings)
It immediately surfaced genuine sprawl + a 4× duplicated guard — the feature works.
Design notes
exec.LookPathis injected so the binary check is testable without touching the real PATH.~/.claude/settings.json+~/.claude/projects/*/settings.local.json.internal/hooksadded to the guardedtask test:go:unittarget (retro-009: no barego test).Verification
task test:go:unit+test:go:contract+test:go:archall green.task install+hookwise doctorsmoked on real data.Stream B of the post-Dolt backlog (B→C→D). Out of scope: the untracked working-tree files.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
hookwise doctorcommand now performs comprehensive hook safety checks, including inventory analysis, sprawl detection, missing binary detection, network dependency scanning, and duplicate identification.Tests
Chores