Skip to content

fix: add actionable guidance to flag incompatibility errors (fixes #401) - #746

Open
mayank-dev-15 wants to merge 6 commits into
OWASP:mainfrom
mayank-dev-15:fix/better-flag-error-messages
Open

fix: add actionable guidance to flag incompatibility errors (fixes #401)#746
mayank-dev-15 wants to merge 6 commits into
OWASP:mainfrom
mayank-dev-15:fix/better-flag-error-messages

Conversation

@mayank-dev-15

Copy link
Copy Markdown

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-db n
After:
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. n
All 5 error messages updated in src/cli/validate.ts. Existing tests pass (they use substring matching).

Fixes #401

…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 sonukapoor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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").

Comment thread src/cli/validate.ts
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."

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

@sonukapoor

Copy link
Copy Markdown
Collaborator

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.

@sonukapoor

Copy link
Copy Markdown
Collaborator

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.

@sonukapoor

Copy link
Copy Markdown
Collaborator

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 sonukapoor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 sonukapoor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 sonukapoor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.

  2. Line 38 still says "apply fixes interactively" for --fix, but --fix does not prompt for anything - it applies validated fixes and rescans. Worth matching the help text wording here.

  3. 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).

  4. 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 --json still 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: incompatible flag errors should explain which option to use, not just state the conflict

2 participants