test(autofix): exercise the SKILL stage↔resolve contract end-to-end - #7227
Merged
Conversation
Follow-up to #7225, implementing the reviewer's non-blocking suggestions. The staging guard #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 #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 #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.
Collaborator
|
Thanks for the PR! Template looks good ✓ (the "Why it's needed" content is folded into "What this PR does" — fine for a test-only follow-up where the motivation is self-evident). Problem: observed gap, not theoretical. The #7165 regression shipped because the string test re-implemented the runner's Direction: aligned — this is a direct follow-up to reviewer suggestions on #7225, strengthening test coverage for the autofix staging pipeline. No product-direction concerns. Size: not applicable (no core paths touched; Approach: scope is tight — one file, two focused changes (new e2e contract test + indexOf ordering fix). No unrelated changes, no drive-by refactors. The sentinel-value approach and the negative flat-layout case are exactly what this test needs. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓（"Why it's needed" 的内容融入了 "What this PR does"——对于动机不言自明的纯测试后续 PR 来说没问题）。 问题：已观测到的盲点，非理论性问题。#7165 的回归之所以能发布，正是因为字符串测试重新实现了 runner 的 方向：对齐——这是 #7225 评审建议的直接后续，加强 autofix staging 管线的测试覆盖。无产品方向顾虑。 规模：不适用（未触及核心路径；仅 方案：范围紧凑——一个文件，两处聚焦改动（新 e2e 契约测试 + indexOf 顺序断言）。无无关改动，无顺手重构。sentinel 值方案和反向扁平布局用例正是该测试所需的。 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
|
Code review — clean, no blockers. The new e2e contract test does exactly what the string test can't: it stages the real The No correctness, security, or convention issues found. Test results — 61/61 pass, fleet-shepherd 12/12 unaffected: No user-visible behavior change — this is test-only, so tmux real-scenario testing is N/A. 中文说明代码审查——干净，无阻塞项。 新的 e2e 契约测试做了字符串测试做不到的事：把真实的
未发现正确性、安全性或惯例问题。 测试结果——61/61 通过，fleet-shepherd 12/12 不受影响。 无用户可见行为变更——纯测试 PR，tmux 真实场景测试不适用。 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
|
Confidence: 5/5 Focused, test-only follow-up that closes a real coverage gap identified in #7225 review. The e2e contract test exercises the actual runner against the staged layout (positive + negative cases), and the indexOf ordering fix removes a brittle magic-number window. All 61 tests pass, no production code touched, no scope creep. Ready to ship. ✅ 中文说明置信度：5/5 聚焦的纯测试后续 PR，关闭了 #7225 评审中发现的真实覆盖盲点。e2e 契约测试用真实 runner 对 staged 布局做了正向 + 反向验证，indexOf 顺序断言移除了脆弱的魔法数字窗口。61 项测试全部通过，未触及生产代码，无范围蔓延。可以合并。✅ — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
previously 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. ✅
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. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.8-max-preview via Qwen Code /review
wenshao
commented
Jul 19, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
— qwen3.8-max-preview via Qwen Code /review
Applies all four inline suggestions on #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.
qwen-code-ci-bot
previously 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.
No issues found. LGTM! ✅
— qwen3.8-max-preview via Qwen Code /review
yiliang114
previously approved these changes
Jul 19, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Test-only, and it closes the actual blind spot #7225 left: that guard pinned the mirrored layout but re-implemented the runner's <dir>/../SKILL.md convention inside the test, so a coupling change on the runner side would keep the string test green while prod breaks. This runs the real runner with --print-prompt and asserts it reads the staged SKILL, with the flat layout asserted to ENOENT — that's the contract exercised for real.
The follow-up commit also folded in the earlier review points cleanly: process.execPath over bare node, nesting the flat runner under dir/flat/ so its ../SKILL.md can't accidentally resolve to a shared tmpdir()/SKILL.md a concurrent job left behind, reusing withRunnerDir, and the spawnSync timeout. Nothing blocks merge. LGTM.
…ing-test # Conflicts: # scripts/tests/qwen-autofix-workflow.test.js
wenshao
dismissed stale reviews from yiliang114 and qwen-code-ci-bot
via
July 19, 2026 14:35
cd21b9c
wenshao
enabled auto-merge
July 19, 2026 14:35
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.
No issues found. LGTM! ✅
— qwen3.8-max-preview via Qwen Code /review
yiliang114
approved these changes
Jul 19, 2026
Collaborator
|
Released in v0.20.1. |
Collaborator
|
Agent run timed out after 1800000ms ❌ failed |
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
Follow-up to #7225 implementing the reviewer's two non-blocking test suggestions.
1. End-to-end stage↔resolve contract test (the residual blind spot). #7225's guard pins the mirrored layout, but it re-implements
run-agent.mjs's<dir>/../SKILL.mdconvention inside the test (by stripping/scripts/run-agent.mjs). If that coupling ever moves in the runner — someone changes it to../../SKILL.mdor./SKILL.md— the string test stays green while prod breaks again. That's the same class of blind spot that let #7165 ship, just relocated.This adds the one check that exercises the contract for real: stage the actual
run-agent.mjsinto a mirrored tmp layout, run it with--print-prompt, and assert it reads the staged SKILL (sentinel body + a resolved skill directory ending inautofix-skill). The negative case — the flat layout #7165 shipped — is asserted to exit non-zero withENOENT ... SKILL.md, proving the test catches that exact regression.2. Brittle length bound. The
[\s\S]{0,200}window betweencore.hooksPath .huskyand the runner invocation currently spans ~150 chars; two more comment lines there would break the test spuriously (it already forced a comment trim in #7225). Replaced with a direct ordering assertion (indexOfof the hooks re-point <indexOfof the invocation, both within the agent step), which is what the test actually means.(The third suggestion was PR-description wording on #7225 and needs no code.)
Reviewer Test Plan
How to verify
npx vitest run scripts/tests/qwen-autofix-workflow.test.js— 61/61. The new integration test spawns the real runner both ways:autofix-skill/{SKILL.md,scripts/run-agent.mjs}→ exit 0, prompt contains the staged sentinel.run-agent.mjsalone → non-zero,stderrcontainsENOENT+SKILL.md'.npx vitest run scripts/tests/qwen-fleet-shepherd-workflow.test.js— 12/12 (untouched).cpand the end-to-end test fails at the sentinel assertion — the string guard alone would still pass.Tested on
The skill-directory assertion matches the
autofix-skillbasename rather than the full temp path, since macOS canonicalizes/var→/private/var.Risk & Scope
Linked Issues
Follow-up to #7225 (P0 SKILL staging fix).
中文说明
本 PR 做了什么
#7225 的后续,落实评审的两条非阻塞测试建议。
1. 端到端的 stage↔resolve 契约测试(残留盲点)。 #7225 的守卫钉住了镜像布局,但在测试里重新实现了
run-agent.mjs的<dir>/../SKILL.md约定(靠剥掉/scripts/run-agent.mjs)。若该耦合日后在 runner 侧改变(比如改成../../SKILL.md),字符串测试仍绿而生产再次崩溃 —— 正是放行 #7165 的同类盲点,只是换了位置。本 PR 补上唯一真正验证契约的检查:把真实的
run-agent.mjsstage 进镜像 tmp 布局,用--print-prompt实跑,断言它读到的是 staged 的 SKILL(sentinel 正文 + 解析出的 skill 目录以autofix-skill结尾)。反向用例 —— #7165 发布的扁平布局 —— 断言以非零退出并携带ENOENT ... SKILL.md,证明该测试能抓住那次回归。2. 脆弱的长度上限。
core.hooksPath .husky与 runner 调用之间的[\s\S]{0,200}窗口目前约 150 字符;再加两行注释就会误伤(在 #7225 里已被迫精简过一次注释)。改为直接的顺序断言(hooks 复位的indexOf< 调用的indexOf,均在 agent 步骤内),这才是测试的本意。(第三条建议是 #7225 的描述措辞,无需改代码。)
风险与范围
关联 Issue
#7225(P0 SKILL staging 修复)的后续。