fix: stop --sarif and --baseline swallowing the next flag - #111
Conversation
A value-taking flag whose next token starts with `--` is now a value error, so `--sarif --strict` no longer writes a report named `--strict`. Boolean flags reject an inline `=value` instead of enabling anyway. Fixes vyncint#98 Signed-off-by: Cestercian <yashafaid@gmail.com>
vyncint
left a comment
There was a problem hiding this comment.
Reviewed against a local build of the branch, not just the green checks — thank you, this is a careful fix and it goes to the right place.
Reproduced #98's transcript against the PR, all five reported behaviours:
$ cargo-reconverge reconverge check --sarif --strict
error: `--sarif` requires a value (got the flag `--strict`) exit=2, and no ./--strict written
$ cargo-reconverge reconverge check --baseline --strict
error: `--baseline` requires a value (got the flag `--strict`)
$ cargo-reconverge reconverge check --strict=false error: `--strict` takes no value
$ cargo-reconverge reconverge check --strict=0 error: `--strict` takes no value
$ cargo-reconverge reconverge check --show-suppressed=no
error: `--show-suppressed` takes no value
And the happy path still works end to end, with a driver built from the branch, over tests/lint-samples:
$ cargo-reconverge reconverge check --sarif out.sarif --strict
reconverge: 4 deny, 4 confirmed, 15 warning findings # no "(15 hidden…)" tail: strict applied
out.sarif: 25155 bytes, $schema sarif-2.1.0
$ cargo-reconverge reconverge check --sarif=--weird # the escape hatch
./--weird is 25155 bytes
The summary line no longer advises rerunning with the flag that was just passed, which was the part of #98 that made the bug hard to see.
What I particularly like:
- The fix is at the cause, not the symptom. #98 named
check.rs:51-84; puttingsplit_flag/reject_value/require_valueinargs.rsand routing all six commands through them is what stops the two loop shapes drifting again —inspectcalling--ascii=falseunrecognized while the others silently accepted it was the same bug wearing a different coat. - A missing value is a value error, not a usage error.
!wants_usage()is asserted in every new test, so this keeps #63's one-line-diagnosis property instead of burying the reason under 52 lines of usage. Confirmed: 1 line vs 52 for--bogus. - The integration test asserts the filesystem, not just the message —
!scratch.join("--strict").exists()is the assertion that would have caught the original bug. - Help text documents the
--sarif=--weirdform and a test asserts the help contains it, so the escape hatch cannot quietly disappear.
Also checked, all clean: cargo fmt --all --check, cargo clippy --workspace --all-targets -D warnings, and the full workspace suite (20 binaries). Documented forms all still parse — --cc 8.6, --cc=8.6, --message-format json, --message-format=json, --strict --show-suppressed, watch --max-runs=2.
Two notes for the record, neither blocking and neither introduced here:
--sarif=and--baseline=(empty inline value) are still accepted by the parser and fail later. Pre-existing, and arguably fine since the failure is prompt.--sarif --reportsgot the flag `--`. There is no--passthrough convention in this CLI today, so that reads correctly; worth remembering if one is ever added.
Fixes #98
Summary
--sarif --strictwrote the SARIF report to a file named--strictand dropped strict mode;--baselineswallowed the next flag the same way. Boolean flags silently accepted=valueand enabled anyway.Changes
Shared helpers in args.rs: value-taking flags reject a following
--token; booleans reject inline=value. Applied across check/watch/witness/triage/inspect/learn.Testing
cargo test -p cargo-reconverge: 55 passed. clippy and fmt clean.