From 32f002565485a1b1c81400b2beba22baf959537f Mon Sep 17 00:00:00 2001 From: Saim Shafique Date: Fri, 30 Oct 2026 20:25:15 +0500 Subject: [PATCH] fix(approvals): let an allowed share cover the bot's reworded retry The private-share check matched a recent allow-once on the exact content digest, so when the Bot reworded its send after approval it opened a fresh request the person had already answered, looping until someone denied it. The match is now the share itself: same Bot, audience, conversation, within thirty minutes. Co-authored-by: Saim Shafique <128062316+sx4im@users.noreply.github.com> --- CHANGELOG.md | 8 +++ server/src/proactive/private-share.ts | 15 ++++-- server/tests/proactive.test.ts | 73 +++++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f9d3f16f4..8cea904176 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -78,6 +78,14 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. `COMPUTER_MEMORY_HIGH_PERCENT=0` and `COMPUTER_TAB_IDLE_MS=0` (Helm: `computers.memoryHighPercent` and `computers.tabIdleMs`) keep the old behaviour. +### An allowed share is not asked about again when the Bot rewords it + +Sharing private information asks you once per share, and "allow once" covers the Bot's next attempt +for thirty minutes. The check compared the exact bytes of what the Bot was about to send, so when +the Bot reworded its reply after your approval it asked you again — and again — for the same share. +The approval now covers the share itself (this Bot, this audience, this conversation), so the retry +goes through and a different audience still asks. + ### A computer closes browsers nobody is using when its memory runs high A computer runs one browser for each Bot that is browsing, and the only limits were a count and half diff --git a/server/src/proactive/private-share.ts b/server/src/proactive/private-share.ts index d583b4454e..b619415c24 100644 --- a/server/src/proactive/private-share.ts +++ b/server/src/proactive/private-share.ts @@ -9,9 +9,9 @@ * * WHERE PERMISSION LIVES. In the existing approvals ledger, always on, regardless of the owner's * general approvals switch: this is a check dots makes unconditionally. An "allow always" answer is - * a standing rule for this Bot and this audience; "allow once" covers the same content to the same - * audience in the same conversation for thirty minutes, so the Bot's retry after an approval passes - * instead of asking again. + * a standing rule for this Bot and this audience; "allow once" covers the same share — this Bot, + * this audience, this conversation — for thirty minutes, so the Bot's reworded retry after an + * approval passes instead of asking again. * * OUTCOMES. `allowed`; `denied` (the person said no); or `pending`, where the caller must not send * and either throws the returned suspension (headless turns wait and resume on the decision) or @@ -156,10 +156,15 @@ export function createPrivateShareCheck(deps: { }; const action = approvalAction(candidate); - // The retry after an "allow once": same content, same audience, same conversation, recently. + // The retry after an "allow once": the Bot rewords the send, so what the + // approval covers is the share itself — this Bot, this audience, this + // conversation — and not the identical bytes. Without this, every reworded + // retry opens a fresh request the person has already answered. const recent = (await deps.approvals.list(input.ownerUserId)).find( (record) => - record.action.actionDigest === action.actionDigest && + record.action.toolRef === action.toolRef && + record.action.botId === action.botId && + record.action.scope === action.scope && record.action.threadId === action.threadId && (record.decision === "allow_once" || record.decision === "allow_always") && diff --git a/server/tests/proactive.test.ts b/server/tests/proactive.test.ts index 76677b69fd..9b6aa4435c 100644 --- a/server/tests/proactive.test.ts +++ b/server/tests/proactive.test.ts @@ -739,4 +739,77 @@ describe("private information sharing check", () => { expect((await check(input)).status).toBe("allowed"); expect(opened).toHaveLength(0); }); + + test("an allow-once covers the bot's reworded retry to the same audience", async () => { + const gmail = { + kind: "external" as const, + id: "composio-gmail/GMAIL_REPLY_TO_THREAD", + label: "Gmail reply", + recipientUserIds: ["someone@example.com"], + }; + const decidedAt = new Date(); + const { store, opened } = approvals("pending"); + const check = createPrivateShareCheck({ + approvals: { + ...store, + list: async () => [ + { + action: { + toolRef: "openbot/share_private_information", + botId: "bot", + scope: "external:composio-gmail/GMAIL_REPLY_TO_THREAD", + threadId: "thread", + actionDigest: "digest-of-the-first-wording", + }, + decision: "allow_once", + decidedAt, + }, + ], + }, + }); + // The person allowed the share; the bot's retry words the reply differently. + // That must not open a second request for the same share. + const verdict = await check({ + ...input, + audience: gmail, + content: "Reworded reply with different bytes.", + }); + expect(verdict.status).toBe("allowed"); + expect(opened).toHaveLength(0); + }); + + test("an allow-once for another audience still asks again", async () => { + const decidedAt = new Date(); + const { store, opened } = approvals("pending"); + const check = createPrivateShareCheck({ + approvals: { + ...store, + list: async () => [ + { + action: { + toolRef: "openbot/share_private_information", + botId: "bot", + scope: "external:composio-gmail/GMAIL_REPLY_TO_THREAD", + threadId: "thread", + actionDigest: "digest-of-the-first-wording", + }, + decision: "allow_once", + decidedAt, + }, + ], + }, + }); + const verdict = await check({ + ...input, + audience: { + kind: "external" as const, + id: "composio-slack/SLACK_POST_MESSAGE", + label: "Slack post", + recipientUserIds: ["someone@example.com"], + }, + content: "Same words, different audience.", + }); + expect(verdict.status).toBe("pending"); + expect(opened).toHaveLength(1); + }); });