Repository navigation
Conversation
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
|
✅ Deterministic PR hygiene checks passed. |
There was a problem hiding this comment.
🔇 Additional comments: 11
src/update/codex-cli-update-launch-policy.mjs: 1
19-27: LGTM!src/update/codex-cli-update-launch-policy.d.mts: 1
19-20: LGTM!bin/ocx.mjs: 1
62-63: LGTM!Also applies to: 1213-1213, 1217-1217, 1230-1231
src/cli/launcher-context.ts: 1
28-28: LGTM!Also applies to: 54-62, 109-111, 123-123, 129-129, 144-144
tests/cli/cli-codex-cli-update-boundary.test.ts: 1
3-3: LGTM!Also applies to: 5-6, 11-11, 58-190
tests/cli/ocx-launcher-source.test.ts: 1
146-147: LGTM!src/cli/codex-cli-update.ts: 1
224-224: LGTM!src/codex/cli-update-plan.ts: 1
223-224: LGTM!Also applies to: 258-261, 271-271, 276-276, 308-308, 361-361, 500-501
structure/codex-cli-update.md: 1
11-19: LGTM!tests/codex-integration/codex-cli-update-plan.test.ts: 1
477-477: LGTM!Also applies to: 479-499, 618-619
src/codex/cli-update-plan.ts-266-268: 1
266-268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Import the transport key allowlist instead of copying it into
boundKeys.
CODEX_CLI_TRANSPORT_ENV_KEYSinsrc/update/codex-cli-update-launch-policy.mjs(Lines 19-21) already defines the allowlist. The launcher uses it to capture and to strip inherited variables.parseTransportEnvinsrc/cli/launcher-context.tsuses it to validate. This hunk adds a fourth hand-written copy of the same five names.The copies can drift. If a key is added to the shared list, the launcher captures it and the parser accepts it.
codexCliUpdateBoundEnvthen neither removes the ambient alias nor copies the bound value. The npm child then runs with a different transport environment than the captured snapshot. No test or gate detects this.Derive the set from the shared constant.
♻️ Proposed fix
- const boundKeys = new Set([ - "path", "pathext", "http_proxy", "https_proxy", "all_proxy", "no_proxy", "node_extra_ca_certs", - ]); + const boundKeys = new Set<string>(["path", "pathext", ...CODEX_CLI_TRANSPORT_ENV_KEYS]);Add this import near the top of the file. It is outside the selected range.
import { CODEX_CLI_TRANSPORT_ENV_KEYS } from "../update/codex-cli-update-launch-policy.mjs";
src/cli/launcher-context.tsalready imports the same.mjsmodule from TypeScript, andcodex-cli-update-launch-policy.d.mtstypes it. Confirm thatbun scripts/import-cycles.tsstill reports no new cycle forsrc/codex/cli-update-plan.ts.As per coding guidelines: "Do not duplicate provider facts across independent pickers or seeds." The same single-source principle applies to this shared allowlist.
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
0ddc4d2d-45ab-4069-a214-04a4fe4f1e7c
📒 Files selected for processing: 10
bin/ocx.mjssrc/cli/codex-cli-update.tssrc/cli/launcher-context.tssrc/codex/cli-update-plan.tssrc/update/codex-cli-update-launch-policy.d.mtssrc/update/codex-cli-update-launch-policy.mjsstructure/codex-cli-update.mdtests/cli/cli-codex-cli-update-boundary.test.tstests/cli/ocx-launcher-source.test.tstests/codex-integration/codex-cli-update-plan.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.
|
Carried into #7546 together with #7540, with your authorship kept on the original commits and a |
Summary
Bind Codex CLI update planning to transport settings captured by the Node launcher before Bun loads project dotenv. Registry evidence no longer takes proxy or CA settings from the later ambient environment, while legitimate operator settings remain supported through the actual CLI path.
The launcher carries proxy/CA entries in its existing proof-bound snapshot, removes duplicate child environment entries, validates and freezes the parsed map, and forwards it to npm inspection. Casing, aliases, empty values and absence retain their meaning. Older snapshots do not fall back to ambient settings.
Verification
60b1be204964d044790e1f5429df384898f89b07.Checklist
Review readiness
devcommit.Summary by CodeRabbit