Detect sandbox shell-expansion guard rejections; steer agents toward jq -Rs for multi-line safeoutputs bodies#52578
Conversation
…rompt guidance Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR TriageCategory: feature · Risk: medium · Priority: low · Score: 42/100
Recommended action: Adds agent guidance to detect sandbox shell-expansion guard rejections and steer toward
|
There was a problem hiding this comment.
Pull request overview
Adds detection and remediation guidance for sandbox shell-expansion guard rejections.
Changes:
- Detects guard rejection messages and emits a dedicated output.
- Adds regression tests for wrapped and embedded messages.
- Recommends heredoc plus
jq -Rsfor multiline safe-output bodies.
Show a summary per file
| File | Description |
|---|---|
actions/setup/md/mcp_cli_tools_prompt.md |
Documents the safer multiline payload pattern. |
actions/setup/js/detect_agent_errors.cjs |
Detects and reports shell-expansion guard rejections. |
actions/setup/js/detect_agent_errors.test.cjs |
Tests detection and output generation. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
| `max_cache_misses_exceeded=${results.maxCacheMissesExceeded}`, | ||
| `missing_model_pricing_error=${results.missingModelPricingError}`, | ||
| `missing_model_pricing_model_name=${results.missingModelPricingModelName}`, | ||
| `shell_expansion_guard_rejected=${results.shellExpansionGuardRejected}`, |
There was a problem hiding this comment.
Fixed in ac847b8448: the flag is now promoted to agent job outputs, passed to the conclusion job as GH_AW_SHELL_EXPANSION_GUARD_REJECTED, and classified/rendered by handle_agent_failure.cjs ahead of generic timeout handling.
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Reviewed PR #52578 diff against ponytail-review criteria (over-engineering/complexity only): the new isShellExpansionGuardRejectedError()/pattern follows the exact same one-line detector convention already used by isMaxCacheMissesExceededError and siblings, the doc addition in mcp_cli_tools_prompt.md is prose-only, and no new abstractions, dependencies, or reinvented stdlib logic were introduced. Nothing to cut.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST_CHANGES
This adds a new detector, but the new signal still dies inside the step that produced it, so the workflow-level remediation path cannot actually see or act on shell_expansion_guard_rejected.
Blocking theme
The existing plumbing clearly promotes selected detect-agent-errors outputs to job/workflow outputs and downstream env vars (max_cache_misses_exceeded, missing_model_pricing_error, etc.). This PR only changes detect_agent_errors.cjs and the prompt text; it does not update the propagation layer that exposes those values to the conclusion job/workflow callers. As shipped, you get a nicer stderr line in the setup step, but anything making retry/escalation decisions later still only sees the generic timeout path.
That means the core behavioral bug described in the PR body remains: downstream automation still cannot distinguish “retrying the same rejected shell command” from other 5-minute timeouts.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 5.51 AIC · ⌖ 6.41 AIC · ⊞ 6.7K
Comment /review to run again
| `max_cache_misses_exceeded=${results.maxCacheMissesExceeded}`, | ||
| `missing_model_pricing_error=${results.missingModelPricingError}`, | ||
| `missing_model_pricing_model_name=${results.missingModelPricingModelName}`, | ||
| `shell_expansion_guard_rejected=${results.shellExpansionGuardRejected}`, |
There was a problem hiding this comment.
This new output is never promoted beyond the detect step, so downstream jobs still only see a generic timeout and cannot act on at all.
💡 The plumbing is incomplete
This file now emits , but the repo already has a separate propagation layer that maps selected detect-step outputs into job/workflow outputs and downstream env vars (for example and ). Nothing in this PR updates that layer, so the new flag is effectively trapped inside the step that produced it.
As a result, the conclusion job and workflow callers still cannot distinguish this failure mode from the existing generic timeout classification, which undercuts the main remediation described in the PR body.
Please thread the new output through the same promotion path as the other engine-error flags before merge.
There was a problem hiding this comment.
Fixed in ac847b8448: shell_expansion_guard_rejected now follows the same generated output/env propagation path as the other engine-error flags, with lock files regenerated.
There was a problem hiding this comment.
The detection logic, regex pattern, and tests all look solid. The existing review comment at line 297 correctly identifies the only meaningful gap: shell_expansion_guard_rejected is not yet propagated to downstream workflow callers or the conclusion job.
The prompt update in mcp_cli_tools_prompt.md is clear and actionable.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 13.3 AIC · ⌖ 7.01 AIC · ⊞ 5.6K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two gaps.
📋 Key Themes & Highlights
Issues Found
-
Downstream wiring gap (
detect_agent_errors.cjs:297) —shell_expansion_guard_rejectedis emitted to$GITHUB_OUTPUTbuthandle_agent_failure.cjsnever readsGH_AW_SHELL_EXPANSION_GUARD_REJECTED. The diagnostic is invisible in the failure report seen by authors. Every peer output (GH_AW_MAX_CACHE_MISSES_EXCEEDED,GH_AW_MISSING_MODEL_PRICING_ERROR, etc.) has a corresponding env-var read + failure-context template; this one does not. -
Test spec gap (
detect_agent_errors.test.cjs:437) — The test named "only" does not assertagenticEngineTimeout: false. The fixture log includes a timed-out line that, if the timeout pattern were broadened, would silently make both flags true without failing the test.
Positive Highlights
- ✅ Excellent regex design: the two-anchor pattern correctly handles line-wrapped messages (tested explicitly)
- ✅ False-positive guards are present and well-targeted
- ✅
mcp_cli_tools_prompt.mdchange is clear, actionable, and pre-empts the retry loop at its root
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 65.3 AIC · ⌖ 8.44 AIC · ⊞ 7.3K
Comment /matt to run again
| expect(result.http400ResponseError).toBe(false); | ||
| expect(result.capiQuotaExceededError).toBe(false); | ||
| expect(result.invocationCapExceeded).toBe(false); | ||
| expect(result.shellExpansionGuardRejected).toBe(true); |
There was a problem hiding this comment.
[/tdd] The test is named "only" but does not assert agenticEngineTimeout: false. The log fixture includes ##[error]The action 'Execute GitHub Copilot CLI' has timed out after 5 minutes. — if the AGENTIC_ENGINE_TIMEOUT_PATTERN were ever broadened to match that format, both flags would be true and the test would not catch the regression.
Adding expect(result.agenticEngineTimeout).toBe(false) here makes the test a precise specification for the "only" claim and guards against that regression.
@copilot please address this.
There was a problem hiding this comment.
Fixed in ac847b8448: the "only" regression test now asserts agenticEngineTimeout remains false for the shell-expansion guard payload.
| `max_cache_misses_exceeded=${results.maxCacheMissesExceeded}`, | ||
| `missing_model_pricing_error=${results.missingModelPricingError}`, | ||
| `missing_model_pricing_model_name=${results.missingModelPricingModelName}`, | ||
| `shell_expansion_guard_rejected=${results.shellExpansionGuardRejected}`, |
There was a problem hiding this comment.
[/diagnosing-bugs] shell_expansion_guard_rejected is emitted to $GITHUB_OUTPUT but handle_agent_failure.cjs never reads GH_AW_SHELL_EXPANSION_GUARD_REJECTED from the environment, so the diagnostic does not influence the failure report or categorization surfaced to authors.
All peer outputs (GH_AW_MAX_CACHE_MISSES_EXCEEDED, GH_AW_MISSING_MODEL_PRICING_ERROR, etc.) are read in handle_agent_failure.cjs around line 3246 and fed into failure-context templates. Without a corresponding read + context block, this new detection is visible only in the step's stderr log — not in the issue/comment the author sees.
💡 Suggested follow-up
Add to handle_agent_failure.cjs:
const shellExpansionGuardRejected = process.env.GH_AW_SHELL_EXPANSION_GUARD_REJECTED === "true" && agentConclusion === "failure";...and a corresponding shell_expansion_guard_rejected.md template and context block, following the same pattern as max_cache_misses_exceeded.md.
@copilot please address this.
There was a problem hiding this comment.
Fixed in ac847b8448: handle_agent_failure.cjs now reads GH_AW_SHELL_EXPANSION_GUARD_REJECTED, adds a dedicated failure category/title/context template, and suppresses the generic timeout path when this diagnosis is present.
🧪 Test Quality Sentinel Report
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 100/100. 0% implementation tests (threshold: 30%). All 9 new tests for SHELL_EXPANSION_GUARD_REJECTED_PATTERN cover behavioral contracts — exact match, embedded log, case-insensitivity, multi-line anchor split, negative cases, and integration through detectErrors/buildOutputLines.
|
@copilot Quick nudge: there are blocking review findings about downstream propagation of
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Triage: feature / risk: mediumScore: 58/100 (impact+urgency+quality) · Priority: medium · Action: Detects sandbox shell-expansion guard rejections and steers agents to jq -Rs for multi-line safeoutputs bodies. CI green (29 checks). Automated PR triage — see full report issue for details.
|
Addressed in |
The sandbox's shell command-injection guard (in the external Copilot CLI) can reject legitimate multi-line
printfcalls tosafeoutputs create_discussion/create_issue, and the agent retries the identical rejected command until the step's 5-minute timeout fires — turning a benign finding into a full workflow failure.The guard's heuristic itself lives in the vendored Copilot CLI binary, outside this repo's control, so this PR addresses the parts of the remediation that are actionable here:
detect_agent_errors.cjsnow recognizes the guard's rejection message ("...could enable arbitrary code execution... rewrite the command without these expansion patterns") and surfaces it as a newshell_expansion_guard_rejectedoutput, separate from a generic timeout classification. Added unit tests covering the exact payload shape from the issue, including when the message wraps across a line break.mcp_cli_tools_prompt.mdno longer only showsprintffor constructing multi-linebodypayloads (a pattern prone to tripping the guard). It now instructs agents to write content to a temp file via heredoc and inject it withjq -Rs, and explicitly tells the agent not to retry an identical command after an expansion-pattern rejection.