From 3ad61bb5d5001771c2f52c9fa5a79144aee894a5 Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 1 Oct 2026 17:57:08 -0700 Subject: [PATCH 1/4] F2d: push the task head to its branch with a real BranchPusher (#98) GitBranchPusher pushes from the runner-owned repository to https:////.git, with gh as Git's only credential helper and only the Git and gh allowlisted environment. It reads the remote branch first: at the head it does nothing; absent or at a commit the task's ledger records as owned, it pushes with --force-with-lease pinned to the value read; anything else is refused with BranchPushRefused and nothing is pushed. After the push it reads the branch back and requires the head. An abort keeps the caller's reason. Wiring into web/cli.ts stays in #91. Co-Authored-By: Claude Opus 5.5 --- runner/branch-push.ts | 137 ++++++++++++++++++ runner/publish.ts | 2 +- test/runner-branch-push.test.ts | 245 ++++++++++++++++++++++++++++++++ 3 files changed, 383 insertions(+), 1 deletion(-) create mode 100644 runner/branch-push.ts create mode 100644 test/runner-branch-push.test.ts diff --git a/runner/branch-push.ts b/runner/branch-push.ts new file mode 100644 index 00000000..d28ef81c --- /dev/null +++ b/runner/branch-push.ts @@ -0,0 +1,137 @@ +import type { PlanIdentity } from '../core/identity.ts'; +import { runInProcessGroup } from '../agents/process-group.ts'; +import { GIT_OPTIONS, gitEnvironment } from '../git/clone.ts'; +import { ghEnvironment } from '../github/gh-env.ts'; +import type { BranchPusher } from './publish.ts'; +import type { GitCallOptions, RunnerRepository } from './runner-repository.ts'; + +/** + * The real push (#98): makes the task's branch on GitHub point at the task head, from the runner-owned repository. The + * user's checkout is never read or written, and neither the user's Git config nor their credential helpers are used. + */ + +/** The branch holds a commit codeboost did not make, or moved while the push ran. Nothing was pushed; a person decides. */ +export class BranchPushRefused extends Error {} + +export interface GitBranchPusherConfig extends Pick { + /** The runner-owned repository the head is pushed from. */ + readonly repository: RunnerRepository; + /** The `repositoryId` of every identity this pusher serves; any other is refused. */ + readonly repositoryId: string; + /** `owner/name` on GitHub. */ + readonly remote: string; + /** + * The commits the task's ledger records as owned (made by codeboost). Only these may be overwritten. Read after the + * remote branch, so a ledger written meanwhile is seen. + */ + readonly ownedCommits: (identity: PlanIdentity) => Iterable; + /** The push URL. Default `https:///.git`. A local path is allowed, for tests. */ + readonly url?: string; + /** The environment `gh` and Git read their allowlisted variables from. Default `process.env`. */ + readonly env?: NodeJS.ProcessEnv; +} + +const COMMIT_ID = /^(?:[0-9a-f]{40}|[0-9a-f]{64})$/; +// The shape `PullRequestPublisher.branch` makes. Nothing outside `codeboost/` is ever pushed. +const BRANCH = /^codeboost\/[a-z0-9]+(?:-[a-z0-9]+)*$/; +const REMOTE = /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/; +const HOST = /^[A-Za-z0-9.-]+(?::[0-9]+)?$/; +// GitHub token shapes, removed from any text that leaves this module in case a server or proxy echoed one back. +const TOKEN = /\b(?:gh[pousr]_[A-Za-z0-9]{20,}|github_pat_[A-Za-z0-9_]{20,})\b/g; +/** `gh` acts as Git's only credential helper. The empty value first clears any helper set before it. */ +export const CREDENTIAL_HELPER = ['-c', 'credential.helper=', '-c', 'credential.helper=!gh auth git-credential'] as const; + +/** The environment of the push: Git's hardened one plus what `gh` needs to authenticate. Nothing else. */ +export function pushEnvironment(source: NodeJS.ProcessEnv = process.env): NodeJS.ProcessEnv { + // gitEnvironment last: its config isolation (GIT_CONFIG_GLOBAL, GIT_CONFIG_NOSYSTEM) wins over anything from gh's list. + return { ...ghEnvironment(source), ...gitEnvironment(), PATH: source.PATH }; +} + +export function pushUrl(remote: string, env: NodeJS.ProcessEnv = process.env): string { + if (!REMOTE.test(remote) || remote.split('/').some(part => part === '.' || part === '..')) throw new Error('The repository must be owner/name.'); + const host = env.GH_HOST || 'github.com'; + if (!HOST.test(host)) throw new Error('GH_HOST is not a host name.'); + return `https://${host}/${remote}.git`; +} + +/** + * The error for a failed Git call: one line, at most 400 characters of Git's output, with any token removed. `staleLease` + * is read from the whole output, so a long message cannot hide the push's verdict. + */ +export function gitFailure(command: string, status: number | null, output: string, fallback = ''): Error & { staleLease: boolean; status: number | null } { + // A call that did not run to completion (timeout, kill) keeps its cause beside whatever Git printed. + const detail = [output.slice(0, 400), status === null ? fallback : ''].filter(Boolean).join('\n').replace(TOKEN, '[token]'); + // GitHub's refusal of a push that changes a workflow without the workflow scope. + const hint = command === 'push' && /refusing to allow [^\n]* to create or update workflow/i.test(output) + ? ' The push changes .github/workflows, so the GitHub token needs the workflow scope.' : ''; + // Git's message can quote a path; JSON keeps it one line of printable text. + return Object.assign(new Error(`git ${command} failed${status === null ? '' : ` (exit ${status})`}: ${JSON.stringify(detail)}${hint}`), + { staleLease: /\[rejected\][^\n]*\(stale info\)/.test(output), status }); +} + +export class GitBranchPusher implements BranchPusher { + readonly #config: GitBranchPusherConfig; + readonly #url: string; + + constructor(config: GitBranchPusherConfig) { + const url = config.url ?? pushUrl(config.remote, config.env); + if (!url.startsWith('https://') && !url.startsWith('/')) throw new Error('The push URL must be https:// or a local path.'); + this.#config = config; this.#url = url; + } + + async push(identity: PlanIdentity, input: { head: string; branch: string }, signal?: AbortSignal): Promise { + signal?.throwIfAborted(); + if (identity.repositoryId !== this.#config.repositoryId) throw new Error('The task belongs to another repository.'); + if (!COMMIT_ID.test(input.head)) throw new Error('A full commit ID is required.'); + if (!BRANCH.test(input.branch) || input.branch.length > 255) throw new Error('Only a codeboost/ task branch can be pushed.'); + const ref = `refs/heads/${input.branch}`; + try { await this.#git(['cat-file', '-e', `${input.head}^{commit}`], signal); } + catch (error) { + // Only a check that ran to completion proves the commit is missing; a timeout or a kill keeps its own error. + if (typeof (error as { status?: unknown }).status !== 'number') throw error; + throw new Error(`The runner repository has no commit ${input.head}.`); + } + const remote = await this.#read(ref, signal); + // Already there: a push whose outcome was lost, or a retry. Nothing to do. + if (remote === input.head) return; + const owned = new Set(this.#config.ownedCommits(identity)); + if (remote !== null && !owned.has(remote)) + throw new BranchPushRefused(`The branch ${input.branch} holds commit ${remote}, which codeboost did not make. Nothing was pushed.`); + signal?.throwIfAborted(); + // The lease pins the exact value read above (empty: the branch must not exist), so GitHub refuses the push if anyone + // moved the branch since. Only this one ref is sent: no tags, no hooks, no submodules. + try { + await this.#git(['push', '--porcelain', '--no-verify', '--no-follow-tags', '--recurse-submodules=no', + `--force-with-lease=${ref}:${remote ?? ''}`, '--', this.#url, `${input.head}:${ref}`], signal, true); + } catch (error) { + if ((error as { staleLease?: boolean }).staleLease) + throw new BranchPushRefused(`The branch ${input.branch} moved while codeboost pushed it. Nothing was pushed.`); + throw error; + } + // A push that exits 0 does not prove the branch moved: read it back. + const after = await this.#read(ref, signal); + if (after !== input.head) throw new Error(`After the push, the branch ${input.branch} is at ${after ?? 'nothing'}, not ${input.head}.`); + } + + /** The commit `ref` points at on the remote, or null when it does not exist. */ + async #read(ref: string, signal?: AbortSignal): Promise { + const out = await this.#git(['ls-remote', '--refs', '--', this.#url, ref], signal, true); + // ls-remote matches patterns by their tail, so `refs/heads/x/` would match too: keep only the exact name. + const matches = out.split('\n').filter(Boolean).map(line => line.split('\t')).filter(([, name]) => name === ref); + if (matches.length > 1 || (matches[0] && !COMMIT_ID.test(matches[0][0]!))) throw new Error(`The remote's answer for ${ref} is malformed.`); + return matches[0]?.[0] ?? null; + } + + async #git(args: readonly string[], signal?: AbortSignal, remote = false): Promise { + const protocol = this.#url.startsWith('/') ? 'file' : 'https'; + const outcome = await runInProcessGroup('git', [...GIT_OPTIONS, '-c', 'gc.auto=0', '-c', 'maintenance.auto=false', + ...remote ? ['-c', `protocol.${protocol}.allow=always`, ...CREDENTIAL_HELPER] : [], ...args], + { cwd: this.#config.repository.path, env: pushEnvironment(this.#config.env), timeoutMs: this.#config.timeoutMs ?? 120_000, + signal, onProcessGroup: this.#config.onProcessGroup, maxBuffer: 1024 * 1024 }); + // Aborted before or while Git ran: the caller's reason (such as shutdown) is what leaves, not Git's failure. + signal?.throwIfAborted(); + if (outcome.status !== 0) throw gitFailure(args[0]!, outcome.status, [outcome.stderr.trim(), outcome.stdout.trim()].filter(Boolean).join('\n'), + outcome.error?.message); + return outcome.stdout.trim(); + } +} diff --git a/runner/publish.ts b/runner/publish.ts index 83aa9ff3..4b7319ff 100644 --- a/runner/publish.ts +++ b/runner/publish.ts @@ -8,7 +8,7 @@ import type { Store, TaskPullRequest } from './store.ts'; /** * F2d: the pre-PR "already fixed" check and PR opening (design, "Checking whether the issue is already fixed" and - * "Needs human"). Pushing the task head to its branch is D's export plus a runner push; until that exists it is injected. + * "Needs human"). The push of the task head to its branch is injected; `GitBranchPusher` (`branch-push.ts`) is the real one. */ export interface BranchPusher { /** Makes `refs/heads/` on GitHub point at `head`. Settles only when the push finished or failed. */ diff --git a/test/runner-branch-push.test.ts b/test/runner-branch-push.test.ts new file mode 100644 index 00000000..46cf8132 --- /dev/null +++ b/test/runner-branch-push.test.ts @@ -0,0 +1,245 @@ +import { randomUUID } from 'node:crypto'; +import { chmodSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { execFileSync } from 'node:child_process'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, describe, expect, it } from 'vitest'; +import { fixtureGit as git } from './fixtures/git.ts'; +import type { PlanIdentity } from '../core/identity.ts'; +import { GH_ENV_ALLOWLIST } from '../github/gh-env.ts'; +import { BranchPushRefused, CREDENTIAL_HELPER, GitBranchPusher, gitFailure, pushEnvironment, pushUrl } from '../runner/branch-push.ts'; +import { ensureCommit, fetchTaskCommit, openRunnerRepository, type RunnerRepository } from '../runner/runner-repository.ts'; + +const OWNER = '0123456789abcdef0123456789abcdef'; +const IDENTITY: PlanIdentity = { repositoryId: 'repo-1', taskId: 'task-1', planId: 'plan-1' }; +const BRANCH = 'codeboost/issue-7-task-1-0123456789abcdef', REF = `refs/heads/${BRANCH}`; +const roots: string[] = []; +afterEach(() => { for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }); }); + +async function setup() { + const root = mkdtempSync(join(tmpdir(), 'branch-push-')); roots.push(root); + const source = join(root, 'source'), remote = join(root, 'remote.git'); + git(root, 'init', '-q', source); git(source, 'config', 'user.name', 'T'); git(source, 'config', 'user.email', 't@e'); + writeFileSync(join(source, 'a.txt'), 'a\n'); git(source, 'add', '.'); git(source, 'commit', '-qm', 'base'); + git(source, 'tag', 'v1'); + const base = git(source, 'rev-parse', 'HEAD'); + git(root, 'init', '-q', '--bare', remote); + const repository = await openRunnerRepository({ runnerRoot: join(root, 'runner'), runnerOwner: OWNER, repositoryId: 'repo-1', source }); + await ensureCommit(repository, base); + return { root, source, remote, base, repository }; +} +type Setup = Awaited>; + +/** A runner commit on `base`, taken into the runner repository the way execution does. */ +async function runnerCommit(s: Setup, name: string): Promise { + const work = join(s.root, `work-${randomUUID()}`); + git(s.root, '-c', 'protocol.file.allow=always', 'clone', '-q', s.source, work); + git(work, 'config', 'user.name', 'R'); git(work, 'config', 'user.email', 'r@e'); + writeFileSync(join(work, `${name}.txt`), `${name}\n`); git(work, 'add', '.'); git(work, 'commit', '-qm', name); + git(work, 'update-ref', 'refs/heads/codeboost', 'HEAD'); + const file = join(work, 'out.bundle'); + git(work, 'bundle', 'create', '-q', file, 'refs/heads/codeboost', `^${s.base}`); + const head = git(work, 'rev-parse', 'HEAD'); + await fetchTaskCommit(s.repository, { bundle: readFileSync(file), base: s.base, head, attemptId: randomUUID() }); + return head; +} + +function pusher(s: Setup, options: { owned?: (identity: PlanIdentity) => Iterable; env?: NodeJS.ProcessEnv; + repository?: RunnerRepository; onProcessGroup?: (group: { pgid: number }) => void } = {}) { + const spawns: number[] = []; + const instance = new GitBranchPusher({ repository: options.repository ?? s.repository, repositoryId: 'repo-1', remote: 'acme/app', + url: s.remote, env: options.env ?? process.env, ownedCommits: options.owned ?? (() => []), + onProcessGroup: group => { spawns.push(group.pgid); options.onProcessGroup?.(group); } }); + return { instance, spawns }; +} +/** A person pushes the base commit to `ref` on the remote. */ +const personPushes = (s: Setup, ref: string) => git(s.source, '-c', 'protocol.file.allow=always', 'push', '-qf', s.remote, `${s.base}:${ref}`); +const remoteRefs = (s: Setup) => git(s.remote, 'for-each-ref', '--format=%(objectname) %(refname)'); + +describe('GitBranchPusher', () => { + it('creates the branch at the head, sending only that ref, and never writes the user\'s repository', async () => { + const s = await setup(), head = await runnerCommit(s, 'one'), sourceRefs = git(s.source, 'for-each-ref'); + await pusher(s).instance.push(IDENTITY, { head, branch: BRANCH }); + // No tags, no attempt refs, nothing else. + expect(remoteRefs(s)).toBe(`${head} ${REF}`); + expect(git(s.source, 'for-each-ref')).toBe(sourceRefs); + }); + + it('does nothing when the branch is already at the head, as after a push whose outcome was lost', async () => { + const s = await setup(), head = await runnerCommit(s, 'one'); + await pusher(s).instance.push(IDENTITY, { head, branch: BRANCH }); + const again = pusher(s, { owned: () => { throw new Error('the ledger is not read'); } }); + await again.instance.push(IDENTITY, { head, branch: BRANCH }); + // The commit check and one read of the remote; no push. + expect(again.spawns).toHaveLength(2); + expect(remoteRefs(s)).toBe(`${head} ${REF}`); + }); + + it('moves the branch over a commit codeboost made, even when the new head does not descend from it', async () => { + const s = await setup(), first = await runnerCommit(s, 'one'), second = await runnerCommit(s, 'two'); + await pusher(s).instance.push(IDENTITY, { head: first, branch: BRANCH }); + const seen: PlanIdentity[] = []; + await pusher(s, { owned: identity => { seen.push(identity); return [first]; } }).instance.push(IDENTITY, { head: second, branch: BRANCH }); + expect(seen).toEqual([IDENTITY]); + expect(remoteRefs(s)).toBe(`${second} ${REF}`); + }); + + it('refuses a branch that holds a commit codeboost did not make, and pushes nothing', async () => { + const s = await setup(), head = await runnerCommit(s, 'one'), mine = await runnerCommit(s, 'two'); + personPushes(s, REF); + const p = pusher(s, { owned: () => [mine] }); + await expect(p.instance.push(IDENTITY, { head, branch: BRANCH })).rejects.toThrow(BranchPushRefused); + await expect(p.instance.push(IDENTITY, { head, branch: BRANCH })).rejects.toThrow(`holds commit ${s.base}, which codeboost did not make`); + expect(remoteRefs(s)).toBe(`${s.base} ${REF}`); + }); + + it('refuses when the branch moves between the read and the push: the lease is the value read', async () => { + const s = await setup(), first = await runnerCommit(s, 'one'), second = await runnerCommit(s, 'two'); + await pusher(s).instance.push(IDENTITY, { head: first, branch: BRANCH }); + // A person pushes while codeboost reads the ledger, after it read the remote. + const moved = pusher(s, { owned: () => { git(s.remote, 'update-ref', REF, s.base); return [first]; } }); + await expect(moved.instance.push(IDENTITY, { head: second, branch: BRANCH })).rejects.toThrow(`The branch ${BRANCH} moved while codeboost pushed it`); + expect(remoteRefs(s)).toBe(`${s.base} ${REF}`); + }); + + it('refuses when a branch that did not exist is created between the read and the push', async () => { + const s = await setup(), head = await runnerCommit(s, 'one'); + const created = pusher(s, { owned: () => { personPushes(s, REF); return []; } }); + await expect(created.instance.push(IDENTITY, { head, branch: BRANCH })).rejects.toThrow(BranchPushRefused); + expect(remoteRefs(s)).toBe(`${s.base} ${REF}`); + }); + + it('reads only the exact branch, not a longer ref that ends with its name', async () => { + const s = await setup(), head = await runnerCommit(s, 'one'); + personPushes(s, `refs/heads/other/${REF}`); + await pusher(s).instance.push(IDENTITY, { head, branch: BRANCH }); + expect(git(s.remote, 'rev-parse', REF)).toBe(head); + }); + + it('refuses another repository\'s task, a branch outside codeboost/, and a head the runner repository lacks', async () => { + const s = await setup(), head = await runnerCommit(s, 'one'), { instance, spawns } = pusher(s); + await expect(instance.push({ ...IDENTITY, repositoryId: 'repo-2' }, { head, branch: BRANCH })).rejects.toThrow('another repository'); + for (const branch of ['main', 'codeboost/../main', 'codeboost/x/y', 'refs/heads/codeboost/x', 'codeboost/x.lock', 'codeboost/']) + await expect(instance.push(IDENTITY, { head, branch })).rejects.toThrow('Only a codeboost/ task branch'); + await expect(instance.push(IDENTITY, { head: 'HEAD', branch: BRANCH })).rejects.toThrow('full commit ID'); + expect(spawns).toHaveLength(0); + await expect(instance.push(IDENTITY, { head: 'e'.repeat(40), branch: BRANCH })).rejects.toThrow(`has no commit ${'e'.repeat(40)}`); + expect(remoteRefs(s)).toBe(''); + }); + + it('ignores Git configuration from the environment it is given', async () => { + const s = await setup(), head = await runnerCommit(s, 'one'), elsewhere = join(s.root, 'elsewhere.git'); + git(s.root, 'init', '-q', '--bare', elsewhere); + // If these reached Git, the push would go to `elsewhere` instead. + const env = { ...process.env, GIT_CONFIG_COUNT: '1', GIT_CONFIG_KEY_0: `url.${elsewhere}.insteadOf`, GIT_CONFIG_VALUE_0: s.remote, + GIT_CONFIG_PARAMETERS: `'url.${elsewhere}.insteadof'='${s.remote}'` }; + await pusher(s, { env }).instance.push(IDENTITY, { head, branch: BRANCH }); + expect(remoteRefs(s)).toBe(`${head} ${REF}`); + expect(git(elsewhere, 'for-each-ref')).toBe(''); + }); + + it('settles a cancelled push only after Git has exited, and pushes nothing more', async () => { + const s = await setup(), head = await runnerCommit(s, 'one'), abort = new AbortController(); + let pushGroup: number | undefined, calls = 0; + // The third Git call is the push: the commit check and the read come first. + const p = pusher(s, { onProcessGroup: group => { if (++calls === 3) { pushGroup = group.pgid; abort.abort(); } } }); + await expect(p.instance.push(IDENTITY, { head, branch: BRANCH }, abort.signal)).rejects.toMatchObject({ name: 'AbortError' }); + expect(pushGroup).toBeDefined(); + expect(() => process.kill(-pushGroup!, 0)).toThrow(expect.objectContaining({ code: 'ESRCH' })); + expect(calls).toBe(3); + }); + + it('keeps the caller\'s abort reason, whether the abort lands during a Git call or between two', async () => { + const s = await setup(), head = await runnerCommit(s, 'one'); + class Closing extends Error {} + for (const at of [1, 2, 3]) { + const abort = new AbortController(); + let calls = 0; + const p = pusher(s, { onProcessGroup: () => { if (++calls === at) abort.abort(new Closing(`closing at ${at}`)); } }); + await expect(p.instance.push(IDENTITY, { head, branch: BRANCH }, abort.signal)).rejects.toThrow(new Closing(`closing at ${at}`)); + expect(calls).toBe(at); + } + expect(remoteRefs(s)).toBe(''); + }); + + it('gives every remote Git call gh as its credential helper, with no token in any argument or extra variable', async () => { + const s = await setup(), head = await runnerCommit(s, 'one'), bin = join(s.root, 'bin'), log = join(s.root, 'calls.log'); + const token = `ghp_${'B'.repeat(36)}`; + // A Git that records its arguments and environment names, then runs the real Git. + const real = execFileSync('/bin/sh', ['-c', 'command -v git'], { encoding: 'utf8' }).trim(); + mkdirSync(bin); + writeFileSync(join(bin, 'git'), `#!/bin/sh\n{ printf 'ARGS'; printf ' %s' "$@"; printf '\\nENV'; env | sed 's/=.*//' | sort | tr '\\n' ' '; printf '\\n'; } >> '${log}'\nexec '${real}' "$@"\n`); + chmodSync(join(bin, 'git'), 0o755); + const env = { PATH: `${bin}:${process.env.PATH}`, HOME: s.root, GH_TOKEN: token, AWS_SECRET_ACCESS_KEY: 'k', GIT_ASKPASS: '/x' }; + await pusher(s, { env }).instance.push(IDENTITY, { head, branch: BRANCH }); + const lines = readFileSync(log, 'utf8').trim().split('\n'); + const args = lines.filter(line => line.startsWith('ARGS')), names = lines.filter(line => line.startsWith('ENV')); + expect(args.map(line => line.split(' ').find(word => ['cat-file', 'ls-remote', 'push'].includes(word)))).toEqual(['cat-file', 'ls-remote', 'push', 'ls-remote']); + for (const line of args.filter(line => / (ls-remote|push) /.test(line))) expect(line).toContain(CREDENTIAL_HELPER.join(' ')); + expect(lines.join('\n')).not.toContain(token); + for (const line of names) { + expect(line).not.toMatch(/\b(AWS_SECRET_ACCESS_KEY|GIT_ASKPASS)\b/); + expect(line).toMatch(/\bGH_TOKEN\b/); + } + expect(remoteRefs(s)).toBe(`${head} ${REF}`); + }); + + it('requires the branch to be at the head after a push that exited 0', async () => { + const s = await setup(), head = await runnerCommit(s, 'one'), bin = join(s.root, 'bin'); + // A Git whose push reports success but changes nothing. + const real = execFileSync('/bin/sh', ['-c', 'command -v git'], { encoding: 'utf8' }).trim(); + mkdirSync(bin); writeFileSync(join(bin, 'git'), `#!/bin/sh\nfor a in "$@"; do [ "$a" = push ] && exit 0; done\nexec '${real}' "$@"\n`); + chmodSync(join(bin, 'git'), 0o755); + const p = pusher(s, { env: { ...process.env, PATH: `${bin}:${process.env.PATH}` } }); + await expect(p.instance.push(IDENTITY, { head, branch: BRANCH })).rejects.toThrow(`After the push, the branch ${BRANCH} is at nothing, not ${head}`); + }); + + it('removes a token from Git\'s error text', async () => { + const s = await setup(), head = await runnerCommit(s, 'one'), token = `ghp_${'A'.repeat(36)}`; + const missing = new GitBranchPusher({ repository: s.repository, repositoryId: 'repo-1', remote: 'acme/app', + url: join(s.root, token), ownedCommits: () => [] }); + const error = await missing.push(IDENTITY, { head, branch: BRANCH }).then(() => new Error("pushed"), (e: unknown) => e as Error); + expect(error.message).toContain('git ls-remote failed'); + expect(error.message).not.toContain(token); + expect(error.message).toContain('[token]'); + }); +}); + +describe('push invocation', () => { + it('gives Git only the gh allowlist plus its hardened environment', () => { + const env = pushEnvironment({ PATH: '/bin', HOME: '/h', GH_TOKEN: 't', GIT_ASKPASS: '/x', SSH_AUTH_SOCK: '/s', + GIT_CONFIG_GLOBAL: '/home/.gitconfig', GIT_CONFIG_PARAMETERS: 'x', AWS_SECRET_ACCESS_KEY: 'k' }); + const git = ['GIT_CONFIG_NOSYSTEM', 'GIT_CONFIG_GLOBAL', 'GIT_TERMINAL_PROMPT', 'GIT_NO_LAZY_FETCH', 'GIT_GRAFT_FILE']; + const ghSet = ['GH_PROMPT_DISABLED', 'GH_NO_UPDATE_NOTIFIER', 'GH_PAGER', 'NO_COLOR']; + for (const name of Object.keys(env)) expect([...GH_ENV_ALLOWLIST, ...git, ...ghSet]).toContain(name); + expect(env).toMatchObject({ PATH: '/bin', HOME: '/h', GH_TOKEN: 't', GIT_CONFIG_GLOBAL: '/dev/null', GIT_CONFIG_NOSYSTEM: '1', GIT_TERMINAL_PROMPT: '0' }); + expect(env).not.toHaveProperty('GIT_ASKPASS'); + expect(env).not.toHaveProperty('GIT_CONFIG_PARAMETERS'); + }); + + it('uses gh as the only credential helper, with no secret in the arguments', () => { + expect(CREDENTIAL_HELPER).toEqual(['-c', 'credential.helper=', '-c', 'credential.helper=!gh auth git-credential']); + }); + + it('pushes to the repository on GitHub, or on GH_HOST', () => { + expect(pushUrl('acme/app', {})).toBe('https://github.com/acme/app.git'); + expect(pushUrl('acme/app', { GH_HOST: 'ghe.example.com' })).toBe('https://ghe.example.com/acme/app.git'); + for (const remote of ['acme', 'acme/app/x', '../app', 'acme/..', 'acme/a b']) expect(() => pushUrl(remote, {})).toThrow('owner/name'); + expect(() => pushUrl('acme/app', { GH_HOST: 'evil.com/x?' })).toThrow('GH_HOST'); + }); + + it('describes a failure in one line, names the workflow scope, and finds a stale lease in long output', () => { + const workflow = gitFailure('push', 1, 'refusing to allow an OAuth App to create or update workflow `.github/workflows/ci.yml` without `workflow` scope'); + expect(workflow.message).toContain('needs the workflow scope'); + expect(workflow.staleLease).toBe(false); + const long = gitFailure('push', 1, `${'x'.repeat(1000)}\nline two\n!\tabc:${REF}\t[rejected] (stale info)`); + expect(long.staleLease).toBe(true); + expect(long.message).not.toContain('\n'); + expect(gitFailure('push', 1, '!\tabc:refs/heads/x\t[remote rejected] (protected branch hook declined)').staleLease).toBe(false); + // A name that merely contains the word is not GitHub's refusal, and only a push can be refused for it. + expect(gitFailure('ls-remote', 128, "fatal: unable to access 'https://github.com/acme/workflow-engine.git/'").message).not.toContain('workflow scope'); + expect(gitFailure('ls-remote', 128, 'refusing to allow an OAuth App to create or update workflow').message).not.toContain('workflow scope'); + // A call that was killed keeps its cause beside Git's output. + expect(gitFailure('push', null, 'partial output', 'git timed out after 120000 ms').message).toContain('timed out'); + }); +}); From dd67abdd392397ab641d4c31f264801702d64d3a Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 1 Oct 2026 22:57:47 -0700 Subject: [PATCH 2/4] Redact configured tokens before cutting; check commit presence structurally (Copilot round 1) - Git's error text is redacted before it is cut to 400 characters, and the exact GH_TOKEN, GITHUB_TOKEN and enterprise token values are removed as well as known token shapes. The shape pattern no longer needs word boundaries, so a token glued to a path or URL is still removed. - The head check uses cat-file --batch-check: only Git's "missing" answer means the commit is absent; a broken repository keeps Git's own error. Co-Authored-By: Claude Opus 5.5 --- runner/branch-push.ts | 35 +++++++++++++++++++++------------ test/runner-branch-push.test.ts | 28 +++++++++++++++++++++++++- 2 files changed, 49 insertions(+), 14 deletions(-) diff --git a/runner/branch-push.ts b/runner/branch-push.ts index d28ef81c..3beb0023 100644 --- a/runner/branch-push.ts +++ b/runner/branch-push.ts @@ -37,7 +37,9 @@ const BRANCH = /^codeboost\/[a-z0-9]+(?:-[a-z0-9]+)*$/; const REMOTE = /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/; const HOST = /^[A-Za-z0-9.-]+(?::[0-9]+)?$/; // GitHub token shapes, removed from any text that leaves this module in case a server or proxy echoed one back. -const TOKEN = /\b(?:gh[pousr]_[A-Za-z0-9]{20,}|github_pat_[A-Za-z0-9_]{20,})\b/g; +const TOKEN_VARIABLES = ['GH_TOKEN', 'GITHUB_TOKEN', 'GH_ENTERPRISE_TOKEN', 'GITHUB_ENTERPRISE_TOKEN'] as const; +// No word boundaries: a token glued to other text (a URL, a path) is still removed. +const TOKEN = /(?:gh[pousr]_[A-Za-z0-9]{20,}|github_pat_[A-Za-z0-9_]{20,})/g; /** `gh` acts as Git's only credential helper. The empty value first clears any helper set before it. */ export const CREDENTIAL_HELPER = ['-c', 'credential.helper=', '-c', 'credential.helper=!gh auth git-credential'] as const; @@ -54,13 +56,22 @@ export function pushUrl(remote: string, env: NodeJS.ProcessEnv = process.env): s return `https://${host}/${remote}.git`; } +/** Remove every configured token value and every known token shape. */ +export function redact(text: string, secrets: readonly string[]): string { + // Longest first, so a token that contains another is removed whole. + for (const secret of [...secrets].filter(value => value.length >= 8).sort((a, b) => b.length - a.length)) text = text.split(secret).join('[token]'); + return text.replace(TOKEN, '[token]'); +} + /** * The error for a failed Git call: one line, at most 400 characters of Git's output, with any token removed. `staleLease` * is read from the whole output, so a long message cannot hide the push's verdict. */ -export function gitFailure(command: string, status: number | null, output: string, fallback = ''): Error & { staleLease: boolean; status: number | null } { - // A call that did not run to completion (timeout, kill) keeps its cause beside whatever Git printed. - const detail = [output.slice(0, 400), status === null ? fallback : ''].filter(Boolean).join('\n').replace(TOKEN, '[token]'); +export function gitFailure(command: string, status: number | null, output: string, fallback = '', secrets: readonly string[] = []): + Error & { staleLease: boolean; status: number | null } { + // Redacted before it is cut, so a token across the cut cannot leave half of itself. The exact configured tokens go too: + // an enterprise token need not have a known shape. A call that did not run to completion keeps its cause. + const detail = redact([output, status === null ? fallback : ''].filter(Boolean).join('\n'), secrets).slice(0, 400); // GitHub's refusal of a push that changes a workflow without the workflow scope. const hint = command === 'push' && /refusing to allow [^\n]* to create or update workflow/i.test(output) ? ' The push changes .github/workflows, so the GitHub token needs the workflow scope.' : ''; @@ -85,12 +96,10 @@ export class GitBranchPusher implements BranchPusher { if (!COMMIT_ID.test(input.head)) throw new Error('A full commit ID is required.'); if (!BRANCH.test(input.branch) || input.branch.length > 255) throw new Error('Only a codeboost/ task branch can be pushed.'); const ref = `refs/heads/${input.branch}`; - try { await this.#git(['cat-file', '-e', `${input.head}^{commit}`], signal); } - catch (error) { - // Only a check that ran to completion proves the commit is missing; a timeout or a kill keeps its own error. - if (typeof (error as { status?: unknown }).status !== 'number') throw error; - throw new Error(`The runner repository has no commit ${input.head}.`); - } + // Git answers " missing" for an absent object; any other failure (no repository, permissions) keeps Git's error. + const found = await this.#git(['cat-file', '--batch-check=%(objectname) %(objecttype)'], signal, false, Buffer.from(`${input.head}\n`)); + if (found === `${input.head} missing`) throw new Error(`The runner repository has no commit ${input.head}.`); + if (found !== `${input.head} commit`) throw new Error(`${input.head} is not a commit in the runner repository.`); const remote = await this.#read(ref, signal); // Already there: a push whose outcome was lost, or a retry. Nothing to do. if (remote === input.head) return; @@ -122,16 +131,16 @@ export class GitBranchPusher implements BranchPusher { return matches[0]?.[0] ?? null; } - async #git(args: readonly string[], signal?: AbortSignal, remote = false): Promise { + async #git(args: readonly string[], signal?: AbortSignal, remote = false, input?: Buffer): Promise { const protocol = this.#url.startsWith('/') ? 'file' : 'https'; const outcome = await runInProcessGroup('git', [...GIT_OPTIONS, '-c', 'gc.auto=0', '-c', 'maintenance.auto=false', ...remote ? ['-c', `protocol.${protocol}.allow=always`, ...CREDENTIAL_HELPER] : [], ...args], { cwd: this.#config.repository.path, env: pushEnvironment(this.#config.env), timeoutMs: this.#config.timeoutMs ?? 120_000, - signal, onProcessGroup: this.#config.onProcessGroup, maxBuffer: 1024 * 1024 }); + signal, onProcessGroup: this.#config.onProcessGroup, maxBuffer: 1024 * 1024, input }); // Aborted before or while Git ran: the caller's reason (such as shutdown) is what leaves, not Git's failure. signal?.throwIfAborted(); if (outcome.status !== 0) throw gitFailure(args[0]!, outcome.status, [outcome.stderr.trim(), outcome.stdout.trim()].filter(Boolean).join('\n'), - outcome.error?.message); + outcome.error?.message, TOKEN_VARIABLES.flatMap(name => (this.#config.env ?? process.env)[name] ?? [])); return outcome.stdout.trim(); } } diff --git a/test/runner-branch-push.test.ts b/test/runner-branch-push.test.ts index 46cf8132..69bc7fdf 100644 --- a/test/runner-branch-push.test.ts +++ b/test/runner-branch-push.test.ts @@ -7,7 +7,7 @@ import { afterEach, describe, expect, it } from 'vitest'; import { fixtureGit as git } from './fixtures/git.ts'; import type { PlanIdentity } from '../core/identity.ts'; import { GH_ENV_ALLOWLIST } from '../github/gh-env.ts'; -import { BranchPushRefused, CREDENTIAL_HELPER, GitBranchPusher, gitFailure, pushEnvironment, pushUrl } from '../runner/branch-push.ts'; +import { BranchPushRefused, CREDENTIAL_HELPER, GitBranchPusher, gitFailure, pushEnvironment, pushUrl, redact } from '../runner/branch-push.ts'; import { ensureCommit, fetchTaskCommit, openRunnerRepository, type RunnerRepository } from '../runner/runner-repository.ts'; const OWNER = '0123456789abcdef0123456789abcdef'; @@ -194,6 +194,26 @@ describe('GitBranchPusher', () => { await expect(p.instance.push(IDENTITY, { head, branch: BRANCH })).rejects.toThrow(`After the push, the branch ${BRANCH} is at nothing, not ${head}`); }); + it('reports a broken runner repository as itself, not as a missing commit', async () => { + const s = await setup(), head = await runnerCommit(s, 'one'), broken = join(s.root, 'not-a-repository'); + mkdirSync(broken); + const error = await pusher(s, { repository: { path: broken, source: s.source } }).instance.push(IDENTITY, { head, branch: BRANCH }) + .then(() => new Error('pushed'), (e: unknown) => e as Error); + expect(error.message).toContain('git cat-file failed'); + expect(error.message).not.toContain('has no commit'); + expect(remoteRefs(s)).toBe(''); + }); + + it('removes a configured enterprise token of any shape from Git\'s error text', async () => { + const s = await setup(), head = await runnerCommit(s, 'one'), token = 'enterprise-secret-0123456789'; + const missing = new GitBranchPusher({ repository: s.repository, repositoryId: 'repo-1', remote: 'acme/app', + url: join(s.root, token), ownedCommits: () => [], env: { ...process.env, GH_ENTERPRISE_TOKEN: token } }); + const error = await missing.push(IDENTITY, { head, branch: BRANCH }).then(() => new Error('pushed'), (e: unknown) => e as Error); + expect(error.message).toContain('git ls-remote failed'); + expect(error.message).not.toContain(token); + expect(error.message).not.toContain('0123456789'); + }); + it('removes a token from Git\'s error text', async () => { const s = await setup(), head = await runnerCommit(s, 'one'), token = `ghp_${'A'.repeat(36)}`; const missing = new GitBranchPusher({ repository: s.repository, repositoryId: 'repo-1', remote: 'acme/app', @@ -239,6 +259,12 @@ describe('push invocation', () => { // A name that merely contains the word is not GitHub's refusal, and only a push can be refused for it. expect(gitFailure('ls-remote', 128, "fatal: unable to access 'https://github.com/acme/workflow-engine.git/'").message).not.toContain('workflow scope'); expect(gitFailure('ls-remote', 128, 'refusing to allow an OAuth App to create or update workflow').message).not.toContain('workflow scope'); + // A token across the 400-character cut leaves no part of itself. + const token = `ghp_${'C'.repeat(36)}`; + const cut = gitFailure('push', 1, `${'x'.repeat(390)}${token}`); + expect(cut.message).not.toContain('ghp_'); + expect(cut.message).toContain('[token]'); + expect(redact(`a ${token} b secret-value-1 c`, ['secret-value-1', 'short'])).toBe('a [token] b [token] c'); // A call that was killed keeps its cause beside Git's output. expect(gitFailure('push', null, 'partial output', 'git timed out after 120000 ms').message).toContain('timed out'); }); From 7000ebc9f76fded0137b9dff1762700291657fd2 Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 1 Oct 2026 23:14:38 -0700 Subject: [PATCH 3/4] Place the token comments beside what they describe (self-review) Co-Authored-By: Claude Opus 5.5 --- runner/branch-push.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/runner/branch-push.ts b/runner/branch-push.ts index 3beb0023..54be862c 100644 --- a/runner/branch-push.ts +++ b/runner/branch-push.ts @@ -36,9 +36,10 @@ const COMMIT_ID = /^(?:[0-9a-f]{40}|[0-9a-f]{64})$/; const BRANCH = /^codeboost\/[a-z0-9]+(?:-[a-z0-9]+)*$/; const REMOTE = /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/; const HOST = /^[A-Za-z0-9.-]+(?::[0-9]+)?$/; -// GitHub token shapes, removed from any text that leaves this module in case a server or proxy echoed one back. +// The variables whose values `gh` may authenticate with; their exact values are removed from error text. const TOKEN_VARIABLES = ['GH_TOKEN', 'GITHUB_TOKEN', 'GH_ENTERPRISE_TOKEN', 'GITHUB_ENTERPRISE_TOKEN'] as const; -// No word boundaries: a token glued to other text (a URL, a path) is still removed. +// GitHub token shapes, removed from any text that leaves this module in case a server or proxy echoed one back. No word +// boundaries: a token glued to other text (a URL, a path) is still removed. const TOKEN = /(?:gh[pousr]_[A-Za-z0-9]{20,}|github_pat_[A-Za-z0-9_]{20,})/g; /** `gh` acts as Git's only credential helper. The empty value first clears any helper set before it. */ export const CREDENTIAL_HELPER = ['-c', 'credential.helper=', '-c', 'credential.helper=!gh auth git-credential'] as const; @@ -58,7 +59,8 @@ export function pushUrl(remote: string, env: NodeJS.ProcessEnv = process.env): s /** Remove every configured token value and every known token shape. */ export function redact(text: string, secrets: readonly string[]): string { - // Longest first, so a token that contains another is removed whole. + // Longest first, so a token that contains another is removed whole. A value under 8 characters is not a real token, + // and removing it would garble ordinary words in the message. for (const secret of [...secrets].filter(value => value.length >= 8).sort((a, b) => b.length - a.length)) text = text.split(secret).join('[token]'); return text.replace(TOKEN, '[token]'); } From 530018bf7c5bee95e35bc7d3ef66fdfd73e80631 Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 1 Oct 2026 23:20:44 -0700 Subject: [PATCH 4/4] Keep a killed call's cause ahead of long Git output (Copilot round 2) Round 1 put the cause after Git's output before the 400-character cut, so 400 characters of output dropped it again. The cause now comes first, with a long-output regression. Co-Authored-By: Claude Opus 5.5 --- runner/branch-push.ts | 3 ++- test/runner-branch-push.test.ts | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/runner/branch-push.ts b/runner/branch-push.ts index 54be862c..7561ba0a 100644 --- a/runner/branch-push.ts +++ b/runner/branch-push.ts @@ -73,7 +73,8 @@ export function gitFailure(command: string, status: number | null, output: strin Error & { staleLease: boolean; status: number | null } { // Redacted before it is cut, so a token across the cut cannot leave half of itself. The exact configured tokens go too: // an enterprise token need not have a known shape. A call that did not run to completion keeps its cause. - const detail = redact([output, status === null ? fallback : ''].filter(Boolean).join('\n'), secrets).slice(0, 400); + // The cause comes first, so Git's output, however long, cannot push it past the cut. + const detail = redact([status === null ? fallback : '', output].filter(Boolean).join('\n'), secrets).slice(0, 400); // GitHub's refusal of a push that changes a workflow without the workflow scope. const hint = command === 'push' && /refusing to allow [^\n]* to create or update workflow/i.test(output) ? ' The push changes .github/workflows, so the GitHub token needs the workflow scope.' : ''; diff --git a/test/runner-branch-push.test.ts b/test/runner-branch-push.test.ts index 69bc7fdf..ce554032 100644 --- a/test/runner-branch-push.test.ts +++ b/test/runner-branch-push.test.ts @@ -267,5 +267,6 @@ describe('push invocation', () => { expect(redact(`a ${token} b secret-value-1 c`, ['secret-value-1', 'short'])).toBe('a [token] b [token] c'); // A call that was killed keeps its cause beside Git's output. expect(gitFailure('push', null, 'partial output', 'git timed out after 120000 ms').message).toContain('timed out'); + expect(gitFailure('push', null, 'y'.repeat(1000), 'git timed out after 120000 ms').message).toContain('timed out'); }); });