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 + } + } + } +}