diff --git a/packages/cli/src/cli.test.ts b/packages/cli/src/cli.test.ts index 8d5e3576..d46f66ce 100644 --- a/packages/cli/src/cli.test.ts +++ b/packages/cli/src/cli.test.ts @@ -4,9 +4,11 @@ import assert from 'node:assert/strict'; import { CLEAN_IGNORED_PATTERNS, SKILL_INSTALL_IGNORED_PATTERNS, + assertSafeRelativePath, decideCleanMode, parseAgentArgs, - resolveSystemPromptPlaceholders + resolveSystemPromptPlaceholders, + stripAgentFlag } from './cli.js'; // The conflict-detection path inside parseAgentArgs uses the module-local @@ -144,6 +146,63 @@ test('decideCleanMode: opencode + --install-in-repo → no mount', () => { assert.deepEqual(decideCleanMode('opencode', true, true), { useClean: false }); }); +test('stripAgentFlag: removes --agent pair preserving surrounding args', () => { + // Degrade path: when the CLI cannot materialize opencode.json (non-mount + // --install-in-repo), it strips the --agent selector so opencode launches + // with its default agent rather than failing to resolve the unknown one. + assert.deepEqual( + stripAgentFlag(['--agent', 'persona-maker']), + [] + ); + assert.deepEqual( + stripAgentFlag(['--foo', '--agent', 'persona-maker', '--bar']), + ['--foo', '--bar'] + ); + assert.deepEqual( + stripAgentFlag(['--keep-me']), + ['--keep-me'] + ); +}); + +test('stripAgentFlag: trailing --agent without a value is preserved (caller decides)', () => { + // Defensive: don't swallow an argv that looks malformed — let the harness + // reject it so the bug surfaces instead of getting silently stripped. + assert.deepEqual(stripAgentFlag(['--agent']), ['--agent']); +}); + +test('stripAgentFlag: removes every --agent pair, not just the first', () => { + // Current producer emits exactly one pair, so behavior is equivalent + // today, but "strip all" is idempotent and safer if a future caller ever + // appends a second --agent for any reason. + assert.deepEqual( + stripAgentFlag(['--agent', 'a', '--agent', 'b']), + [] + ); + assert.deepEqual( + stripAgentFlag(['--before', '--agent', 'a', '--mid', '--agent', 'b', '--after']), + ['--before', '--mid', '--after'] + ); +}); + +test('assertSafeRelativePath: accepts typical relative paths', () => { + // Sanity: representative safe paths the opencode + future harnesses emit. + assert.doesNotThrow(() => assertSafeRelativePath('opencode.json')); + assert.doesNotThrow(() => assertSafeRelativePath('.opencode/config.json')); + assert.doesNotThrow(() => assertSafeRelativePath('nested/dir/file.json')); +}); + +test('assertSafeRelativePath: rejects empty, absolute, and path-traversal inputs', () => { + // Guards materialization against a malformed or adversarial persona trying + // to escape the mount via `join(dir, path)` and overwrite files outside + // the sandbox. Failure must surface with a clear message BEFORE any + // writeFileSync runs. + assert.throws(() => assertSafeRelativePath(''), /non-empty/); + assert.throws(() => assertSafeRelativePath('/etc/passwd'), /absolute/); + assert.throws(() => assertSafeRelativePath('../escape.json'), /\.\./); + assert.throws(() => assertSafeRelativePath('ok/../escape.json'), /\.\./); + assert.throws(() => assertSafeRelativePath('a/../../b.json'), /\.\./); +}); + test('decideCleanMode: claude + clean → engaged', () => { assert.deepEqual(decideCleanMode('claude', true), { useClean: true }); }); diff --git a/packages/cli/src/cli.ts b/packages/cli/src/cli.ts index 7b600bb3..e3bd4986 100644 --- a/packages/cli/src/cli.ts +++ b/packages/cli/src/cli.ts @@ -1,9 +1,9 @@ #!/usr/bin/env node import { spawn, spawnSync } from 'node:child_process'; import { randomBytes } from 'node:crypto'; -import { rmSync } from 'node:fs'; +import { mkdirSync, rmSync, writeFileSync } from 'node:fs'; import { constants, homedir } from 'node:os'; -import { join, resolve as resolvePath } from 'node:path'; +import { dirname, isAbsolute, join, resolve as resolvePath } from 'node:path'; import { pathToFileURL } from 'node:url'; import { @@ -345,6 +345,55 @@ function sessionMountDir(sessionRoot: string): string { return join(sessionRoot, 'mount'); } +/** + * Remove every `--agent ` pair from a harness argv. Used on the non-mount + * opencode path where we cannot safely materialize the persona's + * opencode.json (it would land in the user's real repo), so we fall back to + * launching opencode without a persona-specific agent selection. + * + * Strips all occurrences rather than just the first — the current producer + * (harness-kit's opencode branch) emits exactly one pair, so both behaviors + * are equivalent today, but "remove all" is idempotent and safer if a future + * caller ever appends a second `--agent` for any reason. A trailing `--agent` + * with no following value is preserved so the malformed argv surfaces at the + * harness rather than getting silently swallowed here. + */ +export function stripAgentFlag(args: readonly string[]): string[] { + const out: string[] = []; + for (let i = 0; i < args.length; i += 1) { + if (args[i] === '--agent' && i + 1 < args.length) { + i += 1; + continue; + } + out.push(args[i]); + } + return out; +} + +/** + * Validate that a configFile's relative path is safe to resolve under a + * sandbox/session directory. Rejects absolute paths and any segment equal to + * `..` so a malformed or adversarial persona cannot escape the mount via + * `join()` and overwrite files elsewhere. Called at materialization time so + * the failure surfaces with a clear path before any disk write happens. + */ +export function assertSafeRelativePath(relPath: string): void { + if (!relPath) { + throw new Error('configFile path must be a non-empty relative path'); + } + if (isAbsolute(relPath)) { + throw new Error( + `configFile path must be relative; got absolute path ${JSON.stringify(relPath)}` + ); + } + const segments = relPath.split(/[\\/]+/); + if (segments.some((s) => s === '..')) { + throw new Error( + `configFile path must not contain ".." segments; got ${JSON.stringify(relPath)}` + ); + } +} + /** Patterns hidden from an interactive claude session when `--clean` is set. * Applied by `@relayfile/local-mount` with gitignore semantics, so bare names * match at any depth in the project tree (e.g. `.claude` hides both @@ -485,6 +534,7 @@ async function runInteractive( const spec = buildInteractiveSpec({ harness: runtime.harness, + personaId, model: runtime.model, systemPrompt: runtime.systemPrompt, mcpServers: resolvedMcp, @@ -492,7 +542,29 @@ async function runInteractive( ...(installRoot !== undefined ? { pluginDirs: [installRoot] } : {}) }); for (const w of spec.warnings) process.stderr.write(`warning: ${w}\n`); - const finalArgs = spec.initialPrompt ? [...spec.args, spec.initialPrompt] : [...spec.args]; + + // Config-file materialization strategy: + // - Mount path (opencode default, claude --clean): write each configFile + // into the mount dir via onBeforeLaunch, so it lives only in the + // sandbox and is torn down with the session. + // - Non-mount path: today the only configFile producer is opencode + // (opencode.json for the --agent wiring), and the non-mount opencode + // path only engages under --install-in-repo. Writing opencode.json + // into the user's real repo would pollute the working tree, so we + // degrade: drop --agent from the argv, warn, and launch opencode with + // its default agent. The persona's prompt will not be applied in that + // mode; users who want it should drop --install-in-repo (the mount + // default handles this cleanly). + const hasConfigFiles = spec.configFiles.length > 0; + const degradeConfigFiles = hasConfigFiles && !useClean; + let effectiveArgs: readonly string[] = spec.args; + if (degradeConfigFiles) { + process.stderr.write( + 'warning: --install-in-repo cannot safely materialize the persona agent config (would write opencode.json into your repo); launching without --agent. Drop --install-in-repo to apply the persona prompt.\n' + ); + effectiveArgs = stripAgentFlag(spec.args); + } + const finalArgs = spec.initialPrompt ? [...effectiveArgs, spec.initialPrompt] : [...effectiveArgs]; // Print a sanitized summary rather than raw argv: spec.args for the claude // harness contains the resolved --mcp-config JSON and the full system @@ -536,6 +608,16 @@ async function runInteractive( runtime.harness === 'claude' ? [...CLEAN_IGNORED_PATTERNS] : [...SKILL_INSTALL_IGNORED_PATTERNS]; + // Anything we materialize into the mount via onBeforeLaunch must be + // hidden from the mount-mirror in both directions: without this, any + // opencode.json already present in the real repo would be copied into + // the mount (masking the per-session agent config we write), and the + // fresh write from onBeforeLaunch would sync back out on exit and + // pollute the user's working tree. Added dynamically so this stays + // generic for any future configFile producer. + for (const file of spec.configFiles) { + ignoredPatterns.push(file.path); + } process.stderr.write(`• sandbox mount → ${mountDir}\n`); // Three-stage SIGINT handler layered on top of launchOnMount's own signal // forwarding. launchOnMount catches the first SIGINT to kill the child @@ -604,10 +686,21 @@ async function runInteractive( process.stderr.write(`✓ Synced ${count} change(s) back to the repo${qualifier}.\n`); } }, - ...(deferInstallToMount + ...(deferInstallToMount || hasConfigFiles ? { onBeforeLaunch: (dir: string) => { - runInstallOrThrow(install.command, installLabel, dir); + if (deferInstallToMount) { + runInstallOrThrow(install.command, installLabel, dir); + } + for (const file of spec.configFiles) { + assertSafeRelativePath(file.path); + const target = join(dir, file.path); + // mkdir -p for any subdirs in file.path — the + // InteractiveConfigFile contract allows nested relative + // paths, and writeFileSync would otherwise throw ENOENT. + mkdirSync(dirname(target), { recursive: true }); + writeFileSync(target, file.contents, 'utf8'); + } } } : {}) diff --git a/packages/harness-kit/src/harness.test.ts b/packages/harness-kit/src/harness.test.ts index 35a1d87c..d396b8da 100644 --- a/packages/harness-kit/src/harness.test.ts +++ b/packages/harness-kit/src/harness.test.ts @@ -6,6 +6,7 @@ import { buildInteractiveSpec } from './harness.js'; test('claude branch always emits --mcp-config + --strict-mcp-config', () => { const result = buildInteractiveSpec({ harness: 'claude', + personaId: 'test-persona', model: 'claude-opus-4-6', systemPrompt: 'you are a test' }); @@ -26,6 +27,7 @@ test('claude branch always emits --mcp-config + --strict-mcp-config', () => { test('claude branch serializes resolved mcpServers into the --mcp-config payload', () => { const result = buildInteractiveSpec({ harness: 'claude', + personaId: 'test-persona', model: 'claude-sonnet-4-6', systemPrompt: 'x', mcpServers: { @@ -52,6 +54,7 @@ test('claude branch serializes resolved mcpServers into the --mcp-config payload test('claude branch translates permissions to flags', () => { const result = buildInteractiveSpec({ harness: 'claude', + personaId: 'test-persona', model: 'claude-opus-4-6', systemPrompt: 'x', permissions: { @@ -74,6 +77,7 @@ test('claude branch translates permissions to flags', () => { test('claude branch omits permission flags when unset or empty', () => { const result = buildInteractiveSpec({ harness: 'claude', + personaId: 'test-persona', model: 'claude-opus-4-6', systemPrompt: 'x', permissions: { allow: [], deny: [] } @@ -86,6 +90,7 @@ test('claude branch omits permission flags when unset or empty', () => { test('codex carries system prompt as initial positional; strips provider prefix from model', () => { const result = buildInteractiveSpec({ harness: 'codex', + personaId: 'test-persona', model: 'openai-codex/gpt-5.3-codex', systemPrompt: 'system-directive' }); @@ -97,6 +102,7 @@ test('codex carries system prompt as initial positional; strips provider prefix test('codex warns when mcpServers / permissions are declared', () => { const result = buildInteractiveSpec({ harness: 'codex', + personaId: 'test-persona', model: 'openai-codex/gpt-5.3-codex', systemPrompt: 'x', mcpServers: { foo: { type: 'http', url: 'https://example.com' } }, @@ -107,22 +113,70 @@ test('codex warns when mcpServers / permissions are declared', () => { assert.match(result.warnings[1], /codex harness is not yet wired for runtime permission/); }); -test('opencode carries system prompt via --prompt flag (not as trailing positional cwd)', () => { +test('opencode defines a per-persona agent in opencode.json and selects it with --agent', () => { const result = buildInteractiveSpec({ harness: 'opencode', + personaId: 'test-persona', model: 'opencode/minimax-m2.5', - systemPrompt: 'x' + systemPrompt: 'you are a test' }); assert.equal(result.bin, 'opencode'); - assert.deepEqual(result.args, ['--model', 'minimax-m2.5', '--prompt', 'x']); - // initialPrompt must be null so the caller does not append systemPrompt as - // a trailing positional — opencode would interpret it as a project dir. + // No --prompt (that flag pre-fills the TUI input with the system prompt as + // a *user* message) and no bare -m (opencode's -m expects provider/model; + // earlier code stripped the provider and silently fell back to the default + // model). Model + system prompt now live in the emitted opencode.json, + // selected by persona id via --agent. + assert.deepEqual(result.args, ['--agent', 'test-persona']); + assert.ok(!result.args.includes('--prompt')); + assert.ok(!result.args.includes('--model')); + assert.ok(!result.args.includes('-m')); assert.equal(result.initialPrompt, null); }); +test('opencode configFiles carries a well-formed opencode.json with the agent definition', () => { + const result = buildInteractiveSpec({ + harness: 'opencode', + personaId: 'test-persona', + model: 'opencode/minimax-m2.5', + systemPrompt: 'you are a test' + }); + assert.equal(result.configFiles.length, 1); + const [file] = result.configFiles; + assert.equal(file.path, 'opencode.json'); + const parsed = JSON.parse(file.contents); + assert.deepEqual(parsed, { + agent: { + 'test-persona': { + model: 'opencode/minimax-m2.5', + prompt: 'you are a test', + mode: 'primary' + } + } + }); +}); + +test('claude and codex emit an empty configFiles array', () => { + const claude = buildInteractiveSpec({ + harness: 'claude', + personaId: 'test-persona', + model: 'claude-opus-4-6', + systemPrompt: 'x' + }); + assert.deepEqual(claude.configFiles, []); + + const codex = buildInteractiveSpec({ + harness: 'codex', + personaId: 'test-persona', + model: 'openai-codex/gpt-5.3-codex', + systemPrompt: 'x' + }); + assert.deepEqual(codex.configFiles, []); +}); + test('claude branch appends --plugin-dir per entry in pluginDirs', () => { const result = buildInteractiveSpec({ harness: 'claude', + personaId: 'test-persona', model: 'claude-opus-4-6', systemPrompt: 'x', pluginDirs: ['/tmp/session-a/claude/plugin', '/tmp/session-b/claude/plugin'] @@ -141,12 +195,14 @@ test('claude branch appends --plugin-dir per entry in pluginDirs', () => { test('claude branch omits --plugin-dir when pluginDirs is empty or absent', () => { const withEmpty = buildInteractiveSpec({ harness: 'claude', + personaId: 'test-persona', model: 'claude-opus-4-6', systemPrompt: 'x', pluginDirs: [] }); const without = buildInteractiveSpec({ harness: 'claude', + personaId: 'test-persona', model: 'claude-opus-4-6', systemPrompt: 'x' }); @@ -157,6 +213,7 @@ test('claude branch omits --plugin-dir when pluginDirs is empty or absent', () = test('non-claude harnesses warn and ignore pluginDirs', () => { const codex = buildInteractiveSpec({ harness: 'codex', + personaId: 'test-persona', model: 'x', systemPrompt: 'x', pluginDirs: ['/tmp/session/plugin'] @@ -166,6 +223,7 @@ test('non-claude harnesses warn and ignore pluginDirs', () => { const opencode = buildInteractiveSpec({ harness: 'opencode', + personaId: 'test-persona', model: 'x', systemPrompt: 'x', pluginDirs: ['/tmp/session/plugin'] @@ -179,6 +237,7 @@ test('warnings are returned, not printed — library consumers route I/O themsel // test would leak output into the test runner. We just assert the shape. const result = buildInteractiveSpec({ harness: 'codex', + personaId: 'test-persona', model: 'x', systemPrompt: 'x', mcpServers: { a: { type: 'http', url: 'https://x' } } diff --git a/packages/harness-kit/src/harness.ts b/packages/harness-kit/src/harness.ts index 4353fc10..1dda75c8 100644 --- a/packages/harness-kit/src/harness.ts +++ b/packages/harness-kit/src/harness.ts @@ -4,6 +4,18 @@ import type { PersonaPermissions } from '@agentworkforce/workload-router'; +/** + * A config file the caller should materialize before launching the harness. + * Paths are relative to the harness's cwd — typically the mount dir when a + * sandbox is in use, otherwise process.cwd(). Pure data, no I/O here. + */ +export interface InteractiveConfigFile { + /** Relative path (from cwd) where the file should be written. */ + path: string; + /** Exact file contents, already serialized. */ + contents: string; +} + /** Result of translating a persona's runtime into a spawnable command. */ export interface InteractiveSpec { /** Binary to exec (e.g. `claude`, `codex`, `opencode`). */ @@ -14,8 +26,8 @@ export interface InteractiveSpec { * If set, the caller should append this as the final positional argument * — used by harnesses that don't support a separate system-prompt flag * to carry the persona's system prompt as the initial user prompt. - * Currently only codex takes this path; opencode uses its own `--prompt` - * flag (wired directly into `args`) and claude uses `--append-system-prompt`, + * Currently only codex takes this path; claude uses `--append-system-prompt` + * and opencode writes the prompt into `opencode.json` (see `configFiles`), * so both return `null` here. */ initialPrompt: string | null; @@ -24,10 +36,23 @@ export interface InteractiveSpec { * support MCP yet, ignoring". Callers decide whether to print them. */ warnings: string[]; + /** + * Config files the caller must write (relative to the harness cwd) before + * launch. Opencode uses this to materialize an `opencode.json` carrying + * the persona's agent definition (model + system prompt) so `--agent` can + * resolve it; claude and codex return an empty array. + */ + configFiles: InteractiveConfigFile[]; } export interface BuildInteractiveSpecInput { harness: Harness; + /** + * Persona id — used as the opencode agent name. Claude and codex ignore + * this field today; keeping it required here keeps call sites honest and + * lets future harnesses consume it without another type change. + */ + personaId: string; model: string; systemPrompt: string; /** Env-resolved MCP servers (pass the output of `resolveMcpServersLenient().servers`). */ @@ -66,18 +91,23 @@ function hasAnyPermission(p: PersonaPermissions | undefined): boolean { * codex has no dedicated system-prompt flag today — callers append it as * the final positional `[PROMPT]`. * - * The opencode branch uses opencode's own `--prompt` flag (wired directly - * into `args`) and returns `initialPrompt: null`. The earlier behavior of - * appending the prompt as a trailing positional was unsafe: `opencode`'s - * bare form treats a trailing positional as a project directory, which - * caused the TUI to fail with "Failed to change directory to …". + * The opencode branch routes model + system prompt through opencode's + * agent abstraction (see https://opencode.ai/config.json: `agent..{ + * model, prompt, mode }`). It emits an `opencode.json` via `configFiles` + * for the caller to materialize at cwd, and selects it with `--agent + * `. It deliberately does NOT use `--prompt` (that flag + * pre-fills the TUI input with a user message) or bare `-m` (opencode + * expects full `provider/model` form and silently falls back to its + * default when given a stripped model name). `initialPrompt` is `null` + * so callers do not append a trailing positional, which opencode would + * otherwise interpret as a project directory. * * Both codex and opencode emit a warning if the persona declares * `mcpServers` or `permissions` — those features aren't wired for those * harnesses yet. */ export function buildInteractiveSpec(input: BuildInteractiveSpecInput): InteractiveSpec { - const { harness, model, systemPrompt, mcpServers, permissions, pluginDirs } = input; + const { harness, personaId, model, systemPrompt, mcpServers, permissions, pluginDirs } = input; const warnings: string[] = []; const hasPluginDirs = pluginDirs !== undefined && pluginDirs.length > 0; @@ -107,7 +137,7 @@ export function buildInteractiveSpec(input: BuildInteractiveSpecInput): Interact if (permissions?.mode) { args.push('--permission-mode', permissions.mode); } - return { bin: 'claude', args, initialPrompt: null, warnings }; + return { bin: 'claude', args, initialPrompt: null, warnings, configFiles: [] }; } case 'codex': { if (mcpServers && Object.keys(mcpServers).length > 0) { @@ -129,7 +159,8 @@ export function buildInteractiveSpec(input: BuildInteractiveSpecInput): Interact bin: 'codex', args: ['-m', stripProviderPrefix(model)], initialPrompt: systemPrompt, - warnings + warnings, + configFiles: [] }; } case 'opencode': { @@ -148,16 +179,50 @@ export function buildInteractiveSpec(input: BuildInteractiveSpecInput): Interact 'pluginDirs is currently claude-only; ignoring under the opencode harness. Skills must be staged via opencode conventions.' ); } - // opencode's bare form is `opencode [project]` where the trailing - // positional is a project directory, NOT a prompt. Carry the persona's - // system prompt via `--prompt` (top-level TUI flag) so it isn't parsed - // as a cwd. + // opencode resolves a persona's system prompt + model through its own + // "agent" abstraction (see https://opencode.ai/config.json: `agent..{ + // model, prompt, mode }`). Earlier revisions tried to use `--prompt` for + // the system prompt and `-m` for the model directly, but that was wrong + // on two counts: + // (a) `--prompt` pre-fills the TUI input buffer with a *user* message, + // not the agent's instructions — the persona's systemPrompt ended + // up sitting unsent in the chat field. + // (b) `-m` expects `provider/model` (opencode's own docs); stripping + // the `opencode/` prefix left just `gpt-5-nano`, which opencode + // could not resolve and silently fell back to its default model. + // The correct shape is an `opencode.json` under cwd defining an agent + // with the persona's prompt + full-provider-form model, selected via + // `--agent ` at launch. We emit that file via configFiles + // so the CLI can drop it into the mount dir before exec. + const agentConfig = { + agent: { + [personaId]: { + model, + prompt: systemPrompt, + mode: 'primary' + } + } + }; return { bin: 'opencode', - args: ['--model', stripProviderPrefix(model), '--prompt', systemPrompt], + args: ['--agent', personaId], initialPrompt: null, - warnings + warnings, + configFiles: [ + { + path: 'opencode.json', + contents: JSON.stringify(agentConfig, null, 2) + '\n' + } + ] }; } + default: { + // Exhaustiveness guard: if `Harness` gains a new variant, this + // assertion will fail to compile and force the maintainer to handle + // the new case above rather than silently dropping into an untyped + // fallthrough at runtime. + const _exhaustive: never = harness; + throw new Error(`Unhandled harness: ${String(_exhaustive)}`); + } } } diff --git a/packages/harness-kit/src/index.ts b/packages/harness-kit/src/index.ts index 46e7033c..eb870343 100644 --- a/packages/harness-kit/src/index.ts +++ b/packages/harness-kit/src/index.ts @@ -19,6 +19,7 @@ export { export { buildInteractiveSpec, type BuildInteractiveSpecInput, + type InteractiveConfigFile, type InteractiveSpec } from './harness.js';