Skip to content

fix(cloud): actually check the dev-session protocol version the server answers - #91

Merged
AetherAI3 merged 1 commit into
mainfrom
fix/dev-protocol-version-check
Aug 20, 2026
Merged

fix(cloud): actually check the dev-session protocol version the server answers#91
AetherAI3 merged 1 commit into
mainfrom
fix/dev-protocol-version-check

Conversation

@AetherAI3

Copy link
Copy Markdown
Owner

The defect

CloudBrain declares a dev-session protocol version, sends it, types the answer, and never looks at it.

On POST /agent/dev/sessions the client sends protocol_version: DEV_PROTOCOL_VERSION (brain_cloud.ts:39). The response type declares protocol_version: number (brain_cloud.ts:44-50). The code then did this (:80, :87-89):

this.sessionId = created.session_id;
queue.push({ type: "stage", name: "execute", face: "⟨◉⟩" });
await this.devPump(queue);

Two holes, both reachable from a 200:

  1. 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:137 maps unknown-shaped frames through ?? and Number() — 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 ships dev_session_protocol_versions: [1] (src/generated/agent_capabilities.ts:11), and capabilities.ts already refuses an incompatible contract major version — but dev_session_protocol_versions was referenced in exactly one file and consumed by zero.

  2. session_id was never validated. A 200 without one produced undefined flowing 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.ts has 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 returns null for a usable session, or the message to fail the run with:

  • session_id must be a non-empty string. Otherwise: refuse, rather than build a request path out of an absent id.
  • protocol_version must 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's dev_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 to brain_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/catch that implements the isLegacyServer 404/403 fallback, and it throws rather than calling legacyPump. 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 off origin/main @ c165be0. Windows 11, Git Bash. No git stash was used at any point in producing this branch.

$ cd ~/agent-w4dev-wt && npm run typecheck
> tsc -p tsconfig.json --noEmit
(clean)

Full suite:

$ cd ~/agent-w4dev-wt && export TEMP='C:\w4tmp' TMP='C:\w4tmp' && time npm test
tmpdir= C:\w4tmp
...
ℹ tests 1122
ℹ suites 0
ℹ pass 1121
ℹ fail 0
ℹ cancelled 0
ℹ skipped 1
ℹ todo 0
ℹ duration_ms 128468.0626

real    2m54.546s

A note on that TEMP. This branch is cut from main @ 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:\w4tmp is 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 a main that includes #89, the default TEMP works and no redirection is needed.

The dev-session file on its own, naming each new test:

$ node --test --test-isolation=none dist/test/brain_cloud_dev.test.js
✔ dev session: create carries effort + capabilities; tool_call surfaces and sendToolResult POSTs upstream (83.1157ms)
✔ dev session: a replayed frame (seq <= high-water mark) is skipped — a mutating tool_call never fires twice (2.1593ms)
✔ dev session: a dropped stream reconnects from last_seq and finishes (1012.8612ms)
✔ dev session: a server error frame ends the run done ok:false (never fabricated success) (2.2697ms)
✔ dev session: done ok:false from the server stays ok:false (1.71ms)
✔ dev session: control() posts pause/steer to the control route (34.1282ms)
✔ dev session: close() tears the server session down (DELETE) (23.2455ms)
✔ dev session: a transient tool-result POST failure is retried (idempotent upstream) (1201.9599ms)
✔ legacy fallback: a 404 on session create degrades to the one-way chat stream (23.2312ms)
✔ a non-404 create failure surfaces as an error, not a silent legacy downgrade (1.1963ms)
✔ devProtocolVersions reads the accepted set from the capability contract (0.312ms)
✔ checkDevSession names both versions so the message is actionable (0.2778ms)
✔ a create response with no session_id fails the run instead of streaming /undefined/stream (0.6503ms)
✔ an unsupported dev protocol version fails the run and does NOT downgrade to legacy (0.6943ms)
✔ a build whose contract advertises v2 attaches to a v2 session (0.8624ms)
ℹ tests 15
ℹ pass 15
ℹ fail 0

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; null means "omit the field entirely") so a malformed or mismatched 200 can 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 blank session_id is named in the message.
  • a create response with no session_id fails the run instead of streaming /undefined/stream — asserts an error event and that no request URL ever contains undefined.
  • an unsupported dev protocol version fails the run and does NOT downgrade to legacy — asserts the error names both versions, that /agent/chat/stream was 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 clean done.

Blast radius

  • src/core/brain_cloud.ts and test/brain_cloud_dev.test.ts only. No generated file is edited — src/generated/agent_capabilities.ts is read (through capabilities.ts), never written.
  • CloudBrain's constructor gains an optional parameter; both existing call sites compile and behave unchanged.
  • New import edge: brain_cloud.tscapabilities.ts. capabilities.ts imports only the generated contract and the ApiClient type, so no cycle is introduced.
  • Against a server that answers as today (session_id present, 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

  • Strictness on a missing protocol_version. A 200 that 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.
  • The session stream frame carries its own protocol_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.
  • The client still sends only a single 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.

…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>
@AetherAI3
AetherAI3 merged commit ed094dc into main Aug 20, 2026
5 checks passed
@AetherAI3
AetherAI3 deleted the fix/dev-protocol-version-check branch August 20, 2026 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant