Skip to content

Commit 32c3701

Browse files
committed
fix: deliver advisory notes only at a turn boundary
Delivering as soon as a review finished could steer notes about an earlier turn into a turn already under way, redirecting work in progress. Notes now arrive only at the start of a turn, so a review that finishes mid-turn waits for the following one.
1 parent bc9f0bc commit 32c3701

3 files changed

Lines changed: 59 additions & 2 deletions

File tree

docs/configuration/config-files.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,7 @@ Inside the TUI, `/model <role>` assigns a role from the model picker, `/model <r
177177

178178
## `advisor`
179179

180-
`advisor` enables a second-opinion reviewer: after a completed user turn, a second model reviews the conversation and returns notes. Notes are delivered into the next turn, at its start when the review has already finished or as soon as the review completes, which may be after that turn is under way.
180+
`advisor` enables a second-opinion reviewer: after a completed user turn, a second model reviews the conversation and returns notes. Notes are delivered at the start of the next turn after the review finishes, so a review may lag a turn.
181181

182182
| Field | Type | Default | Description |
183183
| --- | --- | --- | --- |

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,6 @@ export class SessionAdvisor {
143143
'</advisory>',
144144
].join('\n');
145145
this.#pendingAdvisory = block;
146-
this.#deliverPending();
147146
} finally {
148147
if (id !== undefined) this.session.agents.delete(id);
149148
}

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

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,64 @@ describe('SessionAdvisor', () => {
7272
);
7373
});
7474

75+
it('waits until the next turn when a review finishes mid-turn', async () => {
76+
const fixture = await createFixture({ advisorAlias: 'advisor' });
77+
const reviewGate = createDeferred<void>();
78+
const activeTurnGate = createDeferred<void>();
79+
const generate = fixture.main.rawGenerate;
80+
let generateCall = 0;
81+
vi.spyOn(fixture.main, 'rawGenerate').mockImplementation(async (...args) => {
82+
generateCall += 1;
83+
const currentCall = generateCall;
84+
const result = await generate(...args);
85+
if (currentCall === 2) await reviewGate.promise;
86+
if (currentCall === 3) await activeTurnGate.promise;
87+
return result;
88+
});
89+
const steer = vi.spyOn(fixture.main.turn, 'steer').mockReturnValue(null);
90+
queueReview(fixture.scripted, 'Check the active turn.', 'concern');
91+
92+
await runMainTurn(fixture.main, { kind: 'user' });
93+
94+
queueReview(fixture.scripted);
95+
const turnId = fixture.main.turn.prompt(
96+
[{ type: 'text', text: 'Continue.' }],
97+
{ kind: 'user' },
98+
);
99+
expect(turnId).not.toBeNull();
100+
const activeTurn = fixture.main.turn.waitForCurrentTurn();
101+
await vi.waitFor(() => {
102+
expect(fixture.scripted.calls).toHaveLength(3);
103+
expect(fixture.main.turn.hasActiveTurn).toBe(true);
104+
});
105+
106+
reviewGate.resolve();
107+
await waitForAdvisor(fixture);
108+
const callsWhileActive = steer.mock.calls.length;
109+
110+
activeTurnGate.resolve();
111+
await activeTurn;
112+
await vi.waitFor(() => {
113+
expect(fixture.scripted.calls).toHaveLength(4);
114+
expect(fixture.session.agents.size).toBe(1);
115+
});
116+
117+
fixture.scripted.mockNextResponse({ type: 'text', text: 'Following turn.' });
118+
await runMainTurn(fixture.main, { kind: 'system_trigger', name: 'follow-up' });
119+
120+
expect(callsWhileActive).toBe(0);
121+
expect(steer).toHaveBeenCalledOnce();
122+
expect(steer).toHaveBeenCalledWith(
123+
[
124+
{
125+
type: 'text',
126+
text: expect.stringContaining('- [concern] Check the active turn.'),
127+
},
128+
],
129+
{ kind: 'hook_result', event: 'advisor' },
130+
);
131+
});
132+
75133
it('contains errors from delivering notes at turn start', async () => {
76134
const fixture = await createFixture({ advisorAlias: 'advisor' });
77135
const error = new Error('steer failed');

0 commit comments

Comments
 (0)