fix(compat): withHooks must decorate toModelOutput — hook feedback and block reasons are invisible to the model (#180)#202
Conversation
…ock reasons reach the model (#180) withHooks decorated only execute. Every #127 tool renders from its own typed output shape, so hook artifacts were dropped at the model-visible layer: - PostToolUse feedback (applyFeedback's additive hookFeedback field on object results) was invisible — renderBashSection etc. render only their own fields. - A PreToolUse block on a non-bash tool returns the generic HookBlockedResult, and the tool's own renderer (e.g. readFile's output.content) then rendered undefined — the model saw nothing actionable, couldn't follow adjust-don't-retry. wrapTool now also decorates toModelOutput (same pattern as withReminders, async): a blocked call renders the block reason directly; feedback attached to an object result is appended in a <hook-feedback> section after the base render. String results already carry feedback inline, so no double-append. Typed execute outputs are unchanged (programmatic callers observe them bit-identical). Bash blocks keep rendering via their own stderr+exit shape (never a HookBlockedResult). Tests: blocked custom-renderer tool renders the reason not undefined; object-result feedback is model-visible after the base; execute output unchanged; composes with withReminders in both wrap orders. Part of #196.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesHook model-output decoration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/ai-claude-compat/src/tool-hooks.ts (1)
211-224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
baseModelOutputduplicatessystem-reminder.ts's helper of the same name/purpose.This file's
baseModelOutput(base, ctx)andsystem-reminder.ts'sbaseModelOutput(tool, ctx)both compute "what the SDK would render by default", with slightly different signatures. Any future change to the SDK-default fallback logic (e.g. the fix above) needs to land in both places to stay consistent. Consider extracting one shared helper (e.g. taking the tool/toModelOutput fn) that bothwithHooksandwithReminderscall.🤖 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 `@packages/ai-claude-compat/src/tool-hooks.ts` around lines 211 - 224, Extract the duplicated SDK-default rendering logic from baseModelOutput in tool-hooks.ts and the corresponding helper in system-reminder.ts into one shared helper that accepts the tool’s toModelOutput function and output context. Update withHooks and withReminders to use the shared helper while preserving the existing string-to-text, non-string-to-json, and async behavior.
🤖 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 `@packages/ai-claude-compat/src/tool-hooks.ts`:
- Around line 213-224: Update baseModelOutput and the default path used by
decoratedToModelOutput so object outputs have their hookFeedback field removed
before being rendered as the base JSON value, while preserving separate
extraction and appending through objectHookFeedback and appendHookFeedback. Keep
string outputs and custom toModelOutput handlers unchanged, and add coverage for
tools using the default renderer to ensure feedback appears only in the
hook-feedback section.
---
Nitpick comments:
In `@packages/ai-claude-compat/src/tool-hooks.ts`:
- Around line 211-224: Extract the duplicated SDK-default rendering logic from
baseModelOutput in tool-hooks.ts and the corresponding helper in
system-reminder.ts into one shared helper that accepts the tool’s toModelOutput
function and output context. Update withHooks and withReminders to use the
shared helper while preserving the existing string-to-text, non-string-to-json,
and async behavior.
🪄 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.yml
Review profile: CHILL
Plan: Pro
Run ID: c23a8110-b56d-4a73-b3c9-89297b48d6ed
📒 Files selected for processing (2)
packages/ai-claude-compat/src/tool-hooks.test.tspackages/ai-claude-compat/src/tool-hooks.ts
…n't shown twice (#180 CR) CR (Major): a tool with NO custom toModelOutput + an object result showed PostToolUse feedback twice — applyFeedback embeds hookFeedback in the object, the default json base render echoed it, and decoratedToModelOutput appended it again. #127 tools (custom text renderers that never read hookFeedback) were unaffected, but plain object tools (e.g. an MCP tool with no renderer) double-rendered. baseModelOutput now strips hookFeedback before the default json render, so the feedback appears only in the appended <hook-feedback> section. Real tool output is preserved. Regression test with a default-renderer tool asserts the feedback marker appears exactly once.
OGtwelve
force-pushed
the
fix/hooks-tomodeloutput-180
branch
from
f61bf1d to
a74f10d
Compare
1 participant
Closes #180. Last of the XS/S P1 batch from the parity verification (#196). A #121×#127 interaction bug — each is correct alone.
Problem
withHooksdecorated onlyexecute. Every #127 tool renders from its own typed output shape, so hook artifacts were dropped at the model-visible layer:applyFeedbackattacheshookFeedback(an additive field on object results), but e.g.renderBashSectionrenders only stdout/stderr/exit — the feedback never reached the model.HookBlockedResult, but the tool's renderer then reads its own fields — readFile's({output}) => ({type:'text', value: output.content})renderedundefined. The model saw nothing actionable, couldn't follow adjust-don't-retry.Fix
wrapToolnow also decoratestoModelOutput(same async-aware pattern aswithReminders): a blocked call renders the block reason directly; feedback attached to an object result is appended in a<hook-feedback>section after the base render. String results already carry feedback inline, so no double-append. Typed execute outputs are unchanged (programmatic callers observe them bit-identical). Bash blocks keep rendering via their own stderr+exit shape.Tests
undefined; object-result feedback is model-visible after the base output; execute output unchanged; composes withwithRemindersin both wrap orders.test:node(compat 262) + biome.Part of #196.
Summary by CodeRabbit