fix: add actionable guidance to flag incompatibility errors (fixes #401) - #746
fix: add actionable guidance to flag incompatibility errors (fixes #401)#746mayank-dev-15 wants to merge 6 commits into
Conversation
…ASP#401) Error messages now explain what each flag does and suggest which one to use: - --offline/--offline-db vs --osv-url: choose offline or online mode - --no-cache vs --offline: cache only applies to online scans - --fix vs --json: interactive fixes or JSON output, not both - --create-pr vs --json: PR or JSON output, not both - --report vs --json: HTML report or JSON output, pick one
sonukapoor
left a comment
There was a problem hiding this comment.
The guidance descriptions are a real improvement over the bare "cannot be used with" messages - the no-cache one especially, since that relationship isn't obvious. One more thing: the new guidance text isn't actually exercised by the existing tests. Jest's toThrow(string) does substring matching, so toThrow("--fix cannot be used with --json") still passes - but a future edit to the guidance prose won't be caught at all. Worth updating a couple of the assertions to cover something distinctive from the new text, like toThrow("apply validated fixes") or toThrow("local advisory DB").
| throw new Error("--fix cannot be used with --json"); | ||
| throw new Error( | ||
| "--fix cannot be used with --json. " + | ||
| "Use --fix to apply fixes interactively, or --json to output scan results as JSON — not both at once." |
There was a problem hiding this comment.
Two things on this line. interactively isn't accurate for --fix - the flag applies validated direct fixes and rescans without any prompting. Worth updating to match the help text: "Use --fix to apply validated fixes and rescan..."
Also there's an em dash before "not both at once" - project style uses hyphens throughout, so that should be a hyphen or just "pick one."
|
Hey @mayank-dev-15 — this branch is now behind main. Could you rebase against main and push when you get a chance? Once that's done and the changes from the last review are addressed, we can move forward. |
|
Hey @mayank-dev-15 - this branch is a bit behind main. Could you rebase against main and force-push? Happy to review once it's up to date. |
|
Hey, this branch is behind main - could you rebase against the latest main and push? Happy to pick this up for review once it's up to date. |
sonukapoor
left a comment
There was a problem hiding this comment.
Good improvement - the messages now explain the why behind each incompatibility, which makes them genuinely useful at the terminal rather than just opaque. The --osv-url URL format guard is a nice addition. Approving.
sonukapoor
left a comment
There was a problem hiding this comment.
Thanks for this - the expanded messages are genuinely more helpful, especially the --no-cache explanation. A few things to sort out before it's ready.
Three messages not yet expanded: --allow-private-osv-url requires --osv-url (line 7), --create-pr requires --fix (line 43), and --base can only be used with --create-pr (line 54) still have the original short form. The goal of this PR is consistent, actionable guidance - a developer hitting any of those three flags gets a much weaker error than someone hitting --fix + --json. Please expand those three to match the style of the ones you've already done.
Missing test coverage: The --create-pr cannot be used with --json check has no coverage in tests/validate.test.ts. Since you're in this file anyway, a one-liner expect(() => validateOptions({ createPr: true, json: true, fix: true })).rejects.toThrow(...) fills the only gap in the flag validation tests.
sonukapoor
left a comment
There was a problem hiding this comment.
Hey @mayank-dev-15, thanks for keeping this branch up to date with main. Looking at the current diff though, it is still just the original commit from June 23 - the five commits since then are all "merge main into this branch" and do not touch validate.ts, so none of the changes we asked for over the last two review rounds have landed yet. Consolidating what is still outstanding so it is all in one place:
-
Three messages are still in the old bare format:
--allow-private-osv-url requires --osv-url(line 7),--create-pr requires --fix(line 43), and--base can only be used with --create-pr(line 54). The whole point of this PR is consistent guidance across all the flag-incompatibility errors, so these three need the same "here is what each flag does, here is what to do instead" treatment as the other five. -
Line 38 still says "apply fixes interactively" for
--fix, but--fixdoes not prompt for anything - it applies validated fixes and rescans. Worth matching the help text wording here. -
There are now three em dashes in the file (lines 38, 49, 60). We do not use em dashes anywhere in this repo - hyphens, or just rewrite ("not both at once" becomes "not both" or "pick one" without any dash at all).
-
Test coverage still has not caught up with the new message text. Jest's
toThrow(string)does substring matching, so every existing assertion in tests/validate.test.ts still passes against the old short-form prefix and never actually exercises the new guidance sentence. Pick one distinctive phrase per message ("Choose offline mode", "apply validated fixes", "not both") and assert on that instead, so a future edit that breaks the guidance text gets caught. And--create-pr cannot be used with --jsonstill has no test at all - that is the one gap in the flag-validation suite.
One optional thing while you are in here: five of these throw blocks now follow the identical shape ("X cannot be used with Y. Use X to ..., or Y to ... - not both."). Not blocking, but a small helper like incompatibleFlagsError(flagA, whatA, flagB, whatB) would cut the repetition. Totally fine to leave as-is if you would rather not touch it in this PR.
Once those are in, this should be ready to go - the actual guidance text is good, this is really just catching up the last two rounds of feedback.
Error messages now explain what each flag does and suggest which one to use.
Before:
nError: --fix cannot be used with --json Error: --offline/--offline-db cannot be used with --osv-url Error: --no-cache cannot be used with --offline or --offline-dbnAfter:
nError: --fix cannot be used with --json. Use --fix to apply fixes interactively, or --json to output scan results as JSON - not both at once. Error: --offline/--offline-db cannot be used with --osv-url. Choose offline mode (local DB) or online mode (custom OSV endpoint), not both. Error: --no-cache cannot be used with --offline or --offline-db. In offline mode the local advisory DB is used directly; --no-cache only applies to online scans.nAll 5 error messages updated in src/cli/validate.ts. Existing tests pass (they use substring matching).
Fixes #401