From 05cd9db82eb07ab1716368e3c69aa371104dc321 Mon Sep 17 00:00:00 2001 From: Ogden-2688 Date: Tue, 27 Oct 2026 15:21:16 -0400 Subject: [PATCH 1/3] Test that bad input from the host ends only what it should Three tests drive a server over a stream pair written as raw lines: - a request whose JSON string holds U+2028 or U+2029 gets its answer; - a malformed line is logged, and the next request is answered; - a stream error ends serve() and is logged. On main, with @animalabs/mcpl-core 0.3.0 and no 'error' listener on the connection, each of the three ends the test process with an unheard 'error'. --- test/connection-errors.test.ts | 105 +++++++++++++++++++++++++++++++++ 1 file changed, 105 insertions(+) create mode 100644 test/connection-errors.test.ts diff --git a/test/connection-errors.test.ts b/test/connection-errors.test.ts new file mode 100644 index 0000000..7127b3e --- /dev/null +++ b/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) => { + 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(); + 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 { + 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 { + 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; error?: unknown }; + assert.ok(answer.result ?? answer.error, `request ${id} gets a JSON-RPC answer`); + } +}); + +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)}`, + ); +}); From 50afa965f8dc2ef3eedba7fe1d1b4ffd3939d01f Mon Sep 17 00:00:00 2001 From: Ogden-2688 Date: Tue, 27 Oct 2026 15:21:16 -0400 Subject: [PATCH 2/3] Use @animalabs/mcpl-core 0.4.0, and hear the errors it reports 0.4.0 reads a line only at \n, so U+2028 and U+2029 inside a JSON string no longer split a frame on Node 24. It also closes a connection once when one of its streams fails, and reports that as 'error'. Its acceptTcp no longer leaves listeners on the server, so the server's own 'error' between accepts is the caller's to hear. - package.json: @animalabs/mcpl-core ^0.4.0, and engines.node >=22.12.0, 0.4.0's own floor. The lockfile takes 0.4.0. - serve() listens for its connection's 'error' and logs it. A malformed line leaves the connection open; a stream error closes it, so serve() returns: over TCP the next host can connect, and under --stdio the process shuts down as it does when stdin ends. - The TCP server gets an 'error' listener once it is listening, which logs. A failed accept while one is pending still rejects that accept. - scripts/e2e/fake-host.mjs recognises a closed connection by ConnectionClosedError's name: two of 0.4.0's three texts for it no longer say "connection closed". The comment above it names the class. - README.md and deploy/github-mcpl.env.example give the Node floor as 22.12.0. - Two changelog fragments: the Node floor (breaking) and the fix. --- README.md | 6 +++--- changelog.d/mcpl-core-0.4.breaking.md | 2 ++ changelog.d/mcpl-core-0.4.fixed.md | 9 +++++++++ deploy/github-mcpl.env.example | 2 +- package-lock.json | 12 ++++++------ package.json | 4 ++-- scripts/e2e/fake-host.mjs | 4 ++-- src/index.ts | 1 + src/server.ts | 1 + 9 files changed, 27 insertions(+), 14 deletions(-) create mode 100644 changelog.d/mcpl-core-0.4.breaking.md create mode 100644 changelog.d/mcpl-core-0.4.fixed.md diff --git a/README.md b/README.md index dfa2ec7..8c00ad9 100644 --- a/README.md +++ b/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 ~/github-mcpl cd ~/github-mcpl ``` @@ -461,7 +461,7 @@ npm test npm run build # tsc into dist/ ``` -Node 22, ESM, TypeScript. Stdout carries only JSON-RPC: every diagnostic goes +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. diff --git a/changelog.d/mcpl-core-0.4.breaking.md b/changelog.d/mcpl-core-0.4.breaking.md new file mode 100644 index 0000000..d43e3ce --- /dev/null +++ b/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. diff --git a/changelog.d/mcpl-core-0.4.fixed.md b/changelog.d/mcpl-core-0.4.fixed.md new file mode 100644 index 0000000..38cd1fe --- /dev/null +++ b/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. diff --git a/deploy/github-mcpl.env.example b/deploy/github-mcpl.env.example index 3fb3ae8..ed37b8c 100644 --- a/deploy/github-mcpl.env.example +++ b/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= diff --git a/package-lock.json b/package-lock.json index 73bcf38..c89f8ef 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8,7 +8,7 @@ "name": "@connectome/github-mcpl", "version": "0.1.0", "dependencies": { - "@animalabs/mcpl-core": "^0.3.0", + "@animalabs/mcpl-core": "^0.4.0", "octokit": "^4.1.0" }, "bin": { @@ -20,16 +20,16 @@ "typescript": "^5.5.0" }, "engines": { - "node": ">=22.0.0" + "node": ">=22.12.0" } }, "node_modules/@animalabs/mcpl-core": { - "version": "0.3.0", - "resolved": "https://registry.npmjs.org/@animalabs/mcpl-core/-/mcpl-core-0.3.0.tgz", - "integrity": "sha512-9BEmfvVeutdQCfIv5sJNuMt/WGiwsBSJ5i5DVz2dxqtH1beuPKX/2CNk5n2OtZixUeUNDPakmLT86jLi8O42Yg==", + "version": "0.4.0", + "resolved": "https://registry.npmjs.org/@animalabs/mcpl-core/-/mcpl-core-0.4.0.tgz", + "integrity": "sha512-+XMPYcQb8ausCcSEnj/8nJw6tZuOTprBbtUFCna+/hOVJ7GUg/uMinBhuqmTpKqVgOe38VxbMP6/Zucvgxxu4A==", "license": "MIT", "engines": { - "node": ">=20.0.0" + "node": ">=22.12.0" } }, "node_modules/@esbuild/aix-ppc64": { diff --git a/package.json b/package.json index 16b75b1..4cf0cbf 100644 --- a/package.json +++ b/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" } } diff --git a/scripts/e2e/fake-host.mjs b/scripts/e2e/fake-host.mjs index 964030c..423fc48 100644 --- a/scripts/e2e/fake-host.mjs +++ b/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. */ diff --git a/src/index.ts b/src/index.ts index 87a4673..4ffbdad 100644 --- a/src/index.ts +++ b/src/index.ts @@ -200,6 +200,7 @@ async function main(): Promise { const tcpServer = net.createServer(); tcpServer.listen(tcpPort, '127.0.0.1'); await new Promise((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 diff --git a/src/server.ts b/src/server.ts index 9a6f4f5..5daccc0 100644 --- a/src/server.ts +++ b/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 { + conn.on('error', (err) => this.logger.error('connection error', { error: String(err) })); this.conn = conn; this.grant.reset(); this.channels = new ChannelRegistry({ From 6d7234543678737e395c87f0b7ff0a599c0d634c Mon Sep 17 00:00:00 2001 From: Ogden-2688 Date: Tue, 27 Oct 2026 16:14:07 -0400 Subject: [PATCH 3/3] Test on Node 24 in CI too, and check the U+2028 answer is a result On Node 22, readline doesn't end a line at U+2028 or U+2029, so the test for those frames passes there with @animalabs/mcpl-core 0.3.0 too. CI now runs on Node 22 and 24, so it can fail where the split happens. The test now asserts each of the two requests is answered with a result, as the malformed-line test does, rather than with any answer. --- .github/workflows/ci.yml | 7 +++++-- test/connection-errors.test.ts | 4 ++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ba09526..11815b3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -11,8 +11,11 @@ permissions: jobs: typecheck-and-test: - name: Typecheck & Test (node 22) + name: Typecheck & Test (node ${{ matrix.node }}) runs-on: ubuntu-latest + strategy: + matrix: + node: [22, 24] steps: - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6.1.0 @@ -22,7 +25,7 @@ jobs: - name: Setup Node.js uses: actions/setup-node@249970729cb0ef3589644e2896645e5dc5ba9c38 # v6.5.0 with: - node-version: 22 + node-version: ${{ matrix.node }} # npm ci installs the lock verbatim: npm >= 11 'npm install' silently # re-resolves platform packages missing from the lock, so only npm ci diff --git a/test/connection-errors.test.ts b/test/connection-errors.test.ts index 7127b3e..873fb21 100644 --- a/test/connection-errors.test.ts +++ b/test/connection-errors.test.ts @@ -70,8 +70,8 @@ test('a request whose string holds U+2028 or U+2029 is answered', { timeout: 500 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; error?: unknown }; - assert.ok(answer.result ?? answer.error, `request ${id} gets a JSON-RPC answer`); + const answer = (await h.answerTo(id)) as { result?: unknown }; + assert.ok(answer.result, `request ${id} is answered with a result`); } });