mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-08-07 07:45:54 +00:00
7c1a93477c
* perf(review): scope Agent 7's build/test to the workspaces the diff changed Agent 7's brief runs `npm run build` then `npm test` with a 120s deadline. On this repo a cold full build is 125s, so the mandated command cannot finish inside the mandated deadline — measured across the harness's own transcripts, 71 `npm run build` timeouts, each verifying nothing before the model spent more turns ruling the timeout environmental and improvising a narrower command. `qwen review build-test` replaces it. It installs if needed, then builds only the workspaces the diff touches (from the plan's files[] and the root package.json workspaces) plus their dependents and dependencies, deps-first — for a leaf PR, a handful of packages instead of all of them. When a compile fails on `TS2307: Cannot find module '@scope/pkg'` naming a workspace, it adds that package and retries, so a tsconfig `paths` edge into another package's sources (which package.json never declares) self-corrects rather than being modelled. A command that runs out of time is reported as infrastructure, never as a Critical against the PR; a build failure in a file the diff did not touch is reported as pre-existing. Agent 7's brief (in lib/agent-briefs.ts) now leads with build-test and falls back to the maven/gradle/cargo/go/pytest precedence only when it reports `toolchain: unsupported`, so non-npm repos are unchanged. buildRoleBrief injects the concrete command with absolute --plan/--worktree paths, beside the test-efficacy probe it already injects. The install moves out of Step 1 (nothing before Agent 7 needs node_modules, and running it there blocked the whole fan-out) into build-test, with QWEN_SKIP_PREPARE=1 so `npm ci` does not trigger this repo's prepare hook — which builds and bundles every workspace, ~190s, wasted before a scoped build. npm ci drops from ~161s to ~27s. And todo_write is banned during a review: this document is the plan, and each call is a whole model turn (measured at 179-377s per run of restating steps already written down). `isWorkspaceMember` moves from test-efficacy.ts to the shared lib/workspaces.ts (re-exported to keep its callers), since build-test needs the same glob walk. * test(review): add build-test to the expected subcommand roster * fix(review): guard the build-test prompt and treat every timeout as infrastructure Addresses the /review findings on this PR: - agent-prompt.ts: the build-test command block is now guarded on `pr !== undefined`, matching the adjacent test-efficacy block. Without it, `report.prNumber` being absent resolved `--out` to `qwen-review-pr-undefined-build-test.json` — a report the agent writes and downstream never finds. - build-test.ts: a timed-out `npm ci` now aborts instead of building against the partial `node_modules` a timeout leaves behind (distinct from a `prepare` failure, which leaves a complete tree). A timed-out test command now produces an infrastructure note rather than "correlate this failure with the diff — a failure is a Critical", which contradicted the brief. The handler's triple `argv` cast is consolidated. Tests: absolute-path + no-`undefined` assertions for the build-test command block, and install-timeout / test-timeout infrastructure-note coverage. * fix(review): survive the tool timeout, partial installs, local mode, and Windows Addresses the review on this PR: - **Tool timeout (High).** build-test runs install + builds + tests in one process; the agent's default 120s `run_shell_command` timeout would kill it — the failure this PR fixes, one level up. The welded command block now tells the agent to invoke it with `timeout: 600000` (the shell tool's max). - **Partial `node_modules` (Medium).** The install gate is now npm's completeness marker (`node_modules/.package-lock.json`), not bare directory existence, so a partial tree left by a timeout (here or from the outer kill) triggers a reinstall instead of a build against half a tree. A timed-out install also removes the partial tree before aborting. - **Local reviews (Medium).** Agent 7 runs on local/file reviews too, which have no worktree or PR number. The build-test block now emits there as well, scoped to the project root, so the brief's "run build-test, below" is no longer a mandate with no command. - **Windows merge queue (Medium).** The real-spawn fixtures used POSIX `touch`/`test -f`/`sleep`, which the Windows `test_windows` job (cmd.exe) fails on; rewritten with portable `node -e`, plus explicit per-test timeouts. Smaller points: an unmodeled workspace glob (`**`, inner `*`, `foo-*`) now returns `toolchain: "unsupported"` instead of a false "nothing to build"; a widened-away command is dropped from `timedOut` as well as `build[]`; the `ok` docstring is corrected; timeout detection prefers spawnSync's authoritative `ETIMEDOUT`. * fix(review): abort widening on a build timeout, and close the round-2 review gaps Addresses the second review pass: - **Widening loop ignored a timeout (Critical).** A build killed at the deadline leaves partial output that can contain a `Cannot find module` line; the loop read that as a too-small build set and retried under another full deadline, up to the attempt cap — 4× the wall clock for what is infrastructure, not a graph gap. A timed-out build now aborts at once, the way the install path already does. - **Widening scanned trimmed output.** `unresolvedWorkspaceDeps` reads the per-command output, which was head+tail only; a `Cannot find module` in the omitted middle (a long tsc log) ended widening early and surfaced a real gap as a false build failure. `trimOutput` now rescues module-resolution error lines from the middle, so the report stays bounded and the widening signal survives. - **Unquoted `--workspace`.** A workspace dir with a space or shell metacharacter would split under `shell: true`; the build and test commands now quote it. - **Handler had no error boundary.** A missing/invalid plan threw a raw stack trace as the whole of Agent 7's result; the handler now prints the descriptive message and exits non-zero. Coverage the review asked for: the widening attempt-cap exhaustion, a build timeout mid-widening, negated-workspace exclusion from the build set, the `changedFilesFrom` error path, and `readWorkspaceGlobs`' object form. * test(review): drive build-test through the exec seam, not real npm The new build/test scoping tests spawned real `npm run` processes. Under the full suite's parallelism that hung CI (each npm start is slow, and a `node -e` setTimeout fixture leaked past the process kill), and the fixtures were POSIX-only. These tests are about which packages get built, in what order, and how a result is classified — not about npm's own workspace resolution. Driving them through the injectable `exec` seam makes them deterministic, instant, leak-free and platform-independent. No production code changes; the suite drops from a hang to ~6s. * fix(review): support single-package and non-npm repos in build-test Addresses the follow-up review's findings. - **Single-package npm repos (no `workspaces`) had no build/test path (regression).** build-test now treats a workspace-less `package.json` with a build/test script as a single root package: it installs, builds `npm run build` and tests `npm test` (no `--workspace`), keeping the deadline and timeout-as-data for the most common repo shape. Only when the root has no build/test script does it report `unsupported` — and the brief's fallback now installs dependencies first, since build-test's own install runs only on the npm path. - **yarn/bun workspace repos got a false "install failed".** `workspaces` is also yarn/bun syntax, but they write no `package-lock.json`, so the completeness marker was never present and `npm ci` fail-fasted over a usable tree. Install now runs only when a root `package-lock.json` exists; a non-npm tree that is already present is trusted (the build is the authoritative signal). - **Per-command deadline lowered 600s → 300s**, kept strictly below the 600 000 ms tool timeout the brief welds, so a single hung command's own deadline fires — and the report lands — before the outer shell kill would discard it. (A giant PR whose commands sum past the tool ceiling remains an acknowledged follow-up; a dynamic cumulative budget is the full fix.) Smaller points: the install-timeout `rmSync` is wrapped (best-effort, `maxRetries`) so a race with orphaned grandchildren can't replace the report; the local-mode `.` worktree fallback is gated on `pr === undefined` so a PR-mode report never builds the user's checkout (help text corrected); the now-unreachable widen-path `timedOut` filter is removed; and `changedFilesFrom` rejects a non-object plan with a descriptive error. * fix(review): close the generic-repo false-green and false-Critical gaps The verification review found three ways the generic (non-this-repo) surface re-opened the two failure modes this command is built to prevent. All three now hand off or self-correct instead. - **F1 (false green).** A changed dir the workspace globs map to a non-package — a nested package listed before a `*` that also claims its parent segment, or a loose file under a `packages/*` base — was dropped from the build set silently: zero commands, `ok: true`, "Everything passed". Any affected dir not in the package map now trips the `unsupported` handoff (naming the dir), never a bare green. - **F2 (false Critical).** A review worktree is cold. A yarn/bun/pnpm repo (same `workspaces` field, no `package-lock.json`) got no install on any path — build-test's install is npm-gated and the brief's fallback never fired — so the build failed with `Cannot find module` inside the PR's own files, steered toward a Critical. A non-npm repo with no installed tree now hands off, naming the tool (`yarn install --frozen-lockfile`, etc.) to install with first. - **F3 (mis-order terminal-fail).** When both the needer and an undeclared-needed package are changed and the alphabet orders the needer first, the compiler named an in-set package, `missing` came out empty, and the run terminal-failed though the corrected order builds green. The widening filter is now `!built.has(dir)` rather than `!set.includes(dir)`, so an in-set-but-unbuilt package re-seeds into `alsoBuild` (which sorts first) and fixes the order; the attempt cap still bounds it. Tests for all three, matching the review's runnable repros. The non-blocking notes (cumulative-budget overrun, incremental report write, array-args spawn) remain for the dynamic-budget follow-up. * test(review): tighten the widening-cap assertions Per review: assert exactly three widenings (`toBe(3)`, not `>= 3`) so an over-widening regression is caught, and assert `rep.test` is empty — the exhaustion branch returns before the test loop, so a refactor that reordered the test loop above that return would otherwise pass undetected. |
||
|---|---|---|
| .. | ||
| scripts | ||
| src | ||
| vendor | ||
| index.ts | ||
| package.json | ||
| test-setup.ts | ||
| tsconfig.json | ||
| vitest.config.ts | ||