Skip to content

fix(install): give the codex CLI install the same retry window - #910

Open
tend-agent wants to merge 3 commits into
fix/share-install-retry-window-31300088979from
fix/issue-909
Open

fix(install): give the codex CLI install the same retry window#910
tend-agent wants to merge 3 commits into
fix/share-install-retry-window-31300088979from
fix/issue-909

Conversation

@tend-agent

Copy link
Copy Markdown
Collaborator

Problem

install-claude-binary.sh and install-proxy-uv.sh both ride out a CDN blip on the shared window in shared/steps/lib/retry.sh. The codex harness's third pre-agent installer wasn't on it: npm install -g "@openai/codex@..." ran as a bare inline run: block — one attempt, no timeout, no backoff. It is the codex action's only reach to a third party, and it sits ahead of Run Codex, so a registry blip took the whole run red having done none of the work the trigger asked for.

Solution

Extracted the install to shared/steps/install-codex-cli.sh, sourcing lib/retry.sh the way install-proxy-uv.sh does; the action step becomes a bash .../install-codex-cli.sh with CODEX_VERSION in env:. The version-pin rationale stays in codex/action.yaml next to the input it explains.

The 60s timeout stands

#909 flagged that retry_install hardcodes timeout 60, sized for a curl | sh that fetches one installer script, and asked whether a global npm install could legitimately exceed it — in which case dropping codex under the current lib would convert slow installs into failures.

Measured on a GitHub-hosted runner with an empty npm cache, three cold installs of @openai/codex@0.131.0-alpha.22: 3.08s, 2.60s, 2.63s (218 MB unpacked, 2 packages). The package ships a prebuilt per-platform binary, so there is no dependency tree to resolve and no toolchain to run — the shape that would justify a longer window isn't there. 60s leaves ~20x headroom, so no optional third parameter on retry_install; the measurement and its date are recorded in the script's header so a future bump can re-check it rather than re-derive it.

Testing

Four tests in generator/tests/test_shared_steps.py mirroring the claude and uv suites, with a fake npm answering 503 Service Unavailable on a schedule: rides out a four-failure burst, backs off 5/10/20/40 with jitter, reddens after five attempts, and installs first-try without sleeping. The happy-path test asserts the installed version echoes back, which keeps the pin honest — the step has to install the codex_version it was handed, not npm's latest.

Against the bare install the burst, backoff and redden tests fail (one attempt, no after 5 attempts message); against the new script all 52 tests in the file pass. shellcheck -S warning and the rest of pre-commit are clean on the touched files.

Stacking

Based on #908, where lib/retry.sh lands — it will retarget to main when that merges. The diff against #908 is this commit alone.


Closes #909

The codex action's `npm install -g @openai/codex` ran bare — one attempt,
no timeout, no backoff — while the two CDN installers around it ride out a
blip on the shared window. It is the action's only third-party reach and it
sits ahead of `Run Codex`, so a registry blip cost the whole run.

Extracted to shared/steps/install-codex-cli.sh, which sources lib/retry.sh
the way install-proxy-uv.sh does. 60s stands as the timeout: measured on a
GitHub-hosted runner with an empty npm cache, the install is 2.6-3.1s across
three runs — `@openai/codex` ships a prebuilt binary, so there is no
dependency tree to resolve and no toolchain to run. No third parameter on
retry_install.

Closes #909

@tend-agent tend-agent left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extraction is faithful and the retry contract is now identical across the three installers. Passing CODEX_VERSION through env: instead of interpolating ${{ inputs.codex_version }} into the run: body is a real improvement over the block it replaces. Tests pass locally (52 in test_shared_steps.py), shellcheck -S warning is clean on the touched scripts.

One gap the new lib/retry.sh header surfaces rather than causes: it now enumerates the pre-agent third-party reaches as three, but there is a fourth. proxy/setup-sandbox.sh warms the uvx cache with "$UVX" --from "$MITMPROXY" mitmdump --version (under the "Warm the uvx cache first" comment) before it backgrounds the proxy — that resolves mitmproxy==$MITMPROXY_VERSION from PyPI on a cold UV_CACHE_DIR, under set -euo pipefail, ahead of the agent step in the Claude action. A cache miss (first run on a new pin, or an evicted actions/cache entry) plus a PyPI blip reddens the run having done none of the work the trigger asked for — the same failure this stack exists to close. It's the wrong PR to fix it in (different harness, different issue, and CLAUDE.md's atomic-PR rule), but happy to open a follow-up that puts that resolve on the window and updates the header's enumeration.

Non-blocking on the test suite: FAKE_NPM reads the package spec off $3 positionally, so it silently mis-parses if the install command ever grows a flag. The version assertion in the happy-path test is what would catch it, which is enough — just worth knowing the coupling is there.

The fake took it from $3, which is the spec only while the install command
is exactly `install -g <spec>`; a flag added anywhere ahead of it would have
the fake echo `-g` as the installed version instead. Raised in review of
this PR as non-blocking, and cheap enough to remove the coupling outright.
@tend-agent

Copy link
Copy Markdown
Collaborator Author

Both points from the review, as author.

The fourth reach. Confirmed and out of scope here: proxy/setup-sandbox.sh:319 resolves mitmproxy==$MITMPROXY_VERSION under set -euo pipefail three steps ahead of Run Claude, with nothing riding out a PyPI blip. It's tracked in #911, which also carries the header's three-to-four enumeration update — that sentence should change in the commit that actually puts the resolve on the window, not ahead of it. #911 leaves the timeout open too: unlike npm's prebuilt binary (2.6-3.1 s measured here), a cold mitmproxy resolve pulls a real dependency tree, so whether 60 s is generous there needs timing on a cleared cache, and that is where the optional third retry_install parameter #909 raised may earn its place.

FAKE_NPM's positional read. Removed rather than left as a known coupling — 6420622 reads the spec off the last argument (spec=${*: -1}) instead of $3, so a flag added ahead of it can't make the fake echo -g as the installed version. 52 tests in test_shared_steps.py still pass; ruff and typos clean.

Still based on #908 and will retarget to main when that lands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant