diff --git a/AGENTS.md b/AGENTS.md index 73b05f7..ed719c7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -55,6 +55,7 @@ Requires the `project` (or `read:project`) OAuth scope on the `gh` token; `src/e `gh` accepts `--label`, `--assignee`, `--reviewer`, `--project`, and the `--add-*`/`--remove-*` variants once per value, so gh-axi must collect _every_ occurrence. Use `getAllFlags`/`takeAllFlags` plus `pushRepeated`; `getFlag`/`takeFlag` keep only the first occurrence and silently discard the rest, which is the bug that recurred as #55, #57, and #75. Both collectors reject a dangling (`--label` with nothing after it) or blank (`--label=`) value with a `VALIDATION_ERROR` instead of dropping it. +`getFlag`/`takeFlag` similarly reject a flag-like value (one starting with `--`) rather than treating it as the value, so a dangling single-value flag can't silently swallow the next flag's token. Pick the collector that matches the surrounding file: `issue.ts` reads args non-destructively (`getAllFlags`), `pr.ts` consumes them (`takeAllFlags`). When a flag becomes repeatable, mark it `(repeatable)` in that command's `*_HELP` string. diff --git a/src/args.ts b/src/args.ts index 2b6c439..528519f 100644 --- a/src/args.ts +++ b/src/args.ts @@ -4,6 +4,16 @@ function flagEqualsPrefix(flag: string): string { return `${flag}=`; } +/** + * A dangling flag (no value given before the next `--flag` token) must never + * silently swallow that next flag as its own value - doing so both loses the + * next flag and can leak the wrong token into a positional slot downstream. + */ +function rejectFlagLikeValue(value: string, flag: string): void { + if (value.startsWith("--")) + throw new AxiError(`${flag} requires a value`, "VALIDATION_ERROR"); +} + /** Get a flag's value from --flag value or --flag=value without modifying args. */ export function getFlag(args: string[], name: string): string | undefined { const equalsPrefix = flagEqualsPrefix(name); @@ -11,7 +21,9 @@ export function getFlag(args: string[], name: string): string | undefined { const arg = args[i]; if (arg === name) { if (i + 1 >= args.length) return undefined; - return args[i + 1]; + const val = args[i + 1]; + rejectFlagLikeValue(val, name); + return val; } if (arg.startsWith(equalsPrefix)) { return arg.slice(equalsPrefix.length); @@ -27,6 +39,7 @@ export function takeFlag(args: string[], flag: string): string | undefined { const arg = args[i]; if (arg === flag) { const val = args[i + 1]; + if (val !== undefined) rejectFlagLikeValue(val, flag); args.splice(i, 2); return val; } @@ -53,7 +66,7 @@ export function takeBoolFlag(args: string[], flag: string): boolean { } function requireFlagValue(value: string, flag: string): string { - if (value.trim() === "") + if (value.trim() === "" || value.startsWith("--")) throw new AxiError(`${flag} requires a value`, "VALIDATION_ERROR"); return value; } diff --git a/src/commands/repo.ts b/src/commands/repo.ts index 8fe8ae6..9776da8 100644 --- a/src/commands/repo.ts +++ b/src/commands/repo.ts @@ -2,7 +2,7 @@ import { encode } from '@toon-format/toon'; import type { RepoContext } from '../context.js'; import { ghJson, ghExec } from '../gh.js'; import { AxiError } from '../errors.js'; -import { getFlag, hasFlag } from '../args.js'; +import { getFlag, hasFlag, takeFlag, takeBoolFlag } from '../args.js'; import { field, lower, @@ -167,21 +167,26 @@ async function forkRepo(args: string[], ctx?: RepoContext): Promise { } async function listRepos(args: string[], ctx?: RepoContext): Promise { + // Consume value-taking flags before scanning for positionals, so a bare + // flag value (e.g. the "200" in --limit 200) can never be mistaken for + // the optional owner positional when no owner is given. + const limit = takeFlag(args, '--limit') ?? '30'; + const visibility = takeFlag(args, '--visibility'); + const language = takeFlag(args, '--language'); + const archived = takeBoolFlag(args, '--archived'); + const positionals = args.filter((a) => !a.startsWith('--')); const owner = positionals[1]; // optional - const limit = getFlag(args, '--limit') ?? '30'; const ghArgs = [ 'repo', 'list', '--json', 'name,description,visibility,primaryLanguage,stargazerCount,updatedAt', '--limit', limit, ]; if (owner) ghArgs.splice(2, 0, owner); // insert owner after 'list' - const visibility = getFlag(args, '--visibility'); if (visibility) ghArgs.push('--visibility', visibility); - const language = getFlag(args, '--language'); if (language) ghArgs.push('--language', language); - if (hasFlag(args, '--archived')) ghArgs.push('--archived'); + if (archived) ghArgs.push('--archived'); const repos = await ghJson[]>(ghArgs); const isEmpty = repos.length === 0; diff --git a/src/commands/run.ts b/src/commands/run.ts index 9fa8627..3a59be0 100644 --- a/src/commands/run.ts +++ b/src/commands/run.ts @@ -94,7 +94,7 @@ function takeViewFlagValue(args: string[], flag: string): string | undefined { const present = args.includes(flag); const value = takeFlag(args, flag); if (!present) return undefined; - if (!value || value.startsWith("--")) { + if (!value) { throw new AxiError(`Missing value for ${flag}`, "VALIDATION_ERROR"); } return value; diff --git a/test/args.test.ts b/test/args.test.ts index e9afe84..10ee7dd 100644 --- a/test/args.test.ts +++ b/test/args.test.ts @@ -33,6 +33,12 @@ describe("getFlag", () => { expect(getFlag(["--state", "open", "--repo"], "--repo")).toBeUndefined(); }); + it("throws instead of swallowing an adjacent flag as the value", () => { + expect(() => + getFlag(["--repo", "--state", "open"], "--repo"), + ).toThrow("--repo requires a value"); + }); + it("does not modify the args array", () => { const args = ["--repo", "cli/cli"]; getFlag(args, "--repo"); @@ -68,6 +74,14 @@ describe("takeFlag", () => { expect(val).toBe("val"); expect(args).toEqual(["--other"]); }); + + it("throws instead of swallowing an adjacent flag as the value", () => { + const args = ["--limit", "--visibility", "public"]; + expect(() => takeFlag(args, "--limit")).toThrow( + "--limit requires a value", + ); + expect(args).toEqual(["--limit", "--visibility", "public"]); + }); }); describe("hasFlag", () => { diff --git a/test/commands/repo.test.ts b/test/commands/repo.test.ts index d70a239..02f7516 100644 --- a/test/commands/repo.test.ts +++ b/test/commands/repo.test.ts @@ -200,6 +200,51 @@ describe('repoCommand', () => { expect(result).toContain('showing first 10'); }); + + it('does not mistake the --limit value for the owner positional when no owner is given', async () => { + mockedGhJson.mockResolvedValue([]); + + await repoCommand(['list', '--limit', '10'], ctx); + + expect(mockedGhJson).toHaveBeenCalledWith([ + 'repo', 'list', + '--json', 'name,description,visibility,primaryLanguage,stargazerCount,updatedAt', + '--limit', '10', + ]); + }); + + it('passes an explicit owner positional through to gh repo list', async () => { + mockedGhJson.mockResolvedValue([]); + + await repoCommand(['list', 'octo', '--limit', '10'], ctx); + + expect(mockedGhJson).toHaveBeenCalledWith([ + 'repo', 'list', 'octo', + '--json', 'name,description,visibility,primaryLanguage,stargazerCount,updatedAt', + '--limit', '10', + ]); + }); + + it('throws instead of misreading an adjacent flag as the --limit value', async () => { + await expect( + repoCommand(['list', '--limit', '--visibility', 'public'], ctx), + ).rejects.toThrow('--limit requires a value'); + expect(mockedGhJson).not.toHaveBeenCalled(); + }); + + it('does not mistake --visibility or --language values for the owner positional', async () => { + mockedGhJson.mockResolvedValue([]); + + await repoCommand(['list', '--visibility', 'public', '--language', 'TypeScript'], ctx); + + expect(mockedGhJson).toHaveBeenCalledWith([ + 'repo', 'list', + '--json', 'name,description,visibility,primaryLanguage,stargazerCount,updatedAt', + '--limit', '30', + '--visibility', 'public', + '--language', 'TypeScript', + ]); + }); }); describe('clone', () => { diff --git a/test/commands/run.test.ts b/test/commands/run.test.ts index 9aeadab..26e786c 100644 --- a/test/commands/run.test.ts +++ b/test/commands/run.test.ts @@ -455,7 +455,7 @@ describe("runCommand", () => { runCommand(["view", "100", "--job", "--log"], ctx), ).rejects.toMatchObject({ code: "VALIDATION_ERROR", - message: "Missing value for --job", + message: "--job requires a value", }); expect(mockedGhExec).not.toHaveBeenCalled(); expect(mockedGhJson).not.toHaveBeenCalled();