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. ✅
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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
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
|
@copilot Please address the blocking review feedback below, refresh the branch if needed, then run the
|
No follow-up edits are needed, so I did not run Branch status: clean at Local validation in this pass: |
|
@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. |
|
@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. |
|
@copilot add workflow integration tests |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added workflow integration coverage in |
|
@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 · ◷