Validate client tool input against the tool's schema - #330684
Validate client tool input against the tool's schema#330684Ryan Ewen (RyanEwen) wants to merge 3 commits into
Conversation
`createBrowserPageLink` takes `string | URI`, and when it receives `undefined` the `typeof pageId === 'string'` check is false, so the value falls through to `pageId.toString()` and throws "Cannot read properties of undefined (reading 'toString')". All ten browser tools call it from `prepareToolInvocation` without a guard, so any of them invoked with empty parameters throws before the tool's own validation runs. The tools already handle the case correctly in `invoke` (for example `clickBrowserTool` returns an error result when `pageId` is absent), so the guard exists one layer too late: throwing out of `prepareToolInvocation` leaves the tool call awaiting a completion that never arrives. Return the plain unlinked label when there is no page id, so the call reaches `invoke` and is reported as a normal tool error.
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: Kyle Cutler (@kycutler)Matched files:
Joaquín Ruales (@jruales)Matched files:
|
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull request overview
Prevents browser tool preparation from throwing when pageId is missing.
Changes:
- Returns an unlinked browser label for missing page IDs.
- Adds regression coverage for empty tool parameters.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
browserToolHelpers.ts |
Adds the missing-page fallback. |
browserToolHelpers.test.ts |
Tests helper and tool behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Trim the helper's JSDoc to the contract itself; the disconnect sequence that produces empty parameters belongs in the PR description rather than coupling a generic helper to agent-host details. Assert the exact `toolResultError` in the invoke test. The previous assertion only checked that a result object was returned, which a success would also satisfy.
|
Related: these three share a root cause. Noting it so they can be triaged together rather than as three unrelated patches. The workbench executes a client tool off the stream-mapper
If client tools executed only on the SDK-driven invocation, with the stream-mapper ready used for rendering only, all three would be structurally impossible: one authoritative trigger, never without real input, and registration ordered before execution. I have not proposed that as a PR because I do not know whether the stream-mapper ready is deliberately actionable. Starting the tool as soon as its arguments finish streaming, instead of waiting for the SDK round trip, is a plausible latency win, and the permission flow may already depend on the current ordering. That is a call for whoever owns the design. Each of the three fixes a reproducible bug on its own and carries a test that fails without it, so they are safe to land in the meantime. If the structural fix is preferred, #330683 and #330684 can reasonably be closed in its favour; #330730 is worth keeping either way, since buffering a result that arrives before its handler is the same defensive behaviour |
|
Hi Ryan Ewen (@RyanEwen), can you open an issue with the above findings? Then we can triage it through the normal process. At a glance it's likely that a better fix would be somewhere within the agent host or protocol -- e.g. for this issue, performing validation of the arguments against the tool schema should catch an undefined page ID. |
|
Opened #330899 with the findings. Included your point about validating arguments against the tool schema: that looks like the right boundary for the empty- The open design question there is whether the stream-mapper ready is meant to be actionable at all, or whether it should drive rendering only and leave execution to the SDK invocation. |
A client tool call whose arguments never streamed reaches the workbench
with no toolInput, which resolveToolInput turns into '{}'. That empty
object then looks like valid parameters, so the tool runs with nothing:
the browser tools throw during preparation because pageId is undefined,
and every one of them declares it required.
Check the parsed parameters against the tool's own inputSchema at the
single point a client tool is invoked, and fail the call with an
attributable error when a required property is absent. The model sees
why the call failed instead of the tool crashing or acting on empty
input, and this covers every client tool that declares a schema rather
than one helper.
Replaces the defensive guard in createBrowserPageLink, whose signature
goes back to requiring a page id.
Ryan Ewen (RyanEwen)
changed the title
|
Reworked along the lines suggested: validation now happens against the tool's Two things the code confirmed while doing it:
Validation is at As noted on #330899, this covers the payload class only. The double invocation (#330683) and the dropped result (#330730) are ordering problems and are unaffected. |
Problem
A client tool call whose arguments never streamed reaches the workbench with no
toolInput.resolveToolInputturns that into the string'{}'(agentHostSessionHandler.ts:446-449), which parses into an empty object and is then handed to the tool as ordinary parameters. The tool runs with nothing.For the browser tools that means
pageIdisundefinedand preparation throws, which is what this PR originally reported. All nine of them declarerequired: ['pageId'], so the input was already known to be invalid before the tool was ever called.Fix
Validate the parsed parameters against the tool's own
inputSchemaat_executeClientTool, documented as "the one place a client tool is actually invoked", and fail the call when a declared required property is absent.The failure goes through the existing
fail(...)path, so the model receives an attributableinvalidInputerror naming the missing property rather than a crash or a silently degraded run. Tools that declare no schema, or no required properties, are unaffected.This supersedes the first revision of this PR, which made
createBrowserPageLinkreturn a default label for a missing page id. That guarded one call site and still let the invalid invocation proceed; the helper's signature is restored to requiring a page id.Suggested by Kyle Cutler (@kycutler) in review. Root cause of the empty-argument path is tracked in #330899, along with two sibling defects from the same seam (#330683, #330730) that this does not address, since those are ordering rather than payload problems.
Verification
New test in
agentHostClientTools.test.ts: a tool declaringrequired: ['pageId'], driven withtoolInput: '{}', is not invoked and completes withsuccess: falseand codeinvalidInputnaming the property. Verified to fail without the change (the tool is invoked with empty parameters).1128 agent-session tests and 143 browserView tests pass.