-
Notifications
You must be signed in to change notification settings - Fork 681
fix(codex): warn when codex-shim install cannot prove routing #1169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,69 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| currentExternalCodexModelProvider, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| getCodexRoutingKind, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| type CodexRoutingKind, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } from "../codex/inject"; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import { loadConfig, resolveEnvValue } from "../config"; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| const PROXY_ENV_KEYS = [ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "HTTP_PROXY", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "HTTPS_PROXY", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "ALL_PROXY", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "http_proxy", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "https_proxy", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "all_proxy", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ] as const; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| export interface CodexShimReadinessInputs { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| routingKind: CodexRoutingKind; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| externalProvider: string | null; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| processProxyEnvPresent: boolean; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| configuredProxyResolved: boolean; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| function externalProviderLabel(provider: string | null): string { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return provider | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ? `external model_provider ${JSON.stringify(provider)}` | ||||||||||||||||||||||||||||||||||||||||||||||||||
| : "the active Codex route"; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| export function codexShimReadinessWarnings( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| inputs: CodexShimReadinessInputs, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ): string[] { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const warnings: string[] = []; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const provider = externalProviderLabel(inputs.externalProvider); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| if (inputs.routingKind === "unknown") { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| warnings.push( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| inputs.externalProvider | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ? `Codex still selects ${provider}. The shim can start OpenCodex, but it does not redirect that provider; point it at the live OpenCodex /v1 endpoint with wire_api = "responses", or switch to the built-in openai provider and run 'ocx sync'.` | ||||||||||||||||||||||||||||||||||||||||||||||||||
| : "Codex routing could not be verified. The shim can start OpenCodex, but it may not redirect Codex; run 'ocx doctor' before relying on autostart.", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } else if (inputs.routingKind === "custom-local") { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| warnings.push( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| `Codex uses ${provider} through a user-owned local gateway. The shim can start OpenCodex, but OpenCodex does not own that route; run 'ocx doctor' to verify its lifecycle.`, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } else if (inputs.routingKind === "custom-remote") { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| warnings.push( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| `Codex uses ${provider} through a remote gateway. The shim only starts a local OpenCodex proxy and will not affect those requests.`, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| if (inputs.processProxyEnvPresent && !inputs.configuredProxyResolved) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| warnings.push( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "Proxy environment variables are present only in this process while config.proxy is unset or unresolved. Codex launchers and background services may not inherit them; persist config.proxy before relying on autostart.", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| return warnings; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| export function collectCodexShimReadinessWarnings(): string[] { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const config = loadConfig(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
AGENTS.md reference: docs-site/AGENTS.md:L7-L10 Useful? React with 👍 / 👎. |
||||||||||||||||||||||||||||||||||||||||||||||||||
| return codexShimReadinessWarnings({ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| routingKind: getCodexRoutingKind(), | ||||||||||||||||||||||||||||||||||||||||||||||||||
| externalProvider: currentExternalCodexModelProvider(), | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If Useful? React with 👍 / 👎.
Comment on lines
+64
to
+65
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When installation runs inside, or globally trusts, a repository whose AGENTS.md reference: src/AGENTS.md:L24-L26 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When AGENTS.md reference: AGENTS.md:L228-L230 Useful? React with 👍 / 👎. |
||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+61
to
+68
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win Keep readiness collection advisory when Codex config reads fail. Line 64 fails closed to Catch this read failure and use 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,144 @@ | ||
| import { describe, expect, test } from "bun:test"; | ||
| import { spawnSync } from "node:child_process"; | ||
| import { | ||
| chmodSync, | ||
| mkdirSync, | ||
| mkdtempSync, | ||
| rmSync, | ||
| writeFileSync, | ||
| } from "node:fs"; | ||
| import { tmpdir } from "node:os"; | ||
| import { dirname, join } from "node:path"; | ||
| import { fileURLToPath } from "node:url"; | ||
| import { codexShimReadinessWarnings } from "../src/cli/codex-shim-readiness"; | ||
|
|
||
| const repoRoot = dirname(fileURLToPath(new URL("../package.json", import.meta.url))); | ||
| const cliPath = join(repoRoot, "src", "cli", "index.ts"); | ||
|
|
||
| const ready = { | ||
| routingKind: "native" as const, | ||
| externalProvider: null, | ||
| processProxyEnvPresent: false, | ||
| configuredProxyResolved: false, | ||
| }; | ||
|
|
||
| describe("Codex shim install readiness", () => { | ||
| test("keeps a clean install green for native and managed routing", () => { | ||
| expect(codexShimReadinessWarnings(ready)).toEqual([]); | ||
| expect(codexShimReadinessWarnings({ | ||
| ...ready, | ||
| routingKind: "opencodex-local", | ||
| })).toEqual([]); | ||
| }); | ||
|
|
||
| test("warns when an external provider is not routed through OpenCodex", () => { | ||
| const warnings = codexShimReadinessWarnings({ | ||
| ...ready, | ||
| routingKind: "unknown", | ||
| externalProvider: "custom", | ||
| }); | ||
|
|
||
| expect(warnings).toHaveLength(1); | ||
| expect(warnings[0]).toContain('external model_provider "custom"'); | ||
| expect(warnings[0]).toContain("live OpenCodex /v1 endpoint"); | ||
| expect(warnings[0]).toContain('wire_api = "responses"'); | ||
|
|
||
| }); | ||
|
|
||
| test("distinguishes user-owned local and remote routes", () => { | ||
| const local = codexShimReadinessWarnings({ | ||
| ...ready, | ||
| routingKind: "custom-local", | ||
| externalProvider: "gateway", | ||
| }); | ||
| expect(local).toHaveLength(1); | ||
| expect(local[0]).toContain("user-owned local gateway"); | ||
| expect(local[0]).toContain("ocx doctor"); | ||
|
|
||
| const remote = codexShimReadinessWarnings({ | ||
| ...ready, | ||
| routingKind: "custom-remote", | ||
| externalProvider: "gateway", | ||
| }); | ||
| expect(remote).toHaveLength(1); | ||
| expect(remote[0]).toContain("remote gateway"); | ||
| expect(remote[0]).toContain("will not affect those requests"); | ||
| }); | ||
|
|
||
| test("warns about process-only proxy settings without exposing a URL", () => { | ||
| const warnings = codexShimReadinessWarnings({ | ||
| ...ready, | ||
| processProxyEnvPresent: true, | ||
| configuredProxyResolved: false, | ||
| }); | ||
| expect(warnings).toHaveLength(1); | ||
| expect(warnings[0]).toContain("config.proxy"); | ||
| expect(warnings[0]).toContain("may not inherit"); | ||
| expect(warnings[0]).not.toContain("://"); | ||
|
|
||
| expect(codexShimReadinessWarnings({ | ||
| ...ready, | ||
| processProxyEnvPresent: true, | ||
| configuredProxyResolved: true, | ||
| })).toEqual([]); | ||
| }); | ||
|
|
||
| test("the install command surfaces readiness warnings without leaking the proxy URL", () => { | ||
| if (process.platform === "win32") return; | ||
|
|
||
| const root = mkdtempSync(join(tmpdir(), "ocx-shim-readiness-")); | ||
| const codexHome = join(root, "codex-home"); | ||
| const opencodexHome = join(root, "opencodex-home"); | ||
| const binDir = join(root, "bin"); | ||
| mkdirSync(codexHome); | ||
| mkdirSync(opencodexHome); | ||
| mkdirSync(binDir); | ||
| try { | ||
| writeFileSync(join(codexHome, "config.toml"), [ | ||
| 'model_provider = "custom"', | ||
| "", | ||
| "[model_providers.custom]", | ||
| 'name = "OpenAI"', | ||
| 'wire_api = "responses"', | ||
| "", | ||
| ].join("\n"), "utf8"); | ||
| writeFileSync(join(opencodexHome, "config.json"), `${JSON.stringify({ | ||
| port: 10100, | ||
| providers: { | ||
| openai: { | ||
| adapter: "openai-responses", | ||
| baseUrl: "https://chatgpt.com/backend-api/codex", | ||
| authMode: "forward", | ||
| }, | ||
| }, | ||
| defaultProvider: "openai", | ||
| }, null, 2)}\n`, "utf8"); | ||
| const codex = join(binDir, "codex"); | ||
| writeFileSync(codex, "#!/bin/sh\nexit 0\n", "utf8"); | ||
| chmodSync(codex, 0o755); | ||
|
|
||
| const proxyUrl = "http://user:secret@127.0.0.1:7890"; | ||
| const result = spawnSync(process.execPath, [cliPath, "codex-shim", "install"], { | ||
| cwd: repoRoot, | ||
| env: { | ||
| ...process.env, | ||
| CODEX_HOME: codexHome, | ||
| OPENCODEX_HOME: opencodexHome, | ||
| PATH: `${binDir}:${process.env.PATH ?? ""}`, | ||
| HTTP_PROXY: proxyUrl, | ||
| HTTPS_PROXY: proxyUrl, | ||
| }, | ||
| encoding: "utf8", | ||
| }); | ||
|
|
||
| expect(result.status).toBe(0); | ||
| expect(result.stdout).toStartWith("⚠️ Codex autostart shim installed"); | ||
| expect(result.stderr).toContain('external model_provider "custom"'); | ||
| expect(result.stderr).toContain("config.proxy"); | ||
| expect(`${result.stdout}\n${result.stderr}`).not.toContain(proxyUrl); | ||
| expect(`${result.stdout}\n${result.stderr}`).not.toContain("user:secret"); | ||
| } finally { | ||
| rmSync(root, { recursive: true, force: true }); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
config.tomlselects an external provider throughprofile = "work"and[profiles.work].model_provider,currentExternalCodexModelProvider()returns that provider, butgetCodexRoutingKind()only classifies root routing and can returnnativeoropencodex-local; this function consequently returns no warning. The shim's subsequentensurealso preserves that external route, so installation appears green even though Codex requests bypass OpenCodex. CheckexternalProviderindependently 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 👍 / 👎.