diff --git a/.changeset/patterns-matches-stack.md b/.changeset/patterns-matches-stack.md index 1581a49..f5d786b 100644 --- a/.changeset/patterns-matches-stack.md +++ b/.changeset/patterns-matches-stack.md @@ -20,3 +20,5 @@ codacy patterns eslint9 --disable-all --matches-stack false ``` The summary printed after a bulk update still reports counts for the whole tool, not just the updated subset. + +Only `true` and `false` are accepted as values. Because Commander's optional-value syntax consumes the next token, a lax parser would let `codacy patterns gh org repo --matches-stack eslint` silently swallow the tool name and then fail with a confusing positional-count error; the flag now rejects non-boolean values with a message that says what to do instead. diff --git a/SPECS/commands/tools-and-patterns.md b/SPECS/commands/tools-and-patterns.md index 9cf0145..efa4a49 100644 --- a/SPECS/commands/tools-and-patterns.md +++ b/SPECS/commands/tools-and-patterns.md @@ -128,7 +128,7 @@ codacy patterns gh my-org my-repo eslint --disable-all --severities Minor | `--enabled` | `-e` | Show only enabled patterns (list mode only) | | `--disabled` | `-D` | Show only disabled patterns (list mode only) | | `--recommended` | `-r` | Show only recommended patterns | -| `--matches-stack [value]` | `-k` | Filter by whether patterns match the repository stack. Tri-state: the bare flag or `true` sends `matchesStack=true`, `false` sends `matchesStack=false`, omitting it sends nothing | +| `--matches-stack [value]` | `-k` | Filter by whether patterns match the repository stack. Tri-state: the bare flag or `true` sends `matchesStack=true`, `false` sends `matchesStack=false`, omitting it sends nothing. Any other value is **rejected** — see below | | `--enable-all` | `-E` | Bulk enable matching patterns | | `--disable-all` | `-X` | Bulk disable matching patterns | @@ -174,7 +174,33 @@ overview call deliberately carries **no** filters — including `--matches-stack ## Tests -File: `src/commands/patterns.test.ts` — 35 tests. +### Why `--matches-stack` parses strictly + +Commander's optional-value syntax (`[value]`) greedily consumes the next token, +including one meant as a positional. With a lax parser, + +``` +codacy patterns gh my-org my-repo --matches-stack eslint +``` + +sets `matchesStack=true` and silently swallows `eslint`, so the command then +fails with `Ambiguous arguments for 'patterns'. Expected 1 or 4 positional +arguments, got 3.` — which never mentions the flag that ate the tool name. + +`strictBooleanOption()` (`utils/options.ts`) therefore accepts only `true` or +`false` and rejects anything else up front: + +``` +error: option '-k, --matches-stack [value]' argument 'eslint' is invalid. +expected "true" or "false". If "eslint" was meant as an argument, place it +before --matches-stack, or pass --matches-stack on its own to mean true. +``` + +> ⚠️ `issues --false-positives [value]` still uses the lax `parseBooleanOption` +> and has the same swallow hazard. Left as-is to keep this change in scope — +> worth switching to `strictBooleanOption` in a follow-up. + +File: `src/commands/patterns.test.ts` — 36 tests. --- diff --git a/src/commands/patterns.test.ts b/src/commands/patterns.test.ts index 5480fe1..a0089af 100644 --- a/src/commands/patterns.test.ts +++ b/src/commands/patterns.test.ts @@ -416,6 +416,37 @@ describe("patterns command", () => { await run("-k", "false"); expectMatchesStack(false); }); + + // Commander's `[value]` syntax greedily eats the next token, so without a + // strict parser `patterns gh org repo --matches-stack eslint` would set + // matchesStack=true and silently drop the tool name, failing later with a + // positional-count error that never mentions the flag. + it("rejects a non-boolean value instead of swallowing a positional", async () => { + const program = createProgram(); + // exitOverride must be set on the subcommand too — it does not propagate + // from the parent, and Commander reports the bad value on `patterns`. + program.exitOverride(); + program.configureOutput({ writeErr: () => {} }); + for (const cmd of program.commands) { + cmd.exitOverride(); + cmd.configureOutput({ writeErr: () => {} }); + } + + await expect( + program.parseAsync([ + "node", + "test", + "patterns", + "gh", + "test-org", + "test-repo", + "--matches-stack", + "eslint", + ]), + ).rejects.toThrow(/expected "true" or "false"/); + + expect(AnalysisService.listRepositoryToolPatterns).not.toHaveBeenCalled(); + }); }); it("should show ☑️ icon for patterns enforced by a coding standard", async () => { diff --git a/src/commands/patterns.ts b/src/commands/patterns.ts index a5117ba..ff33f4b 100644 --- a/src/commands/patterns.ts +++ b/src/commands/patterns.ts @@ -17,7 +17,7 @@ import { CONFIG_FILE_LOCKED_MESSAGE, PATTERN_JSON_FIELDS, } from "../utils/formatting"; -import { parseBooleanOption } from "../utils/options"; +import { strictBooleanOption } from "../utils/options"; import { AnalysisService } from "../api/client/services/AnalysisService"; import { ConfiguredPattern } from "../api/client/models/ConfiguredPattern"; import { SeverityLevel } from "../api/client/models/SeverityLevel"; @@ -183,7 +183,7 @@ export function registerPatternsCommand(program: Command) { .option( "-k, --matches-stack [value]", "filter by whether patterns match the repository stack (true, false, or omit)", - parseBooleanOption, + strictBooleanOption("--matches-stack"), ) .option("-E, --enable-all", "bulk enable matching patterns") .option("-X, --disable-all", "bulk disable matching patterns") @@ -262,13 +262,11 @@ Examples: const { severities, categories } = parseFilters(opts); - // Tri-state: `--matches-stack`/`--matches-stack true` sends true, - // `--matches-stack false` sends false, and omitting it sends nothing. - // Read explicitly rather than by truthiness so an explicit `false` - // stays distinct from "not requested". - let matchesStackFilter: boolean | undefined; - if (opts.matchesStack === true) matchesStackFilter = true; - else if (opts.matchesStack === false) matchesStackFilter = false; + // Already a tri-state: Commander supplies `true` for the bare flag, + // strictBooleanOption returns a boolean for an explicit value, and the + // key is absent when the flag is omitted. Passed through as-is so an + // explicit `false` stays distinct from "not requested". + const matchesStackFilter: boolean | undefined = opts.matchesStack; if (opts.enableAll || opts.disableAll) { await handleBulkUpdate({ diff --git a/src/utils/options.test.ts b/src/utils/options.test.ts index 9662dbe..f81b2c7 100644 --- a/src/utils/options.test.ts +++ b/src/utils/options.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect } from "vitest"; -import { parseBooleanOption } from "./options"; +import { InvalidArgumentError } from "commander"; +import { parseBooleanOption, strictBooleanOption } from "./options"; describe("parseBooleanOption", () => { it('coerces "true" to true', () => { @@ -24,3 +25,30 @@ describe("parseBooleanOption", () => { expect(parseBooleanOption("")).toBe(true); }); }); + +describe("strictBooleanOption", () => { + const parse = strictBooleanOption("--matches-stack"); + + it('accepts "true" and "false"', () => { + expect(parse("true")).toBe(true); + expect(parse("false")).toBe(false); + }); + + it("is case-insensitive", () => { + expect(parse("TRUE")).toBe(true); + expect(parse("False")).toBe(false); + }); + + it("rejects anything else", () => { + // Commander's optional-value syntax would otherwise swallow a positional + // argument as this option's value; rejecting it surfaces the mistake. + expect(() => parse("eslint")).toThrow(InvalidArgumentError); + expect(() => parse("")).toThrow(InvalidArgumentError); + }); + + it("names the flag and the offending value in the error", () => { + expect(() => parse("eslint")).toThrow(/expected "true" or "false"/); + expect(() => parse("eslint")).toThrow(/"eslint"/); + expect(() => parse("eslint")).toThrow(/--matches-stack/); + }); +}); diff --git a/src/utils/options.ts b/src/utils/options.ts index 514fe4f..48119f7 100644 --- a/src/utils/options.ts +++ b/src/utils/options.ts @@ -1,3 +1,5 @@ +import { InvalidArgumentError } from "commander"; + /** * Shared coercion helpers for Commander option values. */ @@ -7,13 +9,37 @@ * * Commander only invokes this parser when a value is actually supplied, so the * bare flag (`--flag`) yields boolean `true` without passing through here. - * Anything other than a case-insensitive `"false"` is treated as `true`, which - * keeps `--flag`, `--flag true` and `--flag TRUE` equivalent. + * Anything other than a case-insensitive `"false"` is treated as `true`. * - * Read the resulting option as a tri-state — `true` / `false` / `undefined` - * (omitted) — rather than with a truthiness check, so "omitted" stays distinct - * from an explicit `false`. + * Prefer {@link strictBooleanOption} on any command that also takes positional + * arguments — see the warning there. */ export function parseBooleanOption(value: string): boolean { return value.toLowerCase() !== "false"; } + +/** + * Strict parser for a tri-state boolean option declared as `--flag [value]`, + * accepting only a case-insensitive `"true"` or `"false"`. + * + * Commander's optional-value syntax greedily consumes the next token, even one + * meant as a positional argument. On a command that takes positionals, a lax + * parser turns `patterns gh org repo --matches-stack eslint` into + * `matchesStack=true` with the tool name silently swallowed, and the command + * then fails with a confusing complaint about the positional count that never + * mentions the flag. Rejecting non-boolean values converts that into an + * immediate, self-explanatory error instead. + * + * @param flag the user-facing flag name, used in the error message + */ +export function strictBooleanOption(flag: string) { + return (value: string): boolean => { + const normalized = value.toLowerCase(); + if (normalized === "true") return true; + if (normalized === "false") return false; + throw new InvalidArgumentError( + `expected "true" or "false". If "${value}" was meant as an argument, ` + + `place it before ${flag}, or pass ${flag} on its own to mean true.`, + ); + }; +}