fix(providers): shape tool schemas to the wire so the door survives - #2227
Conversation
There was a problem hiding this comment.
Changes requested: The retry path infers a schema rejection from an unrelated 400, and the changed wire and operator contracts are not reflected in their specs.
Warning
Changes requested · head 810f0be · 3 findings: 1 major, 2 minor
| Severity | Finding | Where |
|---|---|---|
| major | F1 Any HTTP 400 is misattributed to the first lookaround tool | src/core/dispatch/operator.ts:1170 |
| minor | F2 Spec contradiction — model-proxy.md item 1: tool definitions are no longer forwarded byte-for-byte | docs/reference/specs/model-proxy.md:11 |
| minor | F3 Spec contradiction — routing-and-config.md item 29: a schema 400 now retries instead of stopping as a typed refusal | docs/reference/specs/routing-and-config.md:52 |
Full review
F1 — Major, high confidence. request-rejected plus status 400 does not prove a tool schema caused the failure. A 400 for an unsupported model option will remove an unrelated lookaround-bearing tool, record a false diagnosis, and retry routing with reduced capabilities. The real catalogue also contains four such tools while the retry budget is two. Carry explicit schema-rejection metadata—including tool and keyword—from the provider boundary, and retry only when that evidence exists.
F2 — Minor, high confidence. Item 1 promises tool definitions reach the provider byte-for-byte, and the Responses validation criterion repeats that guarantee. The proxy now removes unsupported pattern fields. Update the behavior and validation row to document per-wire schema shaping and bind the new conformance test.
F3 — Minor, high confidence. Item 29 explicitly says an OpenAI schema 400 renders a typed refusal and stops. The implementation now retries without a tool. Amend the behavior and validation criterion to describe the intended exception and its audit record.
|
Adding a fourth finding, minor, to the round at F4 —
This is not a request to enumerate the Responses validator. It is a request that the code tell the truth about what it covers:
The reason this stays minor rather than major is F1's mechanism: an unknown refused construct costs one failed call and one degraded tool, not the door — provided F1 is fixed so the re-ask fires on evidence. If F1's fix narrows the re-ask to explicit schema-rejection metadata from the provider boundary, please make sure an unrecognised construct still reaches that path rather than falling to Related, and worth folding into F1's fix: |
520e721 to
e922e38
Compare
|
Re-review requested at F1 (major), the misattributed 400. The operator no longer infers the offending tool by scanning the prompt for the first lookaround carrier. The model-proxy boundary mints explicit schema-rejection evidence naming the tool and the keyword, and the re-ask fires only when that evidence exists, removing only the tool it names — so a 400 raised by something else, an unsupported model option for instance, no longer drops an unrelated tool and retries with reduced capabilities. The schema re-ask budget is separated from the structured-violation counter, so a turn that has already spent a violation still has its re-ask. F2 and F3, the spec contradictions. F4, the guard's narrowness. The comment states that lookaround is the one measured mismatch and that the operator re-ask covers an unmeasured one, and the conformance row asserts the catalogue is clean of known-refused constructs rather than implying none can exist. An unrecognised construct still reaches the re-ask path rather than falling to History and description. The branch is squashed to one commit at the same tree the fixes were validated on —
|
There was a problem hiding this comment.
Changes requested: The model-proxy path is fixed, but the direct pi adapter can still infer and remove an unrelated tool from a tool-less schema 400.
Warning
Changes requested · head e922e38 · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Tool-less schema errors still select the first matching tool | src/core/provider.ts:342 |
Full review
F1 — The direct pi adapter still mints schemaRejection when the provider names only a schema keyword. If several offered tools contain that keyword, carrying[0] arbitrarily attributes the rejection to the first and the operator removes an unrelated tool. Require exactly one provider-named offered tool and one named keyword; otherwise keep the generic typed refusal.
Co-Authored-By: coreplane-switchboard[bot] <318072483+coreplane-switchboard[bot]@users.noreply.github.com>
e922e38 to
439b802
Compare
|
Re-review requested at F1, the tool-less schema 400 in the direct pi adapter. The regression: Three earlier guarantees remain pinned by their own tests, so this was not bought by undoing the previous round: an explicit schema rejection is still re-asked without its tool even when the keyword is unmeasured; schema re-asks still have their own budget after an ordinary structured violation; and a provider-rejected request without explicit evidence is one typed refusal, never a re-ask or a One judgement call flagged rather than asserted. With the lookaround-implies- The branch is one commit, rebased onto current main, which now carries #2190's fix. |
There was a problem hiding this comment.
LGTM: The direct pi adapter now requires unambiguous provider-named tool and keyword evidence, and the regression covers ambiguous keyword-only refusals.
Note
Approved · head 439b802 · no findings
Full review
The prior finding is resolved, and no new issues remain.
Pointing the model defaults at first-party OpenAI took the whole door down: four of the operator's tools carry a JSON-schema
patternwith a regex lookaround, the Responses API rejects the entire request with a 400 before generating anything, and every ask fell through togeneral. The proxy now strips constructs measured as refused and records each one; only boundary-vouched evidence naming an offered tool and keyword lets the operator re-ask without that tool.Why: Closes #2189 and #2188. Production sits on OpenRouter today purely because of this. It is one of three fixes gating the switch to first-party OpenAI, with #2190 and #2099; it builds on the typed provider-failure seam from #2195 rather than beside it.
Where to look
non_decision.Feedback wanted: Please scrutinize whether the boundary's exact-one quoted tool and keyword gate can misattribute provider prose, and whether four schema repairs is the right bound when evidence names an absent or repeated tool.
Risk: If the shaping is wrong the door goes down on OpenAI exactly as it does today, which is total: no review, coding or ship ask can start. If it over-strips, a tool silently loses an input constraint and falls back to its own validation. Blast radius is every operator turn on the Responses wire; rollback is a revert plus the config staying on OpenRouter, which is where it already is. 400 changed lines across 4 files, all in the two named seams — no split considered, since the proxy half and the operator half are useless apart.
Verified: 125 focused tests plus
hygiene:check,specs:checkand typecheck passed on the branch; full suite is CI-gated, and the live-API proof is human-gated below.Decisions (3)
REPO_SLUGinsrc/core/delivery.tsand move on. Measuring the emitted catalogue killed that: 52 tools, 21 patterns, and four carrying the lookaround shape —delivery_report,pulls_enqueue,pulls_merge,pulls_rebase— and one refused tool fails the whole request. A per-tool fix leaves three live and reopens the moment a fifth is written.request-rejected400 names only the failure class. The proxy mints authenticated tool-and-keyword evidence only when provider prose identifies schema validation, exactly one offered tool and exactly one keyword in its sent schema; a generic 400 remains a refusal.Validation (9 criteria)
src/channels/modelProxy.test.ts::tool-schema conformance on each wire::the operator catalogue is clean of every construct known to be refused on each wire— builds the live 52-tool catalogue and asserts the four lookaround carriers by nameanthropic-messagesandopenai-chat, 0 onopenai-responsessrc/channels/modelProxy.test.ts::keeps native Responses patterns and property names byte-for-bytenon_decisionsrc/core/dispatch/operator.test.ts::runOperator — the loop over a scripted model::an explicit schema rejection is re-asked without its tool even when the keyword is unmeasuredprovider rejected tool "…" schema keyword "…"; re-asked without that toolsrc/core/dispatch/operator.test.ts::runOperator — the loop over a scripted model::schema re-asks have their own budget after an ordinary structured violationmodel-proxy.mditem 1 limits shaping claims to constructs measured as refusedsrc/channels/modelProxy.test.ts::tool-schema conformance on each wire::the operator catalogue is clean of every construct known to be refused on each wirerouting-and-config.mditem 29 binds schema repair to boundary evidence and its own four-repair budgetsrc/core/dispatch/operator.test.ts::runOperator — the loop over a scripted model::an explicit schema rejection is re-asked without its tool even when the keyword is unmeasured;::schema re-asks have their own budget after an ordinary structured violationshapeToolSchemasForWireis exported for exactly this probe; no receipt yet, and it is the only pre-flip proof, since the operator resolves its model from config alone and no per-user trial reaches itFor agents
The head
e922e38dc618fa2d49a8f3eb936169cde16eb8e0is the review-reworked branch, squashed to one commit. This pull request was opened from the operator session rather than by the runner: the ship parent aborted twice without opening one — attempt 1 as #2197 (clean handoff read as "ended before ready"), attempt 2 withoperator: {outcome: "non_decision", reason: "the operator failed: answer cut at the output cap (374 tokens)"}, which is #2099. Both pushes are recordedby: "salvage". No code was written or amended in this description-only refresh; the branch head was left untouched.shapeToolSchemasForWire(shape, body)is the named export a live-API probe should import; it is a no-op for any shape other thanopenai-responses.This PR does not close the OpenAI flip risk on its own. The output cap (#2099) is a separate gate: the cap is derived for answer text alone while the production openai block sets
capField: max_output_tokens, which on the Responses API counts reasoning tokens too, so at effort medium a reasoning turn can spend it before emitting a tool call. On 2026-09-21 that was masked, because the schema 400 fails at request validation before generation ever starts. Do not read a green review here as the door being safe on OpenAI.To reproduce the original defect: build the operator catalogue, send it to
/v1/responses, and observeinvalid_request_error/invalid_json_schemabefore any tokens.