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
85 changes: 63 additions & 22 deletions apps/teams-bot/src/__tests__/cards.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,30 +92,24 @@ describe('buildResponseCard', () => {
expect(actions[1].data.action).toBe('need_more_help');
});

// The card no longer accepts a displayId at all, so the leak-carrying input
// has to arrive through a field that still exists. `responseText` is the
// pipeline's own output, which is exactly where a stray identifier could
// come from in practice.
// There is deliberately NO "responseText carries no TKT-" guard here. The card
// has no displayId input any more, so the only way an identifier reaches it is
// through `responseText` — the pipeline's own output, which this card renders
// verbatim by contract. A guard fed a TKT- responseText would fail correctly
// and a guard fed clean text cannot fail at all, so what is worth pinning is
// that the BUILDER never adds an identifier of its own: that is the assertion
// below, over the whole serialized card, across both body shapes.
it.each([
['high confidence', 0.9],
['low confidence (extra disclaimer block)', 0.5],
])('renders no identifier-shaped string anywhere visible — %s', (_label, confidence) => {
const card = buildResponseCard({
responseText: 'Here is the answer.',
confidence,
});

expect(visibleCardText(card)).not.toMatch(/TKT-/);
});

it('carries nothing but the action name in its submit payloads', () => {
])('carries nothing but the action name in its submit payloads — %s', (_label, confidence) => {
// `data` ships to the reporter's client too. It used to carry the ticket
// displayId on the claim that clicks needed it to resolve; card-actions.ts
// ignores `data` entirely and resolves by conversation id, so the field
// was dead payload. This pins it staying gone.
const card = buildResponseCard({
responseText: 'Here is the answer.',
confidence: 0.9,
confidence,
});

const actions = card.actions as Array<{ data: Record<string, unknown> }>;
Expand Down Expand Up @@ -153,6 +147,7 @@ describe('buildTicketCreatedCard', () => {
it('builds a ticket acknowledgment card', () => {
const card = buildTicketCreatedCard({
title: 'Help with integration',
aiJobEnqueued: true,
});

expect(card.type).toBe('AdaptiveCard');
Expand All @@ -167,22 +162,68 @@ describe('buildTicketCreatedCard', () => {
// property today is a no-op; the day the builder reads it, this fails.
const card = buildTicketCreatedCard({
title: 'Help with integration',
aiJobEnqueued: true,
ticketDisplayId: 'TKT-LEAK01',
} as TicketCreatedCardOptions);

expect(visibleCardText(card)).not.toContain('TKT-LEAK01');
expect(visibleCardText(card)).not.toMatch(/TKT-/);
// The bare token too: `title` is Markdown-escaped now, so a rendered
// 'TKT-LEAK01' would arrive as 'TKT\\-LEAK01' and slip past the two
// assertions above.
expect(visibleCardText(card)).not.toContain('LEAK01');
});

it('echoes the caller-supplied title verbatim', () => {
// Documented boundary: `title` is the reporter's own message text
// (handlers/message.ts passes truncate(message.content)), so it is
// rendered as-is. Callers must never put an internal displayId here —
// this builder does not sanitize, and this test pins that contract.
const card = buildTicketCreatedCard({ title: 'my ref is TKT-USERTYPED' });
it.each([
[
'a link',
'[click here](https://phish.example)',
'\\[click here\\]\\(https://phish.example\\)',
],
['bold', 'this is **urgent**', 'this is \\*\\*urgent\\*\\*'],
['a heading', '# ship it', '\\# ship it'],
['a code span', 'run `rm -rf /`', 'run \\`rm \\-rf /\\`'],
['a bullet list', '- one\n- two', '\\- one\n\\- two'],
[
'an image',
'![](https://tracker.example/x.png)',
'\\!\\[\\]\\(https://tracker.example/x.png\\)',
],
])('escapes reporter-supplied Markdown in the title — %s', (_label, title, expected) => {
// A TextBlock renders its text as Markdown in Teams and there is no flag
// to turn that off, so unescaped reporter text made the reporter the
// author of markup inside the bot's own card — a link they typed became a
// real, bot-endorsed link. Escaped, it renders as the literal characters.
const card = buildTicketCreatedCard({ title, aiJobEnqueued: true });

const body = card.body as Array<{ text: string }>;
expect(body[1].text).toBe(expected);
});

it('leaves text with no Markdown characters untouched', () => {
// Escaping must not tax ordinary sentences with stray backslashes.
const card = buildTicketCreatedCard({
title: 'CopilotKit runtime returns 500 on Next 15',
aiJobEnqueued: true,
});

const body = card.body as Array<{ text: string }>;
expect(body[1].text).toBe('CopilotKit runtime returns 500 on Next 15');
});

it('escapes the title without hiding an identifier the caller put there', () => {
// The contract changed only for Markdown: this builder now owns
// neutralizing reporter-controlled markup. It still does not sanitize in
// any other sense — escaping does not remove a displayId — so callers
// remain responsible for keeping internal identifiers out of `title`.
const card = buildTicketCreatedCard({
title: 'my ref is TKT-USERTYPED',
aiJobEnqueued: true,
});

const body = card.body as Array<{ text: string }>;
expect(body[1].text).toBe('my ref is TKT-USERTYPED');
expect(body[1].text).toBe('my ref is TKT\\-USERTYPED');
expect(body[1].text).toContain('my ref is');
});
});

Expand Down
48 changes: 43 additions & 5 deletions apps/teams-bot/src/__tests__/message.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -151,19 +151,57 @@ describe('handleMessage', () => {
const context = makeContext();
await handleMessage(context);

// ConversationReference is stored via a separate update after ticket creation
expect(prisma.ticket.update).toHaveBeenCalledWith({
where: { id: 'ticket-internal-id' },
data: {
// Written in the ticket insert itself, not by a follow-up update.
expect(prisma.ticket.create).toHaveBeenCalledWith({
data: expect.objectContaining({
additionalInfo: {
conversationReference: expect.objectContaining({
serviceUrl: 'https://smba.trafficmanager.net/teams/',
conversationId: 'conv-123',
botId: 'bot-id',
}),
},
},
}),
});
expect(prisma.ticket.update).not.toHaveBeenCalled();
});

it('makes the ConversationReference durable before the AI job is enqueued', async () => {
// The worker claims an AI_RESPONSE job as soon as its row exists and reads
// the serviceUrl off ticket.additionalInfo. Writing the reference after the
// enqueue raced that read, and the worker fell back to the hardcoded global
// serviceUrl — the wrong host for tenants in other regions, so delivery
// failed. Ordering, not just presence, is the fix.
const context = makeContext();
await handleMessage(context);

const createCall = vi.mocked(prisma.ticket.create).mock.calls[0][0] as {
data: Record<string, unknown>;
};
expect(createCall.data.additionalInfo).toBeDefined();

const referenceWriteOrder = vi.mocked(prisma.ticket.create).mock.invocationCallOrder[0];
const enqueueOrder = vi.mocked(createJob).mock.invocationCallOrder[0];
expect(referenceWriteOrder).toBeLessThan(enqueueOrder);
});

it('never attaches a ConversationReference to a ticket for a reply', async () => {
// Replies are appended to an existing ticket, and the orphaned-reply
// fallback files a ticket for a conversation Outpost was never part of.
// Neither may claim a thread for proactive messaging.
vi.mocked(prisma.ticket.findFirst).mockResolvedValue(null);

const context = makeContext({
replyToId: 'missing-parent-id',
text: 'thanks, that worked!',
});
await handleMessage(context);

const createCall = vi.mocked(prisma.ticket.create).mock.calls[0][0] as {
data: Record<string, unknown>;
};
expect(createCall.data.additionalInfo).toBeUndefined();
expect(prisma.ticket.update).not.toHaveBeenCalled();
});

it('ignores messages in unmonitored channels', async () => {
Expand Down
47 changes: 44 additions & 3 deletions apps/teams-bot/src/cards/ticket-created-card.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,54 @@
export interface TicketCreatedCardOptions {
title: string;
/** Whether an AI_RESPONSE job was actually queued for this ticket. */
aiJobEnqueued: boolean;
}

/**
* Markdown-active characters in the subset Teams renders inside an Adaptive Card
* `TextBlock`: emphasis (`*`, `_`), code (backtick), strikethrough (`~`), links
* and images (`[`, `]`, `(`, `)`, `!`), headings (`#`), block quotes (`>`),
* bullets (`-`, `+`), table pipes (`|`), and the escape character itself (`\`).
*
* Everything here is escapable punctuation in CommonMark, so a backslash-escaped
* copy renders as the literal character — the reporter sees exactly what they
* typed, and none of it is interpreted.
*/
const MARKDOWN_ACTIVE = /[\\`*_~[\]()#>|!+-]/g;

/**
* Neutralize Markdown in reporter-supplied text.
*
* A TextBlock always renders its `text` as Markdown in Teams — there is no flag
* to turn that off — so reporter text placed there is reporter-controlled markup:
* `[click here](https://phish.example)` in a Teams message became a real link in
* the bot's own acknowledgment card, borrowing the bot's credibility.
*
* Escaping is the fix rather than stripping (which silently mangles legitimate
* text like `**important**` or a path with underscores) and rather than moving
* the text into a `RichTextBlock`/`TextRun` (structurally non-Markdown, but it
* relies on the client honoring that distinction, and if Teams ever rendered
* Markdown there the injection would be back with nothing catching it).
*/
function escapeMarkdown(text: string): string {
return text.replace(MARKDOWN_ACTIVE, (char) => `\\${char}`);
}

/**
* Build an Adaptive Card acknowledging ticket creation.
*
* Deliberately carries no ticket displayId. That identifier is internal — it
* belongs in the dashboard and team slash commands, not in reporter-facing copy.
*
* `title` is the reporter's own message text (handlers/message.ts passes
* truncate(message.content, 200)), so it is Markdown-escaped here rather than
* rendered verbatim: this builder owns keeping reporter-controlled markup from
* being interpreted by the Teams renderer. It does NOT sanitize in any other
* sense — escaping does not hide an identifier, so callers still own keeping
* internal displayIds out of this field.
*/
export function buildTicketCreatedCard(options: TicketCreatedCardOptions): Record<string, unknown> {
const { title } = options;
const { title, aiJobEnqueued } = options;

return {
type: 'AdaptiveCard',
Expand All @@ -24,13 +63,15 @@ export function buildTicketCreatedCard(options: TicketCreatedCardOptions): Recor
},
{
type: 'TextBlock',
text: title,
text: escapeMarkdown(title),
wrap: true,
isSubtle: true,
},
{
type: 'TextBlock',
text: 'Our AI assistant is reviewing your question...',
text: aiJobEnqueued
? 'Our AI assistant is reviewing your question...'
: 'A team member will review your question and follow up.',
wrap: true,
},
],
Expand Down
38 changes: 24 additions & 14 deletions apps/teams-bot/src/handlers/message.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,30 @@ export async function handleMessage(context: TurnContext): Promise<void> {

if (!isMonitored) return;

// Teams-specific ConversationReference for proactive messaging.
//
// Handed to the inbound handler instead of written afterwards: handle()
// enqueues the AI_RESPONSE job, and the worker can claim that job the
// moment the row exists. A reference written after handle() returned was
// therefore racing the worker, which read the ticket without one and fell
// back to the hardcoded global serviceUrl below — wrong host for tenants
// in other regions, so delivery failed. Passing it in makes it durable in
// the same insert as the ticket, before any job can be claimed.
//
// The handler ignores it on replies, including the orphaned-reply
// fallback: that ticket belongs to a conversation Outpost was never part
// of, and storing a reference for it would claim the thread for proactive
// messaging (see the isOrphanedReply branch below).
const conversationReference = {
serviceUrl: activity.serviceUrl ?? 'https://smba.trafficmanager.net/teams/',
conversationId: activity.conversation.id,
botId: activity.recipient.id,
};

// Delegate to the shared inbound handler
const result = await inboundHandler.handle(message);
const result = await inboundHandler.handle(message, {
ticketAdditionalInfo: { conversationReference },
});

// An orphaned reply also reports isNewTicket: true — a ticket really was
// created — but it is NOT a conversation Outpost opened. Teams sets
Expand All @@ -72,19 +94,6 @@ export async function handleMessage(context: TurnContext): Promise<void> {
// would claim that thread for proactive messaging. Both are skipped; the
// ticket still exists for a human to pick up from the dashboard.
if (result.isNewTicket && !result.isOrphanedReply) {
// Store Teams-specific ConversationReference for proactive messaging
const conversationReference = {
serviceUrl: activity.serviceUrl ?? 'https://smba.trafficmanager.net/teams/',
conversationId: activity.conversation.id,
botId: activity.recipient.id,
};
await prisma.ticket.update({
where: { id: result.ticketId },
data: {
additionalInfo: { conversationReference },
},
});

// New ticket: post acknowledgment card.
//
// Teams is deliberately the only platform that still acknowledges.
Expand All @@ -101,6 +110,7 @@ export async function handleMessage(context: TurnContext): Promise<void> {
// other platforms did.
const card = buildTicketCreatedCard({
title: truncate(message.content, 200),
aiJobEnqueued: result.aiJobEnqueued,
});

const reply = MessageFactory.attachment(
Expand Down
4 changes: 4 additions & 0 deletions apps/worker/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
* - TRACKER_SYNC: Push changes to external trackers
* - JOB_CLEANUP: Periodic cleanup of old jobs and sync events
* - GITHUB_REACTION_POLL: Poll GitHub reactions on AI comments (no webhook exists)
* - PENDING_RESPONSE_SWEEP: Settle AI responses stranded in PENDING by a dead job
*/

import http from 'node:http';
Expand All @@ -32,6 +33,7 @@ import {
createTrackerSyncHandler,
handleJobCleanup,
handleGithubReactionPoll,
handlePendingResponseSweep,
} from '@copilotkit/outpost/queue';
import { buildSyncEngine } from './build-sync-engine.js';

Expand Down Expand Up @@ -69,6 +71,7 @@ const worker = new Worker({
[JobType.TRACKER_SYNC]: 1,
[JobType.JOB_CLEANUP]: 1,
[JobType.GITHUB_REACTION_POLL]: 1,
[JobType.PENDING_RESPONSE_SWEEP]: 1,
},
jobTimeouts: {
[JobType.AI_RESPONSE]: 120_000, // 2 minutes — AI pipeline is slow
Expand All @@ -88,6 +91,7 @@ worker.on(JobType.HUBSPOT_SYNC, handleHubSpotSync);
worker.on(JobType.TRACKER_SYNC, handleTrackerSync);
worker.on(JobType.JOB_CLEANUP, handleJobCleanup);
worker.on(JobType.GITHUB_REACTION_POLL, handleGithubReactionPoll);
worker.on(JobType.PENDING_RESPONSE_SWEEP, handlePendingResponseSweep);

// ─── Start Scheduler ──────────────────────────────────────────────────────

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
-- Add a nullable idempotency slot rather than constraining historical BOT
-- messages. PostgreSQL permits multiple NULLs in a unique index, so existing
-- rows remain valid while new primary AI responses claim one slot per ticket.
ALTER TABLE "Message" ADD COLUMN "responseKey" TEXT;

CREATE UNIQUE INDEX "Message_ticketId_responseKey_key"
ON "Message"("ticketId", "responseKey");
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
CREATE TYPE "MessageResponseState" AS ENUM ('PENDING', 'DELIVERED', 'ESCALATED');

ALTER TABLE "Message"
ADD COLUMN "responseState" "MessageResponseState",
ADD COLUMN "responseJobId" TEXT,
ADD COLUMN "responseError" TEXT;
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
-- Move the two PENDING sub-states of a primary AI response out of the free-text
-- responseError column, which an operations surface reads as "what went wrong".
--
-- Purely additive: both columns are new, deliveryConfirmed carries a DEFAULT so
-- existing rows are correct without a backfill (no historical row was ever
-- delivery-confirmed), and escalationRequiredReason is NULL for every existing
-- row, which is exactly "no escalation is owed". No unique index is created.
ALTER TABLE "Message"
ADD COLUMN "deliveryConfirmed" BOOLEAN NOT NULL DEFAULT false,
ADD COLUMN "escalationRequiredReason" TEXT;
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
-- Index the delivery-state columns PENDING_RESPONSE_SWEEP scans.
--
-- The sweep runs on a fixed schedule and selects primary AI responses that are
-- still PENDING and older than its stranded threshold, ordered by createdAt.
-- Unindexed, that is a full sequential scan of "Message" — the fastest-growing
-- table in the schema — every few minutes. Leading with "responseState" makes the
-- scan proportional to the small set of in-flight responses instead of to message
-- history; "createdAt" second serves both the age cutoff and the ordering.
--
-- Purely additive: creates an index only, no column or data changes.
CREATE INDEX "Message_responseState_createdAt_idx" ON "Message"("responseState", "createdAt");
Loading