diff --git a/packages/cli/src/commands/review.test.ts b/packages/cli/src/commands/review.test.ts index 35be397032a..ba3e8a572e9 100644 --- a/packages/cli/src/commands/review.test.ts +++ b/packages/cli/src/commands/review.test.ts @@ -54,6 +54,7 @@ describe('reviewCommand', () => { 'comment-status', 'load-rules', 'agent-prompt', + 'emit-workflow', 'build-test', 'base-tree', 'scratch-tree', diff --git a/packages/cli/src/commands/review.ts b/packages/cli/src/commands/review.ts index e2bc5b27950..c9ec9bff47e 100644 --- a/packages/cli/src/commands/review.ts +++ b/packages/cli/src/commands/review.ts @@ -26,6 +26,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 { scratchTreeCommand } from './review/scratch-tree.js'; @@ -69,6 +70,7 @@ export const reviewCommand: CommandModule = { .command(commentStatusCommand) .command(loadRulesCommand) .command(agentPromptCommand) + .command(emitWorkflowCommand) .command(buildTestCommand) .command(baseTreeCommand) .command(scratchTreeCommand) @@ -94,7 +96,7 @@ export const reviewCommand: CommandModule = { .command(cleanupCommand) .demandCommand( 1, - 'Specify a subcommand: run, parse-args, match-remote, meta, issue-context, fetch-diff, comment-body, fetch-pr, capture-local, plan-diff, repo-context, pr-context, comment-status, load-rules, agent-prompt, build-test, base-tree, scratch-tree, test-delta, drive, ab-drive, mock-provider, extract-step, script-lint, revert-hunk, resolve-anchors, check-coverage, cost-ledger, presubmit, test-efficacy, test-plan, findings, recover-findings, publish-assets, compose-review, save-artifact, submit, or cleanup.', + 'Specify a subcommand: run, parse-args, match-remote, meta, issue-context, fetch-diff, comment-body, fetch-pr, capture-local, plan-diff, repo-context, pr-context, comment-status, load-rules, agent-prompt, emit-workflow, build-test, base-tree, scratch-tree, test-delta, drive, ab-drive, mock-provider, extract-step, script-lint, revert-hunk, resolve-anchors, check-coverage, cost-ledger, presubmit, test-efficacy, test-plan, findings, recover-findings, publish-assets, compose-review, save-artifact, submit, or cleanup.', ) .version(false), handler: () => { diff --git a/packages/cli/src/commands/review/agent-prompt.ts b/packages/cli/src/commands/review/agent-prompt.ts index bdc04c4573d..3fba9b83329 100644 --- a/packages/cli/src/commands/review/agent-prompt.ts +++ b/packages/cli/src/commands/review/agent-prompt.ts @@ -1351,8 +1351,12 @@ function fetchedShaOf(report: PlanReport): string | undefined { * have any. Resolved against the process cwd, like every other use of * `worktreePath` here: the report stores it repo-relative and review commands * run from the project root. + * + * Exported for `emit-workflow`, which must probe the tree exactly the way this + * command's handler does: both paths build through `buildLaunch`, and a probe + * only one of them ran is a divergence in the briefs the two paths bake. */ -function worktreeResidueOf(report: PlanReport): WorktreeResidue { +export function worktreeResidueOf(report: PlanReport): WorktreeResidue { const wt = report.worktreePath; if (typeof wt !== 'string' || !wt) return { paths: [], total: 0 }; // Hand over the sha fetch-pr recorded: committing the contamination moves @@ -2305,12 +2309,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 + * 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 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( report: PlanReport, planPath: string, spec: { diff --git a/packages/cli/src/commands/review/emit-workflow.test.ts b/packages/cli/src/commands/review/emit-workflow.test.ts new file mode 100644 index 00000000000..ce3477996ef --- /dev/null +++ b/packages/cli/src/commands/review/emit-workflow.test.ts @@ -0,0 +1,808 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { + existsSync, + lstatSync, + mkdirSync, + mkdtempSync, + readdirSync, + readFileSync, + realpathSync, + rmSync, + symlinkSync, + writeFileSync, +} from 'node:fs'; +import { createHash } from 'node:crypto'; +import { tmpdir } from 'node:os'; +import { basename, dirname, join, resolve, sep } from 'node:path'; +import { execFileSync } from 'node:child_process'; +import { Storage } from '@qwen-code/qwen-code-core'; + +const mocks = vi.hoisted(() => ({ + writeStdoutLine: vi.fn(), + writeStderrLine: vi.fn(), + buildLaunchOverride: null as + | null + | (() => { + key: string; + prompt: string; + }), +})); + +vi.mock('../../utils/stdioHelpers.js', () => ({ + writeStdoutLine: mocks.writeStdoutLine, + writeStderrLine: mocks.writeStderrLine, +})); +// Delegation mock: every case runs the REAL builder unless it stands in a +// return of its own. The one shape no plan can produce — a key disagreeing +// with the roster — is how the CLI-internal mismatch guard is pinned below. +vi.mock('./agent-prompt.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + buildLaunch: (...args: Parameters) => + mocks.buildLaunchOverride + ? mocks.buildLaunchOverride() + : actual.buildLaunch(...args), + }; +}); +import { + buildFanOutRoster, + emitWorkflowCommand, + fanOutBlocker, +} from './emit-workflow.js'; +import { buildLaunch } from './agent-prompt.js'; +import { briefPath, readRecordedPrompts } from './lib/prompt-record.js'; +import { RESIDUE_PATH_CAP, worktreeResidue } from './lib/worktree.js'; +import { isolateHostGitConfig } from './lib/test-utils.js'; +import { requiredAgents, type RosterPlan } from './lib/roster.js'; +import { + GeneratedWorkflowDirUnavailableError, + reviewWorkflowsDir, + reviewWorkflowScriptPath, +} from './lib/paths.js'; +import type { PlanReport } from './lib/report.js'; + +beforeEach(() => { + mocks.writeStdoutLine.mockClear(); + mocks.writeStderrLine.mockClear(); + mocks.buildLaunchOverride = null; +}); + +/** + * A small local review: uncommitted changes, no PR, no worktree, under both + * Step 3A thresholds (srcDiffLines <= 500, diffLines <= 3200). + */ +function localPlan(over: Record = {}): PlanReport { + return { + diffPathAbsolute: '/abs/.qwen/tmp/qwen-review-local-diff.txt', + diffLines: 240, + diffChars: 8000, + srcDiffLines: 180, + testDiffLines: 60, + docsDiffLines: 0, + generatedDiffLines: 0, + untrackedFiles: [], + effort: 'high', + chunks: [ + { + id: 1, + startLine: 1, + endLine: 240, + lines: 240, + chars: 8000, + maxLineChars: 120, + oversized: false, + files: [{ path: 'src/a.ts', newStart: 1, newEnd: 200 }], + }, + ], + files: [ + { + path: 'src/a.ts', + kind: 'source', + heavy: false, + addedLines: 150, + removedLines: 30, + fileLines: 400, + }, + ], + budget: { toolCalls: 40 }, + ...over, + } as unknown as PlanReport; +} + +describe('emit-workflow — the roster it bakes into the script', () => { + let dir: string; + let planPath: string; + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'emit-wf-')); + planPath = join(dir, 'plan.json'); + writeFileSync(planPath, JSON.stringify(localPlan()), 'utf8'); + }); + + afterEach(() => { + rmSync(dir, { recursive: true, force: true }); + }); + + // Routing through `requiredAgents` is what makes the fan-out and the gate + // that checks it read one list. A roster this command shortened would be a + // dimension nobody reviewed, reported as a complete review. + it('emits exactly the agents the plan requires, under the keys coverage looks up', () => { + const plan = localPlan(); + const agents = buildFanOutRoster(plan, planPath); + expect(agents.map((a) => a.key)).toEqual( + requiredAgents(plan as unknown as RosterPlan).map((r) => r.key), + ); + expect(agents.length).toBeGreaterThan(1); + }); + + // Byte-parity with the hand-launched path is structural — both go through + // `buildLaunch` — and this pins it so a future refactor that gives this + // command its own builder fails here rather than in a review whose delivery + // check reads "the prompt was rewritten". + it('emits the same prompt the hand-launched roster would', () => { + const plan = localPlan(); + const agents = buildFanOutRoster(plan, planPath); + for (const req of requiredAgents(plan as unknown as RosterPlan)) { + const { key, prompt } = buildLaunch( + plan, + planPath, + { role: req.role as never, file: req.file }, + undefined, + ); + expect(agents.find((a) => a.key === key)?.prompt).toBe(prompt); + } + }); + + it('threads the project rules into every brief, like --roster does', () => { + const plan = localPlan(); + const rules = 'RULE: never call it a nit — MARKER-8f3a'; + const agents = buildFanOutRoster(plan, planPath, rules); + // Agent 7 runs deterministic build and test commands, not a review, so + // its brief carries no rules; every reviewing role's does. + const reviewing = agents.filter((a) => a.key !== '7'); + expect(reviewing.length).toBeGreaterThan(1); + for (const a of reviewing) { + // The rules live in the brief the agent reads, not in the launch line. + expect(readFileSync(briefPath(planPath, a.key), 'utf8')).toContain( + 'MARKER-8f3a', + ); + } + }); + + // `check-coverage` compares each launch against what the CLI recorded + // handing out. Without a record, a launched agent reads as one that never + // ran — the whole roster would come back as unlaunched. + it('records every prompt it hands out, so the coverage gate can match them', () => { + const agents = buildFanOutRoster(localPlan(), planPath); + const recorded = readRecordedPrompts(planPath); + for (const a of agents) { + expect(recorded.get(a.key)).toBe(a.prompt); + } + }); + + it('carries the effort the plan recorded, not a caller argument', () => { + // A medium plan drops the three adversarial personas. The roster reads + // `plan.effort`, so this command cannot be asked for a different set. + const high = buildFanOutRoster(localPlan(), planPath); + const medium = buildFanOutRoster(localPlan({ effort: 'medium' }), planPath); + expect(high.length).toBeGreaterThan(medium.length); + expect(medium.map((a) => a.key)).not.toContain('6a'); + }); + + it('builds a diff-only review too, not just a local one', () => { + // `diff-only` (cross-repo lightweight) has no tree, so its roster drops + // 1c and 7. + const diffOnly = localPlan({ untrackedFiles: undefined }); + const agents = buildFanOutRoster(diffOnly, planPath); + expect(agents.map((a) => a.key)).not.toContain('7'); + expect(agents.map((a) => a.key)).not.toContain('1c'); + expect(agents.length).toBeGreaterThan(1); + }); +}); + +describe('emit-workflow — what it refuses', () => { + let dir: string; + let planPath: string; + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'emit-wf-')); + planPath = join(dir, 'plan.json'); + writeFileSync(planPath, JSON.stringify(localPlan()), 'utf8'); + }); + + afterEach(() => { + rmSync(dir, { recursive: true, force: true }); + }); + + // A 3B roster grows one agent per chunk while a workflow run is wall-clock + // capped end to end and the generated script fails closed on any agent + // that does not deliver — the bigger the fan-out, the more certain the run + // is to exhaust its budget and discard every agent. (A large result is not + // truncated away: the scheduler persists it and hands the model a pointer — + // the caps are the bound.) The refusal must name that bound — the builder + // below can express the roster perfectly well. + it('refuses a territory fan-out, naming the delivery bound', () => { + const territory = localPlan({ srcDiffLines: 2000, diffLines: 6000 }); + expect(fanOutBlocker(territory as unknown as RosterPlan)).toMatch( + /territory fan-out \(Step 3B\)/, + ); + expect(() => buildFanOutRoster(territory, planPath)).toThrow( + /wall-clock capped/, + ); + // Refused BEFORE anything is written: no brief, no record. + expect(readRecordedPrompts(planPath).size).toBe(0); + }); + + // A plan whose sizes failed to arrive is not a small review — its topology + // is UNKNOWABLE, and `isTerritoryFanOut`'s missing-to-zero coercion would + // bake the guess that it is 3A into the script. + it('refuses an unsized plan — unknowable topology is not 3A', () => { + for (const unsized of [ + localPlan({ srcDiffLines: null, diffLines: null }), + localPlan({ srcDiffLines: undefined }), + localPlan({ diffLines: Number.NaN }), + ]) { + expect(fanOutBlocker(unsized as unknown as RosterPlan)).toMatch( + /no usable diff size/, + ); + expect(() => buildFanOutRoster(unsized, planPath)).toThrow( + /no usable diff size/, + ); + } + expect(readRecordedPrompts(planPath).size).toBe(0); + }); + + // The roster key is what `check-coverage` looks up and what the brief was + // written under. `requiredAgents` and `buildLaunch` derive it the same way, + // so a mismatch is a contradiction inside the CLI that no plan can produce + // — the guard is pinned directly, with the builder stood in for. Left + // uncaught, every delivery check downstream reads "brief never reached an + // agent" on a run that did everything right. + it('refuses a roster key the builder did not build under', () => { + mocks.buildLaunchOverride = () => ({ key: 'WRONG-KEY', prompt: 'PROMPT' }); + expect(() => buildFanOutRoster(localPlan(), planPath)).toThrow( + /built "WRONG-KEY" where the roster requires/, + ); + // The mismatched prompt was never recorded as handed out. + expect(readRecordedPrompts(planPath).size).toBe(0); + }); + + it('accepts a plan exactly at the 3A thresholds', () => { + expect( + fanOutBlocker( + localPlan({ + srcDiffLines: 500, + diffLines: 3200, + }) as unknown as RosterPlan, + ), + ).toBeNull(); + }); + + // A worktree is not a refusal: `agent({workingDir})` exists and the + // generated script passes it. + it('emits a worktree review rather than refusing it', () => { + const agents = buildFanOutRoster( + localPlan({ worktreePath: '.qwen/tmp/review-pr-42' }), + planPath, + ); + expect(agents.length).toBeGreaterThan(1); + }); +}); + +describe('emit-workflow — where it writes', () => { + let dir: string; + let projectDir: string; + + beforeEach(() => { + // Canonicalized so both sides of every comparison spell paths alike: + // `reviewWorkflowScriptPath` realpaths an EXISTING plan before hashing, + // and a fixture planted before the plan exists would otherwise digest + // the aliased tmpdir spelling while the handler digests the canonical + // one — on hosts whose tmpdir resolves through a symlink (macOS's + // /var -> /private/var) that is one script name on each side. + dir = realpathSync(mkdtempSync(join(tmpdir(), 'emit-wf-'))); + projectDir = join(dir, 'project-dir'); + // The harness exports both for every review subcommand; the session id + // carries a dot on purpose, because the harness's directory names do not. + vi.stubEnv('QWEN_CODE_PROJECT_DIR', projectDir); + vi.stubEnv('QWEN_CODE_SESSION_ID', 'sess.1'); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + rmSync(dir, { recursive: true, force: true }); + }); + + function run(plan: string): void { + (emitWorkflowCommand.handler as (a: unknown) => void)({ plan }); + } + + // Not a preference: `Workflow({scriptPath})` loads through + // `readWorkflowFileSecurely`, which accepts the saved-workflow directories + // and the generated-scripts root and nothing else. A script beside the + // plan is a script the tool will not open; a script in a saved directory + // is a slash command. + it('writes the script under the generated-scripts root, per session', () => { + const plan = join(dir, 'plan.json'); + writeFileSync(plan, JSON.stringify(localPlan()), 'utf8'); + run(plan); + + const scriptPath = reviewWorkflowScriptPath(plan); + // The readable sanitized prefix, plus a digest of the RAW session id — + // sanitizing is lossy, and the digest is what keeps two sessions whose + // ids flatten identically apart (see the collision case below). + const sessionDigest = createHash('sha256') + .update('sess.1') + .digest('hex') + .slice(0, 8); + expect(dirname(scriptPath)).toBe( + join( + projectDir, + 'workflows', + 'generated', + 'review', + `sess_1-${sessionDigest}`, + ), + ); + expect(existsSync(scriptPath)).toBe(true); + const script = readFileSync(scriptPath, 'utf8'); + expect(script).toContain('export const meta'); + expect(script).toContain('const AGENTS = ['); + expect(script).toContain('parallel('); + // The one line the skill will parse to build its single Workflow call; + // it must carry the absolute path, or the dispatch has nothing to load. + expect(mocks.writeStdoutLine).toHaveBeenCalledWith( + `scriptPath: ${resolve(scriptPath)}`, + ); + }); + + // The other stdout line is the dispatch contract the orchestrator acts + // on: how many agents, ONE Workflow call, no hand-built agent calls. + // Unpinned, the count or the instruction could drift without notice. + it('prints the dispatch guidance beside the path', () => { + const plan = join(dir, 'plan.json'); + writeFileSync(plan, JSON.stringify(localPlan()), 'utf8'); + run(plan); + const count = requiredAgents(localPlan() as unknown as RosterPlan).length; + expect(mocks.writeStdoutLine).toHaveBeenCalledWith( + `${count} agents required. The fan-out is a workflow: make ONE ` + + 'Workflow call with the scriptPath below and no `args`, and do not ' + + 'build agent calls by hand for this step.', + ); + }); + + // The writer's directory and the loader's trusted root are computed by two + // packages from two inputs. Pin them together through core's own function: + // a project dir exported by the harness is `storage.getProjectDir()`, and + // the loader trusts `storage.getGeneratedWorkflowsDir()` under it. + it('lands inside the root the Workflow loader trusts', () => { + const home = join(dir, 'home'); + vi.stubEnv('QWEN_HOME', join(home, '.qwen')); + vi.stubEnv('QWEN_RUNTIME_DIR', ''); + const storage = new Storage(join(dir, 'workspace')); + const env = { + QWEN_CODE_PROJECT_DIR: storage.getProjectDir(), + QWEN_CODE_SESSION_ID: 'sess', + }; + const root = storage.getGeneratedWorkflowsDir(); + expect(reviewWorkflowsDir(env).startsWith(root + sep)).toBe(true); + expect(reviewWorkflowScriptPath('/tmp/p.json', env).startsWith(root)).toBe( + true, + ); + }); + + it('refuses to build anything when the harness exported no project dir', () => { + vi.stubEnv('QWEN_CODE_PROJECT_DIR', ''); + const plan = join(dir, 'plan.json'); + writeFileSync(plan, JSON.stringify(localPlan()), 'utf8'); + expect(() => run(plan)).toThrow(GeneratedWorkflowDirUnavailableError); + // Decided before anything was written: a roster whose briefs and records + // exist for a script that has nowhere to go would read to the coverage + // gate as a launched fan-out that returned nothing. + expect(readRecordedPrompts(plan).size).toBe(0); + expect(existsSync(join(projectDir))).toBe(false); + }); + + // A blocked plan writes nothing at all — including the session directory: + // creating it before the blocker check left every refusal an empty tree a + // later sweep would find. + it('creates no directory for a plan it refuses', () => { + const plan = join(dir, 'plan.json'); + writeFileSync( + plan, + JSON.stringify(localPlan({ srcDiffLines: 2000, diffLines: 6000 })), + 'utf8', + ); + expect(() => run(plan)).toThrow(/territory fan-out/); + expect(existsSync(projectDir)).toBe(false); + }); + + it('falls back to a fixed subdirectory when there is no session id', () => { + vi.stubEnv('QWEN_CODE_SESSION_ID', ''); + expect(reviewWorkflowsDir()).toBe( + join(projectDir, 'workflows', 'generated', 'review', 'no-session'), + ); + }); + + // `sanitizeFilenameComponent` is lossy — `sess.1` and `sess_1` both flatten + // to `sess_1` — so the raw-id digest is what keeps two concurrent sessions + // apart. Without it they would select the same script target for the same + // plan, and the later atomic rename would hand one session's roster, rules, + // and worktree pin to the other. + it('keeps sessions that sanitize identically in separate directories', () => { + const envFor = (session: string): NodeJS.ProcessEnv => ({ + QWEN_CODE_PROJECT_DIR: projectDir, + QWEN_CODE_SESSION_ID: session, + }); + const a = reviewWorkflowsDir(envFor('sess.1')); + const b = reviewWorkflowsDir(envFor('sess_1')); + expect(a).not.toBe(b); + // The readable prefix survives on both. + expect(basename(a).startsWith('sess_1-')).toBe(true); + expect(basename(b).startsWith('sess_1-')).toBe(true); + // ...and the same plan cannot overwrite across the collision. + const plan = join(dir, 'plan.json'); + expect(reviewWorkflowScriptPath(plan, envFor('sess.1'))).not.toBe( + reviewWorkflowScriptPath(plan, envFor('sess_1')), + ); + }); + + // Nothing large may travel through the model: `args` is inline-only and the + // sandbox cannot read files, so a roster passed as args is a roster the + // model has to retype — the failure this command exists to remove. + it('carries every recorded prompt inside the script itself', () => { + const plan = join(dir, 'plan.json'); + writeFileSync(plan, JSON.stringify(localPlan()), 'utf8'); + run(plan); + + const script = readFileSync(reviewWorkflowScriptPath(plan), 'utf8'); + const recorded = readRecordedPrompts(plan); + expect(recorded.size).toBeGreaterThan(1); + for (const prompt of recorded.values()) { + // The prompt is inside the file as a JSON string literal, so compare + // against its serialized form rather than the raw text. + expect(script).toContain(JSON.stringify(prompt)); + } + }); + + it('bakes the worktree pin from the plan', () => { + const plan = join(dir, 'plan.json'); + writeFileSync( + plan, + JSON.stringify(localPlan({ worktreePath: '.qwen/tmp/review-pr-42' })), + 'utf8', + ); + run(plan); + expect(readFileSync(reviewWorkflowScriptPath(plan), 'utf8')).toContain( + 'const WORKING_DIR = ".qwen/tmp/review-pr-42";', + ); + }); + + it('replaces a symlinked script entry without writing through it', () => { + const victim = join(dir, 'victim.js'); + const plan = join(dir, 'plan.json'); + const scriptPath = reviewWorkflowScriptPath(plan); + mkdirSync(dirname(scriptPath), { recursive: true }); + writeFileSync(victim, 'keep me', 'utf8'); + symlinkSync(victim, scriptPath); + writeFileSync(plan, JSON.stringify(localPlan()), 'utf8'); + + run(plan); + expect(readFileSync(victim, 'utf8')).toBe('keep me'); + expect(lstatSync(scriptPath).isSymbolicLink()).toBe(false); + expect(readFileSync(scriptPath, 'utf8')).toContain('export const meta'); + }); + + // The writer shares the loader's canonical-containment policy: a symlinked + // root or session directory would carry the script — embedding every review + // prompt — outside the trusted root and print a scriptPath the loader then + // refuses. The refusal must land before ANY write: briefs and prompt + // records are the delivery evidence, and evidence for a script that went + // nowhere safe would read to the coverage gate as a launched fan-out. + it('refuses a symlinked session directory before writing anything', () => { + const plan = join(dir, 'plan.json'); + writeFileSync(plan, JSON.stringify(localPlan()), 'utf8'); + const external = join(dir, 'external'); + mkdirSync(external, { recursive: true }); + const sessionDir = reviewWorkflowsDir(); + mkdirSync(dirname(sessionDir), { recursive: true }); + symlinkSync(external, sessionDir); + + expect(() => run(plan)).toThrow(/symlinked/); + expect(readdirSync(external)).toEqual([]); + expect(readRecordedPrompts(plan).size).toBe(0); + }); + + it('refuses a symlinked generated root before writing anything', () => { + const plan = join(dir, 'plan.json'); + writeFileSync(plan, JSON.stringify(localPlan()), 'utf8'); + const external = join(dir, 'external'); + mkdirSync(external, { recursive: true }); + const root = join(projectDir, 'workflows', 'generated'); + mkdirSync(dirname(root), { recursive: true }); + symlinkSync(external, root); + + expect(() => run(plan)).toThrow(/symlinked/); + expect(readdirSync(external)).toEqual([]); + expect(readRecordedPrompts(plan).size).toBe(0); + }); + + // The loop's middle component: dropped by a refactor, `lstatSync` on the + // absent session directory ENOENTs THROUGH the link, the loop breaks, and + // `mkdirSync` follows it — a stray session directory lands outside the + // trusted root before the canonical-containment check throws. Refused, but + // no longer before writing. + it('refuses a symlinked review subdirectory before writing anything', () => { + const plan = join(dir, 'plan.json'); + writeFileSync(plan, JSON.stringify(localPlan()), 'utf8'); + const external = join(dir, 'external'); + mkdirSync(external, { recursive: true }); + const reviewDir = join(projectDir, 'workflows', 'generated', 'review'); + mkdirSync(dirname(reviewDir), { recursive: true }); + symlinkSync(external, reviewDir); + + expect(() => run(plan)).toThrow(/symlinked/); + expect(readdirSync(external)).toEqual([]); + expect(readRecordedPrompts(plan).size).toBe(0); + }); + + it('leaves no temp file behind, on success or on a failed write', () => { + const plan = join(dir, 'plan.json'); + writeFileSync(plan, JSON.stringify(localPlan()), 'utf8'); + run(plan); + const scriptDir = dirname(reviewWorkflowScriptPath(plan)); + const tempFiles = () => + readdirSync(scriptDir).filter((n) => n.endsWith('.tmp')); + expect(tempFiles()).toEqual([]); + + // The failed-write half: the rename throws AFTER the temp file exists, + // the case the finally cleanup exists for. A non-empty directory at the + // target makes renameSync throw on every platform. + const scriptPath = reviewWorkflowScriptPath(plan); + rmSync(scriptPath); + mkdirSync(scriptPath); + writeFileSync(join(scriptPath, 'blocker'), 'keep me out', 'utf8'); + expect(() => run(plan)).toThrow(); + expect(tempFiles()).toEqual([]); + }); + + it('names a script per plan, so concurrent reviews do not overwrite each other', () => { + const a = join(dir, 'plan-a.json'); + const b = join(dir, 'plan-b.json'); + expect(reviewWorkflowScriptPath(a)).not.toBe(reviewWorkflowScriptPath(b)); + // A relative spelling of the same plan is the same script. + const cwd = process.cwd(); + process.chdir(dir); + try { + expect(reviewWorkflowScriptPath('plan-a.json')).toBe( + reviewWorkflowScriptPath(a), + ); + } finally { + process.chdir(cwd); + } + }); + + // The identity must survive canonical spellings of one file: on macOS the + // same existing plan is `/var/...` and `/private/var/...` at once, and the + // loader canonicalizes with realpath before it checks containment. A link + // is the platform-neutral shape of that same divergence. + it('names an existing plan one script however the path is spelled', () => { + const plan = join(dir, 'plan.json'); + writeFileSync(plan, JSON.stringify(localPlan()), 'utf8'); + const alias = join(dir, 'alias.json'); + symlinkSync(plan, alias); + expect(reviewWorkflowScriptPath(alias)).toBe( + reviewWorkflowScriptPath(plan), + ); + }); + + // Every sibling review subcommand pins its cannot-read-the-plan guard at + // handler level; this one is the same guard for this command's entry point. + it('refuses an unreadable plan path before writing anything', () => { + const plan = join(dir, 'missing-plan.json'); + expect(() => run(plan)).toThrow(/cannot read the plan/); + expect(readRecordedPrompts(plan).size).toBe(0); + expect(existsSync(reviewWorkflowScriptPath(plan))).toBe(false); + }); + + it('refuses an unreadable rules path before writing anything', () => { + const plan = join(dir, 'plan.json'); + writeFileSync(plan, JSON.stringify(localPlan()), 'utf8'); + expect(() => + (emitWorkflowCommand.handler as (a: unknown) => void)({ + plan, + rules: join(dir, 'missing-rules.md'), + }), + ).toThrow(/cannot read the rules/); + expect(readRecordedPrompts(plan).size).toBe(0); + expect(existsSync(reviewWorkflowScriptPath(plan))).toBe(false); + }); + + it('threads --rules through the handler into every reviewing brief', () => { + const plan = join(dir, 'plan.json'); + writeFileSync(plan, JSON.stringify(localPlan()), 'utf8'); + const rulesPath = join(dir, 'rules.md'); + writeFileSync( + rulesPath, + 'RULE: every brief must carry this — MARKER-rules-7c1e', + 'utf8', + ); + + (emitWorkflowCommand.handler as (a: unknown) => void)({ + plan, + rules: rulesPath, + }); + + expect(existsSync(reviewWorkflowScriptPath(plan))).toBe(true); + const keys = [...readRecordedPrompts(plan).keys()]; + // Agent 7 runs deterministic build and test commands, not a review, so + // its brief carries no rules; every reviewing role's does. + const reviewing = keys.filter((key) => key !== '7'); + expect(reviewing.length).toBeGreaterThan(1); + for (const key of reviewing) { + expect(readFileSync(briefPath(plan, key), 'utf8')).toContain( + 'MARKER-rules-7c1e', + ); + } + }); +}); + +describe('emit-workflow — residue parity with the hand-launched path', () => { + // The live #9207 shape: a shared review worktree carrying a modified file + // and a probe no commit contains. A REAL linked worktree, because the + // probe's identity gate fails closed for anything else — a bare repo + // fixture could not measure the healthy path. + let repo: string; + let tree: string; + let headSha: string; + let dir: string; + let planPath: string; + let gitIsolation: ReturnType; + + beforeEach(() => { + gitIsolation = isolateHostGitConfig(); + repo = mkdtempSync(join(tmpdir(), 'emit-wf-residue-')); + execFileSync('git', ['init', '-q', '-b', 'main'], { cwd: repo }); + execFileSync('git', ['config', 'user.email', 't@t.t'], { cwd: repo }); + execFileSync('git', ['config', 'user.name', 't'], { cwd: repo }); + writeFileSync(join(repo, 'a.ts'), 'export const x = 1;\n'); + execFileSync('git', ['add', '-A'], { cwd: repo }); + execFileSync('git', ['commit', '-qm', 'head'], { cwd: repo }); + // Every worktree-mode fetch records the fetched head sha in the plan, + // and the residue probe fails closed without a usable one — so the + // fixture anchors like a real plan. + headSha = execFileSync('git', ['rev-parse', 'HEAD'], { + cwd: repo, + encoding: 'utf8', + }).trim(); + tree = join(repo, '.qwen', 'tmp', 'review-wt'); + mkdirSync(dirname(tree), { recursive: true }); + execFileSync('git', ['worktree', 'add', '--detach', '-q', tree, 'HEAD'], { + cwd: repo, + }); + writeFileSync(join(tree, 'a.ts'), 'export const x = 2;\n'); + writeFileSync(join(tree, '__probe__.test.ts'), 'it("x", () => {});'); + + dir = mkdtempSync(join(tmpdir(), 'emit-wf-')); + planPath = join(dir, 'plan.json'); + writeFileSync( + planPath, + JSON.stringify(localPlan({ worktreePath: tree, fetchedSha: headSha })), + 'utf8', + ); + }); + + afterEach(() => { + rmSync(repo, { recursive: true, force: true }); + rmSync(dir, { recursive: true, force: true }); + gitIsolation.dispose(); + }); + + // The hand-launched path probes the worktree and threads what it finds + // into every build; this command goes through the same `buildLaunch`, so + // a dirty tree must change both sides identically. Compared on the + // BRIEFS, not the launch prompts: the residue block is evidence for the + // agent reading the brief, and the launch prompt only points at it — a + // prompt-level comparison passes with the block silently dropped. + it('bakes the same residue evidence the hand-launched roster would', () => { + const plan = localPlan({ worktreePath: tree, fetchedSha: headSha }); + const residue = worktreeResidue(tree, RESIDUE_PATH_CAP, headSha); + expect(residue.paths.length).toBeGreaterThan(0); + + const agents = buildFanOutRoster(plan, planPath); + const workflowBriefs = new Map( + agents.map((a) => [ + a.key, + readFileSync(briefPath(planPath, a.key), 'utf8'), + ]), + ); + + for (const req of requiredAgents(plan as unknown as RosterPlan)) { + // The rebuild the hand-launched path does: `agent-prompt`'s handler + // probes the tree and threads the result into every build. + buildLaunch( + plan, + planPath, + req.role === 'chunk' + ? { chunk: req.chunk } + : { role: req.role as never, file: req.file }, + undefined, + residue, + ); + expect(workflowBriefs.get(req.key)).toBe( + readFileSync(briefPath(planPath, req.key), 'utf8'), + ); + } + // The fixture IS dirty, so the paragraph must actually be present — a + // clean tree would let the byte comparison pass vacuously. + for (const brief of workflowBriefs.values()) { + expect(brief).toContain( + 'These paths differ from the commit under review', + ); + } + }); + + // The orchestrator's only notice that the tree it is about to dispatch + // against is not the commit the plan says it is. The hand-launched path + // prints it; this command owes the same. + it('warns on stderr like the hand-launched path, naming the dirty paths', () => { + buildFanOutRoster( + localPlan({ worktreePath: tree, fetchedSha: headSha }), + planPath, + ); + expect(mocks.writeStderrLine).toHaveBeenCalledWith( + expect.stringContaining( + 'the review worktree carries changes its commit does not', + ), + ); + expect(mocks.writeStderrLine).toHaveBeenCalledWith( + expect.stringContaining('a.ts'), + ); + }); + + it('warns that an unmeasured tree is not a clean one', () => { + buildFanOutRoster( + localPlan({ + worktreePath: join(dir, 'not-a-worktree'), + fetchedSha: headSha, + }), + planPath, + ); + expect(mocks.writeStderrLine).toHaveBeenCalledWith( + expect.stringContaining( + 'could not measure whether the review worktree is clean', + ), + ); + }); + + // The two warnings above name the unhealthy states only — the healthy + // run, the common case, is silence. A regression that warned on every + // worktree review would pass both of them and fail here. + it('stays silent on a measured-clean worktree', () => { + const cleanTree = join(repo, '.qwen', 'tmp', 'review-wt-clean'); + mkdirSync(dirname(cleanTree), { recursive: true }); + execFileSync( + 'git', + ['worktree', 'add', '--detach', '-q', cleanTree, 'HEAD'], + { cwd: repo }, + ); + // The fixture must be measured-clean, or the silence proves nothing: + // an unmeasured tree warns, and a probe that never ran is silent too. + const residue = worktreeResidue(cleanTree, RESIDUE_PATH_CAP, headSha); + expect(residue.paths).toEqual([]); + expect(residue.unmeasured).toBeUndefined(); + + buildFanOutRoster( + localPlan({ worktreePath: cleanTree, fetchedSha: headSha }), + planPath, + ); + expect(mocks.writeStderrLine).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/cli/src/commands/review/emit-workflow.ts b/packages/cli/src/commands/review/emit-workflow.ts new file mode 100644 index 00000000000..635f972ee0e --- /dev/null +++ b/packages/cli/src/commands/review/emit-workflow.ts @@ -0,0 +1,299 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// `qwen review emit-workflow`: the Step 3A fan-out as a workflow the runtime +// dispatches, instead of a roster the orchestrator hand-launches. +// +// `--roster` and this command build the same prompts from the same plan +// through the same function (`buildLaunch`). What differs is who launches +// them. `--roster` prints ~13 blocks and asks the orchestrator to copy each +// one into an agent call, in a single response, without editing any of them — +// three conventions this skill's gate list exists because they get broken. +// This command writes those prompts into a script file, so the orchestrator's +// call carries one path and no payload. +// +// What this does NOT change, deliberately: the briefs, the prompts, the +// roster, the coverage evidence, and how findings come back. The agents are +// the same agents reading the same briefs. That is what makes an A/B against +// the hand-launched path readable. +// +// Nothing routes through this command yet. The skill still builds its roster +// with `agent-prompt --roster`; this command exists so the generated dispatch +// can be evaluated on its own before the skill is taught to ask for it. + +import type { CommandModule } from 'yargs'; +import { readFileSync, renameSync, rmSync, writeFileSync } from 'node:fs'; +import { randomUUID } from 'node:crypto'; +import { resolve } from 'node:path'; +import { writeStderrLine, writeStdoutLine } from '../../utils/stdioHelpers.js'; +import { buildLaunch, worktreeResidueOf } from './agent-prompt.js'; +import { isTerritoryFanOut, usableLineCount } from './lib/budget.js'; +import { + ensureWritableReviewWorkflowsDir, + inertPath, + reviewWorkflowScriptPath, +} from './lib/paths.js'; +import { recordPrompt } from './lib/prompt-record.js'; +import type { PlanReport } from './lib/report.js'; +import { requiredAgents, type RosterPlan } from './lib/roster.js'; +import { + buildReviewWorkflowScript, + type WorkflowAgentSpec, +} from './workflow-script.js'; + +interface EmitWorkflowArgs { + plan: string; + rules?: string; +} + +/** + * What about this plan the generated fan-out cannot serve. `null` when + * nothing does. + * + * Every entry names a fact about what the workflow path cannot do yet, not a + * policy dial, so the list shrinks as the runtime gains capability. + */ +export function fanOutBlocker(plan: RosterPlan): string | null { + // The size ruling must precede the topology ruling: `isTerritoryFanOut` + // coerces a missing or null size to 0, so a plan whose counts failed to + // arrive — an older CLI's, or ones `JSON.stringify` corrupted from `NaN` + // to `null` — would classify as "not a territory fan-out" and be baked + // into a script as if it were small. Unknown size is not small size. + if (!usableLineCount(plan.srcDiffLines) || !usableLineCount(plan.diffLines)) { + return ( + 'this plan carries no usable diff size fields, so its topology is ' + + 'unknown and it may be a territory fan-out.' + ); + } + + // Both paths build the same chunk prompts through `buildLaunch`, and the + // emitter is topology-agnostic — a 3B roster serializes exactly like a 3A + // one. What differs is DELIVERY against the workflow runtime's caps: a run + // is wall-clock capped end to end, each subagent attempt is capped on turns + // and minutes, and an attempt that hits either becomes a `null` the + // fail-closed guard in the generated script then reads as a missing agent — + // discarding every agent that DID deliver. A large result is not truncated + // away: the scheduler persists it and hands the model a pointer. The bound + // is the caps. A 3A roster is bounded; a 3B roster is one agent per chunk + // plus the whole-diff agents, so it grows with the diff toward them without + // bound. This blocks on the roster's growth, not on the topology name, so + // it lifts the moment the runtime's caps grow with the fan-out. + if (isTerritoryFanOut(plan)) { + return ( + 'this plan is a territory fan-out (Step 3B), whose roster grows one ' + + 'agent per chunk while a workflow run is wall-clock capped end to ' + + 'end and the generated script fails closed on any agent that does ' + + 'not deliver — the larger the fan-out, the more certain the run is ' + + 'to exhaust its budget and discard every agent it dispatched.' + ); + } + + return null; +} + +/** + * Throw when the plan is one the generated fan-out cannot serve. Called by + * the handler BEFORE the session directory is created — a blocked plan must + * leave no empty tree a later sweep finds — and again by the builder, whose + * contract direct callers rely on. + */ +function refuseBlockedFanOut(plan: RosterPlan): void { + const blocker = fanOutBlocker(plan); + if (blocker) { + throw new Error( + `emit-workflow: ${blocker} Use \`agent-prompt --roster\` for this review.`, + ); + } +} + +/** + * The roster this plan requires, each entry carrying the prompt the + * hand-launched path would have printed for it. + * + * Writes as it goes — `buildLaunch` writes each brief beside the plan, and + * each prompt is recorded — because those two artifacts ARE the delivery + * evidence: the brief is what the agent reads, and the record is what + * `check-coverage` compares the launch against. Building them without writing + * them would produce a roster no gate could check. + */ +export function buildFanOutRoster( + report: PlanReport, + planPath: string, + rules?: string, +): WorkflowAgentSpec[] { + const plan = report as RosterPlan; + + refuseBlockedFanOut(plan); + + // The state of the shared review worktree AT BUILD TIME, probed the same + // way the hand-launched path does (agent-prompt's handler) and threaded + // into every build below: both paths go through `buildLaunch`, and its + // byte-parity invariant covers the residue evidence block too. A probe + // only one path ran would leave the other's briefs silent about a dirty + // tree — every dispatched agent then reads foreign files as the PR's code, + // and no gate catches it, because each path records its own prompts and + // coverage compares like with like. + const residue = worktreeResidueOf(report); + if (residue.unmeasured) { + writeStderrLine( + `warning: could not measure whether the review worktree is clean (reason: ` + + `${inertPath(residue.unmeasured)}). Every brief built by this call says so; an unmeasured tree is ` + + 'not a clean one.', + ); + } + if (residue.paths.length > 0) { + const unlisted = residue.total - residue.paths.length; + writeStderrLine( + `warning: the review worktree carries changes its commit does not: ${residue.paths + .map(inertPath) + .join(', ')}` + + (unlisted > 0 + ? ` (and ${unlisted} more — this list is capped; \`git status --porcelain --untracked-files=all\` has the full set)` + : '') + + '. Every brief built by this call names those paths and says a defect confined to them ' + + 'is not a finding; the code-reading ones also carry the rule that evidence comes from ' + + '`git show HEAD:`. Restore them BEFORE dispatching the workflow — a probe left in the ' + + "shared tree reads to an auditor as the PR's own code, and to Agent 7's build and test " + + "run as the PR's own failure — and then RE-RUN this same command so the script is rebuilt: " + + 'the suppression above is baked into the briefs it writes, so dispatching it after a ' + + 'restore tells every agent to drop findings in a file that is by then exactly the ' + + "PR's code. (The prompt records are overwritten, so a rebuild is what the delivery " + + 'check compares against.)', + ); + } + + return requiredAgents(plan).map((req): WorkflowAgentSpec => { + const { key, prompt } = buildLaunch( + report, + planPath, + // `role: 'chunk'` reaches this only from a territory fan-out, refused + // above; `buildLaunch`'s own chunk branch handles it if that ever + // changes, so there is nothing to assert here. + req.role === 'chunk' + ? { chunk: req.chunk } + : { role: req.role, file: req.file }, + rules, + residue, + ); + // The same guard `--roster` makes, for the same reason: the roster is + // what coverage holds the run to, and the key is what the brief was + // written under. If they ever disagree, every delivery check downstream + // reads "brief never reached an agent" on a run that did everything right. + if (key !== req.key) { + throw new Error( + `emit-workflow: built "${key}" where the roster requires "${req.key}" ` + + '— the agent could never be matched to the requirement. This is a ' + + 'bug in the CLI, not in the call.', + ); + } + // What was handed out, at a path derived from the plan. `check-coverage` + // compares this against the prompt the harness recorded the agent being + // launched with; an unrecorded launch reads as an agent that never ran. + recordPrompt(planPath, key, prompt); + return { key, prompt }; + }); +} + +function runEmitWorkflow(args: EmitWorkflowArgs): void { + let report: PlanReport; + try { + report = JSON.parse(readFileSync(args.plan, 'utf8')) as PlanReport; + } catch (err) { + throw new Error( + `emit-workflow: cannot read the plan ${args.plan}: ${(err as Error).message}`, + ); + } + + // Same refusal as `agent-prompt`, for the same reason: a rules path that + // does not resolve would silently review without the project rules the run + // was told to enforce. + let rules: string | undefined; + if (args.rules) { + try { + rules = readFileSync(args.rules, 'utf8'); + } catch (err) { + throw new Error( + `emit-workflow: cannot read the rules ${args.rules}: ` + + `${(err as Error).message}. Omit --rules if this review has none.`, + ); + } + } + + // Refused BEFORE the session directory exists: a blocked plan writes + // nothing at all, and the directory `ensureWritableReviewWorkflowsDir` + // creates would otherwise outlive the refusal as an empty tree a later + // sweep finds. The builder repeats the same check for direct callers. + refuseBlockedFanOut(report as RosterPlan); + + // Resolved BEFORE anything is written: the env contract can be missing, and + // a roster whose briefs and records were written for a script that then had + // nowhere to go would read to the coverage gate as a launched fan-out that + // returned nothing. + const scriptPath = reviewWorkflowScriptPath(args.plan); + // Validated for the same reason, one step further: a symlinked root or + // session directory would carry the write outside the trusted root, so the + // refusal must land before the briefs and records exist too. Also creates + // the session directory, so the write below finds it. + ensureWritableReviewWorkflowsDir(); + + const agents = buildFanOutRoster(report, args.plan, rules); + + const temporaryPath = `${scriptPath}.${randomUUID()}.tmp`; + // Temp-and-rename, and the write is inside the cleanup too: a failure + // mid-write (ENOSPC, EIO) throws AFTER the temp file exists, and a finally + // that only covered the rename would leave the half-written shape behind. + // The rename also replaces an existing entry at the target — a symlink + // planted there is replaced, never written through. + try { + // The worktree pin travels with the roster. `plan.worktreePath` is the + // same value `agent-prompt --roster` tells the orchestrator to put in + // `working_dir` on every Agent call, so both paths pin the same tree by + // construction rather than by two conventions kept in step. + const planWorktree = (report as RosterPlan).worktreePath; + const worktreePath = + typeof planWorktree === 'string' ? planWorktree : undefined; + writeFileSync( + temporaryPath, + buildReviewWorkflowScript(agents, worktreePath), + { encoding: 'utf8', flag: 'wx' }, + ); + renameSync(temporaryPath, scriptPath); + } finally { + rmSync(temporaryPath, { force: true }); + } + + // One path and a count. Nothing here is a prompt: the prompts are inside the + // script, which nobody is asked to read, retype or relay — which is the + // property this command exists for. + writeStdoutLine( + `${agents.length} agents required. The fan-out is a workflow: make ONE ` + + 'Workflow call with the scriptPath below and no `args`, and do not ' + + 'build agent calls by hand for this step.', + ); + writeStdoutLine(`scriptPath: ${resolve(scriptPath)}`); +} + +export const emitWorkflowCommand: CommandModule = { + command: 'emit-workflow', + describe: + 'Emit the Step 3A fan-out as a runnable workflow script, so the roster ' + + 'is dispatched by code instead of hand-launched', + builder: (yargs) => + yargs + .option('plan', { + type: 'string', + demandOption: true, + describe: 'Path to the plan report from Step 1', + }) + .option('rules', { + type: 'string', + describe: + 'Path to the project rules from Step 2, if the project has any', + }), + handler: (argv) => { + runEmitWorkflow(argv as unknown as EmitWorkflowArgs); + }, +}; diff --git a/packages/cli/src/commands/review/lib/budget.ts b/packages/cli/src/commands/review/lib/budget.ts index 43dd674992a..9b78d7b3432 100644 --- a/packages/cli/src/commands/review/lib/budget.ts +++ b/packages/cli/src/commands/review/lib/budget.ts @@ -361,7 +361,7 @@ export function cappedRoundTier( * the sweep, the tool budget) have a safe floor to land on; the tier has no * such floor — landing on `0` there means "small diff", the costliest cap. */ -function usableLineCount(v: unknown): v is number { +export function usableLineCount(v: unknown): v is number { return typeof v === 'number' && Number.isFinite(v) && v >= 0; } diff --git a/packages/cli/src/commands/review/lib/paths.ts b/packages/cli/src/commands/review/lib/paths.ts index 96f40efab74..350aaa9923f 100644 --- a/packages/cli/src/commands/review/lib/paths.ts +++ b/packages/cli/src/commands/review/lib/paths.ts @@ -9,9 +9,16 @@ // preferences resolve under Storage's project directory. Use `path.join` // rather than string concatenation so Windows backslashes are produced. -import { existsSync, realpathSync, statSync } from 'node:fs'; +import { createHash } from 'node:crypto'; +import { + existsSync, + lstatSync, + mkdirSync, + realpathSync, + statSync, +} from 'node:fs'; import { isAbsolute, join, relative, resolve, sep } from 'node:path'; -import { Storage } from '@qwen-code/qwen-code-core'; +import { sanitizeFilenameComponent, Storage } from '@qwen-code/qwen-code-core'; import { safeTarget } from '../../../utils/paths.js'; /** @@ -42,6 +49,160 @@ export const REVIEW_TMP_DIR = join('.qwen', 'tmp'); export const REVIEWS_DIR = join('.qwen', 'reviews'); export const REVIEW_CACHE_DIR = join('.qwen', 'review-cache'); +/** + * Where a generated review fan-out script has to live. + * + * Not a choice: `Workflow({scriptPath})` loads through + * `readWorkflowFileSecurely`, which realpaths the file and accepts only the + * two saved-workflow directories and the generated-scripts root, + * `Storage.getGeneratedWorkflowsDir()` = `/workflows/generated`. + * The saved directories are out: every `.js` in them is also a `/` + * slash command, and a review's fan-out has no business in the user's + * command namespace. The generated root is reached through the same env the + * harness exports for the transcript readers — `QWEN_CODE_PROJECT_DIR` is + * `storage.getProjectDir()` of the session that will dispatch the script — + * so the writer and the loader compute the same directory by construction. + * Nested one level per session, so a session's scripts can be swept as a + * unit and two sessions reviewing in one project never share a file. + */ +export const GENERATED_WORKFLOWS_SUBDIR = join('workflows', 'generated'); + +/** Subdirectory of the generated root that review scripts live under. */ +export const REVIEW_WORKFLOWS_SUBDIR = 'review'; + +/** Filename prefix for generated fan-out scripts. */ +export const REVIEW_WORKFLOW_PREFIX = 'qwen-review-'; + +/** + * Why a generated script has nowhere to go. Never conflated with a bad plan: + * the env contract is the harness's, not the caller's. + */ +export class GeneratedWorkflowDirUnavailableError extends Error {} + +function projectDirFromEnv(env: NodeJS.ProcessEnv): string { + const projectDir = env['QWEN_CODE_PROJECT_DIR']?.trim(); + if (!projectDir) { + throw new GeneratedWorkflowDirUnavailableError( + 'the CLI did not export QWEN_CODE_PROJECT_DIR, so there is no directory ' + + 'the Workflow tool would load a generated script from. Run this ' + + 'command from inside a qwen session.', + ); + } + return projectDir; +} + +/** + * The per-session directory name. The sanitized prefix keeps the directory + * readable and swept by the same rules as the harness's transcript dirs, + * but sanitizing is lossy — `sess.1` and `sess_1` both flatten to + * `sess_1` — so the digest of the RAW id is what keeps two concurrent + * sessions apart: without it they would select the same script target for + * the same plan, and the later atomic rename would dispatch one session's + * roster, rules, and worktree pin to the other. + */ +function reviewSessionDirName(session: string): string { + const digest = createHash('sha256').update(session).digest('hex').slice(0, 8); + return `${sanitizeFilenameComponent(session)}-${digest}`; +} + +/** + * The directory this session's generated review scripts live in: + * `$QWEN_CODE_PROJECT_DIR/workflows/generated/review/`. + * + * Read from the environment, never from an argument, for the same reason the + * transcript readers do: a path the model can choose is a path the model can + * point somewhere the loader will refuse. The session component is sanitized + * exactly as the harness sanitizes its transcript directory, so a session id + * carrying a dot lands in a directory that exists. + */ +export function reviewWorkflowsDir( + env: NodeJS.ProcessEnv = process.env, +): string { + const session = env['QWEN_CODE_SESSION_ID']?.trim(); + return join( + projectDirFromEnv(env), + GENERATED_WORKFLOWS_SUBDIR, + REVIEW_WORKFLOWS_SUBDIR, + session ? reviewSessionDirName(session) : 'no-session', + ); +} + +/** + * The writer half of the loader's canonical-containment policy. + * `readWorkflowFileSecurely` realpaths the script and refuses one outside + * the trusted roots — and refuses a symlinked root outright — so a writer + * that follows a link writes the script (embedding every review prompt) + * where the loader will not read it, outside the root it was meant to stay + * inside. Refuses a symlinked directory from the generated root down to the + * session dir, creates the session dir, then proves the canonical session + * dir stays under the canonical root. Call BEFORE building briefs or prompt + * records: delivery evidence for a script that then has nowhere safe to go + * would read to the coverage gate as a launched fan-out. + */ +export function ensureWritableReviewWorkflowsDir( + env: NodeJS.ProcessEnv = process.env, +): string { + const projectDir = projectDirFromEnv(env); + const dir = reviewWorkflowsDir(env); + const root = join(projectDir, GENERATED_WORKFLOWS_SUBDIR); + for (const component of [root, join(root, REVIEW_WORKFLOWS_SUBDIR), dir]) { + let isLink = false; + try { + isLink = lstatSync(component).isSymbolicLink(); + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err; + break; // absent — the mkdir below creates it as a real directory + } + if (isLink) { + throw new Error( + `refusing to write a generated review script through the symlinked ` + + `directory '${component}' — it would land outside the ` + + 'generated-workflows root the Workflow loader trusts.', + ); + } + } + mkdirSync(dir, { recursive: true }); + const realDir = realpathSync(dir); + const realRoot = realpathSync(root); + if (realDir !== realRoot && !realDir.startsWith(realRoot + sep)) { + throw new Error( + `refusing to write a generated review script: the canonical session ` + + `directory '${realDir}' escapes the canonical generated-workflows ` + + `root '${realRoot}'.`, + ); + } + return dir; +} + +/** + * The generated fan-out script for one plan. + * + * Named by a digest of the plan path so two reviews running in one session + * do not overwrite each other's script, and so re-running `emit-workflow` + * for the same review replaces its own file rather than accumulating. + */ +export function reviewWorkflowScriptPath( + planPath: string, + env: NodeJS.ProcessEnv = process.env, +): string { + const resolved = resolve(planPath); + // Canonicalize an EXISTING plan before hashing: macOS spells the same + // file `/var/...` and `/private/var/...`, and the loader canonicalizes + // with realpath too — hashing the raw spelling would name one plan two + // scripts (and break the identity a relative vs absolute path must keep). + let canonical = resolved; + try { + canonical = realpathSync(resolved); + } catch { + // Not on disk — nothing to canonicalize; the read fails on its own terms. + } + const digest = createHash('sha256') + .update(canonical) + .digest('hex') + .slice(0, 10); + return join(reviewWorkflowsDir(env), `${REVIEW_WORKFLOW_PREFIX}${digest}.js`); +} + /** * Filename prefix for review-worktree lease files under `REVIEW_TMP_DIR`. * Lives here, not in `review-worktree-lease.ts`, because the review diff --git a/packages/cli/src/commands/review/test-efficacy.test.ts b/packages/cli/src/commands/review/test-efficacy.test.ts index b7f1772b01e..7a90f3bc792 100644 --- a/packages/cli/src/commands/review/test-efficacy.test.ts +++ b/packages/cli/src/commands/review/test-efficacy.test.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { execFileSync, spawnSync } from 'node:child_process'; import { replacementMutantsOf, @@ -34,6 +34,7 @@ import { committedSymlinkProbes, } from './test-efficacy.js'; import { isolateHostGitConfig } from './lib/test-utils.js'; +import { sanitizedGitEnv } from './lib/worktree.js'; import { mkdtempSync, mkdirSync, @@ -46,6 +47,23 @@ import { import { tmpdir } from 'node:os'; import { basename, join } from 'node:path'; +// Hermetic git config for every fixture in this file, the same discipline as +// the sibling integration suites: `sanitizedGitEnv` strips env redirects, but +// the file scopes stay reachable through $HOME/.gitconfig and /etc/gitconfig. +// A persistent runner's ambient `core.sparseCheckout` flipped the very +// semantics the skip-worktree refusal pins — on git 2.39 an active sparse flag +// makes `checkout --force` clear the bit and read clean — so the refusal never +// fired and the run died later on a missing vitest. +let gitIsolation: ReturnType; + +beforeEach(() => { + gitIsolation = isolateHostGitConfig(); +}); + +afterEach(() => { + gitIsolation.dispose(); +}); + // The real root `package.json` workspace list. const GLOBS = [ 'packages/*', @@ -260,7 +278,12 @@ function asCheckout(dir: string): void { 'core.hooksPath=/dev/null/no-hooks', ...args, ], - { cwd: dir, encoding: 'utf8' }, + // Sanitized like the guards these fixtures exist to provoke: an + // ambient GIT_INDEX_FILE (observed on a persistent runner) makes + // add/commit stage into ANOTHER index, and the bit a later + // update-index sets locally can never reproduce the state under + // test — the fixture must build the same index the guard reads. + { cwd: dir, encoding: 'utf8', env: sanitizedGitEnv() }, ); git('init', '-q', '-b', 'main', '--template=', '.'); git('add', '-A'); @@ -452,8 +475,14 @@ describe('committedSymlinkProbes', () => { const repo = mkdtempSync(join(tmpdir(), 'qwen-symlink-mode-')); const isolation = isolateHostGitConfig(); try { + // Sanitized for the reason asCheckout is: the function under test + // reads with a sanitized env, so the fixture must build with one. const g = (...args: string[]) => - execFileSync('git', args, { cwd: repo, encoding: 'utf8' }).trim(); + execFileSync('git', args, { + cwd: repo, + encoding: 'utf8', + env: sanitizedGitEnv(), + }).trim(); g('init', '-q', '-b', 'main'); g('config', 'user.email', 't@t.t'); g('config', 'user.name', 't'); @@ -552,8 +581,15 @@ describe('restoreProbeTreeTracked, through runOneMutant', () => { // about the oracle, not about that. writeFileSync(join(dir, 'b.ts'), 'export const b = 1;\n'); asCheckout(dir); + // With the same sanitized env the guard's own git calls use: an + // ambient discovery redirect (a GIT_INDEX_FILE on a persistent runner) + // writes the bit into ANOTHER index than the guard reads, the refusal + // this test pins never fires, and the mutant run dies later on a + // missing vitest instead — the incident this file's isolation + // discipline exists for. execFileSync('git', ['update-index', '--skip-worktree', 'a.ts'], { cwd: dir, + env: sanitizedGitEnv(), }); writeFileSync(join(dir, 'a.ts'), 'MUTANT\n'); diff --git a/packages/cli/src/commands/review/workflow-script.test.ts b/packages/cli/src/commands/review/workflow-script.test.ts new file mode 100644 index 00000000000..31f23dad8e0 --- /dev/null +++ b/packages/cli/src/commands/review/workflow-script.test.ts @@ -0,0 +1,338 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect } from 'vitest'; +import * as vm from 'node:vm'; +import { REVIEW_BUILTIN_SUBAGENT_TYPE } from '@qwen-code/qwen-code-core'; +import { + buildReviewWorkflowScript, + FAN_OUT_BODY, + type WorkflowAgentSpec, +} from './workflow-script.js'; + +// The generated script is executed here, not merely parsed — which is the +// reason the logic is a fixed constant and only the roster is spliced in. +// Every case below runs the REAL output of `buildReviewWorkflowScript`, so a +// roster that serialized wrong would fail these as surely as a broken loop. +// +// The harness mirrors the runtime's execution shape (workflow-sandbox.ts) +// rather than importing it — `createWorkflowSandbox` is not exported from +// the core package, and exporting it for this test would be a cross-package +// change for no gain in what is being checked: +// - the meta block is STRIPPED, never executed: the sandbox parses it as +// a pure literal, so no live `meta` binding may reach the body; +// - the body runs inside the runtime's strict-mode async IIFE wrapper, so +// an undeclared assignment throws here like it throws at dispatch; +// - the body runs in a vm context binding only the globals this harness +// stands in for — a host-only global like `setTimeout` fails here like +// it fails in the sandbox; +// - the `agent` stub applies the runtime's option gates. +// What it does not mirror: the sandbox's throwing Date/Math replacements +// (a fresh vm context has a real Date, so the determinism case asserts on +// the source instead) and the meta literal parser (not exported from the +// core package; the meta block's purity is asserted on the source below). +// +// The runtime's option allowlist (workflow-sandbox.ts): a key outside it is +// a typo the sandbox refuses at dispatch, so it must be refused here. +const KNOWN_AGENT_OPTS = [ + 'label', + 'phase', + 'schema', + 'model', + 'isolation', + 'agentType', + 'stallMs', + 'workingDir', +]; + +async function runScript( + script: string, + dispatch: (prompt: string, opts: unknown) => Promise, +): Promise<{ + result: unknown; + dispatched: Array<{ prompt: string; opts: unknown }>; + logs: string[]; + phases: string[]; +}> { + const dispatched: Array<{ prompt: string; opts: unknown }> = []; + const logs: string[] = []; + const phases: string[] = []; + + // The runtime's gates: an unknown option is refused, an empty workingDir + // is not "no pin", and workingDir together with isolation is a + // contradiction about who owns the directory's lifetime. + const agent = async (prompt: string, opts: Record) => { + const options = opts ?? {}; + for (const key of Object.keys(options)) { + if (!KNOWN_AGENT_OPTS.includes(key)) { + throw new Error(`agent({${key}}): unknown option.`); + } + } + if (options['workingDir'] !== undefined) { + if ( + typeof options['workingDir'] !== 'string' || + options['workingDir'].trim().length === 0 + ) { + throw new Error('agent({workingDir}): must be a non-empty string.'); + } + if (options['isolation'] !== undefined) { + throw new Error( + 'agent({workingDir, isolation}): incompatible options.', + ); + } + } + dispatched.push({ prompt, opts: options }); + return dispatch(prompt, options); + }; + // Mirrors the runtime's errors-as-data contract: a thunk that rejects + // becomes a `null` element, and the call itself never rejects. + const parallel = async (thunks: Array<() => Promise>) => + Promise.all(thunks.map((t) => t().catch(() => null))); + const phase = (title: string): void => { + phases.push(title); + }; + const log = (message: string): void => { + logs.push(message); + }; + + // Strip the meta block exactly like the runtime does — it is parsed as a + // pure literal there, never executed, so the body must run with no `meta` + // binding either. + const body = script.slice(script.indexOf('\n};') + '\n};'.length); + // The runtime's wrapper: an async IIFE under 'use strict'. + const wrapped = `(async () => {'use strict';\n${body}\n})()`; + const context = vm.createContext({ agent, parallel, phase, log }); + const result: unknown = await new vm.Script(wrapped).runInContext(context); + return { result, dispatched, logs, phases }; +} + +const AGENTS: WorkflowAgentSpec[] = [ + { key: '1a', prompt: 'PROMPT-1a' }, + { key: '2', prompt: 'PROMPT-2' }, + { key: '7', prompt: 'PROMPT-7' }, +]; + +describe('the generated Step 3A fan-out script', () => { + it('opens with a meta block the sandbox will accept as a pure literal', () => { + const script = buildReviewWorkflowScript(AGENTS); + expect(script.startsWith('export const meta = {')).toBe(true); + const metaBlock = script.slice(0, script.indexOf('\n};') + 3); + // `meta` is read before anything executes, so it may hold no variable, + // call, spread or interpolation. + expect(metaBlock).not.toMatch(/\$\{|\bfunction\b|\.\.\./); + }); + + it('uses no non-deterministic builtin — the sandbox throws on them', () => { + const script = buildReviewWorkflowScript(AGENTS); + expect(script).not.toContain('Date.now'); + expect(script).not.toContain('Math.random'); + expect(script).not.toContain('new Date'); + // The sandbox's safeDate throws on these forms too, and the vm harness + // cannot stand in for it — a fresh context carries a working Date. + expect(script).not.toContain('Date.parse'); + expect(script).not.toContain('Date.UTC'); + expect(script).not.toMatch(/\bDate\s*\(/); + }); + + it('dispatches every agent in the roster, once each, in one phase', async () => { + const { result, dispatched, phases } = await runScript( + buildReviewWorkflowScript(AGENTS), + async (prompt) => `said:${prompt}`, + ); + expect(dispatched).toHaveLength(3); + expect(phases).toEqual(['Review']); + expect((result as { rosterSize: number }).rosterSize).toBe(3); + expect((result as { missingRoles: string[] }).missingRoles).toEqual([]); + }); + + it('passes each prompt through untouched, having survived serialization', async () => { + // The prompts are values the CLI computed and this file carries. The + // script's contract is that it does not read, trim, wrap or annotate + // them — the property the whole change exists to make structural. The + // awkward characters are here because the roster reaches the script + // through JSON embedded in JavaScript source, where a stray backtick, + // backslash or `${` would previously have ended the literal. + const tricky: WorkflowAgentSpec[] = [ + { + key: 'x', + prompt: 'back`tick ${notInterpolated} \\ "quote" \n newline', + }, + { key: 'y', prompt: " and 'single' quotes" }, + ]; + const { dispatched } = await runScript( + buildReviewWorkflowScript(tricky), + async (prompt) => `said:${prompt}`, + ); + expect(dispatched.map((d) => d.prompt)).toEqual([ + 'back`tick ${notInterpolated} \\ "quote" \n newline', + " and 'single' quotes", + ]); + }); + + it('asks for the same subagent type the hand-launched path requires', async () => { + // The hand-launched path is mandated to set `subagent_type: + // "review-agent"` (SKILL.md, TYPE_NOTE, and the registry's explicit tool + // list). Dispatching any other type — including the inherit-everything + // `general-purpose` default — runs a different agent over identical + // prompts and makes the A/B between the two paths unreadable. Both + // dispatch branches are exercised: the worktree-pinned shape and the + // shape a review without a worktree takes. + for (const script of [ + buildReviewWorkflowScript(AGENTS), + buildReviewWorkflowScript(AGENTS, '/tmp/review-pr-42'), + ]) { + const { dispatched } = await runScript( + script, + async (prompt) => `said:${prompt}`, + ); + for (const d of dispatched) { + expect((d.opts as { agentType: string }).agentType).toBe( + REVIEW_BUILTIN_SUBAGENT_TYPE, + ); + } + expect( + dispatched.map((d) => (d.opts as { label: string }).label), + ).toEqual(['1a', '2', '7']); + } + }); + + // The pin is the whole reason a worktree review may take this path at all. + // Without it every dispatched agent reads the user's main checkout and + // reports findings that describe the wrong tree — plausibly, and at length. + it('pins every agent to the review worktree when the plan has one', async () => { + const { dispatched } = await runScript( + buildReviewWorkflowScript(AGENTS, '/tmp/review-pr-42'), + async (prompt) => `said:${prompt}`, + ); + expect(dispatched).toHaveLength(3); + for (const d of dispatched) { + expect((d.opts as { workingDir?: string }).workingDir).toBe( + '/tmp/review-pr-42', + ); + // Mutually exclusive with the pin; passing both fails every dispatch. + expect((d.opts as { isolation?: unknown }).isolation).toBeUndefined(); + } + }); + + // `agent({workingDir})` refuses an empty string rather than reading it as + // "no pin", so a review with no worktree must omit the key entirely — not + // pass null, '' or undefined under it. + it('omits the pin entirely for a review with no worktree', async () => { + for (const absent of [undefined, '']) { + const { dispatched } = await runScript( + buildReviewWorkflowScript(AGENTS, absent), + async (prompt) => `said:${prompt}`, + ); + for (const d of dispatched) { + expect('workingDir' in (d.opts as object)).toBe(false); + } + } + }); + + // A required agent that died must fail the step, not shrink it: the roster + // is the set of dimensions the plan says this review needs, and a result + // missing one reads downstream as a complete review that lacks a + // dimension. The coverage gate cannot see this class — it asserts + // launches, and a dead agent WAS launched — so the script itself is the + // consumer, and names the missing role. + it('rejects, naming the agent, when a dispatched agent returned nothing', async () => { + // parallel() reports a dead dispatch as a null element rather than + // throwing. A script that ignored that would return a short findings list + // with no sign a dimension went unreviewed. + await expect( + runScript(buildReviewWorkflowScript(AGENTS), async (prompt) => { + if (prompt === 'PROMPT-2') throw new Error('agent died'); + return `said:${prompt}`; + }), + ).rejects.toThrow(/required agents failed to deliver \(2\)/); + }); + + it('rejects on an undefined return, not a shorter finding set', async () => { + await expect( + runScript(buildReviewWorkflowScript(AGENTS), async (prompt) => + prompt === 'PROMPT-7' ? undefined : `said:${prompt}`, + ), + ).rejects.toThrow(/required agents failed to deliver \(7\)/); + }); + + it('rejects when a result strips to empty, not as delivered', async () => { + // A GOAL-mode dispatch can finish with visible text that strips to + // nothing — a scratchpad-only final message, or a cutoff mid-analysis. + // It fulfilled, so the null check passes it; counting it delivered would + // assert a complete fan-out while one dimension contributed nothing, and + // Step 3D cannot catch it — the agent was launched, so a transcript + // exists. + for (const empty of ['', ' ']) { + await expect( + runScript(buildReviewWorkflowScript(AGENTS), async (prompt) => + prompt === 'PROMPT-2' ? empty : `said:${prompt}`, + ), + ).rejects.toThrow(/required agents failed to deliver \(2\)/); + } + }); + + it('carries each agent return back under its roster key', async () => { + const { result } = await runScript( + buildReviewWorkflowScript(AGENTS), + async (prompt) => `said:${prompt}`, + ); + expect( + (result as { delivered: Array<{ key: string; text: string }> }).delivered, + ).toEqual([ + { key: '1a', text: 'said:PROMPT-1a' }, + { key: '2', text: 'said:PROMPT-2' }, + { key: '7', text: 'said:PROMPT-7' }, + ]); + }); + + // A fan-out where nothing came back is the all-missing case of the same + // fail-closed rule: a failed step, never an empty result the caller could + // aggregate over a diff no agent read. But the partial-failure remedy — + // re-emit and dispatch again — loops here: a rebuild writes the identical + // script with the identical baked-in pin, measured against a real bad-pin + // run where every dispatch was rejected before the first request. The + // message must name the dispatch, not prescribe the loop. + it('throws when every agent failed, naming the dispatch rather than a re-emit', async () => { + const run = runScript(buildReviewWorkflowScript(AGENTS), async () => { + throw new Error('all dead'); + }); + await expect(run).rejects.toThrow( + /every agent failed to deliver \(1a, 2, 7\)/, + ); + const message = await run.catch((err: Error) => err.message); + expect(message).toContain('writes the identical script'); + expect(message).not.toContain('and dispatch again'); + }); + + it('throws on an empty roster rather than reporting a clean review', async () => { + // `buildReviewWorkflowScript([])` is not reachable from the command + // today, but the guard is what makes "nobody ran" impossible to read as + // "nothing to report" if it ever becomes reachable. + await expect( + runScript(buildReviewWorkflowScript([]), async () => 'unused'), + ).rejects.toThrow(/roster is empty/); + }); + + it('splices the subagent type as a literal rather than spelling it', () => { + // The literal comes from the same constant the hand-launched path's + // TYPE_NOTE reads, so a rename of the builtin agent moves both. + const script = buildReviewWorkflowScript(AGENTS); + expect(script).toContain( + `const AGENT_TYPE = ${JSON.stringify(REVIEW_BUILTIN_SUBAGENT_TYPE)};`, + ); + }); + + it('keeps the dispatch logic out of the generated half', () => { + // Only the roster literal varies between reviews. If generation ever + // starts emitting logic, this fails — and the executable guarantee above + // stops covering what actually ships. + const script = buildReviewWorkflowScript(AGENTS); + expect(script.endsWith(FAN_OUT_BODY)).toBe(true); + const generated = script.slice(0, script.length - FAN_OUT_BODY.length); + expect(generated).not.toContain('parallel('); + expect(generated).not.toContain('agent('); + }); +}); diff --git a/packages/cli/src/commands/review/workflow-script.ts b/packages/cli/src/commands/review/workflow-script.ts new file mode 100644 index 00000000000..465685bb02b --- /dev/null +++ b/packages/cli/src/commands/review/workflow-script.ts @@ -0,0 +1,201 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// The Step 3A fan-out, as a workflow script. +// +// The script has two parts and only one of them varies. `FAN_OUT_BODY` is a +// fixed constant — the dispatch loop, the accounting, the fail-closed guards — +// and `buildReviewWorkflowScript` splices three literals in front of it: the +// roster the CLI computed, the worktree pin, and the subagent type. No logic +// is generated, only data, so the part that can be wrong is the part a test +// can execute. +// +// Why the roster is baked in rather than passed as `args`: the Workflow tool +// takes `args` as INLINE JSON ("Pass actual JSON, not a stringified value") +// and the vm sandbox has no filesystem — its globals are `agent`, `parallel`, +// `pipeline`, `phase`, `log`, `console`, `args`, `budget`, `workflow`, and +// nothing that opens a file. So an args-carried roster is a roster the model +// has to retype into its tool call, which is the failure this whole change +// exists to remove. Baked in, the model's call carries one path and no +// payload. +// +// Sandbox constraints this must respect (workflow-sandbox.ts): +// - `meta` must be the first statement and a pure literal. +// - `Date.now()` / `Math.random()` / `new Date()` throw — scripts are +// deterministic so a resume can replay them. +// - `parallel()` takes THUNKS and degrades a failed dispatch to a `null` +// element rather than rejecting. +// +// No template literals in the body below: this file stores script source +// inside host template literals, so a backtick would end one and a `${` would +// splice host state into the script. String concatenation instead, +// deliberately. + +import { REVIEW_BUILTIN_SUBAGENT_TYPE } from '@qwen-code/qwen-code-core'; + +/** One agent, as the generated script's `AGENTS` literal carries it. */ +export interface WorkflowAgentSpec { + /** The roster key — `check-coverage` looks the agent up under this. */ + key: string; + /** The launch prompt, verbatim from `buildLaunch`. Passed, never built. */ + prompt: string; +} + +/** + * The invariant half of the script: everything after the literals. + * + * Exported so its behaviour can be executed and asserted directly, rather + * than inferred from the text of a generated file. + */ +export const FAN_OUT_BODY = ` +if (!Array.isArray(AGENTS) || AGENTS.length === 0) { + // An empty roster is not a clean review, it is a review that dispatched + // nobody. Returning normally here would hand the caller zero findings and + // zero missing roles, which reads as "nothing to report". + throw new Error( + 'review fan-out: the generated roster is empty — no agent would run. ' + + 'Re-run \\'qwen review emit-workflow\\'.', + ); +} + +phase('Review'); +log(AGENTS.length + ' agents required by the plan'); + +// One thunk per required agent, dispatched together. The roster is data the +// CLI computed and wrote into this file; this loop cannot shorten it, and +// there is no branch in which an agent is skipped. +// +// AGENT_TYPE is the subagent type the hand-launched path sets as +// subagent_type, so workflow dispatch runs the same agent, with the same +// explicit tool list, over identical prompts; omitting it would substitute +// the runtime's terse default persona instead. +// +// WORKING_DIR is the review's worktree, or null for a review that has none. +// It is the workflow equivalent of the \`working_dir\` the hand-launched path +// sets on every Agent call: without it a dispatched agent reads the user's +// main checkout and describes the wrong tree. Passed only when there is one, +// because \`agent({workingDir})\` refuses an empty string rather than treating +// it as absent. Never together with \`isolation\` — the worktree already +// exists, and the two options are mutually exclusive. +const returns = await parallel( + AGENTS.map((a) => () => + agent( + a.prompt, + WORKING_DIR + ? { + label: a.key, + phase: 'Review', + agentType: AGENT_TYPE, + workingDir: WORKING_DIR, + } + : { + label: a.key, + phase: 'Review', + agentType: AGENT_TYPE, + }, + ), + ), +); + +// parallel() reports a failed dispatch as a null element rather than +// throwing, and a result whose visible text strips to empty delivered just +// as little — collect both by name. An agent silently missing from the +// fan-out is the one regression this path must not introduce, and the +// coverage gate cannot see this class — it asserts launches, and a +// cap-killed agent WAS launched — so this script is the consumer: any +// missing role fails the step below, instead of reaching the caller as a +// shorter finding set. +const delivered = []; +const missingRoles = []; +for (let i = 0; i < AGENTS.length; i++) { + const value = returns[i]; + if ( + value === null || + value === undefined || + (typeof value === 'string' && value.trim() === '') + ) { + missingRoles.push(AGENTS[i].key); + } else { + delivered.push({ key: AGENTS[i].key, text: value }); + } +} + +// Fail closed on ANY missing required agent, not only when every one died: +// the roster is the set of dimensions the plan says this review needs, and +// a shortened delivered list would let the caller aggregate a review that +// silently lacks one of them. +if (missingRoles.length > 0) { + if (missingRoles.length === AGENTS.length) { + // Nothing delivered is not one dead agent — it is the dispatch itself: + // a pin the runtime rejects fails every dispatch before the first + // request, an exhausted runtime nulls every one of them. The roster and + // the pin are baked into this file, so the re-emit the partial-failure + // message prescribes regenerates the identical script and loops whoever + // follows it; name the dispatch, not the emitter. + throw new Error( + 'review fan-out: every agent failed to deliver (' + + missingRoles.join(', ') + + '). No agent delivered anything, so the failure is the dispatch ' + + 'itself, not one agent — the roster and the worktree pin are ' + + 'baked into this file, and re-running \\'qwen review emit-workflow\\' ' + + 'writes the identical script. Fix what the dispatch reads (the ' + + 'worktree pin, the runtime) and dispatch this same script again.', + ); + } + throw new Error( + 'review fan-out: required agents failed to deliver (' + + missingRoles.join(', ') + + '). Re-run \\'qwen review emit-workflow\\' and dispatch again.', + ); +} + +return { + rosterSize: AGENTS.length, + delivered: delivered, + missingRoles: missingRoles, +}; +`; + +/** + * The full script for one review: `meta`, the roster literal, the worktree + * pin, the subagent type, and the body. + * + * `meta.phases` mirrors the skill's step name so the run's progress display + * reads like the step it is executing. + * + * `worktreePath` is the review's worktree (`plan.worktreePath`), or omitted + * for a review that has none. Baked in as a literal for the same reason the + * roster is: the sandbox has no filesystem and the model's call carries one + * path and no payload, so anything the script needs has to be in the script. + */ +export function buildReviewWorkflowScript( + agents: readonly WorkflowAgentSpec[], + worktreePath?: string, +): string { + // Only the two fields the script reads are serialized. A field written here + // and read nowhere would be a claim the file does not keep. + const roster = agents.map((a) => ({ key: a.key, prompt: a.prompt })); + // `null`, not `undefined`: the script branches on truthiness, and a plan + // that carried an empty string must reach the script as "no worktree" + // rather than as a pin `agent()` would then refuse. + const pin = + typeof worktreePath === 'string' && worktreePath ? worktreePath : null; + return ( + `export const meta = {\n` + + ` name: 'review-step-3a',\n` + + ` description: 'Review Step 3A: launch every agent the plan requires, in one fan-out',\n` + + ` phases: [{ title: 'Review', detail: 'one agent per required role' }],\n` + + `};\n\n` + + `// Written by \`qwen review emit-workflow\`. The roster below is the one\n` + + `// \`check-coverage\` holds this run to; editing it makes the two disagree.\n` + + `const AGENTS = ${JSON.stringify(roster, null, 2)};\n` + + `// The worktree every agent is pinned to, or null when the review has none.\n` + + `const WORKING_DIR = ${JSON.stringify(pin)};\n` + + `// The subagent type the hand-launched path sets on every Agent call.\n` + + `const AGENT_TYPE = ${JSON.stringify(REVIEW_BUILTIN_SUBAGENT_TYPE)};\n` + + FAN_OUT_BODY + ); +}