fix(scan): confine local diff inputs to the selected target - #630
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 |
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
There was a problem hiding this comment.
Found one P2 path-confinement race in the local-diff read path.
| @@ -723,14 +761,9 @@ def make_diff_rank_input(args: argparse.Namespace) -> None: | |||
| elif path.is_symlink(): | |||
| preview = "" | |||
There was a problem hiding this comment.
[P2] Bind the preview read to the checked parent. changed_path_parent_is_within_target() resolves and validates the parent, but this later preview_for(path, ...) re-resolves the path independently. A concurrent process can replace an in-repository parent with a symlink/junction after the check and before this read, causing an external same-name file to be included in rank input and potentially disclosed in scan prompts/artifacts. Open the parent/file through identity-bound handles (or revalidate the opened file against the checked parent) so confinement and reading are one operation; apply the same binding to the inventory path that reads contents.
There was a problem hiding this comment.
I reproduced the parent-swap/read race at 7c30deb8d9eb6464666ed1b2e57cbb5cd36c4ac7 and its base 3e8985ff89d85e1550bd76dfa4b61825a676835c: a controlled swap immediately before the real preview read lets an external synthetic marker reach the rank input on both. This is pre-existing, not a new regression from this diff; the static escaping-parent cases covered here do pass.
I'm holding approval pending a response to this existing concern. Please either bind the open/read to the checked parent, or clarify the intended confinement guarantee and whether concurrent parent replacement is an agreed follow-up. I found no additional blocker in the PR's own delta; its four focused tests and typecheck pass.
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.
Summary
Confine local diff discovery to the selected repository when a changed path has a symlinked or junction parent. Broken symlink leaves remain safe to inventory without dereferencing the requested leaf.
Changes
0.1.24to0.1.25so installed caches receive the corrected generators.Testing
bun test --timeout 30000 ./tests-ts/diff-rank-input.test.ts(4 passed, 0 failed)pnpm run typessdk/typescriptgit diff --checkpnpm run buildpnpm pack --pack-destination ../../distpnpm run check:package <packed-tarball>pnpm run test:packagepnpm run testwithumask 022, seed 472 (1,778 passed, 28 skipped, 0 failed)pnpm run testwithumask 022, seed 1268720731 (1,778 passed, 28 skipped, 0 failed)Risk and rollout
The behavioral change is limited to local working-tree diff inputs; committed revision reads are unchanged. Symlink leaves are still represented without previews, while an external resolved parent now fails closed before either generator reads it. The bundled-plugin version bump refreshes existing installations. This pull request is stacked on #626 and should merge after it.
Public disclosure review