Skip to content

fix(triage): never auto-approve cross-repo refactor PRs - #5744

Merged
yiliang114 merged 1 commit into
QwenLM:mainfrom
wenshao:fix/triage-block-fork-refactor-approve
Jun 23, 2026
Merged

fix(triage): never auto-approve cross-repo refactor PRs#5744
yiliang114 merged 1 commit into
QwenLM:mainfrom
wenshao:fix/triage-block-fork-refactor-approve

Conversation

@wenshao

@wenshao wenshao commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Adds an approval guardrail to the /triage skill so it never auto-approves a cross-repository (fork) PR whose title is a refactor type. Right before the Stage 3 approve step, triage runs a deterministic check on the PR's isCrossRepository flag and its title; when both match it skips gh pr review --approve and escalates to the maintainer instead. Approval is now gated on a positive condition (the guard must explicitly return ok), so a blocked, failed, or empty check never results in an approval. The rule is also documented in the skill's global Rules section so it is visible at the skill entry point.

Why it's needed

A fork refactor PR (#5089, refactor(core): extract Protocol enum and decouple model identity from auth type) was auto-approved by triage and merged without a human maintainer reviewing the structural changes. Refactors touch code broadly and a fork author is not a trusted committer, so this class of PR should always get a human decision rather than an automated LGTM. This adds that safety net for future triage runs — both the GitHub Action (@qwen-code /triage, PR-opened auto-triage) and local /triage.

Reviewer Test Plan

How to verify

The guard is the jq expression added to Stage 3 of the PR workflow. It returns block only when the PR is cross-repository and the title starts with refactor (case-insensitive). Run it against the motivating PR:

# Real data: fork + "refactor(core): ..." -> block
gh pr view 5089 --repo QwenLM/qwen-code --json isCrossRepository,title \
  --jq 'if (.isCrossRepository and (.title | test("^\\s*refactor"; "i"))) then "block" else "ok" end'
# => block

Expected results for synthetic metadata:

isCrossRepository title result
true refactor(core): x block
true refactor!: x block
true Refactor the x (no colon) block
true feat: refactor the x ok
false refactor(core): x ok
true fix: a bug ok

Evidence (Before & After)

N/A — this is a skill documentation (prompt) change with no user-visible / TUI surface.

Tested on

OS Status
🍏 macOS
🪟 Windows N/A
🐧 Linux N/A

Verified the guard expression against PR #5089 and 9 boundary cases on macOS. Windows/Linux marked N/A: this is a markdown-only skill doc and the check is plain gh + jq, which is platform-independent.

Environment (optional)

Local gh + jq. No app runtime involved.

Risk & Scope

  • Main risk or tradeoff: the guard keys off the conventional-commit refactor title prefix, so a refactor mislabeled as feat: / chore: would not be caught. This is the known ceiling of a title-based, skill-level rule.
  • Not validated / out of scope: hard enforcement at the GitHub Actions layer (e.g. auto-dismissing an approval that slips through) is intentionally not part of this PR.
  • Breaking changes / migration notes: none. Only affects whether triage auto-approves; every other triage behavior is unchanged.

Linked Issues

References #5089 as the motivating incident (no closing keyword — that PR is already merged).

中文说明

这个 PR 做了什么

/triage skill 加了一道 approve 护栏:对来自 fork(跨仓库)且标题是 refactor 类型的 PR,绝不自动 approve。在 Stage 3 执行 approve 之前,triage 会对 PR 的 isCrossRepository 标志和标题做一次确定性判定;两者同时命中时跳过 gh pr review --approve,改为升级给维护者。现在 approve 是"正向条件"——护栏必须明确返回 ok 才批,因此判定被拦、失败或为空时都不会 approve。该规则同时写进 skill 的全局 Rules 区,确保在 skill 入口处就可见。

为什么需要

一个 fork 的 refactor PR(#5089,refactor(core): extract Protocol enum and decouple model identity from auth type)被 triage 自动 approve 并合入,期间没有维护者人工审查这些结构性改动。重构改动面广,而 fork 作者并非可信的提交者,所以这类 PR 应当始终交由人来决定,而不是自动 LGTM。本 PR 为未来的 triage(GitHub Action 的 @qwen-code /triage、PR 打开时的自动 triage,以及本地 /triage)补上这道保护。

审查测试计划

如何验证

护栏就是 PR workflow Stage 3 中新增的那段 jq 表达式:仅当 PR 来自跨仓库标题以 refactor 开头(大小写不敏感)时返回 block。对触发本次问题的 PR 运行:

# 真实数据:fork + "refactor(core): ..." -> block
gh pr view 5089 --repo QwenLM/qwen-code --json isCrossRepository,title \
  --jq 'if (.isCrossRepository and (.title | test("^\\s*refactor"; "i"))) then "block" else "ok" end'
# => block

构造元数据的预期结果:

isCrossRepository title 结果
true refactor(core): x block
true refactor!: x block
true Refactor the x(无冒号) block
true feat: refactor the x ok
false refactor(core): x ok
true fix: a bug ok

证据(改动前后)

N/A —— 这是 skill 文档(prompt)改动,无用户可见 / TUI 界面。

测试平台

仅在 macOS 上对 PR #5089 及 9 个边界用例验证了护栏表达式。Windows/Linux 标 N/A:纯 markdown 文档改动,判定只用 gh + jq,与平台无关。

环境(可选)

本地 gh + jq,不涉及应用运行时。

风险与范围

  • 主要风险 / 取舍:护栏依据 conventional-commit 的 refactor 标题前缀判定,因此被误标成 feat: / chore: 的重构不会被拦——这是"基于标题、skill 层规则"方案的固有上限。
  • 未验证 / 范围外:GitHub Actions 层的硬性兜底(例如自动 dismiss 漏网的 approve)有意不在本 PR 内。
  • 破坏性改动 / 迁移说明:无。仅影响 triage 是否自动 approve,其余行为不变。

关联 Issue

引用 #5089 作为触发本次改动的事件(不带关闭关键字——该 PR 已合并)。

A fork (cross-repository) PR whose title is a `refactor` type could be
auto-approved by the /triage skill and merged without a maintainer
reviewing the structural changes (this happened with QwenLM#5089).

Add a deterministic approval guardrail to Stage 3: before approving,
check `isCrossRepository && title ~ /^refactor/i`; on a match, skip
`gh pr review --approve` and escalate to the maintainer instead.
Approval is now a positive condition (the guard must explicitly pass),
so a blocked or empty check never approves. Document the rule in the
skill's global Rules section as well.
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR, @wenshao!

Template looks good ✓ — all required headings present, bilingual body, test plan table, risk section.

On direction: this is a well-motivated safety net. The incident it references (#5089 — a fork refactor that got auto-approved and merged without human review) is exactly the kind of process gap that should be closed. The scope is narrow (just the triage skill prompt, no code changes), which keeps risk low. Aligned with the project's goal of having triage act as a responsible gatekeeper, not a rubber stamp.

On approach: the diff is tight — 16 additions across 2 files, adding the rule in both the global Rules section (SKILL.md) and the Stage 3 approval flow (pr-workflow.md). The "positive condition" design (approve only when guard explicitly returns ok) is solid defensive prompting. One minor observation: the regex ^\s*refactor without a word boundary would also match a title like refactoring: clean up utils, which isn't a standard conventional-commit type. A \b after refactor would tighten it. Not a blocker — conventional commit tooling rarely produces "refactoring" as a type — but worth considering.

Moving on to code review. 🔍

中文说明

感谢贡献,@wenshao

模板完整 ✓ — 所有必需标题齐全,双语正文,测试计划表格,风险区。

方向:动机明确。引用的事件(#5089 — fork refactor 被自动 approve 并合入,未经人工审查)正是应当堵住的流程漏洞。改动范围窄(仅 triage skill prompt,无代码变更),风险低。与项目"triage 应负责任地把关,而非盲目盖章"的目标一致。

方案:diff 精简 — 2 个文件共 16 行新增,同时在全局 Rules 区(SKILL.md)和 Stage 3 审批流程(pr-workflow.md)中加了规则。"正向条件"设计(仅当护栏明确返回 ok 才 approve)是稳健的防御性 prompt 写法。一个小观察:正则 ^\s*refactor 没有 word boundary,也会匹配 refactoring: clean up utils 这类标题,而 "refactoring" 并非标准 conventional-commit type。在 refactor 后加 \b 会更精确。不是 blocker — 常规 commit 工具很少产生 "refactoring" 这种 type — 但值得考虑。

进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code Review

This is a clean, focused change. Two files, 16 additions, 2 deletions — all in the triage skill prompt. No code, no build system, no dependencies.

The approach is sound: the guard lives in exactly the right place (right before the approve step in Stage 3), and the rule is also surfaced in the global Rules section for discoverability. The "positive condition" pattern — requiring GUARD == ok to proceed with approval rather than just checking for GUARD != block — is good defensive prompting. If the jq expression ever fails or returns empty, approval is correctly withheld.

One minor nit (already flagged in Stage 1): the regex ^\s*refactor lacks a word boundary, so a title like refactoring: clean up would be a false positive. Not a blocker given that "refactoring" is not a conventional commit type, but a \b would be tighter.

No correctness bugs, no security concerns, no structural issues.

Real-Scenario Testing

N/A — this is a skill documentation (prompt) change with no user-visible or TUI surface. There is no binary behavior to test in tmux.

Instead, verified the guard expression against the motivating incident (PR #5089) and synthetic boundary cases:

# Real incident: fork + "refactor(core): extract Protocol enum..." => block
$ gh pr view 5089 --repo QwenLM/qwen-code --json isCrossRepository,title \
    --jq 'if (.isCrossRepository and (.title | test("^\\s*refactor"; "i"))) then "block" else "ok" end'
block

# Synthetic boundary cases:
cross=true  title='refactor(core): x'       => block ✓
cross=true  title='refactor!: x'            => block ✓
cross=true  title='Refactor the x'          => block ✓
cross=true  title='feat: refactor the x'    => ok   ✓
cross=false title='refactor(core): x'       => ok   ✓
cross=true  title='fix: a bug'              => ok   ✓

All 6 boundary cases plus the real incident produce the expected result.

中文说明

代码审查

改动干净聚焦:2 个文件,16 行新增,2 行删除 — 全部在 triage skill prompt 中。无代码、无构建系统、无依赖。

方案合理:护栏放在 Stage 3 中最合适的位置(approve 步骤之前),规则同时在全局 Rules 区露出,便于发现。"正向条件"模式 — 要求 GUARD == ok 才能 approve,而不是只检查 GUARD != block — 是良好的防御性 prompt 写法。如果 jq 表达式出错或返回空值,approve 会被正确阻止。

一个小瑕疵(Stage 1 已提到):正则 ^\s*refactor 缺少 word boundary,refactoring: clean up 这样的标题会被误拦。鉴于 "refactoring" 不是标准 conventional commit type,不构成 blocker,但加 \b 更严谨。

无正确性 bug、无安全隐患、无结构性问题。

真实场景测试

N/A — 这是 skill 文档(prompt)改动,无用户可见 / TUI 界面。没有可以在 tmux 中测试的二进制行为变化。

改为对触发事件(PR #5089)和构造边界用例验证护栏表达式:

# 真实事件: fork + "refactor(core): extract Protocol enum..." => block
$ gh pr view 5089 ... => block

# 构造边界用例 (6 个):
全部符合预期 ✓

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Stepping back: this PR is what good open-source contribution looks like. A contributor hit a real process gap (fork refactor #5089 getting auto-approved), diagnosed the root cause precisely, and shipped the minimal fix. The diff is 16 lines across 2 markdown files — no code, no dependencies, no risk of breaking anything.

The guard expression is correct and deterministic. I verified it against the real incident (PR #5089: correctly blocked) and 6 synthetic boundary cases (all pass). The "positive condition" design — requiring explicit ok rather than just absence of block — is the right call for a safety-critical prompt.

The only reservation I have is the missing \b word boundary in the regex, which could false-positive on a refactoring: title. This is genuinely minor: "refactoring" isn't a conventional commit type, and a false positive here just means the maintainer gets asked to look at it manually — which is the safe failure mode anyway.

My independent proposal would have been slightly broader (also blocking revert and similar structural types from forks), but the PR's narrower scope is actually better — it fixes the specific incident without speculative scope creep, and the pattern is easy to extend later if needed.

Ships it. ✅

中文说明

退一步看:这是一个好的开源贡献的范例。贡献者遇到了真实的流程漏洞(fork refactor #5089 被自动 approve),精确诊断了根因,然后提交了最小修复。diff 只有 2 个 markdown 文件共 16 行 — 无代码、无依赖、无破坏风险。

护栏表达式正确且确定。我用真实事件(PR #5089:正确拦截)和 6 个构造边界用例(全部通过)做了验证。"正向条件"设计 — 要求明确返回 ok 而非仅 absence of block — 对安全关键的 prompt 来说是正确的选择。

唯一的保留意见是正则缺少 \b word boundary,可能对 refactoring: 标题产生误拦。这确实很轻微:"refactoring" 不是标准 conventional commit type,而且误拦的结果只是让维护者手动看一下 — 本身就是安全的失败模式。

我自己的独立方案会稍宽一些(也拦截 fork 的 revert 等结构性类型),但 PR 的窄范围其实更好 — 修了具体问题,没有投机性扩大范围,以后需要扩展也容易。

可以合入。✅

Qwen Code · qwen3.7-max

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

@yiliang114 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yiliang114
yiliang114 merged commit 98748e5 into QwenLM:main Jun 23, 2026
28 checks passed
@wenshao
wenshao deleted the fix/triage-block-fork-refactor-approve branch June 23, 2026 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants