diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8623127..125d396 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/README.md b/README.md index aa9670d..446f871 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,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 | | --- | --- | --- | 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 2c6584b..5bddb4c 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 a34d9cb..e748e8d 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({ diff --git a/test/connection-errors.test.ts b/test/connection-errors.test.ts new file mode 100644 index 0000000..873fb21 --- /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 }; + 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)}`, + ); +});