test(triage): regression-guard the triage workflow, and make the git cleanup an allowlist - #7660
Conversation
…cleanup an allowlist Guards the security-critical invariants of qwen-triage.yml that broke silently once already — the `settings_json:` input name was wrong, so the action dropped it and the review agent ran with the full default toolset and no deny list. A new `node:test` suite (wired into the shared HELPER_TESTS list both CI paths run) asserts: the `settings:` input name (never `settings_json:`), the tools.core registration whitelist and the deny list, the fork-PR runner routing invariants, and the git exec-vector cleanup. It also flips that cleanup from a best-effort denylist — which kept missing new families (pager, filter.*, includeIf subsections, url.*, credential…) — to a keep-known-safe allowlist: unset every local config key that isn't plumbing actions/checkout needs (repo format, remote, branch, fetch/gc/pack/index, safe.directory, extensions, submodule url/active/branch — not submodule.*.update, which can be `!cmd`). This closes the whole exec-vector class, including knobs not yet enumerated. The harness runs the workflow's actual allowlist pattern against a scratch repo to prove it unsets every exec family and preserves the checkout plumbing.
|
Thanks for the PR! Template: the body doesn't use the exact template headings, but the content is all there and well-organized — what, why, verification, and a Chinese translation. Not blocking on heading names for a CI/test-only PR. Problem: clearly observed. The settings_json vs settings bug (#7646/#7647) was a real silent failure with no test to catch it, and the denylist kept missing new git-config exec families during the #7647 review. Both problems actually happened. Direction: aligned — this is regression-guarding security-critical CI invariants that already broke once. The allowlist flip is the right architectural call (denylist was unwinnable, as the #7647 review thread showed). Size: not applicable — no core paths touched. All changes are in .github/ (CI workflows and test scripts). 250 total lines: 215 test, 35 workflow config. Approach: scope is tight and every edit earns its place. The test file does both structural assertions (parse the YAML, check keys) and a behavioral test (extract the actual allowlist regex, run it against a scratch repo with real git). The allowlist in qwen-triage.yml is a clean replacement of the denylist — same step, same structure, just inverted logic. No unrelated changes, no scope creep. Moving on to code review. 🔍 中文说明感谢贡献！ 模板：PR body 没有使用模板的标准标题，但内容完整且组织良好——做了什么、为什么、验证方式、中文翻译都有。对于纯 CI/测试 PR，不因标题格式阻塞。 问题：已观测到的真实问题。settings_json vs settings 的 bug（#7646/#7647）是真实的静默失败，当时没有测试能拦截；denylist 在 #7647 review 中不断漏掉新的 git-config exec 家族。两个问题都实际发生过。 方向：对齐——这是对已经出过问题的安全关键 CI 不变量做回归防护。allowlist 翻转是正确的架构决策（denylist 追不完，#7647 review 线程已证明）。 规模：不适用——未触及核心路径。所有改动在 .github/（CI workflow 和测试脚本）。共 250 行：215 行测试，35 行 workflow 配置。 方案：范围紧凑，每处改动都有必要。测试文件同时做了结构断言（解析 YAML、检查 key）和行为测试（提取 workflow 真实的 allowlist 正则，对 scratch repo 用真实 git 跑）。qwen-triage.yml 中的 allowlist 是 denylist 的干净替换——同一步骤、同一结构，只是反转逻辑。无无关改动，无范围蔓延。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
Code ReviewIndependent proposal: for this problem I would do exactly what this PR does — a Findings: no critical blockers, no convention violations. The test file follows the established helper-test pattern ( The allowlist regex is sound. It keeps only plumbing that carries no command — repo format flags, remote/branch/fetch/gc/pack/index, safe.directory, extensions, and submodule url/active/branch. The TestingCI checks at review time (commit
The ubuntu test suite (which runs No TUI surface is touched — this is a CI/test-only PR, so real-scenario tmux testing is not applicable. 中文说明代码审查独立方案： 针对这个问题，我会做和这个 PR 完全一样的事——一个解析 workflow YAML 并断言安全关键不变量的 node:test 套件，接入 HELPER_TESTS，加上 denylist 到 allowlist 的翻转。PR 更进一步，做了行为测试：提取 workflow 真实的 allowlist 正则，对 scratch repo 用真实 git 跑，即使结构断言仍过也能捕获坏 pattern。这是我在第一轮不会想到的好补充。 发现： 无关键阻塞项，无规范违反。 测试文件遵循已有的 helper-test 模式（node:test + node:assert/strict，yaml 包已被 classify-release-notes.test.mjs 使用）。三个 describe 块清晰对应三个不变量族：agent 设置、fork-PR 路由、git 清理。行为测试构造良好——同时设置 plumbing key（必须保留）和每个 exec 家族的向量（必须被清除），然后跑 workflow 自己的 grep + unset 管道，而非 JS 重实现。 allowlist 正则合理。只保留不携带命令的 plumbing——repo 格式标志、remote/branch/fetch/gc/pack/index、safe.directory、extensions、以及 submodule url/active/branch。submodule.*.update 被正确排除（可为 !cmd）。remote.origin.url 保留是因为 actions/checkout 会在 fetch 前覆写它。workflow 中的注释已更新，清晰解释了新方案。 ci.yml 改动是 HELPER_TESTS 的一行追加——最小且正确。 测试review 时的 CI 检查（commit 71526a2）：precheck 成功，Classify PR 成功，label 成功，route 成功。ubuntu 测试套件（运行 HELPER_TESTS 含新测试文件）在 review 时仍在运行（轮询约 10 分钟未完成）。未验证：新测试是否在 CI 中通过。 未触及 TUI 表面——纯 CI/测试 PR，不需要 tmux 实测。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 4/5 — clean, focused PR that regression-guards invariants which already broke once; only nit is CI hadn't finished at review time. This is the right follow-up to #7646/#7647. The original bug (settings_json silently dropped, agent ran unrestricted) had no test to catch it, and the denylist kept sprouting new gaps during review. Both problems are real and observed — not theoretical hardening. The test suite is well-built. Structural assertions parse the actual workflow YAML and check the invariants that matter (settings key name, tool allowlist, deny rules, fork-PR routing, cleanup step). The behavioral test goes further — it extracts the workflow's real allowlist regex and runs it against a scratch repo with actual git, so a broken pattern fails even when the structural checks pass. That's the kind of test that earns its keep in six months when someone edits the regex without fully understanding it. The allowlist flip is the right architectural call. The denylist was unwinnable — every review surfaced another exec family. Keeping only plumbing that carries no command (repo format, remote/branch/fetch, safe.directory, submodule url/active/branch) and unsetting everything else closes the whole class, including knobs not yet enumerated. The deliberate exclusion of submodule.*.update (can be !cmd) shows the author thought about the edge cases. Scope is tight — 3 files, every edit necessary, no drive-by changes. The code follows the established helper-test conventions (node:test, yaml package, HELPER_TESTS wiring). If I had to maintain this in six months, I'd thank the author. Non-blocking nit: the ubuntu test suite (which runs HELPER_TESTS including this new file) was still in_progress after ~10 minutes of polling at review time. The code looks correct, but CI confirmation is pending. 中文说明置信度：4/5 —— 干净、聚焦的 PR，为已经出过问题的不变量做回归防护；唯一小问题是 review 时 CI 尚未完成。 这是 #7646/#7647 的正确后续。原始 bug（settings_json 被静默丢弃，agent 全权限运行）没有测试能拦截，denylist 在 review 中不断冒出新漏洞。两个问题都是已观测到的——不是理论加固。 测试套件构建良好。结构断言解析真实 workflow YAML，检查关键不变量（settings key 名、工具白名单、deny 规则、fork-PR 路由、清理步骤）。行为测试更进一步——提取 workflow 真实的 allowlist 正则，对 scratch repo 用真实 git 跑，所以坏 pattern 即使结构检查仍过也会失败。这种测试在六个月后有人不完全理解地编辑正则时会发挥作用。 allowlist 翻转是正确的架构决策。denylist 追不完——每次 review 都冒出新的 exec 家族。只保留不携带命令的 plumbing（repo 格式、remote/branch/fetch、safe.directory、submodule url/active/branch），其余全部 unset，一次封死整类，包括尚未枚举的。刻意排除 submodule.*.update（可为 !cmd）说明作者考虑了边界情况。 范围紧凑——3 个文件，每处改动都必要，无顺手改动。代码遵循已有的 helper-test 规范。如果六个月后我来维护，我会感谢作者。 非阻塞小问题：ubuntu 测试套件（运行 HELPER_TESTS 含新文件）在 review 时轮询约 10 分钟仍在运行。代码看起来正确，但 CI 确认待定。 — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| for (const vec of [ | ||
| 'hookspath', | ||
| 'core.pager', | ||
| 'fsmonitor', | ||
| 'filter.', | ||
| 'url.https', | ||
| 'credential', | ||
| 'includeif', | ||
| 'include.path', | ||
| 'alias.', | ||
| 'submodule.s.update', | ||
| 'sequence.editor', | ||
| 'diff.external', | ||
| ]) { |
There was a problem hiding this comment.
[Suggestion] The behavioral test's "must be unset" list omits four exec vectors that were explicitly enumerated in the old denylist and named in the workflow's own comment: init.templatedir, core.sshcommand, core.editor, and pager.* (non-core pager prefix). — Concrete cost: if a future edit adds e.g. init\. to the allowlist (to keep init.defaultBranch), init.templatedir would silently survive cleanup — and no test assertion would catch it, because it's not in this list. The regression guard would miss the very regression class it was written to prevent.
| for (const vec of [ | |
| 'hookspath', | |
| 'core.pager', | |
| 'fsmonitor', | |
| 'filter.', | |
| 'url.https', | |
| 'credential', | |
| 'includeif', | |
| 'include.path', | |
| 'alias.', | |
| 'submodule.s.update', | |
| 'sequence.editor', | |
| 'diff.external', | |
| ]) { | |
| for (const vec of [ | |
| 'hookspath', | |
| 'core.pager', | |
| 'core.sshcommand', | |
| 'core.editor', | |
| 'fsmonitor', | |
| 'filter.', | |
| 'url.https', | |
| 'credential', | |
| 'includeif', | |
| 'include.path', | |
| 'init.templatedir', | |
| 'pager.', | |
| 'alias.', | |
| 'submodule.s.update', | |
| 'sequence.editor', | |
| 'diff.external', | |
| ]) { |
(Also add the corresponding set() calls in before():
set('init.templateDir', '/evil');
set('core.sshCommand', 'evil');
set('core.editor', 'evil');
set('pager.diff', 'evil');
```)
_— qwen3.7-max via Qwen Code /review_
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
4 participants
Follow-up to #7646 / #7647. Two things, both about making the triage hardening durable.
1. A regression harness for the workflow (the main deliverable)
The root bug those PRs fixed —
settings_json:instead ofsettings:, silently dropped so the agent ran unrestricted — had no test to catch it. Several other invariants were equally unguarded. Newnode:testsuite (.github/scripts/qwen-triage-workflow.test.mjs, wired into the sharedHELPER_TESTSlist both CI paths run) parsesqwen-triage.ymland asserts:settings:, neversettings_json:;tools.coreregisters the read/analysis tools and does not registerweb_fetch/web_search/save_memory;permissions.denyblocks interpreters, network, and thegit/ghwrite subcommands that materialize PR code;head.repo.full_name == github.repositorywith anubuntu-latestfallback;Negative-controlled: renaming
settings:back tosettings_json:makes the suite fail.2. Git cleanup: denylist → keep-known-safe allowlist
The review on #7647 kept surfacing new git-config exec families the denylist missed (
pager,filter.*,includeIfsubsections,url.*,credential.helper, …). Chasing them one by one is unwinnable, so this flips to an allowlist:--unset-allevery local key that isn't plumbingactions/checkoutneeds —— which closes the whole class, including knobs not yet enumerated.
submodule.*.update(can be!cmd) is deliberately not kept.remote.origin.urlis kept becauseactions/checkoutoverwrites it before any fetch, so a plantedext::transport URL is replaced.The harness doesn't just check the pattern is present — it extracts the workflow's actual allowlist regex and runs it against a scratch repo, asserting every exec family (hooksPath, pager, fsmonitor, filter, url, credential, include/includeIf, alias, submodule.update, sequence.editor, diff.external) is unset and the checkout plumbing (repo format, remote, branch, safe.directory, submodule.url) survives. So a broken pattern fails the test even when the structural checks still pass.
Verification
node --test .github/scripts/qwen-triage-workflow.test.mjs→ 26/26 pass; negative control fails as expected. Allowlist behavior verified against realgitin a scratch repo. The one thing a unit test can't exercise is a liveactions/checkouton a reused ECS workspace — but the kept set is exactly what checkout re-establishes, and checkout rebuilds remote/auth/fetch regardless.中文说明
承接 #7646 / #7647,让 triage 加固可持续。
1. workflow 回归测试(主交付)
那两个 PR 修的根因(
settings_json:写成settings:、被静默丢弃导致 agent 全权限运行)当时没有任何测试能挡。新增node:test套件(.github/scripts/qwen-triage-workflow.test.mjs,接入两条 CI 路径共用的HELPER_TESTS),解析qwen-triage.yml并断言:settings:,绝不用settings_json:;tools.core注册读/分析工具,且不注册web_fetch/web_search/save_memory;permissions.deny封解释器、网络,以及会落地 PR 代码的git/gh写子命令;head.repo.full_name == github.repository,并有ubuntu-latest兜底;带负向对照:把
settings:改回settings_json:,套件会失败。2. git 清理:denylist → 保留白名单
#7647 的 review 不断发现 denylist 漏掉的 git-config exec 家族(
pager、filter.*、includeIf子段、url.*、credential.helper……)。逐个追不完,所以改成白名单:--unset-all掉所有非actions/checkout所需 plumbing 的本地 key,一次封死整类(含尚未枚举的)。submodule.*.update(可为!cmd)刻意不保留;remote.origin.url保留是因为 checkout 会在 fetch 前覆写它。测试不只检查 pattern 存在 —— 它抽取 workflow 真实的白名单正则,对 scratch repo 实跑,断言每个 exec 家族被清除、且 checkout plumbing 保留。pattern 写坏时即便结构断言仍过,行为测试也会失败。
验证
node --test→ 26/26 通过;负向对照如期失败;白名单行为对真实git验证。单测唯一覆盖不到的是 ECS 复用工作区上的真实actions/checkout,但保留集正是 checkout 会重建的那些。