engine/codex: compile error when restricted bash allowlist is configured - #49758
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR Triage
Automated triage — see full report issue for details. Structured data: {
"action": "defer",
"category": "chore",
"pr_number": 49758,
"risk": "high"
}
|
There was a problem hiding this comment.
🟡 Not ready to approve
Explicit deny configurations (bash: false and bash: []) still bypass validation and leave Codex shell access unrestricted.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds engine capability validation to prevent unsupported bash allowlists from being silently ignored.
Changes:
- Adds
BashCommandAllowlistengine capability. - Rejects restricted bash lists for unsupported engines.
- Adds validation and capability tests.
File summaries
| File | Description |
|---|---|
pkg/workflow/agent_validation.go |
Validates bash allowlist support. |
pkg/workflow/agentic_engine.go |
Defines runtime capability. |
pkg/workflow/engine_definition.go |
Defines declarative capability. |
pkg/workflow/compiler_orchestrator_tools.go |
Wires validation into compilation. |
pkg/workflow/bash_command_allowlist_validation_test.go |
Tests validation and capabilities. |
pkg/workflow/compiler_orchestrator_tools_test.go |
Updates Codex compilation test. |
pkg/workflow/claude_engine.go |
Enables Claude capability. |
pkg/workflow/copilot_engine.go |
Enables Copilot capability. |
pkg/workflow/gemini_engine.go |
Enables Gemini capability. |
pkg/workflow/antigravity_engine.go |
Enables Antigravity capability. |
Review details
- Files reviewed: 10/11 changed files
- Comments generated: 1
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| if !hasBashRestrictedAllowlist(tools) { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Fixed in 853d09c. validateBashCommandAllowlistSupport now uses a new hasBashExplicitRestriction helper that returns true for bash: false, bash: [], and non-wildcard command lists — only absent/nil bash, bash: true, and wildcard lists (["*"], [":*"]) bypass the error. The separate hasBashRestrictedAllowlist function used for MCP CLI command injection is unchanged.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
The logic change is correct and well-tested — the new BashCommandAllowlist capability flag and validateBashCommandAllowlistSupport() function properly prevent the silent-bypass scenario described in the PR.
Blocking issue: An unrelated binary file (openai-codex-0.146.0.tgz) has been accidentally committed. This should be removed and added to .gitignore before merging.
Non-blocking observations:
hasBashRestrictedAllowlistreturnsfalseforbash: falseandbash: []. These are not "restricted allowlist" cases in the same way (bash is disabled rather than superficially restricted), so the current behavior is acceptable — but worth a comment in the function doc if the distinction isn't obvious.- The error message is clear and actionable; the test coverage is thorough.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 28.5 AIC · ⌖ 10.5 AIC · ⊞ 5.4K
Comments that could not be inline-anchored
openai-codex-0.146.0.tgz:1
This binary .tgz package file appears to have been accidentally committed to the repository. Binary packages should not be tracked in source control — they belong in .gitignore and installed via the package manager at build/test time.
Please remove this file and add *.tgz (or at least openai-codex-*.tgz) to .gitignore to prevent re-commits.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two issues.
📋 Key Findings
Issues
- Accidentally committed binary (
openai-codex-0.146.0.tgz) — should be removed from source control and gitignored. - Happy-path test removed without replacement —
TestProcessToolsAndMarkdown_CustomEngineNoToolswas repurposed to test the error path only; the previous assertion thatbash: ["*"]compiles correctly for Codex was dropped with no substitute.
Positive Highlights
- ✅ Clean capability-flag design:
BashCommandAllowlistonEngineCapabilitiesfollows the existing pattern perfectly. - ✅ Excellent error message — names the engine, explains the runtime risk, and offers concrete alternatives.
- ✅ Good test coverage in
bash_command_allowlist_validation_test.gocovering all four supporting engines and Codex across wildcard / single / multi-command / absent cases. - ✅ Wiring via
validateEngineToolRequirementsis minimal and idiomatic.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 33 AIC · ⌖ 9.84 AIC · ⊞ 7.1K
Comment /matt to run again
Comments that could not be inline-anchored
openai-codex-0.146.0.tgz:1
[/diagnosing-bugs] This binary artifact (openai-codex-0.146.0.tgz) appears to be accidentally committed to the repository root — it is not referenced by any code in the diff and should not be tracked in source control.
<details>
<summary>💡 Suggested fix</summary>
Remove the file and add it to .gitignore:
git rm --cached openai-codex-0.146.0.tgz
echo '*.tgz' >> .gitignoreCommitting binary archives bloats git history and is likely a leftover debug artefact.
</details>
@…
pkg/workflow/compiler_orchestrator_tools_test.go:679
[/tdd] The updated test no longer verifies a tools.bash list that doesn't trigger the new validation — it only covers the error path. A complementary case (e.g. engine: codex with bash: ["*"] or no bash key) should be added to document and guard the happy path.
<details>
<summary>💡 Suggestion</summary>
Add a second test function TestProcessToolsAndMarkdown_CodexWildcardBashSucceeds with bash: ["*"] and assert require.NoError(t, err) and assert.NotEmpty(t, result.tools). T…
🧪 Test Quality Sentinel Report✅ Test Quality Score: 87/100 — Excellent
📊 Metrics (16 tests)
Test Strengths✅ Table-driven test design: Both new tests use clean table-driven patterns with explicit scenarios for success/error paths. ✅ Comprehensive edge coverage: 11 scenarios in ✅ Error assertion discipline: Tests use ✅ No mock violations: No gomock, testify/mock, or internal-call mocking detected. ✅ Build tag compliance: New test file has mandatory ✅ Integration test: Modified Verdict
|
…ement Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Done — removed in commit chore: remove accidentally committed openai-codex-0.146.0.tgz.
There was a problem hiding this comment.
Verdict: Request changes
Core validator logic (validateBashCommandAllowlistSupport, capability flag wiring, reused hasBashRestrictedAllowlist) is sound and well-tested. One blocking issue: an unrelated 4.5KB binary npm tarball (openai-codex-0.146.0.tgz) was committed to the repo root — it has no connection to the bash-allowlist feature and must be removed before merge.
💡 Review notes
- Capability flag additions for claude/copilot/gemini/antigravity are consistent and gofmt-aligned.
piengine correctly defaults toBashCommandAllowlist: false(unset), so it is also caught by the new validator — same silent-ignore risk as codex, appropriately blocked.- Reuse of
hasBashRestrictedAllowlist(already used for MCP CLI mounting) is appropriate;bash:false/bash:[]semantics (disabled/no-commands) are distinct from a restricted allowlist that is silently ignored, so returning false for those cases is correct for this validator's purpose. - Test coverage is solid across engines and wildcard/absent cases.
- Flagged separately: the stray
openai-codex-0.146.0.tgzbinary artifact.
🔎 Code quality review by PR Code Quality Reviewer · auto · 105.6 AIC · ⌖ 5.07 AIC · ⊞ 7.9K
Comment /review to run again
Comments that could not be inline-anchored
openai-codex-0.146.0.tgz:1
Stray unrelated binary artifact committed to the repo root — this npm tarball has nothing to do with the bash-allowlist feature and should not be part of this PR.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot Please address the remaining review feedback on this PR, then run the pr-finisher skill. Outstanding signals to close:
Branch refresh was requested for this PR. After updates, run the pr-finisher skill. Run details: https://github.com/github/gh-aw/actions/runs/30750494960
|
…ows to bash: ["*"] Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…ndMarkdown Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Both blocking issues are now addressed:
|
|
@copilot Please address the remaining review feedback on this PR, then run the pr-finisher skill. Outstanding signals to close:
After verifying the blocking review state is fully closed, run the pr-finisher skill and summarize merge readiness for the maintainer.
|
Here is the full pr-finisher summary for this PR.
Actions taken this run: Confirmed all prior work is complete and pushed; ran full local validation (green); produced this merge-readiness summary. Hand-off: CI must be re-triggered by a maintainer (close/reopen PR or push) before merge. A maintainer also needs to dismiss the two stale |
tools.bashwith specific commands underengine: codexwas silently accepted but never enforced — the Codex CLI has no mechanism to restrict shell commands, so the allowlist was discarded while the agent ran with--dangerously-bypass-approvals-and-sandbox. This created a false sense of security: the config looked enforced but wasn't.Changes
New
BashCommandAllowlistengine capability — added toEngineCapabilitiesandEngineCapabilitiesDefinition; settruefor Claude, Copilot, Gemini, and Antigravity (all of which maptools.bashentries to their CLI's enforcement mechanism); defaults tofalse(Codex, others)Compile-time validation —
validateBashCommandAllowlistSupport()errors when a non-wildcard bash list is used with an engine that cannot enforce it; wired intovalidateEngineToolRequirements()Wildcards and absent config still compile cleanly — only specific-command lists (e.g.
bash: [git, npm]) trigger the error;bash: ["*"],bash: true, and omittingbashare unaffectedRun: https://github.com/github/gh-aw/actions/runs/30750494960
Run: https://github.com/github/gh-aw/actions/runs/30752949969