diff --git a/AGENTS.md b/AGENTS.md index 529996fc..cce0db6e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -197,10 +197,9 @@ sources must stay erasable (`erasableSyntaxOnly` is on: no enums, no parameter p The broker performs each action without the token leaving it, and prints the number, the URL and the state. - **The agent does not merge on its own, and could not.** The ruleset requires an approving review, - so a merge through this path is blocked exactly as a click would be — which is the point of saying - the gate is branch protection rather than this tool. `--merge` exists for when a human asks for it, - not for routine use. + **The agent does not merge on its own, and could not.** There is no merge action; the ruleset requires + an approving review, so a merge is a human action on GitHub. The point is that the gate is branch + protection rather than this tool. Worth keeping in the rule: the ability to open a pull request does not carry the ability to push to the branch it targets — the token is narrowed to `pull_requests: write`. CI runs on diff --git a/README.md b/README.md index ea955bd9..b016b56b 100644 --- a/README.md +++ b/README.md @@ -654,6 +654,7 @@ actions. Grant them on the App, then approve any changed permissions on each ins | `pr list` | `pull_requests: read` | | `pr diff` | `pull_requests: read`, `contents: read` | | `pr create` | `pull_requests: write`, `contents: read` | +| `pr edit` | `pull_requests: write` | | `pr close` | `pull_requests: write` | | `pr comment` | `pull_requests: write` | | `pr review` | `pull_requests: write` | diff --git a/packages/cli/src/actions.ts b/packages/cli/src/actions.ts index bc8b9491..6c8f1841 100644 --- a/packages/cli/src/actions.ts +++ b/packages/cli/src/actions.ts @@ -141,6 +141,21 @@ const PR_CREATE: GhAction = { hostUser: true, }; +// GitHub: PATCH /repos/{owner}/{repo}/pulls/{pull_number} +const PR_EDIT: GhAction = { + name: 'pr:edit', + command: ['pr', 'edit'], + positionals: [{ name: 'number', type: 'number', required: true }], + flags: [ + { name: 'title', type: 'string' }, + { name: 'body', type: 'string' }, + { name: 'base', type: 'string' }, + ], + permissions: { pull_requests: 'write' }, + timeoutSeconds: 120, + hostUser: true, +}; + // GitHub: POST /repos/{owner}/{repo}/issues/{issue_number}/comments const PR_COMMENT: GhAction = { name: 'pr:comment', @@ -237,6 +252,7 @@ export const GH_ACTIONS: Readonly> = Object.freeze({ 'pr:list': PR_LIST, 'pr:diff': PR_DIFF, 'pr:create': PR_CREATE, + 'pr:edit': PR_EDIT, 'pr:close': PR_CLOSE, 'pr:comment': PR_COMMENT, 'pr:review': PR_REVIEW, diff --git a/packages/cli/src/support.ts b/packages/cli/src/support.ts index 1af9c626..1c775966 100644 --- a/packages/cli/src/support.ts +++ b/packages/cli/src/support.ts @@ -19,6 +19,8 @@ export interface Args { value(name: string): string | undefined; /** Whether a boolean flag was present. */ has(name: string): boolean; + /** Every option name seen, after the parser's normalization. */ + readonly names: readonly string[]; /** Arguments that were not flags. */ readonly positionals: readonly string[]; } @@ -82,6 +84,7 @@ export function parseArgs(argv: readonly string[], options: ParseArgsOptions = { values: (name) => collected.get(name) ?? [], value: (name) => collected.get(name)?.[0], has: (name) => flags.has(name), + names: [...new Set([...collected.keys(), ...flags])], positionals, }; } diff --git a/packages/cli/src/types.ts b/packages/cli/src/types.ts index 5d954347..db32bf97 100644 --- a/packages/cli/src/types.ts +++ b/packages/cli/src/types.ts @@ -106,8 +106,6 @@ export interface JobLogRequest { /** What can be done to a pull request. */ export type PullRequestAction = | 'open' - | 'merge' - | 'update' | 'status' | 'comment' | 'threads' @@ -115,9 +113,6 @@ export type PullRequestAction = | 'edit' | 'resolve'; -/** How a merge should be recorded. */ -export type MergeMethod = 'merge' | 'squash' | 'rebase'; - /** What a provider is asked to do to a pull request. */ export interface PullRequestRequest { readonly host: string; @@ -145,7 +140,6 @@ export interface PullRequestRequest { /** Which side of the diff the line counts on. The new file, unless told otherwise. */ readonly side?: 'left' | 'right'; readonly draft?: boolean; - readonly method?: MergeMethod; } /** One pull request, as GitHub describes it after an action. */ @@ -280,7 +274,6 @@ export interface WireResponse { readonly prUrl?: string; readonly prNumber?: number; readonly prState?: string; - readonly prMerged?: boolean; /** The report, for `action: "status"`. */ readonly prStatus?: string; /** For `op: "action"`. */ diff --git a/packages/client/src/commands/pr.ts b/packages/client/src/commands/pr.ts index 1b572200..ec3d1956 100644 --- a/packages/client/src/commands/pr.ts +++ b/packages/client/src/commands/pr.ts @@ -1,5 +1,5 @@ /** - * `pr` — open, merge or update one pull request through the broker. + * `pr` — open, update or inspect 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. @@ -23,16 +23,24 @@ import { writeRequest, } from 'gcb-cli'; import { requestOverSocket, resolveSocketPath } from '../helper.ts'; -import { baseBranchFor, qualifiedBranch } from './remotes.ts'; +import { baseBranchFor, qualifiedBranch, remoteRepo } from './remotes.ts'; +import { runGh } from './gh.ts'; import { fail, parseArgs, say } from 'gcb-cli'; +/** Every flag the flag-only lifecycle understands. */ +const KNOWN_FLAGS = new Set([ + 'help', 'h', 'draft', 'close', 'comment', 'file', 'line', 'side', 'threads', + 'reply-to', 'edit', 'resolve', 'proxy', 'no-proxy', 'socket', 'host', 'repo', + 'head', 'base', 'title', 'body', 'body-file', 'number', 'via-host', 'status', +]); + /** Longer than a credential request: a token is minted, then the action is performed. */ const PR_TIMEOUT_MS = 60_000; /** Usage text for `pr`. */ export const PR_USAGE = `Usage: gcb pr [options] -Open, merge, update or inspect a pull request through the broker, without the credential +Open, 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; @@ -65,14 +73,12 @@ 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) - --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. Common --socket Broker socket (default: $GIT_BROKER_SOCKET or the socket file) - --host Host as configured, e.g. github.com [required] - --repo Repository the pull request belongs to [required] + --host Host as configured, e.g. github.com (default: github.com) + --repo Repository the pull request belongs to (default: origin remote) -h, --help Show this help `; @@ -81,7 +87,7 @@ Common * Read the action out of the flags. * * @param args - the parsed arguments. - * @returns `merge`, `update` or `open`. + * @returns `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 @@ -95,7 +101,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('merge')) return 'merge' as const; if (args.value('number') !== undefined) return 'update' as const; return 'open' as const; } @@ -108,12 +113,18 @@ 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', 'merge', 'status', 'comment', 'threads', 'via-host', 'no-proxy'], + booleans: ['help', 'draft', 'status', 'comment', 'threads', 'via-host', 'no-proxy'], }); if (args.has('help') || args.has('h')) { say(PR_USAGE); return 0; } + // Unknown options are refused rather than ignored: a typo must not silently fall through to an update. + for (const name of args.names) { + if (!KNOWN_FLAGS.has(name)) fail('unknown option --' + name + ' for gcb pr'); + } + if (args.positionals.length > 0) fail('unexpected argument ' + JSON.stringify(args.positionals[0]) + ' for gcb pr'); + // 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'); @@ -130,19 +141,31 @@ export async function runPr(argv: readonly string[]): Promise { if (args.has('proxy') && args.value('proxy') === undefined) fail('--proxy needs a value'); const proxy = args.has('no-proxy') ? null : args.value('proxy'); const socketPath = resolveSocketPath(args.value('socket'), process.env); - const host = args.value('host'); - const repo = args.value('repo') ?? fail('--repo is required (e.g. owner/name)'); + const looksLikeOption = (value: string): boolean => + value.startsWith('--') || /^-(?=[A-Za-z])/.test(value); + const invalidValue = (name: string): boolean => + args.has(name) || args.values(name).some((value) => value.trim() === '' || looksLikeOption(value)); + const hasValue = (name: string): boolean => args.has(name) || args.values(name).length > 0; + const hostArg = args.value('host'); + if (hasValue('host') && invalidValue('host')) fail('--host needs a value (e.g. github.com)'); + const host = hostArg ?? 'github.com'; + const repoArg = args.value('repo'); + if (hasValue('repo') && invalidValue('repo')) fail('--repo needs a value (e.g. owner/name)'); + let repo = repoArg; + if (repo === undefined) { + const found = remoteRepo('origin', process.cwd()); + if (found === undefined) fail('--repo is required when the checkout has no usable origin remote'); + if (found.host !== host.toLowerCase()) { + fail(`the checkout's remote is on ${found.host}, but --host is ${host}; pass --repo or --host`); + } + repo = `${found.owner}/${found.repo}`; + } + if (repo.trim() === '') fail('--repo needs a value (e.g. owner/name)'); const action = actionOf(args); if (args.has('via-host') && action !== 'open') { fail('--via-host only creates a pull request, so it does not go with ' + JSON.stringify(action)); } - // `--method` takes a value, so `has` — which is about boolean flags — is false for it however it is given. - if (args.value('method') !== undefined && action !== 'merge') { - fail('--method chooses how to merge, so it only goes with --merge'); - } const viaHost = action === 'open' && args.has('via-host'); - if (!host && !viaHost) fail('--host is required (e.g. github.com)'); - if (!repo) fail('--repo is required (e.g. owner/name), also with --via-host: the opener passes it on'); const number = args.value('number'); // `--head origin:feat/x` and `--base upstream:main` name a configured remote instead of a repository @@ -226,6 +249,18 @@ export async function runPr(argv: readonly string[]): Promise { fail(`--resolve needs the id of the thread being resolved, got ${JSON.stringify(resolveId)}`); } + if (action === 'update') { + const ghArgs = ['pr', 'edit', String(number), '--repo', repo, '--host', host]; + if (title !== undefined) ghArgs.push(`--title=${title}`); + if (body !== undefined) ghArgs.push(`--body=${body}`); + if (base !== undefined) ghArgs.push(`--base=${base}`); + const socketArg = args.value('socket'); + if (socketArg !== undefined) ghArgs.push('--socket', socketArg); + if (args.has('no-proxy')) ghArgs.push('--no-proxy'); + else if (args.value('proxy') !== undefined) ghArgs.push('--proxy', args.value('proxy') as string); + return runGh(ghArgs, { unreachableExitCode: 3 }); + } + if (viaHost) { if (head === undefined || title === undefined) fail('--via-host needs --head and --title'); const gitDir = findGitDir(process.cwd()); @@ -293,7 +328,6 @@ export async function runPr(argv: readonly string[]): Promise { ...(title === undefined ? {} : { title }), ...(body === undefined ? {} : { body }), ...(filePath === undefined ? {} : { filePath, line: Number(lineArg), side }), - ...(action === 'merge' ? { method: args.value('method') ?? 'squash' } : {}), draft: args.has('draft'), session: 'pr', pid: process.pid, @@ -319,9 +353,7 @@ export async function runPr(argv: readonly string[]): Promise { const done = action === 'open' ? 'opened' - : action === 'merge' - ? 'merged' - : action === 'comment' + : action === 'comment' ? 'commented on' : action === 'reply' ? 'replied in' diff --git a/packages/host/src/broker.ts b/packages/host/src/broker.ts index 45b776bd..c4e614ae 100644 --- a/packages/host/src/broker.ts +++ b/packages/host/src/broker.ts @@ -38,7 +38,7 @@ import net from 'node:net'; import path from 'node:path'; import { hostConfig, normalizeRepoPath, repoAllowed } from './policy.ts'; -import type { ActionValues, MergeMethod, AuditSink, BrokerConfig, Provider, WireResponse } from 'gcb-cli'; +import type { ActionValues, AuditSink, BrokerConfig, Provider, WireResponse } from 'gcb-cli'; /** Reported by the `ping` operation so the container side can prove what it reached. */ export const BROKER_VERSION = '0.1.0'; @@ -386,8 +386,6 @@ export function createRequestHandler( const action = message['action'] ?? 'open'; if ( action !== 'open' && - action !== 'merge' && - action !== 'update' && action !== 'status' && action !== 'comment' && action !== 'threads' && @@ -407,7 +405,6 @@ export function createRequestHandler( const filePath = message['filePath']; const line = message['line']; const side = message['side'] ?? 'right'; - const method = message['method'] ?? 'squash'; const branch = BRANCH; const usable = (value: unknown): value is string => typeof value === 'string' && branch.test(value) && !value.includes('..'); @@ -415,7 +412,7 @@ export function createRequestHandler( if (action === 'open' && !usable(head)) { return deny(context, CODES.BAD_REQUEST, `bad head ${JSON.stringify(head)}`, 'the head branch name is not usable'); } - if ((action === 'open' || action === 'update') && base !== undefined && !usable(base)) { + if (action === 'open' && base !== undefined && !usable(base)) { return deny(context, CODES.BAD_REQUEST, `bad base ${JSON.stringify(base)}`, 'the base branch name is not usable'); } if (action === 'open' && (typeof title !== 'string' || title.trim() === '' || title.length > MAX_TITLE_CHARS)) { @@ -450,9 +447,6 @@ export function createRequestHandler( if (side !== 'left' && side !== 'right') { return deny(context, CODES.BAD_REQUEST, `bad side ${JSON.stringify(side)}`, 'the side has to be left or right'); } - if (action === 'update' && title === undefined && body === undefined && base === undefined) { - return deny(context, CODES.BAD_REQUEST, 'empty update', 'an update has to change a title, a body or a base'); - } if ((action === 'comment' || action === 'reply' || action === 'edit') && (typeof body !== 'string' || body.trim() === '')) { return deny(context, CODES.BAD_REQUEST, 'empty comment', 'a comment has to say something'); } @@ -470,9 +464,6 @@ export function createRequestHandler( if (action !== 'open' && action !== 'resolve' && action !== 'edit' && (typeof number !== 'number' || !Number.isInteger(number) || number <= 0)) { return deny(context, CODES.BAD_REQUEST, `bad number ${JSON.stringify(number)}`, 'this action needs a pull request number'); } - if (action === 'merge' && method !== 'merge' && method !== 'squash' && method !== 'rebase') { - return deny(context, CODES.BAD_REQUEST, `bad method ${JSON.stringify(method)}`, 'the merge method must be merge, squash or rebase'); - } if (typeof provider.pullRequest !== 'function') { return deny( context, @@ -498,7 +489,6 @@ export function createRequestHandler( ...(typeof base === 'string' ? { base } : {}), ...(typeof title === 'string' ? { title } : {}), ...(typeof body === 'string' ? { body } : {}), - ...(action === 'merge' ? { method: method as MergeMethod } : {}), draft: message['draft'] === true, }); audit.record({ @@ -520,7 +510,6 @@ export function createRequestHandler( prNumber: result.number, prUrl: result.url, prState: result.state, - prMerged: result.merged === true, prStatus: result.status, }; } catch (error) { diff --git a/packages/host/src/providers/github-app.ts b/packages/host/src/providers/github-app.ts index 4fcb75a2..49c56861 100644 --- a/packages/host/src/providers/github-app.ts +++ b/packages/host/src/providers/github-app.ts @@ -423,32 +423,6 @@ export function createGithubAppProvider(options: GithubAppProviderOptions): Prov return pending; } - /** - * Read a pull request that another call has already fetched. - * - * @param text - the JSON body. - * @param fallbackNumber - the number the caller asked about. - * @returns the number, URL and state. - */ - function readPullRequest(text: string): PullRequest { - let parsed: unknown = null; - try { - parsed = JSON.parse(text); - } catch { - parsed = null; - } - const payload = (parsed ?? {}) as { number?: unknown; html_url?: unknown; state?: unknown; merged?: unknown }; - if (typeof payload.number !== 'number' || typeof payload.html_url !== 'string') { - throw new ProviderConfigError('GitHub did not answer with a pull request'); - } - return { - number: payload.number, - url: payload.html_url, - state: typeof payload.state === 'string' ? payload.state : undefined, - merged: payload.merged === true, - }; - } - /** * Report what a pull request is, and how its commit is doing. * @@ -1045,23 +1019,11 @@ function pullRequestNumber(value: unknown): number | undefined { if (request.action === 'status') { return readPullRequestStatus(request, token, collection); } - 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 === 'update') { - method = 'PATCH'; - url = `${collection}/${request.number}`; - body = { title: request.title, body: request.body, base: request.base }; - for (const [key, value] of Object.entries(body)) { - if (value === undefined) delete body[key]; - } - } else if (request.action === 'merge') { - method = 'PUT'; - url = `${collection}/${request.number}/merge`; - body = { merge_method: request.method ?? 'squash' }; - } - let text: string; + const method = 'POST'; + const url = collection; + const body: Record = { title: request.title, body: request.body, head: request.head, base: request.base, draft: request.draft === true }; + try { // as theirs, which is what automated reviewers recognise. The token itself is as wide as that person — // it carries the app permissions and could push and merge — and what is narrow is this code: it is @@ -1082,17 +1044,6 @@ function pullRequestNumber(value: unknown): number | undefined { throw wrapped; } - if (request.action === 'merge') { - // The merge answers with a sha and a message, not the pull request, so read it back for the - // canonical URL — and to report whether GitHub actually merged it. - const merged = JSON.parse(text) as { merged?: unknown }; - if (merged.merged !== true) { - throw new ProviderConfigError('GitHub did not merge the pull request'); - } - const { text: after } = await callWithToken('GET', `${collection}/${request.number}`, token); - return readPullRequest(after); - } - let parsed: unknown = null; try { parsed = JSON.parse(text); diff --git a/test/unit/actions.test.ts b/test/unit/actions.test.ts index c2367ad2..305272d5 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:edit', 'pr:close', 'pr:comment', 'pr:review', @@ -93,6 +94,13 @@ describe('buildGhArgv', () => { ]); }); + it('builds the edit flags', () => { + assert.deepEqual( + buildGhArgv('pr:edit', { number: 7, title: 't', base: 'main' }, 'acme/widget'), + ['pr', 'edit', '7', '--title', 't', '--base', 'main', '--repo', 'acme/widget'], + ); + }); + it('accepts numeric flag values without stringifying them first', () => { assert.deepEqual(buildGhArgv('pr:list', { limit: 10 }, 'acme/widget'), [ 'pr', diff --git a/test/unit/broker.test.ts b/test/unit/broker.test.ts index 6afba1ff..90f0c192 100644 --- a/test/unit/broker.test.ts +++ b/test/unit/broker.test.ts @@ -280,27 +280,6 @@ describe('broker request handling', () => { assert.match(unsupported.reason ?? '', /cannot manage pull requests/); }); - 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: 'open' }); - }, - }; - const badMethod = await makeHarness(['acme/widget'], managed).handle({ - ...REQUEST, - op: 'pull-request', - action: 'merge', - number: 7, - method: 'fast-forward', - }); - assert.equal(badMethod.code, CODES.BAD_REQUEST); - assert.deepEqual(seen, [], 'the provider was never asked to do it'); - }); - it('reports a pull request without changing it', async () => { const asked: string[] = []; const reader: Provider = { @@ -399,7 +378,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: 'merge' }); + const wrongAction = await handle({ ...comments, action: 'status' }); 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 dfd2f8d8..a46103d5 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -8,6 +8,7 @@ import assert from 'node:assert/strict'; import { execFileSync } from 'node:child_process'; import { generateKeyPairSync } from 'node:crypto'; +import net from 'node:net'; import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; @@ -22,6 +23,8 @@ import { parseArgs, listFlag, insideMountedPath, mountedPaths } from '../../pack import { performSetup } from '../../packages/client/src/commands/setup.ts'; import type { BrokerConfig, GithubAppHostConfig } from '../../packages/cli/src/types.ts'; import { runProbe } from '../../packages/client/src/commands/probe.ts'; +import { encodeMessage, parseMessage } from '../../packages/cli/src/socket-protocol.ts'; +import { runAsync } from '../common/run.ts'; /** Scratch space for the whole file. */ const scratch = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-commands-')); @@ -1151,7 +1154,7 @@ describe('the anchor flags, at the CLI boundary', () => { }); it('refuses an anchor on an action that is not a comment', () => { - const closed = cli([...base, '--merge', '--file', 'Dockerfile', '--line', '4', '--body', 'x']); + const closed = cli([...base, '--status', '--file', 'Dockerfile', '--line', '4', '--body', 'x']); assert.equal(closed.status, 1); assert.match(closed.stderr, /only go with --comment/); }); @@ -1170,6 +1173,43 @@ describe('the anchor flags, at the CLI boundary', () => { assert.match(result.stdout, /Usage: gcb gh/); }); + it('refuses unknown options, including removed merge flags', () => { + for (const flag of [['--merge'], ['-merge'], ['--merge-xxx'], ['--method', 'squash'], ['--method-xxx', 'squash'], ['--xafajbsfbafabsjkf']]) { + const result = cli([...base, ...flag, '--title', 'a title']); + assert.equal(result.status, 1); + assert.match(result.stderr, /unknown option/); + } + + const value = cli([...base, '--title=--not-a-flag']); + assert.doesNotMatch(value.stderr, /unknown option/, 'a value is not an option'); + }); + + it('refuses a valueless or option-looking repo', () => { + for (const argv of [['--repo', '--number', '7', '--status'], ['--repo', '-h', '--number', '7', '--status'], ['--repo=--help', '--number', '7', '--status'], ['--repo', 'acme/widget', '--repo', '-h', '--number', '7', '--status']]) { + const result = cli(argv); + assert.equal(result.status, 1); + assert.match(result.stderr, /--repo needs a value/); + } + }); + + it('refuses an option-looking host', () => { + for (const argv of [ + ['--host', '-h', '--repo', 'acme/widget', '--number', '7', '--status'], + ['--host=--help', '--repo', 'acme/widget', '--number', '7', '--status'], + ['--host', 'github.com', '--host', '-h', '--repo', 'acme/widget', '--number', '7', '--status'], + ]) { + const result = cli(argv); + assert.equal(result.status, 1); + assert.match(result.stderr, /--host needs a value/); + } + }); + + it('refuses unexpected positionals', () => { + const result = cli(['--repo', 'acme/widget', '--number', '7', '--status', 'stray']); + assert.equal(result.status, 1); + assert.match(result.stderr, /unexpected argument "stray"/); + }); + it('refuses a plain --comment and names the anchor requirement', () => { const moved = cli([...base, '--comment', '--body', 'x']); assert.equal(moved.status, 1); @@ -1201,11 +1241,6 @@ describe('the anchor flags, at the CLI boundary', () => { } }); - it('refuses --method unless the action merges', () => { - const updated = cli([...base, '--method', 'squash', '--title', 'a title']); - assert.equal(updated.status, 1); - assert.match(updated.stderr, /only goes with --merge/); - }); it('refuses an edit with nothing to say, or with no comment to change', () => { const noBody = cli([...base, '--edit', '99']); @@ -1246,6 +1281,157 @@ describe('the anchor flags, at the CLI boundary', () => { assert.match(noNumber.stderr, /no broker socket/, 'it reached socket resolution'); assert.doesNotMatch(noNumber.stderr, /--number is required/, 'without being asked for a number'); }); + + it('defaults the host to github.com', () => { + const result = cli(['--repo', 'acme/widget', '--number', '7', '--status']); + assert.equal(result.status, 1); + assert.match(result.stderr, /no broker socket/, 'validation got past the host'); + assert.doesNotMatch(result.stderr, /--host is required/); + }); + + it('refuses an explicitly empty host', () => { + const result = cli(['--host', '', '--repo', 'acme/widget', '--number', '7', '--status']); + assert.equal(result.status, 1); + assert.match(result.stderr, /--host needs a value/); + }); + + it('refuses a valueless host', () => { + const result = cli(['--host', '--repo', 'acme/widget', '--number', '7', '--status']); + assert.equal(result.status, 1); + assert.match(result.stderr, /--host needs a value/); + }); + + it('resolves the repository from origin', async () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-pr-origin-')); + execFileSync('git', ['init', '--quiet'], { cwd: root }); + execFileSync('git', ['remote', 'add', 'origin', 'https://github.com/acme/widget.git'], { cwd: root }); + const socketPath = path.join(root, 'broker.sock'); + let request: Record | undefined; + const server = net.createServer((socket) => { + let buffer = ''; + socket.on('data', (chunk: Buffer) => { + buffer += chunk.toString('utf8'); + const newline = buffer.indexOf('\n'); + if (newline < 0) return; + request = parseMessage(buffer.slice(0, newline)) ?? undefined; + socket.end(encodeMessage({ ok: true, prStatus: 'open', prNumber: 7, prUrl: 'https://example.test/7' })); + }); + }); + await new Promise((resolve) => server.listen(socketPath, resolve)); + try { + const result = await runAsync(process.execPath, [ + '--conditions=gcb-development', + path.resolve('packages/client/bin/gcb.js'), + 'pr', '--number', '7', '--status', '--socket', socketPath, + ], { + cwd: root, + env: { ...process.env, HOME: home, GIT_BROKER_SOCKET: '', GIT_BROKER_SOCKET_FILE: '' }, + }); + assert.equal(result.status, 0); + assert.equal(request?.['path'], 'acme/widget'); + } finally { + await new Promise((resolve, reject) => server.close((error) => error ? reject(error) : resolve())); + fs.rmSync(root, { recursive: true, force: true }); + } + }); + + it('routes an update to gh pr edit', async () => { + const socketPath = path.join(scratch, 'pr-update.sock'); + let request: Record | undefined; + const server = net.createServer((socket) => { + let buffer = ''; + socket.on('data', (chunk: Buffer) => { + buffer += chunk.toString('utf8'); + const newline = buffer.indexOf('\n'); + if (newline < 0) return; + request = parseMessage(buffer.slice(0, newline)) ?? undefined; + socket.end(encodeMessage({ ok: true, stdout: 'updated\n', exitCode: 0 })); + }); + }); + await new Promise((resolve) => server.listen(socketPath, resolve)); + try { + const result = await runAsync(process.execPath, [ + '--conditions=gcb-development', + 'packages/client/bin/gcb.js', + 'pr', '--number', '7', '--title', 'a title', '--repo', 'acme/widget', '--socket', socketPath, + ], { + cwd: path.resolve('.'), + env: { ...process.env, HOME: home, GIT_BROKER_SOCKET: '', GIT_BROKER_SOCKET_FILE: '' }, + }); + assert.equal(result.status, 0); + assert.equal(request?.['op'], 'action'); + assert.equal(request?.['ghAction'], 'pr:edit'); + assert.equal(request?.['path'], 'acme/widget'); + } finally { + await new Promise((resolve, reject) => server.close((error) => error ? reject(error) : resolve())); + } + }); + + it('passes a front-matter body to gh pr edit', async () => { + const bodyFile = path.join(scratch, 'front-matter.md'); + fs.writeFileSync(bodyFile, '---\ntitle: x\n---\n\nbody\n'); + const socketPath = path.join(scratch, 'pr-body.sock'); + let request: Record | undefined; + const server = net.createServer((socket) => { + let buffer = ''; + socket.on('data', (chunk: Buffer) => { + buffer += chunk.toString('utf8'); + const newline = buffer.indexOf('\n'); + if (newline < 0) return; + request = parseMessage(buffer.slice(0, newline)) ?? undefined; + socket.end(encodeMessage({ ok: true, stdout: 'updated\n', exitCode: 0 })); + }); + }); + await new Promise((resolve) => server.listen(socketPath, resolve)); + try { + const result = await runAsync(process.execPath, [ + '--conditions=gcb-development', + 'packages/client/bin/gcb.js', + 'pr', '--number', '7', '--body-file', bodyFile, '--repo', 'acme/widget', '--socket', socketPath, + ], { + cwd: path.resolve('.'), + env: { ...process.env, HOME: home, GIT_BROKER_SOCKET: '', GIT_BROKER_SOCKET_FILE: '' }, + }); + assert.equal(result.status, 0); + assert.equal((request?.['values'] as Record | undefined)?.['body'], '---\ntitle: x\n---\n\nbody\n'); + } finally { + await new Promise((resolve, reject) => server.close((error) => error ? reject(error) : resolve())); + } + }); + + it('sends the default host to the broker', async () => { + const socketPath = path.join(scratch, 'pr-host-default.sock'); + let request: Record | undefined; + const server = net.createServer((socket) => { + let buffer = ''; + socket.on('data', (chunk: Buffer) => { + buffer += chunk.toString('utf8'); + const newline = buffer.indexOf('\n'); + if (newline < 0) return; + request = parseMessage(buffer.slice(0, newline)) ?? undefined; + socket.end(encodeMessage({ ok: true, prStatus: 'open', prNumber: 7, prUrl: 'https://example.test/7' })); + }); + }); + await new Promise((resolve) => server.listen(socketPath, resolve)); + try { + const result = await runAsync(process.execPath, [ + '--conditions=gcb-development', + 'packages/client/bin/gcb.js', + 'pr', + '--repo', 'acme/widget', + '--number', '7', + '--status', + '--socket', socketPath, + ], { + cwd: path.resolve('.'), + env: { ...process.env, HOME: home, GIT_BROKER_SOCKET: '', GIT_BROKER_SOCKET_FILE: '' }, + }); + assert.equal(result.status, 0); + assert.equal(request?.['host'], 'github.com'); + } finally { + await new Promise((resolve, reject) => server.close((error) => error ? reject(error) : resolve())); + } + }); }); describe('the help path', () => { diff --git a/test/unit/github-app.test.ts b/test/unit/github-app.test.ts index 511aa8c1..a79357dc 100644 --- a/test/unit/github-app.test.ts +++ b/test/unit/github-app.test.ts @@ -695,29 +695,6 @@ describe('createGithubAppProvider', () => { assert.equal(posted?.body?.['draft'], false); }); - it('merges a pull request with the method it was told to use', async () => { - const stub = createStubFetch({ - installations: [{ id: 42, account: { login: 'acme' } }], - nowMs, - appPermissions: { pull_requests: 'write', contents: 'read' }, - installationPermissions: { pull_requests: 'write', contents: 'read' }, - }); - const pr = await providerFor(stub).pullRequest!({ - action: 'merge', - host: 'github.com', - owner: 'acme', - repo: 'widget', - number: 42, - method: 'rebase', - }); - - const put = stub.calls.find((call) => call.method === 'PUT'); - assert.equal(put?.url, 'https://api.github.com/repos/acme/widget/pulls/42/merge'); - assert.deepEqual(put?.body, { merge_method: 'rebase' }); - assert.equal(pr.url, 'https://github.com/acme/widget/pull/42'); - assert.equal(pr.merged, true, 'and the state comes from reading the pull request back'); - }); - it('reports a pull request, and how its commit is doing', async () => { const stub = createStubFetch({ installations: [{ id: 42, account: { login: 'acme' } }],