Uh oh!
There was an error while loading. Please reload this page.
Omit CLI-only safeoutputs instructions when bash is disabled; make cli-proxy/bash incompatibility explicit - #53548
Conversation
Thanks for working on this! 👋 This PR addresses a real pain point in #53532 where strict Copilot workflows with Bash disabled still receive CLI-only safeoutputs instructions, creating a mismatch between what the prompt directs and what tools are actually available. A few things to keep in mind as you complete this implementation:
This is in great shape once the implementation is complete and tests are added!
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
❌ Design Decision Gate 🏗️ failed during design decision gate check. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
✅ Ponytail Reviewer completed successfully!
|
✅ 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.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
There was a problem hiding this comment.
The implementation is clean and well-structured.
- Validation split is correct:
validateCLIProxyBashCompatibility(non-strict) rejects onlycli-proxy: true+ bash disabled.validateStrictToolsadditionally requires the explicitcli-proxy: falsedeclaration — the two layers are properly separated. - Prompt gating (
buildMCPCLIPromptSection→data.BashDisabled) prevents confusing CLI-only instructions from reaching agents that have no shell — the key behavioral fix. - Codemod insertion logic correctly finds the first field of the
tools:block and inserts before it, preserving natural YAML ordering. - Duplicate helper logic (
isBashExplicitlyRefused/frontmatterRefusesBash) is acceptable given the package boundary — both are identical and correct. - Test coverage is thorough across validator, codemod, prompt section, and strict mode.
No blocking issues found. ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 38 AIC · ⌖ 11.7 AIC · ⊞ 5.7K
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Request changes
This patch introduces one merge-blocking regression: it makes tools.bash: false incompatible with Pi workflows by unconditionally requiring tools.cli-proxy: false in strict mode, even though Pi explicitly requires tools.cli-proxy: true and relies on CLI-mounted MCP tools instead of shell access.
Blocking theme
- The new strict-mode rule conflicts with existing engine requirements and rejects a valid no-shell configuration for Pi.
- The change needs an engine-aware exception or the compatibility rule needs to be narrowed so it only applies to engines that actually depend on bash for CLI access.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 14.9 AIC · ⌖ 6.78 AIC · ⊞ 4.5K
Comment /review to run again
There was a problem hiding this comment.
Ponytail review: scoped to complexity/over-engineering only.
pkg/cli/codemod_cli_proxy_bash.go:L219-237: yagni: frontmatterRefusesBash duplicates tools_validation.go's isBashExplicitlyRefused (identical bool/[]any switch logic, different map depth). Export/reuse one helper instead of maintaining two copies.
Everything else (codemod line-rewriting, validation, prompt-section gating) is proportionate to the bug being fixed — no speculative abstractions or unused flexibility found.
net: -14 lines possible.
Generated by ✂️ Ponytail Reviewer for #53548 · auto · 28.2 AIC · ⌖ 5.51 AIC · ⊞ 7.3K
Comment /ponytail to run again
Comments that could not be inline-anchored
pkg/cli/codemod_cli_proxy_bash.go:237
L219-237: yagni: frontmatterRefusesBash duplicates tools_validation.go's isBashExplicitlyRefused (identical bool/[]any switch logic, just a different map nesting depth). Export/reuse one helper instead of maintaining two copies.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — commenting on two minor issues; no blocking changes.
📋 Key Themes & Highlights
Key Themes
- Logic duplication:
frontmatterRefusesBash(codemod package) andisBashExplicitlyRefused(workflow package) implement the same semantics independently — drift risk if one is updated. - Test boundary clarity:
TestValidateCLIProxyBashCompatibilitydoesn't explicitly assert thatbash: falsewith nocli-proxykey is valid — this leaves the function's narrow scope (only blockingcli-proxy: true) unspecified in the tests. - Codemod insertion order:
setCLIProxyFalseInToolsinserts before the first field rather than afterbash:, so the generated change putscli-proxy:abovebash:which is slightly surprising to readers.
Positive Highlights
- ✅ Root cause addressed properly — both prompt suppression and validation guards prevent re-introduction.
- ✅ Strict-mode rule mirrors the existing
min-integrity: none ⇒ explicit bashpattern — consistent design. - ✅ Codemod is well-tested (idempotency, both insert and rewrite paths covered).
- ✅ Changeset, docs, and all affected workflows updated atomically.
- ✅
validateCLIProxyBashCompatibilityis correctly called in bothParseWorkflowStringandvalidateWorkflowToolConfigurationsso neither compilation path is missed.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 55.2 AIC · ⌖ 10.2 AIC · ⊞ 7.8K
Comment /matt to run again
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
Prevents shell-disabled workflows from receiving unusable MCP CLI instructions and adds validation plus migration support.
Changes:
- Suppresses MCP CLI prompts when Bash is disabled.
- Rejects incompatible CLI proxy settings and adds a codemod.
- Updates tests, documentation, and affected workflows.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/tools_validation.go | Adds Bash/CLI proxy compatibility validation. |
pkg/workflow/tools_validation_test.go | Tests compatibility validation. |
pkg/workflow/strict_mode_validation_test.go | Tests strict-mode requirements. |
pkg/workflow/strict_mode_network_validation.go | Requires explicit CLI proxy disablement. |
pkg/workflow/mcp_cli_mount.go | Omits CLI prompts without Bash. |
pkg/workflow/mcp_cli_mount_test.go | Tests prompt omission. |
pkg/workflow/compiler_string_api.go | Applies validation to string parsing. |
pkg/workflow/compiler_orchestrator_workflow.go | Applies validation during compilation. |
pkg/workflow/compiler_orchestrator_workflow_test.go | Updates disabled-Bash fixture. |
pkg/workflow/bash_anonymous_validation_test.go | Updates strict Bash fixture. |
pkg/cli/fix_codemods.go | Registers the migration codemod. |
pkg/cli/fix_codemods_test.go | Verifies codemod registration and order. |
pkg/cli/compile_guard_policy_test.go | Updates guard-policy fixture. |
pkg/cli/codemod_cli_proxy_bash.go | Implements CLI proxy migration. |
pkg/cli/codemod_cli_proxy_bash_test.go | Tests codemod behavior. |
docs/src/content/docs/reference/tools.md | Documents the incompatibility. |
.github/workflows/smoke-agent-public-none.md | Explicitly disables CLI proxy. |
.github/workflows/smoke-agent-public-none.lock.yml | Recompiles the public smoke workflow. |
.github/workflows/smoke-agent-all-none.md | Explicitly disables CLI proxy. |
.github/workflows/smoke-agent-all-none.lock.yml | Recompiles the all-none smoke workflow. |
.github/workflows/ai-moderator.md | Disables MCP CLI mounting. |
.github/workflows/ai-moderator.lock.yml | Recompiles the moderator workflow. |
.changeset/cli-proxy-incompatible-with-bash-disabled.md | Records the behavioral change. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 23/23 changed files
- Comments generated: 3
- Review effort level: Balanced
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
gh-aw-bot
commented
Aug 18, 2026
@copilot Please address the blocking review feedback below, refresh the branch if needed, then run the
|
gh-aw-bot
commented
Aug 18, 2026
No follow-up edits are needed, so I did not run Branch status: clean at Local validation in this pass: |
gh-aw-bot
commented
Aug 18, 2026
@copilot Please confirm the current state of this PR, resolve any remaining addressed review feedback in the UI if applicable, and run the
|
Confirmed current state: no follow-up edits are needed, so I did not run Branch is clean at Local validation in this pass passed: Remaining items appear to be maintainer/action-system side only: maintainer review or stale blocking review cleanup if GitHub still gates on it, and normal CI/check completion or re-trigger if branch protection requires it. |
gh-aw-bot
commented
Aug 18, 2026
@copilot Please confirm the current state of the addressed review feedback on this PR, refresh the branch if needed, then run the
|
Confirmed current state: no follow-up edits are needed, so I did not run Addressed review feedback: the review-thread API still reports all 6 prior addressed threads as Branch: clean at CI/checks: latest referenced PR Sous Chef run Local validation: Remaining items appear to be maintainer/action-system side only: stale blocking review cleanup if GitHub still gates on it, and normal CI/check completion or re-trigger if branch protection requires it. |
pelikhan
commented
Aug 18, 2026
@copilot add workflow integration tests |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added workflow integration coverage in |
gh-aw-bot
commented
Aug 18, 2026
@copilot Please address the remaining review feedback on this PR, refresh the branch if needed, then run the
|
…orkflow-safeoutputs Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
PR TriageCategory: bug | Risk: high | Priority: medium (score 58)
Recommended action: defer — address CHANGES_REQUESTED first
|
Current state after this pass: branch refreshed in Validation passed locally: Code review found only an unrelated upstream |
🎉 This pull request is included in a new release. Release: |
A strict workflow that must set
tools.bash: false(e.g.tools.github.min-integrity: none) still received the<mcp-clis>prompt section, which tells the agent to invokesafeoutputsfrom bash and that it is not an MCP tool. With no shell tool in the harness, a model following that instruction can finish without emitting any safe output — silently, when failure reporting is disabled.Changes
pkg/workflow/mcp_cli_mount.go):buildMCPCLIPromptSectionreturnsnilwhen shell execution is fully refused (bash: falseorbash: []). The<mcp-clis>section andGH_AW_MCP_CLI_SERVERS_LISTare dropped, leaving the<safe-output-tools>MCP guidance as the only advertised path. The mount step is untouched, soengine.commandharnesses keep their CLI wrappers onPATH.pkg/workflow/tools_validation.go):tools.cli-proxy: trueis now rejected when bash is disabled — CLI wrappers are only reachable from a shell.pkg/workflow/strict_mode_network_validation.go): a disabledtools.bashnow requires an explicittools.cli-proxy: false, mirroring the existingmin-integrity: none⇒ explicittools.bashrule, so the incompatibility is visible in the source.pkg/cli/codemod_cli_proxy_bash.go):cli-proxy-false-when-bash-disabledinsertscli-proxy: falseinto thetools:block, or rewrites an existingcli-proxy: true, viagh aw fix.ai-moderator(which pairedbash: falsewithcli-proxy: true),smoke-agent-all-none, andsmoke-agent-public-noneupdated and recompiled. Two existing tests usingbash: false/bash: []gained the now-required explicit setting.reference/tools.md), changeset, and tests for the prompt suppression, validation matrix, strict rule, and codemod.Resulting lock diff for such a workflow:
mcp_cli_tools_with_safeoutputs_prompt.mdandGH_AW_MCP_CLI_SERVERS_LISTno longer appear inGH_AW_PROMPT_CONFIG.Note: the strict-mode rule is a compile-time behavior change for existing workflows that disable bash without stating
cli-proxy: false; the codemod is the intended migration path.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.
Run: https://github.com/github/gh-aw/actions/runs/32109331559> Generated by 👨🍳 PR Sous Chef · gpt54 · 30 AIC · ⌖ 11.4 AIC · ⊞ 8.8K · ◷
Run: https://github.com/github/gh-aw/actions/runs/32123781702> Generated by 👨🍳 PR Sous Chef · gpt54 · 41.3 AIC · ⌖ 7.88 AIC · ⊞ 8.8K · ◷