mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-08-22 15:15:18 +00:00
chore(core): P3 self-review R1 — align worktree suffix wording + 6 test gaps
R1 of pre-push adversarial self-review on PR #5034 surfaced 6 confirmed findings across 6 diverse lenses (correctness / security / reuse-altitude / self-invariant / consumer-breakage / test-gaps). Each finding faced 2 independent skeptics defaulting to refuted=true; 6 survived majority challenge. Source code: - Worktree-preserved suffix wording now matches AgentTool's formatWorktreeSuffix (agent.ts:1700-1719) verbatim, including the `git worktree add <path> <branch>` recovery hint for the directory- removed-but-branch-preserved race. Test gaps closed: - schema-mode success after 1 nudge (round-2 args captured) - schema-mode success after 2 nudges (round-3 args captured) - schema-mode + agentType together — floor disallowedTools still unioned - schema-mode caller-abort takes priority over the StructuredOutput terminal error (signal.aborted check at workflow-orchestrator.ts:489-490) - override path dispose() runs in finally on the success path - override path dispose() runs in finally on the terminate-mode-error path Declined R1 finding: negative tests for invalid opt types (schema/model/ agentType passed null/number/empty-string). Adding upfront type validation is scope creep — upstream does not, P1/P2 do not, and the workflow tool is model-authored where these inputs are extremely unlikely. Existing AJV / SubagentManager downstream errors are descriptive enough. Will revisit if R2 makes a stronger case. 166/166 tests pass (workflow suite + adjacent + workflow-orchestrator). typecheck + lint clean across packages/core, packages/cli, integration-tests, sdk, webui.
This commit is contained in:
LaZzyMan
parent
6b4d721638
commit
e1c5ec79c3
2 changed files with 255 additions and 2 deletions
|
|
@ -1062,4 +1062,247 @@ describe('WorkflowOrchestrator P3 — agentType / model / isolation / schema', (
|
|||
await dispatch('extract', { schema: { type: 'object' } });
|
||||
expect(calls[0].eventEmitterAttached).toBe(true);
|
||||
});
|
||||
|
||||
// R1 self-review (P3-T6 gap): the schema-mode state machine in
|
||||
// createSchemaEventEmitter has a `state.result === null` guard that
|
||||
// allows the model to RECOVER from earlier failed attempts. Only the
|
||||
// 0-failure and 3+-failure boundaries were tested; the 1-failure and
|
||||
// 2-failure recovery transitions had no coverage. A regression
|
||||
// inverting the guard, or one where pendingArgs cleanup discards the
|
||||
// recovered args, would slip past the previous tests.
|
||||
it('schema-mode: success on 2nd attempt (1 nudge then valid) captures round-2 args', async () => {
|
||||
const { config } = fakeConfigWithMgr({
|
||||
onCreate: async () => ({
|
||||
finalText: '',
|
||||
terminateMode: 'CANCELLED',
|
||||
runWithEmitter: (emitter) => {
|
||||
// Round 1: invalid args, validation fails.
|
||||
emitter.emit('tool_call', {
|
||||
subagentId: 'sub',
|
||||
round: 1,
|
||||
callId: 'c1',
|
||||
name: 'structured_output',
|
||||
args: { bad: 'shape' },
|
||||
description: '',
|
||||
isOutputMarkdown: false,
|
||||
timestamp: 1,
|
||||
});
|
||||
emitter.emit('tool_result', {
|
||||
subagentId: 'sub',
|
||||
round: 1,
|
||||
callId: 'c1',
|
||||
name: 'structured_output',
|
||||
success: false,
|
||||
error: 'validation failed',
|
||||
responseParts: [],
|
||||
resultDisplay: '',
|
||||
durationMs: 1,
|
||||
timestamp: 1,
|
||||
});
|
||||
// Round 2: corrected args, validation passes. Must be captured
|
||||
// as the result, not the round-1 args.
|
||||
emitter.emit('tool_call', {
|
||||
subagentId: 'sub',
|
||||
round: 2,
|
||||
callId: 'c2',
|
||||
name: 'structured_output',
|
||||
args: { ok: true, attempt: 2 },
|
||||
description: '',
|
||||
isOutputMarkdown: false,
|
||||
timestamp: 2,
|
||||
});
|
||||
emitter.emit('tool_result', {
|
||||
subagentId: 'sub',
|
||||
round: 2,
|
||||
callId: 'c2',
|
||||
name: 'structured_output',
|
||||
success: true,
|
||||
responseParts: [],
|
||||
resultDisplay: '',
|
||||
durationMs: 1,
|
||||
timestamp: 2,
|
||||
});
|
||||
},
|
||||
}),
|
||||
});
|
||||
const dispatch = createProductionDispatch(config);
|
||||
const result = await dispatch('extract', {
|
||||
schema: { type: 'object' },
|
||||
});
|
||||
expect(result).toEqual({ ok: true, attempt: 2 });
|
||||
});
|
||||
|
||||
it('schema-mode: success on 3rd attempt (2 nudges then valid) captures round-3 args', async () => {
|
||||
const { config } = fakeConfigWithMgr({
|
||||
onCreate: async () => ({
|
||||
finalText: '',
|
||||
terminateMode: 'CANCELLED',
|
||||
runWithEmitter: (emitter) => {
|
||||
for (let r = 1; r <= 2; r++) {
|
||||
emitter.emit('tool_call', {
|
||||
subagentId: 'sub',
|
||||
round: r,
|
||||
callId: `c${r}`,
|
||||
name: 'structured_output',
|
||||
args: { bad: r },
|
||||
description: '',
|
||||
isOutputMarkdown: false,
|
||||
timestamp: r,
|
||||
});
|
||||
emitter.emit('tool_result', {
|
||||
subagentId: 'sub',
|
||||
round: r,
|
||||
callId: `c${r}`,
|
||||
name: 'structured_output',
|
||||
success: false,
|
||||
error: 'validation failed',
|
||||
responseParts: [],
|
||||
resultDisplay: '',
|
||||
durationMs: 1,
|
||||
timestamp: r,
|
||||
});
|
||||
}
|
||||
emitter.emit('tool_call', {
|
||||
subagentId: 'sub',
|
||||
round: 3,
|
||||
callId: 'c3',
|
||||
name: 'structured_output',
|
||||
args: { ok: true, attempt: 3 },
|
||||
description: '',
|
||||
isOutputMarkdown: false,
|
||||
timestamp: 3,
|
||||
});
|
||||
emitter.emit('tool_result', {
|
||||
subagentId: 'sub',
|
||||
round: 3,
|
||||
callId: 'c3',
|
||||
name: 'structured_output',
|
||||
success: true,
|
||||
responseParts: [],
|
||||
resultDisplay: '',
|
||||
durationMs: 1,
|
||||
timestamp: 3,
|
||||
});
|
||||
},
|
||||
}),
|
||||
});
|
||||
const dispatch = createProductionDispatch(config);
|
||||
const result = await dispatch('extract', {
|
||||
schema: { type: 'object' },
|
||||
});
|
||||
expect(result).toEqual({ ok: true, attempt: 3 });
|
||||
});
|
||||
|
||||
// R1 self-review (P3-T6 gap): the disallowed-tool floor invariant
|
||||
// declares "ALWAYS applies regardless of agentType". The
|
||||
// single-option tests above exercise floor+agentType and schema
|
||||
// separately, but not their composition. A regression making the
|
||||
// floor conditional on schema being unset (e.g. mistakenly moving
|
||||
// the union inside an `if (opts.schema === undefined)` branch)
|
||||
// would pass the existing tests.
|
||||
it('schema-mode + agentType: floor disallowedTools still unioned', async () => {
|
||||
const { config, calls } = fakeConfigWithMgr({
|
||||
findSubagentByName: async () => ({
|
||||
name: 'Permissive',
|
||||
description: 'allows SendMessage explicitly',
|
||||
systemPrompt: 'permissive',
|
||||
level: 'project',
|
||||
disallowedTools: ['Foo'],
|
||||
}),
|
||||
onCreate: async (_call, _ee) => ({
|
||||
finalText: '',
|
||||
terminateMode: 'CANCELLED',
|
||||
runWithEmitter: (emitter) => {
|
||||
emitter.emit('tool_call', {
|
||||
subagentId: 'sub',
|
||||
round: 1,
|
||||
callId: 'c1',
|
||||
name: 'structured_output',
|
||||
args: { ok: true },
|
||||
description: '',
|
||||
isOutputMarkdown: false,
|
||||
timestamp: 1,
|
||||
});
|
||||
emitter.emit('tool_result', {
|
||||
subagentId: 'sub',
|
||||
round: 1,
|
||||
callId: 'c1',
|
||||
name: 'structured_output',
|
||||
success: true,
|
||||
responseParts: [],
|
||||
resultDisplay: '',
|
||||
durationMs: 1,
|
||||
timestamp: 1,
|
||||
});
|
||||
},
|
||||
}),
|
||||
});
|
||||
const dispatch = createProductionDispatch(config);
|
||||
await dispatch('extract', {
|
||||
agentType: 'Permissive',
|
||||
schema: { type: 'object' },
|
||||
});
|
||||
const disallowed = calls[0].config.disallowedTools ?? [];
|
||||
expect(disallowed).toEqual(
|
||||
expect.arrayContaining(['Foo', 'send_message', 'exit_plan_mode']),
|
||||
);
|
||||
});
|
||||
|
||||
// R1 self-review (P3-T6 gap): caller-abort taking priority over
|
||||
// "completed without StructuredOutput" is a contract boundary the
|
||||
// dispatch enforces at the explicit `if (signal?.aborted)` check.
|
||||
// Without this test, a refactor removing the check would silently
|
||||
// convert user-cancelled schema runs into schema-failure errors.
|
||||
it('schema-mode: caller abort takes priority over terminal "no structured_output" error', async () => {
|
||||
const externalAbort = new AbortController();
|
||||
const { config } = fakeConfigWithMgr({
|
||||
onCreate: async () => ({
|
||||
finalText: '',
|
||||
terminateMode: 'CANCELLED',
|
||||
runWithEmitter: (_emitter) => {
|
||||
// Caller-side abort fires while the subagent is in flight but
|
||||
// before any structured_output call. After execute() returns,
|
||||
// signal.aborted is true AND state.result is still null — the
|
||||
// dispatch must throw AbortError, not the StructuredOutput
|
||||
// terminal error.
|
||||
externalAbort.abort();
|
||||
},
|
||||
}),
|
||||
});
|
||||
const dispatch = createProductionDispatch(config, externalAbort.signal);
|
||||
await expect(
|
||||
dispatch('extract', { schema: { type: 'object' } }),
|
||||
).rejects.toThrow(/aborted/i);
|
||||
});
|
||||
|
||||
// R1 self-review (P3-T6 gap): the override path's dispose() must run
|
||||
// in a finally so per-agent MCP processes / hooks don't leak past the
|
||||
// dispatch — including on the exception path. The test harness has a
|
||||
// `disposed` counter that no test asserts on; this closes that gap on
|
||||
// both the success and the thrown-from-execute paths.
|
||||
it('override path always calls dispose() on the success path', async () => {
|
||||
// Use model-only override (no agentType) so we don't go through the
|
||||
// SubagentManager resolution path. The ephemeral-default branch
|
||||
// still routes through createAgentHeadless and therefore dispose().
|
||||
const helper = fakeConfigWithMgr({
|
||||
onCreate: async () => ({ finalText: 'done', terminateMode: 'GOAL' }),
|
||||
});
|
||||
const dispatch = createProductionDispatch(helper.config);
|
||||
await dispatch('hi', { model: 'qwen3-max' });
|
||||
expect(helper.disposed).toBeGreaterThanOrEqual(1);
|
||||
});
|
||||
|
||||
it('override path always calls dispose() even when terminateMode is non-GOAL', async () => {
|
||||
const helper = fakeConfigWithMgr({
|
||||
onCreate: async () => ({
|
||||
finalText: '',
|
||||
terminateMode: 'ERROR', // non-GOAL → dispatch throws after execute
|
||||
}),
|
||||
});
|
||||
const dispatch = createProductionDispatch(helper.config);
|
||||
await expect(dispatch('hi', { model: 'qwen3-max' })).rejects.toThrow(
|
||||
/terminate mode: ERROR/,
|
||||
);
|
||||
expect(helper.disposed).toBeGreaterThanOrEqual(1);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -764,11 +764,21 @@ function appendWorktreePreservedSuffix(
|
|||
finalText: string,
|
||||
preserved: WorktreePreservedInfo,
|
||||
): string {
|
||||
// Wording mirrors AgentTool's formatWorktreeSuffix (agent.ts:1700-1719)
|
||||
// verbatim so a user who has seen both tools' worktree-preserved messages
|
||||
// sees one consistent shape. AgentTool's version includes the
|
||||
// `git worktree add <path> <branch>` recovery hint for the
|
||||
// directory-removed-but-branch-preserved race; the Workflow path hits the
|
||||
// same race (cleanupWorkflowWorktree's result.branchPreserved branch) so
|
||||
// it gets the same hint.
|
||||
const sep = finalText.endsWith('\n') ? '\n' : '\n\n';
|
||||
if (preserved.path) {
|
||||
return `${finalText}${sep}[worktree preserved at ${preserved.path} on branch ${preserved.branch}]`;
|
||||
return `${finalText}${sep}[worktree preserved: ${preserved.path} (branch ${preserved.branch})]`;
|
||||
}
|
||||
return `${finalText}${sep}[worktree branch preserved: ${preserved.branch} (directory already removed)]`;
|
||||
return (
|
||||
`${finalText}${sep}[worktree directory removed; branch ${preserved.branch} ` +
|
||||
`preserved — recover with \`git worktree add <path> ${preserved.branch}\`]`
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue