fix(scan): confine local diff inputs to the selected target - #630
fix(scan): confine local diff inputs to the selected target#630mldangelo-oai wants to merge 12 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. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97cb4a2a80
ℹ️ 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".
jameshiester-oai
left a comment
jameshiester-oai
left a comment
There was a problem hiding this comment.
Found one P2 path-confinement race in the local-diff read path.
kmbroai
left a comment
kmbroai
left a comment
There was a problem hiding this comment.
Critical review
Reviewed 7c30deb8d9eb6464666ed1b2e57cbb5cd36c4ac7, against its declared #626 base. The local-diff boundary fix is necessary and this implementation handles leaf symlinks more correctly than the overlapping #472 change. No blocking correctness issue found in this delta.
Necessity and correctness
A repository-relative lexical path is not enough to establish where its parent directories resolve. The shared parent check is applied before local inventory or preview processing, without applying a live-filesystem rule to committed revision blobs. That is the right distinction.
Starting at path.parent also preserves the existing behavior for symlink leaves: the inventory can omit them and ranking can represent them without reading the destination. I tested a real staged broken symlink against both branches. This head succeeds in both generators; #472 aborts while strictly resolving the leaf. Keep that legitimate control alongside the external-parent rejection tests.
Simplification and overlap
This and #472 should share one local-diff implementation. Prefer this parent-only rule for the overlapping generator changes and retain #472's separate snapshot-recursion work only if it is still needed. Merging both helpers would create two subtly different definitions of the same boundary.
The package-cache regression is larger than the production change and duplicates a mock marketplace installer. Its predecessor is a copy of the new plugin with an older manifest version; it demonstrates restaging and installed-source equality, not execution of the genuinely older broken helper. Reuse the existing runtime-upgrade fixture rather than growing another installer harness in the ranking suite. The unrelated safety-identifier assertion belongs with environment/packaging tests. These are non-blocking simplification opportunities, not reasons to remove the real-Git boundary tests.
Keep the synchronized bundle version bump, and preserve the #626 dependency when landing. The later #631 bundle change should carry the combined payload rather than reuse a version for different contents.
Verification
Ran diff-rank-input.test.ts with seed 12345: 4 passed, 0 failed, Bun 1.3.14/Linux with locally available dependencies. Also ran the independent real-Git comparison described above. I did not rerun the full stack's package tests or native Windows junction controls.
Keep the confinement bundle distinct from its updated parent and verify the predecessor cache upgrade.
|
@codex review |
|
@codex security review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35329b6df9
ℹ️ 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".
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
faizan-oai
left a comment
faizan-oai
left a comment
There was a problem hiding this comment.
Requesting changes at 564150fd for the remaining Windows leaf-read regression, with evidence in the existing discussion. An ordinary selected file disappearing before its read aborts the local-patch scan instead of being skipped; current-head native Windows CI reproduces it. The Windows backend's exception class bypasses the new skip handlers in both generators.
The bound-read fix and POSIX skip handling are verified; please retain both when fixing Windows leaf-error handling. Review covers this PR's delta against #626, not unreviewed parent #623.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d2f5e53c5
ℹ️ 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 |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. You're on a roll. 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". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
faizan-oai
left a comment
faizan-oai
left a comment
There was a problem hiding this comment.
Reviewed 446ff8fdbe08bdcb9a341a25fe79c867f1ed9e03 against declared base 429ecd18a88866f8d16b736187066b19776789c9 (#626). The previously reported read/compatibility concerns are addressed: bound reads remain in place, ordinary verified-leaf failures are skipped, and parent/root/reparse failures remain rejected. No blocking concern remains in this delta.
Verification: 6 focused tests and 26 portable backend probes passed; the latter are not native sharing-contention tests. Full current-head node-ci 32812064205, attempt 2 now passes all 25 jobs, including native Windows. Head, base, CI, and discussions were rechecked before approval. Unchanged local suites were not rerun for this CI-only follow-up.
This supersedes my changes-requested review on 564150fd. Approval covers only this PR's delta; the #626-first dependency remains, and this does not approve parent #623.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f35a487dd
ℹ️ 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".
| sample = source.read(4096) | ||
| if is_binary_sample(sample): | ||
| return "", True | ||
| data = sample + source.read() |
There was a problem hiding this comment.
When an opened changed file is on NFS/FUSE or removable storage and either source.read() raises EIO or another non-permission OSError, the exception escapes to both callers, which misreport it as a confinement violation and abort the entire local-patch scan. Before this change, preview_for caught OSError around both opening and reading and caused the unavailable file to be skipped. The fresh evidence beyond the prior open-time fixes is a read-time failure after the descriptor has opened successfully; catch read/close errors as transient leaf failures without weakening parent validation.
AGENTS.md reference: AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed at 8f35a487 against declared base 3518b216. With a real regular-file open followed by fault-injected EIO at either read or stream close, ranking exits 1 and inventory exits 2 with the confinement error; the base exits 0 and skips the file in all six comparisons. This also existed at 446ff8f, so I missed it in my earlier review; the restack did not introduce it.
Please preserve the old leaf-read skip behavior with an OSError catch limited to the opened stream's read/close operation (generate_rank_input.py:396–401), keeping parent/descriptor validation failures fatal. These were synthetic fault-injection tests, not a real storage outage.
| elif _descriptor_relative_reads_available(): | ||
| root_descriptor = _open_verified_scan_directory(target) |
There was a problem hiding this comment.
If the selected repository directory itself is renamed and another directory is installed at the same path after changed_path_parent_is_within_target returns, _open_verified_scan_directory(target) records and validates only the replacement directory's identity, so the preview reads its file and sends unrelated contents to the model. I reproduced this on POSIX by swapping the repository root in the confinement callback; the generated row contained the replacement tree's external marker. The fresh evidence beyond the child-parent replacement coverage is that replacing the repository root, rather than src, bypasses both the static check and the new descriptor-relative reader; capture the root identity before enumerating Git changes and require that identity during every bound read, including the Windows backend.
AGENTS.md reference: sdk/typescript/AGENTS.md:L22-L23
Useful? React with 👍 / 👎.
faizan-oai
left a comment
faizan-oai
left a comment
There was a problem hiding this comment.
Requesting changes for the confirmed leaf read/close regression. Synthetic EIO after a successful open now aborts both local-patch generators instead of skipping the unavailable file. Keep parent/descriptor validation failures fatal while restoring the leaf-read skip behavior.
Reviewed 8f35a487 against declared base 3518b216. The separate root-replacement behavior also reproduces in that base, so it is not an introduced regression behind this request. Full current-head CI is also failing; that remains a separate reason to withhold approval.
4 participants
Summary
Keep local working-tree diff inputs inside the selected repository even when a changed file's parent is replaced after its initial confinement check.
Changes
0.1.44to0.1.45and retain cached-installation and safety-identifier forwarding coverage.Testing
bun test --timeout 30000 --seed 12345 tests-ts/diff-rank-input.test.ts(6 passed, 97 assertions; parent-replacement, Windows missing-leaf, and Windows sharing-violation regressions each failed before their respective fixes)bun test --timeout 30000 --seed 12345 --test-name-pattern 'upgrades the predecessor bundled cache and restores' tests-ts/runtime.test.ts(1 passed)./node_modules/.bin/tsc --noEmitnode scripts/generate-models.cjs --checkgit diff --checkRisk and rollout
Only local working-tree diff reads change. Committed revision blobs and deleted-path semantics remain unchanged; broken symlink leaves are still skipped by inventory and represented without previews by ranking. POSIX reads use descriptor-relative opens, while Windows uses the existing locked-handle backend. Ordinary missing, unreadable, or temporarily locked leaf files retain their previous skip behavior on both platforms; missing parents, replaced directories, reparse points, and escaped paths still fail closed. The bundle version advances beyond the parent so cached installations receive the updated generators. This pull request remains stacked on #626 and must not land before its parent.
Public disclosure review