Skip to content

feat: render SARIF fixes and formatter diffs as GitHub suggestions - #149

Draft
bugale wants to merge 9 commits into
mainfrom
bugale/sarif-suggestions
Draft

feat: render SARIF fixes and formatter diffs as GitHub suggestions#149
bugale wants to merge 9 commits into
mainfrom
bugale/sarif-suggestions

Conversation

@bugale

@bugale bugale commented Jul 25, 2026

Copy link
Copy Markdown
Owner

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 allgenerateSarif 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.

@bugale bugale changed the title feat: render SARIF fixes as GitHub suggestions feat: render SARIF fixes and formatter diffs as GitHub suggestions Jul 26, 2026
@bugale
bugale force-pushed the bugale/sarif-suggestions branch from e7f37f0 to 0b0bc13 Compare July 27, 2026 18:29
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.

A fix is 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>
@bugale
bugale force-pushed the bugale/sarif-suggestions branch from f05eb97 to 037ed03 Compare July 27, 2026 21:24
Comment thread .github/workflows/check-code.yml Outdated
Comment thread .github/workflows/check-code.yml Outdated
Comment thread .gitattributes
Comment thread src/index.ts Outdated
Comment thread src/index.ts Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
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 change of that terminator alone leaves the old and the new lines identical, so the

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?
If a formatter added a newline to the end of a file that didn't have it, I think we should report it.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BEEP BOOP! I am Copilot using Bugale's account: It is reported — the issue, the log line, the SARIF result, the summary row and the step failure all happen exactly as for any other change. The only thing dropped is the suggestion, and the README sentence is about the fix, not about the report. I'll reword it if it reads as "not reported".

The reason is that such a run has nothing to suggest. Git prints it as:

-};  //  namespace sent
\ No newline at end of file
+};  //  namespace sent

Both sides carry identical text — the only difference is the terminator, and the \ No newline at end of file marker that carries it is filtered out just above (it arrives as an ordinary change object with a duplicated line number, and left in, its text would land inside the suggestion).

So the fix would replace the line with itself. GitHub renders that suggestion, a reviewer clicks Commit suggestion, nothing changes, and the comment returns on the next run with no action that can resolve it.

There is a version of this that would work — appending an empty line and relying on GitHub to terminate the last line of an applied suggestion, which is what reviewdog does — but I could not verify that behaviour: there is no API for applying a suggestion, so it needs a human click to settle. Guessing at it is what produced the blank-vs-deletion bug further up this PR, so I left the line reported and the suggestion off. Say the word and I'll take the reviewdog approach.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do what reviewdog does

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BEEP BOOP! I am Copilot using Bugale's account: Done, in e8b2ec4.

I read reviewdog's rule at the source rather than copying the behaviour by description — reviewdog/reviewdog, parser/diff.go, master:

107  if hunk.EOFNewline == diff.LineAdded {
108      // Adding a blank line here should prompt an EOF newline
109      // to be inserted (rather than a complete blank line).
110      // This is known to work with GitHub review suggestions, at least.
113      state.newLines = append(state.newLines, "")
114      // NOTE: this doesn't handle the case of a deleted eof newline
115      // because it's much rarer in practice.

So a trailing empty line in the suggestion body asks GitHub for a newline at the end of the file rather than for a blank line, and a terminator-only change now gets a suggestion whose text repeats the line and adds that empty line, instead of no fix at all. joinFixLines terminates every line rather than joining between them, so ['int a = 0;', ''] renders as int a = 0;\n\n — the line, then the empty one — which is exactly reviewdog's shape.

Two details worth stating, since I took its scope as well as its trick:

  • It appends the empty line after the hunk loop, to the run still open at the end of the hunk, so only a run at EOF is affected. Ours is the same: the marker can only occur at EOF, so the run being extended is necessarily the last one.
  • Its NOTE at 114-115 is deliberate — a removed EOF newline is not handled. We match that, because no suggestion can express "drop the final newline"; that case is still reported without a fix.

The marker git diff prints inherits the type of the change before it, which gives four shapes, all covered by unit tests:

markers in the chunk meaning fix
on the - side only the formatter added the terminator text + a trailing empty line
on the + side only the terminator was removed none
on both sides neither side ends with a newline, so it is unchanged text only, marker excluded
none ordinary change text only

Two results in __tests__/diff.output.json moved: src/eof.c gains the empty line in its insertedContent (" int last = 0;\n" -> " int last = 0;\n\n"), and src/term.c — the terminator-only case that previously had no fix — gains a whole fixes block. Result count is unchanged at 10.

64 tests, lint clean, dist/ rebuilt and byte-reproducible.

Comment thread README.md
bugale and others added 6 commits July 28, 2026 01:03
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>
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 whose text is appended to the message of every
issue, and which is therefore the whole message of a format that carries
none.

A fix is held as the list of lines it replaces the reported ones with
rather than as a single string, so replacing them with nothing and
replacing them with one empty line are different values. Recovering the
lines by splitting a string tells the two apart nowhere, 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. Both values 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, so fixes are written out that way, which is what
makes the round trip through a generated SARIF file lossless.

GitHub's suggestion parser strips one trailing newline from the block and
then tests it for emptiness, so a suggestion whose whole content is one
empty line renders, and applies, exactly like an empty one. Measured by
posting suggestions on a real pull request and reading their rendered HTML
back: a content of "\n" draws zero addition rows, like "", while "\n\n"
draws two, " \n" draws one, and "\nint x;\n" draws two, so this cannot be
worked around by encoding it differently. Such a fix is extended to a
neighbouring line as well, which is the same borrow a run of pure
insertions already needs and is shared with it. When there is no
neighbour, as in a file that is a single blank line, the issue is reported
without a fix rather than with a suggestion that deletes a line the
formatter asked to keep, and the generated SARIF carries the widened,
still exact, replacement in the former case and no fix in the latter. A
suggestion whose text reduces to a lone newline is also dropped at the
comment boundary itself, so a fix read from a SARIF file cannot render as
a deletion either.

A run that only terminates the last line of a file not ending with a
newline leaves the old and the new lines identical, so it is reported
without a fix rather than as a suggestion replacing a line with itself,
which cannot be applied to any effect and leaves behind a comment that no
reviewer can resolve by clicking it.

The fixtures cover a diff of CRLF content, whose structural lines are
terminated by a newline alone while its content lines carry a carriage
return, because the decision to read this one input byte for byte lives in
`index.ts`, which the unit tests do not reach at all. That fixture needs
`-text` to survive being committed, as normalization would turn it into an
ordinary diff and make it pass either way. They also cover blanking a line
through the SARIF path, which carried the identical defect, and the
comment guard is covered by feeding it a fix read from a SARIF file, since
the diff path can no longer produce one and the guard would otherwise read
as removable to anyone reasoning from that path alone.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
`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>
Octokit reads a response as text only when its content type starts with
text/ or ends with charset=utf-8, matched case sensitively. GitHub sends
application/vnd.github.v3.diff; charset=utf-8, which passes, but the
uppercase UTF-8 spelling is the canonical one and any trailing parameter
also defeats the anchored match. In those cases the diff arrives as an
ArrayBuffer, which was cast straight to a string.

Parsing that cast value yields no files at all, with no error, so every
issue is treated as absent from the pull request. The comments are then
all dropped and the step passes, reporting nothing on a pull request that
may carry thousands of findings.

Reporting this rather than only guarding it, because the filtering commit
made the consequence worse: the issues are now filtered before the SARIF
is generated, so an undecoded diff writes an empty SARIF, and a workflow
uploading that file to code scanning closes the alerts that are still
present, where before the written file was complete regardless.

Decoding the buffer removes the failure rather than reporting it, and a
response that is neither text nor bytes now throws instead of silently
matching every issue against nothing.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Decoding the response guarantees that the bytes became a string, not that
the string is a diff. A proxy answering with an error page, or any future
variant of the same content type confusion, still parses to no file at
all, and the result is a silent one: every issue is treated as not being a
part of the pull request, no comment is posted and the step passes.
Validating the outcome rather than the content type covers that class
without having to enumerate it.

A response that arrives as an object already fails the step, because
nothing can be matched against a diff that is not there. A response that
arrives as text but is not a diff has exactly the same consequence, so
warning about it alone would leave the same failure fatal or silent
depending on nothing more than the type the response happened to have.

Reporting it is not enough on its own either. Where the diff is used to
decide which issues are new, it is not decoration but the filter, so a
diff that cannot be parsed leaves no basis for saying that no new issue
was found. Passing the step then reports a conclusion that was never
computed, which is the failure that is worth avoiding rather than the one
that is worth logging.

A pull request that genuinely changes nothing produces an empty diff
rather than an unparsable one, so requiring the diff to be non-empty keeps
that case quiet. Failing also collapses the duplicate report, since the
diff is parsed once to filter the issues and once to place the comments.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The only status check required to merge is Required Checks, which
aggregates the other checks of the repository by matching their names
against GitHub Action.*, Check.* and .*[lL]int.*. The job running the
tests is named Jest, which matches none of those, and the aggregating
action treats a check outside its list as one that was not scheduled to
run, so it succeeds no matter how 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 the branch protection
of the default branch allows the merge.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@bugale
bugale force-pushed the bugale/sarif-suggestions branch from 037ed03 to 54da279 Compare July 27, 2026 22:17
SARIF requires endLine to be at least startLine, and no comment anchor can express an inverted range. Every producer that supplies one - the SARIF region, the pylint endLine and the eline capture group - now has it dropped at parse time, so the issue is anchored on its start line alone and the generated region and deletedRegion are both well formed.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@bugale
bugale force-pushed the bugale/sarif-suggestions branch from 16ac5c3 to b0bc650 Compare July 28, 2026 07:27
A formatter that only terminates the last line of a file produces a diff
whose old and new lines are identical, which was reported without a fix
because a suggestion repeating the line changes nothing. A suggestion can
express it after all, the way reviewdog does: a trailing empty line in the
suggestion body asks GitHub for a newline at the end of the file rather
than for a blank line, so the fix is the changed lines followed by an
empty one.

The terminator is taken to have been added when the marker git diff prints
appears only on the old side of the chunk. A marker on both sides means
neither side ends with a newline, and a marker on the new side alone means
the terminator was removed, which no suggestion can express and which is
still reported without a fix.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant