diff --git a/CHANGELOG.md b/CHANGELOG.md index b27dd4cdd..58e2e26cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,13 @@ 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. 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 ### Added diff --git a/docs/TUI.md b/docs/TUI.md index d44e42db9..8b2e13aa6 100644 --- a/docs/TUI.md +++ b/docs/TUI.md @@ -342,6 +342,14 @@ 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 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 +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.ts b/src/permission/gate.ts index e104f5dc8..181aef23b 100644 --- a/src/permission/gate.ts +++ b/src/permission/gate.ts @@ -42,6 +42,20 @@ 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 @@ -58,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 diff --git a/src/tui/decision-truncation.test.ts b/src/tui/decision-truncation.test.ts index c560c59ba..37c6e700c 100644 --- a/src/tui/decision-truncation.test.ts +++ b/src/tui/decision-truncation.test.ts @@ -90,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 11cd1a726..3c54a8bb7 100644 --- a/src/tui/gate-events.ts +++ b/src/tui/gate-events.ts @@ -1,7 +1,12 @@ 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; question: string; options: string[]; resolve: (result: OperatorResult) => void; @@ -21,6 +26,8 @@ export interface OperatorGateEvent { } export interface PermissionGateEvent { + /** Minted by the session emitter at gate emit, 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 3adf722fe..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, @@ -39,11 +40,13 @@ const baseRequest = (overrides: Partial = {}): PermissionRequ ...overrides, }); +const unavailable = { allow: false, message: APPROVAL_UNAVAILABLE_MESSAGE }; + describe("permissionChoicesFromRequest", () => { test("always includes reject + accept once", () => { - const choices = permissionChoicesFromRequest(baseRequest()); + const choices = permissionChoicesFromRequest(baseRequest(), "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 }]); }); @@ -64,13 +67,14 @@ describe("permissionChoicesFromRequest", () => { baseRequest({ scopes: [scopeWithPattern, onceScope], }), + "req-1", ); 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, @@ -81,7 +85,7 @@ 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({ scopes: [ @@ -93,18 +97,30 @@ 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", () => { @@ -115,23 +131,24 @@ describe("approvalOutcomeFromSelection", () => { { id: "b", label: "B", pattern: "b*" }, ], }), + "req-1", ); 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 is unavailable without falling back to index", () => { + const choices = permissionChoicesFromRequest(baseRequest(), "req-1"); expect( approvalOutcomeFromSelection(choices, { index: 1, id: "missing", }), - ).toEqual({ allow: true }); + ).toEqual(unavailable); }); }); @@ -212,41 +229,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({ + test("selection id → option; omitted or unknown id → cancel", () => { + const choices = operatorChoicesFromOptions(["A", "B"], "ask-1"); + expect(operatorResultFromSelection(choices, { index: 0, id: "ask-1:0" })).toEqual({ kind: "option", index: 0, }); - expect(operatorResultFromSelection(opts, { index: 1 })).toEqual({ + expect(operatorResultFromSelection(choices, { index: 1, id: "ask-1:1" })).toEqual({ kind: "option", index: 1, }); - expect(operatorResultFromSelection(opts, { index: -1 })).toEqual({ + expect(operatorResultFromSelection(choices, { index: 0 })).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", }); }); @@ -298,6 +316,7 @@ describe("wireGates", () => { try { const dispose = wireGates(emitter, shell); emitter.emit("permission.gate", { + id: "req-1", request, resolve: (outcome: unknown) => { resolved = outcome; @@ -332,7 +351,7 @@ describe("wireGates", () => { }; try { const dispose = wireGates(emitter, shell); - emitter.emit("permission.gate", { request, resolve: () => {} }); + emitter.emit("permission.gate", { id: "req-1", request, resolve: () => {} }); const collapsed = shell.overlayBodyLines.join("\n"); expect(collapsed).toContain("1) echo start"); @@ -377,6 +396,7 @@ describe("wireGates", () => { try { const dispose = wireGates(emitter, shell); emitter.emit("operator.gate", { + id: "ask-1", question: "Proceed?", options: ["Cancel", "Continue"], resolve: (result: unknown) => { @@ -396,6 +416,303 @@ 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("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", { + id: "req-a", + request: baseRequest({ + 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", { + id: "req-b", + request: baseRequest({ + 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("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", { + id: "req-a", + request: baseRequest({ + 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", { + id: "req-b", + request: baseRequest({ + 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("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, { + 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 id is unavailable 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(unavailable); + 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, { @@ -411,7 +728,7 @@ describe("wireGates", () => { }; 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); @@ -437,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); @@ -457,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); @@ -478,6 +795,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 +820,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 +845,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: () => {}, @@ -567,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, @@ -591,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, @@ -621,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; @@ -653,11 +976,13 @@ 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", { + id: "req-2", request: baseRequest({ tool: "queued_tool" }), resolve: () => { resolveCount += 1; @@ -700,11 +1025,13 @@ 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", { + id: "req-2", request: baseRequest({ tool: "queued_tool" }), resolve: (outcome: unknown) => { resolveCount += 1; @@ -738,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; @@ -778,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; @@ -787,6 +1116,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", { + id: "req-2", request: baseRequest({ tool: "queued_tool" }), resolve: (outcome: unknown) => { queuedResolveCount += 1; @@ -817,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; @@ -851,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; @@ -884,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; @@ -916,12 +1249,14 @@ 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", { + id: "req-2", request: baseRequest({ tool: "queued_tool" }), resolve: (outcome: unknown) => { secondResolved = outcome; @@ -970,6 +1305,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 +1338,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) => { @@ -1037,10 +1374,12 @@ describe("operator.gate auto-cancel", () => { try { wireGates(emitter, shell); emitter.emit("permission.gate", { + id: "req-1", request: baseRequest(), resolve: () => {}, }); emitter.emit("operator.gate", { + id: "ask-1", question: "Proceed?", options: ["Yes", "No"], resolve: (result: unknown) => { @@ -1074,12 +1413,14 @@ describe("operator.gate auto-cancel", () => { try { wireGates(emitter, shell); emitter.emit("permission.gate", { + id: "req-1", request: baseRequest(), resolve: (outcome: unknown) => { firstResolved = outcome; }, }); emitter.emit("operator.gate", { + id: "ask-1", question: "Proceed?", options: ["Yes", "No"], resolve: (result: unknown) => { @@ -1120,6 +1461,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 +1492,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 +1521,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 +1532,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) => { @@ -1218,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; @@ -1231,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(); } @@ -1249,6 +1596,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) => { @@ -1275,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", @@ -1338,6 +1687,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, @@ -1467,12 +1817,14 @@ describe("operator question overlay", () => { try { wireGates(emitter, shell); emitter.emit("permission.gate", { + id: "req-1", request: baseRequest(), resolve: (o: unknown) => { approved = o; }, }); 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..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,22 +61,26 @@ 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 => `${askId}:${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 } : {}), @@ -83,20 +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 }; - } + 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 { @@ -148,40 +157,35 @@ 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). + * Unknown or omitted id → cancel. 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; - } - } - if (index < 0 || index >= options.length) { + if (selection.id === undefined) { return { kind: "cancel" }; } - return { kind: "option", index }; + const byId = choices.itemIds.indexOf(selection.id); + if (byId >= 0) { + return { kind: "option", index: byId }; + } + return { kind: "cancel" }; } export function operatorCancelResult(): OperatorResult { @@ -294,7 +298,11 @@ export function wireGates( function onPermission(ev: PermissionGateEvent): void { hooks.onGateOpened(); const resolve = onceClosed(hooks.onGateClosed, ev.resolve); - const choices = permissionChoicesFromRequest(ev.request); + if (typeof ev.id !== "string" || ev.id.length === 0) { + resolve({ allow: false, message: APPROVAL_UNAVAILABLE_MESSAGE }); + return; + } + 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. @@ -370,10 +378,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 +426,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 +474,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..31e786161 100644 --- a/src/tui/mention-popup.test.ts +++ b/src/tui/mention-popup.test.ts @@ -253,6 +253,7 @@ describe("@ popup narrows as you type", () => { let resolved: unknown; emitter.emit("permission.gate", { + id: "req-1", request: { tool: "run_shell", action: "Run shell command", @@ -305,6 +306,7 @@ 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", diff --git a/src/tui/overlay-overflow.test.ts b/src/tui/overlay-overflow.test.ts index 79d8e8985..ecf164a99 100644 --- a/src/tui/overlay-overflow.test.ts +++ b/src/tui/overlay-overflow.test.ts @@ -227,13 +227,14 @@ describe("gate-wire approval overflow on short terminal", () => { 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!; @@ -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!; @@ -312,7 +314,7 @@ describe("gate-wire approval overflow on short terminal", () => { 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/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..503414123 100644 --- a/src/tui/overlays.test.ts +++ b/src/tui/overlays.test.ts @@ -321,6 +321,153 @@ describe("overlay accept callbacks", () => { ); }); + 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, { + 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([ + { + 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(); + } + }, + { width: 80, height: 24 }, + ); + }); + + 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..0d8664a8d 100644 --- a/src/tui/product-host.test.ts +++ b/src/tui/product-host.test.ts @@ -193,6 +193,7 @@ describe("mountProductHost", () => { scopes: [], }; emitter.emit("permission.gate", { + id: "req-1", request, resolve: (outcome: unknown) => { resolved = outcome; @@ -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..8185a475f 100644 --- a/src/tui/request-approval.test.ts +++ b/src/tui/request-approval.test.ts @@ -99,6 +99,24 @@ describe("createGateRequestApproval", () => { captured?.resolve({ allow: true }); expect((await pending).allow).toBe(true); }); + + test("mints a unique event id without reading PermissionRequest", async () => { + const ids: string[] = []; + const requestApproval = createGateRequestApproval({ + emitGate: (event) => { + ids.push(event.id); + event.resolve({ allow: true }); + return true; + }, + approvalTimeout: noTimeout, + }); + 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]); + }); }); // attachApprovalBudget is the mechanism createGateRequestApproval builds on diff --git a/src/tui/request-approval.ts b/src/tui/request-approval.ts index 155da2992..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. */ @@ -71,6 +72,7 @@ export function createGateRequestApproval(args: CreateGateRequestApprovalArgs): }); const timeout = args.approvalTimeout(); const event: PermissionGateEvent = { + id: randomUUID(), request, resolve: finish, ...(timeout !== undefined ? timeout : {}), @@ -82,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/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..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]; @@ -774,9 +790,29 @@ export function acceptOverlaySelection(shell: AppShell): void { return; } - const id = bag?.primaryBindings.itemIds[idx]; - // Type-to-filter plants "(no matches)" with an empty-id sentinel. Stay open. - if (id === "") return; + const painted = shell.overlayList.select.getSelectedOption()?.value; + const itemIds = bag?.primaryBindings.itemIds ?? []; + 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. 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 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 39d8502ad..320134ccb 100644 --- a/src/tui/slash-popup-gate.test.ts +++ b/src/tui/slash-popup-gate.test.ts @@ -95,6 +95,7 @@ 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",