[nissa-seru]

This moves github-mcpl to @animalabs/mcpl-core 0.4.0, and raises github-mcpl's Node floor to 22.12.0, 0.4.0's own. With 0.4.0, on Node 24, a host request whose string holds U+2028 or U+2029 is answered, where on main at c243333 one holding U+2028 ends the process. github-mcpl now also listens for the errors its connection reports. A malformed line from the host is logged, and the connection stays open. A stream error is logged and ends that connection, and under --stdio the process then shuts down as it does when the host closes stdin.

3dd5289, this head, merges main at 77996a2, which holds #19 and #20, into this branch, to resolve the conflicts #19 made in .github/workflows/ci.yml and README.md. At this head, CI runs the typecheck and the whole suite once, on Node 22.12.0, this branch's floor, and checks that the server installs, builds and starts on Node 24 and on the newest Node release.

Who needs to act

Operators running github-mcpl on a Node earlier than 22.12.0. engines.node goes from >=22.0.0 to >=22.12.0, the engines.node of @animalabs/mcpl-core 0.4.0 (npm view @animalabs/mcpl-core@0.4.0 engines). README.md and deploy/github-mcpl.env.example now give the new floor.

Problem

On main at c243333, github-mcpl uses mcpl-core 0.3.0 and attaches no 'error' listener to the host's connection. On Node 24, 0.3.0's reader ends a line at U+2028, so a frame whose JSON string holds one arrives split, and the connection reports it as a "Malformed JSON-RPC message" 'error'. With no listener, the error is thrown. The same happens for a line that isn't JSON and for an error on the stream.

• The new tests, on c243333's code: all three fail with the connection's error. The U+2028 and U+2029 test fails on its U+2028 request, with an error that begins "Malformed JSON-RPC message: Unterminated string in JSON".
• A bare McplConnection with no 'error' listener, on 0.3.0 and Node 24: the process exits 1 on a request holding U+2028, on a malformed line and on a stream error.
• A build of c243333, with errors injected: with stdin destroyed by an error under --stdio, or a host resetting its TCP socket, the process exits 1. Over TCP, the next host is then refused.

Changes

• package.json and package-lock.json: @animalabs/mcpl-core ^0.3.0 becomes ^0.4.0, locked at 0.4.0. No other package in the lockfile changes version. 0.4.0 ends a line at \n alone (fix(connection): end a stream's line at \n alone, read a WebSocket message whole, and send U+2028/U+2029 escaped mcpl-core-ts#13), so the frame reaches the parser whole.
• src/server.ts: serve() attaches an 'error' listener to the connection that logs the error. In 0.4.0 a malformed line is reported as an 'error' and the connection stays open, and a stream error closes the connection and is then reported (Close McplConnection when either stream fails, and report the error once mcpl-core-ts#11). With the listener, a malformed line is logged and the next request is answered. A stream error is logged, and serve() returns. Over TCP the next host can then connect. Under --stdio the process shuts down as it does when the host closes stdin.
• src/index.ts: once the TCP server is listening, an 'error' listener on it logs the error. 0.4.0's acceptTcp leaves no listener on the server once it settles (fix(connection): acceptTcp leaves no listener on the server once it settles mcpl-core-ts#12), so without this one, a server error between accepts ends the process, as a run of a build shows below.
• .github/workflows/ci.yml: against main, the install and start check loses its Node 22.0.0 leg, which checks main's engines floor, and keeps Node 24 and the newest release. The comment over the jobs now gives 22.12.0 as engines' floor. The whole suite runs at Node 22.12.0, as on main.
• scripts/e2e/fake-host.mjs: isGone recognises a closed connection by err.name === 'ConnectionClosedError' instead of matching "connection closed" in the message.
• Changelog: changelog.d/mcpl-core-0.4.breaking.md for the Node floor, and changelog.d/mcpl-core-0.4.fixed.md for U+2028 and U+2029, a malformed line, a stream error, and a TCP server error while a host is connected.

Tests

test/connection-errors.test.ts is new, with three tests. Each serves one connection whose host side writes raw lines:

• a request whose string holds U+2028, and one holding U+2029, are each answered with a result;
• a malformed line is logged, and the next request is answered;
• a stream error ends serve() and is logged.

On this head's files apart from .github/workflows/ci.yml, on Linux x86_64 with Node 22.12.0, in a clean checkout after npm ci, with mcpl-core 0.4.0 installed, test/connection-errors.test.ts passes 3 of 3. With the installed reader changed to end a line at U+2028 as well as at \n, the U+2028 and U+2029 test fails and the other two pass. So on Node 22.12.0, where CI runs the tests, the test fails for a reader that splits at U+2028. The comment on 0.4.0's reader says "Lines end at '\n' and nowhere else" (src/connection.ts:299 to :300 at mcpl-core-ts's v0.4.0).

At 94b403d, the head before this one, on Linux, in a clean checkout after npm ci: typecheck and build succeed with Node 24.22.1. We ran the command that npm test runs, with a TAP reporter added, on Node 24.22.1 and on Node 22.12.0. Each run has 620 tests: 600 pass, 18 are skipped, and 2 fail, the same 2 README tests as below.

On Linux with Node 24.22.1, before either merge of main:

• 6d72345, in a clean checkout after npm ci: typecheck and build succeed. npm test runs 609 tests: 589 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. c243333, the same way, gives 606, 586, 18 and the same 2.
• Red-first: with the three tests as 05cd9db added them, on c243333's code, in a clean checkout of 05cd9db after npm ci, all three fail. The U+2028 and U+2029 test then asked for any JSON-RPC answer, where the head's asks for a result.
• With serve()'s listener deleted from a checkout whose src and tests are 50afa96's: the U+2028 and U+2029 test passes, and the other two fail.
• A build of 50afa96, whose src is 6d72345's, with a probe and a preload beside it that inject each error:
  • under --stdio, a stream error is logged and the process exits 0;
  • over TCP, a reset socket is logged, the process stays up, and a second host is answered;
  • a TCP server error while a host is connected is logged, the process stays up, and the connected host and the next one are answered. With the server's listener removed, the same error ends the process with exit 1.

What this leaves

• A failed accept while no host is connected still ends the process, as on main at c243333. The server's listener logs the error first.
• No test reaches the TCP server's listener, since it's set up in main(). The runs of the built server above are its evidence.
• A reader that ends a line at U+2028 on some Node versions only, as Node's readline does. mcpl-core 0.3.0 creates a readline interface over its input (src/connection.ts:79 at mcpl-core-ts's v0.3.0). On Linux x86_64, fed one line holding U+2028 and one holding U+2029, node:readline gave one line each on Node 22.12.0, and two each on Node 24.22.1 and on Node 26.13.0. CI runs the tests only on Node 22.12.0, the version on which node:readline gave one line for each.
• The open Merge, update-branch, workflow runs and review reads, behind opt-in allow-lists #11 also changes src/index.ts and src/server.ts. This adds one line to each, and merges cleanly with Merge, update-branch, workflow runs and review reads, behind opt-in allow-lists #11's head, 9127a61.
• The open Comments: keep only those one rule allows, outside the files #11 changes #15 also changes the comment above isGone, so the two conflict there.

---

Ogden-2688 made the change and both merges of main. Esther-2584 read the change and both merges, and ran the built server. Daniel-2612 wrote this description, and Fiona-2757 updated it for the second merge.

[github-actions]

Automated review

Recommendation: merge and fast-follow. 1 medium severity finding can be fixed after the merge.

Reviewed commit 50afa96 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 fits. It moves to the next version of the project's own core dependency and adds logging for connection errors through the existing Logger, and it updates the docs and changelog the way the project asks.

4 reasons with sources

The higher Node floor (22.12.0) is a cost operators carry, and the change records it as a breaking change as it should.

• It fixes crashes in the server's own connection handling, using the dependency the project already uses. @animalabs/mcpl-core was already the transport. Only its version changes, and the lockfile shows no other package changing version. The new 'error' listeners sit in serve() and in main()'s TCP setup, where the connection and the server are created. (package.json (dependencies))
• Errors are logged through the Logger to stderr, as the README requires. serve() uses this.logger.error, and index.ts uses log.error, so stdout still carries only JSON-RPC. (README.md (development section: "every diagnostic goes through the Logger"))
• The higher Node floor follows from the dependency and is recorded as a breaking change for operators. The breaking fragment opens with - **Operators:** …, as the convention asks. The fixed fragment uses the documented <slug>.<kind>.md naming. The README and deploy/github-mcpl.env.example are updated to the new floor. (changelog.d/README.md)
• The new test file and the fake-host change belong to the same version bump. isGone now matches 0.4.0's error name (ConnectionClosedError) instead of message text. The new tests cover the behaviour this change adds. Neither is unrelated work. (scripts/e2e/fake-host.mjs)

Findings

1 finding of medium or high severity, see the inline comment:

• Medium severity, test coverage. CI on Node 22 cannot catch a return of the U+2028/U+2029 bug, which only appears on Node 24.

4 more findings, less severe

• Low severity, maintainability, scripts/e2e/fake-host.mjs:440-442. isGone now relies on a single error name and nothing tests it. The old check matched 'connection closed' in the message. The new one accepts only err.name === 'ConnectionClosedError'. If 0.4.0 rejects a pending request with the underlying stream error instead (for example after the stream-error change in mcpl-core-ts#11), isGone returns false. The run would then go on sending commands to a dead server and report each one as a new failure. No test or e2e run in the PR covers this path.
• Low severity, maintainability, src/server.ts:659. Two different paths log the same 'connection error' message. The new listener logs 'connection error'. The existing catch block at lines 698-701 logs the same message for any rejection from nextMessage/handleInitialize that is not a ConnectionClosedError. If a stream error both emits 'error' and rejects the pending read with the same error, it is logged twice, and operators cannot tell which path wrote a given line. Use a distinct message, or log in one place only.
• Low severity, test coverage, src/index.ts:203. No test covers the TCP server's 'error' listener. The new tcpServer.on('error', ...) is only shown working by manual runs with injected errors, as the description admits. Nothing stops it from being removed or moved before listening resolves. Moving the TCP accept loop into a function the tests can call would let a test emit a server error while a host is connected.
• Low severity, test coverage, test/connection-errors.test.ts:72-74. The U+2028 test also passes when the request gets an error answer. assert.ok(answer.result ?? answer.error) passes on any JSON-RPC error with the right id, so the test does not check that the cursor string reached the handler intact. The fixed path should answer tools/list with a result, as the malformed-line test expects. Assert on answer.result.

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

[github-actions]

Medium severity, test coverage. CI on Node 22 cannot catch a return of the U+2028/U+2029 bug, which only appears on Node 24.

The PR description says the U+2028 split only happens with mcpl-core 0.3.0's reader on Node 24. .github/workflows/ci.yml runs only node-version: 22, so on CI this test passes with either 0.3.0 or 0.4.0. It cannot fail there. The red-first run was done by hand on Node 24. Add a Node 24 job to the CI matrix, or the regression this test exists for will not be caught in CI.