feat(slack): mirror GitHub + Discord tickets into one internal Slack channel - #150
NathanTarbert wants to merge 11 commits into
Conversation
…channel Every ticket opens a Slack thread; follow-ups and the AI's reply post underneath it, so one thread is the whole life of one ticket. Read-only in v1 — replying in Slack does not post back to the source. Thread identity reuses TicketExternalLink (plugin `slack`, externalId `channelId:ts`), the same table the Linear and GitHub links use, so the unique(ticketId, plugin) constraint is what prevents a ticket from ever opening two threads. Ships inert. SLACK_MIRROR_MODE defaults to `off` and an unrecognized value fails closed; with no SLACK_MIRROR_CHANNEL_ID the producers never enqueue. The flag is deliberately independent of SHADOW_MODE: that flag protects community surfaces where real reporters are watching, while this targets an internal team channel, so staging posting here is intended rather than a violation of the standing shadow-mode rule. Two correctness details worth calling out: - An AI reply is labelled with whether it actually reached the reporter. Shadow mode, a failed post, and a suppressed (ungrounded) draft all leave an AI Message row that nobody outside saw; mirroring those as if they were delivered would reproduce the divergence #148 describes. A suppressed run counts as undelivered even though a post succeeded, because what went out was the safe replacement copy, not the draft the mirror renders. - Slack-sourced tickets are never mirrored. If the mirror channel were also monitored, each mirror post would arrive as inbound, open a ticket, mirror again, and loop. Mirror failures never touch the reporter's path: enqueue errors are logged and swallowed in both producers.
CR loop — Round 1 findings (12 reviewers, standard mode)Partition: 20 (a) mandatory · 10 (b) ledger-only · 5 (c) pre-existing · 1 (d) other-PR subject. Ledger: Fixes are not yet applied — see the note at the bottom. (a) Code — mandatory
A2/A3/A5/A6 are one root cause — the link row is not treated as the idempotent source of thread identity — so they are being fixed as a single structural change, not four patches. (a) Docs — claims this diff makes that the code contradicts
(a) Tests — defects in the tests added by this PR
(d) Not this PR — but worth a look now
StatusReview round is complete and verified. The fix cycle has not landed: all six fix agents died mid-work (2 stalls, 4 "connection closed mid-response"), leaving the PR branch untouched at |
…dentity The mirror read the link, posted to Slack, then created the link. Two jobs for one ticket (the ticket job and the reply job, at SLACK_MIRROR concurrency 2) could both read "no link", both post a root message, and the loser of @@unique([ticketId, plugin]) died on P2002 — leaving a duplicate Slack thread and a dead-lettered job. Any retry after a successful post did the same. Four related defects, one cause: the link row was not treated as the identity of the thread. - parseThreadTs -> parseThreadRef, returning {channelId, ts} with both halves required. Replies now post to the channel recorded in the link instead of whatever SLACK_MIRROR_CHANNEL_ID currently says, so changing the configured channel no longer detaches every existing ticket's replies. The channel half was being written and never read. - The create is wrapped in a P2002 catch (duck-typed on err.code, so the Prisma runtime stays out of this handler) that re-reads the row and threads under the winner's ts rather than opening a rival thread. - A row whose externalId cannot be parsed is repaired in place via update. It previously re-entered the open-thread path and hit the unique constraint on every attempt, forever. The already-mirrored short-circuit now requires a PARSEABLE link, so a malformed row reaches repair exactly once. - SLACK_MIRROR concurrency 2 -> 1, plus an explicit 60s jobTimeouts entry (a reply can make two postMessage calls through WebClient's rate-limit sleeps, and the 30s default could cut that off mid-flight and duplicate work). Call-site enumeration: - parseThreadTs: removed; sole caller was handleSlackMirror's thread lookup, now calling parseThreadRef. Zero remaining references (grep). - parseThreadRef: new; called only from handleSlackMirror. - SLACK_MIRROR_PLUGIN: unchanged; readers are this handler and the tests. - handleSlackMirror signature unchanged; worker registration at apps/worker/src/index.ts:90 still holds. - JobType.SLACK_MIRROR concurrency/timeout maps: read only by Worker's scheduler in packages/outpost/queue/src/worker.ts; both keys are optional and additive. Not fixed here, still open: message-level reply dedup. A duplicate reply job can still re-post the same message; it can no longer spawn a rival thread. That needs a per-message marker and is tracked separately. Tests: 4 new cases in the handler suite (P2002 recovery, malformed-link repair, reply routes to the link's channel when config differs, duplicate reply opens no thread). Red-green verified — 3 of the 4 fail before this change. Handler suite 20/20, packages/outpost 958 tests, typecheck clean across 10 packages.
Round 1 of the CR loop returned 20 mandatory findings across 12 reviewers. This lands the rest of them (the link-idempotency lever went in as b58ff89). Code - Both producers now route through one allowlist. `isMirrorableSource` is the single place deciding what gets mirrored, and it is an ALLOWLIST (Discord, GitHub issues, GitHub discussions) rather than "anything but SLACK". The rule previously existed only in the inbound producer, so the AI-reply producer mirrored everything — a Slack-sourced ticket's AI reply opened a thread in the mirror channel, and the denylist silently pulled in TEAMS/EMAIL/WEB/MANUAL/ LINEAR tickets the feature was never specified for. - `live` with no SLACK_BOT_TOKEN now reads as DISABLED and logs why once, per process. It used to read as enabled, so every job reached buildPoster, threw, and burned five attempts into the dead-letter queue — one per ticket, forever. - Permanent Slack errors are classified and not retried: not_in_channel, channel_not_found, channel_is_archived, invalid_auth, account_inactive, missing_scope. Each failure message names the remedy. - Reply jobs are validated BEFORE anything is posted. A reply with no messageId used to reach the thread-opening post first, so every retry posted another root message to Slack; same for a messageId naming a row that no longer exists. - The undelivered label states the reason the producer recorded instead of guessing. `delivered: boolean` became `delivery: 'delivered' | 'shadow' | 'withheld' | 'post-failed' | 'no-adapter'`, set at the branch that knows. "withheld or shadow mode" was being printed for five distinct causes, which asserted a cause nobody established — the misreporting this label exists to prevent. An AI reply with no delivery status renders "unconfirmed", never delivered: unknown is not the same as fine. - `source` is now declared on SlackMirrorPayload and sent by both producers. It was riding CreateJobFn's index signature undeclared, which is how the two producers drifted into different payload shapes. - Mirror-enqueue failures log the error class and stack, so schema drift is not swallowed as "a queue hiccup". Queue (additive) - JobResult gains an optional `retryable`. A handler that sets it false is dead-lettered immediately instead of consuming every attempt. Omitting it — which every pre-existing handler does — preserves the old behavior exactly; both directions are covered by tests. Docs — each of these was a claim the diff made that the code contradicted - The loop rationale was wrong. `.env.example` and docs/deployment.md justified keeping the mirror channel unmonitored by saying the bot would read its own mirror posts and open a ticket per post. It cannot: the Slack bot drops events carrying bot_id (apps/slack-bot/src/events/message.ts). The advice stands as defense-in-depth and against duplicated context; the false mechanism is gone. - Scope is stated by naming isMirrorableSource rather than listing sources that drift. - The absolute "check SHADOW_MODE on any new outbound path" rule now carries the mirror's exception where the RULE is stated, with a gating table. - Documented that shadow needs no token, that any mode is inert without a channel ID, and — the one that would have made this ship dead — that the PRODUCERS gate on the same config, so the vars are needed on outpost-discord-bot and outpost-github-app as well as outpost-worker, not the worker alone. Tests (the reviewers found real defects in the ones this PR added) - The mirror suite was nested inside describe('restoreShadowMode') instead of describe('handleAiResponse'), inheriting an unrelated afterEach and duplicating setup. Relocated; the duplicated setup is gone. - `delivered: true` passed only on leftover mockPostResponse state from an earlier suite. It now re-stubs and asserts the post happened. - The suppressed-undelivered case never asserted a post occurred, so it survived the exact regression it targets. It asserts it now. - `toContain('missing')` matched the ticket id, not the field — it asserted nothing. Now matches the field name. - The enum test listed five members and omitted SLACK_MIRROR; only the count had been bumped. - The inbound suite pinned an env-dependent default; the env-derived fallback is now covered explicitly, including that it is OFF when unset. Call-site enumeration - isSlackMirrorEnabled: callers inbound.ts:139, ai-response.ts:296, slack-mirror.ts:124 — all three still hold; the added token requirement only narrows when it returns true. - isMirrorableSource: new; called from inbound.ts:161 and ai-response.ts:296. - CreateJobFn: `source` stays REQUIRED. Narrowing it broke assignability for every bot wrapper (discord-bot/src/events/{message-create,thread-create}.ts declare it required) — verified by typecheck, reverted, field declared on the payload instead. - JobResult.retryable: read only in worker.ts's failure dispatch; optional, so the ten existing handlers are unaffected. - SlackMirrorPayload.delivered -> delivery: producers ai-response.ts:301 and the handler's formatReplyPost were the only readers; zero remaining references to `delivered` (grep). Not fixed, still open: message-level reply dedup (a duplicate reply job re-posts the same message; it cannot spawn a rival thread). Needs a per-message marker. Verified: typecheck 10/10, build 10/10, 1,765 tests pass. Red-green confirmed on the worker's permanent-failure path. Prettier clean on every file whose baseline was clean; worker.ts was already prettier-dirty on main and was left unformatted to avoid unrelated churn.
Confirmation round (8 reviewers) returned a new bucket (a). Several items were defects in the round-1 fixes themselves, which is what the round is for. Introduced by the previous commit, now fixed - The token requirement in `isSlackMirrorEnabled` made the PRODUCERS treat a tokenless `live` as disabled. Since the producers run in the bots and only enqueue, that left the mirror silently dead while the docs said the token belonged on the worker. Split the predicate: `isSlackMirrorEnabled` (mode + channel) gates the producers, `canSlackMirrorPost` additionally requires a token and gates the consumer, which reports a permanent failure naming the variable. The bot token no longer has to be spread to services that never post. - `retryable: false` dead-lettered by presenting the attempt as `maxAttempts`, writing a fabricated attempt count — an exhausted-retry trail for a job that ran once. `handleFailure` now takes an explicit `permanent` flag and records the true count. - The mirror payload forwarded the AI job's optional `source` hint, so it could record `source: undefined` while the inbound producer sent a resolved value. Sends the resolved source now, and the type's comment no longer overclaims. Also fixed - An unrecognized `delivery` value indexed to `undefined` and rendered the literal string "undefined" into Slack. Unknown values fall back to "unconfirmed". - An unknown `kind` fell through both branches: it posted a root message and returned success. Rejected up front as permanent. - "Accepted but no ts" was retryable even though the post had landed, so every retry posted another root message. Now permanent, and says the message was posted but cannot be tracked. - Ticket bodies and replies were interpolated into Slack mrkdwn unescaped, so a reporter on a public tracker could inject links and mentions into an internal channel. Escapes &, <, > per Slack's formatting rules. - Slack error classification preferred a substring scan that was order-dependent; now prefers the API's own code with a word-boundary fallback, and covers token_expired, token_revoked, not_authed, msg_too_long. - An unrecognized SLACK_MIRROR_MODE failed closed silently, indistinguishable from a deliberate `off`. It logs now. - WebClient's default retry policy can sleep for minutes, outliving the 60s job timeout and leaving a post in flight after the worker gave up. Capped to 2 retries with a 2s ceiling. - Corrected comments that overclaimed: the idempotency guarantee covers THREAD identity only (a duplicate reply still re-posts), the text cap is a readability budget rather than a Slack hard limit, and the shadow log now says shadow does not persist thread identity so repeated opens are expected. Tests - Ambient-environment sensitivity, reproduced by a reviewer: 3 tests in the inbound suite failed with SLACK_MIRROR_MODE exported, and 7 in the AI suite failed with SHADOW_MODE=true inherited. Both suites now neutralize and restore the ambient values. - The permanent-failure test now pins `attempts: 1` — the assertion whose absence hid the fabricated count. - New coverage: no-ts branch, unknown kind, live-without-token, mrkdwn escaping, unrecognized delivery, the split producer/consumer predicates, empty channel id. - Fixture corrections: `source: 'GITHUB'` was not a TicketSource value, and the mirror fixture encoded live-with-null-token, a config production rejects. Call-site enumeration - isSlackMirrorEnabled: inbound.ts:139, ai-response.ts:296, slack-mirror.ts:124 — all three still hold; the predicate only widened (token no longer required). - canSlackMirrorPost: new; sole caller slack-mirror.ts, after the enabled check. - resetSlackMirrorWarnings: removed with the warn latch it served; zero remaining references (grep), test import updated. - handleFailure: sole caller is worker.ts's failure dispatch, both arms updated; the new parameter defaults to false so the throw path is unchanged. Still open, tracked: message-level reply dedup (needs a per-message marker); truncation splits surrogate pairs; a new WebClient per job. Verified: typecheck 10/10, 1,774 tests pass, prettier clean on files whose baseline was clean.
Six conflicts. Four were unions where main and this branch each added a job
type — `PENDING_RESPONSE_SWEEP` and `SLACK_MIRROR` — so the enum, payload map,
queue barrel and worker registration take both. Two needed real work.
`handlers/ai-response.ts`: main restructured the delivery and escalation flow
(`responseDelivered`, `deliveryFailure`, `nonDeliveryEscalationReason`,
`requiredEscalationRecorded`), which moved every place the mirror's `delivery`
label was being set. Took main's structure wholesale and re-threaded the label
into it: `delivered`/`withheld` where `responseDelivered` is set after a
successful post, `post-failed` in the post catch alongside `deliveryFailure`,
`shadow` and `no-adapter` unchanged on their own paths.
Worth flagging because a marker-only resolution would have shipped it: git left
`delivery = 'post-failed'` sitting inside main's *escalation-recording* catch,
since both sides happened to have a `} catch (error) {` at that point. That
catch fires when the escalation write fails, which has nothing to do with a post
failing — the mirror would have reported a delivered answer as post-failed.
Removed.
`__tests__/ai-response.test.ts`: both sides appended a whole test suite at the
same place and git interleaved them, so deleting markers would have spliced one
feature's suite into the other's. Rebuilt from the three merge stages instead —
main's 2630-line version plus this branch's four additions (the vitest import
and its ambient-SHADOW_MODE guard, the mutable mirror config, three predicates
on the platforms mock, and the 114-line mirror suite verbatim).
`JobType` count assertion moved 11 → 12, and now names both recently added types
so a future failure says which one is missing rather than just that the number
moved.
63 files / 1145 tests in packages/outpost; discord-bot, worker, slack-bot and
github-app suites all green.
CPK-7936 Rebase PR #150 — Slack ticket mirror (conflicting; must follow PR #191)
PR #150 — Why it must follow #191It shares 8 files with #191 and 6 with #187 — including There is a real design interaction, not just a textual conflict: #150 mirrors AI replies with a delivery reason ( Scope remindersShips inert behind Follow-ups already split out as outpost#152 — a duplicate reply job still re-posts its message, UNBLOCKED 2026-08-20 — #191 merged, so the dependency this ticket recorded is gone. Moved out of Blocked. Current state: #150 is still a draft and still |
|
Merged Six conflicts. Four were straightforward, two were not, and one of those would have shipped a real bug if I'd resolved it by deleting markers. The four unionsMain added
|
| State | Where it lands now |
|---|---|
delivered / withheld |
alongside responseDelivered = true, after a successful post |
post-failed |
in the postResponse catch, next to deliveryFailure = message |
shadow |
unchanged |
no-adapter |
unchanged (still the declared default, which also covers adapter-construction failure) |
The part worth flagging. Because both sides happened to have a } catch (error) { at the same point, git left delivery = 'post-failed' sitting inside main's escalation-recording catch — the one that fires when the escalationRequiredReason write fails. That has nothing to do with a post failing, so a successfully delivered answer whose escalation write failed would have been mirrored as post-failed. Which is precisely the class of misreporting your SlackMirrorDelivery type exists to prevent — the type comment says the reason travels with the payload "instead of being inferred", and this would have inferred wrong. Removed.
__tests__/ai-response.test.ts — rebuilt from the merge stages
Both sides appended a whole test suite at the same location and git interleaved them, so deleting markers would have spliced main's "answers the opening message" suite into the middle of your mirror suite. Your side's first hunk ended mid-object literal, with its continuation stranded in a common region.
So I rebuilt it: main's 2630-line version, plus your four additions applied on top — the vitest import and its ambient-SHADOW_MODE guard, the mutable mockMirrorConfig, the three mirror predicates on the platforms mock, and your 114-line mirror suite lifted in verbatim (inserted at the close of describe('handleAiResponse'), where it was). Nothing of yours was reworded.
Result: 104 tests in that file, all passing, including all five delivery-label cases (delivered, withheld, post-failed, no-adapter, mirror-off).
One count assertion
JobType "has exactly 11 job types" → 12, since both sides added one. It now also names both recently-added types, so the next failure says which is missing instead of just that the number moved.
Verified
packages/outpost 63 files / 1145 tests. apps/discord-bot 8/8, apps/worker 1/1, apps/slack-bot 7/7, apps/github-app 6/6 — all green.
Ready for review. I haven't approved it: the delivery-label re-threading is a judgment call about your feature's semantics, and you should confirm the four states land where you intended — particularly that adapter-construction failure is meant to read as no-adapter rather than getting its own label.
#250's format check covers the files a PR touches, and these four were already unformatted on main. Cosmetic only — re-export lists collapsed or wrapped, long calls broken across lines. Verified: 1145 tests passing in packages/outpost, unchanged.
… orphaned `f25ec6f` wrapped two long signatures in PrismaLike across lines, which moved `args: any` one line further down than its `eslint-disable-next-line`. The directive then covered `create: (` instead, so #250's new Lint step reported two no-explicit-any errors and two unused-directive warnings on a file the formatting was supposed to leave alone. Directives now sit on the `args: any` line itself. Verified against a local merge with main (where eslint.config.mjs exists): `pnpm lint` 10/10, and prettier is still clean on every file this branch touches.
jerelvelarde
left a comment
jerelvelarde
left a comment
There was a problem hiding this comment.
First review this has ever had — it has been open since 1 August with no reviewer requested, which is the reason, not a verdict on it. I read the whole diff, ran a multi-dimension pass (correctness, silent failure, flag/config, tests, merge order) and put every finding through a refutation round. Three of twenty-nine did not survive; what is below did.
The handler itself is the strongest part and I could not break it. Things I verified and that nobody should re-check:
- Thread identity is safe against wrong-thread posts.
parseThreadRefsplits on the first colon and neither a Slack channel ID nor atscontains one, sochannelId:tsround-trips exactly. Replies post tothread.channelIdread off the link row rather thanconfig.channelId, so re-pointingSLACK_MIRROR_CHANNEL_IDcannot orphan an existing ticket's replies. - The P2002 race handling is genuinely careful — the loser re-reads the winner's row and uses its thread rather than retrying a create that can only fail again, and a row whose
externalIdcannot be parsed is repaired in place rather than duplicated. - The delivered/undelivered label is trustworthy at the source.
pipelineResult.responsereally is the withheld draft whileformattedcarries the safe replacement, and neitherDiscordAdapter.postResponsenorGitHubAdapter.postResponsehas a dry-run short-circuit — sodeliveredcannot be fabricated. - Escaping and truncation are ordered correctly (
escapeMrkdwn(truncate(...))), so a cut can never land inside an entity. plugin: 'slack'collides with nothing — the only otherTicketExternalLinkwriters are the Linear sync and the import script.
Three things before this lands.
Blocker 1 — the production switch has no test, and I confirmed it by mutation
inbound.ts:196 — this.mirrorToSlack = config.mirrorToSlack ?? isSlackMirrorEnabled(readSlackMirrorConfig()). That ?? fallback is the only production enable path: I grepped every new InboundHandler(...) in apps/ and none of the six passes mirrorToSlack, so production always takes the env branch.
Nothing tests it. Replacing only the fallback:
this.mirrorToSlack = config.mirrorToSlack ?? false;
leaves 1145 tests passing, 63 files — all green. (Replacing the whole expression kills 5, which is what makes this easy to miss: the switch looks covered.)
So the line that turns the feature on in production is the one line no test exercises, and if it were ever wrong the symptom is an empty Slack channel that looks exactly like "nobody filed anything today". Closing it is one test: set SLACK_MIRROR_MODE=live and SLACK_MIRROR_CHANNEL_ID, construct new InboundHandler({ prisma, createJob }) with no mirrorToSlack, handle a Discord message, assert a SLACK_MIRROR job was enqueued.
Blocker 2 — GitHub follow-ups never reach the mirror, and the PR body says they do
The body promises "community follow-ups and the AI's reply post underneath it, so a single Slack thread is the whole life of one ticket." That holds for Discord only.
apps/github-app/src/webhooks/issue-comment.ts:54 appends its Message row with a bare prisma.message.create and never constructs an InboundHandler, so it never reaches enqueueSlackMirror. And apps/github-app/src/webhooks/ contains exactly three files — discussion-created.ts, issue-comment.ts, issues-opened.ts — so GitHub Discussions have no comment webhook at all.
Concretely: issue #42 opens a ticket, the Slack thread opens, the AI reply threads under it. The reporter then comments "still repros on 1.62.3", "here's a stack trace", "this is blocking our launch". None of it reaches Slack, nothing logs the omission, and an engineer scanning the mirror channel sees a ticket with one AI answer and concludes the reporter went quiet.
Either enqueue the reply job in handleIssueComment behind the same isSlackMirrorEnabled + isMirrorableSource pair, or — given Discussions have no webhook to hook at all — say plainly in the body and docs/deployment.md that v1 carries GitHub ticket-opens and AI replies only, with community follow-ups Discord-only. I do not mind which; I mind that the current text asserts the first.
Blocker 3 — merge order, and it is silent
#262 (open) relocates the entire worker-construction and handler-registration block of apps/worker/src/index.ts into a new startWorker(). This PR adds exactly three lines to that block: [JobType.SLACK_MIRROR]: 1 in concurrencyByType, the jobTimeouts entry, and worker.on(JobType.SLACK_MIRROR, handleSlackMirror).
The merge comes out clean in either direction and the natural resolution drops all three. apps/worker/src/index.ts has no tests, so lint, typecheck and the full suite all stay green while the mirror is silently unregistered — every SLACK_MIRROR job then sits in the queue with no handler.
My suggested order across the four worker PRs is #224 → #262 → #150 → #233, with the three lines re-applied by hand on this branch afterwards, where the mirror tests are in front of whoever does it.
Worth settling
docs/deployment.mdand.env.examplepromise the opposite of whatlive-without-token does. They say it "logs why once"; the handler dead-letters one job per ticket and per reply, and that mirror content is then gone permanently rather than retried once the token appears.- A non-P2002 failure on the link write after a successful Slack post makes the retry open a second thread. The post lands, the row does not, the retry finds no link and posts another root. The unique constraint prevents a second tracked thread, not a second posted one — worth softening the body's "what stops a ticket from ever opening two threads".
- Mirror config lives on the producers but not the worker, so a worker with the mirror off returns success for every mirror job and logs nothing — the
skippedmarker is discarded. A job that did nothing reportsCOMPLETED. - Shadow mode's "posts nothing" contract is tested for the thread-opening post but never for replies, and the reply path's permanent-error catch can be gutted with a green suite.
Nit worth one line
"Deliberately independent of SHADOW_MODE" is a reasonable call for an internal channel and I am not arguing with it — but it is not quite true in practice: the Discord bot bypasses InboundHandler entirely when SHADOW_MODE=true, so neither new producer call site runs in staging. The mirror therefore cannot be exercised on staging the way docs/deployment.md implies. Worth a sentence so nobody plans a shadow validation window around it.
Send blocker 1 (one test) and a decision on blocker 2 and I will re-verify immediately. The handler is ready; it is the seams around it.
`config.mirrorToSlack ?? isSlackMirrorEnabled(readSlackMirrorConfig())` is the only way the mirror turns on in production -- no `new InboundHandler(...)` in apps/ passes the flag, so all six call sites take the env branch. Every existing test in that block passes `mirrorToSlack` explicitly, so the fallback was the one line no test exercised. That is easy to miss rather than careless: replacing the WHOLE expression kills five tests, so the switch looks covered. Replacing only the fallback with `false` left the entire suite green, and the symptom in production would be an empty Slack channel -- indistinguishable from nobody having filed anything. Six cases: live and shadow enable it; unset, explicitly off, an unrecognized mode, and live-without-a-channel all stay off; an explicit `mirrorToSlack: false` still overrides a live environment, so a caller opting out is not overridden by a stray variable. The suite already neutralizes ambient SLACK_MIRROR_* for the whole file, so these set and restore per test rather than relying on it. Mutation-checked: the fallback returning `false` now fails 2; returning `true` fails 15.
The stays-off table carries a third column explaining why each row is a realistic mistake, but the callback took two parameters. Vitest runs it either way; tsc does not, and shared/tsconfig.json compiles test files, so the package build failed.
|
Blocker 1 is done in Blocker 1 — the enable pathYour diagnosis of why it was easy to miss is the useful part: replacing the whole expression kills five tests, so the switch looks covered, and only the fallback is unexercised. Six cases now:
Your exact mutation — fallback only, to I pushed a broken commit first and the follow-up fixes it. The Blocker 2 — my read, but it is your callI would correct the body rather than wire it up, for this release. The wiring is not one call site: So: body and Blocker 3 — merge orderAgreed, and I have said the same on #262. Order #224 → #262 → #150 → #233, with the three The restYour four "worth settling" items and the shadow-mode nit are all fair and none is a one-liner — the dead-letter-vs-"logs why once" contradiction in the docs is the one I would take first, since it is a promise the code does not keep and mirror content is lost permanently rather than retried. Happy to take them in this PR or a follow-up, whichever you prefer. One thing to know before this branch moves: it is 48 commits behind |
… does Two claims the code does not keep. The PR body and these docs read as though a Slack thread is the whole life of a ticket. That holds for Discord. A follow-up comment on a GitHub issue or discussion never reaches the mirror: apps/github-app/src/webhooks/ issue-comment.ts appends its Message row with a bare prisma.message.create and never constructs an InboundHandler, so it never enqueues a mirror job, and Discussions have no comment webhook at all. Nothing logs the omission, which is what makes it worth writing down rather than leaving to be discovered: an engineer scanning the mirror channel sees a ticket with one AI answer and concludes the reporter went quiet. The docs now say the opposite -- check the GitHub thread before concluding anything from a quiet mirror thread. Wiring it is not one call site and Discussions would need a webhook that does not exist, so v1 carries the stated scope and the wiring is filed separately. A stated limit reads correctly; a half-covered promise does not. Second: `live` without SLACK_BOT_TOKEN was documented as reporting itself disabled and logging why once. It does not -- each mirror job dead-letters, one per ticket and one per reply, and that content is gone rather than retried once the token appears. Set the token before setting live. Docs only; no behaviour change.
|
Blocker 2 addressed as the documentation half, blocker 3's prerequisite done, and the stale branch is current. Blocker 2 — stated rather than wired, and filedTook the option I flagged: the text now says what the code does. The part I made sure landed is your point about how it fails, because that is the half a scope note usually omits: nothing logs the omission, so a Slack thread that stops after the AI reply is not evidence the reporter went quiet. The docs now say to check the GitHub thread before concluding anything from a quiet mirror thread. The wiring is #268, split into its two halves — issue comments are a handler change behind the same I also fixed the second doc claim you flagged in the same commit. The branch is currentIt was 62 commits behind and predates #250, so One thing to know about that run: the first Blocker 3 — merge orderAgreed and unchanged: #224 → #262 → #150 → #233. After #262 lands, this branch's three lines ( Still open from your reviewThe four smaller items are untouched and I did not want to bundle them into a docs commit: the second-thread-on-retry after a non-P2002 failure, mirror config living on the producers but not the worker (so a mirror-off worker returns success for every job and discards the Two other follow-ups filed from this session's reviews while I was in the area: #266 (a timed-out handler stays invisible to |
jerelvelarde
left a comment
jerelvelarde
left a comment
There was a problem hiding this comment.
All three settled, and the one that needed proving is proved. Approving.
Blocker 1 — verified with the exact mutation
this.mirrorToSlack = config.mirrorToSlack ?? false — the fallback-only change that previously left all 1145 tests green — now fails 2. Run against head, 1358 passing baseline.
The six cases are the right six, and the one I would not have asked for is the best of them: an explicit mirrorToSlack: false still overrides a live environment. That pins the direction a caller opting out cannot be overridden by a stray variable, which is the failure that would actually hurt someone.
And you pushed a broken commit, then said so. The tsc-vs-vitest split on a two-parameter it.each callback is a genuinely easy one to eat — shared/tsconfig.json compiles test files and vitest does not care. Recording that you read the turbo summary too fast is worth more than the fix; it is the same class as the hasPerTypeLimits test on #262 that exercised the path production never takes.
Blocker 2 — your read is right, and I would have argued for it
Correct the claim, don't half-wire it. An engineer who knows the mirror is Discord-only for follow-ups reads a quiet thread correctly; one who has been told otherwise reads it as the reporter going quiet. A stated limit beats a half-kept promise.
What makes the doc actually good is the sentence I did not ask for:
Nothing logs the omission, so a Slack thread that stops after the AI reply means "no Discord follow-ups", not "the reporter went quiet" — check the GitHub thread itself before concluding anything from the mirror.
That is the failure mode, written where the person who would hit it is looking. Scope notes usually stop at "not supported" and leave the reader to discover how it misleads.
#268 splitting into its two halves is right too, and the line about narrowing this wording if only the first half ships is the part that keeps it honest — a stated limit is worth something only while it stays true.
The live-without-token correction landed in the same commit, which was the "worth settling" item I would have taken first. Dead-lettering one job per ticket and per reply, with the content gone rather than retried, is a materially different promise from "logs why once".
Blocker 3 — agreed, and the branch is current
#224 → #262 → #150 → #233. #262 is approved as of today and waiting behind #224.
Merging main in was the right call independent of the review — 62 commits behind and no flat config meant the Lint gate would have failed whatever I decided. Clean merge, 10/10, 1358 tests.
Two things to carry, neither blocking
The three SLACK_MIRROR registration lines. After #262 lands, apps/worker/src/index.ts moves its worker-construction block into startWorker(), and the natural merge drops the concurrencyByType entry, the jobTimeouts entry and the worker.on(...). Nothing goes red — that file has no tests. Whoever does the re-apply should run the mirror suite immediately after, and it should be the same person, in the same sitting.
The flake you flagged. One failure on the first post-merge pnpm test, green standalone and on two --force runs. Agreed it reads as a turbo race rather than anything from the merge — and thank you for saying so rather than re-running until it was green and moving on. If it surfaces in CI, that note is what stops someone bisecting the merge for an hour.
The remaining "worth settling" items — the reply-path shadow-mode coverage, the mrkdwn escaping sites, the second-thread-on-link-failure window — are fine as a follow-up. None is a one-liner and none blocks a v1 that ships inert.
2 participants
Builds the Slack ticket mirror described on the roadmap. Every ticket from GitHub and Discord opens a thread in one internal Slack channel; community follow-ups and the AI's reply post underneath it, so a single Slack thread is the whole life of one ticket.
Read-only in v1 — replying inside Slack does not post back to the source.
Ships inert
SLACK_MIRROR_MODEdefaults tooff, and an unrecognized value fails closed rather than posting. With noSLACK_MIRROR_CHANNEL_IDthe producers never enqueue, so merging this changes nothing until the Slack app is configured.shadowlogs exactly what a live run would post, and needs no token.The flag is deliberately independent of
SHADOW_MODE. That flag protects community surfaces where real reporters are watching; the mirror targets an internal team channel, so staging posting here is intended rather than a violation of the standing shadow-mode rule. Called out indocs/deployment.mdso it does not read as one.How it works
packages/outpost/queue/src/types.ts(SLACK_MIRROR)packages/outpost/shared/src/platforms/slack-mirror-config.tspackages/outpost/queue/src/handlers/slack-mirror.tspackages/outpost/shared/src/platforms/inbound.tspackages/outpost/queue/src/handlers/ai-response.tsapps/worker/src/index.tsThread identity reuses
TicketExternalLink(pluginslack, externalIdchannelId:ts) — the same table the Linear and GitHub links use. Theunique(ticketId, plugin)constraint is what stops a ticket from ever opening two threads. Areplythat finds no thread opens one first, so enabling the mirror partway through a live conversation does not drop messages.Two correctness details
An AI reply is labelled with whether it actually reached the reporter. Shadow mode, a failed post, and a suppressed (ungrounded) draft each leave an AI
Messagerow that nobody outside ever saw. Mirroring those as if delivered would reproduce exactly the divergence #148 describes between what the DB records and what was published. A suppressed run counts as undelivered even though a post succeeded, because what went out was the pipeline's safe replacement copy, not the draft the mirror renders.Slack-sourced tickets are never mirrored. If the mirror channel were also monitored, each mirror post would arrive as an inbound message, open a ticket, mirror that, and loop. The guard is in the producer;
docs/deployment.mdalso says to keep the mirror channel out ofMONITORED_CHANNEL_IDS, which is the durable fix.Mirror failures never touch the reporter's path — enqueue errors are logged and swallowed in both producers.
Verification
pnpm build,pnpm typecheck— clean across all 10 packagespnpm test— 1,729 passing, 0 failingNot verified: no live Slack workspace was exercised.
chat.postMessageis behind the injectableSlackPosterseam and is faked in tests, so the real API call is the one thing still unproven — worth ashadowrun before flipping tolive.Follow-ups not in scope
Replying from Slack back to the source, per-source channel routing, and backfilling threads for tickets that predate the mirror.