Fix codex engine on self-hosted runners: prefer npm-installed binary over vendored system binary#20161
Fix codex engine on self-hosted runners: prefer npm-installed binary over vendored system binary#20161
Conversation
…up to prefer workflow-installed binary over vendored system binary Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Hey This PR is well-aligned with the contribution guidelines. Here's a quick summary of the checklist:
One minor note: the PR is currently in draft state — remember to mark it ready for review once any final validation passes. Everything looks good — this PR is ready for maintainer review once it's promoted out of draft! 🚀
|
|
❌ Smoke Project encountered failures. Check the logs for details. |
|
📰 DEVELOPING STORY: Smoke Copilot ARM64 reports was cancelled. Our correspondents are investigating the incident... |
|
❌ Smoke Temporary ID encountered failures. Check the logs for details. |
|
❌ Smoke Multi PR failed to create multiple PRs. Check the logs. |
|
📰 DEVELOPING STORY: Smoke Copilot reports was cancelled. Our correspondents are investigating the incident... |
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
✅ All tools validated successfully! Agent Container Smoke Test confirms agent container is ready. |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
There was a problem hiding this comment.
Pull request overview
This PR fixes Codex CLI resolution on self-hosted runners by ensuring the workflow-installed npm global binary takes precedence over any preinstalled/vendored codex already present on the runner’s PATH.
Changes:
- Updated
GetNpmBinPathSetup()to prepend the npm global prefixbindirectory ahead of hostedtoolcache paths. - Injected the same npm-prefix PATH setup into Codex non-AWF execution commands (both standard and agent-file variants).
- Updated and added tests to assert PATH setup ordering and non-AWF command inclusion.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/workflow/engine_helpers.go | Prepends npm global prefix bin to PATH in GetNpmBinPathSetup() before hostedtoolcache bins. |
| pkg/workflow/codex_engine.go | Adds npm-prefix PATH export to non-AWF Codex execution command templates. |
| pkg/workflow/engine_helpers_test.go | Extends PATH-setup assertions and refactors the “no GOROOT” chain test to use the function output. |
| pkg/workflow/codex_engine_test.go | Adds coverage ensuring non-AWF Codex execution includes npm prefix PATH setup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
pkg/workflow/engine_helpers.go
Outdated
| // ensures the Go version set by actions/setup-go takes precedence. | ||
| // AWF's entrypoint.sh exports GOROOT before the user command runs. | ||
| return `export PATH="$(find /opt/hostedtoolcache -maxdepth 4 -type d -name bin 2>/dev/null | tr '\n' ':')$PATH"; [ -n "$GOROOT" ] && export PATH="$GOROOT/bin:$PATH" || true` | ||
| return `export PATH="$(npm config get prefix 2>/dev/null)/bin:$(find /opt/hostedtoolcache -maxdepth 4 -type d -name bin 2>/dev/null | tr '\n' ':')$PATH"; [ -n "$GOROOT" ] && export PATH="$GOROOT/bin:$PATH" || true` |
There was a problem hiding this comment.
$(npm config get prefix)/bin will expand to /bin when npm config get prefix returns an empty string (or npm is unavailable), which unintentionally prepends /bin and can change command resolution. Consider using npm bin -g (returns the bin dir directly) or guarding with a non-empty prefix (e.g., only prepend when the computed prefix/bin path is non-empty) to avoid injecting /bin: into PATH.
| return `export PATH="$(npm config get prefix 2>/dev/null)/bin:$(find /opt/hostedtoolcache -maxdepth 4 -type d -name bin 2>/dev/null | tr '\n' ':')$PATH"; [ -n "$GOROOT" ] && export PATH="$GOROOT/bin:$PATH" || true` | |
| return `NPM_PREFIX="$(npm config get prefix 2>/dev/null || true)"; HOSTED_BINS="$(find /opt/hostedtoolcache -maxdepth 4 -type d -name bin 2>/dev/null | tr '\n' ':')"; if [ -n "$NPM_PREFIX" ]; then export PATH="$NPM_PREFIX/bin:${HOSTED_BINS}$PATH"; else export PATH="${HOSTED_BINS}$PATH"; fi; [ -n "$GOROOT" ] && export PATH="$GOROOT/bin:$PATH" || true` |
pkg/workflow/codex_engine.go
Outdated
| // Prepend the npm global prefix bin to PATH so the workflow-installed (latest) codex | ||
| // binary takes precedence over any vendored/system-installed binary on self-hosted runners. | ||
| // This mirrors the PATH setup used in AWF mode via GetNpmBinPathSetup(). | ||
| npmPrefixSetup := `export PATH="$(npm config get prefix 2>/dev/null)/bin:$PATH"` |
There was a problem hiding this comment.
The PATH setup here has the same /bin injection edge case as GetNpmBinPathSetup: if npm config get prefix is empty (or npm isn't found), the export becomes PATH="/bin:$PATH", which can unintentionally change command resolution. Prefer npm bin -g or conditionally prepend only when the computed directory is non-empty.
| // Prepend the npm global prefix bin to PATH so the workflow-installed (latest) codex | |
| // binary takes precedence over any vendored/system-installed binary on self-hosted runners. | |
| // This mirrors the PATH setup used in AWF mode via GetNpmBinPathSetup(). | |
| npmPrefixSetup := `export PATH="$(npm config get prefix 2>/dev/null)/bin:$PATH"` | |
| // Prepend the npm global bin directory to PATH so the workflow-installed (latest) codex | |
| // binary takes precedence over any vendored/system-installed binary on self-hosted runners. | |
| // This mirrors the PATH setup used in AWF mode via GetNpmBinPathSetup(). | |
| npmPrefixSetup := `npm_bin_dir="$(npm bin -g 2>/dev/null || true)" | |
| if [ -n "$npm_bin_dir" ]; then | |
| export PATH="$npm_bin_dir:$PATH" | |
| fi` |
|
Commit pushed:
|
Agent Container Tool Check
Result: 12/12 tools available ✅ Overall Status: PASS
|
|
Smoke test 22849956541 by
Overall:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR correctly fixes the codex path resolution issue on self-hosted runners by prepending the npm global prefix bin directory to PATH in both AWF and non-AWF execution paths. The fix is clean, well-documented, and includes comprehensive tests. No concerns.
📰 BREAKING: Report filed by Smoke Copilot
pkg/workflow/engine_helpers.go
Outdated
| // ensures the Go version set by actions/setup-go takes precedence. | ||
| // AWF's entrypoint.sh exports GOROOT before the user command runs. | ||
| return `export PATH="$(find /opt/hostedtoolcache -maxdepth 4 -type d -name bin 2>/dev/null | tr '\n' ':')$PATH"; [ -n "$GOROOT" ] && export PATH="$GOROOT/bin:$PATH" || true` | ||
| return `export PATH="$(npm config get prefix 2>/dev/null)/bin:$(find /opt/hostedtoolcache -maxdepth 4 -type d -name bin 2>/dev/null | tr '\n' ':')$PATH"; [ -n "$GOROOT" ] && export PATH="$GOROOT/bin:$PATH" || true` |
There was a problem hiding this comment.
Prepending the npm global prefix bin first is the correct fix for the self-hosted runner issue. This ensures the workflow-installed codex binary (with --dangerously-bypass-approvals-and-sandbox) takes precedence over any vendored binary in the system PATH.
| } else { | ||
| // Build the command without AWF wrapping | ||
| // Reuse commandName already determined above | ||
| // |
There was a problem hiding this comment.
Good defensive approach - using 2>/dev/null with the npm command prevents errors from causing issues when npm isn't available in the PATH. The fallback gracefully results in an empty prefix which keeps the PATH setup functional.
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
@copilot there are multiple patching of PATH in a single command, can it be made cleaner? |
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
💥 [THE END] — Illustrated by Smoke Claude
pkg/workflow/codex_engine.go
Outdated
| // Prepend the npm global prefix bin to PATH so the workflow-installed (latest) codex | ||
| // binary takes precedence over any vendored/system-installed binary on self-hosted runners. | ||
| // This mirrors the PATH setup used in AWF mode via GetNpmBinPathSetup(). | ||
| npmPrefixSetup := `export PATH="$(npm config get prefix 2>/dev/null)/bin:$PATH"` |
There was a problem hiding this comment.
Good fix! Prepending the npm global bin ensures the workflow-installed Codex takes precedence. This is consistent with the AWF path setup in GetNpmBinPathSetup().
pkg/workflow/engine_helpers.go
Outdated
| // ensures the Go version set by actions/setup-go takes precedence. | ||
| // AWF's entrypoint.sh exports GOROOT before the user command runs. | ||
| return `export PATH="$(find /opt/hostedtoolcache -maxdepth 4 -type d -name bin 2>/dev/null | tr '\n' ':')$PATH"; [ -n "$GOROOT" ] && export PATH="$GOROOT/bin:$PATH" || true` | ||
| return `export PATH="$(npm config get prefix 2>/dev/null)/bin:$(find /opt/hostedtoolcache -maxdepth 4 -type d -name bin 2>/dev/null | tr '\n' ':')$PATH"; [ -n "$GOROOT" ] && export PATH="$GOROOT/bin:$PATH" || true` |
There was a problem hiding this comment.
The npm prefix is now correctly prepended before hostedtoolcache entries. This ensures the latest npm-installed binary wins over any vendored system binary on self-hosted runners.
…ween AWF and non-AWF codex modes Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 33e43ac. The non-AWF execution now reuses |
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
💥 [THE END] — Illustrated by Smoke Claude
| @@ -0,0 +1,4 @@ | |||
| --- | |||
| "gh-aw": patch | |||
There was a problem hiding this comment.
Good use of a changeset file for this patch. The description clearly explains the PATH setup change for npm-installed Codex binary on self-hosted runners.
| # shellcheck disable=SC1003 | ||
| sudo -E awf --env-all --container-workdir "${GITHUB_WORKSPACE}" --allow-domains "172.30.0.1,api.openai.com,api.snapcraft.io,archive.ubuntu.com,azure.archive.ubuntu.com,crl.geotrust.com,crl.globalsign.com,crl.identrust.com,crl.sectigo.com,crl.thawte.com,crl.usertrust.com,crl.verisign.com,crl3.digicert.com,crl4.digicert.com,crls.ssl.com,host.docker.internal,json-schema.org,json.schemastore.org,keyserver.ubuntu.com,ocsp.digicert.com,ocsp.geotrust.com,ocsp.globalsign.com,ocsp.identrust.com,ocsp.sectigo.com,ocsp.ssl.com,ocsp.thawte.com,ocsp.usertrust.com,ocsp.verisign.com,openai.com,packagecloud.io,packages.cloud.google.com,packages.microsoft.com,ppa.launchpad.net,s.symcb.com,s.symcd.com,security.ubuntu.com,ts-crl.ws.symantec.com,ts-ocsp.ws.symantec.com" --log-level info --proxy-logs-dir /tmp/gh-aw/sandbox/firewall/logs --enable-host-access --image-tag 0.23.0 --skip-pull --enable-api-proxy \ | ||
| -- /bin/bash -c 'export PATH="$(find /opt/hostedtoolcache -maxdepth 4 -type d -name bin 2>/dev/null | tr '\''\n'\'' '\'':'\'')$PATH"; [ -n "$GOROOT" ] && export PATH="$GOROOT/bin:$PATH" || true && INSTRUCTION="$(cat /tmp/gh-aw/aw-prompts/prompt.txt)" && codex ${GH_AW_MODEL_AGENT_CODEX:+-c model="$GH_AW_MODEL_AGENT_CODEX" }exec --dangerously-bypass-approvals-and-sandbox --skip-git-repo-check "$INSTRUCTION"' 2>&1 | tee -a /tmp/gh-aw/agent-stdio.log | ||
| -- /bin/bash -c 'export PATH="$(npm config get prefix 2>/dev/null)/bin:$(find /opt/hostedtoolcache -maxdepth 4 -type d -name bin 2>/dev/null | tr '\''\n'\'' '\'':'\'')$PATH"; [ -n "$GOROOT" ] && export PATH="$GOROOT/bin:$PATH" || true && INSTRUCTION="$(cat /tmp/gh-aw/aw-prompts/prompt.txt)" && codex ${GH_AW_MODEL_AGENT_CODEX:+-c model="$GH_AW_MODEL_AGENT_CODEX" }exec --dangerously-bypass-approvals-and-sandbox --skip-git-repo-check "$INSTRUCTION"' 2>&1 | tee -a /tmp/gh-aw/agent-stdio.log |
There was a problem hiding this comment.
The PATH setup change from find /opt/hostedtoolcache to prepending $(npm config get prefix 2>/dev/null)/bin: looks correct. This ensures npm-installed binaries take priority over vendored system binaries on self-hosted runners.
On self-hosted runners with a pre-installed (vendored)
codexbinary, the older system binary shadows the workflow-installed@openai/codex@latestin PATH, causing failures when the old binary doesn't support--dangerously-bypass-approvals-and-sandbox.Changes
GetNpmBinPathSetup()(AWF mode): Prepend the npm global prefixbindirectory before hostedtoolcache entries so the npm-installed binary wins over any system-installed one. Previously only/opt/hostedtoolcache/*/binwas added, which doesn't exist on most self-hosted runners. Uses a temp variable and${_npm_prefix:+${_npm_prefix}/bin:}parameter expansion to safely guard against an empty prefix (avoids injecting a bare/bininto PATH when npm is unavailable).Non-AWF execution commands in
codex_engine.go: Replaced the separate inline PATH export withGetNpmBinPathSetup(), making it the single source of truth for PATH setup in both AWF and non-AWF modes. Both agent-file and standard execution variants use the same function.On GitHub-hosted runners the npm prefix (
/usr/local/bin) is already in PATH, making this a no-op. On self-hosted runners it guarantees the freshly-installed latest binary is resolved first.TestGetNpmBinPathSetupto assert the_npm_prefixtemp var and${_npm_prefix:+...}expansion are present. RefreshedTestGetNpmBinPathSetup_GorootOrderingto derive from the actual function output. AddedTestGetNpmBinPathSetup_EmptyNpmPrefixDoesNotInjectBinverifying the empty-prefix guard. AddedTestCodexEngineNonAWFNpmPathSetupcovering both standard and agent-file non-AWF execution paths.💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
Changeset
Warning
The following domains were blocked by the firewall during workflow execution:
ab.chatgpt.comgithub.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.
✨ PR Review Safe Output Test - Run 22849956573
✨ PR Review Safe Output Test - Run 22853945663