diff --git a/README.md b/README.md index dfa2ec7..db21349 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,30 @@ 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, 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. 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` 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. @@ -410,7 +439,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 +451,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 +464,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 +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 merge, no branch deletion, no release-branch write.** +- **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.** @@ -473,7 +525,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..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. 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; writes commits only through GitHub's pull-request merge and update-branch.", "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..3cea953 --- /dev/null +++ b/changelog.d/merge-and-actions.added.md @@ -0,0 +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 + 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`): the newest 50 reviews on a pull request and + how many there are in all, 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..46b5574 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,28 @@ 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 BranchFacts, type RepoRef, + type ReviewList, type ReviewState, type ReviewThread, type ReviewThreadComment, type SubmittedReview, + type SubmittedReviewState, type ThreadRef, type UpdateIssueInput, type UpdatePullRequestInput, + type WorkflowRun, } from './types.js'; // ───────────────────────────────────────────────────────────────────────────── @@ -111,6 +120,7 @@ const CHECK_RUN_PAGE = 100; * rather than summarized from a prefix. */ const CHECK_RUN_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 +281,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 +318,41 @@ export interface CheckRunRow { 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`. */ +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 +419,55 @@ 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'; +} + +type GqlReview = NonNullable< + NonNullable['pullRequest']>['reviews']>['nodes'] +>[number]; + +function mapReview(node: NonNullable): PullRequestReview { + return { + 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 ?? '', + }; +} + +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 +1470,176 @@ export class GitHubAdapter implements GitHubApi { return this.mapPull(repo, data); } + // ─────────────────────────────────────────────────────────────────────────── + // Reviews (read) + // ─────────────────────────────────────────────────────────────────────────── + + /** + * 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 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 } + } + } + } +}`; + 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 }; + } + + // ─────────────────────────────────────────────────────────────────────────── + // 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}`, + }); + }); + } + + 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 + // ─────────────────────────────────────────────────────────────────────────── + + /** 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 +1853,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..9b317d9 --- /dev/null +++ b/src/tools-merge.ts @@ -0,0 +1,306 @@ +/** + * 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` (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, 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. + */ + +import { + MERGE_FEATURE_SET, + type BranchFacts, + 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 (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 ${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, 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; + 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)}`; + } + 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, + 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 (full, or 7+ hex digits); 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 that is nobody’s ' + + 'base: not allow-listed, not protected, not the default branch, not the base of another open pull request.', + }, + }, + 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"). 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 (full, or 7+ hex digits)' }, + }, + 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, 'merging'); + if (blocked) refuse(blocked); + requireMergeBase(pr.base.ref, pull.repo, ctx); + if (expected !== undefined) requireHeadSha(expected, pr); + + 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) { + // 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 { + try { + 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)}`); + } + } + } + return toolResult(out.join('\n')); + } + case TOOL_UPDATE_BRANCH: { + const pull = resolveThread(args, ctx, 'pull'); + const expected = optionalString(args, 'sha'); + // A merge commit onto the head branch is a write, so it takes a merge's + // fences: an open pull request into an allowed base, and a head that is + // not itself a base (on `dev → main`, this would merge main into dev). + 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 ${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: + 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..db111a7 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, @@ -39,6 +40,7 @@ import { type PullRequest, type PullRequestFile, type RepoRef, + type ReviewList, type ReviewThread, type ThreadKind, type ThreadRef, @@ -173,6 +175,36 @@ export function renderReviewThreads(pull: ThreadRef, threads: readonly ReviewThr ); } +/** + * 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, 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( + `${total} review${total === 1 ? '' : 's'} on ${ref(pull.repo, pull.number)}, oldest first`, + dropped > 0 && `… ${dropped} older not shown (list bound ${LIMITS.listEntries})`, + ...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)}`, + 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 +312,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 +369,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..41a91d1 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,94 @@ 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; +} + +/** 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 { + 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 +549,29 @@ export interface GitHubApi { requestReviewers(pull: ThreadRef, logins: string[]): Promise; createPullRequest(repo: RepoRef, input: CreatePullRequestInput): Promise; + // ── Reviews (read) ── + /** + * 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; + /** 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; + /** What decides whether a merged head branch may be deleted. */ + branchFacts(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 +603,15 @@ export const GITHUB_API_METHODS = [ 'updatePullRequest', 'requestReviewers', 'createPullRequest', + 'listReviews', + 'mergePullRequest', + 'updatePullRequestBranch', + 'deleteBranch', + 'branchFacts', + 'listWorkflowRuns', + 'getWorkflowRun', + 'dispatchWorkflow', + 'rerunWorkflowRun', 'listIssuesSince', 'listIssueCommentsSince', 'listReviewCommentsSince', @@ -529,6 +656,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 +687,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 +697,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 +711,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 +736,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 +758,7 @@ export const TOOLS_BY_FEATURE_SET: 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[] = []; @@ -1052,6 +1222,15 @@ export function allowedBases(repo: RepoRef, entries: readonly string[]): string[ return out; } +/** + * A workflow file name as `.github/workflows/` holds it: `review.yml`. No + * directory, no numeric id — the allow-list and the tools name a workflow by + * its file, the one name a reader of the repository can check. + */ +export function isWorkflowFileName(name: string): boolean { + return /^[A-Za-z0-9][A-Za-z0-9._-]*\.ya?ml$/.test(name); +} + function csv(raw: string | undefined): string[] { return (raw ?? '') .split(',') @@ -1103,6 +1282,35 @@ function readPort(name: EnvVar, raw: string | undefined, problems: string[]): nu return n; } +/** + * 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[], + noun = 'branch', +): string[] { + const entries = csv(raw); + for (const entry of entries) { + 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(`${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`); + } + } + return entries; +} + /** * Validate the environment into a {@link Config}, or throw {@link ConfigError} * listing every problem by variable name. @@ -1190,16 +1398,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, 'workflow file'); + 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 +1425,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..be3bff7 100644 --- a/test/githubAdapter.test.ts +++ b/test/githubAdapter.test.ts @@ -980,6 +980,178 @@ 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 asks GraphQL for the newest reviews and the total, so the newest verdict is never cut off', async () => { + const r = await rig(); + 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: { oid: HEAD_SHA }, + submittedAt: '2026-10-01T00:00:00Z', + url: `https://github.com/x#pullrequestreview-${id}`, + }); + 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 612', + commitId: HEAD_SHA, + submittedAt: '2026-10-01T00:00:00Z', + html_url: 'https://github.com/x#pullrequestreview-612', + }); + 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 () => { + 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..b03ed8b 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,26 @@ 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 BranchFacts, type RepoRef, + type ReviewList, type ReviewThread, type SubmittedReview, + type WorkflowRun, type ThreadRef, type ToolResult, type UpdateIssueInput, @@ -98,6 +108,22 @@ 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; + /** 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; @@ -242,6 +268,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 +612,91 @@ 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)}`); + } + // 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 { + 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 (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 }); + } + 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 +777,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 +821,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..1d1a9d9 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,51 @@ 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); + } + // 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', () => { assert.throws( () => readConfig({ GITHUB_REPOS: 'rayforce', GITHUB_WEBHOOK_PORT: '0' }), @@ -344,8 +392,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..0aea0d4 --- /dev/null +++ b/test/toolsMerge.test.ts @@ -0,0 +1,286 @@ +/** + * 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 = 'abc8'.padEnd(40, '0'); + +function seeded(): FakeGitHubApi { + const api = new FakeGitHubApi(); + api.seedPullRequest({ number: 8, headRepo: TEST_REPO, head: { ref: 'feature-8', sha: HEAD } }); + 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 abc80+\) 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: '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/], + [{ 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, 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 () => { + 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 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.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: '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 () => { + 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', });