impl 1 for hooks in command frontmatters #371
Conversation
WalkthroughIntroduces a hooks system with a new CLI command handler, hook type enumeration, default frontmatter integration, and a JSON-based runtime dispatcher. Includes pre-tool-use validation to restrict file modifications under the spectr/changes directory during the apply command. Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as CLI Command
participant Handler as hooks.Handle
participant Dispatch as Dispatch Logic
participant PreToolUse as Pre-Tool-Use Handler
User->>CLI: spectr hooks HookPreToolUse --command "apply"
CLI->>Handler: Run() → Handle(HookType, command, stdin, stdout)
Handler->>Handler: Decode JSON from stdin → HookInput
Handler->>Dispatch: Dispatch based on HookType
Dispatch->>PreToolUse: HookPreToolUse detected
PreToolUse->>PreToolUse: Validate: apply command?<br/>Check tool: Edit/Write?<br/>Parse FilePath<br/>Match spectr/changes pattern?
PreToolUse-->>Dispatch: Return HookOutput (Blocked: true/false, Message)
Dispatch-->>Handler: Result
Handler->>Handler: Encode HookOutput to JSON
Handler-->>User: Write JSON to stdout
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
@greptile |
![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
🤖 Fix all issues with AI agents
In `@internal/domain/hooktype.go`:
- Around line 37-41: The current check int(h) < len(names) allows negative
HookType values to index names and panic; update the guard in the HookType
string conversion (the method using variable h and the names slice) to ensure h
is within bounds by checking both lower and upper bounds (e.g., int(h) >= 0 &&
int(h) < len(names)) and return unknownHookType otherwise so negative values no
longer cause an index-out-of-range panic.
In `@internal/hooks/pretooluse.go`:
- Around line 37-45: The hook currently swallows malformed JSON and empty file
path cases by returning an empty HookOutput; update the error handling around
json.Unmarshal(input.ToolInput, &ti) and the subsequent ti.FilePath check
(referencing toolInput, input.ToolInput, ti.FilePath and HookOutput) to
explicitly handle failures: when Unmarshal returns an error, log or return a
HookOutput containing an error/warning message and a non-allowing status for
apply, and likewise return a clear error HookOutput if ti.FilePath == "" instead
of silently allowing; ensure the change is surfaced where the hook's apply logic
consumes HookOutput so tests can assert behavior for malformed JSON and empty
FilePath.
🧹 Nitpick comments (4)
internal/domain/hooktype.go (1)
6-58: Three parallel lists must be kept in sync — consider a single source of truth.The iota constants (Lines 6-18), the
namesslice inString()(Lines 24-36), and the slice inAllHookTypes()(Lines 46-58) all enumerate the same set. Adding or reordering a hook type requires updating all three in lockstep, which is error-prone.A common Go pattern is a single
var hookTypes = [...]struct{ typ HookType; name string }{…}table that drivesString(),AllHookTypes(), andParseHookType().internal/domain/frontmatter_slashnext_test.go (1)
54-60: Hard-coded field count is brittle.If another field is added to frontmatter, this assertion silently breaks. Consider computing the expected count from
len(expectedValues) + 1(for hooks) instead of a magic4.♻️ Suggested improvement
- // Verify total field count (3 scalar + 1 hooks = 4) - if len(fm) != 4 { + // Verify total field count (scalar fields + hooks) + expectedFieldCount := len(expectedValues) + 1 // +1 for hooks + if len(fm) != expectedFieldCount { t.Errorf( - "SlashNext frontmatter has %d fields, want 4", + "SlashNext frontmatter has %d fields, want %d", len(fm), + expectedFieldCount, ) }internal/hooks/handler.go (1)
40-57: Consider logging or returning an error for unknown hook types.Lines 43-53 explicitly enumerate all known no-op hook types, and line 56 returns the same empty output as a catch-all default. If a new
HookTypeis added but not wired intodispatch, it silently returns a no-op with no signal. A log statement or distinct handling for the default case would aid debugging.internal/hooks/pretooluse_test.go (1)
12-152: Consolidate unit tests into a table-driven test witht.Run().Tests 1–7 share identical structure: build
HookInput, callhandlePreToolUse, assertBlocked. These are a textbook candidate for a single table-driven test per the coding guidelines.♻️ Example table-driven refactor
func TestHandlePreToolUse(t *testing.T) { tests := []struct { name string command string toolName string toolInput string wantBlock bool }{ { name: "blocks proposal modification during apply", command: "apply", toolName: "Edit", toolInput: `{"file_path": "spectr/changes/foo/proposal.md", "old_string": "a", "new_string": "b"}`, wantBlock: true, }, { name: "allows tasks.jsonc during apply", command: "apply", toolName: "Write", toolInput: `{"file_path": "spectr/changes/foo/tasks.jsonc", "content": "{}"}`, wantBlock: false, }, { name: "allows non-apply command", command: "proposal", toolName: "Edit", toolInput: `{"file_path": "spectr/changes/foo/proposal.md", "old_string": "a", "new_string": "b"}`, wantBlock: false, }, // ... remaining cases } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { input := HookInput{ SessionID: "test-session", HookEventName: "PreToolUse", ToolName: tt.toolName, ToolInput: json.RawMessage(tt.toolInput), } output := handlePreToolUse(tt.command, &input) if output.Blocked != tt.wantBlock { t.Errorf("Blocked = %v, want %v", output.Blocked, tt.wantBlock) } }) } }As per coding guidelines: "Use table-driven test style with
t.Run()for test organization" and "Use table-driven tests with t.Run() subtests and fixtures stored in testdata/".
| if int(h) < len(names) { | ||
| return names[h] | ||
| } | ||
|
|
||
| return unknownHookType |
There was a problem hiding this comment.
Negative HookType values cause an index-out-of-range panic.
int(h) < len(names) is true for negative values (e.g., HookType(-1)), so names[h] will panic.
🐛 Proposed fix
- if int(h) < len(names) {
+ if int(h) >= 0 && int(h) < len(names) {📝 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.
| if int(h) < len(names) { | |
| return names[h] | |
| } | |
| return unknownHookType | |
| if int(h) >= 0 && int(h) < len(names) { | |
| return names[h] | |
| } | |
| return unknownHookType |
🤖 Prompt for AI Agents
In `@internal/domain/hooktype.go` around lines 37 - 41, The current check int(h) <
len(names) allows negative HookType values to index names and panic; update the
guard in the HookType string conversion (the method using variable h and the
names slice) to ensure h is within bounds by checking both lower and upper
bounds (e.g., int(h) >= 0 && int(h) < len(names)) and return unknownHookType
otherwise so negative values no longer cause an index-out-of-range panic.
| // Parse tool_input to get file_path | ||
| var ti toolInput | ||
| if err := json.Unmarshal(input.ToolInput, &ti); err != nil { | ||
| return &HookOutput{} | ||
| } | ||
|
|
||
| if ti.FilePath == "" { | ||
| return &HookOutput{} | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for toolInput struct definition
rg -n 'type toolInput' --type=goRepository: connerohnesorge/spectr
Length of output: 116
🏁 Script executed:
#!/bin/bash
# Get full context of pretooluse.go to understand the function behavior
wc -l internal/hooks/pretooluse.goRepository: connerohnesorge/spectr
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Show the full pretooluse.go file to understand the complete logic
cat -n internal/hooks/pretooluse.goRepository: connerohnesorge/spectr
Length of output: 2011
🏁 Script executed:
#!/bin/bash
# Check the toolInput struct definition in types.go
rg -A5 'type toolInput struct' internal/hooks/types.goRepository: connerohnesorge/spectr
Length of output: 127
🏁 Script executed:
#!/bin/bash
# Check if there are any comments or context around the HookInput struct
rg -B2 -A8 'type HookInput struct' internal/hooks/types.goRepository: connerohnesorge/spectr
Length of output: 479
🏁 Script executed:
#!/bin/bash
# Check if there are any tests or usage examples of Edit/Write tools
rg -n 'Edit\|Write' --type=go internal/hooks/ -B2 -A2Repository: connerohnesorge/spectr
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation or comments about the tool input expectations
rg -n 'ToolInput' --type=go -B1 -A1Repository: connerohnesorge/spectr
Length of output: 2225
🏁 Script executed:
#!/bin/bash
# Check the test file to see if there are tests for malformed input
cat -n internal/hooks/pretooluse_test.goRepository: connerohnesorge/spectr
Length of output: 6110
Add tests for malformed JSON and empty FilePath cases; consider explicit error handling instead of silent allowance.
Lines 39-40 silently allow the request if json.Unmarshal fails, and lines 43-44 do the same if FilePath is empty. While the actual tools would also fail to parse malformed input, this creates a defense-in-depth gap—a mismatch between the hook's JSON schema expectations and what the tool receives could allow writes to protected files.
The tests confirm that the codebase consistently uses {"file_path": "..."} format, so the immediate risk is low. However, the lack of test coverage for malformed/empty input cases and the silent bypass pattern make the guard less robust. Consider either blocking with an error message or logging a warning when parsing fails during the apply command.
🤖 Prompt for AI Agents
In `@internal/hooks/pretooluse.go` around lines 37 - 45, The hook currently
swallows malformed JSON and empty file path cases by returning an empty
HookOutput; update the error handling around json.Unmarshal(input.ToolInput,
&ti) and the subsequent ti.FilePath check (referencing toolInput,
input.ToolInput, ti.FilePath and HookOutput) to explicitly handle failures: when
Unmarshal returns an error, log or return a HookOutput containing an
error/warning message and a non-allowing status for apply, and likewise return a
clear error HookOutput if ti.FilePath == "" instead of silently allowing; ensure
the change is surfaced where the hook's apply logic consumes HookOutput so tests
can assert behavior for malformed JSON and empty FilePath.
1 participant
Summary by CodeRabbit
New Features
Tests