From feb0fff9fa9c38ff8ba79d1648794d16e5228ff3 Mon Sep 17 00:00:00 2001 From: Ogden-2688 Date: Wed, 28 Oct 2026 19:30:14 -0400 Subject: [PATCH 1/5] test: run the wake-policy tests in CI The example recipes' wake policies were checked against agent-framework's EventGate only where a sibling checkout happened to exist. CI and a standalone clone skipped all 18, so the ordering they pin (which rule wins each event under first-match-wins, the ordering an earlier round got wrong) was checked nowhere routinely. wp8-docs.test.ts checks only the recipes' shape: swapping github-machine-noise with github-direct-address in recipe-maintainer-clone.json passes every one of its checks and fails three of these. - CI's suite job installs @animalabs/agent-framework with --no-save after the type-check, so package.json and the lock stay as they are and an operator's npm ci installs nothing more. Without a version it takes the newest release, so a matcher change there turns this CI red on the next pull request, which is when the recipes need a look. As a dev dependency it would add 43 packages and about 100 MB to every operator's install. - Off CI the tests still skip, and the skip reason now names the --no-save install. With CI set, a missing module fails them with that reason as the message, so a runner that lost the step can't turn them into checks that pass without running. - "the gate matcher was actually exercised" goes. It asserted the module path under a skip that already required it, so it could not fail. - The header's paragraph on where agent-framework comes from goes; the skip reason says how to provide it. --- .github/workflows/ci.yml | 3 +++ test/wp8-policies.test.ts | 35 ++++++++++++++--------------------- 2 files changed, 17 insertions(+), 21 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8623127..0423b9a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -39,6 +39,9 @@ jobs: - name: Type-check run: npm run typecheck + - name: Install agent-framework for the wake-policy tests, unsaved + run: npm install --no-save @animalabs/agent-framework + - name: Test run: npm test diff --git a/test/wp8-policies.test.ts b/test/wp8-policies.test.ts index 50a232e..5278148 100644 --- a/test/wp8-policies.test.ts +++ b/test/wp8-policies.test.ts @@ -8,12 +8,6 @@ * 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. */ import { strict as assert } from 'node:assert'; @@ -88,6 +82,7 @@ const candidates = gateModuleCandidates(); const gatePath = candidates.find((p) => existsSync(p)); async function loadGate(): Promise { + assert.ok(gatePath, skipReason); // A computed specifier on purpose: the path is outside the package, so it // must not become a compile-time module resolution. return (await import(gatePath as string)) as GateModule; @@ -110,7 +105,9 @@ async function gateFor(recipeName: string): Promise<{ probe(info: GateEventInfo) const skipReason = `no agent-framework beside this checkout (${candidates.length} paths tried); ` + - 'set GITHUB_MCPL_GATE_MODULE to the gate module to run these'; + 'run `npm install --no-save @animalabs/agent-framework`, or set GITHUB_MCPL_GATE_MODULE to the gate module, to run these'; + +const skip = !gatePath && !process.env.CI && skipReason; // ─── the events, as tags.ts (.4) will produce them ────────────────────────── @@ -184,44 +181,44 @@ const push: GateEventInfo = { // ─── 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 () => { + test(`${name}: a green build on the agent's own thread does not wake it`, { skip }, async () => { const d = (await gateFor(name)).probe(ownThreadCiSuccess); assert.equal(d.trigger, false, `woke via ${d.policyName}`); assert.equal(d.policyName, 'github-machine-noise'); }); - test(`${name}: a broken build on the agent's own thread wakes it`, { skip: gatePath ? false : skipReason }, async () => { + test(`${name}: a broken build on the agent's own thread wakes it`, { skip }, async () => { const d = (await gateFor(name)).probe(ownThreadCiFailure); assert.equal(d.trigger, true); assert.equal(d.policyName, 'github-ci-failure-own-thread'); }); - test(`${name}: a bot's pull request neither wakes nor debounces`, { skip: gatePath ? false : skipReason }, async () => { + test(`${name}: a bot's pull request neither wakes nor debounces`, { skip }, async () => { const d = (await gateFor(name)).probe(botOpenedPull); assert.equal(d.trigger, false); assert.equal(d.policyName, 'github-other-bots', 'a bot must be excluded before any wake or debounce rule'); assert.equal(d.behavior, 'skip', 'skip, not a debounce that fires 60 s later'); }); - test(`${name}: a human mention wakes it`, { skip: gatePath ? false : skipReason }, async () => { + test(`${name}: a human mention wakes it`, { skip }, async () => { const d = (await gateFor(name)).probe(humanMention); assert.equal(d.trigger, true); assert.equal(d.policyName, 'github-direct-address'); }); - test(`${name}: its own comment does not wake it`, { skip: gatePath ? false : skipReason }, async () => { + test(`${name}: its own comment does not wake it`, { skip }, async () => { const d = (await gateFor(name)).probe(ownComment); assert.equal(d.trigger, false); assert.equal(d.policyName, 'github-self'); }); - test(`${name}: a push does not wake it`, { skip: gatePath ? false : skipReason }, async () => { + test(`${name}: a push does not wake it`, { skip }, async () => { const d = (await gateFor(name)).probe(push); assert.equal(d.trigger, false); assert.equal(d.policyName, 'github-machine-noise'); }); - test(`${name}: no rule is unreachable`, { skip: gatePath ? false : skipReason }, async () => { + test(`${name}: no rule is unreachable`, { skip }, async () => { const { findShadowedPolicies, formatShadowWarning } = await loadGate(); const warnings = findShadowedPolicies(recipe(name).modules.wake.policies); assert.deepEqual(warnings.map(formatShadowWarning), [], 'first match wins: an earlier rule eats a later one'); @@ -230,20 +227,20 @@ 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 () => { +test('recipe-maintainer-clone.json: a human opening a pull request debounces, it does not wake at once', { skip }, async () => { const d = (await gateFor('recipe-maintainer-clone.json')).probe(humanOpenedPull); assert.equal(d.policyName, 'github-new-work'); assert.deepEqual(d.behavior, { debounce: 60000 }); assert.equal(d.trigger, false, 'a debounce never triggers inline; it fires once the burst settles'); }); -test('recipe-contributor.json: a human opening a pull request is ambient, the contributor does not triage', { skip: gatePath ? false : skipReason }, async () => { +test('recipe-contributor.json: a human opening a pull request is ambient, the contributor does not triage', { skip }, async () => { const d = (await gateFor('recipe-contributor.json')).probe(humanOpenedPull); assert.equal(d.trigger, false); assert.equal(d.policyName, 'github-ambient'); }); -test('recipe-contributor.json: a thread the agent took part in wakes it, issue or pull request', { skip: gatePath ? false : skipReason }, async () => { +test('recipe-contributor.json: a thread the agent took part in wakes it, issue or pull request', { skip }, async () => { const gate = await gateFor('recipe-contributor.json'); for (const kind of ['issue', 'pull'] as const) { const d = gate.probe({ @@ -257,7 +254,3 @@ test('recipe-contributor.json: a thread the agent took part in wakes it, issue o assert.equal(d.policyName, 'github-participated-threads'); } }); - -test('the gate matcher was actually exercised', { skip: gatePath ? false : skipReason }, () => { - assert.ok(gatePath, 'resolved agent-framework'); -}); From 3ee2faf3f7cb89ae00b00f293bd18fee1b161370 Mon Sep 17 00:00:00 2001 From: Ogden-2688 Date: Wed, 28 Oct 2026 19:30:14 -0400 Subject: [PATCH 2/5] test: skip the two README host tests off CI only, naming what is missing "the credential split creates both files at mode 600" runs README's app-manifests block, which calls jq three times, and "an empty host reaches the registration script by following step 1" clones this checkout. On a host without jq, or in a copy that isn't a git checkout, they failed for that reason alone, and a run that always fails two tests reads as broken. Each now skips with the reason when its prerequisite is missing, and only when CI is unset or empty. Under CI a missing prerequisite still fails the test, so a runner image without jq can't turn the credential check into one that cannot fail. --- test/wp8-docs.test.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/test/wp8-docs.test.ts b/test/wp8-docs.test.ts index 2ff0b24..459e9d4 100644 --- a/test/wp8-docs.test.ts +++ b/test/wp8-docs.test.ts @@ -326,7 +326,11 @@ test('README: the manifest-callback listener parses and captures a code', async } }); -test('README: the credential split creates both files at mode 600', () => { +const noJq = spawnSync('jq', ['--version']).error !== undefined; + +test('README: the credential split creates both files at mode 600', { + skip: noJq && !process.env.CI && 'jq is not on PATH, and the block calls it', +}, () => { const block = shellBlock('README.md', 'app-manifests'); const { home, cfg } = freshHome(); try { @@ -465,7 +469,9 @@ test('README: the checkout exists before a step runs something out of it', () => 'docs/deploy.md builds the checkout README step 1 made'); }); -test('README: an empty host reaches the registration script by following step 1', () => { +test('README: an empty host reaches the registration script by following step 1', { + skip: !existsSync(root('.git')) && !process.env.CI && 'this copy is not a git checkout, and step 1 clones it', +}, () => { const home = mkdtempSync(join(tmpdir(), 'wp8-host-')); try { const step1 = setupBlocks().find((b) => /git clone/.test(b)); From 7dc216c1f566f6bc6bbda5648ffd08afcd8a348a Mon Sep 17 00:00:00 2001 From: Ogden-2688 Date: Wed, 28 Oct 2026 19:40:30 -0400 Subject: [PATCH 3/5] test: remove tests that repeat another check or test nothing of the server Each removed test either repeats a check that stays, or checks a test double or a type rather than what the server does. Repeated by a test that stays: - content "an event with a body but nothing in it says so instead of rendering blank": "only an all-whitespace body counts as empty" renders the same event with ' ' among its blanks and asserts the (no body) tail. - tags "tags are deduplicated and namespaced": the per-kind tagsFor table tests compare each kind's sorted tags exactly, and no row lists a duplicate or an unnamespaced tag. - integration "finding 6: plain MCP keeps the whole tool surface": "a plain-MCP host sees tools only" already compared plain MCP's listing with the whole tool surface; the one check of its own, that the comment tool is listed by name, moves there. - poller "cursors are whole seconds, so the second a pass starts in is read again": it read the stored strings only. githubAdapter's "a row changed later in the second a pass started in arrives next pass" shows the behaviour through the next pass's since and what it delivers. Already checked by the type-check CI runs over src and test: - githubAdapter "every GitHubApi method is implemented on the adapter": GitHubAdapter implements GitHubApi. - poller "IngressState is a structural subset of the frozen StateStore": its runtime assertions checked the file's own double; the assignment is checked where src/index.ts hands the Poller the GithubState. A test double tested against itself: - poller "the gate the state keeps is bounded by the contract POLL_DEDUP_RING" checked MemoryIngressState, this file's double. The store's own ring is pinned by state's "the poll dedup ring remembers the last 2000 keys and no more". --- test/content.test.ts | 5 ----- test/githubAdapter.test.ts | 8 -------- test/integration.test.ts | 10 +--------- test/poller.test.ts | 31 ------------------------------- test/tags.test.ts | 8 -------- 5 files changed, 1 insertion(+), 61 deletions(-) diff --git a/test/content.test.ts b/test/content.test.ts index 466c540..a965627 100644 --- a/test/content.test.ts +++ b/test/content.test.ts @@ -193,11 +193,6 @@ test('a bot actor is marked as one in the header', () => { assert.ok(text.includes('ci[bot] (bot)'), text); }); -test('an event with a body but nothing in it says so instead of rendering blank', () => { - const text = only(renderEvent(event('comment.created', { body: ' ' }))); - assert.ok(text.includes('(no body)')); -}); - test('an event that carries no body renders no body section', () => { const text = only(renderEvent(event('comment.deleted'))); assert.ok(!text.includes('(no body)')); diff --git a/test/githubAdapter.test.ts b/test/githubAdapter.test.ts index 519fff5..2660111 100644 --- a/test/githubAdapter.test.ts +++ b/test/githubAdapter.test.ts @@ -33,7 +33,6 @@ import { import { Poller } from '../src/poller.js'; import { GithubState } from '../src/state.js'; import { - GITHUB_API_METHODS, readConfig, silentLogger, type NormalizedEvent, @@ -734,13 +733,6 @@ test('a GraphQL errors array becomes a GitHubApiError, not a silent empty list', // Every GitHubApi method maps to its documented call // ───────────────────────────────────────────────────────────────────────────── -test('every GitHubApi method is implemented on the adapter', async () => { - const r = await rig(); - for (const name of GITHUB_API_METHODS) { - assert.equal(typeof (r.adapter as unknown as Record)[name], 'function', `missing ${name}`); - } -}); - test('reads hit the documented REST paths and map the payloads', async () => { const r = await rig(); diff --git a/test/integration.test.ts b/test/integration.test.ts index 1601559..a0a7fb8 100644 --- a/test/integration.test.ts +++ b/test/integration.test.ts @@ -304,6 +304,7 @@ test('a plain-MCP host sees tools only: no manifest, no push, no channels, no ro // server useless to an ordinary MCP client. const listed = (await h.host.sendRequest('tools/list', {})) as { tools: { name: string }[] }; assert.equal(listed.tools.length, h.server.toolDefinitions().length); + assert.ok(listed.tools.some((tool) => tool.name === 'comment')); assert.match(textOf(await callTool(h, 'get_issue', { number: 41 })), /#41/); // Every MCPL method does not exist on this connection. @@ -2085,15 +2086,6 @@ test('finding 5: a publish Notification on an ungranted capability writes nothin assert.deepEqual(api.callsTo('createComment'), [], 'the refusal was logged, not written'); }); -test('finding 6: plain MCP keeps the whole tool surface', async (t) => { - const h = rig(t); - await initialize(h, false); - const listed = (await h.host.sendRequest('tools/list', {})) as { tools: { name: string }[] }; - // No grant exists on this connection, so there is nothing to filter by. - assert.equal(listed.tools.length, h.server.toolDefinitions().length); - assert.ok(listed.tools.some((tool) => tool.name === 'comment')); -}); - test('finding 7: a thread the bot opened is its own thread with no comment on it', async (t) => { const api = new FakeGitHubApi(); api.seedIssue({ number: 41, author: { login: TEST_BOT.login, isBot: true } }); diff --git a/test/poller.test.ts b/test/poller.test.ts index c8933a0..c72fff9 100644 --- a/test/poller.test.ts +++ b/test/poller.test.ts @@ -29,7 +29,6 @@ import { type NormalizedEvent, type PollCursor, type RepoRef, - type StateStore, } from '../src/types.js'; import { FakeGitHubApi, TEST_BOT, TEST_REPO } from './harness.js'; @@ -786,20 +785,6 @@ test('a title or body edit, and a reopen, still arrive under polling', async () // 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); - } -}); - 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. @@ -933,13 +918,6 @@ test('a stored position with milliseconds is read from its whole second', async ); }); -test('cursors are whole seconds, so the second a pass starts in is read again', async () => { - const r = rig({ now: '2026-10-14T00:00:00.700Z' }); - await r.poller.pollOnce(); - assert.equal(r.state.lastPollAt(), '2026-10-14T00:00:00.000Z'); - assert.deepEqual(r.state.pollCursors()['anima-research/rayforce:issues'], { since: '2026-10-14T00:00:00.000Z' }); -}); - test('a refused first pass keeps the window it read', async () => { const api = new FakeGitHubApi(); api.seedIssue({ number: 41, updatedAt: '2026-10-13T23:59:30.000Z' }); @@ -958,15 +936,6 @@ test('a refused first pass keeps the window it read', async () => { assert.equal(r.state.lastPollAt(), first.since, 'the retry asks from the same start, not one moved on by the clock'); }); -test('the gate the state keeps is bounded by the contract POLL_DEDUP_RING', async () => { - 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 // ───────────────────────────────────────────────────────────────────────────── diff --git a/test/tags.test.ts b/test/tags.test.ts index 1c56fe5..8f52f80 100644 --- a/test/tags.test.ts +++ b/test/tags.test.ts @@ -207,14 +207,6 @@ test('every tag the table expects is in the declared vocabulary', () => { assert.deepEqual(sorted(EMITTED_TAGS), sorted([...CORE_TAGS, ...GITHUB_TAGS])); }); -test('tags are deduplicated and namespaced', () => { - for (const row of TAG_TABLE) { - const { tags } = tagsFor(event(row.kind, row.event), ctx(row.ctx)); - assert.equal(new Set(tags).size, tags.length, `${row.kind} emitted a duplicate`); - for (const tag of tags) assert.match(tag, /^(chat|github):/, `${tag} is not namespaced`); - } -}); - // ───────────────────────────────────────────────────────────────────────────── // Closure rules // ───────────────────────────────────────────────────────────────────────────── From e8825238c5909b409f1f9e9f9c27977128b5e169 Mon Sep 17 00:00:00 2001 From: Ogden-2688 Date: Wed, 28 Oct 2026 19:44:21 -0400 Subject: [PATCH 4/5] test: make the state crash test crash, and retitle the polled-heads test "a crash between the write and the rename never replaces the state" names the requirement that the state file is never left half-written, and no other test pins it: "flush writes a versioned state file atomically and leaves no temp file" checks a flush that finished. The old body produced no crash. It wrote a truncated temp file by hand, outside flush's path, so nothing in it could fail if flush wrote the state file in place. It now runs a child process that flushes once, copies the state file it wrote, then stops with exit 7 at its next flush's rename, after the temp file is written and synced. The test asserts that the state file is the copy byte for byte, and that it reloads with only the first flush's write. The stop is in a child, so the shared test process's fs/promises is never changed. With flush mutated to write the state file in place, the child exits 0 and the test fails. It passes on Node 22.12.0, CI's suite version, and 24.22.1. "the polled heads are bounded, survive a flush, and tolerate an older file (amendment 2a)" neither flushes nor loads an older file (the next test loads one). It is retitled to what it checks: the bound, oldest out first, and re-recording a head refreshing its place. --- test/state.test.ts | 44 ++++++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/test/state.test.ts b/test/state.test.ts index 204f7ec..b245295 100644 --- a/test/state.test.ts +++ b/test/state.test.ts @@ -7,6 +7,7 @@ */ import { strict as assert } from 'node:assert'; +import { spawnSync } from 'node:child_process'; import { mkdtempSync, readFileSync, readdirSync, statSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -176,25 +177,32 @@ test('a flushed state reloads with its lineage and its rollback reach', async () assert.deepEqual(plan.edited.map((e) => e.previousBody), ['before']); }); -test('a crash between the write and the rename never replaces the state', async () => { +test('a crash between the write and the rename never replaces the state', () => { const file = tempFile(); - const state = GithubState.load(file); - const cp = state.createCheckpoint(); - state.recordCreatedComment({ repo: REPO, commentId: 11, review: false }); - 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'); - - assert.equal(readFileSync(file, 'utf8'), good); + // node:fs/promises' named exports, as state.ts imports them, follow a change + // to its CommonJS exports only once syncBuiltinESMExports() has run. + const firstFlush = `${file}.first-flush`; + const crash = ` + import { copyFileSync } from 'node:fs'; + import { createRequire, syncBuiltinESMExports } from 'node:module'; + import { GithubState } from ${JSON.stringify(new URL('../src/state.ts', import.meta.url).href)}; + const state = GithubState.load(${JSON.stringify(file)}); + state.createCheckpoint(); + state.recordCreatedComment({ repo: ${JSON.stringify(REPO)}, commentId: 11, review: false }); + await state.flush(); + copyFileSync(${JSON.stringify(file)}, ${JSON.stringify(firstFlush)}); + createRequire(import.meta.url)('node:fs/promises').rename = () => process.exit(7); + syncBuiltinESMExports(); + state.recordCreatedComment({ repo: ${JSON.stringify(REPO)}, commentId: 12, review: false }); + await state.flush(); + `; + const r = spawnSync(process.execPath, ['--import', 'tsx', '--input-type=module', '-e', crash], { encoding: 'utf8' }); + assert.equal(r.status, 7, `the second flush should have stopped at its rename:\n${r.stderr}`); + + assert.equal(readFileSync(file, 'utf8'), readFileSync(firstFlush, 'utf8'), 'the state file is the first flush\'s, byte for byte'); const reloaded = GithubState.load(file); - assert.deepEqual(reloaded.lineage().map((c) => c.id), [cp.id]); - assert.deepEqual(reloaded.snapshot().created[cp.id].map((c) => c.commentId), [11]); - assert.deepEqual(strayTempFiles(file), [`state.json.tmp-99999-deadbeef`]); + const [cp] = reloaded.lineage(); + assert.deepEqual(reloaded.rollback(cp.id)?.created.map((c) => c.commentId), [11]); }); test('concurrent flushes serialize and the file always parses', async () => { @@ -262,7 +270,7 @@ test('the poll dedup ring remembers the last 2000 keys and no more', () => { assert.equal(state.seenPollKey(`issue:${POLL_DEDUP_RING - 1}:t`), true); }); -test('the polled heads are bounded, survive a flush, and tolerate an older file (amendment 2a)', async () => { +test('the polled heads are bounded, oldest out first, and re-recording a head refreshes its place', async () => { const state = GithubState.empty(); assert.equal(state.polledHead('github:anima-research/rayforce#58'), null); state.setPolledHead('github:anima-research/rayforce#58', 'a'.repeat(40)); From 1ddd961c0ccd9656f9a93336c253ee53c6d1f8a0 Mon Sep 17 00:00:00 2001 From: Ogden-2688 Date: Wed, 28 Oct 2026 19:44:21 -0400 Subject: [PATCH 5/5] test: GitHub read counts as upper bounds, and assert what titles name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Read counts take three forms now. A cost is at most its bound: tests that counted GitHub reads per event asserted exact numbers, so a change that read less would fail them, though fewer reads is more rate-limit headroom. A premise is at least one read: where a test warms a thread's history before the case it is about, the warming read is asserted as having happened, since with an upper bound alone the cache and invalidation tests would pass without ever being warmed. A count stays exact only where the number itself is the requirement. - integration: the history and author reads in "a human reply to the bot's own review comment is a reply to self", "a review reply a tool wrote is remembered…", "a successful tool write updates the cached thread knowledge", the two round 3 finding 3 tests and round 3 finding 2. Where a thread is warmed first, the warming reads are premises and the reads after them are bounds. "an issue thread costs two bounded lookups" is now "costs at most two lookups". - poller: the transient-failure, still-running, given-up and refused-retry tests each follow one head, and now assert at most one check-run lookup in each pass, through a helper that runs one pass. "at most 20 pull-request heads are checked in one pass" asserts at most 20 lookups and reports; the round-robin test still pins a full 20 in pass one. Where a read has to happen, the test already shows it through what the read alone can know: the reply-to-self flag that needs the history, or the delivery the retry makes. Titles and what the tests assert: - poller "the backlog is bounded: past its cap the oldest head waiting is forgotten" asserted only the size, which a backlog dropping the newest head also passes. It now drains the backlog and asserts the two oldest heads are the ones gone, comparing the heads looked up as a set. With the eviction mutated to drop the newest, it fails. - toolsReview "a review without inline comments passes comments: undefined" pinned the call object's own key with a strict deepEqual. The adapter sends comments to GitHub only when the list is non-empty, so it is now "a review without inline comments sends none, and says so", asserting the event, the body, no comments, and the tool's "with 0 inline comments". - tags "every tag the table expects is in the declared vocabulary": the line comparing EMITTED_TAGS with [...CORE_TAGS, ...GITHUB_TAGS] goes; it restated src/tags.ts's own definition. --- test/integration.test.ts | 32 ++++++++++++++-------------- test/poller.test.ts | 46 ++++++++++++++++++++++------------------ test/tags.test.ts | 1 - test/toolsReview.test.ts | 11 +++++----- 4 files changed, 46 insertions(+), 44 deletions(-) diff --git a/test/integration.test.ts b/test/integration.test.ts index a0a7fb8..a8f694a 100644 --- a/test/integration.test.ts +++ b/test/integration.test.ts @@ -748,12 +748,12 @@ test('a human reply to the bot’s own review comment is a reply to self', async // The lookup is two bounded reads for a pull request, and then cached: a // second event on the same thread asks GitHub nothing. - assert.equal(api.callsTo('listComments').length, 1); - assert.equal(api.callsTo('listReviewThreads').length, 1); + assert.ok(api.callsTo('listComments').length >= 1, 'the first event read the comments'); + assert.ok(api.callsTo('listReviewThreads').length >= 1, 'and the review threads'); await deliver('issue_comment', fixture('issue_comment.created.on_pull.json'), 'reply-2'); await until(() => pushes(h).length === 2, 'the second event'); - assert.equal(api.callsTo('listComments').length, 1, 'the thread history is cached'); - assert.equal(api.callsTo('listReviewThreads').length, 1); + assert.ok(api.callsTo('listComments').length <= 1, 'the thread history is cached'); + assert.ok(api.callsTo('listReviewThreads').length <= 1); // The second event is on the same thread, so it is own-thread too. const second = (pushes(h)[1].origin as { metadata: Record }).metadata; assert.equal(second.isOwnThread, true); @@ -1422,7 +1422,7 @@ test('a review reply a tool wrote is remembered, so a human answer to it is a re await deliver('issue_comment', fixture('issue_comment.created.on_pull.json'), 'warm-1'); await until(() => pushes(h).length === 1, 'the warming event'); assert.equal(metadataOf(pushes(h)[0]).isOwnThread, false, 'the bot is not in this thread yet'); - assert.equal(api.callsTo('listReviewThreads').length, 1); + assert.ok(api.callsTo('listReviewThreads').length >= 1, 'the warming event loaded the history'); const written = await callTool(h, 'reply_in_thread', { thread: PULL, @@ -1451,7 +1451,7 @@ test('a review reply a tool wrote is remembered, so a human answer to it is a re assert.equal(metadata.isOwnThread, true); assert.equal(metadata.isReplyToSelf, true, 'the comment it answers is the one the tool wrote'); assert.ok((pushes(h)[1].tags ?? []).includes('chat:reply'), (pushes(h)[1].tags ?? []).join(',')); - assert.equal(api.callsTo('listReviewThreads').length, 1, 'and nothing was looked up a second time'); + assert.ok(api.callsTo('listReviewThreads').length <= 1, 'and nothing was looked up a second time'); }); test('an issue comment the bot wrote is never taken for the review comment a reply names', async (t) => { @@ -1489,7 +1489,7 @@ test('an issue comment the bot wrote is never taken for the review comment a rep assert.equal(metadata.isReplyToSelf, false, 'but not the review comment this answers'); }); -test('an issue thread costs two bounded lookups: the author and the comments', async (t) => { +test('an issue thread costs at most two lookups: the author and the comments', async (t) => { const api = new FakeGitHubApi(); api.seedIssue({ number: 41 }); api.seedComment(ISSUE, { author: TEST_BOT, body: 'on it' }); @@ -1504,8 +1504,8 @@ test('an issue thread costs two bounded lookups: the author and the comments', a const metadata = (pushes(h)[0].origin as { metadata: Record }).metadata; assert.equal(metadata.isOwnThread, true, 'the bot has spoken on this issue'); assert.equal(metadata.mentioned, true, 'and the body mentions it'); - assert.equal(api.callsTo('getIssue').length, 1, 'the author, for a thread the bot may have opened'); - assert.equal(api.callsTo('listComments').length, 1); + assert.ok(api.callsTo('getIssue').length <= 1, 'at most one author read, for a thread the bot may have opened'); + assert.ok(api.callsTo('listComments').length <= 1, 'at most one comments read'); assert.equal(api.callsTo('listReviewThreads').length, 0, 'an issue has no review threads'); assert.equal(api.callsTo('getPullRequest').length, 0, 'and it is not a pull request'); }); @@ -2122,7 +2122,7 @@ test('finding 7: a successful tool write updates the cached thread knowledge', a await h.server.onEvent(commentEvent('after-write')); await until(() => pushes(h).length === 2, 'the second event'); assert.equal((pushes(h)[1].origin as { metadata: { isOwnThread: boolean } }).metadata.isOwnThread, true); - assert.equal(api.callsTo('listComments').length, lookups, 'and it cost no new history lookup'); + assert.ok(api.callsTo('listComments').length <= lookups, 'and it cost no new history lookup'); }); test('finding 7: a write recorded before a restart is still an own thread', async (t) => { @@ -2399,7 +2399,7 @@ test('round 3, finding 2: a history read that completes after a tool write keeps assert.equal(metadata.isOwnThread, true, 'the write is still known'); assert.equal(metadata.isReplyToSelf, true, 'and so is the id it created'); assert.ok((pushes(h)[1].tags ?? []).includes('chat:reply'), (pushes(h)[1].tags ?? []).join(',')); - assert.equal(api.callsTo('listReviewThreads').length, 1, 'one history read, and it was not repeated'); + assert.ok(api.callsTo('listReviewThreads').length <= 1, 'at most one history read'); }); test('round 3, finding 3: a tool touch leaves the thread unloaded, so the first event still reads its history', async (t) => { @@ -2423,12 +2423,12 @@ test('round 3, finding 3: a tool touch leaves the thread unloaded, so the first const metadata = metadataOf(pushes(h)[0]); assert.equal(metadata.isOwnThread, true); assert.equal(metadata.isReplyToSelf, true, 'the bot wrote the comment this answers'); - assert.equal(api.callsTo('listReviewThreads').length, 1, 'exactly one history read'); + assert.ok(api.callsTo('listReviewThreads').length <= 1, 'at most one history read'); // And it is loaded now: a second event asks GitHub nothing. await h.server.onEvent(commentEvent('second', PULL)); await until(() => pushes(h).length === 2, 'the second event'); - assert.equal(api.callsTo('listReviewThreads').length, 1); + assert.ok(api.callsTo('listReviewThreads').length <= 1); }); test('round 3, finding 3: create_review’s inline comments invalidate the history, and one reload finds them', async (t) => { @@ -2443,7 +2443,7 @@ test('round 3, finding 3: create_review’s inline comments invalidate the histo // again on its own. await h.server.onEvent(commentEvent('warm', PULL)); await until(() => pushes(h).length === 1, 'the warming event'); - assert.equal(api.callsTo('listReviewThreads').length, 1); + assert.ok(api.callsTo('listReviewThreads').length >= 1, 'the warming event loaded the history'); const review = await callTool(h, 'create_review', { number: 58, @@ -2463,7 +2463,7 @@ test('round 3, finding 3: create_review’s inline comments invalidate the histo await h.server.onEvent(reviewReply('answer-the-inline-one', inline.id, 2500000079)); await until(() => pushes(h).length === 2, 'the human reply'); assert.equal(metadataOf(pushes(h)[1]).isReplyToSelf, true, 'the reload found the inline comment'); - assert.equal(api.callsTo('listReviewThreads').length, before + 1, 'exactly one reload'); + assert.ok(api.callsTo('listReviewThreads').length <= before + 1, 'at most one reload'); // The submitted review's own id is not a comment id, and is never treated // as one: a reply naming it is not a reply to the bot. @@ -2471,5 +2471,5 @@ test('round 3, finding 3: create_review’s inline comments invalidate the histo await h.server.onEvent(reviewReply('answer-the-review-id', submitted.id, 2500000080)); await until(() => pushes(h).length === 3, 'the reply naming the review id'); assert.equal(metadataOf(pushes(h)[2]).isReplyToSelf, false); - assert.equal(api.callsTo('listReviewThreads').length, before + 1, 'and the record is loaded again'); + assert.ok(api.callsTo('listReviewThreads').length <= before + 1, 'and naming the review id reads nothing more'); }); diff --git a/test/poller.test.ts b/test/poller.test.ts index c72fff9..cf93a58 100644 --- a/test/poller.test.ts +++ b/test/poller.test.ts @@ -161,6 +161,14 @@ function rig(opts: RigOptions = {}): Rig { return { api, state, poller, events, checkCalls }; } +async function onePass(r: Rig) { + const before = r.checkCalls.length; + const result = await r.poller.pollOnce(); + const lookups = r.checkCalls.length - before; + assert.ok(lookups <= 1, `${lookups} check-run lookups for one head in one pass`); + return result; +} + /** A completed summary for `sha`, failing on one job. */ function failedSummary(sha: string): ChecksSummary { return { @@ -574,8 +582,8 @@ test('at most 20 pull-request heads are checked in one pass', async () => { const r = rig({ api }); const result = await r.poller.pollOnce(); assert.equal(MAX_CHECKED_PULLS_PER_POLL, 20); - assert.equal(api.callsTo('getCheckRuns').length, 20); - assert.equal(result.delivered.filter((e) => e.kind === 'checks.completed').length, 20); + assert.ok(api.callsTo('getCheckRuns').length <= 20); + assert.ok(result.delivered.filter((e) => e.kind === 'checks.completed').length <= 20); }); test('a check-runs lookup that throws does not lose the rest of the pass, and the head is kept', async () => { @@ -953,7 +961,7 @@ 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(); + const first = await onePass(r); assert.deepEqual( first.delivered.map((e) => e.kind), // Amendment 2a: this pull request was created before the cursor and the @@ -965,7 +973,7 @@ test('a transient lookup failure is retried on the next pass, after the cursor m // 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(); + const second = await onePass(r); assert.deepEqual((await api.listIssuesSince(TEST_REPO, r.state.lastPollAt() as string)).rows, []); assert.deepEqual( second.delivered.map((e) => e.kind), @@ -974,7 +982,6 @@ test('a transient lookup failure is retried on the next pass, after the cursor m assert.equal(second.delivered[0].sha, pull.head.sha); assert.deepEqual(second.delivered[0].checks?.failed.map((f) => f.name), ['clippy']); assert.equal(second.checksBacklog, 0, 'a settled head leaves the backlog'); - assert.deepEqual(r.checkCalls, [pull.head.sha, pull.head.sha]); }); test('a head whose checks are still running waits, then reports when they finish', async () => { @@ -993,7 +1000,7 @@ test('a head whose checks are still running waits, then reports when they finish // 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(); + const result = await onePass(r); assert.deepEqual( result.delivered.filter((e) => e.kind === 'checks.completed'), [], @@ -1003,14 +1010,13 @@ test('a head whose checks are still running waits, then reports when they finish } finished = true; - const last = await r.poller.pollOnce(); + const last = await onePass(r); assert.deepEqual( last.delivered.map((e) => e.kind), ['checks.completed'], ); assert.equal(last.delivered[0].checks?.conclusion, 'failure'); assert.equal(last.checksBacklog, 0); - assert.equal(r.checkCalls.length, 4); }); test('a head that never gets check runs is given up on after its attempts', async () => { @@ -1020,17 +1026,16 @@ test('a head that never gets check runs is given up on after its attempts', asyn // matches does. const r = rig({ api, maxHeadAttempts: 3 }); - assert.equal((await r.poller.pollOnce()).checksBacklog, 1); - assert.equal((await r.poller.pollOnce()).checksBacklog, 1); - const last = await r.poller.pollOnce(); + assert.equal((await onePass(r)).checksBacklog, 1); + assert.equal((await onePass(r)).checksBacklog, 1); + const last = await onePass(r); assert.equal(last.checksBacklog, 0, 'the head aged out'); assert.deepEqual( 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((await onePass(r)).checkLookups, 0); assert.equal(MAX_HEAD_ATTEMPTS, 10); }); @@ -1086,18 +1091,19 @@ test('an over-cap backlog drains round-robin instead of starving the tail', asyn test('the backlog is bounded: past its cap the oldest head waiting is forgotten', async () => { const api = new FakeGitHubApi(); + const pulls: ReturnType[] = []; for (let i = 1; i <= 6; i++) { - api.seedPullRequest({ number: 300 + i, updatedAt: '2026-10-13T12:00:00.000Z' }); + pulls.push(api.seedPullRequest({ number: 300 + i, updatedAt: '2026-10-13T12:00:00.000Z' })); } const r = rig({ api, maxHeadBacklog: 4, - maxCheckedPulls: 0, // no draining, so only the enqueue bound is on trial + maxCheckedPulls: 4, getCheckRuns: async (_repo, sha) => ({ sha, conclusion: null, total: 1, failed: [] }), }); const result = await r.poller.pollOnce(); - assert.equal(result.checkLookups, 0); assert.equal(result.checksBacklog, 4); + assert.deepEqual([...r.checkCalls].sort(), pulls.slice(2).map((p) => p.head.sha).sort(), 'the two oldest heads are the ones gone'); assert.equal(MAX_HEAD_BACKLOG, 200); }); @@ -1207,7 +1213,7 @@ 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(); + const first = await onePass(r); assert.deepEqual( first.delivered.map((e) => e.kind), // Amendment 2a: this pull request was created before the cursor and the @@ -1224,7 +1230,7 @@ test('a checks event refused long after the cursor moved past its row is still r // Pass two: the checks settle and the listener refuses. settled = true; - const second = await r.poller.pollOnce(); + const second = await onePass(r); assert.equal(second.failed, 1); assert.equal(second.delivered.length, 0); assert.equal(second.checksBacklog, 1, 'the head must survive the refusal'); @@ -1232,7 +1238,7 @@ test('a checks event refused long after the cursor moved past its row is still r assert.equal(second.cursorAdvanced, false); // Pass three: the retry lands, and only now is the head retired. - const third = await r.poller.pollOnce(); + const third = await onePass(r); assert.deepEqual( third.delivered.map((e) => e.kind), ['checks.completed'], @@ -1241,8 +1247,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]); }); test('a checks event the gate has already spoken for retires its head instead of waiting forever', async () => { diff --git a/test/tags.test.ts b/test/tags.test.ts index 8f52f80..716528c 100644 --- a/test/tags.test.ts +++ b/test/tags.test.ts @@ -204,7 +204,6 @@ test('every tag the table expects is in the declared vocabulary', () => { assert.ok(EMITTED_TAGS.includes(tag), `${tag} is not declared in EMITTED_TAGS`); } } - assert.deepEqual(sorted(EMITTED_TAGS), sorted([...CORE_TAGS, ...GITHUB_TAGS])); }); // ───────────────────────────────────────────────────────────────────────────── diff --git a/test/toolsReview.test.ts b/test/toolsReview.test.ts index efef850..5b906a1 100644 --- a/test/toolsReview.test.ts +++ b/test/toolsReview.test.ts @@ -124,16 +124,15 @@ test('side defaults to RIGHT, the post-image', async () => { ]); }); -test('a review without inline comments passes comments: undefined', async () => { +test('a review without inline comments sends none, and says so', async () => { const api = new FakeGitHubApi(); api.seedPullRequest({ number: 8, author: HUMAN }); const out = ok(await reviewToolModule.call('create_review', { number: 8, event: 'COMMENT', body: 'looks fine' }, ctx(api))); - assert.deepEqual(api.callsTo('createReview')[0].args[1], { - event: 'COMMENT', - body: 'looks fine', - comments: undefined, - }); + const input = api.callsTo('createReview')[0].args[1] as CreateReviewInput; + assert.equal(input.event, 'COMMENT'); + assert.equal(input.body, 'looks fine'); + assert.equal((input.comments ?? []).length, 0); assert.match(out, /with 0 inline comments/); });