feat(ai): add read-only OpenCodex provider - #3698
Conversation
![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: 8a3fdf7878
ℹ️ 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".
| catalog("Alibaba Token Plan", ["ALIBABA_TOKEN_PLAN_API_KEY"], { oauthProvider: "alibaba-token-plan" }), | ||
| ), | ||
| descriptor("openai", "gpt-5.4", config => openaiModelManagerOptions(config)), | ||
| descriptor("opencodex", "gpt-5.4", () => openCodexModelManagerOptions(), { allowUnauthenticated: true }), |
There was a problem hiding this comment.
allowUnauthenticated only causes the model manager to run; it does not add the provider to ModelRegistry.#keylessProviders. Consequently, even when the local proxy returns a valid catalog, getAvailable() rejects every OpenCodex model because there is neither a stored credential nor a keyless entry, and /login opencodex deliberately persists nothing. The newly discovered provider therefore cannot be selected or used from the coding-agent CLI.
Useful? React with 👍 / 👎.
| function normalizeModel(row: CatalogRow, endpoint: OpenCodexEndpoint): Model<"openai-responses"> | undefined { | ||
| const rawId = typeof row.id === "string" ? row.id.trim() : typeof row.model === "string" ? row.model.trim() : ""; | ||
| if (!rawId || rawId.includes("\n")) return undefined; | ||
| const publicId = `opencodex/${rawId}`; |
There was a problem hiding this comment.
Model selectors are constructed throughout the coding agent as ${model.provider}/${model.id}, so prefixing id here produces selectors such as opencodex/opencodex/provider/model. A documented or manually supplied selector like opencodex/provider/model is parsed as provider opencodex plus ID provider/model and cannot match the stored ID opencodex/provider/model; the descriptor's default gpt-5.4 also cannot match a discovered opencodex/gpt-5.4 ID. Preserve rawId as id and use wireModelId solely for the transport mapping.
Useful? React with 👍 / 👎.
| return ( | ||
| typeof health.version === "string" || | ||
| (typeof health.pid === "number" && Number.isFinite(health.pid)) || | ||
| (typeof health.port === "number" && Number.isFinite(health.port)) |
There was a problem hiding this comment.
When a stale runtime endpoint or port 10100 is occupied by another service, any generic response such as { "ok": true, "version": "1.0" } or { "ok": true, "pid": 42 } passes this check. If that service also exposes a compatible /api/models, it is registered as OpenCodex and subsequent prompts can be sent to the foreign listener. Validate an OpenCodex-specific discriminator rather than accepting generic version, PID, or port fields.
Useful? React with 👍 / 👎.
8a3fdf7 to
bc72a69
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc72a69c60
ℹ️ 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".
| async function readRuntimeEndpoint(): Promise<string | undefined> { | ||
| const home = process.env.OPENCODEX_HOME?.trim() || path.join(os.homedir(), ".opencodex"); | ||
| try { | ||
| const raw = JSON.parse(await fs.readFile(path.join(home, "runtime-port.json"), "utf8")) as RuntimePortFile; |
There was a problem hiding this comment.
readRuntimeEndpoint uses fs.readFile for file-content access, contrary to the repository contract requiring Bun.file() for reads and limiting node:fs/promises to directory operations. Replace this with await Bun.file(path.join(home, "runtime-port.json")).text() so the new provider follows the required Bun runtime conventions.
AGENTS.md reference: AGENTS.md:L114-L122
Useful? React with 👍 / 👎.
| id: "opencodex", | ||
| name: "OpenCodex (local proxy status)", | ||
| available: true, |
There was a problem hiding this comment.
Adding this entry to getOAuthProviders() makes the shipped pi-ai login opencodex command pass through the generic packages/ai/src/cli.ts login path, which unconditionally prints Credentials saved to ~/.gjc/agent/agent.db. The OpenCodex branch only performs a status probe and persists nothing—even when the proxy is unavailable—so this command now gives users a false credential-success confirmation; the package CLI needs status-only handling analogous to the coding-agent controller.
Useful? React with 👍 / 👎.
|
Signed: Yeachan-Heo / gajae-code AI-provider red-team VERDICT: REQUEST_CHANGES Exact head reviewed: Hostile findings:
Scope is otherwise bounded to OpenCodex provider discovery/status, Responses routing, and focused tests/changelog. No merge performed. Formal REQUEST_CHANGES review submission was rejected by GitHub because the authenticated owner cannot request changes on an own-authored PR; this signed comment is the attached terminal disposition. |
|
Signed: Yeachan-Heo / gajae-code AI-provider red-team VERDICT: REQUEST_CHANGES Exact head remains Latest Codex automated review was attached to this exact commit ( Provider/auth/contract review: the OpenCodex addition is scoped to read-only local discovery/status, Responses model routing, raw wire-model preservation, OAuth/status plumbing, focused tests, and changelog updates. It does not persist credentials. Blocking red-team finding remains: endpoint identity is not actually enforced. Merge safety is also invalidated by base drift: PR base is #3699 remains deferred and dirty; it was not reopened or reviewed. No merge performed. |
bc72a69 to
c1fd8b0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1fd8b0671
ℹ️ 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".
| export function openCodexModelManagerOptions(): ModelManagerOptions<"openai-responses"> { | ||
| return { | ||
| providerId: "opencodex", | ||
| cacheTtlMs: OPENCODEX_MODEL_CACHE_TTL_MS, |
There was a problem hiding this comment.
After a successful discovery, this generic model cache persists each model's identity-bound baseUrl; on subsequent starts a fresh authoritative cache skips fetchDynamicModels, while a failed refresh merges cached models back in. Therefore, if OpenCodex exits and another Responses-compatible listener later occupies the cached port, prompts can be routed to it without executing the strict /healthz identity check. Fresh evidence in model-manager.ts is that both the fresh-cache fast path and failed-fetch fallback retain these cached models, so this provider needs revalidation before cached routes are published or used.
Useful? React with 👍 / 👎.
| const models = rows | ||
| .map(row => normalizeModel(row, endpoint)) | ||
| .filter((model): model is Model<"openai-responses"> => model !== undefined); | ||
| return models.length > 0 ? models : null; |
There was a problem hiding this comment.
When /api/models validly returns an empty array—for example after the proxy loses all configured upstream models—this converts the successful response to null. ModelManager interprets null as a dynamic-fetch failure and merges the previous cached catalog back in, leaving removed models selectable even though the proxy explicitly reported none. Return the empty array so the authoritative cache and visible catalog are cleared.
Useful? React with 👍 / 👎.
|
VERDICT: REQUEST_CHANGES Exact head: Blocking security finding — discovery follows redirects outside the loopback boundary. Additional test hole: the new tests cover foreign hosts and identity/port mismatches, but not redirects, malformed catalog rows beyond newline ids, or the unauthenticated provider’s no-endpoint/static-model behavior. — |
Discover local OpenCodex endpoints without owning proxy credentials or lifecycle, expose cached models with raw wire ids, and make /login opencodex a read-only status probe. Tested: focused OpenCodex, registration, model, auth, and schema suites (29 tests) Not-tested: full workspace typecheck remains affected by existing sibling-worktree package links
Bun 1.3.14 models fetch.preconnect as part of the fetch function type. Keep the test spies installed in place and preserve that property on mocked implementations so the provider check compiles without changing runtime behavior. Lore-id: 3698\nConstraint: repair the affected AI/provider CI lane only\nConstraint: preserve read-only OpenCodex behavior\nRejected: weakening type checks | hides the CI failure\nConfidence: high\nScope-risk: narrow\nReversibility: easy\nTested: OpenCodex and stream focused tests; AI and coding-agent checks; affected-plan replay and evidence validation
Runtime metadata could select a foreign listener, and health responses were accepted without proving the OpenCodex identity or matching port. Restrict endpoint candidates to loopback IPs and require strict health identity and port binding before model discovery. Lore-id: 3698\nConstraint: keep discovery local and read-only\nConstraint: do not persist credentials\nRejected: accept arbitrary host or loose health payloads | permits foreign listener confusion\nConfidence: high\nScope-risk: narrow\nReversibility: easy\nTested: OpenCodex focused tests and packages/ai check
OpenCodex probes must not follow a loopback redirect into a foreign listener. Force redirect:error for health and catalog requests and cover redirect rejection while retaining loopback identity and strict port binding. Lore-id: 3698\nConstraint: keep OpenCodex discovery loopback-only\nConstraint: do not persist credentials\nRejected: follow redirects | permits SSRF and foreign catalog ingestion\nConfidence: high\nScope-risk: narrow\nReversibility: easy\nTested: redirect regression tests and packages/ai check
c1fd8b0 to
1b023bf
Compare
|
Signed: Yeachan-Heo / fresh OpenCodex successor red-team review VERDICT: MERGE_READY Reviewed exact head The redirect escape is repaired at the shared discovery boundary: Red-team checks:
Exact CI run — |
1 participant
Summary
/api/modelsmodels withopencodex/<id>selectors and raw wire IDs/login opencodexstatus reprobesVerification
bun test packages/ai/test/openai-opencodex-responses.test.ts packages/ai/test/register-builtins.test.ts packages/ai/test/models-lazy.test.ts packages/ai/test/auth-storage-check-credentials.test.ts packages/coding-agent/test/models-config-tool-choice-support.test.ts(29 passed)