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/README.md b/SPECS/README.md index c9c0ad0..ee0e88a 100644 --- a/SPECS/README.md +++ b/SPECS/README.md @@ -87,4 +87,4 @@ _No pending tasks._ All commands implemented. | 2026-07-30 | (OD-378, review follow-up) `pull-requests` table polish + a real data bug. **Bug:** Complexity rendered as "no data" on every PR because the API omits the flat top-level `deltaComplexity` and only returns `quality.deltaComplexity` (while still sending a top-level `deltaClonesCount`) — new shared `prQualityMetric(pr, key)` in `utils/formatting.ts` reads the nested `quality` value first and falls back to the flat field; also applied to `repository`'s Open PR table and `pull-request`'s Analysis section, which had the same bug. **Layout:** `✓` moved to the first column; metric order now matches `repositories` (issues → complexity → duplication → coverage); the Coverage column is dropped entirely when no listed PR has a coverage value (new `hasAnyPrCoverage()` — repos without coverage return `diffCoverage.cause` and no numbers on any PR); missing metric values now render as a dim `-` instead of `N/A` in `formatDelta`/`formatPrCoverage`/`formatPrIssues`, matching `formatStandards`/`formatCountCell`/`formatCoverageCell`; and a zero issue count renders as a bare `0` rather than `+0`/`-0` (`-0` read as a negative), matching what `pull-request`'s Files table and `formatDelta` already did. **JSON:** added `quality.resultReasons`/`coverage.resultReasons` (Codacy review suggestion — they drive the per-metric gate coloring, so consumers need them to see which gates passed/failed) plus the `quality.*` metric mirrors the table actually renders (23 new tests, 544 total) | | 2026-08-11 | (OD-489) Repository (project) token support. New `--repository-token ` on every command (plus `CODACY_PROJECT_TOKEN`), sent as the `project-token` header; account tokens keep `api-token`. `src/utils/auth.ts` rewritten around a `RemoteAuth` discriminated union carrying both kind and source, replacing `checkApiToken()` with `resolveAuth(this)` / `resolveAccountAuth(this, why)` / `requireAccountToken(...)` / `fetchIfAccountToken(...)`. Precedence matches `codacy-analysis` exactly — flag > `CODACY_PROJECT_TOKEN` > `CODACY_API_TOKEN` > stored login — so `vitest.config.mts` now blanks `CODACY_PROJECT_TOKEN` (it outranks the account token and is exported job-wide by the coverage reporter, so tests would otherwise depend on the developer's shell). Codacy whitelists only 13 operations for repository tokens, so `tool`/`patterns`/`pattern` work unchanged, `issues` (incl. `--overview`) and `tools --import` work, and the 9 account-only commands plus `repository`'s 6 management flags, `issues --ignore`/`--ignored`, and `tools --import --force` (only when standards exist) **fail fast before any request** with a message naming the operation, the reason, and where the token came from. `repository`'s dashboard skips the two non-whitelisted calls: the table keeps the "Open Pull Requests" header with an explanatory line, and JSON keeps `pullRequests: []` (so `jq '.pullRequests[]'` still works) plus an additive `unavailable: ["pullRequests"]` — under an account token the payload is byte-identical. Also added the long-missing `.catch()` on the PR call so an account token lacking PR access degrades instead of losing the whole dashboard, and fixed `login`'s 401 message, which told repository-token users their token was "invalid" when it is rejected by `/user` by design. New `SPECS/repository-tokens.md` (whitelist + matrix, re-verify on every `npm run update-api`) and `SPECS/missing-endpoints.md` (ranked gaps for follow-up Linear tasks) (40 new tests, 606 total) | | 2026-09-07 | HTTP/HTTPS proxy + TLS support (issue #40). Node's global `fetch` — used by the generated client and the MITRE CVE lookup in `commands/finding.ts` — ignores `HTTP_PROXY`/`HTTPS_PROXY`/`NO_PROXY`, so the CLI was unusable behind a corporate proxy. Rather than reimplement it, this delegates to `configureProxy()` from `@codacy/tooling` (pinned `0.1.0` → `0.23.0`, the same function `analysis-cli` calls), which installs a global `undici` dispatcher doing per-request protocol + `NO_PROXY` routing, bare `host:port` normalization, and `SSL_CERT_FILE`/`NODE_EXTRA_CA_CERTS` CA loading. New `src/utils/proxy.ts` is a ~4-line seam — `configureProxyFromEnv()` calls it and routes its deliberate fail-loud throw (unreadable/non-PEM CA bundle) into `handleError()`, giving red `Error: ` and exit 1 like every other failure here; `analysis-cli` exits 2 because it has a documented exit-code scheme, which this CLI does not. Called at the top of `src/index.ts`, above `OpenAPI.BASE` (ordering is only constrained to precede `program.parse`, since the dispatcher is resolved per request). Kept top-level rather than in the `preAction` hook so a typo'd `SSL_CERT_FILE` fails even on `--version`. Deliberately zero-argument: env is the sole input, which is what keeps parity exact. Superseded external PR #39, which hand-rolled the same feature with `undici@8.10.1` — that requires Node ≥ 22.19.0 against this package's `engines: ">=20"`, so `require("undici")` threw at module load and the CLI would not start at all on any Node 20.x; tooling's `undici@^6.21.0` supports Node ≥ 18.17. That regression passed CI, so `ci.yml` gained a smoke step running the built entry point (plain, with `HTTPS_PROXY`, and with a bad `SSL_CERT_FILE` expected to fail) — previously nothing executed `src/index.ts`, since every command test builds a bare `new Command()`. Upstream owns the proxy semantics and their 24 tests, so only the seam is tested here. Pinned exactly rather than with a caret: for a pre-1.0 package `^0.22.0` spans patches only (`>=0.22.0 <0.23.0-0`), so a caret would have bought silent patch drift against a dependency this repo has no proxy coverage for, without ever picking up a minor. Two findings from this work were fixed upstream and taken here via 0.23.0 — `undici` now loads lazily behind `configureProxy`'s early-out (an unproxied `--version` went from +27 ms to +0 ms against a `main` build), and a malformed proxy URL now fails with `Invalid HTTPS_PROXY value "...": `, naming the setting and redacting any credentials instead of surfacing a bare `Invalid URL` (4 new tests, 614 total) | -| 2026-09-09 | New `-k, --matches-stack [value]` filter on `patterns`, surfacing the API's `matchesStack` query param (filter a tool's code patterns by whether they match the repository's detected stack). Tri-state, matching the existing `issues --false-positives`: the bare flag or `true` sends `matchesStack=true`, `false` sends `matchesStack=false`, omitting it sends nothing — read explicitly rather than by truthiness so an explicit `false` stays distinct from "not requested". Applies in **both** list mode (`listRepositoryToolPatterns`) and bulk mode (`updateRepositoryToolPatterns`), like every other filter; the post-update `toolPatternsOverview` call deliberately stays unfiltered, since its counts describe the whole tool rather than the updated subset. The shared tri-state coercion `parseBooleanOption` moved out of `issues.ts` into a new `utils/options.ts` (+ tests) and is now imported by both commands. **Required an API bump: pinned `57.3.9` → `57.4.17`** (`matchesStack` first ships in `57.4.14`; `57.4.17` is the latest published build). The spec delta is purely additive — 2 unused new operations, 4 new schemas, `stackTagsFilterParam` on `listOrganizationRepositories` (unused; the CLI calls `...WithAnalysis`) — but `matchesStack` is inserted *mid-signature* on `listRepositoryToolPatterns` (arg 12, before `sort`), so every full-positional-arg assertion in `patterns.test.ts` gained a trailing `undefined`; `pattern.ts`/`issues.ts` stop at `search` (arg 9) and were unaffected. `SPECS/repository-tokens.md` re-verified: `57.4.x` now declares the `ProjectTokenAuth` scheme in the spec (it was absent in `57.3.9`), making the whitelist machine-checkable, and it is **14** operations, not 13 — the addition is `searchAiInventoryCategories`, unused here. `patterns` stays fully whitelisted, so no new token guard (11 new tests, 625 total) | +| 2026-09-09 | New `-k, --matches-stack [value]` filter on `patterns`, surfacing the API's `matchesStack` query param (filter a tool's code patterns by whether they match the repository's detected stack). Tri-state, matching the existing `issues --false-positives`: the bare flag or `true` sends `matchesStack=true`, `false` sends `matchesStack=false`, omitting it sends nothing — read explicitly rather than by truthiness so an explicit `false` stays distinct from "not requested". Applies in **both** list mode (`listRepositoryToolPatterns`) and bulk mode (`updateRepositoryToolPatterns`), like every other filter; the post-update `toolPatternsOverview` call deliberately stays unfiltered, since its counts describe the whole tool rather than the updated subset. The shared tri-state coercion `parseBooleanOption` moved out of `issues.ts` into a new `utils/options.ts` (+ tests) and is now imported by both commands. **Required an API bump: pinned `57.3.9` → `57.4.17`** (`matchesStack` first ships in `57.4.14`; `57.4.17` is the latest published build). The spec delta is purely additive — 2 unused new operations, 4 new schemas, `stackTagsFilterParam` on `listOrganizationRepositories` (unused; the CLI calls `...WithAnalysis`) — but `matchesStack` is inserted *mid-signature* on `listRepositoryToolPatterns` (arg 12, before `sort`), so every full-positional-arg assertion in `patterns.test.ts` gained a trailing `undefined`; `pattern.ts`/`issues.ts` stop at `search` (arg 9) and were unaffected. `SPECS/repository-tokens.md` re-verified: `57.4.x` now declares the `ProjectTokenAuth` scheme in the spec (it was absent in `57.3.9`), making the whitelist machine-checkable, and it is **14** operations, not 13 — the addition is `searchAiInventoryCategories`, unused here. `patterns` stays fully whitelisted, so no new token guard. **Review follow-up:** `--matches-stack` parses strictly (`strictBooleanOption` in the new `utils/options.ts`) rather than reusing the lax `parseBooleanOption` — Commander's `[value]` syntax eats the next token, so `patterns gh org repo --matches-stack eslint` otherwise swallowed the tool name and failed with a positional-count error that never named the flag; it now errors immediately with a message saying where to put the argument. `issues --false-positives` keeps the lax parser and the same latent hazard — out of scope here, flagged in the spec as a follow-up (16 new tests, 630 total) | 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.`, + ); + }; +}