Skip to content

Commit e8e42c7

Browse files
Haiderclaude
andcommitted
fix(tui): pass workspace on yolo auto-approve and stop duplicate replies
Two defects introduced by the previous review round, found by CodeRabbit. The workspace was never sent on the flush path. `set()` took an optional workspace argument, but the app.tsx caller did not pass one, so replies from flushPendingPermissions went out with `workspace: undefined` while the session permission UI sends `project.workspace.current()` on every reply. Fixed at the source instead of at the call site — sync already has the project context, and threading the value through each caller is how it went missing in the first place. Replies could be sent twice for one request. flushPendingPermissions does not remove requests from the store; removal waits for the server's `permission.replied` event. Toggling yolo off and on again, or two set() calls, therefore replied to an already-settled id. The server rejects the second reply and autoApprove treated that rejection as a lost reply, putting a prompt back on screen for a request that was already answered. Now guarded by an in-flight set for concurrent duplicates plus optimistic removal on success for sequential ones — the event handler's removal stays idempotent. Adds a regression test for the double-enable path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01964Prd1Sz5JwWZmNTrFdiU
1 parent c5b7001 commit e8e42c7

2 files changed

Lines changed: 64 additions & 1 deletion

File tree

packages/tui/src/context/sync.tsx

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -282,26 +282,63 @@ export const {
282282
)
283283
}
284284

285+
// Remove a settled permission request from the store. Idempotent — both the
286+
// `permission.replied` event and a successful auto-approve call it.
287+
function removePermission(sessionID: string, requestID: string) {
288+
const requests = store.permission[sessionID]
289+
if (!requests?.length) return
290+
const match = search(requests, requestID, (r) => r.id)
291+
if (!match.found) return
292+
setStore(
293+
"permission",
294+
sessionID,
295+
produce((draft) => {
296+
draft.splice(match.index, 1)
297+
}),
298+
)
299+
}
300+
301+
// Requests currently being auto-approved. A reply is not removed from
302+
// store.permission until the server's `permission.replied` event lands, so without
303+
// this a second flush (toggle off then on again, or two set() calls) would reply to
304+
// the same request twice. The second reply targets an already-settled request, the
305+
// server rejects it, and the rejection would be misread as a lost reply — putting a
306+
// prompt back on screen for something already answered.
307+
const autoApproving = new Set<string>()
308+
285309
// Auto-approve on behalf of the user. MUST fail loudly: the handler does not enqueue
286310
// the request, so if the reply is lost the server-side Deferred in Permission.ask
287311
// never settles and the agent hangs with nothing on screen explaining why.
288312
//
289313
// `throwOnError: true` is required — the generated SDK client defaults to returning
290314
// `{ error }` rather than throwing (packages/sdk/js/src/gen/client/client.gen.ts), so
291315
// a plain `.catch()` here would never fire on an ordinary HTTP failure.
316+
//
317+
// The workspace is read here rather than taken from callers: the session permission
318+
// UI passes `project.workspace.current()` on every reply, and threading it through
319+
// each call site is exactly how it went missing on the flush path.
292320
async function autoApprove(request: PermissionRequest, workspace?: string) {
321+
if (autoApproving.has(request.id)) return
322+
autoApproving.add(request.id)
293323
try {
294324
await sdk.client.permission.reply(
295-
{ requestID: request.id, reply: "once", workspace },
325+
{ requestID: request.id, reply: "once", workspace: workspace ?? project.workspace.current() },
296326
{ throwOnError: true },
297327
)
328+
// Drop it now rather than waiting for the server's `permission.replied` event.
329+
// The in-flight set above only covers concurrent duplicates; a later flush (toggle
330+
// off, then on again) would still find the request sitting in the store and reply
331+
// to an already-settled id. The event handler's removal is idempotent.
332+
removePermission(request.sessionID, request.id)
298333
} catch (e) {
299334
console.error("yolo mode auto-approve failed", {
300335
error: e instanceof Error ? e.message : String(e),
301336
requestID: request.id,
302337
})
303338
// Fall back to asking the user rather than silently swallowing the request.
304339
enqueuePermission(request)
340+
} finally {
341+
autoApproving.delete(request.id)
305342
}
306343
}
307344

packages/tui/test/cli/cmd/tui/yolo-sync.test.tsx

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -220,6 +220,32 @@ describe("tui sync: yolo auto-approve", () => {
220220
}
221221
})
222222

223+
// Regression: flushPendingPermissions does not remove requests from the store — that
224+
// waits for the server's permission.replied event. Toggling off and on again, or two
225+
// set() calls, would otherwise reply to the same request twice; the server rejects the
226+
// second, autoApprove misreads the rejection as a lost reply, and a prompt reappears
227+
// for a request that was already answered.
228+
test("enabling twice does not reply to the same request twice", async () => {
229+
const { app, emit, sync, replies, tmp } = await setup()
230+
try {
231+
emit(askEvent(ROOT, "perm_1"))
232+
await wait(() => pending(sync, ROOT).length === 1)
233+
234+
sync.yolo.set(ROOT, true)
235+
await wait(() => replies.length === 1)
236+
// Second enable while the first reply is still settling.
237+
sync.yolo.set(ROOT, true)
238+
await Bun.sleep(300)
239+
240+
expect(replies).toHaveLength(1)
241+
// And no phantom prompt got re-inserted.
242+
expect(pending(sync, ROOT)).toHaveLength(0)
243+
} finally {
244+
app.renderer.destroy()
245+
await tmp[Symbol.asyncDispose]()
246+
}
247+
})
248+
223249
test("deleting a session drops its yolo override", async () => {
224250
const { app, emit, sync, tmp } = await setup()
225251
try {

0 commit comments

Comments
 (0)