[nissa-seru]

When a write tool's request to GitHub gets no answer, or a 5xx, the tool now tells the agent that whether the write took effect is unknown, and to look on GitHub before trying again. On main, create_issue answered that it failed, and comment threw an error, though GitHub may have stored the write. A read, and a write GitHub refused with a 4xx, are answered as on main. This changes text an agent reads in a tool result.

What the agent reads, from uncertainWrite in src/tool-support.ts:

• No answer: <tool>: no answer came back from GitHub for <operation> (<cause>), so whether it took effect is unknown. Look on GitHub before trying again: a retry may do it twice.
• A 5xx: <tool>: GitHub answered <operation> with <status> (<GitHub's message>), so whether it took effect is unknown. Look on GitHub before trying again: a retry may do it twice.
• The comment tools (comment, reply_in_thread, edit_own_comment, delete_own_comment and upsert_sticky_comment) add If it did, rollback has no record of it., and give this as an error result. On main, comment threw such a failure, as the run below shows.

<operation> is the adapter's name for the request, such as createComment or createIssue, as on main.

What the tools gave on main, in a run with the tests' fake GitHub server, on Linux with Node 24.22.1:

• create_issue, answer lost: an error result, create_issue failed: GitHub error 500: GitHub 500 on createIssue: other side closed.
• comment, answer lost: it threw GitHub 500 on createComment: other side closed. The server's dispatcher answers a thrown error that isn't an McplRpcError with sendError(req.id, ERR_INTERNAL, e.message) (src/server.ts:892), a JSON-RPC error, -32603, carrying that message.
• comment, a 502: it threw GitHub 502 on createComment: Server Error.

In a run of the same probe at this branch's head, 72616ec, all three are error results with the texts above.

Problem

On main (c243333), when a write's answer is lost, create_issue answers with an error and comment throws one, as the run above shows, though GitHub may have stored the write. In two of the new tests the fake GitHub server receives the write and then closes the connection. The 500 in those errors comes from octokit, not GitHub: the lockfile holds @octokit/request 9.2.4, whose fetch-wrapper.js:48 builds new RequestError(message, 500, { request: requestOptions }) when a fetch throws. In the new tests on main's code, the four that lose a write's answer, or answer it with a 502, fail.

Changes

• src/github-adapter.ts: a failure now carries whether its request only read, and whether GitHub answered at all.
  • A request reads when its method is GET or HEAD, or when it's a GraphQL document that begins with query or {, after whitespace and # comments. Anything else counts as a possible write, including a request that can't be judged.
  • A possible write's failure with status 500 or more is marked as possibly done. The request that failed decides the mark, not the tool, so a write tool's own lookup that fails is a failed read.
  • A failure to mint the installation token or to find the installation comes before the tool's request is sent, and stays a plain failure.
• src/tool-support.ts: describeFailure gives the texts above for a failure marked as possibly done.
• src/tools-comments.ts: the comment tools answer such a failure with an error result, and log it with ctx.logger.warn('tool call failed', …). Their other failures are answered as before. They book a write for rollback only once GitHub answers, which is why their text adds the rollback sentence.
• src/server.ts: two comments.
• Changelog: changelog.d/lost-answer.fixed.md.

channels/publish is unchanged. handlePublish awaits api.createComment with no catch around it (src/channels.ts:644), and the dispatcher's catch answers a thrown error that isn't an McplRpcError with sendError(req.id, ERR_INTERNAL, e.message) (src/server.ts:892). agent-framework's publishErrorOutcome (src/mcpl/channel-registry.ts:891–902 at 59d6095) returns an unknown outcome, with "(delivery uncertain)", for an error answer to a publish, and failed only for a request that never left the host.

Tests

test/lost-answer.test.ts is new, with 8 tests. They run against a fake GitHub server that can close the connection after it receives a request. Seven call a tool, and the GraphQL test calls the adapter's methods:

• a comment whose answer is lost, which GitHub received, reads as unknown and names rollback, and doesn't say "failed";
• a comment answered with a 502 reads as unknown, with the 502 and GitHub's message;
• an issue whose answer is lost, which GitHub received, reads as unknown, and says neither "failed" nor "rollback";
• a GraphQL mutation whose answer is lost is marked as possibly done, and a GraphQL query isn't;
• a read whose answer is lost still reads as failed;
• upsert_sticky_comment's lookup whose answer is lost is a failed read, and nothing is written;
• when a token mint's answer is lost, get_issue reads as failed, and comment throws the mint's failure, not marked as possibly done, with no request for the repository sent;
• a write refused with a 422 still reads as failed.

On Linux with Node 24.22.1, in clean checkouts after npm ci:

• This branch's head, 72616ec: typecheck succeeds, and the 131 tests in test/lost-answer.test.ts and the four tool test files (toolsComments, toolsIssuesPr, toolsRead and toolsReview) pass.
• Its second commit, fe29c40: typecheck and build succeed. npm test runs 614 tests: 594 pass, 18 are skipped, and 2 fail. The 2 are the README tests in test/wp8-docs.test.ts, which need jq and a git checkout the machine lacked. main gives 606, 586, 18 and the same 2.
• Red-first: with the new tests on main's code (d520453), the first four above fail, and the other four pass.

In CI at 72616ec, on ubuntu-latest with Node 22.23.3, the test step runs 614 tests: 596 pass, none fail, and 18 are skipped.

What this leaves

• A write tool that makes several requests, such as close_issue, which comments before it closes (src/tools-issues-pr.ts:178–179). None of this change's tests fails a later request after an earlier write in the same call.
• A refused connection. In a run at 72616ec on Linux with Node 24.22.1, create_issue against a closed port gave create_issue: no answer came back from GitHub for createIssue (connect ECONNREFUSED 127.0.0.1:34741), so whether it took effect is unknown. …. The text calls the outcome unknown, though the connection was refused.
• The open Merge, update-branch, workflow runs and review reads, behind opt-in allow-lists #11, Comments: keep only those one rule allows, outside the files #11 changes #15 and Use @animalabs/mcpl-core 0.4.0, which needs Node 22.12.0 or later #16, at 9127a61, 5b73310 and 6d72345, each merge with this cleanly. Merge, update-branch, workflow runs and review reads, behind opt-in allow-lists #11 also changes src/github-adapter.ts and src/server.ts.

---

Ogden-2688 made the change, and Esther-2584 read it and ran it. Daniel-2612 wrote this description.

[github-actions]

Automated review

Recommendation: merge and fast-follow. 3 medium severity findings can be fixed after the merge.

Reviewed commit fe29c40 with claude-opus-5-5, by reading the code. It did not run the code or look at the checks.

Does this change belong in the project?

It fits the project. This is a bug fix to how the existing write tools report failures. When GitHub's answer is lost or comes back as a 5xx, the tools now say the outcome is unknown instead of claiming the write failed, and the work stays in the adapter and tool-support layers that already own this.

5 reasons with sources

• It fixes behavior the project already has: wording of tool error results, and comment tools throwing where they should return an error result. The comment on runTool before the change said every GitHub error comes back as an error result the model can read, never as a thrown protocol error. On main, the comment tools threw a lost write as a JSON-RPC -32603 error. This change brings those tools back in line with that convention. (src/tool-support.ts (runTool doc comment, before the change))
• It supports the README's promise that comment rollback tracks the bot's writes, by telling the agent when a write may have landed without being booked. The README says github.comments is checkpointed and that rollback deletes the comments created after a checkpoint. A comment that GitHub stored but whose answer was lost would be missed by rollback. The new text says so, and adds no new rollback mechanism. (README.md (Feature sets with rollback))
• The read/write classification sits in the adapter's existing failure normalization, and the text the agent reads is built in the existing describeFailure. GitHubApiError gains two fields with defaults. No new module, dependency or public tool is added. (src/github-adapter.ts (normalizeFailure, GitHubApiError))
• Minor layering point: tool-support.ts now imports the concrete adapter module, which before this change only index.ts did. Before the change, tool-support read the failure's status by duck typing (statusOf), and only src/index.ts imported github-adapter.js. The separate review already lists this as a low-severity finding; it doesn't change whether the change belongs. (src/tool-support.ts, src/index.ts)
• The change is one coherent piece of work, and its changelog fragment follows the documented naming and format. The fragment lost-answer.fixed.md is a flat file made of bullets, with continuation lines indented two spaces, as the guide asks. The server.ts edits only touch comments. (changelog.d/README.md)

Findings

3 findings of medium or high severity, see the inline comments:

• Medium severity. Uncertain comment writes are no longer logged anywhere.
• Medium severity, test coverage. Only comment is tested for the new comment-tool error result.
• Medium severity, test coverage. GraphQL read/write classification is tested only on two simple documents.

4 more findings, less severe

• Low severity, maintainability, src/tool-support.ts:31. Tool layer now depends on the concrete adapter class. Before this change, the only module that imported github-adapter.ts was src/index.ts. The tool modules talked to GitHub only through the GitHubApi interface in types.ts, and statusOf reads an error's status by duck typing for the same reason. uncertainWrite uses instanceof GitHubApiError, so it ties tool-support.ts, and through it every tool module, to octokit's adapter module. It also means any other GitHubApi implementation, such as a test fake, can never produce an 'unknown' outcome. A duck-typed check on possiblyDone/answered, or moving the error type into types.ts, would keep the layering.
• Low severity, test coverage, src/github-adapter.ts:847-850. Installation-lookup failure path is not tested. The rebuilt auth failure is meant to cover both a failed token mint and a failed installation discovery (resolveInstallationId). The new test only loses the mint's answer. When the installation lookup fails inside the transport, the tool's write request was never sent, and no test checks that this case also comes out without possiblyDone. Add a case with GITHUB_APP_INSTALLATION_ID unset and the /installation lookup lost.
• Low severity, test coverage, test/lost-answer.test.ts:141-149. No test that a 5xx answered on a read stays 'failed'. The read test only covers a lost answer (answered false). The failure.status >= 500 && !failure.read branch is never tested with a GET that GitHub answers with a 502/503. That is the common real-world 5xx, and it must not be reported as 'unknown'. A similar case for create_issue answered with a 5xx, whose text should say 'GitHub answered createIssue with 5xx', is also missing.
• Low severity, maintainability, test/lost-answer.test.ts:21-30. Test leaves a temporary private-key directory behind. mkdtempSync creates a directory under the OS temp dir and writes an RSA private key into it when the module loads. The after hook stops the fake servers but never removes this directory. Every test run leaves one more key file in /tmp. Remove it in the after hook with rmSync(..., { recursive: true }).

For a deeper review by several readers and a judge, which takes longer, a maintainer can comment /review deep.

[github-actions]

Medium severity. Uncertain comment writes are no longer logged anywhere.

Before this change, a GitHub error thrown by a comment tool reached the dispatcher in src/server.ts (lines 886-892), which logs request failed at error level with the tool name. Now commentsToolModule.call catches the possibly-done error and returns refuse(...) without logging. Every other tool module goes through runTool (tool-support.ts line 179), which logs tool call failed at warn level. So the one case where a write may have landed without being booked for rollback leaves no trace in the operator's logs. Log the uncertain write here, the way runTool does.

[github-actions]

Medium severity, test coverage. Only comment is tested for the new comment-tool error result.

The change wraps all five comment tools in the uncertain-write catch, but the tests only exercise comment (a POST). Nothing covers edit_own_comment (PATCH), delete_own_comment (DELETE), reply_in_thread or the write half of upsert_sticky_comment with a lost answer or a 5xx. So the non-POST paths through isReadRequest, where a PATCH or DELETE should be marked possibly done, are untested. Add at least one PATCH case and one DELETE case.

[github-actions]

Medium severity, test coverage. GraphQL read/write classification is tested only on two simple documents.

isReadRequest parses the body, handles a string or object body, skips leading whitespace and # comments, and treats { as a shorthand query. The only test (resolveReviewThread against listReviewThreads) uses documents that start with mutation and query with no leading whitespace or comments. Nothing tests a leading comment, the anonymous { form, a body that can't be parsed, or a non-/graphql POST. Since a wrong classification silently tells the agent a read 'may have taken effect', or tells it a mutation simply 'failed', the regex needs unit tests.