From 7132ba90c437f0e662d73e9d6a8cf7c8d58cd8cd Mon Sep 17 00:00:00 2001 From: Noa Flaherty Date: Sun, 1 Mar 2026 09:29:47 -0500 Subject: [PATCH 1/2] =?UTF-8?q?fix:=20harden=20canonical=20identity=20bind?= =?UTF-8?q?ing=20=E2=80=94=20close=20remaining=20post-merge=20gaps=20from?= =?UTF-8?q?=20#11006?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix local IPC fallback context field names (conversationExternalId, actorExternalId) - Make voice guardian dispatch binding-self-healing via ensureVellumGuardianBinding - Add access_request to DECISIONABLE_KINDS with self-healing and migration update - Rename residual isTrusted symbol to trustedAudience in call-pointer-messages - Update all test fixtures to supply guardianPrincipalId for decisionable kinds Co-Authored-By: Claude Opus 4.6 --- .../src/__tests__/actor-token-service.test.ts | 7 +- .../canonical-guardian-store.test.ts | 54 +++++++++-- .../__tests__/channel-approval-routes.test.ts | 2 + ...nfirmation-request-guardian-bridge.test.ts | 1 + .../guardian-principal-id-roundtrip.test.ts | 14 ++- .../non-member-access-request.test.ts | 97 ++++++++++--------- .../src/__tests__/send-endpoint-busy.test.ts | 6 ++ assistant/src/calls/call-pointer-messages.ts | 6 +- assistant/src/calls/guardian-dispatch.ts | 25 ++++- .../src/memory/canonical-guardian-store.ts | 1 + .../126-backfill-guardian-principal-id.ts | 24 ++--- .../src/runtime/access-request-helper.ts | 21 +++- assistant/src/runtime/local-actor-identity.ts | 4 +- 13 files changed, 173 insertions(+), 89 deletions(-) diff --git a/assistant/src/__tests__/actor-token-service.test.ts b/assistant/src/__tests__/actor-token-service.test.ts index 4cfce2c2212..d7449db5d6c 100644 --- a/assistant/src/__tests__/actor-token-service.test.ts +++ b/assistant/src/__tests__/actor-token-service.test.ts @@ -652,11 +652,14 @@ describe('resolveLocalIpcGuardianContext', () => { expect(ctx.sourceChannel).toBe('vellum'); }); - test('returns fallback guardian context when no vellum binding exists', () => { - // No binding created — fresh DB state + test('returns guardian context with principal when no vellum binding exists (pre-bootstrap self-heal)', () => { + // No binding created — fresh DB state. Pre-bootstrap path self-heals + // by creating a vellum binding, then resolves through the shared pipeline + // with correct field names (conversationExternalId, actorExternalId). const ctx = resolveLocalIpcGuardianContext(); expect(ctx.trustClass).toBe('guardian'); expect(ctx.sourceChannel).toBe('vellum'); + expect(ctx.guardianPrincipalId).toBeDefined(); }); test('respects custom sourceChannel parameter', () => { diff --git a/assistant/src/__tests__/canonical-guardian-store.test.ts b/assistant/src/__tests__/canonical-guardian-store.test.ts index fbbab35dd87..852038a86be 100644 --- a/assistant/src/__tests__/canonical-guardian-store.test.ts +++ b/assistant/src/__tests__/canonical-guardian-store.test.ts @@ -41,6 +41,10 @@ import { getDb, initializeDb, resetDb } from '../memory/db.js'; initializeDb(); +// All decisionable kinds (tool_approval, pending_question, access_request) +// require a guardianPrincipalId. Use a constant for test fixtures. +const TEST_PRINCIPAL = 'test-principal-id'; + function resetTables(): void { const db = getDb(); db.run('DELETE FROM canonical_guardian_deliveries'); @@ -71,6 +75,7 @@ describe('canonical-guardian-store', () => { conversationId: 'conv-1', requesterExternalUserId: 'user-1', guardianExternalUserId: 'guardian-1', + guardianPrincipalId: TEST_PRINCIPAL, callSessionId: 'session-1', pendingQuestionId: 'pq-1', questionText: 'Can I run this tool?', @@ -94,6 +99,7 @@ describe('canonical-guardian-store', () => { const req = createCanonicalGuardianRequest({ kind: 'access_request', sourceType: 'channel', + guardianPrincipalId: TEST_PRINCIPAL, }); expect(req.id).toBeTruthy(); @@ -111,6 +117,7 @@ describe('canonical-guardian-store', () => { const created = createCanonicalGuardianRequest({ kind: 'tool_approval', sourceType: 'voice', + guardianPrincipalId: TEST_PRINCIPAL, }); const fetched = getCanonicalGuardianRequest(created.id); @@ -127,16 +134,16 @@ describe('canonical-guardian-store', () => { // ── listCanonicalGuardianRequests ───────────────────────────────── test('lists all requests with no filters', () => { - createCanonicalGuardianRequest({ kind: 'tool_approval', sourceType: 'voice' }); - createCanonicalGuardianRequest({ kind: 'access_request', sourceType: 'channel' }); + createCanonicalGuardianRequest({ kind: 'tool_approval', sourceType: 'voice', guardianPrincipalId: TEST_PRINCIPAL }); + createCanonicalGuardianRequest({ kind: 'access_request', sourceType: 'channel', guardianPrincipalId: TEST_PRINCIPAL }); const all = listCanonicalGuardianRequests(); expect(all).toHaveLength(2); }); test('filters by status', () => { - createCanonicalGuardianRequest({ kind: 'tool_approval', sourceType: 'voice' }); - const req2 = createCanonicalGuardianRequest({ kind: 'access_request', sourceType: 'channel' }); + createCanonicalGuardianRequest({ kind: 'tool_approval', sourceType: 'voice', guardianPrincipalId: TEST_PRINCIPAL }); + const req2 = createCanonicalGuardianRequest({ kind: 'access_request', sourceType: 'channel', guardianPrincipalId: TEST_PRINCIPAL }); updateCanonicalGuardianRequest(req2.id, { status: 'approved' }); const pending = listCanonicalGuardianRequests({ status: 'pending' }); @@ -153,11 +160,13 @@ describe('canonical-guardian-store', () => { kind: 'tool_approval', sourceType: 'voice', guardianExternalUserId: 'guardian-A', + guardianPrincipalId: TEST_PRINCIPAL, }); createCanonicalGuardianRequest({ kind: 'tool_approval', sourceType: 'voice', guardianExternalUserId: 'guardian-B', + guardianPrincipalId: TEST_PRINCIPAL, }); const filtered = listCanonicalGuardianRequests({ guardianExternalUserId: 'guardian-A' }); @@ -170,11 +179,13 @@ describe('canonical-guardian-store', () => { kind: 'tool_approval', sourceType: 'voice', conversationId: 'conv-X', + guardianPrincipalId: TEST_PRINCIPAL, }); createCanonicalGuardianRequest({ kind: 'tool_approval', sourceType: 'voice', conversationId: 'conv-Y', + guardianPrincipalId: TEST_PRINCIPAL, }); const filtered = listCanonicalGuardianRequests({ conversationId: 'conv-X' }); @@ -182,18 +193,18 @@ describe('canonical-guardian-store', () => { }); test('filters by sourceType', () => { - createCanonicalGuardianRequest({ kind: 'tool_approval', sourceType: 'voice' }); - createCanonicalGuardianRequest({ kind: 'tool_approval', sourceType: 'channel' }); - createCanonicalGuardianRequest({ kind: 'tool_approval', sourceType: 'desktop' }); + createCanonicalGuardianRequest({ kind: 'tool_approval', sourceType: 'voice', guardianPrincipalId: TEST_PRINCIPAL }); + createCanonicalGuardianRequest({ kind: 'tool_approval', sourceType: 'channel', guardianPrincipalId: TEST_PRINCIPAL }); + createCanonicalGuardianRequest({ kind: 'tool_approval', sourceType: 'desktop', guardianPrincipalId: TEST_PRINCIPAL }); const voiceOnly = listCanonicalGuardianRequests({ sourceType: 'voice' }); expect(voiceOnly).toHaveLength(1); }); test('filters by kind', () => { - createCanonicalGuardianRequest({ kind: 'tool_approval', sourceType: 'voice' }); - createCanonicalGuardianRequest({ kind: 'pending_question', sourceType: 'voice' }); - createCanonicalGuardianRequest({ kind: 'access_request', sourceType: 'channel' }); + createCanonicalGuardianRequest({ kind: 'tool_approval', sourceType: 'voice', guardianPrincipalId: TEST_PRINCIPAL }); + createCanonicalGuardianRequest({ kind: 'pending_question', sourceType: 'voice', guardianPrincipalId: TEST_PRINCIPAL }); + createCanonicalGuardianRequest({ kind: 'access_request', sourceType: 'channel', guardianPrincipalId: TEST_PRINCIPAL }); const toolOnly = listCanonicalGuardianRequests({ kind: 'tool_approval' }); expect(toolOnly).toHaveLength(1); @@ -204,16 +215,19 @@ describe('canonical-guardian-store', () => { kind: 'tool_approval', sourceType: 'voice', guardianExternalUserId: 'guardian-A', + guardianPrincipalId: TEST_PRINCIPAL, }); createCanonicalGuardianRequest({ kind: 'tool_approval', sourceType: 'channel', guardianExternalUserId: 'guardian-A', + guardianPrincipalId: TEST_PRINCIPAL, }); createCanonicalGuardianRequest({ kind: 'access_request', sourceType: 'voice', guardianExternalUserId: 'guardian-A', + guardianPrincipalId: TEST_PRINCIPAL, }); const filtered = listCanonicalGuardianRequests({ @@ -230,6 +244,7 @@ describe('canonical-guardian-store', () => { const req = createCanonicalGuardianRequest({ kind: 'tool_approval', sourceType: 'voice', + guardianPrincipalId: TEST_PRINCIPAL, }); const updated = updateCanonicalGuardianRequest(req.id, { @@ -260,6 +275,7 @@ describe('canonical-guardian-store', () => { const req = createCanonicalGuardianRequest({ kind: 'tool_approval', sourceType: 'voice', + guardianPrincipalId: TEST_PRINCIPAL, }); const resolved = resolveCanonicalGuardianRequest(req.id, 'pending', { @@ -278,6 +294,7 @@ describe('canonical-guardian-store', () => { const req = createCanonicalGuardianRequest({ kind: 'tool_approval', sourceType: 'channel', + guardianPrincipalId: TEST_PRINCIPAL, }); const resolved = resolveCanonicalGuardianRequest(req.id, 'pending', { @@ -293,6 +310,7 @@ describe('canonical-guardian-store', () => { const req = createCanonicalGuardianRequest({ kind: 'tool_approval', sourceType: 'voice', + guardianPrincipalId: TEST_PRINCIPAL, }); // Try to resolve with wrong expected status @@ -311,6 +329,7 @@ describe('canonical-guardian-store', () => { const req = createCanonicalGuardianRequest({ kind: 'tool_approval', sourceType: 'voice', + guardianPrincipalId: TEST_PRINCIPAL, }); // First resolve succeeds @@ -353,6 +372,7 @@ describe('canonical-guardian-store', () => { sourceChannel: 'twilio', conversationId: 'conv-voice-1', guardianExternalUserId: 'guardian-phone', + guardianPrincipalId: TEST_PRINCIPAL, callSessionId: 'call-123', pendingQuestionId: 'pq-456', questionText: 'What is the gate code?', @@ -374,6 +394,7 @@ describe('canonical-guardian-store', () => { conversationId: 'conv-tg-1', requesterExternalUserId: 'requester-tg-user', guardianExternalUserId: 'guardian-tg-user', + guardianPrincipalId: TEST_PRINCIPAL, toolName: 'execute_code', inputDigest: 'sha256:abcdef', expiresAt: new Date(Date.now() + 120_000).toISOString(), @@ -394,6 +415,7 @@ describe('canonical-guardian-store', () => { sourceType: 'desktop', conversationId: 'conv-desktop-1', guardianExternalUserId: 'guardian-desktop', + guardianPrincipalId: TEST_PRINCIPAL, questionText: 'User wants to access settings', }); @@ -408,6 +430,7 @@ describe('canonical-guardian-store', () => { const req = createCanonicalGuardianRequest({ kind: 'tool_approval', sourceType: 'voice', + guardianPrincipalId: TEST_PRINCIPAL, }); const d1 = createCanonicalGuardianDelivery({ @@ -436,6 +459,7 @@ describe('canonical-guardian-store', () => { const req = createCanonicalGuardianRequest({ kind: 'tool_approval', sourceType: 'voice', + guardianPrincipalId: TEST_PRINCIPAL, }); const deliveries = listCanonicalGuardianDeliveries(req.id); @@ -446,10 +470,12 @@ describe('canonical-guardian-store', () => { const pendingReq = createCanonicalGuardianRequest({ kind: 'pending_question', sourceType: 'voice', + guardianPrincipalId: TEST_PRINCIPAL, }); const resolvedReq = createCanonicalGuardianRequest({ kind: 'pending_question', sourceType: 'voice', + guardianPrincipalId: TEST_PRINCIPAL, }); updateCanonicalGuardianRequest(resolvedReq.id, { status: 'approved' }); @@ -476,6 +502,7 @@ describe('canonical-guardian-store', () => { const req = createCanonicalGuardianRequest({ kind: 'pending_question', sourceType: 'voice', + guardianPrincipalId: TEST_PRINCIPAL, }); createCanonicalGuardianDelivery({ @@ -498,6 +525,7 @@ describe('canonical-guardian-store', () => { const req = createCanonicalGuardianRequest({ kind: 'tool_approval', sourceType: 'voice', + guardianPrincipalId: TEST_PRINCIPAL, }); const delivery = createCanonicalGuardianDelivery({ requestId: req.id, @@ -525,6 +553,7 @@ describe('canonical-guardian-store', () => { const req = createCanonicalGuardianRequest({ kind: 'pending_question', sourceType: 'voice', + guardianPrincipalId: TEST_PRINCIPAL, }); createCanonicalGuardianDelivery({ requestId: req.id, @@ -544,10 +573,12 @@ describe('canonical-guardian-store', () => { const pendingReq = createCanonicalGuardianRequest({ kind: 'pending_question', sourceType: 'voice', + guardianPrincipalId: TEST_PRINCIPAL, }); const resolvedReq = createCanonicalGuardianRequest({ kind: 'pending_question', sourceType: 'voice', + guardianPrincipalId: TEST_PRINCIPAL, }); updateCanonicalGuardianRequest(resolvedReq.id, { status: 'approved' }); @@ -574,6 +605,7 @@ describe('canonical-guardian-store', () => { const req = createCanonicalGuardianRequest({ kind: 'pending_question', sourceType: 'voice', + guardianPrincipalId: TEST_PRINCIPAL, }); // Two delivery rows targeting the same chat for the same request @@ -602,6 +634,7 @@ describe('canonical-guardian-store', () => { const req = createCanonicalGuardianRequest({ kind: 'pending_question', sourceType: 'voice', + guardianPrincipalId: TEST_PRINCIPAL, }); createCanonicalGuardianDelivery({ requestId: req.id, @@ -620,6 +653,7 @@ describe('canonical-guardian-store', () => { const req = createCanonicalGuardianRequest({ kind: 'pending_question', sourceType: 'voice', + guardianPrincipalId: TEST_PRINCIPAL, }); createCanonicalGuardianDelivery({ requestId: req.id, diff --git a/assistant/src/__tests__/channel-approval-routes.test.ts b/assistant/src/__tests__/channel-approval-routes.test.ts index f22309ababf..b3936fb900f 100644 --- a/assistant/src/__tests__/channel-approval-routes.test.ts +++ b/assistant/src/__tests__/channel-approval-routes.test.ts @@ -2789,6 +2789,7 @@ describe('NL approval routing via destination-scoped canonical requests', () => sourceChannel: 'twilio', conversationId: 'conv-voice-nl-1', toolName: 'shell', + guardianPrincipalId: 'test-principal-id', expiresAt: new Date(Date.now() + 60_000).toISOString(), // guardianExternalUserId intentionally omitted }); @@ -2842,6 +2843,7 @@ describe('NL approval routing via destination-scoped canonical requests', () => sourceType: 'voice', sourceChannel: 'twilio', toolName: 'shell', + guardianPrincipalId: 'test-principal-id', expiresAt: new Date(Date.now() + 60_000).toISOString(), }); diff --git a/assistant/src/__tests__/confirmation-request-guardian-bridge.test.ts b/assistant/src/__tests__/confirmation-request-guardian-bridge.test.ts index 1164280762a..ec514e05b82 100644 --- a/assistant/src/__tests__/confirmation-request-guardian-bridge.test.ts +++ b/assistant/src/__tests__/confirmation-request-guardian-bridge.test.ts @@ -118,6 +118,7 @@ function makeCanonicalRequest(overrides: Record = {}) { conversationId: 'conv-1', requesterExternalUserId: 'requester-1', guardianExternalUserId: 'guardian-1', + guardianPrincipalId: 'test-principal-id', toolName: 'bash', status: 'pending', requestCode: generateCanonicalRequestCode(), diff --git a/assistant/src/__tests__/guardian-principal-id-roundtrip.test.ts b/assistant/src/__tests__/guardian-principal-id-roundtrip.test.ts index 4dd50da2fb1..d371d06e284 100644 --- a/assistant/src/__tests__/guardian-principal-id-roundtrip.test.ts +++ b/assistant/src/__tests__/guardian-principal-id-roundtrip.test.ts @@ -116,13 +116,21 @@ describe('guardianPrincipalId roundtrip', () => { expect(fetched!.decidedByPrincipalId).toBeNull(); }); - test('creates request without guardianPrincipalId (defaults to null)', () => { + test('access_request requires guardianPrincipalId (decisionable kind)', () => { + // access_request is now decisionable — creating one without a principal + // should throw IntegrityError. + expect(() => createCanonicalGuardianRequest({ + kind: 'access_request', + sourceType: 'desktop', + })).toThrow('guardianPrincipalId'); + + // With a principal, creation succeeds const req = createCanonicalGuardianRequest({ kind: 'access_request', sourceType: 'desktop', + guardianPrincipalId: 'access-req-principal', }); - - expect(req.guardianPrincipalId).toBeNull(); + expect(req.guardianPrincipalId).toBe('access-req-principal'); expect(req.decidedByPrincipalId).toBeNull(); }); diff --git a/assistant/src/__tests__/non-member-access-request.test.ts b/assistant/src/__tests__/non-member-access-request.test.ts index 8f9baebb85f..ad613792723 100644 --- a/assistant/src/__tests__/non-member-access-request.test.ts +++ b/assistant/src/__tests__/non-member-access-request.test.ts @@ -138,12 +138,12 @@ function buildInboundRequest(overrides: Record = {}): Request { const body: Record = { sourceChannel: 'telegram', interface: 'telegram', - externalChatId: 'chat-123', + conversationExternalId: 'chat-123', externalMessageId: `msg-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`, content: 'Hello, can I use this assistant?', - senderExternalUserId: 'user-unknown-456', - senderName: 'Alice Unknown', - senderUsername: 'alice_unknown', + actorExternalId: 'user-unknown-456', + actorDisplayName: 'Alice Unknown', + actorUsername: 'alice_unknown', replyCallbackUrl: 'http://localhost:7830/deliver/telegram', ...overrides, }; @@ -206,8 +206,8 @@ describe('non-member access request notification', () => { expect(emitSignalCalls[0].sourceEventName).toBe('ingress.access_request'); expect(emitSignalCalls[0].sourceChannel).toBe('telegram'); const payload = emitSignalCalls[0].contextPayload as Record; - expect(payload.senderExternalUserId).toBe('user-unknown-456'); - expect(payload.senderName).toBe('Alice Unknown'); + expect(payload.actorExternalId).toBe('user-unknown-456'); + expect(payload.actorDisplayName).toBe('Alice Unknown'); // A canonical access request was created const pending = listCanonicalGuardianRequests({ @@ -258,9 +258,9 @@ describe('non-member access request notification', () => { expect(pending.length).toBe(1); }); - test('access request is created and signal emitted even without same-channel guardian binding', async () => { - // No guardian binding on any channel — access request should still be - // created and notification signal emitted (null guardianExternalUserId). + test('access request is created with self-healed principal even without same-channel guardian binding', async () => { + // No guardian binding on any channel — self-heal creates a vellum binding + // so the access_request (now decisionable) has a guardianPrincipalId. const req = buildInboundRequest(); const resp = await handleChannelInbound(req, undefined, TEST_BEARER_TOKEN); const json = await resp.json() as Record; @@ -276,7 +276,7 @@ describe('non-member access request notification', () => { expect(emitSignalCalls.length).toBe(1); expect(emitSignalCalls[0].sourceEventName).toBe('ingress.access_request'); - // Canonical request was created with null guardianExternalUserId + // Canonical request was created with a self-healed principal const pending = listCanonicalGuardianRequests({ status: 'pending', requesterExternalUserId: 'user-unknown-456', @@ -284,7 +284,9 @@ describe('non-member access request notification', () => { kind: 'access_request', }); expect(pending.length).toBe(1); - expect(pending[0].guardianExternalUserId).toBeNull(); + // Self-heal bootstraps a vellum binding — guardianExternalUserId is now set + expect(pending[0].guardianExternalUserId).toBeDefined(); + expect(pending[0].guardianPrincipalId).toBeDefined(); }); test('cross-channel fallback: SMS guardian binding resolves for Telegram access request', async () => { @@ -319,7 +321,7 @@ describe('non-member access request notification', () => { expect(pending[0].guardianExternalUserId).toBe('guardian-sms-user'); }); - test('no notification when senderExternalUserId is absent', async () => { + test('no notification when actorExternalId is absent', async () => { createBinding({ assistantId: 'self', channel: 'telegram', @@ -327,13 +329,12 @@ describe('non-member access request notification', () => { guardianDeliveryChatId: 'guardian-chat-789', }); - // Message without senderExternalUserId — can't identify the requester. - // The ACL check requires senderExternalUserId to look up members, - // so without it the non-member gate is bypassed entirely. + // Message without actorExternalId — the handler returns BAD_REQUEST. const req = buildInboundRequest({ - senderExternalUserId: undefined, + actorExternalId: undefined, }); - await handleChannelInbound(req, undefined, TEST_BEARER_TOKEN); + const resp = await handleChannelInbound(req, undefined, TEST_BEARER_TOKEN); + expect(resp.status).toBe(400); // No access request notification should fire (no identity to notify about) expect(emitSignalCalls.length).toBe(0); @@ -345,12 +346,12 @@ describe('access-request-helper unit tests', () => { resetState(); }); - test('notifyGuardianOfAccessRequest returns no_sender_id when senderExternalUserId is absent', () => { + test('notifyGuardianOfAccessRequest returns no_sender_id when actorExternalId is absent', () => { const result = notifyGuardianOfAccessRequest({ canonicalAssistantId: 'self', sourceChannel: 'telegram', - externalChatId: 'chat-123', - senderExternalUserId: undefined, + conversationExternalId: 'chat-123', + actorExternalId: undefined, }); expect(result.notified).toBe(false); @@ -363,13 +364,13 @@ describe('access-request-helper unit tests', () => { expect(pending.length).toBe(0); }); - test('notifyGuardianOfAccessRequest creates request with null guardianExternalUserId when no binding exists', () => { + test('notifyGuardianOfAccessRequest creates request with self-healed principal when no binding exists', () => { const result = notifyGuardianOfAccessRequest({ canonicalAssistantId: 'self', sourceChannel: 'telegram', - externalChatId: 'chat-123', - senderExternalUserId: 'unknown-user', - senderName: 'Bob', + conversationExternalId: 'chat-123', + actorExternalId: 'unknown-user', + actorDisplayName: 'Bob', }); expect(result.notified).toBe(true); @@ -383,7 +384,9 @@ describe('access-request-helper unit tests', () => { kind: 'access_request', }); expect(pending.length).toBe(1); - expect(pending[0].guardianExternalUserId).toBeNull(); + // Self-heal bootstraps a vellum binding + expect(pending[0].guardianExternalUserId).toBeDefined(); + expect(pending[0].guardianPrincipalId).toBeDefined(); // Signal was emitted expect(emitSignalCalls.length).toBe(1); @@ -401,8 +404,8 @@ describe('access-request-helper unit tests', () => { const result = notifyGuardianOfAccessRequest({ canonicalAssistantId: 'self', sourceChannel: 'telegram', - externalChatId: 'tg-chat', - senderExternalUserId: 'unknown-tg-user', + conversationExternalId: 'tg-chat', + actorExternalId: 'unknown-tg-user', }); expect(result.notified).toBe(true); @@ -438,8 +441,8 @@ describe('access-request-helper unit tests', () => { const result = notifyGuardianOfAccessRequest({ canonicalAssistantId: 'self', sourceChannel: 'telegram', - externalChatId: 'chat-123', - senderExternalUserId: 'unknown-user', + conversationExternalId: 'chat-123', + actorExternalId: 'unknown-user', }); expect(result.notified).toBe(true); @@ -457,13 +460,13 @@ describe('access-request-helper unit tests', () => { expect(payload.guardianBindingChannel).toBe('telegram'); }); - test('notifyGuardianOfAccessRequest for voice channel includes senderName in contextPayload', () => { + test('notifyGuardianOfAccessRequest for voice channel includes actorDisplayName in contextPayload', () => { const result = notifyGuardianOfAccessRequest({ canonicalAssistantId: 'self', sourceChannel: 'voice', - externalChatId: '+15559998888', - senderExternalUserId: '+15559998888', - senderName: 'Alice Caller', + conversationExternalId: '+15559998888', + actorExternalId: '+15559998888', + actorDisplayName: 'Alice Caller', }); expect(result.notified).toBe(true); @@ -471,8 +474,8 @@ describe('access-request-helper unit tests', () => { const payload = emitSignalCalls[0].contextPayload as Record; expect(payload.sourceChannel).toBe('voice'); - expect(payload.senderName).toBe('Alice Caller'); - expect(payload.senderExternalUserId).toBe('+15559998888'); + expect(payload.actorDisplayName).toBe('Alice Caller'); + expect(payload.actorExternalId).toBe('+15559998888'); expect(payload.senderIdentifier).toBe('Alice Caller'); // Canonical request should exist @@ -489,9 +492,9 @@ describe('access-request-helper unit tests', () => { const result = notifyGuardianOfAccessRequest({ canonicalAssistantId: 'self', sourceChannel: 'telegram', - externalChatId: 'chat-123', - senderExternalUserId: 'unknown-user', - senderName: 'Test User', + conversationExternalId: 'chat-123', + actorExternalId: 'unknown-user', + actorDisplayName: 'Test User', }); expect(result.notified).toBe(true); @@ -507,9 +510,9 @@ describe('access-request-helper unit tests', () => { const result = notifyGuardianOfAccessRequest({ canonicalAssistantId: 'self', sourceChannel: 'telegram', - externalChatId: 'chat-123', - senderExternalUserId: 'revoked-user', - senderName: 'Revoked User', + conversationExternalId: 'chat-123', + actorExternalId: 'revoked-user', + actorDisplayName: 'Revoked User', previousMemberStatus: 'revoked', }); @@ -544,9 +547,9 @@ describe('access-request-helper unit tests', () => { const result = notifyGuardianOfAccessRequest({ canonicalAssistantId: 'self', sourceChannel: 'voice', - externalChatId: '+15556667777', - senderExternalUserId: '+15556667777', - senderName: 'Noah', + conversationExternalId: '+15556667777', + actorExternalId: '+15556667777', + actorDisplayName: 'Noah', }); expect(result.notified).toBe(true); @@ -584,9 +587,9 @@ describe('access-request-helper unit tests', () => { const result = notifyGuardianOfAccessRequest({ canonicalAssistantId: 'self', sourceChannel: 'telegram', - externalChatId: 'chat-123', - senderExternalUserId: 'unknown-user', - senderName: 'Alice', + conversationExternalId: 'chat-123', + actorExternalId: 'unknown-user', + actorDisplayName: 'Alice', }); expect(result.notified).toBe(true); diff --git a/assistant/src/__tests__/send-endpoint-busy.test.ts b/assistant/src/__tests__/send-endpoint-busy.test.ts index b86b0da56ad..489c0911e8d 100644 --- a/assistant/src/__tests__/send-endpoint-busy.test.ts +++ b/assistant/src/__tests__/send-endpoint-busy.test.ts @@ -333,6 +333,7 @@ describe('POST /v1/messages — queue-if-busy and hub publishing', () => { sourceChannel: 'vellum', conversationId, toolName: 'call_start', + guardianPrincipalId: 'test-principal-id', status: 'pending', requestCode: 'ABC123', expiresAt: new Date(Date.now() + 5 * 60 * 1000).toISOString(), @@ -389,6 +390,7 @@ describe('POST /v1/messages — queue-if-busy and hub publishing', () => { conversationId, toolName: 'call_start', status: 'pending', + guardianPrincipalId: 'test-principal-id', requestCode: 'C0FFEE', expiresAt: new Date(Date.now() + 5 * 60 * 1000).toISOString(), }); @@ -450,6 +452,7 @@ describe('POST /v1/messages — queue-if-busy and hub publishing', () => { conversationId, toolName: 'call_start', status: 'pending', + guardianPrincipalId: 'test-principal-id', requestCode: 'DEF456', expiresAt: new Date(Date.now() + 5 * 60 * 1000).toISOString(), }); @@ -505,6 +508,7 @@ describe('POST /v1/messages — queue-if-busy and hub publishing', () => { conversationId, toolName: 'call_start', status: 'pending', + guardianPrincipalId: 'test-principal-id', requestCode: 'Q2D456', expiresAt: new Date(Date.now() + 5 * 60 * 1000).toISOString(), }); @@ -560,6 +564,7 @@ describe('POST /v1/messages — queue-if-busy and hub publishing', () => { conversationId, toolName: 'call_start', status: 'pending', + guardianPrincipalId: 'test-principal-id', requestCode: 'GHI789', expiresAt: new Date(Date.now() + 5 * 60 * 1000).toISOString(), }); @@ -613,6 +618,7 @@ describe('POST /v1/messages — queue-if-busy and hub publishing', () => { conversationId, toolName: 'call_start', status: 'pending', + guardianPrincipalId: 'test-principal-id', requestCode: 'JKL012', expiresAt: new Date(Date.now() + 5 * 60 * 1000).toISOString(), }); diff --git a/assistant/src/calls/call-pointer-messages.ts b/assistant/src/calls/call-pointer-messages.ts index ab504228512..d8719c91fc0 100644 --- a/assistant/src/calls/call-pointer-messages.ts +++ b/assistant/src/calls/call-pointer-messages.ts @@ -126,11 +126,11 @@ export async function addPointerMessage( const outcomeKeyword = eventOutcomeKeywords[event]; if (outcomeKeyword) requiredFacts.push(outcomeKeyword); - const isTrusted = + const trustedAudience = audienceMode === 'trusted' || (audienceMode === 'auto' && resolvePointerAudienceTrust(conversationId)); - if (isTrusted && pointerMessageProcessor) { + if (trustedAudience && pointerMessageProcessor) { // Route through the daemon session — the assistant generates the // pointer text as a natural conversation turn, shaped by context, // identity, and preferences. @@ -141,7 +141,7 @@ export async function addPointerMessage( } catch (err) { log.warn({ err, event, conversationId }, 'Daemon pointer processing failed, falling back to deterministic'); } - } else if (!isTrusted && pointerMessageProcessor) { + } else if (!trustedAudience && pointerMessageProcessor) { log.debug({ event, conversationId }, 'Untrusted audience — using deterministic pointer copy'); } diff --git a/assistant/src/calls/guardian-dispatch.ts b/assistant/src/calls/guardian-dispatch.ts index 77c487df975..b9dbb9a0d4d 100644 --- a/assistant/src/calls/guardian-dispatch.ts +++ b/assistant/src/calls/guardian-dispatch.ts @@ -17,6 +17,7 @@ import { import { getActiveBinding } from '../memory/guardian-bindings.js'; import { emitNotificationSignal } from '../notifications/emit-signal.js'; import type { NotificationDeliveryResult } from '../notifications/types.js'; +import { ensureVellumGuardianBinding } from '../runtime/guardian-vellum-migration.js'; import { getLogger } from '../util/logger.js'; import { getUserConsultationTimeoutMs } from './call-constants.js'; import type { CallPendingQuestion } from './types.js'; @@ -93,8 +94,28 @@ async function dispatchGuardianQuestionInner(params: GuardianDispatchParams): Pr // level guardian identity. Resolve the principal from the vellum binding // (the canonical assistant-level binding) so the request is attributed to // the assistant's guardian principal. - const vellumBinding = getActiveBinding(assistantId, 'vellum'); - const guardianPrincipalId = vellumBinding?.guardianPrincipalId ?? undefined; + let vellumBinding = getActiveBinding(assistantId, 'vellum'); + let guardianPrincipalId = vellumBinding?.guardianPrincipalId ?? undefined; + + // Self-heal: if the vellum binding is missing or lacks a principal, + // bootstrap it so the pending_question request can be attributed. + if (!guardianPrincipalId) { + log.info( + { callSessionId, assistantId, hadBinding: !!vellumBinding }, + 'Vellum binding missing or lacks principal — self-healing for voice dispatch', + ); + const healedPrincipalId = ensureVellumGuardianBinding(assistantId); + vellumBinding = getActiveBinding(assistantId, 'vellum'); + guardianPrincipalId = vellumBinding?.guardianPrincipalId ?? healedPrincipalId; + } + + if (!guardianPrincipalId) { + log.error( + { callSessionId, assistantId }, + 'Voice guardian dispatch: no guardianPrincipalId after self-heal — cannot create pending_question', + ); + return; + } // Create the canonical guardian request as the primary record. const request = createCanonicalGuardianRequest({ diff --git a/assistant/src/memory/canonical-guardian-store.ts b/assistant/src/memory/canonical-guardian-store.ts index 6bdf4d9c0be..c1e2c71d31c 100644 --- a/assistant/src/memory/canonical-guardian-store.ts +++ b/assistant/src/memory/canonical-guardian-store.ts @@ -190,6 +190,7 @@ const DECISIONABLE_KINDS = new Set([ 'tool_approval', 'tool_grant_request', 'pending_question', + 'access_request', ]); export function createCanonicalGuardianRequest(params: CreateCanonicalGuardianRequestParams): CanonicalGuardianRequest { diff --git a/assistant/src/memory/migrations/126-backfill-guardian-principal-id.ts b/assistant/src/memory/migrations/126-backfill-guardian-principal-id.ts index 5f37a07c157..fed67494841 100644 --- a/assistant/src/memory/migrations/126-backfill-guardian-principal-id.ts +++ b/assistant/src/memory/migrations/126-backfill-guardian-principal-id.ts @@ -25,7 +25,8 @@ import { withCrashRecovery } from './validate-migration-state.js'; * b. For desktop-originated requests (sourceType = 'desktop' or * sourceChannel = 'vellum') that lack guardianExternalUserId, * use the assistant principal derived in step 1. - * c. Pending requests that cannot be deterministically bound are expired. + * c. Pending requests that cannot be deterministically bound are expired + * (including access_request rows, which are now principal-bound). * * 4. Idempotent: uses checkpoint key + only updates rows with NULL * guardianPrincipalId. @@ -151,10 +152,8 @@ export function migrateBackfillGuardianPrincipalId(database: DrizzleDb): void { const principal = externalToP.get(req.guardian_external_user_id); if (principal) { updateStmt.run(principal, now, req.id); - } else if (req.kind !== 'access_request') { + } else { // Cannot deterministically map — will expire below. - // Exclude access_request: non-decisionable, proceeds via - // invite flow and does not require principal binding. unboundRequestIds.push(req.id); } } @@ -175,20 +174,15 @@ export function migrateBackfillGuardianPrincipalId(database: DrizzleDb): void { ).run(assistantPrincipal, now); } - // 3c. Expire remaining pending DECISIONABLE requests that still - // have no guardian_principal_id, regardless of their expires_at - // value. These requests can never be approved in the principal- - // based system, so they must be expired proactively. - // - // Exclude `access_request` rows: they are non-decisionable and - // proceed via the invite flow without requiring principal-bound - // approve/deny. Expiring them would drop valid in-flight access - // requests that the guardian has not yet acted on. + // 3c. Expire remaining pending requests that still have no + // guardian_principal_id. These requests can never be approved + // in the principal-based system, so they must be expired + // proactively. This includes access_request rows which are + // now decisionable and principal-bound. const stillUnbound = raw.query( `SELECT id FROM canonical_guardian_requests WHERE guardian_principal_id IS NULL - AND status = 'pending' - AND kind != 'access_request'`, + AND status = 'pending'`, ).all() as Array<{ id: string }>; for (const req of stillUnbound) { diff --git a/assistant/src/runtime/access-request-helper.ts b/assistant/src/runtime/access-request-helper.ts index cd8caa733e7..dc7a0f1cab7 100644 --- a/assistant/src/runtime/access-request-helper.ts +++ b/assistant/src/runtime/access-request-helper.ts @@ -26,6 +26,7 @@ import { emitNotificationSignal } from '../notifications/emit-signal.js'; import type { NotificationDeliveryResult } from '../notifications/types.js'; import { getLogger } from '../util/logger.js'; import { getGuardianBinding } from './channel-guardian-service.js'; +import { ensureVellumGuardianBinding } from './guardian-vellum-migration.js'; import { GUARDIAN_APPROVAL_TTL_MS } from './routes/channel-route-shared.js'; const log = getLogger('access-request-helper'); @@ -114,14 +115,24 @@ export function notifyGuardianOfAccessRequest( { sourceChannel, fallbackChannel: guardianBindingChannel, canonicalAssistantId }, 'Using cross-channel guardian binding fallback for access request', ); - } else { - log.debug( - { sourceChannel, canonicalAssistantId }, - 'No guardian binding for access request — proceeding without guardian identity', - ); } } + // Self-heal: access_request is now decisionable and requires a principal. + // If no binding was found (or the binding lacks a principal), bootstrap the + // vellum binding so the request can be properly attributed. + if (!guardianPrincipalId) { + log.info( + { sourceChannel, canonicalAssistantId }, + 'No guardian principal for access request — self-healing vellum binding', + ); + const healedPrincipalId = ensureVellumGuardianBinding(canonicalAssistantId); + const vellumBinding = getGuardianBinding(canonicalAssistantId, 'vellum'); + guardianExternalUserId = vellumBinding?.guardianExternalUserId ?? guardianExternalUserId; + guardianPrincipalId = vellumBinding?.guardianPrincipalId ?? healedPrincipalId; + guardianBindingChannel = guardianBindingChannel ?? 'vellum'; + } + // The conversationId is assistant-scoped so the dedupe query below only // matches requests for the same assistant. Without this, a pending request // from assistant A could be returned for assistant B, allowing the caller diff --git a/assistant/src/runtime/local-actor-identity.ts b/assistant/src/runtime/local-actor-identity.ts index c53a418aee2..390ab7a59ea 100644 --- a/assistant/src/runtime/local-actor-identity.ts +++ b/assistant/src/runtime/local-actor-identity.ts @@ -55,8 +55,8 @@ export function resolveLocalIpcGuardianContext( const guardianCtx = resolveGuardianContext({ assistantId, sourceChannel: 'vellum', - externalChatId: 'local', - senderExternalUserId: principalId, + conversationExternalId: 'local', + actorExternalId: principalId, }); return toGuardianRuntimeContext(sourceChannel, guardianCtx); } From fa9ab011054b356311fda998ef30b5e6e3695326 Mon Sep 17 00:00:00 2001 From: Noa Flaherty Date: Sun, 1 Mar 2026 09:36:20 -0500 Subject: [PATCH 2/2] fix: bump migration checkpoint to v3 so access_request backfill runs on upgraded databases Addresses Codex review: the withCrashRecovery checkpoint key was still v2, meaning databases that already completed v2 would skip the new access_request principal-binding and expiration logic. Co-Authored-By: Claude Opus 4.6 --- .../src/memory/migrations/126-backfill-guardian-principal-id.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assistant/src/memory/migrations/126-backfill-guardian-principal-id.ts b/assistant/src/memory/migrations/126-backfill-guardian-principal-id.ts index fed67494841..661e24e827a 100644 --- a/assistant/src/memory/migrations/126-backfill-guardian-principal-id.ts +++ b/assistant/src/memory/migrations/126-backfill-guardian-principal-id.ts @@ -32,7 +32,7 @@ import { withCrashRecovery } from './validate-migration-state.js'; * guardianPrincipalId. */ export function migrateBackfillGuardianPrincipalId(database: DrizzleDb): void { - withCrashRecovery(database, 'migration_backfill_guardian_principal_id_v2', () => { + withCrashRecovery(database, 'migration_backfill_guardian_principal_id_v3', () => { const raw = getSqliteFrom(database); // Guard: tables must exist