From 14b56683610ca00a12232978b96680da54244003 Mon Sep 17 00:00:00 2001 From: Ogden-2688 Date: Wed, 28 Oct 2026 10:18:36 -0400 Subject: [PATCH 1/4] Wait for the host to catch up, not 50 ms, before asserting nothing arrived Three integration tests delivered a webhook event the server must not pass on, slept 50 ms and then asserted the host had received nothing. On a busy machine 50 ms can pass before anything would have arrived, so the checks could pass without proving anything. Each now asserts the delivery was answered 2xx, so a refused delivery can't pass for an absence. The harness's deliver already waits for the listener to finish handling the event, and the tests then make one host request and await its answer: the server writes to the host in order on one stream, so anything it sent while handling the event has arrived by then. The three checks guard the server's egress check as a whole, not each path inside it. --- test/integration.test.ts | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/test/integration.test.ts b/test/integration.test.ts index 0e342a9..1601559 100644 --- a/test/integration.test.ts +++ b/test/integration.test.ts @@ -121,6 +121,16 @@ async function refusal(call: Promise): Promise<{ code: number; message: /** JSON-RPC: the method does not exist on this connection. */ const ERR_METHOD_NOT_FOUND = -32601; + +// The server writes to the host in order on one stream, so once the answer to +// a later request is back, everything it sent before has arrived. +async function hostCaughtUp(h: Harness): Promise { + await h.host.sendRequest('tools/list', {}); +} + +function accepted(res: Response): void { + assert.ok(res.status >= 200 && res.status < 300, `the delivery was refused with ${res.status}`); +} /** MCPL §6.6: a capability the host did not grant. */ const ERR_CAPABILITY_DENIED = -32002; /** MCPL §8.7: the checkpoint is unknown or pruned. */ @@ -250,8 +260,8 @@ test('a host that grants nothing gets refusals on every tool, channel and push', // And no push: an event has nowhere to go. const deliver = await webhook(t, h); - await deliver('issues', fixture('issues.opened.json'), 'denied-1'); - await new Promise((r) => setTimeout(r, 50)); + accepted(await deliver('issues', fixture('issues.opened.json'), 'denied-1')); + await hostCaughtUp(h); assert.deepEqual(pushes(h), [], 'no push/event under an empty grant'); assert.deepEqual(incoming(h), [], 'and no channels/incoming'); @@ -315,8 +325,8 @@ test('a plain-MCP host sees tools only: no manifest, no push, no channels, no ro // And no event reaches the host: there is no push and no channel. const deliver = await webhook(t, h); - await deliver('issues', fixture('issues.opened.json'), 'plain-1'); - await new Promise((r) => setTimeout(r, 50)); + accepted(await deliver('issues', fixture('issues.opened.json'), 'plain-1')); + await hostCaughtUp(h); assert.deepEqual(h.hostSaw, [], 'the server sent the host nothing at all'); }); @@ -576,8 +586,8 @@ test('a disabled github.events stops the traffic at once, push and incoming alik h.host.sendNotification(method.FEATURE_SETS_UPDATE, { disabled: [EVENTS_FEATURE_SET] }); await until(() => !h.server.grant.isFeatureSetActive(EVENTS_FEATURE_SET), 'the notification to land'); - await deliver('issues', fixture('issues.closed.json'), 'after-1'); - await new Promise((r) => setTimeout(r, 50)); + accepted(await deliver('issues', fixture('issues.closed.json'), 'after-1')); + await hostCaughtUp(h); assert.equal(pushes(h).length, 1, 'no second push'); assert.equal(incoming(h).length, 1, 'and no second delivery'); }); From a64cc4394e1a70ba457e01cd83cc95335cd53f71 Mon Sep 17 00:00:00 2001 From: Ogden-2688 Date: Wed, 28 Oct 2026 10:18:36 -0400 Subject: [PATCH 2/4] Prove the 2000-id dedup ring in process, not with 2004 HTTP posts The test for the webhook listener's dedup ring posted 2004 signed deliveries over loopback, one after another, taking about 5 s of a suite run on the VPS. - webhook: DeliveryRing is exported with its capacity, and the listener has a read-only dedupCapacity beside rememberedDeliveries. - A unit test of the ring at DELIVERY_DEDUP_RING checks 2000 ids remembered, a replay at the boundary still remembered, and two more ids forgetting both gate-0 and gate-1: the ring keeps the last 2000 ids by first arrival, and a replay doesn't move an id. - The HTTP test of a replayed delivery also asserts the listener remembers one id and holds a ring of 2000. With it, the listener's replay branch, its new-id branch and its ring size stay tested through HTTP; eviction is tested on the ring alone. - The test file's header now leaves the ring's case out of "every case goes through a real node:http server". The old test's comment said re-seeing an id moves it to the tail. It doesn't. --- src/webhook.ts | 8 ++++++-- test/webhook.test.ts | 35 +++++++++++++++-------------------- 2 files changed, 21 insertions(+), 22 deletions(-) diff --git a/src/webhook.ts b/src/webhook.ts index eba3833..58d560f 100644 --- a/src/webhook.ts +++ b/src/webhook.ts @@ -78,11 +78,11 @@ export interface WebhookListenerOptions { } /** A bounded FIFO set: `add` returns true when the id was already present. */ -class DeliveryRing { +export class DeliveryRing { private readonly seen = new Set(); private readonly order: string[] = []; - constructor(private readonly capacity: number) {} + constructor(readonly capacity: number) {} add(id: string): boolean { if (this.seen.has(id)) return true; @@ -173,6 +173,10 @@ export class WebhookListener { return this.ring.size; } + get dedupCapacity(): number { + return this.ring.capacity; + } + // ── Request handling ── private handle(req: IncomingMessage, res: ServerResponse): void { diff --git a/test/webhook.test.ts b/test/webhook.test.ts index 1aea9c2..dc3affb 100644 --- a/test/webhook.test.ts +++ b/test/webhook.test.ts @@ -1,8 +1,8 @@ /** * Webhook listener (TASK-SRHKG.1.3): the order of the checks is the test. * - * Every case below goes through a real `node:http` server on a loopback port - * and a real `fetch`, because the properties under test — "rejected before + * Every case below but the dedup ring's goes through a real `node:http` server + * on a loopback port and a real `fetch`, because the properties under test — "rejected before * parsing", "2xx returned before the event is processed", "one stderr line" — * are properties of the request path and not of a function. */ @@ -13,7 +13,7 @@ import { readFileSync } from 'node:fs'; import { dirname, join } from 'node:path'; import { fileURLToPath } from 'node:url'; import { test, type TestContext } from 'node:test'; -import { MAX_BODY_BYTES, WebhookListener, webhookListenerFromConfig } from '../src/webhook.js'; +import { DELIVERY_DEDUP_RING, DeliveryRing, MAX_BODY_BYTES, WebhookListener, webhookListenerFromConfig } from '../src/webhook.js'; import { CheckGate } from '../src/normalize.js'; import { Poller, type IngressState } from '../src/poller.js'; import type { ChecksSummary, Config, Logger, NormalizedEvent, RepoRef } from '../src/types.js'; @@ -208,25 +208,20 @@ test('a replayed delivery id is dropped and the event is emitted once', async (t await r.listener.whenIdle(); assert.equal(r.events.length, 1); assert.equal(r.log.lines.filter((l) => l.includes('replay')).length, 1, r.log.text()); + assert.equal(r.listener.rememberedDeliveries, 1); + assert.equal(r.listener.dedupCapacity, 2000); }); -test('the dedup gate remembers 2000 ids and then forgets the oldest', async (t) => { - const r = await rig(t); - const body = fixture('ping.json'); - // `ping` is answered after the gate check, so it exercises dedup cheaply. - for (let i = 0; i < 2000; i++) await r.deliver('ping', body, `gate-${i}`); - assert.equal(r.listener.rememberedDeliveries, 2000); - // Still remembered at the capacity boundary. - assert.equal((await r.deliver('ping', body, 'gate-0')).status, 200); - assert.equal(r.log.lines.filter((l) => l.includes('replay')).length, 1); - - // One more distinct id than the gate holds evicts `gate-1` (the oldest - // after `gate-0` was re-seen at the tail is `gate-1`). - for (let i = 2000; i < 2002; i++) await r.deliver('ping', body, `gate-${i}`); - assert.equal(r.listener.rememberedDeliveries, 2000); - assert.equal((await r.deliver('ping', body, 'gate-1')).status, 200); - // Re-accepted, not reported as a replay: exactly one replay line so far. - assert.equal(r.log.lines.filter((l) => l.includes('replay')).length, 1, r.log.text()); +test('the dedup ring remembers 2000 ids and then forgets the oldest', () => { + const ring = new DeliveryRing(DELIVERY_DEDUP_RING); + for (let i = 0; i < 2000; i++) assert.equal(ring.add(`gate-${i}`), false, `gate-${i} is new`); + assert.equal(ring.size, 2000); + assert.equal(ring.add('gate-0'), true, 'still remembered at the capacity boundary'); + + for (let i = 2000; i < 2002; i++) assert.equal(ring.add(`gate-${i}`), false, `gate-${i} is new`); + assert.equal(ring.size, 2000); + assert.equal(ring.add('gate-0'), false, 'two more ids than the ring holds have evicted gate-0, replayed or not'); + assert.equal(ring.add('gate-1'), false, 'and gate-1'); }); // ───────────────────────────────────────────────────────────────────────────── From 59c7e3a51f7caec40dd6674565529fefcc8c55ca Mon Sep 17 00:00:00 2001 From: Ogden-2688 Date: Wed, 28 Oct 2026 10:18:36 -0400 Subject: [PATCH 3/4] Test the changelog release in process, and the CLI three times Each of the 24 changelog tests ran the script as a child Node process, about 5 s of a suite run on the VPS. - scripts/release-changelog.mjs exports release(dir), which returns the line the CLI prints and throws ReleaseError for a refusal, and ReleaseError. The CLI runs it on the current directory only when the script is the program Node was started with, comparing real paths, since argv[1] keeps a symlinked path that import.meta.url resolves. An absent argv[1], or one that names no file, means it isn't. - scripts/release-changelog.d.mts declares the two exports for the typecheck, which covers the tests. - All 24 existing cases call release() in process with the same assertions. Three new tests spawn Node: the CLI releasing through a symlink to the script; the CLI refusing a duplicate version with a fragment pending, which exits 1 with its reason on stderr and touches nothing; and an import of the script with argv[1] set to a glob pattern, which runs nothing and touches nothing. --- scripts/release-changelog.d.mts | 2 ++ scripts/release-changelog.mjs | 54 ++++++++++++++++++++------------ test/release-changelog.test.ts | 55 +++++++++++++++++++++++++++++---- 3 files changed, 86 insertions(+), 25 deletions(-) create mode 100644 scripts/release-changelog.d.mts diff --git a/scripts/release-changelog.d.mts b/scripts/release-changelog.d.mts new file mode 100644 index 0000000..6f556af --- /dev/null +++ b/scripts/release-changelog.d.mts @@ -0,0 +1,2 @@ +export class ReleaseError extends Error {} +export function release(dir: string): string; diff --git a/scripts/release-changelog.mjs b/scripts/release-changelog.mjs index a8a855d..a740ab1 100644 --- a/scripts/release-changelog.mjs +++ b/scripts/release-changelog.mjs @@ -12,10 +12,12 @@ import { existsSync, readdirSync, readFileSync, + realpathSync, unlinkSync, writeFileSync, } from "node:fs"; import { join } from "node:path"; +import { fileURLToPath } from "node:url"; const CHANGELOG = "CHANGELOG.md"; const FRAGMENT_DIR = "changelog.d"; @@ -32,7 +34,7 @@ const HEADINGS = { // process end on its own with exitCode 1. process.exit() would race the // stderr write when stderr is a pipe (asynchronous on macOS), leaving the // caller a bare failure status with no reason attached. -class ReleaseError extends Error {} +export class ReleaseError extends Error {} const fail = (msg) => { throw new ReleaseError(msg); }; @@ -54,10 +56,10 @@ const isBlockConstruct = (l) => isRule(l.trim()) || isHeading(itemContent(l)); // a well-formed fragment file aborts the release, so an entry can never be // silently left out (e.g. a fragment created under a 'fix/' subdirectory // because the slug was taken verbatim from a branch name). -function collectFragments() { +function collectFragments(dir) { const fragments = []; - if (!existsSync(FRAGMENT_DIR)) return fragments; - const entries = readdirSync(FRAGMENT_DIR, { withFileTypes: true }).sort( + if (!existsSync(join(dir, FRAGMENT_DIR))) return fragments; + const entries = readdirSync(join(dir, FRAGMENT_DIR), { withFileTypes: true }).sort( (a, b) => (a.name < b.name ? -1 : a.name > b.name ? 1 : 0), ); for (const entry of entries) { @@ -77,7 +79,7 @@ function collectFragments() { `'.<${CATEGORIES.join("|")}>.md' so the entry is not silently stranded.`, ); } - const body = readFileSync(join(FRAGMENT_DIR, name), "utf8").trim(); + const body = readFileSync(join(dir, FRAGMENT_DIR, name), "utf8").trim(); if (!body) fail(`${FRAGMENT_DIR}/${name}: empty fragment.`); const offending = body .split("\n") @@ -140,10 +142,10 @@ function mergeFragments(body, frags) { return chunks.join("\n\n"); } -function main() { - const { version } = JSON.parse(readFileSync("package.json", "utf8")); - const text = readFileSync(CHANGELOG, "utf8"); - const fragments = collectFragments(); +export function release(dir) { + const { version } = JSON.parse(readFileSync(join(dir, "package.json"), "utf8")); + const text = readFileSync(join(dir, CHANGELOG), "utf8"); + const fragments = collectFragments(dir); // Exactly one Unreleased heading. A second one silently strands entries: // only the first is ever cut, so anything filed under a later heading is @@ -185,22 +187,36 @@ function main() { // comes first) and would inject the version heading into the wrong place. const date = new Date().toISOString().slice(0, 10); writeFileSync( - CHANGELOG, + join(dir, CHANGELOG), text.slice(0, header.index) + `## Unreleased\n\n## ${version} — ${date}\n\n${merged}\n\n` + rest, ); - for (const f of fragments) unlinkSync(join(FRAGMENT_DIR, f.name)); - console.log( + for (const f of fragments) unlinkSync(join(dir, FRAGMENT_DIR, f.name)); + return ( `${CHANGELOG}: released '## ${version} — ${date}' from ${fragments.length} ` + - "fragment(s) plus the Unreleased section.", + "fragment(s) plus the Unreleased section." ); } -try { - main(); -} catch (e) { - if (!(e instanceof ReleaseError)) throw e; - console.error(`release-changelog: ${e.message}`); - process.exitCode = 1; +// import.meta.url names the script's real path, and argv[1] the path it was +// run by, which a symlink can differ from. When a module imports this one, +// argv[1] is that program's, and need not name a file at all. +function isMain() { + if (process.argv[1] === undefined) return false; + try { + return realpathSync(process.argv[1]) === fileURLToPath(import.meta.url); + } catch { + return false; + } +} + +if (isMain()) { + try { + console.log(release(process.cwd())); + } catch (e) { + if (!(e instanceof ReleaseError)) throw e; + console.error(`release-changelog: ${e.message}`); + process.exitCode = 1; + } } diff --git a/test/release-changelog.test.ts b/test/release-changelog.test.ts index 9d4e42b..a39892e 100644 --- a/test/release-changelog.test.ts +++ b/test/release-changelog.test.ts @@ -1,7 +1,3 @@ -// Black-box coverage of scripts/release-changelog.mjs: each case runs the -// real script in a throwaway directory, so assembly, validation, and the -// deletion of consumed fragments are all exercised exactly as `npm version` -// would run them. import { test } from "node:test"; import assert from "node:assert/strict"; import { execFileSync } from "node:child_process"; @@ -10,11 +6,13 @@ import { mkdtempSync, readdirSync, readFileSync, + symlinkSync, writeFileSync, } from "node:fs"; import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; -import { fileURLToPath } from "node:url"; +import { fileURLToPath, pathToFileURL } from "node:url"; +import { release, ReleaseError } from "../scripts/release-changelog.mjs"; const SCRIPT = fileURLToPath( new URL("../scripts/release-changelog.mjs", import.meta.url), @@ -60,7 +58,16 @@ function setup(f: Fixture): string { function run(dir: string): { status: number; stdout: string; stderr: string } { try { - const stdout = execFileSync(process.execPath, [SCRIPT], { + return { status: 0, stdout: `${release(dir)}\n`, stderr: "" }; + } catch (e) { + if (!(e instanceof ReleaseError)) throw e; + return { status: 1, stdout: "", stderr: `release-changelog: ${e.message}\n` }; + } +} + +function runCli(dir: string, script = SCRIPT): { status: number; stdout: string; stderr: string } { + try { + const stdout = execFileSync(process.execPath, [script], { cwd: dir, encoding: "utf8", stdio: ["ignore", "pipe", "pipe"], @@ -200,3 +207,39 @@ for (const [name, fixture, message] of refusals) { assert.deepEqual(pending(dir), before.pending, "no fragment may be deleted on refusal"); }); } + +test("the CLI releases from the directory it runs in, also when run through a symlink", () => { + const dir = setup({ fragments: { "a.fixed.md": "- Fixed thing.\n" } }); + const linked = join(dir, "release-changelog.mjs"); + symlinkSync(SCRIPT, linked); + const r = runCli(dir, linked); + assert.equal(r.status, 0, r.stderr); + assert.match(r.stdout, /^CHANGELOG\.md: released '## 1\.1\.0 — \d{4}-\d{2}-\d{2}' from 1 fragment\(s\) plus the Unreleased section\.\n$/); + assert.equal(section(changelog(dir), "1.1.0"), "### Fixed\n\n- Fixed thing."); + assert.deepEqual(pending(dir), ["README.md"]); +}); + +test("the CLI refuses with exit status 1 and its reason on stderr, touching nothing", () => { + const dir = setup({ version: "1.0.0", fragments: { "a.fixed.md": "- x\n" } }); + const before = { changelog: changelog(dir), pending: pending(dir) }; + const r = runCli(dir); + assert.equal(r.status, 1, `expected refusal, got exit ${r.status}:\n${r.stdout}${r.stderr}`); + assert.equal(r.stdout, ""); + assert.match(r.stderr, /^release-changelog: .*'## 1\.0\.0' section already exists/); + assert.equal(changelog(dir), before.changelog, "CHANGELOG.md must be untouched"); + assert.deepEqual(pending(dir), before.pending, "no fragment may be deleted on refusal"); +}); + +test("importing the script runs nothing, whatever argv[1] names", () => { + const dir = setup({ fragments: { "a.fixed.md": "- x\n" } }); + const before = { changelog: changelog(dir), pending: pending(dir) }; + const code = `process.argv[1] = "test/*.test.ts"; await import(${JSON.stringify(pathToFileURL(SCRIPT).href)});`; + const stdout = execFileSync(process.execPath, ["--input-type=module", "-e", code], { + cwd: dir, + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + }); + assert.equal(stdout, ""); + assert.equal(changelog(dir), before.changelog); + assert.deepEqual(pending(dir), before.pending); +}); From 738b552753076fdac5f133a3b728841965bfeeab Mon Sep 17 00:00:00 2001 From: Ogden-2688 Date: Wed, 28 Oct 2026 10:18:36 -0400 Subject: [PATCH 4/4] Run the test files in one process, and bound CI's job at 10 minutes node --test starts a process per test file by default, and each one loads tsx and transforms src/ again: about 0.25 s of CPU per file before any test runs. With --experimental-test-isolation=none the files run one after another in one process. With this branch's test files, the flag alone took the suite's CPU from 9.7-9.8 s to 3.6-3.7 s on a shared 12-core host, about 63% less, in two alternating pairs of runs. The verdicts were the same on Node 24.22.1 and 22.12.0, and in three imposed file orders. Node 22.7 refuses the flag and 22.8 takes it, so npm test needs Node 22.8 or later. What one process shares: the only test that changes process-wide state, githubAdapter.test.ts's process.stderr.write swap, restores it in a finally. No test writes process.env, globalThis, process listeners, process.exitCode, the cwd or the umask; src/'s module-level sets are constants, and the poller's timer is unref'd. A handle a test leaves open keeps the run from ending under either mode, so CI's job gets timeout-minutes: 10 instead of GitHub's 360-minute default, and a leak shows as a failed job. --- .github/workflows/ci.yml | 1 + package.json | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ba09526..649ff0f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -13,6 +13,7 @@ jobs: typecheck-and-test: name: Typecheck & Test (node 22) runs-on: ubuntu-latest + timeout-minutes: 10 steps: - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6.1.0 diff --git a/package.json b/package.json index 16b75b1..2c6584b 100644 --- a/package.json +++ b/package.json @@ -10,7 +10,7 @@ "scripts": { "build": "tsc", "typecheck": "tsc --noEmit", - "test": "node --import tsx --test test/*.test.ts", + "test": "node --import tsx --test --experimental-test-isolation=none test/*.test.ts", "start": "node --import tsx src/index.ts", "clean": "rm -rf dist", "version": "node scripts/release-changelog.mjs && git add CHANGELOG.md changelog.d"