From 350c4ca9669bbef96b38f61a548f7d0b57b2d966 Mon Sep 17 00:00:00 2001 From: Harrison Ngo Date: Thu, 26 Feb 2026 14:52:09 -0500 Subject: [PATCH] fix: propagate guardian code delivery failures to prevent premature requester notification Co-Authored-By: Claude --- .../__tests__/access-request-decision.test.ts | 50 +++++++++++++++++-- .../runtime/routes/access-request-decision.ts | 37 +++++++++++++- .../routes/guardian-approval-interception.ts | 39 +++++++++++---- 3 files changed, 112 insertions(+), 14 deletions(-) diff --git a/assistant/src/__tests__/access-request-decision.test.ts b/assistant/src/__tests__/access-request-decision.test.ts index 09521863b4d..8f4d5fe6090 100644 --- a/assistant/src/__tests__/access-request-decision.test.ts +++ b/assistant/src/__tests__/access-request-decision.test.ts @@ -41,10 +41,14 @@ mock.module('../util/logger.js', () => ({ }), })); -// Track deliverChannelReply calls +// Track deliverChannelReply calls and allow injecting failures const deliverReplyCalls: Array<{ url: string; payload: Record }> = []; +let deliverReplyError: Error | null = null; mock.module('../runtime/gateway-client.js', () => ({ deliverChannelReply: async (url: string, payload: Record) => { + if (deliverReplyError) { + throw deliverReplyError; + } deliverReplyCalls.push({ url, payload }); }, })); @@ -64,6 +68,7 @@ import { deliverVerificationCodeToGuardian, notifyRequesterOfApproval, notifyRequesterOfDenial, + notifyRequesterOfDeliveryFailure, } from '../runtime/routes/access-request-decision.js'; initializeDb(); @@ -233,10 +238,11 @@ describe('access request decision handler', () => { describe('access request notification delivery', () => { beforeEach(() => { deliverReplyCalls.length = 0; + deliverReplyError = null; }); - test('delivers verification code to guardian', async () => { - await deliverVerificationCodeToGuardian({ + test('delivers verification code to guardian and returns ok', async () => { + const result = await deliverVerificationCodeToGuardian({ replyCallbackUrl: 'http://localhost:7830/deliver/telegram', guardianChatId: 'guardian-chat-789', requesterIdentifier: 'user-unknown-456', @@ -245,6 +251,7 @@ describe('access request notification delivery', () => { bearerToken: 'test-token', }); + expect(result.ok).toBe(true); expect(deliverReplyCalls.length).toBe(1); const call = deliverReplyCalls[0]; expect(call.payload.chatId).toBe('guardian-chat-789'); @@ -254,6 +261,26 @@ describe('access request notification delivery', () => { expect(text).toContain('10 minutes'); }); + test('returns failure result when guardian code delivery fails', async () => { + deliverReplyError = new Error('Gateway timeout'); + + const result = await deliverVerificationCodeToGuardian({ + replyCallbackUrl: 'http://localhost:7830/deliver/telegram', + guardianChatId: 'guardian-chat-789', + requesterIdentifier: 'user-unknown-456', + verificationCode: '123456', + assistantId: 'self', + bearerToken: 'test-token', + }); + + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.reason).toBe('Gateway timeout'); + } + // No calls should have been recorded (error thrown before push) + expect(deliverReplyCalls.length).toBe(0); + }); + test('notifies requester of approval', async () => { await notifyRequesterOfApproval({ replyCallbackUrl: 'http://localhost:7830/deliver/telegram', @@ -284,4 +311,21 @@ describe('access request notification delivery', () => { const text = call.payload.text as string; expect(text).toContain('denied'); }); + + test('notifies requester of delivery failure', async () => { + await notifyRequesterOfDeliveryFailure({ + replyCallbackUrl: 'http://localhost:7830/deliver/telegram', + requesterChatId: 'chat-123', + assistantId: 'self', + bearerToken: 'test-token', + }); + + expect(deliverReplyCalls.length).toBe(1); + const call = deliverReplyCalls[0]; + expect(call.payload.chatId).toBe('chat-123'); + const text = call.payload.text as string; + expect(text).toContain('approved'); + expect(text).toContain('unable to deliver'); + expect(text).toContain('try again'); + }); }); diff --git a/assistant/src/runtime/routes/access-request-decision.ts b/assistant/src/runtime/routes/access-request-decision.ts index 1480ca72290..2adc7826766 100644 --- a/assistant/src/runtime/routes/access-request-decision.ts +++ b/assistant/src/runtime/routes/access-request-decision.ts @@ -21,6 +21,10 @@ const log = getLogger('access-request-decision'); export type AccessRequestDecisionAction = 'approve' | 'deny'; +export type DeliveryResult = + | { ok: true } + | { ok: false; reason: string }; + export interface AccessRequestDecisionResult { handled: boolean; type: 'approved' | 'denied' | 'stale' | 'idempotent'; @@ -108,7 +112,7 @@ export async function deliverVerificationCodeToGuardian(params: { verificationCode: string; assistantId: string; bearerToken?: string; -}): Promise { +}): Promise { const text = `You approved access for ${params.requesterIdentifier}. ` + `Give them this verification code: ${params.verificationCode}. ` + `The code expires in 10 minutes.`; @@ -119,11 +123,14 @@ export async function deliverVerificationCodeToGuardian(params: { text, assistantId: params.assistantId, }, params.bearerToken); + return { ok: true }; } catch (err) { log.error( { err, guardianChatId: params.guardianChatId }, 'Failed to deliver verification code to guardian', ); + const reason = err instanceof Error ? err.message : String(err); + return { ok: false, reason }; } } @@ -154,6 +161,34 @@ export async function notifyRequesterOfApproval(params: { } } +/** + * Notify the requester that something went wrong delivering the verification + * code and they should try again later. Sent instead of the "enter the code" + * message when guardian code delivery fails. + */ +export async function notifyRequesterOfDeliveryFailure(params: { + replyCallbackUrl: string; + requesterChatId: string; + assistantId: string; + bearerToken?: string; +}): Promise { + const text = 'Your access request was approved, but we were unable to ' + + 'deliver the verification code. Please try again later.'; + + try { + await deliverChannelReply(params.replyCallbackUrl, { + chatId: params.requesterChatId, + text, + assistantId: params.assistantId, + }, params.bearerToken); + } catch (err) { + log.error( + { err, requesterChatId: params.requesterChatId }, + 'Failed to notify requester of delivery failure', + ); + } +} + /** * Notify the requester that the guardian has denied their access request. */ diff --git a/assistant/src/runtime/routes/guardian-approval-interception.ts b/assistant/src/runtime/routes/guardian-approval-interception.ts index 0ad7233b22d..588b717031e 100644 --- a/assistant/src/runtime/routes/guardian-approval-interception.ts +++ b/assistant/src/runtime/routes/guardian-approval-interception.ts @@ -35,6 +35,8 @@ import { deliverVerificationCodeToGuardian, notifyRequesterOfApproval, notifyRequesterOfDenial, + notifyRequesterOfDeliveryFailure, + type DeliveryResult, } from './access-request-decision.js'; import { buildGuardianDenyContext, @@ -998,8 +1000,9 @@ async function handleAccessRequestApproval( // Approved: deliver the verification code to the guardian and notify the requester. const requesterIdentifier = approval.requesterExternalUserId; + let codeDelivered = true; if (decisionResult.verificationCode) { - await deliverVerificationCodeToGuardian({ + const deliveryResult: DeliveryResult = await deliverVerificationCodeToGuardian({ replyCallbackUrl, guardianChatId: approval.guardianChatId, requesterIdentifier, @@ -1007,14 +1010,31 @@ async function handleAccessRequestApproval( assistantId, bearerToken, }); + if (!deliveryResult.ok) { + log.error( + { reason: deliveryResult.reason, approvalId: approval.id }, + 'Skipping requester notification — verification code was not delivered to guardian', + ); + codeDelivered = false; + } } - await notifyRequesterOfApproval({ - replyCallbackUrl, - requesterChatId: approval.requesterChatId, - assistantId, - bearerToken, - }); + if (codeDelivered) { + await notifyRequesterOfApproval({ + replyCallbackUrl, + requesterChatId: approval.requesterChatId, + assistantId, + bearerToken, + }); + } else { + // Let the requester know something went wrong without revealing details + await notifyRequesterOfDeliveryFailure({ + replyCallbackUrl, + requesterChatId: approval.requesterChatId, + assistantId, + bearerToken, + }); + } // Emit guardian_decision (approved) signal void emitNotificationSignal({ @@ -1038,9 +1058,8 @@ async function handleAccessRequestApproval( dedupeKey: `trusted-contact:guardian-decision:${approval.id}`, }); - // Emit verification_sent signal — the code has been created and - // delivered to the guardian for out-of-band relay to the requester. - if (decisionResult.verificationSessionId) { + // Only emit verification_sent when the code was actually delivered to the guardian. + if (decisionResult.verificationSessionId && codeDelivered) { void emitNotificationSignal({ sourceEventName: 'ingress.trusted_contact.verification_sent', sourceChannel: approval.channel,