fix(tools): object-root union tool parameter schemas so they advertise parameters - #2190
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a provider-interop bug where union-rooted TypeBox tool parameter schemas (e.g., Type.Union([...])) are advertised upstream with no parameters, making such tools effectively uncallable for clients that type tool arguments from the advertised schema. It does this by normalizing union roots (when all branches are object-rooted) into an equivalent object-rooted schema that remains equally strict by retaining the original union branches under anyOf.
Changes:
- Added
normalizeToolParameterSchemato rewrite union-rooted, object-branch-only schemas into an advertisable object-root while keeping the originalanyOffor strict validation equivalence. - Applied the normalization in
wrapToolDefinition, ensuring the advertised schema and validated schema are the same object for every tool adapter path. - Added a new Vitest suite covering schema advertising, strictness equivalence, coercion behavior, caching, and warning behavior; updated docs and changelog accordingly.
Show a summary per file
| File | Description |
|---|---|
| packages/coding-agent/src/core/tools/tool-parameter-schema.ts | Implements union-root normalization + memoization + one-time warning for unadvertisable roots. |
| packages/coding-agent/src/core/tools/tool-definition-wrapper.ts | Normalizes tool parameters in the central tool adapter to keep advertised/validated schemas aligned. |
| packages/coding-agent/test/tool-parameter-schema.test.ts | Adds coverage validating advertising behavior, validation equivalence, coercion behavior, caching, and warnings. |
| packages/coding-agent/docs/extensions.md | Documents the object-root requirement and the union-of-objects normalization behavior. |
| packages/coding-agent/CHANGELOG.md | Adds a user-facing “Fixed” entry describing the defect and the normalization-based fix. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Lite
58eac96 to
6043d6f
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (2)
packages/coding-agent/src/core/tools/tool-parameter-schema.ts:96
mergeObjectBranchesclaims the rewritten root preserves the union's accepted value set, but if any branch allows additional properties (i.e.additionalPropertiesis notfalse), the merged root can become more restrictive than the original union. Example: if branch B is open and does not declare a key that exists in branch A, the original union can accept that key with any value via branch B, but the merged root will still validate it against branch A’s property schema and can reject it beforeanyOfis considered.
To keep the “accepted set is unchanged” guarantee, only perform the merge when every branch is closed (additionalProperties: false), or adjust the merge so branch-specific keys don’t constrain open branches.
const closed = objectBranches.every((branch) => branch.additionalProperties === false);
return Type.Object(properties, {
...(closed ? { additionalProperties: false } : {}),
// Retaining the branches keeps validation exactly as strict as the union root.
anyOf: branches,
packages/coding-agent/docs/extensions.md:1957
- This paragraph states that a
Type.Unionof object-rooted branches is rewritten in a way that keeps validation “exactly as strict as the union”. That only holds if the normalization doesn’t introduce stricter root-levelpropertiesconstraints than at least one open branch would have allowed. If you keep the current implementation, it would be worth documenting the limitation (e.g. that branches should be closed withadditionalProperties: false) so extension authors don’t get surprising rejections.
Tool `parameters` must be object-rooted. A provider advertises a tool from the root `properties`/`required` keywords alone, so a root carrying neither is advertised as a tool with no parameters and clients then send every argument as a string. A `Type.Union` of object-rooted branches is still accepted: Atomic rewrites it to an equivalent object root — branch properties merged and optional, a differing discriminator unioned, the original branches retained under `anyOf` — so validation stays exactly as strict as the union. Any other non-object root is left as authored and warns once at registration.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Updated: I found a working registry mirror, so the suite now actually runs and the PR body carries real output instead of an explanation of why it was missing. Two changes since the first push:
|
…e parameters
A provider builds a tool's advertised `input_schema` from the root
`properties`/`required` keywords alone, and a `Type.Union` root serializes to
`{anyOf}` carrying neither, so such a tool shipped as
`{"type":"object","properties":{},"required":[]}`. Clients that type tool
arguments from the advertised schema then sent every argument as a string while
validation still enforced the union, so a schema-backed structured_output step
could not complete at all.
Rewrite a union root whose branches are all object-rooted into an equivalent
object root, in `wrapToolDefinition` — the single adapter every tool passes
through, and the one place where the advertised schema and the validated schema
are guaranteed to be the same object. Branch properties are merged, a
discriminator that differs across branches is unioned rather than first-wins,
a property is required only when every branch requires it, and the original
branches are retained under `anyOf` so validation stays exactly as strict.
Optionality uses `Type.Optional` because TypeBox encodes it with a marker rather
than the `required` array. Rewrites are memoized per authored schema so repeated
registry refreshes reuse one compiled validator.
The rewritten root also restores the in-place argument coercion that
`Value.Convert` cannot apply to a union. A root that is neither object-rooted nor
a union of object roots cannot be advertised at all; it is left as authored and
warns once at registration instead of failing silently at turn time.
6043d6f to
e7520c2
Compare
|
Rebased onto latest Re-verified on the rebased base after a fresh
Body updated with the final regression numbers. |
There was a problem hiding this comment.
Review details
Suppressed comments (3)
packages/coding-agent/src/core/tools/tool-parameter-schema.ts:20
- The docstring’s claim that the rewrite leaves the accepted value set “unchanged” is only guaranteed when branches reject extra keys (e.g.,
additionalProperties: false). If any branch allows additional properties, adding mergedpropertiesat the root can newly validate (and potentially reject) keys that were previously treated as unrestricted “extra” properties in other branches when names collide across branches. Please qualify this guarantee (or tighten the rewrite preconditions) so the documentation matches the actual JSON Schema semantics.
* coercion applies in place. Validation strictness is preserved by keeping the
* original branches under `anyOf` on the rewritten root, so a value must satisfy
* both the merged object and one full branch. The accepted value set is
* therefore unchanged.
packages/coding-agent/docs/extensions.md:1957
- This paragraph states the union rewrite keeps validation “exactly as strict as the union”. That is only guaranteed when union branches reject undeclared keys (e.g.,
additionalProperties: false). If branches allow additional properties, the merged root’spropertiescan start validating (and rejecting) keys that were previously permitted as extras in some branches when property names collide across branches. Consider qualifying this statement to avoid overpromising equivalence.
Tool `parameters` must be object-rooted. A provider advertises a tool from the root `properties`/`required` keywords alone, so a root carrying neither is advertised as a tool with no parameters and clients then send every argument as a string. A `Type.Union` of object-rooted branches is still accepted: Atomic rewrites it to an equivalent object root — branch properties merged and optional, a differing discriminator unioned, the original branches retained under `anyOf` — so validation stays exactly as strict as the union. Any other non-object root is left as authored and warns once at registration.
packages/coding-agent/CHANGELOG.md:7
- The changelog entry claims the rewrite keeps validation “exactly as strict” and that “the set of accepted arguments is unchanged”. That equivalence only strictly holds when union branches reject undeclared keys (e.g.,
additionalProperties: false), otherwise merged rootpropertiescan constrain keys that were previously allowed as extra properties in some branches when names collide across branches. Please reword to avoid asserting full set-equivalence if it’s not guaranteed.
- Union-rooted tool parameter schemas are now advertised with real parameters. A provider builds a tool's `input_schema` from the root `properties`/`required` keywords alone, and a `Type.Union` root serializes to `{anyOf}` carrying neither, so such a tool shipped as `{"type":"object","properties":{},"required":[]}`. Any client that types tool arguments from the advertised schema then sent every argument as a string — arrays and objects arrived as JSON text — while validation still enforced the union and rejected them, so a schema-backed `structured_output` step could not complete at all and exhausted its corrective attempts. A union root whose branches are all object-rooted is now rewritten once, in the single adapter every tool passes through, into an equivalent object root: branch properties are merged, a discriminator that differs across branches becomes a union of its literals, a property is required only when every branch requires it, and the original branches are retained under `anyOf` so validation stays exactly as strict — the set of accepted arguments is unchanged. The rewritten root also restores the in-place argument coercion that `Value.Convert` cannot apply to a union. A root that is neither object-rooted nor a union of object roots cannot be advertised at all; it is left untouched and now warns once at registration instead of silently producing an argument-less tool at turn time ([#2189](https://github.com/bastani-inc/atomic/issues/2189)).
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Hi @elefthei, thanks for your patience, and sorry for the delay. We have been working through a larger refactor and the pi 0.84.1 dependency upgrade. Since your last update, Could you also address the concern in the latest Copilot review that open union branches may become stricter after normalization, either with focused coverage or an explanation of why the accepted input set remains unchanged? Please let us know if you have any questions. |
Closes #2189
Problem
A tool whose
parametersschema is union-rooted is advertised to the provider with no parameters at all, while validation still enforces the full union — so the tool is uncallable.convertToolsbuilds the advertisedinput_schemafrom the rootproperties/requiredkeywords alone. AType.Unionroot serializes to{anyOf}and carries neither, so the tool ships as{"type":"object","properties":{},"required":[]}. Clients that type tool arguments from the advertised schema then send every argument as a string; validation rejects them against the union that is still enforced. Every branch of a discriminated union requires a container, so no valid call is constructible.This makes
createStructuredOutputTool({ schema })unusable with a discriminated-union schema — the natural shape for a "return one of N turn kinds" contract — and a workflow stage with such a schema fails hard withSTRUCTURED_OUTPUT_MISSING_ERRORafter exhausting its corrective prompts. It is invisible to clients that emit raw JSON tool input, which is why it has not surfaced before.Solution
Rewrite a union root whose branches are all object-rooted into an equivalent object root:
propertiesmakes it advertisable; the retainedanyOfkeeps validation exactly as strict, so the set of accepted arguments is unchanged.Applied in
wrapToolDefinition— the single adapter every tool passes through (built-ins, SDKcustomTools, extensionregisterTool, MCP,createStructuredOutputTool), and the one place where the advertised schema and the validated schema are guaranteed to be the same object.Two details are load-bearing:
requiredarray. A raw JSON merge over-requires and rejects every branch but one, so merged properties are wrapped inType.Optional.kindto one branch's literal and rejects the others, so colliding keys are unioned.Rewrites are memoized per authored schema, so repeated registry refreshes reuse one schema — and therefore one entry in the provider's identity-keyed compiled-validator cache.
Object-rooted schemas are returned unchanged (identity, no allocation). A root that is neither object-rooted nor a union of object roots cannot be advertised at all; it is left as authored and warns once at registration instead of failing silently at turn time.
The rewritten root also restores the in-place argument coercion that
Value.Convertcannot apply to a union. Note this part is currently masked:validateToolArgumentsskips itscoerceWithJsonSchemafallback only for schemas carrying theTypeBox.Kindown-symbol, and TypeBox 1.3.7 schemas carry no such symbol, so the fallback runs and already coerces union roots. Measured both ways.Tests
packages/coding-agent/test/tool-parameter-schema.test.ts(new, 8 tests). The load-bearing one is a differential table asserting the rewritten root accepts exactly what the union accepted, across valid branches, option-count boundaries, undeclared root/nested properties, cross-branch field leaks, a bare discriminator, and a container sent as a string.Regression check
This change sits in the adapter every tool passes through, so I ran the suites on this branch and on
mainand compared the failing file sets rather than reading a raw pass count.main@bastani/atomicpackagetest:unitThe package suite'''s failing set was identical to
main— every one of those files fails onmaintoo, and this branch adds 8 passing tests and no new failure.Four files have appeared as branch-only failures across runs. All four are pre-existing flakes on this machine, not regressions:
test/unit/status-writer.test.ts,test/unit/interactive-engine-generation-lifecycle.test.ts-> pass in isolation, 2 files / 19 tests passed.test/suite/regressions/1223-startup-lazy-builtins.test.ts,test/suite/regressions/1704-lazy-tool-lifecycle-round2.test.ts-> pass in isolation, 2 files / 17 tests passed. These two do exercise tool registration, so I checked them specifically; under full-suite load they fail withspawnSync bun ETIMEDOUT, an environment timeout rather than an assertion.registerTool,parameters:,createStructuredOutputTool,wrapToolDefinition, orType.Union, except the two lifecycle fixtures noted above, which pass when not competing for the machine.This is a Windows dev box, not CI; trust CI over these numbers for the pre-existing failures.
Breaking changes
None. Object-rooted schemas — every built-in and every documented example — take an identity path and are byte-identical to today. Only union roots change, and they change from "uncallable" to "callable with unchanged validation semantics".
Follow-ups
Two upstream defects in
@earendil-works/pi-aiare described in #2189 and are not addressed here, because this fix makes them unreachable from Atomic:convertToolscould mergeanyOfbranch properties itself; note itsstrict === truepath spreads the legacy schema last, clobberingproperties/requiredback to empty.validateToolArgumentsdiscards the return value ofValue.Convert, which is the reason union roots get no coercion of their own.Greptile Summary
This change rewrites object-branch union tool schemas into provider-advertisable object roots while retaining the original union branches for validation.
The exercised failure hypothesis—that merging branch properties would broaden or otherwise alter validation for branch-specific required fields, discriminator literals, mixed open/closed object branches, conflicting property shapes, or nested coercion—was disproved by executed before/after checks. The normalized schema exposed provider-facing properties, preserved all tested acceptance and rejection outcomes, and coerced nested values as intended. The focused coding-agent test file passed all 8 tests.
Confidence Score: 5/5
The PR is safe to merge; no blocking failure remains.
Focused runtime verification showed that provider-facing parameter advertisement and nested coercion work while the tested union validation behavior remains unchanged.
What T-Rex did
Reviews (3): Last reviewed commit: "fix(tools): object-root union tool param..." | Re-trigger Greptile