fix(ci): don't fail triage cleanup when there is nothing to clean - #7688
Conversation
The 'Clean stale agent state' step strips non-allowlisted keys from the persistent workspace's local git config through a `git config --list | grep -ivE <allowlist> | while ...` pipeline. When the config holds only allowlisted keys — the steady state on a reused runner this step already sanitized, since actions/checkout's post step removes its auth extraheader at job end — grep matches nothing and exits 1. Under the default `bash -e` shell combined with the script's `set -o pipefail`, that kills the step exactly when there is nothing to clean, before any output, and every downstream triage step is skipped (seen on run 30095456731, runner ecs-qwen-runner-sg-4). Guard the grep with `|| true` so an empty match feeds an empty loop instead of failing the job. Sanitization behavior is unchanged: non-allowlisted keys (core.pager, include.path, `!`-aliases) are still stripped. The workflow test harness missed this because its allowlist test re-assembles the pipeline without the step's shell flags and always plants non-allowlisted keys first. Add a steady-state regression test that runs the step's actual script under `bash -e` against a config holding only allowlisted keys, plus a negative control that strips the guard and demands the step die — red on the old workflow, green now.
2f1a5ac to
f4f3b68
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push,因为这会使已有的评审评论失效。另外,供日后参考:作为集成流程的一部分,机器人始终会自动将所有改动压缩(squash)为单个提交。 |
|
Thanks for the PR! Template: the body uses Problem: observed bug with solid evidence. The linked CI run (30095456731) shows the "Clean stale agent state" step dying with exit 1 and no output. The root cause is well-explained: Direction: straightforward CI infrastructure fix, clearly in scope. Size: not applicable — the changed file ( Approach: the scope is exactly right. Moving on to code review. 🔍 中文说明感谢贡献! 模板:PR 正文使用了 问题:已观测到的 bug,证据充分。关联的 CI run(30095456731)显示 "Clean stale agent state" 步骤以 exit 1 静默失败。根因解释清晰: 方向:直接的 CI 基础设施修复,明确在范围内。 规模:不适用——变更文件( 方案:范围恰好。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Code review — independent proposal first: given grep-exits-1-on-no-match killing a The diff has two parts: 1. Workflow fix (
2. Regression test (
No correctness issues, no security concerns, no convention violations. Nothing to flag. CI test evidence (from the PR's own checks at
Unit tests don't directly exercise a workflow YAML change, but the new regression test covers the fix. No failures observed; ubuntu test still running. Real-scenario testing: N/A — CI workflow change, nothing user-visible to drive in tmux. 中文说明代码审查——先独立提出方案:鉴于 grep 零匹配时退出码 1 会在 diff 包含两部分: 1. Workflow 修复(
2. 回归测试(
无正确性问题、无安全隐患、无规范违反。无需指出任何问题。 CI 测试证据(来自 PR 自身在 真实场景测试:N/A——CI workflow 变更,无用户可见行为可在 tmux 中驱动。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — clean across every stage; would merge without hesitation. This is a textbook small fix: real observed failure, clear root cause, minimal idiomatic change, accurate comment — and now a regression test that runs the actual step script under The PR body doesn't follow the template headings, but for a CI fix with reproduction, before/after verification, and a regression test, the substance is all there. Not worth a round-trip. 中文说明置信度:5/5——各阶段均无问题,毫不犹豫地合并。 这是一个教科书式的小修复:真实观测到的故障、清晰的根因、最小化的惯用修改、准确的注释——现在还有一个回归测试,在 PR 正文未使用模板标题,但对于附带复现、before/after 验证和回归测试的 CI 修复,实质内容齐全,不值得为此多一轮往返。 — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
中文说明
— qwen3.7-max via Qwen Code /review
doudouOUC
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
yiliang114
left a comment
There was a problem hiding this comment.
Approve ✅
Classic bash -e + pipefail trap — grep exits 1 when there's nothing to match, killing the whole step exactly when there's nothing to clean. The { grep ... || true; } guard is the minimal correct fix.
Tests are well-structured: positive case (steady state succeeds) + negative control (without the guard, same state fails). The negative control is especially valuable — it proves the guard is load-bearing, not dead code.
|
Released in v0.21.1. |
|
PR #7688 已合并(7月24日),CI bot 的评论只是通知该修复已随 v0.21.1 发布。无需代码变更,无需回复。确认收到。 ✅ completed |
Problem
The triage job on run 30095456731 (PR #7672) failed in Clean stale agent state with exit 1 and no output, before checkout ever ran. The failure has nothing to do with the PR being triaged.
The step sanitizes the persistent workspace's local git config through:
When the config holds only allowlisted keys,
grepmatches nothing and exits 1. The step runs under the Actions defaultbash -eshell, and the script's ownset -o pipefailpropagates grep's status through the pipeline — so the step dies silently.That empty-match state is not an edge case; it is the steady state on a reused runner: a previous run of this very step already stripped everything non-allowlisted, and
actions/checkout's post step removes its auth extraheader at job end, leaving only allowlisted plumbing keys (core.*,remote.*,branch.*). In other words, the step fails exactly when there is nothing to clean. Downstream, "Check triage response" then reports the misleading "global CLI install" error, and a re-run landing on the same runner fails the same way.Reproduction (fresh repo, config = allowlisted keys only):
Fix
Guard the grep with
|| trueso an empty match feeds an empty loop instead of failing the job.Verified that sanitization is unchanged: with
core.pager,include.path, and a!-command alias planted, the fixed step still strips all three and exits 0.Regression test
The workflow test harness (
qwen-triage-workflow.test.mjs) missed this because its allowlist test re-assembles the pipeline without the step's shell flags and always plants non-allowlisted keys first. Added a steady-state test that runs the step's actual script underbash -eagainst a config holding only allowlisted keys, plus a negative control that strips the guard and demands the step die — red on the unfixed workflow (26/28), green with the fix (28/28).中文说明
问题
run 30095456731(PR #7672)的 triage job 在 Clean stale agent state 步骤以 exit 1 失败,无任何输出,checkout 尚未执行。失败与被 triage 的 PR 本身无关。
该步骤通过如下管道清理持久 workspace 的本地 git config:
当 config 中只剩 allowlist 内的键时,
grep匹配不到任何行,退出码为 1。该步骤运行在 Actions 默认的bash -eshell 下,脚本自身的set -o pipefail又把 grep 的退出码传播到整条管道——于是步骤静默死亡。这个"空匹配"并非罕见边界,而是复用 runner 上的稳态:上一次运行的同一步骤已把非 allowlist 键清理干净,
actions/checkout的 post 步骤也会在 job 结束时移除 auth extraheader,只剩下 allowlist 内的键(core.*、remote.*、branch.*)。也就是说,恰恰在没有东西可清理时步骤反而失败。下游 "Check triage response" 随后报出误导性的 "global CLI install" 错误;re-run 若调度到同一台处于稳态的 runner 会以同样方式再次失败。复现(全新仓库,config 仅含 allowlist 键):
修复
给 grep 加
|| true兜底,空匹配时向 while 循环输入空内容,而不是让整个 job 失败。已验证清理行为不变:植入
core.pager、include.path和!命令 alias 后,修复后的步骤仍会全部清除并以 exit 0 结束。回归测试
workflow 测试 harness(
qwen-triage-workflow.test.mjs)之前没抓到这个 bug:其 allowlist 测试自行重组管道(不带步骤的 shell 标志),且总是先植入非 allowlist 键。本 PR 新增稳态回归测试:在只含 allowlist 键的 config 上以bash -e运行步骤的真实脚本,并附带一个去掉兜底后必须失败的负向对照——未修复的 workflow 下 26/28 红,修复后 28/28 绿。