feat: render SARIF fixes and formatter diffs as GitHub suggestions - #149
feat: render SARIF fixes and formatter diffs as GitHub suggestions#149bugale wants to merge 9 commits into
Conversation
e7f37f0 to
0b0bc13
Compare
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>
f05eb97 to
037ed03
Compare
| 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 |
There was a problem hiding this comment.
Why?
If a formatter added a newline to the end of a file that didn't have it, I think we should report it.
There was a problem hiding this comment.
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 sentBoth 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.
There was a problem hiding this comment.
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
NOTEat 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.
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>
037ed03 to
54da279
Compare
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>
16ac5c3 to
b0bc650
Compare
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>
Adds native support for GitHub suggested changes, driven by standard SARIF
fixes, plus adiffinput 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 suggestionsIssuegains an optionalfix, read byparseSariffromfixes[0].artifactChanges[0].replacements[0].insertedContentand re-emitted bygenerateSarif, so the parse/generate round trip stays lossless and code scanning receives the fix too.addCommentsappends the suggestion block after the identifier line. Being last is what makes the body well formed — nothing follows the closing fence.insertedContent.textrenders 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 fix's
deletedRegionis 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 absentendColumnmeans the end of the text ofendLine, so the two usual spellings of a whole line replacement differ by exactly one line terminator:deletedRegionfor lines 3–4insertedContent.text{startLine: 3, endLine: 4}{startLine: 3, startColumn: 1, endLine: 5, endColumn: 1}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
deletedRegioncovering 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 linesisNewIssuerequired 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.
parseAddedLinesbecomesparseDiffLinesand 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:addCommentsnow skips issues spanning a line outside the diff. That guard is not optional — all comments go out in onecreateReview, so a single out-of-hunk anchor returns 422 and loses the whole batch.Behaviour change worth your attention: this affects
onlyNew(failOnlyNewat 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 plainfeat— 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 suggestionsThe two commits above only let a linter that already speaks SARIF carry a fix. This one removes that requirement: a new
diffinput format reads the output ofgit diff, soclang-format -ifollowed bygit diffis 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 oneParserplus wiring. Everything downstream — the SARIF uploaded to code scanning, the job summary,onlyNew— works unchanged.Three things it has to get right:
git diffprints 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.line/elinecome fromdel.lnandnormal.ln1, and the new side becomes the fix.Two details that only show up on real input.
git diffemits\ No newline at end of fileas an ordinary change with a duplicated line number, so it has to be filtered or the marker text lands inside the suggestion. Andindex.tsstripped every\rbefore 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
messageinput 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.fixis astring[]: 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-inclusivedeletedRegion, and stripping that trailing newline arrives at the empty string as well.That has one consequence for the SARIF written out. The two
deletedRegionspellings 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.fixand 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 renderedbody_htmlback. Independently reproduced on this PR:""(delete)"\n"(blank one line)"\n\n"" \n""\nint x;\n"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 extra0afaithfully, 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 diffstill prints a deleted and an added line, because the bytes differ — but the two lines have identical text: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.textcame 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 outputThe only breaking commit here, and the one to look at first.
failOnlyNewnarrowed 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
onlyNewand 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:failOnlyNewno longer exists. GitHub does not reject awith:key the action never declared, but it is not silent about it either: the runner emits anUnexpected input(s) 'failOnlyNew', valid inputs are [...]warning annotation that namesonlyNewamong the valid set (actions/runner,ActionRunner.cs). The input is still dropped, soonlyNewfalls back to itsfalsedefault and the run considers every issue — withfaildefaulting totrue, a stale workflow fails on the whole corpus rather than quietly narrowing.onlyNewset, 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 leaveonlyNewunset on the scanning run.addCommentskeeps its own two guards regardless ofonlyNew, since a comment still cannot be anchored outside the pull request's diff.Migrating off the old name
A workflow left on
failOnlyNewdoes not fail open —addCommentskeeps its own guards, so the comments stay correct. It fails closed:failOnIssuesno 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 saysfound 8238 issuesand 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
onlyNewignores 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 bufferFound 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:application/vnd.github.v3.diff; charset=utf-8application/vnd.github.v3.diff; charset=UTF-8application/vnd.github.v3.diff; charset=utf-8; boundary=xapplication/vnd.github.v3.diffGitHub currently sends the lowercase spelling, so this is latent rather than live — but
UTF-8is the RFC-canonical casing, and any trailing parameter defeats the anchor too.getPrDiffcast the result straight to a string withas unknown as string, which is what let it through the type checker.The consequence is total and silent.
parseDiffon a castArrayBufferreturns zero files, with no error, so every issue failsisNewIssueand is dropped with only adebugline. Measured on a real 5,010-file repository by the consuming session, varying nothing but the header:A pull request with 7,724 genuine findings passes clean.
Why it belongs in this PR rather than a follow-up. At
v4.0.0the SARIF is generated and written before the diff is fetched at all —generateSarifruns on the unfiltered issues, andgetPrDiffis not called until several lines later. A transport failure there cannot reach the SARIF; it loses the comments and, withfailOnlyNew, the failure. TheonlyNewcommit moves filtering ahead ofgenerateSarif, 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
sarifis set — it never uploads to code scanning itself. For a consumer that wires the file intoupload-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:
commentonlyfailOnIssues, so the job still goes redonlyNew(orfailOnlyNewbefore it)A consuming session confirmed this against a real repository on both
v2.3.0andv4.0.0, whose bundles carry the identical guard at the identical line:comment: truealone 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
getPrDiffitself needs the API.fix: fail on a pull request diff that cannot be parsedDecoding 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-8matches 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.diffis a vendor media type, and the consuming session found that PowerShell's HTTP client classifies it as bytes even withcharset=utf-8present, because it ignores the charset parameter entirely. Octokit'scharset=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 groupUnrelated 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 againstGitHub Action.*,Check.*and.*[lL]int.*. The job that runs the tests is namedJest, which matches none of the three, andbugroup-checkstreats 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
mainallows the merge. AddingJest.*closes it; I checked that every matrix variant (Jest (ubuntu-latest)and friends) matches, and thatAuto-merge,Releaseand 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.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 —
failOnIssuesstill receives the complete filtered list, so the verdict and its count are never capped, only the view.Testing
sariffixfixture 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.difffixture 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.diffcrlffixture pair plus a CI matrix entry, covering a diff of CRLF content end to end. The unit tests reachparseFormatDiffdirectly, so nothing covered the one line inindex.tsthat 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.isNewIssuetests, addedisCommentableIssueones, and replaced thefailOnIssuesdiff tests with directfilterNewIssuesones.pylintonlynewCI matrix entry has its own fixture pair, since its output is filtered to nothing — previously it reusedpylint's input againstnoissues' 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) andnpm run packageare 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.