Skip to content

fix(route): honor custom table wire on runtime routes - #79

Open
asto18089 wants to merge 14 commits into
pinvou3-cleanfrom
fix/custom-wire-runtime-route
Open

asto18089 wants to merge 14 commits into
pinvou3-cleanfrom
fix/custom-wire-runtime-route

Conversation

@asto18089

@asto18089 asto18089 commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

A named [providers.<name>] table with wire = "responses" never reached the wire that actually serves a turn. RouteRequest had no wire channel, so candidate.protocol() came from Custom's backward-compatible static policy (WirePolicy::Fixed(ChatCompletions)), and every per-turn client built by DeepSeekClient::from_candidate bound Chat — while the ambient spawn-time client (DeepSeekClient::new) and provider_capability_with_wire honored the override. Under the Hmbown#3384 host-resolved-route architecture (every Op::SendMessage carries a runtime-resolved route and install_resolved_runtime_route rebuilds the client via from_candidate), a config that said Responses talked to {base}/chat/completions in practice.

Changes:

  1. RouteRequest.wire_override (config crate) — honored only for ProviderKind::Custom; built-ins keep their descriptor policy even if a stray override is set. The minted candidate carries the wire-true protocol and endpoint key (responses / messages / chat), so receipts, preflight, and the per-turn client binding all read the same fact.
  2. tui route layer — custom_wire_override_for(config) reads the active table's dialect (the same provider_wire_dialect source provider_wire_format_for_config uses) and threads it through resolve_runtime_route*, taken from the identity-scoped config so a pinned turn path (per-thread routing, reload_config, fleet pins, session restore) reads the pinned table's wire, never the ambient selection's. Client-internal re-resolutions (rebound_for_model_protocol, per-request routing, limit lookups) pin the transport's own wire so an engine-internal model switch cannot downgrade an endpoint-scoped client.
  3. Encrypted-reasoning capture — widened from Codex-only to every Responses route that sends include: ["reasoning.encrypted_content"]: the Codex OAuth backend, OpenCode Zen's Responses roster, and wire = "responses" Custom tables (DeepSeek's plain reasoning_text and Concentrate stay excluded). Captured state is tagged endpoint-scoped (custom/<table> for Custom, from the client's frozen provider identity), and the replay gate requires that exact tag plus api shape plus model, so table A's encrypted reasoning never replays onto table B. Without capture, a Custom/Zen Responses route streamed reasoning items that were never persisted, and multi-turn tool continuations replayed function_call items without their paired reasoning items.

Consumer census: the wire dialect parse lives once, in the config crate (wire_dialect_override next to the ProviderConfigToml::wire field it reads). provider_wire_format_for_config (ambient client), provider_capability_with_wire, the resolver, and the app-server /v1/chat/completions pass-through all consume it, so include/capture/resolution agree by construction; a wire = "responses" table reaching the pass-through fails closed at its ChatCompletions-only guard instead of silently receiving a chat body. The config-free resolve_route_candidate* wrappers keep the static policy (validation/display only; no client construction).

Tests

  • custom_wire_override_mints_a_wire_true_candidate / wire_override_is_ignored_for_builtin_kinds (config): override → Responses/Messages candidates + endpoint keys (including the explicit chat arm); default stays Chat; built-ins immune.
  • forkguard_named_table_wire_{responses,anthropic,without_wire}* (tui route_runtime): named-table dialect → runtime candidate protocol — the exact regression Pinvou PR feat(permissions): external_directory gate + broader-pattern always-allow (#411 #412) Hmbown/Codewhale#625 hit (three review rounds missed it because nothing pinned the wire on the runtime path).
  • forkguard_identity_pinned_route_reads_the_pinned_tables_wire / forkguard_ambient_wire_override_does_not_leak_onto_pinned_chat_tables (tui route_runtime): the two cross-table directions on resolve_runtime_route_for_identity — the pinned table's wire+endpoint must agree even when the ambient selection is a different table.
  • custom_table_wire_override_reaches_the_app_route (app-server): the pass-through resolves the table's Responses wire (the handler guard then rejects, fail closed).
  • forkguard_custom_responses_route_turn_client_posts_to_the_responses_endpoint (client): full turn-path construction (resolve_runtime_route → from_candidate) against a loopback wiremock asserting the POST path is /v1/responses with store:false, the include, and the verbatim model — the path-level pin.
  • forkguard_custom_responses_stream_captures_encrypted_reasoning_as_opaque_state / forkguard_custom_responses_replays_only_exact_model_opaque_reasoning_state: capture yields a custom/<table>-tagged opaque state with the wire model; replay includes it on exact table+model match and drops it on table switch or model switch.
  • opencode_zen_responses_stream_captures_encrypted_reasoning (client): the Zen roster captures encrypted reasoning the same way, tagged opencode-zen.
  • forkguard_custom_chat_stream_does_not_capture_encrypted_reasoning (client): a Chat-wire Custom table never mints opaque state even when a Responses-shaped stream reaches the handler — the capture gate keys on the transport's wire, not the event shape (mutation-checked: deleting the wire half fails exactly this pin).
  • forkguard_custom_responses_capture_tolerates_missing_or_empty_encrypted_content (client): missing or empty encrypted_content is not captured and the stream keeps flowing (mutation-checked against the empty-content filter).
  • forkguard_named_table_unrecognized_wire_keeps_the_chat_default (tui route_runtime): a typo'd dialect (wire = "respones") degrades to the legacy Chat policy at both the override reader and the runtime candidate.
  • route_is_valid_for_model (preflight) validates the table's own dialect (provider_readiness), so receipts/preflight see the same protocol candidate the turn path mints.

Review history

Round 1 review (2026-09-29, first comment) raised 1 blocker + 4 should-fixes; all are addressed by 747329d5 + 2871dfff (identity-scoped override read; canonical config-crate dialect parse shared by app-server; endpoint-scoped Custom replay tag; Zen capture; predicate dedup), and the follow-up round 633ba6b9 binds the resolver's Custom wire gate once (the endpoint-key remap and the protocol selection can no longer drift apart), spells the request field RequestProtocol per the route module's naming convention, pins the override's remaining edges (placeholder base URL, no capability/pricing resurrection, no Zen demotion, no foreign-provider replay), and documents why the rebound_for_model_protocol mismatch tail is unreachable for Custom clients. cd281909e closes the remaining round-1 residue: the three edge pins above plus the preflight wire threading.

Out of scope

Two adjacent gaps reproduce on the pristine base and stay untouched here — both are pre-existing and would smuggle unrelated behavior changes into this PR: the legacy Modelstudio*Anthropic capability-report mismatch in provider_capability_with_wire (the doctor claims Chat while the client speaks Messages; outside the Custom class this PR targets), and the prompt-suggestion gate that still keys on the static chat-completions predicate rather than the resolved route's protocol. The stray blank-line churn in some wire_override: None, test literals is cosmetic and left as-is.

Verification notes

  • cargo test -p codewhale-config -p codewhale-tui -p codewhale-app-server --lib: all green in isolation; under full-suite local parallel load pre-existing remote_control tests flake (they pass in isolation on both this branch and the pristine base 61cb769be), so local-machine load flake, not this change. CI is the authority.
  • cargo clippy --workspace --all-features --locked -- -D warnings -A clippy::uninlined_format_args clean; cargo fmt --check clean.

No-Issue: the gap was surfaced by the parent review (Pinvou/pinvou-agent#625, round 4); the config-wire feature itself ships upstream via Hmbown#1519 - no issue tracks it in this repository.

Round-3 fresh audit + fixes (2026-09-30)

A fresh eight-domain audit of the round-2 head (cd281909e) verified every earlier fix genuine (identity-scoped override, endpoint-scoped replay, single parse, gate bound once; mutation checks confirmed the pins can fail) and found no P1. It did find one behavioral hole plus a cluster of should-fixes, all addressed on this branch (new head ffd897f04):

  1. Replay key carried no endpoint identity (2a4bd2f61) — the tag pins the table name, not the URL behind it: a table whose base_url is edited between sessions replayed the old endpoint's encrypted blobs to the new one. OpaqueReasoningState gains a serde-default endpoint fingerprint (catalog base_url_fingerprint of the client's frozen base URL); capture mints it, the replay gate requires a match, and fingerprint-less legacy states keep replaying (the legacy arm was tightened to fail-closed on Custom in the round-4 review fix below).
  2. Include/capture lockstep was roster-coincidental (7d01b8684) — the capture gate enumerated providers positively while the builder's include gate was a negative list; a future Responses-wire builtin would silently ship include-without-capture. Both now derive from one shared predicate.
  3. Typo'd wire dialects degraded with zero diagnostics (8c34f2323) — wire_dialect_override now logs a warning on unrecognized non-empty values (recognized chat spellings stay silent; the parse stays total), and the canonical alias sets/normalization/degrade contract gained direct unit tests in the config crate.
  4. The fix made /provider display diverge (02826611c) — picker rows resolved through the config-free wrapper and still showed Chat for a wire = "responses" row. The wrapper now takes the override explicitly; the picker, session-state, and model-switch receipts thread their table's dialect, while callers that never consume candidate.protocol() pass None with that reason stated. Also corrects the two-pass resolver comment, the readiness comment (it reads the ambient selection — the same table as its base URL), the app-server comment (this ingress reads the literal [providers.custom] field), and the ProviderConfigToml::wire field doc.
  5. Untested plumbs pinned (1cae5e776) — the runtime receipt's wire_override (protocol + endpoint key for responses/anthropic/chat tables through resolve_runtime_options), a resolution pin for route_is_valid_for_model with a wire table, and the app-server ChatCompletions-only guard posted through the real router asserting 400 provider_wire_format_unsupported (replacing the tautological non_chat_completions_provider_rejected, which asserted a struct field against itself).
  6. Seam and transport pins (7f4364d2f) — a capture-to-replay seam test through the real per-turn client (capture off a live stream → state into turn-loop-placed history → turn 2's prepared body leads with the paired reasoning item), and a wire = "anthropic" transport test (resolve → from_candidate → POST {base}/v1/messages with x-api-key/anthropic-version). The responses turn-path test now builds its client from the resolved route's identity-scoped config, mirroring the production install path.

Round-3 residue, deliberately not addressed here: session-history growth of encrypted blobs (compaction caps thinking text but not state; stripping or capping state risks re-creating the unpaired-replay failure and deserves its own design pass), the app-server pass-through's pre-existing named-table blindness (endpoint, not just wire — changing which table serves the ingress is a behavior change beyond this PR), and the separate built-in vendor wire_prefers_anthropic list in config lib.rs (a distinct dialect space; folding it into the Custom parse would silently widen built-in alias acceptance).

Verification on ffd897f04: cargo test -p codewhale-config -p codewhale-tui -p codewhale-app-server -p codewhale-core --lib green locally with CI's RUST_MIN_STACK (config 645 / core 82 / app-server 101 / tui 11947, 0 failed); cargo clippy --workspace --all-features --locked -- -D warnings -A clippy::uninlined_format_args clean; cargo fmt --check clean. CI is the authority.

Round-4 review fix (2026-09-30)

The review on ffd897f04 confirmed the round-3 direction and caught the one remaining hole — the legacy arm of the endpoint gate — fixed in 57ae408d9:

  • Fingerprint-less Custom states replayed unconditionally — the gate's state.endpoint == None arm matched every legacy (pre-fingerprint) state, so a custom/<table> state saved before this PR kept riding the table's wire after the table's base_url was edited: precisely the cross-endpoint replay the fingerprint was added to close, and one that bricks the old session against the new endpoint (the undecryptable blob stays in history and keeps matching tag/api/model). Custom tags (custom and custom/<table>) are the only tags whose endpoint can move under a stable tag, so they now fail closed: no proof of origin, no replay. Built-in providers have fixed catalog URLs, so their legacy states provably came from the only endpoint the tag ever had and keep replaying (reviewer-suggested carve-out). The upgrade cost on Custom is one turn of reasoning continuity; fresh captures carry the fingerprint and replay resumes.
  • Pins updated accordingly: the legacy Custom assertion now requires a reasoning-free request body (flipping the gate back fails exactly this pin — mutation-checked), the legacy-root custom tag gets the same pin, and a fixed-endpoint (Codex) legacy state pins the carve-out side.

Verification on 57ae408d9: cargo test -p codewhale-tui --lib 11948 passed / 0 failed and cargo test -p codewhale-core --lib 82 passed / 0 failed (one pre-existing remote_control load flake failed once under full-suite parallel load and passes on rerun, same as documented for earlier rounds); cargo clippy --workspace --all-features --locked -- -D warnings -A clippy::uninlined_format_args clean; cargo fmt --check clean. CI is the authority.

Pairing

Parent side: Pinvou/pinvou-agent#625 (app bridge + named table + catalog) pins this branch as the candidate head and carries the fork-register entry; re-pin to pinvou3-clean after this PR merges (the register will need the new candidate head 57ae408d9 after this round).

A named [providers.<name>] table with wire = "responses" (or
"anthropic") minted a Chat Completions candidate: RouteRequest had no
wire channel, so candidate.protocol() came from Custom's
backward-compatible static policy and every per-turn client built by
DeepSeekClient::from_candidate bound Chat regardless of the table's
dialect. The ambient spawn-time client (DeepSeekClient::new) and
provider_capability_with_wire honored the override, so hosts that
resolve routes per turn - the Hmbown#3384 architecture - silently talked to
{base}/chat/completions while their config said Responses.

RouteRequest gains a wire_override honored only for ProviderKind::Custom
(built-ins keep their descriptor policy); the tui route layer populates
it from the active table's dialect (custom_wire_override_for, the same
source provider_wire_format_for_config reads), and the client-internal
rebind/request/limit resolutions pin the transport's own wire so an
engine-internal model switch cannot downgrade it. The minted candidate
is now wire-true end to end: receipts, preflight, and the per-turn
client all read the same protocol.

Pinvou PR Hmbown#625 routes catalog GPT models through such a table and hit
exactly this gap (three review rounds missed it because no test pinned
the wire on the runtime path).

Signed-off-by: asto <asto18089@126.com>
Encrypted reasoning-item capture was gated on the Codex provider, so a
wire = "responses" Custom table streamed reasoning items that were
never persisted and replayed id-less function_call continuations without
their paired reasoning items - the per-round reasoning-continuity loss
the OpenAI cookbook documents for store:false.

Widen the capture gate to every Responses route that sends
include: ["reasoning.encrypted_content"] (Codex plus Custom tables;
DeepSeek and Concentrate stay excluded with their own contracts). The
replay gate already matches the captured provider tag, so Custom states
replay unchanged; both directions are pinned for the Custom route.

Signed-off-by: asto <asto18089@126.com>
asto18089 added a commit to Pinvou/pinvou-agent that referenced this pull request Sep 29, 2026
Bumps the CodeWhale gitlink to the T9 candidate head
(98d4709b905ca5a5abce42da4a2b322c4a88d7b5, Pinvou/CodeWhale#79): the
runtime-route resolver now honors a custom table's wire dialect, so the
responses-wire routing this PR builds in build_dt_config actually
drives the /responses client on every turn, with encrypted reasoning
capture/replay on the route.

Candidate-period registration: fork-guard EXPECTED_HEAD/COMMITS point
at the candidate (50 commits above upstream), six T9 fingerprints pin
the engine behaviors, and both registers document the topic with the
re-pin obligation - once #79 squash-merges into pinvou3-clean, the
landing wave re-pins the public maintenance-branch head and recycles
the candidate constants.

Signed-off-by: asto18089 <asto18089@126.com>
Signed-off-by: asto <asto18089@126.com>
@asto18089

Copy link
Copy Markdown
Collaborator Author

Fresh review of fix/custom-wire-runtime-route @ 98d4709b905

Base check: reviewed against live pinvou3-clean = 61cb769be5 — identical to the PR's merge-base (behind_by: 0), so no rebase was required. CI green on this head; I additionally ran the suites locally (cargo test -p codewhale-config -p codewhale-tui -p codewhale-app-server --lib, clippy -D warnings, fmt --check): all new tests pass, clippy/fmt clean, and the only local failures are the two pre-existing remote_control flake-profile tests, which pass in isolation — consistent with the body's flake note in substance.

This is a real fix for a real blocker: the root-cause diagnosis (named-table wire never reaching RouteRequest, so per-turn from_candidate clients bound Chat while the ambient client honored the override) checks out against the base tree, and it matches parent PR Hmbown#625's round-4 B1 verbatim. The design (request-channel override, mirroring base_url_override) is the right seam, the tests are genuine fix-detectors (six of eight fail without the fix), and I found no smuggled changes: all 13 files map 1:1 to the two commits' claims, the wire_override: None literals are compile fallout or disclosed static-policy sites, no CHANGELOG hunks (per policy), DCO clean. Keeping the capture commit in this PR is fine by me — commit 1 alone makes Custom-Responses routes reachable while still losing reasoning items on multi-turn tool continuations, so the two commits are one behavior boundary; the commit-level separation plus the body section is adequate disclosure.

However, there is one blocker — the fix has a hole exactly in the divergence class it claims to close — plus four should-fixes.

B1 (BLOCKER) — the wire override is read from the ambient config, not the identity-scoped config, on every pinned turn path

crates/tui/src/route_runtime.rs:797 builds route_config = prepared_route_config(config, &identity, …), which clones and calls scope_to_provider_identity (config.rs:5502) — re-pointing provider at the identity's table key, which is what route_config.deepseek_base_url() (:809) resolves the endpoint from. But :813 passes custom_wire_override_for(config) — the unscoped ambient config.

For resolve_runtime_route (the ambient entry point) identity and config.provider coincide by construction, so the new tests pass. But resolve_runtime_route_for_identity is a production turn path whose identity is resolved independently of the ambient selection — per-thread persisted identities (runtime_threads.rs:3260/3295/3297 resolved_route_for_thread, :3361 reload_config which then installs the candidate for every active thread, :7215 start_turn fixed-model path, :5388 create_thread with explicit model_provider_id), fleet pins (lib.rs:8209 → validate() → from_candidate), session restore (tui/ui/apply.rs:3307), and route-cache warm (tui/ui/provider_routes.rs:420). For any of these, when the thread/pin is on table B while the ambient config.provider selects table A:

  • Direction (i): B has wire = "responses", A doesn't → override None → Chat candidate → the exact mis-route this PR fixes, still live on every pinned route.
  • Direction (ii): A has wire = "responses", B is an ordinary chat relay → override Some(Responses) → a Responses candidate for B's chat endpoint, bound by from_candidate and installed by reload_config → POSTs to {base}/responses on a chat-only relay. On base 61cb769be table B worked (static Chat everywhere), so this direction is a regression for multi-table setups.

Note start_turn's auto path (:7189) already passes the scoped thread_config while the fixed-model path right below doesn't — the inconsistency is visible inside one function. The fix is one argument: pass custom_wire_override_for(&route_config) (for legacy literal-root identities scoping removes the literal table, dialect → None → static Chat, which is correct since that shape has no wire field), plus a regression test that resolves table B via resolve_runtime_route_for_identity while config.provider selects table A — which is precisely why the current suite can't see this.

Should-fixes

S1 — census claim "agree by construction" is false on the app-server pass-through. crates/app-server/src/chat_completions.rs:115 forwards with wire_override: None and endpoint_preserves_raw_model_ids explicitly serves Custom (:218-226), so for wire = "responses" the resolver mints Chat and the "Only ChatCompletions providers are supported" guard at :350 passes — a chat body is silently forwarded to {base}/chat/completions, the exact mis-route claim 1 forbids. Not a regression (base identical) and this ingress speaks chat upstream by design, but threading the override would convert this to the handler's intended fail-closed 400; until then the census claim needs scoping. The string→WireFormat parse for ProviderConfigToml.wire currently lives only in tui — per the repo's own rung-2 rule it belongs in the config crate next to the field (which also unblocks resolve_runtime_options_with_secrets at config/src/lib.rs:3382, another static-policy site with provider_cfg.wire in scope).

S2 — include-without-capture persists on OpenCode Zen, so claim 4's invariant is false as stated. The capture gate (client/responses.rs:174-177) is Codex + Custom-Responses, but the include gate (:144-146, !is_deepseek && !is_concentrate) also covers OpencodeZen — a first-class Responses provider (ModelAware closed roster, 24 responses models, dedicated RouteShape::OpencodeZen arm at client.rs:2170). Zen sends store:false + include: ["reasoning.encrypted_content"] and never captures, so the exact unpaired-function_call replay loss the body describes still exists there. Pre-existing, not a regression — but the new comment "mirrors the capture gate" encodes the wrong invariant. Either add Zen to reasoning_origin (the replay gate would match its tag) or rescope comment + body to "Codex and Custom tables".

S3 — the replay gate has no endpoint identity, and this PR newly populates the hazard. responses.rs:821-824 matches provider == "custom" && api == "openai-responses" && model == request.model — no table/endpoint identity, and every named table shares the slug "custom" (provider.rs:1684). Two wire = "responses" tables A and B serving the same model id: reasoning state captured under A rides in session history (OpaqueReasoningState serializes into persisted Thinking blocks; nothing on resume strips it) and is POSTed to B after a provider switch — typically a 400 that then repeats every turn. Opaque ciphertext, so no readable leak, but endpoint-A-scoped material ships to B. The gate predates this PR, but only this PR mints "custom"-tagged states, so the population is introduced here. Suggested: fold the table's persisted id (the client already holds provider_identity) into the tag, or add an endpoint digest; old states simply stop replaying, which is fine at this age.

S4 — "by construction" rests on duplicated alias predicates. The PR promotes wire_config_prefers_anthropic/responses to pub(crate) (tui/config.rs:9702/9718) for custom_wire_override_for, but client.rs:1825-1849 keeps byte-identical private copies feeding provider_wire_format_for_config. A future alias added to one list silently desyncs ambient wire from runtime candidate — the drift claim 5 says is closed. Have client.rs consume the canonical copies (no dependency-direction obstacle; client.rs already imports both modules). Ideally custom_wire_override_for then collapses into the shared mapping entirely — note the resolver already treats Some(ChatCompletions) exactly like None, so even passing provider_wire_format_for_config(Custom, Some(config)) straight through would be semantics-preserving.

Minors / nits

  • M1 (body): the verification note's "three pre-existing remote_control/client translate tests flake" doesn't match the tree: exactly one client translate test exists (client.rs:8339, passes under load); the flaky pair is remote_control::tests::restart_rehydrates_runtime_chat_ownership_until_terminal_cursor and separate_predispatch_crashes_on_one_run_get_distinct_recovery_turn_ids. Substance of the note (pre-existing, isolation-green, CI authoritative) holds; the census should be corrected.
  • M2 (tests): config tests never exercise the Some(WireFormat::ChatCompletions) arm (explicit chat ≡ default), Custom + base_url_override: None, or that a wire-true custom candidate still carries RouteCapabilities::default() / PricingSku::UnknownOrStale; the route_runtime anthropic test omits endpoint_key == "messages"; the capture test never asserts state.model (half of the replay key).
  • M3 (nit): stray #[allow(clippy::too_many_arguments)] on the one-argument custom_wire_override_for (route_runtime.rs:454); the x3 repeated pin expression (api_provider == Custom).then_some(self.wire_format) would read better as one named helper; several hunks add a stray blank line before wire_override: None,; wire_override: Option<WireFormat> could be spelled Option<RequestProtocol> per the route module's own naming convention (route/mod.rs:17-25) — same type.
  • M4 (claim scoping): provider_capability_with_wire vs provider_wire_format_for_config disagree for the legacy Modelstudio*Anthropic kinds (config.rs:850-858 vs client.rs:1767-1771) — outside this PR's Custom class, but claim 5 should be scoped to Custom explicitly.

Verified clean

Root cause vs base tree (no wire channel; Custom::wire_policy() = Fixed(ChatCompletions) ignoring endpoint key; ambient/candidate split exactly as described); resolver gate Custom-only with Some(Chat) ≡ None, endpoint_key/protocol never disagreeing, no new error paths, capabilities/pricing reset not bypassed; from_candidate binds candidate.protocol(); client pin sites transport-true in every constructor; rebound_for_model_protocol fail-closed for Custom (mismatch branch unreachable); all resolve_route_candidate* consumers wire-independent (validation/display only — provider_readiness included); capture fail-closed (empty encrypted_content filtered; no plaintext fallback on gate miss), no logging of reasoning material, no new network surface, per-turn cost O(1); pairing evidence verified (parent Hmbown#625 round-4 B1 and the fork register pinning this exact head); DCO and commit convention conformant; no dead code added.

Verdict: CHANGES_REQUESTED — B1 must land (it's small: one argument + one test); S1–S4 are strongly recommended in the same round or explicitly pinned as follow-ups with the census claims rescoped accordingly.

@asto18089 asto18089 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.

Companion note from the fresh paired review of Pinvou/pinvou-agent#625 (9 lanes, head 98d4709b9 verified against base 61cb769be). The core mechanism holds up end-to-end: the wiremock gate drives the real per-turn construction (resolve_runtime_route → from_candidate → create_message_stream), the dialect is single-sourced through provider_wire_dialect, legacy behavior with wire_override: None reduces byte-identically to the old path, and capture/replay widening is order-safe and compaction-safe. One required item and a few follow-ups from the engine lanes:

Required — app-server ingress stays wire-blind for the tables this PR introduces. crates/app-server/src/chat_completions.rs:115-123 resolves with wire_override: None; Custom is Fixed(Chat), so route.protocol() is Chat and the provider_wire_format_unsupported guard at :351-361 can never fire for a wire = "responses" table — the chat body goes to {base}/chat/completions (:257) and fails with an opaque upstream 404 instead of the clean rejection the guard promises. Either thread the override (or read the table and fail closed) here, or declare the boundary in the fork register + PR body with a tracked follow-up.

Follow-ups (non-blocking, evidence-checked):

  • custom_wire_override_for(config) reads the unscoped config on the identity-scoped path (route_runtime.rs:805-814 vs route_config scoped at :793) — a cache-replay/validation resolution for a non-current custom identity reads the globally-selected table's wire; fix is passing &route_config.
  • Replay is endpoint-blind across Custom routes (tag+model only, responses.rs:821-825): a base-url edit re-sends gateway A's encrypted_content to gateway B. Surfaced 400, provider-encrypted blob — consider endpoint identity in the tag.
  • Two guards are unpinned: deleting the wire_format == Responses half of the capture gate (responses.rs:174-177) passes the suite, and the missing/empty encrypted_content filter (responses.rs:506) survives mutation.
  • A malformed wire value silently degrades to Chat — no validation warning, no test.
  • The wire-alias matchers now exist in two byte-identical copies (config.rs:9705/9721 vs client.rs:1826/1842); alias drift would split ambient vs candidate wire.
  • provider_readiness::route_is_valid_for_model passes wire_override: None for Custom — outcome-identical today, but it validates a protocol candidate that is not the per-turn one.

Nit: stray #[allow(clippy::too_many_arguments)] sits above the doc comment of the one-arg custom_wire_override_for (route_runtime.rs:451), copied from the many-arg neighbor below.

Review round 1 of #79 (B1, S1): the wire override was read from the
ambient `Config` while `resolve_runtime_route_for_identity*` resolves the
endpoint from the identity-scoped clone, so on every pinned turn path
(per-thread routing, `reload_config`, fleet pins, session restore) the
override could name a different `[providers.<name>]` table than the one
supplying the endpoint: a pinned responses table rode Chat, and an
ambient responses table wired Chat-only relays for `/responses` (a
regression against the static-policy base for multi-table setups). The
override now comes from the scoped clone and is computed only for
`ProviderKind::Custom`.

The per-config `wire` dialect parse moves into the config crate next to
the field it reads (`wire_dialect_override` plus the alias predicates), so
the tui wire-format/capability readers share one alias list with the
resolver instead of byte-identical private copies. The app-server
`/v1/chat/completions` pass-through threads the same override from the
`provider_cfg` that supplies its endpoint, so a `wire = "responses"`
table now fails closed at the handler's ChatCompletions-only guard
instead of silently receiving a chat body.

Also pins the explicit `chat` dialect arm and the `messages` endpoint key
in tests, and drops a copy-pasted `#[allow(clippy::too_many_arguments)]`
on the one-argument helper.

Signed-off-by: asto <asto18089@126.com>
Review round 1 of #79 (S2, S3): every named Custom table shares the
`custom` provider slug, so Custom-tagged opaque reasoning state replayed
onto any table resolving the same model id — table A's encrypted
reasoning rode table B's wire after a provider switch. The replay gate
now compares an endpoint-scoped tag (`custom/<table>` from the client's
frozen provider identity); no released build mints the old bare tag, so
nothing stops replaying.

Capture also widens to the OpenCode Zen Responses roster, which sends
`include: ["reasoning.encrypted_content"]` and `store: false` but never
captured — the same unpaired-function_call replay loss this PR fixed for
Custom tables. Include and capture now cover the same route set by
construction, and the turn-path pin expressions collapse into
`pinned_wire_override` / `reasoning_provider_tag` helpers.

Signed-off-by: asto <asto18089@126.com>
@asto18089

Copy link
Copy Markdown
Collaborator Author

Round-1 fixes pushed — 747329d5 + 2871dfff

All findings from the round-1 review that I judged actionable are addressed; two cosmetic/out-of-scope items declined. Details:

B1 (blocker) — fixed in 747329d5. The wire override now comes from the identity-scoped clone in resolve_runtime_route_for_identity_with_limits (endpoint and wire always name the same table), computed only for Custom. Regression tests pin both directions through the public authority: forkguard_identity_pinned_route_reads_the_pinned_tables_wire (pinned responses table under an ambient chat selection) and forkguard_ambient_wire_override_does_not_leak_onto_pinned_chat_tables (ambient responses must not wire a pinned chat-only relay) — both fail on the parent commit.

S1 — fixed in 747329d5. The wire dialect parse now lives in the config crate (wire_dialect_override + the alias predicates, next to the ProviderConfigToml::wire field). The app-server /v1/chat/completions pass-through threads the override from the same provider_cfg that supplies its endpoint, so a wire = "responses" table now hits the handler's ChatCompletions-only guard (fail closed, provider_wire_format_unsupported) instead of silently receiving a chat body. resolve_runtime_options_with_secrets threads it too, closing the stale-receipt trap. Pinned by custom_table_wire_override_reaches_the_app_route.

S2 — fixed in 2871dfff. Capture widened to OpenCode Zen's Responses roster (opencode_zen_responses_stream_captures_encrypted_reasoning), so include and capture cover the same route set by construction and the comment/body no longer overclaim.

S3 — fixed in 2871dfff. Captured state is tagged custom/<table> from the client's frozen provider identity, and the replay gate requires that exact tag. New cross-table negative (custom/other_relay never replays table A's state); the no-released-build-mints-the-old-tag property makes the migration a non-issue.

S4 — fixed in 747329d5. client.rs's private predicate copies are gone; tui consumes the canonical config-crate parse, so the "by construction" claim is now literally true. The custom_wire_override_for helper collapsed to the shared wire_dialect_override.

Minors also fixed: explicit chat dialect arm + messages endpoint-key assertions, state.model capture assertion, the copy-pasted #[allow(clippy::too_many_arguments)] on the one-argument helper, and the repeated pin expression collapsed into pinned_wire_override / reasoning_provider_tag.

Declined (with reasons, per review discipline): the stray blank-line churn in wire_override: None, test literals (cosmetic, fmt-clean); wire_override: Option<WireFormat> vs RequestProtocol spelling (same alias type, churn only); the pre-existing provider_capability_with_wire mismatch for legacy Modelstudio*Anthropic kinds (outside this PR's Custom class — the body's claim is scoped to Custom instead).

Local verification on 2871dfff: config 640 / app-server 101 / tui 11932 passed, clippy -D warnings clean, fmt --check clean; the only full-suite failures are the pre-existing remote_control load flakes (isolation-green, untouched by this diff). CI is the authority. The body's census and flake-note wording are corrected accordingly, and the parent register re-pin note now names the new candidate head.

Round-1 review residue: the resolver's Custom wire channel was gated by
two `provider_kind == Custom` checks forty lines apart - the endpoint-key
remap and the protocol selection - so a future edit to one gate could
silently desync the key from the protocol. The override is now bound once
above both consumers. The new field is also spelled `RequestProtocol`
per the route module's naming convention (same alias; request-side wire
shapes are spelled that way there).

Test pins: the override mints the wire even on the descriptor's loopback
placeholder base URL; a wire-true custom candidate still carries default
capabilities and UnknownOrStale pricing (no first-party fact
resurrection); a Chat override cannot demote a Responses-descriptor
builtin (OpenCode Zen); and Codex-minted opaque reasoning state never
replays onto a Custom route. The protocol-mismatch tail of
`rebound_for_model_protocol` now documents that the pinned override makes
it unreachable for Custom clients.

Signed-off-by: asto <asto18089@126.com>
@asto18089

Copy link
Copy Markdown
Collaborator Author

Round-2 fixes pushed — 633ba6b9 (review residue)

Follow-up on my round-1 review's minor/residue items, on top of the round-1 fix round:

  • Resolver gate bound once: the Custom wire channel was gated by two provider_kind == Custom checks forty lines apart (endpoint-key remap vs protocol selection) — a future edit to one could silently desync the key from the protocol. One binding now feeds both.
  • Naming convention: the new request field is spelled Option<RequestProtocol> per route/mod.rs's own rule ("the request/response wire shape is spelled RequestProtocol"); same alias, so zero behavior change.
  • Edge pins added: the override mints the wire on the descriptor's loopback placeholder base URL (fail closed locally); a wire-true custom candidate still carries default capabilities and UnknownOrStale pricing (no first-party fact resurrection); a Chat override cannot demote a Responses-descriptor builtin (OpenCode Zen); Codex-minted opaque state never replays onto a Custom route.
  • Comment: rebound_for_model_protocol's protocol-mismatch tail now records that the pinned override makes it unreachable for Custom clients.
  • Body: added an explicit Out of scope section naming the two pre-existing adjacent gaps we are deliberately not fixing here (the Modelstudio*Anthropic capability-report mismatch and the prompt-suggestion static gate) so they don't get lost.

Still declined, deliberately: fixing those two pre-existing gaps inside this PR (scope smuggling), and the blank-line churn (cosmetic).

Local: config 641 / tui full suite green except the known pre-existing remote_control load flakes (they reproduce identically on the CI-green parent head on this machine, isolation-green everywhere), clippy -D warnings clean, fmt --check clean. CI is the authority.

@asto18089 asto18089 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.

Rebase follow-up: cd281909e lands the remaining round-1 residue on top of the 747329d57/2871dfff9/633ba6b9e wave — my earlier working tree had parallel implementations of B1/S1/S2, so it was dropped in favor of this wave's designs (identity-scoped tag and config-crate dialect parse included) and only the genuinely missing pieces were ported:

  • forkguard_custom_chat_stream_does_not_capture_encrypted_reasoning — a Chat-wire Custom table never mints opaque state even when a Responses-shaped stream reaches handle_responses_stream; deleting the wire half of the capture gate fails exactly this pin.
  • forkguard_custom_responses_capture_tolerates_missing_or_empty_encrypted_content — missing/empty encrypted_content is skipped while the stream keeps flowing and both blocks close; deleting the empty-content filter fails exactly this pin.
  • forkguard_named_table_unrecognized_wire_keeps_the_chat_default — a typo'd dialect degrades to the legacy Chat policy at both the override reader and the runtime candidate (pins wire_dialect_override's silent-degrade contract).
  • route_is_valid_for_model (preflight) now validates the table's own dialect, so receipts/preflight see the same protocol candidate the turn path mints — outcome-identical today, but protocol-conditional validation can no longer drift from the turn path.

Verified on the rebased head: tui full suite 11932 pass / 0 fail (the four known runtime-dir contention flakes pass serially), config 642/0, core 82/0, app-server 102/0, clippy/fmt clean. The parent PR Hmbown#625 gitlink and its T9 register/fingerprints (14) are rebased onto this head.

asto18089 added a commit to Pinvou/pinvou-agent that referenced this pull request Sep 29, 2026
Bumps the CodeWhale gitlink to the T9 candidate head
(98d4709b905ca5a5abce42da4a2b322c4a88d7b5, Pinvou/CodeWhale#79): the
runtime-route resolver now honors a custom table's wire dialect, so the
responses-wire routing this PR builds in build_dt_config actually
drives the /responses client on every turn, with encrypted reasoning
capture/replay on the route.

Candidate-period registration: fork-guard EXPECTED_HEAD/COMMITS point
at the candidate (50 commits above upstream), six T9 fingerprints pin
the engine behaviors, and both registers document the topic with the
re-pin obligation - once #79 squash-merges into pinvou3-clean, the
landing wave re-pins the public maintenance-branch head and recycles
the candidate constants.

Signed-off-by: asto18089 <asto18089@126.com>
Signed-off-by: asto <asto18089@126.com>
Round-1 residue pins for the custom wire channel, rebased onto the identity-scoped override work:

- A Chat-wire Custom table must not mint opaque reasoning state even when a Responses-shaped stream reaches handle_responses_stream - the capture gate keys on the transport's wire, not the event shape. Deleting the wire half of the gate fails exactly this pin.
- Missing or empty encrypted_content must not be captured (an empty blob would poison every later turn) while the stream keeps flowing and both reasoning blocks close. Deleting the empty-content filter fails exactly this pin.
- A typo'd wire = "respones" dialect degrades to the legacy Chat default at both the override reader and the runtime candidate, so the silent-degrade contract of the shared dialect parser stays deliberate.
- route_is_valid_for_model now validates the wire the per-turn route would mint: the dialect of the same table provider_config_for resolves, which also supplies the validated base URL. Outcome-identical today (Custom route validation is protocol-independent), but any future protocol-conditional validation would otherwise silently drift from the turn path.

Signed-off-by: asto <asto18089@126.com>
The endpoint-scoped replay key compared the provider tag, api shape, and
model - but the tag only pins the named Custom table's NAME, not the URL
behind it. A table whose base_url is edited between sessions (proxy to
vendor-direct, relay swap) replayed the old endpoint's encrypted blobs to
the new one, producing sticky 400s whose cause was invisible.

OpaqueReasoningState gains a serde-default endpoint fingerprint (the
catalog's base_url_fingerprint of the client's frozen base URL). Capture
mints it; the replay gate requires it to match, while fingerprint-less
states from fixed-endpoint providers and pre-existing sessions keep
replaying. The custom capture/replay/turn tests pin capture binding,
mismatch drop, and legacy replay.

Signed-off-by: asto <asto18089@126.com>
The capture gate enumerated its providers positively (Codex, Zen, Custom
Responses) while the body builder's include gate was a negative list
(not DeepSeek, not Concentrate). They coincided only through the current
provider roster: a future Responses-wire builtin would silently start
sending include: ["reasoning.encrypted_content"] without capturing,
re-creating the unpaired function_call replay loss on multi-turn tool
continuations.

Both gates now derive from one shared predicate
(responses_route_sends_encrypted_reasoning_include), so the lockstep the
PR body claims holds by construction. The transport-wire half stays in
the capture gate; the current route set is unchanged.

Signed-off-by: asto <asto18089@126.com>
A typo'd wire value ("respones") degraded silently to the default Chat
Completions policy on every surface, so a user who typo'd the one string
that switches their endpoint's protocol saw an opaque wrong-wire failure
at request time with no diagnostic anywhere. wire_dialect_override now
logs a warning for non-empty unrecognized values; recognized explicit
chat spellings (chat, openai, ...) stay silent, the parse stays total,
and the degrade contract is unchanged.

Also pins the canonical alias sets, the normalization (case, whitespace,
underscore/hyphen), and the degrade behavior with direct unit tests in
the crate that owns them — coverage was previously indirect via the tui.

Signed-off-by: asto <asto18089@126.com>
The /provider picker resolved rows through the config-free candidate
wrapper, which pins the static Chat policy — so after the runtime fix a
wire = "responses" row bound Responses per turn while the picker still
displayed Chat Completions as its supported protocol. The wrapper now
takes the wire override explicitly; the picker passes the dialect of the
same row-scoped table that supplied its base URL, and the session-state
and model-switch receipts thread the ambient table's dialect the same
way. Callers that never consume candidate.protocol() (unpinned child
admission, the /model receipt) pass None with that reason stated.

Also corrects three inaccurate comments left by earlier rounds: the
two-pass resolver note (the wire override rides both passes), the
readiness claim (it reads the ambient selection, the same table as its
base URL — identity-pinned tables are the route layer's job), and the
app-server claim (this ingress reads the literal [providers.custom]
field, not the named-table map), plus the ProviderConfigToml::wire field
doc, which still described only the built-in dual-wire vendors.

Signed-off-by: asto <asto18089@126.com>
Three seams could regress silently because nothing bound them:

- config: the runtime receipt's wire_override (deleting it changed no
  receipt a test asserted) - the new test resolves responses/anthropic/
  chat tables through resolve_runtime_options and pins protocol plus
  endpoint key.
- tui: route_is_valid_for_model's dialect threading now has a
  resolution pin for a wire table. The bool interface cannot observe
  protocol directly (validation is protocol-independent); the resolver
  and receipt pins above carry the protocol assertions.
- app-server: the ChatCompletions-only guard is now posted through the
  real router and asserted as a 400 provider_wire_format_unsupported.
  This replaces non_chat_completions_provider_rejected, which built a
  struct and asserted a field against itself without invoking the
  handler.

Signed-off-by: asto <asto18089@126.com>
Two coverage gaps the audit found:

- Capture was tested at the client and replay at the pure builder, so an
  asymmetric edit to the tag or endpoint fingerprint derivation would
  only be caught by luck. The new seam test captures off a real stream
  on the per-turn client, places the state into history the way the turn
  loop commits it, and asserts turn 2's prepared request body leads with
  the paired reasoning item.
- wire = "anthropic" custom tables were pinned only at the candidate
  level, one layer short of the wire. The new transport test drives
  resolve_runtime_route -> from_candidate against a loopback mock and
  asserts the POST path, x-api-key/anthropic-version headers, and the
  verbatim model.

Also builds the custom responses turn-path client from the resolved
route's identity-scoped config instead of the ambient config, mirroring
the production install path - in a two-table setup the ambient table
would freeze the wrong identity onto the pinned endpoint.

Signed-off-by: asto <asto18089@126.com>
Signed-off-by: asto <asto18089@126.com>
@asto18089
asto18089 force-pushed the fix/custom-wire-runtime-route branch from cd28190 to ffd897f Compare September 30, 2026 03:52
@asto18089

Copy link
Copy Markdown
Collaborator Author

Round-3 fresh audit complete — fixes pushed (ffd897f04)

A fresh eight-domain audit (config/resolver, route call sites, capture/replay lifecycle, app-server, smuggle census, performance/concurrency, adversarial failure hunt, test-quality with live mutation checks) ran against cd281909e. Verdicts: all round-1/2 fixes independently verified genuine (the identity-scoped override reads the scoped clone before the dialect read; capture keys on the transport wire; tags are endpoint-scoped; the alias predicates are single-sourced). Mutation checks confirmed the load-bearing pins fail when the fix breaks (resolver ignores the override → mints test red; ambient read restored → both identity tests red in the exact mis-route directions; wire half deleted from the capture gate → chat-stream negative red). Smuggle census clean — every hunk maps to a claim or a test; nothing needs splitting. No P1 found.

The audit found one behavioral hole plus should-fixes; all valuable findings are now fixed on this branch (7 commits, new head ffd897f04, full list in the updated PR body):

  • 2a4bd2f61 — replay key now carries the endpoint fingerprint (the audit's P2): the tag pins the table name, not the URL, so an edited base_url replayed old-endpoint blobs to the new endpoint. OpaqueReasoningState.endpoint (serde-default) closes it; legacy states keep replaying.
  • 7d01b8684 — include/capture gates derive from one shared predicate (lockstep by construction, not by roster coincidence).
  • 8c34f2323 — unrecognized wire dialects now log a warning; canonical alias sets get direct config-crate unit tests.
  • 02826611c — the /provider picker (and session/model-switch receipts) now resolve with the table's wire, fixing the Chat-label divergence this PR's own fix created; three inaccurate comments + the wire field doc corrected.
  • 1cae5e776 — the three untested plumbs pinned (config receipt protocol+endpoint-key, readiness resolution, real-router 400 provider_wire_format_unsupported; the tautological guard test replaced).
  • 7f4364d2f — capture→replay seam test through the real client + the wire = "anthropic" transport test; the responses turn-path test now builds from the identity-scoped route.config like production.
  • e74eca695 (amend of cd281909e) — honest commit type (fix, it carried the readiness threading) and single Signed-off-by.

Declined, with reasons: capping/stripping encrypted blobs in session history (needs a design pass — naive caps re-create the unpaired-replay failure this PR fixes), threading the app-server pass-through to named tables (pre-existing endpoint-resolution semantics, a behavior change beyond scope; the comment now scopes the claim), and folding the built-in vendor wire_prefers_anthropic list into the canonical parse (a separate dialect space; would silently widen built-in alias acceptance). Known environmental note: the tui provider_picker tests stack-overflow locally without RUST_MIN_STACK on this machine — reproduces on the pristine base, CI sets 8 MiB and is green.

Verification on ffd897f04: config 645 / core 82 / app-server 101 / tui 11947 all green locally (CI's RUST_MIN_STACK), workspace clippy -D warnings clean, fmt --check clean. CI is the authority.

@JensenChen28 JensenChen28 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

当前 head 的必需门禁均已通过,round-3 的 endpoint fingerprint 方向也正确,但仍有一个跨 endpoint 重放缺口,需要修复后再批准。

[P1] fingerprint 缺失时,Custom opaque reasoning state 仍会无条件重放到当前 endpoint。 crates/tui/src/client/responses.rs:880-883 把 state.endpoint == None 判为匹配;对应测试还明确固定了该行为。这样,升级前已保存的 custom/<table> state 在用户修改同一 table 的 base_url 后,会把旧 endpoint 产生的 encrypted_content 发给新 endpoint。table tag 只能固定名称,无法证明旧 state 来自当前 URL,因此这正好绕过本轮新增的 endpoint 边界。

请对 Custom state 采用 fail-closed 迁移:缺少 fingerprint 时不重放;如需兼容固定 endpoint provider,可只为其保留 None => true。同时把回归测试改为断言 fingerprint-less Custom state 不进入请求。这样既保留固定 provider 的旧会话兼容,也不会把旧网关的 opaque payload 发给重新指向的网关。

验证范围:复核了 cd281909e..ffd897f04 的新增提交、capture/include 共用谓词、capture/replay fingerprint 流向和相关回归测试;current-head required checks 全绿。未在本机重跑完整测试套件。

@asto18089

Copy link
Copy Markdown
Collaborator Author

Round-4 fresh audit — no blockers, no majors; head ffd897f04 approved as-is modulo the minors below

Fresh 11-track review (claim completeness, design, smuggle census, route semantics, capture/replay, security/fail-closed, performance, test quality + static mutation analysis, edge cases, concurrency/conventions, PR-body honesty) against live pinjou3-clean = 61cb769be5. Base check: the head is a direct descendant of the base tip (merge-base == 61cb769be5); no rebase was needed. All evidence below was re-derived from the tree at ffd897f04, not carried over from earlier rounds.

The four gate questions

  1. Useful / complete / root-caused: YES. The base bug reproduces verbatim: base RouteRequest had no wire channel (route/resolver.rs:51-68), Custom::wire_policy was Fixed(ChatCompletions) (provider.rs:1718-1727), and from_candidate (client.rs:1190-1198) bound Chat for per-turn clients while DeepSeekClient::new/provider_capability_with_wire honored the override. The fix lands at the root (wire channel on RouteRequest, identity-scoped threading at the route layer) and every turn-serving construction site now carries a wire decision — all 7 production RouteRequest literals, all candidate.protocol() consumers, ambient new sites, engine install/compaction, app-server ingress, CLI/one-shot flows were audited individually. The one remaining Chat-emitting path (background prompt suggestions) is pre-existing and disclosed.
  2. Elegant: YES (acceptable-to-elegant). The explicit channel is justified — the resolver is deliberately config-free/pure and has no table identity, base_url_override is the in-struct precedent, and a dynamic Custom::wire_policy would need trait changes plus resolver config access. The dedup is real: two byte-identical private parser copies deleted with all consumers migrated, zero #[allow(dead_code)] movement, pre-existing base_url_fingerprint reused, ~82% of the diff is tests.
  3. Smuggled changes: NONE. 21/21 files classified hunk-by-hunk; the admitted blank-line churn is 23 blank lines in test literals, assertions untouched. The encrypted-reasoning widening is the one hidden second feature, but it is load-bearing for the fixed route (pre-PR those routes already sent include with capture absent — replay was broken) and shares the exact Codex machinery.
  4. Bug-free: no new functional or performance bugs found. Route semantics (endpoint-key→URL identity with the ambient path, [providers.deepseek] builtin-mapping consistent across all 7 consumers, rebound_for_model_protocol unreachability holds, limits unaffected), capture/replay (pairing order verified end-to-end, replay emit byte-identical to the Codex path, compaction cannot split a pair, reload/restore graceful), concurrency (one frozen config snapshot per turn; reload fails closed pre-swap; no client pool exists on this path), performance (all new costs per-turn/per-request/per-picker-open, O(1) per SSE event, fingerprint computed once per request).

Verification (local + CI)

  • cargo test --lib on ffd897f04: config 645/0, core 82/0, app-server 101/0, tui 11937 passed with 4 failures — the two remote_control flakes pass in isolation (matching the PR body's disclosure); the two sandbox::process_hardening::no_new_privs child-process tests fail even isolated on this arm64 host but are untouched by the diff and pass in CI. cargo fmt --check clean; cargo clippy --workspace --all-features --locked -- -D warnings -A clippy::uninlined_format_args clean. CI on the head: gate/check/CodeQL/Gitleaks/DCO all green.
  • Static mutation analysis of the PR's pins: wire-override builtin immunity, capture-keyed-on-transport-wire, endpoint-fingerprint replay gate, and custom-only gate all have failing tests under deletion. The two claimed mutation checks and the tautology replacement (non_chat_completions_provider_rejected really did assert a struct field against itself) verify as claimed. PR-body honesty: one stale sentence (below), everything else checked out.

Should-fix minors (none blocking)

  1. Stale census sentence in the PR body: "The config-free resolve_route_candidate* wrappers keep the static policy (validation/display only; no client construction)" is contradicted by round-3 item 4 in the same body — the picker and session-state/apply/init receipts now thread the dialect through resolve_route_candidate_with_context_metadata* (route_runtime.rs:446-460). Reword the census paragraph.
  2. Second alias list in the same crate: wire_prefers_anthropic (config/src/lib.rs:4592-4610) keeps a byte-identical normalization+alias copy for the same wire field while the new parse's doc claims "one alias list cannot drift from another" (provider.rs:1729-1732). Only the kind-scoping needs to stay separate; the string half can delegate (matches!(kind, ...) || provider::wire_dialect_prefers_anthropic(wire), −12 lines), otherwise adding an alias yields a DeepSeek table with an Anthropic wire and a Chat default base URL.
  3. Builtin-table typo never warns: the field doc (config/src/lib.rs:166-169) says unrecognized values "fall back … with a warning", but the warn is only reachable from Custom-gated call sites — wire = "respones" under [providers.deepseek] is silent. Narrow the doc or validate every table at config load.
  4. Whitespace-only blobs capture: the encrypted_content filter (responses.rs:549) is !is_empty; " " passes and replays every turn. !v.trim().is_empty() is a one-character tighten (the tolerance test only covers "").
  5. Untested pins (deletion-survivable today): the rebind/same-client wire pins (client.rs:1484,1543) and the "Custom client never reaches the rebuild tail" invariant; the app-server Custom-only gate from the builtin side; the picker's protocol display threading; the warning emission (no tracing harness exists). One rebound_for_model_protocol test on a wire=responses custom client would close the largest gap.
  6. Fingerprint normalization inheritance (pre-existing primitive, newly security-load-bearing): credential-only userinfo edits don't rotate the fingerprint (secret-stripping is correct, but tenancy changes pass), and all degenerate/secret-bearing URLs hash to one constant. Both are defused in practice by the tag gate + client URL validation; worth hardening (per-input redacted digest / coarse tenancy discriminator) or documenting at the gate.
  7. Docs: the custom-table wire key is still absent from docs/CONFIGURATION.md's provider-table key list and docs/PROVIDERS.md's custom-table section, even though this PR makes it per-turn authoritative.

Pre-existing follow-ups (verified on pristine base, correctly out of scope here)

  • The built-in dual-protocol counterpart of this exact bug class: wire = "anthropic" on [providers.minimax]/Modelstudio tables flips the ambient base URL to the vendor's Anthropic surface while the runtime route candidate stays Chat (route_runtime.rs:819-821 + resolver.rs:269-271 gate) — a Chat client bound to an Anthropic URL. Same class, outside the Custom class this PR targets; strongest candidate for the next PR.
  • Prompt suggestions still gate on the static chat-completions predicate and hardcode chat/completions (disclosed); a one-line fail-closed tightening in resolve_credentials_for_identity (reject when candidate.protocol() != ChatCompletions) would stop the mis-POST without implementing a Responses suggestion transport.
  • Multi-reasoning-item responses keep only the last state (pre-existing Codex machinery, untouched); session-history growth of encrypted blobs (disclosed residue).

DCO 13/13, zero dead-code movement, scoped crates/tui/AGENTS.md rules respected. Verdict: approve at ffd897f04; the minors can land as a docs/body touch-up or a small follow-up round at the author's discretion.

Review on this PR flagged the legacy-state arm of the endpoint gate:
state.endpoint == None matched unconditionally, so a Custom-table state
saved before the endpoint field existed kept replaying after the table's
base_url was edited — exactly the cross-endpoint leak the fingerprint
was added to close, and one that bricks the old session against the new
endpoint (the undecryptable blob stays in history and keeps matching).

Custom tags (the legacy root 'custom' and named 'custom/<table>') are
the only tags whose endpoint can move under a stable tag, so they now
fail closed: no proof of origin, no replay. Built-in providers have
fixed catalog URLs, so their legacy states provably came from the only
endpoint the tag ever had and keep replaying. The upgrade cost on
Custom is one turn of reasoning continuity; fresh captures carry the
fingerprint and replay resumes.

Pins: the legacy Custom state assertion is flipped to require a
reasoning-free request body, the legacy-root 'custom' tag gets the same
pin, and a fixed-endpoint (Codex) legacy state pins the carve-out side.

Signed-off-by: asto <asto18089@126.com>
@asto18089

Copy link
Copy Markdown
Collaborator Author

感谢评审,P1 成立,已在 57ae408d9 修复。

修复内容:replay 闸的 legacy 分支从 None => true 改为 fail-closed——Custom tag(legacy root custom 与命名表 custom/<table>,即 reasoning_provider_tag 可能铸造的全部 Custom 身份)缺少 fingerprint 时不再重放:没有来源证明,就不该把旧 blob 送上重指向的 endpoint。评审建议的固定 endpoint provider 例外照单采纳:内建 provider 的 base URL 来自静态目录、env 只带 key,其 legacy state 不可能来自别处,因此保留 None => true 以保住旧会话的 reasoning 连续性。Custom 侧的升级代价是第一轮丢失 reasoning 连续性,新 capture 带上 fingerprint 后即恢复。

测试:按评审要求把 legacy Custom 断言翻转为「请求体不得含 reasoning item」(变异验证:把谓词砍回 tag == "custom" 时恰好这条 pin 变红);补了 legacy-root custom tag 的同款 pin,以及固定 endpoint(Codex)legacy state 继续重放的正向 pin。

验证:tui 11948 / core 82 全绿(一次 remote_control 全量并发 flake,与此前轮次相同,重跑即绿);workspace clippy -D warnings 净;fmt 净。

正文已加 Round-4 段并标注 round-3 中被本修复取代的「legacy keeps replaying」表述。

@JensenChen28 JensenChen28 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

当前修复已让 fingerprint-less Custom state 正确 fail closed,原始跨 table/base_url 缺口在 Custom 路径上已解决。但同一判断把所有非 Custom tag 当作固定 endpoint,而配置层明确允许内置 provider 使用 custom endpoint,因此仍有同类跨 endpoint 重放缺口。必需 check 也仍在运行。

// fixed-endpoint provider's URL cannot have
// changed and its old sessions keep replaying.
let endpoint_matches = match &state.endpoint {
None => !is_custom_reasoning_tag(&state.provider),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P1] None => !is_custom_reasoning_tag(...) 仍会把旧 state 发往已改址的内置 provider。 内置 tag 并不等于固定 URL:配置支持 OPENAI_BASE_URL、OPENAI_CODEX_BASE_URL,Config::provider_uses_custom_endpoint 也明确处理这些路由;OpenaiCodex 构造器甚至按 custom endpoint 切换凭据。于是 fingerprint-less openai-codex(或其他 Responses provider)state 在 base URL 改成另一个网关后仍返回 true,测试还把该行为固定为兼容性。请根据当前请求是否使用可变/custom endpoint 决定 legacy policy,而不是从 provider tag 推断;无法证明 URL 固定时应 fail closed。至少补一条 OpenAI Codex custom-base-url 改址的回归,断言旧无 fingerprint state 不进入请求。

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants