Add guided gh aw fix diagnostic for restricted tools.bash on engines that ignore allow-listing - #51102
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
gh aw fix diagnostic for restricted tools.bash on engines that ignore allow-listing
There was a problem hiding this comment.
Pull request overview
Adds a guided gh aw fix diagnostic for unsupported restricted Bash configurations.
Changes:
- Adds and registers a capability-driven guided codemod.
- Shares Bash-restriction detection with strict compilation.
- Adds tests, release notes, and regenerated workflow output.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/agent_validation.go |
Exports shared restriction detection. |
pkg/cli/fix_codemods.go |
Registers the codemod. |
pkg/cli/fix_codemods_test.go |
Updates registry expectations. |
pkg/cli/codemod_bash_allowlist_unsupported_engine.go |
Implements the diagnostic. |
pkg/cli/codemod_bash_allowlist_unsupported_engine_test.go |
Tests codemod behavior. |
.github/workflows/smoke-cursor.lock.yml |
Refreshes generated workflow output. |
.changeset/bash-allowlist-unsupported-engine-guided-error.md |
Documents the patch. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Balanced
|
|
|
|
|
|
|
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. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
The implementation is well-structured, follows the guided-error pattern correctly, and the test coverage is comprehensive.
Two pre-existing inline comments already flag the main concerns:
-
Shallow frontmatter inspection (line 29): the codemod reads the raw top-level
toolsmap from frontmatter, but the compiler counterpart operates on the merged tools after resolving imports and includes. A workflow that restricts bash only via an imported tool definition would be flagged bygh aw compile --strictbut silently pass the codemod. -
Unescaped user-controlled content in error message (line 73):
describeBashRestrictionformats YAML command strings directly into the error output via%v. Commands containing ANSI escape sequences or newlines can spoof diagnostic lines on terminals that render control characters.
No new blocking issues found beyond those two.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 23.9 AIC · ⊞ 5.4K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (2 tests)
Quality Highlights✅ Comprehensive edge-case coverage — 11 distinct scenarios in table-driven Apply test:
✅ Semantic invariant validation — Tests enforce that:
✅ Design contract testing — Both tests focus on public behavior and contracts:
✅ Code quality — Proper build tag, no mock violations, testify best practices, clear test names. Verdict
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (226 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /grill-with-docs, /tdd, and /codebase-design — requesting changes on three issues.
📋 Key Themes & Highlights
Key Themes
- Hardcoded engine list in error message (line 56):
"copilot, claude, or gemini"will drift as engines gainBashCommandAllowlistsupport. The capability is already queried from the registry for detection; it should drive the message too. - Terminal output injection (line 73, pre-existing comment):
cmdfrom workflow YAML is inserted into stderr unescaped — control characters or newlines could spoof diagnostics. - Detection scope gap (line 29, pre-existing comment): only the raw top-level
toolsmap is inspected; merged/imported tool definitions are not considered. - Implicit test assumption (test line 117): the "default engine" no-op relies on
copilotbeing the default and supporting the capability — a comment explaining that chain would make the test robust to future change.
Positive Highlights
- ✅ Excellent pattern: guided (non-auto-rewriting) codemod that explains both options and their semantics trade-offs
- ✅ Shared detection logic (
HasBashExplicitRestriction) exported and reused between compiler and codemod — no drift - ✅ Comprehensive test table covering every bash restriction shape and all engine scenarios
- ✅ Follows the established
top-level-env-secrets-guided-errorpattern consistently
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 41.7 AIC · ⊞ 7.1K
Comment /matt to run again
|
@copilot Please address the current review feedback, refresh the branch if GitHub can update it cleanly, and run the Open review items to address (newest first):
Branch refresh was requested. Run context: https://github.com/github/gh-aw/actions/runs/31200043159
|
|
@copilot codex now support specifying |
…ive tools from imports - Use fmt.Sprintf(%q) in describeBashRestriction to prevent terminal output spoofing via ANSI control sequences or embedded newlines in YAML-sourced command strings - Add ApplyWithContext optional field to Codemod struct for codemods that need the workflow file path to resolve effective configuration from imports/includes - Update fix_command.go to call ApplyWithContext (if set) instead of Apply so the bash allowlist codemod receives the file path at runtime - Add resolveEffectiveBashTools helper: resolves imports via parser.ProcessImportsFromFrontmatterWithSource and markdown includes via parser.ExpandIncludesWithManifest, then merges with top-level tools; fast path skips resolution when the top-level tools already declares a bash key (it wins) - Update tests: fix expected error string for quoted commands; add control-character and newline spoofing test cases; add ApplyWithContext import-sourced restriction test Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed both review items in commit Terminal-spoofing (line 73): Import resolution (line 29): Added an optional |
…e-read fix, test clarity - Add EnginesWithCapability(predicate) to EngineRegistry so callers can derive the supported-engine list from the registry rather than hardcoding it; this addresses the grill-with-docs thread that flagged 'copilot, claude, or gemini' as fragile as new engines gain BashCommandAllowlist support - Use EnginesWithCapability in the guided error message in applyBashAllowlistUnsupportedEngineCheck so the list stays registry-accurate - Extract bashVal := effectiveTools["bash"] once before HasBashExplicitRestriction so describeBashRestriction reads the same value (codebase-design thread fix) - Rename 'default engine' test case to explicitly call out that it relies on copilot being the default engine and having BashCommandAllowlist (tdd thread fix) - Update test assertion to check for individual engine names rather than the full hardcoded string to remain correct as the sorted list evolves Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
gh aw compile --strictrejects workflows that pair an engine without bash allow-list support (today:codex) with a restrictedtools.bash, butgh aw fixreported "No fixes needed" — leaving users with a broken workflow and no remediation path.Changes
pkg/cli/codemod_bash_allowlist_unsupported_engine.go, IDbash-allowlist-unsupported-engine-guided-error): flags any explicit bash restriction (bash: [cmd, ...],bash: [],bash: false) on an engine whose capabilities lackBashCommandAllowlist. Capability is read from the global engine registry rather than hardcodingcodex, so it stays correct as engines change. Unknown/custom engines are a no-op.bash: ["*"]makes unrestricted access explicit, and switching engines changes which agent runs — so the codemod never mutates the file and instead names the offending config plus both options. This matches the existingtop-level-env-secrets-guided-errorpattern.hasBashExplicitRestriction→workflow.HasBashExplicitRestrictionso the codemod and the strict-mode compiler check cannot drift.bash: true, supported/default/unknown engines, notools). Changeset included.Behavior
The issue suggested the id
codex-bash-allowlist-to-wildcard; I went with an engine-agnostic id since the check is capability-driven, but happy to rename.