diff --git a/AGENTS.md b/AGENTS.md index f9fe5f3b..e12f0bdb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -191,7 +191,9 @@ sources must stay erasable (`erasableSyntaxOnly` is on: no enums, no parameter p usually one a later push replaced: read the newest run for the commit, not the first. - **Open a pull request; the agent opens it, and can close or update it.** `gcb pr` covers most of the lifecycle: `--head`/`--base`/`--title` to open, - `--number` with `--close`, or `--number` with neither (which updates the title, body or base). + `--number` with none of the acting flags (which updates the title, body or base). Closing is the + catalogued `pr:close` action: `gcb pr close ` (or `gcb gh pr close`), and `--close` + refuses with a pointer there. The broker performs each action without the token leaving it, and prints the number, the URL and the state. diff --git a/README.md b/README.md index 8e675d6a..6dadde78 100644 --- a/README.md +++ b/README.md @@ -123,6 +123,11 @@ gcb gh run view 123456 --log --repo owner/name `-R` is the short form of `--repo`, as in `gh` itself. +`gcb pr ` and `gcb run ` are shorthands for the same actions when the +catalog has one — `gcb pr view 12`, `gcb pr close 12`, `gcb run list`. The flag-only `gcb pr` form +described below is still what runs the lifecycle `gh` cannot express (opening as a person, review +threads); `gcb pr --close` is gone and refuses with a pointer at `gcb gh pr close`. + Inside a checkout, `--repo` can be omitted only when `origin` names a usable GitHub repository whose host matches `--host`; otherwise pass `--repo`, or the matching `--host`. diff --git a/ROADMAP.md b/ROADMAP.md index e175ae96..f691d75c 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -204,8 +204,9 @@ host_broker: - built-ins first (`setup`, `init`, `stage`, `compose`, `probe`, `diagnose`, `authorize`, `app`, `host`, `config`, `version`, `help`); - `gcb gh …` explicit; - - `gcb pr view …`, `gcb run view …` etc. resolve to the `gh` subcommand. (Explicit - `gcb gh pr view` is wired; the shorthand is not.) + - `gcb pr view …`, `gcb run view …` etc. resolve to the `gh` subcommand. (Wired: any `pr` + subcommand the catalog has, plus `run`; the flag-only `gcb pr` stays for the lifecycle `gh` + cannot express, and `--close` refuses with a pointer at `gcb gh pr close`.) - [ ] Remove the built-in `pr` and `logs` commands. `gcb run view --job --log` replaces `logs`. - [x] Build the command table. Every entry has: - canonical subcommand (no aliases, no abbreviations); @@ -261,6 +262,7 @@ host_broker: | `pr:list` | `gh pr list` | `pull_requests: read` | | `pr:diff` | `gh pr diff` | `pull_requests: read`, `contents: read` | | `pr:create` | `gh pr create` | `pull_requests: write`, `contents: read` | + | `pr:close` | `gh pr close` | `pull_requests: write` | | `pr:comment` | `gh pr comment` | `pull_requests: write` | | `pr:review` | `gh pr review` | `pull_requests: write` | | `issue:create` | `gh issue create` | `issues: write` | diff --git a/packages/cli/src/actions.ts b/packages/cli/src/actions.ts index 6abf354d..bc8b9491 100644 --- a/packages/cli/src/actions.ts +++ b/packages/cli/src/actions.ts @@ -152,6 +152,17 @@ const PR_COMMENT: GhAction = { hostUser: true, }; +// GitHub GraphQL: closePullRequest +const PR_CLOSE: GhAction = { + name: 'pr:close', + command: ['pr', 'close'], + positionals: [{ name: 'number', type: 'number', required: true }], + flags: [], + permissions: { pull_requests: 'write' }, + timeoutSeconds: 60, + hostUser: true, +}; + // GitHub: POST /repos/{owner}/{repo}/pulls/{pull_number}/reviews const PR_REVIEW: GhAction = { name: 'pr:review', @@ -226,6 +237,7 @@ export const GH_ACTIONS: Readonly> = Object.freeze({ 'pr:list': PR_LIST, 'pr:diff': PR_DIFF, 'pr:create': PR_CREATE, + 'pr:close': PR_CLOSE, 'pr:comment': PR_COMMENT, 'pr:review': PR_REVIEW, 'issue:create': ISSUE_CREATE, diff --git a/packages/cli/src/index.ts b/packages/cli/src/index.ts index bdfdd9bc..0ceef907 100644 --- a/packages/cli/src/index.ts +++ b/packages/cli/src/index.ts @@ -20,7 +20,7 @@ import { platformRefusal } from './platform.ts'; import { packageVersion } from './version.ts'; /** Commands implemented by the container-side package. */ -export const CLIENT_COMMANDS = ['setup', 'probe', 'logs', 'pr', 'gh'] as const; +export const CLIENT_COMMANDS = ['setup', 'probe', 'logs', 'pr', 'run', 'gh'] as const; /** Commands implemented by the host-side package. */ export const HOST_COMMANDS = [ diff --git a/packages/cli/src/types.ts b/packages/cli/src/types.ts index 58156e9f..82aca14a 100644 --- a/packages/cli/src/types.ts +++ b/packages/cli/src/types.ts @@ -100,7 +100,6 @@ export interface JobLogRequest { /** What can be done to a pull request. */ export type PullRequestAction = | 'open' - | 'close' | 'merge' | 'update' | 'status' diff --git a/packages/client/src/commands/gh.ts b/packages/client/src/commands/gh.ts index d5bc76e2..56a35414 100644 --- a/packages/client/src/commands/gh.ts +++ b/packages/client/src/commands/gh.ts @@ -48,6 +48,7 @@ function repoFromCheckout(cwd: string): { repo: string; host: string } | undefin export const GH_USAGE = `Usage: gcb gh [options] Run a catalogued GitHub action through the broker. +gcb pr and gcb run are shorthands for the same actions. Common -R, --repo Repository the action belongs to (default: origin remote) @@ -71,7 +72,10 @@ export async function runGh(argv: readonly string[]): Promise { } const [domain, command, ...rest] = argv; if (domain === undefined || command === undefined) { - fail('gcb gh needs a command, for example: gcb gh pr view 12 --repo owner/name'); + // 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. + say(GH_USAGE); + return 1; } const actionName = `${domain}:${command}`; let action; diff --git a/packages/client/src/commands/pr.ts b/packages/client/src/commands/pr.ts index 328013c4..f2016abd 100644 --- a/packages/client/src/commands/pr.ts +++ b/packages/client/src/commands/pr.ts @@ -1,5 +1,8 @@ /** - * `pr` — open, close, merge or update one pull request through the broker. + * `pr` — open, merge or update one pull request through the broker. + * + * `pr ` runs as the catalogued gh action when the catalog has one (`pr:view`, + * `pr:close`, …); this file is the flag-only lifecycle for the actions gh cannot express. * * The credential never leaves the broker. The broker mints a token narrowed to `pull_requests: write` * plus read access to the branches it names, performs the action itself, and answers with the number, @@ -29,9 +32,12 @@ const PR_TIMEOUT_MS = 60_000; /** Usage text for `pr`. */ export const PR_USAGE = `Usage: gcb pr [options] -Open, close, merge, update or inspect a pull request through the broker, without the credential +Open, merge, update or inspect a pull request through the broker, without the credential ever reaching this side. +A subcommand the gh action catalog has (pr view, pr close, …) runs as that action instead; +gcb gh --help lists them. The flags below are the flag-only lifecycle gh cannot express. + Opening (the default) --head The branch holding the change [required] May be :, e.g. origin:feat/x, to name a fork @@ -56,7 +62,6 @@ Acting on one --edit Replace the body of that review comment, with --body/--body-file (an id --threads prints; no --number is needed) --resolve Mark that review thread resolved (the id --threads prints) - --close Close it --merge Merge it (branch protection still applies) --method merge, squash (default) or rebase With --number and none of the above, the title, body or base is updated. @@ -73,7 +78,7 @@ Common * Read the action out of the flags. * * @param args - the parsed arguments. - * @returns `close`, `merge`, `update` or `open`. + * @returns `merge`, `update` or `open`. */ function actionOf(args: { has(name: string): boolean; value(name: string): string | undefined }) { // `--resolve`, `--reply-to` and `--edit` take a value, and a bare one is parsed as a boolean flag with @@ -87,7 +92,6 @@ function actionOf(args: { has(name: string): boolean; value(name: string): strin if (args.has('threads')) return 'threads' as const; if (args.has('comment')) return 'comment' as const; if (args.has('status')) return 'status' as const; - if (args.has('close')) return 'close' as const; if (args.has('merge')) return 'merge' as const; if (args.value('number') !== undefined) return 'update' as const; return 'open' as const; @@ -101,12 +105,16 @@ function actionOf(args: { has(name: string): boolean; value(name: string): strin */ export async function runPr(argv: readonly string[]): Promise { const args = parseArgs(argv, { - booleans: ['help', 'draft', 'close', 'merge', 'status', 'comment', 'threads', 'via-host'], + booleans: ['help', 'draft', 'merge', 'status', 'comment', 'threads', 'via-host'], }); if (args.has('help') || args.has('h')) { say(PR_USAGE); return 0; } + // Before --repo: the point of the message is to name the command that does it now. + if (args.has('close') || args.value('close') !== undefined) { + fail('--close moved to: gcb gh pr close --repo owner/name'); + } const socketPath = resolveSocketPath(args.value('socket'), process.env); const host = args.value('host'); @@ -296,9 +304,7 @@ export async function runPr(argv: readonly string[]): Promise { const done = action === 'open' ? 'opened' - : action === 'close' - ? 'closed' - : action === 'merge' + : action === 'merge' ? 'merged' : action === 'comment' ? 'commented on' diff --git a/packages/client/src/index.ts b/packages/client/src/index.ts index ca8741dc..e82db86a 100644 --- a/packages/client/src/index.ts +++ b/packages/client/src/index.ts @@ -3,10 +3,27 @@ * * The host commands are not imported here; installing `gcb-client` never pulls `yaml`. */ +import { GH_ACTIONS } from 'gcb-cli'; import type { CommandBackend } from 'gcb-cli'; export { runHelper } from './helper.ts'; +/** + * The catalogued subcommands of one domain, for the help text. + * + * The dispatch below reads the same table, so this cannot list a subcommand that does not run and a + * new action shows up in the help without a second edit. + * + * @param domain - the command before the colon, e.g. `pr`. + * @returns the subcommands, joined for one usage line. + */ +function cataloguedSubcommands(domain: string): string { + return Object.values(GH_ACTIONS) + .filter((action) => action.command[0] === domain) + .map((action) => action.command[1]) + .join('|'); +} + export const CLIENT_HELP = `gcb-client — container-side git credential helper and broker client. Usage: gcb [options] @@ -15,7 +32,9 @@ Commands setup Configure this environment's git to use the broker probe Ask the broker what it would do for a repository, without pushing logs Read a workflow job's log through the broker - pr Use the broker's pull-request operations + pr Use the broker's pull-request lifecycle; these run as gh actions: + ${cataloguedSubcommands('pr')} + run Run a workflow run action: ${cataloguedSubcommands('run')} gh Run a catalogued GitHub action through the broker As a git credential helper @@ -37,8 +56,20 @@ async function run(argv: readonly string[]): Promise { return (await import('./commands/probe.ts')).runProbe(rest); case 'logs': return (await import('./commands/logs.ts')).runLogs(rest); - case 'pr': + case 'pr': { + // The catalog is the one source for an action, so the shorthand asks it rather than keeping a + // second list: a subcommand the catalog does not have stays with the flag-only lifecycle. + // Bare `pr` asks the catalog too, where `run` needs no branch: the flag-only path would only + // tell it that --repo is required. + 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/pr.ts')).runPr(rest); + } + case 'run': + return (await import('./commands/gh.ts')).runGh(['run', ...rest]); case 'gh': return (await import('./commands/gh.ts')).runGh(rest); default: @@ -49,7 +80,7 @@ async function run(argv: readonly string[]): Promise { export const clientBackend: CommandBackend = { role: 'client', - commands: ['setup', 'probe', 'logs', 'pr', 'gh'], + commands: ['setup', 'probe', 'logs', 'pr', 'run', 'gh'], help: CLIENT_HELP, run, }; diff --git a/packages/host/src/broker.ts b/packages/host/src/broker.ts index 2c2bc240..bd03b8bf 100644 --- a/packages/host/src/broker.ts +++ b/packages/host/src/broker.ts @@ -382,7 +382,6 @@ export function createRequestHandler( const action = message['action'] ?? 'open'; if ( action !== 'open' && - action !== 'close' && action !== 'merge' && action !== 'update' && action !== 'status' && diff --git a/packages/host/src/providers/github-app.ts b/packages/host/src/providers/github-app.ts index b485cd4b..f68305d8 100644 --- a/packages/host/src/providers/github-app.ts +++ b/packages/host/src/providers/github-app.ts @@ -1036,11 +1036,7 @@ function pullRequestNumber(value: unknown): number | undefined { let method = 'POST'; let url = collection; let body: Record = { title: request.title, body: request.body, head: request.head, base: request.base, draft: request.draft === true }; - if (request.action === 'close') { - method = 'PATCH'; - url = `${collection}/${request.number}`; - body = { state: 'closed' }; - } else if (request.action === 'update') { + if (request.action === 'update') { method = 'PATCH'; url = `${collection}/${request.number}`; body = { title: request.title, body: request.body, base: request.base }; diff --git a/test/unit/actions.test.ts b/test/unit/actions.test.ts index 123e3c61..c2367ad2 100644 --- a/test/unit/actions.test.ts +++ b/test/unit/actions.test.ts @@ -17,6 +17,7 @@ describe('gh action catalog', () => { 'pr:list', 'pr:diff', 'pr:create', + 'pr:close', 'pr:comment', 'pr:review', 'issue:create', @@ -25,6 +26,7 @@ describe('gh action catalog', () => { ]); assert.equal('pr:merge' in GH_ACTIONS, false); assert.equal(getGhAction('pr:create').command.join(' '), 'pr create'); + assert.equal(getGhAction('pr:close').command.join(' '), 'pr close'); }); it('keeps creation token permissions narrow', () => { diff --git a/test/unit/broker.test.ts b/test/unit/broker.test.ts index ab023738..6afba1ff 100644 --- a/test/unit/broker.test.ts +++ b/test/unit/broker.test.ts @@ -280,24 +280,16 @@ describe('broker request handling', () => { assert.match(unsupported.reason ?? '', /cannot manage pull requests/); }); - it('closes a pull request, and refuses a merge method it does not know', async () => { + it('refuses a merge method it does not know', async () => { const seen: string[] = []; const managed: Provider = { name: 'with-prs', getCredential: () => Promise.reject(new Error('not used here')), pullRequest: (request) => { seen.push(request.action); - return Promise.resolve({ number: request.number ?? 0, url: 'u', state: 'closed' }); + return Promise.resolve({ number: request.number ?? 0, url: 'u', state: 'open' }); }, }; - const { handle, events } = makeHarness(['acme/widget'], managed); - const response = await handle({ ...REQUEST, op: 'pull-request', action: 'close', number: 7 }); - - assert.equal(response.ok, true); - assert.deepEqual(seen, ['close']); - assert.equal(response.prState, 'closed'); - assert.equal(events.at(-1)?.['action'], 'close'); - const badMethod = await makeHarness(['acme/widget'], managed).handle({ ...REQUEST, op: 'pull-request', @@ -306,7 +298,7 @@ describe('broker request handling', () => { method: 'fast-forward', }); assert.equal(badMethod.code, CODES.BAD_REQUEST); - assert.deepEqual(seen, ['close'], 'and the provider was never asked to do it'); + assert.deepEqual(seen, [], 'the provider was never asked to do it'); }); it('reports a pull request without changing it', async () => { @@ -407,7 +399,7 @@ describe('broker request handling', () => { assert.equal(response.code, 'bad-request', label + ' has to be refused by the broker'); } - const wrongAction = await handle({ ...comments, action: 'close' }); + const wrongAction = await handle({ ...comments, action: 'merge' }); assert.equal(wrongAction.code, 'bad-request', 'and an anchor only belongs on a comment'); }); diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts index 6b9121f2..c6adda73 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -1112,7 +1112,7 @@ describe('the anchor flags, at the CLI boundary', () => { const home = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-pr-home-')); after(() => fs.rmSync(home, { recursive: true, force: true })); - const cli = (args: readonly string[]): { status: number; stderr: string } => { + const cli = (args: readonly string[]): { status: number; stdout: string; stderr: string } => { try { execFileSync('node', ['--conditions=development', 'packages/client/bin/gcb.js', 'pr', ...args], { cwd: path.resolve('.'), @@ -1123,10 +1123,14 @@ describe('the anchor flags, at the CLI boundary', () => { // reach a broker someone happens to have running. env: { ...process.env, HOME: home, GIT_BROKER_SOCKET: '', GIT_BROKER_SOCKET_FILE: '' }, }); - return { status: 0, stderr: '' }; + return { status: 0, stdout: '', stderr: '' }; } catch (error) { const failed = error as { status?: number | null; stderr?: string }; - return { status: failed.status ?? -1, stderr: String(failed.stderr ?? '') }; + return { + status: failed.status ?? -1, + stdout: String((failed as { stdout?: string }).stdout ?? ''), + stderr: String(failed.stderr ?? ''), + }; } }; @@ -1143,11 +1147,35 @@ describe('the anchor flags, at the CLI boundary', () => { }); it('refuses an anchor on an action that is not a comment', () => { - const closed = cli([...base, '--close', '--file', 'Dockerfile', '--line', '4', '--body', 'x']); + const closed = cli([...base, '--merge', '--file', 'Dockerfile', '--line', '4', '--body', 'x']); assert.equal(closed.status, 1); assert.match(closed.stderr, /only go with --comment/); }); + it('refuses --close with a value too', () => { + for (const argv of [['--close', '7'], ['--close=7']]) { + const moved = cli([...base, ...argv]); + assert.equal(moved.status, 1); + assert.match(moved.stderr, /gcb gh pr close/); + } + }); + + it('shows help for bare gcb pr', () => { + const result = cli([]); + assert.equal(result.status, 1); + assert.match(result.stdout, /Usage: gcb gh/); + }); + + it('points --close at the gh action', () => { + const moved = cli([...base, '--close']); + assert.equal(moved.status, 1); + assert.match(moved.stderr, /gcb gh pr close/); + + const bare = cli(['--close']); + assert.equal(bare.status, 1); + assert.match(bare.stderr, /gcb gh pr close/, 'before --repo, so the message names the replacement'); + }); + it('refuses a side that is neither, and a line that is not a line', () => { const side = cli([...base, '--comment', '--file', 'Dockerfile', '--line', '4', '--side', 'middle', '--body', 'x']); assert.match(side.stderr, /--side must be left or right/); diff --git a/test/unit/gh-command.test.ts b/test/unit/gh-command.test.ts index 651068b7..691f44da 100644 --- a/test/unit/gh-command.test.ts +++ b/test/unit/gh-command.test.ts @@ -14,14 +14,18 @@ import type { AuditSink, BrokerConfig, Provider, StaticHostConfig } from '../../ const CLIENT_BIN = fileURLToPath(new URL('../../packages/client/bin/gcb.js', import.meta.url)); /** Run the installed client as a child, so its output never replaces the test reporter. */ -function runGhCli(args: readonly string[], cwd?: string): Promise { +function runClient(args: readonly string[], cwd?: string): Promise { return tryRunAsync( process.execPath, - ['--conditions=development', CLIENT_BIN, 'gh', ...args], + ['--conditions=development', CLIENT_BIN, ...args], { cwd, env: process.env, timeoutMs: 5000 }, ); } +function runGhCli(args: readonly string[], cwd?: string): Promise { + return runClient(['gh', ...args], cwd); +} + describe('gcb gh', () => { it('reports truncation and timed-out writes', async () => { const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-gh-output-')); @@ -200,6 +204,42 @@ describe('gcb gh', () => { } }); + it('routes pr subcommands and run through the gh action', async () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-gh-shorthand-')); + 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() }; + const actions: string[] = []; + const provider: Provider = { + name: 'static', + getCredential: async () => ({ username: 'u', password: 'p', expiresAt: Date.now() + 1000 }), + runAction: async (request) => { + actions.push(request.action); + 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 pr = await runClient(['pr', 'view', '7', '--repo', 'acme/widget', '--socket', socketPath]); + assert.equal(pr.status, 0); + const close = await runClient(['pr', 'close', '7', '--repo', 'acme/widget', '--socket', socketPath]); + assert.equal(close.status, 0); + const run = await runClient(['run', 'list', '--repo', 'acme/widget', '--socket', socketPath]); + assert.equal(run.status, 0); + assert.deepEqual(actions, ['pr:view', 'pr:close', 'run:list']); + } finally { + await broker.close(); + fs.rmSync(root, { recursive: true, force: true }); + } + }); + 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'); @@ -232,3 +272,11 @@ describe('gcb gh', () => { } }); }); + +describe('bare run', () => { + it('prints the catalog help', async () => { + const result = await runClient(['run']); + assert.equal(result.status, 1); + assert.match(result.stdout, /Usage: gcb gh/); + }); +});