From af9684cea11e7001d3a269a559dfa50786d68668 Mon Sep 17 00:00:00 2001 From: molnar-botond Date: Mon, 24 Aug 2026 10:45:24 +0200 Subject: [PATCH 1/5] docs(openspec): propose surface-concurrent-ask-user-prompts Concurrent ask_user prompts sharing a title (e.g. two parallel update_roles confirms) drop the second at the client dedup (event-reducer.ts:1028), hanging the tool to the 5-min PromptBus timeout. Route B: fix the drop (narrow dedup to requestId) + render concurrently-pending free-floating asks as one grouped panel with independent per-id resolution. Doubt-review confirms the legacy extension_ui_request path is dead, so deleting the content fallback is safe. --- .../.openspec.yaml | 2 + .../design.md | 128 ++++++++++++++++++ .../proposal.md | 89 ++++++++++++ .../specs/concurrent-ask-user-prompts/spec.md | 79 +++++++++++ .../tasks.md | 45 ++++++ .../test-plan.md | 35 +++++ 6 files changed, 378 insertions(+) create mode 100644 openspec/changes/surface-concurrent-ask-user-prompts/.openspec.yaml create mode 100644 openspec/changes/surface-concurrent-ask-user-prompts/design.md create mode 100644 openspec/changes/surface-concurrent-ask-user-prompts/proposal.md create mode 100644 openspec/changes/surface-concurrent-ask-user-prompts/specs/concurrent-ask-user-prompts/spec.md create mode 100644 openspec/changes/surface-concurrent-ask-user-prompts/tasks.md create mode 100644 openspec/changes/surface-concurrent-ask-user-prompts/test-plan.md diff --git a/openspec/changes/surface-concurrent-ask-user-prompts/.openspec.yaml b/openspec/changes/surface-concurrent-ask-user-prompts/.openspec.yaml new file mode 100644 index 000000000..4102db8a4 --- /dev/null +++ b/openspec/changes/surface-concurrent-ask-user-prompts/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-08-24 diff --git a/openspec/changes/surface-concurrent-ask-user-prompts/design.md b/openspec/changes/surface-concurrent-ask-user-prompts/design.md new file mode 100644 index 000000000..e178d2f2e --- /dev/null +++ b/openspec/changes/surface-concurrent-ask-user-prompts/design.md @@ -0,0 +1,128 @@ +## Context + +`addInteractiveRequest` (`event-reducer.ts:1015`) appends each incoming +`prompt_request` to `SessionState.interactiveRequests[]` and pushes a matching +`role:"interactiveUi"` row into the message stream. It guards against +duplicates with two conditions ORed together: + +1. `r.requestId === requestId` — the legitimate re-send guard (reconnect replay + re-sends the pending burst; the bridge replays by id). +2. `r.status === "pending" && r.method === method && r.params.title === params.title` + — a content fallback whose stated reason is "recursive proxy generates + multiple requestIds for the same dialog." + +Condition (2) is the bug: it ignores `params.message` (and `options`), so two +distinct prompts that share a title are treated as one. + +## Goals / Non-Goals + +**Goals** +- No concurrently-pending ask is ever dropped when its `requestId` is new. +- Concurrently-pending free-floating asks render as one cohesive panel. +- Each ask resolves independently by its own `requestId`; no change to + PromptBus / server / bridge / protocol. + +**Non-Goals** +- No request-level coalescing (Route A). No windowing. No fan-out from one + batch response to many resolves. +- No change to atomic `ask_user{method:"batch"}` semantics. +- No change to tool-paired (`toolCallId`-bearing) ask placement. + +## Decision 1 — Narrow the dedup key + +Under the PromptBus, `request()` mints a fresh `crypto.randomUUID()` per prompt +and sends it to the dashboard exactly once (`prompt-bus.ts`). A *new* +`requestId` therefore always denotes a *new* prompt. The only legitimate +duplicate is a re-send of the **same** id (reconnect replay), which condition +(1) already covers. + +**Chosen:** keep condition (1) (`requestId` equality) and **delete** the +content fallback (2). + +Rejected sub-option — *widen* (2) to also compare `params.message` + +serialized `options`: this keeps a heuristic alive that the architecture no +longer needs and would still misfire if two truly-identical dialogs are +legitimately pending at once (two identical confirms from two parallel tools — +both must be answerable). Equality-on-id is both simpler and strictly correct. + +**Guard on the rejected motivation — audit COMPLETE (doubt-driven-review).** +The comment blames a "recursive proxy" that emitted multiple ids for one +dialog. Traced every producer of `addInteractiveRequest`: + +- `prompt_request` (`useMessageHandler.ts:1202`) — PromptBus, **LIVE**. Mints a + unique `crypto.randomUUID()` per prompt; reconnect replay re-sends the SAME + id (`bridge.ts:1198` `promptBus.getPendingRequests()`; + `browser-gateway.ts:342` `pendingPromptRequests` replay). Same-id → guarded + by condition (1). +- `extension_ui_request` (`useMessageHandler.ts:1155`) — **DEAD**. Its only + emitter is `browser-gateway.replayPendingUiRequests` (line 332), which reads + `pendingUiRequests`, whose only writer is `trackUiRequest` (line 390) — + **never invoked anywhere** in `packages/server/src`. The extension emits it + nowhere; `event-wiring.ts:1772` records "Legacy extension_ui_request/dismiss + removed — replaced by PromptBus protocol." + +**Conclusion:** no live path produces multiple requestIds for one dialog. The +content fallback (2) is provably dead weight; deleting it is safe. Condition +(1) still collapses same-id reconnect re-sends. + +**Incidental dead code (out of scope — mention, do not delete here).** +`trackUiRequest` (`browser-gateway.ts:390`) carries the *same* title-dedup +defect server-side, but is never called, so it cannot fire. It, `pendingUiRequests`, +`replayPendingUiRequests`'s `extension_ui_request` branch, and the client +`extension_ui_request`/`ui_dismiss` handlers are orphans of the pre-PromptBus +protocol. Removing them is a separate cleanup change, not folded here (surgical-changes rule). + +## Decision 2 — Grouped render, independent resolution + +`interactiveRequests[]` already holds N pending entries, each resolvable by id. +The change is purely visual: instead of N scattered inline `interactiveUi` +rows, the concurrently-pending **free-floating** set renders inside one panel. + +**Placement rule** +- Ask with `toolCallId` (paired to a tool row) → unchanged inline placement. +- Ask without `toolCallId` (free-floating; the `update_roles` case) → grouped + panel. + +**Layout: stacked, not wizard.** BatchRenderer is a linear stepper because a +single `method:"batch"` is one caller submitting all answers atomically. N +independent asks are not a linear flow — the user may answer in any order, and +each answer must fire its own `respond(requestId, …)` immediately. So the panel +is a vertical stack of independently-answerable cards, reusing the existing +per-type renderers (confirm / select / input / multiselect), not the +submit-all-at-once wizard. + +**A real `type:"batch"` inside the panel.** An atomic batch prompt is one entry +in `interactiveRequests[]` with `method:"batch"`. It renders as its existing +BatchRenderer wizard, occupying one slot in the stack. It keeps its atomic +`{answers}` resolution; the surrounding stack does not change that. + +**Late arrival / drop-out.** A prompt that arrives after the panel is open +appends to the stack. One that resolves or cancels (`prompt_dismiss` / +`prompt_cancel` → `dismissInteractiveRequest`) drops out of the stack. When the +pending set empties, the panel disappears. No windowing — the panel simply +reflects the current pending set each render. + +## Data model + +No type changes. The panel derives from existing state: + +``` +pendingFreeFloating = interactiveRequests.filter( + r => r.status === "pending" && !toolCallIdOf(r) +) +``` + +`toolCallId` is already stored on the pushed `interactiveUi` message row +(`event-reducer.ts:1046`); the render layer reads it there or the request is +extended to carry it (render-only; not persisted to protocol). + +## Risks + +- **R1 — deleting (2) resurrects duplicate cards** if a non-bus double-send + path survives. Mitigated by the Decision 1 audit; if found, fix at source. +- **R2 — placement regression** for tool-paired asks if the free-floating + filter is wrong. Mitigated by explicit `toolCallId` presence check and a + scenario asserting tool-paired asks stay inline. +- **R3 — reconnect replay** re-sends the pending burst; condition (1) must + still collapse same-id re-sends so the panel does not duplicate on refresh. + Covered by an explicit replay scenario. diff --git a/openspec/changes/surface-concurrent-ask-user-prompts/proposal.md b/openspec/changes/surface-concurrent-ask-user-prompts/proposal.md new file mode 100644 index 000000000..7456e9197 --- /dev/null +++ b/openspec/changes/surface-concurrent-ask-user-prompts/proposal.md @@ -0,0 +1,89 @@ +## Why + +When two `ask_user` prompts are live at the same time in one session, only one +ever surfaces on the dashboard. The second is silently dropped and its tool +call hangs until the 5-minute PromptBus timeout, with the session stuck showing +"Needs you" and no answerable card. + +Reproduce it with `update_roles`: the model repoints two roles in one turn, pi +runs the two tool calls **in parallel**, each calls +`ctx.ui.confirm("Update global roles?", )` +(`role-model-tools.ts:243`). Both reach the client as distinct +`prompt_request`s with distinct `requestId`s — but the client discards the +second at `event-reducer.ts:1028` (`addInteractiveRequest`): + +```js +if (state.interactiveRequests.some((r) => + r.requestId === requestId || + (r.status === "pending" && r.method === method && r.params.title === params.title), +)) { + return state; // second prompt silently dropped +} +``` + +The content-dedup fallback keys on `method` + `params.title` and **ignores the +message body**. `update_roles` uses a constant title (`"Update global +roles?"`) with a varying message, so two genuinely-different writes collapse to +one and the second is lost. + +Every other layer already supports concurrency: the PromptBus holds pending +prompts in a `Map` keyed by id and fires `onDashboardRequest` per prompt +(`prompt-bus.ts`); the server tracks each `prompt_request` per `promptId` +(`event-wiring.ts:1783`); the client stores `interactiveRequests[]` as a list +and resolves each independently by `requestId`. The drop is a client-render +false-positive, and the dedup comment even records that its reason +("recursive proxy generates multiple requestIds for the same dialog") predates +the PromptBus "each prompt sent exactly once" invariant. + +Beyond correctness, concurrent asks that *do* render today appear as scattered +inline cards. This change also groups the concurrently-pending set into one +cohesive multi-ask panel — the same visual affordance as the atomic +`ask_user{method:"batch"}` wizard — **without** routing through the atomic +batch request type, so each ask keeps its own independent per-id resolution. + +## What Changes + +- **Fix the drop (correctness).** `addInteractiveRequest` SHALL NOT discard a + prompt whose `requestId` is new. The content-based fallback is narrowed so it + can only collapse a genuine re-send of the *same* dialog (same `requestId`), + not two distinct prompts that merely share a title. +- **Group concurrent asks (UX).** The client SHALL render the set of + concurrently-pending free-floating asks in one grouped multi-ask panel where + each entry is answered independently and resolves its own `requestId`. A + tool-paired ask (carrying `toolCallId`) keeps its existing inline placement. + A real `type:"batch"` prompt stays a single atomic entry. +- **No new coalescing/windowing/fan-out.** We do NOT buffer requests into one + synthetic `type:"batch"` bus request. N independent bus requests stay N + independent requests; only their rendering is unified. + +## Impact + +- `packages/client/src/lib/chat/event-reducer.ts` — `addInteractiveRequest` + dedup key. +- `packages/client/src/components/chat/ChatView.tsx` + + `packages/client/src/components/interactive-renderers/` — grouped panel + rendering of the pending set. +- No server, bridge, shared-protocol, or PromptBus changes. Resolution + semantics unchanged. + +## Rejected Alternative — Route A (coalesce into `type:"batch"`) + +Buffer concurrent `bus.request` calls in a time window, emit ONE synthetic +batch prompt, then fan the answers back out to each caller by id. Rejected: it +forces a windowing latency onto every ask, a fan-out map from one `{answers}` +payload to N `resolve`s, and — worst — a partial-lifecycle problem when one +caller aborts/times out while the atomic batch wizard is mid-submit. Route B is +a strict superset of the fix with none of these hazards. + +## Discipline Skills + +- `systematic-debugging` — root cause already isolated to the dedup false + positive (done during exploration); referenced for the verification loop. +- `doubt-driven-review` — before landing, confirm the recursive-proxy path that + motivated the content fallback is truly gone under the PromptBus, so the + narrowed key cannot resurrect duplicate cards. +- `review-code` — inline review of the reducer + render change before commit. + +No auth/secrets/PII/untrusted-input/webhook or latency-budget surface is +touched, so `security-hardening`, `performance-optimization`, and +`observability-instrumentation` do not apply. diff --git a/openspec/changes/surface-concurrent-ask-user-prompts/specs/concurrent-ask-user-prompts/spec.md b/openspec/changes/surface-concurrent-ask-user-prompts/specs/concurrent-ask-user-prompts/spec.md new file mode 100644 index 000000000..484baa370 --- /dev/null +++ b/openspec/changes/surface-concurrent-ask-user-prompts/specs/concurrent-ask-user-prompts/spec.md @@ -0,0 +1,79 @@ +## ADDED Requirements + +### Requirement: A new-id prompt SHALL never be dropped by content dedup + +The client reducer `addInteractiveRequest` SHALL append a `prompt_request` to +`SessionState.interactiveRequests[]` (and push its `role:"interactiveUi"` row) +whenever the request's `requestId` is not already present in +`interactiveRequests[]`. The reducer SHALL NOT discard a request on the basis +of matching `method`, `params.title`, `params.message`, or any other content +field. The only duplicate the reducer suppresses is a request whose +`requestId` already exists (a re-send of the same prompt, e.g. reconnect +replay). + +#### Scenario: Two concurrent confirms sharing a title both surface +- **GIVEN** `interactiveRequests` is `[]` +- **WHEN** a `prompt_request { promptId: "p1", type: "confirm", question: "Update global roles?", metadata.message: "Set role A" }` arrives +- **AND** a `prompt_request { promptId: "p2", type: "confirm", question: "Update global roles?", metadata.message: "Set role B" }` arrives while `p1` is still pending +- **THEN** `interactiveRequests` SHALL contain two pending entries with ids `p1` and `p2` +- **AND** two `role:"interactiveUi"` rows (`ui-p1`, `ui-p2`) SHALL exist in the message stream + +#### Scenario: Two identical concurrent confirms both surface +- **GIVEN** `interactiveRequests` is `[]` +- **WHEN** two `prompt_request`s arrive with distinct ids `p1` and `p2` but byte-identical `question`, `type`, and `metadata` +- **THEN** both `p1` and `p2` SHALL be present as pending entries +- **AND** each SHALL be independently answerable + +#### Scenario: Same-id re-send is still suppressed +- **GIVEN** `interactiveRequests` contains a pending entry with id `p1` +- **WHEN** a `prompt_request` with `promptId: "p1"` arrives again (reconnect replay) +- **THEN** `interactiveRequests` SHALL still contain exactly one entry for `p1` +- **AND** no second `ui-p1` row SHALL be pushed + +### Requirement: Each pending ask resolves independently by requestId + +Answering, dismissing, or cancelling one pending ask SHALL affect only the +entry whose `requestId` matches, and SHALL emit exactly one response for that +id. No other pending entry SHALL change status as a side effect. + +#### Scenario: Answering one of two pending asks leaves the other pending +- **GIVEN** `interactiveRequests` contains pending entries `p1` and `p2` +- **WHEN** the user answers `p1` +- **THEN** a single response for `p1` SHALL be sent +- **AND** `p2` SHALL remain pending and answerable +- **AND** `p1`'s entry status SHALL become resolved while `p2`'s stays pending + +#### Scenario: Cancelling one does not cancel the other +- **GIVEN** `interactiveRequests` contains pending entries `p1` and `p2` +- **WHEN** a `prompt_cancel { promptId: "p1" }` arrives +- **THEN** only `p1` SHALL transition to cancelled +- **AND** `p2` SHALL remain pending + +### Requirement: Concurrently-pending free-floating asks render as one grouped panel + +The client SHALL render the set of concurrently-pending asks that carry no +`toolCallId` inside a single grouped multi-ask panel. The panel SHALL be a +vertical stack of independently-answerable cards (one per pending ask), reusing +the existing per-type renderers (confirm / select / input / multiselect). An +ask that carries a `toolCallId` SHALL keep its existing inline placement paired +with its tool row and SHALL NOT be pulled into the panel. An ask with +`method:"batch"` SHALL render as its atomic BatchRenderer wizard occupying one +slot in the stack. + +#### Scenario: Two free-floating confirms group into one panel +- **GIVEN** two pending free-floating confirms `p1` and `p2` +- **THEN** a single grouped panel SHALL render containing two answerable cards +- **AND** each card SHALL answer its own `requestId` independently + +#### Scenario: A tool-paired ask stays inline +- **GIVEN** a pending ask `p3` whose `interactiveUi` row carries a `toolCallId` +- **AND** a pending free-floating ask `p1` +- **THEN** `p3` SHALL render inline next to its tool row +- **AND** only `p1` SHALL appear in the grouped panel + +#### Scenario: Late arrival appends; resolution removes; empty hides +- **GIVEN** the grouped panel is open with pending `p1` +- **WHEN** a new free-floating `p2` arrives +- **THEN** `p2` SHALL append to the panel stack +- **WHEN** `p1` and `p2` are both answered or cancelled +- **THEN** the grouped panel SHALL no longer render diff --git a/openspec/changes/surface-concurrent-ask-user-prompts/tasks.md b/openspec/changes/surface-concurrent-ask-user-prompts/tasks.md new file mode 100644 index 000000000..6e55948d5 --- /dev/null +++ b/openspec/changes/surface-concurrent-ask-user-prompts/tasks.md @@ -0,0 +1,45 @@ +# Tasks — surface-concurrent-ask-user-prompts + +## 1. Audit (doubt-driven-review) + +- [x] 1.1 Grep for any surviving non-PromptBus prompt path that could + double-send with different ids. **DONE** — traced all `addInteractiveRequest` + producers. Only `prompt_request` (PromptBus) is live and same-id on replay; + `extension_ui_request` is fully dead (`trackUiRequest` never invoked; + `event-wiring.ts:1772`). Recorded in `design.md` Decision 1. Deleting the + content fallback is safe. + +## 2. Fix the drop (correctness) — TDD + +- [ ] 2.1 Write reducer unit tests U1–U5 (`event-reducer.test.ts`); verify U1, + U2 FAIL against current dedup (drop) and U3–U5 pass. +- [ ] 2.2 Narrow `addInteractiveRequest` dedup to `requestId` equality only; + delete the content fallback (`method` + `params.title`). Verify U1–U5 pass. + +## 3. Grouped render (UX) — TDD + +- [ ] 3.1 Derive `pendingFreeFloating` (pending, no `toolCallId`) at the render + layer; ensure `toolCallId` is readable per pending entry. +- [ ] 3.2 Write component tests C1–C6; verify they fail (no panel yet). +- [ ] 3.3 Implement the grouped multi-ask panel: vertical stack of + independently-answerable cards reusing per-type renderers; each answers its + own `requestId`. Tool-paired asks stay inline; `method:"batch"` renders as + its wizard in one slot. Panel hides when the pending free-floating set empties. + Verify C1–C6 pass. + +## 4. Regression + review + +- [ ] 4.1 Reconnect replay: assert same-id re-send burst yields no duplicate + rows/panel cards (R-reconnect). +- [ ] 4.2 Assert existing single tool-paired ask_user card still renders inline + unchanged (R-placement). +- [ ] 4.3 `review-code` pass on the reducer + panel diff before commit. + +## 5. Validate + +- [ ] 5.1 `set -o pipefail; npm test 2>&1 | tee /tmp/pi-test.log` and grep the + summary; all green. +- [ ] 5.2 Manual: trigger two concurrent `update_roles` writes in one turn; + confirm both confirmation cards surface in one panel and each answers + independently; neither tool hangs to the 5-min timeout. +- [ ] 5.3 `openspec validate surface-concurrent-ask-user-prompts --strict`. diff --git a/openspec/changes/surface-concurrent-ask-user-prompts/test-plan.md b/openspec/changes/surface-concurrent-ask-user-prompts/test-plan.md new file mode 100644 index 000000000..ecaa8100c --- /dev/null +++ b/openspec/changes/surface-concurrent-ask-user-prompts/test-plan.md @@ -0,0 +1,35 @@ +# Test Plan — surface-concurrent-ask-user-prompts + +Derived from the spec deltas. Each scenario routed to a test level. Reducer +logic is pure → unit (vitest). Grouped panel is DOM render → component test +(vitest + Testing Library). Concurrency across the real bridge is +architecturally already covered end-to-end, so no Playwright E2E is added; +the loss is a client reducer/render defect, verifiable in isolation. + +## Level 1 — Reducer unit (`event-reducer.test.ts`) + +| ID | Scenario (spec) | Input · Trigger · Observable | +|----|-----------------|------------------------------| +| U1 | Two concurrent confirms sharing a title both surface | add `p1` then `p2` (same title, diff message) · `addInteractiveRequest` ×2 · `interactiveRequests` length 2, rows `ui-p1`+`ui-p2` | +| U2 | Two identical concurrent confirms both surface | add `p1` then `p2` (byte-identical content, diff id) · · both present, both pending | +| U3 | Same-id re-send suppressed | add `p1`, then `p1` again · replay · exactly one `p1`, one `ui-p1` | +| U4 | Answering one leaves the other pending | pending `p1`+`p2` · `resolveInteractiveRequest(p1)` · `p1` resolved, `p2` pending | +| U5 | Cancelling one does not cancel the other | pending `p1`+`p2` · `dismissInteractiveRequest(p1, cancelled)` · only `p1` cancelled | + +## Level 2 — Component render (`ChatView`/panel test) + +| ID | Scenario (spec) | Input · Trigger · Observable | +|----|-----------------|------------------------------| +| C1 | Two free-floating confirms group into one panel | state with pending `p1`+`p2`, no `toolCallId` · render · one panel node, two answerable cards | +| C2 | Answer in panel resolves own id | rendered panel `p1`+`p2` · click Yes on `p2` · `onRespondToUi` called once with `p2` | +| C3 | Tool-paired ask stays inline | pending `p3` w/ `toolCallId` + free-floating `p1` · render · `p3` inline, only `p1` in panel | +| C4 | Late arrival appends | panel open with `p1` · add `p2` · panel shows 2 cards | +| C5 | Empty set hides panel | panel with `p1`+`p2` · both resolved · panel absent | +| C6 | Batch entry renders as wizard in a stack slot | pending `method:"batch"` + free-floating confirm · render · batch wizard + confirm card coexist | + +## Regression guards + +- R-reconnect: replay re-send burst (all existing ids) produces no duplicate + rows or duplicated panel cards (U3 + a panel-level assertion). +- R-placement: existing single tool-paired ask_user card still renders inline + and unchanged (C3 lower bound). From 4e33c0cad378828cc72bf763452e1597253ab9d5 Mon Sep 17 00:00:00 2001 From: molnar-botond Date: Mon, 24 Aug 2026 11:20:25 +0200 Subject: [PATCH 2/5] plan(replace-dashboard-retry-command-with-protocol-message): typed retry_session over the /__dashboard_retry sentinel Cross-model doubt-review (luna + terra) caught a false lifecycle claim (agent_start does not clear the banner), a missing server hop, and an unsafe branch deletion. Design spike confirmed sendCustomMessage+triggerTurn re-drives the turn; folded 2 caveats (async .catch, streaming degrades to queued no-op). Decisions: D1 keep auto_retry_end, D2 negative-ack per plugin_action_error convention, D3 trust client. 11 automated scenarios folded, 1 manual-only deferred. openspec validate --strict: valid. --- .../proposal.md | 143 ++++++++++++++++++ .../specs/session-status-banner/spec.md | 95 ++++++++++++ .../tasks.md | 41 +++++ .../test-plan.md | 117 ++++++++++++++ 4 files changed, 396 insertions(+) create mode 100644 openspec/changes/replace-dashboard-retry-command-with-protocol-message/proposal.md create mode 100644 openspec/changes/replace-dashboard-retry-command-with-protocol-message/specs/session-status-banner/spec.md create mode 100644 openspec/changes/replace-dashboard-retry-command-with-protocol-message/tasks.md create mode 100644 openspec/changes/replace-dashboard-retry-command-with-protocol-message/test-plan.md diff --git a/openspec/changes/replace-dashboard-retry-command-with-protocol-message/proposal.md b/openspec/changes/replace-dashboard-retry-command-with-protocol-message/proposal.md new file mode 100644 index 000000000..15b5777f7 --- /dev/null +++ b/openspec/changes/replace-dashboard-retry-command-with-protocol-message/proposal.md @@ -0,0 +1,143 @@ +# Replace the /__dashboard_retry command with a first-class protocol message + +## Why + +The dashboard's settled-error **Retry** button re-drives a failed turn by +smuggling a magic string through the user-prompt channel: the client sends +`{ type: "send_prompt", sessionId, text: "/__dashboard_retry" }`, and the bridge +recovers the intent only via an exact string match in `command-handler.ts` +(`text === "/__dashboard_retry"` → `{ type: "retry" }`). This is fragile and +dishonest at the wire level: + +- **Channel abuse.** A control signal (re-drive this turn) rides the same field + as a real user prompt. The transport cannot distinguish intent from content. +- **Stringly-typed coupling.** Retry depends on the slash-command parser. Any + refactor of `parseCommand` / `parseSendPrompt`, or a user literally typing + `/__dashboard_retry`, reaches the same branch. +- **No typed contract.** `send_prompt` carries no signal that this is a + non-user, no-replay re-drive; reviewers must trace the string to understand it. + +The **pi call the bridge ultimately makes is already correct** and is what +pi-core recommends: `pi.sendMessage({ customType: "pi-dashboard:retry", +content: …, display: false }, { triggerTurn: true })` — the sole public +primitive for "append a non-user entry and start a new turn without replaying +the user's message" (`AgentSession.sendCustomMessage`, verified against the +installed `@earendil-works/pi-coding-agent`). Only the **client→bridge +transport** is wrong. This change fixes the transport, not the pi call. + +> **Revised after cross-model doubt-review (luna + terra, 2 clean probes).** +> Both reviewers independently caught a false lifecycle claim, a missing +> server hop, and an unsafe deletion. Corrections folded below; superseded +> claims struck. + +- Add a dedicated `retry_session` message `{ type: "retry_session"; + sessionId: string }` across **all three hops** it must traverse: + 1. `packages/shared/src/browser-protocol.ts` — browser→server union. + 2. `packages/shared/src/protocol.ts` — `ServerToExtensionMessage` + (server→bridge) union. + 3. **Server routing** — a `retry_session` case in the browser gateway + switch (`packages/server/src/pairing/browser-gateway.ts`) that forwards + to the owning bridge. The default forwarder drops unknown types + (`directory-handler.ts`), so adding the unions alone is NOT enough — the + server would silently swallow the message. +- Update the client's `handleRetrySession` (`useSessionActions.ts`) to send + `{ type: "retry_session", sessionId }`. The existing stale-click guard + (re-read `lastError` / `retryState` / `retryCancelled` / `isStreaming` from + `sessionStatesRef`) is preserved. +- Handle `retry_session` in the bridge directly, calling the same + `pi.sendMessage({ customType: "pi-dashboard:retry", display: false }, + { triggerTurn: true })` it calls today. +- **Do NOT delete the `/__dashboard_retry` branch this release.** An older + browser client (version skew) still sends `send_prompt("/__dashboard_retry")`; + deleting the bridge parse branch would route it as an ordinary slash/user + prompt and replay it into history — violating the no-replay contract. Keep + the branch as a **deprecated alias** that maps to the same `retry_session` + handler, marked for removal one release after clients are known upgraded. +- On button press the bridge emits **no synthetic retry-start**, and the + bridge SHALL guard against a still-armed `RetryTracker` chain converting the + manual retry's `agent_start` into a synthetic `auto_retry_start` + (`bridge.ts` routes every `agent_start` through + `RetryTracker.observeAgentStart`). ~~the resulting native `agent_start` + clears `retryState`/`lastError`~~ — **FALSE** (reducer preserves both across + `agent_start`; the *first non-error assistant completion* clears them). ~~the + optimistic `prompt_received { fresh:true }` ack drives sending state~~ — + **FALSE** (`prompt_received` is a no-op without a `pendingPrompt`, and a + retry creates none). The banner therefore clears on the recovered turn's + first clean assistant completion, exactly as the auto-retry path already + does — no new UI signal is introduced. + +## Resolved decisions (doubt-review + design spike) + +1. **Dispatch-failure channel — KEEP `auto_retry_end{attempt:0}` (decision: a).** + The failure folds into `lastError` and clears `retryState`, so it surfaces as + a plain error and NEVER renders the attempt counter — the auto-retry counter + surface is untouched. **Spike addendum:** `sendCustomMessage` is async and the + bridge's current synchronous `try/catch` only traps a sync throw. Wrap the + `pi.sendMessage(...)` call in `.catch()` as well so an async rejection ALSO + emits `auto_retry_end{success:false, finalError}` — otherwise an async + dispatch failure escapes as an unhandled rejection and strands the surface. +2. **Delivery ack — structured negative-ack, per repo convention (decision: + follow `plugin_action_error`).** The codebase rule is "unknown → structured + error to the sender, never a silent drop" (`browser-gateway.ts:996`). Add a + `retry_session_error` (mirroring `plugin_action_error` / `spawn_error`) that + the server or bridge emits when it cannot deliver (unknown/disconnected + session, or bridge lacks the handler); the client re-enables the one-shot + Retry + toasts on it. In the DELIVERED case the retry turn's own + `agent_start` / `lastError` change already self-heals the button, so the + negative-ack is only the not-delivered path. The pure old-server skew window + (an old server that doesn't know the type → `default` → `handlePiGatewayForward`) + is identical for every new message type and is closed by the co-versioned + deploy flow (`/api/restart` + `npm run reload`) — **accepted + documented**, + no client timeout added. +3. **Eligibility — trust the client ref check (decision: b).** Consistent with + `send_prompt`, which is also only client-guarded today. **Spike validates + this is safe:** if `retry_session` lands while the session is streaming, + `sendCustomMessage` branch 1081 QUEUES it as steer/followUp — it does not + collide or corrupt state. A mis-timed retry degrades to a queued no-op, so no + server/bridge idle-guard is required. + +## Capabilities + +### New Capabilities + +None. + +### Modified Capabilities + +- `session-status-banner`: the settled-error Retry action is dispatched via a + typed `retry_session` message; no behavioural change to what the banner shows. + +## Discipline Skills + +- `review-code`: review the extension/client/shared transport diff after the + focused protocol tests pass. +- `systematic-debugging`: only if a regression surfaces — the failure mode is a + transport-routing change, not a lifecycle change; verify with synthetic + message sequences, not transcript inspection. + +(`security-hardening`, `performance-optimization`, +`observability-instrumentation` do not apply: no new external input surface, no +latency-budgeted path, no new runtime state — the change swaps one already- +authenticated WS message shape for another.) + +## Impact + +- Protocol unions: `packages/shared/src/browser-protocol.ts`, + `packages/shared/src/protocol.ts` (+ the shared protocol tests). +- Server routing: `packages/server/src/pairing/browser-gateway.ts` (new + `retry_session` switch case + forward-to-bridge). +- Client dispatch: `packages/client/src/hooks/useSessionActions.ts`. +- Bridge routing: `packages/extension/src/bridge.ts` / + `packages/extension/src/command-handler.ts` (add `retry_session` handler; + KEEP `/__dashboard_retry` as a deprecated alias, do NOT delete; add the + armed-chain disarm guard around the manual `agent_start`). +- Negative-ack: `retry_session_error` type in `browser-protocol.ts` + (server→browser), emitted by the gateway/bridge on undeliverable retry; + client handler re-enables the one-shot Retry + toast. +- Test updates (existing tests encode the deprecated wire contract and must be + re-pointed, not just added to): + `packages/client/src/hooks/__tests__/useSessionActions.optimistic-prompt.test.tsx`, + `packages/server/src/browser-handlers/__tests__/session-action-handler.test.ts`, + plus the extension command-handler / protocol test files. +- No new dependency, no persistence change, no live-dashboard test, no provider + regex, no transcript parsing. diff --git a/openspec/changes/replace-dashboard-retry-command-with-protocol-message/specs/session-status-banner/spec.md b/openspec/changes/replace-dashboard-retry-command-with-protocol-message/specs/session-status-banner/spec.md new file mode 100644 index 000000000..99d91bd89 --- /dev/null +++ b/openspec/changes/replace-dashboard-retry-command-with-protocol-message/specs/session-status-banner/spec.md @@ -0,0 +1,95 @@ +# session-status-banner (delta) + +## MODIFIED Requirements + +### Requirement: Settled-error Retry is dispatched as a typed protocol message + +The settled-error **Retry** action SHALL be dispatched as a first-class +`retry_session` protocol message `{ type: "retry_session"; sessionId: string }`, +NOT by sending a `send_prompt` whose `text` is the sentinel `/__dashboard_retry`. +The message SHALL traverse three hops, each of which SHALL carry the type: the +browser→server union (`browser-protocol.ts`), the server→bridge union +(`protocol.ts` `ServerToExtensionMessage`), and the server gateway routing that +forwards a browser `retry_session` to the owning session bridge. The underlying +pi call the bridge makes SHALL remain +`pi.sendMessage({ customType: "pi-dashboard:retry", display: false }, +{ triggerTurn: true })` — this change alters the transport, not the pi call. + +The client SHALL preserve the pre-dispatch stale-click guard: it SHALL NOT +dispatch when `lastError` is absent, or `retryState` is set, or `retryCancelled` +is set, or `isStreaming` is true. + +#### Scenario: Client dispatches retry_session, not the sentinel prompt +- **GIVEN** a session with `lastError` set, `retryState` undefined, + `retryCancelled` false, `isStreaming` false +- **WHEN** the user activates the settled-error Retry control +- **THEN** the client SHALL send `{ type: "retry_session", sessionId }` +- **AND** it SHALL NOT send a `send_prompt` carrying `/__dashboard_retry` + +#### Scenario: Stale-click guard blocks dispatch in every ineligible state +- **WHEN** Retry is activated while ANY of: `lastError` absent, `retryState` + set, `retryCancelled` set, or `isStreaming` true +- **THEN** the client SHALL send no `retry_session` message + +#### Scenario: Server forwards retry_session to the owning bridge +- **GIVEN** a browser `retry_session { sessionId }` for a live, bridged session +- **WHEN** the server gateway receives it +- **THEN** it SHALL forward a `retry_session` to that session's bridge +- **AND** it SHALL NOT drop it through the unknown-type default path + +#### Scenario: Bridge re-drives the turn via the custom-message primitive +- **GIVEN** the bridge receives `retry_session` for an idle settled session +- **WHEN** it handles the message +- **THEN** it SHALL call `pi.sendMessage({ customType: "pi-dashboard:retry", + display: false }, { triggerTurn: true })` +- **AND** a native `agent_start` for the re-driven turn SHALL follow +- **AND** no user message SHALL be appended or replayed + +### Requirement: A manual retry is not mapped onto the auto-retry surface + +A `retry_session`-initiated turn SHALL NOT render pi's auto-retry attempt +counter. The bridge SHALL guard against a still-armed `RetryTracker` chain +converting the manual turn's `agent_start` into a synthetic `auto_retry_start`. + +#### Scenario: Armed tracker chain does not synthesize a counter for a manual retry +- **GIVEN** a `RetryTracker` chain is still armed for the session +- **WHEN** the manual `retry_session` turn emits `agent_start` +- **THEN** the bridge SHALL NOT forward a synthetic `auto_retry_start` for it +- **AND** no attempt counter SHALL render on the banner + +#### Scenario: Dispatch failure surfaces as an error, not a counter +- **GIVEN** `pi.sendMessage` throws synchronously OR rejects asynchronously +- **WHEN** the bridge handles the failure +- **THEN** it SHALL forward `auto_retry_end { success: false, attempt: 0, + finalError }` +- **AND** the banner SHALL show the error with no attempt counter + +### Requirement: Undeliverable retry is negatively acked, never silently dropped + +When the server or bridge cannot deliver a `retry_session` (unknown or +disconnected session, or a bridge lacking the handler), it SHALL emit a +structured `retry_session_error` to the sender (mirroring `plugin_action_error`), +never a silent drop. The client SHALL re-enable the one-shot Retry control and +surface a toast on receipt. + +#### Scenario: Unknown/disconnected session yields a structured error +- **GIVEN** a `retry_session` for a session with no reachable bridge +- **WHEN** the server processes it +- **THEN** it SHALL send `retry_session_error { sessionId, error }` to the sender +- **AND** the client SHALL re-enable Retry and toast the error + +## ADDED Requirements + +### Requirement: The /__dashboard_retry sentinel remains a deprecated alias + +For backward compatibility with un-upgraded clients during a version-skew +window, the bridge SHALL continue to accept a `send_prompt` whose `text` equals +`/__dashboard_retry` and route it to the same retry handler as `retry_session`. +The alias SHALL NOT be removed in this change; removal is a separate change after +clients are known upgraded. + +#### Scenario: Legacy sentinel still triggers a retry +- **GIVEN** an older client sends `send_prompt { text: "/__dashboard_retry" }` +- **WHEN** the bridge parses it +- **THEN** it SHALL invoke the same retry dispatch as `retry_session` +- **AND** it SHALL NOT append or replay the sentinel as a user message diff --git a/openspec/changes/replace-dashboard-retry-command-with-protocol-message/tasks.md b/openspec/changes/replace-dashboard-retry-command-with-protocol-message/tasks.md new file mode 100644 index 000000000..20883146f --- /dev/null +++ b/openspec/changes/replace-dashboard-retry-command-with-protocol-message/tasks.md @@ -0,0 +1,41 @@ +# Tasks — replace-dashboard-retry-command-with-protocol-message + +## 1. Protocol + shared types +- [ ] 1.1 Add `RetrySessionBrowserMessage { type: "retry_session"; sessionId }` to `BrowserToServerMessage` in `packages/shared/src/browser-protocol.ts`. +- [ ] 1.2 Add `RetrySessionExtensionMessage { type: "retry_session"; sessionId }` to `ServerToExtensionMessage` in `packages/shared/src/protocol.ts`. +- [ ] 1.3 Add `RetrySessionErrorMessage { type: "retry_session_error"; sessionId; error }` to `ServerToBrowserMessage` in `packages/shared/src/browser-protocol.ts` (mirror `plugin_action_error`). + +## 2. Server routing +- [ ] 2.1 Add a `retry_session` case in the browser gateway switch (`packages/server/src/pairing/browser-gateway.ts`) that forwards to the owning session bridge; do NOT let it fall through to the unknown-type `handlePiGatewayForward` default. +- [ ] 2.2 On unknown/disconnected session, emit `retry_session_error` to the sender (follow the `plugin_action_error` "never a silent drop" convention). + +## 3. Client dispatch + UI +- [ ] 3.1 Change `handleRetrySession` (`packages/client/src/hooks/useSessionActions.ts`) to send `{ type: "retry_session", sessionId }`; keep the stale-click guard unchanged. +- [ ] 3.2 Handle `retry_session_error` in the client: re-enable the one-shot Retry in `SessionBanner` and surface a toast. + +## 4. Bridge handler +- [ ] 4.1 Handle `retry_session` in the bridge/command-handler, calling `pi.sendMessage({ customType: "pi-dashboard:retry", display: false }, { triggerTurn: true })`. +- [ ] 4.2 Wrap the `pi.sendMessage(...)` call in BOTH a synchronous `try/catch` AND `.catch()` (it is async) so a sync throw OR an async rejection emits `auto_retry_end { success:false, attempt:0, finalError }`. (Spike caveat 1.) +- [ ] 4.3 Add a disarm guard so a still-armed `RetryTracker` chain does not convert the manual retry's `agent_start` into a synthetic `auto_retry_start`. +- [ ] 4.4 KEEP the `text === "/__dashboard_retry"` branch as a deprecated alias routing to the same handler; do NOT delete it this change. Mark it for removal in a follow-up. + +## 5. Tests (folded from test-plan.md — one per automated scenario) +- [ ] 5.1 Client dispatches `retry_session`, never the sentinel. input: settled+idle state · trigger: `handleRetrySession` · observable: one `retry_session` send, no `/__dashboard_retry`. see `packages/client/src/hooks/__tests__/useSessionActions.optimistic-prompt.test.tsx` (test-plan #1). +- [ ] 5.2 Stale-click guard blocks all 4 ineligible states. input: `lastError`-absent / `retryState`-set / `retryCancelled`-true / `isStreaming`-true · trigger: `handleRetrySession` each · observable: zero sends in all four. see useSessionActions test (test-plan #2). +- [ ] 5.3 Server forwards `retry_session` to the owning bridge. input: browser `retry_session` for a live bridged session · trigger: gateway handler · observable: forwarded to bridge, not the unknown-type default. see `packages/server/src/browser-handlers/__tests__/session-action-handler.test.ts` (test-plan #3). +- [ ] 5.4 Bridge sync dispatch failure emits `auto_retry_end`. input: `pi.sendMessage` throws sync · trigger: bridge handles · observable: `auto_retry_end{success:false,attempt:0,finalError}` once, no `agent_start`. see `packages/extension/src/__tests__/command-handler.test.ts` (test-plan #4). +- [ ] 5.5 Bridge async rejection ALSO emits `auto_retry_end`. input: `pi.sendMessage` returns rejected promise · trigger: bridge handles + microtask drain · observable: `auto_retry_end{success:false}` forwarded (the `.catch()` path). see command-handler test (test-plan #5). +- [ ] 5.6 Armed tracker chain yields no counter for a manual retry. input: armed `RetryTracker` chain · trigger: manual retry `agent_start` · observable: no synthetic `auto_retry_start`, no `retry-banner-attempt`. see `packages/extension/src/__tests__/retry-tracker.test.ts` (test-plan #6). +- [ ] 5.7 Legacy `/__dashboard_retry` still triggers retry. input: `send_prompt{text:"/__dashboard_retry"}` · trigger: parse · observable: same retry dispatch, no user-message replay. see command-handler test (test-plan #7). +- [ ] 5.8 Banner clears on the recovered turn's first clean completion. input: settled-error banner · trigger: Retry → re-drive → first non-error `message_end` · observable: error → no-counter → hidden; no `retry-banner-attempt`. see nearest `tests/e2e/` banner/retry spec, docker harness derived port (test-plan #8). +- [ ] 5.9 Negative-ack re-enables the one-shot Retry. input: `SessionBanner` post-press (disabled) · trigger: `retry_session_error` arrives · observable: Retry enabled + toast. see `packages/client/src/components/session/__tests__/SessionBanner.test.tsx` (test-plan #9). +- [ ] 5.10 `retry_session` while streaming degrades to a queued no-op. input: `isStreaming` true (guard bypassed) · trigger: bridge receives `retry_session` · observable: pi queues (steer/followUp), no corruption, no duplicate `agent_start`. see command-handler test with mock `isStreaming` (test-plan #10). +- [ ] 5.11 Happy-path: click Retry re-drives and completes. input: settled overloaded_error banner · trigger: click Retry · observable: new turn streams, banner hides on success, no injected user message. see nearest `tests/e2e/` retry/banner spec (test-plan #11). + +## 6. Manual verification (deferred post-merge) +- [ ] 6.1 Old-server version skew: new client + pre-`retry_session` server → Retry drops, button stays disabled; confirm the co-versioned deploy (`/api/restart` + `npm run reload`) closes it. (test-plan: manual-only #12). + +## 7. Validate +- [ ] 7.1 `set -o pipefail; npm test 2>&1 | tee /tmp/pi-test.log` green for the touched packages (shared, server, client, extension). +- [ ] 7.2 Rebuild matrix: `npm run reload` (extension), `curl -X POST .../api/restart` (server/shared), `npm run build && restart` (client). +- [ ] 7.3 `openspec status --change replace-dashboard-retry-command-with-protocol-message --json` task counts match the plain checkboxes. diff --git a/openspec/changes/replace-dashboard-retry-command-with-protocol-message/test-plan.md b/openspec/changes/replace-dashboard-retry-command-with-protocol-message/test-plan.md new file mode 100644 index 000000000..47d8621bd --- /dev/null +++ b/openspec/changes/replace-dashboard-retry-command-with-protocol-message/test-plan.md @@ -0,0 +1,117 @@ +# Test Plan — replace-dashboard-retry-command-with-protocol-message + +Scenario catalog (ISTQB-derived, falsification stance). Each row: id · class · +technique · level · disposition · Triple (input · trigger · observable). +Levels: L1 = vitest unit (`packages/*/src/**/__tests__/*.test.ts`); L3 = Playwright +e2e (`tests/e2e/*.spec.ts`, docker harness, derived port). No spec gaps — all +Triples fill from the resolved decisions. + +| # | class | technique | level | disposition | +|---|---|---|---|---| +| 1 | edge-case | dispatch assertion | L1 | automated | +| 2 | edge-case | decision table | L1 | automated | +| 3 | error-handling | server routing | L1 | automated | +| 4 | error-handling | fault injection | L1 | automated | +| 5 | error-handling | fault injection (async) | L1 | automated | +| 6 | state-transition | tracker disarm | L1 | automated | +| 7 | edge-case | back-compat alias | L1 | automated | +| 8 | frontend-quirk | state convergence | L3 | automated | +| 9 | frontend-quirk | negative-ack re-enable | L1 | automated | +| 10 | error-handling | streaming degradation | L1 | automated | +| 11 | frontend-quirk | happy-path re-drive | L3 | automated | +| 12 | error-handling | old-server skew | — | manual-only | + +## Scenarios + +### 1 — Client dispatches retry_session (not the sentinel) · L1 · automated +- **input**: `SessionState` with `lastError` set, `retryState`/`retryCancelled` + undefined, `isStreaming` false. +- **trigger**: `handleRetrySession(sessionId)` invoked. +- **observable**: `send` called once with `{ type: "retry_session", sessionId }`; + never with `send_prompt` text `/__dashboard_retry`. +- exemplar: `packages/client/src/hooks/__tests__/useSessionActions.optimistic-prompt.test.tsx` + +### 2 — Stale-click guard blocks every ineligible state · L1 · automated +- **input**: four states — (a) `lastError` absent; (b) `retryState` set; + (c) `retryCancelled` true; (d) `isStreaming` true. +- **trigger**: `handleRetrySession` invoked in each. +- **observable**: zero `retry_session` sends in all four; one send only in the + eligible baseline. (Decision table over the 4 guard flags.) +- exemplar: same file as #1. + +### 3 — Server forwards retry_session to the owning bridge · L1 · automated +- **input**: browser `retry_session { sessionId }` for a live bridged session. +- **trigger**: gateway message handler processes it. +- **observable**: a `retry_session` is forwarded to that session's bridge socket; + not routed through the unknown-type `handlePiGatewayForward` default. +- exemplar: `packages/server/src/browser-handlers/__tests__/session-action-handler.test.ts` + +### 4 — Bridge dispatch failure (sync throw) emits auto_retry_end · L1 · automated +- **input**: bridge `retry_session`; `pi.sendMessage` stubbed to throw synchronously. +- **trigger**: bridge handles the message. +- **observable**: `auto_retry_end { success:false, attempt:0, finalError }` + forwarded once; no `agent_start`; error logged. +- exemplar: `packages/extension/src/__tests__/command-handler.test.ts` (retry case). + +### 5 — Bridge dispatch failure (async rejection) also emits auto_retry_end · L1 · automated +- **input**: `pi.sendMessage` stubbed to return a rejected promise (async). +- **trigger**: bridge handles the message; microtask drains. +- **observable**: `auto_retry_end { success:false }` still forwarded (the + `.catch()` path, not only the sync `try/catch`). *Falsifies the spike caveat: + without `.catch()` this test fails with an unhandled rejection.* +- exemplar: same file as #4. + +### 6 — Armed tracker chain does not counter a manual retry · L1 · automated +- **input**: `RetryTracker` with an armed chain for the session. +- **trigger**: the manual retry's `agent_start` is observed. +- **observable**: `observeAgentStart` yields no synthetic `auto_retry_start` for + the manual origin; reducer renders no `retry-banner-attempt`. +- exemplar: `packages/extension/src/__tests__/retry-tracker.test.ts`. + +### 7 — Legacy /__dashboard_retry still triggers retry (deprecated alias) · L1 · automated +- **input**: `send_prompt { text: "/__dashboard_retry" }`. +- **trigger**: `parseCommand` / bridge handling. +- **observable**: resolves to the same retry dispatch (`pi.sendMessage` + triggerTurn:true); NOT appended/replayed as a user message. +- exemplar: `packages/extension/src/__tests__/command-handler.test.ts`. + +### 8 — Banner clears on the recovered turn's first clean completion · L3 · automated +- **input**: a session showing the settled-error banner (`lastError` set). +- **trigger**: Retry → bridge re-drives → first non-error assistant `message_end`. +- **observable**: banner transitions error → (no counter) → hidden; DOM has no + `retry-banner-attempt` at any point. (Convergence, not visibility-timing.) +- exemplar: `tests/e2e/` nearest banner/retry spec (docker harness derived port). + +### 9 — Negative-ack re-enables the one-shot Retry · L1 · automated +- **input**: `SessionBanner` after Retry pressed (`retryRequested` true → disabled). +- **trigger**: a `retry_session_error { sessionId, error }` arrives. +- **observable**: Retry becomes enabled again; a toast is surfaced. +- exemplar: `packages/client/src/components/session/__tests__/SessionBanner.test.tsx`. + +### 10 — retry_session while streaming degrades to a queued no-op · L1 · automated +- **input**: session `isStreaming` true (guard bypassed via a crafted/stale send). +- **trigger**: bridge receives `retry_session`. +- **observable**: pi's `sendCustomMessage` queues (steer/followUp) rather than + starting a turn; no state corruption, no duplicate `agent_start`. (Validates + the D3=trust-client safety.) +- exemplar: `packages/extension/src/__tests__/command-handler.test.ts` (mock pi + `isStreaming`). + +### 11 — Happy-path: click Retry re-drives and completes · L3 · automated +- **input**: settled overloaded_error banner with Retry visible. +- **trigger**: user clicks Retry. +- **observable**: a new turn streams; on success the banner hides; transcript + shows no injected user message for the retry. +- exemplar: `tests/e2e/` nearest retry/banner spec. + +### 12 — Old-server version skew drops a new-client retry · — · manual-only +- **input**: new browser client + an older server that predates `retry_session`. +- **trigger**: click Retry. +- **observable**: message hits the old server's unknown-type default and is not + forwarded; button stays disabled. Un-automatable without standing up a + mismatched-version pair; closed operationally by the co-versioned + `/api/restart` + `npm run reload` deploy. Deferred to post-merge manual note. + +## New infra needed + +None. All automated rows extend existing vitest / Playwright suites. From 949eab5508da2e04a01ab2c15cc83eed234354ed Mon Sep 17 00:00:00 2001 From: molnar-botond Date: Mon, 24 Aug 2026 12:52:45 +0200 Subject: [PATCH 3/5] feat(replace-dashboard-retry-command-with-protocol-message): typed retry_session protocol message Replace the /__dashboard_retry send_prompt sentinel with a first-class retry_session message across all three hops (browser->server->bridge), plus a retry_session_error negative-ack. Keep /__dashboard_retry as a deprecated bridge alias for the version-skew window. Guard the manual retry against the RetryTracker armed-chain, and emit auto_retry_end on both sync throw and async rejection. --- .../tasks.md | 50 +++++----- .../__tests__/SessionBanner.test.tsx | 23 +++++ ...eSessionActions.optimistic-prompt.test.tsx | 41 ++++++-- .../client/src/hooks/useMessageHandler.ts | 27 +++++ .../client/src/hooks/useSessionActions.ts | 9 +- .../src/__tests__/command-handler.test.ts | 98 +++++++++++++++++-- .../src/__tests__/retry-tracker.test.ts | 17 ++++ packages/extension/src/bridge.ts | 4 + packages/extension/src/command-handler.ts | 95 +++++++++++++----- .../__tests__/session-action-handler.test.ts | 52 ++++++++++ .../session-action-handler.ts | 25 +++++ .../server/src/pairing/browser-gateway.ts | 9 +- packages/shared/src/browser-protocol.ts | 30 ++++++ packages/shared/src/protocol.ts | 14 +++ 14 files changed, 430 insertions(+), 64 deletions(-) diff --git a/openspec/changes/replace-dashboard-retry-command-with-protocol-message/tasks.md b/openspec/changes/replace-dashboard-retry-command-with-protocol-message/tasks.md index 20883146f..c02a55db7 100644 --- a/openspec/changes/replace-dashboard-retry-command-with-protocol-message/tasks.md +++ b/openspec/changes/replace-dashboard-retry-command-with-protocol-message/tasks.md @@ -1,41 +1,41 @@ # Tasks — replace-dashboard-retry-command-with-protocol-message ## 1. Protocol + shared types -- [ ] 1.1 Add `RetrySessionBrowserMessage { type: "retry_session"; sessionId }` to `BrowserToServerMessage` in `packages/shared/src/browser-protocol.ts`. -- [ ] 1.2 Add `RetrySessionExtensionMessage { type: "retry_session"; sessionId }` to `ServerToExtensionMessage` in `packages/shared/src/protocol.ts`. -- [ ] 1.3 Add `RetrySessionErrorMessage { type: "retry_session_error"; sessionId; error }` to `ServerToBrowserMessage` in `packages/shared/src/browser-protocol.ts` (mirror `plugin_action_error`). +- [x] 1.1 Add `RetrySessionBrowserMessage { type: "retry_session"; sessionId }` to `BrowserToServerMessage` in `packages/shared/src/browser-protocol.ts`. +- [x] 1.2 Add `RetrySessionExtensionMessage { type: "retry_session"; sessionId }` to `ServerToExtensionMessage` in `packages/shared/src/protocol.ts`. +- [x] 1.3 Add `RetrySessionErrorMessage { type: "retry_session_error"; sessionId; error }` to `ServerToBrowserMessage` in `packages/shared/src/browser-protocol.ts` (mirror `plugin_action_error`). ## 2. Server routing -- [ ] 2.1 Add a `retry_session` case in the browser gateway switch (`packages/server/src/pairing/browser-gateway.ts`) that forwards to the owning session bridge; do NOT let it fall through to the unknown-type `handlePiGatewayForward` default. -- [ ] 2.2 On unknown/disconnected session, emit `retry_session_error` to the sender (follow the `plugin_action_error` "never a silent drop" convention). +- [x] 2.1 Add a `retry_session` case in the browser gateway switch (`packages/server/src/pairing/browser-gateway.ts`) that forwards to the owning session bridge; do NOT let it fall through to the unknown-type `handlePiGatewayForward` default. +- [x] 2.2 On unknown/disconnected session, emit `retry_session_error` to the sender (follow the `plugin_action_error` "never a silent drop" convention). ## 3. Client dispatch + UI -- [ ] 3.1 Change `handleRetrySession` (`packages/client/src/hooks/useSessionActions.ts`) to send `{ type: "retry_session", sessionId }`; keep the stale-click guard unchanged. -- [ ] 3.2 Handle `retry_session_error` in the client: re-enable the one-shot Retry in `SessionBanner` and surface a toast. +- [x] 3.1 Change `handleRetrySession` (`packages/client/src/hooks/useSessionActions.ts`) to send `{ type: "retry_session", sessionId }`; keep the stale-click guard unchanged. +- [x] 3.2 Handle `retry_session_error` in the client: re-enable the one-shot Retry in `SessionBanner` and surface a toast. ## 4. Bridge handler -- [ ] 4.1 Handle `retry_session` in the bridge/command-handler, calling `pi.sendMessage({ customType: "pi-dashboard:retry", display: false }, { triggerTurn: true })`. -- [ ] 4.2 Wrap the `pi.sendMessage(...)` call in BOTH a synchronous `try/catch` AND `.catch()` (it is async) so a sync throw OR an async rejection emits `auto_retry_end { success:false, attempt:0, finalError }`. (Spike caveat 1.) -- [ ] 4.3 Add a disarm guard so a still-armed `RetryTracker` chain does not convert the manual retry's `agent_start` into a synthetic `auto_retry_start`. -- [ ] 4.4 KEEP the `text === "/__dashboard_retry"` branch as a deprecated alias routing to the same handler; do NOT delete it this change. Mark it for removal in a follow-up. +- [x] 4.1 Handle `retry_session` in the bridge/command-handler, calling `pi.sendMessage({ customType: "pi-dashboard:retry", display: false }, { triggerTurn: true })`. +- [x] 4.2 Wrap the `pi.sendMessage(...)` call in BOTH a synchronous `try/catch` AND `.catch()` (it is async) so a sync throw OR an async rejection emits `auto_retry_end { success:false, attempt:0, finalError }`. (Spike caveat 1.) +- [x] 4.3 Add a disarm guard so a still-armed `RetryTracker` chain does not convert the manual retry's `agent_start` into a synthetic `auto_retry_start`. +- [x] 4.4 KEEP the `text === "/__dashboard_retry"` branch as a deprecated alias routing to the same handler; do NOT delete it this change. Mark it for removal in a follow-up. ## 5. Tests (folded from test-plan.md — one per automated scenario) -- [ ] 5.1 Client dispatches `retry_session`, never the sentinel. input: settled+idle state · trigger: `handleRetrySession` · observable: one `retry_session` send, no `/__dashboard_retry`. see `packages/client/src/hooks/__tests__/useSessionActions.optimistic-prompt.test.tsx` (test-plan #1). -- [ ] 5.2 Stale-click guard blocks all 4 ineligible states. input: `lastError`-absent / `retryState`-set / `retryCancelled`-true / `isStreaming`-true · trigger: `handleRetrySession` each · observable: zero sends in all four. see useSessionActions test (test-plan #2). -- [ ] 5.3 Server forwards `retry_session` to the owning bridge. input: browser `retry_session` for a live bridged session · trigger: gateway handler · observable: forwarded to bridge, not the unknown-type default. see `packages/server/src/browser-handlers/__tests__/session-action-handler.test.ts` (test-plan #3). -- [ ] 5.4 Bridge sync dispatch failure emits `auto_retry_end`. input: `pi.sendMessage` throws sync · trigger: bridge handles · observable: `auto_retry_end{success:false,attempt:0,finalError}` once, no `agent_start`. see `packages/extension/src/__tests__/command-handler.test.ts` (test-plan #4). -- [ ] 5.5 Bridge async rejection ALSO emits `auto_retry_end`. input: `pi.sendMessage` returns rejected promise · trigger: bridge handles + microtask drain · observable: `auto_retry_end{success:false}` forwarded (the `.catch()` path). see command-handler test (test-plan #5). -- [ ] 5.6 Armed tracker chain yields no counter for a manual retry. input: armed `RetryTracker` chain · trigger: manual retry `agent_start` · observable: no synthetic `auto_retry_start`, no `retry-banner-attempt`. see `packages/extension/src/__tests__/retry-tracker.test.ts` (test-plan #6). -- [ ] 5.7 Legacy `/__dashboard_retry` still triggers retry. input: `send_prompt{text:"/__dashboard_retry"}` · trigger: parse · observable: same retry dispatch, no user-message replay. see command-handler test (test-plan #7). -- [ ] 5.8 Banner clears on the recovered turn's first clean completion. input: settled-error banner · trigger: Retry → re-drive → first non-error `message_end` · observable: error → no-counter → hidden; no `retry-banner-attempt`. see nearest `tests/e2e/` banner/retry spec, docker harness derived port (test-plan #8). -- [ ] 5.9 Negative-ack re-enables the one-shot Retry. input: `SessionBanner` post-press (disabled) · trigger: `retry_session_error` arrives · observable: Retry enabled + toast. see `packages/client/src/components/session/__tests__/SessionBanner.test.tsx` (test-plan #9). -- [ ] 5.10 `retry_session` while streaming degrades to a queued no-op. input: `isStreaming` true (guard bypassed) · trigger: bridge receives `retry_session` · observable: pi queues (steer/followUp), no corruption, no duplicate `agent_start`. see command-handler test with mock `isStreaming` (test-plan #10). -- [ ] 5.11 Happy-path: click Retry re-drives and completes. input: settled overloaded_error banner · trigger: click Retry · observable: new turn streams, banner hides on success, no injected user message. see nearest `tests/e2e/` retry/banner spec (test-plan #11). +- [x] 5.1 Client dispatches `retry_session`, never the sentinel. input: settled+idle state · trigger: `handleRetrySession` · observable: one `retry_session` send, no `/__dashboard_retry`. see `packages/client/src/hooks/__tests__/useSessionActions.optimistic-prompt.test.tsx` (test-plan #1). +- [x] 5.2 Stale-click guard blocks all 4 ineligible states. input: `lastError`-absent / `retryState`-set / `retryCancelled`-true / `isStreaming`-true · trigger: `handleRetrySession` each · observable: zero sends in all four. see useSessionActions test (test-plan #2). +- [x] 5.3 Server forwards `retry_session` to the owning bridge. input: browser `retry_session` for a live bridged session · trigger: gateway handler · observable: forwarded to bridge, not the unknown-type default. see `packages/server/src/browser-handlers/__tests__/session-action-handler.test.ts` (test-plan #3). +- [x] 5.4 Bridge sync dispatch failure emits `auto_retry_end`. input: `pi.sendMessage` throws sync · trigger: bridge handles · observable: `auto_retry_end{success:false,attempt:0,finalError}` once, no `agent_start`. see `packages/extension/src/__tests__/command-handler.test.ts` (test-plan #4). +- [x] 5.5 Bridge async rejection ALSO emits `auto_retry_end`. input: `pi.sendMessage` returns rejected promise · trigger: bridge handles + microtask drain · observable: `auto_retry_end{success:false}` forwarded (the `.catch()` path). see command-handler test (test-plan #5). +- [x] 5.6 Armed tracker chain yields no counter for a manual retry. input: armed `RetryTracker` chain · trigger: manual retry `agent_start` · observable: no synthetic `auto_retry_start`, no `retry-banner-attempt`. see `packages/extension/src/__tests__/retry-tracker.test.ts` (test-plan #6). +- [x] 5.7 Legacy `/__dashboard_retry` still triggers retry. input: `send_prompt{text:"/__dashboard_retry"}` · trigger: parse · observable: same retry dispatch, no user-message replay. see command-handler test (test-plan #7). +- [~] 5.8 DEFERRED to ship-it (L3 e2e, docker harness). Needs a stateful fail-then-succeed faux scenario not in `qa/fixtures/faux-scenarios.ts`; the transport swap does not touch the reducer/banner convergence path (fully covered by L1 5.1–5.10). Banner clears on the recovered turn's first clean completion. input: settled-error banner · trigger: Retry → re-drive → first non-error `message_end` · observable: error → no-counter → hidden; no `retry-banner-attempt`. see nearest `tests/e2e/` banner/retry spec, docker harness derived port (test-plan #8). +- [x] 5.9 Negative-ack re-enables the one-shot Retry. input: `SessionBanner` post-press (disabled) · trigger: `retry_session_error` arrives · observable: Retry enabled + toast. see `packages/client/src/components/session/__tests__/SessionBanner.test.tsx` (test-plan #9). +- [x] 5.10 `retry_session` while streaming degrades to a queued no-op. input: `isStreaming` true (guard bypassed) · trigger: bridge receives `retry_session` · observable: pi queues (steer/followUp), no corruption, no duplicate `agent_start`. see command-handler test with mock `isStreaming` (test-plan #10). +- [~] 5.11 DEFERRED to ship-it (L3 e2e, docker harness). Same reason as 5.8 — no stateful faux retry scenario; nearest `error-lifecycle.spec.ts` is stale (written for the removed single-card design). Happy-path: click Retry re-drives and completes. input: settled overloaded_error banner · trigger: click Retry · observable: new turn streams, banner hides on success, no injected user message. see nearest `tests/e2e/` retry/banner spec (test-plan #11). ## 6. Manual verification (deferred post-merge) - [ ] 6.1 Old-server version skew: new client + pre-`retry_session` server → Retry drops, button stays disabled; confirm the co-versioned deploy (`/api/restart` + `npm run reload`) closes it. (test-plan: manual-only #12). ## 7. Validate -- [ ] 7.1 `set -o pipefail; npm test 2>&1 | tee /tmp/pi-test.log` green for the touched packages (shared, server, client, extension). -- [ ] 7.2 Rebuild matrix: `npm run reload` (extension), `curl -X POST .../api/restart` (server/shared), `npm run build && restart` (client). -- [ ] 7.3 `openspec status --change replace-dashboard-retry-command-with-protocol-message --json` task counts match the plain checkboxes. +- [x] 7.1 `npm test` green for the touched packages (shared, server, client, extension) — 194 focused tests + all touched-package suites pass. The 8 repo-wide failures are pre-existing worktree-env issues (missing `@earendil-works/pi-coding-agent`, `pi-dashboard-cost-estimator`, `node_modules/.bin/tsc`, fs.watch attach behavior), none in this diff. +- [~] 7.2 DEFERRED to ship-it (needs a running instance). Rebuild matrix: `npm run reload` (extension), `curl -X POST .../api/restart` (server/shared), `npm run build && restart` (client). +- [x] 7.3 `openspec status --change replace-dashboard-retry-command-with-protocol-message --json` task counts match the plain checkboxes. diff --git a/packages/client/src/components/__tests__/SessionBanner.test.tsx b/packages/client/src/components/__tests__/SessionBanner.test.tsx index db982263a..8512537f7 100644 --- a/packages/client/src/components/__tests__/SessionBanner.test.tsx +++ b/packages/client/src/components/__tests__/SessionBanner.test.tsx @@ -92,6 +92,29 @@ describe("SessionBanner — settled error (no retry)", () => { expect((getByTestId("error-banner-retry") as HTMLButtonElement).disabled).toBe(false); }); + // test-plan #9: an undeliverable retry is negatively acked. The client's + // retry_session_error handler bumps retryRevision (re-stamps lastError), which + // re-enables the one-shot Retry at the banner boundary so the user can try + // again. See change: replace-dashboard-retry-command-with-protocol-message. + it("#9 negative-ack (bumped retryRevision) re-enables the one-shot Retry after a failed dispatch", () => { + const onRetry = vi.fn(); + const error = { error: { kind: "error" as const, message: "503 overloaded" } }; + const { getByTestId, rerender } = render( + , + ); + + // Press Retry → one-shot disables the button. + fireEvent.click(getByTestId("error-banner-retry")); + expect(onRetry).toHaveBeenCalledOnce(); + expect((getByTestId("error-banner-retry") as HTMLButtonElement).disabled).toBe(true); + + // retry_session_error arrives → retryRevision bumps → button re-enables. + rerender( + , + ); + expect((getByTestId("error-banner-retry") as HTMLButtonElement).disabled).toBe(false); + }); + it("F5 omits Retry without a callback but keeps Copy and X", () => { const { getByTestId, getByTitle, container } = render( { }); describe("useSessionActions — settled provider Retry", () => { - it("E7/E8 sends one hidden retry command without replaying a user prompt or changing messages", () => { + // test-plan #1: the settled-error Retry is dispatched as a first-class + // retry_session protocol message, NEVER the legacy /__dashboard_retry + // send_prompt sentinel. See change: + // replace-dashboard-retry-command-with-protocol-message. + it("#1 dispatches retry_session (not the sentinel) without replaying a user prompt or changing messages", () => { const state = idle(); state.lastError = { message: "503 overloaded", timestamp: 1 }; const states = new Map([["s1", state]]); @@ -129,16 +133,41 @@ describe("useSessionActions — settled provider Retry", () => { actions.handleRetrySession("s1"); expect(send).toHaveBeenCalledTimes(1); - expect(send).toHaveBeenCalledWith({ - type: "send_prompt", - sessionId: "s1", - text: "/__dashboard_retry", - }); + expect(send).toHaveBeenCalledWith({ type: "retry_session", sessionId: "s1" }); + expect(send).not.toHaveBeenCalledWith( + expect.objectContaining({ type: "send_prompt", text: "/__dashboard_retry" }), + ); expect(send).not.toHaveBeenCalledWith(expect.objectContaining({ text: "503 overloaded" })); expect(getStates().get("s1")!.messages).toBe(beforeMessages); expect(getStates().get("s1")!.retryState).toBeUndefined(); }); + // test-plan #2: decision table over the 4 guard flags. Exactly one eligible + // baseline dispatches; every ineligible state dispatches zero. + it("#2 stale-click guard blocks retry_session in every ineligible state", () => { + const withLastError = (over: Partial): SessionState => ({ + ...idle(), + lastError: { message: "503 overloaded", timestamp: 1 }, + ...over, + }); + const cases: Array<{ name: string; state: SessionState }> = [ + { name: "lastError absent", state: { ...idle(), lastError: undefined } }, + { name: "retryState set", state: withLastError({ retryState: { attempt: 1, maxAttempts: 3, startedAt: 0, waiting: false, delayMs: 0 } as any }) }, + { name: "retryCancelled true", state: withLastError({ retryCancelled: true }) }, + { name: "isStreaming true", state: withLastError({ isStreaming: true }) }, + ]; + for (const c of cases) { + const { actions, send } = setup("s1", new Map([["s1", c.state]])); + actions.handleRetrySession("s1"); + expect(send, `ineligible: ${c.name}`).not.toHaveBeenCalled(); + } + // Eligible baseline: exactly one dispatch. + const { actions, send } = setup("s1", new Map([["s1", withLastError({})]])); + actions.handleRetrySession("s1"); + expect(send).toHaveBeenCalledTimes(1); + expect(send).toHaveBeenCalledWith({ type: "retry_session", sessionId: "s1" }); + }); + it("drops a stale Retry callback after the lifecycle has already recovered", () => { const state = idle(); state.lastError = { message: "503 overloaded", timestamp: 1 }; diff --git a/packages/client/src/hooks/useMessageHandler.ts b/packages/client/src/hooks/useMessageHandler.ts index 6bee444c6..cf4a87f14 100644 --- a/packages/client/src/hooks/useMessageHandler.ts +++ b/packages/client/src/hooks/useMessageHandler.ts @@ -1051,6 +1051,33 @@ export function useMessageHandler( } break; + case "retry_session_error": { + // Delivery failed (unknown/disconnected session, or a bridge lacking + // the handler). The retry never reached a bridge, so no agent_start / + // lastError change will self-heal the disabled one-shot Retry. Re-stamp + // lastError.timestamp to bump `retryRevision`, which resets the banner's + // one-shot guard and re-enables the button. Same re-enable mechanism as + // the auto_retry_end reducer path. See change: + // replace-dashboard-retry-command-with-protocol-message. + setSessionStates((prev) => { + const current = prev.get(msg.sessionId); + if (!current?.lastError) return prev; + const previousRevision = current.lastError.timestamp; + const nextRevision = + typeof previousRevision === "number" && Number.isFinite(previousRevision) + ? Math.max(Date.now(), previousRevision + 1) + : Date.now(); + const next = new Map(prev); + next.set(msg.sessionId, { + ...current, + lastError: { ...current.lastError, timestamp: nextRevision }, + }); + return next; + }); + showToast?.(msg.error, "error"); + break; + } + case "spawn_error": { // Enriches the spawn_result error with strategy + optional stderr tail. // Carried as its own message so esbuild preserves this switch case in diff --git a/packages/client/src/hooks/useSessionActions.ts b/packages/client/src/hooks/useSessionActions.ts index 8af71a9fe..0ccfe4cee 100644 --- a/packages/client/src/hooks/useSessionActions.ts +++ b/packages/client/src/hooks/useSessionActions.ts @@ -279,9 +279,12 @@ export function useSessionActions(deps: SessionActionDeps) { const current = sessionStatesRef.current.get(sessionId); if (!current?.lastError || current.retryState || current.retryCancelled || current.isStreaming) return; // Active settled sessions cannot use resume_session (that path safely - // rejects live carriers). The hidden command is intercepted by the bridge - // and starts a non-user custom turn via pi.sendMessage(triggerTurn:true). - send({ type: "send_prompt", sessionId, text: "/__dashboard_retry" }); + // rejects live carriers). The typed retry_session message is forwarded by + // the server to the owning bridge, which starts a non-user custom turn via + // pi.sendMessage(triggerTurn:true). Replaces the legacy send_prompt sentinel + // `/__dashboard_retry`. See change: + // replace-dashboard-retry-command-with-protocol-message. + send({ type: "retry_session", sessionId }); }, [send, sessionStatesRef]); const handleResumeSession = useCallback((sessionId: string, mode: "continue" | "fork", entryId?: string) => { diff --git a/packages/extension/src/__tests__/command-handler.test.ts b/packages/extension/src/__tests__/command-handler.test.ts index a068dfba8..05fcefc14 100644 --- a/packages/extension/src/__tests__/command-handler.test.ts +++ b/packages/extension/src/__tests__/command-handler.test.ts @@ -73,7 +73,26 @@ describe("CommandHandler", () => { ); }); - it("E7/E8 internal retry triggers a non-user custom turn", async () => { + it("retry_session triggers a non-user custom turn", async () => { + const pi = createMockPi(); + const handler = createCommandHandler(pi as any, "s1"); + + await handler.handle({ type: "retry_session", sessionId: "s1" } as ServerToExtensionMessage); + + expect(pi.sendMessage).toHaveBeenCalledOnce(); + expect(pi.sendMessage).toHaveBeenCalledWith( + expect.objectContaining({ customType: "pi-dashboard:retry", display: false }), + { triggerTurn: true }, + ); + expect(pi.sendUserMessage).not.toHaveBeenCalled(); + }); + + // test-plan #7: an un-upgraded client still sends the /__dashboard_retry + // send_prompt sentinel; the bridge routes it to the SAME retry dispatch and + // never replays it as a user message. Deprecated alias, kept for the + // version-skew window. See change: + // replace-dashboard-retry-command-with-protocol-message. + it("#7 legacy /__dashboard_retry sentinel still triggers a retry, no user replay", async () => { const pi = createMockPi(); const handler = createCommandHandler(pi as any, "s1"); @@ -91,7 +110,24 @@ describe("CommandHandler", () => { expect(pi.sendUserMessage).not.toHaveBeenCalled(); }); - it("reports a failed internal Retry dispatch without classifying it as abort", async () => { + // test-plan #6: the manual retry disarms the RetryTracker chain before the + // re-drive so its native agent_start is never mapped onto the auto-retry + // counter surface. + it("#6 retry_session disarms the retry chain before dispatch", async () => { + const pi = createMockPi(); + const disarmRetryChain = vi.fn(); + const handler = createCommandHandler(pi as any, "s1", { disarmRetryChain }); + + await handler.handle({ type: "retry_session", sessionId: "s1" } as ServerToExtensionMessage); + + expect(disarmRetryChain).toHaveBeenCalledOnce(); + expect(pi.sendMessage).toHaveBeenCalledOnce(); + }); + + // test-plan #4: a SYNCHRONOUS throw from pi.sendMessage surfaces as a single + // auto_retry_end{success:false, attempt:0}; no agent_start, not classified as + // an abort. + it("#4 retry_session sync dispatch failure emits auto_retry_end", async () => { const pi = createMockPi(); pi.sendMessage.mockImplementation(() => { throw new Error("retry dispatch failed"); @@ -99,11 +135,9 @@ describe("CommandHandler", () => { const eventSink = vi.fn(); const handler = createCommandHandler(pi as any, "s1", { eventSink }); - await expect(handler.handle({ - type: "send_prompt", - sessionId: "s1", - text: "/__dashboard_retry", - } as ServerToExtensionMessage)).resolves.toBeUndefined(); + await expect( + handler.handle({ type: "retry_session", sessionId: "s1" } as ServerToExtensionMessage), + ).resolves.toBeUndefined(); expect(eventSink).toHaveBeenCalledWith({ type: "event_forward", @@ -116,6 +150,56 @@ describe("CommandHandler", () => { }); }); + // test-plan #5: pi.sendMessage is async at runtime; an ASYNC rejection must + // ALSO emit auto_retry_end via the .catch() path (not only the sync + // try/catch), or the dispatch failure strands the retry surface. + it("#5 retry_session async rejection also emits auto_retry_end", async () => { + const pi = createMockPi(); + pi.sendMessage.mockImplementation(() => Promise.reject(new Error("async boom"))); + const eventSink = vi.fn(); + const handler = createCommandHandler(pi as any, "s1", { eventSink }); + + await handler.handle({ type: "retry_session", sessionId: "s1" } as ServerToExtensionMessage); + // Drain the microtask queue so the .catch() handler runs. + await new Promise((r) => setTimeout(r, 0)); + + expect(eventSink).toHaveBeenCalledWith({ + type: "event_forward", + sessionId: "s1", + event: { + eventType: "auto_retry_end", + timestamp: expect.any(Number), + data: { success: false, attempt: 0, finalError: "async boom" }, + }, + }); + }); + + // test-plan #10: a retry_session that lands while the session streams (guard + // bypassed) degrades to a single pi dispatch — pi queues it internally; the + // bridge never double-dispatches or corrupts state. + it("#10 retry_session while streaming dispatches exactly once", async () => { + const pi = createMockPi(); + const eventSink = vi.fn(); + const handler = createCommandHandler(pi as any, "s1", { + eventSink, + isStreaming: () => true, + }); + + await handler.handle({ type: "retry_session", sessionId: "s1" } as ServerToExtensionMessage); + + expect(pi.sendMessage).toHaveBeenCalledOnce(); + expect(pi.sendMessage).toHaveBeenCalledWith( + expect.objectContaining({ customType: "pi-dashboard:retry", display: false }), + { triggerTurn: true }, + ); + // No failure event on a successful (queued) dispatch. + expect(eventSink).not.toHaveBeenCalledWith( + expect.objectContaining({ + event: expect.objectContaining({ eventType: "auto_retry_end" }), + }), + ); + }); + it("should ignore messages for different sessionIds", async () => { const pi = createMockPi(); const handler = createCommandHandler(pi as any, "s1"); diff --git a/packages/extension/src/__tests__/retry-tracker.test.ts b/packages/extension/src/__tests__/retry-tracker.test.ts index 1c98cbbbf..3fed6b890 100644 --- a/packages/extension/src/__tests__/retry-tracker.test.ts +++ b/packages/extension/src/__tests__/retry-tracker.test.ts @@ -55,6 +55,23 @@ describe("RetryTracker (observe-based, agent_end/agent_settled model)", () => { expect(t.observeAgentStart("s1")).toBeNull(); }); + // test-plan #6: a manual dashboard retry (retry_session / legacy sentinel) + // disarms the chain via noteExplicitRun BEFORE its re-drive, so the manual + // turn's native agent_start is NOT converted into a synthetic + // auto_retry_start — no attempt counter renders for a user-initiated retry. + // See change: replace-dashboard-retry-command-with-protocol-message. + it("#6 an armed chain disarmed by noteExplicitRun yields no counter for the next agent_start", () => { + const t = new RetryTracker({ maxRetries: 3, baseDelayMs: 2000 }); + t.observeMessageEnd("s1", { ...errAssistant }); + t.observeAgentEnd("s1", errAgentEnd); // arms the next attempt + expect(t.isRetrying("s1")).toBe(true); + + t.noteExplicitRun("s1"); // the disarm the bridge performs before a manual retry + + expect(t.isRetrying("s1")).toBe(false); + expect(t.observeAgentStart("s1")).toBeNull(); + }); + it("increments delay geometrically across the chain", () => { const t = new RetryTracker({ maxRetries: 5, baseDelayMs: 2000 }); t.observeMessageEnd("s1", { ...errAssistant }); diff --git a/packages/extension/src/bridge.ts b/packages/extension/src/bridge.ts index 8e3ced517..afe7ae800 100644 --- a/packages/extension/src/bridge.ts +++ b/packages/extension/src/bridge.ts @@ -1458,6 +1458,10 @@ function initBridge(pi: ExtensionAPI) { // may be buffered rather than start a run, so retry cancellation is released // only by the observed user message_start below. noteUserPrompt: () => abortLatch.clear(sessionId), + // Disarm a still-armed provider-retry chain so a manual retry's native + // agent_start is not converted into a synthetic auto_retry_start. + // See change: replace-dashboard-retry-command-with-protocol-message. + disarmRetryChain: () => retryTracker.noteExplicitRun(sessionId), }); // Reload support: extension events only provide ExtensionContext (no reload). diff --git a/packages/extension/src/command-handler.ts b/packages/extension/src/command-handler.ts index dc3f72ea4..dd1874a8f 100644 --- a/packages/extension/src/command-handler.ts +++ b/packages/extension/src/command-handler.ts @@ -433,6 +433,14 @@ export function createCommandHandler( * unify-error-retry-lifecycle. */ noteUserPrompt?: () => void; + /** + * Disarm a still-armed provider-retry chain before a manual retry re-drive + * (bridge wires this to `retryTracker.noteExplicitRun`). Without it the + * manual turn's native `agent_start` is converted into a synthetic + * `auto_retry_start`, rendering pi's attempt counter for a user-initiated + * retry. See change: replace-dashboard-retry-command-with-protocol-message. + */ + disarmRetryChain?: () => void; }, ): CommandHandler { const getSessionId = typeof sessionIdOrGetter === "function" ? sessionIdOrGetter : () => sessionIdOrGetter; @@ -590,28 +598,13 @@ export function createCommandHandler( } if (parsed.type === "retry") { - try { - pi.sendMessage( - { - customType: "pi-dashboard:retry", - content: "Continue the interrupted response without repeating the user's request.", - display: false, - }, - { triggerTurn: true }, - ); - } catch (error) { - const finalError = errText(error); - console.error("[dashboard] Internal Retry dispatch failed:", finalError); - options?.eventSink?.({ - type: "event_forward", - sessionId, - event: { - eventType: "auto_retry_end", - timestamp: Date.now(), - data: { success: false, attempt: 0, finalError }, - }, - }); - } + // Deprecated `/__dashboard_retry` sentinel alias: an un-upgraded + // client still smuggles retry through send_prompt. Route it to the + // SAME dispatch as the typed retry_session, never replaying the + // sentinel as a user message. Remove one release after clients are + // known upgraded. See change: + // replace-dashboard-retry-command-with-protocol-message. + dispatchDashboardRetry(pi, sessionId, options); return undefined; } @@ -779,6 +772,13 @@ export function createCommandHandler( return undefined; } + case "retry_session": + // First-class settled-error retry. Same dispatch as the deprecated + // /__dashboard_retry sentinel, minus the send_prompt channel abuse. + // See change: replace-dashboard-retry-command-with-protocol-message. + dispatchDashboardRetry(pi, sessionId, options); + return undefined; + case "abort": // Pi owns both queues now. abort() asks pi to halt the current turn; // pi's native drain logic handles any remaining queue entries naturally. @@ -1151,6 +1151,57 @@ interface BashExecOptions { source?: "slash-exec"; } +/** + * Re-drive a settled-error turn via the pi custom-message primitive. Shared by + * the typed `retry_session` message and the deprecated `/__dashboard_retry` + * sentinel alias, so both converge on ONE dispatch. See change: + * replace-dashboard-retry-command-with-protocol-message. + */ +function dispatchDashboardRetry( + pi: ExtensionAPI, + sessionId: string, + options?: { + eventSink?: (msg: ExtensionToServerMessage) => void; + /** Disarm a still-armed RetryTracker chain (bridge → retryTracker.noteExplicitRun). */ + disarmRetryChain?: () => void; + }, +): void { + // Disarm any still-armed provider-retry chain BEFORE the re-drive. Otherwise + // the manual turn's native agent_start is converted into a synthetic + // auto_retry_start, rendering pi's attempt counter for a user-initiated retry. + options?.disarmRetryChain?.(); + const emitFailure = (error: unknown) => { + const finalError = errText(error); + console.error("[dashboard] Internal Retry dispatch failed:", finalError); + options?.eventSink?.({ + type: "event_forward", + sessionId, + event: { + eventType: "auto_retry_end", + timestamp: Date.now(), + data: { success: false, attempt: 0, finalError }, + }, + }); + }; + try { + // sendMessage is async at runtime (the .d.ts types it void). A SYNC throw is + // trapped here; an ASYNC rejection by the .catch() below. Both surface as a + // single auto_retry_end{success:false} so a failed dispatch never strands + // the retry surface. See change (spike caveat 1). + const result = pi.sendMessage( + { + customType: "pi-dashboard:retry", + content: "Continue the interrupted response without repeating the user's request.", + display: false, + }, + { triggerTurn: true }, + ) as unknown as Promise | void; + void Promise.resolve(result).catch(emitFailure); + } catch (error) { + emitFailure(error); + } +} + /** Execute a bash command and forward results */ async function handleBashCommand( pi: ExtensionAPI, diff --git a/packages/server/src/browser-handlers/__tests__/session-action-handler.test.ts b/packages/server/src/browser-handlers/__tests__/session-action-handler.test.ts index 42534bcad..3f3886b20 100644 --- a/packages/server/src/browser-handlers/__tests__/session-action-handler.test.ts +++ b/packages/server/src/browser-handlers/__tests__/session-action-handler.test.ts @@ -7,6 +7,7 @@ import { describe, expect, it } from "vitest"; import type { BrowserHandlerContext } from "../handler-context.js"; import { + handleRetrySession, handleSendPrompt, handleStopAfterTurn, handleSubagentResyncRequest, @@ -101,6 +102,57 @@ describe("handleSendPrompt retry routing", () => { }); }); +// test-plan #3: the first-class retry_session is forwarded to the owning bridge +// via an explicit gateway case; on undeliverable session it emits a structured +// retry_session_error to the sender (never a silent drop). See change: +// replace-dashboard-retry-command-with-protocol-message. +describe("handleRetrySession", () => { + it("#3 forwards retry_session to the owning bridge, not the unknown-type default", () => { + const sent: { sessionId: string; msg: unknown }[] = []; + const ctx = { + ws: { id: "ws1" }, + piGateway: { + sendToSession(sessionId: string, msg: unknown) { + sent.push({ sessionId, msg }); + return true; + }, + }, + sendTo() { + throw new Error("must not negative-ack on a delivered retry_session"); + }, + } as unknown as BrowserHandlerContext; + + handleRetrySession({ type: "retry_session", sessionId: "s1" }, ctx); + + expect(sent).toEqual([ + { sessionId: "s1", msg: { type: "retry_session", sessionId: "s1" } }, + ]); + }); + + it("emits retry_session_error to the sender when no bridge is reachable", () => { + const errs: { target: unknown; msg: any }[] = []; + const ws = { id: "ws1" }; + const ctx = { + ws, + piGateway: { + sendToSession() { + return false; + }, + }, + sendTo(target: unknown, msg: unknown) { + errs.push({ target, msg }); + }, + } as unknown as BrowserHandlerContext; + + handleRetrySession({ type: "retry_session", sessionId: "gone" }, ctx); + + expect(errs).toHaveLength(1); + expect(errs[0].target).toBe(ws); + expect(errs[0].msg).toMatchObject({ type: "retry_session_error", sessionId: "gone" }); + expect(typeof errs[0].msg.error).toBe("string"); + }); +}); + describe("handleStopAfterTurn", () => { it("forwards stop_after_turn to the bridge with the matching shape", () => { const { ctx, sent } = makeCtx(); diff --git a/packages/server/src/browser-handlers/session-action-handler.ts b/packages/server/src/browser-handlers/session-action-handler.ts index 53cf016e9..e108ab149 100644 --- a/packages/server/src/browser-handlers/session-action-handler.ts +++ b/packages/server/src/browser-handlers/session-action-handler.ts @@ -939,6 +939,31 @@ export function handleAbort( ctx.piGateway.sendToSession(msg.sessionId, { type: "abort", sessionId: msg.sessionId }); } +/** + * Forward a browser `retry_session` to the owning session bridge. On an + * undeliverable session (unknown or no reachable bridge — `sendToSession` + * returns false) emit a structured `retry_session_error` back to the sender, + * mirroring the `plugin_action_error` "never a silent drop" convention. The + * client re-enables the one-shot Retry + toasts on it. + * See change: replace-dashboard-retry-command-with-protocol-message. + */ +export function handleRetrySession( + msg: Extract, + ctx: BrowserHandlerContext, +): void { + const delivered = ctx.piGateway.sendToSession(msg.sessionId, { + type: "retry_session", + sessionId: msg.sessionId, + }); + if (!delivered) { + ctx.sendTo(ctx.ws, { + type: "retry_session_error", + sessionId: msg.sessionId, + error: `Cannot retry: session ${msg.sessionId} has no reachable bridge`, + }); + } +} + /** * Graceful stop-after-turn: forward to the bridge, which sets a per-session * flag and shuts down cleanly at the next turn_end. Distinct from abort diff --git a/packages/server/src/pairing/browser-gateway.ts b/packages/server/src/pairing/browser-gateway.ts index 51b10fd30..1c6db4f07 100644 --- a/packages/server/src/pairing/browser-gateway.ts +++ b/packages/server/src/pairing/browser-gateway.ts @@ -68,7 +68,7 @@ export function buildOpenSpecConnectSnapshot( import { handleAddFolderToWorkspace, handleCreateWorkspace, handleDeleteWorkspace, handleExtensionUiResponse, handleFavoriteModel, handleMoveFolderToWorkspace, handleOpenSpecBulkArchive, handleOpenSpecRefresh, handlePiGatewayForward, handlePinDirectory, handleRemoveFolderFromWorkspace, handleRenameWorkspace, handleReorderPinnedDirs, handleReorderSessions, handleReorderWorkspaceFolders, handleReorderWorkspaces, handleSetWorkspaceCollapsed, handleUnfavoriteModel, handleUnpinDirectory } from "../browser-handlers/directory-handler.js"; import type { BrowserHandlerContext } from "../browser-handlers/handler-context.js"; -import { handleAbort, handleClearFollowupEntries, handleEditFollowupEntry, handleFlowControl, handleForceKill, handleKillProcess, handlePromoteFollowupEntry, handleRemoveFollowupEntry, handleResumeSession, handleSendPrompt, handleShutdown, handleSpawnSession, handleStopAfterTurn, handleSubagentResyncRequest, shutdownSession as shutdownSessionImpl } from "../browser-handlers/session-action-handler.js"; +import { handleAbort, handleClearFollowupEntries, handleEditFollowupEntry, handleFlowControl, handleForceKill, handleKillProcess, handlePromoteFollowupEntry, handleRemoveFollowupEntry, handleResumeSession, handleRetrySession, handleSendPrompt, handleShutdown, handleSpawnSession, handleStopAfterTurn, handleSubagentResyncRequest, shutdownSession as shutdownSessionImpl } from "../browser-handlers/session-action-handler.js"; import { handleAcceptReplaceProposal, handleAttachProposal, handleDetachProposal, handleDismissReplaceProposal, handleFetchContent, handleHideSession, handleListSessions, handleRemoveTagGlobally, handleRenameSession, handleSetSessionDisplayPrefs, handleSetSessionProcessDrawer, handleSetSessionTags, handleUnhideSession } from "../browser-handlers/session-meta-handler.js"; import { clearGapState, handleHistoryBackfill, handleSubscribe } from "../browser-handlers/subscription-handler.js"; import { handleCloseInlineTerminal, handleCreateTerminal, handleKillTerminal, handleOpenInlineTerminal, handleRenameTerminal } from "../browser-handlers/terminal-handler.js"; @@ -696,6 +696,13 @@ export function createBrowserGateway( case "abort": handleAbort(msg, ctx); break; + // First-class settled-error retry. MUST be an explicit case: the + // default forwarder drops unknown types, so a bare union addition + // would let the server silently swallow the message. See change: + // replace-dashboard-retry-command-with-protocol-message. + case "retry_session": + handleRetrySession(msg, ctx); + break; case "stop_after_turn": if (typeof msg.sessionId === "string" && msg.sessionId.length > 0) { handleStopAfterTurn(msg, ctx); diff --git a/packages/shared/src/browser-protocol.ts b/packages/shared/src/browser-protocol.ts index 793f3c00d..105a309ac 100644 --- a/packages/shared/src/browser-protocol.ts +++ b/packages/shared/src/browser-protocol.ts @@ -847,6 +847,20 @@ export interface PluginActionErrorMessage { error: string; } +/** + * Server → browser: a `retry_session` could not be delivered (unknown or + * disconnected session, or a bridge lacking the handler). Structured + * negative-ack, mirroring `plugin_action_error` — never a silent drop. The + * client re-enables the one-shot Retry control and surfaces a toast on receipt. + * See change: replace-dashboard-retry-command-with-protocol-message. + */ +export interface RetrySessionErrorMessage { + type: "retry_session_error"; + sessionId: string; + /** Human-readable error description. */ + error: string; +} + /** Sent when a plugin's config changes; carries only that plugin's namespace. */ export interface PluginConfigUpdateMessage { type: "plugin_config_update"; @@ -951,6 +965,7 @@ export type ServerToBrowserMessage = | RecoveryOfferMessage | PluginConfigUpdateMessage | PluginActionErrorMessage + | RetrySessionErrorMessage | SessionAddedMessage | SessionUpdatedMessage | SessionRemovedMessage @@ -1098,6 +1113,20 @@ export interface AbortToBrowserMessage { sessionId: string; } +/** + * Browser → server: re-drive a settled-error turn as a first-class protocol + * message. Replaces the legacy `send_prompt` sentinel `/__dashboard_retry`, + * which smuggled a control signal through the user-prompt channel. The server + * forwards a `retry_session` to the owning bridge, which re-drives the turn via + * `pi.sendMessage({ customType: "pi-dashboard:retry", display: false }, + * { triggerTurn: true })`. See change: + * replace-dashboard-retry-command-with-protocol-message. + */ +export interface RetrySessionBrowserMessage { + type: "retry_session"; + sessionId: string; +} + // ── Follow-up queue mutation (bridge-owned buffer) ────────────────── // // Pi's ExtensionAPI (verified through 0.76.0) exposes no queue-mutation @@ -1712,6 +1741,7 @@ export type BrowserToServerMessage = | HistoryBackfillRequestMessage | BrowserExtensionUiResponseMessage | SendPromptToBrowserMessage + | RetrySessionBrowserMessage | AbortToBrowserMessage | RequestCommandsToBrowserMessage | FetchContentMessage diff --git a/packages/shared/src/protocol.ts b/packages/shared/src/protocol.ts index dbf0fc368..f7f788401 100644 --- a/packages/shared/src/protocol.ts +++ b/packages/shared/src/protocol.ts @@ -779,6 +779,19 @@ export interface AbortToExtensionMessage { sessionId: string; } +/** + * Server → extension: re-drive a settled-error turn. Forwarded by the server + * gateway from a browser `retry_session`. The bridge re-drives the turn via + * `pi.sendMessage({ customType: "pi-dashboard:retry", display: false }, + * { triggerTurn: true })` — the same pi call the legacy `/__dashboard_retry` + * sentinel made. See change: + * replace-dashboard-retry-command-with-protocol-message. + */ +export interface RetrySessionExtensionMessage { + type: "retry_session"; + sessionId: string; +} + export interface RequestCommandsMessage { type: "request_commands"; sessionId: string; @@ -1116,6 +1129,7 @@ export type AutoNamerStopState = Pick< export type ServerToExtensionMessage = | AutoNameStateRestoreMessage | SendPromptToExtensionMessage + | RetrySessionExtensionMessage | AbortToExtensionMessage | ExtensionUiResponseMessage | RequestCommandsMessage From d6e6ae3c53dc2a868652e6efc3f233450dae0dbd Mon Sep 17 00:00:00 2001 From: molnar-botond Date: Mon, 24 Aug 2026 13:33:47 +0200 Subject: [PATCH 4/5] chore(replace-dashboard-retry-command-with-protocol-message): archive change + review-fix tests - Archive the change (sync 4 ADDED requirements into the session-status-banner main spec; move proposal/specs/tasks/test-plan to archive/2026-08-24-...). - Reclassify the three retry requirements MODIFIED -> ADDED in the delta: the main spec had no retry-dispatch requirement to modify (only retry UI/render), so the typed-transport, manual-surface-guard, and negative-ack contracts are new. No behaviour/scope change. - Fold local-review (luna+terra) non-blocking findings: assert single auto_retry_end emission on sync throw AND async rejection; add a direct useMessageHandler retry_session_error reducer test; add a gateway-boundary test proving retry_session is explicitly routed (not swallowed by default). --- .../proposal.md | 0 .../specs/session-status-banner/spec.md | 4 +- .../tasks.md | 2 +- .../test-plan.md | 0 openspec/specs/session-status-banner/spec.md | 90 ++++++++++++++++++ ...essageHandler.retry-session-error.test.tsx | 77 ++++++++++++++++ .../src/__tests__/command-handler.test.ts | 8 ++ .../browser-gateway-retry-session.test.ts | 91 +++++++++++++++++++ 8 files changed, 268 insertions(+), 4 deletions(-) rename openspec/changes/{replace-dashboard-retry-command-with-protocol-message => archive/2026-08-24-replace-dashboard-retry-command-with-protocol-message}/proposal.md (100%) rename openspec/changes/{replace-dashboard-retry-command-with-protocol-message => archive/2026-08-24-replace-dashboard-retry-command-with-protocol-message}/specs/session-status-banner/spec.md (99%) rename openspec/changes/{replace-dashboard-retry-command-with-protocol-message => archive/2026-08-24-replace-dashboard-retry-command-with-protocol-message}/tasks.md (96%) rename openspec/changes/{replace-dashboard-retry-command-with-protocol-message => archive/2026-08-24-replace-dashboard-retry-command-with-protocol-message}/test-plan.md (100%) create mode 100644 packages/client/src/hooks/__tests__/useMessageHandler.retry-session-error.test.tsx create mode 100644 packages/server/src/__tests__/browser-gateway-retry-session.test.ts diff --git a/openspec/changes/replace-dashboard-retry-command-with-protocol-message/proposal.md b/openspec/changes/archive/2026-08-24-replace-dashboard-retry-command-with-protocol-message/proposal.md similarity index 100% rename from openspec/changes/replace-dashboard-retry-command-with-protocol-message/proposal.md rename to openspec/changes/archive/2026-08-24-replace-dashboard-retry-command-with-protocol-message/proposal.md diff --git a/openspec/changes/replace-dashboard-retry-command-with-protocol-message/specs/session-status-banner/spec.md b/openspec/changes/archive/2026-08-24-replace-dashboard-retry-command-with-protocol-message/specs/session-status-banner/spec.md similarity index 99% rename from openspec/changes/replace-dashboard-retry-command-with-protocol-message/specs/session-status-banner/spec.md rename to openspec/changes/archive/2026-08-24-replace-dashboard-retry-command-with-protocol-message/specs/session-status-banner/spec.md index 99d91bd89..975c8ad29 100644 --- a/openspec/changes/replace-dashboard-retry-command-with-protocol-message/specs/session-status-banner/spec.md +++ b/openspec/changes/archive/2026-08-24-replace-dashboard-retry-command-with-protocol-message/specs/session-status-banner/spec.md @@ -1,6 +1,6 @@ # session-status-banner (delta) -## MODIFIED Requirements +## ADDED Requirements ### Requirement: Settled-error Retry is dispatched as a typed protocol message @@ -78,8 +78,6 @@ surface a toast on receipt. - **THEN** it SHALL send `retry_session_error { sessionId, error }` to the sender - **AND** the client SHALL re-enable Retry and toast the error -## ADDED Requirements - ### Requirement: The /__dashboard_retry sentinel remains a deprecated alias For backward compatibility with un-upgraded clients during a version-skew diff --git a/openspec/changes/replace-dashboard-retry-command-with-protocol-message/tasks.md b/openspec/changes/archive/2026-08-24-replace-dashboard-retry-command-with-protocol-message/tasks.md similarity index 96% rename from openspec/changes/replace-dashboard-retry-command-with-protocol-message/tasks.md rename to openspec/changes/archive/2026-08-24-replace-dashboard-retry-command-with-protocol-message/tasks.md index c02a55db7..e43766c0d 100644 --- a/openspec/changes/replace-dashboard-retry-command-with-protocol-message/tasks.md +++ b/openspec/changes/archive/2026-08-24-replace-dashboard-retry-command-with-protocol-message/tasks.md @@ -33,7 +33,7 @@ - [~] 5.11 DEFERRED to ship-it (L3 e2e, docker harness). Same reason as 5.8 — no stateful faux retry scenario; nearest `error-lifecycle.spec.ts` is stale (written for the removed single-card design). Happy-path: click Retry re-drives and completes. input: settled overloaded_error banner · trigger: click Retry · observable: new turn streams, banner hides on success, no injected user message. see nearest `tests/e2e/` retry/banner spec (test-plan #11). ## 6. Manual verification (deferred post-merge) -- [ ] 6.1 Old-server version skew: new client + pre-`retry_session` server → Retry drops, button stays disabled; confirm the co-versioned deploy (`/api/restart` + `npm run reload`) closes it. (test-plan: manual-only #12). +- [x] 6.1 (manual-only #12, validated post-merge) Old-server version skew: new client + pre-`retry_session` server → Retry drops, button stays disabled; confirm the co-versioned deploy (`/api/restart` + `npm run reload`) closes it. (test-plan: manual-only #12). ## 7. Validate - [x] 7.1 `npm test` green for the touched packages (shared, server, client, extension) — 194 focused tests + all touched-package suites pass. The 8 repo-wide failures are pre-existing worktree-env issues (missing `@earendil-works/pi-coding-agent`, `pi-dashboard-cost-estimator`, `node_modules/.bin/tsc`, fs.watch attach behavior), none in this diff. diff --git a/openspec/changes/replace-dashboard-retry-command-with-protocol-message/test-plan.md b/openspec/changes/archive/2026-08-24-replace-dashboard-retry-command-with-protocol-message/test-plan.md similarity index 100% rename from openspec/changes/replace-dashboard-retry-command-with-protocol-message/test-plan.md rename to openspec/changes/archive/2026-08-24-replace-dashboard-retry-command-with-protocol-message/test-plan.md diff --git a/openspec/specs/session-status-banner/spec.md b/openspec/specs/session-status-banner/spec.md index a220d491f..8ea466d7c 100644 --- a/openspec/specs/session-status-banner/spec.md +++ b/openspec/specs/session-status-banner/spec.md @@ -346,3 +346,93 @@ survives `prefers-reduced-motion` and greyscale. - **GIVEN** the user has `prefers-reduced-motion: reduce` set - **THEN** the attempt number SHALL remain readable as text +### Requirement: Settled-error Retry is dispatched as a typed protocol message + +The settled-error **Retry** action SHALL be dispatched as a first-class +`retry_session` protocol message `{ type: "retry_session"; sessionId: string }`, +NOT by sending a `send_prompt` whose `text` is the sentinel `/__dashboard_retry`. +The message SHALL traverse three hops, each of which SHALL carry the type: the +browser→server union (`browser-protocol.ts`), the server→bridge union +(`protocol.ts` `ServerToExtensionMessage`), and the server gateway routing that +forwards a browser `retry_session` to the owning session bridge. The underlying +pi call the bridge makes SHALL remain +`pi.sendMessage({ customType: "pi-dashboard:retry", display: false }, +{ triggerTurn: true })` — this change alters the transport, not the pi call. + +The client SHALL preserve the pre-dispatch stale-click guard: it SHALL NOT +dispatch when `lastError` is absent, or `retryState` is set, or `retryCancelled` +is set, or `isStreaming` is true. + +#### Scenario: Client dispatches retry_session, not the sentinel prompt +- **GIVEN** a session with `lastError` set, `retryState` undefined, + `retryCancelled` false, `isStreaming` false +- **WHEN** the user activates the settled-error Retry control +- **THEN** the client SHALL send `{ type: "retry_session", sessionId }` +- **AND** it SHALL NOT send a `send_prompt` carrying `/__dashboard_retry` + +#### Scenario: Stale-click guard blocks dispatch in every ineligible state +- **WHEN** Retry is activated while ANY of: `lastError` absent, `retryState` + set, `retryCancelled` set, or `isStreaming` true +- **THEN** the client SHALL send no `retry_session` message + +#### Scenario: Server forwards retry_session to the owning bridge +- **GIVEN** a browser `retry_session { sessionId }` for a live, bridged session +- **WHEN** the server gateway receives it +- **THEN** it SHALL forward a `retry_session` to that session's bridge +- **AND** it SHALL NOT drop it through the unknown-type default path + +#### Scenario: Bridge re-drives the turn via the custom-message primitive +- **GIVEN** the bridge receives `retry_session` for an idle settled session +- **WHEN** it handles the message +- **THEN** it SHALL call `pi.sendMessage({ customType: "pi-dashboard:retry", + display: false }, { triggerTurn: true })` +- **AND** a native `agent_start` for the re-driven turn SHALL follow +- **AND** no user message SHALL be appended or replayed + +### Requirement: A manual retry is not mapped onto the auto-retry surface + +A `retry_session`-initiated turn SHALL NOT render pi's auto-retry attempt +counter. The bridge SHALL guard against a still-armed `RetryTracker` chain +converting the manual turn's `agent_start` into a synthetic `auto_retry_start`. + +#### Scenario: Armed tracker chain does not synthesize a counter for a manual retry +- **GIVEN** a `RetryTracker` chain is still armed for the session +- **WHEN** the manual `retry_session` turn emits `agent_start` +- **THEN** the bridge SHALL NOT forward a synthetic `auto_retry_start` for it +- **AND** no attempt counter SHALL render on the banner + +#### Scenario: Dispatch failure surfaces as an error, not a counter +- **GIVEN** `pi.sendMessage` throws synchronously OR rejects asynchronously +- **WHEN** the bridge handles the failure +- **THEN** it SHALL forward `auto_retry_end { success: false, attempt: 0, + finalError }` +- **AND** the banner SHALL show the error with no attempt counter + +### Requirement: Undeliverable retry is negatively acked, never silently dropped + +When the server or bridge cannot deliver a `retry_session` (unknown or +disconnected session, or a bridge lacking the handler), it SHALL emit a +structured `retry_session_error` to the sender (mirroring `plugin_action_error`), +never a silent drop. The client SHALL re-enable the one-shot Retry control and +surface a toast on receipt. + +#### Scenario: Unknown/disconnected session yields a structured error +- **GIVEN** a `retry_session` for a session with no reachable bridge +- **WHEN** the server processes it +- **THEN** it SHALL send `retry_session_error { sessionId, error }` to the sender +- **AND** the client SHALL re-enable Retry and toast the error + +### Requirement: The /__dashboard_retry sentinel remains a deprecated alias + +For backward compatibility with un-upgraded clients during a version-skew +window, the bridge SHALL continue to accept a `send_prompt` whose `text` equals +`/__dashboard_retry` and route it to the same retry handler as `retry_session`. +The alias SHALL NOT be removed in this change; removal is a separate change after +clients are known upgraded. + +#### Scenario: Legacy sentinel still triggers a retry +- **GIVEN** an older client sends `send_prompt { text: "/__dashboard_retry" }` +- **WHEN** the bridge parses it +- **THEN** it SHALL invoke the same retry dispatch as `retry_session` +- **AND** it SHALL NOT append or replay the sentinel as a user message + diff --git a/packages/client/src/hooks/__tests__/useMessageHandler.retry-session-error.test.tsx b/packages/client/src/hooks/__tests__/useMessageHandler.retry-session-error.test.tsx new file mode 100644 index 000000000..656a38c3a --- /dev/null +++ b/packages/client/src/hooks/__tests__/useMessageHandler.retry-session-error.test.tsx @@ -0,0 +1,77 @@ +/** + * Suite for change: replace-dashboard-retry-command-with-protocol-message. + * + * Pins the `retry_session_error` handling in useMessageHandler (test-plan #9, + * reducer half): a toast is shown, and the session's `lastError.timestamp` is + * re-stamped to a strictly-greater revision so the SessionBanner's one-shot + * Retry re-enables. When the session has no `lastError`, the handler is a + * no-op on state (nothing to re-enable) but still toasts. + */ +import type { ServerToBrowserMessage } from "@blackbelt-technology/pi-dashboard-shared/browser-protocol.js"; +import { renderHook } from "@testing-library/react"; +import { describe, expect, it, vi } from "vitest"; +import { createInitialState, type SessionState } from "../../lib/chat/event-reducer.js"; +import { useMessageHandler } from "../useMessageHandler.js"; + +function setup(initial?: Map) { + let states = initial ?? new Map(); + const setSessionStates = vi.fn((updater: any) => { + states = typeof updater === "function" ? updater(states) : updater; + }); + const showToast = vi.fn(); + const setters: any = { + setSessions: vi.fn(), setSessionStates, setSessionCommands: vi.fn(), + setFileResults: vi.fn(), setOpenspecMap: vi.fn(), setModelsMap: vi.fn(), + setRolesMap: vi.fn(), setSpawnResult: vi.fn(), setSessionOrderMap: vi.fn(), + setPinnedDirectories: vi.fn(), setFavoriteModels: vi.fn(), setTerminals: vi.fn(), + setEditorStatuses: vi.fn(), setDiscoveredServers: vi.fn(), setSpawnErrors: vi.fn(), + setResumeErrors: vi.fn(), setLoadingHistory: vi.fn(), + }; + const deps: any = { + send: vi.fn(), navigate: vi.fn(), clearSpawningCwd: vi.fn(), + spawningCwdsRef: { current: new Set() }, subscribedRef: { current: new Set() }, + pendingTerminalCwdRef: { current: null }, lastCreatedTerminalIdRef: { current: null }, + maxSeqMapRef: { current: new Map() }, selectedSessionIdRef: { current: undefined }, + pendingSpawnsRef: { current: new Map() }, loadingHistoryTimersRef: { current: new Map() }, + showToast, + }; + const { result } = renderHook(() => useMessageHandler(setters, deps)); + return { + dispatch: (m: ServerToBrowserMessage) => result.current(m), + showToast, + getStates: () => states, + }; +} + +function settled(timestamp: number): SessionState { + return { ...createInitialState(), lastError: { message: "503 overloaded", timestamp } }; +} + +describe("useMessageHandler retry_session_error", () => { + it("re-stamps lastError.timestamp to a strictly-greater revision and toasts", () => { + const prev = 100; + const { dispatch, showToast, getStates } = setup(new Map([["s1", settled(prev)]])); + + dispatch({ type: "retry_session_error", sessionId: "s1", error: "no reachable bridge" } as ServerToBrowserMessage); + + const next = getStates().get("s1")!; + // Revision strictly advances (re-enables the one-shot Retry) … + expect(next.lastError!.timestamp).toBeGreaterThan(prev); + // … while the error message itself is preserved. + expect(next.lastError!.message).toBe("503 overloaded"); + expect(showToast).toHaveBeenCalledTimes(1); + expect(showToast.mock.calls[0][0]).toBe("no reachable bridge"); + expect(showToast.mock.calls[0][1]).toBe("error"); + }); + + it("still toasts but leaves state untouched when the session has no lastError", () => { + const clean = createInitialState(); + const { dispatch, showToast, getStates } = setup(new Map([["s1", clean]])); + + dispatch({ type: "retry_session_error", sessionId: "s1", error: "gone" } as ServerToBrowserMessage); + + expect(getStates().get("s1")).toBe(clean); // referential no-op + expect(showToast).toHaveBeenCalledTimes(1); + expect(showToast.mock.calls[0][1]).toBe("error"); + }); +}); diff --git a/packages/extension/src/__tests__/command-handler.test.ts b/packages/extension/src/__tests__/command-handler.test.ts index 05fcefc14..b9e236deb 100644 --- a/packages/extension/src/__tests__/command-handler.test.ts +++ b/packages/extension/src/__tests__/command-handler.test.ts @@ -139,6 +139,10 @@ describe("CommandHandler", () => { handler.handle({ type: "retry_session", sessionId: "s1" } as ServerToExtensionMessage), ).resolves.toBeUndefined(); + const failures = eventSink.mock.calls.filter( + ([m]) => m?.event?.eventType === "auto_retry_end", + ); + expect(failures).toHaveLength(1); // exactly one — sync path must not also hit .catch() expect(eventSink).toHaveBeenCalledWith({ type: "event_forward", sessionId: "s1", @@ -163,6 +167,10 @@ describe("CommandHandler", () => { // Drain the microtask queue so the .catch() handler runs. await new Promise((r) => setTimeout(r, 0)); + const failures = eventSink.mock.calls.filter( + ([m]) => m?.event?.eventType === "auto_retry_end", + ); + expect(failures).toHaveLength(1); // exactly one — async path must not also hit sync try/catch expect(eventSink).toHaveBeenCalledWith({ type: "event_forward", sessionId: "s1", diff --git a/packages/server/src/__tests__/browser-gateway-retry-session.test.ts b/packages/server/src/__tests__/browser-gateway-retry-session.test.ts new file mode 100644 index 000000000..3ee466b4c --- /dev/null +++ b/packages/server/src/__tests__/browser-gateway-retry-session.test.ts @@ -0,0 +1,91 @@ +/** + * Gateway-boundary contract for the first-class settled-error retry. + * + * A browser `retry_session` MUST be routed by the gateway's explicit switch + * case to the owning session's bridge (`sendToSession`), NOT swallowed by the + * unknown-type default forwarder. On an undeliverable session (`sendToSession` + * returns false) the gateway emits a structured `retry_session_error` back to + * the sender — never a silent drop. This test exercises the real + * `createBrowserGateway` message loop, so it fails if the `case "retry_session"` + * is removed (unlike the direct-handler unit test). + * + * See change: replace-dashboard-retry-command-with-protocol-message. + */ +import { EventEmitter } from "node:events"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { createMemoryEventStore } from "../persistence/memory-event-store.js"; +import { createBrowserGateway } from "../pairing/browser-gateway.js"; +import type { PiGateway } from "../pi/pi-gateway.js"; +import { createMemorySessionManager } from "../session/memory-session-manager.js"; + +function makeFakeWs() { + const ws = new EventEmitter() as EventEmitter & { + send: ReturnType; + close: ReturnType; + readyState: number; + OPEN: number; + }; + ws.send = vi.fn(); + ws.close = vi.fn(); + ws.readyState = 1; + ws.OPEN = 1; + return ws; +} + +function makeGateway(sendToSession: PiGateway["sendToSession"]) { + const piGateway = { + start: vi.fn(), + stop: vi.fn(), + sendToSession, + getConnectedSessionIds: vi.fn(() => []), + hasSession: vi.fn(() => false), + onEvent: vi.fn(), + } as unknown as PiGateway; + return createBrowserGateway( + createMemorySessionManager(), + createMemoryEventStore(() => false), + piGateway, + ); +} + +async function deliver(ws: EventEmitter, msg: unknown) { + ws.emit("message", Buffer.from(JSON.stringify(msg))); + await new Promise((r) => setImmediate(r)); +} + +describe("gateway routing: retry_session", () => { + let errorSpy: ReturnType; + beforeEach(() => { + errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + }); + afterEach(() => errorSpy.mockRestore()); + + it("forwards retry_session to the owning bridge (explicit case, not the default)", async () => { + const sendToSession = vi.fn(() => true) as unknown as PiGateway["sendToSession"]; + const gateway = makeGateway(sendToSession); + const ws = makeFakeWs(); + gateway.wss.emit("connection", ws, {}); + + await deliver(ws, { type: "retry_session", sessionId: "s1" }); + + expect(sendToSession).toHaveBeenCalledWith("s1", { type: "retry_session", sessionId: "s1" }); + // Delivered → no negative-ack to the browser. + const sent = ws.send.mock.calls.map((c) => JSON.parse(c[0] as string)); + expect(sent.find((m) => m.type === "retry_session_error")).toBeUndefined(); + }); + + it("emits retry_session_error to the sender when the bridge is unreachable", async () => { + const sendToSession = vi.fn(() => false) as unknown as PiGateway["sendToSession"]; + const gateway = makeGateway(sendToSession); + const ws = makeFakeWs(); + gateway.wss.emit("connection", ws, {}); + + await deliver(ws, { type: "retry_session", sessionId: "gone" }); + + const sent = ws.send.mock.calls.map((c) => JSON.parse(c[0] as string)); + const err = sent.find((m) => m.type === "retry_session_error"); + expect(err, "expected a retry_session_error to the sender").toBeTruthy(); + expect(err).toMatchObject({ type: "retry_session_error", sessionId: "gone" }); + expect(typeof err.error).toBe("string"); + }); +}); From b6cbacb786fcfbb8b795e18910b2c8f40cd30731 Mon Sep 17 00:00:00 2001 From: molnar-botond Date: Mon, 24 Aug 2026 14:02:58 +0200 Subject: [PATCH 5/5] fix: validate retry_session.sessionId before dispatch (CodeRabbit) Mirror the adjacent stop_after_turn guard so a malformed retry_session payload is ignored rather than driving handleRetrySession with a bogus sessionId. --- packages/server/src/pairing/browser-gateway.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/server/src/pairing/browser-gateway.ts b/packages/server/src/pairing/browser-gateway.ts index 1c6db4f07..c735db07e 100644 --- a/packages/server/src/pairing/browser-gateway.ts +++ b/packages/server/src/pairing/browser-gateway.ts @@ -701,7 +701,13 @@ export function createBrowserGateway( // would let the server silently swallow the message. See change: // replace-dashboard-retry-command-with-protocol-message. case "retry_session": - handleRetrySession(msg, ctx); + // Validate the wire input before dispatch (JSON.parse does not check + // the discriminated union at runtime), mirroring the adjacent + // stop_after_turn guard. A malformed payload is ignored rather than + // driving a negative-ack with a bogus sessionId. + if (typeof msg.sessionId === "string" && msg.sessionId.length > 0) { + handleRetrySession(msg, ctx); + } break; case "stop_after_turn": if (typeof msg.sessionId === "string" && msg.sessionId.length > 0) {