fix(codex): warn when codex-shim install cannot prove routing (#1169) - #1289
Conversation
`ocx codex-shim install` reported a clean green success even when the launcher was installed but Codex routing was not provably pointed at OpenCodex — an external `model_provider`, a user-owned local or remote gateway, or routing that cannot be verified. It now reports a warning for those cases. It also warns when outbound proxy variables exist only in the current process while `config.proxy` is unset or unresolved, because Codex launchers and background services such as launchd may not inherit that environment. Proxy values are never printed. The change is advisory only: install still succeeds with the same exit code, and the shim still fail-open execs the real Codex launcher. Republished from #1169 by TyroneXie, whose branch was 335 commits behind dev. Rebased onto f5147cb with no conflicts; authorship preserved below. Co-authored-by: TyroneXie <328347833@qq.com>
📝 WalkthroughWalkthroughThe Codex shim installation flow now performs read-only routing and proxy readiness checks. It reports warnings for unverifiable or custom routes and process-local proxy settings, redacts proxy values, and documents the behavior in English and Chinese. ChangesCodex shim readiness
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CodexShimInstall
participant diagnoseCodexShim
participant collectCodexShimReadinessWarnings
participant CLIOutput
CodexShimInstall->>diagnoseCodexShim: diagnose installed shim
diagnoseCodexShim-->>CodexShimInstall: shim health
CodexShimInstall->>collectCodexShimReadinessWarnings: collect routing and proxy readiness
collectCodexShimReadinessWarnings-->>CodexShimInstall: warning strings
CodexShimInstall->>CLIOutput: report success or warnings
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/index.ts`:
- Around line 1168-1171: Update the Codex shim reporting around
diagnoseCodexShim so an installed but unhealthy diagnostic cannot produce a
green marker. Preserve readiness warnings for healthy shims, include the
diagnostic summary when healthy is false, and base the status icon on both
r.installed and the diagnostic health/warnings outcome.
- Around line 1167-1170: Wrap the readiness probe call in the codex-shim install
flow, specifically around collectCodexShimReadinessWarnings(), with a fail-open
try/catch. Preserve successful warning collection, but convert any probe failure
into a non-blocking warning so installCodexShim() still completes with exit
status zero.
In `@tests/codex-shim-readiness.test.ts`:
- Line 87: Replace the early Windows return in the readiness test with Bun’s
test.skipIf(process.platform === "win32") wrapper around the test definition,
preserving the existing install command and assertions while recording Windows
runs as skipped.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ed0b65ab-de5f-441a-b1b7-8d3f43354ba1
📒 Files selected for processing (5)
docs-site/src/content/docs/reference/cli/lifecycle.mddocs-site/src/content/docs/zh-cn/reference/cli/lifecycle.mdsrc/cli/codex-shim-readiness.tssrc/cli/index.tstests/codex-shim-readiness.test.ts
| const { collectCodexShimReadinessWarnings } = await import("./codex-shim-readiness"); | ||
| const warnings = diagnoseCodexShim().healthy | ||
| ? collectCodexShimReadinessWarnings() | ||
| : []; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether loadConfig throws on unreadable/corrupt config and whether the reader is guarded.
set -euo pipefail
ast-grep run --pattern 'export function loadConfig($$$) { $$$ }' --lang typescript src/config.ts | head -80
rg -nP -C4 '\b(readFileSync|JSON\.parse|throw)\b' src/config.ts | rg -n -C4 'loadConfig' || true
# Check whether any other CLI call site already wraps this readiness helper.
rg -nP -C4 'collectCodexShimReadinessWarnings' src testsRepository: lidge-jun/opencodex
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -u
printf 'Repo files count: '
git ls-files | wc -l
printf '\nLocate relevant files:\n'
fd -a 'index\.ts|codex-shim-readiness\.ts|config\.ts|inject\.ts|lifecycle\.md' . | sed 's#^\./##'
printf '\nSearch relevant symbols broadly:\n'
rg -n 'collectCodexShimReadinessWarnings|loadConfig|currentExternalCodexModelProvider|diagnoseCodexShim|installCodexShim|readConfig|config.json' . --glob '!node_modules' --glob '!dist' --glob '!build' || true
printf '\ngit status/stat:\n'
git status --short
git diff --stat || trueRepository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -u
printf 'src/cli/index.ts install section:\n'
sed -n '1145,1182p' src/cli/index.ts | cat -n
printf '\nsrc/cli/codex-shim-readiness.ts:\n'
sed -n '1,120p' src/cli/codex-shim-readiness.ts | cat -n
printf '\nsrc/codex/inject.ts relevant external provider functions:\n'
rg -n -C5 'currentExternalCodexModelProvider|shouldInjectApiAuthHeader|getCodexRoutingKind' src/codex/inject.ts
printf '\nRelevant lifecycle doc excerpt:\n'
sed -n '276,302p' docs-site/src/content/docs/reference/cli/lifecycle.md | cat -nRepository: lidge-jun/opencodex
Length of output: 11723
🏁 Script executed:
#!/bin/bash
set -u
printf 'src/config.ts loadConfig summary area:\n'
sed -n '924,1045p' src/config.ts | cat -n
printf '\nconfig migration/read-error handling tests around loadConfig failures:\n'
sed -n '160,190p' tests/config.test.ts | cat -n
sed -n '853,880p' tests/config.test.ts | cat -n
sed -n '976,1000p' tests/config.test.ts | cat -n
printf '\nCodex inject relevant reads around currentExternalCodexModelProvider/getCodexRoutingKind:\n'
sed -n '68,92p' src/codex/inject.ts | cat -n
sed -n '378,398p' src/codex/inject.ts | cat -n
printf '\nBehavioral probe of currentExternalCodexModelProvider pattern:\n'
node - <<'JS'
const fs = require('fs');
const path = '/tmp/opencodex-codex-readiness-probe-toml';
for (const content of ['x_model_provider = "abc"\n', '[model]\nx_model_provider = "abc"\n']) {
fs.writeFileSync(path, content, 'utf8');
const exists = fs.existsSync(path);
try {
const actual = exists ? fs.readFileSync(path, 'utf8') : null;
console.log('before content=', content.trim(), 'exists=', exists, 'read=', actual.trim());
} catch (error) {
console.log('before content=', content.trim(), 'exists=', exists, 'read_threw=', error.code);
}
try {
fs.unlinkSync(path);
} catch {}
}
JSRepository: lidge-jun/opencodex
Length of output: 13349
Guard the readiness probe on successful shim install.
src/cli/index.ts:1168-1169 calls collectCodexShimReadinessWarnings() directly after installCodexShim() succeeds. That helper calls loadConfig() and currentExternalCodexModelProvider(), and currentExternalCodexModelProvider() can throw between existsSync(CODEX_CONFIG_PATH) and readFileSync(CODEX_CONFIG_PATH) when the file is removed or becomes unreadable. As written, an advisory/read-only check can abort the ocx codex-shim install case and exit non-zero after the shim was installed, contradicting the advisory, read-only behavior documented in docs-site/src/content/docs/reference/cli/lifecycle.md. Wrap this call in a fail-open try/catch so install returns zero and converts probe failures to non-blocking warnings.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/index.ts` around lines 1167 - 1170, Wrap the readiness probe call in
the codex-shim install flow, specifically around
collectCodexShimReadinessWarnings(), with a fail-open try/catch. Preserve
successful warning collection, but convert any probe failure into a non-blocking
warning so installCodexShim() still completes with exit status zero.
| const warnings = diagnoseCodexShim().healthy | ||
| ? collectCodexShimReadinessWarnings() | ||
| : []; | ||
| console.log(`${r.installed && warnings.length === 0 ? "✅ " : "⚠️ "}${r.message}`); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Report an unhealthy shim instead of discarding the diagnostic verdict.
diagnoseCodexShim() can return installed: true with healthy: false (src/codex/shim.ts lines 1179-1211: a missing wrapper, a missing backup, or a wrapper that is not an OpenCodex shim). In that case line 1170 sets warnings to [], and line 1171 prints ✅ because r.installed is true and warnings.length === 0. The command therefore reports a green result for the exact state the new diagnostic was added to detect.
Suppress the green marker when the diagnostic is not healthy, and surface the diagnostic summary.
🐛 Proposed fix: honor the unhealthy verdict
- const warnings = diagnoseCodexShim().healthy
- ? collectCodexShimReadinessWarnings()
- : [];
- console.log(`${r.installed && warnings.length === 0 ? "✅ " : "⚠️ "}${r.message}`);
+ const diagnostic = diagnoseCodexShim();
+ const warnings = diagnostic.healthy
+ ? collectCodexShimReadinessWarnings()
+ : [diagnostic.summary];
+ console.log(`${r.installed && diagnostic.healthy && warnings.length === 0 ? "✅ " : "⚠️ "}${r.message}`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const warnings = diagnoseCodexShim().healthy | |
| ? collectCodexShimReadinessWarnings() | |
| : []; | |
| console.log(`${r.installed && warnings.length === 0 ? "✅ " : "⚠️ "}${r.message}`); | |
| const diagnostic = diagnoseCodexShim(); | |
| const warnings = diagnostic.healthy | |
| ? collectCodexShimReadinessWarnings() | |
| : [diagnostic.summary]; | |
| console.log(`${r.installed && diagnostic.healthy && warnings.length === 0 ? "✅ " : "⚠️ "}${r.message}`); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/index.ts` around lines 1168 - 1171, Update the Codex shim reporting
around diagnoseCodexShim so an installed but unhealthy diagnostic cannot produce
a green marker. Preserve readiness warnings for healthy shims, include the
diagnostic summary when healthy is false, and base the status icon on both
r.installed and the diagnostic health/warnings outcome.
| }); | ||
|
|
||
| test("the install command surfaces readiness warnings without leaking the proxy URL", () => { | ||
| if (process.platform === "win32") return; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Bun test runner test.skipIf conditional skip API
💡 Result:
In Bun's test runner, test.skipIf() is a modifier used to conditionally skip a test based on a provided boolean condition [1]. Usage The test.skipIf(condition) method accepts a boolean value [2]. If the condition evaluates to true, the test will be skipped [2][3]. Example ts import { test } from "bun:test"; const isMacOS = process.platform === "darwin"; test.skipIf(isMacOS)("only runs on non-macOS platforms", () => { // This test will be skipped if the platform is macOS }); Key Details - Scope: It can be used directly on individual tests [1] or on describe blocks as describe.skipIf() to skip groups of tests [4][5]. - Complementary API: If you want to run a test only when a condition is met (the opposite of skipping), you can use .if() (e.g., test.if(condition)(...)) [2][1]. - Related Modifiers: Other similar conditional modifiers exist, such as todoIf(condition), which marks a test as "to-do" only if the condition is met [3][6]. Documentation for these APIs can be found in the bun:test module reference [4][5].
Citations:
- 1: https://bun.com/docs/test/writing-tests
- 2: https://bun.com/reference/bun/test/Test/skipIf
- 3: https://bun.sh/reference/bun/test/Test/skipIf
- 4: https://bun.com/reference/bun/test
- 5: https://bun.sh/reference/bun/test
- 6: https://bun.sh/reference/bun/test/Test
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant file outline:"
ast-grep outline tests/codex-shim-readiness.test.ts --view compact || true
echo
echo "Relevant test section:"
nl -ba tests/codex-shim-readiness.test.ts | sed -n '70,110p'
echo
echo "Other early Windows returns and skipIf usage in tests:"
rg -n 'process\.platform === "win32"(?!\s*&&)|\.skipIf\(|\.only\(' tests || trueRepository: lidge-jun/opencodex
Length of output: 395
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant test section:"
awk '{printf "%6d\t%s\n", NR, $0}' tests/codex-shim-readiness.test.ts | sed -n '70,110p'
echo
echo "Other early Windows returns and skipIf usage in tests:"
rg -n 'process\.platform === "win32"(?!\s*&&)|\.skipIf\(|\.only\(' tests || trueRepository: lidge-jun/opencodex
Length of output: 2195
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Early Windows returns:"
grep -n 'process\.platform === "win32"' tests || true
echo
echo "Other skipIf/only usage in tests:"
grep -n '\.skipIf(\|\.only(' tests || trueRepository: lidge-jun/opencodex
Length of output: 271
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Early Windows returns:"
grep -rn 'process\.platform === "win32"' tests || true
echo
echo "Other skipIf/only usage in tests:"
grep -rn '\.skipIf(\|\.only(' tests || true
echo
echo "Full test around issue:"
awk '{printf "%6d\t%s\n", NR, $0}' tests/codex-shim-readiness.test.ts | sed -n '84,145p'Repository: lidge-jun/opencodex
Length of output: 11163
Use test.skipIf for the Windows-only guard.
At tests/codex-shim-readiness.test.ts:87, if (process.platform === "win32") return; exits before the install command is invoked and before any assertions run. This leaves the Windows run reporting a passing test while skipping coverage of this Windows-incompatible setup. Use test.skipIf(process.platform === "win32")(...) so Bun records the test as skipped.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/codex-shim-readiness.test.ts` at line 87, Replace the early Windows
return in the readiness test with Bun’s test.skipIf(process.platform ===
"win32") wrapper around the test definition, preserving the existing install
command and assertions while recording Windows runs as skipped.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eac8143466
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (inputs.routingKind === "unknown") { | ||
| warnings.push( | ||
| inputs.externalProvider |
There was a problem hiding this comment.
When config.toml selects a provider through profile = "work" and [profiles.work] model_provider = "anthropic", currentExternalCodexModelProvider() correctly returns anthropic, but getCodexRoutingKind() ignores profile sections and returns native. Because this branch only uses externalProvider for an unknown route, codex-shim install still prints a green result even though Codex bypasses OpenCodex. Warn whenever externalProvider is non-null, or classify the effective profile route, and add a profile-based regression case.
Useful? React with 👍 / 👎.
| routingKind: getCodexRoutingKind(), | ||
| externalProvider: currentExternalCodexModelProvider(), |
There was a problem hiding this comment.
If the Codex config.toml exists but cannot be read—for example because of permissions or a transient Windows sharing error—getCodexRoutingKind() degrades to unknown, but this subsequent call rereads the same file without a catch and throws. The shim may already have been installed successfully, yet this new advisory check makes the command exit nonzero with an uncaught error. Catch this probe failure and report the unverifiable-routing warning instead.
Useful? React with 👍 / 👎.
| processProxyEnvPresent: PROXY_ENV_KEYS.some(key => Boolean(process.env[key]?.trim())), | ||
| configuredProxyResolved: Boolean(resolveEnvValue(config.proxy)?.trim()), |
There was a problem hiding this comment.
When config.proxy is an environment reference such as ${HTTP_PROXY}, running install from a shell where that variable is set makes configuredProxyResolved true and suppresses the warning. A later Codex launch from launchd, a desktop app, or another environment may not contain that variable, leaving config.proxy unresolved and outbound requests without the required proxy—the exact handoff risk this check is intended to expose. Distinguish a persisted literal proxy from a value that resolves only through the current process, or warn for environment-backed proxy configuration.
Useful? React with 👍 / 👎.
1 participant
Summary
Republishes @TyroneXie's #1169 on current
dev. Their branch was 335 commits behind, so this is a maintainer rebase; the commit carries aCo-authored-bytrailer and the change is theirs.ocx codex-shim installreported a clean green success even when the launcher was installed but Codex routing was not provably pointed at OpenCodex — an externalmodel_provider, a user-owned local or remote gateway, or routing that cannot be verified. Those cases now produce a warning instead of unqualified success.It also warns when outbound proxy variables exist only in the current process while
config.proxyis unset or unresolved, because Codex launchers and background services such as launchd may not inherit that environment. Proxy values are never printed.The change is advisory only: install still succeeds with the same exit code, and the shim still fail-open execs the real Codex launcher.
Supersedes #1169, which can be closed once this lands.
Verification
Rebased onto
f5147cbc8and re-verified after that rebase:bun run test— 9994 pass / 7 skip / 0 fail across 626 filesbun test tests/codex-shim-readiness.test.ts— 5 pass / 0 fail, including a real CLI install into isolatedCODEX_HOME/OPENCODEX_HOMEdirectories asserting no proxy URL or credential is printedbun run typecheck— cleanbun run privacy:scan— passeddiagnoseCodexShimstill exists atsrc/codex/shim.ts:1179, so the newsrc/cli/index.tscall site resolves against currentdevrather than the branch's original base.Checklist
privacy:scanis green.Summary by CodeRabbit
New Features
ocx doctor.Documentation