fix(review): harden SKILL.md against weak-model rule skipping#4340
Conversation
Weak models often skip parts of the long /review prompt and fall back to familiar defaults — `gh pr checkout` instead of the worktree flow, or running the autofix prompt even when the user passed `--comment` (which means "only post inline comments, don't mutate code"). Three reinforcements, all in SKILL.md (no CLI changes): - Promote the two most commonly violated rules to the top of the "Critical rules" list: worktree is mandatory for PR reviews, and `--comment` skips Step 8 entirely. - Add an inline blockquote at the top of the Step 1 PR branch that names the specific forbidden commands (`gh pr checkout`, `git checkout`, `git switch`, `git pull`, `git reset --hard`). - Add an explicit skip block at the top of Step 8 listing the three conditions that bypass autofix — `--comment`, cross-repo lightweight mode, or no fixable findings — so a weak model doesn't have to infer them from scattered earlier text.
📋 Review SummaryThis PR strengthens the SKILL.md review skill by promoting two commonly-violated rules to the top "Critical rules" section and adding explicit guard blocks to prevent weak/fast models from skipping key constraints. The changes are documentation-only (no CLI code changes) and address real observed failure modes: models switching the user's working tree instead of using the mandatory worktree flow, and running autofix prompts when 🔍 General Feedback
🎯 Specific Feedback🔵 Low
✅ Highlights
|
There was a problem hiding this comment.
Pull request overview
Reinforces the /review bundled skill instructions to prevent weak models from skipping key rules, specifically around mandatory PR worktree isolation and disabling autofix behavior when --comment is used.
Changes:
- Promotes worktree isolation and
--comment→ skip autofix guidance to the top “Critical rules” section. - Adds an explicit “MANDATORY worktree flow” warning block in Step 1, including a list of forbidden git commands that mutate the user’s working tree.
- Adds a prominent “Skip Step 8” header in the Autofix section enumerating bypass conditions (
--comment, cross-repo lightweight mode, no fixable findings).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Follow-up to the initial harden pass, addressing the inline review comments on PR QwenLM#4340. Rule #1 (worktree mandatory): - Scope it to **same-repo PR reviews** so cross-repo PRs running in lightweight mode (no matching local remote, no worktree) don't read as a contradiction. - Replace "Your very first action" with "After argument parsing and remote detection, the first command that touches code state" — the literal "very first" was wrong since `--comment` parsing and URL/remote disambiguation legitimately run before `fetch-pr`. - Align the forbidden-command list with the Step 1 blockquote (add `git pull` and `git reset --hard`) so a weak model that only reads the Critical rules section sees the same five commands as a model that reaches the blockquote at the point of use. - Add an explicit "cross-repo PRs use lightweight mode" parenthetical so the same model knows where to look for the alternative path. Step 8 skip block: - Drop the redundant third bullet ("no Critical or Suggestion findings with concrete, applicable fixes") — it was both logically equivalent to the "Otherwise" clause below and used a different qualifier ("concrete, applicable" vs "clear, unambiguous"), risking a weak model treating them as two distinct thresholds. - "ANY of the following" → "EITHER" since only two bullets remain. - Fold the no-findings case into the Otherwise clause as a no-op note.
pomelo-nwu
left a comment
There was a problem hiding this comment.
Solid hardening of the review skill, and the approach is right — promoting the two most-violated rules to the top "Critical rules" block and restating the Step 8 skip at its point-of-use is exactly how you make a long prompt robust against weaker/faster models that don't read the whole thing.
A few notes:
- Giving worktree-isolation slot #1 is well justified:
gh pr checkout/git switchcontaminating the user's working tree is a far worse failure than a stylistic slip, so it earns the top slot over the language rule. - The redundancy between Critical rule #2 and the new Step 8 skip block is good, not duplication — a model that jumps straight to Step 8 still sees the
--commentskip. The PR body already calls this out. - Minor: the forbidden-command list is now stated in both Critical rule #1 and the Step 1 blockquote. If they ever drift, a weak model may trust whichever it reads first. Consider keeping the authoritative list in one place and referencing it from the other — and adding the note that
git fetch <remote> pull/<n>/head:<ref>is the one allowed fetch (it's in the PR description but not in SKILL.md itself).
Direction and content both LGTM.
中文
对 review skill 的合理强化,方向正确:把两条最常违反的规则提到顶部 Critical rules,并在 Step 8 处就地重申跳过条件,正是让长 prompt 对弱/快模型更稳健的做法。
- worktree 隔离规则排到 #1 合理:
gh pr checkout/git switch污染用户工作树比风格问题严重得多,值得压过语言规则居首。 - Critical #2 与新增 Step 8 跳过块的冗余是有意为之、并非重复——直接跳到 Step 8 的模型仍能看到
--comment跳过规则,PR 描述已说明。 - 小建议:禁用命令清单现在同时出现在 Critical #1 和 Step 1 引用块两处,若日后不同步,弱模型可能信它先读到的那份。可考虑只在一处维护权威清单、另一处引用;并补一句
git fetch <remote> pull/<n>/head:<ref>是唯一允许的 fetch(你在 PR 描述提了,但 SKILL.md 正文没写)。
方向与内容都 LGTM。
Summary
Weak models (and small fast models) running
/reviewsometimes skip parts of the long SKILL.md prompt and fall back to familiar defaults:gh pr checkout/git checkout <branch>to switch the working tree into the PR, instead of going throughqwen review fetch-prwhich isolates the PR into an ephemeral worktree. This contaminates the user's local branch state.--comment, which is supposed to mean "only post inline PR comments, do not touch code".Both of these are already implied by SKILL.md, but the relevant rules are scattered in the middle of long sections and easy to miss. This PR reinforces them with three small additions — no CLI changes:
--commentskips Step 8 entirely. Existing language/API rules become 如何自定义密钥文件 .env可能与其他文件冲突 #3 and Are you interested in AI Terminal? #4.⚠️ MANDATORY worktree flowblockquote that names the specific forbidden commands (gh pr checkout,git checkout <branch>,git switch,git pull,git reset --hard) and points atqwen review fetch-pras the only entry point.--commentspecified, cross-repo lightweight mode, or no fixable findings. A weak model now sees the skip rule before it sees "If there are Critical or Suggestion findings…".Notes
git fetch <remote> pull/<n>/head:<ref>is not in the forbidden list — it doesn't touch the working tree and is whatqwen review fetch-pritself uses internally.allowedToolschanges, so the existingbundled-skills.integration.test.tsparse test continues to cover this file.Test plan
/review <pr-number>with a smaller/faster model and confirm it uses the worktree at.qwen/tmp/review-pr-<n>(verify withgit worktree list) and does not switch the user's HEAD./review <pr-number> --commentwith the same model and confirm Step 8 is skipped silently (no "Apply auto-fixes? (y/n)" prompt) and flow goes Step 7 → Step 9 directly./review <pr-number>(without--comment) and confirm Step 8 still triggers as before for fixable findings.