Repository navigation
fix: preserve scan directory identities and isolate Git fixtures - #1833
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. |
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for the cleanup. This owner-level merge-safety review at 147daa6 found no material issue. All 19 settings are local to disposable Git fixtures and occur before the relevant Git operations; explicit conversion coverage and production Git behavior remain intact.
With inherited core.autocrlf=true, 35 focused cases passed and two platform cases skipped. Six failures reproduced on the exact base: existing fixture ownership/Git-boundary limitations and an unchanged sparse-checkout expectation under conversion. The changed fixture cohort emitted no line-ending warnings. Types, changed-file formatting and the plugin build passed.
Native Windows behavior and the full SDK suite were not exercised locally.
…/codex/pr-refresh-1833
…dangelo/codex/repair-pr1833-file-identity
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for the fix. This owner-level merge-safety review at 587c86f found no material issue. File, parent and scan-root identities stay exact through comparisons; permission checks, the Windows device fallback and public results retain their behavior. All 19 Git settings stay local to disposable fixtures.
Types, formatting and SDK/plugin builds passed, along with 10 import-boundary and 10 inherited-CRLF fixture cases. The directory-ID collision regression passes on head and fails with the exact base implementation.
The broader contract/custom-validation run had 12 passes and 121 failures at the local environment’s existing output-ancestry boundary; two MCP fixture cases hit that boundary too. Both new file-identity cases are blocked there on base and head. Full Windows SDK CI is still running; it is not represented as passed.
# Conflicts: # sdk/typescript/src/contract.ts
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for the fix. This owner-level merge-safety review of ad34fa4 found no material issues across all 15 changed files. Root and parent identities stay exact through comparisons; permission checks and the Windows device fallback retain their behavior. All 19 Git settings are local to disposable fixtures.
Fresh SDK/plugin builds, SDK/MCP types, formatting and 12 inherited-CRLF fixture cases passed. The root-collision regression passes on head; an exact-base control gets past the rounded identity check and reaches the later ownership guard.
The contract/custom-validation run had 12 passes and 121 failures at the existing local output-ancestry ownership boundary, including both added file-collision cases. No full local SDK or native Windows run is claimed. Hosted CI is green on a merge with the exact reviewed parents and an identical source tree.
Summary
Checked scan reads now retain exact file and directory identities so distinct large filesystem identifiers cannot compare equal after numeric rounding. Synthetic Git repositories also keep their LF line-ending setting local, avoiding inherited conversion warnings in Windows tests.
Changes
core.autocrlf=falseat 19 disposable fixture initialization sites across 14 SDK test files. Explicit line-ending conversion tests retain their own overrides.mainwhile retaining the fixture changes.Testing
Merged
mainatfe9d77e1c3d9into6c8c23e99103. This refresh changes only the Dockerfile's two Node image references. Every other tracked file is byte-identical toad34fa4dbc35.git diff --checkpassed; no runtime tests were rerun for this container-only merge. The source checks below remain attributed to their recorded pre-refresh heads; fresh hosted CI must qualify the new head.Merged
mainat5f521759e7c6intoad34fa4dbc35. Fresh Linux checks on that earlier head passed:build:ci, SDK/MCP types, full SDK formatting, andgit diff --check.Validation used Node 22.23.3 and Bun 1.3.14. Dependency lockfiles and unchanged plugin sources were matched before reusing dependency installations and private copies of existing bundled/native artifacts. Native binaries were not rebuilt. Full SDK and cross-platform suites were not rerun; fresh hosted CI was required for that earlier head.
The exact bigint file, parent, and root identity implementation is unchanged by this merge. Earlier replacement regressions remain historical; neither the seeded nor deferred unseeded full suite is claimed complete. The historical Windows failure did not record filesystem identifiers, so its precise cause remains unasserted.
Risk and rollout
The identity change preserves existing scan-file protections without changing serialized scan data or public CLI/SDK surface. Permission checks still receive the same mode and owner values, and the Windows device fallback remains intact. Fixture-local Git settings affect only disposable test repositories.
Public disclosure review