Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 47 additions & 3 deletions assistant/src/__tests__/access-request-decision.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, unknown> }> = [];
let deliverReplyError: Error | null = null;
mock.module('../runtime/gateway-client.js', () => ({
deliverChannelReply: async (url: string, payload: Record<string, unknown>) => {
if (deliverReplyError) {
throw deliverReplyError;
}
deliverReplyCalls.push({ url, payload });
},
}));
Expand All @@ -64,6 +68,7 @@ import {
deliverVerificationCodeToGuardian,
notifyRequesterOfApproval,
notifyRequesterOfDenial,
notifyRequesterOfDeliveryFailure,
} from '../runtime/routes/access-request-decision.js';

initializeDb();
Expand Down Expand Up @@ -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',
Expand All @@ -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');
Expand All @@ -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',
Expand Down Expand Up @@ -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');
});
});
37 changes: 36 additions & 1 deletion assistant/src/runtime/routes/access-request-decision.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -108,7 +112,7 @@ export async function deliverVerificationCodeToGuardian(params: {
verificationCode: string;
assistantId: string;
bearerToken?: string;
}): Promise<void> {
}): Promise<DeliveryResult> {
const text = `You approved access for ${params.requesterIdentifier}. `
+ `Give them this verification code: ${params.verificationCode}. `
+ `The code expires in 10 minutes.`;
Expand All @@ -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 };
}
}

Expand Down Expand Up @@ -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<void> {
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.
*/
Expand Down
39 changes: 29 additions & 10 deletions assistant/src/runtime/routes/guardian-approval-interception.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ import {
deliverVerificationCodeToGuardian,
notifyRequesterOfApproval,
notifyRequesterOfDenial,
notifyRequesterOfDeliveryFailure,
type DeliveryResult,
} from './access-request-decision.js';
import {
buildGuardianDenyContext,
Expand Down Expand Up @@ -998,23 +1000,41 @@ 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,
verificationCode: decisionResult.verificationCode,
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;
}
Comment on lines +1013 to +1019
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Orphaned verification session left active when guardian code delivery fails

When deliverVerificationCodeToGuardian fails, the approval has already been atomically resolved to 'approved' and a verification session has already been created (both happen inside handleAccessRequestDecision at access-request-decision.ts:58-101 before the delivery attempt). The session remains in awaiting_response status with a valid secret, but the code was never delivered to the guardian.

Root Cause and Impact

The sequence in handleAccessRequestApproval is:

  1. handleAccessRequestDecision() → resolves approval to 'approved' AND creates verification session (access-request-decision.ts:62 and access-request-decision.ts:86-94)
  2. deliverVerificationCodeToGuardian() → fails (guardian-approval-interception.ts:1005-1012)
  3. codeDelivered = false → requester is told "try again later" (guardian-approval-interception.ts:1029-1036)

But there is no cleanup of the orphaned verification session. The session sits in awaiting_response status for 10 minutes with a code nobody knows. More critically, the approval is already consumed (status: 'approved'), so if the requester "tries again later" by sending a new message, findPendingAccessRequestForRequester won't find a pending approval — the flow cannot be retried.

The requester is permanently stuck: the approval is consumed, the code was never delivered, and there's no retry path. The session will eventually expire, but the user experience is a dead end.

The fix should either (a) revoke/expire the verification session when delivery fails (e.g., updateSessionStatus(decisionResult.verificationSessionId, 'revoked')) so it doesn't linger, or (b) roll back the approval status so it can be retried, or (c) both.

Prompt for agents
In assistant/src/runtime/routes/guardian-approval-interception.ts, inside the handleAccessRequestApproval function, when deliveryResult.ok is false (lines 1013-1019), the orphaned verification session created by handleAccessRequestDecision is left in 'awaiting_response' status. Import updateSessionStatus from '../channel-guardian-service.js' and add a call to revoke the session:

    if (!deliveryResult.ok) {
      log.error(...);
      codeDelivered = false;
      // Revoke the orphaned session so it doesn't linger
      if (decisionResult.verificationSessionId) {
        updateSessionStatus(decisionResult.verificationSessionId, 'revoked');
      }
    }

This prevents the orphaned session from sitting in awaiting_response for 10 minutes. You should also consider whether the approval itself should be rolled back to 'pending' so the guardian can retry, since the current 'approved' status makes the flow non-retryable.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

}

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({
Expand All @@ -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,
Expand Down