Skip to content

fix(provider): preserve transport and surface clone failures - #4587

Open
aheritier wants to merge 5 commits into
mainfrom
fix/4582-checked-provider-clones
Open

aheritier wants to merge 5 commits into
mainfrom
fix/4582-checked-provider-clones

Conversation

@aheritier

@aheritier aheritier commented Oct 13, 2026 •

Copy link
Copy Markdown
Collaborator

🤖 Automated implementer agent — this comment was posted by the implementer bot from Docker Agentic Platform, not by a human developer

Closes #4582. Part of #4580; independently shippable on existing models.

Changes

  • Add checked provider.Clone(ctx, p, opts...) (Provider, error) with actionable reconstruction/configuration errors. Keep the public CloneWithOptions signature, deprecate it, and return an error-only provider on failure rather than the original provider.
  • Migrate title generation, both MCP sampling handlers, LLM compaction and context-window resolution. Failed clones perform no base inference; title candidates exhaust nonfatally, sampling reports the error, and compaction keeps session history intact. Existing gauge signatures, hook-supplied summaries and native compaction remain supported.
  • Preserve original Google Model Garden dispatch independently of catalogue/display identity. Native Anthropic Vertex clones retain project/location/host, model-in-URL 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.
  • Preserve ordered/cumulative options, structured-output preservation/removal, gateway clearing and explicit thinking-off intent through literal, environment-expanded, inherited/custom and routed configurations. Capture configured none/0 before normalization and apply strict-off to config-driven as well as option-driven leaves.
  • Isolate mutable configuration, schemas, custom-provider definitions and named routed children. Capture only referenced routing definitions, so caller-map mutations do not alter future clones and unrelated uncopyable definitions do not reject a router. Referenced unsupported values fail closed, without panic/reference fallback. Routed children retain their existing token limits.
  • Document the checked API, per-caller failure behavior, serialization contract and native Vertex transport/auth guarantees.

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:

CGO_ENABLED=1 PATH=/tmp/p2-bin:$PATH task build
CGO_ENABLED=1 PATH=/tmp/p2-bin:$PATH task test
CGO_ENABLED=1 PATH=/tmp/p2-bin:$PATH task --force lint
CGO_ENABLED=1 go test -count=1 -race -shuffle=on ./pkg/model/provider/... ./pkg/runtime/... ./pkg/sessiontitle/...
CGO_ENABLED=0 PATH=/tmp/p2-bin:$PATH task test-wasm-providers
# Includes: go test ./e2e -run '^TestWasmProviderDependencies$'
git diff --check

Pinned local Task v3.53.1 / golangci-lint v2.14.0; Go 1.27.2. SDK versions and go.mod/go.sum unchanged. 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-71218dfafc41 approved exact head 979a07fc9041ba2da36b7300ecb542b1195cf06f, with F1/F2/F3 accepted fixed and no remaining findings. The first request_changes and 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 canonical test-wasm-providers gate 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 require 9007199254740993 exactly in both an OpenAI extra-body field and a schema bound; restoring the old copy functions makes those tests fail with 9007199254740992. 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 changing latest.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 in pkg/runtime/model_switcher.go remains 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-7a1fadec1e3c approved exact head 755e1a4c2ba4d56de0a636f6a666ff2a6c33f8af with 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.

Signed-off-by: Arnaud Héritier <aheritier@users.noreply.github.com>
Signed-off-by: Arnaud Héritier <aheritier@users.noreply.github.com>
@aheritier aheritier added area/docs Documentation changes area/providers For features/issues/fixes related to LLM providers (Bedrock, LiteLLM, Qwen, custom, etc.) area/runtime Runtime engine, agent loop execution, tool dispatch, loop detection area/sessions For features/issues/fixes related to session lifecycle (resume, persistence, export) kind/fix PR fixes a bug (maps to fix:). Use on PRs only. labels Oct 13, 2026
@aheritier
aheritier marked this pull request as ready for review October 13, 2026 16:39
@aheritier
aheritier requested a review from a team as a code owner October 13, 2026 16:39
Copilot AI balanced review requested due to automatic review settings October 13, 2026 16:39

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.Clone with 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.

Comment thread pkg/model/provider/clone.go Outdated
Comment thread pkg/model/provider/options/options.go Outdated
@aheritier
aheritier marked this pull request as draft October 13, 2026 16:51
Signed-off-by: Arnaud Héritier <aheritier@users.noreply.github.com>
@aheritier
aheritier marked this pull request as ready for review October 13, 2026 17:26

@aheritier aheritier left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread pkg/model/provider/clone_matrix_test.go Outdated
require.NoError(t, json.Unmarshal(body(), &payload))
assert.Equal(t, "none", payload["reasoning_effort"])
assert.NotContains(t, payload, "response_format")
if shape != "routed" {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.

@aheritier
aheritier marked this pull request as draft October 14, 2026 09:05
Signed-off-by: Arnaud Héritier <aheritier@users.noreply.github.com>
Signed-off-by: Arnaud Héritier <aheritier@users.noreply.github.com>
@aheritier
aheritier marked this pull request as ready for review October 14, 2026 09:54

This branch has not been deployed

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

Labels

area/docs Documentation changes area/providers For features/issues/fixes related to LLM providers (Bedrock, LiteLLM, Qwen, custom, etc.) area/runtime Runtime engine, agent loop execution, tool dispatch, loop detection area/sessions For features/issues/fixes related to session lifecycle (resume, persistence, export) kind/fix PR fixes a bug (maps to fix:). Use on PRs only.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(provider): preserve transport and surface clone failures [P2; independent]

2 participants