Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 54 additions & 4 deletions packages/cli/src/commands/review/agent-prompt.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2773,8 +2773,18 @@ describe('buildRoleBrief — every agent, not just the territory ones', () => {
// The command runs install + builds + tests in one process; the agent's default
// 120s shell timeout would kill it — the very failure this command prevents, one
// level up. So the block tells the agent to pass the tool's max, 600000ms.
//
// Pinned PER SITE, not per prompt: three sites supply the directive (the
// first call, the resume paragraph's "Same …", the efficacy probe's
// "… too"), so a whole-prompt `toContain` stayed green with any one of
// them deleted — and the deleted first-call directive is exactly the
// 120s mid-install kill this assertion's own comment names.
const p = buildRoleBrief(PR_PLAN, '7', { planPath: '/abs/tmp/plan.json' });
expect(p).toContain(`timeout: ${SHELL_TOOL_MAX_TIMEOUT_MS}`);
expect(p).toContain(
`Invoke it with \`timeout: ${SHELL_TOOL_MAX_TIMEOUT_MS}\`:`,
);
expect(p).toContain(`Same \`timeout: ${SHELL_TOOL_MAX_TIMEOUT_MS}\``);
expect(p).toContain(`\`timeout: ${SHELL_TOOL_MAX_TIMEOUT_MS}\` too`);
});

it('tells Agent 7 how to CONTINUE a run one call could not finish', () => {
Expand All @@ -2784,8 +2794,14 @@ describe('buildRoleBrief — every agent, not just the territory ones', () => {
// call teaches the agent to report a truncated dimension as a finished
// one — which is what three live reviews did.
const p = buildRoleBrief(PR_PLAN, '7', { planPath: '/abs/tmp/plan.json' });
expect(p).toContain('testScope.notRun');
expect(p).toContain('"clamped": true');
// Anchored to the continuation PARAGRAPH's own sentence: the bare
// literals are also supplied verbatim by the role-7 base brief
// (agent-briefs), so `toContain('testScope.notRun')` stayed green with
// the whole paragraph deleted.
expect(p).toContain(
'Work is left when `testScope.notRun` is non-empty, or when any ' +
'`test[]` entry has `"clamped": true`',
);

// Asserted on the CONTINUATION BLOCK ALONE, which is the whole point. The
// first cut of this test searched the entire prompt: `--resume` matched the
Expand All @@ -2802,11 +2818,45 @@ describe('buildRoleBrief — every agent, not just the territory ones', () => {
// `resolve` — not spelled as POSIX literals: on Windows the prompt carries
// `C:\\abs\\tmp\\plan.json`, and a hardcoded expectation fails there for a
// reason that has nothing to do with the continuation block.
expect(resumeBlock[0]).toContain('review build-test');
// The FULL wrapper, on THIS block: the whole-prompt pins are satisfied
// by the first invocation block and vice versa, so a wrapper deleted
// from either one shipped green — and a resume block without it execs
// bare PATH `qwen`, an old global that lacks `build-test` entirely.
expect(resumeBlock[0]).toContain(
'"${QWEN_CODE_CLI:-qwen}" review build-test',
);
expect(resumeBlock[0]).toContain(`--plan ${resolve('/abs/tmp/plan.json')}`);
// The tree too: a continuation against a different tree measures a
// different run. Never asserted before — a dropped `--worktree` line
// shipped green.
expect(resumeBlock[0]).toContain(
`--worktree ${resolve('.qwen/tmp/review-pr-6766')}`,
);
expect(resumeBlock[0]).toContain(
`--out ${join(resolve('/abs/tmp'), 'qwen-review-pr-6766-build-test.json')}`,
);
expect(resumeBlock[0]).toContain('--resume');

// And the FIRST invocation block carries its own wrapper and tree — the
// same two elements, scoped to the block that must supply them.
const firstBlock = fences.find(
(f) => f.includes('review build-test') && !f.includes('--resume'),
);
expect(firstBlock).toBeDefined();
expect(firstBlock).toContain('"${QWEN_CODE_CLI:-qwen}" review build-test');
expect(firstBlock).toContain(
`--worktree ${resolve('.qwen/tmp/review-pr-6766')}`,
);

// The third-shape sentence, at BOTH prose sites — the role-7 base brief
// and the welded resume paragraph each carry it, so a single toContain
// is satisfied by either and a one-site deletion ships green. Counted,
// not just matched: deleting the sentence anywhere drops the count, and
// an agent missing it treats the endedBeforeTests shape as continuable,
// spending a MAX_RESUME_CALLS slot on a --resume that can only answer
// "ended before its test phase".
expect(p.split('"endedBeforeTests": true').length - 1).toBe(2);
expect(p.split('do not spend a continuation on it').length - 1).toBe(2);
});

it('welds the PR into Agent 0 — an unqualified number judges the wrong issue', () => {
Expand Down
13 changes: 7 additions & 6 deletions packages/cli/src/commands/review/agent-prompt.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1430,13 +1430,14 @@ export function buildRoleBrief(
'at 106s and `packages/cli` at 401s, before the rest). Work is left when ' +
'`testScope.notRun` is non-empty, or when any `test[]` entry has ' +
'`"clamped": true` — a suite the budget started too late and killed, which ' +
'says nothing about the suite. A third shape carries no field at all: a ' +
'says nothing about the suite. A third shape ends before any suite: a ' +
'single-package repo whose budget ran out before its one suite has an ' +
'empty `test[]` and no `testScope`, and only its `note` says so — read ' +
'the note before calling the dimension finished. That shape cannot be ' +
'continued (a continuation has no recorded scope to read, and answers ' +
'"ended before its test phase" without running anything): report the ' +
'dimension UNFINISHED and do not spend a continuation on it. A resumed ' +
'empty `test[]`, no `testScope`, and `"endedBeforeTests": true` — the ' +

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The rewritten third-shape continuation sentence is pinned by no test at either prose site — this round's new pins (the adjacent testScope.notRun/"clamped": true sentence) make the unpinned rewrite beside them stand out. No assertion in agent-prompt.test.ts matches endedBeforeTests, UNFINISHED, or third shape, neither in this resume paragraph nor in the role-7 base brief's matching sentence at lib/agent-briefs.ts:538. — Failure scenario: deleting the sentence ships green; Agent 7 then treats the endedBeforeTests shape as continuable and spends one of its MAX_RESUME_CALLS on a --resume that can only answer "ended before its test phase" — the wasted-continuation loop the sentence exists to prevent.

Witness (mutation probe this round): deleted the sentence at BOTH sites → full agent-prompt.test.ts suite Tests 263 passed (263) — the deletion ships green.

Suggested fix: pin a distinctive fragment per site, e.g. expect(p).toContain('"endedBeforeTests": true') and the 'do not spend a continuation on it' clause, on both the buildRoleBrief resume paragraph and the BRIEFS role-7 brief.

中文说明

重写后的「第三形态」续跑句子在两个文案位点都没有测试钉住——本轮新增的钉住(相邻的 testScope.notRun/"clamped": true 句)反而让旁边这句未钉住的重写显得突出。agent-prompt.test.ts 中没有任何断言匹配 endedBeforeTestsUNFINISHEDthird shape,无论是这个续跑段落还是 lib/agent-briefs.ts:538 处 role-7 基础 brief 里的对应句子。

失败场景:删除这句话后整个套件仍绿;Agent 7 随后会把 endedBeforeTests 形态当作可续跑,把一次 MAX_RESUME_CALLS 花在一个只能回答「ended before its test phase」的 --resume 上——正是这句话要防止的浪费续跑循环。

证据(本轮变异探针):在两个位点同时删除该句 → agent-prompt.test.ts 全套 Tests 263 passed (263)——删除后仍全绿。

修复建议:在两个位点各钉一个可区分的片段,例如对 buildRoleBrief 续跑段落与 BRIEFS role-7 brief 同时断言 expect(p).toContain('"endedBeforeTests": true') 与 'do not spend a continuation on it' 子句。

— qwen3.8-max via Qwen Code /review (v0.21.13)

"report's own stamp — with the note naming the unrun suite. That shape " +
'cannot be continued (a continuation has no recorded scope to read; a ' +
'`--resume` on it answers "ended before its test phase" and points at a ' +
'fresh run): report the dimension UNFINISHED and do not spend a ' +
'continuation on it. A resumed ' +
'call skips install and build and ' +
'runs only what is left, merging into the SAME report file. Same ' +
`\`timeout: ${SHELL_TOOL_MAX_TIMEOUT_MS}\`, and at most ` +
Expand Down
Loading
Loading