From a01f4df70d5c3fa5db81b656fb99f31a20024150 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Tue, 8 Sep 2026 22:07:28 -0700 Subject: [PATCH 1/5] Bind ask selector rows by request id --- CHANGELOG.md | 6 + docs/TUI.md | 3 + src/permission/gate.test.ts | 36 ++++++ src/permission/gate.ts | 11 +- src/permission/types.ts | 4 + src/tui/decision-truncation.test.ts | 1 + src/tui/gate-events.ts | 2 + src/tui/gate-wire.test.ts | 192 +++++++++++++++++++++++----- src/tui/gate-wire.ts | 55 ++++---- src/tui/mention-popup.test.ts | 2 + src/tui/overlay-overflow.test.ts | 5 +- src/tui/overlay-view.test.ts | 46 +++++++ src/tui/overlay-view.ts | 11 +- src/tui/overlays.test.ts | 36 ++++++ src/tui/product-host.test.ts | 2 + src/tui/request-approval.test.ts | 33 +++++ src/tui/request-approval.ts | 5 + src/tui/runner/session.ts | 3 + src/tui/shell/chrome.ts | 2 + src/tui/shell/overlay-host.ts | 4 +- src/tui/slash-popup-gate.test.ts | 1 + 21 files changed, 400 insertions(+), 60 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b27dd4cdd..fe2250814 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,12 @@ parallel copies under `docs/` or `scripts/notes/`. At cut time: rename not a second copy of `env.authorize`: it consumes the prior verdict when the same call (id, name, and arguments) is cached, and decides on a cache miss. +### Fixed + +- Sequential TUI ask and permission selectors paint the live question's option + labels. Rows bind by ask and request id rather than render-order index, so a + later question cannot keep the previous question's choices. + ## [0.3.18] - 2026-09-08 ### Added diff --git a/docs/TUI.md b/docs/TUI.md index d44e42db9..7752de88e 100644 --- a/docs/TUI.md +++ b/docs/TUI.md @@ -342,6 +342,9 @@ viewport kit: shared windowing, keep-active-visible, and page/jump behavior. There is exactly one scroll lease at a time; keyboard paging and the mouse wheel both follow whichever surface currently holds it, so a modal open on top of the transcript never lets the wheel move the transcript underneath it. +Ask and permission rows bind by ask/request id, never render-order index; +painted labels are the live payload, and stale rows whose ids are not in the +new payload are dropped. "Current" is never inferred. For the model picker, the row marked `(current)` is read live from the session's actual active provider/model on diff --git a/src/permission/gate.test.ts b/src/permission/gate.test.ts index 5e5c3acfa..99d8de562 100644 --- a/src/permission/gate.test.ts +++ b/src/permission/gate.test.ts @@ -236,3 +236,39 @@ describe("standing grant covers a later git worktree command (CL-5638)", () => { expect(prompts).toBe(1); }); }); + +describe("ask decision mints request.id", () => { + test("the copy handed to requestApproval has a non-empty id", async () => { + let captured: PermissionRequest | undefined; + const gate = createPermissionGate({ + approvals: [], + interactive: true, + skipPermissions: false, + reactorGated: false, + requestApproval: async (request) => { + captured = request; + return { allow: true }; + }, + }); + const verdict = await gate.evaluate(shellCall("npm test || true")); + expect(verdict.allowed).toBe(true); + expect(captured?.id).toEqual(expect.any(String)); + expect(captured?.id?.length).toBeGreaterThan(0); + }); + + test("authorizeCall ask request carries a minted id", async () => { + const gate = createPermissionGate({ + approvals: [], + interactive: true, + skipPermissions: false, + reactorGated: false, + requestApproval: async () => ({ allow: true }), + }); + const result = await gate.authorizeCall(shellCall("npm test || true")); + expect(result.effect).toBe("ask"); + if (result.effect !== "ask") throw new Error("expected ask"); + const id = result.request.id; + expect(id).toEqual(expect.any(String)); + expect(id?.length).toBeGreaterThan(0); + }); +}); diff --git a/src/permission/gate.ts b/src/permission/gate.ts index e104f5dc8..f6b1a8d64 100644 --- a/src/permission/gate.ts +++ b/src/permission/gate.ts @@ -1,5 +1,6 @@ import type { ToolCall } from "@intx/types/runtime"; import { isAbsolute, resolve } from "node:path"; +import { randomUUID } from "node:crypto"; import type { Approval, ApprovalOutcome, @@ -47,6 +48,12 @@ import { NOOP_APPROVAL_LOG, type ApprovalLog, type ApprovalOutcomeKind } from ". // autoDeny in gate-wire.ts and the timeout branch in tui/request-approval.ts's // finish() usage); anything else that denies is a plain operator/unavailable // decision. +function withRequestId(request: PermissionRequest): PermissionRequest { + return request.id !== undefined && request.id.length > 0 + ? request + : { ...request, id: randomUUID() }; +} + function classifyOutcome(outcome: ApprovalOutcome | undefined): ApprovalOutcomeKind { if (outcome === undefined) return "deny"; if (!outcome.allow) { @@ -664,7 +671,7 @@ export function createPermissionGate(options: PermissionGateOptions): Permission const requestForOperator = anySecret ? { ...request, scopes: [] } : request; return { kind: "ask", - request: requestForOperator, + request: withRequestId(requestForOperator), anySecret, segmentCount: segments.length, }; @@ -692,7 +699,7 @@ export function createPermissionGate(options: PermissionGateOptions): Permission }; } - return { kind: "ask", request, anySecret: false, segmentCount: 0 }; + return { kind: "ask", request: withRequestId(request), anySecret: false, segmentCount: 0 }; } return { kind: "allow" }; }; diff --git a/src/permission/types.ts b/src/permission/types.ts index ab5db7ac9..4cc26933c 100644 --- a/src/permission/types.ts +++ b/src/permission/types.ts @@ -56,6 +56,10 @@ export interface PermissionRequest { // withheld for a reason beyond the ordinary "no persistent option exists // yet" case. Plain literal text, never model-authored. notice?: string; + // Set by the gate on the copy handed to requestApproval, never on + // buildRequests matching/display copies. Overlay rows bind by this id + // rather than render-order index (see gate-wire.ts). + id?: string; // Set by the gate right before handing this request to requestApproval, so // whichever surface actually renders it (see gate-wire.ts's overlay host) // can report the moment it reached the operator's screen — distinct from diff --git a/src/tui/decision-truncation.test.ts b/src/tui/decision-truncation.test.ts index c560c59ba..e5b05ca9c 100644 --- a/src/tui/decision-truncation.test.ts +++ b/src/tui/decision-truncation.test.ts @@ -33,6 +33,7 @@ const hintRequest: PermissionRequest = { hint: HINT, }, ], + id: "req-1", }; function bodySelect(view: ReturnType): SelectRenderable { diff --git a/src/tui/gate-events.ts b/src/tui/gate-events.ts index 11cd1a726..30f15a0ad 100644 --- a/src/tui/gate-events.ts +++ b/src/tui/gate-events.ts @@ -2,6 +2,8 @@ import type { ApprovalOutcome, PermissionRequest } from "../permission/types.js" import type { OperatorResult } from "../agent/tools.js"; export interface OperatorGateEvent { + /** Minted by the session emitter, never by the TUI overlay. */ + id: string; question: string; options: string[]; resolve: (result: OperatorResult) => void; diff --git a/src/tui/gate-wire.test.ts b/src/tui/gate-wire.test.ts index 3adf722fe..e8e7db7b1 100644 --- a/src/tui/gate-wire.test.ts +++ b/src/tui/gate-wire.test.ts @@ -36,14 +36,15 @@ const baseRequest = (overrides: Partial = {}): PermissionRequ action: "Run shell command", subject: "bun test", scopes: [], + id: "req-1", ...overrides, }); describe("permissionChoicesFromRequest", () => { test("always includes reject + accept once", () => { - const choices = permissionChoicesFromRequest(baseRequest()); + const choices = permissionChoicesFromRequest(baseRequest({ id: "req-1" })); expect(choices.items).toEqual(["Reject", "Accept once"]); - expect(choices.itemIds).toEqual([PERMISSION_DENY_ID, PERMISSION_ONCE_ID]); + expect(choices.itemIds).toEqual([`req-1:${PERMISSION_DENY_ID}`, `req-1:${PERMISSION_ONCE_ID}`]); expect(choices.outcomes).toEqual([{ allow: false }, { allow: true }]); }); @@ -62,15 +63,16 @@ describe("permissionChoicesFromRequest", () => { }; const choices = permissionChoicesFromRequest( baseRequest({ + id: "req-1", scopes: [scopeWithPattern, onceScope], }), ); expect(choices.items).toEqual(["Reject", "Accept once", "Allow git *", "Allow this path"]); expect(choices.itemIds).toEqual([ - PERMISSION_DENY_ID, - PERMISSION_ONCE_ID, - "session-git", - "once-extra", + `req-1:${PERMISSION_DENY_ID}`, + `req-1:${PERMISSION_ONCE_ID}`, + "req-1:session-git", + "req-1:once-extra", ]); expect(choices.outcomes[2]).toEqual({ allow: true, @@ -84,6 +86,7 @@ describe("approvalOutcomeFromSelection", () => { test("index maps to parallel outcomes; OOB denies", () => { const choices = permissionChoicesFromRequest( baseRequest({ + id: "req-1", scopes: [ { id: "proj", @@ -110,6 +113,7 @@ describe("approvalOutcomeFromSelection", () => { test("id preferred over index when present", () => { const choices = permissionChoicesFromRequest( baseRequest({ + id: "req-1", scopes: [ { id: "a", label: "A", pattern: "a*" }, { id: "b", label: "B", pattern: "b*" }, @@ -118,20 +122,20 @@ describe("approvalOutcomeFromSelection", () => { ); const byId = approvalOutcomeFromSelection(choices, { index: 0, - id: "b", + id: "req-1:b", }); expect(byId.allow).toBe(true); expect(byId.persist?.id).toBe("b"); }); - test("unknown id falls back to index", () => { - const choices = permissionChoicesFromRequest(baseRequest()); + test("unknown id denies without falling back to index", () => { + const choices = permissionChoicesFromRequest(baseRequest({ id: "req-1" })); expect( approvalOutcomeFromSelection(choices, { index: 1, id: "missing", }), - ).toEqual({ allow: true }); + ).toEqual({ allow: false }); }); }); @@ -212,41 +216,42 @@ describe("permissionBodyFromRequest", () => { }); describe("operatorChoicesFromOptions / operatorResultFromSelection", () => { - test("choices mirror options with index string ids", () => { + test("choices mirror options with ask-scoped ids", () => { const opts = ["Cancel", "Option A", "Option B"]; - const choices = operatorChoicesFromOptions(opts); + const choices = operatorChoicesFromOptions(opts, "ask-1"); expect(choices.items).toEqual(opts); - expect(choices.itemIds).toEqual(["0", "1", "2"]); + expect(choices.itemIds).toEqual(["ask-1:0", "ask-1:1", "ask-1:2"]); }); test("selection index → option; OOB → cancel", () => { - const opts = ["A", "B"]; - expect(operatorResultFromSelection(opts, { index: 0 })).toEqual({ + const choices = operatorChoicesFromOptions(["A", "B"], "ask-1"); + expect(operatorResultFromSelection(choices, { index: 0 })).toEqual({ kind: "option", index: 0, }); - expect(operatorResultFromSelection(opts, { index: 1 })).toEqual({ + expect(operatorResultFromSelection(choices, { index: 1 })).toEqual({ kind: "option", index: 1, }); - expect(operatorResultFromSelection(opts, { index: -1 })).toEqual({ + expect(operatorResultFromSelection(choices, { index: -1 })).toEqual({ kind: "cancel", }); - expect(operatorResultFromSelection(opts, { index: 9 })).toEqual({ + expect(operatorResultFromSelection(choices, { index: 9 })).toEqual({ kind: "cancel", }); }); - test("id string index preferred when valid", () => { - const opts = ["A", "B", "C"]; - expect(operatorResultFromSelection(opts, { index: 0, id: "2" })).toEqual({ + test("id preferred when present in itemIds", () => { + const choices = operatorChoicesFromOptions(["A", "B", "C"], "ask-1"); + expect(operatorResultFromSelection(choices, { index: 0, id: "ask-1:2" })).toEqual({ kind: "option", index: 2, }); - // non-decimal / out of range id ignored → use index - expect(operatorResultFromSelection(opts, { index: 1, id: "nope" })).toEqual({ - kind: "option", - index: 1, + expect(operatorResultFromSelection(choices, { index: 1, id: "nope" })).toEqual({ + kind: "cancel", + }); + expect(operatorResultFromSelection(choices, { index: 1, id: "2" })).toEqual({ + kind: "cancel", }); }); @@ -294,6 +299,7 @@ describe("wireGates", () => { action: "Run shell command", subject: "bun test", scopes: [], + id: "req-1", }; try { const dispose = wireGates(emitter, shell); @@ -329,6 +335,7 @@ describe("wireGates", () => { action: "Run shell command", subject: "echo start && cat > notes.txt < { try { const dispose = wireGates(emitter, shell); emitter.emit("operator.gate", { + id: "ask-1", question: "Proceed?", options: ["Cancel", "Continue"], resolve: (result: unknown) => { @@ -396,6 +404,116 @@ describe("wireGates", () => { }); }); + test("sequential operator asks paint B's labels and id-scoped values, not A's", async () => { + await withTestRenderer(async (h) => { + const shell = createAppShell(h.renderer, { + terminal: { columns: 80, rows: 24 }, + run: "idle", + }); + const emitter = new EventEmitter(); + let resolvedA: unknown; + let resolvedB: unknown; + try { + const dispose = wireGates(emitter, shell); + emitter.emit("operator.gate", { + id: "ask-a", + question: "Ask A?", + options: ["Stay on A", "Leave A"], + resolve: (result: unknown) => { + resolvedA = result; + }, + }); + expect(shell.overlayKind).toBe("operator"); + expect(shell.overlayList?.select.options.map((option) => option.name)).toEqual([ + "Stay on A", + "Leave A", + ]); + + acceptOverlaySelection(shell); + expect(resolvedA).toEqual({ kind: "option", index: 0 }); + expect(shell.overlayList).toBeNull(); + + emitter.emit("operator.gate", { + id: "ask-b", + question: "Ask B?", + options: ["Go with B", "Skip B"], + resolve: (result: unknown) => { + resolvedB = result; + }, + }); + expect(shell.overlayKind).toBe("operator"); + const painted = shell.overlayList?.select.options ?? []; + expect(painted.map((option) => option.name)).toEqual(["Go with B", "Skip B"]); + expect(painted.map((option) => option.value)).toEqual(["ask-b:0", "ask-b:1"]); + expect(painted.map((option) => option.value)).not.toContain("ask-a:0"); + expect(painted.map((option) => option.value)).not.toContain("0"); + + acceptOverlaySelection(shell); + expect(resolvedB).toEqual({ kind: "option", index: 0 }); + + dispose(); + } finally { + shell.dispose(); + } + }); + }); + + test("operator.gate without id cancels without opening", async () => { + await withTestRenderer(async (h) => { + const shell = createAppShell(h.renderer, { + terminal: { columns: 80, rows: 24 }, + run: "idle", + }); + const emitter = new EventEmitter(); + let resolved: unknown; + try { + const dispose = wireGates(emitter, shell); + emitter.emit("operator.gate", { + question: "Proceed?", + options: ["Cancel", "Continue"], + resolve: (result: unknown) => { + resolved = result; + }, + }); + expect(resolved).toEqual({ kind: "cancel" }); + expect(shell.overlayKind).not.toBe("operator"); + dispose(); + } finally { + shell.dispose(); + } + }); + }); + + test("permission.gate without request.id denies without opening", async () => { + await withTestRenderer(async (h) => { + const shell = createAppShell(h.renderer, { + terminal: { columns: 80, rows: 24 }, + run: "idle", + }); + const emitter = new EventEmitter(); + let resolved: unknown; + try { + const dispose = wireGates(emitter, shell); + emitter.emit("permission.gate", { + request: { + tool: "run_shell", + action: "Run shell command", + subject: "bun test", + scopes: [], + }, + resolve: (outcome: unknown) => { + resolved = outcome; + }, + }); + expect(resolved).toEqual({ allow: false }); + expect(shell.overlayKind).not.toBe("permissions"); + dispose(); + } finally { + shell.dispose(); + } + }); + }); + test("gate decisions do not replay the request into the transcript", async () => { await withTestRenderer(async (h) => { const shell = createAppShell(h.renderer, { @@ -408,6 +526,7 @@ describe("wireGates", () => { action: "Run shell command", subject: "ls -la ~/.corbits/projects", scopes: [], + id: "req-1", }; try { const dispose = wireGates(emitter, shell); @@ -478,6 +597,7 @@ describe("gate decisions stay out of the transcript", () => { try { wireGates(emitter, shell); emitter.emit("operator.gate", { + id: "ask-1", question: "Proceed?", options: ["Cancel", "Continue"], resolve: () => {}, @@ -502,6 +622,7 @@ describe("gate decisions stay out of the transcript", () => { try { wireGates(emitter, shell); emitter.emit("operator.gate", { + id: "ask-1", question: "Proceed?", options: ["Cancel", "Continue"], resolve: () => {}, @@ -526,6 +647,7 @@ describe("gate decisions stay out of the transcript", () => { try { wireGates(emitter, shell); emitter.emit("operator.gate", { + id: "ask-1", question: "Proceed?", options: ["Cancel", "Continue"], resolve: () => {}, @@ -658,7 +780,7 @@ describe("gate decisions stay out of the transcript", () => { }); const before = shell.streamLog.length; emitter.emit("permission.gate", { - request: baseRequest({ tool: "queued_tool" }), + request: baseRequest({ tool: "queued_tool", id: "req-2" }), resolve: () => { resolveCount += 1; }, @@ -705,7 +827,7 @@ describe("gate decisions stay out of the transcript", () => { }); const before = shell.streamLog.length; emitter.emit("permission.gate", { - request: baseRequest({ tool: "queued_tool" }), + request: baseRequest({ tool: "queued_tool", id: "req-2" }), resolve: (outcome: unknown) => { resolveCount += 1; resolved = outcome; @@ -787,7 +909,7 @@ describe("gate decisions stay out of the transcript", () => { // opening — dispose must deny it without ever displaying it. const before = shell.streamLog.length; emitter.emit("permission.gate", { - request: baseRequest({ tool: "queued_tool" }), + request: baseRequest({ tool: "queued_tool", id: "req-2" }), resolve: (outcome: unknown) => { queuedResolveCount += 1; queuedResolved = outcome; @@ -922,7 +1044,7 @@ describe("permission.gate auto-deny", () => { }, }); emitter.emit("permission.gate", { - request: baseRequest({ tool: "queued_tool" }), + request: baseRequest({ tool: "queued_tool", id: "req-2" }), resolve: (outcome: unknown) => { secondResolved = outcome; }, @@ -970,6 +1092,7 @@ describe("operator.gate auto-cancel", () => { try { wireGates(emitter, shell); emitter.emit("operator.gate", { + id: "ask-1", question: "Proceed?", options: ["Yes", "No"], resolve: (result: unknown) => { @@ -1002,6 +1125,7 @@ describe("operator.gate auto-cancel", () => { try { wireGates(emitter, shell); emitter.emit("operator.gate", { + id: "ask-1", question: "Proceed?", options: ["Yes", "No"], resolve: (result: unknown) => { @@ -1041,6 +1165,7 @@ describe("operator.gate auto-cancel", () => { resolve: () => {}, }); emitter.emit("operator.gate", { + id: "ask-1", question: "Proceed?", options: ["Yes", "No"], resolve: (result: unknown) => { @@ -1080,6 +1205,7 @@ describe("operator.gate auto-cancel", () => { }, }); emitter.emit("operator.gate", { + id: "ask-1", question: "Proceed?", options: ["Yes", "No"], resolve: (result: unknown) => { @@ -1120,6 +1246,7 @@ describe("operator.gate auto-cancel", () => { try { wireGates(emitter, shell); emitter.emit("operator.gate", { + id: "ask-1", question: "Proceed?", options: ["Yes", "No"], resolve: () => { @@ -1150,6 +1277,7 @@ describe("operator.gate auto-cancel", () => { wireGates(emitter, shell); const before = shell.streamLog.length; emitter.emit("operator.gate", { + id: "ask-1", question: "Proceed?", options: ["Yes", "No"], resolve: () => {}, @@ -1178,6 +1306,7 @@ describe("operator.gate auto-cancel", () => { let queuedResolved: unknown; const dispose = wireGates(emitter, shell); emitter.emit("operator.gate", { + id: "ask-open", question: "Proceed?", options: ["Yes", "No"], resolve: (r: unknown) => { @@ -1188,6 +1317,7 @@ describe("operator.gate auto-cancel", () => { // opening — dispose must cancel it without ever displaying it. const before = shell.streamLog.length; emitter.emit("operator.gate", { + id: "ask-queued", question: "Also proceed?", options: ["Yes", "No"], resolve: (r: unknown) => { @@ -1249,6 +1379,7 @@ describe("Esc on a gate overlay settles the awaited promise", () => { try { wireGates(emitter, shell); emitter.emit("operator.gate", { + id: "ask-1", question: "Proceed?", options: ["Cancel", "Continue"], resolve: (result: unknown) => { @@ -1284,6 +1415,7 @@ describe("permission overlay height", () => { label: `Always allow scope ${i}`, pattern: `p${i}`, })), + id: "req-1", }, resolve: () => {}, }); @@ -1338,6 +1470,7 @@ describe("operator question overlay", () => { const emitter = new EventEmitter(); wireGates(emitter, shell); emitter.emit("operator.gate", { + id: "ask-1", question: "Scope for this run is still . What should it be?", options: [...options], resolve: onResolve, @@ -1473,6 +1606,7 @@ describe("operator question overlay", () => { }, }); emitter.emit("operator.gate", { + id: "ask-queued", question: "Scope for this run?", options: ["repo only"], resolve: (r: unknown) => { diff --git a/src/tui/gate-wire.ts b/src/tui/gate-wire.ts index c67544126..37cd01b62 100644 --- a/src/tui/gate-wire.ts +++ b/src/tui/gate-wire.ts @@ -61,18 +61,19 @@ export function permissionChoicesFromRequest(request: PermissionRequest): Permis const items: string[] = []; const itemIds: string[] = []; const outcomes: ApprovalOutcome[] = []; + const rowId = (part: string): string => `${request.id}:${part}`; items.push("Reject"); - itemIds.push(PERMISSION_DENY_ID); + itemIds.push(rowId(PERMISSION_DENY_ID)); outcomes.push({ allow: false }); items.push("Accept once"); - itemIds.push(PERMISSION_ONCE_ID); + itemIds.push(rowId(PERMISSION_ONCE_ID)); outcomes.push({ allow: true }); for (const scope of request.scopes) { items.push(scope.label); - itemIds.push(scope.id); + itemIds.push(rowId(scope.id)); outcomes.push({ allow: true, ...(scope.pattern !== null ? { persist: scope as ApprovalScope } : {}), @@ -95,6 +96,7 @@ export function approvalOutcomeFromSelection( if (byId >= 0) { return choices.outcomes[byId] ?? { allow: false }; } + return { allow: false }; } return choices.outcomes[selection.index] ?? { allow: false }; } @@ -148,40 +150,38 @@ export interface OperatorGateChoices { } /** - * Operator options → list rows. itemIds are decimal index strings ("0", "1", …) - * so hosts can round-trip without a parallel outcomes array. + * Operator options → list rows. itemIds are `${askId}:${index}` so sequential + * asks cannot collide on render-order index. */ -export function operatorChoicesFromOptions(options: readonly string[]): OperatorGateChoices { +export function operatorChoicesFromOptions( + options: readonly string[], + askId: string, +): OperatorGateChoices { return { items: [...options], - itemIds: options.map((_, i) => String(i)), + itemIds: options.map((_, i) => `${askId}:${i}`), }; } /** * Map selection → OperatorResult. - * Out-of-range or missing option → cancel (safe closed). + * Present unknown id → cancel (safe closed). No index fallback. */ export function operatorResultFromSelection( - options: readonly string[], + choices: OperatorGateChoices, selection: GateSelection, ): OperatorResult { - let index = selection.index; if (selection.id !== undefined) { - const parsed = Number.parseInt(selection.id, 10); - if ( - Number.isInteger(parsed) && - parsed >= 0 && - parsed < options.length && - String(parsed) === selection.id - ) { - index = parsed; + const byId = choices.itemIds.indexOf(selection.id); + if (byId >= 0) { + return { kind: "option", index: byId }; } + return { kind: "cancel" }; } - if (index < 0 || index >= options.length) { + if (selection.index < 0 || selection.index >= choices.items.length) { return { kind: "cancel" }; } - return { kind: "option", index }; + return { kind: "option", index: selection.index }; } export function operatorCancelResult(): OperatorResult { @@ -294,6 +294,10 @@ export function wireGates( function onPermission(ev: PermissionGateEvent): void { hooks.onGateOpened(); const resolve = onceClosed(hooks.onGateClosed, ev.resolve); + if (ev.request.id === undefined || ev.request.id.length === 0) { + resolve({ allow: false }); + return; + } const choices = permissionChoicesFromRequest(ev.request); const collapsedBody = permissionBodyFromRequest(ev.request, { hint: true }); // Nothing was collapsed → no expand affordance, so the overlay leaves the @@ -370,10 +374,11 @@ export function wireGates( // Esc must settle the awaited promise (as a deny), not abandon it — // an unresolved gate hangs the run until the process is killed. onCancel: () => { + const denyId = choices.itemIds[0]; settle( approvalOutcomeFromSelection(choices, { index: 0, - id: PERMISSION_DENY_ID, + ...(denyId !== undefined ? { id: denyId } : {}), }), ); }, @@ -417,7 +422,11 @@ export function wireGates( function onOperator(ev: OperatorGateEvent): void { hooks.onGateOpened(); const resolve = onceClosed(hooks.onGateClosed, ev.resolve); - const choices = operatorChoicesFromOptions(ev.options); + if (typeof ev.id !== "string" || ev.id.length === 0) { + resolve(operatorCancelResult()); + return; + } + const choices = operatorChoicesFromOptions(ev.options, ev.id); // Guarded the same way as the permission gate: correctness must not rest // on callers of closeInsetOverlay remembering to null the cancel hook // before dispatching accept — a future accept-via-close path that forgets @@ -461,7 +470,7 @@ export function wireGates( clearTimers(); operatorTeardowns.delete(teardown); resolve( - operatorResultFromSelection(ev.options, { + operatorResultFromSelection(choices, { index: sel.index, ...(sel.id !== undefined ? { id: sel.id } : {}), }), diff --git a/src/tui/mention-popup.test.ts b/src/tui/mention-popup.test.ts index a02b96d46..c07213ff0 100644 --- a/src/tui/mention-popup.test.ts +++ b/src/tui/mention-popup.test.ts @@ -258,6 +258,7 @@ describe("@ popup narrows as you type", () => { action: "Run shell command", subject: "bun test", scopes: [], + id: "req-1", }, resolve: (outcome: unknown) => { resolved = outcome; @@ -310,6 +311,7 @@ describe("@ popup narrows as you type", () => { action: "Run shell command", subject: "bun test", scopes: [], + id: "req-1", }, resolve: () => {}, }); diff --git a/src/tui/overlay-overflow.test.ts b/src/tui/overlay-overflow.test.ts index 79d8e8985..db480e5de 100644 --- a/src/tui/overlay-overflow.test.ts +++ b/src/tui/overlay-overflow.test.ts @@ -222,6 +222,7 @@ describe("gate-wire approval overflow on short terminal", () => { label: `Always allow scope ${i}`, pattern: `p${i}`, })), + id: "req-1", }; try { primeSession(shell); @@ -269,6 +270,7 @@ describe("gate-wire approval overflow on short terminal", () => { primeSession(shell); const dispose = wireGates(emitter, shell); emitter.emit("operator.gate", { + id: "ask-1", question: tallBody, options: [...options], resolve: (result: unknown) => { @@ -276,7 +278,7 @@ describe("gate-wire approval overflow on short terminal", () => { }, }); - const choices = operatorChoicesFromOptions(options); + const choices = operatorChoicesFromOptions(options, "ask-1"); expect(shell.overlayKind).toBe("operator"); expect(shell.overlayItems).toEqual([...choices.items]); const list = shell.overlayList!; @@ -308,6 +310,7 @@ describe("gate-wire approval overflow on short terminal", () => { action: "Run shell command", subject: 'git commit -m "line one\nline two\nline three\nline four\nline five"', scopes: [{ id: "session", label: "Allow for session", pattern: "git *" }], + id: "req-1", }; try { primeSession(shell); diff --git a/src/tui/overlay-view.test.ts b/src/tui/overlay-view.test.ts index 49193c315..0fdfde3c0 100644 --- a/src/tui/overlay-view.test.ts +++ b/src/tui/overlay-view.test.ts @@ -116,6 +116,52 @@ describe("overlay view", () => { }); }); + test("repainting a list replaces labels and values so sequential asks cannot keep stale rows", async () => { + await withTestRenderer(async (h) => { + const view = createOverlayView(h.renderer); + h.renderer.root.add(view.host); + const list = createOverlayList(h.renderer, { count: 2, items: 2 }); + const base = { + kind: "operator" as const, + paletteCommands: [], + list, + bodyLines: [], + bodyFgs: [], + answer: null, + describe: () => undefined, + }; + view.paintList( + { + ...base, + items: ["Stay on A", "Leave A"], + itemIds: ["ask-a:0", "ask-a:1"], + }, + 80, + ); + expect(bodySelect(view).options.map((option) => option.name)).toEqual([ + "Stay on A", + "Leave A", + ]); + expect(bodySelect(view).options.map((option) => option.value)).toEqual([ + "ask-a:0", + "ask-a:1", + ]); + + view.paintList( + { + ...base, + items: ["Go with B", "Skip B"], + itemIds: ["ask-b:0", "ask-b:1"], + }, + 80, + ); + const painted = bodySelect(view).options; + expect(painted.map((option) => option.name)).toEqual(["Go with B", "Skip B"]); + expect(painted.map((option) => option.value)).toEqual(["ask-b:0", "ask-b:1"]); + expect(painted.map((option) => option.value)).not.toContain("ask-a:0"); + }); + }); + test("title hints follow offered actions and answer ownership", async () => { await withTestRenderer(async (h) => { const view = createOverlayView(h.renderer); diff --git a/src/tui/overlay-view.ts b/src/tui/overlay-view.ts index a74afe06b..ed1d7ba11 100644 --- a/src/tui/overlay-view.ts +++ b/src/tui/overlay-view.ts @@ -24,6 +24,7 @@ export interface OverlayTitlePresentation extends Pick< export interface OverlayListPresentation { readonly kind: PrimaryOverlayKind | null; readonly items: readonly string[]; + readonly itemIds?: readonly string[]; readonly paletteCommands: readonly Pick[]; readonly list: OverlayList | null; readonly bodyLines: readonly string[]; @@ -356,10 +357,12 @@ export function createOverlayView(ctx: RenderContext) { // second row of air — nothing wraps, nothing clips. list.setHeight(list.height, decision ? DECISION_CHOICE_ROWS : 1); list.select.showSelectionIndicator = true; - list.select.options = presentation.items.map((label) => ({ - name: label, - description: "", - })); + list.select.options = presentation.items.map((label, i) => { + const id = presentation.itemIds?.[i]; + return id === undefined + ? { name: label, description: "" } + : { name: label, description: "", value: id }; + }); // An empty list renders nothing — the renderable would still claim a row // for its background, spending layout budget a chooser with no choices did // not reserve. diff --git a/src/tui/overlays.test.ts b/src/tui/overlays.test.ts index 17050d922..8c470ec7d 100644 --- a/src/tui/overlays.test.ts +++ b/src/tui/overlays.test.ts @@ -321,6 +321,42 @@ describe("overlay accept callbacks", () => { ); }); + test("sequential operator opens paint B's labels and ids, not A's", async () => { + await withTestRenderer( + async (h) => { + const shell = createAppShell(h.renderer, { + terminal: { columns: 80, rows: 24 }, + wireKeys: false, + }); + try { + openOperatorOverlay(shell, { + body: "Ask A?", + choices: ["Stay on A", "Leave A"], + itemIds: ["ask-a:0", "ask-a:1"], + }); + expect(shell.overlayList?.select.options.map((option) => option.name)).toEqual([ + "Stay on A", + "Leave A", + ]); + closeInsetOverlay(shell); + + openOperatorOverlay(shell, { + body: "Ask B?", + choices: ["Go with B", "Skip B"], + itemIds: ["ask-b:0", "ask-b:1"], + }); + const painted = shell.overlayList?.select.options ?? []; + expect(painted.map((option) => option.name)).toEqual(["Go with B", "Skip B"]); + expect(painted.map((option) => option.value)).toEqual(["ask-b:0", "ask-b:1"]); + expect(painted.map((option) => option.value)).not.toContain("ask-a:0"); + } finally { + shell.dispose(); + } + }, + { width: 80, height: 24 }, + ); + }); + test("operator accept fires shell-level onOperator when no per-open", async () => { await withTestRenderer( async (h) => { diff --git a/src/tui/product-host.test.ts b/src/tui/product-host.test.ts index dac561bc5..f3d0a0cff 100644 --- a/src/tui/product-host.test.ts +++ b/src/tui/product-host.test.ts @@ -191,6 +191,7 @@ describe("mountProductHost", () => { action: "run", subject: "ls", scopes: [], + id: "req-1", }; emitter.emit("permission.gate", { request, @@ -212,6 +213,7 @@ describe("mountProductHost", () => { const { host, emitter } = await mountHeadless(); try { emitter.emit("operator.gate", { + id: "ask-1", question: "Proceed?", options: ["Cancel", "Continue"], resolve: (_result: unknown) => {}, diff --git a/src/tui/request-approval.test.ts b/src/tui/request-approval.test.ts index 3910911a2..013972e83 100644 --- a/src/tui/request-approval.test.ts +++ b/src/tui/request-approval.test.ts @@ -99,6 +99,39 @@ describe("createGateRequestApproval", () => { captured?.resolve({ allow: true }); expect((await pending).allow).toBe(true); }); + + test("forwards the gate-minted request id without minting a replacement", async () => { + const withId: PermissionRequest = { ...request, id: "req-from-gate" }; + let captured: PermissionGateEvent | undefined; + const requestApproval = createGateRequestApproval({ + emitGate: (event) => { + captured = event; + return true; + }, + approvalTimeout: noTimeout, + }); + const pending = requestApproval(withId); + expect(captured?.request).toBe(withId); + expect(captured?.request.id).toBe("req-from-gate"); + captured?.resolve({ allow: true }); + await pending; + }); + + test("does not mint an id when the request has none", async () => { + let captured: PermissionGateEvent | undefined; + const requestApproval = createGateRequestApproval({ + emitGate: (event) => { + captured = event; + return true; + }, + approvalTimeout: noTimeout, + }); + const pending = requestApproval(request); + expect(captured?.request).toBe(request); + expect(captured?.request.id).toBeUndefined(); + captured?.resolve({ allow: false }); + await pending; + }); }); // attachApprovalBudget is the mechanism createGateRequestApproval builds on diff --git a/src/tui/request-approval.ts b/src/tui/request-approval.ts index 155da2992..a50dbf792 100644 --- a/src/tui/request-approval.ts +++ b/src/tui/request-approval.ts @@ -70,6 +70,11 @@ export function createGateRequestApproval(args: CreateGateRequestApprovalArgs): kind: "permission", }); const timeout = args.approvalTimeout(); + if (request.id === undefined || request.id.length === 0) { + logger.warn("permission gate request missing id for {tool}; emitting without minting", { + tool: request.tool, + }); + } const event: PermissionGateEvent = { request, resolve: finish, diff --git a/src/tui/runner/session.ts b/src/tui/runner/session.ts index 04e2471aa..70c680568 100644 --- a/src/tui/runner/session.ts +++ b/src/tui/runner/session.ts @@ -9,6 +9,7 @@ */ import { join } from "node:path"; +import { randomUUID } from "node:crypto"; import { EventEmitter } from "node:events"; import { localSettingsPath, @@ -273,6 +274,7 @@ export async function assembleTUISession( }); const timeout = approvalTimeout(); const event: OperatorGateEvent = { + id: randomUUID(), question, options, resolve: finish, @@ -295,6 +297,7 @@ export async function assembleTUISession( }); const timeout = approvalTimeout(); const event: OperatorGateEvent = { + id: randomUUID(), question: `Trust local MCP server "${server.name}" for this project?` + (server.command !== undefined diff --git a/src/tui/shell/chrome.ts b/src/tui/shell/chrome.ts index c33d3d9df..e844702ba 100644 --- a/src/tui/shell/chrome.ts +++ b/src/tui/shell/chrome.ts @@ -438,10 +438,12 @@ export function activeOverlayItemId(shell: AppShell, list: OverlayList): string export function paintOverlayList(shell: AppShell): void { const list = shell.overlayList; if (!list) return; + const bag = shellInternals(shell); shell.overlayView.paintList( { kind: shell.overlayKind, items: shell.overlayItems, + ...(bag !== undefined ? { itemIds: bag.primaryBindings.itemIds } : {}), paletteCommands: shell.paletteCommands, list, bodyLines: shell.overlayBodyLines, diff --git a/src/tui/shell/overlay-host.ts b/src/tui/shell/overlay-host.ts index c77323553..017e91260 100644 --- a/src/tui/shell/overlay-host.ts +++ b/src/tui/shell/overlay-host.ts @@ -774,7 +774,9 @@ export function acceptOverlaySelection(shell: AppShell): void { return; } - const id = bag?.primaryBindings.itemIds[idx]; + const painted = shell.overlayList.select.getSelectedOption()?.value; + const itemIds = bag?.primaryBindings.itemIds ?? []; + const id = typeof painted === "string" && itemIds.includes(painted) ? painted : itemIds[idx]; // Type-to-filter plants "(no matches)" with an empty-id sentinel. Stay open. if (id === "") return; const value = bag?.primaryBindings.itemValues[idx]; diff --git a/src/tui/slash-popup-gate.test.ts b/src/tui/slash-popup-gate.test.ts index 39d8502ad..4b923224d 100644 --- a/src/tui/slash-popup-gate.test.ts +++ b/src/tui/slash-popup-gate.test.ts @@ -100,6 +100,7 @@ function emitPermissionGate( action: "Run shell command", subject: "bun test", scopes: [], + id: extra?.tool ?? "req-1", }, resolve, ...(extra?.timeoutMs !== undefined ? { timeoutMs: extra.timeoutMs } : {}), From 7b82988944c050d949734a6c15ebd26a168bc736 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Tue, 8 Sep 2026 22:28:07 -0700 Subject: [PATCH 2/5] Fail closed when ask overlay accept is not id-keyed --- src/tui/gate-wire.test.ts | 68 +++++++++++++++++++++++++++++++++++ src/tui/overlays.test.ts | 40 +++++++++++++++++++++ src/tui/shell/overlay-host.ts | 10 +++++- 3 files changed, 117 insertions(+), 1 deletion(-) diff --git a/src/tui/gate-wire.test.ts b/src/tui/gate-wire.test.ts index e8e7db7b1..5b6e1122b 100644 --- a/src/tui/gate-wire.test.ts +++ b/src/tui/gate-wire.test.ts @@ -458,6 +458,74 @@ describe("wireGates", () => { }); }); + test("sequential permission.gate asks paint B's labels and id-scoped values, not A's", async () => { + await withTestRenderer(async (h) => { + const shell = createAppShell(h.renderer, { + terminal: { columns: 80, rows: 24 }, + run: "idle", + }); + const emitter = new EventEmitter(); + let resolvedA: unknown; + let resolvedB: unknown; + try { + const dispose = wireGates(emitter, shell); + emitter.emit("permission.gate", { + request: baseRequest({ + id: "req-a", + subject: "git status", + scopes: [{ id: "scope-a", label: "Allow git A", pattern: "git A*" }], + }), + resolve: (outcome: unknown) => { + resolvedA = outcome; + }, + }); + expect(shell.overlayKind).toBe("permissions"); + expect(shell.overlayList?.select.options.map((option) => option.name)).toEqual([ + "Reject", + "Accept once", + "Allow git A", + ]); + + closeInsetOverlay(shell); + expect(resolvedA).toEqual({ allow: false }); + expect(shell.overlayList).toBeNull(); + + emitter.emit("permission.gate", { + request: baseRequest({ + id: "req-b", + subject: "git push", + scopes: [{ id: "scope-b", label: "Allow git B", pattern: "git B*" }], + }), + resolve: (outcome: unknown) => { + resolvedB = outcome; + }, + }); + expect(shell.overlayKind).toBe("permissions"); + const painted = shell.overlayList?.select.options ?? []; + expect(painted.map((option) => option.name)).toEqual([ + "Reject", + "Accept once", + "Allow git B", + ]); + expect(painted.map((option) => option.value)).toEqual([ + `req-b:${PERMISSION_DENY_ID}`, + `req-b:${PERMISSION_ONCE_ID}`, + "req-b:scope-b", + ]); + expect(painted.map((option) => option.value)).not.toContain(`req-a:${PERMISSION_DENY_ID}`); + expect(painted.map((option) => option.value)).not.toContain(`req-a:${PERMISSION_ONCE_ID}`); + expect(painted.map((option) => option.value)).not.toContain("req-a:scope-a"); + + acceptOverlaySelection(shell); + expect(resolvedB).toEqual({ allow: false }); + + dispose(); + } finally { + shell.dispose(); + } + }); + }); + test("operator.gate without id cancels without opening", async () => { await withTestRenderer(async (h) => { const shell = createAppShell(h.renderer, { diff --git a/src/tui/overlays.test.ts b/src/tui/overlays.test.ts index 8c470ec7d..a2ca6a445 100644 --- a/src/tui/overlays.test.ts +++ b/src/tui/overlays.test.ts @@ -321,6 +321,46 @@ describe("overlay accept callbacks", () => { ); }); + test("gate accept with a painted value missing from live itemIds denies instead of remapping by index", async () => { + await withTestRenderer( + async (h) => { + const shell = createAppShell(h.renderer, { + terminal: { columns: 80, rows: 24 }, + wireKeys: false, + }); + try { + const accepted: OverlaySelection[] = []; + let cancelled = 0; + openPermissionsOverlay(shell, { + items: ["Reject", "Accept once"], + itemIds: ["req-b:__deny__", "req-b:__once__"], + isGate: true, + echoChoice: false, + onAccept: (s) => accepted.push(s), + onCancel: () => { + cancelled += 1; + }, + }); + moveOverlaySelection(shell, 1); + const list = shell.overlayList; + if (!list) throw new Error("expected an open overlay list"); + list.select.options = [ + { name: "Reject", description: "", value: "req-a:__deny__" }, + { name: "Accept once", description: "", value: "req-a:__once__" }, + ]; + list.select.setSelectedIndex(1); + acceptOverlaySelection(shell); + expect(accepted).toEqual([]); + expect(cancelled).toBe(1); + expect(shell.overlayList).toBeNull(); + } finally { + shell.dispose(); + } + }, + { width: 80, height: 24 }, + ); + }); + test("sequential operator opens paint B's labels and ids, not A's", async () => { await withTestRenderer( async (h) => { diff --git a/src/tui/shell/overlay-host.ts b/src/tui/shell/overlay-host.ts index 017e91260..58691fd87 100644 --- a/src/tui/shell/overlay-host.ts +++ b/src/tui/shell/overlay-host.ts @@ -776,7 +776,15 @@ export function acceptOverlaySelection(shell: AppShell): void { const painted = shell.overlayList.select.getSelectedOption()?.value; const itemIds = bag?.primaryBindings.itemIds ?? []; - const id = typeof painted === "string" && itemIds.includes(painted) ? painted : itemIds[idx]; + const idKeyed = typeof painted === "string" && itemIds.includes(painted); + // Gate accept is id-keyed. A painted Select value missing from the live + // itemIds is a stale or mismatched row — remapping via index would bind + // Enter to the new question's same-index choice. Fail closed instead. + if (bag?.primaryBindings.isGate === true && !idKeyed) { + closeInsetOverlay(shell); + return; + } + const id = idKeyed ? painted : itemIds[idx]; // Type-to-filter plants "(no matches)" with an empty-id sentinel. Stay open. if (id === "") return; const value = bag?.primaryBindings.itemValues[idx]; From 7ba90a9b8276f67e3dcca380a22d6e6322c17d6c Mon Sep 17 00:00:00 2001 From: Sawyer Date: Wed, 9 Sep 2026 09:19:12 -0700 Subject: [PATCH 3/5] Lock sequential permission accept once on the live ask --- src/tui/gate-wire.test.ts | 54 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/src/tui/gate-wire.test.ts b/src/tui/gate-wire.test.ts index 5b6e1122b..f9fc72358 100644 --- a/src/tui/gate-wire.test.ts +++ b/src/tui/gate-wire.test.ts @@ -526,6 +526,60 @@ describe("wireGates", () => { }); }); + test("sequential permission.gate Accept once on B allows B", async () => { + await withTestRenderer(async (h) => { + const shell = createAppShell(h.renderer, { + terminal: { columns: 80, rows: 24 }, + run: "idle", + }); + const emitter = new EventEmitter(); + let resolvedA: unknown; + let resolvedB: unknown; + try { + const dispose = wireGates(emitter, shell); + emitter.emit("permission.gate", { + request: baseRequest({ + id: "req-a", + subject: "git status", + scopes: [{ id: "scope-a", label: "Allow git A", pattern: "git A*" }], + }), + resolve: (outcome: unknown) => { + resolvedA = outcome; + }, + }); + closeInsetOverlay(shell); + expect(resolvedA).toEqual({ allow: false }); + expect(shell.overlayList).toBeNull(); + + emitter.emit("permission.gate", { + request: baseRequest({ + id: "req-b", + subject: "git push", + scopes: [{ id: "scope-b", label: "Allow git B", pattern: "git B*" }], + }), + resolve: (outcome: unknown) => { + resolvedB = outcome; + }, + }); + expect(shell.overlayKind).toBe("permissions"); + expect(shell.overlayList?.select.options.map((option) => option.value)).toEqual([ + `req-b:${PERMISSION_DENY_ID}`, + `req-b:${PERMISSION_ONCE_ID}`, + "req-b:scope-b", + ]); + + moveOverlaySelection(shell, 1); + acceptOverlaySelection(shell); + expect(resolvedB).toEqual({ allow: true }); + expect(resolvedB).not.toEqual({ allow: false }); + + dispose(); + } finally { + shell.dispose(); + } + }); + }); + test("operator.gate without id cancels without opening", async () => { await withTestRenderer(async (h) => { const shell = createAppShell(h.renderer, { From cb2fb2136094dd4c321d2f16b2e86cfe1a4ef8de Mon Sep 17 00:00:00 2001 From: Sawyer Date: Wed, 9 Sep 2026 11:35:37 -0700 Subject: [PATCH 4/5] Fail closed stale ask accept as unavailable not reject Overlay ask ids mint on the gate event at emit, not on PermissionRequest. Escape still denies. An empty or mismatched Enter cannot impersonate Reject. --- CHANGELOG.md | 5 +- docs/TUI.md | 10 +- src/permission/gate.test.ts | 36 ------ src/permission/gate.ts | 39 +++---- src/permission/types.ts | 4 - src/tui/decision-truncation.test.ts | 2 +- src/tui/gate-events.ts | 5 + src/tui/gate-wire.test.ts | 172 ++++++++++++++++++++++------ src/tui/gate-wire.ts | 56 ++++----- src/tui/mention-popup.test.ts | 4 +- src/tui/overlay-overflow.test.ts | 7 +- src/tui/overlays.test.ts | 77 ++++++++++++- src/tui/product-host.test.ts | 2 +- src/tui/request-approval.test.ts | 35 ++---- src/tui/request-approval.ts | 11 +- src/tui/shell/overlay-host.ts | 50 ++++++-- src/tui/slash-popup-gate.test.ts | 2 +- 17 files changed, 329 insertions(+), 188 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fe2250814..58e2e26cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,8 +25,9 @@ parallel copies under `docs/` or `scripts/notes/`. At cut time: rename ### Fixed - Sequential TUI ask and permission selectors paint the live question's option - labels. Rows bind by ask and request id rather than render-order index, so a - later question cannot keep the previous question's choices. + labels. Overlay rows bind by an ask id minted at emit, not render-order + index. A stale or empty accept fail-closes as unavailable rather than + impersonating Reject; Escape still denies. ## [0.3.18] - 2026-09-08 diff --git a/docs/TUI.md b/docs/TUI.md index 7752de88e..1919924a8 100644 --- a/docs/TUI.md +++ b/docs/TUI.md @@ -342,9 +342,13 @@ viewport kit: shared windowing, keep-active-visible, and page/jump behavior. There is exactly one scroll lease at a time; keyboard paging and the mouse wheel both follow whichever surface currently holds it, so a modal open on top of the transcript never lets the wheel move the transcript underneath it. -Ask and permission rows bind by ask/request id, never render-order index; -painted labels are the live payload, and stale rows whose ids are not in the -new payload are dropped. +Ask and permission rows are namespaced by the ask id minted when the overlay +opens. Paint replaces the whole options array (labels and ids together); +there is no drop-by-id merge. Enter binds by the painted id against the live +bag — a painted value that is not in that bag, or Enter on an empty gate +with no answer field, fail-closes the accept as unavailable rather than +remapping by index or treating it as Reject. Escape still denies a +permission and cancels an operator question through the dismiss path. "Current" is never inferred. For the model picker, the row marked `(current)` is read live from the session's actual active provider/model on diff --git a/src/permission/gate.test.ts b/src/permission/gate.test.ts index 99d8de562..5e5c3acfa 100644 --- a/src/permission/gate.test.ts +++ b/src/permission/gate.test.ts @@ -236,39 +236,3 @@ describe("standing grant covers a later git worktree command (CL-5638)", () => { expect(prompts).toBe(1); }); }); - -describe("ask decision mints request.id", () => { - test("the copy handed to requestApproval has a non-empty id", async () => { - let captured: PermissionRequest | undefined; - const gate = createPermissionGate({ - approvals: [], - interactive: true, - skipPermissions: false, - reactorGated: false, - requestApproval: async (request) => { - captured = request; - return { allow: true }; - }, - }); - const verdict = await gate.evaluate(shellCall("npm test || true")); - expect(verdict.allowed).toBe(true); - expect(captured?.id).toEqual(expect.any(String)); - expect(captured?.id?.length).toBeGreaterThan(0); - }); - - test("authorizeCall ask request carries a minted id", async () => { - const gate = createPermissionGate({ - approvals: [], - interactive: true, - skipPermissions: false, - reactorGated: false, - requestApproval: async () => ({ allow: true }), - }); - const result = await gate.authorizeCall(shellCall("npm test || true")); - expect(result.effect).toBe("ask"); - if (result.effect !== "ask") throw new Error("expected ask"); - const id = result.request.id; - expect(id).toEqual(expect.any(String)); - expect(id?.length).toBeGreaterThan(0); - }); -}); diff --git a/src/permission/gate.ts b/src/permission/gate.ts index f6b1a8d64..181aef23b 100644 --- a/src/permission/gate.ts +++ b/src/permission/gate.ts @@ -1,6 +1,5 @@ import type { ToolCall } from "@intx/types/runtime"; import { isAbsolute, resolve } from "node:path"; -import { randomUUID } from "node:crypto"; import type { Approval, ApprovalOutcome, @@ -43,17 +42,25 @@ import { NOOP_APPROVAL_LOG, type ApprovalLog, type ApprovalOutcomeKind } from ". // Closes out an operator prompt: ends the wait span and records the outcome. // buildRequests yields at most one request per tool call, and the two prompt // sites below are mutually exclusive, so this runs once per prompt shown. +function finishApprovalWait( + telemetry: Telemetry, + waitSpanId: string, + tool: string, + outcome: ApprovalOutcome | undefined, +): void { + const decision = outcome !== undefined && outcome.allow ? "allow" : "deny"; + end(waitSpanId, outcome !== undefined ? { decision } : undefined); + telemetry.capture("permission_prompt", { + decision, + permission_kind: classifyPermissionKind(tool), + }); +} + // Classifies a settled ApprovalOutcome into the approval-log taxonomy. // gate-wire.ts's timeout/abort auto-denies carry a fixed message text (see // autoDeny in gate-wire.ts and the timeout branch in tui/request-approval.ts's // finish() usage); anything else that denies is a plain operator/unavailable // decision. -function withRequestId(request: PermissionRequest): PermissionRequest { - return request.id !== undefined && request.id.length > 0 - ? request - : { ...request, id: randomUUID() }; -} - function classifyOutcome(outcome: ApprovalOutcome | undefined): ApprovalOutcomeKind { if (outcome === undefined) return "deny"; if (!outcome.allow) { @@ -65,20 +72,6 @@ function classifyOutcome(outcome: ApprovalOutcome | undefined): ApprovalOutcomeK return outcome.persist !== undefined ? "allow-with-scope" : "allow-once"; } -function finishApprovalWait( - telemetry: Telemetry, - waitSpanId: string, - tool: string, - outcome: ApprovalOutcome | undefined, -): void { - const decision = outcome !== undefined && outcome.allow ? "allow" : "deny"; - end(waitSpanId, outcome !== undefined ? { decision } : undefined); - telemetry.capture("permission_prompt", { - decision, - permission_kind: classifyPermissionKind(tool), - }); -} - export type GateVerdict = { allowed: true } | { allowed: false; reason: string }; // One shell segment's forced-ask guard: a secret-path reference or a @@ -671,7 +664,7 @@ export function createPermissionGate(options: PermissionGateOptions): Permission const requestForOperator = anySecret ? { ...request, scopes: [] } : request; return { kind: "ask", - request: withRequestId(requestForOperator), + request: requestForOperator, anySecret, segmentCount: segments.length, }; @@ -699,7 +692,7 @@ export function createPermissionGate(options: PermissionGateOptions): Permission }; } - return { kind: "ask", request: withRequestId(request), anySecret: false, segmentCount: 0 }; + return { kind: "ask", request, anySecret: false, segmentCount: 0 }; } return { kind: "allow" }; }; diff --git a/src/permission/types.ts b/src/permission/types.ts index 4cc26933c..ab5db7ac9 100644 --- a/src/permission/types.ts +++ b/src/permission/types.ts @@ -56,10 +56,6 @@ export interface PermissionRequest { // withheld for a reason beyond the ordinary "no persistent option exists // yet" case. Plain literal text, never model-authored. notice?: string; - // Set by the gate on the copy handed to requestApproval, never on - // buildRequests matching/display copies. Overlay rows bind by this id - // rather than render-order index (see gate-wire.ts). - id?: string; // Set by the gate right before handing this request to requestApproval, so // whichever surface actually renders it (see gate-wire.ts's overlay host) // can report the moment it reached the operator's screen — distinct from diff --git a/src/tui/decision-truncation.test.ts b/src/tui/decision-truncation.test.ts index e5b05ca9c..37c6e700c 100644 --- a/src/tui/decision-truncation.test.ts +++ b/src/tui/decision-truncation.test.ts @@ -33,7 +33,6 @@ const hintRequest: PermissionRequest = { hint: HINT, }, ], - id: "req-1", }; function bodySelect(view: ReturnType): SelectRenderable { @@ -91,6 +90,7 @@ describe("decision choice rendering", () => { const emitter = new EventEmitter(); const dispose = wireGates(emitter, shell); emitter.emit("permission.gate", { + id: "req-1", request: hintRequest, resolve: () => {}, }); diff --git a/src/tui/gate-events.ts b/src/tui/gate-events.ts index 30f15a0ad..601f19e4c 100644 --- a/src/tui/gate-events.ts +++ b/src/tui/gate-events.ts @@ -1,6 +1,9 @@ import type { ApprovalOutcome, PermissionRequest } from "../permission/types.js"; import type { OperatorResult } from "../agent/tools.js"; +/** Fail-closed settle when no approval UI can bind the operator's accept. */ +export const APPROVAL_UNAVAILABLE_MESSAGE = "no approval UI available; request denied" as const; + export interface OperatorGateEvent { /** Minted by the session emitter, never by the TUI overlay. */ id: string; @@ -23,6 +26,8 @@ export interface OperatorGateEvent { } export interface PermissionGateEvent { + /** Minted at overlay-open by the TUI emitter, never on PermissionRequest. */ + id: string; request: PermissionRequest; resolve: (outcome: ApprovalOutcome) => void; /** diff --git a/src/tui/gate-wire.test.ts b/src/tui/gate-wire.test.ts index f9fc72358..b8691e511 100644 --- a/src/tui/gate-wire.test.ts +++ b/src/tui/gate-wire.test.ts @@ -18,6 +18,7 @@ import { } from "./shell/overlay-host.js"; import { moveOverlaySelection, toggleOverlayExpand } from "./shell/overlay-list.js"; import { streamRowGutter } from "./stream.js"; +import { APPROVAL_UNAVAILABLE_MESSAGE } from "./gate-events.js"; import { approvalOutcomeFromSelection, operatorCancelResult, @@ -36,13 +37,14 @@ const baseRequest = (overrides: Partial = {}): PermissionRequ action: "Run shell command", subject: "bun test", scopes: [], - id: "req-1", ...overrides, }); +const unavailable = { allow: false, message: APPROVAL_UNAVAILABLE_MESSAGE }; + describe("permissionChoicesFromRequest", () => { test("always includes reject + accept once", () => { - const choices = permissionChoicesFromRequest(baseRequest({ id: "req-1" })); + const choices = permissionChoicesFromRequest(baseRequest(), "req-1"); expect(choices.items).toEqual(["Reject", "Accept once"]); expect(choices.itemIds).toEqual([`req-1:${PERMISSION_DENY_ID}`, `req-1:${PERMISSION_ONCE_ID}`]); expect(choices.outcomes).toEqual([{ allow: false }, { allow: true }]); @@ -63,9 +65,9 @@ describe("permissionChoicesFromRequest", () => { }; const choices = permissionChoicesFromRequest( baseRequest({ - id: "req-1", scopes: [scopeWithPattern, onceScope], }), + "req-1", ); expect(choices.items).toEqual(["Reject", "Accept once", "Allow git *", "Allow this path"]); expect(choices.itemIds).toEqual([ @@ -83,10 +85,9 @@ describe("permissionChoicesFromRequest", () => { }); describe("approvalOutcomeFromSelection", () => { - test("index maps to parallel outcomes; OOB denies", () => { + test("id maps to parallel outcomes; omitted or unknown id is unavailable", () => { const choices = permissionChoicesFromRequest( baseRequest({ - id: "req-1", scopes: [ { id: "proj", @@ -96,29 +97,41 @@ describe("approvalOutcomeFromSelection", () => { }, ], }), + "req-1", ); - expect(approvalOutcomeFromSelection(choices, { index: 0 })).toEqual({ + expect( + approvalOutcomeFromSelection(choices, { + index: 0, + id: `req-1:${PERMISSION_DENY_ID}`, + }), + ).toEqual({ allow: false, }); - expect(approvalOutcomeFromSelection(choices, { index: 1 })).toEqual({ + expect( + approvalOutcomeFromSelection(choices, { + index: 0, + id: `req-1:${PERMISSION_ONCE_ID}`, + }), + ).toEqual({ allow: true, }); - expect(approvalOutcomeFromSelection(choices, { index: 2 }).allow).toBe(true); - expect(approvalOutcomeFromSelection(choices, { index: 2 }).persist?.id).toBe("proj"); - expect(approvalOutcomeFromSelection(choices, { index: 99 })).toEqual({ - allow: false, - }); + expect(approvalOutcomeFromSelection(choices, { index: 0, id: "req-1:proj" }).allow).toBe(true); + expect(approvalOutcomeFromSelection(choices, { index: 0, id: "req-1:proj" }).persist?.id).toBe( + "proj", + ); + expect(approvalOutcomeFromSelection(choices, { index: 0 })).toEqual(unavailable); + expect(approvalOutcomeFromSelection(choices, { index: 99 })).toEqual(unavailable); }); test("id preferred over index when present", () => { const choices = permissionChoicesFromRequest( baseRequest({ - id: "req-1", scopes: [ { id: "a", label: "A", pattern: "a*" }, { id: "b", label: "B", pattern: "b*" }, ], }), + "req-1", ); const byId = approvalOutcomeFromSelection(choices, { index: 0, @@ -128,14 +141,14 @@ describe("approvalOutcomeFromSelection", () => { expect(byId.persist?.id).toBe("b"); }); - test("unknown id denies without falling back to index", () => { - const choices = permissionChoicesFromRequest(baseRequest({ id: "req-1" })); + test("unknown id is unavailable without falling back to index", () => { + const choices = permissionChoicesFromRequest(baseRequest(), "req-1"); expect( approvalOutcomeFromSelection(choices, { index: 1, id: "missing", }), - ).toEqual({ allow: false }); + ).toEqual(unavailable); }); }); @@ -223,17 +236,17 @@ describe("operatorChoicesFromOptions / operatorResultFromSelection", () => { expect(choices.itemIds).toEqual(["ask-1:0", "ask-1:1", "ask-1:2"]); }); - test("selection index → option; OOB → cancel", () => { + test("selection id → option; omitted or unknown id → cancel", () => { const choices = operatorChoicesFromOptions(["A", "B"], "ask-1"); - expect(operatorResultFromSelection(choices, { index: 0 })).toEqual({ + expect(operatorResultFromSelection(choices, { index: 0, id: "ask-1:0" })).toEqual({ kind: "option", index: 0, }); - expect(operatorResultFromSelection(choices, { index: 1 })).toEqual({ + expect(operatorResultFromSelection(choices, { index: 1, id: "ask-1:1" })).toEqual({ kind: "option", index: 1, }); - expect(operatorResultFromSelection(choices, { index: -1 })).toEqual({ + expect(operatorResultFromSelection(choices, { index: 0 })).toEqual({ kind: "cancel", }); expect(operatorResultFromSelection(choices, { index: 9 })).toEqual({ @@ -299,11 +312,11 @@ describe("wireGates", () => { action: "Run shell command", subject: "bun test", scopes: [], - id: "req-1", }; try { const dispose = wireGates(emitter, shell); emitter.emit("permission.gate", { + id: "req-1", request, resolve: (outcome: unknown) => { resolved = outcome; @@ -335,11 +348,10 @@ describe("wireGates", () => { action: "Run shell command", subject: "echo start && cat > notes.txt < {} }); + emitter.emit("permission.gate", { id: "req-1", request, resolve: () => {} }); const collapsed = shell.overlayBodyLines.join("\n"); expect(collapsed).toContain("1) echo start"); @@ -470,8 +482,8 @@ describe("wireGates", () => { try { const dispose = wireGates(emitter, shell); emitter.emit("permission.gate", { + id: "req-a", request: baseRequest({ - id: "req-a", subject: "git status", scopes: [{ id: "scope-a", label: "Allow git A", pattern: "git A*" }], }), @@ -491,8 +503,8 @@ describe("wireGates", () => { expect(shell.overlayList).toBeNull(); emitter.emit("permission.gate", { + id: "req-b", request: baseRequest({ - id: "req-b", subject: "git push", scopes: [{ id: "scope-b", label: "Allow git B", pattern: "git B*" }], }), @@ -538,8 +550,8 @@ describe("wireGates", () => { try { const dispose = wireGates(emitter, shell); emitter.emit("permission.gate", { + id: "req-a", request: baseRequest({ - id: "req-a", subject: "git status", scopes: [{ id: "scope-a", label: "Allow git A", pattern: "git A*" }], }), @@ -552,8 +564,8 @@ describe("wireGates", () => { expect(shell.overlayList).toBeNull(); emitter.emit("permission.gate", { + id: "req-b", request: baseRequest({ - id: "req-b", subject: "git push", scopes: [{ id: "scope-b", label: "Allow git B", pattern: "git B*" }], }), @@ -580,6 +592,71 @@ describe("wireGates", () => { }); }); + test("Enter with a painted id missing from the live bag is unavailable", async () => { + await withTestRenderer(async (h) => { + const shell = createAppShell(h.renderer, { + terminal: { columns: 80, rows: 24 }, + run: "idle", + }); + const emitter = new EventEmitter(); + let resolved: unknown; + try { + const dispose = wireGates(emitter, shell); + emitter.emit("permission.gate", { + id: "req-b", + request: baseRequest({ subject: "git push" }), + resolve: (outcome: unknown) => { + resolved = outcome; + }, + }); + expect(shell.overlayKind).toBe("permissions"); + const list = shell.overlayList; + if (!list) throw new Error("expected an open overlay list"); + list.select.options = [ + { name: "Reject", description: "", value: `req-a:${PERMISSION_DENY_ID}` }, + { name: "Accept once", description: "", value: `req-a:${PERMISSION_ONCE_ID}` }, + ]; + list.select.setSelectedIndex(1); + acceptOverlaySelection(shell); + expect(resolved).toEqual(unavailable); + expect(shell.overlayList).toBeNull(); + dispose(); + } finally { + shell.dispose(); + } + }); + }); + + test("Enter on an empty permission list is unavailable, not reject", async () => { + await withTestRenderer(async (h) => { + const shell = createAppShell(h.renderer, { + terminal: { columns: 80, rows: 24 }, + run: "idle", + }); + const emitter = new EventEmitter(); + let resolved: unknown; + try { + const dispose = wireGates(emitter, shell); + emitter.emit("permission.gate", { + id: "req-b", + request: baseRequest({ subject: "git push" }), + resolve: (outcome: unknown) => { + resolved = outcome; + }, + }); + expect(shell.overlayKind).toBe("permissions"); + shell.overlayItems = []; + acceptOverlaySelection(shell); + expect(resolved).toEqual(unavailable); + expect(resolved).not.toEqual({ allow: false }); + expect(shell.overlayList).toBeNull(); + dispose(); + } finally { + shell.dispose(); + } + }); + }); + test("operator.gate without id cancels without opening", async () => { await withTestRenderer(async (h) => { const shell = createAppShell(h.renderer, { @@ -606,7 +683,7 @@ describe("wireGates", () => { }); }); - test("permission.gate without request.id denies without opening", async () => { + test("permission.gate without id is unavailable without opening", async () => { await withTestRenderer(async (h) => { const shell = createAppShell(h.renderer, { terminal: { columns: 80, rows: 24 }, @@ -627,7 +704,7 @@ describe("wireGates", () => { resolved = outcome; }, }); - expect(resolved).toEqual({ allow: false }); + expect(resolved).toEqual(unavailable); expect(shell.overlayKind).not.toBe("permissions"); dispose(); } finally { @@ -648,11 +725,10 @@ describe("wireGates", () => { action: "Run shell command", subject: "ls -la ~/.corbits/projects", scopes: [], - id: "req-1", }; try { const dispose = wireGates(emitter, shell); - emitter.emit("permission.gate", { request, resolve: () => {} }); + emitter.emit("permission.gate", { id: "req-1", request, resolve: () => {} }); expect(shell.streamLog.filter((r) => r.meta === "permission")).toHaveLength(0); @@ -678,7 +754,7 @@ describe("gate decisions stay out of the transcript", () => { const emitter = new EventEmitter(); try { wireGates(emitter, shell); - emitter.emit("permission.gate", { request: baseRequest(), resolve: () => {} }); + emitter.emit("permission.gate", { id: "req-1", request: baseRequest(), resolve: () => {} }); const before = shell.streamLog.length; acceptOverlaySelection(shell); @@ -698,7 +774,7 @@ describe("gate decisions stay out of the transcript", () => { const emitter = new EventEmitter(); try { wireGates(emitter, shell); - emitter.emit("permission.gate", { request: baseRequest(), resolve: () => {} }); + emitter.emit("permission.gate", { id: "req-1", request: baseRequest(), resolve: () => {} }); const before = shell.streamLog.length; closeInsetOverlay(shell); @@ -811,6 +887,7 @@ describe("gate decisions stay out of the transcript", () => { wireGates(emitter, shell); const before = shell.streamLog.length; emitter.emit("permission.gate", { + id: "req-1", request: baseRequest(), resolve: () => {}, timeoutMs: 5, @@ -835,6 +912,7 @@ describe("gate decisions stay out of the transcript", () => { wireGates(emitter, shell); const before = shell.streamLog.length; emitter.emit("permission.gate", { + id: "req-1", request: baseRequest(), resolve: () => {}, signal: controller.signal, @@ -865,6 +943,7 @@ describe("gate decisions stay out of the transcript", () => { wireGates(emitter, shell); const before = shell.streamLog.length; emitter.emit("permission.gate", { + id: "req-1", request: baseRequest(), resolve: () => { resolveCount += 1; @@ -897,12 +976,14 @@ describe("gate decisions stay out of the transcript", () => { try { wireGates(emitter, shell); emitter.emit("permission.gate", { + id: "req-1", request: baseRequest(), resolve: () => {}, }); const before = shell.streamLog.length; emitter.emit("permission.gate", { - request: baseRequest({ tool: "queued_tool", id: "req-2" }), + id: "req-2", + request: baseRequest({ tool: "queued_tool" }), resolve: () => { resolveCount += 1; }, @@ -944,12 +1025,14 @@ describe("gate decisions stay out of the transcript", () => { // Occupies the overlay host so the second request queues instead of // opening — the drain below must resolve it without ever opening it. emitter.emit("permission.gate", { + id: "req-1", request: baseRequest(), resolve: () => {}, }); const before = shell.streamLog.length; emitter.emit("permission.gate", { - request: baseRequest({ tool: "queued_tool", id: "req-2" }), + id: "req-2", + request: baseRequest({ tool: "queued_tool" }), resolve: (outcome: unknown) => { resolveCount += 1; resolved = outcome; @@ -982,6 +1065,7 @@ describe("gate decisions stay out of the transcript", () => { wireGates(emitter, shell); const before = shell.streamLog.length; emitter.emit("permission.gate", { + id: "req-1", request: baseRequest(), resolve: () => { resolveCount += 1; @@ -1022,6 +1106,7 @@ describe("gate decisions stay out of the transcript", () => { let queuedResolved: unknown; const dispose = wireGates(emitter, shell); emitter.emit("permission.gate", { + id: "req-1", request: baseRequest(), resolve: () => { openResolveCount += 1; @@ -1031,7 +1116,8 @@ describe("gate decisions stay out of the transcript", () => { // opening — dispose must deny it without ever displaying it. const before = shell.streamLog.length; emitter.emit("permission.gate", { - request: baseRequest({ tool: "queued_tool", id: "req-2" }), + id: "req-2", + request: baseRequest({ tool: "queued_tool" }), resolve: (outcome: unknown) => { queuedResolveCount += 1; queuedResolved = outcome; @@ -1061,6 +1147,7 @@ describe("permission.gate auto-deny", () => { try { wireGates(emitter, shell); emitter.emit("permission.gate", { + id: "req-1", request: baseRequest(), resolve: (outcome: unknown) => { resolved = outcome; @@ -1095,6 +1182,7 @@ describe("permission.gate auto-deny", () => { try { wireGates(emitter, shell); emitter.emit("permission.gate", { + id: "req-1", request: baseRequest(), resolve: (outcome: unknown) => { resolved = outcome; @@ -1128,6 +1216,7 @@ describe("permission.gate auto-deny", () => { try { wireGates(emitter, shell); emitter.emit("permission.gate", { + id: "req-1", request: baseRequest(), resolve: (outcome: unknown) => { resolveCount += 1; @@ -1160,13 +1249,15 @@ describe("permission.gate auto-deny", () => { try { wireGates(emitter, shell); emitter.emit("permission.gate", { + id: "req-1", request: baseRequest(), resolve: (outcome: unknown) => { firstResolved = outcome; }, }); emitter.emit("permission.gate", { - request: baseRequest({ tool: "queued_tool", id: "req-2" }), + id: "req-2", + request: baseRequest({ tool: "queued_tool" }), resolve: (outcome: unknown) => { secondResolved = outcome; }, @@ -1283,6 +1374,7 @@ describe("operator.gate auto-cancel", () => { try { wireGates(emitter, shell); emitter.emit("permission.gate", { + id: "req-1", request: baseRequest(), resolve: () => {}, }); @@ -1321,6 +1413,7 @@ describe("operator.gate auto-cancel", () => { try { wireGates(emitter, shell); emitter.emit("permission.gate", { + id: "req-1", request: baseRequest(), resolve: (outcome: unknown) => { firstResolved = outcome; @@ -1470,6 +1563,7 @@ describe("Esc on a gate overlay settles the awaited promise", () => { try { wireGates(emitter, shell); emitter.emit("permission.gate", { + id: "req-1", request: baseRequest(), resolve: (outcome: unknown) => { resolveCount += 1; @@ -1483,6 +1577,7 @@ describe("Esc on a gate overlay settles the awaited promise", () => { expect(shell.overlayList).toBeNull(); expect(resolveCount).toBe(1); expect(resolved).toEqual({ allow: false }); + expect(resolved).not.toEqual(unavailable); } finally { shell.dispose(); } @@ -1528,6 +1623,7 @@ describe("permission overlay height", () => { const emitter = new EventEmitter(); wireGates(emitter, shell); emitter.emit("permission.gate", { + id: "req-1", request: { tool: "run_shell", action: "Run shell command", @@ -1537,7 +1633,6 @@ describe("permission overlay height", () => { label: `Always allow scope ${i}`, pattern: `p${i}`, })), - id: "req-1", }, resolve: () => {}, }); @@ -1722,6 +1817,7 @@ describe("operator question overlay", () => { try { wireGates(emitter, shell); emitter.emit("permission.gate", { + id: "req-1", request: baseRequest(), resolve: (o: unknown) => { approved = o; diff --git a/src/tui/gate-wire.ts b/src/tui/gate-wire.ts index 37cd01b62..631488273 100644 --- a/src/tui/gate-wire.ts +++ b/src/tui/gate-wire.ts @@ -18,7 +18,11 @@ import { setOverlayBody, } from "./shell/overlay-host.js"; import { EXPAND_KEY } from "./stream.js"; -import type { OperatorGateEvent, PermissionGateEvent } from "./gate-events.js"; +import { + APPROVAL_UNAVAILABLE_MESSAGE, + type OperatorGateEvent, + type PermissionGateEvent, +} from "./gate-events.js"; import { createPermissionRequestQueue, wirePermissionGrantReconciliation, @@ -40,13 +44,13 @@ export const PERMISSION_EXPAND_KEY = EXPAND_KEY; export interface PermissionGateChoices { readonly items: readonly string[]; readonly itemIds: readonly string[]; - /** Parallel to items — index into this on accept. */ + /** Parallel to itemIds — looked up by selection id. */ readonly outcomes: readonly ApprovalOutcome[]; } export interface GateSelection { readonly index: number; - /** When present, preferred over index for outcome lookup. */ + /** When present, the only lookup key. Omitted id fail-closes. */ readonly id?: string; } @@ -57,11 +61,14 @@ export interface GateSelection { * the list (see permissionBodyFromRequest) instead of being truncated inside * a choice row. */ -export function permissionChoicesFromRequest(request: PermissionRequest): PermissionGateChoices { +export function permissionChoicesFromRequest( + request: PermissionRequest, + askId: string, +): PermissionGateChoices { const items: string[] = []; const itemIds: string[] = []; const outcomes: ApprovalOutcome[] = []; - const rowId = (part: string): string => `${request.id}:${part}`; + const rowId = (part: string): string => `${askId}:${part}`; items.push("Reject"); itemIds.push(rowId(PERMISSION_DENY_ID)); @@ -84,21 +91,21 @@ export function permissionChoicesFromRequest(request: PermissionRequest): Permis } /** - * Map overlay selection index/id → ApprovalOutcome. - * Unknown / out-of-range defaults to deny (safe closed). + * Map overlay selection id → ApprovalOutcome. + * Unknown or omitted id fail-closes as unavailable. No index fallback. */ export function approvalOutcomeFromSelection( choices: PermissionGateChoices, selection: GateSelection, ): ApprovalOutcome { - if (selection.id !== undefined) { - const byId = choices.itemIds.indexOf(selection.id); - if (byId >= 0) { - return choices.outcomes[byId] ?? { allow: false }; - } - return { allow: false }; + if (selection.id === undefined) { + return { allow: false, message: APPROVAL_UNAVAILABLE_MESSAGE }; + } + const byId = choices.itemIds.indexOf(selection.id); + if (byId >= 0) { + return choices.outcomes[byId] ?? { allow: false, message: APPROVAL_UNAVAILABLE_MESSAGE }; } - return choices.outcomes[selection.index] ?? { allow: false }; + return { allow: false, message: APPROVAL_UNAVAILABLE_MESSAGE }; } export interface PermissionBodyOpts { @@ -165,23 +172,20 @@ export function operatorChoicesFromOptions( /** * Map selection → OperatorResult. - * Present unknown id → cancel (safe closed). No index fallback. + * Unknown or omitted id → cancel. No index fallback. */ export function operatorResultFromSelection( choices: OperatorGateChoices, selection: GateSelection, ): OperatorResult { - if (selection.id !== undefined) { - const byId = choices.itemIds.indexOf(selection.id); - if (byId >= 0) { - return { kind: "option", index: byId }; - } + if (selection.id === undefined) { return { kind: "cancel" }; } - if (selection.index < 0 || selection.index >= choices.items.length) { - return { kind: "cancel" }; + const byId = choices.itemIds.indexOf(selection.id); + if (byId >= 0) { + return { kind: "option", index: byId }; } - return { kind: "option", index: selection.index }; + return { kind: "cancel" }; } export function operatorCancelResult(): OperatorResult { @@ -294,11 +298,11 @@ export function wireGates( function onPermission(ev: PermissionGateEvent): void { hooks.onGateOpened(); const resolve = onceClosed(hooks.onGateClosed, ev.resolve); - if (ev.request.id === undefined || ev.request.id.length === 0) { - resolve({ allow: false }); + if (typeof ev.id !== "string" || ev.id.length === 0) { + resolve({ allow: false, message: APPROVAL_UNAVAILABLE_MESSAGE }); return; } - const choices = permissionChoicesFromRequest(ev.request); + const choices = permissionChoicesFromRequest(ev.request, ev.id); const collapsedBody = permissionBodyFromRequest(ev.request, { hint: true }); // Nothing was collapsed → no expand affordance, so the overlay leaves the // bare key unclaimed. diff --git a/src/tui/mention-popup.test.ts b/src/tui/mention-popup.test.ts index c07213ff0..31e786161 100644 --- a/src/tui/mention-popup.test.ts +++ b/src/tui/mention-popup.test.ts @@ -253,12 +253,12 @@ describe("@ popup narrows as you type", () => { let resolved: unknown; emitter.emit("permission.gate", { + id: "req-1", request: { tool: "run_shell", action: "Run shell command", subject: "bun test", scopes: [], - id: "req-1", }, resolve: (outcome: unknown) => { resolved = outcome; @@ -306,12 +306,12 @@ describe("@ popup narrows as you type", () => { const pending = openAtMentionSuggestions(shell); emitter.emit("permission.gate", { + id: "req-1", request: { tool: "run_shell", action: "Run shell command", subject: "bun test", scopes: [], - id: "req-1", }, resolve: () => {}, }); diff --git a/src/tui/overlay-overflow.test.ts b/src/tui/overlay-overflow.test.ts index db480e5de..ecf164a99 100644 --- a/src/tui/overlay-overflow.test.ts +++ b/src/tui/overlay-overflow.test.ts @@ -222,19 +222,19 @@ describe("gate-wire approval overflow on short terminal", () => { label: `Always allow scope ${i}`, pattern: `p${i}`, })), - id: "req-1", }; try { primeSession(shell); const dispose = wireGates(emitter, shell); emitter.emit("permission.gate", { + id: "req-1", request, resolve: (outcome: unknown) => { resolved = outcome; }, }); - const choices = permissionChoicesFromRequest(request); + const choices = permissionChoicesFromRequest(request, "req-1"); expect(shell.overlayKind).toBe("permissions"); expect(shell.overlayItems).toEqual([...choices.items]); const list = shell.overlayList!; @@ -310,12 +310,11 @@ describe("gate-wire approval overflow on short terminal", () => { action: "Run shell command", subject: 'git commit -m "line one\nline two\nline three\nline four\nline five"', scopes: [{ id: "session", label: "Allow for session", pattern: "git *" }], - id: "req-1", }; try { primeSession(shell); const dispose = wireGates(emitter, shell); - emitter.emit("permission.gate", { request, resolve: () => {} }); + emitter.emit("permission.gate", { id: "req-1", request, resolve: () => {} }); const body = permissionBodyFromRequest(request, { hint: true }); // The raw body still carries the collapsed-command hint — only what diff --git a/src/tui/overlays.test.ts b/src/tui/overlays.test.ts index a2ca6a445..503414123 100644 --- a/src/tui/overlays.test.ts +++ b/src/tui/overlays.test.ts @@ -321,7 +321,7 @@ describe("overlay accept callbacks", () => { ); }); - test("gate accept with a painted value missing from live itemIds denies instead of remapping by index", async () => { + test("gate accept with a painted value missing from live itemIds dispatches that id instead of remapping by index", async () => { await withTestRenderer( async (h) => { const shell = createAppShell(h.renderer, { @@ -350,8 +350,79 @@ describe("overlay accept callbacks", () => { ]; list.select.setSelectedIndex(1); acceptOverlaySelection(shell); - expect(accepted).toEqual([]); - expect(cancelled).toBe(1); + expect(accepted).toEqual([ + { + kind: "permissions", + index: 1, + label: "Accept once", + id: "req-a:__once__", + }, + ]); + expect(cancelled).toBe(0); + expect(shell.overlayList).toBeNull(); + } finally { + shell.dispose(); + } + }, + { width: 80, height: 24 }, + ); + }); + + test("stranded operator gate Enter fail-closes via onAccept without id, not onCancel", async () => { + await withTestRenderer( + async (h) => { + const shell = createAppShell(h.renderer, { + terminal: { columns: 80, rows: 24 }, + wireKeys: false, + }); + try { + const accepted: OverlaySelection[] = []; + let cancelled = 0; + openOperatorOverlay(shell, { + body: "Proceed?", + choices: [], + isGate: true, + echoChoice: false, + onAccept: (s) => accepted.push(s), + onCancel: () => { + cancelled += 1; + }, + }); + acceptOverlaySelection(shell); + expect(accepted).toEqual([{ kind: "operator", index: 0, label: "" }]); + expect(cancelled).toBe(0); + expect(shell.overlayList).toBeNull(); + } finally { + shell.dispose(); + } + }, + { width: 80, height: 24 }, + ); + }); + + test("gate Enter on an empty permission list fail-closes via onAccept without id, not onCancel", async () => { + await withTestRenderer( + async (h) => { + const shell = createAppShell(h.renderer, { + terminal: { columns: 80, rows: 24 }, + wireKeys: false, + }); + try { + const accepted: OverlaySelection[] = []; + let cancelled = 0; + openPermissionsOverlay(shell, { + items: [], + itemIds: [], + isGate: true, + echoChoice: false, + onAccept: (s) => accepted.push(s), + onCancel: () => { + cancelled += 1; + }, + }); + acceptOverlaySelection(shell); + expect(accepted).toEqual([{ kind: "permissions", index: 0, label: "" }]); + expect(cancelled).toBe(0); expect(shell.overlayList).toBeNull(); } finally { shell.dispose(); diff --git a/src/tui/product-host.test.ts b/src/tui/product-host.test.ts index f3d0a0cff..0d8664a8d 100644 --- a/src/tui/product-host.test.ts +++ b/src/tui/product-host.test.ts @@ -191,9 +191,9 @@ describe("mountProductHost", () => { action: "run", subject: "ls", scopes: [], - id: "req-1", }; emitter.emit("permission.gate", { + id: "req-1", request, resolve: (outcome: unknown) => { resolved = outcome; diff --git a/src/tui/request-approval.test.ts b/src/tui/request-approval.test.ts index 013972e83..8185a475f 100644 --- a/src/tui/request-approval.test.ts +++ b/src/tui/request-approval.test.ts @@ -100,37 +100,22 @@ describe("createGateRequestApproval", () => { expect((await pending).allow).toBe(true); }); - test("forwards the gate-minted request id without minting a replacement", async () => { - const withId: PermissionRequest = { ...request, id: "req-from-gate" }; - let captured: PermissionGateEvent | undefined; + test("mints a unique event id without reading PermissionRequest", async () => { + const ids: string[] = []; const requestApproval = createGateRequestApproval({ emitGate: (event) => { - captured = event; + ids.push(event.id); + event.resolve({ allow: true }); return true; }, approvalTimeout: noTimeout, }); - const pending = requestApproval(withId); - expect(captured?.request).toBe(withId); - expect(captured?.request.id).toBe("req-from-gate"); - captured?.resolve({ allow: true }); - await pending; - }); - - test("does not mint an id when the request has none", async () => { - let captured: PermissionGateEvent | undefined; - const requestApproval = createGateRequestApproval({ - emitGate: (event) => { - captured = event; - return true; - }, - approvalTimeout: noTimeout, - }); - const pending = requestApproval(request); - expect(captured?.request).toBe(request); - expect(captured?.request.id).toBeUndefined(); - captured?.resolve({ allow: false }); - await pending; + expect((await requestApproval(request)).allow).toBe(true); + expect((await requestApproval(request)).allow).toBe(true); + expect(ids).toHaveLength(2); + expect(ids[0]).toEqual(expect.any(String)); + expect(ids[0]?.length).toBeGreaterThan(0); + expect(ids[1]).not.toBe(ids[0]); }); }); diff --git a/src/tui/request-approval.ts b/src/tui/request-approval.ts index a50dbf792..6f8a31225 100644 --- a/src/tui/request-approval.ts +++ b/src/tui/request-approval.ts @@ -1,8 +1,9 @@ import { getLogger } from "@intx/log"; +import { randomUUID } from "node:crypto"; import { LOG_NAMESPACE_ROOT } from "../branding.js"; import type { ApprovalOutcome, PermissionRequest, RequestApproval } from "../permission/types.js"; import { getToolApprovalBudget } from "./tool-execution-watchdog.js"; -import type { PermissionGateEvent } from "./gate-events.js"; +import { APPROVAL_UNAVAILABLE_MESSAGE, type PermissionGateEvent } from "./gate-events.js"; export interface CreateGateRequestApprovalArgs { /** Emits the gate event to the UI; returns false when nothing is listening. */ @@ -70,12 +71,8 @@ export function createGateRequestApproval(args: CreateGateRequestApprovalArgs): kind: "permission", }); const timeout = args.approvalTimeout(); - if (request.id === undefined || request.id.length === 0) { - logger.warn("permission gate request missing id for {tool}; emitting without minting", { - tool: request.tool, - }); - } const event: PermissionGateEvent = { + id: randomUUID(), request, resolve: finish, ...(timeout !== undefined ? timeout : {}), @@ -87,7 +84,7 @@ export function createGateRequestApproval(args: CreateGateRequestApprovalArgs): logger.warn("permission gate emitted with no listener for {tool}; denying", { tool: request.tool, }); - finish({ allow: false, message: "no approval UI available; request denied" }); + finish({ allow: false, message: APPROVAL_UNAVAILABLE_MESSAGE }); } }); } diff --git a/src/tui/shell/overlay-host.ts b/src/tui/shell/overlay-host.ts index 58691fd87..f050f4e85 100644 --- a/src/tui/shell/overlay-host.ts +++ b/src/tui/shell/overlay-host.ts @@ -740,14 +740,30 @@ export function acceptOverlaySelection(shell: AppShell): void { confirmCopySelection(shell); return; } - // Nothing to choose: Enter must not synthesize a phantom row and resolve the - // gate with it. The answer field (when offered) already claimed Enter. - if (shell.overlayItems.length === 0) return; + + const bag = shellInternals(shell); + const kind = shell.overlayKind ?? "demo"; + // Empty chooser: Enter must not synthesize a phantom row. Stay open when a + // free-text answer field is the way to reply, or when this is not a live + // gate. A live gate with nowhere to answer fail-closes via onAccept with no + // id so the helper can mark unavailable instead of hanging or impersonating + // Esc/Reject through onCancel. + if (shell.overlayItems.length === 0) { + if (bag?.primaryBindings.isGate !== true || overlayAnswerState(shell) !== null) return; + const perOpen = bag.primaryBindings.onAccept ?? null; + bag.primaryBindings.onCancel = null; + const release = reserveOverlayHost(shell); + closeInsetOverlay(shell); + try { + dispatchOverlayAccept(shell, { kind, index: 0, label: "" }, perOpen); + } finally { + release(); + } + return; + } const idx = shell.overlayList.activeIndex; const label = shell.overlayItems[idx] ?? `item ${idx}`; - const kind = shell.overlayKind ?? "demo"; - const bag = shellInternals(shell); if (kind === "palette") { const cmd = shell.paletteCommands[idx]; @@ -779,14 +795,24 @@ export function acceptOverlaySelection(shell: AppShell): void { const idKeyed = typeof painted === "string" && itemIds.includes(painted); // Gate accept is id-keyed. A painted Select value missing from the live // itemIds is a stale or mismatched row — remapping via index would bind - // Enter to the new question's same-index choice. Fail closed instead. - if (bag?.primaryBindings.isGate === true && !idKeyed) { - closeInsetOverlay(shell); - return; + // Enter to the new question's same-index choice. Dispatch the painted id + // (or omit id) so the gate helper fail-closes as unavailable instead of + // impersonating Reject through onCancel. + let id: string | undefined; + if (idKeyed) { + id = painted; + } else if (bag?.primaryBindings.isGate !== true) { + id = itemIds[idx]; + } else if (typeof painted === "string") { + id = painted; + } + // Type-to-filter plants "(no matches)" with an empty-id sentinel. Stay open + // on non-gate lists. A live gate must not dead-end — omit the sentinel so + // the helper fail-closes as unavailable. + if (id === "") { + if (bag?.primaryBindings.isGate !== true) return; + id = undefined; } - const id = idKeyed ? painted : itemIds[idx]; - // Type-to-filter plants "(no matches)" with an empty-id sentinel. Stay open. - if (id === "") return; const value = bag?.primaryBindings.itemValues[idx]; const selection: OverlaySelection = { kind, diff --git a/src/tui/slash-popup-gate.test.ts b/src/tui/slash-popup-gate.test.ts index 4b923224d..320134ccb 100644 --- a/src/tui/slash-popup-gate.test.ts +++ b/src/tui/slash-popup-gate.test.ts @@ -95,12 +95,12 @@ function emitPermissionGate( extra?: { readonly timeoutMs?: number; readonly tool?: string }, ): void { emitter.emit("permission.gate", { + id: extra?.tool ?? "req-1", request: { tool: extra?.tool ?? "run_shell", action: "Run shell command", subject: "bun test", scopes: [], - id: extra?.tool ?? "req-1", }, resolve, ...(extra?.timeoutMs !== undefined ? { timeoutMs: extra.timeoutMs } : {}), From f80957514a6c81be9c5273a454c3880f71faf25b Mon Sep 17 00:00:00 2001 From: Sawyer Date: Wed, 9 Sep 2026 12:23:53 -0700 Subject: [PATCH 5/5] Document that ask ids mint at gate emit Ids exist on the gate event before the overlay opens, so the selector contract and PermissionGateEvent comment should not say they mint at overlay-open. --- docs/TUI.md | 5 +++-- src/tui/gate-events.ts | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/TUI.md b/docs/TUI.md index 1919924a8..8b2e13aa6 100644 --- a/docs/TUI.md +++ b/docs/TUI.md @@ -342,8 +342,9 @@ viewport kit: shared windowing, keep-active-visible, and page/jump behavior. There is exactly one scroll lease at a time; keyboard paging and the mouse wheel both follow whichever surface currently holds it, so a modal open on top of the transcript never lets the wheel move the transcript underneath it. -Ask and permission rows are namespaced by the ask id minted when the overlay -opens. Paint replaces the whole options array (labels and ids together); +Ask and permission rows are namespaced by the ask id minted on the gate +event at emit, before the overlay opens. Paint replaces the whole options +array (labels and ids together); there is no drop-by-id merge. Enter binds by the painted id against the live bag — a painted value that is not in that bag, or Enter on an empty gate with no answer field, fail-closes the accept as unavailable rather than diff --git a/src/tui/gate-events.ts b/src/tui/gate-events.ts index 601f19e4c..3c54a8bb7 100644 --- a/src/tui/gate-events.ts +++ b/src/tui/gate-events.ts @@ -26,7 +26,7 @@ export interface OperatorGateEvent { } export interface PermissionGateEvent { - /** Minted at overlay-open by the TUI emitter, never on PermissionRequest. */ + /** Minted by the session emitter at gate emit, never on PermissionRequest. */ id: string; request: PermissionRequest; resolve: (outcome: ApprovalOutcome) => void;