fix(workbench): confine inventories and snapshots to the selected target - #472
fix(workbench): confine inventories and snapshots to the selected target#472mldangelo-oai wants to merge 21 commits into
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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 Please review the current head, |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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 Please review commit |
|
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: Didn't find any major issues. What shall we delve into next? 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". |
mldangelo-oai
changed the title
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: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
![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: 9249639ad6
ℹ️ 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".
kmbroai
left a comment
kmbroai
left a comment
There was a problem hiding this comment.
Critical review
Reviewed 9249639ad66448521317150ff2507f1868b516d1. The confinement invariant is necessary, but the current leaf-resolution rule rejects a valid local diff. Consolidate the overlapping diff work with #630 rather than landing two competing implementations.
Correctness
[P2] Do not dereference a broken symlink leaf that will not be read. In existing_ancestor_is_within_target, lstat() succeeds for a broken symlink, then resolve(strict=True) raises. Both new callers turn that into a fatal inventory/ranking error before reaching their existing leaf-symlink handling. The earlier inline finding remains applicable.
I created a real temporary Git repository, committed a regular file, then staged broken.py -> missing.py within the repository. At this head, the actual inventory command exits 2 and ranking exits 1, both with a changed-path inspection error and neither producing output. The same fixture against #630 exits 0 for both: inventory omits the link and ranking records it with an empty preview. This is a compatibility regression, not a request to follow an out-of-target link.
Use the containing-directory/nearest-existing-parent check for local diff paths, while leaving the leaf's existing no-dereference behavior intact. Preserve rejection when the parent resolves outside the selected target.
Necessity and simplification
Snapshot recursion still needs to retain the original selected target rather than silently promote a nested checkout to a new authorized root. The _selected_target propagation addresses that distinct issue, and optional progress counting should remain nonblocking.
However, #630 changes the same inventory and ranking call sites with a narrower helper and already covers the broken-leaf case. Prefer one shared local-diff rule, then keep this PR's remaining snapshot-specific protection separately reviewable. Do not maintain both existing_ancestor_is_within_target and changed_path_parent_is_within_target with subtly different leaf semantics.
The large canonical-path test combines many independent scenarios and patches Git/Python helpers repeatedly. Split its meaningful cases into focused tests sharing the existing fixture; an early failure should not prevent later alias, nested-root, and progress checks from running. That is non-blocking cleanup after the correctness fix.
Verification
The supplied diff/canonical-path suites pass 8 tests, 1 skip, 0 failures on Bun 1.3.14/Linux. The additional real-Git broken-link control fails here and passes on #630 as described. No native Windows/macOS validation or full package run is claimed.
|
Codex Review: Didn't find any major issues. Nice work! 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. |
2 participants
Summary
Keep changed-file inventories, ranking previews and Git snapshots inside the selected target without rejecting valid broken symlink leaves.
Changes
Testing
Eight affected and adjacent suites: 195 passed, 14 platform-dependent skips.
Real Codex upgrades from 0.1.60 and 0.1.62 passed, followed by real Git inventory/ranking checks against the installed scripts.
Types, formatting, build, Ruff and Python 3.10 syntax passed.
Rebuilt npm package validation and full installed-package smoke passed.
Full suite and native Windows were not rerun. Current-head CI is left for a separate pass.
Final main refresh (
fd98a90): package 0.1.21 includes the MCP launcher-permission fix; feature source and bundled payload are unchanged. Types/model generation, formatting, build, 28 focused package/report/launcher tests, static artifact verification and full installed-package smoke passed, including MCP initialization. CI was not awaited.Risk and rollout
Revision mode continues to read committed blobs. Safe missing, deleted, nested and alias-equivalent paths retain their behavior; escaped working-tree parents remain blocked. Progress estimates skip unsafe paths without weakening snapshot validation. No new public options or dependencies. This PR remains a draft.
Public disclosure review
Existing commit contact metadata and account-specific automated review links prevent the second attestation. This update uses synthetic fixtures and a GitHub noreply commit identity; historical metadata and other authors' comments are unchanged.