Repository navigation
fix(ratchet): gate like --fail-on when no baseline exists - #1314
theluckystrike wants to merge 5 commits into
Conversation
sonukapoor
left a comment
There was a problem hiding this comment.
Thanks for this. The fix itself is right, and I checked it does what #1293 asks on a single folder: with no baseline and a high finding it now exits 1, writes the baseline and prints both lines, and with a baseline present nothing changes. I ran the repro from the issue body on examples/direct-fixable.
One thing needs sorting before it can land, and it is an easy one to miss. --ratchet has a second implementation in src/scan/multi-folder-scan.ts. handleMultiFolderRatchet writes a baseline per folder and then continues, so the gate never runs there either. Same tree, same flags, both built from your branch:
| mode | exit code |
|---|---|
| single folder | 1 |
| multi folder | 0, still a silent pass |
So as it stands this fixes half of #1293 while carrying Fixes #1293, which would close the issue with the monorepo path still passing silently on every fresh CI checkout.
I am happy either way on how you want to handle that. You can extend this PR to cover handleMultiFolderRatchet, which is the same shape you have already written, or we split #1293 and you keep the credit for this half. If you would rather split it, drop the Fixes line here so the issue stays open.
Three smaller things inline.
|
Thanks @sonukapoor for the detailed review and for merging #1278. I've pushed 24ea7aa to cover the second ratchet path too. First runs now save each folder's baseline and apply the fail-on threshold, including a mix of existing and missing baselines.
The debt hint only appears when findings exist. JSON suppresses both explanatory lines, and single-folder fix mode still exits 0. Added regression cases cover those guards, clean trees, threshold handling and the unsupported multi-folder fix case.
The focused checks passed 153 tests. The full suite has 33 offline database failures on both the baseline and this branch, so I can't call the whole suite green.
Thank you,
Michael Lip
Founder of https://zovo.one, 108 Chrome extensions with 150,000+ users
Also building https://makeroll.com and https://ml0x.com, open-source guardrails for Claude Code agents
Open source has to win, and small fixes like this one are my part of it
GitHub Sponsors keeps these fixes coming, https://github.com/sponsors/theluckystrike
|
sonukapoor
left a comment
There was a problem hiding this comment.
The multi-folder half is right. Same two-folder fixture, no baselines, --fail-on high: main exits 0, your branch exits 1, and a clean tree still exits 0 on both. Single folder behaves the same. Full suite green on the branch, 168 suites and 2371 tests, and the branch is current with main, so that is the merge result.
Your tests discriminate too: reverting the multi-folder return kills six of them, dropping the count > 0 guard kills two, dropping the --json guard kills two.
My third ask last time was wrong and I am withdrawing it. I asked you to pin the !options.fix check on the exit expression. That condition cannot be reached. The whole ratchet block sits in the else arm of if (options.fix), so options.fix is always false there, and under --fix no baseline is written at all, on main as well as here. There is nothing to pin, and both !options.fix checks are dead rather than load-bearing. Removing them would be a fair tidy-up but I am not asking for it.
One thing left before this lands, and it is docs rather than code. website/docs/ratcheting.md still says of the first run:
This creates
.cve-lite/baseline.jsonin your project root and exits 0.
That is now false whenever the tree carries a finding at or above --fail-on, which is the exact case this PR fixes. Could you update that sentence here? src/cli/help.ts and website/docs/cli-reference.md both describe --ratchet without mentioning that the save run gates, so a few words in each would be welcome, but the ratcheting.md sentence is the one that contradicts what ships.
Separately, and not yours to fix: a first run with --ratchet --json now exits 1 while printing no reason, because the gate line is --json-suppressed. That suppression is what I asked for last time, so revisiting it is my call rather than a problem with your work. It also writes no JSON file at all, but main does the same on that path, so that half is pre-existing and the same family as #1315. I am filing it separately.
Four small things inline, all optional.
| } | ||
| }); | ||
|
|
||
| it("--ratchet with no baseline and a clean tree exits 0 and writes the baseline", async () => { |
There was a problem hiding this comment.
Small thing: this is titled "a clean tree" but it seeds createFinding({ severity: "low" }), so the tree is not clean, it just carries nothing at or above high. The behaviour is worth covering and the assertions are right, it is the name that will mislead whoever reads it next, especially since the genuinely clean case is the first row of the it.each below. Something like "below the threshold" would say what it does.
| expect(writeBaselineMock).toHaveBeenCalledWith("/project/a", expect.any(Array)); | ||
| expect(writeBaselineMock).toHaveBeenCalledWith("/project/b", expect.any(Array)); | ||
| expect(exitCode).toBe(EXIT_OK); | ||
| expect(exitCode).toBe(EXIT_FINDINGS); |
There was a problem hiding this comment.
This expectation and the matching one further down both assert EXIT_FINDINGS now, and they reach it through baseOptions, which sets failOn: "none". Since none ranks 0 and the gate counts findings >= failLevel, --fail-on none gates on every finding, so these two pass for an incidental reason rather than because the threshold logic works.
Your new it.each above covers the real thresholds properly, so nothing is genuinely untested. I would just pass an explicit failOn: "high" in these two so they say what they mean.
| console.log(chalk.red(`${r.subfolder}/: ${gateLine}`)); | ||
| } | ||
| } | ||
| if (count > 0 && !options.json) { |
There was a problem hiding this comment.
This prints once per folder, so a monorepo with twenty subfolders gets the same sentence twenty times with only the path changing. The gate line above it genuinely is per folder because the counts differ, but the instruction does not vary. Collecting the paths and printing one line after the loop would read better, or printing the hint once without a path. Not a blocker.
| ), | ||
| ); | ||
| const gateLine = failingGateSummary( | ||
| [{ label: "vulnerability", findings: r.sorted }], |
There was a problem hiding this comment.
label: "vulnerability" is at four sites across the two scan files now, two of them added here. #1288 is touching these same files, so this is the kind of literal that quietly diverges in a merge. A shared constant beside RATCHET_BASELINE_COMMIT_HINT would settle it. Entirely optional, and say if you would rather leave it for #1288 to pick up.
…un gate Address review on OWASP#1314: - Multi-folder --ratchet collects the saved baselines that carry findings and prints a single commit hint after the loop instead of one per folder. - Build the first-run gate input from GATE_CLASS_LABELS (OWASP#1288) in both scan paths, so no "vulnerability" literal is left. - Rename the single-folder test that seeds a low finding to say it is below the threshold, and pass an explicit failOn "high" to the two multi-folder tests that relied on baseOptions' "none". - ratcheting.md no longer says the first run exits 0; it gates on --fail-on. Note the same in cli-reference.md and --help.
|
Thanks @sonukapoor. All of it is in 8c98b84, on top of a merge of current main.
The docs don't promise a zero exit on the first run anymore. ratcheting.md, the ratchet row in cli-reference.md and the help text now say the run that saves a baseline still applies the fail-on gate. The test is renamed to "findings below the threshold", and the two multi-folder expectations pass an explicit failOn of high. The baseline hint prints once after the loop with every path joined, and both tests that cover it require exactly one hint. Since #1288 landed GATE_CLASS_LABELS, the two first-run sites use GATE_CLASS_LABELS.vulnerability, so there's no label literal left in src/scan.
I left the two dead options.fix negations alone, as you said, and the json first-run message is yours.
One thing outside this PR caught my eye. "Updating the baseline" in ratcheting.md says running ratchet again resets it, but with a baseline present it only gates. I can send that fix as its own small PR. Say the word.
The full suite's green at 168 suites and 2374 tests, and tsc, the test lint script and build pass.
Thank you,
Michael Lip
Founder of https://zovo.one, 108 Chrome extensions with 150,000+ users
Also building https://makeroll.com and https://ml0x.com, open-source guardrails for Claude Code agents
Open source has to win, and small fixes like this one are my part of it
GitHub Sponsors keeps these fixes coming, https://github.com/sponsors/theluckystrike
|
Fixes #1293
With no committed baseline,
--ratchetwrote one and exited 0, so a CI job that checks out fresh every run couldn't ever fail. This follows the decision in the issue body. When no baseline exists,--fail-onapplies exactly as if--ratchetwere absent, the baseline's still written, and a line says that committing .cve-lite/baseline.json is how you accept the existing debt.Two integration tests cover it. With no baseline and a finding at or above
--fail-on, the scan exits 1, writes the baseline and prints the gate line and the commit line. On a clean tree it exits 0 and prints no gate line. The first one fails on main with exit 0.npm run lint:testspasses, and the full suite passed afteradvisories sync(2314 tests), the same steps ci.yml runs.