fix(codex): warn when codex-shim install cannot prove routing - #1169
fix(codex): warn when codex-shim install cannot prove routing#1169TyroneXie wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthrough
ChangesCodex shim readiness
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant User
participant codexShimInstall
participant diagnoseCodexShim
participant ReadinessCollector
participant CLIOutput
User->>codexShimInstall: run install
codexShimInstall->>diagnoseCodexShim: verify installed shim
codexShimInstall->>ReadinessCollector: collect routing and proxy warnings
ReadinessCollector-->>codexShimInstall: return warnings
codexShimInstall->>CLIOutput: print success or warning status
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Review readiness checklist
0/4 boxes ticked. This PR stays in draft until every box above is ticked. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/codex-shim-readiness.ts`:
- Around line 61-68: Update collectCodexShimReadinessWarnings to catch failures
from currentExternalCodexModelProvider and pass externalProvider: null so
readiness collection remains advisory when the Codex config is unreadable or
removed. Add a regression test covering that read failure and verify the
unresolved-routing warning is returned instead of throwing.
🪄 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: c0a63ab5-1167-442f-a412-dc3bf2a7eeda
📒 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
| export function collectCodexShimReadinessWarnings(): string[] { | ||
| const config = loadConfig(); | ||
| return codexShimReadinessWarnings({ | ||
| routingKind: getCodexRoutingKind(), | ||
| externalProvider: currentExternalCodexModelProvider(), | ||
| processProxyEnvPresent: PROXY_ENV_KEYS.some(key => Boolean(process.env[key]?.trim())), | ||
| configuredProxyResolved: Boolean(resolveEnvValue(config.proxy)?.trim()), | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep readiness collection advisory when Codex config reads fail.
Line 64 fails closed to "unknown" when Codex config cannot be read. Line 65 then calls currentExternalCodexModelProvider(), whose upstream implementation reads the same file without a catch. If the file is unreadable or changes between the existence check and read, codex-shim install throws instead of reporting the unresolved-routing warning.
Catch this read failure and use externalProvider: null. Add a regression test for an unreadable or removed Codex config.
Proposed fix
export function collectCodexShimReadinessWarnings(): string[] {
const config = loadConfig();
+ const routingKind = getCodexRoutingKind();
+ let externalProvider: string | null = null;
+ try {
+ externalProvider = currentExternalCodexModelProvider();
+ } catch {
+ // Routing is already classified as unknown when config cannot be read.
+ }
+
return codexShimReadinessWarnings({
- routingKind: getCodexRoutingKind(),
- externalProvider: currentExternalCodexModelProvider(),
+ routingKind,
+ externalProvider,
processProxyEnvPresent: PROXY_ENV_KEYS.some(key => Boolean(process.env[key]?.trim())),
configuredProxyResolved: Boolean(resolveEnvValue(config.proxy)?.trim()),
});
}📝 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.
| export function collectCodexShimReadinessWarnings(): string[] { | |
| const config = loadConfig(); | |
| return codexShimReadinessWarnings({ | |
| routingKind: getCodexRoutingKind(), | |
| externalProvider: currentExternalCodexModelProvider(), | |
| processProxyEnvPresent: PROXY_ENV_KEYS.some(key => Boolean(process.env[key]?.trim())), | |
| configuredProxyResolved: Boolean(resolveEnvValue(config.proxy)?.trim()), | |
| }); | |
| export function collectCodexShimReadinessWarnings(): string[] { | |
| const config = loadConfig(); | |
| const routingKind = getCodexRoutingKind(); | |
| let externalProvider: string | null = null; | |
| try { | |
| externalProvider = currentExternalCodexModelProvider(); | |
| } catch { | |
| // Routing is already classified as unknown when config cannot be read. | |
| } | |
| return codexShimReadinessWarnings({ | |
| routingKind, | |
| externalProvider, | |
| processProxyEnvPresent: PROXY_ENV_KEYS.some(key => Boolean(process.env[key]?.trim())), | |
| configuredProxyResolved: Boolean(resolveEnvValue(config.proxy)?.trim()), | |
| }); |
🤖 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/codex-shim-readiness.ts` around lines 61 - 68, Update
collectCodexShimReadinessWarnings to catch failures from
currentExternalCodexModelProvider and pass externalProvider: null so readiness
collection remains advisory when the Codex config is unreadable or removed. Add
a regression test covering that read failure and verify the unresolved-routing
warning is returned instead of throwing.
![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: d8968b7e6e
ℹ️ 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".
| const warnings: string[] = []; | ||
| const provider = externalProviderLabel(inputs.externalProvider); | ||
|
|
||
| if (inputs.routingKind === "unknown") { |
There was a problem hiding this comment.
When config.toml selects an external provider through profile = "work" and [profiles.work].model_provider, currentExternalCodexModelProvider() returns that provider, but getCodexRoutingKind() only classifies root routing and can return native or opencodex-local; this function consequently returns no warning. The shim's subsequent ensure also preserves that external route, so installation appears green even though Codex requests bypass OpenCodex. Check externalProvider independently before accepting a routing kind as ready, and add a focused profile-selector regression test.
AGENTS.md reference: AGENTS.md:L228-L230
Useful? React with 👍 / 👎.
| const config = loadConfig(); | ||
| return codexShimReadinessWarnings({ | ||
| routingKind: getCodexRoutingKind(), | ||
| externalProvider: currentExternalCodexModelProvider(), |
There was a problem hiding this comment.
If config.toml exists but cannot be read, is replaced between the existence check and read, or is a directory, currentExternalCodexModelProvider() throws because unlike getCodexRoutingKind() it does not catch readFileSync failures. This happens after installCodexShim() has already modified the launcher, so an advisory probe turns a successful installation into an uncaught non-zero CLI failure without printing the install result. Treat probe errors as unverifiable routing and emit the warning instead of allowing them to escape.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| export function collectCodexShimReadinessWarnings(): string[] { | ||
| const config = loadConfig(); |
There was a problem hiding this comment.
loadConfig() is a mutating loader: it hardens directory and secret permissions and, when an invalid configuration cannot be repaired, writes a timestamped .invalid-* backup. Consequently, every fresh ocx codex-shim install process can create another copy of an invalid configuration—including any credentials it contains—even though the newly added lifecycle documentation says these readiness checks are read-only. Use the existing observe-only readConfigDiagnostics() path to inspect config.proxy without altering the user's configuration state.
AGENTS.md reference: docs-site/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
| routingKind: getCodexRoutingKind(), | ||
| externalProvider: currentExternalCodexModelProvider(), | ||
| 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 ${HTTPS_PROXY}, this check treats it as persistent merely because it resolves in the installer process. If Codex is later launched from a desktop session or another shell that does not inherit that variable, the shim-started proxy loads the same reference as unresolved and loses outbound connectivity, yet installation was reported green; generated background-service environments likewise do not preserve arbitrary proxy variables. Distinguish a literal persisted proxy from an environment reference whose value exists only now, and add focused coverage for this handoff case.
AGENTS.md reference: AGENTS.md:L228-L230
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8968b7e6e
ℹ️ 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".
| routingKind: getCodexRoutingKind(), | ||
| externalProvider: currentExternalCodexModelProvider(), |
There was a problem hiding this comment.
When installation runs inside, or globally trusts, a repository whose .codex/config.toml selects an external provider, these probes inspect only the global CODEX_CONFIG_PATH, so a native or OpenCodex-managed global route produces no warning even though Codex's merged project configuration bypasses the proxy. The repository already models this concrete override in collectProjectCodexConfigWarnings() and analyzeProjectCodexConfig(); include those diagnostics in readiness and add a focused project-config regression test before printing green.
AGENTS.md reference: src/AGENTS.md:L24-L26
Useful? React with 👍 / 👎.
|
@TyroneXie Republished this on current A green "installed" message when routing can't actually be proven is the kind of success report that costs someone an afternoon, so this is worth landing. I re-verified on the rebased head rather than trusting the old run: full suite 9994 pass / 0 fail, your 5 readiness tests pass, I'll close this once #1289 lands. Nothing needed from you. |
…#1289) `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>
|
Landed on Closing this as superseded rather than stale. Thanks for catching it; a green success message that can't actually prove routing is exactly the kind of thing that wastes someone's afternoon. |
2 participants
Summary
ocx codex-shim installnow reports a warning instead of a clean green success when the launcher is installed but Codex routing is not provably pointed at OpenCodex: an externalmodel_provider, a user-owned local/remote gateway, or unverifiable routing.config.proxyis unset or unresolved, because Codex launchers and background services (for example launchd) may not inherit that environment. Proxy values are never printed.Verification
bun test tests/codex-shim-readiness.test.ts— 5 pass, including a real CLI install in isolatedCODEX_HOME/OPENCODEX_HOMEdirectories asserting that no proxy URL or credential is printed.bun run typecheck— pass.git diff --check— pass.EADDRINUSEon port-0 test listeners andEPERMon test temp directories) prevented a clean full-suite run in this environment. Focused tests and typecheck are green.Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit