Restore the cursor tests #295 deleted - #299
Merged
Merged
Conversation
#295 removed `aSearchedInboxIsRefusedTheAdvance` and `theLegacyInboxCommandMovesNoCursor` and added nothing in their place, so the two rules it exists to enforce shipped with no test at all: `search` combined with `advanceCursor` had zero coverage anywhere in the repo, and the string that names the version skew appeared in no test. The behaviour is right — the review verified it by probe against the merged daemon — but nothing would catch it being undone. These are the review's own probes, which go further than the deleted pair. They drive the decoded query through `GraphStore.mailbox` after a `Codable` round trip rather than trusting the CLI never to send the pair; they check `highestDeliveredID` in every answer shape including a room pruned under a reader whose cursor sits below what survived; they exercise the page through `Mailroom.serve` with more synthetic posts than a room can hold, which is the only way to reach the paging path now that a page is the room's own cap; and they assert the legacy refusal reaches the asking connection while a bystander's socket stays empty. That last one needs a blocking read with a deadline. The review's first attempt used `MSG_DONTWAIT` immediately after `handle`, which races the outbound channel and reads empty — indistinguishable from a broadcast that never happened, and it gave a false pass on a build where the bug was still present. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BP43ags4cn8fq2ZZdv85J9
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
1 participant
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.
Test-only, on merged code.
#295 removed
aSearchedInboxIsRefusedTheAdvanceandtheLegacyInboxCommandMovesNoCursorand added nothing in their place. So the two rules that PR exists to enforce shipped with no test at all:searchcombined withadvanceCursorhad zero coverage anywhere in the repo, and the string naming the version skew appeared in no test.The behaviour is correct — the review verified it by probe against the merged daemon, and I verified the source myself — but nothing would catch it being undone. Found by the #295 review after the merge; confirmed by grepping
main.What these cover
These are the review's own probes, and they go further than the pair that was deleted:
aSearchedInboxIsRefusedAndMovesNothing— drives the decoded query throughGraphStore.mailboxafter aCodableround trip, rather than trusting that the CLI never sends the pair. The protocol is reachable by the remote shim and by anything a loop writes itself.highestDeliveredIDIsHonestInEveryShape— every answer shape, including a room pruned under a reader whose cursor sits below what survived.onePageAlwaysHoldsEverythingTheRoomCanKeep— exercises the page throughMailroom.servewith more synthetic posts than a room can hold, which is the only way to reach the paging path now that a page is the room's own cap.theLegacyRefusalMustNotBeBroadcastToBystanders— the refusal reaches the asking connection while a bystander's socket stays empty.One trap worth knowing
That last test needs a blocking read with a deadline. The review's first attempt used
MSG_DONTWAITimmediately afterhandle, which races the outbound channel and reads empty — indistinguishable from a broadcast that never happened. It gave a false pass on a build where the bug was still present, and the reviewer caught it only by re-running with a 1s deadline.Anyone writing a socket assertion in this codebase will hit that.
Not included
A fifth probe,
aReaderThatNeverReadIsStillToldWhatItLost, fails onmain:prunedUnreadis0for a reader whoselastMailroomReadisnil, so a loop created and reading late is told it lost nothing when posts were in fact pruned. That is a real defect in shipped code, not in these tests, and it needs a decision about stamping the cursor at node creation — which would also change whether a new loop sees the existing backlog. Filed separately rather than smuggled in here.Verified: the four tests pass on current
main(cf90bb6),xcodebuildexit 0.Related to #288, #295.
🤖 Generated with Claude Code
https://claude.ai/code/session_01BP43ags4cn8fq2ZZdv85J9