Skip to content

Fix misplaced single-line PR-review suggestion anchoring#8930

Open
JesperSchulz wants to merge 3 commits into
mainfrom
jesperschulz-fix-pr-review-suggestion-anchoring
Open

Fix misplaced single-line PR-review suggestion anchoring#8930
JesperSchulz wants to merge 3 commits into
mainfrom
jesperschulz-fix-pr-review-suggestion-anchoring

Conversation

@JesperSchulz

@JesperSchulz JesperSchulz commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Problem

The Copilot PR Review orchestration (tools/Code Review/scripts/Invoke-CopilotPRReview.ps1) can anchor a suggestion block to the wrong line, so applying the suggestion corrupts the file.

Seen on #8893: a style suggestion (prefix the call with this., CodeCop AA0248) was anchored to a comment line (2196) instead of the statement it edits (2198). Auto-applying would overwrite the comment and leave the real call unprefixed.

Root cause

Resolve-SuggestionPlacement's single-line branch re-anchored only by exact whitespace-insensitive equality between the suggested line and a file line within ±8 of the model anchor. But a single-line suggestion almost always edits a line, so it is by definition not equal to the line it replaces (this. was inserted). No match was ever found, and the code fell through to "trust the anchor" — returning the model's unreliable line number, which pointed at a nearby comment.

Fix

  • Re-anchor single-line suggestions by character-LCS similarity over a ±8 window. Pick the clear winner using an absolute confidence floor plus an ambiguity margin over the runner-up.
  • When no candidate is confident/unambiguous, return $null so the caller suppresses the suggestion (posts a manual snippet) instead of trusting a wrong anchor — i.e. never produce an auto-applicable misplaced suggestion.
  • Multi-line (additive-span) behaviour is unchanged.

Fixes AB#640948

@JesperSchulz JesperSchulz requested review from a team June 30, 2026 13:28
@github-actions github-actions Bot added this to the Version 29.0 milestone Jun 30, 2026
Comment thread tools/Code Review/tests/Resolve-SuggestionPlacement.Tests.ps1 Fixed
@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Copilot PR Review

Iteration 4 · Outcome: not-applicable

The diff contains no AL changes. The only changed file is a PowerShell script (tools/Code Review/scripts/Invoke-CopilotPRReview.ps1). All six sub-skills (al-performance-review, al-security-review, al-privacy-review, al-upgrade-review, al-style-review, al-ui-review) require technologies:[al] and returned not-applicable. The super-skill self-review pass also produced no findings as there is no AL code to analyse.

Knowledge source: https://github.com/microsoft/BCQuality@822cae1b2771ac25f665f73369f69093bd4fd630

No findings were posted for this iteration.

Orchestrator pre-filter (13 file(s) excluded)

  • layer-disabled (knowledge) : 13 file(s)

Findings produced by the Copilot CLI agent against BCQuality at 822cae1b2771ac25f665f73369f69093bd4fd630. Reply 👎 on any inline comment to flag false positives.

Resolve-SuggestionPlacement only re-anchored single-line suggestions by exact
whitespace-insensitive equality. A one-line suggestion that edits a line (e.g.
inserting `this.` for CodeCop AA0248) is never equal to the line it replaces, so
it fell through to "trust the model anchor" and posted on the wrong line — a
neighbouring comment in #8893. Applying it would corrupt the file.

Re-anchor by character-LCS similarity over a +/-8 window instead: pick the clear
best line (absolute floor + ambiguity margin), and return $null to suppress the
suggestion block (manual snippet) when no confident, unambiguous target exists.

Fixes AB#640948

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@JesperSchulz JesperSchulz force-pushed the jesperschulz-fix-pr-review-suggestion-anchoring branch from 800bb07 to 521c866 Compare June 30, 2026 13:50
The model-reported anchor for a single-line suggestion can be off by more
than 8 lines (e.g. PR #8933 VendorNL Confirm() was off by 10, label rename
off by 9). Widen the search window to 40 lines either side so the correct
target within the same procedure is considered, while the 0.5 similarity
floor and 0.1 ambiguity margin keep an unrelated look-alike from winning.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Widening the search window exposed a precision problem: a label-rename
suggestion whose added Comment text echoes the field captions at the
Error()/Confirm() call site would re-anchor onto the call site (PR #8933
GenJournalLineNL/GeneralLedgerSetupNL 'Text1000000/1' findings), which is
worse than the original mis-anchor.

Raise the similarity floor to 0.6 and use a bounded window (20). Genuine
edit targets - an edited statement or a renamed declaration - score
~0.75-0.99 and re-anchor confidently (the PR #8933 Confirm() findings and
the PartnerTypeMismatchMsg->Qst rename now land on the right line). Lower-
confidence look-alikes stay below the floor and are suppressed, so the
caller posts a manual snippet instead of a wrong auto-applicable anchor.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants