mirror of
https://github.com/stablyai/orca.git
synced 2026-10-30 00:27:39 +00:00
fix(terminal): stop gating OutputSpan on the desktop multiplex path
The OutputSpan gate is right for `terminal.subscribe` and wrong for
`terminal.multiplex`. Keying spans off a declared `outputSpan` capability treats
"peer did not declare it" as "peer cannot decode opcode 15", which is false on
multiplex: opcode 15 shipped in v1.4.147 (c0f0810dd9, 2026-07-19) and every
capability a peer could declare to prove it is younger — `ackOutput` by 9 days,
`ackOutputSourceRanges` by 10. So the gate read the entire installed base as
incapable and downgraded it.
The downgrade cannot be made safe there. A plain Output frame carries only
`(data, seq)`, so a seq-tracking peer reconstructs a run as `seq - data.length`,
which a transformed run (`rawLength !== data.length`) contradicts by definition.
Three failures reproduced through the real dispatcher against the shipped desktop
capability set:
- a zero-byte absorbed-query frame fails `TerminalSourceRangeLedger.canAccept`
(`encodedBytes > 0`), parks in `ackPendingOutput`, and head-blocks the flush
loop on the same head forever — all later output is lost;
- every transformed emission reads as a dropped-frame gap in `detectOutputGap`
and triggers `requestResyncSnapshot`;
- a >48 KiB transformed run takes the `shouldDelayFinalSeq` arm, which omits
`sourceRanges`, flipping the ledger's mapping mode and detaching the stream.
Multiplex now always frames transformed runs as OutputSpan; the vestigial
`outputSpan` capability is removed from the multiplex schema, stream state, and
the renderer's subscribe frame. Subscribe keeps the gate: mobile ignores Output
`seq` outright, which is what makes the downgrade lossless there — that property,
not the capability flag, is the licence, so the chunker option is now
`transformedRuns: 'span' | 'downgrade-to-text'` and the unsound call site cannot
be written by accident.
Coverage for the fallback arm was absent — every fixture was a 14-character
string, so the multi-chunk downgrade never ran and deleting a yield (48 KiB of
silently dropped output) passed everything. Adds over-cap, zero-byte and
per-frame-seq contracts plus a 600-case fuzz, transformed and multi-chunk
transformed steps to the cross-version journey (which pins current code against
the real v1.4.173 release client), and repairs the JS benchmark that the required
options argument broke.
Refs STA-3482
This commit is contained in:
@@ -51,27 +51,54 @@ peer confirms it. The existing pattern is `SetOutputPaused` (opcode 16):
|
||||
- the client sends opcode 16 only after that echo (`stream.supportsOutputPause`);
|
||||
- the host only acts on opcode 16 when it negotiated it (`stream.supportsOutputPause`).
|
||||
|
||||
`OutputSpan` (opcode 15) is the worked example of getting this wrong. It shipped
|
||||
ungated on both the legacy `terminal.subscribe` stream and the multiplex stream, so
|
||||
transformed emissions (SSH relay, startup ingress, daemon keep-tail salvage) reached
|
||||
mobile as an opcode its vendored table does not contain — dropped with no error, no
|
||||
banner, no retry. It is now gated on `capabilities.outputSpan`, and a sender that has
|
||||
not been told the peer can decode a span **downgrades to plain `Output`** rather than
|
||||
sending nothing. That downgrade is the part worth copying: for an opcode that carries
|
||||
user-visible data, "don't send it" is still data loss, so a gate needs a fallback that
|
||||
preserves the payload, not just a suppressed frame.
|
||||
`OutputSpan` (opcode 15) is the worked example, and it is worth reading closely because
|
||||
the obvious fix was wrong on half the paths.
|
||||
|
||||
It shipped ungated in v1.4.147 (2026-07-19) on both the legacy `terminal.subscribe`
|
||||
stream and the multiplex stream, so transformed emissions (SSH relay, startup ingress,
|
||||
daemon keep-tail salvage) reached mobile as an opcode its vendored table does not
|
||||
contain — dropped with no error, no banner, no retry. On `terminal.subscribe` it is now
|
||||
gated on `capabilities.outputSpan`, and a sender that has not been told the peer can
|
||||
decode a span **downgrades to plain `Output`** rather than sending nothing.
|
||||
|
||||
On `terminal.multiplex` it is deliberately **not** gated. Two reasons, and both
|
||||
generalise:
|
||||
|
||||
**A capability cannot retroactively prove incapability.** The gate keys off the peer
|
||||
declaring `outputSpan`, a flag no shipped build sends. Opcode 15 predates every
|
||||
capability a multiplex peer could declare — `ackOutput` by 9 days, `ackOutputSourceRanges`
|
||||
by 10 — so "did not declare it" is indistinguishable between a build from before the
|
||||
opcode and the entire installed base from after it. Since mixed versions are the normal
|
||||
state, a gate added after an opcode ships reads the whole fleet as incapable.
|
||||
|
||||
**A fallback is only a fallback if it is lossless for that peer.** A plain `Output`
|
||||
frame carries `(data, seq)` and nothing else, so the receiver can only reconstruct a run
|
||||
as `seq - data.length`. A transformed run has `rawLength !== data.length` by definition.
|
||||
Downgrading it to `Output` therefore cannot be made correct for a peer that tracks
|
||||
`seq`, and the desktop multiplex client does: it head-blocks the ack queue on a
|
||||
zero-byte frame, reads a false dropped-frame gap on every transformed emission, and
|
||||
detaches the stream when a >48 KiB run flips the source-range ledger's mapping mode.
|
||||
The downgrade works on `terminal.subscribe` for exactly one reason — mobile ignores
|
||||
Output `seq` entirely — and that fact, not the capability flag, is what licenses it.
|
||||
|
||||
So: for an opcode carrying user-visible data, "don't send it" is still data loss, and a
|
||||
gate needs a fallback that preserves the payload. But check what the fallback frame can
|
||||
actually express for the peer you are aiming it at before you reach for one, and prefer
|
||||
gating at the point where the new opcode was genuinely new rather than adding a gate to
|
||||
a path where it has already shipped.
|
||||
|
||||
### Mobile vendors its own opcode table
|
||||
|
||||
`mobile/` is a separate pnpm workspace and cannot import `src/shared/`, so
|
||||
`mobile/src/transport/terminal-stream-protocol.ts` holds a **hand-copied** enum. It
|
||||
knows only 1-6 and 12. Adding an opcode to the shared file therefore does not add it
|
||||
to mobile, and nothing fails — not typecheck, not lint, not the cross-version harness,
|
||||
which pairs desktop builds only. The two tables drift silently by construction.
|
||||
`mobile/` is a separate pnpm workspace, so
|
||||
`mobile/src/transport/terminal-stream-protocol.ts` has historically held a **hand-copied**
|
||||
enum knowing only 1-6 and 12. Adding an opcode to the shared file did not add it to
|
||||
mobile, and nothing failed — not typecheck, not lint, not the cross-version harness,
|
||||
which pairs desktop builds only. The two tables drifted silently by construction.
|
||||
|
||||
So "is this opcode safe to send?" cannot be answered by reading the shared enum. Until
|
||||
the vendored copy is generated from the shared one, treat every mobile-reachable
|
||||
opcode above 12 as unsupported unless the stream negotiated it.
|
||||
`terminal-subscribe-output-span-capability.test.ts` now parses mobile's real source and
|
||||
forces an explicit per-opcode decision, so a new opcode is neither "mobile decodes it"
|
||||
nor "gated" until someone says which. Until mobile shares the table outright, treat
|
||||
every mobile-reachable opcode above 12 as unsupported unless the stream negotiated it.
|
||||
|
||||
Reuse an existing opcode with a new optional payload field (Rule 1) whenever that
|
||||
expresses the change; reach for a new opcode only when framing genuinely differs.
|
||||
|
||||
Reference in New Issue
Block a user