diff --git a/changelog.d/lost-answer.fixed.md b/changelog.d/lost-answer.fixed.md new file mode 100644 index 0000000..b37d646 --- /dev/null +++ b/changelog.d/lost-answer.fixed.md @@ -0,0 +1,7 @@ +- A write tool that got no answer from GitHub, or a 5xx, no longer says it + failed: it says whether the write took effect is unknown, and to look on + GitHub before trying again, since a retry may do it twice. The comment + tools, which answered such a failure with a JSON-RPC error, now answer with + an error result saying so, and add that rollback has no record of the write + if it took effect. A read, and a write GitHub refused with a 4xx, still say + they failed. diff --git a/src/github-adapter.ts b/src/github-adapter.ts index 00d6419..36321af 100644 --- a/src/github-adapter.ts +++ b/src/github-adapter.ts @@ -192,6 +192,8 @@ export class GitHubApiError extends Error { githubMessage: string, /** The GitHubApi method that failed, e.g. `getIssue`. Never a URL. */ readonly operation: string, + readonly possiblyDone = false, + readonly answered = true, ) { const safe = redactSecrets(githubMessage); super(`GitHub ${status} on ${operation}: ${safe}`); @@ -230,6 +232,8 @@ interface Failure { status: number; message: string; headers: Record; + read: boolean; + answered: boolean; } // ───────────────────────────────────────────────────────────────────────────── @@ -835,12 +839,15 @@ export class GitHubAdapter implements GitHubApi { try { return await fn(); } catch (raw) { - // Auth runs inside the transport, so a failure to mint a token or to - // find the installation surfaces here wrapped in octokit's generic - // "the fetch threw" error. Unwrap it and let the real status and the - // real operation out instead of a 500 from `getIssue`. + // Auth runs inside the transport, before the request is sent, so a + // failure to mint a token or to find the installation surfaces here + // wrapped in octokit's generic "the fetch threw" error. Unwrap it and + // let the real status and the real operation out instead of a 500 + // from `getIssue`, as a failure of a request that never went out. const authFailure = authFailureIn(raw); - if (authFailure) throw authFailure; + if (authFailure) { + throw new GitHubApiError(authFailure.status, authFailure.githubMessage, authFailure.operation); + } const failure = normalizeFailure(raw); @@ -864,7 +871,13 @@ export class GitHubAdapter implements GitHubApi { continue; } - throw new GitHubApiError(failure.status, this.redact(failure.message), operation); + throw new GitHubApiError( + failure.status, + this.redact(failure.message), + operation, + failure.status >= 500 && !failure.read, + failure.answered, + ); } } } @@ -1713,8 +1726,9 @@ export function summarizeChecks( } /** - * Turn whatever octokit threw into a status, GitHub's message and the headers - * the retry decision needs. Nothing else of the error is kept: octokit's + * Turn whatever octokit threw into a status, GitHub's message, the headers + * the retry decision needs, whether the request only read, and whether GitHub + * answered at all. Nothing else of the error is kept: octokit's * `RequestError` carries the request it made, including its headers, and that * object must not travel any further than this function. */ @@ -1722,10 +1736,13 @@ function normalizeFailure(raw: unknown): Failure { const err = raw as { status?: number; message?: string; + request?: { method?: string; url?: string; body?: unknown }; response?: { headers?: Record; data?: unknown; status?: number }; errors?: Array<{ message?: string }>; } | null; + const read = isReadRequest(err?.request?.method, err?.request?.url, err?.request?.body); + const answered = err?.response !== undefined; const headers: Record = {}; for (const [k, v] of Object.entries(err?.response?.headers ?? {})) { if (typeof v === 'string' || typeof v === 'number') headers[k.toLowerCase()] = String(v); @@ -1735,10 +1752,26 @@ function normalizeFailure(raw: unknown): Failure { // a GraphqlResponseError, which has no status of its own. if (Array.isArray(err?.errors) && err.errors.length > 0 && err?.status === undefined) { const message = err.errors.map((e) => e?.message ?? '').filter(Boolean).join('; '); - return { status: err.response?.status ?? 200, message: message || 'GraphQL error', headers }; + return { status: err.response?.status ?? 200, message: message || 'GraphQL error', headers, read, answered }; } const data = err?.response?.data as { message?: string } | undefined; const message = data?.message ?? err?.message ?? 'unknown error'; - return { status: err?.status ?? err?.response?.status ?? 0, message, headers }; + return { status: err?.status ?? err?.response?.status ?? 0, message, headers, read, answered }; +} + +function isReadRequest(method: string | undefined, url: string | undefined, body: unknown): boolean { + const verb = method?.toUpperCase(); + if (verb === 'GET' || verb === 'HEAD') return true; + if (verb !== 'POST' || !url?.endsWith('/graphql')) return false; + let document = body; + if (typeof document === 'string') { + try { + document = JSON.parse(document); + } catch { + return false; + } + } + const query = (document as { query?: unknown } | null)?.query; + return typeof query === 'string' && /^(?:\s|#[^\n]*)*(?:query\b|\{)/.test(query); } diff --git a/src/server.ts b/src/server.ts index 9a6f4f5..a34d9cb 100644 --- a/src/server.ts +++ b/src/server.ts @@ -982,16 +982,17 @@ export class GitHubMcplServer { }; const result = await module.call(name, args, ctx); - // A refused or failed call touched nothing: it may have read a thread on - // the way to refusing, and that is not a reason to announce a channel or - // to claim the bot has written anywhere. + // An error result claims nothing: the call may have read a thread on the + // way to refusing, or written where GitHub's answer was lost, and neither + // is a reason to announce a channel or to claim the bot has written + // anywhere. if (result.isError === true) return result; await this.afterToolCall(name, touches, changes); // §8.2: a rollback-capable feature set checkpoints after the write, and // the result names the checkpoint and its parent. A failed call is not - // checkpointed — there is nothing to undo. + // checkpointed: nothing it may have done is booked to undo. if (module.featureSet !== COMMENTS_FEATURE_SET) return result; try { return { ...result, state: await this.checkpoint(COMMENTS_FEATURE_SET) }; diff --git a/src/tool-support.ts b/src/tool-support.ts index ddc4f0b..4a13964 100644 --- a/src/tool-support.ts +++ b/src/tool-support.ts @@ -142,6 +142,8 @@ export function describeFailure(toolName: string, err: unknown): string { const tool = field(toolName); if (err instanceof ToolRefusal) return `${tool} refused: ${err.message}`; if (err instanceof ToolArgumentError) return `${tool}: ${err.message}`; + const uncertain = uncertainWrite(err); + if (uncertain) return `${tool}: ${uncertain}`; const message = err instanceof Error ? err.message : String(err); const status = statusOf(err); return status === undefined @@ -149,10 +151,27 @@ export function describeFailure(toolName: string, err: unknown): string { : `${tool} failed: GitHub error ${status}: ${bounded(message, LIMITS.commentChars)}`; } +export function uncertainWrite(err: unknown): string | null { + const e = err as { + possiblyDone?: unknown; + answered?: unknown; + status?: unknown; + operation?: unknown; + githubMessage?: unknown; + } | null; + if (e?.possiblyDone !== true) return null; + const operation = field(String(e.operation)); + const detail = bounded(String(e.githubMessage), LIMITS.commentChars); + const what = + e.answered === false + ? `no answer came back from GitHub for ${operation} (${detail})` + : `GitHub answered ${operation} with ${String(e.status)} (${detail})`; + return `${what}, so whether it took effect is unknown. Look on GitHub before trying again: a retry may do it twice.`; +} + /** - * Run one tool body. Every refusal, malformed call and GitHub error comes back - * as an error *result* the model can read (slack-mcpl's convention), never as - * a thrown protocol error: the call was permitted, it just did not happen. + * A refusal, a malformed call or a GitHub error comes back as an error result + * the model can read, not as a JSON-RPC error. */ export async function runTool( tool: string, diff --git a/src/tools-comments.ts b/src/tools-comments.ts index de9ee53..7813556 100644 --- a/src/tools-comments.ts +++ b/src/tools-comments.ts @@ -35,7 +35,7 @@ import { type StateRollbackResult, } from '@animalabs/mcpl-core'; import { McplRpcError } from './errors.js'; -import { THREAD_PROPERTY, parseThread, type Parsed } from './tool-support.js'; +import { THREAD_PROPERTY, field, parseThread, uncertainWrite, type Parsed } from './tool-support.js'; import type { GithubStateStore, ThreadLocator } from './state.js'; import { COMMENTS_FEATURE_SET, @@ -357,24 +357,36 @@ export const COMMENT_TOOLS: ToolDefinition[] = [ export const commentsToolModule: ToolModule = { featureSet: COMMENTS_FEATURE_SET, tools: COMMENT_TOOLS, - call(name, args, ctx) { - switch (name) { - case TOOL_COMMENT: - return comment(args, ctx); - case TOOL_REPLY_IN_THREAD: - return replyInThread(args, ctx); - case TOOL_EDIT_OWN_COMMENT: - return editOwnComment(args, ctx); - case TOOL_DELETE_OWN_COMMENT: - return deleteOwnComment(args, ctx); - case TOOL_UPSERT_STICKY_COMMENT: - return upsertStickyComment(args, ctx); - default: - return Promise.resolve(refuse(`unknown tool: ${name}`)); + async call(name, args, ctx) { + try { + return await callComment(name, args, ctx); + } catch (err) { + const uncertain = uncertainWrite(err); + if (!uncertain) throw err; + const text = `${field(name)}: ${uncertain} If it did, rollback has no record of it.`; + ctx.logger.warn('tool call failed', { tool: field(name), detail: text }); + return refuse(text); } }, }; +function callComment(name: string, args: Record, ctx: ToolContext): Promise { + switch (name) { + case TOOL_COMMENT: + return comment(args, ctx); + case TOOL_REPLY_IN_THREAD: + return replyInThread(args, ctx); + case TOOL_EDIT_OWN_COMMENT: + return editOwnComment(args, ctx); + case TOOL_DELETE_OWN_COMMENT: + return deleteOwnComment(args, ctx); + case TOOL_UPSERT_STICKY_COMMENT: + return upsertStickyComment(args, ctx); + default: + return Promise.resolve(refuse(`unknown tool: ${name}`)); + } +} + async function comment(args: Record, ctx: ToolContext): Promise { const thread = parseThreadArg(args.thread, ctx); if (!thread.ok) return refuse(thread.error); diff --git a/test/lost-answer.test.ts b/test/lost-answer.test.ts new file mode 100644 index 0000000..f229511 --- /dev/null +++ b/test/lost-answer.test.ts @@ -0,0 +1,212 @@ +import { strict as assert } from 'node:assert'; +import { generateKeyPairSync } from 'node:crypto'; +import { mkdtempSync, writeFileSync } from 'node:fs'; +import { createServer, type Server } from 'node:http'; +import type { AddressInfo } from 'node:net'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { after, test } from 'node:test'; +import { GitHubAdapter, GitHubApiError } from '../src/github-adapter.js'; +import { GithubState } from '../src/state.js'; +import { commentsToolModule } from '../src/tools-comments.js'; +import { issuesToolModule } from '../src/tools-issues-pr.js'; +import { readToolModule } from '../src/tools-read.js'; +import { readConfig, silentLogger, type RepoRef, type ThreadRef, type ToolContext, type ToolResult } from '../src/types.js'; + +const REPO: RepoRef = { owner: 'anima-research', name: 'rayforce' }; +const PULL: ThreadRef = { repo: REPO, number: 12, kind: 'pull' }; +const R = '/repos/anima-research/rayforce'; +const INSTALLATION_ID = 42; + +const KEY_FILE = join(mkdtempSync(join(tmpdir(), 'github-mcpl-key-')), 'app.pem'); +writeFileSync( + KEY_FILE, + generateKeyPairSync('rsa', { + modulusLength: 2048, + publicKeyEncoding: { type: 'spki', format: 'pem' }, + privateKeyEncoding: { type: 'pkcs8', format: 'pem' }, + }).privateKey, + { mode: 0o600 }, +); + +type Answer = { status: number; body?: unknown } | 'lost'; + +// 'lost' keeps the whole request and closes the socket without answering, as +// a connection that drops after GitHub stored a write does. +class FakeGitHub { + readonly received: { method: string; path: string; body: string }[] = []; + private readonly answers = new Map([ + [`POST /app/installations/${INSTALLATION_ID}/access_tokens`, { status: 201, body: { token: 'ghs_test', expires_at: new Date(Date.now() + 3_600_000).toISOString() } }], + [`GET ${R}/installation`, { status: 200, body: { id: INSTALLATION_ID } }], + ]); + private server: Server | null = null; + baseUrl = ''; + + on(route: string, answer: Answer): this { + this.answers.set(route, answer); + return this; + } + + async start(): Promise { + this.server = createServer((req, res) => { + let body = ''; + req.on('data', (chunk: Buffer) => (body += chunk.toString('utf8'))); + req.on('end', () => { + const path = new URL(req.url ?? '/', 'http://x').pathname; + this.received.push({ method: req.method ?? '', path, body }); + const answer = this.answers.get(`${req.method} ${path}`) ?? { status: 404, body: { message: 'Not Found' } }; + if (answer === 'lost') { + req.socket.destroy(); + return; + } + res.writeHead(answer.status, { 'content-type': 'application/json' }); + res.end(answer.body === undefined ? '' : JSON.stringify(answer.body)); + }); + }); + await new Promise((resolve) => this.server!.listen(0, '127.0.0.1', resolve)); + this.baseUrl = `http://127.0.0.1:${(this.server.address() as AddressInfo).port}`; + } + + async stop(): Promise { + await new Promise((resolve) => this.server?.close(() => resolve()) ?? resolve()); + } +} + +const fakes: FakeGitHub[] = []; +after(async () => { + await Promise.all(fakes.map((f) => f.stop())); +}); + +async function rig(routes: Record): Promise<{ fake: FakeGitHub; adapter: GitHubAdapter; ctx: ToolContext }> { + const fake = new FakeGitHub(); + for (const [route, answer] of Object.entries(routes)) fake.on(route, answer); + await fake.start(); + fakes.push(fake); + const config = readConfig({ + GITHUB_APP_ID: '123456', + GITHUB_APP_PRIVATE_KEY_FILE: KEY_FILE, + GITHUB_APP_INSTALLATION_ID: String(INSTALLATION_ID), + GITHUB_REPOS: 'anima-research/rayforce', + GITHUB_BOT_LOGIN: 'rayforce-clone[bot]', + GITHUB_PR_BASE_ALLOW: 'dev', + }); + const logger = silentLogger(); + const adapter = new GitHubAdapter(config, { baseUrl: fake.baseUrl, logger, sleep: async () => {} }); + const ctx: ToolContext = { + api: adapter, + state: GithubState.empty(null, logger), + botLogin: 'rayforce-clone[bot]', + repos: [REPO], + prBaseAllow: ['dev'], + logger, + }; + return { fake, adapter, ctx }; +} + +function text(result: ToolResult): string { + return result.content.map((c) => ('text' in c ? c.text : '')).join('\n'); +} + +test('a comment GitHub stored whose answer was lost reads as unknown, not as failed', { timeout: 10_000 }, async () => { + const { fake, ctx } = await rig({ [`POST ${R}/issues/7/comments`]: 'lost' }); + + const result = await commentsToolModule.call('comment', { thread: 7, body: 'hello' }, ctx); + + assert.ok(fake.received.some((r) => r.method === 'POST' && r.path === `${R}/issues/7/comments` && r.body.includes('hello')), 'GitHub got the comment'); + assert.equal(result.isError, true); + const said = text(result); + assert.match(said, /whether it took effect is unknown/); + assert.match(said, /rollback has no record of it/); + assert.doesNotMatch(said, /failed/); +}); + +test("GitHub's own 5xx on a write reads as unknown too", { timeout: 10_000 }, async () => { + const { ctx } = await rig({ [`POST ${R}/issues/7/comments`]: { status: 502, body: { message: 'Server Error' } } }); + + const result = await commentsToolModule.call('comment', { thread: 7, body: 'hello' }, ctx); + + assert.equal(result.isError, true); + assert.match(text(result), /GitHub answered createComment with 502 \(Server Error\), so whether it took effect is unknown/); +}); + +test('an issue GitHub created whose answer was lost reads as unknown, not as failed', { timeout: 10_000 }, async () => { + const { fake, ctx } = await rig({ [`POST ${R}/issues`]: 'lost' }); + + const result = await issuesToolModule.call('create_issue', { title: 'A bug' }, ctx); + + assert.ok(fake.received.some((r) => r.method === 'POST' && r.path === `${R}/issues`), 'GitHub got the issue'); + assert.equal(result.isError, true); + assert.match(text(result), /^create_issue: no answer came back from GitHub for createIssue \(.+\), so whether it took effect is unknown/); + assert.doesNotMatch(text(result), /failed|rollback/); +}); + +test('a read whose answer was lost still reads as failed', { timeout: 10_000 }, async () => { + const { ctx } = await rig({ [`GET ${R}/issues/7`]: 'lost' }); + + const result = await readToolModule.call('get_issue', { number: 7 }, ctx); + + assert.equal(result.isError, true); + assert.match(text(result), /^get_issue failed: /); + assert.doesNotMatch(text(result), /unknown/); +}); + +test("a write tool's own lookup whose answer was lost is a failed read, and nothing is written", { timeout: 10_000 }, async () => { + const { fake, ctx } = await rig({ [`GET ${R}/issues/7/comments`]: 'lost' }); + + const failure = await commentsToolModule.call('upsert_sticky_comment', { thread: 7, marker: 'ci-status', body: 'green' }, ctx).then( + () => assert.fail('upsert_sticky_comment should have thrown'), + (err: unknown) => err, + ); + + assert.ok(failure instanceof GitHubApiError); + assert.equal(failure.operation, 'listComments'); + assert.notEqual(failure.possiblyDone, true); + assert.ok(!fake.received.some((r) => r.method !== 'GET' && r.path.startsWith(R)), 'nothing was written'); +}); + +test("a token mint whose answer was lost is a failure of the tool's request, which never went out", { timeout: 10_000 }, async () => { + const { fake, ctx } = await rig({ [`POST /app/installations/${INSTALLATION_ID}/access_tokens`]: 'lost' }); + + const read = await readToolModule.call('get_issue', { number: 7 }, ctx); + assert.equal(read.isError, true); + assert.match(text(read), /^get_issue failed: /); + assert.doesNotMatch(text(read), /unknown/); + + const write = await commentsToolModule.call('comment', { thread: 7, body: 'hello' }, ctx).then( + () => assert.fail('comment should have thrown'), + (err: unknown) => err, + ); + assert.ok(write instanceof GitHubApiError); + assert.equal(write.operation, 'mintInstallationToken'); + assert.notEqual(write.possiblyDone, true); + assert.ok(!fake.received.some((r) => r.path.startsWith(R)), 'no request for the repository went out'); +}); + +test('a write GitHub refused with a 4xx still reads as failed', { timeout: 10_000 }, async () => { + const { ctx } = await rig({ [`POST ${R}/issues`]: { status: 422, body: { message: 'Validation Failed' } } }); + + const result = await issuesToolModule.call('create_issue', { title: 'A bug' }, ctx); + + assert.equal(result.isError, true); + assert.match(text(result), /^create_issue failed: GitHub error 422: /); +}); + +test('a GraphQL mutation whose answer was lost may have taken effect; a GraphQL query may not', { timeout: 10_000 }, async () => { + const { adapter } = await rig({ 'POST /graphql': 'lost' }); + + const mutation = await adapter.resolveReviewThread('PRRT_x').then( + () => assert.fail('resolveReviewThread should have thrown'), + (err: unknown) => err, + ); + assert.ok(mutation instanceof GitHubApiError); + assert.equal(mutation.possiblyDone, true); + assert.equal(mutation.answered, false); + + const query = await adapter.listReviewThreads(PULL).then( + () => assert.fail('listReviewThreads should have thrown'), + (err: unknown) => err, + ); + assert.ok(query instanceof GitHubApiError); + assert.equal(query.possiblyDone, false); +}); +