Skip to content

fix(claude): keep a marker for tool_result document blocks - #939

Open
DevMello wants to merge 1 commit into
lidge-jun:devfrom
DevMello:fix/tool-result-document-blocks
Open

fix(claude): keep a marker for tool_result document blocks#939
DevMello wants to merge 1 commit into
lidge-jun:devfrom
DevMello:fix/tool-result-document-blocks

Conversation

@DevMello

@DevMello DevMello commented Aug 3, 2026

Copy link
Copy Markdown

Summary

When a tool result contains a document block, the inbound translation dropped it and routed providers received an empty tool output, so the model concluded the tool returned nothing. A document in a plain user message already gets a [document: title] marker; the tool-result path now emits the same marker.

Verification

  • New test in tests/claude-inbound.test.ts: titled, untitled, and mixed text plus document results.
  • Reproduced against a mock upstream: the tool message arrived as "" before the fix and [document] after.
  • bun run test, typecheck, lint:gui, privacy:scan.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Summary by CodeRabbit

  • Bug Fixes

    • Document attachments in tool results are now represented correctly instead of producing empty output.
    • Document titles are preserved when available; untitled documents use a clear placeholder.
  • Tests

    • Added coverage to verify document handling, text preservation, and parseable tool results.

A tool_result whose content is a document block translated to an
empty tool output, so routed models treated the attachment as a tool
that returned nothing. Surface the same "[document: title]" marker
the user-message path already emits.
@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 2ee7eda4-6f5e-45cb-b750-683196449cfa

📥 Commits

Reviewing files that changed from the base of the PR and between 6a7351b and 3cdf190.

📒 Files selected for processing (2)
  • src/claude/inbound.ts
  • tests/claude-inbound.test.ts

📝 Walkthrough

Walkthrough

Changes

Tool-result document handling

Layer / File(s) Summary
Document marker conversion and validation
src/claude/inbound.ts, tests/claude-inbound.test.ts
Tool-result document blocks now become [document: <title>] or [document] text markers. The regression test verifies text preservation, Responses input text conversion, and successful request parsing.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: ingwannu, lidge-jun, wibias

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: preserving markers for document blocks in Claude tool results.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@lidge-jun

Copy link
Copy Markdown
Owner

Carried onto the review stack as #953 (stack 3/3), unmodified.

Your commits were taken with git cherry-pick -x, so they keep your authorship — git log --format='%an' on the stack branch shows you, not me. No content was changed; the diff on the stack is byte-identical to what you wrote here, and it applied to dev with no conflict resolution.

Verified on the stack: bun x tsc --noEmit exit 0, and the full suite at 7691 pass / 8 skip / 0 fail across 507 files.

This PR stays open until #953 lands. If a maintainer prefers to take yours directly instead, that path is unaffected — the stack commits get dropped and this one merges. Once #953 merges I'll close this as carried, with the credit already in the commit history rather than in a comment.

Stack: #951 (plan, base dev) → #952 (#908 long-context pricing) → #953 (this carry). Review bottom-up.

Thanks for the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants