Repository navigation
Conversation
…ng the root The Messages adapter unconditionally ran the Responses-style root flatten (strip oneOf/anyOf/allOf/enum/not at the root, merge branch properties) on every tool's input_schema, commented as stripping shapes "which Anthropic rejects". The 2026-11-05 document check finds no such rejection: the tool-use guide defines input_schema as "A JSON Schema object defining the expected parameters for the tool" (platform.claude.com/docs/en/agents-and-tools/tool-use/ implement-tool-use) with no root-composition restriction, the structured-outputs strict-restrictions list names none either, and DeepSeek's Anthropic-compatible line documents input_schema as fully supported. The rewrite had no document basis and silently altered declared tool semantics, so the unconditional call is removed: input_schema reaches the wire exactly as declared, the synthesized constraint-note plumbing goes with it, and the provider-neutral registry sanitizer upstream is unchanged. strict passthrough is untouched. One shared builder change covers all four Messages lanes (Anthropic, DeepseekAnthropic, MinimaxAnthropic, Modelstudio Anthropic). Signed-off-by: asto <asto18089@126.com>
REQUEST_CHANGES — fresh multi-lane reviewFresh 8-lane review (pipeline map, history/upstream, scope/claims audit, local gates, live doc verification, mutation execution, adversarial falsification of the blocking finding). Rebase: no-op verified — head The diff quality is good — but the removal's core premise conflates document basis with behavioral basis, and it re-introduces a recorded live failure mode on the genuine Anthropic lane. MAJOR-1 — Removing the root-composition strip re-exposes a recorded live HTTP 400 on the genuine Anthropic laneThe three document claims are literally accurate (verified fetched 2026-11-05: the tool-use guide's "A JSON Schema object defining the expected parameters for the tool"; the structured-outputs strict-restrictions list names no root-composition ban; DeepSeek's Anthropic-compat page says
Consequences on
The probe declaration says no live probes were used, so there is also no evidence the current endpoint accepts root unions. As merged, this trades "silently altered contract" for a recorded hard request failure on the flagship provider. Suggested directions (pick one):
MINOR-1 — "Schemas without root compositions are byte-identical to before" is overstated (body + new comment)The removed call also stripped MINOR-2 — MCP framing"The provider-neutral registry sanitizer that runs upstream of this adapter is unchanged" holds for the native registry but not for MCP tools, which reach MINOR-3 — Test pins only root
|
…ction Ship input_schema verbatim on every Messages lane except the one whose validator is recorded rejecting root compositions. The genuine Anthropic endpoint has returned HTTP 400 "input_schema does not support oneOf, allOf, or anyOf at the top level" since at least 2025-07 (f8e7573 quotes a live failure; CHANGELOG 0.9.1; anthropics/claude-code#4886 and #30212 report the identical server-side error, neither marked fixed), so ApiProvider::Anthropic now narrows exactly those three root keywords via sanitize_for_anthropic_messages: branch properties merge into the root, the dropped exactly-one / at-least-one constraint becomes a description note, and everything the record does not name — root enum/not, dependentRequired/dependentSchemas, the declared root type, non-object roots — ships verbatim. DeepSeek-Anthropic, MiniMax- Anthropic, and Model Studio Anthropic lanes keep the verbatim shipping: DeepSeek documents input_schema as fully supported and the others document no restrictions (fetched 2026-11-05). For apply_patch and finance, whose root unions triggered the old flatten, the Anthropic-lane wire bytes are identical to the previous release line; schemas without root compositions change only where the old flatten deleted unrecorded keywords. Pins: the three non-Anthropic lanes ship a root oneOf verbatim with no note; the Anthropic lane strips the union, merges branch properties, notes an empty description, and preserves enum/not/dependent keywords and a non-object enum root byte-identically. Module-level pins cover the sanitizer directly. Signed-off-by: asto <asto18089@126.com>
Fix wave pushed (head
|
Fresh re-review (2026-11-06, 9 lanes, head
|
Fresh-review follow-ups on the narrowed Anthropic input_schema pass: - Pin the shared sanitize() anchor inside sanitize_for_anthropic_messages: it is load-bearing (the previous release line collapsed root nullable unions through it) and deleting it left every new test green while a root anyOf:[X, null] would ship the recorded-rejection union shape. - Key the narrowing gate on the first-party endpoint, not provider identity alone: a custom wire=anthropic provider pointed at api.anthropic.com now gets the same recorded-rejection protection the thinking gates apply via is_native_anthropic_base_url, and a gateway base URL still ships verbatim; both pinned at the adapter level. - Correct the CHANGELOG receipt (0.9.0, not 0.9.1) and carve the shared registry normalization out of the ships-verbatim doc claims. Signed-off-by: asto <asto18089@126.com>
|
Fix wave pushed: MAJOR-1 (sanitize anchor unpinned) — fixed. New module test M-1 (gate predicate) — fixed. The gate is now M-2 (body accuracy) — fixed. PR body now cites CHANGELOG §0.9.0 and enumerates the full verbatim set (Openmodel, the OpencodeZen Message-capable route, dual-wire M-4 (commit-1 bisect hazard) — left as-is (2-commit history, commit 2's message accurately describes the net tree; squashing pushed history for this was judged not worth it). NITs — the in-body comment was rewritten while fixing M-1/M-2 and now carries the shared-normalization carve-out; the test-shape overlap between adapter and module levels is kept deliberately (the adapter test uniquely pins gate wiring + description passthrough). Local verification on |
Round-3 fresh-review follow-ups on the narrowed Anthropic input_schema pass, each found by a dedicated review lane: - Replace a root that is not a JSON object at all (malformed MCP or server output) with an empty object, mirroring sanitize_for_responses: the API defines input_schema as a JSON Schema object, and shipping the raw value verbatim re-exposed the whole-request HTTP 400 failure class this pass exists to prevent. Declared non-object schemas (a string-enum root) still ship verbatim. - Restore type:"object" when stripping a root composition leaves a populated object schema whose root declared no type: the previous release line always forced the type, and the merge otherwise ships a typeless schema that invites non-object arguments. - Pin the explicit post-merge prune (a single-branch oneOf spreads its required groups after sanitize's own prune ran), add the missing root-allOf strip/merge/note fixture, pin the no-injection negative, and make the dependent-keyword fixtures well-formed with exact assertions instead of is_some() checks. - Correct the remaining stale CHANGELOG 0.9.1 citations (the rationale is recorded under 0.9.0) and document the replacement and type restore in the module doc, the pass doc comment, and the adapter call-site comment. Signed-off-by: asto <asto18089@126.com>
Round-3 fresh review — 11 lanes, from scratch (head now 78da100)Third from-scratch review of this PR (prior rounds' conclusions treated as reference only). Rebase first: The four review questions1. Actually useful, root-caused, complete — YES, re-verified firsthand today.
2. Elegant, no reinvention — YES. The new pass reuses 3. Smuggled changes — NONE. Full diff touches exactly two files and every hunk serves the new pass, the gate, or their tests. Commit-1's message asserts the conclusion the PR later retracted (transient bisect hazard only — this repo squash-merges, so intermediates never ship). 4. Bug-free — one MAJOR found and fixed; the rest MINOR/NIT.
Differential harness result (old
|
| input | identical? |
|---|---|
| apply_patch / finance (real schemas) | byte-identical incl. notes |
single-element allOf, nested compositions |
identical |
composition + root enum/not/dependent* |
intended divergence (kept verbatim now) |
| non-object-branch compositions | intended divergence (no forced type:"object" / empty schema) |
| untyped composition root with property branches | identical after the type-restore fix |
Mutation ledger (this round)
All nine executed mutations red on their pins: gate-off, sanitize_for_responses swap, note-drop, endpoint-clause-drop, shared-sanitize()-anchor-drop, merge-skip (round-2 set, re-verified in round 2 and pins re-checked), plus this round's prune-removal, guard-removal, and injection-polarity flip (each red on exactly its pin; negative pins stay green).
Verified-sound notes
- Performance strictly same-or-better: verbatim lanes drop both full-tree walks; the narrowing lane runs one full walk vs two at base; clone count unchanged; note generation is deterministic (fixed keyword order, stable sort) so prompt-cache keys don't flake; single per-request call site, no hot-loop exposure.
- Local gates: fmt clean, 72 schema_sanitize + 42 client::anthropic tests green; clippy shows only pre-existing base items (byte-identical files); CI green on 698c41e and re-running on 78da100.
- Accepted disclosures (unchanged by this round): degenerate declared schemas (e.g. a root
oneOfof two string enums) now ship as their post-normalization self instead of a forced{type:"object",properties:{}}; verbatim lanes' third-party validators are undocumented territory by design; the gate keys on provider identity or endpoint, so a provider-Anthropic proxy override narrows (conservative, matches the previous release line) while a custom provider at the same gateway ships verbatim.
No-Issue: review round and follow-ups confined to the PR's own scope (records-grounded hardening of the same two files); the repository has issues disabled.
Round-4 fresh review — 12 lanes, from scratch (head 78da100; 3 MAJOR found)Fourth from-scratch review; all prior-round conclusions treated as reference only, every finding below re-derived this round. Rebase first: The four review questions1. Actually useful, root-caused, complete — YES for the recorded scope, with one completeness gap (M1).
2. Elegant — YES. A strictness-enum parameterization of 3. Smuggled changes — NONE. The diff is 5 hunks in exactly the two declared files; every hunk classifies as required-by-purpose or supporting pin. 4. Bug-free — not yet: 3 MAJOR.
Executed mutation ledger (this round, dedicated worktree)All 8 body-claimed mutations re-executed and red exactly as claimed: gate-off (C2+C4), Differential harness result (executed: exact base pipeline vs head pipeline)
The body's byte-identity claim for MINOR / advisory
Verified sound (no action)Performance strictly improves (narrow lane 2 full walks → 1; verbatim lanes 2 → 0; clone profile unchanged; no new recursion; 1000-branch merge is linear; deterministic note/merge ordering via fixed keyword array + Verdict: 0 BLOCKER / 3 MAJOR / ~9 MINOR. The three MAJORs are small, well-scoped fixes squarely inside the PR's own direction (a two-line condition extension, two one-fixture pins, one disclosure/decision); happy to re-verify a fix wave. No-Issue: review round confined to the PR's own scope (records-grounded hardening of the same two files); the repository has issues disabled. |
Round-4 review fix wave: - A root composition whose branches carry no properties (an allOf of $refs, required-only or boolean branches) merged nothing, and type restoration required non-empty merged properties, so the narrowing lane shipped a constraint-free typeless schema with usually no note — the model-facing contract lost its object shape, which the pass's own doc names as the reason restoration exists. The object type is now restored for every stripped composition, and when the merge contributes no properties the dropped constraint is surfaced as a generic keyword-specific note. Pinned (allOf-of-$refs and boolean branches). - Two pin gaps closed: the gate's identity disjunct (an Anthropic-identity client at a proxy base URL asserted to narrow) and the root-only strip scope (a nested oneOf under properties asserted to survive). - Pins for the remaining round-4 findings: verbatim lanes ship a schema the registry would normalize byte-identically (no normalization on the else branch), root properties win over same-named branch properties, the note keyword precedence is pinned, Openmodel joins the verbatim lane loop, and apply_patch/finance byte-identity with the previous release line is now a committed test. Signed-off-by: asto <asto18089@126.com>
Round-4 fix wave pushed — fd2d309All round-4 MAJORs fixed plus the actionable MINORs; every new pin mutation-verified. 120/120 across MAJOR-1 — object floor for property-less compositions (fixed)A root composition whose branches carry no properties (an MAJOR-2 — both pin gaps closed
MAJOR-3 — gateway-fronting-Claude class (decision: disclose)Adopted the disclosure option rather than narrowing all MINOR pins added (each mutation red on exactly its pin)
Mutation ledger for the fix wave (all executed, each red on exactly its target pin unless noted): (1) restoring the Not adopted (with reasons)
No-Issue: fix wave confined to the PR's own two files and the review findings; the repository has issues disabled. |
Round-5 fresh review — 8 lanes, from scratch (head
|
| Mutation | Site | Named pin | Result |
|---|---|---|---|
gate → false |
anthropic.rs:117-118 |
anthropic_lane_narrows_the_recorded_root_composition_keywords |
RED |
narrow pass → sanitize_for_responses |
anthropic.rs:126 |
anthropic_lane_keeps_unrecorded_keywords_verbatim |
RED |
drop sanitize(schema) call |
schema_sanitize.rs:166 |
anthropic_messages_sanitize_keeps_the_shared_registry_normalization |
RED |
| gate drops endpoint clause | anthropic.rs:117-118 |
custom_anthropic_wire_clients_narrow_by_first_party_endpoint |
RED |
type floor gated on merged_properties |
schema_sanitize.rs:188 |
anthropic_messages_sanitize_restores_the_object_floor_for_property_less_compositions |
RED |
| verbatim branch runs narrow pass | anthropic.rs:135-137 |
non_anthropic_messages_lane_ships_a_schema_the_registry_would_normalize |
RED |
note join "\n\n" → "\n" (adversarial) |
anthropic.rs:130 |
proxy + narrowing tests (exact-eq) | CAUGHT |
swap find order (adversarial) |
schema_sanitize.rs:176 |
— | ESCAPED → MINOR-9 |
| prune before merge (adversarial) | schema_sanitize.rs:191 |
— | behavior-neutral (dead mutation, provably equivalent) |
drop !properties.is_empty() (adversarial) |
schema_sanitize.rs:184-187 |
— | ESCAPED → MINOR-9 |
All six PR-claimed pins bite — genuine test failures, not compile errors. The two escape claims rest on caller-completeness evidence (both call sites of the pass enumerated; the mutated binary run over the complete caller set — client::anthropic:: + schema_sanitize:: + dialect_seam, 130 tests — stays green), not a completed full-suite run.
Local gates (own target dirs; two independent dirs agreed on clippy)
cargo fmt --all -- --check— PASScargo test -p codewhale-tui --libscoped:anthropic_messages17/17,schema_sanitize77/77 (full module 76/76 + 1 name overlap),client::anthropic44/44 — all PASS, no cross-test interference- dead-code budget 397/420, locale parity 14×2101/2101, provider registry / crate boundaries / command manifest / README translations — all PASS
- clippy: only the 2 pre-existing base errors above (see NOTE-2)
Convention compliance
CHANGELOG absence is per policy — CONTRIBUTING.md:191-197 mandates changelog entries written on main at merge time ("A PR that carries changelog hunks will be asked to strip them"), confirmed by recent merged PRs (#131/#133/#134/#135) and by the f8e7573 precedent (entry landed a day later via the release-manager commit). Tests colocated per CONTRIBUTING.md:49-52, matching sibling adapter practice (ab6c68a). Fork-register deferral to the parent gitlink-advance is exactly what the parent fork-policy.md §3/§4 prescribes; no CodeWhale-side artifacts required.
Review-criteria summary
- Useful + complete + root-caused: yes — the unconditional flatten (itself record-triggered for only three keywords, extrapolated beyond them per the receipts) is replaced by record-scoped narrowing on the lane with the recorded rejection; every claim audited maps to committed code/tests; the round-4 gaps are closed and pinned.
- Elegant: shared helper layer reused (
sanitize,merge_root_composition_properties,root_composition_constraint_note,prune_dangling_required); the new pass is a thin orchestrator; honest duplication guarded by the cross-pass byte-equality test; no wheel reinvention. - Smuggling: CLEAN — exactly 2 files, deletions exactly the old flatten block + one doc line, DCO 5/5, every added line described in commit messages, no weakened pre-existing tests, no tautological pins.
- Bug-free: no blocker/major found across the eight lanes including 14 empirical sanitizer repros; the nine MINORs are edge-shape, model-facing-copy or disclosure items, none affecting the recorded tools or the common path.
Verdict: 0 BLOCKER / 0 MAJOR / 9 MINOR / 2 NOTE — approve direction. MINOR-1 (one body sentence) and MINOR-3 (one pin) are the two I'd most want before merge; MINOR-9 is two one-fixture pins if you want the note machinery fully pinned in-wave; the rest are optional follow-up material.
Problem
The Anthropic Messages adapter unconditionally ran a Responses-style root flatten on every tool's
input_schema. Two independent records frame the fix:Anthropic API error (HTTP 400 … invalid_request_error): tools.1.custom.input_schema: input_schema does not support oneOf, allOf, or anyOf at the top level— and CHANGELOG §0.9.0 records the same rationale. Externally, [BUG] input_schema does not support oneOf, allOf, or anyOf at the top level anthropics/claude-code#4886 (2025-07) and #30212 (2026-03) report the identical server-side 400, both closed as not planned without ever being marked fixed; the same 400 recurs in #84056 (2026-08, closed not planned) and root-levelallOfis still reported silently dropped in #95504 (open, 2026-09). Upstream still defends this lane today (d29abf3, 2026-11-03,RouteToolProjection::Relaxedretainssanitize_for_responses).input_schemasimply as "A JSON Schema object defining the expected parameters for the tool"; the structured-outputs strict-restrictions list names no root-composition ban; the recorded 400 names onlyoneOf/allOf/anyOf, neverenum/not; DeepSeek's Anthropic-compatible line documentsinput_schemaas "Fully Supported" (api-docs.deepseek.com/guides/anthropic_api); MiniMax and Model Studio document no restrictions.The first revision of this PR removed the flatten on every lane, which re-exposed the recorded failure mode on the genuine Anthropic lane —
apply_patch(rootoneOf) andfinance(rootanyOf) are model-visible built-ins, and MCP schemas carry no registry sanitization upstream of this adapter. This revision keeps the verbatim goal and scopes the exception to the lane with a recorded rejection.Changes
sanitize_for_anthropic_messages(crates/tui/src/tools/schema_sanitize.rs) narrows exactly the recorded keywords: strips rootoneOf/anyOf/allOf, merges branch properties into the root, and surfaces the dropped exactly-one / at-least-one constraint as a description note. Unlike the old Responses-style flatten it preserves rootenum/not,dependentRequired/dependentSchemas, the declared root type, and declared non-object roots — none are in any record. Where a stripped composition leaves a root with no declaredtype, the object type is restored (the old line always forced it) — unconditionally, so compositions whose branches carry no properties (allOfof$refs, required-only or boolean branches) still keep the object floor; when the merge contributes no properties at all, the dropped constraint is surfaced as a generic keyword-specific note so the model is not left without guidance for a fully erased argument contract. A root that is not a JSON object at all (malformed server or MCP output) is replaced by an empty object exactly as the Responses pass does, since the API definesinput_schemaas a JSON Schema object. The shared registry normalization (sanitize: nullable-union collapse, bare-objectpropertiesinjection, dangling-required pruning) still applies first, and the doc comment carves that out explicitly.build_anthropic_body(crates/tui/src/client/anthropic.rs) applies that pass on the first-party lane:ApiProvider::Anthropicor a base URL onapi.anthropic.com(is_native_anthropic_base_url— the same endpoint predicate the adaptive/redacted-thinking gates in the same function already use), so a customwire="anthropic"provider pointed at the genuine endpoint gets the same recorded-rejection protection; provider identity stays in the gate beside the endpoint becauseANTHROPIC_BASE_URLoverrides commonly front the genuine validator through a proxy. Everything else shipsinput_schemaverbatim: the DeepSeek-Anthropic, MiniMax-Anthropic, and Model Studio Anthropic lanes,Openmodel, theOpencodeZenMessage-capable route, dual-wire built-ins flipped byproviders.<id>.wire = "anthropic", and custom providers pointed at third-party gateways.Behavior
apply_patchandfinanceship byte-identical to the previous release line, notes preserved (executed differential harness, now also a committed test). For arbitrary schemas the wire intentionally diverges where the old flatten acted without a record: only rootoneOf/anyOf/allOfare stripped (branch properties merged, constraint note kept — or a generic note when the branches carry no properties); rootenum/not, tree-widedependentRequired/dependentSchemas, and declared non-object roots now ship verbatim; a non-object root (array/scalar) is still replaced by an empty object; a stripped composition leaving a schema with no declared root type getstype:"object"restored. MCP schemas on this lane keep thesanitize()normalization they had before; it remains their only normalization.input_schemaships exactly as declared, including root unions. Providers that reject such shapes surface their own provider error instead of a quietly altered contract.strictpassthrough untouched; the provider-neutral registry sanitizer for native tools unchanged; Responses/xAI/Kimi passes untouched.Test evidence
non_anthropic_messages_lanes_ship_input_schema_verbatim_without_root_flattening: a rootoneOfships verbatim with no note on the DeepSeek, MiniMax, Model Studio, and Openmodel lanes.non_anthropic_messages_lane_ships_a_schema_the_registry_would_normalize: a bare object (a shape the sharedsanitize()would mutate) ships byte-identically on a verbatim lane, pinning that the else branch runs no normalization at all.anthropic_lane_narrows_the_recorded_root_composition_keywords: union stripped, branch properties merged, note appended, empty description replaced by the note alone.anthropic_identity_narrows_even_on_a_proxy_base_url: anApiProvider::Anthropicclient at a non-anthropic.combase URL still narrows — the only behavior distinguishing the identity disjunct from the endpoint clause.anthropic_lane_keeps_unrecorded_keywords_verbatim: root type,dependentRequired/dependentSchemas, rootnot, and a non-objectenumroot all ship byte-identically on the Anthropic lane.custom_anthropic_wire_clients_narrow_by_first_party_endpoint: a customwire="anthropic"provider atapi.anthropic.comgets the narrowing; the same provider shape at a third-party gateway ships verbatim.allOf-of-$refs and boolean branches), nested-composition preservation (root-only strip scope), root-property-wins merge precedence, the note keyword precedence under a dual composition, the no-injection negative, non-schema-root replacement (array/scalar →{}), the explicit post-merge required prune (single-branchoneOfcollapse corner), a rootallOfstrip/merge/note fixture, the shared-registry anchor (nullable-union collapse, bare-object injection, dangling-required pruning), and a committed previous-release-line equality test running the realapply_patch/financeschemas through both pipelines (registry pre-pass included,Value+ serialized-bytes + note equality).sanitize_for_responsesin place of the narrow pass fails the unrecorded-keywords test; dropping the sharedsanitize()call fails the registry-anchor test; dropping theis_native_anthropic_base_urlclause fails the custom-endpoint test; note-order reversal fails the exact-note assertions; removing the explicit post-merge prune, the non-object-root replacement, or the scoped type restoration each fails its own new pin while the negative pins stay green. Fix-wave mutations, each red on exactly its new pin: restoring themerged_propertiesconjunct fails the object-floor test; flipping the generic-note condition reds the object-floor and merge tests; widening the strip tree-wide fails the nested-composition pin; dropping the identity clause fails the proxy-base-URL test; normalizing the verbatim else-branch fails the registry-would-normalize test; last-wins merge precedence fails the root-property pin; swapping the note keyword order fails the dual-composition pin.Known limitation: verbatim lanes that front genuine Claude
The verbatim rule is grounded in each provider's own documentation, but a
wire="anthropic"lane can transitively front the genuine Anthropic validator: a custom provider pointed at a third-party gateway, or a gateway lane (Openmodel, OpencodeZen) serving claude-family models upstream. On such a lane a root union in a tool schema — including the every-turnapply_patch(rootoneOf) andfinance(rootanyOf) built-ins — can surface the recorded rejection from the upstream validator where the previous release line silently flattened it (observed in the wild as client → one-api-style gateway → Claude upstream → rootanyOf→400 upstream_error). Doc-silence is the operative evidence on these lanes — DeepSeek's own endpoint has recorded rejections of other schema keywords (regex escapes,type: "custom") — so the trade-off is explicit: raw provider errors name the exact tool and keywords and are actionable, while narrowing every lane would re-alter contracts on providers that genuinely support unions. If this bites in practice, extending the recorded-rejection narrowing to everywire="anthropic"lane is a self-contained follow-up (a narrowed schema is valid for any upstream).Merge-conflict surface with #137
Still edits
crates/tui/src/client/anthropic.rsandcrates/tui/src/tools/schema_sanitize.rsonly; nochat.rsor configuse-block changes, so no conflict surface with #137.Fork-vs-upstream note
Upstream flattens every Messages lane (
RouteToolProjection::Relaxed, which additionally strips rootenum/notand forcestype:"object"). This branch matches upstream behavior on the genuine Anthropic lane for the recorded keywords and diverges only on lanes whose providers document full support or no restrictions; the parent repository's fork register will need this noted when the gitlink advances.Probe declaration
No live probes were used. The first-party-lane narrowing is grounded in the recorded in-repo and upstream receipts above; the verbatim lanes are grounded in the three fetched documents. If a live probe later shows the genuine endpoint now accepts root unions, the lane gate can be dropped in a follow-up.
No-Issue: provider compatibility batch grounded in recorded provider behavior and the published provider docs (fetched 2026-11-05, re-verified 2026-11-06); the repository has issues disabled.