fix: normalize Windows configuration paths - #492
Conversation
ebc3b06 to
1779fc9
Compare
1634968 to
9aa9b31
Compare
f806da9 to
6c18515
Compare
mldangelo-oai
left a comment
There was a problem hiding this comment.
Reviewed 6c185159fefe for correctness and simplification. One optional test simplification. The production changes preserve the intended configuration precedence.
This is a non-blocking review. I did not run the full SDK suite or native Windows tests locally.
mldangelo-oai
left a comment
There was a problem hiding this comment.
Rechecked 6c18515. The earlier simplification suggestions remain non-blocking. Approving the reviewed head.
mldangelo-oai
left a comment
There was a problem hiding this comment.
Re-reviewed 020c2b3, including the test-only update since the earlier approval. The previous review thread is addressed, and I found no new blocking issue.
CI is green on this head, including Windows, but the branch still conflicts with main in sdk/typescript/src/cli.ts. The resolution should retain both resolveCliPath(directory, input) and lstat(path, { bigint: true }). A fresh approval should be attached to the conflict-free replacement head after its checks pass.
Local validation: the diff whitespace check and focused Python preflight smoke checks passed. I did not run the full SDK suite or native Windows tests locally.
Summary
Severity: P1–P2. Windows path spelling, environment-key casing, home-relative values, and machine-wide configuration could cause valid Codex Security settings to be missed or resolved against the wrong location. This makes those inputs resolve consistently while preserving existing configuration precedence.
Changes
Testing
pnpm run format— passed.pnpm run types— passed.pnpm run build— passed.git diff --check origin/main...HEAD— passed.Risk and rollout
The changes are limited to configuration discovery and path normalization. Existing precedence remains unchanged, and non-Windows path behavior is preserved apart from explicit home expansion where the CLI already accepts a path. No migration or rollout step is required.
Public disclosure review