feat(review): add Maven multi-module verification - #9099
Conversation
`review build-test` combined three responsibilities in one module: reading the review plan, deciding which toolchain can be verified deterministically, and implementing npm workspace installation, affected-package selection, dependency widening, build execution, test execution, and reporting. Move the npm implementation behind an internal `ReviewToolchainAdapter` contract. `build-test.ts` keeps CLI routing, plan reading, output trimming, env shaping, and the spawn boundary; `lib/npm-toolchain.ts` owns npm detection and the verification algorithm; `lib/disk.ts` holds the shared free-disk floors. This is a move, not a rewrite: 95% of the lines removed from build-test.ts reappear verbatim in the new files. The CLI arguments, the BuildTestReport JSON shape, and every npm behaviour are unchanged, and the existing build-test suite is the compatibility oracle for that. Selection requires exactly one applicable adapter and fails closed to the `unsupported` handoff otherwise, so a second toolchain lands as a registration rather than another branch in this file.
`review build-test` produced deterministic evidence for npm projects but handed Maven projects back to an agent for ad hoc command selection. That made Java verification inconsistent and provided no reliable module ownership, timeout classification, or fresh test-report evidence. Register a Maven adapter on the toolchain boundary. It maps changed files to the nearest ancestor Maven project, prefers the root Maven Wrapper, runs one scoped `test` or build-only `test-compile` from the reactor root with `-am` upstream expansion, and emits module-qualified Surefire/Failsafe evidence limited to reports this invocation created or updated. Maven, not this adapter, is the authority on the reactor. Ownership is a nearest-ancestor `pom.xml` walk; whether that project is active under the current profiles, JDK, and `<modules>` inheritance is answered by Maven rejecting the `-pl` selector, before anything is compiled. A changed POM runs the reactor rather than a guessed inheritance closure. Driven by alibaba/fastjson2 and alibaba/druid, where nested, profile-activated, and standalone POMs make a text-level reactor model unsafe. The same principle applies to the command line: a recorded Maven run carries its lifecycle, module set, and `-am` flag as `CommandResult.maven`, straight from the values the adapter rendered the command from. `test-plan` settles a Test Plan claim against those facts and parses only the claim, which is the one side that is genuinely free text. Downstream: base-tree accepts successful Maven build-only results, test-plan settles Maven command and Surefire count claims, Agent 7 gains a Maven branch, and Maven failures do not enter the npm-only test-delta rerun path. Fails closed for mixed npm+Maven roots and for module directories a `-pl` selector cannot express. Does not claim Maven effective-model or CI-matrix parity, and does not implement Gradle, JaCoCo, or Maven-aware base-side test-delta.
# Conflicts: # docs/design/review-toolchain-adapters.md # packages/cli/src/commands/review/build-test.test.ts # packages/cli/src/commands/review/build-test.ts # packages/cli/src/commands/review/lib/disk.ts # packages/cli/src/commands/review/lib/npm-toolchain.ts # packages/cli/src/commands/review/lib/toolchain.ts
Address all 31 findings from the qwen3.8-max review: - Ownership: a src/-nested POM that would collapse to the root project now fails closed to reactor-wide (a real <module>src/core</module> would otherwise go untested under a green -pl . verdict) - Selector gate: reject leading '-' (commons-cli re-reads the value as an option) and '!' (Maven exclusion) module dirs, widening to the full reactor like the existing ,/:/% rejections - Verdict: a skip-tests marker with zero reports is a swallowed failure, not a pass; a wrapper distribution-download failure is classified infrastructure; classification runs on SGR-stripped output so colored Maven logs cannot launder failures - Evidence: sweep truncation (scan cap, fan-out bound) and parse rejections (oversized/unreadable/zero-suite reports) count as unknown evidence and fail closed like the fresh-report cap; the sweep streams entries instead of materializing unbounded Dirent arrays, and never follows a symlinked report dir - Propagation: CommandResult.evidenceCapped records the refused-to- certify outcome; test-plan excludes it from finished(), observedTestCounts, and ranFailed like infrastructure - Claim side: failureInsideClaim also mines surviving failing [maven-test-report] lines (case-cap truncation can erase every per-case line of the claimed module); attached -pl=/--projects= quoted selectors are consumed like the space form; claim -pl modules normalize a leading ./; coordinate selectors stay unsettleable instead of cross-matching module dir names; deploy/ site/plugin-goal work in a claim forces the phase-reduction disclosure; a header-only failing report emits a fallback [maven-test-failure] line; clean rollup lines carry the per-report clamped passed totals the parser expects - Agent 7 brief no longer forbids the sanctioned unsupported fallback it instructs five lines later; design doc and PR description corrected where they drifted from the code
- neverRan keys on evidence (zero reports + zero framing), not wrapper history: a wrapper bump runs the reactor green and must not read as "never ran" - fresh-report parse-cap overflow discloses sampling instead of refusing certification; parser rejections and truncated sweeps stay fail-closed - non-zero-exit acquisition scans read the whole output so dependency, launch, and disk deaths after `-am` upstream tests read as infrastructure; source-failure suppression keeps the prelude-only scan - the Surefire stdout-summary scan is gated like its exit-0 siblings when visible fresh reports are green; the relocated-reports shape survives - polyglot roots run the npm toolchain and disclose the Maven half instead of running nothing - the skipped-tests marker takes an evidence rescue slot - observedTestCounts emits the changed-module subtotal beside the -am reactor total - a phase-less bare runner span extracts as a path claim, not a command claim that can never settle - test-delta reads Maven's exit-0 verdict flags as failures instead of stating "no PR-side test command failed" - the claim pipeline tokenizes with shell-quote parse(), replacing the hand-rolled quote-aware splitter
Fail closed on the remaining greenwash shapes in the Maven report parser (swallowing exempt CDATA, unterminated sections, testcase nesting in quoted attributes), stop echoed infrastructure wording and forged selector rejections from laundering or hiding genuine failures, tighten never-ran evidence for diff-modified wrappers, and close the Test Plan holes where capped/exit-0 failure evidence settled nothing or contradicted its build-test verdict.
Maintainer call on #8777: stop patching XML corners, close the class. Eight review rounds kept finding real release-direction holes because maven-toolchain.ts hand-rolled a parser for untrusted, PR-controlled surefire XML — XML has infinitely many corners, and each round found the next one. Same anti-pattern the review skill hit with a hand-rolled CommonMark scanner and closed by adopting markdown-it (#9020). - Surefire/Failsafe reports now parse with a strict, throwing XML parser (saxes): well-formedness, CDATA, comments, entities, and self-closing tags are the parser's job. A parse rejection joins the fail-closed unknown-evidence states — never read green. The hand- rolled tag walk, attribute regexes, CDATA/comment stripping, and the surefire-stdout CDATA exemption with its swallowing probes are gone (~500 lines). - Pass/fail is a fail-closed state machine over three authoritative signals: exit code + parsed report elements + the maven.config settings that change what exit 0 means. Green requires positive structural evidence, never the absence of a failure substring. The human-stdout scrapers are demoted to a fallback for runs with no reports; with reports present the structured signals judge, and the scrapers can only convict under a distrusted exit 0. - The parse-count cap is gone — it was itself a leak (a failing report ordered past the cap certified green). Every fresh report is parsed; the sweep's path cap and the per-file size cap still bound cost. The clean rollup is uncapped: one attributed line per project, so no module lands in an unattributed omitted tail. - Maven CLI arg grammar is one spec-referenced tokenizer pass over .mvn/maven.config (dependency inputs, fail-never/testFailureIgnore, quiet/log-file, skip-tests) that fails closed on ambiguity: an unclassifiable flag makes the verdict distrust a bare exit 0. Imprecision can only ever be stricter, never release. The existing test suite stays the oracle: contract pins pass under the swapped implementation; the cases that pinned hand-rolled XML-corner mechanics collapsed to "malformed ⇒ fail-closed" plus structural pins (CDATA/comments are opaque text, escaped attribute values parse cleanly, adversarial shapes reject in bounded time). Net -297 lines. The rule goes into the toolchain-adapter design doc: when judging pass/fail from untrusted tool output, prefer the tool's authoritative structured signals over scraping human stdout; parse with a real parser and treat parse failure as fail-closed; pass requires positive structural evidence; where a grammar must be parsed, use one spec-referenced tokenizer and fail-closed on ambiguity.
wenshao
changed the title
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical. Eight review rounds on the parent PR (#8777) kept finding release-direction holes in the hand-rolled Surefire XML scanning (the round-8 findings are named one by one in the maintainer decision there), and that decision froze the patch loop and asked for exactly this: close the XML-corner class with a real parser instead of enumerating corners. Direction: aligned — this is the class-closing refactor the decision prescribes, in the separate-PR form it asked for, with the existing test suite kept as the oracle as mandated. Merge order is stated and sound: parent first, this after. Size: not a core module path ( Approach: the scope is right and matches the prescribed design point for point — strict throwing parser (saxes, already in the lockfile through jsdom, so no new package), fail-closed state machine over exit code + parsed report elements + config facts, one spec-referenced tokenizer with fail-closed ambiguity, stdout scrapers demoted to the no-report fallback, the parse-count cap removed, the clean rollup uncapped with per-project attribution, and the rule written into the design doc. I don't see a simpler path, and the diff carries nothing unrelated (the devDependencies reorder in package.json is an alphabetical-sort fix, negligible). Risk: no elevated risk signals — none of the revert-correlated paths are touched. One structural note for the reviewer: Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到，不是理论性的。父 PR（#8777）经过八轮 review 仍不断在手写的 Surefire XML 扫描上发现放行方向的漏洞（第 8 轮的具体发现在那里的维护者决定中被逐一点名），该决定冻结了补丁循环，要求的正是这件事：用真解析器关掉 XML 角落类，而不是继续逐个枚举。 方向：对齐——本 PR 就是该决定规定的关类重构，按决定要求拆成独立 PR，并按要求保留现有测试套件作为 oracle。合入顺序已声明且合理：父 PR 先行，本 PR 随后。 规模：不涉及核心模块路径（ 方案：范围合理，且与规定的设计逐点对应——严格抛错解析器（saxes，已因 jsdom 存在于 lockfile，未引入新包）、基于退出码 + 解析报告元素 + config 事实的 fail-closed 状态机、单趟规范引用 tokenizer 且歧义即 fail-closed、stdout 抓取器降为无报告时的 fallback、移除解析数量帽、干净汇总取消上限并保留逐项目归属、规则写入设计文档。没有发现更简路径，diff 也不含无关改动（package.json 里 devDependencies 的顺序调整是字母序修正，可忽略）。 风险：无升级风险信号——未触及任何与 revert 相关的高风险路径。给 reviewer 的一个结构性说明： 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewI wrote my independent proposal from the title and motivation first: swap the hand-rolled tag walk for a strict streaming parser (saxes was the obvious pick — already in the lockfile through jsdom), fail-closed on any parse rejection, restructure the verdict onto exit code + parsed elements + config facts, and keep the existing suite as the oracle. The PR matches that point for point, so the review was about whether the implementation actually holds the invariant. What I verified against the code at the reviewed commit:
No critical findings, no convention violations. Net −202 lines per the API (743+/945−; the production file alone is −313), and the diff carries nothing unrelated. TestingWhat CI evidence actually exists on this PR:
None of these is a test suite, and none will be: Sandboxed verification would settle the remaining gap now: Not verified: typecheck/build (no CI on this base); real-Maven end-to-end on the three new config-tokenizer spellings (the author marks it out of scope — unit oracle only). 中文说明代码审查我先按标题与动机写了独立方案：把手写的 tag walk 换成严格流式解析器（saxes 是显然的选择——已因 jsdom 存在于 lockfile）、解析拒绝即 fail-closed、裁决重构到退出码 + 解析元素 + config 事实上、现有套件继续作为 oracle。PR 与该方案逐点对应，因此审查重点是实现是否真的守住不变式。要点：解析器替换后首个良构性错误即否定整份报告，BOM 剥离避免把真实写家的合法报告误判；saxes 只展开五个预定义实体、忽略 DTD 实体声明，2 MiB 帽下不存在实体放大或外部实体面；解析数量帽的移除以"路径帽 ⇒ truncation ⇒ evidenceCapped"兜底，绿蕴含解析完整，旧帽的漏点由新的 TEST-ZFailed 用例钉死；状态机中"被怀疑的退出码 0 + 干净报告"的定罪臂虽不含依赖/启动措辞，但 Maven 对真实依赖解析死亡的框架是 Failed to execute goal，由 isGoalFailure 捕获，两个方向均有测试钉住；tokenizer 单趟分类， 无阻断性问题，无约定违规。按 API 计净 −202 行（743+/945−，生产文件自身 −313），无夹带改动。 测试本 PR 的 base 是父 PR 的分支，而 沙箱验证可以立刻补上这个缺口： 未验证：typecheck/build（此 base 无 CI）；新 tokenizer 三种拼写的真 Maven 端到端（作者声明超出范围，仅单测 oracle）。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean static review of exactly the refactor the maintainer decision prescribed; the one standing nit is that no test suite has executed on this stacked base, so the oracle passing rests on the author's word until CI fires at retarget. Stepping back: this is the rare case where the gate's skepticism meets a paper trail. The parent PR's eight rounds named the holes, the maintainer's freeze comment prescribed the class-closing point by point, and this PR delivers that prescription without drift. My independent proposal (saxes + fail-closed state machine + suite-as-oracle) is what the PR does; I probed the conviction arm, the cap removal, and the entity surface looking for a release-direction leak and found pins instead of gaps. The two semantic shifts are the strict direction, documented in the design doc. If I had to maintain this in six months, the commented state machine reads better than the hand-rolled scan it replaces, and the XML-corner class is closed by construction instead of enumeration. The 4 rather than 5: I could not run anything — no CI on this base, and this review never executes PR code — so "the oracle passes" is the author's macOS self-run until the merge path re-targets and main-based CI takes over (which it must, on either merge order). The gap and its remedy are named in the testing section ( Approving, pinned to the reviewed commit. When the parent lands and this retargets, branch protection dismisses the approval on the next push and the real suite takes over. 🚀 中文说明置信度：4/5 —— 对维护者决定所规定的重构做了干净的静态审查；唯一保留项是此 stacked base 上没有任何测试套件运行，oracle 是否通过在改 base 触发 CI 之前只有作者的说法。 回顾：这是门禁的怀疑态度遇到完整纸面记录的少见情形。父 PR 的八轮 review 点名了漏洞，维护者的冻结决定逐点规定了关类方案，本 PR 不走样地兑现了该规定。我的独立方案（saxes + fail-closed 状态机 + 套件作 oracle）与 PR 一致；我探查了定罪臂、帽移除与实体面，想找放行方向的泄漏，找到的是测试钉而不是缺口。两处语义偏移都是严格方向，且已写入设计文档。六个月后维护这段代码时，带注释的状态机比被替换的手写扫描更好读，XML 角落类由构造关闭而非枚举。 给 4 而不是 5：我无法运行任何东西——此 base 无 CI，审查也从不执行 PR 代码——"oracle 通过"在合入路径改 base、main-base CI 接手之前只是作者的 macOS 自测（无论哪种合入顺序都必须经过这一步）。缺口与补法已写在测试部分（ 按已审查的提交 approve。父 PR 合入、本 PR 改 base 后，分支保护会在下一次 push 时撤销该 approve，届时由真实套件接手。🚀 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
The Maven adapter's test oracle asserted exact substrings of the
human-facing notes ('infrastructure evidence', 'Correlate compiler or
test errors', 'Maven test passed') across ~97 call sites. Rewording any
of those phrases meant touching every site — the wording-churn risk the
maintenance review flagged.
Export the three markers as named constants from the adapter and emit
them in the notes; the tests import and assert on the constants. Each
branch's marker stays distinct, so the assertions still discriminate
which verdict branch produced a note — they only stop caring about the
surrounding prose. Rewording a marker is now a one-line contract edit
that every assertion follows, instead of a 97-site search-and-replace.
Scoped to maven-toolchain.test.ts: the npm adapter has its own
"Correlate…" wording, and build/test-plan assertions on npm notes are
left untouched.
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — I completed every check I opened; no check was left unfinished at the ceiling.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "PR #9099 (feat(review): add Maven multi-module…": none — all planned checks completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks I started were completed within budget., and 18 more.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 3.
Test Plan (not a blocker): ./mvnw — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — I completed every check I opened; no check was left unfinished at the ceiling.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"PR #9099 (feat(review): add Maven multi-module…"：none — all planned checks completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks I started were completed within budget.，另有 18 条。
未审查：反向审计——在 3 轮的反审轮数上限内未收敛。
Test Plan（非阻断）：./mvnw — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.11)
Addresses the 35 actionable findings from the round-1 review of the consolidated Maven toolchain PR. Adversarial evidence-channel shapes (symlinked wrapper target, printSummary suppression, POM-configured testFailureIgnore, forged stdout) are deferred as a class with rationale. State machine (maven-toolchain.ts): - Selector-rejection discard no longer drops runs whose evidence was rejected/truncated/rescue-overflow — those are the capped states the verdict fails closed on, and an echoed rejection line must not hide them behind an unsupported handoff. - testsSuppressed is no longer gated on the absence of reports (a module-local skip writes no reports while -am upstream modules do), and both it and neverRan exempt build-only runs (no test phase). - The config tokenizer fails closed on an existing-but-untokenizable maven.config (oversized/unreadable), on scope-altering flags (-pl/-rf/-N/-P), and classifies -Dtest/-Dit.test/ -Dsurefire.failIfNoSpecifiedTests as exit-semantics-changing like skipTests. -lax-checksums joins the valueless set. - acquisitionFailure is gated on !evidenceCapped so capped evidence cannot launder a source failure into infrastructure. test-plan.ts: - shellTokens keeps quoted-empty words (a real argv slot) instead of dropping them, so such claims reach the unknown-work check. - Phase matching lowercases tokens consistently with the runner check; MAVEN_UNRUN_WORK_RE gains site-deploy; MAVEN_SINGLE_DASH_LONGS gains the neutral-flag and repository-layout spellings; scopesNonPl models -llr/--legacy-local-repository/--lax-checksums. - owningModuleOf skips src-nested POMs (matching the adapter), so a planted pom.xml cannot re-own a compile failure out of the claim. - The -am carve-out excludes a capped run only when its failure location is positively known outside the claim; zero-attribution capped runs stay in matches for the capped cascade to rule. - cappedDefinitiveRuling and its ranking arm exclude infrastructure. - The bare-runner/path-claim guards key on work and runner position. test-delta.ts: a Maven run killed at its deadline after Surefire wrote failing reports joins the disclosure instead of being dropped. build-test.ts: the mixed-root Maven fallback fires on no-execution (install null and empty build/test), covering npm's zero-affected workspace return and pre-execution ok:false returns, not just the unsupported concession. Docs/notes: design doc's mixed-root rule and flag enumeration (six flags incl. swallowedReports) match the shipped code; capped-evidence notes and the evidenceCapped JSDoc drop the inaccurate log-file cause; Agent 7 brief qualifies the Maven scope sentence for reactor-wide runs and broadens the timeout-evidence exception. Tests: pin the infra+capped ruling, the mixed-root fallback boundaries, the file-level symlink defense, the diff-modified-wrapper caveat, and the new config-tokenizer behaviors.
The capped-definitive arm admitted a never-ran run that also carries evidenceCapped (rescueOverflow fired by a stub wrapper's own output is not proof the toolchain started), ruling it contradicted with wording describing a run that started — where the capped cascade promises unchecked/'Maven never started'. Return null for never-ran runs so the cascade's promised wording applies, matching the arm's own comment.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — the chunk was fully read in one un-truncated pass and all cross-checks completed.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "**Change context:** PR #9099 registers a Maven…": none — I did not run the vitest suites (out of my cross-file dimension's scope; the recommended tests cover the pins I read).; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — every claim in the chunk was verified against source., and 11 more.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Test Plan (not a blocker): ./mvnw — no such file or directory.
[Critical] R1-2 (round-1 finding, maven-toolchain.test.ts:2943): relocated-reports defense rests entirely on Surefire's stdout Tests run: summary, which printSummary=false switches off — exit-0 runs over failing tests can certify green with both evidence channels dark. Author deferred as adversarial evidence-channel class; the mechanism still stands at the reviewed commit.
[Critical] R1-3 (round-1 finding, maven-toolchain.test.ts:3405): wrapper-identity model compares changed files only against literal mvnw/mvnw.cmd/.mvn/wrapper/** paths — editing a symlinked wrapper's TARGET changes the executed bytes without setting executedWrapperChanged. Author deferred as adversarial; the mechanism still stands at the reviewed commit.
[Critical] R1-7 (round-1 finding, maven-toolchain.ts:1692): distrustExit0 is built solely from .mvn/maven.config facts; a POM-configured Surefire testFailureIgnore with relocated <reportsDirectory> can certify green. Author deferred as adversarial; the mechanism still stands at the reviewed commit.
[Critical] R1-11 (round-1 finding, maven-toolchain.ts:2194): the dependency/disk arms of acquisitionFailure read PR-forgeable stdout — an exit-0 run whose only failure evidence is dependency-worded stdout classifies as infrastructure. Author deferred as adversarial; the mechanism still stands at the reviewed commit.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："You are review agent reverse-audit — Reverse audit agent…"：none — the chunk was fully read in one un-truncated pass and all cross-checks completed.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"**Change context:** PR #9099 registers a Maven…"：none — I did not run the vitest suites (out of my cross-file dimension's scope; the recommended tests cover the pins I read).；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — every claim in the chunk was verified against source.，另有 11 条。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
Test Plan（非阻断）：./mvnw — no such file or directory。
[Critical] R1-2 (round-1 finding, maven-toolchain.test.ts:2943): relocated-reports defense rests entirely on Surefire's stdout Tests run: summary, which printSummary=false switches off — exit-0 runs over failing tests can certify green with both evidence channels dark. Author deferred as adversarial evidence-channel class; the mechanism still stands at the reviewed commit.
[Critical] R1-3 (round-1 finding, maven-toolchain.test.ts:3405): wrapper-identity model compares changed files only against literal mvnw/mvnw.cmd/.mvn/wrapper/** paths — editing a symlinked wrapper's TARGET changes the executed bytes without setting executedWrapperChanged. Author deferred as adversarial; the mechanism still stands at the reviewed commit.
[Critical] R1-7 (round-1 finding, maven-toolchain.ts:1692): distrustExit0 is built solely from .mvn/maven.config facts; a POM-configured Surefire testFailureIgnore with relocated <reportsDirectory> can certify green. Author deferred as adversarial; the mechanism still stands at the reviewed commit.
[Critical] R1-11 (round-1 finding, maven-toolchain.ts:2194): the dependency/disk arms of acquisitionFailure read PR-forgeable stdout — an exit-0 run whose only failure evidence is dependency-worded stdout classifies as infrastructure. Author deferred as adversarial; the mechanism still stands at the reviewed commit.
— qwen3.8-max via Qwen Code /review (v0.21.11)
Addresses the round-2 review's actionable findings; the deep adversarial evidence-channel shapes are deferred as a class with rationale. Config tokenizer (maven-toolchain.ts): - Split maven.config on all three line terminators Maven's Files.lines accepts (\n, \r\n, lone \r) — a CR-only config no longer mashes arguments into one token and bypasses scope-altering classification. - classifyDefine admits maven.test.skip.exec, the zero-test selection filters (-Dgroups/-DexcludedGroups/-Dsurefire.includesFile/ -Dsurefire.excludesFile), and maven.main.skip (a compile-skip fact); bare -Dmaven.repo.local[.tail] defines are dependency inputs. - Attached -T/--threads= spellings consume inert like the paired form. - markerSafe strips U+2028/U+2029 (line terminators for the m-flag marker-mining regexes). Verdict state machine: - Build-only runs distrust a bare exit 0 over maven.main.skip (the compile mojos they exist to verify were skipped). - detectMavenOwnership escalates a config-declared dependency input to reactor-wide ahead of the documentation/metadata exemptions. - The exit-0-over-fresh-failures note names the actual swallow cause (fail-never vs testFailureIgnore-style). test-plan.ts: - The -am carve-out never applies to capped runs (their markers are incomplete by construction) and carves a suppressed run only for a GLOBAL skip (new MavenCommandFacts.globalSkip), not a stdout-only module-local skip. - observedTestCounts excludes plain non-zero-exit runs (the commonest failed shape), mirroring ruleCommand's green-settlement family. - Comment fixes: capped never-ran wording, recorded-flag coexistence example, MAX_CONFIG_BYTES grammar. test-delta.ts: - The timed-out-failure arm gates on the structured Maven fact (t.maven) and broadens to stdout-only failure shapes — a timed-out npm test that merely echoes a marker is no longer admitted, and a timed-out Maven run with a failing Surefire summary no longer drops out. build-test.ts: MavenCommandFacts gains optional globalSkip. Tests: pin the CR-only config, the new classifyDefine properties, the bare repo.local input, the build-only compile-skip distrust, the .github config-input escalation, the timed-out-arm both directions, and the install-non-null fallback boundary; update the capped -am and suppressed pins to the new behavior.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 8: couldn't run the collocated vitest suite (no node_modules in the review worktree; didn't install to keep it lightweight) — static verification only.; chunk 8: could not run the collocated vitest suites ( build-test.test.ts covers this code extensively) — the review worktree has no node_modules and I did not install…; "agent reverse-audit (round 2)": none — no check left unfinished.; chunk 6: did not execute build-test.test.ts under vitest (worktree has no node_modules ; a full monorepo install was out of proportion to the remaining risk) — all ve…; "agent reverse-audit (round 1)": none — all planned checks completed (~15 of ~54 calls)., and 7 more.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Test Plan (not a blocker): ./mvnw — no such file or directory.
[Critical] R1-2 (round-1 finding, maven-toolchain.test.ts:2945): the relocated-reports defense rests entirely on Surefire's stdout Tests run: summary, which printSummary=false switches off — exit-0 runs over failing tests can certify green with both evidence channels dark. Author deferred as adversarial evidence-channel class; the mechanism still stands at the reviewed commit (no code in round 3 closes the shape; no summary-suppression distrust exists).
[Critical] R1-3 (round-1 finding, maven-toolchain.test.ts:3407): the wrapper-identity model compares changed files only against literal mvnw/mvnw.cmd/.mvn/wrapper/** paths — editing a symlinked wrapper's TARGET changes the executed bytes without setting executedWrapperChanged. Author deferred as adversarial; this round's wrapper-identity walk confirms the mechanism still stands at the reviewed commit.
[Critical] R1-7 (round-1 finding, maven-toolchain.ts:1771): distrustExit0 is built solely from .mvn/maven.config facts; a POM-configured Surefire testFailureIgnore with relocated <reportsDirectory> can certify green. Author deferred as adversarial; the mechanism still stands at the reviewed commit — and round 3's R3-9 executed the broader POM-channel green-wash on real Maven.
[Critical] R1-11 (round-1 finding, maven-toolchain.ts:2284): the dependency/disk arms of acquisitionFailure read PR-forgeable stdout — an exit-0 run whose only failure evidence is dependency-worded stdout classifies as infrastructure. Author deferred as adversarial; the mechanism still stands at the reviewed commit — and round 3's R3-10 executed a non-forgeable misclassification in the same arms.
[Critical] R2-6 (test-plan.ts:1440): owningModuleOf's planted-pom defense only rejects pom.xml beneath a src/ path with a TRAILING slash — a decoy pom at <module>/src/pom.xml (a dir ending in /src) re-owns an in-claim compile failure, and the -am carve-out discards a definitively failed in-claim run. Author deferred; RE-PROBED this round: {withoutPlantedPom: 'contradicted', withPlantedPom: 'unchecked'} — the mechanism still stands.
[Critical] R2-8 (maven-toolchain.ts:2009): the selector-rejection early return keys on the absence of VISIBLE fresh reports — an echoed rejection line combined with reports the sweep cannot see (a PR-relocated <reportsDirectory>) discards a run the scrapers would have convicted. Author deferred; RE-PROBED this round: exit 1 + forged rejection line + failing stdout summary + no visible reports → unsupported ok:true — the mechanism still stands.
[Critical] R2-9 (maven-toolchain.ts:712): deletion TOCTOU in the freshness layer — a PR whose tests run in the worktree can delete its own failing Surefire XML between write and sweep; the removal side of the freshness layer is unguarded. Author deferred; this round's freshness walk confirms the removal side still treats disappearance as never-existed (no rejected/truncated flag) and the diff adds no guard — the mechanism still stands.
[Critical] R2-13 (maven-toolchain.ts:2049): the [INFO] Tests are skipped. marker channel — the ONLY defense against a POM-local <skipTests> the config analyzer cannot see — is blinded by a PR-writable -q in .mvn/maven.config (quiet feeds only neverRan, structurally disabled whenever any reports exist). Author deferred; this round's state-machine walk confirms no quiet distrust was added — the mechanism still stands.
[Critical] R2-24 (maven-toolchain.ts:707) — ESCALATED Suggestion→Critical with executed evidence: the cap system bounds each single dimension but not the aggregate product — MAX_REPORT_PATHS (20,000) × MAX_REPORT_BYTES (2 MiB) of synchronous read+parse is reachable after the build returns, outside any deadline check. Author deferred as self-DoS ('verdict already fail-closed'); RE-PROBED this round with executed numbers: 20 sparse 2 MiB reports → 116.8 s (~5.8 s/file; saxes scans malformed input to EOF), extrapolated ~32 hours at the cap product; the cumulative-byte-budget fix collapses the same input to 12 ms. A review harness stalled for hours on PR-controlled input is a denial of the review itself, which the deferral's rationale does not answer.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 8：couldn't run the collocated vitest suite (no node_modules in the review worktree; didn't install to keep it lightweight) — static verification only.；chunk 8：could not run the collocated vitest suites ( build-test.test.ts covers this code extensively) — the review worktree has no node_modules and I did not install…；"agent reverse-audit (round 2)"：none — no check left unfinished.；chunk 6：did not execute build-test.test.ts under vitest (worktree has no node_modules ; a full monorepo install was out of proportion to the remaining risk) — all ve…；"agent reverse-audit (round 1)"：none — all planned checks completed (~15 of ~54 calls).，另有 7 条。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
Test Plan（非阻断）：./mvnw — no such file or directory。
[Critical] R1-2 (round-1 finding, maven-toolchain.test.ts:2945): the relocated-reports defense rests entirely on Surefire's stdout Tests run: summary, which printSummary=false switches off — exit-0 runs over failing tests can certify green with both evidence channels dark. Author deferred as adversarial evidence-channel class; the mechanism still stands at the reviewed commit (no code in round 3 closes the shape; no summary-suppression distrust exists).
[Critical] R1-3 (round-1 finding, maven-toolchain.test.ts:3407): the wrapper-identity model compares changed files only against literal mvnw/mvnw.cmd/.mvn/wrapper/** paths — editing a symlinked wrapper's TARGET changes the executed bytes without setting executedWrapperChanged. Author deferred as adversarial; this round's wrapper-identity walk confirms the mechanism still stands at the reviewed commit.
[Critical] R1-7 (round-1 finding, maven-toolchain.ts:1771): distrustExit0 is built solely from .mvn/maven.config facts; a POM-configured Surefire testFailureIgnore with relocated <reportsDirectory> can certify green. Author deferred as adversarial; the mechanism still stands at the reviewed commit — and round 3's R3-9 executed the broader POM-channel green-wash on real Maven.
[Critical] R1-11 (round-1 finding, maven-toolchain.ts:2284): the dependency/disk arms of acquisitionFailure read PR-forgeable stdout — an exit-0 run whose only failure evidence is dependency-worded stdout classifies as infrastructure. Author deferred as adversarial; the mechanism still stands at the reviewed commit — and round 3's R3-10 executed a non-forgeable misclassification in the same arms.
[Critical] R2-6 (test-plan.ts:1440): owningModuleOf's planted-pom defense only rejects pom.xml beneath a src/ path with a TRAILING slash — a decoy pom at <module>/src/pom.xml (a dir ending in /src) re-owns an in-claim compile failure, and the -am carve-out discards a definitively failed in-claim run. Author deferred; RE-PROBED this round: {withoutPlantedPom: 'contradicted', withPlantedPom: 'unchecked'} — the mechanism still stands.
[Critical] R2-8 (maven-toolchain.ts:2009): the selector-rejection early return keys on the absence of VISIBLE fresh reports — an echoed rejection line combined with reports the sweep cannot see (a PR-relocated <reportsDirectory>) discards a run the scrapers would have convicted. Author deferred; RE-PROBED this round: exit 1 + forged rejection line + failing stdout summary + no visible reports → unsupported ok:true — the mechanism still stands.
[Critical] R2-9 (maven-toolchain.ts:712): deletion TOCTOU in the freshness layer — a PR whose tests run in the worktree can delete its own failing Surefire XML between write and sweep; the removal side of the freshness layer is unguarded. Author deferred; this round's freshness walk confirms the removal side still treats disappearance as never-existed (no rejected/truncated flag) and the diff adds no guard — the mechanism still stands.
[Critical] R2-13 (maven-toolchain.ts:2049): the [INFO] Tests are skipped. marker channel — the ONLY defense against a POM-local <skipTests> the config analyzer cannot see — is blinded by a PR-writable -q in .mvn/maven.config (quiet feeds only neverRan, structurally disabled whenever any reports exist). Author deferred; this round's state-machine walk confirms no quiet distrust was added — the mechanism still stands.
[Critical] R2-24 (maven-toolchain.ts:707) — ESCALATED Suggestion→Critical with executed evidence: the cap system bounds each single dimension but not the aggregate product — MAX_REPORT_PATHS (20,000) × MAX_REPORT_BYTES (2 MiB) of synchronous read+parse is reachable after the build returns, outside any deadline check. Author deferred as self-DoS ('verdict already fail-closed'); RE-PROBED this round with executed numbers: 20 sparse 2 MiB reports → 116.8 s (~5.8 s/file; saxes scans malformed input to EOF), extrapolated ~32 hours at the cap product; the cumulative-byte-budget fix collapses the same input to 12 ms. A review harness stalled for hours on PR-controlled input is a denial of the review itself, which the deferral's rationale does not answer.
— qwen3.8-max via Qwen Code /review (v0.21.11)
| (/^\[maven-test-failure\] /m.test(t.output ?? '') || | ||
| (t.output ?? |
There was a problem hiding this comment.
[Critical] R3-1: Operator precedence defeats the stdout-failure-evidence gate in the widened timed-out failed filter: the expression parses as t.output ?? (''.split('\n').some(...)), so for any non-empty output the disjunct evaluates to the output string itself (truthy) and the three line classifiers are dead code — every timed-out Maven command with non-empty output is admitted as 'failed'. — Failure scenario: a Maven run killed by its deadline with only [INFO] output (no failure evidence) enters failed, lands in skippedUnrecognised, and the report asserts unattributed test failures for a pure infrastructure timeout, blocking the all-clear path — contradicting the design's 'timeout is never a finding' contract. Probe-verified through real runTestDelta (PR code admits; the fix flips to all-clear while the failing-summary twin is still admitted). — Suggested fix: rewrap as ((t.output ?? '').split('\n').some(...)); add a negative test (timed-out + benign output not admitted).
中文说明
运算符优先级使超时 failed 过滤器的 stdout 失败证据门失效：该表达式实际解析为 t.output ?? (''.split('\n').some(...))——只要输出非空，该分支就取到输出字符串本身（真值），三个行分类器成为死代码，任何输出非空的超时 Maven 命令都会被视为“失败”。失败场景：被 deadline 杀死、输出只有 [INFO]（无失败证据）的 Maven 运行进入 failed，报告对纯基础设施超时断言“失败未归因”，挡住 all-clear——与“超时永不构成 finding”的设计契约相悖。已用真实 runTestDelta 探针验证（PR 代码准入；修复后翻转为 all-clear，失败汇总孪生形态仍被准入）。建议修复：改写为 ((t.output ?? '').split('\n').some(...))，并补“超时+无害输出不被准入”的负例测试。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| if (gitHasPath(worktree, baseSha, 'pom.xml')) { | ||
| return unavailable(mavenBaseNote); | ||
| } |
There was a problem hiding this comment.
[Critical] R3-2: The root-pom gate runs BEFORE npmAtBase is computed, so a mixed-root base (root pom.xml plus an npm-applicable manifest/workspaces) returns unavailable and permanently disables A/B attribution — contradicting this same PR's PR-side rule that prefers and runs npm on exactly this shape (whose recorded npm commands test-delta's rerun grammar consumes). The PR's own tests pin both halves of the asymmetry green. — Failure scenario: on exactly the polyglot roots the PR side now certifies via npm, Agent 7 loses measurement permanently and falls back to the path rule the brief itself says misclassifies in both directions — an environment-flaky test in a touched file gets filed as a Critical against the PR where base-tree measurement would have demoted it to pre-existing. — Suggested fix: compute npmAtBase first and gate the root-pom return on its absence.
中文说明
root-pom 闸门先于 npmAtBase 计算，混合根 base（根 pom.xml + npm 适用的 manifest/workspaces）直接返回 unavailable，永久禁用 A/B 归因——与本 PR 在 PR 侧对同一形态“优先并运行 npm”的规则矛盾（其记录的 npm 命令可被 test-delta 的重跑语法消费）。PR 自己的两个测试把这一不对称的两半都钉成绿。失败场景：恰在 PR 侧如今用 npm 认证的多语言根上，Agent 7 永久失去测量手段，回退到 brief 自己承认“双向都会误分类”的路径规则——被触及文件中的环境性 flaky 测试会被当作 Critical 归给 PR，而 base-tree 测量本可将其降级为既有失败。建议修复：先计算 npmAtBase，仅在其为假时才因 root pom 返回 unavailable。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| const caseLines = failing.flatMap((summary) => { | ||
| const cases = summary.failedCases.map( | ||
| (testcase) => | ||
| `[maven-test-failure] ${markerSafe(summary.report)}: ${markerSafe(testcase)}`, |
There was a problem hiding this comment.
[Critical] R3-3: Marker lines embed PR-controlled testcase names sanitized only for line terminators, and the consumer failureInsideClaim (test-plan.ts:1473) substring-mines that output WITHOUT line anchoring — while the sibling freshTestFailures IS line-anchored. — Failure scenario: probe-verified — a green claimed module's testcase name carrying embedded [maven-test-failure] <module>/target/… text (or a module dir name containing failures=9 forging the rollup arm) flips the -am carve-out, and the claim reads contradicted on a failure that lives outside the claim's scope. The design doc's own rule: verdicts that must survive a hostile PR belong in structured fields, not mined text. — Suggested fix: line-anchor the consumer (per-line startsWith('[maven-test-failure] ' + prefix + 'target/') — markerSafe already forbids forged line breaks) and parse the structured trailing : tests=…, failures=N fields instead of substring-mining failures=[1-9].
中文说明
marker 行内嵌 PR 可控的 testcase 名（仅过滤行终止符），而消费方 failureInsideClaim（test-plan.ts:1473）对该输出做未行锚定的子串挖掘——兄弟谓词 freshTestFailures 却是行锚定的。失败场景：已探针验证——绿色被声明模块的 testcase 名内嵌 [maven-test-failure] <module>/target/… 文本（或模块目录名含 failures=9 伪造 rollup 分支）即可翻转 -am carve-out，使声明因声明范围之外的失败被判 contradicted。设计文档自身的规则：必须在 hostile PR 下存活的裁决应放在结构化字段而非挖掘文本。建议修复：消费方按行锚定（逐行 startsWith('[maven-test-failure] ' + prefix + 'target/')——markerSafe 已禁止伪造换行），并解析行尾结构化字段 : tests=…, failures=N，不再子串挖掘 failures=[1-9]。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| /^(groups|excludedGroups)=/.test(property) || | ||
| /^surefire\.(includes|excludes)File=/.test(property) |
There was a problem hiding this comment.
[Critical] R3-4: The zero-test filter regexes require =, but Maven defaults a VALUELESS define to true — the exact semantics this same function patches for bare maven.repo.local. Verified on real Maven 3.8.7 / Surefire 3.2.5: .mvn/maven.config with the single line -Dgroups → Tests run: 0, BUILD SUCCESS, exit 0, zero TEST-*.xml, no skip marker; adapter probe: bare -Dgroups → ok: true while -Dgroups=nonexistent → ok: false, testsSuppressed: true — the bare spelling is trusted, the = sibling distrusted: a release-direction inversion of 'imprecision can only be stricter'. — Suggested fix: make the = optional.
| /^(groups|excludedGroups)=/.test(property) || | |
| /^surefire\.(includes|excludes)File=/.test(property) | |
| /^(groups|excludedGroups)(=.*)?$/.test(property) || | |
| /^surefire\.(includes|excludes)File=/.test(property) |
中文说明
零测试过滤正则要求 =，但 Maven 把无值 define 默认为 true——本函数已为裸 maven.repo.local 补丁过完全相同的语义。真实 Maven 3.8.7 / Surefire 3.2.5 验证：maven.config 单行 -Dgroups → Tests run: 0、BUILD SUCCESS、退出 0、零 TEST-*.xml、无跳过标记；adapter 探针：裸 -Dgroups → ok: true，而 -Dgroups=nonexistent → ok: false, testsSuppressed: true——裸拼写被信任，= 兄弟被不信任：对“不精确只会更严”的放行方向翻转。建议修复：使 = 可选。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| dependencyInputs: string[]; | ||
| } | ||
|
|
||
| function analyzeMavenConfig(root: string): MavenConfigFacts { |
There was a problem hiding this comment.
[Critical] R3-5: The exit-0 model ingests ONLY .mvn/maven.config; the sibling PR-writable .mvn/jvm.config injects the same exit-0-altering properties as JVM system properties via MAVEN_OPTS, which plugin @Parameter(property=…) resolution honors. Verified on real Maven (deliberately broken source): control mvn test-compile → exit 1; with .mvn/jvm.config = -Dmaven.main.skip=true → exit 0, BUILD SUCCESS, zero .class files; the adapter over the identical shape returns ok: true with no classification flag — a diff that compiled nothing certified green. The maven.config twin IS modeled (the R2-7 fix); this channel is not. — Suggested fix: tokenize .mvn/jvm.config with the same line-based reader and size/ambiguity semantics, classify its -D… entries through the same classifyDefine, and OR the resulting facts into configFacts.
中文说明
退出 0 模型只摄取 .mvn/maven.config；同为 PR 可写的 .mvn/jvm.config 会经 MAVEN_OPTS 以 JVM 系统属性形式注入同样能改变退出 0 语义的属性，插件 @Parameter(property=…) 解析会采纳。真实 Maven 验证（故意破坏源码）：对照 mvn test-compile 退出 1；加入 .mvn/jvm.config = -Dmaven.main.skip=true 后退出 0、BUILD SUCCESS、零 .class 文件；adapter 对同一形态返回 ok: true 且无任何分类标志——把“什么都没编译”的 diff 认证为绿。maven.config 孪生渠道已建模（R2-7 修复），此渠道未建模。建议修复：用同一行式读取器与大小/歧义语义分词 .mvn/jvm.config，其 -D… 条目经同一 classifyDefine 分类，并把所得事实并入 configFacts。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| // environmental arm is its only consumer — so the carve-out must not | ||
| // hide a capped one from that arm. | ||
| !c.infrastructure && | ||
| (finished(c) || c.evidenceCapped === true) && |
There was a problem hiding this comment.
[Suggestion] R3-27: Dead, self-contradicting disjunct: finished() already requires !c.evidenceCapped, and the predicate's trailing conjunct c.evidenceCapped !== true rejects capped runs unconditionally — mutation-verified dead (deleting it ships the full suite green, 195/195). The block comment ('A CAPPED run is NEVER carved') is enforced solely by the trailing clause; the disjunct reads as its opposite and invites a reconciling edit that flips capped runs into the carve-out — excluded on failureInsideClaim computed from markers incomplete by construction. Reported by three independent auditors. — Suggested fix: delete the disjunct.
| (finished(c) || c.evidenceCapped === true) && | |
| finished(c) && |
中文说明
死的自相矛盾分支：finished() 已要求 !c.evidenceCapped，谓词尾部合取项 c.evidenceCapped !== true 又无条件拒绝 capped 运行——变异验证为死代码（删除后全套件绿，195/195）。块注释（“capped 运行永不被 carve”）仅由尾部子句强制；该分支读起来恰与之相反，诱使“调和性”编辑把 capped 运行翻入 carve-out——按构造即不完整的 marker 计算的 failureInsideClaim 会将其排除。三位审计者独立报告。建议修复：删除该分支。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| command: './mvnw test', | ||
| exitCode: null, | ||
| seconds: 300, | ||
| timedOut: true, |
There was a problem hiding this comment.
[Suggestion] R3-28: The cmd.timedOut exclusion disjunct is never pinned alone — all seven timedOut fixtures (both toolchains) bundle exitCode: null; the fixtures even pin the null-exit disjunct ALONE with a comment noting the bundling. Mutation-verified: removing cmd.timedOut || leaves all 16 observedTestCounts tests green. The shape is producible: spawnTimedOut's authoritative ETIMEDOUT arm places no constraint on status, and exitCode copies r.status straight — a deadline kill racing a child with a real exit status passes every other gate in the family. — Suggested fix: add a timedOut-with-numeric-exitCode fixture pair expecting exclusion.
中文说明
cmd.timedOut 排除分支从未被单独钉住——全部 7 处 timedOut fixture（双工具链）都捆绑 exitCode: null；fixture 甚至单独钉住 null-exit 分支并以注释说明捆绑。变异验证：移除 cmd.timedOut || 后 16 个 observedTestCounts 测试全绿。该形态可产生：spawnTimedOut 的权威 ETIMEDOUT 分支不约束 status，exitCode 直接复制 r.status——deadline 杀死与带真实退出码的子进程竞态时可通过家族中所有其他门。建议修复：补带数值退出码的 timedOut fixture 对并断言被排除。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| const runnerHead = | ||
| MAVEN_RUNNER_RE.test(tokens[0] ?? '') && hasWorkBeyondRunner(tokens); |
There was a problem hiding this comment.
[Suggestion] R3-29: A cd-prefixed Maven span (cd moduleA && ./mvnw test — a normal nested-module spelling) yields ZERO claims: the ^-anchored runner regex fails at the span site, and runnerHead suppresses the path claim at the mining site. Probe-verified: extractClaims returns [] and runTestPlan rules nothing; pre-change the loop pushed the checkable moduleA/./mvnw claim. Caveat the author needs: the PR's own test 'does not lift a Maven runner out of its command span' deliberately pins the suppression — but nothing argues the span should produce no claim at all, and a fix will have to update that pin. — Suggested fix: gate the suppression on the span actually yielding a command claim (or skip suppression when a cd base is present).
中文说明
cd 前缀的 Maven span（cd moduleA && ./mvnw test——常见的嵌套模块写法）产出零声明：^ 锚定的 runner 正则在 span 层失败，runnerHead 又在挖掘层吞掉路径声明。探针验证：extractClaims 返回 []，runTestPlan 不结算任何声明；改动前循环会推出可检查的 moduleA/./mvnw 声明。作者需注意：PR 自己的测试 'does not lift a Maven runner out of its command span' 有意钉住了抑制——但没有任何理据说明 span 应产出零声明，修复时需一并更新该钉。建议修复：仅当 span 确实产出命令声明时才抑制（或存在 cd 基目录时不抑制）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| expect(rep.toolchain).toBe('maven'); | ||
| expect(rep.ok).toBe(true); | ||
| expect(rep.note).not.toContain('executed the npm toolchain only'); |
There was a problem hiding this comment.
[Suggestion] R3-30: Same spy-gap pattern as R3-14. This workspace-mixed-root fallback test cannot distinguish 'the Maven half ran' from Maven's zero-module early return — probe-verified: replaying the fixture with a changed file ownership maps to zero modules hits exactly that return ({toolchain:'maven', ok:true, 'complete answer', exec called 0 times}), yet all three assertions pass. The early return precedes the mixed-root note append, so only the sibling cold-yarn test incidentally catches the shape — while this test's own comment names R1-22 ('changed modules never compiled or tested') as its reason to exist. — Suggested fix: assert something actually ran (expect(rep.build.length + rep.test.length).toBeGreaterThan(0)) or mirror the sibling's mixed-note assertion.
中文说明
与 R3-14 同一 spy 缺口模式。此 workspace 混合根回退测试无法区分“Maven 半程真的执行”与 Maven 零模块早退——探针验证：把变更文件改成归属映射为零模块，恰好命中该早退（{toolchain:'maven', ok:true, 'complete answer', exec 调用 0 次}），三个断言仍全过。该早退先于混合根 note 追加，因此只有兄弟 cold-yarn 测试偶然覆盖此形态——而本测试的注释自称因 R1-22（“变更模块从未编译或测试”）而存在。建议修复：断言确有执行（expect(rep.build.length + rep.test.length).toBeGreaterThan(0)）或对齐兄弟测试的混合根 note 断言。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| const omitted = s.length - KEEP_HEAD - KEEP_TAIL; | ||
| const matched: Array<{ line: string; index: number; evidence: boolean }> = []; | ||
| middle.split('\n').forEach((line, index) => { | ||
| // The widening loop reads module errors from the ORIGINAL bytes. |
There was a problem hiding this comment.
[Suggestion] R3-31: The new inline comment contradicts the retained block comment nine lines above and the code: run() returns output: trimmed.text (build-test.ts:413 — raw bytes are never stored), and the widening loop calls unresolvedWorkspaceDeps(failure.output, …) (npm-toolchain.ts:542) — it reads the TRIMMED output. — Failure scenario: a maintainer reading 'widening reads the ORIGINAL bytes' concludes the MODULE_ERROR_RE rescue is dead weight and deletes it; a long TypeScript log then pushes a Cannot find module line past the head and before the tail, widening ends early, and a real dependency-graph gap surfaces as a false build error against the PR — exactly the failure the block comment documents the rescue as preventing. — Suggested fix: reword to match the code (the rescue matches module errors against the original, unstripped line, and the rescued line keeps those bytes, which the widening loop reads back from the trimmed output).
| // The widening loop reads module errors from the ORIGINAL bytes. | |
| // The widening loop reads these rescued lines back from the TRIMMED output. |
中文说明
新增行内注释与上方保留的块注释及代码均矛盾：run() 返回 output: trimmed.text（build-test.ts:413——原始字节从不保存），widening 循环调用 unresolvedWorkspaceDeps(failure.output, …)（npm-toolchain.ts:542）——读的是裁剪后的输出。失败场景：维护者按“widening 读取原始字节”推断 MODULE_ERROR_RE 救援是死代码并删除；长 TypeScript 日志随后把 Cannot find module 行推出头部、又早于尾部，widening 提前结束，真实依赖图缺口会以针对 PR 的假构建错误浮现——正是块注释记载该救援所要防止的失败。建议修复：改写注释与代码一致（救援按未剥离的原始行匹配模块错误，被救援行保留这些字节，widening 循环再从裁剪后输出读回它们）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
3 participants
What this PR does
Registers a Maven adapter on the toolchain boundary.
review build-testnow recognizes Maven roots, maps changed files to the owning module, prefers the root Maven Wrapper, runs one scopedtest(or build-onlytest-compile) from the reactor root with-amupstream expansion, and emits module-qualified Surefire/Failsafe evidence limited to reports the invocation itself created or updated.The evidence is read by a strict, throwing XML parser, and the pass/fail verdict is a fail-closed state machine over three authoritative signals: the exit code, the parsed report elements, and the build-config settings that change what exit 0 means. Green requires positive structural evidence — a parsed report with no failing element — never the mere absence of a failure substring. A report the parser rejects (oversized, unreadable, or malformed) is unknown evidence and never reads green. The human-stdout scrapers are a fallback for runs with no reports; with reports present the structured signals judge, and the scrapers can only convict under a distrusted exit 0.
Downstream consumers follow:
test-plansettles Maven command and Surefire count claims, Agent 7 gains a Maven branch, and Maven failures do not enter the npm-onlytest-deltarerun path.base-treeskips Maven merge bases in this release (its A/B consumer is npm-only, so a Maven base build would be cost without a consumer); the gate lifts when Maven delta attribution exists.Why it's needed
The build-and-test path produced deterministic evidence for npm projects but handed Maven projects back to an agent for ad hoc command selection. Module ownership, command choice, result parsing, timeout classification, and failure attribution were all agent decisions, which is the weakest possible foundation for a review verdict.
Driven by active use in
alibaba/fastjson2andalibaba/druid: both are real multi-module reactors where core changes must run through the root wrapper, and where nested, profile-activated, and standalone POMs make a naive model unsafe.Design note: Maven owns the reactor
This adapter does not model the Maven reactor. It reads Maven's answer back:
pom.xmlwalk, skipping directories beneath asrc/tree (a POM there is maven-invoker or archetype test data); when that skip would collapse a changed path to the root project, the run widens to the full reactor instead — a reactor can aggregate a real module undersrc/, and-pl .would compile only the root.<modules>inheritance — is answered by Maven rejecting the-plselector (Could not find the selected project in the reactor), which it does before compiling anything. That rejection becomes theunsupportedhandoff.Design note: close the XML-corner class with a real parser
Surefire/Failsafe XML is PR-controlled, untrusted input. An earlier revision of this work hand-rolled its parser — a tag walk, attribute regexes, and CDATA/comment stripping with swallowing probes — and eight review rounds each found the next corner, because XML has infinitely many. This is the same anti-pattern the review skill hit with a hand-rolled CommonMark scanner and closed by adopting
markdown-it(#9020). This PR adopts a strict, throwing XML parser (saxes) instead: well-formedness, CDATA, comments, entities, self-closing tags, and nesting are the parser's job, and a parse rejection fails closed. The same lesson is written intodocs/design/review-toolchain-adapters.mdas the rule for judging untrusted tool output.With it: the fresh-report parse-count cap is gone (it was itself a leak — a failing report ordered past the cap once certified green); every fresh report is parsed, bounded only by the sweep's path cap and the per-file size cap; and the clean evidence rollup is uncapped, one attributed line per project. The
.mvn/maven.configreader is one spec-referenced tokenizer pass that fails closed on ambiguity: a flag it cannot classify makes the verdict distrust a bare exit 0, so imprecision can only ever be stricter, never release.Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review/— 68 files, 2795 tests; all pass except two pre-existing machine-environment failures unrelated to this change (the bundled-skill tmpdir-symlink case and the real-git probe timeout)../mvnw --batch-mode --no-transfer-progress -pl core -am test, while a changed POM,.mvn/**, or the wrapper runs the full reactor and build-only mode usestest-compile.-plrejection with no fresh reports and check it returnstoolchain: "unsupported"naming the project; with fresh reports present the same wording is treated as echoed test stdout and the run keeps its captured evidence.<failure>sample inside CDATA or a comment is opaque text (no phantom failure, no forged green); a malformed report (multi-root, unterminated section, truncated mid-case) is rejected fail-closed; an escaped</testcase>attribute value parses cleanly; a failing report ordered past a thousand siblings is still evidence.,,:,%, a leading-,!, or+, or surrounding whitespace cannot reach a-plselector and widens the run to the full reactor with a disclosure. These are directory names read off disk, so this gate is the only thing between them and ashell: truecommand line../mvnw testclaim to be settled by the scoped command actually run, but does not equate claims carrying different-Pprofiles or-plscopes.Evidence
The review suite is the oracle: the contract pins pass under the strict parser, and the cases that pinned hand-rolled mechanics collapsed to structural pins (malformed ⇒ fail-closed, CDATA/comments opaque, adversarial shapes rejected in bounded time). Adapter behaviour is exercised through the injectable executor, so no test spawns Maven or touches the network.
Prettier and ESLint clean;
tsc --noEmitreports no errors undersrc/commands/review/(the errors it does report are pre-existing and confined toserve/andacp-integration/, an unbuiltpackages/acp-bridge/distin a fresh worktree).Tested on
Risk & Scope
src/fixture tree,.mvn/**, the settings or repository locations.mvn/maven.confignames, or the executed wrapper). Over-attributing costs a visible failure carrying Maven's own output; under-attributing would ship the diff's own breakage as someone else's outage, so the carve-outs are deliberately narrow.verifyexecution, JaCoCo, mutation testing, Maven-aware base-sidetest-delta, and mixed-toolchain aggregation. Downstream (-amd) expansion is deliberately not used: it selects the whole reactor on exactly the repositories this targets, and a run that spends its whole deadline proves nothing — downstream coverage stays with the project's CI matrix.BuildTestReport.toolchainadds the"maven"discriminant, andCommandResultgains optional classification flags (infrastructure,swallowedFailure,evidenceCapped,testsSuppressed,neverRan,swallowedReports). Existing npm behaviour and report fields are unchanged; roots where both npm and Maven apply fail closed instead of choosing one implicitly.packages/cligains asaxesdependency.Linked Issues
Supersedes #8777 (closed in favor of this consolidated PR). Replaces #8394, which was split into #8776 (merged) and this work.
中文说明
此 PR 的改动
在 toolchain 边界上注册 Maven adapter。
review build-test现在可以识别 Maven 根目录，把变更文件映射到所属模块，优先使用根 Maven Wrapper，从 reactor 根目录执行一条带-am上游扩展的 scopedtest（或 build-onlytest-compile），并输出仅限本次执行新建或更新的、带模块路径的 Surefire/Failsafe 证据。证据由严格、会抛异常的 XML 解析器读取；pass/fail 裁决是基于三个权威信号的 fail-closed 状态机：退出码、解析后的报告元素、以及改变"退出码 0 含义"的构建配置。绿必须有正向结构证据——解析出的报告里没有失败元素——绝不仅仅因为"没匹配到失败子串"。解析器拒绝的报告（超大、不可读或畸形）是未知证据，绝不读绿。人类 stdout 抓取器只是"无报告时的 fallback"：有报告时由结构化信号裁决，抓取器只可在"退出码 0 不被信任"时定罪，不能放行。
下游随之跟进：
test-plan可结算 Maven 命令与 Surefire 计数声明，Agent 7 增加 Maven 分支，Maven 失败不会进入仅支持 npm 的test-delta重跑路径。base-tree本版本跳过 Maven merge base（其 A/B 消费方仅支持 npm，构建 Maven base 有成本无消费方）；待 Maven delta 归因存在后放开该闸门。为什么需要
原有的构建与测试路径能为 npm 项目产出确定性证据，但 Maven 项目只能交还给 agent 临时选择命令。模块归属、命令选择、结果解析、超时分类、失败归因全部是 agent 的判断——这是 review 结论所能依赖的最薄弱的基础。
由
alibaba/fastjson2和alibaba/druid的实际使用驱动：两者都是真实的多模块 reactor，core 变更必须通过根 wrapper 验证，而嵌套模块、profile 激活模块和独立 POM 使得朴素建模并不安全。设计说明：reactor 由 Maven 说了算
本 adapter 不为 Maven reactor 建模，而是读回 Maven 的答案：
pom.xml目录，跳过src/树下的目录（那里的 POM 是 maven-invoker 或 archetype 测试数据）；当这个跳过会把变更路径塌缩到根项目时，运行扩大到完整 reactor——reactor 完全可能把src/下的真实模块聚合进来，而-pl .只会编译根项目。<modules>继承下是否属于 reactor，由 Maven 拒绝-pl选择器来回答（Could not find the selected project in the reactor），而且它在编译任何东西之前就返回。这个拒绝会转成unsupported交接。设计说明：用真解析器关掉 XML 角落类
Surefire/Failsafe XML 是 PR 可控的不可信输入。本工作的早先版本手搓解析器——tag walk、属性正则、带吞咽探针的 CDATA/注释剥离——八轮 review 每轮都找到下一个角落，因为 XML 的角落无穷多。这与本 skill 曾用手搓 CommonMark 扫描器、最终换用
markdown-it才关掉的反模式完全相同（#9020）。本 PR 改用严格、会抛异常的 XML 解析器（saxes）：良构性、CDATA、注释、实体、自闭合标签、嵌套全部交给解析器，解析被拒即 fail-closed。同一条经验作为"判定不可信工具输出"的规则写入了docs/design/review-toolchain-adapters.md。随之：新鲜报告的解析数量上限被移除（它本身就是漏点——排在帽外的失败报告曾认证为绿）；每一份新鲜报告都被解析，成本仅由扫描路径上限与单文件大小上限约束；干净证据汇总取消上限，每个项目一行带归属。
.mvn/maven.config读取器是一趟引用规范的 tokenizer，歧义即 fail-closed：无法分类的标志让裁决不信任裸的退出码 0，使不精确只会更严、永不放行。Reviewer Test Plan
验证方式
cd packages/cli && npx vitest run src/commands/review/——68 个文件、2795 个测试；除两个与本改动无关的既有机器环境失败（bundled-skill 的 tmpdir 符号链接用例、real-git 探针超时）外全部通过。./mvnw --batch-mode --no-transfer-progress -pl core -am test；而 POM、.mvn/**或 wrapper 变更运行完整 reactor，build-only 模式使用test-compile。-pl拒绝，检查它返回toolchain: "unsupported"并指明项目；有新鲜报告时同样的措辞按测试 stdout 回显处理，运行保留其已捕获的证据。<failure>样本是不透明文本（无幻影失败、无伪造绿）；畸形报告（多根、未终止段、中途截断）fail-closed 拒绝；转义的</testcase>属性值正常解析；排在一千个兄弟报告之后的失败报告仍是证据。,、:、%、前导-/!/+或首尾空白时不能进入-pl选择器，运行会扩大到完整 reactor 并加以说明。这些是从磁盘读到的目录名，此闸门是它们与shell: true命令行之间唯一的屏障。./mvnw test声明可以由实际执行的 scoped 命令结算，但带不同-Pprofile 或-plscope 的声明不会被视为同一次执行。证据
review 套件即 oracle：契约钉在严格解析器下通过；钉手搓机制的用例坍缩为结构化钉（畸形 ⇒ fail-closed、CDATA/注释不透明、对抗形态有界时间拒绝）。adapter 行为通过可注入的 executor 验证，没有任何测试会拉起 Maven 或触网。
Prettier 与 ESLint 通过；
tsc --noEmit在src/commands/review/下无错误（其报告的错误均为既有问题且集中在serve/与acp-integration/，全新 worktree 中未构建的packages/acp-bridge/dist）。测试平台
风险与范围
src/fixture 树之外的任何 POM、.mvn/**、.mvn/maven.config指向的 settings 或仓库位置、以及实际执行的 wrapper）。多归因的代价是一条带 Maven 原始输出的可见失败；少归因则会把 diff 自己的破坏当成别人的故障，因此这些豁免被刻意收窄。verify执行、JaCoCo、变异测试、Maven 感知的 base 侧test-delta，以及多 toolchain 聚合。下游（-amd）扩展被刻意排除：在本工作针对的这些仓库上它会选中整个 reactor，而一次耗尽全部 deadline 的运行什么也证明不了——下游覆盖仍由项目 CI 矩阵负责。BuildTestReport.toolchain增加"maven"判别值，CommandResult增加可选分类标志（infrastructure、swallowedFailure、evidenceCapped、testsSuppressed、neverRan、swallowedReports）。现有 npm 行为与报告字段不变；npm 与 Maven 同时适用的根目录会 fail closed，而不是隐式二选一。packages/cli新增saxes依赖。关联 Issue
取代 #8777（为支持本合并 PR 而关闭）。替换 #8394——它被拆分为 #8776（已合入）与本工作。