fix: read Claude Code tool_response in PostToolUse hook#578
Conversation
|
@yixinforu is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe post-tool-use hook now prefers Claude Code's ChangesTool Response Fallback
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 unit tests (beta)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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.
🧹 Nitpick comments (1)
test/post-tool-use-hook.test.ts (1)
26-77: ⚡ Quick winEnsure test server cleanup runs on all failure paths.
server.close()should be in afinallyblock so rejected spawn/parse paths don’t leak the server and hang subsequent tests.Suggested refactor
async function runHook(payload: Record<string, unknown>) { let received: unknown; const server = createServer(async (req, res) => { const body = await readBody(req); received = JSON.parse(body); res.writeHead(200, { "Content-Type": "application/json" }); res.end(JSON.stringify({ ok: true })); }); await new Promise<void>((resolve) => { server.listen(0, "127.0.0.1", resolve); }); - const address = server.address(); - if (!address || typeof address === "string") { - throw new Error("test server did not bind to a TCP port"); - } - - const result = await new Promise<{ - exitCode: number | null; - stderr: string; - stdout: string; - }>((resolve, reject) => { - const child = spawn(process.execPath, [HOOK], { - env: { - PATH: process.env["PATH"] ?? "", - AGENTMEMORY_URL: `http://127.0.0.1:${address.port}`, - }, - stdio: ["pipe", "pipe", "pipe"], - }); - - let stdout = ""; - let stderr = ""; - child.stdout.on("data", (chunk) => { - stdout += chunk.toString(); - }); - child.stderr.on("data", (chunk) => { - stderr += chunk.toString(); - }); - child.on("error", reject); - child.on("close", (exitCode) => { - resolve({ exitCode, stderr, stdout }); - }); - child.stdin.end(JSON.stringify(payload)); - }); - - await new Promise<void>((resolve, reject) => { - server.close((err) => (err ? reject(err) : resolve())); - }); - - return { ...result, received }; + try { + const address = server.address(); + if (!address || typeof address === "string") { + throw new Error("test server did not bind to a TCP port"); + } + + const result = await new Promise<{ + exitCode: number | null; + stderr: string; + stdout: string; + }>((resolve, reject) => { + const child = spawn(process.execPath, [HOOK], { + env: { + PATH: process.env["PATH"] ?? "", + AGENTMEMORY_URL: `http://127.0.0.1:${address.port}`, + }, + stdio: ["pipe", "pipe", "pipe"], + }); + + let stdout = ""; + let stderr = ""; + child.stdout.on("data", (chunk) => { + stdout += chunk.toString(); + }); + child.stderr.on("data", (chunk) => { + stderr += chunk.toString(); + }); + child.on("error", reject); + child.on("close", (exitCode) => { + resolve({ exitCode, stderr, stdout }); + }); + child.stdin.end(JSON.stringify(payload)); + }); + + return { ...result, received }; + } finally { + await new Promise<void>((resolve, reject) => { + server.close((err) => (err ? reject(err) : resolve())); + }); + } }🤖 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 `@test/post-tool-use-hook.test.ts` around lines 26 - 77, The test can leak the HTTP server when the spawn/parse Promise rejects; modify runHook so server.close(...) is always called by moving the server.close call into a finally block that runs after the spawn/parse Promise (the block that awaits the child process result and assigns result), ensuring the server is closed whether that Promise resolves or rejects; locate the runHook function and wrap the child spawn/await logic in try { ... } finally { server.close((err) => err ? reject(err) : null) } (or use await new Promise for close inside finally) so server.close is invoked on all failure paths.
🤖 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.
Nitpick comments:
In `@test/post-tool-use-hook.test.ts`:
- Around line 26-77: The test can leak the HTTP server when the spawn/parse
Promise rejects; modify runHook so server.close(...) is always called by moving
the server.close call into a finally block that runs after the spawn/parse
Promise (the block that awaits the child process result and assigns result),
ensuring the server is closed whether that Promise resolves or rejects; locate
the runHook function and wrap the child spawn/await logic in try { ... } finally
{ server.close((err) => err ? reject(err) : null) } (or use await new Promise
for close inside finally) so server.close is invoked on all failure paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a28d08de-aab0-41d5-8a4e-ca302d1ef760
📒 Files selected for processing (3)
plugin/scripts/post-tool-use.mjssrc/hooks/post-tool-use.tstest/post-tool-use-hook.test.ts
|
Fixed. Moved the test server cleanup into a finally block so the server is closed even if the child process or request |
1 participant
Summary
tool_responsefor Claude CodePostToolUsepayloadstool_outputfallback for existing hook payloadsCloses #539
Summary by CodeRabbit