fix: persist advisor config, review only user turns, expand advisor role refs

Author: elkaix

docs/configuration/config-files.md

@@ -76,8 +76,6 @@ Fields in the config file fall into two categories: **top-level scalars** that d
 | Field | Type | Default | Description |
 | --- | --- | --- | --- |
 | `default_model` | `string` | — | Default model alias; must be defined in `models` |
-| `model_roles` | `table` | — | Model role assignments → [`model_roles`](#model_roles) |
-| `advisor` | `table` | — | Second-opinion reviewer → [`advisor`](#advisor) |
 | `default_thinking` | `boolean` | `false` | Whether new sessions enable Thinking (deep reasoning) mode by default; can be toggled from the model menu inside a session. Even when set to `true`, `[thinking].mode = "off"` will still force Thinking off |
 | `default_permission_mode` | `string` | `manual` | Default permission mode for new sessions; one of `manual` (prompt each time), `yolo` (auto-approve tool actions, but the agent may still ask questions), or `auto` (fully autonomous — the agent decides everything without asking, except a `DynamicWorkflow` call, which still shows its plan for approval) |
 | `default_plan_mode` | `boolean` | `false` | Whether new sessions start in Plan mode (produce a plan before executing) by default |
@@ -88,6 +86,8 @@ Fields in the config file fall into two categories: **top-level scalars** that d
 | `workflow_size_guideline` | `string` | `medium` | Advisory subagent-count target for one Dynamic Workflow; one of `small` (about 5), `medium` (about 15), `large` (about 40), or `unrestricted` (no target). Exceeding it emits a warning rather than blocking the run; the `PYTHINKER_CODE_WORKFLOW_SIZE_GUIDELINE` environment variable overrides it |
 | `providers` | `table` | `{}` | API provider table → [`providers`](#providers) |
 | `models` | `table` | — | Model alias table → [`models`](#models) |
+| `model_roles` | `table` | — | Model role assignments → [`model_roles`](#model_roles) |
+| `advisor` | `table` | — | Second-opinion reviewer → [`advisor`](#advisor) |
 | `thinking` | `table` | — | Default parameters for Thinking mode → [`thinking`](#thinking) |
 | `loop_control` | `table` | — | Agent loop control parameters → [`loop_control`](#loop_control) |
 | `background` | `table` | — | Background task runtime parameters → [`background`](#background) |
@@ -96,7 +96,7 @@ Fields in the config file fall into two categories: **top-level scalars** that d
 | `permission` | `table` | — | Initial permission rules → [`permission`](#permission) |
 | `hooks` | `array<table>` | — | Lifecycle hooks; see [Hooks](../customization/hooks.md) |
 
-The following sections cover each of the nested tables in turn: `providers`, `models`, `model_roles`, `thinking`, `loop_control`, `background`, `experimental`, `services`, and `permission`.
+The following sections cover each of the nested tables in turn: `providers`, `models`, `model_roles`, `advisor`, `thinking`, `loop_control`, `background`, `experimental`, `services`, and `permission`.
 
 ## `providers`
 

packages/agent-core/src/config/toml.ts

@@ -8,6 +8,7 @@ import {
   PythinkerConfigSchema,
   formatConfigValidationError,
   getDefaultConfig,
+  type AdvisorConfig,
   type BackgroundConfig,
   type ExperimentalConfig,
   type HookDefConfig,
@@ -500,6 +501,7 @@ export function configToTomlData(config: PythinkerConfig): Record<string, unknow
     out['model_roles'] = cloneUnknown(config.modelRoles);
   }
   setSection(out, 'thinking', config.thinking, thinkingToToml);
+  setSection(out, 'advisor', config.advisor, advisorToToml);
   setSection(out, 'services', config.services, servicesToToml);
   setSection(out, 'loop_control', config.loopControl, loopControlToToml);
   setSection(out, 'background', config.background, backgroundToToml);
@@ -585,6 +587,14 @@ function thinkingToToml(thinking: ThinkingConfig, rawThinking: unknown): Record<
   return out;
 }
 
+function advisorToToml(advisor: AdvisorConfig, rawAdvisor: unknown): Record<string, unknown> {
+  const out = cloneRecord(rawAdvisor);
+  for (const [key, value] of Object.entries(advisor)) {
+    setDefined(out, camelToSnake(key), value);
+  }
+  return out;
+}
+
 function permissionToToml(
   permission: PermissionConfig,
   rawPermission: unknown,

packages/agent-core/src/session/session-advisor.ts

@@ -1,6 +1,6 @@
 import type { PromptOrigin } from '../agent/context';
 import { InMemoryAgentRecordPersistence } from '../agent/records';
-import { resolveModelRoleAlias } from '../config/model-roles';
+import { expandModelRef, resolveModelRoleAlias } from '../config/model-roles';
 import { HookEngine } from './hooks';
 import type { Session } from '.';
 
@@ -42,7 +42,8 @@ export class SessionAdvisor {
 
   /** Called when a main-agent turn starts. Delivers notes without starting a new turn. */
   onMainTurnStarted(origin: PromptOrigin): void {
-    this.#reviewCurrentTurn = origin.kind === 'user' || origin.kind === 'system_trigger';
+    // Autonomous turns must not compound advisor cost.
+    this.#reviewCurrentTurn = origin.kind === 'user';
     queueMicrotask(() => this.#deliverPending());
   }
 
@@ -66,7 +67,10 @@ export class SessionAdvisor {
 
     const main = this.session.getReadyAgent('main');
     if (main === undefined) return;
-    const advisorAlias = config.advisor.model ?? resolveModelRoleAlias(config, 'advisor');
+    const advisorAlias =
+      config.advisor.model === undefined
+        ? resolveModelRoleAlias(config, 'advisor')
+        : expandModelRef(config, config.advisor.model);
     if (!main.config.canResolveModel(advisorAlias) || advisorAlias === undefined) return;
 
     const mainAlias = main.config.modelAlias;

packages/agent-core/test/config/configs.test.ts

@@ -7,6 +7,7 @@ import { afterEach, describe, expect, it } from 'vitest';
 
 import { ErrorCodes, PythinkerError } from '../../src/errors';
 import {
+  type PythinkerConfig,
   PythinkerConfigSchema,
   ensureConfigFile,
   loadRuntimeConfig,
@@ -272,20 +273,16 @@ source = { kind = "apiJson", url = "https://registry.example/api.json", apiKey =
     expect(readConfigFile(configPath).modelRoles).toBeUndefined();
   });
 
-  it('round-trips advisor config', async () => {
+  it('writes typed advisor config over stale raw data', async () => {
     const configPath = join(makeTempDir(), 'advisor.toml');
-    const config = parseConfigString(
-      '[advisor]\nenabled = true\nmodel = "reviewer"\ninstructions = "Check risks."\n',
-      configPath,
-    );
-
-    expect(config.advisor).toEqual({
-      enabled: true,
-      model: 'reviewer',
-      instructions: 'Check risks.',
-    });
+    const config: PythinkerConfig = {
+      providers: {},
+      advisor: { enabled: true, model: 'reviewer' },
+      raw: { advisor: { enabled: false, model: 'stale-reviewer' } },
+    };
 
     await writeConfigFile(configPath, config);
+    expect(await readFile(configPath, 'utf-8')).toContain('[advisor]');
     expect(readConfigFile(configPath).advisor).toEqual(config.advisor);
   });
 

packages/agent-core/test/session/session-advisor.test.ts

@@ -6,6 +6,7 @@ import { testKaos } from '../fixtures/test-kaos';
 import { afterEach, describe, expect, it, vi } from 'vitest';
 
 import type { Agent } from '../../src/agent';
+import type { PromptOrigin } from '../../src/agent/context';
 import type { PythinkerConfig } from '../../src/config';
 import type { ResolvedAgentProfile } from '../../src/profile';
 import type { SDKSessionRPC } from '../../src/rpc';
@@ -40,7 +41,7 @@ describe('SessionAdvisor', () => {
     const steer = vi.spyOn(fixture.main.turn, 'steer').mockReturnValue(null);
     queueReview(fixture.scripted, 'Check the error path.', 'concern');
 
-    await runMainTurn(fixture.main);
+    await runMainTurn(fixture.main, { kind: 'user' });
     await waitForAdvisor(fixture);
 
     expect(spawn).toHaveBeenCalledOnce();
@@ -65,6 +66,30 @@ describe('SessionAdvisor', () => {
     await fixture.session.close();
   });
 
+  it('does not review system-trigger turns', async () => {
+    const fixture = await createFixture({ advisorAlias: 'advisor' });
+    const spawn = vi.spyOn(fixture.session, 'createAgent');
+
+    fixture.scripted.mockNextResponse({ type: 'text', text: 'Continued.' });
+    await runMainTurn(fixture.main, { kind: 'system_trigger', name: 'goal-continuation' });
+    await flushAsync();
+
+    expect(spawn).not.toHaveBeenCalled();
+    await fixture.session.close();
+  });
+
+  it('expands an explicit advisor role reference', async () => {
+    const fixture = await createFixture({ advisorAlias: 'reviewer', advisorModel: '@advisor' });
+    const spawn = vi.spyOn(fixture.session, 'createAgent');
+    queueReview(fixture.scripted);
+
+    await runMainTurn(fixture.main);
+    await waitForAdvisor(fixture);
+
+    expect((await spawn.mock.results[0]!.value).agent.config.modelAlias).toBe('reviewer');
+    await fixture.session.close();
+  });
+
   it('skips a cross-provider advisor and warns once', async () => {
     const fixture = await createFixture({ advisorAlias: 'cross-advisor' });
     const spawn = vi.spyOn(fixture.session, 'createAgent');
@@ -149,11 +174,87 @@ describe('SessionAdvisor', () => {
     expect(steer).not.toHaveBeenCalled();
     await fixture.session.close();
   });
+
+  it('contains advisor errors without affecting the main turn', async () => {
+    const fixture = await createFixture({ advisorAlias: 'advisor' });
+    const error = new Error('advisor failed');
+    vi.spyOn(fixture.session, 'createAgent').mockRejectedValueOnce(error);
+    const debug = vi.spyOn(fixture.session.log, 'debug');
+
+    fixture.scripted.mockNextResponse({ type: 'text', text: 'Done.' });
+    await expect(runMainTurn(fixture.main)).resolves.toBeUndefined();
+    await vi.waitFor(() =>
+      expect(debug).toHaveBeenCalledWith('advisor run failed', { error }),
+    );
+
+    expect(fixture.main.turn.hasActiveTurn).toBe(false);
+    await fixture.session.close();
+  });
+
+  it('disables the advisor after three consecutive failures', async () => {
+    const fixture = await createFixture({ advisorAlias: 'advisor' });
+    const spawn = vi
+      .spyOn(fixture.session, 'createAgent')
+      .mockRejectedValue(new Error('advisor failed'));
+    const warn = vi.spyOn(fixture.session.log, 'warn');
+
+    for (let turn = 0; turn < 3; turn += 1) {
+      fixture.scripted.mockNextResponse({ type: 'text', text: 'Done.' });
+      await runMainTurn(fixture.main);
+      await flushAsync();
+    }
+    await vi.waitFor(() =>
+      expect(warn).toHaveBeenCalledWith('advisor disabled after three consecutive failures'),
+    );
+
+    fixture.scripted.mockNextResponse({ type: 'text', text: 'Done.' });
+    await runMainTurn(fixture.main);
+    await flushAsync();
+
+    expect(spawn).toHaveBeenCalledTimes(3);
+    await fixture.session.close();
+  });
+
+  it('counts an aborted advisor wait as a failure', async () => {
+    const fixture = await createFixture({ advisorAlias: 'advisor' });
+    const timeoutError = new Error('advisor timed out');
+    const originalCreate = fixture.session.createAgent.bind(fixture.session);
+    const spawn = vi
+      .spyOn(fixture.session, 'createAgent')
+      .mockRejectedValueOnce(new Error('first failure'))
+      .mockRejectedValueOnce(new Error('second failure'))
+      .mockImplementationOnce((...args) => originalCreate(...args));
+    const timeout = vi
+      .spyOn(AbortSignal, 'timeout')
+      .mockReturnValue(AbortSignal.abort(timeoutError));
+    const warn = vi.spyOn(fixture.session.log, 'warn');
+
+    for (let turn = 0; turn < 2; turn += 1) {
+      fixture.scripted.mockNextResponse({ type: 'text', text: 'Done.' });
+      await runMainTurn(fixture.main);
+      await flushAsync();
+    }
+    queueReview(fixture.scripted);
+    await runMainTurn(fixture.main);
+    await vi.waitFor(() =>
+      expect(warn).toHaveBeenCalledWith('advisor disabled after three consecutive failures'),
+    );
+
+    fixture.scripted.mockNextResponse({ type: 'text', text: 'Done.' });
+    await runMainTurn(fixture.main);
+    await flushAsync();
+
+    expect(timeout).toHaveBeenCalledWith(120_000);
+    expect(spawn).toHaveBeenCalledTimes(3);
+    timeout.mockRestore();
+    await fixture.session.close();
+  });
 });
 
 interface FixtureOptions {
   readonly enabled?: boolean;
-  readonly advisorAlias?: 'advisor' | 'cross-advisor';
+  readonly advisorAlias?: 'advisor' | 'cross-advisor' | 'reviewer';
+  readonly advisorModel?: string;
 }
 
 async function createFixture(options: FixtureOptions = {}): Promise<{
@@ -193,18 +294,21 @@ function testConfig(options: FixtureOptions): PythinkerConfig {
     models: {
       main: { provider: 'primary', model: 'main', maxContextSize: 100_000 },
       advisor: { provider: 'primary', model: 'advisor', maxContextSize: 100_000 },
+      reviewer: { provider: 'primary', model: 'reviewer', maxContextSize: 100_000 },
       'cross-advisor': {
         provider: 'secondary',
         model: 'cross-advisor',
         maxContextSize: 100_000,
       },
     },
-    ...(options.advisorAlias === undefined
-      ? {}
-      : { modelRoles: { advisor: options.advisorAlias } }),
-    ...(options.enabled === true || options.advisorAlias !== undefined
-      ? { advisor: { enabled: true } }
-      : {}),
+    modelRoles:
+      options.advisorAlias === undefined ? undefined : { advisor: options.advisorAlias },
+    advisor:
+      options.enabled === true ||
+      options.advisorAlias !== undefined ||
+      options.advisorModel !== undefined
+        ? { enabled: true, model: options.advisorModel }
+        : undefined,
   };
 }
 
@@ -222,8 +326,8 @@ function queueReview(
   });
 }
 
-async function runMainTurn(main: Agent): Promise<void> {
-  const turnId = main.turn.prompt([{ type: 'text', text: 'Continue.' }]);
+async function runMainTurn(main: Agent, origin?: PromptOrigin): Promise<void> {
+  const turnId = main.turn.prompt([{ type: 'text', text: 'Continue.' }], origin);
   expect(turnId).not.toBeNull();
   await main.turn.waitForCurrentTurn();
 }