fix(autofix): resolve owning package for nested paths; report verify-failed handoffs as not pushed - #7330
Conversation
…failed handoffs as not pushed The verify gate mapped each changed file to a flat `packages/<dir>` and read `<dir>/package.json`, which ENOENT-crashed on nested packages such as packages/channels/base — the container packages/channels has no package.json. Walk each changed file up to its nearest package.json in both the issue-fix and review-address verify steps, and skip any candidate that still has none. When such a verify failure follows an agent commit, the review-address handoff rendered the agent's optimistic address-summary.md (which can cite a commit SHA) under a neutral "what I found" heading, so a maintainer chased a commit that was discarded with the runner workspace. An EXIT trap now records any post-commit non-zero exit as outcome=failed, and the handoff states plainly that the change did NOT pass the gate and was NOT pushed. Tests: walk-up detection over a nested package tree, the outcome=failed trap, and the not-pushed handoff wording — each mutation-verified.
|
Thanks for the PR! Template looks good ✓ Problem: observed bug with clear evidence — the verify gate ENOENT-crashed on nested packages ( Direction: CI infrastructure fix for the autofix verification gate — squarely within scope. No auth/sandbox/model/public-contract concerns. Size: not applicable — no core module paths touched. All changes are in Approach: the scope feels right — two focused fixes that both directly address the observed failures. The resolver reads on-disk workspaces globs (not Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:已观测到的 bug,有明确证据——验证门在嵌套包上 ENOENT 崩溃( 方向:autofix 验证门的 CI 基础设施修复——完全在范围内。无 auth/sandbox/模型/公共契约方面的顾虑。 规模:不适用——未触及核心模块路径。所有改动在 方案:范围合理——两个聚焦修复都直接针对已观测到的故障。解析器读取磁盘上的 workspaces globs(非 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
ReviewBoth defects are real and worth fixing, and the verbatim-extraction test style is the right technique for a workflow-only change. I re-ran Three issues, one of which I think blocks. 1. Walk-up stops at any
|
|
Code review — independent proposal first: I'd replace the flat Reading the diff against that baseline:
No critical blockers. No AGENTS.md violations. Real-scenario testing — this is a CI/workflow-only PR (no user-facing CLI behavior), so the functional test is exercising the resolver against the real repo layout and running the unit tests: Resolver output on the real repo: 中文说明代码审查——先独立提出方案:我会用展开 root 对照该基线阅读 diff:
无关键阻塞项。无 AGENTS.md 违规。 真实场景测试——这是纯 CI/workflow PR(无用户可见 CLI 行为),功能测试为对真实仓库布局运行解析器并执行单元测试。解析器在真实仓库上输出正确:嵌套 workspace、fixture 归属、排除项均符合预期。所有单元测试通过(63/63 + 14/14)。 — Qwen Code · qwen3.7-max Reviewed at |
|
Confidence: 5/5 — clean across every stage; would merge without hesitation. This PR fixes two concrete, observed failures in the autofix verify gate — the nested-package ENOENT crash and the misleading handoff — with a minimal, well-tested solution. The resolver's design choices are all justified: on-disk globs (not node_modules) so branch-added workspaces are visible, longest-prefix matching so fixtures don't steal ownership, shared staging so the two gates can't drift, and a loud failure on an empty workspace set rather than a silent skip. The committed flag's exit-code discrimination (1 vs 128) is the kind of careful detail that prevents the next "chasing a phantom commit" confusion. My independent proposal matched the PR's approach — I didn't find a simpler path. Every change in the diff serves the stated goal; no drive-by refactors or scope creep. The tests are thorough and the resolver works correctly against the real repo layout. If I had to maintain this in six months, the comments and structure would make it straightforward. 中文说明置信度:5/5——每个阶段都干净;毫不犹豫可以合并。 本 PR 用最小化、充分测试的方案修复了 autofix 验证门中两个具体的、已观测到的故障——嵌套包 ENOENT 崩溃和误导性 handoff。解析器的设计选择都有充分理由:磁盘 globs(非 node_modules)使分支新增 workspace 可见,最长前缀匹配使 fixture 不窃取归属,共享暂存使两个门不会漂移,空 workspace 集合大声失败而非静默跳过。committed 标志的退出码判别(1 vs 128)正是防止下一次"追幻影 commit"困惑的细致之处。 我的独立方案与 PR 一致——没找到更简路径。diff 中每个改动都服务于既定目标;无顺手重构或范围蔓延。测试全面,解析器在真实仓库布局上工作正确。 — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.8-max-preview via Qwen Code /review
… script Addresses review on #7330. Extract the changed-file → owning-package walk into .github/scripts/resolve-owning-packages.sh, staged to RUNNER_TEMP from the trusted base alongside check-settings-schema.sh and invoked from both verify gates, so the two gates cannot drift into resolving packages differently (the 8-line walk was otherwise duplicated verbatim in each). Updates the package-scripts test that pinned the old inline grep. Narrow the verify-failed handoff lead-in to "This change was NOT pushed": four paths set outcome=failed BEFORE the deterministic gate runs (agent abort via failure.md, dirty tree, unchanged branch, missing address-summary.md), so the previous "did NOT pass the verification gate" claim was factually wrong for them. The specific reason stays in the headline and the quoted summary.
The repo's shellcheck gate runs --enable=all --severity=style, under which
bare $f/$d references trip SC2250 (prefer ${var}). Brace them to match the
convention already used in check-settings-schema.sh, and update the script
content assertions accordingly. Verified with shellcheck 0.11.0 using the
exact CI flags: clean.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
…ndoff on commit existence Addresses the deeper review on #7330. Blocking issue: the "nearest package.json" resolver mapped a change under a workspace's fixture/example package (e.g. packages/cli/src/commands/extensions/examples/starter) to that fixture, whose test script is not Vitest — silently SKIPPING packages/cli's own tests, a coverage regression invisible in the log. Resolve against the authoritative `npm query .workspace` set instead and take each file's longest-prefix workspace: nested workspaces (packages/channels/base) match exactly, fixtures and non-workspace paths (packages/sdk-python, packages/README.md, the excluded packages/desktop) drop. Also harden the resolver against a final line with no trailing newline and against an unmatched last line, which under `set -o pipefail` would otherwise abort the script. Handoff wording: keying "was NOT pushed / commit discarded" on outcome=failed was wrong for the abort paths (failure.md, dirty tree, unchanged branch, missing address-summary.md), which set outcome=failed before ever making a commit. Record committed=true right after checkout — before any gate can fail — and key the wording on that; the abort/no-op paths keep the neutral framing. This removes the EXIT trap entirely (its only observable effect was that wording), so it no longer mislabels pre-commit failures either.
|
Thanks — this is an excellent catch on #1; all three are addressed in 3a0cfa8. #1 (blocking) — resolve against the npm workspace set, not "any package.json". Two robustness bugs surfaced while writing that test, both fixed in the script: a final changed-path with no trailing newline was dropped ( #2 + #3 — key the wording on commit-existence, and the trap is gone. The gate now records Smaller notes.
|
Re-review (3a0cfa8)All three previous findings are genuinely fixed — verified, not just read:
One new issue that I think matters, plus smaller ones. A.
|
…tested; harden resolver Addresses the re-review on #7330. The resolver sourced its workspace set from `npm query .workspace`, which reads node_modules — installed from the BASE checkout. A workspace the PR branch ADDS (a new channel adapter, a new sdk — the issue-fix job's whole purpose) was invisible, so its tests were silently skipped, and for a nested new package the ENOENT crash this PR fixes turned into a silent skip. Expand the set from the on-disk root package.json `workspaces` globs instead (shallow `dir/*` + literals, honouring `!` negations, keeping dirs with a package.json): it reflects the branch, matches what `npm run --workspace` accepts downstream, and needs no install. Verified to reproduce `npm query`'s set exactly on the current tree. Also from the review: - Fail the gate loudly on an empty/unreadable workspace set instead of the silent "no package changes" skip, and drop the now-unneeded `|| true` at both resolver call sites (the resolver already exits 0 on legitimate no-match). - Record committed=true at the TOP of the step (ref-only diff), covering an agent that commits then aborts, and count only `git diff --quiet` exit 1 as a commit (128 is a git error, not a discarded commit). - Correct the two call-site comments that still described the superseded nearest-package.json approach. Also hardens the resolver against a final changed-path with no trailing newline and an unmatched last line under `set -o pipefail`.
|
Sharp catch on A — that one would have quietly re-opened the exact hole. All fixed in 1de8989. A — resolve from the on-disk manifest, not B — loud on empty, C — comments corrected. Both Smaller notes:
Also hardened the resolver against a final changed-path with no trailing newline and an unmatched last line under Local re-verification with the exact CI toolchain: shellcheck 0.11.0 ( |
Re-review (1de8989) — LGTMEverything from the last round is addressed, and I verified the substantive parts rather than reading them. A — on-disk workspace expansion. The replacement is exactly equivalent to what it replaced on the current manifest, which is the property that matters for not regressing: …and it now also resolves branch-added workspaces, which was the bug. B — loud failure. C — both call-site comments now describe the actual algorithm. Minor notes — both fixed, and the risk I was implicitly asking you to take on the 81/81 pass locally; Optional polish (non-blocking)
Nothing here blocks. Nice iteration — the resolver ended up more correct than the |
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.8-max-preview via Qwen Code /review
| # Map each changed file to its OWNING package — the nearest ancestor | ||
| # with a package.json — via the trusted staged resolver, shared with |
There was a problem hiding this comment.
[Suggestion] The comment describes the resolver as "the nearest ancestor with a package.json" — the exact algorithm the resolver deliberately does NOT do. The resolver script's own header (resolve-owning-packages.sh lines 10–14) explicitly states the workspace set is npm query .workspace, NOT "any ancestor directory that has a package.json". — Concrete cost: a future maintainer debugging fixture-package resolution reads this comment, concludes the resolver should do nearest-package.json walking, and "fixes" it — reintroducing the fixture-shadowing bug where packages/cli/src/commands/extensions/examples/starter's package.json shadows packages/cli's tests.
| # Map each changed file to its OWNING package — the nearest ancestor | |
| # with a package.json — via the trusted staged resolver, shared with | |
| # Map each changed file to its OWNING package — the longest-prefix | |
| # npm workspace (from `npm query .workspace`) — via the trusted staged resolver, shared with |
— qwen3.8-max-preview via Qwen Code /review
| # Map each changed file to its OWNING package — the nearest ancestor | ||
| # with a package.json — via the trusted staged resolver, shared with |
There was a problem hiding this comment.
[Suggestion] Same misleading comment as the issue-fix verify gate (~line 1039): "the nearest ancestor with a package.json" describes the algorithm the resolver was written to replace. See the resolver script's header for the correct description. — Concrete cost: same misdirection risk, doubled across both call sites.
| # Map each changed file to its OWNING package — the nearest ancestor | |
| # with a package.json — via the trusted staged resolver, shared with | |
| # Map each changed file to its OWNING package — the longest-prefix | |
| # npm workspace (from `npm query .workspace`) — via the trusted staged resolver, shared with |
— qwen3.8-max-preview via Qwen Code /review
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: six-months-later-maintainer — Agent 6c subagent execution failed. Not reviewed: build-and-test — Agent 7 infrastructure failure (worktree inaccessible during review). Not reviewed: coverage — could not read the agents' transcripts (no subagent transcripts at /home/github-runner/actions-runner-22/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-22--work-qwen-code-qwen-code/subagents/c32eeb27-1b8d-42e5-8542-f31885cdd009 (ENOENT: no such file or directory, scandir '/home/github-runner/actions-runner-22/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-22--work-qwen-code-qwen-code/subagents/c32eeb27-1b8d-42e5-8542-f31885cdd009'). The harness writes one per agent; if there are none, either no agents ran or the harness could not write them.), so this run cannot show that any of the diff was read. Not reviewed: verification — could not check that Step 4 and Step 5 ran (no subagent transcripts at /home/github-runner/actions-runner-22/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-22--work-qwen-code-qwen-code/subagents/c32eeb27-1b8d-42e5-8542-f31885cdd009 (ENOENT: no such file or directory, scandir '/home/github-runner/actions-runner-22/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-22--work-qwen-code-qwen-code/subagents/c32eeb27-1b8d-42e5-8542-f31885cdd009'). The harness writes one per agent; if there are none, either no agents ran or the harness could not write them.).
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
ytahdn
left a comment
There was a problem hiding this comment.
LGTM. 两个 autofix verification gate 缺陷修复均正确。
嵌套包解析:resolve-owning-packages.sh 从磁盘 workspaces globs 展开真实 workspace 集合(非 npm query),最长前缀匹配 == ""/* 正确处理嵌套包,set -euo pipefail + if 形式避免 pipefail 下 no-match 退出,空集合 exit 1 大声失败。两个 verify gate 均从 trusted base checkout 暂存脚本,防止 PR 分支篡改。
commit 状态记录:|| committed_rc=$? 防止 set -e 触发,仅 exit 1(真实 diff)设置 committed=true,exit 0/128 均不设置。Handoff 措辞基于 COMMITTED(非 OUTCOME)正确区分 committed-but-unpushed vs abort/no-op。
测试覆盖 5 个新用例,边界条件处理到位。
— qwen3.7-plus via Qwen Code /review
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
* feat(autofix): raise the strict round cap from 5 to 10 Measured across the last 40 bot-authored PRs: 17 finished at round 0, 12 at 1, 4 at 2, 2 at 3, 1 at 4, and 3 reached the cap of 5. All three that reached it merged AT it rather than stalling — and one of those spent two of its five rounds on the verify-gate ENOENT that QwenLM#7330 has since fixed. So the ceiling was never the thing that stopped a PR, but it sat close enough to bind on a bad day with no headroom. 10 gives that headroom. The cap exists to stop an unproductive LOOP, not to ration ordinary iteration; a genuinely stuck PR still stops, just later. Deliberately not larger: retries for a transient model or gate failure increment the same counter, so the cap also bounds how much one bad provider window can spend. API_AUTH_MAX_ROUNDS stays at 3 and still short-circuits the errors only a maintainer can fix. Replaces the literal `MAX_ROUNDS: '5'` assertion with the ordering the numbers must satisfy — auth cap < strict cap < takeover cap — so the values stay tunable and a cap that stops binding fails instead. * fix(scripts): anchor round-cap regex with word boundary (QwenLM#7412) --------- Co-authored-by: wenshao <wenshao@example.com> Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com>
|
Released in v0.20.1. |
What this PR does
Fixes two defects in the autofix verification gate — the per-package test re-run and the failure handoff — that together stranded and misreported #7246.
1. Resolve each changed file to its owning npm workspace. The gate mapped every changed file to a flat
packages/<dir>and read<dir>/package.json. That ENOENT-crashed on nested packages (a change underpackages/channels/basemapped to the containerpackages/channels, which has nopackage.json), taking down the whole verify step. The first fix walked up to the nearestpackage.json— but "has apackage.json" ≠ "is a workspace": a change under a fixture package inside a workspace's src tree (e.g.packages/cli/src/commands/extensions/examples/starter) would resolve to the fixture, whose test script is not Vitest, and silently skippackages/cli's own tests. So the gate now resolves against the real workspace set — expanded from the on-disk rootpackage.jsonworkspacesglobs (shallowdir/*+ literals, honouring!negations, keeping dirs with apackage.json) — and takes each file's longest-prefix workspace: nested workspaces (packages/channels/base) match exactly, and fixtures / non-workspace paths (packages/sdk-python, a top-levelpackages/README.md, the excludedpackages/desktop) match nothing and are dropped. Reading the globs on disk rather thannpm query/node_modules matters because node_modules reflects the base checkout — a workspace the branch adds (a new channel adapter, a new sdk, which is the issue-fix job's whole point) would otherwise be invisible and its tests silently skipped. The logic lives in a shared script.github/scripts/resolve-owning-packages.sh, staged toRUNNER_TEMPfrom the trusted base alongsidecheck-settings-schema.shand invoked from both verify gates (with no|| true, so a resolver error fails the gate loudly), so the two cannot drift.2. Only call a change "not pushed" when a commit actually existed. When a verify failure follows an agent commit, the review-address handoff rendered the agent's own
address-summary.md— which reads like a success and can cite a commit SHA — under a neutral "What I found before stopping:" heading, so a maintainer chased a commit that was discarded with the runner (the "no commit record" confusion on #7246). The gate now recordscommitted=trueat the top of the step (a ref-only diff), before the failure.md early-exits, and the handoff keys its "was NOT pushed — any commit referenced below … has been discarded" wording on that, not onoutcome=failed. The abort / no-op paths (failure.md, dirty tree, unchanged branch, missingaddress-summary.md) setoutcome=failedbefore ever committing, so they correctly keep the neutral framing.Why it's needed
The repo's
packages/channels/*layout meant a single nested-package review comment crashed the gate; the "nearest package.json" heuristic would have turned that crash into a silent test-coverage hole around fixture packages; and the resulting handoff pointed the maintainer at a discarded commit. This makes the gate resolve the repo's real workspace set and makes the failure report honest.Reviewer Test Plan
How to verify
npx vitest run scripts/tests/qwen-autofix-workflow.test.js— 67/67. Key coverage:workspacesglobs + members, nonpm install) including a branch-added top-level and nested workspace, a fixturepackage.jsoninside a workspace's src tree, and a!-excluded workspace; asserts the branch-added packages resolve, the fixture change resolves topackages/cli(never the fixture), and the excluded/non-workspace paths drop. A separate test asserts the resolver fails loudly (non-zero + message) on an empty workspace set.git, records the flag only whengit diff --quietexits 1 (a real diff) — not on 0 (no commit) or 128 (a git error).COMMITTED— "was NOT pushed" when a commit existed, neutral "What I found before stopping" otherwise. Neither verify gate carries an EXIT trap.cpand invokeresolve-owning-packages.shfromRUNNER_TEMP(mirrors the schema-gate staging assertions).npx vitest run scripts/tests/package-scripts.test.js— the verify step invokes the resolver.--enable=all --severity=styleclean on the script; actionlint 1.7.12 clean on the workflow;js-yamlparses; bothverifysteps passbash -n.Evidence (Before & After)
Tested on
Environment (optional)
Unit tests only — no
npm install; the resolver reads the on-disk manifest directly.Risk & Scope
package.jsonworkspaces globs (no install needed) — verified to reproducenpm query .workspace's set exactly on the current tree. It is hardened against a final changed-path with no trailing newline and an unmatched last line underset -o pipefail, and fails loudly on an empty set. A workflow-only PR (nopackages/changes) still skips package tests as before.Linked Issues
Fixes the verification-gate crash and misleading handoff observed on #7246. Same "make the managed-PR loop fail honestly" line as #7229 (crash), #7247 (model API retry).
中文说明
本 PR 做了什么
修复 autofix 验证门里的两个缺陷(逐包测试重跑 + 失败 handoff),它们共同导致 #7246 先搁置、又被误报。
1. 把每个改动文件解析到其归属的 npm workspace。 门原先把改动文件映射成扁平的
packages/<dir>再读<dir>/package.json,对嵌套包会 ENOENT 崩溃(packages/channels/base的改动映射到无package.json的容器packages/channels)。第一版改成"向上走到最近的package.json",但"有package.json"≠"是 workspace":改动落在某 workspace 源码树内的 fixture 包(如packages/cli/src/commands/extensions/examples/starter)会解析到该 fixture(其 test 脚本非 Vitest),从而静默跳过packages/cli自己的测试。所以现在门解析真实 workspace 集合——从磁盘上 rootpackage.json的workspacesglobs 展开(浅层dir/*+ 字面量,遵守!排除,保留含package.json的目录)——取每个文件的最长前缀 workspace:嵌套 workspace(packages/channels/base)精确匹配,fixture / 非-workspace 路径(packages/sdk-python、顶层packages/README.md、被排除的packages/desktop)不匹配即丢弃。读磁盘 globs 而非npm query/node_modules 很关键:node_modules 反映的是基座 checkout,分支新增的 workspace(新 channel adapter、新 sdk,正是 issue-fix job 的目的)否则不可见、测试被静默跳过。逻辑在共享脚本.github/scripts/resolve-owning-packages.sh,从可信基座 stage 到RUNNER_TEMP、两个门调用(无|| true,解析器出错即让门大声失败),不会漂移。2. 只有确有 commit 才称"未被推送"。 验证失败发生在 agent 提交之后时,review-address 的 handoff 会把 agent 的
address-summary.md(读起来像成功、可能引用 commit SHA)放在中性的 "What I found before stopping:" 标题下,维护者去追一个随 runner 丢弃的 commit(#7246 的"看不到提交记录")。现在门在 step 顶部(ref-only diff)、failure.md 早退之前记录committed=true,handoff 的"was NOT pushed …"措辞键控它、而非outcome=failed。中止 / no-op 路径(failure.md、脏树、分支无改动、缺address-summary.md)在提交前就置了outcome=failed,因此正确保留中性措辞。为什么需要
仓库的
packages/channels/*结构使一条嵌套包 review 评论就能崩门;"最近 package.json"启发式又会把崩溃变成 fixture 包周围的静默测试覆盖漏洞;随后的 handoff 还把维护者指向被丢弃的 commit。本 PR 让门解析真实 workspace 集合,并让失败报告诚实。评审验证
npx vitest run scripts/tests/qwen-autofix-workflow.test.js—— 67/67:解析器构建 workspace 树(root workspaces globs + 成员,免npm install),含分支新增的顶层与嵌套 workspace、workspace 源码树内的 fixture、!排除的 workspace;断言分支新增包被解析、fixture 改动解析到packages/cli、排除/非-workspace 路径丢弃;另有测试断言空 workspace 集合时大声失败(非零 + 消息);committed片段用 PATH-stubgit驱动 0/1/128,仅退出 1 记为 commit;handoff 键控COMMITTED;两个门都无 EXIT trap。npx vitest run scripts/tests/package-scripts.test.js—— verify 步骤调用解析器。--enable=all --severity=styleclean;actionlint 1.7.12 clean;YAML 可解析;两个verify步骤过bash -n。风险与范围
package.json的 workspaces globs(免安装)——已验证在当前树上逐字复现npm query .workspace的集合。对"末行无换行"与"末行无匹配"(set -o pipefail下会中止脚本)做了加固,空集合时大声失败。仅改 workflow(无packages/改动)的 PR 仍如常跳过包测试。关联 Issue
修复在 #7246 上观察到的验证门崩溃与误导性 handoff。与 #7229(崩溃)、#7247(模型 API 重试)同一"让被托管 PR 循环诚实失败"主线。