-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(ci): resolve latest and retry the triage runner's qwen CLI installs #8367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ded42dd
47bca86
dff317e
09e9447
8506aa0
0df14bc
dfe2583
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -220,15 +220,48 @@ describe('qwen-triage tmux workflow', () => { | |||||||||||||||||
| }); | ||||||||||||||||||
|
|
||||||||||||||||||
| it('pins the action reinstall to the version the job already runs', () => { | ||||||||||||||||||
| const ensure = step('Ensure qwen CLI'); | ||||||||||||||||||
| expect(workflow).toContain("id: 'ensure_qwen'"); | ||||||||||||||||||
| expect(workflow).toContain( | ||||||||||||||||||
| 'echo "version=$(qwen --version)" >> "${GITHUB_OUTPUT}"', | ||||||||||||||||||
| ); | ||||||||||||||||||
| // Every exit-0 path of the step must emit the version output — the action | ||||||||||||||||||
| // pin reads it, and a path that forgets it re-pins the reinstall to an | ||||||||||||||||||
| // empty string (which the action treats as `latest`, resurrecting the | ||||||||||||||||||
| // stale-dist-tag bug the pin exists to prevent). Three success paths, | ||||||||||||||||||
| // four emission sites (fallback-to-installed is reached twice: registry | ||||||||||||||||||
| // down, install failed), plus fresh-install. | ||||||||||||||||||
| const emissions = ( | ||||||||||||||||||
| ensure.match(/echo "version=\$\{?\w+\}?" >> "\$\{GITHUB_OUTPUT\}"/g) ?? [] | ||||||||||||||||||
| ).length; | ||||||||||||||||||
| expect(emissions).toBeGreaterThanOrEqual(4); | ||||||||||||||||||
| expect(workflow).toContain( | ||||||||||||||||||
| "qwen_cli_version: '${{ steps.ensure_qwen.outputs.version }}'", | ||||||||||||||||||
| ); | ||||||||||||||||||
| }); | ||||||||||||||||||
|
|
||||||||||||||||||
| it('resolves latest, retries the install, and never trusts a cwd .npmrc', () => { | ||||||||||||||||||
| // The step used to short-circuit on any pre-installed qwen — the same bug | ||||||||||||||||||
| // the review runner fixed in c9f0d0657 — and its stale version output then | ||||||||||||||||||
| // pinned the action reinstall to a stale release. Pin the repaired shape: | ||||||||||||||||||
| // resolve-then-compare, a bounded retry loop, the RUNNER_TEMP cwd (npm | ||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The comment below claims this test pins "resolve-then-compare", but no assertion covers the staleness comparison Add an assertion for the comparison, e.g.: expect(ensure).toContain('[ "$have" = "$want" ]');中文说明下方注释声称本测试钉住了 “resolve-then-compare”,但没有任何断言覆盖版本比较 — qwen3.8-max-preview via Qwen Code /review (v0.21.3) |
||||||||||||||||||
| // reads a cwd .npmrc from the persistent workspace into a global install), | ||||||||||||||||||
| // and the registry-down fallback that still triages on the installed CLI. | ||||||||||||||||||
| const ensure = step('Ensure qwen CLI'); | ||||||||||||||||||
| expect(ensure).toContain("@qwen-code/qwen-code@latest' version"); | ||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] No assertion covers the
Suggested change
中文说明没有断言覆盖 — qwen3.8-max-preview via Qwen Code /review (v0.21.3) |
||||||||||||||||||
| expect(ensure).toContain('for attempt in 1 2 3; do'); | ||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The retry loops added to
Suggested change
中文说明[建议] 添加到 — qwen3.8-max-preview via Qwen Code /review |
||||||||||||||||||
| expect(ensure).toContain('cd "${RUNNER_TEMP:?}"'); | ||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] No assertion covers the install command's target
Suggested change
中文说明没有断言覆盖安装命令的目标 — qwen3.8-max-preview via Qwen Code /review (v0.21.3) |
||||||||||||||||||
| expect(ensure).toContain('triaging with installed qwen'); | ||||||||||||||||||
|
Comment on lines
+250
to
+251
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The rewritten
Suggested change
中文说明重写后的 — qwen3.8-max-preview via Qwen Code /review (v0.21.3) |
||||||||||||||||||
| // The two hard-fail paths must keep failing the job — pin the exit code, | ||||||||||||||||||
| // not just the message: a softened `exit 0` would otherwise ship undetected. | ||||||||||||||||||
| // Anchor each message to its own `exit 1`; the step has two hard-fail | ||||||||||||||||||
| // sites, so an unanchored `toContain('exit 1')` stays green when either | ||||||||||||||||||
| // one is softened while the other still fails the job. | ||||||||||||||||||
| expect(ensure).toMatch( | ||||||||||||||||||
| /::error::qwen is not installed and the npm registry query failed'\n\s*exit 1/, | ||||||||||||||||||
| ); | ||||||||||||||||||
| expect(ensure).toMatch( | ||||||||||||||||||
| /::error::qwen is not installed and installing \$want failed after 3 attempts"\n\s*exit 1/, | ||||||||||||||||||
| ); | ||||||||||||||||||
| }); | ||||||||||||||||||
|
|
||||||||||||||||||
| it('passes triage output through env before bash reads it', () => { | ||||||||||||||||||
| const checkStep = step('Check triage response'); | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -565,6 +598,8 @@ describe('qwen-triage tmux workflow', () => { | |||||||||||||||||
| ); | ||||||||||||||||||
| expect(installStep).toContain('qwen --version'); | ||||||||||||||||||
| expect(installStep).toContain('tmux -V'); | ||||||||||||||||||
| expect(installStep).toContain('for attempt in 1 2 3; do'); | ||||||||||||||||||
| expect(installStep).toContain('failed after 3 attempts'); | ||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] This lane's new retry assertion pins the error message but not the guard's
Suggested change
中文说明本 lane 新增的重试断言只钉住了错误消息,没钉住守卫的 — qwen3.8-max-preview via Qwen Code /review (v0.21.3) |
||||||||||||||||||
| expect(resolverStep).not.toContain('tmux'); | ||||||||||||||||||
| expect(resolverStep).not.toContain('npm install'); | ||||||||||||||||||
| expect(resolverStep).not.toContain('qwen --version'); | ||||||||||||||||||
|
|
@@ -3979,6 +4014,8 @@ describe('qwen-triage verify round-3 hardening', () => { | |||||||||||||||||
| // No hardcoded Playwright pin here either: the apt list must track | ||||||||||||||||||
| // current Playwright so it covers the lockfile-matched binary below. | ||||||||||||||||||
| expect(tools).not.toMatch(/playwright@[\d.]/); | ||||||||||||||||||
| expect(tools).toContain('for attempt in 1 2 3; do'); | ||||||||||||||||||
| expect(tools).toContain('failed after 3 attempts'); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Browser binary: downloaded after npm ci, and by the CLI of the | ||||||||||||||||||
| // package the capture harness actually imports — never a hardcoded pin | ||||||||||||||||||
|
|
||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] The 10-line retry-install block is duplicated verbatim across the workflow (lines 615–623, 1172–1181, 2456–2465), and the copies have already drifted — the
Ensure qwen CLIstep carries--loglevel=error --no-auditand pins@$want, while these two siblings do not. — Failure scenario: when the retry policy next needs a change (bumping to 5 attempts, adding--no-audit), the fix must be applied to three locations in lockstep; missing one silently diverges the lanes. The drift is already present.Note: the tmux/verify installs run before checkout, so a repo-local
.github/scripts/install-qwen.shwould not be on disk there — extraction needs a pre-checkout vehicle (composite action or script fetched at a pinned ref).中文说明
[建议] 10 行重试安装块在 workflow 中逐字复制了三份(615–623、1172–1181、2456–2465 行),且副本已经漂移——
Ensure qwen CLI步骤带有--loglevel=error --no-audit并钉住@$want,而这两个兄弟步骤没有。— 故障场景:当下次需要更改重试策略(增加到 5 次、添加--no-audit)时,必须同步修改三处;漏掉一处就会悄悄分化各 lane。漂移已经存在。注意:tmux/verify 安装在 checkout 之前运行,因此仓库内的
.github/scripts/install-qwen.sh在那里不存在——提取需要 checkout 前的载体(composite action 或按固定 ref 获取的脚本)。— qwen3.8-max-preview via Qwen Code /review