Codex engine: move CODEX_HOME off /tmp to restore MCP helper resolution fleet-wide#54687
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Moves Codex runtime state to a workspace-backed home so MCP helpers resolve consistently across workflows.
Changes:
- Adds and propagates a shared workspace-backed
CODEX_HOME. - Preserves legacy diagnostics paths via symlinks.
- Aligns external threat-detection configuration and tests.
Show a summary per file
| File | Description |
|---|---|
pkg/constants/constants.go |
Defines the Codex home expression. |
pkg/workflow/codex_engine.go |
Uses the new home and links diagnostics. |
pkg/workflow/codex_engine_test.go |
Verifies Codex home and log linking. |
pkg/workflow/mcp_environment.go |
Exposes the new home during MCP setup. |
pkg/workflow/mcp_environment_test.go |
Updates MCP environment expectations. |
pkg/workflow/threat_detection_external.go |
Stages external detector config under the new home. |
pkg/workflow/threat_detection_isolation_test.go |
Verifies detector config placement. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (2)
pkg/workflow/codex_engine.go:388
- This second non-firewall command has the same directory-replacement problem:
ln -sfnnests the link when/tmp/gh-aw/mcp-config/logsalready exists as a directory, leaving diagnostics consumers on the stale directory. Remove that entry before creating the compatibility symlink.
ln -sfn "$CODEX_HOME/logs" %s/logs
pkg/workflow/codex_engine.go:404
- The firewall path also uses
ln -sfn, which nests the symlink instead of replacing a pre-existing real logs directory. On a persistent runner this leavesGetInternalLogsDir()pointed at stale/empty data. Explicitly remove the fixed legacy path before linking it to the new CODEX_HOME logs.
base := fmt.Sprintf("mkdir -p \"$CODEX_HOME/logs\" %s && ln -sfn \"$CODEX_HOME/logs\" %s/logs && touch %s", constants.TmpMcpConfigDir, constants.TmpMcpConfigDir, AgentStepSummaryPath)
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Balanced
| fmt.Sprintf(" if: %s\n", detectionStepCondition), | ||
| " run: |\n", | ||
| fmt.Sprintf(" mkdir -p %q %q %q\n", constants.ShellMcpConfigDir, constants.TmpMcpConfigDir, constants.TmpMcpConfigLogsDir), | ||
| fmt.Sprintf(" mkdir -p %q %q %q\n", constants.ShellMcpConfigDir, constants.CodexHomeDirExpr, constants.TmpMcpConfigLogsDir), |
| ln -sfn "$CODEX_HOME/logs" %s/logs | ||
| %s%s 2>&1 | tee %s`, AgentCLIStartMsPath, AgentStepSummaryPath, logFile, codexTmpConfigDir, codexTmpConfigDir, schemaWritePrefix, codexCommand, logFile) |
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #54687 does not have the 'implementation' label and has only 26 new lines of code in business logic directories (threshold: 100).
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully!
|
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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
This moves CODEX_HOME to a sane writable location, but the compatibility story is still incomplete: the external detector path does not install the same legacy-log shim, and the ln -sfn approach is stateful enough to fail on runners where /tmp/gh-aw/mcp-config/logs already exists as a real directory.
Blocking themes
- The external threat-detect flow now diverges from the main Codex runtime and can still strand diagnostics at the old path.
- The legacy log-path migration is nondeterministic because
ln -sfndoes not replace an existing directory.
Those are both correctness issues in the recovery path for a fleet-wide outage fix, so I don't think this is safe to merge yet.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 8.63 AIC · ⌖ 6.96 AIC · ⊞ 7K
Comment /review to run again
| fmt.Sprintf(" if: %s\n", detectionStepCondition), | ||
| " run: |\n", | ||
| fmt.Sprintf(" mkdir -p %q %q %q\n", constants.ShellMcpConfigDir, constants.TmpMcpConfigDir, constants.TmpMcpConfigLogsDir), | ||
| fmt.Sprintf(" mkdir -p %q %q %q\n", constants.ShellMcpConfigDir, constants.CodexHomeDirExpr, constants.TmpMcpConfigLogsDir), |
There was a problem hiding this comment.
This only changes CODEX_HOME in the env block, but the external detector path never runs buildCodexExecutionCommand/codexPathSetup, so it still materializes /tmp/gh-aw/mcp-config/logs as a plain directory and never creates the compatibility symlink there. Any detector-side code that still reads the legacy logs path will now silently miss Codex logs, which breaks diagnostics exactly on the path this PR is trying to stabilize.
💡 Why this is still broken
The detector setup here does:
mkdir -p "/tmp/gh-aw/mcp-config/logs"but it never does the new:
ln -sfn "$CODEX_HOME/logs" /tmp/gh-aw/mcp-config/logsBecause the external detector reuses only the rendered env: block from the engine step, it does not inherit the runtime compatibility shim added in codex_engine.go. That leaves two different Codex log layouts depending on which execution path the workflow takes.
Please mirror the same symlink setup in the detector preparation path, or stop creating the legacy directory there and route all consumers to the workspace-backed location explicitly.
| mkdir -p "$CODEX_HOME/logs" | ||
| %s%s 2>&1 | tee %s`, AgentCLIStartMsPath, AgentStepSummaryPath, logFile, schemaWritePrefix, codexCommand, logFile) | ||
| mkdir -p "$CODEX_HOME/logs" %s | ||
| ln -sfn "$CODEX_HOME/logs" %s/logs |
There was a problem hiding this comment.
ln -sfn is not enough here because it does not replace an existing real directory, so if /tmp/gh-aw/mcp-config/logs already exists from an earlier attempt you end up with /tmp/gh-aw/mcp-config/logs/logs -> ... instead of repointing the legacy path. That leaves later readers on the stale directory and makes the fix nondeterministic across reused runners.
💡 Why this can still fail after the first bad run
The compatibility path was historically a directory, and /tmp/gh-aw is not guaranteed to be pristine on every invocation. With a pre-existing directory:
ln -sfn "$CODEX_HOME/logs" /tmp/gh-aw/mcp-config/logscreates a nested symlink inside that directory rather than replacing it. Consumers opening /tmp/gh-aw/mcp-config/logs/... still hit the old directory tree, so log mirroring silently stops working depending on runner state.
Please remove or rename the destination when it is a directory before creating the symlink, or copy logs into the legacy path instead of relying on ln -sfn semantics.
Test Quality Assessment 🧪SummaryThis PR is a refactoring/maintenance PR that updates existing test assertions to track constant value changes in production code. No new behavioral test functions were added. Test Changes
Modified Tests
Quality ScoreScore: N/A — Infrastructure/Maintenance Only This PR contains only test maintenance (assertion updates tracking constant refactoring) rather than new behavioral test coverage. Recommendation✅ Approve — Test changes are appropriate maintenance for the production code refactoring. All modified tests continue to verify the same behavioral contracts. No guideline violations detected.
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: Infrastructure/Maintenance Only
Test changes are appropriate maintenance for the production code refactoring. All modified tests continue to verify the same behavioral contracts. No new behavioral tests added, and no guideline violations detected.
|
/smoke-codex |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes on one correctness bug and two maintainability gaps.
📋 Key Themes & Highlights
Key Issues
- 🐛 Correctness (high):
constants.CodexHomeDirExpris an Actions expression string (${{ github.workspace }}/...). It is passed tomkdir -p %qinside the external-detector shellrun:block. The Actions runtime resolves${{ ... }}expressions inenv:andrun:YAML values, but not when the string appears as a literal argument inside a shell heredoc orfmt.Sprintfformat — the directory created will be literally named${{ github.workspace }}/.gh-aw/codex-home, which will fail or create a stray directory. A shell-expanded constant (e.g.$GITHUB_WORKSPACE/.gh-aw/codex-home) is needed for shell contexts. - 🧪 Test gap:
codexPathSetupgained the sameln -sfnbehavior asbuildCodexExecutionCommand, but no test covers the symlink on the non-harness path. - 🔁 Duplication: The
mkdir + ln -sfnblock is copy-pasted across bothfmt.Sprintfbranches; a small helper would unify it.
Positive Highlights
- ✅ Root cause correctly identified:
/tmprestriction blocks MCP helper alias creation;github.workspaceis the right fix for the agent step. - ✅ Backward-compatibility preserved via the
ln -sfnlog mirror. - ✅ Tests updated to reference the constant rather than hardcoding the path.
- ✅
CodexHomeDirExprcomment clearly documents the Actions-expression nature of the value.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 35.3 AIC · ⌖ 10.1 AIC · ⊞ 7.6K
Comment /matt to run again
| @@ -398,7 +401,7 @@ func (e *CodexEngine) codexAllowedDomains(workflowData *WorkflowData) string { | |||
| } | |||
|
|
|||
| func (e *CodexEngine) codexPathSetup(workflowData *WorkflowData, detectionSchemaWriteCmd string) string { | |||
There was a problem hiding this comment.
[/tdd] codexPathSetup now has the same ln -sfn behavior as buildCodexExecutionCommand, but there is no test asserting it. The test added in codex_engine_test.go only covers buildCodexExecutionCommand; the harness-less code path through codexPathSetup is untested for the symlink.
💡 Suggested test
Add a case to the existing codexPathSetup test (or add a new one) that asserts:
assert.Contains(t, result, `ln -sfn "$CODEX_HOME/logs" /tmp/gh-aw/mcp-config/logs`)Without this, a future edit to codexPathSetup could silently drop the symlink and break MCP log consumers on the non-harness path.
@copilot please address this.
| codexTmpConfigDir := constants.TmpMcpConfigDir | ||
| if harnessScriptName != "" { | ||
| return fmt.Sprintf(`set -o pipefail | ||
| printf '%%s' "$(date +%%s%%3N)" > %s |
There was a problem hiding this comment.
[/codebase-design] The mkdir -p ... && ln -sfn ... block is now duplicated verbatim across both the harness and non-harness fmt.Sprintf branches in buildCodexExecutionCommand.
💡 Suggestion
Extract a small helper that returns the setup snippet:
func codexLogSetupSnippet(tmpConfigDir string) string {
return fmt.Sprintf("mkdir -p \"$CODEX_HOME/logs\" %s\nln -sfn \"$CODEX_HOME/logs\" %s/logs", tmpConfigDir, tmpConfigDir)
}This collapses the duplication and makes the invariant (CODEX_HOME/logs ↔ /tmp symlink) visible in one place — easier to maintain and harder to partially update.
@copilot please address this.
| fmt.Sprintf(" if: %s\n", detectionStepCondition), | ||
| " run: |\n", | ||
| fmt.Sprintf(" mkdir -p %q %q %q\n", constants.ShellMcpConfigDir, constants.TmpMcpConfigDir, constants.TmpMcpConfigLogsDir), | ||
| fmt.Sprintf(" mkdir -p %q %q %q\n", constants.ShellMcpConfigDir, constants.CodexHomeDirExpr, constants.TmpMcpConfigLogsDir), |
There was a problem hiding this comment.
[/diagnosing-bugs] constants.CodexHomeDirExpr is a GitHub Actions expression string (${{ github.workspace }}/...), not a resolved shell path. Passing it to mkdir -p %q will literally try to create a directory named ${{ github.workspace }}/.gh-aw/codex-home, which will fail at runtime.
💡 Fix
The external detector step is a raw shell run: block. Either:
- Use
constants.ShellMcpConfigDir-style shell-expanded form (e.g.$GITHUB_WORKSPACE/.gh-aw/codex-home) and introduce aCodexHomeDirShellconstant, or - Add a
CODEX_HOMEenv var to this step (like the agent step does) and then reference"$CODEX_HOME"in the shell command.
Using an unresolved Actions expression inside %q in a shell heredoc will produce a broken mkdir call.
@copilot please address this.
There was a problem hiding this comment.
Review: Codex engine — move CODEX_HOME off /tmp
The intent is correct: using ${{ github.workspace }} instead of /tmp means Codex can create MCP helper aliases in a repo-relative writable location that survives across steps and satisfies tooling that requires a persistent home directory.
No new blocking issues found beyond the two already flagged in open review threads:
ln -sfnidempotency (codex_engine.go:380): if/tmp/gh-aw/mcp-config/logsalready exists as a real directory from a prior run on a non-ephemeral runner,ln -sfnsilently nests the symlink inside it rather than replacing it. Arm -rf /tmp/gh-aw/mcp-config/logsbefore thelnwould make this safe.- External detector logs gap (
threat_detection_external.go:31): the detection step createsTmpMcpConfigLogsDiras a real directory but never runs thelnmirroring logic, so diagnostics that read from/tmp/gh-aw/mcp-config/logswill not see Codex log output for external detector runs.
Non-blocking observations:
%q-formattingCodexHomeDirExprinmkdir -pis safe: GitHub Actions expands${{ github.workspace }}at the YAML layer before the shell runs.- Test coverage is appropriately updated; the new assertions on the symlink line and on
CodexHomeDirExprare accurate.
Please address the two open review threads before merging.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 25.4 AIC · ⌖ 9.08 AIC · ⊞ 6.2K
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation...
|
There was a problem hiding this comment.
Ponytail review — over-engineering only.
net: -6 lines possible.
Generated by ✂️ Ponytail Reviewer for #54687 · auto · 23.3 AIC · ⌖ 4.45 AIC · ⊞ 7.3K
Comment /ponytail to run again
| if workflowData.IsDetectionRun { | ||
| schemaWritePrefix = detectionSchemaWriteCmd + " && " | ||
| } | ||
| codexTmpConfigDir := constants.TmpMcpConfigDir |
There was a problem hiding this comment.
L372: yagni: codexTmpConfigDir is a one-line alias for constants.TmpMcpConfigDir used only to shorten two Sprintf calls below. Use constants.TmpMcpConfigDir directly at both call sites, delete the variable.
|
|
||
| func (e *CodexEngine) codexPathSetup(workflowData *WorkflowData, detectionSchemaWriteCmd string) string { | ||
| base := "mkdir -p \"$CODEX_HOME/logs\" && touch " + AgentStepSummaryPath | ||
| base := fmt.Sprintf("mkdir -p \"$CODEX_HOME/logs\" %s && ln -sfn \"$CODEX_HOME/logs\" %s/logs && touch %s", constants.TmpMcpConfigDir, constants.TmpMcpConfigDir, AgentStepSummaryPath) |
There was a problem hiding this comment.
L376-379, L385-388, L404: shrink: the mkdir -p "$CODEX_HOME/logs" X && ln -sfn "$CODEX_HOME/logs" X/logs pair is now duplicated three times (twice in buildCodexExecutionCommand, once in codexPathSetup). Extract one helper, e.g. codexLogsBridgeCmd(), and call it from all three sites.
|
@copilot CI is failing on this PR and it still needs forward progress. Please:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…ary-path Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in commit
I also invoked |
PR Triage
|
PR TriageCategory: Codex CODEX_HOME fix, fleet-wide impact. Blocked mergeable + 2x CHANGES_REQUESTED. Automated triage — run 32572524009
|
Codex runs were failing at turn 0 across multiple workflows because helper alias creation was blocked when
CODEX_HOMEwas under/tmp, producing a fleet-wide outage pattern instead of a single-workflow misconfiguration. This change applies the fix at engine config level so all Codex workflows inherit the same valid runtime path behavior.Root-cause correction (engine-level)
\${{ github.workspace }}/.gh-aw/codex-home./tmp/gh-aw/mcp-config.Codex runtime/log path compatibility
/tmp/gh-aw/mcp-config/logsdiagnostics path by linking it to$CODEX_HOME/logs, so existing log consumers continue to work.Threat-detection Codex path alignment
config.tomlinto the same workspace-backedCODEX_HOME, keeping detector and agent paths consistent.Constant semantics clarity
CodexHomeDirExpris an Actions-expression path (not a shell-resolved literal) to prevent misuse in non-YAML contexts.run: https://github.com/github/gh-aw/actions/runs/32547644097> Generated by 👨🍳 PR Sous Chef · gpt54 · 19.9 AIC · ⌖ 9.76 AIC · ⊞ 9.5K · ◷