feat: add comprehensive integration tests for hook firing behavior #52
Conversation
- Add tests to verify hook isolation and prevent duplicate execution - Test edge cases including race conditions and error handling - Add transcript-based tests for realistic scenarios - Document findings about hook system architecture These tests ensure that pre-tool-use and post-tool-use hooks execute independently and that multiple hooks don't fire when they shouldn't. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughThis update introduces a major refactor and simplification of the Claude hooks system. It streamlines hook handler definitions, overhauls transcript handling to use cached, line-based approaches, and unifies hook payload and result types. The CLI and initialization logic are rewritten for synchronous, minimal operation. Documentation and integration tests are added and updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as User/CLI
participant Init as Init Command
participant FS as File System
participant Bun as Bun Executable
CLI->>Init: Run `claude-hooks init`
Init->>FS: Check for existing hooks
alt No hooks or --force
Init->>FS: Create directories
Init->>FS: Copy template files (index.ts, lib.ts, session.ts)
Init->>Bun: Locate Bun executable
Init->>FS: Write settings.json mapping hook types to Bun commands
Init-->>CLI: Display success message
else Hooks exist and no --force
Init-->>CLI: Error and abort
end
sequenceDiagram
participant HookRunner as runHooks
participant STDIN as Hook Input
participant Handler as HookHandler
participant Transcript as Transcript Cache
STDIN->>HookRunner: Send JSON payload
HookRunner->>Transcript: (if needed) Load transcript lines (cached)
HookRunner->>Handler: Dispatch to matching handler by type
Handler->>Transcript: Call transcript helper functions
Handler-->>HookRunner: Return HookResult or undefined
HookRunner-->>STDIN: Output result as JSON (if any)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Suggested labels
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
test/integration/hook-firing.test.ts (1)
1-6: Sort imports according to project standards.Imports should be sorted to maintain consistency.
-import {expect} from 'chai' -import {spawn} from 'node:child_process' -import * as fs from 'fs-extra' -import * as path from 'node:path' -import {fileURLToPath} from 'node:url' -import {tmpdir} from 'node:os' +import { expect } from 'chai' +import * as fs from 'fs-extra' +import { spawn } from 'node:child_process' +import { tmpdir } from 'node:os' +import * as path from 'node:path' +import { fileURLToPath } from 'node:url'test/integration/hook-edge-cases.test.ts (1)
1-6: Sort imports according to project standards.-import {expect} from 'chai' -import {spawn} from 'node:child_process' -import * as fs from 'fs-extra' -import * as path from 'node:path' -import {fileURLToPath} from 'node:url' -import {tmpdir} from 'node:os' +import { expect } from 'chai' +import * as fs from 'fs-extra' +import { spawn } from 'node:child_process' +import { tmpdir } from 'node:os' +import * as path from 'node:path' +import { fileURLToPath } from 'node:url'optimization-summary.md (1)
46-46: Remove trailing colons from headings.Markdown headings should not have trailing punctuation.
-### Stream-based processing for large files: +### Stream-based processing for large files-### Efficient searching: +### Efficient searching-### Get recent messages efficiently: +### Get recent messages efficientlyAlso applies to: 58-58, 66-66
test/integration/transcript-based-hooks.test.ts (1)
241-316: Comprehensive state isolation test!The test thoroughly validates:
- Shared state management between hook invocations
- Isolation between PreToolUse and PostToolUse handlers
- Detection of potential state contamination
One suggestion: Consider adding a test case where the state file doesn't exist initially to ensure proper initialization handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
optimization-summary.md(1 hunks)templates/hooks/lib-optimized.ts(1 hunks)test/integration/README.md(1 hunks)test/integration/hook-edge-cases.test.ts(1 hunks)test/integration/hook-firing.test.ts(1 hunks)test/integration/transcript-based-hooks.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
templates/**/*
📄 CodeRabbit Inference Engine (CLAUDE.md)
Hook templates must be stored in the
templates/directory and copied to the user's.claude/directory when initialized.
Files:
templates/hooks/lib-optimized.ts
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
PR: johnlindquist/claude-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:51:09.840Z
Learning: Ensure all existing tests pass before pushing.
📚 Learning: applies to .claude/{settings.json,hooks/index.ts,hooks/lib.ts,hooks/session.ts} : the generated hook...
Learnt from: CR
PR: johnlindquist/claude-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:51:09.840Z
Learning: Applies to .claude/{settings.json,hooks/index.ts,hooks/lib.ts,hooks/session.ts} : The generated hook system must create `.claude/settings.json`, `.claude/hooks/index.ts`, `.claude/hooks/lib.ts`, and `.claude/hooks/session.ts`.
Applied to files:
test/integration/README.mdtest/integration/hook-firing.test.tstest/integration/hook-edge-cases.test.tstest/integration/transcript-based-hooks.test.tstemplates/hooks/lib-optimized.ts
📚 Learning: test files should be organized and run using the provided bun scripts: `bun test`, `bun run test:uni...
Learnt from: CR
PR: johnlindquist/claude-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:51:09.840Z
Learning: Test files should be organized and run using the provided bun scripts: `bun test`, `bun run test:unit`, `bun run test:integration`, `bun run test:smoke`, `bun run test:coverage`.
Applied to files:
test/integration/README.mdtest/integration/hook-firing.test.tstest/integration/hook-edge-cases.test.tstest/integration/transcript-based-hooks.test.ts
📚 Learning: applies to templates/**/* : hook templates must be stored in the `templates/` directory and copied t...
Learnt from: CR
PR: johnlindquist/claude-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:51:09.840Z
Learning: Applies to templates/**/* : Hook templates must be stored in the `templates/` directory and copied to the user's `.claude/` directory when initialized.
Applied to files:
test/integration/README.md
📚 Learning: test from different directories to catch path-related issues....
Learnt from: CR
PR: johnlindquist/claude-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:51:09.840Z
Learning: Test from different directories to catch path-related issues.
Applied to files:
test/integration/README.md
📚 Learning: test with a tsconfig.json in the current directory....
Learnt from: CR
PR: johnlindquist/claude-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:51:09.840Z
Learning: Test with a tsconfig.json in the current directory.
Applied to files:
test/integration/README.mdtest/integration/hook-firing.test.tstest/integration/hook-edge-cases.test.tstest/integration/transcript-based-hooks.test.ts
📚 Learning: hooks are executed using the bun runtime (required dependency)....
Learnt from: CR
PR: johnlindquist/claude-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:51:09.840Z
Learning: Hooks are executed using the Bun runtime (required dependency).
Applied to files:
test/integration/README.mdtest/integration/hook-firing.test.tstest/integration/hook-edge-cases.test.tstest/integration/transcript-based-hooks.test.tstemplates/hooks/lib-optimized.ts
🪛 GitHub Actions: test
test/integration/hook-firing.test.ts
[error] 401-401: Biome lint error: Unused variable 'lastLine'.
[error] 421-421: Biome lint error: Unused function parameter 'code'.
[error] 438-438: Biome lint error: Unused variable 'e' in catch block.
[error] 1-6: Biome assist/source/organizeImports: Imports and exports are not sorted.
[error] 19-380: Biome formatting errors detected. Formatter would have printed changes to commas, whitespace, and code style.
test/integration/hook-edge-cases.test.ts
[error] 40-40: Biome lint error: Unused variable 'executionLog'.
[error] 361-361: Biome lint error: Unused function parameter 'code'.
[error] 376-376: Biome lint error: Unused variable 'e' in catch block.
[error] 1-6: Biome assist/source/organizeImports: Imports and exports are not sorted.
[error] 17-379: Biome formatting errors detected. Formatter would have printed changes to whitespace, commas, and code style.
test/integration/transcript-based-hooks.test.ts
[error] 345-345: Biome lint error: Unused function parameter 'code'.
[error] 360-360: Biome lint error: Unused variable 'e' in catch block.
[error] 1-6: Biome assist/source/organizeImports: Imports and exports are not sorted.
[error] 14-75: Biome formatting errors detected. Formatter would have printed changes to commas, quotes, and whitespace.
templates/hooks/lib-optimized.ts
[error] 5-5: Biome lint error: Unused import 'once' from 'events'.
[error] 597-597: Biome lint error: Unused variable 'e' in catch block.
[error] 3-5: Biome assist/source/organizeImports: Imports and exports are not sorted.
🪛 markdownlint-cli2 (0.17.2)
optimization-summary.md
46-46: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
58-58: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
66-66: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🔇 Additional comments (6)
test/integration/README.md (1)
1-53: Well-documented integration test suite!The README provides excellent documentation of the hook system's architecture and test coverage. The key findings section clearly explains the isolation model and command-line argument-based hook type determination.
optimization-summary.md (1)
1-86: Excellent optimization documentation!The summary clearly explains all the optimizations implemented, including streaming, caching, and new functions. The usage examples and migration guidance are particularly helpful.
test/integration/transcript-based-hooks.test.ts (4)
78-108: Well-structured test setup!The test setup properly:
- Creates isolated temporary directories for each test
- Copies required hook template files
- Generates sample transcript data
- Handles bun executable location with appropriate fallback
- Ensures cleanup after each test
110-177: Excellent test coverage for transcript access and hook isolation!The test effectively validates:
- Transcript data access without triggering unintended hooks
- Proper isolation between PreToolUse and PostToolUse hooks
- Both single message and full conversation history retrieval
The use of console.log markers for assertions is a good pattern for integration testing with spawned processes.
179-239: Strong recursion prevention test!The test effectively validates:
- Detection and prevention of recursive hook calls
- Proper error handling with permission denial
- Tool usage history analysis without triggering recursion
The recursionDepth tracking is a good defensive mechanism.
12-76: Fix formatting issues in sample transcript data.The sample transcript has formatting issues that need to be addressed to pass linting.
Key formatting issues to fix:
- Use consistent quotes throughout (prefer single quotes)
- Remove trailing commas after last object properties
- Fix indentation and whitespace
For the text content on line 50, ensure proper escaping:
- text: 'I\'ll help you implement a login feature. Let me first check your existing code.' + text: "I'll help you implement a login feature. Let me first check your existing code."Likely an incorrect or invalid review comment.
templates/hooks/lib-optimized.ts
Outdated
| import * as fs from 'fs' | ||
| import * as readline from 'readline' | ||
| import { once } from 'events' | ||
|
|
There was a problem hiding this comment.
Remove unused import and sort imports.
The once import from 'events' is unused. Also, imports should be sorted according to the project's linting rules.
import * as fs from 'fs'
-import * as readline from 'readline'
-import { once } from 'events'
+import * as readline from 'readline'📝 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.
| import * as fs from 'fs' | |
| import * as readline from 'readline' | |
| import { once } from 'events' | |
| import * as fs from 'fs' | |
| import * as readline from 'readline' |
🧰 Tools
🪛 GitHub Actions: test
[error] 5-5: Biome lint error: Unused import 'once' from 'events'.
[error] 3-5: Biome assist/source/organizeImports: Imports and exports are not sorted.
🤖 Prompt for AI Agents
In templates/hooks/lib-optimized.ts around lines 3 to 6, remove the unused
import 'once' from 'events' and reorder the remaining imports to comply with the
project's linting rules for import sorting.
templates/hooks/lib-optimized.ts
Outdated
| setInterval(() => { | ||
| const now = Date.now() | ||
| for (const [path, cache] of transcriptCache.entries()) { | ||
| if (now - cache.timestamp > CACHE_TTL) { | ||
| transcriptCache.delete(path) | ||
| } | ||
| } | ||
| }, CACHE_TTL) |
There was a problem hiding this comment.
Potential memory leak with uncleaned interval.
The setInterval for cache cleanup will keep running indefinitely and could prevent proper process termination. Consider storing the interval ID and providing a cleanup function.
+// Store interval ID for cleanup
+let cacheCleanupInterval: NodeJS.Timeout | null = null;
+
// Auto-clear old cache entries
-setInterval(() => {
+cacheCleanupInterval = setInterval(() => {
const now = Date.now()
for (const [path, cache] of transcriptCache.entries()) {
if (now - cache.timestamp > CACHE_TTL) {
transcriptCache.delete(path)
}
}
}, CACHE_TTL)
+
+// Export cleanup function
+export function stopCacheCleanup(): void {
+ if (cacheCleanupInterval) {
+ clearInterval(cacheCleanupInterval)
+ cacheCleanupInterval = null
+ }
+}🤖 Prompt for AI Agents
In templates/hooks/lib-optimized.ts around lines 351 to 358, the setInterval
used for cache cleanup runs indefinitely, risking a memory leak and preventing
process termination. Fix this by storing the interval ID returned by setInterval
in a variable and expose a cleanup function that calls clearInterval on this ID
to stop the interval when no longer needed.
templates/hooks/lib-optimized.ts
Outdated
| handleHookPayload(inputData, hook_type, handlers).catch((error) => { | ||
| console.error('Hook error:', error) | ||
| console.log(JSON.stringify({action: 'continue'})) | ||
| }) | ||
| } catch (e) { | ||
| // Not a complete JSON yet, wait for more data | ||
| } | ||
| }) |
There was a problem hiding this comment.
Handle or remove unused catch variable.
The catch block has an unused variable 'e'. Either use it for error handling or remove it.
try {
const inputData = JSON.parse(buffer)
buffer = '' // Clear buffer on successful parse
handleHookPayload(inputData, hook_type, handlers).catch((error) => {
console.error('Hook error:', error)
console.log(JSON.stringify({action: 'continue'}))
})
- } catch (e) {
+ } catch {
// Not a complete JSON yet, wait for more data
}📝 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.
| handleHookPayload(inputData, hook_type, handlers).catch((error) => { | |
| console.error('Hook error:', error) | |
| console.log(JSON.stringify({action: 'continue'})) | |
| }) | |
| } catch (e) { | |
| // Not a complete JSON yet, wait for more data | |
| } | |
| }) | |
| handleHookPayload(inputData, hook_type, handlers).catch((error) => { | |
| console.error('Hook error:', error) | |
| console.log(JSON.stringify({action: 'continue'})) | |
| }) | |
| } catch { | |
| // Not a complete JSON yet, wait for more data | |
| } |
🧰 Tools
🪛 GitHub Actions: test
[error] 597-597: Biome lint error: Unused variable 'e' in catch block.
🤖 Prompt for AI Agents
In templates/hooks/lib-optimized.ts around lines 593 to 600, the catch block
declares an unused variable 'e'. To fix this, either remove the variable name
from the catch block if it is not needed or use it within the catch block for
error handling or logging. This will eliminate the unused variable warning and
clarify the intent of the error handling.
| let stdout = '' | ||
| let stderr = '' | ||
| let lastLine = '' | ||
|
|
||
| child.stdout.on('data', (data) => { | ||
| const output = data.toString() | ||
| stdout += output | ||
| // Keep track of the last line for the JSON response | ||
| const lines = output.trim().split('\n') | ||
| if (lines.length > 0) { | ||
| lastLine = lines[lines.length - 1] | ||
| } | ||
| }) |
There was a problem hiding this comment.
Remove unused variable 'lastLine'.
The lastLine variable is declared but never used after being assigned.
let stdout = ''
let stderr = ''
- let lastLine = ''
child.stdout.on('data', (data) => {
const output = data.toString()
stdout += output
- // Keep track of the last line for the JSON response
- const lines = output.trim().split('\n')
- if (lines.length > 0) {
- lastLine = lines[lines.length - 1]
- }
})📝 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.
| let stdout = '' | |
| let stderr = '' | |
| let lastLine = '' | |
| child.stdout.on('data', (data) => { | |
| const output = data.toString() | |
| stdout += output | |
| // Keep track of the last line for the JSON response | |
| const lines = output.trim().split('\n') | |
| if (lines.length > 0) { | |
| lastLine = lines[lines.length - 1] | |
| } | |
| }) | |
| let stdout = '' | |
| let stderr = '' | |
| child.stdout.on('data', (data) => { | |
| const output = data.toString() | |
| stdout += output | |
| }) |
🧰 Tools
🪛 GitHub Actions: test
[error] 401-401: Biome lint error: Unused variable 'lastLine'.
🤖 Prompt for AI Agents
In test/integration/hook-firing.test.ts between lines 399 and 411, the variable
'lastLine' is assigned but never used. Remove the declaration and all
assignments to 'lastLine' to clean up unused code.
test/integration/hook-firing.test.ts
Outdated
| child.on('close', (code) => { | ||
| // Try to parse the response from the last line | ||
| let response = {} | ||
| try { | ||
| // Filter out console.log lines and find the JSON response | ||
| const lines = stdout.trim().split('\n') | ||
| const jsonLine = lines.find(line => { | ||
| try { | ||
| JSON.parse(line) | ||
| return true | ||
| } catch { | ||
| return false | ||
| } | ||
| }) | ||
| if (jsonLine) { | ||
| response = JSON.parse(jsonLine) | ||
| } | ||
| } catch (e) { | ||
| // Ignore parse errors | ||
| } | ||
|
|
||
| resolve({ | ||
| stdout, | ||
| stderr, | ||
| response | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Remove unused parameters and variables in error handling.
The code parameter and catch variable e are unused.
- child.on('close', (code) => {
+ child.on('close', () => {
// Try to parse the response from the last line
let response = {}
try {
// Filter out console.log lines and find the JSON response
const lines = stdout.trim().split('\n')
const jsonLine = lines.find(line => {
try {
JSON.parse(line)
return true
} catch {
return false
}
})
if (jsonLine) {
response = JSON.parse(jsonLine)
}
- } catch (e) {
+ } catch {
// Ignore parse errors
}🧰 Tools
🪛 GitHub Actions: test
[error] 421-421: Biome lint error: Unused function parameter 'code'.
[error] 438-438: Biome lint error: Unused variable 'e' in catch block.
🤖 Prompt for AI Agents
In test/integration/hook-firing.test.ts around lines 421 to 447, the 'code'
parameter in the 'close' event handler and the catch variable 'e' in the
try-catch block are declared but never used. Remove the 'code' parameter from
the event handler function and omit the catch variable by using an empty catch
block to clean up the code and avoid unused variable warnings.
| // Helper function | ||
| async function runHook(bunExecutable: string, scriptPath: string, hookType: string, payload: any): Promise<{ | ||
| stdout: string | ||
| stderr: string | ||
| response: any | ||
| }> { | ||
| return new Promise((resolve, reject) => { | ||
| const child = spawn(bunExecutable, [scriptPath, hookType], { | ||
| cwd: path.dirname(scriptPath) | ||
| }) | ||
|
|
||
| let stdout = '' | ||
| let stderr = '' | ||
|
|
||
| child.stdout.on('data', (data) => { | ||
| stdout += data.toString() | ||
| }) | ||
|
|
||
| child.stderr.on('data', (data) => { | ||
| stderr += data.toString() | ||
| }) | ||
|
|
||
| child.on('error', (error) => { | ||
| reject(error) | ||
| }) | ||
|
|
||
| child.on('close', (code) => { | ||
| let response = {} | ||
| try { | ||
| const lines = stdout.trim().split('\n') | ||
| const jsonLine = lines.find(line => { | ||
| try { | ||
| JSON.parse(line) | ||
| return true | ||
| } catch { | ||
| return false | ||
| } | ||
| }) | ||
| if (jsonLine) { | ||
| response = JSON.parse(jsonLine) | ||
| } | ||
| } catch (e) { | ||
| // Ignore parse errors | ||
| } | ||
|
|
||
| resolve({ | ||
| stdout, | ||
| stderr, | ||
| response | ||
| }) | ||
| }) | ||
|
|
||
| child.stdin.write(JSON.stringify(payload)) | ||
| child.stdin.end() | ||
| }) | ||
| } No newline at end of file |
There was a problem hiding this comment.
Fix unused parameters in helper function.
The helper function has two unused parameters that trigger linting errors.
Apply this diff to fix the unused parameters:
- child.on('close', (code) => {
+ child.on('close', () => {
let response = {}
try {
const lines = stdout.trim().split('\n')
const jsonLine = lines.find(line => {
try {
JSON.parse(line)
return true
} catch {
return false
}
})
if (jsonLine) {
response = JSON.parse(jsonLine)
}
- } catch (e) {
+ } catch {
// Ignore parse errors
}Additionally, consider adding the exit code to the return value for debugging purposes:
child.on('close', (code) => {
// ... existing logic ...
resolve({
stdout,
stderr,
response,
exitCode: code
})
})🧰 Tools
🪛 GitHub Actions: test
[error] 345-345: Biome lint error: Unused function parameter 'code'.
[error] 360-360: Biome lint error: Unused variable 'e' in catch block.
🤖 Prompt for AI Agents
In test/integration/transcript-based-hooks.test.ts around lines 319 to 374, the
helper function runHook has unused parameters causing lint errors. Remove or
refactor the unused parameters to eliminate these errors. Additionally, modify
the child process 'close' event handler to include the exit code in the resolved
object by adding an exitCode property set to the code argument, improving
debugging information.
…r STDIN buffering
…nd proper Oclif logging
…d include SessionStart
…d compiled locations
…nId handling; chore: add signal-exit dep and regenerate manifest
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
templates/hooks/lib.ts (1)
103-123: Type mismatch: readline.Interface is not AsyncIterableThe function signature declares a return type of
AsyncIterable<string>, butreadline.Interfacedoesn't directly implement this interface. You need to make the readline interface async-iterable.-export function getTranscriptStream(): AsyncIterable<string> { +export async function* getTranscriptStream(): AsyncIterable<string> { const transcriptPath = process.env.TRANSCRIPT_PATH if (!transcriptPath) { - return (async function* () {})() + return } const fs = require('fs') const readline = require('readline') try { const fileStream = fs.createReadStream(transcriptPath) const rl = readline.createInterface({ input: fileStream, crlfDelay: Infinity, }) - return rl + for await (const line of rl) { + yield line + } } catch (_error) { - return (async function* () {})() + // Empty generator on error } }
♻️ Duplicate comments (3)
test/integration/hook-firing.test.ts (3)
328-328: Remove unused variable.The
_lastLinevariable is declared but never used after assignment.- let _lastLine = ''
348-348: Remove unused parameter.The
_codeparameter in the close event handler is unused.- child.on('close', (_code) => { + child.on('close', () => {
359-366: Remove unused catch variables.The catch variables
_eare unused and can be omitted.- } catch (_e) { + } catch { // If parsing fails, try to find JSON in the output try { const jsonMatch = stdout.match(/\{[^}]*\}/) if (jsonMatch) { response = JSON.parse(jsonMatch[0]) } - } catch (_e) { + } catch { // Ignore parse errors }
🧹 Nitpick comments (6)
test/integration/hook-edge-cases.test.ts (1)
72-72: Remove unnecessary unused variable declarationThe
_executionLogvariable declaration serves no purpose and should be removed entirely.- // This tests the scenario where both hooks were firing - const _executionLog: string[] = [] - + // This tests the scenario where both hooks were firing +templates/hooks/lib.ts (1)
198-240: Clean implementation of the hook runner with proper async handlingThe
runHooksfunction properly buffers stdin input and handles both synchronous and asynchronous handlers. The JSON error output format is good for structured logging.Consider adding runtime validation of the input structure to ensure it matches the expected
HookArgsinterface, rather than just casting it.test/commands/init.test.ts (1)
166-172: Consider adding a simpler bun detection test.While mocking the entire bun detection is complex, you could test the error message by temporarily manipulating PATH or using a subprocess with a controlled environment where bun is not available.
Would you like me to provide an example of testing bun detection without complex mocking?
test/smoke/generated-files.test.ts (2)
11-29: Consider adding random suffix for better test isolation.While the timestamp provides some uniqueness, adding a random suffix (like in the unit tests) would provide better isolation when tests run in parallel.
- testDir = path.join(os.tmpdir(), `claude-hooks-smoke-${Date.now()}`) + testDir = path.join(os.tmpdir(), `claude-hooks-smoke-${Date.now()}-${Math.random().toString(36).substring(7)}`)
150-159: Minimal session.ts validation.The test only verifies file existence and presence of exports. Consider adding more specific checks if session.ts has expected structure or key functions.
src/commands/init.ts (1)
62-64: Consider checking all hook files for completeness.Currently only checks
index.ts. For more robust detection, consider checking all required files.private hasExistingHooks(hooksDir: string): boolean { - return fs.existsSync(path.join(hooksDir, 'index.ts')) + const requiredFiles = ['index.ts', 'lib.ts', 'session.ts'] + return requiredFiles.some(file => fs.existsSync(path.join(hooksDir, file))) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
.prettierrc.json(0 hunks)biome.json(1 hunks)package.json(1 hunks)src/commands/help.ts(1 hunks)src/commands/init.ts(1 hunks)src/commands/logs.ts(8 hunks)templates/hooks/index.ts(1 hunks)templates/hooks/lib.ts(2 hunks)templates/hooks/session.ts(2 hunks)test/commands/init.test.ts(2 hunks)test/integration/cli.test.ts(3 hunks)test/integration/hook-edge-cases.test.ts(1 hunks)test/integration/hook-firing.test.ts(1 hunks)test/integration/transcript-based-hooks.test.ts(1 hunks)test/smoke/generated-files.test.ts(4 hunks)
💤 Files with no reviewable changes (1)
- .prettierrc.json
✅ Files skipped from review due to trivial changes (2)
- biome.json
- src/commands/logs.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/integration/transcript-based-hooks.test.ts
🧰 Additional context used
📓 Path-based instructions (4)
package.json
📄 CodeRabbit Inference Engine (CLAUDE.md)
The project must use ESM modules (
"type": "module").
Files:
package.json
{package.json,src/commands/**/*}
📄 CodeRabbit Inference Engine (CLAUDE.md)
When improving CLI help, update the package.json description to match README, update command descriptions in the static description field, run
npx oclif manifestafter changes, and consider removing irrelevant plugins.
Files:
package.jsonsrc/commands/help.tssrc/commands/init.ts
templates/**/*
📄 CodeRabbit Inference Engine (CLAUDE.md)
Hook templates must be stored in the
templates/directory and copied to the user's.claude/directory when initialized.
Files:
templates/hooks/session.tstemplates/hooks/index.tstemplates/hooks/lib.ts
src/commands/**/*
📄 CodeRabbit Inference Engine (CLAUDE.md)
Commands should be implemented in the
src/commands/directory.
Files:
src/commands/help.tssrc/commands/init.ts
🧠 Learnings (15)
📓 Common learnings
Learnt from: CR
PR: johnlindquist/claude-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:51:09.840Z
Learning: Ensure all existing tests pass before pushing.
Learnt from: CR
PR: johnlindquist/claude-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:51:09.840Z
Learning: Applies to .claude/{settings.json,hooks/index.ts,hooks/lib.ts,hooks/session.ts} : The generated hook system must create `.claude/settings.json`, `.claude/hooks/index.ts`, `.claude/hooks/lib.ts`, and `.claude/hooks/session.ts`.
Learnt from: CR
PR: johnlindquist/claude-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:51:09.840Z
Learning: Applies to templates/**/* : Hook templates must be stored in the `templates/` directory and copied to the user's `.claude/` directory when initialized.
📚 Learning: 2025-07-28T15:51:09.840Z
Learnt from: CR
PR: johnlindquist/claude-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:51:09.840Z
Learning: Applies to {package.json,src/commands/**/*} : When improving CLI help, update the package.json description to match README, update command descriptions in the static description field, run `npx oclif manifest` after changes, and consider removing irrelevant plugins.
Applied to files:
package.jsonsrc/commands/help.tstest/integration/cli.test.tstest/commands/init.test.tssrc/commands/init.ts
📚 Learning: 2025-07-28T15:51:09.840Z
Learnt from: CR
PR: johnlindquist/claude-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:51:09.840Z
Learning: Applies to package.json : The project must use ESM modules (`"type": "module"`).
Applied to files:
package.json
📚 Learning: 2025-07-28T15:51:09.840Z
Learnt from: CR
PR: johnlindquist/claude-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:51:09.840Z
Learning: Remember that published packages don't include devDependencies.
Applied to files:
package.json
📚 Learning: 2025-07-28T15:51:09.840Z
Learnt from: CR
PR: johnlindquist/claude-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:51:09.840Z
Learning: Applies to bin/run.js : Set `NODE_ENV=production` in bin/run.js BEFORE importing oclif/core to prevent TypeScript detection warnings.
Applied to files:
package.jsontest/integration/hook-edge-cases.test.tstest/integration/cli.test.tstest/commands/init.test.tstest/smoke/generated-files.test.tssrc/commands/init.ts
📚 Learning: 2025-07-28T15:51:09.840Z
Learnt from: CR
PR: johnlindquist/claude-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:51:09.840Z
Learning: Applies to .claude/{settings.json,hooks/index.ts,hooks/lib.ts,hooks/session.ts} : The generated hook system must create `.claude/settings.json`, `.claude/hooks/index.ts`, `.claude/hooks/lib.ts`, and `.claude/hooks/session.ts`.
Applied to files:
templates/hooks/session.tssrc/commands/help.tstest/integration/hook-edge-cases.test.tstest/integration/cli.test.tstest/integration/hook-firing.test.tstest/commands/init.test.tstest/smoke/generated-files.test.tstemplates/hooks/index.tssrc/commands/init.tstemplates/hooks/lib.ts
📚 Learning: 2025-07-28T15:51:09.840Z
Learnt from: CR
PR: johnlindquist/claude-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:51:09.840Z
Learning: Applies to templates/**/* : Hook templates must be stored in the `templates/` directory and copied to the user's `.claude/` directory when initialized.
Applied to files:
templates/hooks/session.tstest/integration/cli.test.tstest/commands/init.test.tstest/smoke/generated-files.test.tstemplates/hooks/index.tssrc/commands/init.ts
📚 Learning: 2025-07-28T15:51:09.840Z
Learnt from: CR
PR: johnlindquist/claude-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:51:09.840Z
Learning: Test files should be organized and run using the provided bun scripts: `bun test`, `bun run test:unit`, `bun run test:integration`, `bun run test:smoke`, `bun run test:coverage`.
Applied to files:
test/integration/hook-edge-cases.test.tstest/integration/cli.test.tstest/integration/hook-firing.test.tstest/commands/init.test.tstest/smoke/generated-files.test.ts
📚 Learning: 2025-07-28T15:51:09.840Z
Learnt from: CR
PR: johnlindquist/claude-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:51:09.840Z
Learning: Hooks are executed using the Bun runtime (required dependency).
Applied to files:
test/integration/hook-edge-cases.test.tstest/integration/hook-firing.test.tstest/commands/init.test.tstest/smoke/generated-files.test.tstemplates/hooks/index.tstemplates/hooks/lib.ts
📚 Learning: 2025-07-28T15:51:09.840Z
Learnt from: CR
PR: johnlindquist/claude-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:51:09.840Z
Learning: Test with a tsconfig.json in the current directory.
Applied to files:
test/integration/hook-edge-cases.test.tstest/integration/cli.test.tstest/integration/hook-firing.test.tstest/commands/init.test.tstest/smoke/generated-files.test.ts
📚 Learning: 2025-07-28T15:51:09.840Z
Learnt from: CR
PR: johnlindquist/claude-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:51:09.840Z
Learning: Test from different directories to catch path-related issues.
Applied to files:
test/integration/hook-edge-cases.test.tstest/integration/cli.test.tstest/integration/hook-firing.test.tstest/commands/init.test.tstest/smoke/generated-files.test.ts
📚 Learning: 2025-07-28T15:51:09.840Z
Learnt from: CR
PR: johnlindquist/claude-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:51:09.840Z
Learning: Always test with the actual published npm package using `npx package-namelatest` before declaring a fix complete.
Applied to files:
test/integration/cli.test.tstest/smoke/generated-files.test.ts
📚 Learning: 2025-07-28T15:51:09.840Z
Learnt from: CR
PR: johnlindquist/claude-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:51:09.840Z
Learning: Ensure all existing tests pass before pushing.
Applied to files:
test/integration/cli.test.ts
📚 Learning: 2025-07-28T15:51:09.840Z
Learnt from: CR
PR: johnlindquist/claude-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:51:09.840Z
Learning: Applies to bin/run.js : Use `NODE_ENV=production` to affect behavior during import time.
Applied to files:
test/integration/cli.test.tstest/commands/init.test.tstest/smoke/generated-files.test.ts
📚 Learning: 2025-07-28T15:51:09.840Z
Learnt from: CR
PR: johnlindquist/claude-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:51:09.840Z
Learning: Commands for building, linting, formatting, and cleaning should use the provided bun scripts (e.g., `bun run build`, `bun run lint`, `bun run format`, `bun run clean`).
Applied to files:
test/commands/init.test.tstest/smoke/generated-files.test.tssrc/commands/init.ts
🧬 Code Graph Analysis (1)
templates/hooks/session.ts (1)
templates/hooks/lib.ts (1)
HookArgs(4-30)
🔇 Additional comments (36)
package.json (1)
17-18: Dependencies added appropriately for CLI functionality.The addition of
orafor CLI spinners andsignal-exitfor process signal handling aligns well with the project's CLI nature and the refactoring mentioned in the AI summary.templates/hooks/session.ts (2)
4-4: Import update aligns with unified type system.The addition of
HookArgsimport supports the new unified hook payload structure mentioned in the AI summary.
14-19: Improved session ID extraction with fallback logic.The updated logic properly handles both the new
sessionIdproperty fromHookArgsand the legacysession_idproperty, with sensible early return when no session ID is available. This prevents creating empty session files.src/commands/help.ts (3)
5-5: Good use of override keyword.Proper TypeScript practice for overriding base class properties.
21-21: Consistent command output handling.Using
this.warninstead ofconsole.errorfollows oclif best practices for command output.
58-62: Hook types updated to match refactored system.The removal of "SubagentStop" and addition of "SessionStart" aligns with the simplified hook system described in the AI summary.
test/integration/cli.test.ts (3)
19-21: Improved test cleanup strategy.Moving cleanup from
afterEachtoafteris more efficient for integration tests that don't modify shared state between test cases.
32-49: Comprehensive file structure verification.The test now properly verifies all expected files are created and validates the settings structure matches the new simplified format.
113-131: Good test coverage for new CLI features.The new test suite properly validates the
--settingsflag functionality, ensuring custom settings files are created correctly.test/integration/hook-firing.test.ts (3)
1-46: Comprehensive test setup for hook integration testing.The test suite properly sets up isolated environments for each test, copies necessary template files, and includes proper cleanup. This follows testing best practices for integration tests.
48-115: Excellent hook isolation verification.The test properly verifies that only the intended hook handler executes by checking both stderr logs and response content. This addresses the core PR objective of preventing multiple hooks from firing incorrectly.
267-308: Good coverage of blocking behavior.The test properly validates that PreToolUse hooks can conditionally block execution, which is critical security functionality.
test/integration/hook-edge-cases.test.ts (1)
34-34: Proper use of underscore prefix for unused variablesThe underscore prefix convention correctly indicates these variables are intentionally unused, which is a standard practice in TypeScript/JavaScript.
Also applies to: 40-40
templates/hooks/lib.ts (4)
32-53: Well-structured type definitions with good backwards compatibilityThe unified
HookResultinterface and flexibleHookHandlertype are well-designed. The backwards compatibility alias forHookPayloadis a thoughtful addition that prevents breaking changes.
55-101: Efficient caching implementation with reasonable TTLThe transcript caching with a 5-second TTL is a good optimization for preventing excessive file reads while ensuring data freshness.
125-151: Flexible and well-implemented search functionalityThe
searchTranscriptfunction provides excellent flexibility with support for both string and regex patterns, case sensitivity options, result limits, and reverse search capability.
153-196: Comprehensive set of transcript query utilitiesThe helper functions provide a good range of transcript querying capabilities. The timestamp-based filtering and regex patterns for message types are well-implemented.
test/commands/init.test.ts (5)
12-35: Excellent test isolation setup!The use of unique temporary directories with timestamp and random string suffixes, along with proper CWD management, ensures complete test isolation and prevents race conditions in parallel test execution.
37-52: Good handling of different execution contexts.The fallback mechanism ensures tests work in both development and compiled environments.
54-103: Comprehensive verification of initialization output.Tests correctly verify all required files are created and settings structure matches the expected format, aligning with the documented requirements for
.claude/{settings.json,hooks/index.ts,hooks/lib.ts,hooks/session.ts}.
105-152: Thorough testing of force flag behavior.Good coverage of both the error case (hooks already exist) and the overwrite scenario. The assertion pattern using
errorOccurredflag ensures the error is actually thrown.
154-164: Clean test for custom settings file feature.Effectively validates the --settings flag functionality.
test/smoke/generated-files.test.ts (4)
31-57: Comprehensive validation of settings.json structure.Tests properly verify the new simplified hook configuration format.
59-101: Well-structured validation of index.ts content.The regex patterns effectively verify the expected code structure while allowing for formatting variations.
103-148: Thorough validation of lib.ts components.Good coverage of the unified hook interfaces and transcript handling utilities.
161-191: Excellent runtime validation test.This integration test effectively validates that the generated hooks can actually execute with proper input/output handling. The graceful handling of missing bun with
this.skip()is appropriate.templates/hooks/index.ts (4)
32-39: Clear example handler.Good template for users to extend with their own post-processing logic.
42-46: Well-implemented notification handler.Good use of default severity level.
49-67: Clean session lifecycle handlers.Good examples for session start and stop events. The consistent use of
console.errorkeeps stdout available for hook results.
70-76: Clean hook registration.All handlers properly registered with the hook system.
src/commands/init.ts (6)
7-30: Well-structured command definition.Clear description, examples, and flags that align with the documented CLI behavior.
31-60: Clean and logical initialization flow.Good separation of concerns with clear error messages at each step.
72-94: Robust template file copying.Good handling of both source and compiled contexts with proper error reporting.
96-134: Comprehensive bun detection strategy.Excellent coverage of different installation locations and methods with proper cross-platform support.
136-155: Clean settings file generation.Elegant use of reduce to build the hooks configuration object.
163-171: Fix optional chaining in example code.The example uses
args.toolArgs?.commandbut this could still error if toolArgs is defined but command is not.export const PreToolUse: HookHandler = (args) => { - if (args.toolName === 'Bash' && args.toolArgs?.command?.includes('rm -rf')) { + const command = (args.toolArgs as any)?.command + if (args.toolName === 'Bash' && command?.includes('rm -rf')) { return { block: true, message: "Blocked dangerous command" } } }⛔ Skipped due to learnings
Learnt from: CR PR: johnlindquist/claude-hooks#0 File: CLAUDE.md:0-0 Timestamp: 2025-07-28T15:51:09.840Z Learning: Applies to {package.json,src/commands/**/*} : When improving CLI help, update the package.json description to match README, update command descriptions in the static description field, run `npx oclif manifest` after changes, and consider removing irrelevant plugins.Learnt from: CR PR: johnlindquist/claude-hooks#0 File: CLAUDE.md:0-0 Timestamp: 2025-07-28T15:51:09.840Z Learning: Always test with the actual published npm package using `npx package-namelatest` before declaring a fix complete.
| // Block dangerous commands | ||
| if (command.includes('rm -rf /') || command.includes('rm -rf ~')) { | ||
| console.error('❌ Dangerous command detected! Blocking execution.') | ||
| return { | ||
| permissionDecision: 'deny', | ||
| permissionDecisionReason: `Dangerous command detected: ${command}`, | ||
| block: true, | ||
| message: `Dangerous command blocked: ${command}`, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Strengthen dangerous command detection.
The current detection using includes() might miss command variations or have false positives. Consider using more robust pattern matching.
// Block dangerous commands
- if (command.includes('rm -rf /') || command.includes('rm -rf ~')) {
+ // Use regex for more robust detection
+ const dangerousPatterns = [
+ /\brm\s+-rf\s+\/(?:\s|$)/, // rm -rf / with word boundaries
+ /\brm\s+-rf\s+~(?:\/|$|\s)/, // rm -rf ~ or ~/...
+ /\brm\s+-rf\s+\/\*/, // rm -rf /*
+ ]
+
+ if (dangerousPatterns.some(pattern => pattern.test(command))) {
return {
block: true,
message: `Dangerous command blocked: ${command}`,
}
}📝 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.
| // Block dangerous commands | |
| if (command.includes('rm -rf /') || command.includes('rm -rf ~')) { | |
| console.error('❌ Dangerous command detected! Blocking execution.') | |
| return { | |
| permissionDecision: 'deny', | |
| permissionDecisionReason: `Dangerous command detected: ${command}`, | |
| block: true, | |
| message: `Dangerous command blocked: ${command}`, | |
| } | |
| } | |
| } | |
| // Block dangerous commands | |
| + // Use regex for more robust detection | |
| + const dangerousPatterns = [ | |
| + /\brm\s+-rf\s+\/(?:\s|$)/, // rm -rf / with word boundaries | |
| + /\brm\s+-rf\s+~(?:\/|$|\s)/, // rm -rf ~ or ~/... | |
| + /\brm\s+-rf\s+\/\*/, // rm -rf /* | |
| + ] | |
| + | |
| + if (dangerousPatterns.some(pattern => pattern.test(command))) { | |
| return { | |
| block: true, | |
| message: `Dangerous command blocked: ${command}`, | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In templates/hooks/index.ts around lines 19 to 26, the current dangerous command
detection uses simple string includes which can miss variations or cause false
positives. Replace the includes checks with regular expressions that more
precisely match dangerous command patterns like 'rm -rf /' or 'rm -rf ~' to
improve accuracy and robustness of detection.
| // This file contains types and utilities for Claude hooks | ||
| // DO NOT MODIFY - This is auto-generated |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Misleading comment for a template file
This file is in the templates/ directory, which according to the coding guidelines should be copied to the user's .claude/ directory when initialized. The comment "DO NOT MODIFY - This is auto-generated" is misleading since this is the source template, not the generated output.
-// This file contains types and utilities for Claude hooks
-// DO NOT MODIFY - This is auto-generated
+// This file contains types and utilities for Claude hooks
+// Template file - will be copied to .claude/hooks/lib.ts during initialization📝 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.
| // This file contains types and utilities for Claude hooks | |
| // DO NOT MODIFY - This is auto-generated | |
| // This file contains types and utilities for Claude hooks | |
| // Template file - will be copied to .claude/hooks/lib.ts during initialization |
🤖 Prompt for AI Agents
In templates/hooks/lib.ts at lines 1 to 2, the comment states "DO NOT MODIFY -
This is auto-generated," which is misleading because this file is a source
template meant to be copied and potentially modified by users. Update the
comment to clarify that this is a template file intended for copying to the
user's .claude/ directory and can be modified there, removing the "DO NOT
MODIFY" warning.
Summary
Test plan
[x] All integration tests pass
[x] Tests verify hook isolation works correctly
[x] Edge cases and race conditions are covered
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores
Tests