diff --git a/scripts/e2e/fake-host.mjs b/scripts/e2e/fake-host.mjs index 964030c..6316bd4 100644 --- a/scripts/e2e/fake-host.mjs +++ b/scripts/e2e/fake-host.mjs @@ -1,28 +1,19 @@ #!/usr/bin/env node /** - * A scripted MCPL host for the WP10 end-to-end run (TASK-SRHKG.1.10). + * A scripted MCPL host: it spawns the built server, speaks MCPL JSON-RPC over + * its stdio, grants every capability the manifest declares, and records every + * request the server sends so a scenario can assert on it. * - * It is the host side of `test/harness.ts`, moved off an in-memory stream pair - * and onto a child process's stdio: it spawns the built server - * (`node dist/src/index.js --stdio`), speaks MCPL JSON-RPC (newline-delimited, - * one object per line) over that pipe, grants every capability the manifest - * declares, answers every request a conforming host answers, and records - * every inbound request so a scenario can assert on tags and metadata. + * It reads no environment value itself; the child inherits the environment + * untouched. `--log ` appends the transcript, every command but `quit`, + * the server's notifications and the child's stderr to that file, and the + * server's stderr can name the installation id. * - * It carries NO credentials. The App id, the private key path and the state - * file come from the environment the operator sourced before running it, and - * are inherited by the child untouched; this script never reads, prints or - * logs an environment value. The one line it prints about the child is its - * exit status. - * - * Operator interface: one JSON command per line on stdin, one JSON result per - * line on stdout, so a scenario is a file of commands and its transcript is - * the evidence. `--log ` additionally appends every frame in both - * directions, plus the child's stderr, to a durable log. + * One JSON command per line on stdin, one JSON result per line on stdout: * * {"cmd":"call","name":"comment","args":{...}} tools/call * {"cmd":"publish","params":{...}} channels/publish (Request) - * {"cmd":"notify-publish","params":{...}} channels/publish (Notification, §14.3) + * {"cmd":"notify-publish","params":{...}} channels/publish (Notification) * {"cmd":"checkpoint"} state/checkpoint * {"cmd":"rollback","params":{...}} state/rollback * {"cmd":"tools"} tools/list @@ -34,26 +25,20 @@ * {"cmd":"sleep","ms":1000} * {"cmd":"quit"} shut the child down * - * `wait` resolves on the first *recorded* request matching `method` (and, when - * given, whose JSON contains the `contains` substring) — including one that - * arrived before the wait was issued, so a scenario never races the poller. + * `wait` resolves on the first recorded request matching `method` (and, when + * given, whose JSON contains `contains`), including one that arrived before + * the wait was issued. * - * **A fulfilled RPC is not the same as a successful one.** JSON-RPC answers a - * refused tool call with a *result* carrying `isError`, not with an error - * object, so a host that reports "ok" for every answered request calls a - * refusal a pass — which is how a scenario can look green while the connector - * declined to do it. Every command is therefore judged on its result - * ({@link verdictOf}): `isError` on a tool result, `delivered: false` on a - * publish, and `success: false` or a non-empty `notUndone` on a rollback - * receipt are all failures. + * A tool call the connector refuses comes back as a result carrying `isError`, + * not as a JSON-RPC error, so results are judged: `isError` fails any command, + * and so do `delivered: false` from `publish` and `success: false` or a + * non-empty `notUndone` from `rollback`. * - * Add `"expectError": true` to a command whose refusal *is* the scenario (a - * base outside the allow-list). Then a refusal is the pass and a success is - * the failure. Two things it never excuses, because neither is the connector - * answering: a timeout, and the child exiting underneath the command. + * `"expectError": true` marks a command whose refusal is the scenario: a + * refusal passes and a success fails. A timeout, or the child exiting under + * the command, fails either way. * - * Any failure sets the process exit status to 1, so a scenario file cannot - * pass by being read without being checked. + * Any failure sets the exit status to 1. */ import { spawn } from 'node:child_process'; @@ -62,10 +47,6 @@ import { createInterface } from 'node:readline'; import * as path from 'node:path'; import { McplConnection, method } from '@animalabs/mcpl-core'; -// ───────────────────────────────────────────────────────────────────────────── -// Arguments -// ───────────────────────────────────────────────────────────────────────────── - const argv = process.argv.slice(2); function flag(name, fallback) { const i = argv.indexOf(name); @@ -74,20 +55,9 @@ function flag(name, fallback) { const serverEntry = flag('--server', path.resolve('dist/src/index.js')); const logPath = flag('--log', null); -/** - * How long a server gets to honour SIGTERM before it is killed. Ten seconds - * for a real run — the state flush is the thing being waited for — and - * overridable so the lifecycle check can exercise the escalation without - * sitting through it. - */ const shutdownGraceMs = Number.parseInt(flag('--shutdown-grace', '10000'), 10); const logStream = logPath ? createWriteStream(logPath, { flags: 'a' }) : null; -/** - * The capability set the manifest declares — `test/harness.ts`'s `FULL_GRANT`. - * The brief asks for the full grant, so nothing here is conditional: a - * scenario that wanted a narrower grant would be a different run. - */ const FULL_GRANT = [ 'tools', 'channels.register', @@ -98,31 +68,16 @@ const FULL_GRANT = [ 'channels.typing', ]; -// ───────────────────────────────────────────────────────────────────────────── -// Logging -// ───────────────────────────────────────────────────────────────────────────── - -/** One JSON line to stdout (the operator's transcript) and to `--log`. */ function emit(obj) { const line = JSON.stringify({ t: new Date().toISOString(), ...obj }); process.stdout.write(line + '\n'); logStream?.write(line + '\n'); } -/** Log-only: the frame traffic, which is evidence but not an answer. */ function trace(obj) { logStream?.write(JSON.stringify({ t: new Date().toISOString(), ...obj }) + '\n'); } -// ───────────────────────────────────────────────────────────────────────────── -// The child -// ───────────────────────────────────────────────────────────────────────────── - -// `stdio: pipe` on all three: stdout is the JSON-RPC channel and must not be -// shared with a terminal, and stderr is the server's diagnostics, which the -// report cites. -// A `.js` entry runs under this Node; anything else (the launcher, -// deploy/github-mcpl) is spawned as the host spawns it, by its own path. const [command, args] = serverEntry.endsWith('.js') ? [process.execPath, [serverEntry, '--stdio']] : [serverEntry, ['--stdio']]; @@ -134,53 +89,15 @@ const child = spawn(command, args, { createInterface({ input: child.stderr }).on('line', (line) => trace({ dir: 'stderr', line })); /** - * The child's exit, kept from the moment it is spawned. - * - * Taken here and not inside `quit()`: `child.once('exit')` registered after - * the child has already gone never fires, so a shutdown that asked for it - * then would wait forever for an event that happened while it was not - * looking. A promise created now is already settled by then and resolves at - * once. + * Kept from the moment the child is spawned: `child.once('exit')` registered + * after the child has gone never fires. */ let exitInfo = null; -/** - * Set before the child is ever signalled, so the exit that follows is known - * to be the one this host asked for. - */ let shuttingDown = false; -/** - * The child exited and nobody asked it to. - * - * Tracked separately from the per-command failures because an idle crash - * belongs to no command: the server could die between two commands, or after - * the last one, and the run would print the exit and still return 0 (review - * round 2, finding 3) — a scenario file that ends in `quit`, or just ends, - * looked exactly as green as one the server survived. - */ let unexpectedExit = false; -/** - * The shutdown this host asked for did not go cleanly (review round 3, - * finding 2). - * - * Asking a server to stop and being told it crashed on the way out is not a - * successful run: the exit status is how a server reports that its own - * shutdown failed — a state file it could not flush, a listener that would not - * close — and a host that only checked *whether* the exit was expected - * reported `{code: 3, expected: true}` and then returned 0. - */ let badShutdown = false; -/** SIGTERM was ignored long enough that the host had to escalate. */ let escalated = false; -/** - * Did the child stop the way a healthy server stops when asked? - * - * Two shapes are clean, and only two: it exited 0 under its own signal - * handler, or it had no handler and died of the SIGTERM this host sent, which - * is the default disposition and not a fault. Having to escalate to SIGKILL is - * never clean, whatever the child managed to report on the way out: it means - * the shutdown this host asked for did not happen inside its grace period. - */ function shutdownWasClean(code, signal) { if (escalated) return false; return code === 0 || (code === null && signal === 'SIGTERM'); @@ -192,8 +109,6 @@ const exited = new Promise((resolve) => { const clean = shuttingDown && shutdownWasClean(code, signal); if (!shuttingDown) { unexpectedExit = true; - // Reported as a failed result and not only as the `child-exit` line, so - // the one-line-per-failure reading of a transcript stays true. emit({ event: 'result', ok: false, @@ -216,26 +131,16 @@ const exited = new Promise((resolve) => { }); }); -/** Rejects if the child ever exits, so a command in flight can race it. */ const childLost = exited.then((info) => { const err = new Error(`the server exited (code ${info.code}, signal ${info.signal}) with this command in flight`); err.fatal = true; throw err; }); -// Every command that wins its race abandons this promise; without a handler of -// its own, the eventual exit at `quit()` would surface as an unhandled -// rejection and bury the run's real output. childLost.catch(() => {}); const conn = McplConnection.fromStreams(child.stdout, child.stdin); -// ───────────────────────────────────────────────────────────────────────────── -// Recording, and answering as a host does -// ───────────────────────────────────────────────────────────────────────────── - -/** Every request the server sent, in arrival order. */ const recorded = []; -/** Pending `wait` commands, checked on every arrival. */ const waiters = []; function matches(entry, spec) { @@ -260,8 +165,6 @@ function record(req) { conn.on('request', (req) => { record(req); switch (req.method) { - // The registry announces a thread, and a host that granted - // channels.register accepts it. case method.CHANNELS_REGISTER: case method.CHANNELS_CHANGED: { const p = req.params ?? {}; @@ -278,24 +181,13 @@ conn.on('request', (req) => { conn.sendResponse(req.id, { accepted: true }); break; default: - // Anything else is the server asking for something this host does not - // implement; refusing is honest and shows up in the transcript. conn.sendError(req.id, -32601, `unexpected ${req.method}`); } }); conn.on('notification', (notif) => trace({ dir: 'inbound-notification', method: notif.method, params: notif.params })); -// ───────────────────────────────────────────────────────────────────────────── -// Handshake -// ───────────────────────────────────────────────────────────────────────────── - async function handshake() { - // Raced against the child, like every command: a server that dies on a bad - // configuration (an unreadable key, a repository it cannot see) dies before - // it answers `initialize`, and waiting out the RPC timeout would report that - // as a slow host rather than as the startup failure its stderr already - // explains. const init = await Promise.race([handshakeRequests(), childLost]); emit({ event: 'initialized', serverInfo: init.serverInfo, capabilities: init.capabilities }); const receipt = await Promise.race([ @@ -315,10 +207,6 @@ async function handshakeRequests() { return { serverInfo: init?.serverInfo, capabilities: init?.capabilities }; } -// ───────────────────────────────────────────────────────────────────────────── -// Commands -// ───────────────────────────────────────────────────────────────────────────── - async function run(c) { switch (c.cmd) { case 'call': @@ -364,11 +252,6 @@ async function run(c) { } } -// ───────────────────────────────────────────────────────────────────────────── -// Verdicts -// ───────────────────────────────────────────────────────────────────────────── - -/** The text blocks of a tool result, joined — what the refusal reason is in. */ function textOf(result) { return (result?.content ?? []) .filter((b) => b?.type === 'text') @@ -376,22 +259,6 @@ function textOf(result) { .join('\n'); } -/** - * Did the server actually do what the command asked? - * - * Three answer shapes carry a failure inside a perfectly well-formed JSON-RPC - * *result*, and each one is a scenario that must not read as a pass: - * - * - a tool result with `isError: true` — the refusal shape of MCP; - * - a publish that reports `delivered: false`, with the reason it gives - * (an empty body, an unroutable channel); - * - a rollback receipt with `success: false`, or with entries in - * `notUndone` — comments the rollback could not undo. A receipt that - * reports leftovers is not a clean rollback however it is labelled. - * - * Anything else is taken at face value: this host asserts on shapes it knows - * and does not try to grade prose. - */ function verdictOf(c, result) { if (result && typeof result === 'object') { if (result.isError === true) return { ok: false, reason: textOf(result) || 'tool reported isError' }; @@ -408,13 +275,6 @@ function verdictOf(c, result) { return { ok: true }; } -/** - * Run one command, losing the race if the child dies under it. - * - * Without this a command sent to a server that has already exited sits until - * its own timeout — thirty seconds of nothing, reported as a timeout rather - * than as the crash it was. - */ async function runGuarded(c) { if (exitInfo) { const err = new Error(`the server had already exited (code ${exitInfo.code}, signal ${exitInfo.signal})`); @@ -424,46 +284,29 @@ async function runGuarded(c) { return await Promise.race([run(c), childLost]); } -/** A timeout or a dead child: never the connector's considered answer. */ function isFatal(err) { return err?.fatal === true || /timed out/i.test(String(err?.message ?? err)) || isGone(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 - * 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. + * The stream closing usually beats the `exit` event, so a child that dies under + * a command surfaces as mcpl-core's `Connection closed`. */ function isGone(err) { return err?.fatal === true || /connection closed/i.test(String(err?.message ?? err)); } -/** Wait briefly for the child's exit, so a failure can name its code. */ async function exitCode() { if (exitInfo) return exitInfo; const grace = new Promise((resolve) => setTimeout(() => resolve(null), 2000)); return await Promise.race([exited, grace]); } -/** - * Stop the child the way its own signal handlers expect, and wait for it: the - * poll cursor and the checkpoint lineage are flushed in `SIGTERM`'s handler, - * and scenario 9 restarts the server against that file. Killing the pipe - * instead would lose the flush and make the restart scenario a false failure. - */ async function quit() { - // Before anything else, including the early return: an exit already - // recorded was unexpected and stays that way, and an exit still to come is - // this shutdown's own. shuttingDown = true; if (exitInfo) return; child.kill('SIGTERM'); const timer = setTimeout(() => { - // Recorded before the signal, so the exit handler — which runs while this - // function is still awaiting — already knows the shutdown was escalated. escalated = true; child.kill('SIGKILL'); }, shutdownGraceMs); @@ -471,16 +314,9 @@ async function quit() { clearTimeout(timer); } -// ───────────────────────────────────────────────────────────────────────────── -// Main -// ───────────────────────────────────────────────────────────────────────────── - try { await handshake(); } catch (err) { - // The server never came up. Its stderr is in the log and says why; this is - // the one line that says the run did not start, and it exits non-zero so a - // scenario cannot be read as having passed. emit({ event: 'result', ok: false, cmd: 'handshake', error: String(err?.message ?? err) }); await quit(); logStream?.end(); @@ -492,7 +328,6 @@ let failed = false; for await (const line of rl) { const text = line.trim(); - // Blank lines and `#` comments keep a scenario file readable. if (!text || text.startsWith('#')) continue; let c; try { @@ -505,7 +340,6 @@ for await (const line of rl) { if (c.cmd === 'quit') break; trace({ dir: 'command', c }); - // What happened, before `expectError` is allowed an opinion about it. let outcome; let fatal = false; let gone = false; @@ -516,8 +350,6 @@ for await (const line of rl) { } catch (err) { fatal = isFatal(err); gone = isGone(err); - // Give the exit a moment to land, so the transcript names the status the - // server died with instead of only the broken pipe it left behind. const info = gone ? await exitCode() : null; outcome = { ok: false, @@ -527,10 +359,6 @@ for await (const line of rl) { }; } - // A scenario whose point is the refusal says so, and then the refusal is the - // pass — but a *missing* refusal is a failure, because an allow-list that - // let the call through is exactly what such a scenario exists to catch. A - // timeout and a dead child are never excused: neither is an answer. if (c.expectError && !fatal) { outcome = outcome.ok ? { ok: false, error: 'expected a refusal and the call succeeded', result: outcome.result } @@ -540,14 +368,10 @@ for await (const line of rl) { if (!outcome.ok) failed = true; emit({ event: 'result', cmd: c.cmd, label: c.label, ...outcome }); - // The server is gone; every command after this one would report the same - // thing, so stop and let the exit status say so. if (gone) break; } await quit(); conn.close(); logStream?.end(); -// An idle crash and a failed shutdown each fail the run on their own: no -// command has to have noticed either. process.exit(failed || unexpectedExit || badShutdown ? 1 : 0); diff --git a/scripts/e2e/host-lifecycle-check.mjs b/scripts/e2e/host-lifecycle-check.mjs index 9bc58c4..174be44 100644 --- a/scripts/e2e/host-lifecycle-check.mjs +++ b/scripts/e2e/host-lifecycle-check.mjs @@ -1,24 +1,4 @@ #!/usr/bin/env node -/** - * The one automated check on {@link ../e2e/fake-host.mjs}'s own behaviour. - * - * `npm test` covers `src/`; the end-to-end host is a script and nothing in the - * suite would catch a regression in it. Three review rounds found three - * separate ways it reported a green run over a server that had failed — a - * refusal taken for a success (round 2), a crash between commands (round 3 - * finding 3), and a server that crashed *during* its own shutdown (round 3 - * finding 2) — so the lifecycle it has to get right is checked here instead. - * - * It needs no credentials and no network: each case spawns the host against a - * tiny stub server written to a temp directory, which answers the MCPL - * handshake and then behaves badly in one specific way. What is asserted is - * the host's **exit status** and the verdict on its `child-exit` line, because - * those are what a scenario run is read by. - * - * node scripts/e2e/host-lifecycle-check.mjs - * - * Exits 0 when every case holds, 1 on the first that does not, naming it. - */ import { spawn } from 'node:child_process'; import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; @@ -27,15 +7,8 @@ import * as path from 'node:path'; const HOST = path.join(import.meta.dirname, 'fake-host.mjs'); const workspace = mkdtempSync(path.join(tmpdir(), 'github-mcpl-host-check-')); +const IDLE_BEFORE_INPUT_MS = 400; -/** - * A stub server, parameterised by how it misbehaves. - * - * `onSigterm`: `'clean'` exits 0, `'fail'` exits 3, `'ignore'` does nothing at - * all so the host has to escalate, and `'default'` installs no handler so the - * signal itself ends the process. `crashWhenIdle` exits 3 shortly after the - * grant, with no command in flight. - */ function stub(name, { onSigterm = 'clean', crashWhenIdle = false } = {}) { const file = path.join(workspace, `${name}.mjs`); writeFileSync( @@ -63,19 +36,10 @@ setInterval(() => {}, 1000); return file; } -/** - * Run the host against `server`, closing its stdin the way `closing` says. - * - * `quit` sends the command; `eof` just closes the pipe. Either way the input - * is delayed, so the stub has a window to misbehave while the host sits idle - * on stdin with nothing in flight — which is the state round 3's finding 3 - * was about and which a command sent immediately would hide. - */ function run(server, closing, extra = []) { return new Promise((resolve, reject) => { const child = spawn(process.execPath, [HOST, '--server', server, '--shutdown-grace', '700', ...extra], { stdio: ['pipe', 'pipe', 'pipe'], - // No credentials: the stub needs none, and this check must not read any. env: { PATH: process.env.PATH }, }); let out = ''; @@ -86,7 +50,7 @@ function run(server, closing, extra = []) { setTimeout(() => { if (closing === 'quit') child.stdin.write('{"cmd":"quit"}\n'); child.stdin.end(); - }, 400); + }, IDLE_BEFORE_INPUT_MS); child.on('exit', (code) => { const lines = out .split('\n') @@ -165,7 +129,6 @@ try { if (testCase.expected !== undefined && got.childExit?.expected !== testCase.expected) { problems.push(`child-exit.expected ${got.childExit?.expected}, wanted ${testCase.expected}`); } - // A failing run has to say why, or the status is the only evidence there is. if (testCase.exit === 1 && got.failures.length === 0) problems.push('exit 1 with no failed result reported'); if (testCase.exit === 0 && got.failures.length > 0) { problems.push(`exit 0 but ${got.failures.length} failed result(s)`); diff --git a/scripts/release-changelog.mjs b/scripts/release-changelog.mjs index a740ab1..2432edf 100644 --- a/scripts/release-changelog.mjs +++ b/scripts/release-changelog.mjs @@ -1,13 +1,6 @@ -// Runs as npm's `version` lifecycle hook (see package.json): at that point -// package.json already carries the new version, and files staged here are -// included in the release commit that `npm version` then creates and tags. -// -// Folds the pending fragments in changelog.d/ (one file per change, -// `..md`) together with anything filed -// directly under the standing `## Unreleased` section into a new -// `## X.Y.Z — YYYY-MM-DD` section, deletes the consumed fragments, and -// leaves a fresh empty `## Unreleased` above it. Refuses to release when -// there is nothing to release, or when the input's shape is ambiguous. +// Runs as npm's `version` lifecycle hook: package.json already carries the +// new version, and files staged here go into the release commit that +// `npm version` then creates and tags. import { existsSync, readdirSync, @@ -21,7 +14,6 @@ import { fileURLToPath } from "node:url"; const CHANGELOG = "CHANGELOG.md"; const FRAGMENT_DIR = "changelog.d"; -// Canonical subsection order; a fragment's category must be one of these. const CATEGORIES = ["breaking", "added", "changed", "fixed"]; const HEADINGS = { breaking: "Breaking", @@ -30,32 +22,22 @@ const HEADINGS = { fixed: "Fixed", }; -// Refusals throw and are reported at the entry point, which then lets the -// 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. +// Refusals throw and set exitCode rather than calling process.exit(), which +// can cut off a stderr write still pending: Node writes to a pipe +// asynchronously on POSIX. export class ReleaseError extends Error {} const fail = (msg) => { throw new ReleaseError(msg); }; -// Fragment grammar: every non-blank line starts a bullet or continues one -// (indented two or more spaces; nested bullets included). Headings and -// thematic breaks are refused wherever they appear — at top level a '## ' -// line would splice a fake section boundary into the released changelog, -// and as item content ('- ## x', ' ## x') they still render as headings. +// A heading inside a list item still renders as a heading. const isBulletStart = (l) => /^[-*] /.test(l); const isContinuation = (l) => /^ {2,}\S/.test(l); const itemContent = (l) => l.replace(/^\s*(?:[-*]\s+)?/, ""); const isHeading = (s) => /^#{1,6}(\s|$)/.test(s); -// Thematic breaks may carry interior whitespace ('- - -', '* * *'). const isRule = (s) => /^([-*_=])(\s*\1){2,}\s*$/.test(s); const isBlockConstruct = (l) => isRule(l.trim()) || isHeading(itemContent(l)); -// The directory is scanned fail-closed: anything that is not README.md or -// 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(dir) { const fragments = []; if (!existsSync(join(dir, FRAGMENT_DIR))) return fragments; @@ -100,11 +82,6 @@ function collectFragments(dir) { return fragments; } -// Merge fragment bullets into the Unreleased body's subsection structure. -// Directly-filed entries are kept; a fragment joins the first subsection -// whose title starts with its category (so audience-qualified headings like -// '### Breaking (module authors only)' still attract 'breaking' fragments), -// or a new canonical subsection. Output is emitted in canonical order. function mergeFragments(body, frags) { const preamble = []; const parts = []; @@ -147,9 +124,6 @@ export function release(dir) { 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 - // never released and never reaches the GitHub release notes. const headings = [...text.matchAll(/^## Unreleased[ \t]*$/gm)]; if (headings.length === 0) { fail(`no '## Unreleased' section in ${CHANGELOG} — add one before releasing.`); @@ -181,10 +155,6 @@ export function release(dir) { ); } - // Spliced by index rather than string-replaced: `text.replace("## Unreleased", …)` - // would hit the first *substring* occurrence, which is not necessarily the - // heading the regex matched (an inline mention of `## Unreleased` in prose - // comes first) and would inject the version heading into the wrong place. const date = new Date().toISOString().slice(0, 10); writeFileSync( join(dir, CHANGELOG), diff --git a/src/channels.ts b/src/channels.ts index 71abe2b..e0f5dcb 100644 --- a/src/channels.ts +++ b/src/channels.ts @@ -1,45 +1,3 @@ -/** - * Channels (TASK-SRHKG.1.5) — one MCPL channel per issue or pull request. - * - * The contract's channel bullets, and nothing beyond them: - * - * - **Descriptors.** `github:/#`, type `github`, - * `bidirectional`, label `"/#: "` with the - * title bounded to 80 chars, address `{ owner, name, number, kind }`. - * The id, label and address all come from types.ts so the format lives in - * exactly one place. - * - **Lazy registration.** A channel is announced on the first event on a - * thread ({@link ChannelRegistry.deliver}) or the first tool touch of one - * ({@link ChannelRegistry.ensureChannel}) — never a bulk enumeration of - * the repository, which would register every issue a repo ever had. Every - * touch of one channel is serialized against the others, so a registration - * still in flight cannot swallow what the touch behind it knows. Every - * announcement — `channels/register` and `channels/changed` — asks - * {@link ChannelRegistryOptions.canRegister} first, so the registry never - * exercises a capability the host has not granted. - * - **`channels/changed`** when a registered channel's label changes (the - * title was edited, or arrived after a title-less tool touch) or when the - * thread closes or reopens. What the registry reports is what the host - * *acknowledged*: a refused or undelivered update leaves the descriptor - * where it was and is retried on the channel's next touch. - * - **Incoming.** {@link ChannelRegistry.deliver} takes the outputs of .4 - * (`tagsFor` → tags + metadata, `renderEvent` → content) and sends one - * `channels/incoming` message, with the contract's thread-id rules, after - * asking {@link ChannelRegistryOptions.canDeliver} — the registration it - * waits behind is a round trip, and the grant can narrow inside it. - * - **Publish.** {@link ChannelRegistry.handlePublish} maps a host publish - * onto `createComment` or `replyToReviewComment`. - * - **Typing.** A no-op that answers success: GitHub has no typing surface. - * - * `channels/open`, `channels/close` and `channels/acknowledge` are not - * declared for this server, so nothing here implements them. - * - * The host connection is reached through {@link HostConnection}, a one-method - * seam this module defines, so .9 can hand in an `McplConnection` and a test - * can hand in a recorder. Stdout belongs to JSON-RPC; every diagnostic goes - * through the {@link Logger}, which writes to stderr. - */ - import { ERR_UNKNOWN_CHANNEL, method, @@ -72,21 +30,8 @@ import { type ThreadRef, } from './types.js'; -/** JSON-RPC: the request's params are not usable (App. A). */ const ERR_INVALID_PARAMS = -32602; -// ───────────────────────────────────────────────────────────────────────────── -// Seams and shapes this module defines (not in types.ts; .9 reconciles) -// ───────────────────────────────────────────────────────────────────────────── - -/** - * The host, as this module needs it: one way to send a request and await its - * result. `McplConnection.sendRequest` satisfies it directly. - * - * Deliberately requests only — a notification cannot report whether the host - * accepted a descriptor or a message, and §14.5 makes that per-item answer - * the point of `channels/register` and `channels/changed`. - */ export interface HostConnection { send(method: string, params: unknown): Promise<unknown>; } @@ -95,72 +40,26 @@ export interface ChannelRegistryOptions { host: HostConnection; logger?: Logger; /** - * May the registry announce a descriptor *right now*? - * - * Consulted immediately before each of the two outbound calls that announce - * one — `channels/register` and `channels/changed` — rather than once when - * the registry is built: the grant is the host's and it can narrow at any - * time (§5.4), so a registration decided before a `featureSets/update` must - * not go out after it. A denied answer leaves the channel unregistered and - * retryable, exactly as a host refusal does. - * - * Absent means "yes", which is what a test driving the registry on its own - * wants; .9 passes `channels.register` from the live grant. + * Asked before each announcement rather than once, since MCPL lets the host + * narrow the grant at any time. */ canRegister?: () => boolean; - /** - * May the registry deliver a `channels/incoming` message *right now*? - * - * Asked for the same reason as {@link ChannelRegistryOptions.canRegister}, - * and at the same moment relative to the send — but it is a *separate* - * question, and the delivery used to be decided only by the caller before - * the registration it waits behind (WP9 review round 2, finding 1). - * {@link ChannelRegistry.deliver} awaits a queued registration or refresh - * before it can send, and a `featureSets/update` the host acknowledged in - * that window has to stop the message: the answer is taken after the touch, - * immediately before `channels/incoming` goes out. - * - * Absent means "yes"; .9 passes the live `channels.incoming` grant and the - * selection of `github.events`. - */ canDeliver?: () => boolean; } /** - * `channels/publish` as this server needs it. SPEC 0.5's - * {@link ChannelsPublishParams} carries no `threadId`, but the contract routes - * a publish by exactly that (`rt:<id>` → review reply, `review:<id>` and - * absent → comment), and §14.4 already hands the host a `threadId` on - * `context/beforeInference`. So the field is optional here and .9 reconciles - * it with whatever the host actually sends (see the report's contract gaps). + * mcpl-core's `ChannelsPublishParams` has no `threadId`, and a publish is + * routed by it, so the field is added here as optional. */ export interface GitHubPublishParams extends ChannelsPublishParams { threadId?: string; } -/** Which GitHubApi call a publish took. */ export type PublishRoute = 'comment' | 'review-reply' | 'none'; /** - * What {@link ChannelRegistry.handlePublish} returns — a - * {@link ChannelsPublishResult} the server can answer the host with, widened - * with what .7's state has to record to be able to roll the write back: - * - * - `delivered` — whether a comment was created. - * - `messageId` — `String(commentId)`, the SPEC's string message id. - * - `commentId` — the created comment's numeric GitHub id, the handle - * `updateComment`/`deleteComment` (and so rollback) take. - * - `htmlUrl` — the created comment's `html_url`. - * - `route` — which GitHubApi call was used; `'none'` when nothing was sent. - * - `isReviewComment` — true for a review-thread reply, which GitHub edits - * and deletes through `pulls/comments/{id}` rather than - * `issues/comments/{id}` (the `review` flag of those seam methods). - * - `thread` — the issue or PR written to. - * - `reason` — why nothing was delivered, when `delivered` is false. - * - * A refusal is not reported here: an unknown channel and an attachment both - * throw {@link McplRpcError} so the server answers a JSON-RPC error object, - * which §6.6 requires over a result carrying a failure flag. + * MCPL wants a refusal as a JSON-RPC error object, not a result carrying a + * failure flag, so a refused publish throws instead of returning one of these. */ export interface PublishOutcome extends ChannelsPublishResult { delivered: boolean; @@ -173,38 +72,22 @@ export interface PublishOutcome extends ChannelsPublishResult { reason?: string; } -/** What {@link ChannelRegistry.deliver} reports back to the pipeline (.9). */ export interface DeliveryOutcome { - /** False when the event has no thread, or the host refused the message. */ delivered: boolean; channelId?: string; messageId?: string; threadId?: string; - /** Why nothing was delivered. */ reason?: string; } /** - * The metadata of an incoming message: everything `tagsFor` produced, plus the - * contract's `sender`. mcpl-core's `IncomingChannelMessage.author` is - * `{ id, name }` and cannot carry `isBot`, so the contract's - * `sender { login, isBot }` rides in the (open) `metadata` object, where the - * host's `metadataTrue` matchers can reach it. + * mcpl-core's `IncomingChannelMessage.author` is `{ id, name }` and can't carry + * `isBot`, so `sender` rides in the open `metadata`. */ export interface IncomingMetadata extends EventMetadata { sender: { login: string; isBot: boolean }; } -// ───────────────────────────────────────────────────────────────────────────── -// Thread ids and descriptors -// ───────────────────────────────────────────────────────────────────────────── - -/** - * The channel-level `threadId` of an event, per the contract: a review comment - * belongs to `rt:<root review comment id>` (the root is the comment the reply - * chain hangs off, so a reply reports its `inReplyTo`), a review summary to - * `review:<review id>`, and a top-level comment to no thread at all. - */ export function threadIdForEvent(event: NormalizedEvent): string | undefined { if (event.reviewComment) { const root = event.reviewComment.inReplyTo ?? event.reviewComment.id; @@ -214,12 +97,6 @@ export function threadIdForEvent(event: NormalizedEvent): string | undefined { return undefined; } -/** - * The descriptor of one thread. `title` absent means no title is known yet (a - * tool touched the thread before any event named it); the label is then the - * bare `<owner>/<name>#<number>` and the first title that arrives updates it - * through `channels/changed`. - */ export function toDescriptor(thread: ThreadRef, title?: string, state: ThreadState = 'open'): ChannelDescriptor { return { id: channelId(thread), @@ -231,10 +108,8 @@ export function toDescriptor(thread: ThreadRef, title?: string, state: ThreadSta }; } -/** Open or closed, the only descriptor state a label change tracks. */ export type ThreadState = 'open' | 'closed'; -/** The event kinds that move a thread between open and closed. */ function stateAfter(event: NormalizedEvent, current: ThreadState): ThreadState { switch (event.kind) { case 'issue.closed': @@ -249,34 +124,13 @@ function stateAfter(event: NormalizedEvent, current: ThreadState): ThreadState { } } -// ───────────────────────────────────────────────────────────────────────────── -// The registry -// ───────────────────────────────────────────────────────────────────────────── - -/** - * One registered channel. The descriptor the host **acknowledged** is kept - * apart from the title and state this server knows: a `channels/changed` the - * host refuses, or that never reaches it, must leave the two sides agreeing - * about what the host believes, and must stay retryable on the next touch. - */ interface ChannelRecord { thread: ThreadRef; - /** The last descriptor the host accepted — what `list()` and `get()` report. */ acknowledged: ChannelDescriptor; - /** The title this server knows; absent until one arrives. May be ahead of `acknowledged`. */ title?: string; - /** The state this server knows. May be ahead of `acknowledged`. */ state: ThreadState; } -/** - * Does the host already know this descriptor? `label`, `metadata` and - * `address` are what can move for a given channel id: the first two with the - * thread's title and state, and the address with its `kind`, which a tool - * touch may have registered as a guess and an event later corrects (WP9 - * review round 3, finding 1). `type` and `direction` are constants of this - * server. - */ function sameDescriptor(a: ChannelDescriptor, b: ChannelDescriptor): boolean { return ( a.id === b.id && @@ -286,23 +140,12 @@ function sameDescriptor(a: ChannelDescriptor, b: ChannelDescriptor): boolean { ); } -/** - * The registered channels of one connection. A new connection gets a new - * registry: registration is per-grant and per-host (§14.5), so nothing here - * outlives the connection that announced it. - */ export class ChannelRegistry { private readonly host: HostConnection; private readonly logger: Logger; private readonly canRegister: () => boolean; private readonly canDeliver: () => boolean; private readonly records = new Map<string, ChannelRecord>(); - /** - * The tail of the work queued for a channel. Every touch of one channel runs - * after the previous one has settled, so a registration still in flight - * cannot swallow the title and state of the event behind it, and two touches - * cannot each decide to send `channels/changed`. - */ private readonly queue = new Map<string, Promise<void>>(); constructor(opts: ChannelRegistryOptions) { @@ -312,59 +155,22 @@ export class ChannelRegistry { this.canDeliver = opts.canDeliver ?? (() => true); } - /** Every registered descriptor — the body of a `channels/list` answer. */ list(): ChannelDescriptor[] { return [...this.records.values()].map((r) => r.acknowledged); } - /** The descriptor of a channel id, or undefined when it is not registered. */ get(id: string): ChannelDescriptor | undefined { return this.records.get(id)?.acknowledged; } - /** - * The issue or PR a registered channel id names. The id itself carries no - * `issue | pull` discriminator (see docs/contract-notes.md), so the registry - * — which registered it — is the only place the full `ThreadRef` exists. - */ threadFor(id: string): ThreadRef | undefined { return this.records.get(id)?.thread; } - /** - * Register the channel of `thread` if it is not registered yet, and update - * its label if `title` or the thread's state moved. This is the "first tool - * touch" entry point: a tool that acts on a thread calls it with whatever - * title it already fetched (or none). - * - * Returns the descriptor the host has acknowledged, or undefined when the - * host refused the registration. - */ async ensureChannel(thread: ThreadRef, title?: string, state?: ThreadState): Promise<ChannelDescriptor | undefined> { return this.touch(thread, (current) => ({ title, state: state ?? current?.state ?? 'open' })); } - /** - * Register or reconcile one channel, serialized against every other touch of - * it. `fields` is evaluated inside the queue, against the record as it stands - * *then*, so a caller can derive the title and state it wants from the - * channel's current ones without reading a value that a queued touch is - * about to change. - * - * A registered channel also takes the `kind` of the thread it is touched - * with. The channel id carries no `issue | pull` discriminator, so this - * record is the only place the full `ThreadRef` lives — and `kind` is the - * one part of it that can be wrong, because the first touch of a thread may - * have been a tool call naming `#58`, which defaults to `issue` whether or - * not 58 is a pull request. The caller is responsible for handing over a - * *resolved* kind (the server resolves every one through - * `ThreadKnowledge`'s rule 2, and an event's is authoritative by - * construction), so the newest one wins: it becomes the thread this channel - * publishes to, and `refresh` tells the host through `channels/changed`, - * because the descriptor's address moved with it. Without this a review - * reply on such a channel was refused for the rest of the connection (WP9 - * review round 3, finding 1). - */ private touch( thread: ThreadRef, fields: (current: ChannelRecord | undefined) => { title?: string; state: ThreadState }, @@ -383,26 +189,20 @@ export class ChannelRegistry { }); } - /** Run `work` after everything already queued for `id` has settled. */ private enqueue<T>(id: string, work: () => Promise<T>): Promise<T> { const previous = this.queue.get(id) ?? Promise.resolve(); - // `work` runs however the previous touch ended: one failed registration - // must not wedge the channel's queue. const run = previous.then(work, work); const tail = run.then( () => undefined, () => undefined, ); const settled = tail.then(() => { - // Nothing queued behind this one: drop the entry rather than keep one - // promise per channel ever touched. if (this.queue.get(id) === settled) this.queue.delete(id); }); this.queue.set(id, settled); return run; } - /** Announce one new channel with `channels/register` (§14.5, itemized). */ private async register( thread: ThreadRef, title: string | undefined, @@ -410,10 +210,6 @@ export class ChannelRegistry { ): Promise<ChannelDescriptor | undefined> { const descriptor = toDescriptor(thread, title, state); if (!this.canRegister()) { - // Not recorded, and nothing sent: announcing a channel is - // `channels.register`, and a path the host did not grant is denied by - // its absence (§5.4). The next touch tries again, so a widened grant - // picks the channel up. this.logger.info('channel not announced', { channel: descriptor.id, reason: 'registration is not granted' }); return undefined; } @@ -428,8 +224,6 @@ export class ChannelRegistry { const entry = result?.results?.find((r) => r.id === descriptor.id); if (entry && entry.accepted === false) { - // Not recorded: a channel marked known here would never be announced - // again after the host's policy widened. this.logger.info('host refused channel', { channel: descriptor.id, reason: entry.reason }); return undefined; } @@ -439,15 +233,6 @@ export class ChannelRegistry { return descriptor; } - /** - * Take in a title and a state, and tell the host with `channels/changed` - * when the descriptor they produce is not the one it acknowledged. - * - * The record's acknowledged descriptor advances only on an accepted update: - * a host that answers `accepted: false`, and a request that never gets an - * answer, both leave it where it was, so `get()` keeps reporting what the - * host actually believes and the next touch of the channel retries. - */ private async refresh(record: ChannelRecord, title?: string, state?: ThreadState): Promise<void> { if (title !== undefined) record.title = title; if (state !== undefined) record.state = state; @@ -456,9 +241,6 @@ export class ChannelRegistry { if (sameDescriptor(desired, record.acknowledged)) return; if (!this.canRegister()) { - // `channels/changed` re-announces a descriptor, so it needs the same - // authority as the registration it amends. The record keeps what the - // host acknowledged and the next touch retries. this.logger.info('channel update not announced', { channel: desired.id, reason: 'registration is not granted' }); return; } @@ -469,8 +251,6 @@ export class ChannelRegistry { | ChannelsChangedResult | undefined; } catch (err) { - // The host keeps the descriptor it has; the channel itself still works, - // and the next touch sends the update again. this.logger.warn('channels/changed failed', { channel: desired.id, error: (err as Error).message }); return; } @@ -485,17 +265,6 @@ export class ChannelRegistry { this.logger.debug('channel changed', { channel: desired.id, label: desired.label, state: record.state }); } - // ── Incoming ── - - /** - * Deliver one event to its channel: register the channel if this is the - * first the host hears of the thread, update the label if the title or state - * moved, then send one `channels/incoming` message. - * - * `tags`, `metadata` and `content` are what .4 produced for this event - * (`tagsFor` and `renderEvent`); this module never re-derives them, and - * `event.raw` never leaves the pipeline. - */ async deliver( event: NormalizedEvent, tags: string[], @@ -503,27 +272,17 @@ export class ChannelRegistry { content: ContentBlock[], ): Promise<DeliveryOutcome> { if (!event.thread) { - // A push or a repository-level event belongs to no channel; .9 sends - // those as push/event only. return { delivered: false, reason: 'no-thread' }; } const thread = event.thread; const id = channelId(thread); - // Both derived against the record as it stands when the touch runs, not as - // it stands now: a registration or an update may still be in flight. const descriptor = await this.touch(thread, (current) => ({ title: event.title, state: stateAfter(event, current?.state ?? 'open'), })); if (!descriptor) return { delivered: false, channelId: id, reason: 'channel-not-registered' }; - // After the touch, not before it: the registration this delivery waited - // behind is an awaited round trip to the host, and a `featureSets/update` - // acknowledged in that window must stop the message (WP9 review round 2, - // finding 1). The event is dropped rather than queued — a stale event - // delivered after the host narrowed its policy is exactly what the - // reduction asked not to happen. if (!this.canDeliver()) { this.logger.info('channel delivery not sent', { channel: descriptor.id, @@ -569,25 +328,14 @@ export class ChannelRegistry { }; } - // ── Publish ── - /** - * A host publish onto GitHub, routed by `threadId` exactly as the contract - * says: - * - * - absent → `createComment` on the issue or PR; - * - `rt:<id>` → `replyToReviewComment` in that review conversation; - * - `review:<id>` → `createComment` (a review summary has no reply API). - * - * Refusals, both as JSON-RPC error objects (§6.6): - * - an unknown or unregistered channel id → `-32023 Unknown channel`; - * - any non-text content block → `-32602 Invalid params`, naming the - * block type. Attachments are not supported in v1. - * - a `threadId` that is neither `rt:<id>` nor `review:<id>`, or an - * `rt:<id>` reply on an issue (review comments live on pull requests - * only) → `-32602 Invalid params`. - * - * See {@link PublishOutcome} for the return shape. + * A publish with no `threadId`, or with `review:<id>`, becomes a comment on + * the thread, since GitHub has no reply API for a review summary; `rt:<id>` + * becomes a reply in that review conversation. A publish whose text is empty + * once trimmed posts nothing and returns `delivered: false`, `route: 'none'` + * and `reason: 'empty-content'`. Refused with -32023 for a channel that isn't + * registered, and with -32602 for a non-text block, a `threadId` of another + * form, or an `rt:` reply on an issue. */ async handlePublish(params: GitHubPublishParams, api: GitHubApi): Promise<PublishOutcome> { const record = this.records.get(params.channelId); @@ -613,8 +361,6 @@ export class ChannelRegistry { .join('\n') .trim(); if (!text) { - // Nothing to post. Not an error: an empty turn is a host decision, and - // an empty GitHub comment would be worse than none. this.logger.debug('publish skipped', { channel: params.channelId, reason: 'empty-content' }); return { delivered: false, route: 'none', thread, reason: 'empty-content' }; } @@ -640,7 +386,6 @@ export class ChannelRegistry { return this.published(comment, 'review-reply', thread, true); } - // No threadId, or a review summary: both are a top-level comment. const comment = await api.createComment(thread, text); return this.published(comment, 'comment', thread, false); } @@ -658,13 +403,9 @@ export class ChannelRegistry { }; } - // ── Typing ── - /** - * `channels/typing` — a no-op that answers success. GitHub has no typing - * indicator on an issue or a PR, and the alternative (a placeholder comment) - * would be a visible write nobody asked for. SPEC §14 fixes no result shape - * for the method, so the answer is an empty success object. + * GitHub has no typing indicator on an issue or pull request, so + * `channels/typing` answers an empty success and does nothing. */ handleTyping(params: ChannelsTypingParams): Record<string, never> { this.logger.debug('typing ignored', { channel: params.channelId }); diff --git a/src/content.ts b/src/content.ts index 697838c..36a241d 100644 --- a/src/content.ts +++ b/src/content.ts @@ -1,26 +1,3 @@ -/** - * Rendering (TASK-SRHKG.1.4; contract section "Tags and metadata" and "Tool - * results are text blocks rendered by content.ts, bounded"). - * - * Pure functions: data in, `ContentBlock[]` out. Everything the model reads - * from GitHub passes through here, which is why the bounds live in one file: - * - * - a body is bounded to `LIMITS.commentChars` (4000) with a note saying how - * much was dropped — never a silent cut; - * - a diff hunk is bounded to `CONTENT_LIMITS.diffHunkLines` (40) lines; - * - a list is bounded to `LIMITS.listEntries` (50) entries with a "more" note; - * - a title is bounded to `LIMITS.titleChars` (80), as channel labels are. - * - * Markdown is passed through verbatim — the agent reads and writes the same - * dialect GitHub does. HTML comments are passed through too: a sticky marker - * (`<!-- sticky:release-notes -->`) is how `upsert_sticky_comment` finds its - * own comment again, so the agent has to be able to see one. - * - * `NormalizedEvent.raw` is never read here. The renderers touch only the named - * fields of the contract's types, so no raw payload field can leak into a - * block. - */ - import type { ContentBlock } from '@animalabs/mcpl-core'; import { LIMITS, @@ -38,28 +15,18 @@ import { type ThreadRef, } from './types.js'; -/** The bounds the contract states in prose rather than in `LIMITS`. */ export const CONTENT_LIMITS = { - /** Lines of a diff hunk shown beside a review comment. */ diffHunkLines: 40, } as const; -// ───────────────────────────────────────────────────────────────────────────── -// Primitives -// ───────────────────────────────────────────────────────────────────────────── - function textBlock(text: string): ContentBlock { return { type: 'text', text }; } /** - * A body, bounded with its own notice and otherwise byte-for-byte what GitHub - * stored. - * - * Trimming is used to *detect* an empty body and nowhere else: leading - * whitespace is Markdown (four spaces open an indented code block, two at the - * end of a line are a hard break), so stripping it would silently rewrite the - * author's text into something that renders differently. + * Leading and trailing whitespace is Markdown (four spaces open a code block, + * two at a line's end are a hard break), so a body is trimmed only to detect + * an empty one. */ function body(text: string | undefined): string { const raw = text ?? ''; @@ -67,7 +34,6 @@ function body(text: string | undefined): string { return bounded(raw, LIMITS.commentChars); } -/** A one-line title for a header, bounded like a channel label. */ function title(text: string): string { return bounded(text.replace(/\s+/g, ' ').trim(), LIMITS.titleChars); } @@ -76,16 +42,10 @@ function actor(a: EventActor): string { return a.isBot ? `${a.login} (bot)` : a.login; } -/** `owner/name#number`, or `owner/name` for an event with no thread. */ function ref(repo: { owner: string; name: string }, number?: number): string { return number === undefined ? repoSlug(repo) : `${repoSlug(repo)}#${number}`; } -/** - * Render at most `LIMITS.listEntries` items, then say how many were dropped. - * Every list the model sees goes through this, so "50 and a note" is one - * decision rather than seven. - */ function list<T>(items: readonly T[], render: (item: T) => string): string[] { const shown = items.slice(0, LIMITS.listEntries).map(render); if (items.length > LIMITS.listEntries) { @@ -96,7 +56,6 @@ function list<T>(items: readonly T[], render: (item: T) => string): string[] { return shown; } -/** A diff hunk, bounded to 40 lines, saying how many it dropped. */ export function boundedDiffHunk(hunk: string): string { const lines = hunk.split('\n'); if (lines.length <= CONTENT_LIMITS.diffHunkLines) return hunk; @@ -109,18 +68,6 @@ function line(path: string, lineNo: number | null | undefined): string { return lineNo === null || lineNo === undefined ? `${path}:?` : `${path}:${lineNo}`; } -// ───────────────────────────────────────────────────────────────────────────── -// renderEvent -// ───────────────────────────────────────────────────────────────────────────── - -/** - * One event as one text block: a header line (repo#number, kind, actor, link), - * then whatever the kind carries. - * - * A review comment adds its `path:line` and its diff hunk; a checks event adds - * the failed job names and URLs; a review adds its verdict. The body, when - * there is one, is last and bounded. - */ export function renderEvent(event: NormalizedEvent): ContentBlock[] { const lines: string[] = [ `${ref(event.repo, event.thread?.number)} ${event.kind} by ${actor(event.actor)} — ${event.html_url}`, @@ -146,18 +93,11 @@ export function renderEvent(event: NormalizedEvent): ContentBlock[] { lines.push(...list(c.failed, (f) => `- ${f.name} — ${f.url}`)); } - // Deletions and synchronizes carry no body; only say "(no body)" where a - // body is the point of the event. if (event.body !== undefined) lines.push('', body(event.body)); return [textBlock(lines.join('\n'))]; } -// ───────────────────────────────────────────────────────────────────────────── -// Tool results -// ───────────────────────────────────────────────────────────────────────────── - -/** `get_issue`. */ export function renderIssue(issue: Issue): ContentBlock[] { const state = issue.stateReason ? `${issue.state} (${issue.stateReason})` : issue.state; const lines = [ @@ -170,7 +110,6 @@ export function renderIssue(issue: Issue): ContentBlock[] { return [textBlock(lines.join('\n'))]; } -/** `get_pull_request`. */ export function renderPullRequestSummary(pr: PullRequest): ContentBlock[] { const state = pr.merged ? 'merged' : pr.draft ? `${pr.state} (draft)` : pr.state; const mergeable = pr.mergeable === null ? 'computing' : pr.mergeable ? 'yes' : 'no'; @@ -194,7 +133,6 @@ export function renderPullRequestSummary(pr: PullRequest): ContentBlock[] { return [textBlock(lines.join('\n'))]; } -/** `list_comments`: a thread's comments, each body bounded, the list capped. */ export function renderThread(thread: ThreadRef, comments: readonly Comment[]): ContentBlock[] { const header = `${ref(thread.repo, thread.number)} ${thread.kind}: ${comments.length} comment(s)`; if (comments.length === 0) return [textBlock(`${header}\n(no comments)`)]; @@ -209,19 +147,15 @@ export function renderThread(thread: ThreadRef, comments: readonly Comment[]): C return [textBlock([header, '', rendered.join('\n\n')].join('\n'))]; } -/** `list_review_threads`: review conversations on a diff. */ export function renderReviewThreads(threads: readonly ReviewThread[]): ContentBlock[] { if (threads.length === 0) return [textBlock('(no review threads)')]; const rendered = list(threads, (t) => { - // Both identifiers, because they address different things: `t.id` is the - // GraphQL node id `resolve_thread`/`unresolve_thread` take, `rt:<root - // comment id>` is the channel thread id `reply_in_thread` takes. An agent - // reading this result needs each for a different tool. + // `t.id` is the GraphQL node id `resolve_thread` and `unresolve_thread` + // take, and `rt:<root comment id>` is the id `reply_in_thread` takes. const state = t.isResolved ? 'resolved' : 'unresolved'; const head = `${line(t.path, t.line)} (${t.side}) ${state} — thread ${t.id} (reply id rt:${t.comments[0]?.id ?? '?'})`; const comments = list(t.comments, (c) => - // The body is not indented: an indent is Markdown, and this one is not - // the author's. + // Not indented: an indent is Markdown. [` #${c.id} by ${actor(c.author)} at ${c.updatedAt ?? c.createdAt}`, body(c.body)].join('\n'), ); return [head, ...comments].join('\n'); @@ -231,7 +165,6 @@ export function renderReviewThreads(threads: readonly ReviewThread[]): ContentBl ]; } -/** `get_check_runs`: the rolled-up conclusion on one sha and what failed. */ export function renderChecks(checkRuns: ChecksSummary): ContentBlock[] { const lines = [ `checks on ${checkRuns.sha}: ${checkRuns.conclusion ?? 'pending'}, ${checkRuns.total} run, ${checkRuns.failed.length} failed`, @@ -241,7 +174,6 @@ export function renderChecks(checkRuns: ChecksSummary): ContentBlock[] { return [textBlock(lines.join('\n'))]; } -/** `search_issues`. */ export function renderSearch(results: readonly IssueSearchHit[]): ContentBlock[] { if (results.length === 0) return [textBlock('(no matches)')]; const lines = [ @@ -255,11 +187,6 @@ export function renderSearch(results: readonly IssueSearchHit[]): ContentBlock[] return [textBlock(lines.join('\n'))]; } -/** - * `list_pull_request_files`. Not named in .4's function list, but the list - * bound belongs with the other six rather than being re-invented in .6; names - * and stats only, as the contract requires (no patch text). - */ export function renderPullRequestFiles(files: readonly PullRequestFile[]): ContentBlock[] { if (files.length === 0) return [textBlock('(no files)')]; const lines = [ diff --git a/src/errors.ts b/src/errors.ts index e5cd58e..d4193a7 100644 --- a/src/errors.ts +++ b/src/errors.ts @@ -1,17 +1,7 @@ /** - * Typed JSON-RPC errors for MCPL handlers. - * - * SPEC 0.5 §6.6: rejection is diagnostics, not authorization. A handler that - * cannot answer MUST return a JSON-RPC *error object* with a documented code — - * never silence, and never a result carrying a failure flag. The error tells - * the peer what happened; it grants nothing and negotiates nothing. - * - * Codes come from @animalabs/mcpl-core (App. A, §14.6): - * -32002 Capability denied — `data: { capability }` - * -32602 Invalid params - * - * Copied from slack-mcpl's src/errors.ts (protocol-generic; no Slack-specific - * logic). grant.ts needs it, so it lands with the .1 skeleton. + * MCPL SPEC §6.6: a method that will never be answered must return an error, + * and a host's rejection is a JSON-RPC error object with the documented code, + * not a result carrying a failure flag. These errors take that shape. */ import { ERR_CAPABILITY_DENIED } from '@animalabs/mcpl-core'; @@ -27,7 +17,6 @@ export class McplRpcError extends Error { } } -/** §14.6 / §5.4 — the method requires a capability not in the effective grant. */ export function capabilityDenied(capability: string): McplRpcError { return new McplRpcError(ERR_CAPABILITY_DENIED, `Capability denied: ${capability}`, { capability }); } diff --git a/src/grant.ts b/src/grant.ts index 1962a40..2a91e0c 100644 --- a/src/grant.ts +++ b/src/grant.ts @@ -1,29 +1,8 @@ /** - * Capability grant state (SPEC 0.5 §5.3, §5.4, §6.4, §6.7). - * - * The grant is the security boundary. This class holds what the *host* told - * this connection it may do, and nothing else. The message-to-grant step, the - * wildcard matching, and the §6.4 derivation all come from @animalabs/mcpl-core - * (`grantFromUpdate`, `capabilityGranted`, `deriveFeatureSets`); this wrapper - * adds the connection-lifetime state around them and the degradation receipt. - * - * - `effectiveCapabilities` is the sole normative allowlist. Every path not - * present is denied; absence is the denial, there is no unspecified state - * (§5.4). - * - `deniedCapabilities` is derived diagnostic data. It is read only to detect - * a malformed policy message and never participates in an authorization - * decision (§5.4). - * - Until the initial policy exchange completes, every capability-dependent - * behavior is unavailable (§5.3). `has()` therefore returns false before the - * first grant-bearing `featureSets/update` Request. - * - * Nothing here widens anything. The degradation receipt this produces is - * testimony about consequences, never an assertion of entitlement (§6.7). - * - * Ported from slack-mcpl's src/grant.ts (itself from zulip-mcp), which is - * protocol-generic. The github adaptation is the `uses` validation of this - * server's own declarations and {@link CapabilityGrant.toolAllowed}, which - * gates a `tools/call` on the github feature set that owns the tool. + * MCPL SPEC §5.3, §5.4: `effectiveCapabilities` is the only allowlist, and a + * path it doesn't name is denied; `deniedCapabilities` is diagnostic and never + * authorizes. Nothing is granted before the first grant-bearing + * `featureSets/update` Request. */ import { @@ -56,73 +35,42 @@ export class CapabilityGrant { constructor(private declarations: Record<string, FeatureSetDeclaration> = {}) {} - /** - * Point the grant at the feature-set declarations of a newly installed - * manifest (§17.5). Declarations are what degradation is derived from - * (§6.4); they are not authority, so this widens nothing — the effective - * grant is untouched and stays whatever the host last sent. - */ + /** Declarations aren't authority (MCPL §6.4), so this widens nothing. */ setDeclarations(declarations: Record<string, FeatureSetDeclaration>): void { this.declarations = declarations; } - /** - * Forget the grant: a new connection starts from nothing (§5.3) — the - * previous peer's policy is not this peer's. - */ + /** A new connection starts with no grant (MCPL §5.3). */ reset(): void { this.state = emptyGrantState(); } - /** True once a grant-bearing `featureSets/update` Request has been accepted. */ isReady(): boolean { return this.state.ready; } - /** - * Is `path` in the effective grant? False before the initial policy - * exchange, and false for any path the host did not name. - */ has(path: string): boolean { if (!this.state.ready) return false; return capabilityGranted(this.state.effectiveCapabilities, path); } - /** Is a declared feature set currently active? */ isFeatureSetActive(name: string): boolean { if (!this.state.ready) return false; if (!featureSetSelected(this.state, name)) return false; return this.missingFor(name).length === 0; } - /** - * Did the host *select* this feature set — i.e. not disable it (§6.7) — - * whatever its declared `uses` derive to? - * - * Selection and activity differ for a set whose surfaces are gated per - * capability rather than per set: `github.events` declares `uses: - * ['pushEvents']`, so it is inactive under a channels-only grant, and yet a - * `channels/incoming` delivery is still granted. The pipeline asks this - * first, so a host that turned github.events off gets neither path, and - * asks `isFeatureSetActive` for the push it declared. - */ isFeatureSetSelected(name: string): boolean { if (!this.state.ready) return false; return featureSetSelected(this.state, name); } - /** Capability paths a declared feature set needs but has not been granted. */ missingFor(name: string): string[] { const decl = this.declarations[name]; if (!decl) return []; return decl.uses.filter((use) => !this.has(use)); } - /** - * May a `tools/call` for `toolName` run? `tools` has to be granted, and the - * github feature set that owns the tool has to be active. A tool no feature - * set claims is not reachable: absence is the denial (§5.4). - */ toolAllowed(toolName: string): boolean { if (!this.has('tools')) return false; const featureSet = featureSetForTool(toolName); @@ -131,12 +79,8 @@ export class CapabilityGrant { } /** - * Feature sets of this server whose own `uses` is invalid under the closed - * §6.2 vocabulary — absent, empty, or naming a path that is not a capability - * path. A host disables such a set with reason `invalid_uses` (§6.4), so this - * is a declaration bug on our side, not a policy outcome; the startup path - * and the degradation receipt both surface it rather than letting the feature - * set go quietly missing. + * A host disables a feature set whose `uses` is absent, empty or not a + * capability path, with reason `invalid_uses` (MCPL §6.2, §6.4). */ invalidDeclarations(): Array<{ featureSet: string; unrecognized: string[] }> { return Object.entries(this.declarations) @@ -146,22 +90,10 @@ export class CapabilityGrant { } /** - * Apply a `featureSets/update` and return the degradation receipt (§6.7). - * - * Throws {@link McplRpcError} when the message is malformed — §5.4 requires - * the receiving side to fail closed and reject a policy naming a path in - * both `effectiveCapabilities` and `deniedCapabilities`. Fail-closed means - * exactly that: the grant that results holds nothing and is not ready, so a - * malformed message cannot leave a previous, wider grant standing. - * - * `form` is how the message arrived. §6.7 requires the **Request** form for - * any change to the effective grant and says a Notification "cannot - * establish a ready state". A Notification therefore never alters the grant - * except to apply `disabled` reductions — reductions are respected whatever - * the carrier — and any other grant-bearing field it carries is discarded - * with a note, because honouring a widening from an unacknowledgeable - * message would have this server acting on a path the host cannot know it - * accepted. + * MCPL §5.4: a policy naming a path in both `effectiveCapabilities` and + * `deniedCapabilities` is malformed, and the grant fails closed: it holds + * nothing and isn't ready. §6.7: only a Request can change the effective + * grant, so a Notification applies only its `disabled` reductions. */ apply( params: FeatureSetsUpdateParams, @@ -180,8 +112,7 @@ export class CapabilityGrant { } if (form === 'request' && params.effectiveCapabilities === undefined) { - // §5.4: effectiveCapabilities is the sole allowlist. Absent means the - // empty allowlist, not "unchanged" and not "everything". + // MCPL §5.4: an absent `effectiveCapabilities` is the empty allowlist. notes.push( 'No effectiveCapabilities in featureSets/update; treating the grant as empty (SPEC 0.5 §5.4).', ); @@ -219,7 +150,6 @@ export class CapabilityGrant { }), ); for (const [featureSet, why] of Object.entries(derivation.disabled)) { - // Not the host's policy: our own declaration is unusable (§6.4). if (why.reason === 'invalid_uses') { notes.push( `Feature set ${featureSet} declares an invalid \`uses\` and is disabled (SPEC 0.5 §6.4)` + diff --git a/src/normalize.ts b/src/normalize.ts index 5d8a440..61b255e 100644 --- a/src/normalize.ts +++ b/src/normalize.ts @@ -1,17 +1,3 @@ -/** - * Ingress normalization (TASK-SRHKG.1.3, §Ingress). - * - * One place turns a GitHub delivery into a {@link NormalizedEvent}: a raw - * webhook payload (webhook.ts) or a polled row (poller.ts). Everything here - * is pure and total — a payload is untrusted input, so a field that is - * missing or of the wrong type makes the event drop rather than throw — with - * one exception at the bottom, {@link resolveCheckEvents}, which is the async - * check-run lookup the contract puts on a `checks.completed` event. - * - * `raw` is kept on every event (the contract's field) and is never read by - * anything downstream: tags.ts and content.ts see only the named fields. - */ - import { createHash } from 'node:crypto'; import { parseRepoSlug, @@ -32,43 +18,23 @@ import { type ThreadRef, } from './types.js'; -// ───────────────────────────────────────────────────────────────────────────── -// Deliveries and poll rows (types the contract implies; see the .3 report) -// ───────────────────────────────────────────────────────────────────────────── - -/** - * One verified webhook delivery. `id` is `X-GitHub-Delivery` (the event id the - * contract names), `event` is `X-GitHub-Event`, `payload` is the parsed JSON — - * parsed only after webhook.ts has checked the signature. - */ export interface WebhookDelivery { id: string; event: string; payload: unknown; - /** ISO-8601; webhook.ts stamps it when the body finished arriving. */ receivedAt: string; } -/** - * Which `since` query a poll row came from. Part of the dedup key and of the - * poll event id (`poll:<kind>:<id>:<updated_at>`, contract amendment 1). - */ export type PollRowKind = 'issue' | 'issue_comment' | 'review_comment' | 'checks'; -/** `<kind>:<id>:<updated_at>` — what `StateStore.seenPollKey` remembers. */ export function pollKey(kind: PollRowKind, id: number | string, updatedAt: string): string { return `${kind}:${id}:${updatedAt}`; } -/** `poll:<kind>:<id>:<updated_at>` — the `NormalizedEvent.id` of a polled event. */ export function pollEventId(key: string): string { return `poll:${key}`; } -// ───────────────────────────────────────────────────────────────────────────── -// Untrusted-JSON accessors -// ───────────────────────────────────────────────────────────────────────────── - type Json = Record<string, unknown>; function asObject(value: unknown): Json | undefined { @@ -97,11 +63,6 @@ function at(value: unknown, ...path: string[]): unknown { return cursor; } -// ───────────────────────────────────────────────────────────────────────────── -// Shared field extraction -// ───────────────────────────────────────────────────────────────────────────── - -/** `login` ending in `[bot]`, or GitHub's own `type: 'Bot'`. */ export function actorFrom(user: unknown): EventActor | undefined { const login = asString(at(user, 'login')); if (!login) return undefined; @@ -109,7 +70,6 @@ export function actorFrom(user: unknown): EventActor | undefined { return { login, isBot: type === 'Bot' || login.endsWith('[bot]') }; } -/** The repository a delivery is about, from `repository.full_name`. */ export function repoFrom(payload: unknown): RepoRef | undefined { const full = asString(at(payload, 'repository', 'full_name')); if (full) { @@ -144,8 +104,6 @@ function reviewStateFrom(value: unknown): ReviewState { case 'changes_requested': return 'changes_requested'; default: - // `commented`, and `dismissed`/anything else a future action adds: a - // review that is neither an approval nor a block is a comment. return 'commented'; } } @@ -166,15 +124,10 @@ function thread(repo: RepoRef, number: number | undefined, kind: ThreadKind): Th return number === undefined ? undefined : { repo, number, kind }; } -/** A delivery that fans out gets `<delivery id>#<n>`; a single event keeps the id. */ function eventId(deliveryId: string, index: number, total: number): string { return total > 1 ? `${deliveryId}#${index}` : deliveryId; } -// ───────────────────────────────────────────────────────────────────────────── -// Webhook payloads -// ───────────────────────────────────────────────────────────────────────────── - const ISSUE_ACTIONS: Record<string, NormalizedEventKind> = { opened: 'issue.opened', edited: 'issue.edited', @@ -199,12 +152,9 @@ const PULL_ACTIONS: Record<string, NormalizedEventKind> = { const REVIEW_COMMENT_ACTIONS: Record<string, NormalizedEventKind> = { created: 'review_comment.created', edited: 'review_comment.edited', - // The contract names no `review_comment.deleted`; a deleted review comment - // is a deleted comment that happens to carry `reviewComment`. deleted: 'comment.deleted', }; -/** The webhook event names this ingress maps. Anything else is dropped. */ export const HANDLED_WEBHOOK_EVENTS: readonly string[] = [ 'issues', 'issue_comment', @@ -216,15 +166,6 @@ export const HANDLED_WEBHOOK_EVENTS: readonly string[] = [ 'push', ]; -/** - * A verified delivery, normalized. Returns `[]` for an event or an action the - * contract names no kind for (`issues.labeled`, `pull_request.reopened`, - * `ping`, `installation`, …) and for a payload too malformed to place — - * dropping is the only safe answer, and webhook.ts logs the drop. - * - * Only `check_suite`/`check_run` can return more than one event, one per head - * sha (§Ingress); the ids are then `<delivery>#<n>`. - */ export function normalizeWebhook(delivery: WebhookDelivery): NormalizedEvent[] { const { payload } = delivery; const repo = repoFrom(payload); @@ -312,8 +253,7 @@ export function normalizeWebhook(delivery: WebhookDelivery): NormalizedEvent[] { body: asString(review.body), html_url, // The commit the review was written against, which is not the PR's - // current head once the branch moved under the reviewer. Only an - // older payload without `commit_id` falls back to the head. + // current head once the branch moved under the reviewer. sha: asString(review.commit_id) ?? asString(at(pull, 'head', 'sha')), review: { id, state: reviewStateFrom(review.state) }, }, @@ -374,13 +314,6 @@ export function normalizeWebhook(delivery: WebhookDelivery): NormalizedEvent[] { type EventBase = Pick<NormalizedEvent, 'id' | 'receivedAt' | 'repo' | 'raw'>; -/** - * §Ingress: "one `checks.completed` event per head sha". A suite has one head - * sha; the thread is the first pull request in `pull_requests` whose head is - * that sha, so a suite on a branch with no PR still produces one (thread-less) - * event. `failed` is filled from the payload's own check runs if it carries - * any and otherwise by {@link resolveCheckEvents}. - */ function normalizeCheckSuite(base: EventBase, payload: unknown, actor: EventActor | undefined): NormalizedEvent[] { const suite = asObject(at(payload, 'check_suite')); if (!suite) return []; @@ -398,8 +331,6 @@ function normalizeCheckSuite(base: EventBase, payload: unknown, actor: EventActo ...base, kind: 'checks.completed', thread: thread(base.repo, number, 'pull'), - // A check suite's `sender` is the App that produced it; the suite has no - // human actor, so an absent sender falls back to the App's slug. actor: actor ?? { login: asString(at(suite, 'app', 'slug')) ?? 'github', isBot: true }, html_url, sha, @@ -409,12 +340,9 @@ function normalizeCheckSuite(base: EventBase, payload: unknown, actor: EventActo } /** - * A single `check_run.completed`. Same kind as a suite (the contract names no - * other), keyed on the run's head sha, with the run itself pre-listed in - * `failed` when it failed — {@link resolveCheckEvents} then replaces that with the - * whole sha's failures. A deployment subscribed to both events sees two - * deliveries for one sha; they carry distinct delivery ids, so the dedup ring - * does not merge them. + * GitHub delivers `check_suite` and `check_run` separately, each with its own + * delivery id, so a deployment subscribed to both sees two deliveries for one + * sha. */ function normalizeCheckRun(base: EventBase, payload: unknown, actor: EventActor | undefined): NormalizedEvent[] { const run = asObject(at(payload, 'check_run')); @@ -443,20 +371,9 @@ function normalizeCheckRun(base: EventBase, payload: unknown, actor: EventActor ]; } -// ───────────────────────────────────────────────────────────────────────────── -// Poll rows (contract amendments 1 and 2a) -// ───────────────────────────────────────────────────────────────────────────── - /** - * True when `at` is at or after `since` — both ISO-8601 instants. - * - * Compared as instants and not as strings: GitHub stamps a row - * `2026-10-14T12:28:38Z` while a cursor is written with milliseconds - * (`2026-10-14T12:28:38.000Z`, or `.500Z` from an older release), and - * lexicographically `'Z'` (0x5A) sorts - * *after* `'.'` (0x2E) — so a string compare calls the earlier instant the - * later one. An unparseable stamp answers false, which is the fail-safe - * direction: the row keeps the kind it had before amendment 2a. + * GitHub stamps a row without milliseconds while a cursor carries them, and + * `'Z'` sorts after `'.'`, so these compare as instants, not as strings. */ export function atOrAfter(at: string, since: string): boolean { const a = Date.parse(at); @@ -465,57 +382,9 @@ export function atOrAfter(at: string, since: string): boolean { } /** - * An issue or pull-request row from `listIssuesSince`. - * - * A `since` query reports *that* a row changed, never *how* — there is no - * action — so amendment 2a derives the one from the row's own `created_at` - * against `since`, the start of the window the poller has not finished - * reading (`PollCursor.since` in types.ts): - * - * - **opened** when the row was created at or after `since` and the poller - * hasn't announced it as new already (`announced`): this pass is the - * first that could have seen it, so `issue.opened` / `pull.opened`. A - * row is read again inside its window after a stopped read, a refused - * delivery, or in the second a pass starts in, and `announced` keeps it - * from being opened twice. - * - otherwise an **issue** row is `issue.edited` (or `issue.closed`, below) - * when its state, title or body differs from what the last pass recorded - * for it ({@link polledMarker}), and is **dropped** when they do not. A - * comment bumps its issue's `updated_at`, and the comment row of the same - * pass already carries the change; re-announcing the issue beside it, - * attributed to the issue's author, woke the bot on its own comments - * (deviation D1 and the 2026-10-15 rehearsal in docs/e2e-report.md). - * - otherwise an open **pull-request** row is `pull.synchronize` when its - * head sha moved away from `previousHeadSha` (what the last pass recorded - * for it), and is **dropped** — null — when it did not. A pull request's - * row is touched by every comment on it, and those comment and - * review-comment rows are collected by the same pass and already carry - * the change; re-announcing the pull request beside each of them is the - * duplicate volume amendment 2a exists to stop. An unknown head (the - * adapter fetches at most `POLL_PR_HEAD_LIMIT` per pass) cannot be shown - * to have moved, so it drops the same way. - * - * A *first* sighting — no `previousHeadSha`, because this poll state has - * never recorded one — counts as moved, so an open pull request older - * than the cursor arrives once as `pull.synchronize` rather than not at - * all. It is the honest half of a choice with no clean answer: the row - * was not opened now (so `pull.opened` would lie about when), and - * dropping it would leave a cold start blind to work already in flight. - * The cost is bounded and smaller than it sounds: a poller with no stored - * cursor queries from one interval back (`Poller.pollOnce`), so a first - * pass sees only the rows touched in that interval — one `synchronize` per - * pull request *updated in the last poll interval*, not one per pull - * request that happens to be open. - * - **closed** rows are `issue.closed` / `pull.closed`, whenever they were - * created, but only on the pass that sees the close: a closed row whose - * marker is unchanged is a comment's bump and is dropped. The actor is - * `closed_by` when the list carries it, so the bot's own close is - * from-self. - * - * A first sighting — no recorded marker — always counts as a change. - * - * The dedup key still carries `updated_at`, so a row that does survive this - * arrives once per change. + * GitHub's `since` list says that a row changed, not how: it carries no + * action. A comment bumps its issue's or pull request's `updated_at` without + * changing the thread itself. */ export function normalizePolledIssue( repo: RepoRef, @@ -557,24 +426,12 @@ export function normalizePolledIssue( }; } -/** - * What the poller records per thread to tell a real change from a comment's - * bump: an open pull request's head sha (undefined when the adapter did not - * fetch it, which records nothing), `closed` for a closed pull request, and - * for an issue its state with a hash of its title and body. - */ export function polledMarker(row: PolledIssue): string | undefined { if (row.isPullRequest) return row.state === 'closed' ? 'closed' : row.headSha || undefined; const text = createHash('sha256').update(`${row.title}\0${row.body}`).digest('hex').slice(0, 16); return `issue:${row.state}:${text}`; } -/** - * An issue comment or a review comment from `listIssueCommentsSince` / - * `listReviewCommentsSince`. Same caveat as above: a poll cannot tell a new - * comment from an edited one, so both arrive as `*.created` with - * `updated_at` in the id. - */ export function normalizePolledComment( repo: RepoRef, row: PolledComment, @@ -598,8 +455,6 @@ export function normalizePolledComment( reviewComment: { id: row.id, path: row.path ?? '', - // A `since` comment row carries no line, side or diff hunk; the - // thread read (`listReviewThreads`) is where those live. line: null, side: 'RIGHT' as DiffSide, diffHunk: '', @@ -611,72 +466,18 @@ export function normalizePolledComment( }; } -/** - * The check lifecycle, in one place. - * - * A head sha can report its checks many times over — two workflows are two - * `check_suite.completed` deliveries, a deployment subscribed to `check_run` - * gets one per job, a poll pass sees whatever `getCheckRuns` says at that - * moment, and the webhook and the poller can be doing all of that at once — - * while §Ingress asks for one `checks.completed` per head sha. Four rules, - * and the thing that makes them hold under concurrency is the third: - * - * 1. **A head enters the lifecycle** when a pull-request row or a check - * webhook names it. - * 2. **It leaves when its checks are settled and the event was accepted.** - * Settled means every run on the sha has finished - * ({@link isCompletedSummary}); a sha with a suite still running is held - * back by {@link resolveCheckEvents} rather than reported with a - * provisional conclusion. Accepted means the listener took it without - * throwing — the poller keeps the head in its backlog until then. - * 3. **Every emission for one head sha is serialized.** {@link CheckGate} - * runs them one after another through a per-sha promise chain, whichever - * ingress path they came from. So an emission never has to reason about - * another one being in flight: by the time it runs, every earlier - * emission for its sha has already either committed a mark or failed. - * 4. **A mark is committed only after the listener accepted the event**, and - * an emission the committed mark already {@link coversMark covers} is - * dropped. A refused or thrown delivery commits nothing and leaves no - * trace, so the next emission for that sha — the retry, or the other - * path — still gets to deliver it. - * - * There is deliberately no reservation, no provisional entry and no rollback: - * the only writes are commits, and a commit means "the listener has this". - * Two earlier designs tried to make a claim-then-release state machine safe - * and each left a window where a refusal erased a newer accepted result or a - * pending claim hid work from the poller. - */ - -/** A {@link ChecksSummary} whose runs have all finished. */ export type CompletedChecksSummary = ChecksSummary & { conclusion: CheckConclusion }; -/** Have the runs on this sha finished, so that `checks.completed` is true? */ export function isCompletedSummary(summary: ChecksSummary): summary is CompletedChecksSummary { return summary.total > 0 && summary.conclusion !== null; } -/** - * The dedup key of a polled check result: - * `checks:<sha>:<conclusion>:<n>:<fingerprint of the failed job names>`. - * - * A check result has no `updated_at` to key on, so the third field is what - * makes the result distinct news instead — its conclusion and *which* jobs - * failed. A sha can legitimately settle twice: a suite created after the first - * poll adds its own failures, or replaces them with others of the same count - * (review round 4), and neither may look like a duplicate of the earlier - * result. The names are hashed so the key stays bounded. - */ export function checksPollKey(summary: CompletedChecksSummary): string { const names = summary.failed.map((f) => f.name).sort(); const fingerprint = createHash('sha1').update(names.join('\n')).digest('hex').slice(0, 12); return pollKey('checks', summary.sha, `${summary.conclusion}:${summary.failed.length}:${fingerprint}`); } -/** - * The check state of a polled pull-request head. The summary has to be - * *completed*: a head whose runs are still going belongs in the poller's - * backlog, not in a `checks.completed` event (rule 2). - */ export function normalizePolledChecks( repo: RepoRef, pull: ThreadRef, @@ -701,23 +502,8 @@ export function normalizePolledChecks( /** - * §Ingress: a `checks.completed` event carries "the failed job names and URLs - * (from check runs through `GitHubApi.getCheckRuns`)". The delivery that - * triggered it names at most the one suite or job that just finished, so the - * whole sha is looked up once per event — and that lookup is also what decides - * whether the event is ready at all (rule 2): - * - * - **settled** (every run finished): the summary replaces `checks`, so the - * event carries every failed job on the sha and not just this suite's. - * - **still running** (runs exist, no conclusion yet): the event is *held - * back*. Another suite is about to finish and GitHub will deliver again; - * emitting now would report a conclusion that is about to change and - * would spend the sha's one emission on a partial job list. - * - **no runs known** (`total === 0`) or **the lookup failed**: the event - * goes out as the payload had it. A check event without job names still - * wakes a recipe, and there is nothing better to wait for. - * - * Non-checks events pass through untouched. + * A check delivery names only the suite or job that just finished, so the + * whole sha's runs are read here. */ export async function resolveCheckEvents( events: readonly NormalizedEvent[], @@ -744,7 +530,6 @@ export async function resolveCheckEvents( if (isCompletedSummary(summary)) { out.push({ ...event, checks: { conclusion: summary.conclusion, failed: summary.failed } }); } else if (summary.total === 0) { - // The API knows of no runs on this sha, so the payload is all there is. out.push(event); } else { logger?.info('checks event held back: another suite is still running on this head', { @@ -756,17 +541,8 @@ export async function resolveCheckEvents( return out; } -// ───────────────────────────────────────────────────────────────────────────── -// One checks.completed per head sha: serialize, then commit -// ───────────────────────────────────────────────────────────────────────────── - -/** How many head shas the committed-mark ring remembers. */ export const CHECK_MARK_RING = 2000; -/** - * How bad a conclusion is, ascending. `null` (nothing settled yet) ranks - * below every real conclusion, so the first real one always gets through. - */ const CONCLUSION_SEVERITY: Record<CheckConclusion, number> = { success: 0, skipped: 1, @@ -782,43 +558,21 @@ function severity(conclusion: CheckConclusion | null): number { return conclusion === null ? -1 : CONCLUSION_SEVERITY[conclusion]; } -/** - * What a head sha has already told the listener — the *committed* record, only - * ever written after an accepted delivery. - * - * It accumulates rather than replaces: `rank` is the worst conclusion seen and - * `failed` the union of the job names reported. That is what makes an older - * result unable to undo a newer one, and it bounds how often one sha can be - * emitted (once per severity level, plus once per job name that appears). - */ export interface CheckMark { - /** {@link CONCLUSION_SEVERITY} of the conclusion; `-1` for none yet. */ rank: number; - /** Failed job names, sorted and deduplicated. */ failed: readonly string[]; } -/** The mark of a `checks.completed` event, or null for any other event. */ export function checkMarkOf(event: NormalizedEvent): CheckMark | null { if (event.kind !== 'checks.completed' || !event.sha) return null; const names = [...new Set((event.checks?.failed ?? []).map((f) => f.name))].sort(); return { rank: severity(event.checks?.conclusion ?? null), failed: names }; } -/** - * Does what the listener already has say everything `mark` would say? - * - * True when the committed conclusion is at least as bad and names at least the - * same failed jobs — then `mark` is a duplicate, a weaker re-run, or a shorter - * view of the same failures, and emitting it would only repeat the sha. False - * when `mark` is a stronger conclusion or names a job the listener has not - * heard about, which is the case that must supersede. - */ export function coversMark(committed: CheckMark, mark: CheckMark): boolean { return committed.rank >= mark.rank && mark.failed.every((name) => committed.failed.includes(name)); } -/** The worst conclusion and every failed job name either mark knows. */ function mergeMarks(a: CheckMark, b: CheckMark): CheckMark { return { rank: Math.max(a.rank, b.rank), @@ -826,52 +580,24 @@ function mergeMarks(a: CheckMark, b: CheckMark): CheckMark { }; } -/** - * The gate that makes "one `checks.completed` per head sha" true across every - * ingress path, keyed on `<owner>/<name>@<sha>`. - * - * Two pieces, and neither of them is a state machine: - * - * - **a per-sha promise chain**, so all emissions for one head sha run one - * after another. A webhook delivery and a poll pass for the same sha do - * not race; the second one simply waits its turn. - * - **a bounded ring of committed marks**, written only after a delivery was - * accepted. Nothing is ever written speculatively, so there is nothing to - * release and no window in which a refusal can erase what another - * emission already got accepted. - */ export class CheckGate { private readonly committed = new Map<string, CheckMark>(); - /** The tail of each head sha's queue. Absent once the queue has drained. */ private readonly queues = new Map<string, Promise<unknown>>(); constructor(private readonly capacity: number = CHECK_MARK_RING) {} - /** `<owner>/<name>@<sha>`. */ static key(repo: RepoRef, sha: string): string { return `${repo.owner}/${repo.name}@${sha}`; } - /** What the listener has already accepted for this head sha, if anything. */ committedMark(repo: RepoRef, sha: string): CheckMark | undefined { return this.committed.get(CheckGate.key(repo, sha)); } - /** - * Deliver one checks event for this head sha, in turn. - * - * Waits for every emission already queued for the sha, then compares `mark` - * against what those emissions committed. Returns false without calling - * `deliver` when the committed mark already covers it. Otherwise calls - * `deliver`, commits `mark` once it resolves, and returns true — or lets its - * rejection through untouched, having committed nothing. - */ async emit(repo: RepoRef, sha: string, mark: CheckMark, deliver: () => Promise<void>): Promise<boolean> { const key = CheckGate.key(repo, sha); const run = async (): Promise<boolean> => { - // Read inside the chain: every earlier emission for this sha has - // settled by now, so this sees their commits and none of their attempts. const seen = this.committed.get(key); if (seen !== undefined && coversMark(seen, mark)) return false; await deliver(); @@ -880,27 +606,17 @@ export class CheckGate { }; const ahead = this.queues.get(key); - // `then(run, run)` on both outcomes: an emission that was refused must not - // stall the queue behind it — that refusal is exactly when the next one - // needs to run. const mine: Promise<boolean> = ahead === undefined ? run() : ahead.then(run, run); this.queues.set(key, mine); try { return await mine; } finally { - // Drop the queue when nothing is waiting behind this emission, so a - // quiet sha costs nothing and the map cannot grow without bound. if (this.queues.get(key) === mine) this.queues.delete(key); } } private commit(key: string, mark: CheckMark): void { - // Re-read rather than trusting the value `run` saw: merging with whatever - // is committed now means an emission can only ever add to the record, - // never lower it. const current = this.committed.get(key); - // Re-setting an existing key keeps its insertion position, so a sha that - // is emitted again does not get a fresh lease on the ring. this.committed.set(key, current === undefined ? mark : mergeMarks(current, mark)); if (this.committed.size > this.capacity) { const oldest = this.committed.keys().next(); @@ -908,12 +624,10 @@ export class CheckGate { } } - /** Head shas with a committed mark. */ get size(): number { return this.committed.size; } - /** Head shas with an emission in flight or queued. */ get inFlight(): number { return this.queues.size; } diff --git a/src/poller.ts b/src/poller.ts index 00ff107..8164f00 100644 --- a/src/poller.ts +++ b/src/poller.ts @@ -1,28 +1,6 @@ /** - * Polling ingress (TASK-SRHKG.1.3, §Ingress, as amended by contract - * amendment 1). - * - * The contract's first draft polled `GET /notifications`; an App installation - * token cannot read that endpoint, so the fallback asks the repository what - * changed since its cursor instead — `listIssuesSince`, - * `listIssueCommentsSince`, `listReviewCommentsSince` — and derives the check - * state of the open pull-request heads it saw from `getCheckRuns`. - * - * Two properties the acceptance criteria name, and where they live: - * - * - **The interval floor.** `effectiveIntervalMs` is never below - * `MIN_POLL_INTERVAL_MS` (60 s) and never below GITHUB_POLL_INTERVAL_MS; - * a server-sent `X-Poll-Interval` can only raise it, through - * {@link Poller.observePollInterval}. - * - **Record only after delivery.** The dedup ring is read once per pass - * and a key is written back only once its event has been handed to the - * listener without throwing, so a failed delivery is re-attempted on the - * next pass instead of being silently consumed. The cursors advance for - * the same reason: only when the whole pass delivered. - * - * Each repository's three lists keep a cursor of their own - * ({@link PollCursor}), so a list whose read the page bound stopped reads on - * from where it stopped, while the rest move to the start of the pass. + * An App installation token can't read `GET /notifications`, so polling asks + * each repository what changed since its cursor. */ import { @@ -56,127 +34,61 @@ import { } from './types.js'; import type { EventListener } from './webhook.js'; -/** Amendment 1: "at most 20 PRs per poll" get their check runs fetched. */ export const MAX_CHECKED_PULLS_PER_POLL = 20; +const HEAD_BACKLOG_DRAIN_PASSES = 10; -/** - * How many pull-request heads may wait for a check lookup at once. - * - * The backlog exists because the issue cursor is not a work queue: it moves - * forward whether or not the check lookups it implied succeeded, so a head - * whose lookup failed, or that the {@link MAX_CHECKED_PULLS_PER_POLL} cap cut - * off, or whose runs had not finished yet, would otherwise be dropped and - * never asked about again. At the 60 s floor and 20 lookups a pass, this many - * heads drain in ten minutes; past it the oldest head is forgotten, which - * bounds the backlog against a repository that updates pull requests faster - * than the poller can check them. - */ -export const MAX_HEAD_BACKLOG = 200; +export const MAX_HEAD_BACKLOG = HEAD_BACKLOG_DRAIN_PASSES * MAX_CHECKED_PULLS_PER_POLL; -/** - * How many passes a head may report nothing — a failing lookup, or a sha with - * no check runs at all — before the poller gives up on it. A head whose runs - * are merely still going does not spend an attempt: that is progress, and CI - * is allowed to take longer than ten minutes. - */ export const MAX_HEAD_ATTEMPTS = 10; -/** The part of {@link GitHubApi} the poller uses. */ export type PollerApi = Pick< GitHubApi, 'listIssuesSince' | 'listIssueCommentsSince' | 'listReviewCommentsSince' | 'getCheckRuns' >; -/** - * The durable state the poller needs: the cursors, the dedup ring and the last - * polled head of each open pull request (amendment 2a). A structural subset of - * `StateStore` (types.ts), which .7 implements over GITHUB_STATE_FILE — - * `snapshot()` is in the interface so that the ring can be *read* without - * being written to, which is what lets a key be recorded only after its event - * was delivered. - */ export interface IngressState { lastPollAt(): string | null; setLastPollAt(when: string): void; - /** Each list's own cursor, keyed by {@link pollCursorKey}. */ pollCursors(): Readonly<Record<string, PollCursor>>; - /** Replace them all: a list left out starts from `lastPollAt` next time. */ setPollCursors(cursors: Record<string, PollCursor>): void; - /** Per list, the rows announced as new in its window, id → created_at. */ polledNew(): Readonly<Record<string, Record<string, string>>>; setPolledNew(rows: Record<string, Record<string, string>>): void; - /** Records `key` in the ring and reports whether it was already there. */ seenPollKey(key: string): boolean; - /** The head sha the last pass recorded for this pull request, or null. */ polledHead(channel: string): string | null; setPolledHead(channel: string, sha: string): void; - /** Read-only view; the poller reads `pollKeys` once per pass. */ snapshot(): { pollKeys: string[] }; } export interface PollerOptions { api: PollerApi; - /** GITHUB_REPOS. The poller only ever asks these repositories. */ repos: readonly RepoRef[]; state: IngressState; - /** GITHUB_POLL_INTERVAL_MS. 0 means polling is off and `start()` is a no-op. */ intervalMs: number; onEvent: EventListener; logger?: Logger; - /** Overridable in tests. */ now?: () => Date; maxCheckedPulls?: number; maxHeadBacklog?: number; maxHeadAttempts?: number; - /** - * The "one `checks.completed` per head sha" gate. Pass the webhook - * listener's own gate when both ingress paths are live, so an emission from - * one path is serialized against the other instead of racing it. - */ checkGate?: CheckGate; } -/** The three lists the poller reads from each repository. */ export type PollList = 'issues' | 'issue_comments' | 'review_comments'; -/** `<owner>/<name>:<list>`, the key of a list's cursor in the state. */ export function pollCursorKey(repo: RepoRef, list: PollList): string { return `${repoSlug(repo)}:${list}`; } -/** What one pass did, for the caller's diagnostics and for the tests. */ export interface PollResult { - /** - * The start of a list with no cursor of its own: the stored `lastPollAt` - * as kept, or one interval back in whole seconds. The list asks GitHub from - * its whole second. - */ since: string; - /** Events handed to the listener, in order. */ delivered: NormalizedEvent[]; - /** Candidates whose key was already in the ring. */ duplicates: number; - /** Events whose listener threw; their keys were not recorded. */ failed: number; - /** - * Whether the cursors moved: each list's to the start of this pass, or, for - * a read the page bound stopped, to where it stopped. - */ cursorAdvanced: boolean; - /** `getCheckRuns` calls this pass made; never above the cap. */ checkLookups: number; - /** Heads still waiting for a conclusion when the pass ended. */ checksBacklog: number; } -/** - * A pull-request head waiting for its check runs to settle. Held across - * passes, keyed `<owner>/<name>@<sha>`, independent of the issue cursor. - */ -/** - * What became of one hand-off: the listener took it, or another accepted - * emission for the same head sha already covered it. A refusal throws. - */ type HandOff = 'delivered' | 'covered'; interface PendingHead { @@ -184,44 +96,16 @@ interface PendingHead { sha: string; number: number; html_url: string; - /** The row's `updated_at`, kept so a check candidate sorts chronologically. */ at: string; - /** Passes that reported nothing; {@link MAX_HEAD_ATTEMPTS} gives up. */ attempts: number; } interface Candidate { key: string; event: NormalizedEvent; - /** Sort key: the row's `updated_at`, so a pass delivers chronologically. */ at: string; - /** - * The backlog entry this candidate settles, if any. The head is retired - * only once the listener has accepted the event (or the ring confirms it was - * already delivered): a head removed at lookup time would be unrecoverable - * once the cursor had moved past the pull-request row that queued it. - */ retires?: string; - /** - * The pull-request head this candidate's event announces, committed to - * `pollHeads` on exactly the same terms as {@link Candidate.retires}: - * **only once the listener has accepted the event.** - * - * Writing it at collection time lost a refused `pull.synchronize` for good - * (review round 2, finding 1). The cursor is held back by the refusal, so - * the row is queried again — but the next pass compared it against the head - * already saved, found no change, dropped the row, and let the cursor move - * on. The commits were never announced and nothing retried them, which is - * exactly the guarantee {@link Poller.pollOnce} documents. A row that - * produced *no* event is different: its head did not move, so recording it - * at collection time loses nothing. - */ head?: { channel: string; sha: string }; - /** - * An issue row announced as new, remembered in `pollNew` on the same terms - * as {@link Candidate.head}, so a refused pass's accepted openings stay - * remembered when the retry reads them again. - */ fresh?: { list: string; id: string; createdAt: string }; } @@ -234,7 +118,6 @@ function wholeSecond(ms: number): string { return new Date(Math.floor(ms / 1000) * 1000).toISOString(); } -/** A stored position as a whole second, whatever spelling or precision it was kept in. */ function atSecond(iso: string): string { return wholeSecond(Date.parse(iso)); } @@ -251,13 +134,7 @@ export class Poller { private readonly maxHeadBacklog: number; private readonly maxHeadAttempts: number; private readonly checkGate: CheckGate; - /** - * Heads waiting for a conclusion, oldest first. Insertion order is the - * drain order, and a head the pass could not settle is moved to the back, - * so a backlog longer than the per-pass cap still drains round-robin. - */ private readonly backlog = new Map<string, PendingHead>(); - /** The last `X-Poll-Interval` the API reported, in ms. Can only raise. */ private serverIntervalMs = 0; private timer: NodeJS.Timeout | undefined; private running = false; @@ -277,29 +154,18 @@ export class Poller { this.checkGate = opts.checkGate ?? new CheckGate(); } - /** Heads still waiting for their check runs to settle. */ get checkBacklogSize(): number { return this.backlog.size; } - /** GITHUB_POLL_INTERVAL_MS = 0: polling off. */ get enabled(): boolean { return this.configuredIntervalMs > 0; } - /** - * The delay the next pass waits. The 60 s floor and GITHUB_POLL_INTERVAL_MS - * are both floors, and a server-sent interval can only push it up. - */ get effectiveIntervalMs(): number { return Math.max(MIN_POLL_INTERVAL_MS, this.configuredIntervalMs, this.serverIntervalMs); } - /** - * Honour an `X-Poll-Interval` response header (seconds). Returns the - * interval now in force. A value that would poll faster than the floor is - * recorded but cannot lower it. - */ observePollInterval(value: string | number | null | undefined): number { const seconds = typeof value === 'string' ? Number(value.trim()) : typeof value === 'number' ? value : NaN; if (!Number.isFinite(seconds) || seconds <= 0) return this.effectiveIntervalMs; @@ -314,7 +180,6 @@ export class Poller { return this.effectiveIntervalMs; } - /** Start the loop. The first pass runs after one interval, not immediately. */ start(): void { if (!this.enabled || this.running) return; this.running = true; @@ -329,7 +194,6 @@ export class Poller { } } - /** Resolve once the in-flight pass, if any, has finished. */ async whenIdle(): Promise<void> { await this.pass; } @@ -344,53 +208,26 @@ export class Poller { .finally(() => this.schedule()); this.pass = pass; }, this.effectiveIntervalMs); - // A poll timer must never be the reason the process stays alive. this.timer.unref?.(); } - /** - * One pass: query every allow-listed repository's lists since their cursors, drain as - * much of the check backlog as the per-pass cap allows, drop what the dedup - * ring already holds, deliver the rest oldest-first, record each key after - * its delivery, and advance the cursors if no delivery failed. - * - * A failed *lookup* does not hold the cursors back: the head stays in the - * backlog instead, which is the thing that gets retried. A failed - * *delivery* does hold it back, so its row is queried again. - * - * For that retry to mean anything, nothing a refused event would be - * recognised by on the next pass may be written before it is accepted. - * Three things are therefore committed inside the delivery loop and nowhere - * else: the dedup key, the pull-request head a `pull.synchronize` announces - * ({@link Candidate.head}), and a row announced as new ({@link Candidate.fresh}). - */ async pollOnce(): Promise<PollResult> { const startedAt = this.now(); const stored = this.state.lastPollAt(); const since = stored ?? wholeSecond(startedAt.getTime() - this.effectiveIntervalMs); - // A first pass keeps the window it reads before it reads, so however the - // pass ends, refused or thrown, the next asks from the same start rather - // than one the clock has moved on. if (stored === null) this.state.setLastPollAt(since); const receivedAt = startedAt.toISOString(); - // Where a list read to the end starts next: this pass's start, so a row - // changed after its read is asked for, in whole seconds, so its second is too. const startSecond = wholeSecond(startedAt.getTime()); const candidates: Candidate[] = []; - // Where each list reads from next, written only if the pass delivers. A - // repository no longer in GITHUB_REPOS drops out of it. const cursors: Record<string, PollCursor> = {}; for (const repo of this.repos) { candidates.push(...(await this.collectRows(repo, since, startSecond, receivedAt, cursors))); } - // One drain for every repository together, so the cap is a cap on the - // pass and not on each repository in turn. const checks = await this.drainHeadBacklog(receivedAt); candidates.push(...checks.candidates); candidates.sort((a, b) => (a.at === b.at ? a.key.localeCompare(b.key) : a.at.localeCompare(b.at))); - // One read of the ring per pass: the write happens after delivery. const alreadySeen = new Set(this.state.snapshot().pollKeys); const result: PollResult = { since, @@ -399,20 +236,14 @@ export class Poller { failed: 0, cursorAdvanced: false, checkLookups: checks.lookups, - // Filled in after the delivery loop: a head is only retired once its - // event has been accepted, so the size before the loop means nothing. checksBacklog: 0, }; const deliveredThisPass = new Set<string>(); for (const candidate of candidates) { - // A head leaves the backlog once this event is known to be delivered — - // by this pass, or by an earlier one, or by the webhook path. const retire = (): void => { if (candidate.retires !== undefined) this.backlog.delete(candidate.retires); }; - // Committed on exactly the terms `retire` is: the listener took the - // event, or the ring says an earlier pass already got it through. const commitHead = (): void => { if (candidate.head) this.state.setPolledHead(candidate.head.channel, candidate.head.sha); if (candidate.fresh) { @@ -434,9 +265,6 @@ export class Poller { try { outcome = await this.hand(candidate.event); } catch (err) { - // Nothing is recorded and nothing is retired: the key stays out of - // the dedup ring, the head stays in the backlog, and the cursors are - // held back below, so the next pass tries again. result.failed += 1; this.logger.warn('poll event delivery failed; its key stays unrecorded for the next pass', { key: candidate.key, @@ -446,8 +274,6 @@ export class Poller { } if (outcome === 'covered') { - // Another emission for this head sha was accepted and said everything - // this one would have, so the head is done — not waiting on anybody. result.duplicates += 1; retire(); this.logger.info('poll checks result dropped: this head sha already reported this result or a stronger one', { @@ -457,7 +283,6 @@ export class Poller { continue; } - // Recorded only now, after the listener took it. this.state.seenPollKey(candidate.key); deliveredThisPass.add(candidate.key); result.delivered.push(candidate.event); @@ -469,12 +294,8 @@ export class Poller { if (result.failed === 0) { this.state.setPollCursors(cursors); - // Every list has delivered what changed before this pass's start, or - // before where a stopped read stopped, whichever is earlier. const stops = Object.values(cursors).flatMap((cursor) => cursor.resumeAt ?? []); this.state.setLastPollAt([startSecond, ...stops].reduce((a, b) => (Date.parse(b) < Date.parse(a) ? b : a))); - // A row stays remembered as announced new while it is created inside its - // list's window, the only rows the window's start would call new again. const remembered: Record<string, Record<string, string>> = {}; for (const [list, rows] of Object.entries(this.state.polledNew())) { const window = cursors[list]?.since; @@ -489,7 +310,6 @@ export class Poller { }); } this.logger.debug('poll pass done', { - // Where a list with no cursor of its own started; the others started at their own. fallbackSince: since, delivered: result.delivered.length, duplicates: result.duplicates, @@ -500,19 +320,6 @@ export class Poller { return result; } - // ── One repository's rows ── - - /** - * The issue, comment and review-comment rows one repository changed since - * each list's cursor. Every open pull-request head it saw is *enqueued* for - * a check lookup rather than looked up here, so the cap and the retries - * belong to the backlog and not to one repository's turn. - * - * Each list's next cursor goes into `next`: the start of this pass when - * the read reached the end of the list, and otherwise where it stopped, - * with the window's start kept, since the rows past the stop are still - * to be read. - */ private async collectRows( repo: RepoRef, fallback: string, @@ -523,8 +330,6 @@ export class Poller { const cursors = this.state.pollCursors(); const read = async <T>(list: PollList, ask: (from: string) => Promise<PollRead<T>>) => { const key = pollCursorKey(repo, list); - // Stored positions are read as whole seconds, so one kept with - // milliseconds, as by a release before this, starts at its second. const since = atSecond(cursors[key]?.since ?? fallback); const stop = cursors[key]?.resumeAt; const { rows, resumeAt } = await ask(stop === undefined ? since : atSecond(stop)); @@ -536,9 +341,6 @@ export class Poller { const issues = await read('issues', (from) => this.api.listIssuesSince(repo, from)); const announced = this.state.polledNew()[issues.key] ?? {}; for (const row of issues.rows) { - // What the last pass recorded for this thread (amendment 2a for a pull - // request's head, a state-and-text fingerprint for an issue) turns a - // real change into an event and drops the row a comment only bumped. const channel = channelId({ repo, number: row.number, kind: row.isPullRequest ? 'pull' : 'issue' }); const seen = announced[String(row.id)] !== undefined; const event = normalizePolledIssue(repo, row, receivedAt, issues.since, this.state.polledHead(channel) ?? undefined, seen); @@ -546,11 +348,6 @@ export class Poller { const marker = polledMarker(row); const openHead = row.isPullRequest && row.state === 'open' && row.headSha ? row.headSha : undefined; if (event) { - // The marker rides with the candidate and is committed on acceptance, - // so a refusal leaves the poller still believing the old one and the - // next pass announces the change again (finding 1). Deferring it also - // means a `listIssueCommentsSince` that throws later in this very - // method cannot leave a marker advanced for a pass that never delivered. out.push({ key: pollKey('issue', row.id, row.updatedAt), event, @@ -559,11 +356,6 @@ export class Poller { ...(fresh ? { fresh: { list: issues.key, id: String(row.id), createdAt: row.createdAt } } : {}), }); } else if (marker !== undefined) { - // No event: this row was dropped *because* the marker is unchanged, so - // what is written here is the value already stored. It costs nothing - // and refreshes the entry's place in the eviction order, which is why - // a long-lived thread is not evicted ahead of ones nobody has looked - // at since. this.state.setPolledHead(channel, marker); } if (openHead !== undefined) this.enqueueHead(repo, row); @@ -577,16 +369,11 @@ export class Poller { return out; } - // ── The check backlog ── - - /** Remember a head to check. An already-queued head keeps its place. */ private enqueueHead(repo: RepoRef, row: PolledIssue): void { const sha = row.headSha as string; const key = CheckGate.key(repo, sha); const queued = this.backlog.get(key); if (queued) { - // Same head seen again: refresh what the event will say, keep the - // queue position and the attempt count. queued.at = row.updatedAt; queued.html_url = row.html_url; queued.number = row.number; @@ -612,12 +399,6 @@ export class Poller { } } - /** - * Ask about as many waiting heads as the per-pass cap allows, oldest first. - * A head that settled leaves the backlog with a candidate; a head that did - * not is moved to the back, so the next pass starts with someone else and a - * long backlog drains round-robin instead of starving behind the same 20. - */ private async drainHeadBacklog(receivedAt: string): Promise<{ candidates: Candidate[]; lookups: number }> { const due = [...this.backlog.values()].slice(0, this.maxCheckedPulls); if (this.backlog.size > due.length) { @@ -642,21 +423,14 @@ export class Poller { if (!isCompletedSummary(summary)) { if (summary.total === 0) { - // Nothing is running on this sha and nothing has run: it may be a - // head no workflow matches, so it ages out rather than waiting. this.retainHead(key, head, 'the head has no check runs'); } else { - // Runs exist and have not finished: `checks.completed` would be a - // lie, so wait — and do not spend an attempt on real progress. this.rotateHead(key, head); this.logger.debug('poll head still running its checks', { sha: head.sha, runs: summary.total }); } continue; } - // Settled — but the head stays queued until the delivery loop retires - // it, so a refused event can be retried even after the cursors have moved - // past the pull-request row that queued this head (rule 2). this.rotateHead(key, head); const pull: ThreadRef = { repo: head.repo, number: head.number, kind: 'pull' }; candidates.push({ @@ -669,12 +443,6 @@ export class Poller { return { candidates, lookups }; } - /** - * Hand one event to the listener. A `checks.completed` event goes through - * the gate, which serializes it against every other emission for its head - * sha and reports whether what the listener already accepted covers it; any - * other event goes straight out. A refusal propagates. - */ private async hand(event: NormalizedEvent): Promise<HandOff> { const mark = checkMarkOf(event); if (mark === null) { @@ -687,7 +455,6 @@ export class Poller { return delivered ? 'delivered' : 'covered'; } - /** Keep a head that reported nothing, until it has used up its attempts. */ private retainHead(key: string, head: PendingHead, why: string, err?: unknown): void { head.attempts += 1; const fields = { @@ -706,7 +473,6 @@ export class Poller { this.logger.debug('poll will ask about this head again next pass', fields); } - /** Move to the back of the queue: a Map keeps insertion order. */ private rotateHead(key: string, head: PendingHead): void { this.backlog.delete(key); this.backlog.set(key, head); diff --git a/src/state.ts b/src/state.ts index e973be5..2557b7c 100644 --- a/src/state.ts +++ b/src/state.ts @@ -1,35 +1,8 @@ /** - * Durable state and rollback bookkeeping (TASK-SRHKG.1.7). - * - * The contract's state bullet, and SPEC 0.5 §8: - * - * - a JSON file at GITHUB_STATE_FILE, written atomically (temp file plus - * rename), carrying a `version` so a future shape change is detectable; - * - checkpoint lineage `{ id, parent, createdAt }`, a tree: rolling back and - * writing again branches (§8.2), exactly as slack-mcpl's `StateTracker` - * does, with the lineage made durable and the bookkeeping widened from - * "messages sent" to "comments created, edited and deleted"; - * - per checkpoint: the comments this server created, the previous bodies of - * the comments it edited, and the comments it deleted; - * - `rollback(checkpointId)` computes what has to be undone (delete the - * created, restore the edited) and returns null for an unknown checkpoint - * so the caller can answer -32005 (§8.7); - * - the polling cursors, one per repository and list in `pollCursors` with - * `lastPollAt` where a list with none of its own starts, the rows - * announced as new while inside their list's window (`pollNew`), the - * dedup ring of the last {@link POLL_DEDUP_RING} poll keys (contract - * amendment 1), and the last - * polled head sha of each open pull request, bounded by - * {@link POLL_HEAD_MEMORY} (amendment 2a). - * - * **Which checkpoint a write lands under.** A tool records its write under - * `current()`, and the routing layer creates a checkpoint *after* the call and - * reports it in `state` (§8.2: the checkpoint names the state *including* that - * write, which is slack-mcpl's order too). So the bucket keyed by checkpoint C - * holds the writes that happened while C was current — the writes that came - * *after* C was created. Rolling back to C therefore undoes C's own bucket and - * the buckets of every checkpoint descending from C, and leaves everything - * recorded before C existed ({@link ROOT_BUCKET}) alone. + * MCPL checkpoints form a tree: rolling back and then writing again branches, + * and a checkpoint names the state including the write reported with it. So a + * write is recorded under the checkpoint current when it's made, and a + * rollback to C undoes what is recorded under C and its descendants. */ import { randomUUID } from 'node:crypto'; @@ -52,40 +25,30 @@ import { type ThreadKind, } from './types.js'; -/** The `version` field of the on-disk JSON. Bumped when the shape changes. */ export const STATE_VERSION = 1; /** - * The bucket writes land in before any checkpoint exists. No rollback touches - * it: a write that preceded every checkpoint is part of the state the oldest - * checkpoint names, so there is nothing to undo it back to. + * Writes made before any checkpoint. The oldest checkpoint's state includes + * them, so no rollback undoes them. */ export const ROOT_BUCKET = 'root'; -/** The thread a recorded comment hangs on, so a receipt can name it. */ export interface ThreadLocator { number: number; kind: ThreadKind; } -/** - * {@link CreatedComment} plus the thread it was created on (the contract's - * "created comment ids (with thread)"). The delete path needs only - * `repo`/`commentId`/`review`; the thread is what the rollback receipt prints. - */ export interface CreatedCommentRecord extends CreatedComment { thread?: ThreadLocator; } -/** {@link EditedComment} plus its thread, for the same reason. */ export interface EditedCommentRecord extends EditedComment { thread?: ThreadLocator; } /** - * A comment this server deleted after a checkpoint. GitHub has no undelete, so - * rollback can only report it — the body is kept so the receipt can say what - * was lost without the host having to ask GitHub. + * GitHub has no undelete, so rollback can only report a deleted comment; its + * body is kept for the receipt. */ export interface DeletedCommentRecord { repo: string; @@ -95,26 +58,14 @@ export interface DeletedCommentRecord { thread?: ThreadLocator; } -/** - * Re-exported from types.ts, which WP9 made the home of the status so - * {@link StateStore.checkpointStatus} could be declared on the one state seam. - */ export type { CheckpointStatus }; -/** {@link RollbackPlan} plus the deletes that cannot be undone at all. */ export interface CommentRollbackPlan extends RollbackPlan { created: CreatedCommentRecord[]; edited: EditedCommentRecord[]; deleted: DeletedCommentRecord[]; } -/** - * The seam this package installs: {@link StateStore} widened by the two things - * `types.ts` could not carry — the deletes (which rollback can only report) and - * the richer plan {@link GithubState.rollback} returns. `ToolContext.state` is - * typed `StateStore`, so the comment tools narrow to this; .9 installs - * {@link GithubState} and nothing else implements it. - */ export interface GithubStateStore extends StateStore { recordCreatedComment(comment: CreatedCommentRecord): void; recordEditedComment(comment: EditedCommentRecord): void; @@ -124,7 +75,6 @@ export interface GithubStateStore extends StateStore { snapshot(): GithubStateSnapshot; } -/** The on-disk JSON: {@link StateSnapshot} plus `version` and the deletes. */ export interface GithubStateSnapshot extends StateSnapshot { version: number; created: Record<string, CreatedCommentRecord[]>; @@ -148,16 +98,8 @@ export function emptySnapshot(): GithubStateSnapshot { }; } -/** - * The {@link StateStore} the tools and the poller see, over an in-memory - * snapshot that {@link GithubState.flush} writes atomically. - * - * Without a state file the store still works for the life of the process and - * `flush()` is a no-op (contract notes: GITHUB_STATE_FILE is optional). - */ export class GithubState implements GithubStateStore { private snap: GithubStateSnapshot; - /** Serializes flushes: two renames must not interleave. */ private writing: Promise<void> = Promise.resolve(); private constructor( @@ -168,16 +110,14 @@ export class GithubState implements GithubStateStore { this.snap = snap; } - /** A fresh store. `file` null keeps the state in memory only. */ static empty(file: string | null = null, logger: Logger = silentLogger()): GithubState { return new GithubState(file, emptySnapshot(), logger); } /** - * Load the state file, or start empty when it is absent, unreadable, not - * JSON, or written by a version this build does not know. A damaged file is - * never a startup failure: the bookkeeping is this server's own, and losing - * it costs rollback reach, not host data. One stderr line names the file. + * A missing state file starts empty. One that can't be read, isn't JSON or + * isn't a snapshot this version reads is reported on stderr, and the server + * starts empty instead of failing to start. */ static load(file: string | null, logger: Logger = silentLogger()): GithubState { if (!file) return GithubState.empty(null, logger); @@ -208,8 +148,6 @@ export class GithubState implements GithubStateStore { return new GithubState(file, snap, logger); } - // ── Checkpoints (§8.2) ── - createCheckpoint(): CheckpointRecord { const record: CheckpointRecord = { id: `chk_${randomUUID().slice(0, 8)}`, @@ -225,13 +163,10 @@ export class GithubState implements GithubStateStore { return this.snap.current; } - /** Every checkpoint, in creation order; `parent` carries the tree. */ lineage(): CheckpointRecord[] { return this.snap.checkpoints.map((c) => ({ ...c })); } - // ── Write bookkeeping ── - recordCreatedComment(comment: CreatedCommentRecord): void { this.bucket(this.snap.created).push({ ...comment }); } @@ -251,11 +186,6 @@ export class GithubState implements GithubStateStore { return list; } - // ── Rollback (§8.5) ── - - /** - * Is `checkpointId` still rollable? See {@link CheckpointStatus}. - */ checkpointStatus(checkpointId: string): CheckpointStatus { if (!this.snap.checkpoints.some((c) => c.id === checkpointId)) return 'unknown'; const parentOf = new Map(this.snap.checkpoints.map((c) => [c.id, c.parent])); @@ -265,28 +195,8 @@ export class GithubState implements GithubStateStore { return 'abandoned'; } - /** - * What undoing back to `checkpointId` means, newest action first, and null - * when the checkpoint is not `live` — unknown, or abandoned by an earlier - * rollback (the caller answers -32005, §8.7). - * - * Refusing an abandoned checkpoint is the honest answer, not a limitation - * worth hiding: once a rollback moved `current` off a branch, that branch's - * undo records are spent and this server keeps no redo records, so it cannot - * put the comments back the way that checkpoint named them. Accepting it - * would report a restoration that did not happen. - * - * The buckets consumed are the checkpoint's own and every descendant's; they - * are cleared and `current` moves to the checkpoint, so the next write - * branches from it (§8.2) and a second rollback to the same checkpoint is a - * no-op rather than a double undo. An action the caller could not carry out - * books itself again (it records under `current`, which is now the - * checkpoint), so a retry sees it and cannot claim a completed rollback. - */ rollback(checkpointId: string): CommentRollbackPlan | null { if (this.checkpointStatus(checkpointId) !== 'live') return null; - // Newest first: creation order reversed, so an edit that followed another - // edit of the same comment is restored before the older body overwrites it. const ids = this.subtreeIds(checkpointId).reverse(); const plan: CommentRollbackPlan = { created: [], edited: [], deleted: [] }; for (const id of ids) { @@ -301,7 +211,6 @@ export class GithubState implements GithubStateStore { return plan; } - /** `checkpointId` and every checkpoint descending from it, creation order. */ private subtreeIds(checkpointId: string): string[] { const parentOf = new Map(this.snap.checkpoints.map((c) => [c.id, c.parent])); const inSubtree = (id: string): boolean => { @@ -313,8 +222,6 @@ export class GithubState implements GithubStateStore { return this.snap.checkpoints.filter((c) => inSubtree(c.id)).map((c) => c.id); } - // ── Polling cursor and dedup ring (amendment 1) ── - lastPollAt(): string | null { return this.snap.lastPollAt; } @@ -352,14 +259,6 @@ export class GithubState implements GithubStateStore { return this.snap.pollHeads[channel] ?? null; } - /** - * Record a pull request's head sha, bounded by {@link POLL_HEAD_MEMORY}. - * - * Insertion order is the eviction order, so re-recording an unchanged head - * deletes the key before setting it again: otherwise a long-lived pull - * request polled every minute would keep its original (now oldest) slot and - * be evicted ahead of heads nobody has looked at since. - */ setPolledHead(channel: string, sha: string): void { delete this.snap.pollHeads[channel]; this.snap.pollHeads[channel] = sha; @@ -369,19 +268,10 @@ export class GithubState implements GithubStateStore { } } - // ── Persistence ── - snapshot(): GithubStateSnapshot { return structuredClone(this.snap); } - /** - * Write the snapshot atomically: a uniquely named temp file beside the state - * file, fsync'd, then renamed over it. A crash before the rename leaves the - * previous state file intact and a stray `.tmp-*` beside it, which - * {@link GithubState.load} never reads; a crash after it leaves the new - * state. No reader ever sees a half-written file. - */ async flush(): Promise<void> { const file = this.file; if (!file) return; @@ -398,17 +288,11 @@ export class GithubState implements GithubStateStore { } await rename(tmp, file); }); - // Keep the chain alive past a failed write, but let the caller see the error. this.writing = run.catch(() => undefined); await run; } } -/** - * Accept a parsed state file only when it is this version and has the shape; - * missing optional parts are filled in, anything else is rejected (null) so the - * caller starts empty rather than running on a half-understood file. - */ function normalize(parsed: unknown): GithubStateSnapshot | null { if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) return null; const raw = parsed as Partial<GithubStateSnapshot>; @@ -422,29 +306,18 @@ function normalize(parsed: unknown): GithubStateSnapshot | null { snap.created = buckets(raw.created); snap.edited = buckets(raw.edited); snap.deleted = buckets(raw.deleted); - // Only an instant: it is the start of every list with no cursor of its own. snap.lastPollAt = typeof raw.lastPollAt === 'string' && Number.isFinite(Date.parse(raw.lastPollAt)) ? raw.lastPollAt : null; - // Additive like `pollHeads`: a file written before them starts every list - // from `lastPollAt`, with no row remembered as announced new. snap.pollCursors = cursors(raw.pollCursors); snap.pollNew = {}; if (raw.pollNew && typeof raw.pollNew === 'object' && !Array.isArray(raw.pollNew)) { for (const [list, rows] of Object.entries(raw.pollNew)) snap.pollNew[list] = strings(rows); } snap.pollKeys = Array.isArray(raw.pollKeys) ? raw.pollKeys.filter((k): k is string => typeof k === 'string') : []; - // Additive since amendment 2a, so a state file written before it is still a - // valid version-1 snapshot: an absent map just means no head is known yet. snap.pollHeads = strings(raw.pollHeads); return snap; } -/** - * Poll cursors from untrusted JSON. An entry whose `since` isn't an instant is - * dropped, and its list starts from `lastPollAt`, which is never later than - * where any list stopped; a `resumeAt` that isn't one is left out, so the - * list reads from its `since`. - */ function cursors(raw: unknown): Record<string, PollCursor> { if (!raw || typeof raw !== 'object' || Array.isArray(raw)) return {}; const instant = (value: unknown): value is string => typeof value === 'string' && Number.isFinite(Date.parse(value)); @@ -458,7 +331,6 @@ function cursors(raw: unknown): Record<string, PollCursor> { return out; } -/** A `Record<string, string>` from untrusted JSON; non-string values dropped. */ function strings(raw: unknown): Record<string, string> { if (!raw || typeof raw !== 'object' || Array.isArray(raw)) return {}; const out: Record<string, string> = {}; diff --git a/src/tags.ts b/src/tags.ts index b946032..ec41606 100644 --- a/src/tags.ts +++ b/src/tags.ts @@ -1,28 +1,7 @@ /** - * Event tags and wake metadata (TASK-SRHKG.1.4; contract section "Tags and - * metadata"; MCPL RFC-001). - * - * Pure functions: `tagsFor(event, ctx)` takes a `NormalizedEvent` and what the - * caller knows about the deployment, and returns the tag set plus the metadata - * the host matches `metadataTrue` rules against. No I/O, no api calls — the - * caller (.5 channels, .9 push/event) has already fetched everything. - * - * Two invariants this module exists to hold: - * - * 1. `chat:ambient` is never emitted beside `chat:mention` or `chat:reply`. - * RFC-001 §4.1 makes host-side closure additive (`chat:mention ⇒ - * chat:addressed`), so an event carrying both umbrellas is not - * interpretable by a first-match-wins rule list. - * 2. `chat:from-self` is emitted for exactly one login, `ctx.botLogin`. - * Another bot is `chat:from-bot`; a human is `chat:from-human`. - * - * On `chat:addressed`: the frozen contract enumerates the core tags this - * server emits (`CORE_TAGS` in types.ts) and the umbrella is not among them. - * RFC-001 §4.1 defines its expansion as normative *host* behaviour ("Hosts - * MUST expand them"), and names direct emission and reliance on closure as - * both conforming, so emitting only the specific tag keeps the emitted - * vocabulary equal to the contract's list. Invariant 1 is what makes that - * safe: closure can never produce `chat:addressed` beside `chat:ambient` here. + * MCPL RFC-001 §4.1: hosts expand `chat:mention` and `chat:reply` to + * `chat:addressed`, and a producer should not emit `chat:ambient` beside a tag + * that implies `chat:addressed`, so this module never does. */ import type { TagOntology } from '@animalabs/mcpl-core'; @@ -39,58 +18,23 @@ import { type TagResult, } from './types.js'; -// ───────────────────────────────────────────────────────────────────────────── -// Bounds and context -// ───────────────────────────────────────────────────────────────────────────── - -/** - * "Every string field is bounded" (contract). `LIMITS` bounds what reaches the - * model; these bound the much smaller scalars of metadata, which a host keeps - * per event and matches rules against. - */ export const METADATA_LIMITS = { - /** `repo`, `actor`, `sha`. */ scalarChars: 120, - /** `htmlUrl`. GitHub's own links are far below this; a hostile one is cut. */ urlChars: 512, } as const; -/** - * What `tagsFor` needs beyond {@link TagContext}, which the frozen contract - * fixed before these two facts were known to be underivable from a - * `NormalizedEvent`. Both are optional, so a `TagContext` is still accepted. - * - * - `merged`: `pull.closed` carries no merged/unmerged discriminator (the - * payload's `pull_request.merged` lives in `raw`, which never reaches this - * module), yet `github:pr=merged` is in the contract's vocabulary. The - * ingress (.3) or the caller passes it; absent means "not known to be - * merged" and only `github:pr=closed` is emitted. - * - `reviewRequestedForSelf`: `pull.review_requested` carries no requested - * reviewer, so the event alone cannot say whether *this* bot was asked. - * Absent means "assume the request concerns the bot" — the conservative - * choice for a wake signal; `false` suppresses `isReviewRequested`. - */ export interface GitHubTagContext extends TagContext { merged?: boolean; reviewRequestedForSelf?: boolean; } -// ───────────────────────────────────────────────────────────────────────────── -// Detection helpers -// ───────────────────────────────────────────────────────────────────────────── - function escapeRegExp(s: string): string { return s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); } /** - * Does `body` @-mention the bot? - * - * A GitHub App's login is `name[bot]`, but GitHub writes the mention as - * `@name` — and a human typing the full `@name[bot]` means the same thing. So - * both spellings count. The lookbehind keeps `foo@name` (an email, a handle - * inside a word) from matching, and the trailing boundary keeps `@name` from - * matching `@name-other`. + * A GitHub App's login is `name[bot]`, but GitHub writes its mention as + * `@name`, so both spellings count. */ export function mentionsBot(body: string | undefined, botLogin: string): boolean { if (!body || !botLogin) return false; @@ -100,16 +44,10 @@ export function mentionsBot(body: string | undefined, botLogin: string): boolean return re.test(body); } -/** Does `body` carry a URL? Drives `chat:has-link`. */ export function hasLink(body: string | undefined): boolean { return !!body && /\bhttps?:\/\/[^\s<>()]+/i.test(body); } -/** - * Which check conclusions count as a CI failure. `null` is "still running", - * and `neutral`/`cancelled`/`skipped`/`stale` are not failures — they must not - * wake a recipe that watches for broken builds. - */ const FAILING_CONCLUSIONS: readonly CheckConclusion[] = ['failure', 'timed_out', 'action_required']; function isFailure(event: NormalizedEvent): boolean { @@ -119,11 +57,6 @@ function isFailure(event: NormalizedEvent): boolean { return checks.failed.length > 0; } -// ───────────────────────────────────────────────────────────────────────────── -// tagsFor -// ───────────────────────────────────────────────────────────────────────────── - -/** The kinds whose body is new text, and so the only ones a mention can address. */ const MENTION_KINDS: ReadonlySet<NormalizedEvent['kind']> = new Set([ 'issue.opened', 'issue.edited', @@ -135,37 +68,23 @@ const MENTION_KINDS: ReadonlySet<NormalizedEvent['kind']> = new Set([ 'review_comment.edited', ]); -/** - * The tag set and wake metadata for one event. - * - * `ctx.isOwnThread` and `ctx.isReplyToSelf` are facts only the caller can - * know (the channel registry in .5 tracks the threads the bot authored or - * commented in, and the ids of its own comments); they arrive as booleans. - */ export function tagsFor(event: NormalizedEvent, ctx: GitHubTagContext): TagResult { const tags = new Set<string>(); - // ── Sender: exactly one of the three, from-self only for the bot login ── const fromSelf = !!ctx.botLogin && event.actor.login === ctx.botLogin; if (fromSelf) tags.add('chat:from-self'); else if (event.actor.isBot) tags.add('chat:from-bot'); else tags.add('chat:from-human'); - // ── Addressing: mention, reply, or ambient — never ambient beside either ── - // Only an event that brings its text counts a mention: a close, a reopen or - // new commits re-carry a body the bot already saw when it was written. const mentioned = MENTION_KINDS.has(event.kind) && mentionsBot(event.body, ctx.botLogin); const isOwnThread = ctx.isOwnThread === true; const isReplyToSelf = ctx.isReplyToSelf === true; - // The contract: "chat:reply when the event is on a thread the bot authored - // or has commented in, or replies to the bot's comment". const isReply = isOwnThread || isReplyToSelf; if (mentioned) tags.add('chat:mention'); if (isReply) tags.add('chat:reply'); if (!mentioned && !isReply) tags.add('chat:ambient'); - // `chat:dm` never: GitHub has no 1:1 channel this server speaks on. + // `chat:dm` never: GitHub has no one-to-one channel. - // ── Locus, lifecycle, content ── if (event.kind === 'review_comment.created' || event.kind === 'review_comment.edited') { tags.add('chat:thread'); } @@ -179,7 +98,6 @@ export function tagsFor(event: NormalizedEvent, ctx: GitHubTagContext): TagResul if (event.kind === 'comment.deleted') tags.add('chat:deleted'); if (hasLink(event.body)) tags.add('chat:has-link'); - // ── The github: namespace ── const isReviewRequested = event.kind === 'pull.review_requested' && ctx.reviewRequestedForSelf !== false; const ciFailure = isFailure(event); @@ -197,8 +115,6 @@ export function tagsFor(event: NormalizedEvent, ctx: GitHubTagContext): TagResul tags.add(`github:pr=${event.kind.slice('pull.'.length)}`); break; case 'pull.closed': - // A merge is also a close, and both values are in the vocabulary, so a - // rule written on either fires. Tags are a set, not a state machine. tags.add('github:pr=closed'); if (ctx.merged === true) tags.add('github:pr=merged'); break; @@ -210,8 +126,6 @@ export function tagsFor(event: NormalizedEvent, ctx: GitHubTagContext): TagResul tags.add('github:review-comment'); break; case 'checks.completed': - // Only the two values the contract enumerates. A neutral or cancelled - // suite is neither, and stays untagged rather than being called a pass. if (ciFailure) tags.add('github:ci=failure'); else if (event.checks?.conclusion === 'success') tags.add('github:ci=success'); break; @@ -221,8 +135,6 @@ export function tagsFor(event: NormalizedEvent, ctx: GitHubTagContext): TagResul case 'comment.created': case 'comment.edited': case 'comment.deleted': - // Issue and PR conversation comments carry no `github:` tag of their - // own: the core vocabulary already says everything a recipe routes on. break; } @@ -248,31 +160,12 @@ export function tagsFor(event: NormalizedEvent, ctx: GitHubTagContext): TagResul return { tags: [...tags], metadata }; } -/** - * Every tag `tagsFor` can emit. It is exactly the frozen contract's two lists: - * `CORE_TAGS` already omits `chat:dm` (GitHub has no 1:1 channel this server - * speaks on) and `chat:addressed` (the host's closure, see the file header). - */ export const EMITTED_TAGS: readonly string[] = [...CORE_TAGS, ...GITHUB_TAGS]; -// ───────────────────────────────────────────────────────────────────────────── -// The producer ontology (RFC-001 §5), for the feature-set declaration in .6 -// ───────────────────────────────────────────────────────────────────────────── - /** - * This producer's tag ontology — a hint catalog, not a closed schema. - * - * Two rules it obeys deliberately: - * - * - **No `implies` edge into `chat:*`.** RFC-001 §4.1: a producer-declared - * edge into the reserved namespace lets a server promote its own traffic - * into the band a consumer reserved for being spoken to. The core closure - * is the host's and is fixed by the spec; nothing here touches it. - * - **No suggested treatment that promotes ambient traffic.** The rules - * below only ever *lower* the treatment of unaddressed traffic (ambient, - * bot authors, own echoes, deletions). The single `immediate` rule is for - * CI failures on a thread the agent is already in — never for ambient. - * They are advisory either way (§5.2) and apply only on acceptance. + * MCPL RFC-001 §4.1: a producer's `implies` edge into `chat:*` would promote + * its own traffic into the band reserved for being addressed, so there is none. + * Suggested treatments are advisory (§5.2). */ export const GITHUB_TAG_ONTOLOGY: TagOntology = { coreTags: [...CORE_TAGS], @@ -339,9 +232,6 @@ export const GITHUB_TAG_ONTOLOGY: TagOntology = { ci: { desc: 'Rolled-up conclusion of the checks on a head sha', values: ['success', 'failure'] }, actor: { desc: 'Standing of the actor in this deployment', values: ['maintainer'] }, }, - // Advisory, and only ever quieter than the host's own default for traffic - // nobody addressed. First match wins, so the `mute`/`throttle` rules below - // can never be reached by an event that is also addressed. defaultTreatment: [ { tagsAny: ['chat:from-self'], behavior: 'mute' }, { tagsAny: ['chat:deleted'], behavior: 'mute' }, @@ -353,5 +243,4 @@ export const GITHUB_TAG_ONTOLOGY: TagOntology = { open: false, }; -/** The producer name every tag set this server emits is attributed to. */ export const TAG_ONTOLOGY_PRODUCER = TAG_PRODUCER; diff --git a/src/tool-support.ts b/src/tool-support.ts index 4a13964..6d4612a 100644 --- a/src/tool-support.ts +++ b/src/tool-support.ts @@ -1,21 +1,3 @@ -/** - * Tool support — the arguments, refusals, failures and per-field bounds every - * ToolModule shares (TASK-SRHKG.1.9, moved out of tools-read.ts). - * - * .6 put these in `tools-read.ts` because its write scope was those four files - * and both write modules needed them; .7 then grew its own thread parser in - * `tools-comments.ts` because the two packages were built in parallel. WP9 - * reconciles both: the helpers live here, and {@link parseThread} is the one - * parser for every form of "which issue or pull request" — `owner/name#12`, - * `github:owner/name#12`, `#12`, `12`, and `{ owner, name, number, kind }`. - * - * The render* functions stay where they are. `content.ts` renders an *event* - * for a channel delivery; these tools render a *result* for the model, and - * only .6's renderers bound the fields the contract's `LIMITS` does not name - * (a login, a branch, a sha, a node id, a path, a URL, an echoed query). See - * the WP9 report for that decision. - */ - import { LIMITS, bounded, @@ -28,59 +10,26 @@ import { type ToolContext, type ToolResult, } from './types.js'; -// ───────────────────────────────────────────────────────────────────────────── -// Bounds -// ───────────────────────────────────────────────────────────────────────────── -/** - * Per-field bounds for everything that reaches the model. - * - * `LIMITS` (types.ts) fixes the three the contract names — a comment body at - * 4k, a list at 50 entries, a title at 80. Every *other* string on the way out - * is bounded here, because the contract's three do not cover a label, a login, - * a branch, a sha, a node id, a path, a URL, or an argument echoed back in a - * message, and an unbounded field is an unbounded result no matter how well - * the enumerated ones behave. - */ export const FIELD = { - /** A login, a label, a branch, a state, a check name, a timestamp. */ name: LIMITS.titleChars, - /** `owner/name`: GitHub allows 39 + 1 + 100 characters. */ slug: 160, - /** A commit sha — or whatever was passed as one. */ sha: 64, - /** A GraphQL node id (a review thread). */ nodeId: 128, - /** A file path in a diff. */ path: 200, - /** An echoed search query. */ query: 200, - /** A URL: long enough that it stays clickable. */ url: 300, } as const; -/** - * A value quoted for a message, bounded. Every "got …" in an argument error, - * every name in a refusal and every confirmation line goes through it, so no - * message can be made arbitrarily long by making an argument arbitrarily long. - */ export function echo(value: unknown, max: number = FIELD.name): string { if (typeof value !== 'string') return bounded(JSON.stringify(value) ?? String(value), max); - // Bound first, quote second: truncating the quoted form would eat the - // closing quote and leave the message looking unterminated. return JSON.stringify(bounded(value, Math.max(0, max - 2))); } -/** A string field on the way out, bounded to its kind. */ export function field(value: string, max: number = FIELD.name): string { return bounded(value, max); } -/** - * The first {@link LIMITS.listEntries} entries, joined on one line, naming - * what was dropped. For a list that is a field (labels, reviewers, logins) - * rather than a block. - */ export function boundedInline(items: readonly string[], max: number = FIELD.name): string { const shown = items.slice(0, LIMITS.listEntries).map((item) => field(item, max)); if (items.length > shown.length) { @@ -89,16 +38,6 @@ export function boundedInline(items: readonly string[], max: number = FIELD.name return shown.join(', '); } -// ───────────────────────────────────────────────────────────────────────────── -// Refusals, failures, results -// ───────────────────────────────────────────────────────────────────────────── - -/** - * A policy refusal: the repository is outside GITHUB_REPOS, the base branch is - * outside GITHUB_PR_BASE_ALLOW, the review would approve the bot's own pull - * request. Not an error from GitHub and not a malformed call — the tool could - * have done it and must not, and the reason names what was refused. - */ export class ToolRefusal extends Error { constructor(reason: string) { super(reason); @@ -110,7 +49,6 @@ export function refuse(reason: string): never { throw new ToolRefusal(reason); } -/** A malformed call: a missing argument, a wrong type, a value out of domain. */ export class ToolArgumentError extends Error { constructor(message: string) { super(message); @@ -118,7 +56,6 @@ export class ToolArgumentError extends Error { } } -/** A text-block tool result (the contract: tool results are text blocks). */ export function toolResult(text: string): ToolResult { return { content: [{ type: 'text', text }] }; } @@ -127,17 +64,11 @@ export function errorResult(text: string): ToolResult { return { content: [{ type: 'text', text }], isError: true }; } -/** An HTTP status carried by an octokit error, when there is one. */ function statusOf(err: unknown): number | undefined { const status = (err as { status?: unknown } | null)?.status; return typeof status === 'number' ? status : undefined; } -/** - * One line for a failed call. A GitHub error is reported with its status and - * its message, as the contract requires; nothing else about the response is - * forwarded, so no header and no secret can ride along. - */ export function describeFailure(toolName: string, err: unknown): string { const tool = field(toolName); if (err instanceof ToolRefusal) return `${tool} refused: ${err.message}`; @@ -187,10 +118,6 @@ export async function runTool( } } -// ───────────────────────────────────────────────────────────────────────────── -// Arguments -// ───────────────────────────────────────────────────────────────────────────── - export function requireString(args: Record<string, unknown>, key: string): string { const value = args[key]; if (typeof value !== 'string' || value.trim() === '') { @@ -206,7 +133,6 @@ export function optionalString(args: Record<string, unknown>, key: string): stri return value; } -/** A positive integer, which is what every GitHub number and line is. */ export function requireNumber(args: Record<string, unknown>, key: string): number { const value = args[key]; const n = typeof value === 'string' && value.trim() !== '' ? Number(value) : value; @@ -241,7 +167,6 @@ export function requireStringArray(args: Record<string, unknown>, key: string): }); } -/** A value from a closed set; `fallback` applies when the argument is absent. */ export function oneOf<T extends string>( args: Record<string, unknown>, key: string, @@ -256,12 +181,6 @@ export function oneOf<T extends string>( return value as T; } -/** - * The repository a call acts on: the `repo` argument when given, otherwise the - * single repository GITHUB_REPOS names. A repository outside the allow-list is - * refused by name — for reads as well as writes, since the installation token - * could not reach it anyway and a named refusal beats a 404. - */ export function resolveRepo(args: Record<string, unknown>, ctx: ToolContext): RepoRef { const allowed = ctx.repos; if (allowed.length === 0) { @@ -279,11 +198,6 @@ export function resolveRepo(args: Record<string, unknown>, ctx: ToolContext): Re return requireAllowedRepo(repo, allowed); } -/** - * A repository, checked against GITHUB_REPOS. The refusal names the - * repository asked for and the ones allowed, so the caller can correct it in - * one step. - */ export function requireAllowedRepo(repo: RepoRef, allowed: readonly RepoRef[]): RepoRef { if (!repoAllowed(repo, allowed)) { refuse( @@ -296,32 +210,16 @@ export function requireAllowedRepo(repo: RepoRef, allowed: readonly RepoRef[]): return repo; } -// ───────────────────────────────────────────────────────────────────────────── -// The thread argument — one parser (WP9) -// ───────────────────────────────────────────────────────────────────────────── - -/** Either a parsed value or the refusal message explaining why not. */ export type Parsed<T> = { ok: true; value: T } | { ok: false; error: string }; /** - * Which issue or pull request a call acts on, from every form the two tool - * families accept: - * - * - `12` or `"12"` or `"#12"` — the number, in the one repository - * GITHUB_REPOS names; with several, the repository has to be spelled; - * - `"owner/name#12"` and `"github:owner/name#12"` — the channel id's own - * spelling, so a thread can be named exactly as an event named it; - * - `{ owner, name, number, kind }`, or `{ repo: "owner/name", number }`, - * or `{ repo: "name", number }`. - * - * `kindDefault` applies when the form carries no `kind`; `issue` is also the - * REST path issue comments take on a pull request. The repository allow-list - * is **not** checked here — the two callers report a refusal differently (a - * thrown {@link ToolRefusal} for .6, an error *result* for .7) — so each one - * calls {@link requireAllowedRepo} itself. - * - * Returns the refusal message rather than throwing, because .7's tools answer - * a malformed `thread` with an error result the model can read. + * The `thread` forms the tools accept: `12`, `"12"` or `"#12"` in the one + * repository GITHUB_REPOS names; `"owner/name#12"` or `"github:owner/name#12"`, + * the channel id's spelling; `{ owner, name, number, kind }` and + * `{ repo: "owner/name", number }`; and `{ repo: "name", number }`, which + * takes its owner from the one repository GITHUB_REPOS names. Without a + * `kind`, `kindDefault` applies; issue comments on a pull request take the + * `issue` path in GitHub's REST API. */ export function parseThread( raw: unknown, @@ -346,7 +244,6 @@ export function parseThread( number = Number(m[3]); } else if (raw && typeof raw === 'object') { const obj = raw as Record<string, unknown>; - // `repo` accepts either `owner/name` or just the repository name. const repoField = typeof obj.repo === 'string' ? obj.repo : undefined; if (repoField?.includes('/')) { [owner, name] = repoField.split('/', 2); @@ -378,12 +275,6 @@ export function parseThread( return { ok: true, value: { repo: { owner, name }, number: number as number, kind: kind ?? kindDefault } }; } -/** - * `{ repo, number, kind }` for a .6 tool: the `thread` argument in any of - * {@link parseThread}'s forms when one is given, otherwise the `repo` and - * `number` pair those tools declare. `kind` is the calling tool's own, so - * `get_issue` stays an issue read whatever the argument spelled. - */ export function resolveThread(args: Record<string, unknown>, ctx: ToolContext, kind: ThreadKind): ThreadRef { if (args.thread !== undefined && args.thread !== null) { const parsed = parseThread(args.thread, ctx, kind); @@ -394,11 +285,6 @@ export function resolveThread(args: Record<string, unknown>, ctx: ToolContext, k return { repo: resolveRepo(args, ctx), number: requireNumber(args, 'number'), kind }; } -// ───────────────────────────────────────────────────────────────────────────── -// Input-schema fragments -// ───────────────────────────────────────────────────────────────────────────── - -/** Every tool takes it; absent means the single repository in GITHUB_REPOS. */ export const REPO_PROPERTY = { type: 'string', description: @@ -412,11 +298,6 @@ export const NUMBER_PROPERTY = { description: 'Issue or pull-request number', } as const; -/** - * The `thread` argument {@link parseThread} reads — .7's comment tools declare - * it, and it lives beside its parser so the declared forms and the accepted - * forms are one list. - */ export const THREAD_PROPERTY = { description: 'The issue or pull request: "owner/name#12", "#12", 12, or { owner, name, number, kind }.', anyOf: [ diff --git a/src/tools-comments.ts b/src/tools-comments.ts index 7813556..a13dddc 100644 --- a/src/tools-comments.ts +++ b/src/tools-comments.ts @@ -1,33 +1,3 @@ -/** - * `github.comments` — the rollback-capable write tools (TASK-SRHKG.1.7). - * - * The contract's feature-set bullet: `comment`, `reply_in_thread`, - * `edit_own_comment`, `delete_own_comment`, `upsert_sticky_comment`. Every one - * of them refuses a repository outside GITHUB_REPOS by name, and every write - * records into {@link GithubState} under the current checkpoint and flushes, so - * {@link rollbackComments} can undo it after a restart as well as before one. - * - * Two arguments the contract names but does not shape, decided here and - * reported for .9 to reconcile: - * - * - **`thread`** accepts `{ owner?, name?, number, kind? }` or the string - * forms `owner/name#12`, `github:owner/name#12`, `#12`, `12`. Owner and - * name default to the entry of GITHUB_REPOS when it holds one, and are - * required when it holds several; `kind` defaults to `issue`, which is - * also the REST path issue - * comments take on a pull request. - * - **`edit_own_comment` and `delete_own_comment` take `thread` as well as - * `commentId`, and an optional `review`.** `GitHubApi` has no - * `getComment`, so the only way to check that the author is the bot — the - * refusal the contract demands — is to list the thread's comments and find - * it there. Issue comments and review comments are numbered separately, so - * `review` names the endpoint; omitted, an id that exists on both is - * refused rather than guessed at. - * - * Tool results are plain text blocks bounded by {@link LIMITS}; .4's - * `content.ts` does not exist on `main` yet, so nothing here imports it. - */ - import { ERR_CHECKPOINT_NOT_FOUND, textContent, @@ -60,11 +30,6 @@ import { type ToolResult, } from './types.js'; -// ───────────────────────────────────────────────────────────────────────────── -// Argument parsing -// ───────────────────────────────────────────────────────────────────────────── - -/** A refusal: the message the tool answers with, as a text block. */ function refuse(message: string): ToolResult { return { content: [textContent(message)], isError: true }; } @@ -73,11 +38,6 @@ function ok(message: string): ToolResult { return { content: [textContent(bounded(message, LIMITS.commentChars))] }; } -/** - * `thread` is parsed by {@link parseThread} in `tool-support.ts` — WP9 made it - * the one parser for every form both tool families accept, so .6's `repo` - * plus `number` and .7's single `thread` cannot drift apart. - */ function parseThreadArg(raw: unknown, ctx: ToolContext, kindDefault: ThreadKind = 'issue'): Parsed<ThreadRef> { return parseThread(raw, ctx, kindDefault); } @@ -95,10 +55,6 @@ function parseCommentId(raw: unknown): Parsed<number> { return { ok: true, value }; } -/** - * Which endpoint owns the comment id: `true` review comment, `false` issue - * comment, absent "look on both and refuse an id that exists on each". - */ function parseEndpoint(raw: unknown): Parsed<boolean | undefined> { if (raw === undefined || raw === null) return { ok: true, value: undefined }; if (typeof raw !== 'boolean') { @@ -107,18 +63,11 @@ function parseEndpoint(raw: unknown): Parsed<boolean | undefined> { return { ok: true, value: raw }; } -/** The allow-list check every write tool makes, naming the repository. */ function repoRefused(repo: RepoRef, ctx: ToolContext): string | null { if (repoAllowed(repo, ctx.repos)) return null; return `refused: ${repoSlug(repo)} is not in GITHUB_REPOS (${ctx.repos.map(repoSlug).join(', ') || 'empty'})`; } -/** - * The repository of a checkpoint record, or why the undo must not touch it: a - * record written under one GITHUB_REPOS may be replayed under another (the - * state file outlives a configuration change), and rollback is a write like - * any other, so the allow-list binds it exactly as it binds the tools. - */ function recordRepo(slug: string, ctx: ToolContext): Parsed<RepoRef> { const repo = parseRepoSlug(slug); if (!repo) return { ok: false, error: `unparsable repository "${slug}" in the checkpoint record` }; @@ -126,12 +75,6 @@ function recordRepo(slug: string, ctx: ToolContext): Parsed<RepoRef> { return denied ? { ok: false, error: denied } : { ok: true, value: repo }; } -/** - * `ToolContext.state` is `StateStore`, which WP9 widened to carry the delete - * record and the §8.7 status. What is still only ours is the *plan* - * `rollback` returns — {@link CommentRollbackPlan} also lists the deletes - * nothing can undo. One cast, named here rather than scattered. - */ function store(ctx: Pick<ToolContext, 'state'>): GithubStateStore { return ctx.state as GithubStateStore; } @@ -140,50 +83,24 @@ function locator(thread: ThreadRef): ThreadLocator { return { number: thread.number, kind: thread.kind }; } -// ───────────────────────────────────────────────────────────────────────────── -// Sticky markers -// ───────────────────────────────────────────────────────────────────────────── - -/** - * The HTML comment a sticky body is recognised by. A bare `marker` ("ci-status") - * becomes `<!-- ci-status -->`; a marker already written as an HTML comment is - * kept verbatim, so a recipe may pass either. - */ export function stickyMarker(marker: string): string { const trimmed = marker.trim(); return /^<!--[\s\S]*-->$/.test(trimmed) ? trimmed : `<!-- ${trimmed} -->`; } -/** The body to write: the marker is kept, exactly once, at the top. */ export function stickyBody(marker: string, body: string): string { const html = stickyMarker(marker); return body.includes(html) ? body : `${html}\n${body}`; } -// ───────────────────────────────────────────────────────────────────────────── -// Finding a comment the bot may own -// ───────────────────────────────────────────────────────────────────────────── - -/** Where a comment lives, which decides the REST path of an edit or delete. */ interface FoundComment { comment: Comment; - /** True for a review comment (`pulls/comments/{id}`). */ review: boolean; } /** - * Find `commentId` on `thread`, on the endpoint the caller named. - * - * GitHub numbers issue comments (`issues/comments/{id}`) and review comments - * (`pulls/comments/{id}`) from separate sequences, so one number can name one - * of each on the same pull request — and `thread.kind` cannot tell them apart, - * since a pull request carries comments of both kinds. So the endpoint is the - * caller's to name: - * - * - `review: true` looks only among the review conversations; - * - `review: false` only among the issue comments; - * - omitted looks on both and, when the id exists on both, refuses and says - * so rather than picking one and editing the wrong comment. + * GitHub numbers issue comments and review comments from separate sequences, + * so one id can name one of each on the same pull request. */ async function findComment( thread: ThreadRef, @@ -193,11 +110,6 @@ async function findComment( ): Promise<Parsed<FoundComment>> { const where = `${repoSlug(thread.repo)}#${thread.number}`; const asIssue = review === true ? null : await findIssueComment(thread, commentId, ctx); - // An issue has no review conversations, so when the endpoint is left open - // only a pull request is asked; `review: true` asks regardless of the kind - // the caller gave. A lookup that fails is not evidence of absence: the - // endpoint stays unknown and nothing is written (a timeout must never pick - // the issue comment). let asReview: FoundComment | null = null; if (review === true || (review === undefined && thread.kind === 'pull')) { const lookup = await findReviewComment(thread, commentId, ctx); @@ -226,17 +138,11 @@ async function findComment( return { ok: false, error: `no comment ${commentId} on ${where}${endpoint}` }; } -/** `issues/comments/{id}`: top-level comments, on an issue or a pull request. */ async function findIssueComment(thread: ThreadRef, commentId: number, ctx: ToolContext): Promise<FoundComment | null> { const comment = (await ctx.api.listComments(thread)).find((c) => c.id === commentId); return comment ? { comment, review: false } : null; } -/** - * `pulls/comments/{id}`: comments inside a review conversation. A lookup that - * throws is reported as `failed`, distinct from an empty answer: the caller - * decides, and never treats a failure as "not there". - */ async function findReviewComment( thread: ThreadRef, commentId: number, @@ -272,11 +178,6 @@ async function findReviewComment( return { found: null }; } -// ───────────────────────────────────────────────────────────────────────────── -// The tools -// ───────────────────────────────────────────────────────────────────────────── - -/** The `thread` argument's schema; its parser and its shape live together. */ const THREAD_SCHEMA = THREAD_PROPERTY; export const COMMENT_TOOLS: ToolDefinition[] = [ @@ -353,7 +254,6 @@ export const COMMENT_TOOLS: ToolDefinition[] = [ }, ]; -/** The `github.comments` ToolModule .9 routes `tools/call` to. */ export const commentsToolModule: ToolModule = { featureSet: COMMENTS_FEATURE_SET, tools: COMMENT_TOOLS, @@ -511,10 +411,6 @@ async function upsertStickyComment(args: Record<string, unknown>, ctx: ToolConte ); } -/** - * The entry point .5's `channels/publish` shares with these tools: a comment - * this server just created, booked under the current checkpoint and flushed. - */ export async function recordCreated( created: Pick<Comment, 'id'>, thread: ThreadRef, @@ -530,10 +426,6 @@ export async function recordCreated( await ctx.state.flush(); } -/** - * The entry point .5 uses when it edits a comment outside these tools: the - * previous body, booked under the current checkpoint and flushed. - */ export async function recordEdited( commentId: number, previousBody: string, @@ -551,11 +443,6 @@ export async function recordEdited( await ctx.state.flush(); } -// ───────────────────────────────────────────────────────────────────────────── -// state/rollback (SPEC §8.5, §8.6) -// ───────────────────────────────────────────────────────────────────────────── - -/** One action the rollback took, or could not take. */ export interface RollbackItem { /** `delete` undoes a create, `restore` undoes an edit, `none` is unundoable. */ action: 'delete' | 'restore' | 'none'; @@ -563,34 +450,21 @@ export interface RollbackItem { commentId: number; review: boolean; thread?: ThreadLocator; - /** Why it could not be undone. Absent on a success. */ reason?: string; } -/** - * One thing a rollback actually did, reported to the caller as it happens - * (review round 3, finding 1). - * - * Deliberately *not* part of {@link CommentRollbackResult}: `body` is comment - * prose and the receipt goes to the host over the wire, while this is an - * internal fact the server needs to keep its thread knowledge true. It is - * reported per success rather than derived from the receipt afterwards so that - * the order is the order the writes happened in — a delete that lowers the - * bot's latest comment must be seen before a restore that refreshes it. - */ export interface RollbackEffect { action: 'delete' | 'restore'; thread: ThreadRef; commentId: number; review: boolean; - /** The body now on GitHub. A restore has one; a delete does not. */ body?: string; } /** - * The §8.6 response, plus the receipt the contract asks for: what was undone - * and what was not. Extra result fields are explicitly tolerated (§8), so a - * host that only knows `checkpoint`/`success`/`reason` still reads this. + * MCPL's rollback result, plus a receipt of what was undone and what wasn't. + * MCPL tolerates extra result fields, so a host that knows only `checkpoint`, + * `success` and `reason` still reads it. */ export interface CommentRollbackResult extends StateRollbackResult { undone: RollbackItem[]; @@ -599,31 +473,15 @@ export interface CommentRollbackResult extends StateRollbackResult { /** * Undo the comment writes made after `params.checkpoint`, best effort: delete - * what was created, restore the previous body of what was edited, and report - * every action that failed and every delete that cannot be undone at all. + * what was created, and give each edited comment back the body it had at the + * checkpoint in one update. The receipt names every action that failed and + * every delete, which GitHub can't undo. A record naming a repository outside + * GITHUB_REPOS is reported as not undone and never executed. Whatever wasn't + * undone, deletes included, stays booked under the checkpoint, so a retry + * attempts it again and never reports a complete rollback over it. * - * Three properties the receipt rests on: - * - * - **The allow-list binds the undo.** A record naming a repository outside - * GITHUB_REPOS is reported, by name, as not undone — never executed. - * - **A comment is its repository, its id *and* its kind.** An issue comment - * and a review comment can share an id (they are numbered by different - * endpoints), so the dedup key carries `review` and so does every call. - * - **One restore per comment, to the oldest body in the range.** Several - * edits of one comment collapse into a single update to the body it had at - * the checkpoint; the intermediate bodies are never written, and a failure - * retains only that one target, so no retry can resurrect a body the - * checkpoint never named. - * - **An action that did not happen stays booked.** Whatever could not be - * undone is recorded again under the checkpoint before the result is - * flushed, so a retry attempts it once more and cannot report a completed - * rollback over a state that was never restored. A delete is booked again - * too: GitHub cannot restore it, so no retry of this checkpoint will ever - * be a full success, and the receipt keeps saying so. - * - * Throws {@link McplRpcError} -32005 for a checkpoint that is unknown or was - * abandoned by an earlier rollback (§8.7) — the only outcome the §8.6 result - * shape cannot express. + * Throws -32005 for a checkpoint that is unknown or that an earlier rollback + * abandoned, which MCPL's rollback result has no way to say. */ export async function rollbackComments( params: StateRollbackParams, @@ -655,11 +513,8 @@ export async function rollbackComments( const undone: RollbackItem[] = []; const notUndone: RollbackItem[] = []; - /** Comments this rollback actually deleted, by {@link idKey}. */ const deletedKeys = new Set<string>(); - /** Comments a create record was already handled for, duplicates included. */ const handledKeys = new Set<string>(); - /** Records to book again, because their action did not happen. */ const keptCreated: typeof plan.created = []; const keptEdited: typeof plan.edited = []; @@ -698,22 +553,14 @@ export async function rollbackComments( } } - // One restore per comment: the body it had when the checkpoint was taken, - // which is the *oldest* previous body in the range. The plan is newest first, - // so the last record for an identity is the oldest one; the bodies in between - // are states the rollback never has to pass through, and writing them would - // be a detour whose failure could strand the comment on an intermediate - // version. `Map` keeps the first insertion's position and the last value, so - // this collapses the records without reordering the comments. + // A `Map` keeps a key's first position and its last value, and the plan is + // newest first, so each comment collapses to its oldest body in place. const targetEdit = new Map<string, (typeof plan.edited)[number]>(); for (const edited of plan.edited) { targetEdit.set(idKey(edited.repo, edited.commentId, edited.review), edited); } for (const [key, edited] of targetEdit) { - // A comment this rollback deleted is gone: the delete subsumes the restore, - // so it is neither an action nor a failure. A delete that *failed* leaves - // the comment standing, and its edit is restored as usual. if (deletedKeys.has(key)) continue; const item: RollbackItem = { action: 'restore', @@ -757,10 +604,6 @@ export async function rollbackComments( }); } - // Book the unresolved records again, in the order they were made: the plan is - // newest first and a bucket replays in insertion order. Only the collapsed - // restore target is kept per comment, so a retry aims at the same body this - // run did and a superseded intermediate body can never come back. for (const record of keptCreated.reverse()) state.recordCreatedComment(record); for (const record of keptEdited.reverse()) state.recordEditedComment(record); for (const record of [...(plan.deleted ?? [])].reverse()) state.recordDeletedComment(record); @@ -784,10 +627,6 @@ export async function rollbackComments( return result; } -/** - * A comment's identity for dedup: the repository, the endpoint that owns the - * id, and the id. `issues/12` and `pulls/12` are two different comments. - */ function idKey(repo: string, commentId: number, review: boolean): string { return `${repo}#${review ? 'pulls' : 'issues'}/${commentId}`; } diff --git a/src/tools-review.ts b/src/tools-review.ts index c15d60a..412b597 100644 --- a/src/tools-review.ts +++ b/src/tools-review.ts @@ -1,20 +1,8 @@ /** - * github.review — submitting a review and resolving review threads - * (TASK-SRHKG.1.6). - * - * create_review — COMMENT | APPROVE | REQUEST_CHANGES, with inline - * comments on diff lines. - * resolve_thread — mark a review thread resolved. - * unresolve_thread — reopen one. - * - * Not rollback-capable: GitHub has no API to withdraw a submitted review, so - * nothing here can be undone and the declaration says so (ROLLBACK_CAPABLE). - * - * The refusal that matters: a clone must not approve or demand changes on its - * own pull request. A verdict on one's own work is not review, and GitHub's own - * 422 would arrive too late to be legible, so the author is checked against - * GITHUB_BOT_LOGIN first and the refusal names the login. A plain COMMENT - * review on its own pull request is allowed — that is just talking. + * GitHub has no API to withdraw a submitted review, so these tools aren't + * rollback-capable. GitHub refuses a verdict on one's own pull request with a + * 422, so the author is checked against GITHUB_BOT_LOGIN first and the refusal + * names the login. */ import { @@ -52,22 +40,13 @@ import { const REVIEW_EVENTS: readonly ReviewEvent[] = ['COMMENT', 'APPROVE', 'REQUEST_CHANGES']; const DIFF_SIDES: readonly DiffSide[] = ['LEFT', 'RIGHT']; -/** A verdict on code is only a verdict when someone else wrote it. */ const VERDICT_EVENTS: readonly ReviewEvent[] = ['APPROVE', 'REQUEST_CHANGES']; -/** GitHub logins are case-insensitive; `rayforce-clone[bot]` is one login. */ +/** GitHub logins are case-insensitive. */ function sameLogin(a: string, b: string): boolean { return a.toLowerCase() === b.toLowerCase(); } -/** - * Validate the inline comments, position by position, and hand them on exactly - * as given: `createReview` must receive the path, line and side the caller - * named, because a review comment on the wrong line is worse than none. - * - * `side` defaults to RIGHT — the post-image, which is what a comment on a - * proposed change almost always means; LEFT addresses a removed line. - */ export function parseReviewComments(value: unknown): ReviewCommentInput[] { if (value === undefined || value === null) return []; if (!Array.isArray(value)) throw new ToolArgumentError('"comments" must be an array of inline comments'); @@ -102,11 +81,9 @@ export function parseReviewComments(value: unknown): ReviewCommentInput[] { } /** - * The review thread id `resolve_thread` takes is the GraphQL node id that - * `list_review_threads` prints, not the channel-level `rt:<comment id>`. The - * two name different things (contract notes, "GitHubApi signatures"), so the - * channel form is refused with the difference spelled out instead of being - * silently sent to an API that would 404. + * `resolve_thread` takes the GraphQL node id `list_review_threads` prints, not + * the channel's `rt:<comment id>`, so that form is refused with the + * difference named. */ export function requireReviewThreadId(args: Record<string, unknown>): string { const threadId = requireString(args, 'threadId'); @@ -121,19 +98,8 @@ export function requireReviewThreadId(args: Record<string, unknown>): string { } /** - * A review thread this server is allowed to touch, established rather than - * assumed. - * - * A GraphQL node id names a thread without naming its repository, so checking - * the `repo` argument proves nothing about the node the call then mutates: a - * node id from any repository the installation can reach would be resolved — - * and the confirmation would name the allowed repository, which is worse than - * failing. So the pull request is named too, and the thread has to be one of - * *its* threads: the repository allow-list is checked on the pull request - * (`resolveThread`), and membership on the list GitHub returns for it. - * - * The cost is one extra read per resolve, which is the price of the id being - * unforgeable here. + * A GraphQL node id names a thread without its repository, so the pull request + * is named too and the thread has to be one of its threads. */ async function requireThreadOnPull( args: Record<string, unknown>, @@ -152,14 +118,6 @@ async function requireThreadOnPull( return { pull, threadId }; } -/** - * A tool result, not a channel delivery: it stays here rather than moving to - * `content.ts`. WP9 kept the two renderer families apart on purpose — - * `content.ts` bounds bodies, titles and list length for an event an agent - * reads as a message, and these renderers bound each *field* (`field()` on the - * url here) for a result an agent reads as the outcome of its own call. One - * family cannot do both without dropping bounds. - */ function renderSubmittedReview(pull: ThreadRef, review: SubmittedReview, inline: number): string { return ( `submitted ${review.state} review ${review.id} on ${repoSlug(pull.repo)}#${pull.number} with ` + @@ -269,7 +227,6 @@ async function callReview(name: string, args: Record<string, unknown>, ctx: Tool const review = await ctx.api.createReview(pull, { event, body, - // Exactly the positions the caller gave, unreordered and unrounded. comments: comments.length > 0 ? comments : undefined, }); return toolResult(renderSubmittedReview(pull, review, comments.length)); @@ -293,7 +250,6 @@ async function callReview(name: string, args: Record<string, unknown>, ctx: Tool } } -/** The github.review ToolModule the server (.9) registers. */ export const reviewToolModule: ToolModule = { featureSet: REVIEW_FEATURE_SET, tools: reviewTools, diff --git a/src/webhook.ts b/src/webhook.ts index 58d560f..65a3004 100644 --- a/src/webhook.ts +++ b/src/webhook.ts @@ -1,21 +1,10 @@ /** - * Webhook ingress (TASK-SRHKG.1.3, §Ingress). - * - * A plain `node:http` server on GITHUB_WEBHOOK_PORT and GITHUB_WEBHOOK_PATH. - * The order of the checks is the security property, so it is spelled out - * once here and followed exactly in {@link WebhookListener.handle}: - * - * 1. method and path → 405 / 404 - * 2. body read with a 1 MiB cap → 413, socket destroyed - * 3. `X-Hub-Signature-256` over the *raw bytes*, constant-time → 401 - * 4. `X-GitHub-Delivery` dedup against a ring of the last 2000 ids → 200 - * 5. `ping` → 200, nothing emitted - * 6. 2xx **sent**, then: parse, repo allow-list, normalize, listener - * - * Nothing before step 3 parses the body, and nothing before step 3 touches - * the dedup ring — an unsigned request can neither be interpreted nor evict - * a real delivery id. The webhook secret is held in a closure, never logged, - * and never put in a response body. + * The webhook endpoint answers 405 or 404 for another method or path, 413 past + * a 1 MiB body (destroying the socket), 401 for a bad `X-Hub-Signature-256`, + * 400 without `X-GitHub-Delivery` or `X-GitHub-Event`, 200 for a delivery id + * among the last 2,000 or a `ping`, and 202 before it handles anything else. + * Nothing parses the body or touches the delivery ring before the signature + * is checked. */ import { createHmac, timingSafeEqual } from 'node:crypto'; @@ -39,10 +28,8 @@ import { type RepoRef, } from './types.js'; -/** §Ingress body cap: 1 MiB of raw delivery. */ export const MAX_BODY_BYTES = 1024 * 1024; -/** §Ingress: "a ring of the last 2000 ids dedups replays". */ export const DELIVERY_DEDUP_RING = 2000; const SIGNATURE_HEADER = 'x-hub-signature-256'; @@ -50,34 +37,20 @@ const SIGNATURE_PREFIX = 'sha256='; const DELIVERY_HEADER = 'x-github-delivery'; const EVENT_HEADER = 'x-github-event'; -/** What the ingress hands a normalized event to (.9 wires the pipeline). */ export type EventListener = (event: NormalizedEvent) => void | Promise<void>; export interface WebhookListenerOptions { - /** GITHUB_WEBHOOK_SECRET_FILE's contents. Secret: never logged. */ secret: string; - /** GITHUB_WEBHOOK_PATH; defaults to {@link DEFAULT_WEBHOOK_PATH}. */ path?: string; - /** GITHUB_REPOS. A delivery for anything else is dropped with one line. */ repos: readonly RepoRef[]; - /** Where a verified, allowed, fresh event goes. */ onEvent: EventListener; - /** Used to fill `checks.failed` on a `checks.completed` event. */ api?: Pick<GitHubApi, 'getCheckRuns'>; - /** - * The "one `checks.completed` per head sha" gate. Pass the poller's own gate - * to serialize the two ingress paths against each other; left out, the - * listener keeps a private one. - */ checkGate?: CheckGate; logger?: Logger; - /** Overridable in tests; the stamp on `NormalizedEvent.receivedAt`. */ now?: () => Date; - /** Overridable in tests. Defaults to {@link MAX_BODY_BYTES}. */ maxBodyBytes?: number; } -/** A bounded FIFO set: `add` returns true when the id was already present. */ export class DeliveryRing { private readonly seen = new Set<string>(); private readonly order: string[] = []; @@ -100,12 +73,6 @@ export class DeliveryRing { } } -/** - * The listener. `listen()` binds, `close()` unbinds, and `whenIdle()` waits - * for the post-response processing of every delivery accepted so far — the - * contract answers 2xx *before* the event is handled, so a test that wants - * to assert on the handling has to wait for it. - */ export class WebhookListener { private readonly secret: string; private readonly path: string; @@ -132,7 +99,6 @@ export class WebhookListener { this.maxBodyBytes = opts.maxBodyBytes ?? MAX_BODY_BYTES; } - /** Bind and resolve with the port actually bound (0 picks a free one). */ async listen(port: number, host = '127.0.0.1'): Promise<number> { if (this.server) throw new Error('webhook listener is already listening'); const server = createServer((req, res) => { @@ -161,14 +127,12 @@ export class WebhookListener { await this.whenIdle(); } - /** Resolve once every accepted delivery has finished being processed. */ async whenIdle(): Promise<void> { while (this.inFlight.size > 0) { await Promise.all([...this.inFlight]); } } - /** How many delivery ids the ring currently remembers (tests, diagnostics). */ get rememberedDeliveries(): number { return this.ring.size; } @@ -177,11 +141,8 @@ export class WebhookListener { return this.ring.capacity; } - // ── Request handling ── - private handle(req: IncomingMessage, res: ServerResponse): void { if (req.method !== 'POST') { - // A browser hitting the path is noise, not a delivery. respond(res, 405, 'method not allowed'); req.resume(); return; @@ -237,9 +198,7 @@ export class WebhookListener { }); } - /** Steps 3 to 6. `body` is the raw bytes; it is not parsed before step 6. */ private afterBody(req: IncomingMessage, res: ServerResponse, body: Buffer): void { - // 3. Signature over the raw bytes, before anything interprets them. if (!this.signatureValid(header(req, SIGNATURE_HEADER), body)) { this.logger.warn('webhook delivery rejected: X-Hub-Signature-256 did not verify', { delivery: header(req, DELIVERY_HEADER) ?? '(none)', @@ -256,21 +215,20 @@ export class WebhookListener { return; } - // 4. Replay. Only verified deliveries may write to the ring. if (this.ring.add(delivery)) { this.logger.info('webhook delivery dropped: replay of a delivery id already seen', { delivery, event }); respond(res, 200, 'duplicate delivery'); return; } - // 5. GitHub's own reachability check. + // GitHub's `ping` checks that the endpoint is reachable; it carries no + // event. if (event === 'ping') { this.logger.info('webhook ping answered', { delivery }); respond(res, 200, 'pong'); return; } - // 6. 2xx first, then the work. respond(res, 202, 'accepted'); const receivedAt = this.now().toISOString(); const task = this.process({ id: delivery, event, body, receivedAt }).catch((err: unknown) => { @@ -285,10 +243,9 @@ export class WebhookListener { } /** - * HMAC-SHA256 over the raw body, compared in constant time. A missing, - * misprefixed or wrong-length header is rejected without a compare (there - * is nothing of equal length to compare against, and the length of a - * signature is not a secret). + * GitHub signs the raw body with HMAC-SHA256 as `sha256=<hex>`. A header of + * the wrong form or length is refused before the constant-time compare, since + * a signature's length isn't secret. */ private signatureValid(provided: string | undefined, body: Buffer): boolean { if (!provided || !provided.startsWith(SIGNATURE_PREFIX)) return false; @@ -329,8 +286,6 @@ export class WebhookListener { return; } - // One stderr line per dropped delivery, naming the repository the - // allow-list does not hold (§Ingress). const repo = events[0].repo; if (!repoAllowed(repo, this.repos)) { this.logger.warn('webhook delivery dropped: repository outside GITHUB_REPOS', { @@ -341,8 +296,6 @@ export class WebhookListener { return; } - // The lookup also decides readiness: a sha with a suite still running is - // held back here, so the one emission it gets names every failed job. const resolved = this.api ? await resolveCheckEvents(events, this.api, this.logger) : events; for (const normalized of resolved) { @@ -351,11 +304,6 @@ export class WebhookListener { await this.onEvent(normalized); continue; } - // §Ingress wants one checks event per head sha, and two suites on one - // commit are two deliveries the delivery-id ring cannot collapse. The - // gate serializes this emission against every other one for the same - // sha — including a poll pass running right now — and drops it if what - // the listener already accepted covers it. const delivered = await this.checkGate.emit(normalized.repo, normalized.sha as string, mark, async () => { await this.onEvent(normalized); }); @@ -375,18 +323,12 @@ function header(req: IncomingMessage, name: string): string | undefined { return typeof value === 'string' ? value : Array.isArray(value) ? value[0] : undefined; } -/** Responses are plain text and never echo a header, a body or the secret. */ function respond(res: ServerResponse, status: number, message: string): void { if (res.writableEnded) return; res.writeHead(status, { 'content-type': 'text/plain; charset=utf-8' }); res.end(`${message}\n`); } -/** - * The listener a validated {@link Config} describes, or `null` when - * GITHUB_WEBHOOK_PORT is absent ("webhook listener off"). `readConfig` - * already guarantees a secret whenever the port is set. - */ export function webhookListenerFromConfig( config: Config, deps: Pick<WebhookListenerOptions, 'onEvent' | 'api' | 'logger' | 'checkGate'>, diff --git a/test/channels.test.ts b/test/channels.test.ts index 025f4b2..a6591c5 100644 --- a/test/channels.test.ts +++ b/test/channels.test.ts @@ -1,19 +1,3 @@ -/** - * Channels (TASK-SRHKG.1.5): registration, the label bound, incoming shape and - * thread-id rules, publish routing, refusals, typing. - * - * Both seams the package declares are exercised for real: - * - * - the host, over a `McplConnection` pair, so every descriptor and every - * incoming message is actually serialized to JSON-RPC and parsed back on - * the host side before it is asserted on (the same fake-host wiring - * test/harness.ts uses, kept here because the .1 skeleton's server does - * not route channel methods yet — .9 does that, and `harness()` then - * drives this module through the server); - * - GitHub, through the harness's {@link FakeGitHubApi}: no network, and the - * call log is what publish routing is asserted against. - */ - import assert from 'node:assert/strict'; import { PassThrough } from 'node:stream'; import { test } from 'node:test'; @@ -32,27 +16,13 @@ import { McplRpcError } from '../src/errors.js'; import { LIMITS, channelId, type EventMetadata, type NormalizedEvent, type ThreadRef } from '../src/types.js'; import { FakeGitHubApi, TEST_BOT, TEST_REPO, until } from './harness.js'; -// ───────────────────────────────────────────────────────────────────────────── -// Fake host -// ───────────────────────────────────────────────────────────────────────────── - interface FakeHost { - /** What ChannelRegistry sends through. */ conn: HostConnection; - /** Every request the registry sent, in order, as the host parsed it. */ saw: JsonRpcRequest[]; sent(m: string): JsonRpcRequest[]; - /** Channel ids the host answers `accepted: false` for. */ refuse: Set<string>; - /** Methods the host answers with a JSON-RPC error instead of a result. */ errorOn: Set<string>; - /** What the host answers to `channels/incoming`. */ acceptIncoming: boolean; - /** - * Stop answering: every request the host receives from now on is parked. - * The returned function sends every parked answer, in arrival order — the - * host that takes its time, so a test can decide what races what. - */ hold(): () => void; close(): void; } @@ -63,7 +33,6 @@ function fakeHost(): FakeHost { const serverConn = McplConnection.fromStreams(toServer, toHost); const hostConn = McplConnection.fromStreams(toHost, toServer); - /** Parked answers while the host is holding; see FakeHost.hold. */ let parked: (() => void)[] | null = null; const state: FakeHost = { @@ -123,20 +92,14 @@ function fakeHost(): FakeHost { return state; } -/** A logger that says nothing: these tests assert on traffic, not on stderr. */ const quiet = { debug() {}, info() {}, warn() {}, error() {} }; function registry(host: FakeHost): ChannelRegistry { return new ChannelRegistry({ host: host.conn, logger: quiet }); } -// ───────────────────────────────────────────────────────────────────────────── -// mcpl-core shape validation (acceptance 1) -// ───────────────────────────────────────────────────────────────────────────── - const DIRECTIONS = ['outbound', 'inbound', 'bidirectional']; -/** Every required member of mcpl-core's ChannelDescriptor, as the host got it. */ function assertValidDescriptor(value: unknown): ChannelDescriptor { assert.equal(typeof value, 'object'); const d = value as Record<string, unknown>; @@ -154,7 +117,6 @@ function assertValidDescriptor(value: unknown): ChannelDescriptor { return d as unknown as ChannelDescriptor; } -/** Every required member of mcpl-core's IncomingChannelMessage. */ function assertValidIncoming(value: unknown): IncomingChannelMessage { assert.equal(typeof value, 'object'); const m = value as Record<string, unknown>; @@ -183,10 +145,6 @@ function assertValidIncoming(value: unknown): IncomingChannelMessage { return m as unknown as IncomingChannelMessage; } -// ───────────────────────────────────────────────────────────────────────────── -// Fixtures -// ───────────────────────────────────────────────────────────────────────────── - const ISSUE: ThreadRef = { repo: TEST_REPO, number: 7, kind: 'issue' }; const PULL: ThreadRef = { repo: TEST_REPO, number: 12, kind: 'pull' }; @@ -225,10 +183,6 @@ function event(partial: Partial<NormalizedEvent> = {}): NormalizedEvent { const TEXT: ContentBlock[] = [{ type: 'text', text: 'rendered body' }]; -// ───────────────────────────────────────────────────────────────────────────── -// Registration -// ───────────────────────────────────────────────────────────────────────────── - test('registration is lazy: nothing is announced until a thread is touched', async () => { const host = fakeHost(); const reg = registry(host); @@ -286,9 +240,6 @@ test("a registration still in flight does not swallow the next touch's title and const reg = registry(host); const id = channelId(ISSUE); - // A tool touches the thread before anything knows its title, and the host - // takes its time answering; the event behind it closes the issue and names - // it. Both fields belong to the host, and neither may be lost to the race. const release = host.hold(); const first = reg.ensureChannel(ISSUE); await until(() => host.sent(method.CHANNELS_REGISTER).length === 1, 'the registration to reach the host'); @@ -382,10 +333,6 @@ test('the label title is bounded to 80 chars, notice included', async () => { host.close(); }); -// ───────────────────────────────────────────────────────────────────────────── -// channels/changed -// ───────────────────────────────────────────────────────────────────────────── - test('a title change sends channels/changed with the new label', async () => { const host = fakeHost(); const reg = registry(host); @@ -441,7 +388,6 @@ test('a channels/changed the host refuses leaves the acknowledged descriptor in 'the registry reports what the host acknowledged, not what it was told', ); - // A later touch with no title of its own still owes the host the update. host.refuse.clear(); await reg.ensureChannel(ISSUE); const changed = host.sent(method.CHANNELS_CHANGED); @@ -452,7 +398,6 @@ test('a channels/changed the host refuses leaves the acknowledged descriptor in ); assert.equal(reg.get(id)?.label, 'anima-research/rayforce#7: Second title'); - // Acknowledged now: a further touch owes nothing. await reg.ensureChannel(ISSUE); assert.equal(host.sent(method.CHANNELS_CHANGED).length, 2); host.close(); @@ -465,7 +410,6 @@ test('a channels/changed that errors in transport stays retryable', async () => await reg.ensureChannel(ISSUE, 'First title'); host.errorOn.add(method.CHANNELS_CHANGED); - // A host error on a label update is logged, not thrown: the channel works. await reg.ensureChannel(ISSUE, 'Second title'); assert.equal(host.sent(method.CHANNELS_CHANGED).length, 1); assert.equal(reg.get(id)?.label, 'anima-research/rayforce#7: First title', 'the cache did not advance'); @@ -491,10 +435,6 @@ test('an event that moves neither title nor state sends no channels/changed', as host.close(); }); -// ───────────────────────────────────────────────────────────────────────────── -// Incoming -// ───────────────────────────────────────────────────────────────────────────── - test('incoming carries the contract shape: sender, timestamp, tags, metadata', async () => { const host = fakeHost(); const reg = registry(host); @@ -566,10 +506,6 @@ test('a host that refuses the message reports it undelivered', async () => { host.close(); }); -// ───────────────────────────────────────────────────────────────────────────── -// threadId rules -// ───────────────────────────────────────────────────────────────────────────── - test('threadId rules: top-level comments carry none, review comments rt:, review summaries review:', () => { assert.equal(threadIdForEvent(event({ kind: 'comment.created' })), undefined); assert.equal(threadIdForEvent(event({ kind: 'issue.opened' })), undefined); @@ -633,10 +569,6 @@ test('a top-level comment sends no threadId member at all', async () => { host.close(); }); -// ───────────────────────────────────────────────────────────────────────────── -// Publish routing -// ───────────────────────────────────────────────────────────────────────────── - async function publishFixture(): Promise<{ host: FakeHost; reg: ChannelRegistry; api: FakeGitHubApi }> { const host = fakeHost(); const reg = registry(host); @@ -746,10 +678,6 @@ test('publish joins the text blocks and skips an empty turn without an error', a host.close(); }); -// ───────────────────────────────────────────────────────────────────────────── -// Refusals -// ───────────────────────────────────────────────────────────────────────────── - async function refusal(fn: () => Promise<unknown>): Promise<McplRpcError> { try { await fn(); @@ -850,10 +778,6 @@ test('a review-thread reply on an issue channel is refused with -32602', async ( host.close(); }); -// ───────────────────────────────────────────────────────────────────────────── -// Typing -// ───────────────────────────────────────────────────────────────────────────── - test('typing is a no-op that answers success', async () => { const host = fakeHost(); const reg = registry(host); diff --git a/test/content.test.ts b/test/content.test.ts index 466c540..0fce4f6 100644 --- a/test/content.test.ts +++ b/test/content.test.ts @@ -1,14 +1,3 @@ -/** - * content.ts — a render test for every `NormalizedEventKind`, the documented - * bounds (4k body, 40-line diff hunk, 50-entry list, 80-char title), Markdown - * and sticky-marker passthrough, and the promise that no raw payload field - * reaches a block. - * - * Long bodies are one repeated character (`FILL`), so "no rendered body - * exceeds 4000 chars" can be asserted directly on the output: the longest run - * of that character in a block is the longest body the renderer let through. - */ - import { strict as assert } from 'node:assert'; import { test } from 'node:test'; import { @@ -43,11 +32,9 @@ const ISSUE: ThreadRef = { repo: REPO, number: 7, kind: 'issue' }; const PULL: ThreadRef = { repo: REPO, number: 12, kind: 'pull' }; const URL = 'https://github.com/anima-research/rayforce/issues/7'; -/** One repeated character, so the longest run of it is the longest body shown. */ const FILL = 'z'; const HUGE_BODY = FILL.repeat(10_000); -/** The one text block a renderer returns. */ function only(blocks: ReturnType<typeof renderEvent>): string { assert.equal(blocks.length, 1); const block = blocks[0]; @@ -56,7 +43,6 @@ function only(blocks: ReturnType<typeof renderEvent>): string { return (block as { text: string }).text; } -/** The longest run of `FILL` in the text — the longest body that got through. */ function longestBody(text: string): number { return (text.match(new RegExp(`${FILL}+`, 'g')) ?? []).reduce( (max, run) => Math.max(max, run.length), @@ -81,16 +67,6 @@ function event( }; } -// ───────────────────────────────────────────────────────────────────────────── -// renderEvent: one row, and one test, per NormalizedEventKind -// ───────────────────────────────────────────────────────────────────────────── - -/** - * The payload each kind carries, the way the ingress (.3) fills it in. Kept - * here rather than imported from tags.test.ts so neither test file runs the - * other's cases; the coverage test below is what keeps both tables in step - * with `NORMALIZED_EVENT_KINDS`. - */ const RENDER_TABLE: Array<{ kind: NormalizedEvent['kind']; event?: Partial<NormalizedEvent> }> = [ { kind: 'issue.opened', event: { title: 'Rate limiter drops bursts', body: 'Repro inside.' } }, { kind: 'issue.edited', event: { title: 'Rate limiter drops bursts', body: 'Clarified.' } }, @@ -161,8 +137,6 @@ for (const row of RENDER_TABLE) { const text = only(renderEvent(ev)); const header = text.split('\n')[0]; - // The header line: repo#number (or repo, for a threadless event), kind, - // actor, link. if (ev.thread) assert.ok(header.startsWith(`anima-research/rayforce#${ev.thread.number} `), header); else assert.ok(header.startsWith('anima-research/rayforce '), header); assert.ok(header.includes(row.kind), header); @@ -203,10 +177,6 @@ test('an event that carries no body renders no body section', () => { assert.ok(!text.includes('(no body)')); }); -// ───────────────────────────────────────────────────────────────────────────── -// Bounds -// ───────────────────────────────────────────────────────────────────────────── - test('an event body is bounded to LIMITS.commentChars with a note', () => { const text = only(renderEvent(event('issue.opened', { body: HUGE_BODY }))); assert.ok(longestBody(text) <= LIMITS.commentChars); @@ -228,7 +198,6 @@ test('a diff hunk is bounded to 40 lines with a note', () => { const hunk = Array.from({ length: 200 }, (_, i) => `+line ${i}`).join('\n'); const bounded = boundedDiffHunk(hunk); const lines = bounded.split('\n'); - // 40 kept lines plus the one-line note. assert.equal(lines.length, CONTENT_LIMITS.diffHunkLines + 1); assert.ok(lines[lines.length - 1].includes('160 lines dropped'), lines[lines.length - 1]); assert.ok(!bounded.includes('+line 40')); @@ -269,10 +238,6 @@ test('a review comment with no line number renders path:?', () => { assert.ok(!text.includes('```diff'), 'an empty hunk renders no fence'); }); -// ───────────────────────────────────────────────────────────────────────────── -// Passthrough and leak guards -// ───────────────────────────────────────────────────────────────────────────── - test('Markdown and HTML comments (sticky markers) pass through verbatim', () => { const body = [ '## Heading', @@ -315,8 +280,6 @@ test('a body is rendered byte for byte: indentation, tabs and trailing spaces su ]; for (const [name, text] of rendered) { assert.ok(text.includes(raw), `${name} altered the body`); - // The body is last in every block, so an exact tail is an exact body: a - // stripped indent or a swallowed trailing space fails here. assert.ok(text.endsWith(raw), `${name} did not end with the body verbatim`); } }); @@ -326,7 +289,6 @@ test('only an all-whitespace body counts as empty', () => { const text = only(renderEvent(event('comment.created', { body: blank }))); assert.ok(text.endsWith('(no body)'), JSON.stringify(blank)); } - // Whitespace *around* content is content, and is kept. const padded = ' \n x '; assert.ok(only(renderEvent(event('comment.created', { body: padded }))).endsWith(padded)); }); @@ -350,10 +312,6 @@ test('no raw payload field reaches a rendered block', () => { } }); -// ───────────────────────────────────────────────────────────────────────────── -// Tool-result renderers -// ───────────────────────────────────────────────────────────────────────────── - function issue(over: Partial<Issue> = {}): Issue { return { number: 7, @@ -409,8 +367,6 @@ function comment(over: Partial<Comment> = {}): Comment { function reviewThread(over: Partial<ReviewThread> = {}): ReviewThread { return { - // The GraphQL node id resolve_thread/unresolve_thread take — deliberately - // unlike the `rt:901` reply id, so a test cannot pass on one by accident. id: 'PRRT_graphql_node_123', path: 'src/limiter.ts', line: 42, @@ -523,9 +479,6 @@ test('renderReviewThreads shows path, side, resolution, both ids and bounds bodi }); test('renderReviewThreads carries the GraphQL node id beside the rt: reply id', () => { - // Two different tools, two different ids: resolve_thread/unresolve_thread - // take ReviewThread.id (a GraphQL node id), reply_in_thread takes the - // channel thread id rt:<root comment id>. Dropping either strands the agent. const threads: ReviewThread[] = [ reviewThread({ id: 'PRRT_kwDOAbCdEf4Axxxx', comments: [reviewComment({ id: 901 })] }), reviewThread({ @@ -615,10 +568,6 @@ test('renderPullRequestFiles lists names and stats only, capped', () => { assert.ok(only(renderPullRequestFiles([])).includes('(no files)')); }); -// ───────────────────────────────────────────────────────────────────────────── -// The bound holds for every kind, not just the ones with a body in the table -// ───────────────────────────────────────────────────────────────────────────── - test('no rendered event body exceeds LIMITS.commentChars, on any kind', () => { for (const kind of NORMALIZED_EVENT_KINDS) { const text = only(renderEvent(event(kind, { body: HUGE_BODY, title: HUGE_BODY }))); diff --git a/test/lost-answer.test.ts b/test/lost-answer.test.ts index f229511..acd3e3f 100644 --- a/test/lost-answer.test.ts +++ b/test/lost-answer.test.ts @@ -31,8 +31,6 @@ writeFileSync( type Answer = { status: number; body?: unknown } | 'lost'; -// 'lost' keeps the whole request and closes the socket without answering, as -// a connection that drops after GitHub stored a write does. class FakeGitHub { readonly received: { method: string; path: string; body: string }[] = []; private readonly answers = new Map<string, Answer>([ diff --git a/test/normalize.test.ts b/test/normalize.test.ts index 0cebf90..1d5b283 100644 --- a/test/normalize.test.ts +++ b/test/normalize.test.ts @@ -1,15 +1,3 @@ -/** - * Fixture → NormalizedEvent (TASK-SRHKG.1.3). - * - * Every file in test/fixtures/ is a hand-written payload shaped like GitHub's - * documented webhook JSON — `octocat`/`hubot` placeholders, invented ids, no - * real text and no secret material. The table below is the documentation of - * the mapping: one row per fixture, naming the kind and every field the - * acceptance criteria call for (thread, actor, sha, review, reviewComment, - * checks). A fixture with no row fails the "every fixture is covered" test, - * so a new payload cannot be added without saying what it normalizes to. - */ - import { strict as assert } from 'node:assert'; import { readFileSync, readdirSync } from 'node:fs'; import { dirname, join } from 'node:path'; @@ -51,7 +39,6 @@ const HEAD = '4d7b2e8c1a93f05627de4b1c8a0f3927e5c6d418'; const FIRST_HEAD = '1f0c9a4d2b7e55c3a81d6e40b9f27c3d5a8e1b02'; const DEV_SHA = '9ab3c1d7e5f20418b6c93d7a2e1f08c4d6b5a397'; -/** `issue_comment.created.on_pull.json` → the `issue_comment` event. */ function eventNameOf(fixture: string): string { return fixture.split('.', 1)[0]; } @@ -68,11 +55,8 @@ function load(fixture: string): WebhookDelivery { const ISSUE_41: ThreadRef = { repo: TEST_REPO, number: 41, kind: 'issue' }; const PULL_58: ThreadRef = { repo: TEST_REPO, number: 58, kind: 'pull' }; -/** What each fixture must normalize to. `events: []` means "dropped". */ interface Expectation { - /** One entry per event the delivery produces, in order. */ events: Partial<NormalizedEvent>[]; - /** Set when the fixture is deliberately not for the allow-listed repo. */ foreignRepo?: boolean; } @@ -106,8 +90,6 @@ const EXPECTED: Record<string, Expectation> = { actor: { login: 'hubot', isBot: false }, body: '@rayforce-clone[bot] can you reproduce this on dev?', html_url: 'https://github.com/anima-research/rayforce/issues/41#issuecomment-2200000007', - // Amendment 2b: a top-level comment carries its own id, so the - // turn-order rule can place it in the thread. comment: { id: 2200000007 }, }, ], @@ -167,7 +149,7 @@ const EXPECTED: Record<string, Expectation> = { kind: 'review.submitted', thread: PULL_58, actor: { login: 'octocat', isBot: false }, - // The commit reviewed, not the PR's current head (which is HEAD). + // The commit reviewed, not the pull request's current head. sha: FIRST_HEAD, review: { id: 2400000044, state: 'approved' }, }, @@ -214,7 +196,6 @@ const EXPECTED: Record<string, Expectation> = { events: [ { kind: 'checks.completed', - // The suite's head sha is the head of PR 58, so the event is threaded. thread: PULL_58, actor: { login: 'rayforce-clone[bot]', isBot: true }, sha: HEAD, @@ -227,8 +208,6 @@ const EXPECTED: Record<string, Expectation> = { { kind: 'checks.completed', thread: PULL_58, - // A second *failing* workflow on the same commit: the one whose jobs - // were being swallowed by the sha dedup (review round 2, finding 2). sha: HEAD, checks: { conclusion: 'failure', failed: [] }, }, @@ -239,8 +218,6 @@ const EXPECTED: Record<string, Expectation> = { { kind: 'checks.completed', thread: PULL_58, - // A second workflow on the same commit: same head sha as - // check_suite.completed.json, different suite and delivery. sha: HEAD, checks: { conclusion: 'success', failed: [] }, }, @@ -264,8 +241,6 @@ const EXPECTED: Record<string, Expectation> = { kind: 'checks.completed', thread: PULL_58, sha: HEAD, - // The run that just failed is pre-listed; resolveCheckEvents replaces it - // with every failure on the sha. checks: { conclusion: 'failure', failed: [{ name: 'test (ubuntu-latest)', url: 'https://github.com/anima-research/rayforce/runs/2700001234' }], @@ -285,7 +260,7 @@ const EXPECTED: Record<string, Expectation> = { }, ], }, - // GitHub's reachability check: webhook.ts answers it, nothing is emitted. + // GitHub's ping checks the webhook's reachability; it carries no event. 'ping.json': { events: [] }, 'issues.opened.foreign_repo.json': { events: [{ kind: 'issue.opened', thread: { repo: { owner: 'other-org', name: 'other-repo' }, number: 3, kind: 'issue' } }], @@ -319,13 +294,11 @@ for (const fixture of fixtureFiles()) { `${fixture} event ${i}: field ${field}`, ); } - // Fields every event carries, whatever the fixture. assert.equal(event.id, delivery.id); assert.equal(event.receivedAt, RECEIVED_AT); assert.ok(event.html_url.startsWith('https://github.com/'), 'html_url is a github URL'); assert.ok(event.actor.login.length > 0, 'actor has a login'); if (!expectation.foreignRepo) assert.deepEqual(event.repo, TEST_REPO); - // The contract keeps `raw` on the event (and nothing downstream reads it). assert.equal(event.raw, delivery.payload); }); }); @@ -333,13 +306,9 @@ for (const fixture of fixtureFiles()) { test('an unhandled event or action is dropped rather than guessed at', () => { const payload = JSON.parse(readFileSync(join(FIXTURES, 'issues.opened.json'), 'utf8')); - // An action with no kind in the contract. assert.deepEqual(normalizeWebhook({ ...load('issues.opened.json'), payload: { ...payload, action: 'labeled' } }), []); - // An event with no mapping at all. assert.deepEqual(normalizeWebhook({ ...load('issues.opened.json'), event: 'installation_repositories' }), []); - // A payload with no repository cannot be placed. assert.deepEqual(normalizeWebhook({ ...load('issues.opened.json'), payload: { action: 'opened' } }), []); - // Nothing throws on junk. assert.deepEqual(normalizeWebhook({ ...load('issues.opened.json'), payload: 42 }), []); assert.deepEqual(normalizeWebhook({ ...load('issues.opened.json'), payload: null }), []); }); @@ -374,7 +343,6 @@ test('actor and repo extraction', () => { assert.deepEqual(actorFrom({ login: 'some-app', type: 'Bot' }), { login: 'some-app', isBot: true }); assert.equal(actorFrom({}), undefined); assert.deepEqual(repoFrom({ repository: { full_name: 'anima-research/rayforce' } }), TEST_REPO); - // No full_name: owner.login + name is the fallback. assert.deepEqual(repoFrom({ repository: { owner: { login: 'a' }, name: 'b' } }), { owner: 'a', name: 'b' }); assert.equal(repoFrom({ repository: { full_name: 'not-a-slug' } }), undefined); }); @@ -385,10 +353,6 @@ test('threadKindFromUrl tells an issue comment on a PR from one on an issue', () assert.equal(threadKindFromUrl(undefined), 'issue'); }); -// ───────────────────────────────────────────────────────────────────────────── -// Poll rows (contract amendment 1) -// ───────────────────────────────────────────────────────────────────────────── - const POLLED_ISSUE: PolledIssue = { id: 2100000001, number: 41, @@ -402,7 +366,6 @@ const POLLED_ISSUE: PolledIssue = { html_url: 'https://github.com/anima-research/rayforce/issues/41', }; -/** The cursor the poll pass queried with, after POLLED_ISSUE was created. */ const SINCE = '2026-10-13T08:00:00.000Z'; test('poll ids and dedup keys have the documented shape', () => { @@ -413,7 +376,8 @@ test('poll ids and dedup keys have the documented shape', () => { test('a polled issue row normalizes by state, and an edit keeps issue.edited', () => { const open = normalizePolledIssue(TEST_REPO, POLLED_ISSUE, RECEIVED_AT, SINCE); assert.ok(open); - // Created before the cursor: a poll cannot say which field changed. + // Created before the cursor: GitHub's list gives updated_at, not which + // field changed. assert.equal(open.kind, 'issue.edited'); assert.equal(open.id, 'poll:issue:2100000001:2026-10-13T09:00:00Z'); assert.deepEqual(open.thread, ISSUE_41); @@ -426,35 +390,28 @@ test('a polled issue row normalizes by state, and an edit keeps issue.edited', ( assert.equal(closed.kind, 'issue.closed'); }); -// ── Amendment 2a: created_at against the cursor ── - test('an issue created at or after the cursor is issue.opened (amendment 2a)', () => { const after = normalizePolledIssue(TEST_REPO, { ...POLLED_ISSUE, createdAt: '2026-10-13T08:30:00Z' }, RECEIVED_AT, SINCE); assert.ok(after); assert.equal(after.kind, 'issue.opened'); - // The boundary is inclusive: created exactly at the cursor is still new, - // because `since` is the instant the previous pass ran *at*. const exactly = normalizePolledIssue(TEST_REPO, { ...POLLED_ISSUE, createdAt: SINCE }, RECEIVED_AT, SINCE); assert.ok(exactly); assert.equal(exactly.kind, 'issue.opened'); - // One millisecond before it is not. const before = normalizePolledIssue(TEST_REPO, { ...POLLED_ISSUE, createdAt: '2026-10-13T07:59:59.999Z' }, RECEIVED_AT, SINCE); assert.ok(before); assert.equal(before.kind, 'issue.edited'); }); test('created_at is compared as an instant, not as a string (amendment 2a)', () => { - // The regression a string compare produces: GitHub stamps the row without - // milliseconds, the cursor carries them, and `'Z'` > `'.'` lexicographically. - // 12:00:00Z is *before* 12:00:00.500Z and must read as an edit. + // GitHub stamps a row without milliseconds while the cursor carries them, + // and 'Z' sorts after '.', so 12:00:00Z must compare before 12:00:00.500Z. const row = { ...POLLED_ISSUE, createdAt: '2026-10-13T12:00:00Z' }; const edited = normalizePolledIssue(TEST_REPO, row, RECEIVED_AT, '2026-10-13T12:00:00.500Z'); assert.ok(edited); assert.equal(edited.kind, 'issue.edited'); - // And an unparseable stamp falls back to the pre-amendment kind. const unparseable = normalizePolledIssue(TEST_REPO, { ...POLLED_ISSUE, createdAt: '' }, RECEIVED_AT, SINCE); assert.ok(unparseable); assert.equal(unparseable.kind, 'issue.edited'); @@ -475,7 +432,6 @@ test('a polled pull-request row normalizes by state and keeps the head sha', () assert.deepEqual(open.thread, PULL_58); assert.equal(open.sha, HEAD); - // Closed stays closed however old it is. const closed = normalizePolledIssue(TEST_REPO, { ...row, state: 'closed', createdAt: '2026-10-01T00:00:00Z' }, RECEIVED_AT, SINCE); assert.ok(closed); assert.equal(closed.kind, 'pull.closed'); @@ -491,17 +447,14 @@ test('an open pull-request row seen again is synchronize on a new head, and drop createdAt: '2026-10-12T08:00:00Z', }; - // Head moved since the last pass recorded it: new commits. const moved = normalizePolledIssue(TEST_REPO, row, RECEIVED_AT, SINCE, 'a'.repeat(40)); assert.ok(moved); assert.equal(moved.kind, 'pull.synchronize'); assert.equal(moved.sha, HEAD); - // Same head: the row was touched by a comment, which the pass collects as - // its own row. Nothing to announce here. + // Same head: a comment bumped the row's updated_at. assert.equal(normalizePolledIssue(TEST_REPO, row, RECEIVED_AT, SINCE, HEAD), null); - // An unknown head cannot be shown to have moved, so it drops the same way. assert.equal(normalizePolledIssue(TEST_REPO, { ...row, headSha: undefined }, RECEIVED_AT, SINCE, HEAD), null); assert.equal(normalizePolledIssue(TEST_REPO, { ...row, headSha: '' }, RECEIVED_AT, SINCE, undefined), null); }); @@ -515,13 +468,10 @@ test('a polled top-level comment carries its own id, a review comment does not ( updatedAt: '2026-10-13T09:00:00Z', html_url: 'https://github.com/anima-research/rayforce/issues/41#issuecomment-2200000031', }; - // Amendment 2b places a comment in its thread's turn order, which needs its - // id; a `since` row has always carried one, it was just thrown away. assert.deepEqual(normalizePolledComment(TEST_REPO, top, 'issue_comment', RECEIVED_AT).comment, { id: 2200000031 }); - // A review comment's id stays in `reviewComment` alone: the two endpoints - // number their comments separately and an id from one must never be read as - // an id from the other. + // GitHub numbers issue comments and review comments separately, so an id + // from one endpoint must never be read as an id from the other. const review = normalizePolledComment(TEST_REPO, { ...top, path: 'src/lib.rs' }, 'review_comment', RECEIVED_AT); assert.equal(review.comment, undefined); assert.equal(review.reviewComment?.id, 2200000031); @@ -592,13 +542,8 @@ test('a polled check summary is keyed on the conclusion, not on a timestamp', () test('isCompletedSummary is what stops a pending head becoming checks.completed', () => { const settled: ChecksSummary = { sha: HEAD, conclusion: 'success', total: 2, failed: [] }; assert.equal(isCompletedSummary(settled), true); - // Runs exist but have not finished: `checks.completed` would be a lie. assert.equal(isCompletedSummary({ ...settled, conclusion: null }), false); - // No runs at all on the sha. assert.equal(isCompletedSummary({ sha: HEAD, conclusion: null, total: 0, failed: [] }), false); - // `normalizePolledChecks` only accepts the narrowed type, so the poller - // cannot build a pending event even by mistake — this is a type-level - // guarantee, asserted here through the guard that produces the narrowing. if (isCompletedSummary(settled)) { assert.match( normalizePolledChecks(TEST_REPO, PULL_58, settled, 'u', RECEIVED_AT).id, @@ -607,10 +552,6 @@ test('isCompletedSummary is what stops a pending head becoming checks.completed' } }); -// ───────────────────────────────────────────────────────────────────────────── -// The checks lifecycle: marks, serialization, supersession -// ───────────────────────────────────────────────────────────────────────────── - function mark(rank: number, ...failed: string[]): CheckMark { return { rank, failed: [...new Set(failed)].sort() }; } @@ -627,7 +568,6 @@ test('checkMarkOf reads a mark off a checks event and nothing off any other', () const [green] = normalizeWebhook(load('check_suite.completed.second_suite.json')); assert.deepEqual(checkMarkOf(green), mark(RANK_SUCCESS)); const [run] = normalizeWebhook(load('check_run.completed.json')); - // The run pre-lists itself as failed, so the mark names that job. assert.deepEqual(checkMarkOf(run), mark(RANK_FAILURE, 'test (ubuntu-latest)')); const [issue] = normalizeWebhook(load('issues.opened.json')); assert.equal(checkMarkOf(issue), null); @@ -651,28 +591,19 @@ test('a mark names its failed jobs sorted and deduplicated, so it compares by se }); test('coversMark is what decides dedup against supersession', () => { - // Identical: nothing new to say. assert.equal(coversMark(mark(RANK_FAILURE, 'lint'), mark(RANK_FAILURE, 'lint')), true); - // A stronger conclusion is news. assert.equal(coversMark(mark(RANK_SUCCESS), mark(RANK_FAILURE)), false); - // A weaker one is not: a second workflow finishing green says nothing new. assert.equal(coversMark(mark(RANK_FAILURE), mark(RANK_SUCCESS)), true); assert.equal(coversMark(mark(RANK_SKIPPED), mark(RANK_SUCCESS)), true); assert.equal(coversMark(mark(RANK_FAILURE), mark(RANK_CANCELLED)), true); - // A job the listener has not heard about is news, whatever the rank. assert.equal(coversMark(mark(RANK_FAILURE, 'lint'), mark(RANK_FAILURE, 'lint', 'test')), false); assert.equal(coversMark(mark(RANK_FAILURE), mark(RANK_FAILURE, 'lint')), false); - // A shorter view of the same failures is not. assert.equal(coversMark(mark(RANK_FAILURE, 'lint', 'test'), mark(RANK_FAILURE, 'lint')), true); assert.equal(coversMark(mark(RANK_FAILURE, 'lint', 'test'), mark(RANK_FAILURE)), true); - // "Nothing settled yet" is covered by anything at all. assert.equal(coversMark(mark(RANK_SUCCESS), mark(RANK_NONE)), true); assert.equal(coversMark(mark(RANK_NONE), mark(RANK_SUCCESS)), false); }); -// ── The gate: commit only after an accepted delivery ── - -/** Deliver `m` for `sha` through `gate`, recording the order of events. */ async function emit( gate: CheckGate, sha: string, @@ -701,7 +632,6 @@ test('a refused delivery commits nothing, so the next emission still delivers', ); assert.equal(gate.committedMark(TEST_REPO, HEAD), undefined, 'a refusal leaves no trace'); assert.equal(gate.size, 0); - // The retry — or the other ingress path — gets to deliver it. assert.equal(await emit(gate, HEAD, mark(RANK_FAILURE, 'lint')), true); assert.deepEqual(gate.committedMark(TEST_REPO, HEAD), mark(RANK_FAILURE, 'lint')); }); @@ -717,7 +647,6 @@ test('the gate drops an emission the committed mark covers, without calling the assert.equal(await emit(gate, HEAD, mark(RANK_FAILURE, 'lint'), deliver), false); assert.equal(await emit(gate, HEAD, mark(RANK_SUCCESS), deliver), false); assert.equal(called, 0, 'the listener is never told something it already has'); - // A new job name, or a worse conclusion, still gets through. assert.equal(await emit(gate, HEAD, mark(RANK_FAILURE, 'lint', 'test', 'audit'), deliver), true); assert.equal(called, 1); }); @@ -726,9 +655,7 @@ test('a committed mark accumulates, so an older result cannot undo a newer one', const gate = new CheckGate(); await emit(gate, HEAD, mark(RANK_FAILURE, 'test')); await emit(gate, HEAD, mark(RANK_FAILURE, 'lint')); - // The record is the union, not the last word. assert.deepEqual(gate.committedMark(TEST_REPO, HEAD), mark(RANK_FAILURE, 'lint', 'test')); - // And a success arriving late does not lower the conclusion. assert.equal(await emit(gate, HEAD, mark(RANK_SUCCESS)), false); assert.deepEqual(gate.committedMark(TEST_REPO, HEAD), mark(RANK_FAILURE, 'lint', 'test')); }); @@ -751,17 +678,13 @@ test('the gate forgets the oldest head sha past its capacity', async () => { assert.equal(gate.size, 3); await emit(gate, 'd', mark(RANK_SUCCESS)); assert.equal(gate.size, 3); - // `a` was evicted, so it is news again; `d` is still remembered. assert.equal(gate.committedMark(TEST_REPO, 'a'), undefined); assert.deepEqual(gate.committedMark(TEST_REPO, 'd'), mark(RANK_SUCCESS)); - // Being emitted again does not renew a sha's lease: `b` stays the oldest. await emit(gate, 'b', mark(RANK_FAILURE)); await emit(gate, 'e', mark(RANK_SUCCESS)); assert.equal(gate.committedMark(TEST_REPO, 'b'), undefined, 'b was evicted despite re-emitting'); }); -// ── The gate: serialization ── - test('two emissions for one head sha run one after another, never interleaved', async () => { const gate = new CheckGate(); const trace: string[] = []; @@ -775,7 +698,6 @@ test('two emissions for one head sha run one after another, never interleaved', await firstHeld; trace.push('first out'); }); - // Queued behind the first, even though it was started while that one blocks. const second = emit(gate, HEAD, mark(RANK_FAILURE, 'lint', 'test'), async () => { trace.push('second in'); trace.push('second out'); @@ -800,7 +722,6 @@ test('emissions for different head shas do not wait for each other', async () => const first = emit(gate, HEAD, mark(RANK_FAILURE), async () => { await firstHeld; }); - // A different sha, so it runs now rather than queueing. assert.equal(await emit(gate, DEV_SHA, mark(RANK_SUCCESS)), true); assert.equal(gate.inFlight, 1, 'only the blocked sha still has a queue'); letFirstFinish?.(); @@ -808,8 +729,6 @@ test('emissions for different head shas do not wait for each other', async () => }); test('an emission queued behind a refused one still runs, and sees no mark', async () => { - // The round-3 regression in miniature: a refusal must not stall the queue - // behind it, and must not look to that queue like a delivery. const gate = new CheckGate(); let letFirstFail: (() => void) | undefined; const firstHeld = new Promise<void>((resolve) => { @@ -833,9 +752,6 @@ test('an emission queued behind a refused one still runs, and sees no mark', asy }); test('a refusal cannot erase what a later accepted emission committed', async () => { - // Round-3 finding 2: A blocks on [lint], B delivers the fuller - // [lint, test] and is accepted, then A is refused. A never wrote anything, - // so B's record stands and the next identical delivery dedups. const gate = new CheckGate(); let letAFail: (() => void) | undefined; const aHeld = new Promise<void>((resolve) => { @@ -852,7 +768,6 @@ test('a refusal cannot erase what a later accepted emission committed', async () await assert.rejects(() => a, /refused A/); assert.equal(await b, true); assert.deepEqual(gate.committedMark(TEST_REPO, HEAD), mark(RANK_FAILURE, 'lint', 'test')); - // C: the same result again, which must now be a duplicate. let cDelivered = false; assert.equal( await emit(gate, HEAD, mark(RANK_FAILURE, 'lint', 'test'), async () => { @@ -871,10 +786,6 @@ test('supersession is decided inside the queue, against the mark committed by th }); let secondDelivered = false; - // The first emission will commit [lint, test]; the second, queued behind it - // with only [lint], must read the *committed* mark when its turn comes and - // find itself covered — not the empty record it would have seen at queue - // time. const first = emit(gate, HEAD, mark(RANK_FAILURE, 'lint', 'test'), async () => { await firstHeld; }); @@ -888,14 +799,8 @@ test('supersession is decided inside the queue, against the mark committed by th assert.equal(secondDelivered, false); }); -// ───────────────────────────────────────────────────────────────────────────── -// resolveCheckEvents: enrichment is also the readiness gate -// ───────────────────────────────────────────────────────────────────────────── - test('resolveCheckEvents holds back a head whose other suite is still running', async () => { const api = new FakeGitHubApi(); - // Runs exist on the sha but nothing has settled: reporting now would spend - // the sha's one emission on a partial job list. api.seedCheckRuns(TEST_REPO, { sha: HEAD, conclusion: null, @@ -941,9 +846,8 @@ test('resolveCheckEvents forwards the event unchanged when the lookup throws', a }); test('resolveCheckEvents keeps the payload conclusion when the API knows no runs', async () => { - // The fake answers `{ total: 0 }` for an unseeded sha, the way a 200 with an - // empty list does; the payload is then all there is to go on, and holding - // the event back would mean never reporting it. + // GitHub answers a sha with no check runs with an empty list, as the fake + // does for an unseeded sha. const [event] = await resolveCheckEvents(normalizeWebhook(load('check_suite.completed.json')), new FakeGitHubApi()); assert.equal(event.checks?.conclusion, 'failure'); assert.deepEqual(event.checks?.failed, []); @@ -959,12 +863,9 @@ test('checksPollKey and the polled event id carry the conclusion, the job count assert.match(checksPollKey(one), new RegExp(`^checks:${HEAD}:failure:1:[0-9a-f]{12}$`)); const two = { ...one, failed: [...one.failed, { name: 'test', url: 'u' }] }; assert.match(checksPollKey(two), new RegExp(`^checks:${HEAD}:failure:2:[0-9a-f]{12}$`)); - // Same count, different job (review round 4): a different key. const other = { ...one, failed: [{ name: 'test', url: 'u' }] }; assert.notEqual(checksPollKey(other), checksPollKey(one)); - // The fingerprint is order-independent and ignores urls. const reordered = { ...two, failed: [{ name: 'test', url: 'x' }, { name: 'lint', url: 'y' }] }; assert.equal(checksPollKey(reordered), checksPollKey(two)); - // The event id is the key, so the durable dedup ring and the event agree. assert.equal(normalizePolledChecks(TEST_REPO, PULL_58, two, 'u', RECEIVED_AT).id, `poll:${checksPollKey(two)}`); }); diff --git a/test/poller.test.ts b/test/poller.test.ts index c8933a0..b1a24a4 100644 --- a/test/poller.test.ts +++ b/test/poller.test.ts @@ -1,16 +1,3 @@ -/** - * Polling ingress (TASK-SRHKG.1.3, contract amendment 1). - * - * The two acceptance properties are asserted directly: - * - * - the interval floor, including what an `X-Poll-Interval` may and may not - * do to it (`observePollInterval`); - * - "marks read only after delivery" — the amendment's equivalent of a - * notification mark-read is the dedup gate and the cursor, so a key is - * written only once the listener took its event, and a failed delivery is - * re-attempted on the next pass instead of being consumed. - */ - import { strict as assert } from 'node:assert'; import { test } from 'node:test'; import { @@ -35,16 +22,10 @@ import { FakeGitHubApi, TEST_BOT, TEST_REPO } from './harness.js'; const OTHER_REPO = { owner: 'other-org', name: 'other-repo' }; -/** - * `IngressState` over a Map, with `StateStore`'s own gate semantics: - * `seenPollKey` records on a miss, `snapshot()` only reads. - */ class MemoryIngressState implements IngressState { private cursor: string | null = null; private readonly keys: string[] = []; - /** Amendment 2a: the last polled head of each pull request, by channel id. */ readonly heads = new Map<string, string>(); - /** Every `setLastPollAt`, in order — the cursor's whole history. */ readonly cursorWrites: string[] = []; constructor(cursor: string | null = null) { @@ -105,7 +86,6 @@ interface Rig { state: MemoryIngressState; poller: Poller; events: NormalizedEvent[]; - /** Every sha `getCheckRuns` was asked about, in order, across all passes. */ checkCalls: string[]; } @@ -120,10 +100,6 @@ interface RigOptions { checkGate?: CheckGate; onEvent?: (event: NormalizedEvent) => void; repos?: readonly { owner: string; name: string }[]; - /** - * Stand in for `getCheckRuns`. `call` is the 1-based number of the lookup - * for this rig, so a test can script "throw once, then answer". - */ getCheckRuns?: (repo: RepoRef, sha: string, call: number) => Promise<ChecksSummary>; } @@ -133,8 +109,6 @@ function rig(opts: RigOptions = {}): Rig { const events: NormalizedEvent[] = []; const now = new Date(opts.now ?? '2026-10-14T00:00:00.000Z'); const checkCalls: string[] = []; - // Delegate to the fake (so its own call log keeps working) but record the - // shas here too, because the backlog's drain order is the thing under test. const pollerApi: PollerApi = { listIssuesSince: (repo, since) => api.listIssuesSince(repo, since), listIssueCommentsSince: (repo, since) => api.listIssueCommentsSince(repo, since), @@ -162,7 +136,6 @@ function rig(opts: RigOptions = {}): Rig { return { api, state, poller, events, checkCalls }; } -/** A completed summary for `sha`, failing on one job. */ function failedSummary(sha: string): ChecksSummary { return { sha, @@ -172,13 +145,7 @@ function failedSummary(sha: string): ChecksSummary { }; } -// ───────────────────────────────────────────────────────────────────────────── -// The interval floor and X-Poll-Interval -// ───────────────────────────────────────────────────────────────────────────── - test('the effective interval never drops below the 60 s floor', () => { - // readConfig already refuses a sub-floor GITHUB_POLL_INTERVAL_MS; the poller - // does not rely on that and floors it again. assert.equal(rig({ intervalMs: 1 }).poller.effectiveIntervalMs, MIN_POLL_INTERVAL_MS); assert.equal(rig({ intervalMs: MIN_POLL_INTERVAL_MS }).poller.effectiveIntervalMs, MIN_POLL_INTERVAL_MS); assert.equal(rig({ intervalMs: 300_000 }).poller.effectiveIntervalMs, 300_000); @@ -188,13 +155,10 @@ test('the effective interval never drops below the 60 s floor', () => { test('X-Poll-Interval raises the interval and can never lower it', () => { const { poller } = rig({ intervalMs: 300_000 }); - // Below the configured interval: honoured (recorded) but not obeyed downward. assert.equal(poller.observePollInterval('60'), 300_000); assert.equal(poller.observePollInterval('120'), 300_000); - // Above it: obeyed. assert.equal(poller.observePollInterval('900'), 900_000); assert.equal(poller.effectiveIntervalMs, 900_000); - // And a later smaller value drops back only to the configured floor. assert.equal(poller.observePollInterval('65'), 300_000); }); @@ -222,10 +186,6 @@ test('GITHUB_POLL_INTERVAL_MS = 0 is polling off and start() does nothing', asyn assert.equal(rig({ intervalMs: MIN_POLL_INTERVAL_MS }).poller.enabled, true); }); -// ───────────────────────────────────────────────────────────────────────────── -// The cursor -// ───────────────────────────────────────────────────────────────────────────── - test('a pass queries every allow-listed repo since the cursor and advances it', async () => { const r = rig({ cursor: '2026-10-13T00:00:00.000Z', now: '2026-10-14T00:00:00.000Z' }); const result = await r.poller.pollOnce(); @@ -271,17 +231,11 @@ test('only the allow-listed repositories are ever asked', async () => { for (const call of api.calls) assert.deepEqual(call.args[0], TEST_REPO); }); -// ───────────────────────────────────────────────────────────────────────────── -// Rows → events -// ───────────────────────────────────────────────────────────────────────────── - -// ── Amendment 2a through the real poller ── - test('a freshly opened issue arrives as issue.opened, an older one as issue.edited (amendment 2a)', async () => { const api = new FakeGitHubApi(); - // Created after the cursor: this pass is the first that could have seen it. api.seedIssue({ number: 41, createdAt: '2026-10-13T10:00:00.000Z', updatedAt: '2026-10-13T10:00:00.000Z' }); - // Created before it, touched since: an edit, and a poll cannot say of what. + // Created before the cursor and updated since: GitHub's list gives + // updated_at, not what changed. api.seedIssue({ number: 42, createdAt: '2026-10-01T00:00:00.000Z', updatedAt: '2026-10-13T11:00:00.000Z' }); const r = rig({ api, cursor: '2026-10-13T09:00:00.000Z' }); @@ -290,7 +244,6 @@ test('a freshly opened issue arrives as issue.opened, an older one as issue.edit result.delivered.map((e) => `${e.thread?.number}:${e.kind}`), ['41:issue.opened', '42:issue.edited'], ); - // The tag the recipes key on follows the kind. assert.equal(result.delivered[0].kind, 'issue.opened'); }); @@ -299,7 +252,6 @@ test('an open pull request whose head did not move is dropped on the next pass ( const pull = api.seedPullRequest({ number: 58, createdAt: '2026-10-01T00:00:00.000Z', updatedAt: '2026-10-13T12:00:00.000Z' }); const r = rig({ api, cursor: '2026-10-13T09:00:00.000Z' }); - // Pass one: never seen before, so its head counts as moved — one sighting. const first = await r.poller.pollOnce(); assert.deepEqual( first.delivered.map((e) => e.kind), @@ -307,9 +259,7 @@ test('an open pull request whose head did not move is dropped on the next pass ( ); assert.equal(r.state.heads.get('github:anima-research/rayforce#58'), pull.head.sha); - // Pass two: a comment bumps the pull request's row without moving its head. - // The comment is the event; re-announcing the pull request beside it is the - // duplicate amendment 2a removes. + // A comment bumps the pull request's updated_at without moving its head. api.seedComment({ repo: TEST_REPO, number: 58, kind: 'pull' }, { id: 7002, updatedAt: '2026-10-14T01:00:00.000Z' }); pull.updatedAt = '2026-10-14T01:00:00.000Z'; const second = await r.poller.pollOnce(); @@ -318,7 +268,6 @@ test('an open pull request whose head did not move is dropped on the next pass ( ['comment.created'], ); - // Pass three: new commits. The head moved, so the row is a synchronize again. pull.head = { ...pull.head, sha: 'f'.repeat(40) }; pull.updatedAt = '2026-10-14T02:00:00.000Z'; const third = await r.poller.pollOnce(); @@ -349,7 +298,6 @@ test('a refused pull.synchronize is retried on the next pass and delivered once updatedAt: '2026-10-13T12:00:00.000Z', }); - // Pass one: never seen, so the head counts as moved — and the host refuses. let refuse = true; const attempts: string[] = []; const r = rig({ @@ -364,15 +312,9 @@ test('a refused pull.synchronize is retried on the next pass and delivered once const first = await r.poller.pollOnce(); assert.equal(first.failed, 1); assert.deepEqual(first.delivered, []); - // The cursor is held back, as it always was… assert.equal(first.cursorAdvanced, false); - // …and the head must be held back with it. Committing it here is what lost - // the event for good: the next pass would compare the row against this very - // sha, find no change, drop it, and let the cursor move past it. assert.equal(r.state.heads.get('github:anima-research/rayforce#58'), undefined); - // Pass two: the same row is queried again (the cursor never moved) and the - // host takes it this time. refuse = false; const second = await r.poller.pollOnce(); assert.deepEqual( @@ -384,10 +326,8 @@ test('a refused pull.synchronize is retried on the next pass and delivered once assert.equal(second.cursorAdvanced, true); assert.equal(second.failed, 0); assert.deepEqual(attempts, ['pull.synchronize', 'pull.synchronize'], 'attempted twice, delivered once'); - // Accepted, so now it is committed. assert.equal(r.state.heads.get('github:anima-research/rayforce#58'), pull.head.sha); - // Pass three: nothing moved since, so the row is dropped as usual. pull.updatedAt = '2026-10-14T01:00:00.000Z'; const third = await r.poller.pollOnce(); assert.deepEqual(third.delivered, [], 'and the retry did not leave it announcing forever'); @@ -400,8 +340,6 @@ test('a collection failure after a changed head does not advance the head (findi createdAt: '2026-10-01T00:00:00.000Z', updatedAt: '2026-10-13T12:00:00.000Z', }); - // The issue rows are read first and this one is read after them, so it - // throws with the changed head already in hand but nothing delivered. api.listIssueCommentsSince = async () => { throw new Error('GitHub 502 on the comment page'); }; @@ -414,7 +352,6 @@ test('a collection failure after a changed head does not advance the head (findi 'a pass that delivered nothing advanced nothing', ); - // So the recovered pass still announces the commits. delete (api as { listIssueCommentsSince?: unknown }).listIssueCommentsSince; const recovered = await r.poller.pollOnce(); assert.deepEqual( @@ -430,11 +367,7 @@ test('a dropped pull-request row still records its head, so the next pass agrees const r = rig({ api, cursor: '2026-10-13T09:00:00.000Z' }); await r.poller.pollOnce(); - // Two further passes, each with a comment that bumps the pull request's row. - // The comment is delivered every time — which is what proves the pass really - // collected these rows — while the pull-request row beside it is dropped - // every time: the head is recorded on a drop as well as on a delivery, so no - // pass ever mistakes the unchanged head for a change. + // Each comment bumps the pull request's updated_at without moving its head. let id = 7100; for (const at of ['2026-10-14T01:00:00.000Z', '2026-10-14T02:00:00.000Z']) { api.seedComment({ repo: TEST_REPO, number: 58, kind: 'pull' }, { id: id++, updatedAt: at }); @@ -452,7 +385,7 @@ test('issues, comments and review comments updated since the cursor are delivere const api = new FakeGitHubApi(); const issue = api.seedIssue({ number: 41, updatedAt: '2026-10-13T10:00:00.000Z' }); api.seedComment({ repo: TEST_REPO, number: 41, kind: 'issue' }, { id: 7001, updatedAt: '2026-10-13T11:00:00.000Z' }); - const pull = api.seedPullRequest({ number: 58, updatedAt: '2026-10-13T12:00:00.000Z' }); + const pull = api.seedPullRequest({ number: 58, createdAt: '2026-10-01T00:00:00.000Z', updatedAt: '2026-10-13T12:00:00.000Z' }); api.seedReviewThread( { repo: TEST_REPO, number: 58, kind: 'pull' }, { @@ -476,8 +409,6 @@ test('issues, comments and review comments updated since the cursor are delivere const result = await r.poller.pollOnce(); assert.deepEqual( result.delivered.map((e) => e.kind), - // `pull.synchronize`, not `pull.opened`: amendment 2a reserves `opened` - // for a row created at or after the cursor, and this one predates it. ['issue.edited', 'comment.created', 'pull.synchronize', 'review_comment.created'], ); assert.deepEqual( @@ -489,12 +420,10 @@ test('issues, comments and review comments updated since the cursor are delivere 'poll:review_comment:9001:2026-10-13T13:00:00.000Z', ], ); - // The acceptance fields: thread, actor, sha, review-comment detail. assert.deepEqual(result.delivered[3].thread, { repo: TEST_REPO, number: 58, kind: 'pull' }); assert.equal(result.delivered[3].reviewComment?.path, 'src/scheduler/tick.rs'); assert.equal(result.delivered[2].sha, pull.head.sha); assert.deepEqual(result.delivered[0].actor, { login: 'human-one', isBot: false }); - // `receivedAt` is the start of the pass, not the row's timestamp. for (const e of result.delivered) assert.equal(e.receivedAt, '2026-10-14T00:00:00.000Z'); }); @@ -520,7 +449,6 @@ test('an open pull-request head gets one checks event per conclusion, and none w assert.deepEqual(checks[0].thread, { repo: TEST_REPO, number: 58, kind: 'pull' }); assert.deepEqual(api.callsTo('getCheckRuns')[0].args, [TEST_REPO, pull.head.sha]); - // A head with no runs at all produces nothing. const bare = new FakeGitHubApi(); bare.seedPullRequest({ number: 59, updatedAt: '2026-10-13T12:00:00.000Z' }); const r2 = rig({ api: bare }); @@ -532,7 +460,6 @@ test('an open pull-request head gets one checks event per conclusion, and none w }); test('a settled head whose failed jobs change with the same count is news, not a duplicate', async () => { - // Review round 4: the poll key must distinguish failure *sets*, not counts. const api = new FakeGitHubApi(); const pull = api.seedPullRequest({ number: 58, updatedAt: '2026-10-13T12:00:00.000Z' }); const sets = [ @@ -547,7 +474,6 @@ test('a settled head whose failed jobs change with the same count is news, not a const first = await r.poller.pollOnce(); assert.deepEqual(first.delivered.filter((e) => e.kind === 'checks.completed').map((e) => e.checks?.failed.map((f) => f.name)), [['lint']]); - // The same open head is updated again; its runs now fail on a different job. api.seedPullRequest({ number: 58, head: pull.head, updatedAt: '2026-10-14T01:00:00.000Z' }); const second = await r.poller.pollOnce(); assert.equal(second.duplicates, 0, 'a different failed set is not a duplicate'); @@ -581,7 +507,7 @@ test('at most 20 pull-request heads are checked in one pass', async () => { test('a check-runs lookup that throws does not lose the rest of the pass, and the head is kept', async () => { const api = new FakeGitHubApi(); - api.seedPullRequest({ number: 58, updatedAt: '2026-10-13T12:00:00.000Z' }); + api.seedPullRequest({ number: 58, createdAt: '2026-10-01T00:00:00.000Z', updatedAt: '2026-10-13T12:00:00.000Z' }); const r = rig({ api, getCheckRuns: async () => { @@ -591,12 +517,8 @@ test('a check-runs lookup that throws does not lose the rest of the pass, and th const result = await r.poller.pollOnce(); assert.deepEqual( result.delivered.map((e) => e.kind), - // Amendment 2a: this pull request was created before the cursor and the - // poll state has no head for it yet, so its first sighting is a synchronize. ['pull.synchronize'], ); - // A lookup failure is not a delivery failure, so the cursor still moves — - // which is only safe because the head itself is retained for another pass. assert.equal(result.failed, 0); assert.equal(result.cursorAdvanced, true); assert.equal(result.checkLookups, 1); @@ -604,10 +526,6 @@ test('a check-runs lookup that throws does not lose the rest of the pass, and th assert.equal(r.poller.checkBacklogSize, 1); }); -// ───────────────────────────────────────────────────────────────────────────── -// "Marked only after delivery": the gate and the cursor -// ───────────────────────────────────────────────────────────────────────────── - test('a row already in the gate is not delivered twice', async () => { const api = new FakeGitHubApi(); api.seedIssue({ number: 41, updatedAt: '2026-10-13T10:00:00.000Z' }); @@ -617,9 +535,6 @@ test('a row already in the gate is not delivered twice', async () => { assert.equal(first.delivered.length, 1); assert.deepEqual(r.state.snapshot().pollKeys, ['issue:41:2026-10-13T10:00:00.000Z']); - // Rewind the cursor so the very same row is queried again, and forget its - // marker as an eviction would: now the gate, and not the cursor or the - // marker, is the only thing that can stop the duplicate. r.state.setLastPollAt('2026-10-13T00:00:00.000Z'); r.state.setPollCursors({}); r.state.heads.clear(); @@ -634,7 +549,6 @@ test('an edit of the same row is a fresh event: the key carries updated_at', asy const r = rig({ api }); assert.equal((await r.poller.pollOnce()).delivered.length, 1); - // Edited after the pass that first delivered it, so the next pass sees it. issue.body = 'edited body'; issue.updatedAt = '2026-10-14T01:00:00.000Z'; const second = await r.poller.pollOnce(); @@ -653,7 +567,6 @@ test('a key is recorded only after its event was delivered', async () => { const r = rig({ api, onEvent: (event) => { - // Inside the listener the key is not in the gate yet. ringDuringDelivery.push(r.state.snapshot().pollKeys); assert.equal(event.kind, 'issue.edited'); }, @@ -682,7 +595,6 @@ test('a failed delivery leaves its key unrecorded, holds the cursor, and is retr assert.equal(first.cursorAdvanced, false); assert.equal(r.state.lastPollAt(), '2026-10-13T00:00:00.000Z', 'the cursor must not move'); - // The next pass therefore sees the row again and delivers it. const second = await r.poller.pollOnce(); assert.equal(attempts, 2); assert.equal(second.delivered.length, 1); @@ -717,16 +629,10 @@ test('one failure does not block the rows around it, and only it is retried', as second.delivered.map((e) => e.thread?.number), [42], ); - // The two that already landed recorded their markers, so their unchanged - // rows drop before the gate; the refused one never recorded its marker. assert.equal(second.duplicates, 0); assert.equal(second.cursorAdvanced, true); }); -// ───────────────────────────────────────────────────────────────────────────── -// A comment's bump is not an issue change (the 2026-10-15 rehearsal) -// ───────────────────────────────────────────────────────────────────────────── - test('the bot\'s comment and close on an issue that mentions it do not come back as a human mention', async () => { const api = new FakeGitHubApi(); const issue = api.seedIssue({ @@ -739,7 +645,7 @@ test('the bot\'s comment and close on an issue that mentions it do not come back const first = await r.poller.pollOnce(); assert.deepEqual(first.delivered.map((e) => e.kind), ['issue.opened']); - // The bot comments: the comment bumps the issue's updated_at, nothing else. + // A comment bumps the issue's updated_at and changes nothing else on it. const bot = { login: TEST_BOT.login, isBot: true }; api.seedComment({ repo: TEST_REPO, number: 10, kind: 'issue' }, { author: bot, body: 'Seen.', updatedAt: '2026-10-14T01:00:00.000Z' }); issue.updatedAt = '2026-10-14T01:00:00.000Z'; @@ -747,7 +653,6 @@ test('the bot\'s comment and close on an issue that mentions it do not come back assert.deepEqual(second.delivered.map((e) => e.kind), ['comment.created'], 'no issue row beside the comment'); assert.equal(second.delivered[0].actor.login, TEST_BOT.login); - // The bot closes it: delivered once, as the bot's own act. issue.state = 'closed'; issue.updatedAt = '2026-10-14T02:00:00.000Z'; api.closedBy.set(10, bot); @@ -755,7 +660,6 @@ test('the bot\'s comment and close on an issue that mentions it do not come back assert.deepEqual(third.delivered.map((e) => e.kind), ['issue.closed']); assert.equal(third.delivered[0].actor.login, TEST_BOT.login, 'closed_by, not the author'); - // A human comments on the closed issue: the comment, and no second close. api.seedComment({ repo: TEST_REPO, number: 10, kind: 'issue' }, { body: 'thanks', updatedAt: '2026-10-14T03:00:00.000Z' }); issue.updatedAt = '2026-10-14T03:00:00.000Z'; const fourth = await r.poller.pollOnce(); @@ -782,18 +686,10 @@ test('a title or body edit, and a reopen, still arrive under polling', async () 'a reopen changes the state half of the marker'); }); -// ───────────────────────────────────────────────────────────────────────────── -// The state seam -// ───────────────────────────────────────────────────────────────────────────── - test('IngressState is a structural subset of the frozen StateStore', () => { - // A compile-time assertion first: .7's `state.ts` can be handed straight to - // the poller. (`snapshot()` is in IngressState precisely so the gate can be - // read without being written to.) const asIngress = (store: StateStore): IngressState => store; assert.equal(typeof asIngress, 'function'); - // And a runtime one, over the member names the poller actually calls. const state = new MemoryIngressState(); for (const member of ['lastPollAt', 'setLastPollAt', 'pollCursors', 'setPollCursors', 'polledNew', 'setPolledNew', 'seenPollKey', 'snapshot'] as const) { assert.equal(typeof state[member], 'function', member); @@ -801,8 +697,6 @@ test('IngressState is a structural subset of the frozen StateStore', () => { }); test('each list starts from lastPollAt until it has a cursor of its own, then keeps it', async () => { - // A state file from before per-list cursors carries `lastPollAt` alone, and - // one cursor for a repository no longer polled. const r = rig({ cursor: '2026-10-13T00:00:00.000Z', repos: [TEST_REPO, OTHER_REPO] }); r.state.setPollCursors({ 'gone/repo:issues': { since: '2026-10-01T00:00:00.000Z' } }); @@ -819,7 +713,6 @@ test('each list starts from lastPollAt until it has a cursor of its own, then ke 'other-org/other-repo:review_comments': { since: start }, }); - // A list catching up reads from where it stopped; the others from their own. r.state.setPollCursors({ ...r.state.pollCursors(), 'anima-research/rayforce:issues': { since: '2026-10-13T00:00:00.000Z', resumeAt: '2026-10-13T06:00:00Z' }, @@ -866,14 +759,12 @@ test('a refused pass keeps the rows it announced as new, so a retry does not ope 'anima-research/rayforce:issues': { '41': '2026-10-13T12:00:00.000Z', '58': '2026-10-13T12:00:00.000Z' }, }); - // Both change before the retry: the issue is edited, the pull request pushed. issue.body = 'edited'; issue.updatedAt = '2026-10-13T13:00:00.000Z'; pull.head = { ...pull.head, sha: 'f'.repeat(40) }; pull.updatedAt = '2026-10-13T13:00:00.000Z'; const second = await r.poller.pollOnce(); assert.deepEqual(second.delivered.map((e) => e.kind).sort(), ['comment.created', 'issue.edited', 'pull.synchronize']); - // The pass delivered, and its window starts after both were created. assert.deepEqual(r.state.fresh, {}); }); @@ -902,7 +793,6 @@ test('lastPollAt is the earliest place a list stopped, and never past the pass s await r.poller.pollOnce(); assert.equal(r.state.lastPollAt(), '2026-10-13T07:00:00Z'); - // A stop stamped after the pass started, as a row changed during it can be. api.listIssuesSince = async () => ({ rows: [] }); api.listIssueCommentsSince = async () => ({ rows: [], resumeAt: '2026-10-14T00:00:05Z' }); await r.poller.pollOnce(); @@ -911,7 +801,6 @@ test('lastPollAt is the earliest place a list stopped, and never past the pass s test('a first pass whose read throws keeps the window it read', async () => { const api = new FakeGitHubApi(); - // The very first read throws, so the window must be kept before any read. api.listIssuesSince = async () => { throw new Error('GitHub 502 on the issues page'); }; @@ -962,18 +851,13 @@ test('the gate the state keeps is bounded by the contract POLL_DEDUP_RING', asyn const state = new MemoryIngressState('2026-10-13T00:00:00.000Z'); for (let i = 0; i < POLL_DEDUP_RING + 5; i++) assert.equal(state.seenPollKey(`issue:${i}:t`), false); assert.equal(state.snapshot().pollKeys.length, POLL_DEDUP_RING); - // The oldest five were forgotten; the newest are still remembered. assert.equal(state.seenPollKey('issue:0:t'), false); assert.equal(state.seenPollKey(`issue:${POLL_DEDUP_RING + 4}:t`), true); }); -// ───────────────────────────────────────────────────────────────────────────── -// The check backlog: unfinished check work survives the cursor -// ───────────────────────────────────────────────────────────────────────────── - test('a transient lookup failure is retried on the next pass, after the cursor moved on', async () => { const api = new FakeGitHubApi(); - const pull = api.seedPullRequest({ number: 58, updatedAt: '2026-10-13T12:00:00.000Z' }); + const pull = api.seedPullRequest({ number: 58, createdAt: '2026-10-01T00:00:00.000Z', updatedAt: '2026-10-13T12:00:00.000Z' }); api.seedCheckRuns(TEST_REPO, failedSummary(pull.head.sha)); const r = rig({ api, @@ -983,19 +867,14 @@ test('a transient lookup failure is retried on the next pass, after the cursor m }, }); - // Pass one: the row lands, the lookup fails, the head is kept. const first = await r.poller.pollOnce(); assert.deepEqual( first.delivered.map((e) => e.kind), - // Amendment 2a: this pull request was created before the cursor and the - // poll state has no head for it yet, so its first sighting is a synchronize. ['pull.synchronize'], ); assert.equal(first.checksBacklog, 1); assert.equal(first.cursorAdvanced, true); - // Pass two: the cursor has moved past the row, so `listIssuesSince` returns - // nothing — the backlog is the only reason the checks are asked about again. const second = await r.poller.pollOnce(); assert.deepEqual((await api.listIssuesSince(TEST_REPO, r.state.lastPollAt() as string)).rows, []); assert.deepEqual( @@ -1014,15 +893,11 @@ test('a head whose checks are still running waits, then reports when they finish let finished = false; const r = rig({ api, - // A short attempt budget: a pending head must not spend attempts, or it - // would be given up on before CI finishes. maxHeadAttempts: 2, getCheckRuns: async (_repo, sha) => finished ? failedSummary(sha) : { sha, conclusion: null, total: 3, failed: [] }, }); - // Three passes with runs still going: no event, and the head is still held - // even though that is more passes than maxHeadAttempts. for (const pass of [1, 2, 3]) { const result = await r.poller.pollOnce(); assert.deepEqual( @@ -1047,8 +922,8 @@ test('a head whose checks are still running waits, then reports when they finish test('a head that never gets check runs is given up on after its attempts', async () => { const api = new FakeGitHubApi(); api.seedPullRequest({ number: 58, updatedAt: '2026-10-13T12:00:00.000Z' }); - // Unseeded sha: the fake answers `{ total: 0 }`, as a head no workflow - // matches does. + // GitHub answers total_count 0 for a head no workflow matches, as the fake + // does for an unseeded sha. const r = rig({ api, maxHeadAttempts: 3 }); assert.equal((await r.poller.pollOnce()).checksBacklog, 1); @@ -1059,7 +934,6 @@ test('a head that never gets check runs is given up on after its attempts', asyn r.events.filter((e) => e.kind === 'checks.completed'), [], ); - // And it is not asked about a fourth time. assert.equal((await r.poller.pollOnce()).checkLookups, 0); assert.equal(r.checkCalls.length, 3); assert.equal(MAX_HEAD_ATTEMPTS, 10); @@ -1080,14 +954,11 @@ test('heads beyond the per-pass cap are drained by the following passes', async assert.equal(first.delivered.filter((e) => e.kind === 'checks.completed').length, 20); assert.equal(first.checksBacklog, 5, 'the five the cap cut off are kept'); - // The cursor has moved past every row, so without the backlog this pass - // would do nothing at all — the regression the reviewer reproduced. const second = await r.poller.pollOnce(); assert.equal(second.checkLookups, 5); assert.equal(second.delivered.filter((e) => e.kind === 'checks.completed').length, 5); assert.equal(second.checksBacklog, 0); - // Every one of the 25 heads was reported, exactly once. const reported = r.events.filter((e) => e.kind === 'checks.completed').map((e) => e.sha); assert.equal(reported.length, 25); assert.deepEqual([...reported].sort(), [...shas].sort()); @@ -1100,7 +971,6 @@ test('an over-cap backlog drains round-robin instead of starving the tail', asyn const pull = api.seedPullRequest({ number: 200 + i, updatedAt: '2026-10-13T12:00:00.000Z' }); shas.push(pull.head.sha); } - // Nothing ever settles, so the only question is who gets asked. const r = rig({ api, getCheckRuns: async (_repo, sha) => ({ sha, conclusion: null, total: 3, failed: [] }) }); await r.poller.pollOnce(); @@ -1108,8 +978,6 @@ test('an over-cap backlog drains round-robin instead of starving the tail', asyn r.checkCalls.length = 0; await r.poller.pollOnce(); - // The five that were starved are asked first, because the 20 that were - // examined went to the back of the queue. assert.deepEqual(r.checkCalls.slice(0, 5), shas.slice(20, 25)); assert.deepEqual(r.checkCalls.slice(5), shas.slice(0, 15)); assert.equal(r.poller.checkBacklogSize, 25); @@ -1123,7 +991,7 @@ test('the backlog is bounded: past its cap the oldest head waiting is forgotten' const r = rig({ api, maxHeadBacklog: 4, - maxCheckedPulls: 0, // no draining, so only the enqueue bound is on trial + maxCheckedPulls: 0, getCheckRuns: async (_repo, sha) => ({ sha, conclusion: null, total: 1, failed: [] }), }); const result = await r.poller.pollOnce(); @@ -1151,7 +1019,6 @@ test('the poller reports a head sha once: a second conclusion must be worse', as assert.equal((await r.poller.pollOnce()).delivered.filter((e) => e.kind === 'checks.completed').length, 1); - // The same head settles green later (a second workflow): not news. conclusion = { sha: pull.head.sha, conclusion: 'success', total: 3, failed: [] }; api.seedIssue({ number: 41, updatedAt: '2026-10-14T01:00:00.000Z' }); const pr = api.seedPullRequest({ number: 58, updatedAt: '2026-10-14T01:00:00.000Z' }); @@ -1167,7 +1034,6 @@ test('a poller and a listener sharing a CheckGate report a head sha once', async const api = new FakeGitHubApi(); const pull = api.seedPullRequest({ number: 58, updatedAt: '2026-10-13T12:00:00.000Z' }); const gate = new CheckGate(); - // The webhook path got there first and had this exact result accepted. const mark: CheckMark = { rank: 7, failed: ['clippy'] }; assert.equal(await gate.emit(TEST_REPO, pull.head.sha, mark, async () => {}), true); const r = rig({ api, checkGate: gate, getCheckRuns: async (_r, sha) => failedSummary(sha) }); @@ -1201,7 +1067,6 @@ test('a refused checks event commits no mark and keeps the backlog slot it needs assert.equal(first.cursorAdvanced, false, 'a failed delivery holds the cursor'); assert.equal(first.checksBacklog, 1, 'and the head stays queued'); - // The retry lands, and only then is the mark committed and the head retired. const second = await r.poller.pollOnce(); assert.equal(attempts, 2); const delivered = second.delivered.filter((e) => e.kind === 'checks.completed'); @@ -1210,17 +1075,9 @@ test('a refused checks event commits no mark and keeps the backlog slot it needs assert.equal(second.checksBacklog, 0, 'the accepted head left the backlog'); }); -// ───────────────────────────────────────────────────────────────────────────── -// Review round 2: a head leaves the lifecycle only once its event is accepted -// ───────────────────────────────────────────────────────────────────────────── - test('a checks event refused long after the cursor moved past its row is still retried', async () => { - // The regression the reviewer reproduced. Three passes: - // 1. the PR row lands, the checks are still running → cursor advances - // 2. the checks settle, the listener refuses the event - // 3. the retry, with the row long gone from the since-query const api = new FakeGitHubApi(); - const pull = api.seedPullRequest({ number: 58, updatedAt: '2026-10-13T12:00:00.000Z' }); + const pull = api.seedPullRequest({ number: 58, createdAt: '2026-10-01T00:00:00.000Z', updatedAt: '2026-10-13T12:00:00.000Z' }); const gate = new CheckGate(); let settled = false; let refusals = 0; @@ -1237,23 +1094,17 @@ test('a checks event refused long after the cursor moved past its row is still r }, }); - // Pass one: the row is delivered, the checks are pending, the cursor moves. const first = await r.poller.pollOnce(); assert.deepEqual( first.delivered.map((e) => e.kind), - // Amendment 2a: this pull request was created before the cursor and the - // poll state has no head for it yet, so its first sighting is a synchronize. ['pull.synchronize'], ); assert.equal(first.cursorAdvanced, true); assert.equal(first.checksBacklog, 1); - // From here on the pull-request row is behind the cursor: the backlog is - // the only thing that can produce this checks event. const cursor = r.state.lastPollAt() as string; assert.deepEqual((await api.listIssuesSince(TEST_REPO, cursor)).rows, []); - // Pass two: the checks settle and the listener refuses. settled = true; const second = await r.poller.pollOnce(); assert.equal(second.failed, 1); @@ -1262,7 +1113,6 @@ test('a checks event refused long after the cursor moved past its row is still r assert.equal(gate.size, 0, 'and nothing must be committed for it'); assert.equal(second.cursorAdvanced, false); - // Pass three: the retry lands, and only now is the head retired. const third = await r.poller.pollOnce(); assert.deepEqual( third.delivered.map((e) => e.kind), @@ -1272,7 +1122,6 @@ test('a checks event refused long after the cursor moved past its row is still r assert.deepEqual(third.delivered[0].checks?.failed.map((f) => f.name), ['clippy']); assert.equal(third.checksBacklog, 0); assert.equal(refusals, 1); - // Three lookups: pending, settled-and-refused, settled-and-accepted. assert.deepEqual(r.checkCalls, [pull.head.sha, pull.head.sha, pull.head.sha]); }); @@ -1282,7 +1131,6 @@ test('a checks event the gate has already spoken for retires its head instead of const gate = new CheckGate(); const r = rig({ api, checkGate: gate, getCheckRuns: async (_repo, sha) => failedSummary(sha) }); - // The webhook path had this exact result accepted first. assert.equal( await gate.emit(TEST_REPO, pull.head.sha, { rank: 7, failed: ['clippy'] }, async () => {}), true, @@ -1292,7 +1140,6 @@ test('a checks event the gate has already spoken for retires its head instead of assert.equal(first.delivered.filter((e) => e.kind === 'checks.completed').length, 0); assert.equal(first.duplicates, 1); assert.equal(first.checksBacklog, 0, 'dedup confirms delivery, so the head is done'); - // And it is not looked up again. assert.equal((await r.poller.pollOnce()).checkLookups, 0); }); @@ -1303,12 +1150,10 @@ test('a head whose event was accepted is not re-delivered by the durable gate ei const first = await r.poller.pollOnce(); assert.equal(first.delivered.filter((e) => e.kind === 'checks.completed').length, 1); - // Both share the row's `updated_at`, so the key breaks the tie. const keys = r.state.snapshot().pollKeys; assert.equal(keys.length, 2); assert.match(keys[0], new RegExp(`^checks:${pull.head.sha}:failure:1:[0-9a-f]{12}$`)); assert.equal(keys[1], `issue:${pull.number}:2026-10-13T12:00:00.000Z`); - // The head is retired, so nothing re-queues it and nothing re-delivers it. const second = await r.poller.pollOnce(); assert.equal(second.checkLookups, 0); assert.equal(second.delivered.length, 0); diff --git a/test/state.test.ts b/test/state.test.ts index 204f7ec..3780ae1 100644 --- a/test/state.test.ts +++ b/test/state.test.ts @@ -1,11 +1,3 @@ -/** - * state.ts — checkpoint lineage, rollback planning, and the atomic state file - * (TASK-SRHKG.1.7). - * - * Every test works in its own temp directory; nothing here touches the network - * or a real state file path. - */ - import { strict as assert } from 'node:assert'; import { mkdtempSync, readFileSync, readdirSync, statSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; @@ -20,7 +12,6 @@ function tempFile(name = 'state.json'): string { return join(mkdtempSync(join(tmpdir(), 'github-mcpl-state-')), name); } -/** A Logger that keeps what it was told, so a test can assert on diagnostics. */ function capturingLogger(): Logger & { lines: string[] } { const lines: string[] = []; const push = (level: string) => (message: string) => { @@ -38,10 +29,6 @@ function strayTempFiles(file: string): string[] { return readdirSync(dir).filter((n) => n.includes('.tmp-')); } -// ───────────────────────────────────────────────────────────────────────────── -// Checkpoint lineage (§8.2) -// ───────────────────────────────────────────────────────────────────────────── - test('checkpoints form a lineage, and a rollback branches it', async () => { const state = GithubState.empty(); assert.equal(state.current(), null); @@ -60,7 +47,6 @@ test('checkpoints form a lineage, and a rollback branches it', async () => { ], ); - // Rolling back moves `current` back; the next checkpoint branches from there. assert.ok(state.rollback(first.id)); assert.equal(state.current(), first.id); const branch = state.createCheckpoint(); @@ -78,10 +64,6 @@ test('an unknown checkpoint plans nothing (the caller answers -32005)', () => { assert.equal(state.rollback('chk_nope'), null); }); -// ───────────────────────────────────────────────────────────────────────────── -// Rollback planning -// ───────────────────────────────────────────────────────────────────────────── - test('rollback plans the writes recorded after the checkpoint, newest first', () => { const state = GithubState.empty(); const base = state.createCheckpoint(); @@ -101,13 +83,9 @@ test('rollback plans the writes recorded after the checkpoint, newest first', () assert.deepEqual(plan.edited.map((e) => [e.commentId, e.previousBody]), [[5, 'before']]); assert.deepEqual(plan.deleted.map((d) => d.commentId), [6]); - // The buckets are consumed: a second rollback to the same checkpoint is a - // no-op, not a double undo. const again = state.rollback(base.id); assert.ok(again); assert.deepEqual(again, { created: [], edited: [], deleted: [] }); - // The checkpoints the first rollback left behind are no longer rollable: - // their undo records are spent and nothing can rebuild the state they named. assert.equal(state.checkpointStatus(afterFirst.id), 'abandoned'); assert.equal(state.rollback(afterFirst.id), null); }); @@ -132,14 +110,9 @@ test('two edits of one comment restore the oldest body last', () => { const plan = state.rollback(base.id); assert.ok(plan); - // Applied in order, the last write wins, so the oldest body is the one left. assert.deepEqual(plan.edited.map((e) => e.previousBody), ['second', 'first']); }); -// ───────────────────────────────────────────────────────────────────────────── -// The state file -// ───────────────────────────────────────────────────────────────────────────── - test('flush writes a versioned state file atomically and leaves no temp file', async () => { const file = tempFile(); const state = GithubState.load(file); @@ -154,7 +127,6 @@ test('flush writes a versioned state file atomically and leaves no temp file', a assert.deepEqual(written.created[cp.id].map((c) => c.commentId), [11]); assert.equal(written.lastPollAt, '2026-10-14T00:00:00.000Z'); assert.deepEqual(strayTempFiles(file), []); - // The file holds this server's bookkeeping; no group or other access. assert.equal(statSync(file).mode & 0o077, 0); }); @@ -184,9 +156,6 @@ test('a crash between the write and the rename never replaces the state', async await state.flush(); const good = readFileSync(file, 'utf8'); - // What a crash mid-flush leaves behind: a truncated temp file beside an - // untouched state file. `flush` renames only a fully written temp file, so - // the half-written one is not the state and is never read as the state. const truncated = `${file}.tmp-99999-deadbeef`; writeFileSync(truncated, '{"version":1,"checkpoints":[{"id":"chk_half'); @@ -225,9 +194,9 @@ test('a damaged or foreign state file starts empty with one stderr line', () => assert.deepEqual(GithubState.load(future, futureLog).lineage(), []); assert.match(futureLog.lines[0], /not a version 1 snapshot/); - // An absent file is the normal first start: no diagnostics at all. const fresh = capturingLogger(); - assert.deepEqual(GithubState.load(tempFile(), fresh).lineage(), []); + const absent = tempFile(); + assert.deepEqual(GithubState.load(absent, fresh).lineage(), []); assert.deepEqual(fresh.lines, []); }); @@ -240,10 +209,6 @@ test('without a state file the store works in memory and flush is a no-op', asyn assert.equal(state.current(), cp.id); }); -// ───────────────────────────────────────────────────────────────────────────── -// Poll cursor and dedup ring (amendment 1) -// ───────────────────────────────────────────────────────────────────────────── - test('the poll dedup ring remembers the last 2000 keys and no more', () => { const state = GithubState.empty(); assert.equal(state.lastPollAt(), null); @@ -252,12 +217,12 @@ test('the poll dedup ring remembers the last 2000 keys and no more', () => { assert.equal(state.seenPollKey('comment:1:t0'), false); assert.equal(state.seenPollKey('comment:1:t0'), true); - // A new updated_at is a new key: an edit of the same comment is not a replay. + // GitHub stamps an edit with a new updated_at, to the second, so an edit + // in a later second has a new key and isn't a replay. assert.equal(state.seenPollKey('comment:1:t1'), false); for (let i = 0; i < POLL_DEDUP_RING; i++) state.seenPollKey(`issue:${i}:t`); assert.equal(state.snapshot().pollKeys.length, POLL_DEDUP_RING); - // The oldest keys fell out of the ring, the newest are still there. assert.equal(state.seenPollKey('comment:1:t0'), false); assert.equal(state.seenPollKey(`issue:${POLL_DEDUP_RING - 1}:t`), true); }); @@ -267,19 +232,14 @@ test('the polled heads are bounded, survive a flush, and tolerate an older file assert.equal(state.polledHead('github:anima-research/rayforce#58'), null); state.setPolledHead('github:anima-research/rayforce#58', 'a'.repeat(40)); assert.equal(state.polledHead('github:anima-research/rayforce#58'), 'a'.repeat(40)); - // Last write wins: this is "the head the last pass saw", not a history. state.setPolledHead('github:anima-research/rayforce#58', 'b'.repeat(40)); assert.equal(state.polledHead('github:anima-research/rayforce#58'), 'b'.repeat(40)); - // Bounded, and the *oldest* entry is the one that goes. for (let i = 0; i < POLL_HEAD_MEMORY; i++) state.setPolledHead(`github:${REPO}#${1000 + i}`, 'c'.repeat(40)); assert.equal(Object.keys(state.snapshot().pollHeads).length, POLL_HEAD_MEMORY); assert.equal(state.polledHead('github:anima-research/rayforce#58'), null); assert.equal(state.polledHead(`github:${REPO}#${1000 + POLL_HEAD_MEMORY - 1}`), 'c'.repeat(40)); - // Re-recording an unchanged head refreshes its place in the eviction order, - // so a pull request polled every minute is not evicted ahead of heads - // nobody has looked at since. const oldest = `github:${REPO}#1000`; state.setPolledHead(oldest, 'c'.repeat(40)); state.setPolledHead(`github:${REPO}#9999`, 'd'.repeat(40)); @@ -289,8 +249,6 @@ test('the polled heads are bounded, survive a flush, and tolerate an older file test('a state file written before amendment 2a still loads, with no heads known', () => { const file = tempFile(); - // A version-1 snapshot from before `pollHeads` existed: the field is - // additive, so this is still a valid file and not a damaged one. writeFileSync( file, JSON.stringify({ @@ -306,11 +264,9 @@ test('a state file written before amendment 2a still loads, with no heads known' ); const log = capturingLogger(); const state = GithubState.load(file, log); - // Loaded, not reset: the cursor and the ring are the ones on disk. assert.equal(state.lastPollAt(), '2026-10-14T00:00:00.000Z'); assert.equal(state.seenPollKey('issue:1:t0'), true); assert.deepEqual(state.snapshot().pollHeads, {}); - // Nor per-list cursors, so every list starts from `lastPollAt`. assert.deepEqual(state.pollCursors(), {}); assert.deepEqual(state.polledNew(), {}); assert.deepEqual(log.lines, []); @@ -392,7 +348,6 @@ test('a polled head round-trips through a flush (amendment 2a)', async () => { const reloaded = GithubState.load(file, capturingLogger()); assert.equal(reloaded.polledHead('github:anima-research/rayforce#58'), 'e'.repeat(40)); - // A non-string value in the file is dropped rather than trusted. const raw = readSnapshot(file) as unknown as { pollHeads: Record<string, unknown> }; raw.pollHeads['github:anima-research/rayforce#59'] = 42; writeFileSync(file, JSON.stringify(raw)); @@ -411,13 +366,9 @@ test('a checkpoint abandoned by an earlier rollback is refused, not accepted emp assert.equal(state.checkpointStatus(b.id), 'live'); assert.ok(state.rollback(a.id)); - // The rollback spent A's records and moved off B's branch. B named a state - // this server can no longer rebuild, so it stops being rollable — answering - // it with an empty plan would report a restoration that never happened. state.recordEditedComment({ repo: REPO, commentId: 5, previousBody: 'A', review: false }); assert.equal(state.checkpointStatus(b.id), 'abandoned'); assert.equal(state.rollback(b.id), null); - // A itself stays live: it is still the current lineage. assert.equal(state.checkpointStatus(a.id), 'live'); assert.ok(state.rollback(a.id)); assert.equal(state.checkpointStatus('chk_never'), 'unknown'); diff --git a/test/tags.test.ts b/test/tags.test.ts index 1c56fe5..4253e8b 100644 --- a/test/tags.test.ts +++ b/test/tags.test.ts @@ -1,12 +1,3 @@ -/** - * tags.ts — table-driven over every `NormalizedEventKind`, plus the detection - * rules (mention, self, maintainer), the closure invariants and the bounds. - * - * The table is the point: the coverage test below fails the moment a kind is - * added to the contract without a row here, so "every kind has a tags row" is - * a test rather than a promise. - */ - import { strict as assert } from 'node:assert'; import { test } from 'node:test'; import { @@ -33,7 +24,6 @@ const BOT = 'rayforce-clone[bot]'; const ISSUE: ThreadRef = { repo: REPO, number: 7, kind: 'issue' }; const PULL: ThreadRef = { repo: REPO, number: 12, kind: 'pull' }; -/** A deployment with no maintainers and no thread history, unless overridden. */ function ctx(over: Partial<GitHubTagContext> = {}): GitHubTagContext { return { botLogin: BOT, maintainers: [], ...over }; } @@ -56,16 +46,10 @@ function sorted(tags: readonly string[]): string[] { return [...tags].sort(); } -// ───────────────────────────────────────────────────────────────────────────── -// The table: one row per NormalizedEventKind -// ───────────────────────────────────────────────────────────────────────────── - interface Row { kind: NormalizedEventKind; - /** What the kind carries beyond the baseline event. */ event?: Partial<NormalizedEvent>; ctx?: Partial<GitHubTagContext>; - /** The exact tag set, for a human author with no thread history. */ tags: string[]; } @@ -215,12 +199,7 @@ test('tags are deduplicated and namespaced', () => { } }); -// ───────────────────────────────────────────────────────────────────────────── -// Closure rules -// ───────────────────────────────────────────────────────────────────────────── - -/** The kinds whose body is new text: the only ones a mention can address. */ -const TEXT_KINDS: readonly string[] = [ +const NEW_TEXT_KINDS: readonly string[] = [ 'issue.opened', 'issue.edited', 'pull.opened', @@ -232,11 +211,9 @@ const TEXT_KINDS: readonly string[] = [ ]; test('a close, reopen, push or new commits never count the mention in a body they re-carry', () => { - // The 2026-10-15 rehearsal: a polled issue.closed re-carried the issue's - // body, and its old mention woke the bot that had just closed it. for (const kind of NORMALIZED_EVENT_KINDS) { const { tags, metadata } = tagsFor(event(kind, { body: '@rayforce-clone please look' }), ctx()); - const counts = TEXT_KINDS.includes(kind); + const counts = NEW_TEXT_KINDS.includes(kind); assert.equal(tags.includes('chat:mention'), counts, `${kind}: chat:mention`); assert.equal(metadata.mentioned, counts, `${kind}: metadata.mentioned`); } @@ -251,14 +228,12 @@ test('chat:ambient never co-occurs with chat:mention or chat:reply, on any kind' ]; for (const kind of NORMALIZED_EVENT_KINDS) { for (const [label, ctxOver, eventOver] of addressing) { - // A mention alone addresses only the kinds that bring their text. - if (label === 'mention' && !TEXT_KINDS.includes(kind)) continue; + if (label === 'mention' && !NEW_TEXT_KINDS.includes(kind)) continue; const { tags } = tagsFor(event(kind, eventOver), ctx(ctxOver)); const addressed = tags.includes('chat:mention') || tags.includes('chat:reply'); assert.ok(addressed, `${kind} / ${label}: expected an addressed tag`); assert.ok(!tags.includes('chat:ambient'), `${kind} / ${label}: ambient beside addressed`); } - // And the converse: unaddressed traffic is ambient and nothing else. const { tags } = tagsFor(event(kind), ctx()); assert.ok(tags.includes('chat:ambient'), `${kind}: expected chat:ambient`); assert.ok(!tags.includes('chat:mention') && !tags.includes('chat:reply')); @@ -293,10 +268,6 @@ test('exactly one sender tag, and from-self only for the bot login', () => { } }); -// ───────────────────────────────────────────────────────────────────────────── -// Mention detection -// ───────────────────────────────────────────────────────────────────────────── - test('mentionsBot accepts both spellings of an App login and rejects near misses', () => { const yes = [ '@rayforce-clone please look', @@ -332,10 +303,6 @@ test('hasLink finds http(s) URLs only', () => { assert.ok(!hasLink(undefined)); }); -// ───────────────────────────────────────────────────────────────────────────── -// github: tags that need more than the kind -// ───────────────────────────────────────────────────────────────────────────── - test('a merged pull request carries both closed and merged', () => { const closed = tagsFor(event('pull.closed'), ctx()).tags; assert.ok(closed.includes('github:pr=closed')); @@ -358,9 +325,7 @@ test('ci tags and isCiFailure follow the conclusion, not the kind', () => { [{ conclusion: 'failure', failed: [{ name: 'test', url: 'u' }] }, 'github:ci=failure', true], [{ conclusion: 'timed_out', failed: [] }, 'github:ci=failure', true], [{ conclusion: 'action_required', failed: [] }, 'github:ci=failure', true], - // A green roll-up with a failed job in the list is still a failure. [{ conclusion: 'success', failed: [{ name: 'flaky', url: 'u' }] }, 'github:ci=failure', true], - // Neither a pass nor a failure: no ci tag at all. [{ conclusion: 'neutral', failed: [] }, null, false], [{ conclusion: 'cancelled', failed: [] }, null, false], [{ conclusion: 'skipped', failed: [] }, null, false], @@ -398,10 +363,6 @@ test('isReviewRequested is set for pull.review_requested and suppressible', () = } }); -// ───────────────────────────────────────────────────────────────────────────── -// Metadata -// ───────────────────────────────────────────────────────────────────────────── - test('metadata carries the scalar fields of the thread, actor and event', () => { const { metadata } = tagsFor( event('review_comment.created', { @@ -481,9 +442,7 @@ test('every metadata string is bounded and no raw payload reaches metadata', () ])); }); -// ───────────────────────────────────────────────────────────────────────────── -// The producer ontology (RFC-001 §5) -// ───────────────────────────────────────────────────────────────────────────── +// The producer ontology: MCPL RFC-001 §5. test('the ontology declares the core tags this server emits and no others', () => { assert.deepEqual(sorted(GITHUB_TAG_ONTOLOGY.coreTags ?? []), sorted(CORE_TAGS)); @@ -512,7 +471,6 @@ test('no suggested treatment promotes unaddressed traffic', () => { if (matched.includes('chat:ambient') || matched.includes('chat:from-bot')) { assert.notEqual(rule.behavior, 'immediate', JSON.stringify(rule)); } - // An `immediate` suggestion is only ever reached by addressed traffic. if (rule.behavior === 'immediate') { assert.ok( (rule.tagsAll ?? []).some((t) => t === 'chat:mention' || t === 'chat:reply'), diff --git a/test/toolsComments.test.ts b/test/toolsComments.test.ts index fa01bc0..7b8c3e9 100644 --- a/test/toolsComments.test.ts +++ b/test/toolsComments.test.ts @@ -1,13 +1,3 @@ -/** - * tools-comments.ts — the `github.comments` tools and `state/rollback` - * (TASK-SRHKG.1.7). - * - * Everything runs against `FakeGitHubApi` and an in-memory {@link GithubState}; - * no octokit, no network. The tests drive the tools the way .9's routing will: - * a tool call, then a checkpoint, so the checkpoint names the state *including* - * that write (SPEC §8.2). - */ - import { strict as assert } from 'node:assert'; import { mkdtempSync } from 'node:fs'; import { tmpdir } from 'node:os'; @@ -73,7 +63,6 @@ function text(result: ToolResult): string { return result.content.map((block) => (block as TextContent).text ?? '').join('\n'); } -/** One tool call the way .9 routes it: call, then checkpoint the new state. */ async function call(f: Fixture, name: string, args: Record<string, unknown>): Promise<ToolResult> { const result = await commentsToolModule.call(name, args, f.ctx); f.state.createCheckpoint(); @@ -84,10 +73,6 @@ function bodies(api: FakeGitHubApi, thread: ThreadRef): Promise<string[]> { return api.listComments(thread).then((list) => list.map((c) => c.body)); } -// ───────────────────────────────────────────────────────────────────────────── -// The module surface -// ───────────────────────────────────────────────────────────────────────────── - test('the module declares exactly the contract\'s github.comments tools', () => { assert.equal(commentsToolModule.featureSet, COMMENTS_FEATURE_SET); assert.deepEqual( @@ -107,10 +92,6 @@ test('an unknown tool name is refused, not thrown', async () => { assert.match(text(result), /unknown tool/); }); -// ───────────────────────────────────────────────────────────────────────────── -// comment / reply_in_thread -// ───────────────────────────────────────────────────────────────────────────── - test('comment posts and books the new comment under the current checkpoint', async () => { const f = fixture(); f.api.seedIssue({ number: 7 }); @@ -186,10 +167,6 @@ test('reply_in_thread replies into a review conversation and books it as a revie assert.match(text(badId), /expected a review conversation/); }); -// ───────────────────────────────────────────────────────────────────────────── -// edit_own_comment / delete_own_comment -// ───────────────────────────────────────────────────────────────────────────── - test('edit_own_comment rewrites the bot\'s comment and books the previous body', async () => { const f = fixture(); f.api.seedIssue({ number: 7 }); @@ -258,8 +235,6 @@ test('delete_own_comment deletes the bot\'s comment and books an undeletable rec assert.equal(f.api.callsTo('deleteComment').length, 0); assert.deepEqual(f.state.snapshot().deleted, {}); - // The refused call already checkpointed (as .9's routing does for any call), - // so the delete books under the checkpoint current at that moment. const current = f.state.current(); assert.notEqual(current, base.id); const result = await call(f, TOOL_DELETE_OWN_COMMENT, { thread: 7, commentId: own.id }); @@ -270,15 +245,10 @@ test('delete_own_comment deletes the bot\'s comment and books an undeletable rec ]); }); -// ───────────────────────────────────────────────────────────────────────────── -// upsert_sticky_comment -// ───────────────────────────────────────────────────────────────────────────── - test('the sticky marker is an HTML comment, however it was passed', () => { assert.equal(stickyMarker('ci-status'), '<!-- ci-status -->'); assert.equal(stickyMarker('<!-- ci-status -->'), '<!-- ci-status -->'); assert.equal(stickyBody('ci-status', 'green'), '<!-- ci-status -->\ngreen'); - // A body that already carries the marker keeps it where the caller put it. assert.equal(stickyBody('ci-status', 'green\n<!-- ci-status -->'), 'green\n<!-- ci-status -->'); }); @@ -305,7 +275,6 @@ test('upsert_sticky_comment creates once, then edits in place, keeping the marke assert.equal(f.api.callsTo('updateComment').length, 1); assert.match(text(second), /updated sticky <!-- ci-status -->/); - // A different marker is a different sticky comment. const other = await call(f, TOOL_UPSERT_STICKY_COMMENT, { thread: 7, marker: 'plan', body: 'the plan' }); assert.notEqual(other.isError, true); assert.equal((await f.api.listComments(ISSUE)).length, 3); @@ -324,10 +293,6 @@ test('upsert_sticky_comment never edits a marker-carrying comment written by som assert.equal(f.api.callsTo('updateComment').length, 0); }); -// ───────────────────────────────────────────────────────────────────────────── -// state/rollback (SPEC §8.5, §8.6) -// ───────────────────────────────────────────────────────────────────────────── - test('rollback after two comments and one edit deletes two and restores one', async () => { const f = fixture(); f.api.seedIssue({ number: 7 }); @@ -346,7 +311,6 @@ test('rollback after two comments and one edit deletes two and restores one', as assert.equal(result.success, true); assert.equal(result.checkpoint, base.id); assert.equal(result.reason, undefined); - // Newest action first: the second comment, the first comment, then the body. assert.deepEqual( result.undone.map((i) => [i.action, i.commentId]), [ @@ -368,7 +332,6 @@ test('rollback across a comment someone else removed reports what it could not u await call(f, TOOL_COMMENT, { thread: 7, body: 'first' }); await call(f, TOOL_COMMENT, { thread: 7, body: 'second' }); - // A maintainer deletes one of them out of band, outside these tools. const gone = (await f.api.listComments(ISSUE))[0]; await f.api.deleteComment(TEST_REPO, gone.id); @@ -398,8 +361,6 @@ test('rollback reports a delete it can never undo, and skips an edit of a commen const result = await rollbackComments({ featureSet: COMMENTS_FEATURE_SET, checkpoint: base.id }, f.ctx); assert.equal(result.success, false); - // The create is undone by the delete; the edit of that same comment needs no - // restore, so it is neither undone nor reported as a failure. assert.deepEqual( result.undone.map((i) => [i.action, i.commentId]), [['delete', created.id]], @@ -411,8 +372,6 @@ test('rollback reports a delete it can never undo, and skips an edit of a commen assert.match(result.notUndone[0].reason ?? '', /GitHub cannot restore a deleted comment/); assert.deepEqual(await bodies(f.api, ISSUE), []); - // The delete stays booked: this checkpoint can never be fully restored, and - // a retry keeps saying so rather than reporting an empty success. const retry = await rollbackComments({ featureSet: COMMENTS_FEATURE_SET, checkpoint: base.id }, f.ctx); assert.equal(retry.success, false); assert.deepEqual(retry.undone, []); @@ -457,15 +416,12 @@ test('rollback survives a restart: the bookkeeping is on the state file', async await call(first, TOOL_EDIT_OWN_COMMENT, { thread: 7, commentId: own.id, body: 'edited before the restart' }); const createdId = (await api.listComments(ISSUE))[1].id; - // A restart: a new store over the same file, the same GitHub behind it. const restarted = GithubState.load(file); assert.deepEqual(restarted.lineage().map((c) => c.id).slice(0, 1), [base.id]); const ctx: ToolContext = { ...first.ctx, state: restarted }; const result = await rollbackComments({ featureSet: COMMENTS_FEATURE_SET, checkpoint: base.id }, ctx); assert.equal(result.success, true); - // Deletes run before restores: a create and an edit of the same comment - // cannot then fight over it. assert.deepEqual( result.undone.map((i) => [i.action, i.commentId]), [ @@ -474,7 +430,6 @@ test('rollback survives a restart: the bookkeeping is on the state file', async ], ); assert.deepEqual(await bodies(api, ISSUE), ['original body']); - // The rollback itself is flushed, so a second restart does not redo it. assert.deepEqual(GithubState.load(file).rollback(base.id), { created: [], edited: [], deleted: [] }); }); @@ -484,7 +439,6 @@ test('recordCreated and recordEdited are the entry points .5 shares with these t const own = f.api.seedComment(ISSUE, { body: 'original body', author: { login: TEST_BOT.login, isBot: true } }); const base = f.state.createCheckpoint(); - // What .5's publish does: write through the api, then book it here. const published = await f.api.createComment(ISSUE, 'published through a channel'); await recordCreated(published, ISSUE, false, f.ctx); await f.api.updateComment(TEST_REPO, own.id, 'rewritten through a channel'); @@ -511,10 +465,6 @@ test('a write outside the allow-list never reaches the api or the state', async assert.equal(f.state.snapshot().created[base.id], undefined); }); -// ───────────────────────────────────────────────────────────────────────────── -// Review round 1 — the four findings -// ───────────────────────────────────────────────────────────────────────────── - test('rollback refuses to touch a repository outside the allow-list, and keeps the record', async () => { const f = fixture(); f.api.seedIssue({ number: 7 }); @@ -523,9 +473,6 @@ test('rollback refuses to touch a repository outside the allow-list, and keeps t const created = (await f.api.listComments(ISSUE))[0]; f.api.clearCalls(); - // The same state, replayed under a context that no longer allows the - // repository the record names (a configuration change, or a stale state - // file). Rollback is a write like any other: the allow-list binds it. const narrowed: ToolContext = { ...f.ctx, repos: [OTHER_REPO] }; const refused = await rollbackComments({ featureSet: COMMENTS_FEATURE_SET, checkpoint: base.id }, narrowed); assert.equal(refused.success, false); @@ -537,8 +484,6 @@ test('rollback refuses to touch a repository outside the allow-list, and keeps t assert.deepEqual(f.api.callLog(), [], 'no api call is made for a refused repository'); assert.deepEqual(await bodies(f.api, ISSUE), ['written while rayforce was allowed']); - // The record survived the refusal, so a correctly configured retry still - // undoes it: a refusal is not a silent discard. const retry = await rollbackComments({ featureSet: COMMENTS_FEATURE_SET, checkpoint: base.id }, f.ctx); assert.equal(retry.success, true); assert.deepEqual(retry.undone.map((i) => i.commentId), [created.id]); @@ -559,8 +504,6 @@ test('a checkpoint abandoned by an earlier rollback raises -32005 instead of rep assert.equal(toA.success, true); assert.deepEqual(await bodies(f.api, ISSUE), ['A']); - // A new write branches from A; B's branch is gone, and with it any way back - // to the state B named. await call(f, TOOL_EDIT_OWN_COMMENT, { thread: 7, commentId: own.id, body: 'C' }); assert.deepEqual(await bodies(f.api, ISSUE), ['C']); @@ -577,7 +520,6 @@ test('a checkpoint abandoned by an earlier rollback raises -32005 instead of rep }, ); assert.deepEqual(await bodies(f.api, ISSUE), ['C'], 'nothing was touched by the refused rollback'); - // Rolling back to A, which is still the current lineage, keeps working. const again = await rollbackComments({ featureSet: COMMENTS_FEATURE_SET, checkpoint: a.id }, f.ctx); assert.equal(again.success, true); assert.deepEqual(await bodies(f.api, ISSUE), ['A']); @@ -587,11 +529,8 @@ test('an issue comment and a review comment sharing an id are two different comm const f = fixture(); f.api.seedIssue({ number: 7 }); f.api.seedPullRequest({ number: 8 }); - // GitHub numbers issue comments and review comments from different - // sequences, so one id can name both. (FakeGitHubApi's updateComment and - // deleteComment ignore the `review` flag when they pick a target — the - // assertions below are on the calls the module makes, not on the fake's - // choice of victim.) + // GitHub numbers issue comments and review comments from separate + // sequences, so one id can name both. const shared = 500; const issueComment = f.api.seedComment(ISSUE, { id: shared, @@ -616,8 +555,6 @@ test('an issue comment and a review comment sharing an id are two different comm const result = await rollbackComments({ featureSet: COMMENTS_FEATURE_SET, checkpoint: base.id }, f.ctx); assert.equal(result.success, true); - // Both actions run: deleting the issue comment must not swallow the restore - // of the review comment that happens to carry the same number. assert.deepEqual(f.api.callsTo('deleteComment')[0].args, [TEST_REPO, shared, false]); assert.deepEqual(f.api.callsTo('updateComment')[0].args, [TEST_REPO, shared, 'original review body', true]); assert.deepEqual( @@ -638,7 +575,6 @@ test('a transient failure keeps its record, so a retry cannot claim a completed await call(f, TOOL_EDIT_OWN_COMMENT, { thread: 7, commentId: own.id, body: 'edited' }); const created = (await f.api.listComments(ISSUE))[1]; - // GitHub is briefly unavailable for both undo calls. const realDelete = f.api.deleteComment.bind(f.api); const realUpdate = f.api.updateComment.bind(f.api); let failures = 2; @@ -663,8 +599,6 @@ test('a transient failure keeps its record, so a retry cannot claim a completed ); assert.deepEqual(await bodies(f.api, ISSUE), ['edited', 'created']); - // Both records are still booked under the checkpoint, so the retry (GitHub - // is back) finishes the job instead of reporting an empty success. const snapshot = f.state.snapshot(); assert.deepEqual(snapshot.created[base.id].map((c) => c.commentId), [created.id]); assert.deepEqual(snapshot.edited[base.id].map((e) => e.previousBody), ['original body']); @@ -681,11 +615,6 @@ test('a transient failure keeps its record, so a retry cannot claim a completed assert.deepEqual(await bodies(f.api, ISSUE), ['original body']); }); -// ───────────────────────────────────────────────────────────────────────────── -// Review round 2 — the two findings -// ───────────────────────────────────────────────────────────────────────────── - -/** Three edits of one comment: A → B → C → D, with the checkpoint at A. */ async function threeEdits(f: Fixture): Promise<{ base: string; own: number }> { f.api.seedIssue({ number: 7 }); const own = f.api.seedComment(ISSUE, { body: 'A', author: { login: TEST_BOT.login, isBot: true } }); @@ -709,15 +638,12 @@ test('rollback restores the body the checkpoint named, never an intermediate one result.undone.map((i) => [i.action, i.commentId]), [['restore', own]], ); - // One update, carrying the oldest body in the range: B and C are states the - // rollback has no reason to pass through. assert.deepEqual( f.api.callsTo('updateComment').map((c) => c.args[2]), ['A'], ); assert.deepEqual(await bodies(f.api, ISSUE), ['A']); - // Nothing is left pending, so a retry is a no-op rather than a second write. assert.deepEqual(f.state.snapshot().edited, {}); f.api.clearCalls(); const retry = await rollbackComments({ featureSet: COMMENTS_FEATURE_SET, checkpoint: base }, f.ctx); @@ -731,7 +657,6 @@ test('a failed restore retains only the oldest body, and the retry lands on it', const f = fixture(); const { base, own } = await threeEdits(f); - // GitHub refuses the first restore attempt. const realUpdate = f.api.updateComment.bind(f.api); let failures = 1; f.api.updateComment = async (repo, commentId, body, review) => { @@ -748,8 +673,6 @@ test('a failed restore retains only the oldest body, and the retry lands on it', ); assert.deepEqual(await bodies(f.api, ISSUE), ['D'], 'the failed restore changed nothing'); - // Exactly one record is retained, and it is the oldest body — retaining the - // superseded ones would let a retry put B back over a restored A. const pending = f.state.snapshot().edited[base]; assert.equal(pending.length, 1); assert.equal(pending[0].previousBody, 'A'); @@ -761,8 +684,6 @@ test('a failed restore retains only the oldest body, and the retry lands on it', [['restore', own]], ); assert.deepEqual(await bodies(f.api, ISSUE), ['A']); - // Across both runs the only body ever written back is the one at the - // checkpoint; the comment never visits B or C. assert.deepEqual([...new Set(f.api.callsTo('updateComment').map((c) => c.args[2]))], ['A']); const again = await rollbackComments({ featureSet: COMMENTS_FEATURE_SET, checkpoint: base }, f.ctx); @@ -772,14 +693,8 @@ test('a failed restore retains only the oldest body, and the retry lands on it', }); /** - * A pull request carrying one issue comment and one review comment that share - * id 500 — two different comments, numbered by two different endpoints. - * - * (`FakeGitHubApi.updateComment`/`deleteComment` ignore the `review` flag when - * they pick a target and always scan the issue comments first, so the - * assertions below are on the arguments the module passes and on the body it - * read, not on which of the two the fake then mutated. Fresh fixtures keep - * that from leaking between cases.) + * GitHub numbers issue comments and review comments separately, so a pull + * request can carry one of each with the same id. */ function colliding(): { f: Fixture; shared: number } { const f = fixture(); @@ -847,7 +762,6 @@ test('edit_own_comment refuses a colliding id until the caller names the endpoin assert.equal(badFlag.isError, true); assert.match(text(badFlag), /^review: true for a review comment/); - // review: true takes the review-conversation comment and its REST path. const reviewCase = colliding(); const reviewCp = reviewCase.f.state.createCheckpoint(); const edited = await call(reviewCase.f, TOOL_EDIT_OWN_COMMENT, { @@ -873,7 +787,6 @@ test('edit_own_comment refuses a colliding id until the caller names the endpoin }, ]); - // review: false takes the top-level comment, on the issue-comments path. const issueCase = colliding(); const issueCp = issueCase.f.state.createCheckpoint(); const issueEdited = await call(issueCase.f, TOOL_EDIT_OWN_COMMENT, { diff --git a/test/toolsIssuesPr.test.ts b/test/toolsIssuesPr.test.ts index ec3aa1c..9e7d786 100644 --- a/test/toolsIssuesPr.test.ts +++ b/test/toolsIssuesPr.test.ts @@ -1,11 +1,3 @@ -/** - * github.issues and github.pr (TASK-SRHKG.1.6). - * - * The two refusals the contract turns on are asserted twice over: a base - * outside GITHUB_PR_BASE_ALLOW on creation and on a retarget, and a repository - * outside GITHUB_REPOS on every write. Everything runs against FakeGitHubApi. - */ - import { strict as assert } from 'node:assert'; import { test } from 'node:test'; import { issuesToolModule, prToolModule, requireAllowedBase } from '../src/tools-issues-pr.js'; @@ -24,12 +16,12 @@ import { } from '../src/types.js'; import { FakeGitHubApi, TEST_BOT, TEST_REPO } from './harness.js'; +const NO_CHECKPOINT_STORE = undefined as unknown as StateStore; + function ctx(api: FakeGitHubApi, over: Partial<ToolContext> = {}): ToolContext { return { api, - // Neither feature set is rollback-capable, so neither touches the - // checkpoint store; an attempt would throw here rather than pass quietly. - state: undefined as unknown as StateStore, + state: NO_CHECKPOINT_STORE, botLogin: TEST_BOT.login, repos: [TEST_REPO], prBaseAllow: ['dev'], @@ -74,10 +66,6 @@ test('the two modules declare their feature sets and no merge tool exists', () = assert.ok(!names.some((n) => /merge|delete_branch/.test(n))); }); -// ───────────────────────────────────────────────────────────────────────────── -// github.issues -// ───────────────────────────────────────────────────────────────────────────── - test('create_issue opens an issue in the default repository', async () => { const api = new FakeGitHubApi(); const out = ok( @@ -184,10 +172,6 @@ test('a GitHub error from an issue write carries its status', async () => { assert.equal(out, 'reopen_issue failed: GitHub error 403: Issue is locked'); }); -// ───────────────────────────────────────────────────────────────────────────── -// github.pr — the base allow-list -// ───────────────────────────────────────────────────────────────────────────── - test('create_pull_request opens a pull request against an allowed base', async () => { const api = new FakeGitHubApi(); const out = ok( @@ -238,7 +222,6 @@ test('edit_pull_request refuses a retarget outside GITHUB_PR_BASE_ALLOW', async const out = failure(await prToolModule.call('edit_pull_request', { number: 8, base: 'main' }, ctx(api))); assert.match(out, /edit_pull_request refused: base branch "main" is not in GITHUB_PR_BASE_ALLOW/); assert.equal(api.calls.length, 0); - // The pull request still points where it did. assert.equal((await api.getPullRequest({ repo: TEST_REPO, number: 8, kind: 'pull' })).base.ref, 'dev'); }); @@ -310,7 +293,6 @@ test('with several repositories, the base allow-list is read per repository', as const second = { owner: TEST_REPO.owner, name: 'chronicle' }; const two = ctx(api, { repos: [TEST_REPO, second], - // rayforce merges into dev, everything else into main. prBaseAllow: ['anima-research/rayforce:dev', 'main'], }); const create = (repo: string, base: string) => @@ -323,7 +305,6 @@ test('with several repositories, the base allow-list is read per repository', as assert.match(refused, /base branch "dev" is not in GITHUB_PR_BASE_ALLOW \(allowed on anima-research\/chronicle: main\)/); assert.equal(api.callsTo('createPullRequest').length, 3); - // And the repository itself can no longer be left out. const missing = failure( await prToolModule.call('create_pull_request', { head: 'fix', base: 'main', title: 'Fix' }, two), ); @@ -355,10 +336,6 @@ test('a GitHub error from a pull-request write carries its status', async () => }); -// ───────────────────────────────────────────────────────────────────────────── -// Echoed arguments are bounded -// ───────────────────────────────────────────────────────────────────────────── - test('a refused base branch is echoed bounded', async () => { const api = new FakeGitHubApi(); const out = failure( @@ -382,7 +359,6 @@ test('request_review bounds the logins it confirms, in count and in length', asy assert.match(out, new RegExp(`… 4 more not shown \\(list bound ${LIMITS.listEntries}\\)`)); assert.ok(!out.includes('reviewer-50'), 'the 51st login is not echoed'); assert.ok(out.length < LIMITS.listEntries * (LIMITS.titleChars + 2) + 200, `confirmation is ${out.length} chars`); - // The request itself is untouched by the rendering bound: all 54 were asked. assert.deepEqual(api.callsTo('requestReviewers')[0].args, [PULL, logins]); }); diff --git a/test/toolsReview.test.ts b/test/toolsReview.test.ts index efef850..571fcb8 100644 --- a/test/toolsReview.test.ts +++ b/test/toolsReview.test.ts @@ -1,15 +1,3 @@ -/** - * github.review (TASK-SRHKG.1.6). - * - * Three things are load-bearing here and each is asserted directly: - * - * - the inline comments reach GitHubApi.createReview with the exact - * positions the caller gave (path, line, side), in order; - * - an APPROVE or REQUEST_CHANGES on the bot's own pull request is refused - * by name, and refused before the review is submitted; - * - a malformed inline position is rejected rather than guessed at. - */ - import { strict as assert } from 'node:assert'; import { test } from 'node:test'; import { ToolArgumentError } from '../src/tool-support.js'; @@ -27,12 +15,14 @@ import { } from '../src/types.js'; import { FakeGitHubApi, TEST_BOT, TEST_REPO } from './harness.js'; +const NO_CHECKPOINT_STORE = undefined as unknown as StateStore; + function ctx(api: FakeGitHubApi, over: Partial<ToolContext> = {}): ToolContext { return { api, - // github.review is not rollback-capable (a submitted review cannot be - // withdrawn), so it never touches the checkpoint store. - state: undefined as unknown as StateStore, + // A submitted GitHub review can't be withdrawn, so github.review keeps no + // checkpoints. + state: NO_CHECKPOINT_STORE, botLogin: TEST_BOT.login, repos: [TEST_REPO], prBaseAllow: ['dev'], @@ -71,10 +61,6 @@ test('the module declares github.review and its three tools', () => { ]); }); -// ───────────────────────────────────────────────────────────────────────────── -// create_review -// ───────────────────────────────────────────────────────────────────────────── - test('create_review submits the inline comments with the exact positions', async () => { const api = new FakeGitHubApi(); api.seedPullRequest({ number: 8, author: HUMAN }); @@ -100,7 +86,6 @@ test('create_review submits the inline comments with the exact positions', async } satisfies CreateReviewInput); assert.match(out, /submitted commented review \d+ on anima-research\/rayforce#8 with 2 inline comments/); - // The fake opened one review thread per inline comment, at those positions. const threads = await api.listReviewThreads(PULL); assert.deepEqual( threads.map((t) => `${t.path}:${t.line}:${t.side}`), @@ -193,7 +178,6 @@ test('a COMMENT review on the bot’s own pull request is allowed and costs no e api.seedPullRequest({ number: 8, author: { login: TEST_BOT.login, isBot: true } }); ok(await reviewToolModule.call('create_review', { number: 8, event: 'COMMENT', body: 'note to self' }, ctx(api))); - // No getPullRequest: only a verdict needs to know who the author is. assert.deepEqual(api.callLog(), ['createReview']); }); @@ -240,10 +224,6 @@ test('a review on a repository outside GITHUB_REPOS is refused by name', async ( assert.equal(api.calls.length, 0); }); -// ───────────────────────────────────────────────────────────────────────────── -// Inline comment positions -// ───────────────────────────────────────────────────────────────────────────── - test('an inline comment position is validated, never guessed', async () => { const api = new FakeGitHubApi(); api.seedPullRequest({ number: 8, author: HUMAN }); @@ -261,7 +241,7 @@ test('an inline comment position is validated, never guessed', async () => { assert.match(failure(await review([{ path: 'a.rs', line: 0, body: 'x' }])), /\.line" must be a positive integer/); assert.match(failure(await review([{ path: 'a.rs', line: -3, body: 'x' }])), /\.line" must be a positive integer/); assert.match(failure(await review([{ path: 'a.rs', line: 2.5, body: 'x' }])), /\.line" must be a positive integer/); - // A line number as a string is not a line number: GitHub would 422 on it. + // GitHub answers a line given as a string with a 422. assert.match(failure(await review([{ path: 'a.rs', line: '42', body: 'x' }])), /got "42"/); assert.match( failure(await review([{ path: 'a.rs', line: 42, side: 'BOTH', body: 'x' }])), @@ -285,17 +265,11 @@ test('parseReviewComments is exact and total', () => { assert.throws(() => parseReviewComments([{ path: 'a.rs', line: 1, body: '' }]), ToolArgumentError); }); -// ───────────────────────────────────────────────────────────────────────────── -// resolve_thread / unresolve_thread -// ───────────────────────────────────────────────────────────────────────────── - test('resolve_thread and unresolve_thread move a thread of the named pull request', async () => { const api = new FakeGitHubApi(); api.seedPullRequest({ number: 8, author: HUMAN }); api.seedReviewThread(PULL, { id: 'RT_kwDO', isResolved: false }); - // `repo` omitted: the single allowed repository is the one membership is - // established in. const resolved = ok(await reviewToolModule.call('resolve_thread', { number: 8, threadId: 'RT_kwDO' }, ctx(api))); assert.equal(resolved, 'resolved review thread RT_kwDO on anima-research/rayforce#8'); assert.deepEqual(api.callLog(), ['listReviewThreads', 'resolveReviewThread']); @@ -329,8 +303,8 @@ test('resolve_thread refuses a node id that is not on the pull request', async ( const api = new FakeGitHubApi(); api.seedPullRequest({ number: 8, author: HUMAN }); api.seedReviewThread(PULL, { id: 'RT_ours', isResolved: false }); - // A thread in a repository the allow-list does not cover. Its node id names - // no repository, so the only thing that can refuse it is membership. + // A review thread's GraphQL node id names no repository, so only the + // allow-list's membership check can refuse a foreign thread. const foreignPull: ThreadRef = { repo: OTHER_REPO, number: 8, kind: 'pull' }; api.seedPullRequest({ repo: OTHER_REPO, number: 8 }); api.seedReviewThread(foreignPull, { id: 'RT_foreign', isResolved: false }); @@ -340,7 +314,6 @@ test('resolve_thread refuses a node id that is not on the pull request', async ( out, /resolve_thread refused: review thread "RT_foreign" is not one of the 1 review thread on anima-research\/rayforce#8/, ); - // Nothing was mutated: not the foreign thread, not the allowed one. assert.deepEqual(api.callLog(), ['listReviewThreads']); assert.equal(api.callsTo('resolveReviewThread').length, 0); assert.equal((await api.listReviewThreads(foreignPull))[0].isResolved, false); @@ -373,8 +346,6 @@ test('a thread id on another pull request of the allowed repository is refused', test('resolve_thread needs the pull request, a thread id, and rejects the channel thread id', async () => { const api = new FakeGitHubApi(); - // The pull request is required: without it there is nothing to establish - // membership against. assert.match( failure(await reviewToolModule.call('resolve_thread', { threadId: 'RT_x' }, ctx(api))), /"number" must be a positive integer, got undefined/, @@ -383,7 +354,6 @@ test('resolve_thread needs the pull request, a thread id, and rejects the channe failure(await reviewToolModule.call('resolve_thread', { number: 8 }, ctx(api))), /"threadId" is required and must be a non-empty string/, ); - // `rt:<comment id>` addresses a channel thread, not the GraphQL node. assert.match( failure(await reviewToolModule.call('resolve_thread', { number: 8, threadId: 'rt:1001' }, ctx(api))), /"threadId" is the review thread id from list_review_threads \(a GraphQL node id\), not the channel thread id "rt:1001"/, @@ -425,8 +395,6 @@ test('the input schemas advertise what the runtime enforces', () => { const comments = byName.get('create_review')!.inputSchema.properties.comments as { items: { properties: Record<string, Record<string, unknown>>; required: string[] }; }; - // A line is an integer ≥ 1, not any number: a host that validates the schema - // refuses 2.5 and "42" before the call, as parseReviewComments does after. assert.equal(comments.items.properties.line.type, 'integer'); assert.equal(comments.items.properties.line.minimum, 1); assert.deepEqual(comments.items.properties.side.enum, ['LEFT', 'RIGHT']); @@ -435,7 +403,6 @@ test('the input schemas advertise what the runtime enforces', () => { for (const name of ['resolve_thread', 'unresolve_thread']) { const schema = byName.get(name)!.inputSchema; - // The pull request is part of the contract of these tools now. assert.deepEqual(schema.required, ['number', 'threadId']); assert.equal((schema.properties.number as Record<string, unknown>).type, 'integer'); assert.equal((schema.properties.number as Record<string, unknown>).minimum, 1); diff --git a/test/wp8-policies.test.ts b/test/wp8-policies.test.ts index 50a232e..7f2c61f 100644 --- a/test/wp8-policies.test.ts +++ b/test/wp8-policies.test.ts @@ -1,19 +1,10 @@ /** - * The example recipes' wake policies, run through the host's own matcher. + * The example recipes' wake policies, run through agent-framework's own + * EventGate matcher: a wake policy set is ordered, first-match-wins, and + * `metadataTrue` is an OR-list. * - * A wake policy set is ordered, first-match-wins, and `metadataTrue` is an - * OR-list — three facts that make "obviously correct" orderings wrong in - * ways only evaluation catches (review round 1: an `isOwnThread` wake placed - * above the CI-success skip woke the agent on every green build of its own - * pull request). So these tests do not re-implement the semantics: they load - * `EventGate` from connectome's agent-framework and ask `probe()`, which is - * the same matcher the host runs, side-effect-free and offline. - * - * agent-framework is not a dependency of this package — it is a sibling - * checkout. When it cannot be found (a standalone clone, CI), these tests - * skip with the paths they looked in; the contract checks in - * wp8-docs.test.ts still run. Point GITHUB_MCPL_GATE_MODULE at - * `dist/src/gate/event-gate.js` to force a location. + * agent-framework is not a dependency of this package, so these tests load it + * from a sibling checkout and skip when there is none. */ import { strict as assert } from 'node:assert'; @@ -28,7 +19,6 @@ const packageDir = fileURLToPath(new URL('..', import.meta.url)); const recipe = (name: string): any => JSON.parse(readFileSync(join(packageDir, 'examples', name), 'utf8')); -/** The host's event shape, as much of it as a wake policy can match on. */ interface GateEventInfo { content: string; eventType: string; @@ -50,12 +40,6 @@ interface GateModule { formatShadowWarning(warning: unknown): string; } -/** - * Where a sibling agent-framework checkout can be: beside this package in a - * `repos/` tree, or — when this package is a linked git worktree, whose - * parent directory holds nothing else — beside the checkout its `.git` file - * points at. - */ function gateModuleCandidates(): string[] { const seeds = [packageDir]; const dotGit = join(packageDir, '.git'); @@ -93,12 +77,10 @@ async function loadGate(): Promise<GateModule> { return (await import(gatePath as string)) as GateModule; } -/** An EventGate over one recipe's wake config, with every host callback inert. */ async function gateFor(recipeName: string): Promise<{ probe(info: GateEventInfo): GateDecision }> { const { EventGate } = await loadGate(); return new EventGate({ - // A fresh temp dir per gate: the constructor seeds gate.json there, and - // nothing in this test may touch a real agent's config. + // EventGate's constructor seeds gate.json at configPath. configPath: join(mkdtempSync(join(tmpdir(), 'github-mcpl-gate-')), 'gate.json'), initialConfig: recipe(recipeName).modules.wake, emitTrace: () => {}, @@ -112,15 +94,12 @@ const skipReason = `no agent-framework beside this checkout (${candidates.length} paths tried); ` + 'set GITHUB_MCPL_GATE_MODULE to the gate module to run these'; -// ─── the events, as tags.ts (.4) will produce them ────────────────────────── - const base = { eventType: 'mcpl:push-event', serverId: 'github', channelId: 'github:owner/repo#12', }; -/** A green build on the agent's own pull request. */ const ownThreadCiSuccess: GateEventInfo = { ...base, content: 'All checks have passed', @@ -128,7 +107,6 @@ const ownThreadCiSuccess: GateEventInfo = { metadata: { isOwnThread: true, isCiFailure: false, fromSelf: false, repo: 'owner/repo', number: 12 }, }; -/** The same pull request, build broken. */ const ownThreadCiFailure: GateEventInfo = { ...base, content: 'build-and-test failed', @@ -136,7 +114,6 @@ const ownThreadCiFailure: GateEventInfo = { metadata: { isOwnThread: true, isCiFailure: true, fromSelf: false, repo: 'owner/repo', number: 12 }, }; -/** Dependabot opens a pull request nobody asked the agent about. */ const botOpenedPull: GateEventInfo = { ...base, channelId: 'github:owner/repo#13', @@ -145,7 +122,6 @@ const botOpenedPull: GateEventInfo = { metadata: { isOwnThread: false, mentioned: false, fromSelf: false, repo: 'owner/repo', number: 13 }, }; -/** A human opens a pull request. */ const humanOpenedPull: GateEventInfo = { ...base, channelId: 'github:owner/repo#14', @@ -154,7 +130,6 @@ const humanOpenedPull: GateEventInfo = { metadata: { isOwnThread: false, mentioned: false, isMaintainer: true, repo: 'owner/repo', number: 14 }, }; -/** A human says the agent's name. */ const humanMention: GateEventInfo = { ...base, eventType: 'mcpl:channel-incoming', @@ -163,7 +138,6 @@ const humanMention: GateEventInfo = { metadata: { mentioned: true, isOwnThread: false, fromSelf: false, repo: 'owner/repo', number: 12 }, }; -/** The agent's own comment coming back round. */ const ownComment: GateEventInfo = { ...base, eventType: 'mcpl:channel-incoming', @@ -172,7 +146,6 @@ const ownComment: GateEventInfo = { metadata: { fromSelf: true, isOwnThread: true, repo: 'owner/repo', number: 12 }, }; -/** A push to a branch, no thread attached. */ const push: GateEventInfo = { ...base, channelId: 'github:owner/repo#0', @@ -181,8 +154,6 @@ const push: GateEventInfo = { metadata: { isOwnThread: false, fromSelf: false, repo: 'owner/repo' }, }; -// ─── what both recipes must do ─────────────────────────────────────────────── - for (const name of ['recipe-maintainer-clone.json', 'recipe-contributor.json']) { test(`${name}: a green build on the agent's own thread does not wake it`, { skip: gatePath ? false : skipReason }, async () => { const d = (await gateFor(name)).probe(ownThreadCiSuccess); @@ -228,8 +199,6 @@ for (const name of ['recipe-maintainer-clone.json', 'recipe-contributor.json']) }); } -// ─── where they differ ─────────────────────────────────────────────────────── - test('recipe-maintainer-clone.json: a human opening a pull request debounces, it does not wake at once', { skip: gatePath ? false : skipReason }, async () => { const d = (await gateFor('recipe-maintainer-clone.json')).probe(humanOpenedPull); assert.equal(d.policyName, 'github-new-work');