mirror of
https://github.com/THU-MAIC/OpenMAIC.git
synced 2026-10-02 09:24:43 +08:00
fix(pbl-v2): forward the resolved thinking config to the simulator narrator
Review finding from @cosarah on #1006, and a correct one: the director-narrator pass was the last site still dropping the resolved config. Both callers passed `thinkingConfig` into `runDirectorNarratorPass`, but the helper never destructured it and never handed it to `callLLM`, so the provider silently fell back to the model default instead of honouring the `pbl-v2-runtime:simulator` route. Earlier revisions of this PR documented that as pre-existing and deliberately untouched. That defence stopped holding once the hardcoded thinking policy was deleted: the PR's invariant became "every PBL v2 site resolves thinking through the shared entry point, and the deployment decides", and the narrator was the one site contradicting it. Coverage per the review: the simulator turn is now driven end to end against a scenario fixture, asserting that BOTH provider calls — the streamed character line and the non-streamed narrator pass — see the incoming config, that neither substitutes one when the request carries none, and that both are accounted for under their own usage sources. The `['generate','stream']` assertion keeps the test from silently covering only one of the two calls. Fault-injected to confirm it bites: dropping the new fourth argument again turns "forwards an incoming config to the character line AND the narrator" red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -262,7 +262,17 @@ async function runDirectorNarratorPass(args: {
|
||||
* warns against re-describing what the prior act already covered. */
|
||||
firstEntry?: boolean;
|
||||
}): Promise<string[]> {
|
||||
const { project, milestone, microtask, phase, thread, languageModel, signal, firstEntry } = args;
|
||||
const {
|
||||
project,
|
||||
milestone,
|
||||
microtask,
|
||||
phase,
|
||||
thread,
|
||||
languageModel,
|
||||
thinkingConfig,
|
||||
signal,
|
||||
firstEntry,
|
||||
} = args;
|
||||
const system = buildNarratorSystemPrompt(project, milestone, microtask);
|
||||
const history = buildSimulatorHistory(thread, 'director');
|
||||
const greetingNudge = firstEntry
|
||||
@@ -277,8 +287,6 @@ async function runDirectorNarratorPass(args: {
|
||||
const messages = [...history, { role: 'user' as const, content: nudge }];
|
||||
|
||||
try {
|
||||
// No thinking argument: the narrator pass has never resolved a thinking
|
||||
// config, and this PR does not start (its `thinkingConfig` arg is unused).
|
||||
const result = await callLLM(
|
||||
{
|
||||
model: languageModel,
|
||||
@@ -287,6 +295,12 @@ async function runDirectorNarratorPass(args: {
|
||||
...(signal ? { abortSignal: signal } : {}),
|
||||
},
|
||||
'pbl-v2-simulator-narrator',
|
||||
undefined,
|
||||
// Same contract as every other site: the request / stage route decides.
|
||||
// This pass used to drop the config on the floor — its callers passed one
|
||||
// and the helper never forwarded it, so the provider silently fell back to
|
||||
// the model default.
|
||||
thinkingConfig,
|
||||
);
|
||||
const text = (result.text ?? '').trim();
|
||||
if (!text) return [];
|
||||
|
||||
@@ -35,6 +35,7 @@ vi.mock('@/lib/server/usage-storage', () => ({
|
||||
|
||||
import { thinkingContext } from '@/lib/ai/thinking-context';
|
||||
import { runTaskEvaluation } from '@/lib/pbl/v2/agents/evaluator';
|
||||
import { runSimulatorTurn } from '@/lib/pbl/v2/agents/simulator';
|
||||
import { addSubmission } from '@/lib/pbl/v2/operations/submission';
|
||||
import type { PBLProjectV2 } from '@/lib/pbl/v2/types';
|
||||
import type { PBLSSEEvent } from '@/lib/pbl/v2/api/sse';
|
||||
@@ -99,6 +100,48 @@ function mkProject(): PBLProjectV2 {
|
||||
};
|
||||
}
|
||||
|
||||
/** SCENARIO fixture, trimmed from simulator.test.ts — enough for one roleplay
|
||||
* turn, which fires BOTH simulator provider calls (character line + narrator). */
|
||||
function scenarioProject(): PBLProjectV2 {
|
||||
return {
|
||||
language: 'zh-CN',
|
||||
roles: [{ id: 'role-i', type: 'instructor', name: '教练' }],
|
||||
threads: [{ agentId: 'role-i', messages: [] }],
|
||||
updatedAt: 'ts',
|
||||
scenario: {
|
||||
setting: '校园咖啡馆的午后',
|
||||
goal: '练习倾听与共情',
|
||||
learnerRole: '你是林夏的好朋友',
|
||||
characters: [{ id: 'c1', name: '林夏', persona: '内向', situation: '情绪低落' }],
|
||||
},
|
||||
milestones: [
|
||||
{
|
||||
id: 'ms-rp',
|
||||
title: '和林夏聊一聊',
|
||||
status: 'active',
|
||||
order: 0,
|
||||
documents: [],
|
||||
scenarioStage: 'roleplay',
|
||||
briefing: '你坐在咖啡馆里。',
|
||||
microtasks: [
|
||||
{
|
||||
id: 'beat-1',
|
||||
title: 'beat',
|
||||
status: 'in_progress',
|
||||
assignee: 'user',
|
||||
hints: [],
|
||||
order: 0,
|
||||
description: '林夏坐在你对面。',
|
||||
narration: '你们走进了一家安静的咖啡馆。',
|
||||
},
|
||||
],
|
||||
},
|
||||
],
|
||||
evaluations: [],
|
||||
engagementEvents: [],
|
||||
} as unknown as PBLProjectV2;
|
||||
}
|
||||
|
||||
const NOT_CAPTURED = Symbol('provider was never invoked');
|
||||
|
||||
/** Run one task evaluation against a scripted model, capturing what the
|
||||
@@ -185,3 +228,82 @@ describe('PBL v2 runtime goes through the shared LLM entry point (#1003)', () =>
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* The simulator turn makes TWO provider calls — the streamed character line and
|
||||
* the non-streamed director-narrator pass. The narrator was the last site still
|
||||
* dropping the resolved config: both callers passed one in and the helper never
|
||||
* forwarded it, so the provider silently fell back to the model default. Caught
|
||||
* in review on #1006.
|
||||
*/
|
||||
describe('PBL v2 simulator turn — both provider calls see the resolved config', () => {
|
||||
beforeEach(() => {
|
||||
usageMock.normalizeUsage.mockClear();
|
||||
usageMock.recordUsage.mockClear();
|
||||
delete process.env.LLM_THINKING_DISABLED;
|
||||
});
|
||||
|
||||
async function runSimulator(thinkingConfig?: ThinkingConfig) {
|
||||
const seen: Array<{ kind: 'stream' | 'generate'; thinking: unknown }> = [];
|
||||
|
||||
const model = new MockLanguageModelV3({
|
||||
doGenerate: async () => {
|
||||
seen.push({ kind: 'generate', thinking: thinkingContext.getStore() });
|
||||
return {
|
||||
content: [{ type: 'text' as const, text: '她放下杯子,抬头看你。' }],
|
||||
finishReason: { unified: 'stop' as const, raw: 'stop' },
|
||||
usage: USAGE,
|
||||
warnings: [],
|
||||
};
|
||||
},
|
||||
doStream: async () => {
|
||||
seen.push({ kind: 'stream', thinking: thinkingContext.getStore() });
|
||||
return { stream: convertArrayToReadableStream(textStep('嗯……你来了。')) };
|
||||
},
|
||||
});
|
||||
|
||||
const events: PBLSSEEvent[] = [];
|
||||
for await (const ev of runSimulatorTurn({
|
||||
project: scenarioProject(),
|
||||
userMessage: '',
|
||||
phase: 'greeting',
|
||||
languageModel: model,
|
||||
...(thinkingConfig ? { thinkingConfig } : {}),
|
||||
})) {
|
||||
events.push(ev);
|
||||
}
|
||||
expect(events.at(-1)?.type).toBe('done');
|
||||
return seen;
|
||||
}
|
||||
|
||||
it('forwards an incoming config to the character line AND the narrator', async () => {
|
||||
const thinkingConfig: ThinkingConfig = { mode: 'enabled', effort: 'low' };
|
||||
const seen = await runSimulator(thinkingConfig);
|
||||
|
||||
// Both passes must have run, or this test would silently cover only one.
|
||||
expect(seen.map((s) => s.kind).sort()).toEqual(['generate', 'stream']);
|
||||
for (const call of seen) {
|
||||
expect(call.thinking).toEqual(thinkingConfig);
|
||||
}
|
||||
});
|
||||
|
||||
it('substitutes nothing on either call when the request carries no config', async () => {
|
||||
const seen = await runSimulator();
|
||||
expect(seen.map((s) => s.kind).sort()).toEqual(['generate', 'stream']);
|
||||
for (const call of seen) {
|
||||
expect(call.thinking).toBeUndefined();
|
||||
}
|
||||
});
|
||||
|
||||
it('accounts for both passes under their own usage sources', async () => {
|
||||
await runSimulator();
|
||||
await vi.waitFor(() => {
|
||||
expect(usageMock.recordUsage).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ source: 'pbl-v2-simulator' }),
|
||||
);
|
||||
expect(usageMock.recordUsage).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ source: 'pbl-v2-simulator-narrator' }),
|
||||
);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user