From ef49160a98e5eadcfecfad928971c1a2946ef311 Mon Sep 17 00:00:00 2001 From: Ogden-2688 Date: Sat, 24 Oct 2026 14:20:07 -0400 Subject: [PATCH] update_branch: write only to a head that is nobody's base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit update_branch writes a merge commit onto a pull request's head branch, and the module header said it refuses a head that is itself a base branch. It checked only the allow-lists, so a head GitHub knows as a base but no allow-list names was still written to: on a `main → dev` pull request with `dev` the only allowed base, it asked GitHub to merge `dev` into `main`. A head in a fork was written to as well, though the README says a write outside GITHUB_REPOS is refused and delete_branch leaves a fork's branch to its owner. update_branch now refuses a head whose repository GitHub no longer reports, or that isn't the pull request's own, before it asks GitHub about the head. After the allow-list and sha checks, and before the write, it asks branchFacts about the head, as delete_branch does, and refuses a default branch, a protected branch, or another open pull request's base. If branchFacts fails, it refuses and says nothing was written. Both fences compare owner and name ignoring case, as GitHub does: a pull request's repository keeps the spelling GITHUB_REPOS gave it, while GitHub reports the head repository in its own. deletionBlockedOnGitHub is renamed baseOnGitHub, since both paths ask it. The module header, the tool's description, the BranchFacts docs, the README and the unreleased fragment say what update_branch refuses, that only merging refuses a draft, and that a moved head is refused given the caller's sha. The README and baseOnGitHub's doc say what GitHub can't tell: an unprotected branch that isn't the default and that nothing targets reads as nobody's base. Tests: update_branch refuses a `main → dev` head GitHub reports as default, protected or another open pull request's base; refuses a head in a fork, in another repository of the same owner, or in an unknown one; writes nothing when branchFacts fails; and asks before it writes. A head repository spelled in another case is the pull request's own, for update and deletion, and deletion keeps a head in another repository of the same owner. FakeGitHubApi.branchFactsError throws while it is set. --- README.md | 13 ++-- changelog.d/merge-and-actions.added.md | 9 ++- src/tools-merge.ts | 82 ++++++++++++++++++++------ src/types.ts | 8 +-- test/harness.ts | 3 + test/toolsMerge.test.ts | 78 ++++++++++++++++++++++-- 6 files changed, 158 insertions(+), 35 deletions(-) diff --git a/README.md b/README.md index db21349..0264ac7 100644 --- a/README.md +++ b/README.md @@ -361,11 +361,14 @@ Notes worth knowing before you grant them: lives in the base repository and is nobody's base: not named by an allow-list, not protected on GitHub, not the default branch, and not the base of another open pull request — so a back-merge `release → dev` keeps - `release` even though `release` is in no allow-list. + a `release` that no allow-list names, as long as GitHub protects it, it is + the default branch, or another open pull request targets it. - `update_branch` merges the base into a pull request's head branch, as GitHub's "Update branch" button does. That writes a commit, so it takes a merge's fences: an open pull request into a base `GITHUB_MERGE_BASE_ALLOW` - names, and a head that is not itself a base branch. + names, and a head branch of the pull request's own repository that is + nobody's base, judged as for `delete_branch`. A fork's branch is left to + its owner. - `dispatch_workflow` and `rerun_workflow_run` act only on workflow files `GITHUB_WORKFLOW_ALLOW` names (empty = none), and a dispatch runs only on a base branch, so the workflow that runs is one that was reviewed and merged. @@ -497,9 +500,9 @@ reach metadata and never reach the model. without. (RFC-005 references for logs and diffs are on the roadmap, not here.) - **No push.** Commits are written only by GitHub's own pull-request merge - (onto an allowed base) and "Update branch" (onto the head of a pull request - into an allowed base). No tool pushes a commit, creates a branch or touches - a branch outside a pull request. + (onto an allowed base) and "Update branch" (onto a head branch that is + nobody's base, of a pull request into an allowed base). No tool pushes a + commit, creates a branch or touches a branch outside a pull request. - **Polling is no faster than 60 s** and cannot see pushes that touch no thread. - **No reactions, no labels, no assignees, no projects, no releases.** diff --git a/changelog.d/merge-and-actions.added.md b/changelog.d/merge-and-actions.added.md index 3cea953..ea34667 100644 --- a/changelog.d/merge-and-actions.added.md +++ b/changelog.d/merge-and-actions.added.md @@ -2,9 +2,12 @@ at the head sha the caller read, optionally deleting the merged head branch when it is nobody's base) and `update_branch`. Both act only on pull requests into the bases `GITHUB_MERGE_BASE_ALLOW` names, which is empty by - default: granting the feature set alone writes nothing. Drafts, closed and - merged pull requests and a moved head are refused before GitHub is asked; - branch protection stays GitHub's. + default: granting the feature set alone writes nothing. `merge_pull_request` + refuses a draft. Both refuse a closed or merged pull request, and, given + the `sha` the caller read, a head that has moved since, before GitHub is + asked to write; `update_branch` writes only to a head branch of the pull + request's own repository that is nobody's base. + Branch protection stays GitHub's. - `github.actions` feature set: `list_workflow_runs`, `dispatch_workflow` and `rerun_workflow_run`. Dispatch and re-run act only on the workflow files `GITHUB_WORKFLOW_ALLOW` names (empty by default), and a dispatch runs only diff --git a/src/tools-merge.ts b/src/tools-merge.ts index 9b317d9..f790468 100644 --- a/src/tools-merge.ts +++ b/src/tools-merge.ts @@ -11,13 +11,14 @@ * granting github.pr: opening a pull request and landing one are different * decisions. Not rollback-capable — a merge cannot be unmerged. * - * The refusals are the point of this file, and all of them are taken before - * GitHub is asked: + * The refusals are the point of this file, and each is taken before GitHub is + * asked for the write it guards: * * - the base must be in GITHUB_MERGE_BASE_ALLOW, which is empty by default: * a deployment that never set it cannot merge anything, whatever the host * grants; - * - a draft, a closed or an already-merged pull request is refused by name; + * - a closed or an already-merged pull request is refused by name, and a + * draft is refused when merging; * - with `sha` (full, or a prefix of 7 or more hex digits), a head that has * moved since the caller read it is refused — a merge only ever lands the * commit somebody looked at (GitHub enforces the full sha again, with a @@ -27,8 +28,10 @@ * protected on GitHub, the default branch, or the base of another open * pull request; * - `update_branch` writes a merge commit onto the head branch, so it takes - * the same fences as a merge — an open pull request into an allowed base — - * and refuses a head that is itself a base branch. + * the allowed base, the open pull request and the head sha from a merge's + * fences, though not the draft, and writes only to a head branch of the + * pull request's own repository that is nobody's base, judged as for + * `delete_branch`. * * Branch protection — required checks, required approvals, linear history — * is GitHub's to enforce, and its refusal (405) comes back as it is. @@ -41,7 +44,6 @@ import { TOOL_UPDATE_BRANCH, allowedBases, repoSlug, - sameRepo, type MergeMethod, type PullRequest, type RepoRef, @@ -95,6 +97,15 @@ function notMergeable(pr: PullRequest, action: 'merging' | 'updating'): string | return null; } +/** + * Whether two references name one repository. GitHub's owner and repository + * names are case-insensitive, and a pull request's own repository keeps the + * spelling GITHUB_REPOS gave it while its head repository has GitHub's. + */ +function sameRepository(a: RepoRef, b: RepoRef): boolean { + return a.owner.toLowerCase() === b.owner.toLowerCase() && a.name.toLowerCase() === b.name.toLowerCase(); +} + /** A sha as a caller may pass one: 7 to 40 hex digits, any case. */ const SHA_PREFIX = /^[0-9a-f]{7,40}$/; @@ -134,13 +145,13 @@ function baseBranches(pr: PullRequest, ctx: ToolContext): Set { * Whether the merged head branch may be deleted, as far as the pull request * itself can tell, and if not, why not. Only a branch of the base repository * itself — a fork's branch is its owner's — and never a branch an allow-list - * names as a base. {@link deletionBlockedOnGitHub} asks GitHub the rest. + * names as a base. {@link baseOnGitHub} asks GitHub the rest. */ function branchDeletionBlocked(pr: PullRequest, ctx: ToolContext): string | null { const branch = pr.head.ref; if (!branch) return 'the pull request names no head branch'; if (!pr.headRepo) return 'GitHub no longer knows the head repository (a deleted fork?)'; - if (!sameRepo(pr.headRepo, pr.repo)) { + if (!sameRepository(pr.headRepo, pr.repo)) { return `the head branch lives in ${field(repoSlug(pr.headRepo), FIELD.slug)}, not in ${repoSlug(pr.repo)}`; } if (baseBranches(pr, ctx).has(branch)) { @@ -150,13 +161,15 @@ function branchDeletionBlocked(pr: PullRequest, ctx: ToolContext): string | null } /** - * The part of "is this somebody's base?" only GitHub knows. A release branch - * kept *out* of the allow-lists — so that nothing here can target it — is - * still a base: a back-merge `release → dev` must not delete `release`. So a - * protected branch, the default branch, and a branch other open pull requests - * target are all kept. + * The part of "is this somebody's base?" only GitHub knows, and why, or null. + * A release branch kept *out* of the allow-lists — so that nothing here can + * target it — is still a base, which a back-merge `release → dev` should + * neither delete nor write into. GitHub can tell when the branch is protected, + * is the default branch, or is the base of another open pull request; an + * unprotected branch that isn't the default and that nothing targets reads as + * nobody's base. */ -function deletionBlockedOnGitHub(branch: string, facts: BranchFacts): string | null { +function baseOnGitHub(branch: string, facts: BranchFacts): string | null { if (facts.isDefault) return `${echo(branch)} is the repository's default branch`; if (facts.protected) return `${echo(branch)} is protected on GitHub`; if (facts.openPullsBasedOn > 0) return `${echo(branch)} is the base of another open pull request`; @@ -202,9 +215,10 @@ export const mergeTools: ToolDefinition[] = [ name: TOOL_UPDATE_BRANCH, description: 'Bring a pull request’s head branch up to date by merging its base into it (GitHub’s ' + - '"Update branch"). Only an open pull request into a base GITHUB_MERGE_BASE_ALLOW names, and never ' + - 'when the head is itself a base branch. GitHub does the merge in the background; the head sha ' + - 'changes when it lands. With `sha`, a head that has moved is refused.', + '"Update branch"). Only an open pull request into a base GITHUB_MERGE_BASE_ALLOW names, and only ' + + 'when the head is a branch of this repository that is nobody’s base: not allow-listed, not protected, ' + + 'not the default branch, not the base of another open pull request. GitHub does the merge in the ' + + 'background; the head sha changes when it lands. With `sha`, a head that has moved is refused.', inputSchema: { type: 'object', properties: { @@ -255,7 +269,7 @@ async function callMerge(name: string, args: Record, ctx: ToolC out.push(`head branch kept: ${why}`); } else { try { - const onGitHub = deletionBlockedOnGitHub(pr.head.ref, await ctx.api.branchFacts(pull.repo, pr.head.ref)); + const onGitHub = baseOnGitHub(pr.head.ref, await ctx.api.branchFacts(pull.repo, pr.head.ref)); if (onGitHub) { out.push(`head branch kept: ${onGitHub}`); } else { @@ -274,11 +288,23 @@ async function callMerge(name: string, args: Record, ctx: ToolC const expected = optionalString(args, 'sha'); // A merge commit onto the head branch is a write, so it takes a merge's // fences: an open pull request into an allowed base, and a head that is - // not itself a base (on `dev → main`, this would merge main into dev). + // nobody's base (on `dev → main`, this would merge main into dev). The + // head must be a branch of this repository: a write lands only in + // GITHUB_REPOS, and a fork's branch is its owner's. const pr = await ctx.api.getPullRequest(pull); const blocked = notMergeable(pr, 'updating'); if (blocked) refuse(blocked); requireMergeBase(pr.base.ref, pull.repo, ctx); + if (!pr.headRepo) { + refuse(`GitHub no longer knows the head repository of ${repoSlug(pull.repo)}#${pull.number} (a deleted fork?)`); + } + if (!sameRepository(pr.headRepo, pr.repo)) { + refuse( + `the head branch of ${repoSlug(pull.repo)}#${pull.number} lives in ` + + `${field(repoSlug(pr.headRepo), FIELD.slug)}, not in ${repoSlug(pr.repo)}; update_branch writes only ` + + `to a branch of the pull request's own repository`, + ); + } if (baseBranches(pr, ctx).has(pr.head.ref)) { refuse( `the head of ${repoSlug(pull.repo)}#${pull.number}, ${echo(pr.head.ref)}, is a base branch here ` + @@ -286,6 +312,24 @@ async function callMerge(name: string, args: Record, ctx: ToolC ); } if (expected !== undefined) requireHeadSha(expected, pr); + // The rest of "is the head somebody's base?" is GitHub's to say, as for + // delete_branch. A head GitHub can't be asked about is not written to. + let facts: BranchFacts; + try { + facts = await ctx.api.branchFacts(pull.repo, pr.head.ref); + } catch (err) { + refuse( + `couldn't ask GitHub whether ${echo(pr.head.ref)} is a base ` + + `(${describeFailure('branchFacts', err)}); nothing was written`, + ); + } + const onGitHub = baseOnGitHub(pr.head.ref, facts); + if (onGitHub) { + refuse( + `the head of ${repoSlug(pull.repo)}#${pull.number} is a base on GitHub: ${onGitHub}; ` + + `update_branch never writes to one`, + ); + } await ctx.api.updatePullRequestBranch(pull, pr.head.sha); return toolResult( `asked GitHub to merge ${field(pr.base.ref)} into ${field(pr.head.ref)} ` + diff --git a/src/types.ts b/src/types.ts index 41a91d1..497915c 100644 --- a/src/types.ts +++ b/src/types.ts @@ -365,9 +365,9 @@ export interface ReviewList { } /** - * What GitHub knows about a branch that bears on deleting it: a protected - * branch, the default branch, or a branch other open pull requests target is - * somebody's base, whatever the allow-lists say. + * What GitHub knows about a branch that bears on deleting it or writing into + * it: a protected branch, the default branch, or a branch other open pull + * requests target is somebody's base, whatever the allow-lists say. */ export interface BranchFacts { protected: boolean; @@ -563,7 +563,7 @@ export interface GitHubApi { updatePullRequestBranch(pull: ThreadRef, expectedHeadSha?: string): Promise; /** Delete `refs/heads/` in `repo`. */ deleteBranch(repo: RepoRef, branch: string): Promise; - /** What decides whether a merged head branch may be deleted. */ + /** What decides whether a merged head branch may be deleted, or a head branch written into. */ branchFacts(repo: RepoRef, branch: string): Promise; // ── Actions ── diff --git a/test/harness.ts b/test/harness.ts index b03ed8b..f118ed6 100644 --- a/test/harness.ts +++ b/test/harness.ts @@ -122,6 +122,8 @@ export class FakeGitHubApi implements GitHubApi { deleteBranchError: Error | null = null; /** What `branchFacts` answers, by branch name; absent = an ordinary unprotected branch. */ readonly branchFactsByName = new Map(); + /** What `branchFacts` throws instead, while set (GitHub unreachable, or a 404). */ + branchFactsError: Error | null = null; /** A head that moves after the tool read it and before GitHub merges (the 409 race). */ moveHeadBeforeMerge: string | null = null; private nextNumber = 1; @@ -626,6 +628,7 @@ export class FakeGitHubApi implements GitHubApi { async branchFacts(repo: RepoRef, branch: string): Promise { this.record('branchFacts', repo, branch); + if (this.branchFactsError !== null) throw this.branchFactsError; return this.branchFactsByName.get(branch) ?? { protected: false, isDefault: false, openPullsBasedOn: 0 }; } diff --git a/test/toolsMerge.test.ts b/test/toolsMerge.test.ts index 0aea0d4..3ed8c43 100644 --- a/test/toolsMerge.test.ts +++ b/test/toolsMerge.test.ts @@ -2,9 +2,10 @@ * github.merge — merge_pull_request and update_branch. * * The refusals carry the weight: GITHUB_MERGE_BASE_ALLOW is deny-by-default, - * a moved head is refused before GitHub is asked, a draft, closed or merged - * pull request is refused by name, and `delete_branch` never deletes a fork's - * branch or a base branch. Everything runs against FakeGitHubApi. + * a moved head is refused before GitHub is asked, a closed or merged pull + * request is refused by name, and so is a draft when merging, and + * `delete_branch` never deletes a fork's branch or a base branch. Everything + * runs against FakeGitHubApi. */ import { strict as assert } from 'node:assert'; @@ -13,6 +14,7 @@ import { mergeToolModule } from '../src/tools-merge.js'; import { MERGE_FEATURE_SET, silentLogger, + type BranchFacts, type StateStore, type ThreadRef, type ToolContext, @@ -212,6 +214,8 @@ test('delete_branch keeps a branch GitHub says is a base: protected, default, or test('delete_branch keeps a fork’s branch, an unknown head repository, and any base branch', async () => { const cases: Array<[Record, Partial, RegExp]> = [ [{ headRepo: { owner: 'a-fork', name: 'rayforce' } }, {}, /head branch kept: the head branch lives in a-fork\/rayforce/], + // Another repository of the same owner is another repository too. + [{ headRepo: { owner: 'anima-research', name: 'rayforce-2' } }, {}, /head branch kept: the head branch lives in anima-research\/rayforce-2/], [{ headRepo: null }, {}, /head branch kept: GitHub no longer knows the head repository/], [{ headRepo: undefined }, {}, /head branch kept: GitHub no longer knows the head repository/], // A pull request from one base branch into another: `main` → `dev`. @@ -251,6 +255,8 @@ test('update_branch merges the base in, guarded by the head it read', async () = assert.match(out, /asked GitHub to merge dev into feature-8 \(anima-research\/rayforce#8, head abc80+\)/); // The full head sha, whatever form the caller gave, and also when it gave none. assert.deepEqual(api.callsTo('updatePullRequestBranch')[0].args, [PULL, HEAD]); + // GitHub is asked whether the head is a base before anything is written. + assert.deepEqual(api.callLog().slice(-2), ['branchFacts', 'updatePullRequestBranch']); ok(await mergeToolModule.call('update_branch', { number: 8 }, ctx(api))); assert.deepEqual(api.callsTo('updatePullRequestBranch')[1].args, [PULL, HEAD]); @@ -276,10 +282,74 @@ test('update_branch takes a merge’s fences: allowed base, open pull request, h } // A draft may be brought up to date; it just may not be merged. const api = new FakeGitHubApi(); - api.seedPullRequest({ number: 8, draft: true, head: { ref: 'feature-8', sha: HEAD } }); + api.seedPullRequest({ number: 8, draft: true, headRepo: TEST_REPO, head: { ref: 'feature-8', sha: HEAD } }); ok(await mergeToolModule.call('update_branch', { number: 8 }, ctx(api))); }); +test('update_branch writes to no head GitHub says is a base: default, protected, or targeted by an open PR', async () => { + // `main → dev`, with `dev` the only allowed base: no allow-list names `main`, + // so only GitHub can say that it is a base. + const cases: Array<[BranchFacts, RegExp]> = [ + [{ protected: false, isDefault: true, openPullsBasedOn: 0 }, /"main" is the repository's default branch/], + [{ protected: true, isDefault: false, openPullsBasedOn: 0 }, /"main" is protected on GitHub/], + [{ protected: false, isDefault: false, openPullsBasedOn: 1 }, /"main" is the base of another open pull request/], + ]; + for (const [facts, expected] of cases) { + const api = new FakeGitHubApi(); + api.seedPullRequest({ number: 8, headRepo: TEST_REPO, head: { ref: 'main', sha: HEAD } }); + api.branchFactsByName.set('main', facts); + const out = failure(await mergeToolModule.call('update_branch', { number: 8 }, ctx(api))); + assert.match(out, /^update_branch refused: the head of anima-research\/rayforce#8 is a base on GitHub: /); + assert.match(out, expected); + assert.deepEqual(api.callsTo('branchFacts')[0].args, [TEST_REPO, 'main']); + assert.equal(api.callsTo('updatePullRequestBranch').length, 0); + } +}); + +test('update_branch writes only to a head branch of the pull request’s own repository', async () => { + const cases: Array<[Record, RegExp]> = [ + [{ headRepo: { owner: 'a-fork', name: 'rayforce' } }, /the head branch of anima-research\/rayforce#8 lives in a-fork\/rayforce, not in anima-research\/rayforce/], + // Another repository of the same owner is another repository too. + [{ headRepo: { owner: 'anima-research', name: 'rayforce-2' } }, /lives in anima-research\/rayforce-2, not in anima-research\/rayforce/], + [{ headRepo: null }, /GitHub no longer knows the head repository of anima-research\/rayforce#8/], + [{ headRepo: undefined }, /GitHub no longer knows the head repository of anima-research\/rayforce#8/], + ]; + for (const [partial, expected] of cases) { + const api = new FakeGitHubApi(); + api.seedPullRequest({ number: 8, head: { ref: 'feature-8', sha: HEAD }, ...partial }); + const out = failure(await mergeToolModule.call('update_branch', { number: 8 }, ctx(api))); + assert.match(out, expected); + assert.equal(api.callsTo('branchFacts').length, 0); + assert.equal(api.callsTo('updatePullRequestBranch').length, 0); + } +}); + +test('a head repository GitHub spells in another case is the pull request’s own repository', async () => { + // The pull request's repository keeps the spelling GITHUB_REPOS gave it; the + // head repository comes back as GitHub spells it, and GitHub ignores case. + const respelled = { owner: 'Anima-Research', name: 'RayForce' }; + const update = new FakeGitHubApi(); + update.seedPullRequest({ number: 8, headRepo: respelled, head: { ref: 'feature-8', sha: HEAD } }); + ok(await mergeToolModule.call('update_branch', { number: 8 }, ctx(update))); + assert.equal(update.callsTo('updatePullRequestBranch').length, 1); + + const merge = new FakeGitHubApi(); + merge.seedPullRequest({ number: 8, headRepo: respelled, head: { ref: 'feature-8', sha: HEAD } }); + const out = ok(await mergeToolModule.call('merge_pull_request', { number: 8, delete_branch: true }, ctx(merge))); + assert.match(out, /\ndeleted branch feature-8$/); +}); + +test('update_branch writes nothing when GitHub can’t say whether the head is a base', async () => { + const api = seeded(); + api.branchFactsError = Object.assign(new Error('Not Found'), { status: 404 }); + const out = failure(await mergeToolModule.call('update_branch', { number: 8 }, ctx(api))); + assert.match( + out, + /^update_branch refused: couldn't ask GitHub whether "feature-8" is a base \(branchFacts failed: GitHub error 404: Not Found\); nothing was written$/, + ); + assert.equal(api.callsTo('updatePullRequestBranch').length, 0); +}); + test('an unknown tool name is refused by the module', async () => { const out = failure(await mergeToolModule.call('force_push', {}, ctx(seeded()))); assert.match(out, /unknown tool for github\.merge: force_push/);