fix(openai-chat): log redacted invalid tool-call shape - #1530
Conversation
|
✅ Deterministic PR hygiene checks passed. |
📝 WalkthroughWalkthroughThe OpenAI adapter now classifies malformed streamed and buffered tool-call payloads. It logs shape-only diagnostics before returning the existing invalid-tool-calls error. Tests cover logging, redaction, disabled defaults, validation precedence, and streaming metadata. ChangesTool-call diagnostics
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
…idation Rebase follow-up on top of @Ingwannu's diagnostic commit. #1531 landed between this PR being written and being rebased, and it moved the validation the diagnostic describes. The streamed path now validates function/name/arguments/id at ingest rather than only checking container and member shapes, so the diagnostic no longer skips per-member inspection in stream mode, and the three new ingest rejections log through the same helper. Blank and whitespace-only names are now rejected on the buffered path; reporting that as name_invalid would claim a type problem for a correctly-typed value, so it gets its own reason code. A diagnostic that describes a boundary the code no longer has is worse than none: it would send provider-compatibility work after the wrong shape.
Review follow-up. The buffered validator checks the function container first, then id/name/arguments types together, and only then the blank name. The diagnostic checked id before the container and blank name before arguments, so a payload carrying two defects at once was reported under the wrong reason. That matters precisely because of what this diagnostic is for: it exists to point provider-compatibility work at the shape that was rejected. A wrong reason code sends that work after the wrong shape. Four tests each carry two defects at once, so only the matching order produces the expected reason.
fdb26c6 to
be101c3
Compare
|
Rebased onto current Why it conflicted. #1531 merged between this being written and being rebased, and it changed the exact validation this diagnostic describes. Two consequences: The streamed path now validates The buffered path now rejects blank and whitespace-only names. The diagnostic would have reported that as A second review round then caught a precedence bug in my own follow-up. The validator checks the Privacy was re-verified independently: reason codes and modes are finite literals, On whether this is still worth landing now that #1531 fixed #1514: yes. #1531 fixed one known shape; this records which check rejected a payload, which is what makes the next provider incompatibility diagnosable instead of a guess. It stays opt-in and off by default. Verification at |
![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
🤖 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 `@src/adapters/openai-chat.ts`:
- Around line 295-298: Update logInvalidToolCalls to return immediately when
isDebugEnabled() is false, before calling diagnoseInvalidToolCalls; retain the
existing diagnostic classification and debugProviderDiagnostic behavior when
debugging is enabled.
🪄 Autofix
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.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 83f1de84-cbf2-46c8-8095-70c0202ce456
📒 Files selected for processing (2)
src/adapters/openai-chat.tstests/openai-chat-hardening.test.ts
| function logInvalidToolCalls(mode: "stream" | "response", rawToolCalls: unknown): void { | ||
| const diagnostic = diagnoseInvalidToolCalls(rawToolCalls, mode); | ||
| if (diagnostic) debugProviderDiagnostic("openai-chat", "invalid-tool-calls", { mode, ...diagnostic }); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Gate classification when debug diagnostics are disabled.
Line 296 calls diagnoseInvalidToolCalls before debugProviderDiagnostic checks the debug setting. A malformed array is already scanned by the validator. This adds a second scan even when diagnostics are disabled. Return before classification when isDebugEnabled() is false.
Proposed fix
function logInvalidToolCalls(mode: "stream" | "response", rawToolCalls: unknown): void {
+ if (!isDebugEnabled()) return;
const diagnostic = diagnoseInvalidToolCalls(rawToolCalls, mode);
if (diagnostic) debugProviderDiagnostic("openai-chat", "invalid-tool-calls", { mode, ...diagnostic });
}📝 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.
| function logInvalidToolCalls(mode: "stream" | "response", rawToolCalls: unknown): void { | |
| const diagnostic = diagnoseInvalidToolCalls(rawToolCalls, mode); | |
| if (diagnostic) debugProviderDiagnostic("openai-chat", "invalid-tool-calls", { mode, ...diagnostic }); | |
| } | |
| function logInvalidToolCalls(mode: "stream" | "response", rawToolCalls: unknown): void { | |
| if (!isDebugEnabled()) return; | |
| const diagnostic = diagnoseInvalidToolCalls(rawToolCalls, mode); | |
| if (diagnostic) debugProviderDiagnostic("openai-chat", "invalid-tool-calls", { mode, ...diagnostic }); | |
| } |
🤖 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 `@src/adapters/openai-chat.ts` around lines 295 - 298, Update
logInvalidToolCalls to return immediately when isDebugEnabled() is false, before
calling diagnoseInvalidToolCalls; retain the existing diagnostic classification
and debugProviderDiagnostic behavior when debugging is enabled.
|
Full-suite result at All four Linux CI shards are green here as well. |
2 participants
Summary
tool_callsvalidation boundaryCloses no issue; supports diagnosis of #1483.
Verification
bun scripts/test.ts tests/openai-chat-hardening.test.ts tests/issue-452-empty-503.test.ts tests/debug.test.ts(66 passed, 0 failed)bun run typecheckbun run privacy:scangit diff --checkTo capture one redacted reproduction after this lands:
ocx debug provider on ocx debug provider logs -f # reproduce once ocx debug provider offExpected diagnostic shape:
Checklist
Summary by CodeRabbit