diff --git a/container/agent-runner/src/providers/claude.harness.test.ts b/container/agent-runner/src/providers/claude.harness.test.ts index 34b9d9d40..61ab4ca40 100644 --- a/container/agent-runner/src/providers/claude.harness.test.ts +++ b/container/agent-runner/src/providers/claude.harness.test.ts @@ -1,13 +1,12 @@ /** * Harness-capability mapping in the Claude provider: the disallow list is the - * fixed set plus capability-driven entries (fail closed), the PreToolUse hook - * blocks exactly that list, and unknown resolved keys warn once at - * construction. Pure — no SDK, no DB (the hook's container_state write is - * try/caught by design). + * fixed set plus capability-driven entries (fail closed), and the PreToolUse + * hook blocks exactly that list. Pure — no SDK, no DB (the hook's + * container_state write is try/caught by design). */ -import { afterEach, describe, expect, it, spyOn } from 'bun:test'; +import { describe, expect, it } from 'bun:test'; -import { ClaudeProvider, SDK_DISALLOWED_TOOLS, buildDisallowedTools, createPreToolUseHook } from './claude.js'; +import { SDK_DISALLOWED_TOOLS, buildDisallowedTools, createPreToolUseHook } from './claude.js'; type LooseHook = (input: unknown) => Promise>; @@ -18,6 +17,7 @@ describe('buildDisallowedTools', () => { for (const fixed of SDK_DISALLOWED_TOOLS) expect(list).toContain(fixed); expect(list).toContain('Workflow'); expect(list).toContain('DesignSync'); + expect(list).toContain('ReportFindings'); } }); @@ -47,19 +47,3 @@ describe('createPreToolUseHook', () => { expect(res.continue).toBe(true); }); }); - -describe('ClaudeProvider constructor capability handling', () => { - const errSpy = spyOn(console, 'error'); - - afterEach(() => { - errSpy.mockClear(); - }); - - it('warns once per unknown key, never for the known pair', () => { - new ClaudeProvider({ harnessCapabilities: { 'agent-teams': 'off', workflow: 'on', monitor: 'off' } }); - const calls = errSpy.mock.calls.map((c) => String(c[0])); - expect(calls.filter((m) => m.includes('Unknown harness capability'))).toHaveLength(1); - expect(calls.some((m) => m.includes('monitor'))).toBe(true); - expect(calls.some((m) => m.includes("'agent-teams'") || m.includes("'workflow'"))).toBe(false); - }); -}); diff --git a/container/agent-runner/src/providers/claude.ts b/container/agent-runner/src/providers/claude.ts index d9592cb2f..3595efacf 100644 --- a/container/agent-runner/src/providers/claude.ts +++ b/container/agent-runner/src/providers/claude.ts @@ -26,6 +26,8 @@ function log(msg: string): void { // Code UI affordances; in a headless container they'd appear stuck. // - DesignSync: desktop design-tool integration — nothing to sync with in a // headless container, and a ~9.3KB/turn schema (wire-measured). +// - ReportFindings: code-review-reporting UI affordance with no headless host +// surface to receive it (~1.9KB/turn schema). export const SDK_DISALLOWED_TOOLS = [ 'CronCreate', 'CronDelete', @@ -37,20 +39,33 @@ export const SDK_DISALLOWED_TOOLS = [ 'EnterWorktree', 'ExitWorktree', 'DesignSync', + 'ReportFindings', ]; /** - * Effective disallow list for a provider instance: the fixed set plus - * capability-driven entries from the group's resolved harness-capability map - * (container.json, resolved host-side — see src/harness-capabilities.ts on - * the host). `workflow` off — the default — adds `Workflow` as - * defense-in-depth: the primary OFF mechanism is the host-reconciled - * `disableWorkflows` settings key, and this backstop covers a malformed or - * hand-reverted settings.json. Absent or unknown state fails closed. + * Configurable capabilities that block a harness tool when OFF, mapped to the + * tool name. This is the runner half of the capability contract (the host half + * is the settings reconciler); it is genuinely runner-specific because + * `disallowedTools` is a Claude concept the host tree can't name. The block is + * defense-in-depth — the primary OFF mechanism is the host-reconciled settings + * key — covering a malformed or hand-reverted settings.json. + */ +const CAPABILITY_DISALLOWS: Record = { + workflow: 'Workflow', +}; + +/** + * Effective disallow list for a provider instance: the fixed set plus, for each + * capability in CAPABILITY_DISALLOWS whose resolved state is not explicitly + * `on`, its tool. Absent or unexpected state fails closed (tool stays blocked). + * `caps` is the resolved map from container.json (host-resolved: only known + * keys, valid values). */ export function buildDisallowedTools(caps?: Record): string[] { const list = [...SDK_DISALLOWED_TOOLS]; - if (caps?.workflow !== 'on') list.push('Workflow'); + for (const [key, tool] of Object.entries(CAPABILITY_DISALLOWS)) { + if (caps?.[key] !== 'on') list.push(tool); + } return list; } @@ -182,11 +197,12 @@ function formatTranscriptMarkdown(messages: ParsedMessage[], title?: string | nu * (like createPreCompactHook) because the blocklist is per-instance — it * depends on the group's resolved harness capabilities. */ -export function createPreToolUseHook(blockedTools: string[]): HookCallback { +export function createPreToolUseHook(blockedTools: Iterable): HookCallback { + const blocked = new Set(blockedTools); return async (input) => { const i = input as { tool_name?: string; tool_input?: Record }; const toolName = i.tool_name ?? ''; - if (blockedTools.includes(toolName)) { + if (blocked.has(toolName)) { return { decision: 'block', stopReason: `Tool '${toolName}' is not available in this environment — use the nanoclaw equivalent.`, @@ -366,6 +382,7 @@ export class ClaudeProvider implements AgentProvider { private model?: string; private effort?: string; private disallowedTools: string[]; + private preToolUseHook: HookCallback; constructor(options: ProviderOptions = {}) { this.assistantName = options.assistantName; @@ -377,13 +394,12 @@ export class ClaudeProvider implements AgentProvider { ...(options.env ?? {}), CLAUDE_CODE_AUTO_COMPACT_WINDOW, }; + // The resolved harness-capability map in container.json is host-resolved: + // only known keys with valid values reach here, so the runner never has to + // reason about unknown keys. Blocklist + hook are per-instance but immutable + // after construction — build both once. this.disallowedTools = buildDisallowedTools(options.harnessCapabilities); - // 'agent-teams' is host-settings-managed — no runner mechanism, but it is - // a known key; warning on it would fire for every group on every boot. - const knownKeys = new Set(['agent-teams', 'workflow']); - for (const key of Object.keys(options.harnessCapabilities ?? {})) { - if (!knownKeys.has(key)) log(`Unknown harness capability key (no claude-provider mechanism): ${key}`); - } + this.preToolUseHook = createPreToolUseHook(this.disallowedTools); } isSessionInvalid(err: unknown): boolean { @@ -454,7 +470,7 @@ export class ClaudeProvider implements AgentProvider { settingSources: ['project', 'user', 'local'], mcpServers: this.mcpServers, hooks: { - PreToolUse: [{ hooks: [createPreToolUseHook(this.disallowedTools)] }], + PreToolUse: [{ hooks: [this.preToolUseHook] }], PostToolUse: [{ hooks: [postToolUseHook] }], PostToolUseFailure: [{ hooks: [postToolUseHook] }], PreCompact: [{ hooks: [createPreCompactHook(this.assistantName)] }], diff --git a/docs/harness-capabilities.md b/docs/harness-capabilities.md index 55c1b9bf0..c7b0700d2 100644 --- a/docs/harness-capabilities.md +++ b/docs/harness-capabilities.md @@ -23,7 +23,12 @@ ncl groups config update --id --harness-capabilities 'workflow=on,age ncl groups restart --id # apply ``` -`default` clears the per-group override (it is never stored). Changes from inside a container are rejected — like `cli_scope`, this is operator-only. +`default` clears the per-group override (it is never stored). Config changes through `ncl` from inside a container are rejected — like `cli_scope`, the sanctioned/persistent path is operator-only. + +### Enforcement strength (be precise about the boundary) + +- **`workflow` off** has two independent locks: the reconciled `disableWorkflows` settings key *and* a runner-side `disallowedTools` block. The tool cannot come back inside a running container even if the settings file is edited. +- **`agent-teams` off** has only one mechanism: the absence of the env key from the group's `settings.json`. That file is mounted **read-write** into the container (the CLI needs to write transcripts there), and `settingSources` also loads project/local settings from the agent-writable workspace. So an agent that actively rewrites its own settings can re-enable teams **for the current container lifetime**, until the next spawn re-reconciles it. Treat `agent-teams=off` as **configuration hygiene enforced at spawn**, not a hard adversarial boundary inside a live container — the real trust boundary remains the container sandbox + OneCLI. A follow-up ([nanocoai/nanoclaw#TBD](https://github.com/nanocoai/nanoclaw/issues)) will mount the managed settings source read-only to close this. ## [BREAKING] Agent teams default off diff --git a/src/claude-md-compose.ts b/src/claude-md-compose.ts index fdbdc0159..88534f91b 100644 --- a/src/claude-md-compose.ts +++ b/src/claude-md-compose.ts @@ -222,7 +222,7 @@ function syncSymlink(linkPath: string, target: string): void { fs.symlinkSync(target, linkPath); } -function writeAtomic(filePath: string, content: string): void { +export function writeAtomic(filePath: string, content: string): void { const tmp = `${filePath}.tmp-${process.pid}`; fs.writeFileSync(tmp, content); fs.renameSync(tmp, filePath); diff --git a/src/cli/dispatch.ts b/src/cli/dispatch.ts index 997006959..16ebca74b 100644 --- a/src/cli/dispatch.ts +++ b/src/cli/dispatch.ts @@ -87,16 +87,17 @@ export async function dispatch( return err(req.id, 'forbidden', 'CLI access is scoped to this agent group.'); } - // Block cli_scope changes from group-scoped agents (privilege escalation) - if (req.args.cli_scope !== undefined || req.args['cli-scope'] !== undefined) { - return err(req.id, 'forbidden', 'Cannot change cli_scope from a group-scoped agent.'); - } - - // Same escalation shape for harness capabilities: an agent must not be - // able to re-enable harness features (agent teams, Workflow) that the - // operator turned off for its group. - if (req.args.harness_capabilities !== undefined || req.args['harness-capabilities'] !== undefined) { - return err(req.id, 'forbidden', 'Cannot change harness_capabilities from a group-scoped agent.'); + // Operator-only config fields — block changes from group-scoped agents + // (privilege escalation: cli_scope widens CLI access, harness + // capabilities re-enable harness features the operator turned off). This + // gate runs on RAW args before normalizeArgs, so we normalize each + // incoming key to canonical snake_case and compare — one spelling per + // protected field, no spelling-variant bypass. Add future fields HERE. + const OPERATOR_ONLY_ARGS = new Set(['cli_scope', 'harness_capabilities']); + for (const key of Object.keys(req.args)) { + if (OPERATOR_ONLY_ARGS.has(key.replace(/-/g, '_'))) { + return err(req.id, 'forbidden', `Cannot change ${key.replace(/-/g, '_')} from a group-scoped agent.`); + } } // Auto-fill agent-group-related args so the agent doesn't need diff --git a/src/cli/resources/groups-config.test.ts b/src/cli/resources/groups-config.test.ts index 013b588ad..e66226fa2 100644 --- a/src/cli/resources/groups-config.test.ts +++ b/src/cli/resources/groups-config.test.ts @@ -23,7 +23,7 @@ vi.mock('../../config.js', async () => { const TEST_DIR = '/tmp/nanoclaw-test-cli-groups-config'; import { initTestDb, closeDb, runMigrations, createAgentGroup } from '../../db/index.js'; -import { ensureContainerConfig, getContainerConfig } from '../../db/container-configs.js'; +import { ensureContainerConfig, getContainerConfig, updateContainerConfigJson } from '../../db/container-configs.js'; import { dispatch } from '../dispatch.js'; // Side-effect import: registers the `groups-*` commands. import './groups.js'; @@ -58,30 +58,32 @@ afterEach(() => { }); describe('groups config update --harness-capabilities', () => { - it('sets an override and renders it as (override) in the resolved view', async () => { + type Resolved = Record; + + it('sets an override and marks it as an override in the resolved view', async () => { const resp = await configUpdate('agent-teams=on'); expect(resp.ok).toBe(true); if (resp.ok) { const data = resp.data as { harness_capabilities: Record; - harness_capabilities_resolved: Record; + harness_capabilities_resolved: Resolved; }; expect(data.harness_capabilities).toEqual({ 'agent-teams': 'on' }); - expect(data.harness_capabilities_resolved['agent-teams']).toBe('on (override)'); - expect(data.harness_capabilities_resolved.workflow).toBe('off (default)'); + expect(data.harness_capabilities_resolved['agent-teams']).toEqual({ state: 'on', source: 'override' }); + expect(data.harness_capabilities_resolved.workflow).toEqual({ state: 'off', source: 'default' }); } expect(JSON.parse(getContainerConfig(GID)!.harness_capabilities)).toEqual({ 'agent-teams': 'on' }); }); - it('`default` clears the override and the resolved view returns to (default)', async () => { + it('`default` clears the override and the resolved view returns to default', async () => { await configUpdate('agent-teams=on'); const resp = await configUpdate('agent-teams=default'); expect(resp.ok).toBe(true); if (resp.ok) { - const data = resp.data as { harness_capabilities_resolved: Record }; - expect(data.harness_capabilities_resolved['agent-teams']).toBe('off (default)'); + const data = resp.data as { harness_capabilities_resolved: Resolved }; + expect(data.harness_capabilities_resolved['agent-teams']).toEqual({ state: 'off', source: 'default' }); } expect(JSON.parse(getContainerConfig(GID)!.harness_capabilities)).toEqual({}); }); @@ -109,13 +111,25 @@ describe('groups config update --harness-capabilities', () => { if (resp.ok) { const data = resp.data as { harness_capabilities: Record; - harness_capabilities_resolved: Record; + harness_capabilities_resolved: Resolved; }; expect(data.harness_capabilities).toEqual({ workflow: 'on' }); expect(data.harness_capabilities_resolved).toEqual({ - 'agent-teams': 'off (default)', - workflow: 'on (override)', + 'agent-teams': { state: 'off', source: 'default' }, + workflow: { state: 'on', source: 'override' }, }); } }); + + it('a stored invalid value reports source=default, not a lying override', async () => { + // Direct DB write bypassing validation (version skew / manual edit). + updateContainerConfigJson(GID, 'harness_capabilities', { workflow: 'sideways' }); + const resp = await dispatch({ id: 't', command: 'groups-config-get', args: { id: GID } }, hostCtx); + + expect(resp.ok).toBe(true); + if (resp.ok) { + const data = resp.data as { harness_capabilities_resolved: Resolved }; + expect(data.harness_capabilities_resolved.workflow).toEqual({ state: 'off', source: 'default' }); + } + }); }); diff --git a/src/cli/resources/groups.ts b/src/cli/resources/groups.ts index 42d348e3e..0a0945377 100644 --- a/src/cli/resources/groups.ts +++ b/src/cli/resources/groups.ts @@ -14,25 +14,19 @@ import { } from '../../db/container-configs.js'; import { initGroupFilesystem } from '../../group-init.js'; import { createAgentFromTemplate } from '../../templates/create-agent.js'; -import { parseHarnessCapabilitiesArg, resolveHarnessCapabilities } from '../../harness-capabilities.js'; +import { + HARNESS_CAPABILITY_KEYS, + parseHarnessCapabilitiesArg, + parseHarnessOverrides, + resolveHarnessCapabilities, +} from '../../harness-capabilities.js'; import type { AgentGroup, ContainerConfigRow } from '../../types.js'; import { registerResource } from '../crud.js'; -/** Raw override map from the stored JSON; malformed degrades to {} (resolve warns). */ -function harnessOverrides(row: ContainerConfigRow): Record { - try { - const parsed: unknown = JSON.parse(row.harness_capabilities); - return parsed && typeof parsed === 'object' && !Array.isArray(parsed) ? (parsed as Record) : {}; - } catch (err) { - if (!(err instanceof SyntaxError)) throw err; - return {}; - } -} - /** Deserialize JSON columns for display. */ function presentConfig(row: ContainerConfigRow): Record { - const overrides = harnessOverrides(row); - const resolved = resolveHarnessCapabilities(row.harness_capabilities); + const overrides = parseHarnessOverrides(row.harness_capabilities); + const resolved = resolveHarnessCapabilities(overrides); return { agent_group_id: row.agent_group_id, provider: row.provider, @@ -48,8 +42,15 @@ function presentConfig(row: ContainerConfigRow): Record { additional_mounts: JSON.parse(row.additional_mounts), cli_scope: row.cli_scope, harness_capabilities: overrides, + // Structured {state, source} per key — machine-readable and honest: source + // is 'override' only when a VALID override is stored (an invalid stored + // value resolves to the default, so it reports 'default', not a lie). harness_capabilities_resolved: Object.fromEntries( - Object.entries(resolved).map(([k, v]) => [k, `${v} (${k in overrides ? 'override' : 'default'})`]), + Object.entries(resolved).map(([k, state]) => { + const ov = (overrides as Record)[k]; + const source = ov === 'on' || ov === 'off' ? 'override' : 'default'; + return [k, { state, source }]; + }), ), updated_at: row.updated_at, }; @@ -272,7 +273,7 @@ registerResource({ description: 'Update container config fields. Changes are saved but do NOT take effect until you run `ncl groups restart`. ' + 'Use --id and any of: --provider, --model, --effort, --image-tag, --assistant-name, --max-messages-per-prompt, --cli-scope, ' + - "--harness-capabilities 'key=on|off|default[,key=...]' (keys: agent-teams, workflow; `default` clears the per-group override).", + `--harness-capabilities 'key=on|off|default[,key=...]' (keys: ${HARNESS_CAPABILITY_KEYS.join(', ')}; \`default\` clears the per-group override).`, handler: async (args) => { const id = args.id as string; if (!id) throw new Error('--id is required'); @@ -300,26 +301,31 @@ registerResource({ updates.cli_scope = scope; } - // Harness capabilities are a JSON column of sparse overrides — apply - // set/clear ops on the stored map (`default` clears; never stored). - let harnessChanged = false; - const capsArg = (args['harness-capabilities'] ?? args.harness_capabilities) as string | undefined; - if (capsArg !== undefined) { - const ops = parseHarnessCapabilitiesArg(String(capsArg)); - const overrides = harnessOverrides(row); - for (const key of ops.clear) delete overrides[key]; - Object.assign(overrides, ops.set); - updateContainerConfigJson(id, 'harness_capabilities', overrides); - harnessChanged = true; + // Harness capabilities are a sparse-override JSON column. parse returns + // a key→directive map (last-wins per key); apply each: `default` clears + // the override, on/off set it. + let nextOverrides: Record | undefined; + if (args.harness_capabilities !== undefined) { + const directives = parseHarnessCapabilitiesArg(String(args.harness_capabilities)); + nextOverrides = parseHarnessOverrides(row.harness_capabilities); + for (const [key, directive] of Object.entries(directives)) { + if (directive === 'default') delete nextOverrides[key]; + else nextOverrides[key] = directive; + } } - if (Object.keys(updates).length === 0 && !harnessChanged) { + if (Object.keys(updates).length === 0 && nextOverrides === undefined) { throw new Error( 'Nothing to update — provide at least one of: --provider, --model, --effort, --image-tag, --assistant-name, --max-messages-per-prompt, --cli-scope, --harness-capabilities', ); } - if (Object.keys(updates).length > 0) updateContainerConfigScalars(id, updates); + // Single transaction: a failure in either write (e.g. a bad scalar + // value) rolls back both, so the command can never half-apply. + getDb().transaction(() => { + if (nextOverrides !== undefined) updateContainerConfigJson(id, 'harness_capabilities', nextOverrides); + if (Object.keys(updates).length > 0) updateContainerConfigScalars(id, updates); + })(); const updated = getContainerConfig(id)!; return presentConfig(updated); diff --git a/src/db/container-configs.ts b/src/db/container-configs.ts index b86481224..645af8a80 100644 --- a/src/db/container-configs.ts +++ b/src/db/container-configs.ts @@ -36,11 +36,11 @@ export function createContainerConfig(config: ContainerConfigRow): void { `INSERT INTO container_configs ( agent_group_id, provider, model, effort, image_tag, assistant_name, max_messages_per_prompt, skills, mcp_servers, packages_apt, packages_npm, - additional_mounts, updated_at + additional_mounts, cli_scope, harness_capabilities, updated_at ) VALUES ( @agent_group_id, @provider, @model, @effort, @image_tag, @assistant_name, @max_messages_per_prompt, @skills, @mcp_servers, @packages_apt, @packages_npm, - @additional_mounts, @updated_at + @additional_mounts, @cli_scope, @harness_capabilities, @updated_at )`, ) .run(config); diff --git a/src/group-init.ts b/src/group-init.ts index 915b43f29..75feb2963 100644 --- a/src/group-init.ts +++ b/src/group-init.ts @@ -1,39 +1,30 @@ import fs from 'fs'; import path from 'path'; +import { writeAtomic } from './claude-md-compose.js'; import { DATA_DIR, GROUPS_DIR } from './config.js'; import { ensureContainerConfig } from './db/container-configs.js'; +import { HARNESS_CAPABILITIES } from './harness-capabilities.js'; import { log } from './log.js'; import { providerProvidesAgentSurfaces } from './providers/provider-container-registry.js'; import type { HarnessCapabilityState } from './harness-capabilities.js'; import type { AgentGroup } from './types.js'; -// Managed harness keys (the teams env key, disableWorkflows) are deliberately -// NOT in the static default — they enter settings.json exclusively through -// reconcileHarnessSettings() from the group's resolved capability state. -const DEFAULT_SETTINGS_JSON = - JSON.stringify( - { - env: { - CLAUDE_CODE_ADDITIONAL_DIRECTORIES_CLAUDE_MD: '1', - CLAUDE_CODE_DISABLE_AUTO_MEMORY: '0', - }, - hooks: { - PreCompact: [ - { - hooks: [ - { - type: 'command', - command: 'bun /app/src/compact-instructions.ts', - }, - ], - }, - ], - }, - }, - null, - 2, - ) + '\n'; +const PRE_COMPACT_COMMAND = 'bun /app/src/compact-instructions.ts'; + +// Base settings for a brand-new group. Managed harness keys (the teams env key, +// disableWorkflows) are deliberately NOT here — they enter settings.json +// exclusively through the reconciler from the group's resolved capability state, +// applied on top of this base in the same write. +const DEFAULT_SETTINGS = { + env: { + CLAUDE_CODE_ADDITIONAL_DIRECTORIES_CLAUDE_MD: '1', + CLAUDE_CODE_DISABLE_AUTO_MEMORY: '0', + }, + hooks: { + PreCompact: [{ hooks: [{ type: 'command', command: PRE_COMPACT_COMMAND }] }], + }, +}; /** * Initialize the on-disk filesystem state for an agent group. Idempotent — @@ -127,19 +118,7 @@ export function initGroupFilesystem( } const settingsFile = path.join(claudeDir, 'settings.json'); - if (!fs.existsSync(settingsFile)) { - fs.writeFileSync(settingsFile, DEFAULT_SETTINGS_JSON); - initialized.push('settings.json'); - } else { - ensurePreCompactHook(settingsFile, initialized); - } - - // Runs in BOTH branches: a freshly written default must be reconciled in - // the same call, or the group's first container lifetime ships without - // its capability state (the default file deliberately omits managed keys). - if (opts?.harnessCapabilities) { - reconcileHarnessSettings(settingsFile, opts.harnessCapabilities, initialized); - } + applyGroupSettings(settingsFile, opts?.harnessCapabilities, initialized); // Skills directory — created empty here; symlinks are synced at spawn // time by container-runner.ts based on container.json skills selection. @@ -160,87 +139,95 @@ export function initGroupFilesystem( } } -const PRE_COMPACT_COMMAND = 'bun /app/src/compact-instructions.ts'; +type SettingsObject = Record; -/** - * Patch an existing settings.json to add the PreCompact hook if missing. - * Runs on every group init so pre-existing groups pick up the hook. - */ -function ensurePreCompactHook(settingsFile: string, initialized: string[]): void { - try { - const raw = fs.readFileSync(settingsFile, 'utf-8'); - const settings = JSON.parse(raw); - - // Check if there's already a PreCompact hook with our command. - const existing = settings.hooks?.PreCompact as unknown[] | undefined; - if (existing && JSON.stringify(existing).includes(PRE_COMPACT_COMMAND)) return; - - // Add the hook, preserving existing hooks. - if (!settings.hooks) settings.hooks = {}; - if (!settings.hooks.PreCompact) settings.hooks.PreCompact = []; - settings.hooks.PreCompact.push({ - hooks: [{ type: 'command', command: PRE_COMPACT_COMMAND }], - }); - - fs.writeFileSync(settingsFile, JSON.stringify(settings, null, 2) + '\n'); - initialized.push('settings.json (added PreCompact hook)'); - } catch { - // Don't break init if settings.json is malformed — it'll use whatever's there. - } +function asObject(value: unknown): SettingsObject | null { + return value && typeof value === 'object' && !Array.isArray(value) ? (value as SettingsObject) : null; } -const TEAMS_ENV_KEY = 'CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS'; - /** - * Reconcile the managed harness-capability keys in settings.json to the - * group's resolved capability state. Manages exactly two keys — the - * agent-teams env key and `disableWorkflows` — adding AND removing them; - * everything else in the file (operator hand-edits, the PreCompact hook, - * unmanaged env keys) is preserved verbatim. Runs on every spawn, so - * pre-existing groups converge to the configured state, including removal - * of the legacy always-on teams key. See docs/harness-capabilities.md. + * Bring a group's settings.json to its desired state in a single read-modify- + * write: ensure the PreCompact hook is present and reconcile the managed + * harness-capability keys to `caps`. A fresh file is composed from + * DEFAULT_SETTINGS + the capability keys in one atomic write; an existing file + * is mutated in memory and rewritten only if it changed. Any content that isn't + * a JSON object (malformed, `null`, a scalar, an array) is left untouched with + * a warning — settings trouble must never break group init or block a spawn. */ -function reconcileHarnessSettings( +function applyGroupSettings( settingsFile: string, - caps: Record, + caps: Record | undefined, initialized: string[], ): void { + if (!fs.existsSync(settingsFile)) { + const settings = JSON.parse(JSON.stringify(DEFAULT_SETTINGS)) as SettingsObject; + reconcileHarnessKeys(settings, caps); + writeAtomic(settingsFile, JSON.stringify(settings, null, 2) + '\n'); + initialized.push('settings.json'); + return; + } + + let raw: string; + let parsed: unknown; try { - const raw = fs.readFileSync(settingsFile, 'utf-8'); - const settings = JSON.parse(raw) as Record & { env?: Record }; - - // agent-teams: the settings env key is the ONLY working switch on the - // pinned CLI — settings env strictly beats SDK options env, so presence - // in this file IS the state. - if (caps['agent-teams'] === 'on') { - if (!settings.env) settings.env = {}; - settings.env[TEAMS_ENV_KEY] = '1'; - } else if (settings.env && TEAMS_ENV_KEY in settings.env) { - delete settings.env[TEAMS_ENV_KEY]; - } - - // workflow: disableWorkflows removes the Workflow tool AND its agent-types - // catalog block from every request. The runner's disallowedTools backstop - // covers a malformed or hand-reverted file — that backstop is what makes - // the warn-and-continue below acceptable. - if (caps.workflow === 'off') { - settings.disableWorkflows = true; - } else if ('disableWorkflows' in settings) { - delete settings.disableWorkflows; - } - - const next = JSON.stringify(settings, null, 2) + '\n'; - if (next === raw) return; // no churn on the every-spawn path - - // tmp+rename: two sessions of one group can spawn concurrently; both - // compute identical output from the same DB row, so last-rename-wins is - // harmless — but a torn write mid-read by the CLI would not be. - const tmp = `${settingsFile}.tmp-${process.pid}`; - fs.writeFileSync(tmp, next); - fs.renameSync(tmp, settingsFile); - initialized.push('settings.json (reconciled harness capabilities)'); + raw = fs.readFileSync(settingsFile, 'utf-8'); + parsed = JSON.parse(raw); } catch (err) { if (!(err instanceof SyntaxError)) throw err; - log.warn('settings.json is malformed — skipping harness-capability reconcile', { settingsFile }); + log.warn('settings.json is malformed — leaving it untouched', { settingsFile }); + return; + } + const settings = asObject(parsed); + if (!settings) { + log.warn('settings.json is not a JSON object — leaving it untouched', { settingsFile }); + return; + } + + ensurePreCompactHook(settings); + reconcileHarnessKeys(settings, caps); + + const next = JSON.stringify(settings, null, 2) + '\n'; + if (next === raw) return; // no churn on the every-spawn path + writeAtomic(settingsFile, next); + initialized.push('settings.json (updated)'); +} + +/** Ensure the PreCompact archiving hook is present, preserving existing hooks. */ +function ensurePreCompactHook(settings: SettingsObject): void { + const hooks = asObject(settings.hooks) ?? (settings.hooks = {}); + const existing = Array.isArray(hooks.PreCompact) ? (hooks.PreCompact as unknown[]) : (hooks.PreCompact = []); + if (JSON.stringify(existing).includes(PRE_COMPACT_COMMAND)) return; + existing.push({ hooks: [{ type: 'command', command: PRE_COMPACT_COMMAND }] }); +} + +/** + * Reconcile the managed harness-capability keys to `caps`, iterating the + * registry's host mechanisms — adding AND removing each key so pre-existing + * groups converge (including removal of the legacy always-on teams key). + * Everything unmanaged in the file is preserved. `caps` undefined (non-spawn + * callers) leaves capability keys untouched. + */ +function reconcileHarnessKeys( + settings: SettingsObject, + caps: Record | undefined, +): void { + if (!caps) return; + for (const [key, def] of Object.entries(HARNESS_CAPABILITIES)) { + const state = caps[key] ?? def.default; + const m = def.host; + if (m.kind === 'env') { + // Presence of the env key IS the on-state. + if (state === 'on') { + const env = asObject(settings.env) ?? (settings.env = {}); + env[m.key] = '1'; + } else { + const env = asObject(settings.env); + if (env && m.key in env) delete env[m.key]; + } + } else { + // disableFlag: the flag is set true when the capability is OFF. + if (state === 'off') settings[m.key] = true; + else if (m.key in settings) delete settings[m.key]; + } } } diff --git a/src/harness-capabilities.test.ts b/src/harness-capabilities.test.ts index 205e7cd7c..756368f23 100644 --- a/src/harness-capabilities.test.ts +++ b/src/harness-capabilities.test.ts @@ -28,30 +28,47 @@ describe('resolveHarnessCapabilities', () => { expect(resolveHarnessCapabilities('"off"')).toEqual(HARNESS_CAPABILITY_DEFAULTS); }); - it('passes unknown keys through and keeps defaults for garbage values on known keys', () => { - const resolved = resolveHarnessCapabilities('{"monitor":"off","workflow":"sideways"}'); - expect(resolved.monitor).toBe('off'); + it('drops unknown keys and garbage values back to defaults (never leaks into the resolved map)', () => { + const resolved = resolveHarnessCapabilities('{"monitor":"off","workflow":"sideways","future":{"nested":1}}'); + expect(Object.hasOwn(resolved, 'monitor')).toBe(false); // unknown key dropped + expect(Object.hasOwn(resolved, 'future')).toBe(false); // unknown non-string dropped expect(resolved.workflow).toBe('off'); // garbage value → default, not 'sideways' expect(resolved['agent-teams']).toBe('off'); }); + + it('accepts an already-parsed override map', () => { + expect(resolveHarnessCapabilities({ workflow: 'on' })).toEqual({ 'agent-teams': 'off', workflow: 'on' }); + }); + + it('does not treat inherited Object.prototype names as known keys', () => { + // `key in obj` would be true for 'constructor'/'toString'; Object.hasOwn is not. + const resolved = resolveHarnessCapabilities('{"constructor":"on","toString":"off"}'); + expect(resolved).toEqual({ 'agent-teams': 'off', workflow: 'off' }); + }); }); describe('parseHarnessCapabilitiesArg', () => { - it('parses set and clear operations', () => { - expect(parseHarnessCapabilitiesArg('agent-teams=on')).toEqual({ set: { 'agent-teams': 'on' }, clear: [] }); - expect(parseHarnessCapabilitiesArg('workflow=default')).toEqual({ set: {}, clear: ['workflow'] }); + it('parses on/off/default directives into a key→directive map', () => { + expect(parseHarnessCapabilitiesArg('agent-teams=on')).toEqual({ 'agent-teams': 'on' }); + expect(parseHarnessCapabilitiesArg('workflow=default')).toEqual({ workflow: 'default' }); expect(parseHarnessCapabilitiesArg('agent-teams=on, workflow=default')).toEqual({ - set: { 'agent-teams': 'on' }, - clear: ['workflow'], + 'agent-teams': 'on', + workflow: 'default', }); }); - it('normalizes key case and underscores', () => { - expect(parseHarnessCapabilitiesArg('AGENT_TEAMS=ON')).toEqual({ set: { 'agent-teams': 'on' }, clear: [] }); + it('resolves a repeated key last-wins', () => { + expect(parseHarnessCapabilitiesArg('workflow=on,workflow=default')).toEqual({ workflow: 'default' }); + expect(parseHarnessCapabilitiesArg('workflow=default,workflow=on')).toEqual({ workflow: 'on' }); }); - it('rejects unknown keys with the allowed set in the message', () => { + it('normalizes key case and underscores', () => { + expect(parseHarnessCapabilitiesArg('AGENT_TEAMS=ON')).toEqual({ 'agent-teams': 'on' }); + }); + + it('rejects unknown keys, including inherited prototype names', () => { expect(() => parseHarnessCapabilitiesArg('web=off')).toThrow(/unknown harness capability "web".*agent-teams/); + expect(() => parseHarnessCapabilitiesArg('constructor=on')).toThrow(/unknown harness capability "constructor"/); }); it('rejects bad values and malformed pairs', () => { diff --git a/src/harness-capabilities.ts b/src/harness-capabilities.ts index 65e35b785..9bb8eafdc 100644 --- a/src/harness-capabilities.ts +++ b/src/harness-capabilities.ts @@ -1,70 +1,113 @@ /** * Harness-capability registry: which harness-native features NanoClaw exposes - * as per-group toggles, and their code defaults. + * as per-group toggles, their code defaults, and how the host applies each one + * to a group's settings.json. * - * Policy lives here on the host; mechanism lives provider-side in the agent - * runner. Per-group overrides are a sparse JSON map in - * `container_configs.harness_capabilities`; the RESOLVED map (defaults ⊕ - * overrides) is what gets materialized into container.json and applied to the - * group's settings.json by the reconciler in group-init.ts. Fixed-off - * capabilities (scheduling, ask-user, plan/worktree, DesignSync) are not keys - * here by design — a toggle nobody can meaningfully use is surface without - * value. See docs/harness-capabilities.md. + * This is the single host-side source of truth. Per-group overrides are a + * sparse JSON map in `container_configs.harness_capabilities`; the RESOLVED map + * (defaults ⊕ overrides) is materialized into container.json and applied to the + * group's settings.json by the reconciler in group-init.ts, which iterates the + * `host` mechanism of every entry below — so adding a capability is one entry + * here plus (if it also needs a runtime tool block) one line in the runner's + * CAPABILITY_DISALLOWS map. Fixed-off capabilities (scheduling, ask-user, + * plan/worktree, DesignSync) are not keys here by design — a toggle nobody can + * meaningfully use is surface without value. See docs/harness-capabilities.md. */ import { log } from './log.js'; export type HarnessCapabilityState = 'on' | 'off'; /** - * Configurable keys and their defaults. Adding a key is additive — no schema - * change. Keys are lowercase kebab-case; both toggles default OFF because - * NanoClaw's own systems (a2a messaging, host-side orchestration) are the - * authoritative equivalents. + * How the settings reconciler expresses a capability in settings.json. + * - `env`: the presence of an env key IS the on-state (the key is written when + * on, deleted when off). Verified on the pinned CLI: settings env strictly + * beats SDK options env, so this file is the only working switch. + * - `disableFlag`: a top-level boolean set `true` when the capability is OFF + * (disable-style flag), deleted when on. */ -export const HARNESS_CAPABILITY_DEFAULTS: Record = { - 'agent-teams': 'off', - workflow: 'off', +export type HostMechanism = { kind: 'env'; key: string } | { kind: 'disableFlag'; key: string }; + +export interface CapabilityDef { + default: HarnessCapabilityState; + host: HostMechanism; +} + +/** + * Configurable capabilities. Both default OFF because NanoClaw's own systems + * (a2a messaging, host-side orchestration) are the authoritative equivalents. + */ +export const HARNESS_CAPABILITIES: Record = { + 'agent-teams': { default: 'off', host: { kind: 'env', key: 'CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS' } }, + workflow: { default: 'off', host: { kind: 'disableFlag', key: 'disableWorkflows' } }, }; +export const HARNESS_CAPABILITY_KEYS = Object.keys(HARNESS_CAPABILITIES); + +/** Derived key→default map, for callers that only need the defaults. */ +export const HARNESS_CAPABILITY_DEFAULTS: Record = Object.fromEntries( + Object.entries(HARNESS_CAPABILITIES).map(([key, def]) => [key, def.default]), +); + const VALID_STATES = new Set(['on', 'off']); +function isKnownKey(key: string): boolean { + return Object.hasOwn(HARNESS_CAPABILITIES, key); +} + +// Durable bad state (a stale key or garbage value persisted in the DB) would +// otherwise log on every spawn — under scheduled-task wakes, forever. Warn once +// per distinct problem per host process instead, so real warnings aren't buried. +const warnedOnce = new Set(); +function warnOnce(signature: string, message: string, data: Record): void { + if (warnedOnce.has(signature)) return; + warnedOnce.add(signature); + log.warn(message, data); +} + /** - * Resolve a group's stored override JSON against the code defaults. - * - * Malformed JSON degrades to defaults with a warning rather than throwing: - * unlike structural config (mcp_servers, mounts), capabilities have a safe - * fallback, and the only sanctioned write path (ncl) validates its input. - * Unknown keys pass through untouched (with a warning) so the runner's own - * unknown-key diagnostics stay exercised; known keys with garbage values - * fall back to their default. + * Parse a group's stored override JSON into the raw sparse map. Malformed or + * non-object JSON degrades to {} with a warning rather than throwing: unlike + * structural config (mcp_servers, mounts), capabilities have a safe fallback, + * and the only sanctioned write path (ncl) validates its input. + */ +export function parseHarnessOverrides(overridesJson: string | null | undefined): Record { + if (!overridesJson) return {}; + try { + const parsed: unknown = JSON.parse(overridesJson); + if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) { + return parsed as Record; + } + warnOnce(`shape:${overridesJson}`, 'harness_capabilities is not a JSON object — using defaults', { + value: overridesJson, + }); + } catch (err) { + if (!(err instanceof SyntaxError)) throw err; + warnOnce(`syntax:${overridesJson}`, 'harness_capabilities is malformed JSON — using defaults', { + value: overridesJson, + }); + } + return {}; +} + +/** + * Resolve stored overrides (JSON string, or an already-parsed map) against the + * code defaults. Unknown keys and invalid values are dropped (they fall back to + * defaults) so the resolved map only ever contains known keys with valid + * states — the runner therefore never has to reason about unknown keys. */ export function resolveHarnessCapabilities( - overridesJson: string | null | undefined, + overrides: string | Record | null | undefined, ): Record { - let overrides: Record = {}; - if (overridesJson) { - try { - const parsed: unknown = JSON.parse(overridesJson); - if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) { - overrides = parsed as Record; - } else { - log.warn('harness_capabilities is not a JSON object — using defaults', { value: overridesJson }); - } - } catch (err) { - if (!(err instanceof SyntaxError)) throw err; - log.warn('harness_capabilities is malformed JSON — using defaults', { value: overridesJson }); - } - } + const raw = typeof overrides === 'string' || overrides == null ? parseHarnessOverrides(overrides) : overrides; const resolved: Record = { ...HARNESS_CAPABILITY_DEFAULTS }; - for (const [key, value] of Object.entries(overrides)) { - if (!(key in HARNESS_CAPABILITY_DEFAULTS)) { - log.warn('Unknown harness capability key in overrides — passing through', { key }); - resolved[key] = value as HarnessCapabilityState; + for (const [key, value] of Object.entries(raw)) { + if (!isKnownKey(key)) { + warnOnce(`unknown:${key}`, 'Unknown harness capability key in overrides — dropping', { key }); continue; } if (typeof value !== 'string' || !VALID_STATES.has(value)) { - log.warn('Invalid harness capability value — using default', { key, value }); + warnOnce(`value:${key}:${String(value)}`, 'Invalid harness capability value — using default', { key, value }); continue; } resolved[key] = value as HarnessCapabilityState; @@ -72,21 +115,17 @@ export function resolveHarnessCapabilities( return resolved; } -export interface HarnessCapabilityOps { - set: Record; - clear: string[]; -} - /** * Parse the ncl `--harness-capabilities` flag value: comma-separated `k=v` * pairs where v is `on`, `off`, or `default` (`default` clears the override — * it is never stored). Keys are normalized (trim, lowercase, `_`→`-`) and - * validated against the registry. Throws with an actionable message on the - * first problem. + * validated against the registry. Returns a plain key→directive map, so a + * repeated key resolves last-wins by ordinary assignment. Throws with an + * actionable message on the first problem. */ -export function parseHarnessCapabilitiesArg(input: string): HarnessCapabilityOps { - const allowed = Object.keys(HARNESS_CAPABILITY_DEFAULTS).join(', '); - const ops: HarnessCapabilityOps = { set: {}, clear: [] }; +export function parseHarnessCapabilitiesArg(input: string): Record { + const allowed = HARNESS_CAPABILITY_KEYS.join(', '); + const out: Record = {}; const pairs = input .split(',') .map((p) => p.trim()) @@ -104,16 +143,14 @@ export function parseHarnessCapabilitiesArg(input: string): HarnessCapabilityOps .slice(eq + 1) .trim() .toLowerCase(); - if (!(key in HARNESS_CAPABILITY_DEFAULTS)) { + if (!isKnownKey(key)) { throw new Error(`unknown harness capability "${key}" — configurable keys: ${allowed}`); } - if (value === 'default') { - ops.clear.push(key); - } else if (VALID_STATES.has(value)) { - ops.set[key] = value as HarnessCapabilityState; + if (value === 'default' || VALID_STATES.has(value)) { + out[key] = value as HarnessCapabilityState | 'default'; } else { throw new Error(`harness capability "${key}" must be on, off, or default — got "${value}"`); } } - return ops; + return out; }