feat(pull): name the push server's refusal codes, and record them in the lab - #560
Merged
Merged
Conversation
…the lab A publish the push server refuses is not answered and does not error — it closes the WebSocket with a code and a reason string. `CloseReasons` knew one of those, `4010`. The rest were magic numbers, and `/pull-lab` was not listening for `close` at all, which is why a refused frame and an un-echoed frame looked identical to it across several rounds of measurement. Eleven codes added, separated by what they mean rather than by their range. `WRONG_REQUEST_DATA`, `NO_CHANNELS_FOUND`, `PRIVATE_CHANNEL_NOT_ALLOWED`, `INVALID_CHANNEL_SIGNATURE` and others reject a FRAME; `WRONG_CHANNEL_ID`, `NO_PUBLIC_CHANNEL_ID` and `TOO_MANY_CONNECTIONS` sit in the same numeric range and are connection-level, so `isFrameRefusalCloseCode()` exists rather than leaving callers to test for 401x. The docblock also marks the direction: everything below 4010 is a code the CLIENT sends to `disconnect()`, and none of the new ones may be used that way. The lab attaches a `close` listener beside its frame tap and exports every closure with `frameRefusal`, alongside a new `encodeWindow` giving check 9's start and end — `checks[].ms` is a duration, so without it the report invited a correlation it gave the reader no way to make. Closures reset per run and are capped, like the other collectors; the server's `reason` is truncated and called out in the export's warning, being the only string in the report the page did not author. What this does NOT do is prove anything by its absence, and the surrounding text now says so everywhere it had started to imply otherwise. Only a structurally unparseable frame trips `4013`; a scalar written at the wrong field number parses cleanly, is broadcast with an empty body, and produces no close code and no echo — which is exactly what a `warn` looks like. `pull-protobuf.md` is reworked around the same discipline. An audit of an on-prem stand's push-server `.proto` files reports field numbers matching ours on the send side, verified here against `model.js` and the lite codec. That report is not in this repository and cannot be checked from it; the stand it came from names a push-server version on a different axis from the protocol number this codec targets, and had publishing disabled. So it raises confidence in the schema without closing the question, it covers the send direction only, and the document now says all of that instead of declaring the risk closed. The exit criterion is split by which risk a DELETION actually moves: not the schema, which is equally wrong in both implementations, but whether the lite codec agrees with `model.js` and whether it fails the same way where the schema is silent. Both need fuzzing rather than a portal. See #559. Smaller corrections the audit prompted, each verified against this code: nothing gates publishing on being an application — `isPublishingEnabled()` reads only `serverVersion > 3 && publish_enabled` — so the skill's "there is no client-side send in an application" and the migration note's "the only one that works there" were false rather than merely imprecise. The server reads only `requests[0]` of a batch, recorded in `pull.proto` where the schema says `repeated`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F22e2ft66y7nuBJjzdThBr
IgorShevchik
force-pushed
the
claude/pull-close-reasons
branch
from
September 23, 2026 10:09
876e0b8 to
08b94b7
Compare
`check-api-reference-index.mjs` gates every public value export on being listed, and it is a separate CI job from `docs-lint` — so a local `docs-lint` run stays green while CI fails, which is what happened here. `isFrameRefusalCloseCode` added to the Realtime table, and `CloseReasons` moved out of "Not yet covered by a guide" now that its row links to one. That guide had to exist for the link to resolve, so `96.error-codes.md` gains a "Pull close codes" section. It is deliberately a section of its own rather than rows in the error-codes table: these never reach a caller as an error, they arrive on the socket's `close` event. The section carries the split the enum makes — frame-level versus connection-level, and which codes the client sends rather than the server — and the caution that matters most, that a close code is evidence when it arrives and proves nothing by its absence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F22e2ft66y7nuBJjzdThBr
…split Second review round on the same change. Five corrections, two of them to text a reader would have acted on. The enum's own docblock said "everything above is a code the CLIENT sends", with `WRONG_CHANNEL_ID` sitting directly above it — and the server sends that one, which `PullClient.onWebSocketDisconnect` reads off the `close` event. Two paragraphs further down the same block classified it correctly. Fixed, and `isFrameRefusalCloseCode` now tests membership of a named set rather than a range: the range gave identical answers only because the allocation happens to be contiguous, so a future code added inside or outside it would have needed two edits that look unrelated. `close-reasons.unit.spec.ts` pins the boundary — every frame-level code, the three connection-level ones that share the numeric range, the client's own codes, the unallocated gaps, and that no two names collapse onto one value. There was no test at all for a new public export whose whole content is where its boundary falls. The lab's `warn` text, the one an operator reads, still said a misaddressed frame is "dropped without a word" — the reading this change exists to replace. Such a frame is broadcast, with an empty body; what is missing is anything the page can match as an echo. Corrected there and in the three other places it was phrased as "no echo at all", which is a claim about the wire that the raw-frame tap is elsewhere used to make. The mechanism was labelled proto3 in a proto2 schema, and stated without its own limit: skipping applies to an UNUSED field number. Collide with a declared field of a different wire type and the decoder throws, so `4013` does fire. R3 was described as "does the lite codec behave LIKE the library", with `readSender` as the example — but that divergence is deliberate and documented as such two sections away, so the definition condemned the fix it cited. R3 is about the lite codec being SAFE where the schema is silent, not identical. The differential suite was also undersold as a byte comparison of a dozen inputs; it carries an unknown-field battery, a `oneof` case, a decode-defaults case and a round trip. It is still hand-written samples, which is the actual argument. Remaining: stale pointers to the retired criterion numbering, the audit's behavioural claims now attributed rather than asserted in both the contributing doc and the user-facing guide, the README's check-9 outcome list gaining the `JSSDK_PULL_SEND_REFUSED` fail and acknowledging `pass`, two docblocks that had drifted onto the wrong declaration, and `MAX_REASON_CHARS` described as bytes when it cuts UTF-16 units. Ran the four block-typecheck gates this time, which the previous round did not: docs 212 blocks, skills 93, jsdoc 46, contributing 7, all clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F22e2ft66y7nuBJjzdThBr
The residual items from the review, none of which changes behaviour except the eviction rule. `socketClosures` trimmed the oldest entry when it hit the cap, and checks 10 and 11 run after check 9 — so a reconnect storm in that tail could evict the one entry `encodeWindow` exists to line up against, leaving an empty list the report tells the reader "means nothing either way". A false negative on the most informative field in the file. Frame refusals are now never evicted; they are rare, so keeping all of them costs nothing. `now()` existed and two of the new timestamps open-coded `Date.now() - startedAt` instead of calling it, which is one more chance for the bases to drift apart in a report whose whole point is lining two of them up. The trap section was headed "The two traps" and then described a third in prose, which left the count incoherent once the sentence that reconciled it was removed. Now three, numbered, each marked encode- or decode-side — and the two places that cited "two of the three traps are on the encode side" are corrected, since by the document's own description only trap 3 is. Last unattributed "an application's Pull client is receive-only" removed from the `JSSDK_PULL_PUBLIC_IDS_UNAVAILABLE` description, which is the one place the claim reached a user with no qualification around it; the description now names the actual cause, that the method is outside the application REST surface. Two more occurrences gained "documented as", matching how the rest of the repo attributes it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F22e2ft66y7nuBJjzdThBr
…dy saw Third review round. One real defect, and three cases of a claim stated more strongly than it holds. `rest.slice(-keep)` with `keep === 0` returns a copy of the WHOLE array — `-0 === 0`, and `slice(0)` copies. So the closure trim stopped trimming the moment frame refusals alone reached the cap, in precisely the scenario it was added for: a portal refusing every publish, a long session, a report that grows without bound. `rest.slice(rest.length - keep)` with `keep` clamped to `rest.length`, verified on the three boundary shapes. `close-reasons.unit.spec.ts` opened by claiming it "exists to fail when someone adds a code and updates only one of the two places". It did not: its three lists are hand-written and nothing tied them to the enum, so a member added and left unclassified passed every case. Now it enumerates `CloseReasons` and asserts each value appears in exactly one list. Checked by mutation — removing `TOO_MANY_CONNECTIONS` from its list fails with that name in the message. The audit hedge in `CloseReasons` was a dangling docblock: a blank line separated it from the first member, so it bound to no declaration and reached no IDE tooltip. What a consumer hovered was twelve unhedged assertions of server behaviour — exact thresholds, exact mechanisms — from a report the same block calls unverifiable. Merged onto the declaration, and the four bare server facts now say "per the audit". The block also claimed flatly that a refused frame always comes back as a close; the same audit describes a refusal path that answers with nothing, which is now stated. `96.error-codes.md` kept the strong half of "a close code's absence proves nothing" and dropped the caveat, contradicting the table fifteen lines above it: a message with no receivers, a private channel or a bad signature IS well-formed and DOES produce a close. Corrected — it is the encoding, not the message, that a silent run fails to exonerate. The wire-type caveat in `pull-protobuf.md` had it backwards. A generated decoder switches on the field number alone (`switch (tag >>> 3)`) and reads by the declared type, so "it throws" does not follow from a collision. For this schema the reliable throw is a varint written where a length-delimited field is declared; the document's own example — a string at `expiry`'s number — is the least deterministic case in it; and four of five fields being length-delimited means most misplacements parse cleanly. The silent bucket is the larger one, which is the actual argument. R3 had a method and no stopping rule, so as written it could never be declared done. It now names the corpus and the pass condition. R4 is new and was owned by none of the other three: `Sender.id` diverges deliberately, R2 waives it and R3 is satisfied by it, but a caller sees a behaviour change and after the deletion the lite behaviour simply is the behaviour. And the order of operations is now stated — every one of R2, R3 and R4 is measured against `model.js`, so deletion does not reduce them, it removes the ability to measure them. Also: the lab's check-9 text said "two informative outcomes" and then named a third in the same string. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F22e2ft66y7nuBJjzdThBr
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.
What
An audit of the push-server sources on an on-prem stand answered the open questions from #559. This lands the parts that are code, and — after a five-reviewer pass — states carefully what the audit does and does not establish.
1. The refusal codes
A publish the push server refuses is not answered and does not error. It closes the WebSocket with a code and a reason string.
CloseReasonsknew one of them,4010.Eleven added, separated by meaning rather than by numeric range:
WRONG_REQUEST_DATArequestsemptyREQUEST_COMMAND_NOT_ALLOWEDWRONG_REQUEST_COMMANDTOO_MANY_MESSAGESNO_CHANNELS_FOUNDreceiversemptyTOO_MANY_CHANNELSINVALID_CHANNEL_IDPRIVATE_CHANNEL_NOT_ALLOWEDisPrivatewas trueINVALID_CHANNEL_SIGNATURENO_PUBLIC_CHANNEL_IDTOO_MANY_CONNECTIONS4010,4012and4029share the range but are connection-level, so a caller testing "is it 401x, my publish failed" would misclassify all three. HenceisFrameRefusalCloseCode(). The docblock also marks direction: everything below 4010 is a code the client passes todisconnect(), and none of the new ones may be used that way.2. The lab records closures — and says what they are worth
A
closelistener beside the frame tap. The export gainssocketClosures(each entry withframeRefusal) andencodeWindow— check 9's start and end, becausechecks[].msis a duration, so the report previously invited a correlation it gave the reader no way to make. Closures reset per run and are capped like the other collectors.The absence of a close code proves nothing, and every surrounding text now says so. Only a structurally unparseable frame trips
4013; a scalar written at the wrong field number parses cleanly, is broadcast with an empty body, and yields no close code and no echo — exactly what awarnlooks like. A closure inside the window is correlation, not cause, on a connection shared with subscriptions and heartbeats; and the tap runs off a 2-second poll, so a socket opened and closed between ticks is never seen.3. The schema: examined, not closed
The audit reports field numbers matching ours on the send side, verified here against
protobuf/model.js(Receiver.encode, tags 10/16/26) andprotobuf-lite/messages.ts(writeReceiver):An earlier revision of this PR called that "R1 closed". It should not have. The doc now carries the provenance with the claim:
Receiveris quoted. The three nesting field numbers the encoder hardcodes come fromrequest.proto, unreproduced, and nothing was said aboutOutgoingMessage,Sender,ResponseBatchor theoneof— which is where the doc's own worked example (fixed32vssfixed32) lives.4. The exit criterion, split by what a deletion moves
model.js? Open. Closable by fuzzing, no portal..protoaudit and a byte-for-byte differential on well-formed input.readSenderis the worked example: returning{}instead of proto3 defaults threw insidedecodeIdand took the whole batch with it.Also recorded: the echo question is settleable without trusting any server-side claim — a second tab subscribed to
SubscriptionType.Clientreceiving the first tab's publish distinguishes "server excludes the sender" from "the frame was refused".5. Corrections the audit prompted, verified against this code
isPublishingEnabled()isserverVersion > 3 && publish_enabled— nothing gates publishing on being an application. Soskills/b24jssdk-helpers/SKILL.md's "The Pull client only RECEIVES / there is no client-side send in an application" and the migration note's "the only one that works there" were false, not merely imprecise. Both corrected; the guidance (publish from the back end) stands, only the stated reason changed.sendMessageToChannels()never makes the lookup at all, which the skill now says.pull.protorecords that the server reads onlyrequests[0]of a batch, where the schema saysrepeated.96.error-codes.md'sJSSDK_PULL_SEND_REFUSEDrow now points at the close codes — deliberately not adding them to that table, since they never reach a caller as an error.Review
Five reviewers. Findings acted on: the "R1 closed" overreach and its provenance (reviewer 4), the enum's mixed directions and the false "401x = publish refused" generalisation (2, 4),
socketClosuresnot resetting and uncapped (1, 5), no way to correlate with check 9 (1), the server-authoredreasonentering the export unbounded and unflagged (5), the self-contradiction left atpull-protobuf.md:265and the stale pointers at :104/:218/:273 (3), the six repo locations still asserting an application cannot publish (3), and the README's report-shape list (3). One finding declined with reason: the eleven codes do not belong in96.error-codes.md's table.Reviewer 3 also caught that the previous commit body began a line with
Source:, which release-please would have minted into a spurious changelog entry. Commit reworded and force-pushed.Verification
typecheck(jssdk + nuxt playground),eslint,pnpm lint:md,docs-lint --strict, and the Pull suites (12 files, 91 tests) pass locally.Follow-up
#559 carries the remaining work: fuzz differentials for R2 and R3, after which
protobuf/can go.🤖 Generated with Claude Code
https://claude.ai/code/session_01F22e2ft66y7nuBJjzdThBr