Skip to content

fix(tui): hold DeepSeek strict shaping to documented rules - #147

Open
asto18089 wants to merge 7 commits into
pinvou3-cleanfrom
fix/deepseek-strict-doc-conformance
Open

asto18089 wants to merge 7 commits into
pinvou3-cleanfrom
fix/deepseek-strict-doc-conformance

Conversation

@asto18089

@asto18089 asto18089 commented Nov 5, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Three conformance gaps in the DeepSeek strict tool pipeline: (1) optional properties were kept by manufacturing an undocumented nullable: true keyword; (2) non-numeric const shipped although the strict documents support const for number/integer schemas only; (3) the whole pipeline acted on any route whose provider was typed DeepSeek, so a misconfigured provider/base_url entry could receive DeepSeek-only reshaping that strips keywords the other provider's own documents support.

Document evidence (fetched 2026-11-05)

DeepSeek strict function-calling contract — the "strict Mode (Beta)" section of https://api-docs.deepseek.com/guides/tool_calls/, fetched 2026-11-05 (strict mode is documented for tools only; there is no documented response_format strict contract). Verbatim anchor points: the strict subset requires every property listed in required and additionalProperties: false ("All properties of every object must be set as required..."); the documented supported-type list includes enum and anyOf, const is documented only for number/integer schemas, and neither null nor a nullable keyword appears anywhere in the strict-mode documentation; default appears (under number/integer) with no ban; format is documented with an enumerated set (email, hostname, ipv4, ipv6, uuid); the exclusiveMinimum/exclusiveMaximum forms shown are numeric. The official hosts also cover the family's api.deepseeki.com alias — not because the docs bless it (they never mention it; docs/REBRAND.md records it as a legacy typo host kept only in URL heuristics for existing configs, codewhale-hq#1079) — but to mirror the compat set the rest of the codebase already recognizes (client.rs, client/chat.rs, the doctor).

The same section's usage step says "In the tools parameter,all function need to set the strict property to true" and its validation paragraph says "If the schema does not conform to the specifications or contains JSON schema types that are not supported by the server, an error message will be returned" — but the documents never state how the server treats a MIXED set (tools flagged strict: true beside tools shipped without the flag). This pipeline ships mixed sets by design: a tool whose schema cannot conform fails closed per tool (ship unchanged, no flag) while its siblings keep the flag and tool_choice follows the route gate. The server's mixed-set semantics (reject the request / request-wide downgrade / per-tool validation) are undispositioned by the documents; a one-shot live probe is the follow-up that would settle it, and the pipeline's discarded all_strict return is the lever for an all-or-nothing policy if one is ever needed (the call site carries that disposition in a comment).

Changes (head 1165fb30a, plus review fixes)

  1. Optional properties no longer manufacture nullable. A property the schema does not require cannot satisfy "every property listed in required" honestly, and nullable: true is a keyword outside the documented strict subset — so the property is removed from properties instead. Its name is reported through sanitize_for_strict, which prepare_tools_for_strict_mode surfaces as a description note listing the removed optional parameters. Removed subtrees are not reshaped or reported; their internals go with them.
  2. Non-numeric const lands on the documented enum keyword. A non-numeric const (string, bool, object, array) becomes enum: [v] (numeric const stays put; a non-numeric const beside an existing enum yields to it; a const: null is dropped outright — null is not a supported type, and landing it on enum would ship a null member). When the const is dropped beside an existing enum, the accepted values widen to the whole enum even if the const disagreed — every documented alternative is worse (narrowing the enum, or shipping an undocumented keyword), so this is deliberate and test-pinned. default keeps passing through verbatim.
  3. The pipeline is scoped to official DeepSeek-family routes. route_is_official_deepseek_family (config.rs) requires provider typing (Deepseek/DeepseekCN/DeepseekAnthropic) and an api.deepseek.com / api.deepseeki.com host with a strict host-boundary check; the engine's turn-surface build, the child sub-agent surface, and the doctor now all answer from one composition (strict_tool_mode_for_route). A misconfigured provider no longer receives DeepSeek-only reshaping that strips keywords its own documents support (OpenAI documents minItems/maxItems, for one). Because the flag flows out of the same surface policy, tool_choice: "required" and the preview surface follow the same gate.

Review fixes (same branch)

First round (410a5ca68): the doctor answers from the route gate (provider_mismatch for an official URL under a non-family provider, official_route for official-host non-canonical paths including the DeepseekAnthropic default /anthropic), config.rs citations corrected to the real contract location, and the two claimed-but-untested behaviors pinned (empty-description note replacement; const yielding to an existing enum).

Second round (10a09d5bb, fresh 2026-11-06 review):

  • Required nullables fail closed. A schema still carrying nullable or a null-type anyOf branch after the collapse pass cannot be represented faithfully under a strict flag, so that tool ships unchanged and non-strict instead of sending the undocumented keyword under strict: true (the exact silent-400 class this PR exists to prevent).
  • The schema walk no longer corrupts keyword-named properties. The recursion used to descend into the properties name map as if it were a schema, where the keyword strips and object rules rewrote property names: a required property named const was silently renamed to enum with a garbage schema while required still demanded const (every call invalid under additionalProperties: false), a properties property was erased with spurious schema keys injected into the map, and the removal note could name parameters that never existed. Property/definition schemas are now visited through their maps (properties, $defs, definitions), and literal keywords (enum, const, default, examples) ship verbatim.
  • dependentRequired tables are stripped like the other undocumented bounds keywords (patternProperties, minLength/maxLength, minItems/maxItems).
  • The built-in remember schema declares note required — execute rejects its absence for append and revise (the retire action ignores the value but the field stays required; see round three), and the all-optional schema left strict mode nothing to keep (the tool was callable only with an empty object).
  • Doctor vs wire: on canonical non-beta endpoints the Anthropic Messages wire forwards function.strict untranslated (the chat-completions strip does not apply there), so the DeepseekAnthropic family provider on such a URL is now reported official_route with the flag sent, and the beta-endpoint recommendation (a chat-completions surface) is withheld for that dialect.
  • The provider_mismatch / custom_endpoint messages now state that tool_choice falls back to auto — the only lever users lost when the gate stopped forcing it on out-of-scope routes.
  • New tests pin all of the above plus the engine turn-surface gate wiring across three route quadrants (in-scope / custom endpoint / non-family provider on an official URL).

Third round (76fefa87a, follow-up to the same fresh review — close the remaining conformance gaps the review found):

  • A removal that empties a tool fails closed. The strict contract removes every property the schema left optional. Action-discriminated tools (tasks, github, automation, web.run, validate_data, load_skill, harness) mark their whole root property set optional and dispatch on the values execute re-reads — under the removal rule they shipped strict: true callable only with an empty object, with a note telling the model never to send the parameters their runtime still demands. Such a tool now ships unchanged and non-strict, the same fail-closed rule as required nullables, with no removal note (nothing was removed from the schema that ships). Partially-optional tools keep the removal-and-note behavior; per-action schema honesty for sub-actions remains open as a follow-up (its scope after the fourth round is listed under Disclosed residual gaps).
  • Null-carrying shapes fail closed across encodings. The gate now rejects — alongside nullable and null-type union branches — type arrays (the documented subset's union mechanism is anyOf; a type array is a draft-4-style union the documents never describe), bare {"type":"null"} nodes, const: null, and enum members that are null: each is a required nullable wearing another keyword, and each ships undocumented constructs under strict: true today. MCP tool schemas reach this gate verbatim (McpPool::to_api_tools clones them unnormalized), so these encodings are externally reachable even though no built-in schema uses them.
  • Structural and conditional keywords fail closed like the MFJS path. not, if/then/else, contains, prefixItems, propertyNames, unevaluatedProperties/unevaluatedItems, and tuple-form items carry constraints the reshaping cannot express — the same family the Moonshot MFJS sanitizer rejects with UnsupportedKeyword. A schema relying on one now ships unchanged and non-strict rather than reshaped-and-strict.
  • Widening-only strips extended. Draft-7 dependencies (both the name-table and the schema form), uniqueItems, and the property/containment count bounds (minProperties/maxProperties/minContains/maxContains) strip like dependentRequired and the length bounds — dropping them only widens what the endpoint accepts; with every property required and additionalProperties: false the property-count bounds are redundant or contradictory anyway.
  • The documents' $def spelling is walked as a name map. The singular spelling the DeepSeek documents themselves use joins $defs/definitions, so a definition named after a stripped keyword (e.g. const) survives intact instead of being deleted or renamed on that path.
  • The route gate no longer trims whitespace. The doctor's URL classifier and the client's strict-host sets string-match the URL without trimming; admitting a padded URL at the gate alone would put the pipeline in scope on a route the wire does not canonicalize and leak the flag past the non-beta strip. A padded URL now degrades to out-of-scope (moved to the matrix's negative list).
  • Doctor: the Anthropic family answers from the wire on every official path. The Beta arm used to show the chat-dialect ready verdict for a deepseek-anthropic route pointed at https://api.deepseek.com/beta — the same dialect mismatch the second round fixed for non-beta URLs. The hoisted Anthropic branch now answers official_route there too (the Messages wire forwards the flag on its own dialect; the chat-surface recommendation stays withheld). The custom_endpoint message names https, and the provider_mismatch message calls api.deepseeki.com a recognized DeepSeek endpoint URL rather than an official one (its official status is a legacy-typo compat entry, per docs/REBRAND.md).
  • remember wording. The schema comment states the real contract: note is runtime-required for append and revise; retire ignores the value but the field stays required (otherwise strict mode would strip it).
  • New tests pin all of the above; the per-tool note tests gained a required property so they keep pinning the note path instead of the new fail-closed path.

Fourth round (705546c94 + 3b2dec71a, follow-up to the second fresh review of 2026-11-06):

  • The doctor's only actionable advice is no longer self-defeating. The fallback_non_beta recommendation was rendered through the authority-only URL redaction used for configured endpoints, so the recommended https://api.deepseek.com/beta displayed as https://api.deepseek.com — a URL where the chat wire still strips function.strict (chat.rs strip set). Following the advice landed the user straight back into the same verdict, and the JSON test pinned the stripped value. The recommendation is a built-in constant, never a configured URL, so the JSON report and the human output now emit it verbatim and the test pins the full https://api.deepseek.com/beta value.
  • update_goal and send_later fail closed by name. Their schemas keep one discriminating parameter required (status / action) while execute demands sibling parameters the schema marks optional — after the removal pass they shipped strict: true callable only with values the runtime rejects (update_goal for every accepted status value: evidence/verification for complete, blocker for blocked, verification for not_achieved, advisory for advisory; send_later for three of four actions: message for schedule, trigger_id for read/cancel). The documented strict subset cannot express those per-value requirements (if/then and root alternatives are outside it), and widening the schema requirements would make non-strict callers send junk for every other value — so the pipeline keeps an explicit name list (STRICT_UNUSABLE_TOOLS) with the runtime contracts cited at the definition, and census tests feed the tools' real schemas through the pipeline so the list cannot drift (the read-only send_later variant fails closed identically).
  • The compatibility gate judges the shape that ships. The gate now runs after the optional-removal pass on an enforced clone: a null-carrying shape (nullable, null-type union branch, type array, bare type:null, const: null, null enum member) under an optional property is faithfully represented by that removal, so the tool keeps strict mode and the property is removed with the standard note — previously one such parameter failed the whole tool closed (every Pydantic-style MCP schema with an optional X | null parameter). The same shapes under a required property still fail closed, per arm. The null-const and schema-form dependencies checks run before the enforce pass, because the strip destroys those constraints before a post-enforce gate could judge them.
  • A removal that empties a nested object fails closed — the root-emptied rule one level down. A required object whose every property is optional ships callable only with {} while the runtime still reads the removed members; a required bare {"type":"object"} parameter collapses to the same shape once sanitize injects the empty properties map (under additionalProperties: false that shape cannot carry a value). Authored no-argument roots keep the root-level exemption. Verified built-in-catalog impact: none (the only shipped empty-property roots are authored no-arg tools whose execute ignores input); the exposure is MCP-supplied schemas.
  • Widening-only strips extended again. format values outside the documented enumeration (email, hostname, ipv4, ipv6, uuid) are dropped — any other value ships a constraint outside the documented list — and draft-4 boolean exclusiveMinimum/exclusiveMaximum are dropped alongside (the documents list the numeric form only). Both removals only widen what the endpoint accepts.
  • Schema-form dependencies fails closed like dependentSchemas. Both spellings express the same construct; the strip used to drop the draft-7 form while the gate rejected the modern one. The schema form now fails closed (detected pre-strip), the name-table form keeps its widening-only strip.
  • Doctor wording follows the gate's own vocabulary. The official_route messages say "DeepSeek-family" rather than "official": the gate also accepts the legacy api.deepseeki.com compat host, which docs/REBRAND.md records as recognized but not official — the same distinction the provider_mismatch arm already drew.
  • The strict_tool_mode field doc no longer claims tool_choice falls back to auto when individual tools fail closed. tool_choice follows the route gate, not per-tool outcomes: it stays required on an in-scope route even when some tools ship non-strict, and falls back to auto only when the route itself is out of scope (the doctor's out-of-scope messages were already accurate).
  • Tests hardened against single-arm regressions. The null-family fail-closed tests are pinned per arm (a shared fixture let a single-keyword regression pass), the nine structural/conditional keywords are pinned individually, and the new behaviors above each carry positive and negative pins. Every new rule was mutation-verified locally: flipping the nested-emptied rule, the pre-strip detector, the format/exclusive strips, the name-list check, the doctor rendering, or the gate order each turns at least one test red.

Fifth round (1165fb30a, fixes from the 2026-11-09 fresh 9-lane review):

  • The compatibility gate walks name maps and literals like its siblings. The round-2 walk fix reached the walkers but not the round-3/4 gate: has_strict_incompatible_composition — the only one of the three walkers without the name-map and literal-keyword arms — judged properties/$defs entries as if they were schemas, so a REQUIRED property named after a gate keyword (nullable, contains, not, if/then/else, prefixItems, propertyNames, unevaluatedProperties/unevaluatedItems) or a $defs definition named not failed the tool closed on its name alone (the base only misfired on oneOf/allOf/dependentSchemas names; this widening was new in rounds 3–4 and is now fixed), and object-shaped enum members or a default value shaped like {"nullable": true} failed it on their shape. The gate now walks properties/$defs/definitions/$def as name maps and skips enum/const/default/examples as values; real keyword occurrences still fail closed (the per-keyword loop pins each one). Built-in-catalog impact: none — the exposure was MCP-supplied schemas only.
  • The nine structural/conditional keywords share one constant (STRICT_UNREPRESENTABLE_KEYWORDS) between the gate and the test loop that pins each of them, so the two lists cannot drift.
  • A root map shape fails closed. A root that declares no properties but takes arbitrary keys (additionalProperties in schema form or true) collapsed under the enforce pass into properties: {} + additionalProperties: false — strict: true callable only with {} while the runtime reads the keys (the same degenerate shape the nested rule already fails closed, one level up from it). Such a root now ships unchanged and non-strict; authored no-argument roots (additionalProperties absent or false) keep the exemption. The detection lives pre-enforce (strict_schema_supported, plus a has_unrepresentable_stripped_constraint arm): the enforce pass normalizes additionalProperties only on object schemas, so a typeless node carrying a non-false additionalProperties would have shipped the authored keyword verbatim under strict: true — it now fails closed too. Verified built-in-catalog impact: none (every built-in additionalProperties is false beside non-empty properties); the exposure is MCP-supplied schemas.
  • The doctor answers from the Responses dialect's wire. A route whose model resolves to the Responses endpoint (client/responses.rs hardcodes "strict": false per tool and tool_choice: "auto"; the resolver sends uncatalogued deepseek-v<digit> models — and the bundled deepseek-v4-flash row — there) got reshaped schemas with zero enforcement while the doctor reported ready/official_route with function_strict_sent: true. A new responses_dialect verdict (any family provider whose resolved endpoint key is responses, on any official URL) now says so: reshaping still applies, no strict enforcement is sent, tool_choice stays auto, and no base-URL recommendation is offered (the dialect is model-driven, not URL-driven — pointing the route at /beta does not change it). The verdict reads the same resolution the live client uses (DoctorApiTarget now carries the resolved endpoint key); exact catalog rows stay authoritative (deepseek-v4-pro → chat → ready on beta, test-pinned). This closes the same doctor-vs-wire class round two closed for the Anthropic dialect and rounds 3–4 refined for the chat dialect.
  • The mixed-strict-set reality is dispositioned. The discarded all_strict return now carries the disposition at its only production call site (see the Document evidence section): mixed sets are intentional per-tool fail-closed, tool_choice follows the route gate, and the return stays the lever for an all-or-nothing policy if a probe ever shows the server rejects or downgrades mixed sets. The census tests already pin the per-tool semantics (!prepare_tools_for_strict_mode(...)).
  • Body disclosures extended under Disclosed residual gaps: an additionalProperties schema form beside named properties is narrowed to false by the documented contract without a note (the only silent semantic loss in the pipeline — every other unrepresentable constraint gets a note or fail-closed), and landing a non-numeric const on enum is not purely conservative for bool/object/array members (the documents show string examples only; if the validator rejects such members the documented outcome is an error, where dropping the const outright would only widen).
  • New tests pin all of the above (required property named contains; definition named not beside a structural definition still failing closed; literal-shaped values; root map shapes including the additionalProperties: false exemption; typeless map keyword; required nested map; responses_dialect verdict + JSON; catalog-row precedence over the versioned rule), and the fixes are mutation-verified: reverting the gate's name-map arms turns the three keep-strict pins red (3 failed), removing the detector map arm, the root-map check, or the doctor's responses arm each turns exactly its own pin red.

Disclosed residual gaps (out of scope here, honestly stated)

  • Degraded-action tools in strict mode — schema-optional parameters that some sub-actions runtime-require, where the tool stays usable overall and the removal note names what not to send: Git (show needs rev, blame needs path), lsp (definition/references need line), pandoc_convert (binary targets need output_path), bash (the sandbox-escalation retry needs sandbox_permissions/justification), plus the hidden agents/message/agents/followup on strict-route transcript replay. remember's revise/retire remain strict-unreachable as before. Per-action schema honesty (or per-action fail-closure) for these remains the follow-up.
  • Unrecognized schema-map containers (e.g. OpenAPI components.schemas) are still walked as schemas — a pre-existing class, reachable only via pathological MCP input.
  • Annotation keywords (examples, title) ship verbatim, riding the same undocumented-but-tolerated-keyword reality the pipeline's own description notes rely on.
  • Landing a non-numeric const on a single-member enum is reasoned but not purely conservative for bool/object/array members: the documents show enum with string examples only, so if the strict validator rejects non-string members the documented outcome is an error, where dropping the const outright would only have widened. Deliberate, test-pinned, and called out here for completeness.
  • purge_context's hardcoded strict marker (purge.rs) is pre-existing and outside this pipeline (no reshaping, no tool_choice, fires regardless of the flag) — a separate concern.
  • An additionalProperties schema form (or true) beside named, required properties is silently narrowed to additionalProperties: false — the documented strict contract mandates it, but it is the pipeline's only silent semantic loss: every other unrepresentable constraint is either noted (removed optionals) or fails the tool closed. The named properties keep working; only extra-key acceptance disappears.

Test evidence (red-green)

CI on this branch: fork-ci check runs cargo nextest run --workspace --all-features --locked. The run on head 10a09d5bb (run 42074363684): 15,017 tests started, 15 skipped, 0 failed — check, gate, Gitleaks all SUCCESS. The run on head 76fefa87a (run 42192582794) is green: check, gate, Gitleaks all SUCCESS. The run on head 3b2dec71a (run 42232236062): 15,037 tests, 15 skipped, 0 failed — check, gate, Gitleaks all SUCCESS. Locally on 3b2dec71a, the affected filters are green: schema_sanitize (89 tests — nine new pins: nested-emptied fail-closed, required bare-object fail-closed, schema-form dependencies fail-closed plus its optional-position counterpart, per-arm null shapes, optional-position null shapes staying strict, the structural-keyword loop, per-arm null-const, undocumented format/draft-4 exclusive strips), strict_mode (22), strict_tool_mode_doctor (9 — the recommendation now pins the full /beta value), the census tests (strict_mode_fails_the_update_goal_tool_closed, strict_mode_fails_the_send_later_tool_closed with the read-only variant, strict_mode_keeps_remember_usable_with_its_note), tools::remember (10), tools::goal (28), tools::send_later (15), the route-gate matrix and composition tests, the engine turn-surface gate test, the strict filter (90) and the deepseek filter (159) across the lib, with cargo fmt --check and the gate's exact clippy invocation clean; the full workspace suite runs in CI (15,037 tests, 0 failed on this head). Six local mutations (nested-emptied rule, pre-strip detector, format/exclusive strips, name-list check, doctor JSON rendering, gate order) each turn at least one test red. On round five (1165fb30a): CI re-runs on the pushed head; locally the affected filters are green — schema_sanitize (95 tests: six new pins — required property named contains, definition named not beside a structural one, literal-shaped values, root map shapes with the additionalProperties: false exemption, typeless map keyword, required nested map), strict_mode (28), strict_tool_mode_doctor (11 — responses_dialect verdict + JSON and catalog-row precedence), the strict filter (98), tools::remember (10), tools::goal (28), tools::send_later (15), the full codewhale-tui lib (12,449 passed / 0 failed, excluding the two speech/voice modules that hang on this audio-less host) and every other workspace crate's lib suite green, with cargo fmt --check and the gate's exact clippy invocation clean. Round-5 mutations: reverting the gate's name-map arms → 3 red; detector map arm, root-map check, doctor responses arm → exactly 1 red each.

Behavior changes

  • Optional properties disappear from strict-mode DeepSeek tools (with an explicit description note) instead of gaining an undocumented nullable: true. A null-carrying shape under an optional property no longer fails the whole tool closed — the removal represents it and the tool keeps strict mode.
  • Tools whose whole root property set is optional now stay non-strict with their original schema (previously they shipped strict: true with every parameter removed — callable only with an empty object). A required nested object whose every property is optional — or a required bare {"type":"object"} parameter — now fails closed the same way. Partially-optional tools keep the removal-and-note behavior.
  • update_goal and send_later stay non-strict with their original schemas (previously they shipped strict: true while every — or three of four — of their dispatch values failed at runtime).
  • Required nullables: a tool whose schema needs nullable / null-type unions now stays non-strict with its original schema (previously it was marked strict with the undocumented keyword on the wire). The same fail-closed covers type arrays, bare {"type":"null"}, const: null, and null enum members in required positions, plus the MFJS-family structural/conditional keywords, a schema-form dependencies table, and a null const.
  • Non-numeric const ships as a single-member enum; a null const is dropped; dependentRequired, draft-7 dependencies name tables, uniqueItems, the property/containment count bounds, undocumented format values, and draft-4 boolean exclusiveMinimum/exclusiveMaximum are stripped.
  • Non-official routes typed as DeepSeek (or official DeepSeek hosts typed as another provider) no longer receive the strict pipeline, tool_choice: "required", or the strict preview; the doctor messages state the tool_choice fallback explicitly. Official DeepSeek-family routes keep strict mode; their wire schemas follow the strict contract exactly (optional parameters removed with a description note, non-numeric const landed on enum, null-carrying and structural shapes fail closed) — a wire change on those routes too, not a no-op. A whitespace-padded base_url degrades to out-of-scope instead of being admitted by the gate alone.
  • remember keeps its note parameter in strict mode and stays usable (append remains the default action); its other parameters are optional and are removed with a note — which leaves revise/retire reachable only outside strict mode; per-action schema honesty is a follow-up (see Disclosed residual gaps).
  • Doctor verdicts follow the gate: official beta → ready; official non-beta → fallback_non_beta (strict flag stripped, reshaping still applies; the recommendation now carries the full https://api.deepseek.com/beta path) — except the Anthropic-family provider, which is official_route on every official path, beta included (the Anthropic wire forwards the flag); official custom path → official_route (in scope); DeepSeek URL under a non-family provider → provider_mismatch; anything else → custom_endpoint (out of scope). The in-scope messages say "DeepSeek-family" rather than "official" (the legacy compat host is recognized, not official).
  • A tool whose required properties or $defs definitions are merely NAMED after gate keywords stays strict (rounds 3–4 failed it closed on the name alone); object-shaped enum members and schema-shaped default values likewise no longer trigger the gate.
  • A root map shape (additionalProperties schema form or true beside no properties) now ships non-strict with its original schema instead of strict: true callable only with {}; a typeless node with a non-false additionalProperties fails closed instead of shipping the authored keyword verbatim under a strict flag. Authored no-argument roots keep strict mode.
  • Doctor: a family route whose model resolves to the Responses endpoint now reports responses_dialect (reshaping without enforcement — the wire sends strict: false and tool_choice: "auto") instead of claiming the flag was sent; exact catalog rows keep their current verdicts.

Merge-conflict surface

With #137: this branch additionally touches tools/goal.rs and tools/send_later.rs (census tests only — no overlap with #137); #137's and this branch's appended test regions collide textually in core/engine/tests.rs (both add tests at the end of the module) — whoever merges second rebases; there is no semantic interaction. There is no conflict surface with #137's client/chat.rs use hunk, and the config.rs hunks do not collide. With #144: this branch rewrites lines inside #144's sanitize_for_strict hunk context and both branches add tests in overlapping regions — a merge-tree of this head onto #144's head auto-merges clean; the gate reshapes upstream of #144's sanitize_for_xai_parameters, compatible either merge order. With #143 (Moonshot MFJS): heavy same-file overlap in schema_sanitize.rs plus a disjoint config.rs doc hunk, but the MFJS pipeline is separate — a merge-tree of this head onto #143's head is clean. With #146: touches only client/anthropic.rs — zero file overlap with this branch (merge-tree clean). Round five additionally touches core/engine/tool_catalog.rs (comment only) and extends the lib.rs doctor and schema_sanitize.rs strict region — tool_catalog.rs is untouched by #137/#143/#144/#146, so the new surface does not collide; the schema_sanitize.rs overlap note with #143 still holds (round five stays inside the strict region; the MFJS region is untouched).

Probe declaration

No live probes were used in this change; every rule above is grounded in the published strict contract cited above (fetched 2026-11-05).

No-Issue: provider compatibility batch grounded in the published provider docs (fetched 2026-11-05); the repository has issues disabled.

…t and routes

Three conformance fixes, one pipeline.

Optional properties no longer manufacture nullable. The strict documents
support neither null nor a nullable keyword, so mark_nullable injected
an undocumented keyword to keep an optional property accepting omission.
The property is removed from the schema instead — preserving the strict
contract's all-required plus additionalProperties:false shape — and its
name is reported through sanitize_for_strict, which
prepare_tools_for_strict_mode surfaces as a description note listing the
removed optional parameters. Removed subtrees are not reshaped or
reported: a removed property's internals go with it.

Non-numeric const lands on the documented enum keyword. The strict
documents support const for number/integer schemas only; a
string/bool const now becomes enum:[v] instead of shipping an
undocumented keyword (numeric const stays put; a non-numeric const
beside an existing enum yields to it). default keeps passing through
verbatim — the documents show it and list no ban.

The whole pipeline now acts only on official DeepSeek-family routes.
route_is_official_deepseek_family (config.rs) requires provider typing
(Deepseek/DeepseekCN/DeepseekAnthropic) and an api.deepseek.com /
api.deepseeki.com host with a strict host-boundary check; the engine's
turn surface build and the child sub-agent surface apply it, so a
misconfigured provider no longer receives DeepSeek-only reshaping that
strips keywords its own documents support (minItems/maxItems on OpenAI).
Because the flag flows out of the same surface policy, tool_choice
required and the preview surface follow the same gate. The doctor's
custom-endpoint verdict flips accordingly (nothing strict is sent to a
custom endpoint), and the config field doc states the scope.

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

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

从零复审(8 lane + 亲证,head 908e1b414)

实质判定:三个改动方向与实现均正确,值得合并——门控完备(prepare_tools_for_strict_mode 唯一咽喉 tool_catalog.rs:886;ToolSurfacePolicy::new 全部 6 处调用点核过;tool_choice:"required"、preview、子代理面全部继承同一 gated flag,子代理客户端=克隆会话客户端,gate 与实际端点一致);无其他 nullable 伪造点;20 个 hunk 逐一映射到三条声明,零夹带;变换逻辑(移除先于递归、无累积、保序、缓存稳定)正确;测试真红绿;CI 全绿(nextest 实跑 15,004 passed)。

但有三组 P2 应在合并前修复:

P2-1 doctor 与新 gate 互相矛盾(本 PR 只改了 custom 分支)

doctor_strict_tool_mode_status(lib.rs:7503)的 Beta/NonBeta 分支只看 URL 不看 provider(known_deepseek_base_url_kind,6 个精确 URL):

  • provider=openai + https://api.deepseek.com/beta → doctor 报 ready/function_strict_sent: true,而新 gate 恰恰拦截这个组合(本 PR 自己的测试 config/tests.rs "a DeepSeek URL under a non-DeepSeek provider stays out of scope" 钉死了它越界)。PR 修复前 doctor 说法属实,修复后变成假话,且这正是 PR 的动机场景。
  • 反向同样失真:None(custom)分支新消息 "this custom endpoint keeps its tools unreshaped" 对官方 host 任意路径为假——包括 DeepseekAnthropic 的默认端点 https://api.deepseek.com/anthropic(config/models.rs:12):gate 放行并整形+tool_choice:"required",doctor 却说"未整形"。

建议:doctor 分支统一走 route_is_official_deepseek_family(config.api_provider(), &target.base_url)(官方路由内再按 beta/non-beta/custom-path 细分),并补 provider-mismatch 的 doctor 测试。

P2-2 正文/引用四处与事实不符(对文档合规主题的 PR 是硬伤)

  1. 引用源 https://api-docs.deepseek.com/guides/structured_output 页面不存在(SPA 回退到 "Your First API Call",sitemap 无此页;/guides/function_calling 同样不存在)。逐字核实的契约实际在 https://api-docs.deepseek.com/guides/tool_calls/ 的 "strict Mode (Beta)" 小节(且仅对 tools 记载——反而支持本 PR 前提;config.rs:985 的代码注释引用同样要改)。
  2. "Full suite green: 3198 passing" 与 CI 矛盾:run 41994057296 实为 15,004 passed(3198 是某测试的序号计数器)。
  3. "including the subagent-surface scoping tests" ——该测试不存在:subagent/tests.rs 唯一改动是 schema 级 fleet enum 测试,子代理路由 gate 无任何测试覆盖。
  4. "official DeepSeek-family routes behave exactly as before" 作为线上行为声明不成立(官方路由上可选参数移除+description 注记+const→enum 都是 wire 变化)。

另:api.deepseeki.com 在官方文档 0 命中,且本仓 docs/REBRAND.md:70 明言它是 legacy typo host、不是官方端点(仅为存量配置保留 URL 启发式)。config.rs:995 注释与正文的 "the official hosts also cover the family's api.deepseeki.com alias the rest of the codebase treats as official" 应改为"镜像代码库既有兼容接受集(REBRAND.md 定位为非官方、不作 fallback)"。

P2-3 两个被声称的行为没有测试钉住

  • 空 description 的注记分支(schema_sanitize.rs:69-71,note 整段替换 description)——现有测试的 tool 均有非空 description。
  • 非数值 const 让位于既有 enum 的分支(schema_sanitize.rs:344-346,注释与正文都声称 "yields to it")。

P3 备忘(不阻塞,跟进即可)

  • 值位递归污染(继承):enum/default/examples/$defs 里的 schema 形对象字面量被当 schema 节点处理——base 就在篡改,本 PR 使后果升级为删除+幻影名进注记;后续宜跳过 enum/default/examples 值的递归。
  • {"const": null} → enum:[null],null 在所引文档子集之外。
  • const+enum 冲突静默让位 vs 同文件 Kimi 路径 fail-closed(ConflictingLiteralConstraint)不对称,宜在注释说明取舍。
  • const 按值判数值而非按 schema 类型({"type":"string","const":3} 保留 const)。
  • 可选参数被移除这一用户可感知损失只写给了模型(注记),config 字段文档/doctor 未告知用户(严格模式是 opt-in,逃生门存在)。
  • gate 判定在 engine.rs:5727 与 subagent/mod.rs:12944 重复内联,宜提为 Config 方法。
  • 注记无长度上限(病态 schema 50+ 可选名会全量进 description)。
  • config.rs:3096 字段文档括号 "(api.deepseek.com hosts)" 漏了 gate 同样放行的 api.deepseeki.com。
  • :443/http:///无 scheme 等合法官方写法静默失去 strict(fail-closed 方向安全,仅 doctor 可见且无恢复建议)。
  • purge.rs:532 硬编码 strict: Some(true) 在 gate 外(既有,非本 PR 引入)。

配对父仓 PR(合并后必做)

strict 管线系上游继承(fd8c9fdb2),本 PR 是首个 fork 分叉,父仓 docs/fork-modifications.md 目前零登记。配对 PR 需:gitlink bump、fork-guard.sh EXPECTED_HEAD/EXPECTED_COMMITS 93→94、fork-modifications.md 新 T 条目、新测试(route-gate 矩阵 / optional-removal / const→enum)指纹与行为测试登记、./scripts/fork-guard.sh 实跑。


结论:按上述 P2-1/2/3 修复(改动面小)后即可合并。修复将由评审方直接推送到本分支。

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

从零复审(8 lane + 亲证,head 908e1b414)

实质判定:三个改动方向与实现均正确,值得合并——门控完备(prepare_tools_for_strict_mode 唯一咽喉 tool_catalog.rs:886;ToolSurfacePolicy::new 全部 6 处调用点核过;tool_choice:"required"、preview、子代理面全部继承同一 gated flag,子代理客户端=克隆会话客户端,gate 与实际端点一致);无其他 nullable 伪造点;20 个 hunk 逐一映射到三条声明,零夹带;变换逻辑(移除先于递归、无累积、保序、缓存稳定)正确;测试真红绿;CI 全绿(nextest 实跑 15,004 passed)。

但有三组 P2 应在合并前修复:

P2-1 doctor 与新 gate 互相矛盾(本 PR 只改了 custom 分支)

doctor_strict_tool_mode_status(lib.rs:7503)的 Beta/NonBeta 分支只看 URL 不看 provider(known_deepseek_base_url_kind,6 个精确 URL):

  • provider=openai + https://api.deepseek.com/beta → doctor 报 ready/function_strict_sent: true,而新 gate 恰恰拦截这个组合(本 PR 自己的测试 config/tests.rs "a DeepSeek URL under a non-DeepSeek provider stays out of scope" 钉死了它越界)。PR 修复前 doctor 说法属实,修复后变成假话,且这正是 PR 的动机场景。
  • 反向同样失真:None(custom)分支新消息 "this custom endpoint keeps its tools unreshaped" 对官方 host 任意路径为假——包括 DeepseekAnthropic 的默认端点 https://api.deepseek.com/anthropic(config/models.rs:12):gate 放行并整形+tool_choice:"required",doctor 却说"未整形"。

建议:doctor 分支统一走 route_is_official_deepseek_family(config.api_provider(), &target.base_url)(官方路由内再按 beta/non-beta/custom-path 细分),并补 provider-mismatch 的 doctor 测试。

P2-2 正文/引用四处与事实不符(对文档合规主题的 PR 是硬伤)

  1. 引用源 https://api-docs.deepseek.com/guides/structured_output 页面不存在(SPA 回退到 "Your First API Call",sitemap 无此页;/guides/function_calling 同样不存在)。逐字核实的契约实际在 https://api-docs.deepseek.com/guides/tool_calls/ 的 "strict Mode (Beta)" 小节(且仅对 tools 记载——反而支持本 PR 前提;config.rs:985 的代码注释引用同样要改)。
  2. "Full suite green: 3198 passing" 与 CI 矛盾:run 41994057296 实为 15,004 passed(3198 是某测试的序号计数器)。
  3. "including the subagent-surface scoping tests" ——该测试不存在:subagent/tests.rs 唯一改动是 schema 级 fleet enum 测试,子代理路由 gate 无任何测试覆盖。
  4. "official DeepSeek-family routes behave exactly as before" 作为线上行为声明不成立(官方路由上可选参数移除+description 注记+const→enum 都是 wire 变化)。

另:api.deepseeki.com 在官方文档 0 命中,且本仓 docs/REBRAND.md:70 明言它是 legacy typo host、不是官方端点(仅为存量配置保留 URL 启发式)。config.rs:995 注释与正文的 "the official hosts also cover the family's api.deepseeki.com alias the rest of the codebase treats as official" 应改为"镜像代码库既有兼容接受集(REBRAND.md 定位为非官方、不作 fallback)"。

P2-3 两个被声称的行为没有测试钉住

  • 空 description 的注记分支(schema_sanitize.rs:69-71,note 整段替换 description)——现有测试的 tool 均有非空 description。
  • 非数值 const 让位于既有 enum 的分支(schema_sanitize.rs:344-346,注释与正文都声称 "yields to it")。

P3 备忘(不阻塞,跟进即可)

  • 值位递归污染(继承):enum/default/examples/$defs 里的 schema 形对象字面量被当 schema 节点处理——base 就在篡改,本 PR 使后果升级为删除+幻影名进注记;后续宜跳过 enum/default/examples 值的递归。
  • {"const": null} → enum:[null],null 在所引文档子集之外。
  • const+enum 冲突静默让位 vs 同文件 Kimi 路径 fail-closed(ConflictingLiteralConstraint)不对称,宜在注释说明取舍。
  • const 按值判数值而非按 schema 类型({"type":"string","const":3} 保留 const)。
  • 可选参数被移除这一用户可感知损失只写给了模型(注记),config 字段文档/doctor 未告知用户(严格模式是 opt-in,逃生门存在)。
  • gate 判定在 engine.rs:5727 与 subagent/mod.rs:12944 重复内联,宜提为 Config 方法。
  • 注记无长度上限(病态 schema 50+ 可选名会全量进 description)。
  • config.rs:3096 字段文档括号 "(api.deepseek.com hosts)" 漏了 gate 同样放行的 api.deepseeki.com。
  • :443/http:///无 scheme 等合法官方写法静默失去 strict(fail-closed 方向安全,仅 doctor 可见且无恢复建议)。
  • purge.rs:532 硬编码 strict: Some(true) 在 gate 外(既有,非本 PR 引入)。

配对父仓 PR(合并后必做)

strict 管线系上游继承(fd8c9fdb2),本 PR 是首个 fork 分叉,父仓 docs/fork-modifications.md 目前零登记。配对 PR 需:gitlink bump、fork-guard.sh EXPECTED_HEAD/EXPECTED_COMMITS 93→94、fork-modifications.md 新 T 条目、新测试(route-gate 矩阵 / optional-removal / const→enum)指纹与行为测试登记、./scripts/fork-guard.sh 实跑。


结论:按上述 P2-1/2/3 修复(改动面小)后即可合并。修复将由评审方直接推送到本分支。

…laimed strict behaviors

Review fixes for the DeepSeek strict pipeline PR:

- doctor_strict_tool_mode_status now answers from
  route_is_official_deepseek_family (the same predicate the engine gate
  uses) instead of classifying the URL alone: an official DeepSeek URL
  under a non-family provider is reported as the new `provider_mismatch`
  status (strict_sent=false) instead of a false `ready`, and
  official-host custom paths — including the DeepseekAnthropic default
  api.deepseek.com/anthropic endpoint, which the gate reshapes — are
  reported as the new `official_route` status (strict_sent=true) instead
  of a false "keeps its tools unreshaped". The doctor icon match covers
  both statuses.
- Correct the config.rs citations: the strict contract lives in the
  "strict Mode (Beta)" section of api-docs.deepseek.com/guides/tool_calls
  (guides/structured_output does not exist), and api.deepseeki.com is a
  legacy typo host kept for config compatibility per docs/REBRAND.md,
  not an official host. The strict_tool_mode field doc now also states
  the on-route wire changes (optional-parameter removal + description
  note, const -> enum).
- Pin the two claimed-but-untested behaviors: the removal note replaces
  an empty tool description, and a non-numeric const yields to an
  existing enum (agreeing or contradicting).

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

Copy link
Copy Markdown
Collaborator Author

评审修复已推送(410a5ca68,承接上方评审意见的三组 P2):

  1. doctor 改为从 route gate 取答(doctor_strict_tool_mode_status 现以 route_is_official_deepseek_family 为前置判定):官方 URL + 非 DeepSeek 家族 provider → 新状态 provider_mismatch(不再假报 ready);官方 host 非规范路径(含 DeepseekAnthropic 默认端点 api.deepseek.com/anthropic)→ 新状态 official_route(不再假报 "unreshaped",如实反映整形 + tool_choice:"required" 生效、strict 标志按客户端行为转发);doctor 图标匹配同步覆盖两个新状态。补 provider_mismatch 与 official_route 两个 doctor 测试。
  2. 引用修正:config.rs 契约出处改为 guides/tool_calls 的 "strict Mode (Beta)"(原 guides/structured_output 页面不存在);api.deepseeki.com 措辞对齐 docs/REBRAND.md(legacy typo host、仅配置兼容保留,非官方端点);strict_tool_mode 字段文档补充路由内 wire 变化(可选参数移除 + description 注记、const→enum)。
  3. 钉住两个已声称未测行为:空 description 时注记整体替换(strict_mode_note_replaces_an_empty_tool_description);非数值 const 让位于既有 enum(strict_sanitize_non_numeric_const_yields_to_an_existing_enum)。

正文四处失实已同步修正(引用 URL、"3198 passing"→CI 实跑 15,004 passed/15 skipped、"subagent-surface scoping tests" 表述改为如实的覆盖说明、"official routes behave exactly as before" 改为准确 wire 变化描述)。

本地验证:cargo fmt --check 干净;靶向测试全绿(doctor 7/7、schema_sanitize 67/67、route-gate 1/1、fleet-enum 1/1);clippy 按 fork-ci allowlist(-D warnings + 五项 -A)通过。全量套件以 CI check(nextest workspace)为准。

Round-2 review found the strict pipeline still shipping undocumented
shapes and the doctor still disagreeing with one wire:

- Fail closed on required nullables: `has_strict_incompatible_composition`
  now rejects `nullable` and null-type `anyOf` branches at any depth.
  `sanitize` collapses the common `X | null` union into `nullable: true`,
  so the keyword is how a required nullable survives to the gate; such
  tools ship unchanged and non-strict instead of carrying an undocumented
  keyword under a strict flag.
- Never walk name maps or literals as schemas: `enforce_strict_subset`
  used to descend into the `properties` map itself, where the keyword
  strips and object rules rewrote property NAMES — a required property
  named `const` was renamed to `enum` with a garbage schema while
  `required` still demanded `const` (every call invalid under
  `additionalProperties: false`), a `properties` property was erased with
  spurious keys injected into the name map, and the removal note could
  name parameters that never existed. Property and definition schemas are
  now visited through their maps (`properties`, `$defs`, `definitions`),
  and literal keywords (`enum`, `const`, `default`, `examples`) ship
  verbatim.
- Strip `dependentRequired` like the other undocumented bounds keywords.
- Declare the `remember` tool's `note` parameter required — `execute`
  already rejects its absence for every writing action, and the
  all-optional schema left strict mode nothing to keep (the tool was
  callable only with an empty object).
- One route composition: new `strict_tool_mode_for_route` answers for the
  engine turn surface, the child sub-agent surface, and the doctor, so
  the surfaces cannot disagree. The doctor no longer claims
  `function.strict` is stripped for the Anthropic-family provider on
  canonical non-beta endpoints — the Anthropic Messages wire forwards it
  untranslated — and the out-of-scope messages now state that
  `tool_choice` falls back to `auto`.
- Tests: nullable fail-closed (collapsed and three-branch unions),
  keyword-named property survival (required + optional), `$defs`/literal
  walking, `dependentRequired` strip, cross-level dedupe, the route
  composition, the engine turn-surface wiring across three route
  quadrants, and the Anthropic forwarding verdict.

Local: `schema_sanitize` 73, `strict*` 70, `strict_tool_mode_doctor` 8,
`tools::remember` 9 — all green; fmt and clippy clean under the CI
allowlist.

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

Copy link
Copy Markdown
Collaborator Author

Round-2 fresh review (2026-11-06, 8 lanes + hand verification) — verdict: all three original fixes are real and root-caused, zero smuggling (28/28 hunks claim-accounted), but two P1 gaps against the "documents-faithful" claim plus wiring/test/doctor findings. All fixed in 10a09d5bb:

Fixed

  1. Required nullables shipped nullable: true under strict: true (contradicted the "no more nullable" claim): sanitize collapses X | null unions into nullable: true and neither the gate nor the strip pass rejected it → now fail-closed at has_strict_incompatible_composition (nullable / null-type anyOf branch at any depth → tool unchanged, non-strict). Repro that used to pass: {"mode":{"anyOf":[{"type":"string"},{"type":"null"}]},"required":["mode"]}.
  2. Keyword-named properties corrupted strict schemas: the recursion descended into the properties name-map as a schema — a required property named const was renamed to enum: [<its schema>] with required left dangling (every call invalid under additionalProperties: false, no note), a properties property was erased with spurious keys injected, patternProperties-named properties were deleted, and removal notes could name parameters that never existed. Now name maps (properties/$defs/definitions) are walked as maps, literal keywords (enum/const/default/examples) ship verbatim.
  3. dependentRequired stripped like the other undocumented bounds keywords.
  4. remember declared note required (the handler already rejects its absence) — the tool was previously reduced to an empty-object call in strict mode.
  5. Doctor vs Anthropic wire: DeepseekAnthropic on canonical non-beta endpoints was reported fallback_non_beta/strict-stripped, but client/anthropic.rs forwards tool.strict untranslated → now official_route with the flag sent, beta recommendation withheld for that dialect. Verified against anthropic.rs forwarding + chat.rs chat-wire-only strip.
  6. provider_mismatch/custom_endpoint messages now state the tool_choice → auto fallback; strict_tool_mode_for_route unifies the engine/sub-agent/doctor composition; new tests pin the route composition, the engine turn-surface wiring (in-scope / custom endpoint / non-family-on-official), and every behavior above.
  7. Body corrected: "no more nullable" claim, const+enum widening disclosure, fix(tui): pass xAI root unions of object branches through #144 added to the merge-conflict surface (this branch rewrites context lines inside fix(tui): pass xAI root unions of object branches through #144's sanitize_for_strict hunk — whoever merges second rebases; no semantic interaction).

Local: schema_sanitize 73, all strict* 70, strict_tool_mode_doctor 8, tools::remember 9 — green; fmt + clippy clean under the CI allowlist.

Known and deliberately not in this PR (follow-up material): purge.rs hardcodes strict: Some(true) on the ad-hoc purge tool outside the gate with a non-conforming schema (pre-existing); GET /v1/config echoes the raw flag rather than the gated effective value; removal-note names are not path-qualified; no root-shape fail-closed on the strict path (the strict docs' type list suggests non-object roots may be legal, so not assumed); doctor --json status vocabulary gained values (no in-repo consumer); parent/child gate reads provider from different sources (resolved route vs configured).

…emas

The strict pipeline now fails a tool closed instead of shipping it
strict when the documented subset cannot serve it faithfully:
- optional removal that would strip every declared parameter (an
  all-optional root left action-discriminated tools callable only with
  an empty object while their runtime still demands the values)
- type arrays and bare null types, null-carrying const/enum members
- the structural/conditional keyword family the Moonshot MFJS path
  already rejects (not, if/then/else, contains, prefixItems,
  propertyNames, unevaluated*, tuple-form items)
Draft-7 dependencies, uniqueItems, and the property/containment bounds
strip like the other widening-only bounds; the documents' singular
$def spelling is walked as a name map; a null const drops outright
instead of landing on enum.

The route gate no longer trims whitespace, matching the doctor
classifier and the wire strip sets, so a padded URL degrades to
out-of-scope instead of leaking the flag past the non-beta strip. The
doctor answers the Anthropic family from the wire on every official
path (beta included) instead of the chat-dialect ready verdict, names
https in the custom-endpoint message, and calls the deepseeki host a
recognized URL rather than an official endpoint. The remember schema
comment states the retire exception instead of claiming note is
required for every writing action.

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

Copy link
Copy Markdown
Collaborator Author

Round-3 fixes pushed (76fefa87a) — closes the findings from the fresh 2026-11-06 review that round 2 did not cover:

  1. Removal that empties a tool now fails closed (prepare_tools_for_strict_mode). Action-discriminated tools (tasks, github, automation, web.run, validate_data, load_skill, harness) mark their whole root property set optional and dispatch on the values execute re-reads; under the removal rule they shipped strict: true callable only with an empty object while a note told the model never to send the parameters the runtime still demands. Such a tool now ships unchanged and non-strict (same rule as required nullables, no note). Partially-optional tools keep removal+note; per-action schema honesty (update_goal's evidence, remember's replaces/evidence, send_later, request_user_input) stays open as a follow-up.

  2. Null-carrying shapes fail closed across encodings. The gate additionally rejects type arrays (the subset's union mechanism is anyOf), bare {"type":"null"}, const: null, and null enum members — each a required nullable wearing another keyword, each previously shipping undocumented constructs under strict: true. Reachable via MCP schemas, which arrive at this gate unnormalized (McpPool::to_api_tools clones verbatim), even though no built-in schema uses these encodings. A null const now drops outright in the strip (instead of landing on enum as [null]) for direct sanitize_for_strict callers.

  3. Structural/conditional keywords fail closed like the MFJS path: not, if/then/else, contains, prefixItems, propertyNames, unevaluatedProperties/unevaluatedItems, tuple-form items — the same family sanitize_for_kimi_parameters rejects with UnsupportedKeyword — now keep a tool non-strict instead of shipping it strict with constraints the reshaping cannot express.

  4. Widening-only strips extended: draft-7 dependencies (both forms), uniqueItems, minProperties/maxProperties/minContains/maxContains join the strip set (dropping only widens; the count bounds are redundant or contradictory under all-required + additionalProperties: false).

  5. $def (the documents' singular spelling) walks as a name map like $defs/definitions, so a definition named after a stripped keyword is no longer deleted or renamed on that path.

  6. Route gate no longer trims whitespace — the doctor classifier and the wire strip sets string-match without trimming, so gate-only trimming admitted padded URLs whose flag then leaked past the non-beta strip; a padded URL degrades to out-of-scope (matrix test moved to the negative list).

  7. Doctor: the Anthropic family now answers from the wire on every official path — beta included (official_route, flag forwarded; the chat-dialect ready message is gone for that provider). custom_endpoint names https; provider_mismatch calls api.deepseeki.com a recognized DeepSeek endpoint URL instead of an official one (it is a legacy-typo compat entry per docs/REBRAND.md).

  8. remember schema comment now states the real contract (note required for append/revise; retire ignores the value but the field stays required), replacing the false "required for every writing action".

Local verification on 76fefa87a: schema_sanitize 80/0 (seven new pins; the per-tool note tests gained a required property so they keep pinning the note path), strict_tool_mode_doctor 9/0, route-gate matrix + composition, engine turn-surface gate, tools::remember 9/0, strict filter 78/0, deepseek filter 159/0 — all green; cargo fmt --check and the gate's exact clippy invocation clean. CI on this head: run 42192582794 — check/gate/Gitleaks all SUCCESS.

Known non-goals recorded for follow-ups: purge_context's hardcoded strict: Some(true) bypasses the route gate; GET /v1/config echoes the raw flag; per-action schema honesty for partially-optional tools (see 1); doctor/engine URL resolution diverges for a legacy file-owned root base_url pointing at a foreign host.

… tools

The compatibility gate now judges the shape that would actually ship:
the enforce pass removes optional properties before the gate walks the
schema, so a null-carrying shape under an OPTIONAL property is exempt
(the removal already represents it faithfully) while the same shape
under a required property still fails the tool closed. Constraints the
enforce pass strips before the gate can see them — a null `const` and
a schema-form draft-7 `dependencies` table (semantically
`dependentSchemas`, which the gate fails closed on) — are detected on
the sanitized shape with the optional-removal in view.

A removal that empties a nested object now fails closed like the
root-emptied rule: a required object whose every property is optional
ships callable only with `{}` while the runtime still reads the
removed members, and a required bare `{"type":"object"}` parameter
collapses to the same shape once `sanitize` injects the empty
`properties`.

`update_goal` and `send_later` fail closed by name: their schemas keep
one discriminating parameter required while `execute` demands sibling
parameters the schema marks optional, so after the removal pass they
shipped `strict: true` callable only with values the runtime rejects
(`update_goal` for every status value, `send_later` for three of four
actions). The documented strict subset cannot express those per-value
requirements, so the tools ship unchanged and non-strict; census tests
feed their real schemas through the pipeline so the list cannot drift.

The strip drops `format` values outside the documented enumeration
(email, hostname, ipv4, ipv6, uuid) and draft-4 boolean
`exclusiveMinimum`/`exclusiveMaximum` — both widening-only — and the
null-family and structural-keyword fail-closed tests are pinned per arm
so a single-keyword regression cannot pass.

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

The fallback_non_beta recommendation ships with its `/beta` path intact.
Both renderings passed it through the authority-only URL redaction used
for configured endpoints, so the doctor advised
`https://api.deepseek.com` — a URL where the chat wire still strips
`function.strict` — sending the user straight back into the same
verdict. The recommendation is a built-in constant, never a configured
URL, so no redaction is needed; the JSON report and the human output now
emit it verbatim and the test pins the full value.

The official_route messages say "DeepSeek-family" instead of
"official": the gate also accepts the legacy `api.deepseeki.com`
compat host, which docs/REBRAND.md records as recognized but not
official — the same distinction the provider_mismatch arm already drew.

The `strict_tool_mode` field doc no longer claims `tool_choice` falls
back to `auto` when individual tools fail closed: `tool_choice` follows
the route gate, not per-tool outcomes — it stays `required` on an
in-scope route and falls back only when the route itself is out of
scope.

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

Copy link
Copy Markdown
Collaborator Author

Fourth-round fixes are pushed (head 3b2dec71a, commits 705546c94 + 3b2dec71a), from the second fresh review of 2026-11-06 (11 lanes, all findings personally re-verified before fixing).

Four P1s fixed:

  1. Doctor advice was self-defeating. The fallback_non_beta recommendation rendered through the authority-only URL redaction, so https://api.deepseek.com/beta displayed as https://api.deepseek.com — a URL where the chat wire still strips function.strict. Following the doctor's only actionable advice landed back in the same verdict, and the JSON test pinned the stripped value. The recommendation is a built-in constant, never a configured URL, so both renderings now emit it verbatim (705546c94's sibling commit), and the test pins the full /beta value.
  2. update_goal shipped strict: true while every accepted status failed at runtime (execute demands evidence/verification/blocker/advisory — all schema-optional, all removed by the strict pass), and send_later shipped strict with three of four actions dead. Same degenerate class the third round fixed at root level, dodged by one surviving discriminator parameter. No schema-level analysis can see it (the requirement is per-value; if/then and root alternatives are outside the subset), so both tools now fail closed by name (STRICT_UNUSABLE_TOOLS, runtime contracts cited at the definition), with census tests feeding their real schemas through the pipeline so the list cannot drift.
  3. remember's required: ["note"] had no pin — reverting it to an all-optional schema passed every test. A census test now runs the real RememberTool schema through strict prep; the null-family fail-closed tests are also split per arm and the nine structural/conditional keywords are pinned individually (shared fixtures let single-keyword regressions pass).
  4. The strict_tool_mode field doc claimed a tool_choice fallback that isn't wired — the prepare_tools_for_strict_mode bool is discarded; tool_choice follows the route gate and stays required on in-scope routes even when tools fail closed. The doc now states the real rule.

Also in this round:

  • The compatibility gate now judges the shape that ships (post-enforce): a null-carrying shape under an optional property keeps strict mode (the removal represents it faithfully) instead of failing the whole tool — every Pydantic-style MCP schema with an optional X | null parameter benefits; required-position shapes still fail closed per arm.
  • A removal that empties a nested object now fails closed like the root-emptied rule (required nested all-optional objects, and required bare {"type":"object"} parameters, previously shipped strict callable only with {}); built-in catalog verified unimpacted, exposure is MCP-supplied schemas.
  • Schema-form draft-7 dependencies fails closed like its semantic twin dependentSchemas (detected pre-strip; the name-table form keeps its widening-only strip); format values outside the documented enumeration (email, hostname, ipv4, ipv6, uuid) and draft-4 boolean exclusiveMinimum/exclusiveMaximum strip as widening-only.
  • official_route doctor messages say "DeepSeek-family" rather than "official" (the legacy api.deepseeki.com compat host is recognized, not official — docs/REBRAND.md).

Every new rule was mutation-verified locally (six flips, each turning at least one test red). Residual degraded-action tools (Git show/blame, lsp definition/references, pandoc binary targets, bash sandbox-escalation retry, hidden agents/message/agents/followup, remember revise/retire) and the other accepted residuals are now disclosed explicitly in the PR body under "Disclosed residual gaps" instead of being folded into a vaguer follow-up sentence.

Local on 3b2dec71a: schema_sanitize 89 / strict_mode 22 / strict_tool_mode_doctor 9 / strict 90 / deepseek 159 / tools::remember 10 / tools::goal 28 / tools::send_later 15 plus the census tests, cargo fmt --check clean, and the gate's exact clippy invocation clean. CI on this head: run 42232236062 — 15,037 tests, 15 skipped, 0 failed; check, gate, Gitleaks all SUCCESS.

@asto18089

Copy link
Copy Markdown
Collaborator Author

PR #147 fresh 评审(2026-11-09,9 lanes + 逐项一手复核)

基线

  • base pinjou3-clean @ 11cba403dc;merge-base == base tip,分支已在最新 main 上,无需 rebase,无冲突(PR API merge_base_sha 与 base tip 相同,本地 git merge-base 同证)。head 3b2dec71ad 评审期间未变动。
  • CI 独立核实:run 42232236062(head 3b2dec71a)= 15,037 tests / 15 skipped / 0 failed,check/gate/Gitleaks 全 SUCCESS;本地 CI-exact clippy(fork-ci.yml 原命令)与 cargo fmt --check 干净;本地全 workspace --lib 套件见文末补充。
  • 方法:9 条独立 lane(主张↔diff 完整性 / 文档对齐(live 抓取 2026-11-08) / 路由门接线与攻击面 / sanitizer 特性 bug(实际运行 probe)/ 走私检测(逐 hunk + 逐 commit)/ 设计 / 性能(release 实测)/ 变异验证(实际跑 8 组变异)/ 独立交叉复核),所有 P 级结论均有行级证据或多 lane 收敛。

四项重点

  1. 有用、完整、根因修复:是。 三条主张逐 hunk 对账全覆盖,反向(每条声明→hunk)也无 fabricated;optional 移除的 description note 端到端追踪到 wire(tool_catalog.rs:886 → chat.rs:3194 序列化 → anthropic.rs:100-110 转发),模型真实可见;rounds 1–4 声明的 ~38 个测试逐一读体核验,确实钉住所称行为;census 测试喂的是 UpdateGoalTool::new(...).input_schema() 真实 schema(goal.rs:2010、send_later.rs:1012,1036),名单/schema 漂移即红(实测移除 "update_goal" 恰好只红 census 一项)。residual gaps 披露逐条对码属实(purge.rs base/head 字节相同;Git/lsp 的 schema-optional-runtime-required 与披露一致;MCP schema 未规范化直达门禁属实)。
  2. 优雅:是,无需重构。 不是第三个并行 walker——在既有 strict 家族内扩展并复用 sanitize/required_names/is_null_type 等 helper;MFJS 与 strict 契约本质不同(白名单拒绝 vs 变换+fail-closed 保留),不共享是正确取舍,与 fix(tui): hold Moonshot tool schemas to the MFJS docs #143 无交错;三个 surface(engine turn / 子 agent / doctor)从同一个纯函数 strict_tool_mode_for_route 取答,结构上不可能再分歧;STRICT_UNUSABLE_TOOLS 名单 + census 防漂移是诚实工程取舍(真正的契约在 execute 运行时里,schema walker 原理上看不见);pass 顺序不变式(pre-strip → enforce → post-gate-on-shipped-shape)在判定点就地注释。P3 打磨项见下。
  3. 走私:无。 双 lane 独立对账收敛:11 个文件全 Rust,零 Cargo.toml/lock、零 i18n、零配置文件改动;mark_nullable 机制删净无残尸;无 TODO/FIXME、无调试输出、无 feature flag 翻转。remember schema required:["note"] 是全局 wire 变更(非仅 strict 路由),但 base 的 execute 早已 runtime 拒绝缺 note 的 append/revise——对齐既有运行时事实,已披露,且不拆分(拆了 strict 模式下 remember 全 opt 就是原 bug)。非 DeepSeek 路由 wire 零变化(xAI/responses/MFJS 入口与共享 sanitize() 均未触碰;pipeline 唯一生产入口受门禁约束)。
  4. bug:1 个 PR 引入的 P2 代码缺陷 + 2 个 P2 级契约/披露缺口;无 P0/P1。 性能实测无回归(in-scope 路由常数因子 ~1.4–1.5×,52 工具×115 节点目录 2.3ms/请求、156 工具 7ms,线性验证 3×目录→3.0×耗时;out-of-scope 路由相对 base 净改善——base 曾在 flag 开启时对任何路由每步跑全 pipeline)。作者声明的 6 组变异全部实测验证(8 组变体,零 fake-green,逐组记录红测试名单);九个结构关键字的 gate 表与测试循环表完全对称,删 contains 单臂即红对应 pin。

P2-1(代码缺陷,请求修复):strict 门禁把 name map / 字面量关键字当作 schema —— round-2 的 walker 修复没有同步到门禁

has_unrepresentable_stripped_constraint 有 name-map 臂(schema_sanitize.rs:277 "properties" | "$defs" | "definitions" | "$def")和字面量跳过臂(:292 "enum" | "const" | "default" | "examples" => {}),enforce_strict_subset 同样有(:570-577)。但 round-3/4 扩建的门禁 has_strict_incompatible_composition 是三个 walker 里唯一没有这两组臂的:contains_key("nullable")(:316)、九关键字 key 扫描(:361-374)直接对递归进来的任何对象生效,而末尾递归 obj.values().any(...)(:403-405)无差别下钻。后果(两条 lane 独立 probe 实测,head/base 双向验证):

{"type":"object",
 "properties":{"needle":{"type":"string"},
               "contains":{"type":"string","description":"substring to match"}},
 "required":["needle","contains"]}

全 required、完全可表达——期望 strict = Some(true)(base 如此),head 实际 strict = None:properties 名表被当 schema,"contains" 在名表层命中结构关键字臂 → 整工具被误判 fail-closed。触发名集合:nullable, oneOf, allOf, dependentSchemas, not, if, then, else, contains, prefixItems, propertyNames, unevaluatedProperties, unevaluatedItems 作为 required 属性名或 $defs 定义名。其中 oneOf/allOf/dependentSchemas 是 base 既有的同类误伤,其余全是本 PR 扩大的新误伤类(含 nullable、contains 这类常见英文词)。同根源第二形态:门禁还把字面量值当 schema 下钻——enum 成员是对象(如 [{"contains":"abc"},{"exact":"abc"}])或 default 值形如 {"nullable":true} 时同样误判(has_unrepresentable_stripped_constraint 对这两类都有豁免,门禁没有)。

方向评估:fail-safe(宁可非 strict,schema 原样保留),不会发出错误 strict 输出;内置目录 grep 无触发名(唯一 dependentSchemas 在 subagent/mod.rs:9622,是真实条件schema、按设计 fail-closed),当前仅 MCP 提供的 schema 可达——但这正是 PR 自己强调的外部输入面。更重要的是它直接违背 PR round-2 自己确立并修复的原则("schema walk 不得把 name map 当 schema"):walker 修了,门禁没跟上,round-3/4 的门禁扩臂把盲区越修越大。

修复建议:给 has_strict_incompatible_composition 补上与 sibling 相同的两类臂——name map 臂按命名 schema 下钻(properties/$defs/definitions/$def,子节点 is_root=false)、字面量臂整类跳过;补钉:required 属性名 contains/nullable → 仍 strict、$defs 定义名 not → 仍 strict、对象 enum 成员 → 仍 strict。

P2-2(契约缺口,请求处置):文档"all function need to set the strict property to true"一句话被整份证据引用遗漏,而默认配置构造的恰是 mixed set

PR 的 Document evidence 逐句引用了 strict 契约(object 规则、支持类型表、format 白名单、$def 拼写……),唯独没引用也没处置 step-2 的这一句。而行为上(两条 lane 独立确认):prepare_tools_for_strict_mode 的 all_strict 返回值在生产唯一调用点被丢弃(tool_catalog.rs:886,base 同款,pre-existing);chat.rs:3198-3202 逐工具仅 Some 才写 strict;于是任一工具 fail-closed(update_goal/send_later 按名单总是 fail-closed,而 goal 工具在提供 goal_state 时默认注册,registry.rs:1305-1307)时,默认 /beta 路由的每个请求发的都是 mixed set(多数 strict:true + 少数无 strict 键)+ tool_choice:"required"。文档同段的 error 句("If the schema does not conform... an error message will be returned")对 mixed set 下 server 按请求还是按工具生效没有定论,三种行为(拒请求 / 全请求降级 / 仅校验 strict:true 工具)后果完全不同且均未被论证。

请求:在 body 证据节引用并处置这句话(说明为何按工具 fail-closed 是可接受语义、或改行为);最好补一次最小 live probe(PR 声明未做 probe——一次 mixed-set 探测即可把三种未知定成一种已知);在丢弃 all_strict 处加一行注释说明取舍。

P2-3(doctor 盲区,请求披露或对齐):Responses 方言 — doctor 报 ready/official_route + function_strict_sent: true,但 responses wire 硬编码 strict:false + tool_choice:"auto"

responses.rs:1239 逐工具硬编码 "strict": false、responses.rs:183 有工具时 tool_choice 恒 auto;resolver.rs:240-251, 629-636 把目录外 deepseek-v<digit>* 模型路由到 endpoint_key = "responses";而 doctor(lib.rs:7508-7612)只读 provider + base_url,从不查 endpoint_key。后果:该方言下 schema 被 reshape(optional 参数被删+note)却零 enforcement,doctor 仍报 flag 已发。三处代码 base 与 head 逐行相同——盲区本身 pre-existing;但 round-2 修 Anthropic 方言"doctor vs wire"时修的正是这个类(当时按 wire 转发事实修正了 verdict),第三个方言漏了,且新增的 official_route 臂上"the flag is sent"措辞是本 PR 新写的。请求:doctor 纳入 endpoint_key 判定(如实报告 responses 方言无 enforcement),或 body 披露 residual + 判定处代码注释。

P3(不阻塞,逐条有据)

  1. STRICT_UNUSABLE_TOOLS 之外,门禁的结构关键字族在 gate matches! 表与测试循环数组重复存在两份(:364-373 vs :1635-1645)——当前对称,但第十字关键字只加一边会静默漂移;建议共享 const。
  2. STRICT_UNUSABLE_TOOLS 把工具运行时知识放进 schema 模块(models::Tool 是 wire 类型);census 已从工具侧钉住耦合,第三个成员出现时再迁 per-tool 声明即可。
  3. 三处 strict_tool_mode_for_route 调用点各自重推 URL(deepseek_base_url());一个小包装可消除"喂错 URL 来源"的可能。
  4. 根级 additionalProperties map-tool({"type":"object","additionalProperties":{...}})在 root 豁免下 ship strict:true 只能调 {},与嵌套同形的 fail-closed 不一致——pre-existing,但披露理由("authored no-arg tools")并不覆盖 map root;MCP 可达。
  5. pattern/multipleOf/$ref/$defs 在文档子集外却 ship under strict:true(pre-existing,pattern 甚至有测试钉住保留)——与 PR 自己的 fail-closed 哲学相悖的残留 conformance 缺口,建议披露或后续处理。
  6. additionalProperties schema 形式被静默收窄为 false——wire 契约要求,但对其他不可表达约束都走 note/fail-closed 的体系里这是唯一的静默语义损失;建议 body 补一句。
  7. 非数值 const → enum:[v] 对 bool/object/array 成员并非纯保守(文档只给了 string 示例;validator 若拒 enum:[true] 是 documented error,而丢弃 const 是纯放宽)——已测试钉住的合理取舍,建议 body 注明该残余风险。
  8. 文档化 strict 契约只存在于 /beta chat-completions;非 beta 与 Anthropic 方言上的 reshape 是披露过的薄地基(doctor 已引导 /beta)。
  9. 官方 host 非规范 chat 路径上 flag 保留但 URL 不经 beta 化(pre-existing;doctor "flag is sent" 字面为真)。
  10. doctor 人类可读输出那行 println!(lib.rs:4738)无测试(JSON pin 已验证充分;structural_url_authority 回归若只落在 println 上不会被抓住)。
  11. purge_context 硬编码 strict 标记(purge.rs:532)在 out-of-scope 路由照样发 strict:true——base/head 字节相同,已如实披露为独立事项。

结论

Request Changes —— P2-1 修复、P2-2 处置、P2-3 披露或对齐后即达 merge-grade。 除上述三项外,本 PR 质量在评审过的 fork PR 里属最高档:主张真实完整、根因到位、零走私、变异与 census 双重防漂移、性能有界、文档措辞逐句诚实(config 字段文档 config.rs:3113-3129 逐句核验无一假句)。三项均是小改动(一个 walker 臂补齐 + 两处披露),不改架构。

本地补充验证(CARGO_TARGET_DIR 预热增量编译,RUST_MIN_STACK=16777216 CARGO_PROFILE_TEST_DEBUG=0,--test-threads=4):CI-exact clippy 全 workspace 干净;cargo fmt --check 干净;除 tui 外全部 crate --lib 绿(~1,948 tests);tui --lib(skip speech/voice 两个在本机无声卡环境下会挂起的模块,22 filtered out)12,441 passed / 0 failed(首跑曾 1 failed,复跑同命令未复现,为本机与并行构建争抢 CPU 的负载 flake;CI nextest 在同一 head 上全量 15,037/0 为准)。

…r from every wire

Round-5 fixes from the 2026-11-09 fresh review:

- The composition gate now walks `properties`/`$defs`/`definitions`/`$def`
  as name maps and skips `enum`/`const`/`default`/`examples` as values,
  like its sibling walkers: a required property named after a gate
  keyword (or a definition named `not`, an object-shaped `enum` member,
  a `default` shaped like a keyword) no longer fails the tool closed on
  its name or shape alone.
- The nine structural/conditional keywords share one constant between
  the gate and the test loop that pins each of them.
- A root map shape (`additionalProperties` schema form or `true` beside
  no properties) fails closed pre-enforce instead of shipping
  `strict: true` callable only with `{}`; a typeless node with a
  non-`false` `additionalProperties` fails closed instead of shipping
  the authored keyword verbatim (the enforce pass normalizes only
  object schemas). Authored no-argument roots keep the exemption.
- The doctor reports a new `responses_dialect` verdict when the route's
  model resolves to the Responses endpoint (the wire sends
  `strict: false` and `tool_choice: "auto"`): reshaping applies, no
  enforcement is sent, and no base-URL recommendation is offered —
  the dialect is model-driven, not URL-driven. Exact catalog rows stay
  authoritative.
- The discarded `all_strict` return carries the mixed-set disposition
  at the only production call site: the documents describe strict mode
  with every function flagged and leave the server's mixed-set behavior
  undocumented, so the return stays the lever for an all-or-nothing
  policy if a probe ever shows one is needed.

Signed-off-by: asto <asto18089@126.com>
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.

1 participant