Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 2 additions & 16 deletions apps/daemon/src/prompts/system.ts
Original file line number Diff line number Diff line change
Expand Up @@ -520,16 +520,6 @@ export interface ComposeInput {
// Skill identifier. Required when critique is enabled;
// ignored when critique is disabled or omitted.
critiqueSkill?: { id: string } | undefined;
// External MCP servers the daemon already holds a valid OAuth Bearer
// token for at spawn time. We surface the list to the model so it does
// NOT chase Claude Code's synthetic `*_authenticate` /
// `*_complete_authentication` tools that get injected when the HTTP
// transport's first connect transiently flips a server into
// needs-auth state — the Bearer is in `.mcp.json`, the real tools are
// available, and burning a turn on a redundant OAuth dance just
// confuses the user.
connectedExternalMcp?: ReadonlyArray<{ id: string; label?: string | undefined }>
| undefined;
// Optional `## Active plugin` / `## Plugin inputs` block. The daemon's
// plugin module renders this from an AppliedPluginSnapshot; we splice
// it in after the active skill so the plugin description sits next to
Expand Down Expand Up @@ -595,7 +585,6 @@ export function composeSystemPrompt({
critique,
critiqueBrand,
critiqueSkill,
connectedExternalMcp,
pluginBlock,
activeStageBlocks,
streamFormat,
Expand Down Expand Up @@ -940,9 +929,6 @@ export function composeSystemPrompt({
parts.push(ACTIVE_DESIGN_SYSTEM_VISUAL_DIRECTION_OVERRIDE);
}

const mcpDirective = renderConnectedExternalMcpDirective(connectedExternalMcp);
if (mcpDirective) parts.push(mcpDirective);

if (resolvedExecutionProfile === 'filesystem') {
parts.push(FILESYSTEM_HANDOFF_OVERRIDE);
}
Expand Down Expand Up @@ -1072,7 +1058,7 @@ If this is a plain API run where filesystem tools are unavailable, output the sa
// `*_authenticate` / `*_complete_authentication` tool for them. If
// the real tools really are missing, surface that as a separate
// failure instead of pivoting to the synthetic flow.
function renderConnectedExternalMcpDirective(
export function renderConnectedExternalMcpDirective(
connectedExternalMcp:
| ReadonlyArray<{ id: string; label?: string | undefined }>
| undefined,
Expand All @@ -1087,8 +1073,8 @@ function renderConnectedExternalMcpDirective(
})
.filter((line): line is string => typeof line === 'string');
if (lines.length === 0) return '';
// No leading separator: callers place this in a `---`-joined slice.
return [
'\n\n---\n\n',
'## External MCP servers — already authenticated\n\n',
'The following external MCP servers are already authenticated for this run via an OAuth Bearer token the daemon injected into `.mcp.json`. You can call their real tools directly:\n\n',
lines.join('\n'),
Expand Down
17 changes: 10 additions & 7 deletions apps/daemon/src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import net from 'node:net';
import { executionProfileFromStreamFormat, PLUGIN_SHARE_ACTION_PLUGIN_IDS } from '@open-design/contracts';
import {
composeSystemPrompt,
renderConnectedExternalMcpDirective,
resolveExclusiveSurface,
} from './prompts/system.js';
import { emittedRenderableQuestionForm } from './question-form-detect.js';
Expand Down Expand Up @@ -3372,7 +3373,6 @@ export async function startServer({
streamFormat,
locale,
sessionMode,
connectedExternalMcp,
appliedPluginSnapshotId,
mediaExecution,
byokMediaDefaults,
Expand Down Expand Up @@ -3888,9 +3888,6 @@ export async function startServer({
byokMediaDefaults,
streamFormat,
executionProfile: executionProfileFromStreamFormat(streamFormat),
connectedExternalMcp: Array.isArray(connectedExternalMcp)
? connectedExternalMcp
: undefined,
...(pluginBlock ? { pluginBlock } : {}),
...(activeStageBlocks ? { activeStageBlocks } : {}),
userInstructions,
Expand Down Expand Up @@ -4384,7 +4381,6 @@ export async function startServer({
streamFormat: def?.streamFormat ?? 'plain',
locale,
sessionMode: runSessionMode,
connectedExternalMcp,
mediaExecution: run?.mediaExecution,
byokMediaDefaults,
// Plan §3.M2 / §3.V1 — forward the run's snapshot id so the
Expand Down Expand Up @@ -4686,9 +4682,16 @@ export async function startServer({
'Do not mention this title task to the user. Continue with the normal answer after the title marker.',
].join('\n')
: '';
// The connected-external-MCP directive reflects live OAuth token state,
// which flips mid-conversation as Bearers expire/refresh. Keeping it out of
// the cached stable prefix (daemonSystemPrompt) and re-sending it here in
// the per-turn slice keeps the upstream prompt-cache prefix byte-stable
// across resumes (protecting the conversation-history cache) while still
// giving the model the current MCP auth state on every turn.
const mcpConnectedDirective = renderConnectedExternalMcpDirective(connectedExternalMcp);
const clientInstructionParts = includeStableInstructions
? [researchCommandContract, runContextPrompt, browserUsePromptGuard, titleGenerationPrompt, systemPrompt]
: [researchCommandContract, runContextPrompt, browserUsePromptGuard, titleGenerationPrompt];
? [researchCommandContract, runContextPrompt, mcpConnectedDirective, browserUsePromptGuard, titleGenerationPrompt, systemPrompt]
: [researchCommandContract, runContextPrompt, mcpConnectedDirective, browserUsePromptGuard, titleGenerationPrompt];
const clientInstructionPrompt = clientInstructionParts
.map((part) => (typeof part === 'string' ? part.trim() : ''))
.filter(Boolean)
Expand Down
76 changes: 76 additions & 0 deletions apps/daemon/tests/codex-session-resume.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import path from 'node:path';
import { afterEach, describe, expect, it } from 'vitest';

import { startServer } from '../src/server.js';
import { writeMcpConfig } from '../src/mcp-config.js';
import { setToken } from '../src/mcp-tokens.js';

// End-to-end coverage for codex native (capture-style) session resume.
//
Expand Down Expand Up @@ -224,6 +226,80 @@ describe('codex native session resume', () => {
expect(afterIntervening.argv).not.toContain('resume');
expect(afterIntervening.argv[0]).toBe('exec');
});

// Guards the fix that moved the connected-external-MCP directive out of the
// cached `daemonSystemPrompt` and into the per-turn instruction slice. The
// directive reflects live OAuth Bearer validity, so keeping it in the cached
// prefix churned the whole prompt-cache prefix (history included) whenever a
// token expired mid-conversation. Now it must ride in the per-turn slice, i.e.
// be re-sent on EVERY turn — including a clean resume, which never re-sends the
// cached stable block. On origin/main the directive lived in the stable block,
// so a clean resume dropped it and the turn-2 assertion below goes red.
it('re-sends the connected-MCP directive in the per-turn slice on resume turns', async () => {
binDir = await mkdtemp(path.join(os.tmpdir(), 'od-codex-mcp-bin-'));
const { bin, logPath } = await writeCapturingCodex(binDir, 'codex-mcp');

clearTelemetryEnv();
started = (await startServer({ port: 0, returnServer: true })) as StartedServer;
await putConfig(started.url, {
agentId: 'codex',
agentCliEnv: { codex: { CODEX_BIN: bin } },
telemetry: { metrics: true, content: false, artifactManifest: false },
privacyDecisionAt: Date.now(),
});

// A connected external MCP server = enabled config + a live (non-expired)
// OAuth Bearer. That is exactly what makes the daemon render the
// "already authenticated" directive.
const dataDir = process.env.OD_DATA_DIR;
if (!dataDir) throw new Error('OD_DATA_DIR is required for the MCP directive test');
await writeMcpConfig(dataDir, {
servers: [
{
id: 'github',
label: 'GitHub',
transport: 'http',
url: 'https://mcp.test.invalid/github',
enabled: true,
},
],
});
await setToken(dataDir, 'github', {
Comment thread
lefarcen marked this conversation as resolved.
accessToken: 'live-access-token',
tokenType: 'Bearer',
expiresAt: Date.now() + 3_600_000,
savedAt: Date.now(),
});

const conversationId = await createConversation(started.url);
const turn1 = await sendRunAndWait(started.url, conversationId, 'first user request');
expect(turn1.status).toBe('succeeded');
const turn2 = await sendRunAndWait(
started.url,
conversationId,
'second user request please',
);
expect(turn2.status).toBe('succeeded');

const runs = await readChatTurnExecs(logPath, conversationId);
expect(runs).toHaveLength(2);
const [create, resume] = runs as [ExecInvocation, ExecInvocation];

const MCP_MARKER = 'External MCP servers — already authenticated';
// A marker that only appears inside the cached stable block (daemonSystemPrompt).
const STABLE_BLOCK_MARKER = '# Identity and workflow charter (background)';

// Turn 1 (fresh seed) carries the directive.
expect(create.stdin).toContain(MCP_MARKER);
expect(create.stdin).toContain('`github`');

// Turn 2 is a clean resume: the cached stable block is NOT re-sent...
expect(resume.argv.slice(0, 2)).toEqual(['exec', 'resume']);
expect(resume.stdin).not.toContain(STABLE_BLOCK_MARKER);
// ...but the MCP directive IS, because it now lives in the per-turn slice.
expect(resume.stdin).toContain(MCP_MARKER);
expect(resume.stdin).toContain('`github`');
});
});

// Minimal fake Claude CLI (claude-stream-json): emits an init frame + one
Expand Down
89 changes: 48 additions & 41 deletions apps/daemon/tests/prompts/system.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@ import { fileURLToPath } from 'node:url';

import { describe, expect, it } from 'vitest';

import { composeSystemPrompt, resolveExclusiveSurface } from '../../src/prompts/system.js';
import {
composeSystemPrompt,
renderConnectedExternalMcpDirective,
resolveExclusiveSurface,
} from '../../src/prompts/system.js';

const __filename = fileURLToPath(import.meta.url);
const __dirname = path.dirname(__filename);
Expand Down Expand Up @@ -415,66 +419,69 @@ describe('composeSystemPrompt', () => {
});
});

describe('connectedExternalMcp directive', () => {
it('omits the directive when no servers are passed', () => {
// The connected-external-MCP directive reflects live OAuth token state, which
// flips mid-conversation as Bearers expire/refresh. It now rides in the
// per-turn instruction slice (server.ts), NOT the cached system prompt, so it
// no longer churns the cacheable prefix across resumes. composeSystemPrompt
// must therefore never emit it; the exported renderer is tested directly.
describe('connectedExternalMcp directive is no longer in the system prompt', () => {
it('never emits the MCP directive from composeSystemPrompt', () => {
const prompt = composeSystemPrompt({});
expect(prompt).not.toContain('External MCP servers — already authenticated');
expect(prompt).not.toContain('mcp__<server>__authenticate');
});

it('omits the directive when an empty array is passed', () => {
const prompt = composeSystemPrompt({ connectedExternalMcp: [] });
it('keeps the media-execution-disabled block, still with no MCP directive', () => {
const prompt = composeSystemPrompt({
metadata: { kind: 'image' },
mediaExecution: { mode: 'disabled' },
});
expect(prompt).toContain('Open Design-owned media execution is **disabled for this run**');
expect(prompt).not.toContain('## Media generation contract');
expect(prompt).not.toContain('External MCP servers — already authenticated');
});
});

it('lists each connected server and forbids the synthetic auth tools', () => {
const prompt = composeSystemPrompt({
connectedExternalMcp: [
{ id: 'higgsfield-openclaw', label: 'Higgsfield (OpenClaw)' },
{ id: 'github' },
],
});
describe('renderConnectedExternalMcpDirective', () => {
it('returns an empty string for no / empty servers', () => {
expect(renderConnectedExternalMcpDirective(undefined)).toBe('');
expect(renderConnectedExternalMcpDirective([])).toBe('');
});

expect(prompt).toContain('## External MCP servers — already authenticated');
expect(prompt).toContain('`higgsfield-openclaw`');
expect(prompt).toContain('Higgsfield (OpenClaw)');
expect(prompt).toContain('`github`');
expect(prompt).toContain(
it('lists each connected server and forbids the synthetic auth tools', () => {
const directive = renderConnectedExternalMcpDirective([
{ id: 'higgsfield-openclaw', label: 'Higgsfield (OpenClaw)' },
{ id: 'github' },
]);
expect(directive).toContain('## External MCP servers — already authenticated');
expect(directive).toContain('`higgsfield-openclaw`');
expect(directive).toContain('Higgsfield (OpenClaw)');
expect(directive).toContain('`github`');
expect(directive).toContain(
'**Do NOT call any tool whose name matches `mcp__<server>__authenticate` or `mcp__<server>__complete_authentication`',
);
expect(prompt).toContain('localhost:<random>/callback');
expect(prompt).toContain('Settings → External MCP');
expect(directive).toContain('localhost:<random>/callback');
expect(directive).toContain('Settings → External MCP');
});

it('skips entries with blank ids and emits no directive when nothing usable remains', () => {
const prompt = composeSystemPrompt({
connectedExternalMcp: [
it('skips entries with blank ids and emits nothing when none remain', () => {
expect(
renderConnectedExternalMcpDirective([
{ id: ' ', label: 'blank' },
{ id: '', label: 'empty' },
] as any,
});
expect(prompt).not.toContain('External MCP servers — already authenticated');
] as any),
).toBe('');
});

it('does not duplicate the label when it equals the id', () => {
const prompt = composeSystemPrompt({
connectedExternalMcp: [{ id: 'github', label: 'github' }],
});
expect(prompt).toContain('- `github`\n');
expect(prompt).not.toContain('- `github` (github)');
const directive = renderConnectedExternalMcpDirective([{ id: 'github', label: 'github' }]);
expect(directive).toContain('- `github`\n');
expect(directive).not.toContain('- `github` (github)');
});

it('keeps external MCP tools visible when OD-owned media execution is disabled', () => {
const prompt = composeSystemPrompt({
connectedExternalMcp: [{ id: 'external-media', label: 'External media' }],
metadata: { kind: 'image' },
mediaExecution: { mode: 'disabled' },
});

expect(prompt).toContain('## External MCP servers — already authenticated');
expect(prompt).toContain('`external-media`');
expect(prompt).toContain('Open Design-owned media execution is **disabled for this run**');
expect(prompt).not.toContain('## Media generation contract');
it('has no leading separator so it composes cleanly in a `---`-joined slice', () => {
const directive = renderConnectedExternalMcpDirective([{ id: 'github' }]);
expect(directive.startsWith('## External MCP servers')).toBe(true);
});
});

Expand Down
Loading