From 13c2d651b8d7aa70cb272c2937a7f48b1863200b Mon Sep 17 00:00:00 2001 From: harry-harish <22562634+harry-harish@users.noreply.github.com> Date: Thu, 9 Jul 2026 21:06:22 +0530 Subject: [PATCH 1/9] refactor(peek-mcp): move session-bundle lib from peek-cli so the MCP layer can build .peekbundle Move packages/peek-cli/src/lib/session-bundle.ts verbatim to packages/peek-mcp/src/session-bundle.ts (updating the sha256Hex import to the local native-host/audit-chain.js copy). Add a ./session-bundle subpath export to @peekdev/mcp and add tar as a runtime dependency. Repoint all peek-cli consumers to @peekdev/mcp/session-bundle: - src/commands/sessions.ts - src/lib/import-session.ts - src/commands/sessions.bundle.test.ts - src/lib/session-bundle.test.ts Delete the old peek-cli/src/lib/session-bundle.ts (no thin re-export; zero duplicate logic). All existing bundle/import tests pass unchanged. Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com> --- .../peek-cli/src/commands/sessions.bundle.test.ts | 2 +- packages/peek-cli/src/commands/sessions.ts | 12 ++++++------ packages/peek-cli/src/lib/import-session.ts | 2 +- packages/peek-cli/src/lib/session-bundle.test.ts | 4 ++-- packages/peek-mcp/package.json | 5 +++++ .../src/lib => peek-mcp/src}/session-bundle.ts | 2 +- pnpm-lock.yaml | 3 +++ 7 files changed, 19 insertions(+), 11 deletions(-) rename packages/{peek-cli/src/lib => peek-mcp/src}/session-bundle.ts (98%) diff --git a/packages/peek-cli/src/commands/sessions.bundle.test.ts b/packages/peek-cli/src/commands/sessions.bundle.test.ts index b19f9391..579719a0 100644 --- a/packages/peek-cli/src/commands/sessions.bundle.test.ts +++ b/packages/peek-cli/src/commands/sessions.bundle.test.ts @@ -3,8 +3,8 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { gzipSync } from 'node:zlib'; import { openDb } from '@peekdev/mcp/db'; +import { unpackBundle, verifyBundle } from '@peekdev/mcp/session-bundle'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { unpackBundle, verifyBundle } from '../lib/session-bundle.js'; import { runSessions } from './sessions.js'; let home: string; diff --git a/packages/peek-cli/src/commands/sessions.ts b/packages/peek-cli/src/commands/sessions.ts index 2f3af18a..2b3b991b 100644 --- a/packages/peek-cli/src/commands/sessions.ts +++ b/packages/peek-cli/src/commands/sessions.ts @@ -17,6 +17,12 @@ import { writeFileSync } from 'node:fs'; import { parseArgs } from 'node:util'; import { openDb } from '@peekdev/mcp/db'; import { loadSessionEvents } from '@peekdev/mcp/mcp/event-blobs'; +import { + FULLSNAPSHOT_CAVEAT, + packBundle, + unpackBundle, + verifyBundle, +} from '@peekdev/mcp/session-bundle'; import type { Database } from 'better-sqlite3'; import { deleteSession, @@ -39,12 +45,6 @@ import { import { importSessionBundle } from '../lib/import-session.js'; import { formatBytes, pad } from '../lib/output.js'; import { defaultDbPath, rrwebEventsDir } from '../lib/peek-home.js'; -import { - FULLSNAPSHOT_CAVEAT, - packBundle, - unpackBundle, - verifyBundle, -} from '../lib/session-bundle.js'; /** Open the shared DB for reading (migrations applied so a fresh DB is valid). */ function open(): Database { diff --git a/packages/peek-cli/src/lib/import-session.ts b/packages/peek-cli/src/lib/import-session.ts index a2c231b0..d9b38df0 100644 --- a/packages/peek-cli/src/lib/import-session.ts +++ b/packages/peek-cli/src/lib/import-session.ts @@ -6,9 +6,9 @@ import { randomUUID } from 'node:crypto'; import { mkdirSync, rmSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; import { gzipSync } from 'node:zlib'; +import type { UnpackedBundle } from '@peekdev/mcp/session-bundle'; import type { Database } from 'better-sqlite3'; import { rrwebEventsDir } from './peek-home.js'; -import type { UnpackedBundle } from './session-bundle.js'; export interface ImportOptions { /** true (default): mint a fresh id; false: keep originalSessionId. */ diff --git a/packages/peek-cli/src/lib/session-bundle.test.ts b/packages/peek-cli/src/lib/session-bundle.test.ts index ae051002..66000244 100644 --- a/packages/peek-cli/src/lib/session-bundle.test.ts +++ b/packages/peek-cli/src/lib/session-bundle.test.ts @@ -1,14 +1,14 @@ import { mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { afterEach, beforeEach, describe, expect, it } from 'vitest'; import { BUNDLE_FORMAT_VERSION, FULLSNAPSHOT_CAVEAT, packBundle, unpackBundle, verifyBundle, -} from './session-bundle.js'; +} from '@peekdev/mcp/session-bundle'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; let dir: string; beforeEach(() => { diff --git a/packages/peek-mcp/package.json b/packages/peek-mcp/package.json index 3f58b681..b48e0a7e 100644 --- a/packages/peek-mcp/package.json +++ b/packages/peek-mcp/package.json @@ -45,6 +45,10 @@ "./mcp/event-blobs": { "types": "./dist/mcp/event-blobs.d.ts", "import": "./dist/mcp/event-blobs.js" + }, + "./session-bundle": { + "types": "./dist/session-bundle.d.ts", + "import": "./dist/session-bundle.js" } }, "files": [ @@ -65,6 +69,7 @@ "@cubenest/rrweb-core": "workspace:*", "@modelcontextprotocol/sdk": "^1.29.0", "better-sqlite3": "^12.11.1", + "tar": "^7.4.3", "zod": "^3.25.76" }, "devDependencies": { diff --git a/packages/peek-cli/src/lib/session-bundle.ts b/packages/peek-mcp/src/session-bundle.ts similarity index 98% rename from packages/peek-cli/src/lib/session-bundle.ts rename to packages/peek-mcp/src/session-bundle.ts index 7e6f3796..f5306d55 100644 --- a/packages/peek-cli/src/lib/session-bundle.ts +++ b/packages/peek-mcp/src/session-bundle.ts @@ -6,7 +6,7 @@ import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { create, extract } from 'tar'; -import { sha256Hex } from './audit-chain.js'; +import { sha256Hex } from './native-host/audit-chain.js'; export const BUNDLE_FORMAT_VERSION = 1; diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 7a97e315..d35eef41 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -281,6 +281,9 @@ importers: better-sqlite3: specifier: ^12.11.1 version: 12.11.1 + tar: + specifier: ^7.4.3 + version: 7.5.17 zod: specifier: ^3.25.76 version: 3.25.76 From c25ac9d2a69602bce32727fb85da44a6d9fffb13 Mon Sep 17 00:00:00 2001 From: harry-harish <22562634+harry-harish@users.noreply.github.com> Date: Thu, 9 Jul 2026 21:18:16 +0530 Subject: [PATCH 2/9] =?UTF-8?q?feat(peek-mcp):=20share=5Fsession=20tool=20?= =?UTF-8?q?=E2=80=94=20consent-gated=20session=20bundle=20export?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the share_session MCP tool: given a sessionId (and optional surface name, default 'Slack'), elicits an explicit egress consent card via MCP elicitInput before producing a .peekbundle temp file. The design: • Egress is never silent — if the client lacks elicitation capability the tool denies unconditionally; deny / cancel also denies. No bundle is written without an explicit approve. • NOT tied to the Level-3 act gate — sharing already-recorded data is distinct from acting on the live browser; the consent card IS the authorization, level-independent. • On approve: loads session events + console/network rows from the local DB (same read paths as generate_playwright_repro + get_session_*), calls packBundle (from the Task-1 session-bundle lib) to a temp file in os.tmpdir() named .peekbundle, returns { ok, bundlePath, filename, sizeBytes, caveat: FULLSNAPSHOT_CAVEAT }. Bundle bytes are never returned inline. • On deny: returns { ok: false, result: 'denied' }, no file written. • Audit: every call (approve or deny) writes a share_session audit entry (tool + sessionId + surface + approver). Bundle bytes/content never reach the audit log. Additions: - elicitation.ts: buildEgressConsentMessage() — egress-specific consent copy (distinct from the Action-verb buildElicitMessage used by execute_action). - audit.ts: AuditTool union widened to include 'share_session'. - server.ts: registerTools() adds tool #19 share_session; dispatchShareSession() implements the consent-check + db-load + packBundle + audit path. - PEEK_MCP_TOOLS constant updated (20 tools). - test/share-session.test.ts: 9 tests covering (a) deny→no file, (b) approve→ valid verifyBundle-passing bundle, (c) approved call writes share_session audit entry (no bytes), plus no-capability→deny, unknown-session error, and tool-list registration. - test/server.test.ts: updated tool count 19→20. - .changeset/peek-share-session-tool.md: minor changeset. Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com> --- .changeset/peek-share-session-tool.md | 15 + packages/peek-mcp/src/mcp/elicitation.ts | 15 + packages/peek-mcp/src/mcp/server.ts | 219 ++++++++++- packages/peek-mcp/src/native-host/audit.ts | 8 +- packages/peek-mcp/test/server.test.ts | 4 +- packages/peek-mcp/test/share-session.test.ts | 371 +++++++++++++++++++ 6 files changed, 626 insertions(+), 6 deletions(-) create mode 100644 .changeset/peek-share-session-tool.md create mode 100644 packages/peek-mcp/test/share-session.test.ts diff --git a/.changeset/peek-share-session-tool.md b/.changeset/peek-share-session-tool.md new file mode 100644 index 00000000..af609515 --- /dev/null +++ b/.changeset/peek-share-session-tool.md @@ -0,0 +1,15 @@ +--- +'@peekdev/mcp': minor +--- + +Add `share_session` MCP tool — consent-gated session bundle export. + +`share_session(sessionId, surface?)` elicits an explicit egress consent card +(naming what is exported and where) before producing a portable `.peekbundle` +temp file from a recorded session. On deny → `{ ok: false, result: 'denied' }`, +no file written. On approve → `{ ok: true, bundlePath, filename, sizeBytes, caveat }`. +The bundle contains the masked session recording (DOM + console/network events). +Every approved export is recorded to `~/.peek/audit.log` as `tool: share_session` +(session ID + surface; bundle bytes are never written to the audit log). +Designed for connector-driven upload flows (e.g. Slack `@peek share this session`); +the connector is responsible for uploading and deleting the temp file. diff --git a/packages/peek-mcp/src/mcp/elicitation.ts b/packages/peek-mcp/src/mcp/elicitation.ts index 4a4e8961..9c4a18be 100644 --- a/packages/peek-mcp/src/mcp/elicitation.ts +++ b/packages/peek-mcp/src/mcp/elicitation.ts @@ -101,6 +101,21 @@ export function maskValue(value: string): string { return `${first ?? ''}•••${last ?? ''}`; } +/** + * Human-facing egress consent-card text for a session bundle export. Distinct + * from {@link buildElicitMessage} (which is for live-browser actions): this + * describes a *data-egress* event — the bundle leaving the local-first store. + * + * @param sessionId The session being exported (e.g. `s_acme-orders-0001`). + * @param surface Where the bundle will go (e.g. `Slack`, `Discord`). + */ +export function buildEgressConsentMessage(sessionId: string, surface: string): string { + return ( + `peek wants to upload this session's bundle (${sessionId} — recorded DOM + ` + + `console/network, masked) to ${surface}. This data leaves your local-first peek store. Approve?` + ); +} + /** Human-facing consent-card text for an action. peek-mcp does NOT classify the * action as destructive/act (that lives in the SW gate) — it only describes it, * masking any literal value that would otherwise persist in the client's chat diff --git a/packages/peek-mcp/src/mcp/server.ts b/packages/peek-mcp/src/mcp/server.ts index 20e372fd..af055841 100644 --- a/packages/peek-mcp/src/mcp/server.ts +++ b/packages/peek-mcp/src/mcp/server.ts @@ -17,10 +17,13 @@ // §B3 token budgets (counts not dumps; truncated fields; capped lists). import { createRequire } from 'node:module'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; import type { Database } from 'better-sqlite3'; import { z } from 'zod'; import { openReadonlyDb } from '../db/open.js'; +import { FULLSNAPSHOT_CAVEAT, packBundle } from '../session-bundle.js'; // Read version at runtime from this package's package.json so the MCP // `serverInfo.version` reply always matches what npm shipped. The relative @@ -28,7 +31,7 @@ import { openReadonlyDb } from '../db/open.js'; const _require = createRequire(import.meta.url); const _pkg = _require('../../package.json') as { version: string }; export const SERVER_VERSION = _pkg.version; -import { existsSync, readFileSync } from 'node:fs'; +import { existsSync, readFileSync, statSync } from 'node:fs'; import { verifyAuditChain, verifySummary } from '../native-host/audit-chain.js'; import { auditHeadPath, readHead } from '../native-host/audit-head.js'; import { @@ -42,7 +45,7 @@ import { } from '../native-host/audit.js'; import { ActionSchema } from './action-schema.js'; import { buildCausalChain } from './causal-chain.js'; -import { buildElicitMessage, elicitConsent } from './elicitation.js'; +import { buildEgressConsentMessage, buildElicitMessage, elicitConsent } from './elicitation.js'; import { SessionEventsError, loadEventsUpToTs, loadSessionEvents } from './event-blobs.js'; import { extractDomMutationsInWindow, @@ -1136,6 +1139,45 @@ export function createPeekMcpServer(options: CreatePeekMcpServerOptions = {}): P return await dispatchPairingRequest({ code }); }, ); + + // 19. share_session (consent-gated egress export) ------------------------- + // Produces a .peekbundle temp file from a recorded session ONLY after an + // explicit egress consent card (MCP elicitInput). Does NOT gate on Level 3 + // — exporting already-recorded data is distinct from acting on the live + // browser. On deny → { ok: false, result: 'denied' }, no file written. + // On approve → { ok: true, bundlePath, filename, sizeBytes, caveat }. + // Every approved export is audit-logged (tool + sessionId + approver; NOT + // the bundle bytes or content). Temp file ownership is the caller's — + // connector-core deletes it after upload. + server.registerTool( + 'share_session', + { + title: 'Export a session bundle for sharing', + description: + 'Export a session as a portable .peekbundle file to share with teammates (e.g. upload to a Slack thread). ' + + 'Requires explicit egress consent — a card naming what is exported and where; deny produces no file. ' + + 'On approve, returns the temp file path and size; the bundle never leaves your machine without consent. ' + + 'The bundle contains the masked session recording (DOM + console/network); review the caveat before sharing.', + inputSchema: { + sessionId: z.string().describe('Session id from list_recent_sessions.'), + surface: z + .string() + .default('Slack') + .describe( + "The surface the bundle will be shared to (e.g. 'Slack', 'Discord'). Shown in the consent card.", + ), + }, + annotations: { + readOnlyHint: false, + destructiveHint: false, + idempotentHint: false, + openWorldHint: true, + }, + }, + async ({ sessionId, surface }) => { + return await dispatchShareSession({ sessionId, surface }); + }, + ); } // --- Act-tool dispatch (shared between execute_action + request_authorization) --- @@ -1329,6 +1371,177 @@ export function createPeekMcpServer(options: CreatePeekMcpServerOptions = {}): P ...(response.approved && response.secret !== undefined ? { secret: response.secret } : {}), }); } + + // --- share_session dispatch (consent-gated egress export) --- + // + // 1. Probe elicitation capability — if the client supports it, elicit an + // egress consent card. If the client does NOT support elicitation, deny + // unconditionally: egress without explicit consent is not allowed. + // 2. On deny → audit-log + return { ok: false, result: 'denied' }, no file. + // 3. On approve → load the session from the DB + blob, packBundle to a temp + // file, audit-log the export, return { ok: true, bundlePath, ... }. + async function dispatchShareSession(input: { + sessionId: string; + surface: string; + }): Promise> { + const requestStartedAtMs = Date.now(); + const clientImpl = server.server.getClientVersion(); + const client = clientImpl?.name ?? 'unknown'; + const auditWriteOptions: AuditWriteOptions = + options.auditLogPath !== undefined ? { path: options.auditLogPath } : {}; + + const consentMsg = buildEgressConsentMessage(input.sessionId, input.surface); + const elicitOutcome = await elicitConsent(server.server, consentMsg); + + // Egress without consent is not allowed. If the client lacks elicitation + // capability we cannot obtain consent → deny unconditionally. + if (!elicitOutcome.elicited || elicitOutcome.verdict === 'deny') { + const denyReason = !elicitOutcome.elicited + ? 'elicitation not supported by client' + : elicitOutcome.reason; + const draft: DraftAuditEntry = { + ts: new Date(requestStartedAtMs).toISOString(), + tool: 'share_session', + args: { sessionId: input.sessionId, surface: input.surface }, + approver: 'user', + client, + sessionId: input.sessionId, + result: 'denied', + error: denyReason, + }; + try { + appendAuditEntry(draft, auditWriteOptions); + } catch (err) { + console.error( + `peek-mcp: share_session audit log write failed — ${err instanceof Error ? err.message : String(err)}`, + ); + } + return jsonResult({ ok: false, result: 'denied', reason: denyReason }); + } + + // Approved — load session data and build the bundle. + const handle = getDb(); + if (!handle) { + return jsonResult({ ok: false, result: 'error', error: NO_DB_MESSAGE }); + } + const row = getSessionSummaryRow(handle, input.sessionId); + if (!row) { + return jsonResult({ + ok: false, + result: 'error', + error: `No session found with id '${input.sessionId}'.`, + }); + } + + // Load events (same path as generate_playwright_repro). + const ev = eventsFor(input.sessionId); + if (!ev.ok) { + return jsonResult({ ok: false, result: 'error', error: ev.message }); + } + + // Load console + network rows for the bundle payload (all rows, not just errors). + const consoleEvents = ( + handle + .prepare( + 'SELECT ts_ms, level, message, stack FROM console_events WHERE session_id = ? ORDER BY ts_ms ASC', + ) + .all(input.sessionId) as Array<{ + ts_ms: number; + level: string; + message: string; + stack: string | null; + }> + ).map((r) => ({ + ts: r.ts_ms, + level: r.level, + message: r.message, + ...(r.stack !== null ? { stack: r.stack } : {}), + })); + + const networkEvents = ( + handle + .prepare( + 'SELECT ts_ms, method, url, status, status_text, resource_type, duration_ms, error_text FROM network_events WHERE session_id = ? ORDER BY ts_ms ASC', + ) + .all(input.sessionId) as Array<{ + ts_ms: number; + method: string; + url: string; + status: number | null; + status_text: string | null; + resource_type: string | null; + duration_ms: number | null; + error_text: string | null; + }> + ).map((r) => ({ + ts: r.ts_ms, + method: r.method, + url: r.url, + ...(r.status !== null ? { status: r.status } : {}), + ...(r.status_text !== null ? { statusText: r.status_text } : {}), + ...(r.resource_type !== null ? { resourceType: r.resource_type } : {}), + ...(r.duration_ms !== null ? { durationMs: r.duration_ms } : {}), + ...(r.error_text !== null ? { errorText: r.error_text } : {}), + })); + + // Pack the bundle to a temp file named for the session. + const safeId = input.sessionId.replace(/[^a-zA-Z0-9_-]/g, '_'); + const filename = `${safeId}.peekbundle`; + const bundlePath = join(tmpdir(), filename); + + try { + packBundle(bundlePath, { + session: { + id: row.id, + origin: row.origin, + url: row.url, + title: row.title, + startedAt: row.startedAt, + durationMs: row.durationMs, + eventCount: row.eventCount, + errorCount: row.errorCount, + }, + consoleEvents, + networkEvents, + events: ev.events, + }); + } catch (err) { + return jsonResult({ + ok: false, + result: 'error', + error: `Failed to pack bundle: ${err instanceof Error ? err.message : String(err)}`, + }); + } + + const sizeBytes = statSync(bundlePath).size; + + // Audit-log the approved export. Args record tool + sessionId + surface + // (never the bundle content or bytes). + const draft: DraftAuditEntry = { + ts: new Date(requestStartedAtMs).toISOString(), + tool: 'share_session', + args: { sessionId: input.sessionId, surface: input.surface }, + approver: 'connector-elicit', + client, + sessionId: input.sessionId, + result: 'ok', + }; + try { + appendAuditEntry(draft, auditWriteOptions); + } catch (err) { + console.error( + `peek-mcp: share_session audit log write failed — ${err instanceof Error ? err.message : String(err)}`, + ); + } + + return jsonResult({ + ok: true, + bundlePath, + filename, + sizeBytes, + caveat: FULLSNAPSHOT_CAVEAT, + }); + } } /** The tool names this server registers, for smoke tests / docs. */ @@ -1361,4 +1574,6 @@ export const PEEK_MCP_TOOLS = [ 'verify_audit_log', // SP4: connector pairing handshake. 'request_pairing', + // Consent-gated egress export (.peekbundle to a temp file). + 'share_session', ] as const; diff --git a/packages/peek-mcp/src/native-host/audit.ts b/packages/peek-mcp/src/native-host/audit.ts index 41a634d1..d62af41e 100644 --- a/packages/peek-mcp/src/native-host/audit.ts +++ b/packages/peek-mcp/src/native-host/audit.ts @@ -41,8 +41,12 @@ export function auditLogPath(): string { return join(peekHomeDir(), 'audit.log'); } -/** Tool names that produce audit entries — the Level-3+ MCP write tools + SP4 pairing. */ -export type AuditTool = 'execute_action' | 'request_authorization' | 'request_pairing'; +/** Tool names that produce audit entries — the Level-3+ MCP write tools + SP4 pairing + egress export. */ +export type AuditTool = + | 'execute_action' + | 'request_authorization' + | 'request_pairing' + | 'share_session'; /** Approver values, ordered by what the dispatcher will record. */ export type AuditApprover = diff --git a/packages/peek-mcp/test/server.test.ts b/packages/peek-mcp/test/server.test.ts index a35f495d..b680e60e 100644 --- a/packages/peek-mcp/test/server.test.ts +++ b/packages/peek-mcp/test/server.test.ts @@ -358,9 +358,9 @@ describe('peek MCP server: graceful no-DB', () => { eventsDir: join(dir, 'rrweb-events'), }); try { - // tools/list still works (8 read + search_sessions + get_page_view + get_element_detail + 2 write + 2 suggest + 1 handoff + set_intent + verify_audit_log + request_pairing). + // tools/list still works (8 read + search_sessions + get_page_view + get_element_detail + 2 write + 2 suggest + 1 handoff + set_intent + verify_audit_log + request_pairing + share_session). const { tools } = await client.listTools(); - expect(tools).toHaveLength(19); + expect(tools).toHaveLength(20); // and a call returns the friendly message rather than erroring. const res = await client.callTool({ name: 'list_recent_sessions', arguments: {} }); expect(textOf(res as never)).toContain('No sessions recorded yet'); diff --git a/packages/peek-mcp/test/share-session.test.ts b/packages/peek-mcp/test/share-session.test.ts new file mode 100644 index 00000000..3e95e29c --- /dev/null +++ b/packages/peek-mcp/test/share-session.test.ts @@ -0,0 +1,371 @@ +// Tests for the share_session MCP tool (Task 2 — consent-gated egress export). +// +// Three required scenarios (from the task-2 brief): +// (a) deny → { ok: false, result: 'denied' }, no bundle file written. +// (b) approve → { ok: true, bundlePath, filename, sizeBytes, caveat }; +// the file at bundlePath passes verifyBundle. +// (c) approved call writes a 'share_session' audit entry (no bundle bytes). +// +// Approach: connect via InMemoryTransport; stub getClientCapabilities + +// elicitInput on the underlying SDK Server (same pattern as server.elicit.test.ts) +// to control elicitation responses without a real MCP elicitation round-trip. +// Use seedStore to create a real SQLite DB with a minimal session. + +import { existsSync, readFileSync, rmSync } from 'node:fs'; +import { mkdtempSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { Client } from '@modelcontextprotocol/sdk/client/index.js'; +import { InMemoryTransport } from '@modelcontextprotocol/sdk/inMemory.js'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { createPeekMcpServer } from '../src/mcp/server.js'; +import { unpackBundle, verifyBundle } from '../src/session-bundle.js'; +import { documentWith, el, freshIds, fullSnapshot, text } from './fixtures/rrweb.js'; +import { seedStore } from './fixtures/seed.js'; + +let dir: string; + +beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'peek-share-session-')); + freshIds(); +}); + +afterEach(() => { + rmSync(dir, { recursive: true, force: true }); +}); + +/** Parse the first text content block of a tool result as JSON. */ +function parseJson(result: { content: Array<{ type: string; text?: string }> }): unknown { + const block = result.content.find((c) => c.type === 'text'); + return JSON.parse(block?.text ?? 'null'); +} + +/** Build a connected client+server pair. */ +async function connectClient(opts: { + dbPath: string; + eventsDir: string; + auditLogPath: string; +}): Promise<{ + client: Client; + peek: ReturnType; + close: () => Promise; +}> { + const peek = createPeekMcpServer({ + dbPath: opts.dbPath, + eventsDir: opts.eventsDir, + auditLogPath: opts.auditLogPath, + }); + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + const client = new Client({ name: 'test-connector', version: '0.0.0' }); + await Promise.all([peek.server.connect(serverTransport), client.connect(clientTransport)]); + return { + client, + peek, + close: async () => { + await client.close(); + peek.close(); + }, + }; +} + +/** Stub elicitation on the SDK Server instance (mirrors server.elicit.test.ts). */ +function stubElicitation( + peek: ReturnType, + action: 'accept' | 'decline' | 'cancel', +): void { + const sdkServer = peek.server.server as unknown as Record; + sdkServer.getClientCapabilities = () => ({ elicitation: { form: {} } }); + sdkServer.elicitInput = async () => ({ action }); +} + +/** Seed a store with one session that has events, console errors, and network events. */ +function seedSessionWithData(homeDir: string): { + dbPath: string; + eventsDir: string; + sessionId: string; +} { + const sessionId = 's_test-share-session-0001'; + const now = new Date('2026-07-09T10:00:00.000Z'); + const events = [ + fullSnapshot(documentWith([el('h1', { children: [text('hello')] })]), now.getTime()), + ]; + const { dbPath, eventsDir } = seedStore(homeDir, [ + { + id: sessionId, + createdAt: now.toISOString(), + updatedAt: new Date(now.getTime() + 5000).toISOString(), + url: 'https://example.com/app', + title: 'Test App', + origin: 'https://example.com', + events, + consoleErrors: [ + { + ts: now.getTime() + 1000, + message: 'Uncaught TypeError: x is not a function', + stack: 'at app.js:1', + }, + ], + networkErrors: [ + { + ts: now.getTime() + 2000, + method: 'GET', + url: 'https://example.com/api/data', + status: 404, + }, + ], + }, + ]); + return { dbPath, eventsDir, sessionId }; +} + +describe('share_session — deny', () => { + it('(a) decline → { ok: false, result: denied }, no bundle file written', async () => { + const { dbPath, eventsDir, sessionId } = seedSessionWithData(dir); + const auditLogPath = join(dir, 'audit.log'); + const { client, peek, close } = await connectClient({ dbPath, eventsDir, auditLogPath }); + try { + stubElicitation(peek, 'decline'); + + const res = await client.callTool({ + name: 'share_session', + arguments: { sessionId, surface: 'Slack' }, + }); + + const body = parseJson(res as never) as Record; + expect(body.ok).toBe(false); + expect(body.result).toBe('denied'); + + // No bundle file should have been written to tmpdir. + const safeId = sessionId.replace(/[^a-zA-Z0-9_-]/g, '_'); + const expectedPath = join(tmpdir(), `${safeId}.peekbundle`); + // Clean up the file if it somehow got written (shouldn't happen). + expect(existsSync(expectedPath)).toBe(false); + } finally { + await close(); + } + }); + + it('(a) cancel → { ok: false, result: denied } too', async () => { + const { dbPath, eventsDir, sessionId } = seedSessionWithData(dir); + const auditLogPath = join(dir, 'audit.log'); + const { client, peek, close } = await connectClient({ dbPath, eventsDir, auditLogPath }); + try { + stubElicitation(peek, 'cancel'); + + const res = await client.callTool({ + name: 'share_session', + arguments: { sessionId, surface: 'Slack' }, + }); + + const body = parseJson(res as never) as Record; + expect(body.ok).toBe(false); + expect(body.result).toBe('denied'); + } finally { + await close(); + } + }); + + it('(a) no elicitation capability → denied (egress requires consent)', async () => { + const { dbPath, eventsDir, sessionId } = seedSessionWithData(dir); + const auditLogPath = join(dir, 'audit.log'); + const { client, close } = await connectClient({ dbPath, eventsDir, auditLogPath }); + try { + // No stubElicitation call — client has no elicitation capability. + const res = await client.callTool({ + name: 'share_session', + arguments: { sessionId, surface: 'Slack' }, + }); + + const body = parseJson(res as never) as Record; + expect(body.ok).toBe(false); + expect(body.result).toBe('denied'); + } finally { + await close(); + } + }); + + it('(a) deny writes a denied audit entry (no bundle bytes)', async () => { + const { dbPath, eventsDir, sessionId } = seedSessionWithData(dir); + const auditLogPath = join(dir, 'audit.log'); + const { client, peek, close } = await connectClient({ dbPath, eventsDir, auditLogPath }); + try { + stubElicitation(peek, 'decline'); + + await client.callTool({ + name: 'share_session', + arguments: { sessionId, surface: 'Slack' }, + }); + + const contents = readFileSync(auditLogPath, 'utf8'); + const lines = contents.split('\n').filter((l) => l.length > 0); + expect(lines).toHaveLength(1); + const entry = JSON.parse(lines[0] as string) as Record; + expect(entry.tool).toBe('share_session'); + expect(entry.result).toBe('denied'); + expect(entry.sessionId).toBe(sessionId); + // No bundle bytes in audit entry. + expect(JSON.stringify(entry)).not.toContain('bundlePath'); + expect(JSON.stringify(entry)).not.toContain('events.json'); + } finally { + await close(); + } + }); +}); + +describe('share_session — approve', () => { + it('(b) approve → ok:true, bundlePath whose file passes verifyBundle', async () => { + const { dbPath, eventsDir, sessionId } = seedSessionWithData(dir); + const auditLogPath = join(dir, 'audit.log'); + const { client, peek, close } = await connectClient({ dbPath, eventsDir, auditLogPath }); + let bundlePath: string | undefined; + try { + stubElicitation(peek, 'accept'); + + const res = await client.callTool({ + name: 'share_session', + arguments: { sessionId, surface: 'Slack' }, + }); + + const body = parseJson(res as never) as Record; + expect(body.ok).toBe(true); + expect(typeof body.bundlePath).toBe('string'); + expect(typeof body.filename).toBe('string'); + expect(typeof body.sizeBytes).toBe('number'); + expect(body.sizeBytes as number).toBeGreaterThan(0); + expect(typeof body.caveat).toBe('string'); + expect((body.caveat as string).length).toBeGreaterThan(0); + + bundlePath = body.bundlePath as string; + expect(existsSync(bundlePath)).toBe(true); + + // The file must be a valid bundle. + const unpacked = unpackBundle(bundlePath); + expect(() => verifyBundle(unpacked)).not.toThrow(); + + // filename must end with .peekbundle and contain the session id (sanitized). + expect(body.filename as string).toMatch(/\.peekbundle$/); + } finally { + // Clean up the temp bundle file. + if (bundlePath && existsSync(bundlePath)) rmSync(bundlePath); + await close(); + } + }); + + it('(b) bundle contains the session data (origin, url, event count)', async () => { + const { dbPath, eventsDir, sessionId } = seedSessionWithData(dir); + const auditLogPath = join(dir, 'audit.log'); + const { client, peek, close } = await connectClient({ dbPath, eventsDir, auditLogPath }); + let bundlePath: string | undefined; + try { + stubElicitation(peek, 'accept'); + + const res = await client.callTool({ + name: 'share_session', + arguments: { sessionId, surface: 'Slack' }, + }); + + const body = parseJson(res as never) as Record; + bundlePath = body.bundlePath as string; + + const unpacked = unpackBundle(bundlePath); + verifyBundle(unpacked); + + // Session row fields should be present. + expect((unpacked.session.session as Record).id).toBe(sessionId); + expect((unpacked.session.session as Record).origin).toBe( + 'https://example.com', + ); + // Events should be present. + expect(unpacked.events.length).toBeGreaterThan(0); + // Console events should be present. + expect(Array.isArray(unpacked.session.consoleEvents)).toBe(true); + // Network events should be present. + expect(Array.isArray(unpacked.session.networkEvents)).toBe(true); + } finally { + if (bundlePath && existsSync(bundlePath)) rmSync(bundlePath); + await close(); + } + }); + + it('(c) approve writes a share_session audit entry with no bundle bytes', async () => { + const { dbPath, eventsDir, sessionId } = seedSessionWithData(dir); + const auditLogPath = join(dir, 'audit.log'); + const { client, peek, close } = await connectClient({ dbPath, eventsDir, auditLogPath }); + let bundlePath: string | undefined; + try { + stubElicitation(peek, 'accept'); + + const res = await client.callTool({ + name: 'share_session', + arguments: { sessionId, surface: 'Slack' }, + }); + + const body = parseJson(res as never) as Record; + bundlePath = body.bundlePath as string; + + const contents = readFileSync(auditLogPath, 'utf8'); + const lines = contents.split('\n').filter((l) => l.length > 0); + expect(lines).toHaveLength(1); + const entry = JSON.parse(lines[0] as string) as Record; + + // Required audit fields. + expect(entry.tool).toBe('share_session'); + expect(entry.result).toBe('ok'); + expect(entry.sessionId).toBe(sessionId); + expect(entry.approver).toBe('connector-elicit'); + + // Args must record the surface (so the audit shows where it went) but NOT the bundle bytes. + const args = entry.args as Record; + expect(args.sessionId).toBe(sessionId); + expect(args.surface).toBe('Slack'); + + // No bundle content in the audit entry. + const entryStr = JSON.stringify(entry); + expect(entryStr).not.toContain('events.json'); + expect(entryStr).not.toContain('session.json'); + // bundlePath may appear in args but should NOT carry the file content. + const parsedAgain = JSON.parse(entryStr) as Record; + const argsStr = JSON.stringify(parsedAgain.args as Record); + expect(argsStr).not.toContain('FullSnapshot'); + } finally { + if (bundlePath && existsSync(bundlePath)) rmSync(bundlePath); + await close(); + } + }); + + it('(c) approve with unknown session → error (no file, no audit ok entry)', async () => { + const { dbPath, eventsDir } = seedSessionWithData(dir); + const auditLogPath = join(dir, 'audit.log'); + const { client, peek, close } = await connectClient({ dbPath, eventsDir, auditLogPath }); + try { + stubElicitation(peek, 'accept'); + + const res = await client.callTool({ + name: 'share_session', + arguments: { sessionId: 's_does-not-exist', surface: 'Slack' }, + }); + + const body = parseJson(res as never) as Record; + // Should return an error (session not found). + expect(body.ok).toBe(false); + expect(body.result).toBe('error'); + } finally { + await close(); + } + }); +}); + +describe('share_session — tool is registered', () => { + it('share_session appears in the tool list', async () => { + const { dbPath, eventsDir } = seedSessionWithData(dir); + const auditLogPath = join(dir, 'audit.log'); + const { client, close } = await connectClient({ dbPath, eventsDir, auditLogPath }); + try { + const tools = await client.listTools(); + const names = tools.tools.map((t) => t.name); + expect(names).toContain('share_session'); + } finally { + await close(); + } + }); +}); From 5f2c0a37323a4e9f47d18431550a160363b976cb Mon Sep 17 00:00:00 2001 From: harry-harish <22562634+harry-harish@users.noreply.github.com> Date: Thu, 9 Jul 2026 21:19:42 +0530 Subject: [PATCH 3/9] test(peek-mcp): update smoke + tool-list counts for share_session (20 tools) Bring stdio-smoke.test.ts and server.test.ts in sync with the new tool count after adding share_session in the previous commit. Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com> --- packages/peek-mcp/test/stdio-smoke.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/peek-mcp/test/stdio-smoke.test.ts b/packages/peek-mcp/test/stdio-smoke.test.ts index 78e0a167..37fa4ae4 100644 --- a/packages/peek-mcp/test/stdio-smoke.test.ts +++ b/packages/peek-mcp/test/stdio-smoke.test.ts @@ -73,7 +73,7 @@ afterEach(() => { }); describe.skipIf(!built)('peek-mcp stdio smoke (built bin)', () => { - it('completes initialize + tools/list over real stdio, returning all 19 tools', async () => { + it('completes initialize + tools/list over real stdio, returning all 20 tools', async () => { child = spawn(process.execPath, [distEntry], { env: { ...process.env, PEEK_HOME: home }, stdio: ['pipe', 'pipe', 'pipe'], @@ -119,6 +119,8 @@ describe.skipIf(!built)('peek-mcp stdio smoke (built bin)', () => { 'set_intent', // Audit-log integrity check (read-only; no args). 'verify_audit_log', + // Consent-gated egress export (.peekbundle to a temp file). + 'share_session', ].sort(), ); }); From 8203d089efdc4f6feb3b5769e998a2b345831f87 Mon Sep 17 00:00:00 2001 From: harry-harish <22562634+harry-harish@users.noreply.github.com> Date: Thu, 9 Jul 2026 21:27:02 +0530 Subject: [PATCH 4/9] fix(peek-mcp): share_session post-review fixes (temp-file nonce, statSync guard, injection sanitize, audit approver, test gaps) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix 1: bundle filename now includes a Date.now() nonce (${safeId}-${nonce}.peekbundle) so two concurrent approved exports of the same session produce distinct temp files. Fix 2: statSync moved inside the packBundle try/catch so a post-approve FS error returns {ok:false} instead of propagating as an unhandled MCP throw. Fix 3: sessionId sanitized (strip non-printable / control chars) before interpolation into the egress consent card text — prevents newline injection into the card. Fix 4: no-elicitation-capability deny now records approver:'connector-elicit' instead of 'user' (user was never asked); actual user decline/cancel keeps approver:'user'. Tests: cancel test gains no-file assertion; new unknown-action test locks fail-closed behavior; decline file check updated to pattern scan (robust after nonce addition). Gate condition (!elicitOutcome.elicited || elicitOutcome.verdict === 'deny') unchanged. Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com> --- packages/peek-mcp/src/mcp/elicitation.ts | 5 +- packages/peek-mcp/src/mcp/server.ts | 18 +++++-- packages/peek-mcp/test/share-session.test.ts | 52 ++++++++++++++++++-- 3 files changed, 66 insertions(+), 9 deletions(-) diff --git a/packages/peek-mcp/src/mcp/elicitation.ts b/packages/peek-mcp/src/mcp/elicitation.ts index 9c4a18be..cf512fb7 100644 --- a/packages/peek-mcp/src/mcp/elicitation.ts +++ b/packages/peek-mcp/src/mcp/elicitation.ts @@ -110,8 +110,11 @@ export function maskValue(value: string): string { * @param surface Where the bundle will go (e.g. `Slack`, `Discord`). */ export function buildEgressConsentMessage(sessionId: string, surface: string): string { + // Sanitize sessionId for display: strip control characters (incl. newlines) so + // a crafted id cannot inject into the consent card text. + const safeSessionId = sessionId.replace(/[^\x20-\x7E]/g, '_'); return ( - `peek wants to upload this session's bundle (${sessionId} — recorded DOM + ` + + `peek wants to upload this session's bundle (${safeSessionId} — recorded DOM + ` + `console/network, masked) to ${surface}. This data leaves your local-first peek store. Approve?` ); } diff --git a/packages/peek-mcp/src/mcp/server.ts b/packages/peek-mcp/src/mcp/server.ts index af055841..061df944 100644 --- a/packages/peek-mcp/src/mcp/server.ts +++ b/packages/peek-mcp/src/mcp/server.ts @@ -1399,11 +1399,17 @@ export function createPeekMcpServer(options: CreatePeekMcpServerOptions = {}): P const denyReason = !elicitOutcome.elicited ? 'elicitation not supported by client' : elicitOutcome.reason; + // Fix 4: distinguish a system/no-capability deny (user was never asked) + // from a genuine user decline/cancel — 'connector-elicit' signals the deny + // came from the infrastructure layer, not from a user decision. + const denyApprover: import('../native-host/audit.js').AuditApprover = !elicitOutcome.elicited + ? 'connector-elicit' + : 'user'; const draft: DraftAuditEntry = { ts: new Date(requestStartedAtMs).toISOString(), tool: 'share_session', args: { sessionId: input.sessionId, surface: input.surface }, - approver: 'user', + approver: denyApprover, client, sessionId: input.sessionId, result: 'denied', @@ -1485,10 +1491,13 @@ export function createPeekMcpServer(options: CreatePeekMcpServerOptions = {}): P })); // Pack the bundle to a temp file named for the session. + // Fix 1: append a per-call timestamp nonce so two concurrent approved exports + // of the same session produce distinct files and cannot overwrite each other. const safeId = input.sessionId.replace(/[^a-zA-Z0-9_-]/g, '_'); - const filename = `${safeId}.peekbundle`; + const filename = `${safeId}-${Date.now()}.peekbundle`; const bundlePath = join(tmpdir(), filename); + let sizeBytes: number; try { packBundle(bundlePath, { session: { @@ -1505,6 +1514,9 @@ export function createPeekMcpServer(options: CreatePeekMcpServerOptions = {}): P networkEvents, events: ev.events, }); + // Fix 2: statSync is inside the try so a post-approve file-system error returns + // { ok:false } with an audit entry rather than propagating as an unhandled throw. + sizeBytes = statSync(bundlePath).size; } catch (err) { return jsonResult({ ok: false, @@ -1513,8 +1525,6 @@ export function createPeekMcpServer(options: CreatePeekMcpServerOptions = {}): P }); } - const sizeBytes = statSync(bundlePath).size; - // Audit-log the approved export. Args record tool + sessionId + surface // (never the bundle content or bytes). const draft: DraftAuditEntry = { diff --git a/packages/peek-mcp/test/share-session.test.ts b/packages/peek-mcp/test/share-session.test.ts index 3e95e29c..35e57d7f 100644 --- a/packages/peek-mcp/test/share-session.test.ts +++ b/packages/peek-mcp/test/share-session.test.ts @@ -136,16 +136,18 @@ describe('share_session — deny', () => { expect(body.result).toBe('denied'); // No bundle file should have been written to tmpdir. + const { readdirSync } = await import('node:fs'); const safeId = sessionId.replace(/[^a-zA-Z0-9_-]/g, '_'); - const expectedPath = join(tmpdir(), `${safeId}.peekbundle`); - // Clean up the file if it somehow got written (shouldn't happen). - expect(existsSync(expectedPath)).toBe(false); + const tmpFiles = readdirSync(tmpdir()).filter( + (f) => f.startsWith(safeId) && f.endsWith('.peekbundle'), + ); + expect(tmpFiles).toHaveLength(0); } finally { await close(); } }); - it('(a) cancel → { ok: false, result: denied } too', async () => { + it('(a) cancel → { ok: false, result: denied }, no bundle file written', async () => { const { dbPath, eventsDir, sessionId } = seedSessionWithData(dir); const auditLogPath = join(dir, 'audit.log'); const { client, peek, close } = await connectClient({ dbPath, eventsDir, auditLogPath }); @@ -160,6 +162,48 @@ describe('share_session — deny', () => { const body = parseJson(res as never) as Record; expect(body.ok).toBe(false); expect(body.result).toBe('denied'); + + // No bundle file should have been written (cancel is fail-closed). + expect(body.bundlePath).toBeUndefined(); + // Verify no .peekbundle files for this session exist in tmpdir. + const { readdirSync } = await import('node:fs'); + const safeId = sessionId.replace(/[^a-zA-Z0-9_-]/g, '_'); + const tmpFiles = readdirSync(tmpdir()).filter( + (f) => f.startsWith(safeId) && f.endsWith('.peekbundle'), + ); + expect(tmpFiles).toHaveLength(0); + } finally { + await close(); + } + }); + + it('(a) unknown/malformed elicit action → { ok: false, result: denied }, no bundle file written', async () => { + const { dbPath, eventsDir, sessionId } = seedSessionWithData(dir); + const auditLogPath = join(dir, 'audit.log'); + const { client, peek, close } = await connectClient({ dbPath, eventsDir, auditLogPath }); + try { + // Stub an unknown action value to verify fail-closed behavior for unrecognized responses. + const sdkServer = peek.server.server as unknown as Record; + sdkServer.getClientCapabilities = () => ({ elicitation: { form: {} } }); + sdkServer.elicitInput = async () => ({ action: 'something-unknown' }); + + const res = await client.callTool({ + name: 'share_session', + arguments: { sessionId, surface: 'Slack' }, + }); + + const body = parseJson(res as never) as Record; + expect(body.ok).toBe(false); + expect(body.result).toBe('denied'); + + // No bundle file should have been written (unknown action is fail-closed). + expect(body.bundlePath).toBeUndefined(); + const { readdirSync } = await import('node:fs'); + const safeId = sessionId.replace(/[^a-zA-Z0-9_-]/g, '_'); + const tmpFiles = readdirSync(tmpdir()).filter( + (f) => f.startsWith(safeId) && f.endsWith('.peekbundle'), + ); + expect(tmpFiles).toHaveLength(0); } finally { await close(); } From 09219fc33a4bc5e8ba52744f223de7852a86af61 Mon Sep 17 00:00:00 2001 From: harry-harish <22562634+harry-harish@users.noreply.github.com> Date: Thu, 9 Jul 2026 21:33:26 +0530 Subject: [PATCH 5/9] feat(connector-core): postFile surface hook + share_session result interception MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add optional SurfaceAdapter.postFile? hook (conversationId, filePath, filename, comment?) and intercept approved share_session tool results in ConnectorRuntime.handleConsentResponse: upload via adapter.postFile, then delete the temp bundle (try/finally — file is always removed, even on upload failure). Denied results and adapters without postFile degrade gracefully. Four new tests: approved share_session → postFile called + file deleted; upload failure → file still deleted (try/finally); {ok:false} → no postFile, no throw; no postFile on adapter → text note, no crash. Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com> --- packages/connector-core/src/runtime.test.ts | 192 ++++++++++++++++++++ packages/connector-core/src/runtime.ts | 33 ++++ packages/connector-core/src/surface.ts | 10 + 3 files changed, 235 insertions(+) diff --git a/packages/connector-core/src/runtime.test.ts b/packages/connector-core/src/runtime.test.ts index 74943ae6..9f5fe2f6 100644 --- a/packages/connector-core/src/runtime.test.ts +++ b/packages/connector-core/src/runtime.test.ts @@ -1,3 +1,6 @@ +import { mkdtemp, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; import { describe, expect, it, vi } from 'vitest'; import type { AgentOutcome, Brain, Session } from './brain.js'; import type { PeekMcp } from './mcp.js'; @@ -748,3 +751,192 @@ describe('ConnectorRuntime start() loads existing secret', () => { expect(fakeMcp.setConnectorSecret).not.toHaveBeenCalled(); }); }); + +// --------------------------------------------------------------------------- +// share_session interception tests (Task 3) +// --------------------------------------------------------------------------- + +/** Extend FakeAdapter with postFile tracking. */ +class FileAdapter extends FakeAdapter { + postFileCalls: Array<{ + conversationId: string; + filePath: string; + filename: string; + comment: string; + }> = []; + async postFile(conversationId: string, filePath: string, filename: string, comment?: string) { + this.postFileCalls.push({ conversationId, filePath, filename, comment: comment ?? '' }); + } +} + +/** Build a scripted consent brain that suspends on the named tool. */ +function shareSessionBrain(toolName = 'share_session'): Brain { + let toolResultSeen = false; + return { + newSession: (): Session => ({ history: [] }), + appendUserText: () => {}, + appendToolResult: () => { + toolResultSeen = true; + }, + runTurn: async () => + toolResultSeen + ? { kind: 'done', text: 'shared' } + : { + kind: 'consent', + action: { + toolUseId: 'tu-share-1', + toolName, + input: { sessionId: 's1', surface: 'slack' }, + createdAt: 0, + }, + }, + }; +} + +describe('share_session interception — postFile + temp file delete', () => { + it('calls adapter.postFile with the right args and deletes the temp file on approval', async () => { + // Create a real temp file to assert deletion. + const dir = await mkdtemp(join(tmpdir(), 'peek-test-')); + const bundlePath = join(dir, 'session.peekbundle'); + await writeFile(bundlePath, 'bundle-data'); + + const toolResult = JSON.stringify({ + ok: true, + bundlePath, + filename: 'session.peekbundle', + sizeBytes: 11, + caveat: 'contains data', + }); + + const callTool = vi.fn().mockResolvedValue(toolResult); + const adapter = new FileAdapter(); + const brain = shareSessionBrain(); + const store = new SessionStore(brain.newSession); + const mcp = { callTool, onElicit: () => {} } as unknown as PeekMcp; + const runtime = new ConnectorRuntime({ adapter, brain, mcp, store }); + await runtime.start(); + + adapter.msgHandler?.({ conversationId: 'c1', userId: 'u', text: 'share my session' }); + await vi.waitFor(() => expect(adapter.consents).toHaveLength(1)); + + const correlationId = adapter.consents[0]?.[1].correlationId ?? ''; + adapter.consentHandler?.({ conversationId: 'c1', correlationId, decision: 'approve' }); + await vi.waitFor(() => expect(adapter.texts).toContainEqual(['c1', 'shared'])); + + // postFile must have been called with the correct args. + expect(adapter.postFileCalls).toHaveLength(1); + expect(adapter.postFileCalls[0]?.conversationId).toBe('c1'); + expect(adapter.postFileCalls[0]?.filePath).toBe(bundlePath); + expect(adapter.postFileCalls[0]?.filename).toBe('session.peekbundle'); + + // The temp file must have been deleted. + await expect(rm(bundlePath, { force: false })).rejects.toThrow(); + + // Cleanup the temp dir (bundle already gone, just the dir remains). + await rm(dir, { recursive: true, force: true }); + }); + + it('deletes the temp file even when upload (postFile) throws — try/finally guarantee', async () => { + const dir = await mkdtemp(join(tmpdir(), 'peek-test-')); + const bundlePath = join(dir, 'session.peekbundle'); + await writeFile(bundlePath, 'bundle-data'); + + const toolResult = JSON.stringify({ + ok: true, + bundlePath, + filename: 'session.peekbundle', + sizeBytes: 11, + caveat: '', + }); + const callTool = vi.fn().mockResolvedValue(toolResult); + + class ThrowingFileAdapter extends FakeAdapter { + async postFile(_c: string, _fp: string, _fn: string, _comment?: string): Promise { + throw new Error('Slack upload failed'); + } + } + + const adapter = new ThrowingFileAdapter(); + const brain = shareSessionBrain(); + const store = new SessionStore(brain.newSession); + const mcp = { callTool, onElicit: () => {} } as unknown as PeekMcp; + const runtime = new ConnectorRuntime({ adapter, brain, mcp, store }); + await runtime.start(); + + adapter.msgHandler?.({ conversationId: 'c1', userId: 'u', text: 'share' }); + await vi.waitFor(() => expect(adapter.consents).toHaveLength(1)); + + const correlationId = adapter.consents[0]?.[1].correlationId ?? ''; + // The postFile throws but the runtime should swallow it into the error path + // and the turn should still complete (error posted to brain, runLoop continues). + adapter.consentHandler?.({ conversationId: 'c1', correlationId, decision: 'approve' }); + + // Wait for the runtime to finish the turn (either postText or continuation). + await vi.waitFor(() => + expect(adapter.texts.length + adapter.confirmations.length).toBeGreaterThan(0), + ); + + // The temp file must be deleted despite the upload failure. + await expect(rm(bundlePath, { force: false })).rejects.toThrow(); + + await rm(dir, { recursive: true, force: true }); + }); + + it('does not call postFile and does not throw when result is { ok: false }', async () => { + const toolResult = JSON.stringify({ ok: false, result: 'denied', reason: 'user denied' }); + const callTool = vi.fn().mockResolvedValue(toolResult); + const adapter = new FileAdapter(); + const brain = shareSessionBrain(); + const store = new SessionStore(brain.newSession); + const mcp = { callTool, onElicit: () => {} } as unknown as PeekMcp; + const runtime = new ConnectorRuntime({ adapter, brain, mcp, store }); + await runtime.start(); + + adapter.msgHandler?.({ conversationId: 'c1', userId: 'u', text: 'share' }); + await vi.waitFor(() => expect(adapter.consents).toHaveLength(1)); + + const correlationId = adapter.consents[0]?.[1].correlationId ?? ''; + adapter.consentHandler?.({ conversationId: 'c1', correlationId, decision: 'approve' }); + await vi.waitFor(() => expect(adapter.texts).toContainEqual(['c1', 'shared'])); + + // No postFile should have been called. + expect(adapter.postFileCalls).toHaveLength(0); + }); + + it('degrades to a text note (no crash) when adapter.postFile is undefined', async () => { + const dir = await mkdtemp(join(tmpdir(), 'peek-test-')); + const bundlePath = join(dir, 'session.peekbundle'); + await writeFile(bundlePath, 'bundle-data'); + + const toolResult = JSON.stringify({ + ok: true, + bundlePath, + filename: 'session.peekbundle', + sizeBytes: 11, + caveat: '', + }); + const callTool = vi.fn().mockResolvedValue(toolResult); + + // FakeAdapter has no postFile method. + const adapter = new FakeAdapter(); + const brain = shareSessionBrain(); + const store = new SessionStore(brain.newSession); + const mcp = { callTool, onElicit: () => {} } as unknown as PeekMcp; + const runtime = new ConnectorRuntime({ adapter, brain, mcp, store }); + await runtime.start(); + + adapter.msgHandler?.({ conversationId: 'c1', userId: 'u', text: 'share' }); + await vi.waitFor(() => expect(adapter.consents).toHaveLength(1)); + + const correlationId = adapter.consents[0]?.[1].correlationId ?? ''; + adapter.consentHandler?.({ conversationId: 'c1', correlationId, decision: 'approve' }); + + // Should complete without crash. + await vi.waitFor(() => expect(adapter.texts).toContainEqual(['c1', 'shared'])); + + // Temp file should still be deleted. + await expect(rm(bundlePath, { force: false })).rejects.toThrow(); + + await rm(dir, { recursive: true, force: true }); + }); +}); diff --git a/packages/connector-core/src/runtime.ts b/packages/connector-core/src/runtime.ts index b3600712..c5504006 100644 --- a/packages/connector-core/src/runtime.ts +++ b/packages/connector-core/src/runtime.ts @@ -1,3 +1,4 @@ +import { rm } from 'node:fs/promises'; import type { AgentOutcome, Brain } from './brain.js'; import type { PeekMcp } from './mcp.js'; import type { SecretStore } from './secret-store.js'; @@ -280,6 +281,38 @@ export class ConnectorRuntime { text = `Tool call failed: ${String(err)}`; isError = true; } + + // share_session interception: upload the temp bundle and delete it. + // The temp file is always deleted (try/finally) so session data is never + // left on disk, even if the upload fails. Upload errors are caught so the + // turn continues; the error is fed back to the brain as an error result. + if (pending.toolName === 'share_session' && !isError) { + let parsed: { ok: boolean; bundlePath?: string; filename?: string } | null = null; + try { + parsed = JSON.parse(text) as { ok: boolean; bundlePath?: string; filename?: string }; + } catch { + // Not valid JSON — leave text as-is and fall through. + } + if (parsed?.ok && parsed.bundlePath && parsed.filename) { + const { bundlePath, filename } = parsed; + try { + if (adapter.postFile) { + await adapter.postFile(r.conversationId, bundlePath, filename, 'peek session bundle'); + text = `Session bundle "${filename}" shared successfully.`; + } else { + text = `Session bundle ready at ${bundlePath} (filename: "${filename}"). The surface adapter does not support file upload.`; + } + } catch (uploadErr) { + // Upload failed: surface a descriptive error to the brain so the + // conversation can continue. The finally block still deletes the file. + text = `Session bundle export failed during upload: ${String(uploadErr)}`; + isError = true; + } finally { + await rm(bundlePath, { force: true }); + } + } + } + brain.appendToolResult(stored.session, pending.toolUseId, text, isError); await adapter.postConfirmation(r.conversationId, 'Approved — acting…'); } else { diff --git a/packages/connector-core/src/surface.ts b/packages/connector-core/src/surface.ts index 294180b2..38019785 100644 --- a/packages/connector-core/src/surface.ts +++ b/packages/connector-core/src/surface.ts @@ -30,4 +30,14 @@ export interface SurfaceAdapter { conversationId: string, err: { kind: string; headline: string; hint: string }, ): Promise; + /** Optional: upload a local file to the surface (e.g. post a .peekbundle to a Slack channel). + * Adapters that do not support file uploads may omit this method; the runtime degrades to a + * text note so no crash occurs. The runtime calls this during share_session interception and + * always deletes the temp file (try/finally) regardless of upload success or failure. */ + postFile?( + conversationId: string, + filePath: string, + filename: string, + comment?: string, + ): Promise; } From ec556537868e6de7d432d82816e99fa2a8dced67 Mon Sep 17 00:00:00 2001 From: harry-harish <22562634+harry-harish@users.noreply.github.com> Date: Thu, 9 Jul 2026 21:45:51 +0530 Subject: [PATCH 6/9] fix(connector-core): relocate share_session interception to the callTool boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit share_session is classified 'read' by classify(), so SdkBrain.runTurn runs it inline via its injected callTool and feeds the {ok,bundlePath} result straight back into the tool-use loop — it never emits a {kind:'consent'} outcome. The 09219fc interception lived in handleConsentResponse (consent-only path), so the bundle upload + temp-file cleanup never fired in production; the old tests passed only because a synthetic brain forced the suspend path. Fix (wiring option a): the runtime now exposes interceptCallTool(name, input, inner). The connector-slack composition root hands the brain runtime.interceptCallTool(...) instead of the bare mcp.callTool, so an approved share_session result triggers adapter.postFile + temp-file cleanup on the REAL inline path regardless of classification. Chosen over a PeekMcp post-tool hook because the post-processing needs the surface adapter + active conversationId, which the runtime holds and PeekMcp deliberately does not — keeping PeekMcp a pure transport and the brain seeing a plain (name,input)=>Promise. - Remove the share_session block from handleConsentResponse (wrong path). - Add interceptCallTool() + private #postProcessShareSession(): postFile in try, temp delete in finally; {ok:false}/unparseable → unchanged; no postFile → filename-only degrade note (no raw temp path leak); upload error → surfaced to the brain so the turn continues. - Replace the four synthetic-consent-brain tests with four that drive the REAL SdkBrain (read/inline routing). The MANDATORY test asserts no consent card is posted, postFile fires with the active conversationId, and the temp file is deleted — it fails against the 09219fc wrong-path code. Co-Authored-By: Claude Opus 4.8 Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com> --- packages/connector-core/src/runtime.test.ts | 239 +++++++++++++------- packages/connector-core/src/runtime.ts | 110 ++++++--- packages/connector-slack/src/index.ts | 14 +- 3 files changed, 252 insertions(+), 111 deletions(-) diff --git a/packages/connector-core/src/runtime.test.ts b/packages/connector-core/src/runtime.test.ts index 9f5fe2f6..c0452362 100644 --- a/packages/connector-core/src/runtime.test.ts +++ b/packages/connector-core/src/runtime.test.ts @@ -1,10 +1,12 @@ import { mkdtemp, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; +import type Anthropic from '@anthropic-ai/sdk'; import { describe, expect, it, vi } from 'vitest'; import type { AgentOutcome, Brain, Session } from './brain.js'; import type { PeekMcp } from './mcp.js'; import { ConnectorRuntime, classifyError } from './runtime.js'; +import { SdkBrain } from './sdk-brain.js'; import type { SecretStore } from './secret-store.js'; import { SessionStore } from './store.js'; import type { ConsentResponse, InboundMessage, SurfaceAdapter } from './surface.js'; @@ -769,33 +771,88 @@ class FileAdapter extends FakeAdapter { } } -/** Build a scripted consent brain that suspends on the named tool. */ -function shareSessionBrain(toolName = 'share_session'): Brain { - let toolResultSeen = false; +function anthropicMsg( + content: Anthropic.ContentBlock[], + stop: Anthropic.Message['stop_reason'], +): Anthropic.Message { return { - newSession: (): Session => ({ history: [] }), - appendUserText: () => {}, - appendToolResult: () => { - toolResultSeen = true; - }, - runTurn: async () => - toolResultSeen - ? { kind: 'done', text: 'shared' } - : { - kind: 'consent', - action: { - toolUseId: 'tu-share-1', - toolName, - input: { sessionId: 's1', surface: 'slack' }, - createdAt: 0, - }, - }, - }; + id: 'm', + type: 'message', + role: 'assistant', + model: 'x', + content, + stop_reason: stop, + stop_sequence: null, + usage: { input_tokens: 0, output_tokens: 0 }, + } as Anthropic.Message; +} + +/** A createMessage mock that first emits a `share_session` tool_use (a READ tool, + * so SdkBrain runs it INLINE via its injected callTool — never as a consent + * outcome), then a plain text end_turn once the tool result is fed back. */ +function shareSessionCreateMessage(): ( + req: Anthropic.MessageCreateParamsNonStreaming, +) => Promise { + return vi + .fn() + .mockResolvedValueOnce( + anthropicMsg( + [ + { + type: 'tool_use', + id: 'tu-share-1', + name: 'share_session', + input: { sessionId: 's1', surface: 'slack' }, + caller: { type: 'direct' }, + } as Anthropic.ContentBlock, + ], + 'tool_use', + ), + ) + .mockResolvedValueOnce( + anthropicMsg([{ type: 'text', text: 'shared', citations: [] }], 'end_turn'), + ); } -describe('share_session interception — postFile + temp file delete', () => { - it('calls adapter.postFile with the right args and deletes the temp file on approval', async () => { - // Create a real temp file to assert deletion. +/** + * Build a runtime wired to a REAL SdkBrain whose injected callTool routes through + * `runtime.interceptCallTool(...)` — exactly as the connector-slack composition + * root does. `innerCallTool` is the mocked low-level MCP callTool. This is the + * production path: share_session is read-classified, so SdkBrain calls it inline + * and the result flows through interceptCallTool, NOT handleConsentResponse. + * + * This mirroring is the whole point of the Critical fix — a test that drove a + * synthetic {kind:'consent'} brain (as commit 09219fc's tests did) would exercise + * the WRONG path and pass even though the interception never fires in production. + */ +function makeRealBrainRuntime(deps: { + adapter: SurfaceAdapter; + innerCallTool: (name: string, input: unknown) => Promise; +}): { runtime: ConnectorRuntime } { + // Forward reference — mirrors the connector-slack composition root's construction-time + // dependency cycle (the brain's callTool closes over the runtime). + // biome-ignore lint/style/useConst: forward reference for a construction-time dependency cycle + let runtimeRef: ConnectorRuntime; + const brain = new SdkBrain({ + createMessage: shareSessionCreateMessage(), + callTool: (name, input) => + runtimeRef.interceptCallTool(name, input, (n, i) => deps.innerCallTool(n, i)), + tools: [], + model: 'm', + extendedReasoning: false, + }); + const store = new SessionStore(() => brain.newSession()); + // The runtime only touches mcp.callTool on the consent path; on the read path + // it is never called. A stub keeps the type happy. + const mcp = { callTool: vi.fn(), onElicit: () => {} } as unknown as PeekMcp; + const runtime = new ConnectorRuntime({ adapter: deps.adapter, brain, mcp, store }); + runtimeRef = runtime; + return { runtime }; +} + +describe('share_session interception — REAL SdkBrain inline (read) routing', () => { + it('MANDATORY: an approved share_session from the real SdkBrain triggers postFile + temp-file delete', async () => { + // Real temp file so deletion is observable. const dir = await mkdtemp(join(tmpdir(), 'peek-test-')); const bundlePath = join(dir, 'session.peekbundle'); await writeFile(bundlePath, 'bundle-data'); @@ -807,32 +864,34 @@ describe('share_session interception — postFile + temp file delete', () => { sizeBytes: 11, caveat: 'contains data', }); + // Low-level MCP callTool the brain drives INLINE (share_session is read). + const innerCallTool = vi.fn().mockResolvedValue(toolResult); - const callTool = vi.fn().mockResolvedValue(toolResult); const adapter = new FileAdapter(); - const brain = shareSessionBrain(); - const store = new SessionStore(brain.newSession); - const mcp = { callTool, onElicit: () => {} } as unknown as PeekMcp; - const runtime = new ConnectorRuntime({ adapter, brain, mcp, store }); + const { runtime } = makeRealBrainRuntime({ adapter, innerCallTool }); await runtime.start(); + // Drive a real turn end-to-end: message -> SdkBrain.runTurn -> inline + // callTool(share_session) -> interceptCallTool -> postFile + delete. adapter.msgHandler?.({ conversationId: 'c1', userId: 'u', text: 'share my session' }); - await vi.waitFor(() => expect(adapter.consents).toHaveLength(1)); - - const correlationId = adapter.consents[0]?.[1].correlationId ?? ''; - adapter.consentHandler?.({ conversationId: 'c1', correlationId, decision: 'approve' }); await vi.waitFor(() => expect(adapter.texts).toContainEqual(['c1', 'shared'])); - // postFile must have been called with the correct args. + // The brain ran share_session inline (never a consent outcome). + expect(innerCallTool).toHaveBeenCalledWith('share_session', { + sessionId: 's1', + surface: 'slack', + }); + // No consent card was ever posted (proves the read/inline path, not consent). + expect(adapter.consents).toHaveLength(0); + + // postFile fired with the active conversationId + bundle args. expect(adapter.postFileCalls).toHaveLength(1); expect(adapter.postFileCalls[0]?.conversationId).toBe('c1'); expect(adapter.postFileCalls[0]?.filePath).toBe(bundlePath); expect(adapter.postFileCalls[0]?.filename).toBe('session.peekbundle'); - // The temp file must have been deleted. + // Temp file deleted. await expect(rm(bundlePath, { force: false })).rejects.toThrow(); - - // Cleanup the temp dir (bundle already gone, just the dir remains). await rm(dir, { recursive: true, force: true }); }); @@ -848,7 +907,7 @@ describe('share_session interception — postFile + temp file delete', () => { sizeBytes: 11, caveat: '', }); - const callTool = vi.fn().mockResolvedValue(toolResult); + const innerCallTool = vi.fn().mockResolvedValue(toolResult); class ThrowingFileAdapter extends FakeAdapter { async postFile(_c: string, _fp: string, _fn: string, _comment?: string): Promise { @@ -857,53 +916,33 @@ describe('share_session interception — postFile + temp file delete', () => { } const adapter = new ThrowingFileAdapter(); - const brain = shareSessionBrain(); - const store = new SessionStore(brain.newSession); - const mcp = { callTool, onElicit: () => {} } as unknown as PeekMcp; - const runtime = new ConnectorRuntime({ adapter, brain, mcp, store }); + const { runtime } = makeRealBrainRuntime({ adapter, innerCallTool }); await runtime.start(); adapter.msgHandler?.({ conversationId: 'c1', userId: 'u', text: 'share' }); - await vi.waitFor(() => expect(adapter.consents).toHaveLength(1)); - - const correlationId = adapter.consents[0]?.[1].correlationId ?? ''; - // The postFile throws but the runtime should swallow it into the error path - // and the turn should still complete (error posted to brain, runLoop continues). - adapter.consentHandler?.({ conversationId: 'c1', correlationId, decision: 'approve' }); - - // Wait for the runtime to finish the turn (either postText or continuation). - await vi.waitFor(() => - expect(adapter.texts.length + adapter.confirmations.length).toBeGreaterThan(0), - ); + // The upload error is surfaced to the brain as the tool result; the brain's + // second createMessage still returns end_turn 'shared', so the turn completes. + await vi.waitFor(() => expect(adapter.texts).toContainEqual(['c1', 'shared'])); - // The temp file must be deleted despite the upload failure. + // Temp file deleted despite the upload failure. await expect(rm(bundlePath, { force: false })).rejects.toThrow(); - await rm(dir, { recursive: true, force: true }); }); it('does not call postFile and does not throw when result is { ok: false }', async () => { const toolResult = JSON.stringify({ ok: false, result: 'denied', reason: 'user denied' }); - const callTool = vi.fn().mockResolvedValue(toolResult); + const innerCallTool = vi.fn().mockResolvedValue(toolResult); const adapter = new FileAdapter(); - const brain = shareSessionBrain(); - const store = new SessionStore(brain.newSession); - const mcp = { callTool, onElicit: () => {} } as unknown as PeekMcp; - const runtime = new ConnectorRuntime({ adapter, brain, mcp, store }); + const { runtime } = makeRealBrainRuntime({ adapter, innerCallTool }); await runtime.start(); adapter.msgHandler?.({ conversationId: 'c1', userId: 'u', text: 'share' }); - await vi.waitFor(() => expect(adapter.consents).toHaveLength(1)); - - const correlationId = adapter.consents[0]?.[1].correlationId ?? ''; - adapter.consentHandler?.({ conversationId: 'c1', correlationId, decision: 'approve' }); await vi.waitFor(() => expect(adapter.texts).toContainEqual(['c1', 'shared'])); - // No postFile should have been called. expect(adapter.postFileCalls).toHaveLength(0); }); - it('degrades to a text note (no crash) when adapter.postFile is undefined', async () => { + it('degrades to a filename-only text note (no raw temp path) when adapter.postFile is undefined, and still deletes the temp file', async () => { const dir = await mkdtemp(join(tmpdir(), 'peek-test-')); const bundlePath = join(dir, 'session.peekbundle'); await writeFile(bundlePath, 'bundle-data'); @@ -915,28 +954,68 @@ describe('share_session interception — postFile + temp file delete', () => { sizeBytes: 11, caveat: '', }); - const callTool = vi.fn().mockResolvedValue(toolResult); - // FakeAdapter has no postFile method. - const adapter = new FakeAdapter(); - const brain = shareSessionBrain(); - const store = new SessionStore(brain.newSession); - const mcp = { callTool, onElicit: () => {} } as unknown as PeekMcp; + // Capture the tool-result text fed back to the brain so we can assert the + // degrade note references the filename only and never the raw OS temp path. + let feedbackText = ''; + const innerCallTool = vi.fn().mockResolvedValue(toolResult); + + // FakeAdapter has no postFile. The second createMessage captures the + // tool_result the interception fed back to the brain, so we can assert the + // degrade note references the filename only (never the raw OS temp path). + // biome-ignore lint/style/useConst: forward reference (dependency cycle — see makeRealBrainRuntime). + let runtimeRef: ConnectorRuntime; + const createMessage = vi + .fn() + .mockResolvedValueOnce( + anthropicMsg( + [ + { + type: 'tool_use', + id: 'tu-share-1', + name: 'share_session', + input: { sessionId: 's1', surface: 'slack' }, + caller: { type: 'direct' }, + } as Anthropic.ContentBlock, + ], + 'tool_use', + ), + ) + .mockImplementationOnce((req: Anthropic.MessageCreateParamsNonStreaming) => { + // The last user message carries the tool_result the interception produced. + const last = req.messages.at(-1) as { content: Anthropic.ToolResultBlockParam[] }; + const block = last.content.find((b) => b.tool_use_id === 'tu-share-1'); + feedbackText = typeof block?.content === 'string' ? block.content : ''; + return Promise.resolve( + anthropicMsg([{ type: 'text', text: 'shared', citations: [] }], 'end_turn'), + ); + }); + + const adapter = new FakeAdapter(); // no postFile + const brain = new SdkBrain({ + createMessage, + callTool: (name, input) => + runtimeRef.interceptCallTool(name, input, (n, i) => innerCallTool(n, i)), + tools: [], + model: 'm', + extendedReasoning: false, + }); + const store = new SessionStore(() => brain.newSession()); + const mcp = { callTool: vi.fn(), onElicit: () => {} } as unknown as PeekMcp; const runtime = new ConnectorRuntime({ adapter, brain, mcp, store }); + runtimeRef = runtime; await runtime.start(); adapter.msgHandler?.({ conversationId: 'c1', userId: 'u', text: 'share' }); - await vi.waitFor(() => expect(adapter.consents).toHaveLength(1)); - - const correlationId = adapter.consents[0]?.[1].correlationId ?? ''; - adapter.consentHandler?.({ conversationId: 'c1', correlationId, decision: 'approve' }); - - // Should complete without crash. await vi.waitFor(() => expect(adapter.texts).toContainEqual(['c1', 'shared'])); - // Temp file should still be deleted. - await expect(rm(bundlePath, { force: false })).rejects.toThrow(); + // The degrade note references the filename, never the raw OS temp path. + expect(feedbackText).toContain('session.peekbundle'); + expect(feedbackText).not.toContain(bundlePath); + expect(feedbackText).not.toContain(dir); + // Temp file still deleted. + await expect(rm(bundlePath, { force: false })).rejects.toThrow(); await rm(dir, { recursive: true, force: true }); }); }); diff --git a/packages/connector-core/src/runtime.ts b/packages/connector-core/src/runtime.ts index c5504006..8f28c755 100644 --- a/packages/connector-core/src/runtime.ts +++ b/packages/connector-core/src/runtime.ts @@ -120,6 +120,81 @@ export class ConnectorRuntime { constructor(private readonly deps: RuntimeDeps) {} + /** + * Wrap a low-level `callTool` (typically `mcp.callTool`) so an approved + * `share_session` result is post-processed on the REAL path the brain + * actually takes. + * + * `share_session` is classified `'read'` by `classify()`, so `SdkBrain.runTurn` + * runs it INLINE via its injected `callTool` and feeds the result straight back + * into the tool-use loop — it never emits a `{kind:'consent'}` outcome and so + * never flows through `handleConsentResponse`. The bundle upload + temp-file + * cleanup therefore has to happen at THIS boundary (the callTool the brain + * consumes), not in the consent handler. + * + * The interception needs the surface adapter (`postFile`), which `PeekMcp` + * does not hold — the runtime does. So the runtime owns the wrapper and the + * composition root hands the brain `runtime.interceptCallTool(name, input, inner)` + * instead of the bare `mcp.callTool`. This keeps `PeekMcp` a pure transport and + * preserves the brain/runtime separation (the brain still only sees a plain + * `(name, input) => Promise`). + * + * The `conversationId` for the upload is read from `#activeConversationId`, + * which is set for the duration of the turn in `runLoop`. + */ + async interceptCallTool( + name: string, + input: unknown, + inner: (name: string, input: unknown) => Promise, + ): Promise { + const text = await inner(name, input); + if (name !== 'share_session') return text; + return this.#postProcessShareSession(this.#activeConversationId, text); + } + + /** + * Given a `share_session` tool-result string, upload the temp bundle via the + * surface adapter and delete it. Returns the (possibly rewritten) text to feed + * back to the brain. + * + * - `{ok:true, bundlePath, filename}` → `adapter.postFile(...)` inside `try`, + * temp bundle deleted in `finally` (success AND failure). + * - `{ok:false}` (or unparseable / missing fields) → returned unchanged; no + * upload, no throw, no delete (there is no temp file to remove). + * - adapter without `postFile` → degrade to a text note (referencing the + * `filename` only, never the raw OS temp path) and still delete the file. + * - upload error → surfaced to the brain as an error note; the turn continues. + */ + async #postProcessShareSession( + conversationId: string | undefined, + text: string, + ): Promise { + const { adapter } = this.deps; + let parsed: { ok: boolean; bundlePath?: string; filename?: string } | null = null; + try { + parsed = JSON.parse(text) as { ok: boolean; bundlePath?: string; filename?: string }; + } catch { + return text; // not valid JSON — leave as-is + } + if (!parsed?.ok || !parsed.bundlePath || !parsed.filename) return text; + const { bundlePath, filename } = parsed; + try { + if (adapter.postFile && conversationId) { + await adapter.postFile(conversationId, bundlePath, filename, 'peek session bundle'); + return `Session bundle "${filename}" shared successfully.`; + } + // No file-upload support (or no active conversation to post into): reference + // the filename only — never leak the raw OS temp path back to the model/user. + return `Session bundle "${filename}" is ready, but this surface cannot upload files, so it was not shared.`; + } catch (uploadErr) { + // Upload failed: surface a descriptive error to the brain so the + // conversation can continue. The finally block still deletes the file. + return `Session bundle export failed during upload: ${String(uploadErr)}`; + } finally { + await rm(bundlePath, { force: true }); + } + } + async start(): Promise { const { adapter, mcp, secretStore } = this.deps; // Load any previously-persisted pairing secret and arm it so every @@ -282,36 +357,11 @@ export class ConnectorRuntime { isError = true; } - // share_session interception: upload the temp bundle and delete it. - // The temp file is always deleted (try/finally) so session data is never - // left on disk, even if the upload fails. Upload errors are caught so the - // turn continues; the error is fed back to the brain as an error result. - if (pending.toolName === 'share_session' && !isError) { - let parsed: { ok: boolean; bundlePath?: string; filename?: string } | null = null; - try { - parsed = JSON.parse(text) as { ok: boolean; bundlePath?: string; filename?: string }; - } catch { - // Not valid JSON — leave text as-is and fall through. - } - if (parsed?.ok && parsed.bundlePath && parsed.filename) { - const { bundlePath, filename } = parsed; - try { - if (adapter.postFile) { - await adapter.postFile(r.conversationId, bundlePath, filename, 'peek session bundle'); - text = `Session bundle "${filename}" shared successfully.`; - } else { - text = `Session bundle ready at ${bundlePath} (filename: "${filename}"). The surface adapter does not support file upload.`; - } - } catch (uploadErr) { - // Upload failed: surface a descriptive error to the brain so the - // conversation can continue. The finally block still deletes the file. - text = `Session bundle export failed during upload: ${String(uploadErr)}`; - isError = true; - } finally { - await rm(bundlePath, { force: true }); - } - } - } + // NOTE: share_session is NOT intercepted here. It is classified 'read' by + // classify(), so SdkBrain runs it inline via its injected callTool and it + // never becomes a {kind:'consent'} outcome — so it never reaches this + // consent handler on the real path. The upload + temp-file cleanup lives at + // the callTool boundary in interceptCallTool()/#postProcessShareSession(). brain.appendToolResult(stored.session, pending.toolUseId, text, isError); await adapter.postConfirmation(r.conversationId, 'Approved — acting…'); diff --git a/packages/connector-slack/src/index.ts b/packages/connector-slack/src/index.ts index fbd90ae6..d3aa4074 100644 --- a/packages/connector-slack/src/index.ts +++ b/packages/connector-slack/src/index.ts @@ -46,9 +46,20 @@ async function main(): Promise { apiKey: brainConfig.apiKey, baseURL: brainConfig.baseURL, }); + // Forward-reference the runtime so the brain's callTool routes through + // runtime.interceptCallTool. share_session is 'read'-classified, so SdkBrain + // runs it inline via this callTool (never as a consent outcome) — the runtime + // wrapper is where the bundle upload + temp-file cleanup happens on the real + // path. The runtime is assigned below, before start(), so the reference is + // Forward reference: the brain's callTool closes over runtimeRef, but the runtime needs the + // brain at construction (dependency cycle). Assigned once below, before start(), so it is + // always resolved by the time any tool call fires. + // biome-ignore lint/style/useConst: forward reference for a construction-time dependency cycle + let runtimeRef: ConnectorRuntime; const brain = new SdkBrain({ createMessage: (req) => anthropic.messages.create(req), - callTool: (name, input) => mcp.callTool(name, input), + callTool: (name, input) => + runtimeRef.interceptCallTool(name, input, (n, i) => mcp.callTool(n, i)), tools, model: brainConfig.model, extendedReasoning: !brainConfig.baseURL, @@ -58,6 +69,7 @@ async function main(): Promise { const sessionStore = new SessionStore(() => brain.newSession()); const adapter = new SlackAdapter(slackConfig); const runtime = new ConnectorRuntime({ adapter, brain, mcp, store: sessionStore, secretStore }); + runtimeRef = runtime; // Determine paired-state before start() so we can decide whether to run the // first-run pairing flow. start() re-reads the same secret to arm the MCP From 57e46b88b86578e08156278eaa4b0af4961c594c Mon Sep 17 00:00:00 2001 From: harry-harish <22562634+harry-harish@users.noreply.github.com> Date: Thu, 9 Jul 2026 21:52:29 +0530 Subject: [PATCH 7/9] feat(connector-slack): postFile via files.uploadV2 for share_session Implements SurfaceAdapter.postFile on SlackAdapter using files.uploadV2. Reads the temp bundle with readFileSync; branches on threadTs presence to satisfy the FileThreadDestinationArgument vs FileChannelDestinationArgument discriminated union under exactOptionalPropertyTypes. Temp-file deletion stays with the runtime (Task 3). Three new tests cover the threaded, slash-command (no thread_ts), and upload-error paths. Co-Authored-By: Claude Opus 4.8 Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com> --- .../connector-slack/src/slack-adapter.test.ts | 91 +++++++++++++++++++ packages/connector-slack/src/slack-adapter.ts | 32 +++++++ 2 files changed, 123 insertions(+) diff --git a/packages/connector-slack/src/slack-adapter.test.ts b/packages/connector-slack/src/slack-adapter.test.ts index afd03f9e..9f75d885 100644 --- a/packages/connector-slack/src/slack-adapter.test.ts +++ b/packages/connector-slack/src/slack-adapter.test.ts @@ -2,6 +2,11 @@ import { describe, expect, it, vi } from 'vitest'; import { SlackAdapter, stripMention } from './slack-adapter.js'; import { parseConsentValue, suggestedPrompts } from './slack-adapter.js'; +// Mock node:fs so readFileSync is controllable in tests — must be hoisted before imports +vi.mock('node:fs', () => ({ + readFileSync: vi.fn(), +})); + describe('parseConsentValue', () => { it('parses a well-formed correlation payload', () => { expect( @@ -415,3 +420,89 @@ describe('SlackAdapter #activeThreads cap (Fix 4)', () => { expect(received).toEqual(['recent thread reply']); }); }); + +describe('SlackAdapter.postFile', () => { + // Import the mocked readFileSync after vi.mock hoisting resolves. + // We use a dynamic import inside each test so the mock is in scope. + + it('reads the file and calls files.uploadV2 with channel, thread_ts, bytes, filename, and initial_comment', async () => { + const { readFileSync } = await import('node:fs'); + const fakeBytes = Buffer.from('bundle-bytes'); + vi.mocked(readFileSync).mockReturnValue(fakeBytes); + + const uploadV2 = vi.fn().mockResolvedValue({}); + const { adapter } = makeAdapter(); + // Extend the mocked client with files.uploadV2 + (adapter as unknown as { app: { client: unknown } }).app.client = { + chat: { postMessage: vi.fn().mockResolvedValue({}) }, + assistant: { threads: { setStatus: vi.fn().mockResolvedValue({}) } }, + files: { uploadV2 }, + }; + // Seed the route so postFile can resolve channel + threadTs + ( + adapter as unknown as { routes: Map } + ).routes.set('conv-1', { channel: 'C10', threadTs: 'T10' }); + + await adapter.postFile( + 'conv-1', + '/tmp/bundle.peekbundle', + 'bundle.peekbundle', + 'peek session bundle', + ); + + expect(vi.mocked(readFileSync)).toHaveBeenCalledWith('/tmp/bundle.peekbundle'); + expect(uploadV2).toHaveBeenCalledTimes(1); + const arg = uploadV2.mock.calls[0]?.[0] as Record; + expect(arg.channel_id).toBe('C10'); + expect(arg.thread_ts).toBe('T10'); + expect(arg.file).toBe(fakeBytes); + expect(arg.filename).toBe('bundle.peekbundle'); + expect(arg.initial_comment).toBe('peek session bundle'); + }); + + it('omits thread_ts from the uploadV2 call when the route has no threadTs (slash path)', async () => { + const { readFileSync } = await import('node:fs'); + vi.mocked(readFileSync).mockReturnValue(Buffer.from('bytes')); + + const uploadV2 = vi.fn().mockResolvedValue({}); + const { adapter } = makeAdapter(); + (adapter as unknown as { app: { client: unknown } }).app.client = { + chat: { postMessage: vi.fn().mockResolvedValue({}) }, + assistant: { threads: { setStatus: vi.fn().mockResolvedValue({}) } }, + files: { uploadV2 }, + }; + // Route without threadTs (slash command path) — omit the key entirely (exactOptionalPropertyTypes) + ( + adapter as unknown as { routes: Map } + ).routes.set('cmd-C2-u1', { channel: 'C2' }); + + await adapter.postFile('cmd-C2-u1', '/tmp/a.peekbundle', 'a.peekbundle'); + + expect(uploadV2).toHaveBeenCalledTimes(1); + const arg = uploadV2.mock.calls[0]?.[0] as Record; + expect(arg.channel_id).toBe('C2'); + expect('thread_ts' in arg).toBe(false); // must be absent, not undefined + expect('initial_comment' in arg).toBe(false); // no comment → key must be absent + }); + + it('rejects when files.uploadV2 rejects (surfaces the error)', async () => { + const { readFileSync } = await import('node:fs'); + vi.mocked(readFileSync).mockReturnValue(Buffer.from('bytes')); + + const uploadErr = new Error('network error'); + const uploadV2 = vi.fn().mockRejectedValue(uploadErr); + const { adapter } = makeAdapter(); + (adapter as unknown as { app: { client: unknown } }).app.client = { + chat: { postMessage: vi.fn().mockResolvedValue({}) }, + assistant: { threads: { setStatus: vi.fn().mockResolvedValue({}) } }, + files: { uploadV2 }, + }; + ( + adapter as unknown as { routes: Map } + ).routes.set('conv-err', { channel: 'C3', threadTs: 'T3' }); + + await expect(adapter.postFile('conv-err', '/tmp/b.peekbundle', 'b.peekbundle')).rejects.toThrow( + 'network error', + ); + }); +}); diff --git a/packages/connector-slack/src/slack-adapter.ts b/packages/connector-slack/src/slack-adapter.ts index e091d267..50d6407e 100644 --- a/packages/connector-slack/src/slack-adapter.ts +++ b/packages/connector-slack/src/slack-adapter.ts @@ -1,3 +1,4 @@ +import { readFileSync } from 'node:fs'; import type { ConsentRequest, ConsentResponse, @@ -152,6 +153,37 @@ export class SlackAdapter implements SurfaceAdapter { await this.post(conversationId, blocks); } + async postFile( + conversationId: string, + filePath: string, + filename: string, + comment?: string, + ): Promise { + const r = this.route(conversationId); + const file = readFileSync(filePath); + // files.uploadV2 uses a discriminated union on thread_ts (exactOptionalPropertyTypes): + // FileThreadDestinationArgument — channel_id required + thread_ts required (string) + // FileChannelDestinationArgument — channel_id optional + thread_ts must be ABSENT (never) + // We must not spread a maybe-undefined thread_ts; branch explicitly so each call + // path satisfies exactly one union arm with no optional keys in the wrong positions. + if (r.threadTs !== undefined) { + await this.app.client.files.uploadV2({ + channel_id: r.channel, + thread_ts: r.threadTs, + file, + filename, + ...(comment !== undefined ? { initial_comment: comment } : {}), + }); + } else { + await this.app.client.files.uploadV2({ + channel_id: r.channel, + file, + filename, + ...(comment !== undefined ? { initial_comment: comment } : {}), + }); + } + } + private emit( conversationId: string, channel: string, From d6f7ada064c8bb24c264b17cd53919e857ac9fc6 Mon Sep 17 00:00:00 2001 From: harry-harish <22562634+harry-harish@users.noreply.github.com> Date: Thu, 9 Jul 2026 22:02:21 +0530 Subject: [PATCH 8/9] =?UTF-8?q?docs(peek):=20THREATMODEL=20=E2=80=94=20sha?= =?UTF-8?q?re=5Fsession=20egress?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add §"Session egress — share_session (SP5 / connector upload)" documenting the one path where a recorded session bundle leaves peek's local-first store and is uploaded to a third-party cloud (Slack via files.uploadV2). Covers: what the bundle contains (masked-at-capture DOM + console/network), the explicit egress consent card gate, the files:write scope requirement, temp-file lifecycle (deleted after upload, success or failure), audit log entry format, and honest residual limits (Slack retention + capture-time masking caveat). Also adds attack surface #9 to the enumeration at the top of the file. Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com> --- docs/peek/THREATMODEL.md | 81 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/docs/peek/THREATMODEL.md b/docs/peek/THREATMODEL.md index a2ee55db..575c3b82 100644 --- a/docs/peek/THREATMODEL.md +++ b/docs/peek/THREATMODEL.md @@ -42,6 +42,10 @@ project's pre-launch supply-chain hygiene controls (see 8. **Chrome Web Store update channel** — once published, CWS pushes updates to all installs. Trust boundary: Cubenest publisher account → all extension users. +9. **`share_session` connector upload** — the one path where a recorded + session bundle leaves the local store and is uploaded to a third-party + cloud (e.g. Slack via `files.uploadV2`). Trust boundary: peek local + store → third-party cloud service. ## Mitigations already in place @@ -183,6 +187,83 @@ independently auditable. | `consentDelegated` flag on local socket | Malicious local process forges flag to bypass Level-3 local banner for non-destructive actions | `mitigated` (SP4 pairing) | SW now also requires a valid pairing secret; an unpaired process falls through to the local banner. Unconditional destructive guard + TOCTOU re-check + Level ≥ 3 precondition still apply. | | Connector secret at rest (OS keychain by default; `0600` file fallback) | Local attacker reads the keychain entry / fallback file and presents the secret | `accepted` (local-peer-trust class) | Defends against unpaired processes, not a fully compromised user account. SP6a moved the secret to the OS keychain by default; `--insecure-store` selects the `0600` file. | +## Session egress — `share_session` (SP5 / connector upload) + +SP5 introduces the **one path where recorded session data leaves peek's +local-first store for a third-party cloud**. The `share_session` MCP tool +exports a recorded session as a `.peekbundle` and hands it to the active +connector for upload to its chat surface (e.g. a Slack thread via +`files.uploadV2`). + +### What the bundle contains + +A `.peekbundle` is a portable archive of the session's rrweb event stream — +DOM snapshots, console events, and network events. The content is **masked +at capture time** by `peek-extension/src/relay/mask.ts` before it ever reaches +the local store. The masking is a capture-time transformation, not a security +redaction: the bundle contains real session activity, just with PII fields +replaced per the masking rules in effect when the session was recorded. It is +not a sanitised summary. + +### Consent gate + +`share_session` never runs silently. Before producing any file the tool +presents an explicit egress consent card naming what is being exported and +where. The card uses the same elicitation consent mechanism as +`execute_action` (SP3b) and is independent of the Level-3 act gate: a user +at Level 1 can deny `execute_action` entirely and still encounter the +`share_session` consent card when the connector requests an upload. On +deny, no file is written and the tool returns `{ ok: false, result: 'denied' }`. + +### Slack upload path + +When the connector is `@peekdev/connector-slack`, the approved bundle is +uploaded to Slack via the `files.uploadV2` API. This requires the +`files:write` OAuth scope on the Slack app. The upload is made by the +connector process using the `xoxb` bot token stored in the OS keychain +(SP6b-1). peek itself does not hold or transmit the Slack token; the +connector receives the `bundlePath` from `share_session` and performs the +upload independently. + +### Temp-file lifecycle + +The `.peekbundle` is written to a process-scoped temp path with a random +nonce suffix. After the connector reports upload success or failure, the +connector-core layer deletes the temp file. The deletion is best-effort on +failure paths, and the temp path is in the OS temp directory (not +`~/.peek/`), so it is subject to normal OS temp-dir cleanup if the process +exits abnormally before deletion. + +### Audit log + +Every approved export is recorded to `~/.peek/audit.log` (mode `0600`) as +`tool: share_session`, including the session ID, the surface destination, +and the approver tag (e.g. `connector-elicit` for a delegated-consent +approval). The bundle bytes and the bundle path are never written to the +audit log. + +### Residual limits (honest framing) + +- **Once the bundle is in Slack it is under Slack's retention and access + controls**, not peek's. peek's local-first guarantees do not extend past + the upload boundary. Workspace admins, eDiscovery exports, and Slack's + own data-retention policies apply to the uploaded file. +- **Masking is capture-time, not security redaction.** If the masking + configuration at record time was incomplete (e.g. a custom input field + not covered by the default mask rules), the unmasked value may be in the + bundle. Users should treat the uploaded bundle with the same care as the + original session. +- **The consent card names the destination surface but not every downstream + recipient.** In a public Slack channel the uploaded file is visible to all + channel members. peek cannot enumerate channel membership at upload time. + +### Attack surface rows + +| Surface | Threat | Grade | Notes | +|---|---|---|---| +| `share_session` egress to Slack | Recorded session data (masked DOM + console/network) leaves the local store and enters Slack's cloud | `accepted` (explicit consent required) | Gated by an elicitation consent card before any file is written. Temp file deleted after upload. Audit log records the export. Residual: Slack retention applies post-upload; masking completeness depends on capture-time config. | +| Temp `.peekbundle` file on disk | Brief window between bundle write and upload deletion; a local process could read the file | `accepted` (local-peer-trust class) | Temp file has a random-nonce suffix. Sits in OS temp dir with normal umask. Deleted on success and on failure (best-effort). The window is bounded by the upload round-trip latency. | + ## Cross-references - [`docs/peek/PRIVACY_POLICY.md`](PRIVACY_POLICY.md) From d86af0d11637ac7392e66f864ca736eac779c89f Mon Sep 17 00:00:00 2001 From: harry-harish <22562634+harry-harish@users.noreply.github.com> Date: Thu, 9 Jul 2026 22:29:21 +0530 Subject: [PATCH 9/9] fix(share-session): address CodeRabbit CHANGES_REQUESTED (3 major + 3 nitpicks) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MAJOR 1 (connector-core/runtime.ts): wrap rm() in finally in its own try/catch so an EACCES/EBUSY cleanup failure never clobbers the real upload result returned by the try/catch above. MAJOR 2 (peek-mcp/elicitation.ts): sanitize surface like sessionId — add safeSurface = surface.replace(/[^\x20-\x7E]/g,'_').slice(0,60) before interpolating into the egress consent card to prevent newline/control-char injection. MAJOR 3 (peek-mcp/server.ts): add appendAuditEntry (result:'error', approver:'connector-elicit') to all 4 post-consent error returns (no DB, session-not-found, event-load failure, pack failure), each wrapped in its own try/catch. Add test: approved unknown session → audit entry with result:'error' and approver:'connector-elicit'. NITPICK A (connector-slack/slack-adapter.ts): switch readFileSync → async readFile from node:fs/promises; update test mock accordingly. NITPICK B (server.ts): add .max(60) cap to surface zod schema. NITPICK C (server.ts): document temp-file ownership in tool description (connector uploads-and-deletes; direct caller is responsible). Skipped: audit.ts:380-399 (pre-existing, 0 diff lines in this PR). Deferred: server.ts 1448-1492 SQL helpers (in diff but non-trivial refactor, correctness-neutral, follow-up cleanup). Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com> --- packages/connector-core/src/runtime.ts | 10 ++- .../connector-slack/src/slack-adapter.test.ts | 20 ++--- packages/connector-slack/src/slack-adapter.ts | 4 +- packages/peek-mcp/src/mcp/elicitation.ts | 6 +- packages/peek-mcp/src/mcp/server.ts | 79 ++++++++++++++++++- packages/peek-mcp/test/share-session.test.ts | 36 +++++++++ 6 files changed, 138 insertions(+), 17 deletions(-) diff --git a/packages/connector-core/src/runtime.ts b/packages/connector-core/src/runtime.ts index 8f28c755..da61e597 100644 --- a/packages/connector-core/src/runtime.ts +++ b/packages/connector-core/src/runtime.ts @@ -191,7 +191,15 @@ export class ConnectorRuntime { // conversation can continue. The finally block still deletes the file. return `Session bundle export failed during upload: ${String(uploadErr)}`; } finally { - await rm(bundlePath, { force: true }); + try { + await rm(bundlePath, { force: true }); + } catch (cleanupErr) { + // Cleanup errors (e.g. EACCES, EBUSY) must never clobber the real return + // value from the try/catch above. Log and swallow. + console.error( + `peek connector: failed to delete temp bundle ${bundlePath} — ${cleanupErr instanceof Error ? cleanupErr.message : String(cleanupErr)}`, + ); + } } } diff --git a/packages/connector-slack/src/slack-adapter.test.ts b/packages/connector-slack/src/slack-adapter.test.ts index 9f75d885..be9a9e7c 100644 --- a/packages/connector-slack/src/slack-adapter.test.ts +++ b/packages/connector-slack/src/slack-adapter.test.ts @@ -2,9 +2,9 @@ import { describe, expect, it, vi } from 'vitest'; import { SlackAdapter, stripMention } from './slack-adapter.js'; import { parseConsentValue, suggestedPrompts } from './slack-adapter.js'; -// Mock node:fs so readFileSync is controllable in tests — must be hoisted before imports -vi.mock('node:fs', () => ({ - readFileSync: vi.fn(), +// Mock node:fs/promises so readFile is controllable in tests — must be hoisted before imports +vi.mock('node:fs/promises', () => ({ + readFile: vi.fn(), })); describe('parseConsentValue', () => { @@ -426,9 +426,9 @@ describe('SlackAdapter.postFile', () => { // We use a dynamic import inside each test so the mock is in scope. it('reads the file and calls files.uploadV2 with channel, thread_ts, bytes, filename, and initial_comment', async () => { - const { readFileSync } = await import('node:fs'); + const { readFile } = await import('node:fs/promises'); const fakeBytes = Buffer.from('bundle-bytes'); - vi.mocked(readFileSync).mockReturnValue(fakeBytes); + vi.mocked(readFile).mockResolvedValue(fakeBytes); const uploadV2 = vi.fn().mockResolvedValue({}); const { adapter } = makeAdapter(); @@ -450,7 +450,7 @@ describe('SlackAdapter.postFile', () => { 'peek session bundle', ); - expect(vi.mocked(readFileSync)).toHaveBeenCalledWith('/tmp/bundle.peekbundle'); + expect(vi.mocked(readFile)).toHaveBeenCalledWith('/tmp/bundle.peekbundle'); expect(uploadV2).toHaveBeenCalledTimes(1); const arg = uploadV2.mock.calls[0]?.[0] as Record; expect(arg.channel_id).toBe('C10'); @@ -461,8 +461,8 @@ describe('SlackAdapter.postFile', () => { }); it('omits thread_ts from the uploadV2 call when the route has no threadTs (slash path)', async () => { - const { readFileSync } = await import('node:fs'); - vi.mocked(readFileSync).mockReturnValue(Buffer.from('bytes')); + const { readFile } = await import('node:fs/promises'); + vi.mocked(readFile).mockResolvedValue(Buffer.from('bytes')); const uploadV2 = vi.fn().mockResolvedValue({}); const { adapter } = makeAdapter(); @@ -486,8 +486,8 @@ describe('SlackAdapter.postFile', () => { }); it('rejects when files.uploadV2 rejects (surfaces the error)', async () => { - const { readFileSync } = await import('node:fs'); - vi.mocked(readFileSync).mockReturnValue(Buffer.from('bytes')); + const { readFile } = await import('node:fs/promises'); + vi.mocked(readFile).mockResolvedValue(Buffer.from('bytes')); const uploadErr = new Error('network error'); const uploadV2 = vi.fn().mockRejectedValue(uploadErr); diff --git a/packages/connector-slack/src/slack-adapter.ts b/packages/connector-slack/src/slack-adapter.ts index 50d6407e..8655d160 100644 --- a/packages/connector-slack/src/slack-adapter.ts +++ b/packages/connector-slack/src/slack-adapter.ts @@ -1,4 +1,4 @@ -import { readFileSync } from 'node:fs'; +import { readFile } from 'node:fs/promises'; import type { ConsentRequest, ConsentResponse, @@ -160,7 +160,7 @@ export class SlackAdapter implements SurfaceAdapter { comment?: string, ): Promise { const r = this.route(conversationId); - const file = readFileSync(filePath); + const file = await readFile(filePath); // files.uploadV2 uses a discriminated union on thread_ts (exactOptionalPropertyTypes): // FileThreadDestinationArgument — channel_id required + thread_ts required (string) // FileChannelDestinationArgument — channel_id optional + thread_ts must be ABSENT (never) diff --git a/packages/peek-mcp/src/mcp/elicitation.ts b/packages/peek-mcp/src/mcp/elicitation.ts index cf512fb7..c612d94c 100644 --- a/packages/peek-mcp/src/mcp/elicitation.ts +++ b/packages/peek-mcp/src/mcp/elicitation.ts @@ -113,9 +113,13 @@ export function buildEgressConsentMessage(sessionId: string, surface: string): s // Sanitize sessionId for display: strip control characters (incl. newlines) so // a crafted id cannot inject into the consent card text. const safeSessionId = sessionId.replace(/[^\x20-\x7E]/g, '_'); + // Sanitize surface the same way: it is caller-supplied and interpolated into + // the consent card, so a crafted value could inject newlines/control chars to + // spoof the approval prompt. + const safeSurface = surface.replace(/[^\x20-\x7E]/g, '_').slice(0, 60); return ( `peek wants to upload this session's bundle (${safeSessionId} — recorded DOM + ` + - `console/network, masked) to ${surface}. This data leaves your local-first peek store. Approve?` + `console/network, masked) to ${safeSurface}. This data leaves your local-first peek store. Approve?` ); } diff --git a/packages/peek-mcp/src/mcp/server.ts b/packages/peek-mcp/src/mcp/server.ts index 061df944..4b567ce3 100644 --- a/packages/peek-mcp/src/mcp/server.ts +++ b/packages/peek-mcp/src/mcp/server.ts @@ -1157,11 +1157,14 @@ export function createPeekMcpServer(options: CreatePeekMcpServerOptions = {}): P 'Export a session as a portable .peekbundle file to share with teammates (e.g. upload to a Slack thread). ' + 'Requires explicit egress consent — a card naming what is exported and where; deny produces no file. ' + 'On approve, returns the temp file path and size; the bundle never leaves your machine without consent. ' + - 'The bundle contains the masked session recording (DOM + console/network); review the caveat before sharing.', + 'The bundle contains the masked session recording (DOM + console/network); review the caveat before sharing. ' + + 'The returned bundlePath is a temp file: a connector (e.g. connector-core) uploads it and deletes it automatically; ' + + 'a direct MCP caller (Claude Desktop, MCP Inspector) is responsible for reading and deleting the returned path.', inputSchema: { sessionId: z.string().describe('Session id from list_recent_sessions.'), surface: z .string() + .max(60) .default('Slack') .describe( "The surface the bundle will be shared to (e.g. 'Slack', 'Discord'). Shown in the consent card.", @@ -1428,20 +1431,72 @@ export function createPeekMcpServer(options: CreatePeekMcpServerOptions = {}): P // Approved — load session data and build the bundle. const handle = getDb(); if (!handle) { + const noDbDraft: DraftAuditEntry = { + ts: new Date(requestStartedAtMs).toISOString(), + tool: 'share_session', + args: { sessionId: input.sessionId, surface: input.surface }, + approver: 'connector-elicit', + client, + sessionId: input.sessionId, + result: 'error', + error: NO_DB_MESSAGE, + }; + try { + appendAuditEntry(noDbDraft, auditWriteOptions); + } catch (auditErr) { + console.error( + `peek-mcp: share_session audit log write failed — ${auditErr instanceof Error ? auditErr.message : String(auditErr)}`, + ); + } return jsonResult({ ok: false, result: 'error', error: NO_DB_MESSAGE }); } const row = getSessionSummaryRow(handle, input.sessionId); if (!row) { + const notFoundError = `No session found with id '${input.sessionId}'.`; + const notFoundDraft: DraftAuditEntry = { + ts: new Date(requestStartedAtMs).toISOString(), + tool: 'share_session', + args: { sessionId: input.sessionId, surface: input.surface }, + approver: 'connector-elicit', + client, + sessionId: input.sessionId, + result: 'error', + error: notFoundError, + }; + try { + appendAuditEntry(notFoundDraft, auditWriteOptions); + } catch (auditErr) { + console.error( + `peek-mcp: share_session audit log write failed — ${auditErr instanceof Error ? auditErr.message : String(auditErr)}`, + ); + } return jsonResult({ ok: false, result: 'error', - error: `No session found with id '${input.sessionId}'.`, + error: notFoundError, }); } // Load events (same path as generate_playwright_repro). const ev = eventsFor(input.sessionId); if (!ev.ok) { + const evLoadDraft: DraftAuditEntry = { + ts: new Date(requestStartedAtMs).toISOString(), + tool: 'share_session', + args: { sessionId: input.sessionId, surface: input.surface }, + approver: 'connector-elicit', + client, + sessionId: input.sessionId, + result: 'error', + error: ev.message, + }; + try { + appendAuditEntry(evLoadDraft, auditWriteOptions); + } catch (auditErr) { + console.error( + `peek-mcp: share_session audit log write failed — ${auditErr instanceof Error ? auditErr.message : String(auditErr)}`, + ); + } return jsonResult({ ok: false, result: 'error', error: ev.message }); } @@ -1518,10 +1573,28 @@ export function createPeekMcpServer(options: CreatePeekMcpServerOptions = {}): P // { ok:false } with an audit entry rather than propagating as an unhandled throw. sizeBytes = statSync(bundlePath).size; } catch (err) { + const packError = `Failed to pack bundle: ${err instanceof Error ? err.message : String(err)}`; + const packFailDraft: DraftAuditEntry = { + ts: new Date(requestStartedAtMs).toISOString(), + tool: 'share_session', + args: { sessionId: input.sessionId, surface: input.surface }, + approver: 'connector-elicit', + client, + sessionId: input.sessionId, + result: 'error', + error: packError, + }; + try { + appendAuditEntry(packFailDraft, auditWriteOptions); + } catch (auditErr) { + console.error( + `peek-mcp: share_session audit log write failed — ${auditErr instanceof Error ? auditErr.message : String(auditErr)}`, + ); + } return jsonResult({ ok: false, result: 'error', - error: `Failed to pack bundle: ${err instanceof Error ? err.message : String(err)}`, + error: packError, }); } diff --git a/packages/peek-mcp/test/share-session.test.ts b/packages/peek-mcp/test/share-session.test.ts index 35e57d7f..f5e0b73f 100644 --- a/packages/peek-mcp/test/share-session.test.ts +++ b/packages/peek-mcp/test/share-session.test.ts @@ -397,6 +397,42 @@ describe('share_session — approve', () => { await close(); } }); + + it('(c) approved unknown session → audit entry with result:error, approver:connector-elicit, no bundle bytes', async () => { + // CodeRabbit MAJOR 3: approved-but-errored calls must write an audit entry + // so the log never misses a record that consent was granted. + const { dbPath, eventsDir } = seedSessionWithData(dir); + const auditLogPath = join(dir, 'audit.log'); + const { client, peek, close } = await connectClient({ dbPath, eventsDir, auditLogPath }); + try { + stubElicitation(peek, 'accept'); + + await client.callTool({ + name: 'share_session', + arguments: { sessionId: 's_does-not-exist', surface: 'Slack' }, + }); + + // An audit entry must have been written even though the operation failed. + const contents = readFileSync(auditLogPath, 'utf8'); + const lines = contents.split('\n').filter((l) => l.length > 0); + expect(lines.length).toBeGreaterThanOrEqual(1); + const entry = JSON.parse(lines[0] as string) as Record; + + // Must record the consent was granted (approver=connector-elicit) and that it errored. + expect(entry.tool).toBe('share_session'); + expect(entry.result).toBe('error'); + expect(entry.approver).toBe('connector-elicit'); + expect(entry.sessionId).toBe('s_does-not-exist'); + + // Must not contain any bundle bytes. + const entryStr = JSON.stringify(entry); + expect(entryStr).not.toContain('events.json'); + expect(entryStr).not.toContain('session.json'); + expect(entryStr).not.toContain('FullSnapshot'); + } finally { + await close(); + } + }); }); describe('share_session — tool is registered', () => {