fix(review): don't overclaim a submit-gate bypass on a same-account write - #7718
Merged
Conversation
…rite The cleanup bypass audit lists reviews and issue comments by the reviewing account inside the review window that the submit receipt does not vouch for. That set legitimately includes writes this review never made: a concurrent `/review`, a triage/autofix bot, or the user themselves, all posting under the same account, produce exactly the same shape — the audit cannot tell them apart from a genuine `gh pr review` bypass by metadata alone. Observed in practice: an observe-only (`no --comment`) quick pass that posted nothing had a concurrent same-account CHANGES_REQUESTED review land in its window, and the warning — relayed verbatim into the review summary — read "a write bypassed the submit gate", alarming over what was an external write. Reframe the footer so it no longer asserts a bypass as the default reading: the likely cause is benign (named account, another workflow, or a bot), and a write here is a real gate bypass only if its content is this review's own output. The detection logic and the per-write detail lines are unchanged, so nothing that was surfaced before is hidden now — only the conclusion the copy draws is corrected.
Collaborator
Collaborator
|
Thanks for the PR! Template: headings differ from the template ( Problem: observed, not theoretical. During dogfooding, an observe-only Direction: aligned. This is a copy fix for the review cleanup audit's warning footer — the detection stays fail-safe, only the conclusion the copy draws is corrected. Within scope. Size: 13 production logic lines ( Approach: exactly the right scope — only the footer text changes, detection logic ( Moving on to code review. 🔍 中文说明感谢贡献！ 模板：标题与模板不同（ 问题：已观测到的，非理论性的。dogfooding 期间，一次只观察的 方向：对齐。这是 review cleanup 审计告警页脚的文案修正——检测保持 fail-safe，只纠正文案所下的结论。在范围内。 规模：13 行生产逻辑（ 方案：范围恰好——只改页脚文案，检测逻辑（ 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
Code ReviewIndependent proposal: given the problem (footer overclaims a gate bypass on a same-account write), I would rewrite the footer text to lead with the benign explanation, name the account, qualify the bypass claim, and preserve the relay instruction. Then update the SKILL.md doc to match and extend the test. That's exactly what this PR does — no simpler path missed. Findings: no blockers. The new footer text is a clear improvement: it leads with "The likely cause is benign", names the account via the interpolated login, and qualifies — "a real bypass of that gate only if its content is this review's own output" — instead of asserting a bypass outright. The relay instruction ("Relay this warning verbatim") is preserved, so the warning still reaches the human. The test additions are well-designed: pinning the interpolation shape The SKILL.md update is consistent — same reframing, same qualification. The comment update on Detection logic, audit window, and per-write detail lines are all unchanged — nothing that surfaced before is hidden now. TestingThis is a copy/wording change to an internal review tool's warning output — nothing user-visible in the TUI. Real-scenario testing: N/A. CI evidence (from the PR's own checks on the reviewed commit):
Ubuntu unit tests pass. Windows/macOS/integration skipped (by design for this change). No failures. 中文说明代码审查独立方案： 给定问题（页脚对同账号写入过度声称 gate 绕过），我会重写页脚文案，以良性解释开头、具名账号、限定绕过说法，并保留转达指令。然后同步更新 SKILL.md 文档并扩展测试。这正是本 PR 所做的——没有遗漏更简路径。 发现： 无阻塞项。 新页脚文案是明确的改进：以"The likely cause is benign"开头，通过插值的登录名具名账号，并限定——"a real bypass of that gate only if its content is this review's own output"——而非直接断言绕过。转达指令（"Relay this warning verbatim"）保留，告警仍会到达人类。 测试新增设计良好：固定插值形状 SKILL.md 更新一致——同样的重构、同样的限定。
检测逻辑、审计窗口、每条写入的明细行均未改动——之前会浮现的东西现在一样浮现。 测试这是内部 review 工具告警输出的文案修改——TUI 中无用户可见变化。真实场景测试：N/A。 CI 证据来自 PR 自身在受审 commit 上的检查。Ubuntu 单元测试通过，Windows/macOS/集成测试按设计跳过。无失败。 — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
|
Confidence: 5/5 — clean across every stage; would merge without hesitation. The problem is real and observed — a concurrent same-account write during an observe-only review run produced a misleading "you bypassed the submit gate" warning. The fix is exactly right: only the footer copy changes, the fail-safe detection stays untouched, and the new wording leads with the benign explanation while still surfacing genuine bypasses for a human to judge. The test pins the new framing well (especially the interpolation shape check), the SKILL.md doc is consistent, and CI is green. Minimal, focused, well-motivated — this is the kind of PR that makes the tooling less alarming without making it less safe. 中文说明置信度：5/5 —— 每个阶段都干净；毫不犹豫合并。 问题是真实且已观测到的——一次只观察的 review 运行期间，同账号并发写入产生了误导性的"你绕过了 submit gate"告警。修复恰好：只改页脚文案，fail-safe 检测不动，新措辞以良性解释开头，同时将真正的绕过浮现给人类判断。测试很好地固定了新措辞（特别是插值形状检查），SKILL.md 文档一致，CI 全绿。最小、聚焦、动机充分——这类 PR 让工具不再那么吓人，同时不降低安全性。 — Qwen Code · qwen3.8-max-preview Reviewed at |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
approved these changes
Jul 25, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
previously approved these changes
Jul 25, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.7-max via Qwen Code /review
Collaborator
Author
Review — copy-only fix to the bypass-audit footerNo blockers. The diff is one template literal in Verified locally at
|
| Mutation applied at head | Result |
|---|---|
| footer reverted to the base wording | ✗ fails on likely cause is benign → the new assertions are load-bearing |
drop only (${me}) from the footer |
✗ fails on toContain('reviewer') → the account-name interpolation is pinned |
drop Relay this warning verbatim … so a human can judge. |
✓ 25/25 still green → that sentence is pinned by nothing |
Findings (all nits)
1. cleanup.ts:359 — "you" is ambiguous, and ambiguous in the wrong direction.
This string has two readers: the model that reads cleanup's stdout, and the user who reads the relayed summary. To the user, "you" is the user — intended. To the model, "you" is itself, and the model posting under the reviewing account is precisely the bypass this tripwire exists to catch (the dogfooded case in the docstring at cleanup.ts:100-105). So for the one reader who might be the offender, the copy now opens by calling that case benign. The base wording ("if the user did this themselves") had no such reading. Third person removes it with no loss:
The likely cause is benign — the user (from another terminal), another workflow,
or a bot posting under the same account (${me}) produces exactly this shape.
2. cleanup.ts:71-72 — stale quote left behind.
The CLOCK_SKEW_MS docstring justifies erring toward over-flagging with "the warning copy already covers "the user did this themselves"". That sentence no longer exists in the warning copy after this PR. Requote it, or point at the new phrasing.
3. "or a bot" is true for only one of the two channels.
isAutomationComment already drops issue comments whose first line carries <!-- qwen-, so this repo's marked bot comments never reach this footer. findUnsanctionedReviews has no marker filter, so bot reviews do. The claim is accurate for reviews and for unmarked bot comments, mildly over-broad for exactly the ones the audit deliberately filters out.
4. cleanup.test.ts:556 — toContain('reviewer') is load-bearing today, coincidence-prone tomorrow.
It discriminates only because the header line says "the reviewing account", not "reviewer" (mutation row 2 above proves it currently bites). Any future copy edit that uses the word "reviewer" anywhere makes it silently vacuous. Pin the interpolation shape instead: toContain('(reviewer)').
5. The relay instruction is asserted nowhere in the suite (grep -n 'verbatim\|Relay' cleanup.test.ts → 0 hits; mutation row 3 confirms). It is the sentence that actually moves the warning to a human — the rest of the audit is inert without it. Pre-existing gap, but this PR rewrites that exact string, so it is the cheap moment to add expect(warnings.join('\n')).toContain('Relay this warning verbatim').
6. Design note, follow-up rather than this PR. The footer now delegates the discriminator — "a real bypass only if its content is this review's own output" — to the reader, and the first reader is the party that may be the offender. The file already does content-based discrimination one function up (isAutomationComment over RawIssueComment.body), and GET /pulls/{n}/reviews returns body as well (just not declared on RawReview). Deciding it mechanically would let the footer state a fact instead of ranking two readings in prose.
7. SKILL.md:961 still frames it bypass-first ("a write that bypassed the gate, something the user did by hand, or another workflow…") and carries none of the new discriminator. The model's interpretation comes from there at least as much as from the warning line; a one-sentence sync keeps the two surfaces from disagreeing.
Scope / risk
Copy only — no behavior, no API, no perf, no new security surface. ${me} is a gh login from currentUser(), and a failure there already unwinds into skipNote, so there is no empty-interpolation path in practice.
中文说明
审查——bypass 审计页脚的纯文案修正
无阻塞项。 diff 只有 auditPrWrites 里的一个模板字符串加三条断言;findUnsanctionedReviews / findUnsanctionedIssueComments、审计窗口、时钟偏移边界与每条写入的明细行确实未动,之前会浮现的东西现在一样浮现。前提成立:仅凭同账号元数据无法区分并发 /review、机器人、手动发帖与真正的 gh pr review 绕过,而原页脚却直接下了结论。
本地在 8d27b8f 上验证(隔离 worktree,packages/cli)
vitest run src/commands/review/cleanup.test.ts→ 25/25 全绿(与 PR 描述一致)- 两个改动文件的
eslint --max-warnings 0+prettier --check→ 干净
变异 A/B(检验新断言是否真的钉住新文案):
| 在 head 上施加的变异 | 结果 |
|---|---|
| 页脚回退为原措辞 | ✗ 在 likely cause is benign 处失败 → 新断言是有效的 |
仅删掉页脚里的 (${me}) |
✗ 在 toContain('reviewer') 处失败 → 账号名插值被钉住 |
删掉 Relay this warning verbatim … so a human can judge. |
✓ 仍然 25/25 全绿 → 这句话没有任何测试覆盖 |
问题(均为小项)
1. cleanup.ts:359 —— "you" 有歧义,且歧义方向不利。
这段字符串有两个读者:读取 cleanup 标准输出的模型,以及读到转达小结的用户。对用户来说 "you" 指用户——符合本意;对模型来说 "you" 指它自己,而模型以 reviewing account 发帖恰恰就是该 tripwire 要抓的绕过(见 cleanup.ts:100-105 的实测记录)。于是对那个可能就是肇事者的读者,文案开头把该情形称作良性。原措辞("if the user did this themselves")没有这种读法。改成第三人称即可消除,且无任何损失:
The likely cause is benign — the user (from another terminal), another workflow,
or a bot posting under the same account (${me}) produces exactly this shape.
2. cleanup.ts:71-72 —— 残留的过期引用。
CLOCK_SKEW_MS 的注释用 "the warning copy already covers "the user did this themselves"" 来论证倾向于过度标记,而该句在本 PR 后已不存在于告警文案中。请重新引用或指向新措辞。
3. "or a bot" 只对两个通道之一成立。
isAutomationComment 已经把首行带 <!-- qwen- 的 issue comment 过滤掉,所以本仓库带 marker 的机器人评论根本到不了这个页脚;而 findUnsanctionedReviews 没有 marker 过滤,机器人 review 会到。该说法对 review 和未带 marker 的机器人评论准确,对被刻意过滤掉的那部分则略微过宽。
4. cleanup.test.ts:556 的 toContain('reviewer') 今天有效,明天易失效。
它之所以有区分度,只是因为表头写的是 "the reviewing account" 而非 "reviewer"(上表第 2 行证明当前确实会咬住)。今后任何在文案里用到 "reviewer" 一词的改动都会让它悄悄变成空断言。建议改为钉住插值形状:toContain('(reviewer)')。
5. 转达指令在整个套件里没有任何断言(grep -n 'verbatim\|Relay' cleanup.test.ts → 0 命中;上表第 3 行已确认)。它是真正把告警送到人眼前的那一句,没有它审计的其余部分都是空转。这是既有欠账,但本 PR 恰好重写了这条字符串,是最省事的补上时机:expect(warnings.join('\n')).toContain('Relay this warning verbatim')。
6. 设计备注(后续项,非本 PR)。 页脚现在把判别标准——"只有当其内容是本次审查自己的产出时才是真正的绕过"——交给读者,而第一个读者正是可能的肇事方。同一文件上一层已经在做基于内容的判别(isAutomationComment 读 RawIssueComment.body),且 GET /pulls/{n}/reviews 同样返回 body(只是 RawReview 未声明)。把判别做成机械的,页脚就能陈述事实,而不必在散文里给两种读法排序。
7. SKILL.md:961 仍以绕过为先("a write that bypassed the gate, something the user did by hand, or another workflow…"),且完全没有新的判别标准。模型的理解至少同等程度来自那里而非这行告警;补一句保持两处不打架即可。
影响面 / 风险
纯文案——无行为、无 API、无性能、无新增安全面。${me} 来自 currentUser() 的 gh login,其失败已经收敛到 skipNote,实际不存在空插值路径。
Address review feedback on the bypass-audit warning reframe:
- The footer led with "you", but cleanup's stdout is read by the model as well
as relayed to the user. To the model "you" is itself, and the model posting
under the reviewing account is the exact bypass this tripwire catches — so the
copy opened by calling that case benign to the one reader who might be the
offender. Switch to third person ("the user (from another terminal), another
workflow, or a bot").
- Requote the CLOCK_SKEW_MS docstring: it justified over-flagging by pointing at
warning copy ("the user did this themselves") that this change removed.
- Sync SKILL.md Step 9: it still framed a flagged write bypass-first with none
of the new discriminator; lead with the external same-account reading and add
"a write that bypassed the gate only if its content is this review's own
output", matching the footer.
- Tests: pin the interpolation shape `(reviewer)` instead of the bare word
(the header also says "reviewing account", so the old assertion stayed green
even with the account name dropped), and assert the "Relay this warning
verbatim" sentence — the line that moves the warning to a human, previously
covered by nothing.
yiliang114
previously approved these changes
Jul 25, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Approve ✅
Correct fix for an overclaiming warning footer. The old wording implied a submit-gate bypass when the unvouched writes are usually benign same-account activity (concurrent review, triage bot, autofix). The new wording leads with 'likely cause is benign', names the account, and qualifies the bypass claim — a real bypass only if the content is this review's own output. Test assertions match the new language.
wenshao
dismissed stale reviews from yiliang114 and qwen-code-ci-bot
via
July 25, 2026 15:51
2d1a0db
Collaborator
Author
|
Thanks — the mutation A/B is exactly the check I'd want on a copy-only PR, and it caught a real gap. Addressed in
Deferred, on purpose:
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jul 25, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
yiliang114
approved these changes
Jul 25, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Re-approve ✅
New commit refines the previous approval:
- Third-person wording ('the user from another terminal' instead of 'you') — more accurate for terminal summary context
- SKILL.md synced with new footer wording
- Test hardening:
toContain('(reviewer)')pins the interpolation shape,toContain('Relay this warning verbatim')pins the relay instruction
All clean refinements, no concerns.
Collaborator
Review + live E2E verification report (real build, tmux, real GitHub audit)Verdict: correct and verified against the live scenario it was written for — I ran the real audit against a genuine external same-account write and the new footer frames it accurately while detection stays fully intact. The live E2E (this is the PR's exact dogfooding scenario, reproduced)Earlier today this account ( Three things verified at once: (1) detection unchanged — the external write is still flagged with full detail lines; (2) the account is named in the footer interpolation; (3) the conclusion is now true — this write really was benign-external, and under the old copy this same run would have concluded "a write bypassed the submit gate" about a comment the review never made. Code review reasoning
No concerns — small, honest fix that makes an alarm truthful without making it quieter. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The cleanup bypass audit (added in #7691) lists every review and issue comment by the reviewing account inside the review window that the submit receipt does not vouch for, and warns about them — the warning is relayed verbatim into the review summary. The detection is deliberately fail-safe, but the footer overclaims: it concludes "otherwise a write bypassed the submit gate", when the same set legitimately includes writes this review never made.
A same-account write from another source produces the exact same shape the audit flags — a concurrent
/review, a triage/autofix bot, or the user reviewing by hand, all posting under the same GitHub account — and metadata alone cannot distinguish that from a genuinegh pr reviewbypass.Observed while dogfooding: an observe-only (
no --comment) quick pass that posted nothing had a concurrent same-accountCHANGES_REQUESTEDreview land inside its window. The audit flagged it and the summary carried "a write bypassed the submit gate" — an alarming, misleading conclusion about what was an external write.What changed
Only the audit's footer copy. It now:
The detection logic (
findUnsanctionedReviews/findUnsanctionedIssueComments), the audit window, and the per-write detail lines are all unchanged — nothing that surfaced before is hidden now. Only the conclusion the copy draws is corrected, so a concurrent same-account write no longer reads as "you bypassed the submit gate" while a genuine bypass is still surfaced for a human to judge.Testing
cleanup.test.ts: the in-window-review test now also asserts the footer leads with the benign framing, names the account, and qualifies the bypass claim (real bypass of that gate only if …) rather than asserting one outright.vitestcleanup suite green (25/25);eslint --max-warnings 0+prettierclean on the changed files.tscfor the change is clean (the pre-existing@qwen-code/channel-github/ serve type errors onmainare unrelated).中文说明
概述
cleanup 的绕过审计（#7691 引入）会列出审查窗口内、由 reviewing account 发出、且 submit receipt 未背书的所有 review 与 issue comment 并告警——该告警会被逐字转达进审查小结。检测本身是刻意 fail-safe 的,但页脚措辞过头:它下的结论是"otherwise a write bypassed the submit gate"（否则就是有写入绕过了 submit gate）,而这一集合里合法地包含了本次审查从未发出的写入。
同一账号下另一来源的写入,会产生与审计标记完全相同的形状——并发的
/review、triage/autofix 机器人、或用户本人手动审查,都在同一 GitHub 账号下发帖——仅凭元数据无法把它和真正的gh pr review绕过区分开。dogfooding 时实测到: 一次只观察（
不加 --comment）、什么都没发的 quick pass,其窗口内落入了一条同账号并发的CHANGES_REQUESTEDreview。审计把它标记了,小结里带上了"a write bypassed the submit gate"——对一次外部写入下了一个吓人且误导的结论。改动内容
只改审计的页脚文案。现在它:
检测逻辑（
findUnsanctionedReviews/findUnsanctionedIssueComments）、审计窗口、以及每条写入的明细行都未改动——之前会浮现的东西现在一样浮现。只纠正了文案所下的结论,使得同账号并发写入不再被读成"你绕过了 submit gate",而真正的绕过依旧会被浮现给人来判断。测试
cleanup.test.ts:窗口内 review 的用例现在还断言页脚以良性措辞开头、具名账号、并把绕过说法限定（real bypass of that gate only if …）,而非直接断言绕过。vitestcleanup 套件全绿（25/25）;改动文件eslint --max-warnings 0+prettier干净。本改动的tsc干净（main上既有的@qwen-code/channel-github/ serve 类型报错与本改动无关）。