diff --git a/docs/design/review-toolchain-adapters.md b/docs/design/review-toolchain-adapters.md new file mode 100644 index 00000000000..6b9c90e8f0a --- /dev/null +++ b/docs/design/review-toolchain-adapters.md @@ -0,0 +1,234 @@ +# Review toolchain adapters + +## Status + +Accepted, implemented. This document covers the extraction of the toolchain +adapter boundary: the npm-specific `qwen review build-test` behavior moves +behind an internal contract without changing its command-line interface or +report format. The phase that adds the first second adapter appends its own +section here. + +## Problem + +`qwen review build-test` currently combines three responsibilities in one +module: + +1. Reading the review plan and selecting changed files. +2. Deciding which repository toolchain can be verified deterministically. +3. Implementing npm workspace installation, affected-package selection, + dependency widening, build execution, test execution, and result reporting. + +The command works well for npm repositories, but its public report models the +implementation directly as `toolchain: "npm" | "unsupported"`. Agent 7 falls +back to prompt-directed Maven, Gradle, Cargo, Go, or Python commands when the +npm path is unsupported. That fallback is useful, but it is not deterministic +infrastructure: module selection, command choice, result parsing, timeout +classification, and failure attribution remain agent decisions. + +Adding Maven and Gradle directly to `build-test.ts` would create a growing +conditional command rather than a stable cross-language verification boundary. +It would also make the existing npm behavior harder to protect while new +languages are added. + +## Goals + +P0 must: + +- Introduce a small internal toolchain adapter contract. +- Move npm repository detection and npm build/test execution behind the npm + adapter. +- Preserve the `qwen review build-test` CLI arguments. +- Preserve the existing `BuildTestReport` JSON shape and all npm behavior. +- Preserve the exported `runBuildTest`, `trimOutput`, `buildRunEnv`, + `spawnTimedOut`, and `unresolvedWorkspaceDeps` test seams. +- Keep unsupported repositories on the existing Agent 7 fallback path. +- Make Maven and Gradle additions possible without modifying command routing or + verdict composition. + +## Non-goals + +P0 does not: + +- Execute Maven or Gradle. +- Support multiple toolchains in one repository. +- Define a third-party plugin API or dynamic adapter loading. +- Parse test coverage artifacts such as Istanbul, LCOV, or JaCoCo. +- Generalize `test-efficacy`, which remains npm workspace and Vitest specific. +- Change Agent 7 prompts, findings, verdicts, or coverage gates. +- Change the `BuildTestReport` JSON schema. + +Multi-toolchain repositories are an expected future requirement, but P0 does +not introduce an unused aggregation model. The adapter contract is scoped to +one verification target so a later orchestrator can select multiple targets +without changing an individual adapter. + +## Current behavior to preserve + +The npm implementation currently: + +- Treats a root package with build or test scripts as a single package. +- Supports the modeled npm workspace glob shapes. +- Selects changed workspaces from plan file paths. +- Builds affected workspaces and their reverse dependents. +- Widens or reorders the build set when the compiler names an undeclared + workspace dependency. +- Tests the affected workspaces and every workspace declared to depend on + them that defines a test script. +- Runs `npm ci` only for an npm repository with an incomplete dependency tree. +- Avoids `npm ci` for warm Yarn, pnpm, and Bun trees. +- Classifies unsupported layouts as a handoff, not a successful verification. +- Classifies timeouts, insufficient disk, and unusable installs as + infrastructure rather than PR findings. +- Removes failed intermediate widening attempts from the final evidence. +- Supports build-only verification for merge-base trees. + +The existing focused test suite is the compatibility oracle for these rules. + +## Design + +### Adapter contract + +Add an internal `ReviewToolchainAdapter` interface with: + +- An `applies` method that decides whether the adapter owns the repository. +- A `run` method that receives normalized build/test arguments and changed file + paths and returns the existing report shape. + +P0 registers one built-in adapter, npm. It applies when the root +`package.json` describes something npm can build — workspaces, or a root +`build`/`test` script; the adapter's existing execution logic then decides +whether the npm layout and dependency state are supported or require the +structured handoff used today. The registry is a fixed array in code. There is no extension +discovery or configuration surface. + +P0 deliberately does not claim to solve mixed-toolchain selection. Static +repository detection alone cannot know whether an adapter will later decline +because of changed-file ownership or cold dependency state. The Maven phase must +design target selection from two real adapters and their module models rather +than freezing a speculative priority rule now. + +### Command boundary + +`build-test.ts` remains the CLI boundary and compatibility facade. It: + +1. Resolves the worktree. +2. Reads and validates changed file paths from the review plan. +3. Selects the sole applicable built-in adapter, failing closed to the + unsupported report when zero or more than one apply. +4. Calls the adapter. +5. Emits the unchanged JSON report. + +The npm-specific implementation owns package discovery, install policy, +workspace selection, build ordering, widening, tests, and npm-specific notes. + +### Report compatibility + +P0 deliberately keeps: + +```text +toolchain: "npm" | "unsupported" +``` + +Changing this to a new generic schema in the same refactor would require +coordinated edits to Agent 7, base-tree, test-plan, test-delta, tests, and any +external scripts consuming the report. The adapter boundary does not require +that migration. + +A later Maven/Gradle phase can widen the discriminant while adding the first +new behavior, with tests for each downstream consumer. + +### Shared execution primitives + +Command execution, output trimming, timeout detection, and environment shaping +remain shared exports from the command module in P0 because adjacent review +commands and existing tests consume them. The npm-specific dependency widening +helper moves with the npm adapter and is re-exported from the command module for +compatibility. + +The adapter receives the injectable executor already used by the existing unit +tests. It does not import the command's runtime executor, so the dependency stays +one-way: the command selects the adapter and passes execution in. Type-only +imports may reference the existing report types without creating a runtime +cycle. This preserves deterministic tests without spawning npm. + +## Files + +P0 changes: + +- `packages/cli/src/commands/review/build-test.ts` + - Retains CLI routing and compatibility exports. + - Selects and invokes the built-in adapter. +- `packages/cli/src/commands/review/lib/toolchain.ts` + - Defines the internal adapter and detection contracts. + - Selects the sole applicable adapter (zero or more than one fails closed). +- `packages/cli/src/commands/review/lib/npm-toolchain.ts` + - Owns npm detection and the existing npm verification algorithm. +- `packages/cli/src/commands/review/lib/npm-toolchain.test.ts` + - Pins adapter selection and contract-level behavior. +- `packages/cli/src/commands/review/build-test.test.ts` + - Remains the end-to-end compatibility suite for the command facade. + +## Testing + +Focused tests must prove: + +1. An npm workspace selects the npm adapter. +2. A single-root npm package selects the npm adapter. +3. A non-npm repository produces the existing unsupported report. +4. An unmodeled npm layout remains unsupported rather than returning a false + green result. +5. Existing build ordering, widening, install, timeout, disk, and test behavior + remains unchanged through `runBuildTest`. +6. The serialized report shape remains unchanged. + +Verification commands: + +```bash +cd packages/cli && npx vitest run src/commands/review/ +npm run typecheck +``` + +## Future phases + +### A second toolchain + +The boundary exists so a second language lands as a registration rather than +another branch in `build-test.ts`. Whichever comes first — Maven, Gradle — +should prefer a checked-in wrapper, take its project model from the build tool +itself rather than re-deriving one from the manifests, select the projects the +diff changed, and parse the JUnit XML the run produced. A build it cannot +model must fail closed to an unsupported handoff, never to a partial green. + +### Coverage artifacts + +Istanbul/LCOV and JaCoCo should normalize into a language-independent +changed-line and changed-branch coverage model. Coverage numbers are evidence +for a concrete untested behavior, not an automatic Critical threshold. + +### Multiple toolchains + +A later orchestration layer may detect multiple verification roots and invoke +one adapter per target. It should aggregate evidence while preserving each +command's toolchain, root, module, and infrastructure status. This phase avoids +specifying that before two real adapters demonstrate the common boundary. + +## Risks + +- **Accidental report drift:** protected by the existing `build-test` suite and + explicit report-shape assertions. +- **Adapter abstraction without behavior:** this phase is justified only if npm + detection and execution move behind the adapter rather than adding an empty + interface around unchanged branching. +- **Premature generalization:** the contract intentionally excludes coverage, + mutation, CI discovery, and multi-toolchain aggregation. +- **False applicability:** the npm adapter applies only when the root + `package.json` can scope something — workspaces or a root build/test script. + A package.json with neither workspaces nor build/test scripts (husky, a lint + config, a script-less docs site) does not apply, so a future adapter can own + such a root alone rather than losing it to a manifest npm cannot scope. + +## Open questions + +Report-schema widening for a second toolchain — the `toolchain` discriminant +and any per-command classification flags — is deferred to the phase that +introduces that behavior, along with multi-toolchain aggregation. diff --git a/packages/cli/src/commands/review/build-test.test.ts b/packages/cli/src/commands/review/build-test.test.ts index 97e901210fa..beeda8cbde7 100644 --- a/packages/cli/src/commands/review/build-test.test.ts +++ b/packages/cli/src/commands/review/build-test.test.ts @@ -14,6 +14,10 @@ import { unresolvedWorkspaceDeps, buildRunEnv, } from './build-test.js'; +import { + npmToolchainAdapter, + unresolvedWorkspaceDeps as toolchainUnresolvedWorkspaceDeps, +} from './lib/npm-toolchain.js'; import type { WorkspacePackage } from './lib/workspaces.js'; const statfsSyncMock = vi.hoisted(() => vi.fn()); @@ -35,6 +39,13 @@ const PKGS: WorkspacePackage[] = [ ]; describe('unresolvedWorkspaceDeps', () => { + it('re-exports the npm-toolchain implementation', () => { + // Pins the module boundary this PR establishes (npm specifics live in + // lib/npm-toolchain.ts): reverting the re-export to an inline copy ships + // green unless this identity is asserted. + expect(unresolvedWorkspaceDeps).toBe(toolchainUnresolvedWorkspaceDeps); + }); + it('finds the workspace package a TS2307 names', () => { const out = "src/a.ts(23,8): error TS2307: Cannot find module '@x/webui' or its " + @@ -117,10 +128,17 @@ describe('runBuildTest', () => { }); afterEach(() => { + vi.restoreAllMocks(); rmSync(root, { recursive: true, force: true }); }); - it('reports `unsupported` for a repo with no workspaces, rather than guessing', () => { + it('treats a package.json with no build role as no npm project at all', () => { + // Docs sites, husky, and lint configs put a script-less package.json in + // repos with nothing npm can scope. It must not make npm apply, or such a + // root would claim the selection away from a second adapter that could + // have verified the diff. + // The handoff note is still npm's precise one: the repo IS npm-shaped, + // and naming why it cannot be scoped beats a generic "no project here". writeFileSync(join(root, 'package.json'), JSON.stringify({ name: 'r' })); writePlan(['src/a.ts']); const rep = runBuildTest({ @@ -129,9 +147,127 @@ describe('runBuildTest', () => { timeout: 5, install: false, }); + expect(rep).toEqual({ + toolchain: 'unsupported', + affected: [], + buildSet: [], + widenedWith: [], + install: null, + build: [], + test: [], + ok: true, + timedOut: [], + note: + 'No npm package here to scope (no workspaces, and the root has no build/test ' + + 'script). Fall back to the build/test precedence in your brief — installing ' + + 'dependencies first — and give each command a deadline it can actually meet.', + }); + }); + + it('surfaces the declared-but-empty workspaces note when no adapter applies', () => { + writeFileSync( + join(root, 'package.json'), + JSON.stringify({ name: 'r', workspaces: ['packages/*'] }), + ); + writePlan(['src/a.ts']); + const rep = runBuildTest({ + plan: planPath, + worktree: root, + timeout: 5, + install: false, + }); expect(rep.toolchain).toBe('unsupported'); - expect(rep.ok).toBe(true); - expect(rep.build).toEqual([]); + expect(rep.note).toContain( + 'declares npm workspaces, but none resolve to a package', + ); + }); + + it('keeps the complete generic unsupported report when no adapter applies', () => { + writePlan(['src/a.java']); + + expect( + runBuildTest({ + plan: planPath, + worktree: root, + timeout: 5, + install: false, + }), + ).toEqual({ + toolchain: 'unsupported', + affected: [], + buildSet: [], + widenedWith: [], + install: null, + build: [], + test: [], + ok: true, + timedOut: [], + note: + 'No supported npm project here to scope. Fall back to the ' + + 'build/test precedence in your brief — installing dependencies first — ' + + 'and give each command a deadline it can actually meet.', + }); + }); + + it('coerces fractional and zero deadlines at the spawn boundary', () => { + // spawnSync validates `timeout` as an unsigned integer: a decimal + // --timeout used to throw ERR_OUT_OF_RANGE out of the whole call (no + // report, no --out file), and --timeout 0 armed no kill timer at all. + pkg('.', { name: 'r', scripts: { test: 'vitest run' } }); + writePlan(['src/a.ts']); + + // 1.005s * 1000 = 1004.9999999999999 in IEEE-754: exactly the value + // spawnSync rejects. 0.1 rounds to an integer and would mask the + // regression; the explicit --budget keeps the wall-clock remainder + // above the fractional deadline so it is the binding term. + const fractional = runBuildTest({ + plan: planPath, + worktree: root, + timeout: 1.005, + budget: 600, + install: false, + }); + expect(fractional.toolchain).toBe('npm'); + expect(fractional.test).toHaveLength(1); + expect(fractional.test[0]?.deadlineMs).toBe(1005); + + // Same explicit budget: without it a zero --timeout also zeroes the + // whole-call budget, and the run discloses instead of reaching the spawn + // boundary this case is about. + const zero = runBuildTest({ + plan: planPath, + worktree: root, + timeout: 0, + budget: 600, + install: false, + }); + expect(zero.test[0]?.deadlineMs).toBe(1); + }); + + it('rejects non-finite --timeout and --budget with a descriptive error', () => { + // yargs `type: 'number'` hands over NaN for `--timeout abc`; NaN + // defeats every budget comparison and reaches spawnSync as an + // invalid deadline — ERR_OUT_OF_RANGE with no report at all. + pkg('.', { name: 'r', scripts: { test: 'vitest run' } }); + writePlan(['src/a.ts']); + + expect(() => + runBuildTest({ + plan: planPath, + worktree: root, + timeout: Number.NaN, + install: false, + }), + ).toThrow(/--timeout must be a finite number/); + expect(() => + runBuildTest({ + plan: planPath, + worktree: root, + timeout: 60, + budget: Number.POSITIVE_INFINITY, + install: false, + }), + ).toThrow(/--budget must be a finite number/); }); it('reports `unsupported` — not a false "nothing to build" — for an unmodeled glob', () => { @@ -155,7 +291,12 @@ describe('runBuildTest', () => { install: false, }); expect(rep.toolchain).toBe('unsupported'); - expect(rep.note).toContain('does not model'); + // The unscopable npm half no longer applies at selection — but the repo + // IS an npm project whose layout cannot be scoped, so the note is npm's + // precise unmodeled-glob wording, not the generic "no project here". + expect(rep.note).toContain( + 'uses a workspace glob shape this command does not model', + ); expect(rep.note).not.toContain('no package to build'); }); @@ -1267,6 +1408,107 @@ describe('runBuildTest', () => { expect(rep.ok).toBe(true); }); + it('names budget-stopped UNTESTABLE suites in notRun too', () => { + // The budget-break push must be UNFILTERED: a suite the build phase + // left unbuilt (untestable) and the budget then never attempted + // otherwise stayed in testScope.workspaces — reported as run and + // passed though zero test commands executed. + writeFileSync( + join(root, 'package.json'), + JSON.stringify({ name: 'r', workspaces: ['packages/*'] }), + ); + pkg('packages/a', { + name: '@x/a', + scripts: { build: 'exit 0', test: 'exit 0' }, + }); + pkg('packages/d', { + name: '@x/d', + dependencies: { '@x/a': '*', '@x/x': '*' }, + scripts: { build: 'exit 0', test: 'exit 0' }, + }); + pkg('packages/x', { + name: '@x/x', + scripts: { build: 'exit 0' }, + }); + writePlan(['packages/a/src/a.ts']); + + const rep = runBuildTest({ + plan: planPath, + worktree: root, + timeout: 60, + budget: 16, + install: false, + exec: (command) => { + if (command.startsWith('npm run build')) { + // Real wall clock, so the budget actually drains. + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 2000); + } + return { + command, + exitCode: 0, + seconds: 1, + timedOut: false, + output: '', + }; + }, + }); + + // `a` builds (2s), then the floor stops the build phase with x and d + // unbuilt, and the test phase starts below the floor. + expect(rep.test).toEqual([]); + expect(rep.testScope?.workspaces).toEqual([]); + expect(rep.testScope?.notRun).toEqual(['packages/a', 'packages/d']); + expect(rep.note).toContain('not run: packages/a, packages/d'); + expect(rep.ok).toBe(true); + }); + + it('discloses a budget-stopped single-root suite instead of claiming no test script', () => { + // The workspace branch names the budget when every suite was trimmed; + // a single-root repo carries no testScope, and its note used to claim + // the package defines no test script — though the script is exactly + // why the suite sits in notRun. + writeFileSync( + join(root, 'package.json'), + JSON.stringify({ + name: 'r', + scripts: { build: 'exit 0', test: 'exit 0' }, + }), + ); + writePlan(['src/a.ts']); + + const calls: string[] = []; + const rep = runBuildTest({ + plan: planPath, + worktree: root, + timeout: 60, + budget: 16, + install: false, + exec: (command) => { + calls.push(command); + if (command.startsWith('npm run build')) { + // Real wall clock, so the budget actually drains. + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 2000); + } + return { + command, + exitCode: 0, + seconds: 1, + timedOut: false, + output: '', + }; + }, + }); + + expect(calls).toEqual(['npm run build']); + expect(rep.test).toEqual([]); + expect(rep.note).toContain( + 'whole-call budget was spent before any suite could run', + ); + expect(rep.note).not.toContain('defines no test script'); + expect(rep.note).toContain('not run: .'); + expect(rep.ok).toBe(true); + }); + it('runs the AFFECTED workspace first, so the budget trims dependents, never the changed suite', () => { // The closure is alphabetical — `alpha` before `zebra` — but the diff // changed zebra, and its own suite is the one most likely to catch the @@ -2413,4 +2655,86 @@ describe('runBuildTest', () => { expect(rep.note).not.toContain('Critical'); expect(rep.note).not.toContain('Correlate'); }); + + it('routes the run through the selected toolchain adapter (pins the delegation)', () => { + // This PR's whole change is that runBuildTest selects an adapter and delegates + // to adapter.run. Nothing else pinned that boundary: reverting the facade to the + // old inline implementation kept every report-shape test green. Spy on the + // adapter so a revert (adapter never called) turns this red. + writeFileSync( + join(root, 'package.json'), + JSON.stringify({ name: 'r', workspaces: ['packages/*'] }), + ); + pkg('packages/a', { name: '@x/a', scripts: { build: 'exit 0' } }); + writePlan(['packages/a/src/x.ts']); + + const runSpy = vi.spyOn(npmToolchainAdapter, 'run'); + + const rep = runBuildTest({ + plan: planPath, + worktree: root, + timeout: 60, + install: false, + exec: okExec, + }); + + expect(runSpy).toHaveBeenCalledTimes(1); + // The arguments are forwarded to the adapter unchanged. + expect(runSpy).toHaveBeenCalledWith( + expect.objectContaining({ + root, + changedFiles: ['packages/a/src/x.ts'], + timeout: 60, + install: false, + exec: expect.any(Function), + }), + ); + // And the report runBuildTest returns IS the adapter's report. + expect(rep).toBe(runSpy.mock.results[0]?.value); + runSpy.mockRestore(); + }); + + it('defaults the adapter exec to the real runner when none is injected', () => { + // Every other test injects a fake exec, so the production default path + // (args.exec undefined -> the real `run`) had zero coverage. Dropping the + // `?? run` fallback would hand the adapter exec: undefined and crash the first + // real `qwen review build-test`. Mock the adapter to capture the args it + // receives (so no real npm spawns) and pin that exec is a function. + writeFileSync( + join(root, 'package.json'), + JSON.stringify({ name: 'r', workspaces: ['packages/*'] }), + ); + pkg('packages/a', { name: '@x/a', scripts: { build: 'exit 0' } }); + writePlan(['packages/a/src/x.ts']); + + let receivedExec: unknown; + const runSpy = vi + .spyOn(npmToolchainAdapter, 'run') + .mockImplementation((args) => { + receivedExec = args.exec; + return { + toolchain: 'npm', + affected: [], + buildSet: [], + widenedWith: [], + install: null, + build: [], + test: [], + ok: true, + timedOut: [], + note: '', + }; + }); + + runBuildTest({ + plan: planPath, + worktree: root, + timeout: 60, + install: false, + // no `exec` — exercise the production default path + }); + + expect(receivedExec).toBeTypeOf('function'); + runSpy.mockRestore(); + }); }); diff --git a/packages/cli/src/commands/review/build-test.ts b/packages/cli/src/commands/review/build-test.ts index 3eca1118f16..2acfe20d029 100644 --- a/packages/cli/src/commands/review/build-test.ts +++ b/packages/cli/src/commands/review/build-test.ts @@ -39,54 +39,23 @@ import type { CommandModule } from 'yargs'; import { spawnSync } from 'node:child_process'; -import { - existsSync, - readFileSync, - rmSync, - statfsSync, - writeFileSync, -} from 'node:fs'; +import { existsSync, readFileSync, writeFileSync } from 'node:fs'; import { join, resolve } from 'node:path'; import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; +import { npmToolchainAdapter } from './lib/npm-toolchain.js'; import { - affectedWorkspaces, - buildSetFor, - hasUnmodeledWorkspaceGlob, - readRootPackage, - readWorkspaceGlobs, - readWorkspacePackages, - reverseDependencyClosure, - scriptFansOut, - type WorkspacePackage, -} from './lib/workspaces.js'; -import { resolveTestScope, type TestScope } from './lib/workspace-scope.js'; + selectToolchainAdapter, + type ReviewToolchainAdapter, +} from './lib/toolchain.js'; +import { type TestScope } from './lib/workspace-scope.js'; /** - * A workspace dir is interpolated into a shell command line inside double - * quotes. The dirs come from the REVIEWED repo's tree and root manifest — - * PR-authored input — and POSIX shells expand `$()` and backticks even inside - * double quotes, so an unescaped name is a command-injection path. Escape the - * characters that stay live inside double quotes. (Safe names, which is every - * real one, pass through unchanged.) POSIX scope only: on Windows `shell: - * true` is cmd.exe, where backslash escapes are not honored and `%VAR%` - * expands inside double quotes — a `"` cannot appear in a Windows dir name, - * so the breakout surface there is narrower, but the escape is not a - * cmd.exe-proof seal. + * The root toolchains build-test can select. One today; the registry exists so + * the next one is a registration rather than another branch in this file. */ -function shellArg(dir: string): string { - return `"${dir.replace(/[\\"$`]/g, '\\$&')}"`; -} - -/** The build command for a dir: the root package takes no `--workspace`. */ -function buildCommand(dir: string): string { - return dir === '.' - ? 'npm run build' - : `npm run build --workspace=${shellArg(dir)}`; -} -/** The test command for a dir: the root package takes no `--workspace`. */ -function testCommand(dir: string): string { - return dir === '.' ? 'npm test' : `npm test --workspace=${shellArg(dir)}`; -} +export const toolchainAdapters: readonly ReviewToolchainAdapter[] = [ + npmToolchainAdapter, +]; /** A command this run actually executed, and what it did. */ export interface CommandResult { @@ -106,7 +75,7 @@ export interface CommandResult { } export interface BuildTestReport { - /** `npm` when the workspace scoping applied; `unsupported` otherwise. */ + /** The scoped toolchain that ran, or `unsupported` when selection was unsafe. */ toolchain: 'npm' | 'unsupported'; /** Workspace dirs the diff changed. */ affected: string[]; @@ -220,48 +189,6 @@ export function trimOutput(s: string): string { return s.slice(0, KEEP_HEAD) + marker + s.slice(-KEEP_TAIL); } -/** - * Free-disk floors for the preflights below, in bytes. - * - * Dogfooded on a live review: with ~2.7G free, `npm ci` on this monorepo ran 33 - * seconds, died on `ENOSPC`, and the now-full disk went on to fail every agent - * scheduled after this command — a disk a command fills is not a failure that - * stays contained to that command. The installed `node_modules` here is ~1.4G, - * and npm stages cache and temp writes on the same filesystem while it - * materialises the tree, so 3 GiB is the least an install can be trusted with. - * The build phase writes far less (`dist/` and tsbuildinfo) and gets a lower - * floor — enough that a compile cannot be the thing that fills the disk. Like - * the deadline, a floor violation is skip-and-disclose, never a finding: an - * environment that cannot fit the command is not a defect in the diff. - */ -const INSTALL_MIN_FREE_BYTES = 3 * 1024 ** 3; -const BUILD_MIN_FREE_BYTES = 1024 ** 3; -/** - * Below this much remaining whole-call budget a command is NOT attempted: npm - * cannot boot and produce signal in a few hundred milliseconds, so an - * "attempt" would manufacture a fake timeout (exitCode null, ok flips false) - * where an honest notRun says exactly what happened. 15s covers an npm/vitest - * cold start with headroom for a small suite. - */ -const BUDGET_MIN_ATTEMPT_MS = 15_000; - -/** - * Free bytes on the filesystem holding `dir`, or `null` where that cannot be - * measured (`statfsSync` is not available on every platform). An unmeasurable - * disk lets the run proceed: the preflight exists to prevent failures, not to - * invent them. - */ -function freeDiskBytes(dir: string): number | null { - try { - const s = statfsSync(dir); - return s.bavail * s.bsize; - } catch { - return null; - } -} - -const gib = (bytes: number): string => (bytes / 1024 ** 3).toFixed(1); - /** * The environment every build/test/install command runs under. * @@ -288,11 +215,17 @@ export function buildRunEnv( function run(command: string, cwd: string, timeoutMs: number): CommandResult { const started = Date.now(); + // spawnSync validates `timeout` as an unsigned integer: the adapters' + // budget arithmetic can hand it a fractional value (a decimal --timeout + // or --budget), which throws ERR_OUT_OF_RANGE and kills the whole call + // with no report, or zero, which arms no kill timer at all. Coerce once + // at the one boundary every command crosses. + const deadlineMs = Math.max(1, Math.round(timeoutMs)); const r = spawnSync(command, { cwd, shell: true, encoding: 'utf8', - timeout: timeoutMs, + timeout: deadlineMs, maxBuffer: 64 * 1024 * 1024, // A build that asks a question is a build that hangs until the deadline. stdio: ['ignore', 'pipe', 'pipe'], @@ -309,38 +242,11 @@ function run(command: string, cwd: string, timeoutMs: number): CommandResult { seconds: Math.round((Date.now() - started) / 1000), timedOut, output: trimOutput(`${r.stdout ?? ''}${r.stderr ?? ''}`), - deadlineMs: timeoutMs, + deadlineMs, }; } -/** - * Workspace packages the compiler said it could not resolve. - * - * Only names that belong to a workspace of *this* repo are returned. A missing - * third-party module is a broken install or a genuine defect in the diff — not - * something a wider build set can fix — and widening on it would loop. - */ -export function unresolvedWorkspaceDeps( - output: string, - packages: WorkspacePackage[], -): string[] { - const known = new Map(packages.map((p) => [p.name, p.dir])); - const found = new Set(); - // `error TS2307: Cannot find module '@qwen-code/webui' or its corresponding - // type declarations.` — and the same shape from a bundler. - const re = /Cannot find module '([^']+)'|Could not resolve "([^"]+)"/g; - let m: RegExpExecArray | null; - while ((m = re.exec(output)) !== null) { - const name = m[1] ?? m[2]; - if (!name) continue; - // `@scope/pkg/sub` resolves against the package `@scope/pkg`. - const base = name.startsWith('@') - ? name.split('/').slice(0, 2).join('/') - : name.split('/')[0]; - if (known.has(base)) found.add(base); - } - return [...found]; -} +export { unresolvedWorkspaceDeps } from './lib/npm-toolchain.js'; interface BuildTestArgs { plan: string; @@ -406,686 +312,86 @@ function changedFilesFrom(planPath: string): string[] { } export function runBuildTest(args: BuildTestArgs): BuildTestReport { - const root = resolve(args.worktree); - const perCommandMs = args.timeout * 1000; - // The whole-call wall-clock budget for the call, in milliseconds — measured - // from the TOP of the run, so install and build time count against it. The - // default keeps 30s of headroom under the 600-second tool timeout the brief - // welds onto the call: the clock outside starts before node does, and the - // report write must still fit. The floor is one command deadline: a tiny - // --timeout must not turn the headroom into a negative budget that starves - // every suite. - const callBudgetMs = - (args.budget ?? Math.max(args.timeout, args.timeout * 2 - 30)) * 1000; - const runStarted = Date.now(); - /** Budget left for the whole call; every phase spends from it. */ - const remainingMs = (): number => callBudgetMs - (Date.now() - runStarted); - /** The deadline a timed-out command was actually given, in whole seconds. */ - const deadlineSecs = (r: CommandResult): number => - Math.round((r.deadlineMs ?? perCommandMs) / 1000); - const exec = args.exec ?? run; - const changed = changedFilesFrom(args.plan); - - // `unsupported`: build-test cannot safely scope this repo, so the agent's brief - // falls back to its build/test precedence (installing dependencies first). `ok` is - // true because nothing was found wrong — it is a handoff, not a failure. - const unsupportedReport = (note: string): BuildTestReport => ({ - toolchain: 'unsupported', - affected: [], - buildSet: [], - widenedWith: [], - install: null, - build: [], - test: [], - ok: true, - timedOut: [], - note, - }); - - const globs = readWorkspaceGlobs(root); - let { packages, skipped } = readWorkspacePackages(root); - - // The root package, read once: it decides single-root mode below, and in a - // workspace monorepo its own test suite is still a dependent the closure - // must see (a root that declares a dependency on a changed workspace). - const rootPkg = readRootPackage(root); - - // A workspace-less `package.json` with a build/test script is the most common npm - // repo shape — treat the root as a single package so it keeps the install, the - // deadline, and timeout-as-data, instead of dropping to a precedence list that no - // longer installs. Its build/test commands take no `--workspace` (dir `.`). - let singleRoot = false; - const unmodeled = globs.length > 0 && hasUnmodeledWorkspaceGlob(globs); - if (!unmodeled && globs.length === 0 && rootPkg) { - packages = [rootPkg]; - singleRoot = true; + // yargs `type: 'number'` coerces `--timeout abc` to NaN rather than + // rejecting it; NaN defeats every budget-floor comparison and reaches + // spawnSync as an invalid deadline — ERR_OUT_OF_RANGE with no report. + // Reject both flags at the one boundary every call crosses. + if (!Number.isFinite(args.timeout)) { + throw new Error( + `build-test: --timeout must be a finite number of seconds (got ${String(args.timeout)}).`, + ); } - - // `unsupported` when there is nothing to scope, OR when the layout uses a glob - // shape the walker does not model (`packages/**`, `foo-*`, `*/lib`). The second - // is load-bearing: without it, a diff inside an unmodeled workspace resolves to an - // EMPTY affected set and the report says "no package to build" — a confident false - // green for the review's one deterministic check. Falling back to the brief's - // precedence list is the safe direction. The unmodeled check comes FIRST because - // `packages/**` also makes `readWorkspacePackages` find nothing. - if ( - unmodeled || - (!singleRoot && (globs.length === 0 || packages.length === 0)) - ) { - return unsupportedReport( - unmodeled - ? 'This repo uses a workspace glob shape this command does not model ' + - '(e.g. `**`, an inner `*`, or a `foo-*` prefix), so it cannot safely decide ' + - 'which packages the diff touches. Fall back to the build/test precedence in ' + - 'your brief, and give each command a deadline it can actually meet.' - : 'No npm package here to scope (no workspaces, and the root has no build/test ' + - 'script). Fall back to the build/test precedence in your brief — installing ' + - 'dependencies first — and give each command a deadline it can actually meet.', + if (args.budget !== undefined && !Number.isFinite(args.budget)) { + throw new Error( + `build-test: --budget must be a finite number of seconds (got ${String(args.budget)}).`, ); } - - // A single-root repo builds and tests its one package whenever the diff changes - // anything; a workspace repo maps the changed files to the workspaces they live in. - const affected = singleRoot - ? changed.length > 0 - ? ['.'] - : [] - : affectedWorkspaces(changed, globs); - - // The test scope, decided up front so the report can disclose it even when - // there is nothing to run. Undefined for a single-root repo — its one suite - // is its full suite, and its report must not change shape — and for a - // build-only call: the merge-base probe runs no tests, and a testScope it - // never executed would claim a decision the run did not make. - // The root joins the graph whenever it is a package with a build or test - // script — not only when it has a TEST suite. Its declared dependencies are - // edges either way: a member that names the root as a dependency is reached - // THROUGH the root, and a build-only root dropped from the graph takes every - // such transitive dependent with it, silently. Which of the root's own - // scripts run is decided separately (build loop: its `build`; test scope: - // its `test`, unless it fans out over every workspace — see below). - let testScope = - singleRoot || args.buildOnly - ? undefined - : resolveTestScope({ - changed, - globs, - packages, - skipped, - rootPackage: rootPkg, - rootTestFansOut: rootPkg?.scripts.includes('test') - ? scriptFansOut(rootPkg.scriptsText['test']) - : false, - }); - // The SAME graph feeds the build set, so the built set and the tested set - // cannot drift apart — and it is the same graph for a build-only probe as - // for the full run, or the merge-base probe measures a different tree than - // the run it is the baseline for ("same set, same commands, same verdict"). - // The root goes FIRST: on a name collision a member must win (this repo's - // root and packages/cli share the name `@qwen-code/qwen-code`). - const scopeGraph = !singleRoot && rootPkg ? [rootPkg, ...packages] : packages; - - // With no affected workspace there is nothing to run at all. Three diffs land - // here: an empty one; a build-only call (the merge-base probe), which measures - // nothing about this PR's tests by design; and a diff the workspaces cannot - // feel (the license family, or a member a negation excludes). Anything else - // outside the workspaces is disclosed through testScope.caveat — there is no - // full-suite fallback that could cover it (see the test phase below). - if (affected.length === 0) { + const root = resolve(args.worktree); + const changedFiles = changedFilesFrom(args.plan); + const runArgs = { + root, + changedFiles, + timeout: args.timeout, + install: args.install, + buildOnly: args.buildOnly, + budget: args.budget, + exec: args.exec ?? run, + }; + const { adapter, applicable } = selectToolchainAdapter( + root, + toolchainAdapters, + ); + if (!adapter) { + if (applicable.length > 1) { + // Unreachable with one registered adapter, and deliberately kept: the + // selection contract is "exactly one, or nothing", and the second + // adapter must land in a file that already refuses to guess between + // them rather than one that has to grow the branch. + return { + toolchain: 'unsupported', + affected: [], + buildSet: [], + widenedWith: [], + install: null, + build: [], + test: [], + ok: true, + timedOut: [], + note: + 'More than one toolchain applies at the repository root. build-test will ' + + 'not guess which one owns this diff, so it ran nothing — report the ' + + 'ambiguity as a handoff instead of substituting ad hoc build or test ' + + 'commands.', + }; + } + // A root package.json marks an npm-shaped repo that npm's own gate refused + // (an unmodeled workspace glob, workspaces that resolve to no package, or + // no root build/test script). Delegate the handoff to the npm adapter so + // the report carries its precise reason instead of the generic one — an + // agent told "no npm project here" about a repo that IS one gets a worse + // steer than the shape it cannot scope named. run() returns its + // unsupported report before executing any command on every root where + // applies() is false. + if (existsSync(join(root, 'package.json'))) { + return npmToolchainAdapter.run(runArgs); + } return { - toolchain: 'npm', + toolchain: 'unsupported', affected: [], buildSet: [], widenedWith: [], install: null, build: [], test: [], - ...(testScope ? { testScope } : {}), ok: true, timedOut: [], - note: args.buildOnly - ? `The diff changes ${changed.length} file(s), none of them inside a ` + - 'workspace. There is no package to build, and tests are out of scope ' + - 'for a build-only probe.' - : testScope?.caveat - ? `The diff changes ${changed.length} file(s), none of them inside a ` + - 'workspace. There is no package to build and no test to run, but ' + - `the scope decision recorded a caveat: ${testScope.caveat}.` - : `The diff changes ${changed.length} file(s), none of them inside a ` + - "workspace (nothing the workspaces' tests can feel). There is no " + - 'package to build and no test to run — this is a complete answer, ' + - 'not a skipped step.', - }; - } - - // The dir→package map is built from the SCOPE GRAPH, not the workspace list - // alone: when the root joins the graph, a member that names it as a - // dependency puts `.` in the build set, and the root's own `build` must run - // like any other package's — skipping it would compile dependents against - // artifacts of the root that were never produced. - const byDir = new Map(scopeGraph.map((p) => [p.dir, p])); - - // A changed dir the walker mapped to something that is NOT a package (a nested - // package listed before a `*` that also claims its parent segment; a loose file - // directly under a `packages/*` base) would be dropped from the build set without - // a trace: zero commands, `ok: true`, "Everything passed" — the confident false - // green this command exists to prevent. If any affected dir is not a known - // package, the scoping cannot be trusted; hand the whole thing to the brief's - // precedence rather than certify a build that never ran. - const unmapped = affected.filter((d) => d !== '.' && !byDir.has(d)); - if (unmapped.length > 0) { - return unsupportedReport( - `The diff touches ${unmapped.join(', ')}, which the workspace globs map to no ` + - 'package (a nested package ordered before a `*`, or a loose file under a ' + - 'workspace base). Scoping cannot be trusted here, so fall back to the ' + - 'build/test precedence in your brief — installing dependencies first — rather ' + - 'than trust a scoped build that would silently skip it.', - ); - } - - // No `testScope` in the initializer: every return that fires before the - // test loop runs zero suites, and a scope on it would read as "the suites - // ran" in the agent's brief. It is attached only once the scope executes. - const results: BuildTestReport = { - toolchain: 'npm', - affected, - buildSet: [], - widenedWith: [], - install: null, - build: [], - test: [], - ok: true, - timedOut: [], - note: '', - }; - - // The install. It lives here, not in the orchestrator, because nothing before - // this command needs `node_modules`: the eleven diff-reading agents read the - // diff and grep the source. Run from the orchestrator it blocks the fan-out; - // run here it overlaps the other agents, which are still reading. - // - // A non-zero exit is NOT the end of the run, and finding that out cost a live - // review. `npm ci` executes the project's `prepare` lifecycle script, and this - // repo's runs `npm run build` and `npm run bundle` — the whole monorepo. On the - // PR under review that build hit a **pre-existing** type error in a package the - // diff does not touch, `npm ci` exited 1, and this command gave up having built - // and tested nothing: the one deterministic signal a review has, withheld - // because an unrelated package failed to compile during an install. - // - // The packages were installed. `node_modules` was on disk. So the test is not - // the exit code, it is whether the tree we need is there — and the scoped build - // below is the authoritative answer anyway. Report the install failure, and - // carry on to ask the question the review actually came to ask. - // - // A **timeout** is the exception, and it is not the same case. A `prepare` hook - // that fails leaves a *complete* `node_modules` and only the post-install build - // broken; a timeout kills `npm ci` mid-download and leaves a **partial** tree. - // Building against that produces "module not found" errors that look like defects - // in the diff and are not — so a timed-out install aborts, exactly like an install - // that left no tree at all. - // - // Whether to install is gated on npm's **completeness marker**, not the bare - // directory. `npm ci` writes `node_modules/.package-lock.json` only once the tree - // is fully materialised, so a partial tree — left by a timeout here, or by the - // agent's own shell-tool kill one level up — has the directory but not the marker. - // Gating on the directory would let every later run *skip* the install and build - // against that partial tree; gating on the marker reinstalls it. - // - // But `npm ci` is only right for an npm repo. `workspaces` is also yarn/bun/pnpm - // syntax, and those write no `package-lock.json`, so `npm ci` would fail-fast on - // the missing lockfile and mislabel a perfectly usable `node_modules` as a failed - // install. So install only when there IS a `package-lock.json` (an npm repo) whose - // tree is incomplete; a non-npm repo that already has a tree is trusted — the build - // is the authoritative signal, by this command's own argument. - const npmLock = existsSync(join(root, 'package-lock.json')); - const installComplete = (): boolean => - existsSync(join(root, 'node_modules', '.package-lock.json')); - - // A non-npm repo (yarn/bun/pnpm — `workspaces` is their syntax too) with no - // installed tree cannot be installed here: `npm ci` needs the npm lockfile, and - // building against absent dependencies fails with `Cannot find module` **inside the - // PR's own changed files** — the false-Critical steer this command exists to - // prevent. A review worktree is cold by construction, so this is the common case, - // not an edge. Hand it to the brief, naming the tool to install with. (The warm - // case — a tree already present — is trusted below and never reaches here.) - if (args.install && !npmLock && !existsSync(join(root, 'node_modules'))) { - const altLock = [ - ['yarn.lock', 'yarn install --frozen-lockfile'], - ['pnpm-lock.yaml', 'pnpm install --frozen-lockfile'], - ['bun.lockb', 'bun install --frozen-lockfile'], - ['bun.lock', 'bun install --frozen-lockfile'], - ].find(([f]) => existsSync(join(root, f))); - return unsupportedReport( - altLock - ? `This is a ${altLock[0]} repo with no installed \`node_modules\`, so \`npm ci\` ` + - `cannot install it. Run \`${altLock[1]}\` first, then fall back to the ` + - 'build/test precedence in your brief, each command with a deadline it can meet.' - : 'There is no lockfile and no `node_modules` here, so nothing can be installed ' + - 'deterministically. Install dependencies first, then fall back to the ' + - 'build/test precedence in your brief.', - ); - } - if (args.install && npmLock && !installComplete()) { - // Disk preflight. The deadline already treats "cannot finish in time" as an - // infrastructure result and skips ahead with a disclosure; "cannot fit on - // the disk" is the same class of result, discovered before the command runs - // instead of 33 seconds into it. An `npm ci` that dies on ENOSPC is - // strictly worse than one that never starts: it leaves a partial tree AND a - // full disk that fails every agent scheduled after this one. - const installCmd = 'npm ci --no-audit --no-fund'; - const free = freeDiskBytes(root); - if (free !== null && free < INSTALL_MIN_FREE_BYTES) { - results.ok = false; - results.note = - `Insufficient disk space (${gib(free)}G free, need ~${gib(INSTALL_MIN_FREE_BYTES)}G): ` + - `skipped \`${installCmd}\`, so nothing could be built or tested. This ` + - 'is an environment issue, not a code finding — report it as ' + - 'informational.'; - return results; - } - if (remainingMs() < BUDGET_MIN_ATTEMPT_MS) { - // The same floor as the build/test loops: a sub-second `npm ci` cannot - // produce anything but a fake timeout, so skip and disclose instead. - results.ok = false; - results.note = - `The whole-call budget was spent before the install could start ` + - `(${args.budget != null ? `--budget ${args.budget}s` : 'default budget'}), ` + - 'so nothing could be built or tested. This is an infrastructure ' + - 'result, not a defect in the diff — report it as informational.'; - return results; - } - const install = exec( - installCmd, - root, - Math.min(perCommandMs, remainingMs()), - ); - results.install = install; - if (install.timedOut) results.timedOut.push(install.command); - // A timeout leaves a partial tree — remove it, so this is not mistaken next time - // for a complete install to build against. `spawnSync`'s SIGTERM only kills the - // direct shell; the orphaned `npm`/`node` grandchildren keep writing the tree, so - // `rmSync` can race them and throw `ENOTEMPTY` — which must not replace the whole - // report with a raw error. Best-effort with retries; the marker gate below still - // decides the outcome. - if (install.timedOut) { - try { - rmSync(join(root, 'node_modules'), { - recursive: true, - force: true, - maxRetries: 3, - }); - } catch { - // Best effort — a partial tree left behind is caught by the marker gate. - } - } - if (install.timedOut || !installComplete()) { - results.ok = false; - results.note = install.timedOut - ? `\`${install.command}\` ran out of time (${deadlineSecs(install)}s) and left an ` + - 'incomplete `node_modules`, so nothing could be built or tested against it. ' + - 'This is an infrastructure result, not a defect in the diff — report it as ' + - 'informational.' - : 'The install failed and left no usable `node_modules`, so nothing could be ' + - 'built or tested. This is an environment failure, not a defect in the diff — ' + - 'report it as informational.'; - return results; - } - } - - // The same preflight before the build phase, at a lower floor. A warm tree - // skips the install (and its 3 GiB gate) entirely, but a compile that hits - // ENOSPC mid-write fails with errors that read as defects in the diff — and - // leaves the disk full for everything that runs after this command. - const freeForBuild = freeDiskBytes(root); - if (freeForBuild !== null && freeForBuild < BUILD_MIN_FREE_BYTES) { - results.ok = false; - results.note = - `Insufficient disk space (${gib(freeForBuild)}G free, need ~${gib(BUILD_MIN_FREE_BYTES)}G): ` + - 'skipped the build and tests rather than fill the disk mid-compile. This ' + - 'is an environment issue, not a code finding — report it as informational.'; - return results; - } - - const alsoBuild: string[] = []; - let set = buildSetFor(affected, scopeGraph); - const built = new Set(); - const widened = new Set(); - // A root build that fans out over the workspaces (`npm run build - // --workspaces`) is an aggregator: it produces no artifacts of its own, the - // scoped loop already builds the members it drives, and as one bare command - // it is exactly the whole-monorepo build this module exists to stop - // running. Only a NON-fan-out root build — one that compiles the root's own - // sources — is worth its deadline. - const rootBuildRuns = - !!rootPkg?.scripts.includes('build') && - !scriptFansOut(rootPkg.scriptsText['build']); - // One predicate for both the loop skip and the reported set: a fan-out - // root's build does not run — never in single-root mode, where the root is - // the only package there is. - const rootBuildSkipped = !singleRoot && !rootBuildRuns; - const notBuilt: string[] = []; - - // Build, and let the compiler correct the set. Three widenings is generous: each - // one is a package the graph could not have known about, and a fourth would mean - // the graph is not wrong but absent. Every command spends from the same - // whole-call budget as the tests — an unbounded build phase would hand the - // outer shell kill a report the budget exists to save. - for (let attempt = 0; attempt <= 3; attempt++) { - let failure: CommandResult | null = null; - - for (const dir of set) { - if (built.has(dir)) continue; - const pkg = byDir.get(dir); - if (!pkg?.scripts.includes('build')) { - built.add(dir); // Nothing to build is not a failure to build. - continue; - } - if (dir === '.' && rootBuildSkipped) { - // Fan-out aggregator root: the members it drives are built by this - // very loop; the bare `npm run build` would re-build all of them - // inside one deadline (see above). - built.add(dir); - continue; - } - if (remainingMs() < BUDGET_MIN_ATTEMPT_MS) { - // The budget is spent: stop building and disclose. Suites of unbuilt - // packages must not run either — a suite against artifacts never - // compiled manufactures failures the diff did not cause (the exact - // lesson of the scoped-build/full-test cascade). - notBuilt.push( - ...set.filter( - (d) => !built.has(d) && byDir.get(d)?.scripts.includes('build'), - ), - ); - break; - } - const r = exec( - buildCommand(dir), - root, - Math.min(perCommandMs, remainingMs()), - ); - results.build.push(r); - if (r.timedOut) results.timedOut.push(r.command); - if (r.exitCode !== 0) { - failure = r; - break; - } - built.add(dir); - } - - if (!failure) break; - - // Did it fail because the set was too small — or mis-ordered? The declared graph - // under-approximates whenever a package reaches into another's *sources* (a - // tsconfig `paths` entry into `../cli/src/...` compiles that package's imports - // without declaring a dependency), and the compiler names the package it could - // not resolve. Filter on `!built.has(dir)`, not `!set.includes(dir)`: when BOTH - // the needer and the undeclared-needed package are affected and the alphabet - // ordered the needer first, the named package is already IN the set but not yet - // built — re-seeding it into `alsoBuild` (which sorts first) fixes the order. The - // attempt cap bounds the loop; a package that is truly missing is not in the map. - // - // A **timeout** must not enter this path. A build killed at the deadline leaves - // partial output that can happen to contain a `Cannot find module` line, which - // would look like a too-small build set and trigger a retry — another full - // deadline, and another, up to the attempt cap. A timeout is infrastructure, not - // a graph gap: report it and stop, the same way the install path does. - const missing = failure.timedOut - ? [] - : unresolvedWorkspaceDeps(failure.output, packages).filter((name) => { - const dir = packages.find((p) => p.name === name)?.dir; - return dir && !built.has(dir); - }); - if (missing.length === 0 || failure.timedOut || attempt === 3) { - results.ok = false; - results.note = failure.timedOut - ? `\`${failure.command}\` ran out of time (${deadlineSecs(failure)}s). That is an ` + - 'infrastructure result, not a defect in the diff — report it as informational.' - : `\`${failure.command}\` failed. Correlate the errors below with the diff: a ` + - 'compile error in a file the PR changed is a Critical; one in a file it did not ' + - 'touch is a pre-existing failure, and belongs in the terminal, not on the PR.'; - results.buildSet = ( - rootBuildSkipped ? set.filter((d) => d !== '.') : set - ).filter((d) => !notBuilt.includes(d)); - results.widenedWith = [...widened]; - return results; - } - - // Drop the failed attempt from the report. It is about to be retried with the - // package it asked for, and it is **not evidence about this PR**: the build set - // was too small, which is this command's mistake, not the author's. Left in - // `build[]`, an agent told "a build failure in a changed file is a Critical" - // reads `packages/vscode-ide-companion rc=2` and files exactly that — a public - // blocker on a PR whose build passes. (A timed-out failure cannot reach here — it - // is terminal above — so only `build[]`, never `timedOut`, can hold it.) - results.build = results.build.filter((r) => r !== failure); - - for (const name of missing) widened.add(name); - for (const name of missing) { - const dir = packages.find((p) => p.name === name)?.dir; - if (dir) alsoBuild.push(dir); - } - // As `alsoBuild`, never as `affected`. The compiler asked for this package - // because something compiles *against* it; the PR did not change it, so its - // consumers cannot have been broken by the PR and must not be built. - set = buildSetFor(affected, scopeGraph, alsoBuild); - } - - // The build set reports what was (to be) BUILT: a fan-out root whose build - // was skipped — an aggregator the loop already covered member by member — - // and packages the budget stopped before building must not linger in it, or - // the report names builds that never ran. - results.buildSet = ( - rootBuildSkipped ? set.filter((d) => d !== '.') : set - ).filter((d) => !notBuilt.includes(d)); - results.widenedWith = [...widened]; - if (notBuilt.length > 0) results.notBuilt = [...notBuilt].sort(); - - // Test what the diff can break: the changed workspaces plus their - // reverse-dependency closure — exactly the suites that define a test script. - // Testing the changed ones alone under-tests in the one way a compile cannot - // catch: a behaviour change in `core` leaves every dependent compiling and - // still fails their suites. The closure is a subset of the build set (which - // adds compile-time dependencies on top), so every tested package was built - // above, with everything it compiles against. - // - // When the scope decision recorded a caveat — a graph it could not fully - // compute, a changed file outside every workspace, a closure past half the - // testable suites — the scoped set still runs and the caveat discloses what - // it may miss. There is NO fallback to the repo's root `npm test`: on a - // large monorepo that command cannot finish inside a command deadline (this - // repo's suite took 31 minutes in CI against a 300-second deadline, and a - // third of recent diffs would have hit the fallback), so the fallback would - // only ever report a timeout — zero signal framed as a failure. The scoped - // set is the run that covers the diff — each command keeps its own deadline. - // - // Those per-command deadlines SUM, though, and a large closure can sum past - // the whole-call ceiling the brief welds on (600s by default) — the outer - // shell kill then discards the report entirely. So the loop below runs - // against a whole-call budget that EVERY phase (install, builds, tests) - // spends from: each command gets the smaller of its own deadline and what - // remains. A suite killed at the budget boundary is a timeout — already - // framed as infrastructure — and a partial attempt is signal where a - // never-attempted suite is none. Below the floor an attempt cannot even - // boot npm, so the suite goes to notRun instead of manufacturing a fake - // timeout. A partial report is signal; a discarded one is the "71 - // timeouts, nothing verified" failure this command exists to end. - const rootHasTest = !!rootPkg?.scripts.includes('test'); - const testDirs = args.buildOnly - ? [] - : !testScope - ? affected // single root: its one package, exactly as before scoping - : testScope.workspaces; - const runnable = (dir: string): boolean => - dir === '.' ? rootHasTest : !!byDir.get(dir)?.scripts.includes('test'); - // Affected first: the changed workspace's own suite is the highest-value - // one and must be unstarvable — the dependents are the widening, and the - // widening is what a budget should trim. (The closure is alphabetical, so - // without this a `zebra` change would run `alpha`'s suite and starve its - // own.) - const affectedSet = new Set(affected); - const runnableDirs = [ - ...testDirs.filter((d) => affectedSet.has(d) && runnable(d)), - ...testDirs.filter((d) => !affectedSet.has(d) && runnable(d)), - ]; - // Suites of packages the budget left UNBUILT cannot run — against artifacts - // never compiled, their failures would be manufactured, not measured. - const untestable = - notBuilt.length > 0 - ? new Set(reverseDependencyClosure(notBuilt, scopeGraph)) - : new Set(); - const notRun: string[] = []; - for (let i = 0; i < runnableDirs.length; i++) { - const dir = runnableDirs[i]; - if (untestable.has(dir)) { - notRun.push(dir); - continue; - } - const remaining = remainingMs(); - if (remaining < BUDGET_MIN_ATTEMPT_MS) { - // Below the floor an "attempt" cannot even boot npm — it would - // manufacture a fake timeout where an honest notRun says what happened. - notRun.push(...runnableDirs.slice(i).filter((d) => !untestable.has(d))); - break; - } - const r = exec(testCommand(dir), root, Math.min(perCommandMs, remaining)); - results.test.push(r); - if (r.timedOut) results.timedOut.push(r.command); - if (r.exitCode !== 0) results.ok = false; - } - - // A budget stop is STRUCTURAL, not just prose: `testScope.workspaces` is - // documented (and quoted by the agent's brief) as exactly the suites that - // ran, so the trimmed suites leave it, and `notRun` names them. Sorted, so - // both fields are stable and comparable. - notRun.sort(); - const partialNote = - [ - notBuilt.length > 0 - ? `the build phase reached the whole-call budget — not built: ` + - notBuilt.join(', ') - : '', - notRun.length > 0 - ? `the whole-call budget (${Math.round(callBudgetMs / 1000)}s) was ` + - `spent with ${notRun.length} suite(s) still to run — not run: ` + - notRun.join(', ') - : '', - ] - .filter(Boolean) - .join('; ') || undefined; - if (testScope && partialNote) { - const ran = testScope.workspaces.filter((d) => !notRun.includes(d)); - testScope = { - workspaces: ran, - ...(notRun.length > 0 ? { notRun } : {}), - caveat: testScope.caveat - ? `${testScope.caveat}; ${partialNote}` - : partialNote, + note: + 'No supported npm project here to scope. Fall back to the ' + + 'build/test precedence in your brief — installing dependencies first — ' + + 'and give each command a deadline it can actually meet.', }; } - - // The scope was executed — only now may the report carry it. Every return - // between the initializer and here ran zero test commands and must not - // claim a scoping decision; the one exception, the nothing-to-run answer - // above, carries the scope precisely because the empty scope IS the answer. - if (testScope) results.testScope = testScope; - - if (!results.note) { - const failed = [...results.build, ...results.test].filter( - (r) => r.exitCode !== 0, - ); - // A timeout is a failure (its exitCode is null), but it is NOT a defect in the - // diff, and the note must not tell the agent to correlate it with one — the - // brief says timeouts are infrastructure, and an agent trusts the data over its - // instructions. So a test that runs out of time gets the same infrastructure - // framing the build-timeout path already gives, not the "a failure is a Critical" - // message meant for a real compile/assertion failure. - const realFailures = failed.filter((r) => !r.timedOut); - if (results.ok) { - // The tests sentence names the scope, because it is the agent's report - // that has to be able to say what was and was not run: a scoped run - // names its suites, and a caveat says what the scope may miss. - let testsClause: string; - if (args.buildOnly) { - testsClause = '. Tests were not run (build-only).'; - } else if (!testScope) { - testsClause = - results.test.length === 0 - ? ', but the package defines no test script, so no tests ran.' - : ' and ran the tests of the changed ones. Everything passed.'; - } else if (testScope.workspaces.length === 0) { - testsClause = testScope.notRun?.length - ? ', but the whole-call budget was spent before any suite could run.' - : ', but no workspace in scope defines a test script, so no tests ran.'; - } else { - // The scoped list is filtered to dependents WITH a test script; a - // build-only dependent is built but never tested, so the note must - // not claim every declared dependent was covered. - testsClause = - ` and ran the tests scoped to ${testScope.workspaces.join(', ')} — ` + - 'the changed workspaces and every workspace declared to depend on ' + - 'them that defines a test script. Everything passed.'; - } - if (testScope?.caveat) testsClause += ` Caveat: ${testScope.caveat}.`; - // The root is not a workspace: count it separately, or a 22-member repo - // reports "of 23" — a number in a report whose thesis is honest numbers. - // (A single-root repo's one package IS '.', and counts as the one.) - const builtWorkspaces = results.buildSet.filter( - (d) => singleRoot || d !== '.', - ).length; - const rootSuffix = - !singleRoot && results.buildSet.includes('.') && !rootBuildSkipped - ? ' (plus the root package)' - : ''; - results.note = - `Built ${builtWorkspaces} of ${packages.length} workspaces${rootSuffix} (the ${affected.length} the ` + - `diff changes, plus what they compile against${ - widened.size - ? `, plus ${[...widened].join(', ')} the compiler asked for` - : '' - })${testsClause}`; - } else if (realFailures.length === 0) { - results.note = - `${failed.length} command(s) ran out of time (${deadlineSecs(failed[0])}s). A timeout is an ` + - 'infrastructure result, not a defect in the diff — report it as informational.'; - } else { - results.note = - `${realFailures.length} command(s) failed. Correlate each error with the diff: a failure in a ` + - 'file the PR changed is a Critical; one in a file it did not touch is pre-existing.' + - (failed.length > realFailures.length - ? ' (Commands that timed out are infrastructure, not findings.)' - : ''); - } - } - - // A failure note must carry the caveat too — the note is what the brief - // renders first, and "a test failed AND the budget dropped suites" must not - // read as a plain failure. (The ok branch already appended it above.) - if (results.testScope?.caveat && !results.note.includes('Caveat:')) { - results.note += ` Caveat: ${results.testScope.caveat}.`; - } - - // Single-root repos carry no testScope, so a budget stop is disclosed on - // the note itself. (With a scope, the caveat above already says it.) - if (partialNote && !results.testScope) { - results.note = results.note - ? `${results.note} ${partialNote}.` - : partialNote; - } - - // The install exited non-zero but left a usable tree, so the run went ahead. Say - // so — the build and test results below are real, and the install failure is not - // a finding about this PR. (A `prepare` script that builds the whole project, - // as this repo's does, fails on any pre-existing error anywhere in it.) - if (results.install && results.install.exitCode !== 0) { - results.note = - `\`${results.install.command}\` exited ${results.install.exitCode} but left a usable ` + - '`node_modules`, so the build and test below ran anyway and their results stand. ' + - 'The install failure is an environment/infrastructure result — report it as ' + - 'informational, never as a Critical, and never against this PR. ' + - results.note; - } - return results; + return adapter.run(runArgs); } export const buildTestCommand: CommandModule = { @@ -1138,7 +444,8 @@ export const buildTestCommand: CommandModule = { .option('install', { type: 'boolean', default: true, - describe: 'Run `npm ci` first when node_modules is absent', + describe: + 'Fetch dependencies first: `npm ci` when node_modules is absent', }) .option('build-only', { type: 'boolean', diff --git a/packages/cli/src/commands/review/lib/disk.ts b/packages/cli/src/commands/review/lib/disk.ts new file mode 100644 index 00000000000..d3a3414f2c7 --- /dev/null +++ b/packages/cli/src/commands/review/lib/disk.ts @@ -0,0 +1,41 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { statfsSync } from 'node:fs'; + +/** + * Free-disk floors for the toolchain preflights, in bytes. + * + * Dogfooded on a live review: with ~2.7G free, `npm ci` on this monorepo ran 33 + * seconds, died on `ENOSPC`, and the now-full disk went on to fail every agent + * scheduled after this command — a disk a command fills is not a failure that + * stays contained to that command. The installed `node_modules` here is ~1.4G, + * and npm stages cache and temp writes on the same filesystem while it + * materialises the tree, so 3 GiB is the least an install can be trusted with. + * The build phase writes far less (`dist/` and tsbuildinfo) and gets a lower + * floor — enough that a compile cannot be the thing that fills the disk. + * Like the deadline, a floor violation is skip-and-disclose, never a finding: + * an environment that cannot fit the command is not a defect in the diff. + */ +export const INSTALL_MIN_FREE_BYTES = 3 * 1024 ** 3; +export const BUILD_MIN_FREE_BYTES = 1024 ** 3; + +/** + * Free bytes on the filesystem holding `dir`, or `null` where that cannot be + * measured (`statfsSync` is not available on every platform). An unmeasurable + * disk lets the run proceed: the preflight exists to prevent failures, not to + * invent them. + */ +export function freeDiskBytes(dir: string): number | null { + try { + const s = statfsSync(dir); + return s.bavail * s.bsize; + } catch { + return null; + } +} + +export const gib = (bytes: number): string => (bytes / 1024 ** 3).toFixed(1); diff --git a/packages/cli/src/commands/review/lib/npm-toolchain.test.ts b/packages/cli/src/commands/review/lib/npm-toolchain.test.ts new file mode 100644 index 00000000000..700a6198904 --- /dev/null +++ b/packages/cli/src/commands/review/lib/npm-toolchain.test.ts @@ -0,0 +1,233 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { npmToolchainAdapter } from './npm-toolchain.js'; +import { selectToolchainAdapter } from './toolchain.js'; + +const statfsSyncMock = vi.hoisted(() => vi.fn()); +vi.mock('node:fs', async (importOriginal) => { + const actual = (await importOriginal()) as Record; + const mock = { ...actual, statfsSync: statfsSyncMock }; + return { ...mock, default: mock }; +}); + +// Plenty of disk by default, so this suite behaves the same on a nearly-full +// machine as on an empty one — the low-disk case below opts in explicitly. +beforeEach(() => { + statfsSyncMock.mockReturnValue({ bavail: 16 * 1024 ** 3, bsize: 1 }); +}); + +const okExec = (command: string) => ({ + command, + exitCode: 0, + seconds: 1, + timedOut: false, + output: '', +}); + +describe('npm toolchain adapter', () => { + let root: string; + + beforeEach(() => { + root = mkdtempSync(join(tmpdir(), 'npm-toolchain-')); + }); + + afterEach(() => { + rmSync(root, { recursive: true, force: true }); + }); + + it('selects the npm adapter for a repository with package.json', () => { + writeFileSync( + join(root, 'package.json'), + JSON.stringify({ name: 'root', workspaces: ['packages/*'] }), + ); + mkdirSync(join(root, 'packages', 'a'), { recursive: true }); + writeFileSync( + join(root, 'packages', 'a', 'package.json'), + JSON.stringify({ name: '@x/a', scripts: { build: 'exit 0' } }), + ); + + expect(selectToolchainAdapter(root, [npmToolchainAdapter])).toEqual({ + adapter: npmToolchainAdapter, + applicable: [npmToolchainAdapter], + }); + expect(npmToolchainAdapter.applies(root)).toBe(true); + }); + + it('does not select the npm adapter for a non-npm repository', () => { + expect(selectToolchainAdapter(root, [npmToolchainAdapter])).toEqual({ + adapter: null, + applicable: [], + }); + expect(npmToolchainAdapter.applies(root)).toBe(false); + }); + + it('fails closed when more than one adapter applies', () => { + writeFileSync( + join(root, 'package.json'), + JSON.stringify({ scripts: { build: 'exit 0' } }), + ); + const other = { applies: () => true, run: npmToolchainAdapter.run }; + + // The applicable list is walked once and returned with the selection, so + // the caller's ambiguity note does not re-walk the workspace trees. + expect(selectToolchainAdapter(root, [npmToolchainAdapter, other])).toEqual({ + adapter: null, + applicable: [npmToolchainAdapter, other], + }); + }); + + it('returns the unchanged report shape for a supported single-root package', () => { + writeFileSync( + join(root, 'package.json'), + JSON.stringify({ + name: 'root', + scripts: { build: 'exit 0', test: 'exit 0' }, + }), + ); + + expect( + npmToolchainAdapter.run({ + root, + changedFiles: ['src/index.ts'], + timeout: 5, + budget: 600, + install: false, + exec: okExec, + }), + ).toEqual({ + toolchain: 'npm', + affected: ['.'], + buildSet: ['.'], + widenedWith: [], + install: null, + build: [ + { + command: 'npm run build', + exitCode: 0, + seconds: 1, + timedOut: false, + output: '', + }, + ], + test: [ + { + command: 'npm test', + exitCode: 0, + seconds: 1, + timedOut: false, + output: '', + }, + ], + ok: true, + timedOut: [], + note: + 'Built 1 of 1 workspaces (the 1 the diff changes, plus what they compile ' + + 'against) and ran the tests of the changed ones. Everything passed.', + }); + }); + + it('keeps an unmodeled npm layout on the structured unsupported path', () => { + writeFileSync( + join(root, 'package.json'), + JSON.stringify({ name: 'root', workspaces: ['packages/**'] }), + ); + + const report = npmToolchainAdapter.run({ + root, + changedFiles: ['packages/a/src/x.ts'], + timeout: 5, + install: false, + exec: okExec, + }); + expect(report).toEqual({ + toolchain: 'unsupported', + affected: [], + buildSet: [], + widenedWith: [], + install: null, + build: [], + test: [], + ok: true, + timedOut: [], + note: + 'This repo uses a workspace glob shape this command does not model ' + + '(e.g. `**`, an inner `*`, or a `foo-*` prefix), so it cannot safely decide ' + + 'which packages the diff touches. Fall back to the build/test precedence in ' + + 'your brief, and give each command a deadline it can actually meet.', + }); + }); + + it('reports insufficient disk space instead of building on a full disk', () => { + statfsSyncMock.mockReturnValue({ bavail: 5.4e8, bsize: 1 }); // ~0.5G free + writeFileSync( + join(root, 'package.json'), + JSON.stringify({ + name: 'root', + scripts: { build: 'exit 0', test: 'exit 0' }, + }), + ); + + const report = npmToolchainAdapter.run({ + root, + changedFiles: ['src/index.ts'], + timeout: 5, + install: false, + exec: okExec, + }); + + expect(report.ok).toBe(false); + expect(report.build).toEqual([]); + expect(report.test).toEqual([]); + expect(report.note).toContain('Insufficient disk space'); + }); + + it('does not treat a workspace repo as single-root when the root has scripts', () => { + // The single-root guard must fire ONLY for a workspace-less repo. A monorepo + // whose root package.json also has build/test scripts (this repo's shape) must + // still map the diff to its workspace. Forcing the guard on would set + // singleRoot, scope the build to '.', and silently skip the changed workspace. + writeFileSync( + join(root, 'package.json'), + JSON.stringify({ + name: 'root', + workspaces: ['packages/*'], + scripts: { build: 'exit 0', test: 'exit 0' }, + }), + ); + mkdirSync(join(root, 'packages', 'a'), { recursive: true }); + writeFileSync( + join(root, 'packages', 'a', 'package.json'), + JSON.stringify({ + name: '@x/a', + scripts: { build: 'exit 0', test: 'exit 0' }, + }), + ); + + const report = npmToolchainAdapter.run({ + root, + changedFiles: ['packages/a/src/x.ts'], + timeout: 5, + budget: 600, + install: false, + exec: okExec, + }); + + expect(report.toolchain).toBe('npm'); + // The diff maps to the workspace, NOT the root package. + expect(report.affected).toEqual(['packages/a']); + expect(report.build.map((b) => b.command)).toEqual([ + 'npm run build --workspace="packages/a"', + ]); + expect(report.test.map((t) => t.command)).toEqual([ + 'npm test --workspace="packages/a"', + ]); + }); +}); diff --git a/packages/cli/src/commands/review/lib/npm-toolchain.ts b/packages/cli/src/commands/review/lib/npm-toolchain.ts new file mode 100644 index 00000000000..81f3ea27be5 --- /dev/null +++ b/packages/cli/src/commands/review/lib/npm-toolchain.ts @@ -0,0 +1,823 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { existsSync, rmSync } from 'node:fs'; +import { join } from 'node:path'; +import type { BuildTestReport, CommandResult } from '../build-test.js'; +import { + BUILD_MIN_FREE_BYTES, + INSTALL_MIN_FREE_BYTES, + freeDiskBytes, + gib, +} from './disk.js'; +import { + affectedWorkspaces, + buildSetFor, + hasUnmodeledWorkspaceGlob, + readRootPackage, + readWorkspaceGlobs, + readWorkspacePackages, + reverseDependencyClosure, + scriptFansOut, + type WorkspacePackage, +} from './workspaces.js'; +import { resolveTestScope, type TestScope } from './workspace-scope.js'; +import type { ReviewToolchainAdapter, ToolchainRunArgs } from './toolchain.js'; + +/** + * Below this much remaining whole-call budget a command is NOT attempted: npm + * cannot boot and produce signal in a few hundred milliseconds, so an + * "attempt" would manufacture a fake timeout (exitCode null, ok flips false) + * where an honest notRun says exactly what happened. 15s covers an npm/vitest + * cold start with headroom for a small suite. + */ +const BUDGET_MIN_ATTEMPT_MS = 15_000; + +/** + * A workspace dir is interpolated into a shell command line inside double + * quotes. The dirs come from the REVIEWED repo's tree and root manifest — + * PR-authored input — and POSIX shells expand `$()` and backticks even inside + * double quotes, so an unescaped name is a command-injection path. Escape the + * characters that stay live inside double quotes. (Safe names, which is every + * real one, pass through unchanged.) POSIX scope only: on Windows `shell: + * true` is cmd.exe, where backslash escapes are not honored and `%VAR%` + * expands inside double quotes — a `"` cannot appear in a Windows dir name, + * so the breakout surface there is narrower, but the escape is not a + * cmd.exe-proof seal. + */ +function shellArg(dir: string): string { + return `"${dir.replace(/[\\"$`]/g, '\\$&')}"`; +} + +/** The build command for a dir: the root package takes no `--workspace`. */ +function buildCommand(dir: string): string { + return dir === '.' + ? 'npm run build' + : `npm run build --workspace=${shellArg(dir)}`; +} +/** The test command for a dir: the root package takes no `--workspace`. */ +function testCommand(dir: string): string { + return dir === '.' ? 'npm test' : `npm test --workspace=${shellArg(dir)}`; +} + +/** + * Workspace packages the compiler said it could not resolve. + * + * Only names that belong to a workspace of *this* repo are returned. A missing + * third-party module is a broken install or a genuine defect in the diff — not + * something a wider build set can fix — and widening on it would loop. + */ +export function unresolvedWorkspaceDeps( + output: string, + packages: WorkspacePackage[], +): string[] { + const known = new Map(packages.map((p) => [p.name, p.dir])); + const found = new Set(); + // `error TS2307: Cannot find module '@qwen-code/webui' or its corresponding + // type declarations.` — and the same shape from a bundler. + const re = /Cannot find module '([^']+)'|Could not resolve "([^"]+)"/g; + let m: RegExpExecArray | null; + while ((m = re.exec(output)) !== null) { + const name = m[1] ?? m[2]; + if (!name) continue; + // `@scope/pkg/sub` resolves against the package `@scope/pkg`. + const base = name.startsWith('@') + ? name.split('/').slice(0, 2).join('/') + : name.split('/')[0]; + if (known.has(base)) found.add(base); + } + return [...found]; +} + +function runNpmToolchain(args: ToolchainRunArgs): BuildTestReport { + const { root, changedFiles: changed, exec } = args; + const perCommandMs = args.timeout * 1000; + // The whole-call wall-clock budget for the call, in milliseconds — measured + // from the TOP of the run, so install and build time count against it. The + // default keeps 30s of headroom under the 600-second tool timeout the brief + // welds onto the call: the clock outside starts before node does, and the + // report write must still fit. The floor is one command deadline: a tiny + // --timeout must not turn the headroom into a negative budget that starves + // every suite. + const callBudgetMs = + (args.budget ?? Math.max(args.timeout, args.timeout * 2 - 30)) * 1000; + const runStarted = Date.now(); + /** Budget left for the whole call; every phase spends from it. */ + const remainingMs = (): number => callBudgetMs - (Date.now() - runStarted); + /** The deadline a timed-out command was actually given, in whole seconds. */ + const deadlineSecs = (r: CommandResult): number => + Math.round((r.deadlineMs ?? perCommandMs) / 1000); + + // `unsupported`: build-test cannot safely scope this repo, so the agent's brief + // falls back to its build/test precedence (installing dependencies first). `ok` is + // true because nothing was found wrong — it is a handoff, not a failure. + const unsupportedReport = (note: string): BuildTestReport => ({ + toolchain: 'unsupported', + affected: [], + buildSet: [], + widenedWith: [], + install: null, + build: [], + test: [], + ok: true, + timedOut: [], + note, + }); + + const globs = readWorkspaceGlobs(root); + let { packages, skipped } = readWorkspacePackages(root); + + // The root package, read once: it decides single-root mode below, and in a + // workspace monorepo its own test suite is still a dependent the closure + // must see (a root that declares a dependency on a changed workspace). + const rootPkg = readRootPackage(root); + + // A workspace-less `package.json` with a build/test script is the most common npm + // repo shape — treat the root as a single package so it keeps the install, the + // deadline, and timeout-as-data, instead of dropping to a precedence list that no + // longer installs. Its build/test commands take no `--workspace` (dir `.`). + let singleRoot = false; + const unmodeled = globs.length > 0 && hasUnmodeledWorkspaceGlob(globs); + if (!unmodeled && globs.length === 0 && rootPkg) { + packages = [rootPkg]; + singleRoot = true; + } + + // `unsupported` when there is nothing to scope, OR when the layout uses a glob + // shape the walker does not model (`packages/**`, `foo-*`, `*/lib`). The second + // is load-bearing: without it, a diff inside an unmodeled workspace resolves to an + // EMPTY affected set and the report says "no package to build" — a confident false + // green for the review's one deterministic check. Falling back to the brief's + // precedence list is the safe direction. The unmodeled check comes FIRST because + // `packages/**` also makes `readWorkspacePackages` find nothing. + if ( + unmodeled || + (!singleRoot && (globs.length === 0 || packages.length === 0)) + ) { + return unsupportedReport( + unmodeled + ? 'This repo uses a workspace glob shape this command does not model ' + + '(e.g. `**`, an inner `*`, or a `foo-*` prefix), so it cannot safely decide ' + + 'which packages the diff touches. Fall back to the build/test precedence in ' + + 'your brief, and give each command a deadline it can actually meet.' + : globs.length > 0 + ? 'This repo declares npm workspaces, but none resolve to a package with a ' + + 'readable manifest, so there is nothing to scope. Fall back to the ' + + 'build/test precedence in your brief — installing dependencies first — ' + + 'and give each command a deadline it can actually meet.' + : 'No npm package here to scope (no workspaces, and the root has no build/test ' + + 'script). Fall back to the build/test precedence in your brief — installing ' + + 'dependencies first — and give each command a deadline it can actually meet.', + ); + } + + // A single-root repo builds and tests its one package whenever the diff changes + // anything; a workspace repo maps the changed files to the workspaces they live in. + const affected = singleRoot + ? changed.length > 0 + ? ['.'] + : [] + : affectedWorkspaces(changed, globs); + + // The test scope, decided up front so the report can disclose it even when + // there is nothing to run. Undefined for a single-root repo — its one suite + // is its full suite, and its report must not change shape — and for a + // build-only call: the merge-base probe runs no tests, and a testScope it + // never executed would claim a decision the run did not make. + // The root joins the graph whenever it is a package with a build or test + // script — not only when it has a TEST suite. Its declared dependencies are + // edges either way: a member that names the root as a dependency is reached + // THROUGH the root, and a build-only root dropped from the graph takes every + // such transitive dependent with it, silently. Which of the root's own + // scripts run is decided separately (build loop: its `build`; test scope: + // its `test`, unless it fans out over every workspace — see below). + let testScope: TestScope | undefined = + singleRoot || args.buildOnly + ? undefined + : resolveTestScope({ + changed, + globs, + packages, + skipped, + rootPackage: rootPkg, + rootTestFansOut: rootPkg?.scripts.includes('test') + ? scriptFansOut(rootPkg.scriptsText['test']) + : false, + }); + // The SAME graph feeds the build set, so the built set and the tested set + // cannot drift apart — and it is the same graph for a build-only probe as + // for the full run, or the merge-base probe measures a different tree than + // the run it is the baseline for ("same set, same commands, same verdict"). + // The root goes FIRST: on a name collision a member must win (this repo's + // root and packages/cli share the name `@qwen-code/qwen-code`). + const scopeGraph = !singleRoot && rootPkg ? [rootPkg, ...packages] : packages; + + // With no affected workspace there is nothing to run at all. Three diffs land + // here: an empty one; a build-only call (the merge-base probe), which measures + // nothing about this PR's tests by design; and a diff the workspaces cannot + // feel (the license family, or a member a negation excludes). Anything else + // outside the workspaces is disclosed through testScope.caveat — there is no + // full-suite fallback that could cover it (see the test phase below). + if (affected.length === 0) { + return { + toolchain: 'npm', + affected: [], + buildSet: [], + widenedWith: [], + install: null, + build: [], + test: [], + ...(testScope ? { testScope } : {}), + ok: true, + timedOut: [], + note: args.buildOnly + ? `The diff changes ${changed.length} file(s), none of them inside a ` + + 'workspace. There is no package to build, and tests are out of scope ' + + 'for a build-only probe.' + : testScope?.caveat + ? `The diff changes ${changed.length} file(s), none of them inside a ` + + 'workspace. There is no package to build and no test to run, but ' + + `the scope decision recorded a caveat: ${testScope.caveat}.` + : `The diff changes ${changed.length} file(s), none of them inside a ` + + "workspace (nothing the workspaces' tests can feel). There is no " + + 'package to build and no test to run — this is a complete answer, ' + + 'not a skipped step.', + }; + } + + // The dir→package map is built from the SCOPE GRAPH, not the workspace list + // alone: when the root joins the graph, a member that names it as a + // dependency puts `.` in the build set, and the root's own `build` must run + // like any other package's — skipping it would compile dependents against + // artifacts of the root that were never produced. + const byDir = new Map(scopeGraph.map((p) => [p.dir, p])); + + // A changed dir the walker mapped to something that is NOT a package (a nested + // package listed before a `*` that also claims its parent segment; a loose file + // directly under a `packages/*` base) would be dropped from the build set without + // a trace: zero commands, `ok: true`, "Everything passed" — the confident false + // green this command exists to prevent. If any affected dir is not a known + // package, the scoping cannot be trusted; hand the whole thing to the brief's + // precedence rather than certify a build that never ran. + const unmapped = affected.filter((d) => d !== '.' && !byDir.has(d)); + if (unmapped.length > 0) { + return unsupportedReport( + `The diff touches ${unmapped.join(', ')}, which the workspace globs map to no ` + + 'package (a nested package ordered before a `*`, or a loose file under a ' + + 'workspace base). Scoping cannot be trusted here, so fall back to the ' + + 'build/test precedence in your brief — installing dependencies first — rather ' + + 'than trust a scoped build that would silently skip it.', + ); + } + + // No `testScope` in the initializer: every return that fires before the + // test loop runs zero suites, and a scope on it would read as "the suites + // ran" in the agent's brief. It is attached only once the scope executes. + const results: BuildTestReport = { + toolchain: 'npm', + affected, + buildSet: [], + widenedWith: [], + install: null, + build: [], + test: [], + ok: true, + timedOut: [], + note: '', + }; + + // The install. It lives here, not in the orchestrator, because nothing before + // this command needs `node_modules`: the eleven diff-reading agents read the + // diff and grep the source. Run from the orchestrator it blocks the fan-out; + // run here it overlaps the other agents, which are still reading. + // + // A non-zero exit is NOT the end of the run, and finding that out cost a live + // review. `npm ci` executes the project's `prepare` lifecycle script, and this + // repo's runs `npm run build` and `npm run bundle` — the whole monorepo. On the + // PR under review that build hit a **pre-existing** type error in a package the + // diff does not touch, `npm ci` exited 1, and this command gave up having built + // and tested nothing: the one deterministic signal a review has, withheld + // because an unrelated package failed to compile during an install. + // + // The packages were installed. `node_modules` was on disk. So the test is not + // the exit code, it is whether the tree we need is there — and the scoped build + // below is the authoritative answer anyway. Report the install failure, and + // carry on to ask the question the review actually came to ask. + // + // A **timeout** is the exception, and it is not the same case. A `prepare` hook + // that fails leaves a *complete* `node_modules` and only the post-install build + // broken; a timeout kills `npm ci` mid-download and leaves a **partial** tree. + // Building against that produces "module not found" errors that look like defects + // in the diff and are not — so a timed-out install aborts, exactly like an install + // that left no tree at all. + // + // Whether to install is gated on npm's **completeness marker**, not the bare + // directory. `npm ci` writes `node_modules/.package-lock.json` only once the tree + // is fully materialised, so a partial tree — left by a timeout here, or by the + // agent's own shell-tool kill one level up — has the directory but not the marker. + // Gating on the directory would let every later run *skip* the install and build + // against that partial tree; gating on the marker reinstalls it. + // + // But `npm ci` is only right for an npm repo. `workspaces` is also yarn/bun/pnpm + // syntax, and those write no `package-lock.json`, so `npm ci` would fail-fast on + // the missing lockfile and mislabel a perfectly usable `node_modules` as a failed + // install. So install only when there IS a `package-lock.json` (an npm repo) whose + // tree is incomplete; a non-npm repo that already has a tree is trusted — the build + // is the authoritative signal, by this command's own argument. + const npmLock = existsSync(join(root, 'package-lock.json')); + const installComplete = (): boolean => + existsSync(join(root, 'node_modules', '.package-lock.json')); + + // A non-npm repo (yarn/bun/pnpm — `workspaces` is their syntax too) with no + // installed tree cannot be installed here: `npm ci` needs the npm lockfile, and + // building against absent dependencies fails with `Cannot find module` **inside the + // PR's own changed files** — the false-Critical steer this command exists to + // prevent. A review worktree is cold by construction, so this is the common case, + // not an edge. Hand it to the brief, naming the tool to install with. (The warm + // case — a tree already present — is trusted below and never reaches here.) + if (args.install && !npmLock && !existsSync(join(root, 'node_modules'))) { + const altLock = [ + ['yarn.lock', 'yarn install --frozen-lockfile'], + ['pnpm-lock.yaml', 'pnpm install --frozen-lockfile'], + ['bun.lockb', 'bun install --frozen-lockfile'], + ['bun.lock', 'bun install --frozen-lockfile'], + ].find(([f]) => existsSync(join(root, f))); + return unsupportedReport( + altLock + ? `This is a ${altLock[0]} repo with no installed \`node_modules\`, so \`npm ci\` ` + + `cannot install it. Run \`${altLock[1]}\` first, then fall back to the ` + + 'build/test precedence in your brief, each command with a deadline it can meet.' + : 'There is no lockfile and no `node_modules` here, so nothing can be installed ' + + 'deterministically. Install dependencies first, then fall back to the ' + + 'build/test precedence in your brief.', + ); + } + if (args.install && npmLock && !installComplete()) { + // Disk preflight. The deadline already treats "cannot finish in time" as an + // infrastructure result and skips ahead with a disclosure; "cannot fit on + // the disk" is the same class of result, discovered before the command runs + // instead of 33 seconds into it. An `npm ci` that dies on ENOSPC is + // strictly worse than one that never starts: it leaves a partial tree AND a + // full disk that fails every agent scheduled after this one. + const installCmd = 'npm ci --no-audit --no-fund'; + const free = freeDiskBytes(root); + if (free !== null && free < INSTALL_MIN_FREE_BYTES) { + results.ok = false; + results.note = + `Insufficient disk space (${gib(free)}G free, need ~${gib(INSTALL_MIN_FREE_BYTES)}G): ` + + `skipped \`${installCmd}\`, so nothing could be built or tested. This ` + + 'is an environment issue, not a code finding — report it as ' + + 'informational.'; + return results; + } + if (remainingMs() < BUDGET_MIN_ATTEMPT_MS) { + // The same floor as the build/test loops: a sub-second `npm ci` cannot + // produce anything but a fake timeout, so skip and disclose instead. + results.ok = false; + results.note = + `The whole-call budget was spent before the install could start ` + + `(${args.budget != null ? `--budget ${args.budget}s` : 'default budget'}), ` + + 'so nothing could be built or tested. This is an infrastructure ' + + 'result, not a defect in the diff — report it as informational.'; + return results; + } + const install = exec( + installCmd, + root, + Math.min(perCommandMs, remainingMs()), + ); + results.install = install; + if (install.timedOut) results.timedOut.push(install.command); + // A timeout leaves a partial tree — remove it, so this is not mistaken next time + // for a complete install to build against. `spawnSync`'s SIGTERM only kills the + // direct shell; the orphaned `npm`/`node` grandchildren keep writing the tree, so + // `rmSync` can race them and throw `ENOTEMPTY` — which must not replace the whole + // report with a raw error. Best-effort with retries; the marker gate below still + // decides the outcome. + if (install.timedOut) { + try { + rmSync(join(root, 'node_modules'), { + recursive: true, + force: true, + maxRetries: 3, + }); + } catch { + // Best effort — a partial tree left behind is caught by the marker gate. + } + } + if (install.timedOut || !installComplete()) { + results.ok = false; + results.note = install.timedOut + ? `\`${install.command}\` ran out of time (${deadlineSecs(install)}s) and left an ` + + 'incomplete `node_modules`, so nothing could be built or tested against it. ' + + 'This is an infrastructure result, not a defect in the diff — report it as ' + + 'informational.' + : 'The install failed and left no usable `node_modules`, so nothing could be ' + + 'built or tested. This is an environment failure, not a defect in the diff — ' + + 'report it as informational.'; + return results; + } + } + + // The install exited non-zero but left a usable tree, so the run went + // ahead: frame the failure on EVERY return from here on — the disk + // preflight and build-failure returns below fire before the final one, + // and without the framing the agent can file the install failure as an + // additional Critical against the PR. + const frameInstallFailure = (): void => { + if (results.install && results.install.exitCode !== 0) { + results.note = + `\`${results.install.command}\` exited ${results.install.exitCode} but left a usable ` + + '`node_modules`, so the run went ahead. ' + + 'The install failure is an environment/infrastructure result — report it as ' + + 'informational, never as a Critical, and never against this PR. ' + + results.note; + } + }; + + // The same preflight before the build phase, at a lower floor. A warm tree + // skips the install (and its 3 GiB gate) entirely, but a compile that hits + // ENOSPC mid-write fails with errors that read as defects in the diff — and + // leaves the disk full for everything that runs after this command. + const freeForBuild = freeDiskBytes(root); + if (freeForBuild !== null && freeForBuild < BUILD_MIN_FREE_BYTES) { + results.ok = false; + results.note = + `Insufficient disk space (${gib(freeForBuild)}G free, need ~${gib(BUILD_MIN_FREE_BYTES)}G): ` + + 'skipped the build and tests rather than fill the disk mid-compile. This ' + + 'is an environment issue, not a code finding — report it as informational.'; + frameInstallFailure(); + return results; + } + + const alsoBuild: string[] = []; + let set = buildSetFor(affected, scopeGraph); + const built = new Set(); + const widened = new Set(); + // A root build that fans out over the workspaces (`npm run build + // --workspaces`) is an aggregator: it produces no artifacts of its own, the + // scoped loop already builds the members it drives, and as one bare command + // it is exactly the whole-monorepo build this module exists to stop + // running. Only a NON-fan-out root build — one that compiles the root's own + // sources — is worth its deadline. + const rootBuildRuns = + !!rootPkg?.scripts.includes('build') && + !scriptFansOut(rootPkg.scriptsText['build']); + // One predicate for both the loop skip and the reported set: a fan-out + // root's build does not run — never in single-root mode, where the root is + // the only package there is. + const rootBuildSkipped = !singleRoot && !rootBuildRuns; + const notBuilt: string[] = []; + + // Build, and let the compiler correct the set. Three widenings is generous: each + // one is a package the graph could not have known about, and a fourth would mean + // the graph is not wrong but absent. Every command spends from the same + // whole-call budget as the tests — an unbounded build phase would hand the + // outer shell kill a report the budget exists to save. + for (let attempt = 0; attempt <= 3; attempt++) { + let failure: CommandResult | null = null; + + for (const dir of set) { + if (built.has(dir)) continue; + const pkg = byDir.get(dir); + if (!pkg?.scripts.includes('build')) { + built.add(dir); // Nothing to build is not a failure to build. + continue; + } + if (dir === '.' && rootBuildSkipped) { + // Fan-out aggregator root: the members it drives are built by this + // very loop; the bare `npm run build` would re-build all of them + // inside one deadline (see above). + built.add(dir); + continue; + } + if (remainingMs() < BUDGET_MIN_ATTEMPT_MS) { + // The budget is spent: stop building and disclose. Suites of unbuilt + // packages must not run either — a suite against artifacts never + // compiled manufactures failures the diff did not cause (the exact + // lesson of the scoped-build/full-test cascade). + notBuilt.push( + ...set.filter( + (d) => !built.has(d) && byDir.get(d)?.scripts.includes('build'), + ), + ); + break; + } + const r = exec( + buildCommand(dir), + root, + Math.min(perCommandMs, remainingMs()), + ); + results.build.push(r); + if (r.timedOut) results.timedOut.push(r.command); + if (r.exitCode !== 0) { + failure = r; + break; + } + built.add(dir); + } + + if (!failure) break; + + // Did it fail because the set was too small — or mis-ordered? The declared graph + // under-approximates whenever a package reaches into another's *sources* (a + // tsconfig `paths` entry into `../cli/src/...` compiles that package's imports + // without declaring a dependency), and the compiler names the package it could + // not resolve. Filter on `!built.has(dir)`, not `!set.includes(dir)`: when BOTH + // the needer and the undeclared-needed package are affected and the alphabet + // ordered the needer first, the named package is already IN the set but not yet + // built — re-seeding it into `alsoBuild` (which sorts first) fixes the order. The + // attempt cap bounds the loop; a package that is truly missing is not in the map. + // + // A **timeout** must not enter this path. A build killed at the deadline leaves + // partial output that can happen to contain a `Cannot find module` line, which + // would look like a too-small build set and trigger a retry — another full + // deadline, and another, up to the attempt cap. A timeout is infrastructure, not + // a graph gap: report it and stop, the same way the install path does. + const missing = failure.timedOut + ? [] + : unresolvedWorkspaceDeps(failure.output, packages).filter((name) => { + const dir = packages.find((p) => p.name === name)?.dir; + return dir && !built.has(dir); + }); + if (missing.length === 0 || failure.timedOut || attempt === 3) { + results.ok = false; + results.note = failure.timedOut + ? `\`${failure.command}\` ran out of time (${deadlineSecs(failure)}s). That is an ` + + 'infrastructure result, not a defect in the diff — report it as informational.' + : `\`${failure.command}\` failed. Correlate the errors below with the diff: a ` + + 'compile error in a file the PR changed is a Critical; one in a file it did not ' + + 'touch is a pre-existing failure, and belongs in the terminal, not on the PR.'; + results.buildSet = ( + rootBuildSkipped ? set.filter((d) => d !== '.') : set + ).filter((d) => !notBuilt.includes(d)); + results.widenedWith = [...widened]; + frameInstallFailure(); + return results; + } + + // Drop the failed attempt from the report. It is about to be retried with the + // package it asked for, and it is **not evidence about this PR**: the build set + // was too small, which is this command's mistake, not the author's. Left in + // `build[]`, an agent told "a build failure in a changed file is a Critical" + // reads `packages/vscode-ide-companion rc=2` and files exactly that — a public + // blocker on a PR whose build passes. (A timed-out failure cannot reach here — it + // is terminal above — so only `build[]`, never `timedOut`, can hold it.) + results.build = results.build.filter((r) => r !== failure); + + for (const name of missing) widened.add(name); + for (const name of missing) { + const dir = packages.find((p) => p.name === name)?.dir; + if (dir) alsoBuild.push(dir); + } + // As `alsoBuild`, never as `affected`. The compiler asked for this package + // because something compiles *against* it; the PR did not change it, so its + // consumers cannot have been broken by the PR and must not be built. + set = buildSetFor(affected, scopeGraph, alsoBuild); + } + + // The build set reports what was (to be) BUILT: a fan-out root whose build + // was skipped — an aggregator the loop already covered member by member — + // and packages the budget stopped before building must not linger in it, or + // the report names builds that never ran. + results.buildSet = ( + rootBuildSkipped ? set.filter((d) => d !== '.') : set + ).filter((d) => !notBuilt.includes(d)); + results.widenedWith = [...widened]; + if (notBuilt.length > 0) results.notBuilt = [...notBuilt].sort(); + + // Test what the diff can break: the changed workspaces plus their + // reverse-dependency closure — exactly the suites that define a test script. + // Testing the changed ones alone under-tests in the one way a compile cannot + // catch: a behaviour change in `core` leaves every dependent compiling and + // still fails their suites. The closure is a subset of the build set (which + // adds compile-time dependencies on top), so every tested package was built + // above, with everything it compiles against. + // + // When the scope decision recorded a caveat — a graph it could not fully + // compute, a changed file outside every workspace, a closure past half the + // testable suites — the scoped set still runs and the caveat discloses what + // it may miss. There is NO fallback to the repo's root `npm test`: on a + // large monorepo that command cannot finish inside a command deadline (this + // repo's suite took 31 minutes in CI against a 300-second deadline, and a + // third of recent diffs would have hit the fallback), so the fallback would + // only ever report a timeout — zero signal framed as a failure. The scoped + // set is the run that covers the diff — each command keeps its own deadline. + // + // Those per-command deadlines SUM, though, and a large closure can sum past + // the whole-call ceiling the brief welds on (600s by default) — the outer + // shell kill then discards the report entirely. So the loop below runs + // against a whole-call budget that EVERY phase (install, builds, tests) + // spends from: each command gets the smaller of its own deadline and what + // remains. A suite killed at the budget boundary is a timeout — already + // framed as infrastructure — and a partial attempt is signal where a + // never-attempted suite is none. Below the floor an attempt cannot even + // boot npm, so the suite goes to notRun instead of manufacturing a fake + // timeout. A partial report is signal; a discarded one is the "71 + // timeouts, nothing verified" failure this command exists to end. + const rootHasTest = !!rootPkg?.scripts.includes('test'); + const testDirs = args.buildOnly + ? [] + : !testScope + ? affected // single root: its one package, exactly as before scoping + : testScope.workspaces; + const runnable = (dir: string): boolean => + dir === '.' ? rootHasTest : !!byDir.get(dir)?.scripts.includes('test'); + // Affected first: the changed workspace's own suite is the highest-value + // one and must be unstarvable — the dependents are the widening, and the + // widening is what a budget should trim. (The closure is alphabetical, so + // without this a `zebra` change would run `alpha`'s suite and starve its + // own.) + const affectedSet = new Set(affected); + const runnableDirs = [ + ...testDirs.filter((d) => affectedSet.has(d) && runnable(d)), + ...testDirs.filter((d) => !affectedSet.has(d) && runnable(d)), + ]; + // Suites of packages the budget left UNBUILT cannot run — against artifacts + // never compiled, their failures would be manufactured, not measured. + const untestable = + notBuilt.length > 0 + ? new Set(reverseDependencyClosure(notBuilt, scopeGraph)) + : new Set(); + const notRun: string[] = []; + for (let i = 0; i < runnableDirs.length; i++) { + const dir = runnableDirs[i]; + if (untestable.has(dir)) { + notRun.push(dir); + continue; + } + const remaining = remainingMs(); + if (remaining < BUDGET_MIN_ATTEMPT_MS) { + // Below the floor an "attempt" cannot even boot npm — it would + // manufacture a fake timeout where an honest notRun says what happened. + // Unfiltered: an untestable dir the budget also stopped must still + // leave `testScope.workspaces` (which names what RAN), and no dir at + // index >= i can already have been pushed. + notRun.push(...runnableDirs.slice(i)); + break; + } + const r = exec(testCommand(dir), root, Math.min(perCommandMs, remaining)); + results.test.push(r); + if (r.timedOut) results.timedOut.push(r.command); + if (r.exitCode !== 0) results.ok = false; + } + + // A budget stop is STRUCTURAL, not just prose: `testScope.workspaces` is + // documented (and quoted by the agent's brief) as exactly the suites that + // ran, so the trimmed suites leave it, and `notRun` names them. Sorted, so + // both fields are stable and comparable. + notRun.sort(); + const partialNote = + [ + notBuilt.length > 0 + ? `the build phase reached the whole-call budget — not built: ` + + notBuilt.join(', ') + : '', + notRun.length > 0 + ? `the whole-call budget (${Math.round(callBudgetMs / 1000)}s) was ` + + `spent with ${notRun.length} suite(s) still to run — not run: ` + + notRun.join(', ') + : '', + ] + .filter(Boolean) + .join('; ') || undefined; + if (testScope && partialNote) { + const ran = testScope.workspaces.filter((d) => !notRun.includes(d)); + testScope = { + workspaces: ran, + ...(notRun.length > 0 ? { notRun } : {}), + caveat: testScope.caveat + ? `${testScope.caveat}; ${partialNote}` + : partialNote, + }; + } + + // The scope was executed — only now may the report carry it. Every return + // between the initializer and here ran zero test commands and must not + // claim a scoping decision; the one exception, the nothing-to-run answer + // above, carries the scope precisely because the empty scope IS the answer. + if (testScope) results.testScope = testScope; + + if (!results.note) { + const failed = [...results.build, ...results.test].filter( + (r) => r.exitCode !== 0, + ); + // A timeout is a failure (its exitCode is null), but it is NOT a defect in the + // diff, and the note must not tell the agent to correlate it with one — the + // brief says timeouts are infrastructure, and an agent trusts the data over its + // instructions. So a test that runs out of time gets the same infrastructure + // framing the build-timeout path already gives, not the "a failure is a Critical" + // message meant for a real compile/assertion failure. + const realFailures = failed.filter((r) => !r.timedOut); + if (results.ok) { + // The tests sentence names the scope, because it is the agent's report + // that has to be able to say what was and was not run: a scoped run + // names its suites, and a caveat says what the scope may miss. + let testsClause: string; + if (args.buildOnly) { + testsClause = '. Tests were not run (build-only).'; + } else if (!testScope) { + testsClause = + results.test.length === 0 + ? notRun.length > 0 + ? // The loop pushed the suite to notRun: the script exists. + ', but the whole-call budget was spent before any suite could run.' + : ', but the package defines no test script, so no tests ran.' + : ' and ran the tests of the changed ones. Everything passed.'; + } else if (testScope.workspaces.length === 0) { + testsClause = testScope.notRun?.length + ? ', but the whole-call budget was spent before any suite could run.' + : ', but no workspace in scope defines a test script, so no tests ran.'; + } else { + // The scoped list is filtered to dependents WITH a test script; a + // build-only dependent is built but never tested, so the note must + // not claim every declared dependent was covered. + testsClause = + ` and ran the tests scoped to ${testScope.workspaces.join(', ')} — ` + + 'the changed workspaces and every workspace declared to depend on ' + + 'them that defines a test script. Everything passed.'; + } + if (testScope?.caveat) testsClause += ` Caveat: ${testScope.caveat}.`; + // The root is not a workspace: count it separately, or a 22-member repo + // reports "of 23" — a number in a report whose thesis is honest numbers. + // (A single-root repo's one package IS '.', and counts as the one.) + const builtWorkspaces = results.buildSet.filter( + (d) => singleRoot || d !== '.', + ).length; + const rootSuffix = + !singleRoot && results.buildSet.includes('.') && !rootBuildSkipped + ? ' (plus the root package)' + : ''; + results.note = + `Built ${builtWorkspaces} of ${packages.length} workspaces${rootSuffix} (the ${affected.length} the ` + + `diff changes, plus what they compile against${ + widened.size + ? `, plus ${[...widened].join(', ')} the compiler asked for` + : '' + })${testsClause}`; + } else if (realFailures.length === 0) { + results.note = + `${failed.length} command(s) ran out of time (${deadlineSecs(failed[0])}s). A timeout is an ` + + 'infrastructure result, not a defect in the diff — report it as informational.'; + } else { + results.note = + `${realFailures.length} command(s) failed. Correlate each error with the diff: a failure in a ` + + 'file the PR changed is a Critical; one in a file it did not touch is pre-existing.' + + (failed.length > realFailures.length + ? ' (Commands that timed out are infrastructure, not findings.)' + : ''); + } + } + + // A failure note must carry the caveat too — the note is what the brief + // renders first, and "a test failed AND the budget dropped suites" must not + // read as a plain failure. (The ok branch already appended it above.) + if (results.testScope?.caveat && !results.note.includes('Caveat:')) { + results.note += ` Caveat: ${results.testScope.caveat}.`; + } + + // Single-root repos carry no testScope, so a budget stop is disclosed on + // the note itself. (With a scope, the caveat above already says it.) + if (partialNote && !results.testScope) { + results.note = results.note + ? `${results.note} ${partialNote}.` + : partialNote; + } + + // The build and test results below are real, and the install failure is + // not a finding about this PR. (A `prepare` script that builds the whole + // project, as this repo's does, fails on any pre-existing error anywhere + // in it.) + frameInstallFailure(); + return results; +} + +export const npmToolchainAdapter: ReviewToolchainAdapter = { + // A root package.json alone is not an npm build project — docs sites, husky, + // and lint configs put one in Java repos. Apply only when runNpmToolchain can + // actually scope something: MODELED workspaces that resolve to at least one + // package, or a root build/test script. Mirroring the run-side gate here + // matters at mixed roots: an unmodeled-glob declaration (`packages/**`, + // `foo-*`) or a zero-package glob used to apply npm anyway, block a second + // adapter's selection, and drop the repo to the very `unsupported` handoff + // this guard exists to prevent — even though npm.run would immediately + // concede unsupported and the other adapter alone would have succeeded. + // When ZERO adapters + // apply at an npm-shaped root, runBuildTest delegates here anyway so the + // report carries runNpmToolchain's precise handoff note (the unmodeled-glob + // gate below is that diagnostic path, not dead code). + applies: (root) => { + const globs = readWorkspaceGlobs(root); + if (globs.length > 0) { + return ( + !hasUnmodeledWorkspaceGlob(globs) && + readWorkspacePackages(root).packages.length > 0 + ); + } + return readRootPackage(root) !== null; + }, + run: runNpmToolchain, +}; diff --git a/packages/cli/src/commands/review/lib/toolchain.ts b/packages/cli/src/commands/review/lib/toolchain.ts new file mode 100644 index 00000000000..53874d6f5e6 --- /dev/null +++ b/packages/cli/src/commands/review/lib/toolchain.ts @@ -0,0 +1,51 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { BuildTestReport, CommandResult } from '../build-test.js'; + +export interface ToolchainRunArgs { + root: string; + changedFiles: string[]; + timeout: number; + /** + * Gates the adapter's dependency-acquisition step — npm's `npm ci` today. + */ + install: boolean; + buildOnly?: boolean; + /** + * Whole-call wall-clock budget in seconds, measured from the top of the + * call — undefined leaves the adapter its default (2× `timeout` minus + * startup headroom, floored at one per-command deadline). + */ + budget?: number; + exec: (command: string, cwd: string, timeoutMs: number) => CommandResult; +} + +export interface ReviewToolchainAdapter { + applies(root: string): boolean; + run(args: ToolchainRunArgs): BuildTestReport; +} + +export interface ToolchainSelection { + /** The single adapter that applies, or null when zero or several do. */ + adapter: ReviewToolchainAdapter | null; + /** + * Every adapter whose applies() held — walked once here, reused by the + * caller for the ambiguity note instead of re-walking the trees. + */ + applicable: readonly ReviewToolchainAdapter[]; +} + +export function selectToolchainAdapter( + root: string, + adapters: readonly ReviewToolchainAdapter[], +): ToolchainSelection { + const applicable = adapters.filter((adapter) => adapter.applies(root)); + return { + adapter: applicable.length === 1 ? applicable[0] : null, + applicable, + }; +}