From 40e64024d8ea2afef2206d7eb841ef25c2a9559a Mon Sep 17 00:00:00 2001 From: antra-tess Date: Thu, 22 Oct 2026 23:08:53 -0700 Subject: [PATCH 1/2] Merge, update-branch, workflow runs and review reads, behind opt-in allow-lists Two new feature sets and one read tool, so an agent can carry a pull request from review to merge without a human running gh for it: - github.merge: merge_pull_request (merge/squash/rebase at the head sha the caller read; optional delete_branch) and update_branch. Merges only into the bases GITHUB_MERGE_BASE_ALLOW names, empty by default, so granting the feature set alone merges nothing. Draft, closed, merged and moved-head pull requests are refused before GitHub is asked; delete_branch never touches a fork's branch or a base branch. - github.actions: list_workflow_runs, dispatch_workflow, rerun_workflow_run. Dispatch and re-run act only on workflow files GITHUB_WORKFLOW_ALLOW names (empty by default), and a dispatch runs only on a base branch. Built for re-requesting a review from a workflow that skips comments from bots. - list_reviews (github.read): every submitted review, paged to the end. app-manifest.json now asks for contents: write: GitHub refuses resolveReviewThread to an App without it, which is why resolve_thread failed with "Resource not accessible by integration". README's Security section lists every permission by the feature set that needs it. Co-Authored-By: Claude Opus 5.5 --- README.md | 80 +++++-- app-manifest.json | 4 +- changelog.d/merge-and-actions.added.md | 13 + changelog.d/merge-and-actions.changed.md | 7 + deploy/github-mcpl.env.example | 7 + docs/deploy.md | 15 +- src/feature-sets.ts | 24 +- src/github-adapter.ts | 242 ++++++++++++++++++- src/index.ts | 12 +- src/server.ts | 12 + src/tools-actions.ts | 290 +++++++++++++++++++++++ src/tools-issues-pr.ts | 7 +- src/tools-merge.ts | 236 ++++++++++++++++++ src/tools-read.ts | 48 +++- src/types.ts | 192 ++++++++++++++- test/githubAdapter.test.ts | 158 ++++++++++++ test/harness.ts | 138 ++++++++++- test/integration.test.ts | 14 +- test/smoke.test.ts | 49 +++- test/toolsActions.test.ts | 184 ++++++++++++++ test/toolsMerge.test.ts | 218 +++++++++++++++++ test/toolsRead.test.ts | 53 ++++- test/webhook.test.ts | 2 + test/wp8-docs.test.ts | 7 +- 24 files changed, 1947 insertions(+), 65 deletions(-) create mode 100644 changelog.d/merge-and-actions.added.md create mode 100644 changelog.d/merge-and-actions.changed.md create mode 100644 src/tools-actions.ts create mode 100644 src/tools-merge.ts create mode 100644 test/toolsActions.test.ts create mode 100644 test/toolsMerge.test.ts diff --git a/README.md b/README.md index dfa2ec7..f26a6ee 100644 --- a/README.md +++ b/README.md @@ -18,15 +18,18 @@ or several of one owner; one process per owner. rollback deletes the comments created after the checkpoint and restores the bodies of the ones edited after it. Nothing GitHub cannot undo is checkpointed, and nothing is granted that the host did not grant. -- **No merge, ever.** Not in v1, not behind a flag. The base branch of any - pull request the server opens or retargets is checked against an allow-list. +- **Merge only where the operator said so.** Merging is its own feature set, + `github.merge`, and it merges only into the bases `GITHUB_MERGE_BASE_ALLOW` + names — empty by default, so a deployment that never set it cannot merge + anything, whatever a host grants. The base of any pull request the server + opens or retargets is checked against `GITHUB_PR_BASE_ALLOW` the same way. Built on [`@animalabs/mcpl-core`](../mcpl-core-ts); sibling of [slack-mcpl](../slack-mcpl) and [telegram-mcpl](../telegram-mcpl). > **Status: v1, run end to end against a live GitHub App.** The server answers a > host's handshake in MCPL or in plain MCP, applies the grant it is given -> (absence is denial), serves the twenty-two tools of the five tool feature +> (absence is denial), serves the twenty-eight tools of the seven tool feature > sets, registers a channel per thread and publishes into one, checkpoints > every comment write and rolls it back, and turns a webhook delivery or a > poll pass into a `push/event` and a `channels/incoming` with tags and wake @@ -70,7 +73,7 @@ install -d -m 700 ~/.config/github-mcpl ### 3. Create the GitHub App from the manifest [`app-manifest.json`](app-manifest.json) is a GitHub App manifest: it names -the five permissions the tools need and the eight events the ingress +the five permissions the default tools need and the eight events the ingress understands, so you do not have to tick 40 checkboxes correctly. GitHub's manifest flow is three steps — post the manifest, catch the code GitHub sends back, exchange the code for the App's credentials — and the third is the one @@ -241,6 +244,8 @@ told where Node is instead of looking. | `GITHUB_POLL_INTERVAL_MS` | no | `0` (off) | Polling fallback: issues and PRs, issue comments and review comments updated since each list's cursor, plus the checks on the heads of the open PRs seen. `0` is off; a non-zero value below `60000` is refused. Partial — it cannot see a submitted review, a review request or a push ([what polling misses](docs/deploy.md#option-3--polling-only-no-webhook-at-all)) — and it is not meant to run alongside the webhook. | | `GITHUB_REPOS` | yes | — | Comma-separated `owner/name` allow-list; a write anywhere else is refused by name. Several entries must share one owner (one App installation, one token); a second owner is a second server process. With several, every tool call names its repository. | | `GITHUB_PR_BASE_ALLOW` | no | empty = no PR target allowed | Branches a pull request may target or be retargeted to. A plain `branch` applies to every repository, `owner/name:branch` to that one: `anima-research/rayforce:dev,main`. | +| `GITHUB_MERGE_BASE_ALLOW` | no | empty = no merge | Branches `merge_pull_request` may merge into, in the same format. Naming a branch here is what turns merging on; granting `github.merge` alone does not. | +| `GITHUB_WORKFLOW_ALLOW` | no | empty = no dispatch or re-run | Workflow files (`review.yml`, or `owner/name:review.yml` for one repository) that `dispatch_workflow` and `rerun_workflow_run` may start. Never name a release or publish workflow you would not let the agent run. | | `GITHUB_STATE_FILE` | no | unset = in-memory only | Durable JSON: checkpoint lineage, rollback bookkeeping, and the polling state: a cursor for each repository and list (`pollCursors`), `lastPollAt` where a list with none starts, and the rows announced as new (`pollNew`). A list with a cursor of its own starts from it, not from `lastPollAt`. | | `GITHUB_BOT_LOGIN` | yes | — | The App's bot login, e.g. `rayforce-clone[bot]`. From-self and mention detection depend on it. | | `GITHUB_MAINTAINERS` | no | empty | Comma-separated logins marked `isMaintainer` in metadata and tagged `github:actor=maintainer`. | @@ -330,11 +335,13 @@ surface is checked at its own call — `channels.register` to announce a channel | Feature set | Tools | Rollback | | --- | --- | --- | | `github.events` | — (push events and `channels/incoming` delivery) | no | -| `github.read` | `get_issue`, `get_pull_request`, `list_comments`, `list_review_threads`, `list_pull_request_files`, `get_check_runs`, `search_issues` | no | +| `github.read` | `get_issue`, `get_pull_request`, `list_comments`, `list_review_threads`, `list_reviews`, `list_pull_request_files`, `get_check_runs`, `search_issues` | no | | `github.comments` | `comment`, `reply_in_thread`, `edit_own_comment`, `delete_own_comment`, `upsert_sticky_comment` | **yes** | | `github.review` | `create_review`, `resolve_thread`, `unresolve_thread` | no (GitHub cannot delete a review) | | `github.issues` | `create_issue`, `edit_issue`, `close_issue`, `reopen_issue` | no | | `github.pr` | `create_pull_request`, `edit_pull_request`, `request_review` | no | +| `github.merge` | `merge_pull_request`, `update_branch` | no (a merge cannot be undone) | +| `github.actions` | `list_workflow_runs`, `dispatch_workflow`, `rerun_workflow_run` | no | Notes worth knowing before you grant them: @@ -344,8 +351,25 @@ Notes worth knowing before you grant them: `rayforce-audit` does with ``. - `create_pull_request` and a base change in `edit_pull_request` refuse a base outside `GITHUB_PR_BASE_ALLOW`. Empty means no pull request may be opened. -- `github.pr` has no merge tool and no branch deletion. There is nothing to - disable; the capability does not exist. +- `github.pr` has no merge tool and no branch deletion: opening a pull request + and landing one are separate grants. `merge_pull_request` is + `github.merge`'s, and it refuses a base outside `GITHUB_MERGE_BASE_ALLOW` + (empty = no merge), a draft, a closed or merged pull request, and — given + `sha`, the head you reviewed — a head that has moved since. Branch + protection (required checks, approvals) stays GitHub's to enforce. + `delete_branch: true` deletes the merged head branch only when it lives in + the base repository and is not a base branch any allow-list names. +- `update_branch` merges the base into a pull request's head branch, as + GitHub's "Update branch" button does. +- `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. + The use it was built for: a review workflow that ignores comments from bots + still takes a `workflow_dispatch` (`review.yml` with `{"pr-number": "12"}`). + `list_workflow_runs` is a read and is not fenced. +- `list_reviews` reads every submitted review to the end — a pull request + that has been through many bot review rounds passes GitHub's first page + quickly, and the newest verdict is on the last one. - Every write tool refuses a repository outside `GITHUB_REPOS`, by name, before the call reaches GitHub. @@ -410,7 +434,7 @@ reach metadata and never reach the model. other logins. A separate service user would not change that on a host where the agent has passwordless `sudo`, which is the usual agent host. What bounds a leaked key is the rest of this list: the repository allow-list, the - permissions below, no merge. Generating a new key in the App's settings and + permissions below, the merge and workflow allow-lists. Generating a new key in the App's settings and deleting the old one revokes it. [docs/deploy.md](docs/deploy.md#who-can-read-the-key) has the cases where isolation is real. - **An allow-list of repositories.** `GITHUB_REPOS` is required. Events from @@ -422,7 +446,12 @@ reach metadata and never reach the model. - **A base allow-list.** `GITHUB_PR_BASE_ALLOW` is empty by default, which means no pull request may be opened or retargeted at all. For rayforce it is `dev`, so the release branch is unreachable even by mistake. -- **No merge.** No tool merges, closes a release branch or deletes a branch. +- **Merge and workflows are opt-in twice.** A host has to grant + `github.merge` or `github.actions`, *and* the operator has to name the + bases (`GITHUB_MERGE_BASE_ALLOW`) or the workflow files + (`GITHUB_WORKFLOW_ALLOW`) they may touch. Either alone does nothing. Keep + release branches out of both, and protect them on GitHub as well: branch + protection is the fence that holds even if the key leaks. - **Bodies from strangers are data, not instructions.** An issue body, a comment, a review and a check's output are text written by other people, reaching a model that holds a write token. The server forwards them as @@ -430,12 +459,27 @@ reach metadata and never reach the model. prompt has to say so, and both example recipes do. Treat anyone who can open an issue on the repository as someone who can type into your agent's context. -- **The permissions are the minimum the tools need:** `issues: write` (issue - tools and issue comments), `pull_requests: write` (PR tools, reviews, - review-comment replies, review-thread resolution), `contents: read` (file - lists and refs; never write), `checks: read` (check runs and conclusions), - `metadata: read` (mandatory for every App). No `administration`, no - `workflows`, no `members`, no OAuth user flow — the App acts only as itself. +- **The permissions, by what needs them.** Grant the App only the rows for + the feature sets you enable; a tool whose permission is missing fails with + GitHub's 403 and nothing else breaks. + + | Permission | Needed by | + | --- | --- | + | `metadata: read` | every App (mandatory) | + | `issues: write` | `github.issues`, issue comments | + | `pull_requests: write` | `github.pr`, `github.review`, review-comment replies, `list_reviews` | + | `checks: read` | `get_check_runs`, the check summary of `get_pull_request` | + | `contents: read` | file lists and refs | + | `contents: write` | `github.merge` (merge, update branch, delete the merged branch), and `resolve_thread` / `unresolve_thread`: GitHub refuses `resolveReviewThread` to an App that cannot write contents, whatever its pull-request permission | + | `workflows: write` | `merge_pull_request` and `update_branch` on a pull request that changes `.github/workflows/` (GitHub refuses an App that lacks it) | + | `actions: write` | `github.actions` (`actions: read` is enough for `list_workflow_runs` alone) | + + [`app-manifest.json`](app-manifest.json) asks for the minimum surface: + everything above except `workflows` and `actions`, which only the opt-in + feature sets use. Add those two on the App's **Permissions** page when you + enable `github.merge` or `github.actions`, and accept the new permissions + on the installation. No `administration`, no `members`, no OAuth user flow + — the App acts only as itself. ## Limits in v1 @@ -447,7 +491,9 @@ reach metadata and never reach the model. the agent reads the log by following the URL as a human would, or does without. (RFC-005 references for logs and diffs are on the roadmap, not here.) -- **No merge, no branch deletion, no release-branch write.** +- **No push.** The App can merge a pull request and delete its merged head + branch, but no tool writes 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.** @@ -473,7 +519,7 @@ the network. | `webhook.ts`, `poller.ts` | .3 | Ingress: signature-checked deliveries and repository since-polls become `NormalizedEvent`s; dedup; allow-list. | | `tags.ts`, `content.ts` | .4 | Pure functions: `tagsFor`, `renderEvent`, `renderThread`, `renderPullRequestSummary`, `renderChecks`, all bounded by `LIMITS`. | | `channels.ts` | .5 | Channel registry and descriptors, lazy registration, `channels/incoming`, `channels/publish`, typing as a no-op. | -| `feature-sets.ts`, `tools-*.ts` | .6 | Feature-set declarations and the read, issues, pr and review tools. | +| `feature-sets.ts`, `tools-*.ts` | .6 | Feature-set declarations and the read, issues, pr and review tools; the merge (`tools-merge.ts`) and actions (`tools-actions.ts`) tools came after v1. | | `tools-comments.ts`, `state.ts` | .7 | Comment tools including the sticky upsert; checkpoint lineage and rollback. | | `server.ts`, `index.ts` | .9 | Handshake, `featureSets/update`, tool routing, the event pipeline, plain-MCP mode. | diff --git a/app-manifest.json b/app-manifest.json index 034274e..bca28c0 100644 --- a/app-manifest.json +++ b/app-manifest.json @@ -1,6 +1,6 @@ { "name": "rayforce-clone", - "description": "An agent living in one GitHub repository: it wakes on issues, comments, pull requests, reviews and check results, and answers in the thread it was spoken to in. Runs as github-mcpl, an MCPL connector. No merge, no release-branch write.", + "description": "An agent living in one GitHub repository: it wakes on issues, comments, pull requests, reviews and check results, and answers in the thread it was spoken to in. Runs as github-mcpl, an MCPL connector. Merges only into the bases its operator allows; never pushes.", "url": "https://example.com/github-mcpl", "public": false, "hook_attributes": { @@ -13,7 +13,7 @@ "default_permissions": { "issues": "write", "pull_requests": "write", - "contents": "read", + "contents": "write", "checks": "read", "metadata": "read" }, diff --git a/changelog.d/merge-and-actions.added.md b/changelog.d/merge-and-actions.added.md new file mode 100644 index 0000000..5c233f5 --- /dev/null +++ b/changelog.d/merge-and-actions.added.md @@ -0,0 +1,13 @@ +- `github.merge` feature set: `merge_pull_request` (merge, squash or rebase, + at the head sha the caller read, optionally deleting the merged head branch) + and `update_branch`. It merges only into the bases `GITHUB_MERGE_BASE_ALLOW` + names, which is empty by default: granting the feature set alone merges + nothing. Drafts, closed and merged pull requests and a moved head are + refused before GitHub is asked; 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 + on a base branch. Built for re-requesting a review from a workflow that + ignores comments from bots. +- `list_reviews` (`github.read`): every submitted review on a pull request, + read to the last page, ending on the newest verdict. diff --git a/changelog.d/merge-and-actions.changed.md b/changelog.d/merge-and-actions.changed.md new file mode 100644 index 0000000..37b7e6b --- /dev/null +++ b/changelog.d/merge-and-actions.changed.md @@ -0,0 +1,7 @@ +- **Operators:** `app-manifest.json` asks for `contents: write` instead of + `contents: read`. GitHub refuses `resolveReviewThread` to an App that cannot + write contents, so `resolve_thread` and `unresolve_thread` failed with + "Resource not accessible by integration" on an App made from the old + manifest. Raise the permission on the App's **Permissions** page and accept + it on the installation; add `actions: write` and `workflows: write` there + too if you enable `github.actions` or `github.merge`. diff --git a/deploy/github-mcpl.env.example b/deploy/github-mcpl.env.example index 3fb3ae8..2966056 100644 --- a/deploy/github-mcpl.env.example +++ b/deploy/github-mcpl.env.example @@ -27,6 +27,13 @@ GITHUB_REPOS=REPLACE_WITH_OWNER/REPLACE_WITH_REPO # `branch` for every repository, `owner/name:branch` for one of them. # e.g. a repository that merges into dev; the contributor recipe's targets use main. GITHUB_PR_BASE_ALLOW=dev +# What merge_pull_request (github.merge) may merge into, same format. Empty +# (the default) means no merge at all, whatever the host grants. +#GITHUB_MERGE_BASE_ALLOW=dev +# Workflow files dispatch_workflow and rerun_workflow_run (github.actions) may +# start: `review.yml` everywhere, `owner/name:ci.yml` in one repository. Empty +# means none. Never a release or publish workflow. +#GITHUB_WORKFLOW_ALLOW=review.yml GITHUB_MAINTAINERS=REPLACE_WITH_MAINTAINER_LOGIN # --- ingress: ONE of the two ---------------------------------------------- diff --git a/docs/deploy.md b/docs/deploy.md index fcfa022..5bca764 100644 --- a/docs/deploy.md +++ b/docs/deploy.md @@ -139,11 +139,16 @@ deliberate choice, not an oversight: reads any file of any user. A second user there guards against accidents and costs upkeep (a sudo rule, a wrapper, logs the agent cannot see); it does not stop an agent that was talked into reading the key. -- **What bounds a leaked key** is the App itself: installed on one - repository, `contents: read` (no push, no new branch), no merge tool, PR - bases checked against `GITHUB_PR_BASE_ALLOW`. The worst case is comments, - issues and reviews under the bot's name on that one repository. Revoke it - by generating a new key in the App's settings and deleting the old one. +- **What bounds a leaked key** is the App itself: the repositories it is + installed on and the permissions it holds — not this server's allow-lists, + which a leaked key does not go through. With the manifest's permissions a + leaked key can write comments, issues and reviews, and — because + `contents: write` is what `resolve_thread` and merging need — push to any + branch that branch protection does not cover. So install the App on the + repositories it serves and no more, and protect every branch that matters + (require a pull request, and restrict who may push) on GitHub itself. + Revoke a key by generating a new one in the App's settings and deleting + the old one. - **Two agents on one host** each get their own unix user and home. A home is mode 750 by default on current Ubuntu, so one agent's `~/.config/github-mcpl` is not readable by the other. diff --git a/src/feature-sets.ts b/src/feature-sets.ts index 684e74d..648389b 100644 --- a/src/feature-sets.ts +++ b/src/feature-sets.ts @@ -1,8 +1,8 @@ /** * Feature-set declarations — MCPL 0.5 (SPEC §5.1, §6.1, §6.2), TASK-SRHKG.1.6. * - * Six feature sets, exactly as the contract's "Feature sets and tools" section - * names them: + * Eight feature sets: the six of the contract's "Feature sets and tools" + * section, and the two added after it (merge, actions): * * github.events — ingress: push/event and channels/incoming. No tools. * github.read — the read tools (tools-read.ts). @@ -10,6 +10,9 @@ * github.review — create_review, resolve_thread, unresolve_thread. * github.issues — create_issue, edit_issue, close_issue, reopen_issue. * github.pr — create_pull_request, edit_pull_request, request_review. + * github.merge — merge_pull_request, update_branch (tools-merge.ts). + * github.actions — list_workflow_runs, dispatch_workflow, rerun_workflow_run + * (tools-actions.ts). * * Which tool belongs to which set is not restated here: `TOOLS_BY_FEATURE_SET` * in types.ts is the single answer, and `ROLLBACK_CAPABLE` the single answer to @@ -54,12 +57,14 @@ import type { TreatmentRule, } from '@animalabs/mcpl-core'; import { + ACTIONS_FEATURE_SET, COMMENTS_FEATURE_SET, CORE_TAGS, EVENTS_FEATURE_SET, FEATURE_SETS, GITHUB_TAGS, ISSUES_FEATURE_SET, + MERGE_FEATURE_SET, PR_FEATURE_SET, READ_FEATURE_SET, REVIEW_FEATURE_SET, @@ -181,8 +186,8 @@ const DESCRIPTIONS: Record = { 'reviews, review comments, check results and pushes — as events and as channel ' + 'deliveries. Carries no tools.', [READ_FEATURE_SET]: - 'Read issues, pull requests, comments, review threads, changed files, check runs and ' + - 'search results from the allowed repository. No writes.', + 'Read issues, pull requests, comments, review threads, submitted reviews, changed files, ' + + 'check runs and search results from the allowed repository. No writes.', [COMMENTS_FEATURE_SET]: 'Write comments on issues and pull requests: a new comment, a reply inside a review ' + 'thread, an edit or delete of the bot’s own comment, and a sticky comment kept ' + @@ -196,11 +201,18 @@ const DESCRIPTIONS: Record = { 'first) and reopen it.', [PR_FEATURE_SET]: 'Open a pull request against an allowed base branch, edit its title, body, base or ' + - 'draft state, and request reviewers. Merging and branch deletion are not offered.', + 'draft state, and request reviewers. Merging is github.merge, granted separately.', + [MERGE_FEATURE_SET]: + 'Merge a pull request into a base branch GITHUB_MERGE_BASE_ALLOW names (none by default), at ' + + 'the head sha the caller read, by merge, squash or rebase, optionally deleting the merged ' + + 'head branch; and bring a pull request branch up to date with its base. A merge cannot be undone.', + [ACTIONS_FEATURE_SET]: + 'List GitHub Actions workflow runs; dispatch a workflow GITHUB_WORKFLOW_ALLOW names (none by ' + + 'default) on a base branch, with inputs; and re-run such a run, all jobs or only the failed ones.', }; /** - * The six declarations, keyed by feature-set name (§6.1). + * The eight declarations, keyed by feature-set name (§6.1). * * `rollback` is derived from `ROLLBACK_CAPABLE` rather than written out, and it * is stated explicitly (including `false`) so a host reads a decision rather diff --git a/src/github-adapter.ts b/src/github-adapter.ts index 00d6419..3ad0985 100644 --- a/src/github-adapter.ts +++ b/src/github-adapter.ts @@ -41,6 +41,7 @@ import { type ChecksSummary, type Comment, type Config, + type DispatchWorkflowInput, type CreateIssueInput, type CreatePullRequestInput, type CreateReviewInput, @@ -52,20 +53,26 @@ import { type IssueSearchHit, type IssueState, type IssueStateReason, + type ListWorkflowRunsInput, type Logger, + type MergePullRequestInput, + type MergeResult, type PolledComment, type PolledIssue, type PollRead, type PullRequest, type PullRequestFile, + type PullRequestReview, type RepoRef, type ReviewState, type ReviewThread, type ReviewThreadComment, type SubmittedReview, + type SubmittedReviewState, type ThreadRef, type UpdateIssueInput, type UpdatePullRequestInput, + type WorkflowRun, } from './types.js'; // ───────────────────────────────────────────────────────────────────────────── @@ -111,6 +118,15 @@ const CHECK_RUN_PAGE = 100; * rather than summarized from a prefix. */ const CHECK_RUN_MAX_PAGES = 5; + +/** + * `list_reviews` pages: 100 reviews a page, at most this many pages. A pull + * request that has been through many bot review rounds passes GitHub's default + * page of 30 quickly, and the *latest* verdict is what a caller wants, so the + * read goes to the end rather than stopping at the first page. + */ +const REVIEW_PAGE = 100; +const REVIEW_MAX_PAGES = 5; /** * Requests one `…Since` poll makes before it stops, past which it goes on * only to finish a second it is part way through, and leaves the rest for a @@ -271,7 +287,11 @@ interface ApiPull { merged_at?: string | null; mergeable?: boolean | null; node_id?: string; - head?: { ref?: string | null; sha?: string | null } | null; + head?: { + ref?: string | null; + sha?: string | null; + repo?: { name?: string | null; owner?: { login?: string | null } | null } | null; + } | null; base?: { ref?: string | null; sha?: string | null } | null; user?: ApiActor | null; requested_reviewers?: Array | null; @@ -304,6 +324,32 @@ export interface CheckRunRow { html_url?: string | null; } +/** One row of `GET /repos/{o}/{r}/pulls/{n}/reviews`. */ +interface ApiReview { + id?: number; + user?: ApiActor | null; + state?: string | null; + body?: string | null; + commit_id?: string | null; + submitted_at?: string | null; + html_url?: string | null; +} + +/** One row of `GET /repos/{o}/{r}/actions/runs`. */ +interface ApiWorkflowRun { + id?: number; + name?: string | null; + path?: string | null; + event?: string | null; + status?: string | null; + conclusion?: string | null; + head_branch?: string | null; + head_sha?: string | null; + run_attempt?: number | null; + created_at?: string | null; + html_url?: string | null; +} + interface GqlActor { login?: string | null; __typename?: string | null; @@ -370,6 +416,51 @@ function reviewStateOf(state: string | null | undefined): ReviewState { return 'commented'; } +/** A review's state as `GET …/reviews` spells it, including the two `reviewStateOf` folds away. */ +function submittedReviewStateOf(state: string | null | undefined): SubmittedReviewState { + const upper = (state ?? '').toUpperCase(); + if (upper === 'APPROVED') return 'approved'; + if (upper === 'CHANGES_REQUESTED') return 'changes_requested'; + if (upper === 'DISMISSED') return 'dismissed'; + if (upper === 'PENDING') return 'pending'; + return 'commented'; +} + +function mapReview(data: ApiReview): PullRequestReview { + return { + id: data.id ?? 0, + author: actorOf(data.user), + state: submittedReviewStateOf(data.state), + body: data.body ?? '', + commitId: data.commit_id ?? '', + submittedAt: data.submitted_at ?? '', + html_url: data.html_url ?? '', + }; +} + +function mapWorkflowRun(data: ApiWorkflowRun): WorkflowRun { + return { + id: data.id ?? 0, + name: data.name ?? '', + path: data.path ?? '', + event: data.event ?? '', + status: data.status ?? '', + conclusion: data.conclusion ?? null, + headBranch: data.head_branch ?? '', + headSha: data.head_sha ?? '', + runAttempt: data.run_attempt ?? 1, + createdAt: data.created_at ?? '', + html_url: data.html_url ?? '', + }; +} + +/** The repository a head branch lives in, or null when GitHub no longer knows (a deleted fork). */ +function headRepoOf(head: ApiPull['head']): RepoRef | null { + const owner = head?.repo?.owner?.login; + const name = head?.repo?.name; + return owner && name ? { owner, name } : null; +} + function mapIssue(repo: RepoRef, data: ApiIssue): Issue { return { number: data.number ?? 0, @@ -1372,6 +1463,154 @@ export class GitHubAdapter implements GitHubApi { return this.mapPull(repo, data); } + // ─────────────────────────────────────────────────────────────────────────── + // Reviews (read) + // ─────────────────────────────────────────────────────────────────────────── + + /** + * Every review on a pull request, oldest first (GitHub's order). Paged to + * the end up to {@link REVIEW_MAX_PAGES} pages: the newest verdict is the one + * that matters and it is on the *last* page, so stopping at the first would + * answer with a stale one. + */ + async listReviews(pull: ThreadRef): Promise { + const rows: ApiReview[] = []; + for (let page = 1; page <= REVIEW_MAX_PAGES; page++) { + const batch = await this.call('listReviews', async () => { + const res = await this.octokit.request('GET /repos/{owner}/{repo}/pulls/{pull_number}/reviews', { + owner: pull.repo.owner, + repo: pull.repo.name, + pull_number: pull.number, + per_page: REVIEW_PAGE, + page, + }); + return (Array.isArray(res.data) ? res.data : []) as ApiReview[]; + }); + rows.push(...batch); + if (batch.length < REVIEW_PAGE) break; + if (page === REVIEW_MAX_PAGES) { + this.log('warn', 'listReviews: page bound reached; later reviews not read', { + repo: repoSlug(pull.repo), + number: pull.number, + read: rows.length, + }); + } + } + return rows.map(mapReview); + } + + // ─────────────────────────────────────────────────────────────────────────── + // Merge + // ─────────────────────────────────────────────────────────────────────────── + + /** + * `PUT …/pulls/{n}/merge` with the head sha the caller read: GitHub answers + * 409 when the head has moved since, so a merge can never take a commit + * nobody looked at. 405 (not mergeable: a failing required check, a + * missing approval, a conflict) and 422 come back as they are. + */ + async mergePullRequest(pull: ThreadRef, input: MergePullRequestInput): Promise { + return this.call('mergePullRequest', async () => { + const res = await this.octokit.request('PUT /repos/{owner}/{repo}/pulls/{pull_number}/merge', { + owner: pull.repo.owner, + repo: pull.repo.name, + pull_number: pull.number, + merge_method: input.method, + sha: input.sha, + ...(input.title !== undefined ? { commit_title: input.title } : {}), + ...(input.message !== undefined ? { commit_message: input.message } : {}), + }); + const data = res.data as { merged?: boolean; sha?: string | null; message?: string | null }; + return { merged: data.merged === true, sha: data.sha ?? '', message: data.message ?? '' }; + }); + } + + async updatePullRequestBranch(pull: ThreadRef, expectedHeadSha?: string): Promise { + await this.call('updatePullRequestBranch', async () => { + await this.octokit.request('PUT /repos/{owner}/{repo}/pulls/{pull_number}/update-branch', { + owner: pull.repo.owner, + repo: pull.repo.name, + pull_number: pull.number, + ...(expectedHeadSha !== undefined ? { expected_head_sha: expectedHeadSha } : {}), + }); + }); + } + + async deleteBranch(repo: RepoRef, branch: string): Promise { + await this.call('deleteBranch', async () => { + await this.octokit.request('DELETE /repos/{owner}/{repo}/git/refs/{ref}', { + owner: repo.owner, + repo: repo.name, + ref: `heads/${branch}`, + }); + }); + } + + // ─────────────────────────────────────────────────────────────────────────── + // Actions + // ─────────────────────────────────────────────────────────────────────────── + + /** The newest runs first (GitHub's order), one page of at most `limit`. */ + async listWorkflowRuns(repo: RepoRef, input: ListWorkflowRunsInput): Promise { + const filters = { + per_page: Math.max(1, Math.min(input.limit, LIMITS.listEntries)), + ...(input.headSha !== undefined ? { head_sha: input.headSha } : {}), + ...(input.branch !== undefined ? { branch: input.branch } : {}), + ...(input.event !== undefined ? { event: input.event } : {}), + }; + return this.call('listWorkflowRuns', async () => { + const res = + input.workflow === undefined + ? await this.octokit.request('GET /repos/{owner}/{repo}/actions/runs', { + owner: repo.owner, + repo: repo.name, + ...filters, + }) + : await this.octokit.request('GET /repos/{owner}/{repo}/actions/workflows/{workflow_id}/runs', { + owner: repo.owner, + repo: repo.name, + workflow_id: input.workflow, + ...filters, + }); + const data = res.data as { workflow_runs?: ApiWorkflowRun[] }; + return (data.workflow_runs ?? []).slice(0, filters.per_page).map(mapWorkflowRun); + }); + } + + async getWorkflowRun(repo: RepoRef, runId: number): Promise { + return this.call('getWorkflowRun', async () => { + const res = await this.octokit.request('GET /repos/{owner}/{repo}/actions/runs/{run_id}', { + owner: repo.owner, + repo: repo.name, + run_id: runId, + }); + return mapWorkflowRun(res.data as ApiWorkflowRun); + }); + } + + async dispatchWorkflow(repo: RepoRef, input: DispatchWorkflowInput): Promise { + await this.call('dispatchWorkflow', async () => { + await this.octokit.request('POST /repos/{owner}/{repo}/actions/workflows/{workflow_id}/dispatches', { + owner: repo.owner, + repo: repo.name, + workflow_id: input.workflow, + ref: input.ref, + inputs: input.inputs, + }); + }); + } + + async rerunWorkflowRun(repo: RepoRef, runId: number, failedOnly: boolean): Promise { + await this.call('rerunWorkflowRun', async () => { + await this.octokit.request( + failedOnly + ? 'POST /repos/{owner}/{repo}/actions/runs/{run_id}/rerun-failed-jobs' + : 'POST /repos/{owner}/{repo}/actions/runs/{run_id}/rerun', + { owner: repo.owner, repo: repo.name, run_id: runId }, + ); + }); + } + // ─────────────────────────────────────────────────────────────────────────── // Polling fallback (contract amendment 1) and identity // ─────────────────────────────────────────────────────────────────────────── @@ -1585,6 +1824,7 @@ export class GitHubAdapter implements GitHubApi { mergeable: data.mergeable ?? null, head: { ref: data.head?.ref ?? '', sha: headSha }, base: { ref: data.base?.ref ?? '', sha: data.base?.sha ?? '' }, + headRepo: headRepoOf(data.head), author: actorOf(data.user), requestedReviewers: (data.requested_reviewers ?? []) .map((r) => r?.login ?? '') diff --git a/src/index.ts b/src/index.ts index 87a4673..d621a76 100644 --- a/src/index.ts +++ b/src/index.ts @@ -8,7 +8,7 @@ * * What startup does, in order: validate the environment (`readConfig`, which * names every offending variable and no secret value), build the GitHub - * adapter, load the checkpoint store, register the five tool modules, start + * adapter, load the checkpoint store, register the seven tool modules, start * whichever ingress paths the configuration asks for, and serve. * * Stdout is the JSON-RPC channel and nothing else writes to it; every @@ -23,15 +23,17 @@ import { Poller } from './poller.js'; import { GitHubMcplServer } from './server.js'; import { GithubState } from './state.js'; import { commentsToolModule } from './tools-comments.js'; +import { actionsToolModule } from './tools-actions.js'; import { issuesToolModule, prToolModule } from './tools-issues-pr.js'; +import { mergeToolModule } from './tools-merge.js'; import { readToolModule } from './tools-read.js'; import { reviewToolModule } from './tools-review.js'; import { ConfigError, readConfig, repoSlug, stderrLogger, type Config, type Logger } from './types.js'; import { WebhookListener, webhookListenerFromConfig } from './webhook.js'; /** - * The five modules the six feature sets need. `github.events` carries no - * tools (it is the ingress), so there is no module for it; the other five are + * The seven modules the eight feature sets need. `github.events` carries no + * tools (it is the ingress), so there is no module for it; the other seven are * exactly `TOOLS_BY_FEATURE_SET`'s keys. */ function registerTools(server: GitHubMcplServer): void { @@ -40,6 +42,8 @@ function registerTools(server: GitHubMcplServer): void { server.register(reviewToolModule); server.register(issuesToolModule); server.register(prToolModule); + server.register(mergeToolModule); + server.register(actionsToolModule); } /** What `main` has to shut down, in the order it has to shut them down. */ @@ -89,6 +93,8 @@ async function build(config: Config, log: Logger): Promise<{ server: GitHubMcplS repos: config.repos, maintainers: config.maintainers, prBaseAllow: config.prBaseAllow, + mergeBaseAllow: config.mergeBaseAllow, + workflowAllow: config.workflowAllow, state, logger: log, }); diff --git a/src/server.ts b/src/server.ts index 9a6f4f5..06b1186 100644 --- a/src/server.ts +++ b/src/server.ts @@ -106,6 +106,10 @@ export interface GitHubServerOptions { maintainers?: readonly string[]; /** GITHUB_PR_BASE_ALLOW; what `github.pr` may target. */ prBaseAllow?: readonly string[]; + /** GITHUB_MERGE_BASE_ALLOW; what `github.merge` may merge into. */ + mergeBaseAllow?: readonly string[]; + /** GITHUB_WORKFLOW_ALLOW; what `github.actions` may dispatch or re-run. */ + workflowAllow?: readonly string[]; /** * The checkpoint store. Absent keeps an in-memory {@link GithubState}, which * is what the contract notes say GITHUB_STATE_FILE's absence means. @@ -292,6 +296,9 @@ const THREAD_ARGUMENT: ReadonlySet = new Set([ 'updateIssue', 'updatePullRequest', 'requestReviewers', + 'listReviews', + 'mergePullRequest', + 'updatePullRequestBranch', ]); /** @@ -395,6 +402,9 @@ const PULL_ONLY: ReadonlySet = new Set([ 'replyToReviewComment', 'updatePullRequest', 'requestReviewers', + 'listReviews', + 'mergePullRequest', + 'updatePullRequestBranch', ]); /** @@ -642,6 +652,8 @@ export class GitHubMcplServer { botLogin: this.options.botLogin ?? '', repos: this.options.repos ?? [], prBaseAllow: this.options.prBaseAllow ?? [], + mergeBaseAllow: this.options.mergeBaseAllow ?? [], + workflowAllow: this.options.workflowAllow ?? [], logger: this.logger, }; } diff --git a/src/tools-actions.ts b/src/tools-actions.ts new file mode 100644 index 0000000..a83d656 --- /dev/null +++ b/src/tools-actions.ts @@ -0,0 +1,290 @@ +/** + * github.actions — GitHub Actions workflow runs: read them, dispatch one, + * re-run one. + * + * list_workflow_runs — the newest runs, by pull request, sha, branch, + * workflow or event. + * dispatch_workflow — a `workflow_dispatch` of an allowed workflow file, on + * an allowed base branch, with inputs. + * rerun_workflow_run — re-run every job of a run, or only the failed ones. + * + * Why it exists: a review workflow that ignores comments from bots (they are + * not OWNER, MEMBER or COLLABORATOR) still takes a `workflow_dispatch`, and a + * flaky job is re-run rather than re-pushed. + * + * The fences, both deny-by-default: + * + * - only the workflow files GITHUB_WORKFLOW_ALLOW names may be dispatched or + * re-run. A workflow is code that runs with the repository's secrets; a + * release or publish workflow must not become reachable because a review + * workflow is. Listing runs is a read and is not fenced. + * - a dispatch runs the workflow file as it is on `ref`, so `ref` must be a + * base branch (GITHUB_PR_BASE_ALLOW or GITHUB_MERGE_BASE_ALLOW): the copy + * of the workflow that runs is one that was reviewed and merged, not + * whatever a feature branch says. + * + * Not rollback-capable: a run that has started cannot be un-started. + */ + +import { + ACTIONS_FEATURE_SET, + LIMITS, + TOOL_DISPATCH_WORKFLOW, + TOOL_LIST_WORKFLOW_RUNS, + TOOL_RERUN_WORKFLOW_RUN, + allowedBases, + isWorkflowFileName, + repoSlug, + type RepoRef, + type ToolContext, + type ToolDefinition, + type ToolModule, + type ToolResult, + type WorkflowRun, +} from './types.js'; +import { + FIELD, + REPO_PROPERTY, + ToolArgumentError, + boundedInline, + echo, + field, + optionalBoolean, + optionalNumber, + optionalString, + refuse, + requireNumber, + requireString, + resolveRepo, + resolveThread, + runTool, + toolResult, +} from './tool-support.js'; + +/** GitHub accepts at most this many inputs on one dispatch. */ +const MAX_DISPATCH_INPUTS = 25; +/** A dispatch input value, bounded: GitHub's own limit is far above any real use. */ +const MAX_INPUT_CHARS = 1000; +const DEFAULT_RUNS = 20; + +/** The file part of a workflow path: `.github/workflows/review.yml` → `review.yml`. */ +function workflowFile(path: string): string { + const slash = path.lastIndexOf('/'); + return slash < 0 ? path : path.slice(slash + 1); +} + +/** A workflow file name argument, or a refusal saying what one looks like. */ +function requireWorkflowName(raw: string): string { + const name = raw.trim(); + if (!isWorkflowFileName(name)) { + throw new ToolArgumentError( + `"workflow" must be a workflow file name as .github/workflows/ holds it (e.g. review.yml), got ${echo(raw)}`, + ); + } + return name; +} + +/** GITHUB_WORKFLOW_ALLOW, enforced for one repository; the refusal names what is allowed. */ +export function requireAllowedWorkflow(file: string, repo: RepoRef, ctx: ToolContext): string { + const allowed = allowedBases(repo, ctx.workflowAllow ?? []); + if (allowed.includes(file)) return file; + const detail = + allowed.length === 0 + ? `GITHUB_WORKFLOW_ALLOW allows no workflow on ${repoSlug(repo)}` + : `allowed on ${repoSlug(repo)}: ${boundedInline(allowed)}`; + refuse(`workflow ${echo(file)} is not in GITHUB_WORKFLOW_ALLOW (${detail})`); +} + +/** + * The branch a dispatch runs on: `ref` when given, which must be a base branch + * here; otherwise the one base branch, when there is exactly one. + */ +function dispatchRef(raw: string | undefined, repo: RepoRef, ctx: ToolContext): string { + const bases = [ + ...new Set([...allowedBases(repo, ctx.prBaseAllow), ...allowedBases(repo, ctx.mergeBaseAllow ?? [])]), + ]; + if (raw === undefined || raw.trim() === '') { + if (bases.length === 1) return bases[0]; + if (bases.length === 0) { + refuse(`no base branch is allowed on ${repoSlug(repo)} (GITHUB_PR_BASE_ALLOW, GITHUB_MERGE_BASE_ALLOW), so there is nothing to dispatch on`); + } + throw new ToolArgumentError(`"ref" is required: several base branches are allowed on ${repoSlug(repo)} (${boundedInline(bases)})`); + } + const ref = raw.trim(); + if (!bases.includes(ref)) { + refuse( + `ref ${echo(ref)} is not a base branch on ${repoSlug(repo)} (${ + bases.length === 0 ? 'none is allowed' : `allowed: ${boundedInline(bases)}` + }); a dispatch runs the workflow as that branch has it, so only a reviewed branch may be named`, + ); + } + return ref; +} + +/** `inputs`, as GitHub takes them: string values, at most {@link MAX_DISPATCH_INPUTS}. */ +function parseInputs(value: unknown): Record { + if (value === undefined || value === null) return {}; + if (typeof value !== 'object' || Array.isArray(value)) { + throw new ToolArgumentError('"inputs" must be an object of input names to values'); + } + const entries = Object.entries(value as Record); + if (entries.length > MAX_DISPATCH_INPUTS) { + throw new ToolArgumentError(`"inputs" has ${entries.length} entries; GitHub accepts at most ${MAX_DISPATCH_INPUTS}`); + } + const out: Record = {}; + for (const [key, raw] of entries) { + if (key.trim() === '') throw new ToolArgumentError('"inputs" has an empty input name'); + if (typeof raw !== 'string' && typeof raw !== 'number' && typeof raw !== 'boolean') { + throw new ToolArgumentError(`"inputs.${field(key)}" must be a string, number or boolean`); + } + const text = String(raw); + if (text.length > MAX_INPUT_CHARS) { + throw new ToolArgumentError(`"inputs.${field(key)}" is ${text.length} characters; the bound is ${MAX_INPUT_CHARS}`); + } + out[key] = text; + } + return out; +} + +function renderRun(run: WorkflowRun): string { + const state = run.status === 'completed' ? `completed/${field(run.conclusion ?? 'unknown')}` : field(run.status); + return ( + `run ${run.id} ${field(run.name)} (${field(workflowFile(run.path), FIELD.path)}) [${state}] ${field(run.event)} ` + + `on ${field(run.headBranch)} ${field(run.headSha.slice(0, 12), FIELD.sha)}` + + `${run.runAttempt > 1 ? ` attempt ${run.runAttempt}` : ''} ${field(run.createdAt)} ${field(run.html_url, FIELD.url)}` + ); +} + +export const actionsTools: ToolDefinition[] = [ + { + name: TOOL_LIST_WORKFLOW_RUNS, + description: + 'List GitHub Actions workflow runs, newest first: id, workflow, status and conclusion, event, ' + + 'branch, sha and URL. Narrow by pull request (its head sha), sha, branch, workflow file or event.', + inputSchema: { + type: 'object', + properties: { + repo: REPO_PROPERTY, + number: { type: 'integer', minimum: 1, description: 'Pull request: runs on its current head sha' }, + sha: { type: 'string', description: 'Runs on this head sha' }, + branch: { type: 'string', description: 'Runs on this branch' }, + workflow: { type: 'string', description: 'Workflow file name, e.g. review.yml' }, + event: { type: 'string', description: 'Triggering event, e.g. pull_request, workflow_dispatch' }, + limit: { + type: 'integer', + minimum: 1, + description: `Max runs (default ${DEFAULT_RUNS}, cap ${LIMITS.listEntries})`, + }, + }, + }, + }, + { + name: TOOL_DISPATCH_WORKFLOW, + description: + 'Start a workflow that has a workflow_dispatch trigger. The workflow file must be in ' + + 'GITHUB_WORKFLOW_ALLOW and `ref` a base branch (default: the only one allowed); the run uses the ' + + 'workflow as that branch has it. E.g. workflow review.yml with inputs {"pr-number": "12"} asks for a review.', + inputSchema: { + type: 'object', + properties: { + repo: REPO_PROPERTY, + workflow: { type: 'string', description: 'Workflow file name, e.g. review.yml' }, + ref: { type: 'string', description: 'Branch to run on; must be a base branch. Optional when only one is allowed' }, + inputs: { + type: 'object', + description: 'The workflow_dispatch inputs, name to value (values are sent as strings)', + additionalProperties: { type: ['string', 'number', 'boolean'] }, + }, + }, + required: ['workflow'], + }, + }, + { + name: TOOL_RERUN_WORKFLOW_RUN, + description: + 'Re-run a workflow run — every job, or with failed_only only the failed ones. The run’s workflow ' + + 'file must be in GITHUB_WORKFLOW_ALLOW. list_workflow_runs prints run ids.', + inputSchema: { + type: 'object', + properties: { + repo: REPO_PROPERTY, + run_id: { type: 'integer', minimum: 1, description: 'Workflow run id' }, + failed_only: { type: 'boolean', description: 'Re-run only the failed jobs (default false)' }, + }, + required: ['run_id'], + }, + }, +]; + +async function callActions(name: string, args: Record, ctx: ToolContext): Promise { + switch (name) { + case TOOL_LIST_WORKFLOW_RUNS: { + const limit = Math.min(optionalNumber(args, 'limit') ?? DEFAULT_RUNS, LIMITS.listEntries); + const workflowRaw = optionalString(args, 'workflow'); + const workflow = workflowRaw === undefined ? undefined : requireWorkflowName(workflowRaw); + let repo: RepoRef; + let headSha = optionalString(args, 'sha')?.trim() || undefined; + let scope: string; + if (args.number !== undefined && args.number !== null) { + if (headSha !== undefined) throw new ToolArgumentError('give "number" or "sha", not both'); + const pull = resolveThread(args, ctx, 'pull'); + const pr = await ctx.api.getPullRequest(pull); + repo = pull.repo; + headSha = pr.head.sha; + scope = `${repoSlug(repo)}#${pull.number} head ${field(headSha, FIELD.sha)}`; + } else { + repo = resolveRepo(args, ctx); + scope = headSha === undefined ? repoSlug(repo) : `${repoSlug(repo)} at ${field(headSha, FIELD.sha)}`; + } + const runs = await ctx.api.listWorkflowRuns(repo, { + workflow, + headSha, + branch: optionalString(args, 'branch')?.trim() || undefined, + event: optionalString(args, 'event')?.trim() || undefined, + limit, + }); + if (runs.length === 0) return toolResult(`no workflow runs on ${scope}`); + return toolResult( + [`${runs.length} workflow run${runs.length === 1 ? '' : 's'} on ${scope}, newest first`, ...runs.map(renderRun)].join( + '\n', + ), + ); + } + case TOOL_DISPATCH_WORKFLOW: { + const repo = resolveRepo(args, ctx); + const workflow = requireAllowedWorkflow(requireWorkflowName(requireString(args, 'workflow')), repo, ctx); + const ref = dispatchRef(optionalString(args, 'ref'), repo, ctx); + const inputs = parseInputs(args.inputs); + await ctx.api.dispatchWorkflow(repo, { workflow, ref, inputs }); + const shown = Object.entries(inputs).map(([k, v]) => `${field(k)}=${echo(v)}`); + return toolResult( + `dispatched ${field(workflow)} on ${field(ref)} of ${repoSlug(repo)}` + + `${shown.length > 0 ? ` with ${boundedInline(shown)}` : ''}; ` + + `list_workflow_runs workflow=${field(workflow)} shows the run once GitHub queues it`, + ); + } + case TOOL_RERUN_WORKFLOW_RUN: { + const repo = resolveRepo(args, ctx); + const runId = requireNumber(args, 'run_id'); + const failedOnly = optionalBoolean(args, 'failed_only') ?? false; + // The run is read first: the allow-list is on workflow files, and only + // the run knows which file it came from. + const run = await ctx.api.getWorkflowRun(repo, runId); + requireAllowedWorkflow(workflowFile(run.path), repo, ctx); + await ctx.api.rerunWorkflowRun(repo, runId, failedOnly); + return toolResult( + `re-run of ${failedOnly ? 'the failed jobs of ' : ''}run ${runId} (${field(run.name)}, ` + + `${field(workflowFile(run.path), FIELD.path)}) on ${repoSlug(repo)} requested\n${field(run.html_url, FIELD.url)}`, + ); + } + default: + throw new ToolArgumentError(`unknown tool for ${ACTIONS_FEATURE_SET}: ${field(name)}`); + } +} + +/** The github.actions ToolModule the server registers. */ +export const actionsToolModule: ToolModule = { + featureSet: ACTIONS_FEATURE_SET, + tools: actionsTools, + call: (name, args, ctx) => runTool(name, ctx, () => callActions(name, args, ctx)), +}; diff --git a/src/tools-issues-pr.ts b/src/tools-issues-pr.ts index d339c95..ddb3172 100644 --- a/src/tools-issues-pr.ts +++ b/src/tools-issues-pr.ts @@ -15,8 +15,9 @@ * GITHUB_PR_BASE_ALLOW, and is refused by name otherwise. An empty * allow-list means no pull request may be opened at all — deny by default. * - * There is deliberately no merge tool and no branch deletion: a clone proposes, - * a human merges. + * There is no merge tool and no branch deletion here: opening a pull request + * and landing one are different grants, and the second is github.merge's + * (tools-merge.ts), behind its own allow-list, GITHUB_MERGE_BASE_ALLOW. */ import { @@ -205,7 +206,7 @@ export const prTools: ToolDefinition[] = [ name: TOOL_CREATE_PULL_REQUEST, description: 'Open a pull request. The base branch must be one of the branches GITHUB_PR_BASE_ALLOW ' + - 'names; any other base is refused. There is no merge tool: a human merges.', + 'names; any other base is refused. Merging is merge_pull_request (github.merge), granted separately.', inputSchema: { type: 'object', properties: { diff --git a/src/tools-merge.ts b/src/tools-merge.ts new file mode 100644 index 0000000..a6a1b7d --- /dev/null +++ b/src/tools-merge.ts @@ -0,0 +1,236 @@ +/** + * github.merge — merging a pull request, and bringing its branch up to date. + * + * merge_pull_request — merge, squash or rebase a pull request into its base, + * at the head sha the caller read; optionally delete + * the merged head branch. + * update_branch — merge the base into the head branch (GitHub's + * "Update branch" button). + * + * A separate feature set, so a host grants merging on purpose and never by + * 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 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; + * - with `sha`, 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 same sha again, with a 409, in case it moves in between); + * - `delete_branch` deletes only a head branch in the base repository + * itself, never one a base allow-list names. + * + * Branch protection — required checks, required approvals, linear history — + * is GitHub's to enforce, and its refusal (405) comes back as it is. + */ + +import { + MERGE_FEATURE_SET, + TOOL_MERGE_PULL_REQUEST, + TOOL_UPDATE_BRANCH, + allowedBases, + repoSlug, + sameRepo, + type MergeMethod, + type PullRequest, + type RepoRef, + type ToolContext, + type ToolDefinition, + type ToolModule, + type ToolResult, +} from './types.js'; +import { + FIELD, + REPO_PROPERTY, + ToolArgumentError, + boundedInline, + describeFailure, + echo, + field, + oneOf, + optionalBoolean, + optionalString, + refuse, + resolveThread, + runTool, + toolResult, +} from './tool-support.js'; + +const MERGE_METHODS: readonly MergeMethod[] = ['merge', 'squash', 'rebase']; + +/** + * GITHUB_MERGE_BASE_ALLOW, enforced for one repository. Like + * `requireAllowedBase`, the refusal names the branch and the branches allowed, + * so the caller can tell a misconfiguration from a wrong pull request. + */ +export function requireMergeBase(base: string, repo: RepoRef, ctx: ToolContext): string { + const bases = allowedBases(repo, ctx.mergeBaseAllow ?? []); + if (bases.includes(base)) return base; + const allowed = + bases.length === 0 + ? `GITHUB_MERGE_BASE_ALLOW allows no branch on ${repoSlug(repo)}, so nothing there may be merged` + : `allowed on ${repoSlug(repo)}: ${boundedInline(bases)}`; + refuse(`base branch ${echo(base)} is not in GITHUB_MERGE_BASE_ALLOW (${allowed})`); +} + +/** Why a pull request cannot be merged as it stands, or null when it can be tried. */ +function notMergeable(pr: PullRequest): string | null { + const at = `${repoSlug(pr.repo)}#${pr.number}`; + if (pr.merged) return `${at} is already merged`; + if (pr.state === 'closed') return `${at} is closed; reopen it before merging`; + if (pr.draft) return `${at} is a draft; mark it ready for review (edit_pull_request draft: false) before merging`; + return null; +} + +/** + * Whether the merged head branch may be deleted, 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 that any allow-list names as a base: deleting `main` because + * someone opened `main → release` would be the worst outcome this tool has. + */ +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)) { + return `the head branch lives in ${field(repoSlug(pr.headRepo), FIELD.slug)}, not in ${repoSlug(pr.repo)}`; + } + const protectedBranches = new Set([ + pr.base.ref, + ...allowedBases(pr.repo, ctx.prBaseAllow), + ...allowedBases(pr.repo, ctx.mergeBaseAllow ?? []), + ]); + if (protectedBranches.has(branch)) { + return `${echo(branch)} is a base branch here (GITHUB_PR_BASE_ALLOW / GITHUB_MERGE_BASE_ALLOW) and is never deleted`; + } + return null; +} + +export const mergeTools: ToolDefinition[] = [ + { + name: TOOL_MERGE_PULL_REQUEST, + description: + 'Merge a pull request into its base branch, which must be one GITHUB_MERGE_BASE_ALLOW names. ' + + 'Pass `sha` — the head sha you reviewed (get_pull_request prints it) — and the merge is refused ' + + 'if the head has moved since. Drafts, closed and merged pull requests are refused; branch ' + + 'protection (required checks, approvals) is enforced by GitHub. A merge cannot be undone.', + inputSchema: { + type: 'object', + properties: { + repo: REPO_PROPERTY, + number: { type: 'integer', minimum: 1, description: 'Pull-request number' }, + sha: { + type: 'string', + description: 'The head sha this merge is for; refused if the head has moved (recommended)', + }, + method: { + type: 'string', + enum: [...MERGE_METHODS], + description: 'merge (a merge commit, the default), squash, or rebase', + }, + title: { type: 'string', description: 'Commit title (merge and squash only)' }, + message: { type: 'string', description: 'Commit message (merge and squash only)' }, + delete_branch: { + type: 'boolean', + description: + 'Delete the head branch once merged (default false). Only a branch of this repository, never a base branch.', + }, + }, + required: ['number'], + }, + }, + { + 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"). GitHub does the merge in the background; the head sha changes when it lands. ' + + 'With `sha`, GitHub refuses if the head is no longer that commit.', + inputSchema: { + type: 'object', + properties: { + repo: REPO_PROPERTY, + number: { type: 'integer', minimum: 1, description: 'Pull-request number' }, + sha: { type: 'string', description: 'The head sha you expect the branch to be at' }, + }, + required: ['number'], + }, + }, +]; + +async function callMerge(name: string, args: Record, ctx: ToolContext): Promise { + switch (name) { + case TOOL_MERGE_PULL_REQUEST: { + const pull = resolveThread(args, ctx, 'pull'); + const method = oneOf(args, 'method', MERGE_METHODS, 'merge'); + const expected = optionalString(args, 'sha'); + const title = optionalString(args, 'title'); + const message = optionalString(args, 'message'); + const deleteBranch = optionalBoolean(args, 'delete_branch') ?? false; + if (method === 'rebase' && (title !== undefined || message !== undefined)) { + throw new ToolArgumentError('"title" and "message" apply to merge and squash; a rebase writes no commit of its own'); + } + + const pr = await ctx.api.getPullRequest(pull); + const blocked = notMergeable(pr); + if (blocked) refuse(blocked); + requireMergeBase(pr.base.ref, pull.repo, ctx); + if (expected !== undefined && expected.trim() !== pr.head.sha) { + refuse( + `the head of ${repoSlug(pull.repo)}#${pull.number} is ${field(pr.head.sha, FIELD.sha)}, not ` + + `${echo(expected, FIELD.sha)}: it moved since it was read. Read it again (and its reviews and ` + + `checks) before merging.`, + ); + } + + const result = await ctx.api.mergePullRequest(pull, { method, sha: pr.head.sha, title, message }); + if (!result.merged) { + refuse(`GitHub did not merge ${repoSlug(pull.repo)}#${pull.number}: ${field(result.message, FIELD.url)}`); + } + + const out = [ + `merged ${repoSlug(pull.repo)}#${pull.number} (${field(pr.head.ref)} at ${field(pr.head.sha, FIELD.sha)}) ` + + `into ${field(pr.base.ref)} by ${method}: ${field(result.sha, FIELD.sha)}`, + ]; + if (deleteBranch) { + const why = branchDeletionBlocked(pr, ctx); + if (why) { + out.push(`head branch kept: ${why}`); + } else { + // The merge has happened whatever this does next, so a failed + // deletion is reported beside it rather than turning a successful + // merge into an error result. A repository that deletes merged + // branches itself answers 422 here, which is the same end state. + try { + await ctx.api.deleteBranch(pull.repo, pr.head.ref); + out.push(`deleted branch ${field(pr.head.ref)}`); + } catch (err) { + out.push(`head branch not deleted: ${describeFailure('deleteBranch', err)}`); + } + } + } + return toolResult(out.join('\n')); + } + case TOOL_UPDATE_BRANCH: { + const pull = resolveThread(args, ctx, 'pull'); + const expected = optionalString(args, 'sha'); + await ctx.api.updatePullRequestBranch(pull, expected?.trim()); + return toolResult( + `asked GitHub to merge the base of ${repoSlug(pull.repo)}#${pull.number} into its head branch; ` + + `it runs in the background, and get_pull_request shows the new head sha once it lands`, + ); + } + default: + throw new ToolArgumentError(`unknown tool for ${MERGE_FEATURE_SET}: ${field(name)}`); + } +} + +/** The github.merge ToolModule the server registers. */ +export const mergeToolModule: ToolModule = { + featureSet: MERGE_FEATURE_SET, + tools: mergeTools, + call: (name, args, ctx) => runTool(name, ctx, () => callMerge(name, args, ctx)), +}; diff --git a/src/tools-read.ts b/src/tools-read.ts index 178aa8d..6bf5d60 100644 --- a/src/tools-read.ts +++ b/src/tools-read.ts @@ -3,7 +3,7 @@ * rendering support the write modules share (TASK-SRHKG.1.6). * * Tools: get_issue, get_pull_request, list_comments, list_review_threads, - * list_pull_request_files, get_check_runs, search_issues. None of them writes + * list_pull_request_files, get_check_runs, search_issues, list_reviews. None of them writes * anything, so github.read is not rollback-capable. * * The shared argument helpers, the allow-list check and the failure formatting @@ -31,6 +31,7 @@ import { TOOL_LIST_COMMENTS, TOOL_LIST_PULL_REQUEST_FILES, TOOL_LIST_REVIEW_THREADS, + TOOL_LIST_REVIEWS, TOOL_SEARCH_ISSUES, type ChecksSummary, type Comment, @@ -38,6 +39,7 @@ import { type IssueSearchHit, type PullRequest, type PullRequestFile, + type PullRequestReview, type RepoRef, type ReviewThread, type ThreadKind, @@ -173,6 +175,35 @@ export function renderReviewThreads(pull: ThreadRef, threads: readonly ReviewThr ); } +/** + * Reviews, oldest first, ending on the newest — which is the one a caller + * deciding whether to merge needs, so when the list is longer than the bound + * it is the *oldest* that are dropped, and the line saying so comes first. + */ +export function renderReviews(pull: ThreadRef, reviews: readonly PullRequestReview[]): string { + if (reviews.length === 0) return `no reviews on ${ref(pull.repo, pull.number)}`; + const shown = reviews.slice(-LIMITS.listEntries); + const dropped = reviews.length - shown.length; + return lines( + `${reviews.length} review${reviews.length === 1 ? '' : 's'} on ${ref(pull.repo, pull.number)}, oldest first`, + dropped > 0 && `… ${dropped} older not shown (list bound ${LIMITS.listEntries})`, + ...shown.map((r) => + lines( + `— id ${r.id} ${field(r.state)} by ${field(r.author.login)} at ${field(r.submittedAt || 'not submitted')} ` + + `on ${field(r.commitId.slice(0, 12), FIELD.sha)} ${field(r.html_url, FIELD.url)}`, + r.body.trim() !== '' && bounded(r.body.trim(), REVIEW_BODY_CHARS), + ), + ), + ); +} + +/** + * A review summary in a list of up to fifty: long enough for a verdict line + * and the findings count, short enough that the list stays readable. The full + * text is one URL away. + */ +const REVIEW_BODY_CHARS = 600; + export function renderFiles(pull: ThreadRef, files: readonly PullRequestFile[]): string { if (files.length === 0) return `no changed files on ${ref(pull.repo, pull.number)}`; return lines( @@ -280,6 +311,17 @@ export const readTools: ToolDefinition[] = [ required: ['sha'], }, }, + { + name: TOOL_LIST_REVIEWS, + description: + 'List the reviews submitted on a pull request, oldest first and ending on the newest: verdict ' + + '(approved, changes_requested, commented, dismissed), reviewer, the head sha reviewed, and the summary.', + inputSchema: { + type: 'object', + properties: { repo: REPO_PROPERTY, number: { type: 'integer', minimum: 1, description: 'Pull-request number' } }, + required: ['number'], + }, + }, { name: TOOL_SEARCH_ISSUES, description: @@ -326,6 +368,10 @@ async function callRead(name: string, args: Record, ctx: ToolCo const pull = resolveThread(args, ctx, 'pull'); return toolResult(renderFiles(pull, await ctx.api.listPullRequestFiles(pull))); } + case TOOL_LIST_REVIEWS: { + const pull = resolveThread(args, ctx, 'pull'); + return toolResult(renderReviews(pull, await ctx.api.listReviews(pull))); + } case TOOL_GET_CHECK_RUNS: { const repo = resolveRepo(args, ctx); const sha = requireString(args, 'sha'); diff --git a/src/types.ts b/src/types.ts index 69e1cc8..f329841 100644 --- a/src/types.ts +++ b/src/types.ts @@ -219,6 +219,13 @@ export interface PullRequest { mergeable: boolean | null; head: GitRef; base: GitRef; + /** + * The repository the head branch lives in: the base repository for a branch + * pushed there, a fork otherwise, `null` when the fork was deleted. Optional + * because only the merge tools read it (a head branch is only ever deleted + * where it is the base repository's own). + */ + headRepo?: RepoRef | null; author: EventActor; requestedReviewers: string[]; createdAt: string; @@ -336,6 +343,76 @@ export interface CreatePullRequestInput { draft?: boolean; } +/** A submitted review as `list_reviews` reports it. `pending` is a draft review. */ +export type SubmittedReviewState = 'approved' | 'changes_requested' | 'commented' | 'dismissed' | 'pending'; + +export interface PullRequestReview { + id: number; + author: EventActor; + state: SubmittedReviewState; + body: string; + /** The head sha the review was submitted against. */ + commitId: string; + /** Absent on a pending review. */ + submittedAt: string; + html_url: string; +} + +export type MergeMethod = 'merge' | 'squash' | 'rebase'; + +export interface MergePullRequestInput { + method: MergeMethod; + /** The head sha the merge is for; GitHub refuses with 409 when the head has moved. */ + sha: string; + /** Commit title (merge and squash only). */ + title?: string; + /** Commit message (merge and squash only). */ + message?: string; +} + +export interface MergeResult { + merged: boolean; + /** The merge commit (or the last rebased commit) on the base branch. */ + sha: string; + message: string; +} + +/** One row of `GET /repos/{o}/{r}/actions/runs`. */ +export interface WorkflowRun { + id: number; + /** The workflow's name, e.g. `Review`. */ + name: string; + /** The workflow file, e.g. `.github/workflows/review.yml`. */ + path: string; + event: string; + /** queued | in_progress | completed | waiting | requested | pending */ + status: string; + /** Null until the run completes. */ + conclusion: string | null; + headBranch: string; + headSha: string; + runAttempt: number; + createdAt: string; + html_url: string; +} + +export interface ListWorkflowRunsInput { + /** A workflow file name (`review.yml`); absent = every workflow. */ + workflow?: string; + headSha?: string; + branch?: string; + event?: string; + limit: number; +} + +export interface DispatchWorkflowInput { + /** A workflow file name (`review.yml`), checked against GITHUB_WORKFLOW_ALLOW. */ + workflow: string; + /** The branch whose copy of the workflow runs. */ + ref: string; + inputs: Record; +} + /** * Polling fallback (contract amendment 1). An App installation token cannot * read `GET /notifications`, so the poller asks the repository what changed @@ -454,6 +531,23 @@ export interface GitHubApi { requestReviewers(pull: ThreadRef, logins: string[]): Promise; createPullRequest(repo: RepoRef, input: CreatePullRequestInput): Promise; + // ── Reviews (read) ── + /** Every submitted review on a pull request, oldest first, paged to the end (bounded). */ + listReviews(pull: ThreadRef): Promise; + + // ── Merge ── + mergePullRequest(pull: ThreadRef, input: MergePullRequestInput): Promise; + /** Merge the base into the head branch; `expectedHeadSha` guards against a moved head. */ + updatePullRequestBranch(pull: ThreadRef, expectedHeadSha?: string): Promise; + /** Delete `refs/heads/` in `repo`. */ + deleteBranch(repo: RepoRef, branch: string): Promise; + + // ── Actions ── + listWorkflowRuns(repo: RepoRef, input: ListWorkflowRunsInput): Promise; + getWorkflowRun(repo: RepoRef, runId: number): Promise; + dispatchWorkflow(repo: RepoRef, input: DispatchWorkflowInput): Promise; + rerunWorkflowRun(repo: RepoRef, runId: number, failedOnly: boolean): Promise; + // ── Polling fallback (amendment 1) and identity ── /** `GET /repos/{o}/{r}/issues?state=all&sort=updated&since=`: issues and PRs. */ listIssuesSince(repo: RepoRef, since: string): Promise>; @@ -485,6 +579,14 @@ export const GITHUB_API_METHODS = [ 'updatePullRequest', 'requestReviewers', 'createPullRequest', + 'listReviews', + 'mergePullRequest', + 'updatePullRequestBranch', + 'deleteBranch', + 'listWorkflowRuns', + 'getWorkflowRun', + 'dispatchWorkflow', + 'rerunWorkflowRun', 'listIssuesSince', 'listIssueCommentsSince', 'listReviewCommentsSince', @@ -529,6 +631,17 @@ export interface ToolContext { repos: readonly RepoRef[]; /** GITHUB_PR_BASE_ALLOW entries, as written; {@link allowedBases} resolves them per repository. */ prBaseAllow: readonly string[]; + /** + * GITHUB_MERGE_BASE_ALLOW entries, as written, resolved the same way. Absent + * or empty means no pull request may be merged at all. + */ + mergeBaseAllow?: readonly string[]; + /** + * GITHUB_WORKFLOW_ALLOW entries, as written: the workflow files the + * github.actions tools may dispatch or re-run (`review.yml`, or + * `owner/name:review.yml` for one repository). Absent or empty means none. + */ + workflowAllow?: readonly string[]; logger: Logger; } @@ -549,6 +662,8 @@ export const COMMENTS_FEATURE_SET = 'github.comments'; export const REVIEW_FEATURE_SET = 'github.review'; export const ISSUES_FEATURE_SET = 'github.issues'; export const PR_FEATURE_SET = 'github.pr'; +export const MERGE_FEATURE_SET = 'github.merge'; +export const ACTIONS_FEATURE_SET = 'github.actions'; export const FEATURE_SETS = [ EVENTS_FEATURE_SET, @@ -557,6 +672,8 @@ export const FEATURE_SETS = [ REVIEW_FEATURE_SET, ISSUES_FEATURE_SET, PR_FEATURE_SET, + MERGE_FEATURE_SET, + ACTIONS_FEATURE_SET, ] as const; export type FeatureSetName = (typeof FEATURE_SETS)[number]; @@ -569,6 +686,7 @@ export const TOOL_LIST_REVIEW_THREADS = 'list_review_threads'; export const TOOL_LIST_PULL_REQUEST_FILES = 'list_pull_request_files'; export const TOOL_GET_CHECK_RUNS = 'get_check_runs'; export const TOOL_SEARCH_ISSUES = 'search_issues'; +export const TOOL_LIST_REVIEWS = 'list_reviews'; /** github.comments */ export const TOOL_COMMENT = 'comment'; @@ -593,6 +711,15 @@ export const TOOL_CREATE_PULL_REQUEST = 'create_pull_request'; export const TOOL_EDIT_PULL_REQUEST = 'edit_pull_request'; export const TOOL_REQUEST_REVIEW = 'request_review'; +/** github.merge */ +export const TOOL_MERGE_PULL_REQUEST = 'merge_pull_request'; +export const TOOL_UPDATE_BRANCH = 'update_branch'; + +/** github.actions */ +export const TOOL_LIST_WORKFLOW_RUNS = 'list_workflow_runs'; +export const TOOL_DISPATCH_WORKFLOW = 'dispatch_workflow'; +export const TOOL_RERUN_WORKFLOW_RUN = 'rerun_workflow_run'; + /** * Which feature set owns which tool. The server (.9) routes a `tools/call` by * this map and refuses a tool whose feature set is not active. @@ -606,6 +733,7 @@ export const TOOLS_BY_FEATURE_SET: Record 0 && !repoAllowed(scope, repos)) { + problems.push(`${name}: "${entry}" names a repository outside GITHUB_REPOS`); + } + } + return entries; +} + /** * Validate the environment into a {@link Config}, or throw {@link ConfigError} * listing every problem by variable name. @@ -1190,16 +1363,13 @@ export function readConfig(env: Env = process.env): Config { } } - const prBaseAllow = csv(env.GITHUB_PR_BASE_ALLOW); - for (const entry of prBaseAllow) { - const at = entry.indexOf(':'); - if (at < 0) continue; - const scope = parseRepoSlug(entry.slice(0, at)); - const branch = entry.slice(at + 1); - if (!scope || !branch) { - problems.push(`GITHUB_PR_BASE_ALLOW: "${entry}" is neither a branch nor owner/name:branch`); - } else if (repos.length > 0 && !repoAllowed(scope, repos)) { - problems.push(`GITHUB_PR_BASE_ALLOW: "${entry}" names a repository outside GITHUB_REPOS`); + const prBaseAllow = readBaseAllow('GITHUB_PR_BASE_ALLOW', env.GITHUB_PR_BASE_ALLOW, repos, problems); + const mergeBaseAllow = readBaseAllow('GITHUB_MERGE_BASE_ALLOW', env.GITHUB_MERGE_BASE_ALLOW, repos, problems); + const workflowAllow = readBaseAllow('GITHUB_WORKFLOW_ALLOW', env.GITHUB_WORKFLOW_ALLOW, repos, problems); + for (const entry of workflowAllow) { + const file = entry.slice(entry.indexOf(':') + 1); + if (file && !isWorkflowFileName(file)) { + problems.push(`GITHUB_WORKFLOW_ALLOW: "${entry}" is not a workflow file name such as review.yml`); } } @@ -1220,6 +1390,8 @@ export function readConfig(env: Env = process.env): Config { pollIntervalMs, repos, prBaseAllow, + mergeBaseAllow, + workflowAllow, stateFile: env.GITHUB_STATE_FILE?.trim() || null, botLogin, maintainers: csv(env.GITHUB_MAINTAINERS), diff --git a/test/githubAdapter.test.ts b/test/githubAdapter.test.ts index 519fff5..9c34ad9 100644 --- a/test/githubAdapter.test.ts +++ b/test/githubAdapter.test.ts @@ -980,6 +980,164 @@ test('pull request create, retarget, reviewers — and the draft flag through Gr assertNoTokenLeak(r); }); +test('merge, update-branch and branch deletion hit their REST paths with the head sha guard', async () => { + const r = await rig(); + const p = '/repos/anima-research/rayforce'; + r.fake + .on(`PUT ${p}/pulls/12/merge`, { body: { merged: true, sha: 'beef'.padEnd(40, '0'), message: 'Pull Request successfully merged' } }) + .on(`PUT ${p}/pulls/12/update-branch`, { status: 202, body: { message: 'Updating pull request branch.' } }) + .on(`DELETE ${p}/git/refs/heads%2Ffix%2Fthing`, { status: 204 }) + .on(`DELETE ${p}/git/refs/heads/fix/thing`, { status: 204 }); + + const merged = await r.adapter.mergePullRequest(PULL, { method: 'squash', sha: HEAD_SHA, title: 'Fix it (#12)' }); + assert.deepEqual(r.fake.for(`PUT ${p}/pulls/12/merge`)[0].body, { + merge_method: 'squash', + sha: HEAD_SHA, + commit_title: 'Fix it (#12)', + }); + assert.deepEqual(merged, { merged: true, sha: 'beef'.padEnd(40, '0'), message: 'Pull Request successfully merged' }); + + await r.adapter.updatePullRequestBranch(PULL, HEAD_SHA); + assert.deepEqual(r.fake.for(`PUT ${p}/pulls/12/update-branch`)[0].body, { expected_head_sha: HEAD_SHA }); + await r.adapter.updatePullRequestBranch(PULL); + assert.deepEqual(r.fake.for(`PUT ${p}/pulls/12/update-branch`)[1].body, {}, 'no guard when none was given'); + + await r.adapter.deleteBranch(REPO, 'fix/thing'); + const deletes = r.fake.calls.filter((c) => c.method === 'DELETE' && c.path.startsWith(`${p}/git/refs/`)); + assert.equal(deletes.length, 1); + // The branch name may arrive with its slashes encoded or not; it must name + // exactly refs/heads/fix/thing either way. + assert.equal(decodeURIComponent(deletes[0].path), `${p}/git/refs/heads/fix/thing`); + assertNoTokenLeak(r); +}); + +test('a 405 from the merge endpoint is the caller’s error, with GitHub’s reason', async () => { + const r = await rig(); + r.fake.on('PUT /repos/anima-research/rayforce/pulls/12/merge', { + status: 405, + body: { message: 'Required status check "ci" is expected.' }, + }); + await assert.rejects(r.adapter.mergePullRequest(PULL, { method: 'merge', sha: HEAD_SHA }), (err: unknown) => { + assert.ok(err instanceof GitHubApiError); + assert.equal(err.status, 405); + assert.equal(err.operation, 'mergePullRequest'); + assert.match(err.message, /Required status check/); + return true; + }); +}); + +test('listReviews pages to the end, so the newest verdict is never cut off', async () => { + const r = await rig(); + const review = (id: number, state: string) => ({ + id, + user: { login: id % 2 ? 'github-actions[bot]' : 'maintainer', type: id % 2 ? 'Bot' : 'User' }, + state, + body: `review ${id}`, + commit_id: HEAD_SHA, + submitted_at: '2026-10-01T00:00:00Z', + html_url: `https://github.com/x#pullrequestreview-${id}`, + }); + r.fake.on('GET /repos/anima-research/rayforce/pulls/12/reviews', (req) => + req.query.page === '1' + ? { body: Array.from({ length: 100 }, (_, i) => review(i + 1, 'COMMENTED')) } + : { body: [review(101, 'DISMISSED'), review(102, 'APPROVED')] }, + ); + const reviews = await r.adapter.listReviews(PULL); + const asked = r.fake.for('GET /repos/anima-research/rayforce/pulls/12/reviews'); + assert.deepEqual( + asked.map((c) => [c.query.per_page, c.query.page]), + [ + ['100', '1'], + ['100', '2'], + ], + ); + assert.equal(reviews.length, 102); + assert.deepEqual(reviews.at(-1), { + id: 102, + author: { login: 'maintainer', isBot: false }, + state: 'approved', + body: 'review 102', + commitId: HEAD_SHA, + submittedAt: '2026-10-01T00:00:00Z', + html_url: 'https://github.com/x#pullrequestreview-102', + }); + assert.equal(reviews.at(-2)?.state, 'dismissed'); +}); + +test('workflow runs: list by repo or by workflow file, read one, dispatch, re-run', async () => { + const r = await rig(); + const p = '/repos/anima-research/rayforce/actions'; + const run = { + id: 9001, + name: 'Review', + path: '.github/workflows/review.yml', + event: 'workflow_dispatch', + status: 'completed', + conclusion: 'success', + head_branch: 'main', + head_sha: HEAD_SHA, + run_attempt: 2, + created_at: '2026-10-02T00:00:00Z', + html_url: 'https://github.com/anima-research/rayforce/actions/runs/9001', + }; + r.fake + .on(`GET ${p}/runs`, { body: { total_count: 1, workflow_runs: [run] } }) + .on(`GET ${p}/workflows/review.yml/runs`, { body: { total_count: 1, workflow_runs: [run] } }) + .on(`GET ${p}/runs/9001`, { body: run }) + .on(`POST ${p}/workflows/review.yml/dispatches`, { status: 204 }) + .on(`POST ${p}/runs/9001/rerun`, { status: 201, body: {} }) + .on(`POST ${p}/runs/9001/rerun-failed-jobs`, { status: 201, body: {} }); + + const all = await r.adapter.listWorkflowRuns(REPO, { headSha: HEAD_SHA, limit: 5 }); + assert.deepEqual(r.fake.for(`GET ${p}/runs`)[0].query, { per_page: '5', head_sha: HEAD_SHA }); + assert.deepEqual(all, [ + { + id: 9001, + name: 'Review', + path: '.github/workflows/review.yml', + event: 'workflow_dispatch', + status: 'completed', + conclusion: 'success', + headBranch: 'main', + headSha: HEAD_SHA, + runAttempt: 2, + createdAt: '2026-10-02T00:00:00Z', + html_url: 'https://github.com/anima-research/rayforce/actions/runs/9001', + }, + ]); + + await r.adapter.listWorkflowRuns(REPO, { workflow: 'review.yml', branch: 'main', event: 'workflow_dispatch', limit: 500 }); + assert.deepEqual(r.fake.for(`GET ${p}/workflows/review.yml/runs`)[0].query, { + per_page: '50', + branch: 'main', + event: 'workflow_dispatch', + }); + + assert.equal((await r.adapter.getWorkflowRun(REPO, 9001)).path, '.github/workflows/review.yml'); + + await r.adapter.dispatchWorkflow(REPO, { workflow: 'review.yml', ref: 'main', inputs: { 'pr-number': '12' } }); + assert.deepEqual(r.fake.for(`POST ${p}/workflows/review.yml/dispatches`)[0].body, { + ref: 'main', + inputs: { 'pr-number': '12' }, + }); + + await r.adapter.rerunWorkflowRun(REPO, 9001, false); + await r.adapter.rerunWorkflowRun(REPO, 9001, true); + assert.equal(r.fake.for(`POST ${p}/runs/9001/rerun`).length, 1); + assert.equal(r.fake.for(`POST ${p}/runs/9001/rerun-failed-jobs`).length, 1); + assertNoTokenLeak(r); +}); + +test('a pull request carries the repository its head branch lives in', async () => { + const r = await rig(); + r.fake.on('GET /repos/anima-research/rayforce/pulls/12', { + body: { ...PULL_PAYLOAD, head: { ...PULL_PAYLOAD.head, repo: { name: 'rayforce', owner: { login: 'a-fork' } } } }, + }); + assert.deepEqual((await r.adapter.getPullRequest(PULL)).headRepo, { owner: 'a-fork', name: 'rayforce' }); + r.fake.on('GET /repos/anima-research/rayforce/pulls/12', { body: { ...PULL_PAYLOAD, head: { ...PULL_PAYLOAD.head, repo: null } } }); + assert.equal((await r.adapter.getPullRequest(PULL)).headRepo, null, 'a deleted fork is null, not a guess'); +}); + // ───────────────────────────────────────────────────────────────────────────── // Polling fallback (contract amendment 1) // ───────────────────────────────────────────────────────────────────────────── diff --git a/test/harness.ts b/test/harness.ts index e2afa12..5e7010b 100644 --- a/test/harness.ts +++ b/test/harness.ts @@ -23,7 +23,9 @@ import { } from '@animalabs/mcpl-core'; import { GitHubMcplServer, type GitHubServerOptions } from '../src/server.js'; import { commentsToolModule } from '../src/tools-comments.js'; +import { actionsToolModule } from '../src/tools-actions.js'; import { issuesToolModule, prToolModule } from '../src/tools-issues-pr.js'; +import { mergeToolModule } from '../src/tools-merge.js'; import { readToolModule } from '../src/tools-read.js'; import { reviewToolModule } from '../src/tools-review.js'; import { @@ -35,18 +37,24 @@ import { type CreateIssueInput, type CreatePullRequestInput, type CreateReviewInput, + type DispatchWorkflowInput, type EventActor, type GitHubApi, type Issue, type IssueSearchHit, + type ListWorkflowRunsInput, + type MergePullRequestInput, + type MergeResult, type PolledComment, type PolledIssue, type PollRead, type PullRequest, type PullRequestFile, + type PullRequestReview, type RepoRef, type ReviewThread, type SubmittedReview, + type WorkflowRun, type ThreadRef, type ToolResult, type UpdateIssueInput, @@ -98,6 +106,18 @@ export class FakeGitHubApi implements GitHubApi { private readonly files = new Map(); private readonly checks = new Map(); private readonly reviews: SubmittedReview[] = []; + private readonly reviewLists = new Map(); + private readonly runs = new Map(); + /** Branches `deleteBranch` removed, as `owner/name:branch`. */ + readonly deletedBranches: string[] = []; + /** Every `dispatchWorkflow`, in order. */ + readonly dispatches: Array<{ repo: RepoRef; input: DispatchWorkflowInput }> = []; + /** Every `rerunWorkflowRun`, in order. */ + readonly reruns: Array<{ repo: RepoRef; runId: number; failedOnly: boolean }> = []; + /** What the next `mergePullRequest` answers instead of merging, when set (GitHub's `merged: false`). */ + mergeRefusal: string | null = null; + /** What the next `deleteBranch` throws instead, when set (a 422 from a repo that already deleted it). */ + deleteBranchError: Error | null = null; private nextNumber = 1; private nextId = 1000; private lastStamp = 0; @@ -242,6 +262,42 @@ export class FakeGitHubApi implements GitHubApi { this.checks.set(`${repoSlug(repo)}@${summary.sha}`, summary); } + /** The reviews `listReviews` answers for one pull request, oldest first. */ + seedReviews(pull: ThreadRef, reviews: Array>): PullRequestReview[] { + const list = reviews.map((partial, i) => ({ + id: partial.id ?? this.nextId++, + author: humanActor('human-one'), + state: 'commented' as const, + body: '', + commitId: `head${pull.number}`.padEnd(40, '0'), + submittedAt: new Date(Date.parse(T0) + i * 1000).toISOString(), + html_url: `https://github.com/${repoSlug(pull.repo)}/pull/${pull.number}#pullrequestreview-${i}`, + ...partial, + })); + this.reviewLists.set(key(pull.repo, pull.number), list); + return list; + } + + /** The workflow runs of one repository, newest first as GitHub lists them. */ + seedWorkflowRuns(repo: RepoRef, runs: Array>): WorkflowRun[] { + const list = runs.map((partial) => ({ + id: partial.id ?? this.nextId++, + name: 'CI', + path: '.github/workflows/ci.yml', + event: 'pull_request', + status: 'completed', + conclusion: 'success', + headBranch: 'feature', + headSha: 'f'.repeat(40), + runAttempt: 1, + createdAt: T0, + html_url: `https://github.com/${repoSlug(repo)}/actions/runs/${partial.id ?? 0}`, + ...partial, + })); + this.runs.set(repoSlug(repo), list); + return list; + } + /** The submitted reviews, in order — createReview has no read method. */ submittedReviews(): SubmittedReview[] { return [...this.reviews]; @@ -550,6 +606,80 @@ export class FakeGitHubApi implements GitHubApi { }); } + // ── Reviews, merge, actions ── + + async listReviews(pull: ThreadRef): Promise { + this.record('listReviews', pull); + if (!this.pulls.has(key(pull.repo, pull.number))) { + throw new Error(`no such pull request: ${key(pull.repo, pull.number)}`); + } + return [...(this.reviewLists.get(key(pull.repo, pull.number)) ?? [])]; + } + + async mergePullRequest(pull: ThreadRef, input: MergePullRequestInput): Promise { + this.record('mergePullRequest', pull, input); + const pr = this.pulls.get(key(pull.repo, pull.number)); + if (!pr) throw new Error(`no such pull request: ${key(pull.repo, pull.number)}`); + if (this.mergeRefusal !== null) { + const message = this.mergeRefusal; + this.mergeRefusal = null; + return { merged: false, sha: '', message }; + } + if (input.sha !== pr.head.sha) { + throw Object.assign(new Error('Head branch was modified. Review and try the merge again.'), { status: 409 }); + } + pr.merged = true; + pr.state = 'closed'; + pr.updatedAt = new Date().toISOString(); + return { merged: true, sha: 'merge'.padEnd(40, '0'), message: 'Pull Request successfully merged' }; + } + + async updatePullRequestBranch(pull: ThreadRef, expectedHeadSha?: string): Promise { + this.record('updatePullRequestBranch', pull, expectedHeadSha); + const pr = this.pulls.get(key(pull.repo, pull.number)); + if (!pr) throw new Error(`no such pull request: ${key(pull.repo, pull.number)}`); + if (expectedHeadSha !== undefined && expectedHeadSha !== pr.head.sha) { + throw Object.assign(new Error('expected head sha didn’t match current head ref.'), { status: 422 }); + } + } + + async deleteBranch(repo: RepoRef, branch: string): Promise { + this.record('deleteBranch', repo, branch); + if (this.deleteBranchError !== null) { + const err = this.deleteBranchError; + this.deleteBranchError = null; + throw err; + } + this.deletedBranches.push(`${repoSlug(repo)}:${branch}`); + } + + async listWorkflowRuns(repo: RepoRef, input: ListWorkflowRunsInput): Promise { + this.record('listWorkflowRuns', repo, input); + return (this.runs.get(repoSlug(repo)) ?? []) + .filter((r) => input.headSha === undefined || r.headSha === input.headSha) + .filter((r) => input.branch === undefined || r.headBranch === input.branch) + .filter((r) => input.event === undefined || r.event === input.event) + .filter((r) => input.workflow === undefined || r.path.endsWith(`/${input.workflow}`)) + .slice(0, input.limit); + } + + async getWorkflowRun(repo: RepoRef, runId: number): Promise { + this.record('getWorkflowRun', repo, runId); + const run = (this.runs.get(repoSlug(repo)) ?? []).find((r) => r.id === runId); + if (!run) throw Object.assign(new Error('Not Found'), { status: 404 }); + return run; + } + + async dispatchWorkflow(repo: RepoRef, input: DispatchWorkflowInput): Promise { + this.record('dispatchWorkflow', repo, input); + this.dispatches.push({ repo, input }); + } + + async rerunWorkflowRun(repo: RepoRef, runId: number, failedOnly: boolean): Promise { + this.record('rerunWorkflowRun', repo, runId, failedOnly); + this.reruns.push({ repo, runId, failedOnly }); + } + // ── Polling fallback (amendment 1) and identity ── private inRepo(repo: RepoRef, threadKey: string): boolean { @@ -630,7 +760,7 @@ export interface HarnessOptions extends GitHubServerOptions { /** What the host answers to push/event (default true). */ pushAccepted?: boolean; /** - * Register the five tool modules, as `src/index.ts` does. Default true; a + * Register the seven tool modules, as `src/index.ts` does. Default true; a * test that wants the bare server (the .1 smoke test) passes false. */ tools?: boolean; @@ -674,16 +804,20 @@ export function harness(opts: HarnessOptions = {}): Harness { repos: opts.repos ?? [TEST_REPO], maintainers: opts.maintainers, prBaseAllow: opts.prBaseAllow, + mergeBaseAllow: opts.mergeBaseAllow, + workflowAllow: opts.workflowAllow, state: opts.state, logger: opts.logger, }); if (opts.tools !== false) { - // The same five src/index.ts registers; github.events carries no tools. + // The same seven src/index.ts registers; github.events carries no tools. server.register(readToolModule); server.register(commentsToolModule); server.register(reviewToolModule); server.register(issuesToolModule); server.register(prToolModule); + server.register(mergeToolModule); + server.register(actionsToolModule); } const hostSaw: JsonRpcRequest[] = []; diff --git a/test/integration.test.ts b/test/integration.test.ts index 0e342a9..a8d12bb 100644 --- a/test/integration.test.ts +++ b/test/integration.test.ts @@ -130,7 +130,7 @@ const ERR_UNKNOWN_CHECKPOINT = -32005; // The handshake // ───────────────────────────────────────────────────────────────────────────── -test('the MCPL handshake answers the six feature-set declarations and MCP tools', async (t) => { +test('the MCPL handshake answers the eight feature-set declarations and MCP tools', async (t) => { const h = rig(t); const result = await initialize(h, true); const manifest = (result.capabilities as { experimental: { mcpl: Record } }).experimental.mcpl; @@ -145,6 +145,8 @@ test('the MCPL handshake answers the six feature-set declarations and MCP tools' 'github.review', 'github.issues', 'github.pr', + 'github.merge', + 'github.actions', ]); assert.equal(manifest.pushEvents, true); assert.deepEqual(manifest.channels, { @@ -232,7 +234,7 @@ test('a host that grants nothing gets refusals on every tool, channel and push', const receipt = await grant(h, []); assert.equal(receipt.mode, 'degraded'); - assert.equal(receipt.unavailableFeatures.length, 6, 'all six disabled'); + assert.equal(receipt.unavailableFeatures.length, 8, 'all eight disabled'); // Every tool, by name, from every module. assert.ok(h.server.toolDefinitions().length >= 20, 'the whole tool surface is registered'); @@ -259,7 +261,7 @@ test('a host that grants nothing gets refusals on every tool, channel and push', // "a host has to be able to see the surface it is deciding about" — which // was wrong twice over (WP9 review, finding 6): the surface a host decides // about is the *manifest's* feature-set declarations, which `initialize` - // already carried, and a listing that offers 22 tools while every call is + // already carried, and a listing that offers 28 tools while every call is // refused is a listing that lies. const listed = (await h.host.sendRequest('tools/list', {})) as { tools: { name: string }[] }; assert.deepEqual(listed.tools, [], 'the empty allowlist offers no tool'); @@ -270,10 +272,10 @@ test('an unknown tool name is refused by name, before the grant is consulted', a await initialize(h, true); // No grant at all, so a capability answer would be the "available" one — // and a typo is still reported as a typo (§ the .9 brief). - const unknown = await refusal(callTool(h, 'merge_pull_request', {})); + const unknown = await refusal(callTool(h, 'force_push', {})); assert.equal(unknown.code, ERR_METHOD_NOT_FOUND); - assert.match(unknown.message, /Unknown tool: merge_pull_request/); - assert.deepEqual(unknown.data, { tool: 'merge_pull_request' }); + assert.match(unknown.message, /Unknown tool: force_push/); + assert.deepEqual(unknown.data, { tool: 'force_push' }); }); // ───────────────────────────────────────────────────────────────────────────── diff --git a/test/smoke.test.ts b/test/smoke.test.ts index 8f500e0..30ad94f 100644 --- a/test/smoke.test.ts +++ b/test/smoke.test.ts @@ -182,6 +182,9 @@ test('readConfig validates a whole environment', () => { assert.equal(config.webhookPort, 8443); assert.equal(config.pollIntervalMs, 60000); assert.deepEqual(config.prBaseAllow, ['dev']); + // Merging and workflow dispatch are off unless named. + assert.deepEqual(config.mergeBaseAllow, []); + assert.deepEqual(config.workflowAllow, []); assert.deepEqual(config.maintainers, ['human-one', 'human-two']); assert.equal(config.installationId, null); // Both files were written with a trailing "\n", as `jq -r` writes them. @@ -264,6 +267,46 @@ test('readConfig accepts several repositories of one owner, and a per-repository assert.match(problems({ GITHUB_REPOS: 'anima-research/rayforce', GITHUB_PR_BASE_ALLOW: 'rayforce:' })[0], /neither a branch nor/); }); +test('GITHUB_MERGE_BASE_ALLOW and GITHUB_WORKFLOW_ALLOW parse like the base allow-list', () => { + const dir = mkdtempSync(join(tmpdir(), 'github-mcpl-')); + const pem = join(dir, 'app.pem'); + writeFileSync(pem, 'NOT-A-REAL-KEY placeholder for tests\n', { mode: 0o600 }); + const base = { + GITHUB_APP_ID: '12345', + GITHUB_APP_PRIVATE_KEY_FILE: pem, + GITHUB_BOT_LOGIN: 'rayforce-clone[bot]', + GITHUB_REPOS: 'anima-research/rayforce, anima-research/chronicle', + }; + const config = readConfig({ + ...base, + GITHUB_MERGE_BASE_ALLOW: 'main, anima-research/rayforce:dev', + GITHUB_WORKFLOW_ALLOW: 'review.yml, anima-research/chronicle:ci.yaml', + }); + assert.deepEqual(allowedBases(TEST_REPO, config.mergeBaseAllow), ['main', 'dev']); + assert.deepEqual(allowedBases({ owner: 'anima-research', name: 'chronicle' }, config.mergeBaseAllow), ['main']); + assert.deepEqual(allowedBases(TEST_REPO, config.workflowAllow), ['review.yml']); + assert.deepEqual(allowedBases({ owner: 'anima-research', name: 'chronicle' }, config.workflowAllow), [ + 'review.yml', + 'ci.yaml', + ]); + + const problems = (env: Record): string[] => { + try { + readConfig({ ...base, ...env }); + } catch (err) { + assert.ok(err instanceof ConfigError); + return err.problems; + } + assert.fail('expected a ConfigError'); + }; + assert.match(problems({ GITHUB_MERGE_BASE_ALLOW: 'other-org/x:main' })[0], /^GITHUB_MERGE_BASE_ALLOW: .*outside GITHUB_REPOS/); + assert.match(problems({ GITHUB_WORKFLOW_ALLOW: 'anima-research/gate:ci.yml' })[0], /^GITHUB_WORKFLOW_ALLOW: .*outside GITHUB_REPOS/); + // A workflow is named by its file, so a path, an id or a name is a startup error. + for (const bad of ['.github/workflows/ci.yml', '12345', 'Review', 'ci.yml.bak']) { + assert.match(problems({ GITHUB_WORKFLOW_ALLOW: bad })[0], /not a workflow file name/, bad); + } +}); + test('readConfig names every problem at once', () => { assert.throws( () => readConfig({ GITHUB_REPOS: 'rayforce', GITHUB_WEBHOOK_PORT: '0' }), @@ -344,8 +387,10 @@ test('every tool belongs to exactly one feature set', () => { assert.equal(featureSetForTool(tool), featureSet); } } - assert.equal(seen.size, 22); - assert.equal(featureSetForTool('merge_pull_request'), undefined); + assert.equal(seen.size, 28); + assert.equal(featureSetForTool('merge_pull_request'), 'github.merge'); + assert.equal(featureSetForTool('dispatch_workflow'), 'github.actions'); + assert.equal(featureSetForTool('force_push'), undefined); }); test('the tag vocabulary is the contract\'s', () => { diff --git a/test/toolsActions.test.ts b/test/toolsActions.test.ts new file mode 100644 index 0000000..608a928 --- /dev/null +++ b/test/toolsActions.test.ts @@ -0,0 +1,184 @@ +/** + * github.actions — list_workflow_runs, dispatch_workflow, rerun_workflow_run. + * + * Two fences, both deny-by-default and both asserted here: a workflow outside + * GITHUB_WORKFLOW_ALLOW is never dispatched or re-run, and a dispatch only + * ever runs on a base branch. Everything runs against FakeGitHubApi. + */ + +import { strict as assert } from 'node:assert'; +import { test } from 'node:test'; +import { actionsToolModule } from '../src/tools-actions.js'; +import { + ACTIONS_FEATURE_SET, + silentLogger, + type StateStore, + type ToolContext, + type ToolResult, +} from '../src/types.js'; +import { FakeGitHubApi, TEST_BOT, TEST_REPO } from './harness.js'; + +function ctx(api: FakeGitHubApi, over: Partial = {}): ToolContext { + return { + api, + state: undefined as unknown as StateStore, + botLogin: TEST_BOT.login, + repos: [TEST_REPO], + prBaseAllow: ['dev'], + mergeBaseAllow: [], + workflowAllow: ['review.yml'], + logger: silentLogger(), + ...over, + }; +} + +function text(result: ToolResult): string { + assert.equal(result.content.length, 1); + return (result.content[0] as { type: 'text'; text: string }).text; +} + +function ok(result: ToolResult): string { + assert.equal(result.isError, undefined, `expected success, got: ${text(result)}`); + return text(result); +} + +function failure(result: ToolResult): string { + assert.equal(result.isError, true, `expected an error result, got: ${text(result)}`); + return text(result); +} + +const HEAD = 'head8'.padEnd(40, '0'); + +function seeded(): FakeGitHubApi { + const api = new FakeGitHubApi(); + api.seedPullRequest({ number: 8 }); + api.seedWorkflowRuns(TEST_REPO, [ + { id: 301, name: 'Review', path: '.github/workflows/review.yml', event: 'workflow_dispatch', headBranch: 'dev', headSha: 'd'.repeat(40) }, + { id: 302, name: 'CI', path: '.github/workflows/ci.yml', status: 'completed', conclusion: 'failure', headSha: HEAD, headBranch: 'feature-8' }, + { id: 303, name: 'Publish', path: '.github/workflows/publish.yml', event: 'push', headBranch: 'dev' }, + ]); + return api; +} + +test('the module declares github.actions and its three tools', () => { + assert.equal(actionsToolModule.featureSet, ACTIONS_FEATURE_SET); + assert.deepEqual( + actionsToolModule.tools.map((t) => t.name), + ['list_workflow_runs', 'dispatch_workflow', 'rerun_workflow_run'], + ); +}); + +test('list_workflow_runs by pull request reads its head and lists the runs on it', async () => { + const api = seeded(); + const out = ok(await actionsToolModule.call('list_workflow_runs', { number: 8 }, ctx(api))); + assert.match(out, /^1 workflow run on anima-research\/rayforce#8 head head80+, newest first\n/); + assert.match(out, /run 302 CI \(ci\.yml\) \[completed\/failure\] pull_request on feature-8 head80000000/); + assert.deepEqual(api.callsTo('listWorkflowRuns')[0].args[1], { + workflow: undefined, + headSha: HEAD, + branch: undefined, + event: undefined, + limit: 20, + }); +}); + +test('list_workflow_runs filters by workflow file, and is not fenced by the allow-list', async () => { + const api = seeded(); + const out = ok( + await actionsToolModule.call('list_workflow_runs', { workflow: 'publish.yml', limit: 500 }, ctx(api, { workflowAllow: [] })), + ); + assert.match(out, /run 303 Publish \(publish\.yml\)/); + assert.equal((api.callsTo('listWorkflowRuns')[0].args[1] as { limit: number }).limit, 50, 'capped at the list bound'); +}); + +test('list_workflow_runs refuses number and sha together, and a workflow path', async () => { + const api = seeded(); + assert.match(failure(await actionsToolModule.call('list_workflow_runs', { number: 8, sha: HEAD }, ctx(api))), /give "number" or "sha", not both/); + assert.match( + failure(await actionsToolModule.call('list_workflow_runs', { workflow: '.github/workflows/ci.yml' }, ctx(api))), + /"workflow" must be a workflow file name/, + ); + assert.equal( + ok(await actionsToolModule.call('list_workflow_runs', { sha: 'nothing' }, ctx(api))), + 'no workflow runs on anima-research/rayforce at nothing', + ); +}); + +test('dispatch_workflow runs an allowed workflow on the only base branch, with string inputs', async () => { + const api = seeded(); + const out = ok( + await actionsToolModule.call('dispatch_workflow', { workflow: 'review.yml', inputs: { 'pr-number': 8, verbose: true } }, ctx(api)), + ); + assert.match(out, /^dispatched review\.yml on dev of anima-research\/rayforce with pr-number="8", verbose="true"/); + assert.deepEqual(api.dispatches, [ + { repo: TEST_REPO, input: { workflow: 'review.yml', ref: 'dev', inputs: { 'pr-number': '8', verbose: 'true' } } }, + ]); +}); + +test('dispatch_workflow refuses a workflow outside GITHUB_WORKFLOW_ALLOW', async () => { + const api = seeded(); + const out = failure(await actionsToolModule.call('dispatch_workflow', { workflow: 'publish.yml' }, ctx(api))); + assert.match(out, /dispatch_workflow refused: workflow "publish\.yml" is not in GITHUB_WORKFLOW_ALLOW \(allowed on anima-research\/rayforce: review\.yml\)/); + const none = failure(await actionsToolModule.call('dispatch_workflow', { workflow: 'review.yml' }, ctx(api, { workflowAllow: [] }))); + assert.match(none, /GITHUB_WORKFLOW_ALLOW allows no workflow on anima-research\/rayforce/); + assert.deepEqual(api.dispatches, []); +}); + +test('dispatch_workflow runs only on a base branch, and asks when there are several', async () => { + const api = seeded(); + const feature = failure(await actionsToolModule.call('dispatch_workflow', { workflow: 'review.yml', ref: 'feature-8' }, ctx(api))); + assert.match(feature, /ref "feature-8" is not a base branch on anima-research\/rayforce \(allowed: dev\)/); + + const several = ctx(api, { prBaseAllow: ['dev'], mergeBaseAllow: ['main'] }); + assert.match(failure(await actionsToolModule.call('dispatch_workflow', { workflow: 'review.yml' }, several)), /"ref" is required: several base branches .*\(dev, main\)/); + ok(await actionsToolModule.call('dispatch_workflow', { workflow: 'review.yml', ref: 'main' }, several)); + assert.deepEqual(api.dispatches.map((d) => d.input.ref), ['main']); + + const noBase = failure(await actionsToolModule.call('dispatch_workflow', { workflow: 'review.yml' }, ctx(api, { prBaseAllow: [] }))); + assert.match(noBase, /no base branch is allowed on anima-research\/rayforce/); +}); + +test('dispatch_workflow validates its inputs', async () => { + const api = seeded(); + const call = (inputs: unknown) => actionsToolModule.call('dispatch_workflow', { workflow: 'review.yml', inputs }, ctx(api)); + assert.match(failure(await call(['8'])), /"inputs" must be an object/); + assert.match(failure(await call({ nested: { a: 1 } })), /"inputs\.nested" must be a string, number or boolean/); + assert.match(failure(await call(Object.fromEntries(Array.from({ length: 26 }, (_, i) => [`k${i}`, 'v'])))), /at most 25/); + assert.match(failure(await call({ big: 'x'.repeat(1001) })), /"inputs\.big" is 1001 characters/); + assert.deepEqual(api.dispatches, []); +}); + +test('rerun_workflow_run re-runs a run of an allowed workflow, all jobs or only the failed ones', async () => { + const api = seeded(); + const all = ok(await actionsToolModule.call('rerun_workflow_run', { run_id: 301 }, ctx(api))); + assert.match(all, /^re-run of run 301 \(Review, review\.yml\) on anima-research\/rayforce requested/); + const failed = ok( + await actionsToolModule.call('rerun_workflow_run', { run_id: 302, failed_only: true }, ctx(api, { workflowAllow: ['review.yml', 'ci.yml'] })), + ); + assert.match(failed, /^re-run of the failed jobs of run 302 \(CI, ci\.yml\)/); + assert.deepEqual(api.reruns, [ + { repo: TEST_REPO, runId: 301, failedOnly: false }, + { repo: TEST_REPO, runId: 302, failedOnly: true }, + ]); +}); + +test('rerun_workflow_run refuses a run of a workflow outside the allow-list', async () => { + const api = seeded(); + const out = failure(await actionsToolModule.call('rerun_workflow_run', { run_id: 303 }, ctx(api))); + assert.match(out, /workflow "publish\.yml" is not in GITHUB_WORKFLOW_ALLOW/); + assert.deepEqual(api.reruns, []); + const missing = failure(await actionsToolModule.call('rerun_workflow_run', { run_id: 999 }, ctx(api))); + assert.match(missing, /rerun_workflow_run failed: GitHub error 404/); +}); + +test('a scoped allow-list entry applies to its repository only', async () => { + const api = seeded(); + const other = { owner: 'anima-research', name: 'chronicle' }; + const scoped = ctx(api, { repos: [TEST_REPO, other], workflowAllow: ['anima-research/chronicle:review.yml'] }); + const out = failure( + await actionsToolModule.call('dispatch_workflow', { repo: 'anima-research/rayforce', workflow: 'review.yml' }, scoped), + ); + assert.match(out, /GITHUB_WORKFLOW_ALLOW allows no workflow on anima-research\/rayforce/); + ok(await actionsToolModule.call('dispatch_workflow', { repo: 'anima-research/chronicle', workflow: 'review.yml' }, scoped)); + assert.deepEqual(api.dispatches.map((d) => d.repo), [other]); +}); diff --git a/test/toolsMerge.test.ts b/test/toolsMerge.test.ts new file mode 100644 index 0000000..ff6c5eb --- /dev/null +++ b/test/toolsMerge.test.ts @@ -0,0 +1,218 @@ +/** + * 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. + */ + +import { strict as assert } from 'node:assert'; +import { test } from 'node:test'; +import { mergeToolModule } from '../src/tools-merge.js'; +import { + MERGE_FEATURE_SET, + silentLogger, + type StateStore, + type ThreadRef, + type ToolContext, + type ToolResult, +} from '../src/types.js'; +import { FakeGitHubApi, TEST_BOT, TEST_REPO } from './harness.js'; + +function ctx(api: FakeGitHubApi, over: Partial = {}): ToolContext { + return { + api, + // github.merge is not rollback-capable and never touches the store. + state: undefined as unknown as StateStore, + botLogin: TEST_BOT.login, + repos: [TEST_REPO], + prBaseAllow: ['dev'], + mergeBaseAllow: ['dev'], + logger: silentLogger(), + ...over, + }; +} + +function text(result: ToolResult): string { + assert.equal(result.content.length, 1); + return (result.content[0] as { type: 'text'; text: string }).text; +} + +function ok(result: ToolResult): string { + assert.equal(result.isError, undefined, `expected success, got: ${text(result)}`); + return text(result); +} + +function failure(result: ToolResult): string { + assert.equal(result.isError, true, `expected an error result, got: ${text(result)}`); + return text(result); +} + +const PULL: ThreadRef = { repo: TEST_REPO, number: 8, kind: 'pull' }; +const HEAD = 'head8'.padEnd(40, '0'); + +function seeded(): FakeGitHubApi { + const api = new FakeGitHubApi(); + api.seedPullRequest({ number: 8, headRepo: TEST_REPO }); + return api; +} + +test('the module declares github.merge and its two tools', () => { + assert.equal(mergeToolModule.featureSet, MERGE_FEATURE_SET); + assert.deepEqual( + mergeToolModule.tools.map((t) => t.name), + ['merge_pull_request', 'update_branch'], + ); +}); + +test('merge_pull_request merges at the head sha it read, by merge commit by default', async () => { + const api = seeded(); + const out = ok(await mergeToolModule.call('merge_pull_request', { number: 8, sha: HEAD }, ctx(api))); + assert.match(out, /^merged anima-research\/rayforce#8 \(feature-8 at head80+\) into dev by merge: merge0+$/); + assert.deepEqual(api.callsTo('mergePullRequest')[0].args, [ + PULL, + { method: 'merge', sha: HEAD, title: undefined, message: undefined }, + ]); + assert.deepEqual(api.deletedBranches, [], 'no branch deleted unless asked'); +}); + +test('merge_pull_request passes squash with a title and message', async () => { + const api = seeded(); + ok( + await mergeToolModule.call( + 'merge_pull_request', + { number: 8, method: 'squash', title: 'Fix it (#8)', message: 'body' }, + ctx(api), + ), + ); + assert.deepEqual(api.callsTo('mergePullRequest')[0].args[1], { + method: 'squash', + sha: HEAD, + title: 'Fix it (#8)', + message: 'body', + }); +}); + +test('a rebase takes no commit title or message', async () => { + const api = seeded(); + const out = failure(await mergeToolModule.call('merge_pull_request', { number: 8, method: 'rebase', title: 'x' }, ctx(api))); + assert.match(out, /"title" and "message" apply to merge and squash/); + assert.equal(api.callsTo('mergePullRequest').length, 0); +}); + +test('an empty GITHUB_MERGE_BASE_ALLOW means nothing may be merged', async () => { + for (const over of [{ mergeBaseAllow: [] }, { mergeBaseAllow: undefined }]) { + const api = seeded(); + const out = failure(await mergeToolModule.call('merge_pull_request', { number: 8 }, ctx(api, over))); + assert.match( + out, + /merge_pull_request refused: base branch "dev" is not in GITHUB_MERGE_BASE_ALLOW \(GITHUB_MERGE_BASE_ALLOW allows no branch on anima-research\/rayforce, so nothing there may be merged\)/, + ); + assert.equal(api.callsTo('mergePullRequest').length, 0); + } +}); + +test('a base GITHUB_PR_BASE_ALLOW names is not thereby mergeable', async () => { + const api = seeded(); + const out = failure( + await mergeToolModule.call('merge_pull_request', { number: 8 }, ctx(api, { prBaseAllow: ['dev'], mergeBaseAllow: ['main'] })), + ); + assert.match(out, /base branch "dev" is not in GITHUB_MERGE_BASE_ALLOW \(allowed on anima-research\/rayforce: main\)/); +}); + +test('a head that moved since it was read is refused before GitHub is asked', async () => { + const api = seeded(); + const out = failure(await mergeToolModule.call('merge_pull_request', { number: 8, sha: 'abc123' }, ctx(api))); + assert.match(out, /the head of anima-research\/rayforce#8 is head80+, not "abc123": it moved since it was read/); + assert.equal(api.callsTo('mergePullRequest').length, 0); +}); + +test('draft, closed and merged pull requests are refused by name', async () => { + const cases: Array<[Record, RegExp]> = [ + [{ draft: true }, /#9 is a draft; mark it ready for review/], + [{ state: 'closed' }, /#9 is closed; reopen it before merging/], + [{ state: 'closed', merged: true }, /#9 is already merged/], + ]; + for (const [partial, expected] of cases) { + const api = new FakeGitHubApi(); + api.seedPullRequest({ number: 9, ...partial }); + const out = failure(await mergeToolModule.call('merge_pull_request', { number: 9 }, ctx(api))); + assert.match(out, expected); + assert.equal(api.callsTo('mergePullRequest').length, 0); + } +}); + +test('GitHub answering merged: false is an error result with its message', async () => { + const api = seeded(); + api.mergeRefusal = 'Pull Request is not mergeable'; + const out = failure(await mergeToolModule.call('merge_pull_request', { number: 8 }, ctx(api))); + assert.match(out, /GitHub did not merge anima-research\/rayforce#8: Pull Request is not mergeable/); +}); + +test('a repository outside GITHUB_REPOS is refused before anything is read', async () => { + const api = seeded(); + const out = failure(await mergeToolModule.call('merge_pull_request', { repo: 'anima-research/gate', number: 8 }, ctx(api))); + assert.match(out, /repository anima-research\/gate is not in GITHUB_REPOS/); + assert.deepEqual(api.callLog(), []); +}); + +test('delete_branch deletes the merged head branch of this repository', async () => { + const api = seeded(); + const out = ok(await mergeToolModule.call('merge_pull_request', { number: 8, delete_branch: true }, ctx(api))); + assert.match(out, /\ndeleted branch feature-8$/); + assert.deepEqual(api.deletedBranches, ['anima-research/rayforce:feature-8']); + // Deleted only after the merge answered. + assert.deepEqual(api.callLog().slice(-2), ['mergePullRequest', 'deleteBranch']); +}); + +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/], + [{ 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`. + [ + { headRepo: TEST_REPO, head: { ref: 'main', sha: HEAD } }, + { prBaseAllow: ['dev', 'main'] }, + /head branch kept: "main" is a base branch here/, + ], + [ + { headRepo: TEST_REPO, head: { ref: 'release', sha: HEAD } }, + { mergeBaseAllow: ['dev', 'anima-research/rayforce:release'] }, + /head branch kept: "release" is a base branch here/, + ], + ]; + for (const [partial, over, expected] of cases) { + const api = new FakeGitHubApi(); + api.seedPullRequest({ number: 8, ...partial }); + const out = ok(await mergeToolModule.call('merge_pull_request', { number: 8, delete_branch: true }, ctx(api, over))); + assert.match(out, /^merged /, 'the merge itself still happened'); + assert.match(out, expected); + assert.deepEqual(api.deletedBranches, []); + assert.equal(api.callsTo('deleteBranch').length, 0); + } +}); + +test('a failed branch deletion is reported beside a merge that happened, not as an error', async () => { + const api = seeded(); + api.deleteBranchError = Object.assign(new Error('Reference does not exist'), { status: 422 }); + const out = ok(await mergeToolModule.call('merge_pull_request', { number: 8, delete_branch: true }, ctx(api))); + assert.match(out, /^merged anima-research\/rayforce#8/); + assert.match(out, /head branch not deleted: deleteBranch failed: GitHub error 422: Reference does not exist/); +}); + +test('update_branch asks GitHub to merge the base in, with the expected head when given', async () => { + const api = seeded(); + const out = ok(await mergeToolModule.call('update_branch', { number: 8, sha: HEAD }, ctx(api))); + assert.match(out, /asked GitHub to merge the base of anima-research\/rayforce#8 into its head branch/); + assert.deepEqual(api.callsTo('updatePullRequestBranch')[0].args, [PULL, HEAD]); + + const moved = failure(await mergeToolModule.call('update_branch', { number: 8, sha: 'abc' }, ctx(api))); + assert.match(moved, /update_branch failed: GitHub error 422: expected head sha didn’t match/); +}); + +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/); +}); diff --git a/test/toolsRead.test.ts b/test/toolsRead.test.ts index 03de272..bed513d 100644 --- a/test/toolsRead.test.ts +++ b/test/toolsRead.test.ts @@ -10,7 +10,9 @@ import { strict as assert } from 'node:assert'; import { test } from 'node:test'; import type { TagBehavior, TreatmentRule } from '@animalabs/mcpl-core'; import { buildFeatureSets, buildServerCapabilities, GITHUB_TAG_ONTOLOGY } from '../src/feature-sets.js'; +import { actionsToolModule } from '../src/tools-actions.js'; import { issuesToolModule, prToolModule } from '../src/tools-issues-pr.js'; +import { mergeToolModule } from '../src/tools-merge.js'; import { readToolModule } from '../src/tools-read.js'; import { reviewToolModule } from '../src/tools-review.js'; import { @@ -84,10 +86,10 @@ const PULL: ThreadRef = { repo: TEST_REPO, number: 8, kind: 'pull' }; // Feature sets // ───────────────────────────────────────────────────────────────────────────── -test('six feature sets are declared, with the contract’s rollback flags', () => { +test('eight feature sets are declared, with the contract’s rollback flags', () => { const declared = buildFeatureSets(); assert.deepEqual(Object.keys(declared).sort(), [...FEATURE_SETS].sort()); - assert.equal(Object.keys(declared).length, 6); + assert.equal(Object.keys(declared).length, 8); for (const [name, declaration] of Object.entries(declared)) { assert.ok(declaration.description.length > 0, `${name} needs a description`); @@ -96,7 +98,15 @@ test('six feature sets are declared, with the contract’s rollback flags', () = } // Only github.comments can undo what it did (§8.1). assert.equal(declared[COMMENTS_FEATURE_SET].rollback, true); - for (const name of [EVENTS_FEATURE_SET, READ_FEATURE_SET, REVIEW_FEATURE_SET, ISSUES_FEATURE_SET, PR_FEATURE_SET]) { + for (const name of [ + EVENTS_FEATURE_SET, + READ_FEATURE_SET, + REVIEW_FEATURE_SET, + ISSUES_FEATURE_SET, + PR_FEATURE_SET, + 'github.merge', + 'github.actions', + ]) { assert.equal(declared[name].rollback, false, `${name} is not rollback-capable`); } }); @@ -110,7 +120,7 @@ test('uses names only the paths a call site needs', () => { assert.deepEqual(declared[EVENTS_FEATURE_SET].uses, ['pushEvents']); // channels/publish writes a comment, which is what github.comments authorizes. assert.deepEqual(declared[COMMENTS_FEATURE_SET].uses, ['tools', 'channels.publish']); - for (const name of [READ_FEATURE_SET, REVIEW_FEATURE_SET, ISSUES_FEATURE_SET, PR_FEATURE_SET]) { + for (const name of [READ_FEATURE_SET, REVIEW_FEATURE_SET, ISSUES_FEATURE_SET, PR_FEATURE_SET, 'github.merge', 'github.actions']) { assert.deepEqual(declared[name].uses, ['tools'], `${name} only needs tools`); } // github.events has no tools (the contract), so it must not claim the path. @@ -157,7 +167,14 @@ test('the manifest advertises what the declarations use', () => { }); test('every tool belongs to exactly one feature set', () => { - const modules: ToolModule[] = [readToolModule, issuesToolModule, prToolModule, reviewToolModule]; + const modules: ToolModule[] = [ + readToolModule, + issuesToolModule, + prToolModule, + reviewToolModule, + mergeToolModule, + actionsToolModule, + ]; const owner = new Map(); for (const module of modules) { for (const tool of module.tools) { @@ -186,6 +203,32 @@ test('every tool belongs to exactly one feature set', () => { assert.deepEqual(unimplemented.sort(), [...TOOLS_BY_FEATURE_SET[COMMENTS_FEATURE_SET]].sort()); }); +test('list_reviews ends on the newest review and drops the oldest past the bound', async () => { + const api = new FakeGitHubApi(); + api.seedPullRequest({ number: 8 }); + const last = LIMITS.listEntries + 2; + api.seedReviews( + PULL, + Array.from({ length: last + 1 }, (_, i) => + i === last + ? { id: i + 1, state: 'approved' as const, body: 'LGTM — merge' } + : { id: i + 1, state: 'commented' as const, body: `round ${i + 1}` }, + ), + ); + + const out = ok(await readToolModule.call('list_reviews', { number: 8 }, ctx(api))); + assert.match(out, /^53 reviews on anima-research\/rayforce#8, oldest first/); + assert.match(out, /… 3 older not shown \(list bound 50\)/); + assert.doesNotMatch(out, /— id 3 /, 'the oldest are the ones dropped'); + assert.match(out, /— id 53 approved by human-one .*\nLGTM — merge$/); +}); + +test('list_reviews on a pull request with none says so', async () => { + const api = new FakeGitHubApi(); + api.seedPullRequest({ number: 8 }); + assert.equal(ok(await readToolModule.call('list_reviews', { number: 8 }, ctx(api))), 'no reviews on anima-research/rayforce#8'); +}); + test('every read tool takes an optional repo and declares its required arguments', () => { for (const tool of readToolModule.tools) { assert.ok(tool.inputSchema.properties.repo, `${tool.name} must accept repo`); diff --git a/test/webhook.test.ts b/test/webhook.test.ts index 1aea9c2..309be7a 100644 --- a/test/webhook.test.ts +++ b/test/webhook.test.ts @@ -399,6 +399,8 @@ function config(overrides: Partial = {}): Config { pollIntervalMs: 0, repos: [TEST_REPO], prBaseAllow: ['dev'], + mergeBaseAllow: [], + workflowAllow: [], stateFile: null, botLogin: 'rayforce-clone[bot]', maintainers: [], diff --git a/test/wp8-docs.test.ts b/test/wp8-docs.test.ts index 2ff0b24..c074541 100644 --- a/test/wp8-docs.test.ts +++ b/test/wp8-docs.test.ts @@ -59,12 +59,15 @@ const RECIPES = ['examples/recipe-maintainer-clone.json', 'examples/recipe-contr // ─── 1. the App manifest ───────────────────────────────────────────────────── -test('app-manifest.json lists exactly the permissions the tools need', () => { +test('app-manifest.json lists exactly the permissions the default tools need', () => { const m = json('app-manifest.json'); + // contents: write because GitHub refuses resolveReviewThread to an App + // without it; `actions` and `workflows` are added by hand for the opt-in + // github.actions and github.merge (README, Security). assert.deepEqual(m.default_permissions, { issues: 'write', pull_requests: 'write', - contents: 'read', + contents: 'write', checks: 'read', metadata: 'read', }); From 9127a612455388f85a3d482203d2f6a2262e92d6 Mon Sep 17 00:00:00 2001 From: antra-tess Date: Thu, 22 Oct 2026 23:40:36 -0700 Subject: [PATCH 2/2] Review fixes: update_branch and delete_branch fences, sha prefixes, newest reviews - update_branch writes a merge commit, so it now takes a merge's fences: an open pull request into a GITHUB_MERGE_BASE_ALLOW base, and a head that is not itself a base branch. Granting github.merge alone writes nothing. - delete_branch also asks GitHub: a protected branch, the default branch, or the base of another open pull request is kept, so a back-merge release -> dev never deletes a release branch left out of the allow-lists. - The sha guard accepts a 7+ hex-digit prefix in any case (the tools print 12-character shas) and sends the full head sha; a malformed sha is an argument error, not "moved". - list_reviews reads GraphQL reviews(last: 50) with totalCount, so a long history drops its oldest reviews, never the newest verdict. - Test for the 409 race (no branch deletion after it); the workflow allow-list's errors name workflow files, not branches; README and the manifest no longer claim no tool writes a commit. Co-Authored-By: Claude Opus 5.5 --- README.md | 28 +++-- app-manifest.json | 2 +- changelog.d/merge-and-actions.added.md | 15 +-- src/github-adapter.ts | 131 ++++++++++++--------- src/tools-merge.ts | 150 ++++++++++++++++++------- src/tools-read.ts | 21 ++-- src/types.ts | 57 ++++++++-- test/githubAdapter.test.ts | 68 ++++++----- test/harness.ts | 21 +++- test/smoke.test.ts | 5 + test/toolsMerge.test.ts | 92 +++++++++++++-- 11 files changed, 418 insertions(+), 172 deletions(-) diff --git a/README.md b/README.md index f26a6ee..db21349 100644 --- a/README.md +++ b/README.md @@ -355,21 +355,26 @@ Notes worth knowing before you grant them: and landing one are separate grants. `merge_pull_request` is `github.merge`'s, and it refuses a base outside `GITHUB_MERGE_BASE_ALLOW` (empty = no merge), a draft, a closed or merged pull request, and — given - `sha`, the head you reviewed — a head that has moved since. Branch - protection (required checks, approvals) stays GitHub's to enforce. - `delete_branch: true` deletes the merged head branch only when it lives in - the base repository and is not a base branch any allow-list names. + `sha`, the head you reviewed, full or 7+ hex digits — a head that has moved + since. Branch protection (required checks, approvals) stays GitHub's to + enforce. `delete_branch: true` deletes the merged head branch only when it + 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. - `update_branch` merges the base into a pull request's head branch, as - GitHub's "Update branch" button does. + 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. - `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. The use it was built for: a review workflow that ignores comments from bots still takes a `workflow_dispatch` (`review.yml` with `{"pr-number": "12"}`). `list_workflow_runs` is a read and is not fenced. -- `list_reviews` reads every submitted review to the end — a pull request - that has been through many bot review rounds passes GitHub's first page - quickly, and the newest verdict is on the last one. +- `list_reviews` answers the newest 50 reviews and the total — a pull request + that has been through many bot review rounds passes GitHub's first REST page + quickly, and REST pages oldest first, so it asks GraphQL for the last ones. - Every write tool refuses a repository outside `GITHUB_REPOS`, by name, before the call reaches GitHub. @@ -491,9 +496,10 @@ reach metadata and never reach the model. the agent reads the log by following the URL as a human would, or does without. (RFC-005 references for logs and diffs are on the roadmap, not here.) -- **No push.** The App can merge a pull request and delete its merged head - branch, but no tool writes a commit, creates a branch or touches a branch - outside a pull request. +- **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. - **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/app-manifest.json b/app-manifest.json index bca28c0..caa7fb6 100644 --- a/app-manifest.json +++ b/app-manifest.json @@ -1,6 +1,6 @@ { "name": "rayforce-clone", - "description": "An agent living in one GitHub repository: it wakes on issues, comments, pull requests, reviews and check results, and answers in the thread it was spoken to in. Runs as github-mcpl, an MCPL connector. Merges only into the bases its operator allows; never pushes.", + "description": "An agent living in one GitHub repository: it wakes on issues, comments, pull requests, reviews and check results, and answers in the thread it was spoken to in. Runs as github-mcpl, an MCPL connector. Merges only into the bases its operator allows; writes commits only through GitHub's pull-request merge and update-branch.", "url": "https://example.com/github-mcpl", "public": false, "hook_attributes": { diff --git a/changelog.d/merge-and-actions.added.md b/changelog.d/merge-and-actions.added.md index 5c233f5..3cea953 100644 --- a/changelog.d/merge-and-actions.added.md +++ b/changelog.d/merge-and-actions.added.md @@ -1,13 +1,14 @@ - `github.merge` feature set: `merge_pull_request` (merge, squash or rebase, - at the head sha the caller read, optionally deleting the merged head branch) - and `update_branch`. It merges only into the bases `GITHUB_MERGE_BASE_ALLOW` - names, which is empty by default: granting the feature set alone merges - nothing. Drafts, closed and merged pull requests and a moved head are - refused before GitHub is asked; branch protection stays GitHub's. + 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. - `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 on a base branch. Built for re-requesting a review from a workflow that ignores comments from bots. -- `list_reviews` (`github.read`): every submitted review on a pull request, - read to the last page, ending on the newest verdict. +- `list_reviews` (`github.read`): the newest 50 reviews on a pull request and + how many there are in all, ending on the newest verdict. diff --git a/src/github-adapter.ts b/src/github-adapter.ts index 3ad0985..46b5574 100644 --- a/src/github-adapter.ts +++ b/src/github-adapter.ts @@ -63,7 +63,9 @@ import { type PullRequest, type PullRequestFile, type PullRequestReview, + type BranchFacts, type RepoRef, + type ReviewList, type ReviewState, type ReviewThread, type ReviewThreadComment, @@ -119,14 +121,6 @@ const CHECK_RUN_PAGE = 100; */ const CHECK_RUN_MAX_PAGES = 5; -/** - * `list_reviews` pages: 100 reviews a page, at most this many pages. A pull - * request that has been through many bot review rounds passes GitHub's default - * page of 30 quickly, and the *latest* verdict is what a caller wants, so the - * read goes to the end rather than stopping at the first page. - */ -const REVIEW_PAGE = 100; -const REVIEW_MAX_PAGES = 5; /** * Requests one `…Since` poll makes before it stops, past which it goes on * only to finish a second it is part way through, and leaves the rest for a @@ -324,15 +318,24 @@ export interface CheckRunRow { html_url?: string | null; } -/** One row of `GET /repos/{o}/{r}/pulls/{n}/reviews`. */ -interface ApiReview { - id?: number; - user?: ApiActor | null; - state?: string | null; - body?: string | null; - commit_id?: string | null; - submitted_at?: string | null; - html_url?: string | null; +/** `pullRequest.reviews(last: n)`: the newest reviews and the count of all of them. */ +interface GqlReviewsResponse { + repository?: { + pullRequest?: { + reviews?: { + totalCount?: number | null; + nodes?: Array<{ + databaseId?: number | null; + author?: GqlActor | null; + state?: string | null; + body?: string | null; + commit?: { oid?: string | null } | null; + submittedAt?: string | null; + url?: string | null; + } | null> | null; + } | null; + } | null; + } | null; } /** One row of `GET /repos/{o}/{r}/actions/runs`. */ @@ -426,15 +429,19 @@ function submittedReviewStateOf(state: string | null | undefined): SubmittedRevi return 'commented'; } -function mapReview(data: ApiReview): PullRequestReview { +type GqlReview = NonNullable< + NonNullable['pullRequest']>['reviews']>['nodes'] +>[number]; + +function mapReview(node: NonNullable): PullRequestReview { return { - id: data.id ?? 0, - author: actorOf(data.user), - state: submittedReviewStateOf(data.state), - body: data.body ?? '', - commitId: data.commit_id ?? '', - submittedAt: data.submitted_at ?? '', - html_url: data.html_url ?? '', + id: node.databaseId ?? 0, + author: gqlActorOf(node.author), + state: submittedReviewStateOf(node.state), + body: node.body ?? '', + commitId: node.commit?.oid ?? '', + submittedAt: node.submittedAt ?? '', + html_url: node.url ?? '', }; } @@ -1468,35 +1475,32 @@ export class GitHubAdapter implements GitHubApi { // ─────────────────────────────────────────────────────────────────────────── /** - * Every review on a pull request, oldest first (GitHub's order). Paged to - * the end up to {@link REVIEW_MAX_PAGES} pages: the newest verdict is the one - * that matters and it is on the *last* page, so stopping at the first would - * answer with a stale one. + * The newest {@link LIMITS.listEntries} reviews, through GraphQL's + * `reviews(last:)`: REST pages oldest first and has no way to ask for the + * end, so a bounded REST read of a long history would drop exactly the + * newest verdict. `totalCount` says how many older ones were left out. */ - async listReviews(pull: ThreadRef): Promise { - const rows: ApiReview[] = []; - for (let page = 1; page <= REVIEW_MAX_PAGES; page++) { - const batch = await this.call('listReviews', async () => { - const res = await this.octokit.request('GET /repos/{owner}/{repo}/pulls/{pull_number}/reviews', { - owner: pull.repo.owner, - repo: pull.repo.name, - pull_number: pull.number, - per_page: REVIEW_PAGE, - page, - }); - return (Array.isArray(res.data) ? res.data : []) as ApiReview[]; - }); - rows.push(...batch); - if (batch.length < REVIEW_PAGE) break; - if (page === REVIEW_MAX_PAGES) { - this.log('warn', 'listReviews: page bound reached; later reviews not read', { - repo: repoSlug(pull.repo), - number: pull.number, - read: rows.length, - }); + async listReviews(pull: ThreadRef): Promise { + const query = `query($owner:String!,$name:String!,$number:Int!,$last:Int!){ + repository(owner:$owner,name:$name){ + pullRequest(number:$number){ + reviews(last:$last){ + totalCount + nodes{ databaseId author{ login __typename } state body commit{ oid } submittedAt url } } } - return rows.map(mapReview); + } +}`; + const data = await this.graphql('listReviews', query, { + owner: pull.repo.owner, + name: pull.repo.name, + number: pull.number, + last: LIMITS.listEntries, + }); + const reviews = (data.repository?.pullRequest?.reviews?.nodes ?? []) + .filter((n): n is NonNullable => n != null) + .map(mapReview); + return { total: Math.max(data.repository?.pullRequest?.reviews?.totalCount ?? 0, reviews.length), reviews }; } // ─────────────────────────────────────────────────────────────────────────── @@ -1546,6 +1550,31 @@ export class GitHubAdapter implements GitHubApi { }); } + async branchFacts(repo: RepoRef, branch: string): Promise { + return this.call('branchFacts', async () => { + const [branchRes, repoRes, pullsRes] = await Promise.all([ + this.octokit.request('GET /repos/{owner}/{repo}/branches/{branch}', { + owner: repo.owner, + repo: repo.name, + branch, + }), + this.octokit.request('GET /repos/{owner}/{repo}', { owner: repo.owner, repo: repo.name }), + this.octokit.request('GET /repos/{owner}/{repo}/pulls', { + owner: repo.owner, + repo: repo.name, + state: 'open', + base: branch, + per_page: 1, + }), + ]); + return { + protected: (branchRes.data as { protected?: boolean }).protected === true, + isDefault: (repoRes.data as { default_branch?: string }).default_branch === branch, + openPullsBasedOn: Array.isArray(pullsRes.data) ? pullsRes.data.length : 0, + }; + }); + } + // ─────────────────────────────────────────────────────────────────────────── // Actions // ─────────────────────────────────────────────────────────────────────────── diff --git a/src/tools-merge.ts b/src/tools-merge.ts index a6a1b7d..9b317d9 100644 --- a/src/tools-merge.ts +++ b/src/tools-merge.ts @@ -18,11 +18,17 @@ * 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; - * - with `sha`, 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 same sha again, with a 409, in case it moves in between); + * - 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 + * 409, in case it moves in between); * - `delete_branch` deletes only a head branch in the base repository - * itself, never one a base allow-list names. + * itself, and never one that is a base anywhere: named by an allow-list, + * 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. * * Branch protection — required checks, required approvals, linear history — * is GitHub's to enforce, and its refusal (405) comes back as it is. @@ -30,6 +36,7 @@ import { MERGE_FEATURE_SET, + type BranchFacts, TOOL_MERGE_PULL_REQUEST, TOOL_UPDATE_BRANCH, allowedBases, @@ -77,20 +84,57 @@ export function requireMergeBase(base: string, repo: RepoRef, ctx: ToolContext): refuse(`base branch ${echo(base)} is not in GITHUB_MERGE_BASE_ALLOW (${allowed})`); } -/** Why a pull request cannot be merged as it stands, or null when it can be tried. */ -function notMergeable(pr: PullRequest): string | null { +/** Why a pull request cannot be merged (or updated) as it stands, or null when it can be tried. */ +function notMergeable(pr: PullRequest, action: 'merging' | 'updating'): string | null { const at = `${repoSlug(pr.repo)}#${pr.number}`; if (pr.merged) return `${at} is already merged`; - if (pr.state === 'closed') return `${at} is closed; reopen it before merging`; - if (pr.draft) return `${at} is a draft; mark it ready for review (edit_pull_request draft: false) before merging`; + if (pr.state === 'closed') return `${at} is closed; reopen it before ${action}`; + if (action === 'merging' && pr.draft) { + return `${at} is a draft; mark it ready for review (edit_pull_request draft: false) before merging`; + } return null; } +/** A sha as a caller may pass one: 7 to 40 hex digits, any case. */ +const SHA_PREFIX = /^[0-9a-f]{7,40}$/; + +/** + * The caller's `sha` against the head GitHub reports. A prefix of seven or + * more hex digits is accepted, in either case — the tools print 12-character + * shas — and the full head sha is what goes to GitHub. Anything else that does + * not match is a head that moved, and the refusal says which head it is now. + */ +function requireHeadSha(expected: string, pr: PullRequest): void { + const asked = expected.trim().toLowerCase(); + if (!SHA_PREFIX.test(asked)) { + throw new ToolArgumentError(`"sha" must be a commit sha (7 to 40 hex digits), got ${echo(expected, FIELD.sha)}`); + } + if (!pr.head.sha.toLowerCase().startsWith(asked)) { + refuse( + `the head of ${repoSlug(pr.repo)}#${pr.number} is ${field(pr.head.sha, FIELD.sha)}, not ` + + `${echo(expected, FIELD.sha)}: it moved since it was read. Read it again (and its reviews and ` + + `checks) before acting on it.`, + ); + } +} + +/** + * The branches no tool here writes to or deletes: the pull request's own base + * and every branch an allow-list names as a base. + */ +function baseBranches(pr: PullRequest, ctx: ToolContext): Set { + return new Set([ + pr.base.ref, + ...allowedBases(pr.repo, ctx.prBaseAllow), + ...allowedBases(pr.repo, ctx.mergeBaseAllow ?? []), + ]); +} + /** - * Whether the merged head branch may be deleted, 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 that any allow-list names as a base: deleting `main` because - * someone opened `main → release` would be the worst outcome this tool has. + * 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. */ function branchDeletionBlocked(pr: PullRequest, ctx: ToolContext): string | null { const branch = pr.head.ref; @@ -99,17 +143,26 @@ function branchDeletionBlocked(pr: PullRequest, ctx: ToolContext): string | null if (!sameRepo(pr.headRepo, pr.repo)) { return `the head branch lives in ${field(repoSlug(pr.headRepo), FIELD.slug)}, not in ${repoSlug(pr.repo)}`; } - const protectedBranches = new Set([ - pr.base.ref, - ...allowedBases(pr.repo, ctx.prBaseAllow), - ...allowedBases(pr.repo, ctx.mergeBaseAllow ?? []), - ]); - if (protectedBranches.has(branch)) { + if (baseBranches(pr, ctx).has(branch)) { return `${echo(branch)} is a base branch here (GITHUB_PR_BASE_ALLOW / GITHUB_MERGE_BASE_ALLOW) and is never deleted`; } return 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. + */ +function deletionBlockedOnGitHub(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`; + return null; +} + export const mergeTools: ToolDefinition[] = [ { name: TOOL_MERGE_PULL_REQUEST, @@ -125,7 +178,8 @@ export const mergeTools: ToolDefinition[] = [ number: { type: 'integer', minimum: 1, description: 'Pull-request number' }, sha: { type: 'string', - description: 'The head sha this merge is for; refused if the head has moved (recommended)', + description: + 'The head sha this merge is for (full, or 7+ hex digits); refused if the head has moved (recommended)', }, method: { type: 'string', @@ -137,7 +191,8 @@ export const mergeTools: ToolDefinition[] = [ delete_branch: { type: 'boolean', description: - 'Delete the head branch once merged (default false). Only a branch of this repository, never a base branch.', + 'Delete the head branch once merged (default false). Only 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.', }, }, required: ['number'], @@ -147,14 +202,15 @@ 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"). GitHub does the merge in the background; the head sha changes when it lands. ' + - 'With `sha`, GitHub refuses if the head is no longer that commit.', + '"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.', inputSchema: { type: 'object', properties: { repo: REPO_PROPERTY, number: { type: 'integer', minimum: 1, description: 'Pull-request number' }, - sha: { type: 'string', description: 'The head sha you expect the branch to be at' }, + sha: { type: 'string', description: 'The head sha you expect the branch to be at (full, or 7+ hex digits)' }, }, required: ['number'], }, @@ -175,16 +231,10 @@ async function callMerge(name: string, args: Record, ctx: ToolC } const pr = await ctx.api.getPullRequest(pull); - const blocked = notMergeable(pr); + const blocked = notMergeable(pr, 'merging'); if (blocked) refuse(blocked); requireMergeBase(pr.base.ref, pull.repo, ctx); - if (expected !== undefined && expected.trim() !== pr.head.sha) { - refuse( - `the head of ${repoSlug(pull.repo)}#${pull.number} is ${field(pr.head.sha, FIELD.sha)}, not ` + - `${echo(expected, FIELD.sha)}: it moved since it was read. Read it again (and its reviews and ` + - `checks) before merging.`, - ); - } + if (expected !== undefined) requireHeadSha(expected, pr); const result = await ctx.api.mergePullRequest(pull, { method, sha: pr.head.sha, title, message }); if (!result.merged) { @@ -196,17 +246,22 @@ async function callMerge(name: string, args: Record, ctx: ToolC `into ${field(pr.base.ref)} by ${method}: ${field(result.sha, FIELD.sha)}`, ]; if (deleteBranch) { + // The merge has happened whatever this does next, so a failed check + // or deletion is reported beside it rather than turning a successful + // merge into an error result. A repository that deletes merged + // branches itself answers 404 or 422 here, which is the same end state. const why = branchDeletionBlocked(pr, ctx); if (why) { out.push(`head branch kept: ${why}`); } else { - // The merge has happened whatever this does next, so a failed - // deletion is reported beside it rather than turning a successful - // merge into an error result. A repository that deletes merged - // branches itself answers 422 here, which is the same end state. try { - await ctx.api.deleteBranch(pull.repo, pr.head.ref); - out.push(`deleted branch ${field(pr.head.ref)}`); + const onGitHub = deletionBlockedOnGitHub(pr.head.ref, await ctx.api.branchFacts(pull.repo, pr.head.ref)); + if (onGitHub) { + out.push(`head branch kept: ${onGitHub}`); + } else { + await ctx.api.deleteBranch(pull.repo, pr.head.ref); + out.push(`deleted branch ${field(pr.head.ref)}`); + } } catch (err) { out.push(`head branch not deleted: ${describeFailure('deleteBranch', err)}`); } @@ -217,10 +272,25 @@ async function callMerge(name: string, args: Record, ctx: ToolC case TOOL_UPDATE_BRANCH: { const pull = resolveThread(args, ctx, 'pull'); const expected = optionalString(args, 'sha'); - await ctx.api.updatePullRequestBranch(pull, expected?.trim()); + // 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). + const pr = await ctx.api.getPullRequest(pull); + const blocked = notMergeable(pr, 'updating'); + if (blocked) refuse(blocked); + requireMergeBase(pr.base.ref, pull.repo, ctx); + 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 ` + + `(GITHUB_PR_BASE_ALLOW / GITHUB_MERGE_BASE_ALLOW); update_branch never writes to one`, + ); + } + if (expected !== undefined) requireHeadSha(expected, pr); + await ctx.api.updatePullRequestBranch(pull, pr.head.sha); return toolResult( - `asked GitHub to merge the base of ${repoSlug(pull.repo)}#${pull.number} into its head branch; ` + - `it runs in the background, and get_pull_request shows the new head sha once it lands`, + `asked GitHub to merge ${field(pr.base.ref)} into ${field(pr.head.ref)} ` + + `(${repoSlug(pull.repo)}#${pull.number}, head ${field(pr.head.sha, FIELD.sha)}); it runs in the background, ` + + `and get_pull_request shows the new head sha once it lands`, ); } default: diff --git a/src/tools-read.ts b/src/tools-read.ts index 6bf5d60..db111a7 100644 --- a/src/tools-read.ts +++ b/src/tools-read.ts @@ -39,8 +39,8 @@ import { type IssueSearchHit, type PullRequest, type PullRequestFile, - type PullRequestReview, type RepoRef, + type ReviewList, type ReviewThread, type ThreadKind, type ThreadRef, @@ -176,18 +176,19 @@ export function renderReviewThreads(pull: ThreadRef, threads: readonly ReviewThr } /** - * Reviews, oldest first, ending on the newest — which is the one a caller - * deciding whether to merge needs, so when the list is longer than the bound - * it is the *oldest* that are dropped, and the line saying so comes first. + * The newest reviews, oldest of them first, ending on the newest — which is + * the one a caller deciding whether to merge needs. When the history is longer + * than the bound, the adapter has already dropped the *oldest*; the line + * saying how many comes first. */ -export function renderReviews(pull: ThreadRef, reviews: readonly PullRequestReview[]): string { - if (reviews.length === 0) return `no reviews on ${ref(pull.repo, pull.number)}`; - const shown = reviews.slice(-LIMITS.listEntries); - const dropped = reviews.length - shown.length; +export function renderReviews(pull: ThreadRef, list: ReviewList): string { + const { total, reviews } = list; + if (total === 0 || reviews.length === 0) return `no reviews on ${ref(pull.repo, pull.number)}`; + const dropped = total - reviews.length; return lines( - `${reviews.length} review${reviews.length === 1 ? '' : 's'} on ${ref(pull.repo, pull.number)}, oldest first`, + `${total} review${total === 1 ? '' : 's'} on ${ref(pull.repo, pull.number)}, oldest first`, dropped > 0 && `… ${dropped} older not shown (list bound ${LIMITS.listEntries})`, - ...shown.map((r) => + ...reviews.map((r) => lines( `— id ${r.id} ${field(r.state)} by ${field(r.author.login)} at ${field(r.submittedAt || 'not submitted')} ` + `on ${field(r.commitId.slice(0, 12), FIELD.sha)} ${field(r.html_url, FIELD.url)}`, diff --git a/src/types.ts b/src/types.ts index f329841..41a91d1 100644 --- a/src/types.ts +++ b/src/types.ts @@ -358,6 +358,24 @@ export interface PullRequestReview { html_url: string; } +/** The newest reviews (oldest of them first) and the count of all of them. */ +export interface ReviewList { + total: number; + reviews: PullRequestReview[]; +} + +/** + * 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. + */ +export interface BranchFacts { + protected: boolean; + isDefault: boolean; + /** Open pull requests whose base is this branch (counted to 1 or more, not exactly). */ + openPullsBasedOn: number; +} + export type MergeMethod = 'merge' | 'squash' | 'rebase'; export interface MergePullRequestInput { @@ -532,8 +550,12 @@ export interface GitHubApi { createPullRequest(repo: RepoRef, input: CreatePullRequestInput): Promise; // ── Reviews (read) ── - /** Every submitted review on a pull request, oldest first, paged to the end (bounded). */ - listReviews(pull: ThreadRef): Promise; + /** + * The newest {@link LIMITS.listEntries} reviews on a pull request, oldest + * of them first, and how many there are in all — so a long history drops + * its *oldest* reviews, never the newest verdict. + */ + listReviews(pull: ThreadRef): Promise; // ── Merge ── mergePullRequest(pull: ThreadRef, input: MergePullRequestInput): Promise; @@ -541,6 +563,8 @@ 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. */ + branchFacts(repo: RepoRef, branch: string): Promise; // ── Actions ── listWorkflowRuns(repo: RepoRef, input: ListWorkflowRunsInput): Promise; @@ -583,6 +607,7 @@ export const GITHUB_API_METHODS = [ 'mergePullRequest', 'updatePullRequestBranch', 'deleteBranch', + 'branchFacts', 'listWorkflowRuns', 'getWorkflowRun', 'dispatchWorkflow', @@ -1178,9 +1203,11 @@ export class ConfigError extends Error { export type Env = Record; /** - * The branches a pull request on `repo` may target: every plain entry of - * GITHUB_PR_BASE_ALLOW plus the `owner/name:branch` entries scoped to this - * repository. Order and duplicates as written; the caller only asks `includes`. + * The entries of a scoped allow-list that apply to `repo`: every plain entry + * plus the `owner/name:` entries scoped to this repository. Used for + * branches (GITHUB_PR_BASE_ALLOW, GITHUB_MERGE_BASE_ALLOW) and for workflow + * files (GITHUB_WORKFLOW_ALLOW). Order and duplicates as written; the caller + * only asks `includes`. */ export function allowedBases(repo: RepoRef, entries: readonly string[]): string[] { const out: string[] = []; @@ -1256,11 +1283,19 @@ function readPort(name: EnvVar, raw: string | undefined, problems: string[]): nu } /** - * A base-branch allow-list (GITHUB_PR_BASE_ALLOW, GITHUB_MERGE_BASE_ALLOW): - * `branch` for every repository, `owner/name:branch` for one. A scoped entry - * must parse and must name a repository in GITHUB_REPOS. + * A scoped allow-list — of base branches (GITHUB_PR_BASE_ALLOW, + * GITHUB_MERGE_BASE_ALLOW) or of workflow files (GITHUB_WORKFLOW_ALLOW): + * `` for every repository, `owner/name:` for one. A scoped entry + * must parse and must name a repository in GITHUB_REPOS. `noun` names what + * the values are, so an error about workflow files does not talk of branches. */ -function readBaseAllow(name: EnvVar, raw: string | undefined, repos: readonly RepoRef[], problems: string[]): string[] { +function readBaseAllow( + name: EnvVar, + raw: string | undefined, + repos: readonly RepoRef[], + problems: string[], + noun = 'branch', +): string[] { const entries = csv(raw); for (const entry of entries) { const at = entry.indexOf(':'); @@ -1268,7 +1303,7 @@ function readBaseAllow(name: EnvVar, raw: string | undefined, repos: readonly Re const scope = parseRepoSlug(entry.slice(0, at)); const branch = entry.slice(at + 1); if (!scope || !branch) { - problems.push(`${name}: "${entry}" is neither a branch nor owner/name:branch`); + problems.push(`${name}: "${entry}" is neither a ${noun} nor owner/name:${noun}`); } else if (repos.length > 0 && !repoAllowed(scope, repos)) { problems.push(`${name}: "${entry}" names a repository outside GITHUB_REPOS`); } @@ -1365,7 +1400,7 @@ export function readConfig(env: Env = process.env): Config { const prBaseAllow = readBaseAllow('GITHUB_PR_BASE_ALLOW', env.GITHUB_PR_BASE_ALLOW, repos, problems); const mergeBaseAllow = readBaseAllow('GITHUB_MERGE_BASE_ALLOW', env.GITHUB_MERGE_BASE_ALLOW, repos, problems); - const workflowAllow = readBaseAllow('GITHUB_WORKFLOW_ALLOW', env.GITHUB_WORKFLOW_ALLOW, repos, problems); + const workflowAllow = readBaseAllow('GITHUB_WORKFLOW_ALLOW', env.GITHUB_WORKFLOW_ALLOW, repos, problems, 'workflow file'); for (const entry of workflowAllow) { const file = entry.slice(entry.indexOf(':') + 1); if (file && !isWorkflowFileName(file)) { diff --git a/test/githubAdapter.test.ts b/test/githubAdapter.test.ts index 9c34ad9..be3bff7 100644 --- a/test/githubAdapter.test.ts +++ b/test/githubAdapter.test.ts @@ -1026,42 +1026,56 @@ test('a 405 from the merge endpoint is the caller’s error, with GitHub’s rea }); }); -test('listReviews pages to the end, so the newest verdict is never cut off', async () => { +test('listReviews asks GraphQL for the newest reviews and the total, so the newest verdict is never cut off', async () => { const r = await rig(); - const review = (id: number, state: string) => ({ - id, - user: { login: id % 2 ? 'github-actions[bot]' : 'maintainer', type: id % 2 ? 'Bot' : 'User' }, + const node = (id: number, state: string, bot: boolean) => ({ + databaseId: id, + author: bot ? { login: 'github-actions', __typename: 'Bot' } : { login: 'maintainer', __typename: 'User' }, state, body: `review ${id}`, - commit_id: HEAD_SHA, - submitted_at: '2026-10-01T00:00:00Z', - html_url: `https://github.com/x#pullrequestreview-${id}`, + commit: { oid: HEAD_SHA }, + submittedAt: '2026-10-01T00:00:00Z', + url: `https://github.com/x#pullrequestreview-${id}`, }); - r.fake.on('GET /repos/anima-research/rayforce/pulls/12/reviews', (req) => - req.query.page === '1' - ? { body: Array.from({ length: 100 }, (_, i) => review(i + 1, 'COMMENTED')) } - : { body: [review(101, 'DISMISSED'), review(102, 'APPROVED')] }, - ); - const reviews = await r.adapter.listReviews(PULL); - const asked = r.fake.for('GET /repos/anima-research/rayforce/pulls/12/reviews'); - assert.deepEqual( - asked.map((c) => [c.query.per_page, c.query.page]), - [ - ['100', '1'], - ['100', '2'], - ], - ); - assert.equal(reviews.length, 102); - assert.deepEqual(reviews.at(-1), { - id: 102, + r.fake.on('POST /graphql', { + body: { + data: { + repository: { + pullRequest: { + reviews: { totalCount: 612, nodes: [node(611, 'DISMISSED', true), null, node(612, 'APPROVED', false)] }, + }, + }, + }, + }, + }); + const list = await r.adapter.listReviews(PULL); + const sent = r.fake.for('POST /graphql')[0].body; + assert.match(String(sent.query), /reviews\(last:\$last\)\{\s*totalCount/); + assert.deepEqual(sent.variables, { owner: 'anima-research', name: 'rayforce', number: 12, last: 50 }); + assert.equal(list.total, 612); + assert.equal(list.reviews.length, 2, 'a null node is skipped, not mapped'); + assert.deepEqual(list.reviews[1], { + id: 612, author: { login: 'maintainer', isBot: false }, state: 'approved', - body: 'review 102', + body: 'review 612', commitId: HEAD_SHA, submittedAt: '2026-10-01T00:00:00Z', - html_url: 'https://github.com/x#pullrequestreview-102', + html_url: 'https://github.com/x#pullrequestreview-612', }); - assert.equal(reviews.at(-2)?.state, 'dismissed'); + assert.deepEqual(list.reviews[0].author, { login: 'github-actions', isBot: true }); + assert.equal(list.reviews[0].state, 'dismissed'); +}); + +test('branchFacts reads protection, the default branch and the open pull requests based on a branch', async () => { + const r = await rig(); + const p = '/repos/anima-research/rayforce'; + r.fake + .on(`GET ${p}/branches/release`, { body: { name: 'release', protected: true } }) + .on(`GET ${p}`, { body: { default_branch: 'dev' } }) + .on(`GET ${p}/pulls`, { body: [{ number: 3 }] }); + assert.deepEqual(await r.adapter.branchFacts(REPO, 'release'), { protected: true, isDefault: false, openPullsBasedOn: 1 }); + assert.deepEqual(r.fake.for(`GET ${p}/pulls`)[0].query, { state: 'open', base: 'release', per_page: '1' }); }); test('workflow runs: list by repo or by workflow file, read one, dispatch, re-run', async () => { diff --git a/test/harness.ts b/test/harness.ts index 5e7010b..b03ed8b 100644 --- a/test/harness.ts +++ b/test/harness.ts @@ -51,7 +51,9 @@ import { type PullRequest, type PullRequestFile, type PullRequestReview, + type BranchFacts, type RepoRef, + type ReviewList, type ReviewThread, type SubmittedReview, type WorkflowRun, @@ -118,6 +120,10 @@ export class FakeGitHubApi implements GitHubApi { mergeRefusal: string | null = null; /** What the next `deleteBranch` throws instead, when set (a 422 from a repo that already deleted it). */ deleteBranchError: Error | null = null; + /** What `branchFacts` answers, by branch name; absent = an ordinary unprotected branch. */ + readonly branchFactsByName = new Map(); + /** A head that moves after the tool read it and before GitHub merges (the 409 race). */ + moveHeadBeforeMerge: string | null = null; private nextNumber = 1; private nextId = 1000; private lastStamp = 0; @@ -608,12 +614,19 @@ export class FakeGitHubApi implements GitHubApi { // ── Reviews, merge, actions ── - async listReviews(pull: ThreadRef): Promise { + async listReviews(pull: ThreadRef): Promise { this.record('listReviews', pull); if (!this.pulls.has(key(pull.repo, pull.number))) { throw new Error(`no such pull request: ${key(pull.repo, pull.number)}`); } - return [...(this.reviewLists.get(key(pull.repo, pull.number)) ?? [])]; + // GraphQL's `reviews(last: 50)`: the newest, and the count of all. + const all = this.reviewLists.get(key(pull.repo, pull.number)) ?? []; + return { total: all.length, reviews: all.slice(-LIMITS.listEntries) }; + } + + async branchFacts(repo: RepoRef, branch: string): Promise { + this.record('branchFacts', repo, branch); + return this.branchFactsByName.get(branch) ?? { protected: false, isDefault: false, openPullsBasedOn: 0 }; } async mergePullRequest(pull: ThreadRef, input: MergePullRequestInput): Promise { @@ -625,6 +638,10 @@ export class FakeGitHubApi implements GitHubApi { this.mergeRefusal = null; return { merged: false, sha: '', message }; } + if (this.moveHeadBeforeMerge !== null) { + pr.head = { ...pr.head, sha: this.moveHeadBeforeMerge }; + this.moveHeadBeforeMerge = null; + } if (input.sha !== pr.head.sha) { throw Object.assign(new Error('Head branch was modified. Review and try the merge again.'), { status: 409 }); } diff --git a/test/smoke.test.ts b/test/smoke.test.ts index 30ad94f..1d1a9d9 100644 --- a/test/smoke.test.ts +++ b/test/smoke.test.ts @@ -305,6 +305,11 @@ test('GITHUB_MERGE_BASE_ALLOW and GITHUB_WORKFLOW_ALLOW parse like the base allo for (const bad of ['.github/workflows/ci.yml', '12345', 'Review', 'ci.yml.bak']) { assert.match(problems({ GITHUB_WORKFLOW_ALLOW: bad })[0], /not a workflow file name/, bad); } + // A malformed scoped entry is described as what the list holds: files, not branches. + assert.match( + problems({ GITHUB_WORKFLOW_ALLOW: 'anima-research/rayforce:' })[0], + /^GITHUB_WORKFLOW_ALLOW: "anima-research\/rayforce:" is neither a workflow file nor owner\/name:workflow file$/, + ); }); test('readConfig names every problem at once', () => { diff --git a/test/toolsMerge.test.ts b/test/toolsMerge.test.ts index ff6c5eb..0aea0d4 100644 --- a/test/toolsMerge.test.ts +++ b/test/toolsMerge.test.ts @@ -50,11 +50,11 @@ function failure(result: ToolResult): string { } const PULL: ThreadRef = { repo: TEST_REPO, number: 8, kind: 'pull' }; -const HEAD = 'head8'.padEnd(40, '0'); +const HEAD = 'abc8'.padEnd(40, '0'); function seeded(): FakeGitHubApi { const api = new FakeGitHubApi(); - api.seedPullRequest({ number: 8, headRepo: TEST_REPO }); + api.seedPullRequest({ number: 8, headRepo: TEST_REPO, head: { ref: 'feature-8', sha: HEAD } }); return api; } @@ -69,7 +69,7 @@ test('the module declares github.merge and its two tools', () => { test('merge_pull_request merges at the head sha it read, by merge commit by default', async () => { const api = seeded(); const out = ok(await mergeToolModule.call('merge_pull_request', { number: 8, sha: HEAD }, ctx(api))); - assert.match(out, /^merged anima-research\/rayforce#8 \(feature-8 at head80+\) into dev by merge: merge0+$/); + assert.match(out, /^merged anima-research\/rayforce#8 \(feature-8 at abc80+\) into dev by merge: merge0+$/); assert.deepEqual(api.callsTo('mergePullRequest')[0].args, [ PULL, { method: 'merge', sha: HEAD, title: undefined, message: undefined }, @@ -123,11 +123,37 @@ test('a base GITHUB_PR_BASE_ALLOW names is not thereby mergeable', async () => { test('a head that moved since it was read is refused before GitHub is asked', async () => { const api = seeded(); - const out = failure(await mergeToolModule.call('merge_pull_request', { number: 8, sha: 'abc123' }, ctx(api))); - assert.match(out, /the head of anima-research\/rayforce#8 is head80+, not "abc123": it moved since it was read/); + const out = failure(await mergeToolModule.call('merge_pull_request', { number: 8, sha: 'def4567' }, ctx(api))); + assert.match(out, /the head of anima-research\/rayforce#8 is abc80+, not "def4567": it moved since it was read/); assert.equal(api.callsTo('mergePullRequest').length, 0); }); +test('a short or upper-case sha of the head is the head, and the full sha goes to GitHub', async () => { + for (const sha of [HEAD.slice(0, 7), HEAD.slice(0, 12), HEAD.toUpperCase(), ` ${HEAD} `]) { + const api = seeded(); + ok(await mergeToolModule.call('merge_pull_request', { number: 8, sha }, ctx(api))); + assert.equal((api.callsTo('mergePullRequest')[0].args[1] as { sha: string }).sha, HEAD, sha); + } +}); + +test('a sha that is not 7 to 40 hex digits is a malformed call, not a moved head', async () => { + for (const sha of ['abc12', 'not-a-sha', 'g'.repeat(40), 'a'.repeat(41)]) { + const api = seeded(); + const out = failure(await mergeToolModule.call('merge_pull_request', { number: 8, sha }, ctx(api))); + assert.match(out, /"sha" must be a commit sha \(7 to 40 hex digits\)/, sha); + assert.doesNotMatch(out, /moved/); + } +}); + +test('a head that moves between the read and the merge is GitHub’s 409, and no branch is deleted', async () => { + const api = seeded(); + api.moveHeadBeforeMerge = 'f'.repeat(40); + const out = failure(await mergeToolModule.call('merge_pull_request', { number: 8, sha: HEAD, delete_branch: true }, ctx(api))); + assert.match(out, /merge_pull_request failed: GitHub error 409: Head branch was modified/); + assert.equal(api.callsTo('deleteBranch').length, 0); + assert.equal(api.callsTo('branchFacts').length, 0); +}); + test('draft, closed and merged pull requests are refused by name', async () => { const cases: Array<[Record, RegExp]> = [ [{ draft: true }, /#9 is a draft; mark it ready for review/], @@ -162,8 +188,25 @@ test('delete_branch deletes the merged head branch of this repository', async () const out = ok(await mergeToolModule.call('merge_pull_request', { number: 8, delete_branch: true }, ctx(api))); assert.match(out, /\ndeleted branch feature-8$/); assert.deepEqual(api.deletedBranches, ['anima-research/rayforce:feature-8']); - // Deleted only after the merge answered. - assert.deepEqual(api.callLog().slice(-2), ['mergePullRequest', 'deleteBranch']); + // Deleted only after the merge answered, and after GitHub said it is nobody's base. + assert.deepEqual(api.callLog().slice(-3), ['mergePullRequest', 'branchFacts', 'deleteBranch']); +}); + +test('delete_branch keeps a branch GitHub says is a base: protected, default, or targeted by an open PR', async () => { + const cases: Array<[{ protected: boolean; isDefault: boolean; openPullsBasedOn: number }, RegExp]> = [ + // The back-merge case: `release` kept out of every allow-list, protected on GitHub. + [{ protected: true, isDefault: false, openPullsBasedOn: 0 }, /head branch kept: "feature-8" is protected on GitHub/], + [{ protected: false, isDefault: true, openPullsBasedOn: 0 }, /head branch kept: "feature-8" is the repository's default branch/], + [{ protected: false, isDefault: false, openPullsBasedOn: 1 }, /head branch kept: "feature-8" is the base of another open pull request/], + ]; + for (const [facts, expected] of cases) { + const api = seeded(); + api.branchFactsByName.set('feature-8', facts); + const out = ok(await mergeToolModule.call('merge_pull_request', { number: 8, delete_branch: true }, ctx(api))); + assert.match(out, /^merged /); + assert.match(out, expected); + assert.equal(api.callsTo('deleteBranch').length, 0); + } }); test('delete_branch keeps a fork’s branch, an unknown head repository, and any base branch', async () => { @@ -202,14 +245,39 @@ test('a failed branch deletion is reported beside a merge that happened, not as assert.match(out, /head branch not deleted: deleteBranch failed: GitHub error 422: Reference does not exist/); }); -test('update_branch asks GitHub to merge the base in, with the expected head when given', async () => { +test('update_branch merges the base in, guarded by the head it read', async () => { const api = seeded(); - const out = ok(await mergeToolModule.call('update_branch', { number: 8, sha: HEAD }, ctx(api))); - assert.match(out, /asked GitHub to merge the base of anima-research\/rayforce#8 into its head branch/); + const out = ok(await mergeToolModule.call('update_branch', { number: 8, sha: HEAD.slice(0, 12) }, ctx(api))); + 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]); + ok(await mergeToolModule.call('update_branch', { number: 8 }, ctx(api))); + assert.deepEqual(api.callsTo('updatePullRequestBranch')[1].args, [PULL, HEAD]); - const moved = failure(await mergeToolModule.call('update_branch', { number: 8, sha: 'abc' }, ctx(api))); - assert.match(moved, /update_branch failed: GitHub error 422: expected head sha didn’t match/); + const moved = failure(await mergeToolModule.call('update_branch', { number: 8, sha: 'def4567' }, ctx(api))); + assert.match(moved, /update_branch refused: the head of anima-research\/rayforce#8 is abc80+, not "def4567"/); +}); + +test('update_branch takes a merge’s fences: allowed base, open pull request, head not a base', async () => { + const cases: Array<[Record, Partial, RegExp]> = [ + // GITHUB_MERGE_BASE_ALLOW unset: granting github.merge alone writes nothing. + [{}, { mergeBaseAllow: [] }, /base branch "dev" is not in GITHUB_MERGE_BASE_ALLOW/], + [{ state: 'closed' }, {}, /#8 is closed; reopen it before updating/], + [{ state: 'closed', merged: true }, {}, /#8 is already merged/], + // `main → dev` would merge dev into main. + [{ head: { ref: 'main', sha: HEAD } }, { prBaseAllow: ['dev', 'main'] }, /the head of anima-research\/rayforce#8, "main", is a base branch here/], + ]; + for (const [partial, over, expected] of cases) { + const api = new FakeGitHubApi(); + api.seedPullRequest({ number: 8, headRepo: TEST_REPO, head: { ref: 'feature-8', sha: HEAD }, ...partial }); + const out = failure(await mergeToolModule.call('update_branch', { number: 8 }, ctx(api, over))); + assert.match(out, expected); + assert.equal(api.callsTo('updatePullRequestBranch').length, 0); + } + // 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 } }); + ok(await mergeToolModule.call('update_branch', { number: 8 }, ctx(api))); }); test('an unknown tool name is refused by the module', async () => {