diff --git a/packages/cli/src/commands/review/agent-prompt.test.ts b/packages/cli/src/commands/review/agent-prompt.test.ts index 6b9710343a0..93bdcaa002d 100644 --- a/packages/cli/src/commands/review/agent-prompt.test.ts +++ b/packages/cli/src/commands/review/agent-prompt.test.ts @@ -1253,6 +1253,50 @@ describe('--roster — every prompt the plan requires, in one call', () => { rmSync(dir, { recursive: true, force: true }); } }); + + it('emits the working_dir parameter note when worktreePath is present', () => { + // A run that passed both `working_dir` and `isolation: "worktree"` failed + // all 11 agents (mutually exclusive). The roster is the last text the + // orchestrator reads before constructing agent calls — the parameter note + // must be there, not just 400 lines back in SKILL.md. + const dir = mkdtempSync(join(tmpdir(), 'ap-roster-wt-')); + try { + const wt = '.qwen/tmp/review-pr-9999'; + const plan = join(dir, 'plan.json'); + writeFileSync( + plan, + JSON.stringify({ ...PLAN, worktreePath: wt, prNumber: '9999' }), + ); + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + roster: true, + }); + const printed = (writeStdoutLine as unknown as Mock).mock + .calls[0][0] as string; + expect(printed).toContain(`working_dir: "${wt}"`); + expect(printed).toContain('Do NOT set `isolation`'); + expect(printed).toContain('mutually exclusive'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('omits the parameter note when worktreePath is absent', () => { + const dir = mkdtempSync(join(tmpdir(), 'ap-roster-nowt-')); + try { + const plan = join(dir, 'plan.json'); + writeFileSync(plan, JSON.stringify(PLAN)); + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + roster: true, + }); + const printed = (writeStdoutLine as unknown as Mock).mock + .calls[0][0] as string; + expect(printed).not.toContain('Do NOT set `isolation`'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); }); // Dogfooded on a real 3A review: the orchestrator delivered Step 3 prompts verbatim diff --git a/packages/cli/src/commands/review/agent-prompt.ts b/packages/cli/src/commands/review/agent-prompt.ts index d62b76369a4..9824797b60c 100644 --- a/packages/cli/src/commands/review/agent-prompt.ts +++ b/packages/cli/src/commands/review/agent-prompt.ts @@ -1257,6 +1257,22 @@ function runRoster(report: PlanReport, planPath: string, rules?: string): void { recordPrompt(planPath, key, prompt); return `───── agent ${i + 1} of ${roster.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` + // and `isolation: "worktree"` failed all 11 agents (mutually exclusive) and + // the review produced nothing. The roster is the last text the orchestrator + // reads before constructing agent calls — a reminder here is worth more than + // one 400 lines back in SKILL.md. + const wt = report.worktreePath; + const paramNote = + typeof wt === 'string' && wt + ? `\n\n**Agent tool parameters (worktree mode):** Set ` + + `\`working_dir: "${wt}"\` and ` + + `\`subagent_type: "general-purpose"\`, \`run_in_background: false\` ` + + `on EVERY agent call below. Do NOT set \`isolation\` — the worktree ` + + `already exists; \`isolation\` creates a new copy and is mutually ` + + `exclusive with \`working_dir\`.` + : ''; writeStdoutLine( [ `${roster.length} agents required. Launch one agent per block below, ` + @@ -1268,7 +1284,8 @@ function runRoster(report: PlanReport, planPath: string, rules?: string): void { `either is missing, this output was truncated in transit: every prompt ` + `is also recorded on disk, so rebuild just the missing blocks with ` + `--chunk , or --role (--file for an invariant agent), ` + - `plus the same --rules this call was given.`, + `plus the same --rules this call was given.` + + paramNote, ...blocks, `───── end of roster — ${roster.length} agents ─────`, ].join('\n\n'), diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 91e6c1ec233..5a653b6653c 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -382,7 +382,7 @@ A check you perform silently is a check you skip, and this one has been skipped: **Every agent MUST return inline: set `subagent_type: "general-purpose"` and `run_in_background: false` on every `agent` call.** Do NOT fork them — never set `subagent_type: "fork"`. A fork runs fire-and-forget and its findings never come back to you, so the review would stall in Step 4 with nothing to aggregate. You need every agent's findings returned to you inline. -**For same-repo PR reviews (worktree mode), every `agent` call MUST also set `working_dir: ""`** — the `worktreePath` from the Step 1 fetch report (a repo-relative path like `.qwen/tmp/review-pr-`; pass it through as-is). This sets each agent's working directory to the PR worktree, so its `git diff`, `grep_search`, file reads, and Agent 7's build/test **resolve against the PR's code, not the user's main checkout**. It is a deterministic, harness-level cwd pin — it does NOT depend on the agent remembering to `cd`, and it is what makes reviewing multiple PRs concurrently safe. (It pins the working directory; it is not a hard filesystem sandbox — an absolute path could still reach elsewhere — but normal review operations stay inside the worktree.) This rule applies to **every** agent the review workflow launches — not just the Step 3 dimension agents, but also the Step 4 verification agent and the Step 5 reverse-audit agents (both restated below). Do NOT set `working_dir` for **local-diff, file-path, or cross-repo lightweight** reviews — those have no worktree, so the agents run in the main project directory. +**For same-repo PR reviews (worktree mode), every `agent` call MUST also set `working_dir: ""`** — the `worktreePath` from the Step 1 fetch report (a repo-relative path like `.qwen/tmp/review-pr-`; pass it through as-is). This sets each agent's working directory to the PR worktree, so its `git diff`, `grep_search`, file reads, and Agent 7's build/test **resolve against the PR's code, not the user's main checkout**. It is a deterministic, harness-level cwd pin — it does NOT depend on the agent remembering to `cd`, and it is what makes reviewing multiple PRs concurrently safe. (It pins the working directory; it is not a hard filesystem sandbox — an absolute path could still reach elsewhere — but normal review operations stay inside the worktree.) This rule applies to **every** agent the review workflow launches — not just the Step 3 dimension agents, but also the Step 4 verification agent and the Step 5 reverse-audit agents (both restated below). Do NOT set `working_dir` for **local-diff, file-path, or cross-repo lightweight** reviews — those have no worktree, so the agents run in the main project directory. **Do NOT set `isolation` on review agents.** `isolation: "worktree"` creates a brand-new worktree copy of the repo; the review worktree already exists at `worktreePath`, and `isolation` is mutually exclusive with `working_dir` — passing both fails every agent call with a parameter error and the review produces nothing. **You no longer compose these prompts. `qwen review agent-prompt` does** — one `--roster` call builds every one of them, and each block it prints goes to its agent unedited. It already contains everything the list below used to ask you to remember: `diffPathAbsolute` and the exact `read_file` ranges for that role (its own `offset`/`limit` for a chunk agent; every chunk for a whole-diff or 3A agent; the post-change file plus `addedRanges[]` and its own `diffRange` for an invariant agent), the agent's focus areas, the severity definitions verbatim, the finding format, and the project rules. **Never give an agent a `git diff` command** — see "Diff capture and the review topology" in Step 1 for why. In worktree-mode PR reviews the agent's `working_dir` is the PR worktree, so `grep_search` and source-file reads resolve against the PR's code automatically — the agent must NOT `cd` into the worktree or prefix absolute paths for those.