Skip to content

Commit 758590c

Browse files
committed
feat: add an advisor that reviews each turn with a second model
After each completed main-agent turn, an opt-in advisor runs the conversation past a second model (the advisor model role or an explicit override) and buffers its notes; they are injected as an <advisory> block when the next turn starts, never launching a turn on their own. The advisor runs only when its model shares the session model's provider, and disables itself after three consecutive failures.
1 parent 62123eb commit 758590c

9 files changed

Lines changed: 543 additions & 0 deletions

File tree

.changeset/advisor-runtime.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@pythoughts/pythinker-code": minor
3+
---
4+
5+
Add an opt-in advisor: a second model reviews the conversation after each completed turn and its notes appear as an `<advisory>` block in the agent's next turn; enable with `[advisor] enabled = true` plus an advisor model (the `advisor` model role or `[advisor] model`), and it runs only when the advisor shares the session model's provider.

docs/configuration/config-files.md

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,7 @@ Fields in the config file fall into two categories: **top-level scalars** that d
7777
| --- | --- | --- | --- |
7878
| `default_model` | `string` || Default model alias; must be defined in `models` |
7979
| `model_roles` | `table` || Model role assignments → [`model_roles`](#model_roles) |
80+
| `advisor` | `table` || Second-opinion reviewer → [`advisor`](#advisor) |
8081
| `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 |
8182
| `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) |
8283
| `default_plan_mode` | `boolean` | `false` | Whether new sessions start in Plan mode (produce a plan before executing) by default |
@@ -174,6 +175,26 @@ Roles take effect in two places:
174175

175176
Inside the TUI, `/model <role>` assigns a role from the model picker, `/model <role> clear` removes it, and `/model roles` lists the current assignments. See [Slash commands](../reference/slash-commands.md).
176177

178+
## `advisor`
179+
180+
`advisor` enables a second-opinion reviewer: after each completed turn, a second model reviews the conversation and returns notes, which appear in the agent's context as an `<advisory>` block at the start of its next turn. The advisor never interrupts or slows a running turn.
181+
182+
| Field | Type | Default | Description |
183+
| --- | --- | --- | --- |
184+
| `enabled` | `boolean` | `false` | Turn the advisor on. It also needs a model: set `model` here or lock one to the `advisor` role |
185+
| `model` | `string` || Model alias for the advisor; when unset, the `advisor` entry in `model_roles` is used |
186+
| `instructions` | `string` || Extra instructions appended to the advisor's system prompt |
187+
188+
The advisor sends the session conversation to the advisor model. As a safety default, it runs only when the advisor model uses the same provider entry as the session model; a cross-provider advisor stays inactive and logs one warning.
189+
190+
```toml
191+
[advisor]
192+
enabled = true
193+
194+
[model_roles]
195+
advisor = "reviewer-model"
196+
```
197+
177198
## `thinking`
178199

179200
`thinking` sets the global default behavior for Thinking mode. `mode = "off"` forces Thinking off even when the top-level `default_thinking = true`.

packages/agent-core/src/agent/index.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,7 @@ export interface AgentOptions {
119119
readonly lsp?: LspManager;
120120
readonly additionalDirs?: readonly string[];
121121
readonly fileCheckpoints?: SessionFileCheckpointStore;
122+
readonly onEvent?: (event: AgentEvent) => void;
122123
}
123124

124125
export class Agent {
@@ -150,6 +151,7 @@ export class Agent {
150151
readonly worktree?: SessionWorktree;
151152
readonly lsp?: LspManager;
152153
private readonly fileCheckpoints?: SessionFileCheckpointStore;
154+
private readonly onEvent?: (event: AgentEvent) => void;
153155
private currentFileCheckpointId?: string;
154156

155157
readonly llmRequestLogger: LlmRequestLogger;
@@ -195,6 +197,7 @@ export class Agent {
195197
this.worktree = options.worktree;
196198
this.lsp = options.lsp;
197199
this.fileCheckpoints = options.fileCheckpoints;
200+
this.onEvent = options.onEvent;
198201

199202
this.llmRequestLogger = new LlmRequestLogger(this.log);
200203
this.blobStore = options.homedir
@@ -570,6 +573,7 @@ export class Agent {
570573

571574
emitEvent(event: AgentEvent): void {
572575
if (this.records.restoring) return;
576+
this.onEvent?.(event);
573577
void this.rpc?.emitEvent?.(event);
574578
}
575579

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,14 @@ export const ThinkingConfigSchema = z.object({
5757

5858
export type ThinkingConfig = z.infer<typeof ThinkingConfigSchema>;
5959

60+
export const AdvisorConfigSchema = z.object({
61+
enabled: z.boolean().optional(),
62+
model: z.string().optional(),
63+
instructions: z.string().optional(),
64+
});
65+
66+
export type AdvisorConfig = z.infer<typeof AdvisorConfigSchema>;
67+
6068
export const PermissionModeSchema = z.enum(['yolo', 'manual', 'auto']);
6169

6270
export const WorkflowSizeGuidelineSchema = z.enum(['small', 'medium', 'large', 'unrestricted']);
@@ -283,6 +291,7 @@ export const PythinkerConfigSchema = z.object({
283291
outputStyle: z.string().trim().min(1).optional(),
284292
models: z.record(z.string(), ModelAliasSchema).optional(),
285293
thinking: ThinkingConfigSchema.optional(),
294+
advisor: AdvisorConfigSchema.optional(),
286295
planMode: z.boolean().optional(),
287296
yolo: z.boolean().optional(),
288297
defaultThinking: z.boolean().optional(),
@@ -310,6 +319,7 @@ export type PythinkerConfig = z.infer<typeof PythinkerConfigSchema>;
310319
const ProviderConfigPatchSchema = ProviderConfigFieldsSchema.partial();
311320
const ModelAliasPatchSchema = ModelAliasSchema.partial();
312321
const ThinkingConfigPatchSchema = ThinkingConfigSchema.partial();
322+
const AdvisorConfigPatchSchema = AdvisorConfigSchema.partial();
313323
const PermissionConfigPatchSchema = PermissionConfigSchema.partial();
314324
const LoopControlPatchSchema = LoopControlSchema.partial();
315325
const BackgroundConfigPatchSchema = BackgroundConfigSchema.partial();
@@ -329,6 +339,7 @@ export const PythinkerConfigPatchSchema = z
329339
outputStyle: z.string().trim().min(1).optional(),
330340
models: z.record(z.string(), ModelAliasPatchSchema).optional(),
331341
thinking: ThinkingConfigPatchSchema.optional(),
342+
advisor: AdvisorConfigPatchSchema.optional(),
332343
planMode: z.boolean().optional(),
333344
yolo: z.boolean().optional(),
334345
defaultThinking: z.boolean().optional(),

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -303,6 +303,8 @@ export function transformTomlData(data: Record<string, unknown>): Record<string,
303303
result[targetKey] = transformRecord(value, transformModelData);
304304
} else if (targetKey === 'thinking' && isPlainObject(value)) {
305305
result[targetKey] = transformPlainObject(value);
306+
} else if (targetKey === 'advisor' && isPlainObject(value)) {
307+
result[targetKey] = transformPlainObject(value);
306308
} else if (targetKey === 'permission' && isPlainObject(value)) {
307309
result[targetKey] = transformPermissionData(value);
308310
} else if (targetKey === 'hooks' && Array.isArray(value)) {

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ import {
6363
} from '../skill';
6464
import { noopTelemetryClient, type TelemetryClient } from '../telemetry';
6565
import { SessionSubagentHost } from './subagent-host';
66+
import { SessionAdvisor } from './session-advisor';
6667
import type { ToolServices } from '../tools/support/services';
6768
import { FlagResolver, type ExperimentalFlagResolver } from '../flags';
6869
import { abortError } from '../utils/abort';
@@ -366,6 +367,7 @@ export class Session {
366367
readonly worktree: SessionWorktree;
367368
readonly lsp: LspManager;
368369
readonly fileCheckpoints: SessionFileCheckpointStore | undefined;
370+
readonly advisor: SessionAdvisor;
369371
private fileChangedWatcher?: FSWatcher;
370372
private readonly fileChangedWatcherReady: Promise<void>;
371373
private fileChangedWatchCwd: string;
@@ -399,6 +401,7 @@ export class Session {
399401
this.log =
400402
this.logHandle?.logger ??
401403
(options.id === undefined ? log : log.createChild({ sessionId: options.id }));
404+
this.advisor = new SessionAdvisor(this);
402405
this.rpc = options.rpc;
403406
this.experimentalFlags = options.experimentalFlags ?? new FlagResolver();
404407
this.agentProfiles = {
@@ -1329,6 +1332,16 @@ export class Session {
13291332
lsp: this.lsp,
13301333
additionalDirs: this.listWorkspaceDirectories().map((entry) => entry.path),
13311334
fileCheckpoints: this.fileCheckpoints,
1335+
onEvent:
1336+
id === 'main'
1337+
? (event) => {
1338+
if (event.type === 'turn.started') {
1339+
this.advisor.onMainTurnStarted(event.origin);
1340+
} else if (event.type === 'turn.ended' && event.reason === 'completed') {
1341+
this.advisor.onMainTurnEnded();
1342+
}
1343+
}
1344+
: undefined,
13321345
});
13331346
agent.setFileCheckpointId(parentAgent?.fileCheckpointId);
13341347
return agent;
Lines changed: 181 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,181 @@
1+
import type { PromptOrigin } from '../agent/context';
2+
import { InMemoryAgentRecordPersistence } from '../agent/records';
3+
import { resolveModelRoleAlias } from '../config/model-roles';
4+
import { HookEngine } from './hooks';
5+
import type { Session } from '.';
6+
7+
const ADVISOR_SYSTEM_PROMPT =
8+
"You are a quiet second-opinion reviewer watching another agent's coding session. Point out real risks, mistakes, and better options. Do not repeat what went well. Return your notes with StructuredOutput; return an empty notes array when you have nothing important.";
9+
const ADVISOR_USER_PROMPT = 'Review the conversation so far and return your advisory notes.';
10+
const ADVISOR_OUTPUT_SCHEMA = {
11+
type: 'object',
12+
required: ['notes'],
13+
properties: {
14+
notes: {
15+
type: 'array',
16+
items: {
17+
type: 'object',
18+
required: ['note'],
19+
properties: {
20+
note: { type: 'string' },
21+
severity: { enum: ['nit', 'concern', 'blocker'] },
22+
},
23+
},
24+
},
25+
},
26+
} as const;
27+
28+
interface AdvisoryNote {
29+
readonly note: string;
30+
readonly severity?: 'nit' | 'concern' | 'blocker';
31+
}
32+
33+
export class SessionAdvisor {
34+
#running = false;
35+
#disabled = false;
36+
#warnedCrossProvider = false;
37+
#consecutiveFailures = 0;
38+
#reviewCurrentTurn = false;
39+
#pendingAdvisory: string | undefined;
40+
41+
constructor(private readonly session: Session) {}
42+
43+
/** Called when a main-agent turn starts. Delivers notes without starting a new turn. */
44+
onMainTurnStarted(origin: PromptOrigin): void {
45+
this.#reviewCurrentTurn = origin.kind === 'user' || origin.kind === 'system_trigger';
46+
queueMicrotask(() => this.#deliverPending());
47+
}
48+
49+
/** Called after each completed main-agent turn. Never throws; never blocks the caller. */
50+
onMainTurnEnded(): void {
51+
const shouldReview = this.#reviewCurrentTurn;
52+
this.#reviewCurrentTurn = false;
53+
if (!shouldReview) return;
54+
if (this.#running || this.#disabled) return;
55+
this.#running = true;
56+
void this.#run()
57+
.catch((error: unknown) => this.#recordFailure(error))
58+
.finally(() => {
59+
this.#running = false;
60+
});
61+
}
62+
63+
async #run(): Promise<void> {
64+
const config = this.session.options.config;
65+
if (config?.advisor?.enabled !== true) return;
66+
67+
const main = this.session.getReadyAgent('main');
68+
if (main === undefined) return;
69+
const advisorAlias = config.advisor.model ?? resolveModelRoleAlias(config, 'advisor');
70+
if (!main.config.canResolveModel(advisorAlias) || advisorAlias === undefined) return;
71+
72+
const mainAlias = main.config.modelAlias;
73+
if (mainAlias === undefined) return;
74+
const advisorProvider = config.models?.[advisorAlias]?.provider ?? config.defaultProvider;
75+
const mainProvider = config.models?.[mainAlias]?.provider ?? config.defaultProvider;
76+
if (advisorProvider !== mainProvider) {
77+
if (!this.#warnedCrossProvider) {
78+
this.#warnedCrossProvider = true;
79+
this.session.log.warn('advisor skipped because its provider differs from the main model', {
80+
advisorProvider,
81+
mainProvider,
82+
});
83+
}
84+
return;
85+
}
86+
87+
let id: string | undefined;
88+
try {
89+
const created = await this.session.createAgent(
90+
{
91+
type: 'sub',
92+
generate: main.rawGenerate,
93+
persistence: new InMemoryAgentRecordPersistence(),
94+
hookEngine: new HookEngine(),
95+
},
96+
{ parentAgentId: main.agentId, persistMetadata: false },
97+
);
98+
id = created.id;
99+
const child = created.agent;
100+
child.config.update({
101+
modelAlias: advisorAlias,
102+
thinkingLevel: 'off',
103+
systemPrompt:
104+
ADVISOR_SYSTEM_PROMPT +
105+
(config.advisor.instructions === undefined
106+
? ''
107+
: `\n\n${config.advisor.instructions}`),
108+
});
109+
child.tools.setActiveTools([]);
110+
child.context.useProjectedHistoryFrom(main.context);
111+
const turnId = child.turn.prompt(
112+
[{ type: 'text', text: ADVISOR_USER_PROMPT }],
113+
{ kind: 'system_trigger', name: 'advisor' },
114+
ADVISOR_OUTPUT_SCHEMA,
115+
);
116+
if (turnId === null) throw new Error('Advisor turn could not start.');
117+
const result = await child.turn.waitForCurrentTurn(AbortSignal.timeout(120_000));
118+
if (result.event.reason !== 'completed') {
119+
throw new Error('Advisor turn did not complete.');
120+
}
121+
const notes = parseNotes(result.event.structuredOutput);
122+
this.#consecutiveFailures = 0;
123+
if (notes.length === 0) return;
124+
125+
const lines = notes.map(({ note, severity }) =>
126+
severity === undefined ? `- ${note}` : `- [${severity}] ${note}`,
127+
);
128+
const block = [
129+
'<advisory>',
130+
'The following notes are from a second reviewing model. Weigh them; do not blindly obey.',
131+
...lines,
132+
'</advisory>',
133+
].join('\n');
134+
this.#pendingAdvisory = block;
135+
this.#deliverPending();
136+
} finally {
137+
if (id !== undefined) this.session.agents.delete(id);
138+
}
139+
}
140+
141+
#recordFailure(error: unknown): void {
142+
this.#consecutiveFailures += 1;
143+
this.session.log.debug('advisor run failed', { error });
144+
if (this.#consecutiveFailures < 3) return;
145+
this.#disabled = true;
146+
this.session.log.warn('advisor disabled after three consecutive failures');
147+
}
148+
149+
#deliverPending(): void {
150+
const main = this.session.getReadyAgent('main');
151+
if (this.#pendingAdvisory === undefined || main?.turn.hasActiveTurn !== true) return;
152+
const block = this.#pendingAdvisory;
153+
this.#pendingAdvisory = undefined;
154+
main.turn.steer([{ type: 'text', text: block }], {
155+
kind: 'hook_result',
156+
event: 'advisor',
157+
});
158+
}
159+
}
160+
161+
function parseNotes(output: unknown): AdvisoryNote[] {
162+
if (typeof output !== 'object' || output === null || !Array.isArray((output as { notes?: unknown }).notes)) {
163+
throw new Error('Advisor did not return structured notes.');
164+
}
165+
return (output as { notes: unknown[] }).notes.map((value) => {
166+
if (typeof value !== 'object' || value === null) {
167+
throw new Error('Advisor returned an invalid note.');
168+
}
169+
const { note, severity } = value as { note?: unknown; severity?: unknown };
170+
if (typeof note !== 'string') throw new Error('Advisor returned an invalid note.');
171+
if (
172+
severity !== undefined &&
173+
severity !== 'nit' &&
174+
severity !== 'concern' &&
175+
severity !== 'blocker'
176+
) {
177+
throw new Error('Advisor returned an invalid severity.');
178+
}
179+
return { note, severity } as AdvisoryNote;
180+
});
181+
}

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,23 @@ source = { kind = "apiJson", url = "https://registry.example/api.json", apiKey =
241241
expect(readConfigFile(configPath).modelRoles).toEqual({ small: 'x' });
242242
});
243243

244+
it('round-trips advisor config', async () => {
245+
const configPath = join(makeTempDir(), 'advisor.toml');
246+
const config = parseConfigString(
247+
'[advisor]\nenabled = true\nmodel = "reviewer"\ninstructions = "Check risks."\n',
248+
configPath,
249+
);
250+
251+
expect(config.advisor).toEqual({
252+
enabled: true,
253+
model: 'reviewer',
254+
instructions: 'Check risks.',
255+
});
256+
257+
await writeConfigFile(configPath, config);
258+
expect(readConfigFile(configPath).advisor).toEqual(config.advisor);
259+
});
260+
244261
it('round-trips an API key environment reference without an API key', async () => {
245262
const configPath = join(makeTempDir(), 'api-key-env-var.toml');
246263
const config = parseConfigString(
@@ -609,6 +626,22 @@ describe('harness config schema and patch merge', () => {
609626
expect(merged.modelRoles).toEqual({ small: 'y', advisor: 'z' });
610627
});
611628

629+
it('deep-merges advisor patches', () => {
630+
const merged = mergeConfigPatch(
631+
{
632+
providers: {},
633+
advisor: { enabled: true, model: 'reviewer', instructions: 'Check risks.' },
634+
},
635+
{ advisor: { instructions: 'Check correctness.' } },
636+
);
637+
638+
expect(merged.advisor).toEqual({
639+
enabled: true,
640+
model: 'reviewer',
641+
instructions: 'Check correctness.',
642+
});
643+
});
644+
612645
it('deep-merges experimental config patches', () => {
613646
const base = parseConfigString(`
614647
[experimental]

0 commit comments

Comments
 (0)