From 2a25a68afefe1711e83a00963d130f8bde2b84e7 Mon Sep 17 00:00:00 2001 From: Noa Flaherty Date: Wed, 25 Feb 2026 22:07:55 -0500 Subject: [PATCH] fix: address review feedback from PR #9336 Co-Authored-By: Claude --- .../approval-message-composer.test.ts | 16 ++++----- .../src/__tests__/channel-guardian.test.ts | 6 ++-- .../src/runtime/approval-message-composer.ts | 12 ++++--- .../runtime/routes/inbound-message-handler.ts | 34 ++++++++++++++----- .../Settings/SettingsConnectTab.swift | 33 ++++++++++-------- 5 files changed, 61 insertions(+), 40 deletions(-) diff --git a/assistant/src/__tests__/approval-message-composer.test.ts b/assistant/src/__tests__/approval-message-composer.test.ts index 00993c39561..744900c3c24 100644 --- a/assistant/src/__tests__/approval-message-composer.test.ts +++ b/assistant/src/__tests__/approval-message-composer.test.ts @@ -127,14 +127,12 @@ describe('approval-message-composer', () => { expect(msg).toContain('Code did not match.'); }); - test('guardian_verify_challenge_setup includes verifyCommand and ttlSeconds', () => { + test('guardian_verify_challenge_setup includes verifyCommand in six-digit code format', () => { const msg = getFallbackMessage({ scenario: 'guardian_verify_challenge_setup', - verifyCommand: '/verify abc123', - ttlSeconds: 30, + verifyCommand: '123456', }); - expect(msg).toContain('/verify abc123'); - expect(msg).toContain('30'); + expect(msg).toContain('six-digit code: 123456'); }); }); @@ -195,16 +193,14 @@ describe('approval-message-composer', () => { // ----------------------------------------------------------------------- describe('verification scenario resilience', () => { - test('guardian_verify_challenge_setup includes verify command and TTL', () => { + test('guardian_verify_challenge_setup includes verify code', () => { const msg = composeApprovalMessage({ scenario: 'guardian_verify_challenge_setup', - verifyCommand: '/guardian_verify abc123def456', - ttlSeconds: 600, + verifyCommand: '987654', }); expect(typeof msg).toBe('string'); expect(msg.length).toBeGreaterThan(0); - expect(msg).toContain('/guardian_verify abc123def456'); - expect(msg).toContain('600'); + expect(msg).toContain('six-digit code: 987654'); }); test('guardian_verify_failed includes failure reason', () => { diff --git a/assistant/src/__tests__/channel-guardian.test.ts b/assistant/src/__tests__/channel-guardian.test.ts index 1d1aebaff4b..0510a9615a4 100644 --- a/assistant/src/__tests__/channel-guardian.test.ts +++ b/assistant/src/__tests__/channel-guardian.test.ts @@ -395,21 +395,21 @@ describe('guardian service challenge validation', () => { expect(result.ttlSeconds).toBe(600); expect(result.instruction).toBeDefined(); expect(result.instruction.length).toBeGreaterThan(0); - expect(result.instruction).toContain('code you were given'); + expect(result.instruction).toContain(`six-digit code: ${result.secret}`); }); test('createVerificationChallenge produces a non-empty instruction for telegram channel', () => { const result = createVerificationChallenge('asst-1', 'telegram'); expect(result.instruction).toBeDefined(); expect(result.instruction.length).toBeGreaterThan(0); - expect(result.instruction).toContain('code you were given'); + expect(result.instruction).toContain(`six-digit code: ${result.secret}`); }); test('createVerificationChallenge produces a non-empty instruction for sms channel', () => { const result = createVerificationChallenge('asst-1', 'sms'); expect(result.instruction).toBeDefined(); expect(result.instruction.length).toBeGreaterThan(0); - expect(result.instruction).toContain('code you were given'); + expect(result.instruction).toContain(`six-digit code: ${result.secret}`); }); test('validateAndConsumeChallenge succeeds with correct secret', () => { diff --git a/assistant/src/runtime/approval-message-composer.ts b/assistant/src/runtime/approval-message-composer.ts index 47c2c3b4d78..c15a40ea86a 100644 --- a/assistant/src/runtime/approval-message-composer.ts +++ b/assistant/src/runtime/approval-message-composer.ts @@ -216,13 +216,17 @@ export function getFallbackMessage(context: ApprovalMessageContext): string { case 'guardian_verify_failed': return `Verification failed. ${context.failureReason ?? 'Please try again.'}`; - case 'guardian_verify_challenge_setup': + case 'guardian_verify_challenge_setup': { + // All channels now use 6-digit numeric codes. The instruction must + // include the code so the macOS client (and other consumers) can + // parse it from the instruction text. The "six-digit code: " + // format is shared across channels for consistency. + const code = context.verifyCommand ?? 'the verification code'; if (context.channel === 'voice') { - // Voice challenges use a six-digit numeric code that can be spoken aloud - const code = context.verifyCommand ?? 'the verification code'; return `To complete guardian verification, speak or enter the six-digit code: ${code}.`; } - return `To complete guardian verification, reply in the channel with the code you were given.`; + return `To complete guardian verification, send the six-digit code: ${code}.`; + } case 'guardian_verify_status_bound': return 'A guardian is currently active for this channel.'; diff --git a/assistant/src/runtime/routes/inbound-message-handler.ts b/assistant/src/runtime/routes/inbound-message-handler.ts index 3f119c57e81..ef57da1cf28 100644 --- a/assistant/src/runtime/routes/inbound-message-handler.ts +++ b/assistant/src/runtime/routes/inbound-message-handler.ts @@ -74,16 +74,21 @@ const log = getLogger('runtime-http'); * 1. `/guardian_verify ` (legacy command format) * 2. `/guardian_verify@BotName ` (Telegram group format) * 3. A bare 6-digit numeric code as the entire message - * Returns the verification code if recognized, or undefined otherwise. + * Returns `{ code, isExplicitCommand }` if recognized, or undefined otherwise. + * `isExplicitCommand` is true for legacy /guardian_verify commands (explicit + * intent) and false for bare codes (which need additional gating to avoid + * intercepting normal 6-digit messages like zip codes or PINs). */ -function parseGuardianVerifyCommand(content: string): string | undefined { +function parseGuardianVerifyCommand(content: string): { code: string; isExplicitCommand: boolean } | undefined { // Legacy /guardian_verify command format const commandMatch = content.match(/^\/guardian_verify(?:@\S+)?\s+(\S+)/); - if (commandMatch) return commandMatch[1]; + if (commandMatch) return { code: commandMatch[1], isExplicitCommand: true }; // Bare 6-digit numeric code const bareMatch = content.match(/^\d{6}$/); - return bareMatch?.[0]; + if (bareMatch) return { code: bareMatch[0], isExplicitCommand: false }; + + return undefined; } export async function handleChannelInbound( @@ -192,8 +197,8 @@ export async function handleChannelInbound( // /guardian_verify must bypass the ACL membership check — users without a // member record need to verify before they can be recognized as members. - const guardianVerifyCode = parseGuardianVerifyCommand(trimmedContent); - const isGuardianVerifyCommand = guardianVerifyCode !== undefined; + const guardianVerifyParsed = parseGuardianVerifyCommand(trimmedContent); + const isGuardianVerifyCommand = guardianVerifyParsed !== undefined; // /start gv_ bootstrap commands must also bypass ACL — the user // hasn't been verified yet and needs to complete the bootstrap handshake. @@ -592,15 +597,28 @@ export async function handleChannelInbound( // delivered via template-driven deterministic messages and the command // is short-circuited — it NEVER enters the agent pipeline. This // prevents verification commands from producing agent-generated copy. + // + // Bare 6-digit codes are only intercepted when there is actually a + // pending challenge or active outbound session for this channel. + // Without this guard, normal 6-digit messages (zip codes, PINs, etc.) + // would be swallowed by the verification handler and never reach the + // agent pipeline. Legacy /guardian_verify commands are always + // intercepted because the explicit command prefix signals clear intent. + const shouldInterceptVerification = guardianVerifyParsed !== undefined && + (guardianVerifyParsed.isExplicitCommand || + !!getPendingChallenge(canonicalAssistantId, sourceChannel) || + !!findActiveSession(canonicalAssistantId, sourceChannel)); + if ( !result.duplicate && - guardianVerifyCode !== undefined && + shouldInterceptVerification && + guardianVerifyParsed !== undefined && body.senderExternalUserId ) { const verifyResult = validateAndConsumeChallenge( canonicalAssistantId, sourceChannel, - guardianVerifyCode, + guardianVerifyParsed.code, body.senderExternalUserId, externalChatId, body.senderUsername, diff --git a/clients/macos/vellum-assistant/Features/Settings/SettingsConnectTab.swift b/clients/macos/vellum-assistant/Features/Settings/SettingsConnectTab.swift index b2123fa9a9e..5c5d9fffa72 100644 --- a/clients/macos/vellum-assistant/Features/Settings/SettingsConnectTab.swift +++ b/clients/macos/vellum-assistant/Features/Settings/SettingsConnectTab.swift @@ -1498,28 +1498,32 @@ struct SettingsConnectTab: View { private func guardianInstructionSubtext(channel: String) -> String { if channel == "telegram" { let handle = store.telegramBotUsername.map { "@\($0)" } ?? "your bot" - return "Message \(handle) with the below command within the next 10 minutes" + return "Message \(handle) with the below code within the next 10 minutes" } else if channel == "voice" { let number = store.twilioPhoneNumber ?? "your assistant" return "Call \(number) and say the six-digit code below within the next 10 minutes" } else { let number = store.twilioPhoneNumber ?? "your assistant" - return "Text \(number) with the below command within the next 10 minutes" + return "Text \(number) with the below code within the next 10 minutes" } } - /// Extracts the `/guardian_verify ` command from a raw instruction string. + /// Extracts a guardian verification code from a raw instruction string. + /// Supports two formats: + /// 1. Legacy `/guardian_verify ` command + /// 2. "six-digit code: 123456" (used by all channels now) private func extractGuardianCommand(from instruction: String) -> String? { - guard let range = instruction.range(of: #"`?/guardian_verify\s+[0-9a-fA-F]+`?"#, options: .regularExpression) else { - return nil + // Try legacy /guardian_verify command format first + if let range = instruction.range(of: #"`?/guardian_verify\s+[0-9a-fA-F]+`?"#, options: .regularExpression) { + return String(instruction[range]).trimmingCharacters(in: CharacterSet(charactersIn: "`")) } - return String(instruction[range]).trimmingCharacters(in: CharacterSet(charactersIn: "`")) + // Fall back to six-digit code format (all channels now use 6-digit numeric codes) + return extractSixDigitCode(from: instruction) } - /// Extracts a six-digit verification code from voice-style instruction text. - /// Voice instructions use a format like "...six-digit code: 123456..." instead of the - /// `/guardian_verify ` command used by Telegram and SMS channels. - private func extractVoiceGuardianCode(from instruction: String) -> String? { + /// Extracts a six-digit verification code from instruction text. + /// Matches the format "six-digit code: 123456" used by all channels. + private func extractSixDigitCode(from instruction: String) -> String? { guard let range = instruction.range(of: #"six-digit code:\s*(\d{6})"#, options: .regularExpression) else { return nil } @@ -1533,11 +1537,10 @@ struct SettingsConnectTab: View { @ViewBuilder private func guardianInstructionView(channel: String, instruction: String) -> some View { - // Voice uses a different instruction format ("six-digit code: 123456") vs - // Telegram/SMS which use "/guardian_verify ". - let command: String? = channel == "voice" - ? extractVoiceGuardianCode(from: instruction) - : extractGuardianCommand(from: instruction) + // All channels now use 6-digit numeric codes. extractGuardianCommand + // handles both the legacy /guardian_verify format and the new + // "six-digit code: 123456" format. + let command: String? = extractGuardianCommand(from: instruction) let isCopied = guardianCommandCopiedChannel == channel VStack(alignment: .leading, spacing: VSpacing.sm) {