From 774e39164f261f2c36c49cbac76b83979f36f079 Mon Sep 17 00:00:00 2001 From: Bugale Date: Fri, 14 Aug 2026 23:09:38 +0300 Subject: [PATCH] feat: comment on an issue whose fix reaches beyond the pull request diff An issue was commented on only when the diff showed every line it spans, because GitHub rejects a comment anchored outside the diff. A formatter that reformats a whole statement because one of its lines changed reports a range reaching past the three context lines the diff carries around that change, so such an issue failed the step without any comment explaining it. The comment is now anchored on the run of lines that the diff does show around an added line of the range, which is the widest anchor GitHub accepts. The fix of an issue reaching beyond that anchor is rendered as a plain code block naming the lines it covers rather than as a suggestion, since applying it in one click would rewrite lines the pull request does not show. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- README.md | 10 ++++-- __tests__/bugalint.test.ts | 71 +++++++++++++++++++++++++++++++++----- dist/index.js | 53 ++++++++++++++++++++-------- src/bugalint.ts | 58 +++++++++++++++++++++++-------- 4 files changed, 153 insertions(+), 39 deletions(-) diff --git a/README.md b/README.md index 1859337..42d49fe 100644 --- a/README.md +++ b/README.md @@ -39,8 +39,9 @@ steps: repository. If set to an empty string, the action will not write a SARIF file. The SARIF is always generated and printed to the workflow log. - `comment`: Set to true to comment on the PR with the issues. If set to false or ommitted, the action will not comment on the PR. Issues that carry a fix are - commented as [suggested changes](#suggested-changes). An issue is commented on only if every line it spans is part of the pull request's diff, as GitHub - rejects comments anchored outside it. + commented as [suggested changes](#suggested-changes). An issue is commented on only if the pull request's diff adds at least one of the lines it spans, as + GitHub rejects comments anchored outside the diff. An issue whose range reaches beyond what the diff shows is anchored on the part of it that the diff does + show, and its fix is then rendered as plain text rather than as a suggestion, since applying it would rewrite lines the pull request does not show. - `summary`: True by default - generates a markdown summary for the job. If set to false, the action will not generate a markdown summary. @@ -210,6 +211,11 @@ commented as a suggestion. To have one commented, widen the replacement to cover The fixes Bugalint writes out always use the second form, so a fix survives being read back from a SARIF file that Bugalint itself generated. +A fix is offered as a suggestion only when the pull request's diff shows every line the issue spans. A formatter reformatting a whole statement because one of +its lines changed reports a range reaching past the three context lines the diff carries around that change, and GitHub rejects a comment anchored outside the +diff. Such an issue is still commented on, anchored on the lines of its range that the diff does show, with its fix rendered as a plain code block and a note +naming the lines it covers, since applying it in one click would rewrite lines the pull request does not show. + ### Example With Custom Regex This is an example of how this action can be used to parse the output of a hypothetical custom linter called `mylinter`, which outputs issues in the following diff --git a/__tests__/bugalint.test.ts b/__tests__/bugalint.test.ts index 32d8386..39b1fb2 100644 --- a/__tests__/bugalint.test.ts +++ b/__tests__/bugalint.test.ts @@ -7,7 +7,7 @@ import { getRegexParser, parseDiffLines, isNewIssue, - isCommentableIssue, + getCommentAnchor, failOnIssues, filterNewIssues, _testExports, @@ -75,11 +75,26 @@ index 1111111..2222222 100644 expect(isNewIssue({ path: 'A/B/test.py' }, diffLines, '.')).toBe(false) }) - it('comments only on issues whose whole range is in the diff', () => { - expect(isCommentableIssue({ path: 'A/B/test.py', line: 1, eline: 3 }, diffLines, '.')).toBe(true) - expect(isCommentableIssue({ path: 'A/B/test.py', line: 3, eline: 4 }, diffLines, '.')).toBe(false) - expect(isCommentableIssue({ path: 'A/B/other.py', line: 1 }, diffLines, '.')).toBe(false) - expect(isCommentableIssue({ path: 'A/B/test.py' }, diffLines, '.')).toBe(false) + it('anchors a comment on the whole range when the diff shows all of it', () => { + const anchor = { path: 'A/B/test.py', line: 1, eline: 3, partial: false } + expect(getCommentAnchor({ path: 'A/B/test.py', line: 1, eline: 3 }, diffLines, '.')).toStrictEqual(anchor) + }) + + it('anchors a comment on the shown part of a range the diff cuts off', () => { + const anchor = { path: 'A/B/test.py', line: 3, eline: 3, partial: true } + expect(getCommentAnchor({ path: 'A/B/test.py', line: 3, eline: 4 }, diffLines, '.')).toStrictEqual(anchor) + }) + + it('relativizes the anchor path to the analysis path', () => { + const anchor = { path: 'A/B/test.py', line: 3, eline: 3, partial: false } + expect(getCommentAnchor({ path: 'test.py', line: 3 }, diffLines, 'A\\B')).toStrictEqual(anchor) + }) + + it('anchors nothing on an issue the diff does not add', () => { + expect(getCommentAnchor({ path: 'A/B/test.py', line: 1 }, diffLines, '.')).toBeUndefined() + expect(getCommentAnchor({ path: 'A/B/other.py', line: 1 }, diffLines, '.')).toBeUndefined() + expect(getCommentAnchor({ path: 'A/B/test.py' }, diffLines, '.')).toBeUndefined() + expect(getCommentAnchor({ line: 2 }, diffLines, '.')).toBeUndefined() }) describe('failOnIssues', () => { @@ -152,6 +167,46 @@ describe('commentBody', () => { }) }) +describe('fixBeyondDiff', () => { + const diff = [ + 'diff --git a/A.cpp b/A.cpp', + 'index 1111111..2222222 100644', + '--- a/A.cpp', + '+++ b/A.cpp', + '@@ -874,7 +874,7 @@', + ' &obja,', + ' &iosb,', + ' nullptr,', + '- FLAG_A | FLAG_B | FLAG_C,', + '+ FLAG_A | FLAG_B,', + ' nullptr,', + ' 0,', + ' CreateFileTypeNone,', + '' + ].join('\n') + const diffLines = parseDiffLines(diff) + const issue = { level: 'warning' as const, path: 'A.cpp', line: 868, eline: 883, fix: ['status = f(a,', ' b);'] } + const tag = '' + + it('anchors on the lines the hunk shows when the fix reaches past both of its ends', () => { + expect(getCommentAnchor(issue, diffLines, '.')).toStrictEqual({ path: 'A.cpp', line: 874, eline: 880, partial: true }) + }) + + it('offers the fix as plain text rather than a suggestion, naming the lines it covers', () => { + expect(_testExports.buildCommentBody(tag, 'clang-format', issue, true)).toBe( + `${tag}\n[warning:clang-format]\n` + + 'The pull request diff does not show all of lines 868-883, so this replacement cannot be offered as a suggestion:\n' + + '```\nstatus = f(a,\n b);\n```' + ) + }) + + it('still offers a suggestion once the diff shows the whole fix', () => { + expect(_testExports.buildCommentBody(tag, 'clang-format', issue)).toBe( + `${tag}\n[warning:clang-format]\n\`\`\`suggestion\nstatus = f(a,\n b);\n\`\`\`` + ) + }) +}) + describe('sarifFix', () => { const fixOf = (region: Region, deletedRegion: Region, text: string): string[] | undefined => { const log = { @@ -304,8 +359,8 @@ describe('invertedRange', () => { it('never reaches a comment anchor whose start follows its end', () => { const diffLines = parseDiffLines('diff --git a/t.py b/t.py\n--- a/t.py\n+++ b/t.py\n@@ -1,1 +1,3 @@\n x\n+y\n+z\n') - expect(isCommentableIssue({ path: 't.py', line: 2, eline: 3 }, diffLines, '.')).toBe(true) - expect(isCommentableIssue({ path: 't.py', line: 3, eline: 2 }, diffLines, '.')).toBe(false) + expect(getCommentAnchor({ path: 't.py', line: 2, eline: 3 }, diffLines, '.')).toStrictEqual({ path: 't.py', line: 2, eline: 3, partial: false }) + expect(getCommentAnchor({ path: 't.py', line: 3, eline: 2 }, diffLines, '.')).toBeUndefined() }) }) diff --git a/dist/index.js b/dist/index.js index 85d8ff2..8f8f9c2 100644 --- a/dist/index.js +++ b/dist/index.js @@ -29940,7 +29940,7 @@ exports.addComments = addComments; exports.getPrDiff = getPrDiff; exports.parseDiffLines = parseDiffLines; exports.isNewIssue = isNewIssue; -exports.isCommentableIssue = isCommentableIssue; +exports.getCommentAnchor = getCommentAnchor; exports.filterNewIssues = filterNewIssues; exports.failOnIssues = failOnIssues; exports.createSummary = createSummary; @@ -30201,14 +30201,19 @@ function getKnownParser(identifier, message) { function getRegexParser(regex, message, levelMap) { return (input) => appendMessage(parseRegex(input, regex, levelMap), message); } -function buildCommentBody(commentTag, identifier, issue) { +function buildCommentBody(commentTag, identifier, issue, partial = false) { const identifiers = `[${[issue.level, identifier, issue.id, issue.sym].filter((n) => n).join(':')}]`; const body = `${commentTag}\n${[issue.msg != null && issue.msg !== '' ? `**${issue.msg}**` : undefined, identifiers].filter((n) => n).join('\n')}`; if (issue.fix == null || (issue.fix.length === 1 && issue.fix[0] === '')) { return body; } const fence = '`'.repeat(Math.max(3, ...Array.from(issue.fix.join('\n').matchAll(/`+/g), (m) => m[0].length + 1))); - return `${body}\n${fence}suggestion\n${issue.fix.map((line) => `${line}\n`).join('')}${fence}`; + const fix = issue.fix.map((line) => `${line}\n`).join(''); + if (!partial) { + return `${body}\n${fence}suggestion\n${fix}${fence}`; + } + const note = `The pull request diff does not show all of lines ${issue.line}-${issue.eline ?? issue.line}, so this replacement cannot be offered as a suggestion:`; + return `${body}\n${note}\n${fence}\n${fix}${fence}`; } async function addComments(issues, prDiff, githubToken, identifier, owner, repo, prNumber, analysisPath) { /* eslint camelcase: ["error", {allow: ['^pull_number$', '^comment_id$', '^start_side$', '^start_line$']}] */ @@ -30227,22 +30232,22 @@ async function addComments(issues, prDiff, githubToken, identifier, owner, repo, const comments = []; for (const issue of issues) { (0, core_1.debug)(`Processing issue on ${issue.path}:${issue.line}`); - if (!isCommentableIssue(issue, diffLines, analysisPath)) { - (0, core_1.debug)(`Skipping issue on ${issue.path}:${issue.line} because it is not on lines the pull request diff shows`); + const anchor = getCommentAnchor(issue, diffLines, analysisPath); + if (anchor == null) { + (0, core_1.debug)(`Skipping issue on ${issue.path}:${issue.line} because it is not on lines the pull request adds`); continue; } if (comments.length >= 50) { (0, core_1.warning)('More than 50 comments detected. Only the first 50 will be posted.'); break; } - const endLine = issue.eline ?? issue.line; const args = { - path: normalizePath(issue.path, analysisPath), + path: anchor.path, side: 'RIGHT', start_side: 'RIGHT', - line: endLine, - start_line: endLine === issue.line ? undefined : issue.line, - body: buildCommentBody(commentTag, identifier, issue) + line: anchor.eline, + start_line: anchor.eline === anchor.line ? undefined : anchor.line, + body: buildCommentBody(commentTag, identifier, issue, anchor.partial) }; (0, core_1.debug)(`Generating comment ${JSON.stringify(args)}`); comments.push(args); @@ -30304,12 +30309,30 @@ function isNewIssue(issue, diffLines, analysisPath) { const lines = diffLines[normalizePath(issue.path, analysisPath)]; return issueLines(issue.line, issue.eline).some((line) => lines?.[line] ?? false); } -function isCommentableIssue(issue, diffLines, analysisPath) { - if (!isNewIssue(issue, diffLines, analysisPath)) { - return false; +function getCommentAnchor(issue, diffLines, analysisPath) { + if (issue.path == null || issue.line == null) { + return undefined; } - const lines = diffLines[normalizePath(issue.path, analysisPath)]; - return issueLines(issue.line, issue.eline).every((line) => lines?.[line] != null); + const commentPath = normalizePath(issue.path, analysisPath); + const lines = diffLines[commentPath]; + const eline = issue.eline ?? issue.line; + const range = issueLines(issue.line, eline); + const added = range.find((line) => lines?.[line] ?? false); + if (added == null) { + return undefined; + } + if (range.every((line) => lines?.[line] != null)) { + return { path: commentPath, line: issue.line, eline, partial: false }; + } + let start = added; + let end = added; + while (start > issue.line && lines?.[start - 1] != null) { + start--; + } + while (end < eline && lines?.[end + 1] != null) { + end++; + } + return { path: commentPath, line: start, eline: end, partial: true }; } function filterNewIssues(issues, prDiff, analysisPath) { const diffLines = parseDiffLines(prDiff); diff --git a/src/bugalint.ts b/src/bugalint.ts index e59f08e..a8b0d59 100644 --- a/src/bugalint.ts +++ b/src/bugalint.ts @@ -297,14 +297,19 @@ export function getRegexParser(regex: RegExp, message: string, levelMap?: Record return (input: string) => appendMessage(parseRegex(input, regex, levelMap), message) } -function buildCommentBody(commentTag: string, identifier: string, issue: Issue): string { +function buildCommentBody(commentTag: string, identifier: string, issue: Issue, partial = false): string { const identifiers = `[${[issue.level, identifier, issue.id, issue.sym].filter((n) => n).join(':')}]` const body = `${commentTag}\n${[issue.msg != null && issue.msg !== '' ? `**${issue.msg}**` : undefined, identifiers].filter((n) => n).join('\n')}` if (issue.fix == null || (issue.fix.length === 1 && issue.fix[0] === '')) { return body } const fence = '`'.repeat(Math.max(3, ...Array.from(issue.fix.join('\n').matchAll(/`+/g), (m) => m[0].length + 1))) - return `${body}\n${fence}suggestion\n${issue.fix.map((line) => `${line}\n`).join('')}${fence}` + const fix = issue.fix.map((line) => `${line}\n`).join('') + if (!partial) { + return `${body}\n${fence}suggestion\n${fix}${fence}` + } + const note = `The pull request diff does not show all of lines ${issue.line}-${issue.eline ?? issue.line}, so this replacement cannot be offered as a suggestion:` + return `${body}\n${note}\n${fence}\n${fix}${fence}` } export async function addComments( @@ -336,8 +341,9 @@ export async function addComments( const comments = [] for (const issue of issues) { debug(`Processing issue on ${issue.path}:${issue.line}`) - if (!isCommentableIssue(issue, diffLines, analysisPath)) { - debug(`Skipping issue on ${issue.path}:${issue.line} because it is not on lines the pull request diff shows`) + const anchor = getCommentAnchor(issue, diffLines, analysisPath) + if (anchor == null) { + debug(`Skipping issue on ${issue.path}:${issue.line} because it is not on lines the pull request adds`) continue } if (comments.length >= 50) { @@ -345,14 +351,13 @@ export async function addComments( break } - const endLine = issue.eline ?? issue.line const args = { - path: normalizePath(issue.path, analysisPath), + path: anchor.path, side: 'RIGHT', start_side: 'RIGHT', - line: endLine, - start_line: endLine === issue.line ? undefined : issue.line, - body: buildCommentBody(commentTag, identifier, issue) + line: anchor.eline, + start_line: anchor.eline === anchor.line ? undefined : anchor.line, + body: buildCommentBody(commentTag, identifier, issue, anchor.partial) } debug(`Generating comment ${JSON.stringify(args)}`) comments.push(args) @@ -421,12 +426,37 @@ export function isNewIssue(issue: Issue, diffLines: DiffLines, analysisPath: str return issueLines(issue.line, issue.eline).some((line) => lines?.[line] ?? false) } -export function isCommentableIssue(issue: Issue, diffLines: DiffLines, analysisPath: string): issue is Issue & Required> { - if (!isNewIssue(issue, diffLines, analysisPath)) { - return false +export interface CommentAnchor { + path: string + line: number + eline: number + partial: boolean +} + +export function getCommentAnchor(issue: Issue, diffLines: DiffLines, analysisPath: string): CommentAnchor | undefined { + if (issue.path == null || issue.line == null) { + return undefined } - const lines: Record | undefined = diffLines[normalizePath(issue.path, analysisPath)] - return issueLines(issue.line, issue.eline).every((line) => lines?.[line] != null) + const commentPath = normalizePath(issue.path, analysisPath) + const lines: Record | undefined = diffLines[commentPath] + const eline = issue.eline ?? issue.line + const range = issueLines(issue.line, eline) + const added = range.find((line) => lines?.[line] ?? false) + if (added == null) { + return undefined + } + if (range.every((line) => lines?.[line] != null)) { + return { path: commentPath, line: issue.line, eline, partial: false } + } + let start = added + let end = added + while (start > issue.line && lines?.[start - 1] != null) { + start-- + } + while (end < eline && lines?.[end + 1] != null) { + end++ + } + return { path: commentPath, line: start, eline: end, partial: true } } export function filterNewIssues(issues: Iterable, prDiff: string, analysisPath: string): Issue[] {