Skip to content

core: the deferred tool_call bridge enforces a declaration-schema layer that 8 tool families never enforce themselves #12999

Description

@yiliang114

Summary

resolveDeferredToolCall (the deferred tool_call bridge added by PR #12901) pre-validates bridged arguments against the target's model-visible declaration schema, target.schema.parametersJsonSchema.

For any tool that does not override BaseDeclarativeTool.validateToolParams, that pre-check duplicates build()-time validation exactly — packages/core/src/tools/tools.ts:510-513 runs SchemaValidator.validate(this.schema.parametersJsonSchema, params) and nothing else before validateToolParamValues. So the pre-check is a no-op difference for those tools.

But eight tool families override validateToolParams and never run that schema layer at all — no super.validateToolParams call and no SchemaValidator.validate call:

file line
packages/core/src/tools/enter-worktree.ts 275
packages/core/src/tools/skill.ts 257
packages/core/src/tools/agent/agent.ts 1113
packages/core/src/tools/exitPlanMode.ts 466
packages/core/src/tools/exit-worktree.ts 611
packages/core/src/tools/team-plan-approval.ts 153
packages/core/src/tools/askUserQuestion.ts 341
packages/core/src/tools/todoWrite.ts 677

For those eight, the bridge becomes the first and only place the declaration schema is enforced. A bridged call can therefore be refused for a parameter the target itself accepts, which is exactly the outcome the pre-check's own rationale comment says to avoid (tool-call.ts: "running it here would refuse calls the very next stage accepts").

Reproduce with:

grep -rn "override validateToolParams" packages/*/src --include=*.ts | grep -v "\.test\.ts"
# then, for each hit, check whether the body calls super.validateToolParams or SchemaValidator.validate

Concrete failure: bridged agent fork refused in a non-team session

Observed as an A/B against the merge base during review of PR #12901 (head 4c7ebf961c), identical input on both arms:

BASE: declaration has `name` property : false
BASE: description tells model to pass : true
BASE: tool.validateToolParams(args)  : null
BASE: bridged call -> RESOLVED -> agent
PR  : declaration has `name` property : false
PR  : description tells model to pass : true
PR  : validateToolParams(args)       : null
PR  : bridged call -> REFUSED [invalid_tool_params]: ... params must NOT have additional properties.

The mechanism, all in packages/core/src/tools/agent/agent.ts:

  1. AgentTool's parameter schema is additionalProperties: false (:873).
  2. updateDescriptionAndSchema() deletes properties.name whenever isAgentTeamEnabled() is false (:1091-1095).
  3. The same method builds the ## When to fork instruction "Pass a short `name` (one or two words, lowercase) so the user can track the fork." unconditionally (:1056), while a sibling name sentence in the same method is gated on teamEnabled (:997-1002, with a comment recording that sending it ungated was itself a prior bug).
  4. AgentTool.validateToolParams (:1113+) has no super call and no SchemaValidator call, so it tolerates name; execution merely logs "Ignoring teammate name ... because no team is active" (:2568-2571), and the comment above it states this tolerance is deliberate ("prompts may still pass it without a team").

So in a non-team session where settings.tools.eager demotes agent to deferred+hidden (making the bridge its only invocation path), the model reads a description telling it to pass name beside a schema that has no name property, bridges {description, prompt, subagent_type:'fork', name:'docs'}, and the call is refused. Because an additionalProperties refusal names no key, the model has nothing to remove and no way to repair the call; each refusal also accrues a strike toward VALIDATION_RETRY_LOOP_THRESHOLD. The identical direct call succeeds.

The description/schema contradiction in step 3 exists on both arms, so the divergence is introduced by the pre-check alone.

Why this is not fixed inside PR #12901

Two candidate shapes were reviewed there and both are design decisions rather than mechanical fixes:

The deeper problem is the one review entry R3-11 on that PR named: the bridge has no way to learn a target's real contract except by enumerating families at the pre-check site. Every future target that completes, projects or relaxes its arguments between resolution and build() needs a new exemption there, discovered by whoever ships the breakage. Two review rounds produced three such cases (omni media-policy's lockedArguments projection, truncation-completed arguments, and AgentTool's unenforced declaration), and the third shipped as a fails-closed regression.

Proposed direction

Consolidate at the build path, as the PR author proposed during round 3: let the stage that owns the target's contract produce the refusal, and let a target declare its own enforcement rather than having the pre-check model it per family. Whichever shape is chosen, the acceptance bar is:

  • a bridged call must not be refused for a parameter the target's own validateToolParams accepts;
  • Deferred tool_call schema allows empty arguments for tools with required fields #12889's behaviour must be preserved — an empty arguments object that misses required target fields is still refused at the bridge with a message naming the target and the missing field (pinned by refuses an empty arguments object that misses required target fields in packages/core/src/tools/tool-call.test.ts);
  • the pre-check must stay at the schema layer only, leaving value-level rules to build() (pinned by pre-checks only the schema layer, leaving value-level rules to build(), which asserts expect(valueRuleSpy).not.toHaveBeenCalled()).

Source

Review threads R4-1 (Critical, fails-closed regression) and R3-11 (Suggestion, re-asserted after a round-3 decline) on PR #12901. Filed so the contract-ownership mechanism has a home outside that PR's scope; that PR keeps the schema-layer pre-check as shipped.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

category/toolsTool integration and executionneed-discussionpriority/P2Medium - Moderately impactful, noticeable problemscope/corestatus/ready-for-humanSpecified but requires human judgment to implement; not suitable for an autonomous agenttype/bugSomething isn't working as expected

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions