fix(autofix): stage SKILL.md beside run-agent.mjs so review-address boots (P0, regression from #7165) - #7225
Merged
Merged
Conversation
…oots P0 regression from #7165. The review-address agent invokes a trusted staged copy of run-agent.mjs to avoid executing the PR branch's version on the host with the model key. run-agent.mjs resolves its instructions as `<own dir>/../SKILL.md`, but the staging was flat (${RUNNER_TEMP}/run-agent.mjs), so ../SKILL.md pointed at ${RUNNER_TEMP}/../SKILL.md = /home/runner/work/SKILL.md — which does not exist. Every review-address run since #7165 merged crashed with ENOENT before reading any feedback: the entire feedback-addressing and takeover path was down on main. Fix: stage the runner AND its SKILL in a mirrored layout (${RUNNER_TEMP}/autofix-skill/{SKILL.md,scripts/run-agent.mjs}) and invoke the staged runner from there, so ../SKILL.md resolves to the staged SKILL. This also closes a latent gap — the model's instructions now come from the trusted base too, never the checked-out PR branch. Regression guard: the suite pinned the cp string and the invocation but never checked SKILL.md was resolvable from the staged location. The test now derives the staged runner path from the invocation, computes <dir>/../SKILL.md, and asserts a cp stages exactly that — a flat re-stage fails. 60/60 + 12/12.
Collaborator
|
Thanks for the quick fix! Template looks good ✓ Problem: Observed P0 regression with clear evidence — every review-address run since #7165 crashes with Direction: CI infrastructure fix for the autofix loop — clearly within scope and urgently needed. The fix also tightens security by sourcing model instructions from the trusted base. Size: Not applicable — no core package paths touched. Approach: Minimal and focused — mirror the skill's on-disk layout ( Moving on to code review. 🔍 中文说明感谢快速修复！ 模板完整 ✓ 问题： 已观测到的 P0 回归，证据清晰——自 #7165 合入后，每次 review-address 运行都在读取反馈前以 方向： autofix 循环的 CI 基础设施修复，完全在范围内且紧急。修复同时从可信基座加载模型指令，安全性也有提升。 规模： 不适用——未触及核心包路径。 方案： 最小且聚焦——镜像 skill 的磁盘布局，使相对路径 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
Code ReviewIndependent proposal: given that Comparison with the diff: the PR does exactly the mirror-layout approach. Clean match. The workflow change is three lines replacing one: The test change is the interesting part — instead of just updating the expected path string, it derives the staged runner path from the invocation regex, computes where No critical blockers. No convention violations. No unrelated changes. TestingThis is a CI workflow change (GitHub Actions YAML + test file) — no CLI behavior to exercise in tmux. The appropriate verification is the test suite: YAML validation: 中文说明代码审查独立方案： 既然 与 diff 对比： PR 正是采用镜像布局方案，完全吻合。 workflow 变更用三行替换一行： 测试变更是亮点——不是简单更新预期路径字符串，而是从调用正则推导 staged runner 路径，计算 无关键阻塞项，无规范违反，无无关改动。 测试这是 CI workflow 变更（GitHub Actions YAML + 测试文件）——无 CLI 行为可在 tmux 中验证。适当的验证是测试套件：60/60 autofix 测试通过，12/12 fleet shepherd 测试通过。YAML 解析有效。 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
|
Confidence: 5/5 — clean P0 regression fix, minimal diff, well-evidenced problem, improved test invariant. This is exactly the fix I'd write independently. The problem is real and clearly demonstrated — every review-address run since #7165 crashes with ENOENT before reading feedback. The mirror-layout approach is the simplest correct solution: three shell lines in the workflow, one invocation path update, and a test that now derives the staging invariant from the invocation rather than pinning a string. The test improvement is the kind of thing that prevents the next regression in this area. No reservations. Ship it. 中文说明置信度：5/5 —— 干净的 P0 回归修复，最小 diff，问题证据充分，测试不变量改进。 这正是我会独立写出的修复。问题真实且演示清晰——自 #7165 以来每次 review-address 运行都在读取反馈前以 ENOENT 崩溃。镜像布局是最简正确方案：workflow 中三行 shell、一处调用路径更新，加上一个从调用推导 stage 不变量（而非钉死字符串）的测试。测试改进正是防止此区域下次回归的那种东西。 无保留意见。可以合入。 — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
approved these changes
Jul 19, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Collaborator
Author
Code Review — P0 autofix staging fixVerdict: LGTM. Correct root-cause fix, minimal and surgical, with a test that now actually guards the failure mode. Notes and one optional hardening below. What it does
Verification I performed (against the PR head)
Strengths
Suggestions (optional, non-blocking)
RiskLow — confined to file layout under |
qqqys
approved these changes
Jul 19, 2026
Contributor
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. |
Collaborator
Author
|
Thanks for the thorough verification — merged. Both non-blocking suggestions are addressed in the follow-up #7227:
中文说明多谢细致核验 —— 已合入。两条非阻塞建议均在 follow-up #7227 落实:
|
This was referenced Jul 19, 2026
pull Bot
pushed a commit
to bhardwajRahul/qwen-code
that referenced
this pull request
Jul 19, 2026
…QwenLM#7229) * fix(autofix): a no-output crash must not advance the review watermark When a review-address run crashes AFTER prepare (so NEWEST is set) but BEFORE the agent writes any verdict — no address-summary.md, no-action.md, or failure.md — the handoff stamped the marker with ts=NEWEST, advancing the feedback watermark as if the feedback had been evaluated. It hadn't. The next scan then saw 'nothing new since <NEWEST>' and never retried, stranding the PR on a purely transient crash. That is exactly what happened to QwenLM#7219 during the QwenLM#7165 SKILL-staging outage: the run crashed at promptFor (ENOENT) at 09:50, the handoff advanced the watermark to 09:50:56, and even after QwenLM#7225 fixed the crash the loop considered all prior feedback 'addressed' and would not re-engage. Fix: on a no-output crash (NEWEST set, DETAIL_FILE empty) stamp the sentinel ts instead — it is excluded from EVAL_WM, so the watermark does not move and the next scan retries the same feedback. The round still increments, so a PERSISTENT crash is bounded by MAX_ROUNDS and ends in a terminal handoff rather than looping forever. Agent-produced handoffs (verify failed after real output) keep advancing the watermark as before. Replay test extended to assert BOTH MARK_TS and MARK_ROUND across all three shapes: output+verify-fail → advance; no-output crash → sentinel (retry); pre-prepare crash → terminal. 60/60 + 12/12. * fix(autofix): correct the final-attempt crash headline per review Two review findings on the no-output-crash handoff: - The headline promised 'it will retry on the next scan' even on the final attempt, but at MARK_ROUND == MAX_ROUNDS the scan's round-cap gate skips the PR and the cap-reached notice is takeover-only — so a maintainer was told a retry was coming that never comes. The headline now branches: 'it will retry' only while MARK_ROUND < MAX_ROUNDS, otherwise 'this was the last automatic attempt; a human should take over'. - It embedded a Run log URL that the report block already appends to every handoff, duplicating it in the comment. Removed from the headline. Replay test extended: mid-attempt headline promises retry and carries no Run log; final-attempt headline says human-takeover and never 'retry'. 60/60 + 12/12. --------- Co-authored-by: wenshao <wenshao@example.com>
pull Bot
pushed a commit
to bit-cook/qwen-code
that referenced
this pull request
Jul 19, 2026
…wenLM#7227) * test(autofix): exercise the SKILL stage↔resolve contract end-to-end Follow-up to QwenLM#7225, implementing the reviewer's non-blocking suggestions. The staging guard QwenLM#7225 added pins the mirrored LAYOUT but re-implements run-agent.mjs's `<dir>/../SKILL.md` convention in the test. If that coupling ever moves in the RUNNER (e.g. ../../SKILL.md), the string test stays green while prod breaks again — the same class of blind spot that let QwenLM#7165 ship. This adds the one check that exercises the contract for real: stage the actual runner into a mirrored tmp layout, run it with --print-prompt, and assert it reads the staged SKILL (sentinel body + resolved skill dir). The negative case — the flat layout QwenLM#7165 shipped — is asserted to crash with ENOENT, proving the test catches that regression. Also replaces the brittle fixed-width `[\s\S]{0,200}` bound between `core.hooksPath .husky` and the runner invocation with a direct ordering assertion (indexOf), so adding a comment between the two lines can no longer fail the test spuriously. 61/61 + 12/12. * test(autofix): harden the stage↔resolve integration test per review Applies all four inline suggestions on QwenLM#7227: - spawn process.execPath, not the bare 'node' string, so a version-manager shim or a PATH without node can't turn the test into an opaque 'null !== 0'. - nest the flat-layout runner under dir/flat/ so its ../SKILL.md resolves to dir/SKILL.md (never created) instead of a shared tmpdir()/SKILL.md that a concurrent job could leave behind and make the negative case pass spuriously — a real flake in the deflake-test itself. - reuse the existing withRunnerDir helper instead of duplicating its mkdtemp/try/finally/rmSync. - bound each spawnSync with timeout: 10_000 so a hung runner fails the test instead of the whole CI job (spawnSync blocks the event loop, so vitest's async timeout can't fire). 61/61 + 12/12. --------- Co-authored-by: wenshao <wenshao@example.com>
Collaborator
|
Released in v0.20.1. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Fixes a P0 regression from #7165 that has taken the autofix loop's entire review-address path down on
main.#7165 stages a trusted copy of
run-agent.mjs(so the agent never executes the checked-out PR branch's version on the host with the model key). Butrun-agent.mjsresolves its instructions relative to its own location:The staging was flat —
${RUNNER_TEMP}/run-agent.mjs— so../SKILL.mdresolved to${RUNNER_TEMP}/../SKILL.md=/home/runner/work/SKILL.md, which does not exist. Every review-address run since #7165 merged crashed:— before reading any feedback. Feedback-addressing and takeover were both down on
main.The fix stages the runner and its SKILL in a mirrored layout and invokes the staged runner from there, so
../SKILL.mdresolves to the staged SKILL:This also closes a latent gap: the model's instructions now come from the trusted base too, never the checked-out PR branch (the issue-phase invocations, which run the in-repo copy before any untrusted checkout, are unchanged).
Why it's needed
The loop's core function — addressing review feedback on bot PRs and takeover PRs — has been failing on every run since #7165 merged (07:35 UTC). The agent crashes before it can read the feedback, so it produces a "could not start evaluation" handoff at best and a red job at worst.
Reviewer Test Plan
How to verify
npx vitest run scripts/tests/qwen-autofix-workflow.test.js— 60/60. The staging test previously pinned thecpstring and the invocation but never checked SKILL.md was resolvable from the staged location — exactly the blind spot that let this ship. It now derives the staged runner path from the invocation, computes<dir>/../SKILL.md, and asserts acpstages exactly that path (plus the mirroredmkdir -p .../scripts). A flat re-stage fails the suite.npx vitest run scripts/tests/qwen-fleet-shepherd-workflow.test.js— 12/12 (untouched).run:block passesbash -n.promptFor.Evidence (Before & After)
Tested on
Risk & Scope
${RUNNER_TEMP}and which path the review-address step invokes. The issue-phase invocations (in-repo copy, pre-checkout) are unchanged. Security strictly improves: model instructions now come from the trusted base.Linked Issues
Regression fix for #7165.
中文说明
本 PR 做了什么
修复 #7165 引入的 P0 回归 —— 它已经让
main上 autofix 循环的整条「处理评审反馈(review-address)」路径瘫痪。#7165 会 stage 一份可信的
run-agent.mjs(避免在宿主上以带模型密钥的环境执行检出的 PR 分支版本)。但run-agent.mjs相对自身位置解析指令文件:原先是扁平 stage ——
${RUNNER_TEMP}/run-agent.mjs—— 于是../SKILL.md落到${RUNNER_TEMP}/../SKILL.md=/home/runner/work/SKILL.md,该文件不存在。#7165 合入后每一次 review-address 运行都在读取任何反馈之前崩溃(ENOENT)。反馈处理与接管两条路都断了。修复:把 runner 与其 SKILL 以镜像布局一起 stage,并从该位置调用,使
../SKILL.md解析到 staged 的 SKILL:同时顺带关闭一个潜在缺口:模型指令现在也来自可信基座,而非检出的 PR 分支(issue 阶段在任何不可信检出之前跑仓库内副本,不受影响)。
为什么需要
循环的核心功能 —— 处理 bot PR 与接管 PR 的评审反馈 —— 自 #7165 合入(07:35 UTC)起每次运行都失败。agent 在读到反馈前就崩溃。
评审验证
npx vitest run scripts/tests/qwen-autofix-workflow.test.js—— 60/60。staging 测试此前只钉了cp字符串与调用串,却从未检查 SKILL.md 能否从 staged 位置解析到 —— 正是这个盲点放行了本次回归。现在测试从调用串推出 staged runner 路径,计算<dir>/../SKILL.md,断言存在一条cp恰好 stage 该路径(以及镜像的mkdir -p .../scripts);扁平 stage 会让套件失败。npx vitest run scripts/tests/qwen-fleet-shepherd-workflow.test.js—— 12/12(未改动)。run:块通过bash -n。promptFor崩溃。风险与范围
${RUNNER_TEMP}下的布局,以及 review-address 步骤调用哪条路径。issue 阶段调用(仓库内副本、检出前)不变。安全面严格改善:模型指令改为来自可信基座。关联 Issue
#7165 的回归修复。