feat(viewport): bounded best-effort tail read and snapshot-first attach (plan #959) - #963
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #963
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← feat/a5-bounded-tail-read · 23 files · opt-in bounded viewport read + snapshot-first SSE (plan #959)
Lenses run: L1, L2, L5, L6, L7, L8 (skip: L3 no host/Wasm/UI flip; L4 no CI/wasm/deploy; L9 no palette/DOM chrome)
AGENTS.md read: yes (also docs/feature-divide.md, SECURITY.md, plan #959)
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1+L5 | parseViewportMode (lib/viewportStreamProtocol.ts · GET) treats ?viewportVersion=1 with neither hydrate=tail nor startIndex as {kind:'indexed', startIndex:0} via q.get('startIndex') ?? '0'. That is full origin replay of the durable chunk log under the new framing. Plan G1/G2 and #924 exist specifically because legacy startIndex=0 ships historical thinking and is unbounded in run length. POST defaulting to 0 is correct (new run). GET is not. |
1. Long-running turn, H0 ≫ 2048. 2. Client opts into v1 the same way POST does: GET /api/turns/:runId/stream?sessionId=:id&viewportVersion=1 (no hydrate, no cursor) — #960 is likely to copy this. 3. Handler takes the indexed path, open(0), and forwards every stored reasoning_delta as turn_event. Cold caps (2048 / 8 MiB / 5 s) never run. |
Defender: docs list only hydrate=tail and startIndex=N; implicit 0 matches legacy. Fails: docs never describe this third GET form; protocol tests never assert GET viewportVersion=1 alone is rejected (lib/viewportStreamProtocol.test.ts only checks POST); route tests never hit it (viewport.test.ts rejects version=2 / hydrate+startIndex / 1e2 / duplicate sessionId, not the omitted-selector case). Production host is not flipped yet, so this is not a current-user outage — it is a merge-now protocol footgun that makes G1 false for the obvious v1 GET. |
high |
| Minor | L1 | Negotiated live path (lib/agent/viewportStream.ts · nextFrame) schedules the first run.status poll at TURN_STREAM_STATUS_POLL_MS (1 s). bodyForRun / pipeRunReadable uses a 0-delay first poll specifically to unstick cancelled/failed hanging getReadable(). The v1 route never calls bodyForRun on the negotiated branch, so it also skips the cancelled/failed “never getReadable()” short-circuit. |
Attach hydrate=tail or hot startIndex=N to a cancelled/failed run whose producer never closed the writable (documented C16 class). Legacy: immediate synthetic SSE, no SDK readable. v1: open() hangs; recovering/live waits 5 s sample budget (cold) + 1 s first poll before viewport_end. |
Defender: recoverViewport already times out reads at 5 s; T8 is best-effort history for terminal runs. Partially true — not unbounded — but the 0-delay poll exists in-tree for this hang class and was not carried over. Tests only cover hung readables after a 1 s advance (viewportStream.test.ts). |
high |
| Nit | L8 | SECURITY.md “Read-only partial viewport” says these routes never “publish session/transcript data.” They exist to publish a partial transcript/carrier view to the owning session. Overstated; the rest of the paragraph (auth, bound pointer, no secrets/run-inputs) is accurate. |
A later reader treats “never publish transcript data” as the security invariant and flags the JSON viewport 200 as a regression, or omits row allowlisting. | Defender: meant “don’t publish as a public dataset / don’t write.” The sentence still says “publish.” | high |
Residual risk
Cold recovery is best-effort: a burst produced during the sample window is skipped (gap:true); one Blob read() still materializes a whole ≤8 MiB object before the byte check; blob.read / SDK getInfo are not abort-linked so late work can finish after the deadline (documented in docs/harness-limits.md). getReadable is a durable-chunk stream (one formatTurnSse write per stored index) so treating each read() as one frame is consistent with Workflows 4.8.4 startIndex — but unlike pipeRunReadable there is no parseSseChunk rest-buffer; if a world implementation ever coalesces or splits chunk bytes, parseViewportEvent (blocks.length !== 1) will skip a whole batch as one index or fatal UTF-8 will close live with STREAM_UNAVAILABLE. stillOwned is only checked before releasing the snapshot, not during the live tail (same TOCTOU as today’s attach). No production HarnessHost flip — #924 stays open.
Merge guidance
- CONCERNS: do not merge until the Major is fixed — GET v1 must not origin-replay unless the client explicitly sent
startIndex=0(orhydrate=tailfor bounded recovery). - Suggested patch:
parseViewportModeGET returnsnullwhenviewportVersion=1and neitherhydratenorstartIndexis present; add protocol + route tests that this 400s beforegetRun/ Blob. Optional: 0-delay first status poll; tighten the SECURITY.md sentence.
What was not attacked
Live Workflows getReadable / getInfo against origin Production; Vercel Blob byte-range behavior; DO runner; Gateway; Wasm paint beyond the two int/viewport-read.int.test.ts rows (read as source, not re-executed here); #960 host wiring.
|
Follow-up on the adversarial review (CONCERNS): landed in
|
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #963
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← feat/a5-bounded-tail-read · 23 files · opt-in bounded viewport read + snapshot-first SSE (plan #959), including follow-up 81ad3fe
Lenses run: L1, L2, L5, L6, L7, L8 (skip: L3 no host/Wasm/UI flip; L4 no CI/wasm/deploy; L9 no palette/DOM chrome)
AGENTS.md read: yes (also docs/feature-divide.md, SECURITY.md, plan #959)
Prior review (81ad3fe) fixed implicit GET v1 origin replay, the 0-delay live status poll, and the SECURITY.md wording. Those do not survive re-attack. The cancelled/failed getReadable gate did.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1+L5+L6 | Negotiated GET still touches getReadable on already-cancelled/failed runs. bodyForRun exists because that readable can hang while run.status is terminal (C16). The v1 branch never calls bodyForRun. Cold attach (app/api/turns/[runId]/stream/route.ts) awaits runReader.nextIndex() before the SSE body, and nextIndex is getReadable({ startIndex: -1 }) — the live tail, the hang point. Timeout maps to sanitized 503 with no viewport_state, no head snapshot, no viewport_end. Plan T8 / locked decision 2: a terminal run still returns sampled/head display; emit recovering state before H0. Indexed v1 open(N) has the same hole if open() itself never returns (the 0-delay poll never starts). |
1. Stop a live turn so Workflow status is cancelled but the producer never closed the writable. 2. GET /api/turns/:runId/stream?sessionId=:id&viewportVersion=1&hydrate=tail. 3. Handler calls nextIndex() → hanging getReadable({startIndex:-1}). 4. After VIEWPORT_RECOVERY_MAX_MS the route catch returns {error:'Viewport stream unavailable.'} 503. Blob head is never offered. Legacy attach would have returned synthetic SSE immediately and never called getReadable. |
Defender: 0-delay poll in viewportStream.nextFrame unsticks a hanging reader. Fails: poll is armed only after open().getReader(); the route hang is before viewportStream is constructed. Defender: recoverViewport already times out sample reads at 5s and falls back to head. Fails: that code is not reached if nextIndex() throws into the route catch. Defender: production host is not flipped. Fails: this is the #960 cold GET; T8 is in-scope; viewport.test.ts mocks getTailIndex as instant and never asserts cancelled+hanging getReadable is skipped (pipeRunReadable.test.ts rows 13b/13c cover that class on the legacy path only). |
high |
Residual risk
Cold recovery remains best-effort: a burst during the sample window is skipped (gap:true); one Blob read() still materializes a whole ≤8 MiB object before the byte check; blob.read / SDK getInfo are not abort-linked so late work can finish after the deadline (documented in docs/harness-limits.md). getReadable is treated as one stored frame per read() with no parseSseChunk rest-buffer — coalesced/split chunk bytes skip a batch or fatal UTF-8 closes live with STREAM_UNAVAILABLE. stillOwned is only checked before releasing the snapshot, not during the live tail (same TOCTOU as today’s attach). No production HarnessHost flip — #924 stays open. getTailIndex is a structural optional; absent helper fail-closes 503 rather than origin-falling-back (tested).
Merge guidance
- CONCERNS: do not merge until the Major is fixed — negotiated GET must not call
getReadablewhenrun.statusis alreadycancelled/failed(same gate asbodyForRun). Cold must still return head snapshot +viewport_end. Emitviewport_statebefore the H0 probe so a slow/unavailable tail cannot 503 with an empty body. - Suggested patch: race
run.status(1 s, same asbodyForRun) beforenextIndex/open. On cancelled/failed: skip every SDK readable; cold usesreadViewportHeadonly; then syntheticviewport_end. Move H0 capture intoviewportStreamafterviewport_state. Add route tests that a hanginggetReadableis never invoked for cancelled/failed hydrate=tail and indexed GET.
What was not attacked
Live Workflows getReadable / getTailIndex against origin Production; Vercel Blob byte-range behavior; DO runner; Gateway; Wasm paint beyond the two int/viewport-read.int.test.ts rows (read as source, not re-executed here); #960 host wiring.
Adversarial review on PR #963: negotiated GET raced nextIndex/open before the C16 cancelled/failed gate, so a hanging tail readable 503'd with no snapshot. Status is checked first (same 1s race as bodyForRun); cold hydrate still returns a head snapshot then viewport_end.
|
Follow-up on the second adversarial review (CONCERNS): landed in
|
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #963
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← feat/a5-bounded-tail-read · 23 files · opt-in bounded viewport read + snapshot-first SSE (plan #959), including follow-ups 81ad3fe and 7e35603
Lenses run: L1, L2, L5, L6, L7, L8 (skip: L3 no host/Wasm/UI flip; L4 no CI/wasm/deploy; L9 no palette/DOM chrome)
AGENTS.md read: yes (also docs/feature-divide.md, SECURITY.md, plan #959)
Prior reviews fixed implicit GET v1 origin replay (81ad3fe) and the cancelled/failed getReadable gate (7e35603). Those do not survive re-attack. The remaining hole is the running cold path still blocking the SSE body on H0.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1+L5 | Cold GET still awaits runReader.nextIndex() and a second status() before new Response() (app/api/turns/[runId]/stream/route.ts). Plan #959 locked decision 2 step 1: after auth, emit viewport_state so #960 can show Busy/Stop while recovering; H0 is step 2 under the same 5 s recovery clock as head+sample. getTailIndex is world.streams.getInfo (workflow 4.8.4) — independent of the data reader, but still a network metadata call. A slow/unavailable tail 503s with no viewport_state, no Blob head, no viewport_error. Route also spends up to 1 s + 5 s (H0) + 5 s (redundant status) outside recoverViewport's fresh 5 s clock, so optional recovery can exceed the locked 5 s budget before the first SSE byte. 7e35603 only skipped this for already-cancelled/failed. |
1. Running turn; Workflows getInfo / getTailIndex hangs or the readable has no helper. 2. GET …/stream?sessionId=:id&viewportVersion=1&hydrate=tail. 3. Handler awaits nextIndex() (getReadable({startIndex:-1}) + getTailIndex) up to VIEWPORT_RECOVERY_MAX_MS before constructing the body. 4. Timeout/throw → {error:'Viewport stream unavailable.'} 503, empty body. Blob head is never offered. Client cannot paint recovering/Busy. Contrast: cancelled/failed now returns state+head+viewport_end. viewport.test.ts locks the 503 (unavailable initial tail fails attach, never guesses zero). |
Defender: plan decision 1 / 2.5 says 503 when initial live-tail metadata is unavailable; guessing startIndex=0 was the first review's Major. Fails: 503-vs-guess-zero is the right failure mode, not the right ordering. After a 200 SSE has started, in-band viewport_error + head snapshot (replace:false / skipStream) fails the live attach without guessing origin and without an empty HTTP body. Defender: production host is not flipped. Fails: this is the #960 cold GET; T8/decision 2 step 1 is in-scope; previous review already asked to move H0 after viewport_state and only the C16 skip landed. Defender: getReadable construction is inert until pull, so startIndex:-1 is not origin replay. True — not this finding. The hang/fail is getInfo, not replay. |
high |
| Minor | L1 | Cancelled/failed cold snapshot reports resumeIndex: 0 and sampledRange:{start:0,end:0} because the route forces initial=0 to avoid nextIndex. Terminal path correctly skips getReadable and ends, but the snapshot cursor is a lie about the tail. |
1. Cancel a long run. 2. hydrate=tail → snapshot resumeIndex:0 then viewport_end. 3. A #960 client that persists resumeIndex as turnStreamCursor will later send explicit startIndex=0. Indexed GET on a still-cancelled run now short-circuits to viewport_end (no replay) — bounded. Residual is a poisoned cursor if a later non-terminal attach uses it. |
Defender: viewport_end follows; docs say resumeIndex is not proof of history; host is not wired. Partially true — not a live origin replay today. Still a dishonest H on the snapshot the host is supposed to consume. |
medium |
Residual risk
Cold recovery remains best-effort: a burst during the sample window is skipped (gap:true); one Blob read() materializes a whole ≤8 MiB object before the byte check; blob.read / SDK getInfo are not abort-linked so late work can finish after the deadline (documented in docs/harness-limits.md). getReadable is treated as one stored frame per read() with no parseSseChunk rest-buffer — coalesced/split chunk bytes skip a batch or fatal UTF-8 closes live with STREAM_UNAVAILABLE. stillOwned is only checked before releasing the snapshot, not during the live tail (same TOCTOU as today’s attach). Status-probe timeout (1 s) then treating the run as non-hang-class is the same race as bodyForRun. No production HarnessHost flip — #924 stays open. getTailIndex is a real 4.8.4 helper (getInfo); absent helper fail-closes rather than origin-falling-back.
Merge guidance
- CONCERNS: do not merge until the Major is fixed — negotiated cold GET must
return new Response(viewportStream(…))immediately after auth. Capture H0 afterviewport_state, under the same 5 s recovery deadline as head+sample. Missing/hanging H0: head-only snapshot (do notopen(0)), then in-bandviewport_error/ no live attach — never empty 503. Keep the C16 skip (already inviewportStream). Drop the route’s redundant secondstatus()wait. - Suggested tests: running + missing
getTailIndexis 200 withviewport_statethenviewport_error,getReadablenever called with{startIndex:0}; hanginggetTailIndexstill emits state before the deadline; existing cancelled/failed rows stay green.
What was not attacked
Live Workflows getReadable / getInfo against origin Production; Vercel Blob byte-range behavior; DO runner; Gateway; Wasm paint beyond the two int/viewport-read.int.test.ts rows (read as source, not re-executed here); #960 host wiring.
Adversarial review on PR #963: cold GET awaited nextIndex/status before the SSE body, so a slow/unavailable getTailIndex 503'd with no viewport_state and no Blob head. H0 now runs after recovering state under the same 5s clock as head+sample; missing tail is in-band viewport_error after the head snapshot, never open(0).
|
Follow-up on the third adversarial review (CONCERNS): landed in
|
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #963
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← feat/a5-bounded-tail-read · 23 files · opt-in bounded viewport read + snapshot-first SSE (plan #959), including follow-ups 81ad3fe, 7e35603, 669563a
Lenses run: L1, L2, L5, L6, L7, L8 (skip: L3 no host/Wasm/UI flip; L4 no CI/wasm/deploy; L9 no palette/DOM chrome)
AGENTS.md read: yes (also docs/feature-divide.md, SECURITY.md, plan #959)
Prior reviews fixed implicit GET v1 origin replay (81ad3fe), the cancelled/failed getReadable gate (7e35603), and H0-before-SSE (669563a). Those do not survive re-attack. The remaining hole is the live nextFrame poll treating completed like hang-class cancel.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1+L6 | nextFrame (lib/agent/viewportStream.ts) 0-delay-polls isTerminalRunStatus, which includes completed, and a new nextFrame runs on every stored frame. pipeRunReadable 0-delay-polls only on the first waiting pull, injects only while pullWaiting is still true (buffered microtask read beats the macrotask), then uses a 1 s cadence — with an explicit test completed + buffered text_delta then hang → delta then done; inject does not precede. Viewport copied the 0-delay hang-unstick from review 1 but not the completed-drain discipline. Docs (docs/agent-stream.md) already claim “Completed-run buffered frames drain before a hung read is resolved from a terminal status poll.” The only completed unit row is a hung empty readable (viewportStream.test.ts polls a hung readable and emits synthetic completed). |
1. Completed turn with stored chunks still in the Workflow log (C16 completed replay), or a live v1 POST whose run.status flips to completed while done/text_delta is still a network read away. 2. GET …/stream?sessionId=:id&viewportVersion=1&startIndex=0 (documented explicit replay) or the live loop after snapshot/POST. 3. After any delivered frame (or on the first network fetch), nextFrame starts read() and setTimeout(poll, 0). 4. status === 'completed' is already resolved; the 0-delay macrotask wins over an async getReadable chunk. 5. Client gets viewport_end and never sees remaining text_delta / producer done (finishReason, cwd, usage). Indexed reconnect of a just-finished turn is empty. |
Defender: in-memory test streams enqueue in start(), so read() is a microtask and beats setTimeout(0) — existing rows are green. Fails: production getReadable is WorkflowServerReadableStream over getInfo/chunk fetch, not sync enqueue; pipeRunReadable.test.ts exists because that race is real. Defender: skipReadable already covers cancelled/failed; completed is supposed to wrap getReadable (same C16 comment on bodyForRun). Fails: wrap-and-drain is the point; 0-delay completed skips the drain. Defender: no production host yet. Fails: POST ?viewportVersion=1 is in this PR; #960 hot resume of a completed run is the documented startIndex=N path. |
high |
| Minor | L1 | ViewportReducer.apply (lib/sessionViewport.ts) does if (ev.type === 'reasoning_delta') { this.lastKind = ''; return; }. Omitting historical thinking is required; resetting lastKind is not. Locked decision 3: “Adjacent sampled text deltas concatenate into a bounded recent assistant excerpt.” A thinking token between two text deltas becomes two assistant rows. |
1. Cold hydrate=tail samples text_delta "Hello" → reasoning_delta → text_delta " world". 2. Snapshot paints two assistant rows "Hello" and " world" instead of "Hello world". 3. #960 host hydrates that split into the ring. |
Defender: live host treats thinking as a ring boundary, so splitting is consistent. Fails: the snapshot does not paint the thinking row, so the boundary is invisible and the concatenate rule is the one that applies. Defender: modest fragmentation is waived. True for splice/alignment; not a license to break the one coalesce the reducer claims to do. Tests only cover reasoning-then-text, never reasoning-between-text (never retains historical thinking or appends aggregate done over text). |
high |
Residual risk
Cold recovery remains best-effort: a burst during the sample window is skipped (gap:true); one Blob read() materializes a whole ≤8 MiB object before the byte check; blob.read / SDK getInfo are not abort-linked so late work can finish after the deadline (documented in docs/harness-limits.md). getReadable is treated as one stored frame per read() with no parseSseChunk rest-buffer — coalesced/split chunk bytes skip a batch or fatal UTF-8 closes live with STREAM_UNAVAILABLE. stillOwned is only checked before releasing the snapshot, not during the live tail. Cancelled/failed cold snapshot still reports resumeIndex:0 (accepted after review 3). Status-probe timeout then treating the run as non-hang-class is the same race as bodyForRun. No production HarnessHost flip — #924 stays open. Tests mock DI (createProdServices / getRun / requireSessionUser); no new PGlite / createDbConnection (L6 cost gate not tripped).
Merge guidance
- CONCERNS: do not merge until the Major is fixed —
nextFrame0-delay poll may unstickcancelled/failedonly (hang class).completedmust drain buffered/in-flight stored frames; syntheticviewport_endonly after a 1 s hung read or readable EOF (same discipline aspipeRunReadable). Add a unit row: completed + bufferedtext_deltathen hang → delta(s) thenviewport_end, inject does not precede. - Minor: stop resetting
lastKindwhen droppingreasoning_delta; add a reducer row that thinking-between-text still concatenates.
What was not attacked
Live Workflows getReadable / getInfo against origin Production; Vercel Blob byte-range behavior; DO runner; Gateway; Wasm paint beyond the two int/viewport-read.int.test.ts rows (read as source, not re-executed here); #960 host wiring.
Adversarial review on PR #963: nextFrame 0-delay-polled isTerminalRunStatus (including completed) on every frame, truncating C16 completed replay and live producer done. 0-delay unsticks cancelled/failed only; completed waits 1s hung or EOF. Reducer no longer splits adjacent text across omitted thinking.
|
Follow-up on the fourth adversarial review (CONCERNS): landed in
|
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #963
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← feat/a5-bounded-tail-read · 23 files · opt-in bounded viewport read + snapshot-first SSE (plan #959), including follow-ups 81ad3fe, 7e35603, 669563a, 4e3998d
Lenses run: L1, L2, L5, L6, L7, L8 (skip: L3 no host/Wasm/UI flip; L4 no CI/wasm/deploy; L9 no palette/DOM chrome)
AGENTS.md read: yes (also docs/feature-divide.md, SECURITY.md, plan #959)
Prior reviews fixed implicit GET v1 origin replay (81ad3fe), the cancelled/failed getReadable gate (7e35603), H0-before-SSE (669563a), and completed 0-delay truncation (4e3998d). Those do not survive re-attack. The remaining hole is that unknown H0 is still encoded as resume cursor 0.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1+L6 | viewportStream (lib/agent/viewportStream.ts) on hanging/missing nextIndex() sets initialIndex = 0, skipStream: true, then yields viewport_snapshot with resumeIndex: 0 before viewport_error STREAM_UNAVAILABLE. ViewportStreamDecoder treats an explicit snapshot as a cursor jump (this.nextIndex = index). Plan #959 / docs/agent-stream.md: unknown position must close, never guess; GET v1 without a selector is already 400 because implicit 0 is the #924 class. This path reintroduces that cursor through the snapshot the decoder is required to apply. recoverViewport skipStream copies opts.initialIndex through as resumeIndex (lib/sessions/viewportRead.ts). Route test unavailable initial tail still emits recovering state and head; never origin-replays (app/api/turns/[runId]/stream/viewport.test.ts) only asserts this response did not open(0) — it never asserts resumeIndex is absent/non-zero, and the decoder starts with nextIndex === undefined so the jump to 0 is untested. Empty-run H0 is a successful getTailIndex() === -1 → next 0; that is not this catch. |
1. Long running turn, H0 ≫ 2048; Workflows getInfo / getTailIndex hangs or the readable has no helper. 2. GET …/stream?sessionId=:id&viewportVersion=1&hydrate=tail. 3. Client uses ViewportStreamDecoder (the contract this PR ships). 4. Records: viewport_state → snapshot {resumeIndex:0, source:'stored_head', sampledRange:{start:0,end:0}} → viewport_error. Decoder nextIndex is now 0. 5. Docs say preserve last applied cursor on error. 6. Next attach GET …&viewportVersion=1&startIndex=0 is explicit origin replay of historical reasoning_delta. Server did not open(0) on the failed hydrate; it published 0 as the applied resume. |
Defender: stream then errors; #960 is not flipped; open(0) is not called on this response; resumeIndex is “just a transport position.” Fails: the decoder in this PR must jump on snapshot; error handling says keep last applied cursor; startIndex=0 is a legal indexed GET (not 400); tests never pin resumeIndex on the H0-fail fixture; G1/G2 are false for the documented hydrate client that reconnects from the snapshot it just applied. |
high |
Residual risk
Attack did not re-breach C16 cancelled/failed getReadable, implicit GET v1 without a selector, H0-before-viewport_state, or completed 0-delay drain. Workflows getTailIndex is streams.getInfo (independent of the data reader; construction is inert until pull) so cancel-immediately on the -1 probe is not origin replay. Live chunk = one stored write, so per-read parseViewportEvent matches the SDK index. Best-effort omission, 8 MiB JSON.parse cost, and a 1 s C16 status race before the first SSE byte remain accepted. Production host is still unnegotiated — this finding is the protocol footgun #960 will copy.
Merge guidance
CONCERNS: do not merge until the Major is fixed. On unknown H0, emit the head snapshot for display without a resume cursor jump (resumeIndex omitted / not applied by the decoder). Empty-run resumeIndex: 0 stays valid only when nextIndex() actually returned 0. Pin it in the existing H0-fail route + decoder tests.
What was not attacked
Live DO runner, prod Gateway, real Workflows getInfo latency, #960 host consumer, Wasm paint beyond int/viewport-read.int.test.ts.
Adversarial review on PR #963: hanging/missing getTailIndex still yielded viewport_snapshot with resumeIndex:0 before STREAM_UNAVAILABLE. The decoder treats a snapshot as a cursor jump, so the next attach became explicit origin replay — the #924 class GET v1 already 400s. Display-only snapshots now omit resumeIndex; the decoder does not jump. Empty-run 0 remains valid only when nextIndex() actually returned 0.
|
Follow-up on the fifth adversarial review (CONCERNS): landed in
|
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #963
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← feat/a5-bounded-tail-read · 23 files · opt-in bounded viewport read + snapshot-first SSE (plan #959), including follow-ups through e9f39d7
Lenses run: L1, L2, L5, L6, L7, L8 (skip: L3 no host/Wasm/UI flip; L4 no CI/wasm/deploy; L9 no palette/DOM chrome)
AGENTS.md read: yes (also docs/feature-divide.md, SECURITY.md, plan #959)
Prior reviews fixed implicit GET v1 origin replay (81ad3fe), cancelled/failed getReadable (7e35603), H0-before-SSE (669563a), completed 0-delay drain (4e3998d), and display-only snapshots when H0 is unknown (e9f39d7). Those do not survive re-attack. The remaining hole is the T9 real-Wasm proof, which still models the pre-669563a caller-captured H0 and was not re-run after H0 moved inside viewportStream.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L6 | int/viewport-read.int.test.ts pre-calls adapter.nextIndex() (first getTailIndex = H0−1) then passes that as startIndex into viewportStream({ cold }). After 669563a, cold overwrites that value with a second nextIndex() (lib/agent/viewportStream.ts · H0 capture). The mock's second probe returns H = H0+7, so the sample open is H−2048 (97959) not H0−2048 (97952). expect(opens).toContain(H0 - 2048) fails. Follow-up comments after 669563a only ran targeted viewport/turns vitest files — not npm run test:int. T9/T1 no longer proves production samples from max(0,H0−2048). There is also no unit row that nextIndex returning 5000 then 5070 samples open(2952) and live-attaches at 5070 (constant-100 stream tests cannot catch this). |
1. vitest run --project int int/viewport-read.int.test.ts on e9f39d7. 2. First it throws on opens.toContain(100000-2048) while opens is [-1,-1,97959,-1,100007]. 3. Plan T9/T10 and the PR body (“14 passed, including 2 new int rows”) are stale vs HEAD. merge-pr’s default npm test does not run this project, so the red T9 can ship. |
Defender: production GET passes startIndex: 0 for cold and never pre-probes, so the live algorithm still samples from the first in-stream H0. True for app/api/turns/[runId]/stream/route.ts — this is not a current origin-replay bug. Fails as a merge gate: T9 is the locked real-Wasm evidence that G1’s origin skip holds through the production composition (viewportStream + recoverViewport + decoder + ring), and it is red / proving the old caller-H0 shape. Defender: later assertions (no HISTORICAL_THINKING, latest sampled assistant, live text) still pass so the row is “mostly green.” Fails: those still pass because 97959 < H0 keeps the mock on the historical branch; they do not assert the sample origin. |
high |
Residual risk
Cold recovery remains best-effort: a burst during the sample window is skipped (gap:true); one Blob read() materializes a whole ≤8 MiB object before the byte check; blob.read / SDK getInfo are not abort-linked so late work can finish after the deadline (documented in docs/harness-limits.md). getReadable({startIndex:-1}) for H0 is construction-inert until pull (workflow 4.8.4) and getTailIndex is a separate streams.getInfo; cancel-before-pull is the intended metadata probe, not proven against origin Production. getReadable is treated as one stored frame per read() with no parseSseChunk rest-buffer — coalesced/split chunk bytes skip a batch or fatal UTF-8 closes live with STREAM_UNAVAILABLE. stillOwned is only checked before releasing the snapshot, not during the live tail (same TOCTOU as today’s attach). Stored done/error close the iterator without viewport_end (synthetic end is status-only); #960 must treat stream EOF after a stored done as terminal. No production HarnessHost flip — #924 stays open.
Merge guidance
- CONCERNS: do not merge until the Major is fixed — T9 must drive cold recovery the same way the route does (
startIndex: 0, no callernextIndex()), and a unit row must lock sampleopen(H0-2048)vs a later final-probe tail. - Suggested patch: drop the pre-probe in
int/viewport-read.int.test.ts; passstartIndex: 0and letviewportStreamcapture H0. AddviewportStream.test.tscoverage thatnextIndex5000 then 5070 samples from 2952 and live-opens 5070. Re-runvitest run --project intplus the viewport unit files.
What was not attacked
Live Workflows getReadable / getInfo against origin Production; Vercel Blob byte-range behavior; DO runner; Gateway; Wasm paint beyond reading int/viewport-read.int.test.ts (not re-executed here — no node_modules / harness artifact in this review workspace); #960 host wiring.
Adversarial review on PR #963: T9 pre-called nextIndex() then passed that as startIndex, so after H0 moved inside viewportStream the sample window was the later final-probe tail. Drive cold recovery like the route (startIndex 0) and unit-lock open(H0-2048) vs a grown final probe.
|
Follow-up on the sixth adversarial review (CONCERNS): landed in
|
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #963
Verdict: PASS WITH NOTES
Repo: btipling/invincible
Scope: main ← feat/a5-bounded-tail-read · 23 files · opt-in bounded viewport read + snapshot-first SSE (plan #959), including follow-ups through 19308d1
Lenses run: L1, L2, L5, L6, L7, L8 (skip: L3 no host/Wasm/UI flip; L4 no CI/wasm/deploy; L9 no palette/DOM chrome)
AGENTS.md read: yes (also docs/feature-divide.md, SECURITY.md, plan #959, workflow@4.8.4 getReadable/getTailIndex contract)
Prior reviews fixed implicit GET v1 origin replay (81ad3fe), cancelled/failed getReadable (7e35603), H0-before-SSE (669563a), completed 0-delay drain (4e3998d), display-only snapshots when H0 is unknown (e9f39d7), and T9 sampling from the first in-stream H0 (19308d1). Those do not survive re-attack.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Minor | L1+L6 | recoverViewport (lib/sessions/viewportRead.ts) still writes resumeIndex: opts.initialIndex on skipStream: true. That value was never captured from nextIndex() — it is the caller’s inbound guess (cold route always passes 0). viewportStream deletes the property before encode, so today’s HTTP path is safe, but the service returns a guessed cursor and lib/sessions/viewportRead.test.ts skipStream is head-only… never asserts it is absent. Review 5’s invariant lives only in the stream wrapper. |
1. Future caller (JSON compose, #960 helper, another route) uses recoverViewport({ skipStream: true, initialIndex: 0 }) and forwards resumeIndex into a snapshot. 2. ViewportStreamDecoder jumps to 0. 3. Next attach GET …&startIndex=0 is explicit origin replay of historical reasoning_delta — the #924 class GET v1 already 400s when the selector is omitted. |
Defender: only viewportStream calls recoverViewport and it deletes resumeIndex when !resumeKnown. True for this PR’s HTTP path (hydrate cancelled/failed + H0-fail tests pin absence). Fails as a merge-quality lock: the function that “never probes the run” still mints a transport cursor; the unit row that names skipStream does not pin the review-5 invariant at the service that produces the snapshot. |
high |
| Minor | L6 | Plan T5/T9 require a single omission note on a replacing snapshot (VIEWPORT_HISTORY_NOTE). recoverViewport appends it, but no unit, route, or int/viewport-read.int.test.ts row asserts the text. A regression that drops [...rows, note] or the rows.length > 1 replace gate stays green. |
1. Delete the note append in recoverViewport. 2. vitest run + vitest run --project int int/viewport-read.int.test.ts still pass. 3. #960 hydrates a replacing tail with no “earlier omitted” system row. |
Defender: gap / incomplete / historyComplete:false are asserted and are the machine contract; the note is display copy. Partially true for flags. Fails T9’s locked shipping matrix (“omission note”) and leaves the only user-visible omission sentence unpinned. |
high |
Residual risk
Attack did not re-breach C16 cancelled/failed getReadable, implicit GET v1 without a selector, H0-before-viewport_state, completed 0-delay drain, guessed resumeIndex: 0 on the encoded snapshot, or T9’s sample origin. Cold recovery remains best-effort: a burst during the sample window is skipped (gap:true); one Blob read() materializes a whole ≤8 MiB object before the byte check; blob.read / SDK getInfo are not abort-linked so late work can finish after the deadline (documented in docs/harness-limits.md). getReadable({startIndex:-1}) is construction-inert until pull (workflow 4.8.4) and getTailIndex is a separate streams.getInfo; cancel-before-pull is the intended metadata probe. getReadable is treated as one stored frame per read() with no parseSseChunk rest-buffer — coalesced/split chunk bytes skip a batch or fatal UTF-8 closes live with STREAM_UNAVAILABLE. stillOwned is only checked before releasing the snapshot, not during the live tail (same TOCTOU as today’s attach), and is itself 1 s-bounded. Stored done/error close the iterator without viewport_end (synthetic end is status-only); #960 must treat a stored done/error and stream EOF as terminal. Status snapshot hang still has the same 1 s race as bodyForRun before the hang-class gate. No production HarnessHost flip — #924 stays open.
Merge guidance
PASS WITH NOTES: safe to merge from this attack. The two Minors should land in this PR (omit resumeIndex inside recoverViewport when skipStream; pin the omission note in the skipStream/sample unit rows and T9) so the review-5 cursor invariant and T9 matrix are not wrapper-only.
What was not attacked
Live Workflows getReadable / getInfo against origin Production; Vercel Blob byte-range behavior; DO runner; Gateway; Wasm paint beyond reading int/viewport-read.int.test.ts (not re-executed in this sandbox); #960 host wiring.
Adversarial review on PR #963: recoverViewport minted resumeIndex from the caller's inbound guess on skipStream. Omit it at the service so a guessed cursor cannot be encoded. Pin VIEWPORT_HISTORY_NOTE on replacing snapshots (unit + T9).
|
Follow-up on the seventh adversarial review (PASS WITH NOTES): landed in
|
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #963
Verdict: PASS WITH NOTES
Repo: btipling/invincible
Scope: main ← feat/a5-bounded-tail-read · 23 files · opt-in bounded viewport read + snapshot-first SSE (plan #959), HEAD 77cc674
Lenses run: L1, L2, L5, L6, L7, L8 (skip: L3 no host/Wasm/UI flip; L4 no CI/wasm/deploy; L9 no palette/DOM chrome)
AGENTS.md read: yes (also docs/feature-divide.md, SECURITY.md, plan #959, workflow getReadable/getTailIndex contract)
Prior reviews fixed implicit GET v1 origin replay (81ad3fe), cancelled/failed getReadable (7e35603), H0-before-SSE (669563a), completed 0-delay drain (4e3998d), display-only snapshots when H0 is unknown (e9f39d7), T9 sampling from the first in-stream H0 (19308d1), skipStream resumeIndex + omission-note pins (77cc674). Those do not survive re-attack.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Minor | L2 | ViewportStreamDecoder snapshot parse (lib/viewportStreamProtocol.ts · parse) spreads the raw JSON object ({...rest}) and only checks objectRecord(o.carriers). Rows are re-run through parseViewportRow; carriers are not re-run through viewportCarriers / normalizeSessionCwd / sanitizeQueue. Extra keys (workingNotes, personaSnapshot, host-absolute cwd) survive into the decoded record. This PR ships that decoder as the client contract (docs/agent-stream.md). |
1. A buggy encoder or any proxy that can rewrite SSE data: adds carriers: { cwd: "/etc", workingNotes: "…" } (or extra snapshot keys). 2. Decoder returns them. 3. #960 folds snapshot.carriers into the session the way the server-side allowlist promised it would not. |
Defender: only this server encodes; same-origin authenticated SSE; TypeScript erases extras; #960 can sanitize again. True for today's HTTP path. Fails as a merge-quality lock: SECURITY.md says carriers are allowlisted; the encoder honors that and the published parser does not. viewportCarriers() cannot be called as-is on encoded carriers (logicalCwd vs cwd). |
high |
Residual risk
Attack did not re-breach C16 cancelled/failed getReadable, implicit GET v1 without a selector, H0-before-viewport_state, completed 0-delay drain, guessed resumeIndex: 0 on the encoded snapshot, skipStream minting a transport cursor, T9 sample origin, or T9 omission-note pins. Cold recovery remains best-effort: a burst during the sample window is skipped (gap:true); one Blob read() materializes a whole ≤8 MiB object before the byte check; blob.read / SDK getInfo are not abort-linked so late work can finish after the deadline (documented in docs/harness-limits.md). getReadable({startIndex:-1}) is construction-inert until pull (workflow 4.8.4) and getTailIndex is a separate streams.getInfo; cancel-before-pull is the intended metadata probe. Live getReadable is treated as one stored chunk per read() matching the SDK write index; parseViewportEvent rejects multi-block chunks (legacy pipeRunReadable still uses a rest buffer only to detect terminal events on the byte pipe). stillOwned is only checked before releasing the snapshot, not during the live tail (same TOCTOU as today's attach), and is itself 1 s-bounded. Stored done/error close the iterator without viewport_end (synthetic end is status-only); #960 must treat a stored done/error and stream EOF as terminal. Status snapshot hang still has the same 1 s race as bodyForRun before the hang-class gate. JSON GET /api/sessions/:id/viewport does not append VIEWPORT_HISTORY_NOTE (flags only); that surface is not the SSE snapshot. No production HarnessHost flip — #924 stays open.
Merge guidance
PASS WITH NOTES: safe to merge from this attack. The Minor should land in this PR (reconstruct snapshot records from allowlisted fields; re-sanitize encoded carriers on decode) so the SECURITY.md carrier boundary is not encoder-only.
What was not attacked
Live Workflows getReadable / getInfo against origin Production; Vercel Blob byte-range behavior; DO runner; Gateway; Wasm paint beyond reading int/viewport-read.int.test.ts (durable-turn int check on this SHA is green); #960 host wiring.
|
Follow-up on the eighth adversarial review (PASS WITH NOTES): landed in
|
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #963
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← feat/a5-bounded-tail-read · 23 files · opt-in bounded viewport read + snapshot-first SSE (plan #959), including follow-ups through 3c1d1f2
Lenses run: L1, L2, L5, L6, L7, L8 (skip: L3 no host/Wasm/UI flip; L4 no CI/wasm/deploy; L9 no palette/DOM chrome)
AGENTS.md read: yes (also docs/feature-divide.md, SECURITY.md, plan #959)
Prior reviews (81ad3fe … 3c1d1f2) fixed implicit GET v1 origin replay, cancelled/failed getReadable, H0-before-SSE, completed 0-delay drain, guessed resumeIndex:0, T9 sample origin, skipStream cursor minting, and decoder snapshot allowlisting. Those survive re-attack. GET negotiated pre-stream errors stay sanitized. POST does not.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L2+L6 | Negotiated POST /api/turns?viewportVersion=1 still interpolates failClosed(err) (app/api/turns/route.ts outer catch ~826, and the live-guard getRun catch ~389). SECURITY.md’s new viewport section says negotiated stream failures use sanitized codes, not backend connection details. GET v1 already maps throws to {error:'Viewport stream unavailable.'} and unit-locks a planted provider URL/token-private-detail. POST v1 does not. |
1. POST /api/turns?viewportVersion=1 with Accept: text/event-stream. 2. start() (or the in-flight getRun probe) throws Unable to connect to https://workflow.internal/?token=…. 3. Client receives 503 {error:'Unable to start durable turn (fail closed): Unable to connect to https://workflow.internal/?token=…'}. No private, no-store. Legacy POST is unchanged (G4); this is the new negotiated client. route.test.ts covers invalid negotiation 400 and happy-path indexed events; it never asserts a planted SDK throw is redacted on v1. |
Defender: G4 leaves unnegotiated POST errors alone; failClosed is the existing start-failure contract. Fails: the SECURITY.md paragraph is in this diff and names negotiated mode; GET already treated the same leak as Major. Defender: production host is not flipped. Fails: this is the #960 POST; the sanitization test exists only on GET. Defender: viewportStream itself emits in-band viewport_error. Fails: start() throws before new Response(viewportStream(…)), so the HTTP catch is the body. |
high |
Residual risk
Cold recovery remains best-effort: a burst during the sample window is skipped (gap:true); one Blob read() materializes a whole ≤8 MiB object before the byte check; blob.read / SDK getInfo are not abort-linked so late work can finish after the deadline (documented in docs/harness-limits.md). getReadable is treated as one stored frame per read() with no parseSseChunk rest-buffer — coalesced/split chunk bytes skip a batch or fatal UTF-8 closes live with STREAM_UNAVAILABLE. stillOwned is only checked before releasing the snapshot, not during the live tail (same TOCTOU as today’s attach). Status-probe timeout (1 s) then treating the run as non-hang-class is the same race as bodyForRun. nextIndex still constructs getReadable({startIndex:-1}) (inert until pull; getTailIndex is getInfo) and cancels the data side — a world that aborted getInfo on cancel() would fail H0 closed rather than origin-replay. No production HarnessHost flip — #924 stays open.
Merge guidance
- CONCERNS: do not merge until negotiated POST never interpolates
err.message. - Suggested patch: when
indexedViewport, both POSTfailClosedcatches return the same sanitized 503 +private, no-store, no-transformas GET v1. Leave legacy POSTfailClosedstrings unchanged (G4). Add a route row:start()throw with a planted URL/token is 503{error:'Viewport stream unavailable.'}and the secret is absent.
What was not attacked
Live Workflows getReadable / getTailIndex / getInfo against origin Production; Vercel Blob byte-range behavior; DO runner; Gateway; Wasm paint beyond the two int/viewport-read.int.test.ts rows (read as source, not re-executed here); #960 host wiring.
Adversarial review on PR #963: POST ?viewportVersion=1 interpolated failClosed(err) on start()/live-guard throws, leaking SDK connection details the GET v1 path already redacts. Return the same sanitized 503 plus private/no-store; leave legacy POST strings unchanged.
|
Follow-up on the ninth adversarial review (CONCERNS): landed in
|
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #963
Verdict: PASS WITH NOTES
Repo: btipling/invincible
Scope: main ← feat/a5-bounded-tail-read · 23 files · opt-in bounded viewport read + snapshot-first SSE (plan #959), HEAD 54abaf9
Lenses run: L1, L2, L5, L6, L7, L8 (skip: L3 no host/Wasm/UI flip; L4 no CI/wasm/deploy; L9 no palette/DOM chrome)
AGENTS.md read: yes (also docs/feature-divide.md, SECURITY.md, plan #959)
Prior reviews (81ad3fe … 54abaf9) fixed implicit GET v1 origin replay, cancelled/failed getReadable, H0-before-SSE, completed 0-delay drain, guessed resumeIndex:0, T9 sample origin, skipStream cursor minting, decoder snapshot allowlisting, and negotiated POST failClosed interpolation. Those survive re-attack.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Minor | L1+L6 | readViewportHead (lib/sessions/viewportRead.ts) spreads emptyViewport() (gap:true) and never sets gap:false after a successful scoped head parse. recoverViewport overwrites gap from the sample; the JSON route returns the head view as-is. app/api/sessions/[id]/viewport/route.test.ts pins hasEarlier/replace and never gap. |
1. GET /api/sessions/:id/viewport on a 10-row head with no prev. 2. Body is source:'stored_head', hasEarlier:false, gap:true. 3. #960 idle restore treats gap as a stream-sample skip (the field’s meaning on snapshots) and shows omission chrome on every session open, including a one-message session. |
Defender: incomplete:true is unconditional; #960 can key off hasEarlier. Fails as a published flag: gap is the sample-skip bit (start>0 / unread interval / growth), not “view is partial.” hasEarlier:false + gap:true is incoherent. Head-only JSON is the idle path in docs/session-model.md. |
high |
| Minor | L1+L6 | ViewportReducer.apply (lib/sessionViewport.ts) drops skill_attached (parsed/allowlisted, never painted). Plan emits that event at turn start before the model. Cold hydrate=tail prefers a non-empty stream sample over the Blob head (no merge), so a short in-flight turn’s kind-7 row is in the sample window and then discarded. sessionViewport.test.ts parses the event and never asserts reducer output. |
1. /create-plan (or any attach) then a few hundred text_deltas. 2. Refresh GET …/stream?viewportVersion=1&hydrate=tail. 3. Sample includes skill_attached + text; reducer keeps assistant text only. 4. Snapshot replace:true has no skill_attached row; Wasm never shows Skill attached: create-plan. Envelope attachedSkills is often still the previous turn (running PATCH is turn id/status only). |
Defender: plan waived exact history; live post-H0 events still forward skill_attached; Blob head may have the role. Fails for the short-turn case: H0 < 2048 so the event is in the recent window (not “earlier omitted”), and choosing stream_tail throws away a head that might have held the row. Live events after H0 do not replay start-of-turn attaches. |
high |
| Minor | L5 | GET /api/sessions/:id/viewport copies the SSE stream maxDuration = 1800 (app/api/sessions/[id]/viewport/route.ts). Work is a 5 s viewportWait around one Blob read() that is not abort-linked (docs/harness-limits.md). After the JSON 200, a hung blob.read keeps the Node event loop non-empty. |
Authenticated client issues concurrent JSON viewport GETs while Blob GET hangs. Each isolate can remain alive until the 30-minute Function ceiling instead of dying near the 5 s recovery budget. Stream routes need 1800; this route does not. | Defender: hang is a Blob brownout, already listed as residual. Fails: maxDuration is the kill ceiling for this new route; copying 1800 turns a documented late-read into a 30-minute concurrency amplifier. Vercel Node stays up while the event loop has pending work. |
high |
| Nit | L2 | ViewportStreamDecoder snapshot parse (lib/viewportStreamProtocol.ts) accepts any typeof sessionId === 'string'. runId is sanitizeTurnRunId; rows/carriers are re-sanitized. sessionId is not isRedisSafeOpaqueId. |
Hostile SSE block with sessionId that is not Redis-safe. Decoder returns it. #960 that keys restore/PUT off snapshot.sessionId (instead of the request id) adopts a non-opaque id. |
Defender: same-origin encoder; #960 can ignore the field. True today. The eighth-review allowlist lock already treats this parser as the client contract; sessionId is the one identity field left unchecked. |
high |
Residual risk
Attack did not re-breach C16 cancelled/failed getReadable, implicit GET v1 without a selector, H0-before-viewport_state, completed 0-delay drain, guessed resumeIndex: 0, skipStream cursor minting, T9 sample origin, decoder extra-key/cwd allowlisting, or negotiated POST failClosed interpolation. Cold recovery remains best-effort: a burst during the sample window is skipped (gap:true); one Blob read() materializes a whole ≤8 MiB object before the byte check; blob.read / SDK getInfo are not abort-linked so late work can finish after the deadline (documented in docs/harness-limits.md). getReadable is treated as one stored frame per read() with no parseSseChunk rest-buffer — coalesced/split chunk bytes skip a batch or fatal UTF-8 closes live with STREAM_UNAVAILABLE. stillOwned runs only on the cold path before releasing the snapshot, not during the live tail (same TOCTOU as today’s attach), and is itself 1 s-bounded. Stored done/error close the iterator without viewport_end; #960 must treat those and stream EOF as terminal. Status-probe timeout (1 s) then treating the run as non-hang-class is the same race as bodyForRun. Explicit GET startIndex=0 is still origin replay (documented, not the default). No production HarnessHost flip — #924 stays open.
Merge guidance
PASS WITH NOTES: safe to merge from this attack. The Minors should land in this PR (JSON/readViewportHead gap:false on a successful scoped parse; paint skill_attached in ViewportReducer; JSON viewport maxDuration near the 5 s budget, not 1800) so #960 does not inherit a lying gap flag, missing kind-7 rows on short turns, or a 30-minute Function pin. The Nit is a one-line decoder bind.
What was not attacked
Live Workflows getReadable / getTailIndex / getInfo against origin Production; Vercel Blob byte-range behavior; DO runner; Gateway; Wasm paint beyond reading int/viewport-read.int.test.ts (not re-executed here); #960 host wiring.
Adversarial review on PR #963: successful Blob heads inherited emptyViewport's gap:true; ViewportReducer dropped start-of-turn skill_attached; JSON viewport copied the SSE 1800s maxDuration. Decoder now requires a Redis-safe snapshot sessionId.
|
Follow-up on the tenth adversarial review (PASS WITH NOTES): landed in
|
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #963
Verdict: PASS WITH NOTES
Repo: btipling/invincible
Scope: main ← feat/a5-bounded-tail-read @ d5debb6 · 23 files · opt-in bounded viewport read + snapshot-first SSE (plan #959), eleventh pass
Lenses run: L1, L2, L5, L6, L7, L8 (skip: L3 no production host/Wasm/UI flip; L4 no CI/wasm/deploy files; L9 no palette/DOM chrome)
AGENTS.md read: yes (also docs/feature-divide.md, SECURITY.md, plan #959)
Prior reviews (81ad3fe … d5debb6) fixed implicit GET v1 origin replay, cancelled/failed getReadable, H0-before-SSE, completed 0-delay drain, guessed resumeIndex:0, T9 sample origin, skipStream cursor minting, decoder snapshot allowlisting, negotiated POST failClosed interpolation, JSON gap:false on a successful head, skill_attached paint, JSON maxDuration, and Redis-safe snapshot sessionId. Those survive re-attack at d5debb6.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Minor | L8+L1 | Living protocol docs (docs/agent-stream.md record table) never say a stored done/error closes the iterator with no viewport_end. Code in lib/agent/viewportStream.ts · live loop if (event?.type === 'done' || event?.type === 'error') return. EOF without those events does synthesize viewport_end from run.status. |
1. Negotiated POST/GET; producer writes a normal done chunk. 2. Server yields turn_event{done} and closes. 3. #960 (or any EventSource-style consumer) waits for viewport_end to clear Busy/Stop. 4. Stream is already EOF; Busy hangs until Function maxDuration 1800s or abort. The first viewportStream.test.ts row already ends on turn_event only; the completed-drain row ends on viewport_end because it never stored done. |
Defender: plan said keep stored done/error distinct from synthetic status; int consumer already breaks on reader.done. True, and merging them would lie about cancelled (error: Request cancelled. ≠ failed). The hole is the living contract: #960 has no in-tree sentence that EOF + done/error are terminal. Residual of earlier passes never promoted this to a finding. |
high |
Residual risk
Attack did not re-breach C16 cancelled/failed getReadable, implicit GET v1 without a selector, H0-before-viewport_state, completed 0-delay drain, guessed resumeIndex: 0, skipStream cursor minting, T9 sample origin, decoder extra-key/cwd/sessionId allowlisting, negotiated POST failClosed interpolation, JSON gap, skill-row paint, or JSON maxDuration. Cold recovery remains best-effort: a burst during the sample window is skipped (gap:true); one Blob read() materializes a whole ≤8 MiB object before the byte check; blob.read / SDK getInfo are not abort-linked so late work can finish after the deadline (documented in docs/harness-limits.md). getReadable is treated as one stored frame per read() with no parseSseChunk rest-buffer — consistent with Workflows 4.8.4 chunk/startIndex semantics (one write() of formatTurnSse per index) but a world that coalesced/split bytes would skip a batch or fatal UTF-8-close live with STREAM_UNAVAILABLE. stillOwned runs only on the cold path before releasing the snapshot, not during the live tail (same TOCTOU as today’s attach), and is itself 1 s-bounded. Status-probe timeout (1 s) then treating the run as non-hang-class is the same race as bodyForRun; envelope turnStatus other than cancelling is collapsed to running for cold.status and only matters when that probe times out. Explicit GET startIndex=0 is still origin replay (documented, not the default). createViewportRunReader.nextIndex still constructs getReadable({startIndex:-1}) (SDK: last one chunk) and cancels the data side; getTailIndex is getStreamInfo. No production HarnessHost flip — #924 stays open.
Merge guidance
PASS WITH NOTES: safe to merge from this attack. Land the Minor in this PR so #960 does not inherit an unstated close rule: document that stored done/error are producer-terminal and do not emit viewport_end; consumers must treat those events, viewport_end, viewport_error, and reader EOF as terminal. Do not synthesize viewport_end after done (cancelled inject is an error event, not failed).
What was not attacked
Live Workflows getReadable / getTailIndex / getInfo against origin Production; Vercel Blob byte-range behavior; DO runner; Gateway; Wasm paint beyond reading int/viewport-read.int.test.ts (not re-executed here; int-durable is green on this SHA); #960 host wiring.
Adversarial review on PR #963: living protocol docs did not say a stored done/error closes the iterator with no viewport_end, so a #960 consumer waiting on viewport_end would hang Busy after a normal turn. Document the close set (done/error, viewport_end, viewport_error, reader EOF) and unit-lock that error/done do not synthesize viewport_end.
|
Follow-up on the adversarial review (PASS WITH NOTES): landed in
|
1 participant
Summary
Adds the opt-in, read-only, bounded recent-viewport backend from plan #959:
a disposable partial view (never a full transcript or model seed), a bounded
recent tail + latest Blob head-only recovery, an explicit resume position,
and a snapshot-first live stream with indexed event records.
Status: HANDOFF-READY phase 1. The current production host still uses the
unnegotiated legacy stream; switching it and closing #924 belongs to #960.
What ships
lib/sessions/viewportRead.ts,lib/workflows/viewportRunReader.ts)2048recent stored frames,8 MiBdecoded bytes,5 soptional work, with one final
1 stail probe. No origin replay, noancestry/
prevreconstruction, no retry chase; overload omits history andresumes at a known raw index with
gap/incompleteflags.readViewportHeadreads only the current scoped Blob head (≥0, ≤1transcriptPointer), never foreign bodies, and makes no writes.lib/sessionViewport.ts) — allowlisted event/role parsing,bounded recent rows, stripped sampling reasoning, visible excerpt markers,
new carrier allowlist (no persona/notes/model/compaction bodies or secrets),
and whole-JSON
2 MiB/2048fitting. Missing prompt/earlier rows areoptional-history misses, not errors.
lib/viewportStreamProtocol.ts) — incrementalUTF-8/CRLF SSE codec;
viewport_state→ oneviewport_snapshot→ indexedturn_eventrecords; syntheticviewport_end/viewport_errornever consumea stored raw index. Known malformed frames skip at their index; a lost
position closes instead of guessing.
GET /api/turns/:runId/stream?sessionId=:id&viewportVersion=1&hydrate=tail(cold) and
…&viewportVersion=1&startIndex=N(hot indexed) — one response,no second cold GET. Default/legacy route bytes are unchanged.
POST /api/turns?viewportVersion=1— indexed events for new runs withoutchanging inference/start args.
GET /api/sessions/:id/viewport— head-only JSON display view.the stream route's tenancy gate (
404/401); a scopedisObjectIdBoundTocheck precedes any read; ownership is rechecked before releasing a recovered
snapshot. No inference/tool execution, run start/cancel or session writes in
recovery. Negotiated pre-stream errors return sanitized
503and never leakSDK details.
historyComplete:falseis unconditional.Tests
npm test(default + tenancy): 3,581 passed, including the existing POSTturns suite (70) and new negotiated-route, auth/foreign, streaming
cleanup/budget, SDK-failure and codec/reducer/fitting suites.
npm run test:int(real Wasm): 14 passed, including 2 newint/viewport-read.int.test.tsrows that run the production read service →real parser → real Wasm ring: historical reasoning is never painted,
sampled latest rows survive, and post-handoff live reasoning/text arrive with
a strict absolute cursor.
npm run typecheckandnpm run buildpass; DI/drizzle gates andgit diff --checkpass.Docs
docs/session-model.md,docs/agent-stream.md,docs/harness-limits.md,SECURITY.mdandAGENTS.mdnow document the bounded read path, negotiatedrecord grammar, recovery budget caveats, and security boundary. README/native
protocol/env remain untouched (no production host flip or new secret/export in
this phase).
Performance / resource notes
all-chain/all-history retained payloads, no global exact merge.
VIEWPORT_RECOVERY_MAX_BYTEScounts decoded sample bytes, includingreasoning that is immediately discarded.
VIEWPORT_RESPONSE_MAX_BYTESbounds one escaped snapshot/control payload;the long-lived live SSE stream may exceed it over time (unchanged).
Not included
HarnessHostcold-policy flip,prefixThroughLastUserremoval,Busy/Stop wiring and the bug: F5 / reload fast-forward replays entire durable stream (need latest-page hydrate, no thinking replay) #924 close gate (phase 2 / plan: A5 viewport — phase 2 — usable tail-first F5 and real-Wasm #924 close gate #960).
best-effort scope (plan: A5 viewport — phase 1 — bounded best-effort tail read and snapshot-first attach #959 review notes).
caps or inference selection.
Plan / issue links
Fixes #959 · Refs #958 #553 #924 (does not close #924 before phase 2).