diff --git a/products/tasks/mcp/apps/LoopReviewView.test.ts b/products/tasks/mcp/apps/LoopReviewView.test.ts new file mode 100644 index 000000000000..4e914e854e41 --- /dev/null +++ b/products/tasks/mcp/apps/LoopReviewView.test.ts @@ -0,0 +1,61 @@ +jest.mock('@posthog/mcp-ui', () => ({ DescriptionList: () => null }), { virtual: true }) +jest.mock('@posthog/quill', () => ({ Button: () => null, Card: () => null, CardContent: () => null }), { + virtual: true, +}) +jest.mock('lucide-react', () => ({ Check: () => null }), { virtual: true }) + +import { + describeFixReviewComments, + describePosthogAccess, + describeRunBehavior, + type LoopReviewBehaviors, + type LoopReviewConnectors, + type LoopReviewData, +} from './LoopReviewView' + +describe('LoopReviewView', () => { + describe('describeFixReviewComments', () => { + it.each<[string, LoopReviewBehaviors | undefined, string]>([ + ['fix_review_comments without watch_ci', { fix_review_comments: true, watch_ci: false }, 'Yes'], + ['fix_review_comments with watch_ci', { fix_review_comments: true, watch_ci: true }, 'Yes'], + ['watch_ci only', { fix_review_comments: false, watch_ci: true }, 'No'], + ['neither flag', { fix_review_comments: false, watch_ci: false }, 'No'], + ['missing behaviors', undefined, 'No'], + ['iteration cap', { fix_review_comments: true, max_fix_iterations: 5 }, 'Yes (up to 5 iterations)'], + ['zero iteration cap', { fix_review_comments: true, max_fix_iterations: 0 }, 'Yes (up to 0 iterations)'], + ])('%s', (_label, behaviors, expected) => { + expect(describeFixReviewComments(behaviors)).toBe(expected) + }) + }) + + describe('describeRunBehavior', () => { + it.each<[string, Pick, string]>([ + ['no triggers shows the default overlap policy', {}, 'Manual only · skips overlapping runs'], + [ + 'cron schedule trigger', + { triggers: [{ type: 'schedule', config: { cron_expression: '0 9 * * 1' } }] }, + 'Schedule (0 9 * * 1) · skips overlapping runs', + ], + ['paused loop', { enabled: false }, 'Manual only · paused · skips overlapping runs'], + ['allow policy', { overlap_policy: 'allow' }, 'Manual only · allows overlapping runs'], + ['cancel_previous policy', { overlap_policy: 'cancel_previous' }, 'Manual only · cancels the previous run'], + [ + 'unknown policy falls back to the raw value', + { overlap_policy: 'defer' as unknown as LoopReviewData['overlap_policy'] }, + 'Manual only · defer', + ], + ])('%s', (_label, data, expected) => { + expect(describeRunBehavior(data)).toBe(expected) + }) + }) + + describe('describePosthogAccess', () => { + it.each<[string, LoopReviewConnectors | undefined, string]>([ + ['missing connectors', undefined, 'Read-only'], + ['read_only scope', { posthog_mcp_scopes: 'read_only' }, 'Read-only'], + ['full scope', { posthog_mcp_scopes: 'full' }, 'Full (read-write)'], + ])('%s', (_label, connectors, expected) => { + expect(describePosthogAccess(connectors)).toBe(expected) + }) + }) +}) diff --git a/products/tasks/mcp/apps/LoopReviewView.tsx b/products/tasks/mcp/apps/LoopReviewView.tsx index f0c46c034da8..a267175489e8 100644 --- a/products/tasks/mcp/apps/LoopReviewView.tsx +++ b/products/tasks/mcp/apps/LoopReviewView.tsx @@ -19,6 +19,7 @@ export interface LoopReviewBehaviors { create_prs?: boolean watch_ci?: boolean fix_review_comments?: boolean + max_fix_iterations?: number } export interface LoopReviewContextOutputs { @@ -37,6 +38,11 @@ export interface LoopReviewNotificationChannel { enabled?: boolean } +export interface LoopReviewConnectors { + mcp_installation_ids?: string[] + posthog_mcp_scopes?: 'read_only' | 'full' +} + /** The loop config the agent assembled — identical in shape to the `loops-create` tool * arguments, so the "Create loop" button can forward it unchanged. */ export interface LoopReviewData { @@ -49,11 +55,14 @@ export interface LoopReviewData { visibility?: string repositories?: LoopReviewRepository[] triggers?: LoopReviewTrigger[] + enabled?: boolean + overlap_policy?: 'skip' | 'allow' | 'cancel_previous' behaviors?: LoopReviewBehaviors + connectors?: LoopReviewConnectors + sandbox_environment?: string | null notifications?: Record context_target?: LoopReviewContextTarget | null _posthogUrl?: string - [key: string]: unknown } export interface LoopReviewState { @@ -91,11 +100,35 @@ function describeTrigger(trigger: LoopReviewTrigger): string { return 'API' } -function describeTriggers(triggers: LoopReviewTrigger[] | undefined): string { - if (!triggers || triggers.length === 0) { - return 'Manual only' +const OVERLAP_LABELS: Record = { + skip: 'skips overlapping runs', + allow: 'allows overlapping runs', + cancel_previous: 'cancels the previous run', +} + +export function describeRunBehavior(data: Pick): string { + const parts: string[] = [] + if (!data.triggers || data.triggers.length === 0) { + parts.push('Manual only') + } else { + parts.push(data.triggers.map(describeTrigger).join(', ')) + } + if (data.enabled === false) { + parts.push('paused') } - return triggers.map(describeTrigger).join(', ') + // the server defaults overlap_policy to 'skip', so always show the effective policy + const overlapPolicy = data.overlap_policy ?? 'skip' + parts.push(OVERLAP_LABELS[overlapPolicy] ?? overlapPolicy) + return parts.join(' · ') +} + +export function describePosthogAccess(connectors: LoopReviewConnectors | undefined): string { + return connectors?.posthog_mcp_scopes === 'full' ? 'Full (read-write)' : 'Read-only' +} + +function describeConnectors(connectors: LoopReviewConnectors | undefined): string { + const ids = connectors?.mcp_installation_ids ?? [] + return ids.length > 0 ? ids.join(', ') : 'None' } function describeRepository(repositories: LoopReviewRepository[] | undefined): string { @@ -137,8 +170,12 @@ function describeModel(data: LoopReviewData): string { return `${adapter} · ${model} · ${reasoning} reasoning` } -function describeAutoFix(behaviors: LoopReviewBehaviors | undefined): string { - return behaviors?.watch_ci && behaviors?.fix_review_comments ? 'On' : 'Off' +export function describeFixReviewComments(behaviors: LoopReviewBehaviors | undefined): string { + if (!behaviors?.fix_review_comments) { + return 'No' + } + const cap = behaviors.max_fix_iterations + return cap != null ? `Yes (up to ${cap} iterations)` : 'Yes' } export function LoopReviewView({ data, onCreate, state }: LoopReviewViewProps): ReactElement { @@ -165,17 +202,22 @@ export function LoopReviewView({ data, onCreate, state }: LoopReviewViewProps): const items: { label: string; value: ReactNode }[] = [ { label: 'Name', value: data.name?.trim() || 'Not set' }, + ...(data.description?.trim() ? [{ label: 'Description', value: data.description }] : []), { label: 'Visibility', value: data.visibility === 'team' ? 'Team' : 'Personal' }, { label: 'What it does', value: {data.instructions?.trim() || 'No prompt'}, }, - { label: 'Runs', value: describeTriggers(data.triggers) }, + { label: 'Runs', value: describeRunBehavior(data) }, { label: 'Context', value: describeContext(data.context_target) }, { label: 'Repository', value: describeRepository(data.repositories) }, { label: 'Model', value: describeModel(data) }, { label: 'Opens PRs', value: data.behaviors?.create_prs ? 'Yes' : 'No' }, - { label: 'Auto-fix PRs', value: describeAutoFix(data.behaviors) }, + { label: 'Watches CI', value: data.behaviors?.watch_ci ? 'Yes' : 'No' }, + { label: 'Fixes review comments', value: describeFixReviewComments(data.behaviors) }, + { label: 'PostHog access', value: describePosthogAccess(data.connectors) }, + { label: 'Connectors', value: describeConnectors(data.connectors) }, + { label: 'Sandbox', value: data.sandbox_environment || 'None' }, { label: 'Notifications', value: describeNotifications(data.notifications) }, ] diff --git a/products/tasks/mcp/apps/index.ts b/products/tasks/mcp/apps/index.ts index 1d16d20b291e..c5a2656a67e6 100644 --- a/products/tasks/mcp/apps/index.ts +++ b/products/tasks/mcp/apps/index.ts @@ -1,6 +1,7 @@ export { LoopReviewView, type LoopReviewBehaviors, + type LoopReviewConnectors, type LoopReviewContextOutputs, type LoopReviewContextTarget, type LoopReviewData, diff --git a/services/mcp/src/lib/build-tool-result.ts b/services/mcp/src/lib/build-tool-result.ts index 026668198388..870a6dba8204 100644 --- a/services/mcp/src/lib/build-tool-result.ts +++ b/services/mcp/src/lib/build-tool-result.ts @@ -2,6 +2,7 @@ import { RESOURCE_URI_META_KEY } from '@modelcontextprotocol/ext-apps/server' import { estimateTokens } from '@/lib/estimate-tokens' import { formatResponse } from '@/lib/response' +import { isPrepareConfirmedActionResult } from '@/tools/confirmed-action-runtime' import { POSTHOG_FORMATTED_RESULTS_OVERRIDE_KEY, POSTHOG_META_KEY } from '@/tools/types' import { APP_DATA_META_KEY, type AnalyticsMetadata, type WithAnalytics } from '@/ui-apps/types' @@ -180,5 +181,13 @@ export function buildToolResultPayload(opts: BuildToolResultOptions): ToolResult payload._meta[APP_DATA_META_KEY] = structuredContent as Record } } + // `-prepare` tools have no UI resource, so UI apps driving a confirmed action + // read the hash from the app-only `_meta` channel instead of structuredContent. + if (isPrepareConfirmedActionResult(handlerResult)) { + payload._meta = { + ...payload._meta, + [APP_DATA_META_KEY]: rawResult as Record, + } + } return payload } diff --git a/services/mcp/src/tools/confirmed-action-runtime.ts b/services/mcp/src/tools/confirmed-action-runtime.ts index 6387d1628f49..5ed845fb0253 100644 --- a/services/mcp/src/tools/confirmed-action-runtime.ts +++ b/services/mcp/src/tools/confirmed-action-runtime.ts @@ -105,6 +105,14 @@ export interface PrepareConfirmedActionResult { next_steps: string } +export function isPrepareConfirmedActionResult(value: unknown): value is PrepareConfirmedActionResult { + if (value === null || typeof value !== 'object' || Array.isArray(value)) { + return false + } + const candidate = value as Partial + return typeof candidate.confirmation_hash === 'string' && candidate.confirmation_word === CONFIRMATION_WORD +} + /** * Run at the top of a `-prepare` tool's handler. Signs the args into a * hash and returns the payload the model relays to the user. diff --git a/services/mcp/src/ui-apps/apps/loops-review-confirm.ts b/services/mcp/src/ui-apps/apps/loops-review-confirm.ts new file mode 100644 index 000000000000..efc2d82eb810 --- /dev/null +++ b/services/mcp/src/ui-apps/apps/loops-review-confirm.ts @@ -0,0 +1,47 @@ +import type { LoopReviewData } from 'products/tasks/mcp/apps' + +import { APP_DATA_META_KEY } from '../types' + +// `loops-create` is a confirmed action (prepare/execute) and the card's click is the human +// confirmation step, so only fields the card renders may travel: anything else in `data` +// would be created without ever being reviewed. Keying off `LoopReviewData` makes the +// compiler reject a card field that isn't listed here. +const REVIEWED_FIELDS: Record, true> = { + name: true, + description: true, + instructions: true, + runtime_adapter: true, + model: true, + reasoning_effort: true, + visibility: true, + repositories: true, + triggers: true, + enabled: true, + overlap_policy: true, + behaviors: true, + connectors: true, + sandbox_environment: true, + notifications: true, + context_target: true, +} + +export function buildReviewedConfig(data: LoopReviewData): Record { + const reviewed: Record = {} + for (const key of Object.keys(REVIEWED_FIELDS)) { + reviewed[key] = data[key as keyof LoopReviewData] + } + return reviewed +} + +// `-prepare` tools have no UI resource, so the server mirrors the result onto the app-only +// `_meta` channel; `structuredContent` stays as a fallback for hosts that forward it. +export function extractConfirmationHash(prepared: { + _meta?: Record + structuredContent?: unknown +}): string | undefined { + const preparedData = (prepared._meta?.[APP_DATA_META_KEY] ?? prepared.structuredContent) as + | { confirmation_hash?: unknown } + | undefined + const hash = preparedData?.confirmation_hash + return typeof hash === 'string' && hash.length > 0 ? hash : undefined +} diff --git a/services/mcp/src/ui-apps/apps/loops-review.tsx b/services/mcp/src/ui-apps/apps/loops-review.tsx index 8c6c2eabba06..f2fd4fb8a16e 100644 --- a/services/mcp/src/ui-apps/apps/loops-review.tsx +++ b/services/mcp/src/ui-apps/apps/loops-review.tsx @@ -7,6 +7,7 @@ import { createRoot } from 'react-dom/client' import { LoopReviewView, type LoopReviewData, type LoopReviewState } from 'products/tasks/mcp/apps' import { AppWrapper } from '../components/AppWrapper' +import { buildReviewedConfig, extractConfirmationHash } from './loops-review-confirm' function LoopReviewApp(): JSX.Element { return ( @@ -30,13 +31,9 @@ function LoopReviewContent({ data, app }: { data: LoopReviewData; app: App | nul } setState({ loading: true, error: null, createdName: null }) try { - // `loops-create` is a confirmed action (prepare/execute), so an agent can't plant a - // persistent loop without an explicit human step. This button IS that step: prepare with - // the reviewed config unchanged (`loops-review`'s schema is the `loops-create` body), - // then execute with the returned hash — the click supplies the confirmation. const prepared = await app.callServerTool({ name: 'loops-create-prepare', - arguments: data as Record, + arguments: buildReviewedConfig(data), }) if (prepared.isError) { const message = @@ -45,10 +42,13 @@ function LoopReviewContent({ data, app }: { data: LoopReviewData; app: App | nul setState({ loading: false, error: message, createdName: null }) return } - const confirmationHash = (prepared.structuredContent as { confirmation_hash?: string } | undefined) - ?.confirmation_hash + const confirmationHash = extractConfirmationHash(prepared) if (!confirmationHash) { - setState({ loading: false, error: 'Failed to create the loop.', createdName: null }) + setState({ + loading: false, + error: 'Failed to create the loop: the server did not return a confirmation hash.', + createdName: null, + }) return } const result = await app.callServerTool({ diff --git a/services/mcp/tests/unit/build-tool-result.test.ts b/services/mcp/tests/unit/build-tool-result.test.ts index 0575b17bcf0c..0530c3d70379 100644 --- a/services/mcp/tests/unit/build-tool-result.test.ts +++ b/services/mcp/tests/unit/build-tool-result.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from 'vitest' import { EXEC_BUILT_PAYLOAD, markExecPayload, buildToolResultPayload, isToolCallPayload } from '@/lib/build-tool-result' import { POSTHOG_FORMATTED_RESULTS_OVERRIDE_KEY, POSTHOG_META_KEY } from '@/tools/types' +import { APP_DATA_META_KEY } from '@/ui-apps/types' // Simulates a `query-trends` handler return value: a UI-resource tool that // carries both the raw `results` object and a pre-formatted pipe-delimited table @@ -270,3 +271,44 @@ describe('isToolCallPayload — nominal brand', () => { expect(isToolCallPayload(42)).toBe(false) }) }) + +describe('buildToolResultPayload — confirmed-action prepare results', () => { + const prepareResult = { + confirmation_hash: 'signed-token-abc', + confirmation_word: 'confirm', + action: 'create loop', + message: "About to create the loop 'Open PR Summary'. Reply 'confirm' to create it.", + next_steps: 'Surface the message above to the user.', + } + + it('carries the prepare payload on _meta so UI apps can read the hash', () => { + const payload = buildToolResultPayload({ + handlerResult: prepareResult, + toolMeta: undefined, + toolName: 'loops-create-prepare', + params: { name: 'Open PR Summary' }, + }) + + expect(payload._meta?.[APP_DATA_META_KEY]).toMatchObject({ + confirmation_hash: 'signed-token-abc', + confirmation_word: 'confirm', + }) + expect(payload).not.toHaveProperty('structuredContent') + expect(payload.content[0]!.text).toContain('signed-token-abc') + }) + + it.each([ + ['non-prepare object result', { results: [{ id: 1 }] }], + ['confirmation_hash without the confirm word', { confirmation_hash: 'abc', confirmation_word: 'yes' }], + ['non-string confirmation_hash', { confirmation_hash: 42, confirmation_word: 'confirm' }], + ])('does not attach _meta for %s', (_label, handlerResult) => { + const payload = buildToolResultPayload({ + handlerResult, + toolMeta: undefined, + toolName: 'whatever', + params: {}, + }) + + expect(payload).not.toHaveProperty('_meta') + }) +}) diff --git a/services/mcp/tests/unit/loops-review-confirm.test.ts b/services/mcp/tests/unit/loops-review-confirm.test.ts new file mode 100644 index 000000000000..ed60e520c76e --- /dev/null +++ b/services/mcp/tests/unit/loops-review-confirm.test.ts @@ -0,0 +1,59 @@ +import { describe, expect, it } from 'vitest' + +import { buildReviewedConfig, extractConfirmationHash } from '@/ui-apps/apps/loops-review-confirm' +import { APP_DATA_META_KEY } from '@/ui-apps/types' + +import type { LoopReviewData } from 'products/tasks/mcp/apps' + +describe('loops-review confirm helpers', () => { + describe('buildReviewedConfig', () => { + it('forwards reviewed fields and drops anything else', () => { + const data = { + name: 'Open PR summary', + instructions: 'Summarize open PRs', + visibility: 'team', + overlap_policy: 'allow', + _posthogUrl: 'https://us.posthog.com', + injected_field: 'must never travel', + } as unknown as LoopReviewData + + const config = buildReviewedConfig(data) + + expect(config).toMatchObject({ + name: 'Open PR summary', + instructions: 'Summarize open PRs', + visibility: 'team', + overlap_policy: 'allow', + }) + expect(config).not.toHaveProperty('_posthogUrl') + expect(config).not.toHaveProperty('injected_field') + }) + }) + + describe('extractConfirmationHash', () => { + it.each<[string, { _meta?: Record; structuredContent?: unknown }, string | undefined]>([ + [ + 'reads the hash from the _meta app channel', + { _meta: { [APP_DATA_META_KEY]: { confirmation_hash: 'hash-from-meta' } } }, + 'hash-from-meta', + ], + [ + 'prefers _meta over structuredContent', + { + _meta: { [APP_DATA_META_KEY]: { confirmation_hash: 'hash-from-meta' } }, + structuredContent: { confirmation_hash: 'hash-from-structured' }, + }, + 'hash-from-meta', + ], + [ + 'falls back to structuredContent', + { structuredContent: { confirmation_hash: 'hash-from-structured' } }, + 'hash-from-structured', + ], + ['returns undefined when neither channel has a hash', { _meta: {} }, undefined], + ['returns undefined for a non-string hash', { structuredContent: { confirmation_hash: 42 } }, undefined], + ])('%s', (_label, prepared, expected) => { + expect(extractConfirmationHash(prepared)).toBe(expected) + }) + }) +})