-
Notifications
You must be signed in to change notification settings - Fork 514
Detect sandbox shell-expansion guard rejections; steer agents toward jq -Rs for multi-line safeoutputs bodies #52578
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
0aebbef
2059a22
d584db5
ac847b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,14 @@ | |
| * Detected from the agent stdio log (text pattern) and the AWF firewall audit | ||
| * JSONL log (`unknown_model_ai_credits` event type). Both sources are checked | ||
| * and their results merged. | ||
| * - shell_expansion_guard_rejected: The sandbox's shell command-injection guard | ||
| * rejected a shell command for containing (or appearing to contain) bash | ||
| * expansion patterns (command substitution, indirect expansion, parameter | ||
| * transformation, etc.), e.g. "...could enable arbitrary code execution. | ||
| * Please rewrite the command without these expansion patterns." This can | ||
| * misfire on benign multi-line `printf`/`safeoutputs` CLI invocations; agents | ||
| * should switch to the `jq -Rs` file-piping pattern instead of retrying the | ||
| * same command verbatim. | ||
| * This replaces the individual bash scripts (detect_inference_access_error.sh, | ||
| * detect_mcp_policy_error.sh) with a single JavaScript step. | ||
| * | ||
|
|
@@ -173,6 +181,16 @@ const INVOCATION_CAP_EXCEEDED_PATTERN = buildCombinedPattern(MAX_RUNS_EXCEEDED_P | |
| // parseMaxCacheMissesExceededFromEventLog(). | ||
| const MAX_CACHE_MISSES_EXCEEDED_PATTERN = /(?:\bmax_cache_misses_exceeded\b|\bmaximum\s+consecutive\s+cache\s+misses\s+exceeded\b)/i; | ||
|
|
||
| // Pattern: the sandbox's shell command-injection guard rejected a shell command believed to | ||
| // contain dangerous bash expansion patterns (command substitution, indirect expansion, parameter | ||
| // transformation, backtick substitution, etc.). Observed message form: | ||
| // "...indirect expansion, or nested command substitution) that could enable arbitrary code | ||
| // execution. Please rewrite the command without these expansion patterns." | ||
| // This guard can misfire on benign multi-line printf/safeoutputs CLI invocations. Retrying the | ||
| // identical command is pointless — it will be rejected again — so this is surfaced as a distinct, | ||
| // actionable diagnostic instead of a generic shell failure. | ||
| const SHELL_EXPANSION_GUARD_REJECTED_PATTERN = /could enable arbitrary code execution\b[\s\S]{0,200}?\brewrite the command without these expansion patterns\b/i; | ||
|
|
||
| /** | ||
| * Determines if the collected output contains the observed Copilot/CAPI quota exhaustion error. | ||
| * @param {string} output - Collected stdout+stderr from the process | ||
|
|
@@ -205,6 +223,18 @@ function isMaxCacheMissesExceededError(output) { | |
| return MAX_CACHE_MISSES_EXCEEDED_PATTERN.test(output); | ||
| } | ||
|
|
||
| /** | ||
| * Determines if the collected output shows the sandbox's shell command-injection guard | ||
| * rejected a command for containing (or appearing to contain) dangerous bash expansion | ||
| * patterns. Retrying the same command verbatim will not succeed; the agent should switch | ||
| * to the `jq -Rs` file-piping pattern for multi-line safeoutputs CLI bodies instead. | ||
| * @param {string} output - Collected stdout+stderr from the process | ||
| * @returns {boolean} | ||
| */ | ||
| function isShellExpansionGuardRejectedError(output) { | ||
| return SHELL_EXPANSION_GUARD_REJECTED_PATTERN.test(output); | ||
| } | ||
|
|
||
| /** | ||
| * Normalize model names to a single safe line for GitHub Actions outputs and issue titles. | ||
| * @param {string} value | ||
|
|
@@ -227,7 +257,7 @@ function extractMissingModelPricingModelName(logContent) { | |
| /** | ||
| * Detect known error patterns in a log string and return detection results. | ||
| * @param {string} logContent - Contents of the agent stdio log | ||
| * @returns {{ inferenceAccessError: boolean, mcpPolicyError: boolean, agenticEngineTimeout: boolean, modelNotSupportedError: boolean, http400ResponseError: boolean, capiQuotaExceededError: boolean, invocationCapExceeded: boolean, maxCacheMissesExceeded: boolean, missingModelPricingError: boolean, missingModelPricingModelName: string }} | ||
| * @returns {{ inferenceAccessError: boolean, mcpPolicyError: boolean, agenticEngineTimeout: boolean, modelNotSupportedError: boolean, http400ResponseError: boolean, capiQuotaExceededError: boolean, invocationCapExceeded: boolean, maxCacheMissesExceeded: boolean, missingModelPricingError: boolean, missingModelPricingModelName: string, shellExpansionGuardRejected: boolean }} | ||
| */ | ||
| function detectErrors(logContent) { | ||
| const missingModelPricingModelName = extractMissingModelPricingModelName(logContent); | ||
|
|
@@ -242,12 +272,13 @@ function detectErrors(logContent) { | |
| maxCacheMissesExceeded: isMaxCacheMissesExceededError(logContent), | ||
| missingModelPricingError: missingModelPricingModelName !== "", | ||
| missingModelPricingModelName, | ||
| shellExpansionGuardRejected: isShellExpansionGuardRejectedError(logContent), | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Build GitHub Actions output lines from detection results. | ||
| * @param {{ inferenceAccessError: boolean, mcpPolicyError: boolean, agenticEngineTimeout: boolean, modelNotSupportedError: boolean, http400ResponseError: boolean, capiQuotaExceededError: boolean, invocationCapExceeded: boolean, maxCacheMissesExceeded: boolean, missingModelPricingError: boolean, missingModelPricingModelName: string }} results | ||
| * @param {{ inferenceAccessError: boolean, mcpPolicyError: boolean, agenticEngineTimeout: boolean, modelNotSupportedError: boolean, http400ResponseError: boolean, capiQuotaExceededError: boolean, invocationCapExceeded: boolean, maxCacheMissesExceeded: boolean, missingModelPricingError: boolean, missingModelPricingModelName: string, shellExpansionGuardRejected: boolean }} results | ||
| * @returns {string[]} | ||
| */ | ||
| function buildOutputLines(results) { | ||
|
|
@@ -263,12 +294,13 @@ function buildOutputLines(results) { | |
| `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}`, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 incompleteThis 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/diagnosing-bugs] All peer outputs ( 💡 Suggested follow-upAdd to const shellExpansionGuardRejected = process.env.GH_AW_SHELL_EXPANSION_GUARD_REJECTED === "true" && agentConclusion === "failure";...and a corresponding @copilot please address this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in |
||
| ]; | ||
| } | ||
|
|
||
| /** | ||
| * Write GitHub Actions outputs to $GITHUB_OUTPUT. | ||
| * @param {{ inferenceAccessError: boolean, mcpPolicyError: boolean, agenticEngineTimeout: boolean, modelNotSupportedError: boolean, http400ResponseError: boolean, capiQuotaExceededError: boolean, invocationCapExceeded: boolean, maxCacheMissesExceeded: boolean, missingModelPricingError: boolean, missingModelPricingModelName: string }} results | ||
| * @param {{ inferenceAccessError: boolean, mcpPolicyError: boolean, agenticEngineTimeout: boolean, modelNotSupportedError: boolean, http400ResponseError: boolean, capiQuotaExceededError: boolean, invocationCapExceeded: boolean, maxCacheMissesExceeded: boolean, missingModelPricingError: boolean, missingModelPricingModelName: string, shellExpansionGuardRejected: boolean }} results | ||
| */ | ||
| function writeOutputs(results) { | ||
| const outputFile = process.env.GITHUB_OUTPUT; | ||
|
|
@@ -351,6 +383,11 @@ function main() { | |
| if (results.missingModelPricingError && !auditMissingPricing) { | ||
| process.stderr.write(`[detect-agent-errors] Detected missing model pricing: model "${results.missingModelPricingModelName}" has no AI credits pricing configured\n`); | ||
| } | ||
| if (results.shellExpansionGuardRejected) { | ||
| process.stderr.write( | ||
| "[detect-agent-errors] Detected sandbox shell expansion guard rejection: a shell command was rejected for dangerous bash expansion patterns; use the jq -Rs file-piping pattern for multi-line safeoutputs CLI bodies instead of retrying\n" | ||
| ); | ||
| } | ||
|
|
||
| writeOutputs(results); | ||
| } | ||
|
|
@@ -378,5 +415,7 @@ module.exports = { | |
| INVOCATION_CAP_EXCEEDED_PATTERN, | ||
| MAX_CACHE_MISSES_EXCEEDED_PATTERN, | ||
| MISSING_MODEL_PRICING_PATTERN, | ||
| SHELL_EXPANSION_GUARD_REJECTED_PATTERN, | ||
| isShellExpansionGuardRejectedError, | ||
| buildOutputLines, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,8 @@ const { | |
| INVOCATION_CAP_EXCEEDED_PATTERN, | ||
| MAX_CACHE_MISSES_EXCEEDED_PATTERN, | ||
| MISSING_MODEL_PRICING_PATTERN, | ||
| SHELL_EXPANSION_GUARD_REJECTED_PATTERN, | ||
| isShellExpansionGuardRejectedError, | ||
| extractMissingModelPricingModelName, | ||
| buildOutputLines, | ||
| } = require("./detect_agent_errors.cjs"); | ||
|
|
@@ -326,6 +328,7 @@ describe("detect_agent_errors.cjs", () => { | |
| expect(result.maxCacheMissesExceeded).toBe(false); | ||
| expect(result.missingModelPricingError).toBe(false); | ||
| expect(result.missingModelPricingModelName).toBe(""); | ||
| expect(result.shellExpansionGuardRejected).toBe(false); | ||
| }); | ||
|
|
||
| it("detects inference access error only", () => { | ||
|
|
@@ -416,6 +419,24 @@ describe("detect_agent_errors.cjs", () => { | |
| expect(result.invocationCapExceeded).toBe(true); | ||
| }); | ||
|
|
||
| it("detects shell expansion guard rejection only (issue github/gh-aw#52254 payload shape)", () => { | ||
| const log = [ | ||
| "[copilot-harness] attempt 1: shell(safeoutputs create_discussion --title 'MCP toolset unavailable' --body \"...\\n...\")", | ||
| "Command rejected: shell command contains dangerous patterns (command substitution, indirect expansion, or nested command substitution) that could enable arbitrary code execution. Please rewrite the command without these expansion patterns.", | ||
| "[copilot-harness] attempt 2: retrying identical command", | ||
| "Command rejected: shell command contains dangerous patterns (command substitution, indirect expansion, or nested command substitution) that could enable arbitrary code execution. Please rewrite the command without these expansion patterns.", | ||
| "##[error]The action 'Execute GitHub Copilot CLI' has timed out after 5 minutes.", | ||
| ].join("\n"); | ||
| const result = detectErrors(log); | ||
| expect(result.inferenceAccessError).toBe(false); | ||
| expect(result.mcpPolicyError).toBe(false); | ||
| expect(result.modelNotSupportedError).toBe(false); | ||
| expect(result.http400ResponseError).toBe(false); | ||
| expect(result.capiQuotaExceededError).toBe(false); | ||
| expect(result.invocationCapExceeded).toBe(false); | ||
| expect(result.shellExpansionGuardRejected).toBe(true); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The test is named "only" but does not assert Adding @copilot please address this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in |
||
| }); | ||
|
|
||
| it("detects both capi quota and invocation-cap flags when both signatures are present", () => { | ||
| const result = detectErrors("CAPIError: Too Many Requests\nCAPIError: 429 Maximum LLM invocations exceeded (25/25)"); | ||
| expect(result.capiQuotaExceededError).toBe(true); | ||
|
|
@@ -621,6 +642,51 @@ commentary" has no AI credits pricing`; | |
| }); | ||
| }); | ||
|
|
||
| describe("SHELL_EXPANSION_GUARD_REJECTED_PATTERN / isShellExpansionGuardRejectedError", () => { | ||
| // Exact payload shape from the issue report (github/gh-aw#52254): the sandbox's shell | ||
| // command-injection guard rejected a benign multi-line `printf` call to `safeoutputs | ||
| // create_discussion` for containing bash expansion patterns. | ||
| const ISSUE_REJECTION_MESSAGE = | ||
| "Command rejected: shell command contains dangerous patterns (command substitution, " + | ||
| "indirect expansion, or nested command substitution) that could enable arbitrary code " + | ||
| "execution. Please rewrite the command without these expansion patterns."; | ||
|
|
||
| it("matches the exact rejection message from the issue report", () => { | ||
| expect(SHELL_EXPANSION_GUARD_REJECTED_PATTERN.test(ISSUE_REJECTION_MESSAGE)).toBe(true); | ||
| expect(isShellExpansionGuardRejectedError(ISSUE_REJECTION_MESSAGE)).toBe(true); | ||
| }); | ||
|
|
||
| it("matches when embedded in larger multi-line log output", () => { | ||
| const log = [ | ||
| "[copilot-harness] attempt 1: invoking shell(safeoutputs create_discussion --title ... --body ...)", | ||
| ISSUE_REJECTION_MESSAGE, | ||
| "[copilot-harness] attempt 2: retrying identical command", | ||
| ISSUE_REJECTION_MESSAGE, | ||
| "##[error]The action 'Execute GitHub Copilot CLI' has timed out after 5 minutes.", | ||
| ].join("\n"); | ||
| expect(isShellExpansionGuardRejectedError(log)).toBe(true); | ||
| }); | ||
|
|
||
| it("is case-insensitive", () => { | ||
| expect(SHELL_EXPANSION_GUARD_REJECTED_PATTERN.test(ISSUE_REJECTION_MESSAGE.toUpperCase())).toBe(true); | ||
| }); | ||
|
|
||
| it("matches when the two anchor phrases are split across a line break", () => { | ||
| const wrapped = "Command rejected: ...that could enable arbitrary code execution.\nPlease rewrite the command without these expansion patterns."; | ||
| expect(isShellExpansionGuardRejectedError(wrapped)).toBe(true); | ||
| }); | ||
|
|
||
| it("does not match unrelated shell errors", () => { | ||
| expect(isShellExpansionGuardRejectedError("bash: safeoutputs: command not found")).toBe(false); | ||
| expect(isShellExpansionGuardRejectedError("permission denied by workflow tool permissions")).toBe(false); | ||
| expect(isShellExpansionGuardRejectedError("")).toBe(false); | ||
| }); | ||
|
|
||
| it("does not match arbitrary code execution mentions without the rewrite guidance", () => { | ||
| expect(isShellExpansionGuardRejectedError("This could enable arbitrary code execution if left unchecked.")).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe("WATCHDOG_SIGTERM_PATTERN", () => { | ||
| it("matches a process closed line with SIGTERM and watchdogFired=true", () => { | ||
| const log = "[copilot-harness] attempt 1: process closed exitCode=1 signal=SIGTERM duration=12m 38s hasOutput=true watchdogFired=true"; | ||
|
|
@@ -799,5 +865,23 @@ commentary" has no AI credits pricing`; | |
|
|
||
| expect(lines).toContain("max_cache_misses_exceeded=false"); | ||
| }); | ||
|
|
||
| it("emits shell_expansion_guard_rejected=true when detected", () => { | ||
| const lines = buildOutputLines({ | ||
| inferenceAccessError: false, | ||
| mcpPolicyError: false, | ||
| agenticEngineTimeout: false, | ||
| modelNotSupportedError: false, | ||
| http400ResponseError: false, | ||
| capiQuotaExceededError: false, | ||
| invocationCapExceeded: false, | ||
| maxCacheMissesExceeded: false, | ||
| missingModelPricingError: false, | ||
| missingModelPricingModelName: "", | ||
| shellExpansionGuardRejected: true, | ||
| }); | ||
|
|
||
| expect(lines).toContain("shell_expansion_guard_rejected=true"); | ||
| }); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in
ac847b8448: the flag is now promoted to agent job outputs, passed to the conclusion job asGH_AW_SHELL_EXPANSION_GUARD_REJECTED, and classified/rendered byhandle_agent_failure.cjsahead of generic timeout handling.