diff --git a/packages/client/src/commands/gh.ts b/packages/client/src/commands/gh.ts index 56a35414..31c8dc7f 100644 --- a/packages/client/src/commands/gh.ts +++ b/packages/client/src/commands/gh.ts @@ -63,14 +63,22 @@ ${ACTION_LINES}`; * Run the `gh` command. * * @param argv - arguments after `gh`. - * @returns the process exit code: 0 success, 1 refusal. + * @returns the process exit code: 0 success, 1 refusal, and the requested unreachable code + * (1 by default; `pr` passes 3 for runPr's contract). */ -export async function runGh(argv: readonly string[]): Promise { - if (argv.length === 0 || argv.includes('--help') || argv.includes('-h')) { +export async function runGh( + argv: readonly string[], + options: { readonly unreachableExitCode?: number } = {}, +): Promise { + if (argv.length === 0) { say(GH_USAGE); - return argv.length === 0 ? 1 : 0; + return 1; + } + const [domain, command, ...rawRest] = argv; + if (domain === '--help' || domain === '-h' || command === '--help' || command === '-h') { + say(GH_USAGE); + return 0; } - const [domain, command, ...rest] = argv; if (domain === undefined || command === undefined) { // A caller with a domain and no command (`gcb run`, `gcb gh pr`) gets the catalog rather than one // example: the answer is every action, and the example would only be right for one of them. @@ -82,11 +90,95 @@ export async function runGh(argv: readonly string[]): Promise { try { action = getGhAction(actionName); } catch { + if (rawRest.includes('--help') || rawRest.includes('-h')) { + say(GH_USAGE); + return 0; + } fail(`unknown gh action ${JSON.stringify(actionName)}`); } + // A known value-taking flag consumes the next token only when that token is a value. A following + // known flag is not taken as a value (so a missing value fails as missing), except a literal + // --help/-h: a body that is exactly one of those is data, not a help request. The parser then + // sees --body=--help. + const valueFlags = new Set([ + ...action.flags.filter((flag) => flag.type !== 'boolean').map((flag) => flag.name), + 'repo', + 'R', + 'host', + 'socket', + ]); + + const allowed = new Set([ + ...action.flags.map((flag) => flag.name), + 'repo', + 'R', + 'host', + 'socket', + ]); + const knownFlags = new Set([...allowed, 'help']); + + const rest: string[] = []; + let missingValue: string | undefined; + let help = false; + for (let index = 0; index < rawRest.length; index += 1) { + const raw = rawRest[index]; + // Match parseArgs: -h is help everywhere; any other single-dash letter is its long flag. A + // single-dash operand belongs to a preceding value flag and was consumed before this point. + const argument = raw?.replace(/^-h$/, '--help').replace(/^-(?=[A-Za-z])/, '--'); + if (argument === undefined) continue; + if (argument === '--') { + rest.push(...rawRest.slice(index)); + break; + } + if (argument === '--help') { + help = true; + rest.push(argument); + continue; + } + const name = argument.startsWith('--') && !argument.includes('=') ? argument.slice(2) : undefined; + if (name === undefined || !valueFlags.has(name)) { + rest.push(argument); + continue; + } + const next = rawRest[index + 1]; + if (next === undefined) { + missingValue ??= `${raw} needs a value`; + rest.push(argument); + continue; + } + if (next === '--help' || next === '-h') { + if (name === 'body') { + rest.push(`${argument}=${next}`); + index += 1; + } else { + help = true; + rest.push(argument); + } + continue; + } + if (next.startsWith('--')) { + const nextName = next.slice(2).split('=')[0]; + if (nextName !== undefined && knownFlags.has(nextName)) { + missingValue ??= `${raw} needs a value`; + } + // Leave both tokens alone: a known flag is a missing value, an unknown one must reach the + // unknown-option check rather than becoming a value. + rest.push(argument); + continue; + } + rest.push(`${argument}=${next}`); + index += 1; + } + if (help) { + say(GH_USAGE); + return 0; + } + if (missingValue !== undefined) fail(missingValue); + const booleans = action.flags.filter((flag) => flag.type === 'boolean').map((flag) => flag.name); const args = parseArgs(rest, { booleans }); + const host = args.value('host') ?? 'github.com'; let repo = args.value('repo') ?? args.value('R'); if (repo === undefined) { @@ -100,14 +192,8 @@ export async function runGh(argv: readonly string[]): Promise { const socketPath = resolveSocketPath(args.value('socket'), process.env); if (socketPath === undefined) fail('no broker socket is configured'); - const allowed = new Set([ - ...action.flags.map((flag) => flag.name), - 'repo', - 'R', - 'host', - 'socket', - ]); for (const argument of rest) { + if (argument === '--') break; const match = /^--([^=]+)/.exec(argument); if (match?.[1] !== undefined && !allowed.has(match[1])) { fail(`unknown option --${match[1]} for gcb gh ${domain} ${command}`); @@ -148,7 +234,8 @@ export async function runGh(argv: readonly string[]): Promise { action.timeoutSeconds * 1000 + 30_000, ); } catch (error) { - fail((error as Error).message); + process.stderr.write(`unreachable: ${(error as Error).message}\n`); + return options.unreachableExitCode ?? 1; } if (response.ok !== true) { if (response.warning) process.stderr.write(`warning: ${response.warning}\n`); diff --git a/packages/client/src/commands/pr.ts b/packages/client/src/commands/pr.ts index f2016abd..729409bf 100644 --- a/packages/client/src/commands/pr.ts +++ b/packages/client/src/commands/pr.ts @@ -53,7 +53,8 @@ Opening (the default) Acting on one --number The pull request number [required] --status Report its state, mergeability and the runs for its commit - --comment Post the body as a comment on it (needs only --number and --body/--body-file) + --comment Post an anchored review comment (also needs --file and --line); + for a plain comment use: gcb gh pr comment --body ... --file Anchor that comment to a line of this file, repository-relative --line The line to anchor to, in the new file unless --side says otherwise --side Which side of the diff --line counts on (default: right) @@ -115,6 +116,11 @@ export async function runPr(argv: readonly string[]): Promise { if (args.has('close') || args.value('close') !== undefined) { fail('--close moved to: gcb gh pr close --repo owner/name'); } + // Before --repo too: a bare `gcb pr --comment` should hear what the flag needs, not that --repo + // is missing. An anchored comment (--file and --line) is still this command's to post. + if (args.has('comment') && args.value('file') === undefined && args.value('line') === undefined) { + fail('--comment only posts an anchored review comment; pass --file and --line'); + } const socketPath = resolveSocketPath(args.value('socket'), process.env); const host = args.value('host'); diff --git a/packages/client/src/index.ts b/packages/client/src/index.ts index e82db86a..9a896ef0 100644 --- a/packages/client/src/index.ts +++ b/packages/client/src/index.ts @@ -64,7 +64,7 @@ async function run(argv: readonly string[]): Promise { if (rest.length === 0) return (await import('./commands/gh.ts')).runGh(['pr']); const sub = rest[0]; if (sub !== undefined && Object.hasOwn(GH_ACTIONS, `pr:${sub}`)) { - return (await import('./commands/gh.ts')).runGh(['pr', ...rest]); + return (await import('./commands/gh.ts')).runGh(['pr', ...rest], { unreachableExitCode: 3 }); } return (await import('./commands/pr.ts')).runPr(rest); } diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts index c6adda73..de423f67 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -1166,6 +1166,16 @@ describe('the anchor flags, at the CLI boundary', () => { assert.match(result.stdout, /Usage: gcb gh/); }); + it('refuses a plain --comment and names the anchor requirement', () => { + const moved = cli([...base, '--comment', '--body', 'x']); + assert.equal(moved.status, 1); + assert.match(moved.stderr, /anchored review comment/); + + const bare = cli(['--comment']); + assert.equal(bare.status, 1); + assert.match(bare.stderr, /anchored review comment/, 'before --repo too'); + }); + it('points --close at the gh action', () => { const moved = cli([...base, '--close']); assert.equal(moved.status, 1); diff --git a/test/unit/gh-command.test.ts b/test/unit/gh-command.test.ts index 691f44da..11b59a6f 100644 --- a/test/unit/gh-command.test.ts +++ b/test/unit/gh-command.test.ts @@ -240,6 +240,68 @@ describe('gcb gh', () => { } }); + it('does not mistake a flag value for a help request', async () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-gh-body-help-')); + const socketPath = path.join(root, 'broker.sock'); + const block: StaticHostConfig = { provider: 'static', allow: ['acme/widget'] }; + const config: BrokerConfig = { + socketPath, + socketMode: 0o600, + auditPath: null, + tokenCacheSkewSeconds: 300, + hosts: { 'github.com': block }, + }; + const audit: AuditSink = { path: null, record: () => {}, close: () => Promise.resolve() }; + let seen: unknown; + const provider: Provider = { + name: 'static', + getCredential: async () => ({ username: 'u', password: 'p', expiresAt: Date.now() + 1000 }), + runAction: async (request) => { + seen = request.values; + return { stdout: '', stderr: '', code: 0, signal: null, truncated: false, timedOut: false }; + }, + }; + const broker = await startBroker({ config, audit, providers: new Map([['github.com', provider]]) }); + try { + const result = await runGhCli([ + 'pr', 'comment', '7', '--body', '--help', '--repo', 'acme/widget', '--socket', socketPath, + ]); + assert.equal(result.status, 0); + assert.deepEqual(seen, { number: '7', body: '--help' }); + + const shortHelp = await runGhCli([ + 'pr', 'comment', '7', '--body', '-h', '--repo', 'acme/widget', '--socket', socketPath, + ]); + assert.equal(shortHelp.status, 0); + assert.deepEqual(seen, { number: '7', body: '-h' }, 'a body of literally -h is data too'); + } finally { + await broker.close(); + fs.rmSync(root, { recursive: true, force: true }); + } + }); + + it('treats --help as help wherever it sits', async () => { + // No broker is involved: each case has to answer before the socket is looked for. + for (const argv of [ + ['--help'], + ['pr', 'view', '--help', '7'], + ['pr', 'view', '7', '--help'], + ['pr', 'view', '7', '-h'], + ['pr', 'create', '--title', '--head', '--help'], + ['pr', 'create', '--title', '-h', '--head', 'feat', '--body', 'b'], + ]) { + const result = await runGhCli(argv); + assert.equal(result.status, 0, argv.join(' ')); + assert.match(result.stdout, /Usage: gcb gh/); + } + }); + + it('keeps the pr unreachable exit code', async () => { + const result = await runClient(['pr', 'view', '7', '--repo', 'acme/widget', '--socket', '/nonexistent']); + assert.equal(result.status, 3); + assert.match(result.stderr, /unreachable/); + }); + it('sends a typed action to the broker and prints its stdout', async () => { const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-gh-command-')); const socketPath = path.join(root, 'broker.sock'); @@ -280,3 +342,65 @@ describe('bare run', () => { assert.match(result.stdout, /Usage: gcb gh/); }); }); + +describe('gh option values', () => { + it('refuses an optional value flag followed by another flag', async () => { + const result = await runGhCli([ + 'pr', 'create', '--head', 'feat', '--title', 't', '--body', 'b', '--reviewer', '--draft', + '--repo', 'acme/widget', '--socket', '/nonexistent', + ]); + assert.equal(result.status, 1); + assert.match(result.stderr, /--reviewer needs a value/); + }); + + it('refuses a value flag followed by another flag', async () => { + const result = await runGhCli([ + 'pr', 'create', '--head', 'feat', '--title', '--draft', '--body', 'b', + '--repo', 'acme/widget', '--socket', '/nonexistent', + ]); + assert.equal(result.status, 1); + assert.match(result.stderr, /--title needs a value/); + }); +}); + +describe('help after a value flag', () => { + it('prints help instead of a missing-value error', async () => { + const result = await runGhCli([ + 'pr', 'create', '--title', '--help', '--head', 'feat', '--body', 'b', + '--repo', 'acme/widget', '--socket', '/nonexistent', + ]); + assert.equal(result.status, 0); + assert.match(result.stdout, /Usage: gcb gh/); + }); +}); + +describe('unknown gh options', () => { + it('does not let an unknown flag become a value', async () => { + const result = await runGhCli([ + 'pr', 'comment', '7', '--body', 'x', '--bogus', 'y', + '--repo', 'acme/widget', '--socket', '/nonexistent', + ]); + assert.equal(result.status, 1); + assert.match(result.stderr, /unknown option --bogus/); + }); +}); + +describe('short gh flags', () => { + it('reports a missing -R value using the typed spelling', async () => { + const result = await runGhCli(['pr', 'view', '7', '-R']); + assert.equal(result.status, 1); + assert.match(result.stderr, /-R needs a value/); + }); +}); + +describe('the -- terminator', () => { + it('does not treat the token after -- as help', async () => { + const result = await runGhCli([ + 'pr', 'view', '7', '--repo', 'acme/widget', '--socket', '/nonexistent', + '--', '--help', + ]); + assert.equal(result.status, 1); + assert.doesNotMatch(result.stdout, /Usage: gcb gh/); + assert.match(result.stderr, /too many positional arguments/); + }); +}); diff --git a/tsconfig.build.base.json b/tsconfig.build.base.json index 5f56957c..36222007 100644 --- a/tsconfig.build.base.json +++ b/tsconfig.build.base.json @@ -4,7 +4,6 @@ "lib": ["ES2023"], "module": "nodenext", "moduleResolution": "nodenext", - "resolveJsonModule": true, "types": ["node"], "strict": true, diff --git a/tsconfig.json b/tsconfig.json index c284a8b4..312c1237 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -4,7 +4,6 @@ "lib": ["ES2023"], "module": "nodenext", "moduleResolution": "nodenext", - "resolveJsonModule": true, "types": ["node"], "strict": true,