fix(workbench): bind finding excerpts to selected source paths - #488
fix(workbench): bind finding excerpts to selected source paths#488mldangelo-oai wants to merge 24 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.
|
@codex review Please review current head |
|
@codex review Please review current head |
|
Codex Review: Didn't find any major issues. Delightful! 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 the current head |
|
Codex Review: Didn't find any major issues. Keep it up! 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 head Python setup and migration tests: 65 passed, including both preview upgrades and the existing comparison-index migration cases. Focused Bun finding-preview tests: 24 passed. Plugin bundle generation, all three portable source checks with Ruff 0.16.1, and git diff --check passed. The full suite is left to CI. |
|
Codex Review: Didn't find any major issues. Hooray! 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 head Python setup and migration tests: 66 passed, including all three preview upgrades and existing comparison-index migration cases. The focused finding-preview migration regression passed after rebuilding the plugin bundle; the earlier 24-test finding-preview run is recorded below. All three portable plugin source checks with Ruff 0.16.1 and git diff --check passed. The full suite is left to CI. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 565639dfdb
ℹ️ 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".
| excerpt_locations.append( | ||
| { | ||
| "endLine": row["end_line"], | ||
| "path": row["relative_path"], |
There was a problem hiding this comment.
When a valid repository-relative path exceeds FINDING_LOCATION_PATH_BYTES, location.path is truncated for the response while excerpt_locations retains the full database value, so sourceExcerpt is read from a file that the reported relative location no longer identifies; two deep paths with the same first 2,048 bytes can even display identically while showing different source. Preserve the prior alignment by omitting the excerpt when bounding changes the path, or perform the lookup using the exact path returned to the caller.
AGENTS.md reference: AGENTS.md:L30-L33
Useful? React with 👍 / 👎.
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
Save nullable source-scope metadata with additive migration 42 and share it across scan-start and preview paths. Preserve published migrations 1-41 and upgrade earlier previews recorded as 34, 40, or 41.
Read selected immutable tree/blob objects without fetching missing data or substituting replacement refs. Preserve unambiguous historical scopes and omit working-tree diff excerpts.
Fix the reviewed descendant-alias ambiguity: colliding Git names now require the existing filesystem witness, even when the finding uses an exact Git spelling.
Replace three excerpt-selection passes and their result cache with one stable priority-ordered pass. Shorten the user documentation; retain root-control preference and safe fallback locations.
Integrate main
70d9eceand adopt its paired plugin markers at 0.1.79. Move the source-scope helper to canonical plugin source so it is included in generated bundles.Give the new helper the standard plugin-script help entrypoint required by the existing source checks; source-scope selection is unchanged.
Use the existing asynchronous Bun subprocess pattern for the isolated-timeout regression after Windows repeatedly returned an immediate timeout from the synchronous call. Preserve the 30-second outer bound and the asserted 100-millisecond inner timeout.
Testing
Merge conflict resolution at
565639dfdb06506c0d3de77c2380ed8d330bcb5aagainstmainat6750642c94edce8d60112f816631de05dc10efd6:Earlier validation recorded for prior heads:
Merge conflict resolution at
11e3b9323af6ed9372c413b86e755abb852d99eaagainstmainat3c44b607dc05639a0e307a50d6dad1f822aeac00:Earlier validation recorded for prior heads:
Windows CI follow-up:
66a858fc858b3b4ce253d7139a37dfd701e39c20.Risk and rollout
Migration 42 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.