From 0eac74f014450c58dda3efbf8f41c7137142a35c Mon Sep 17 00:00:00 2001 From: Sam Xu Date: Sat, 25 Apr 2026 17:07:43 -0700 Subject: [PATCH] feat(cli): substitute \${COMMONLY_AGENT_TOKEN} / \${COMMONLY_API_URL} in MCP config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lets users keep their checked-in env files free of secrets — the wrapper substitutes the runtime token + instance URL at spawn time from values it already has on hand (the saved token record). Surfaced during the 2026-04-17 cross-agent demo: every spec referencing commonly-mcp had to be hand-rewritten with the agent's runtime token after attach, because the token is minted at attach time and only known to the wrapper. Recognised placeholders (substituted everywhere a string appears in the MCP config — env values, command args, and url fields): \${COMMONLY_AGENT_TOKEN} — per-(agent, pod) cm_agent_* runtime token \${COMMONLY_API_URL} — instance URL the agent is attached to \${COMMONLY_INSTANCE_URL} — alias for COMMONLY_API_URL One-pass + literal substitution; no nested expansion, no shell quoting. Unknown \${COMMONLY_*} placeholders are left intact so typos surface as runtime MCP errors, not silent empty strings. Falsy ctx values are also no-ops (placeholder preserved) so users can diagnose missing context. Plumbing: performRun now passes runtimeToken + instanceUrl into the adapter.spawn ctx; claude adapter passes them through to writeMcpConfig. 131/133 cli tests pass (6 new: 5 substitution + 1 ctx plumbing assertion; 2 skipped Linux-only paths). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../adapters.claude.environment.test.mjs | 123 ++++++++++++++++++ cli/__tests__/run-loop.test.mjs | 27 ++++ cli/src/commands/agent.js | 5 + cli/src/lib/adapters/claude.js | 54 +++++++- 4 files changed, 202 insertions(+), 7 deletions(-) diff --git a/cli/__tests__/adapters.claude.environment.test.mjs b/cli/__tests__/adapters.claude.environment.test.mjs index 5e28d8a3..a4ed3fcf 100644 --- a/cli/__tests__/adapters.claude.environment.test.mjs +++ b/cli/__tests__/adapters.claude.environment.test.mjs @@ -137,4 +137,127 @@ describe('claude adapter — ctx.environment', () => { expect(fs.existsSync(path.join(cwd, '.commonly'))).toBe(false); expect(fs.existsSync(path.join(cwd, '.claude'))).toBe(false); }); + + // ── ${COMMONLY_*} placeholder substitution ──────────────────────────────── + // Lets users keep their checked-in env files free of secrets — the + // wrapper substitutes the runtime token + instance URL at spawn time + // from values it already has on hand (the saved token record). + + test('${COMMONLY_AGENT_TOKEN} in MCP env values is substituted with ctx.runtimeToken', async () => { + const { impl } = makeSpawnImpl(); + const environment = { + mcp: [ + { + name: 'commonly', + transport: 'stdio', + command: ['commonly-mcp'], + env: { + COMMONLY_API_URL: '${COMMONLY_API_URL}', + COMMONLY_AGENT_TOKEN: '${COMMONLY_AGENT_TOKEN}', + CUSTOM: 'literal-value-${COMMONLY_AGENT_TOKEN}-suffix', + }, + }, + ], + }; + await claude.spawn('hi', { + sessionId: null, + cwd, + environment, + runtimeToken: 'cm_agent_real_token_12345', + instanceUrl: 'https://api-dev.commonly.me', + _spawnImpl: impl, + }); + const cfg = JSON.parse(fs.readFileSync(path.join(cwd, '.commonly', 'mcp-config.json'), 'utf8')); + expect(cfg.mcpServers.commonly.env.COMMONLY_AGENT_TOKEN).toBe('cm_agent_real_token_12345'); + expect(cfg.mcpServers.commonly.env.COMMONLY_API_URL).toBe('https://api-dev.commonly.me'); + // Substitution is literal — interpolation works inside larger strings. + expect(cfg.mcpServers.commonly.env.CUSTOM).toBe( + 'literal-value-cm_agent_real_token_12345-suffix', + ); + }); + + test('${COMMONLY_INSTANCE_URL} alias substitutes to the same value as ${COMMONLY_API_URL}', async () => { + const { impl } = makeSpawnImpl(); + await claude.spawn('hi', { + sessionId: null, + cwd, + environment: { mcp: [{ name: 'x', transport: 'stdio', command: ['m'], env: { U: '${COMMONLY_INSTANCE_URL}' } }] }, + runtimeToken: 'cm_agent_t', + instanceUrl: 'http://localhost:5000', + _spawnImpl: impl, + }); + const cfg = JSON.parse(fs.readFileSync(path.join(cwd, '.commonly', 'mcp-config.json'), 'utf8')); + expect(cfg.mcpServers.x.env.U).toBe('http://localhost:5000'); + }); + + test('placeholders in command args + url are also substituted', async () => { + const { impl } = makeSpawnImpl(); + await claude.spawn('hi', { + sessionId: null, + cwd, + environment: { + mcp: [ + { + name: 'sse-server', + transport: 'sse', + url: '${COMMONLY_API_URL}/mcp/sse', + }, + { + name: 'arg-server', + transport: 'stdio', + command: ['some-bin', '--token', '${COMMONLY_AGENT_TOKEN}'], + }, + ], + }, + runtimeToken: 'cm_agent_x', + instanceUrl: 'https://api-dev.commonly.me', + _spawnImpl: impl, + }); + const cfg = JSON.parse(fs.readFileSync(path.join(cwd, '.commonly', 'mcp-config.json'), 'utf8')); + expect(cfg.mcpServers['sse-server'].url).toBe('https://api-dev.commonly.me/mcp/sse'); + expect(cfg.mcpServers['arg-server'].args).toEqual(['--token', 'cm_agent_x']); + }); + + test('unknown ${COMMONLY_*} placeholders are left intact (so misspellings surface as MCP errors, not silent empties)', async () => { + const { impl } = makeSpawnImpl(); + await claude.spawn('hi', { + sessionId: null, + cwd, + environment: { + mcp: [{ + name: 'x', + transport: 'stdio', + command: ['m'], + env: { TYPO: '${COMMONLY_AGNT_TOKEN}' /* typo, not a real key */ }, + }], + }, + runtimeToken: 'cm_agent_t', + instanceUrl: 'http://localhost:5000', + _spawnImpl: impl, + }); + const cfg = JSON.parse(fs.readFileSync(path.join(cwd, '.commonly', 'mcp-config.json'), 'utf8')); + expect(cfg.mcpServers.x.env.TYPO).toBe('${COMMONLY_AGNT_TOKEN}'); + }); + + test('substitution is a no-op when ctx.runtimeToken / instanceUrl are absent (literal env values pass through)', async () => { + const { impl } = makeSpawnImpl(); + await claude.spawn('hi', { + sessionId: null, + cwd, + environment: { + mcp: [{ + name: 'x', + transport: 'stdio', + command: ['m'], + env: { LITERAL: 'plain-string', PLACEHOLDER: '${COMMONLY_AGENT_TOKEN}' }, + }], + }, + _spawnImpl: impl, + // Note: no runtimeToken, no instanceUrl. + }); + const cfg = JSON.parse(fs.readFileSync(path.join(cwd, '.commonly', 'mcp-config.json'), 'utf8')); + expect(cfg.mcpServers.x.env.LITERAL).toBe('plain-string'); + // Empty token → placeholder left intact (not substituted with empty string). + expect(cfg.mcpServers.x.env.PLACEHOLDER).toBe('${COMMONLY_AGENT_TOKEN}'); + }); }); diff --git a/cli/__tests__/run-loop.test.mjs b/cli/__tests__/run-loop.test.mjs index 4094eeca..428feaaa 100644 --- a/cli/__tests__/run-loop.test.mjs +++ b/cli/__tests__/run-loop.test.mjs @@ -557,4 +557,31 @@ describe('performRun', () => { expect(ackCalls).toHaveLength(1); expect(ackCalls[0][0]).toContain('/events/e1/ack'); }); + + test('runtimeToken + instanceUrl flow into adapter.spawn ctx', async () => { + // The claude adapter uses these to substitute ${COMMONLY_AGENT_TOKEN} + // and ${COMMONLY_API_URL} placeholders in MCP env values, so users can + // keep their checked-in env files free of secrets. + const events = [makeEvent()]; + const mockGet = jest.fn().mockResolvedValue({ events }); + const mockPost = jest.fn().mockResolvedValue({}); + createClient.mockReturnValue({ get: mockGet, post: mockPost }); + + const spawn = jest.fn(async () => ({ text: 'ok' })); + const adapter = { name: 'stub', detect: stubAdapter.detect, spawn }; + + const { stop } = performRun({ + instanceUrl: 'https://api-dev.commonly.me', + token: 'cm_agent_specific_token', + adapter, + agentName: 'my-stub', + setTimeoutImpl: noopTimeout, + }); + await drainMicrotasks(); + stop(); + + const ctx = spawn.mock.calls[0][1]; + expect(ctx.runtimeToken).toBe('cm_agent_specific_token'); + expect(ctx.instanceUrl).toBe('https://api-dev.commonly.me'); + }); }); diff --git a/cli/src/commands/agent.js b/cli/src/commands/agent.js index dd0cfbba..ff780dcc 100644 --- a/cli/src/commands/agent.js +++ b/cli/src/commands/agent.js @@ -304,6 +304,11 @@ export const performRun = ({ env: process.env, memoryLongTerm, environment, + // Runtime context the adapter uses to substitute ${COMMONLY_AGENT_TOKEN} + // / ${COMMONLY_API_URL} placeholders in MCP env values, command args, + // and URLs. Lets users keep tokens out of their checked-in env files. + runtimeToken: token, + instanceUrl, metadata: { event }, }); diff --git a/cli/src/lib/adapters/claude.js b/cli/src/lib/adapters/claude.js index f7e3a233..f82d488f 100644 --- a/cli/src/lib/adapters/claude.js +++ b/cli/src/lib/adapters/claude.js @@ -79,19 +79,56 @@ const runClaude = ({ cmd, args, cwd, env, timeoutMs, spawnImpl = childSpawn }) = // ── MCP config write — claude consumes this via --mcp-config ───────── -const buildMcpConfig = (mcpServers) => { +// Substitute Commonly-supplied placeholders in MCP env values, command args, +// and URLs so users don't have to hand-paste secrets into their env file +// every time they re-attach. Surfaced during the 2026-04-17 cross-agent demo: +// every spec referencing commonly-mcp had to be rewritten with the agent's +// runtime token after attach, because the token is minted at attach time and +// only known to the wrapper. +// +// Recognised placeholders (substituted everywhere a string appears in the +// MCP config): +// ${COMMONLY_AGENT_TOKEN} — the per-(agent, pod) cm_agent_* runtime token +// ${COMMONLY_API_URL} — the instance URL the agent is attached to +// ${COMMONLY_INSTANCE_URL} — alias for COMMONLY_API_URL (clearer in context) +// +// Substitution is one-pass + literal — no nested expansion, no shell quoting. +// Unknown placeholders are left intact so the user sees a clear runtime error +// from the MCP server rather than a silent empty string. +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 + )); +}; + +const buildMcpConfig = (mcpServers, ctx = {}) => { // Shape: `{ mcpServers: { : { ... } } }` — the standard MCP client // config, which claude's `--mcp-config` reads directly. const mcpServersMap = {}; for (const server of mcpServers) { const entry = { type: server.transport || 'stdio' }; - if (server.url) entry.url = server.url; + if (server.url) entry.url = substitutePlaceholders(server.url, ctx); if (server.command) { const [command, ...args] = server.command; entry.command = command; - if (args.length) entry.args = args; + if (args.length) entry.args = args.map((a) => substitutePlaceholders(a, ctx)); + } + if (server.env) { + entry.env = {}; + for (const [k, v] of Object.entries(server.env)) { + entry.env[k] = substitutePlaceholders(v, ctx); + } } - if (server.env) entry.env = server.env; mcpServersMap[server.name] = entry; } return { mcpServers: mcpServersMap }; @@ -100,11 +137,11 @@ const buildMcpConfig = (mcpServers) => { // Regenerated on every spawn from the env spec; do not hand-edit — the file // is overwritten before each `claude` invocation, so any local changes are // silently clobbered. ADR-008 §invariant #5 (edits propagate on next spawn). -const writeMcpConfig = async (cwd, mcpServers) => { +const writeMcpConfig = async (cwd, mcpServers, ctx = {}) => { const dir = join(cwd, '.commonly'); await mkdir(dir, { recursive: true }); const file = join(dir, 'mcp-config.json'); - await writeFile(file, JSON.stringify(buildMcpConfig(mcpServers), null, 2), 'utf8'); + await writeFile(file, JSON.stringify(buildMcpConfig(mcpServers, ctx), null, 2), 'utf8'); return file; }; @@ -133,7 +170,10 @@ const prepareArgv = async (innerArgv, ctx) => { if (!env) return { cmd: 'claude', args: innerArgv }; if (Array.isArray(env.mcp) && env.mcp.length > 0 && ctx.cwd) { - const configPath = await writeMcpConfig(ctx.cwd, env.mcp); + const configPath = await writeMcpConfig(ctx.cwd, env.mcp, { + runtimeToken: ctx.runtimeToken, + instanceUrl: ctx.instanceUrl, + }); // Insert --mcp-config immediately after the subcommand-style `-p` block // so claude parses it before prompt collection begins. innerArgv = [...innerArgv, '--mcp-config', configPath];