diff --git a/HEAD b/HEAD deleted file mode 100644 index e69de29b..00000000 diff --git a/backend/services/agentMentionService.ts b/backend/services/agentMentionService.ts index 5159aba8..44c43ddf 100644 --- a/backend/services/agentMentionService.ts +++ b/backend/services/agentMentionService.ts @@ -363,7 +363,11 @@ const formatPodContextFrame = (podId: string): string => `[Pod context: this conversation is in pod \`${podId}\`. ` + `When attaching files, call commonly_attach_file({ podId: "${podId}", filePath, message }). ` + `When reading files referenced via [[upload:fileName|...]] in this thread, call ` + - `commonly_read_attachment({ fileName }) — it returns the extracted text in one shot.]`; + `commonly_read_attachment({ fileName }) — it returns the extracted text in one shot. ` + + `Post as yourself only: reply text is delivered under your own agent identity, and any ` + + `mid-turn post must use your own runtime token (commonly_post_message / your token file). ` + + `Never post through an operator's CLI profile (\`commonly pod send\`) or a human user's ` + + `token — that misattributes your words to a human.]`; // Cross-runtime consultation cue. Companion to the pod-context frame // above; same rationale (inline cue beats structured metadata per diff --git a/cli/__tests__/adapters.codex.test.mjs b/cli/__tests__/adapters.codex.test.mjs index 30994b3c..417ba477 100644 --- a/cli/__tests__/adapters.codex.test.mjs +++ b/cli/__tests__/adapters.codex.test.mjs @@ -133,6 +133,61 @@ describe('codex adapter — spawn()', () => { expect(calls[0].args[calls[0].args.length - 1]).toBe('hi'); }); + test('environment.mcp servers become -c mcp_servers.* overrides with substituted token/env', async () => { + // Regression for the 2026-07-22 as-operator attribution incident: the + // adapter used to silently ignore environment.mcp, so a codex agent had + // no commonly_* tools and fell back to posting via the operator's CLI + // profile (misattributing its words to the human). + const { impl, calls } = makeSpawnImpl({ + stdoutChunks: ['{"type":"turn.completed"}\n'], + outputContents: 'ok', + }); + + await codex.spawn('hi', { + sessionId: null, + _spawnImpl: impl, + runtimeToken: 'cm_agent_secret', + instanceUrl: 'https://api.example.test', + environment: { + mcp: [ + { + name: 'commonly', + transport: 'stdio', + command: ['npx', '-y', '@commonlyai/mcp@latest'], + env: { + COMMONLY_API_URL: '${COMMONLY_API_URL}', + COMMONLY_AGENT_TOKEN: '${COMMONLY_AGENT_TOKEN}', + }, + }, + // url-only server: codex has no url transport — must be skipped. + { name: 'remote-only', transport: 'http', url: 'https://mcp.example.test' }, + ], + }, + }); + + const args = calls[0].args; + const cFlags = args + .map((a, i) => (a === '-c' ? args[i + 1] : null)) + .filter(Boolean); + expect(cFlags).toEqual([ + 'mcp_servers.commonly.command="npx"', + 'mcp_servers.commonly.args=["-y","@commonlyai/mcp@latest"]', + 'mcp_servers.commonly.env={COMMONLY_API_URL = "https://api.example.test", COMMONLY_AGENT_TOKEN = "cm_agent_secret"}', + ]); + // Overrides must precede the prompt (last arg) and not disturb -o pairing. + expect(findOutputFile(args)).toBeTruthy(); + expect(args[args.length - 1]).toBe('hi'); + }); + + test('no environment.mcp → no -c flags (argv unchanged for MCP-less agents)', async () => { + const { impl, calls } = makeSpawnImpl({ + stdoutChunks: ['{"type":"turn.completed"}\n'], + outputContents: 'ok', + }); + await codex.spawn('hi', { sessionId: null, _spawnImpl: impl }); + expect(calls[0].args).not.toContain('-c'); + }); + test('spawn opts force stdin to ignore — regression for codex blocking on piped stdin', async () => { // Without this, codex 0.125.0 blocks on "Reading additional input from // stdin..." when spawned from a non-TTY parent (e.g. the run loop). diff --git a/cli/__tests__/attach.test.mjs b/cli/__tests__/attach.test.mjs index 1ea5cc49..05221637 100644 --- a/cli/__tests__/attach.test.mjs +++ b/cli/__tests__/attach.test.mjs @@ -178,14 +178,17 @@ describe('performAttach', () => { }); describe('buildDefaultEnvironment', () => { - test('returns null for adapters that do not consume --mcp-config (codex, stub)', () => { - expect(buildDefaultEnvironment('codex')).toBeNull(); + test('returns null for adapters with no MCP consumption path (stub)', () => { expect(buildDefaultEnvironment('stub')).toBeNull(); expect(buildDefaultEnvironment('does-not-exist')).toBeNull(); }); - test('returns a single mcp entry for claude with placeholder env values', () => { - const env = buildDefaultEnvironment('claude'); + test.each(['claude', 'codex'])('returns a single mcp entry for %s with placeholder env values', (adapterName) => { + // codex joined the set after the 2026-07-22 as-operator attribution + // incident: an MCP-less codex agent has no sanctioned posting tool and + // falls back to whatever it finds in the shell (the operator's CLI + // profile — posting AS the human). + const env = buildDefaultEnvironment(adapterName); expect(env.mcp).toHaveLength(1); expect(env.mcp[0].name).toBe('commonly'); expect(env.mcp[0].transport).toBe('stdio'); diff --git a/cli/__tests__/run-loop.test.mjs b/cli/__tests__/run-loop.test.mjs index 5c8d5280..31ad28c8 100644 --- a/cli/__tests__/run-loop.test.mjs +++ b/cli/__tests__/run-loop.test.mjs @@ -146,6 +146,87 @@ describe('performRun', () => { ); }); + test('self-post detection: a new BOT message during the spawn suppresses the echo', async () => { + // The agent posted mid-turn via commonly_post_message (its own identity) — + // echoing its CLI text would double-post. + let messagesCall = 0; + const mockGet = jest.fn(async (route) => { + if (route === '/api/agents/runtime/memory') return { sections: {} }; + if (route.endsWith('/messages')) { + messagesCall += 1; + return messagesCall === 1 + ? { messages: [{ _id: 'm1', isBot: false }] } + : { messages: [{ _id: 'm1', isBot: false }, { _id: 'm2', isBot: true }] }; + } + return { events: [makeEvent({ _id: 'evt-selfpost' })] }; + }); + const mockPost = jest.fn().mockResolvedValue({}); + createClient.mockReturnValue({ get: mockGet, post: mockPost }); + + const spawn = jest.fn(async () => ({ text: 'narration of what I posted' })); + const adapter = { name: 'stub', detect: stubAdapter.detect, spawn }; + + const { stop } = performRun({ + instanceUrl: 'http://localhost:5000', + token: 'cm_agent_test', + adapter, + agentName: 'my-stub', + setTimeoutImpl: noopTimeout, + }); + await drainMicrotasks(); + stop(); + + expect(mockPost).not.toHaveBeenCalledWith( + '/api/agents/runtime/pods/pod-abc/messages', + { content: 'narration of what I posted' }, + ); + // Turn still acks so the kernel doesn't re-deliver. + expect(mockPost).toHaveBeenCalledWith( + '/api/agents/runtime/events/evt-selfpost/ack', + { result: { outcome: 'posted' } }, + ); + }); + + test('self-post detection: a new HUMAN message during the spawn does NOT suppress the echo', async () => { + // Regression for the 2026-07-22 as-operator attribution incident: an agent + // posting through the operator's CLI profile lands a HUMAN-authored message + // mid-spawn. The old id-only detection treated that as "agent posted + // itself" and swallowed the correctly-attributed wrapper reply, so the + // misattributed copy was the only one in the room. A human-authored new + // message (isBot: false) must not suppress the echo. + let messagesCall = 0; + const mockGet = jest.fn(async (route) => { + if (route === '/api/agents/runtime/memory') return { sections: {} }; + if (route.endsWith('/messages')) { + messagesCall += 1; + return messagesCall === 1 + ? { messages: [{ _id: 'm1', isBot: false }] } + : { messages: [{ _id: 'm1', isBot: false }, { _id: 'm2', isBot: false }] }; + } + return { events: [makeEvent({ _id: 'evt-humanpost' })] }; + }); + const mockPost = jest.fn().mockResolvedValue({}); + createClient.mockReturnValue({ get: mockGet, post: mockPost }); + + const spawn = jest.fn(async () => ({ text: 'my real reply' })); + const adapter = { name: 'stub', detect: stubAdapter.detect, spawn }; + + const { stop } = performRun({ + instanceUrl: 'http://localhost:5000', + token: 'cm_agent_test', + adapter, + agentName: 'my-stub', + setTimeoutImpl: noopTimeout, + }); + await drainMicrotasks(); + stop(); + + expect(mockPost).toHaveBeenCalledWith( + '/api/agents/runtime/pods/pod-abc/messages', + { content: 'my real reply' }, + ); + }); + test('ensures adapter cwd exists before spawning — avoids confusing spawn ENOENT on missing dir', async () => { const agentCwd = path.join(os.tmpdir(), 'commonly-agents', 'cwd-fresh-agent'); fs.rmSync(agentCwd, { recursive: true, force: true }); diff --git a/cli/skills/commonly/SKILL.md b/cli/skills/commonly/SKILL.md index a4368c99..b003d05f 100644 --- a/cli/skills/commonly/SKILL.md +++ b/cli/skills/commonly/SKILL.md @@ -120,6 +120,14 @@ turn with the literal `NO_REPLY`. So: if you already said everything through `NO_REPLY` so the wrapper doesn't post a duplicate. If you *didn't* post via the tool, just let your reply be your final output. Either way — one voice, no echo. +**Post as yourself, never as your operator.** Your reply text and your +`commonly_*` tools carry *your* agent identity. If you have shell access, you may +find an operator's Commonly CLI profile (`commonly pod send`, `~/.commonly/config.json`) +or a human's saved token in your environment — **never post through them**. A message +sent that way appears in the room under the human's name and avatar, which +misattributes your words and breaks the room's provenance. If your own tools are +unavailable mid-turn, say what you need in your final reply instead. + ## The task board Pods have a task board. When work is being tracked: diff --git a/cli/src/commands/agent.js b/cli/src/commands/agent.js index e11d0afd..574ce0cf 100644 --- a/cli/src/commands/agent.js +++ b/cli/src/commands/agent.js @@ -113,12 +113,13 @@ const PROMPT_EVENT_TYPES = new Set([ // ── default environment for adapters that benefit from auto-MCP wiring ───── // Adapters that can consume `mcp[]` from the resolved environment spec. -// `claude` reads it via --mcp-config (see adapters/claude.js). `codex` and -// `stub` do not (codex uses ~/.codex/config.toml at boot, set up server-side -// for cloud-codex; the local codex wrapper doesn't ship MCP wiring yet — -// follow-up after #440). Returning null means "no default" — the wrapper -// proceeds with environment=null exactly like before this change. -const ADAPTERS_WITH_DEFAULT_MCP = new Set(['claude']); +// `claude` reads it via --mcp-config (see adapters/claude.js); `codex` via +// `-c mcp_servers.*` config overrides (see adapters/codex.js — added after +// the 2026-07-22 as-operator attribution incident, where an MCP-less codex +// agent posted through the operator's CLI profile because it had no +// commonly_* tools of its own). `stub` does not. Returning null means "no +// default" — the wrapper proceeds with environment=null exactly like before. +const ADAPTERS_WITH_DEFAULT_MCP = new Set(['claude', 'codex']); // The commonly behavior skill ships inside this package (cli/skills/commonly) // so a fresh attach can mount it into the agent without a network fetch. @@ -437,7 +438,7 @@ export const performRun = ({ // and (if the adapter returns a summary) patch-sync back after. const memoryLongTerm = await readLongTerm(client, { onError }); - // Snapshot the pod's recent message ids so that, after the spawn, we can + // Snapshot the pod's recent messages so that, after the spawn, we can // tell whether the agent posted itself via commonly_post_message. If it // did, its final CLI text is a narration/log — echoing it would duplicate // the message and re-fire any @mention. This is the wrapper-side guarantee @@ -445,17 +446,20 @@ export const performRun = ({ // without relying on the agent to emit NO_REPLY. (A rare concurrent post by // another member during the spawn window can suppress a genuine wrapper // reply — an acceptable trade against a guaranteed double-post.) - const snapshotMessageIds = async () => { + const snapshotMessages = async () => { try { const { messages = [] } = await client.get( `/api/agents/runtime/pods/${eventPodId}/messages`, { limit: 10 }, ); - return new Set(messages.map((m) => String(m._id || m.id))); + return messages; } catch { return null; // detection unavailable — fall back to posting the reply } }; - const preSpawnIds = await snapshotMessageIds(); + const preSpawn = await snapshotMessages(); + const preSpawnIds = preSpawn + ? new Set(preSpawn.map((m) => String(m._id || m.id))) + : null; log(`[${event.type}] spawning ${adapter.name}`); const result = await adapter.spawn(prompt, { @@ -487,10 +491,19 @@ export const performRun = ({ const replyText = (result.text || '').trim(); let agentPostedItself = false; if (preSpawnIds) { - const postSpawnIds = await snapshotMessageIds(); - if (postSpawnIds) { - for (const id of postSpawnIds) { - if (!preSpawnIds.has(id)) { agentPostedItself = true; break; } + const postSpawn = await snapshotMessages(); + if (postSpawn) { + for (const m of postSpawn) { + // Only a NEW message from a bot user counts as "the agent posted + // itself". A new human-authored message must NOT suppress the echo: + // it is either a human typing mid-turn, or the agent misusing an + // operator CLI profile / human token (the 2026-07-22 as-operator + // attribution incident) — in both cases the wrapper still delivers + // the reply under the agent's own identity. + if (!preSpawnIds.has(String(m._id || m.id)) && m.isBot) { + agentPostedItself = true; + break; + } } } } diff --git a/cli/src/lib/adapters/codex.js b/cli/src/lib/adapters/codex.js index a0d7ac60..c139fae1 100644 --- a/cli/src/lib/adapters/codex.js +++ b/cli/src/lib/adapters/codex.js @@ -71,10 +71,64 @@ const buildPrompt = (prompt, memoryLongTerm) => { return `=== Context (your persistent memory) ===\n${memoryLongTerm}\n=== Current turn ===\n${prompt}`; }; +// ── MCP wiring — codex consumes MCP servers via `-c mcp_servers.*` overrides ─ +// +// Same substitution contract as claude.js (see that file for the placeholder +// rationale): ${COMMONLY_AGENT_TOKEN} / ${COMMONLY_API_URL} in the declared +// env spec are replaced with the wrapper's per-(agent, pod) runtime values at +// spawn time. Before this block the codex adapter silently ignored +// `environment.mcp`, so a codex agent had NO sanctioned posting tool — the +// 2026-07-22 as-operator attribution incident was a codex agent falling back +// to the operator's CLI profile because commonly_* tools were never wired. +// +// Codex-specific constraints: +// - stdio/command servers only (no url transport here); url-only entries +// are skipped rather than half-wired. +// - Declared env MUST ride inside the mcp_servers..env table — codex +// does not pass its parent env to the MCP child it spawns (the PR #398 +// cloud-codex lesson; same failure mode locally). +const SUBSTITUTION_KEYS = ['COMMONLY_AGENT_TOKEN', 'COMMONLY_API_URL', 'COMMONLY_INSTANCE_URL']; +const PLACEHOLDER_RE = /\$\{(COMMONLY_[A-Z_]+)\}/g; + +const substitutePlaceholders = (value, ctx) => { + if (typeof value !== 'string') return value; + if (!value.includes('${COMMONLY_')) return value; + const subs = { + COMMONLY_AGENT_TOKEN: ctx.runtimeToken || '', + COMMONLY_API_URL: ctx.instanceUrl || '', + COMMONLY_INSTANCE_URL: ctx.instanceUrl || '', + }; + return value.replace(PLACEHOLDER_RE, (whole, key) => ( + SUBSTITUTION_KEYS.includes(key) && subs[key] ? subs[key] : whole + )); +}; + +// JSON string escaping is a valid subset of TOML basic-string escaping, so +// JSON.stringify doubles as the TOML quoter for command/args/env values. +const toml = (s) => JSON.stringify(String(s)); + +const buildMcpOverrideArgs = (mcpServers, ctx = {}) => { + const flags = []; + for (const server of mcpServers || []) { + if (!server?.name || !Array.isArray(server.command) || !server.command.length) continue; + const [command, ...rest] = server.command.map((a) => substitutePlaceholders(a, ctx)); + flags.push('-c', `mcp_servers.${server.name}.command=${toml(command)}`); + if (rest.length) { + flags.push('-c', `mcp_servers.${server.name}.args=[${rest.map(toml).join(',')}]`); + } + const envEntries = Object.entries(server.env || {}) + .map(([k, v]) => `${k} = ${toml(substitutePlaceholders(v, ctx))}`); + if (envEntries.length) { + flags.push('-c', `mcp_servers.${server.name}.env={${envEntries.join(', ')}}`); + } + } + return flags; +}; + // Build the argv after the `codex` binary. Resume vs new turn is a // subcommand-level distinction in modern codex, not an option flag — keep // that detail isolated here so the spawn path stays linear. -const buildArgs = ({ sessionId, prompt, outputFile }) => { +const buildArgs = ({ sessionId, prompt, outputFile, mcpFlags = [] }) => { // `--dangerously-bypass-approvals-and-sandbox` disables codex CLI's // bubblewrap (bwrap) sandbox + approval prompts. bwrap needs CAP_SYS_ADMIN // or unprivileged user-namespaces — neither available to standard k8s @@ -90,6 +144,7 @@ const buildArgs = ({ sessionId, prompt, outputFile }) => { '--json', '--skip-git-repo-check', '--dangerously-bypass-approvals-and-sandbox', + ...mcpFlags, '-o', outputFile, ]; @@ -219,6 +274,10 @@ export default { sessionId: ctx.sessionId || null, prompt: fullPrompt, outputFile, + mcpFlags: buildMcpOverrideArgs(ctx.environment?.mcp, { + runtimeToken: ctx.runtimeToken, + instanceUrl: ctx.instanceUrl, + }), }); const { threadId } = await runCodex({