From fedcd75bcc8d6b5bb850401f583c8e44e61f689f Mon Sep 17 00:00:00 2001 From: JounQin Date: Wed, 28 Oct 2026 13:58:58 +0000 Subject: [PATCH 01/13] fix: default the pr host to github.com The flag-only lifecycle command required --host while gcb gh already defaulted; github.com is the default everywhere. --- packages/client/src/commands/pr.ts | 5 ++--- test/unit/commands.test.ts | 7 +++++++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/packages/client/src/commands/pr.ts b/packages/client/src/commands/pr.ts index 1b572200..ac59d0f2 100644 --- a/packages/client/src/commands/pr.ts +++ b/packages/client/src/commands/pr.ts @@ -71,7 +71,7 @@ Acting on one Common --socket Broker socket (default: $GIT_BROKER_SOCKET or the socket file) - --host Host as configured, e.g. github.com [required] + --host Host as configured, e.g. github.com (default: github.com) --repo Repository the pull request belongs to [required] -h, --help Show this help `; @@ -130,7 +130,7 @@ 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 host = args.value('host') ?? 'github.com'; const repo = args.value('repo') ?? fail('--repo is required (e.g. owner/name)'); const action = actionOf(args); if (args.has('via-host') && action !== 'open') { @@ -141,7 +141,6 @@ export async function runPr(argv: readonly string[]): Promise { 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'); diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts index dfd2f8d8..01d48bb9 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -1246,6 +1246,13 @@ 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/); + }); }); describe('the help path', () => { From b1116129754a0a5b3417ec0579463f3a7ac52629 Mon Sep 17 00:00:00 2001 From: JounQin Date: Wed, 28 Oct 2026 22:17:41 +0800 Subject: [PATCH 02/13] fix: reject an empty pr host --- packages/client/src/commands/pr.ts | 6 ++++- test/unit/commands.test.ts | 43 ++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/packages/client/src/commands/pr.ts b/packages/client/src/commands/pr.ts index ac59d0f2..338109b7 100644 --- a/packages/client/src/commands/pr.ts +++ b/packages/client/src/commands/pr.ts @@ -130,7 +130,11 @@ 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') ?? 'github.com'; + const hostArg = args.value('host'); + if ((args.has('host') || hostArg !== undefined) && (hostArg === undefined || hostArg.trim() === '')) { + fail('--host needs a value (e.g. github.com)'); + } + const host = hostArg ?? 'github.com'; const repo = args.value('repo') ?? fail('--repo is required (e.g. owner/name)'); const action = actionOf(args); if (args.has('via-host') && action !== 'open') { diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts index 01d48bb9..21f21904 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-')); @@ -1253,6 +1256,46 @@ describe('the anchor flags, at the CLI boundary', () => { 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('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', () => { From 7595bf0f7abdf4cbd777bd291ebc6c99bc13c295 Mon Sep 17 00:00:00 2001 From: JounQin Date: Wed, 28 Oct 2026 14:25:23 +0000 Subject: [PATCH 03/13] test: cover a valueless pr host --- test/unit/commands.test.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts index 21f21904..0c10f62c 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -1263,6 +1263,12 @@ describe('the anchor flags, at the CLI boundary', () => { 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('sends the default host to the broker', async () => { const socketPath = path.join(scratch, 'pr-host-default.sock'); let request: Record | undefined; From 20b94f1225c75990ea15f759c22e6295b332b9fb Mon Sep 17 00:00:00 2001 From: JounQin Date: Wed, 28 Oct 2026 15:12:14 +0000 Subject: [PATCH 04/13] refactor: remove the pr merge action Merge is not an agent action; remove the custom flag, broker/provider path and tests instead of keeping a disabled capability. --- AGENTS.md | 7 ++-- packages/cli/src/types.ts | 6 ---- packages/client/src/commands/pr.ts | 20 +++-------- packages/host/src/broker.ts | 9 +---- packages/host/src/providers/github-app.ts | 41 ----------------------- test/unit/broker.test.ts | 21 ------------ test/unit/commands.test.ts | 7 +--- test/unit/github-app.test.ts | 23 ------------- 8 files changed, 10 insertions(+), 124 deletions(-) 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/packages/cli/src/types.ts b/packages/cli/src/types.ts index 5d954347..309c4e7f 100644 --- a/packages/cli/src/types.ts +++ b/packages/cli/src/types.ts @@ -106,7 +106,6 @@ export interface JobLogRequest { /** What can be done to a pull request. */ export type PullRequestAction = | 'open' - | 'merge' | 'update' | 'status' | 'comment' @@ -115,9 +114,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 +141,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 +275,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 338109b7..6fb3201c 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. @@ -32,7 +32,7 @@ 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,8 +65,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) - --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 @@ -81,7 +79,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 +93,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,7 +105,7 @@ 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); @@ -140,10 +137,6 @@ export async function runPr(argv: readonly string[]): Promise { 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 (!repo) fail('--repo is required (e.g. owner/name), also with --via-host: the opener passes it on'); @@ -296,7 +289,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, @@ -322,9 +314,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..11cd035d 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,7 +386,6 @@ export function createRequestHandler( const action = message['action'] ?? 'open'; if ( action !== 'open' && - action !== 'merge' && action !== 'update' && action !== 'status' && action !== 'comment' && @@ -407,7 +406,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('..'); @@ -470,9 +468,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 +493,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 +514,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..9548b6d1 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. * @@ -1055,10 +1029,6 @@ function pullRequestNumber(value: unknown): number | undefined { 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; @@ -1082,17 +1052,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/broker.test.ts b/test/unit/broker.test.ts index 6afba1ff..4f81c328 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 = { diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts index 0c10f62c..a56ed983 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -1154,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/); }); @@ -1204,11 +1204,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']); 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' } }], From 2775babed2d98a7627fbfbf1ed00f23a218f66bc Mon Sep 17 00:00:00 2001 From: JounQin Date: Wed, 28 Oct 2026 15:18:13 +0000 Subject: [PATCH 05/13] feat: resolve the pr repository from origin Match gcb gh: an omitted --repo uses origin when its host matches --host; an explicit --repo still wins. --- packages/client/src/commands/pr.ts | 15 +++++++++---- test/unit/commands.test.ts | 34 ++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/packages/client/src/commands/pr.ts b/packages/client/src/commands/pr.ts index 6fb3201c..122c9983 100644 --- a/packages/client/src/commands/pr.ts +++ b/packages/client/src/commands/pr.ts @@ -23,7 +23,7 @@ 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 { fail, parseArgs, say } from 'gcb-cli'; /** Longer than a credential request: a token is minted, then the action is performed. */ @@ -70,7 +70,7 @@ Acting on one Common --socket Broker socket (default: $GIT_BROKER_SOCKET or the socket file) --host Host as configured, e.g. github.com (default: github.com) - --repo Repository the pull request belongs to [required] + --repo Repository the pull request belongs to (default: origin remote) -h, --help Show this help `; @@ -132,13 +132,20 @@ export async function runPr(argv: readonly string[]): Promise { fail('--host needs a value (e.g. github.com)'); } const host = hostArg ?? 'github.com'; - const repo = args.value('repo') ?? fail('--repo is required (e.g. owner/name)'); + let repo = args.value('repo'); + 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}`; + } 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)); } const viaHost = action === 'open' && args.has('via-host'); - 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 diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts index a56ed983..e114c5f7 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -1264,6 +1264,40 @@ describe('the anchor flags, at the CLI boundary', () => { 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('sends the default host to the broker', async () => { const socketPath = path.join(scratch, 'pr-host-default.sock'); let request: Record | undefined; From f1a3859e1ae5971794306f97f429d14ed444d57c Mon Sep 17 00:00:00 2001 From: JounQin Date: Wed, 28 Oct 2026 15:26:24 +0000 Subject: [PATCH 06/13] refactor: route pr updates through gh pr edit The catalog now owns PR edits; the custom pull-request update path is gone. --- README.md | 1 + packages/cli/src/actions.ts | 16 ++++++++++++ packages/cli/src/types.ts | 1 - packages/client/src/commands/pr.ts | 13 +++++++++ packages/host/src/broker.ts | 6 +---- packages/host/src/providers/github-app.ts | 16 +++--------- test/unit/actions.test.ts | 1 + test/unit/commands.test.ts | 32 +++++++++++++++++++++++ 8 files changed, 68 insertions(+), 18 deletions(-) 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/types.ts b/packages/cli/src/types.ts index 309c4e7f..db32bf97 100644 --- a/packages/cli/src/types.ts +++ b/packages/cli/src/types.ts @@ -106,7 +106,6 @@ export interface JobLogRequest { /** What can be done to a pull request. */ export type PullRequestAction = | 'open' - | 'update' | 'status' | 'comment' | 'threads' diff --git a/packages/client/src/commands/pr.ts b/packages/client/src/commands/pr.ts index 122c9983..a9ee05fe 100644 --- a/packages/client/src/commands/pr.ts +++ b/packages/client/src/commands/pr.ts @@ -24,6 +24,7 @@ import { } from 'gcb-cli'; import { requestOverSocket, resolveSocketPath } from '../helper.ts'; import { baseBranchFor, qualifiedBranch, remoteRepo } from './remotes.ts'; +import { runGh } from './gh.ts'; import { fail, parseArgs, say } from 'gcb-cli'; /** Longer than a credential request: a token is minted, then the action is performed. */ @@ -229,6 +230,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()); diff --git a/packages/host/src/broker.ts b/packages/host/src/broker.ts index 11cd035d..c4e614ae 100644 --- a/packages/host/src/broker.ts +++ b/packages/host/src/broker.ts @@ -386,7 +386,6 @@ export function createRequestHandler( const action = message['action'] ?? 'open'; if ( action !== 'open' && - action !== 'update' && action !== 'status' && action !== 'comment' && action !== 'threads' && @@ -413,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)) { @@ -448,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'); } diff --git a/packages/host/src/providers/github-app.ts b/packages/host/src/providers/github-app.ts index 9548b6d1..49c56861 100644 --- a/packages/host/src/providers/github-app.ts +++ b/packages/host/src/providers/github-app.ts @@ -1019,19 +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]; - } - } - 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 diff --git a/test/unit/actions.test.ts b/test/unit/actions.test.ts index c2367ad2..34ef0921 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', diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts index e114c5f7..11a6d372 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -1298,6 +1298,38 @@ describe('the anchor flags, at the CLI boundary', () => { } }); + 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('sends the default host to the broker', async () => { const socketPath = path.join(scratch, 'pr-host-default.sock'); let request: Record | undefined; From c70c35f5c789a079ed6e0eed97298d4f6a0b376e Mon Sep 17 00:00:00 2001 From: JounQin Date: Wed, 28 Oct 2026 15:52:35 +0000 Subject: [PATCH 07/13] fix: reject removed merge flags and quote update values Prevent --merge from falling through to an edit, pass update values in --flag=value form, and cover the affected tests. --- packages/client/src/commands/pr.ts | 13 +++++++--- test/unit/actions.test.ts | 7 ++++++ test/unit/commands.test.ts | 40 ++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+), 3 deletions(-) diff --git a/packages/client/src/commands/pr.ts b/packages/client/src/commands/pr.ts index a9ee05fe..bb86676f 100644 --- a/packages/client/src/commands/pr.ts +++ b/packages/client/src/commands/pr.ts @@ -116,6 +116,12 @@ 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'); } + if (args.has('merge') || args.value('merge') !== undefined) { + fail('there is no merge action: merging is a human action on GitHub'); + } + if (args.has('method') || args.value('method') !== undefined) { + fail('--method was removed with --merge: there is no merge action'); + } // 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) { @@ -142,6 +148,7 @@ export async function runPr(argv: readonly string[]): Promise { } 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)); @@ -232,9 +239,9 @@ export async function runPr(argv: readonly string[]): Promise { 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); + 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'); diff --git a/test/unit/actions.test.ts b/test/unit/actions.test.ts index 34ef0921..305272d5 100644 --- a/test/unit/actions.test.ts +++ b/test/unit/actions.test.ts @@ -94,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/commands.test.ts b/test/unit/commands.test.ts index 11a6d372..18b18e9b 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -1173,6 +1173,14 @@ describe('the anchor flags, at the CLI boundary', () => { assert.match(result.stdout, /Usage: gcb gh/); }); + it('refuses the removed merge flags', () => { + for (const flag of [['--merge'], ['--method', 'squash']]) { + const result = cli([...base, ...flag, '--title', 'a title']); + assert.equal(result.status, 1); + assert.match(result.stderr, /no merge action/); + } + }); + it('refuses a plain --comment and names the anchor requirement', () => { const moved = cli([...base, '--comment', '--body', 'x']); assert.equal(moved.status, 1); @@ -1330,6 +1338,38 @@ describe('the anchor flags, at the CLI boundary', () => { } }); + 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; From 5a8bb0d04b810d7fb6a52e34dec8a89930c776ed Mon Sep 17 00:00:00 2001 From: JounQin Date: Wed, 28 Oct 2026 15:53:33 +0000 Subject: [PATCH 08/13] test: exercise the anchor rule on a valid non-comment action --- test/unit/broker.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/broker.test.ts b/test/unit/broker.test.ts index 4f81c328..90f0c192 100644 --- a/test/unit/broker.test.ts +++ b/test/unit/broker.test.ts @@ -378,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'); }); From 2b8379822599da84c5cb0a03a1bf3b5604f7b8ac Mon Sep 17 00:00:00 2001 From: JounQin Date: Wed, 28 Oct 2026 16:03:29 +0000 Subject: [PATCH 09/13] fix: reject unknown pr flags and positionals Removed merge flags now fail through the same unknown-option check as any other typo. --- packages/client/src/commands/pr.ts | 22 ++++++++++++++++------ test/unit/commands.test.ts | 12 +++++++++--- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/packages/client/src/commands/pr.ts b/packages/client/src/commands/pr.ts index bb86676f..b1cc1eaa 100644 --- a/packages/client/src/commands/pr.ts +++ b/packages/client/src/commands/pr.ts @@ -27,6 +27,13 @@ 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; @@ -112,16 +119,19 @@ export async function runPr(argv: readonly string[]): Promise { say(PR_USAGE); return 0; } + // Unknown options are refused rather than ignored: a typo must not silently fall through to an update. + for (const argument of argv) { + if (argument === '--') break; + if (!argument.startsWith('--')) continue; + const name = argument.slice(2).split('=', 1)[0] ?? ''; + 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'); } - if (args.has('merge') || args.value('merge') !== undefined) { - fail('there is no merge action: merging is a human action on GitHub'); - } - if (args.has('method') || args.value('method') !== undefined) { - fail('--method was removed with --merge: there is no merge action'); - } // 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) { diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts index 18b18e9b..e89956c7 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -1173,14 +1173,20 @@ describe('the anchor flags, at the CLI boundary', () => { assert.match(result.stdout, /Usage: gcb gh/); }); - it('refuses the removed merge flags', () => { - for (const flag of [['--merge'], ['--method', 'squash']]) { + it('refuses unknown options, including removed merge flags', () => { + for (const flag of [['--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, /no merge action/); + assert.match(result.stderr, /unknown option/); } }); + 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); From eabe4c78f6e760c2ec1fac9184f54c15e6f09a7c Mon Sep 17 00:00:00 2001 From: JounQin Date: Wed, 28 Oct 2026 16:11:17 +0000 Subject: [PATCH 10/13] fix: reject a valueless repo flag Keep origin as the default only when --repo is absent; an explicitly empty or valueless flag is refused. --- packages/client/src/commands/pr.ts | 4 +++- test/unit/commands.test.ts | 6 ++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/client/src/commands/pr.ts b/packages/client/src/commands/pr.ts index b1cc1eaa..9e592b7c 100644 --- a/packages/client/src/commands/pr.ts +++ b/packages/client/src/commands/pr.ts @@ -149,7 +149,9 @@ export async function runPr(argv: readonly string[]): Promise { fail('--host needs a value (e.g. github.com)'); } const host = hostArg ?? 'github.com'; - let repo = args.value('repo'); + const repoArg = args.value('repo'); + if (args.has('repo') && repoArg === undefined) 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'); diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts index e89956c7..b84369cd 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -1181,6 +1181,12 @@ describe('the anchor flags, at the CLI boundary', () => { } }); + it('refuses a valueless repo', () => { + const result = cli(['--repo', '--number', '7', '--status']); + assert.equal(result.status, 1); + assert.match(result.stderr, /--repo needs a value/); + }); + it('refuses unexpected positionals', () => { const result = cli(['--repo', 'acme/widget', '--number', '7', '--status', 'stray']); assert.equal(result.status, 1); From 58d01f8c191653491acb6b354234084e6a0d93ca Mon Sep 17 00:00:00 2001 From: JounQin Date: Thu, 29 Oct 2026 05:03:02 +0000 Subject: [PATCH 11/13] fix: validate normalized pr option names Use the parser's option names so single-dash typos are caught and values are not mistaken for options. --- packages/cli/src/support.ts | 3 +++ packages/client/src/commands/pr.ts | 5 +---- test/unit/commands.test.ts | 5 ++++- 3 files changed, 8 insertions(+), 5 deletions(-) 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/client/src/commands/pr.ts b/packages/client/src/commands/pr.ts index 9e592b7c..a770a9f1 100644 --- a/packages/client/src/commands/pr.ts +++ b/packages/client/src/commands/pr.ts @@ -120,10 +120,7 @@ export async function runPr(argv: readonly string[]): Promise { return 0; } // Unknown options are refused rather than ignored: a typo must not silently fall through to an update. - for (const argument of argv) { - if (argument === '--') break; - if (!argument.startsWith('--')) continue; - const name = argument.slice(2).split('=', 1)[0] ?? ''; + 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'); diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts index b84369cd..1586cd69 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -1174,11 +1174,14 @@ describe('the anchor flags, at the CLI boundary', () => { }); it('refuses unknown options, including removed merge flags', () => { - for (const flag of [['--merge'], ['--merge-xxx'], ['--method', 'squash'], ['--method-xxx', 'squash'], ['--xafajbsfbafabsjkf']]) { + 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 repo', () => { From 05def2b4548ad0986c1fe02eac078e704bfe6108 Mon Sep 17 00:00:00 2001 From: JounQin Date: Thu, 29 Oct 2026 06:02:07 +0000 Subject: [PATCH 12/13] fix: reject option-looking host and repo values parseArgs consumes single-dash tokens as values before normalization; refuse those at the pr boundary. --- packages/client/src/commands/pr.ts | 9 +++++++-- test/unit/commands.test.ts | 14 +++++++++++--- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/packages/client/src/commands/pr.ts b/packages/client/src/commands/pr.ts index a770a9f1..eb78d83b 100644 --- a/packages/client/src/commands/pr.ts +++ b/packages/client/src/commands/pr.ts @@ -141,13 +141,18 @@ 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 looksLikeOption = (value: string | undefined): boolean => + value !== undefined && /^-(?=[A-Za-z])/.test(value); const hostArg = args.value('host'); - if ((args.has('host') || hostArg !== undefined) && (hostArg === undefined || hostArg.trim() === '')) { + if ((args.has('host') || hostArg !== undefined) && + (hostArg === undefined || hostArg.trim() === '' || looksLikeOption(hostArg))) { fail('--host needs a value (e.g. github.com)'); } const host = hostArg ?? 'github.com'; const repoArg = args.value('repo'); - if (args.has('repo') && repoArg === undefined) fail('--repo needs a value (e.g. owner/name)'); + if ((args.has('repo') || repoArg !== undefined) && (repoArg === undefined || looksLikeOption(repoArg))) { + fail('--repo needs a value (e.g. owner/name)'); + } let repo = repoArg; if (repo === undefined) { const found = remoteRepo('origin', process.cwd()); diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts index 1586cd69..0f888715 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -1184,10 +1184,18 @@ describe('the anchor flags, at the CLI boundary', () => { assert.doesNotMatch(value.stderr, /unknown option/, 'a value is not an option'); }); - it('refuses a valueless repo', () => { - const result = cli(['--repo', '--number', '7', '--status']); + it('refuses a valueless or option-looking repo', () => { + for (const argv of [['--repo', '--number', '7', '--status'], ['--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', () => { + const result = cli(['--host', '-h', '--repo', 'acme/widget', '--number', '7', '--status']); assert.equal(result.status, 1); - assert.match(result.stderr, /--repo needs a value/); + assert.match(result.stderr, /--host needs a value/); }); it('refuses unexpected positionals', () => { From 912eee79c4d368c2bcd605dfa5fcdc05b722c017 Mon Sep 17 00:00:00 2001 From: JounQin Date: Thu, 29 Oct 2026 07:19:32 +0000 Subject: [PATCH 13/13] fix: validate every host and repo occurrence Reject option-looking or empty values on every occurrence, including --host=--help and repeated flags. --- packages/client/src/commands/pr.ts | 16 +++++++--------- test/unit/commands.test.ts | 14 ++++++++++---- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/packages/client/src/commands/pr.ts b/packages/client/src/commands/pr.ts index eb78d83b..ec3d1956 100644 --- a/packages/client/src/commands/pr.ts +++ b/packages/client/src/commands/pr.ts @@ -141,18 +141,16 @@ 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 looksLikeOption = (value: string | undefined): boolean => - value !== undefined && /^-(?=[A-Za-z])/.test(value); + 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 ((args.has('host') || hostArg !== undefined) && - (hostArg === undefined || hostArg.trim() === '' || looksLikeOption(hostArg))) { - fail('--host needs a value (e.g. github.com)'); - } + 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 ((args.has('repo') || repoArg !== undefined) && (repoArg === undefined || looksLikeOption(repoArg))) { - fail('--repo needs a value (e.g. owner/name)'); - } + 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()); diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts index 0f888715..a46103d5 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -1185,7 +1185,7 @@ describe('the anchor flags, at the CLI boundary', () => { }); it('refuses a valueless or option-looking repo', () => { - for (const argv of [['--repo', '--number', '7', '--status'], ['--repo', '-h', '--number', '7', '--status']]) { + 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/); @@ -1193,9 +1193,15 @@ describe('the anchor flags, at the CLI boundary', () => { }); it('refuses an option-looking host', () => { - const result = cli(['--host', '-h', '--repo', 'acme/widget', '--number', '7', '--status']); - assert.equal(result.status, 1); - assert.match(result.stderr, /--host needs a value/); + 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', () => {