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/);