Report a write tool's outcome as unknown when GitHub's answer is lost or is a 5xx

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.

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<string, string>;
+  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,19 +1726,23 @@ 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.
  */
 function normalizeFailure(raw: unknown): Failure {
   const err = raw as {
     status?: number;
     message?: string;
+    request?: { method?: string; url?: string; body?: unknown };
     response?: { headers?: Record<string, unknown>; 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<string, string> = {};
   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);
 }

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

src/tool-support.ts

@@ -142,17 +142,36 @@ 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
     ? `${tool} failed: ${bounded(message, LIMITS.commentChars)}`
     : `${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,

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<string, unknown>, ctx: ToolContext): Promise<ToolResult> {
+  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<string, unknown>, ctx: ToolContext): Promise<ToolResult> {
   const thread = parseThreadArg(args.thread, ctx);
   if (!thread.ok) return refuse(thread.error);