fix(windows): preserve scoped inventories after case-only renames - #633
fix(windows): preserve scoped inventories after case-only renames#633faizan-oai wants to merge 3 commits into
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 scoped-inventory fix? Native controls cover ordinary case-renamed Git checkouts and genuinely case-sensitive directories. Cross-platform CI is running. @codex review the current head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0caf7db1fc
ℹ️ 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".
|
@codex review the current head This follow-up addresses the scoped-query performance comment, uses filesystem directory identity for the macOS case-alias failure, and removes the unrelated README advice. Final focused Windows checks pass; exact-head cross-platform CI is running. |
|
Codex Review: Didn't find any major issues. Breezy! 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". |
|
@codex security review the current head The replacement node-ci workflow passed all 25 jobs on this commit, including both new macOS regressions. The older canceled workflow is being rerun to clear its stale title-gate failures; no source change was needed for those gates. |
kmbroai
left a comment
There was a problem hiding this comment.
Critical review
Reviewed cf49da44cbf0afdb762d177147a5675626d6195e. This fixes a real inventory-correctness problem. I found no blocking correctness issue in the revised diff.
Necessity and correctness
A case-only directory rename can leave Git's index spelling different from the physical directory spelling without making the checkout dirty. Treating a failed case-sensitive pathspec match as an empty scope can then lose source files and mislead both counts and component planning. That is worth fixing in the inventory path rather than asking users to change Git configuration.
The revision addresses the earlier whole-repository-enumeration concern: ordinary scopes use an icase,literal pathspec, followed by filesystem-identity filtering. The string-keyed prefix cache is deliberate: blindly using case-folded Windows path equality would collapse distinct directories on a genuinely case-sensitive volume. Keeping leaf inspection as lstat() also avoids following symlink files just to normalize the inventory.
The TypeScript change reuses normalizeTarget instead of inventing another canonicalization algorithm. Passing absolute candidates preserves literal ~ names, and insertion-ordered Set deduplication removes the previous repeated linear search while preserving order. These are useful simplifications, not unrelated features.
Remaining tradeoffs
The Unicode fallback still enumerates the repository for a scope containing case-varying non-ASCII characters. That is a documented performance tradeoff, not an unresolved version of the ordinary-scope bug. Keep the fallback narrow; do not simplify this into unconditional root enumeration. If Unicode-heavy, many-component repositories become a real workload, measure that case before adding another cache or filesystem-capability probe.
The added tests have meaningful behavioral coverage: real Git operations, candidate counts rather than wall-clock performance assertions, tracked/untracked files, ignored-but-tracked files, and literal-path handling. I would keep the filesystem-identity comparisons; replacing them with lowercased strings would weaken the case-sensitive-volume coverage.
Verification
Ran the three focused suites with seed 12345: component-scan.test.ts, targets.test.ts, and workbench-windows-compatibility.test.ts: 50 passed, 2 skipped, 0 failed on Bun 1.3.14/Linux with locally available dependencies. An initial sandbox run failed on remapped temporary-directory ownership; the ordinary-environment rerun passed. The skips and this Linux run are not native NTFS/macOS evidence; I did not rerun those platform controls or the full CI matrix.
|
Current-head verification: Codex code and security reviews reported no findings for |
|
@codex review the current head This carries the five-file scoped-inventory fix into the current canonical plugin layout. The Python implementation is unchanged from the previous reviewed fix and matches the packed helper bytes. Focused inventory/API checks and the full Node 24 installed-package smoke passed. Three Python fixture failures also reproduced on unmodified main; their separate test-only correction is in #684. |
|
Codex Review: Didn't find any major issues. Keep it up! 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". |
|
Verification follow-up for The previously failing lightweight-tag case explicitly passed in the refreshed Windows Node24 log (seed |
Summary
Keep scoped file inventories, file counts, and automatic component planning consistent after case-only directory renames on Windows.
A normal Git branch switch can leave the physical directory spelling different from the index when ignored build output keeps the directory alive. Git reports a clean working tree, but a case-sensitive pathspec can return an empty inventory for that existing scope.
Changes
icase,literalpathspec, then filter candidates using cached filesystem-directory identity checks. Preserve symbolic-link file entries without following them.samefilerather than path-string equality so case-insensitive macOS aliases and genuinely case-sensitive Windows directories both work.~filenames are not treated as home-directory syntax.plugins/codex-security/scripts/workbench_target.py, the current canonical source; leave the generated runtime payload untracked.Testing
Current main integration
Validated the integration of main
9bd256aon Windows with Node24.19.0, Bun1.3.14, pnpm11.9.0, Python3.12.13, and pytest9.0.3:build:plugin,check:plugin-source,types,format,build, andgit diff --checkpassed.bun test --timeout 30000 tests-ts/targets.test.ts tests-ts/component-scan.test.ts tests-ts/build-plugin.test.ts: 54 passed, 2 POSIX-only skips, 0 failed, 245 assertions; reported seed3164661725.1670890019.test_workbench_target.py,test_generate_rank_input.py,test_generate_in_scope_files.py,test_windows_scope_streams.py, andtest_windows_scan_local_files.py): 97 passed, 2 skipped, 3 failed. An unmodified exact-main snapshot reproduced all three failures: two code-page cases compare Git slash strings with native Windows separators, and one tilde fixture sets onlyHOME. These are two baseline test-fixture defects, not attributed to this fix; their test-only correction is kept separate in #684.pnpm packpassed. The single packedworkbench_target.pyis byte-for-byte identical to the canonical source, including the scoped-query and directory-identity fix. All 118 generated plugin files remain ignored, not tracked.node scripts/check-package.mjs <exact-tgz>with one archive argument passed the full installed smoke: public import, NodeNext types, CLI, credential locking, MCP initialization, bundled Codex, and a nested worker without global Codex. It validated 282 archive entries in 70.61 seconds. The supportednpm_execpathselects npm10.9.2, executed by Node24.19.0.ETARGETfor@openai/codex@0.149.1are retained separately. The same archive passed with a fresh task-local cache; the registry, assertions, smoke stages, and timeouts were unchanged. This does not establish the original cache failure's cause or explain unrelated CI failures.The initial exact-head node-ci run completed with 25 successful jobs and three failures. The Windows Node24 shard7 job failed one unchanged release-script fixture: authoritative retargeted lightweight-tag revalidation expected subprocess status
1, receivednull, after 11.04 ms (seed3096685149). The other two failures are downstream Windows summary gates. Both package-inspection jobs passed.The release-test file is identical to main and the passing companion integration; the identical case explicitly passed in that Windows CI run. The five authoritative-tag cases also passed locally on this head with the failing seed (5 passed, 274 filtered, 0 failed). That isolated result does not reproduce the null status, establish its cause, or validate the entire failed shard. No source, timeout, or policy workaround was added. The testing-description update triggers a fresh automatic CI run, which still needs verification.
Codex code review reported no major issues on
b0b6a8c; maintainer re-review is requested. The main-relative fix remains five files, with the Python implementation moved intact into canonical source.Earlier validation on the pre-integration head
bun test --timeout 30000 tests-ts/component-scan.test.ts tests-ts/targets.test.ts tests-ts/workbench-windows-compatibility.test.ts --seed 12345: 50 passed, 2 platform-specific skips, 0 failed.bun test --timeout 120000 tests-ts/api.test.ts --test-name-pattern 'keeps requested source paths|passes the workbench snapshot contract|removes scoped target files' --seed 12345: 3 passed, 130 filtered, 0 failed.pnpm run types,pnpm run format,pnpm run build, andgit diff --check: passed.~/app.tspassed on default NTFS. A separate case-sensitive Windows-directory control preserved both differently cased source files and scopes.Risk and rollout
Ordinary scoped requests no longer enumerate the whole repository. Case-varying Unicode scopes still use a conservative root-listing fallback, so that less-common path retains the larger-listing cost. Directory identity checks are cached, and normalization deduplication is linear. Missing tracked files remain excluded, and existing target-confinement checks remain in use.
Relative to main, no CLI/API options, stored artifacts, database schema, Git configuration, or runtime dependencies change. The unrelated README setup advice was removed after review.
Public disclosure review