From c07c5f6e9a6c4d49fd557f0f3dcc9d9011b25405 Mon Sep 17 00:00:00 2001 From: "Fredrik Liljegren (Claude Code Fable 5.1)" Date: Fri, 16 Oct 2026 18:05:03 +0200 Subject: [PATCH 1/2] feat: agents post reviews as the App, and automation reaches the MCP with a token Humans post as themselves, from the page; agents only ever post as `diffity[bot]`, and the review names who asked for it. `post_review { session, event }` posts a session's open findings on an installation token the server mints from the App's private key (`DIFFITY_GITHUB_APP_ID` + `DIFFITY_GITHUB_APP_PRIVATE_KEY`); COMMENT or REQUEST_CHANGES only. An agent that cannot sign in, such as a routine, reaches `/mcp` with an automation token minted by `automation_token { name }`, stored hashed, acting as the user who minted it until revoked. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Bwp5QefjsjMFeq6CK6cT6w --- .github/workflows/deploy.yml | 6 + README.md | 49 +++++++- packages/server/src/app.ts | 23 +++- packages/server/src/automation.ts | 66 ++++++++++ packages/server/src/config.ts | 35 +++++- packages/server/src/db.ts | 13 ++ packages/server/src/github-app.ts | 48 ++++++- packages/server/src/github.ts | 14 +++ packages/server/src/mcp.ts | 73 +++++++++++ packages/server/src/review-prompt.md | 10 +- packages/server/src/server.ts | 7 +- packages/server/src/service.ts | 83 +++++++++++-- packages/server/tests/config.test.ts | 21 +++- packages/server/tests/db.test.ts | 2 +- packages/server/tests/github-app.test.ts | 56 ++++++++- packages/server/tests/helpers.ts | 31 +++++ packages/server/tests/mcp.test.ts | 151 +++++++++++++++++++++-- 17 files changed, 650 insertions(+), 38 deletions(-) create mode 100644 packages/server/src/automation.ts diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 410f5bfc..cad52b02 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -81,6 +81,7 @@ jobs: env: GITHUB_APP_CLIENT_ID: ${{ vars.DIFFITY_GITHUB_APP_CLIENT_ID }} GITHUB_APP_SLUG: ${{ vars.DIFFITY_GITHUB_APP_SLUG }} + GITHUB_APP_ID: ${{ vars.DIFFITY_GITHUB_APP_ID }} # /projects//global/backendServices/ IAP_AUDIENCE: ${{ vars.DIFFITY_IAP_AUDIENCE }} run: | @@ -100,6 +101,11 @@ jobs: if [ -n "$GITHUB_APP_SLUG" ]; then env_vars+=",DIFFITY_GITHUB_APP_SLUG=${GITHUB_APP_SLUG}" fi + # Without the App's id and key, agents cannot post reviews as the App. + if [ -n "$GITHUB_APP_ID" ]; then + env_vars+=",DIFFITY_GITHUB_APP_ID=${GITHUB_APP_ID}" + secrets+=",DIFFITY_GITHUB_APP_PRIVATE_KEY=diffity-github-app-private-key:latest" + fi gcloud run deploy diffity \ --project "$PROJECT" \ diff --git a/README.md b/README.md index 203ef427..77c92d96 100644 --- a/README.md +++ b/README.md @@ -39,7 +39,8 @@ The first tool call opens the browser to sign in and allow the connection. Then: - **Posting.** From the page the user posts the findings to the pull request as one review, and pulls GitHub's review threads in. Posting is only for pull request sessions, and refused once the pull request has moved past the session's head. GitHub is reached as the user: through the GitHub - App they connect on **Settings**, or a token pasted there when no App is configured. + App they connect on **Settings**, or a token pasted there when no App is configured. An agent + posts as the App instead (see Automated reviews). The **Sessions** page also lists the open pull requests that ask for the user's review, asked of GitHub's search with the user's own access (cached for a minute). Each row links the user's newest @@ -85,6 +86,41 @@ cookie, no MCP token. A live token is stored hashed, expires after 12 hours, and nothing else: on its one session, or for a queue token on the user's queue and any of the user's sessions. Once it is refused (401), the command exits with status 1. +### Automated reviews + +Humans post as themselves, from the page; agents only ever post as the bot, `diffity[bot]`, and the +bot's review names the human whose token asked for it. + +An agent that cannot sign in, such as a scheduled routine on every new pull request, reaches the +MCP endpoint with an automation token: a signed-in agent mints one with `automation_token { name }`, +and the routine holds it as a secret. It acts as the user who minted it, with every tool, until +revoked; its sessions are that user's and show in their list. It is stored as a sha256 hash. + +```bash +claude mcp add --transport http diffity https://diffity.prod.naturalcycles.net/mcp \ + --header "Authorization: Bearer ${DIFFITY_AUTOMATION_TOKEN}" +``` + +or in `.mcp.json`: + +```json +{ "mcpServers": { "diffity": { "type": "http", "url": "https://diffity.prod.naturalcycles.net/mcp", + "headers": { "Authorization": "Bearer ${DIFFITY_AUTOMATION_TOKEN}" } } } } +``` + +The run is `create_session { repo, pr }`, the server's `review` prompt, `post_review { session, +event: "COMMENT" }`, then `review_done`. `post_review` sends the session's open findings, with its +general comments as the summary, as one review by the App, on an installation token the server +mints from the App's private key; it takes `COMMENT` or `REQUEST_CHANGES`, never an approval. A +second run on the same head finds the existing session, and what is already on GitHub is skipped. + +| Tool | | +|---|---| +| `post_review { session, event, body? }` | Post the open findings as the App; answers `{ posted, skipped, url }` | +| `automation_token { name }` | Mint a token; it is shown once | +| `automation_tokens {}` | The user's tokens: name, created, last used | +| `automation_token_revoke { name }` | Revoke one | + A repository can name its review standards in a `.diffity.json` at its root, which the `get_standards` tool reads: @@ -130,6 +166,8 @@ environment runs the server from source and restarts it when that changes; after | `DIFFITY_ALLOWED_EMAILS` | — | Comma-separated addresses; replaces the domain rule when set | | `DIFFITY_GITHUB_APP_CLIENT_ID`, `DIFFITY_GITHUB_APP_CLIENT_SECRET` | — | The GitHub App users connect on **Settings**; without it, users paste a token there instead | | `DIFFITY_GITHUB_APP_SLUG` | — | The App's URL name, for the install link on **Settings** | +| `DIFFITY_GITHUB_APP_ID` | — | The App's numeric id; with the private key, agents post reviews as the App (`post_review`) | +| `DIFFITY_GITHUB_APP_PRIVATE_KEY` | — | The App's private key (PEM; one line with literal `\n` works too), for the App's own installation tokens | | `DIFFITY_DEV_GITHUB_TOKEN` | — | A token used for users who have not connected GitHub; localhost only | | `GITHUB_API_URL` | `https://api.github.com` | GitHub REST and GraphQL API | | `GITHUB_URL` | `https://github.com` | Where the App's authorization and token exchange happen | @@ -159,7 +197,8 @@ Cloud Run service `diffity` in `nc-innovation-496314`. The infrastructure (IAP l `diffity.prod.naturalcycles.net`, Cloud SQL Postgres over the VPC connector, runtime service account, secrets) is in NaturalCycles/NCInfraIaC. On the load balancer, `/mcp`, `/token`, `/register`, `/revoke`, `/live/await` and the OAuth `/.well-known/*` metadata bypass IAP, since -agents authenticate with diffity's own OAuth or a live token; everything else is behind IAP. The +agents authenticate with diffity's own OAuth, an automation token or a live token; everything else +is behind IAP. The service accepts unauthenticated invocations for those open paths, so the deploy limits its ingress to the load balancer and disables the `run.app` URL. @@ -176,6 +215,8 @@ The deploy runs one instance (pending OAuth consents are held in memory), sets `DIFFITY_GITHUB_APP_SLUG`) for the GitHub App; with it, the secret `diffity-github-app-client-secret` is mounted as `DIFFITY_GITHUB_APP_CLIENT_SECRET`. Without it, users paste a GitHub token on **Settings**. +- optionally the repository variable `DIFFITY_GITHUB_APP_ID`, for agents to post as the App; with + it, the secret `diffity-github-app-private-key` is mounted as `DIFFITY_GITHUB_APP_PRIVATE_KEY`. ## Access and isolation @@ -185,8 +226,8 @@ The deploy runs one instance (pending OAuth consents are held in memory), sets request against Google's keys and `DIFFITY_IAP_AUDIENCE`; a request without it gets 401. - Before a session is created, GitHub is asked whether the user's token can read the repository. Mirror fetches use that token through the environment, never the mirror's config. -- OAuth client secrets, codes, access and refresh tokens and the web session cookie are stored as - sha256 hashes; GitHub tokens (pasted, or the App's access and refresh tokens) are encrypted with +- OAuth client secrets, codes, access and refresh tokens, automation tokens and the web session + cookie are stored as sha256 hashes; GitHub tokens (pasted, or the App's access and refresh tokens) are encrypted with `DIFFITY_SECRET_KEY`. - Posting is only for pull request sessions, and refused once the pull request has moved past the session's head: the user asks for a new session, which the open findings carry over to. diff --git a/packages/server/src/app.ts b/packages/server/src/app.ts index 628ec52c..487d8e0e 100644 --- a/packages/server/src/app.ts +++ b/packages/server/src/app.ts @@ -10,6 +10,7 @@ import { GENERAL_THREAD_FILE_PATH, type LiveStatusResponse } from '@diffity/api' import type { Config } from './config.js'; import { WEB_SESSION_MAX_AGE_SECONDS, type Users, type User, type WebSessions } from './users.js'; import { hashPresentedClientSecret, type OAuthProvider } from './oauth.js'; +import type { AutomationTokens } from './automation.js'; import { ServiceError, describeSession, type ReviewRequests, type ReviewService } from './service.js'; import type { DevLoginProvider, IapLogin } from './login.js'; import { GitHubConnectError, type GitHubAppAccess } from './github-app.js'; @@ -33,6 +34,7 @@ export interface AppDeps { users: Users; webSessions: WebSessions; oauth: OAuthProvider; + automationTokens: AutomationTokens; service: ReviewService; live: Live; devLogin: DevLoginProvider | null; @@ -203,7 +205,22 @@ export function createApp(deps: AppDeps): express.Express { }); const bearer = requireBearerAuth({ - verifier: oauth, + verifier: { + verifyAccessToken: async token => { + const automation = await deps.automationTokens.verify(token); + if (!automation) { + return oauth.verifyAccessToken(token); + } + return { + token, + clientId: `automation:${automation.name}`, + scopes: [], + // The SDK requires an expiry; an automation token lasts until it is revoked. + expiresAt: Number.MAX_SAFE_INTEGER, + extra: { userId: automation.userId, automation: true, automationName: automation.name }, + }; + }, + }, resourceMetadataUrl: getOAuthProtectedResourceMetadataUrl(mcpUrl), }); @@ -213,11 +230,13 @@ export function createApp(deps: AppDeps): express.Express { res.status(401).json({ error: 'The token belongs to no user' }); return; } + const automationName = req.auth!.extra!.automation === true ? String(req.auth!.extra!.automationName) : null; const server = createMcpServer({ service, live, + automationTokens: deps.automationTokens, userId, - agentName: (await oauth.clientsStore.clientName(req.auth!.clientId)) ?? 'Claude', + agentName: automationName ?? (await oauth.clientsStore.clientName(req.auth!.clientId)) ?? 'Claude', version: deps.version, }); // Stateless: every request is complete in itself, so nothing is held between them. diff --git a/packages/server/src/automation.ts b/packages/server/src/automation.ts new file mode 100644 index 00000000..e9b9e7e5 --- /dev/null +++ b/packages/server/src/automation.ts @@ -0,0 +1,66 @@ +import type { Db } from './db.js'; +import { randomToken, sha256 } from './crypto.js'; +import { ServiceError } from './service.js'; + +/** Recognisable in a secret store or a leaked log, and cheap to tell from an OAuth token. */ +export const AUTOMATION_TOKEN_PREFIX = 'dfa_'; +const LAST_USED_RESOLUTION_MS = 60 * 1000; + +export interface AutomationTokenInfo { + name: string; + createdAt: string; + lastUsedAt: string | null; +} + +/** + * Bearer tokens for agents that cannot do OAuth, such as a scheduled routine: they act as the user + * who minted them, with every MCP tool, until revoked. Only their hash is stored. + */ +export class AutomationTokens { + constructor(private readonly db: Db) {} + + async mint(userId: string, name: string, now = Date.now()): Promise { + const token = `${AUTOMATION_TOKEN_PREFIX}${randomToken()}`; + const inserted = await this.db.one( + `INSERT INTO automation_tokens (token_hash, user_id, name, created_at) VALUES ($1, $2, $3, $4) + ON CONFLICT (user_id, name) DO NOTHING RETURNING token_hash`, + [sha256(token), userId, name, new Date(now).toISOString()], + ); + if (!inserted) { + throw new ServiceError(`You already have an automation token named "${name}"; revoke it first or pick another name`); + } + return token; + } + + async list(userId: string): Promise { + const rows = await this.db.query<{ name: string; created_at: string; last_used_at: string | null }>( + 'SELECT name, created_at, last_used_at FROM automation_tokens WHERE user_id = $1 ORDER BY created_at, name', + [userId], + ); + return rows.map(row => ({ name: row.name, createdAt: row.created_at, lastUsedAt: row.last_used_at })); + } + + async revoke(userId: string, name: string): Promise { + const rows = await this.db.query('DELETE FROM automation_tokens WHERE user_id = $1 AND name = $2 RETURNING name', [userId, name]); + return rows.length > 0; + } + + /** Null for anything that is not a live automation token, including every OAuth token. */ + async verify(token: string, now = Date.now()): Promise<{ userId: string; name: string } | null> { + if (!token.startsWith(AUTOMATION_TOKEN_PREFIX)) { + return null; + } + const hash = sha256(token); + const row = await this.db.one<{ user_id: string; name: string; last_used_at: string | null }>( + 'SELECT user_id, name, last_used_at FROM automation_tokens WHERE token_hash = $1', + [hash], + ); + if (!row) { + return null; + } + if (!row.last_used_at || Date.parse(row.last_used_at) + LAST_USED_RESOLUTION_MS <= now) { + await this.db.query('UPDATE automation_tokens SET last_used_at = $1 WHERE token_hash = $2', [new Date(now).toISOString(), hash]); + } + return { userId: row.user_id, name: row.name }; + } +} diff --git a/packages/server/src/config.ts b/packages/server/src/config.ts index 8d24a770..22635309 100644 --- a/packages/server/src/config.ts +++ b/packages/server/src/config.ts @@ -1,4 +1,4 @@ -import { randomBytes } from 'node:crypto'; +import { createPrivateKey, randomBytes } from 'node:crypto'; export interface Config { publicUrl: URL; @@ -34,6 +34,13 @@ export interface GitHubAppConfig { clientSecret: string; /** For the install link; the app's name in its github.com URL. */ slug: string | null; + /** Lets agents post reviews as the App itself; null without the App id and private key. */ + bot: GitHubBotConfig | null; +} + +export interface GitHubBotConfig { + appId: number; + privateKey: string; } export class ConfigError extends Error {} @@ -114,12 +121,36 @@ function parseGitHubApp(env: NodeJS.ProcessEnv): GitHubAppConfig | null { const clientId = env.DIFFITY_GITHUB_APP_CLIENT_ID?.trim(); const clientSecret = env.DIFFITY_GITHUB_APP_CLIENT_SECRET?.trim(); if (!clientId && !clientSecret) { + if (env.DIFFITY_GITHUB_APP_ID?.trim() || env.DIFFITY_GITHUB_APP_PRIVATE_KEY?.trim()) { + throw new ConfigError('DIFFITY_GITHUB_APP_ID needs the App configured: DIFFITY_GITHUB_APP_CLIENT_ID and DIFFITY_GITHUB_APP_CLIENT_SECRET'); + } return null; } if (!clientId || !clientSecret) { throw new ConfigError('DIFFITY_GITHUB_APP_CLIENT_ID and DIFFITY_GITHUB_APP_CLIENT_SECRET go together'); } - return { clientId, clientSecret, slug: env.DIFFITY_GITHUB_APP_SLUG?.trim() || null }; + return { clientId, clientSecret, slug: env.DIFFITY_GITHUB_APP_SLUG?.trim() || null, bot: parseGitHubBot(env) }; +} + +function parseGitHubBot(env: NodeJS.ProcessEnv): GitHubBotConfig | null { + const appId = env.DIFFITY_GITHUB_APP_ID?.trim(); + // A secret pasted as one line keeps its line breaks as literal \n. + const privateKey = env.DIFFITY_GITHUB_APP_PRIVATE_KEY?.trim().replace(/\\n/g, '\n'); + if (!appId && !privateKey) { + return null; + } + if (!appId || !privateKey) { + throw new ConfigError('DIFFITY_GITHUB_APP_ID and DIFFITY_GITHUB_APP_PRIVATE_KEY go together'); + } + if (!/^\d+$/.test(appId)) { + throw new ConfigError(`DIFFITY_GITHUB_APP_ID must be the App's numeric id, got ${appId}`); + } + try { + createPrivateKey(privateKey); + } catch { + throw new ConfigError('DIFFITY_GITHUB_APP_PRIVATE_KEY is not a PEM private key'); + } + return { appId: Number(appId), privateKey }; } export function loadConfig(env: NodeJS.ProcessEnv = process.env): Config { diff --git a/packages/server/src/db.ts b/packages/server/src/db.ts index d710b560..80d2a262 100644 --- a/packages/server/src/db.ts +++ b/packages/server/src/db.ts @@ -219,6 +219,19 @@ export const MIGRATIONS: Migration[] = [ ALTER TABLE users ADD COLUMN live_polled_at BIGINT; `, }, + { + version: 5, + sql: ` + CREATE TABLE automation_tokens ( + token_hash TEXT PRIMARY KEY, + user_id TEXT NOT NULL REFERENCES users(id) ON DELETE CASCADE, + name TEXT NOT NULL, + created_at TEXT NOT NULL, + last_used_at TEXT, + UNIQUE (user_id, name) + ); + `, + }, ]; /** Any constant; two instances starting together take turns migrating. */ diff --git a/packages/server/src/github-app.ts b/packages/server/src/github-app.ts index 3a25224d..ce47aaa2 100644 --- a/packages/server/src/github-app.ts +++ b/packages/server/src/github-app.ts @@ -1,6 +1,8 @@ -import type { GitHubAppConfig } from './config.js'; +import { sign } from 'node:crypto'; +import type { GitHubAppConfig, GitHubBotConfig } from './config.js'; import type { Db, Queryable } from './db.js'; import { decrypt, encrypt, randomToken, sha256 } from './crypto.js'; +import { keyedSerializer } from './git.js'; import type { GitHubAccess, GitHubApi } from './github.js'; type Fetch = typeof fetch; @@ -183,3 +185,47 @@ export class GitHubAppAccess implements GitHubAccess { ); } } + +/** GitHub refuses an App JWT that expires more than 10 minutes out; issued a minute back for clock drift. */ +export function appJwt(bot: GitHubBotConfig, now: number): string { + const seconds = Math.floor(now / 1000); + const encode = (value: unknown) => Buffer.from(JSON.stringify(value)).toString('base64url'); + const unsigned = `${encode({ alg: 'RS256', typ: 'JWT' })}.${encode({ iat: seconds - 60, exp: seconds + 9 * 60, iss: bot.appId })}`; + return `${unsigned}.${sign('RSA-SHA256', Buffer.from(unsigned), bot.privateKey).toString('base64url')}`; +} + +/** + * The App acting as itself, `[bot]` on GitHub, with installation tokens minted from its + * private key. A token is good for an hour and kept per installation until shortly before then. + */ +export class GitHubAppBot { + private readonly tokens = new Map(); + private readonly lock = keyedSerializer(); + + constructor( + private readonly bot: GitHubBotConfig, + private readonly options: { api: GitHubApi; slug: string | null; now?: () => number }, + ) {} + + private now(): number { + return this.options.now?.() ?? Date.now(); + } + + /** The account its comments are under; unknown without the slug. */ + get login(): string | null { + return this.options.slug ? `${this.options.slug}[bot]` : null; + } + + async installationToken(owner: string, repo: string): Promise { + const installationId = await this.options.api.repoInstallationId(appJwt(this.bot, this.now()), owner, repo); + return this.lock(String(installationId))(async () => { + const cached = this.tokens.get(installationId); + if (cached && cached.expiresAt - EXPIRY_MARGIN_MS > this.now()) { + return cached.token; + } + const minted = await this.options.api.createInstallationToken(appJwt(this.bot, this.now()), installationId); + this.tokens.set(installationId, { token: minted.token, expiresAt: Date.parse(minted.expiresAt) }); + return minted.token; + }); + } +} diff --git a/packages/server/src/github.ts b/packages/server/src/github.ts index c6f01697..cb9e5660 100644 --- a/packages/server/src/github.ts +++ b/packages/server/src/github.ts @@ -257,6 +257,20 @@ export class GitHubApi { }; } + /** Asked with the App's JWT; GitHub answers 404 when the App is not installed on the repository. */ + async repoInstallationId(appJwt: string, owner: string, repo: string): Promise { + return (await this.json<{ id: number }>(appJwt, `${repoPath(owner, repo)}/installation`)).id; + } + + async createInstallationToken(appJwt: string, installationId: number): Promise<{ token: string; expiresAt: string }> { + const json = await this.json<{ token: string; expires_at: string }>( + appJwt, + `/app/installations/${installationId}/access_tokens`, + { method: 'POST' }, + ); + return { token: json.token, expiresAt: json.expires_at }; + } + /** The token's own account; asked once per token. */ viewerLogin(token: string): Promise { let login = this.logins.get(token); diff --git a/packages/server/src/mcp.ts b/packages/server/src/mcp.ts index 6563a4ba..844182f2 100644 --- a/packages/server/src/mcp.ts +++ b/packages/server/src/mcp.ts @@ -7,10 +7,12 @@ import { AmbiguousIdError, type ReviewSessionRecord } from './reviews.js'; import { ServiceError, describeSession, type ReviewService } from './service.js'; import { clampToFile, readAnchor, splitLines } from './anchor.js'; import { MAX_WAIT_SECONDS, awaitCommand, type Live } from './live.js'; +import type { AutomationTokens } from './automation.js'; export interface McpContext { service: ReviewService; live: Live; + automationTokens: AutomationTokens; userId: string; /** Who the comments are from, as the connecting client named itself when it registered. */ agentName: string; @@ -529,6 +531,77 @@ export function createMcpServer(ctx: McpContext): McpServer { }), ); + server.registerTool( + 'post_review', + { + description: + "Post the session's open findings to its pull request as one review by the diffity GitHub App, never as the user; " + + 'the review names who asked for it. Findings already on the pull request are skipped. Without `body`, the ' + + "session's general comments are the review's summary. For automated runs; in an interactive review the reader posts from the page.", + inputSchema: { + session: sessionArg, + event: z.enum(['COMMENT', 'REQUEST_CHANGES']), + body: z.string().optional().describe('The review summary'), + }, + }, + guarded(async args => { + const result = await service.postReviewAsBot(await session(args.session), args); + if (!result.reviewUrl && result.failed > 0) { + throw new ServiceError(`Nothing was posted:\n${result.errors.join('\n')}`); + } + return json({ + posted: result.submitted, + skipped: result.skipped, + url: result.reviewUrl, + ...(result.errors.length > 0 ? { errors: result.errors } : {}), + }); + }), + ); + + const tokenName = z.string().trim().min(1).max(100).describe('What the token is for, e.g. "pr-review routine"'); + + server.registerTool( + 'automation_token', + { + description: + 'Mint a bearer token for an agent that cannot sign in, such as a scheduled routine: it reaches this MCP endpoint ' + + 'as you, with every tool, until revoked. Shown once; store it as a secret.', + inputSchema: { name: tokenName }, + }, + guarded(async args => { + const token = await ctx.automationTokens.mint(userId, args.name); + return json({ + name: args.name, + token, + use: `Send it as "Authorization: Bearer " to ${new URL('/mcp', service.publicUrl).href}. It is not shown again.`, + }); + }), + ); + + server.registerTool( + 'automation_tokens', + { + description: 'List your automation tokens, without the tokens themselves.', + inputSchema: {}, + annotations: { readOnlyHint: true }, + }, + guarded(async () => json(await ctx.automationTokens.list(userId))), + ); + + server.registerTool( + 'automation_token_revoke', + { + description: 'Revoke one of your automation tokens by name.', + inputSchema: { name: tokenName }, + }, + guarded(async args => { + if (!(await ctx.automationTokens.revoke(userId, args.name))) { + throw new ServiceError(`You have no automation token named "${args.name}"`); + } + return text(`Revoked automation token "${args.name}"`); + }), + ); + server.registerPrompt( 'review', { diff --git a/packages/server/src/review-prompt.md b/packages/server/src/review-prompt.md index 9114b07a..3408a354 100644 --- a/packages/server/src/review-prompt.md +++ b/packages/server/src/review-prompt.md @@ -35,6 +35,10 @@ tour_done { tour } tour_delete { tour } live_token { session? } → { command }: waits for the reader's questions; without a session, for everything of the user's (see Attend) +post_review { session, event, body? } automated runs only: posts the findings as the diffity App +automation_token { name } → { token }: a bearer for an agent that cannot sign in +automation_tokens {} the user's automation tokens, without the tokens +automation_token_revoke { name } ``` - `create_session` takes the repository and **exactly one** of: `pr`; `base` and `head` (full @@ -175,7 +179,11 @@ Give the user the session `url` and the counts, using the labels you used: > Found: 1 P1, 2 P2. The file list is in reading order; the P1 is on the last stop. The page is where they are read; the user posts them to the pull request from there once they have -been through them. Never post to GitHub yourself. +been through them. In an interactive review, never post to GitHub yourself. + +An automated run — you were asked to post, or you run under an automation token with nobody reading +along — ends with `post_review { session, event: "COMMENT" }` and then `review_done { session }`. The +review goes out as the diffity GitHub App, naming the user it runs for, never as the user. ### Step 7: Stay for questions diff --git a/packages/server/src/server.ts b/packages/server/src/server.ts index 926c9b2f..dbd262c8 100644 --- a/packages/server/src/server.ts +++ b/packages/server/src/server.ts @@ -7,7 +7,8 @@ import { OAuthProvider } from './oauth.js'; import { Reviews } from './reviews.js'; import { Mirrors, githubRemoteUrl, type RemoteUrlBuilder } from './git.js'; import { GitHubApi, StoredTokenAccess, type GitHubAccess } from './github.js'; -import { GitHubAppAccess } from './github-app.js'; +import { GitHubAppAccess, GitHubAppBot } from './github-app.js'; +import { AutomationTokens } from './automation.js'; import { ReviewService } from './service.js'; import { DevLoginProvider, IapLogin } from './login.js'; import { Live } from './live.js'; @@ -56,8 +57,9 @@ export async function startServer( fallbackToken: config.devGitHubToken, }) : null; + const bot = config.githubApp?.bot ? new GitHubAppBot(config.githubApp.bot, { api, slug: config.githubApp.slug }) : null; const access = overrides.gitHubAccess ?? githubApp ?? new StoredTokenAccess(users, config.devGitHubToken); - const service = new ReviewService(reviews, mirrors, api, access, config.publicUrl, users); + const service = new ReviewService(reviews, mirrors, api, access, config.publicUrl, users, bot); const live = new Live(db); const app = createApp({ @@ -65,6 +67,7 @@ export async function startServer( users, webSessions, oauth, + automationTokens: new AutomationTokens(db), service, live, devLogin: config.devLogin ? new DevLoginProvider(config) : null, diff --git a/packages/server/src/service.ts b/packages/server/src/service.ts index c1e1e3c0..eac77a86 100644 --- a/packages/server/src/service.ts +++ b/packages/server/src/service.ts @@ -1,15 +1,17 @@ import { parseDiff } from '@diffity/parser'; -import type { - CommentThread, - DiffResponse, - GitHubDetails, - PullCommentsResult, - ReviewResult, - ReviewSubmission, - Suppressed, +import { + GENERAL_THREAD_FILE_PATH, + type CommentThread, + type DiffResponse, + type GitHubDetails, + type PullCommentsResult, + type ReviewResult, + type ReviewSubmission, + type Suppressed, } from '@diffity/api'; import { isSha, isSafeRepoPath, isRepoName, keyedSerializer, type Mirrors, type NameStatus } from './git.js'; import type { GitHubAccess, GitHubApi, ReviewCommentPayload, ReviewRequest } from './github.js'; +import type { GitHubAppBot } from './github-app.js'; import { prKey, type PrMeta, type ReviewSessionRecord, type Reviews } from './reviews.js'; import type { Users, UserSettings } from './users.js'; import { followRename, reanchor, splitLines } from './anchor.js'; @@ -153,6 +155,7 @@ export class ReviewService { private readonly access: GitHubAccess, readonly publicUrl: URL, private readonly users: Users, + private readonly bot: GitHubAppBot | null = null, ) {} settingsUrl(): string { @@ -471,6 +474,61 @@ export class ReviewService { if (submission.event === 'COMMENT' && submission.comments.length === 0 && !submission.body.trim()) { throw new ServiceError('A comment review needs a summary or at least one comment'); } + return this.submitReview(session, submission, { token, login: this.github.viewerLogin(token), footer: '' }); + } + + /** + * The session's open findings, posted by the GitHub App as itself rather than as anyone, with a + * footer naming who asked for it. A general comment is posted once, as the summary; findings + * already on the pull request are skipped, so a second run on the same head adds only what is new. + */ + async postReviewAsBot( + session: ReviewSessionRecord, + input: { event: 'COMMENT' | 'REQUEST_CHANGES'; body?: string }, + ): Promise { + if (!this.bot) { + throw new ServiceError('Bot posting is not configured on this server'); + } + if (session.prNumber === null) { + throw new ServiceError('Only a pull request session can be posted to or pulled from GitHub'); + } + const threads = await this.reviews.threadsForSession(session.userId, session.id, 'open'); + const general = threads.filter(thread => thread.filePath === GENERAL_THREAD_FILE_PATH && !thread.submittedAt && findingOf(thread)); + const comments = threads.flatMap(thread => { + const finding = findingOf(thread); + return thread.filePath === GENERAL_THREAD_FILE_PATH || !finding ? [] : [{ + threadId: thread.id, + filePath: thread.filePath, + side: thread.side === 'old' ? 'LEFT' as const : 'RIGHT' as const, + startLine: thread.startLine !== thread.endLine ? thread.startLine : null, + endLine: thread.endLine, + body: finding, + }]; + }); + const body = input.body ?? general.map(findingOf).join('\n\n'); + const requester = (await this.users.get(session.userId))?.githubLogin; + const token = await this.bot.installationToken(session.owner, session.repo); + const result = await this.submitReview(session, { event: input.event, body, comments }, { + token, + login: Promise.resolve(this.bot.login), + footer: `— diffity review, requested by ${requester ? `@${requester}` : 'a diffity user'}`, + }); + if (result.reviewUrl && input.body === undefined && general.length > 0) { + await this.reviews.markThreadsSubmitted( + session.userId, + general.map(thread => ({ threadId: thread.id, body: findingOf(thread) })), + { reviewUrl: result.reviewUrl, headSha: session.headSha }, + ); + } + return result; + } + + private async submitReview( + session: ReviewSessionRecord, + submission: ReviewSubmission, + as: { token: string; login: Promise; footer: string }, + ): Promise { + const { token } = as; const { owner, repo } = session; const prNumber = session.prNumber!; return this.githubLock(session.userId)(async () => { @@ -488,7 +546,7 @@ export class ReviewService { const [patch, existing, viewerLogin] = await Promise.all([ this.diffText(session), this.github.pullComments(token, owner, repo, prNumber), - this.github.viewerLogin(token), + as.login, ]); const commentable = commentableLines(patch); const threadIds = await this.reviews.threadsOnTheForge( @@ -540,7 +598,7 @@ export class ReviewService { review = await this.github.createReview(token, owner, repo, prNumber, { commit_id: session.headSha, event: submission.event, - body, + body: [body, as.footer].filter(Boolean).join('\n\n'), comments: payload, }); } catch (err) { @@ -641,6 +699,11 @@ export class ReviewService { } } +/** Only the finding travels, never the conversation that followed it. */ +function findingOf(thread: CommentThread): string | undefined { + return thread.comments.find(comment => comment.kind === 'review')?.body; +} + function sessionMode(input: CreateSessionInput): 'pr' | 'shas' | 'patch' { const hasPr = input.pr !== undefined; const hasHead = input.head !== undefined; diff --git a/packages/server/tests/config.test.ts b/packages/server/tests/config.test.ts index bd45e79f..378e5dab 100644 --- a/packages/server/tests/config.test.ts +++ b/packages/server/tests/config.test.ts @@ -1,9 +1,11 @@ import { describe, expect, it } from 'vitest'; -import { randomBytes } from 'node:crypto'; +import { generateKeyPairSync, randomBytes } from 'node:crypto'; import { ConfigError, isAllowedEmail, loadConfig } from '../src/config.js'; import { decrypt, encrypt, randomToken, safeEqual, sha256 } from '../src/crypto.js'; const key = randomBytes(32).toString('base64'); +const pem = generateKeyPairSync('rsa', { modulusLength: 2048 }).privateKey.export({ type: 'pkcs1', format: 'pem' }).toString(); +const app = { DIFFITY_DATA_DIR: '/d', DIFFITY_GITHUB_APP_CLIENT_ID: 'Iv1.x', DIFFITY_GITHUB_APP_CLIENT_SECRET: 's' }; describe('loadConfig', () => { it('fills in the localhost defaults', () => { @@ -42,6 +44,8 @@ describe('loadConfig', () => { DIFFITY_GITHUB_APP_CLIENT_ID: 'Iv1.x', DIFFITY_GITHUB_APP_CLIENT_SECRET: 's3cret', DIFFITY_GITHUB_APP_SLUG: 'diffity-nc', + DIFFITY_GITHUB_APP_ID: '5312078', + DIFFITY_GITHUB_APP_PRIVATE_KEY: pem, DATABASE_URL: 'postgresql://u:p@10.0.0.3/diffity', DIFFITY_PG_CA: '-----BEGIN CERTIFICATE-----', DIFFITY_TRUST_PROXY: '1', @@ -56,7 +60,7 @@ describe('loadConfig', () => { expect(config.devGitHubToken).toBe('ghp_x'); expect(config.githubApiUrl).toBe('http://127.0.0.1:1'); expect(config.githubUrl).toBe('http://127.0.0.1:2'); - expect(config.githubApp).toEqual({ clientId: 'Iv1.x', clientSecret: 's3cret', slug: 'diffity-nc' }); + expect(config.githubApp).toEqual({ clientId: 'Iv1.x', clientSecret: 's3cret', slug: 'diffity-nc', bot: { appId: 5312078, privateKey: pem.trim() } }); expect(config.databaseUrl).toBe('postgresql://u:p@10.0.0.3/diffity'); expect(config.pgCa).toBe('-----BEGIN CERTIFICATE-----'); expect(config.trustProxy).toBe(1); @@ -77,6 +81,14 @@ describe('loadConfig', () => { expect(proxy('loopback, 10.0.0.0/8')).toBe('loopback, 10.0.0.0/8'); }); + it('takes the App private key as one line with literal \\n, and leaves bot posting off without it', () => { + const oneLine = pem.trim().replace(/\n/g, '\\n'); + expect(oneLine).not.toContain('\n'); + expect(loadConfig({ ...app, DIFFITY_GITHUB_APP_ID: '1', DIFFITY_GITHUB_APP_PRIVATE_KEY: oneLine }).githubApp?.bot) + .toEqual({ appId: 1, privateKey: pem.trim() }); + expect(loadConfig(app).githubApp?.bot).toBeNull(); + }); + it('binds every interface for a public URL, or where told to', () => { expect(loadConfig({ DIFFITY_DATA_DIR: '/d', DIFFITY_PUBLIC_URL: 'https://d.example.com', DIFFITY_SECRET_KEY: key }).bindHost) .toBe('0.0.0.0'); @@ -102,6 +114,11 @@ describe('loadConfig', () => { ], [{ DIFFITY_DATA_DIR: '/d', DIFFITY_GITHUB_APP_CLIENT_ID: 'Iv1.x' }, 'go together'], [{ DIFFITY_DATA_DIR: '/d', DIFFITY_GITHUB_APP_CLIENT_SECRET: 's' }, 'go together'], + [{ ...app, DIFFITY_GITHUB_APP_ID: '1' }, 'DIFFITY_GITHUB_APP_ID and DIFFITY_GITHUB_APP_PRIVATE_KEY go together'], + [{ ...app, DIFFITY_GITHUB_APP_PRIVATE_KEY: pem }, 'go together'], + [{ ...app, DIFFITY_GITHUB_APP_ID: 'diffity', DIFFITY_GITHUB_APP_PRIVATE_KEY: pem }, 'numeric id'], + [{ ...app, DIFFITY_GITHUB_APP_ID: '1', DIFFITY_GITHUB_APP_PRIVATE_KEY: 'not a key' }, 'not a PEM private key'], + [{ DIFFITY_DATA_DIR: '/d', DIFFITY_GITHUB_APP_ID: '1', DIFFITY_GITHUB_APP_PRIVATE_KEY: pem }, 'needs the App configured'], [{ DIFFITY_DATA_DIR: '/d', DIFFITY_DEV_LOGIN: '1', DIFFITY_IAP_AUDIENCE: '/projects/1/global/backendServices/2' }, 'set one'], ])('rejects %j', (env, message) => { expect(() => loadConfig(env)).toThrow(ConfigError); diff --git a/packages/server/tests/db.test.ts b/packages/server/tests/db.test.ts index 789a296e..2c63464b 100644 --- a/packages/server/tests/db.test.ts +++ b/packages/server/tests/db.test.ts @@ -27,7 +27,7 @@ describe('migrations', () => { .map(row => row.tablename); expect(tables).toEqual(expect.arrayContaining([ 'users', 'web_sessions', 'github_oauth_states', 'oauth_clients', 'oauth_codes', 'oauth_tokens', - 'repos', 'sessions', 'threads', 'comments', 'tours', 'tour_steps', 'live_requests', 'live_tokens', 'schema_version', + 'repos', 'sessions', 'threads', 'comments', 'tours', 'tour_steps', 'live_requests', 'live_tokens', 'automation_tokens', 'schema_version', ])); await db.close(); diff --git a/packages/server/tests/github-app.test.ts b/packages/server/tests/github-app.test.ts index 8483841f..05f95218 100644 --- a/packages/server/tests/github-app.test.ts +++ b/packages/server/tests/github-app.test.ts @@ -1,4 +1,7 @@ +import { generateKeyPairSync, verify } from 'node:crypto'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { GitHubApi } from '../src/github.js'; +import { GitHubAppBot, appJwt } from '../src/github-app.js'; import { login, removeDir, startFakeGitHub, startTestServer, tempDir, type FakeGitHub, type TestServer } from './helpers.js'; let github: FakeGitHub; @@ -44,7 +47,7 @@ beforeAll(async () => { }; dataDir = tempDir('github-app'); server = await startTestServer(dataDir, github.url, {}, { - githubApp: { clientId: 'Iv1.client', clientSecret: 'app-secret', slug: 'diffity-nc' }, + githubApp: { clientId: 'Iv1.client', clientSecret: 'app-secret', slug: 'diffity-nc', bot: null }, }); aliceCookie = await login(server.base, 'alice@example.com'); bobCookie = await login(server.base, 'bob@example.com'); @@ -140,3 +143,54 @@ describe('connecting GitHub through the app', () => { expect(await (await get('/settings', aliceCookie)).text()).toContain('Not connected'); }); }); + +describe('the App acting as itself', () => { + const { publicKey, privateKey } = generateKeyPairSync('rsa', { modulusLength: 2048 }); + const bot = { appId: 5312078, privateKey: privateKey.export({ type: 'pkcs1', format: 'pem' }).toString() }; + + function decode(part: string): Record { + return JSON.parse(Buffer.from(part, 'base64url').toString('utf8')) as Record; + } + + it('signs a JWT that verifies against the App key, issued a minute back and good for nine minutes', () => { + const jwt = appJwt(bot, 1_700_000_000_500); + const [header, claims, signature] = jwt.split('.'); + expect(decode(header)).toEqual({ alg: 'RS256', typ: 'JWT' }); + expect(decode(claims)).toEqual({ iat: 1_700_000_000 - 60, exp: 1_700_000_000 + 540, iss: 5312078 }); + expect(verify('RSA-SHA256', Buffer.from(`${header}.${claims}`), publicKey, Buffer.from(signature, 'base64url'))).toBe(true); + const other = generateKeyPairSync('rsa', { modulusLength: 2048 }).publicKey; + expect(verify('RSA-SHA256', Buffer.from(`${header}.${claims}`), other, Buffer.from(signature, 'base64url'))).toBe(false); + }); + + it('mints an installation token once however many ask, keeps it, and mints again shortly before it expires', async () => { + github.installations['acme/widgets'] = 42; + let now = Date.now(); + const app = new GitHubAppBot(bot, { api: new GitHubApi(github.url), slug: 'diffity', now: () => now }); + expect(app.login).toBe('diffity[bot]'); + const before = github.installationTokenRequests.length; + + const [first, second] = await Promise.all([app.installationToken('acme', 'widgets'), app.installationToken('Acme', 'Widgets')]); + expect(second).toBe(first); + expect(await app.installationToken('acme', 'widgets')).toBe(first); + const minted = github.installationTokenRequests.slice(before); + expect(minted).toHaveLength(1); + expect(minted[0].installationId).toBe(42); + const [header, claims, signature] = minted[0].jwt!.split('.'); + expect(verify('RSA-SHA256', Buffer.from(`${header}.${claims}`), publicKey, Buffer.from(signature, 'base64url'))).toBe(true); + + now += 30 * 60 * 1000; + expect(await app.installationToken('acme', 'widgets')).toBe(first); + expect(github.installationTokenRequests.length).toBe(before + 1); + + now += 30 * 60 * 1000; + const renewed = await app.installationToken('acme', 'widgets'); + expect(renewed).not.toBe(first); + expect(github.installationTokenRequests.length).toBe(before + 2); + }); + + it('says what GitHub answered when the App is not installed on the repository', async () => { + const app = new GitHubAppBot(bot, { api: new GitHubApi(github.url), slug: null }); + expect(app.login).toBeNull(); + await expect(app.installationToken('acme', 'elsewhere')).rejects.toThrow(/404 for GET \/repos\/acme\/elsewhere\/installation: Not Found/); + }); +}); diff --git a/packages/server/tests/helpers.ts b/packages/server/tests/helpers.ts index 780e5ed7..e5af2d8e 100644 --- a/packages/server/tests/helpers.ts +++ b/packages/server/tests/helpers.ts @@ -170,6 +170,14 @@ export interface FakeGitHub { searches: string[]; /** Makes `GET /search/issues` answer this status instead. */ failSearchWith: number | null; + /** The GitHub App's installation id per lowercased "owner/repo"; elsewhere it is not installed. */ + installations: Record; + /** Every `POST /app/installations/:id/access_tokens`, with the App JWT it carried. */ + installationTokenRequests: { installationId: number; jwt: string | null }[]; + /** How long a minted installation token lives. */ + installationTokenTtlMs: number; + /** The bearer of each `POST .../reviews`, in order. */ + reviewTokens: (string | null)[]; close(): Promise; } @@ -191,6 +199,10 @@ export async function startFakeGitHub(repos: FakeRepo[]): Promise { reviewRequests: {}, searches: [], failSearchWith: null, + installations: {}, + installationTokenRequests: [], + installationTokenTtlMs: 60 * 60 * 1000, + reviewTokens: [], close: async () => {}, }; const server: Server = createServer((req, res) => { @@ -263,6 +275,24 @@ export async function startFakeGitHub(repos: FakeRepo[]): Promise { return; } + const installation = /^\/repos\/([^/]+)\/([^/]+)\/installation$/.exec(url.pathname); + if (installation) { + const id = fake.installations[`${installation[1]}/${installation[2]}`.toLowerCase()]; + send(id ? 200 : 404, id ? { id } : { message: 'Not Found' }); + return; + } + const accessTokens = /^\/app\/installations\/(\d+)\/access_tokens$/.exec(url.pathname); + if (accessTokens && req.method === 'POST') { + fake.installationTokenRequests.push({ installationId: Number(accessTokens[1]), jwt: token }); + const minted = `ghs_${fake.installationTokenRequests.length}`; + // Installed on every repository here, so the token reads them all. + for (const repo of repos) { + repo.tokens.push(minted); + } + send(201, { token: minted, expires_at: new Date(Date.now() + fake.installationTokenTtlMs).toISOString() }); + return; + } + const match = /^\/repos\/([^/]+)\/([^/]+)(?:\/pulls\/(\d+)(\/.*)?)?$/.exec(url.pathname); const repo = match ? repos.find(r => r.owner.toLowerCase() === match[1].toLowerCase() && r.name.toLowerCase() === match[2].toLowerCase()) @@ -294,6 +324,7 @@ export async function startFakeGitHub(repos: FakeRepo[]): Promise { } const review = JSON.parse(raw) as FakeGitHub['postedReviews'][number]; fake.postedReviews.push(review); + fake.reviewTokens.push(token); const reviewId = nextId++; for (const comment of review.comments) { fake.comments.push({ diff --git a/packages/server/tests/mcp.test.ts b/packages/server/tests/mcp.test.ts index 45ec61cb..d9c0cafd 100644 --- a/packages/server/tests/mcp.test.ts +++ b/packages/server/tests/mcp.test.ts @@ -1,3 +1,4 @@ +import { generateKeyPairSync } from 'node:crypto'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js'; @@ -22,9 +23,10 @@ let dataDir: string; let server: TestServer; let alice: Client; let bob: Client; +let pulls: Record; -async function mcpClient(accessToken: string): Promise { - const transport = new StreamableHTTPClientTransport(new URL(`${server.base}/mcp`), { +async function mcpClient(accessToken: string, base = server.base): Promise { + const transport = new StreamableHTTPClientTransport(new URL(`${base}/mcp`), { requestInit: { headers: { Authorization: `Bearer ${accessToken}` } }, }); const client = new Client({ name: 'test', version: '1.0.0' }); @@ -60,16 +62,20 @@ async function failed(client: Client, name: string, args: Record { + const res = await fetch(`${base}/mcp`, { + method: 'POST', + headers: { Authorization: `Bearer ${token}`, 'Content-Type': 'application/json', Accept: 'application/json, text/event-stream' }, + body: JSON.stringify({ jsonrpc: '2.0', id: 1, method: 'tools/list' }), + }); + return res.status; +} + beforeAll(async () => { fixture = makeFixture(); + pulls = { 1: { title: 'Change line ten', baseSha: fixture.mainTip, headSha: fixture.head1 } }; github = await startFakeGitHub([ - { - owner: 'acme', - name: 'widgets', - private: true, - tokens: ['alice-token', 'bob-token'], - pulls: { 1: { title: 'Change line ten', baseSha: fixture.mainTip, headSha: fixture.head1 } }, - }, + { owner: 'acme', name: 'widgets', private: true, tokens: ['alice-token', 'bob-token'], pulls }, ]); dataDir = tempDir('mcp'); server = await startTestServer(dataDir, github.url, { remoteUrl: fixture.remoteUrl, uiDir: fakeUiDir(dataDir) }); @@ -122,9 +128,10 @@ describe('a review through the tools', () => { it('lists every tool', async () => { const { tools } = await alice.listTools(); expect(tools.map(tool => tool.name).sort()).toEqual([ - 'amend', 'comment', 'create_session', 'dismiss', 'general_comment', 'get_diff', 'get_file', 'get_standards', - 'list_comments', 'list_review_requests', 'list_sessions', 'live_token', 'reply', 'resolve', 'review_done', 'review_start', 'tour_delete', 'tour_done', - 'tour_start', 'tour_step', + 'amend', 'automation_token', 'automation_token_revoke', 'automation_tokens', 'comment', 'create_session', 'dismiss', + 'general_comment', 'get_diff', 'get_file', 'get_standards', 'list_comments', 'list_review_requests', 'list_sessions', + 'live_token', 'post_review', 'reply', 'resolve', 'review_done', 'review_start', 'tour_delete', 'tour_done', 'tour_start', + 'tour_step', ]); }); @@ -240,6 +247,7 @@ describe('the review prompt', () => { expect(text).toMatch(/^Review pull request #7 of acme\/widgets: `create_session \{ repo: "acme\/widgets", pr: 7 \}`\./); expect(text).toContain('review_start'); expect(text).toContain('### Step 6: Hand over the review'); + expect(text).toContain('post_review'); expect(text).not.toContain('user-invocable'); const repoOnly = await alice.getPrompt({ name: 'review', arguments: { repo: 'acme/widgets' } }); expect((repoOnly.messages[0].content as { text: string }).text).toMatch(/^Review a change in acme\/widgets\./); @@ -275,3 +283,122 @@ describe('isolation', () => { await ok(alice, 'tour_delete', { tour: tour.tour }); }); }); + +describe('automation tokens', () => { + it('mint one shown once, list them without it, and keep names unique per user', async () => { + const minted = await ok<{ name: string; token: string; use: string }>(alice, 'automation_token', { name: 'pr routine' }); + expect(minted.token).toMatch(/^dfa_[A-Za-z0-9_-]{43}$/); + expect(minted.use).toContain(`${server.base}/mcp`); + expect(await failed(alice, 'automation_token', { name: 'pr routine' })).toContain('already have an automation token named "pr routine"'); + await ok(bob, 'automation_token', { name: 'pr routine' }); + + const listed = await ok<{ name: string; createdAt: string; lastUsedAt: string | null }[]>(alice, 'automation_tokens', {}); + expect(listed).toEqual([{ name: 'pr routine', createdAt: expect.any(String), lastUsedAt: null }]); + expect(JSON.stringify(listed)).not.toContain(minted.token); + expect(JSON.stringify(await server.db.query('SELECT * FROM automation_tokens'))).not.toContain(minted.token); + await ok(alice, 'automation_token_revoke', { name: 'pr routine' }); + await ok(bob, 'automation_token_revoke', { name: 'pr routine' }); + }); + + it('reach /mcp as the user who minted them, until revoked', async () => { + const { token } = await ok<{ token: string }>(alice, 'automation_token', { name: 'routine' }); + const routine = await mcpClient(token); + try { + const created = await ok<{ session: string }>(routine, 'create_session', { repo: 'acme/widgets', base: fixture.base, head: fixture.head1 }); + expect((await ok<{ session: string }[]>(alice, 'list_sessions', {})).map(s => s.session)).toContain(created.session); + expect((await ok<{ session: string }[]>(bob, 'list_sessions', {})).map(s => s.session)).not.toContain(created.session); + const finding = await ok<{ thread: string }>(routine, 'comment', { session: created.session, file: 'added.ts', line: 1, body: 'From the routine' }); + const [thread] = (await ok<{ id: string; comments: { author: { name: string; type: string } }[] }[]>(alice, 'list_comments', { session: created.session })) + .filter(t => t.id === finding.thread); + expect(thread.comments[0].author).toEqual({ name: 'routine', type: 'agent' }); + expect((await ok<{ lastUsedAt: string | null }[]>(alice, 'automation_tokens', {}))[0].lastUsedAt).not.toBeNull(); + + await ok(alice, 'automation_token_revoke', { name: 'routine' }); + expect(await mcpStatus(token)).toBe(401); + expect(await failed(alice, 'automation_token_revoke', { name: 'routine' })).toContain('no automation token named'); + } finally { + await routine.close(); + } + }); + + it('refuses a made-up one, and leaves OAuth tokens to OAuth', async () => { + expect(await mcpStatus(`dfa_${'x'.repeat(43)}`)).toBe(401); + expect(await mcpStatus('dfa_')).toBe(401); + expect((await alice.listTools()).tools.length).toBeGreaterThan(0); + }); +}); + +describe('post_review', () => { + it('answers that bot posting is not configured on a server without the App key', async () => { + const { session } = await ok<{ session: string }>(alice, 'create_session', { repo: 'acme/widgets', pr: 1 }); + expect(await failed(alice, 'post_review', { session, event: 'COMMENT' })).toContain('not configured on this server'); + }); + + it('takes no approval', async () => { + const { session } = await ok<{ session: string }>(alice, 'create_session', { repo: 'acme/widgets', pr: 1 }); + expect(await failed(alice, 'post_review', { session, event: 'APPROVE' })).toContain('Input validation error'); + const tool = (await alice.listTools()).tools.find(t => t.name === 'post_review')!; + expect((tool.inputSchema.properties as Record).event.enum).toEqual(['COMMENT', 'REQUEST_CHANGES']); + }); + + describe('on a server with the App key', () => { + let botDir: string; + let botServer: TestServer; + let routine: Client; + + beforeAll(async () => { + const pem = generateKeyPairSync('rsa', { modulusLength: 2048 }).privateKey.export({ type: 'pkcs1', format: 'pem' }).toString(); + github.installations['acme/widgets'] = 7; + botDir = tempDir('mcp-bot'); + botServer = await startTestServer( + botDir, + github.url, + { remoteUrl: fixture.remoteUrl, uiDir: fakeUiDir(botDir), gitHubAccess: { tokenFor: async () => 'alice-token' } }, + { githubApp: { clientId: 'Iv1.client', clientSecret: 'secret', slug: 'diffity', bot: { appId: 1, privateKey: pem } } }, + ); + const cookie = await login(botServer.base, 'alice@example.com'); + await botServer.users.setGitHubToken((await botServer.users.findOrCreate('alice@example.com')).id, 'alice-token', 'alice-gh'); + const interactive = await mcpClient((await connectAgent(botServer.base, cookie)).accessToken, botServer.base); + const { token } = await ok<{ token: string }>(interactive, 'automation_token', { name: 'pr routine' }); + await interactive.close(); + routine = await mcpClient(token, botServer.base); + }); + + afterAll(async () => { + await routine?.close(); + await botServer?.close(); + removeDir(botDir); + }); + + it('posts the open findings as the App, with the summary and who asked, and only what is new the second time', async () => { + const { session } = await ok<{ session: string }>(routine, 'create_session', { repo: 'acme/widgets', pr: 1 }); + await ok(routine, 'comment', { session, file: 'src.ts', line: 10, body: 'P2: line ten changed without a test' }); + await ok(routine, 'general_comment', { session, body: 'One finding.' }); + + const before = github.postedReviews.length; + const posted = await ok<{ posted: number; skipped: number; url: string }>(routine, 'post_review', { session, event: 'COMMENT' }); + expect(posted).toEqual({ posted: 1, skipped: 0, url: expect.stringContaining('#pullrequestreview-') }); + expect(github.postedReviews.length).toBe(before + 1); + expect(github.postedReviews.at(-1)).toEqual({ + commit_id: fixture.head1, + event: 'COMMENT', + body: 'One finding.\n\n— diffity review, requested by @alice-gh', + comments: [{ path: 'src.ts', side: 'RIGHT', line: 10, body: 'P2: line ten changed without a test' }], + }); + expect(github.reviewTokens.at(-1)).toMatch(/^ghs_/); + + expect(await ok(routine, 'post_review', { session, event: 'COMMENT' })).toEqual({ posted: 0, skipped: 1, url: null }); + expect(github.postedReviews.length).toBe(before + 1); + }); + + it('refuses once the pull request has moved past the session', async () => { + const { session } = await ok<{ session: string }>(routine, 'create_session', { repo: 'acme/widgets', pr: 1 }); + pulls[1].headSha = fixture.head2; + try { + expect(await failed(routine, 'post_review', { session, event: 'REQUEST_CHANGES', body: 'Changes, please' })).toContain('moved on'); + } finally { + pulls[1].headSha = fixture.head1; + } + }); + }); +}); From 69fbc5df07fb290c18cab22aedc6755b4477acb4 Mon Sep 17 00:00:00 2001 From: "Fredrik Liljegren (Claude Code Fable 5.1)" Date: Fri, 16 Oct 2026 18:07:00 +0200 Subject: [PATCH 2/2] build: the test workflow uses public actions, since a public repository cannot use the private shared ones Every run on the hosted branch failed at "Unable to resolve action naturalcycles/ci-typescript": diffity is public, ci-typescript is private. pnpm/action-setup + actions/setup-node do the same job; the push trigger follows the branch rename to develop. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Bwp5QefjsjMFeq6CK6cT6w --- .github/workflows/test.yml | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 619ad224..86760b08 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -3,7 +3,7 @@ name: test on: pull_request: push: - branches: [hosted-server] + branches: [develop] permissions: contents: read @@ -13,7 +13,14 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v5 - - uses: NaturalCycles/ci-typescript/actions/setup-node-pnpm@v2 + # The shared NaturalCycles/ci-typescript actions live in a private repository, which a public + # repository cannot use; these are the public equivalents (pnpm's version comes from packageManager). + - uses: pnpm/action-setup@v4 + - uses: actions/setup-node@v5 + with: + node-version: 24 + cache: pnpm + - run: pnpm install --frozen-lockfile # Typecheck resolves workspace imports through built dists, so the build comes first. - run: pnpm build - run: pnpm test