fix(maestro): correct connection view race and approval decisions (#140)
Three fixes from acceptance review: - ConnectionPool.mount cached a connection's kind (terminal/structured) permanently from whatever `info` it saw first. A deep link or restored layout mounts before the catalog snapshot names a session's view, so `info` is undefined and a structured session got an xterm connection forever. The pool now tracks the view each cached connection was built for, subscribes to the catalog itself, and replaces a mistyped connection in place the moment the real view resolves — without downgrading a resolved connection just because a later caller couldn't resolve the session, and without treating a session merely absent from a snapshot (mid-removal) as "Terminal". - The approval card offered all three decisions unconditionally. Per the daemon's decision mapping (session.rs's permission_outcome), a decision with no matching PermissionOptionKind in the request's own options resolves Cancelled silently — so "Allow for session" could look like it worked while the daemon withdrew it. Buttons are now gated on the options actually offered. - Clicking a decision disabled the buttons and never re-enabled them if the POST failed. The decision path is async now: a rejection restores the buttons and surfaces the error only if the approval is still the pending one — a superseded/resolved approval's stale rejection is a no-op, not a wrongly revived control. Regression tests: 12 new (connections.test.ts, structured.test.ts, structuredRender.test.ts). 488 tests total.
This commit is contained in:
+137
-5
@@ -2,28 +2,78 @@
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
|
||||
import { ConnectionPool } from "./connections.js";
|
||||
import { CatalogStore } from "./events.js";
|
||||
import type { Session } from "./types.js";
|
||||
|
||||
// The pane's own bytes-out path, stubbed: the escape hatch only needs to know
|
||||
// the pool reaches the focused connection with them.
|
||||
const sent: { id: string; data: string }[] = [];
|
||||
vi.mock("./attach.js", () => ({
|
||||
createAttachConnection: (id: string) => ({
|
||||
const disposed: string[] = [];
|
||||
function fakeConn(kind: string, id: string) {
|
||||
const el = document.createElement("div");
|
||||
el.dataset.kind = kind;
|
||||
el.dataset.session = id;
|
||||
return {
|
||||
sessionId: id,
|
||||
el: document.createElement("div"),
|
||||
kind,
|
||||
el,
|
||||
onMount() {},
|
||||
onUnmount() {},
|
||||
focusTerminal() {},
|
||||
sendInput(data: string) {
|
||||
sent.push({ id, data });
|
||||
},
|
||||
dispose() {},
|
||||
}),
|
||||
dispose() {
|
||||
disposed.push(`${kind}:${id}`);
|
||||
},
|
||||
};
|
||||
}
|
||||
vi.mock("./attach.js", () => ({
|
||||
createAttachConnection: (id: string) => fakeConn("attach", id),
|
||||
}));
|
||||
vi.mock("./structured.js", () => ({
|
||||
createStructuredConnection: (id: string) => fakeConn("structured", id),
|
||||
}));
|
||||
|
||||
afterEach(() => {
|
||||
vi.unstubAllGlobals();
|
||||
sent.length = 0;
|
||||
disposed.length = 0;
|
||||
});
|
||||
|
||||
/** Minimal fixture for the `CatalogState` a `CatalogStore` broadcasts. */
|
||||
function session(id: string, view?: "Terminal" | "Structured"): Session {
|
||||
return {
|
||||
id,
|
||||
cwd: "/repo",
|
||||
command: "claude",
|
||||
env: {},
|
||||
metadata: {},
|
||||
status: "Running",
|
||||
created_at: "2026-01-01T00:00:00Z",
|
||||
view,
|
||||
};
|
||||
}
|
||||
|
||||
/** A `CatalogStore` an id can be pushed into on demand, mirroring
|
||||
* events.test.ts's own injectable-socket pattern. */
|
||||
class FakeCatalogSocket {
|
||||
onmessage: ((ev: { data: string }) => void) | null = null;
|
||||
onopen: (() => void) | null = null;
|
||||
onclose: (() => void) | null = null;
|
||||
onerror: (() => void) | null = null;
|
||||
close(): void {}
|
||||
push(sessions: Session[]): void {
|
||||
this.onmessage?.({ data: JSON.stringify({ type: "Snapshot", sessions }) });
|
||||
}
|
||||
}
|
||||
function fakeStore(): { store: CatalogStore; socket: FakeCatalogSocket } {
|
||||
const socket = new FakeCatalogSocket();
|
||||
const store = new CatalogStore({ socket: () => socket as unknown as WebSocket });
|
||||
store.start();
|
||||
return { store, socket };
|
||||
}
|
||||
|
||||
const A = "aaaaaaaa-0000-0000-0000-0000000000ff";
|
||||
const B = "bbbbbbbb-0000-0000-0000-0000000000ff";
|
||||
|
||||
@@ -125,3 +175,85 @@ describe("ConnectionPool.sendToFocused", () => {
|
||||
expect(sent).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
// A deep link or restored layout can `mount()` a session before the catalog
|
||||
// snapshot names its view — the daemon defaults an absent `view` to
|
||||
// `"Terminal"`, but so does an *unknown* session, and only one of those is
|
||||
// actually resolved (#140 review).
|
||||
describe("ConnectionPool: view resolution races", () => {
|
||||
it("replaces a wrongly-typed connection once the catalog resolves it, without waiting for another mount()", () => {
|
||||
mockFetch();
|
||||
const { store, socket } = fakeStore();
|
||||
const pool = new ConnectionPool({ store });
|
||||
const pane = document.createElement("div");
|
||||
|
||||
pool.mount(A, pane); // no info: deep-link-before-snapshot race
|
||||
expect(disposed).toEqual([]);
|
||||
|
||||
socket.push([session(A, "Structured")]);
|
||||
|
||||
expect(disposed).toEqual(["attach:" + A]);
|
||||
// The fresh connection landed in the same pane nothing re-mounted it into.
|
||||
expect(pane.children).toHaveLength(1);
|
||||
});
|
||||
|
||||
it("also repairs a wrongly-typed connection that is parked, not mounted, without evicting its other parked siblings", () => {
|
||||
mockFetch();
|
||||
const { store, socket } = fakeStore();
|
||||
const pool = new ConnectionPool({ store });
|
||||
const hidden = document.body.lastElementChild as HTMLElement; // this pool's own cache
|
||||
pool.mount(A, document.createElement("div"));
|
||||
pool.mount(B, document.createElement("div"));
|
||||
pool.unmount(A);
|
||||
pool.unmount(B);
|
||||
disposed.length = 0;
|
||||
|
||||
socket.push([session(A, "Structured")]);
|
||||
|
||||
expect(disposed).toEqual(["attach:" + A]);
|
||||
// B's own element is still parked — A's repair didn't wipe its siblings
|
||||
// out of the shared hidden cache.
|
||||
expect(hidden.querySelector(`[data-session="${B}"]`)).not.toBeNull();
|
||||
expect((hidden.querySelector(`[data-session="${A}"]`) as HTMLElement | null)?.dataset.kind).toBe("structured");
|
||||
});
|
||||
|
||||
it("does not replace a connection whose resolved view already matches", () => {
|
||||
mockFetch();
|
||||
const { store, socket } = fakeStore();
|
||||
const pool = new ConnectionPool({ store });
|
||||
const pane = document.createElement("div");
|
||||
pool.mount(A, pane, { name: "a", command: "claude", view: "Structured" });
|
||||
disposed.length = 0;
|
||||
|
||||
socket.push([session(A, "Structured")]);
|
||||
|
||||
expect(disposed).toEqual([]);
|
||||
});
|
||||
|
||||
it("does not downgrade an already-resolved connection when a later mount() call has no info", () => {
|
||||
mockFetch();
|
||||
const pool = new ConnectionPool();
|
||||
const pane = document.createElement("div");
|
||||
pool.mount(A, pane, { name: "a", command: "claude", view: "Structured" });
|
||||
disposed.length = 0;
|
||||
|
||||
pool.mount(A, pane); // e.g. a re-render before the catalog answers again
|
||||
|
||||
expect(disposed).toEqual([]);
|
||||
});
|
||||
|
||||
it("does not treat a session missing from the catalog as Terminal and churn a live connection", () => {
|
||||
mockFetch();
|
||||
const { store, socket } = fakeStore();
|
||||
const pool = new ConnectionPool({ store });
|
||||
const pane = document.createElement("div");
|
||||
pool.mount(A, pane, { name: "a", command: "claude", view: "Structured" });
|
||||
disposed.length = 0;
|
||||
|
||||
// `A` is absent from this snapshot — main.ts's own subscription handles
|
||||
// closing the pane; the pool must not race it by "fixing" the view first.
|
||||
socket.push([session(B, "Terminal")]);
|
||||
|
||||
expect(disposed).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
+88
-13
@@ -3,8 +3,8 @@ import type { AttachInfo } from "./paneTitle.js";
|
||||
import type { PaneConnection } from "./pane.js";
|
||||
import { createStructuredConnection } from "./structured.js";
|
||||
import { setSessionFocus } from "./api.js";
|
||||
import type { CatalogStore } from "./events.js";
|
||||
import type { Uuid } from "./types.js";
|
||||
import type { CatalogState, CatalogStore } from "./events.js";
|
||||
import { sessionName, type SessionView, type Uuid } from "./types.js";
|
||||
|
||||
/**
|
||||
* How many *parked* (off-screen but still-connected) sessions to keep warm
|
||||
@@ -16,6 +16,15 @@ import type { Uuid } from "./types.js";
|
||||
*/
|
||||
const MAX_PARKED = 6;
|
||||
|
||||
/** A session's view defaults to `"Terminal"` wherever it isn't carried —
|
||||
* matching the wire default (`SessionView`'s `#[default]`, types.ts). Only
|
||||
* meaningful once the session itself is known; an altogether-unknown
|
||||
* session (no `AttachInfo`/`Session` at all) is a distinct, un-resolved
|
||||
* state callers must check for separately, not "Terminal". */
|
||||
function resolveView(view: SessionView | undefined): SessionView {
|
||||
return view ?? "Terminal";
|
||||
}
|
||||
|
||||
export interface PoolOptions {
|
||||
/** Fired when the focused session changes (drives sidebar highlight + HUD). */
|
||||
onFocusChange?: (id: Uuid | null) => void;
|
||||
@@ -44,11 +53,18 @@ export interface PoolOptions {
|
||||
export class ConnectionPool {
|
||||
/** All live connections. Insertion order doubles as LRU: oldest first. */
|
||||
private readonly cache = new Map<Uuid, PaneConnection>();
|
||||
/** The view each cached connection was actually created for — a deep link
|
||||
* or a restored layout can `mount()` a session before the catalog
|
||||
* snapshot names its view, defaulting it to `"Terminal"`; this is what
|
||||
* lets a later reconciliation notice the resolved view disagrees (#140
|
||||
* review). */
|
||||
private readonly connViews = new Map<Uuid, SessionView>();
|
||||
/** Sessions currently mounted into a visible pane (never evicted). */
|
||||
private readonly mounted = new Set<Uuid>();
|
||||
private readonly hidden: HTMLElement;
|
||||
private focusedId: Uuid | null = null;
|
||||
private readonly opts: PoolOptions;
|
||||
private readonly unsubscribeStore?: () => void;
|
||||
|
||||
constructor(opts: PoolOptions = {}) {
|
||||
this.opts = opts;
|
||||
@@ -57,6 +73,11 @@ export class ConnectionPool {
|
||||
this.hidden.className = "conn-cache";
|
||||
this.hidden.setAttribute("aria-hidden", "true");
|
||||
document.body.appendChild(this.hidden);
|
||||
// A session `mount()`ed before its catalog entry exists gets a
|
||||
// `"Terminal"` connection by default; once the snapshot actually names
|
||||
// the view, fix any connection that guessed wrong — nothing else
|
||||
// re-`mount()`s an already-open pane just because the catalog updated.
|
||||
this.unsubscribeStore = opts.store?.subscribe((state) => this.reconcileViews(state));
|
||||
}
|
||||
|
||||
/** The focused session, or null when nothing is focused (welcome pane). */
|
||||
@@ -80,18 +101,18 @@ export class ConnectionPool {
|
||||
mount(id: Uuid, pane: HTMLElement, info?: AttachInfo, opts?: { focus?: boolean }): void {
|
||||
let conn = this.cache.get(id);
|
||||
if (conn) {
|
||||
this.cache.delete(id);
|
||||
this.cache.set(id, conn);
|
||||
// `info === undefined` means "session not known yet", not "Terminal" —
|
||||
// never downgrade a resolved connection just because a later caller
|
||||
// couldn't resolve the session. Only a *resolved* view that disagrees
|
||||
// with what this connection was built for forces a replace.
|
||||
if (info !== undefined && resolveView(info.view) !== this.connViews.get(id)) {
|
||||
conn = this.replace(id, info);
|
||||
} else {
|
||||
this.cache.delete(id);
|
||||
this.cache.set(id, conn); // LRU refresh
|
||||
}
|
||||
} else {
|
||||
const connOpts = {
|
||||
onRequestClose: (sid: Uuid) => this.opts.onRequestClose?.(sid),
|
||||
onClosed: (sid: Uuid) => this.opts.onClosed?.(sid),
|
||||
shouldReconnect: (sid: Uuid, code: number) => this.opts.shouldReconnect?.(sid, code) ?? false,
|
||||
};
|
||||
conn =
|
||||
info?.view === "Structured"
|
||||
? createStructuredConnection(id, connOpts, info, this.opts.store)
|
||||
: createAttachConnection(id, connOpts, info);
|
||||
conn = this.create(id, info);
|
||||
this.cache.set(id, conn);
|
||||
}
|
||||
if (conn.el.parentElement !== pane) {
|
||||
@@ -104,6 +125,57 @@ export class ConnectionPool {
|
||||
this.evict();
|
||||
}
|
||||
|
||||
private create(id: Uuid, info?: AttachInfo): PaneConnection {
|
||||
const view = resolveView(info?.view);
|
||||
this.connViews.set(id, view);
|
||||
const connOpts = {
|
||||
onRequestClose: (sid: Uuid) => this.opts.onRequestClose?.(sid),
|
||||
onClosed: (sid: Uuid) => this.opts.onClosed?.(sid),
|
||||
shouldReconnect: (sid: Uuid, code: number) => this.opts.shouldReconnect?.(sid, code) ?? false,
|
||||
};
|
||||
return view === "Structured"
|
||||
? createStructuredConnection(id, connOpts, info, this.opts.store)
|
||||
: createAttachConnection(id, connOpts, info);
|
||||
}
|
||||
|
||||
/** Dispose whatever is cached for `id` and cache a freshly created
|
||||
* connection in its place. Does not touch the DOM or `mounted` — callers
|
||||
* place the new element themselves. */
|
||||
private replace(id: Uuid, info?: AttachInfo): PaneConnection {
|
||||
this.cache.get(id)?.dispose();
|
||||
const fresh = this.create(id, info);
|
||||
this.cache.set(id, fresh);
|
||||
return fresh;
|
||||
}
|
||||
|
||||
/** Proactively fix any cached connection whose view no longer matches the
|
||||
* catalog — the only path that repairs a pane nothing will `mount()`
|
||||
* again on its own (parked, or mounted but not currently being
|
||||
* re-rendered by anything else). A session absent from `state` (not yet
|
||||
* known, or removed) is left alone — that's `main.ts`'s
|
||||
* `closePane`/eviction concern, not a view mismatch. */
|
||||
private reconcileViews(state: CatalogState): void {
|
||||
for (const id of [...this.cache.keys()]) {
|
||||
const session = state.sessions.get(id);
|
||||
if (session === undefined || resolveView(session.view) === this.connViews.get(id)) continue;
|
||||
const existing = this.cache.get(id)!;
|
||||
const parent = existing.el.parentElement;
|
||||
const wasMounted = this.mounted.has(id);
|
||||
const info: AttachInfo = { name: sessionName(session), command: session.command, view: session.view };
|
||||
const fresh = this.replace(id, info);
|
||||
existing.el.remove(); // don't rely on dispose() to clear the DOM
|
||||
if (parent && parent !== this.hidden) {
|
||||
parent.replaceChildren(fresh.el);
|
||||
} else {
|
||||
// Parked: `this.hidden` holds every parked connection's element as
|
||||
// siblings — append, never replaceChildren, or they'd all vanish.
|
||||
this.hidden.appendChild(fresh.el);
|
||||
}
|
||||
if (wasMounted) fresh.onMount({ focus: false });
|
||||
else fresh.onUnmount();
|
||||
}
|
||||
}
|
||||
|
||||
/** Park `id` back into the hidden cache (still connected) and stop showing it. */
|
||||
unmount(id: Uuid): void {
|
||||
if (!this.mounted.delete(id)) return;
|
||||
@@ -180,13 +252,16 @@ export class ConnectionPool {
|
||||
if (!conn) return;
|
||||
conn.dispose();
|
||||
this.cache.delete(id);
|
||||
this.connViews.delete(id);
|
||||
this.mounted.delete(id);
|
||||
if (this.focusedId === id) this.setFocus(null);
|
||||
}
|
||||
|
||||
dispose(): void {
|
||||
this.unsubscribeStore?.();
|
||||
for (const conn of this.cache.values()) conn.dispose();
|
||||
this.cache.clear();
|
||||
this.connViews.clear();
|
||||
this.mounted.clear();
|
||||
this.hidden.remove();
|
||||
}
|
||||
|
||||
@@ -45,6 +45,12 @@ function row(id: string, kind: TranscriptRow["kind"], data: unknown): Transcript
|
||||
|
||||
const idleState: ReducedState = { turn: { responding: false, thinking: false, active_tool_calls: [] } };
|
||||
|
||||
const allApprovalOptions = [
|
||||
{ optionId: "a1", name: "Allow once", kind: "allow_once" as const },
|
||||
{ optionId: "a2", name: "Allow always", kind: "allow_always" as const },
|
||||
{ optionId: "r1", name: "Reject once", kind: "reject_once" as const },
|
||||
];
|
||||
|
||||
function mount(opts: {
|
||||
sockets: FakeSocket[];
|
||||
shouldReconnect?: (id: string, code: number) => boolean;
|
||||
@@ -175,9 +181,9 @@ describe("structured connection: approvals", () => {
|
||||
type: "projection_update",
|
||||
transcript: {
|
||||
type: "append",
|
||||
row: row("approval:1", "approval", { toolCall: { title: "Write x" }, options: [] }),
|
||||
row: row("approval:1", "approval", { toolCall: { title: "Write x" }, options: allApprovalOptions }),
|
||||
},
|
||||
state: { ...idleState, pending_approval: { nonce: "n1", request: { sessionId: "s1", toolCall: {}, options: [] } } },
|
||||
state: { ...idleState, pending_approval: { nonce: "n1", request: { sessionId: "s1", toolCall: {}, options: allApprovalOptions } } },
|
||||
});
|
||||
const buttons = conn.el.querySelectorAll<HTMLButtonElement>(".st-approval-acts button");
|
||||
expect(buttons).toHaveLength(3);
|
||||
@@ -192,11 +198,11 @@ describe("structured connection: approvals", () => {
|
||||
ws.open();
|
||||
ws.emit({
|
||||
type: "transcript_snapshot",
|
||||
snapshot: { cursor: 0, rows: [row("approval:1", "approval", { toolCall: { title: "Write x" }, options: [] })] },
|
||||
snapshot: { cursor: 0, rows: [row("approval:1", "approval", { toolCall: { title: "Write x" }, options: allApprovalOptions })] },
|
||||
});
|
||||
ws.emit({
|
||||
type: "reduced_state",
|
||||
state: { ...idleState, pending_approval: { nonce: "n1", request: { sessionId: "s1", toolCall: {}, options: [] } } },
|
||||
state: { ...idleState, pending_approval: { nonce: "n1", request: { sessionId: "s1", toolCall: {}, options: allApprovalOptions } } },
|
||||
});
|
||||
expect(conn.el.querySelectorAll(".st-approval-acts button")).toHaveLength(3);
|
||||
// Resolved on another client: state clears with no transcript delta at all.
|
||||
@@ -212,11 +218,11 @@ describe("structured connection: approvals", () => {
|
||||
ws.open();
|
||||
ws.emit({
|
||||
type: "transcript_snapshot",
|
||||
snapshot: { cursor: 0, rows: [row("approval:1", "approval", { toolCall: {}, options: [] })] },
|
||||
snapshot: { cursor: 0, rows: [row("approval:1", "approval", { toolCall: {}, options: allApprovalOptions })] },
|
||||
});
|
||||
ws.emit({
|
||||
type: "reduced_state",
|
||||
state: { ...idleState, pending_approval: { nonce: "n1", request: { sessionId: "s1", toolCall: {}, options: [] } } },
|
||||
state: { ...idleState, pending_approval: { nonce: "n1", request: { sessionId: "s1", toolCall: {}, options: allApprovalOptions } } },
|
||||
});
|
||||
const buttons = conn.el.querySelectorAll<HTMLButtonElement>(".st-approval-acts button");
|
||||
// Approval is superseded before the click is handled (buttons already
|
||||
@@ -227,6 +233,65 @@ describe("structured connection: approvals", () => {
|
||||
expect(resolveStructuredApproval).not.toHaveBeenCalled();
|
||||
conn.dispose();
|
||||
});
|
||||
|
||||
it("restores the buttons and surfaces an error when the POST rejects while still pending", async () => {
|
||||
vi.mocked(resolveStructuredApproval).mockRejectedValueOnce(new Error("network down"));
|
||||
const ws = new FakeSocket();
|
||||
const conn = mount({ sockets: [ws] });
|
||||
ws.open();
|
||||
ws.emit({
|
||||
type: "transcript_snapshot",
|
||||
snapshot: { cursor: 0, rows: [row("approval:1", "approval", { toolCall: {}, options: allApprovalOptions })] },
|
||||
});
|
||||
ws.emit({
|
||||
type: "reduced_state",
|
||||
state: { ...idleState, pending_approval: { nonce: "n1", request: { sessionId: "s1", toolCall: {}, options: allApprovalOptions } } },
|
||||
});
|
||||
const buttons = conn.el.querySelectorAll<HTMLButtonElement>(".st-approval-acts button");
|
||||
buttons[0].click();
|
||||
expect(buttons[0].disabled).toBe(true);
|
||||
await Promise.resolve().then(() => Promise.resolve()).then(() => Promise.resolve());
|
||||
expect(buttons[0].disabled).toBe(false);
|
||||
expect(buttons[1].disabled).toBe(false);
|
||||
expect(conn.el.textContent).toContain("network down");
|
||||
conn.dispose();
|
||||
});
|
||||
|
||||
it("does not resurrect buttons or show an error for a decision the approval already moved past", async () => {
|
||||
let reject!: (e: unknown) => void;
|
||||
vi.mocked(resolveStructuredApproval).mockImplementationOnce(
|
||||
() => new Promise((_, r) => { reject = r; }),
|
||||
);
|
||||
const ws = new FakeSocket();
|
||||
const conn = mount({ sockets: [ws] });
|
||||
ws.open();
|
||||
ws.emit({
|
||||
type: "transcript_snapshot",
|
||||
snapshot: { cursor: 0, rows: [row("approval:1", "approval", { toolCall: {}, options: allApprovalOptions })] },
|
||||
});
|
||||
ws.emit({
|
||||
type: "reduced_state",
|
||||
state: { ...idleState, pending_approval: { nonce: "n1", request: { sessionId: "s1", toolCall: {}, options: allApprovalOptions } } },
|
||||
});
|
||||
const buttons = conn.el.querySelectorAll<HTMLButtonElement>(".st-approval-acts button");
|
||||
buttons[0].click();
|
||||
expect(resolveStructuredApproval).toHaveBeenCalledWith("s1", "n1", "allow_once");
|
||||
|
||||
// Superseded by a new approval before the in-flight POST settles.
|
||||
ws.emit({
|
||||
type: "projection_update",
|
||||
transcript: { type: "append", row: row("approval:2", "approval", { toolCall: {}, options: allApprovalOptions }) },
|
||||
state: { ...idleState, pending_approval: { nonce: "n2", request: { sessionId: "s1", toolCall: {}, options: allApprovalOptions } } },
|
||||
});
|
||||
reject(new Error("too late"));
|
||||
await Promise.resolve().then(() => Promise.resolve()).then(() => Promise.resolve());
|
||||
|
||||
expect(conn.el.textContent).not.toContain("too late");
|
||||
const newButtons = conn.el.querySelectorAll<HTMLButtonElement>(".st-approval-acts button");
|
||||
expect(newButtons).toHaveLength(3);
|
||||
expect(newButtons[0].disabled).toBe(false);
|
||||
conn.dispose();
|
||||
});
|
||||
});
|
||||
|
||||
describe("structured connection: reconnection/error/loading", () => {
|
||||
|
||||
+16
-3
@@ -115,13 +115,26 @@ export function createStructuredConnection(
|
||||
};
|
||||
}
|
||||
|
||||
function decide(rowId: string, decision: StructuredApprovalDecision): void {
|
||||
async function decide(rowId: string, decision: StructuredApprovalDecision): Promise<void> {
|
||||
const nonce = state.pending_approval?.nonce;
|
||||
// Already resolved elsewhere between the click and here — the row's
|
||||
// buttons should already be gone, but a stale nonce is a no-op, not an
|
||||
// error (docs/STRUCTURED-VIEW.md: any nonce answers 204).
|
||||
// error (docs/STRUCTURED-VIEW.md: any nonce answers 204). Resolve
|
||||
// quietly: there is nothing for the row to restore.
|
||||
if (!nonce || rowId !== pendingApprovalRowId) return;
|
||||
resolveStructuredApproval(sessionId, nonce, decision).catch((e: unknown) => showError(String(e)));
|
||||
try {
|
||||
await resolveStructuredApproval(sessionId, nonce, decision);
|
||||
} catch (e: unknown) {
|
||||
// Only propagate the failure — so the row re-enables its buttons — if
|
||||
// this is still the pending approval. If it moved on while the
|
||||
// request was in flight (superseded, cancelled, answered elsewhere),
|
||||
// the state-driven re-render already replaced the row; reviving
|
||||
// buttons on it would be reviving a decision that no longer applies.
|
||||
if (rowId === pendingApprovalRowId) {
|
||||
showError(String(e));
|
||||
throw e;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
function mountRow(row: TranscriptRow): void {
|
||||
|
||||
@@ -160,29 +160,41 @@ describe("buildTranscriptRow: tool diffs", () => {
|
||||
});
|
||||
|
||||
describe("buildTranscriptRow: approval", () => {
|
||||
const approvalRow = () =>
|
||||
const approvalRow = (options: Array<{ optionId: string; name: string; kind: string }>) =>
|
||||
row("approval", {
|
||||
sessionId: "s1",
|
||||
toolCall: { toolCallId: "t1", title: "Write outside worktree", locations: [{ path: "/etc/x" }] },
|
||||
options: [],
|
||||
options,
|
||||
});
|
||||
|
||||
it("shows the three decision buttons only when it is the pending approval", () => {
|
||||
const el = buildTranscriptRow(approvalRow(), { pending: true, onDecide: () => {} });
|
||||
const allThreeOptions = [
|
||||
{ optionId: "a1", name: "Allow once", kind: "allow_once" },
|
||||
{ optionId: "a2", name: "Allow always", kind: "allow_always" },
|
||||
{ optionId: "r1", name: "Reject once", kind: "reject_once" },
|
||||
];
|
||||
|
||||
it("shows the three decision buttons only when the request offers all three option kinds", () => {
|
||||
const el = buildTranscriptRow(approvalRow(allThreeOptions), {
|
||||
pending: true,
|
||||
onDecide: () => Promise.resolve(),
|
||||
});
|
||||
const buttons = el.querySelectorAll(".st-approval-acts button");
|
||||
expect(buttons).toHaveLength(3);
|
||||
expect(el.querySelector(".st-approval-path")!.textContent).toBe("/etc/x");
|
||||
});
|
||||
|
||||
it("hides the buttons and shows an answered note once resolved", () => {
|
||||
const el = buildTranscriptRow(approvalRow(), { pending: false, onDecide: () => {} });
|
||||
const el = buildTranscriptRow(approvalRow(allThreeOptions), {
|
||||
pending: false,
|
||||
onDecide: () => Promise.resolve(),
|
||||
});
|
||||
expect(el.querySelector(".st-approval-acts")).toBeNull();
|
||||
expect(el.querySelector(".st-approval-resolved")!.textContent).toBe("answered");
|
||||
});
|
||||
|
||||
it("invokes onDecide with the clicked decision and disables the buttons", () => {
|
||||
const onDecide = vi.fn();
|
||||
const el = buildTranscriptRow(approvalRow(), { pending: true, onDecide });
|
||||
const onDecide = vi.fn().mockResolvedValue(undefined);
|
||||
const el = buildTranscriptRow(approvalRow(allThreeOptions), { pending: true, onDecide });
|
||||
const buttons = el.querySelectorAll<HTMLButtonElement>(".st-approval-acts button");
|
||||
buttons[2].click();
|
||||
expect(onDecide).toHaveBeenCalledWith("deny");
|
||||
@@ -190,9 +202,65 @@ describe("buildTranscriptRow: approval", () => {
|
||||
});
|
||||
|
||||
it("treats a row with no approval state as not pending, defensively", () => {
|
||||
const el = buildTranscriptRow(approvalRow());
|
||||
const el = buildTranscriptRow(approvalRow(allThreeOptions));
|
||||
expect(el.querySelector(".st-approval-acts")).toBeNull();
|
||||
});
|
||||
|
||||
it("omits Allow for session when the request offers no allow_always option", () => {
|
||||
const el = buildTranscriptRow(
|
||||
approvalRow([
|
||||
{ optionId: "a1", name: "Allow once", kind: "allow_once" },
|
||||
{ optionId: "r1", name: "Reject once", kind: "reject_once" },
|
||||
]),
|
||||
{ pending: true, onDecide: () => Promise.resolve() },
|
||||
);
|
||||
const labels = [...el.querySelectorAll(".st-approval-acts button")].map((b) => b.textContent);
|
||||
expect(labels).toEqual(["Allow once", "Deny"]);
|
||||
});
|
||||
|
||||
it("offers Deny for either reject_once or reject_always, never for neither", () => {
|
||||
const rejectAlwaysOnly = buildTranscriptRow(
|
||||
approvalRow([{ optionId: "r2", name: "Reject always", kind: "reject_always" }]),
|
||||
{ pending: true, onDecide: () => Promise.resolve() },
|
||||
);
|
||||
expect([...rejectAlwaysOnly.querySelectorAll(".st-approval-acts button")].map((b) => b.textContent)).toEqual([
|
||||
"Deny",
|
||||
]);
|
||||
});
|
||||
|
||||
it("shows a 'no decision offered' note instead of an empty action row when nothing matches", () => {
|
||||
const el = buildTranscriptRow(approvalRow([{ optionId: "x", name: "Something else", kind: "allow_once" }]), {
|
||||
pending: true,
|
||||
onDecide: () => Promise.resolve(),
|
||||
});
|
||||
// allow_once is offered, so this specific case still shows a button —
|
||||
// use a kind the client doesn't map to any decision at all.
|
||||
expect(el.querySelectorAll(".st-approval-acts button")).toHaveLength(1);
|
||||
|
||||
const none = buildTranscriptRow(approvalRow([]), { pending: true, onDecide: () => Promise.resolve() });
|
||||
expect(none.querySelector(".st-approval-acts")).toBeNull();
|
||||
expect(none.querySelector(".st-approval-resolved")!.textContent).toBe("no decision offered");
|
||||
});
|
||||
|
||||
it("re-enables the buttons when onDecide rejects", async () => {
|
||||
const onDecide = vi.fn().mockRejectedValue(new Error("network down"));
|
||||
const el = buildTranscriptRow(approvalRow(allThreeOptions), { pending: true, onDecide });
|
||||
const buttons = el.querySelectorAll<HTMLButtonElement>(".st-approval-acts button");
|
||||
buttons[0].click();
|
||||
expect(buttons[0].disabled).toBe(true);
|
||||
await Promise.resolve().then(() => Promise.resolve());
|
||||
expect(buttons[0].disabled).toBe(false);
|
||||
expect(buttons[1].disabled).toBe(false);
|
||||
});
|
||||
|
||||
it("leaves the buttons disabled when onDecide resolves", async () => {
|
||||
const onDecide = vi.fn().mockResolvedValue(undefined);
|
||||
const el = buildTranscriptRow(approvalRow(allThreeOptions), { pending: true, onDecide });
|
||||
const buttons = el.querySelectorAll<HTMLButtonElement>(".st-approval-acts button");
|
||||
buttons[0].click();
|
||||
await Promise.resolve().then(() => Promise.resolve());
|
||||
expect(buttons[0].disabled).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe("buildTranscriptRow: notice / unknown kinds", () => {
|
||||
|
||||
+46
-11
@@ -7,6 +7,7 @@ import { escapeHtml } from "./html.js";
|
||||
import { computeDiffLines, diffStats } from "./structuredDiff.js";
|
||||
import type {
|
||||
AcpContentBlock,
|
||||
AcpPermissionOption,
|
||||
AcpPlan,
|
||||
AcpToolCallContent,
|
||||
AcpToolCallLike,
|
||||
@@ -271,11 +272,29 @@ function buildPlainRow(row: TranscriptRow): HTMLElement {
|
||||
|
||||
export interface ApprovalRowState {
|
||||
pending: boolean;
|
||||
onDecide: (decision: StructuredApprovalDecision) => void;
|
||||
/** Resolves once the decision has been submitted; rejects when the POST
|
||||
* failed and this approval is still the one awaiting a decision, so the
|
||||
* caller knows to restore its controls. Never rejects for a stale click
|
||||
* (the approval already moved on) — there is nothing to restore then. */
|
||||
onDecide: (decision: StructuredApprovalDecision) => Promise<void>;
|
||||
}
|
||||
|
||||
/** Which decisions a permission request actually backs, by the
|
||||
* `PermissionOptionKind` values it offers (docs/STRUCTURED-VIEW.md's
|
||||
* "Decision mapping"). A decision with no matching option resolves
|
||||
* `Cancelled` server-side regardless of what the client sent — offering its
|
||||
* button would let the operator "allow" something that silently no-ops. */
|
||||
function offeredDecisions(options: AcpPermissionOption[]): StructuredApprovalDecision[] {
|
||||
const kinds = new Set(options.map((o) => o.kind));
|
||||
const decisions: StructuredApprovalDecision[] = [];
|
||||
if (kinds.has("allow_once")) decisions.push("allow_once");
|
||||
if (kinds.has("allow_always")) decisions.push("allow_for_session");
|
||||
if (kinds.has("reject_once") || kinds.has("reject_always")) decisions.push("deny");
|
||||
return decisions;
|
||||
}
|
||||
|
||||
function buildApprovalRow(row: TranscriptRow, approval: ApprovalRowState): HTMLElement {
|
||||
const request = (row.data ?? {}) as { toolCall?: AcpToolCallLike };
|
||||
const request = (row.data ?? {}) as { toolCall?: AcpToolCallLike; options?: AcpPermissionOption[] };
|
||||
const toolCall = request.toolCall ?? {};
|
||||
const wrap = el("div", "st-row st-row-approval");
|
||||
const card = el("div", "st-approval");
|
||||
@@ -304,17 +323,33 @@ function buildApprovalRow(row: TranscriptRow, approval: ApprovalRowState): HTMLE
|
||||
btn.className = cls ? `act ${cls}` : "act";
|
||||
btn.textContent = label;
|
||||
btn.addEventListener("click", () => {
|
||||
acts.querySelectorAll("button").forEach((b) => (b.disabled = true));
|
||||
approval.onDecide(decision);
|
||||
const buttons = [...acts.querySelectorAll<HTMLButtonElement>("button")];
|
||||
buttons.forEach((b) => (b.disabled = true));
|
||||
approval.onDecide(decision).catch(() => {
|
||||
// Rejects only when this approval is still pending (see
|
||||
// ApprovalRowState) — restore the controls so the operator can
|
||||
// retry; the error itself is surfaced by the connection.
|
||||
buttons.forEach((b) => (b.disabled = false));
|
||||
});
|
||||
});
|
||||
return btn;
|
||||
};
|
||||
acts.append(
|
||||
mk("Allow once", "allow_once", "primary"),
|
||||
mk("Allow for session", "allow_for_session", ""),
|
||||
mk("Deny", "deny", "deny"),
|
||||
);
|
||||
card.appendChild(acts);
|
||||
const offered = offeredDecisions(request.options ?? []);
|
||||
const specs: [StructuredApprovalDecision, string, string][] = [
|
||||
["allow_once", "Allow once", "primary"],
|
||||
["allow_for_session", "Allow for session", ""],
|
||||
["deny", "Deny", "deny"],
|
||||
];
|
||||
for (const [decision, label, cls] of specs) {
|
||||
if (offered.includes(decision)) acts.appendChild(mk(label, decision, cls));
|
||||
}
|
||||
if (acts.children.length > 0) {
|
||||
card.appendChild(acts);
|
||||
} else {
|
||||
const none = el("div", "st-approval-resolved");
|
||||
none.textContent = "no decision offered";
|
||||
card.appendChild(none);
|
||||
}
|
||||
} else {
|
||||
const resolved = el("div", "st-approval-resolved");
|
||||
resolved.textContent = "answered";
|
||||
@@ -343,7 +378,7 @@ export function buildTranscriptRow(row: TranscriptRow, approval?: ApprovalRowSta
|
||||
built = buildToolRow(row);
|
||||
break;
|
||||
case "approval":
|
||||
built = buildApprovalRow(row, approval ?? { pending: false, onDecide: () => {} });
|
||||
built = buildApprovalRow(row, approval ?? { pending: false, onDecide: () => Promise.resolve() });
|
||||
break;
|
||||
default:
|
||||
built = buildPlainRow(row);
|
||||
|
||||
Reference in New Issue
Block a user