Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 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
1 change: 1 addition & 0 deletions packages/cli/src/commands/review.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ describe('reviewCommand', () => {
'comment-status',
'load-rules',
'agent-prompt',
'emit-workflow',
'build-test',
'base-tree',
'test-delta',
Expand Down
4 changes: 3 additions & 1 deletion packages/cli/src/commands/review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { publishAssetsCommand } from './review/publish-assets.js';
import { resolveAnchorsCommand } from './review/resolve-anchors.js';
import { checkCoverageCommand } from './review/check-coverage.js';
import { agentPromptCommand } from './review/agent-prompt.js';
import { emitWorkflowCommand } from './review/emit-workflow.js';
import { buildTestCommand } from './review/build-test.js';
import { baseTreeCommand } from './review/base-tree.js';
import { testDeltaCommand } from './review/test-delta.js';
Expand Down Expand Up @@ -57,6 +58,7 @@ export const reviewCommand: CommandModule = {
.command(commentStatusCommand)
.command(loadRulesCommand)
.command(agentPromptCommand)
.command(emitWorkflowCommand)
.command(buildTestCommand)
.command(baseTreeCommand)
.command(testDeltaCommand)
Expand All @@ -78,7 +80,7 @@ export const reviewCommand: CommandModule = {
.command(cleanupCommand)
.demandCommand(
1,
'Specify a subcommand: run, parse-args, match-remote, fetch-pr, capture-local, plan-diff, repo-context, pr-context, comment-status, load-rules, agent-prompt, build-test, base-tree, test-delta, drive, mock-provider, extract-step, script-lint, resolve-anchors, check-coverage, cost-ledger, presubmit, test-efficacy, test-plan, findings, publish-assets, compose-review, save-artifact, submit, or cleanup.',
'Specify a subcommand: run, parse-args, match-remote, fetch-pr, capture-local, plan-diff, repo-context, pr-context, comment-status, load-rules, agent-prompt, emit-workflow, build-test, base-tree, test-delta, drive, mock-provider, extract-step, script-lint, resolve-anchors, check-coverage, cost-ledger, presubmit, test-efficacy, test-plan, findings, publish-assets, compose-review, save-artifact, submit, or cleanup.',
)
.version(false),
handler: () => {
Expand Down
84 changes: 53 additions & 31 deletions packages/cli/src/commands/review/agent-prompt.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1565,12 +1565,15 @@ export function findingsSection(
* Build one agent's brief and launch prompt, write the brief beside the plan, and
* return the key and the prompt for the caller to record and print.
*
* One body for both callers on purpose: the single-agent path and `--roster` must
* emit byte-identical prompts for the same agent, because the delivery check
* compares agents against records — a drift between the two paths would read as a
* rewritten launch on a run that did everything right.
* One body for every caller on purpose: the single-agent path, `--roster` and
* `emit-workflow` must emit byte-identical prompts for the same agent, because

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.

Byte-identical prompts do not mean identical agents: workflow dispatch replaces the system prompt with one that orders the agent to be terse.

This docstring's invariant is that --roster and emit-workflow launch the same agent. But the emitted script dispatches with only label/phase, which takes the workflow fast path, and that path overrides the system prompt with WORKFLOW_SUBAGENT_SYSTEM_PROMPT (workflow-orchestrator.ts:437-439) — it tells the agent its final text is a return value to a script and ends with "Be concise. The script will parse your output."

Review briefs ask for multi-field findings with file/line/failure-scenario prose. The same roster run through the workflow yields shorter, thinner findings than --roster for the same diff. Since the PR's stated purpose is an A/B comparing two dispatchers, the comparison silently varies the agents' output contract as well as who launches them.

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.

Escalated to maintainer — not fixable from this PR's emitted artifact. Verified at the code: the terseness directive comes from the workflow runtime's fast path — WORKFLOW_SUBAGENT_SYSTEM_PROMPT (workflow-orchestrator.ts) replaces the general-purpose system prompt on dispatches without agentType, and KNOWN_AGENT_OPTS in workflow-sandbox.ts offers no opt-out. So the same roster run through the workflow really does get a different output contract than --roster; that is a property of the runtime, not of the script this command emits.

Options as I see them:

  1. Accept and document that this A/B compares dispatcher + runtime policy as one bundle (cheapest; the comparison stays internally valid, just narrower than the stated premise).
  2. Add a runtime opt that keeps the general-purpose system prompt for dispatched review agents (core change in workflow-orchestrator.ts; also needed for the per-agent caps in the sibling thread).
  3. Hold the A/B until (2) lands.

My recommendation: (1) now, with (2) tracked as a follow-up — but this is an experiment-design call, so I am leaving the question open: which of the three do you want?

中文说明

升级给维护者——无法在本 PR 的发出物层面修复。 已在代码中核实:简洁化指令来自 workflow 运行时的快速路径——WORKFLOW_SUBAGENT_SYSTEM_PROMPT(workflow-orchestrator.ts)在不带 agentType 的分发中替换通用系统提示,而 workflow-sandbox.ts 的 KNOWN_AGENT_OPTS 不提供退出选项。因此同一份 roster 经 workflow 运行时的确会得到与 --roster 不同的输出契约;这是运行时的属性,不是本命令所发脚本的属性。

我看到的选项:

  1. 接受并声明:本 A/B 比较的是分发器 + 运行时策略这一整体(成本最低;比较仍然内部有效,只是范围比声明的前提更窄)。
  2. 为分发的评审 agent 增加保留通用系统提示的运行时选项(workflow-orchestrator.ts 的 core 改动;姊妹线程中的每 agent 上限也需要它)。
  3. 在 (2) 落地前暂缓该 A/B。

我的建议:先按 (1) 执行,并把 (2) 作为后续工作跟踪——但这属于实验设计决策,因此问题保持开放:三者之中您希望选哪一个?

* the delivery check compares agents against records — a drift between the
* paths would read as a rewritten launch on a run that did everything right.
* Exported for that reason: a third caller that rebuilt this would be a second
* implementation of the invariant, and byte-parity would become something a
* test asserts rather than something the code cannot break.
*/
function buildLaunch(
export function buildLaunch(

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.

Declined (no code change) with evidence: the gate for this hunk exists and is exercised — the probe's affected set simply did not include it. emit-workflow.test.ts imports buildLaunch from ./agent-prompt.js for its byte-parity test, and the production/test compilation also depends on the export. Mutation-verified at this commit: removing the export makes the byte-parity test fail (TypeError: buildLaunch is not a function), and npm run typecheck fails too (the test file is inside the typechecked src/**/*.ts include); reverting restores green. So we confirm the first option the finding offers: emit-workflow.test.ts' byte-parity test IS the gate for this hunk, by deliberate placement — the parity assertion lives with the consumer that depends on it.

中文说明

拒绝修改(不动代码),附证据:这个 hunk 的把关测试存在且会运行——只是探针的影响集没有包含它。emit-workflow.test.ts 从 ./agent-prompt.js 导入 buildLaunch 用于其字节一致性测试,生产/测试代码的编译也依赖该导出。已在本提交上做变异验证:去掉 export 后字节一致性测试失败(TypeError: buildLaunch is not a function),npm run typecheck 同样失败(该测试文件在被类型检查的 src/**/*.ts include 之内);还原后恢复全绿。因此确认该发现提供的第一个选项:emit-workflow.test.ts 的字节一致性测试就是这个 hunk 的把关者,且是刻意放置的——一致性断言与依赖它的消费方放在一起。

report: PlanReport,
planPath: string,
spec: {
Expand Down Expand Up @@ -1619,6 +1622,44 @@ function buildLaunch(
return { key, prompt: buildChunkLaunchPrompt(report, id, briefFile) };
}

/**
* Every launch the plan's roster requires, built through `buildLaunch` and
* matched to the roster's keys under one guard.
*
* The roster is what coverage holds the run to, and the key is what the
* records and briefs are written under. They are derived in two files, and if
* they ever disagree, every delivery check downstream reads "brief never
* reached an agent" on a run that did everything right. Both dispatchers —
* `--roster` and `emit-workflow` — consume this one mapping so the guard
* exists once; a caller that rebuilt the loop would be a second implementation
* of the invariant.
*/
export function buildRosterLaunches(
report: PlanReport,
planPath: string,
rules?: string,
): Array<{ req: RequiredAgent; key: string; prompt: string }> {
const roster = requiredAgents(report as RosterPlan);
return roster.map((req) => {
const { key, prompt } = buildLaunch(
report,
planPath,
req.role === 'chunk'
? { chunk: req.chunk }
: { role: req.role, file: req.file },
rules,
);
if (key !== req.key) {
throw new Error(
`built "${key}" where the roster requires "${req.key}" — 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] R3-2: Extracting the key-mismatch guard into the shared buildRosterLaunches silently dropped the agent-prompt: --roster prefix the --roster path's error carried before this diff — the mismatch error is now the one un-attributed diagnostic on the path that produced it, and no test pins either message.

Failure scenario: run qwen review agent-prompt --plan plan.json --roster on a plan where the roster key derivation and buildLaunch's key disagree (the exact internal-CLI-bug condition the guard exists for): the user gets an unprefixed error while every other error from the same build path keeps its agent-prompt: prefix.

Suggested fix: restore command attribution in the shared message, e.g. `agent-prompt: built "${key}" where the roster requires "${req.key}" — ...` — or parametrize the prefix so --roster and emit-workflow each get their own.

中文说明

把 key 不一致守卫抽取进共享的 buildRosterLaunches 时,静默丢失了 --roster 路径在本 diff 之前错误信息所带的 agent-prompt: --roster 前缀——这条不匹配错误现在成为该构建路径上唯一没有命令归属的诊断,且两个信息都无测试固定。

失败场景:在 roster key 派生与 buildLaunch key 不一致的 plan 上运行 qwen review agent-prompt --plan plan.json --roster(这正是该守卫存在的内部 CLI bug 条件):用户得到无前缀的错误,而同一条构建路径上的其他错误都保留 agent-prompt: 前缀。

建议修复:在共享信息中恢复命令归属,例如 `agent-prompt: built "${key}" where the roster requires "${req.key}" — ...`——或将前缀参数化,让 --roster 与 emit-workflow 各自得到正确归属。

— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)

'record could never be matched to the requirement. This is a bug ' +
'in the CLI, not in the call.',
);
}
return { req, key, prompt };
});
}

/**
* The digest that keys a findings role's record and brief: the identity of the
* launch material the key must tell apart — the findings list AND the effective
Expand Down Expand Up @@ -1712,7 +1753,7 @@ function findingsFileFor(
* paste to an agent as if the CLI wrote it. Control characters are flattened to
* spaces, and the separator glyph is stripped so a name cannot imitate one.
*/
function rosterLabel(req: RequiredAgent): string {
export function rosterLabel(req: RequiredAgent): string {
if (req.role === 'chunk') return `chunk ${req.chunk}`;
// The brief's label already reads `Agent 1a: Line-by-line correctness`; the
// rebuild hint downstream names roles, so keep the id visible when the label
Expand All @@ -1737,29 +1778,10 @@ function runRoster(report: PlanReport, planPath: string, rules?: string): void {
// The roster reads `plan.effort` (written by the capturing command), so a
// `medium` plan builds the reduced set here without an `--effort` flag — and
// `check-coverage` holds the run to that same set from the same field.
const roster = requiredAgents(report as RosterPlan);
const blocks = roster.map((req, i) => {
const { key, prompt } = buildLaunch(
report,
planPath,
req.role === 'chunk'
? { chunk: req.chunk }
: { role: req.role, file: req.file },
rules,
);
// The roster is what coverage checks; the key is what this command records
// under. They are derived in two files, and if they ever disagree, every
// delivery check downstream reads "brief never reached an agent" on a run
// that did everything right. Refuse to hand out prompts that cannot match.
if (key !== req.key) {
throw new Error(
`agent-prompt: --roster built "${key}" where the roster requires ` +
`"${req.key}" — the record could never be matched to the requirement. ` +
'This is a bug in the CLI, not in the call.',
);
}
const launches = buildRosterLaunches(report, planPath, rules);
const blocks = launches.map(({ req, key, prompt }, i) => {
recordPrompt(planPath, key, prompt);
return `───── agent ${i + 1} of ${roster.length} — ${rosterLabel(req)} ─────\n\n${prompt}`;
return `───── agent ${i + 1} of ${launches.length} — ${rosterLabel(req)} ─────\n\n${prompt}`;
});
// Worktree-mode reviews: remind the orchestrator of the exact Agent tool
// parameters at the point of action. A run that passed both `working_dir`
Expand Down Expand Up @@ -1792,20 +1814,20 @@ function runRoster(report: PlanReport, planPath: string, rules?: string): void {
`block VERBATIM.`;
writeStdoutLine(
[
`${roster.length} agents required. Launch one agent per block below, ` +
`${launches.length} agents required. Launch one agent per block below, ` +
`passing its block VERBATIM — copy, do not retype. The ───── lines are ` +
`separators, not part of any prompt. This is the same roster ` +
`\`check-coverage\` reads out of the plan: a block you skip or reword is ` +
`a dimension nobody reviewed. Blocks are numbered \`agent k of ` +
`${roster.length}\` and the output ends with an end-of-roster line — if ` +
`${launches.length}\` and the output ends with an end-of-roster line — if ` +
`either is missing, this output was truncated in transit: every prompt ` +
`is also recorded on disk, so rebuild just the missing blocks with ` +
`--chunk <id>, or --role <r> (--file <path> for an invariant agent), ` +
`plus the same --rules this call was given.` +
descNote +
paramNote,
...blocks,
`───── end of roster — ${roster.length} agents ─────`,
`───── end of roster — ${launches.length} agents ─────`,
].join('\n\n'),
);
}
Expand Down
Loading
Loading