Expose schema-derived safeoutputs CLI syntax and add synchronous recursive argument validation - #46959
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…sted strict labels Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (3 additions found). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Adds schema-derived safeoutputs CLI guidance and recursive declaration-time validation.
Changes:
- Shares schema rendering between runtime prompts and CLI help.
- Validates nested schema constraints before handler execution.
- Adds strict-label errors and mode-specific tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/mcp_cli_mount.go |
Sources generated CLI documentation. |
pkg/workflow/mcp_cli_mount_test.go |
Tests prompt output wiring. |
actions/setup/md/mcp_cli_tools_prompt.md |
Updates runtime CLI guidance. |
actions/setup/js/mount_mcp_as_cli.cjs |
Generates mounted-server documentation. |
actions/setup/js/mount_mcp_as_cli.test.cjs |
Tests prompt documentation generation. |
actions/setup/js/mcp_server_core.cjs |
Adds pre-handler schema validation. |
actions/setup/js/mcp_server_core.test.cjs |
Tests synchronous label rejection. |
actions/setup/js/mcp_scripts_validation.cjs |
Implements recursive schema validation. |
actions/setup/js/mcp_scripts_validation.test.cjs |
Tests nested validation and errors. |
actions/setup/js/mcp_cli_schema_docs.cjs |
Implements shared schema documentation rendering. |
actions/setup/js/mcp_cli_schema_docs.test.cjs |
Tests mode-specific syntax. |
actions/setup/js/mcp_cli_bridge.cjs |
Reuses schema renderer for help. |
.github/workflows/smoke-call-workflow.lock.yml |
Refreshes compiled workflow output. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 13/13 changed files
- Comments generated: 3
- Review effort level: Medium
| if (schemaAllowsType(schema, "integer") || schemaAllowsType(schema, "number")) { | ||
| return "<number>"; | ||
| } |
There was a problem hiding this comment.
Addressed in commit 3994799: mixed scalar placeholders are now preserved in signatures (e.g. <number|string>), so temporary-ID string alternatives are shown in the final contract.
| const required = new Set(Array.isArray(chosenSchema.required) ? chosenSchema.required : []); | ||
| const result = {}; | ||
| const keys = Object.keys(properties); | ||
| for (const propertyKey of keys) { | ||
| if (required.has(propertyKey) || propertyKey === "rationale" || propertyKey === "confidence") { | ||
| result[propertyKey] = exampleValueForKey(propertyKey, properties[propertyKey]); | ||
| } |
There was a problem hiding this comment.
Addressed in commit 3994799: disabled-mode add_labels recommendations now omit nested intent fields unless they are required by the final schema, and coverage was added in mcp_cli_schema_docs.test.cjs.
| const selectedKeys = new Set(); | ||
| Object.keys(properties).forEach(key => { | ||
| if (required.has(key) || key === "rationale" || key === "confidence") { | ||
| selectedKeys.add(key); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Addressed in commit 3994799: assign_milestone now carries alternative requirements in schema (anyOf), and recommended examples include a valid milestone argument branch.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Expose schema-derived safeoutputs CLI syntax + recursive validation
Overall this is a clean, well-tested implementation. The architecture is sound — a shared renderer module (mcp_cli_schema_docs.cjs) eliminates drift between --help output and the runtime prompt, and the recursive validation in mcp_scripts_validation.cjs correctly gates bad payloads before handler execution.
One minor correctness note (non-blocking) documented as an inline comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 62.2 AIC · ⌖ 4.46 AIC · ⊞ 4.9K
| function formatSchemaValidationError(toolName, args, error) { | ||
| if (toolName === "add_labels" && typeof error?.path === "string" && /^labels\[\d+\]$/.test(error.path) && Array.isArray(args?.labels)) { | ||
| const index = Number(error.path.match(/^labels\[(\d+)\]$/)?.[1] || -1); | ||
| const receivedLabel = index >= 0 ? args.labels[index] : undefined; |
There was a problem hiding this comment.
The add_labels special-case error message hard-codes "when issue-intent is enabled" regardless of whether issue-intent mode is actually active at runtime. If a caller passes labels: [42] (a number, not a string) outside issue-intent mode, the special branch is skipped (only triggers for typeof receivedLabel === "string"), so only the generic fallback fires — that part is fine. But if someone eventually passes a string label outside issue-intent mode, the error message would mislead them about the root cause.
Consider either:
- Passing the issue-intent flag into
formatSchemaValidationErrorso the message is mode-aware, or - Softening the phrase to
"labels[n] must be an object (string shorthand is not supported)".
Non-blocking, but worth addressing before the helper is reused for other label schemas. @copilot please address this.
There was a problem hiding this comment.
Addressed in commit 3994799: the strict-label type error text was made mode-agnostic (string shorthand is not supported) so it no longer implies issue-intent mode.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 86/100 — Acceptable
📊 Metrics (10 tests)
Per-Test Classification:
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on test coverage gaps and two coupling concerns.
📋 Key Themes & Highlights
Key Issues
isScalarSchemablind tooneOf/anyOf— triggers spurious JSON-pipe mode for all-scalar polymorphic fields (comment on line 29)- Magic string coupling —
scalarPlaceholder,exampleValueForKey, andrenderToolRecommendedExamplehard-code safeoutputs field names (rationale,confidence,labels,suggest,agent,issue_type), making the renderer hard to generalise (comments on lines 50 and 104) formatSchemaValidationErrorper-tool branching — starts accumulating tool-specific branches; schema already contains the data needed to generate the error without hard-coding (comment on line 280)- Missing tests:
oneOfmulti-match case,skipRequiredAtRootintent, andrenderToolRecommendedExamplewith a schema that has norationale/confidencefields (comments on lines 192, 276, 175)
Positive Highlights
- ✅ Excellent idea: single shared renderer eliminates
--helpvs. runtime-prompt drift - ✅ Recursive synchronous validation is well-structured and clearly separates
oneOf,anyOf, type, enum, and property checks - ✅ Test coverage is broad for the new paths — the gaps flagged above are edge cases rather than missing feature coverage
- ✅ Go side change is minimal and correct: a single env-var substitution wires the new output in without scope creep
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 88.3 AIC · ⌖ 5.32 AIC · ⊞ 6.7K
Comment /matt to run again
| return false; | ||
| } | ||
|
|
||
| function isScalarSchema(schema) { |
There was a problem hiding this comment.
[/codebase-design] isScalarSchema does not unwrap oneOf/anyOf, so a property that holds oneOf: [{type:"string"}, {type:"number"}] will be treated as non-scalar and force the whole tool signature into JSON-pipe mode even when both branches are scalar.
💡 Suggestion
Delegating to chooseExampleSchema (already available) or checking whether all branches are scalar would fix this:
function isScalarSchema(schema) {
if (!schema || typeof schema !== "object") return false;
if (Array.isArray(schema.enum) && schema.enum.length > 0) return true;
if (Array.isArray(schema.oneOf) && schema.oneOf.length > 0)
return schema.oneOf.every(s => isScalarSchema(s));
if (Array.isArray(schema.anyOf) && schema.anyOf.length > 0)
return schema.anyOf.every(s => isScalarSchema(s));
return schemaAllowsType(schema, "string") || schemaAllowsType(schema, "number") ||
schemaAllowsType(schema, "integer") || schemaAllowsType(schema, "boolean");
}A test covering oneOf: [{type:"string"}, {type:"number"}] would lock this in.
@copilot please address this.
There was a problem hiding this comment.
Addressed in commit 3994799: isScalarSchema now unwraps oneOf/anyOf and treats all-scalar unions as scalar, with test coverage in mcp_cli_schema_docs.test.cjs.
| return "<number>"; | ||
| } | ||
| if (schemaAllowsType(schema, "string")) { | ||
| if (paramName === "rationale") { |
There was a problem hiding this comment.
[/codebase-design] scalarPlaceholder has hardcoded checks for "rationale", "issue_type", and later "suggest" / "confidence" / "agent" — coupling the renderer to safeoutputs-specific field names. This leaks domain knowledge into a generic rendering utility.
💡 Suggestion
Consider driving these hints from schema metadata instead of field names (e.g. a x-placeholder extension field, or relying purely on maxLength/description). At minimum, extract the special-cased names into a named constant so they are visible as configuration rather than magic strings.
@copilot please address this.
There was a problem hiding this comment.
No code change in this pass. I kept the current field-name hints because this renderer is intentionally specialized for safeoutputs docs in this PR; generalizing via schema metadata/extension keys is a larger refactor that should be handled separately.
| } | ||
| errors.push(error); | ||
| } | ||
| if (successCount !== 1) { |
There was a problem hiding this comment.
[/tdd] When successCount > 1 (value matches more than one oneOf branch), the code falls into the successCount !== 1 branch with an empty errors array and returns "must match exactly one schema option" — but then discards which branches matched. This is the correct JSON Schema semantics, but it is untested: there is no test for the multi-match case.
💡 Suggested test
it("rejects a value that matches multiple oneOf branches", () => {
const schema = {
type: "object",
properties: {
x: { oneOf: [{ type: "string" }, { type: "string", maxLength: 10 }] },
},
};
const result = validateArgumentsAgainstSchema({ x: "hi" }, schema);
expect(result).toMatchObject({ path: "x", message: /exactly one/ });
});@copilot please address this.
There was a problem hiding this comment.
No code change in this pass. validateArgumentsAgainstSchema intentionally returns the generic oneOf exclusivity message without branch details; adding multi-match branch diagnostics/tests is a reasonable follow-up but out of scope for this PR’s targeted fix set.
| } | ||
|
|
||
| function formatSchemaValidationError(toolName, args, error) { | ||
| if (toolName === "add_labels" && typeof error?.path === "string" && /^labels\[\d+\]$/.test(error.path) && Array.isArray(args?.labels)) { |
There was a problem hiding this comment.
[/codebase-design] formatSchemaValidationError is hard-coded to the add_labels tool name and labels[n] path pattern. As more strict-mode tools are added, this function will accumulate more per-tool branches, becoming a maintenance liability.
💡 Suggestion
Consider a registration pattern where each tool supplies its own error formatter, or extract the error-enrichment data (expected shape, required fields) from the schema itself rather than hard-coding it:
// The expected shape is already in the schema — read it out:
const itemSchema = inputSchema?.properties?.labels?.items;
if (itemSchema) {
const expectedKeys = itemSchema.required?.join(", ");
// build message from schema, no per-tool branching needed
}@copilot please address this.
There was a problem hiding this comment.
No code change in this pass. The add_labels-specific enrichment remains intentionally scoped to the strict-label error path introduced here; broader schema-driven formatter registration is a separate refactor.
| const required = new Set(Array.isArray(schema.required) ? schema.required : []); | ||
| const selectedKeys = new Set(); | ||
| Object.keys(properties).forEach(key => { | ||
| if (required.has(key) || key === "rationale" || key === "confidence") { |
There was a problem hiding this comment.
[/tdd] renderToolRecommendedExample includes rationale and confidence in the example output by name-matching alone (line 192), independent of whether the schema marks them required. The test suite does not cover a schema where rationale/confidence are absent entirely — which would silently produce an example that references non-existent fields.
💡 Suggested test
it("omits rationale/confidence when not in schema properties", () => {
const tool = {
name: "noop",
inputSchema: {
type: "object",
properties: { message: { type: "string" } },
required: ["message"],
},
};
const example = renderToolRecommendedExample("safeoutputs", tool);
expect(example).not.toContain("rationale");
expect(example).not.toContain("confidence");
});@copilot please address this.
There was a problem hiding this comment.
Addressed in commit 3994799: recommended examples now only include rationale/confidence when those properties are actually present in the chosen schema branch, with coverage added for the disabled-label case.
| } | ||
|
|
||
| function validateArgumentsAgainstSchema(args, inputSchema) { | ||
| return validateSchemaNode(args, inputSchema, "", { skipRequiredAtRoot: true }); |
There was a problem hiding this comment.
[/tdd] validateArgumentsAgainstSchema passes skipRequiredAtRoot: true, which means a call missing a top-level required field (e.g. add_labels without item_number) silently passes validation. There is no test asserting this boundary — so if the intent changes the omission is invisible.
💡 Suggested test
Add a test documenting the explicit intent:
it("does not reject missing top-level required fields (enforced elsewhere)", () => {
const schema = {
type: "object",
properties: { item_number: { type: "integer" } },
required: ["item_number"],
};
// missing item_number at root is intentionally skipped
expect(validateArgumentsAgainstSchema({}, schema)).toBeNull();
});@copilot please address this.
There was a problem hiding this comment.
No code change in this pass. Root-level required omission is currently intentional (skipRequiredAtRoot: true) because top-level required-field enforcement happens in declaration handling; I left this behavior unchanged.
| return 1; | ||
| } | ||
| if (schemaAllowsType(chosenSchema, "array")) { | ||
| const itemSchema = chooseExampleSchema(chosenSchema.items, key === "labels"); |
There was a problem hiding this comment.
[/tdd] exampleValueForKey has a special case key === "labels" passed down to chooseExampleSchema and used again when recursing for array items (key === "labels" ? "label" : key). This hard-coding means any future array field named something other than "labels" that also needs the object variant will silently get the wrong branch. The current tests do not cover a non-labels array of objects to confirm the fallback behavior.
💡 Suggestion
Pass preferObject as a function option rather than inferring it from the key name, so callers can opt-in explicitly rather than relying on a magic string comparison.
@copilot please address this.
There was a problem hiding this comment.
No code change in this pass. I kept the labels prefer-object heuristic because this PR targets strict add_labels guidance specifically; converting to a generic caller-driven option would be a broader API refactor.
There was a problem hiding this comment.
Review: schema-derived CLI docs + recursive validation
The core mechanics are sound — the shared renderer approach is clean and the recursive validator is a real improvement over the previous string-only checks. However, there are a few correctness and robustness gaps worth addressing before this ships.
Findings summary (5 issues)
High
renderToolRecommendedExampleembeds JSON inside single-quotedprintfwith no escaping — a future string value containing'will produce a broken shell snippet directly in the agent prompt.
Medium
GH_AW_MCP_CLI_SERVERS_LISTnow holds a raw${{ ... }}Actions expression instead of a concrete value. The Go-layer test only asserts the raw token, providing zero coverage of actual rendered output. A wrong step ID or skipped step silently injects the raw expression into the prompt with no error.validateSchemaNodehas no recursion depth guard. It is now in the hot path of every tool call; a malformed/deeply-nested schema can crash the MCP server.
Low
formatSchemaValidationErrorregex/^labels\[\d+\]$/misses nested field paths likelabels[0].confidence, producing a generic error when the more helpful guidance should fire.isScalarSchemaignoresoneOf/anyOf, causing tools with union-scalar properties to unnecessarily use JSON mode in docs.
🔎 Code quality review by PR Code Quality Reviewer · 86.7 AIC · ⌖ 5.23 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/mcp_cli_schema_docs.cjs:315
Single-quote injection in generated printf command: if any example string value contains a single quote, the emitted shell snippet will be syntactically broken.
<details>
<summary>💡 Details and fix</summary>
renderToolRecommendedExample produces:
printf '%s' '<json>' | safeoutputs ...
The JSON payload is embedded inside single quotes. If exampleValueForKey ever returns a string containing ' (e.g. "The report's issue"), the shell string is terminated early, producing a …
pkg/workflow/mcp_cli_mount.go:998
GH_AW_MCP_CLI_SERVERS_LIST now holds a raw Actions expression, making correctness unverifiable at the Go layer: the literal string ${{ steps.mount-mcp-clis.outputs.mcp-cli-servers-list }} is passed as an env var. If the step ID is wrong, the step is skipped, or the output name changes, the raw expression is injected verbatim into the agent prompt — with no error and no test to catch it.
<details>
<summary>💡 Details</summary>
Before this change, serversList was a concrete string bui…
actions/setup/js/mcp_scripts_validation.cjs:507
No recursion depth guard in validateSchemaNode: deeply nested schemas (many levels of oneOf/anyOf/properties) can exhaust the call stack with no safety net.
<details>
<summary>💡 Details and fix</summary>
The function recurses through oneOf, anyOf, object properties, and array items without any depth limit. While schemas derived from well-formed inputSchema objects are unlikely to be deeply nested in practice, a malformed or adversarially constructed schema (e.g. a schem…
actions/setup/js/mcp_scripts_validation.cjs:629
formatSchemaValidationError regex misses nested label field failures: the special-case guidance only fires for labels[N], not labels[N].confidence or labels[N].name.
<details>
<summary>💡 Details and fix</summary>
The regex /^labels\[\d+\]$/ matches only the bare index path. But validateSchemaNode recurses into object properties, so a label that passes the type check but has an invalid confidence value will produce path labels[0].confidence — which does not match the regex. Th…
actions/setup/js/mcp_cli_schema_docs.cjs:130
isScalarSchema does not inspect oneOf/anyOf, so union-typed properties incorrectly trigger full JSON mode for the entire tool.
<details>
<summary>💡 Details and fix</summary>
shouldUseJsonMode returns true if any property is not scalar. isScalarSchema only checks top-level schema.type and schema.enum. A property like { oneOf: [{type:"string"}, {type:"string"}] } has no top-level type, so isScalarSchema returns false, and the whole tool falls back to JSON-mode docs even…
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented the PR-finisher pass and pushed fixes in Key updates in this commit:
Validation run:
|
|
@copilot please run the Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
…tputs-cli-syntax Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. The branch is now up to date with I also attempted |
|
Hey This PR looks well-structured with clear improvements across CLI documentation generation, recursive validation, and error messaging. The description walks through each feature (schema-derived docs, shared renderers, nested validation, and strict error formatting) with great detail. A note on testing: The diff shows only Otherwise, this looks ready for review. ✅
|
|
@copilot merge main, recompile, fix https://github.com/github/gh-aw/actions/runs/29806993901/job/88559521291?pr=46959 |
…tputs-cli-syntax Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in
Validation run: |
PR Triage
Removes schema-discovery-by-failure for safeoutputs CLI by surfacing inputSchema-derived syntax at runtime, and closes add_labels strict-payload validation gap. 30 files +60/-30. AI reviewer commented; one APPROVED (since dismissed). Needs a focused human review of the schema-generation logic. Run §29809051050
|
|
@copilot please run the Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
|
🎉 This pull request is included in a new release. Release: |
This change removes schema-discovery-by-failure for CLI-mounted
safeoutputsby surfacing command syntax from each tool’s finalinputSchema(including issue-intent mode differences). It also closes a validation gap where strictadd_labelsnested payload errors were accepted at declaration time and only failed downstream.Schema-derived CLI docs in runtime prompt
mount_mcp_as_cli.cjsnow generatesGH_AW_MCP_CLI_SERVERS_LISTfrom mounted server tool schemas, with per-tool signatures + recommended invocations for enabledsafeoutputscommands.inputSchema(optional/required/removed intent fields are reflected automatically).mcp_cli_mount.gonow sources the prompt section fromsteps.mount-mcp-clis.outputs.mcp-cli-servers-listinstead of static server-name bullets.Single renderer for prompt +
--helpparityactions/setup/js/mcp_cli_schema_docs.cjs.mcp_cli_bridge.cjsnow uses the same renderer forsafeoutputs <tool> --help, preventing drift between runtime prompt guidance and command help.Recursive synchronous schema validation in MCP core
mcp_scripts_validation.cjs) to enforce nested JSON Schema constraints before handler execution:itemsrequiredoneOf/anyOfenumtypeadditionalPropertiesmcp_server_core.cjsbefore declaration handling, so invalid nested payloads are rejected immediately and not recorded.Strict
add_labelsactionable error pathlabels[n]