feat(harness): turn errors retry the current turn — never drain the queue (plan #759) - #774
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 #774
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← plan/turn-retry-never-drain-queue · 17 files · protocol v19 inv_queued_insert_front + turn retry (plan #759)
HEAD: 4e859e42010eca9eefab922fdce756e388ce168d
Lenses run: L1, L2, L3, L4, L6, L8 (skip: L5 no new poll/history; L7 no clone bind; L9 no palette)
AGENTS.md read: yes · docs/feature-divide.md not changed (host still owns /api/agent + lifecycle; Wasm still owns the FIFO) — divide holds
The shape is right: host is the sole outcome observer, withTransientRetry + injectable classify leaves sandbox callers on classifyVercelError, Stop is permanent / Ready / no Continue, give-up lands Lifecycle.Error so ui.zig terminal = cur_lc == .ready cannot drain, insertFront fails closed at 16, export is in build.zig export_symbol_names, build-harness green, JSON-path tests cover attempts/Continue/full/Stop/401. Feature-divide one-liner: a multi-turn chat still completes from the Wasm composer; the host only arms lifecycle + unshifts Continue.
Two independent holes in the new seams.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1 | withTransientRetry wraps the live sendAgentStream call with the same onStreamEvent closure. assistantAcc / assistantStarted / live tool cards / next.attachedSlugs are not reset between attempts. Production default is streamAgent === true. Every new test forces streamAgent: false. |
Attempt 1 streams tool_start + text_delta, then a retryable 5xx/timeout/error event. Attempts 2–5 POST the same prompt again: tools re-run (writes/exec duplicate), growAssistant pushes another bubble, skill rows restamp. Canvas and sandbox diverge from “retry the current turn”. Fail-closed: if any tool_start (or any ring mutation past the user line) already painted, classify remaining failures permanent — or snapshot/reset the stream accumulators before attempt N+1. |
Defender: “goal 2 is retry the same prompt; JSON tests prove 5 attempts.” Survives: JSON never paints live. The issue’s production path is SSE. Defender: “the plan wanted 5×.” Retrying a turn is not the same as replaying a half-painted stream onto the same ring. | high |
| Major | L1 | inv_queued_insert_front → submit_queue.insertFront shifts every slot but does not touch state.queue_editing_index. queue_band.removeAt already does if (e > i) e - 1 for the same class of shift. Host give-up can fire while Busy-queue-edit is open (that is the queue’s purpose). |
Queue [A, B], operator is editing B (index = 1). Turn gives up → Continue unshifted → [Continue, A, B]. Editor is still index 1 (now A). Blur/Ctrl+Enter saveEdit overwrites A with B’s buffer; B’s unsaved text is gone. Fix: on successful insert-front, if queue_editing_index != null increment it (or cancel the edit). Skip insert while editing is the fail-closed alternative. |
Defender: “who edits during a failing turn?” The FIFO exists specifically to edit follow-ups while Busy; backoff is ~seconds (250 doubling to 4000). Defender: “CloseEdit on empty FIFO.” Count went up, not down — the latch stays. |
high |
| Minor | L6 | Plan tests 1/2/3/5/6/10 all run through sendAgent + streamAgent: false. No stream-retry case, no insert-front-while-editing case. The two Majors above are exactly the untested branches. |
CI stays 1963/1963 while the production SSE path duplicates tools. | Defender: “wasm-int proves the export.” It proves the symbol exists, not the retry×stream interaction. | high |
| Nit | L8 | docs/harness-limits.md Permanent-failures row claims validation gets Continue-if-queued. runHarnessTurn validatePrompt early-return still setLifecycle(Ready) and never calls insertContinueTurnPrompt. Client-side validation is pre-Busy, so it does not drain — the Continue claim is still wrong. runHarnessChat header comment still says “soft API errors leave ready for retry.” |
Docs/code drift only. | Defender: “validation never starts a turn.” Then don’t document it as the give-up+Continue path. | high |
Residual risk
Sibling #763 (plan #760) also ships protocol v19 with a different REQUIRED export (inv_set_queue_promote_allowed). Whichever merges second must take v20 and keep both exports in REQUIRED_FNS / export_symbol_names — the plan said this; the PR bodies have not sequenced it. This PR alone still auto-drains the queue (incl. a parked Continue) on a later successful Ready after Stop — that is goal 5 as locked, not a defect. insertFront’s 256 KiB stack scratch matches pre-existing replaceAt; not new. Live operator smoke of a streamed 5xx-after-tools was not run in this attack.
Merge guidance
CONCERNS: do not merge until the stream-retry accumulator is isolated (or retries are suppressed once the stream has painted) and insert-front adjusts or cancels queue_editing_index. The L6 stream test should land in the same pass. Protocol-union with #763 is a merge-time must, not a reason to land this first unchecked.
What was not attacked
Live DO runner beyond green build-harness; prod Gateway tool replay; in-browser Stop-during-backoff smoke; thinking-collapse on busy→err (existing unit already covers that transition).
…ront re-anchors an open queue edit (PR #774 round-1 review)
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #774 (round 2)
Verdict: PASS WITH NOTES
Repo: btipling/invincible
Scope: main ← plan/turn-retry-never-drain-queue · 20 files · protocol v19 inv_queued_insert_front + turn retry (plan #759)
HEAD: 8c61c1fc9b18073cf350391b5d7a8c319f6f276c (round-1 follow-up on 4e859e4)
Lenses run: L1, L2, L3, L4, L6, L8 (skip: L5 no new poll/history; L7 no clone bind; L9 no palette)
AGENTS.md read: yes · docs/feature-divide.md not changed (host owns /api/agent + lifecycle; Wasm owns the FIFO)
Round 1 (CONCERNS on 4e859e4) asked for two Majors, an L6 Minor, and a docs Nit. This head landed all four:
- Stream replay:
streamPaintedis armed on every ring-painting SSE type (tool_start/tool_result/reasoning_delta/text_delta/skill_attached).withTransientRetry’sclassifyis thenstreamPainted ? permanent : classifyTurnRetry.sendAgentStreamawaitsonEventbefore returning, so the flag is set before classify. JSON path never firesonStreamEvent— 5× loop unchanged there. Tests: painted 500 → 1 send, one tool + one assistant; unpainted 503 → 5 sends. - Edit re-anchor:
insertQueuedFrontarmshas_pending_front_insert;queue_band.reconcileFrontInsert(called fromui.frameafter the empty-queue drop) consumes it and bumpsqueue_editing_indexby one. Failed insert does not arm.reset/inv_clear_messagesclear the latch. Zig tests cover bump / no-op / consume / reset. - Docs: Permanent-failures row no longer claims client-side validation inserts Continue.
Give-up still lands Lifecycle.Error so terminal = cur_lc == .ready cannot drain. Stop stays Ready / no Continue / no retry. insertFront fails closed at 16. Export is in export_symbol_names. build-harness green on this SHA (workflow_dispatch 32548049169). Feature-divide holds.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Nit | L1 | Front-insert latch is a boolean, not a count. reconcileFrontInsert bumps +1 once per consumed signal. |
Two successful insertQueuedFront calls before the next Wasm frame would shift twice and bump once — saveEdit would still land on the neighbor. Production give-up inserts Continue once. No second caller today. A counter (or bump-by-queuedCount delta) would close it if a second host unshift appears. |
Defender: “one Continue per give-up.” Correct for this PR. | high |
Residual risk
GitHub mergeable_state: dirty — this branch conflicts with main (#773 Ctrl+I landed; likely docs/harness-limits.md + .gitignore .invincible). Rebase before merge; not a logic hole in the retry/FIFO seams. Sibling #763 is still open and also ships protocol v19 with a different REQUIRED export (inv_set_queue_promote_allowed). Whichever merges second must take v20 and keep both exports. This PR alone still auto-drains the queue (incl. a parked Continue) on a later successful Ready after Stop — that is goal 5 as locked. usage SSE does not arm streamPainted (status-slot only, no ring paint) — a usage-then-5xx still retries; acceptable. Live operator smoke of a streamed 5xx-after-tools was not run in this attack.
Merge guidance
PASS WITH NOTES. Round-1 Majors are fixed; this is safe to merge after a rebase onto main and the protocol-union with #763 (v20 + both exports if that PR lands first). The leftover Nit is a second-unshift counter, not a drain/security hole.
What was not attacked
Live DO runner beyond green build-harness; prod Gateway tool replay; in-browser Stop-during-backoff; thinking-collapse on busy→err (existing unit already covers that transition).
8c61c1f to
a797a1f
Compare
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #774 (round 3)
Verdict: PASS WITH NOTES
Repo: btipling/invincible
Scope: main ← plan/turn-retry-never-drain-queue · 19 files · protocol v20 inv_queued_insert_front + turn retry (plan #759); sibling #760 already on main as v19
HEAD: a797a1f20c4757e268b941b9f0d6736d4ff4019e
Lenses run: L1, L2, L3, L4, L6, L8 (skip: L5 no new poll/history growth; L7 no clone bind; L9 no palette)
AGENTS.md read: yes · docs/feature-divide.md not changed (host still owns /api/agent + lifecycle; Wasm still owns the FIFO) — divide holds
Round 1 (CONCERNS) required stream-retry isolation and insert-front edit re-anchor. Round 2 (PASS WITH NOTES on 8c61c1f) landed those. This head is the protocol-union rebase: #760 took v19, this sibling is v20 with both REQUIRED exports. Re-traced the production seams on this SHA, not the prior comments.
The shape still holds: withTransientRetry + additive classify (sandbox default untouched), streamPainted fail-closes retries once SSE has painted past the user line, give-up lands Lifecycle.Error so shouldAutoPromote (ready-only) cannot drain, Stop stays Ready / setQueuePromoteAllowed(false) / no Continue, insertFront fails closed at 16, export is in build.zig export_symbol_names and REQUIRED_FNS next to inv_set_queue_promote_allowed. Feature-divide one-liner: a multi-turn chat still completes from the Wasm composer; the host only arms lifecycle + unshifts Continue.
No Blocker or Major survived self-refutation on this SHA.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Minor | L8 | native/harness/README.md still documents protocol 19 and labels inv_queued_insert_front as v19. This PR edited those exact rows on the union rebase and left them one version behind the code. inv_set_queue_promote_allowed is still absent from the export table. |
Operator / next protocol bump treats README as the contract: HARNESS_PROTOCOL_VERSION / PROTOCOL_VERSION / wasm-int are 20 (lib/harnessBridge.ts · bridge.zig). Shipping a “fix README” wasm at 19 against this host fails closed with Harness protocol mismatch: wasm=19 host=20. Inverse: someone “fixes” the host down to 19 and drops the v19 promote-gate export. |
Defender: “AGENTS.md + REQUIRED_FNS + wasm-int pin v20; README is secondary.” Survives: this file is the crate’s protocol table, this PR is the one that rewrote the version line, and the union rule the plan locked is exactly “don’t lose the other export / don’t collide versions.” | high |
| Nit | L8 | docs/harness-limits.md Submit queue → When it runs says “protocol v20 promote gate.” The gate is v19 (inv_set_queue_promote_allowed); v20 is insert-at-front. |
Same class of split-brain as the README, bounded to one cell. | Defender: “v20 is the current protocol.” The gate did not move. | high |
| Nit | L1 | Front-insert latch is still a boolean, not a count (has_pending_front_insert / reconcileFrontInsert bumps +1 once). |
Two successful insertQueuedFront calls before the next Wasm frame would shift twice and bump once — saveEdit lands on the neighbor. Production give-up inserts Continue once. No second caller on this SHA. |
Defender: “one Continue per give-up.” Correct for this PR. | high |
| Nit | L8 | AGENTS.md protocol row + docs/agent-stream.md Error row claim permanent “401/403/validation/4xx”. PERMANENT_TURN_STATUS is a whitelist (400,401,403,404,413,422); 408/429 retry 5× (and the new 408 test asserts that). |
A 408 Gateway timeout is retried five times while the living docs say “4xx → single attempt.” | Defender: “the code comment lists 408/429 as retryable.” Then don’t write “4xx” in AGENTS / agent-stream. | high |
Residual risk
streamPainted is armed on tool_start / tool_result / reasoning_delta / text_delta / skill_attached only. A done event can paint via finalizeAssistant without flipping the flag; sendAgentStream still awaits onEvent before returning, and a done with text normally returns ok: true (no retry). A done-then-error SSE pair with no prior deltas would duplicate the assistant bubble — not a documented server sequence. JSON (streamAgent: false) still retries a tool-running turn; production default streams; that re-run is the existing mid-tool-cancel product rule. Later successful Ready still auto-promotes a parked Continue (goal 5, locked). usage SSE still does not arm streamPainted. Live operator smoke of a streamed 5xx-after-tools was not run in this attack.
Merge guidance
PASS WITH NOTES. Safe to merge from this attack; fix the README protocol row (v20 + both exports) in this PR or a fast follow. The leftover Nits are docs/latch, not drain/security holes. Do not --approve from this skill.
What was not attacked
Live DO runner beyond green zig → harness.wasm on this SHA (32554889823); prod Gateway tool replay; in-browser Stop-during-backoff; thinking-collapse on busy→err (existing unit already covers that transition); full vitest run was not re-executed in this review workspace (traced tests + claimed 1963/1963 in the PR body). GitHub check-runs on the head are build-harness + ignored Vercel only.
Plan to address feedback on PR #774 (Status: DONE)Last updated: 2026-08-22 Feedback items (most complex → least)
#1 —
|
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #774 (round 4)
Verdict: PASS WITH NOTES
Repo: btipling/invincible
Scope: main ← plan/turn-retry-never-drain-queue · 19 files · protocol v20 inv_queued_insert_front + turn retry (plan #759)
HEAD: 05bfbe32e47ed598c1556989850b0f52a77999ba
Lenses run: L1, L2, L3, L4, L6, L8 (skip: L5 no new poll/history growth; L7 no clone bind; L9 no palette)
AGENTS.md read: yes · docs/feature-divide.md not changed (host still owns /api/agent + lifecycle; Wasm still owns the FIFO) — divide holds
Round 3 (PASS WITH NOTES on a797a1f) asked for README v20 + both exports, the promote-gate cell to say v19, and the 4xx → whitelist docs. 05bfbe3 is docs-only and lands those three. Re-traced the production seams on this SHA, not the prior comments.
The shape still holds: withTransientRetry + additive classify (default still classifyVercelError; sandbox callers untouched), streamPainted fail-closes retries once SSE has painted past the user line, give-up lands Lifecycle.Error so shouldAutoPromote (cur == .ready only) cannot drain, Stop stays Ready / setQueuePromoteAllowed(false) / no Continue, insertFront fails closed at 16 / blank, both REQUIRED exports are in build.zig export_symbol_names and REQUIRED_FNS. Protocol is 20 on TS + Zig + README + wasm-int. Feature-divide one-liner: a multi-turn chat still completes from the Wasm composer; the host only arms lifecycle + unshifts Continue.
Round-3 Minor + two docs Nits are gone. No Blocker or Major survived self-refutation on this SHA.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Nit | L1 | Front-insert latch is still a boolean, not a count (has_pending_front_insert / reconcileFrontInsert bumps +1 once). Carried from round 3; author deferred. |
Two successful insertQueuedFront calls before the next Wasm frame would shift twice and bump once — saveEdit lands on the neighbor. Production give-up inserts Continue once. No second caller on this SHA. |
Defender: “one Continue per give-up.” Correct for this PR. Stays a latch, not a drain. | high |
| Nit | L8 | Living docs this SHA just rewrote (docs/harness-limits.md Turn-error recovery + Permanent failures; docs/agent-stream.md Error row; AGENTS.md v20 cell) still say a retryable 5xx/timeout retries 5×. streamPainted makes any post-paint failure a single attempt (harnessChat.ts classify gate; covered by the live-stream test). |
502 after the first reasoning_delta / text_delta / tool_start / skill_attached → 1 send, give-up + Continue. Operator reading the table expects four more tries. Queue is not drained. |
Defender: “fail-closed after paint is the more important lock; Continue still appears.” Then say so in the rows that claim “5 attempts.” | high |
Residual risk
streamPainted is armed on tool_start / tool_result / reasoning_delta / text_delta / skill_attached only. A done event can paint via finalizeAssistant without flipping the flag; sendAgentStream still awaits onEvent before returning, and a done with text normally returns ok: true (no retry). JSON (streamAgent: false) still retries a tool-running turn; production default streams. usage SSE still does not arm streamPainted. A session that emits skill_attached at turn start will skip retry of a later 5xx (duplicate skill row would be worse). insertFront still stack-allocates a 256 KiB scratch; wasm-int insert of a short Continue succeeded, so the default Wasm stack held it. Live operator smoke of Stop-during-backoff was not run here.
Merge guidance
PASS WITH NOTES. Safe to merge from this attack. Round-3 docs holes are closed. The leftover Nits are latch + a retry-count caveat, not drain/security holes. Do not --approve from this skill.
What was not attacked
Live DO runner beyond green zig → harness.wasm on this SHA (32558406080); prod Gateway tool replay; in-browser Stop-during-backoff; thinking-collapse on busy→err (existing unit already covers that transition); full vitest run was not re-executed in this review workspace (traced tests + claimed 1963/1963 in the PR body). GitHub check-runs on the head are build-harness + ignored Vercel only.
Round-4 L8 Nit: living docs claimed a retryable 5xx/timeout retries 5x, but streamPainted makes any post-paint failure a single attempt (harnessChat.ts classify gate). Adds the 1-attempt carve-out to harness-limits Turn-error recovery + Permanent failures, agent-stream error row + Error/timeout/empty row, and AGENTS.md v20 cell.
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #774 (round 5)
Verdict: PASS WITH NOTES
Repo: btipling/invincible
Scope: main ← plan/turn-retry-never-drain-queue · 19 files · protocol v20 inv_queued_insert_front + turn retry (plan #759)
HEAD: 67c001f82038fff6eb5ff4db83b1a3b01f58fa81
Lenses run: L1, L2, L3, L4, L6, L8 (skip: L5 no new poll/history growth; L7 no clone bind; L9 no palette)
AGENTS.md read: yes · docs/feature-divide.md not changed (host still owns /api/agent + lifecycle; Wasm still owns the FIFO) — divide holds
Round 4 (PASS WITH NOTES on 05bfbe3) left two Nits: the boolean front-insert latch, and living docs claiming retryable 5xx retries 5× while streamPainted makes post-paint failures a single attempt. 67c001f is docs-only and lands the second: docs/harness-limits.md Turn-error recovery + Permanent failures, docs/agent-stream.md SSE error + Error/timeout/empty, and the AGENTS.md v20 cell all state 1 attempt once a ring row has been painted. Re-traced the production seams on this SHA.
The shape still holds: withTransientRetry + additive classify (default still classifyVercelError), streamPainted fail-closes retries once SSE has painted past the user line, give-up lands Lifecycle.Error so shouldAutoPromote (cur == .ready only) cannot drain, Stop stays Ready / setQueuePromoteAllowed(false) / no Continue, insertFront fails closed at 16 / blank, both REQUIRED exports are in build.zig export_symbol_names and REQUIRED_FNS. Protocol is 20 on TS + Zig + README + wasm-int. Feature-divide one-liner: a multi-turn chat still completes from the Wasm composer; the host only arms lifecycle + unshifts Continue.
Round-4 docs Nit is gone. No Blocker or Major survived self-refutation on this SHA.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Nit | L1 | Front-insert latch is still a boolean, not a count (has_pending_front_insert / reconcileFrontInsert bumps +1 once). Carried from round 3; author deferred. |
Two successful insertQueuedFront calls before the next Wasm frame would shift twice and bump once — saveEdit lands on the neighbor. Production give-up inserts Continue once. No second caller on this SHA. |
Defender: “one Continue per give-up.” Correct for this PR. Stays a latch, not a drain. | high |
Residual risk
streamPainted is armed on tool_start / tool_result / reasoning_delta / text_delta / skill_attached only — now disclosed in living docs. Production SSE almost always paints (skill row at start, then thinking/tools), so the 5-attempt loop is the clean (never-painted) path; post-paint 5xx is single-attempt give-up + Continue. That is fail-closed against duplicate tool/bubble replay (#756 Goal 2’s “same prompt 5×” is the unpainted case). Follow-up for “continue the failed work from the composer without consuming the queue” is #776 (pause), not this PR. JSON (streamAgent: false) still retries a tool-running turn; production default streams. usage SSE still does not arm streamPainted. insertFront still stack-allocates a 256 KiB scratch; wasm-int insert of a short Continue succeeded. Live operator smoke of Stop-during-backoff was not run here.
Merge guidance
PASS WITH NOTES. Safe to merge from this attack. Round-4 docs hole is closed. The leftover Nit is the deferred boolean latch, not a drain/security hole. Do not --approve from this skill.
What was not attacked
Live DO runner — zig → harness.wasm is pending on this SHA (32559474648; native unchanged vs the previously green head). Prod Gateway tool replay; in-browser Stop-during-backoff; thinking-collapse on busy→err; full vitest run was not re-executed in this review workspace (traced tests + claimed 1963/1963 in the PR body). GitHub check-runs otherwise ignored Vercel only.
1 participant
Closes #759
A turn error no longer looks like a finished turn: the host retries the SAME prompt up to
TURN_RETRY_ATTEMPTS=5 against a retryable error with bounded exponential backoff, stays Busy the whole window, then lands the turn on Error — so the Wasm promote gate (Ready-only) never drains a queued operator item. On give-up with a non-empty queue it insertsContinue the current turnas the new head (never when full).Host (TS) —
lib/harnessChat.tswraps the /api/agent send inwithTransientRetryvia an additiveclassifyseam (lib/sandbox/resilience.ts, defaultclassifyVercelError, sandbox untouched); narrowclassifyTurnRetry= permanent on stop/401/403/other 4xx, retryable on timeout/empty/5xx/network. Give-up setsLifecycle.Error(Stop keeps Ready); Continue-insert fail-closed atHARNESS_QUEUE_MAX_ITEMS=16.Wasm (Zig) —
submit_queue.insertFront,bridge.insertQueuedFront+inv_queued_insert_front, listed inbuild.zig export_symbol_names;ui.zigpromote gate is nowterminal = cur_lc == .ready(err is no longer terminal). Protocol v18→v19 with the REQUIRED export (note: sibling #760 also bumps to v19 — per the plan's union rule the second merged takes v20).Tests: 8 new/updated host retry cases (plan tests 1,2,3,5,6,10 + full-queue fail-closed), classifier-seam back-compat in
resilience.test.ts(test 11),queuedInsertFrontbridge rows + REQUIRED (test 12), 3 newwasm-intv19 rows (artifact proves the export is rooted), and 5 newsubmit_queueZig tests incl. wrap-around + promote-after-insert.Gates green:
npm run typecheck,node scripts/di-gate.mjs, fullvitest run(1963/1963, 126 files incl. wasm-int),npm run build,zig build harness -Doptimize=Debug,zig build test-rich,zig build test-rich-invariants,zig fmt --check. The 4dvui_testinglayout "failed command" lines are the pre-existing missing-font artifacts (byte-identical on clean main).