diff --git a/src/lib/agent/__tests__/settings-conflict-planner.test.ts b/src/lib/agent/__tests__/settings-conflict-planner.test.ts new file mode 100644 index 00000000..f62e5f13 --- /dev/null +++ b/src/lib/agent/__tests__/settings-conflict-planner.test.ts @@ -0,0 +1,202 @@ +/** + * Tests for the pure settings-conflict planner. The planner mirrors the + * orchestration that used to live inline in `runner/shared/bootstrap.ts`: + * given the conflicts returned by `checkAllSettingsConflicts`, it produces + * the log lines, analytics events, and decisions the executor must perform — + * without performing any of them itself. Issue #756. + */ + +import type { SettingsConflict } from '@lib/agent/claude-settings'; +import { + planSettingsConflictActions, + planAutoFixOutcome, +} from '@lib/agent/settings-conflict-planner'; + +const make = ( + source: SettingsConflict['source'], + writable: boolean, + keys: string[] = ['ANTHROPIC_BASE_URL'], +): SettingsConflict => ({ + source, + path: `/tmp/.claude/${source}.json`, + keys, + writable, +}); + +describe('planSettingsConflictActions', () => { + it('emits a single "none" log line and no other actions when there are no conflicts', () => { + const plan = planSettingsConflictActions([]); + + expect(plan.pre).toEqual([ + { kind: 'log', message: '[agent-runner] settings conflicts: none' }, + ]); + expect(plan.autoFix).toBeNull(); + expect(plan.failClosed).toEqual([]); + expect(plan.autoFixCandidates).toEqual([]); + }); + + it('summarises every detected conflict in the initial log line with source(keys)', () => { + const conflicts = [ + make('user', false, ['apiKeyHelper']), + make('project', true, ['ANTHROPIC_BASE_URL', 'apiKeyHelper']), + ]; + + const plan = planSettingsConflictActions(conflicts); + + expect(plan.pre[0]).toEqual({ + kind: 'log', + message: + '[agent-runner] settings conflicts: user(apiKeyHelper); project(ANTHROPIC_BASE_URL,apiKeyHelper)', + }); + }); + + it('emits a "settings conflict detected" analytics event per conflict, remapping managed→org', () => { + const conflicts = [ + make('managed', false, ['apiKeyHelper']), + make('user', false, ['ANTHROPIC_BASE_URL']), + make('project', true, ['apiKeyHelper']), + make('project-local', false, ['ANTHROPIC_BASE_URL']), + ]; + + const detected = planSettingsConflictActions(conflicts).pre.filter( + (a) => a.kind === 'analytics' && a.event === 'settings conflict detected', + ); + + expect(detected).toEqual([ + { + kind: 'analytics', + event: 'settings conflict detected', + props: { level: 'org', keys: ['apiKeyHelper'] }, + }, + { + kind: 'analytics', + event: 'settings conflict detected', + props: { level: 'user', keys: ['ANTHROPIC_BASE_URL'] }, + }, + { + kind: 'analytics', + event: 'settings conflict detected', + props: { level: 'project', keys: ['apiKeyHelper'] }, + }, + { + kind: 'analytics', + event: 'settings conflict detected', + props: { level: 'project-local', keys: ['ANTHROPIC_BASE_URL'] }, + }, + ]); + }); + + it('emits a neutralized log + analytics for each warn-only (user/project-local) conflict', () => { + const userConflict = make('user', false, ['apiKeyHelper']); + const localConflict = make('project-local', false, ['ANTHROPIC_BASE_URL']); + + const plan = planSettingsConflictActions([userConflict, localConflict]); + + const neutralized = plan.pre.filter( + (a) => + (a.kind === 'log' && a.message.includes('neutralized by')) || + (a.kind === 'analytics' && a.event === 'settings conflict neutralized'), + ); + + expect(neutralized).toEqual([ + { + kind: 'log', + message: + `[agent-runner] settings conflict in user (${userConflict.path}) ` + + `neutralized by settingSources:['project'] — not blocking`, + }, + { + kind: 'analytics', + event: 'settings conflict neutralized', + props: { level: 'user', keys: ['apiKeyHelper'] }, + }, + { + kind: 'log', + message: + `[agent-runner] settings conflict in project-local (${localConflict.path}) ` + + `neutralized by settingSources:['project'] — not blocking`, + }, + { + kind: 'analytics', + event: 'settings conflict neutralized', + props: { level: 'project-local', keys: ['ANTHROPIC_BASE_URL'] }, + }, + ]); + }); + + it('schedules auto-fix for writable conflicts and exposes the key list for the success event', () => { + const projA = make('project', true, ['apiKeyHelper']); + const projB = make('project', true, ['ANTHROPIC_BASE_URL']); + + const plan = planSettingsConflictActions([projA, projB]); + + expect(plan.autoFix).toEqual({ + keys: ['apiKeyHelper', 'ANTHROPIC_BASE_URL'], + }); + expect(plan.autoFixCandidates).toEqual([projA, projB]); + expect(plan.failClosed).toEqual([]); + }); + + it('separates fail-closed (managed) from auto-fixable conflicts without scheduling auto-fix for managed', () => { + const managed = make('managed', false, ['apiKeyHelper']); + + const plan = planSettingsConflictActions([managed]); + + expect(plan.autoFix).toBeNull(); + expect(plan.failClosed).toEqual([managed]); + expect(plan.autoFixCandidates).toEqual([]); + }); +}); + +describe('planAutoFixOutcome', () => { + const projA = make('project', true, ['apiKeyHelper']); + const managed = make('managed', false, ['apiKeyHelper']); + + it('on success: emits the auto-neutralized log + analytics and leaves only fail-closed unfixable', () => { + const plan = planSettingsConflictActions([projA, managed]); + + const outcome = planAutoFixOutcome(plan, true); + + expect(outcome.actions).toEqual([ + { + kind: 'log', + message: '[agent-runner] auto-neutralized writable settings conflict', + }, + { + kind: 'analytics', + event: 'settings conflict auto-neutralized', + props: { keys: ['apiKeyHelper'] }, + }, + ]); + expect(outcome.unfixable).toEqual([managed]); + }); + + it('on failure: emits the failing-closed log and adds the auto-fix candidates to unfixable', () => { + const plan = planSettingsConflictActions([projA, managed]); + + const outcome = planAutoFixOutcome(plan, false); + + expect(outcome.actions).toEqual([ + { + kind: 'log', + message: + '[agent-runner] could not back up writable settings conflict — failing closed', + }, + ]); + // fail-closed first (preserves prior ordering), then auto-fix candidates. + expect(outcome.unfixable).toEqual([managed, projA]); + }); + + it('when no auto-fix was scheduled, returns no actions and unfixable = failClosed verbatim', () => { + const plan = planSettingsConflictActions([managed]); + + expect(planAutoFixOutcome(plan, true)).toEqual({ + actions: [], + unfixable: [managed], + }); + expect(planAutoFixOutcome(plan, false)).toEqual({ + actions: [], + unfixable: [managed], + }); + }); +}); diff --git a/src/lib/agent/runner/shared/bootstrap.ts b/src/lib/agent/runner/shared/bootstrap.ts index a151e222..3ba30e92 100644 --- a/src/lib/agent/runner/shared/bootstrap.ts +++ b/src/lib/agent/runner/shared/bootstrap.ts @@ -15,8 +15,12 @@ import { buildRunTags } from '@lib/agent/agent-interface'; import { checkAllSettingsConflicts, backupAndFixClaudeSettings, - classifySettingsConflicts, } from '@lib/agent/claude-settings'; +import { + planSettingsConflictActions, + planAutoFixOutcome, + type PlannerAction, +} from '@lib/agent/settings-conflict-planner'; import { evaluateWizardReadiness, WizardReadiness, @@ -63,6 +67,19 @@ export function sessionToOptions(session: WizardSession): WizardRunOptions { }; } +/** + * Run a {@link PlannerAction} produced by the settings-conflict planner. + * Kept tiny on purpose: the planner owns the decisions, this owns the side + * effects (logToFile + analytics.wizardCapture). + */ +function runPlannerAction(action: PlannerAction): void { + if (action.kind === 'log') { + logToFile(action.message); + } else { + analytics.wizardCapture(action.event, action.props); + } +} + // ── Bootstrap ───────────────────────────────────────────────────────── /** @@ -136,72 +153,31 @@ export async function bootstrapProgram( } } - // 3. Settings conflicts + // 3. Settings conflicts — pure planner decides log/analytics/fail-closed; + // this loop is the imperative shell that performs side effects against the + // plan. See settings-conflict-planner.ts and issue #756. const settingsConflicts = checkAllSettingsConflicts(session.installDir); - logToFile( - `[agent-runner] settings conflicts: ${ - settingsConflicts.length > 0 - ? settingsConflicts - .map((c) => `${c.source}(${c.keys.join(',')})`) - .join('; ') - : 'none' - }`, - ); - - if (settingsConflicts.length > 0) { - for (const conflict of settingsConflicts) { - const level = conflict.source === 'managed' ? 'org' : conflict.source; - analytics.wizardCapture('settings conflict detected', { - level, - keys: conflict.keys, - }); - } - - const { autoFix, failClosed, warnOnly } = - classifySettingsConflicts(settingsConflicts); - - // User-global and project-local files are already neutralized — the agent - // runs with settingSources:['project'], so the SDK never reads them. Record - // it and move on; don't make the user act on a setting that can't bite. - for (const conflict of warnOnly) { - logToFile( - `[agent-runner] settings conflict in ${conflict.source} (${conflict.path}) ` + - `neutralized by settingSources:['project'] — not blocking`, - ); - analytics.wizardCapture('settings conflict neutralized', { - level: conflict.source, - keys: conflict.keys, - }); - } + const plan = planSettingsConflictActions(settingsConflicts); + for (const action of plan.pre) runPlannerAction(action); + let unfixable = plan.failClosed; + if (plan.autoFix !== null) { // Writable project settings.json — the SDK *does* read it, but we can back // it up and remove it (restored at outro). Neutralize without prompting. - let unfixable = failClosed; - if (autoFix.length > 0) { - const fixed = backupAndFixClaudeSettings(session.installDir); - if (fixed) { - logToFile('[agent-runner] auto-neutralized writable settings conflict'); - analytics.wizardCapture('settings conflict auto-neutralized', { - keys: autoFix.flatMap((c) => c.keys), - }); - } else { - // Couldn't remove it — don't run into the redirect; fail closed instead. - logToFile( - '[agent-runner] could not back up writable settings conflict — failing closed', - ); - unfixable = [...failClosed, ...autoFix]; - } - } + const fixed = backupAndFixClaudeSettings(session.installDir); + const outcome = planAutoFixOutcome(plan, fixed); + for (const action of outcome.actions) runPlannerAction(action); + unfixable = outcome.unfixable; + } - // What we cannot neutralize (org-managed, always read by the SDK; or a - // writable file we failed to back up) must be fixed by the user. Fail - // closed: the screen names the file + keys and exits. - if (unfixable.length > 0) { - await getUI().showSettingsOverride(unfixable, () => - backupAndFixClaudeSettings(session.installDir), - ); - logToFile('[agent-runner] settings override resolved'); - } + // What we cannot neutralize (org-managed, always read by the SDK; or a + // writable file we failed to back up) must be fixed by the user. Fail + // closed: the screen names the file + keys and exits. + if (unfixable.length > 0) { + await getUI().showSettingsOverride(unfixable, () => + backupAndFixClaudeSettings(session.installDir), + ); + logToFile('[agent-runner] settings override resolved'); } analytics.wizardCapture('agent started', { diff --git a/src/lib/agent/settings-conflict-planner.ts b/src/lib/agent/settings-conflict-planner.ts new file mode 100644 index 00000000..8de92b71 --- /dev/null +++ b/src/lib/agent/settings-conflict-planner.ts @@ -0,0 +1,169 @@ +/** + * Pure planner for the settings-conflict step of the runner bootstrap. + * + * Takes the conflicts reported by {@link checkAllSettingsConflicts} and + * returns the log lines, analytics events, and decisions the executor must + * perform — without performing any of them. The runner's + * `bootstrapProgram` is the imperative shell that runs side effects against + * this plan; everything routing-related (managed→org analytics, warn-only + * neutralization, fail-closed selection, auto-fix scheduling) is decided here + * and unit-tested in isolation. + * + * Issue: https://github.com/PostHog/wizard/issues/756 + * + * Behavior parity contract: the produced action sequence is the exact set of + * `logToFile` / `analytics.wizardCapture` calls (in order) the inline + * orchestration used to make, plus the auto-fix decision and the final + * `unfixable` list. Changing this changes wizard analytics — keep it + * behavior-preserving unless the issue asks otherwise. + */ + +import { + classifySettingsConflicts, + type SettingsConflict, +} from './claude-settings'; + +/** A single side-effect the executor must perform. */ +export type PlannerAction = + | { kind: 'log'; message: string } + | { + kind: 'analytics'; + event: string; + props: Record; + }; + +/** + * Output of {@link planSettingsConflictActions}. The executor runs `pre` + * unconditionally, then — if `autoFix` is non-null — performs the imperative + * `backupAndFixClaudeSettings` call and feeds the boolean result back through + * {@link planAutoFixOutcome} to obtain the trailing actions + final unfixable + * list. `failClosed` and `autoFixCandidates` are exposed for the outcome + * helper; the executor does not need to read them directly. + */ +export interface SettingsConflictPlan { + pre: PlannerAction[]; + autoFix: { keys: string[] } | null; + failClosed: SettingsConflict[]; + autoFixCandidates: SettingsConflict[]; +} + +/** Result of merging the auto-fix outcome back into the plan. */ +export interface AutoFixOutcome { + actions: PlannerAction[]; + unfixable: SettingsConflict[]; +} + +function describeConflict(c: SettingsConflict): string { + return `${c.source}(${c.keys.join(',')})`; +} + +function analyticsLevelFor(source: SettingsConflict['source']): string { + // Managed settings are surfaced to analytics as "org" — they are deployed by + // IT/MDM, and "org" reads more naturally than "managed" in dashboards. + return source === 'managed' ? 'org' : source; +} + +/** + * Build the pure plan for the conflicts reported during bootstrap. Pure: no + * file I/O, no analytics, no logging. Safe to call in tests without mocks. + */ +export function planSettingsConflictActions( + conflicts: SettingsConflict[], +): SettingsConflictPlan { + const summary = + conflicts.length > 0 ? conflicts.map(describeConflict).join('; ') : 'none'; + const pre: PlannerAction[] = [ + { kind: 'log', message: `[agent-runner] settings conflicts: ${summary}` }, + ]; + + if (conflicts.length === 0) { + return { + pre, + autoFix: null, + failClosed: [], + autoFixCandidates: [], + }; + } + + for (const conflict of conflicts) { + pre.push({ + kind: 'analytics', + event: 'settings conflict detected', + props: { + level: analyticsLevelFor(conflict.source), + keys: conflict.keys, + }, + }); + } + + const { autoFix, failClosed, warnOnly } = + classifySettingsConflicts(conflicts); + + for (const conflict of warnOnly) { + pre.push({ + kind: 'log', + message: + `[agent-runner] settings conflict in ${conflict.source} (${conflict.path}) ` + + `neutralized by settingSources:['project'] — not blocking`, + }); + pre.push({ + kind: 'analytics', + event: 'settings conflict neutralized', + props: { level: conflict.source, keys: conflict.keys }, + }); + } + + return { + pre, + autoFix: + autoFix.length > 0 ? { keys: autoFix.flatMap((c) => c.keys) } : null, + failClosed, + autoFixCandidates: autoFix, + }; +} + +/** + * Merge the boolean result of `backupAndFixClaudeSettings` back into the plan. + * On success the writable files are gone and only `failClosed` remains + * unfixable; on failure the auto-fix candidates fall back to unfixable so the + * settings-override screen names every file the user must still act on. + * + * When the plan never scheduled auto-fix, returns an empty action list and the + * fail-closed conflicts verbatim — the `didFix` argument is then a no-op. + */ +export function planAutoFixOutcome( + plan: SettingsConflictPlan, + didFix: boolean, +): AutoFixOutcome { + if (plan.autoFix === null) { + return { actions: [], unfixable: plan.failClosed }; + } + + if (didFix) { + return { + actions: [ + { + kind: 'log', + message: '[agent-runner] auto-neutralized writable settings conflict', + }, + { + kind: 'analytics', + event: 'settings conflict auto-neutralized', + props: { keys: plan.autoFix.keys }, + }, + ], + unfixable: plan.failClosed, + }; + } + + return { + actions: [ + { + kind: 'log', + message: + '[agent-runner] could not back up writable settings conflict — failing closed', + }, + ], + unfixable: [...plan.failClosed, ...plan.autoFixCandidates], + }; +}