fix: reject non-boolean --matches-stack values instead of eating a positional

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

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

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 () => {

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({

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/);
+  });
+});

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.`,
+    );
+  };
+}