feat(review): Add Maven multi-module verification - #8394
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Thanks for the PR! Template looks good ✓ Problem: this is a structural refactoring — Direction: the extraction itself is reasonable — the file is long and the npm algorithm is self-contained. The adapter interface (28 lines) is minimal. That said, the design doc's "Future phases" (Maven, Gradle, coverage artifacts, multi-toolchain orchestration) is speculative — AGENTS.md's simplicity principle says "no abstractions for single-use code" and "no flexibility that wasn't requested." Since you're the maintainer this is your architectural call, but worth noting the tension. Size: 1022 production logic lines (build-test.ts: 496, npm-toolchain.ts: 498, toolchain.ts: 28), 179 test lines, 233 docs. Above the 1000-line advisory — though the bulk is a verbatim move of existing code, not new logic. Approach: the scope feels right for an extraction. One question: could this have been just "move npm logic to Risk: no elevated risk signals (no high-risk paths matched). Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：这是一个结构性重构—— 方向：抽取本身合理——文件较长且 npm 算法是自包含的。adapter 接口（28 行）很精简。不过设计文档的"未来阶段"（Maven、Gradle、覆盖率产物、多 toolchain 编排）是推测性的——AGENTS.md 的简洁原则说"不为单次使用的代码创建抽象"、"不添加未被要求的灵活性"。作为维护者这是你的架构决策，但值得注意这个张力。 规模：1022 行生产逻辑（build-test.ts: 496, npm-toolchain.ts: 498, toolchain.ts: 28），179 行测试，233 行文档。超过 1000 行建议阈值——但大部分是现有代码的逐字移动，不是新逻辑。 方案：对于抽取来说范围合理。一个问题：是否可以只做"将 npm 逻辑移到 风险：无升级风险信号（未匹配高风险路径）。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal: given "build-test.ts is too long and combines multiple concerns," I would extract the npm verification algorithm into its own module as a plain function export — Comparison with the diff: the PR does exactly that extraction, then wraps it in a 28-line adapter interface ( No correctness bugs, security holes, or regressions found. The behavioral contract is preserved:
The only structural concern (already raised in Stage 1) is the adapter interface for a single implementation. It's 28 lines and doesn't add runtime complexity — No findings that block the merge. TestingThis is an internal refactor with no user-visible behavioral changes — real-scenario testing: N/A. CI evidence from the PR's own checks on Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The main test suite (ubuntu) is still running. macOS/Windows/Integration are skipped (typical for fork PRs). No failures so far. The author reports 42 focused tests passing locally (2 test files: 中文说明代码审查独立方案： 给定"build-test.ts 太长且混合了多个关注点"，我会将 npm 验证算法提取到自己的模块中作为纯函数导出—— 与 diff 的比较： PR 做了完全相同的提取，然后用 28 行 adapter 接口（ 未发现正确性 bug、安全漏洞或回归。行为契约保持不变。唯一的结构性顾虑（已在 Stage 1 提出）是为单一实现添加 adapter 接口。这是 28 行且不增加运行时复杂度。不是阻塞项。 测试内部重构，无用户可见行为变化——真实场景测试：N/A。 CI 证据：主测试套件（ubuntu）仍在运行中。macOS/Windows/集成测试被跳过（fork PR 典型情况）。目前无失败。作者报告本地 42 个聚焦测试通过；这是作者的声明，未在此独立重新运行。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 3/5 — clean review, but the fork-refactor guardrail needs a maintainer's sign-off. Stepping back: this is a well-executed mechanical extraction. The npm algorithm moves verbatim, comments and all, into its own module. The public API is preserved through re-exports. Tests are strengthened from partial shape checks to exact report assertions. No behavioral drift, no correctness issues, no regressions found. The one genuine question is whether the adapter interface earns its place. My independent proposal would have been a plain function export — no interface, no selection function. The PR adds 28 lines of abstraction for one implementation, justified by a design doc describing Maven/Gradle adapters that don't exist yet. AGENTS.md's simplicity principle ("no abstractions for single-use code") points the other way. But the abstraction is tiny, doesn't add runtime complexity, and the author is the maintainer making the architectural call. Not a blocker — a genuine disagreement about taste. The design doc is well-written and carefully scoped (explicit non-goals, "P0 does not…"), but its existence for a single-adapter extraction is the strongest signal that this is building for a hypothetical future rather than solving a present problem. If Maven/Gradle never materialize, the interface is dead weight; if they do, it's a reasonable foundation. That's a bet, not a defect. ⏸️ Deferring to @wenshao — the fork-refactor approval guardrail prevents auto-approval here (cross-repository PR with a 中文说明置信度：3/5 — review 干净，但 fork-refactor 保护规则需要维护者签字。 退一步看：这是一次执行良好的机械性提取。npm 算法逐字移动（包括所有注释）到独立模块。公共 API 通过 re-export 保持。测试从部分形状检查加强为精确 report 断言。未发现行为漂移、正确性问题或回归。 唯一真正的问题是 adapter 接口是否值得存在。我的独立方案是纯函数导出——不要接口、不要选择函数。PR 为一个实现增加了 28 行抽象，由一份描述尚不存在的 Maven/Gradle adapter 的设计文档来证明。AGENTS.md 的简洁原则（"不为单次使用的代码创建抽象"）指向另一个方向。但抽象很小，不增加运行时复杂度，且作者是维护者在做架构决策。不是阻塞项——是关于品味的真诚分歧。 设计文档写得很好且范围谨慎（明确的非目标、"P0 不做……"），但为单一 adapter 提取写设计文档本身是最强的信号：这是为假设性的未来构建，而非解决当前问题。如果 Maven/Gradle 永远不出现，接口就是死重；如果出现，它是合理的基础。这是一个赌注，不是缺陷。 ⏸️ 转交 @wenshao — fork-refactor 审批保护规则阻止了此处的自动批准（跨仓库 PR 且标题为 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. This is a fork PR, so the first round comes from the next scheduled scan (usually within minutes). Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。本 PR 来自 fork，首轮处理将由下一次定时扫描执行（通常几分钟内）。移除 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Test Plan (not a blocker): 42 tests to pass — this review observed 16965 passed.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 Test Plan（非阻断）：42 tests to pass — this review observed 16965 passed。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
changed the title
Three test-efficacy gaps the review found, each a surviving mutant: - runBuildTest's delegation to the selected adapter's run() was asserted only by report shape, so reverting the facade to the old inline implementation kept every test green. A spy on the adapter boundary now reddens on a revert and pins the forwarded arguments. - The `?? run` exec fallback had zero coverage because every test injects a fake exec. A test that injects nothing now pins that the adapter receives a callable default. - The single-root guard's negative side was untested: existing workspace fixtures use script-less roots, so forcing the guard on survived. A workspace repo whose root has build/test scripts now pins that the diff maps to the workspace, not the root package. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…m/wenshao/qwen-code into refactor/review-toolchain-adapter
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #8394Addressed all three inline suggestions from the automated reviewer. Each was a test-efficacy finding: a mutant that survived because the refactor's own tests asserted the report shape but not the behavior the refactor introduced. All three are early-round (round 1) suggestions, are valuable, are consistent with the suite's existing exec-seam / tmpdir conventions, and are in scope — so each is now pinned by one focused new test, and each new test was proven load-bearing by applying the exact mutant the reviewer named and confirming it turns red. Feedback points[rc:3699994608] build-test.test.ts — the refactor is inert to its own test file
[rc:3699994612] build-test.ts — the
|
…wenLM#8394) The toolchain-adapter refactor rewrote Agent 7's brief to be npm/Maven agnostic, replacing the literal "npm run build" wording with a pointer to `qwen review build-test`. Update the stale assertion that still pinned the old string, which broke the Test CI job.
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #8394Feedback triagedThe only actionable item this round was a failed required check:
There were no review bodies, inline comments, or issue-level comments to address. Root causeThe CI This PR's toolchain-adapter refactor deliberately rewrote Agent 7's brief in This is a PR-caused breakage, not a pre-existing one: on Decision & changeRequired (broken test) — fixed minimally. Updated the stale assertion to pin the brief's new, toolchain-agnostic command string, which preserves the test's stated intent ("gives Agent 7 no diff — its evidence is the commands it ran"): - expect(p).toContain('npm run build');
+ expect(p).toContain('qwen review build-test');
Conflict notesNone — VerificationCommands actually run this round (all from the repository root unless noted):
Committed as 中文说明Autofix 审查轮次 — PR #8394分类的反馈本轮唯一可处理的事项是一个失败的必需检查：
没有需要处理的审查正文、行内评论或 issue 级评论。 根因CI 的 本 PR 的工具链适配器重构刻意将 这是由本 PR 造成的破坏，而非既有问题：在 决定与改动必需项（测试损坏）——以最小改动修复。 将该过时的断言更新为固定简报中新的、与工具链无关的命令字符串，从而保留该测试所声明的意图（“给 Agent 7 不提供 diff —— 它的证据是它运行过的命令”）： - expect(p).toContain('npm run build');
+ expect(p).toContain('qwen review build-test');
冲突说明无 —— 验证本轮实际运行的命令（除非另有说明，均在仓库根目录执行）：
已在 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. 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。 Test Plan（非阻断）：./mvnw — no such file or directory。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…er' into refactor/review-toolchain-adapter # Conflicts: # packages/cli/src/commands/review/agent-prompt.test.ts # packages/cli/src/commands/review/build-test.test.ts
|
🤖 AutoFix updated a stale base — the fix did not pass verification, but this PR was behind What I found before stopping: Autofix round summary — PR #8394 (address-review)Outcome: no new code change was required this round. All nine round-2 suggestions from the automated reviewer were verified and are already resolved in the current head (the fixes landed in the earlier commits Automated reviewer suggestions (round 2) — all resolved in code
Run log: https://github.com/QwenLM/qwen-code/actions/runs/30775286030 🧠 Handled by Qwen Code · model/模型 |
Review:
|
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…M#8394) Address the review findings on the Maven toolchain adapter: - Roll clean Surefire/Failsafe reports up per project dir and cap the failing-report and failing-case evidence lines, so a full-reactor run can no longer bypass the bounded-output contract by appending thousands of lines after the trim. - Narrow module builds to the upstream closure (-pl ... -am, dropping -amd): downstream expansion selects the whole reactor on exactly the repos this adapter was built for, and a run that spends its deadline timing out proves nothing. Downstream coverage stays the project's CI matrix, as with the npm adapter's scope. - Select the wrapper a platform can execute (mvnw.cmd on win32) instead of steering a launch failure at the PR. - Classify dependency and network failures only when Maven itself frames them ([ERROR]/[FATAL]), so a test failing with Connection refused in its stdout stays a source finding; launch-level facts stay free-text. - Route repository metadata (VCS/CI config, licenses, editor rules) to unowned instead of a reactor-wide run; unrecognized root files stay fail-closed. - base-tree no longer pays for a Maven base build nothing can consume: test-delta reruns npm commands only this release, so a Maven merge base returns unavailable without building. - Polish: reuse shellQuotePath in the module selector, note the npm-only install scope, treat a spawn-level death without an exit code as infrastructure, record the mtime-granularity tradeoff, disclose non-default report directories, and say when a bare Maven claim was settled by a module-scoped run. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Autofix round summary — PR #8394This round addresses the issue-level review findings on the Maven toolchain Fixed in this commit (issue-level review findings)
Polish items from the same review
Inline findings (automated reviewer round 2 + prior rounds)All nine round-2 suggestions were already fixed in code by earlier commits
The twelve "Addressed" confirmations were each checked against the code; Not actionable here
ConflictNo base conflict ( Verification
中文说明Autofix 本轮总结 — PR #8394本轮处理针对 Maven 工具链 adapter 的 issue 级评审发现，并复核此前提交已修复的行内发现。 本次提交修复的问题（issue 级评审发现）
同一评审中的打磨项
行内发现（自动评审第 2 轮 + 此前各轮）第 2 轮的全部九条建议此前已由本分支的提交修复；本轮逐条对照 HEAD 复核：
十二条 "Addressed" 确认均已逐条对照代码核实，所述修复全部存在于 HEAD，其线程已列入待解决清单。 本轮不处理
冲突无基线冲突（ 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
Review:
|
| construct | result |
|---|---|
<x><![CDATA[a --> b]]></x> |
❌ error |
<x y="a > b"/> (> in an attribute value) |
❌ error |
CDATA with a bare >, comments containing <module>, DOCTYPE, namespaces |
✅ parses |
The first is the cheap one: moduleEntries strips comments and then rejects the file if any <!--/--> survives, so a --> inside CDATA (antrun/checkstyle/xml-generation config) trips it. Stripping <![CDATA[…]]> before that guard fixes it without weakening the guard. Blast radius is the same as #1 — one unparseable POM anywhere in the reactor and the whole repo drops to unsupported, and the brief (correctly) tells the agent not to substitute an ad hoc Maven command.
3. Agent 7's brief overclaims Maven scoping — -amd was dropped, the prompt was not updated — Medium
lib/agent-briefs.ts:448 still says build-test "scopes the run to the changed workspaces/modules plus their required dependency and dependent closure". The Maven command emits -am only — confirmed:
mvn --batch-mode --no-transfer-progress -pl core -am test
docs/design/review-toolchain-adapters.md:207 was updated correctly (-amd selects the whole reactor, so downstream coverage stays with CI). The brief and the PR body's Test Plan step 2 (-pl core -am -amd test) were not. Agent 7 reads that brief as fact, so it can report dependent modules as verified when they were never built.
Related and worth a line in the report note: a change to core/pom.xml is scoped to -pl core -am, and a POM change is precisely the kind that can break dependents. The tradeoff is defensible; it should be stated in the evidence the agent reports, not only in a source comment.
4. toolchainAdapters is documented as precedence-ordered, but selection ignores order — Low-Medium
/** The root toolchains build-test can select, in precedence order. */
export const toolchainAdapters = [npmToolchainAdapter, mavenToolchainAdapter];selectToolchainAdapter returns a value only when exactly one adapter applies; order is never consulted. Drop the phrase, or make selection use it.
The related risk is npmToolchainAdapter.applies = existsSync(package.json). That fires on a root package.json that exists only for a docs site, husky, or a lint config — a shape many Java repos have — and the repo now returns unsupported where the npm path previously at least ran. The body flags mixed roots as an intentional breaking change, but the trigger is broader than "both toolchains genuinely apply". Consider requiring workspaces or a build/test script in applies, which is the condition runNpmToolchain already checks internally.
5. base-tree gates Maven after git worktree add — Low
The early return sits below the worktree add, so a full base checkout of a large Java reactor is materialized and then never built (path: tree is still returned). git cat-file -e <baseSha>:pom.xml would answer the same question before paying for the checkout.
6. Maven infra-vs-source classification reads already-trimmed output — Low
isInfrastructureFailure(result.output) runs on the output the real run passed through trimOutput (head 2k / tail 6k). trimOutput rescues Cannot find module and runner-summary lines from the omitted middle; it has no rescue for [ERROR] Could not resolve dependencies and friends. Maven normally prints its error summary last, so the tail usually holds it — but when it doesn't, the failure mode is the exact one this command exists to prevent: a dependency/network outage filed as a Critical against the PR. Adding the isDependencyFailure markers to trimOutput's rescue predicate closes it cheaply.
7. The wrapper-permission guard uses a raw path comparison — Low
executable === './mvnw' && !args.changedFiles.includes('mvnw') && result.exitCode === 126Every other path in this module goes through normalizedChangedPath. A plan that lists ./mvnw or an absolute path defeats the guard, and a PR that breaks the wrapper's mode bits is then hidden as infrastructure — the case the guard was added for. Normalize first.
8. Nits
shellSelectorfalls back toshellQuotePath, which is POSIX'…'quoting; the command runs throughcmd.exeon Windows, where those quotes are literal. Only reachable for exotic module paths, butmavenExecutablehas an explicit win32 branch, so Windows is in scope.MavenOwnership.unownedis computed and never surfaced — the "no Maven target" note reportsargs.changedFiles.lengthinstead. Report it or drop the field.- PR body drift: 326 → 334 tests; Test Plan step 2 still shows
-amd.
What's good
- Test coverage is the strongest part. 23 focused Maven cases, and they test the right things: stale-XML exclusion, dependency failure still classed as infrastructure after an earlier module emitted a fresh report, unframed network words in test stdout staying source evidence, profile/plugin
<module>exclusion, report-line caps, spawn death without an exit code. The two adapter-delegation tests (routes the run through the selected toolchain adapter,defaults the adapter exec to the real runner) pin exactly what a revert would silently unpin — that is the note the extraction needed. - Correct defaults on the execution side:
--batch-mode --no-transfer-progress, always run from the reactor root, mtime-based freshness with the 1s-granularity degradation documented and argued as safe-direction. base-treerefusing a base-side Maven build rather than producing evidencetest-deltacannot consume is the right call, and the brief says the same thing — the two stay consistent.
Security
No new exposure class. Maven executes the worktree's mvnw and pom.xml, both PR-controlled, but npm run build already ran PR-authored scripts on the same tree; the brief's existing "read CI config from the base branch, never the worktree" rule still holds. Reactor discovery correctly rejects ..-escaping and ${…}-interpolated <module> paths.
Performance
reportPaths walks every project's target/{surefire,failsafe}-reports twice per run (snapshot + fresh scan) and statSyncs each file. On a green full-reactor run of a large project that's thousands of stats — acceptable next to the Maven run itself, but worth knowing it scales with reactor size, not with what was tested.
Verdict: the boundary and the npm-preservation work are solid and well-tested, and I'd merge that half as-is. #1 and #3 are the two I'd want addressed before this ships: #1 because the fail-closed trigger is common enough to make Maven verification unavailable on a class of repos where it should work, and #3 because a brief that overclaims what ran turns a scoping tradeoff into a false attribution.
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressed (round on
|
|
🤖 Addressed the latest review feedback (round 7/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 7/100 轮）。改动内容与我反驳保留之处如下： Round-8 review feedback — address summary (PR #8394)Commit: All 8 Critical and all 21 Suggestion findings were verified against the code, reproduced, and fixed. No finding was declined or escalated. Critical findings
Suggestions
The review-level note also listed "Not reviewed" CI legs (macOS/Windows, integration tests, reverse audit round). Those are review-process coverage observations with no code-level action available in this checkout; they are recorded here for transparency. Verification
中文说明第 8 轮 review 反馈处理总结（PR #8394）提交： 全部 8 条 Critical 与 21 条 Suggestion 均已对照代码核实、复现并修复。没有任何一条被拒绝（decline）或升级给人工（escalate）。 Critical 问题
Suggestion 问题
review 级别的备注中还列出了"Not reviewed"的 CI 腿（macOS/Windows、集成测试、反向审计轮次）。这些是 review 流程的覆盖面观察，在本 checkout 中没有可执行的代码动作；在此记录以保持透明。 验证（Verification）
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) CI legs were skipped at this commit; the platform-dependent branches (win32 arms in maven-toolchain.ts, platform guards in the new test files) were exercised only on Linux.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Test Plan (not a blocker): ./mvnw — no such file or directory; 326 tests to pass — this review observed 18298 passed.
中文说明
未审查：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) CI legs were skipped at this commit; the platform-dependent branches (win32 arms in maven-toolchain.ts, platform guards in the new test files) were exercised only on Linux。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
Test Plan（非阻断）：./mvnw — no such file or directory; 326 tests to pass — this review observed 18298 passed。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| const freshTestFailures = (c: CommandResult): boolean => | ||
| /^\[maven-test-failure\] /m.test(c.output ?? ''); |
There was a problem hiding this comment.
[Critical] [maven-test-failure] marker mining in ruleCommand is not gated on the command being a Maven run — unlike the sibling [maven-test-report] count mining in observedTestCounts, which this same diff explicitly runner-gates (MAVEN_RUNNER_RE.test(cmd.command), with the comment "the same text mined from a non-Maven command's stdout is a fabricated count"). — Failure scenario: in an npm repo, a GREEN (exitCode: 0) npm test run whose stdout merely contains a line beginning with [maven-test-failure] (any test can console.log it; this repo's own maven-toolchain/test-plan suites print those literals) makes ranFailed true; the run is selected via finished(c) && ranFailed(c), and the author's `npm test` Test Plan claim is ruled contradicted with observed "exit 0, but fresh Surefire/Failsafe reports record failures". Before this diff, a green exit-0 match ruled reproduces. Probe-verified at the reviewed commit: gating flips the verdict back to reproduces with all 130 existing tests still green. The marker is only ever emitted by the Maven adapter, and a report is single-toolchain, so in an npm result it can never be legitimate evidence.
| const freshTestFailures = (c: CommandResult): boolean => | |
| /^\[maven-test-failure\] /m.test(c.output ?? ''); | |
| const freshTestFailures = (c: CommandResult): boolean => | |
| MAVEN_RUNNER_RE.test(c.command) && | |
| /^\[maven-test-failure\] /m.test(c.output ?? ''); |
中文说明
ruleCommand 中对 [maven-test-failure] 标记的挖掘没有按 Maven 命令进行门控——而同一 diff 里 observedTestCounts 中对 [maven-test-report] 计数的挖掘却明确做了 runner 门控。失败场景：在 npm 仓库中，一次绿色（exitCode 0）的 npm test 运行，只要其 stdout 中出现以 [maven-test-failure] 开头的行（任何测试都可以打印它；本仓库自己的 maven-toolchain/test-plan 测试套件就包含这些字面量），ranFailed 即为 true，该运行会被选中，作者的 `npm test` Test Plan 声明会被判为 contradicted（observed 为 "exit 0, but fresh Surefire/Failsafe reports record failures"）。此 diff 之前，绿色的 exit-0 匹配判为 reproduces。已在被审提交上探针验证：加上门控后裁决翻转为 reproduces，且现有 130 个测试全部仍通过。该标记只会由 Maven adapter 发出，且一次报告只会是单一工具链，所以在 npm 结果中它绝不可能是合法证据。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| let namedStructure: PomStructure | null = null; | ||
| if (basename(parentPom) === 'pom.xml') { | ||
| targetArtifactId = structures.get(parentPath)?.artifactId ?? null; |
There was a problem hiding this comment.
[Critical] Inheritance chains that pass through a pom.xml in a NON-aggregated directory are silently dropped: only named (non-pom.xml) parent files get worklist chain continuations, so the edge dies here at structures.get(parentPath)?.artifactId for a pom.xml outside the visited reactor projects. — Failure scenario (probe-verified): root aggregates app and parent-bom; app/pom.xml declares <parent> → ../shared/parent/pom.xml (aggregated nowhere); shared/parent/pom.xml declares its own <parent> → ../../parent-bom/pom.xml. readMavenReactor returns inheritors: {}; a diff changing parent-bom/pom.xml yields modules: ['parent-bom'] and runs -pl parent-bom -am test — app, which inherits the changed parent through the chain, is never compiled or tested, and the run reports green. The twin shape (changing the intermediate file itself) correctly fails closed via inactiveProjects; this residual is the pom.xml-spelled intermediate. Residual of the round-8 R8-11 fix family.
Suggested fix: when basename(parentPom) === 'pom.xml' but parentPath is not in structures and the file exists, parse it like the named-file branch (record it and continue its own <parent> chain for the same heir), or fail the reactor closed with a reportable error.
中文说明
经过“非聚合目录中的 pom.xml”的继承链会被静默丢弃：只有非 pom.xml 命名的父文件才会获得工作列表的链式延续，因此对于已访问 reactor 项目之外的 pom.xml，继承边在这里死于 structures.get(parentPath)?.artifactId。失败场景（已探针验证）：根聚合 app 与 parent-bom；app/pom.xml 声明 <parent> → ../shared/parent/pom.xml（该目录未被任何聚合包含）；shared/parent/pom.xml 又声明自己的 <parent> → ../../parent-bom/pom.xml。readMavenReactor 返回 inheritors: {}；修改 parent-bom/pom.xml 的 diff 得到 modules: ['parent-bom'] 并运行 -pl parent-bom -am test —— 通过该链继承了被修改父 POM 的 app 从未被编译或测试，而运行报告为绿色。孪生形态（直接修改中间文件本身）会经由 inactiveProjects 正确 fail closed；此残留是拼写为 pom.xml 的中间文件形态。属于第 8 轮 R8-11 修复家族的残留。建议修复：当 basename(parentPom) === 'pom.xml' 但 parentPath 不在 structures 中且文件存在时，按命名文件分支处理（记录它并为同一继承者继续其自身的 <parent> 链），或以可报告的错误让 reactor fail closed。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| const path = normalizedChangedPath(root, changedFile); | ||
| if (path === null) continue; | ||
| if (path === otherPlatformWrapper) continue; |
There was a problem hiding this comment.
[Critical] The other-platform-wrapper skip runs before the named-parent lookup and the REACTOR_WIDE_FILES routing, so a recorded parent-config file spelled mvnw.cmd (POSIX) or mvnw (win32) is ignored entirely — no closure, no reactor-wide escalation, no target. — Failure scenario (probe-verified): heir app declares <parent> with relativePath ../mvnw.cmd (a root-level named parent POM literally named mvnw.cmd); readMavenReactor records the edge (inheritors: {'.': ['app']}, parentPomFiles: ['mvnw.cmd']). A diff changing mvnw.cmd on POSIX hits this skip; ownership returns empty modules, and runMavenToolchain reports "There is no Maven target to run — this is a complete answer" with zero commands — a green no-op for a parent-config change app inherits. The skip exists so a change confined to the other platform's wrapper doesn't run the reactor, but it must not swallow a file that is simultaneously a recorded build input.
| const path = normalizedChangedPath(root, changedFile); | |
| if (path === null) continue; | |
| if (path === otherPlatformWrapper) continue; | |
| const path = normalizedChangedPath(root, changedFile); | |
| if (path === null) continue; | |
| if (path === otherPlatformWrapper && !namedParentDirs.has(path)) continue; |
中文说明
“另一平台 wrapper”的跳过逻辑先于命名父文件查找与 REACTOR_WIDE_FILES 路由执行，因此一个被记录为父配置文件、但拼写恰好是 mvnw.cmd（POSIX）或 mvnw（win32）的文件会被完全忽略——没有闭包、没有 reactor-wide 升级、没有任何目标。失败场景（已探针验证）：继承者 app 声明 <parent> 且 relativePath 为 ../mvnw.cmd（根目录下一个字面命名为 mvnw.cmd 的命名父 POM）；readMavenReactor 记录了该边。在 POSIX 上修改 mvnw.cmd 的 diff 命中此跳过；ownership 返回空模块集，runMavenToolchain 报告 "没有需要运行的 Maven 目标——这是一个完整答案"，零命令——对一个 app 所继承的父配置变更给出了绿色的无操作。该跳过的存在是为了让仅修改另一平台 wrapper 的变更不触发 reactor 运行，但它不应吞掉同时又是已记录构建输入的文件。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| const src = projectDir === '.' ? 'src' : `${projectDir}/src`; | ||
| return rel === src || rel.startsWith(`${src}/`); |
There was a problem hiding this comment.
[Critical] isUnderTestSourceTree's rel === src equality clause classifies a real Maven project located exactly at a project's src path as test-data fixture, defeating the out-of-reactor fail-closed rule (ownership rule 5) for that shape. — Failure scenario (probe-verified with flip): a standalone, non-aggregated project at <root>/src/pom.xml; the PR changes src/main/java/App.java inside it. nearestMavenProject's upward walk finds src/pom.xml, but isUnderTestSourceTree('src') returns true via the equality half, so the walk returns .; no inactiveProjects entry is added, the adapter runs mvn -pl . -am test and reports "Maven tested . successfully … this run covered the changed modules" — the changed project is never compiled. Removing the equality half flips to unsupported / "outside the root reactor: src" with zero commands; all 124 existing tests still pass without the clause, and real fixture shapes live strictly below src/.
| const src = projectDir === '.' ? 'src' : `${projectDir}/src`; | |
| return rel === src || rel.startsWith(`${src}/`); | |
| const src = projectDir === '.' ? 'src' : `${projectDir}/src`; | |
| return rel.startsWith(`${src}/`); |
中文说明
isUnderTestSourceTree 的 rel === src 相等分支会把恰好位于某项目 src 路径上的真实 Maven 项目判定为测试数据 fixture，从而使针对该形态的 reactor 外 fail-closed 规则（ownership 规则 5）失效。失败场景（已探针验证并翻转）：一个独立、未被聚合的项目位于 <root>/src/pom.xml；PR 修改其中的 src/main/java/App.java。nearestMavenProject 向上查找发现 src/pom.xml，但 isUnderTestSourceTree('src') 经相等分支返回 true，于是查找返回 .；不会添加任何 inactiveProjects 条目，adapter 运行 mvn -pl . -am test 并报告 "Maven tested . successfully … this run covered the changed modules" —— 被修改的项目从未被编译。去掉相等分支后翻转为 unsupported/"outside the root reactor: src" 且零命令；没有该分支时现有 124 个测试全部通过，且真实 fixture 形态都严格位于 src/ 之下。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| const callBudgetMs = | ||
| (args.budget ?? Math.max(args.timeout, args.timeout * 2 - 30)) * 1000; |
There was a problem hiding this comment.
[Critical] Deadline values are never coerced to positive-integer milliseconds before spawnSync: fractional --timeout/--budget values throw ERR_OUT_OF_RANGE (the command dies with a raw message, no report, no --out file), and --timeout 0 silently arms no kill timer. The npm arm is verbatim-moved pre-existing code, but the Maven arm is new in this PR, and its new attemptFloorMs makes --timeout 0 spawnable (the old fixed 15 s floor blocked every attempt). — Failure scenario (measured on node 22): 32.2 * 1000 = 32200.000000000004; spawnSync with timeout: 15000.1 throws "The value of "timeout" is out of range. It must be an unsigned integer"; end-to-end through runBuildTest, both npm (budget: 32.2) and Maven fixtures crash, exit 1, no JSON report. --timeout 0: spawnSync {timeout: 0} arms no kill timer (a 2 s sleep ran to completion, measured) — the Maven lifecycle then runs deadline-less, and a cold-reactor warm-up can hang forever.
Suggested fix: sanitize once, e.g. const perCommandMs = Math.max(1, Math.round(args.timeout * 1000)); and round callBudgetMs the same way (and/or validate integer seconds in the yargs builders of build-test/base-tree/test-delta).
中文说明
截止期数值在传给 spawnSync 之前从未被强制转换为正整数毫秒：小数 --timeout/--budget 会抛出 ERR_OUT_OF_RANGE（命令以裸消息退出，没有报告、没有 --out 文件），而 --timeout 0 会静默地不设置任何终止定时器。npm 分支是逐字搬移的既有代码，但 Maven 分支是本 PR 新增的，且其新的 attemptFloorMs 使 --timeout 0 可以被实际执行（旧的固定 15 秒下限会挡住所有尝试）。失败场景（在 node 22 上实测）：32.2 * 1000 = 32200.000000000004；timeout: 15000.1 的 spawnSync 抛出越界错误；端到端经过 runBuildTest，npm（budget: 32.2）与 Maven fixture 都会崩溃、exit 1、无 JSON 报告。--timeout 0：spawnSync {timeout: 0} 不设置终止定时器（实测 2 秒 sleep 完整跑完）—— Maven 生命周期将在无截止期状态下运行，冷 reactor 的预热可能永久挂起。建议修复：统一取整 sanitise（例如 Math.max(1, Math.round(args.timeout * 1000))），并/或在 yargs builder 中校验整数秒。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| export const mavenToolchainAdapter: ReviewToolchainAdapter = { | ||
| applies: (root) => existsSync(join(root, 'pom.xml')), | ||
| run: runMavenToolchain, | ||
| }; |
There was a problem hiding this comment.
[Suggestion] Maven's applies() does not mirror its run-side gate, so at a mixed npm+Maven root a Maven run that would immediately concede unsupported still blocks the npm adapter — the exact defect class the npm adapter's applies() comment says this PR fixed. This is the round-6 finding R6-22, deferred to a follow-up round; this is that round. — Failure scenario (probe-verified at this commit): mixed root (modeled packages/* workspaces with a member package + root pom.xml containing <module>${x}</module>): runBuildTest returns the generic "Both npm and Maven apply … ran nothing" note with 0 exec calls — although mavenToolchainAdapter.run alone concedes unsupported before executing any command (!parsed.reactor → unsupportedReport), and npmToolchainAdapter.run alone returns toolchain: 'npm' (fully applicable). The review loses its one deterministic build/test verdict, and the ambiguity note hides Maven's concession reason. Safe direction (no false verdict) — Suggestion.
Suggested fix: tighten the gate to mirror the run side, e.g. applies: (root) => readMavenReactor(root).reactor !== null (the npm gate already pays for a full tree walk in applies()); or in the ambiguity branch, discount an applicable adapter whose run concedes unsupported without executing.
中文说明
Maven 的 applies() 没有镜像其运行侧门控，因此在 npm+Maven 混合根目录下，一个本会立即让步为 unsupported 的 Maven 运行仍会挡住 npm adapter——这正是 npm adapter 的 applies() 注释声称本 PR 已修复的缺陷类别。这就是第 6 轮的发现 R6-22，当时被推迟到后续轮次处理；本轮即是该后续轮次。失败场景（已在当前提交探针验证）：混合根（建模的 packages/* workspaces 且有成员包 + 含 <module>${x}</module> 的根 pom.xml）：runBuildTest 返回通用的 "Both npm and Maven apply … ran nothing" 注记且 0 次 exec 调用——尽管 mavenToolchainAdapter.run 单独运行时会在执行任何命令前让步为 unsupported，而 npmToolchainAdapter.run 单独运行会返回 toolchain: 'npm'（完全适用）。评审失去唯一的确定性构建/测试裁决，且歧义注记隐藏了 Maven 的让步原因。方向安全（不会产生错误裁决）——Suggestion。建议修复：收紧门控以镜像运行侧（例如 applies: (root) => readMavenReactor(root).reactor !== null；npm 门控本就已在 applies() 中付出整树遍历的代价），或在歧义分支中对 "运行会不执行任何命令即让步为 unsupported" 的适用 adapter 打折。
— qwen3.8-max via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Deferred (round cap). Maven's applies() not mirroring its run-side gate (R6-22, probe-verified again at this commit) is acknowledged and still valid. This PR is in review round 9; per AGENTS.md, rounds past ~5 land only Critical fixes. Tightening the gate to readMavenReactor(root).reactor !== null (or discounting an unsupported-conceding adapter in the ambiguity branch) will land in a follow-up; the deferral is recorded here so nothing is silently dropped.
中文说明
推迟（轮次上限）。 Maven 的 applies() 未镜像其运行侧门控（R6-22，在本提交再次探针验证）被认可且仍然成立。本 PR 已进入第 9 轮评审；按 AGENTS.md，超过约 5 轮后只落地 Critical 修复。将门控收紧为 readMavenReactor(root).reactor !== null（或在歧义分支中对会让步为 unsupported 的 adapter 打折）将在后续落地；此回复记录推迟以免被静默丢弃。
| P1 intentionally does not implement Maven dependency resolution in TypeScript. | ||
| It reads only the aggregation structure needed to map paths to module directories: |
There was a problem hiding this comment.
[Suggestion] The "Reactor and module ownership" section describes an aggregation-only discovery algorithm ("reads only the aggregation structure", and below "Parent inheritance … remain Maven's job through -am"), but the shipped implementation reads and walks <parent> inheritance edges — contradicting both the code and this same doc's Testing section ("aggregation edges AND <parent> inheritance edges are read from checked-in POMs"). — Concrete cost: this doc is the committed design-to-implementation record per AGENTS.md, and prior rounds already fixed four instances of this same drift class. A maintainer or doc-driven cleanup reconciling code against this section sees the inheritance machinery (inheritors, addDescendantClosure walking children AND inheritors, ~115 lines) declared out-of-spec; removing it per the spec regresses a changed parent POM to -pl <aggregator> only, so inheriting modules are never compiled or tested and the run reports green — the exact false-green class the Criticals on maven-toolchain.ts hunt. The -am attribution is itself inaccurate (-am is upstream-only; the adapter does the inheritor fan-out itself).
Suggested fix: rewrite the section to match the implementation and the Testing section — reads aggregation structure AND <parent> inheritance edges; add a step resolving each project's <parent> <relativePath> (including named parent POM files) and closing over both edge classes; narrow the closing paragraph to effective-POM semantics (dependency resolution/management, reactor ordering) only.
中文说明
"Reactor and module ownership" 一节描述的是仅聚合的发现算法（"只读取聚合结构"，以及下文 "父继承……仍由 Maven 通过 -am 负责"），但实际实现读取并遍历 <parent> 继承边——与代码及同一文档 Testing 一节（"聚合边与 <parent> 继承边都从提交的 POM 中读取"）相矛盾。具体代价：按 AGENTS.md，本文档是提交在案的设计-实现记录，且此前各轮已修复过四个同类漂移实例。维护者或文档驱动的清理工作在按本节对齐代码时，会看到继承机制（inheritors、同时遍历 children 与 inheritors 的 addDescendantClosure，约 115 行）被声明为规范之外；按规范删除它会把被修改父 POM 的构建范围退化为仅 -pl <aggregator>，继承它的模块从不被编译或测试而运行仍报告绿色——正是 maven-toolchain.ts 上那些 Critical 所追猎的假绿类别。-am 的归因本身也不准确（-am 只拉上游；继承者扇出是 adapter 自己做的）。建议修复：改写该节以与实现和 Testing 节一致——读取聚合结构与 <parent> 继承边；增加一步解析每个项目的 <parent> <relativePath>（包括命名父 POM 文件）并对两类边做闭包；把结尾段落收窄到仅 effective-POM 语义（依赖解析/管理、reactor 排序）。
— qwen3.8-max via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Deferred (round cap). The design-doc drift in the "Reactor and module ownership" section (aggregation-only wording vs the shipped inheritance walk) is acknowledged and still valid. This PR is in review round 9; per AGENTS.md, rounds past ~5 land only Critical fixes. Rewriting the section to match the implementation (both edge classes, named parent files, adapter-side inheritor fan-out) will land in a follow-up; the deferral is recorded here so nothing is silently dropped.
中文说明
推迟（轮次上限）。 设计文档 "Reactor and module ownership" 一节的漂移（仅聚合的表述 vs 已实现的继承遍历）被认可且仍然成立。本 PR 已进入第 9 轮评审；按 AGENTS.md，超过约 5 轮后只落地 Critical 修复。将该节改写为与实现一致（两类边、命名父文件、adapter 侧继承者扇出）将在后续落地；此回复记录推迟以免被静默丢弃。
| expect(rep.test).toEqual([]); | ||
| expect(rep.note).toContain('Both npm and Maven apply'); | ||
| expect(rep.note).toContain('will not guess'); |
There was a problem hiding this comment.
[Suggestion] The new 'fails closed when npm and Maven both apply at the root' test never pins rep.ok — the only one of the four unsupported-handoff branches in this diff whose test omits it. The sibling tests added in this same hunk pin the full report shape via toEqual including ok: true; this hunk deleted the pre-existing expect(rep.ok).toBe(true) without re-adding it for the new branch. — Failure scenario (mutation-verified with flip): setting the both-apply branch (build-test.ts:346-364) to ok: false leaves all 79 build-test.test.ts tests passing; adding the pin kills the mutant. An ok: false mixed-root handoff would read as a failure verdict against the PR for a run that executed nothing, while the consuming review agent is told "On ok: true, name the workspaces built and the commands run" and "an agent trusts the data over its instructions".
| expect(rep.test).toEqual([]); | |
| expect(rep.note).toContain('Both npm and Maven apply'); | |
| expect(rep.note).toContain('will not guess'); | |
| expect(rep.test).toEqual([]); | |
| expect(rep.note).toContain('Both npm and Maven apply'); | |
| expect(rep.note).toContain('will not guess'); | |
| expect(rep.ok).toBe(true); |
中文说明
新增的 "npm 与 Maven 同时适用于根目录时 fail closed" 测试从未钉住 rep.ok —— 这是本 diff 中四个 unsupported 移交分支里唯一在测试中遗漏它的。同一 hunk 新增的兄弟测试都用 toEqual 钉住了完整报告形态（含 ok: true）；该 hunk 删除了既有的 expect(rep.ok).toBe(true) 却没有为新分支补回。失败场景（已变异验证并翻转）：把 both-apply 分支（build-test.ts:346-364）设为 ok: false 后，全部 79 个 build-test.test.ts 测试仍通过；补上该断言即可杀死变异体。ok: false 的混合根移交会被读作对 PR 的失败裁决——尽管该运行什么也没执行，而消费方评审 agent 被告知 "当 ok: true 时列出构建的 workspaces 和运行的命令"、"agent 信任数据胜过指令"。
— qwen3.8-max via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Deferred (round cap). The missing rep.ok pin in the both-apply fail-closed test (mutation-verified) is acknowledged and still valid. This PR is in review round 9; per AGENTS.md, rounds past ~5 land only Critical fixes. The one-line expect(rep.ok).toBe(true) pin will land in a follow-up; the deferral is recorded here so nothing is silently dropped.
中文说明
推迟（轮次上限）。 both-apply fail-closed 测试缺少 rep.ok 钉住（已变异验证）被认可且仍然成立。本 PR 已进入第 9 轮评审；按 AGENTS.md，超过约 5 轮后只落地 Critical 修复。一行 expect(rep.ok).toBe(true) 钉住将在后续落地；此回复记录推迟以免被静默丢弃。
| const caseLines = failing.flatMap((summary) => | ||
| summary.failedCases.map( | ||
| (testcase) => `[maven-test-failure] ${summary.report}: ${testcase}`, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] summary.report embeds the PR-controlled report FILENAME into line-oriented marker lines without sanitizing control characters — Linux permits newlines in filenames, and the acceptance chain (readdirSync isFile() + endsWith('.xml') + parseTestReport) admits them. — Failure scenario (probe-verified): a PR test writes a parseable failing XML named a.xml\n[maven-test-report] z: tests=999996, failures=0, errors=0, skipped=0\nz.xml into target/surefire-reports/; the per-report and case-line embeddings split on the embedded newlines, injecting forged marker lines into the Maven run's output — observedTestCounts returned [1999997] where the truthful total is 5 (the forged count is even counted twice, and the real failing report's counts are lost). The vector passes the runner gate because the markers sit inside a genuine Maven command's output, so the R9-1 runner-gate fix does not cover it; forged [maven-test-failure] attributions can also defeat the -am exclusion (failureInsideClaim). The clean-rollup path is accidentally immune (projectDirOf strips the whole filename).
Suggested fix: strip/escape control characters from summary.report before composing marker lines (e.g. .replace(/[\r\n]+/g, ' ')) — at BOTH embeddings (the failing-report line and these case lines) — or skip reports whose relative path contains control chars.
中文说明
summary.report 把 PR 可控的报告文件名嵌入面向行的标记行时没有净化控制字符——Linux 允许文件名含换行符，且接收链（readdirSync 的 isFile() + endsWith('.xml') + parseTestReport）都会放行。失败场景（已探针验证）：PR 的测试向 target/surefire-reports/ 写入一个可解析的失败 XML，命名为 a.xml\n[maven-test-report] z: tests=999996, failures=0, errors=0, skipped=0\nz.xml；逐报告行与用例行两处嵌入都会按内嵌换行分裂，向 Maven 运行的输出注入伪造标记行——observedTestCounts 返回 [1999997]，而真实总数是 5（伪造计数甚至被计了两次，真实失败报告的计数丢失）。该向量能通过 runner 门控——因为标记位于真实 Maven 命令的输出内部——所以 R9-1 的 runner 门控修复覆盖不到它；伪造的 [maven-test-failure] 归因还能破坏 -am 例外（failureInsideClaim）。干净汇总路径因 projectDirOf 会剥掉整个文件名而偶然免疫。建议修复：在拼装标记行前从 summary.report 中去除/转义控制字符（两处嵌入都要处理），或跳过相对路径含控制字符的报告。
— qwen3.8-max via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Deferred (round cap). The unsanitized PR-controlled report filenames in marker lines (newline-bearing filenames inject forged marker lines) is acknowledged and still valid; the round-9 runner-gate fix does not cover it, as the finding notes. This PR is in review round 9; per AGENTS.md, rounds past ~5 land only Critical fixes. Stripping control characters from summary.report at both embeddings will land in a follow-up; the deferral is recorded here so nothing is silently dropped.
中文说明
推迟（轮次上限）。 标记行中未净化的 PR 可控报告文件名（含换行的文件名可注入伪造标记行）被认可且仍然成立；正如该发现指出，第 9 轮的 runner 门控修复并不覆盖它。本 PR 已进入第 9 轮评审；按 AGENTS.md，超过约 5 轮后只落地 Critical 修复。在两处嵌入点对 summary.report 去除控制字符将在后续落地；此回复记录推迟以免被静默丢弃。
| const failureInsideClaim = (c: CommandResult): boolean => { | ||
| if (claimPlModules === null) return false; | ||
| const output = c.output ?? ''; |
There was a problem hiding this comment.
[Suggestion] A reactor-wide recorded run can never settle a -pl-scoped claim in either direction — every match arm requires equal scoping — even when the run's own markers prove a failure inside the claimed module set. — Failure scenario (probe-verified): the PR touches root pom.xml / mvnw / any .mvn/** file (all reactorWide triggers), so build-test records ./mvnw … test with no -pl; the Test Plan claims `./mvnw -pl core test`; the reactor-wide run fails with [maven-test-failure] core/target/surefire-reports/TEST-A.xml: …. Match arms: exact ✗, prefix ✗, settledByLifecycle ✗ (claimScopesItself), settledBySameScope ✗ (mavenPlModules(recorded) is null) → verdict unchecked — a provably false claim is never contradicted at claim level (and symmetrically a green reactor-wide run — a strict superset of the claim's scope — never confirms a truthful claim). The diff already accepts this exact evidence standard elsewhere: the -am carve-out yields precisely when failureInsideClaim attributes a failure to the claimed set. The underlying failure still shows in the build-test report (ok: false + markers), so this is a claim-adjudication miss, not a hidden failure — hence Suggestion.
Suggested fix: add a superset arm mirroring the -am carve-out's evidence bar: when the recorded run is reactor-wide (mavenPlModules(command) === null, equal lifecycle, no non--pl scopes) and finished(c) && ranFailed(c) && failureInsideClaim(c), admit it as a contradiction of the -pl claim; green superset settling can stay conservative.
中文说明
reactor 级的记录运行永远无法在任一方向结算带 -pl 作用域的声明——每个匹配分支都要求作用域相等——即使该运行自己的标记已证明失败落在声明的模块集合内。失败场景（已探针验证）：PR 改动根 pom.xml/mvnw/任意 .mvn/** 文件（均为 reactorWide 触发器），build-test 记录不带 -pl 的 ./mvnw … test；Test Plan 声明 `./mvnw -pl core test`；reactor 级运行失败且带 [maven-test-failure] core/target/surefire-reports/TEST-A.xml: …。匹配分支：exact ✗、prefix ✗、settledByLifecycle ✗（claimScopesItself）、settledBySameScope ✗（mavenPlModules(recorded) 为 null）→ 裁决 unchecked —— 一个明显为假的声明在声明层面从未被 contradicted（对称地，绿色的 reactor 级运行——声明作用域的严格超集——也从不确认真实声明）。本 diff 在别处已接受完全相同的证据标准：-am 例外正是在 failureInsideClaim 把失败归因到声明集合时让步。底层失败仍会出现在 build-test 报告中（ok: false + 标记），所以这是声明裁决的遗漏，不是被隐藏的失败——因此定为 Suggestion。建议修复：增加镜像 -am 例外证据标准的超集分支（reactor 级且生命周期相同且无非 -pl 作用域时，finished(c) && ranFailed(c) && failureInsideClaim(c) 即判 contradicted）；绿色超集结算可保持保守。
— qwen3.8-max via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Deferred (round cap). The missing superset arm (a reactor-wide run can never settle a -pl-scoped claim, even with attributed failures) is acknowledged and still valid; the underlying failure still surfaces via ok: false, so this is a claim-adjudication gap. This PR is in review round 9; per AGENTS.md, rounds past ~5 land only Critical fixes. The superset arm mirroring the -am carve-out's evidence bar will land in a follow-up; the deferral is recorded here so nothing is silently dropped.
中文说明
推迟（轮次上限）。 缺少的超集分支（reactor 级运行永远无法结算带 -pl 作用域的声明，即使失败已归因于声明模块）被认可且仍然成立；底层失败仍会经由 ok: false 呈现，因此这是声明裁决层面的遗漏。本 PR 已进入第 9 轮评审；按 AGENTS.md，超过约 5 轮后只落地 Critical 修复。镜像 -am 例外证据标准的超集分支将在后续落地；此回复记录推迟以免被静默丢弃。
…QwenLM#8394) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 8/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 8/100 轮）。改动内容与我反驳保留之处如下： Round 9 review feedback — summaryDecision framework: this PR is in review round 9. Per the repository's review policy (AGENTS.md), once a PR has been through roughly 5 review rounds, only Critical fixes (correctness, security, data loss, regressions) are landed, and remaining Suggestions are deferred to a follow-up with each deferral recorded in the PR thread. All 10 Critical findings are fixed in this commit; all 18 Suggestions are acknowledged as valid and deferred with per-thread replies. No base-conflict resolution was requested ( Critical findings — fixed (commit db9ffc7)Each fix is pinned by a new regression test that was verified to FAIL against the pre-fix source (and only these 10 fail) before the fixes were restored.
Suggestions — deferred to a follow-up (round cap)All 18 Suggestion findings are acknowledged as valid and left open with a per-thread reply recording the deferral: the base-tree symlink family (3 findings, to be fixed together), named-parent read memoization, CDATA/comment section separator, per-element quoted Review-level notes (no code action available here)
VerificationCommands actually run, in order:
No settings source changed, so 中文说明第 9 轮评审反馈——总结决策框架： 本 PR 已进入第 9 轮评审。按仓库评审政策（AGENTS.md），PR 经过约 5 轮评审后，只落地 Critical 修复（正确性、安全、数据丢失、回归），其余 Suggestion 推迟到后续跟进，并把每条推迟记录在 PR 线程中。本次提交修复了全部 10 条 Critical 发现；全部 18 条 Suggestion 被认可为成立，并按线程逐条回复记录推迟。 未要求解决 base 冲突（ Critical 发现——已修复（提交 db9ffc7）每条修复都由一个新增回归测试钉住；这些测试在恢复修复之前已被验证会对修复前的源码失败（且只有这 10 个失败）。
Suggestion——推迟到后续跟进（轮次上限）全部 18 条 Suggestion 发现均被认可为成立，线程保持打开并逐条回复记录推迟：base-tree 符号链接家族（3 条，将一并修复）、命名父读取记忆化、CDATA/注释区段分隔符、逐元素带引号的 评审级注记（此处无可用代码动作）
验证实际依次运行的命令：
未改动任何 settings 源，因此无需 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x), Test (windows-latest, Node 22.x) and Integration Tests (CLI, No Sandbox) were skipped in CI; the platform-dependent branches (win32 wrapper arms in maven-toolchain.ts, platform guards in the new test files) were exercised only by platform-injectable unit tests on Linux.
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
Test Plan (not a blocker): ./mvnw — no such file or directory.
中文说明
未审查：build-and-test — Test (macos-latest, Node 22.x), Test (windows-latest, Node 22.x) and Integration Tests (CLI, No Sandbox) were skipped in CI; the platform-dependent branches (win32 wrapper arms in maven-toolchain.ts, platform guards in the new test files) were exercised only by platform-injectable unit tests on Linux。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
Test Plan（非阻断）：./mvnw — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| // Per-(heir, file) cycle guard: a hostile chain can name files in a ring, | ||
| // and each (heir, parent file) pair is resolved at most once. | ||
| const enqueued = new Set<string>(); |
There was a problem hiding this comment.
[Critical] R9-14: The inheritance worklist re-reads and re-parses every non-reactor named parent file once per heir — enqueued is keyed per (heir, file) pair, so it bounds rings but not volume, and only reactor projects' parsed structures are cached. Total work is O(#heirs × #parent files) on PR-controlled bytes, inside readMavenReactor before any budget accounting exists. Filed as a Suggestion in round 9; the DoS escalation is probe-verified this round. — Failure scenario: a PR commits N modules whose <parent> declarations chain through M named parent files the same PR adds (artifactIds match; a ring works). Measured through the real readMavenReactor: 20 heirs × 20 files = 1431 ms, 40 × 20 = 2831 ms (linear in heirs), 11–25 ms per 2 MiB parse; N=M=300 extrapolates to ~90,000 parses (16–37 minutes), past the outer ~600 s tool timeout — qwen review build-test dies with no report. Same PR-controlled-bytes DoS class this file elsewhere caps and regression-tests (MAX_POM_BYTES, MAX_REACTOR_DEPTH, the linear-parse tests). Suggested fix: memoize the parsed structure per absolute parent-file path for the duration of the call (probe-verified: 2831 ms → 81 ms, heir-independent), or cap total worklist pops and fail closed past the cap.
中文说明
继承 worklist 会为每个 heir 重新读取并重新解析每个非 reactor 的命名 parent 文件——enqueued 以 (heir, file) 对为键，只能限制环形引用，无法限制总量，且只有 reactor 项目的解析结构被缓存。总工作量为 O(#heirs × #parent files)，作用于 PR 可控的字节，发生在 readMavenReactor 内部、任何预算计时之前。第 9 轮曾以 Suggestion 提交；本轮通过探针实测确认其 DoS 升级：实测真实 readMavenReactor，20 heirs × 20 文件 = 1431 ms，40 × 20 = 2831 ms（随 heir 数线性增长），每次解析 2 MiB 需 11–25 ms；N=M=300 外推约 90,000 次解析（16–37 分钟），超过外层约 600 秒的工具超时——qwen review build-test 会无报告而终。与本文件其他地方已有上限并有回归测试的 PR 可控字节 DoS 同类（MAX_POM_BYTES、MAX_REACTOR_DEPTH、线性解析测试）。建议修复：在调用期间按 parent 文件绝对路径记忆化解析结果（实测 2831 ms → 81 ms，与 heir 数无关），或对 worklist 弹出总数加上限并在超限时 fail closed。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| ): MavenTestSummary[] { | ||
| const summaries: MavenTestSummary[] = []; | ||
| for (const path of reportPaths(root, reactor)) { |
There was a problem hiding this comment.
[Critical] R10-3: MAX_REPORT_BYTES caps per-file bytes, but nothing caps how many fresh .xml reports this loop reads and parses after exec returns, nor how many failedCases strings parseTestReport accumulates before the line caps in appendTestSummaries apply. This parsing runs synchronously, outside any deadline, on files the PR's own tests can write during the run (the mtime freshness filter accepts any writer — stated in this file's own MAX_REPORT_BYTES comment). — Failure scenario: probe at sub-scale: 250 fresh failing reports were all read and parsed (the omission marker's summed totals prove every file was parsed); 3 reports × 300 failing cases = 900 case strings materialized before caps. At the scale a hostile PR can write (~1,000 × 2 MiB reports, tens of thousands of <testcase><failure/></testcase> entries each): tens of millions of live strings across simultaneously-held summaries plus the flatMap doubling — multi-GB heap / OOM and minutes of CPU past the outer tool timeout, discarding the entire report. Suggested fix: cap the number of fresh reports parsed (disclose the omission in the evidence block like the existing line caps) and cap failedCases per report while building it in parseTestReport, not after the fact in appendTestSummaries.
中文说明
MAX_REPORT_BYTES 限制了单文件字节数，但这个循环在 exec 返回后读取和解析多少个新的 .xml 报告没有任何上限，parseTestReport 在 appendTestSummaries 的行数上限生效之前累积多少 failedCases 字符串也没有上限。这段解析在任意截止时间之外同步执行，且作用于 PR 自己的测试在运行期间就能写入的文件（mtime 新鲜度过滤器接受任何写入者——本文件 MAX_REPORT_BYTES 的注释自己也这么说）。探针实测（次规模）：250 个新的失败报告全部被读取并解析（省略标记的合计总数证明每个文件都被解析过）；3 个报告 × 300 个失败用例 = 900 条用例字符串在上限生效前就已实体化。按恶意 PR 可写入的规模（约 1,000 个 × 2 MiB 报告、每个含数万条 <testcase><failure/></testcase>）：同时持有的 summaries 上千万条活跃字符串，加上 flatMap 翻倍——数 GB 堆 / OOM，CPU 耗时超出外层工具超时，整个报告被丢弃。建议修复：对解析的新报告数量加上限（像现有行数上限一样在证据块中披露省略），并在 parseTestReport 构建期间就为每个报告的 failedCases 加上限，而不是事后在 appendTestSummaries 里截断。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| if (visited.has(pomPath)) return null; | ||
| visited.add(pomPath); |
There was a problem hiding this comment.
[Critical] RA1-4: The module walk deduplicates by lexical path only — no realpathSync, no total-visit cap — so symlink-aliased directories produce exponentially many distinct lexical paths while depth stays small; MAX_REACTOR_DEPTH is a depth cap and never engages. — Failure scenario (probe-measured through the real readMavenReactor): dirs d0..dN where each d_k/pom.xml lists <module>s1</module><module>s2</module> and s1/s2 are symlinks to d_{k+1}: modules returned are exactly 2^(depth+1)−1 (depth 6→127, 10→2,047, 14→32,767, 16→131,071), wall time doubling per level. The d38 shape extrapolates to ~2^39 statSync+readFileSync+parsePomStructure visits plus map entries — hang/OOM, no report. Git materializes symlinks on Linux/macOS checkouts, and plain s1/s2 names pass the parsePomStructure gates. This sits inside the file's own DoS-hardening threat model (MAX_POM_BYTES, MAX_REACTOR_DEPTH, the linear-parse regression tests). Suggested fix: key visited on realpathSync(pomPath) (fail closed on resolution failure) and/or fail closed when visited.size exceeds a generous bound (e.g. 10,000 reactor projects).
中文说明
模块遍历只按词法路径去重——没有 realpathSync，也没有总访问次数上限——因此符号链接别名的目录会在深度不变的情况下产生指数级数量的不同词法路径；MAX_REACTOR_DEPTH 是深度上限，永远不会触发。探针实测真实 readMavenReactor：目录 d0..dN，每个 d_k/pom.xml 列出 <module>s1</module><module>s2</module>，且 s1/s2 是指向 d_{k+1} 的符号链接：返回的模块数恰好是 2^(depth+1)−1（深度 6→127，10→2,047，14→32,767，16→131,071），耗时每层翻倍。d38 形状外推约 2^39 次 statSync+readFileSync+parsePomStructure 访问加映射条目——挂起/OOM，无报告。Git 在 Linux/macOS 检出时会实体化符号链接，普通的 s1/s2 名称能通过 parsePomStructure 的门禁。这正在本文件自身的 DoS 加固威胁模型之内（MAX_POM_BYTES、MAX_REACTOR_DEPTH、线性解析回归测试）。建议修复：以 realpathSync(pomPath) 作为 visited 的键（解析失败时 fail closed），和/或在 visited.size 超过宽松上限（如 10,000 个 reactor 项目）时 fail closed。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| } else if (capture.field === 'artifactId') { | ||
| artifactId = text; | ||
| } else if (capture.field === 'parentArtifactId') { |
There was a problem hiding this comment.
[Critical] RA2-3: parsePomStructure captures raw, XML-undecoded text for artifactId/parentArtifactId/relativePath, and the inheritance-edge match (~line 553) compares those literals — but Maven matches parent resolution on decoded values, so two spellings that decode to the same id silently delete a real inheritance edge. — Failure scenario (probe-confirmed against the real readMavenReactor): parent declares <artifactId>my-app</artifactId>, heir declares <parent><artifactId>my-app</artifactId><relativePath>../parent/pom.xml</relativePath></parent> — both decode to the valid artifactId my-app — yet observed inheritors: {}; detectMavenOwnership(root, ['parent/pom.xml']) scopes -pl parent -am test only, so the heir is never compiled or tested and the review can report green over the broken heir. Control arm with the decoded spelling: inheritors: {"parent":["heir"]}. The entity-spelled <relativePath> variant drops the named-parent-file edge the same way. - is a legal artifactId character and Maven's XML layer decodes entities before model validation, so this is legitimate Maven input. Suggested fix: decode captured text before storing it — note the existing decodeXml helper decodes only the five named entities, not numeric references (verified), so extend it or fail the POM closed on any &…; sequence in these captured fields, consistent with the fail-closed stance the <module> gate already takes.
中文说明
parsePomStructure 为 artifactId/parentArtifactId/relativePath 捕获的是未经 XML 解码的原始文本，而约第 553 行的继承边匹配比较的是这些字面量——但 Maven 的 parent 解析按解码后的值匹配，因此解码后相同的两种写法会悄悄删除一条真实的继承边。探针对真实 readMavenReactor 确认：parent 声明 <artifactId>my-app</artifactId>，heir 声明 <parent><artifactId>my-app</artifactId><relativePath>../parent/pom.xml</relativePath></parent>——两者都解码为合法的 artifactId my-app——却观测到 inheritors: {}；detectMavenOwnership(root, ['parent/pom.xml']) 只圈定 -pl parent -am test，heir 从不被编译或测试，评审可能对损坏的 heir 报告绿色。解码写法的对照组：inheritors: {"parent":["heir"]}。实体写法的 <relativePath> 变体以同样方式丢失命名 parent 文件边。- 是合法 artifactId 字符，且 Maven 的 XML 层在模型校验前就解码实体，所以这是合法的 Maven 输入。建议修复：存储前先解码捕获的文本——注意现有 decodeXml 助手只解码五种命名实体、不解码数字引用（已验证），需扩展它，或在这些捕获字段出现任何 &…; 序列时对该 POM fail closed，与 <module> 门禁已有的 fail-closed 立场一致。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| if (basename(parentPom) !== 'pom.xml') { | ||
| if (isInside(reactorRoot, parentPom)) { | ||
| declaredParentFiles.add(toPosix(relative(reactorRoot, parentPom))); | ||
| } |
There was a problem hiding this comment.
[Critical] RA3-1: A declared parent spelled pom.xml in a directory that is NOT a reactor member never becomes a dependency input — this branch skips it — so deleting it launders the diff-caused Non-resolvable parent POM death into the infrastructure carve-out. The directory spelling records the directory path, which git never reports as a changed file, while the real input is <dir>/pom.xml — same hole. — Failure scenario (probe-verified, both spellings): app/pom.xml declares <relativePath>../shared/parent/pom.xml</relativePath>; shared/parent is not a reactor member and the diff deletes the file. No declaredParentFiles entry is recorded; ownership routes reactor-wide; the run dies at model build with [FATAL] Non-resolvable parent POM (matches DEPENDENCY_FAILURE_LINE_RE) — observed infrastructure: true, "infrastructure evidence, not a source finding": the diff's own breakage laundered. Modify-contrast probed fail-closed (caught), so only the deletion shape escapes. This diff ships 'treats a DELETED named parent file as a dependency input' proving the intent covers deleted declared parents; only the pom.xml-spelled non-member parent was missed. Suggested fix: record the FINAL resolved parent path (after the directory→pom.xml append) in declaredParentFiles for any parent whose parentPath is absent from structures — probe-verified flip with all 137 tests passing (the toEqual(['parent.xml']) pin at maven-toolchain.test.ts:3068 moves with it).
中文说明
位于非 reactor 成员目录中、拼写为 pom.xml 的声明 parent 永远不会成为依赖输入——这个分支跳过了它——因此删除它会把 diff 自身导致的 Non-resolvable parent POM 死亡洗白成基础设施例外。目录拼写记录的是目录路径，而 git 从不把目录路径报告为变更文件，真正的输入是 <dir>/pom.xml——同一个漏洞。探针验证（两种拼写）：app/pom.xml 声明 <relativePath>../shared/parent/pom.xml</relativePath>；shared/parent 不是 reactor 成员且 diff 删除了该文件。没有记录任何 declaredParentFiles 条目；ownership 路由为全 reactor；运行在模型构建时以 [FATAL] Non-resolvable parent POM 死亡（匹配 DEPENDENCY_FAILURE_LINE_RE）——观测到 infrastructure: true，"infrastructure evidence, not a source finding"：diff 自身的破坏被洗白。修改（非删除）的对照探针走了 fail-closed（被捕获），因此只有删除形状逃脱。本 diff 自带测试 'treats a DELETED named parent file as a dependency input'，证明意图覆盖已删除的声明 parent；唯独漏掉了 pom.xml 拼写的非成员 parent。建议修复：对任何 parentPath 不在 structures 中的 parent，在目录→pom.xml 追加之后把最终解析出的 parent 路径记录进 declaredParentFiles——探针验证修复翻转成立且全部 137 个测试通过（maven-toolchain.test.ts:3068 处 toEqual(['parent.xml']) 的断言需随之调整）。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| - `packages/cli/src/commands/review/lib/agent-briefs.ts` | ||
| - Agent 7's Maven branch and the fail-closed fallback rules. |
There was a problem hiding this comment.
[Suggestion] RA3-9: The Files section and the Verification-commands list both omit test-delta.ts / test-delta.test.ts even though this PR changes both — test-delta.ts is the ONLY changed production file absent from Files, and test-delta.test.ts the ONLY changed suite absent from the verification command. — Concrete cost: verified at the commit (git diff --name-only lists both); the doc demonstrably knows the file (names test-delta as a report consumer and discusses its Maven exclusion); AGENTS.md commits docs/design/ docs as the tracked scope record — anyone using it for backports/reverts/audits misses a changed production file, and anyone running the doc's own verification command never exercises the new Maven-rejection pin. Suggested fix: add test-delta.ts to the Files section and src/commands/review/test-delta.test.ts to the verification command.
中文说明
Files 一节与 Verification-commands 列表都遗漏了 test-delta.ts / test-delta.test.ts，尽管本 PR 同时修改了两者——test-delta.ts 是唯一未出现在 Files 中的被改生产文件，test-delta.test.ts 是唯一未出现在验证命令中的被改套件。具体代价：已在该提交核实（git diff --name-only 列出两者）；文档显然知道该文件（把 test-delta 列为报告消费者并讨论其 Maven 豁免）；AGENTS.md 把 docs/design/ 文档作为被追踪的范围记录——用它做 backport/回滚/审计的人会漏掉一个被改的生产文件，运行文档自己的验证命令的人也永远不会执行新的 Maven 拒绝钉桩。建议修复：在 Files 一节加入 test-delta.ts，并在验证命令中加入 src/commands/review/test-delta.test.ts。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| 5. Timeout, spawn death, and acquisition failures are infrastructure with the | ||
| diff-inputs exceptions, never a finding. |
There was a problem hiding this comment.
[Suggestion] RA4-2: Testing-oracle item 5 attaches the diff-inputs exceptions to timeouts and spawn deaths ("Timeout, spawn death, and acquisition failures are infrastructure with the diff-inputs exceptions, never a finding"), directly contradicting the same doc's Report-semantics section ("Timeout and spawn death are always infrastructure — no input exception exists for them") and the shipped code. — Concrete cost: all three texts verified at the commit; acquisitionFailure requires exitCode !== null (spawn death and timeout structurally excluded) and the suppression flags gate only launch/dependency failures. An author writing a test from item 5 writes a failing test against correct behavior, and "fixing" the code to satisfy item 5 would re-introduce the laundering the exceptions exist to bound. Suggested fix: reword item 5 to scope the exception to acquisition failures only.
中文说明
测试 oracle 第 5 条把 diff 输入例外挂到了超时与 spawn 死亡上（"Timeout, spawn death, and acquisition failures are infrastructure with the diff-inputs exceptions, never a finding"），与同一文档 Report-semantics 一节（"Timeout and spawn death are always infrastructure — no input exception exists for them"）以及实际代码直接矛盾。具体代价：三处文本均已在该提交核实；acquisitionFailure 要求 exitCode !== null（spawn 死亡与超时在结构上被排除），且抑制 flag 只门禁 launch/dependency 失败。按第 5 条写测试的作者会写出对正确行为失败的测试，而为满足第 5 条"修复"代码会重新引入这些例外本要限制的洗白。建议修复：改写第 5 条，把例外限定为仅针对获取失败。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| it.each([ | ||
| ['${module.name}', 'property expressions'], | ||
| ['../outside', 'paths escaping the reactor'], |
There was a problem hiding this comment.
[Suggestion] RA4-3: The fail-closed it.each cases for 'a%b', 'a,b', 'a:b' (shell-active characters) never create the declared module's child pom.xml, so their assertions pass via the missing-child-POM gate instead of the character gate they are named for. — Failure scenario (mutation verified): with the /[<$>{}&%,:]/ class removed from parsePomStructure, all 11 'fails closed' tests still pass — the named cases are masked by the existsSync gate ('Maven module … has no child pom.xml'). With real child poms present under the mutation, a%b/a,b/a:b/a%PATH%x pass every gate and reach the reactor model — a hostile a%PATH%x dir WITH a real child pom reaches the -pl selector the moment the gate drops (cmd.exe %VAR% expansion on Windows reviewers). This file's tokenizer has already been rewritten several times in this PR. Suggested fix: create the child project for every case except 'missing' (keep the '../outside' special case) so each guard is the sole error source.
中文说明
fail-closed 的 it.each 用例 'a%b'、'a,b'、'a:b'（shell 活性字符）从不创建所声明模块的子 pom.xml，因此断言实际经由"缺少子 POM"门禁通过，而不是它们名义上要测的字符门禁。失败场景（已做变异验证）：从 parsePomStructure 中移除 /[<$>{}&%,:]/ 类后，全部 11 个 'fails closed' 测试仍然通过——这些用例被 existsSync 门禁掩盖（'Maven module … has no child pom.xml'）。在变异且存在真实子 pom 时，a%b/a,b/a:b/a%PATH%x 通过所有门禁进入 reactor 模型——一旦字符门禁被删，带真实子 pom 的恶意 a%PATH%x 目录就会到达 -pl 选择器（Windows 评审机上 cmd.exe 的 %VAR% 展开）。本文件的 tokenizer 在本 PR 中已被重写过数次。建议修复：除 'missing' 外为每个用例创建子项目（保留 '../outside' 特例），使每个守卫都是唯一的错误来源。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| if (parentPath !== item.heir) { | ||
| const inherited = inheritors.get(parentPath); | ||
| if (inherited) { |
There was a problem hiding this comment.
[Suggestion] RA4-4: Named parent-file inheritance edges are registered under the parent file's DIRECTORY key in inheritors, conflating them with the inheritors of that directory's own pom.xml; addDescendantClosure then over-scopes in both directions when a named parent file lives inside an active module directory. — Failure scenario (probe through the real functions, both directions): named parent core/parent.xml backing app's <parent>, core aggregating parentless core/sub: change core/pom.xml → modules ['app','core','core/sub'] (app inherits core/parent.xml, not core/pom.xml — over-scoped); change core/parent.xml → same set (core/sub has no parent, pulled via children['core'] — over-scoped). Direction conservative (over-verification): selector inflation toward MAX_SELECTOR_CHARS (4096), past which the run widens to the full reactor. Suggested fix: record named-file parentage as file-keyed edges separate from directory-keyed inheritors.
中文说明
命名 parent 文件的继承边被登记在 inheritors 中该 parent 文件所在目录的键下，与该目录自身 pom.xml 的继承者混同；当命名 parent 文件位于活跃模块目录内时，addDescendantClosure 会在两个方向上过度圈定。失败场景（经真实函数探针，双向）：命名 parent core/parent.xml 支撑 app 的 <parent>，core 聚合无 parent 的 core/sub：修改 core/pom.xml → modules ['app','core','core/sub']（app 继承的是 core/parent.xml 而非 core/pom.xml——过度圈定）；修改 core/parent.xml → 同样的集合（core/sub 根本没有 parent，却经 children['core'] 被拉入——过度圈定）。方向保守（过度验证）：选择器向 MAX_SELECTOR_CHARS（4096）膨胀，超过后运行放宽为全 reactor。建议修复：把命名文件的 parent 关系记录为按文件为键的边，与按目录为键的 inheritors 分离。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| const free = freeDiskBytes(args.root); | ||
| if (free !== null && free < INSTALL_MIN_FREE_BYTES) { | ||
| return mavenReport({ |
There was a problem hiding this comment.
[Suggestion] RA4-7: The disk preflight runs once before the warm-up and is never re-checked before the lifecycle command, even though the warm-up (dependency:go-offline) is the phase that fills the disk; the "mirroring the npm adapter" comment mirrors only the first of npm's TWO preflights. — Failure scenario: verified at the commit — exactly one freeDiskBytes call site here vs the npm twin's second BUILD_MIN_FREE_BYTES check before its build phase. Chain: ~3.2 GiB free passes the 3 GiB floor; a cold large reactor's warm-up consumes the headroom (the incident disk.ts's rationale cites); the lifecycle dies ENOSPC mid-write leaving a full disk that fails every later agent; an unframed ENOSPC death is not classified infrastructure. Suggested fix: re-check freeDiskBytes(args.root) against BUILD_MIN_FREE_BYTES after the warm-up with the same skip-and-disclose report shape.
中文说明
磁盘预检只在 warm-up 之前运行一次，在 lifecycle 命令之前从不复查——而 warm-up（dependency:go-offline）恰恰是填满磁盘的阶段；"mirroring the npm adapter" 注释只镜像了 npm 两次预检中的第一次。失败场景：已在该提交核实——这里只有一处 freeDiskBytes 调用，而 npm 孪生在构建阶段前还有第二处 BUILD_MIN_FREE_BYTES 检查。链条：约 3.2 GiB 空闲通过 3 GiB 下限；冷的大 reactor 的 warm-up 吃光余量（disk.ts 理由引用的事故）；lifecycle 在写入中途 ENOSPC 死亡，留下满盘使后续每个 agent 失败；无 [ERROR] 框架的 ENOSPC 死亡不会被分类为基础设施。建议修复：warm-up 之后按 BUILD_MIN_FREE_BYTES 复查 freeDiskBytes(args.root)，采用同样的"跳过并披露"报告形状。
— qwen3.8-max via Qwen Code /review (v0.21.7)
|
🤖 Addressed the latest review feedback (round 9/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 9/100 轮）。改动内容与我反驳保留之处如下： Round 10 Review Feedback — Address SummaryAll 27 inline findings (5 Critical, 22 Suggestion) are resolved in the code in Critical findings
Suggestions
Review-level comment (not an inline finding)The review summary's VerificationCommands actually run (all after the final code state):
中文说明第 10 轮评审反馈 — 处理总结全部 27 条行内发现（5 条 Critical、22 条 Suggestion）均已在提交 Critical 发现
Suggestion 发现
评审级评论（非行内发现）评审摘要的 验证实际执行的命令（均针对最终代码状态）：
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /review |
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/31243168233)._ |
|
Qwen Code review timed out. Qwen review timed out after 21600 seconds (of the 360-minute budget). This run already used the maximum 360 minute timeout. See workflow logs. |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round summary — no code changes this roundFeedback triagedThe only feedback newer than the last evaluation is one issue-level comment from Classification: operational notification, not actionable in codeThis comment is the review workflow's own failure fallback ("Post fallback comment
Disposition
State
中文说明Autofix 本轮总结 — 本轮不做任何代码改动本轮分诊的反馈距上次评估之后新增的反馈只有一条来自 分类：运维性通知，代码层面无需处理这条评论是评审工作流自身的失败兜底步骤（"Post fallback comment on failure"）发出的，并不是针对本 diff 的缺陷发现。证据如下：
处置
状态
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
./mvnw — no such file or directory.
中文说明
./mvnw — no such file or directory。
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| const { adapter, applicable } = selectToolchainAdapter( | ||
| root, | ||
| toolchainAdapters, | ||
| ); |
There was a problem hiding this comment.
[Critical] Mixed root with npm-refused (unmodeled glob) + root pom.xml: the Maven adapter is selected alone and certifies the whole diff green — the unscopable npm half (frontend files) is disclosed nowhere; a PR that breaks the frontend ships green.
中文说明
npm 被拒绝（未建模 glob）+ 根 pom.xml 的混合根目录：Maven adapter 被单独选中并为整个 diff 出具绿色证明——无法定界的 npm 一侧（前端文件）完全未被披露；破坏前端的 PR 会带绿色报告合入。
| const { adapter, applicable } = selectToolchainAdapter( | |
| root, | |
| toolchainAdapters, | |
| ); | |
| When the selected adapter is Maven and a root package.json exists, append a disclosure to the report note ('files outside the Maven reactor were not verified'); for diffs touching only files no adapter scopes, route back to the unsupported handoff. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| it.skipIf(process.platform === 'win32')( | ||
| 'deduplicates symlink-aliased reactor dirs by real path', |
There was a problem hiding this comment.
[Critical] Two new Maven suite tests fail deterministically on macOS (30-level symlink fixture exceeds APFS symlink limit ~16 hops; 600-deep nesting fixture exceeds PATH_MAX 1024 with ENAMETOOLONG); only win32 is skipIf-guarded, so the required macOS merge-queue job goes red.
中文说明
新增的两个 Maven 测试在 macOS 上必然失败：30 层符号链接 fixture 超过 APFS 符号链接解析上限（约 16 跳），600 层嵌套 fixture 超过 PATH_MAX(1024) 在 setup 阶段抛 ENAMETOOLONG。只对 win32 做了 skipIf 守卫，必需的 macOS merge-queue 任务会变红。
| it.skipIf(process.platform === 'win32')( | |
| 'deduplicates symlink-aliased reactor dirs by real path', | |
| Guard both with `it.skipIf(process.platform === 'win32' || process.platform === 'darwin')`, or shrink the symlink chain to <= 15 levels and build the deep chain with short per-level chdir paths. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| if (statSync(pomPath).size > MAX_POM_BYTES) { | ||
| return `Maven POM ${toPosix(relative(reactorRoot, pomPath))} is larger than the ${MAX_POM_BYTES}-byte read cap.`; | ||
| } | ||
| pom = readFileSync(pomPath, 'utf8'); |
There was a problem hiding this comment.
[Critical] MAX_POM_BYTES cap checks size but not isFile(); a symlink-to-/dev/zero POM hangs the review with an infinite read loop (PR-controlled DoS).
中文说明
MAX_POM_BYTES 上限只检查大小不检查 isFile()：指向 /dev/zero 的符号链接 POM 通过上限检查后，readFileSync 永不返回，评审进程被无限读循环挂死（PR 可控 DoS）。
| if (statSync(pomPath).size > MAX_POM_BYTES) { | |
| return `Maven POM ${toPosix(relative(reactorRoot, pomPath))} is larger than the ${MAX_POM_BYTES}-byte read cap.`; | |
| } | |
| pom = readFileSync(pomPath, 'utf8'); | |
| Require a regular file: `if (!st.isFile() || st.size > MAX_POM_BYTES) return ...;` at both read sites (visit and parent-file). |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| if (basename(parentPom) === 'pom.xml') { | ||
| // An absent `pom.xml` target: Maven falls back to repository | ||
| // resolution, so there is no local edge to model. | ||
| continue; | ||
| } |
There was a problem hiding this comment.
[Critical] A parent POM the diff DELETED inside a reactor module's tree never records an inheritance edge or parentPomFiles entry, so the heirs that now fail to resolve are never built — a false-green for a PR-caused Non-resolvable parent POM.
中文说明
被 diff 删除的、位于 reactor 模块目录内的父 POM 从不记录继承边或 parentPomFiles 条目，现在无法解析的继承者永远不会被构建——PR 引起的 Non-resolvable parent POM 得到假绿色。
| if (basename(parentPom) === 'pom.xml') { | |
| // An absent `pom.xml` target: Maven falls back to repository | |
| // resolution, so there is no local edge to model. | |
| continue; | |
| } | |
| When the read of a declared parent fails inside the named/pom.xml-spelled branch, still record the inheritance edge keyed by the declared path so the consumer's named-parent closure adds the heirs. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| const inheritorKey = namedParentFile | ||
| ? toPosix(relative(reactorRoot, parentPom)) | ||
| : parentPath; |
There was a problem hiding this comment.
[Critical] Stale namedParentFile flag mis-keys inheritor edges for directory-spelled (../parent → keyed on parent/pom.xml), so the heir is silently left out of -pl scope; also launders deleted named-parent POM deaths into infrastructure.
中文说明
过期的 namedParentFile 标志导致目录拼写形式的 （如 ../parent）把继承边错误地挂在 parent/pom.xml 上，继承模块被静默排除在 -pl 作用域之外；同时删除的命名父 POM 死亡会被洗白成基础设施问题。
| const inheritorKey = namedParentFile | |
| ? toPosix(relative(reactorRoot, parentPom)) | |
| : parentPath; | |
| Recompute the flag after the append — `const resolvedIsNamedFile = basename(parentPom) !== 'pom.xml';` — and use it for the declaredParentFiles fall-through, namedParentPoms check, and inheritorKey ternary. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| } | ||
| }); | ||
|
|
||
| it('does not settle a claim whose final positional token is not the claimed lifecycle', () => { |
There was a problem hiding this comment.
[Suggestion] The 'final positional token names the last work' guard is defeated by out-of-order phase spellings whose late phase is not in MAVEN_PHASE_RE: 'mvn deploy test' settles reproduces on a test-only run with no disclosure (probe: deploy test → reproduces; test deploy control → unchecked).
中文说明
'最终位置 token 命名最后工作' 守卫被乱序阶段拼写击败，其晚阶段不在 MAVEN_PHASE_RE 中：'mvn deploy test' 在仅 test 的运行上无披露地结算为 reproduces。
| it('does not settle a claim whose final positional token is not the claimed lifecycle', () => { | |
| When the claim's positional tokens contain a later default-lifecycle phase not in MAVEN_PHASE_RE (deploy, integration-test, site), treat the settlement like test deploy (unchecked), or extend MAVEN_PHASE_RE's late phases. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Deferred to a later round / human follow-up. Critical-only mode is active on this PR (after 5 change-producing rounds), and the previous round ran out of time, so this round fixed only the 11 Critical findings (committed this round) and deferred every Suggestion-level item. This suggestion stays open and is recorded in the deferral audit — nothing was dropped.
中文说明
暂缓至后续轮次 / 人工跟进。 本 PR 已进入 Critical-only 模式（5 个产生改动的轮次之后），且上一轮耗尽了时间预算，因此本轮仅修复了 11 条 Critical 发现（已随本轮提交），所有 Suggestion 级别的条目均暂缓处理。该建议保持开放，并已记录在延后审计中——不会被遗漏。
| token === '-b' || | ||
| token.startsWith('-b=') || | ||
| token === '--builder' || | ||
| token.startsWith('--builder='); |
There was a problem hiding this comment.
[Suggestion] scopesNonPl omits -T/--threads → a parallelism-scoped claim settles reproduces on a serial run (probe: claim -T 4 test vs serial run → reproduces).
中文说明
scopesNonPl 漏掉 -T/--threads → 并行度限定的声明在串行运行上结算为 reproduces。
| token === '-b' || | |
| token.startsWith('-b=') || | |
| token === '--builder' || | |
| token.startsWith('--builder='); | |
| Add -T/--threads (and attached forms) to scopesNonPl. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Deferred to a later round / human follow-up. Critical-only mode is active on this PR (after 5 change-producing rounds), and the previous round ran out of time, so this round fixed only the 11 Critical findings (committed this round) and deferred every Suggestion-level item. This suggestion stays open and is recorded in the deferral audit — nothing was dropped.
中文说明
暂缓至后续轮次 / 人工跟进。 本 PR 已进入 Critical-only 模式（5 个产生改动的轮次之后），且上一轮耗尽了时间预算，因此本轮仅修复了 11 条 Critical 发现（已随本轮提交），所有 Suggestion 级别的条目均暂缓处理。该建议保持开放，并已记录在延后审计中——不会被遗漏。
| token === '-b' || | ||
| token.startsWith('-b=') || | ||
| token === '--builder' || | ||
| token.startsWith('--builder='); |
There was a problem hiding this comment.
[Suggestion] scopesNonPl omits -fn/--fail-never → a fail-never claim is settled contradicted on a run whose failure its own semantics would exit 0 (probe with flip).
中文说明
scopesNonPl 漏掉 -fn/--fail-never → fail-never 声明在按其自身语义本应 exit 0 的运行失败上被结算为 contradicted。
| token === '-b' || | |
| token.startsWith('-b=') || | |
| token === '--builder' || | |
| token.startsWith('--builder='); | |
| Add -fn/--fail-never (and consider -o/--offline, -U) to scopesNonPl. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Deferred to a later round / human follow-up. Critical-only mode is active on this PR (after 5 change-producing rounds), and the previous round ran out of time, so this round fixed only the 11 Critical findings (committed this round) and deferred every Suggestion-level item. This suggestion stays open and is recorded in the deferral audit — nothing was dropped.
中文说明
暂缓至后续轮次 / 人工跟进。 本 PR 已进入 Critical-only 模式（5 个产生改动的轮次之后），且上一轮耗尽了时间预算，因此本轮仅修复了 11 条 Critical 发现（已随本轮提交），所有 Suggestion 级别的条目均暂缓处理。该建议保持开放，并已记录在延后审计中——不会被遗漏。
| token.startsWith('-b=') || | ||
| token === '--builder' || | ||
| token.startsWith('--builder='); | ||
| const claimTokens = claimed.split(/\s+/); |
There was a problem hiding this comment.
[Suggestion] claimScopesItself uses naive whitespace split → scope-flag words inside a quoted -pl selector misread, false unchecked (probe with flip).
中文说明
claimScopesItself 使用朴素空白切分 → 引号内 -pl 选择器中的作用域标志词被误读，产生虚假的 unchecked。
| const claimTokens = claimed.split(/\s+/); | |
| Derive claimScopesItself/claimOnlyPlScoped from a quote-aware token walk. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Deferred to a later round / human follow-up. Critical-only mode is active on this PR (after 5 change-producing rounds), and the previous round ran out of time, so this round fixed only the 11 Critical findings (committed this round) and deferred every Suggestion-level item. This suggestion stays open and is recorded in the deferral audit — nothing was dropped.
中文说明
暂缓至后续轮次 / 人工跟进。 本 PR 已进入 Critical-only 模式（5 个产生改动的轮次之后），且上一轮耗尽了时间预算，因此本轮仅修复了 11 条 Critical 发现（已随本轮提交），所有 Suggestion 级别的条目均暂缓处理。该建议保持开放，并已记录在延后审计中——不会被遗漏。
| const anyMavenRun = [ | ||
| ...(buildTest?.build ?? []), | ||
| ...(buildTest?.test ?? []), | ||
| ].some((c) => mavenLifecycle(c.command.trim()) !== null); |
There was a problem hiding this comment.
[Suggestion] Goal-style Maven claims can never settle, and the 'was not run' note is factually false for the one goal the review itself does run: './mvnw dependency:go-offline' falls through to 'unchecked — this Maven command was not run by this review' even when the warm-up ran exactly that goal (probe-confirmed).
中文说明
目标风格 Maven 声明永远无法结算，且对评审自身确实运行的唯一目标来说"was not run"说明在事实上为假：./mvnw dependency:go-offline 在 warm-up 恰好运行了该目标的同一 reactor 上仍落入 "unchecked — this Maven command was not run by this review"。
| const anyMavenRun = [ | |
| ...(buildTest?.build ?? []), | |
| ...(buildTest?.test ?? []), | |
| ].some((c) => mavenLifecycle(c.command.trim()) !== null); | |
| Test the recorded install command's goal tokens for the claim, or at minimum count the install run in anyMavenRun so a goal claim the warm-up executed reads reproduces or a note naming the warm-up. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Deferred to a later round / human follow-up. Critical-only mode is active on this PR (after 5 change-producing rounds), and the previous round ran out of time, so this round fixed only the 11 Critical findings (committed this round) and deferred every Suggestion-level item. This suggestion stays open and is recorded in the deferral audit — nothing was dropped.
中文说明
暂缓至后续轮次 / 人工跟进。 本 PR 已进入 Critical-only 模式（5 个产生改动的轮次之后），且上一轮耗尽了时间预算，因此本轮仅修复了 11 条 Critical 发现（已随本轮提交），所有 Suggestion 级别的条目均暂缓处理。该建议保持开放，并已记录在延后审计中——不会被遗漏。
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
./mvnw — no such file or directory.
中文说明
./mvnw — no such file or directory。
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| const { adapter, applicable } = selectToolchainAdapter( | ||
| root, | ||
| toolchainAdapters, | ||
| ); |
There was a problem hiding this comment.
[Critical] Mixed root with npm-refused (unmodeled glob) + root pom.xml: the Maven adapter is selected alone and certifies the whole diff green — the unscopable npm half (frontend files) is disclosed nowhere; a PR that breaks the frontend ships green.
中文说明
npm 被拒绝（未建模 glob）+ 根 pom.xml 的混合根目录：Maven adapter 被单独选中并为整个 diff 出具绿色证明——无法定界的 npm 一侧（前端文件）完全未被披露；破坏前端的 PR 会带绿色报告合入。
| const { adapter, applicable } = selectToolchainAdapter( | |
| root, | |
| toolchainAdapters, | |
| ); | |
| When the selected adapter is Maven and a root package.json exists, append a disclosure to the report note ('files outside the Maven reactor were not verified'); for diffs touching only files no adapter scopes, route back to the unsupported handoff. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| it.skipIf(process.platform === 'win32')( | ||
| 'deduplicates symlink-aliased reactor dirs by real path', |
There was a problem hiding this comment.
[Critical] Two new Maven suite tests fail deterministically on macOS (30-level symlink fixture exceeds APFS symlink limit ~16 hops; 600-deep nesting fixture exceeds PATH_MAX 1024 with ENAMETOOLONG); only win32 is skipIf-guarded, so the required macOS merge-queue job goes red.
中文说明
新增的两个 Maven 测试在 macOS 上必然失败：30 层符号链接 fixture 超过 APFS 符号链接解析上限（约 16 跳），600 层嵌套 fixture 超过 PATH_MAX(1024) 在 setup 阶段抛 ENAMETOOLONG。只对 win32 做了 skipIf 守卫，必需的 macOS merge-queue 任务会变红。
| it.skipIf(process.platform === 'win32')( | |
| 'deduplicates symlink-aliased reactor dirs by real path', | |
| Guard both with `it.skipIf(process.platform === 'win32' || process.platform === 'darwin')`, or shrink the symlink chain to <= 15 levels and build the deep chain with short per-level chdir paths. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| if (statSync(pomPath).size > MAX_POM_BYTES) { | ||
| return `Maven POM ${toPosix(relative(reactorRoot, pomPath))} is larger than the ${MAX_POM_BYTES}-byte read cap.`; | ||
| } | ||
| pom = readFileSync(pomPath, 'utf8'); |
There was a problem hiding this comment.
[Critical] MAX_POM_BYTES cap checks size but not isFile(); a symlink-to-/dev/zero POM hangs the review with an infinite read loop (PR-controlled DoS).
中文说明
MAX_POM_BYTES 上限只检查大小不检查 isFile()：指向 /dev/zero 的符号链接 POM 通过上限检查后，readFileSync 永不返回，评审进程被无限读循环挂死（PR 可控 DoS）。
| if (statSync(pomPath).size > MAX_POM_BYTES) { | |
| return `Maven POM ${toPosix(relative(reactorRoot, pomPath))} is larger than the ${MAX_POM_BYTES}-byte read cap.`; | |
| } | |
| pom = readFileSync(pomPath, 'utf8'); | |
| Require a regular file: `if (!st.isFile() || st.size > MAX_POM_BYTES) return ...;` at both read sites (visit and parent-file). |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| if (basename(parentPom) === 'pom.xml') { | ||
| // An absent `pom.xml` target: Maven falls back to repository | ||
| // resolution, so there is no local edge to model. | ||
| continue; | ||
| } |
There was a problem hiding this comment.
[Critical] A parent POM the diff DELETED inside a reactor module's tree never records an inheritance edge or parentPomFiles entry, so the heirs that now fail to resolve are never built — a false-green for a PR-caused Non-resolvable parent POM.
中文说明
被 diff 删除的、位于 reactor 模块目录内的父 POM 从不记录继承边或 parentPomFiles 条目，现在无法解析的继承者永远不会被构建——PR 引起的 Non-resolvable parent POM 得到假绿色。
| if (basename(parentPom) === 'pom.xml') { | |
| // An absent `pom.xml` target: Maven falls back to repository | |
| // resolution, so there is no local edge to model. | |
| continue; | |
| } | |
| When the read of a declared parent fails inside the named/pom.xml-spelled branch, still record the inheritance edge keyed by the declared path so the consumer's named-parent closure adds the heirs. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| const inheritorKey = namedParentFile | ||
| ? toPosix(relative(reactorRoot, parentPom)) | ||
| : parentPath; |
There was a problem hiding this comment.
[Critical] Stale namedParentFile flag mis-keys inheritor edges for directory-spelled (../parent → keyed on parent/pom.xml), so the heir is silently left out of -pl scope; also launders deleted named-parent POM deaths into infrastructure.
中文说明
过期的 namedParentFile 标志导致目录拼写形式的 （如 ../parent）把继承边错误地挂在 parent/pom.xml 上，继承模块被静默排除在 -pl 作用域之外；同时删除的命名父 POM 死亡会被洗白成基础设施问题。
| const inheritorKey = namedParentFile | |
| ? toPosix(relative(reactorRoot, parentPom)) | |
| : parentPath; | |
| Recompute the flag after the append — `const resolvedIsNamedFile = basename(parentPom) !== 'pom.xml';` — and use it for the declaredParentFiles fall-through, namedParentPoms check, and inheritorKey ternary. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| } | ||
| }); | ||
|
|
||
| it('does not settle a claim whose final positional token is not the claimed lifecycle', () => { |
There was a problem hiding this comment.
[Suggestion] The 'final positional token names the last work' guard is defeated by out-of-order phase spellings whose late phase is not in MAVEN_PHASE_RE: 'mvn deploy test' settles reproduces on a test-only run with no disclosure (probe: deploy test → reproduces; test deploy control → unchecked).
中文说明
'最终位置 token 命名最后工作' 守卫被乱序阶段拼写击败，其晚阶段不在 MAVEN_PHASE_RE 中：'mvn deploy test' 在仅 test 的运行上无披露地结算为 reproduces。
| it('does not settle a claim whose final positional token is not the claimed lifecycle', () => { | |
| When the claim's positional tokens contain a later default-lifecycle phase not in MAVEN_PHASE_RE (deploy, integration-test, site), treat the settlement like test deploy (unchecked), or extend MAVEN_PHASE_RE's late phases. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| token === '-b' || | ||
| token.startsWith('-b=') || | ||
| token === '--builder' || | ||
| token.startsWith('--builder='); |
There was a problem hiding this comment.
[Suggestion] scopesNonPl omits -T/--threads → a parallelism-scoped claim settles reproduces on a serial run (probe: claim -T 4 test vs serial run → reproduces).
中文说明
scopesNonPl 漏掉 -T/--threads → 并行度限定的声明在串行运行上结算为 reproduces。
| token === '-b' || | |
| token.startsWith('-b=') || | |
| token === '--builder' || | |
| token.startsWith('--builder='); | |
| Add -T/--threads (and attached forms) to scopesNonPl. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| token === '-b' || | ||
| token.startsWith('-b=') || | ||
| token === '--builder' || | ||
| token.startsWith('--builder='); |
There was a problem hiding this comment.
[Suggestion] scopesNonPl omits -fn/--fail-never → a fail-never claim is settled contradicted on a run whose failure its own semantics would exit 0 (probe with flip).
中文说明
scopesNonPl 漏掉 -fn/--fail-never → fail-never 声明在按其自身语义本应 exit 0 的运行失败上被结算为 contradicted。
| token === '-b' || | |
| token.startsWith('-b=') || | |
| token === '--builder' || | |
| token.startsWith('--builder='); | |
| Add -fn/--fail-never (and consider -o/--offline, -U) to scopesNonPl. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| token.startsWith('-b=') || | ||
| token === '--builder' || | ||
| token.startsWith('--builder='); | ||
| const claimTokens = claimed.split(/\s+/); |
There was a problem hiding this comment.
[Suggestion] claimScopesItself uses naive whitespace split → scope-flag words inside a quoted -pl selector misread, false unchecked (probe with flip).
中文说明
claimScopesItself 使用朴素空白切分 → 引号内 -pl 选择器中的作用域标志词被误读，产生虚假的 unchecked。
| const claimTokens = claimed.split(/\s+/); | |
| Derive claimScopesItself/claimOnlyPlScoped from a quote-aware token walk. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| const anyMavenRun = [ | ||
| ...(buildTest?.build ?? []), | ||
| ...(buildTest?.test ?? []), | ||
| ].some((c) => mavenLifecycle(c.command.trim()) !== null); |
There was a problem hiding this comment.
[Suggestion] Goal-style Maven claims can never settle, and the 'was not run' note is factually false for the one goal the review itself does run: './mvnw dependency:go-offline' falls through to 'unchecked — this Maven command was not run by this review' even when the warm-up ran exactly that goal (probe-confirmed).
中文说明
目标风格 Maven 声明永远无法结算，且对评审自身确实运行的唯一目标来说"was not run"说明在事实上为假：./mvnw dependency:go-offline 在 warm-up 恰好运行了该目标的同一 reactor 上仍落入 "unchecked — this Maven command was not run by this review"。
| const anyMavenRun = [ | |
| ...(buildTest?.build ?? []), | |
| ...(buildTest?.test ?? []), | |
| ].some((c) => mavenLifecycle(c.command.trim()) !== null); | |
| Test the recorded install command's goal tokens for the claim, or at minimum count the install run in anyMavenRun so a goal claim the warm-up executed reads reproduces or a note naming the warm-up. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
./mvnw — no such file or directory.
中文说明
./mvnw — no such file or directory。
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| const { adapter, applicable } = selectToolchainAdapter( | ||
| root, | ||
| toolchainAdapters, | ||
| ); |
There was a problem hiding this comment.
[Critical] Mixed root with npm-refused (unmodeled glob) + root pom.xml: the Maven adapter is selected alone and certifies the whole diff green — the unscopable npm half (frontend files) is disclosed nowhere; a PR that breaks the frontend ships green.
中文说明
npm 被拒绝（未建模 glob）+ 根 pom.xml 的混合根目录：Maven adapter 被单独选中并为整个 diff 出具绿色证明——无法定界的 npm 一侧（前端文件）完全未被披露；破坏前端的 PR 会带绿色报告合入。
| const { adapter, applicable } = selectToolchainAdapter( | |
| root, | |
| toolchainAdapters, | |
| ); | |
| When the selected adapter is Maven and a root package.json exists, append a disclosure to the report note ('files outside the Maven reactor were not verified'); for diffs touching only files no adapter scopes, route back to the unsupported handoff. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| it.skipIf(process.platform === 'win32')( | ||
| 'deduplicates symlink-aliased reactor dirs by real path', |
There was a problem hiding this comment.
[Critical] Two new Maven suite tests fail deterministically on macOS (30-level symlink fixture exceeds APFS symlink limit ~16 hops; 600-deep nesting fixture exceeds PATH_MAX 1024 with ENAMETOOLONG); only win32 is skipIf-guarded, so the required macOS merge-queue job goes red.
中文说明
新增的两个 Maven 测试在 macOS 上必然失败：30 层符号链接 fixture 超过 APFS 符号链接解析上限（约 16 跳），600 层嵌套 fixture 超过 PATH_MAX(1024) 在 setup 阶段抛 ENAMETOOLONG。只对 win32 做了 skipIf 守卫，必需的 macOS merge-queue 任务会变红。
| it.skipIf(process.platform === 'win32')( | |
| 'deduplicates symlink-aliased reactor dirs by real path', | |
| Guard both with `it.skipIf(process.platform === 'win32' || process.platform === 'darwin')`, or shrink the symlink chain to <= 15 levels and build the deep chain with short per-level chdir paths. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| if (statSync(pomPath).size > MAX_POM_BYTES) { | ||
| return `Maven POM ${toPosix(relative(reactorRoot, pomPath))} is larger than the ${MAX_POM_BYTES}-byte read cap.`; | ||
| } | ||
| pom = readFileSync(pomPath, 'utf8'); |
There was a problem hiding this comment.
[Critical] MAX_POM_BYTES cap checks size but not isFile(); a symlink-to-/dev/zero POM hangs the review with an infinite read loop (PR-controlled DoS).
中文说明
MAX_POM_BYTES 上限只检查大小不检查 isFile()：指向 /dev/zero 的符号链接 POM 通过上限检查后，readFileSync 永不返回，评审进程被无限读循环挂死（PR 可控 DoS）。
| if (statSync(pomPath).size > MAX_POM_BYTES) { | |
| return `Maven POM ${toPosix(relative(reactorRoot, pomPath))} is larger than the ${MAX_POM_BYTES}-byte read cap.`; | |
| } | |
| pom = readFileSync(pomPath, 'utf8'); | |
| Require a regular file: `if (!st.isFile() || st.size > MAX_POM_BYTES) return ...;` at both read sites (visit and parent-file). |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| if (basename(parentPom) === 'pom.xml') { | ||
| // An absent `pom.xml` target: Maven falls back to repository | ||
| // resolution, so there is no local edge to model. | ||
| continue; | ||
| } |
There was a problem hiding this comment.
[Critical] A parent POM the diff DELETED inside a reactor module's tree never records an inheritance edge or parentPomFiles entry, so the heirs that now fail to resolve are never built — a false-green for a PR-caused Non-resolvable parent POM.
中文说明
被 diff 删除的、位于 reactor 模块目录内的父 POM 从不记录继承边或 parentPomFiles 条目，现在无法解析的继承者永远不会被构建——PR 引起的 Non-resolvable parent POM 得到假绿色。
| if (basename(parentPom) === 'pom.xml') { | |
| // An absent `pom.xml` target: Maven falls back to repository | |
| // resolution, so there is no local edge to model. | |
| continue; | |
| } | |
| When the read of a declared parent fails inside the named/pom.xml-spelled branch, still record the inheritance edge keyed by the declared path so the consumer's named-parent closure adds the heirs. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| const inheritorKey = namedParentFile | ||
| ? toPosix(relative(reactorRoot, parentPom)) | ||
| : parentPath; |
There was a problem hiding this comment.
[Critical] Stale namedParentFile flag mis-keys inheritor edges for directory-spelled (../parent → keyed on parent/pom.xml), so the heir is silently left out of -pl scope; also launders deleted named-parent POM deaths into infrastructure.
中文说明
过期的 namedParentFile 标志导致目录拼写形式的 （如 ../parent）把继承边错误地挂在 parent/pom.xml 上，继承模块被静默排除在 -pl 作用域之外；同时删除的命名父 POM 死亡会被洗白成基础设施问题。
| const inheritorKey = namedParentFile | |
| ? toPosix(relative(reactorRoot, parentPom)) | |
| : parentPath; | |
| Recompute the flag after the append — `const resolvedIsNamedFile = basename(parentPom) !== 'pom.xml';` — and use it for the declaredParentFiles fall-through, namedParentPoms check, and inheritorKey ternary. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| } | ||
| }); | ||
|
|
||
| it('does not settle a claim whose final positional token is not the claimed lifecycle', () => { |
There was a problem hiding this comment.
[Suggestion] The 'final positional token names the last work' guard is defeated by out-of-order phase spellings whose late phase is not in MAVEN_PHASE_RE: 'mvn deploy test' settles reproduces on a test-only run with no disclosure (probe: deploy test → reproduces; test deploy control → unchecked).
中文说明
'最终位置 token 命名最后工作' 守卫被乱序阶段拼写击败，其晚阶段不在 MAVEN_PHASE_RE 中：'mvn deploy test' 在仅 test 的运行上无披露地结算为 reproduces。
| it('does not settle a claim whose final positional token is not the claimed lifecycle', () => { | |
| When the claim's positional tokens contain a later default-lifecycle phase not in MAVEN_PHASE_RE (deploy, integration-test, site), treat the settlement like test deploy (unchecked), or extend MAVEN_PHASE_RE's late phases. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| token === '-b' || | ||
| token.startsWith('-b=') || | ||
| token === '--builder' || | ||
| token.startsWith('--builder='); |
There was a problem hiding this comment.
[Suggestion] scopesNonPl omits -T/--threads → a parallelism-scoped claim settles reproduces on a serial run (probe: claim -T 4 test vs serial run → reproduces).
中文说明
scopesNonPl 漏掉 -T/--threads → 并行度限定的声明在串行运行上结算为 reproduces。
| token === '-b' || | |
| token.startsWith('-b=') || | |
| token === '--builder' || | |
| token.startsWith('--builder='); | |
| Add -T/--threads (and attached forms) to scopesNonPl. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| token === '-b' || | ||
| token.startsWith('-b=') || | ||
| token === '--builder' || | ||
| token.startsWith('--builder='); |
There was a problem hiding this comment.
[Suggestion] scopesNonPl omits -fn/--fail-never → a fail-never claim is settled contradicted on a run whose failure its own semantics would exit 0 (probe with flip).
中文说明
scopesNonPl 漏掉 -fn/--fail-never → fail-never 声明在按其自身语义本应 exit 0 的运行失败上被结算为 contradicted。
| token === '-b' || | |
| token.startsWith('-b=') || | |
| token === '--builder' || | |
| token.startsWith('--builder='); | |
| Add -fn/--fail-never (and consider -o/--offline, -U) to scopesNonPl. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| token.startsWith('-b=') || | ||
| token === '--builder' || | ||
| token.startsWith('--builder='); | ||
| const claimTokens = claimed.split(/\s+/); |
There was a problem hiding this comment.
[Suggestion] claimScopesItself uses naive whitespace split → scope-flag words inside a quoted -pl selector misread, false unchecked (probe with flip).
中文说明
claimScopesItself 使用朴素空白切分 → 引号内 -pl 选择器中的作用域标志词被误读，产生虚假的 unchecked。
| const claimTokens = claimed.split(/\s+/); | |
| Derive claimScopesItself/claimOnlyPlScoped from a quote-aware token walk. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
| const anyMavenRun = [ | ||
| ...(buildTest?.build ?? []), | ||
| ...(buildTest?.test ?? []), | ||
| ].some((c) => mavenLifecycle(c.command.trim()) !== null); |
There was a problem hiding this comment.
[Suggestion] Goal-style Maven claims can never settle, and the 'was not run' note is factually false for the one goal the review itself does run: './mvnw dependency:go-offline' falls through to 'unchecked — this Maven command was not run by this review' even when the warm-up ran exactly that goal (probe-confirmed).
中文说明
目标风格 Maven 声明永远无法结算，且对评审自身确实运行的唯一目标来说"was not run"说明在事实上为假：./mvnw dependency:go-offline 在 warm-up 恰好运行了该目标的同一 reactor 上仍落入 "unchecked — this Maven command was not run by this review"。
| const anyMavenRun = [ | |
| ...(buildTest?.build ?? []), | |
| ...(buildTest?.test ?? []), | |
| ].some((c) => mavenLifecycle(c.command.trim()) !== null); | |
| Test the recorded install command's goal tokens for the claim, or at minimum count the install run in anyMavenRun so a goal claim the warm-up executed reads reproduces or a note naming the warm-up. |
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
|
🤖 AutoFix ran out of time before finishing (timeout (7200000ms)) (attempt 10/100) — it will retry on the next scan. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31267968791 🧠 Handled by Qwen Code · model/模型 |
…nt defects (QwenLM#8394) Eleven Critical findings from the latest review round: Evidence integrity (false-green shapes): - Fail closed when fresh Surefire/Failsafe reports exceed the parse cap: unparsed reports carry unknown failure status, so ok can no longer be certified (was: disclose-only, a fail-never exit-0 green over failed runs). - Treat Maven's `[ERROR] Failed to execute goal` framing as a swallowed failure under fail-never — checkstyle/enforcer/spotless goal deaths matched none of the compile/dependency/launch classes and read green. Kept out of SOURCE_FAILURE_LINE_RE so dependency-framed goal failures keep their acquisition carve-out; also rescued from the output trim. - DEPENDENCY_FAILURE_LINE_RE now covers `Non-resolvable import POM`, `Failure to find`, and `Could not find artifact`. POM reads (DoS + deleted-parent scoping): - Require isFile() — not just the size cap — at every MAX_POM_BYTES read site (reactor walk, parent-file read, .mvn/maven.config): a symlink to /dev/zero or a FIFO passed the size check and hung readFileSync. - Record inheritance edges for ABSENT declared parents (the shape a deleting diff leaves), keyed on the paths git reports, so the heirs of a Non-resolvable parent POM join the changed-parent closure instead of shipping the death green. - Recompute the named-parent flag after the directory->pom.xml append: a directory-spelled <relativePath> used to key its heir edge on `dir/pom.xml` (where the closure never looks) and skipped the declared-file recording. Runner and selection honesty: - mavenExecutable falls back to system mvn for an EMPTY wrapper (it passed the existence/exec-bit gates and exited 0 over a build that never started). - Mixed roots (Maven selected while a root package.json exists) disclose on every report that files outside the Maven reactor were NOT verified. Claim settlement: - A Maven run interrupted with fresh recorded failures now out-ranks a green finished sibling; the finished-run ranking used to read `reproduces` while the build-test report said ok:false. Tests: skip the 30-hop symlink and 600-deep nesting fixtures on darwin (APFS symlink limit, PATH_MAX 1024) — both failed deterministically on the macOS merge-queue job — and add focused gates for every fix above.
|
🤖 AutoFix stopped: this counting window now contains 3 time-budget exhaustions (pushed rounds in between included; this round itself may have failed differently). That is 3 full agent runs that pushed nothing. A human should split or reduce the PR (or raise the agent time budget AND its step backstop together), then comment
See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31275438474 🧠 Handled by Qwen Code · model/模型 |
|
⏸️ Takeover paused: this PR reached its round cap (100/100). Comment 中文说明⏸️ 托管已暂停：本 PR 达到轮次上限（100/100）。评论 |
|
@qwen-code /takeover stop |
qwen-code-dev-bot
removed
the
autofix/takeover
|
👋 Takeover released: the autofix loop will no longer engage this PR (an in-flight round, if any, completes its bounded work). Re-apply 中文说明👋 已释放：autofix 循环不再介入此 PR（在飞的一轮如有，将完成其有界工作）。重新打上 |
|
Closing in favour of a split, rebased pair. This PR accumulated 55 issue comments, 132 reviews, and 514 inline comments across 11 review rounds, which made the thread impractical to work from. Replaced by:
What changed beyond the split. The static POM parser is gone. #8394 read the reactor out of the POM text — literal Maven now answers those questions itself. Ownership is a nearest-ancestor Review feedback already addressed here is carried into the new PRs. Apologies for the churn on the thread. |
3 participants
What this PR does
This PR adds deterministic Maven multi-module verification to
/reviewon top of the internal toolchain adapter boundary introduced by the first commit.review build-testnow recognizes root Maven reactors, maps changed files to the deepest default-reactor module, prefers the root Maven Wrapper, runs scopedtestor build-onlytest-compilecommands from the reactor root with Maven upstream/downstream expansion, and emits fresh module-qualified Surefire/Failsafe evidence. Successful Maven build-only results are accepted by base-tree preparation, Test Plan checks understand Maven commands and report counts, and Agent 7 handles Maven infrastructure failures without routing them through the npm-only test-delta path.The implementation fails closed for ambiguous npm+Maven roots, unsafe aggregation paths, profile/plugin module declarations, and standalone or profile-inactive Maven projects outside the root reactor. It does not claim full Maven effective-model or CI-matrix parity.
Why it's needed
The existing
/reviewbuild-and-test path 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, downstream-consumer selection, timeout classification, or fresh test-report evidence. This work is driven by active use in Alibaba Fastjson2 and Druid: both are real multi-module Maven reactors where core changes must be checked through the root wrapper and where nested, profile-activated, and standalone POMs make naive POM scanning unsafe.Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review/build-test.test.ts src/commands/review/base-tree.test.ts src/commands/review/test-plan.test.ts src/commands/review/agent-prompt.test.ts src/commands/review/lib/npm-toolchain.test.ts src/commands/review/lib/maven-toolchain.test.ts; expect all 326 tests to pass../mvnw --batch-mode --no-transfer-progress -pl core -am -amd test, while root Maven configuration changes run the full reactor and build-only mode usestest-compile.<module>elements are excluded from default-reactor discovery, nested direct modules are discovered recursively, and a changed standalone or profile-inactive Maven project returnstoolchain: "unsupported"without running a root command, including a profile-only child project nested under an active reactor module../mvnw testclaim to be settled by the scoped command actually run, but does not equate claims carrying different-Pprofiles or-plscopes.coremaps to the root wrapper command, and Druid's standalonedruid-adminproject fails closed.Evidence (Before & After)
Before: the installed baseline returned
toolchain: "unsupported"for a two-module Maven fixture and did not execute its wrapper.After: the current implementation returned
toolchain: "maven", selectedapp, and executed the fixture wrapper from the reactor root with-pl app -am -amd test. Read-only acceptance smoke checks parsed the default Fastjson2 and Druid reactors and generated the expected root-wrapper command for each repository'scoremodule.Tested on
Environment (optional)
macOS, Node.js 22.22.2. Focused result: 6 test files passed, 326 tests passed. Prettier, ESLint, commit hooks, and
git diff --checkpassed. A workspace-owned fake-wrapper fixture exercised the realrunBuildTestexecution path without network access. Fastjson2, Druid, Trino, and Flink were used for read-only reactor parsing and injected command-generation acceptance checks; their real Maven suites were not executed. The Flink smoke check exposed a profile-only child project nested under an active reactor module; a reproduce-first fixture confirmed the parent-only command could pass without compiling broken child source, and the adapter now fails closed before running Maven. The source CLI entry was blocked by an unrelated unbuilt ACP Bridge dist file. Repository-widenpm run buildand CLInpm run typecheckwere attempted but remain blocked by unrelated existing dependency and cross-package type drift, including the missing@octokit/restdependency and existing ACP/Ink API mismatches; no reported error referenced the files changed by this PR.Risk & Scope
<project>/<modules>/<module>aggregation. Maven remains responsible for dependency ordering and-am/-amdexpansion; the report does not claim a recursively computed complete dependency graph.verifyexecution, JaCoCo, mutation testing, Maven-aware base-side test-delta, real network-dependent Fastjson2/Druid builds, and mixed-toolchain aggregation.BuildTestReport.toolchainadds the"maven"discriminant. Existing npm CLI behavior and report fields remain compatible; roots where both npm and Maven apply now fail closed instead of choosing one implicitly.Linked Issues
N/A
中文说明
此 PR 的改动
此 PR 在首个 commit 引入的内部 toolchain adapter 边界之上，为
/review增加确定性的 Maven 多模块验证。review build-test现在可以识别根 Maven reactor，将变更文件映射到默认 reactor 中最深的模块，优先使用根 Maven Wrapper，从 reactor 根目录执行带 Maven 上游/下游扩展的 scopedtest或仅构建test-compile命令，并输出仅属于本次执行的、带模块路径的 Surefire/Failsafe 证据。base-tree 可以接受成功的 Maven build-only 结果，Test Plan 可以识别 Maven 命令和测试计数，Agent 7 会正确处理 Maven 基础设施失败，同时不会把 Maven 失败送入仅支持 npm 的 test-delta 路径。对于 npm 与 Maven 同时适用的根目录、不安全的聚合路径、profile/plugin 中的 module 声明，以及不属于根 reactor 的独立或当前 profile 未激活 Maven 项目，实现都会 fail closed。此 PR 不声称实现完整 Maven effective model 或完整 CI matrix 对齐。
为什么需要
现有
/reviewbuild-and-test 路径能为 npm 项目生成确定性证据，但 Maven 项目只能交还给 agent 临时选择命令。这使 Java 验证不一致，也缺少可靠的模块归属、下游消费者选择、超时分类和新鲜测试报告证据。本工作由 Alibaba Fastjson2 和 Druid 的实际使用驱动：两者都是真实 Maven 多模块 reactor，core 变更必须通过根 wrapper 验证，而嵌套模块、profile 激活模块和独立 POM 使简单扫描所有 POM 的方案不安全。Reviewer Test Plan
验证方式
cd packages/cli && npx vitest run src/commands/review/build-test.test.ts src/commands/review/base-tree.test.ts src/commands/review/test-plan.test.ts src/commands/review/agent-prompt.test.ts src/commands/review/lib/npm-toolchain.test.ts src/commands/review/lib/maven-toolchain.test.ts，预期 326 个测试全部通过。./mvnw --batch-mode --no-transfer-progress -pl core -am -amd test；根 Maven 配置变更运行完整 reactor，build-only 模式使用test-compile。<module>不会进入默认 reactor，嵌套的直接 module 会递归发现，而变更独立或 profile 未激活 Maven 项目时返回toolchain: "unsupported"，且不会执行根命令，包括嵌套在活跃 reactor module 目录下的 profile-only 子项目。./mvnw test可以由实际执行的 scoped 命令结算，但带不同-Pprofile 或-plscope 的命令不会被错误视为同一次执行。core映射到根 wrapper 命令，Druid 的独立druid-admin项目正确 fail closed。证据（改动前后）
改动前：已安装的基线版本对双模块 Maven fixture 返回
toolchain: "unsupported"，且没有执行 wrapper。改动后：当前实现返回
toolchain: "maven"，选择app，并从 reactor 根目录执行 fixture wrapper，参数为-pl app -am -amd test。只读验收 smoke 检查成功解析 Fastjson2 和 Druid 的默认 reactor，并为两个仓库的core模块生成预期的根 wrapper 命令。测试平台
环境（可选）
macOS，Node.js 22.22.2。聚焦验证结果：6 个测试文件通过，326 个测试通过；Prettier、ESLint、commit hooks 和
git diff --check通过。workspace 内的假 wrapper fixture 在不访问网络的情况下执行了真实runBuildTest路径。Fastjson2、Druid、Trino 和 Flink 用于只读 reactor 解析及注入执行器的命令生成验收，未实际运行其 Maven 测试套件。Flink smoke 检查发现了嵌套在活跃 reactor module 下的 profile-only 子项目；reproduce-first fixture 证明父模块命令可以在完全未编译损坏子项目源码的情况下成功，adapter 现在会在执行 Maven 前 fail closed。源码 CLI 入口被无关的 ACP Bridge dist 未构建问题阻断。已尝试运行全仓npm run build和 CLInpm run typecheck，但仍被无关的既有依赖和跨包类型漂移阻断，包括缺少@octokit/rest和现有 ACP/Ink API 不匹配；错误输出中没有指向此 PR 修改的文件。风险与范围
<project>/<modules>/<module>聚合。依赖顺序和-am/-amd扩展仍由 Maven 负责；report 不声称已递归计算完整依赖图。verify执行、JaCoCo、mutation testing、Maven-aware base-side test-delta、依赖网络的 Fastjson2/Druid 真实构建，以及混合 toolchain 聚合。BuildTestReport.toolchain新增"maven"判别值。现有 npm CLI 行为和 report 字段保持兼容；根目录同时适用 npm 与 Maven 时现在会 fail closed，而不是隐式选择一个。关联 Issue
N/A