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 18 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 repository target without rejecting valid broken symlink leaves.
Changes
dereferencing the changed leaf. Staged and untracked broken symlinks retain
their existing inventory and empty-preview behavior.
while rejecting parent links that escape the selected target.
digests, fallback traversal, and nonblocking progress estimates.
0.1.38to0.1.51, synchronize theSDK and plugin manifest versions, and preserve existing
CODEX_SAFETY_IDENTIFIERforwarding.Testing
Exact head
0cebe50bc4890cbd39c462909ddf5fe4a3445342:12345across eight focused diff, snapshot, runtime, Windowscompatibility, nested Git, patch, compact-diff, and recovery suites:
186 passed, 13 expected platform skips, zero failures, 1,309 assertions.
both inventory and ranking, while escaped parents remain rejected.
0.1.38to0.1.51verifiesdistinct installed roots, refreshed scripts, and safety identifier forwarding.
pnpm run types,pnpm run format,pnpm pack, andgit diff --check:passed.
255 archive entries, 111 bundled plugin files, the public import, strict
NodeNext types, CLI, bundled Codex, and nested worker.
Risk and rollout
Revision mode still reads committed blobs; safe deleted, missing, nested, and
alias-equivalent paths retain their existing behavior. Local working-tree
parents outside the selected target remain blocked. Snapshot recursion keeps
the originally selected target instead of trusting a nested checkout as a new
boundary. The bundled plugin version changes from
0.1.38to0.1.51;existing cached installs receive the updated payload and safety attribution.
This pull request remains a draft, does not publish a package, and has existing
conflicts with
mainthat require separate integration. Exact-headcross-platform CI and human approval remain required.
Public disclosure review
The branch, title, description, commits, changed files, comments, logs,
screenshots, attachments, and links were reviewed. Existing automated review
comments contain access-gated links, so the nonpublic-links attestation remains
unchecked.