fix: restore schema-aware probe detection for zero-arg safeoutputs tools - #49313
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the implementation label and has 63 new lines of code in business logic directories (≤100 threshold). |
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ PR Code Quality Reviewer completed the code quality review. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
There was a problem hiding this comment.
🟡 Not ready to approve
The piped sentinel test overwrites its sentinel arguments before calling the bridge, leaving that invocation path untested.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Restores schema-aware handling of empty arguments for safe-output tools.
Changes:
- Allows zero-input and optional-only tools to reach MCP
tools/call. - Adds bridge and compiler contract tests.
- Adds a patch changeset.
File summaries
| File | Description |
|---|---|
actions/setup/js/mcp_cli_bridge.cjs |
Checks required schema fields before showing help. |
actions/setup/js/mcp_cli_bridge.test.cjs |
Adds guard and MCP routing tests. |
pkg/workflow/safe_outputs_tools_meta_integration_test.go |
Verifies zero-input compiled schemas. |
.changeset/fix-zero-arg-safeoutputs-tools.md |
Documents the patch fix. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| fsReadSyncSpy.mockImplementationOnce(() => 0); // EOF on second read | ||
|
|
||
| try { | ||
| setupMainCall(zeroInputTools, ["dispatch_code_factory"]); |
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report
📊 Metrics (5 tests)
📝 Inflation NoteThe 12.4:1 JS test-to-prod ratio is justified: the new Verdict
References:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Review: schema-aware probe detection fix
The core fix in shouldShowToolHelpForEmptyArgs is correct and well-scoped. The function signature update, null-safe required array check, and the Go integration test that pins the compiler/bridge contract are all solid.
One non-blocking issue flagged inline — the piped {} via . sentinel test has a duplicate setupMainCall inside its try block that replaces the outer one (losing the . sentinel), making the piped-stdin routing untested as written. The readStdinSync spy also likely won't intercept the CJS closure binding. Suggest simplifying or removing that test in favour of the already-correct bare-invocation test.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 25.4 AIC · ⊞ 5.3K
| await main(); | ||
|
|
||
| const toolsCallBody = recordedBodies.find(b => b.method === "tools/call"); | ||
| expect(toolsCallBody).toBeDefined(); |
There was a problem hiding this comment.
The setupMainCall inside the try block overwrites process.argv without the . sentinel, making the outer setupMainCall(zeroInputTools, ["dispatch_code_factory", "."]) dead code — hasStdinJsonPayload will not see . and the piped-stdin path is not exercised as intended.
Additionally, vi.spyOn(require("./mcp_cli_bridge.cjs"), "readStdinSync") is unlikely to intercept the binding captured inside main() (CJS closure), which is why the test uses mockRestore?.(). The test only exercises the fs.readSync mock, not true stdin routing.
Suggested fix: remove the duplicate setupMainCall inside try (keep only the one with ["dispatch_code_factory", "."] before the spy setup), or simplify the test to trust the bare-invocation case already covers zero-arg routing.
@copilot please address this.
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two test correctness issues.
📋 Key Themes & Highlights
Key Themes
- Vacuous test: the "piped stdin" end-to-end test has a duplicate
setupMainCallthat overwrites the.-sentinel argv beforemain()runs, so the piped-stdin code path is never actually exercised. - Dead spy: the
vi.spyOn(require('./mcp_cli_bridge.cjs'), 'readStdinSync')call cannot intercept the internal CJS binding thatmain()holds — the spy does nothing, and themockRestore?.()guard suggests the author sensed something was off.
Positive Highlights
- ✅ Root cause is correctly identified and the fix is minimal and precise.
- ✅ Schema-aware unit tests for
shouldShowToolHelpForEmptyArgsare thorough — all boundary cases (empty schema, optional-only, empty required array, null/undefined tool) are covered. - ✅ Go integration test protects the compiler/bridge contract cleanly.
- ✅ Conservative fallback (no-required when schema is missing) is the right default.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 46.4 AIC · ⊞ 7K
Comment /matt to run again
| fsReadSyncSpy.mockImplementationOnce(() => 0); // EOF on second read | ||
|
|
||
| try { | ||
| setupMainCall(zeroInputTools, ["dispatch_code_factory"]); |
There was a problem hiding this comment.
[/tdd] The .\ sentinel set on line 1295 is overwritten by a second setupMainCall call here (without the . argument), so hasStdinJsonPayload will receive ["dispatch_code_factory"] rather than ["."] — the piped-stdin branch is never actually exercised.
💡 Suggested fix
Remove the duplicate setupMainCall call inside the try block. The process.argv was already configured at line 1295 with the . sentinel; the second call silently resets it:
// Remove this line — it replaces argv that was set with ["."]
// setupMainCall(zeroInputTools, ["dispatch_code_factory"]);
await main();Without the fix the piped-stdin assertion passes vacuously: the test reaches tools/call via the zero-arg (non-stdin) path, not via readStdinSync.
@copilot please address this.
| setupMainCall(zeroInputTools, ["dispatch_code_factory", "."]); | ||
|
|
||
| // Simulate piped `{}` via the . sentinel with stdinContent = "{}" | ||
| const readStdinSyncSpy = vi.spyOn(/** @type {any} */ require("./mcp_cli_bridge.cjs"), "readStdinSync"); |
There was a problem hiding this comment.
[/tdd] vi.spyOn on a CommonJS require() call here won't intercept the readStdinSync that main() already closed over at module-load time — so this spy is effectively dead code and the test is not actually verifying what the comment says it does.
💡 Context
In CJS modules, require() returns a cached module object. If readStdinSync is a local binding (not re-exported as an enumerable property), spying on the cached export does not affect the internal call inside main(). This means the readStdinSync used by main() will call through to the real implementation regardless of the spy.
The fsReadSyncSpy on fs.readSync does intercept the right level, which is why the test likely still passes — but the readStdinSyncSpy lines are misleading. Consider removing them or replacing this test with one that directly exercises readStdinSync independently, keeping the main()-level test simpler.
@copilot please address this.
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Verdict: Request changes
Core fix logic (checking inputSchema.required) is directionally correct, but the new end-to-end test for the piped-stdin path is broken (never exercises what it claims to), and the fallback for an unresolved tool schema silently permits invalid empty-arg calls through to tools/call.
💡 Themes
- Test validity: the "piped
{}via.sentinel" test callssetupMainCall()twice, with the second call overwriting the first's.-sentinel argv beforemain()runs — so the stdin-mocking machinery (fs.readSync mock, isTTY override, module spy) is entirely inert. The test passes for the same trivial reason as the bare-invocation test above it, giving false confidence in stdin/sentinel handling. - Source logic gap:
shouldShowToolHelpForEmptyArgstreats "tool not found in cache" (nullmatchedTool) the same as "tool found with a genuinely empty schema", both defaulting torequired = []and thus allowing the empty-arg call through. This means unknown/stale/mistyped tool names now bypass the fast local help path and silently hit the MCP server instead — reintroducing the original wasteful round-trip problem for a different trigger.
Good parts: the zero-arg-tool Go integration test (TestToolsMetaJSONCompiledWorkflowZeroArgCustomJob) is solid and correctly locks the compiler/bridge contract.
🔎 Code quality review by PR Code Quality Reviewer · auto · 65.6 AIC · ⊞ 7.8K
Comment /review to run again
| fsReadSyncSpy.mockImplementationOnce(() => 0); // EOF on second read | ||
|
|
||
| try { | ||
| setupMainCall(zeroInputTools, ["dispatch_code_factory"]); |
There was a problem hiding this comment.
This test's second setupMainCall() call silently overwrites the first (with the . sentinel), so the test never actually exercises the piped-stdin path it claims to verify.
💡 Details
Line 1295 sets process.argv to include the . sentinel and mocks fs.readSync/isTTY to simulate piped stdin. But line 1315 calls setupMainCall(zeroInputTools, ["dispatch_code_factory"]) again — with no . — right before await main(), which overwrites process.argv and creates a brand-new toolsFile (leaking the first one, since only the last toolsFile value is cleaned up in afterEach). The test therefore passes for the exact same reason as the "bare invocation" test above it (empty args routed to a zero-required-fields schema), not because of stdin/sentinel handling. The elaborate fs.readSync mock and readStdinSyncSpy are dead code with respect to what the test title promises.
Fix: remove the duplicate setupMainCall call on line 1315 so the mocked stdin path from line 1295 is what actually runs through main().
| setupMainCall(zeroInputTools, ["dispatch_code_factory", "."]); | ||
|
|
||
| // Simulate piped `{}` via the . sentinel with stdinContent = "{}" | ||
| const readStdinSyncSpy = vi.spyOn(/** @type {any} */ require("./mcp_cli_bridge.cjs"), "readStdinSync"); |
There was a problem hiding this comment.
The vi.spyOn on readStdinSync from a fresh require() never intercepts the call main() actually makes, so it's dead test scaffolding that misrepresents what's verified.
💡 Details
readStdinSync() is invoked directly inside main() as an internal CommonJS reference, not via module.exports.readStdinSync. Spying on the exported reference from a separate require("./mcp_cli_bridge.cjs") therefore cannot patch the call site main() uses — the spy is set up, never asserted on, and mockRestore?.() even hints the author suspected it might not be a real spy. Combined with the duplicate setupMainCall issue flagged above, none of this stdin-simulation machinery in the test actually does anything; it should either be removed or the module needs to expose stdin reading through an injectable seam (e.g. accept a reader function) so it can genuinely be mocked.
| // Show help only when the schema explicitly declares required fields. | ||
| // Zero-argument and optional-only tools have no required array (or an empty one) | ||
| // and must be allowed to call through to MCP so their output item is recorded. | ||
| const required = matchedTool && matchedTool.inputSchema && Array.isArray(matchedTool.inputSchema.required) ? matchedTool.inputSchema.required : []; |
There was a problem hiding this comment.
Treating an unresolved matchedTool (unknown tool name) the same as a zero-input schema means a genuinely-required-arg tool called with empty args can silently reach tools/call with {} whenever the local tool cache is stale or the name is mistyped.
💡 Details
shouldShowToolHelpForEmptyArgs defaults required to [] whenever matchedTool is null/undefined — which happens both for legitimately schema-less tools and for tools not found in the cached tools array at all (e.g. stale toolsFile, race with a newly-registered custom job, or a simple typo in toolName). In the "not found" case the caller previously got a fast, clear local help message; now it silently proceeds to a network round-trip that will likely fail server-side with a less actionable -32602, regressing exactly the wasteful-retry problem this function's docstring says it exists to prevent — just for a different trigger. Consider distinguishing "tool not found in cache" (show help / unknown command error, as showToolHelp already does) from "tool found with an empty/optional schema" (proceed to call).
|
@copilot run pr-finisher skill |
|
🎉 This pull request is included in a new release. Release: |
Zero-argument custom safe-output tools (no
inputsdeclared) were silently blocked from reaching MCPtools/call. The bridge treated every empty-args call to anysafeoutputstool as a schema-discovery probe, showing help instead of calling the tool — so no safe-output item was recorded and downstream custom jobif:conditions evaluated false.Root cause
shouldShowToolHelpForEmptyArgswas schema-blind — it returnedtruefor all empty-arg safeoutputs calls regardless of whether the tool had required fields:The compiler correctly generates
{ type: "object", properties: {}, additionalProperties: false }(norequiredkey) for zero-input jobs. The bridge just wasn't respecting it.Changes
mcp_cli_bridge.cjs—shouldShowToolHelpForEmptyArgsnow accepts the resolvedmatchedTooland inspectsinputSchema.required. Help is shown only when the array is non-empty; zero-input and optional-only tools pass through totools/callwith{}. The-32602non-retry hint for genuinely invalid calls is preserved.mcp_cli_bridge.test.cjs— replaces the old schema-blind test with a schema-aware suite covering empty schema →false, optional-only →false, missing tool →false, non-empty required →true; adds bridge-level end-to-end tests via a local in-process HTTP server asserting that bare and piped-{}invocations of a zero-input tool reachtools/callwitharguments: {}, and that a required-input tool still shows help.safe_outputs_tools_meta_integration_test.go— newTestToolsMetaJSONCompiledWorkflowZeroArgCustomJobcompiles a workflow with an inputs-omitted custom job and asserts thedynamic_toolsentry hasproperties: {}, norequiredkey, andadditionalProperties: false, protecting the compiler/bridge contract..changeset/fix-zero-arg-safeoutputs-tools.md— patch changeset.