fix(windows): preserve PowerShell environment and activity paths - #634
Conversation
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. |
|
@mldangelo-oai — could you review this Windows environment and PowerShell activity fix? Final focused tests and native controls pass; the PR description records the unresolved broader-suite subprocess failure separately. Cross-platform CI is running. @codex review the current head |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
kmbroai
left a comment
There was a problem hiding this comment.
Critical review
Reviewed 237dc0005300afaeeee9d1a222aaba18136f7c1c. No blocking correctness issue found. The Codex-home fix is necessary; the activity-parser changes are useful but optional presentation work.
Necessity and scope
The home lookup must happen before deleting case aliases from the copied environment. On Windows a plain copied object does not provide the native environment object's case-insensitive lookup behavior, so reading only the uppercase key can discard the user's selected home. Reusing environmentValue on Windows and emitting one canonical child key is the right small fix; preserving POSIX case sensitivity is important.
PowerShell launcher unwrapping and repository-variable recognition improve activity display, but they do not make scans more complete or change coverage. If the parser needs further iteration, it should not delay landing the independent environment fix. Separate commits would make that distinction easier to retain; a larger redesign is unnecessary.
Correctness and simplification
The launcher match is intentionally limited to the emitted -Command form and verifies the executable basename. The path normalization excludes single-quoted variable literals and unrelated variables before reusing the existing traversal filter. That is preferable to trying to implement a general PowerShell parser for optional progress reporting.
Keep unsupported launch forms unchanged rather than guessing. Do not expand this patch into arbitrary PowerShell switches, encoded commands, evaluation of variables, or a dependency on executing PowerShell to discover paths. Similarly, keep inferred activity separate from authoritative coverage. The existing whitespace/token heuristics mean this is still a best-effort display parser, not an exact shell interpreter.
The test cases for literal quotes, escaped variable sigils, mixed-case environment names, and parent traversal are the valuable coverage here; no extra generic parser framework is warranted.
Verification
Ran scan-activity.test.ts and cli-skills.test.ts: 66 passed, 0 failed on Bun 1.3.14/Linux with locally available dependencies. Also ran 4/4 synthetic controls against the actual parser with its platform dependency set to Windows, covering ordinary/braced variables, a single-quoted literal, and traversal rejection. Those controls execute the Windows parser branch, but are not native PowerShell or Windows subprocess tests. I did not rerun the full platform matrix.
|
@codex review the current head This integrates main's canonical plugin source/build layout while retaining the original four-file PowerShell fix and newer CLI behavior. Local focused checks passed (70 tests, one existing Windows skip), as did native PowerShell controls and the full Node 24 installed-package inspection. Both Windows verification jobs have passed on this head; the remaining full CI jobs are still running. |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Summary
Preserve the selected Codex home when Windows environment objects retain a non-uppercase
CODEX_HOMEspelling, and report repository file activity from ordinary PowerShell commands.Changes
-Commandevent form while retaining the script's inner quotes.Testing
Current main integration
Validated the integration of main
9bd256aon Windows using Node24.19.0, Bun1.3.14, pnpm11.9.0, and npm10.9.2:build:plugin,check:plugin-source,types,format,build, andgit diff --check: passed. The 118-file generated plugin payload remains ignored, not tracked.bun test --timeout 30000 tests-ts/cli-skills.test.ts tests-ts/scan-activity.test.ts tests-ts/build-plugin.test.ts: 70 passed, 1 existing Windows skip, 0 failed, 374 assertions; reported seed678080754.patch/verify-fixentrypoint controls, and 16/16 read/literal activity controls passed. Two previously captured native SDK events replay correctly against the current compiled parser. Service/worker fixtures are synthetic; this is not a completed model-backed scan.pnpm packandnode scripts/check-package.mjs <exact-tgz>: passed. The one-argument check ran the full installed smoke, including public import, NodeNext types, CLI, credential locking, MCP initialization, bundled Codex, and a nested worker without global Codex. It validated 282 archive entries in 72.04 seconds. The supportednpm_execpathselects npm10.9.2, executed by Node24.19.0; no smoke stage or timeout was changed.The initial exact-head node-ci run passed all 28 jobs on
1e5f029, including both Windows package-inspection jobs. Codex code review reported no major issues on this head. At this description update, the additional automatic CI run and requested maintainer re-review remain to be verified. The patch relative to main is still the original four files.Earlier validation before this integration
12345: 64 passed, 1 existing Windows skip, 0 failed.git diff --check: passed.workbench-scan-root-alias.test.ts; the same isolated test passes on both unchanged main and this branch, so its cause remains unresolved. That full run predates the final launcher-parser addition; final-source full-suite CI is required.Risk and rollout
No new CLI flags, environment variables, or dependencies. The home-selection change is Windows-only. Activity changes affect displayed commands and file paths, not scan coverage. POSIX variable-name behavior and existing literal/traversal filtering remain covered. Absolute Windows root-case matching and authentication setup guidance are outside this patch.
Public disclosure review