From 9d9571b5f4a07e1c8c3ee6524a18f76ce5cb70e5 Mon Sep 17 00:00:00 2001 From: JounQin Date: Tue, 27 Oct 2026 05:31:15 +0000 Subject: [PATCH 1/6] feat: resolve the gh repository from the checkout remote When --repo is omitted, read origin (or the first remote) and normalize HTTPS, ssh and git@ forms; the broker still validates the result. --- packages/client/src/commands/gh.ts | 48 ++++++++++++++++++++++++++++-- test/unit/gh-command.test.ts | 40 +++++++++++++++++++++++-- 2 files changed, 84 insertions(+), 4 deletions(-) diff --git a/packages/client/src/commands/gh.ts b/packages/client/src/commands/gh.ts index f8c20563..df331957 100644 --- a/packages/client/src/commands/gh.ts +++ b/packages/client/src/commands/gh.ts @@ -4,6 +4,8 @@ * The action catalog defines the command, flags, positionals and permissions. The client only * parses names and values; the broker rebuilds the canonical argv and runs gh with its own token. */ +import { execFileSync } from 'node:child_process'; + import { GH_ACTIONS, getGhAction, parseArgs, say, fail, type ActionValues } from 'gcb-cli'; import { requestOverSocket, resolveSocketPath } from '../helper.ts'; @@ -29,13 +31,54 @@ const ACTION_LINES = Object.values(GH_ACTIONS) }) .join('\n'); +/** + * Read owner/repo from the checkout's git remote. + * + * @returns the normalized repository, or undefined when git/the remote cannot answer. + */ +function repoFromCheckout(): string | undefined { + let remotes: string[]; + try { + remotes = execFileSync('git', ['remote'], { encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }) + .split('\n') + .map((line) => line.trim()) + .filter((line) => line !== ''); + } catch { + return undefined; + } + const name = remotes.includes('origin') ? 'origin' : remotes[0]; + if (name === undefined) return undefined; + let url: string; + try { + url = execFileSync('git', ['remote', 'get-url', name], { + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'ignore'], + }).trim(); + } catch { + return undefined; + } + if (!/^(?:https?:\/\/|ssh:\/\/|git@)/i.test(url)) return undefined; + let pathPart = url.replace(/^[a-z]+:\/\//i, ''); + if (pathPart.startsWith('git@')) pathPart = pathPart.slice(4).replace(':', '/'); + else pathPart = pathPart.replace(/^[^/]+@/, ''); + const slash = pathPart.indexOf('/'); + if (slash < 0) return undefined; + const parts = pathPart + .slice(slash + 1) + .replace(/\.git$/, '') + .replace(/\/+$/, '') + .split('/'); + if (parts.length < 2) return undefined; + return `${parts[parts.length - 2]}/${parts[parts.length - 1]}`; +} + /** Usage text for `gh`. */ export const GH_USAGE = `Usage: gcb gh [options] Run a catalogued GitHub action through the broker. Common - --repo Repository the action belongs to [required] + --repo Repository the action belongs to (default: origin remote) --host Host as configured, e.g. github.com (default: github.com) --socket Broker socket (default: GIT_BROKER_SOCKET) -h, --help Show this help @@ -68,7 +111,8 @@ export async function runGh(argv: readonly string[]): Promise { const booleans = action.flags.filter((flag) => flag.type === 'boolean').map((flag) => flag.name); const args = parseArgs(rest, { booleans }); - const repo = args.value('repo') ?? fail('--repo is required'); + const repo = args.value('repo') ?? repoFromCheckout(); + if (repo === undefined) fail('--repo is required when the checkout has no git remote'); const host = args.value('host') ?? 'github.com'; const socketPath = resolveSocketPath(args.value('socket'), process.env); if (socketPath === undefined) fail('no broker socket is configured'); diff --git a/test/unit/gh-command.test.ts b/test/unit/gh-command.test.ts index e9f020b9..05426d1e 100644 --- a/test/unit/gh-command.test.ts +++ b/test/unit/gh-command.test.ts @@ -1,4 +1,5 @@ import assert from 'node:assert/strict'; +import { execFileSync } from 'node:child_process'; import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; @@ -13,11 +14,11 @@ 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[]): Promise { +function runGhCli(args: readonly string[], cwd?: string): Promise { return tryRunAsync( process.execPath, ['--conditions=development', CLIENT_BIN, 'gh', ...args], - { env: process.env, timeoutMs: 5000 }, + { cwd, env: process.env, timeoutMs: 5000 }, ); } @@ -133,6 +134,41 @@ describe('gcb gh', () => { } }); + it('resolves the repository from the checkout remote', async () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-gh-remote-')); + const checkout = path.join(root, 'checkout'); + fs.mkdirSync(checkout); + execFileSync('git', ['init', '--quiet'], { cwd: checkout }); + execFileSync('git', ['remote', 'add', 'origin', 'git@github.com:acme/widget.git'], { cwd: checkout }); + 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 provider: Provider = { + name: 'static', + getCredential: async () => ({ username: 'u', password: 'p', expiresAt: Date.now() + 1000 }), + runAction: async (request) => { + assert.equal(request.full, 'acme/widget'); + return { stdout: 'ok\n', stderr: '', code: 0, signal: null, truncated: false, timedOut: false }; + }, + }; + const broker = await startBroker({ config, audit, providers: new Map([['github.com', provider]]) }); + try { + const result = await runGhCli(['pr', 'view', '7', '--socket', socketPath], checkout); + assert.equal(result.status, 0); + assert.equal(result.stdout, 'ok\n'); + } 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'); From f6803751d4aec5ca9107601c952fa13e5456be3c Mon Sep 17 00:00:00 2001 From: JounQin Date: Tue, 27 Oct 2026 13:44:48 +0800 Subject: [PATCH 2/6] refactor: reuse the checkout remote resolver in gcb gh The new parser took the last two path segments, so a remote URL with more path than owner/repo could be misread. Use listRemotes/remoteRepo, which already own the URL rules and their tests, and keep the origin-first choice here. A regression test covers the extra-path case. --- packages/client/src/commands/gh.ts | 51 ++++++++---------------------- test/unit/gh-command.test.ts | 15 +++++++++ 2 files changed, 28 insertions(+), 38 deletions(-) diff --git a/packages/client/src/commands/gh.ts b/packages/client/src/commands/gh.ts index df331957..9d51229d 100644 --- a/packages/client/src/commands/gh.ts +++ b/packages/client/src/commands/gh.ts @@ -4,11 +4,10 @@ * The action catalog defines the command, flags, positionals and permissions. The client only * parses names and values; the broker rebuilds the canonical argv and runs gh with its own token. */ -import { execFileSync } from 'node:child_process'; - import { GH_ACTIONS, getGhAction, parseArgs, say, fail, type ActionValues } from 'gcb-cli'; import { requestOverSocket, resolveSocketPath } from '../helper.ts'; +import { listRemotes, remoteRepo } from './remotes.ts'; const ACTION_LINES = Object.values(GH_ACTIONS) .map((action) => { @@ -32,44 +31,20 @@ const ACTION_LINES = Object.values(GH_ACTIONS) .join('\n'); /** - * Read owner/repo from the checkout's git remote. + * Read owner/repo from the checkout's default remote: `origin` if it exists, else the first one. + * + * The URL parsing lives in `remotes.ts`, beside `--head origin:branch`, so the two cannot disagree + * about what a remote points at. * - * @returns the normalized repository, or undefined when git/the remote cannot answer. + * @param cwd - the checkout to ask. + * @returns the normalized repository, or undefined when no remote names one. */ -function repoFromCheckout(): string | undefined { - let remotes: string[]; - try { - remotes = execFileSync('git', ['remote'], { encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }) - .split('\n') - .map((line) => line.trim()) - .filter((line) => line !== ''); - } catch { - return undefined; - } +function repoFromCheckout(cwd: string): string | undefined { + const remotes = listRemotes(cwd); const name = remotes.includes('origin') ? 'origin' : remotes[0]; if (name === undefined) return undefined; - let url: string; - try { - url = execFileSync('git', ['remote', 'get-url', name], { - encoding: 'utf8', - stdio: ['ignore', 'pipe', 'ignore'], - }).trim(); - } catch { - return undefined; - } - if (!/^(?:https?:\/\/|ssh:\/\/|git@)/i.test(url)) return undefined; - let pathPart = url.replace(/^[a-z]+:\/\//i, ''); - if (pathPart.startsWith('git@')) pathPart = pathPart.slice(4).replace(':', '/'); - else pathPart = pathPart.replace(/^[^/]+@/, ''); - const slash = pathPart.indexOf('/'); - if (slash < 0) return undefined; - const parts = pathPart - .slice(slash + 1) - .replace(/\.git$/, '') - .replace(/\/+$/, '') - .split('/'); - if (parts.length < 2) return undefined; - return `${parts[parts.length - 2]}/${parts[parts.length - 1]}`; + const found = remoteRepo(name, cwd); + return found === undefined ? undefined : `${found.owner}/${found.repo}`; } /** Usage text for `gh`. */ @@ -111,8 +86,8 @@ export async function runGh(argv: readonly string[]): Promise { const booleans = action.flags.filter((flag) => flag.type === 'boolean').map((flag) => flag.name); const args = parseArgs(rest, { booleans }); - const repo = args.value('repo') ?? repoFromCheckout(); - if (repo === undefined) fail('--repo is required when the checkout has no git remote'); + const repo = args.value('repo') ?? repoFromCheckout(process.cwd()); + if (repo === undefined) fail('--repo is required when the checkout has no usable GitHub remote'); const host = args.value('host') ?? 'github.com'; const socketPath = resolveSocketPath(args.value('socket'), process.env); if (socketPath === undefined) fail('no broker socket is configured'); diff --git a/test/unit/gh-command.test.ts b/test/unit/gh-command.test.ts index 05426d1e..f8416168 100644 --- a/test/unit/gh-command.test.ts +++ b/test/unit/gh-command.test.ts @@ -169,6 +169,21 @@ describe('gcb gh', () => { } }); + it('refuses a remote URL that names more than owner/repo', async () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-gh-remote-path-')); + try { + const checkout = path.join(root, 'checkout'); + fs.mkdirSync(checkout); + execFileSync('git', ['init', '--quiet'], { cwd: checkout }); + execFileSync('git', ['remote', 'add', 'origin', 'https://github.com/acme/widget/issues'], { cwd: checkout }); + const result = await runGhCli(['pr', 'view', '7', '--socket', path.join(root, 'broker.sock')], checkout); + assert.equal(result.status, 1); + assert.match(result.stderr, /no usable GitHub remote/); + } finally { + 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'); From 647b7ac3e5ea445759e79ec241458ef06af5c9b0 Mon Sep 17 00:00:00 2001 From: JounQin Date: Tue, 27 Oct 2026 06:26:54 +0000 Subject: [PATCH 3/6] fix: check the remote host and ignore git location env Refuse a checkout remote on a different host than --host, and clear GIT_DIR/GIT_WORK_TREE-style variables before asking a specific checkout. --- packages/client/src/commands/gh.ts | 15 ++++++--- packages/client/src/commands/remotes.ts | 29 +++++++++++++--- test/unit/gh-command.test.ts | 44 ++++++++++++++++++++++--- test/unit/remotes.test.ts | 32 ++++++++++++++---- 4 files changed, 101 insertions(+), 19 deletions(-) diff --git a/packages/client/src/commands/gh.ts b/packages/client/src/commands/gh.ts index 9d51229d..404398ba 100644 --- a/packages/client/src/commands/gh.ts +++ b/packages/client/src/commands/gh.ts @@ -39,12 +39,12 @@ const ACTION_LINES = Object.values(GH_ACTIONS) * @param cwd - the checkout to ask. * @returns the normalized repository, or undefined when no remote names one. */ -function repoFromCheckout(cwd: string): string | undefined { +function repoFromCheckout(cwd: string): { repo: string; host: string } | undefined { const remotes = listRemotes(cwd); const name = remotes.includes('origin') ? 'origin' : remotes[0]; if (name === undefined) return undefined; const found = remoteRepo(name, cwd); - return found === undefined ? undefined : `${found.owner}/${found.repo}`; + return found === undefined ? undefined : { repo: `${found.owner}/${found.repo}`, host: found.host }; } /** Usage text for `gh`. */ @@ -86,9 +86,16 @@ export async function runGh(argv: readonly string[]): Promise { const booleans = action.flags.filter((flag) => flag.type === 'boolean').map((flag) => flag.name); const args = parseArgs(rest, { booleans }); - const repo = args.value('repo') ?? repoFromCheckout(process.cwd()); - if (repo === undefined) fail('--repo is required when the checkout has no usable GitHub remote'); const host = args.value('host') ?? 'github.com'; + let repo = args.value('repo'); + if (repo === undefined) { + const found = repoFromCheckout(process.cwd()); + if (found === undefined) fail('--repo is required when the checkout has no usable GitHub 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.repo; + } const socketPath = resolveSocketPath(args.value('socket'), process.env); if (socketPath === undefined) fail('no broker socket is configured'); diff --git a/packages/client/src/commands/remotes.ts b/packages/client/src/commands/remotes.ts index b60c13d0..bf9521ce 100644 --- a/packages/client/src/commands/remotes.ts +++ b/packages/client/src/commands/remotes.ts @@ -16,6 +16,7 @@ import { execFileSync } from 'node:child_process'; export interface RemoteRepo { readonly owner: string; readonly repo: string; + readonly host: string; } /** @@ -29,8 +30,22 @@ export interface RemoteRepo { * @returns the output, or undefined when git refused. */ function git(args: readonly string[], cwd: string): string | undefined { + const env = { ...process.env }; + for (const key of [ + 'GIT_DIR', + 'GIT_WORK_TREE', + 'GIT_INDEX_FILE', + 'GIT_OBJECT_DIRECTORY', + 'GIT_ALTERNATE_OBJECT_DIRECTORIES', + 'GIT_COMMON_DIR', + 'GIT_NAMESPACE', + 'GIT_CEILING_DIRECTORIES', + 'GIT_DISCOVERY_ACROSS_FILESYSTEM', + ]) { + delete env[key]; + } try { - const out = execFileSync('git', [...args], { cwd, encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }); + const out = execFileSync('git', [...args], { cwd, env, encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }); const trimmed = out.replace(/\s+$/, ''); return trimmed === '' ? undefined : trimmed; } catch { @@ -70,22 +85,26 @@ export function remoteRepo(name: string, cwd: string, push = false): RemoteRepo // path, a `file:` URL, a host with no dot — is refused rather than read as an owner that happens to be a // port number or a directory name. let path: string | undefined; + let host: string | undefined; if (/^[a-z+]+:\/\//i.test(url)) { const rest = url.replace(/^[a-z+]+:\/\//i, '').replace(/^[^@/]*@/, ''); const slash = rest.indexOf('/'); const authority = slash < 0 ? '' : rest.slice(0, slash); - const host = authority.split(':')[0] ?? ''; + host = authority.split(':')[0] ?? ''; if (slash >= 0 && host.includes('.')) path = rest.slice(slash + 1); } else { const scp = /^(?:[^@/]+@)?([^/:]+\.[^/:]+):(.+)$/.exec(url); - if (scp !== null && scp[2] !== undefined) path = scp[2]; + if (scp !== null && scp[1] !== undefined && scp[2] !== undefined) { + host = scp[1]; + path = scp[2]; + } } - if (path === undefined) return undefined; + if (path === undefined || host === undefined) return undefined; const cleaned = path.replace(/^\/+/, '').replace(/\/+$/, '').replace(/\.git$/i, ''); const [owner, repo, ...rest] = cleaned.split('/').filter((part) => part !== ''); if (owner === undefined || repo === undefined || rest.length > 0) return undefined; - return { owner, repo }; + return { owner, repo, host: host.toLowerCase() }; } /** diff --git a/test/unit/gh-command.test.ts b/test/unit/gh-command.test.ts index f8416168..a9ed8659 100644 --- a/test/unit/gh-command.test.ts +++ b/test/unit/gh-command.test.ts @@ -8,6 +8,26 @@ import { describe, it } from 'node:test'; import { startBroker } from '../../packages/host/src/broker.ts'; import { tryRunAsync, type RunResult } from '../lib/run.ts'; + +/** Git must not inherit a GIT_DIR/GIT_WORK_TREE that redirects it away from the temp checkout. */ +function cleanGitEnv(): NodeJS.ProcessEnv { + const env = { ...process.env }; + for (const key of [ + 'GIT_DIR', + 'GIT_WORK_TREE', + 'GIT_INDEX_FILE', + 'GIT_OBJECT_DIRECTORY', + 'GIT_ALTERNATE_OBJECT_DIRECTORIES', + 'GIT_COMMON_DIR', + 'GIT_NAMESPACE', + 'GIT_CEILING_DIRECTORIES', + 'GIT_DISCOVERY_ACROSS_FILESYSTEM', + ]) { + delete env[key]; + } + return env; +} + import type { AuditSink, BrokerConfig, Provider, StaticHostConfig } from '../../packages/cli/src/types.ts'; /** Run the client from source, next to this file rather than from the cwd. */ @@ -138,8 +158,8 @@ describe('gcb gh', () => { const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-gh-remote-')); const checkout = path.join(root, 'checkout'); fs.mkdirSync(checkout); - execFileSync('git', ['init', '--quiet'], { cwd: checkout }); - execFileSync('git', ['remote', 'add', 'origin', 'git@github.com:acme/widget.git'], { cwd: checkout }); + execFileSync('git', ['init', '--quiet'], { cwd: checkout, env: cleanGitEnv() }); + execFileSync('git', ['remote', 'add', 'origin', 'git@github.com:acme/widget.git'], { cwd: checkout, env: cleanGitEnv() }); const socketPath = path.join(root, 'broker.sock'); const block: StaticHostConfig = { provider: 'static', allow: ['acme/widget'] }; const config: BrokerConfig = { @@ -174,8 +194,8 @@ describe('gcb gh', () => { try { const checkout = path.join(root, 'checkout'); fs.mkdirSync(checkout); - execFileSync('git', ['init', '--quiet'], { cwd: checkout }); - execFileSync('git', ['remote', 'add', 'origin', 'https://github.com/acme/widget/issues'], { cwd: checkout }); + execFileSync('git', ['init', '--quiet'], { cwd: checkout, env: cleanGitEnv() }); + execFileSync('git', ['remote', 'add', 'origin', 'https://github.com/acme/widget/issues'], { cwd: checkout, env: cleanGitEnv() }); const result = await runGhCli(['pr', 'view', '7', '--socket', path.join(root, 'broker.sock')], checkout); assert.equal(result.status, 1); assert.match(result.stderr, /no usable GitHub remote/); @@ -184,6 +204,22 @@ describe('gcb gh', () => { } }); + it('refuses a checkout remote on another host', async () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-gh-remote-host-')); + try { + const checkout = path.join(root, 'checkout'); + fs.mkdirSync(checkout); + execFileSync('git', ['init', '--quiet'], { cwd: checkout, env: cleanGitEnv() }); + execFileSync('git', ['remote', 'add', 'origin', 'git@gitlab.com:acme/widget.git'], { cwd: checkout, env: cleanGitEnv() }); + const result = await runGhCli(['pr', 'view', '7', '--socket', path.join(root, 'broker.sock')], checkout); + assert.equal(result.status, 1); + assert.match(result.stderr, /gitlab\.com/); + assert.match(result.stderr, /--host/); + } finally { + 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'); diff --git a/test/unit/remotes.test.ts b/test/unit/remotes.test.ts index c750f566..53783cbd 100644 --- a/test/unit/remotes.test.ts +++ b/test/unit/remotes.test.ts @@ -14,12 +14,32 @@ import { after, before, describe, it } from 'node:test'; import { baseBranchFor, listRemotes, qualifiedBranch, remoteRepo } from '../../packages/client/src/commands/remotes.ts'; +/** Git must not inherit a GIT_DIR/GIT_WORK_TREE that redirects it away from the temp checkout. */ +function cleanGitEnv(): NodeJS.ProcessEnv { + const env = { ...process.env }; + for (const key of [ + 'GIT_DIR', + 'GIT_WORK_TREE', + 'GIT_INDEX_FILE', + 'GIT_OBJECT_DIRECTORY', + 'GIT_ALTERNATE_OBJECT_DIRECTORIES', + 'GIT_COMMON_DIR', + 'GIT_NAMESPACE', + 'GIT_CEILING_DIRECTORIES', + 'GIT_DISCOVERY_ACROSS_FILESYSTEM', + ]) { + delete env[key]; + } + return env; +} + + describe('resolving a branch that names a remote', () => { let dir = ''; before(() => { dir = mkdtempSync(join(tmpdir(), 'remotes-')); - const git = (...args: string[]) => execFileSync('git', args, { cwd: dir, stdio: 'ignore' }); + const git = (...args: string[]) => execFileSync('git', args, { cwd: dir, env: cleanGitEnv(), stdio: 'ignore' }); git('init', '-q'); git('remote', 'add', 'origin', 'git@github.com:me/git-credential-broker.git'); git('remote', 'add', 'upstream', 'https://github.com/code-vaults/git-credential-broker.git'); @@ -30,8 +50,8 @@ describe('resolving a branch that names a remote', () => { }); it('reads an SSH url and an HTTPS one the same way', () => { - assert.deepEqual(remoteRepo('origin', dir), { owner: 'me', repo: 'git-credential-broker' }); - assert.deepEqual(remoteRepo('upstream', dir), { owner: 'code-vaults', repo: 'git-credential-broker' }); + assert.deepEqual(remoteRepo('origin', dir), { owner: 'me', repo: 'git-credential-broker', host: 'github.com' }); + assert.deepEqual(remoteRepo('upstream', dir), { owner: 'code-vaults', repo: 'git-credential-broker', host: 'github.com' }); }); it('leaves a plain branch alone, because it is a branch of the repository --repo names', () => { @@ -66,7 +86,7 @@ describe('what a remote URL is allowed to look like', () => { before(() => { dir = mkdtempSync(join(tmpdir(), 'remotes-forms-')); - const git = (...args: string[]) => execFileSync('git', args, { cwd: dir, stdio: 'ignore' }); + const git = (...args: string[]) => execFileSync('git', args, { cwd: dir, env: cleanGitEnv(), stdio: 'ignore' }); git('init', '-q'); git('remote', 'add', 'ported', 'https://git.example.com:8443/me/thing.git'); git('remote', 'add', 'prefixed', 'https://git.example.com/gitlab/group/thing.git'); @@ -79,7 +99,7 @@ describe('what a remote URL is allowed to look like', () => { }); it('does not read a port as an owner', () => { - assert.deepEqual(remoteRepo('ported', dir), { owner: 'me', repo: 'thing' }); + assert.deepEqual(remoteRepo('ported', dir), { owner: 'me', repo: 'thing', host: 'git.example.com' }); }); it('does not read a directory as a host', () => { @@ -102,7 +122,7 @@ describe('the branch a --base means', () => { before(() => { dir = mkdtempSync(join(tmpdir(), 'remotes-base-')); - const git = (...args: string[]) => execFileSync('git', args, { cwd: dir, stdio: 'ignore' }); + const git = (...args: string[]) => execFileSync('git', args, { cwd: dir, env: cleanGitEnv(), stdio: 'ignore' }); git('init', '-q'); git('remote', 'add', 'upstream', 'https://github.com/code-vaults/git-credential-broker.git'); git('remote', 'add', 'origin', 'git@github.com:me/git-credential-broker.git'); From d12781bd06349d20a210733e1389468cd820eb37 Mon Sep 17 00:00:00 2001 From: JounQin Date: Tue, 27 Oct 2026 14:35:53 +0800 Subject: [PATCH 4/6] fix: keep the HTTP(S) port in the remote host remoteRepo dropped the port, so a checkout on https://host:8443 could not match --host host:8443. Keep the port for HTTP(S) remotes, drop it for SSH/git remotes, and cover both. Move the test-only GIT_* cleanup into test/lib/run.ts so both suites share it, and document the omitted --repo default. --- README.md | 3 +++ packages/client/src/commands/remotes.ts | 16 ++++++++----- test/lib/run.ts | 26 +++++++++++++++++++++ test/unit/gh-command.test.ts | 22 +----------------- test/unit/remotes.test.ts | 30 +++++++------------------ 5 files changed, 49 insertions(+), 48 deletions(-) diff --git a/README.md b/README.md index e650da55..8c74619f 100644 --- a/README.md +++ b/README.md @@ -121,6 +121,9 @@ gcb gh pr create --repo owner/name --head feat/x --title "A title" --body "A bod gcb gh run view 123456 --log --repo owner/name ``` +Inside a checkout, `--repo` can be omitted: it is read from `origin` (or the first remote), and the +remote's host must match `--host`. An explicit `--repo` still wins. + The exact command, flags and required values come from [`packages/cli/src/actions.ts`](packages/cli/src/actions.ts); `gcb gh --help` renders that catalog. diff --git a/packages/client/src/commands/remotes.ts b/packages/client/src/commands/remotes.ts index bf9521ce..e646f58f 100644 --- a/packages/client/src/commands/remotes.ts +++ b/packages/client/src/commands/remotes.ts @@ -12,10 +12,11 @@ */ import { execFileSync } from 'node:child_process'; -/** What a remote's URL says: the owner and the repository, for a GitHub-shaped URL. */ +/** What a remote's URL says: the owner, repository and host key, for a GitHub-shaped URL. */ export interface RemoteRepo { readonly owner: string; readonly repo: string; + /** The host key: `host:port` for HTTP(S), or the host alone for SSH and git URLs. */ readonly host: string; } @@ -73,7 +74,7 @@ export function listRemotes(cwd: string): string[] { * * @param name - the remote name. * @param cwd - the directory to ask. - * @returns the owner and repository, or undefined when there is no such remote or the URL is not one. + * @returns the owner, repository and host, or undefined when there is no such remote or the URL is not one. */ export function remoteRepo(name: string, cwd: string, push = false): RemoteRepo | undefined { // `--push` for a head: git lets a remote fetch from one place and push to another, and the branch a @@ -86,12 +87,17 @@ export function remoteRepo(name: string, cwd: string, push = false): RemoteRepo // port number or a directory name. let path: string | undefined; let host: string | undefined; - if (/^[a-z+]+:\/\//i.test(url)) { + const scheme = /^([a-z+]+):\/\//i.exec(url)?.[1]?.toLowerCase(); + if (scheme !== undefined) { const rest = url.replace(/^[a-z+]+:\/\//i, '').replace(/^[^@/]*@/, ''); const slash = rest.indexOf('/'); const authority = slash < 0 ? '' : rest.slice(0, slash); - host = authority.split(':')[0] ?? ''; - if (slash >= 0 && host.includes('.')) path = rest.slice(slash + 1); + const hostname = authority.split(':')[0] ?? ''; + if (slash >= 0 && hostname.includes('.')) { + // An HTTP(S) port is part of the host key the broker matches; an SSH or git port is not. + host = scheme === 'http' || scheme === 'https' ? authority : hostname; + path = rest.slice(slash + 1); + } } else { const scp = /^(?:[^@/]+@)?([^/:]+\.[^/:]+):(.+)$/.exec(url); if (scp !== null && scp[1] !== undefined && scp[2] !== undefined) { diff --git a/test/lib/run.ts b/test/lib/run.ts index 16b3f3ab..6a34872c 100644 --- a/test/lib/run.ts +++ b/test/lib/run.ts @@ -36,6 +36,32 @@ export function sleep(ms: number): Promise { return new Promise((resolve) => setTimeout(resolve, ms)); } +/** + * The process environment without variables that point git at another repository. + * + * A test that runs git in a temporary checkout must not inherit `GIT_DIR`, `GIT_WORK_TREE` and + * friends from the process that started it; otherwise `cwd` is ignored. + * + * @returns a copy of the environment with the repository-location variables removed. + */ +export function cleanGitEnv(): NodeJS.ProcessEnv { + const env = { ...process.env }; + for (const key of [ + 'GIT_DIR', + 'GIT_WORK_TREE', + 'GIT_INDEX_FILE', + 'GIT_OBJECT_DIRECTORY', + 'GIT_ALTERNATE_OBJECT_DIRECTORIES', + 'GIT_COMMON_DIR', + 'GIT_NAMESPACE', + 'GIT_CEILING_DIRECTORIES', + 'GIT_DISCOVERY_ACROSS_FILESYSTEM', + ]) { + delete env[key]; + } + return env; +} + /** * Run a command without blocking the event loop, returning its result without throwing. * diff --git a/test/unit/gh-command.test.ts b/test/unit/gh-command.test.ts index a9ed8659..409a0df8 100644 --- a/test/unit/gh-command.test.ts +++ b/test/unit/gh-command.test.ts @@ -7,27 +7,7 @@ import { fileURLToPath } from 'node:url'; import { describe, it } from 'node:test'; import { startBroker } from '../../packages/host/src/broker.ts'; -import { tryRunAsync, type RunResult } from '../lib/run.ts'; - -/** Git must not inherit a GIT_DIR/GIT_WORK_TREE that redirects it away from the temp checkout. */ -function cleanGitEnv(): NodeJS.ProcessEnv { - const env = { ...process.env }; - for (const key of [ - 'GIT_DIR', - 'GIT_WORK_TREE', - 'GIT_INDEX_FILE', - 'GIT_OBJECT_DIRECTORY', - 'GIT_ALTERNATE_OBJECT_DIRECTORIES', - 'GIT_COMMON_DIR', - 'GIT_NAMESPACE', - 'GIT_CEILING_DIRECTORIES', - 'GIT_DISCOVERY_ACROSS_FILESYSTEM', - ]) { - delete env[key]; - } - return env; -} - +import { cleanGitEnv, tryRunAsync, type RunResult } from '../lib/run.ts'; import type { AuditSink, BrokerConfig, Provider, StaticHostConfig } from '../../packages/cli/src/types.ts'; /** Run the client from source, next to this file rather than from the cwd. */ diff --git a/test/unit/remotes.test.ts b/test/unit/remotes.test.ts index 53783cbd..7fb467ae 100644 --- a/test/unit/remotes.test.ts +++ b/test/unit/remotes.test.ts @@ -13,26 +13,7 @@ import { join } from 'node:path'; import { after, before, describe, it } from 'node:test'; import { baseBranchFor, listRemotes, qualifiedBranch, remoteRepo } from '../../packages/client/src/commands/remotes.ts'; - -/** Git must not inherit a GIT_DIR/GIT_WORK_TREE that redirects it away from the temp checkout. */ -function cleanGitEnv(): NodeJS.ProcessEnv { - const env = { ...process.env }; - for (const key of [ - 'GIT_DIR', - 'GIT_WORK_TREE', - 'GIT_INDEX_FILE', - 'GIT_OBJECT_DIRECTORY', - 'GIT_ALTERNATE_OBJECT_DIRECTORIES', - 'GIT_COMMON_DIR', - 'GIT_NAMESPACE', - 'GIT_CEILING_DIRECTORIES', - 'GIT_DISCOVERY_ACROSS_FILESYSTEM', - ]) { - delete env[key]; - } - return env; -} - +import { cleanGitEnv } from '../lib/run.ts'; describe('resolving a branch that names a remote', () => { let dir = ''; @@ -89,6 +70,7 @@ describe('what a remote URL is allowed to look like', () => { const git = (...args: string[]) => execFileSync('git', args, { cwd: dir, env: cleanGitEnv(), stdio: 'ignore' }); git('init', '-q'); git('remote', 'add', 'ported', 'https://git.example.com:8443/me/thing.git'); + git('remote', 'add', 'ssh-ported', 'ssh://git@git.example.com:2222/me/thing.git'); git('remote', 'add', 'prefixed', 'https://git.example.com/gitlab/group/thing.git'); git('remote', 'add', 'local', '/srv/git/thing.git'); git('remote', 'add', 'files', 'file:///srv/git/thing.git'); @@ -98,8 +80,12 @@ describe('what a remote URL is allowed to look like', () => { rmSync(dir, { recursive: true, force: true }); }); - it('does not read a port as an owner', () => { - assert.deepEqual(remoteRepo('ported', dir), { owner: 'me', repo: 'thing', host: 'git.example.com' }); + it('does not read a port as an owner, and keeps it in an HTTP(S) host', () => { + assert.deepEqual(remoteRepo('ported', dir), { owner: 'me', repo: 'thing', host: 'git.example.com:8443' }); + }); + + it('drops an SSH port, because the API host has none', () => { + assert.deepEqual(remoteRepo('ssh-ported', dir), { owner: 'me', repo: 'thing', host: 'git.example.com' }); }); it('does not read a directory as a host', () => { From a9ebc05483e7827b210fd32b0c8c4af22d210e4d Mon Sep 17 00:00:00 2001 From: JounQin Date: Tue, 27 Oct 2026 06:58:30 +0000 Subject: [PATCH 5/6] refactor: require origin and drop the Git env cleanup Only origin names the default repository; anything else needs --repo. Remove the GIT_* scrubbing that normal CLI invocation does not need. --- README.md | 4 ++-- packages/client/src/commands/gh.ts | 9 ++++----- packages/client/src/commands/remotes.ts | 16 +-------------- test/lib/run.ts | 26 ------------------------- test/unit/gh-command.test.ts | 16 +++++++-------- test/unit/remotes.test.ts | 7 +++---- 6 files changed, 18 insertions(+), 60 deletions(-) diff --git a/README.md b/README.md index 8c74619f..8602d9f1 100644 --- a/README.md +++ b/README.md @@ -121,8 +121,8 @@ gcb gh pr create --repo owner/name --head feat/x --title "A title" --body "A bod gcb gh run view 123456 --log --repo owner/name ``` -Inside a checkout, `--repo` can be omitted: it is read from `origin` (or the first remote), and the -remote's host must match `--host`. An explicit `--repo` still wins. +Inside a checkout, `--repo` can be omitted only when `origin` names a usable GitHub repository; its +host must also match `--host`. Anything else requires an explicit `--repo` or `--host`. The exact command, flags and required values come from [`packages/cli/src/actions.ts`](packages/cli/src/actions.ts); `gcb gh --help` renders that catalog. diff --git a/packages/client/src/commands/gh.ts b/packages/client/src/commands/gh.ts index 404398ba..3eefed27 100644 --- a/packages/client/src/commands/gh.ts +++ b/packages/client/src/commands/gh.ts @@ -31,7 +31,7 @@ const ACTION_LINES = Object.values(GH_ACTIONS) .join('\n'); /** - * Read owner/repo from the checkout's default remote: `origin` if it exists, else the first one. + * Read owner/repo from the checkout's `origin` remote. * * The URL parsing lives in `remotes.ts`, beside `--head origin:branch`, so the two cannot disagree * about what a remote points at. @@ -41,9 +41,8 @@ const ACTION_LINES = Object.values(GH_ACTIONS) */ function repoFromCheckout(cwd: string): { repo: string; host: string } | undefined { const remotes = listRemotes(cwd); - const name = remotes.includes('origin') ? 'origin' : remotes[0]; - if (name === undefined) return undefined; - const found = remoteRepo(name, cwd); + if (!remotes.includes('origin')) return undefined; + const found = remoteRepo('origin', cwd); return found === undefined ? undefined : { repo: `${found.owner}/${found.repo}`, host: found.host }; } @@ -90,7 +89,7 @@ export async function runGh(argv: readonly string[]): Promise { let repo = args.value('repo'); if (repo === undefined) { const found = repoFromCheckout(process.cwd()); - if (found === undefined) fail('--repo is required when the checkout has no usable GitHub remote'); + 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`); } diff --git a/packages/client/src/commands/remotes.ts b/packages/client/src/commands/remotes.ts index e646f58f..c2bf009a 100644 --- a/packages/client/src/commands/remotes.ts +++ b/packages/client/src/commands/remotes.ts @@ -31,22 +31,8 @@ export interface RemoteRepo { * @returns the output, or undefined when git refused. */ function git(args: readonly string[], cwd: string): string | undefined { - const env = { ...process.env }; - for (const key of [ - 'GIT_DIR', - 'GIT_WORK_TREE', - 'GIT_INDEX_FILE', - 'GIT_OBJECT_DIRECTORY', - 'GIT_ALTERNATE_OBJECT_DIRECTORIES', - 'GIT_COMMON_DIR', - 'GIT_NAMESPACE', - 'GIT_CEILING_DIRECTORIES', - 'GIT_DISCOVERY_ACROSS_FILESYSTEM', - ]) { - delete env[key]; - } try { - const out = execFileSync('git', [...args], { cwd, env, encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }); + const out = execFileSync('git', [...args], { cwd, encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }); const trimmed = out.replace(/\s+$/, ''); return trimmed === '' ? undefined : trimmed; } catch { diff --git a/test/lib/run.ts b/test/lib/run.ts index 6a34872c..16b3f3ab 100644 --- a/test/lib/run.ts +++ b/test/lib/run.ts @@ -36,32 +36,6 @@ export function sleep(ms: number): Promise { return new Promise((resolve) => setTimeout(resolve, ms)); } -/** - * The process environment without variables that point git at another repository. - * - * A test that runs git in a temporary checkout must not inherit `GIT_DIR`, `GIT_WORK_TREE` and - * friends from the process that started it; otherwise `cwd` is ignored. - * - * @returns a copy of the environment with the repository-location variables removed. - */ -export function cleanGitEnv(): NodeJS.ProcessEnv { - const env = { ...process.env }; - for (const key of [ - 'GIT_DIR', - 'GIT_WORK_TREE', - 'GIT_INDEX_FILE', - 'GIT_OBJECT_DIRECTORY', - 'GIT_ALTERNATE_OBJECT_DIRECTORIES', - 'GIT_COMMON_DIR', - 'GIT_NAMESPACE', - 'GIT_CEILING_DIRECTORIES', - 'GIT_DISCOVERY_ACROSS_FILESYSTEM', - ]) { - delete env[key]; - } - return env; -} - /** * Run a command without blocking the event loop, returning its result without throwing. * diff --git a/test/unit/gh-command.test.ts b/test/unit/gh-command.test.ts index 409a0df8..a7edf1f4 100644 --- a/test/unit/gh-command.test.ts +++ b/test/unit/gh-command.test.ts @@ -7,7 +7,7 @@ import { fileURLToPath } from 'node:url'; import { describe, it } from 'node:test'; import { startBroker } from '../../packages/host/src/broker.ts'; -import { cleanGitEnv, tryRunAsync, type RunResult } from '../lib/run.ts'; +import { tryRunAsync, type RunResult } from '../lib/run.ts'; import type { AuditSink, BrokerConfig, Provider, StaticHostConfig } from '../../packages/cli/src/types.ts'; /** Run the client from source, next to this file rather than from the cwd. */ @@ -138,8 +138,8 @@ describe('gcb gh', () => { const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-gh-remote-')); const checkout = path.join(root, 'checkout'); fs.mkdirSync(checkout); - execFileSync('git', ['init', '--quiet'], { cwd: checkout, env: cleanGitEnv() }); - execFileSync('git', ['remote', 'add', 'origin', 'git@github.com:acme/widget.git'], { cwd: checkout, env: cleanGitEnv() }); + execFileSync('git', ['init', '--quiet'], { cwd: checkout }); + execFileSync('git', ['remote', 'add', 'origin', 'git@github.com:acme/widget.git'], { cwd: checkout }); const socketPath = path.join(root, 'broker.sock'); const block: StaticHostConfig = { provider: 'static', allow: ['acme/widget'] }; const config: BrokerConfig = { @@ -174,11 +174,11 @@ describe('gcb gh', () => { try { const checkout = path.join(root, 'checkout'); fs.mkdirSync(checkout); - execFileSync('git', ['init', '--quiet'], { cwd: checkout, env: cleanGitEnv() }); - execFileSync('git', ['remote', 'add', 'origin', 'https://github.com/acme/widget/issues'], { cwd: checkout, env: cleanGitEnv() }); + execFileSync('git', ['init', '--quiet'], { cwd: checkout }); + execFileSync('git', ['remote', 'add', 'origin', 'https://github.com/acme/widget/issues'], { cwd: checkout }); const result = await runGhCli(['pr', 'view', '7', '--socket', path.join(root, 'broker.sock')], checkout); assert.equal(result.status, 1); - assert.match(result.stderr, /no usable GitHub remote/); + assert.match(result.stderr, /no usable origin remote/); } finally { fs.rmSync(root, { recursive: true, force: true }); } @@ -189,8 +189,8 @@ describe('gcb gh', () => { try { const checkout = path.join(root, 'checkout'); fs.mkdirSync(checkout); - execFileSync('git', ['init', '--quiet'], { cwd: checkout, env: cleanGitEnv() }); - execFileSync('git', ['remote', 'add', 'origin', 'git@gitlab.com:acme/widget.git'], { cwd: checkout, env: cleanGitEnv() }); + execFileSync('git', ['init', '--quiet'], { cwd: checkout }); + execFileSync('git', ['remote', 'add', 'origin', 'git@gitlab.com:acme/widget.git'], { cwd: checkout }); const result = await runGhCli(['pr', 'view', '7', '--socket', path.join(root, 'broker.sock')], checkout); assert.equal(result.status, 1); assert.match(result.stderr, /gitlab\.com/); diff --git a/test/unit/remotes.test.ts b/test/unit/remotes.test.ts index 7fb467ae..b03b62ae 100644 --- a/test/unit/remotes.test.ts +++ b/test/unit/remotes.test.ts @@ -13,14 +13,13 @@ import { join } from 'node:path'; import { after, before, describe, it } from 'node:test'; import { baseBranchFor, listRemotes, qualifiedBranch, remoteRepo } from '../../packages/client/src/commands/remotes.ts'; -import { cleanGitEnv } from '../lib/run.ts'; describe('resolving a branch that names a remote', () => { let dir = ''; before(() => { dir = mkdtempSync(join(tmpdir(), 'remotes-')); - const git = (...args: string[]) => execFileSync('git', args, { cwd: dir, env: cleanGitEnv(), stdio: 'ignore' }); + const git = (...args: string[]) => execFileSync('git', args, { cwd: dir, stdio: 'ignore' }); git('init', '-q'); git('remote', 'add', 'origin', 'git@github.com:me/git-credential-broker.git'); git('remote', 'add', 'upstream', 'https://github.com/code-vaults/git-credential-broker.git'); @@ -67,7 +66,7 @@ describe('what a remote URL is allowed to look like', () => { before(() => { dir = mkdtempSync(join(tmpdir(), 'remotes-forms-')); - const git = (...args: string[]) => execFileSync('git', args, { cwd: dir, env: cleanGitEnv(), stdio: 'ignore' }); + const git = (...args: string[]) => execFileSync('git', args, { cwd: dir, stdio: 'ignore' }); git('init', '-q'); git('remote', 'add', 'ported', 'https://git.example.com:8443/me/thing.git'); git('remote', 'add', 'ssh-ported', 'ssh://git@git.example.com:2222/me/thing.git'); @@ -108,7 +107,7 @@ describe('the branch a --base means', () => { before(() => { dir = mkdtempSync(join(tmpdir(), 'remotes-base-')); - const git = (...args: string[]) => execFileSync('git', args, { cwd: dir, env: cleanGitEnv(), stdio: 'ignore' }); + const git = (...args: string[]) => execFileSync('git', args, { cwd: dir, stdio: 'ignore' }); git('init', '-q'); git('remote', 'add', 'upstream', 'https://github.com/code-vaults/git-credential-broker.git'); git('remote', 'add', 'origin', 'git@github.com:me/git-credential-broker.git'); From 2271ea9f96c66f9d7cdd7c24713e4cd02c2888d5 Mon Sep 17 00:00:00 2001 From: JounQin Date: Tue, 27 Oct 2026 15:11:03 +0800 Subject: [PATCH 6/6] refactor: ask git for origin directly remoteRepo('origin') already returns undefined when origin is absent, so listing the remotes first only adds a subprocess and an import. Tighten the README wording for when passing --host is enough. --- README.md | 4 ++-- packages/client/src/commands/gh.ts | 4 +--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 8602d9f1..3f66ec27 100644 --- a/README.md +++ b/README.md @@ -121,8 +121,8 @@ gcb gh pr create --repo owner/name --head feat/x --title "A title" --body "A bod gcb gh run view 123456 --log --repo owner/name ``` -Inside a checkout, `--repo` can be omitted only when `origin` names a usable GitHub repository; its -host must also match `--host`. Anything else requires an explicit `--repo` or `--host`. +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`. The exact command, flags and required values come from [`packages/cli/src/actions.ts`](packages/cli/src/actions.ts); `gcb gh --help` renders that catalog. diff --git a/packages/client/src/commands/gh.ts b/packages/client/src/commands/gh.ts index 3eefed27..d8ebe440 100644 --- a/packages/client/src/commands/gh.ts +++ b/packages/client/src/commands/gh.ts @@ -7,7 +7,7 @@ import { GH_ACTIONS, getGhAction, parseArgs, say, fail, type ActionValues } from 'gcb-cli'; import { requestOverSocket, resolveSocketPath } from '../helper.ts'; -import { listRemotes, remoteRepo } from './remotes.ts'; +import { remoteRepo } from './remotes.ts'; const ACTION_LINES = Object.values(GH_ACTIONS) .map((action) => { @@ -40,8 +40,6 @@ const ACTION_LINES = Object.values(GH_ACTIONS) * @returns the normalized repository, or undefined when no remote names one. */ function repoFromCheckout(cwd: string): { repo: string; host: string } | undefined { - const remotes = listRemotes(cwd); - if (!remotes.includes('origin')) return undefined; const found = remoteRepo('origin', cwd); return found === undefined ? undefined : { repo: `${found.owner}/${found.repo}`, host: found.host }; }