From f1a0c1982e8cf21ae9d01c062f41259192c431a3 Mon Sep 17 00:00:00 2001 From: Bugale Date: Sat, 25 Jul 2026 15:37:23 +0300 Subject: [PATCH 01/11] feat: render SARIF fixes as GitHub suggestions Issues that carry a fix are now commented as a suggestion block, which a reviewer can apply in one click. Fixes are read from the standard SARIF fixes[].artifactChanges[].replacements[] of the result, and are re-emitted in the generated SARIF so that code scanning receives them too. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/workflows/check-code.yml | 1 + README.md | 40 ++++++++++++++- __tests__/bugalint.test.ts | 32 ++++++++++++ __tests__/sariffix.input.txt | 83 ++++++++++++++++++++++++++++++++ __tests__/sariffix.output.json | 83 ++++++++++++++++++++++++++++++++ dist/index.js | 31 ++++++++++-- src/bugalint.ts | 34 +++++++++++-- 7 files changed, 294 insertions(+), 10 deletions(-) create mode 100644 __tests__/sariffix.input.txt create mode 100644 __tests__/sariffix.output.json diff --git a/.github/workflows/check-code.yml b/.github/workflows/check-code.yml index cf0d9d9..e0baddd 100644 --- a/.github/workflows/check-code.yml +++ b/.github/workflows/check-code.yml @@ -80,6 +80,7 @@ jobs: - {linter: 'yamllint', format: 'yamllint', regex: '', levelMap: '', analysisPath: '.'} - {linter: 'ghalint', format: 'ghalint', regex: '', levelMap: '', analysisPath: '.'} - {linter: 'sarif', format: 'sarif', regex: '', levelMap: '', analysisPath: '.'} + - {linter: 'sariffix', format: 'sarif', regex: '', levelMap: '', analysisPath: '.'} - {linter: 'flake8subpath', format: 'flake8', regex: '', levelMap: '', analysisPath: 'A\B'} - {linter: 'noissues', format: 'flake8', regex: '', levelMap: '', analysisPath: '.', outcome: 'success'} - {linter: 'noissues', format: 'flake8', regex: '', levelMap: '', analysisPath: '.', fail: 'false', outcome: 'success', name: 'noissues-nofail'} diff --git a/README.md b/README.md index 2295d65..83ce370 100644 --- a/README.md +++ b/README.md @@ -38,7 +38,8 @@ steps: - `sarif`: The path to the output SARIF file this action should generate. If not specified, the action will generate a `sarif.json` file in the root of the 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. +- `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). - `summary`: True by default - generates a markdown summary for the job. If set to false, the action will not generate a markdown summary. @@ -85,7 +86,7 @@ This action supports a bunch of linter output formats, for which no `inputRegex` - `ghalint`: The format of [ghallint](https://github.com/suzuki-shunsuke/ghalint/cmd/ghalint/) linter's parsable output. - `SARIF`: A [standard format for static analysis](https://sarifweb.azurewebsites.net/). This is useful if you already have a SARIF file and want to create a summary - for it, or create comments on the PR. + for it, or create comments on the PR. This is also the only input format that can carry [suggested changes](#suggested-changes). #### Input Regex Named Groups @@ -113,6 +114,41 @@ The supported named groups are: - `ecol`: The end column on which the issue was reported. +### Suggested Changes + +When an issue carries a fix, the comment posted on the pull request contains it as a +[suggested change](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request), +which a reviewer can apply in one click. Fixes are read from the first `replacements` entry of the first `artifactChanges` entry of the result's first `fixes` +entry, so they are only available when `inputFormat` is `sarif`: + +```json +{ + "message": { "text": "Not formatted correctly" }, + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "test.py" }, "region": { "startLine": 3, "endLine": 4 } } }], + "fixes": [ + { + "artifactChanges": [ + { + "artifactLocation": { "uri": "test.py" }, + "replacements": [{ "deletedRegion": { "startLine": 3, "endLine": 4 }, "insertedContent": { "text": "def f():\n return 1" } }] + } + ] + } + ] +} +``` + +GitHub replaces whole lines, which constrains what a fix may contain: + +- `deletedRegion` must cover whole lines, i.e. specify only `startLine` and `endLine`, and must match the region of the result itself, which is what the comment + is anchored to. + +- `insertedContent.text` is the exact text replacing those lines, and should not end with a newline. It is never trimmed, so a trailing newline is rendered as a + trailing empty line. + +- An empty `insertedContent.text` renders as an empty suggestion, which deletes the lines. A replacement consisting of a single empty line is therefore + indistinguishable from a deletion, and cannot be expressed. A producer that needs one should widen the replacement to include a neighbouring line. + ### 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 bcff7ea..d8730c8 100644 --- a/__tests__/bugalint.test.ts +++ b/__tests__/bugalint.test.ts @@ -11,6 +11,7 @@ describe('fullConversion', () => { ['yamllint', getKnownParser('yamllint'), '.'], ['ghalint', getKnownParser('ghalint'), '.'], ['sarif', getKnownParser('sarif'), '.'], + ['sariffix', getKnownParser('sarif'), '.'], ['flake8subpath', getKnownParser('flake8'), 'A\\B'], ['noissues', getKnownParser('flake8'), '.'], [ @@ -91,6 +92,37 @@ index 1111111..2222222 100644 }) }) +describe('commentBody', () => { + const tag = '' + const header = `${tag}\n**Message**\n[warning:test]` + const issue = { level: 'warning' as const, msg: 'Message' } + const body = (fix?: string): string => _testExports.buildCommentBody(tag, 'test', fix === undefined ? issue : { ...issue, fix }) + + it('omits the suggestion when the issue has no fix', () => { + expect(body()).toBe(header) + }) + + it('appends the suggestion after the identifier line', () => { + expect(body('x = 1')).toBe(`${header}\n\`\`\`suggestion\nx = 1\n\`\`\``) + }) + + it('keeps a multi line fix verbatim', () => { + expect(body('def f():\n return 1')).toBe(`${header}\n\`\`\`suggestion\ndef f():\n return 1\n\`\`\``) + }) + + it('renders an empty fix as an empty suggestion, which deletes the lines', () => { + expect(body('')).toBe(`${header}\n\`\`\`suggestion\n\`\`\``) + }) + + it('preserves a trailing newline, which keeps a trailing empty line', () => { + expect(body('x = 1\n')).toBe(`${header}\n\`\`\`suggestion\nx = 1\n\n\`\`\``) + }) + + it('uses a fence longer than the longest backtick run in the fix', () => { + expect(body('doc = "```"')).toBe(`${header}\n\`\`\`\`suggestion\ndoc = "\`\`\`"\n\`\`\`\``) + }) +}) + describe('windowsFileUrl', () => { if (process.platform === 'win32') { it('should convert Windows file URLs to relative URLs', () => { diff --git a/__tests__/sariffix.input.txt b/__tests__/sariffix.input.txt new file mode 100644 index 0000000..be3a1d9 --- /dev/null +++ b/__tests__/sariffix.input.txt @@ -0,0 +1,83 @@ +{ + "version": "2.1.0", + "$schema": "http://json.schemastore.org/sarif-2.1.0-rtm.6", + "runs": [ + { + "tool": { + "driver": { + "name": "test", + "rules": [{ "id": "F001", "name": "formatting" }] + } + }, + "results": [ + { + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "test.py" }, "region": { "startLine": 3, "endLine": 3 } } }], + "level": "warning", + "message": { "text": "Single line replacement" }, + "ruleId": "F001", + "ruleIndex": 0, + "fixes": [ + { + "artifactChanges": [ + { + "artifactLocation": { "uri": "test.py" }, + "replacements": [{ "deletedRegion": { "startLine": 3, "endLine": 3 }, "insertedContent": { "text": "x = 1" } }] + } + ] + } + ] + }, + { + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "test.py" }, "region": { "startLine": 10, "endLine": 12 } } }], + "level": "warning", + "message": { "text": "Multi line replacement" }, + "fixes": [ + { + "artifactChanges": [ + { + "artifactLocation": { "uri": "test.py" }, + "replacements": [{ "deletedRegion": { "startLine": 10, "endLine": 12 }, "insertedContent": { "text": "def f():\n return 1" } }] + } + ] + } + ] + }, + { + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "test.py" }, "region": { "startLine": 20, "endLine": 21 } } }], + "level": "warning", + "message": { "text": "Deletion" }, + "fixes": [ + { + "artifactChanges": [ + { + "artifactLocation": { "uri": "test.py" }, + "replacements": [{ "deletedRegion": { "startLine": 20, "endLine": 21 }, "insertedContent": { "text": "" } }] + } + ] + } + ] + }, + { + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "test.py" }, "region": { "startLine": 30, "endLine": 30 } } }], + "level": "warning", + "message": { "text": "Replacement containing a fence" }, + "fixes": [ + { + "artifactChanges": [ + { + "artifactLocation": { "uri": "test.py" }, + "replacements": [{ "deletedRegion": { "startLine": 30, "endLine": 30 }, "insertedContent": { "text": "doc = \"```\"" } }] + } + ] + } + ] + }, + { + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "test.py" }, "region": { "startLine": 40, "endLine": 40 } } }], + "level": "error", + "message": { "text": "No fix available" } + } + ] + } + ] +} diff --git a/__tests__/sariffix.output.json b/__tests__/sariffix.output.json new file mode 100644 index 0000000..4756af2 --- /dev/null +++ b/__tests__/sariffix.output.json @@ -0,0 +1,83 @@ +{ + "version": "2.1.0", + "$schema": "http://json.schemastore.org/sarif-2.1.0-rtm.6", + "runs": [ + { + "tool": { + "driver": { + "name": "test", + "rules": [{ "id": "F001", "name": "formatting" }] + } + }, + "results": [ + { + "message": { "text": "Single line replacement" }, + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "test.py" }, "region": { "startLine": 3, "endLine": 3 } } }], + "fixes": [ + { + "artifactChanges": [ + { + "artifactLocation": { "uri": "test.py" }, + "replacements": [{ "deletedRegion": { "startLine": 3, "endLine": 3 }, "insertedContent": { "text": "x = 1" } }] + } + ] + } + ], + "level": "warning", + "ruleId": "F001", + "ruleIndex": 0 + }, + { + "message": { "text": "Multi line replacement" }, + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "test.py" }, "region": { "startLine": 10, "endLine": 12 } } }], + "fixes": [ + { + "artifactChanges": [ + { + "artifactLocation": { "uri": "test.py" }, + "replacements": [{ "deletedRegion": { "startLine": 10, "endLine": 12 }, "insertedContent": { "text": "def f():\n return 1" } }] + } + ] + } + ], + "level": "warning" + }, + { + "message": { "text": "Deletion" }, + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "test.py" }, "region": { "startLine": 20, "endLine": 21 } } }], + "fixes": [ + { + "artifactChanges": [ + { + "artifactLocation": { "uri": "test.py" }, + "replacements": [{ "deletedRegion": { "startLine": 20, "endLine": 21 }, "insertedContent": { "text": "" } }] + } + ] + } + ], + "level": "warning" + }, + { + "message": { "text": "Replacement containing a fence" }, + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "test.py" }, "region": { "startLine": 30, "endLine": 30 } } }], + "fixes": [ + { + "artifactChanges": [ + { + "artifactLocation": { "uri": "test.py" }, + "replacements": [{ "deletedRegion": { "startLine": 30, "endLine": 30 }, "insertedContent": { "text": "doc = \"```\"" } }] + } + ] + } + ], + "level": "warning" + }, + { + "message": { "text": "No fix available" }, + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "test.py" }, "region": { "startLine": 40, "endLine": 40 } } }], + "level": "error" + } + ] + } + ] +} diff --git a/dist/index.js b/dist/index.js index 12cfc0c..3f82377 100644 --- a/dist/index.js +++ b/dist/index.js @@ -30000,7 +30000,8 @@ function* parseSarif(input) { line: issue.locations?.[0]?.physicalLocation?.region?.startLine, col: issue.locations?.[0]?.physicalLocation?.region?.startColumn, eline: issue.locations?.[0]?.physicalLocation?.region?.endLine, - ecol: issue.locations?.[0]?.physicalLocation?.region?.endColumn + ecol: issue.locations?.[0]?.physicalLocation?.region?.endColumn, + fix: issue.fixes?.[0]?.artifactChanges?.[0]?.replacements?.[0]?.insertedContent?.text }; } } @@ -30035,12 +30036,13 @@ function generateSarif(issues, identifier, analysisPath) { rulesIndices[issue.id] = rules.length; rules.push({ id: issue.id, name: issue.sym }); } + const uri = issue.path != null ? normalizePath(issue.path, analysisPath) : undefined; results.push({ message: { text: issue.msg ?? undefined }, locations: [ { physicalLocation: { - artifactLocation: { uri: issue.path != null ? normalizePath(issue.path, analysisPath) : undefined }, + artifactLocation: { uri }, region: issue.line != null || issue.col != null || issue.eline != null || issue.ecol != null ? { startLine: issue.line ?? undefined, @@ -30052,6 +30054,18 @@ function generateSarif(issues, identifier, analysisPath) { } } ], + fixes: issue.fix != null && uri != null && issue.line != null + ? [ + { + artifactChanges: [ + { + artifactLocation: { uri }, + replacements: [{ deletedRegion: { startLine: issue.line, endLine: issue.eline ?? issue.line }, insertedContent: { text: issue.fix } }] + } + ] + } + ] + : undefined, level: issue.level ?? undefined, ruleId: issue.id ?? issue.sym ?? undefined, ruleIndex: issue.id != null ? rulesIndices[issue.id] : undefined @@ -30073,6 +30087,14 @@ function getKnownParser(identifier) { function getRegexParser(regex, levelMap) { return (input) => parseRegex(input, regex, levelMap); } +function buildCommentBody(commentTag, identifier, issue) { + const body = `${commentTag}\n**${issue.msg}**\n[${[issue.level, identifier, issue.id, issue.sym].filter((n) => n).join(':')}]`; + if (issue.fix == null) { + return body; + } + const fence = '`'.repeat(Math.max(3, ...Array.from(issue.fix.matchAll(/`+/g), (m) => m[0].length + 1))); + return `${body}\n${fence}suggestion\n${issue.fix === '' ? '' : `${issue.fix}\n`}${fence}`; +} async function addComments(issues, prDiff, githubToken, identifier, owner, repo, prNumber, analysisPath) { /* eslint camelcase: ["error", {allow: ['^pull_number$', '^comment_id$', '^start_side$', '^start_line$']}] */ const octokit = (0, github_1.getOctokit)(githubToken); @@ -30105,7 +30127,7 @@ async function addComments(issues, prDiff, githubToken, identifier, owner, repo, start_side: 'RIGHT', line: endLine, start_line: endLine === issue.line ? undefined : issue.line, - body: `${commentTag}\n**${issue.msg}**\n[${[issue.level, identifier, issue.id, issue.sym].filter((n) => n).join(':')}]` + body: buildCommentBody(commentTag, identifier, issue) }; (0, core_1.debug)(`Generating comment ${JSON.stringify(args)}`); comments.push(args); @@ -30189,7 +30211,8 @@ async function createSummary(issues, identifier, analysisPath) { await core_1.summary.write(); } exports._testExports = { - normalizePath + normalizePath, + buildCommentBody }; diff --git a/src/bugalint.ts b/src/bugalint.ts index 729cd93..f6cfb4f 100644 --- a/src/bugalint.ts +++ b/src/bugalint.ts @@ -15,6 +15,7 @@ interface Issue { col?: number eline?: number ecol?: number + fix?: string } export type Parser = (input: string) => Generator @@ -75,7 +76,8 @@ function* parseSarif(input: string): Generator { line: issue.locations?.[0]?.physicalLocation?.region?.startLine, col: issue.locations?.[0]?.physicalLocation?.region?.startColumn, eline: issue.locations?.[0]?.physicalLocation?.region?.endLine, - ecol: issue.locations?.[0]?.physicalLocation?.region?.endColumn + ecol: issue.locations?.[0]?.physicalLocation?.region?.endColumn, + fix: issue.fixes?.[0]?.artifactChanges?.[0]?.replacements?.[0]?.insertedContent?.text } } } @@ -119,12 +121,13 @@ export function generateSarif(issues: Iterable, identifier: string, analy rulesIndices[issue.id] = rules.length rules.push({ id: issue.id, name: issue.sym }) } + const uri = issue.path != null ? normalizePath(issue.path, analysisPath) : undefined results.push({ message: { text: issue.msg ?? undefined }, locations: [ { physicalLocation: { - artifactLocation: { uri: issue.path != null ? normalizePath(issue.path, analysisPath) : undefined }, + artifactLocation: { uri }, region: issue.line != null || issue.col != null || issue.eline != null || issue.ecol != null ? { @@ -137,6 +140,19 @@ export function generateSarif(issues: Iterable, identifier: string, analy } } ], + fixes: + issue.fix != null && uri != null && issue.line != null + ? [ + { + artifactChanges: [ + { + artifactLocation: { uri }, + replacements: [{ deletedRegion: { startLine: issue.line, endLine: issue.eline ?? issue.line }, insertedContent: { text: issue.fix } }] + } + ] + } + ] + : undefined, level: issue.level ?? undefined, ruleId: issue.id ?? issue.sym ?? undefined, ruleIndex: issue.id != null ? rulesIndices[issue.id] : undefined @@ -161,6 +177,15 @@ export function getRegexParser(regex: RegExp, levelMap?: Record parseRegex(input, regex, levelMap) } +function buildCommentBody(commentTag: string, identifier: string, issue: Issue): string { + const body = `${commentTag}\n**${issue.msg}**\n[${[issue.level, identifier, issue.id, issue.sym].filter((n) => n).join(':')}]` + if (issue.fix == null) { + return body + } + const fence = '`'.repeat(Math.max(3, ...Array.from(issue.fix.matchAll(/`+/g), (m) => m[0].length + 1))) + return `${body}\n${fence}suggestion\n${issue.fix === '' ? '' : `${issue.fix}\n`}${fence}` +} + export async function addComments( issues: Iterable, prDiff: string, @@ -206,7 +231,7 @@ export async function addComments( start_side: 'RIGHT', line: endLine, start_line: endLine === issue.line ? undefined : issue.line, - body: `${commentTag}\n**${issue.msg}**\n[${[issue.level, identifier, issue.id, issue.sym].filter((n) => n).join(':')}]` + body: buildCommentBody(commentTag, identifier, issue) } debug(`Generating comment ${JSON.stringify(args)}`) comments.push(args) @@ -297,5 +322,6 @@ export async function createSummary(issues: Iterable, identifier: string, } export const _testExports = { - normalizePath + normalizePath, + buildCommentBody } From 2836e5629c89682e0f0a56de1f6d86abba4478f0 Mon Sep 17 00:00:00 2001 From: Bugale Date: Sat, 25 Jul 2026 15:41:30 +0300 Subject: [PATCH 02/11] feat: match multi line issues that partially overlap added lines An issue is now considered new when any of the lines it spans was added, instead of requiring all of them. A fix for a multi line issue usually has to touch the lines around the added one, and requiring the whole range to be added silently dropped such issues from both the comments and the failure count. Comments are additionally skipped when the issue spans a line outside the pull request diff, which GitHub rejects. All comments are posted in a single review, so one rejected anchor would drop the whole batch. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- README.md | 6 +++-- __tests__/bugalint.test.ts | 45 ++++++++++++++++++++++---------- dist/index.js | 53 +++++++++++++++++++++++++------------- src/bugalint.ts | 53 +++++++++++++++++++++++++------------- 4 files changed, 105 insertions(+), 52 deletions(-) diff --git a/README.md b/README.md index 83ce370..ed70abd 100644 --- a/README.md +++ b/README.md @@ -39,14 +39,16 @@ 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). + 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. - `summary`: True by default - generates a markdown summary for the job. If set to false, the action will not generate a markdown summary. - `fail`: True by default - fails the step if the linter found any issues. If set to false, the action will not fail the step. - `failOnlyNew`: Set to true to fail only on issues found on lines added in the pull request (requires running on a pull request). If set to false or - omitted, the action will fail on any issue. Ignored if `fail` is set to false. + omitted, the action will fail on any issue. Ignored if `fail` is set to false. An issue spanning several lines is considered new if any one of them was + added, since fixing such an issue usually requires changing the lines around it as well. - `toolName`: _(required)_ The `tool name` that will be written in the SARIF output. This is used by both code scanning and auto-pr-commenting to resolve fixed issues. diff --git a/__tests__/bugalint.test.ts b/__tests__/bugalint.test.ts index d8730c8..c6ad9db 100644 --- a/__tests__/bugalint.test.ts +++ b/__tests__/bugalint.test.ts @@ -1,6 +1,16 @@ import '@microsoft/jest-sarif' import { readFileSync } from 'fs' -import { generateSarif, getKnownParser, getRegexParser, parseAddedLines, isNewIssue, failOnIssues, _testExports, type Parser } from '../src/bugalint' +import { + generateSarif, + getKnownParser, + getRegexParser, + parseDiffLines, + isNewIssue, + isCommentableIssue, + failOnIssues, + _testExports, + type Parser +} from '../src/bugalint' describe('fullConversion', () => { it.each([ @@ -41,24 +51,31 @@ index 1111111..2222222 100644 +import sys +x = 1 ` - const addedLines = parseAddedLines(diff) + const diffLines = parseDiffLines(diff) - it('collects added line numbers per file', () => { - expect(addedLines).toStrictEqual({ 'A/B/test.py': { 2: true, 3: true } }) + it('collects the lines of the diff per file, marking the added ones', () => { + expect(diffLines).toStrictEqual({ 'A/B/test.py': { 1: false, 2: true, 3: true } }) }) - it('accepts issues contained in added lines', () => { - expect(isNewIssue({ path: 'A/B/test.py', line: 2 }, addedLines, '.')).toBe(true) - expect(isNewIssue({ path: 'A/B/test.py', line: 2, eline: 3 }, addedLines, '.')).toBe(true) - expect(isNewIssue({ path: 'test.py', line: 3 }, addedLines, 'A\\B')).toBe(true) + it('accepts issues overlapping an added line', () => { + expect(isNewIssue({ path: 'A/B/test.py', line: 2 }, diffLines, '.')).toBe(true) + expect(isNewIssue({ path: 'A/B/test.py', line: 2, eline: 3 }, diffLines, '.')).toBe(true) + expect(isNewIssue({ path: 'A/B/test.py', line: 1, eline: 2 }, diffLines, '.')).toBe(true) + expect(isNewIssue({ path: 'test.py', line: 3 }, diffLines, 'A\\B')).toBe(true) }) - it('rejects issues not fully contained in added lines', () => { - expect(isNewIssue({ path: 'A/B/test.py', line: 1 }, addedLines, '.')).toBe(false) - expect(isNewIssue({ path: 'A/B/test.py', line: 1, eline: 2 }, addedLines, '.')).toBe(false) - expect(isNewIssue({ path: 'A/B/other.py', line: 2 }, addedLines, '.')).toBe(false) - expect(isNewIssue({ line: 2 }, addedLines, '.')).toBe(false) - expect(isNewIssue({ path: 'A/B/test.py' }, addedLines, '.')).toBe(false) + it('rejects issues not overlapping any added line', () => { + expect(isNewIssue({ path: 'A/B/test.py', line: 1 }, diffLines, '.')).toBe(false) + expect(isNewIssue({ path: 'A/B/other.py', line: 2 }, diffLines, '.')).toBe(false) + expect(isNewIssue({ line: 2 }, diffLines, '.')).toBe(false) + 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) }) describe('failOnIssues', () => { diff --git a/dist/index.js b/dist/index.js index 3f82377..4884614 100644 --- a/dist/index.js +++ b/dist/index.js @@ -29938,8 +29938,9 @@ exports.getKnownParser = getKnownParser; exports.getRegexParser = getRegexParser; exports.addComments = addComments; exports.getPrDiff = getPrDiff; -exports.parseAddedLines = parseAddedLines; +exports.parseDiffLines = parseDiffLines; exports.isNewIssue = isNewIssue; +exports.isCommentableIssue = isCommentableIssue; exports.failOnIssues = failOnIssues; exports.createSummary = createSummary; const github_1 = __nccwpck_require__(3228); @@ -30108,14 +30109,18 @@ async function addComments(issues, prDiff, githubToken, identifier, owner, repo, } } } - const addedLines = parseAddedLines(prDiff); + const diffLines = parseDiffLines(prDiff); const comments = []; for (const issue of issues) { (0, core_1.debug)(`Processing issue on ${issue.path}:${issue.line}`); - if (!isNewIssue(issue, addedLines, analysisPath)) { + if (!isNewIssue(issue, diffLines, analysisPath)) { (0, core_1.debug)(`Skipping issue on ${issue.path}:${issue.line} because it's not in the PR diff`); continue; } + if (!isCommentableIssue(issue, diffLines, analysisPath)) { + (0, core_1.debug)(`Skipping issue on ${issue.path}:${issue.line} because GitHub rejects comments spanning lines outside the PR diff`); + continue; + } if (comments.length >= 50) { (0, core_1.warning)('More than 50 comments detected. Only the first 50 will be posted.'); break; @@ -30144,42 +30149,54 @@ async function getPrDiff(githubToken, owner, repo, prNumber) { const octokit = (0, github_1.getOctokit)(githubToken); return (await octokit.rest.pulls.get({ owner, repo, pull_number: prNumber, mediaType: { format: 'diff' } })).data; } -function parseAddedLines(diff) { - const addedLines = {}; +function parseDiffLines(diff) { + const diffLines = {}; for (const file of (0, parse_diff_1.default)(diff)) { if (file.to == null) { continue; } (0, core_1.debug)(`PR file diff: ${file.to} (${file.chunks.length} chunks)`); - addedLines[file.to] = {}; + diffLines[file.to] = {}; for (const chunk of file.chunks) { for (const change of chunk.changes) { if (change.type === 'add') { - addedLines[file.to][change?.ln] = true; + diffLines[file.to][change.ln] = true; + } + else if (change.type === 'normal') { + diffLines[file.to][change.ln2] = false; } } } } - (0, core_1.debug)(`addedLines: ${JSON.stringify(addedLines)}`); - return addedLines; + (0, core_1.debug)(`diffLines: ${JSON.stringify(diffLines)}`); + return diffLines; +} +function issueLines(line, eline) { + const lines = []; + for (let current = line; current <= (eline ?? line); current++) { + lines.push(current); + } + return lines; } -function isNewIssue(issue, addedLines, analysisPath) { +function isNewIssue(issue, diffLines, analysisPath) { if (issue.path == null || issue.line == null) { return false; } - const normalized = normalizePath(issue.path, analysisPath); - for (let line = issue.line; line <= (issue.eline ?? issue.line); line++) { - if (!addedLines?.[normalized]?.[line]) { - return false; - } + const lines = diffLines[normalizePath(issue.path, analysisPath)]; + return issueLines(issue.line, issue.eline).some((line) => lines?.[line] ?? false); +} +function isCommentableIssue(issue, diffLines, analysisPath) { + if (issue.path == null || issue.line == null) { + return false; } - return true; + const lines = diffLines[normalizePath(issue.path, analysisPath)]; + return issueLines(issue.line, issue.eline).every((line) => lines?.[line] != null); } function failOnIssues(issues, toolName, analysisPath, prDiff) { let failing = [...issues]; if (prDiff != null) { - const addedLines = parseAddedLines(prDiff); - failing = failing.filter((issue) => isNewIssue(issue, addedLines, analysisPath)); + const diffLines = parseDiffLines(prDiff); + failing = failing.filter((issue) => isNewIssue(issue, diffLines, analysisPath)); } if (failing.length > 0) { throw new Error(`${toolName} found ${failing.length} issues`); diff --git a/src/bugalint.ts b/src/bugalint.ts index f6cfb4f..effbb57 100644 --- a/src/bugalint.ts +++ b/src/bugalint.ts @@ -210,15 +210,19 @@ export async function addComments( } } - const addedLines = parseAddedLines(prDiff) + const diffLines = parseDiffLines(prDiff) const comments = [] for (const issue of issues) { debug(`Processing issue on ${issue.path}:${issue.line}`) - if (!isNewIssue(issue, addedLines, analysisPath)) { + if (!isNewIssue(issue, diffLines, analysisPath)) { debug(`Skipping issue on ${issue.path}:${issue.line} because it's not in the PR diff`) continue } + if (!isCommentableIssue(issue, diffLines, analysisPath)) { + debug(`Skipping issue on ${issue.path}:${issue.line} because GitHub rejects comments spanning lines outside the PR diff`) + continue + } if (comments.length >= 50) { warning('More than 50 comments detected. Only the first 50 will be posted.') break @@ -245,51 +249,64 @@ export async function addComments( debug('Sent comments') } -export type AddedLines = Record> +export type DiffLines = Record> export async function getPrDiff(githubToken: string, owner: string, repo: string, prNumber: number): Promise { const octokit = getOctokit(githubToken) return (await octokit.rest.pulls.get({ owner, repo, pull_number: prNumber, mediaType: { format: 'diff' } })).data as unknown as string } -export function parseAddedLines(diff: string): AddedLines { - const addedLines: AddedLines = {} +export function parseDiffLines(diff: string): DiffLines { + const diffLines: DiffLines = {} for (const file of parseDiff(diff)) { if (file.to == null) { continue } debug(`PR file diff: ${file.to} (${file.chunks.length} chunks)`) - addedLines[file.to] = {} + diffLines[file.to] = {} for (const chunk of file.chunks) { for (const change of chunk.changes) { if (change.type === 'add') { - addedLines[file.to][change?.ln] = true + diffLines[file.to][change.ln] = true + } else if (change.type === 'normal') { + diffLines[file.to][change.ln2] = false } } } } - debug(`addedLines: ${JSON.stringify(addedLines)}`) - return addedLines + debug(`diffLines: ${JSON.stringify(diffLines)}`) + return diffLines +} + +function issueLines(line: number, eline?: number): number[] { + const lines: number[] = [] + for (let current = line; current <= (eline ?? line); current++) { + lines.push(current) + } + return lines } -export function isNewIssue(issue: Issue, addedLines: AddedLines, analysisPath: string): issue is Issue & Required> { +export function isNewIssue(issue: Issue, diffLines: DiffLines, analysisPath: string): issue is Issue & Required> { if (issue.path == null || issue.line == null) { return false } - const normalized = normalizePath(issue.path, analysisPath) - for (let line = issue.line; line <= (issue.eline ?? issue.line); line++) { - if (!addedLines?.[normalized]?.[line]) { - return false - } + const lines: Record | undefined = diffLines[normalizePath(issue.path, analysisPath)] + return issueLines(issue.line, issue.eline).some((line) => lines?.[line] ?? false) +} + +export function isCommentableIssue(issue: Issue, diffLines: DiffLines, analysisPath: string): boolean { + if (issue.path == null || issue.line == null) { + return false } - return true + const lines: Record | undefined = diffLines[normalizePath(issue.path, analysisPath)] + return issueLines(issue.line, issue.eline).every((line) => lines?.[line] != null) } export function failOnIssues(issues: Iterable, toolName: string, analysisPath: string, prDiff?: string): void { let failing = [...issues] if (prDiff != null) { - const addedLines = parseAddedLines(prDiff) - failing = failing.filter((issue) => isNewIssue(issue, addedLines, analysisPath)) + const diffLines = parseDiffLines(prDiff) + failing = failing.filter((issue) => isNewIssue(issue, diffLines, analysisPath)) } if (failing.length > 0) { throw new Error(`${toolName} found ${failing.length} issues`) From 99db188176d5c0f18fd9f6c49cd32150b2021ece Mon Sep 17 00:00:00 2001 From: Bugale Date: Sat, 25 Jul 2026 23:42:45 +0300 Subject: [PATCH 03/11] fix: read the deleted region of a SARIF fix A fix is now rendered as a suggestion only when its deleted region covers exactly the lines the comment is anchored to, and the region decides whether the inserted text ends with a line terminator. A region ending at the end of the last line is taken verbatim, while one ending at the beginning of the following line, which is the other usual way of spelling a whole line replacement, has its single trailing newline removed. Trimming unconditionally would have dropped a meaningful trailing empty line, and not trimming at all appended a spurious one. Fixes replacing a part of a line or lines other than the reported ones are ignored rather than rendered, as anchoring them on whole lines would have suggested replacing the whole line with the fragment. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- README.md | 21 ++++++---- __tests__/bugalint.test.ts | 48 +++++++++++++++++++++++ __tests__/sariffix.input.txt | 70 ++++++++++++++++++++++++++++++++++ __tests__/sariffix.output.json | 47 ++++++++++++++++++++--- dist/index.js | 27 ++++++++++--- src/bugalint.ts | 30 ++++++++++++--- 6 files changed, 219 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index ed70abd..897f816 100644 --- a/README.md +++ b/README.md @@ -140,16 +140,23 @@ entry, so they are only available when `inputFormat` is `sarif`: } ``` -GitHub replaces whole lines, which constrains what a fix may contain: +GitHub replaces whole lines, so a fix is rendered only when its `deletedRegion` covers exactly the lines of the result's own region, which is what the comment +is anchored to. Following the SARIF specification, in which an absent `endColumn` means the end of the text of `endLine`, both of the usual ways of writing +such a region are accepted: -- `deletedRegion` must cover whole lines, i.e. specify only `startLine` and `endLine`, and must match the region of the result itself, which is what the comment - is anchored to. +- `{ "startLine": 3, "endLine": 4 }` covers the text of lines 3 to 4 without the line terminator ending line 4, so `insertedContent.text` is the new text of + those lines and must not end with a newline. -- `insertedContent.text` is the exact text replacing those lines, and should not end with a newline. It is never trimmed, so a trailing newline is rendered as a - trailing empty line. +- `{ "startLine": 3, "startColumn": 1, "endLine": 5, "endColumn": 1 }` covers the same lines including the line terminator ending line 4, so + `insertedContent.text` must end with a newline. Exactly one is removed when rendering the suggestion. -- An empty `insertedContent.text` renders as an empty suggestion, which deletes the lines. A replacement consisting of a single empty line is therefore - indistinguishable from a deletion, and cannot be expressed. A producer that needs one should widen the replacement to include a neighbouring line. +Any other `deletedRegion`, such as one replacing a part of a line or lines other than the reported ones, cannot be rendered as a suggestion. Such a fix is +ignored, and the issue is commented on without one. + +The text itself is never trimmed beyond the single line terminator described above, so an additional trailing newline is rendered as a trailing empty line. +An empty `insertedContent.text` renders as an empty suggestion, which deletes the lines. A replacement consisting of a single empty line is written exactly the +same way in either form, so it is indistinguishable from a deletion and cannot be expressed. A producer that needs one should widen the replacement to include +a neighbouring line. ### Example With Custom Regex diff --git a/__tests__/bugalint.test.ts b/__tests__/bugalint.test.ts index c6ad9db..6ebe642 100644 --- a/__tests__/bugalint.test.ts +++ b/__tests__/bugalint.test.ts @@ -1,5 +1,6 @@ import '@microsoft/jest-sarif' import { readFileSync } from 'fs' +import type { Region } from 'sarif' import { generateSarif, getKnownParser, @@ -140,6 +141,53 @@ describe('commentBody', () => { }) }) +describe('sarifFix', () => { + const fixOf = (region: Region, deletedRegion: Region, text: string): string | undefined => { + const log = { + version: '2.1.0', + runs: [ + { + tool: { driver: { name: 'test' } }, + results: [ + { + message: { text: 'Message' }, + locations: [{ physicalLocation: { artifactLocation: { uri: 'test.py' }, region } }], + fixes: [{ artifactChanges: [{ artifactLocation: { uri: 'test.py' }, replacements: [{ deletedRegion, insertedContent: { text } }] }] }] + } + ] + } + ] + } + return [...getKnownParser('sarif')(JSON.stringify(log))][0].fix + } + + it('takes the text of a region ending at the end of the last reported line', () => { + expect(fixOf({ startLine: 3, endLine: 4 }, { startLine: 3, endLine: 4 }, 'a\nb')).toBe('a\nb') + expect(fixOf({ startLine: 3 }, { startLine: 3 }, 'a')).toBe('a') + }) + + it('drops the line terminator of a region ending at the beginning of the following line', () => { + expect(fixOf({ startLine: 3, endLine: 4 }, { startLine: 3, startColumn: 1, endLine: 5, endColumn: 1 }, 'a\nb\n')).toBe('a\nb') + expect(fixOf({ startLine: 3 }, { startLine: 3, startColumn: 1, endLine: 4, endColumn: 1 }, 'a\n')).toBe('a') + }) + + it('keeps a trailing empty line of both forms', () => { + expect(fixOf({ startLine: 3, endLine: 4 }, { startLine: 3, endLine: 4 }, 'a\nb\n')).toBe('a\nb\n') + expect(fixOf({ startLine: 3, endLine: 4 }, { startLine: 3, startColumn: 1, endLine: 5, endColumn: 1 }, 'a\nb\n\n')).toBe('a\nb\n') + }) + + it('ignores a fix replacing a part of a line', () => { + expect(fixOf({ startLine: 3 }, { startLine: 3, startColumn: 5, endLine: 3, endColumn: 7 }, '===')).toBeUndefined() + expect(fixOf({ startLine: 3 }, { startLine: 3, endLine: 3, endColumn: 10 }, 'a')).toBeUndefined() + }) + + it('ignores a fix replacing lines other than the reported ones', () => { + expect(fixOf({ startLine: 3 }, { startLine: 3, endLine: 4 }, 'a')).toBeUndefined() + expect(fixOf({ startLine: 3 }, { startLine: 4 }, 'a')).toBeUndefined() + expect(fixOf({ startLine: 3, endLine: 4 }, { startLine: 3, startColumn: 1, endLine: 4, endColumn: 1 }, 'a\n')).toBeUndefined() + }) +}) + describe('windowsFileUrl', () => { if (process.platform === 'win32') { it('should convert Windows file URLs to relative URLs', () => { diff --git a/__tests__/sariffix.input.txt b/__tests__/sariffix.input.txt index be3a1d9..4d49a00 100644 --- a/__tests__/sariffix.input.txt +++ b/__tests__/sariffix.input.txt @@ -76,6 +76,76 @@ "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "test.py" }, "region": { "startLine": 40, "endLine": 40 } } }], "level": "error", "message": { "text": "No fix available" } + }, + { + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "test.py" }, "region": { "startLine": 50, "endLine": 51 } } }], + "level": "warning", + "message": { "text": "Deleted region including the line terminator" }, + "fixes": [ + { + "artifactChanges": [ + { + "artifactLocation": { "uri": "test.py" }, + "replacements": [ + { + "deletedRegion": { "startLine": 50, "startColumn": 1, "endLine": 52, "endColumn": 1 }, + "insertedContent": { "text": "a = 1\nb = 2\n" } + } + ] + } + ] + } + ] + }, + { + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "test.py" }, "region": { "startLine": 60, "endLine": 60 } } }], + "level": "warning", + "message": { "text": "Replacement of a part of a line" }, + "fixes": [ + { + "artifactChanges": [ + { + "artifactLocation": { "uri": "test.py" }, + "replacements": [ + { + "deletedRegion": { "startLine": 60, "startColumn": 5, "endLine": 60, "endColumn": 7 }, + "insertedContent": { "text": "===" } + } + ] + } + ] + } + ] + }, + { + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "test.py" }, "region": { "startLine": 70, "endLine": 70 } } }], + "level": "warning", + "message": { "text": "Replacement of lines other than the reported ones" }, + "fixes": [ + { + "artifactChanges": [ + { + "artifactLocation": { "uri": "test.py" }, + "replacements": [{ "deletedRegion": { "startLine": 70, "endLine": 71 }, "insertedContent": { "text": "y = 2" } }] + } + ] + } + ] + }, + { + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "test.py" }, "region": { "startLine": 80, "endLine": 80 } } }], + "level": "warning", + "message": { "text": "Replacement ending with an empty line" }, + "fixes": [ + { + "artifactChanges": [ + { + "artifactLocation": { "uri": "test.py" }, + "replacements": [{ "deletedRegion": { "startLine": 80, "endLine": 80 }, "insertedContent": { "text": "z = 3\n" } }] + } + ] + } + ] } ] } diff --git a/__tests__/sariffix.output.json b/__tests__/sariffix.output.json index 4756af2..1f34891 100644 --- a/__tests__/sariffix.output.json +++ b/__tests__/sariffix.output.json @@ -3,12 +3,7 @@ "$schema": "http://json.schemastore.org/sarif-2.1.0-rtm.6", "runs": [ { - "tool": { - "driver": { - "name": "test", - "rules": [{ "id": "F001", "name": "formatting" }] - } - }, + "tool": { "driver": { "name": "test", "rules": [{ "id": "F001", "name": "formatting" }] } }, "results": [ { "message": { "text": "Single line replacement" }, @@ -76,6 +71,46 @@ "message": { "text": "No fix available" }, "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "test.py" }, "region": { "startLine": 40, "endLine": 40 } } }], "level": "error" + }, + { + "message": { "text": "Deleted region including the line terminator" }, + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "test.py" }, "region": { "startLine": 50, "endLine": 51 } } }], + "fixes": [ + { + "artifactChanges": [ + { + "artifactLocation": { "uri": "test.py" }, + "replacements": [{ "deletedRegion": { "startLine": 50, "endLine": 51 }, "insertedContent": { "text": "a = 1\nb = 2" } }] + } + ] + } + ], + "level": "warning" + }, + { + "message": { "text": "Replacement of a part of a line" }, + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "test.py" }, "region": { "startLine": 60, "endLine": 60 } } }], + "level": "warning" + }, + { + "message": { "text": "Replacement of lines other than the reported ones" }, + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "test.py" }, "region": { "startLine": 70, "endLine": 70 } } }], + "level": "warning" + }, + { + "message": { "text": "Replacement ending with an empty line" }, + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "test.py" }, "region": { "startLine": 80, "endLine": 80 } } }], + "fixes": [ + { + "artifactChanges": [ + { + "artifactLocation": { "uri": "test.py" }, + "replacements": [{ "deletedRegion": { "startLine": 80, "endLine": 80 }, "insertedContent": { "text": "z = 3\n" } }] + } + ] + } + ], + "level": "warning" } ] } diff --git a/dist/index.js b/dist/index.js index 4884614..92bdb09 100644 --- a/dist/index.js +++ b/dist/index.js @@ -29985,6 +29985,22 @@ function* parsePylint(input) { }; } } +function parseSarifFix(result, region) { + const replacement = result.fixes?.[0]?.artifactChanges?.[0]?.replacements?.[0]; + const text = replacement?.insertedContent?.text; + if (replacement == null || text == null || region?.startLine == null) { + return undefined; + } + const deleted = replacement.deletedRegion; + const endLine = region.endLine ?? region.startLine; + if (deleted.startLine !== region.startLine || (deleted.startColumn ?? 1) !== 1) { + return undefined; + } + if (deleted.endColumn == null) { + return (deleted.endLine ?? deleted.startLine) === endLine ? text : undefined; + } + return deleted.endColumn === 1 && deleted.endLine === endLine + 1 ? text.replace(/\n$/, '') : undefined; +} function* parseSarif(input) { const log = JSON.parse(input); for (const run of log.runs) { @@ -29992,17 +30008,18 @@ function* parseSarif(input) { continue; } for (const issue of run.results) { + const region = issue.locations?.[0]?.physicalLocation?.region; yield { id: issue.ruleId, sym: issue.ruleIndex != null ? run.tool.driver.rules?.[issue.ruleIndex]?.name : undefined, msg: issue.message.text, level: issue.level, path: issue.locations?.[0]?.physicalLocation?.artifactLocation?.uri, - line: issue.locations?.[0]?.physicalLocation?.region?.startLine, - col: issue.locations?.[0]?.physicalLocation?.region?.startColumn, - eline: issue.locations?.[0]?.physicalLocation?.region?.endLine, - ecol: issue.locations?.[0]?.physicalLocation?.region?.endColumn, - fix: issue.fixes?.[0]?.artifactChanges?.[0]?.replacements?.[0]?.insertedContent?.text + line: region?.startLine, + col: region?.startColumn, + eline: region?.endLine, + ecol: region?.endColumn, + fix: parseSarifFix(issue, region) }; } } diff --git a/src/bugalint.ts b/src/bugalint.ts index effbb57..736ef48 100644 --- a/src/bugalint.ts +++ b/src/bugalint.ts @@ -1,4 +1,4 @@ -import type { Log, ReportingDescriptor, Result } from 'sarif' +import type { Log, Region, ReportingDescriptor, Result } from 'sarif' import { getOctokit } from '@actions/github' import { debug, warning, summary } from '@actions/core' import path from 'path' @@ -60,6 +60,23 @@ function* parsePylint(input: string): Generator { } } +function parseSarifFix(result: Result, region?: Region): string | undefined { + const replacement = result.fixes?.[0]?.artifactChanges?.[0]?.replacements?.[0] + const text = replacement?.insertedContent?.text + if (replacement == null || text == null || region?.startLine == null) { + return undefined + } + const deleted = replacement.deletedRegion + const endLine = region.endLine ?? region.startLine + if (deleted.startLine !== region.startLine || (deleted.startColumn ?? 1) !== 1) { + return undefined + } + if (deleted.endColumn == null) { + return (deleted.endLine ?? deleted.startLine) === endLine ? text : undefined + } + return deleted.endColumn === 1 && deleted.endLine === endLine + 1 ? text.replace(/\n$/, '') : undefined +} + function* parseSarif(input: string): Generator { const log: Log = JSON.parse(input) for (const run of log.runs) { @@ -67,17 +84,18 @@ function* parseSarif(input: string): Generator { continue } for (const issue of run.results) { + const region = issue.locations?.[0]?.physicalLocation?.region yield { id: issue.ruleId, sym: issue.ruleIndex != null ? run.tool.driver.rules?.[issue.ruleIndex]?.name : undefined, msg: issue.message.text, level: issue.level, path: issue.locations?.[0]?.physicalLocation?.artifactLocation?.uri, - line: issue.locations?.[0]?.physicalLocation?.region?.startLine, - col: issue.locations?.[0]?.physicalLocation?.region?.startColumn, - eline: issue.locations?.[0]?.physicalLocation?.region?.endLine, - ecol: issue.locations?.[0]?.physicalLocation?.region?.endColumn, - fix: issue.fixes?.[0]?.artifactChanges?.[0]?.replacements?.[0]?.insertedContent?.text + line: region?.startLine, + col: region?.startColumn, + eline: region?.endLine, + ecol: region?.endColumn, + fix: parseSarifFix(issue, region) } } } From 22000274d258d6a16d6e2f89e4210781e67b013e Mon Sep 17 00:00:00 2001 From: Bugale Date: Sun, 26 Jul 2026 20:58:34 +0300 Subject: [PATCH 04/11] feat: support converting a formatter diff to suggestions Adds a `diff` input format that turns the output of `git diff` into issues carrying whole-line fixes, which makes any formatter that can rewrite files in place a linter reporting suggested changes. Each contiguous run of changed lines becomes one issue rather than each hunk, so the context lines `git diff` prints do not widen the reported range, and issues are anchored on the lines of the old side of the diff, which are the ones the pull request shows and that comments can be attached to. A run that only adds lines is extended to a neighbouring line, preferring the preceding one. Adds a `message` input for the text of the issues of a format that carries none. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/workflows/check-code.yml | 1 + README.md | 36 +++++++++- __tests__/bugalint.test.ts | 67 ++++++++++++++++++ __tests__/diff.input.txt | 72 +++++++++++++++++++ __tests__/diff.output.json | 118 +++++++++++++++++++++++++++++++ action.yml | 4 ++ dist/index.js | 82 +++++++++++++++++++-- src/bugalint.ts | 66 +++++++++++++++++ src/index.ts | 21 ++++-- 9 files changed, 454 insertions(+), 13 deletions(-) create mode 100644 __tests__/diff.input.txt create mode 100644 __tests__/diff.output.json diff --git a/.github/workflows/check-code.yml b/.github/workflows/check-code.yml index e0baddd..cbe0689 100644 --- a/.github/workflows/check-code.yml +++ b/.github/workflows/check-code.yml @@ -81,6 +81,7 @@ jobs: - {linter: 'ghalint', format: 'ghalint', regex: '', levelMap: '', analysisPath: '.'} - {linter: 'sarif', format: 'sarif', regex: '', levelMap: '', analysisPath: '.'} - {linter: 'sariffix', format: 'sarif', regex: '', levelMap: '', analysisPath: '.'} + - {linter: 'diff', format: 'diff', regex: '', levelMap: '', analysisPath: '.'} - {linter: 'flake8subpath', format: 'flake8', regex: '', levelMap: '', analysisPath: 'A\B'} - {linter: 'noissues', format: 'flake8', regex: '', levelMap: '', analysisPath: '.', outcome: 'success'} - {linter: 'noissues', format: 'flake8', regex: '', levelMap: '', analysisPath: '.', fail: 'false', outcome: 'success', name: 'noissues-nofail'} diff --git a/README.md b/README.md index 897f816..cecea92 100644 --- a/README.md +++ b/README.md @@ -67,6 +67,8 @@ steps: root. This is required only when the linter's output contains paths that are relative but not to the repository's root, for which this action will re-relativize them. +- `message`: The message of the issues found by an input format that does not carry one, currently only `diff`. Defaults to `Not formatted correctly`. + - `githubToken`: Relevant only for "comment" mode. The GitHub token to use to post the comment. If not specified, the action will use the action's token. #### Natively Supported Linter Output Formats @@ -88,7 +90,35 @@ This action supports a bunch of linter output formats, for which no `inputRegex` - `ghalint`: The format of [ghallint](https://github.com/suzuki-shunsuke/ghalint/cmd/ghalint/) linter's parsable output. - `SARIF`: A [standard format for static analysis](https://sarifweb.azurewebsites.net/). This is useful if you already have a SARIF file and want to create a summary - for it, or create comments on the PR. This is also the only input format that can carry [suggested changes](#suggested-changes). + for it, or create comments on the PR. It can carry [suggested changes](#suggested-changes). + +- `diff`: The output of `git diff`, which turns [any formatter that rewrites files in place](#formatter-diffs) into a linter reporting suggested changes. + +#### Formatter Diffs + +The `diff` input format turns the output of `git diff` into issues, which makes any formatter that can rewrite files in place a linter reporting +[suggested changes](#suggested-changes): + +```yaml +- run: clang-format -i $(git ls-files '*.cpp') +- run: git diff > clang-format.diff +- uses: bugale/bugalint@v1 + with: + inputFile: 'clang-format.diff' + toolName: 'clang-format' + inputFormat: 'diff' + message: 'Not formatted according to .clang-format' + comment: true +``` + +Every contiguous run of changed lines becomes one issue, rather than every hunk, so the context lines `git diff` prints around each change do not widen the +reported range. Issues are anchored on the lines of the old side of the diff, which are the lines of the committed file that the pull request shows and that +comments can be attached to, while the new side becomes the fix. A run that only adds lines has no line of its own to anchor to, so it is extended to a +neighbouring line, preferring the preceding one, whose content is repeated in the fix. The marker `git diff` prints for a file that does not end with a newline +is ignored, so the last line of such a file is reported like any other. + +Note that a formatter that fails without writing anything produces an empty diff, which is indistinguishable from a formatter that found nothing to fix. The +step running the formatter should therefore fail the job by itself. #### Input Regex Named Groups @@ -120,8 +150,8 @@ The supported named groups are: When an issue carries a fix, the comment posted on the pull request contains it as a [suggested change](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request), -which a reviewer can apply in one click. Fixes are read from the first `replacements` entry of the first `artifactChanges` entry of the result's first `fixes` -entry, so they are only available when `inputFormat` is `sarif`: +which a reviewer can apply in one click. Fixes are produced by the [`diff` input format](#formatter-diffs), and are read from the first `replacements` entry of +the first `artifactChanges` entry of the result's first `fixes` entry when `inputFormat` is `sarif`: ```json { diff --git a/__tests__/bugalint.test.ts b/__tests__/bugalint.test.ts index 6ebe642..7e41945 100644 --- a/__tests__/bugalint.test.ts +++ b/__tests__/bugalint.test.ts @@ -5,6 +5,7 @@ import { generateSarif, getKnownParser, getRegexParser, + getDiffParser, parseDiffLines, isNewIssue, isCommentableIssue, @@ -23,6 +24,7 @@ describe('fullConversion', () => { ['ghalint', getKnownParser('ghalint'), '.'], ['sarif', getKnownParser('sarif'), '.'], ['sariffix', getKnownParser('sarif'), '.'], + ['diff', getKnownParser('diff'), '.'], ['flake8subpath', getKnownParser('flake8'), 'A\\B'], ['noissues', getKnownParser('flake8'), '.'], [ @@ -188,6 +190,71 @@ describe('sarifFix', () => { }) }) +describe('diffFormat', () => { + const diffOf = (lines: string[], terminator = '\n'): string => lines.map((line) => `${line}${terminator}`).join('') + const header = ['diff --git a/a.c b/a.c', 'index 1111111..2222222 100644', '--- a/a.c', '+++ b/a.c', '@@ -1,1 +1,1 @@'] + const firstIssue = (lines: string[], terminator = '\n'): unknown => [...getKnownParser('diff')(diffOf(lines, terminator))][0] + + it('uses the configured message, falling back to a generic one', () => { + const input = diffOf([...header, '-int a=0;', '+int a = 0;']) + expect([...getDiffParser('Run clang-format')(input)][0].msg).toBe('Run clang-format') + expect([...getDiffParser('')(input)][0].msg).toBe('Not formatted correctly') + expect([...getKnownParser('diff')(input)][0].msg).toBe('Not formatted correctly') + }) + + it('anchors on the lines of the old side of the diff', () => { + const lines = [ + 'diff --git a/a.c b/a.c', + '--- a/a.c', + '+++ b/a.c', + '@@ -10,4 +2,3 @@', + ' int a = 0;', + '-int b=1;', + '-int c=2;', + '+int b = 1, c = 2;', + ' return a;' + ] + expect(firstIssue(lines)).toMatchObject({ path: 'a.c', line: 11, eline: 12, fix: 'int b = 1, c = 2;' }) + }) + + it('reports a deletion as an empty fix', () => { + expect(firstIssue(['diff --git a/a.c b/a.c', '--- a/a.c', '+++ b/a.c', '@@ -5,3 +5,2 @@', ' int a = 0;', '-', ' return a;'])).toMatchObject({ + line: 6, + eline: 6, + fix: '' + }) + }) + + it('extends an insertion to the preceding line', () => { + expect(firstIssue(['diff --git a/a.c b/a.c', '--- a/a.c', '+++ b/a.c', '@@ -4,2 +4,3 @@', ' int a = 0;', '+int b = 1;', ' return a;'])).toMatchObject({ + line: 4, + eline: 4, + fix: 'int a = 0;\nint b = 1;' + }) + }) + + it('extends an insertion at the top of a file to the following line', () => { + expect(firstIssue(['diff --git a/a.c b/a.c', '--- a/a.c', '+++ b/a.c', '@@ -1,2 +1,3 @@', '+// header', ' int a = 0;', ' return a;'])).toMatchObject({ + line: 1, + eline: 1, + fix: '// header\nint a = 0;' + }) + }) + + it('ignores the marker of a file not ending with a newline', () => { + expect(firstIssue([...header, '-int a=0;', '\\ No newline at end of file', '+int a = 0;'])).toMatchObject({ line: 1, eline: 1, fix: 'int a = 0;' }) + }) + + it('keeps carriage returns that are part of the content', () => { + expect(firstIssue([...header, '-int a=0;\r', '+int a = 0;\r'])).toMatchObject({ fix: 'int a = 0;\r' }) + }) + + it('strips the carriage returns of a diff whose own lines are terminated by them', () => { + expect(firstIssue([...header, '-int a=0;', '+int a = 0;'], '\r\n')).toMatchObject({ fix: 'int a = 0;' }) + expect(firstIssue([...header, '-int a=0;\r', '+int a = 0;\r'], '\r\n')).toMatchObject({ fix: 'int a = 0;\r' }) + }) +}) + describe('windowsFileUrl', () => { if (process.platform === 'win32') { it('should convert Windows file URLs to relative URLs', () => { diff --git a/__tests__/diff.input.txt b/__tests__/diff.input.txt new file mode 100644 index 0000000..575b471 --- /dev/null +++ b/__tests__/diff.input.txt @@ -0,0 +1,72 @@ +diff --git a/src/single.c b/src/single.c +index 1111111..2222222 100644 +--- a/src/single.c ++++ b/src/single.c +@@ -1,5 +1,5 @@ + int main() { + int a = 0; +- int b=1; ++ int b = 1; + return a + b; + } +diff --git a/src/multi.c b/src/multi.c +index 3333333..4444444 100644 +--- a/src/multi.c ++++ b/src/multi.c +@@ -8,7 +8,6 @@ void g(void) { + int x = 0; +- if (x) +- { +- f( ); +- } ++ if (x) { ++ f(); ++ } + return; + } +diff --git a/src/deletion.c b/src/deletion.c +index 5555555..6666666 100644 +--- a/src/deletion.c ++++ b/src/deletion.c +@@ -18,5 +18,3 @@ void h(void) { + int y = 0; +- +- + return; + } +diff --git a/src/insertion.c b/src/insertion.c +index 7777777..8888888 100644 +--- a/src/insertion.c ++++ b/src/insertion.c +@@ -30,4 +30,5 @@ void i(void) { + int z = 0; + int w = 1; ++ + return; + } +diff --git a/src/top.c b/src/top.c +index 9999999..aaaaaaa 100644 +--- a/src/top.c ++++ b/src/top.c +@@ -1,3 +1,4 @@ ++// clang-format off + #include + #include + int main(void) { +diff --git a/src/fence.c b/src/fence.c +index bbbbbbb..ccccccc 100644 +--- a/src/fence.c ++++ b/src/fence.c +@@ -40,3 +40,3 @@ void j(void) { +- const char* doc = "```"; ++ const char *doc = "```"; + return; + } +diff --git a/src/eof.c b/src/eof.c +index ddddddd..eeeeeee 100644 +--- a/src/eof.c ++++ b/src/eof.c +@@ -50,1 +50,1 @@ void k(void) { +- int last=0; +\ No newline at end of file ++ int last = 0; diff --git a/__tests__/diff.output.json b/__tests__/diff.output.json new file mode 100644 index 0000000..8c10c27 --- /dev/null +++ b/__tests__/diff.output.json @@ -0,0 +1,118 @@ +{ + "version": "2.1.0", + "$schema": "http://json.schemastore.org/sarif-2.1.0-rtm.6", + "runs": [ + { + "tool": { "driver": { "name": "test", "rules": [] } }, + "results": [ + { + "message": { "text": "Not formatted correctly" }, + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "src/single.c" }, "region": { "startLine": 3, "endLine": 3 } } }], + "fixes": [ + { + "artifactChanges": [ + { + "artifactLocation": { "uri": "src/single.c" }, + "replacements": [{ "deletedRegion": { "startLine": 3, "endLine": 3 }, "insertedContent": { "text": " int b = 1;" } }] + } + ] + } + ], + "level": "warning" + }, + { + "message": { "text": "Not formatted correctly" }, + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "src/multi.c" }, "region": { "startLine": 9, "endLine": 12 } } }], + "fixes": [ + { + "artifactChanges": [ + { + "artifactLocation": { "uri": "src/multi.c" }, + "replacements": [{ "deletedRegion": { "startLine": 9, "endLine": 12 }, "insertedContent": { "text": " if (x) {\n f();\n }" } }] + } + ] + } + ], + "level": "warning" + }, + { + "message": { "text": "Not formatted correctly" }, + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "src/deletion.c" }, "region": { "startLine": 19, "endLine": 20 } } }], + "fixes": [ + { + "artifactChanges": [ + { + "artifactLocation": { "uri": "src/deletion.c" }, + "replacements": [{ "deletedRegion": { "startLine": 19, "endLine": 20 }, "insertedContent": { "text": "" } }] + } + ] + } + ], + "level": "warning" + }, + { + "message": { "text": "Not formatted correctly" }, + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "src/insertion.c" }, "region": { "startLine": 31, "endLine": 31 } } }], + "fixes": [ + { + "artifactChanges": [ + { + "artifactLocation": { "uri": "src/insertion.c" }, + "replacements": [{ "deletedRegion": { "startLine": 31, "endLine": 31 }, "insertedContent": { "text": " int w = 1;\n" } }] + } + ] + } + ], + "level": "warning" + }, + { + "message": { "text": "Not formatted correctly" }, + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "src/top.c" }, "region": { "startLine": 1, "endLine": 1 } } }], + "fixes": [ + { + "artifactChanges": [ + { + "artifactLocation": { "uri": "src/top.c" }, + "replacements": [ + { "deletedRegion": { "startLine": 1, "endLine": 1 }, "insertedContent": { "text": "// clang-format off\n#include " } } + ] + } + ] + } + ], + "level": "warning" + }, + { + "message": { "text": "Not formatted correctly" }, + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "src/fence.c" }, "region": { "startLine": 40, "endLine": 40 } } }], + "fixes": [ + { + "artifactChanges": [ + { + "artifactLocation": { "uri": "src/fence.c" }, + "replacements": [{ "deletedRegion": { "startLine": 40, "endLine": 40 }, "insertedContent": { "text": " const char *doc = \"```\";" } }] + } + ] + } + ], + "level": "warning" + }, + { + "message": { "text": "Not formatted correctly" }, + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "src/eof.c" }, "region": { "startLine": 50, "endLine": 50 } } }], + "fixes": [ + { + "artifactChanges": [ + { + "artifactLocation": { "uri": "src/eof.c" }, + "replacements": [{ "deletedRegion": { "startLine": 50, "endLine": 50 }, "insertedContent": { "text": " int last = 0;" } }] + } + ] + } + ], + "level": "warning" + } + ] + } + ] +} diff --git a/action.yml b/action.yml index bbe0ea4..6884a33 100644 --- a/action.yml +++ b/action.yml @@ -44,6 +44,10 @@ inputs: description: "The path at which the analysis took place relative to the repository's root. Used to relativize any paths to the repository root path." required: false default: '.' + message: + description: 'Message of the issues found by formats that do not carry one, such as diff' + required: false + default: 'Not formatted correctly' githubToken: description: 'Github token of the repository (automatically created by Github)' default: ${{ github.token }} diff --git a/dist/index.js b/dist/index.js index 92bdb09..d932ee0 100644 --- a/dist/index.js +++ b/dist/index.js @@ -29936,6 +29936,7 @@ exports._testExports = void 0; exports.generateSarif = generateSarif; exports.getKnownParser = getKnownParser; exports.getRegexParser = getRegexParser; +exports.getDiffParser = getDiffParser; exports.addComments = addComments; exports.getPrDiff = getPrDiff; exports.parseDiffLines = parseDiffLines; @@ -30024,9 +30025,70 @@ function* parseSarif(input) { } } } +const defaultDiffMessage = 'Not formatted correctly'; +function normalizeDiffLineEndings(diff) { + return /^(?:diff --git |@@ ).*\r$/m.test(diff) ? diff.replace(/\r\n/g, '\n') : diff; +} +function normalDiffLine(change) { + return change?.type === 'normal' ? change : undefined; +} +function* parseFormatDiff(input, message) { + for (const file of (0, parse_diff_1.default)(normalizeDiffLineEndings(input))) { + const filePath = file.from ?? file.to; + if (filePath == null) { + continue; + } + for (const chunk of file.chunks) { + const changes = chunk.changes.filter((change) => !change.content.startsWith('\\')); + let index = 0; + while (index < changes.length) { + const start = index; + const deleted = []; + const inserted = []; + while (index < changes.length) { + const change = changes[index]; + if (change.type !== 'del') { + break; + } + deleted.push(change.ln); + index++; + } + while (index < changes.length) { + const change = changes[index]; + if (change.type !== 'add') { + break; + } + inserted.push(change.content.slice(1)); + index++; + } + let location; + if (deleted.length > 0) { + location = { line: deleted[0], eline: deleted[deleted.length - 1], fix: inserted.join('\n') }; + } + else if (inserted.length > 0) { + const before = normalDiffLine(changes[start - 1]); + const after = normalDiffLine(changes[index]); + if (before != null) { + location = { line: before.ln1, eline: before.ln1, fix: [before.content.slice(1), ...inserted].join('\n') }; + } + else if (after != null) { + location = { line: after.ln1, eline: after.ln1, fix: [...inserted, after.content.slice(1)].join('\n') }; + } + } + else { + index++; + } + if (location != null) { + yield { msg: message, level: 'warning', path: filePath, ...location }; + } + } + } + } +} const knownParsers = { pylint: parsePylint, sarif: parseSarif, + diff: (input) => parseFormatDiff(input, defaultDiffMessage), mypy: (input) => parseRegex(input, /^(?[^:\n]+):(?:(?\d+):)?(?:(?\d+):)?(?:(?\d+):)?(?:(?\d+):)? (?[^:\s]+): (?.+?)\s*(?:\[(?\S+)\])?$/gm), flake8: (input) => parseRegex(input, /^(?[^:\n]+):(?\d+):(?\d+): (?\w\d+) (?[^\n]+)$/gm), mdl: (input) => parseRegex(input, /^(?[^:\n]+)(?::(?\d+))?(?::(?\d+))? (?[^/\n]+)\/(?[^\s]+) (?[^\n]+)$/gm), @@ -30105,6 +30167,9 @@ function getKnownParser(identifier) { function getRegexParser(regex, levelMap) { return (input) => parseRegex(input, regex, levelMap); } +function getDiffParser(message) { + return (input) => parseFormatDiff(input, message === '' ? defaultDiffMessage : message); +} function buildCommentBody(commentTag, identifier, issue) { const body = `${commentTag}\n**${issue.msg}**\n[${[issue.level, identifier, issue.id, issue.sym].filter((n) => n).join(':')}]`; if (issue.fix == null) { @@ -32175,6 +32240,15 @@ const fs_1 = __nccwpck_require__(9896); const github_1 = __nccwpck_require__(3228); const core_1 = __nccwpck_require__(7484); const bugalint_1 = __nccwpck_require__(8983); +function getParser(inputFormat, inputRegex, levelMap, message) { + if (inputFormat === '') { + return (0, bugalint_1.getRegexParser)(new RegExp(inputRegex, 'gm'), levelMap === '' ? undefined : JSON.parse(levelMap)); + } + if (inputFormat === 'diff') { + return (0, bugalint_1.getDiffParser)(message); + } + return (0, bugalint_1.getKnownParser)(inputFormat); +} async function run() { try { const inputFile = (0, core_1.getInput)('inputFile'); @@ -32189,10 +32263,10 @@ async function run() { const levelMap = (0, core_1.getInput)('levelMap'); const analysisPath = (0, core_1.getInput)('analysisPath'); const githubToken = (0, core_1.getInput)('githubToken'); - const parser = inputFormat === '' - ? (0, bugalint_1.getRegexParser)(new RegExp(inputRegex, 'gm'), levelMap === '' ? undefined : JSON.parse(levelMap)) - : (0, bugalint_1.getKnownParser)(inputFormat); - const input = (0, fs_1.readFileSync)(inputFile, 'utf-8').replace(/\r/g, ''); + const message = (0, core_1.getInput)('message'); + const parser = getParser(inputFormat, inputRegex, levelMap, message); + const raw = (0, fs_1.readFileSync)(inputFile, 'utf-8'); + const input = inputFormat === 'diff' ? raw : raw.replace(/\r/g, ''); (0, core_1.debug)(`input: ${input}`); const output = (0, bugalint_1.generateSarif)(parser(input), toolName, analysisPath); (0, core_1.info)(`SARIF output: ${JSON.stringify(output, null, 2)}`); diff --git a/src/bugalint.ts b/src/bugalint.ts index 736ef48..5c7698b 100644 --- a/src/bugalint.ts +++ b/src/bugalint.ts @@ -101,9 +101,71 @@ function* parseSarif(input: string): Generator { } } +const defaultDiffMessage = 'Not formatted correctly' + +function normalizeDiffLineEndings(diff: string): string { + return /^(?:diff --git |@@ ).*\r$/m.test(diff) ? diff.replace(/\r\n/g, '\n') : diff +} + +function normalDiffLine(change?: parseDiff.Change): parseDiff.NormalChange | undefined { + return change?.type === 'normal' ? change : undefined +} + +function* parseFormatDiff(input: string, message: string): Generator { + for (const file of parseDiff(normalizeDiffLineEndings(input))) { + const filePath = file.from ?? file.to + if (filePath == null) { + continue + } + for (const chunk of file.chunks) { + const changes = chunk.changes.filter((change) => !change.content.startsWith('\\')) + let index = 0 + while (index < changes.length) { + const start = index + const deleted: number[] = [] + const inserted: string[] = [] + while (index < changes.length) { + const change = changes[index] + if (change.type !== 'del') { + break + } + deleted.push(change.ln) + index++ + } + while (index < changes.length) { + const change = changes[index] + if (change.type !== 'add') { + break + } + inserted.push(change.content.slice(1)) + index++ + } + let location: Pick | undefined + if (deleted.length > 0) { + location = { line: deleted[0], eline: deleted[deleted.length - 1], fix: inserted.join('\n') } + } else if (inserted.length > 0) { + const before = normalDiffLine(changes[start - 1]) + const after = normalDiffLine(changes[index]) + if (before != null) { + location = { line: before.ln1, eline: before.ln1, fix: [before.content.slice(1), ...inserted].join('\n') } + } else if (after != null) { + location = { line: after.ln1, eline: after.ln1, fix: [...inserted, after.content.slice(1)].join('\n') } + } + } else { + index++ + } + if (location != null) { + yield { msg: message, level: 'warning', path: filePath, ...location } + } + } + } + } +} + const knownParsers: Record = { pylint: parsePylint, sarif: parseSarif, + diff: (input: string) => parseFormatDiff(input, defaultDiffMessage), mypy: (input: string) => parseRegex( input, @@ -195,6 +257,10 @@ export function getRegexParser(regex: RegExp, levelMap?: Record parseRegex(input, regex, levelMap) } +export function getDiffParser(message: string): Parser { + return (input: string) => parseFormatDiff(input, message === '' ? defaultDiffMessage : message) +} + function buildCommentBody(commentTag: string, identifier: string, issue: Issue): string { const body = `${commentTag}\n**${issue.msg}**\n[${[issue.level, identifier, issue.id, issue.sym].filter((n) => n).join(':')}]` if (issue.fix == null) { diff --git a/src/index.ts b/src/index.ts index 355b8ad..37b0291 100644 --- a/src/index.ts +++ b/src/index.ts @@ -2,7 +2,17 @@ import { readFileSync, writeFileSync } from 'fs' import type { Result } from 'sarif' import { context } from '@actions/github' import { getInput, getBooleanInput, debug, info, setFailed } from '@actions/core' -import { generateSarif, getKnownParser, getRegexParser, getPrDiff, addComments, createSummary, failOnIssues, type Parser } from '../src/bugalint' +import { generateSarif, getKnownParser, getRegexParser, getDiffParser, getPrDiff, addComments, createSummary, failOnIssues, type Parser } from '../src/bugalint' + +function getParser(inputFormat: string, inputRegex: string, levelMap: string, message: string): Parser { + if (inputFormat === '') { + return getRegexParser(new RegExp(inputRegex, 'gm'), levelMap === '' ? undefined : (JSON.parse(levelMap) as Record)) + } + if (inputFormat === 'diff') { + return getDiffParser(message) + } + return getKnownParser(inputFormat) +} export async function run(): Promise { try { @@ -18,12 +28,11 @@ export async function run(): Promise { const levelMap: string = getInput('levelMap') const analysisPath: string = getInput('analysisPath') const githubToken: string = getInput('githubToken') + const message: string = getInput('message') - const parser: Parser = - inputFormat === '' - ? getRegexParser(new RegExp(inputRegex, 'gm'), levelMap === '' ? undefined : (JSON.parse(levelMap) as Record)) - : getKnownParser(inputFormat) - const input = readFileSync(inputFile, 'utf-8').replace(/\r/g, '') + const parser: Parser = getParser(inputFormat, inputRegex, levelMap, message) + const raw = readFileSync(inputFile, 'utf-8') + const input = inputFormat === 'diff' ? raw : raw.replace(/\r/g, '') debug(`input: ${input}`) const output = generateSarif(parser(input), toolName, analysisPath) info(`SARIF output: ${JSON.stringify(output, null, 2)}`) From f61831a45569aa462a7248537a630638ff664632 Mon Sep 17 00:00:00 2001 From: Bugale Date: Sun, 26 Jul 2026 23:09:45 +0300 Subject: [PATCH 05/11] feat!: filter out old issues before generating any output `failOnlyNew` only narrowed the failure, so a run with it set still wrote every issue to the SARIF, the log, the summary and the pull request comments, and only the step's exit code reflected the filtering. Rename it to `onlyNew` and apply it once, up front, so every consumer of the issues sees the same filtered set. The pull request diff is now fetched at most once per run and the input is parsed once instead of four times. BREAKING CHANGE: the `failOnlyNew` input is renamed to `onlyNew` and no longer affects only the failure. It now also removes the old issues from the SARIF output, the log, the summary and the comments, so uploading that SARIF to code scanning resolves the alerts of the unchanged code. Leave `onlyNew` unset to keep the previous output. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/workflows/check-code.yml | 7 +++--- README.md | 10 +++++--- __tests__/bugalint.test.ts | 27 ++++++++++++-------- action.yml | 4 +-- dist/index.js | 41 +++++++++++++++++------------- src/bugalint.ts | 13 +++++----- src/index.ts | 43 +++++++++++++++++++++++--------- 7 files changed, 91 insertions(+), 54 deletions(-) diff --git a/.github/workflows/check-code.yml b/.github/workflows/check-code.yml index cbe0689..f7cb0a4 100644 --- a/.github/workflows/check-code.yml +++ b/.github/workflows/check-code.yml @@ -86,7 +86,8 @@ jobs: - {linter: 'noissues', format: 'flake8', regex: '', levelMap: '', analysisPath: '.', outcome: 'success'} - {linter: 'noissues', format: 'flake8', regex: '', levelMap: '', analysisPath: '.', fail: 'false', outcome: 'success', name: 'noissues-nofail'} - {linter: 'pylint', format: 'pylint', regex: '', levelMap: '', analysisPath: '.', fail: 'false', outcome: 'success', name: 'pylint-nofail'} - - {linter: 'pylint', format: 'pylint', regex: '', levelMap: '', analysisPath: '.', failOnlyNew: 'true', outcome: 'success', name: 'pylint-onlynew'} + # yamllint disable-line rule:line-length + - {linter: 'pylint', format: 'pylint', regex: '', levelMap: '', analysisPath: '.', onlyNew: 'true', output: 'noissues', outcome: 'success', name: 'pylint-onlynew'} - linter: 'custom' format: '' # yamllint disable-line rule:line-length @@ -111,7 +112,7 @@ jobs: levelMap: ${{ matrix.run.levelMap }} analysisPath: ${{ matrix.run.analysisPath }} fail: ${{ matrix.run.fail || 'true' }} - failOnlyNew: ${{ matrix.run.failOnlyNew || 'false' }} + onlyNew: ${{ matrix.run.onlyNew || 'false' }} - name: Test Outcome run: | if [ "${{ steps.run.outcome }}" != "${{ matrix.run.outcome || 'failure' }}" ]; @@ -120,7 +121,7 @@ jobs: exit 1 fi - name: Create Diff - run: json-diff "__tests__/${{ matrix.run.linter }}.output.json" "sarif.json" | tee diff.txt + run: json-diff "__tests__/${{ matrix.run.output || matrix.run.linter }}.output.json" "sarif.json" | tee diff.txt - name: Test Output run: | if [ -s diff.txt ]; diff --git a/README.md b/README.md index cecea92..8889f47 100644 --- a/README.md +++ b/README.md @@ -46,9 +46,13 @@ steps: - `fail`: True by default - fails the step if the linter found any issues. If set to false, the action will not fail the step. -- `failOnlyNew`: Set to true to fail only on issues found on lines added in the pull request (requires running on a pull request). If set to false or - omitted, the action will fail on any issue. Ignored if `fail` is set to false. An issue spanning several lines is considered new if any one of them was - added, since fixing such an issue usually requires changing the lines around it as well. +- `onlyNew`: Set to true to ignore every issue that is not on a line added in the pull request (requires running on a pull request). The issues are filtered + before anything else happens, so the SARIF file, the workflow log, the summary, the comments and the step's success or failure all reflect only the new + issues. If set to false or omitted, the action considers every issue. An issue spanning several lines is considered new if any one of them was added, since + fixing such an issue usually requires changing the lines around it as well. + + Note that this also removes the old issues from the SARIF, so uploading it to code scanning resolves their alerts. Leave it unset when the SARIF is uploaded + and the alerts of the whole repository should be kept. - `toolName`: _(required)_ The `tool name` that will be written in the SARIF output. This is used by both code scanning and auto-pr-commenting to resolve fixed issues. diff --git a/__tests__/bugalint.test.ts b/__tests__/bugalint.test.ts index 7e41945..8b00eb4 100644 --- a/__tests__/bugalint.test.ts +++ b/__tests__/bugalint.test.ts @@ -10,6 +10,7 @@ import { isNewIssue, isCommentableIssue, failOnIssues, + filterNewIssues, _testExports, type Parser } from '../src/bugalint' @@ -84,30 +85,34 @@ index 1111111..2222222 100644 describe('failOnIssues', () => { it('does nothing when no issues are found', () => { expect(() => { - failOnIssues([], 'test', '.') + failOnIssues([], 'test') }).not.toThrow() }) it('throws when issues are found', () => { expect(() => { - failOnIssues([{ path: 'a.py', line: 1 }, { msg: 'x' }], 'test', '.') + failOnIssues([{ path: 'a.py', line: 1 }, { msg: 'x' }], 'test') }).toThrow('test found 2 issues') }) + }) - it('throws only on new issues when a diff is given', () => { + describe('filterNewIssues', () => { + it('keeps only the issues overlapping an added line', () => { const issues = [ { path: 'A/B/test.py', line: 1 }, - { path: 'A/B/test.py', line: 2 } + { path: 'A/B/test.py', line: 2 }, + { path: 'A/B/test.py', line: 1, eline: 2 }, + { path: 'A/B/other.py', line: 2 } ] - expect(() => { - failOnIssues(issues, 'test', '.', diff) - }).toThrow('test found 1 issues') + expect(filterNewIssues(issues, diff, '.')).toStrictEqual([issues[1], issues[2]]) }) - it('does nothing when a diff is given and all issues are old', () => { - expect(() => { - failOnIssues([{ path: 'A/B/test.py', line: 1 }], 'test', '.', diff) - }).not.toThrow() + it('keeps nothing when all issues are old', () => { + expect(filterNewIssues([{ path: 'A/B/test.py', line: 1 }], diff, '.')).toStrictEqual([]) + }) + + it('relativizes the issue paths to the analysis path', () => { + expect(filterNewIssues([{ path: 'test.py', line: 3 }], diff, 'A\\B')).toHaveLength(1) }) }) }) diff --git a/action.yml b/action.yml index 6884a33..f041060 100644 --- a/action.yml +++ b/action.yml @@ -21,8 +21,8 @@ inputs: description: 'Should the action fail the step if issues are found' required: false default: 'true' - failOnlyNew: - description: 'Should the action fail only on issues found on lines added in the pull request' + onlyNew: + description: 'Should the action consider only issues found on lines added in the pull request, ignoring all others everywhere' required: false default: 'false' toolName: diff --git a/dist/index.js b/dist/index.js index d932ee0..efd564c 100644 --- a/dist/index.js +++ b/dist/index.js @@ -29942,6 +29942,7 @@ exports.getPrDiff = getPrDiff; exports.parseDiffLines = parseDiffLines; exports.isNewIssue = isNewIssue; exports.isCommentableIssue = isCommentableIssue; +exports.filterNewIssues = filterNewIssues; exports.failOnIssues = failOnIssues; exports.createSummary = createSummary; const github_1 = __nccwpck_require__(3228); @@ -30274,12 +30275,12 @@ function isCommentableIssue(issue, diffLines, analysisPath) { const lines = diffLines[normalizePath(issue.path, analysisPath)]; return issueLines(issue.line, issue.eline).every((line) => lines?.[line] != null); } -function failOnIssues(issues, toolName, analysisPath, prDiff) { - let failing = [...issues]; - if (prDiff != null) { - const diffLines = parseDiffLines(prDiff); - failing = failing.filter((issue) => isNewIssue(issue, diffLines, analysisPath)); - } +function filterNewIssues(issues, prDiff, analysisPath) { + const diffLines = parseDiffLines(prDiff); + return [...issues].filter((issue) => isNewIssue(issue, diffLines, analysisPath)); +} +function failOnIssues(issues, toolName) { + const failing = [...issues]; if (failing.length > 0) { throw new Error(`${toolName} found ${failing.length} issues`); } @@ -32256,7 +32257,7 @@ async function run() { const comment = (0, core_1.getBooleanInput)('comment'); const summary = (0, core_1.getBooleanInput)('summary'); const fail = (0, core_1.getBooleanInput)('fail'); - const failOnlyNew = (0, core_1.getBooleanInput)('failOnlyNew'); + const onlyNew = (0, core_1.getBooleanInput)('onlyNew'); const toolName = (0, core_1.getInput)('toolName'); const inputFormat = (0, core_1.getInput)('inputFormat'); const inputRegex = (0, core_1.getInput)('inputRegex'); @@ -32268,27 +32269,33 @@ async function run() { const raw = (0, fs_1.readFileSync)(inputFile, 'utf-8'); const input = inputFormat === 'diff' ? raw : raw.replace(/\r/g, ''); (0, core_1.debug)(`input: ${input}`); - const output = (0, bugalint_1.generateSarif)(parser(input), toolName, analysisPath); + const prNumber = github_1.context.payload.pull_request?.number; + let issues = [...parser(input)]; + let prDiff; + if (onlyNew) { + if (prNumber == null) { + throw new Error('No pull request number found.'); + } + prDiff = await (0, bugalint_1.getPrDiff)(githubToken, github_1.context.repo.owner, github_1.context.repo.repo, prNumber); + issues = (0, bugalint_1.filterNewIssues)(issues, prDiff, analysisPath); + } + const output = (0, bugalint_1.generateSarif)(issues, toolName, analysisPath); (0, core_1.info)(`SARIF output: ${JSON.stringify(output, null, 2)}`); if (sarif !== '') { (0, fs_1.writeFileSync)(sarif, JSON.stringify(output)); } - let prDiff; - if (comment || (fail && failOnlyNew)) { - const prNumber = github_1.context.payload.pull_request?.number; + if (comment) { if (prNumber == null) { throw new Error('No pull request number found.'); } - prDiff = await (0, bugalint_1.getPrDiff)(githubToken, github_1.context.repo.owner, github_1.context.repo.repo, prNumber); - if (comment) { - await (0, bugalint_1.addComments)(parser(input), prDiff, githubToken, toolName, github_1.context.repo.owner, github_1.context.repo.repo, prNumber, analysisPath); - } + prDiff ??= await (0, bugalint_1.getPrDiff)(githubToken, github_1.context.repo.owner, github_1.context.repo.repo, prNumber); + await (0, bugalint_1.addComments)(issues, prDiff, githubToken, toolName, github_1.context.repo.owner, github_1.context.repo.repo, prNumber, analysisPath); } if (summary) { - await (0, bugalint_1.createSummary)(parser(input), toolName, analysisPath); + await (0, bugalint_1.createSummary)(issues, toolName, analysisPath); } if (fail) { - (0, bugalint_1.failOnIssues)(parser(input), toolName, analysisPath, failOnlyNew ? prDiff : undefined); + (0, bugalint_1.failOnIssues)(issues, toolName); } } catch (error) { diff --git a/src/bugalint.ts b/src/bugalint.ts index 5c7698b..be3fde6 100644 --- a/src/bugalint.ts +++ b/src/bugalint.ts @@ -386,12 +386,13 @@ export function isCommentableIssue(issue: Issue, diffLines: DiffLines, analysisP return issueLines(issue.line, issue.eline).every((line) => lines?.[line] != null) } -export function failOnIssues(issues: Iterable, toolName: string, analysisPath: string, prDiff?: string): void { - let failing = [...issues] - if (prDiff != null) { - const diffLines = parseDiffLines(prDiff) - failing = failing.filter((issue) => isNewIssue(issue, diffLines, analysisPath)) - } +export function filterNewIssues(issues: Iterable, prDiff: string, analysisPath: string): Issue[] { + const diffLines = parseDiffLines(prDiff) + return [...issues].filter((issue) => isNewIssue(issue, diffLines, analysisPath)) +} + +export function failOnIssues(issues: Iterable, toolName: string): void { + const failing = [...issues] if (failing.length > 0) { throw new Error(`${toolName} found ${failing.length} issues`) } diff --git a/src/index.ts b/src/index.ts index 37b0291..5dbc604 100644 --- a/src/index.ts +++ b/src/index.ts @@ -2,7 +2,18 @@ import { readFileSync, writeFileSync } from 'fs' import type { Result } from 'sarif' import { context } from '@actions/github' import { getInput, getBooleanInput, debug, info, setFailed } from '@actions/core' -import { generateSarif, getKnownParser, getRegexParser, getDiffParser, getPrDiff, addComments, createSummary, failOnIssues, type Parser } from '../src/bugalint' +import { + generateSarif, + getKnownParser, + getRegexParser, + getDiffParser, + getPrDiff, + addComments, + createSummary, + failOnIssues, + filterNewIssues, + type Parser +} from '../src/bugalint' function getParser(inputFormat: string, inputRegex: string, levelMap: string, message: string): Parser { if (inputFormat === '') { @@ -21,7 +32,7 @@ export async function run(): Promise { const comment: boolean = getBooleanInput('comment') const summary: boolean = getBooleanInput('summary') const fail: boolean = getBooleanInput('fail') - const failOnlyNew: boolean = getBooleanInput('failOnlyNew') + const onlyNew: boolean = getBooleanInput('onlyNew') const toolName: string = getInput('toolName') const inputFormat: string = getInput('inputFormat') const inputRegex: string = getInput('inputRegex') @@ -34,27 +45,35 @@ export async function run(): Promise { const raw = readFileSync(inputFile, 'utf-8') const input = inputFormat === 'diff' ? raw : raw.replace(/\r/g, '') debug(`input: ${input}`) - const output = generateSarif(parser(input), toolName, analysisPath) + + const prNumber = context.payload.pull_request?.number + let issues = [...parser(input)] + let prDiff: string | undefined + if (onlyNew) { + if (prNumber == null) { + throw new Error('No pull request number found.') + } + prDiff = await getPrDiff(githubToken, context.repo.owner, context.repo.repo, prNumber) + issues = filterNewIssues(issues, prDiff, analysisPath) + } + + const output = generateSarif(issues, toolName, analysisPath) info(`SARIF output: ${JSON.stringify(output, null, 2)}`) if (sarif !== '') { writeFileSync(sarif, JSON.stringify(output)) } - let prDiff: string | undefined - if (comment || (fail && failOnlyNew)) { - const prNumber = context.payload.pull_request?.number + if (comment) { if (prNumber == null) { throw new Error('No pull request number found.') } - prDiff = await getPrDiff(githubToken, context.repo.owner, context.repo.repo, prNumber) - if (comment) { - await addComments(parser(input), prDiff, githubToken, toolName, context.repo.owner, context.repo.repo, prNumber, analysisPath) - } + prDiff ??= await getPrDiff(githubToken, context.repo.owner, context.repo.repo, prNumber) + await addComments(issues, prDiff, githubToken, toolName, context.repo.owner, context.repo.repo, prNumber, analysisPath) } if (summary) { - await createSummary(parser(input), toolName, analysisPath) + await createSummary(issues, toolName, analysisPath) } if (fail) { - failOnIssues(parser(input), toolName, analysisPath, failOnlyNew ? prDiff : undefined) + failOnIssues(issues, toolName) } } catch (error) { if (error instanceof Error) { From df5143d73c6786983e9770d5a37bf06c2b0a0503 Mon Sep 17 00:00:00 2001 From: Bugale Date: Sun, 26 Jul 2026 23:49:03 +0300 Subject: [PATCH 06/11] test: cover a formatter diff of CRLF content end to end The diff format is the one format whose input is read byte for byte, because a carriage return in it may be content rather than a line terminator. That decision lives in `index.ts`, which the unit tests do not reach at all: they call `parseFormatDiff` directly, so nothing covered the conditional that skips the strip. It reads like a redundant special case, and removing it silently rewrites the line endings of every suggestion. Add a fixture whose structural lines are terminated by a newline alone while its content lines carry a carriage return, which is what `git diff` of a CRLF file looks like. Removing the conditional changes its output, so the case now fails rather than passing quietly. The fixture needs `-text` to survive being committed, as normalization would turn it into an ordinary diff and make it pass either way. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .gitattributes | 1 + .github/workflows/check-code.yml | 1 + README.md | 3 ++ __tests__/diffcrlf.input.txt | 24 ++++++++++++++ __tests__/diffcrlf.output.json | 56 ++++++++++++++++++++++++++++++++ 5 files changed, 85 insertions(+) create mode 100644 .gitattributes create mode 100644 __tests__/diffcrlf.input.txt create mode 100644 __tests__/diffcrlf.output.json diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 0000000..b6640f1 --- /dev/null +++ b/.gitattributes @@ -0,0 +1 @@ +__tests__/diffcrlf.input.txt -text diff --git a/.github/workflows/check-code.yml b/.github/workflows/check-code.yml index f7cb0a4..34ba5b0 100644 --- a/.github/workflows/check-code.yml +++ b/.github/workflows/check-code.yml @@ -82,6 +82,7 @@ jobs: - {linter: 'sarif', format: 'sarif', regex: '', levelMap: '', analysisPath: '.'} - {linter: 'sariffix', format: 'sarif', regex: '', levelMap: '', analysisPath: '.'} - {linter: 'diff', format: 'diff', regex: '', levelMap: '', analysisPath: '.'} + - {linter: 'diffcrlf', format: 'diff', regex: '', levelMap: '', analysisPath: '.'} - {linter: 'flake8subpath', format: 'flake8', regex: '', levelMap: '', analysisPath: 'A\B'} - {linter: 'noissues', format: 'flake8', regex: '', levelMap: '', analysisPath: '.', outcome: 'success'} - {linter: 'noissues', format: 'flake8', regex: '', levelMap: '', analysisPath: '.', fail: 'false', outcome: 'success', name: 'noissues-nofail'} diff --git a/README.md b/README.md index 8889f47..68c73b6 100644 --- a/README.md +++ b/README.md @@ -124,6 +124,9 @@ is ignored, so the last line of such a file is reported like any other. Note that a formatter that fails without writing anything produces an empty diff, which is indistinguishable from a formatter that found nothing to fix. The step running the formatter should therefore fail the job by itself. +Unlike the other input formats, a diff is read byte for byte, since a carriage return in it may be content rather than a line terminator. A repository storing +its files with CRLF therefore gets suggestions with CRLF in them, instead of suggestions that silently rewrite the line endings of every line they touch. + #### Input Regex Named Groups When using a custom regular expression, it must contains named groups for Bugalint to successfully understand which parts of each line are the issue's diff --git a/__tests__/diffcrlf.input.txt b/__tests__/diffcrlf.input.txt new file mode 100644 index 0000000..02e1bb5 --- /dev/null +++ b/__tests__/diffcrlf.input.txt @@ -0,0 +1,24 @@ +diff --git a/src/crlf.c b/src/crlf.c +index 1111111..2222222 100644 +--- a/src/crlf.c ++++ b/src/crlf.c +@@ -1,5 +1,5 @@ + int main() { + int a = 0; +- int b=1; ++ int b = 1; + return a + b; + } +diff --git a/src/crlfmulti.c b/src/crlfmulti.c +index 3333333..4444444 100644 +--- a/src/crlfmulti.c ++++ b/src/crlfmulti.c +@@ -1,5 +1,6 @@ + void g(void) { +- int x=0; +- int y=1; ++ int x = 0; ++ int y = 1; + use(x, y); ++ extra(); + } diff --git a/__tests__/diffcrlf.output.json b/__tests__/diffcrlf.output.json new file mode 100644 index 0000000..51ba28d --- /dev/null +++ b/__tests__/diffcrlf.output.json @@ -0,0 +1,56 @@ +{ + "version": "2.1.0", + "$schema": "http://json.schemastore.org/sarif-2.1.0-rtm.6", + "runs": [ + { + "tool": { "driver": { "name": "test", "rules": [] } }, + "results": [ + { + "message": { "text": "Not formatted correctly" }, + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "src/crlf.c" }, "region": { "startLine": 3, "endLine": 3 } } }], + "fixes": [ + { + "artifactChanges": [ + { + "artifactLocation": { "uri": "src/crlf.c" }, + "replacements": [{ "deletedRegion": { "startLine": 3, "endLine": 3 }, "insertedContent": { "text": " int b = 1;\r" } }] + } + ] + } + ], + "level": "warning" + }, + { + "message": { "text": "Not formatted correctly" }, + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "src/crlfmulti.c" }, "region": { "startLine": 2, "endLine": 3 } } }], + "fixes": [ + { + "artifactChanges": [ + { + "artifactLocation": { "uri": "src/crlfmulti.c" }, + "replacements": [{ "deletedRegion": { "startLine": 2, "endLine": 3 }, "insertedContent": { "text": " int x = 0;\r\n int y = 1;\r" } }] + } + ] + } + ], + "level": "warning" + }, + { + "message": { "text": "Not formatted correctly" }, + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "src/crlfmulti.c" }, "region": { "startLine": 4, "endLine": 4 } } }], + "fixes": [ + { + "artifactChanges": [ + { + "artifactLocation": { "uri": "src/crlfmulti.c" }, + "replacements": [{ "deletedRegion": { "startLine": 4, "endLine": 4 }, "insertedContent": { "text": " use(x, y);\r\n extra();\r" } }] + } + ] + } + ], + "level": "warning" + } + ] + } + ] +} From c2dc4a1f39cffa249fa25189084faee0ec47613b Mon Sep 17 00:00:00 2001 From: Bugale Date: Mon, 27 Jul 2026 00:01:48 +0300 Subject: [PATCH 07/11] fix: tell blanking a line apart from deleting it A fix was a single string, so the lines to put in place of the reported ones were recovered by splitting it on newlines. That makes no distinction between replacing the lines with nothing and replacing them with one empty line, since joining either gives an empty string, and an empty fix is rendered as an empty suggestion, which GitHub applies as a deletion. A formatter stripping the whitespace of a blank line produces exactly that, so the suggestion removed the line instead of blanking it. Over a real repository this was 51 blocks in 36 files, and applying every suggestion reproduced the formatter's own output for 707 of 743 files, the 36 failures being exactly those files. Hold a fix as the list of lines it replaces the reported ones with, so no lines and one empty line are different values. Both are also expressible in SARIF, but only through a deleted region spanning the line terminator, as the other spelling writes both as an empty text. Fixes are now written out that way, which is what makes the round trip through a generated SARIF file lossless. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- README.md | 11 ++++-- __tests__/bugalint.test.ts | 69 ++++++++++++++++++++++------------ __tests__/diff.input.txt | 9 +++++ __tests__/diff.output.json | 53 ++++++++++++++++++++++---- __tests__/diffcrlf.output.json | 18 +++++++-- __tests__/sariffix.output.json | 25 +++++++++--- dist/index.js | 27 +++++++++---- src/bugalint.ts | 33 +++++++++++----- 8 files changed, 183 insertions(+), 62 deletions(-) diff --git a/README.md b/README.md index 68c73b6..f01dcf1 100644 --- a/README.md +++ b/README.md @@ -119,7 +119,8 @@ Every contiguous run of changed lines becomes one issue, rather than every hunk, reported range. Issues are anchored on the lines of the old side of the diff, which are the lines of the committed file that the pull request shows and that comments can be attached to, while the new side becomes the fix. A run that only adds lines has no line of its own to anchor to, so it is extended to a neighbouring line, preferring the preceding one, whose content is repeated in the fix. The marker `git diff` prints for a file that does not end with a newline -is ignored, so the last line of such a file is reported like any other. +is ignored, so the last line of such a file is reported like any other. A run replacing lines with nothing deletes them, while one replacing them with an empty +line blanks them, which is what a formatter stripping the whitespace of a blank line produces. Note that a formatter that fails without writing anything produces an empty diff, which is indistinguishable from a formatter that found nothing to fix. The step running the formatter should therefore fail the job by itself. @@ -191,9 +192,11 @@ Any other `deletedRegion`, such as one replacing a part of a line or lines other ignored, and the issue is commented on without one. The text itself is never trimmed beyond the single line terminator described above, so an additional trailing newline is rendered as a trailing empty line. -An empty `insertedContent.text` renders as an empty suggestion, which deletes the lines. A replacement consisting of a single empty line is written exactly the -same way in either form, so it is indistinguishable from a deletion and cannot be expressed. A producer that needs one should widen the replacement to include -a neighbouring line. +An empty `insertedContent.text` renders as an empty suggestion, which deletes the lines, in both forms. Replacing the lines with a single empty line is +therefore expressible only in the second form, as a text of exactly one newline — in the first form that same replacement is written as an empty text, which +cannot be told apart from a deletion. A producer restricted to the first form should widen the replacement to include a neighbouring line. + +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. ### Example With Custom Regex diff --git a/__tests__/bugalint.test.ts b/__tests__/bugalint.test.ts index 8b00eb4..c06cf90 100644 --- a/__tests__/bugalint.test.ts +++ b/__tests__/bugalint.test.ts @@ -121,35 +121,39 @@ describe('commentBody', () => { const tag = '' const header = `${tag}\n**Message**\n[warning:test]` const issue = { level: 'warning' as const, msg: 'Message' } - const body = (fix?: string): string => _testExports.buildCommentBody(tag, 'test', fix === undefined ? issue : { ...issue, fix }) + const body = (fix?: string[]): string => _testExports.buildCommentBody(tag, 'test', fix === undefined ? issue : { ...issue, fix }) it('omits the suggestion when the issue has no fix', () => { expect(body()).toBe(header) }) it('appends the suggestion after the identifier line', () => { - expect(body('x = 1')).toBe(`${header}\n\`\`\`suggestion\nx = 1\n\`\`\``) + expect(body(['x = 1'])).toBe(`${header}\n\`\`\`suggestion\nx = 1\n\`\`\``) }) it('keeps a multi line fix verbatim', () => { - expect(body('def f():\n return 1')).toBe(`${header}\n\`\`\`suggestion\ndef f():\n return 1\n\`\`\``) + expect(body(['def f():', ' return 1'])).toBe(`${header}\n\`\`\`suggestion\ndef f():\n return 1\n\`\`\``) }) - it('renders an empty fix as an empty suggestion, which deletes the lines', () => { - expect(body('')).toBe(`${header}\n\`\`\`suggestion\n\`\`\``) + it('renders a fix with no lines as an empty suggestion, which deletes the lines', () => { + expect(body([])).toBe(`${header}\n\`\`\`suggestion\n\`\`\``) }) - it('preserves a trailing newline, which keeps a trailing empty line', () => { - expect(body('x = 1\n')).toBe(`${header}\n\`\`\`suggestion\nx = 1\n\n\`\`\``) + it('renders a single empty line as a suggestion blanking the lines, not deleting them', () => { + expect(body([''])).toBe(`${header}\n\`\`\`suggestion\n\n\`\`\``) + }) + + it('preserves a trailing empty line', () => { + expect(body(['x = 1', ''])).toBe(`${header}\n\`\`\`suggestion\nx = 1\n\n\`\`\``) }) it('uses a fence longer than the longest backtick run in the fix', () => { - expect(body('doc = "```"')).toBe(`${header}\n\`\`\`\`suggestion\ndoc = "\`\`\`"\n\`\`\`\``) + expect(body(['doc = "```"'])).toBe(`${header}\n\`\`\`\`suggestion\ndoc = "\`\`\`"\n\`\`\`\``) }) }) describe('sarifFix', () => { - const fixOf = (region: Region, deletedRegion: Region, text: string): string | undefined => { + const fixOf = (region: Region, deletedRegion: Region, text: string): string[] | undefined => { const log = { version: '2.1.0', runs: [ @@ -169,18 +173,27 @@ describe('sarifFix', () => { } it('takes the text of a region ending at the end of the last reported line', () => { - expect(fixOf({ startLine: 3, endLine: 4 }, { startLine: 3, endLine: 4 }, 'a\nb')).toBe('a\nb') - expect(fixOf({ startLine: 3 }, { startLine: 3 }, 'a')).toBe('a') + expect(fixOf({ startLine: 3, endLine: 4 }, { startLine: 3, endLine: 4 }, 'a\nb')).toStrictEqual(['a', 'b']) + expect(fixOf({ startLine: 3 }, { startLine: 3 }, 'a')).toStrictEqual(['a']) }) it('drops the line terminator of a region ending at the beginning of the following line', () => { - expect(fixOf({ startLine: 3, endLine: 4 }, { startLine: 3, startColumn: 1, endLine: 5, endColumn: 1 }, 'a\nb\n')).toBe('a\nb') - expect(fixOf({ startLine: 3 }, { startLine: 3, startColumn: 1, endLine: 4, endColumn: 1 }, 'a\n')).toBe('a') + expect(fixOf({ startLine: 3, endLine: 4 }, { startLine: 3, startColumn: 1, endLine: 5, endColumn: 1 }, 'a\nb\n')).toStrictEqual(['a', 'b']) + expect(fixOf({ startLine: 3 }, { startLine: 3, startColumn: 1, endLine: 4, endColumn: 1 }, 'a\n')).toStrictEqual(['a']) }) it('keeps a trailing empty line of both forms', () => { - expect(fixOf({ startLine: 3, endLine: 4 }, { startLine: 3, endLine: 4 }, 'a\nb\n')).toBe('a\nb\n') - expect(fixOf({ startLine: 3, endLine: 4 }, { startLine: 3, startColumn: 1, endLine: 5, endColumn: 1 }, 'a\nb\n\n')).toBe('a\nb\n') + expect(fixOf({ startLine: 3, endLine: 4 }, { startLine: 3, endLine: 4 }, 'a\nb\n')).toStrictEqual(['a', 'b', '']) + expect(fixOf({ startLine: 3, endLine: 4 }, { startLine: 3, startColumn: 1, endLine: 5, endColumn: 1 }, 'a\nb\n\n')).toStrictEqual(['a', 'b', '']) + }) + + it('reads an empty text as a deletion in both forms', () => { + expect(fixOf({ startLine: 3 }, { startLine: 3 }, '')).toStrictEqual([]) + expect(fixOf({ startLine: 3 }, { startLine: 3, startColumn: 1, endLine: 4, endColumn: 1 }, '')).toStrictEqual([]) + }) + + it('reads a lone terminator as a single empty line, which only the second form can express', () => { + expect(fixOf({ startLine: 3 }, { startLine: 3, startColumn: 1, endLine: 4, endColumn: 1 }, '\n')).toStrictEqual(['']) }) it('ignores a fix replacing a part of a line', () => { @@ -219,14 +232,22 @@ describe('diffFormat', () => { '+int b = 1, c = 2;', ' return a;' ] - expect(firstIssue(lines)).toMatchObject({ path: 'a.c', line: 11, eline: 12, fix: 'int b = 1, c = 2;' }) + expect(firstIssue(lines)).toMatchObject({ path: 'a.c', line: 11, eline: 12, fix: ['int b = 1, c = 2;'] }) }) - it('reports a deletion as an empty fix', () => { + it('reports a deletion as a fix with no lines', () => { expect(firstIssue(['diff --git a/a.c b/a.c', '--- a/a.c', '+++ b/a.c', '@@ -5,3 +5,2 @@', ' int a = 0;', '-', ' return a;'])).toMatchObject({ line: 6, eline: 6, - fix: '' + fix: [] + }) + }) + + it('distinguishes blanking a line from deleting it', () => { + expect(firstIssue(['diff --git a/a.c b/a.c', '--- a/a.c', '+++ b/a.c', '@@ -5,3 +5,3 @@', ' int a = 0;', '- ', '+', ' return a;'])).toMatchObject({ + line: 6, + eline: 6, + fix: [''] }) }) @@ -234,7 +255,7 @@ describe('diffFormat', () => { expect(firstIssue(['diff --git a/a.c b/a.c', '--- a/a.c', '+++ b/a.c', '@@ -4,2 +4,3 @@', ' int a = 0;', '+int b = 1;', ' return a;'])).toMatchObject({ line: 4, eline: 4, - fix: 'int a = 0;\nint b = 1;' + fix: ['int a = 0;', 'int b = 1;'] }) }) @@ -242,21 +263,21 @@ describe('diffFormat', () => { expect(firstIssue(['diff --git a/a.c b/a.c', '--- a/a.c', '+++ b/a.c', '@@ -1,2 +1,3 @@', '+// header', ' int a = 0;', ' return a;'])).toMatchObject({ line: 1, eline: 1, - fix: '// header\nint a = 0;' + fix: ['// header', 'int a = 0;'] }) }) it('ignores the marker of a file not ending with a newline', () => { - expect(firstIssue([...header, '-int a=0;', '\\ No newline at end of file', '+int a = 0;'])).toMatchObject({ line: 1, eline: 1, fix: 'int a = 0;' }) + expect(firstIssue([...header, '-int a=0;', '\\ No newline at end of file', '+int a = 0;'])).toMatchObject({ line: 1, eline: 1, fix: ['int a = 0;'] }) }) it('keeps carriage returns that are part of the content', () => { - expect(firstIssue([...header, '-int a=0;\r', '+int a = 0;\r'])).toMatchObject({ fix: 'int a = 0;\r' }) + expect(firstIssue([...header, '-int a=0;\r', '+int a = 0;\r'])).toMatchObject({ fix: ['int a = 0;\r'] }) }) it('strips the carriage returns of a diff whose own lines are terminated by them', () => { - expect(firstIssue([...header, '-int a=0;', '+int a = 0;'], '\r\n')).toMatchObject({ fix: 'int a = 0;' }) - expect(firstIssue([...header, '-int a=0;\r', '+int a = 0;\r'], '\r\n')).toMatchObject({ fix: 'int a = 0;\r' }) + expect(firstIssue([...header, '-int a=0;', '+int a = 0;'], '\r\n')).toMatchObject({ fix: ['int a = 0;'] }) + expect(firstIssue([...header, '-int a=0;\r', '+int a = 0;\r'], '\r\n')).toMatchObject({ fix: ['int a = 0;\r'] }) }) }) diff --git a/__tests__/diff.input.txt b/__tests__/diff.input.txt index 575b471..c1f0020 100644 --- a/__tests__/diff.input.txt +++ b/__tests__/diff.input.txt @@ -70,3 +70,12 @@ index ddddddd..eeeeeee 100644 - int last=0; \ No newline at end of file + int last = 0; +diff --git a/src/blank.c b/src/blank.c +index fffffff..ggggggg 100644 +--- a/src/blank.c ++++ b/src/blank.c +@@ -60,3 +60,3 @@ void m(void) { + int a = 0; +- ++ + return a; diff --git a/__tests__/diff.output.json b/__tests__/diff.output.json index 8c10c27..b682e4e 100644 --- a/__tests__/diff.output.json +++ b/__tests__/diff.output.json @@ -13,7 +13,9 @@ "artifactChanges": [ { "artifactLocation": { "uri": "src/single.c" }, - "replacements": [{ "deletedRegion": { "startLine": 3, "endLine": 3 }, "insertedContent": { "text": " int b = 1;" } }] + "replacements": [ + { "deletedRegion": { "startLine": 3, "startColumn": 1, "endLine": 4, "endColumn": 1 }, "insertedContent": { "text": " int b = 1;\n" } } + ] } ] } @@ -28,7 +30,12 @@ "artifactChanges": [ { "artifactLocation": { "uri": "src/multi.c" }, - "replacements": [{ "deletedRegion": { "startLine": 9, "endLine": 12 }, "insertedContent": { "text": " if (x) {\n f();\n }" } }] + "replacements": [ + { + "deletedRegion": { "startLine": 9, "startColumn": 1, "endLine": 13, "endColumn": 1 }, + "insertedContent": { "text": " if (x) {\n f();\n }\n" } + } + ] } ] } @@ -43,7 +50,7 @@ "artifactChanges": [ { "artifactLocation": { "uri": "src/deletion.c" }, - "replacements": [{ "deletedRegion": { "startLine": 19, "endLine": 20 }, "insertedContent": { "text": "" } }] + "replacements": [{ "deletedRegion": { "startLine": 19, "startColumn": 1, "endLine": 21, "endColumn": 1 }, "insertedContent": { "text": "" } }] } ] } @@ -58,7 +65,9 @@ "artifactChanges": [ { "artifactLocation": { "uri": "src/insertion.c" }, - "replacements": [{ "deletedRegion": { "startLine": 31, "endLine": 31 }, "insertedContent": { "text": " int w = 1;\n" } }] + "replacements": [ + { "deletedRegion": { "startLine": 31, "startColumn": 1, "endLine": 32, "endColumn": 1 }, "insertedContent": { "text": " int w = 1;\n\n" } } + ] } ] } @@ -74,7 +83,10 @@ { "artifactLocation": { "uri": "src/top.c" }, "replacements": [ - { "deletedRegion": { "startLine": 1, "endLine": 1 }, "insertedContent": { "text": "// clang-format off\n#include " } } + { + "deletedRegion": { "startLine": 1, "startColumn": 1, "endLine": 2, "endColumn": 1 }, + "insertedContent": { "text": "// clang-format off\n#include \n" } + } ] } ] @@ -90,7 +102,12 @@ "artifactChanges": [ { "artifactLocation": { "uri": "src/fence.c" }, - "replacements": [{ "deletedRegion": { "startLine": 40, "endLine": 40 }, "insertedContent": { "text": " const char *doc = \"```\";" } }] + "replacements": [ + { + "deletedRegion": { "startLine": 40, "startColumn": 1, "endLine": 41, "endColumn": 1 }, + "insertedContent": { "text": " const char *doc = \"```\";\n" } + } + ] } ] } @@ -105,7 +122,29 @@ "artifactChanges": [ { "artifactLocation": { "uri": "src/eof.c" }, - "replacements": [{ "deletedRegion": { "startLine": 50, "endLine": 50 }, "insertedContent": { "text": " int last = 0;" } }] + "replacements": [ + { + "deletedRegion": { "startLine": 50, "startColumn": 1, "endLine": 51, "endColumn": 1 }, + "insertedContent": { "text": " int last = 0;\n" } + } + ] + } + ] + } + ], + "level": "warning" + }, + { + "message": { "text": "Not formatted correctly" }, + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "src/blank.c" }, "region": { "startLine": 61, "endLine": 61 } } }], + "fixes": [ + { + "artifactChanges": [ + { + "artifactLocation": { "uri": "src/blank.c" }, + "replacements": [ + { "deletedRegion": { "startLine": 61, "startColumn": 1, "endLine": 62, "endColumn": 1 }, "insertedContent": { "text": "\n" } } + ] } ] } diff --git a/__tests__/diffcrlf.output.json b/__tests__/diffcrlf.output.json index 51ba28d..14e2287 100644 --- a/__tests__/diffcrlf.output.json +++ b/__tests__/diffcrlf.output.json @@ -13,7 +13,9 @@ "artifactChanges": [ { "artifactLocation": { "uri": "src/crlf.c" }, - "replacements": [{ "deletedRegion": { "startLine": 3, "endLine": 3 }, "insertedContent": { "text": " int b = 1;\r" } }] + "replacements": [ + { "deletedRegion": { "startLine": 3, "startColumn": 1, "endLine": 4, "endColumn": 1 }, "insertedContent": { "text": " int b = 1;\r\n" } } + ] } ] } @@ -28,7 +30,12 @@ "artifactChanges": [ { "artifactLocation": { "uri": "src/crlfmulti.c" }, - "replacements": [{ "deletedRegion": { "startLine": 2, "endLine": 3 }, "insertedContent": { "text": " int x = 0;\r\n int y = 1;\r" } }] + "replacements": [ + { + "deletedRegion": { "startLine": 2, "startColumn": 1, "endLine": 4, "endColumn": 1 }, + "insertedContent": { "text": " int x = 0;\r\n int y = 1;\r\n" } + } + ] } ] } @@ -43,7 +50,12 @@ "artifactChanges": [ { "artifactLocation": { "uri": "src/crlfmulti.c" }, - "replacements": [{ "deletedRegion": { "startLine": 4, "endLine": 4 }, "insertedContent": { "text": " use(x, y);\r\n extra();\r" } }] + "replacements": [ + { + "deletedRegion": { "startLine": 4, "startColumn": 1, "endLine": 5, "endColumn": 1 }, + "insertedContent": { "text": " use(x, y);\r\n extra();\r\n" } + } + ] } ] } diff --git a/__tests__/sariffix.output.json b/__tests__/sariffix.output.json index 1f34891..1fe6676 100644 --- a/__tests__/sariffix.output.json +++ b/__tests__/sariffix.output.json @@ -13,7 +13,9 @@ "artifactChanges": [ { "artifactLocation": { "uri": "test.py" }, - "replacements": [{ "deletedRegion": { "startLine": 3, "endLine": 3 }, "insertedContent": { "text": "x = 1" } }] + "replacements": [ + { "deletedRegion": { "startLine": 3, "startColumn": 1, "endLine": 4, "endColumn": 1 }, "insertedContent": { "text": "x = 1\n" } } + ] } ] } @@ -30,7 +32,12 @@ "artifactChanges": [ { "artifactLocation": { "uri": "test.py" }, - "replacements": [{ "deletedRegion": { "startLine": 10, "endLine": 12 }, "insertedContent": { "text": "def f():\n return 1" } }] + "replacements": [ + { + "deletedRegion": { "startLine": 10, "startColumn": 1, "endLine": 13, "endColumn": 1 }, + "insertedContent": { "text": "def f():\n return 1\n" } + } + ] } ] } @@ -45,7 +52,7 @@ "artifactChanges": [ { "artifactLocation": { "uri": "test.py" }, - "replacements": [{ "deletedRegion": { "startLine": 20, "endLine": 21 }, "insertedContent": { "text": "" } }] + "replacements": [{ "deletedRegion": { "startLine": 20, "startColumn": 1, "endLine": 22, "endColumn": 1 }, "insertedContent": { "text": "" } }] } ] } @@ -60,7 +67,9 @@ "artifactChanges": [ { "artifactLocation": { "uri": "test.py" }, - "replacements": [{ "deletedRegion": { "startLine": 30, "endLine": 30 }, "insertedContent": { "text": "doc = \"```\"" } }] + "replacements": [ + { "deletedRegion": { "startLine": 30, "startColumn": 1, "endLine": 31, "endColumn": 1 }, "insertedContent": { "text": "doc = \"```\"\n" } } + ] } ] } @@ -80,7 +89,9 @@ "artifactChanges": [ { "artifactLocation": { "uri": "test.py" }, - "replacements": [{ "deletedRegion": { "startLine": 50, "endLine": 51 }, "insertedContent": { "text": "a = 1\nb = 2" } }] + "replacements": [ + { "deletedRegion": { "startLine": 50, "startColumn": 1, "endLine": 52, "endColumn": 1 }, "insertedContent": { "text": "a = 1\nb = 2\n" } } + ] } ] } @@ -105,7 +116,9 @@ "artifactChanges": [ { "artifactLocation": { "uri": "test.py" }, - "replacements": [{ "deletedRegion": { "startLine": 80, "endLine": 80 }, "insertedContent": { "text": "z = 3\n" } }] + "replacements": [ + { "deletedRegion": { "startLine": 80, "startColumn": 1, "endLine": 81, "endColumn": 1 }, "insertedContent": { "text": "z = 3\n\n" } } + ] } ] } diff --git a/dist/index.js b/dist/index.js index efd564c..311ec43 100644 --- a/dist/index.js +++ b/dist/index.js @@ -29987,6 +29987,12 @@ function* parsePylint(input) { }; } } +function splitFixText(text, terminated = false) { + return text === '' ? [] : (terminated ? text.replace(/\n$/, '') : text).split('\n'); +} +function joinFixLines(fix) { + return fix.map((line) => `${line}\n`).join(''); +} function parseSarifFix(result, region) { const replacement = result.fixes?.[0]?.artifactChanges?.[0]?.replacements?.[0]; const text = replacement?.insertedContent?.text; @@ -29999,9 +30005,9 @@ function parseSarifFix(result, region) { return undefined; } if (deleted.endColumn == null) { - return (deleted.endLine ?? deleted.startLine) === endLine ? text : undefined; + return (deleted.endLine ?? deleted.startLine) === endLine ? splitFixText(text) : undefined; } - return deleted.endColumn === 1 && deleted.endLine === endLine + 1 ? text.replace(/\n$/, '') : undefined; + return deleted.endColumn === 1 && deleted.endLine === endLine + 1 ? splitFixText(text, true) : undefined; } function* parseSarif(input) { const log = JSON.parse(input); @@ -30064,16 +30070,16 @@ function* parseFormatDiff(input, message) { } let location; if (deleted.length > 0) { - location = { line: deleted[0], eline: deleted[deleted.length - 1], fix: inserted.join('\n') }; + location = { line: deleted[0], eline: deleted[deleted.length - 1], fix: inserted }; } else if (inserted.length > 0) { const before = normalDiffLine(changes[start - 1]); const after = normalDiffLine(changes[index]); if (before != null) { - location = { line: before.ln1, eline: before.ln1, fix: [before.content.slice(1), ...inserted].join('\n') }; + location = { line: before.ln1, eline: before.ln1, fix: [before.content.slice(1), ...inserted] }; } else if (after != null) { - location = { line: after.ln1, eline: after.ln1, fix: [...inserted, after.content.slice(1)].join('\n') }; + location = { line: after.ln1, eline: after.ln1, fix: [...inserted, after.content.slice(1)] }; } } else { @@ -30141,7 +30147,12 @@ function generateSarif(issues, identifier, analysisPath) { artifactChanges: [ { artifactLocation: { uri }, - replacements: [{ deletedRegion: { startLine: issue.line, endLine: issue.eline ?? issue.line }, insertedContent: { text: issue.fix } }] + replacements: [ + { + deletedRegion: { startLine: issue.line, startColumn: 1, endLine: (issue.eline ?? issue.line) + 1, endColumn: 1 }, + insertedContent: { text: joinFixLines(issue.fix) } + } + ] } ] } @@ -30176,8 +30187,8 @@ function buildCommentBody(commentTag, identifier, issue) { if (issue.fix == null) { return body; } - const fence = '`'.repeat(Math.max(3, ...Array.from(issue.fix.matchAll(/`+/g), (m) => m[0].length + 1))); - return `${body}\n${fence}suggestion\n${issue.fix === '' ? '' : `${issue.fix}\n`}${fence}`; + 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${joinFixLines(issue.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$']}] */ diff --git a/src/bugalint.ts b/src/bugalint.ts index be3fde6..e91231b 100644 --- a/src/bugalint.ts +++ b/src/bugalint.ts @@ -15,7 +15,7 @@ interface Issue { col?: number eline?: number ecol?: number - fix?: string + fix?: string[] } export type Parser = (input: string) => Generator @@ -60,7 +60,15 @@ function* parsePylint(input: string): Generator { } } -function parseSarifFix(result: Result, region?: Region): string | undefined { +function splitFixText(text: string, terminated = false): string[] { + return text === '' ? [] : (terminated ? text.replace(/\n$/, '') : text).split('\n') +} + +function joinFixLines(fix: string[]): string { + return fix.map((line) => `${line}\n`).join('') +} + +function parseSarifFix(result: Result, region?: Region): string[] | undefined { const replacement = result.fixes?.[0]?.artifactChanges?.[0]?.replacements?.[0] const text = replacement?.insertedContent?.text if (replacement == null || text == null || region?.startLine == null) { @@ -72,9 +80,9 @@ function parseSarifFix(result: Result, region?: Region): string | undefined { return undefined } if (deleted.endColumn == null) { - return (deleted.endLine ?? deleted.startLine) === endLine ? text : undefined + return (deleted.endLine ?? deleted.startLine) === endLine ? splitFixText(text) : undefined } - return deleted.endColumn === 1 && deleted.endLine === endLine + 1 ? text.replace(/\n$/, '') : undefined + return deleted.endColumn === 1 && deleted.endLine === endLine + 1 ? splitFixText(text, true) : undefined } function* parseSarif(input: string): Generator { @@ -142,14 +150,14 @@ function* parseFormatDiff(input: string, message: string): Generator { } let location: Pick | undefined if (deleted.length > 0) { - location = { line: deleted[0], eline: deleted[deleted.length - 1], fix: inserted.join('\n') } + location = { line: deleted[0], eline: deleted[deleted.length - 1], fix: inserted } } else if (inserted.length > 0) { const before = normalDiffLine(changes[start - 1]) const after = normalDiffLine(changes[index]) if (before != null) { - location = { line: before.ln1, eline: before.ln1, fix: [before.content.slice(1), ...inserted].join('\n') } + location = { line: before.ln1, eline: before.ln1, fix: [before.content.slice(1), ...inserted] } } else if (after != null) { - location = { line: after.ln1, eline: after.ln1, fix: [...inserted, after.content.slice(1)].join('\n') } + location = { line: after.ln1, eline: after.ln1, fix: [...inserted, after.content.slice(1)] } } } else { index++ @@ -227,7 +235,12 @@ export function generateSarif(issues: Iterable, identifier: string, analy artifactChanges: [ { artifactLocation: { uri }, - replacements: [{ deletedRegion: { startLine: issue.line, endLine: issue.eline ?? issue.line }, insertedContent: { text: issue.fix } }] + replacements: [ + { + deletedRegion: { startLine: issue.line, startColumn: 1, endLine: (issue.eline ?? issue.line) + 1, endColumn: 1 }, + insertedContent: { text: joinFixLines(issue.fix) } + } + ] } ] } @@ -266,8 +279,8 @@ function buildCommentBody(commentTag: string, identifier: string, issue: Issue): if (issue.fix == null) { return body } - const fence = '`'.repeat(Math.max(3, ...Array.from(issue.fix.matchAll(/`+/g), (m) => m[0].length + 1))) - return `${body}\n${fence}suggestion\n${issue.fix === '' ? '' : `${issue.fix}\n`}${fence}` + 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${joinFixLines(issue.fix)}${fence}` } export async function addComments( From c148545b71110a5e816ed14d851d93837c7cbad2 Mon Sep 17 00:00:00 2001 From: Bugale Date: Mon, 27 Jul 2026 00:18:04 +0300 Subject: [PATCH 08/11] fix: report a change of a line terminator alone without a fix A formatter that only terminates the last line of a file not ending with a newline produces a diff whose old and new lines are identical, so the fix built from it replaced a line with itself. Such a suggestion cannot be applied to any effect, leaving behind a comment that no reviewer can resolve by clicking it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- README.md | 5 +++-- __tests__/bugalint.test.ts | 6 ++++++ __tests__/diff.input.txt | 8 ++++++++ __tests__/diff.output.json | 5 +++++ dist/index.js | 10 ++++++++-- src/bugalint.ts | 13 ++++++++++--- 6 files changed, 40 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index f01dcf1..afdcfb2 100644 --- a/README.md +++ b/README.md @@ -119,8 +119,9 @@ Every contiguous run of changed lines becomes one issue, rather than every hunk, reported range. Issues are anchored on the lines of the old side of the diff, which are the lines of the committed file that the pull request shows and that comments can be attached to, while the new side becomes the fix. A run that only adds lines has no line of its own to anchor to, so it is extended to a neighbouring line, preferring the preceding one, whose content is repeated in the fix. The marker `git diff` prints for a file that does not end with a newline -is ignored, so the last line of such a file is reported like any other. A run replacing lines with nothing deletes them, while one replacing them with an empty -line blanks them, which is what a formatter stripping the whitespace of a blank line produces. +is ignored, so the last line of such a file is reported like any other. A change of that terminator alone leaves the old and the new lines identical, so the +issue is reported without a fix rather than with a suggestion replacing a line with itself. A run replacing lines with nothing deletes them, while one replacing +them with an empty line blanks them, which is what a formatter stripping the whitespace of a blank line produces. Note that a formatter that fails without writing anything produces an empty diff, which is indistinguishable from a formatter that found nothing to fix. The step running the formatter should therefore fail the job by itself. diff --git a/__tests__/bugalint.test.ts b/__tests__/bugalint.test.ts index c06cf90..c1e9746 100644 --- a/__tests__/bugalint.test.ts +++ b/__tests__/bugalint.test.ts @@ -271,6 +271,12 @@ describe('diffFormat', () => { expect(firstIssue([...header, '-int a=0;', '\\ No newline at end of file', '+int a = 0;'])).toMatchObject({ line: 1, eline: 1, fix: ['int a = 0;'] }) }) + it('reports a change of the line terminator alone without a fix replacing a line with itself', () => { + const issue = firstIssue([...header, '-int a = 0;', '\\ No newline at end of file', '+int a = 0;']) + expect(issue).toMatchObject({ line: 1, eline: 1 }) + expect(issue).not.toHaveProperty('fix') + }) + it('keeps carriage returns that are part of the content', () => { expect(firstIssue([...header, '-int a=0;\r', '+int a = 0;\r'])).toMatchObject({ fix: ['int a = 0;\r'] }) }) diff --git a/__tests__/diff.input.txt b/__tests__/diff.input.txt index c1f0020..a55eb4c 100644 --- a/__tests__/diff.input.txt +++ b/__tests__/diff.input.txt @@ -79,3 +79,11 @@ index fffffff..ggggggg 100644 - + return a; +diff --git a/src/term.c b/src/term.c +index hhhhhhh..iiiiiii 100644 +--- a/src/term.c ++++ b/src/term.c +@@ -70,1 +70,1 @@ void n(void) { +- int last = 0; +\ No newline at end of file ++ int last = 0; diff --git a/__tests__/diff.output.json b/__tests__/diff.output.json index b682e4e..8be86a0 100644 --- a/__tests__/diff.output.json +++ b/__tests__/diff.output.json @@ -150,6 +150,11 @@ } ], "level": "warning" + }, + { + "message": { "text": "Not formatted correctly" }, + "locations": [{ "physicalLocation": { "artifactLocation": { "uri": "src/term.c" }, "region": { "startLine": 70, "endLine": 70 } } }], + "level": "warning" } ] } diff --git a/dist/index.js b/dist/index.js index 311ec43..9b6b11b 100644 --- a/dist/index.js +++ b/dist/index.js @@ -30039,6 +30039,9 @@ function normalizeDiffLineEndings(diff) { function normalDiffLine(change) { return change?.type === 'normal' ? change : undefined; } +function changesContent(removed, inserted) { + return removed.length !== inserted.length || removed.some((line, index) => line !== inserted[index]); +} function* parseFormatDiff(input, message) { for (const file of (0, parse_diff_1.default)(normalizeDiffLineEndings(input))) { const filePath = file.from ?? file.to; @@ -30057,7 +30060,7 @@ function* parseFormatDiff(input, message) { if (change.type !== 'del') { break; } - deleted.push(change.ln); + deleted.push(change); index++; } while (index < changes.length) { @@ -30070,7 +30073,10 @@ function* parseFormatDiff(input, message) { } let location; if (deleted.length > 0) { - location = { line: deleted[0], eline: deleted[deleted.length - 1], fix: inserted }; + const line = deleted[0].ln; + const eline = deleted[deleted.length - 1].ln; + const removed = deleted.map((change) => change.content.slice(1)); + location = changesContent(removed, inserted) ? { line, eline, fix: inserted } : { line, eline }; } else if (inserted.length > 0) { const before = normalDiffLine(changes[start - 1]); diff --git a/src/bugalint.ts b/src/bugalint.ts index e91231b..98b9f86 100644 --- a/src/bugalint.ts +++ b/src/bugalint.ts @@ -119,6 +119,10 @@ function normalDiffLine(change?: parseDiff.Change): parseDiff.NormalChange | und return change?.type === 'normal' ? change : undefined } +function changesContent(removed: string[], inserted: string[]): boolean { + return removed.length !== inserted.length || removed.some((line, index) => line !== inserted[index]) +} + function* parseFormatDiff(input: string, message: string): Generator { for (const file of parseDiff(normalizeDiffLineEndings(input))) { const filePath = file.from ?? file.to @@ -130,14 +134,14 @@ function* parseFormatDiff(input: string, message: string): Generator { let index = 0 while (index < changes.length) { const start = index - const deleted: number[] = [] + const deleted: parseDiff.DeleteChange[] = [] const inserted: string[] = [] while (index < changes.length) { const change = changes[index] if (change.type !== 'del') { break } - deleted.push(change.ln) + deleted.push(change) index++ } while (index < changes.length) { @@ -150,7 +154,10 @@ function* parseFormatDiff(input: string, message: string): Generator { } let location: Pick | undefined if (deleted.length > 0) { - location = { line: deleted[0], eline: deleted[deleted.length - 1], fix: inserted } + const line = deleted[0].ln + const eline = deleted[deleted.length - 1].ln + const removed = deleted.map((change) => change.content.slice(1)) + location = changesContent(removed, inserted) ? { line, eline, fix: inserted } : { line, eline } } else if (inserted.length > 0) { const before = normalDiffLine(changes[start - 1]) const after = normalDiffLine(changes[index]) From d9c7741cc69fe91a74e495baaaed394f18ad0f08 Mon Sep 17 00:00:00 2001 From: Bugale Date: Mon, 27 Jul 2026 00:48:02 +0300 Subject: [PATCH 09/11] feat: reject the renamed failOnlyNew input with an explanatory error GitHub passes every `with:` key to an action and never rejects one the action does not declare, so a workflow left on `failOnlyNew` after the rename silently loses the filtering and fails on every pre-existing issue instead. The error that surfaces names a count of issues, which points the reader at their code rather than at their workflow. Fail up front with the new name instead. The check reads the input rather than a boolean, so it fires for `false` just as it does for `true`, and it runs before anything is written, so a rejected run leaves no output to be mistaken for a filtered one. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/workflows/check-code.yml | 27 +++++++++++++++++++++++++++ README.md | 3 +++ dist/index.js | 3 +++ src/index.ts | 3 +++ 4 files changed, 36 insertions(+) diff --git a/.github/workflows/check-code.yml b/.github/workflows/check-code.yml index 34ba5b0..6c25272 100644 --- a/.github/workflows/check-code.yml +++ b/.github/workflows/check-code.yml @@ -133,3 +133,30 @@ jobs: else echo "Success" fi + gha-renamed: + name: GitHub Action (renamed input) + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 + - name: Run Action + id: run + continue-on-error: true + uses: ./ + with: + inputFile: '__tests__/noissues.input.txt' + toolName: 'test' + inputFormat: 'flake8' + failOnlyNew: 'true' + - name: Test Outcome + run: | + if [ "${{ steps.run.outcome }}" != "failure" ]; + then + echo "Expected the action to reject the renamed input but it ended with ${{ steps.run.outcome }}" + exit 1 + fi + if [ -f sarif.json ]; + then + echo "Expected the action to reject the renamed input before writing any output" + exit 1 + fi diff --git a/README.md b/README.md index afdcfb2..fe08418 100644 --- a/README.md +++ b/README.md @@ -54,6 +54,9 @@ steps: Note that this also removes the old issues from the SARIF, so uploading it to code scanning resolves their alerts. Leave it unset when the SARIF is uploaded and the alerts of the whole repository should be kept. + This input was named `failOnlyNew` before it applied to anything but the failure. Since GitHub silently ignores an input an action does not declare, the + action fails with an explanatory error when the old name is passed, rather than letting it look like it is still in effect. + - `toolName`: _(required)_ The `tool name` that will be written in the SARIF output. This is used by both code scanning and auto-pr-commenting to resolve fixed issues. diff --git a/dist/index.js b/dist/index.js index 9b6b11b..ba34e80 100644 --- a/dist/index.js +++ b/dist/index.js @@ -32269,6 +32269,9 @@ function getParser(inputFormat, inputRegex, levelMap, message) { } async function run() { try { + if ((0, core_1.getInput)('failOnlyNew') !== '') { + throw new Error('The `failOnlyNew` input was renamed to `onlyNew`, which now filters the SARIF, the log, the summary and the comments as well.'); + } const inputFile = (0, core_1.getInput)('inputFile'); const sarif = (0, core_1.getInput)('sarif'); const comment = (0, core_1.getBooleanInput)('comment'); diff --git a/src/index.ts b/src/index.ts index 5dbc604..d979515 100644 --- a/src/index.ts +++ b/src/index.ts @@ -27,6 +27,9 @@ function getParser(inputFormat: string, inputRegex: string, levelMap: string, me export async function run(): Promise { try { + if (getInput('failOnlyNew') !== '') { + throw new Error('The `failOnlyNew` input was renamed to `onlyNew`, which now filters the SARIF, the log, the summary and the comments as well.') + } const inputFile: string = getInput('inputFile') const sarif: string = getInput('sarif') const comment: boolean = getBooleanInput('comment') From faec51d2cfc386ced719dce3e59c0b6448d03355 Mon Sep 17 00:00:00 2001 From: Bugale Date: Mon, 27 Jul 2026 00:55:56 +0300 Subject: [PATCH 10/11] feat: reconcile pull request comments instead of recreating them Bugalint deleted every review comment it had posted and recreated all of them on every run. That re-notified every reviewer about every still open issue on every push, orphaned human replies and broke comment links. Each comment body now starts with ``, where the fingerprint is a sha256 over the rendered comment without the tag: the message, the level, the rule identifiers and the whole suggestion block. An issue anchored on the same file and line range as an existing comment with the same fingerprint is left completely alone, an issue matching nothing gets a new comment, and a comment matching no issue is deleted. The anchor is compared against the line GitHub currently reports rather than the one the comment was created on, since GitHub re-anchors comments as the pull request is pushed to. The fingerprint deliberately carries no line number so it does not churn on a rebase. Matching consumes comment ids, so N identical findings keep exactly N comments. Comments that anyone replied to are never deleted. Comments of older versions carry a tag with no fingerprint, so they are deleted and reposted once per pull request on upgrade. Kept comments count against the existing 50 comment cap, so consecutive runs cannot accumulate 50 comments each. Comments kept only because of a reply are excluded from the count. The filters that keep the single createReview call free of out of diff anchors still run before matching. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- README.md | 29 +++- __tests__/bugalint.test.ts | 294 ++++++++++++++++++++++++++++++++++++- dist/index.js | 96 +++++++++--- src/bugalint.ts | 128 ++++++++++++---- 4 files changed, 491 insertions(+), 56 deletions(-) diff --git a/README.md b/README.md index fe08418..26ee5ae 100644 --- a/README.md +++ b/README.md @@ -40,7 +40,7 @@ steps: - `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. + rejects comments anchored outside it. Comments are [reconciled](#comment-reconciliation) between runs rather than recreated. - `summary`: True by default - generates a markdown summary for the job. If set to false, the action will not generate a markdown summary. @@ -202,6 +202,33 @@ cannot be told apart from a deletion. A producer restricted to the first form sh 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. +### Comment Reconciliation + +Every comment Bugalint posts starts with an invisible HTML comment carrying the `toolName` and a fingerprint of the rest of the comment: the message, the level, +the rule identifiers and the whole suggestion. On every run Bugalint lists the pull request's review comments, and for each issue it would comment on it looks +for one of its own comments anchored on the same file and the same line range and carrying the same fingerprint: + +- A comment that matches an issue is left completely alone. It is neither deleted nor posted again, so pushing to a pull request no longer re-notifies every + reviewer about every issue that did not change, and links to such a comment keep working. + +- An issue matching no comment gets a new one. + +- A comment of the same `toolName` matching no issue is deleted, which is what makes a fixed issue's comment go away. + +The line range is compared to the line GitHub currently reports for the comment rather than the one it was created on. GitHub re-anchors a comment as the pull +request is pushed to, so a comment that merely moved is still recognized. The fingerprint itself covers no line number, so it does not change when lines are +added above the issue. It does cover the suggestion, so an issue whose fix changed is a different comment: the old one is deleted and a new one is posted. + +A comment that anyone replied to is never deleted, so a discussion is not orphaned when the issue that started it is fixed. Comments of other tools, of other +`toolName`s and of humans are never touched. + +Comments posted by versions of Bugalint older than this feature carry a tag with no fingerprint, so they can never match. On the first run after the upgrade +each of them is deleted and its issue is commented on again in the new format, once per pull request. + +At most 50 comments carrying a current issue are kept on a pull request. The comments that are kept count against that limit, so consecutive runs cannot +accumulate 50 comments each: when 48 comments are kept, only 2 new ones are posted. Comments kept only because someone replied to them are not counted, as +they are discussions rather than reports. + ### 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 c1e9746..6b0cbec 100644 --- a/__tests__/bugalint.test.ts +++ b/__tests__/bugalint.test.ts @@ -1,6 +1,8 @@ import '@microsoft/jest-sarif' import { readFileSync } from 'fs' import type { Region } from 'sarif' +import { getOctokit } from '@actions/github' +import { warning } from '@actions/core' import { generateSarif, getKnownParser, @@ -11,10 +13,16 @@ import { isCommentableIssue, failOnIssues, filterNewIssues, + addComments, _testExports, type Parser } from '../src/bugalint' +jest.mock('@actions/github', () => ({ getOctokit: jest.fn() })) +jest.mock('@actions/core', () => ({ ...jest.requireActual('@actions/core'), warning: jest.fn() })) + +/* eslint camelcase: ["error", {allow: ['^comment_id$', '^start_side$', '^start_line$', '^in_reply_to_id$']}] */ + describe('fullConversion', () => { it.each([ ['mypy', getKnownParser('mypy'), '.'], @@ -118,10 +126,16 @@ index 1111111..2222222 100644 }) describe('commentBody', () => { - const tag = '' - const header = `${tag}\n**Message**\n[warning:test]` const issue = { level: 'warning' as const, msg: 'Message' } - const body = (fix?: string[]): string => _testExports.buildCommentBody(tag, 'test', fix === undefined ? issue : { ...issue, fix }) + const header = '**Message**\n[warning:test]' + const built = (fix?: string[]): ReturnType => + _testExports.buildComment('test', fix === undefined ? issue : { ...issue, fix }) + const body = (fix?: string[]): string => built(fix).body.replace(/^\n/, '') + + it('starts with an invisible tag carrying the identifier and the fingerprint', () => { + expect(built().body).toBe(`\n${header}`) + expect(built().fingerprint).toMatch(/^[0-9a-f]{16}$/) + }) it('omits the suggestion when the issue has no fix', () => { expect(body()).toBe(header) @@ -150,6 +164,280 @@ describe('commentBody', () => { it('uses a fence longer than the longest backtick run in the fix', () => { expect(body(['doc = "```"'])).toBe(`${header}\n\`\`\`\`suggestion\ndoc = "\`\`\`"\n\`\`\`\``) }) + + it('gives the same fingerprint to the same rendered comment and a different one to any change', () => { + expect(built(['x = 1']).fingerprint).toBe(built(['x = 1']).fingerprint) + expect(built(['x = 1']).fingerprint).not.toBe(built(['x = 2']).fingerprint) + expect(built(['x = 1']).fingerprint).not.toBe(built().fingerprint) + expect(built().fingerprint).not.toBe(_testExports.buildComment('test', { ...issue, msg: 'Other' }).fingerprint) + expect(built().fingerprint).not.toBe(_testExports.buildComment('test', { ...issue, level: 'error' }).fingerprint) + expect(built().fingerprint).not.toBe(_testExports.buildComment('other', issue).fingerprint) + }) +}) + +describe('commentReconciliation', () => { + type Issue = Parameters[0] extends Iterable ? T : never + + interface ReviewComment { + id: number + body: string + path: string + line?: number + start_line?: number | null + in_reply_to_id?: number + } + + interface DraftComment { + path: string + side: string + start_side: string + line: number + start_line?: number + body: string + } + + interface Calls { + posted: DraftComment[][] + deleted: number[] + order: string[] + } + + const identifier = 'test' + const file = 'a.py' + const otherFile = 'b.py' + const warnings = jest.mocked(warning) + const bodyOf = (issue: Issue, tool = identifier): string => _testExports.buildComment(tool, issue).body + + const diff = (count: number): string => + [file, otherFile] + .map((name) => + [ + `diff --git a/${name} b/${name}`, + `--- a/${name}`, + `+++ b/${name}`, + `@@ -0,0 +1,${count} @@`, + ...Array.from({ length: count }, (_, index) => `+line ${index + 1}`), + '' + ].join('\n') + ) + .join('') + + const issueAt = (line: number, fix?: string[], eline?: number): Issue => ({ msg: 'Message', level: 'warning', path: file, line, eline, fix }) + + const commentOf = (id: number, issue: Issue, overrides: Partial = {}): ReviewComment => ({ + id, + body: bodyOf(issue), + path: file, + line: issue.eline ?? issue.line, + start_line: issue.eline == null ? null : issue.line, + ...overrides + }) + + const run = async (existing: ReviewComment[], issues: Issue[], lines = 10, tool = identifier): Promise => { + const calls: Calls = { posted: [], deleted: [], order: [] } + const pages: ReviewComment[][] = [] + for (let index = 0; index < existing.length; index += 2) { + pages.push(existing.slice(index, index + 2)) + } + const octokit = { + paginate: { + async *iterator(): AsyncGenerator<{ data: ReviewComment[] }> { + for (const page of pages) { + yield { data: page } + } + } + }, + rest: { + pulls: { + listReviewComments: {}, + createReview: async (args: { comments: DraftComment[] }): Promise => { + calls.posted.push(args.comments) + calls.order.push('post') + }, + deleteReviewComment: async (args: { comment_id: number }): Promise => { + calls.deleted.push(args.comment_id) + calls.order.push(`delete ${args.comment_id}`) + } + } + } + } + jest.mocked(getOctokit).mockReturnValue(octokit as unknown as ReturnType) + await addComments(issues, diff(lines), 'token', tool, 'owner', 'repo', 1, '.') + return calls + } + + beforeEach(() => { + warnings.mockClear() + }) + + it('posts a comment of a finding that does not have one yet', async () => { + const issue = issueAt(2, ['x = 1']) + const calls = await run([], [issue]) + expect(calls.posted).toStrictEqual([[{ path: file, side: 'RIGHT', start_side: 'RIGHT', line: 2, start_line: undefined, body: bodyOf(issue) }]]) + expect(calls.deleted).toStrictEqual([]) + }) + + it('leaves the comment of an unchanged finding alone, neither deleting nor reposting it', async () => { + const issue = issueAt(2, ['x = 1']) + const calls = await run([commentOf(1, issue)], [issue]) + expect(calls.posted).toStrictEqual([]) + expect(calls.deleted).toStrictEqual([]) + }) + + it('deletes the comment of a finding that disappeared', async () => { + const calls = await run([commentOf(1, issueAt(2, ['x = 1']))], []) + expect(calls.posted).toStrictEqual([]) + expect(calls.deleted).toStrictEqual([1]) + }) + + it('replaces the comment of a finding whose suggestion changed', async () => { + const changed = issueAt(2, ['x = 2']) + const calls = await run([commentOf(1, issueAt(2, ['x = 1']))], [changed]) + expect(calls.posted).toStrictEqual([[expect.objectContaining({ line: 2, body: bodyOf(changed) })]]) + expect(calls.deleted).toStrictEqual([1]) + }) + + it('replaces the comment of a finding whose message changed', async () => { + const changed = { ...issueAt(2), msg: 'Other' } + const calls = await run([commentOf(1, issueAt(2))], [changed]) + expect(calls.posted).toStrictEqual([[expect.objectContaining({ body: bodyOf(changed) })]]) + expect(calls.deleted).toStrictEqual([1]) + }) + + it('posts before deleting, so a rejected review leaves the old comments in place', async () => { + const calls = await run([commentOf(1, issueAt(2, ['x = 1']))], [issueAt(3, ['y = 1'])]) + expect(calls.order).toStrictEqual(['post', 'delete 1']) + }) + + it('matches the line GitHub currently reports, so a comment that moved with the diff is kept', async () => { + const shifted = commentOf(1, issueAt(2, ['x = 1']), { line: 5, start_line: null }) + const calls = await run([shifted], [issueAt(5, ['x = 1'])]) + expect(calls.posted).toStrictEqual([]) + expect(calls.deleted).toStrictEqual([]) + }) + + it('replaces a comment that GitHub no longer anchors to any line', async () => { + const issue = issueAt(2, ['x = 1']) + const calls = await run([commentOf(1, issue, { line: undefined })], [issue]) + expect(calls.posted).toStrictEqual([[expect.objectContaining({ body: bodyOf(issue) })]]) + expect(calls.deleted).toStrictEqual([1]) + }) + + it('matches the whole range of a multi line comment', async () => { + const issue = issueAt(2, ['x = 1', 'y = 2'], 3) + expect((await run([commentOf(1, issue)], [issue])).deleted).toStrictEqual([]) + const widened = issueAt(1, ['x = 1', 'y = 2'], 3) + const calls = await run([commentOf(1, issue)], [widened]) + expect(calls.posted).toStrictEqual([[expect.objectContaining({ line: 3, start_line: 1 })]]) + expect(calls.deleted).toStrictEqual([1]) + }) + + it('never deletes a comment that was replied to', async () => { + const reply: ReviewComment = { id: 2, body: 'Why?', path: file, line: 2, in_reply_to_id: 1 } + const calls = await run([commentOf(1, issueAt(2, ['x = 1'])), reply], []) + expect(calls.deleted).toStrictEqual([]) + }) + + it('keeps a comment that was replied to even when its finding changed, posting the new one beside it', async () => { + const reply: ReviewComment = { id: 2, body: 'Why?', path: file, line: 2, in_reply_to_id: 1 } + const changed = issueAt(2, ['x = 2']) + const calls = await run([commentOf(1, issueAt(2, ['x = 1'])), reply], [changed]) + expect(calls.posted).toStrictEqual([[expect.objectContaining({ body: bodyOf(changed) })]]) + expect(calls.deleted).toStrictEqual([]) + }) + + it('replaces a comment of an older version, whose tag carries no fingerprint', async () => { + const issue = issueAt(2, ['x = 1']) + const legacy: ReviewComment = { id: 1, body: `\n**Message**\n[warning:test]`, path: file, line: 2 } + const calls = await run([legacy], [issue]) + expect(calls.posted).toStrictEqual([[expect.objectContaining({ body: bodyOf(issue) })]]) + expect(calls.deleted).toStrictEqual([1]) + }) + + it('deletes a comment of an older version whose finding disappeared', async () => { + const legacy: ReviewComment = { id: 1, body: `\n**Message**`, path: file, line: 2 } + expect((await run([legacy], [])).deleted).toStrictEqual([1]) + }) + + it('touches neither comments of other tools nor comments of humans', async () => { + const others: ReviewComment[] = [ + { id: 1, body: '\n**Message**', path: file, line: 2 }, + { id: 2, body: `\n**Message**`, path: file, line: 2 }, + { id: 3, body: 'Looks good to me', path: file, line: 2 }, + { id: 4, body: `Quoting `, path: file, line: 2 } + ] + expect((await run(others, [])).deleted).toStrictEqual([]) + }) + + it('reads a tool name literally rather than as a pattern', async () => { + const issue = issueAt(2, ['x = 1']) + const tool = 'a.b+c' + const foreign: ReviewComment = { id: 1, body: bodyOf(issue, 'aXb+c'), path: file, line: 2 } + const calls = await run([foreign, { ...commentOf(2, issue), body: bodyOf(issue, tool) }], [issue], 10, tool) + expect(calls.posted).toStrictEqual([]) + expect(calls.deleted).toStrictEqual([]) + }) + + it('keeps exactly one comment per identical finding', async () => { + const issue = issueAt(2, ['x = 1']) + const existing = [commentOf(1, issue), commentOf(2, issue)] + const both = await run(existing, [issue, issue]) + expect(both.posted).toStrictEqual([]) + expect(both.deleted).toStrictEqual([]) + const one = await run([commentOf(1, issue), commentOf(2, issue)], [issue]) + expect(one.posted).toStrictEqual([]) + expect(one.deleted).toStrictEqual([2]) + const three = await run([commentOf(1, issue), commentOf(2, issue)], [issue, issue, issue]) + expect(three.posted[0]).toHaveLength(1) + expect(three.deleted).toStrictEqual([]) + }) + + it('still skips a finding whose range leaves the pull request diff', async () => { + expect((await run([], [issueAt(9, undefined, 12)])).posted).toStrictEqual([]) + }) + + it('tells apart the same finding reported on the same line of two files', async () => { + const issue = issueAt(2, ['x = 1']) + const other = { ...issue, path: otherFile } + const both = await run([commentOf(1, issue), { ...commentOf(2, other), path: otherFile }], [issue, other]) + expect(both.posted).toStrictEqual([]) + expect(both.deleted).toStrictEqual([]) + const moved = await run([{ ...commentOf(1, other), path: otherFile }], [issue]) + expect(moved.posted).toStrictEqual([[expect.objectContaining({ path: file, line: 2 })]]) + expect(moved.deleted).toStrictEqual([1]) + }) + + it('posts at most 50 comments', async () => { + const issues = Array.from({ length: 55 }, (_, index) => issueAt(index + 1)) + const calls = await run([], issues, 60) + expect(calls.posted[0]).toHaveLength(50) + expect(warnings).toHaveBeenCalledWith('More than 50 comments detected. Only the first 50 will be posted.') + }) + + it('does not warn about exactly 50 comments', async () => { + const issues = Array.from({ length: 50 }, (_, index) => issueAt(index + 1)) + const calls = await run([], issues, 60) + expect(calls.posted[0]).toHaveLength(50) + expect(warnings).not.toHaveBeenCalled() + }) + + it('counts the comments it keeps against the 50 comment cap', async () => { + const issues = Array.from({ length: 55 }, (_, index) => issueAt(index + 1)) + const existing = issues.slice(0, 48).map((issue, index) => commentOf(index + 1, issue)) + const calls = await run(existing, issues, 60) + expect(calls.posted).toStrictEqual([[expect.objectContaining({ line: 49 }), expect.objectContaining({ line: 50 })]]) + expect(calls.deleted).toStrictEqual([]) + expect(warnings).toHaveBeenCalled() + }) + + it('posts nothing when the comments it keeps already fill the 50 comment cap', async () => { + const issues = Array.from({ length: 55 }, (_, index) => issueAt(index + 1)) + const existing = issues.slice(0, 50).map((issue, index) => commentOf(index + 1, issue)) + const calls = await run(existing, issues, 60) + expect(calls.posted).toStrictEqual([]) + expect(calls.deleted).toStrictEqual([]) + expect(warnings).toHaveBeenCalled() + }) }) describe('sarifFix', () => { diff --git a/dist/index.js b/dist/index.js index ba34e80..bcc7893 100644 --- a/dist/index.js +++ b/dist/index.js @@ -29947,6 +29947,7 @@ exports.failOnIssues = failOnIssues; exports.createSummary = createSummary; const github_1 = __nccwpck_require__(3228); const core_1 = __nccwpck_require__(7484); +const crypto_1 = __nccwpck_require__(6982); const path_1 = __importDefault(__nccwpck_require__(6928)); const parse_diff_1 = __importDefault(__nccwpck_require__(2673)); function* parseRegex(input, regex, levelMap) { @@ -30188,28 +30189,60 @@ function getRegexParser(regex, levelMap) { function getDiffParser(message) { return (input) => parseFormatDiff(input, message === '' ? defaultDiffMessage : message); } -function buildCommentBody(commentTag, identifier, issue) { - const body = `${commentTag}\n**${issue.msg}**\n[${[issue.level, identifier, issue.id, issue.sym].filter((n) => n).join(':')}]`; +function buildCommentContent(identifier, issue) { + const content = `**${issue.msg}**\n[${[issue.level, identifier, issue.id, issue.sym].filter((n) => n).join(':')}]`; if (issue.fix == null) { - return body; + return content; } 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${joinFixLines(issue.fix)}${fence}`; + return `${content}\n${fence}suggestion\n${joinFixLines(issue.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$']}] */ - const octokit = (0, github_1.getOctokit)(githubToken); - (0, core_1.debug)('Deleting old comments'); - const commentTag = ``; +function buildFingerprint(content) { + return (0, crypto_1.createHash)('sha256').update(content).digest('hex').slice(0, 16); +} +function buildCommentTag(identifier, fingerprint) { + return ``; +} +function buildComment(identifier, issue) { + const content = buildCommentContent(identifier, issue); + const fingerprint = buildFingerprint(content); + return { fingerprint, body: `${buildCommentTag(identifier, fingerprint)}\n${content}` }; +} +function buildCommentTagRegex(identifier) { + return new RegExp(`^(?:\r?\n|$)`); +} +function buildCommentKey(commentPath, line, endLine, fingerprint) { + return [commentPath, line, endLine, fingerprint].join('\u0000'); +} +async function listPostedComments(octokit, identifier, owner, repo, prNumber) { + const tagRegex = buildCommentTagRegex(identifier); + const posted = { own: [], replied: new Set(), byKey: new Map() }; for await (const { data: comments } of octokit.paginate.iterator(octokit.rest.pulls.listReviewComments, { owner, repo, pull_number: prNumber })) { - for (const c of comments) { - if (c?.id != null && c?.body?.includes(commentTag)) { - (0, core_1.debug)(`Deleting comment ${c?.id}`); - await octokit.rest.pulls.deleteReviewComment({ owner, repo, comment_id: c?.id }); + for (const comment of comments) { + if (comment?.in_reply_to_id != null) { + posted.replied.add(comment.in_reply_to_id); + } + const match = comment?.id != null ? tagRegex.exec(comment?.body ?? '') : null; + if (match == null) { + continue; } + posted.own.push(comment.id); + const fingerprint = match.groups?.fingerprint; + if (fingerprint == null || comment.line == null) { + continue; + } + const key = buildCommentKey(comment.path, comment.start_line ?? comment.line, comment.line, fingerprint); + posted.byKey.set(key, [...(posted.byKey.get(key) ?? []), comment.id]); } } + (0, core_1.debug)(`Found ${posted.own.length} comments of ${identifier}, ${posted.byKey.size} of which carry a fingerprint`); + return posted; +} +async function addComments(issues, prDiff, githubToken, identifier, owner, repo, prNumber, analysisPath) { + const octokit = (0, github_1.getOctokit)(githubToken); + const posted = await listPostedComments(octokit, identifier, owner, repo, prNumber); const diffLines = parseDiffLines(prDiff); + const kept = new Set(); const comments = []; for (const issue of issues) { (0, core_1.debug)(`Processing issue on ${issue.path}:${issue.line}`); @@ -30221,29 +30254,44 @@ async function addComments(issues, prDiff, githubToken, identifier, owner, repo, (0, core_1.debug)(`Skipping issue on ${issue.path}:${issue.line} because GitHub rejects comments spanning lines outside the PR diff`); continue; } - if (comments.length >= 50) { - (0, core_1.warning)('More than 50 comments detected. Only the first 50 will be posted.'); - break; - } + const commentPath = normalizePath(issue.path, analysisPath); const endLine = issue.eline ?? issue.line; + const { fingerprint, body } = buildComment(identifier, issue); + const existing = posted.byKey.get(buildCommentKey(commentPath, issue.line, endLine, fingerprint))?.shift(); + if (existing != null) { + (0, core_1.debug)(`Keeping comment ${existing} of the issue on ${issue.path}:${issue.line}`); + kept.add(existing); + continue; + } const args = { - path: normalizePath(issue.path, analysisPath), + path: commentPath, side: 'RIGHT', start_side: 'RIGHT', line: endLine, start_line: endLine === issue.line ? undefined : issue.line, - body: buildCommentBody(commentTag, identifier, issue) + body }; (0, core_1.debug)(`Generating comment ${JSON.stringify(args)}`); comments.push(args); } + const budget = Math.max(0, 50 - kept.size); + if (comments.length > budget) { + (0, core_1.warning)('More than 50 comments detected. Only the first 50 will be posted.'); + comments.length = budget; + } + const outdated = posted.own.filter((id) => !kept.has(id) && !posted.replied.has(id)); if (comments.length === 0) { (0, core_1.debug)('No comments to post'); - return; } - (0, core_1.debug)('Sending comments'); - await octokit.rest.pulls.createReview({ owner, repo, pull_number: prNumber, event: 'COMMENT', comments }); - (0, core_1.debug)('Sent comments'); + else { + (0, core_1.debug)('Sending comments'); + await octokit.rest.pulls.createReview({ owner, repo, pull_number: prNumber, event: 'COMMENT', comments }); + (0, core_1.debug)('Sent comments'); + } + for (const id of outdated) { + (0, core_1.debug)(`Deleting comment ${id}`); + await octokit.rest.pulls.deleteReviewComment({ owner, repo, comment_id: id }); + } } async function getPrDiff(githubToken, owner, repo, prNumber) { const octokit = (0, github_1.getOctokit)(githubToken); @@ -30329,7 +30377,7 @@ async function createSummary(issues, identifier, analysisPath) { } exports._testExports = { normalizePath, - buildCommentBody + buildComment }; diff --git a/src/bugalint.ts b/src/bugalint.ts index 98b9f86..f954b86 100644 --- a/src/bugalint.ts +++ b/src/bugalint.ts @@ -1,10 +1,13 @@ import type { Log, Region, ReportingDescriptor, Result } from 'sarif' import { getOctokit } from '@actions/github' import { debug, warning, summary } from '@actions/core' +import { createHash } from 'crypto' import path from 'path' import parseDiff from 'parse-diff' import type { SummaryTableRow } from '@actions/core/lib/summary' +/* eslint camelcase: ["error", {allow: ['^pull_number$', '^comment_id$', '^start_side$', '^start_line$', '^in_reply_to_id$']}] */ + interface Issue { id?: string sym?: string @@ -281,13 +284,77 @@ export function getDiffParser(message: string): Parser { return (input: string) => parseFormatDiff(input, message === '' ? defaultDiffMessage : message) } -function buildCommentBody(commentTag: string, identifier: string, issue: Issue): string { - const body = `${commentTag}\n**${issue.msg}**\n[${[issue.level, identifier, issue.id, issue.sym].filter((n) => n).join(':')}]` +function buildCommentContent(identifier: string, issue: Issue): string { + const content = `**${issue.msg}**\n[${[issue.level, identifier, issue.id, issue.sym].filter((n) => n).join(':')}]` if (issue.fix == null) { - return body + return content } 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${joinFixLines(issue.fix)}${fence}` + return `${content}\n${fence}suggestion\n${joinFixLines(issue.fix)}${fence}` +} + +function buildFingerprint(content: string): string { + return createHash('sha256').update(content).digest('hex').slice(0, 16) +} + +function buildCommentTag(identifier: string, fingerprint: string): string { + return `` +} + +interface Comment { + fingerprint: string + body: string +} + +function buildComment(identifier: string, issue: Issue): Comment { + const content = buildCommentContent(identifier, issue) + const fingerprint = buildFingerprint(content) + return { fingerprint, body: `${buildCommentTag(identifier, fingerprint)}\n${content}` } +} + +function buildCommentTagRegex(identifier: string): RegExp { + return new RegExp(`^(?:\r?\n|$)`) +} + +function buildCommentKey(commentPath: string, line: number, endLine: number, fingerprint: string): string { + return [commentPath, line, endLine, fingerprint].join('\u0000') +} + +interface PostedComments { + own: number[] + replied: Set + byKey: Map +} + +async function listPostedComments( + octokit: ReturnType, + identifier: string, + owner: string, + repo: string, + prNumber: number +): Promise { + const tagRegex = buildCommentTagRegex(identifier) + const posted: PostedComments = { own: [], replied: new Set(), byKey: new Map() } + for await (const { data: comments } of octokit.paginate.iterator(octokit.rest.pulls.listReviewComments, { owner, repo, pull_number: prNumber })) { + for (const comment of comments) { + if (comment?.in_reply_to_id != null) { + posted.replied.add(comment.in_reply_to_id) + } + const match = comment?.id != null ? tagRegex.exec(comment?.body ?? '') : null + if (match == null) { + continue + } + posted.own.push(comment.id) + const fingerprint = match.groups?.fingerprint + if (fingerprint == null || comment.line == null) { + continue + } + const key = buildCommentKey(comment.path, comment.start_line ?? comment.line, comment.line, fingerprint) + posted.byKey.set(key, [...(posted.byKey.get(key) ?? []), comment.id]) + } + } + debug(`Found ${posted.own.length} comments of ${identifier}, ${posted.byKey.size} of which carry a fingerprint`) + return posted } export async function addComments( @@ -300,22 +367,11 @@ export async function addComments( prNumber: number, analysisPath: string ): Promise { - /* eslint camelcase: ["error", {allow: ['^pull_number$', '^comment_id$', '^start_side$', '^start_line$']}] */ const octokit = getOctokit(githubToken) - - debug('Deleting old comments') - const commentTag = `` - for await (const { data: comments } of octokit.paginate.iterator(octokit.rest.pulls.listReviewComments, { owner, repo, pull_number: prNumber })) { - for (const c of comments) { - if (c?.id != null && c?.body?.includes(commentTag)) { - debug(`Deleting comment ${c?.id}`) - await octokit.rest.pulls.deleteReviewComment({ owner, repo, comment_id: c?.id }) - } - } - } - + const posted = await listPostedComments(octokit, identifier, owner, repo, prNumber) const diffLines = parseDiffLines(prDiff) + const kept = new Set() const comments = [] for (const issue of issues) { debug(`Processing issue on ${issue.path}:${issue.line}`) @@ -327,30 +383,46 @@ export async function addComments( debug(`Skipping issue on ${issue.path}:${issue.line} because GitHub rejects comments spanning lines outside the PR diff`) continue } - if (comments.length >= 50) { - warning('More than 50 comments detected. Only the first 50 will be posted.') - break - } + const commentPath = normalizePath(issue.path, analysisPath) const endLine = issue.eline ?? issue.line + const { fingerprint, body } = buildComment(identifier, issue) + const existing = posted.byKey.get(buildCommentKey(commentPath, issue.line, endLine, fingerprint))?.shift() + if (existing != null) { + debug(`Keeping comment ${existing} of the issue on ${issue.path}:${issue.line}`) + kept.add(existing) + continue + } + const args = { - path: normalizePath(issue.path, analysisPath), + path: commentPath, side: 'RIGHT', start_side: 'RIGHT', line: endLine, start_line: endLine === issue.line ? undefined : issue.line, - body: buildCommentBody(commentTag, identifier, issue) + body } debug(`Generating comment ${JSON.stringify(args)}`) comments.push(args) } + + const budget = Math.max(0, 50 - kept.size) + if (comments.length > budget) { + warning('More than 50 comments detected. Only the first 50 will be posted.') + comments.length = budget + } + const outdated = posted.own.filter((id) => !kept.has(id) && !posted.replied.has(id)) if (comments.length === 0) { debug('No comments to post') - return + } else { + debug('Sending comments') + await octokit.rest.pulls.createReview({ owner, repo, pull_number: prNumber, event: 'COMMENT', comments }) + debug('Sent comments') + } + for (const id of outdated) { + debug(`Deleting comment ${id}`) + await octokit.rest.pulls.deleteReviewComment({ owner, repo, comment_id: id }) } - debug('Sending comments') - await octokit.rest.pulls.createReview({ owner, repo, pull_number: prNumber, event: 'COMMENT', comments }) - debug('Sent comments') } export type DiffLines = Record> @@ -445,5 +517,5 @@ export async function createSummary(issues: Iterable, identifier: string, export const _testExports = { normalizePath, - buildCommentBody + buildComment } From e39a73849eb242c1ba660a7eb92329b93a914e89 Mon Sep 17 00:00:00 2001 From: Bugale Date: Mon, 27 Jul 2026 09:47:38 +0300 Subject: [PATCH 11/11] fix: report how many comments the 50 comment cap actually drops Counting kept comments against the cap made the existing warning untrue: a run that keeps 30 comments posts at most 20 while still claiming that only the first 50 will be posted. Report the kept count, the posted count and the total instead, so the reader can tell how much was dropped. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- __tests__/bugalint.test.ts | 6 +++--- dist/index.js | 2 +- src/bugalint.ts | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/__tests__/bugalint.test.ts b/__tests__/bugalint.test.ts index 6b0cbec..58c0897 100644 --- a/__tests__/bugalint.test.ts +++ b/__tests__/bugalint.test.ts @@ -411,7 +411,7 @@ describe('commentReconciliation', () => { const issues = Array.from({ length: 55 }, (_, index) => issueAt(index + 1)) const calls = await run([], issues, 60) expect(calls.posted[0]).toHaveLength(50) - expect(warnings).toHaveBeenCalledWith('More than 50 comments detected. Only the first 50 will be posted.') + expect(warnings).toHaveBeenCalledWith('More than 50 comments detected. Keeping 0 already posted comments and posting 50 of the 55 new ones.') }) it('does not warn about exactly 50 comments', async () => { @@ -427,7 +427,7 @@ describe('commentReconciliation', () => { const calls = await run(existing, issues, 60) expect(calls.posted).toStrictEqual([[expect.objectContaining({ line: 49 }), expect.objectContaining({ line: 50 })]]) expect(calls.deleted).toStrictEqual([]) - expect(warnings).toHaveBeenCalled() + expect(warnings).toHaveBeenCalledWith('More than 50 comments detected. Keeping 48 already posted comments and posting 2 of the 7 new ones.') }) it('posts nothing when the comments it keeps already fill the 50 comment cap', async () => { @@ -436,7 +436,7 @@ describe('commentReconciliation', () => { const calls = await run(existing, issues, 60) expect(calls.posted).toStrictEqual([]) expect(calls.deleted).toStrictEqual([]) - expect(warnings).toHaveBeenCalled() + expect(warnings).toHaveBeenCalledWith('More than 50 comments detected. Keeping 50 already posted comments and posting 0 of the 5 new ones.') }) }) diff --git a/dist/index.js b/dist/index.js index bcc7893..00f8f86 100644 --- a/dist/index.js +++ b/dist/index.js @@ -30276,7 +30276,7 @@ async function addComments(issues, prDiff, githubToken, identifier, owner, repo, } const budget = Math.max(0, 50 - kept.size); if (comments.length > budget) { - (0, core_1.warning)('More than 50 comments detected. Only the first 50 will be posted.'); + (0, core_1.warning)(`More than 50 comments detected. Keeping ${kept.size} already posted comments and posting ${budget} of the ${comments.length} new ones.`); comments.length = budget; } const outdated = posted.own.filter((id) => !kept.has(id) && !posted.replied.has(id)); diff --git a/src/bugalint.ts b/src/bugalint.ts index f954b86..7823e3e 100644 --- a/src/bugalint.ts +++ b/src/bugalint.ts @@ -408,7 +408,7 @@ export async function addComments( const budget = Math.max(0, 50 - kept.size) if (comments.length > budget) { - warning('More than 50 comments detected. Only the first 50 will be posted.') + warning(`More than 50 comments detected. Keeping ${kept.size} already posted comments and posting ${budget} of the ${comments.length} new ones.`) comments.length = budget } const outdated = posted.own.filter((id) => !kept.has(id) && !posted.replied.has(id))