[AnKiT-GaRG2]

Closes #1390

Summary

--fail-on read its value with a fallback to the default threshold and never validated it. A value such as moderate (npm audit's name for medium, so the likeliest mistake for an adopter migrating from npm audit --audit-level=moderate) left the gate at critical, and the run exited 0 on a tree it had just reported as vulnerable.

Change

• Add parseFailOn in src/cli/args.ts, next to parseSbomFormat and in the same style. It accepts critical, high, medium and low (the values documented in the CLI reference), case-insensitively, and throws before the scan starts:
  • Unknown --fail-on severity "moderate". Valid severities: critical, high, medium, low.
  • --fail-on requires a severity. Valid severities: critical, high, medium, low. for a missing, empty or flag-shaped value.
• Use it for the spaced and = forms on both the scan and the overrides parsers.

The value is stored as typed, so the existing (--fail-on HIGH) echo in the gate summary is unchanged. The GitHub Action only passes --fail-on when the input is non-empty, so it is unaffected.

Overlap with #1392

#1392 adds a requireFlagValue guard for a missing value on the same four lines. Both changes reject --fail-on with no value; this one additionally rejects unrecognised values. Whichever lands second only needs to keep the validation in parseFailOn, and I am happy to rebase.

Tests

New tests/cli/fail-on-args.test.ts: the four valid severities in both forms, mixed case, the default when the flag is absent, moderate / severe / warn in both forms, a missing and an empty value, a following flag not being swallowed, and the overrides path.

npm run lint:tests and tsc --noEmit are clean, and all of tests/cli passes except scan-fix-verify-exitcode. The full suite has the same 33 failures with and without this change (the e2e suites and that test, which depend on the synced advisory database in my environment), so nothing new fails.

[sonukapoor]

Thanks for this, and welcome. This is the one I most wanted off that list, because the failure mode is a green build on a tree the scan has just described as vulnerable, and in CI the exit code is the whole signal.

I reproduced both halves on the merge result. An unrecognised value now stops the run before anything is scanned and the message names the four accepted severities, where before it printed nothing at all and exited 0. The flag given with no value does the same. Both the spaced and the = forms are covered on both the scan path and the overrides path, which is the part I would most likely have missed, since that second call site has the identical fallback and is easy to read past.

Against the issue's success criteria: unrecognised value rejected before scanning and naming the valid ones, yes. Valid values in any case still work, and I confirmed HIGH and High still gate because you return the value unchanged rather than the lowercased one. Flag with no value rejected rather than defaulted, yes, including the empty = form. The fourth criterion asked for a test asserting the exit code and that no scan ran, and your tests assert the thrown message at the parser instead. I am taking that as met rather than asking you to change it: a throw during parsing structurally means no scan runs, and a parser unit test is the right level for this. I checked the exit code myself and it is non-zero.

Two things I verified rather than assumed, both because they could have made this a change request.

Your accepted list is the four documented severities, but the threshold comparison underneath understands six: it also takes none and unknown. So on main --fail-on none and --fail-on unknown are honoured, the gate line prints, and after this they are both rejected. I went looking for what that costs. Nothing, as far as I can measure. Neither is documented anywhere, no test or example uses either, and our own Action cannot emit them. More to the point, both rank identically to low in the comparison, and the lowest rank any real finding can reach is that same rank, so neither was ever distinguishable from low on any finding set. The one exception is a finding whose only advisory is scored exactly zero, which is a non-vulnerability, and I saw none of those across five trees including two large real ones. So tightening the code to match what the reference page already documents is the right direction, not a narrowing worth preserving.

The other was whether a rejected value exiting 1 makes a usage error indistinguishable from the gate firing. It does, but that is the existing convention for every invalid flag value in the tool, so you are consistent with it rather than introducing something new. Not yours to change.

Mutation-tested: removing the unrecognised-value check fails four of the twelve tests in your file and leaves the other eight passing, which is the right profile rather than a blanket failure. Full suite on the merge result is 177 suites and 2461 tests green.

One thing to flag so it does not surprise you. @prx-my opened a pull request three hours after yours against a neighbouring issue, and it applies a missing-value guard to four flags including this one. The two cannot both land unchanged. Yours is the only one that validates the value, which is the half that fixes the false negative, so it is the one I want for this flag. That means his will get rebased around yours rather than the other way round, and nothing is needed from you.

Approved.

[sonukapoor]

Merged, thank you @AnKiT-GaRG2, and a good first one to land.

For what it is worth, the thing that made this easy to take was that you covered the overrides path as well as the scan path. That second call site has the identical fallback, it is a long way from the first one, and a fix that only handled the obvious one would have left the same green build reachable by a different command.

[AnKiT-GaRG2]

Thanks @sonukapoor, and for the careful review. Checking none and unknown against the threshold comparison before approving was appreciated. Happy to help rebase or review the neighbouring missing-value PR if it's useful.