fix(workbench): bind finding excerpts to selected source paths - #488
fix(workbench): bind finding excerpts to selected source paths#488mldangelo-oai wants to merge 19 commits into
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. 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 |
|
Codex Review: Didn't find any major issues. 🚀 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 |
|
Codex Review: Didn't find any major issues. Chef's kiss. 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. Breezy! 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". |
|
@codex security review Please review the current head |
|
Codex Review: Didn't find any major issues. Breezy! 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". |
|
@codex review Please review exact head |
|
@codex security review Please review exact head |
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. |
![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: cad99e0640
ℹ️ 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".
| if scope["kind"] == "file": | ||
| return scope["objectId"] if not suffix else None | ||
| relative = PurePosixPath(*suffix).as_posix() | ||
| entry = exact_tree_path(repository, scope["objectId"], relative) |
There was a problem hiding this comment.
On a case-insensitive checkout, when a selected directory contains colliding Git entries such as foo.py and FOO.py, this exact lookup trusts whichever spelling the finding reports before attempting the filesystem-backed ambiguity check. Because the working tree can expose only one of those entries, the spelling does not establish which blob was scanned, so sourceExcerpt can display the sibling blob instead; route normalized collisions through the witness check or omit the excerpt.
AGENTS.md reference: sdk/typescript/AGENTS.md:L27-L27
Useful? React with 👍 / 👎.
kmbroai
left a comment
kmbroai
left a comment
There was a problem hiding this comment.
Critical review
Reviewed head cad99e064009a02ce28acb452fdc28bc021da31b.
Recommendation: fix the remaining descendant-alias ambiguity before merging. Binding optional excerpts to scan-time Git objects and selected paths is necessary; showing a plausible excerpt from the wrong blob is worse than omitting the excerpt.
[P2] Exact descendant spelling bypasses the ambiguity check
The existing comment at source_object_for_path still applies. For a selected directory containing case-colliding Git entries, the first exact_tree_path call does not require unambiguous normalized aliases. If the finding supplies either exact Git spelling, it returns that blob immediately; the filesystem-backed witness is consulted only when the exact lookup fails.
On a case-insensitive checkout, two Git entries such as foo.py and FOO.py can refer to one materialized pathname, so spelling alone does not identify the bytes that were scanned. I created a real Git tree containing both blobs and injected an unresolved filesystem witness. The current function returned the uppercase blob and never invoked the witness. This is a control-path reproduction, not a native case-insensitive filesystem run.
Route normalized collisions through the witness check, or omit the excerpt when identity cannot be established. Do not blanket-reject these repositories or suppress otherwise unambiguous findings. Add a descendant-collision case; the selected-root alias tests do not exercise this shortcut.
Necessity and simplification
Recording scope kind and object identity at scan start is preferable to guessing from today's checkout. Keeping working-tree excerpts unavailable, disabling lazy fetch/replacement substitution for historical reads, and leaving the finding usable when optional excerpt binding fails are appropriate boundaries. The append-only migration and hiding legacy internal recipe fields also keep the public launch recipe clean.
Keep one scope/path authority implementation shared by preview consumers. Avoid widening this optional display feature into a new source-access policy framework, and coordinate its bundled version with the other open plugin changes when assembling a release.
Verification
Ran finding-preview, publication-store, and report-limit suites: 39 passed, 0 failed, plus the targeted Git-tree/witness probe. Linux with Bun 1.3.14 / Node 22.13.1 and cached dependencies; no native Windows/macOS or live model scan was run.
2 participants
Summary
Bind optional finding excerpts to the Git objects and paths selected when a scan starts. Omit excerpts when the scanned source cannot be established, without removing the finding.
Changes
Testing
Native case-insensitive, real-Git descendant collision regression: failed before the fix and passed afterward. Covers file/directory collisions, saved/legacy records and missing witnesses; includes distinct-entry expectations for case-sensitive filesystems.
Focused preview, publication, report and runtime suites: 183 passed, 12 platform-dependent skips.
Types, generated models, formatting, build, Ruff and Python 3.10 syntax passed. Migrations 1–32 are unchanged.
Rebuilt npm package validation and full installed-package smoke passed.
Full suite, seed 488: 1,933 passed, 31 skipped, one sandbox
psdenial. That exact process-group test passed outside the sandbox. Native Windows and current-head CI are 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 tests, static artifact verification and full installed-package smoke passed, including MCP initialization. CI was not awaited.Risk and rollout
Migration 34 does not rewrite existing scans. Ambiguous excerpts are omitted; unambiguous sources, findings and public launch recipes remain available. The plugin version refreshes cached payloads without publishing a package. This PR remains a draft. The existing review finding is addressed in code; its thread has not been resolved or replaced with a new review.
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.