diff --git a/backend/__tests__/unit/services/onboardingMarks.test.js b/backend/__tests__/unit/services/onboardingMarks.test.js index 6e1ff2b195..21e63bc251 100644 --- a/backend/__tests__/unit/services/onboardingMarks.test.js +++ b/backend/__tests__/unit/services/onboardingMarks.test.js @@ -194,14 +194,48 @@ describe('BYO setup marks', () => { 'onboarding.byoCommandType': 'mcp', } }, ); - expect(JSON.stringify(mockUpdateOne.mock.calls[0])).not.toMatch(/cm_agent|command text/i); + expect(JSON.stringify(mockUpdateOne.mock.calls)).not.toMatch(/cm_agent|command text/i); }); - test('keeps only the first command kind and refuses invalid kinds', () => { + test('keeps the first-copy pair to the first kind and refuses invalid kinds', () => { markByoCommandCopied('u1', 'cli'); markByoCommandCopied('u1', 'mcp'); markByoCommandCopied('u1', 'shell'); - expect(mockUpdateOne).toHaveBeenCalledTimes(1); + const firstCopyWrites = mockUpdateOne.mock.calls.filter( + ([filter]) => filter['onboarding.byoCommandCopiedAt'], + ); + expect(firstCopyWrites).toHaveLength(1); + expect(firstCopyWrites[0][1].$set['onboarding.byoCommandType']).toBe('cli'); + expect(mockUpdateOne).toHaveBeenCalledTimes(3); + }); + + test('records each command kind once, so MCP-then-listener is not read as MCP-only (TASK-721)', () => { + markByoCommandCopied('u1', 'mcp'); + markByoCommandCopied('u1', 'mcp'); + markByoCommandCopied('u1', 'cli'); + markByoCommandCopied('u1', 'cli'); + expect(mockUpdateOne).toHaveBeenCalledWith( + { _id: 'u1', isBot: { $ne: true }, 'onboarding.byoMcpCopiedAt': { $exists: false } }, + { $set: { 'onboarding.byoMcpCopiedAt': expect.any(Date) } }, + ); + expect(mockUpdateOne).toHaveBeenCalledWith( + { _id: 'u1', isBot: { $ne: true }, 'onboarding.byoCliCopiedAt': { $exists: false } }, + { $set: { 'onboarding.byoCliCopiedAt': expect.any(Date) } }, + ); + // one first-copy write + one per kind, however often each button is pressed + expect(mockUpdateOne).toHaveBeenCalledTimes(3); + }); + + test('a failed per-kind write is retried on the next copy', async () => { + mockUpdateOne.mockRejectedValueOnce(new Error('down')); + markByoCommandCopied('u1', 'cli'); + await flush(); + mockUpdateOne.mockClear(); + markByoCommandCopied('u1', 'cli'); + expect(mockUpdateOne).toHaveBeenCalledWith( + expect.objectContaining({ 'onboarding.byoCliCopiedAt': { $exists: false } }), + expect.anything(), + ); }); test('records a first BYO runtime attempt for the human installer, not the bot', () => { @@ -275,7 +309,8 @@ describe('the onboarding marks together', () => { expect(mockUpdateOne.mock.calls.map(fieldOf)) .toEqual([ 'onboarding.podOpenedAt', 'onboarding.firstStartAt', 'onboarding.byoOpenedAt', - 'onboarding.byoCommandCopiedAt', 'onboarding.byoFirstConnectionAttemptAt', + 'onboarding.byoCliCopiedAt', 'onboarding.byoCommandCopiedAt', + 'onboarding.byoFirstConnectionAttemptAt', ]); }); @@ -291,6 +326,6 @@ describe('the onboarding marks together', () => { markByoOpened('u1'); markByoCommandCopied('u1', 'cli'); markByoConnectionAttempt('u1', { config: { runtime: { host: 'byo' } } }); - expect(mockUpdateOne).toHaveBeenCalledTimes(10); + expect(mockUpdateOne).toHaveBeenCalledTimes(12); }); }); diff --git a/backend/__tests__/unit/services/stalledConnectService.test.js b/backend/__tests__/unit/services/stalledConnectService.test.js index 921610ca98..bf010ad1df 100644 --- a/backend/__tests__/unit/services/stalledConnectService.test.js +++ b/backend/__tests__/unit/services/stalledConnectService.test.js @@ -220,6 +220,124 @@ describe('stalledConnectService.scan', () => { ); }); + const copiedKinds = (onboarding) => mockUserFindById.mockReturnValue({ + select: () => ({ lean: () => Promise.resolve({ username: 'alice', onboarding }) }), + }); + + it('names the Terminal command when only the MCP snippet was copied (TASK-721)', async () => { + setup(); + copiedKinds({ + byoCommandCopiedAt: new Date(OLD_TOKEN.getTime() + 8 * MINUTE), + byoCommandType: 'mcp', + byoMcpCopiedAt: new Date(OLD_TOKEN.getTime() + 8 * MINUTE), + }); + await scan({ now: NOW }); + const { content } = mockPost.mock.calls[0][0]; + expect(content).toContain('you copied the MCP command'); + expect(content).toContain('but not the listener command that starts me'); + expect(content).toContain('MCP-only clients can call Commonly, but they do not answer @mentions'); + expect(content).toContain('Run `commonly agent run ai1-agent` in a Terminal on the machine where I should live'); + expect(content).toContain("I'll post here the moment I connect"); + }); + + it('does not say the Terminal command was missed when the listener command was copied too', async () => { + setup(); + copiedKinds({ + byoCommandCopiedAt: new Date(OLD_TOKEN.getTime() + 8 * MINUTE), + byoCommandType: 'mcp', + byoMcpCopiedAt: new Date(OLD_TOKEN.getTime() + 8 * MINUTE), + byoCliCopiedAt: new Date(OLD_TOKEN.getTime() + 9 * MINUTE), + }); + await scan({ now: NOW }); + const { content } = mockPost.mock.calls[0][0]; + expect(content).toContain('you copied the listener command'); + expect(content).not.toContain('but not the listener command'); + expect(content).not.toContain('MCP-only clients'); + }); + + it('keeps the older wording for a signup that only has the first-copy pair', async () => { + setup(); + copiedKinds({ + byoCommandCopiedAt: new Date(OLD_TOKEN.getTime() + 8 * MINUTE), + byoCommandType: 'mcp', + }); + await scan({ now: NOW }); + const { content } = mockPost.mock.calls[0][0]; + expect(content).toContain('here is the one thing to check'); + expect(content).not.toContain('but not the listener command'); + }); + + it('does not call a pre-deploy listener copy "never copied" when an MCP copy follows', async () => { + setup(); + copiedKinds({ + byoCommandCopiedAt: new Date(OLD_TOKEN.getTime() + 1 * MINUTE), + byoCommandType: 'cli', + byoMcpCopiedAt: new Date(OLD_TOKEN.getTime() + 5 * MINUTE), + }); + await scan({ now: NOW }); + const { content } = mockPost.mock.calls[0][0]; + expect(content).toContain('you copied the listener command'); + expect(content).not.toContain('but not the listener command'); + }); + + it('makes no "missed it" claim when the listener copy is from an earlier agent and the MCP copy is inside this episode', async () => { + setup(); + copiedKinds({ + byoCommandCopiedAt: new Date(OLD_TOKEN.getTime() - 7 * 24 * 60 * MINUTE), + byoCommandType: 'cli', + byoCliCopiedAt: new Date(OLD_TOKEN.getTime() - 7 * 24 * 60 * MINUTE), + byoMcpCopiedAt: new Date(OLD_TOKEN.getTime() + 3 * MINUTE), + }); + await scan({ now: NOW }); + const { content } = mockPost.mock.calls[0][0]; + // The first-copy mark cannot show whether this seat's listener was copied. + expect(content).toContain('you copied the MCP command'); + expect(content).toContain('here is the one thing to check'); + expect(content).toContain('MCP-only clients can call Commonly'); + expect(content).not.toContain('but not the listener command'); + }); + + it('an unparseable per-kind time falls back to the first-copy pair', async () => { + setup(); + copiedKinds({ + byoCommandCopiedAt: new Date(OLD_TOKEN.getTime() + 1 * MINUTE), + byoCommandType: 'cli', + byoCliCopiedAt: 'garbage', + byoMcpCopiedAt: new Date(OLD_TOKEN.getTime() + 5 * MINUTE), + }); + await scan({ now: NOW }); + expect(mockPost.mock.calls[0][0].content).not.toContain('but not the listener command'); + }); + + it('falls back to the generic nudge when every copy predates the token', async () => { + setup(); + copiedKinds({ + byoCommandCopiedAt: new Date(OLD_TOKEN.getTime() - 60 * MINUTE), + byoCommandType: 'mcp', + byoMcpCopiedAt: new Date(OLD_TOKEN.getTime() - 60 * MINUTE), + }); + await scan({ now: NOW }); + expect(mockPost.mock.calls[0][0].content).toContain('nothing has ever started me'); + }); + + it.each([['linux', 'Linux'], ['windows', 'Windows']])( + 'MCP-only from a %s browser does not say the listener command was missed', + async (os, label) => { + setup(); + copiedKinds({ + byoCommandCopiedAt: new Date(OLD_TOKEN.getTime() + 8 * MINUTE), + byoCommandType: 'mcp', + byoMcpCopiedAt: new Date(OLD_TOKEN.getTime() + 8 * MINUTE), + byoOpenedOs: os, + }); + await scan({ now: NOW }); + const { content } = mockPost.mock.calls[0][0]; + expect(content).toContain(`you first opened this page on ${label}`); + expect(content).toContain('in a Terminal on a Mac'); + expect(content).not.toContain('but not the listener command'); + }, + ); + it('holds fire inside the patience window', async () => { setup({ installs: [install({ runtimeTokens: [{ createdAt: FRESH_TOKEN }] })] }); const r = await scan({ now: NOW }); diff --git a/backend/models/User.ts b/backend/models/User.ts index 427715f980..4370202952 100644 --- a/backend/models/User.ts +++ b/backend/models/User.ts @@ -216,6 +216,9 @@ export interface IUser extends Document { byoCommandCopiedAt?: Date; /** Fixed kind only; command contents and tokens are never stored. */ byoCommandType?: 'mcp' | 'cli'; + /** First copy of each setup command, kept per kind (TASK-721). */ + byoMcpCopiedAt?: Date; + byoCliCopiedAt?: Date; /** First authenticated BYO runtime request, never overwritten. */ byoFirstConnectionAttemptAt?: Date; /** First "invite a teammate" card mounted, and first dismissed. */ @@ -470,6 +473,8 @@ const userSchema = new Schema({ byoOpenedOs: { type: String, enum: ['mac', 'linux', 'windows', 'other'] }, byoCommandCopiedAt: { type: Date }, byoCommandType: { type: String, enum: ['mcp', 'cli'] }, + byoMcpCopiedAt: { type: Date }, + byoCliCopiedAt: { type: Date }, byoFirstConnectionAttemptAt: { type: Date }, inviteCardShownAt: { type: Date }, inviteCardDismissedAt: { type: Date }, diff --git a/backend/services/onboardingMarks.ts b/backend/services/onboardingMarks.ts index 62db8ffb75..71265733d1 100644 --- a/backend/services/onboardingMarks.ts +++ b/backend/services/onboardingMarks.ts @@ -45,6 +45,7 @@ const podOpenedMarked = new Set(); const startPickedMarked = new Set(); const byoOpenedMarked = new Set(); const byoCommandCopiedMarked = new Set(); +const byoCommandKindMarked = new Set(); const byoConnectionAttemptMarked = new Set(); const surfaceMarked = new Map>(); @@ -146,10 +147,31 @@ export function markByoOpened(userId: unknown, os?: unknown): void { * * Only the fixed command kind is kept. The command itself contains a runtime * token, so it must never cross this boundary or be written to telemetry. + * + * Two shapes are written. `byoCommandCopiedAt` / `byoCommandType` keep the + * FIRST copy of either kind, as before. `byoMcpCopiedAt` / `byoCliCopiedAt` + * keep the first copy of EACH kind, so "copied the MCP snippet and never the + * listener command" (TASK-721) is distinguishable from "MCP first, listener + * second", which the first-copy pair cannot tell apart. */ export function markByoCommandCopied(userId: unknown, commandType: 'mcp' | 'cli'): void { const id = String(userId || ''); if (!id || !['mcp', 'cli'].includes(commandType)) return; + const kindField = commandType === 'mcp' ? 'onboarding.byoMcpCopiedAt' : 'onboarding.byoCliCopiedAt'; + if (!byoCommandKindMarked.has(`${id}:${commandType}`)) { + byoCommandKindMarked.add(`${id}:${commandType}`); + try { + User.updateOne( + { _id: id, isBot: { $ne: true }, [kindField]: { $exists: false } }, + { $set: { [kindField]: new Date() } }, + ).catch(() => { + byoCommandKindMarked.delete(`${id}:${commandType}`); + }); + } catch { + // Telemetry fails open: it must never break the copy flow. + byoCommandKindMarked.delete(`${id}:${commandType}`); + } + } if (byoCommandCopiedMarked.has(id)) return; byoCommandCopiedMarked.add(id); try { @@ -239,6 +261,7 @@ export function resetOnboardingMarksForTest(): void { startPickedMarked.clear(); byoOpenedMarked.clear(); byoCommandCopiedMarked.clear(); + byoCommandKindMarked.clear(); byoConnectionAttemptMarked.clear(); surfaceMarked.clear(); } diff --git a/backend/services/stalledConnectCopy.ts b/backend/services/stalledConnectCopy.ts index 73c0dda4f2..8cc657c339 100644 --- a/backend/services/stalledConnectCopy.ts +++ b/backend/services/stalledConnectCopy.ts @@ -42,6 +42,13 @@ export type StalledConnectCopyEvidence = { commandCopiedAt?: Date | string | null; /** Fixed enum only; command contents and tokens never enter copy. */ commandType?: 'mcp' | 'cli' | null; + /** + * First copy of each command kind (TASK-721). When either is present they + * win over the first-copy pair above: only they can say a person copied the + * MCP snippet and never the Terminal command. + */ + mcpCopiedAt?: Date | string | null; + cliCopiedAt?: Date | string | null; /** Token issue time for the current stalled episode. */ tokenIssuedAt?: Date | string | null; /** OS the Bring your own page detected on the first open (byoOpenedOs). */ @@ -95,13 +102,68 @@ export const composeStalledConnectNudge = ({ locale?: NudgeLocale; evidence?: StalledConnectCopyEvidence; }): string => { - const offsetSeconds = copiedAtOffsetSeconds(evidence); + // Per-kind copies say which command is missing (TASK-721). A per-kind mark + // is the FIRST copy of that kind ever, per USER, so it can prove a copy + // happened but never that none did. Two readings follow: + // - `rawTime`: any copy of the kind, on any date. A kind with no raw time at + // all was never copied, the only case where "you missed it" is true. + // - `inEpisode`: the copy falls at or after this token was issued. Only + // that one can be described as part of this setup. + // A missing or unparseable per-kind time is filled from the older first-copy + // pair when the pair names the same kind: a copy made before the per-kind + // marks shipped, or one whose per-kind write failed, must not read as "never". + const hasKindEvidence = Boolean(evidence.mcpCopiedAt || evidence.cliCopiedAt); + const issuedMs = evidence.tokenIssuedAt ? new Date(evidence.tokenIssuedAt).getTime() : NaN; + const parse = (value: Date | string | null | undefined): Date | null => { + if (!value) return null; + const at = new Date(value); + return Number.isNaN(at.getTime()) ? null : at; + }; + const kindTimes = (kind: 'mcp' | 'cli'): { rawTime: Date | null; inEpisode: Date | null } => { + const own = parse(kind === 'mcp' ? evidence.mcpCopiedAt : evidence.cliCopiedAt); + const fromPair = evidence.commandType === kind ? parse(evidence.commandCopiedAt) : null; + const rawTime = own || fromPair; + const inEpisode = rawTime && (Number.isNaN(issuedMs) || rawTime.getTime() >= issuedMs) ? rawTime : null; + return { rawTime, inEpisode }; + }; + const mcp = hasKindEvidence ? kindTimes('mcp') : { rawTime: null, inEpisode: null }; + const cli = hasKindEvidence ? kindTimes('cli') : { rawTime: null, inEpisode: null }; + const mcpAt = mcp.inEpisode; + const cliAt = cli.inEpisode; + // True only when a listener command was never copied at all. + const mcpOnly = hasKindEvidence && mcpAt !== null && cli.rawTime === null; + const effective: StalledConnectCopyEvidence = hasKindEvidence + ? { + ...evidence, + commandType: cliAt ? 'cli' : mcpAt ? 'mcp' : null, + commandCopiedAt: cliAt || mcpAt, + } + : evidence; + const offsetSeconds = copiedAtOffsetSeconds(effective); + if (mcpOnly && offsetSeconds !== null) { + const unavailableOs = evidence.openedOs ? LISTENER_UNAVAILABLE_OS[evidence.openedOs] : undefined; + const when = copiedAtDescription(offsetSeconds); + if (unavailableOs) { + // The page offers the listener command on its Mac tab only, so do not + // tell this person they missed a command their browser never showed. + return `@${installerName} — you copied the MCP command ${when}. ` + + `MCP-only clients can call Commonly, but they do not answer @mentions. ` + + `The listener that does runs only from a Mac for now, and you first opened this page on ${unavailableOs}. ` + + `Run \`${fixCommand}\` in a Terminal on a Mac, or choose “Run it here” on the Bring your own page for a hosted agent. ` + + `I'll post here the moment I connect.`; + } + return `@${installerName} — you copied the MCP command ${when}, but not the listener command that starts me. ` + + `MCP-only clients can call Commonly, but they do not answer @mentions, ` + + `so nothing is listening for @${handle} yet. ` + + `Run \`${fixCommand}\` in a Terminal on the machine where I should live. ` + + `I'll post here the moment I connect.`; + } if (offsetSeconds !== null) { - const copiedThing = evidence.commandType === 'mcp' + const copiedThing = effective.commandType === 'mcp' ? 'MCP command' - : evidence.commandType === 'cli' ? 'listener command' : 'command'; - const unavailableOs = evidence.commandType === 'cli' && evidence.openedOs - ? LISTENER_UNAVAILABLE_OS[evidence.openedOs] + : effective.commandType === 'cli' ? 'listener command' : 'command'; + const unavailableOs = effective.commandType === 'cli' && effective.openedOs + ? LISTENER_UNAVAILABLE_OS[effective.openedOs] : undefined; if (unavailableOs) { return `@${installerName} — you copied the listener command ` @@ -115,7 +177,7 @@ export const composeStalledConnectNudge = ({ + `${copiedAtDescription(offsetSeconds)}; ` + `no connection arrived — here is the one thing to check: ` + `run \`${fixCommand}\` on the machine where I should live.` - + (evidence.commandType === 'mcp' + + (effective.commandType === 'mcp' ? ' MCP-only clients can call Commonly, but they do not answer @mentions.' : '') + ` I'll post here the moment I connect.`; diff --git a/backend/services/stalledConnectService.ts b/backend/services/stalledConnectService.ts index 597fb30b70..9498c058c3 100644 --- a/backend/services/stalledConnectService.ts +++ b/backend/services/stalledConnectService.ts @@ -247,6 +247,8 @@ const nudgeOnce = async ({ onboarding?: { byoCommandCopiedAt?: Date; byoCommandType?: 'mcp' | 'cli'; + byoMcpCopiedAt?: Date; + byoCliCopiedAt?: Date; byoOpenedOs?: 'mac' | 'linux' | 'windows' | 'other'; }; } | null; @@ -293,6 +295,8 @@ const nudgeOnce = async ({ evidence: { commandCopiedAt: installer.onboarding?.byoCommandCopiedAt, commandType: installer.onboarding?.byoCommandType, + mcpCopiedAt: installer.onboarding?.byoMcpCopiedAt, + cliCopiedAt: installer.onboarding?.byoCliCopiedAt, openedOs: installer.onboarding?.byoOpenedOs, tokenIssuedAt: issuedAt, }, diff --git a/docs/runbooks/new-signup-stall.md b/docs/runbooks/new-signup-stall.md index c71a62803e..dc1cf847fe 100644 --- a/docs/runbooks/new-signup-stall.md +++ b/docs/runbooks/new-signup-stall.md @@ -26,6 +26,8 @@ confirm, for example against `lastActive` or whether they sent a message | `firstStartAt` + `firstStartId` | their first message in a pod they created matches a start's label, or the server resolves a click on their own "Run an agent here" | they acted, and which choice | | `byoOpenedAt` | they first load Bring your own (`GET /api/hosted/availability`) | they looked at setup | | `byoOpenedOs` | written with `byoOpenedAt`, from the `?os=` the page sends: `mac`, `linux`, `windows` or `other`. Unset for anyone who opened the page before it shipped, and for an unknown value | whether the listener path (Mac only) could have worked for them | +| `byoCommandCopiedAt` + `byoCommandType` | the first setup command they copy on Bring your own, `mcp` or `cli`; never overwritten, and never the command itself | they started setup, and with which kind | +| `byoMcpCopiedAt`, `byoCliCopiedAt` | the first copy of each kind, written once each (TASK-721). Unset for copies made before they shipped; the stalled-connect nudge then falls back to the pair above. The marks are per person, not per agent, so the nudge counts a kind only when it falls after the stalled token was issued | whether they copied the MCP snippet and never the listener command | Read them together with whether the person sent a message in their workspace, and when (timestamps and ids).