test(claude): add compatibility sentinels and centralize builtin fallback handling#2491
Conversation
This change stops short of broader Claude Code runtime alignment and instead hardens two safe edges: builtin tool prefix handling and source-informed sentinel coverage for future drift checks. Constraint: Must preserve existing default behavior for current users Rejected: Implement control-plane/session alignment now | too much runtime risk for a first slice Confidence: high Scope-risk: narrow Reversibility: clean Directive: Treat the new fixtures as compatibility sentinels, not a full Claude Code schema contract Tested: go test ./test/...; go test ./sdk/translator/...; go test ./internal/runtime/executor -run 'Claude|Builtin|Tool'; go test ./... Not-tested: End-to-end Claude Code direct-connect/session runtime behavior
There was a problem hiding this comment.
Code Review
This pull request introduces a centralized registry for Claude's built-in tools and adds compatibility tests for Claude 'sentinel' message shapes. The tool prefixing logic in claude_executor.go has been refactored to use a dynamic registry seeded with default tools and augmented by the request body. Additionally, new test fixtures and shape validation tests were added for various Claude-specific message types, including tool progress, session state changes, and tool use summaries. I have no feedback to provide.
|
Hi @mpfo0106 — could you please move the following files into
Thanks! |
luispater
left a comment
There was a problem hiding this comment.
Please move the following helper files into internal/runtime/executor/helps/ (the helps package) so helper utilities stay centralized:
internal/runtime/executor/claude_builtin_tools.go→internal/runtime/executor/helps/claude_builtin_tools.gointernal/runtime/executor/claude_builtin_tools_test.go→internal/runtime/executor/helps/claude_builtin_tools_test.go
git mv is preferred to preserve history.
The review asked for the builtin tool registry helper to live with the rest of executor support utilities. This moves the registry code into the helps package, exports the minimal surface executor needs, and keeps behavior tests with the executor while leaving registry-focused checks with the helper. Constraint: Requested layout keeps executor helper utilities centralized under internal/runtime/executor/helps Rejected: Keep the files in executor and reply with rationale | conflicts with requested package organization Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep executor behavior tests near applyClaudeToolPrefix and keep pure registry tests in helps Tested: go test ./internal/runtime/executor/helps ./internal/runtime/executor -run 'Claude|Builtin|Tool'; go test ./test/...; go test ./... Not-tested: End-to-end Claude Code direct-connect/session runtime behavior
luispater
left a comment
There was a problem hiding this comment.
The requested layout change is complete: the Claude builtin helper now lives under internal/runtime/executor/helps/, and applyClaudeToolPrefix now uses the exported helper from that package.
I verified the updated executor/helper tests and the new Claude Code sentinel tests on the PR head, and they pass. No blocking issues remain.
This is an automated Codex review result and still requires manual verification by a human reviewer.
Summary
applyClaudeToolPrefixtool_choice,tool_use,tool_reference, and nestedtool_resultpathsMotivation
This is a safe first step toward keeping CLIProxyAPI grounded against the now-public Claude Code source without introducing broader runtime risk.
The scope is intentionally narrow:
This is compatibility scaffolding, not a broader Claude Code control-plane/session alignment change.
Why this is low-risk
go test ./...passWhy this is useful now that Claude Code source is public
Notes
web_search,code_execution,text_editor,computerTests
go test ./test/...go test ./sdk/translator/...go test ./internal/runtime/executor -run 'Claude|Builtin|Tool'go test ./...