feat: render SARIF fixes and formatter diffs as GitHub suggestions

[bugale]

Adds native support for GitHub suggested changes, driven by standard SARIF fixes, plus a diff input format that produces them from any formatter that can rewrite files in place.

Motivation is a clang-format lint over a large C++ repo: clang-format knows the exact fix for every violation, but bugalint had no way to express one. Embedding a suggestion block in the message does not work — the body template is **${msg}**, so a message ending in a fence produces ```**, the fence never closes, and GitHub swallows the rest of the body into the suggested code.

feat: render SARIF fixes as GitHub suggestions

• Issue gains an optional fix, read by parseSarif from fixes[0].artifactChanges[0].replacements[0].insertedContent and re-emitted by generateSarif, so the parse/generate round trip stays lossless and code scanning receives the fix too.
• addComments appends the suggestion block after the identifier line. Being last is what makes the body well formed — nothing follows the closing fence.
• An empty insertedContent.text renders as an empty suggestion, which is how GitHub expresses a deletion. Rendering it as a block containing one blank line would instead leave a stray blank line behind on every deletion.
• The fence grows past the longest backtick run in the fix, so code containing a fence still renders.

The fix's deletedRegion is what decides whether the inserted text ends with a line terminator, so taking the text verbatim would be wrong in one direction or the other. Per the SARIF specification an absent endColumn means the end of the text of endLine, so the two usual spellings of a whole line replacement differ by exactly one line terminator:

deletedRegion for lines 3–4	covers	insertedContent.text
{startLine: 3, endLine: 4}	the text of both lines, not the terminator ending line 4	must not end with a newline
{startLine: 3, startColumn: 1, endLine: 5, endColumn: 1}	the same lines including that terminator	must end with a newline

So the second form has its single trailing newline removed and the first is taken verbatim. Trimming unconditionally would drop a meaningful trailing empty line; not trimming at all appends a spurious one. Both are expressible, and a producer using either spelling gets the same suggestion.

The region is also checked against the result's own region, and a fix is ignored when it cannot be rendered as a whole line replacement of the anchored lines. Without that, an ESLint- or Semgrep-style character precise fix — say deletedRegion covering columns 5–7 with text === — would render as a suggestion replacing the entire line with ===. The issue is still commented on, just without a suggestion.

feat: match multi line issues that partially overlap added lines

isNewIssue required every line of the range to be an added line. Formatting fixes routinely span a continuation line the PR did not touch, and those were silently dropped from both the comments and the failure count — in the repo that motivated this, 35% of results span three or more lines.

It now matches when any line in the range was added. parseAddedLines becomes parseDiffLines and records context lines too (true = added, false = context). That keeps the same shape and a single pass over the diff, while enabling the second check: addComments now skips issues spanning a line outside the diff. That guard is not optional — all comments go out in one createReview, so a single out-of-hunk anchor returns 422 and loses the whole batch.

Behaviour change worth your attention: this affects onlyNew (failOnlyNew at this point in the branch) for every consumer, not only SARIF ones, since a multi line issue that partially overlaps added lines now counts. I left it as a plain feat — say the word if you would rather it be breaking, or want it split into its own PR.

feat: support converting a formatter diff to suggestions

The two commits above only let a linter that already speaks SARIF carry a fix. This one removes that requirement: a new diff input format reads the output of git diff, so clang-format -i followed by git diff is enough to get suggested changes, with no bespoke converter in between. Detecting changes on disk and turning them into SARIF is squarely bugalint's job, and the suggestion machinery was already parser agnostic, so this is one Parser plus wiring. Everything downstream — the SARIF uploaded to code scanning, the job summary, onlyNew — works unchanged.

Three things it has to get right:

• One issue per contiguous run of changed lines, not per hunk. git diff prints three lines of context around each change; anchoring on the hunk would widen every range by up to six lines and push it outside the pull request's diff. One real file in the consuming repo has 38 hunks but 56 change runs.
• Anchoring on the old side. The formatted text exists nowhere in the pull request. The old side is the committed file, which is what GitHub shows and what a comment can attach to, so line/eline come from del.ln and normal.ln1, and the new side becomes the fix.
• Runs that only add lines. They have no old line to anchor to, and a suggestion cannot attach to nothing, so the range is extended to a neighbouring line — the preceding one, or the following one at the top of a file — whose content is repeated in the fix.

Two details that only show up on real input. git diff emits \ No newline at end of file as an ordinary change with a duplicated line number, so it has to be filtered or the marker text lands inside the suggestion. And index.ts stripped every \r before parsing, which is right for the existing formats but not here, where a carriage return can be content: rewriting CRLF to LF would make a reviewer clicking Commit suggestion commit mixed line endings. Whether a diff is CRLF terminated or merely describes CRLF content is decided by looking at whether git's own header lines end with \r, which is unambiguous and handles both together.

A new message input is appended to the message of every issue, and so becomes the whole message of a format like this one that carries neither a message nor a level. It is empty by default, since a message that applies to every issue equally is workflow-specific and a generic default would just be noise on every comment.

Worth knowing: a formatter that crashes without writing anything produces an empty diff, which is indistinguishable from a clean run. The workflow has to make the formatter step fail by itself — noted in the README.

Blanking a line is not deleting it

The first version of this held a fix as a single string, and a consuming session running the shipped bundle over a real repository found what that costs. The lines to substitute were recovered by splitting on newlines — and [''].join('\n') is '', exactly like [].join('\n'). An empty fix renders as an empty suggestion, which GitHub applies as a deletion. A formatter stripping the whitespace of a blank line produces precisely that diff (-␠␠␠␠ / +), so the suggestion removed the line instead of blanking it.

The oracle is the convincing part: applying every suggestion and comparing byte-for-byte against the formatter's own output gave 707 of 743 files correct with the string representation, and 743 of 743 with a list of lines — the 36 failures being exactly the files containing those 51 blocks.

Worth saying why 0.6% of results justified a representation change. A formatter never inserts blank lines, so a reviewer who applies the bad suggestion is left with a file that is still clean by the formatter's own standard: the check goes green, there is no oscillation to notice and no red build, and the separator line is simply gone. Nothing downstream can catch it. A suggestion that quietly does something other than what it renders is the one failure mode this feature cannot have.

So Issue.fix is a string[]: no lines means delete, one empty line means blank. Nothing about the input formats changes, and the action's inputs are untouched. The SARIF input path had the identical defect and is closed by the same change — a producer expressing "blank these lines" writes a fix whose text is a single line terminator over a terminator-inclusive deletedRegion, and stripping that trailing newline arrives at the empty string as well.

That has one consequence for the SARIF written out. The two deletedRegion spellings are not equally expressive here — the terminator-exclusive one writes both "delete" and "blank" as an empty text, so only the terminator-inclusive one can carry the distinction. Emitted fixes therefore use it uniformly, which is what makes reading back a bugalint-generated SARIF lossless. I verified that directly: every fixture, re-parsed and re-generated, is deep-equal to itself. Both spellings are still accepted on input, and the README says plainly that a producer restricted to the terminator-exclusive one has to widen the range to express a blank line.

GitHub cannot render a one-blank-line suggestion either

Distinguishing the two in Issue.fix and in the emitted SARIF does not, on its own, fix what the reviewer sees, because GitHub's own suggestion renderer has the same bug — and I only learned that by measuring it. Two sessions posted suggestion comments on real pull requests, varying only the fence content, and read the rendered body_html back. Independently reproduced on this PR:

fence content	addition rows drawn
"" (delete)	0
"\n" (blank one line)	0 — identical to a deletion
"\n\n"	2
" \n"	1
"\nint x;\n"	2

So it is not a renderer that cannot draw empty rows — rows 3 and 5 each draw one. One rule fits every point: strip one trailing newline, then test for empty. "\n" collapses onto "" and the block is treated as having no lines at all. That is the same order-of-operations mistake the SARIF reader had to avoid; GitHub's copy is in their renderer, so the conclusion is structural rather than a bug on our side: the ```suggestion wire format cannot express "replace these lines with exactly one empty line". No encoding fixes it. I checked the stored bytes at both ends before accepting that — GitHub keeps the extra 0a faithfully, so nothing is lost in transit, and our producer was already correct.

The consequence is that the narrow suggestion is not merely unhelpful, it is wrong: it renders as deleting a line the formatter asked to keep, indistinguishably from a genuine deletion. So a fix that is exactly one empty line is extended to a neighbouring line, preferring the preceding one — the same borrow a run of pure insertions already needs, and shared with it, so there is exactly one place where an anchor can move. The borrowed line is a context line of the formatter's own diff, and GitHub's hunks carry three lines of context, so it falls outside the pull request diff essentially only when the blanked line is the first line of the file — where there is no preceding line to borrow anyway and it extends forward instead. When there is neither, as in a file that is a single blank line, the issue is reported without a fix, which is the right end of the trade: possibly dropped beats actively wrong.

This is also why the list representation is load-bearing rather than superseded: you cannot decide to widen if you cannot tell [] from ['']. Every other shape is untouched — ['x',''], ['','x'] and ['',''] all render correctly, and are covered. The same guard sits at the comment boundary, so a lone-newline fix arriving through the SARIF input path cannot render as a deletion either.

What I could not measure is what Apply writes, since no API applies a suggestion — that needs a human click. It does not change the decision: the rendered preview is all a reviewer sees before approving, and it currently reads "delete this line".

A change of a line terminator alone yields no fix

The other half of the end-of-file marker handling, found while checking a claim about how reviewdog treats the same case.

A formatter normally terminates the last line of a file that does not end with a newline. When that is the only thing it changes, git diff still prints a deleted and an added line, because the bytes differ — but the two lines have identical text:

-  int last = 0;
\ No newline at end of file
+  int last = 0;

Filtering the marker therefore leaves a fix that replaces a line with itself. GitHub renders it happily, and clicking Commit suggestion cannot change anything, so the comment returns on the next run and no reviewer can ever resolve it by clicking. I confirmed it by running the shipped bundle on that diff: the emitted insertedContent.text came out byte-identical to the line it replaced.

A run whose old and new lines are identical therefore yields the issue without a fix. The line is still reported and the step still fails; the reviewer runs the formatter instead of clicking. That is deliberately the option that assumes nothing about how GitHub applies a suggestion.

Worth being plain about what that costs, because it is not free. The comment sits on a line that is visibly formatted correctly, carries no suggestion, and gives no hint that the difference is an invisible missing newline at the end of the file. In diff mode the message is workflow level and identical for every result, so nothing distinguishes this case from an ordinary one, and a reviewer could reasonably read it as a false positive. Whether the answer is to emit a suggestion after all or to let the message carry the explanation depends on the same unmeasured thing — whether GitHub terminates the last line of an applied suggestion — so it is left until that is known rather than guessed. I have not put a frequency on it either: the affected set is files where the terminator is the only difference, which is a subset of the files lacking one, and nobody has counted it.

reviewdog instead appends an empty line to the suggestion, on the theory that GitHub reads it as the missing terminator rather than as a new blank line (parser/diff.go, hunk.EOFNewline == diff.LineAdded). That would make the comment resolvable by clicking, and it may well be right — but the evidence for it is one manual observation, linked from a source comment that hedges itself with "this is known to work with GitHub review suggestions, at least". There is a test, TestSuggestionContainsEofNewline, and it is worth being precise about what it pins: that reviewdog emits the trailing blank line, not that GitHub reads that blank line as a terminator. No API applies a suggestion, so the second half is not testable by anyone. Two of the bugs fixed in this PR came from exactly that kind of plausible assumption, so this sidesteps the bet rather than taking the other side of it. Say the word if you would rather have it.

feat!: filter out old issues before generating any output

The only breaking commit here, and the one to look at first.

failOnlyNew narrowed the failure and nothing else. A run with it set still wrote every issue to the SARIF, the log, the summary and the comments — only the exit code reflected the filtering, and the same input was parsed four separate times to do it. That split gets worse with the commits above: a diff of a whole formatted repository is thousands of issues, of which a handful are on code the pull request touched, and the summary would list all of them.

So the input is renamed to onlyNew and applied once, up front. Everything downstream receives the same filtered list, the diff is fetched at most once, and the input is parsed once.

This is a breaking change in two ways, which is why it is feat! — though the classification is yours to make:

• failOnlyNew no longer exists. GitHub does not reject a with: key the action never declared, but it is not silent about it either: the runner emits an Unexpected input(s) 'failOnlyNew', valid inputs are [...] warning annotation that names onlyNew among the valid set (actions/runner, ActionRunner.cs). The input is still dropped, so onlyNew falls back to its false default and the run considers every issue — with fail defaulting to true, a stale workflow fails on the whole corpus rather than quietly narrowing.
• With onlyNew set, the SARIF now contains only the new issues. Uploading it to code scanning therefore resolves the alerts of the unchanged code, which is the opposite of what a repository-wide scan wants. The README says so next to the input, and the fix is to leave onlyNew unset on the scanning run.

addComments keeps its own two guards regardless of onlyNew, since a comment still cannot be anchored outside the pull request's diff.

Migrating off the old name

A workflow left on failOnlyNew does not fail open — addComments keeps its own guards, so the comments stay correct. It fails closed: failOnIssues no longer filters anything, so the step fails on every pre-existing issue, and a repository-wide formatter check turns every pull request red, including ones touching none of the linted files. Loud rather than silent, which is the better of the two, but the message says found 8238 issues and so points the reader at their code rather than at their workflow.

The rename cannot be guarded in either direction anyway. A version that predates onlyNew ignores it in silence and fails on every issue, and no code in this PR can reach that consumer. So the README says to move the pin and rename the input in the same commit, which is the only instruction that covers both halves.

fix: decode a pull request diff returned as a buffer

Found by a consuming session while building a mock of the GitHub API, and then measured against the real one rather than left as a hypothetical. This one is pre-existing and shipped — the same code is in v4.0.0 — but it is in here because the breaking commit above makes its consequence considerably worse.

Octokit reads a response as text only when the content type matches /^text\/|charset=utf-8$/. That is case-sensitive and end-anchored:

content type	routed as
application/vnd.github.v3.diff; charset=utf-8	text
application/vnd.github.v3.diff; charset=UTF-8	buffer
application/vnd.github.v3.diff; charset=utf-8; boundary=x	buffer
application/vnd.github.v3.diff	buffer

GitHub currently sends the lowercase spelling, so this is latent rather than live — but UTF-8 is the RFC-canonical casing, and any trailing parameter defeats the anchor too. getPrDiff cast the result straight to a string with as unknown as string, which is what let it through the type checker.

The consequence is total and silent. parseDiff on a cast ArrayBuffer returns zero files, with no error, so every issue fails isNewIssue and is dropped with only a debug line. Measured on a real 5,010-file repository by the consuming session, varying nothing but the header:

charset=utf-8  ->  exit 1, 50 comments, 7724 issues reported
charset=UTF-8  ->  exit 0, 0 comments, 0 errors, 0 warnings

A pull request with 7,724 genuine findings passes clean.

Why it belongs in this PR rather than a follow-up. At v4.0.0 the SARIF is generated and written before the diff is fetched at all — generateSarif runs on the unfiltered issues, and getPrDiff is not called until several lines later. A transport failure there cannot reach the SARIF; it loses the comments and, with failOnlyNew, the failure. The onlyNew commit moves filtering ahead of generateSarif, so the same transport failure now produces an empty SARIF.

To be precise about the consequence, since it is easy to overstate: bugalint only writes that file, and only when sarif is set — it never uploads to code scanning itself. For a consumer that wires the file into upload-sarif, an empty SARIF resolves alerts that are still genuinely present. For a consumer that does not, the loss is confined to comments and the failure. Either way my change converts a silent drop into something strictly worse, so the guard against it is coupled to this branch rather than incidental to it.

The two failure directions are worth separating, because they are not equally bad:

configuration	effect of a diff that parses to nothing
comment only	comments silently vanish, but every issue still reaches failOnIssues, so the job still goes red
onlyNew (or failOnlyNew before it)	issues filter to empty, so the job fails open — a green check over a broken tree

A consuming session confirmed this against a real repository on both v2.3.0 and v4.0.0, whose bundles carry the identical guard at the identical line: comment: true alone is enough to reach the bug.

Decoding the buffer removes the failure rather than reporting it, so no warning is needed in the normal case, and a response that is neither text nor bytes throws instead of matching every issue against nothing. The decode is extracted so it is unit-tested, since getPrDiff itself needs the API.

fix: fail on a pull request diff that cannot be parsed

Decoding fixes the failure that was measured, but it can only guarantee that the bytes became a string, not that the string is a diff. A proxy or gateway answering with an HTML error page still parses to zero files, and the outcome is the one above: every issue treated as not being part of the pull request, no comment, green step. Note that text/html; charset=utf-8 matches the ^text/ clause, so this case decodes perfectly and the decode cannot catch it.

The reason to validate the outcome instead of tightening the content type check is that the header carries no signal a generic sniffer can use. application/vnd.github.v3.diff is a vendor media type, and the consuming session found that PowerShell's HTTP client classifies it as bytes even with charset=utf-8 present, because it ignores the charset parameter entirely. Octokit's charset=utf-8$ clause is what rescues the type rather than what breaks it. Making that regex case-insensitive or unanchored would close the two spellings in the table above and leave the class open; checking what came out of the parse closes the class.

Failing rather than warning is the part a consuming session measured and pushed back on — with a warning, a pull request carrying 7,724 genuine findings still passed, twice warned and green. They were right, and the reason is stronger than a preference about severity. One commit earlier, a response arriving as an object throws. A response arriving as text that is not a diff has precisely the same consequence, and would merely be logged. The same failure would be fatal or silent depending on nothing but the JavaScript type the response happened to have, which is not a distinction worth making.

Beyond consistency: where the diff decides which issues are new, it is not decoration but the filter. A diff that cannot be parsed leaves no basis for the statement "no new issues", so passing the step reports a conclusion that was never computed. Failing says "I could not check", which is true; warning says "I checked and it is clean", which is not.

A pull request that genuinely changes nothing sends an empty diff rather than an unparsable one, which is what keeps the check quiet in the legitimate case. Failing also collapses the duplicate report for free — the diff is parsed once to filter the issues and once to place the comments, so a warning fires twice per run and reads as two incidents.

ci: include the Jest checks in the required check group

Unrelated to the feature, and separable if you would rather have it on its own — but it is the reason I would not trust this PR's own green checks without it.

The only status check required to merge is Required Checks, which aggregates the repository's other checks by matching their names against GitHub Action.*, Check.* and .*[lL]int.*. The job that runs the tests is named Jest, which matches none of the three, and bugroup-checks treats a check outside its list as one that was not scheduled to run — so it succeeds however the tests ended.

A pull request whose tests fail, or whose test file fails to compile and therefore runs no test at all, shows a single red check that nothing enforces, while the required check stays green and branch protection on main allows the merge. Adding Jest.* closes it; I checked that every matrix variant (Jest (ubuntu-latest) and friends) matches, and that Auto-merge, Release and the aggregating job itself still do not.

Verified against a real repository

Everything above is pinned by fixtures, but the branch has also been run end to end by a consuming session over a 5,010 file C++ tree, against the real output of clang-format -i. Reporting it because the interesting numbers are the ones that did not move.

	before the blank line fix	after it
results	8,238	8,238	unchanged
files	743	743	unchanged
fix is empty, i.e. delete	423	372	−51
fix is a single empty line, i.e. blank	0	51	+51
files containing a blanked line	36	36	unchanged

The decomposition is the proof rather than the totals: exactly 51 results moved from the deletion bucket to the blanking bucket, the 372 genuine deletions stayed deletions, and no result appeared, vanished or changed anchor. Alongside it, on the same sweep: no end-of-file marker text reached a suggestion, no fix contained a carriage return, and all 8,238 emitted regions used the terminator-inclusive spelling. The blank-line defect was then confirmed on a throwaway pull request against that repository, on a comment this action itself posted in diff mode: the stored body is a fence, an empty line and a fence, and GitHub drew zero addition rows, while the two neighbouring comments in the same request drew three and one.

Two things were measured there rather than assumed. The terminator-only case is a no-op on that repository — 0 of 8,238 results came back without a fix, which is what an LF-only tree should give, but it is now measured, so it cannot regress them. And the summary bound turned out to be worth knowing: the unfiltered summary for all 8,238 issues is 0.99 MB against GitHub's 1 MiB cap, about 126 bytes per issue, so that repository was within one percent of losing its summary entirely. Filtering up front is what puts it back in reach, which is a benefit of the breaking commit I had not expected to be able to point at.

One limit of that repository as a witness, since it is the one consumer this was measured on: it exercises the 50-comment cap rarely. Issues per file run p50 4, p90 24, p99 126, max 463, so 26 of 743 files individually exceed the cap while holding 34% of all issues. When it does fire, only the comments truncate — failOnIssues still receives the complete filtered list, so the verdict and its count are never capped, only the view.

Testing

• sariffix fixture pair plus a CI matrix entry, covering a single line fix, a multi line fix, a deletion, a fix containing a fence, a result with no fix, a region including the line terminator, a replacement ending in an empty line, a blanked line, a part-of-a-line fix and a fix pointing at the wrong lines. The blanked line sits next to the deletion on purpose: they are the pair the string representation could not tell apart, so the fixture fails if it ever regresses.
• diff fixture pair plus a CI matrix entry, covering a single line replacement, a multi line replacement, a pure deletion, an insertion in the middle of a file, an insertion at the top of a file, a replacement containing a fence, a blanked line, a file that does not end with a newline, a file whose only change is that missing terminator, and a blanked line with no neighbouring line to extend to.
• diffcrlf fixture pair plus a CI matrix entry, covering a diff of CRLF content end to end. The unit tests reach parseFormatDiff directly, so nothing covered the one line in index.ts that decides not to strip carriage returns for this format — it reads like a redundant special case, and deleting it silently rewrites the line endings of every suggestion. Removing it now changes that fixture's output. The fixture is marked -text, since normalizing it on commit would turn it into an ordinary diff and make it pass either way, which is the same class of invisible failure.
• Unit tests on the comment body (empty suggestion, preserved trailing empty line, fence growing), on the region handling (both spellings, both trailing-empty-line cases, both ignored cases) and on the diff format (old side anchoring, both insertion directions, the end-of-file marker, and carriage returns as content versus as terminators).
• The guard that stops a lone-newline fix rendering as a deletion is covered from the SARIF side specifically, since the diff path can no longer produce that shape and the guard would otherwise read as dead code to anyone reasoning from the diff path alone. Mutation-tested: removing it fails two tests, and the second names the input that gets it wrong.
• Updated the isNewIssue tests, added isCommentableIssue ones, and replaced the failOnIssues diff tests with direct filterNewIssues ones.
• The pylintonlynew CI matrix entry has its own fixture pair, since its output is filtered to nothing — previously it reused pylint's input against noissues' expectation through an extra matrix key, which made the entry's expected output depend on a key rather than on its own name.
• npx eslint ., npx jest (54 passing) and npm run package are all clean at every commit on the branch, not only at the head, and every fixture round-trips through the SARIF parser unchanged.

Draft on purpose — please leave it as a draft until the consuming PR has been verified end to end against it.