From fa8db56a96d0d477886f0bc071257c0c350d1e5f Mon Sep 17 00:00:00 2001 From: OluwaJomiloju Date: Thu, 3 Sep 2026 18:40:55 +0100 Subject: [PATCH] fix(portal-clis): reject undefined single-dash flags in the unknown-flag guard (#428) The guard in the four bunli-based CLIs inspected only tokens starting with `--`, so an undefined short flag bypassed it: bunli discarded it, the search ran unfiltered, and the CLI exited 0. Live against jobnet, `search -q "sygeplejerske"` returned all 18,179 ads as a successful search against 667 for the real `--search-string` query - the same shape as review finding F13 that motivated the guard. Both dash forms are now checked. Declared shorts (jobindex's -q) and bunli's built-in -h/-v stay valid. A negative number is rejected too: bunli discards a `-`-prefixed token rather than consuming it as the previous flag's value, so `--radius -5` silently fell back to the default instead of failing its own min(1) schema; a value that must begin with a dash uses the `--flag=value` form. Fixes #426. Co-authored-by: Claude Opus 5 --- .agents/skills/jobbank-search/cli/src/cli.ts | 45 +++++++++++++------ .../cli/tests/cli-flag-validation.test.ts | 29 ++++++++++++ .../skills/jobdanmark-search/cli/src/cli.ts | 45 +++++++++++++------ .../cli/tests/cli-flag-validation.test.ts | 29 ++++++++++++ .agents/skills/jobindex-search/cli/src/cli.ts | 45 +++++++++++++------ .../cli/tests/cli-flag-validation.test.ts | 36 +++++++++++++++ .agents/skills/jobnet-search/cli/src/cli.ts | 45 +++++++++++++------ .../cli/tests/cli-flag-validation.test.ts | 29 ++++++++++++ CHANGELOG.md | 21 +++++++++ 9 files changed, 272 insertions(+), 52 deletions(-) diff --git a/.agents/skills/jobbank-search/cli/src/cli.ts b/.agents/skills/jobbank-search/cli/src/cli.ts index da7ab5d..4bc771b 100644 --- a/.agents/skills/jobbank-search/cli/src/cli.ts +++ b/.agents/skills/jobbank-search/cli/src/cli.ts @@ -14,30 +14,49 @@ for (const command of commands) { cli.command(command) } -// Reject unknown --flags before dispatch. bunli silently discards them, and a +// Reject unknown flags before dispatch. bunli silently discards them, and a // silently discarded filter changes what the search returns without any error // (a wrong flag name once returned an entire portal's database as if it // matched the query). add-portal.md's contract requires a bogus flag to exit 1 // with a JSON error on stderr; this enforces it for the reference CLIs too. +// +// Both dash forms are checked. This loop inspected only `--long` tokens until +// #426, so an undefined short flag was discarded in silence: `-q "..."` on a +// portal whose keyword flag is `--search-string` returned the whole database +// as a successful, unfiltered search. Declared shorts and bunli's built-in +// -h/-v stay valid; every other single-dash token is rejected, including a +// negative number. bunli does not consume a `-`-prefixed token as the previous +// flag's value - it discards it - so `--radius -5` silently fell back to the +// default radius rather than failing its own `min(1)` schema. Erroring on it +// is the same trade linkedin-search already makes. A value that must begin +// with a dash uses the `--flag=value` form, which is checked as a long flag. const argv = process.argv.slice(2) const invoked = commands.find((c) => (c as { name?: string }).name === argv[0]) if (invoked) { - const known = new Set([ - ...Object.keys((invoked as { options?: Record }).options ?? {}), - "help", - "version", - ]) + const options = + (invoked as { options?: Record }).options ?? {} + const known = new Set([...Object.keys(options), "help", "version"]) + const knownShorts = new Set( + Object.values(options) + .map((o) => o?.short) + .filter((s): s is string => typeof s === "string") + .concat("h", "v"), + ) + const rejectFlag = (rendered: string): never => { + writeError( + `unknown flag ${rendered} for '${argv[0]}' - flags are never silently ignored, because a discarded filter changes what the search returns; see --help for the supported flags`, + "UNKNOWN_FLAG", + ) + process.exit(1) + } for (const token of argv.slice(1)) { if (token === "--") break if (token.startsWith("--")) { const flag = token.slice(2).split("=")[0] - if (!known.has(flag)) { - writeError( - `unknown flag --${flag} for '${argv[0]}' - flags are never silently ignored, because a discarded filter changes what the search returns; see --help for the supported flags`, - "UNKNOWN_FLAG", - ) - process.exit(1) - } + if (!known.has(flag)) rejectFlag(`--${flag}`) + } else if (token.startsWith("-") && token !== "-") { + const flag = token.slice(1).split("=")[0] + if (!knownShorts.has(flag)) rejectFlag(`-${flag}`) } } } diff --git a/.agents/skills/jobbank-search/cli/tests/cli-flag-validation.test.ts b/.agents/skills/jobbank-search/cli/tests/cli-flag-validation.test.ts index 2bc133f..3f57af5 100644 --- a/.agents/skills/jobbank-search/cli/tests/cli-flag-validation.test.ts +++ b/.agents/skills/jobbank-search/cli/tests/cli-flag-validation.test.ts @@ -79,4 +79,33 @@ describe("unknown flag rejection", () => { expect(result.exitCode).toBe(1); expect(JSON.parse(result.stderr).code).toBe("UNKNOWN_FLAG"); }); + // #426: the guard inspected only `--long` tokens, so a single-dash flag was + // discarded in silence - the same failure the long-form tests above pin, + // reached by the likelier route. `-q` is the documented short for the + // keyword search in linkedin-search, freehire-search and jobindex-search, + // so it is what a cross-portal habit produces here; live, it returned the + // portal's entire database as a successful, unfiltered search. + test("-q (another portal's short flag) is rejected, not treated as no filter", async () => { + const result = await runCLI(["search", "-q", "test"]); + expect(result.exitCode).toBe(1); + expect(result.stdout).toBe(""); + const error = JSON.parse(result.stderr); + expect(error.code).toBe("UNKNOWN_FLAG"); + expect(error.error).toContain("-q"); + }); + + // bunli discards a `-`-prefixed token instead of consuming it as the + // previous flag's value, so a negative number never reached the option's + // own schema - it silently fell back to the default. Loud beats silent. + test("a negative number is rejected instead of silently falling back to the default", async () => { + const result = await runCLI(["search", "--key", "test", "--limit", "-5"]); + expect(result.exitCode).toBe(1); + expect(JSON.parse(result.stderr).code).toBe("UNKNOWN_FLAG"); + }); + + test("-h still prints help rather than being rejected as unknown", async () => { + const result = await runCLI(["search", "-h"]); + expect(result.exitCode).toBe(0); + expect(result.stderr).toBe(""); + }); }); diff --git a/.agents/skills/jobdanmark-search/cli/src/cli.ts b/.agents/skills/jobdanmark-search/cli/src/cli.ts index be6b536..47910e9 100644 --- a/.agents/skills/jobdanmark-search/cli/src/cli.ts +++ b/.agents/skills/jobdanmark-search/cli/src/cli.ts @@ -17,30 +17,49 @@ for (const command of commands) { cli.command(command) } -// Reject unknown --flags before dispatch. bunli silently discards them, and a +// Reject unknown flags before dispatch. bunli silently discards them, and a // silently discarded filter changes what the search returns without any error // (a wrong flag name once returned an entire portal's database as if it // matched the query). add-portal.md's contract requires a bogus flag to exit 1 // with a JSON error on stderr; this enforces it for the reference CLIs too. +// +// Both dash forms are checked. This loop inspected only `--long` tokens until +// #426, so an undefined short flag was discarded in silence: `-q "..."` on a +// portal whose keyword flag is `--search-string` returned the whole database +// as a successful, unfiltered search. Declared shorts and bunli's built-in +// -h/-v stay valid; every other single-dash token is rejected, including a +// negative number. bunli does not consume a `-`-prefixed token as the previous +// flag's value - it discards it - so `--radius -5` silently fell back to the +// default radius rather than failing its own `min(1)` schema. Erroring on it +// is the same trade linkedin-search already makes. A value that must begin +// with a dash uses the `--flag=value` form, which is checked as a long flag. const argv = process.argv.slice(2) const invoked = commands.find((c) => (c as { name?: string }).name === argv[0]) if (invoked) { - const known = new Set([ - ...Object.keys((invoked as { options?: Record }).options ?? {}), - "help", - "version", - ]) + const options = + (invoked as { options?: Record }).options ?? {} + const known = new Set([...Object.keys(options), "help", "version"]) + const knownShorts = new Set( + Object.values(options) + .map((o) => o?.short) + .filter((s): s is string => typeof s === "string") + .concat("h", "v"), + ) + const rejectFlag = (rendered: string): never => { + writeError( + `unknown flag ${rendered} for '${argv[0]}' - flags are never silently ignored, because a discarded filter changes what the search returns; see --help for the supported flags`, + "UNKNOWN_FLAG", + ) + process.exit(1) + } for (const token of argv.slice(1)) { if (token === "--") break if (token.startsWith("--")) { const flag = token.slice(2).split("=")[0] - if (!known.has(flag)) { - writeError( - `unknown flag --${flag} for '${argv[0]}' - flags are never silently ignored, because a discarded filter changes what the search returns; see --help for the supported flags`, - "UNKNOWN_FLAG", - ) - process.exit(1) - } + if (!known.has(flag)) rejectFlag(`--${flag}`) + } else if (token.startsWith("-") && token !== "-") { + const flag = token.slice(1).split("=")[0] + if (!knownShorts.has(flag)) rejectFlag(`-${flag}`) } } } diff --git a/.agents/skills/jobdanmark-search/cli/tests/cli-flag-validation.test.ts b/.agents/skills/jobdanmark-search/cli/tests/cli-flag-validation.test.ts index 380d9e6..dd5bc3c 100644 --- a/.agents/skills/jobdanmark-search/cli/tests/cli-flag-validation.test.ts +++ b/.agents/skills/jobdanmark-search/cli/tests/cli-flag-validation.test.ts @@ -94,4 +94,33 @@ describe("unknown flag rejection", () => { expect(result.exitCode).toBe(1); expect(JSON.parse(result.stderr).code).toBe("UNKNOWN_FLAG"); }); + // #426: the guard inspected only `--long` tokens, so a single-dash flag was + // discarded in silence - the same failure the long-form tests above pin, + // reached by the likelier route. `-q` is the documented short for the + // keyword search in linkedin-search, freehire-search and jobindex-search, + // so it is what a cross-portal habit produces here; live, it returned the + // portal's entire database as a successful, unfiltered search. + test("-q (another portal's short flag) is rejected, not treated as no filter", async () => { + const result = await runCLI(["search", "-q", "test"]); + expect(result.exitCode).toBe(1); + expect(result.stdout).toBe(""); + const error = JSON.parse(result.stderr); + expect(error.code).toBe("UNKNOWN_FLAG"); + expect(error.error).toContain("-q"); + }); + + // bunli discards a `-`-prefixed token instead of consuming it as the + // previous flag's value, so a negative number never reached the option's + // own schema - it silently fell back to the default. Loud beats silent. + test("a negative number is rejected instead of silently falling back to the default", async () => { + const result = await runCLI(["search", "--text", "test", "--limit", "-5"]); + expect(result.exitCode).toBe(1); + expect(JSON.parse(result.stderr).code).toBe("UNKNOWN_FLAG"); + }); + + test("-h still prints help rather than being rejected as unknown", async () => { + const result = await runCLI(["search", "-h"]); + expect(result.exitCode).toBe(0); + expect(result.stderr).toBe(""); + }); }); diff --git a/.agents/skills/jobindex-search/cli/src/cli.ts b/.agents/skills/jobindex-search/cli/src/cli.ts index e816916..e496c07 100644 --- a/.agents/skills/jobindex-search/cli/src/cli.ts +++ b/.agents/skills/jobindex-search/cli/src/cli.ts @@ -14,30 +14,49 @@ for (const command of commands) { cli.command(command) } -// Reject unknown --flags before dispatch. bunli silently discards them, and a +// Reject unknown flags before dispatch. bunli silently discards them, and a // silently discarded filter changes what the search returns without any error // (a wrong flag name once returned an entire portal's database as if it // matched the query). add-portal.md's contract requires a bogus flag to exit 1 // with a JSON error on stderr; this enforces it for the reference CLIs too. +// +// Both dash forms are checked. This loop inspected only `--long` tokens until +// #426, so an undefined short flag was discarded in silence: `-q "..."` on a +// portal whose keyword flag is `--search-string` returned the whole database +// as a successful, unfiltered search. Declared shorts and bunli's built-in +// -h/-v stay valid; every other single-dash token is rejected, including a +// negative number. bunli does not consume a `-`-prefixed token as the previous +// flag's value - it discards it - so `--radius -5` silently fell back to the +// default radius rather than failing its own `min(1)` schema. Erroring on it +// is the same trade linkedin-search already makes. A value that must begin +// with a dash uses the `--flag=value` form, which is checked as a long flag. const argv = process.argv.slice(2) const invoked = commands.find((c) => (c as { name?: string }).name === argv[0]) if (invoked) { - const known = new Set([ - ...Object.keys((invoked as { options?: Record }).options ?? {}), - "help", - "version", - ]) + const options = + (invoked as { options?: Record }).options ?? {} + const known = new Set([...Object.keys(options), "help", "version"]) + const knownShorts = new Set( + Object.values(options) + .map((o) => o?.short) + .filter((s): s is string => typeof s === "string") + .concat("h", "v"), + ) + const rejectFlag = (rendered: string): never => { + writeError( + `unknown flag ${rendered} for '${argv[0]}' - flags are never silently ignored, because a discarded filter changes what the search returns; see --help for the supported flags`, + "UNKNOWN_FLAG", + ) + process.exit(1) + } for (const token of argv.slice(1)) { if (token === "--") break if (token.startsWith("--")) { const flag = token.slice(2).split("=")[0] - if (!known.has(flag)) { - writeError( - `unknown flag --${flag} for '${argv[0]}' - flags are never silently ignored, because a discarded filter changes what the search returns; see --help for the supported flags`, - "UNKNOWN_FLAG", - ) - process.exit(1) - } + if (!known.has(flag)) rejectFlag(`--${flag}`) + } else if (token.startsWith("-") && token !== "-") { + const flag = token.slice(1).split("=")[0] + if (!knownShorts.has(flag)) rejectFlag(`-${flag}`) } } } diff --git a/.agents/skills/jobindex-search/cli/tests/cli-flag-validation.test.ts b/.agents/skills/jobindex-search/cli/tests/cli-flag-validation.test.ts index bedb368..54f08ef 100644 --- a/.agents/skills/jobindex-search/cli/tests/cli-flag-validation.test.ts +++ b/.agents/skills/jobindex-search/cli/tests/cli-flag-validation.test.ts @@ -77,4 +77,40 @@ describe("unknown flag rejection", () => { expect(error.code).toBe("UNKNOWN_FLAG"); expect(error.error).toContain("--bogus-flag"); }); + + // #426: the guard inspected only `--long` tokens, so a single-dash flag was + // discarded in silence. This CLI is the one portal that declares a short + // (`-q` for --query), so the fix has to reject undeclared shorts without + // breaking the declared one. + test("an undeclared short flag exits 1 with a JSON error", async () => { + const result = await runCLI(["search", "-z", "bogus"]); + expect(result.exitCode).toBe(1); + expect(result.stdout).toBe(""); + const error = JSON.parse(result.stderr); + expect(error.code).toBe("UNKNOWN_FLAG"); + expect(error.error).toContain("-z"); + }); + + // Network-free proof that the declared short survives the guard: -q is + // scanned before --bogus-flag, so naming --bogus-flag in the error means -q + // passed. Asserting -q is accepted directly would require a live search. + test("the declared short -q passes the guard", async () => { + const result = await runCLI(["search", "-q", "test", "--bogus-flag", "xyz"]); + expect(result.exitCode).toBe(1); + const error = JSON.parse(result.stderr); + expect(error.error).toContain("--bogus-flag"); + expect(error.error).not.toContain("-q "); + }); + + test("a negative number is rejected instead of silently falling back to the default", async () => { + const result = await runCLI(["search", "--query", "test", "--limit", "-5"]); + expect(result.exitCode).toBe(1); + expect(JSON.parse(result.stderr).code).toBe("UNKNOWN_FLAG"); + }); + + test("-h still prints help rather than being rejected as unknown", async () => { + const result = await runCLI(["search", "-h"]); + expect(result.exitCode).toBe(0); + expect(result.stderr).toBe(""); + }); }); diff --git a/.agents/skills/jobnet-search/cli/src/cli.ts b/.agents/skills/jobnet-search/cli/src/cli.ts index 4436d81..3f69b3a 100644 --- a/.agents/skills/jobnet-search/cli/src/cli.ts +++ b/.agents/skills/jobnet-search/cli/src/cli.ts @@ -16,30 +16,49 @@ for (const command of commands) { cli.command(command) } -// Reject unknown --flags before dispatch. bunli silently discards them, and a +// Reject unknown flags before dispatch. bunli silently discards them, and a // silently discarded filter changes what the search returns without any error // (a wrong flag name once returned an entire portal's database as if it // matched the query). add-portal.md's contract requires a bogus flag to exit 1 // with a JSON error on stderr; this enforces it for the reference CLIs too. +// +// Both dash forms are checked. This loop inspected only `--long` tokens until +// #426, so an undefined short flag was discarded in silence: `-q "..."` on a +// portal whose keyword flag is `--search-string` returned the whole database +// as a successful, unfiltered search. Declared shorts and bunli's built-in +// -h/-v stay valid; every other single-dash token is rejected, including a +// negative number. bunli does not consume a `-`-prefixed token as the previous +// flag's value - it discards it - so `--radius -5` silently fell back to the +// default radius rather than failing its own `min(1)` schema. Erroring on it +// is the same trade linkedin-search already makes. A value that must begin +// with a dash uses the `--flag=value` form, which is checked as a long flag. const argv = process.argv.slice(2) const invoked = commands.find((c) => (c as { name?: string }).name === argv[0]) if (invoked) { - const known = new Set([ - ...Object.keys((invoked as { options?: Record }).options ?? {}), - "help", - "version", - ]) + const options = + (invoked as { options?: Record }).options ?? {} + const known = new Set([...Object.keys(options), "help", "version"]) + const knownShorts = new Set( + Object.values(options) + .map((o) => o?.short) + .filter((s): s is string => typeof s === "string") + .concat("h", "v"), + ) + const rejectFlag = (rendered: string): never => { + writeError( + `unknown flag ${rendered} for '${argv[0]}' - flags are never silently ignored, because a discarded filter changes what the search returns; see --help for the supported flags`, + "UNKNOWN_FLAG", + ) + process.exit(1) + } for (const token of argv.slice(1)) { if (token === "--") break if (token.startsWith("--")) { const flag = token.slice(2).split("=")[0] - if (!known.has(flag)) { - writeError( - `unknown flag --${flag} for '${argv[0]}' - flags are never silently ignored, because a discarded filter changes what the search returns; see --help for the supported flags`, - "UNKNOWN_FLAG", - ) - process.exit(1) - } + if (!known.has(flag)) rejectFlag(`--${flag}`) + } else if (token.startsWith("-") && token !== "-") { + const flag = token.slice(1).split("=")[0] + if (!knownShorts.has(flag)) rejectFlag(`-${flag}`) } } } diff --git a/.agents/skills/jobnet-search/cli/tests/cli-flag-validation.test.ts b/.agents/skills/jobnet-search/cli/tests/cli-flag-validation.test.ts index 76bc1b3..bfa6f63 100644 --- a/.agents/skills/jobnet-search/cli/tests/cli-flag-validation.test.ts +++ b/.agents/skills/jobnet-search/cli/tests/cli-flag-validation.test.ts @@ -94,4 +94,33 @@ describe("unknown flag rejection", () => { expect(result.exitCode).toBe(1); expect(JSON.parse(result.stderr).code).toBe("UNKNOWN_FLAG"); }); + // #426: the guard inspected only `--long` tokens, so a single-dash flag was + // discarded in silence - the same failure the long-form tests above pin, + // reached by the likelier route. `-q` is the documented short for the + // keyword search in linkedin-search, freehire-search and jobindex-search, + // so it is what a cross-portal habit produces here; live, it returned the + // portal's entire database as a successful, unfiltered search. + test("-q (another portal's short flag) is rejected, not treated as no filter", async () => { + const result = await runCLI(["search", "-q", "test"]); + expect(result.exitCode).toBe(1); + expect(result.stdout).toBe(""); + const error = JSON.parse(result.stderr); + expect(error.code).toBe("UNKNOWN_FLAG"); + expect(error.error).toContain("-q"); + }); + + // bunli discards a `-`-prefixed token instead of consuming it as the + // previous flag's value, so a negative number never reached the option's + // own schema - it silently fell back to the default. Loud beats silent. + test("a negative number is rejected instead of silently falling back to the default", async () => { + const result = await runCLI(["search", "--search-string", "test", "--limit", "-5"]); + expect(result.exitCode).toBe(1); + expect(JSON.parse(result.stderr).code).toBe("UNKNOWN_FLAG"); + }); + + test("-h still prints help rather than being rejected as unknown", async () => { + const result = await runCLI(["search", "-h"]); + expect(result.exitCode).toBe(0); + expect(result.stderr).toBe(""); + }); }); diff --git a/CHANGELOG.md b/CHANGELOG.md index d820b8a..fd15ef6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,27 @@ per-file diff commands. ### Fixed +- **The portal CLIs' unknown-flag guard no longer lets a single-dash flag through** (#426) - + the guard in the four bunli-based CLIs (`jobnet`, `jobbank`, `jobindex`, `jobdanmark`) inspected + only tokens starting with `--`, so an undefined *short* flag bypassed it entirely: bunli + discarded it, the search ran unfiltered, and the CLI exited 0 with no error. Live against + jobnet, `search -q "sygeplejerske"` returned all 18,179 ads as a successful search against 667 + for the real `--search-string` query - the same shape as review finding F13 (jobdanmark, 13,862 + results) that motivated the guard in the first place, reached by the likelier route: `-q` is the + documented short for the keyword search in `linkedin-search`, `freehire-search` and + `jobindex-search`, so a cross-portal habit produces it. Both dash forms are now checked, with + declared shorts (`jobindex`'s `-q`) and bunli's built-in `-h`/`-v` still valid. A negative number + is rejected too rather than skipped: bunli does not consume a `-`-prefixed token as the previous + flag's value, so `--radius -5` silently fell back to the default radius instead of failing its + own `min(1)` schema - erroring on it is the trade `linkedin-search` already makes, and a value + that must begin with a dash uses the `--flag=value` form. `linkedin-search` and + `freehire-search` were unaffected; they normalize `-x` to a long name before checking it. Pinned + by thirteen new cases across the four CLIs' `cli-flag-validation.test.ts`, network-free because + the guard runs before dispatch: eight bug-pinning cases (the short flag and the negative number, + per CLI), each verified to fail on the unfixed guard, plus five regression guards that pass on + both and exist to keep the fix from over-rejecting - `-h` in each CLI, and `jobindex`'s declared + `-q`. + - **`jobdanmark-search` autocomplete no longer dies over one suggestion without text** (#421, closing out the #416/#418 audit - every other deref site in the six CLIs checked and confirmed guarded) - the filter derefed `item.text.toLowerCase()` from a