Skip to content

Commit a9ec332

Browse files
JesperSchulzJesper Schulz-WeddeCopilot
authored
Anchor Copilot PR-review suggestion blocks to the correct line(s) (#8806)
## Problem On [#8772](#8772) two ```suggestion``` blocks were mis-applied, as flagged in review comments: - **`QltyFileImport` (line 68):** the comment anchored `procedure GetDialogTitle(): Text` but the fix `exit(ImportFromLbl);` was meant for line 70. Applying it would overwrite the procedure signature. - **`QltyImportLogEntry` (line 34):** the comment anchored `{` but the suggested code was the whole 4-line field. Applying it replaced `{` with the entire block, duplicating the field instead of just inserting the `DataClassification` line. Root cause: the orchestrator always posted a **single-line** comment at the model's reported `location.line`, but a GitHub suggestion replaces *exactly* the anchored line(s). When the model anchors a different/narrower line than the fix targets, the suggestion corrupts the file. ## Fix `tools/Code Review/scripts/Invoke-CopilotPRReview.ps1` now validates and re-anchors each suggestion against the PR-head file content before posting: - **Single-line fix** → snapped onto the line it actually rewrites (whitespace-insensitive match within a small window; trusts the model's anchor when no unique match exists). - **Multi-line fix** that edits a construct → posted as a **multi-line comment** over the full span it replaces, so inserted lines land in place. - **Unplaceable fix** → the applicable block is dropped and the change is shown as a manual, non-applicable snippet. New helpers: `Get-PrHeadFileLines`, `ConvertTo-LooseLine`, `Test-OrderedSubsequence`, `Resolve-SuggestionPlacement`; `New-ReviewComment` gained `start_line`/`start_side` support; `Build-CommentBody` gained `-SuppressSuggestion`. ## Validation Unit-tested the resolver against both real cases (snaps to 70..70; expands to 33..36) plus edge cases (genuine single-line rewrite trusts anchor, ambiguous match trusts anchor, unplaceable → drop, empty file → drop). Full script parses cleanly. Fixes: [AB#640364](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/640364) Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 9026a4a commit a9ec332

2 files changed

Lines changed: 208 additions & 5 deletions

File tree

tools/Code Review/README.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,16 @@ Every inline finding includes:
9393
`agent_domain`, `agent_finding`) used by the dedup logic on
9494
subsequent iterations.
9595

96+
When a finding carries a concrete fix, the orchestrator renders it as a
97+
GitHub ```suggestion``` block. Because such a block replaces *exactly* the
98+
line(s) its comment is anchored to, the suggested code is matched against the
99+
PR-head file to re-derive the correct RIGHT-side span: a single-line fix is
100+
snapped onto the line it actually rewrites, and a fix that edits a multi-line
101+
construct is posted as a multi-line comment over the whole span (so an
102+
inserted property lands in place instead of duplicating the surrounding
103+
lines). If the fix cannot be anchored with confidence the applicable block is
104+
dropped and the change is shown as a manual, non-applicable snippet.
105+
96106
## Per-PR summary comment
97107

98108
Marker: `<!-- copilot-pr-review-summary -->`. Upserted once per

tools/Code Review/scripts/Invoke-CopilotPRReview.ps1

Lines changed: 198 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -316,13 +316,23 @@ function Get-ReviewComments { return Get-AllPages "/pulls/$PrNumber/comments" }
316316
function Get-IssueComments { return Get-AllPages "/issues/$PrNumber/comments" }
317317

318318
function New-ReviewComment {
319-
param([string] $Body, [string] $Path, [int] $Line, [string] $Side)
319+
param(
320+
[string] $Body, [string] $Path, [int] $Line, [string] $Side,
321+
[int] $StartLine = 0, [string] $StartSide = ''
322+
)
320323

321324
if (-not $Line -or -not $Side) {
322325
throw 'Inline review comments require both line and side.'
323326
}
324327

325328
$payload = @{ body = $Body; commit_id = $PrHeadSha; path = $Path; line = $Line; side = $Side }
329+
# Multi-line comment: GitHub anchors the range over [start_line, line] so a
330+
# ```suggestion``` block replaces every spanned line in place (a single-line
331+
# comment would otherwise replace just $Line, duplicating context).
332+
if ($StartLine -gt 0 -and $StartLine -lt $Line) {
333+
$payload.start_line = $StartLine
334+
$payload.start_side = if ($StartSide) { $StartSide } else { $Side }
335+
}
326336
Invoke-GitHubApi -Method POST -Endpoint "/pulls/$PrNumber/comments" -Body $payload
327337
}
328338

@@ -416,6 +426,139 @@ function Build-LineMap {
416426
return $map
417427
}
418428

429+
# ---------------------------------------------------------------------------
430+
# Suggestion placement (anchor validation for ```suggestion``` blocks)
431+
#
432+
# A GitHub suggestion block replaces *exactly* the line(s) its comment is
433+
# anchored to. The model reports a single semantic `location.line` for a
434+
# finding, which often is not the line (or full span) the suggested code is
435+
# meant to replace — e.g. it anchors a procedure declaration while the fix
436+
# rewrites a statement two lines below, or anchors one line of a multi-line
437+
# field while the suggestion is the whole field plus an inserted property.
438+
# Posting such a suggestion verbatim corrupts the file when applied. These
439+
# helpers re-derive the correct RIGHT-side span by matching the suggested
440+
# code against the actual PR-head file content, so the block lands in place.
441+
# ---------------------------------------------------------------------------
442+
443+
# Cache of PR-head file contents (relative path -> string[] lines) so a file is
444+
# read at most once across all of its findings.
445+
$script:PrHeadFileCache = @{}
446+
447+
function Get-PrHeadFileLines {
448+
param([string] $RelativePath)
449+
450+
if ($script:PrHeadFileCache.ContainsKey($RelativePath)) {
451+
return $script:PrHeadFileCache[$RelativePath]
452+
}
453+
454+
$lines = $null
455+
$full = Join-Path $AnalysisWorkspace $RelativePath
456+
if (Test-Path -LiteralPath $full) {
457+
try {
458+
$lines = @(Get-Content -LiteralPath $full -ErrorAction Stop)
459+
} catch {
460+
Write-Warning "Could not read PR-head file for suggestion placement: $RelativePath ($_)"
461+
$lines = $null
462+
}
463+
}
464+
465+
$script:PrHeadFileCache[$RelativePath] = $lines
466+
return $lines
467+
}
468+
469+
# Whitespace-insensitive comparison key. Indentation and inter-token spacing
470+
# frequently differ between the suggested fix and the original line (the fix is
471+
# often *about* whitespace, e.g. 'exit (X)' -> 'exit(X)'), so boundary/context
472+
# matching collapses all whitespace to find the line a fix corresponds to.
473+
function ConvertTo-LooseLine {
474+
param([string] $Line)
475+
if ($null -eq $Line) { return '' }
476+
return ($Line -replace '\s+', '')
477+
}
478+
479+
# True when every line of $FileSpan appears, in order, somewhere in
480+
# $Suggestion (loose comparison). This holds when the suggestion is the file
481+
# span with extra lines inserted (and/or boundary-preserving edits) — the only
482+
# shape we can safely apply as an in-place multi-line replacement.
483+
function Test-OrderedSubsequence {
484+
param([string[]] $FileSpan, [string[]] $Suggestion)
485+
486+
$sug = @($Suggestion | ForEach-Object { ConvertTo-LooseLine $_ })
487+
$j = 0
488+
foreach ($f in $FileSpan) {
489+
$fl = ConvertTo-LooseLine $f
490+
$found = $false
491+
while ($j -lt $sug.Count) {
492+
$cur = $sug[$j]; $j++
493+
if ($cur -eq $fl) { $found = $true; break }
494+
}
495+
if (-not $found) { return $false }
496+
}
497+
return $true
498+
}
499+
500+
# Resolve the RIGHT-side file span a suggestion should replace.
501+
# Returns @{ startLine; endLine } (1-based, inclusive) or $null when the
502+
# suggestion cannot be placed with confidence (caller drops the block).
503+
function Resolve-SuggestionPlacement {
504+
param([string[]] $FileLines, [int] $AnchorLine, [string[]] $SuggestedLines)
505+
506+
if (-not $FileLines -or $FileLines.Count -eq 0) { return $null }
507+
if (-not $SuggestedLines -or $SuggestedLines.Count -eq 0) { return $null }
508+
509+
$fileCount = $FileLines.Count
510+
if ($AnchorLine -lt 1) { $AnchorLine = 1 }
511+
if ($AnchorLine -gt $fileCount) { $AnchorLine = $fileCount }
512+
513+
$sCount = $SuggestedLines.Count
514+
$firstLoose = ConvertTo-LooseLine $SuggestedLines[0]
515+
$lastLoose = ConvertTo-LooseLine $SuggestedLines[$sCount - 1]
516+
517+
# --- Single-line suggestion: snap to the nearest unique content match. ---
518+
if ($sCount -eq 1) {
519+
if ((ConvertTo-LooseLine $FileLines[$AnchorLine - 1]) -eq $firstLoose) {
520+
return [pscustomobject]@{ startLine = $AnchorLine; endLine = $AnchorLine }
521+
}
522+
for ($d = 1; $d -le 8; $d++) {
523+
$hits = @()
524+
foreach ($cand in @(($AnchorLine - $d), ($AnchorLine + $d))) {
525+
if ($cand -ge 1 -and $cand -le $fileCount -and
526+
(ConvertTo-LooseLine $FileLines[$cand - 1]) -eq $firstLoose) {
527+
$hits += $cand
528+
}
529+
}
530+
if ($hits.Count -eq 1) { return [pscustomobject]@{ startLine = $hits[0]; endLine = $hits[0] } }
531+
if ($hits.Count -gt 1) { break } # ambiguous at this distance
532+
}
533+
# No content match found: a one-line replacement of the model's anchor
534+
# is still safe (it cannot duplicate context), so trust the anchor.
535+
return [pscustomobject]@{ startLine = $AnchorLine; endLine = $AnchorLine }
536+
}
537+
538+
# --- Multi-line suggestion: find an additive span [s,e] near the anchor. ---
539+
$best = $null
540+
$bestInserted = [int]::MaxValue
541+
$lo = [math]::Max(1, $AnchorLine - $sCount - 4)
542+
$hi = [math]::Min($fileCount, $AnchorLine + $sCount + 4)
543+
for ($s = $lo; $s -le $hi; $s++) {
544+
if ((ConvertTo-LooseLine $FileLines[$s - 1]) -ne $firstLoose) { continue }
545+
# An additive replacement never spans more lines than the suggestion.
546+
$eMax = [math]::Min($fileCount, $s + $sCount - 1)
547+
for ($e = $s; $e -le $eMax; $e++) {
548+
if ((ConvertTo-LooseLine $FileLines[$e - 1]) -ne $lastLoose) { continue }
549+
if ($AnchorLine -lt ($s - 1) -or $AnchorLine -gt ($e + 1)) { continue }
550+
$span = @($FileLines[($s - 1)..($e - 1)])
551+
if (-not (Test-OrderedSubsequence -FileSpan $span -Suggestion $SuggestedLines)) { continue }
552+
$inserted = $sCount - ($e - $s + 1)
553+
if ($inserted -lt $bestInserted) {
554+
$bestInserted = $inserted
555+
$best = [pscustomobject]@{ startLine = $s; endLine = $e }
556+
}
557+
}
558+
}
559+
return $best
560+
}
561+
419562
function Test-GlobMatch {
420563
param([string] $Filename, [string] $Pattern)
421564
$f = $Filename -replace '\\', '/'
@@ -1522,7 +1665,7 @@ function Build-ReferenceLink {
15221665
}
15231666

15241667
function Build-CommentBody {
1525-
param([object] $Finding)
1668+
param([object] $Finding, [switch] $SuppressSuggestion)
15261669

15271670
$domain = $Finding.domain
15281671
$severity = $Finding.severity
@@ -1567,11 +1710,21 @@ function Build-CommentBody {
15671710
}
15681711
}
15691712

1570-
if ($suggested) {
1713+
if ($suggested -and -not $SuppressSuggestion) {
15711714
$lines.Add('') | Out-Null
15721715
$lines.Add('```suggestion') | Out-Null
15731716
$lines.Add($suggested) | Out-Null
15741717
$lines.Add('```') | Out-Null
1718+
} elseif ($suggested -and $SuppressSuggestion) {
1719+
# A concrete fix was identified but its target line(s) could not be
1720+
# matched against the PR-head file, so an applicable suggestion block
1721+
# would risk corrupting the file. Surface the intended change as a
1722+
# non-applicable code snippet instead.
1723+
$lines.Add('') | Out-Null
1724+
$lines.Add('**Suggested fix** (apply manually — could not be anchored as a one-click suggestion):') | Out-Null
1725+
$lines.Add('```al') | Out-Null
1726+
$lines.Add($suggested) | Out-Null
1727+
$lines.Add('```') | Out-Null
15751728
}
15761729

15771730
if ($references.Count -gt 0) {
@@ -1701,6 +1854,46 @@ function Post-Findings {
17011854
$location = $LineMaps[$filePath][$lineNumber]
17021855
}
17031856

1857+
# Validate / re-anchor the ```suggestion``` block against the PR-head
1858+
# file so it replaces the correct line(s). When the finding carries a
1859+
# suggested fix we re-derive its RIGHT-side span and post the comment
1860+
# over that span (single- or multi-line). When the fix cannot be placed
1861+
# confidently we suppress the applicable block (Build-CommentBody falls
1862+
# back to a manual snippet) and keep the comment at the model's anchor.
1863+
$suppressSuggestion = $false
1864+
$commentStartLine = 0
1865+
$commentStartSide = ''
1866+
if ($finding.suggestedCode) {
1867+
$suggested = ([string]$finding.suggestedCode).TrimEnd()
1868+
$suggLines = [string[]]@($suggested -split "`r?`n")
1869+
$placement = $null
1870+
$fileLines = Get-PrHeadFileLines -RelativePath $filePath
1871+
if ($fileLines -and $suggLines.Count -gt 0) {
1872+
$placement = Resolve-SuggestionPlacement -FileLines $fileLines -AnchorLine $lineNumber -SuggestedLines $suggLines
1873+
}
1874+
1875+
$placed = $false
1876+
if ($placement) {
1877+
$map = if ($LineMaps.ContainsKey($filePath)) { $LineMaps[$filePath] } else { @{} }
1878+
$spanOk = $true
1879+
for ($ln = [int]$placement.startLine; $ln -le [int]$placement.endLine; $ln++) {
1880+
if (-not ($map.ContainsKey($ln) -and $map[$ln].side -eq 'RIGHT')) { $spanOk = $false; break }
1881+
}
1882+
if ($spanOk) {
1883+
$location = @{ line = [int]$placement.endLine; side = 'RIGHT' }
1884+
if ([int]$placement.startLine -lt [int]$placement.endLine) {
1885+
$commentStartLine = [int]$placement.startLine
1886+
$commentStartSide = 'RIGHT'
1887+
}
1888+
$placed = $true
1889+
}
1890+
}
1891+
if (-not $placed) {
1892+
$suppressSuggestion = $true
1893+
Write-Host "Suggestion for $($filePath):$lineNumber could not be anchored to the diff; posting as a manual snippet."
1894+
}
1895+
}
1896+
17041897
if ($location) {
17051898
$key = "$($filePath):$($location.line):$($location.side)"
17061899
if ($existingKeys.Contains($key)) {
@@ -1711,11 +1904,11 @@ function Post-Findings {
17111904
}
17121905
}
17131906

1714-
$body = Build-CommentBody -Finding $finding
1907+
$body = Build-CommentBody -Finding $finding -SuppressSuggestion:$suppressSuggestion
17151908

17161909
try {
17171910
if ($location) {
1718-
$null = New-ReviewComment -Body $body -Path $filePath -Line $location.line -Side $location.side
1911+
$null = New-ReviewComment -Body $body -Path $filePath -Line $location.line -Side $location.side -StartLine $commentStartLine -StartSide $commentStartSide
17191912
$existingKeys.Add("$($filePath):$($location.line):$($location.side)") | Out-Null
17201913
$existingLocations.Add([pscustomobject]@{ path = $filePath; line = [int]$location.line; side = $location.side }) | Out-Null
17211914
$postedInline++

0 commit comments

Comments
 (0)