fix: an unknown verdict now names the line the runner flagged - #8
Merged
Conversation
The action installed `patchrail>=0.5.0,<0.6.0` and read six fields out of the ci-result. On a log no rule matched, that produced the worst annotation it can produce: `unknown (confidence 0.15)`, subsystem `unknown`, and the advice to "inspect CI log and run the failing job locally" — on a red run, from a tool the user installed precisely so they would not have to. patchrail 0.6.0 reports the runner's own `##[error]` line for the failing step in `runner_errors`. psf/requests run 29295524780 died on a single self-explanatory line — `"github-token" length must be less than or equal to 100 characters long` — and the action had that string in the JSON it was already reading, and dropped it on the floor. Raise the range to `patchrail>=0.6.0,<0.7.0` and surface the field: first line in the annotation, all of them in the job summary, above the generic advice. The class stays `unknown` and the confidence stays 0.15 — an annotation says where the job died, not why, and PatchRail still does not pretend to recognize a log it does not recognize. A log that classifies is byte-for-byte unchanged. The floor moves to 0.6.0 for the same reason it moved to 0.5.0: below it, a tool the job merely named — a `GRADLE_HOME=` line, a `Collecting mypy` from pip, a tool named inside a filename — is read as the cause of death and annotated as a confident wrong answer. These lines are log text from a build any PR author can write, so they are untrusted. GitHub decodes `%0A` in a workflow command back into a newline, so a log containing `%0A::error::...` would close our warning and forge a command of the author's choosing; `%` is escaped first, on the way out. In the job summary they stay inside their code span. Both are tested, along with the cap that keeps a matrix build's worth of annotations from flooding the summary.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
The action pinned
patchrail>=0.5.0,<0.6.0, so its users never received the 0.6.0 precision fixes. Worse, on a log no rule matches it annotated a red run withunknown (confidence 0.15), subsystemunknown, and "inspect CI log and run the failing job locally" — nothing the user would not have known by never running the action at all.patchrail 0.6.0 hands back the runner's own
##[error]line for the failing step inrunner_errors. The action was already reading that JSON and dropping the field on the floor.The fix
action.yml: range →patchrail>=0.6.0,<0.7.0. The floor moves for the same reason it moved to 0.5.0: below it, a tool the job merely named (aGRADLE_HOME=line, aCollecting mypyfrom pip, a tool named inside a filename) gets read as the cause of death and annotated as a confident wrong answer.scripts/annotate.py: surfacerunner_errors— first line in the annotation, all of them in the job summary, above the generic "reproduce it locally" advice, since onunknownit is the only line worth reading.The class stays
unknownand the confidence stays0.15: an annotation says where the job died, not why. A log that does classify is byte-for-byte unchanged, and so is any run pinned to a pre-0.6.0 patchrail via thepatchrail-versioninput (no such key → nothing reported).Before / after, on the real psf/requests log that motivated the patchrail fix:
Security
These lines are log text from a build any PR author can write. GitHub decodes
%0Ainside a workflow command back into a newline, so a log containing%0A::error::…would close our::warningand forge a command of the author's choosing —%is escaped first, on the way out. In the job summary the text stays inside its code span. Both are tested; removing the escape turnstest_a_log_line_cannot_forge_a_second_annotationred.Tests
tests/test_runner_errors.py(12 new): the line reaches the annotation and the summary, it outranks the generic advice,unknownstaysunknown, a classified result is untouched, the annotation stays on one line, the injection and code-span escapes hold, a matrix build cannot flood the summary, a 5000-char stack trace is truncated, and junk in the field (a bare string, a dict, an int,null) is ignored rather than annotated.Full suite: 35 passed against patchrail 0.6.0. Verified end-to-end through the real
explain → annotatepath, not just the unit tests.