Skip to content

Commit b26dce6

Browse files
committed
Verify overlay identity by generation instead of a flag
The prior settle path tracked "is my overlay open" with a boolean the caller had to remember to clear at each settle site, which depended on knowing that the shell closes an accepted or cancelled overlay (and may open the next queued gate) before invoking that callback. A generation counter bumped at every open on the shared host makes that comparison explicit instead of assumed: a settle path checks whether its own captured generation still matches the current one before closing anything, so it can never tear down an overlay opened after its own. Closing an overlay from inside a settle path (autoDeny, or a grant-driven reconcile) re-invokes that overlay's own onCancel, since shell.ts's closeInsetOverlay calls onCancel after notifying close listeners. Recording the decision unconditionally at each call site meant that reentrant invocation wrote a second transcript row for the same request. permissionQueue.settle already reports whether an id was still live to settle, so gating the transcript write on that return value makes the reentrant call a no-op instead of a duplicate.
1 parent cf41cb8 commit b26dce6

1 file changed

Lines changed: 33 additions & 21 deletions

File tree

src/tui-opentui/gate-wire.ts

Lines changed: 33 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -305,13 +305,29 @@ export function wireGates(
305305
emitter,
306306
permissionQueue,
307307
)
308+
// Bumped every time any gate (permission or operator) opens on the shared
309+
// host. A settle path that only knows "my overlay was opened" cannot tell
310+
// whether the host has since moved on to a newer one — the shell closes an
311+
// accepted/cancelled overlay and may open the next queued gate before that
312+
// gate's own settle callback runs — and closing blind would tear down that
313+
// newer overlay instead of its own. Comparing the generation captured at
314+
// open-time against the current one answers that directly, so correctness
315+
// never rests on remembering shell.ts's close-before-callback ordering at
316+
// each call site. openHost is the only place an overlay opens, so it is
317+
// the only place this counter needs to change.
318+
let overlayGeneration = 0
319+
320+
function openHost(open: () => void): void {
321+
overlayGeneration++
322+
open()
323+
}
308324

309325
function openOrQueue(open: () => void): void {
310326
if (shell.overlayList !== null) {
311327
pending.push(open)
312328
return
313329
}
314-
open()
330+
openHost(open)
315331
}
316332

317333
function unqueue(open: () => void): void {
@@ -321,7 +337,7 @@ export function wireGates(
321337

322338
const disposeClosed = onOverlayClosed(shell, () => {
323339
const next = pending.shift()
324-
if (next) next()
340+
if (next) openHost(next)
325341
})
326342

327343
function onPermission(ev: PermissionGateEvent): void {
@@ -334,27 +350,30 @@ export function wireGates(
334350
const collapsedAnything =
335351
formatCommandForApproval(ev.request.subject).payloadCount > 0
336352
let expanded = false
337-
let isOpen = false
353+
// Set only while this gate's own overlay is the one on screen — see
354+
// overlayGeneration above for why the settle path checks it against the
355+
// current generation instead of trusting this alone.
356+
let openedGeneration: number | undefined
338357

339358
// The queue is the single settle guard: once an id is removed (accept,
340359
// cancel, timeout, abort, or a reconciled grant), a later call is a
341360
// no-op instead of double-resolving. Its resolve callback settles
342361
// through the onceClosed-wrapped `resolve` (not ev.resolve directly) so
343362
// hooks.onGateClosed still fires exactly once regardless of which path
344-
// drained this entry. settle's own return value tells a call site
345-
// whether it was the one that actually settled, which is also how
346-
// recordDecision below is guarded against firing twice — closing the
347-
// overlay from inside this callback re-invokes the overlay's own
348-
// onCancel (see shell.ts's closeInsetOverlay), and that reentrant call
349-
// must find the id already gone.
363+
// drained this entry. Closing the overlay from inside this callback
364+
// re-invokes the overlay's own onCancel (see shell.ts's
365+
// closeInsetOverlay, which fires onCancel after notifying close
366+
// listeners) — settle's return value is how a call site tells that
367+
// reentrant call apart from the original one, so recordDecision below
368+
// fires exactly once per gate instead of once per reentry.
350369
const settle = (outcome: ApprovalOutcome): boolean =>
351370
permissionQueue.settle(id, outcome)
352371
const id = permissionQueue.enqueue(ev.request, (outcome) => {
353372
clearTimers()
354-
if (isOpen) {
355-
closeInsetOverlay(shell)
356-
} else {
373+
if (openedGeneration === undefined) {
357374
unqueue(open)
375+
} else if (openedGeneration === overlayGeneration) {
376+
closeInsetOverlay(shell)
358377
}
359378
resolve(outcome)
360379
})
@@ -378,7 +397,7 @@ export function wireGates(
378397
}
379398

380399
const open = (): void => {
381-
isOpen = true
400+
openedGeneration = overlayGeneration
382401
openPermissionsOverlay(shell, {
383402
items: choices.items,
384403
itemIds: choices.itemIds,
@@ -389,10 +408,6 @@ export function wireGates(
389408
echoChoice: false,
390409
...(collapsedAnything ? { onToggleExpand } : {}),
391410
onAccept: (sel: OverlaySelection) => {
392-
// The shell already closed this overlay (and may have opened the
393-
// next queued one) before invoking onAccept — settle must not
394-
// closeInsetOverlay a second time and tear down that next overlay.
395-
isOpen = false
396411
const gateSelection = {
397412
index: sel.index,
398413
...(sel.id !== undefined ? { id: sel.id } : {}),
@@ -402,11 +417,8 @@ export function wireGates(
402417
}
403418
},
404419
// Esc must settle the awaited promise (as a deny), not abandon it —
405-
// an unresolved gate hangs the run until the process is killed. The
406-
// shell has already closed the overlay by the time onCancel runs, for
407-
// the same reason noted in onAccept above.
420+
// an unresolved gate hangs the run until the process is killed.
408421
onCancel: () => {
409-
isOpen = false
410422
const gateSelection = { index: 0, id: PERMISSION_DENY_ID }
411423
if (settle(approvalOutcomeFromSelection(choices, gateSelection))) {
412424
recordDecision(shell, ev.request, choices, gateSelection)

0 commit comments

Comments
 (0)