[pelikhan]

Why

The Daily Community Attribution run exhausted its denial budget after the SDK shell parser treated standalone comments as an executable named #. Four otherwise permitted commands were incorrectly rejected before a genuinely disallowed Python call ended the session.

Changes

• Skip Bash comments without interpreting their quotes or operators; preserve quoted, escaped, and parameter-expansion hashes.
• End comments only at line feeds, not bare carriage returns, matching Bash behavior.
• Fail closed on unsupported nested quoting and unclosed constructs so quote confusion cannot conceal a following disallowed command. Simple quoted variables and parameter defaults remain supported; complex quoted substitutions may now be denied conservatively.
• Reject unsupported unquoted grouping/glob parentheses so extended-glob hashes cannot be mistaken for comments, including when escaped newlines separate a glob operator from its opening parenthesis. Quoted and escaped literal patterns remain supported.
• Add extensive table-driven comment parser coverage, regressions for the four observed false denials, and security cases for nested quoting, carriage returns, extended globs, and malformed contexts.
• Include a patch changeset.

Every extracted executable remains subject to the existing allowlist. Python restrictions, permissions, and denial thresholds are unchanged. No workflow source changes or workflow dispatches are included.

Validation

• make fmt-cjs && make lint-cjs (no lint errors; existing repository warnings remain).
• npm --prefix actions/setup/js run typecheck --silent.
• npm --prefix actions/setup/js run test:js -- bash_command_parser.test.cjs bash_command_parser_comments.test.cjs bash_command_parser_spec_vectors.test.cjs fuzz_bash_command_parser_harness.test.cjs copilot_sdk_driver.test.cjs pi_tool_policy.test.cjs --no-file-parallelism --silent --reporter=dot: all 595 tests passed.
• PATH="/opt/homebrew/bin:$PATH" make agent-report-progress.
• Harmless, builtin-only Bash 3 and modern Bash probes confirm the security cases reach a mock command under Bash semantics, while both single-command and prefix-restricted permission grants reject them. Extended-glob checks cover all five operators and escaped-newline variants.

The original five denied workflow calls are covered by inert permission-handler regressions: the first four are allowed and Python remains denied. No untrusted logged command was executed.

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

🔎 Code quality review by PR Code Quality Reviewer

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.

Design Decision Gate 🏗️

Outcome: reported_no_action

The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible.

Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.

Generated from Design Decision Gate 🏗️ · opus55 · 10.1 AIC · ⌖ 31.8 AIC · ◇ 15.3 AIC · ⊞ 1.9K · ◷

🏗️ ADR gate enforced by Design Decision Gate 🏗️

[github-actions]

✅ Ponytail Reviewer completed successfully!

Generated by Ponytail Reviewer for #70440

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

Test Quality Sentinel

Outcome: reported_no_action

The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible.

Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.

Generated from Test Quality Sentinel · haiku45 · 5.64 AIC · ⌖ 8.25 AIC · ◇ 2.56 AIC · ⊞ 9.2K · ◷

🧪 Test quality analysis by Test Quality Sentinel

[github-actions]

One opportunity to keep the new coverage lean.

net: -35 lines possible.

Generated by ✂️ Ponytail Reviewer for #70440 · codex · gpt56 · 9.97 AIC · ⌖ 5.35 AIC · ⊞ 14.7K
_{Comment /ponytail to run again}

[github-actions]

Review: comment

Found one correctness issue: the new parser still splits pipeline/sequencing operators inside unquoted parameter expansions, causing false permission denials for valid shell commands such as ${value:-foo|bar}. I left an inline comment with a surgical fix and regression cases. Static review only; repository tests were not run per review instructions.

🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · 2.74 AIC · ⌖ 0.544 AIC · ⊞ 1.6K
_{Comment /review to run again}

[Copilot]

🟡 Changes recommended

Confirmed parsing mismatches still allow disallowed commands to bypass shell permissions.

3 open issues

• Incorrectly treats CR and nonstandard whitespace as comment boundaries · New
• Misses expansion openers separated by escaped newlines · New
• Fails to track parameter scope inside command substitutions · New

What changed in this PR

Fix false Copilot SDK shell permission denials for commented scripts in gh-aw’s workflow runtime.

Changes:

• Skip comments while preserving literal hashes.
• Add fail-closed checks for ambiguous quoting and parentheses.
• Expand parser and permission regressions, with a patch changeset.

File	Description
actions/​setup/​js/​copilot_sdk_driver.test.cjs	Add permission-handler regressions.
actions/​setup/​js/​bash_command_parser.test.cjs	Cover comment parsing and literal hashes.
actions/​setup/​js/​bash_command_parser.cjs	Handle comments and reject ambiguous syntax.
actions/​setup/​js/​bash_command_parser_comments.test.cjs	Add comment and security-boundary coverage.
.changeset/​fix-sdk-shell-comment-permissions.md	Document the patch release.

_{Review effort: Max}

[github-actions]

Skills-Based Review 🧠

Applied /diagnosing-bugs and /tdd (per pr-triage: bug_fix). The fix is sound, fail-closed by design, and backed by an extensive new regression suite (bash_command_parser_comments.test.cjs, 351 lines). I verified the new findShellQuoteEnd and rewritten splitOnPipelineOperators logic against real bash -c behavior for ~15 edge cases (parameter-expansion comments, CR-vs-LF comment termination, extended-glob rejection, escaped backslash-newlines, redirection-adjacent #) and found no new regression introduced by this diff.

📋 Key Themes & Highlights

Positive Highlights

• ✅ /diagnosing-bugs: Root cause is addressed, not just the symptom — the old parser mishandled nested quotes/backticks silently; the new findShellQuoteEnd helper returns -1 (fail-closed) for any construct it cannot safely disambiguate, which is the correct security posture for a permission-gating parser.
• ✅ /tdd: bash_command_parser_comments.test.cjs is genuinely specification-quality — table-driven cases with descriptive names covering comment-only requests, CR-vs-LF boundaries, quoted #/&& characters, and unclosed-quote fail-closed behavior.
• ✅ The LF-only comment termination (explicitly rejecting bare-CR as a comment boundary) is a thoughtful, deliberately-tested anti-evasion design choice, confirmed correct against real bash.

One non-blocking observation (/diagnosing-bugs)

While probing adjacent pipeline-operator handling, I found that splitOnPipelineOperators does not treat a bare single & (background operator, distinct from &&) as a segment separator. For example:

extractCommandNamesFromPipeline('sleep 5 & python3 -c \"print(1)\"')
// => [\"sleep\"]  -- python3 is silently dropped

This means buildCopilotSDKPermissionHandler would approve sleep 5 & python3 -c \"print(1)\" if only shell(sleep) is allowlisted — a real allowlist-bypass gap. However, based on the diff (which only adds atWordStart handling to the existing && branch, not any new &-splitting logic) this appears to be pre-existing, not introduced by this PR, so it's out of strict "changed lines only" scope and not blocking. Worth a quick follow-up issue since it's directly adjacent to this PR's subject (shell permission correctness).

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 225.5 AIC · ⌖ 14.9 AIC · ⊞ 10.1K
_{Comment /matt to run again}

[gh-aw-bot]

Caution

agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.

Details

Potential security threats were detected in the agent output.

Review the workflow run logs for details.

@copilot address the following outstanding work in one pass:

1. Update this branch with the latest main using make merge-main, resolving any conflicts and preserving the intended changes.
2. Review (actions/setup/js/bash_command_parser_comments.test.cjs:245): L245: shrink: 60-case line-ending × separator × inert-comment-body matrix repeats the same three assertions. Keep one representative per separator plus the distinct quote/substitution/backslash cases below; the parser only branches on LF vs CRLF and syntax, not prose. - Fix false SDK shell permission denials for comments #70440 (comment)
3. Review (actions/setup/js/bash_command_parser.cjs:190): This still treats | inside an unquoted ${...} expansion as a pipeline operator, so valid commands such as echo ${value:-foo|bar} are split into bogus segments and can be falsely denied (or misclassified for permission matching). - Fix false SDK shell permission denials for comments #70440 (comment)
4. Review (actions/setup/js/bash_command_parser.cjs:94): [P1] Use Bash word boundaries before discarding comments. In echo ok\\r#; forbidden_probe (with an actual CR), Bash treats CR and # as argument characters and executes forbidden_probe. This parser instead resets atWordStart at CR, discards the following command, and approves both shell(echo) and shell(echo ok:*). Escaped CR has the same problem; the (s/redacted) skips after separators also misclassify hashes following vertical tabs or nonbreaking spaces. Preserve these non-Bash whitespace characters, or reject unsupported contexts, throughout splitting and reparsing. Add permission regressions with these characters before hashes, not only inside comments. - Fix false SDK shell permission denials for comments #70440 (comment)
5. Review (actions/setup/js/bash_command_parser.cjs:102): [P1] Recognize expansion openers across escaped newlines. Insert a backslash–LF between $ and { in echo ok ${value:-x #}; forbidden_probe: Bash removes the continuation and executes forbidden_probe, but this parser misses ${ and discards the remainder as a comment. Both shell(echo) and shell(echo ok:*) approve it. The same mismatch bypasses the $' rejection and the quoted $(/${ checks in findShellQuoteEnd. Make opener recognition continuation-aware in both scanners, or reject these forms conservatively, without rewriting literal or commented text. Add permission regressions for all these forms. - Fix false SDK shell permission denials for comments #70440 (comment)
6. Review (actions/setup/js/bash_command_parser.cjs:149): [P1] Track parameter scope inside command substitutions. echo ok $(printf %s ${value:-)x #}); forbidden_probe executes forbidden_probe in Bash, but this scanner closes $() at the ) inside the parameter default. The outer comment scanner then discards the remainder, and shell(echo) approves the request. Track ${...} nesting and reject parentheses inside those parameter bodies until they can be parsed safely. The suggested change preserves simple defaults while failing closed on this ambiguity; add a permission regression for the example. - Fix false SDK shell permission denials for comments #70440 (comment)

Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI.

Sous-chef head: c7397e5
Sous-chef work: 082225aa3a01ad5719d881222036b88d06c6aad0f35c21e21803ec84da3f5239 2d2de7a6be34a500500902265afe88880f5e86f8c273ef20ef36877ed27754fd 9fb5e0eb8e988bf2d561c674decb47b8548c9886814cc67f1127259a8b6e8bf1 d6baf9ff17528466c40109daf82c03d2fc6f838f5b30e9c76f2ce214463a38f1 ff76bcbd611dd4696edb649fe03167a2f3ccad643f6f058b8939b75fc4d510ce
Sous-chef state: cddb39c1b1a74f5c84b2ced4721e988a98255bf38d345e006f3ade9830477ca6

Generated by 👨‍🍳 PR Sous Chef · pi · haiku45 · 4.23 AIC · ⌖ 9.27 AIC · ⊞ 1.1K · ◷
_{Comment /souschef to run again}

[pelikhan]

@copilot compare test suite with existing parsers and increase adverserial test cases

[pelikhan]

Fixed the remaining background-command allowlist bypass noted in the skills-based review in acc6faa: & now separates independently checked commands, |& checks both pipeline stages, and redirection ampersands remain intact. The three inline security findings are already fixed in ede9370; their reported cases still reject.

Expanded the existing specification/property-based coverage with ampersand operators, redirection/escape cases, SDK-identifier fallback cases, and non-Bash whitespace adversarial cases. Added persistent differential checks against the real Bash parser using builtin-only mock commands for background execution, nested quoting, CR boundaries, escaped-newline expansions, and extended globs. All 650 focused tests, type checking, lint (no errors), and make agent-report-progress passed. Permissions and denial thresholds are unchanged; no workflows were dispatched.

[Copilot]

@copilot compare test suite with existing parsers and increase adverserial test cases

Compared the parser’s fixture, vector/metamorphic, and fuzz suites, then added permission-level adversarial cases for background &, |&, redirects, and command substitutions. The tests exposed permission bypasses; fixes and expanded coverage are in b4c4293.