From 6b01d03a73f06e5e0acf9463fbf880fd17804cf8 Mon Sep 17 00:00:00 2001 From: Jerel John Velarde Date: Thu, 20 Aug 2026 09:09:00 -0700 Subject: [PATCH 1/4] fix(shared): make SHADOW_MODE fail closed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the third item in #157, the one that is safety rather than quality. Shadow mode exists so Outpost can run alongside an incumbent without double-posting at real reporters. Every call site tested it with `process.env.SHADOW_MODE === 'true'`, so `SHADOW_MODE=TRUE`, `=1` and `=yes` all read as "not shadow mode" and posted to Discord and GitHub for real. A safety flag failing OPEN on inputs an operator would reasonably expect to work. Now one shared `isShadowMode()` that fails closed: recognized off values are off, recognized on values are on, and anything else that is SET is treated as ON and logged with the value it rejected. The asymmetry is the point — a false positive costs a parallel-run window where nothing posts and someone notices from the logs; a false negative posts machine-generated text at real people under the flag meant to prevent that. Unset still means off. Shadow mode is opt-in, and defaulting an absent variable to ON would make a fresh deployment silently answer nobody. The fail-closed rule applies to values that are set: those are an operator trying to say something, and the safe reading of an unclear instruction is the one that posts nothing. Central rather than per-call-site, as #157 suggested. Three copies of this predicate existed and all three had the same bug, so the discord-bot one now re-exports the shared version instead of reimplementing it — three copies is why this survived. 24 tests, all nine previously-fail-open spellings among them. Reverting the body to `=== 'true'` fails 12. Package total 1075 → 1099, which is exactly the new file; discord-bot unchanged at 66. One thing worth knowing for the next reader: two suites mock `@copilotkit/outpost/shared` wholesale, so the mock had to grow an `isShadowMode` entry. It delegates to the same env check rather than returning a constant — a hardcoded `false` would have left those files' existing SHADOW_MODE-based tests asserting nothing. Refs #157. --- apps/discord-bot/src/lib/shadow-mode.ts | 14 ++-- docker-compose.override.yml | 4 ++ .../queue/src/__tests__/ai-response.test.ts | 11 +++ .../src/__tests__/onboarding-digest.test.ts | 9 +++ .../outpost/queue/src/handlers/ai-response.ts | 4 +- .../queue/src/handlers/onboarding-digest.ts | 4 +- .../shared/src/__tests__/shadow-mode.test.ts | 68 +++++++++++++++++++ packages/outpost/shared/src/index.ts | 1 + packages/outpost/shared/src/shadow-mode.ts | 53 +++++++++++++++ 9 files changed, 156 insertions(+), 12 deletions(-) create mode 100644 docker-compose.override.yml create mode 100644 packages/outpost/shared/src/__tests__/shadow-mode.test.ts create mode 100644 packages/outpost/shared/src/shadow-mode.ts diff --git a/apps/discord-bot/src/lib/shadow-mode.ts b/apps/discord-bot/src/lib/shadow-mode.ts index 06495249..7236a4d8 100644 --- a/apps/discord-bot/src/lib/shadow-mode.ts +++ b/apps/discord-bot/src/lib/shadow-mode.ts @@ -13,9 +13,10 @@ import type { ThreadChannel, Message } from 'discord.js'; * * This enables a parallel-run validation period before full cutover. */ -export function isShadowMode(): boolean { - return process.env.SHADOW_MODE === 'true'; -} +// Re-exported rather than reimplemented. Three copies of this predicate existed +// and all three compared `=== 'true'`; a shared one is the only version of this +// that stays fixed. +export { isShadowMode } from '@copilotkit/outpost/shared'; export interface ShadowResponse { ticketId: string; @@ -48,7 +49,7 @@ export async function logShadowResponse(response: ShadowResponse): Promise console.log( `[Shadow Mode] Logged response for ticket ${response.ticketId} ` + - `(${response.responseTimeMs}ms)`, + `(${response.responseTimeMs}ms)`, ); } @@ -115,10 +116,7 @@ export async function handleShadowThreadCreate( return ticket.id; } catch (error) { - console.error( - `[Shadow Mode] Failed to create ticket for thread ${thread.id}:`, - error, - ); + console.error(`[Shadow Mode] Failed to create ticket for thread ${thread.id}:`, error); return null; } } diff --git a/docker-compose.override.yml b/docker-compose.override.yml new file mode 100644 index 00000000..326cf0e2 --- /dev/null +++ b/docker-compose.override.yml @@ -0,0 +1,4 @@ +services: + postgres: + ports: !override + - '5437:5432' diff --git a/packages/outpost/queue/src/__tests__/ai-response.test.ts b/packages/outpost/queue/src/__tests__/ai-response.test.ts index 8c5f849c..ae94a387 100644 --- a/packages/outpost/queue/src/__tests__/ai-response.test.ts +++ b/packages/outpost/queue/src/__tests__/ai-response.test.ts @@ -73,6 +73,17 @@ const mockGetAdapter = vi.fn().mockReturnValue({ }); vi.mock('@copilotkit/outpost/shared', () => ({ + // The handler under test reads shadow mode through the shared helper now, so + // the mock has to provide it. Delegating to the real env check keeps the + // existing SHADOW_MODE-based tests in this file meaningful — a hardcoded + // `false` would make them assert nothing. + isShadowMode: () => { + const raw = process.env.SHADOW_MODE; + if (raw === undefined) return false; + const v = raw.trim().toLowerCase(); + if (['false', '0', 'no', 'off', ''].includes(v)) return false; + return true; + }, AI_CONFIDENCE: { AUTO_RESPOND: 0.9, HIGH_THRESHOLD: 0.8, diff --git a/packages/outpost/queue/src/__tests__/onboarding-digest.test.ts b/packages/outpost/queue/src/__tests__/onboarding-digest.test.ts index 5a439b22..6f359f23 100644 --- a/packages/outpost/queue/src/__tests__/onboarding-digest.test.ts +++ b/packages/outpost/queue/src/__tests__/onboarding-digest.test.ts @@ -12,6 +12,15 @@ vi.mock('@copilotkit/outpost/db', () => ({ })); vi.mock('@copilotkit/outpost/shared', () => ({ + // The handler reads shadow mode through the shared helper now, so the mock + // has to provide it. Delegating to the real env check keeps this file's + // SHADOW_MODE tests meaningful — a hardcoded false would assert nothing. + isShadowMode: () => { + const raw = process.env.SHADOW_MODE; + if (raw === undefined) return false; + const v = raw.trim().toLowerCase(); + return !['false', '0', 'no', 'off', ''].includes(v); + }, computeFunnelMetrics: vi.fn().mockReturnValue({ stageCounts: { JOINED: 3, CONTACTED: 2, RESPONDED: 1, MEETING_BOOKED: 0 }, conversionRates: { diff --git a/packages/outpost/queue/src/handlers/ai-response.ts b/packages/outpost/queue/src/handlers/ai-response.ts index c79f6005..06457dc4 100644 --- a/packages/outpost/queue/src/handlers/ai-response.ts +++ b/packages/outpost/queue/src/handlers/ai-response.ts @@ -37,7 +37,7 @@ import { prisma } from '@copilotkit/outpost/db'; import { AIPipeline } from '@copilotkit/outpost/ai'; -import { AI_CONFIDENCE, MAX_JOB_ATTEMPTS } from '@copilotkit/outpost/shared'; +import { AI_CONFIDENCE, MAX_JOB_ATTEMPTS, isShadowMode } from '@copilotkit/outpost/shared'; import type { PlatformTarget, TicketSource } from '@copilotkit/outpost/shared'; import { hasAdapter, getAdapter } from '@copilotkit/outpost/shared/platforms'; import { getFeedbackCalibration } from '../feedback-calibration.js'; @@ -821,7 +821,7 @@ export async function handleAiResponse( } let responseDelivered = false; - if (process.env.SHADOW_MODE === 'true') { + if (isShadowMode()) { try { await prisma.message.create({ data: { diff --git a/packages/outpost/queue/src/handlers/onboarding-digest.ts b/packages/outpost/queue/src/handlers/onboarding-digest.ts index f3ceea2d..0c7ba83d 100644 --- a/packages/outpost/queue/src/handlers/onboarding-digest.ts +++ b/packages/outpost/queue/src/handlers/onboarding-digest.ts @@ -9,7 +9,7 @@ */ import { prisma } from '@copilotkit/outpost/db'; -import { computeFunnelMetrics } from '@copilotkit/outpost/shared'; +import { computeFunnelMetrics, isShadowMode } from '@copilotkit/outpost/shared'; import type { OnboardingMember } from '@copilotkit/outpost/shared'; import type { OnboardingDigestPayload, JobResult, JobHandlerContext } from '../types.js'; @@ -116,7 +116,7 @@ export async function handleOnboardingDigest( // Deliver the digest const channelId = process.env.DISCORD_DIGEST_CHANNEL_ID; - if (process.env.SHADOW_MODE === 'true') { + if (isShadowMode()) { // Shadow mode (staging): log the digest instead of posting it, so a // staging worker never delivers to a real Discord channel. console.log(`[Onboarding Digest] Shadow mode — skipping Discord post:\n${digest}`); diff --git a/packages/outpost/shared/src/__tests__/shadow-mode.test.ts b/packages/outpost/shared/src/__tests__/shadow-mode.test.ts new file mode 100644 index 00000000..3c46449b --- /dev/null +++ b/packages/outpost/shared/src/__tests__/shadow-mode.test.ts @@ -0,0 +1,68 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { isShadowMode } from '../shadow-mode.js'; + +describe('isShadowMode', () => { + let original: string | undefined; + let warn: ReturnType; + + beforeEach(() => { + original = process.env.SHADOW_MODE; + warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + }); + + afterEach(() => { + if (original === undefined) delete process.env.SHADOW_MODE; + else process.env.SHADOW_MODE = original; + warn.mockRestore(); + }); + + const set = (value: string | undefined) => { + if (value === undefined) delete process.env.SHADOW_MODE; + else process.env.SHADOW_MODE = value; + }; + + it('is off when the variable is not set', () => { + set(undefined); + expect(isShadowMode()).toBe(false); + // Shadow mode is opt-in: defaulting an absent variable to ON would make a + // fresh deployment silently answer nobody. + expect(warn).not.toHaveBeenCalled(); + }); + + // The regression this module exists for. Every one of these read as + // "not shadow mode" under `=== 'true'` and posted to real Discord and GitHub + // surfaces — a safety flag failing open on values an operator would + // reasonably expect to work. + it.each(['TRUE', 'True', '1', 'yes', 'YES', 'on', 'ON', ' true ', 'tRuE'])( + 'treats %j as ON', + (value) => { + set(value); + expect(isShadowMode()).toBe(true); + }, + ); + + it.each(['false', 'FALSE', '0', 'no', 'off', '', ' '])('treats %j as OFF', (value) => { + set(value); + expect(isShadowMode()).toBe(false); + }); + + // Anything set but unrecognized is an operator trying to say something. The + // safe reading of an unclear instruction is the one that posts nothing. + it.each(['maybe', 'shadow', 'enabled', '2', 'null', 'undefined'])( + 'fails CLOSED on the unrecognized value %j, and says so', + (value) => { + set(value); + expect(isShadowMode()).toBe(true); + expect(warn).toHaveBeenCalledTimes(1); + expect(String(warn.mock.calls[0][0])).toContain(JSON.stringify(value)); + }, + ); + + it('does not warn on values it recognizes either way', () => { + set('true'); + expect(isShadowMode()).toBe(true); + set('false'); + expect(isShadowMode()).toBe(false); + expect(warn).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/outpost/shared/src/index.ts b/packages/outpost/shared/src/index.ts index 7f2394f6..0318e52f 100644 --- a/packages/outpost/shared/src/index.ts +++ b/packages/outpost/shared/src/index.ts @@ -1,6 +1,7 @@ export * from './types.js'; export * from './constants.js'; export * from './utils.js'; +export * from './shadow-mode.js'; export * from './dispatch/index.js'; export * from './sla/index.js'; export * from './onboarding/index.js'; diff --git a/packages/outpost/shared/src/shadow-mode.ts b/packages/outpost/shared/src/shadow-mode.ts new file mode 100644 index 00000000..e4627f6b --- /dev/null +++ b/packages/outpost/shared/src/shadow-mode.ts @@ -0,0 +1,53 @@ +/** + * Whether Outpost is running in shadow mode. + * + * Shadow mode is a safety flag: when it is on, AI responses are generated and + * recorded but never posted to a real community surface. It exists so Outpost + * can run in parallel with an incumbent without double-posting at reporters. + * + * The comparison is the whole point of this module. Every call site used to be + * `process.env.SHADOW_MODE === 'true'`, which means `SHADOW_MODE=TRUE`, + * `SHADOW_MODE=1` and `SHADOW_MODE=yes` all read as "not shadow mode" and post + * to Discord and GitHub for real. That is a safety flag failing OPEN on inputs + * an operator would reasonably expect to work — the direction a safety flag must + * never fail. + * + * So this fails CLOSED instead: anything that looks like an attempt to turn + * shadow mode on turns it on, and anything unrecognized is treated as ON and + * logged. The asymmetry is deliberate. A false positive costs a parallel-run + * window where nothing is posted and someone notices from the logs; a false + * negative posts machine-generated text at real people under a flag that was + * meant to prevent exactly that. + */ + +/** Values that mean "off". Everything else that is set means "on". */ +const EXPLICITLY_OFF = new Set(['false', '0', 'no', 'off', '']); + +/** Values that mean "on" without comment. Others are honored but logged. */ +const EXPLICITLY_ON = new Set(['true', '1', 'yes', 'on']); + +/** + * True when shadow mode is engaged. + * + * Unset means off — shadow mode is opt-in, and defaulting an absent variable to + * ON would make a fresh deployment silently answer nobody. The fail-closed rule + * applies to values that ARE set: those are an operator trying to say something, + * and the safe reading of an unclear instruction is the one that posts nothing. + */ +export function isShadowMode(): boolean { + const raw = process.env.SHADOW_MODE; + if (raw === undefined) return false; + + const normalized = raw.trim().toLowerCase(); + if (EXPLICITLY_OFF.has(normalized)) return false; + if (EXPLICITLY_ON.has(normalized)) return true; + + // Set to something we do not recognize. Treated as ON, and said out loud — + // silently guessing either way is how the original bug survived. + console.warn( + `[Shadow Mode] SHADOW_MODE is set to an unrecognized value ${JSON.stringify(raw)}; ` + + 'treating it as ON so nothing is posted to a real community surface. ' + + 'Use true/false (or 1/0, yes/no, on/off) to be explicit.', + ); + return true; +} From 44ea658e14dfcce11d6bff34b2a48ade89d7ccc1 Mon Sep 17 00:00:00 2001 From: Jerel John Velarde Date: Wed, 9 Sep 2026 06:29:37 -0700 Subject: [PATCH 2/4] style: prettier the files this branch touches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #250 added a format check over the files a PR touches, and three of these were already unformatted on main. Cosmetic only — collapsed single-item type re-exports onto one line and wrapped long template literals. Verified: 1099 tests passing, unchanged from before. --- .../src/__tests__/onboarding-digest.test.ts | 27 +++++++------------ .../queue/src/handlers/onboarding-digest.ts | 16 ++++++++--- packages/outpost/shared/src/index.ts | 13 +++------ 3 files changed, 25 insertions(+), 31 deletions(-) diff --git a/packages/outpost/queue/src/__tests__/onboarding-digest.test.ts b/packages/outpost/queue/src/__tests__/onboarding-digest.test.ts index 6f359f23..af71e856 100644 --- a/packages/outpost/queue/src/__tests__/onboarding-digest.test.ts +++ b/packages/outpost/queue/src/__tests__/onboarding-digest.test.ts @@ -93,7 +93,7 @@ describe('handleOnboardingDigest', () => { it('queries members for the given date range', async () => { mockOnboardingMember.findMany - .mockResolvedValueOnce([makeMemberRow()]) // date-filtered query + .mockResolvedValueOnce([makeMemberRow()]) // date-filtered query .mockResolvedValueOnce([makeMemberRow()]); // all-members query for metrics const ctx = makeContext(); @@ -111,8 +111,8 @@ describe('handleOnboardingDigest', () => { it('handles zero new members gracefully', async () => { mockOnboardingMember.findMany - .mockResolvedValueOnce([]) // no members for the day - .mockResolvedValueOnce([]); // no members overall + .mockResolvedValueOnce([]) // no members for the day + .mockResolvedValueOnce([]); // no members overall const ctx = makeContext(); const result = await handleOnboardingDigest({ date: '2026-04-15' }, ctx); @@ -127,9 +127,7 @@ describe('handleOnboardingDigest', () => { const ctx = makeContext(); await handleOnboardingDigest({ date: '2026-04-15' }, ctx); - const progressCalls = ctx.reportProgress.mock.calls.map( - (c: number[]) => c[0], - ); + const progressCalls = ctx.reportProgress.mock.calls.map((c: number[]) => c[0]); expect(progressCalls).toEqual([10, 50, 70, 90, 100]); }); @@ -151,9 +149,7 @@ describe('handleOnboardingDigest', () => { makeMemberRow({ id: 'om-3', username: 'charlie#9012' }), ]; - mockOnboardingMember.findMany - .mockResolvedValueOnce(members) - .mockResolvedValueOnce(members); + mockOnboardingMember.findMany.mockResolvedValueOnce(members).mockResolvedValueOnce(members); const ctx = makeContext(); const result = await handleOnboardingDigest({ date: '2026-04-15' }, ctx); @@ -230,9 +226,7 @@ describe('handleOnboardingDigest', () => { expect(result.success).toBe(true); expect(mockFetch).not.toHaveBeenCalled(); - expect(consoleSpy).toHaveBeenCalledWith( - expect.stringContaining('Shadow mode'), - ); + expect(consoleSpy).toHaveBeenCalledWith(expect.stringContaining('Shadow mode')); consoleSpy.mockRestore(); vi.unstubAllGlobals(); @@ -266,9 +260,7 @@ describe('handleOnboardingDigest', () => { process.env.DISCORD_TOKEN = 'test-bot-token'; process.env.DISCORD_DIGEST_CHANNEL_ID = '1234567890'; - mockOnboardingMember.findMany - .mockResolvedValueOnce([]) - .mockResolvedValueOnce([]); + mockOnboardingMember.findMany.mockResolvedValueOnce([]).mockResolvedValueOnce([]); const mockFetch = vi.fn().mockResolvedValue({ ok: false, @@ -278,8 +270,9 @@ describe('handleOnboardingDigest', () => { vi.stubGlobal('fetch', mockFetch); const ctx = makeContext(); - await expect(handleOnboardingDigest({ date: '2026-04-15' }, ctx)) - .rejects.toThrow('Discord API error 403'); + await expect(handleOnboardingDigest({ date: '2026-04-15' }, ctx)).rejects.toThrow( + 'Discord API error 403', + ); vi.unstubAllGlobals(); }); diff --git a/packages/outpost/queue/src/handlers/onboarding-digest.ts b/packages/outpost/queue/src/handlers/onboarding-digest.ts index 0c7ba83d..3d45e070 100644 --- a/packages/outpost/queue/src/handlers/onboarding-digest.ts +++ b/packages/outpost/queue/src/handlers/onboarding-digest.ts @@ -106,9 +106,15 @@ export async function handleOnboardingDigest( digestLines.push(''); digestLines.push('Funnel Summary (all time):'); digestLines.push(` Joined: ${metrics.stageCounts.JOINED}`); - digestLines.push(` Contacted: ${metrics.stageCounts.CONTACTED} (${metrics.conversionRates.joinedToContacted}%)`); - digestLines.push(` Responded: ${metrics.stageCounts.RESPONDED} (${metrics.conversionRates.contactedToResponded}%)`); - digestLines.push(` Meeting Booked: ${metrics.stageCounts.MEETING_BOOKED} (${metrics.conversionRates.respondedToMeetingBooked}%)`); + digestLines.push( + ` Contacted: ${metrics.stageCounts.CONTACTED} (${metrics.conversionRates.joinedToContacted}%)`, + ); + digestLines.push( + ` Responded: ${metrics.stageCounts.RESPONDED} (${metrics.conversionRates.contactedToResponded}%)`, + ); + digestLines.push( + ` Meeting Booked: ${metrics.stageCounts.MEETING_BOOKED} (${metrics.conversionRates.respondedToMeetingBooked}%)`, + ); const digest = digestLines.join('\n'); @@ -124,7 +130,9 @@ export async function handleOnboardingDigest( await postToDiscord(channelId, digest); } else { // Development fallback when DISCORD_DIGEST_CHANNEL_ID is not set - console.log(`[Onboarding Digest] DISCORD_DIGEST_CHANNEL_ID not set, logging to console:\n${digest}`); + console.log( + `[Onboarding Digest] DISCORD_DIGEST_CHANNEL_ID not set, logging to console:\n${digest}`, + ); } await context.reportProgress(100); diff --git a/packages/outpost/shared/src/index.ts b/packages/outpost/shared/src/index.ts index 0318e52f..8f9cad14 100644 --- a/packages/outpost/shared/src/index.ts +++ b/packages/outpost/shared/src/index.ts @@ -34,13 +34,8 @@ export type { HandleOptions, InboundPrismaLike, } from './platforms/index.js'; -export type { - PlatformDiscordAdapterConfig, -} from './platforms/index.js'; -export type { - PlatformGitHubAdapterConfig, - GitHubOctokitLike, -} from './platforms/index.js'; +export type { PlatformDiscordAdapterConfig } from './platforms/index.js'; +export type { PlatformGitHubAdapterConfig, GitHubOctokitLike } from './platforms/index.js'; export type { PlatformSlackAdapterConfig, SlackAdapterConfig, @@ -52,6 +47,4 @@ export type { TeamsActivity, TeamsConversationReference, } from './platforms/index.js'; -export type { - PlatformEmailPostmarkAdapterConfig, -} from './platforms/index.js'; +export type { PlatformEmailPostmarkAdapterConfig } from './platforms/index.js'; From d4fdae0f26dae38c62217f0bf736da807f547d4e Mon Sep 17 00:00:00 2001 From: Jerel John Velarde Date: Wed, 9 Sep 2026 06:43:25 -0700 Subject: [PATCH 3/4] =?UTF-8?q?fix(shared):=20address=20review=20=E2=80=94?= =?UTF-8?q?=20fence=20the=20call=20sites,=20stop=20re-implementing=20the?= =?UTF-8?q?=20predicate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nathan's review on #233. The predicate was right; almost nothing outside shadow-mode.test.ts would have noticed if it stopped being right. - Fence the call sites. Every shadow test in both queue suites used SHADOW_MODE='true', the one spelling that reads the same before and after this change, so reverting isShadowMode to === 'true' left both suites green. Adds an it.each over the spellings that used to post for real, to each handler. - Replace both hand-rolled vi.mock predicates with partial mocks. The diff deleted three copies of the comparison and the mocks added two back, already drifted (no EXPLICITLY_ON, no warn). calculateBackoff stays overridden: the real one adds Math.random() jitter. - Pin EXPLICITLY_ON via the absence of the warning. On the boolean alone, shrinking it to ['true'] left all 24 tests passing. - Route scripts/cutover/execute-cutover.ts through the helper. Fourth copy of the comparison; the sweep missed it. - '' no longer counts as off. A cleared variable is set, so it takes the fail-closed path and warns rather than silently posting for real. - Log the resolved mode at worker boot. Not a throw — that is a real behaviour change and belongs with the startup-assertion work. - Scope the header's fail-closed claim to values that ARE set. - Drop docker-compose.override.yml and gitignore it: ports: !override replaced the base list, so postgres stopped publishing 5432 and the README setup path failed on a fresh clone. - docs/deployment.md now says to gate on isShadowMode(), which is the instruction that produced the three copies. 1234 tests passing, up from 1222. --- .gitignore | 6 ++ apps/worker/src/index.ts | 23 +++++++ docker-compose.override.yml | 4 -- docs/deployment.md | 7 ++- .../queue/src/__tests__/ai-response.test.ts | 60 ++++++++++++------- .../src/__tests__/onboarding-digest.test.ts | 52 ++++++++++++---- .../shared/src/__tests__/shadow-mode.test.ts | 23 ++++++- packages/outpost/shared/src/shadow-mode.ts | 24 ++++++-- scripts/cutover/execute-cutover.ts | 29 ++++----- 9 files changed, 168 insertions(+), 60 deletions(-) delete mode 100644 docker-compose.override.yml diff --git a/.gitignore b/.gitignore index d87c787a..f987d6b7 100644 --- a/.gitignore +++ b/.gitignore @@ -70,3 +70,9 @@ yarn.lock # Local AI review pipeline session state (create-review / review-changes) .chalk/ + +# Compose auto-loads this exact filename and it is per-developer by convention. +# One was committed by accident: `ports: !override` REPLACES the base list, so +# postgres stopped publishing 5432 and the README's setup path (db:push against +# localhost:5432) failed on a fresh clone. +docker-compose.override.yml diff --git a/apps/worker/src/index.ts b/apps/worker/src/index.ts index 1eb3cb74..d49ca0e9 100644 --- a/apps/worker/src/index.ts +++ b/apps/worker/src/index.ts @@ -35,8 +35,31 @@ import { handleGithubReactionPoll, handlePendingResponseSweep, } from '@copilotkit/outpost/queue'; +import { isShadowMode } from '@copilotkit/outpost/shared'; import { buildSyncEngine } from './build-sync-engine.js'; +// ─── Announce the resolved posting mode ─────────────────────────────────── + +// One line, at boot, next to the other fail-fast-at-boot decision below. +// +// Without it an operator has no way to answer "which mode am I in?" except to +// wait for a job and infer it from which line got printed. That is a bad way to +// learn the answer for the one flag standing between a parallel-run window and +// machine-generated text arriving in a stranger's support thread. +// +// Deliberately a log and not a throw. Throwing when SHADOW_MODE is absent in +// production is a real behaviour change — staging is documented as +// `SHADOW_MODE=true`, so a variable that fails to carry to a new replica is a +// silent fail-open that this log makes visible but does not prevent. That +// stronger version belongs with the startup-assertion work, not here. +console.log( + isShadowMode() + ? '[Worker] SHADOW MODE ON — responses are generated and recorded, never posted.' + : `[Worker] SHADOW MODE OFF — responses WILL post to real community surfaces (SHADOW_MODE=${ + process.env.SHADOW_MODE ?? 'unset' + }).`, +); + // ─── Build SyncEngine for TRACKER_SYNC handler ──────────────────────────── // BOOT SEMANTICS — deliberate change. This is a top-level await that performs diff --git a/docker-compose.override.yml b/docker-compose.override.yml deleted file mode 100644 index 326cf0e2..00000000 --- a/docker-compose.override.yml +++ /dev/null @@ -1,4 +0,0 @@ -services: - postgres: - ports: !override - - '5437:5432' diff --git a/docs/deployment.md b/docs/deployment.md index 1c0fa261..6ff9e8fa 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -209,8 +209,11 @@ worker's gate is the one that covers **every** platform (GitHub, Slack, Teams) p digest job, because that is where the adapter call lives — so a staging environment must have it set on `outpost-worker`, not only on a bot. -When adding any new outbound post path, check `SHADOW_MODE` before posting — otherwise -staging will deliver to real users regardless of the flag. +When adding any new outbound post path, gate it on `isShadowMode()` from +`@copilotkit/outpost/shared` — not on `process.env.SHADOW_MODE` directly. Reading the +variable is what produced three separate copies of `=== 'true'`, all three of which +treated `SHADOW_MODE=TRUE` as "not shadow mode" and posted for real. The helper is the +only version of this that stays fixed. ### Promotion workflow diff --git a/packages/outpost/queue/src/__tests__/ai-response.test.ts b/packages/outpost/queue/src/__tests__/ai-response.test.ts index ae94a387..f94e40b6 100644 --- a/packages/outpost/queue/src/__tests__/ai-response.test.ts +++ b/packages/outpost/queue/src/__tests__/ai-response.test.ts @@ -72,28 +72,17 @@ const mockGetAdapter = vi.fn().mockReturnValue({ fetchUserInfo: vi.fn(), }); -vi.mock('@copilotkit/outpost/shared', () => ({ - // The handler under test reads shadow mode through the shared helper now, so - // the mock has to provide it. Delegating to the real env check keeps the - // existing SHADOW_MODE-based tests in this file meaningful — a hardcoded - // `false` would make them assert nothing. - isShadowMode: () => { - const raw = process.env.SHADOW_MODE; - if (raw === undefined) return false; - const v = raw.trim().toLowerCase(); - if (['false', '0', 'no', 'off', ''].includes(v)) return false; - return true; - }, - AI_CONFIDENCE: { - AUTO_RESPOND: 0.9, - HIGH_THRESHOLD: 0.8, - SUGGEST: 0.7, - MEDIUM_THRESHOLD: 0.5, - ESCALATE: 0.4, - }, - MAX_JOB_ATTEMPTS: 5, - BACKOFF_BASE_MS: 1000, - BACKOFF_MAX_MS: 300_000, +// Partial mock, so `isShadowMode` below is the shipped function rather than a +// re-implementation of it. The four constants that used to be stubbed here were +// checked against `shared/src/constants.ts` and are identical, so the spread +// supplies them. +// +// `calculateBackoff` stays overridden on purpose: the real one +// (`shared/src/utils.ts:40`) adds `Math.random() * BACKOFF_BASE_MS` of jitter, +// so taking it from the spread would make any assertion on a retry delay +// nondeterministic. This override is the deterministic half of it. +vi.mock('@copilotkit/outpost/shared', async (importOriginal) => ({ + ...(await importOriginal()), calculateBackoff: (attempt: number) => 1000 * Math.pow(2, attempt), })); @@ -749,6 +738,33 @@ describe('handleAiResponse', () => { } }); + // The fence, and the one that matters most: this is the handler that reaches + // real Discord and GitHub surfaces. Every other shadow test in this file + // uses `'true'`, the one spelling that read the same before and after the + // fail-closed change — so reverting `isShadowMode` to `=== 'true'` left all + // 145 tests in the two queue suites green. Each of these used to post. + it.each(['1', 'TRUE', 'yes', 'on', ' true ', 'YES'])( + 'skips post-back when SHADOW_MODE=%j', + async (value) => { + const originalShadow = process.env.SHADOW_MODE; + try { + process.env.SHADOW_MODE = value; + mockPrismaTicket.findUnique.mockResolvedValue(sampleTicket); + mockHasAdapter.mockReturnValue(true); + + const result = await handleAiResponse( + { ticketId: 'tkt-1', source: 'discord' }, + makeContext(), + ); + + expect(result.success).toBe(true); + expect(mockPostResponse).not.toHaveBeenCalled(); + } finally { + restoreShadowMode(originalShadow); + } + }, + ); + it('succeeds even if post-back fails (non-fatal)', async () => { mockPrismaTicket.findUnique.mockResolvedValue(sampleTicket); mockHasAdapter.mockReturnValue(true); diff --git a/packages/outpost/queue/src/__tests__/onboarding-digest.test.ts b/packages/outpost/queue/src/__tests__/onboarding-digest.test.ts index af71e856..07ee61a6 100644 --- a/packages/outpost/queue/src/__tests__/onboarding-digest.test.ts +++ b/packages/outpost/queue/src/__tests__/onboarding-digest.test.ts @@ -11,16 +11,14 @@ vi.mock('@copilotkit/outpost/db', () => ({ }, })); -vi.mock('@copilotkit/outpost/shared', () => ({ - // The handler reads shadow mode through the shared helper now, so the mock - // has to provide it. Delegating to the real env check keeps this file's - // SHADOW_MODE tests meaningful — a hardcoded false would assert nothing. - isShadowMode: () => { - const raw = process.env.SHADOW_MODE; - if (raw === undefined) return false; - const v = raw.trim().toLowerCase(); - return !['false', '0', 'no', 'off', ''].includes(v); - }, +// Partial mock: everything real except the one function this file needs to +// pin. The previous version listed its exports explicitly, which meant +// re-implementing `isShadowMode` — so a diff whose whole point was deleting +// three copies of the comparison added a fourth, and it had already drifted +// (no EXPLICITLY_ON, no warn). Spreading the real module means the SHADOW_MODE +// tests below exercise the shipped function instead of a lookalike. +vi.mock('@copilotkit/outpost/shared', async (importOriginal) => ({ + ...(await importOriginal()), computeFunnelMetrics: vi.fn().mockReturnValue({ stageCounts: { JOINED: 3, CONTACTED: 2, RESPONDED: 1, MEETING_BOOKED: 0 }, conversionRates: { @@ -232,6 +230,40 @@ describe('handleOnboardingDigest', () => { vi.unstubAllGlobals(); }); + // The fence. Every other shadow test here uses `'true'`, which is the one + // spelling that behaves identically before and after the fail-closed change + // — so reverting `isShadowMode` to `=== 'true'` left this whole file green. + // These are the spellings that used to post for real. + it.each(['1', 'TRUE', 'yes', 'on', ' true ', 'YES'])( + 'does not post to Discord when SHADOW_MODE=%j', + async (value) => { + process.env.DISCORD_TOKEN = 'test-bot-token'; + process.env.DISCORD_DIGEST_CHANNEL_ID = '1234567890'; + process.env.SHADOW_MODE = value; + + mockOnboardingMember.findMany + .mockResolvedValueOnce([makeMemberRow()]) + .mockResolvedValueOnce([makeMemberRow()]); + + const mockFetch = vi.fn().mockResolvedValue({ + ok: true, + json: async () => ({ id: 'msg-1' }), + }); + vi.stubGlobal('fetch', mockFetch); + const consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + + const ctx = makeContext(); + const result = await handleOnboardingDigest({ date: '2026-04-15' }, ctx); + + expect(result.success).toBe(true); + expect(mockFetch).not.toHaveBeenCalled(); + expect(consoleSpy).toHaveBeenCalledWith(expect.stringContaining('Shadow mode')); + + consoleSpy.mockRestore(); + vi.unstubAllGlobals(); + }, + ); + it('posts to Discord when SHADOW_MODE is explicitly false', async () => { process.env.DISCORD_TOKEN = 'test-bot-token'; process.env.DISCORD_DIGEST_CHANNEL_ID = '1234567890'; diff --git a/packages/outpost/shared/src/__tests__/shadow-mode.test.ts b/packages/outpost/shared/src/__tests__/shadow-mode.test.ts index 3c46449b..1240759a 100644 --- a/packages/outpost/shared/src/__tests__/shadow-mode.test.ts +++ b/packages/outpost/shared/src/__tests__/shadow-mode.test.ts @@ -34,16 +34,35 @@ describe('isShadowMode', () => { // surfaces — a safety flag failing open on values an operator would // reasonably expect to work. it.each(['TRUE', 'True', '1', 'yes', 'YES', 'on', 'ON', ' true ', 'tRuE'])( - 'treats %j as ON', + 'treats %j as ON, recognized rather than guessed', (value) => { set(value); expect(isShadowMode()).toBe(true); + // Asserting the absence of the warning is what pins EXPLICITLY_ON. + // On the boolean alone, shrinking the set to `['true']` left all of + // these passing — a dropped member still comes back `true` through + // the fail-closed branch, just for the wrong reason and with a + // spurious warning. This also pins `.trim()` and `.toLowerCase()`, + // which were each held down by exactly one OFF case. + expect(warn).not.toHaveBeenCalled(); }, ); - it.each(['false', 'FALSE', '0', 'no', 'off', '', ' '])('treats %j as OFF', (value) => { + it.each(['false', 'FALSE', '0', 'no', 'off'])('treats %j as OFF', (value) => { set(value); expect(isShadowMode()).toBe(false); + expect(warn).not.toHaveBeenCalled(); + }); + + // A cleared value is still a value that IS set, so it takes the same + // fail-closed path as any other unclear instruction. A declared-but-empty + // Railway variable, or a `.env` line with nothing after the `=`, used to + // read as "post for real" silently; it now stops the bot and says why. + it.each(['', ' '])('treats the cleared value %j as ON, and says so', (value) => { + set(value); + expect(isShadowMode()).toBe(true); + expect(warn).toHaveBeenCalledTimes(1); + expect(String(warn.mock.calls[0][0])).toContain(JSON.stringify(value)); }); // Anything set but unrecognized is an operator trying to say something. The diff --git a/packages/outpost/shared/src/shadow-mode.ts b/packages/outpost/shared/src/shadow-mode.ts index e4627f6b..cb4ae005 100644 --- a/packages/outpost/shared/src/shadow-mode.ts +++ b/packages/outpost/shared/src/shadow-mode.ts @@ -12,16 +12,32 @@ * an operator would reasonably expect to work — the direction a safety flag must * never fail. * - * So this fails CLOSED instead: anything that looks like an attempt to turn - * shadow mode on turns it on, and anything unrecognized is treated as ON and + * So every value that IS set fails CLOSED instead: recognized off values are + * off, recognized on values are on, and anything else is treated as ON and * logged. The asymmetry is deliberate. A false positive costs a parallel-run * window where nothing is posted and someone notices from the logs; a false * negative posts machine-generated text at real people under a flag that was * meant to prevent exactly that. + * + * `SHADOW_MODE` being ABSENT is the one exception and it means off, because a + * fresh deployment defaulting to ON would silently answer nobody. That + * exception is stated here rather than only on the function, because a reader + * who skims this header and concludes a missing variable is the safe case has + * formed exactly the belief this module exists to kill. */ -/** Values that mean "off". Everything else that is set means "on". */ -const EXPLICITLY_OFF = new Set(['false', '0', 'no', 'off', '']); +/** + * Values that mean "off". Everything else that is set means "on". + * + * `''` is deliberately NOT here. A declared-but-cleared Railway variable, or a + * `.env` line with nothing after the `=`, is a value that IS set — so by this + * module's own rule it is an operator trying to say something unclear, and the + * safe reading of that is the one that posts nothing. It falls through to the + * unrecognized branch, which turns shadow mode on and says so. The cost is a + * cleared variable stopping the bot instead of starting it, which is the side + * of that trade this module exists to take. + */ +const EXPLICITLY_OFF = new Set(['false', '0', 'no', 'off']); /** Values that mean "on" without comment. Others are honored but logged. */ const EXPLICITLY_ON = new Set(['true', '1', 'yes', 'on']); diff --git a/scripts/cutover/execute-cutover.ts b/scripts/cutover/execute-cutover.ts index 4ff2867d..54de5ff9 100644 --- a/scripts/cutover/execute-cutover.ts +++ b/scripts/cutover/execute-cutover.ts @@ -7,6 +7,7 @@ // npx tsx scripts/cutover/execute-cutover.ts --confirm # execute for real import { PrismaClient } from '@prisma/client'; +import { isShadowMode } from '@copilotkit/outpost/shared'; import { validateQuality } from './validate-quality.js'; // ─── Types ────────────────────────────────────────────────────────────────── @@ -40,7 +41,7 @@ function parseArgs(argv: string[]): CliArgs { skipQualityCheck: false, announcement: "We've upgraded our support bot to provide faster, more accurate responses. " + - "If you notice any issues, please let us know!", + 'If you notice any issues, please let us know!', }; for (let i = 2; i < argv.length; i++) { @@ -63,9 +64,7 @@ function parseArgs(argv: string[]): CliArgs { // ─── Step Execution ───────────────────────────────────────────────────────── function logStep(result: StepResult): void { - const icon = result.status === 'PASS' ? '[OK]' - : result.status === 'FAIL' ? '[FAIL]' - : '[SKIP]'; + const icon = result.status === 'PASS' ? '[OK]' : result.status === 'FAIL' ? '[FAIL]' : '[SKIP]'; console.log(` ${icon} ${result.step}: ${result.message}`); } @@ -76,13 +75,8 @@ export async function runHealthChecks(prisma: PrismaClient): Promise await prisma.$queryRaw`SELECT 1`; // Verify required env vars are set - const required = [ - 'DATABASE_URL', - 'DISCORD_TOKEN', - 'DISCORD_CLIENT_ID', - 'GUILD_ID', - ]; - const missing = required.filter(k => !process.env[k]); + const required = ['DATABASE_URL', 'DISCORD_TOKEN', 'DISCORD_CLIENT_ID', 'GUILD_ID']; + const missing = required.filter((k) => !process.env[k]); if (missing.length > 0) { return { step, @@ -168,8 +162,14 @@ export async function disableShadowMode(confirm: boolean): Promise { // In a real deployment, this would call the Railway API to update env vars. // For now, we verify the current state and log the instruction. + // + // Reads through the shared helper, not `!== 'true'`. This was the fourth + // copy of that comparison and the sweep missed it, which meant + // `SHADOW_MODE=TRUE` had the worker correctly withholding posts while this + // step reported shadow mode already disabled and passed — the same + // fail-open, moved onto the cutover path. const currentValue = process.env.SHADOW_MODE; - if (currentValue !== 'true') { + if (!isShadowMode()) { return { step, status: 'PASS', @@ -228,10 +228,7 @@ export async function verifyTicketData(prisma: PrismaClient): Promise { +export async function executeCutover(prisma: PrismaClient, args: CliArgs): Promise { const log: CutoverLog = { startedAt: new Date(), completedAt: null, From 1a4d03596f0f184a2b85ea1ac017258c8e594450 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20=F0=9F=94=B6=20Tarbert?= <66887028+NathanTarbert@users.noreply.github.com> Date: Sun, 13 Sep 2026 21:48:50 -0400 Subject: [PATCH 4/4] fix(scripts): give the cutover suite a way to resolve the shared package Routing execute-cutover.ts through isShadowMode() left scripts/ unable to resolve @copilotkit/outpost/shared: scripts/ is not a workspace package -- pnpm-workspace.yaml lists only apps/* and packages/* -- and pnpm does not hoist, so there was no node_modules/@copilotkit to walk up to. All 15 tests in scripts/__tests__/cutover.test.ts stopped collecting, including the executeCutover test that exercises disableShadowMode, the function this branch changed. CI could not see it. `pnpm test` is `turbo run test`, which runs per-package tasks only, and the root vitest.config.ts is the sole config whose `include` covers scripts/__tests__/**. It is wired to no turbo task, so that suite is green-by-absence and breaks only for someone running vitest at the repo root. Two parts, because the first alone is not enough: - The root manifest now depends on the workspace package. pnpm honours the workspace protocol in the root manifest, which creates the node_modules entry scripts/ resolves through. - The root vitest config aliases the specifier to source. Without it the dependency resolves to shared/dist, so the suite passes only after a build and fails on a clean checkout with the same opaque "Cannot find package" it was failing with before. Verified by deleting shared/dist: aliased, 4 files / 64 tests pass either way. packages/outpost/vitest.config.ts already aliases this same specifier, so this follows the existing pattern rather than inventing one. Measured: 4 files / 64 tests passing, matching origin/main, against 1 failed file / 49 passing before. turbo build/typecheck/lint/test 10/10, and the shadow-mode mutation still fails 29 tests, so nothing here weakened the fence. Wiring scripts/ up as a real workspace package with its own test and typecheck tasks would close the CI blind spot properly. That is its own change. --- package.json | 3 +++ pnpm-lock.yaml | 4 ++++ vitest.config.ts | 19 +++++++++++++++++++ 3 files changed, 26 insertions(+) diff --git a/package.json b/package.json index 317ecb4b..ee148bc9 100644 --- a/package.json +++ b/package.json @@ -15,6 +15,9 @@ "format": "prettier --write \"**/*.{ts,tsx,js,jsx,json,md}\"", "format:check": "prettier --check \"**/*.{ts,tsx,js,jsx,json,md}\"" }, + "dependencies": { + "@copilotkit/outpost": "workspace:*" + }, "devDependencies": { "@eslint/js": "^9.39.4", "@linear/sdk": "^81.0.0", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 1be55189..87059f99 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -7,6 +7,10 @@ settings: importers: .: + dependencies: + '@copilotkit/outpost': + specifier: workspace:* + version: link:packages/outpost devDependencies: '@eslint/js': specifier: ^9.39.4 diff --git a/vitest.config.ts b/vitest.config.ts index 40f3a2b9..b3b2d05f 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -1,8 +1,27 @@ import { defineConfig } from 'vitest/config'; +import path from 'path'; export default defineConfig({ test: { globals: true, include: ['scripts/__tests__/**/*.test.ts'], }, + resolve: { + alias: { + // `scripts/` is not a workspace package, so the root manifest's + // dependency on `@copilotkit/outpost` is what creates a + // node_modules/@copilotkit for it to resolve through. That alone + // resolves to `shared/dist`, which means this suite would pass only + // after a build and fail on a clean checkout with the same opaque + // "Cannot find package" it was failing with before. + // + // Aliasing to source removes the build from the loop, and matches + // what packages/outpost/vitest.config.ts already does for the same + // specifier. + '@copilotkit/outpost/shared': path.resolve( + __dirname, + 'packages/outpost/shared/src/index.ts', + ), + }, + }, });