feat(codex): pull an authenticated remote catalog into local Codex state - #4481
Conversation
`ocx status` on a connected client read the connection snapshot and the current `service-api-token` independently, so a reconnect or a key rotation between the two reads could send the new token to the snapshot's hub, or the snapshot's hub the new connection's token. The window is real: `collectRemoteHubStatus` awaits a dynamic import before it reads credentials. It now rereads the persisted connection and passes a token only when that connection still matches the snapshot's `serverUrl`, `apiKeyId` and `connectedAt` AND the token fingerprint matches that connection's `tokenFingerprint`. Otherwise it skips the live request and falls back to the snapshot owner's cache, or reports `unavailable`. A withheld token now carries its own cause. `resolveHubState` reported every null token as "this client has no usable data-plane token", which is false for a client that reconnected and holds a perfectly good token for a different hub - the operator would go re-enroll a credential that is not the problem. The caller supplies the reason through the new `withheldTokenReason`, so a changed connection, a missing token file and a fingerprint mismatch are named separately. That folds the maintainer review finding on #4382; it is the same misdiagnosis class as #4169 in the stop path. Verification: bun test tests/cli/cli-status-hub-state.test.ts (20 pass), tests/clients/client-hub-state.test.ts + tests/server/v1-hub-state.test.ts (35 pass), bun run typecheck, bun run structure:check, bun run privacy:scan, and the docs-site build required by docs-site/AGENTS.md (441 pages) - the one actionable CodeRabbit finding on the source pull request. Carried from #4382 by @luvs01. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
Adds `ocx catalog pull <https-url> [--auth-env <NAME>] [--json] [--restart-codex]`, which installs a complete catalog served by another OpenCodex instance's `/v1/catalog` endpoint and then synchronizes `models_cache.json`. `ocx sync` builds a catalog from locally configured providers and `ocx sync-cache` rebuilds the cache from the catalog already on disk; neither consumes a finished catalog from another server, which is the gap #3729 has held open. Acquisition is fail-closed before any local write: HTTPS is required except on loopback, URL-embedded credentials, queries, fragments and redirects are refused, the body is bounded by size and inactivity, and slugs and input modalities are validated. The token is read only by environment-variable name and never from argv. Catalog and cache writes share the Codex catalog write lock and the atomic writer. Three review findings on the source pull request are folded in. A cache rebuild that failed AFTER the catalog write left a new catalog paired with a stale `models_cache.json` while reporting `catalogWritten: false`. The permit rolls back SQLite; `replaceActiveCodexCatalog` is an atomic file write that nothing else undid. The pull now restores the previous catalog bytes, or removes the file when the home had none, before it throws. Both cases have a regression test, and both fail without the restore. `--restart-codex` reported success when only some app-servers stopped, so `ok: true` and exit 0 could be returned while a stale app-server still served the previous catalog from memory. A restart now counts only when nothing failed, nothing survived, and every listed process stopped. An incomplete restart returns `code: "restart_incomplete"` and exit 1 while keeping `catalogWritten` and `cacheSynced` true, because the writes did land. The unused `statSync` import is removed. Documentation now covers the full `--json` envelope, every failure code and its exit status, the host-root `/v1/catalog` path contract, and the two deliberate Phase 1 omissions: no `ETag`/`If-None-Match` conditional request, and no Windows `--restart-desktop-app`. Identical bytes stay a complete no-op, so a home whose cache alone is broken is repaired by `ocx sync-cache` rather than by this command. The seven localized lifecycle pages carry the command too. Closes #3729 Verification: bun test on catalog-remote-pull, skill-ocx, both test-layout guards, and the CLI help/dispatch/registry suites (137 pass); bun run typecheck, structure:check, privacy:scan, skill:surface:check; docs-site build (441 pages). Carried from #4413 by @rrmlima. Co-authored-by: rrmlima <137737127+rrmlima@users.noreply.github.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. |
리뷰 · 우선순위 69 / 80설명 추가되는 명령은 우선순위 69는 멀티 인스턴스 운영 통증을 직접 줄이고, 보안 경계(비밀을 argv에 안 둠·쓰기 전 검증)가 분명하기 때문이다. types/config 분할과 무관하다.
심볼 #4413 / #3729 - 캐리 머지 시 둘 다 닫히는 관계. ETag·Desktop 재시작은 범위 밖(본문도 명시). 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
![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: 394b96dee1
ℹ️ 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".
| // Re-check under K: another writer may have installed these bytes while the request was in flight. | ||
| const lockedCurrent = existsSync(catalogPath) ? readFileSync(catalogPath) : null; | ||
| if (lockedCurrent?.equals(candidate)) return { catalogWritten: false, cacheSynced: false }; | ||
| replaceActiveCodexCatalog(permit, codexHome, { path: catalogPath, content: fetched.content }); |
There was a problem hiding this comment.
When the remote catalog advertises a reasoning effort unsupported by the selected local Codex CLI (for example, max on an older client), this path writes it without checking runtime compatibility, causing Codex to exit while parsing the catalog before its first request. The existing connect and refresh paths explicitly call assertClientCatalogCompatible before writing (src/client/connect.ts:549-554 and 671-674) to prevent this exact failure; apply the same pre-write gate here and return a typed failure.
Useful? React with 👍 / 👎.
| } | ||
| if (slugs.has(slug)) invalid("Remote catalog contains duplicate model slugs"); | ||
| slugs.add(slug); | ||
| if (Object.hasOwn(model, "input_modalities")) { |
There was a problem hiding this comment.
When /v1/catalog contains a row without input_modalities, this conditional skips validation and the pull reports success even though Codex's strict parser requires that field (src/codex/catalog/effort.ts:181-183). This can occur because the serving path's readCatalog accepts any object with a models array (src/codex/catalog/parsing.ts:287-298), including custom or older catalogs; validate all required strict fields before replacing the local last-known-good catalog.
Useful? React with 👍 / 👎.
| resetCodexAppServerCatalogStateCache(); | ||
| } | ||
|
|
||
| export async function pullRemoteCatalog(input: string, options: PullRemoteCatalogOptions = {}): Promise<PullRemoteCatalogResult> { |
There was a problem hiding this comment.
This adds a new catalog acquisition and coordinated-write transaction under both src/cli/ and src/codex/, but the commit updates none of the structure documents mapped to those areas in structure/INDEX.md:100-103. Document the remote pull's ownership, validation, locking, rollback, and CLI lifecycle contracts in every mapped structure document so the architecture source of truth does not omit this writer.
AGENTS.md reference: src/AGENTS.md:L10-L11
Useful? React with 👍 / 👎.
| | `code` | Meaning | Exit | | ||
| | --- | --- | --- | | ||
| | `usage` | The arguments were not a valid `catalog pull` invocation | 2 | | ||
| | `auth_env_missing` | `--auth-env` named a variable that is not set | 1 | |
There was a problem hiding this comment.
When the named environment variable is set to an empty, oversized, or control-character-containing token, validateToken emits credential_invalid with exit 1, but the documented JSON failure table omits that code. Scripts using the advertised stable envelope therefore cannot enumerate this real outcome; add the missing row and its exit status.
AGENTS.md reference: docs-site/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
1 participant
Summary
Carries #4413 by @rrmlima. Closes #3729.
Adds
ocx catalog pull <https-url> [--auth-env <NAME>] [--json] [--restart-codex], which installs a complete catalog served by another OpenCodex instance's/v1/catalogendpoint and then synchronizesmodels_cache.json.ocx syncbuilds a catalog from locally configured providers andocx sync-cacherebuilds the cache from the catalog already on disk; neither consumes a finished catalog from another server, so operators were reimplementing authentication, atomic replacement, cache rebuild and process handling per deployment.Acquisition is fail-closed before any local write. HTTPS is required except on loopback, URL-embedded credentials, queries, fragments and redirects are refused, the body is bounded by size and inactivity, and slugs and input modalities are validated. The token is read only by environment-variable name, never from argv. Catalog and cache writes share the Codex catalog write lock and the atomic writer.
Review findings folded in
A cache rebuild that failed after the catalog write left a new catalog paired with a stale
models_cache.json, while the caller was toldcatalogWritten: false. The write permit rolls back SQLite;replaceActiveCodexCatalogis an atomic file write that nothing else undid. The pull now restores the previous catalog bytes — or removes the file when the home had none — before throwingwrite_failed. Both cases have a regression test, and I confirmed both fail without the restore.--restart-codexreported success when only some app-servers stopped.afterCatalogWriteHandleAppServersreturnsfailedandsurvivingwithout throwing, sostopped.length > 0producedok: trueand exit 0 while a stale app-server was still serving the previous catalog from memory. A restart now counts only when nothing failed, nothing survived, and every listed process stopped:The envelope keeps
catalogWrittenandcacheSyncedtrue there, because those writes did land; reporting the pull as having written nothing would be a second false report.The unused
statSyncimport is removed.Documentation now covers the full
--jsonenvelope, every failure code with its exit status, the host-root/v1/catalogpath contract, and the two deliberate Phase 1 omissions: noETag/If-None-Matchconditional request and no Windows--restart-desktop-app. Identical bytes remain a complete no-op, so a home whose catalog is correct but whose cache is broken is repaired byocx sync-cacherather than by this command — that limit is now written down instead of implied. The seven localized lifecycle pages carry the command.This is the tip of lane L in
devlog/_plan/260913_contributor_carry_train/, stacked on #4476. Its hosted run is the suite proof for both links.Verification
bun test tests/codex-integration/catalog-remote-pull.test.ts— 30 pass, including the two new rollback regressionsbun test tests/ci-workflows/skill-ocx.test.ts tests/test-layout.test.ts tests/test-layout-tooling.test.ts— 33 passbun test tests/cli/cli-help.test.ts tests/cli/cli-dispatch.test.ts tests/cli/cli-registry.test.ts— 74 passbun run typecheck,bun run structure:check,bun run privacy:scan,bun run skill:surface+skill:surface:check— all cleancd docs-site && bun run build— 441 pages, required bydocs-site/AGENTS.mdThe layout entries in
scripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.jsoncame over with the carry and are verified by the two layout guards above. Local full suite was not run, per the lane's instruction.Checklist