fix: reject tool_use blocks with empty id or function name - #1049
EleanorWho wants to merge 5 commits into
Conversation
8097e26 to
b657c66
Compare
|
Unsigned commits: b657c66. Please sign your commits. |
b657c66 to
4fb46c8
Compare
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review
Validates that upstream tool-call id and function name are non-empty before constructing Anthropic tool_use blocks, returning errors instead of silently producing schema-invalid blocks.
Assessment
The finite-response validation and the extracted extract_tool_id_and_name helper are correct. Error types align with their respective call sites (String for transform_response, FilterError for the streaming path). The four new tests cover missing and empty variants for both fields in the finite path.
| Severity | Count |
|---|---|
| Medium | 1 |
Non-inline findings
None — all findings are inline.
leseb
left a comment
There was a problem hiding this comment.
[P2] Missing streaming IDs bypass validation — mod.rs:651
An omitted/non-string id skips emit_tool_block_start, returns Ok, and can emit an orphan input_json_delta. A diagnostic test reproduced this. Validate unopened calls before emitting arguments.
[P2] Nonempty but schema-invalid IDs remain accepted — response.rs:188, mod.rs:731
Issue #941 and the checked-in Anthropic schema require IDs matching ^[a-zA-Z0-9_-]+$. Both paths only reject empty strings, so values such as call.bad still produce invalid tool_use blocks.
Thank you @leseb , these two are all addressed in the commit f567517. The coverage-check error is probably flaky. It's unrelated to my changes, and the coverage-check passes locally. Can you trigger a re-run to see? |
praxis-bot
left a comment
There was a problem hiding this comment.
PR Re-Review
The new commit addresses the previous finding by adding streaming-path tests (INVALID_TOOL_CALL_CASES with five cases including invalid-format id). The is_valid_tool_use_id helper and the streaming condition simplification look correct.
| Severity | Count |
|---|---|
| Medium | 1 |
Non-inline findings
None — all findings are inline.
leseb
left a comment
There was a problem hiding this comment.
Remaining blockers:
- Add the missing finite invalid-format ID regression test.
- Add required synthetic fixture, manifest/generated README, and functional/client coverage for this transformation change.
- Rerun CI; current coverage failure is an unrelated AddrInUse flake.
…oxy#941) The Anthropic translation silently substituted empty strings when upstream tool calls omitted their id or function name, producing schema-invalid tool_use blocks. Validate both fields are non-empty before constructing a block, returning a transformation error (finite) or FilterError (streaming) instead. Signed-off-by: Eleanor Hu <ehu@redhat.com>
…ass validation and Anthropic schema required ID matching. Signed-off-by: Eleanor Hu <ehu@redhat.com>
… for invalide-tool-id. Signed-off-by: Eleanor Hu <ehu@redhat.com>
f567517 to
f296e20
Compare
|
@EleanorWho final: [P2] The new fixture only tests stream: false (invalid-tool-id.yaml:22). The PR changes streaming rejection too, but that path lacks end-to-end and official Anthropic SDK coverage. Thanks |
@leseb A low-level client may therefore observe 200 text/event-stream headers with no body, but this is not a normally completed empty SSE response. The reqwest-based inference fixture runner instead reports a body read error ( This means that adding a fixture expecting a completed “200 + empty SSE body” would require either changing the production filter behavior or teaching the fixture runner to model an aborted stream. Could you confirm which client-visible behavior the SDK coverage should assert? |
The Anthropic translation silently substituted empty strings when upstream tool calls omitted their id or function name, producing schema-invalid tool_use blocks. Validate both fields are non-empty before constructing a block, returning a transformation error (finite) or FilterError (streaming) instead.
Summary
idor functionnameinstead of silently substituting empty strings, which produced schema-invalid Anthropictool_useblocksFilterErroridandnamein both casesRelated issue
Closes #941
Validation
id, emptyid, missingname, emptynamemake testandmake lintpass