fix(ai): answer each ticket once, and stop showing reporters internal ticket IDs - #172
Conversation
…rters
Outpost was replying to every message in a support thread. In the Discord
thread 1535447155735789708 a maintainer posted the real solution and the bot
answered 14 seconds later, summarising that answer back at the thread. In
thread 1531971013791711342 the bot answered a maintainer's question to a
community member. The agent is a first line of defence; a human owns the
thread from the first response onward.
The invariant is now: exactly one AI response per ticket, on the message that
opened it, with no follow-up to any later message regardless of sender --
original reporter, third party, or team member. Team-member detection is
irrelevant to this decision and no longer gates it.
Enforced in two places. The AI_RESPONSE handler drops any job for a ticket
that already carries an AI-authored BOT message; because it reads the ticket's
own history rather than trusting its caller, a future enqueue site cannot
reintroduce the behaviour. Reply paths additionally stop enqueuing, so a job
that would be dropped is never paid for:
- packages/outpost/shared/src/platforms/inbound.ts (Discord, Slack, Teams,
and GitHub issue bodies -- status transitions retained)
- apps/github-app/src/webhooks/issue-comment.ts (its own path, which
bypassed InboundHandler entirely)
- apps/web/src/app/api/webhooks/postmark/route.ts (email replies)
- apps/discord-bot/src/lib/shadow-mode.ts (so shadow mirrors production)
Second, internal ticket displayIds no longer reach reporters. The Discord and
Slack "Ticket TKT-XXXXXXXX created" acknowledgment posts are removed outright
-- they published an internal identifier into a public server and spent a bot
message on nothing the reporter could act on. The three Teams Adaptive Cards
drop displayId from their text; buildResponseCard keeps it in the
Action.Submit data payloads so button clicks still resolve to a ticket.
displayId remains in the dashboard, in team slash commands, and in logs.
Call-site enumeration:
- buildTicketCreatedCard (option removed: ticketDisplayId) -- 1 call site,
apps/teams-bot/src/handlers/message.ts:82, updated. Tests updated.
No remaining references to the removed option.
- buildEscalationCard (option removed: ticketDisplayId) -- 1 call site,
apps/teams-bot/src/handlers/card-actions.ts:106, updated. Tests updated.
No remaining references to the removed option.
- buildResponseCard (signature unchanged; body text changed) -- 1 call site,
apps/teams-bot/src/lib/teams-poster.ts:23. Assumption still holds: it
passes ticketDisplayId, which is still consumed, now only for action
routing. PlatformTeamsAdapter has a separate private buildResponseCard
(packages/outpost/shared/src/platforms/teams.ts:269) that never rendered
a displayId -- unaffected.
- InboundResult.aiJobEnqueued (semantics changed: always false on replies)
-- 1 consumer, apps/discord-bot/src/events/message-create.ts:68, which
only appends a log suffix. Assumption still holds; the log is now
accurate rather than misleading.
- handleAiResponse, handleShadowMessage, handleReply -- signatures
unchanged; handleReply is private with no external callers.
Tests: 1778 pass across all 10 packages. 11 existing tests asserted the old
behaviour and were rewritten -- notably the github-app and discord-bot cases
that locked in "answer every follow-up". Six new tests cover the handler
guard, including that a human reply after the AI response still does not
trigger a second answer, that a first response is not swallowed, and that a
SYSTEM shadow-mode log is not mistaken for the ticket's answer. Red-green
verified: neutralising the guard turns them red.
The guard tests mock the AI pipeline at the class seam rather than driving
LLMock, because the assertion they exist to make is that no model call happens
at all -- generateSupportResponse not being called is the direct expression of
that.
Not addressed here: the response template claims "we've escalated this to our
engineering team" without anything enforcing it. Wiring the copy to real
escalation is deferred by request.
The escalation and ticket-created card doc comments carried the literal six-character sequence \u2014 where an em dash was meant; in a comment that renders as raw escape text. response-card.ts already used the real character, so the diff was internally inconsistent. Comment-only; no behavior change. The escape sequences in the card bodies' text: string values are the pre-existing convention and are left as-is. (cherry picked from commit ac6540eaeec0cb16f854fae58bf4ad90b70b24ae)
The doc comment added in e415e31 claimed the internal ticketDisplayId was "routed through the Action.Submit `data` payloads so button clicks resolve back to the ticket". That was false. Both handlers in apps/teams-bot/src/handlers/card-actions.ts take the payload as `_data` (unused) and resolve the ticket via findTicketByConversationId(context.activity.conversation.id). Nothing ever read data.ticketDisplayId, so the identifier was dead payload that still shipped to the reporter's Teams client inside the card JSON. Removed it from ResponseCardOptions, from both Action.Submit `data` payloads, from PostResponseOptions in lib/teams-poster.ts, and from CardActionData (the receiving half of the same dead field — it declared a required string that never arrives). Doc comments now state what is true: the card carries no identifier anywhere, the handlers resolve by conversation id, and this module is currently unreachable in production — nothing imports teams-poster.ts and the worker posts PlatformTeamsAdapter.buildResponseCard from packages/outpost/shared/src/platforms/teams.ts instead. Module kept, just made correct. Call-site enumeration (grep 'ticketDisplayId|buildResponseCard|postAiResponse|ResponseCardOptions|PostResponseOptions' across the repo): - ResponseCardOptions.ticketDisplayId (removed) — only producer was lib/teams-poster.ts, updated in this commit. No other reference. Holds. - buildResponseCard (apps/teams-bot/src/cards/response-card.ts) — two references: lib/teams-poster.ts (updated) and src/__tests__/cards.test.ts (updated). Both now pass only {responseText, confidence}. Holds; tsc --noEmit is clean, which is the proof there is no third caller. - PostResponseOptions.ticketDisplayId (removed) — NEGATIVE FINDING: nothing imports lib/teams-poster.ts at all (no src file, no test). postAiResponse here has zero call sites, so removing the field breaks no caller. - CardActionData.ticketDisplayId (removed) — only read site was the destructure-free `_data` params of handleIssueSolved / handleNeedMoreHelp, both of which ignore it. NEGATIVE FINDING: no surviving card emits the field either — buildEscalationCard and buildTicketCreatedCard define no `actions`, and PlatformTeamsAdapter.buildResponseCard (packages/outpost/shared/src/platforms/teams.ts:269) returns a card with no `actions` array at all, so the production path never sends button payloads. No backward-compat payload is lost. - Same-named symbols in sibling apps are unrelated and untouched: apps/slack-bot/src/lib/slack-poster.ts still uses ticketDisplayId for Slack block_id / action `value` (Slack routes by action value there, a genuinely live use), and apps/github-app/src/lib/github-poster.ts has its own PostResponseOptions with no such field. Neither imports the Teams card module. Assumptions unaffected. - Test fixture apps/teams-bot/src/__tests__/card-actions.test.ts makeCardContext no longer stuffs ticketDisplayId into activity.value, so the suite now proves the handlers resolve the ticket without it. Tests: the old "still routes the displayId through action data so clicks resolve" case encoded the false claim and is replaced by "carries no ticket identifier anywhere — not in text, not in action data", which asserts Object.keys(action.data) === ['action'] for both buttons plus a JSON.stringify catch-all against /[Dd]isplayId|TKT-/. Red-green: re-added ticketDisplayId to the issue_solved payload → new test failed on the action-data key assertion (RED); restored → 55/55 pass (GREEN). `npx vitest run --reporter=dot` and `npx tsc --noEmit` both clean in apps/teams-bot. (cherry picked from commit bf5f70e609ac31c62f26a78b401b3e499e69cdb8)
The guards added with the leak fix could not detect a regression: - allCardText only read top-level body[].text, skipping nested containers and every actions[].title. - The ticket-created and escalation guards were tautological — neither builder accepts a displayId, so no input could have carried a leak. Replace the helper with visibleCardStrings, a recursive walk over the whole card that collects every string except Action.Submit data payloads (a denylist, so an unknown rendered field still trips the guard), and add a meta-test pinning that the walker reaches nested containers and action titles. Feed each guard an input that WOULD leak if the builder passed it through: the response card already receives the displayId (now checked in both the high- and low-confidence body shapes), and the ack / escalation guards hand the builders a ticketDisplayId property they currently ignore. Also pin that title/reason are echoed verbatim, so the caller-owned boundary is documented rather than assumed. Tests only — no production behavior change. (cherry picked from commit 16b38d2639be56cb6dfe599424efb414ec754e94)
The three inbound reply paths each carried their own literal list of the
ticket statuses a customer reply reopens, and they had drifted:
shared InboundHandler WAITING_ON_CUSTOMER, RESOLVED, CLOSED (reference)
github-app issue-comment WAITING_ON_CUSTOMER, RESOLVED (no CLOSED)
postmark webhook RESOLVED, CLOSED (no WAITING_ON_CUSTOMER)
That is now load-bearing: replies no longer enqueue an AI_RESPONSE, so the
reopen is the ONLY signal a customer follow-up sends to a human. A comment on
a CLOSED GitHub ticket reached nobody, and an email reply to a ticket waiting
on the customer stayed out of the queue.
Fix: one exported predicate in @copilotkit/outpost/shared -
REOPEN_ON_CUSTOMER_REPLY_STATUSES + reopensOnCustomerReply(status) - and all
three paths call it. No path keeps a literal status list.
Tests (red-green verified):
- shared: CLOSED reopen case (was uncovered) + a reopensOnCustomerReply unit
block, including an exhaustiveness check over TicketStatus so a new status
cannot be added without deciding whether a reply reopens it.
- github-app: it.each over the three dormant statuses (CLOSED was uncovered)
plus a negative it.each over OPEN/IN_PROGRESS/WAITING_ON_TEAM.
- apps/web postmark: same pair (WAITING_ON_CUSTOMER was uncovered).
- Both app tests now spread importActual over @copilotkit/outpost/shared
instead of stubbing it wholesale, so they exercise the real predicate.
- RED confirmed three ways: reverting the github condition fails 1 github
test; reverting the postmark condition fails 1 web test; dropping CLOSED
from the shared constant fails 3 shared + 1 github + 1 web test. GREEN
restored after each.
Verification: packages/outpost 969 tests / 60 files pass, typecheck clean;
apps/github-app 43 tests pass, tsc --noEmit clean; apps/web 541 tests /
47 files pass, tsc --noEmit clean; eslint clean on the four source files.
Call-Site Enumeration
---------------------
Added: REOPEN_ON_CUSTOMER_REPLY_STATUSES, reopensOnCustomerReply
(packages/outpost/shared/src/constants.ts). Already public: constants.ts is
re-exported wholesale by shared/src/index.ts ("export * from './constants.js'"),
so no new line was needed in the entry point; verified by apps/web and
apps/github-app importing from '@copilotkit/outpost/shared' and typechecking.
References to the two new symbols (every one):
- shared/src/platforms/inbound.ts:16,266 - handleReply. Holds: the value is
the exact set this file used to inline, so the reference-set behaviour is
unchanged; its existing WAITING_ON_CUSTOMER/RESOLVED tests plus a new CLOSED
test pass.
- shared/src/__tests__/platforms-inbound.test.ts:6,621-653 - new unit block.
Holds: asserts the set contents, the negative statuses, null/undefined
safety, and TicketStatus exhaustiveness.
- apps/github-app/src/webhooks/issue-comment.ts:4,89 - non-team commenter
branch. Holds: ticket.status comes off the Prisma row as a string, which is
exactly the predicate's parameter type; the team-member branch above is
untouched (WAITING_ON_TEAM -> WAITING_ON_CUSTOMER stays its own rule).
- apps/web/src/app/api/webhooks/postmark/route.ts:13,89 - existing-ticket
reply branch. Holds: same string status; the "updatedAt: new Date()" in the
update payload is preserved. This path has no team-member detection, so
every inbound email on a ticket is a customer reply by construction -
applying the customer predicate is correct here.
- apps/web/src/__tests__/postmark-webhook.test.ts:28 - comment only.
Removed: three inline status literals. No exported symbol was removed, so no
external caller could depend on them. Confirmed by grep that no literal reopen
set remains - "=== 'RESOLVED'" / "=== 'CLOSED'" now hit only two unrelated
sites.
Negative findings (checked, deliberately NOT converted):
- packages/outpost/shared/src/sla/checker.ts:67 - "CLOSED || RESOLVED" stops
the SLA clock. Different question (is the ticket finished), and it must not
include WAITING_ON_CUSTOMER; left alone.
- apps/web/src/app/api/accounts/[id]/route.ts:43 - "CLOSED || RESOLVED"
counts finished tickets for an account. Same reason; left alone.
- apps/discord-bot, apps/slack-bot, apps/teams-bot - no literal reopen set;
they all reply through the shared InboundHandler, so they pick the fix up
for free. Their existing reopen tests still pass.
- apps/linear-sync/src/webhooks/sync-handler.ts:19 - declares its own
TicketStatus string union but never gates a reopen on it; no change.
- apps/discord-bot/src/lib/shadow-mode.ts:72 and packages/outpost/db/src/seed.ts
- write "status: 'OPEN'" on ticket CREATE, not a reopen; unaffected.
(cherry picked from commit 4006b902c8e565883863b3a11e36be66b54e4266)
Create stored the ticket's sourceId one way and reply looked it up another:
`message.threadId ?? null` on create vs `message.threadId ?? ''` on lookup.
With no threadId the two could never agree, so every message in that
conversation looked like a brand-new ticket and drew its own AI answer --
defeating the one-answer-per-ticket rule this branch enforces. The Slack
composite key was asymmetric the same way (create required BOTH channelId
and threadId; the lookup built "channelId:" from channelId alone), and
apps/slack-bot/src/events/message.ts built a third variant,
"C123:undefined".
Root cause was three-way duplication of the key-building logic, so the fix
is one definition -- shared/src/platforms/source-id.ts's
buildTicketSourceId(source, threadId, channelId) -- that both the writer and
every reader derive the key from. findTicketBySourceAndThread(source,
threadId, channelId) becomes findTicketBySourceId(source, sourceId): the
reader takes the finished key and has no key-building code left to disagree
with.
Deliberate decision for "no threadId": the helper returns null, not a
placeholder. A ticket with no thread key cannot be found again by any
lookup, so writers store null (the ticket is still created -- dropping a
report is worse) and readers treat null as not-lookup-able and skip the
query entirely rather than searching for '' or "C123:", which can only be a
false miss or a false hit on a malformed row. Slack with no channelId is
also null now instead of a bare thread_ts: a thread_ts is only unique within
a channel, and both Slack post paths already threw for those tickets because
ticket.channel is null in exactly that case. Documented in the module's doc
comment along with the "never inline `${channelId}:${threadId}`" rule.
Tests, red-green verified: reverting inbound.ts to the two inline paths gives
5 failed / 37 passed, widened to 8 failed / 38 passed once the unaddressable
rows join the write/read symmetry table; breaking the helper to return
placeholders gives 3 failed / 3 passed in the new unit test; restored, all
green. Covers the previously-untested Slack empty-threadId lookup arm, the
Slack no-channelId arms, and the no-threadId case for a non-Slack source
(create stores null, reply issues no query, and a threadId-less reply is not
matched against a sourceId: null ticket).
Verified: packages/outpost `vitest run` 984 passed / `tsc -p
shared/tsconfig.json --noEmit` clean; slack-bot 52, discord-bot 57,
teams-bot 56, github-app 37 passed; `tsc --noEmit` clean in all four apps.
eslint cannot run repo-wide (no flat eslint.config.*) -- pre-existing.
Out of scope, untouched: the (source, sourceId) uniqueness constraint / any
Prisma migration, and the Slack subtype filter -- both filed as separate
follow-ups.
Call-Site Enumeration
---------------------
ADDED buildTicketSourceId -- grep -rn "buildTicketSourceId" --include="*.ts",
all 7 references:
- shared/platforms/inbound.ts handleNewTicket: holds. Wants the key to store;
null is a valid Ticket.sourceId (nullable column, PrismaLike.create already
types it string|null).
- shared/platforms/inbound.ts handleReply: holds. Wants the key to search for;
narrows null before calling findTicketBySourceId(...: string).
- shared/platforms/index.ts re-export: holds. Value export from a module that
imports only ../types.js, so no adapter runtime is pulled in.
- shared/index.ts re-export: holds. Same reasoning -- the barrel's
"platform types only, browser-safe" contract is preserved; justified inline.
- apps/slack-bot/src/events/message.ts: holds. source is always
TicketSource.SLACK there (SlackAdapter.parseInboundEvent sets it), so the
Slack arm applies; a null key returns early instead of querying.
- apps/slack-bot/src/lib/tickets.ts findTicketByThreadTs: holds. Its callers
(events/actions.ts:22,:74, commands/assign.ts) were written against findFirst
and already treat null as "not found", so an early null is indistinguishable.
- shared/__tests__/platforms-source-id.test.ts: holds -- unit tests of it.
REMOVED findTicketBySourceAndThread -- 2 hits before (definition + its single
call, both in inbound.ts), 0 hits repo-wide after. It was private, so no
external consumer was even representable. Negative finding: nothing dangles.
Near-miss namesake apps/github-app/src/lib/tickets.ts:11 exports a different
findTicketBySourceId(sourceId) -- separate package, not imported here, not
touched, no collision.
CHANGED the sourceId value written on create -- every writer/reader in the repo:
- apps/web/.../webhooks/postmark/route.ts:112 writes body.MessageID: holds.
Not routed through InboundHandler, and the helper is the identity for EMAIL,
so the formats already agree if it ever is.
- apps/discord-bot/src/lib/shadow-mode.ts:76 writes thread.id: holds --
identical to buildTicketSourceId(DISCORD, thread.id).
- apps/discord-bot/src/lib/tickets.ts:10 and apps/teams-bot/src/lib/tickets.ts:10
read the bare threadId/conversationId: holds. Non-Slack keys are verbatim, so
these match what create stores. Left inline deliberately -- for non-Slack the
builder is the identity function, there is no format to duplicate.
- apps/github-app/src/lib/tickets.ts:11,28 read a prebuilt owner/repo#n: holds,
identity for GITHUB_ISSUE/GITHUB_DISCUSSION, unchanged.
- shared/platforms/slack.ts:178 extractThreadTs splits on the first ':': holds.
The composite format is byte-identical; only would-be bare Slack keys became
null, and those tickets already threw on both post paths.
- shared/platforms/github.ts:418 parseSourceId regex: holds, GitHub keys
unchanged. queue/handlers/github-reaction-poll.ts:22,87 likewise, and it
already null-guards ticket.sourceId.
- queue/handlers/ai-response.ts:276 passes ticket.sourceId to the adapter:
holds -- already string|null per PlatformAdapter.postResponse, and adapters
throw loudly on null, the correct outcome for an unaddressable ticket.
- shared/platforms/{discord,teams,email-postmark}.ts post paths: hold. All
already null-guard sourceId and throw a clear error -- no silent misroute.
- TicketSource is still imported in inbound.ts (used by toPlatformTarget), so
the import is not dead; confirmed by clean tsc.
(cherry picked from commit 33fd91ca68ca1387f29d87e30d0a8afec79a626c)
handleReply stopped enqueuing AI_RESPONSE, but on a ticket-lookup MISS it
fell back to `handleNewTicket({ ...message, isThreadStart: true })` — and
handleNewTicket enqueues. A mid-thread reply became a brand-new "ticket"
titled with the follow-up text, and the bot answered it. The one-answer-per-
ticket rule was routed around by the same file that documents it.
Reachability was not theoretical:
- apps/slack-bot/src/events/message.ts pre-filters replies whose thread is
untracked, so Slack was shielded.
- apps/teams-bot/src/handlers/message.ts has NO such pre-filter and its
monitored-channel gate only runs for thread starts, so a Teams reply
reached the fallback.
- Discord and GitHub reached it for any thread predating Outpost.
handleNewTicket now takes an explicit `{ answer: boolean }` decision from its
caller instead of inferring one. handle() passes `{ answer: true }` for a
genuine thread start; the orphaned-reply fallback passes `{ answer: false }`.
No duplication of handleNewTicket's body, and InboundResult.aiJobEnqueued
stays truthful (false on that path).
ASSUMPTION, stated in the code comment so it is reviewable: an orphaned reply
still CREATES a ticket and persists the message — dropping a customer's
message is worse than filing an oddly-titled ticket — but it is never
answered, because the message that opened the real conversation was never
seen by us. A human picks it up from the dashboard.
Also corrects the now-falsified claim in queue/src/handlers/ai-response.ts
that the ticket-history gate makes the invariant "unroutable-around". That
gate only sees messages on the ticket, so a ticket freshly minted around a
mid-thread message has no prior AI response and sails through it. The gate is
a re-answer guard, not a total gate; the orphaned-reply case must be refused
at the enqueue site, and the comment now says so and points at it.
Tests: both pre-existing fallback tests asserted nothing about createJob,
which is exactly why this shipped. Both now assert it, plus new coverage for
Teams/Discord/GitHub/Slack orphaned replies, a non-team-member sender (the
case that WOULD have been answered), and a genuine thread start still being
answered so the fix is not a blanket mute.
Red-green verified: with `{ answer: false }` flipped to `{ answer: true }`,
7 tests fail in packages/outpost (2 files) and 1 in apps/teams-bot; restored,
all green.
Call-Site Enumeration
---------------------
Symbols changed: `InboundHandler.handleNewTicket` (signature — added required
second param `{ answer: boolean }`).
Symbols added: none exported. Symbols removed: none.
`handleNewTicket` — private; grep over the repo (excluding node_modules)
returns exactly three hits, all in shared/src/platforms/inbound.ts:
- :147 the declaration.
- :133 `handle()` thread-start branch → passes `{ answer: true }`.
Assumption holds: isThreadStart=true is the opening message, the one
message Outpost may answer. Behavior byte-identical to before.
- :252 `handleReply` orphan fallback → passes `{ answer: false }`.
Assumption deliberately INVERTED here; that is the fix.
No external caller exists and none can be added by accident: the method is
`private` and is not re-exported from platforms/index.ts or shared/index.ts
(verified — those export the class, InboundHandlerConfig, CreateJobFn only).
`handleReply` — private; hits at :135 (sole caller, the non-thread-start
branch of handle()) and :226 (declaration). Signature unchanged; its return
type and every non-orphan path are untouched. Third hit is a comment
reference in queue/src/handlers/ai-response.ts:100 (prose, no call).
`InboundResult.aiJobEnqueued` — non-test consumers:
- shared/src/platforms/types.ts:165 — the declaration, `boolean`, unchanged.
- apps/discord-bot/src/events/message-create.ts:68 — logs
" (AI job enqueued)" when true. Assumption still holds and is now MORE
accurate: on an orphaned Discord reply the log no longer claims an
enqueue that would have happened. Log text only, no control flow.
NEGATIVE FINDING: no other runtime code reads aiJobEnqueued — not the Slack,
Teams, or GitHub bots, not the queue, not the dashboard. grep for the
identifier over apps/ and packages/ returns only the above plus test files.
So no caller branches on it and no caller can be broken by it flipping to
false on this path.
`isTeamMember` — still called exactly where it was, now guarded by `answer &&`
short-circuit so the lookup is skipped when no answer is possible. NEGATIVE
FINDING: the returned value was used for nothing but the enqueue decision in
handleNewTicket (the reply path calls it separately for its own status
transitions, untouched), so skipping the query cannot change any other
observable behavior. The reply path's own isTeamMember call at :281 is
unaffected.
`handle` (public entry point) — signature and return type unchanged; all bot
call sites (apps/discord-bot thread-create.ts + message-create.ts,
apps/slack-bot events/message.ts, apps/teams-bot handlers/message.ts,
apps/github-app webhooks/issues-opened.ts + discussion-created.ts) compile and
pass unchanged. NEGATIVE FINDING: no bot needed a per-platform change; the fix
is entirely in the shared handler, so all four platforms are covered at once
and no bot can opt out.
`ai-response.ts` change — comments only, zero code touched, so it has no call
sites and no type surface.
Verification
------------
- packages/outpost: `npx vitest run --reporter=dot` → 60 files, 972 tests, all
passing. (`npx prisma generate` was needed first in this fresh worktree;
without it queue/src/__tests__/scheduler.test.ts fails to load on a missing
.prisma/client — pre-existing env setup, not a code failure.)
- `npx tsc --project shared/tsconfig.json --noEmit` → clean.
- apps/teams-bot 57, apps/discord-bot 57, apps/slack-bot 50,
apps/github-app 37 — all passing.
(cherry picked from commit 6f14b00703f954280922af044bc2500337ef41f9)
The one-response-per-ticket guard reads the BOT Message row, which is committed BEFORE the platform post-back. That made two pre-existing soft-failure paths permanent: 1. The `suggestedResponse` ticket.update ran unguarded between the BOT row and post-back. A throw there aborted the job; every retry then hit the guard, returned success with skipped: true, and nothing was posted or escalated. 2. `adapter.postResponse` throwing was logged and the job still reported success. Before the guard a manual re-enqueue could still deliver the answer; after it, that door is closed. Either way the reporter is silent forever while the DB says they were answered. The guard is the requirement, so it is untouched — instead every path where the answer failed to reach the reporter now hands the thread to a human in the same run: - `suggestedResponse` write is non-fatal (logged); it can no longer strand the job before delivery is attempted. - postResponse throwing, and getAdapter throwing, record a deliveryFailure. - deliveryFailure enqueues ESCALATION with a reason naming the failure, and wins the reason slot over suppression / low confidence (most actionable). - If that ESCALATION enqueue also fails, the job returns success: false so the worker records a failed attempt with the reason on the job row — the one outcome with neither delivery nor a human must not look like success. Escalation-enqueue failures for low confidence / suppression keep the historical success result: there the response did reach the reporter. - The externalCommentId write moved out of the post-back try so bookkeeping failure is not misread as delivery failure. - Sources with no adapter escalate only if the suggestedResponse write failed, since there suggestedResponse IS the delivery path. SHADOW_MODE posts nothing by design and never escalates on that basis. Out of scope (filed separately): the guard's check-then-write race under AI_RESPONSE concurrency 4, whose fix is a uniqueness constraint plus a migration. Nothing here narrows or widens that window — no ordering of the guard read or the Message create changed, and no transaction was added. Call-Site Enumeration --------------------- `handleAiResponse` (exported; signature unchanged) - packages/outpost/queue/src/index.ts:4 re-export — holds, same signature. - apps/worker/src/index.ts:82 `worker.on(JobType.AI_RESPONSE, ...)` — holds. The new success: false path is a JobResult the worker already handles at worker.ts:351 via handleFailure (retry ladder + error text persisted on the job row). A retry after that failure is skipped by the guard and returns success, which completes the job; the recorded error text remains on the row, so the operator signal survives. That is intended: the answer is undeliverable, so retrying generation is pointless. - queue/src/__tests__/ai-response.test.ts — updated, all call sites reviewed. - No other callers (grepped `handleAiResponse` across packages/ and apps/). JobResult.data key `escalated` (semantics widened: now also true on delivery failure) - Grepped `escalated` across packages/ and apps/: every hit is unrelated (AI disclaimer copy, escalation handler log lines, bot escalate commands) except ai-response.test.ts. NEGATIVE FINDING: no production reader of AI_RESPONSE's result.data exists — worker.ts only inspects `success` and stores nothing from `data` — so widening it breaks nothing. JobResult.data key `deliveryFailed` (new) - Only readers are the new tests. No dashboard/API code reads AI_RESPONSE job result payloads (grepped `result.data` under queue/src and apps/). ESCALATION payload `reason` (new value shape for the delivery case) - queue/src/handlers/escalation.ts:33,66,108,130 — holds. `reason` is used only as free text: interpolated into the routing reason, the assignment note and a SYSTEM message. No parsing, no enum matching, no length limit. - apps/* escalate commands construct their own reasons; unaffected. New locals (`deliveryFailure`, `escalationEnqueueError`, `suggestedResponseError`, `escalationReason`, `escalated`) and the widened scope of `externalCommentId` - All function-local to handleAiResponse; no exports, no references outside the handler body. `externalCommentId` changed from a `const` inside the post-back try to a `let` in the enclosing block; its only consumer is the message.update below it, which now runs on the delivered path only. Tests ----- Red-green verified for each behaviour: dropping the postResponse deliveryFailure assignment (3 red), restoring the unguarded suggestedResponse write (2 red), disabling the success: false return (1 red), and moving the externalCommentId write back inside the post-back try (1 red). `npx vitest run` — 973 passed (60 files). `npx tsc --project queue/tsconfig.json --noEmit` — clean. (cherry picked from commit 71dd6a9702f1b7811cee7ae2237bfd3233481cb0)
Outpost answers exactly one message per ticket -- the one that opened it
(the invariant the handler already documents and enforces at step 1b).
The question selection contradicted that: it scanned ticket.messages in
REVERSE and took the LATEST type: 'USER' row.
Replies are still persisted as USER messages, and that is correct -- they
belong in the thread's history. The consequence was that a reporter who
split a thought across two Discord messages in the seconds between ticket
creation and the job dequeuing had the ticket's one and only answer aimed
at the follow-up fragment ("btw I'm on the app router") instead of the
question that opened the thread. One shot, spent on the wrong sentence.
ticket.messages is loaded orderBy: { createdAt: 'asc' }, so the fix is a
forward .find() for the first USER row. The ?? ticket.description ??
ticket.title fallback chain is unchanged.
Judgement call -- conversation history stays FULL
------------------------------------------------
conversationHistory still carries every non-SYSTEM message, including any
follow-up that landed after the opening message, even though the QUESTION
is now the opening message. The two inputs answer different questions:
`question` is what to respond to, `conversationHistory` is what the
responder knows. A Discord follow-up is usually the same thought continued
-- a stack trace, a version number, "on Next 15" -- and it is exactly the
detail that makes the single allowed answer good, so truncating history
would trade a targeting bug for a worse answer. Truncation would also
require inventing a second policy for the non-USER rows after the opening,
with no evidence behind it. Consecutive same-role turns are not a new
condition: the generator already appends `question` after the history, so
the base single-message case has always produced two user turns in a row.
Call-Site Enumeration
---------------------
- `latestUserMessage` (removed, function-local const): repo-wide grep over
*.ts/*.tsx/*.md (excluding node_modules and dist) returns ZERO remaining
references. Nothing outside the handler could see it; it never escaped
the function body. Negative finding: no docs or comments named it either.
- `openingUserMessage` (added, function-local const): 1 reference, the very
next line (line 144). Not exported, not part of any type. No other call
site can be affected.
- `question` (unchanged name, changed VALUE): 1 consumer inside the handler
-- `pipeline.generateSupportResponse(question, ...)` at line 175. Its
contract is "a support question as a string"; still a string, still
non-empty via the same fallback chain, so the assumption holds. The
fallback ordering and the SUPPRESSED/groundedness paths downstream are
value-agnostic and unaffected.
- `pipeline.generateSupportResponse` (signature untouched): other callers
are packages/outpost/ai/src/pipeline{,-groundedness}.test.ts (pass their
own literal questions) and apps/web/src/app/api/qa/route.ts (passes the
user's typed QA question, no ticket involved). Negative finding: none of
them reads a Ticket's messages, so none inherits this behaviour change.
- `conversationHistory` (unchanged shape and value): consumed by
AIPipeline.generateSupportResponse -> generator.buildMessages
(ai/src/generator.ts:222). Unchanged, so its assumptions hold by
construction.
- `ticket.messages` ordering assumption: the only producer is the
`orderBy: { createdAt: 'asc' }` include at line 65 of this same file --
same function, no other loader feeds this variable. The step-1b
already-answered guard also uses a forward `.find()` and is unaffected.
Tests
-----
New describe block "answers the message that opened the ticket" in
queue/src/__tests__/ai-response.test.ts: the key case (opening USER +
later USER -> generates against the opening, asserted on
mockGenerateSupportResponse.mock.calls[0][0]), the companion assertion
that the interim follow-up still arrives as conversationHistory, a
leading-SYSTEM-row case, and the no-USER-message description fallback.
The pre-existing "filters SYSTEM messages from conversation history" test
encoded the bug in its expected question ('Follow up question') and now
expects the opening ('Hello'); its history assertion is unchanged, which
is what pins the judgement call above.
Red-green verified: with the source reverted to the reverse().find() form,
3 tests fail (the two new targeting tests plus the corrected SYSTEM-filter
test); restored, 968 tests pass.
Gates: `npx vitest run --reporter=dot` in packages/outpost -> 60 files,
968 tests, all passing. `npx tsc --project queue/tsconfig.json --noEmit`
-> clean (sibling packages built first so the workspace `dist` type
entrypoints resolve). Prettier clean on both touched files.
(cherry picked from commit 297e555ac62bd88b519a7fb90d7825b3965f43f4)
The one-response-per-ticket guard returned right after reportProgress(20), so a job that succeeded by deciding to do nothing persisted progress=20 on its Job row and read as hung to anything watching job progress. Walk the ladder to 100 before returning, like every other successful exit. Failure exits are left as-is on purpose: the Job row carries status FAILED next to the number, so a partial progress value is the honest reading there and 100 would falsely claim completion. (cherry picked from commit a65c2eaef1e088b24719d622468840415bc0a1e0)
The guard's predicate is `m.type === 'BOT' && m.isAiGenerated`, but the tests only pinned the first half. Deleting `&& m.isAiGenerated` left the whole suite green, because no fixture carried a BOT row with isAiGenerated: false — the shape a human reply sent from the dashboard persists as. Two changes, tests only: - Spell out isAiGenerated on every message fixture. The DB column is non-nullable, so rows that omit it are a shape the handler never sees, and the omission let the guard be satisfied by `undefined`. - Add the BOT + isAiGenerated: false case: a teammate's reply goes out over the bot channel but is not Outpost's one answer, so the AI's single response must still be generated and posted. Mutation-verified: dropping `m.type === 'BOT'` fails the SYSTEM shadow-mode-log test; dropping `&& m.isAiGenerated` fails the new human-BOT-reply test (and the SYSTEM-history filter test). Each half is now independently pinned. (cherry picked from commit 35610d3fa08c9c37921508de21f36ecf02b765c4)
The one-response-per-ticket change made replies never enqueue an AI_RESPONSE for any sender, which quietly turned three "team member reply gets no AI response" tests tautological -- they would keep passing with team-member detection deleted outright. Team-member detection still decides aiJobEnqueued on the NEW-TICKET path, so that is where the assertion belongs. - packages/outpost/shared/.../platforms-inbound.test.ts: deleted 'skips AI_RESPONSE for team member reply'. The reply rule is already covered by two explicit tests, and the new-ticket path already has 'skips AI job when sender is a team member' plus the whole 'team member detection' block. A comment records why no team-member reply test lives there. - apps/discord-bot: deleted the reply-path test in message-create.test.ts and repointed it into thread-create.test.ts (discord's new-ticket path), which had no team-member coverage at all. - apps/slack-bot: repointed in place -- the same handler serves both paths, so the test moved from 'threaded replies' to 'new top-level messages'. Red-green verified with `return false` at the top of InboundHandler.isTeamMember: both repointed tests fail, and the three surviving shared-package team-member tests fail with them. Restored -> all green. No production code touched. (cherry picked from commit ad620a45f82648dff0f408a54b7daff186636d46)
The prose around the one-answer invariant made claims the code does not support, and framed the enforcement backwards. Corrections: - inbound.ts `handle()` / module header: path 1 enqueues AI_RESPONSE only when the sender is not a team member, and it is the only path here that enqueues at all. Team status no longer gates the reply path in any way. - ai-response.ts header: the step list omitted step 1b (already-answered gate), step 5b (platform post-back), and the SHADOW_MODE branch that replaces post-back with a SYSTEM message. - ai-response.ts gate comment: "Five separate code paths could enqueue AI_RESPONSE" was both miscounted and wrong about which. Three enqueue sites exist -- InboundHandler.handleNewTicket (Discord, Slack and Teams all funnel through it), handleShadowThreadCreate in the Discord shadow-mode path, and the Postmark new-email branch. - The gate is a RE-ANSWER guard, not the enforcement point, and the comments now say so everywhere they mention it. A ticket freshly minted around a mid-thread reply carries no prior AI response and passes the gate untouched; likewise a reply on a ticket Outpost never answered. The enqueue-site refusals are what actually hold one-answer-per-ticket, so calling them a cost optimisation or "the cheap arm" was backwards. - source-id.ts: a writer/reader key mismatch no longer means "every reply gets its own AI answer" -- an unmatched reply is filed as an untracked ticket with no answer. The real damage is the lost reply and the duplicate stub. - shadow-mode.ts: the enqueue comment said the worker calls logShadowResponse; it writes the shadow row inline and never calls it. The logShadowResponse docstring said NOTE-type; the row is SYSTEM. Also trimmed the "this used to..." narrative, which was repeated in four places, down to the two sites where it genuinely stops the bug being reintroduced. Comments only -- no behavior change, so no test accompanies it. Verified with `npx turbo typecheck` and `npx turbo test` (all green). (cherry picked from commit 69b4c941a8d8ec18e4b70f65c6b214a3c66bf14d)
Call sites audited: - handleMessage dispatch: apps/teams-bot/src/index.ts -> handlers/message.ts - TeamsAdapter.parseInboundEvent: handlers/message.ts; teams-adapter.test.ts; shared platforms-adapters.test.ts - orphan reply path: InboundHandler.handleReply -> handleNewTicket(answer=false) -> Teams isNewTicket acknowledgment branch - policy tests: message.test.ts; teams-adapter.test.ts; inbound-handler.test.ts
Review of #170 caught a regression this branch introduced. The orphaned-reply fallback creates a ticket with `{ answer: false }` so no AI response is enqueued, but it routes through `handleNewTicket`, which returns `isNewTicket: true` unconditionally. The Teams handler branches on exactly that flag to write a conversationReference and post an acknowledgement card. So a mid-thread "thanks, that worked!" -- Teams sets isThreadStart from the absence of replyToId, and no ticket matches -- made the bot post a card into a conversation it was never part of. That is the unwanted-chatter class this branch exists to remove, reintroduced on the platform most reachable for the orphan path. `InboundResult` now carries `isOrphanedReply`, set true only on that fallback, and `handleNewTicket` takes `{ answer, orphanedReply }` as two independent decisions rather than inferring one from the other. The Teams handler gates both the card and the additionalInfo write on it. `isNewTicket` stays true: a ticket really was created, and other consumers may reasonably care. The existing handleMessage orphan test asserted the bug -- it expected sendActivity to have been called once -- so it would have kept this green forever. It now asserts the ticket and message are still persisted while createJob, sendActivity and ticket.update are not called. Verified red twice: reverting the gate fails it, and gating only the card while leaving the additionalInfo write ungated also fails it, so each half is independently covered. Call-site enumeration for isOrphanedReply -- 1 consumer changed, 7 cleared: - apps/teams-bot/src/handlers/message.ts -- CHANGED. The only consumer with a reporter-visible side effect on the new-ticket branch. - apps/slack-bot/src/events/message.ts -- cleared: pre-filters replies whose thread has no ticket, so it cannot reach the fallback. - apps/discord-bot/src/events/message-create.ts -- cleared: same pre-filter. - apps/github-app/src/webhooks/issue-comment.ts -- cleared: returns when no ticket matches, and never uses InboundHandler. - apps/github-app/src/webhooks/issues-opened.ts and discussion-created.ts -- cleared: isThreadStart is always true, and neither has a reporter-visible side effect on that branch. - apps/discord-bot/src/events/thread-create.ts -- cleared: no ack post since this branch removed it. - apps/web -- cleared: never uses InboundHandler; Postmark has its own path and already implements the orphan rule locally. Two further review findings, same shape as the above: Route the last three inlined `Ticket.sourceId` sites through `buildTicketSourceId` -- shadow-mode.ts on the write side, discord-bot and teams-bot lib/tickets.ts on the read side. Identity for both platforms today, so this fixes no live bug; it closes the drift surface the helper exists for, which already produced one write/read mismatch on this branch. Readers return early on an unaddressable key instead of querying `sourceId: null`, which would match unrelated keyless rows. Adds tickets.test.ts for discord-bot, which had none, and stops shadow-mode.test.ts stubbing the whole shared module -- the helper was mocked away, so no test there could have caught a drift. Correct comment prose that overstated what the code does. The literal — escape was in four files, not the two previously fixed -- github-app's issues-opened and discussion-created carried it too, plus a mangled ellipsis. The orphaned-reply rationale in inbound.ts read as a universal principle, but Discord, GitHub and Slack all pre-filter before reaching it and Postmark implements it locally, so Teams is the only caller that arrives there; the comment now says so. The Teams ack card's divergence from Discord and Slack, which post nothing, is recorded as deliberate rather than left to look like an oversight. Not addressed here, both belonging to the email-threading follow-up: postmark/route.ts inlines the EMAIL key as body.MessageID in three places, and a plain reply with no plus-address is still treated as new and answered, because sourceId is the per-email MessageID and there is no In-Reply-To or References fallback.
jerelvelarde
left a comment
There was a problem hiding this comment.
Approving — this is exactly the validated split
Verified against the split I built and tested locally before the plan was written. Every gate matches.
| Gate | Required | Actual |
|---|---|---|
Files under packages/outpost/db/prisma |
0 | 0 |
packages/outpost/queue/src/worker.ts |
untouched | untouched |
Size vs main |
43 files / 2,133 insertions | 43 files / 2,133 insertions / 387 deletions |
PR-touched packages/outpost suites |
130 passed | 130 passed |
| CI | green | green (Lint/Typecheck/Test, zizmor, notify) |
Composition is right. e02f620 (the 13 reviewed commits) with aa95e94 and 9180591 lifted on top — the Teams channel gate and the orphan-reply silence. Nothing else rode along.
No delta leakage. Grepped for every marker of the unreviewed half — responseKey, claimToken, PRIMARY_AI_RESPONSE, responseState, MessageResponseState, findTicketByReplyMessageIds, reclaimStaleJobs. Zero hits across apps/ and packages/. The job queue and schema are genuinely untouched by this PR, which is what makes it independently revertable.
Both review blockers are present and correct. isOrphanedReply lands in platforms/types.ts, platforms/inbound.ts and teams-bot/src/handlers/message.ts — the ack card and the conversationReference write are both gated on it, and the monitored-channel filter now covers replies as well as thread starts.
ae314de also cleared the em-dash escape nit from my original review. The remaining \uD83C sequences are emoji in string literals, consistent with existing code.
Known limitations shipping with this
Both are tracked and both are closed by #170's remainder — noting them so the gap is explicit while this is in production:
- #175 — an undelivered response can flip its job to COMPLETED on retry.
- #176 — AI_RESPONSE can double-post under timeout-retry. Pre-existing in
main; this PR strictly reduces exposure by enqueuing fewer jobs.
Neither is a regression introduced here, and neither touches the bug this fixes.
On the split itself
Thanks for cutting it cleanly and for the rationale comment on #170 — "the two must not be recombined" is the right instinct and worth keeping in the #170 description as it gets reduced.
The commit hygiene is what made this cheap: two cherry-picks and one trivial test conflict. That's a direct dividend of committing in small scoped pieces rather than one blob.
Good to merge on my end.
Fallout from merging main. This test survived the textual merge and then failed, which is the useful kind of conflict: `message-create.test.ts` had no textual conflict at all, so nothing flagged it. It asserted that a genuine reply enqueues AI_RESPONSE. Since #172 and #191, `InboundHandler` never enqueues for a reply on any platform — Outpost answers once per ticket, on the opening message, and a human owns the thread after that. The assertion was written before that rule existed. What the test is actually here to prove is that the `message.id === threadId` gate distinguishes the starter message from a reply, so it now asserts the reply still gets its Message record, plus an explicit check that nothing was enqueued. That pins the one-answer rule on this path too. The gate stays load-bearing: neutering it fails the two starter-message tests.
Split out of #170 so the customer-facing fix can ship on its own. 43 files, 2,133 insertions, zero database changes. The concurrency work, the response state machine, the job-queue changes, and the email header threading all remain in #170 and are not part of this PR.
The rule
Outpost answers exactly one message per ticket — the message that opened it — and never posts in that thread again, regardless of who speaks next. Not the original reporter, not a teammate, not a third party. The agent is a first line of defence; a human owns the thread from the first response onward.
Team-member detection is irrelevant to this decision and no longer gates it.
Why
Two real threads in the CopilotKit Discord.
A maintainer posted the real solution, and the bot repeated it back at them. Thread 1535447155735789708 — fourteen seconds after a maintainer posted the actual answer, the bot posted its own message summarising that same answer. Nothing added, in public, over the top of the person who had already solved it.
The bot answered the maintainer, and said it was confused. Thread 1531971013791711342 — a maintainer asked a community member a follow-up. The bot answered the maintainer, explaining that it "can't see other users' replies."
That second thread also opened with
🎫 Ticket TKT-4HS8SRR3 created— an internal dashboard reference, published into a public server.What changed
Enqueue sites refuse on replies. No reply enqueues an
AI_RESPONSEjob on any platform:shared/platforms/inbound.tshandleReply(Discord, Slack, Teams, GitHub issue bodies),github-app/webhooks/issue-comment.ts(its own path, which bypassedInboundHandlerentirely),web/api/webhooks/postmark/route.ts, anddiscord-bot/lib/shadow-mode.tsso shadow mirrors production. Only new-ticket enqueue sites remain.This is the load-bearing layer, and it is worth being precise about why. An earlier version of this work described the handler-side gate as making the invariant unroutable-around, with the enqueue-site removals as a cost optimisation. That was backwards: a ticket freshly minted around a mid-thread reply carries no prior AI response and passes the gate untouched. The gate prevents re-answering; the enqueue refusals are what enforce one-answer-per-ticket.
A re-answer gate in the handler, reading the ticket's own message history, so a caller added later inherits the rule.
Internal ticket IDs no longer reach reporters. The Discord and Slack
🎫 Ticket TKT-… createdacknowledgment posts are removed outright. All three Teams Adaptive Cards stop rendering adisplayId— it was also dead payload in the response card'sAction.Submitdata, sincecard-actions.tsignoresdataand resolves by conversation id. The Teams ack card no longer claims an AI is reviewing the question when no AI job was enqueued.displayIdstays in the dashboard, team slash commands, and logs.Supporting fixes, each load-bearing for the rule:
inbound.tshandleNewTicket, which enqueues — so a mid-thread reply became a "new ticket" and got answered.handleNewTicketnow takes an explicit{ answer, orphanedReply }, andInboundResult.isOrphanedReplygates the Teams ack card andconversationReferencewrite.inbound.tssourceIdwas written asthreadId ?? nullbut looked up asthreadId ?? '', so a threadId-less message could never match and every message minted a new ticket. Now one sharedbuildTicketSourceIdhelper across every call site.ai-response.tsUSERmessage rather than the opening one, so a follow-up racing the job stole the ticket's single answer.CLOSED, email omittedWAITING_ON_CUSTOMER. Load-bearing because of this change: reopening is now the only signal a human gets. One shared predicate.Tests
130 tests pass across the four suites this PR touches in
packages/outpost; full CI on the PR.Every behavioural fix carries red-green verification — test written, source broken, RED confirmed, restored, GREEN confirmed. Eleven pre-existing tests asserted the old behaviour and were rewritten, including
github-appanddiscord-botcases that actively locked in "answer every follow-up", and a Teams test that asserted the orphan-reply ack card had been sent.Notable test repairs, because they were green while proving nothing:
body[].text, skipping nested containers and action titles. Now a recursive walker fed leak-carrying inputs, five mutations verified RED.&& m.isAiGeneratedfrom the gate predicate left the whole suite green. Both halves are now independently pinned.The gate tests mock the AI pipeline at the class seam rather than driving
LLMock, because the assertion they exist to make is that no model call happens at all —generateSupportResponsenot being called is the direct expression of that.What this does not solve
This is a circuit breaker. It stops the bot speaking twice; it does not teach it to behave correctly. Two follow-ups are filed rather than folded in:
ai-response.tscollapses every non-SYSTEM message to a bare user-or-assistant role fromMessage.type, discardingMessage.author. Maintainer, reporter and stranger are indistinguishable to the model — the direct cause of the second incident above.Relationship to #170
#170 stays open with the remainder:
(ticketId, responseKey)uniqueness, the response state machine, job claim-token fencing, stale-job reclaim, shutdown drain, Postmark idempotency and header threading — plus four database migrations. That half has not had a review pass and has three known findings outstanding, so it is deliberately not in this PR.These two must not be recombined; independent rollback is the point of the split.