Align Codex AWF secret isolation with Claude engine#34446
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot make recompile |
There was a problem hiding this comment.
Pull request overview
Aligns the Codex engine’s AWF-mode secret isolation behavior with the Claude engine by ensuring Codex/OpenAI API key environment variables are excluded from the container-visible environment while preserving host/AWF proxy authentication behavior.
Changes:
- Treats
CODEX_API_KEYandOPENAI_API_KEYas core secret env vars for AWF--exclude-envgeneration in Codex AWF execution. - Adds test assertions to verify both API key vars are excluded from the AWF container environment for Codex runs.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/codex_engine.go | Updates Codex AWF execution config to exclude CODEX_API_KEY/OPENAI_API_KEY from container-visible env via computed --exclude-env list. |
| pkg/workflow/codex_engine_test.go | Extends Codex AWF execution test to assert both API key vars are excluded in the generated AWF command. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #34446 does not have the 'implementation' label and only adds 10 lines in business logic directories (threshold: 100). |
|
🧪 Test Quality Sentinel completed test quality analysis. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
|
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
|
|
🌑 The shadows whisper... Smoke Codex was cancelled. The oracle requires further meditation... |
|
💫 TO BE CONTINUED... Smoke Claude was cancelled! Our hero faces unexpected challenges... |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — approving. The fix is correct and the blast radius is appropriately narrow.
📋 Key Themes & Highlights
Key Themes
- Correct security fix:
CODEX_API_KEY+OPENAI_API_KEYare now excluded from the AWF container env, matching the pattern used by Claude (ANTHROPIC_API_KEY), Copilot (COPILOT_GITHUB_TOKEN), and Gemini (GEMINI_API_KEY). Two keys are needed because Codex accepts either as a credential alias. - Minor test placement issue: The regression assertions were added to
TestCodexEngineExecutionAddsMountedMCPCLIPathSetup, which tests a different concern; see inline comment for a suggested dedicated test.
Positive Highlights
- ✅ One-line targeted change in Go source — minimal blast radius
- ✅ All 13 Codex lock files correctly regenerated with the new
--exclude-envflags - ✅ Parity with all other engine implementations is now complete
- ✅ PR description clearly explains the root cause and the isolation model
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.7M
| if !strings.Contains(stepContent, "export PATH=\"${RUNNER_TEMP}/gh-aw/mcp-cli/bin:$PATH\"") { | ||
| t.Errorf("Expected mounted MCP CLI bin directory in AWF command, got:\n%s", stepContent) | ||
| } | ||
| if !strings.Contains(stepContent, "--exclude-env CODEX_API_KEY") { |
There was a problem hiding this comment.
[/tdd] The secret-isolation assertions are appended to a test whose name (TestCodexEngineExecutionAddsMountedMCPCLIPathSetup) declares a different responsibility. When this test fails in future, the failure message will be misleading.
💡 Suggestion: dedicated test
Consider extracting these two assertions into their own test so the intent is self-documenting:
func TestCodexEngineExecutionExcludesAPIKeyEnvVars(t *testing.T) {
engine := NewCodexEngine()
workflowData := &WorkflowData{
Name: "test-workflow",
NetworkPermissions: &NetworkPermissions{
Allowed: []string{"defaults"},
Firewall: &FirewallConfig{Enabled: true},
},
}
steps := engine.GetExecutionSteps(workflowData, "/tmp/test.log")
require.NotEmpty(t, steps, "expected at least one execution step")
stepContent := strings.Join([]string(steps[0]), "\n")
assert.Contains(t, stepContent, "--exclude-env CODEX_API_KEY",
"CODEX_API_KEY must be excluded from AWF container env")
assert.Contains(t, stepContent, "--exclude-env OPENAI_API_KEY",
"OPENAI_API_KEY must be excluded from AWF container env")
}This also implicitly tests the base AWF mode (no CLIProxy), which is the scenario where the original bug existed.
There was a problem hiding this comment.
Agreed — the test name could be more specific to the secret isolation assertions. Renaming or splitting the test would improve maintainability.
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · sonnet46 2.8M
|
🚀 Smoke Gemini MISSION COMPLETE! Gemini has spoken. ✨ Caution agentic threat detected Verifying safeoutputs capability |
|
@copilot make test more robust https://github.com/github/gh-aw/actions/runs/26362902350/job/77601573019 fix comments |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (1 test analyzed)
Test Classification Details
Language SupportTests analyzed:
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 70/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%). The 2 new assertions in TestCodexEngineExecutionAddsMountedMCPCLIPathSetup enforce the secret isolation behavioral contract. No guideline violations detected.
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 2.1M
Comments that could not be inline-anchored
pkg/workflow/codex_engine_test.go:110
Misleading test error message: The messages say "must remain available to Codex runtime" but this contradicts the PR's stated intent — the keys should be excluded in AWF mode. This test covers the non-AWF execution path where no --exclude-env flag exists at all (correct behaviour for that path), but the wording implies the keys must always be accessible.
<details>
<summary>💡 Suggested fix</summary>
Update both messages to make the scope explicit:
if strings.Contains(stepConten…
</details>
|
Caution agentic threat detected Smoke Test Results (Gemini)
Overall Status: FAIL Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
Agent Container Tool Check
Result: 12/12 tools available ✅ Overall Status: PASS
|
|
🤖 The smoke test agent was here! 👋 Beep boop — all systems nominal. The Copilot smoke test stopped by to say hi and confirm that GitHub Agentic Workflows is humming along beautifully. Carry on, humans! 🚀 Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
|
Smoke Test 26362944791 Results Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
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. Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
This PR correctly aligns Codex secret isolation with the Claude engine model. The change to pass CODEX_API_KEY and OPENAI_API_KEY to ComputeAWFExcludeEnvVarNames is clean and well-tested.
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · sonnet46 2.8M
| // Exclude Codex/OpenAI API key env vars from the AWF container. | ||
| // AWF's API proxy handles auth, so raw token values should not be | ||
| // visible to in-container tools (e.g., env/printenv). | ||
| ExcludeEnvVarNames: ComputeAWFExcludeEnvVarNames(workflowData, []string{"CODEX_API_KEY", "OPENAI_API_KEY"}), |
There was a problem hiding this comment.
Good change — passing ["CODEX_API_KEY", "OPENAI_API_KEY"] explicitly mirrors the Claude engine pattern and ensures secret isolation parity. Consider adding a comment noting that this list must stay in sync with ComputeAWFExcludeEnvVarNames defaults in awf_helpers.go.
| if !strings.Contains(stepContent, "export PATH=\"${RUNNER_TEMP}/gh-aw/mcp-cli/bin:$PATH\"") { | ||
| t.Errorf("Expected mounted MCP CLI bin directory in AWF command, got:\n%s", stepContent) | ||
| } | ||
| if !strings.Contains(stepContent, "--exclude-env CODEX_API_KEY") { |
There was a problem hiding this comment.
The test assertions for secret exclusion look correct. Verifying both CODEX_API_KEY and OPENAI_API_KEY are excluded aligns with the updated engine behavior.
There was a problem hiding this comment.
Ug! Me see point. Test name confusing. Should say secret in name. Me agree with review.
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · sonnet46 2.8M
Smoke Test: Claude — Run 26362994537Core (#1–#13): ✅ all pass Overall: ✅ PASS (§run) Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
💥 [THE END] — Illustrated by Smoke Claude · opus47 5.6M
Comments that could not be inline-anchored
pkg/workflow/codex_engine.go:1
Smoke test review comment 1: confirming Codex engine alignment with Claude looks good. — Run 26362994537
pkg/workflow/codex_engine_test.go:1
Smoke test review comment 2: nice test coverage on the secret exclusion path. — Run 26362994537
|
📰 DEVELOPING STORY: Smoke Copilot reports failed to deliver outputs. Our correspondents are investigating the incident... |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
Ug! Smoke test agent was here! Me sniff code, me find bugs... actually no bugs. Code good. Caveman approve. 🦴 Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
Smoke Test Results - Run 26362994519
Overall: ✅ PASS Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Me review PR. Secret isolation good. Code clean. Test exist. Ug approve.
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · sonnet46 2.8M
Comments that could not be inline-anchored
pkg/workflow/codex_engine.go:421
Me see FilterEnvForSecrets here. Good secret isolation! Codex engine now only get allowed secrets. Ug, nice security.
pkg/workflow/codex_engine_test.go:457
Me like test. Secret isolation test good. But test name say TestCodexEngine - maybe add secret in name? More clear. Ug.
|
📰 DEVELOPING STORY: Smoke Copilot reports failed to deliver outputs. Our correspondents are investigating the incident... |
Codex engine secret handling diverged from Claude in AWF mode: Codex API credentials were present in step env but not explicitly excluded from container-visible environment. This change aligns Codex with Claude’s secret isolation model while preserving runtime auth behavior.
Secret isolation parity (Codex vs Claude)
CODEX_API_KEYandOPENAI_API_KEYas core secret env vars for--exclude-envgeneration.Behavioral guardrails
✨ PR Review Safe Output Test - Run 26362994537
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.comSee Network Configuration for more information.