From 11d8673112df388b5d010eb95d2ea67b326bf38e Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sat, 29 Aug 2026 13:42:50 -0700 Subject: [PATCH] test(cross-version-wire): derive skew expectations from the baseline under test (#17178) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(cross-version-wire): derive skew expectations from the baseline under test The cross-version wire job pairs current code against whichever release tag is newest, so a hand-written "the old side does not have X" assertion expires by itself: v1.4.192 was the first tag containing the SnapshotStart `terminalOwner` field, and cutting it turned the new-client/old-server pairing red on unrelated pull requests with no code change anywhere. Read what each build publishes from that build. Each host is now paired against a client of its own version to produce a reference, and the skewed pairings are compared against that reference, so the expectation is whatever the release actually shipped. The same class of assertion in the agent-session suite — "the old build advertises no structured capability and registers no structured method" — becomes "each build's advertisement agrees with what it registers", and the "client too old to know this capability" is derived by removing the capability from the baseline's own list. The guard is unchanged in strength: a field the old host still publishes may not be dropped, skew may not change what a host puts on the wire, and a new pairing asserts the oracle still stalls when a peer cannot decode an opcode the other side sends. * test(cross-version-wire): exercise release structured methods * test(cross-version-wire): load the registered method manifest * test(cross-version-wire): assert execution, not registration, on both host gates The release-shaped checkout gate accepted any reply that was not method_not_found, so a registered-but-throwing handler passed it. The capability gate asserted a shared host spy had been called at all, so the second method mapped to that spy could stop reaching the host unnoticed. * test(cross-version): make the release-shaped skew cover the whole agent-session manifest The release-shaped checkout is the only place the "registered means usable" claim is executable today — the baseline release registers none of these methods — and it was exercising one of sixteen. A handler registered and returning an execution error passed the suite. - Declare each method's result in the manifest, so "answered" is the contract rather than "did not say method_not_found". - Give each build a seam to install a host into its own module slot; a release checkout has its own copy, so the working tree's host was never this dispatcher's, and every host-backed method answered structured_agent_session_unsupported — the capability gate's own words. - Run one execution contract over both skews instead of two divergent loops. - Pair the AI Vault never-called spy with a positive control; renaming the runtime method it watches left it green. --------- Co-authored-by: Brennan Benson --- docs/reference/remote-wire-compatibility.md | 43 ++- ...ss-version-agent-session-wire.unit.test.ts | 306 ++++++++++++++---- .../cross-version-terminal-wire.unit.test.ts | 113 +++++-- .../published-field-shape.ts | 38 +++ .../published-field-shape.unit.test.ts | 50 +++ .../terminal-skew-journey.ts | 57 +++- .../versioned-agent-session-wire.ts | 92 +++--- .../versioned-terminal-wire.ts | 34 ++ 8 files changed, 577 insertions(+), 156 deletions(-) create mode 100644 tests/e2e/cross-version-wire/published-field-shape.ts create mode 100644 tests/e2e/cross-version-wire/published-field-shape.unit.test.ts diff --git a/docs/reference/remote-wire-compatibility.md b/docs/reference/remote-wire-compatibility.md index 423149950ce..1fceb67d5d5 100644 --- a/docs/reference/remote-wire-compatibility.md +++ b/docs/reference/remote-wire-compatibility.md @@ -95,16 +95,51 @@ negotiated capabilities differ from the contract. Adding an optional field keeps green (Rule 1); making a client depend on that field turns the new-client/old-host pairing red. +### Never write down what the old side has + +The baseline is whichever release tag is newest, so it moves on every cut. An +expectation of the form "the old side does not have X" — a `not.toHaveProperty`, a +`not.toContain`, a hard-coded field list — stops being true the first time a release +ships X. The suite then reddens on whatever pull request is in flight, with no code +change anywhere, and the job trains people to ignore it. That is worse than no test, +because a rolling baseline eventually contains every additive field the wire has, and +adding one is the sanctioned way to evolve it. + +Derive the expectation from the baseline that was actually checked out: + +- for a published frame, pair each build against a client of its own version and + compare the skewed pairing against that same-version reference, so the expectation + is whatever that build publishes today (`publishedFieldNames` in + `tests/e2e/cross-version-wire/published-field-shape.ts`); +- for a negotiated surface, read the old build's advertised capabilities and + registered method names from its checkout, and assert they agree with each other + rather than asserting the old build lacks them; +- for a "client too old to know X", derive that client's advertised list by removing + X from the baseline's own list, so the gate stays exercised after X ships. + +Name the direction in the assertion. `new client against old server` and `old client +against new server` fail for different reasons, and the host is the only side that +authors a published frame — the terminal `terminalOwner` false positive on 2026-08-29 +was misread as a new client sending an unknown field when the old server was +publishing it. Two things are still safe to state literally: the current build's own +contract, and an invariant that holds for every version. + +Pinning a legacy ref is the fallback when a contract genuinely needs a release from +before a feature shipped, as `cross-version-browser-placement.unit.test.ts` does with +`LEGACY_BROWSER_PLACEMENT_RELEASE_REF`. It does not rot on a cut, but it is +hand-maintained, so prefer deriving. + `tests/e2e/cross-version-wire/cross-version-agent-session-wire.unit.test.ts` pairs the same two builds over the structured `agentSession.*` surface. Because a released build cannot name a capability string its own source never contains, the old side's advertised list and registered method names are read from the extracted checkout rather than hand-written. It covers the three skews that surface can fail on: -- an old client — advertising only what the baseline build defines — is told the whole - surface does not exist and reaches no host method; -- a new client against the old dispatcher gets `method_not_found` on every method, and - can see the absence during negotiation instead of by calling; +- an old client — advertising the baseline's list minus this capability — is told the + whole surface does not exist and reaches no host method; +- a new client against the old dispatcher always gets an answer rather than silence, + and `method_not_found` for every method that release does not register, so the + absence is visible during negotiation instead of by calling; - a cursor survives a host restart: the client's fence is refused as stale with the live one attached, and resuming from the held cursor replays only what it missed. diff --git a/tests/e2e/cross-version-wire/cross-version-agent-session-wire.unit.test.ts b/tests/e2e/cross-version-wire/cross-version-agent-session-wire.unit.test.ts index 093547102c6..0ce15ee6563 100644 --- a/tests/e2e/cross-version-wire/cross-version-agent-session-wire.unit.test.ts +++ b/tests/e2e/cross-version-wire/cross-version-agent-session-wire.unit.test.ts @@ -36,27 +36,69 @@ const WORKSPACE = 'workspace-1' const THREAD = '019fd532-7c11-7a90-b6de-4e1a2c3d5f60' const NOW = 1_800_000_000_000 -/** Every method the structured surface publishes, paired with the host method it - * must reach — a gate that hides one method and leaks another is the bug. */ -const STRUCTURED_CALLS: { method: string; hostMethod: string | null }[] = [ - { method: 'agentSession.createSupport', hostMethod: null }, - { method: 'agentSession.create', hostMethod: 'attach' }, - { method: 'agentSession.ensure', hostMethod: 'attach' }, - { method: 'agentSession.send', hostMethod: 'send' }, - { method: 'agentSession.cancel', hostMethod: 'cancel' }, - { method: 'agentSession.close', hostMethod: 'close' }, - { method: 'agentSession.respondToApproval', hostMethod: 'respondToPrompt' }, - { method: 'agentSession.respondToQuestion', hostMethod: 'respondToPrompt' }, - { method: 'agentSession.setOption', hostMethod: 'setOption' }, - { method: 'agentSession.handoffStatus', hostMethod: 'handoffStatus' }, - { method: 'agentSession.options', hostMethod: 'readOptions' }, - { method: 'agentSession.hold', hostMethod: 'hold' }, - { method: 'agentSession.release', hostMethod: 'release' }, - { method: 'agentSession.history', hostMethod: 'history' }, +/** Every method the structured surface publishes: the host method it must reach, + * and the result it must hand back. A gate that hides one method and leaks + * another is the bug; so is a method that is registered and answers with an + * error, which is why `result` is declared per method rather than inferred from + * "did not say method_not_found". `result` is omitted only where the method + * legitimately answers with no reply at all. */ +const STRUCTURED_CALLS: { + method: string + hostMethod: string | null + result?: Record +}[] = [ + { method: 'agentSession.createSupport', hostMethod: null, result: { supported: true } }, + { + method: 'agentSession.create', + hostMethod: 'attach', + result: { ok: true, replayed: false, value: { sessionId: SESSION } } + }, + { + method: 'agentSession.ensure', + hostMethod: 'attach', + result: { ok: true, replayed: false, value: { sessionId: SESSION } } + }, + { method: 'agentSession.send', hostMethod: 'send', result: { ok: true, replayed: false } }, + { method: 'agentSession.cancel', hostMethod: 'cancel', result: { ok: true, replayed: false } }, + { method: 'agentSession.close', hostMethod: 'close', result: { ok: true } }, + { + method: 'agentSession.respondToApproval', + hostMethod: 'respondToPrompt', + result: { ok: true, replayed: false } + }, + { + method: 'agentSession.respondToQuestion', + hostMethod: 'respondToPrompt', + result: { ok: true, replayed: false } + }, + { + method: 'agentSession.setOption', + hostMethod: 'setOption', + result: { ok: true, replayed: false } + }, + { + method: 'agentSession.handoffStatus', + hostMethod: 'handoffStatus', + result: { owner: 'native' } + }, + { + method: 'agentSession.options', + hostMethod: 'readOptions', + result: { current: { model: 'gpt-live' } } + }, + { method: 'agentSession.hold', hostMethod: 'hold', result: { held: true } }, + { method: 'agentSession.release', hostMethod: 'release', result: { released: true } }, + { + method: 'agentSession.history', + hostMethod: 'history', + result: { ok: true, page: { items: [] } } + }, + // A subscription that opens with nothing to say answers with no reply at all, + // so reaching the host is the only signal that the gate opened. { method: 'agentSession.subscribe', hostMethod: 'subscribe' }, // Teardown runs through the runtime's subscription registry rather than the // host, so its reply is the only signal that the gate opened. - { method: 'agentSession.unsubscribe', hostMethod: null } + { method: 'agentSession.unsubscribe', hostMethod: null, result: { unsubscribed: true } } ] let baselineRef: string @@ -199,6 +241,24 @@ function runtimeStub(): unknown { } } +/** + * What a client too old to know the structured surface advertises: the baseline's + * own list, minus the capability. Derived rather than assumed to be the baseline's + * list as-is — the baseline is the newest release tag, so the day a release ships + * this capability the list would contain it and the gate below would stop being + * exercised at all, on a pull request that changed nothing. + */ +function legacyClientCapabilities(): string[] { + return baseline.capabilities.filter( + (capability) => capability !== STRUCTURED_AGENT_SESSION_RUNTIME_CAPABILITY + ) +} + +/** The structured methods the baseline release actually registers, read from it. */ +function baselineStructuredMethods(): string[] { + return baseline.methodNames.filter((name) => name.startsWith('agentSession.')) +} + /** Every reply one call produced. Streaming methods answer more than once, and a * refusal has to arrive as a reply rather than as silence. */ async function callBuild( @@ -219,6 +279,75 @@ async function callBuild( return replies } +/** The host every skew installs to drive the surface: enough of the real host's + * shape for each handler to run, and a spy per method so "which call reached the + * host" is answerable per call rather than per suite. */ +function structuredHostStub(): Record> { + return { + attach: vi.fn(async () => ({ ok: true, replayed: false, value: { sessionId: SESSION } })), + send: vi.fn(async () => ({ ok: true, replayed: false })), + cancel: vi.fn(async () => ({ ok: true, replayed: false })), + close: vi.fn(async () => undefined), + hold: vi.fn(async () => undefined), + release: vi.fn(() => undefined), + respondToPrompt: vi.fn(async () => ({ ok: true, replayed: false })), + setOption: vi.fn(async () => ({ ok: true, replayed: false })), + requestHandoff: vi.fn(async () => ({ status: { owner: 'native' } })), + handoffStatus: vi.fn(async () => ({ owner: 'native' })), + readOptions: vi.fn(async () => ({ models: [], current: { model: 'gpt-live' } })), + history: vi.fn(() => ({ ok: true, page: { items: [] } })), + subscribe: vi.fn(() => () => undefined), + unsubscribe: vi.fn() + } +} + +/** + * The one thing this suite exists to guarantee, written once and applied per + * build: every method the manifest declares is not merely registered but reaches + * its host method on this call, answers, and answers with its declared result. + * + * Written as a helper rather than inline because a build passing it is the claim, + * and each skew that registers the surface owes the same claim — a check that + * covers one method leaves the rest registered-but-unusable behind a green suite. + */ +async function expectDeclaredSurfaceExecutes( + build: AgentSessionWireBuild, + hostCalls: Record>, + clientCapabilities: readonly string[] +): Promise { + for (const { method, hostMethod, result } of STRUCTURED_CALLS) { + // Two methods share one host method, so "has been called" would already be + // true from the earlier one: only this call's own delta pins the pairing. + const before = hostMethod ? hostCalls[hostMethod].mock.calls.length : 0 + const replies = await callBuild(build, method, paramsFor(method), { + clientKind: 'runtime', + clientCapabilities + }) + if (hostMethod) { + expect( + hostCalls[hostMethod].mock.calls.length - before, + `${build.label}: ${method} did not reach the host` + ).toBe(1) + } + for (const reply of replies) { + expect( + reply, + `${build.label}: ${method} was refused: ${JSON.stringify(reply)}` + ).toMatchObject({ ok: true }) + } + if (result) { + // The declared answer, not merely a non-refusal: a handler that is + // registered and returns an execution error, or hands back someone else's + // envelope, fails here rather than passing as "reached the host". + expect(replies, `${build.label}: ${method} must answer exactly once`).toHaveLength(1) + expect(replies[0], `${build.label}: ${method} answered off-contract`).toMatchObject({ + ok: true, + result + }) + } + } +} + describe('cross-version structured agent sessions', () => { it( 'skews current code against a real published release', @@ -239,22 +368,7 @@ describe('cross-version structured agent sessions', () => { beforeEach(() => { operations = 0 - hostCalls = { - attach: vi.fn(async () => ({ ok: true, replayed: false, value: { sessionId: SESSION } })), - send: vi.fn(async () => ({ ok: true, replayed: false })), - cancel: vi.fn(async () => ({ ok: true, replayed: false })), - close: vi.fn(async () => undefined), - hold: vi.fn(async () => undefined), - release: vi.fn(() => undefined), - respondToPrompt: vi.fn(async () => ({ ok: true, replayed: false })), - setOption: vi.fn(async () => ({ ok: true, replayed: false })), - requestHandoff: vi.fn(async () => ({ status: { owner: 'native' } })), - handoffStatus: vi.fn(async () => ({ owner: 'native' })), - readOptions: vi.fn(async () => ({ models: [], current: { model: 'gpt-live' } })), - history: vi.fn(() => ({ ok: true, page: { items: [] } })), - subscribe: vi.fn(() => () => undefined), - unsubscribe: vi.fn() - } + hostCalls = structuredHostStub() setStructuredAgentSessionHost(hostCalls as unknown as StructuredAgentSessionHost) }) @@ -263,12 +377,13 @@ describe('cross-version structured agent sessions', () => { }) it('is told the whole surface does not exist, and reaches no host method', async () => { - // The old build cannot name the capability, so its clients never send it. - expect(baseline.capabilities).not.toContain(STRUCTURED_AGENT_SESSION_RUNTIME_CAPABILITY) + // Anti-vacuous: the old client still advertises a real list, so the refusal + // below is the capability gate answering, not an empty negotiation. + expect(legacyClientCapabilities().length).toBeGreaterThan(0) for (const { method } of STRUCTURED_CALLS) { const replies = await callBuild(current, method, paramsFor(method), { clientKind: 'runtime', - clientCapabilities: baseline.capabilities + clientCapabilities: legacyClientCapabilities() }) expect(replies, `${method} must answer exactly once`).toHaveLength(1) expect(replies[0]).toMatchObject({ @@ -282,59 +397,91 @@ describe('cross-version structured agent sessions', () => { }) it('is served the same calls once it advertises the capability', async () => { - for (const { method, hostMethod } of STRUCTURED_CALLS) { - const replies = await callBuild(current, method, paramsFor(method), { - clientKind: 'runtime', - clientCapabilities: [ - ...baseline.capabilities, - STRUCTURED_AGENT_SESSION_RUNTIME_CAPABILITY - ] - }) - // A subscription that opens with nothing to say answers with no reply at - // all, so reaching the host is the signal that the gate opened. - if (hostMethod) { - expect(hostCalls[hostMethod], `${method} did not reach the host`).toHaveBeenCalled() - } else { - expect(replies[0], `${method} was refused`).toMatchObject({ ok: true }) - } - for (const reply of replies) { - expect(reply, `${method} was refused: ${JSON.stringify(reply)}`).toMatchObject({ - ok: true - }) - } - } + await expectDeclaredSurfaceExecutes(current, hostCalls, [ + ...legacyClientCapabilities(), + STRUCTURED_AGENT_SESSION_RUNTIME_CAPABILITY + ]) }) }) describe('a new client against an old host', () => { - it('finds no structured method registered on the old build', () => { - expect(baseline.methodNames.filter((name) => name.startsWith('agentSession.'))).toEqual([]) + it('registers the whole surface on the new build', () => { + expect(current.capabilities).toContain(STRUCTURED_AGENT_SESSION_RUNTIME_CAPABILITY) expect(current.methodNames.filter((name) => name.startsWith('agentSession.'))).toHaveLength( STRUCTURED_CALLS.length ) }) it('can detect the absence during negotiation instead of by calling', () => { - expect(current.capabilities).toContain(STRUCTURED_AGENT_SESSION_RUNTIME_CAPABILITY) - expect(baseline.capabilities).not.toContain(STRUCTURED_AGENT_SESSION_RUNTIME_CAPABILITY) + // The invariant that survives a release cut: each build's advertised list and + // its registered methods agree. "The old build has neither" is only true + // until a release ships the surface, and pinning it turns this red on the cut + // rather than on a change. + expect(baseline.capabilities.includes(STRUCTURED_AGENT_SESSION_RUNTIME_CAPABILITY)).toBe( + baselineStructuredMethods().length > 0 + ) // Additive surface: bumping the protocol number would strand every paired // device on this release rather than degrade one feature. expect(current.protocolVersion).toBe(baseline.protocolVersion) }) - it('gets a clean method_not_found from the old dispatcher rather than silence', async () => { + it('gets a clean answer from the old dispatcher rather than silence', async () => { + const registered = new Set(baselineStructuredMethods()) for (const { method } of STRUCTURED_CALLS) { const replies = await callBuild(baseline, method, paramsFor(method), { clientKind: 'runtime', clientCapabilities: current.capabilities }) + // Silence is the failure mode a new client cannot recover from, whatever + // the old build knows; the refusal code is only asserted for the methods + // that release genuinely does not have. expect(replies, `${method} must answer exactly once`).toHaveLength(1) - expect(replies[0], `${method} on the old host`).toMatchObject({ - ok: false, - error: { code: 'method_not_found' } - }) + if (!registered.has(method)) { + expect(replies[0], `${method} on the old host`).toMatchObject({ + ok: false, + error: { code: 'method_not_found' } + }) + } else { + expect(replies[0], `${method} is registered on the old host`).not.toMatchObject({ + ok: false, + error: { code: 'method_not_found' } + }) + } } }) + + it( + 'executes every method a release-shaped checkout registers', + async () => { + // The stand-in for the release that ships this surface: the same source, + // read the way a release checkout reads it rather than through the test + // runner's module graph. It is the only place the "registered means + // usable" claim is executable today, because the baseline registers none + // of these methods — so it has to carry the whole manifest, not a sample. + const releasedCurrent = await loadAgentSessionWireBuild('HEAD') + expect(releasedCurrent.capabilities).toContain(STRUCTURED_AGENT_SESSION_RUNTIME_CAPABILITY) + expect( + releasedCurrent.methodNames.filter((name) => name.startsWith('agentSession.')) + ).toHaveLength(STRUCTURED_CALLS.length) + // Each build owns its own host slot, so the one the suite installed in + // current source is not this dispatcher's. Installing here is also the + // anti-vacuous guard: without it every host-backed method answers + // `structured_agent_session_unsupported`, the same words the capability + // gate uses, and the run would read as a refusal rather than a miss. + const hostCalls = structuredHostStub() + await releasedCurrent.installStructuredHost(hostCalls) + try { + await expectDeclaredSurfaceExecutes( + releasedCurrent, + hostCalls, + releasedCurrent.capabilities + ) + } finally { + await releasedCurrent.installStructuredHost(null) + } + }, + SUITE_TIMEOUT_MS + ) }) describe('an old client against a structured-owned AI Vault row', () => { @@ -429,7 +576,7 @@ describe('cross-version structured agent sessions', () => { {}, { clientKind: 'runtime', - clientCapabilities: baseline.capabilities + clientCapabilities: legacyClientCapabilities() }, runtime ) @@ -474,7 +621,7 @@ describe('cross-version structured agent sessions', () => { params, { clientKind: 'runtime', - clientCapabilities: baseline.capabilities + clientCapabilities: legacyClientCapabilities() }, runtime ) @@ -487,7 +634,7 @@ describe('cross-version structured agent sessions', () => { current, 'session.tabs.createTerminal', { worktree: `id:${WORKSPACE}`, command: `codex resume '${THREAD}'` }, - { clientKind: 'runtime', clientCapabilities: baseline.capabilities }, + { clientKind: 'runtime', clientCapabilities: legacyClientCapabilities() }, runtime ) )[0] @@ -498,12 +645,29 @@ describe('cross-version structured agent sessions', () => { current, 'terminal.send', { terminal: 'terminal-1', text: `codex resume '${THREAD}'`, enter: true }, - { clientKind: 'runtime', clientCapabilities: baseline.capabilities }, + { clientKind: 'runtime', clientCapabilities: legacyClientCapabilities() }, runtime ) )[0] ).toMatchObject({ ok: false, error: { code: 'agent_session_conflict' } }) expect(createMobileSessionTerminal).not.toHaveBeenCalled() + + // The positive control for the three refusals above: the same client, the + // same method, a command that is not this thread's resume, and it lands. + // Without it, a stub whose shape drifted from the runtime would satisfy + // "was never called" by never being reachable at all. + expect( + ( + await callBuild( + current, + 'session.tabs.createTerminal', + { worktree: `id:${WORKSPACE}`, command: 'echo unrelated' }, + { clientKind: 'runtime', clientCapabilities: legacyClientCapabilities() }, + runtime + ) + )[0] + ).toMatchObject({ ok: true }) + expect(createMobileSessionTerminal).toHaveBeenCalledTimes(1) }) }) diff --git a/tests/e2e/cross-version-wire/cross-version-terminal-wire.unit.test.ts b/tests/e2e/cross-version-wire/cross-version-terminal-wire.unit.test.ts index 54d303ac871..6b3a7abe5b6 100644 --- a/tests/e2e/cross-version-wire/cross-version-terminal-wire.unit.test.ts +++ b/tests/e2e/cross-version-wire/cross-version-terminal-wire.unit.test.ts @@ -1,6 +1,18 @@ +// Cross-version coverage for the remote terminal stream, paired in both skew +// directions: current working tree against the newest published release. +// +// What each build publishes is read from that build, never written down here. The +// baseline is whichever release tag is newest, so a list of "fields the old side +// does not have yet" stops being true the moment a release ships one of them — the +// suite then reddens on whatever pull request is in flight, with no code change +// anywhere. Every version-dependent expectation below therefore comes from a +// same-version reference pairing of the build that publishes the frame. + import { afterEach, beforeAll, describe, expect, it } from 'vitest' +import { comparePublishedFields, publishedFieldNames } from './published-field-shape' import { resolveBaselineReleaseRef, selectLatestStableReleaseTag } from './release-checkout' import { + CrossVersionJourneyStall, JOURNEY_INPUTS, JOURNEY_STEPS, runTerminalSkewJourney, @@ -8,6 +20,7 @@ import { } from './terminal-skew-journey' import { loadTerminalWireBuild, + withoutOpcodeSupport, WORKING_TREE, type TerminalWireBuild } from './versioned-terminal-wire' @@ -42,11 +55,17 @@ const EXPECTED_JOURNEY_FRAMES = [ let baselineRef: string let current: TerminalWireBuild let baseline: TerminalWireBuild +/** What a current host publishes to a client of its own version. */ +let currentReference: JourneyRecord +/** What the baseline host publishes to a client of its own version. */ +let baselineReference: JourneyRecord beforeAll(async () => { baselineRef = resolveBaselineReleaseRef() current = await loadTerminalWireBuild(WORKING_TREE) baseline = await loadTerminalWireBuild(baselineRef) + currentReference = await runTerminalSkewJourney({ hostBuild: current, clientBuild: current }) + baselineReference = await runTerminalSkewJourney({ hostBuild: baseline, clientBuild: baseline }) }, SUITE_TIMEOUT_MS) afterEach(() => { @@ -114,20 +133,26 @@ describe('cross-version remote terminal wire', () => { SUITE_TIMEOUT_MS ) - it( - 'current client against current server completes the journey', - async () => { - const record = await runTerminalSkewJourney({ hostBuild: current, clientBuild: current }) - expectJourneyActuallyRan(record) - expectWireCompatible(record) - expect(record.snapshotStarts).toEqual([ - expect.objectContaining({ alternateScreen: false, terminalOwner: 'shell' }), - expect.objectContaining({ alternateScreen: false, terminalOwner: 'shell' }), - expect.objectContaining({ alternateScreen: false, terminalOwner: 'shell' }) - ]) - }, - SUITE_TIMEOUT_MS - ) + it('current client against current server completes the journey, and is the reference for a current host', () => { + expectJourneyActuallyRan(currentReference) + expectWireCompatible(currentReference) + // Current code's own contract in both roles, so it is safe to state literally. + expect(currentReference.snapshotStarts).toEqual([ + expect.objectContaining({ alternateScreen: false, terminalOwner: 'shell' }), + expect.objectContaining({ alternateScreen: false, terminalOwner: 'shell' }), + expect.objectContaining({ alternateScreen: false, terminalOwner: 'shell' }) + ]) + }) + + it('old client against old server completes the journey, and is the reference for an old host', () => { + expect(baselineReference.hostRevision).toBe(baseline.revision) + expect(baselineReference.clientRevision).toBe(baseline.revision) + expectJourneyActuallyRan(baselineReference) + expectWireCompatible(baselineReference) + // Anti-vacuous: a reference read from a pairing that published nothing would + // make every comparison against it trivially true. + expect(publishedFieldNames(baselineReference.snapshotStarts).length).toBeGreaterThan(4) + }) it( 'old client against new server completes the journey', @@ -136,11 +161,10 @@ describe('cross-version remote terminal wire', () => { expect(record.clientRevision).toBe(baseline.revision) expectJourneyActuallyRan(record) expectWireCompatible(record) - expect(record.snapshotStarts).toEqual([ - expect.objectContaining({ alternateScreen: false, terminalOwner: 'shell' }), - expect.objectContaining({ alternateScreen: false, terminalOwner: 'shell' }), - expect.objectContaining({ alternateScreen: false, terminalOwner: 'shell' }) - ]) + // Direction: the NEW host publishes here, and the old client only reads. Skew + // must not change what that host puts on the wire, so the expectation is the + // current host's own reference — whatever fields it carries today. + expect(record.snapshotStarts).toEqual(currentReference.snapshotStarts) }, SUITE_TIMEOUT_MS ) @@ -152,10 +176,53 @@ describe('cross-version remote terminal wire', () => { expect(record.hostRevision).toBe(baseline.revision) expectJourneyActuallyRan(record) expectWireCompatible(record) - for (const start of record.snapshotStarts) { - expect(start).not.toHaveProperty('terminalOwner') - expect(start).not.toHaveProperty('alternateScreen') - } + // Direction: the OLD host publishes here, and the new client only reads. Which + // optional fields that release shipped is a property of the release, so it is + // read from the baseline's own pairing rather than named here. + expect(record.snapshotStarts).toEqual(baselineReference.snapshotStarts) + }, + SUITE_TIMEOUT_MS + ) + + it('adds SnapshotStart fields rather than dropping ones the old host still publishes', () => { + const skew = comparePublishedFields({ + older: publishedFieldNames(baselineReference.snapshotStarts), + newer: publishedFieldNames(currentReference.snapshotStarts) + }) + // Rule 1 is additive-only. A field the old host still publishes is one an old + // client may still read, so dropping it breaks that client with no opcode + // change for the decoder check to catch. + expect( + skew.removed, + `current code stopped publishing SnapshotStart fields ${baselineRef} still publishes ` + + `(it added: ${skew.added.join(', ') || 'nothing'})` + ).toEqual([]) + }) + + it( + 'still fails a pairing whose peer cannot decode an opcode the other side sends', + async () => { + // The regression case for the guard itself: relaxing a stale field list must + // not relax the real incompatibility. A short barrier only bounds a stall + // that is already certain — the frame either arrives at once, or never. + const inputOpcode = Number(current.codec.TerminalStreamOpcode.Input) + const stall = await runTerminalSkewJourney({ + hostBuild: withoutOpcodeSupport(current, 'Input'), + clientBuild: current, + barrierTimeoutMs: 2_000 + }).then( + () => null, + (error: unknown) => error + ) + + expect(stall).toBeInstanceOf(CrossVersionJourneyStall) + const stalled = stall as CrossVersionJourneyStall + expect(stalled.step).toBe('input-reaches-process') + expect(stalled.record.completed).not.toContain('input-reaches-process') + expect(stalled.record.inputAtProcess).toEqual([]) + expect(stalled.record.rejected).toContainEqual( + expect.objectContaining({ direction: 'client-to-host', rawOpcode: inputOpcode }) + ) }, SUITE_TIMEOUT_MS ) diff --git a/tests/e2e/cross-version-wire/published-field-shape.ts b/tests/e2e/cross-version-wire/published-field-shape.ts new file mode 100644 index 00000000000..726824add2e --- /dev/null +++ b/tests/e2e/cross-version-wire/published-field-shape.ts @@ -0,0 +1,38 @@ +/** + * Reading the field shape of a published frame from the frame itself, so a + * cross-version expectation can be stated against the build that produced it. + * + * The baseline this suite pairs against is whichever release tag is newest, and + * that moves on every cut. An expectation written as a literal list of fields the + * old side does or does not have therefore expires by itself: the first release + * containing an already-merged optional field turns the assertion red on whatever + * pull request happens to be in flight, with no code change anywhere. + */ + +export type PublishedFieldSkew = { + /** Names only the newer side publishes — additive, and safe under Rule 1. */ + added: string[] + /** Names the older side still publishes and the newer side dropped — a break. */ + removed: string[] +} + +/** Sorted union of the keys across one published frame sequence. */ +export function publishedFieldNames(payloads: Record[]): string[] { + return [...new Set(payloads.flatMap((payload) => Object.keys(payload)))].sort() +} + +/** + * Which field names the two sides disagree on, by direction. Both sides are read + * from a real pairing; neither is a list this file knows. + */ +export function comparePublishedFields(args: { + older: string[] + newer: string[] +}): PublishedFieldSkew { + const older = new Set(args.older) + const newer = new Set(args.newer) + return { + added: [...newer].filter((name) => !older.has(name)).sort(), + removed: [...older].filter((name) => !newer.has(name)).sort() + } +} diff --git a/tests/e2e/cross-version-wire/published-field-shape.unit.test.ts b/tests/e2e/cross-version-wire/published-field-shape.unit.test.ts new file mode 100644 index 00000000000..53d9874f940 --- /dev/null +++ b/tests/e2e/cross-version-wire/published-field-shape.unit.test.ts @@ -0,0 +1,50 @@ +import { describe, expect, it } from 'vitest' +import { comparePublishedFields, publishedFieldNames } from './published-field-shape' + +describe('published field shape', () => { + it('unions the keys across a frame sequence, so a field only one frame carries counts', () => { + expect( + publishedFieldNames([ + { kind: 'scrollback', seq: 0 }, + { kind: 'scrollback', seq: 27, requestId: 1 }, + { kind: 'scrollback', seq: 27 } + ]) + ).toEqual(['kind', 'requestId', 'seq']) + }) + + it('reads an empty sequence as no fields, which is what the anti-vacuous check tests', () => { + expect(publishedFieldNames([])).toEqual([]) + }) + + it('reports an added field as added and nothing as removed', () => { + expect( + comparePublishedFields({ + older: ['cols', 'kind', 'rows'], + newer: ['alternateScreen', 'cols', 'kind', 'rows', 'terminalOwner'] + }) + ).toEqual({ added: ['alternateScreen', 'terminalOwner'], removed: [] }) + }) + + it('reports a field the newer side stopped publishing as removed', () => { + expect( + comparePublishedFields({ + older: ['cols', 'kind', 'rows', 'source'], + newer: ['cols', 'kind', 'rows'] + }) + ).toEqual({ added: [], removed: ['source'] }) + }) + + it('is silent when both sides publish the same names in a different order', () => { + expect(comparePublishedFields({ older: ['rows', 'cols'], newer: ['cols', 'rows'] })).toEqual({ + added: [], + removed: [] + }) + }) + + it('does not repeat a name a caller passed twice', () => { + expect(comparePublishedFields({ older: [], newer: ['seq', 'seq'] })).toEqual({ + added: ['seq'], + removed: [] + }) + }) +}) diff --git a/tests/e2e/cross-version-wire/terminal-skew-journey.ts b/tests/e2e/cross-version-wire/terminal-skew-journey.ts index e5a84fc1683..136c8ee0179 100644 --- a/tests/e2e/cross-version-wire/terminal-skew-journey.ts +++ b/tests/e2e/cross-version-wire/terminal-skew-journey.ts @@ -63,14 +63,20 @@ function nameOpcode(build: TerminalWireBuild, opcode: number): string { return typeof name === 'string' ? name : `Opcode${opcode}` } -async function barrier(label: string, predicate: () => boolean): Promise { - try { - await vi.waitFor(() => expect(predicate()).toBe(true), { - timeout: BARRIER_TIMEOUT_MS, - interval: 5 - }) - } catch { - throw new Error(`Cross-version journey stalled at barrier: ${label}`) +/** + * A pairing that never advanced. Carries the partial record so a caller can read + * what the wire actually did — which frames the receiving decoder refused, and + * which steps completed before the stall. + */ +export class CrossVersionJourneyStall extends Error { + readonly step: JourneyStep + readonly record: JourneyRecord + + constructor(step: JourneyStep, detail: string, record: JourneyRecord) { + super(`Cross-version journey stalled at ${step}: ${detail}`) + this.name = 'CrossVersionJourneyStall' + this.step = step + this.record = record } } @@ -86,8 +92,11 @@ async function barrier(label: string, predicate: () => boolean): Promise { export async function runTerminalSkewJourney(args: { hostBuild: TerminalWireBuild clientBuild: TerminalWireBuild + /** Only lower this for a pairing whose stall is the expected outcome. */ + barrierTimeoutMs?: number }): Promise { const { hostBuild, clientBuild } = args + const barrierTimeoutMs = args.barrierTimeoutMs ?? BARRIER_TIMEOUT_MS const hostStub: HostTerminalRuntimeStub = createHostTerminalRuntimeStub({ terminalHandle: TERMINAL_HANDLE, initialBuffer: INITIAL_BUFFER @@ -114,6 +123,21 @@ export async function runTerminalSkewJourney(args: { missingRuntimeMethods: hostStub.missingRuntimeMethods } + const barrier = async ( + step: JourneyStep, + detail: string, + predicate: () => boolean + ): Promise => { + try { + await vi.waitFor(() => expect(predicate()).toBe(true), { + timeout: barrierTimeoutMs, + interval: 5 + }) + } catch { + throw new CrossVersionJourneyStall(step, detail, record) + } + } + // Name opcodes with whichever build knows more of them, so an unknown opcode in // the journey reads as `Opcode17` instead of silently borrowing a wrong name. const namingBuild = @@ -168,26 +192,27 @@ export async function runTerminalSkewJourney(args: { try { let terminal = await subscribe() - await barrier('subscribe: client never saw a `subscribed` event', () => subscribedCount >= 1) + await barrier('subscribe', 'client never saw a `subscribed` event', () => subscribedCount >= 1) record.subscribedEvents = link.connections.flatMap((connection) => connection.events.filter((event) => event.type === 'subscribed') ) record.completed.push('subscribe') await barrier( - 'first-snapshot: client never rendered the initial buffer snapshot', + 'first-snapshot', + 'client never rendered the initial buffer snapshot', () => record.snapshotsRendered.length >= 1 ) record.completed.push('first-snapshot') terminal.sendInput(FIRST_INPUT) - await barrier('input-reaches-process: host never wrote the client input to the PTY', () => + await barrier('input-reaches-process', 'host never wrote the client input to the PTY', () => hostStub.writtenInput.includes(FIRST_INPUT) ) record.completed.push('input-reaches-process') hostStub.emitOutput(LIVE_OUTPUT) - await barrier('live-output: client never rendered host output', () => + await barrier('live-output', 'client never rendered host output', () => record.dataRendered.join('').includes(LIVE_OUTPUT.trim()) ) record.completed.push('live-output') @@ -203,7 +228,8 @@ export async function runTerminalSkewJourney(args: { const closesBeforeDrop = record.transportCloses link.disconnect() await barrier( - 'transport-drop: client never observed the transport close', + 'transport-drop', + 'client never observed the transport close', () => record.transportCloses > closesBeforeDrop ) record.completed.push('transport-drop') @@ -211,7 +237,8 @@ export async function runTerminalSkewJourney(args: { const subscribedBeforeReconnect = subscribedCount terminal = await subscribe() await barrier( - 'resubscribe: client never re-established the stream after reconnect', + 'resubscribe', + 'client never re-established the stream after reconnect', () => subscribedCount > subscribedBeforeReconnect ) record.subscribedEvents = link.connections.flatMap((connection) => @@ -220,7 +247,7 @@ export async function runTerminalSkewJourney(args: { record.completed.push('resubscribe') terminal.sendInput(SECOND_INPUT) - await barrier('input-after-reconnect: host never wrote post-reconnect input to the PTY', () => + await barrier('input-after-reconnect', 'host never wrote post-reconnect input to the PTY', () => hostStub.writtenInput.includes(SECOND_INPUT) ) record.completed.push('input-after-reconnect') diff --git a/tests/e2e/cross-version-wire/versioned-agent-session-wire.ts b/tests/e2e/cross-version-wire/versioned-agent-session-wire.ts index 420efcbd994..1b0dc297f81 100644 --- a/tests/e2e/cross-version-wire/versioned-agent-session-wire.ts +++ b/tests/e2e/cross-version-wire/versioned-agent-session-wire.ts @@ -1,9 +1,6 @@ -import { readFileSync, readdirSync } from 'node:fs' -import { join } from 'node:path' import { importReleaseCheckoutModule, materializeReleaseCheckout, - REPO_ROOT, type ReleaseCheckout } from './release-checkout' @@ -16,6 +13,11 @@ import { export const WORKING_TREE = 'working-tree' as const +/** Each build owns its own copy of the module-level host slot, so a host installed + * in current source is invisible to a release checkout's dispatcher. */ +const STRUCTURED_HOST_REGISTRY = + '/src/main/native-chat/agent-session-wire/structured-agent-session-registry.ts' + export type RpcReply = { id: string ok: boolean @@ -53,36 +55,34 @@ export type AgentSessionWireBuild = { /** A dispatcher carrying a method set this build really ships, so an * unknown-method answer is about the method and not an empty registry. */ createDispatcher: (runtime: unknown) => AgentSessionDispatcher + /** Put a host in *this* build's slot. Loaded on call so a release that predates + * the surface stays loadable, and throws rather than no-opping so a build with + * no slot cannot read as a surface that answered. */ + installStructuredHost: (host: unknown) => Promise } type DispatcherModule = { RpcDispatcher: new (options: { runtime: unknown; methods: unknown[] }) => AgentSessionDispatcher } -// A dotted literal in a `name:` position. Deliberately loose: over-matching only -// makes "this build registers no agentSession method" a stronger claim. -const METHOD_NAME = /\bname:\s*'([A-Za-z][A-Za-z0-9]*(?:\.[A-Za-z0-9]+)+)'/g - -/** - * Method names declared under `runtime/rpc/methods`, scanned rather than imported: - * a released build's method manifest reaches Electron, which cannot load here. - */ -function scanMethodNames(root: string): string[] { - const names = new Set() - const walk = (directory: string): void => { - for (const entry of readdirSync(directory, { withFileTypes: true })) { - const full = join(directory, entry.name) - if (entry.isDirectory()) { - walk(full) - } else if (entry.isFile() && entry.name.endsWith('.ts')) { - for (const match of readFileSync(full, 'utf8').matchAll(METHOD_NAME)) { - names.add(match[1]!) - } +function registeredMethodNames(methods: readonly unknown[]): string[] { + return methods + .flatMap((method) => { + if (!method || typeof method !== 'object') { + return [] } - } + const name = Reflect.get(method, 'name') + return typeof name === 'string' ? [name] : [] + }) + .sort() +} + +function applyStructuredHost(module: Record, label: string, host: unknown): void { + const install = module.setStructuredAgentSessionHost + if (typeof install !== 'function') { + throw new Error(`Build ${label} publishes no structured agent-session host registry`) } - walk(join(root, 'src', 'main', 'runtime', 'rpc', 'methods')) - return [...names].sort() + ;(install as (next: unknown) => void)(host) } function capabilityStrings(module: Record): readonly string[] { @@ -94,52 +94,58 @@ function capabilityStrings(module: Record): readonly string[] { } async function loadWorkingTreeBuild(): Promise { - const [protocol, dispatcher, structured, aiVault, sessionTabs, terminal] = await Promise.all([ + const [protocol, dispatcher, methodRegistry] = await Promise.all([ import('../../../src/shared/protocol-version'), import('../../../src/main/runtime/rpc/dispatcher'), - import('../../../src/main/runtime/rpc/methods/structured-agent-session'), - import('../../../src/main/runtime/rpc/methods/ai-vault'), - import('../../../src/main/runtime/rpc/methods/session-tabs'), - import('../../../src/main/runtime/rpc/methods/terminal') + import('../../../src/main/runtime/rpc/methods') ]) const module = dispatcher as unknown as DispatcherModule + const methods = methodRegistry.ALL_RPC_METHODS as unknown[] return { label: WORKING_TREE, revision: WORKING_TREE, capabilities: capabilityStrings(protocol as unknown as Record), protocolVersion: protocol.RUNTIME_PROTOCOL_VERSION, - methodNames: scanMethodNames(REPO_ROOT), + methodNames: registeredMethodNames(methods), createDispatcher: (runtime) => new module.RpcDispatcher({ runtime, - methods: [ - ...(structured.STRUCTURED_AGENT_SESSION_METHODS as unknown[]), - ...(aiVault.AI_VAULT_METHODS as unknown[]), - ...(sessionTabs.SESSION_TAB_METHODS as unknown[]), - ...(terminal.TERMINAL_METHODS as unknown[]) - ] - }) + methods + }), + installStructuredHost: async (host) => { + const registry = + await import('../../../src/main/native-chat/agent-session-wire/structured-agent-session-registry') + applyStructuredHost(registry as unknown as Record, WORKING_TREE, host) + } } } async function loadReleaseBuild(checkout: ReleaseCheckout): Promise { - const [protocol, dispatcher, terminalMethods] = await Promise.all([ + const [protocol, dispatcher, methodRegistry] = await Promise.all([ importReleaseCheckoutModule(checkout, '/src/shared/protocol-version.ts'), importReleaseCheckoutModule(checkout, '/src/main/runtime/rpc/dispatcher.ts'), - importReleaseCheckoutModule(checkout, '/src/main/runtime/rpc/methods/terminal.ts') + importReleaseCheckoutModule(checkout, '/src/main/runtime/rpc/methods/index.ts') ]) const module = dispatcher as unknown as DispatcherModule + const methods = methodRegistry.ALL_RPC_METHODS as unknown[] return { label: checkout.ref, revision: checkout.commit, capabilities: capabilityStrings(protocol), protocolVersion: protocol.RUNTIME_PROTOCOL_VERSION as number, - methodNames: scanMethodNames(checkout.root), + methodNames: registeredMethodNames(methods), createDispatcher: (runtime) => new module.RpcDispatcher({ runtime, - methods: terminalMethods.TERMINAL_METHODS as unknown[] - }) + methods + }), + installStructuredHost: async (host) => { + applyStructuredHost( + await importReleaseCheckoutModule(checkout, STRUCTURED_HOST_REGISTRY), + checkout.ref, + host + ) + } } } diff --git a/tests/e2e/cross-version-wire/versioned-terminal-wire.ts b/tests/e2e/cross-version-wire/versioned-terminal-wire.ts index 08e721900d6..421f78cdb88 100644 --- a/tests/e2e/cross-version-wire/versioned-terminal-wire.ts +++ b/tests/e2e/cross-version-wire/versioned-terminal-wire.ts @@ -149,3 +149,37 @@ export async function loadTerminalWireBuild(ref: string): Promise` rather than borrowing a name this build lost. + TerminalStreamOpcode: Object.fromEntries( + Object.entries(build.codec.TerminalStreamOpcode).filter( + ([name, value]) => name !== opcodeName && value !== opcodeName + ) + ), + decodeTerminalStreamFrame: (bytes) => { + const frame = build.codec.decodeTerminalStreamFrame(bytes) + return frame && frame.opcode === opcode ? null : frame + } + } + } +}