Explain what tripped the fail-on gate and document only-used scope - #1239
Conversation
|
Thanks for this. I am keeping it. Worth flagging so it does not cost you on a future PR: #1000 carries the Keeping this one regardless. It is already written, it covers a gap that has been open since August, and rewriting the same thing myself would throw away your work for no reason. Review to follow. I take these one at a time rather than in a batch, so it will land shortly. |
a9c7b61 to
331940f
Compare
8934a66 to
a9c7b61
Compare
sonukapoor
left a comment
There was a problem hiding this comment.
Thanks for this, and for the write-up. Naming the decision you were implementing and quoting the reproduction made this much faster to review.
The core is right. Before your change that command exits 1 with a green all-clear and nothing naming the gate. After it the last line reads Failing: 9 override hygiene findings at or above low (--fail-on low). The counting predicate is the same one reachesFailOn uses, so the line can't claim a failure the gate didn't cause. Suppressing it in fix mode is right, since the exit code is forced to 0 there, and leaving that guard off the multi-folder path is also right, because multi-folder bails on --fix before the gate. That looks like an oversight and isn't.
Two things before I merge it, both inline below, plus a question and one note that's mine rather than yours.
The docs half I checked separately and it's accurate: --only-used is applied only to the CVE finding list, and override and maintenance findings are never filtered, so the wording matches the behaviour.
shouldFail is now gateLine !== null on both the single and multi-folder paths, so the exit code and the printed reason come from one computation and the third copy of the severity comparison is gone. The parenthetical echoes the raw --fail-on value. normalizeSeverity falls back to critical for an unknown value, so echoing the normalized level told the user they passed a value they never typed. New tests cover the line on a run that trips the gate and its absence under --json, for both paths, plus the raw echo in the unit tests.
|
Thanks for the careful review. All three are in 90ffa3d, on top of a merge of current main.
Also noted on #1000 and the in-house label, thanks for keeping it anyway. |
There was a problem hiding this comment.
This is a strong first contribution, thank you @theluckystrike. Everything from the last round is done, and done the way that was suggested rather than worked around.
The part I care most about is that your tests pin the exit code separately from the printed line. Forcing shouldFail to false while still printing the gate line fails three tests on the single-folder path and five on the multi-folder one, so the coupling cannot silently come apart later. That was the one real risk in deriving the exit from the summary, and you closed it.
Echoing the raw value was the right call on the --fail-on hgih question. A run that quietly gates on critical because the input was a typo is worse than a slightly odd-looking message, and at or above critical (--fail-on hgih) tells someone exactly what happened.
One behaviour change worth putting on the record, because it is a fix and not a side effect. Multi-folder with an empty --fail-on used to exit 1 and now exits 0. The old multi-folder gate term was a bare severity comparison with none of the empty-value guard its two neighbours had, so an empty value normalised to critical and gated the build. action.yml describes that input as "Leave empty to run the scan in informational mode (no exit code on findings)", so the single-folder path was already correct and multi-folder was contradicting our own documented contract. Yours does the documented thing on both. Thanks for flagging it on the thread rather than letting me find it.
Two follow-ups came out of reviewing this, and both are mine to hand out rather than yours to fix here: the three gate-class labels are typed out separately on each scan path so one can drift with the suite green, and the --fix suppression guard has no test holding it in place. I have filed those as #1274 and #1275 so this can land as it is.
Approved. I merge these one at a time rather than in a batch, so it will land shortly.
|
Thanks for the careful review, and for writing down the multi-folder exit change so it is on the record. If you want to hand one of the follow-ups out, I'm happy to take #1275 and add the test that holds the |
|
Yes, take #1275, and thanks for offering. Rather than paraphrase it: CONTRIBUTING says you do not need permission to start, that commenting on the issue is a courtesy so others can see someone is on it rather than a reservation, and that if two people end up on the same thing the first working pull request is the one that lands. So drop a note on #1275 and go. I am not assigning it, and that is the same for everyone here rather than anything about your PR. The check to run is written into the issue. Delete the #1274 is open on the same terms if you want it afterwards, though there is no need to take both. |
|
Merged, thank you @theluckystrike. A good first contribution, and the part I keep coming back to is that you pinned the exit code separately from the printed line, so the coupling you introduced cannot silently come apart later. #1274 and #1275 are the two follow-ups that came out of reviewing this, and #1275 is yours whenever you want to start on it. |
Closes #1000.
The maintainer decision in the issue is option 1, document the scope, plus the gate line. This PR does both parts.
Part one is documentation. The
--only-usedand--fail-onentries in cli-reference.md now state that--only-usedscopes vulnerability findings only, while override hygiene and maintenance risk findings gate independently. Neither is a reachability question, so filtering them would let the flag hide override misconfiguration.Part two is the gate line. No
--fail-onfailure was explained in the output for any finding class. The maintainer's own reproduction shows the problem. Runningexits 1 with nothing in the output naming the gate, the threshold, or the class that tripped it. Grepping the whole output for "fail", "threshold", "gate" and "exceed" returns nothing.
The fix adds a
failingGateSummaryhelper insrc/utils/severity.tsnext toreachesFailOn, since both read the same severity order. It takes the three finding classes the gate ORs, counts how many findings in each class meet or exceed the threshold, and returns one line in the format the issue requested.Both scan paths call it right after the
shouldFailcomputation.src/scan/single-scan.tsskips the line in JSON mode and in fix mode, where the exit code is deliberately zero.src/scan/multi-folder-scan.tsskips it in JSON mode. A clean run, a run without--fail-on, or a run where nothing meets the threshold prints nothing, so existing output is unchanged.Line references from the issue body are stale in the way the maintainer noted, so the patch anchors on the current locations. The gate lives at
src/scan/single-scan.tsaround line 657 andsrc/scan/multi-folder-scan.tsaround line 453 after #1129.Per the sequencing note in the issue, this touches neither
src/cli/args.tsnorsrc/overrides/context-builder.ts, so it does not collide with #1228 or #1202.