Repository navigation
Conversation
Signed-off-by: Arnaud Héritier <aheritier@users.noreply.github.com>
Signed-off-by: Arnaud Héritier <aheritier@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
JSON-based deep copies can silently round large integer values in provider options and structured-output schemas.
2 open findings
What changed in this PR
Adds checked provider cloning, preserves Vertex transport/authentication, and propagates clone failures safely through runtime callers.
Changes:
- Introduces
provider.Clonewith explicit errors and isolated clone configuration. - Migrates title generation, sampling, and compaction to checked cloning.
- Preserves Vertex dispatch/authentication and adds regression coverage and documentation.
| File | Description |
|---|---|
docs/guides/go-sdk/index.md |
Documents cloning contracts and failures. |
docs/providers/google/index.md |
Documents Vertex clone transport guarantees. |
pkg/acp/agent_test.go |
Makes ACP test provider clonable. |
pkg/model/provider/anthropic/vertex/vertex.go |
Preserves wrapped transports with OAuth. |
pkg/model/provider/clone.go |
Implements checked cloning and configuration copying. |
pkg/model/provider/clone_checked_test.go |
Tests checked clone behavior. |
pkg/model/provider/clone_matrix_test.go |
Tests clone option/configuration combinations. |
pkg/model/provider/clone_merge_test.go |
Tests cumulative option merging. |
pkg/model/provider/clone_routed_test.go |
Tests routed configuration isolation. |
pkg/model/provider/clone_test.go |
Updates existing clone tests. |
pkg/model/provider/clone_thinking_test.go |
Tests thinking-off preservation. |
pkg/model/provider/clone_vertex_test.go |
Tests Vertex dispatch and authentication. |
pkg/model/provider/contracts/contracts.go |
Adds dispatch configuration identity. |
pkg/model/provider/factory.go |
Preserves routing state and thinking intent. |
pkg/model/provider/options/options.go |
Adds explicit clears and option cloning. |
pkg/model/provider/options/options_test.go |
Tests option clone preservation. |
pkg/model/provider/rulebased/client.go |
Retains router model options. |
pkg/model/provider/vertexai/modelgarden.go |
Preserves Model Garden dispatch configuration. |
pkg/model/provider/vertexai/modelgarden_test.go |
Tests single option application. |
pkg/runtime/clone_failure_test.go |
Tests caller-visible clone failures. |
pkg/runtime/compaction_context_limit_test.go |
Makes context-limit fixtures clonable. |
pkg/runtime/compactor/compactor.go |
Propagates summary clone failures. |
pkg/runtime/compactor/compactor_test.go |
Tests history preservation on failure. |
pkg/runtime/fallback_test.go |
Makes fallback fixtures clonable. |
pkg/runtime/live_sessions_test.go |
Makes live-session fixtures clonable. |
pkg/runtime/model_context_test.go |
Adds context-provider rebuilding. |
pkg/runtime/runtime_test.go |
Updates runtime provider fixtures. |
pkg/runtime/sampling.go |
Returns sampling clone errors. |
pkg/runtime/session_compaction.go |
Handles context clone failures safely. |
pkg/runtime/session_compaction_test.go |
Covers hook summaries with clone failure. |
pkg/runtime/title_generator_test.go |
Makes title fixtures clonable. |
pkg/runtime/transforms_test.go |
Makes transform fixtures clonable. |
pkg/server/session_manager_test.go |
Makes delayed title provider clonable. |
pkg/sessiontitle/generator.go |
Uses checked cloning for titles. |
pkg/sessiontitle/generator_test.go |
Tests title clone options and failures. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Signed-off-by: Arnaud Héritier <aheritier@users.noreply.github.com>
aheritier
left a comment
There was a problem hiding this comment.
Reviewed quality, performance, security, relevance to #4582, and opportunities for simplification. One blocking acceptance mismatch remains: explicit token caps are silently ignored by routed clones. See inline comment.
Required CI passed for the reviewed head. Exact-head validation relies on fetched source/diff and CI; the local checkout differs, and no live inference was performed. The PR is conflict-free but not merge-ready: GitHub reports REVIEW_REQUIRED and a blocked merge state.
| require.NoError(t, json.Unmarshal(body(), &payload)) | ||
| assert.Equal(t, "none", payload["reasoning_effort"]) | ||
| assert.NotContains(t, payload, "response_format") | ||
| if shape != "routed" { |
There was a problem hiding this comment.
[P2 · blocking] Honor explicit token caps in routed clones
This exemption skips a routed-option requirement in #4582. Clone(..., WithMaxTokens(128)) succeeds, but rulebased.NewClient passes its options through filterOutMaxTokens before constructing both fallback and routed children. Updating the router's own ModelConfig.MaxTokens therefore never constrains the provider making the request. The new TestCloneRoutedClaudeStrictOffOnWire demonstrates the consequence: after requesting 128 tokens, the cloned Anthropic request still uses 8192 and Bedrock still uses 16384.
Thus an MCP sampling maxTokens request or title-generation cap is silently ignored when the active provider is a router. Preserving ordinary per-child token defaults is reasonable, but it needs to be distinguished from an explicit auxiliary clone cap. Please propagate that cap to the children, subject to existing provider-specific floors, and replace this exemption with wire assertions covering both a matched route and fallback, including cumulative clones.
There was a problem hiding this comment.
🤖 Automated implementer agent — this comment was posted by the implementer bot from Docker Agentic Platform, not by a human developer
Fixed in ac3407391a4ca953c29202e98e4263886f6c9fda (including 1a4cd20391e8f55db43a9d5a27ad8a2710e7287d). Explicit Clone(..., WithMaxTokens(n)) caps now reach matched routes and fallback children, while ordinary construction-time router limits still preserve each child's defaults. Existing provider floors remain unchanged: a strict-off 128 cap emits 256 for Anthropic/OpenAI and 128 for Bedrock.
Offline wire regressions cover matched and fallback requests, cumulative/omitted/zero caps, last non-zero overrides (including trailing zero in the same clone call), unchanged ordinary defaults, and actual title generation plus both MCP sampling handlers. The previous production overlay fails the new assertions; corrected code passes, including 50 repetitions. Strict-off, checked-copy and native Vertex regressions remain passing.
At this exact head, author and fresh independent reviewer passed forced build/test/lint, uncached full native tests, targeted provider/runtime/title race+shuffle, and the canonical WASM-provider/dependency checks. Independent verdict: approve, no findings. Local, remote branch and PR head were verified equal. The PR remains draft pending CI; no merge or ready flip.
Signed-off-by: Arnaud Héritier <aheritier@users.noreply.github.com>
Signed-off-by: Arnaud Héritier <aheritier@users.noreply.github.com>

Closes #4582. Part of #4580; independently shippable on existing models.
Changes
provider.Clone(ctx, p, opts...) (Provider, error)with actionable reconstruction/configuration errors. Keep the publicCloneWithOptionssignature, deprecate it, and return an error-only provider on failure rather than the original provider.streamRawPredict, Vertex dialect and OAuth. Both ADC and request-context token sources retain the wrapped transport without leaking a direct API key; OpenAI-compatible Model Garden dispatch remains compatible.none/0before normalization and apply strict-off to config-driven as well as option-driven leaves.Regression evidence
Offline fake providers/HTTP transports cover actual caller failures with zero-base-inference counters, session-history preservation, both Vertex auth paths, contextual sibling-auth containment, hidden config fields, option precedence, capability/restricted-registry parity, and legacy budgets/token floors.
The new routed Claude/off-intent/isolation tests were also run with an overlay restoring the previous factory from
afaa5819abde6a134874efab4ae87fec9477a0d6: the routed Anthropic/Bedrock strict-off, routed configuration-isolation and configured thinking-off regression groups fail against that factory and pass with this change. This negative control uses no live inference.Validation
Validated at
979a07fc9041ba2da36b7300ecb542b1195cf06f:Pinned local Task v3.53.1 / golangci-lint v2.14.0; Go 1.27.2. SDK versions and
go.mod/go.sumunchanged. Initial environment/test/lint failures and the negative control are retained in the validation artifacts, not represented as passing runs.Boundaries
No new Claude 5.x policy activation, SDK/dependency upgrade, configuration/schema change or frozen config modification. Legacy budgets, the disabled sentinel, public compatibility, provider token floors and routed-child token-limit policy are preserved. #4553 is held and unchanged; no duplication or intervention in external #4576/#4579. This PR does not claim the 5.x core or per-stream pricing contract is implemented.
Independent pre-push review
Fresh reviewer
a6deafc1-5cc1-4eb1-9902-71218dfafc41approved exact head979a07fc9041ba2da36b7300ecb542b1195cf06f, with F1/F2/F3 accepted fixed and no remaining findings. The firstrequest_changesand all corrections are retained in the author review ledger/evidence. Reviewer independently forced build/lint and ran uncached full native tests plus fresh targeted race/shuffle successfully. An additional full-runtime WASM attempt is not claimed passing: existing modernc SQLite/libc/errno test dependencies exclude js; supported provider/options/compactor/title packages passed. The canonicaltest-wasm-providersgate passed separately.Numeric-fidelity review correction
Review 5506963708 was COMMENTED / Changes recommended, not an approval. Both numeric-fidelity requests were confirmed. Checked copies now preserve exact scalar types and numeric values instead of decoding interface-backed numbers as
float64. The private helper isolates mutable maps/slices/pointers/arrays/plain structs; unsupported values, cycles, hidden reference-bearing fields and bounded-copy limits return path-named errors. Shared transport/token/store services remain shared. SDK documentation describes the copy contract, including non-finite scalar preservation.Regressions cover large
int64,MaxUint64, narrow/named scalar types, YAML decoding, cumulative and routed clones, structured-output/custom-provider options, mutable isolation and fail-closed paths. Offline wire tests require9007199254740993exactly in both an OpenAI extra-body field and a schema bound; restoring the old copy functions makes those tests fail with9007199254740992. Previous F1/F2/F3 negative controls remain discriminating.Vertex scope/provenance: the real base-and-clone transport tests exposed the same rounding in
SetDispatchConfig(introduced in this PR) and two preexisting Model Garden copy boundaries (catalogue rewrite and OpenAI key stripping). These provider-local copies now use the same checked mechanism, without changinglatest.ModelConfig.Clone, config parsing, schemas, dependencies or SDK versions. Native ADC/token-source and OpenAI-compatible base/clone tests retain numeric values and auth/dispatch behavior. The preexisting shared JSON-clone use inpkg/runtime/model_switcher.goremains outside this correction.The PR stays draft until the corrected head's CI passes. Earlier independent approval applied only to the previous head. Fresh reviewer
7089c7ee-f96b-4d64-91cd-7a1fadec1e3capproved exact head755e1a4c2ba4d56de0a636f6a666ff2a6c33f8afwith no findings before push, independently rerunning forced canonical build/test/lint, uncached full native tests, targeted provider race/shuffle, canonical WASM-provider validation and additional helper/options WASM tests. The author additionally passed targeted runtime/title race coverage. Local, remote branch and PR head were verified equal after push. Full runtime WASM remains unsupported by existing SQLite/libc/errno dependencies and is not claimed passing. Signed/DCO commits are present; local/forge signature trust verification is not claimed.