feat(harness): queue follow-up prompts while Busy - #666
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Opened from plan-review + implement-plan on #664.
Suggested next: adversarial-review on this PR. |
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #666
Verdict: BLOCK
Repo: btipling/invincible
Scope: main ← plan/queue-while-busy @ ed4ca8d · 21 files · Wasm submit-queue (plan #664) + protocol v18
Lenses run: L1, L3, L4, L5, L6, L8, L9 (skip: L2 no new trust/secret/runner surface; L7 no host/config bind)
AGENTS.md read: yes (docs/feature-divide.md also)
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Blocker | L1+L4 | native/harness/src/bridge.zig calls methods on submit_queue.Q (queue.count() / .item / .clear / .removeAt / .replaceAt / .push) but those symbols are module-level fns (pub fn count(q: *const Q) in submit_queue.zig:27-100), not decls on Q. Zig 0.16 will not bind them. enqueueFromUi (:240) is the same bug. |
Open build-harness #32104858730: error: no field or member function named 'count' in 'submit_queue.Q' (10 errors). Merge as-is → next main wasm job is red → no harness-wasm artifact → SHA wait / deploy stall. |
“zig build test-rich exercises the queue.” True and misleading: tests call sq.count(&q) (namespace). The wasm compile is a different call style and is already failed. Moving the fns into Q or writing submit_queue.count(&queue) everywhere is the fix — not “CI flake.” |
high |
| Blocker | L4 | GitHub reports mergeable=false / DIRTY. PR parent is ecc08ea; main has since landed #662 (325e966). AGENTS.md (protocol-ownership row) conflicts. |
“Merge” button is blocked. Even after the compile fix, a merge commit without rebase will fail. | “Rebase is cleanup, not a review finding.” False: this skill treats a confirmed conflict + red required check as merge-blocking. Same class as #662’s prior BLOCK. | high |
| Minor | L9 | composer.submitOrEnqueue (ui/composer.zig:32) catch returns enqueueFromUi Full with no in-canvas error. Composer text is kept (good) but ▶ / Ctrl+Enter is a silent no-op at 16 items. |
Fill 16 follow-ups while Busy, type a 17th, press ▶. Clipboard/composer unchanged, no ember/muted “queue full”, operator retries into the void. | “MAX_ITEMS=16 is generous; they will notice the list.” The band shows “16 queued” but does not say the click failed. Plan locked the cap, not the mute. | high |
Residual risk
After the call-style compile fix + rebase: tryPromoteQueued + promoteIf (peek → queueSubmitFromUi → pop only if has_pending_submit) matches the plan-review lock; Trigger A (busy→ready/err) + Trigger B (queue_closed_edit when terminal) is wired; Stop (queueCancelFromUi) does not clear the FIFO; New/Clear (inv_clear_messages) does. canAutoContinue is intentionally unwired. Left on the table: 4 MiB BSS (16 × 262144 slots) + another SUBMIT_CAP edit buf; queue-row editor is single-line; Full is silent (above). test-rich passing is not proof the wasm export graph links.
Merge guidance
- BLOCK. Do not merge.
- Fix
QAPI (methods or namespace calls — pick one and use it inbridge.zig). Wait forzig → harness.wasmgreen. - Rebase onto current
main(325e966) and resolveAGENTS.md. - Optional: surface queue-full in the band (EMBER/muted), do not drop the composer contents (already kept).
What was not attacked
Live operator smoke (Busy ▶ enqueue / edit / cancel / Stop / promote / New); DO runner cache; future #536 TODO drain actually calling canAutoContinue; 390 px queue-band wrap.
Wasm-ephemeral FIFO above the composer: Busy ▶ / Ctrl+Enter enqueues, edit/cancel in-canvas, Stop keeps the queue. Protocol v18 adds inv_queued_count plus a canAutoContinue seam so a later TODO drain waits for the operator queue.
…666 L1+L4) submit_queue.count/item/push/clear/removeAt/replaceAt are module-level fns, so bridge.zig's `queue.count()` method-call style does not bind and broke the wasm32-freestanding compile (build-harness red). Use submit_queue.NAME(&queue, ...) consistently, matching the test files and the existing canPromote/promoteIf calls.
…666 L9) When the FIFO is at MAX_ITEMS a further ▶/Ctrl+Enter is a silent no-op. Paint an EMBER '· full' tag next to the 'N queued' label so the failed enqueue is visible in-canvas instead of swallowed (composer text is still kept).
ed4ca8d to
d500b0e
Compare
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #666
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← plan/queue-while-busy @ d500b0e · 21 files · Wasm submit-queue (plan #664) + protocol v18
Lenses run: L1, L3, L4, L5, L6, L8, L9 (skip L2: scalar count export only, no new secret/runner surface; skip L7: no host/config bind)
AGENTS.md read: yes · docs/feature-divide.md read · SECURITY.md skipped (no workflow/secret/API change)
Prior BLOCK on ed4ca8d (method-call compile break, dirty merge, silent Full) is fixed: namespace calls, mergeable=CLEAN, · full in EMBER, build-harness green.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1 | queue_editing_index is UI-only and is not cleared when the FIFO dies. inv_clear_messages / reset() wipe slots; ui.zig:379 (the existing n < prev_msg clear path) resets chip/thinking/toolrun and misses the new latch. resetTranscriptScroll is the only clear, and it runs from onInit only. |
1. Busy, enqueue ≥1, click ✎. 2. New / Clear (HarnessHost resetBridge → clearMessages) or rail session switch (hydrateMessages → clearMessages + push). 3. Slots are empty (Goal 7). Latch stays Some(i). 4. desiredHeight treats n==0 && editing as a 80 px ghost band (“0 queued”, no rows, no Clear). 5. Start another turn, enqueue follow-ups. Turn ends → Trigger A sees editing==true → tryPromoteQueued never runs. Recovery is an unmarked Escape. Hydrate onto a longer transcript skips the n < prev_msg branch entirely, so the latch survives even that reset. |
Defender: “Escape recovers; who edits and then New/Clears?” Counter: session switch from the rail during a Busy turn is a normal Goal-7 path, and the clear handler in ui.zig:379 already exists for this exact host call — they updated it for the chip and forgot the new fields. Defender: “canPromote(editing) is the hold.” Counter: the hold is correct while a row exists. After the SoT is empty the hold is a stale ghost that blocks a new queue. |
high |
| Minor | L1+L8 | Plan + docs/harness-limits.md say edit blur saves. queue_band.zig only saves on Ctrl+Enter / ✎; there is no blur path. Clicking the composer leaves the row in edit and holds promote until Escape/✎. |
Enqueue 2 items, click ✎ on item 2 (or item 1), click the composer to type more. In-flight turn reaches Ready. Trigger A is gated on !editing. Follow-ups sit until the operator notices the still-open row. |
Defender: “held-while-editing is locked.” Counter: the lock assumes the operator is in the editor. Blur was the plan’s way to leave it. Docs currently lie. Not a data-loss bug if they hit Escape; it is a drain stall. | high |
| Nit | L6 | queue_preview.test.zig “UTF-8 back-off does not split a codepoint” builds an 80-byte é string and never hits the 100 B cap, so a broken back-off stays green. |
A later 4-byte scalar that straddles byte 100 would be the first real mid-sequence cut. Implementation looks correct (continuation strip + start-byte strip); the test does not prove it. |
Not a live paint bug at N=18-style prompts. Vacuous pin only. | high |
Residual risk
canAutoContinue is intentionally unwired — a future #536 TODO drain that forgets it will still race Ready vs the next Wasm promote frame. tryPromoteQueued itself is untested (only canPromote + promoteIf units); the ui.zig Trigger A/B edge is operator-only. Queue-row textEntry is single-line (multiline=false) against a multiline composer — opening ✎ on a pasted multi-line follow-up may flatten on save (not independently proven against this dvui pin). replaceAt still stack-allocates a 256 KiB tmp on the save path. Operator smoke (plan 9–14) was not re-run here.
Merge guidance
- CONCERNS. Do not merge until the Major is fixed.
- On every queue wipe (
inv_clear_messagesconsumer inui.zigclear/hydrate path, and/orif (queuedCount()==0) drop the latch), clearqueue_editing_index,queue_closed_edit,prev_queue_band_h. Do not set Trigger B when the SoT is already empty. - Optional: actually save-on-blur, or correct the docs; pin a preview test that is ≥100 B mid-sequence.
What was not attacked
Live Preview operator paths (Busy ▶ / Ctrl+Enter / Stop-keeps-queue / 390 px). Production Gateway. Self-hosted runner beyond confirming build-harness #32107628872 is green. Future #536 actually calling canAutoContinue.
Plan to address feedback on PR #666 (Status: DONE)Last updated: 2026-08-18 Feedback items (most complex → least)
#1 — queue_editing_index ghost-latch after ring clear (🟢 DONE)
#2 — blur-to-save on queue-row editor (🟢 DONE)
#3 — vacuous queue_preview UTF-8 back-off test (🟢 DONE)
Progress log
Merge decisionAll three items addressed. Ready for re-review (adversarial review on updated head). |
…UTF-8 back-off test (review #666)
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #666
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← plan/queue-while-busy @ 5a910c7 · 21 files · Wasm submit-queue (plan #664) + protocol v18
Lenses run: L1, L3, L4, L5, L6, L8, L9 (skip L2: count export only, no new secret/runner surface; skip L7: no host/config bind)
AGENTS.md read: yes · docs/feature-divide.md read · SECURITY.md skipped (no workflow/secret/API change)
Re-review after 5a910c7 (claimed fixes for the previous Major / Minor / Nit). build-harness green; mergeable=CLEAN. Compile + · full + UTF-8 preview pin stay fixed.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1 | queue_band.paint blur-save (queue_band.zig:93-105) closes the row editor the first frame a textEntry exists. beginEdit does not focusWidget. Frame 1 of ✎: editing was false at paintRow, so no TE is created and queue_edit_textentry_id is left stale or null. Frame 2: TE is created, id stored, then focused == null or focused != te_id is almost certainly true (focus is still the ✎ button). saveEdit → closeEdit. Also: n < prev_msg clears the index/buf but not queue_edit_textentry_id (ui.zig:398-401), so the next ✎ after New/Clear compares against a dead widget id and save-closes in the same frame as beginEdit. There is no “ignore blur on the open frame” latch. |
1. Busy, enqueue 1+. 2. Click ✎. 3. Editor flashes at most one frame, then the preview row is back. Edits are impossible. After New/Clear, even the flash is gone. | Defender: “dvui auto-focuses a new textEntry.” Even if it did on the create frame, the create frame is after the click frame, and the click left focus on the button — the check treats that as blur. Defender: “if (te_id) skips when null.” Only the first-ever edit skips frame 1; frame 2 still kills it. After any completed edit/close, te_id is null’d; after ring-wipe it is not, which is worse. |
high |
| Major | L1 | Previous Major is only half-fixed. Latch drop is tied to n < prev_msg (ui.zig:379). Rail / restore hydrate is hydrateMessages → clearMessages + push in one batch (harnessBridge.ts:409-412, pushSessionToBridge default clear). Next frame n is the new session length. If that length is ≥ last_msg_count, the branch never runs. FIFO is already empty (inv_clear_messages → submit_queue.clear); queue_editing_index stays Some. desiredHeight (queue_band.zig:15) still treats n==0 && editing as an 80 px ghost band and Trigger A (ui.zig:135) still refuses tryPromoteQueued. |
1. Session A has 3 rows. Busy, enqueue, click ✎ (or leave the latch set). 2. Switch to session B with 20 rows. 3. n=20, prev=3 → hydrate path, not n < prev. 4. Ghost “0 queued” band; next Busy turn’s follow-ups never promote until an unmarked Escape. New/Clear does hit n < prev (one system line) — that is why the author thought this was done. |
Defender: “we added the clear to the existing wipe branch.” That branch is a ring-shrink heuristic, not a queue-SoT observer. Goal 7 is session switch, which is hydrate-longer as often as New. Defender: “also drop when queuedCount()==0.” That is the actual fix; it is not in this commit. |
high |
Residual risk
canAutoContinue is still unused — a later #536 drain that ignores it will race the next Wasm promote. tryPromoteQueued / Trigger A/B have no unit tests. Queue-row TE is multiline=false; a pasted multi-line follow-up can flatten on a successful save. replaceAt still stack-allocates 256 KiB. Operator smoke (Busy ▶ / Stop-keeps-queue / 390 px) was not re-run here. The UTF-8 preview test now fails a no-back-off implementation (ends on a start byte); it still does not pin len == 99.
Merge guidance
- CONCERNS. Do not merge until both Majors are fixed.
- Blur-save:
focusWidgeton the open frame; ignore blur until the TE id has been seen focused at least once; treatfocused == nullas “unknown,” not blur; clearqueue_edit_textentry_idon every latch drop. - Latch vs SoT: if
queuedCount()==0, dropqueue_editing_index/ te id / draft (do not proxy through ringn < prev_msg). - Then re-run operator: ✎ stays open, type, click composer → save; session switch while editing → no ghost, next queue still promotes.
What was not attacked
Live Preview operator paths. Production Gateway. Self-hosted runner beyond confirming build-harness #32191878308 is green. Future #536 calling canAutoContinue.
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #666
Verdict: PASS WITH NOTES
Repo: btipling/invincible
Scope: main ← plan/queue-while-busy · 21 files · +897/−20 · submit queue (Zig + TS)
Lenses run: L1, L2, L3, L4, L5, L6, L7, L8, L9 (skip: none — all applicable)
AGENTS.md read: yes
feature-divide read: yes
SECURITY.md: N/A (no workflow/secrets/runner/API changes — pure harness + bridge)
Re-review context
Round 1 returned CONCERNS with 3 findings (Major L1 ghost-edit, Minor L1+L8 blur-save, Nit L6 vacuous test). Commit 5a910c7 addressed all three. This re-review confirms the fixes and attacks the updated diff for residual issues.
Confirmed fixes from round 1
| # | Finding | Fix at 5a910c7 | Status |
|---|---|---|---|
| 1 | queue_editing_index ghost-latch after ring clear |
Clear queue editing state in ui.zig n < prev_msg path |
✅ Resolved |
| 2 | No blur-to-save on queue-row editor | Track queue_edit_textentry_id, save-and-close on focus loss |
✅ Resolved |
| 3 | Vacuous UTF-8 back-off test (80 B input, 100 B cap) | Real test: 66 B "ab" + € pads past 100 B, byte 100 mid-codepoint | ✅ Resolved |
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Minor | L6 | queue_band.zig has no dvui testing-backend layout test — desiredHeight is pure arithmetic (header + min(count,3)×TOUCH_H) but busy_row_layout.test.zig exists for similar layout math. Queue band layout includes scroll area interaction and one-frame height settle. |
Height math changes introduce a subtle rounding error undetected until an operator sees band clipping. | desiredHeight is trivial arithmetic — no minSizeGet/wrap sampling like composer_last_h. The one-frame settle mirrors prev_chip_visible and needs no test. Layout error would be caught by any manual smoke. |
low |
| Minor | L8 | harnessCanAutoContinue.ts has zero callers — the module header says "do not invent one." A gate function with no consumer is dead code. Scanning for "who gates on the queue" returns a function that nothing imports. |
A future auto-continue implementer misses this gate because it's not wired. | 13 lines, fully unit-tested, self-documents the contract. The header is explicit: "There is no HarnessHost caller yet." Public API before consumer is standard (Rust/Go ship this pattern). Included in this PR to lock the contract alongside the queue. | low |
| Nit | L6 | queue_list_scroll (ScrollInfo) in state.zig is not reset when the ring is cleared (n < prev_msg path in ui.zig) — only the queue editing state is cleared. A stale scroll offset from a previous queue session persists in Wasm memory. |
After Clear → new session → queue items reappear, the scroll area starts at the old scroll offset instead of the top. | Queue is empty after clear (submit_queue.clear in inv_clear_messages) → desiredHeight returns 0 → band doesn't paint. Stale scroll only matters when items reappear, and it would show one row offset at worst. Max 4 rows visible. |
low |
Residual risk
The promote path has a one-frame settle: when tryPromoteQueued succeeds, busy = true is set mid-frame, so the composer switches to busy mode (enqueue+Stop) but the busy row doesn't paint until next frame. This is consistent with the existing one-frame settle pattern for composer height and chip visibility. A rapid-fire promote → enqueue → promote sequence (operator typing at machine speed) would see each subsequent promotion also one-frame-delayed, which is correct — the queue drains sequentially with a frame between each turn start.
Merge guidance
- PASS WITH NOTES — safe to merge from this attack. Prior CONCERNS resolved. Nits/minor findings optional.
- The
harnessCanAutoContinuedead-code finding (Minor L8) is a deliberate seam — not a regression or bug. - The scroll-reset nit is cosmetic and only observable when manually testing queue-after-clear in rapid succession.
What was not attacked
- Live DO runner / production sandbox integration (no TS server changes beyond
inv_queued_countexport) - Production Gateway / model inference interaction (queue is purely in-canvas, host-neutral)
- Full wasm-int test suite (no harness.wasm in this sandbox; CI
build-harnessverified in round 1) - Mobile ~390px queue band layout (band uses full canvas width; rows are 40px TOUCH_H targets — should be fine but not physically tested)
Gate summary
| Gate | Result |
|---|---|
zig fmt --check |
✅ |
zig build test-rich (submit_queue 11/11 + queue_preview 6/6) |
✅ |
zig build test-rich-invariants |
✅ |
zig build harness -Doptimize=Debug |
✅ |
build-harness CI (round 1) |
✅ #32191878308 |
|
Merged with the last adversarial CONCERNS still open. Follow-up: #676 (blur-save kills ✎; latch not dropped on longer-session hydrate). |
1 participant
Summary
Implements btipling/invincible#664 (source btipling/invincible#405): operators can queue follow-up prompts while Busy, then edit / cancel them in-canvas. Stop still only aborts the in-flight turn.
submit_queue.zig, max 16, per-itemSUBMIT_CAP)tryPromoteQueued(peek →queueSubmitFromUi→ pop only if accepted) on busy→ready/err and on edit-closeinv_queued_count+ unusedcanAutoContinue()seam for a later session-TODO draindocs/harness-limits.md,docs/feature-divide.md,AGENTS.md,native/harness/README.mdFixes #664
Refs #405
Refs #536
Test plan
npm run typecheckvitest run lib/harnessCanAutoContinue.test.ts lib/harnessBridge.test.ts— 56 passed (8 newcanAutoContinue+ 1 newqueuedCount+ existing bridge)submit_queue.test.zig/queue_preview.test.zigregistered intest-rich(Zig not in this workspace —build-harnessis the gate)Expected CI delta (TS):
harnessCanAutoContinue+8,harnessBridge+1 vs previouslib/harnessBridge.test.tscount. Zigtest-richadds 11 + 6 host tests on the self-hosted runner.lib/harnessChat.wasm-int.test.tsneedsharness.wasm(protocol 18). This sandbox has no artifact — CIbuild-harness+ fetch is the path. Do not treat a local missing-wasm fail as a product regression.Cloud ops / docs
Cloud ops: N/A (no Production mutate). Docs shipped in this PR (timeless; no phase theater).