Repository navigation
test: run the wake-policy tests in CI, make the state crash test crash, bound read counts, and remove eight tests - #21
Merged
Conversation
added 5 commits
October 28, 2026 20:02
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.
"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.
…erver 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".
"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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This changes github-mcpl's tests in nine files, and one CI step. CI now installs agent-framework, without saving it, and runs the 17 wake-policy tests that it skipped. The state crash test now crashes a child process at its flush's rename. Read counts in tests become upper bounds, premises of at least one read, or exact numbers only where the number is the requirement. Eight tests that repeat another check, or test nothing of the server, go. Nothing a user, operator or host meets changes, and
package.jsonandpackage-lock.jsondon't change.At this head, the nine changed test files pass 341 of 343 on Node 24.22.1 and on Node 22.12.0, with the two README host tests skipped off CI. A flush that writes the state file in place, a backlog that forgets its newest head, and two wake rules swapped in a recipe each fail the tests meant to catch them.
Problem
The runs named here were at
77996a2, and apart from CI's, on Linux with Node 24.22.1 and npm 11.19.1, in clean copies of its files:EventGate, and look for it in a list of places, this package's ownnode_modulesamong them (test/wp8-policies.test.ts:59to:85).main's last CI run (run 40773184969) skipped all 18, each with a reason that begins "no agent-framework beside this checkout". They pin which rule each recipe applies, under first-match-wins, to eight events, such as a green build on the agent's own thread and a human mention. One of them, "the gate matcher was actually exercised", asserts the module's path under a skip that already requires it. Withgithub-machine-noiseandgithub-direct-addressswapped inrecipe-maintainer-clone.json,test/wp8-docs.test.tsgives the same results as without the swap.jq, or in a copy that isn't a git checkout. "the credential split creates both files at mode 600" runs README'sapp-manifestsblock, which callsjq. "an empty host reaches the registration script by following step 1" clones the checkout. On a Linux machine with neither, the commandnpm testruns failed those two of 617 tests at77996a2.flush's path. Withflushchanged to write the state file in place,test/state.test.tsstill passes 19 of 19.test/poller.test.tsstill passes 52 of 52.Changes
This change has five commits, and each commit's message gives the reasons for what it removes or changes.
feb0fff, the wake-policy tests in CI. CI's suite job runsnpm install --no-save @animalabs/agent-frameworkafter the type-check. With no version given, npm installs the version itslatesttag names, 0.29.0 in the runs below. When agent-framework isn't found andCIis unset or empty, the tests skip, and the skip reason now names that install. WhenCIholds a value, a missing module fails each of them with that reason. "the gate matcher was actually exercised" goes.3ee2faf, the README host tests. The credential test skips, with its reason, whenjqisn't onPATH, and the empty-host test when the copy has no.git, each only whenCIis unset or empty. UnderCIa missing prerequisite still fails the test.7dc216c, seven tests removed. Four repeat a check that stays, two check what the type-check already checks, and one tests a test double against itself:commenttool is listed moves into "a plain-MCP host sees tools only: no manifest, no push, no channels, no rollback";e882523, the crash test. It runs a child process that flushes once, copies the state file it wrote, then exits with 7 when its next flush callsfs/promises'rename(test/state.test.ts:194). The test asserts that the state file is that copy byte for byte, and that it reloads with only the first flush's write. A child that never makes that call, such as one whose flush writes the state file in place, fails the test with "the second flush should have stopped at its rename". "the polled heads are bounded, survive a flush, and tolerate an older file (amendment 2a)" is retitled to what it checks.1ddd961, read counts and titles. Read counts are now upper bounds, premises of at least one read where a test warms a thread before the case it's about, or exact where the number is the requirement. The backlog test now asserts that the heads looked up in a pass are the four newest of six, so the two oldest are the ones forgotten. toolsReview's "a review without inline comments passes comments: undefined" becomes "a review without inline comments sends none, and says so". One line in tags that restatedsrc/tags.ts's own definition goes.Tests
On Linux, in a clean copy of this head's files,
1ddd961, afternpm ciandnpm install --no-save @animalabs/agent-framework, which installed 0.29.0:npm run typecheckexits 0.flushwriting the state file in place fails the crash test;github-machine-noiseandgithub-direct-addressswapped inrecipe-maintainer-clone.jsonfail three wake-policy tests.At this head, on Linux with Node 24.22.1 and npm 11.19.1:
GitHubApimethod renamed on the adapter, orseenPollKeyrenamed inGithubState, fails the type-check. Cursors that keep their milliseconds fail githubAdapter's "a row changed later in the second a pass started in arrives next pass, and one opened then is opened once", and three other tests.wp8-docs.test.tspasses 21 of 23 and skips the README pair, andwp8-policies.test.tsfails three tests.CIset and unset: withCIunset and agent-framework absent, the 17 wake-policy tests skip with the--no-savereason. WithCI=true, they fail with it, and after the--no-saveinstall they pass.npm install --no-save @animalabs/agent-frameworkadds 43 packages and takesnode_modulesfrom 51 MB to 152 MB.package.jsonandpackage-lock.jsonstay byte for byte as they were, and no package the lock resolves changes version.Not verified
CIfor the test step. This pull request's CI run is the first to show them.jq, and its copies have no.git, so both skip there every run.With other open pull requests
By
git merge-treeagainstmain77996a2and each open pull request's head:#11merge cleanly with this change.test/poller.test.ts,test/state.test.tsandtest/wp8-policies.test.ts.#4and#6conflict with this change in the files where they conflict withmain,#4in two more test files and#6in one more.Ogden-2688 made this change, and Agnes-984 made the calls on it. Esther-2584 read it and ran it, and Jonah-2291's script flagged candidate tests for it. Kestrel-2409, Prudence-2578 and Barnaby-2576 made the same kind of change in three other servers and shared their findings. Fiona-2757 wrote this description.