Repository navigation
fix: pass structured arguments to shared Go and Make MCP tools - #67894
Conversation
Preserve the string args contract while handling POSIX quoting with shlex and executing an explicit argv without shell expansion. Add regression tests and regenerate importing workflow locks. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Replace string parsing and Python with JavaScript execFile and validated string arrays. Update all authored caller examples and regenerate workflow locks, with executable regression coverage for literal arguments and errors. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
pelikhan
changed the title
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ PR Code Quality Reviewer completed the code quality review. No GitHub write applied yet while investigating safeoutputs write approval failure.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
There was a problem hiding this comment.
🟡 Changes recommended
The generated schemas omit string item constraints for both argument arrays.
1 open finding
What changed in this PR
Migrates shared Go/Make MCP tools from shell-split strings to literal argument arrays, preventing globbing and quoting errors.
Changes:
- Uses Node
execFilewith validated string arrays. - Updates callers, documentation, generated locks, and regression tests.
- Preserves existing permissions, dependencies, and network access.
| File | Description |
|---|---|
.github/workflows/shared/go-make.md |
Defines array-based Go/Make handlers. |
.github/workflows/smoke-claude.md |
Updates Make invocation syntax. |
.github/workflows/smoke-claude.lock.yml |
Recompiles the smoke workflow. |
.github/workflows/daily-cli-performance.md |
Updates MCP examples. |
.github/workflows/daily-cli-performance.lock.yml |
Recompiles the performance workflow. |
pkg/workflow/shared_go_make_test.go |
Tests literal arguments and failures. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (>100 new lines in 📄 Draft ADR committed:
📊 Why this gate triggered
🏗️ Decision captured in the draftDecision: change the Alternatives documented: quote/split the string in Bash ( Key trade-offs: literal argument passing and fast-fail validation, at the cost of a breaking caller contract, loss of shell features (pipes, redirection, globs) through these tools, and a Node runtime requirement on the runner. 📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
The structured-argv switch is directionally right, but I found two follow-up gaps before this is truly finished: the diagnostics path still assumes args is a string, and the published MCP schema now accepts arrays that the handler will still reject at runtime.
Themes
- update tool-call/failure reporting helpers to render
args: string[]instead of dropping or blobifying the command preview - tighten the generated input schema so it actually describes
string[], or keep a compatible surface until it can
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 63 AIC · ⌖ 5.57 AIC · ⊞ 19.7K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd (per pr-triage classification: bug fix). No blocking issues found.
📋 Analysis
Root cause addressed, not just symptom
The old run: wrapper interpolated $INPUT_ARGS unquoted into go $INPUT_ARGS / make $INPUT_ARGS, which is exactly the SC2086 class of bug (word-splitting, globbing, injection via shell metacharacters). The fix removes the shell entirely — execFile("go", args, ...) passes each array element as a literal argv entry with no shell involved — so the root cause (shell reinterpretation of agent-supplied text) is eliminated, not papered over with quoting.
Regression test quality
pkg/workflow/shared_go_make_test.go is a strong regression suite: it reads the actual go-make.md frontmatter, generates the real JavaScript handler via GenerateMCPScriptJavaScriptToolScript, and executes it as a genuine subprocess against a stub go/make binary that records process.argv. Coverage includes spaces, quotes/backslashes, empty strings, embedded whitespace, literal globs, literal shell metacharacters ($(...), backticks, ;, >, ~), invalid input shapes, non-zero exit codes, and missing executables (ENOENT). This directly exercises the injection scenarios the fix targets (e.g. test.name == "literal shell syntax" asserts injected file is never created), which is exactly the kind of instrumentation /diagnosing-bugs calls for around error/edge paths.
Consistency across the three touched Markdown docs
go-make.md, daily-cli-performance.md, and smoke-claude.md were all updated in lockstep with the new array-style examples, and both dependent .lock.yml files were recompiled — verified locally that go test ./pkg/workflow/... -run TestSharedGoMake passes cleanly against the committed HEAD.
Minor, non-blocking observation
The tool-level timeout (default 60s) is unchanged and still enforced by the shared executeProcess helper via child_process timeout option — worth being aware that long-running go test ./... or make agent-finish invocations through the MCP tool (vs. bash) could still hit that ceiling, but this is pre-existing behavior outside this PR's scope.
✅ Positive highlights
- Clean elimination of shell involvement — no
eval, no partial quoting hacks, no Python shim. - Reuses the existing shared
buildEnhancedErrorhelper for consistent error formatting (exit code/signal/stdout/stderr) across tool types. - Validates
argsshape explicitly before spawning the subprocess, failing fast with a clear message instead of a confusing shell error. - Test harness copies the real
mcp-scripts-runner.cjs/mcp_handler_process.cjs/error_helpers.cjshelpers rather than mocking them, so it validates the actual generated artifact end-to-end.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 158.2 AIC · ⌖ 14.6 AIC · ⊞ 10.3K
Comment /matt to run again
Publish array item types through parsing, imported tools, and generated schemas. Preserve literal argument boundaries in command previews and MCP log summaries, with schema and diagnostics regression coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@copilot merge main, fix conflicts, add Tess |
Reuse diagnostic sanitization for server logs and summaries, retain tool metadata, and leave protocol responses unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…rgument-wrappers # Conflicts: # .github/workflows/daily-cli-performance.lock.yml # .github/workflows/smoke-claude.lock.yml Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ppers' into pelikhan-shellcheck-argument-wrappers Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Merged |
|
🎉 This pull request is included in a new release. Release: |
Why
The shared Go/Make MCP wrappers used unquoted argument strings, causing SC2086 warnings, accidental globbing, and incorrect handling of paths with spaces. Replace the ambiguous command-string interface with explicit argument arrays, without Python or shell evaluation.
Approach
mcpscripts-goandmcpscripts-makenow requireargsarrays of strings, for example{"args":["test","./..."]}or{"args":["build","-o","path with spaces/output"]}. Existing string calls must migrate to arrays; shell quoting should not be added to array elements.execFiledirectly. Invalid inputs fail before command execution. Output and command failures use the existing MCP result shape and shared error formatter, retaining stdout, stderr, and exit-code diagnostics.This branch starts from
mainand remains independent of #67811. No Claude network/domain changes, ShellCheck suppressions,eval, Python dependency, or live workflow runs are included.Validation
make buildandmake fmtpassed.TMPDIRto avoid the existing/varversus/private/varpath assertion mismatch.make recompilecompiled all 337 workflows.make agent-report-progresspassed formatting, change-scoped lint, impacted tests, schema freshness, and workflow drift checks../gh-aw compile smoke-claude daily-cli-performance --strict --shellcheck --no-check-updatepassed ShellCheck across 160 run-step resources.The previously observed ignored-filesystem-policy and dry-run-only upload-sarif-removal warnings remain out of scope.
Security / scope note
The committed normal-mode generated locks do not add or remove secrets, actions, container pins, or redirects. Each array element is passed literally without shell expansion. No Actions or live agent workflow was dispatched.