Use @animalabs/mcpl-core 0.4.0, which needs Node 22.12.0 or later

.github/workflows/ci.yml

@@ -10,11 +10,9 @@ permissions:
   contents: read
 
 jobs:
-  # The whole suite runs once, at Node 22.12.0. Node 22.8.0 takes npm test's
-  # --experimental-test-isolation=none, but there every test file fails to
-  # load with 'Unknown file extension ".ts"'. The install and start check
-  # below runs on engines' floor, 22.0.0; on Node 24, a maintenance LTS line
-  # hosts still run; and on the newest Node release.
+  # The whole suite runs once, at Node 22.12.0, engines' floor. The install
+  # and start check below runs on Node 24, a maintenance LTS line hosts still
+  # run, and on the newest Node release.
   typecheck-and-test:
     name: Typecheck & Test (node 22.12.0)
     runs-on: ubuntu-latest
@@ -49,7 +47,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        node: ['22.0.0', '24', 'latest']
+        node: ['24', 'latest']
 
     steps:
       - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6.1.0

README.md

@@ -47,10 +47,10 @@ does and does not protect.
 
 Everything after this runs commands out of this repository — step 3 uses a
 script from it before GitHub has even created the App — so the checkout comes
-first. Node 22 or newer (nvm is fine), and one clone:
+first. Node 22.12.0 or newer (nvm is fine), and one clone:
 
 ```sh
-node --version                                      # v22 or newer
+node --version                                      # v22.12.0 or newer
 git clone <this repository> ~/github-mcpl
 cd ~/github-mcpl
 ```
@@ -461,9 +461,9 @@ npm test
 npm run build     # tsc into dist/
 ```
 
-Node 22 (`npm test` runs on 22.12 or newer), ESM, TypeScript. Stdout
-carries only JSON-RPC: every diagnostic goes through the `Logger`, which
-writes to stderr. No test in this package reaches the network.
+Node 22.12 or newer, ESM, TypeScript. Stdout carries only JSON-RPC: every
+diagnostic goes through the `Logger`, which writes to stderr. No test in this
+package reaches the network.
 
 | File | Work package | Role |
 | --- | --- | --- |

changelog.d/mcpl-core-0.4.breaking.md

@@ -0,0 +1,2 @@
+- **Operators:** github-mcpl needs Node 22.12.0 or later, the floor of
+  `@animalabs/mcpl-core` 0.4.0, which it now uses.

changelog.d/mcpl-core-0.4.fixed.md

@@ -0,0 +1,9 @@
+- A host message whose JSON string holds U+2028 or U+2029 no longer ends the
+  process: on Node 24 the reader split the line there.
+- A malformed line from the host is logged, and its connection stays open. A
+  stream error, such as a reset TCP connection, is logged and closes that
+  connection: over TCP the next host can then connect, and under `--stdio` the
+  process shuts down as it does when the host closes stdin. Before, each of
+  these ended the process with an unheard error.
+- An error the TCP server reports while a host is connected, such as a failed
+  accept of another connection, is logged.

deploy/github-mcpl.env.example

@@ -10,7 +10,7 @@
 # in src/types.ts is what validates them.
 
 # --- runtime ----------------------------------------------------------------
-# The absolute path of Node 22+. README step 7 appends it with
+# The absolute path of Node 22.12.0 or newer. README step 7 appends it with
 # `command -v node`, because a host started by systemd does not see nvm's PATH.
 #GITHUB_MCPL_NODE=
 

package.json

@@ -16,7 +16,7 @@
     "version": "node scripts/release-changelog.mjs && git add CHANGELOG.md changelog.d"
   },
   "dependencies": {
-    "@animalabs/mcpl-core": "^0.3.0",
+    "@animalabs/mcpl-core": "^0.4.0",
     "octokit": "^4.1.0"
   },
   "devDependencies": {
@@ -25,6 +25,6 @@
     "@types/node": "^22.0.0"
   },
   "engines": {
-    "node": ">=22.0.0"
+    "node": ">=22.12.0"
   }
 }

scripts/e2e/fake-host.mjs

@@ -433,12 +433,12 @@ function isFatal(err) {
  * Did this command fail because the server is no longer there?
  *
  * The stream closing usually beats the `exit` event, so a child that dies
- * under a command surfaces as mcpl-core's `Connection closed` rather than as
+ * under a command surfaces as mcpl-core's `ConnectionClosedError` rather than as
  * the exit this host is watching for. Both mean the same thing, and both have
  * to stop the run: every command after them would report it again.
  */
 function isGone(err) {
-  return err?.fatal === true || /connection closed/i.test(String(err?.message ?? err));
+  return err?.fatal === true || err?.name === 'ConnectionClosedError';
 }
 
 /** Wait briefly for the child's exit, so a failure can name its code. */

src/index.ts

@@ -200,6 +200,7 @@ async function main(): Promise<void> {
       const tcpServer = net.createServer();
       tcpServer.listen(tcpPort, '127.0.0.1');
       await new Promise<void>((resolve) => tcpServer.once('listening', resolve));
+      tcpServer.on('error', (err) => log.error('tcp server error', { error: String(err) }));
       log.info('listening', { tcpPort });
       // One host at a time: the grant, the channel registry and the thread
       // memory are per-connection, and a second concurrent peer would be

src/server.ts

@@ -656,6 +656,7 @@ export class GitHubMcplServer {
    * registry (registration is per-host, §14.5) and the thread memory.
    */
   async serve(conn: McplConnection): Promise<void> {
+    conn.on('error', (err) => this.logger.error('connection error', { error: String(err) }));
     this.conn = conn;
     this.grant.reset();
     this.channels = new ChannelRegistry({

test/connection-errors.test.ts

@@ -0,0 +1,105 @@
+import { strict as assert } from 'node:assert';
+import { PassThrough } from 'node:stream';
+import { test } from 'node:test';
+import { McplConnection } from '@animalabs/mcpl-core';
+import { GitHubMcplServer } from '../src/server.js';
+import type { Logger } from '../src/types.js';
+import { FakeGitHubApi, TEST_BOT, TEST_REPO } from './harness.js';
+
+function capturingLogger(): Logger & { lines: string[] } {
+  const lines: string[] = [];
+  const push = (level: string) => (message: string, fields?: Record<string, unknown>) => {
+    lines.push(`${level} ${message} ${JSON.stringify(fields ?? {})}`);
+  };
+  return { lines, debug: push('debug'), info: push('info'), warn: push('warn'), error: push('error') };
+}
+
+/** A server serving one connection whose host side is written as raw lines, so a frame reaches the server's reader byte for byte. */
+function rawHost() {
+  const toServer = new PassThrough();
+  const toHost = new PassThrough();
+  const logger = capturingLogger();
+  const server = new GitHubMcplServer(new FakeGitHubApi(), { botLogin: TEST_BOT.login, repos: [TEST_REPO], logger });
+  const served = server.serve(McplConnection.fromStreams(toServer, toHost));
+
+  const answers = new Map<number, unknown>();
+  let buffered = '';
+  toHost.on('data', (chunk: Buffer) => {
+    buffered += chunk.toString('utf8');
+    let nl: number;
+    while ((nl = buffered.indexOf('\n')) >= 0) {
+      const line = buffered.slice(0, nl);
+      buffered = buffered.slice(nl + 1);
+      const msg = JSON.parse(line) as { id?: number };
+      if (typeof msg.id === 'number') answers.set(msg.id, msg);
+    }
+  });
+
+  async function answerTo(id: number, ms = 2000): Promise<unknown> {
+    const deadline = Date.now() + ms;
+    while (!answers.has(id)) {
+      if (Date.now() > deadline) throw new Error(`no answer to request ${id} within ${ms} ms`);
+      await new Promise((r) => setTimeout(r, 10));
+    }
+    return answers.get(id);
+  }
+
+  async function initialize(): Promise<void> {
+    toServer.write(
+      JSON.stringify({
+        jsonrpc: '2.0',
+        id: 1,
+        method: 'initialize',
+        params: { protocolVersion: '2024-11-05', capabilities: {}, clientInfo: { name: 'raw-host', version: '0' } },
+      }) + '\n',
+    );
+    await answerTo(1);
+    toServer.write(JSON.stringify({ jsonrpc: '2.0', method: 'notifications/initialized' }) + '\n');
+  }
+
+  return { toServer, logger, served, answerTo, initialize };
+}
+
+test('a request whose string holds U+2028 or U+2029 is answered', { timeout: 5000 }, async (t) => {
+  const h = rawHost();
+  t.after(() => h.toServer.end());
+  await h.initialize();
+
+  // Written by hand: JSON.stringify leaves U+2028 and U+2029 unescaped, as a host's encoder may.
+  h.toServer.write(`{"jsonrpc":"2.0","id":2,"method":"tools/list","params":{"cursor":"a\u2028b"}}\n`);
+  h.toServer.write(`{"jsonrpc":"2.0","id":3,"method":"tools/list","params":{"cursor":"a\u2029b"}}\n`);
+
+  for (const id of [2, 3]) {
+    const answer = (await h.answerTo(id)) as { result?: unknown };
+    assert.ok(answer.result, `request ${id} is answered with a result`);
+  }
+});
+
+test('a malformed line is logged, and the next request is answered', { timeout: 5000 }, async (t) => {
+  const h = rawHost();
+  t.after(() => h.toServer.end());
+  await h.initialize();
+
+  h.toServer.write('not json\n');
+  h.toServer.write(JSON.stringify({ jsonrpc: '2.0', id: 2, method: 'tools/list' }) + '\n');
+
+  const answer = (await h.answerTo(2)) as { result?: unknown };
+  assert.ok(answer.result, 'request 2 is answered after the malformed line');
+  assert.ok(
+    h.logger.lines.some((l) => l.startsWith('error ') && l.includes('Malformed JSON-RPC message')),
+    `the malformed line is logged: ${JSON.stringify(h.logger.lines)}`,
+  );
+});
+
+test('a stream error ends serve() and is logged', { timeout: 5000 }, async () => {
+  const h = rawHost();
+  await h.initialize();
+
+  h.toServer.destroy(new Error('read ECONNRESET (test)'));
+  await h.served;
+
+  assert.ok(
+    h.logger.lines.some((l) => l.startsWith('error ') && l.includes('read ECONNRESET (test)')),
+    `the stream error is logged: ${JSON.stringify(h.logger.lines)}`,
+  );
+});