feat(harness): bind Escape key to cancel in-progress turn when canvas is focused - #708
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #708
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← plan/escape-cancel-turn @ 1304bda · 2 files · Escape→queueCancelFromUi + keyboard-table docs
Lenses run: L1, L3, L5, L8, L9 (skip: L2 no trust-boundary/secrets; L4 no workflow/artifact change, zig → harness.wasm green; L6 no tests — plan-locked frame(); L7 no BYO/config)
AGENTS.md read: yes · docs/feature-divide.md yes (Stop/cancel is Wasm control + DOM abort; this PR stays on that path)
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1+L9 | ui.zig Escape scan (~644–649) marks every unhandled .escape .down e.handled = true with no busy gate, then always calls bridge.queueCancelFromUi(). Paint order is composer scan (~617) before status-bar model_picker.paint (~836). Pinned dvui eventMatch returns false when e.handled; MenuWidget.processEventsAfter (called from deinit) is the stock Escape-to-close for dvui.menu / floatingMenu. Comment at ~640–643 and plan #705 both say skip the call / do not mark handled when idle so Esc can pass through. Code does the opposite. |
1. /harness idle, catalog has 2+ models. 2. Open the status-bar model picker. 3. Press Escape. Before: MenuWidget closes the floating menu. After: composer scan consumes Esc; picker stays open. Same steal on Busy+picker (cancel fires, menu still does not close). |
Defender: queueCancelFromUi no-ops when lifecycle != .busy && !has_pending_submit (bridge.zig:288–289), so idle cancel is harmless. That only covers abort, not event consumption. Defender: NavMenu focuses itself (NavMenu.tsx) so DOM Esc still works — true, and the wrong victim; the in-canvas dvui menu is the one that loses the key. Defender: #703 said “model picker has no Esc handler today” — the app has none; dvui does, at MenuWidget.processEventsAfter .escape → close(). Defender: plan-review on #705 said “always mark handled, only call when busy” — that note is what this bug is; the plan lock is idle = no-op and not handled. Fix is the comment’s own text: add busy to the if (in scope at ui.zig:139) so idle Esc is left unmarked. |
high |
| Minor | L8 | docs/harness-limits.md rewrites the ↑/↓ row to “Works while Busy — the queue band is visible…” This branch still has if (!busy and …) at ui.zig:662 and the comment “Skip history when busy”. PR #707 (the actual arrow-while-Busy change) is still open. Plan-review #705 finding 2: “only add the Escape row; ↑/↓ is already correct.” origin/main still says “Not active while Busy.” |
Merge #708 before #707. Keyboard table claims arrows navigate history during a spin. Operator tries ↑ on an empty Busy composer — caret moves (or nothing), history does not. Living docs describe a parallel unmerged PR. | Defender: “forward-compat with #707.” Living docs must describe this merge. Defender: “plan-review thought #707 was on main.” It isn’t (mergedAt: null); this PR still changed the row instead of leaving it. Also copies #707’s inaccurate “queue band is visible while Busy” (band is queuedCount() > 0, independent of Busy). |
high |
Residual risk
Even with busy gated: Busy + open model picker still cancels the turn and leaves the menu up (#703 explicitly deferred picker-vs-cancel). Queue-row Esc still depends on queue_band.paint running first and marking handled — the extra queue_editing_index == null guard is belt-and-suspenders and correct. .repeat is not marked handled (plan: single-fire on .down); a held Esc after cancel could reach a later widget. No frame() test exists to lock the busy gate (same class as every ui.zig event-scan change).
Merge guidance
CONCERNS — do not merge until the Major is fixed: gate the Escape handler on busy (do not mark idle Esc handled). Revert the ↑/↓ docs rewrite; only add the Escape row against main’s current idle wording. Do not wait on #707.
What was not attacked
Live /harness smoke (picker open → Esc; Busy → Esc; queue-edit → Esc). dvui web-backend key routing beyond the pinned eventMatch/MenuWidget sources. Host takePendingCancel / AbortController path (unchanged). Parallel #707 interaction at runtime (docs-only collision in this diff).
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #708 (R2)
Verdict: PASS WITH NOTES
Repo: btipling/invincible
Scope: main ← plan/escape-cancel-turn @ 23739a0 · 2 files · Escape→queueCancelFromUi gated on busy + keyboard-table docs
Lenses run: L1, L3, L5, L8, L9 (skip: L2 no trust-boundary/secrets; L4 no workflow/artifact change — build-harness is main-only, Vercel ignored; L6 no tests — plan-locked frame(); L7 no BYO/config)
AGENTS.md read: yes · docs/feature-divide.md yes (Stop/cancel is Wasm control + DOM abort; this PR stays on that path)
R1 @ 1304bda was CONCERNS (idle Esc marked handled → MenuWidget.processEventsAfter never closes the model picker). 23739a0 adds busy and to the if and stops marking idle Esc handled. Pinned dvui eventMatch still returns false when e.handled; idle path is now unmarked. That Major is dead.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Minor | L8 | Branch is still rooted at 4bc831b (pre-#707). docs/harness-limits.md conflicts with current main on the keyboard table. This head still has if (!busy and state.queue_editing_index == null) for arrows while the docs row already says “Works while Busy.” Trial-merge into origin/main: ui.zig is clean (Escape insert + #707’s !busy removal coexist). Docs conflict is HEAD empty vs incoming Escape row after the shared arrow line. |
1. Merge #708 on GitHub without rebase. 2. Conflict UI on docs/harness-limits.md. 3. Accept current (ours). Escape row never lands. Keyboard table on main has no Esc chord; living docs disagree with the Wasm scan. |
Defender: “the conflict is trivial, just take both.” True if resolved that way — the break is accepting ours. Defender: “arrow rewrite matches main after 7c9d912.” Also true; that is why the conflict is only the new row. Rebase onto main before merge so the PR is conflict-free and the branch is not documenting #707 behavior it does not contain. |
high |
Residual risk
Busy + open model picker: Esc still cancels the turn and leaves the menu up (composer scan is before model_picker.paint ~834; #703 / #705 lock cancel while Busy, picker-vs-cancel was deferred). Queue-row Esc still depends on queue_band.paint running first and marking handled — extra queue_editing_index == null guard is correct. .repeat is not marked handled (plan: single-fire on .down); a held Esc after cancel can reach later widgets. No frame() test locks the busy gate. queueCancelFromUi / host takePendingCancel path unchanged.
Merge guidance
PASS WITH NOTES — prior Major is fixed; safe to merge from this attack after a rebase onto main (or a careful docs conflict resolve that adds the Escape row, does not accept-ours). Do not --approve from this review.
What was not attacked
Live /harness smoke (idle picker → Esc; Busy → Esc; queue-edit → Esc). dvui web-backend key routing beyond the pinned hashKeyCode("Escape") => .escape + MenuWidget / eventMatch sources. Host takePendingCancel / AbortController. Parallel #709 (composer wrap) interaction.
23739a0 to
21a8069
Compare
Closes #705
Refs #703
Summary
Bind Escape to
bridge.queueCancelFromUi()in the harness event scan — the same cancel path as the ■ Stop icon. Today Stop is pointer-only; Escape does nothing to the in-flight round.Changes (2 files, +17/−1)
native/harness/src/ui.zig.queueCancelFromUi()on.escape+.downwhen not editing a queue row. Idle Esc is not marked handled (passes through for DOM nav overlays)..repeatskipped.docs/harness-limits.mdDesign
.down+ queue editor closedbridge.queueCancelFromUi()+ mark handled.down+ queue editor openqueue_band.handleEscape()owns Esc.repeatEscape events.downfires cancelGates (all green)
zig fmt --checkzig build harness -Doptimize=Debugzig build test-rich-invariantsNo TS changes — no vitest/typecheck/di-gate/build. No protocol bump, no bridge changes, no caps changed.