fix(cloud): actually check the dev-session protocol version the server answers - #91
Merged
Merged
Conversation
…r answers CloudBrain sent `protocol_version` on POST /agent/dev/sessions, declared `protocol_version: number` on the response type, and never read it. It also never validated `session_id`, so a 200 without one produced requests against `/agent/dev/sessions/undefined/stream`. The negotiation data already shipped: the generated capability contract carries `dev_session_protocol_versions`, and capabilities.ts already refuses an incompatible contract major version — but that field was referenced in one file and consumed by zero. So a server shipping dev-protocol v2 with any changed frame shape would have been attached to silently, and the decoder's tolerant `??` / `Number()` mapping in stream.ts turns fields this build can no longer find into zeros rather than errors. checkDevSession() now runs immediately after create, before any stream byte: session_id must be a non-empty string, and protocol_version must be in the set this build speaks. On mismatch the run fails with a message naming both versions and telling the user to upgrade. devProtocolVersions() sources that set from the resolved capability contract, falling back to the single version the client declares on the wire. The check sits outside the isLegacyServer 404/403 downgrade and throws rather than calling legacyPump: a version mismatch is not "this server has no dev route", and downgrading would trade a loud, fixable incompatibility for a silent loss of the local tool round-trip. CloudBrain's new capabilities argument is optional, so both existing call sites are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
The defect
CloudBraindeclares a dev-session protocol version, sends it, types the answer, and never looks at it.On
POST /agent/dev/sessionsthe client sendsprotocol_version: DEV_PROTOCOL_VERSION(brain_cloud.ts:39). The response type declaresprotocol_version: number(brain_cloud.ts:44-50). The code then did this (:80,:87-89):Two holes, both reachable from a
200:The version answer is discarded. A handshake where one side declares and the other side never reads the reply is not a handshake. When AETHER-CLOUD ships dev-protocol v2 with any changed frame shape, this client attaches to it and starts decoding. The decoder is deliberately tolerant —
stream.ts:137maps unknown-shaped frames through??andNumber()— so fields this build can no longer find become zeros and empty strings rather than errors. A mis-decoded session that reports plausible-looking progress is precisely the failure a version handshake exists to prevent, and the negotiation was half-built: the generated capability contract already shipsdev_session_protocol_versions: [1](src/generated/agent_capabilities.ts:11), andcapabilities.tsalready refuses an incompatible contract major version — butdev_session_protocol_versionswas referenced in exactly one file and consumed by zero.session_idwas never validated. A200without one producedundefinedflowing straight into the path builders, so the client issued requests against/agent/dev/sessions/undefined/stream,/agent/dev/sessions/undefined/tool-results, and/agent/dev/sessions/undefined/control— and whatever the server made of those became the user's error message.Why CI never caught it
There was nothing to catch. The server fake in
test/brain_cloud_dev.test.tshas always returned{ session_id: "devs_abc", protocol_version: 1, … }— a well-formed, matching answer. Every existing test therefore exercises the happy path, and the happy path is identical whether or not anything reads the version. No test constructed a response the client should refuse, because refusing was not a behaviour the client had. The gap was in the space of inputs nobody generated, not in the assertions.What changed
checkDevSession(created, speaks)runs immediately after create and before a single byte of the stream is read. It returnsnullfor a usable session, or the message to fail the run with:session_idmust be a non-empty string. Otherwise: refuse, rather than build a request path out of an absent id.protocol_versionmust be a finite number present in the set this build speaks. Otherwise: refuse, with a message that names both versions and tells the user to upgrade — e.g.cloud dev session speaks protocol v2 but this build speaks v1 — upgrade the agent (npm i -g aether-agents@latest).devProtocolVersions(contract?)sources that accepted set from the resolved capability contract'sdev_session_protocol_versions, falling back to[DEV_PROTOCOL_VERSION]when no contract is supplied or the field is missing or unusable. This is the piece that makes the contract field load-bearing for the first time: a client shipped with a contract that names[1, 2]negotiates upward without another edit tobrain_cloud.ts.CloudBrain's constructor takes an optional second argument,capabilities?: ResolvedCapabilities. It is optional so that the two existing call sites (src/commands/code.ts,src/core/smoke.ts) are untouched — both are outside this lane. Without it the packaged contract snapshot supplies the accepted set; a caller that has already resolved the server contract should pass it, so that a server legitimately advertising a newer dev protocol is honored rather than refused on stale packaged data.Decision I was asked to make explicitly: this is not a legacy downgrade
The check sits deliberately outside the
try/catchthat implements theisLegacyServer404/403 fallback, and it throws rather than callinglegacyPump. A version mismatch is not "this server has no dev route". Folding it into the downgrade path would convert a loud, fixable incompatibility into a silent loss of the local tool round-trip: the run would appear to work, the code would stay local but the tools would run server-side, and the user would have no way to know why the agent stopped executing on their machine. A refusal that names both versions is the correct outcome; the one-way chat stream remains reserved for servers that genuinely lack the route.Test evidence
Environment: worktree
~/agent-w4dev-wt, a clean worktree offorigin/main@ c165be0. Windows 11, Git Bash. Nogit stashwas used at any point in producing this branch.Full suite:
A note on that
TEMP. This branch is cut frommain@ c165be0, which predates #89, so it does not yet contain the fix that lets the suite run under this machine's default temp directory (which sits inside a version-controlled home directory).C:\w4tmpis the pre-#89 workaround, and it is only about where scratch directories live — nothing in this change reads the filesystem. Once this branch merges with amainthat includes #89, the defaultTEMPworks and no redirection is needed.The dev-session file on its own, naming each new test:
The two pre-existing tests that matter most here — the 404 legacy downgrade and the non-404 surfacing — still pass unchanged, which is what shows the new refusal did not colonise the fallback path.
Five tests added to
test/brain_cloud_dev.test.ts. The existing server fake gained two optional knobs (sessionId,protocolVersion;nullmeans "omit the field entirely") so a malformed or mismatched200can be constructed at all — previously it could not:devProtocolVersions reads the accepted set from the capability contract— reads[1, 2]from a supplied contract, falls back to[1]with no contract, and never yields an empty accept-set from a missing or non-numeric list.checkDevSession names both versions so the message is actionable— accepts a matching session; a mismatch names v2, v1, and "upgrade"; a missing or blanksession_idis named in the message.a create response with no session_id fails the run instead of streaming /undefined/stream— asserts anerrorevent and that no request URL ever containsundefined.an unsupported dev protocol version fails the run and does NOT downgrade to legacy— asserts the error names both versions, that/agent/chat/streamwas never called, and that the dev stream was never attached.a build whose contract advertises v2 attaches to a v2 session— the negotiation actually negotiates: given a contract naming[1, 2], a v2 session runs to a cleandone.Blast radius
src/core/brain_cloud.tsandtest/brain_cloud_dev.test.tsonly. No generated file is edited —src/generated/agent_capabilities.tsis read (throughcapabilities.ts), never written.CloudBrain's constructor gains an optional parameter; both existing call sites compile and behave unchanged.brain_cloud.ts→capabilities.ts.capabilities.tsimports only the generated contract and theApiClienttype, so no cycle is introduced.session_idpresent,protocol_version: 1), behaviour is byte-for-byte unchanged. The only newly reachable outcome is a refusal on responses that previously produced a mis-decoded session or a request against/undefined/.Found but not fixed
protocol_version. A200that omits the field entirely is refused, not defaulted to v1. That is the stricter reading, and I think the right one — an unversioned session is exactly the case where the client cannot know what it is decoding. If a deployed AETHER-CLOUD build is known to omit the field on the create response, this needs a coordinated flip and should be caught before this merges.sessionstream frame carries its ownprotocol_version(see the fake's first frame) and is still not checked against the create response. A server that answered v1 on create and then streamed v2 frames would not be caught. That is a server-side inconsistency rather than a version-skew case, and closing it means deciding what the stream frame is authoritative for — a separate change.protocol_version, not the set it can speak. Now that the accepted set is computed, sending the full list on create would let the server pick the best mutually supported version instead of failing the handshake. That is a wire-contract change and needs the AETHER-CLOUD side first.