Skip to content

fix(tools): object-root union tool parameter schemas so they advertise parameters - #2190

Open
elefthei wants to merge 1 commit into
bastani-inc:mainfrom
elefthei:fix/2189-object-root-tool-parameter-schemas
Open

fix(tools): object-root union tool parameter schemas so they advertise parameters#2190
elefthei wants to merge 1 commit into
bastani-inc:mainfrom
elefthei:fix/2189-object-root-tool-parameter-schemas

Conversation

@elefthei

@elefthei elefthei commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Closes #2189

Problem

A tool whose parameters schema 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.

convertTools builds the advertised input_schema from the root properties/required keywords alone. A Type.Union root 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 with STRUCTURED_OUTPUT_MISSING_ERROR after 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:

{anyOf:[A,B]}  ->  {type:"object", properties:{...merged, optional}, required:[...common], anyOf:[A,B]}

properties makes it advertisable; the retained anyOf keeps validation exactly as strict, so the set of accepted arguments is unchanged.

Applied in wrapToolDefinition — the single adapter every tool passes through (built-ins, SDK customTools, extension registerTool, 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:

  • TypeBox encodes optionality with a marker, not the required array. A raw JSON merge over-requires and rejects every branch but one, so merged properties are wrapped in Type.Optional.
  • A discriminator differs across branches. First-wins merging pins kind to 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.Convert cannot apply to a union. Note this part is currently masked: validateToolArguments skips its coerceWithJsonSchema fallback only for schemas carrying the TypeBox.Kind own-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.

npm run test --workspace=@bastani/atomic -- test/tool-parameter-schema.test.ts
  Test Files  1 passed (1)
       Tests  8 passed (8)

npm run check
  biome check --error-on-warnings .   -> clean (2376 files)
  tsc --noEmit                        -> clean
  check:shrinkwrap                    -> up to date

Regression check

This change sits in the adapter every tool passes through, so I ran the suites on this branch and on main and compared the failing file sets rather than reading a raw pass count.

suite main this branch failing only on this branch
@bastani/atomic package 13 files / 30 tests 13 files / 29 tests none
root test:unit 11 files 13 files 2, both flaky (below)

The package suite'''s failing set was identical to main — every one of those files fails on main too, 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 with spawnSync bun ETIMEDOUT, an environment timeout rather than an assertion.
  • None of the four reference registerTool, parameters:, createStructuredOutputTool, wrapToolDefinition, or Type.Union, except the two lifecycle fixtures noted above, which pass when not competing for the machine.
  • The failure set drifts run to run on identical code: the package suite reported 34, then 29, then 30 failures across three runs.

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-ai are described in #2189 and are not addressed here, because this fix makes them unreachable from Atomic:

  1. convertTools could merge anyOf branch properties itself; note its strict === true path spreads the legacy schema last, clobbering properties/required back to empty.
  2. validateToolArguments discards the return value of Value.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.

T-Rex T-Rex Logs

What T-Rex did

  • Ran the authored before/after TypeScript repro scripts against the object-branch union normalizer and verified that before normalization the provider-facing root exposed no properties and nested config.retries: "3" was not coerced, and after normalization the root exposed kind, prompt, count, config, and venue, with compiled normalized and original union schemas making identical decisions for every exercised input and nested retries coerced to integer 3; eight focused tests passed.
  • Compared the before/after normalization and confirmed that the normalization retained only universally required fields for the root and allowed the same accepted value set as the original compiled union across all tested cases; the hypothesized failure path was contradicted by the checks.

View all artifacts

T-Rex Ran code and verified through T-Rex

Reviews (3): Last reviewed commit: "fix(tools): object-root union tool param..." | Re-trigger Greptile

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 normalizeToolParameterSchema to rewrite union-rooted, object-branch-only schemas into an advertisable object-root while keeping the original anyOf for 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

@elefthei
elefthei force-pushed the fix/2189-object-root-tool-parameter-schemas branch from 58eac96 to 6043d6f Compare August 4, 2026 17:37
Copilot AI review requested due to automatic review settings August 4, 2026 17:37
Comment thread packages/coding-agent/src/core/tools/tool-parameter-schema.ts

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review details

Suppressed comments (2)

packages/coding-agent/src/core/tools/tool-parameter-schema.ts:96

  • mergeObjectBranches claims the rewritten root preserves the union's accepted value set, but if any branch allows additional properties (i.e. additionalProperties is not false), 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 before anyOf is 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.Union of 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-level properties constraints 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 with additionalProperties: 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

@elefthei

elefthei commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

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:

  • npm run check caught an import-ordering violation in the new file (import { Type, type TSchema } -> import { type TSchema, Type }). Fixed and folded into the commit, so the branch was force-updated.
  • Full results are in the body: 8/8 new tests, npm run check clean, and a branch-vs-main comparison of both suites showing no new failures attributable to this change.

…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.
Copilot AI review requested due to automatic review settings August 4, 2026 18:17
@elefthei
elefthei force-pushed the fix/2189-object-root-tool-parameter-schemas branch from 6043d6f to e7520c2 Compare August 4, 2026 18:17
@elefthei

elefthei commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto latest main (was conflicting after 0.9.12 shipped). The changelog entry moved from what is now the released [0.9.12] section into [Unreleased] -> ### Fixed; the released section is untouched, and the changelog diff against main is a pure +4/-0 addition.

Re-verified on the rebased base after a fresh npm ci:

  • npm run check -> clean (biome, tsc --noEmit, shrinkwrap)
  • new test file -> 8/8 passed

Body updated with the final regression numbers.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 merged properties at 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’s properties can 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 root properties can 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

@flora131

Copy link
Copy Markdown
Collaborator

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, main has moved and the branch now conflicts. Could you merge or rebase the latest main and obtain fresh CI?

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.

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.

Union-rooted tool parameter schemas are advertised with no parameters, making the tool uncallable

3 participants