From 68aa87d42e624a46f55bd08d6560463cf4c317a9 Mon Sep 17 00:00:00 2001 From: "clagentic-builder[bot]" Date: Mon, 7 Sep 2026 09:40:20 -0400 Subject: [PATCH] fix(sdk-message-processor): preserve activeTaskToolIds for a pending sub-agent permission on task_notification (lr-b75006) Third instance of a class fixed twice before (lr-8b2e, lr-f969dc, lr-9d4b). Incomplete application of lr-9d4b own fix: task_notification deleted session.activeTaskToolIds[parentId] unconditionally while a sibling guard protects subagentToolOwners against the identical race (sub-agent nested permission still pending). partitionSubagentOwnedPermissions requires BOTH maps to agree a Task is live; with only one guarded, the pending permission was force-denied and purged by sweepClearedPermissionIndex while the browser card stayed rendered -- every click (Allow/Allow-for-Session/Deny) then found nothing to resolve, matching the reported symptom exactly. Fix: compute the same live-ownership snapshot already used to guard subagentToolOwners, and use it to also defer releasing activeTaskToolIds[parentId] and its activity token while a live permission still references the Task. Deferral is safe: tool_result release, per-turn generation bump, and idle-sweep backstop all still apply once the permission resolves. Regression test drives the real task_notification then result sequence end to end at the message-processor layer. Reproduced failing against pre-fix code (assertion on activeTaskToolIds survival failed as predicted), confirmed passing after the fix, full suite green 1584/1584. TASK: lr-b75006 --- lib/sdk-message-processor.js | 49 +++- ...oolids-notification-race-lr-b75006.test.js | 262 ++++++++++++++++++ 2 files changed, 304 insertions(+), 7 deletions(-) create mode 100644 test/sdk-message-processor-activetasktoolids-notification-race-lr-b75006.test.js diff --git a/lib/sdk-message-processor.js b/lib/sdk-message-processor.js index 7ddc6f71..704bd270 100644 --- a/lib/sdk-message-processor.js +++ b/lib/sdk-message-processor.js @@ -932,6 +932,29 @@ function attachMessageProcessor(ctx) { } else if (parsed.yokeType === "task_notification") { var parentId = parsed.parentToolId; + // lr-b75006: compute this BEFORE any cleanup below so both the + // activeTaskToolIds release and the subagentToolOwners prune (further + // down) can consult the SAME live-ownership snapshot. Previously only + // subagentToolOwners was guarded against the race described below; + // activeTaskToolIds was released unconditionally a few lines earlier, + // which silently reintroduced the identical hazard through the other + // half of partitionSubagentOwnedPermissions's check (lib/sdk-permission- + // ownership.js: `owningTaskId && activeTaskToolIds[owningTaskId]`) -- + // see the exception note below for the full mechanism. + var liveOwnedToolIds = {}; + for (var lpk in (session.pendingPermissions || {})) { + var lpEntry = session.pendingPermissions[lpk]; + if (lpEntry && lpEntry.toolUseId) liveOwnedToolIds[lpEntry.toolUseId] = true; + } + var parentOwnsLivePermission = false; + if (parentId && session.subagentToolOwners) { + for (var polk in session.subagentToolOwners) { + if (session.subagentToolOwners[polk] === parentId && liveOwnedToolIds[polk]) { + parentOwnsLivePermission = true; + break; + } + } + } if (parentId) { sendAndRecord(session, { type: "subagent_done", @@ -945,8 +968,25 @@ function attachMessageProcessor(ctx) { // delivered task_notification without a corresponding tool_result // block, or delivered it first). releaseActivity is idempotent — // if tool_result already released this same token, this is a no-op. - if (session.activeTaskToolIds) delete session.activeTaskToolIds[parentId]; - releaseActivity(session, parentId); + // + // lr-b75006 EXCEPTION: do not release activeTaskToolIds[parentId] + // while the sub-agent's own permission request (e.g. its last Bash + // call) is still awaiting the operator's click. A later 'result' (or + // processQueryStream's finally) decides whether to preserve that + // pending permission via partitionSubagentOwnedPermissions, which + // requires activeTaskToolIds[owningTaskId] to still be truthy. If + // this fired unconditionally here, the permission would be force- + // denied and purged by that later cleanup while the UI card is still + // rendered and clickable -- every subsequent Allow/Allow-for-Session/ + // Deny click then finds no pendingPermissions entry to resolve + // (project-sessions.js's permission_response handler no-ops on a + // missing `pending`), reproducing the exact "clicks don't stick" + // symptom lr-9d4b's sibling subagentToolOwners guard (below) was + // already written to prevent -- just via activeTaskToolIds instead. + if (!parentOwnsLivePermission) { + if (session.activeTaskToolIds) delete session.activeTaskToolIds[parentId]; + releaseActivity(session, parentId); + } } if (session.taskIdMap) { for (var k in session.taskIdMap) { @@ -973,11 +1013,6 @@ function attachMessageProcessor(ctx) { // not by Task, and simply becomes inert once no pendingPermissions // entry references it). if (parentId && session.subagentToolOwners) { - var liveOwnedToolIds = {}; - for (var lpk in (session.pendingPermissions || {})) { - var lpEntry = session.pendingPermissions[lpk]; - if (lpEntry && lpEntry.toolUseId) liveOwnedToolIds[lpEntry.toolUseId] = true; - } for (var stk in session.subagentToolOwners) { if (session.subagentToolOwners[stk] === parentId && !liveOwnedToolIds[stk]) { delete session.subagentToolOwners[stk]; diff --git a/test/sdk-message-processor-activetasktoolids-notification-race-lr-b75006.test.js b/test/sdk-message-processor-activetasktoolids-notification-race-lr-b75006.test.js new file mode 100644 index 00000000..523886b7 --- /dev/null +++ b/test/sdk-message-processor-activetasktoolids-notification-race-lr-b75006.test.js @@ -0,0 +1,262 @@ +/** + * Regression test for lr-b75006: a backgrounded sub-agent's still-pending + * permission is force-denied and purged when the PARENT Task's + * `task_notification` (completion) arrives before the operator answers, and + * a later turn-boundary cleanup (`result` / processQueryStream's finally) + * runs afterward. + * + * THIRD INSTANCE OF THIS CLASS (lr-8b2e, lr-f969dc, lr-9d4b). This is an + * incomplete application of lr-9d4b's own fix, not a fresh independent bug: + * lr-9d4b added an explicit exception so `task_notification` would NOT prune + * `session.subagentToolOwners` for a tool id that still has a live + * `pendingPermissions` entry (see the existing BOBBIE test in + * sdk-message-processor-subagent-permission-lr-9d4b.test.js). But + * `task_notification` ALSO deletes `session.activeTaskToolIds[parentId]` + * unconditionally, a few lines before that guard, and nothing mirrored the + * same exception onto it. + * + * `lib/sdk-permission-ownership.js`'s `partitionSubagentOwnedPermissions` -- + * the function both `result` and `processQueryStream`'s `finally` rely on to + * decide whether to preserve a sub-agent-owned pending permission across + * turn-boundary cleanup -- requires BOTH halves of + * `owningTaskId && activeTaskToolIds[owningTaskId]` to be true. Guarding only + * `subagentToolOwners` (which supplies `owningTaskId`) while leaving + * `activeTaskToolIds` unconditionally cleared meant the second half of that + * check was false by the time `result` ran, so the entry was NOT preserved: + * it was force-resolved to `deny` and purged from both + * `session.pendingPermissions` and `sm.permissionRequestIndex` by + * `sweepClearedPermissionIndex`, while the browser's permission card was + * never told to update and stayed rendered as if still awaiting an answer. + * + * Every subsequent click (Allow Once / Allow for Session / Deny) on that + * stale card then hits `project-sessions.js`'s + * `if (!pending) return true;` no-op -- explaining why NONE of the three + * buttons "stuck": the underlying resolver was already gone by the time any + * of them were clicked. + * + * This test drives the exact sequence end-to-end through the real message + * processor (task_notification, THEN a later result) and asserts the + * permission survives both -- the layer this bug's absence of coverage let + * slip through twice already. It fails on pre-fix code (task_notification's + * unconditional activeTaskToolIds delete) and passes once + * activeTaskToolIds[parentId] is deferred using the same live-ownership + * check as subagentToolOwners. + */ + +var test = require("node:test"); +var assert = require("node:assert/strict"); + +var { attachMessageProcessor } = require("../lib/sdk-message-processor"); + +function makeSm() { + return { + skillMeta: [], + workflowMeta: [], + skillNames: [], + slashCommands: null, + currentModel: null, + _savedDefaultModel: null, + sendAndRecord: function (session, obj) { + if (!session.history) session.history = []; + session.history.push(obj); + }, + sendToSession: function () {}, + saveSessionFile: function () {}, + broadcastSessionList: function () {}, + modelsByVendor: {}, + availableModels: [], + availableVendors: [], + installedVendors: [], + }; +} + +function makeProcessor(sm) { + return attachMessageProcessor({ + sm: sm, + send: function () {}, + slug: "test-slug", + cwd: "/tmp", + pushModule: null, + getNotificationsModule: function () { return null; }, + adapter: { vendor: "claude" }, + onProcessingChanged: function () {}, + onTurnDone: null, + onAutoTitle: null, + opts: {}, + discoverSkillDirs: function () { return []; }, + mergeSkills: function () { return []; }, + discoverWorkflows: function () { return []; }, + discoverSkillsWithMeta: function () { return []; }, + mergeSkillsWithMeta: function () { return []; }, + getSDK: null, + }); +} + +function makeSession() { + return { + localId: "s1", + cliSessionId: null, + vendor: "claude", + history: [], + messageUUIDs: [], + blocks: {}, + sentToolResults: {}, + pendingPermissions: {}, + pendingElicitations: {}, + pendingAskUser: {}, + activeTaskToolIds: {}, + taskIdMap: {}, + streamedText: false, + responsePreview: "", + isProcessing: true, + loop: null, + }; +} + +test("lr-b75006: a sub-agent permission still pending survives BOTH task_notification AND a later result, end to end", function () { + var sm = makeSm(); + var processor = makeProcessor(sm); + var session = makeSession(); + + var taskToolId = "toolu_task_b75006"; + var bashToolId = "toolu_sub_bash_b75006"; + + // Parent turn dispatches a backgrounded Task sub-agent. + processor.processSDKMessage(session, { + yokeType: "tool_start", + blockId: 0, + toolId: taskToolId, + toolName: "Task", + }); + processor.processSDKMessage(session, { + yokeType: "block_stop", + blockId: 0, + }); + assert.ok(session.activeTaskToolIds[taskToolId], "Task must be tracked active after block_stop"); + + // The sub-agent issues its own Bash tool_use (e.g. `lore task comment ...`). + processor.processSDKMessage(session, { + yokeType: "subagent_message", + parentToolUseId: taskToolId, + messageRole: "assistant", + content: [ + { type: "tool_use", id: bashToolId, name: "Bash", input: { command: "lore task comment lr-6ab846 --comment ..." } }, + ], + }); + + // sdk-bridge.js's handleCanUseTool registers the pending permission for + // that Bash call -- this is the card the operator sees and clicks. + var resolvedWith = null; + session.pendingPermissions["perm-b75006"] = { + resolve: function (result) { resolvedWith = result; }, + requestId: "perm-b75006", + toolName: "Bash", + toolInput: { command: "lore task comment lr-6ab846 --comment ..." }, + toolUseId: bashToolId, + decisionReason: "", + }; + + // The sub-agent's Task reports completion (task_notification) WHILE its + // own nested permission is still awaiting the operator's click -- the + // exact race this task is about. + processor.processSDKMessage(session, { + yokeType: "task_notification", + parentToolId: taskToolId, + taskId: "task-id-b75006", + status: "completed", + }); + + // Both ownership records that partitionSubagentOwnedPermissions depends on + // must survive task_notification -- not just subagentToolOwners (the half + // lr-9d4b already guarded) but ALSO activeTaskToolIds (the half that was + // still being cleared unconditionally). + assert.equal( + session.subagentToolOwners[bashToolId], + taskToolId, + "subagentToolOwners must survive task_notification while the permission is pending" + ); + assert.ok( + session.activeTaskToolIds[taskToolId], + "activeTaskToolIds must ALSO survive task_notification while the permission is pending -- " + + "this is the half of the ownership check lr-9d4b's guard missed" + ); + assert.ok( + session.pendingPermissions["perm-b75006"], + "the pending permission itself must be untouched by task_notification" + ); + + // Some time later, the PARENT turn's SDK 'result' arrives (a normal event + // in crew-manifest orchestration -- the parent turn wraps up while its + // backgrounded sub-agent's last action is still awaiting operator review). + processor.processSDKMessage(session, { + yokeType: "result", + cost: 0.3, + duration: 1500, + sessionId: "cli-session-b75006", + }); + + // THE ACTUAL BUG: pre-fix, activeTaskToolIds[taskToolId] was already gone + // (deleted unconditionally by task_notification above), so + // partitionSubagentOwnedPermissions's `owningTaskId && activeTaskToolIds[owningTaskId]` + // check failed even though subagentToolOwners still pointed at a live + // Task id. The entry was dropped, force-resolved to deny by + // sweepClearedPermissionIndex, and purged -- while the browser's card + // stayed rendered. This assertion is what a passing pre-fix test run would + // have caught and did not. + assert.ok( + session.pendingPermissions["perm-b75006"], + "the sub-agent's permission must survive the LATER result cleanup too -- " + + "this is the reproduction of lr-b75006's 'clicks don't stick' symptom" + ); + assert.equal( + resolvedWith, + null, + "the resolver must not have been silently force-denied by turn-boundary cleanup" + ); + + // Now the operator's actual click (permission_response handler in + // project-sessions.js) can still find and resolve it. + var pending = session.pendingPermissions["perm-b75006"]; + assert.ok(pending, "permission_response handler must find a live entry to resolve"); + pending.resolve({ behavior: "allow", updatedInput: pending.toolInput }); + + assert.deepEqual( + resolvedWith, + { behavior: "allow", updatedInput: { command: "lore task comment lr-6ab846 --comment ..." } }, + "resolving the surviving entry must settle the original canUseTool Promise -- Allow actually sticks" + ); +}); + +test("lr-b75006: Deny still works normally for an ordinary (non-subagent) permission after a result cleanup", function () { + var sm = makeSm(); + var processor = makeProcessor(sm); + var session = makeSession(); + + // Sanity guard: this fix must not change behavior for the ordinary + // top-level case (no Task/sub-agent involved at all) -- a plain permission + // still pending at 'result' time is unusual but if it happens it must + // still be cleared exactly as before (lr-9d4b's existing top-level test + // covers this too; repeated narrowly here as a same-file cross-check + // against this fix's new liveOwnedToolIds computation). + session.pendingPermissions["perm-top-level-b75006"] = { + resolve: function () {}, + requestId: "perm-top-level-b75006", + toolName: "Bash", + toolInput: { command: "echo hi" }, + toolUseId: "toolu_top_level_b75006", + decisionReason: "", + }; + + processor.processSDKMessage(session, { + yokeType: "result", + cost: 0.1, + duration: 500, + sessionId: "cli-session-b75006-top-level", + }); + + assert.deepEqual( + session.pendingPermissions, + {}, + "a plain top-level permission with no sub-agent ownership must still be cleared normally on result" + ); +});