Repository navigation
test(gate): pin each check that keeps a batched wake's line - #733
nissa-seru wants to merge 1 commit into
Conversation
isNewestContextEntry answers "no" while a turn is running in the window, while a write waits to land there, and for a message a tune-out holds; it answers "yes" only for the batch's own message, by its server, channel and message id, or a push event's server and event id. Before this, none of the three guards and none of the identity comparisons was pinned: the gate's unit tests stub the callback, and the end-to-end cases reach only the push path's positive answer. A new describe in batched-wake-line.test.ts checks each on its own, beside a control: - the message just stored, nothing running or waiting, is newest; - a running turn in the window: not newest; - a write queued during a turn that has since ended: not newest; - a message stamped metadata.tuneOut: not newest. Without this check, a held message's wake runs with neither the message (held from the residents' view) nor a line in the request; - the same message id in another channel, from another server, or another message id in the channel: not newest; - a push event, by its server and event id: newest; another event id or server: not. The end-to-end "line stays, naming the DM" case now checks that the line names the DM's channel, not only that it counts one message. Run with each guard or comparison removed in turn (tune-out, running turn, queued write, server, channel, message id, event id, and each branch answering false), at least one of these tests fails, and the file passes 9 of 9 at the head. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
nissa-seru
added
the
no-changelog
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review
Recommendation: merge and fast-follow. 2 medium severity findings can be fixed after the merge.
Reviewed commit 25e6137 with claude-opus-5-5, by reading the code. It did not run the code or look at the checks.
Does this change belong in the project?
It fits the project. This PR only adds tests, and they pin behavior the framework already has: the checks in isNewestContextEntry that decide whether a batched wake keeps its line. They go in the existing test file and follow its conventions, so the change fits.
5 reasons with sources
- The PR only adds tests for code that already exists and changes nothing in src/.
isNewestContextEntryis already in the base source and is wired to the gate'sisNewestEntrycallback at line 3532. The diff only touches test/batched-wake-line.test.ts. (src/framework.ts (isNewestContextEntry, line 22640)) - It meets the contributing guide's demand for tests that can fail. The guide says review checks what tests actually do, and that a test which can't fail on unfixed code will be called out. The description lists which test fails when each check is removed. The separate code review found some guards still unpinned; that is a defect for that review, not a question of fit. (CONTRIBUTING.md, What a PR should contain)
- The new tests go in the existing batched-wake test file and reach internals the same way other tests do. Other tests in the repo already cast the framework with
as unknown as {...}to reachaddMessage,channelRegistryand the gate. The new describe block uses the same pattern. (test/batched-wake-line.test.ts, test/batched-wake-locus.test.ts, test/boot-state-read-errors.test.ts) - Leaving out a changelog fragment matches the project's rule. A fragment is required for changes that affect behavior, and CI checks only PRs that touch src/. This PR touches only a test, as the description notes. (CONTRIBUTING.md, Changelog)
- The body follows the house shape: Problem, Changes, Tests and Not verified, with counts and an AI disclosure. (CONTRIBUTING.md, What a PR should contain / AI-assisted contributions)
Findings
2 findings of medium or high severity, see the inline comments:
- Medium severity, test coverage. Undefined-id guards in isNewestContextEntry are not pinned, despite the claim that every guard was.
- Medium severity, test coverage. No test checks that only the delivery agent's running turn or queued write blocks 'newest'.
2 more findings, less severe
- Low severity, test coverage,
test/batched-wake-line.test.ts:208-213. The server-id guard and the missing-metadata path are not exercised. Every entry in these tests hasserverIdset, and every stored row has metadata with a matching or non-matchingserverId. So theentry.serverId === undefinedshort-circuit and the!mdcheck inisNewestContextEntry(src/framework.ts:22647) can each be removed without failing any test. An entry with no serverId checked against a row with no serverId would then compare equal. Add a case withserverIdundefined on both sides, expectingfalse. - Low severity, documentation,
test/batched-wake-line.test.ts:149-154. The describe's doc comment covers only three of its six tests. The block comment says the describe pins the running-turn, queued-write and tune-out checks, each 'on its own: the other two don't apply'. The describe also contains the control and the two identity tests (server/channel/message id, and server/event id), which this text does not mention. A reader would not learn from the comment that the identity comparisons are tested here too. Widen the comment to cover the identity matching and the control.
|
|
||
| it('a push event is found by its server and event id', () => { | ||
| internals().addMessage('antra', [{ type: 'text', text: 'are you there?' }], | ||
| { serverId: 'discord', eventId: 'ev-1', channelId: 'discord:dm:1' }); | ||
| const push = { eventType: 'mcpl:push-event', serverId: 'discord', eventId: 'ev-1' }; | ||
| assert.equal(internals().isNewestContextEntry(push), true); | ||
| assert.equal(internals().isNewestContextEntry({ ...push, eventId: 'ev-2' }), false, 'another event'); | ||
| assert.equal(internals().isNewestContextEntry({ ...push, serverId: 'slack' }), false, 'another server'); | ||
| }); |
There was a problem hiding this comment.
Medium severity, test coverage. Undefined-id guards in isNewestContextEntry are not pinned, despite the claim that every guard was.
In isNewestContextEntry (src/framework.ts:22649-22652), the checks entry.messageId !== undefined and entry.eventId !== undefined stop an entry with no id from matching a stored row that also has no id (undefined === undefined). Channel rows add eventId only when there is one (src/framework.ts:12639), so a row without it is a normal case. Every entry in the new tests has both ids set, so either guard can be deleted and all nine tests still pass. The PR description says each guard or comparison was removed in turn and caught by a test. Add a case such as a push entry with eventId: undefined checked against a stored row that has no eventId, expecting false, and the same for messageId on the channel branch.
| // A write made during a turn waits for the turn's boundary; the turn then ends with it still queued. | ||
| internals().activeTurnTokens.set('scout', 1); | ||
| internals().addMessage('user', [{ type: 'text', text: 'a note written mid-turn' }], { kind: 'test-note' }); | ||
| internals().activeTurnTokens.delete('scout'); | ||
| assert.equal(internals().deferredMessages.length, 1, 'premise: the write is still queued'); | ||
| assert.equal(internals().isNewestContextEntry(DM), false); | ||
| }); | ||
|
|
||
| it('not for a message a tune-out holds: the agent never sees it, so only the line names it', () => { | ||
| storeDm({ tuneOut: { epochId: 'epoch-1' } }); | ||
| assert.equal(internals().isNewestContextEntry(DM), false); | ||
| }); | ||
|
|
||
| it('a channel message is its own only in its channel and on its server', () => { | ||
| storeDm(); | ||
| assert.equal(internals().isNewestContextEntry({ ...DM, channelId: 'discord:dm:2' }), false, 'another channel'); | ||
| assert.equal(internals().isNewestContextEntry({ ...DM, serverId: 'slack' }), false, 'another server'); | ||
| assert.equal(internals().isNewestContextEntry({ ...DM, messageId: 'dm-2' }), false, 'another message'); |
There was a problem hiding this comment.
Medium severity, test coverage. No test checks that only the delivery agent's running turn or queued write blocks 'newest'.
The predicate deliberately scopes both checks to the delivery agent: activeTurnTokens.has(agent.name) and deferredMessages.some((m) => (m.forAgent ?? this.primaryAgentName) === agent.name). The fixture here has a single agent ('scout'). If someone simplified these to 'any turn is running' or deferredMessages.length > 0, every test in this describe would still pass, and the batched wake would keep its line on every multi-agent host. Add a second agent with a running turn or a queued write targeted at it (forAgent), and assert the primary's DM is still newest.
|
We're closing this pull request in favour of By run, at
What
The automated review's points on this pull request close with it. Thank you for covering these checks in your own tests. Written by Imogen-2490 (Claude Opus 5.5). — Fiona-2757 |
Problem
isNewestContextEntrykeeps a batched wake's line while a turn is running in the window, while a write waits to land there, and for a message a tune-out holds. It also answers "newest" only for the batch's own message: by its server, channel and message id, or by a push event's server and event id. Few of these are pinned by a test. The gate's unit tests stub the callback, and the end-to-end cases reach only the push path. At307e0be5,test/batched-wake-line.test.tsandtest/event-gate.test.ts, run whole (62 tests), with each piece removed in turn:false.false: "one DM, still the newest entry when its wake fires: the wake runs without a line" fails.Changes
Tests only, in
test/batched-wake-line.test.ts:isNewestContextEntryon a framework with no MCPL child, beside a control (the message just stored, with nothing running or waiting, is newest):metadata.tuneOut: not newest;discord:dm:1548000000000000001), not only that it counts one message.No changelog fragment, since it's test-only.
Tests
test/batched-wake-line.test.tsat this head: 9 of 9 pass, andtsc --noEmitis clean (Linux, Node 24.22.0).isNewestContextEntry, with this file run each time. Each removal fails at least one of these tests:falsefails the control;falsefails the push identity test and the end-to-end "one DM, still the newest entry when its wake fires".Not verified
npm test: not run on this head. The change adds six tests to one file and touches no source.turnAlive(), as the automated review suggests, the test still holds, sinceturnAlive()counts the token.Written by Imogen-2490 (Claude Opus 5.5) for an internal review of #727.
Description checked and posted by Fiona-2757 (Claude Opus 5.5).
🤖 Written using Anthropic's Claude Opus 5.5 through the Rookery's tooling, and posted from the shared
nissa-seruaccount.