diff --git a/packages/cli/src/commands/review.test.ts b/packages/cli/src/commands/review.test.ts index df85be82281..bcbb5adaaa3 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 bb75c00ee16..742caf7a063 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'; @@ -67,6 +68,7 @@ export const reviewCommand: CommandModule = { .command(commentStatusCommand) .command(loadRulesCommand) .command(agentPromptCommand) + .command(emitWorkflowCommand) .command(buildTestCommand) .command(baseTreeCommand) .command(scratchTreeCommand) @@ -90,7 +92,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, mock-provider, extract-step, script-lint, 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, mock-provider, extract-step, script-lint, 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 abfd292b3af..539e2902be3 100644 --- a/packages/cli/src/commands/review/agent-prompt.ts +++ b/packages/cli/src/commands/review/agent-prompt.ts @@ -1348,8 +1348,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 @@ -2302,12 +2306,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/cleanup.test.ts b/packages/cli/src/commands/review/cleanup.test.ts index 38acbed4b0b..4133bd7c747 100644 --- a/packages/cli/src/commands/review/cleanup.test.ts +++ b/packages/cli/src/commands/review/cleanup.test.ts @@ -3,16 +3,36 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { join } from 'node:path'; +import { safeTarget } from '../../utils/paths.js'; +import { REVIEW_TMP_DIR } from './lib/paths.js'; const mocks = vi.hoisted(() => ({ + // The default script naming is the basename transform the pre-existing + // assertions name; the dotfile cleanup test swaps in the REAL digest + // derivation — the one `emit-workflow` writes under. Kept as its own + // member so beforeEach can restore it after that swap. + basenameScriptPath: (planPath: string): string => + `/repo/.qwen/workflows/${planPath + .split('/') + .at(-1)! + .replace(/\.json$/u, '.js')}`, + reviewWorkflowScriptPath: vi.fn(), execFileSync: vi.fn(), existsSync: vi.fn((_path: string): boolean => false), + // The parameter is declared so the path-dependent implementations the + // symlink and workflow tests install stay assignable to this mock's type. lstatSync: vi.fn( - (): { isSymbolicLink: () => boolean; isDirectory: () => boolean } => ({ + ( + _path: string, + ): { + isSymbolicLink: () => boolean; + isDirectory: () => boolean; + } => ({ isSymbolicLink: () => false, isDirectory: () => true, }), ), + findSymlinkedReviewWorkflowPath: vi.fn((): string | undefined => undefined), // The return type is declared so `mockReturnValue` can take string arrays — // the sweep-retention tests hand it the tmp-dir listing. readdirSync: vi.fn((_path: string): string[] => []), @@ -124,6 +144,7 @@ vi.mock('./lib/platform/aone-client.js', () => ({ vi.mock('./lib/paths.js', async (importOriginal) => { const actual = await importOriginal(); + const { safeTarget } = await import('../../utils/paths.js'); return { ...actual, worktreePath: (prNumber: string) => `/repo/.qwen/tmp/review-pr-${prNumber}`, @@ -133,9 +154,16 @@ vi.mock('./lib/paths.js', async (importOriginal) => { reviewBranch: (prNumber: string) => `qwen-review/pr-${prNumber}`, LEASE_PREFIX: 'qwen-review-lease-', REVIEW_TMP_DIR: '/repo/.qwen/tmp', + // Faithful to the real helpers, which flatten the target through + // `safeTarget`: the earlier verbatim interpolation let every test pass + // while the CLI disagreed with `emit-workflow` over the script name of + // any target `safeTarget` rewrites — a dotfile review (#8943). tmpFile: (target: string, suffix: string) => - `/repo/.qwen/tmp/qwen-review-${target}-${suffix}`, - tmpPrefix: (target: string) => `qwen-review-${target}-`, + `/repo/.qwen/tmp/qwen-review-${safeTarget(target)}-${suffix}`, + tmpPrefix: (target: string) => `qwen-review-${safeTarget(target)}-`, + REVIEW_WORKFLOWS_DIR: '/repo/.qwen/workflows', + findSymlinkedReviewWorkflowPath: mocks.findSymlinkedReviewWorkflowPath, + reviewWorkflowScriptPath: mocks.reviewWorkflowScriptPath, }; }); @@ -161,6 +189,13 @@ describe('runCleanup', () => { isDirectory: () => true, }); mocks.existsSync.mockReturnValue(false); + mocks.findSymlinkedReviewWorkflowPath.mockReturnValue(undefined); + mocks.lstatSync.mockImplementation((path: string) => { + if (path === '/repo/.qwen/workflows') { + return { isSymbolicLink: () => false, isDirectory: () => true }; + } + throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); + }); // Implementations survive clearAllMocks — restore the fail-open throw // so one retention test's mtimes cannot leak into the next test. The // readFileSync default is the same story (#9272): a leaked @@ -177,6 +212,10 @@ describe('runCleanup', () => { // path-dependent implementations, and a later test reading the declared // `[]` default would otherwise inherit them. mocks.readdirSync.mockImplementation((_path: string): string[] => []); + // Same leak class for the script naming: the dotfile test swaps in the + // real digest derivation, and a later test asserting the basename names + // must not inherit it. + mocks.reviewWorkflowScriptPath.mockImplementation(mocks.basenameScriptPath); mocks.refExists.mockReturnValue(true); mocks.releaseWorktree.mockReturnValue({ existed: false, @@ -209,6 +248,265 @@ describe('runCleanup', () => { expect(mocks.clearReviewWorktreeLease).not.toHaveBeenCalled(); }); + // `emit-workflow` has to write its generated fan-out script into the + // saved-workflow dir — the Workflow loader refuses anything outside it — and + // that dir is also the user's own saved workflows, where each file is a + // `/` slash command. A review that left one behind would hand the user + // a permanent command for a diff that no longer exists. + it('removes only this target generated fan-out scripts', () => { + mocks.execFileSync.mockReturnValue(Buffer.from('')); + mocks.existsSync.mockImplementation( + ((p: string) => + p === '/repo/.qwen/workflows' || + p === '/repo/.qwen/workflows/qwen-review-pr-123-plan.js' || + p === '/repo/.qwen/workflows/qwen-review-pr-123-fetch.js') as never, + ); + mocks.lstatSync.mockReturnValue({ + isSymbolicLink: () => false, + isDirectory: () => false, + }); + // The foreign entries sit in the listing an enumeration would read, so a + // future sweep that over-deletes past this target's exact paths turns + // the negative assertions below red instead of seeing an empty dir. + mocks.readdirSync.mockImplementation(((path: string) => + path === '/repo/.qwen/workflows' + ? ['qwen-review-a749ec7145.js', 'qwen-review-checklist.js'] + : []) as never); + + runCleanup('pr-123'); + + expect(mocks.rmSync).toHaveBeenCalledWith( + '/repo/.qwen/workflows/qwen-review-pr-123-plan.js', + { force: true }, + ); + expect(mocks.rmSync).toHaveBeenCalledWith( + '/repo/.qwen/workflows/qwen-review-pr-123-fetch.js', + { force: true }, + ); + expect(mocks.rmSync).not.toHaveBeenCalledWith( + '/repo/.qwen/workflows/qwen-review-a749ec7145.js', + expect.anything(), + ); + expect(mocks.rmSync).not.toHaveBeenCalledWith( + '/repo/.qwen/workflows/qwen-review-checklist.js', + expect.anything(), + ); + }); + + it('removes both plan-path spellings of a target safeTarget rewrites', async () => { + // The bundled skill spells the plan path with the target VERBATIM + // (`qwen-review-.gitignore-plan.json`), while `tmpFile` flattens it + // through `safeTarget` — and `emit-workflow` digests whichever path it + // was handed (#8943). Hold the sweep to the REAL script-name derivation + // (not the basename default) so the names asserted are the exact ones + // `emit-workflow` writes. + const actualPaths = + await vi.importActual('./lib/paths.js'); + mocks.reviewWorkflowScriptPath.mockImplementation( + actualPaths.reviewWorkflowScriptPath, + ); + mocks.execFileSync.mockReturnValue(Buffer.from('')); + const rawScript = actualPaths.reviewWorkflowScriptPath( + join(REVIEW_TMP_DIR, 'qwen-review-.gitignore-plan.json'), + ); + const safeScript = actualPaths.reviewWorkflowScriptPath( + join(REVIEW_TMP_DIR, `qwen-review-${safeTarget('.gitignore')}-plan.json`), + ); + // The shape this test exists to cover: two spellings, two digests. + expect(rawScript).not.toBe(safeScript); + mocks.existsSync.mockImplementation( + ((p: string) => p === '/repo/.qwen/workflows') as never, + ); + mocks.lstatSync.mockImplementation(((path: string) => { + if ( + path === '/repo/.qwen/workflows' || + path === rawScript || + path === safeScript + ) { + return { + isSymbolicLink: () => false, + isDirectory: () => path === '/repo/.qwen/workflows', + }; + } + throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); + }) as never); + + runCleanup('.gitignore'); + + expect(mocks.rmSync).toHaveBeenCalledWith(rawScript, { force: true }); + expect(mocks.rmSync).toHaveBeenCalledWith(safeScript, { force: true }); + }); + + it('does not remove a workflow through a symlinked root', () => { + mocks.execFileSync.mockReturnValue(Buffer.from('')); + mocks.refExists.mockReturnValue(false); + mocks.existsSync.mockImplementation( + ((p: string) => + p === '/repo/.qwen/workflows' || + p === '/repo/.qwen/workflows/qwen-review-pr-123-plan.js' || + p === '/repo/.qwen/workflows/qwen-review-pr-123-fetch.js') as never, + ); + mocks.lstatSync.mockImplementation((path: string) => { + if ( + path === '/repo/.qwen/workflows' || + path === '/repo/.qwen/workflows/qwen-review-pr-123-plan.js' || + path === '/repo/.qwen/workflows/qwen-review-pr-123-fetch.js' + ) { + return { + isSymbolicLink: () => false, + isDirectory: () => path === '/repo/.qwen/workflows', + }; + } + throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); + }); + mocks.findSymlinkedReviewWorkflowPath.mockReturnValue( + '/repo/.qwen/workflows', + ); + + runCleanup('pr-123'); + + expect(mocks.rmSync).not.toHaveBeenCalledWith( + '/repo/.qwen/workflows/qwen-review-pr-123-plan.js', + expect.anything(), + ); + expect(mocks.rmSync).not.toHaveBeenCalledWith( + '/repo/.qwen/workflows/qwen-review-pr-123-fetch.js', + expect.anything(), + ); + expect(mocks.writeStderrLine).toHaveBeenCalledWith( + 'Skipping workflow cleanup: /repo/.qwen/workflows is a symlink.', + ); + expect(mocks.writeStdoutLine).not.toHaveBeenCalledWith( + 'Nothing to clean for target "pr-123".', + ); + }); + + it('does not remove a workflow through a symlinked ancestor', () => { + mocks.execFileSync.mockReturnValue(Buffer.from('')); + mocks.refExists.mockReturnValue(false); + mocks.existsSync.mockReturnValue(true); + mocks.findSymlinkedReviewWorkflowPath.mockReturnValue('/repo/.qwen'); + + runCleanup('pr-123'); + + expect(mocks.rmSync).not.toHaveBeenCalledWith( + expect.stringContaining('/workflows/'), + expect.anything(), + ); + expect(mocks.writeStderrLine).toHaveBeenCalledWith( + 'Skipping workflow cleanup: /repo/.qwen is a symlink.', + ); + expect(mocks.writeStdoutLine).not.toHaveBeenCalledWith( + 'Nothing to clean for target "pr-123".', + ); + }); + + it('does not claim nothing when workflow-root inspection fails', () => { + mocks.execFileSync.mockReturnValue(Buffer.from('')); + mocks.refExists.mockReturnValue(false); + mocks.existsSync.mockImplementation( + ((p: string) => p === '/repo/.qwen/workflows') as never, + ); + mocks.findSymlinkedReviewWorkflowPath.mockImplementation(() => { + throw new Error('EACCES'); + }); + + runCleanup('pr-123'); + + expect(mocks.writeStderrLine).toHaveBeenCalledWith( + 'Failed to inspect /repo/.qwen/workflows: EACCES', + ); + expect(mocks.writeStdoutLine).not.toHaveBeenCalledWith( + 'Nothing to clean for target "pr-123".', + ); + }); + + it('unlinks a dangling generated workflow without following it', () => { + const planPath = '/repo/.qwen/workflows/qwen-review-pr-123-plan.js'; + const fetchPath = '/repo/.qwen/workflows/qwen-review-pr-123-fetch.js'; + mocks.execFileSync.mockReturnValue(Buffer.from('')); + mocks.existsSync.mockImplementation( + ((p: string) => p === '/repo/.qwen/workflows') as never, + ); + mocks.lstatSync.mockImplementation((path: string) => { + if (path === '/repo/.qwen/workflows' || path === planPath) { + return { + isSymbolicLink: () => path === planPath, + isDirectory: () => path === '/repo/.qwen/workflows', + }; + } + throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); + }); + + runCleanup('pr-123'); + + expect(mocks.rmSync).toHaveBeenCalledWith(planPath, { force: true }); + expect(mocks.rmSync).not.toHaveBeenCalledWith(fetchPath, expect.anything()); + }); + + it('sweeps this target killed-run temp files, and nothing else', () => { + const planOrphan = + 'qwen-review-pr-123-plan.js.8095c532-aaaa-4bbb-8ccc-1234567890ab.tmp'; + const fetchOrphan = + 'qwen-review-pr-123-fetch.js.91a6d643-bbbb-4ccc-9ddd-234567890abc.tmp'; + const foreignOrphan = + 'qwen-review-pr-456-plan.js.02b7e754-cccc-4ddd-8eee-34567890abcd.tmp'; + mocks.execFileSync.mockReturnValue(Buffer.from('')); + mocks.existsSync.mockImplementation( + ((p: string) => p === '/repo/.qwen/workflows') as never, + ); + mocks.readdirSync.mockImplementation(((path: string) => + path === '/repo/.qwen/workflows' + ? [planOrphan, fetchOrphan, foreignOrphan, 'qwen-review-checklist.js'] + : []) as never); + + runCleanup('pr-123'); + + expect(mocks.rmSync).toHaveBeenCalledWith( + `/repo/.qwen/workflows/${planOrphan}`, + { force: true }, + ); + expect(mocks.rmSync).toHaveBeenCalledWith( + `/repo/.qwen/workflows/${fetchOrphan}`, + { force: true }, + ); + expect(mocks.rmSync).not.toHaveBeenCalledWith( + `/repo/.qwen/workflows/${foreignOrphan}`, + expect.anything(), + ); + expect(mocks.rmSync).not.toHaveBeenCalledWith( + '/repo/.qwen/workflows/qwen-review-checklist.js', + expect.anything(), + ); + }); + + it('does not claim nothing when generated-workflow inspection fails', () => { + const planPath = '/repo/.qwen/workflows/qwen-review-pr-123-plan.js'; + mocks.execFileSync.mockReturnValue(Buffer.from('')); + mocks.refExists.mockReturnValue(false); + mocks.existsSync.mockImplementation( + ((p: string) => p === '/repo/.qwen/workflows') as never, + ); + mocks.lstatSync.mockImplementation((path: string) => { + if (path === '/repo/.qwen/workflows') { + return { isSymbolicLink: () => false, isDirectory: () => true }; + } + if (path === planPath) { + throw new Error('EACCES'); + } + throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); + }); + + runCleanup('pr-123'); + + expect(mocks.writeStderrLine).toHaveBeenCalledWith( + `Failed to inspect ${planPath}: EACCES`, + ); + expect(mocks.writeStdoutLine).not.toHaveBeenCalledWith( + 'Nothing to clean for target "pr-123".', + ); + }); + it('clears the lease when cleanup succeeds', () => { mocks.execFileSync.mockReturnValue(Buffer.from('')); diff --git a/packages/cli/src/commands/review/cleanup.ts b/packages/cli/src/commands/review/cleanup.ts index 8898fa84b97..9157dc94a06 100644 --- a/packages/cli/src/commands/review/cleanup.ts +++ b/packages/cli/src/commands/review/cleanup.ts @@ -22,7 +22,7 @@ import { rmSync, statSync, } from 'node:fs'; -import { dirname, join, resolve } from 'node:path'; +import { basename, dirname, join, resolve } from 'node:path'; import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; import { clearReviewWorktreeLease, @@ -47,6 +47,9 @@ import { reviewBranch, inertPath, REVIEW_TMP_DIR, + REVIEW_WORKFLOWS_DIR, + findSymlinkedReviewWorkflowPath, + reviewWorkflowScriptPath, tmpFile, tmpPrefix, } from './lib/paths.js'; @@ -1008,6 +1011,104 @@ export function runCleanup(target: string): void { } } + // --- Generated fan-out script (under .qwen/workflows/) ---------------- + // The plan path is deterministic for every cleanup target, so it identifies + // the one generated script this run owns, including after a killed run. + // Derive it in BOTH spellings the plan file can carry: the bundled skill + // names that file with the target VERBATIM + // (`qwen-review-.gitignore-plan.json`), while `tmpFile` flattens it through + // `safeTarget` — and `emit-workflow` digests whichever path it was handed. + // A target `safeTarget` rewrites keeps its script (and any killed-run + // `.tmp` beside it) forever unless both spellings are swept (#8943). + const workflowPaths = [ + ...new Set([ + reviewWorkflowScriptPath(tmpFile(target, 'plan.json')), + reviewWorkflowScriptPath( + join(REVIEW_TMP_DIR, `qwen-review-${target}-plan.json`), + ), + ...(/^pr-\d+$/u.test(target) + ? [reviewWorkflowScriptPath(tmpFile(target, 'fetch.json'))] + : []), + ]), + ]; + let workflowRootSafe = false; + try { + const unsafePath = findSymlinkedReviewWorkflowPath(); + if (unsafePath) { + writeStderrLine(`Skipping workflow cleanup: ${unsafePath} is a symlink.`); + failedAny = true; + } else if (existsSync(REVIEW_WORKFLOWS_DIR)) { + workflowRootSafe = true; + } + } catch (err) { + writeStderrLine( + `Failed to inspect ${REVIEW_WORKFLOWS_DIR}: ${(err as Error).message}`, + ); + failedAny = true; + } + + if (workflowRootSafe) { + for (const workflowPath of workflowPaths) { + try { + lstatSync(workflowPath); + } catch (err) { + if ((err as NodeJS.ErrnoException).code === 'ENOENT') continue; + writeStderrLine( + `Failed to inspect ${workflowPath}: ${(err as Error).message}`, + ); + failedAny = true; + continue; + } + try { + rmSync(workflowPath, { force: true }); + writeStdoutLine(`Removed generated workflow: ${workflowPath}`); + removedAny = true; + } catch (err) { + writeStderrLine( + `Failed to remove ${workflowPath}: ${(err as Error).message}`, + ); + failedAny = true; + } + } + + // A run killed between the temp-file write and the rename leaves + // ` 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); + } + } + }); + + it('names the agents that returned nothing instead of dropping them', 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. + const { result } = await runScript( + buildReviewWorkflowScript(AGENTS), + async (prompt) => { + if (prompt === 'PROMPT-2') throw new Error('agent died'); + return `said:${prompt}`; + }, + ); + const r = result as { + delivered: Array<{ key: string }>; + missingRoles: string[]; + }; + expect(r.missingRoles).toEqual(['2']); + expect(r.delivered.map((d) => d.key)).toEqual(['1a', '7']); + }); + + it('treats an undefined return as missing, not as an empty finding set', async () => { + const { result } = await runScript( + buildReviewWorkflowScript(AGENTS), + async (prompt) => (prompt === 'PROMPT-7' ? undefined : `said:${prompt}`), + ); + expect((result as { missingRoles: string[] }).missingRoles).toEqual(['7']); + }); + + it('counts a result that strips to empty as missing, not 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 ['', ' ']) { + const { result } = await runScript( + buildReviewWorkflowScript(AGENTS), + async (prompt) => (prompt === 'PROMPT-2' ? empty : `said:${prompt}`), + ); + const r = result as { + delivered: Array<{ key: string }>; + missingRoles: string[]; + }; + expect(r.missingRoles).toEqual(['2']); + expect(r.delivered.map((d) => d.key)).toEqual(['1a', '7']); + } + }); + + 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 a failed step, not a step with an + // empty result. Returned as a value, it would let the caller aggregate over + // a diff no agent read — the outcome the coverage gate exists to prevent, + // reached without the gate being consulted. + it('throws when every agent failed rather than returning an empty result', async () => { + await expect( + runScript(buildReviewWorkflowScript(AGENTS), async () => { + throw new Error('all dead'); + }), + ).rejects.toThrow(/all 3 agents failed to deliver/); + }); + + 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('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('); + }); +}); + +// The script's fail-closed guarantee is only half in code: it NAMES the roles +// that came back empty, but the coverage step asserts launches against +// transcripts — and a cap-killed or stripped-empty agent left one. The +// consumer of the naming is prose: the skill's Exit-0 branch must treat a +// non-empty missingRoles as a failed step and re-dispatch before Step 3D. Pin +// the branch to the field it consumes; a skill edit that drops the gate fails +// here, next to the script that would orphan it. +describe('the skill consumes the missingRoles the script returns', () => { + const repoRoot = resolve( + dirname(fileURLToPath(import.meta.url)), + '..', + '..', + '..', + '..', + '..', + ); + const SKILL_PATH = join( + repoRoot, + 'packages/core/src/skills/bundled/review/SKILL.md', + ); + const skill = existsSync(SKILL_PATH) + ? readFileSync(SKILL_PATH, 'utf8').replace(/\r\n/g, '\n') + : null; + // A sparse or partial checkout has no skill to read — the same exemption + // run-skill-parity.test.ts grants its own SKILL.md oracles. + const itWithSkill = skill === null ? it.skip : it; + + itWithSkill( + 'the Exit-0 branch gates Step 3D on an empty missingRoles', + () => { + const start = (skill as string).indexOf( + '### Which engine dispatches this fan-out', + ); + const end = (skill as string).indexOf('## Step 3B'); + expect(start).toBeGreaterThan(-1); + expect(end).toBeGreaterThan(start); + const section = (skill as string).slice(start, end); + expect(section).toContain('`missingRoles`'); + expect(section).toContain('A non-empty `missingRoles` is a failed step'); + expect(section).toContain('do not carry it to Step 3D'); + expect(section).toContain('agent-prompt --role '); + expect(section).toContain('QWEN_CODE_WORKFLOW_AGENT_MAX_TURNS'); + }, + ); +}); 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..dd048d12289 --- /dev/null +++ b/packages/cli/src/commands/review/workflow-script.ts @@ -0,0 +1,181 @@ +/** + * @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 one JSON literal in front of it: the +// roster the CLI computed. 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`, which is where the +// first version of this put it: the Workflow tool takes `args` as INLINE JSON +// (`WORKFLOW_PARAM_SCHEMA`: "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 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. + +/** 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 roster literal. + * + * 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. +// +// agentType is the type the hand-launched path sets as subagent_type — +// 'review-agent' — 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: 'review-agent', + workingDir: WORKING_DIR, + } + : { + label: a.key, + phase: 'Review', + agentType: 'review-agent', + }, + ), + ), +); + +// 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 the skill's Exit-0 branch is the +// consumer: a non-empty missingRoles fails the step there. +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 }); + } +} + +if (missingRoles.length > 0) { + log(missingRoles.length + ' agent(s) returned nothing: ' + missingRoles.join(', ')); +} + +// A fan-out where nothing came back is a failed step, not a step with an empty +// result. Returning it as a value would let the caller proceed to aggregation +// over a diff no agent read — the exact outcome the coverage gate exists to +// prevent, arrived at without the gate ever being consulted. +if (delivered.length === 0) { + throw new Error( + 'review fan-out: all ' + AGENTS.length + ' agents failed to deliver (' + + missingRoles.join(', ') + '). Nothing was reviewed.', + ); +} + +return { + rosterSize: AGENTS.length, + delivered: delivered, + missingRoles: missingRoles, +}; +`; + +/** + * The full script for one review: `meta`, the roster literal, the worktree + * pin, and the body. + * + * `meta.phases` mirrors the skill's step names 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` + + FAN_OUT_BODY + ); +} diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index e8b54c013f2..523a7fcffd2 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -3425,6 +3425,25 @@ describe('Settings Loading and Merging', () => { } }); + it('should ignore workspace env overrides for the review workflow gate', () => { + delete process.env['QWEN_REVIEW_WORKFLOW']; + (mockFsExistsSync as Mock).mockReturnValue(true); + (fs.readFileSync as Mock).mockImplementation( + (p: fs.PathOrFileDescriptor) => { + if (p === MOCK_WORKSPACE_SETTINGS_PATH) + return JSON.stringify({ env: { QWEN_REVIEW_WORKFLOW: '1' } }); + return '{}'; + }, + ); + + try { + loadSettings(MOCK_WORKSPACE_DIR); + expect(process.env['QWEN_REVIEW_WORKFLOW']).toBeUndefined(); + } finally { + delete process.env['QWEN_REVIEW_WORKFLOW']; + } + }); + it('should warn when workspace settings define workflowsEnabled', () => { (mockFsExistsSync as Mock).mockReturnValue(true); (fs.readFileSync as Mock).mockImplementation( diff --git a/packages/cli/src/config/shared-env-keys.test.ts b/packages/cli/src/config/shared-env-keys.test.ts index 73d1a04ab11..11ada0d6efe 100644 --- a/packages/cli/src/config/shared-env-keys.test.ts +++ b/packages/cli/src/config/shared-env-keys.test.ts @@ -97,6 +97,21 @@ describe('PROJECT_ENV_HARDCODED_EXCLUSIONS', () => { ); }); + // The workflow gates are user opt-ins: a project `.env` enabling the + // runtime — or supplying /review's A/B gate on the user's behalf — would + // silently route reviews into the experimental workflow path for any user + // who enabled the runtime at user level. + it('excludes the workflow gates so a project .env cannot opt reviews in', () => { + expect(PROJECT_ENV_HARDCODED_EXCLUSIONS).toContain( + 'QWEN_CODE_ENABLE_WORKFLOWS', + ); + expect(PROJECT_ENV_HARDCODED_EXCLUSIONS).toContain( + 'QWEN_CODE_DISABLE_WORKFLOWS', + ); + expect(PROJECT_ENV_HARDCODED_EXCLUSIONS).toContain('QWEN_REVIEW_WORKFLOW'); + expect(isHardcodedProjectEnvExclusion('qwen_review_workflow')).toBe(true); + }); + // The non-Node TLS trust-anchor vars reach the same MITM outcome as // NODE_EXTRA_CA_CERTS for the curl/git/openssl/python tools a session // shells out to; a project .env must not inject an attacker CA. diff --git a/packages/cli/src/config/shared-env-keys.ts b/packages/cli/src/config/shared-env-keys.ts index a05d8a0a602..c69614673c1 100644 --- a/packages/cli/src/config/shared-env-keys.ts +++ b/packages/cli/src/config/shared-env-keys.ts @@ -41,8 +41,12 @@ export const PROJECT_ENV_HARDCODED_EXCLUSIONS = [ 'QWEN_CODE_MEMORY_PROJECT_SCOPE', // Workflow execution is an explicit user opt-in. A project must not enable // it or override a user opt-in through settings.env or a project .env. + // QWEN_REVIEW_WORKFLOW is /review's own A/B gate and is user-owned for the + // same reason: a project `.env` must not supply it on the user's behalf and + // silently route reviews into the workflow path. 'QWEN_CODE_ENABLE_WORKFLOWS', 'QWEN_CODE_DISABLE_WORKFLOWS', + 'QWEN_REVIEW_WORKFLOW', // QWEN_TLS_INSECURE (and NODE_TLS_REJECT_UNAUTHORIZED, which it mirrors) // disable TLS certificate verification for all outbound API connections. A // project `.env` must never enable either — that would let an untrusted repo diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index cab01680dbd..234050d49ea 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -341,6 +341,28 @@ Launch **16 agents** for same-repo **PR** reviews (Agent 1 has three procedural **At medium effort, launch the reduced set:** skip the three adversarial personas (Agents 6a/6b/6c), the two dedicated angles (Agents 1d/1e), and the Agent 8 diff-specialists, launching Agents 0 (PR targets only), 1a, 1b, 1c, 2, 3a, 3b, 3c, 4, 5, and 7 — **11 agents** for a same-repo PR, **10** for a local-diff or file-path review (no Agent 0), **9** for cross-repo lightweight (drop Agent 7 and 1c too, as above). Everything else about 3A is identical — the briefs, the `working_dir` pin, the whiff check, coverage; medium changes only which dimensions launch, not how any agent runs. **Build the roster with `agent-prompt --roster`** — it reads the effort the plan recorded at Step 1 (`plan.effort`), so on a medium plan it omits 6a/6b/6c and 1d/1e from the roster it prints (Agent 8 was never in it) and you launch exactly these agents. `check-coverage` (Step 3D) reads the **same** `plan.effort` and requires exactly these too — no flag to pass, and no way for the roster you launched and the gate that checks it to disagree. (The effort lives in the plan, not in a flag, on purpose: a roster a caller could shrink by omitting a flag is a roster that gets shrunk. If Step 1 recorded no effort, the full roster is required, personas included — the fail-safe, not a medium review.) +### Which engine dispatches this fan-out — ask, do not decide + +**Run this first. It answers, and its answer is the branch:** + +```bash +"${QWEN_CODE_CLI:-qwen}" review emit-workflow --plan \ + [--rules ] +``` + +- **Exit 6** — this review runs the **legacy** path. The command wrote nothing; it prints the reason to stderr (workflows not enabled, `QWEN_REVIEW_WORKFLOW` not set, or a territory fan-out). Continue with `agent-prompt --roster` exactly as described below. This is the default and the expected outcome; it is a routing verdict, not an error, and there is nothing to repair or retry. +- **Exit 0** — the fan-out is a **workflow**. It printed `scriptPath: `. Make **ONE** tool call — `Workflow({ scriptPath: "" })` — with **no `args`**, and do **not** build agent calls by hand for this step. The roster is inside the script; you neither read it nor relay it. The script pins every agent to the review's worktree, so there is no `working_dir` for you to set here. + - **Read the whole result before Step 3D.** A workflow returns every agent's text inside **one** tool result, and that result is subject to the runtime's per-tool output ceiling — where the hand-launched path gets a separate 32 000-character budget per agent. A fourteen-agent fan-out is already at that ceiling. If the result says it was truncated and names a spill file, **`read_file` that file, paging until `isTruncated` is false**, and review from the full text — the same rule as the `--roster` redirect below, for the same reason: the cut lands in the MIDDLE, so entire agents disappear from a result that otherwise reads as complete. Step 3D will not catch it — it checks dispatch against the on-disk transcripts, not against what reached you — so the findings you never read are simply never reported. + - **A non-empty `missingRoles` is a failed step — do not carry it to Step 3D.** The result lists under `missingRoles` every role whose dispatch came back empty, and Step 3D cannot see this class — it checks launches against transcripts, and a dispatch killed by the runtime's per-attempt cap (50 turns or 10 minutes by default; `QWEN_CODE_WORKFLOW_AGENT_MAX_TURNS` / `QWEN_CODE_WORKFLOW_AGENT_MAX_MINUTES` raise them — a large review's Build & Test agent can legitimately run past both) WAS launched, so its transcript exists exactly like a delivered agent's. Rebuild each named role's prompt with `agent-prompt --role ` — the relaunch form below, keeping `--rules` if Step 2 loaded rules — and launch them in one response, pinned to the worktree exactly as the legacy path pins its launches. Then go to Step 3D. If a relaunch also comes back empty, stop and name the dimensions that were never reviewed: proceeding with a named role unreviewed is the silent-missing regression this path must not introduce. + - Then go to Step 3D, which is unchanged: coverage still reads the harness's own transcripts, and the prompts the script dispatched were recorded on disk when it was generated. +- Any other non-zero exit is a real failure — read the message rather than falling back silently. + +Why it is a command and not your judgment: eligibility depends on facts spread across the environment and the plan (two gates, the topology), and a caller that decides for itself is a caller that can decide wrong in the one direction that matters — taking the workflow path for a territory fan-out would hand you a single result the runtime truncates, with whole chunk agents cut out of the middle of a review that still reads as complete. + +The workflow path is **opt-in and under A/B**: it needs both `QWEN_CODE_ENABLE_WORKFLOWS=1` and `QWEN_REVIEW_WORKFLOW=1`. Unsetting the second is the one-switch rollback and changes nothing else about the runtime. What the two paths share is everything that decides quality — the same plan, the same roster, the same briefs, the same prompts built by the same function, the same `review-agent` subagent type, the same worktree pin, the same coverage gate. What differs is who launches them **and how their output comes back**: fourteen Agent results with a budget each, or one Workflow result carrying all fourteen under a single ceiling. That second difference is not cosmetic — it is why the Exit 0 branch above tells you to read the spill file, and why a territory fan-out stays on the legacy path. + +**Everything below is the legacy path** (and, since the workflow path builds the same prompts from the same plan, it stays the description of what the agents receive either way). + **Do not write these prompts, and do not ask for them one at a time. One call builds all of them:** ```bash