From 151a94633045fe2b190574a9aeef149c55175813 Mon Sep 17 00:00:00 2001 From: wenshao Date: Mon, 13 Jul 2026 12:43:31 +0800 Subject: [PATCH 01/12] fix(review): stop rendering a maintainer's blocker as an endorsement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Step 6 re-check exists to stop a review submitting `C=0` while a live blocker still stands on the PR. On #6486 it did exactly that, and the reason was structural rather than a lapse of judgment. `pr-context` quarantined a thread into the mandatory re-check section only if its body contained the literal string `[Critical]` — a marker only /review itself emits. A maintainer built the PR, drove the real CLI, and filed "Finding 1 — Ctrl+F dual-fires ... (blocker)" as an ISSUE comment. Every issue comment settled into "Already discussed — do NOT re-report" as a 240-character snippet, and the first 240 characters of that one were its preamble: "I built this PR from source and drove the real CLI ... to validate the model-toggle hotkey before merge." That reads as an ENDORSEMENT. The blocker began 1143 characters past the cut. Three hours later /review reviewed the same commit — the fix did not land until that evening — and submitted "no blockers". Recognition is now semantic (`carriesBlockerSignal`) and matches assertion patterns rather than word presence, with a negation guard. Blocker-bearing bodies — inline threads and issue comments alike — are promoted into a "Blockers to re-check" section and rendered in full. Word presence was the first cut and it does not survive contact with a real thread: on the live #6486 discussion it promoted 8 of 15 issue comments, of which one was a live blocker. The rest were the triage bot's own template line "No critical blockers." (the word inside its own negation), the author's "### Critical fixes" heading, and a comment quoting `[Critical]` while arguing a finding away. Eight full bodies took the context file from 30 KB to 59 KB and pushed the real blocker to character 43094 — past the 25000 one `read_file` returns. The section held the right blocker and no agent could read it, which is PR #5738's failure reintroduced one section further down. So the section is written FIRST, ahead of the description and the review history: nothing in this file outranks the claims a `C=0` verdict may not be reached without ruling on. On the live thread that moved the heading from character 25961 to 569 and the blocker body from 43094 to 4421. A character budget bounds the section even so; bodies past it degrade to snippets naming their exact fetch, which the re-check already must run before ruling. The fixture is the real comment body, byte for byte, so the regression is pinned against the thread that produced it. --- .../pr-6486-comment-4942713150.md | 114 ++++++ .../src/commands/review/pr-context.test.ts | 362 +++++++++++++++++- .../cli/src/commands/review/pr-context.ts | 329 +++++++++++++--- .../core/src/skills/bundled/review/DESIGN.md | 35 +- .../core/src/skills/bundled/review/SKILL.md | 6 +- 5 files changed, 780 insertions(+), 66 deletions(-) create mode 100644 packages/cli/src/commands/review/__fixtures__/pr-6486-comment-4942713150.md diff --git a/packages/cli/src/commands/review/__fixtures__/pr-6486-comment-4942713150.md b/packages/cli/src/commands/review/__fixtures__/pr-6486-comment-4942713150.md new file mode 100644 index 00000000000..389c874d367 --- /dev/null +++ b/packages/cli/src/commands/review/__fixtures__/pr-6486-comment-4942713150.md @@ -0,0 +1,114 @@ +## 🔬 Maintainer local build & real-run verification + +I built this PR from source and drove the **real CLI** end-to-end (not just unit tests) to validate the model-toggle hotkey before merge. Sharing the results as a merge reference. + +**Environment** + +- macOS (Darwin 24.6.0), Node v22.23.1, built from PR head `d0edcbe9` via `npm run build` (exit 0). +- Driven through a `tmux` PTY with an isolated `HOME` and `settings.json` → `model: { name: "model-a", toggleModel: "model-b", baseUrl: "…" }`, OpenAI auth. + +### ✅ What works + +| Check | Result | +| --------------------------------------------------------------------------------------- | ----------------- | +| `npm run build` | ✅ clean (exit 0) | +| `npm run typecheck` (all packages) | ✅ clean | +| `keyBindings.test.ts` + `keyMatchers.test.ts` | ✅ 52/52 pass | +| Ctrl+F toggles `model-a` → `model-b` (header + footer + info message) | ✅ | +| Repeated Ctrl+F round-trips via `previousModelRef` (`b → a → b`) | ✅ | +| `?` shortcuts panel shows `ctrl+f to toggle model`; `COLUMN_SPLITS` renders every entry | ✅ | + +Raw `Ctrl+F` (byte `0x06`) is recognized directly — no kitty CSI-u sequence needed. + +**Repeated toggle round-trip (works):** +![toggle works](https://raw.githubusercontent.com/QwenLM/qwen-code/pr-assets/pr-6486-model-toggle-verify/1-toggle-works.png) + +**Shortcuts panel (`?`) renders the new binding:** +![shortcuts](https://raw.githubusercontent.com/QwenLM/qwen-code/pr-assets/pr-6486-model-toggle-verify/3-shortcuts.png) + +### 🔴 Finding 1 — Ctrl+F dual-fires: it toggles the model **and** moves the input cursor one char right (blocker) + +This is the earlier **[Critical] dual-fire** raised by `@doudouOUC` — it is **still reproducible**. `text-buffer.ts:2663` still binds `Ctrl+F → move('right')`, and the toggle handler in `AppContainer` and the input handler in `BaseTextInput` are **independent subscribers** of the same `KeypressContext.broadcast()` (which has no stop-propagation). So the `return` in the toggle handler does not stop the text buffer from also acting. The guard added in the last round (`!activePtyId && !embeddedShellFocused`) fixes shell-focus collisions but not this. + +**Proven reproduction** (see screenshot): + +1. Type `1234` → cursor at end. +2. Press `Ctrl+A` → cursor to home (before `1`). +3. Press `Ctrl+F` **once**, then type `X`. +4. Result: **`1X234`** — the cursor moved right by one. (Control run without Ctrl+F yields `X1234`, confirming `Ctrl+A` homed the cursor; the _only_ difference is Ctrl+F's extra `move('right')`.) The model _also_ toggled to `model-b` in the same keypress. + +![dual-fire](https://raw.githubusercontent.com/QwenLM/qwen-code/pr-assets/pr-6486-model-toggle-verify/2-dualfire.png) + +**Impact:** harmless when the cursor is already at end-of-input (the move is a no-op — which is exactly why it slips past casual testing), but it **silently corrupts cursor position when toggling mid-edit**. It also contradicts this PR's own doc change, which removed `Ctrl+F` from the cursor-right row. + +**Suggested fix (one line):** remove `else if (key.ctrl && key.name === 'f') move('right');` at `packages/cli/src/ui/components/shared/text-buffer.ts:2663`. Since the PR already repurposes Ctrl+F and documents `→` as the replacement, this simply aligns the code with the docs. + +### 🟡 Finding 2 — Info message renders a doubled bullet `● ● Switched to model-b` (cosmetic) + +`InfoMessage` already renders a `●` prefix (`StatusMessages.tsx:72`), but the three new messages prepend another `● ` in the text, producing `● ● Switched to …`. Every other `MessageType.INFO` caller in the codebase passes text _without_ a leading bullet and relies on the renderer's prefix. + +**Suggested fix:** drop the leading `● ` from the three strings in `AppContainer.tsx` (lines `3625`, `3644`, `3658`). + +### ⚪ Not exercised live (verified by code + unit tests only) + +The manual-`/model` invalidation path (`onModelChange` clearing `previousModelRef`) and the `⚠ Failed to switch` error path were reviewed in code and covered by the matcher/binding unit tests, but not driven end-to-end — the dummy test environment has no second real model to switch to via `/model`. The invalidation logic reads correctly. + +### Verdict + +Cleanly scoped feature; the toggle, round-trip, header/footer wiring, and shortcut display all work. **One blocker before merge: Finding 1 (Ctrl+F dual-fire)** — a one-line removal of the stale `text-buffer` binding. Finding 2 is trivial polish. With those two changes, this is good to go. 👍 + +
+🇨🇳 中文说明（点击展开） + +## 🔬 维护者本地构建与真机验证 + +我从源码构建了此 PR，并端到端驱动**真实 CLI**（不仅是单元测试）来验证合并前的模型切换快捷键，结果作为合并参考分享如下。 + +**环境** + +- macOS（Darwin 24.6.0），Node v22.23.1，基于 PR HEAD `d0edcbe9` 执行 `npm run build`（退出码 0）。 +- 通过 `tmux` PTY 驱动，使用隔离的 `HOME` 和 `settings.json` → `model: { name: "model-a", toggleModel: "model-b", … }`，OpenAI 鉴权。 + +### ✅ 正常工作的部分 + +| 检查项 | 结果 | +| ----------------------------------------------------------------------------- | ------------------- | +| `npm run build` | ✅ 通过（退出码 0） | +| `npm run typecheck`（全部包） | ✅ 通过 | +| `keyBindings.test.ts` + `keyMatchers.test.ts` | ✅ 52/52 通过 | +| Ctrl+F 将 `model-a` → `model-b`（标题栏 + 底栏 + 提示消息） | ✅ | +| 重复按 Ctrl+F 通过 `previousModelRef` 往返（`b → a → b`） | ✅ | +| `?` 快捷键面板显示 `ctrl+f to toggle model`；`COLUMN_SPLITS` 完整渲染所有条目 | ✅ | + +原始 `Ctrl+F`（字节 `0x06`）可直接识别——无需 kitty CSI-u 序列。（截图见上方英文部分。） + +### 🔴 发现 1 —— Ctrl+F 双重触发：既切换模型**又**把输入光标右移一个字符（阻塞项） + +这正是 `@doudouOUC` 之前提出的 **[Critical] 双重触发** ——**目前仍可复现**。`text-buffer.ts:2663` 仍然绑定 `Ctrl+F → move('right')`，而 `AppContainer` 中的切换处理器和 `BaseTextInput` 中的输入处理器是同一个 `KeypressContext.broadcast()` 的**独立订阅者**（该广播没有停止传播机制）。因此切换处理器里的 `return` 并不能阻止文本缓冲区也执行动作。上一轮新增的守卫（`!activePtyId && !embeddedShellFocused`）修复了 shell 焦点冲突，但没有修复此问题。 + +**已验证的复现步骤**（见截图）： + +1. 输入 `1234` → 光标在末尾。 +2. 按 `Ctrl+A` → 光标移到行首（`1` 之前）。 +3. 按**一次** `Ctrl+F`，然后输入 `X`。 +4. 结果：**`1X234`** —— 光标右移了一位。（不按 Ctrl+F 的对照实验得到 `X1234`，证明 `Ctrl+A` 确实把光标移到了行首；唯一的差别就是 Ctrl+F 额外的 `move('right')`。）同一次按键中模型也切换到了 `model-b`。 + +**影响：** 当光标已在输入末尾时无害（移动是空操作——这正是它能躲过随手测试的原因），但在编辑过程中切换时会**悄悄破坏光标位置**。这也与本 PR 自己的文档改动矛盾（文档已将 `Ctrl+F` 从"右移光标"一行中移除）。 + +**建议修复（一行）：** 删除 `packages/cli/src/ui/components/shared/text-buffer.ts:2663` 的 `else if (key.ctrl && key.name === 'f') move('right');`。由于该 PR 已经把 Ctrl+F 改作他用，并在文档中用 `→` 作为替代，这只是让代码与文档保持一致。 + +### 🟡 发现 2 —— 提示消息出现重复圆点 `● ● Switched to model-b`（外观问题） + +`InfoMessage` 本身已渲染 `●` 前缀（`StatusMessages.tsx:72`），但三条新消息在文本中又加了一个 `● `，导致 `● ● Switched to …`。代码库中其他所有 `MessageType.INFO` 调用都不带前导圆点，依赖渲染器的前缀。 + +**建议修复：** 去掉 `AppContainer.tsx` 中三处（第 `3625`、`3644`、`3658` 行）字符串的前导 `● `。 + +### ⚪ 未做真机验证（仅通过代码与单测确认） + +手动 `/model` 使 `previousModelRef` 失效的路径，以及 `⚠ Failed to switch` 错误路径，已在代码中审阅并被匹配器/绑定单测覆盖，但未端到端驱动——因为 dummy 测试环境没有第二个真实模型可供 `/model` 切换。失效逻辑本身阅读下来是正确的。 + +### 结论 + +功能范围清晰；切换、往返、标题栏/底栏联动、快捷键显示均正常工作。**合并前有一个阻塞项：发现 1（Ctrl+F 双重触发）** —— 只需删除 `text-buffer` 中那一行残留绑定。发现 2 是很小的打磨。改完这两处即可合并。👍 + +
diff --git a/packages/cli/src/commands/review/pr-context.test.ts b/packages/cli/src/commands/review/pr-context.test.ts index ac5f8a90589..f428a585065 100644 --- a/packages/cli/src/commands/review/pr-context.test.ts +++ b/packages/cli/src/commands/review/pr-context.test.ts @@ -5,6 +5,9 @@ */ import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; import type { Argv, CommandModule } from 'yargs'; import { prContextCommand, @@ -13,6 +16,7 @@ import { SUMMARY_MARKER, truncatedHeadings, buildMarkdown, + carriesBlockerSignal, classifyInlineThreads, fullBody, fullCommentBody, @@ -253,7 +257,7 @@ describe('buildMarkdown — review bodies and replied Criticals', () => { { id: 4, user: { login: 'author' }, in_reply_to_id: 3, body: 'done' }, ]; const md = buildMarkdown('1', 'o/r', meta, inline, [], []); - const critSection = md.indexOf('## Replied Criticals'); + const critSection = md.indexOf('## Blockers to re-check'); const discussed = md.indexOf('## Already discussed'); expect(critSection).toBeGreaterThan(-1); expect(critSection).toBeLessThan(discussed); @@ -373,6 +377,360 @@ describe('buildMarkdown — truncation refs are copy-runnable with real coordina }); }); +// PR #6486, comment 4942713150: a maintainer built the PR, drove the real CLI, +// and filed a live blocker (Ctrl+F dual-fires — it toggles the model AND moves +// the cursor, `text-buffer.ts:2663`) as an ISSUE comment. Three hours later +// /review reviewed the same commit `5ede0f3a2`, where the blocker was still +// live — the fix did not land until `34e13ddb4` that evening — and submitted +// "Reviewed — no blockers". +// +// Why it dropped the blocker is structural, not a lapse of judgment. Every +// issue comment is rendered as a 240-char one-line snippet under a heading +// that reads "do NOT re-report", and the first 240 characters of this one are +// its preamble: "I built this PR from source and drove the real CLI ... to +// validate the model-toggle hotkey before merge." That reads as an ENDORSEMENT. +// "Finding 1 — Ctrl+F dual-fires ... (blocker)" begins 1 143 characters past +// the cut. The `[Critical]` marker that promotes a thread into the mandatory +// re-check section never appears in the body at all — the finding is headed +// "🔴 Finding 1". +// +// The fixture is the real comment body, byte for byte. +describe('buildMarkdown — a markerless maintainer blocker must not render as an endorsement (PR #6486 regression)', () => { + const realBody = readFileSync( + join( + dirname(fileURLToPath(import.meta.url)), + '__fixtures__', + 'pr-6486-comment-4942713150.md', + ), + 'utf8', + ); + + const meta = { + title: 'feat(cli): model toggle hotkey', + body: 'Adds Ctrl+F to toggle between two models.', + author: { login: 'Aleks-0' }, + baseRefName: 'main', + headRefName: 'feat/model-toggle-hotkey', + headRefOid: '5ede0f3a2', + additions: 1, + deletions: 1, + changedFiles: 1, + state: 'OPEN', + } as PrMetadata; + + const render = () => + buildMarkdown( + '6486', + 'QwenLM/qwen-code', + meta, + [], + [ + { id: 4942713150, user: { login: 'wenshao' }, body: realBody }, + { + id: 4909062177, + user: { login: 'Aleks-0' }, + body: 'Addressed all 3.', + }, + ], + [], + ); + + it('carries the blocker itself into the context, not just its preamble', () => { + const md = render(); + // The substance the Step 6 re-check has to rule on. None of it survives a + // 240-char snippet, and a reader who never sees it cannot even know there + // is something to fetch. + expect(md).toContain('dual-fires'); + expect(md).toContain('text-buffer.ts:2663'); + }); + + it('does not file it under "do NOT re-report"', () => { + const md = render(); + const alreadyDiscussed = md.indexOf('## Already discussed'); + const blocker = md.indexOf('dual-fires'); + expect(blocker).toBeGreaterThanOrEqual(0); + // Rendered ahead of the settled-discussion section — i.e. in a section the + // re-check must rule on, not one it is told to skip. + expect( + alreadyDiscussed === -1 || blocker < alreadyDiscussed, + 'the blocker is rendered inside "Already discussed — do NOT re-report"', + ).toBe(true); + }); + + it('still lets ordinary chatter settle into Already discussed', () => { + const md = render(); + const alreadyDiscussed = md.indexOf('## Already discussed'); + const chatter = md.indexOf('Addressed all 3.'); + // The promotion must key on blocker substance, not on "issue comment" — + // otherwise every thankyou note becomes a mandatory ruling. + expect(alreadyDiscussed).toBeGreaterThanOrEqual(0); + expect(chatter).toBeGreaterThan(alreadyDiscussed); + }); +}); + +describe('carriesBlockerSignal', () => { + it('recognises a blocker that never uses the [Critical] marker', () => { + // The real PR #6486 heading. Only /review emits `[Critical]`; a human + // types whatever they type, and the old literal-marker gate saw none of it. + expect( + carriesBlockerSignal( + '### 🔴 Finding 1 — Ctrl+F dual-fires: it toggles the model **and** moves the cursor (blocker)', + ), + ).toBe(true); + expect(carriesBlockerSignal('This is still reproducible at HEAD.')).toBe( + true, + ); + expect(carriesBlockerSignal('Must fix before merge: auth bypass.')).toBe( + true, + ); + expect(carriesBlockerSignal('这个问题是阻塞项，合并前必须修复。')).toBe( + true, + ); + }); + + it('still recognises the marker /review emits', () => { + expect(carriesBlockerSignal('**[Critical]** real blocker')).toBe(true); + expect(carriesBlockerSignal('**[critical]** case-insensitive')).toBe(true); + }); + + it('is not fooled by a signal sitting inside its own negation', () => { + expect(carriesBlockerSignal('No critical blockers. LGTM.')).toBe(false); + expect(carriesBlockerSignal('There is not a blocker here.')).toBe(false); + expect(carriesBlockerSignal('Zero must-fix items.')).toBe(false); + // …but a body may BOTH wave off one blocker and assert another. One + // un-negated occurrence is enough to promote. + expect( + carriesBlockerSignal( + 'No critical blockers in the parser. The cache path, though, is a blocker.', + ), + ).toBe(true); + }); + + it('recognises the words people actually write, not the nouns we imagined', () => { + // The second real blocker this list missed. A maintainer's E2E report on + // PR #6638 — a committed extension policy that never reaches a running + // agent's system prompt while the API reports full convergence — is headed + // "86/90 checks pass, 1 blocking gap" and "🔴 Blocking:", and in Chinese + // "阻塞问题". The patterns named the nouns (`blocking issue|defect|bug`, + // `阻塞项`) and not one of them matched, so it would have settled behind a + // 240-char snippet reading "86/90 checks pass … hold up well" — an + // endorsement, exactly as in #6486. + expect( + carriesBlockerSignal( + '## E2E verification — 86/90 checks pass, 1 blocking gap', + ), + ).toBe(true); + expect( + carriesBlockerSignal('### 🔴 Blocking: a committed policy never lands'), + ).toBe(true); + expect( + carriesBlockerSignal('### 🔴 阻塞问题：策略没有到达运行中的 agent'), + ).toBe(true); + }); + + it('does not fire on our own "Non-blocking observations" heading', () => { + // Every verification report files its nits under this heading. Matching a + // bare `blocking` without the lookbehind would promote all of them. + expect(carriesBlockerSignal('### 🟡 Non-blocking observations')).toBe( + false, + ); + expect(carriesBlockerSignal('This is a non-blocking nit.')).toBe(false); + expect(carriesBlockerSignal('非阻塞观察：建议后续跟进')).toBe(false); + }); + + it('guards the Chinese signal in Chinese, not only in English', () => { + // The signal list is bilingual (`阻塞项`); the guard was not. On a repo whose + // PR discussion is substantially Chinese, every "没有阻塞项" — the Chinese half + // of the triage bot's own template — promoted, while its English twin did + // not. A guard that only defends the language it was written in has a hole + // exactly the size of the other language. + expect(carriesBlockerSignal('没有阻塞项。LGTM')).toBe(false); + expect(carriesBlockerSignal('不是阻塞项，可以合并')).toBe(false); + expect(carriesBlockerSignal('经检查无阻塞项')).toBe(false); + expect(carriesBlockerSignal('未发现阻塞项')).toBe(false); + // The assertion still promotes. + expect(carriesBlockerSignal('这是一个阻塞项，必须修复')).toBe(true); + }); + + it('does not promote a severity emoji on a list of repairs', () => { + // The author's "### 🔴 Critical fixes" heading. A bare emoji says nothing + // about who is asserting what — it fired the first implementation and cost + // the read budget the real blocker needed. + expect( + carriesBlockerSignal('### 🔴 Critical fixes\nAddressed all 3.'), + ).toBe(false); + }); + + it('does not let a negation reach across a clause separator', () => { + // The negation guard scans back 40 characters for a negation word, and its + // stop-set only had `.!?`. "No blockers; the cache path is a (blocker)" + // therefore suppressed the real assertion — a false negative, which is the + // costly direction. + expect( + carriesBlockerSignal('No blockers; the cache path is a (blocker)'), + ).toBe(true); + expect( + carriesBlockerSignal('No blockers: the cache path is a (blocker)'), + ).toBe(true); + // …and the plain negation still negates. + expect(carriesBlockerSignal('No critical blockers. LGTM.')).toBe(false); + expect(carriesBlockerSignal('没有阻塞问题：一切正常')).toBe(false); + }); + + it('does not promote ordinary chatter', () => { + // Promotion means a mandatory ruling AND a full-body render. Over-promote + // and the context file outgrows one read — which is its own way of losing + // a blocker, so precision matters in both directions. + expect(carriesBlockerSignal('Addressed all 3 findings, thanks!')).toBe( + false, + ); + expect(carriesBlockerSignal('**[Suggestion]** rename this helper')).toBe( + false, + ); + expect(carriesBlockerSignal('LGTM, nice work')).toBe(false); + expect(carriesBlockerSignal(undefined)).toBe(false); + }); +}); + +describe('blockerSection — both channels, and the budget', () => { + const meta = { + title: 'T', + body: 'D', + author: { login: 'a' }, + baseRefName: 'main', + headRefName: 'b', + headRefOid: 'sha', + additions: 1, + deletions: 1, + changedFiles: 1, + state: 'OPEN', + } as PrMetadata; + + it('carries an inline blocker and an issue-level one in the same section', () => { + // A blocker arrives on whichever channel the reviewer happened to use, and + // the re-check must rule on every one of them. The two are rendered by + // different loops; nothing pinned that they land in the SAME section. + const inline = [ + { + id: 11, + user: { login: 'rev' }, + path: 'a.ts', + line: 3, + body: '**[Critical]** the cache is never invalidated', + }, + { id: 12, user: { login: 'auth' }, in_reply_to_id: 11, body: 'wontfix' }, + ]; + const issue = [ + { + id: 21, + user: { login: 'maint' }, + body: 'Drove the real CLI: Ctrl+F still dual-fires (blocker). See `text-buffer.ts:2663`.', + }, + ]; + const md = buildMarkdown('1', 'o/r', meta, inline, issue, []); + + const section = md.indexOf('## Blockers to re-check'); + const discussed = md.indexOf('## Already discussed'); + const inlineBlocker = md.indexOf('the cache is never invalidated'); + const issueBlocker = md.indexOf('still dual-fires'); + + expect(section).toBeGreaterThanOrEqual(0); + // Both inside the re-check section — i.e. before "Already discussed" + // (or before the end of the file, when that section is absent). + const end = discussed === -1 ? md.length : discussed; + expect(inlineBlocker).toBeGreaterThan(section); + expect(inlineBlocker).toBeLessThan(end); + expect(issueBlocker).toBeGreaterThan(section); + expect(issueBlocker).toBeLessThan(end); + // A reply does not retire a blocker; the thread's reply still renders. + expect(md).toContain('wontfix'); + // And the issue-level one keeps its Referenced-code list. + expect(md).toContain('`text-buffer.ts:2663`'); + }); + + it('degrades a body past the budget to a snippet that names its fetch', () => { + // Promotion means full-body rendering, and full bodies are what blew the + // read window on the live #6486 thread. The budget bounds the section; what + // it must NOT do is drop a blocker silently — a degraded body still says how + // to fetch the rest, which the re-check must do before ruling. + const big = (n: number) => ({ + id: n, + user: { login: 'r' }, + body: `**[Critical]** blocker ${n}: ${'x'.repeat(7000)}`, + }); + const md = buildMarkdown( + '6486', + 'QwenLM/qwen-code', + meta, + [], + [big(1), big(2), big(3)], + [], + ); + expect(md).toContain('## Blockers to re-check'); + // Every blocker is still ANNOUNCED — none vanishes. + for (const n of [1, 2, 3]) { + expect(md).toContain(`(comment ${n})`); + } + // The one past the budget is a snippet, and it names the exact fetch. + expect(md).toContain('section budget spent'); + expect(md).toContain('gh api repos/QwenLM/qwen-code/issues/comments/3'); + }); + + it('renders the bodies that fit in FULL and only degrades past the budget', () => { + // The boundary is the whole point: a budget that degraded everything, or + // nothing, would pass the test above just as well. Blocker 1 must arrive + // whole (that is what makes it rulable); blocker 3 must not. + const big = (n: number) => ({ + id: n, + user: { login: 'r' }, + body: `**[Critical]** blocker ${n} TAIL${n}: ${'x'.repeat(7000)}`, + }); + const md = buildMarkdown( + '6486', + 'QwenLM/qwen-code', + meta, + [], + [big(1), big(2), big(3)], + [], + ); + // 7000-char bodies against a 16000 budget: the first two fit whole… + expect(md).toContain('TAIL1'); + expect(md).toContain('TAIL2'); + // …and the third is the snippet. Its 7000-char tail is not in the file. + expect(md).not.toContain('TAIL3'.padEnd(0) + 'x'.repeat(6900)); + expect(md.match(/section budget spent/g)).toHaveLength(1); + }); + + it('charges its own headings and reference lists against the budget', () => { + // Structural overhead is real characters in a file whose whole purpose is + // fitting inside one `read_file`. Charging only the quoted bodies leaves it + // unbounded — the section can then outgrow the window while its own + // accounting still says it has room. + const withRefs = (n: number) => ({ + id: n, + user: { login: 'r' }, + body: `**[Critical]** blocker ${n} — see \`src/a${n}.ts:10\`, \`src/b${n}.ts:20\`. ${'y'.repeat(5000)}`, + }); + const md = buildMarkdown( + '1', + 'o/r', + meta, + [], + [withRefs(1), withRefs(2), withRefs(3), withRefs(4)], + [], + ); + const section = md.slice( + md.indexOf('## Blockers to re-check'), + md.indexOf('## Description'), + ); + // Bodies alone would be 4 × ~5 100 = 20 400 > 16 000, so degradation must + // kick in; with the overhead charged too, it kicks in no later. + expect(section).toContain('section budget spent'); + // And the section stays inside the window one read returns. + expect(section.length).toBeLessThan(25_000); + }); +}); + describe('classifyInlineThreads', () => { it('is the single walk both the markdown and the stdout count use', () => { const inline: RawComment[] = [ @@ -383,7 +741,7 @@ describe('classifyInlineThreads', () => { { id: 5, user: { login: 'r' }, body: 'open question' }, ]; const t = classifyInlineThreads(inline); - expect(t.repliedCriticalRoots.map((c) => c.id)).toEqual([1]); + expect(t.repliedBlockerRoots.map((c) => c.id)).toEqual([1]); expect(t.repliedRoots.map((c) => c.id)).toEqual([3]); expect(t.openRoots.map((c) => c.id)).toEqual([5]); expect(t.repliesByRoot.get(1)!.map((c) => c.id)).toEqual([2]); diff --git a/packages/cli/src/commands/review/pr-context.ts b/packages/cli/src/commands/review/pr-context.ts index e58326d2ca7..17a0b7f096b 100644 --- a/packages/cli/src/commands/review/pr-context.ts +++ b/packages/cli/src/commands/review/pr-context.ts @@ -156,6 +156,117 @@ export function fullCommentBody( ); } +/** Cap a full issue-comment body; the cut names the issue-comment id. */ +export function fullIssueCommentBody( + s: string | undefined, + id?: number, + ctx?: RefContext, +): string { + return capBody( + s, + id !== undefined ? issueCommentRef(id, ctx) : 'the issue comments API', + ); +} + +/** + * Does this body assert a blocking defect? + * + * The re-check section used to be gated on the literal `[Critical]` marker, + * which only /review itself emits. A human blocker phrased any other way fell + * through to "Already discussed — do NOT re-report", where it is rendered as a + * 240-character snippet. + * + * On PR #6486 a maintainer built the PR, drove the real CLI, and filed + * "🔴 Finding 1 — Ctrl+F dual-fires ... (blocker)" as an issue comment. The + * marker never appeared. The first 240 characters were the report's preamble — + * "I built this PR from source and drove the real CLI ... to validate the + * model-toggle hotkey before merge" — which reads as an ENDORSEMENT, filed + * under a heading that says not to re-report it. The blocker began 1 143 + * characters past the cut. /review reviewed that same commit three hours later + * and submitted "no blockers"; the defect was real and was fixed that evening. + * + * So recognition is semantic. It matches **assertion patterns, not word + * presence**, and that distinction was learned the hard way: the first cut of + * this scanned the whole body for the words `blocker`, `🔴`, `阻塞` and + * `[Critical]`, and on the live #6486 thread it promoted **8 of 15** issue + * comments. Exactly one was a live blocker. The others: + * + * - "**No** critical blockers." — the triage bot's own template line, i.e. the + * word appearing inside its own negation. Hence `NEGATION`. + * - "### 🔴 Critical **fixes**" — the author listing what he had *repaired*. + * A severity emoji says nothing about who is asserting what. + * - a later comment *quoting* `[Critical]` while arguing a finding away. + * + * Promotion is still deliberately fail-safe — a false positive costs one extra + * ruling, a false negative ships the bug — but "cheap" was measured, not + * assumed, and it was wrong: promotion means **full-body** rendering, and those + * 8 bodies took the context file from 30 KB to 59 KB and pushed the real + * blocker to character 43 094, past what one `read_file` returns. A blocker + * rendered where nobody reads it is not better than one rendered as a snippet. + * That is why the section is written FIRST and carries a size budget. + * + * **Tight is not the same as narrow, and the first cut of these patterns was + * narrow.** They named the nouns — `blocking issue|defect|bug`, `阻塞项` — and a + * second real blocker walked straight past them: a maintainer's E2E report on + * PR #6638 (a committed extension policy that never reaches a running agent's + * system prompt, while the API reports full convergence) is headed + * "**86/90 checks pass, 1 blocking gap**" and "🔴 **Blocking:**", and in Chinese + * "**阻塞问题**". Not one pattern matched. It would have settled into "Already + * discussed" behind a 240-character snippet whose visible text is + * _"86/90 checks pass … The store, the REST surface and the secur…"_ — an + * endorsement, again, exactly as in #6486. + * + * So the patterns match the word people actually write (`blocking`, with a + * lookbehind for `non-blocking` — our own reports file their nits under + * "🟡 Non-blocking observations"), and the CJK forms they actually use. Measured + * over 38 real comments from three threads: recall 1/2 → **2/2**, false + * positives **unchanged at 6**. Widening `before merge` / `合并前` would also + * have caught it and cost 2 and 1 more false positives respectively, so those + * are left out. The list is calibrated against real threads, not imagined ones, + * and it stays a **floor**: SKILL.md still scans "Already discussed" in prose. + */ +const BLOCKER_PATTERNS: RegExp[] = [ + /\[critical\]/, // the marker /review itself emits + /\(blocker\)/, // "🔴 Finding 1 — … (blocker)" + /\bis a blocker\b/, + // `blocking` on its own, because that is how people actually write it: a + // "blocking gap", a "🔴 Blocking:" heading. Naming the nouns (`blocking + // issue|defect|bug`) looked precise and missed a real blocker — see below. + // The lookbehind is what keeps "non-blocking" out, which matters: our own + // verification reports file their nits under "🟡 Non-blocking observations". + /(? { + const m = new RegExp(re.source, 'g'); + let hit: RegExpExecArray | null; + while ((hit = m.exec(b)) !== null) { + // Negated occurrences do not count, but a body may both mention "no + // blockers" AND assert one — so a single un-negated occurrence promotes. + if (!NEGATION.test(b.slice(0, hit.index))) return true; + } + return false; + }); +} + /** * One-line snippet that, when it cuts, names the exact fetch for the rest — * a bare `…` marks a cut nobody can act on, and the fail-closed "a body you @@ -226,7 +337,7 @@ export function isReviewWorthShowing(body: string | undefined): boolean { export interface InlineThreads { openRoots: RawComment[]; - repliedCriticalRoots: RawComment[]; + repliedBlockerRoots: RawComment[]; repliedRoots: RawComment[]; repliesByRoot: Map; } @@ -264,22 +375,133 @@ export function classifyInlineThreads(inline: RawComment[]): InlineThreads { ); const allRepliedRoots = roots.filter((c) => repliesByRoot.has(c.id)); // A reply alone does not retire a blocker — "I disagree" is a reply. Any - // replied thread whose root is a Critical finding is pulled out of - // "Already discussed" into its own mandatory re-check section. Matching - // on the marker is fail-safe in this direction: a third party embedding - // it can only ADD their thread to the re-check list, never hide one. - // (It is a floor, not a ceiling: a blocker phrased WITHOUT the marker - // settles into "Already discussed", which is why Step 6's semantic - // re-check scans that section too.) - const repliedCriticalRoots = allRepliedRoots.filter((c) => - (c.body ?? '').includes('[Critical]'), + // replied thread whose root asserts a blocking defect is pulled out of + // "Already discussed" into its own mandatory re-check section. Promotion is + // fail-safe in this direction: a third party can only ADD their thread to + // the re-check list, never hide one. + // + // This used to key on the literal `[Critical]` marker, which only /review + // emits — so a human blocker phrased any other way settled into the section + // headed "do NOT re-report". `carriesBlockerSignal` is the semantic test. + const repliedBlockerRoots = allRepliedRoots.filter((c) => + carriesBlockerSignal(c.body), ); const repliedRoots = allRepliedRoots.filter( - (c) => !(c.body ?? '').includes('[Critical]'), + (c) => !carriesBlockerSignal(c.body), ); const openRoots = roots.filter((c) => !repliesByRoot.has(c.id)); - return { openRoots, repliedCriticalRoots, repliedRoots, repliesByRoot }; + return { openRoots, repliedBlockerRoots, repliedRoots, repliesByRoot }; +} + +/** + * Total characters the blocker section may spend on full bodies. + * + * Full-body rendering is what makes a blocker rulable, but it is not free: on + * the live #6486 thread eight promoted bodies took the context file from 30 KB + * to 59 KB. Tight patterns keep promotion rare; this keeps a pathological + * thread from pushing the section past one `read_file` even so. Bodies past the + * budget degrade to snippets **that name their exact fetch** — which SKILL.md's + * re-check already requires be run before ruling — rather than being dropped. + */ +const BLOCKER_SECTION_BUDGET = 16000; + +function blockerSection( + roots: RawComment[], + issueBlockers: RawComment[], + repliesByRoot: Map, + ctx: RefContext, +): string[] { + if (roots.length === 0 && issueBlockers.length === 0) return []; + const out: string[] = [ + '## Blockers to re-check — a reply alone does NOT retire a blocker; the re-check must rule on each against the code', + '', + '> Bodies are rendered in full; a body cut at a cap names its comment id to fetch, and a body read in part is `cannot tell`, never "no blocker in it".', + '', + ]; + + // Everything this section emits counts against the budget, not just the quoted + // bodies: the headings, the Referenced-code lists and the reply snippets are + // real characters in a file whose whole point is fitting inside one + // `read_file`. Charging only the bodies leaves the overhead unbounded, which + // is how the section outgrows the window while its own accounting says it has + // room. + // The heading and the instruction block are ~600 characters of the budget. + // Starting `spent` at 0 spends them for free, which is the same unbounded + // overhead the `charge()` comment above exists to close. + let spent = out.join('\n').length; + const charge = (lines: string[]): string[] => { + spent += lines.join('\n').length; + return lines; + }; + const sortedRoots = [...roots].sort((a, b) => { + const p = (a.path ?? '').localeCompare(b.path ?? ''); + if (p !== 0) return p; + return (a.line ?? 0) - (b.line ?? 0); + }); + + for (const root of sortedRoots) { + out.push( + ...charge([ + `**\`${root.path ?? '?'}\`:${root.line ?? '?'}** — initiated by @${root.user?.login ?? '?'} (comment ${root.id})`, + '', + ]), + ); + // Gate on what is actually emitted. `quoteBlock` adds `> ` to every line, so + // gating on the raw body undercounts each one by 2 × its line count. + const quoted = quoteBlock(fullCommentBody(root.body, root.id, ctx)); + if (spent + quoted.length <= BLOCKER_SECTION_BUDGET) { + out.push(...charge([quoted, ''])); + } else { + out.push( + ...charge([ + `> ${snippetWithRef(root.body, 400, pullCommentRef(root.id, ctx))}`, + '', + '_(section budget spent — this body is a snippet; fetch it in full before ruling)_', + '', + ]), + ); + } + const replies = repliesByRoot.get(root.id) ?? []; + if (replies.length > 0) { + out.push( + ...charge([ + 'Replies (chronological):', + ...replies.map( + (r) => + `- **@${r.user?.login ?? '?'}**: ${snippetWithRef(r.body, 500, pullCommentRef(r.id, ctx))}`, + ), + '', + ]), + ); + } + } + + // Issue-level blockers carry no path/line — they are whole-PR claims, and an + // out-of-band verification report (build it, drive it, file what broke) is + // exactly the shape that arrives here. + for (const c of issueBlockers) { + out.push( + ...charge([ + `**Issue-level comment** — by @${c.user?.login ?? '?'} (comment ${c.id})`, + '', + ]), + ); + const quoted = quoteBlock(fullIssueCommentBody(c.body, c.id, ctx)); + if (spent + quoted.length <= BLOCKER_SECTION_BUDGET) { + out.push(...charge([quoted, ''])); + } else { + out.push( + ...charge([ + `> ${snippetWithRef(c.body, 400, issueCommentRef(c.id, ctx))}`, + '', + '_(section budget spent — this body is a snippet; fetch it in full before ruling)_', + '', + ]), + ); + } + } + return out; } export function buildMarkdown( @@ -290,10 +512,19 @@ export function buildMarkdown( issue: RawComment[], reviews: RawReview[], ): string { - const { openRoots, repliedCriticalRoots, repliedRoots, repliesByRoot } = + const { openRoots, repliedBlockerRoots, repliedRoots, repliesByRoot } = classifyInlineThreads(inline); const ctx: RefContext = { ownerRepo, prNumber }; + // Issue-level comments are the channel a maintainer's out-of-band review + // arrives on — a build-and-drive report, a "this is still broken" note. They + // all used to settle into "Already discussed" as 240-char snippets, so a + // blocker filed there was invisible to the re-check (PR #6486). Split them: + // the ones asserting a blocking defect join the mandatory re-check section + // and are rendered in full; the rest settle as before. + const blockerIssue = issue.filter((c) => carriesBlockerSignal(c.body)); + const settledIssue = issue.filter((c) => !carriesBlockerSignal(c.body)); + const parts: string[] = []; parts.push(`# PR #${prNumber} — ${meta.title || '(no title)'}`); @@ -312,6 +543,23 @@ export function buildMarkdown( parts.push(PREAMBLE); parts.push(''); + // Blockers FIRST — ahead of the description, the review history, everything. + // + // `read_file` returns the first 25 000 characters and pages by line, so + // whatever is written last is what a long context file loses. This section + // holds the claims a `C=0` verdict is not allowed to be reached without + // ruling on; nothing else in this file outranks it, and the PR description + // certainly does not. + // + // Measured, not assumed. Written after "Open inline comments" (its first + // position) on the live #6486 thread, the heading landed at character 25 961 + // and the blocker body at 43 094 — both past what one read returns. The + // section existed and nobody could see it, which is the PR #5738 failure this + // file already carries a comment about, reintroduced one section further down. + parts.push( + ...blockerSection(repliedBlockerRoots, blockerIssue, repliesByRoot, ctx), + ); + parts.push('## Description'); parts.push(''); if (meta.body && meta.body.trim().length > 0) { @@ -369,54 +617,12 @@ export function buildMarkdown( parts.push(''); } - // Replied Criticals — rendered before the settled threads because the - // Step 6 re-check must rule on every one of them (still stands / fixed by - // this diff / cannot tell); a reply alone never settles a blocker. Root - // bodies are rendered in full (same treatment as review summaries): the - // re-check rules on the claim's failure scenario and proposed fix, which - // is exactly the tail a 1 000-char snippet silently dropped — and a cut - // nobody can see also means the fail-closed "a body read in part is - // `cannot tell`" rule can never fire. - if (repliedCriticalRoots.length > 0) { - parts.push( - '## Replied Criticals — a reply alone does NOT retire a blocker; the re-check must rule on each against the code', - ); - parts.push(''); - parts.push( - '> Root bodies are rendered in full; a body cut at the cap names its comment id to fetch. Replies are one-line snippets that name their comment id when cut.', - ); - parts.push(''); - const sortedCrit = [...repliedCriticalRoots].sort((a, b) => { - const p = (a.path ?? '').localeCompare(b.path ?? ''); - if (p !== 0) return p; - return (a.line ?? 0) - (b.line ?? 0); - }); - for (const root of sortedCrit) { - const replies = repliesByRoot.get(root.id) ?? []; - parts.push( - `**\`${root.path ?? '?'}\`:${root.line ?? '?'}** — initiated by @${root.user?.login ?? '?'} (comment ${root.id})`, - ); - parts.push(''); - parts.push(quoteBlock(fullCommentBody(root.body, root.id, ctx))); - parts.push(''); - if (replies.length > 0) { - parts.push('Replies (chronological):'); - for (const r of replies) { - parts.push( - `- **@${r.user?.login ?? '?'}**: ${snippetWithRef(r.body, 500, pullCommentRef(r.id, ctx))}`, - ); - } - parts.push(''); - } - } - } - // Already-discussed threads — render the full conversation so review // agents can see whether the original concern was addressed (e.g. a // "Fixed in abc123" reply closes the topic). The previous version listed // only root-comment snippets and forced the LLM driver to manually // summarise each reply chain in agent prompts. - if (repliedRoots.length > 0 || issue.length > 0) { + if (repliedRoots.length > 0 || settledIssue.length > 0) { parts.push( '## Already discussed — do NOT re-report unless the latest reply itself raises a new concern', ); @@ -451,10 +657,10 @@ export function buildMarkdown( } } } - if (issue.length > 0) { + if (settledIssue.length > 0) { parts.push('### Issue-level comments (general PR thread)'); parts.push(''); - for (const c of issue) { + for (const c of settledIssue) { parts.push( `- by @${c.user?.login ?? '?'}: ${snippetWithRef(c.body, 240, issueCommentRef(c.id, ctx))}`, ); @@ -532,10 +738,11 @@ async function runPrContext(args: PrContextArgs): Promise { ).length; // Same walk buildMarkdown just rendered from — never a re-implementation, // so this count cannot silently diverge from the file's contents. - const repliedCriticalCount = - classifyInlineThreads(inline).repliedCriticalRoots.length; + const blockerCount = + classifyInlineThreads(inline).repliedBlockerRoots.length + + issue.filter((c) => carriesBlockerSignal(c.body)).length; writeStdoutLine( - `Wrote PR context to ${out} (${inline.length} inline, ${issue.length} issue comments, ${repliedCriticalCount} replied Critical(s), ${meaningfulReviewCount}/${reviews.length} review summaries — review bodies and replied-Critical roots rendered in full)`, + `Wrote PR context to ${out} (${inline.length} inline, ${issue.length} issue comments, ${blockerCount} blocker(s) to re-check, ${meaningfulReviewCount}/${reviews.length} review summaries — review bodies and blocker bodies rendered in full)`, ); // A reader that stops at the threshold loses the tail in silence: `read_file` diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index 5b7e26d2a55..8b14df4b046 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -293,10 +293,43 @@ The resolution is the same one this document already records for presubmit and c - **`parse-args`** owns the grammar. Every previously-shipped parsing bug is a named row in its table-driven tests. The raw string travels **on stdin** (`--stdin` with a quoted heredoc), never as a positional: a flag-first raw string (`/review --effort low`) is consumed by the CLI's own strict parser before the handler runs, and a positional also breaks on quotes and shell metacharacters. Pure-function tests could not see that class — the documented invocation failed only when run against the built binary — so the suite includes yargs-level wiring tests alongside the table. - **`compose-review`** owns event selection and body composition — the C/S table (counting body Criticals and discarded Suggestions), the event caps (cannot-tell existing Criticals, uncoverable chunks, unreviewed dimensions, context-unavailable), the downgrade carve-outs, and the clause composition. Its truth-table tests pin each shipped bug; writing them immediately caught one more instance of the class (all Suggestions discarded → S=0 → APPROVE). The input is validated at the boundary: the producer is a model writing JSON that omits inapplicable fields, so absent counts default to zero and malformed values throw typed errors — before that, an omitted count meant `undefined + 1 = NaN`, which fails every event comparison and would have returned APPROVE over a body-only blocker. 422 recovery stops being a hand-derived recomposition: it is the same call with updated counts, so the "recompute may never upgrade the verdict" guarantee holds by construction. -- **`pr-context`** ends the fetch-prose chain at its root: review bodies **and replied-Critical root bodies** render **in full** (a body-only blocker lives only there; a capped body names its review or comment id so the tail stays fetchable one object at a time, and reply snippets name their comment id when cut), and replied Critical threads are quarantined into their own section instead of settling into "Already discussed" — a reply alone never retires a blocker. The `gh` wrapper's `maxBuffer` rises to 64 MiB, closing the ENOBUFS that killed two subcommands mid-review on a comment-heavy PR. +- **`pr-context`** ends the fetch-prose chain at its root: review bodies **and every blocker-bearing body** render **in full** (a body-only blocker lives only there; a capped body names its review or comment id so the tail stays fetchable one object at a time, and reply snippets name their comment id when cut), and blocker-bearing threads are quarantined into a "Blockers to re-check" section instead of settling into "Already discussed" — a reply alone never retires a blocker. The `gh` wrapper's `maxBuffer` rises to 64 MiB, closing the ENOBUFS that killed two subcommands mid-review on a comment-heavy PR. What deliberately stays prose: everything judgment-shaped — what counts as a Critical, verification, the posting gate's authorization semantics, the angles. A truth table cannot decide whether a finding is real; it can guarantee that a real finding is never mislabeled, dropped by a downgrade, or approved past. +## Why blocker recognition is semantic, not the `[Critical]` marker + +The mandatory re-check section used to be gated on the literal string `[Critical]`. That marker is emitted by exactly one author — `/review` itself. Every human blocker was therefore invisible to the gate, and the fallback was a prose instruction in Step 6 telling the model to also scan "Already discussed" semantically. + +Prose does not beat structure. PR #6486 is the proof, and it cost a shipped blocker. + +A maintainer built the PR, drove the real CLI through a PTY, and found that `Ctrl+F` **dual-fires** — it toggles the model _and_ moves the input cursor, because `text-buffer.ts:2663` still binds `Ctrl+F → move('right')` and both handlers are independent subscribers of a `KeypressContext.broadcast()` that has no stop-propagation. They filed it as an **issue comment**, headed `🔴 Finding 1 — … (blocker)`. No `[Critical]` marker, because a human wrote it. + +Three things then compounded: + +1. Issue comments all settle into **"Already discussed — do NOT re-report"**. +2. They render as **240-character one-line snippets**. +3. The first 240 characters of a verification report are its **preamble**: _"I built this PR from source and drove the real CLI … to validate the model-toggle hotkey before merge. Sharing the results as a merge reference."_ + +So the one artifact that proved the PR was broken was presented to the review agents as a **maintainer endorsement**, in the section that says not to re-report it. The blocker itself began 1 143 characters past the cut. Three hours later `/review` reviewed the same commit — the fix did not land until that evening — and submitted **"Reviewed — no blockers"**. This is precisely the "dropped blocker" failure the Step 6 re-check exists to prevent, and the re-check could not prevent it, because the input it was handed said the opposite of the truth. + +The fix moves the decision out of prose and into `carriesBlockerSignal`: any body asserting a blocking defect — inline thread or issue comment, `[Critical]` or `(blocker)` or "is a blocker" or "must fix" or "still reproducible" or 阻塞项 — is promoted into **"Blockers to re-check"** and rendered **in full**. A bare `🔴` is deliberately **not** a signal, for the reason the next paragraph measures. + +Two properties are deliberate: + +- **Fail-safe direction.** A false positive costs one extra ruling by the re-check; a false negative ships the bug. When in doubt, promote. +- **Precision still matters, in the other direction.** Promotion means full-body rendering, and a context file that outgrows one `read_file` is its own way of losing a blocker (PR #5738, recorded above). The prose scan of "Already discussed" is retained as a floor — `carriesBlockerSignal` recognises the phrasings we have seen, not every phrasing that exists. + +**Both of those were nearly undone by the first implementation, and only a live run showed it.** That version scanned the whole body for the words `blocker`, `🔴`, `阻塞`, `[Critical]`. Run against the real #6486 thread it promoted **8 of 15** issue comments; exactly one was a live blocker. The others were the triage bot's own template line **"No critical blockers."** (the word inside its own negation), the author's **"### 🔴 Critical fixes"** (a severity emoji on a list of repairs), and a later comment _quoting_ `[Critical]` while arguing a finding away. Eight full bodies took the context file from 30 KB to 59 KB and pushed the real blocker to character **43 094** — past the 25 000 one `read_file` returns. The section existed, held the right blocker, and no agent could see it: PR #5738's failure, reintroduced one section further down by the fix for it. + +Three changes, and the ordering one is load-bearing: + +- **The section is written FIRST**, ahead of the description and the review history. Nothing in the file outranks the claims a `C=0` verdict may not be reached without ruling on. On the live thread this moved the heading from char 25 961 to **569**, and the blocker body from 43 094 to **4 421**. +- **Recognition matches assertion patterns, not word presence** — `[Critical]`, `(blocker)`, `is a blocker`, `blocking issue/defect/bug`, `must fix`, `still reproducible/repro/broken/fails`, `阻塞项` — with a **bilingual** negation guard, so neither "no blockers" nor "没有阻塞项" ever promotes. Live promotions dropped 8 → 3 (the one real blocker plus two harmless mentions), and the file 59 KB → 40 KB. +- **The section carries a character budget.** Tight patterns keep promotion rare; the budget keeps a pathological thread from blowing the read window anyway. Bodies past it degrade to snippets **naming their exact fetch**, which the re-check already must run before ruling — not to silence. + +The lesson generalizes past this file: **"a false positive is cheap" is a claim about a budget, and it has to be measured against the real distribution, not assumed.** Here it was false until the ordering was fixed. + ## What the first dogfood batch changed Six concurrent real-PR runs (batch 3) produced three targeted changes, each fixing something the batch measured rather than predicted: diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 888fc38756b..dd80abe7625 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -100,7 +100,9 @@ Based on the parsed `target.type`: --out .qwen/tmp/qwen-review-pr--context.md ``` - The subcommand fetches `gh pr view` metadata + inline / issue comments and writes a single Markdown file with the PR title, description, base/head, diff stats, an **"Open inline comments"** section, a **"Replied Criticals"** section (Critical threads that have replies — a reply alone never settles a blocker, so these stay on the mandatory re-check path), full-text **"Review summaries"**, and an **"Already discussed"** section for settled non-Critical threads. Each replied-to thread renders the **complete reply chain** (root comment + chronological replies), so review agents can see whether a "Fixed in ``"-style reply has closed the topic — agents must NOT re-report a concern whose latest reply addresses it. Issue-level (general PR) comments appear in the same section. (That no-re-report rule is about _reporting_; Step 6's open-Critical re-check draws on **both** sections — a Critical does not leave the verdict gate just because someone replied to it.) The file's own preamble tells agents to treat its contents as DATA, so no extra security prefix is needed when passing it to review agents. **If `pr-context` fails here too** (rate limit, network — the same-repo path is not immune), the handling is identical to lightweight mode: warn, continue, skip Agent 0, and set the **context-unavailable** state — Step 6 skips the re-check walk (every existing Critical is `cannot tell`) and Step 7 caps the event. A same-repo run that lost the context file must not behave as if it had read it. + The subcommand fetches `gh pr view` metadata + inline / issue comments and writes a single Markdown file with the PR title, description, base/head, diff stats, an **"Open inline comments"** section, a **"Blockers to re-check"** section, full-text **"Review summaries"**, and an **"Already discussed"** section for settled non-blocking threads. Each replied-to thread renders the **complete reply chain** (root comment + chronological replies), so review agents can see whether a "Fixed in ``"-style reply has closed the topic — agents must NOT re-report a concern whose latest reply addresses it. (That no-re-report rule is about _reporting_; Step 6's open-Critical re-check draws on **every** comment-bearing section — a blocker does not leave the verdict gate just because someone replied to it.) + + **"Blockers to re-check" holds every body that asserts a blocking defect, whatever channel it arrived on and whatever words it used** — replied inline threads and **issue-level comments** alike, each rendered **in full**. Recognition is semantic (`carriesBlockerSignal`), not the literal `**[Critical]**` marker, because only `/review` emits that marker and a human types whatever they type. This is the fix for a real dropped blocker: on PR #6486 a maintainer built the PR, drove the real CLI, and filed `🔴 Finding 1 — Ctrl+F dual-fires … (blocker)` as an **issue comment**. Every issue comment used to settle into "Already discussed" as a 240-character snippet, and the first 240 characters of that one were its preamble — _"I built this PR from source and drove the real CLI … to validate the model-toggle hotkey before merge"_ — which reads as an **endorsement**, filed under a heading that says not to re-report it. The blocker began 1 143 characters past the cut. `/review` reviewed that same commit three hours later and submitted "no blockers"; the defect was real and was fixed that evening. Promotion is deliberately fail-safe: a false positive costs one extra ruling, a false negative ships the bug. The file's own preamble tells agents to treat its contents as DATA, so no extra security prefix is needed when passing it to review agents. **If `pr-context` fails here too** (rate limit, network — the same-repo path is not immune), the handling is identical to lightweight mode: warn, continue, skip Agent 0, and set the **context-unavailable** state — Step 6 skips the re-check walk (every existing Critical is `cannot tell`) and Step 7 caps the event. A same-repo run that lost the context file must not behave as if it had read it. **`read_file` returns the first `truncateToolOutputThreshold` characters (25 000 by default) and sets `isTruncated`. Read that flag.** On a PR with a long history the context file exceeds it — `pr-context` prints a `warning:` line naming the size and any headings past the cut. When it does, page the remainder with `offset`/`limit` before Step 3, and pass the _whole_ file's contents onward. A review that never reached the open-comment section will report "no blockers" without having seen a single one of them. @@ -714,7 +716,7 @@ If there are none of either, omit this section. ### Before an Approve or a zero-Critical verdict: re-check the open Criticals -A `C=0` outcome — Approve, or a Comment with no Critical — is a claim that nothing blocks the merge. It is not the default you fall back to when your own agents surfaced nothing. **If Step 1 set the context-unavailable state** (`pr-context` failed — lightweight or same-repo), there is no context file to read: skip the walk below, record every existing Critical as `cannot tell` by construction, and carry that into the verdict — which the Step 7 invariant already caps at `COMMENT`. Otherwise, take **each live blocker already on the PR — from every comment-bearing section of the context file: "Open inline comments", "Replied Criticals", "Review summaries", and "Already discussed" (both its inline threads and its issue-level comments)** — and check it against the code as it stands at the reviewed commit. Select **semantically, not by the literal marker**: a `**[Critical]**` prefix qualifies, but so does any body that asserts a blocking defect in other words — a "Critical findings could not be anchored" preamble, an explicit must-fix claim (legacy body-only blockers were emitted markerless, and one such review is exactly what a marker filter once discarded). When unsure whether a body asserts a blocker, re-check it — the cost is one ruling; the alternative is certifying a merge past it. ("Already discussed" is in scope because `pr-context`'s quarantine keys on the literal marker — a fail-safe floor, not a ceiling. A blocker phrased without the marker — "Must fix: authorization bypass" — settles there with its "wontfix" reply, and every issue-level comment lands there too. That section's "do NOT re-report" header governs duplicate-reporting by the finder agents; it does not exempt a body from this re-check.) Review-level bodies matter because an unmappable or 422-relocated blocker lives **only** there — and the context file now carries them **in full**: `pr-context` renders every meaningful review body whole under "Review summaries" (no more 240-character snippets), and pulls every replied-to marker-carrying Critical thread into its own "Replied Criticals" section with the root body rendered in full, because a reply alone never settles a blocker. So the re-check usually needs no separate fetch: read those sections under the file's untrusted-data preamble, paging with `offset`/`limit` until `isTruncated` is false. Review summaries and Replied-Critical roots are rendered in full; the Open and Already-discussed sections use one-line snippets, and **every snippet the renderer cut carries its own `_(truncated — fetch …)_` note naming the exact, already-filled-in command for the rest** — a candidate blocker whose snippet was cut is ruled on only after running that fetch; ruling on the visible prefix alone is the fail-closed violation. Run any such fetch **redirected to a file, never into the terminal** (shell output truncates at 30 000 chars, which would re-truncate the very body being completed): append `--jq .body > .qwen/tmp/qwen-review-{target}-body-.md` to the command the note names, then `read_file` that file, paging until `isTruncated` is false, before ruling. **Fail closed either way:** a body you could not read whole — the capped tail unfetched, or the single-object fetch failing (auth, rate limit, network) — is `cannot tell`, not "no Critical in it": it goes to compose-review's `cannotTellCriticals` input, which serializes it and caps the event at `COMMENT`; a blocker you could not read is never approved past. A reply alone does not retire a blocker — "I disagree" or "wontfix" is a reply, which is exactly why `pr-context` quarantines replied Critical threads in their own section instead of letting them settle into "Already discussed". Only the code decides: a replied-to Critical counts as closed exactly when the re-check below lands on "fixed by this diff", never because the thread has an answer. Record one verdict per Critical: +A `C=0` outcome — Approve, or a Comment with no Critical — is a claim that nothing blocks the merge. It is not the default you fall back to when your own agents surfaced nothing. **If Step 1 set the context-unavailable state** (`pr-context` failed — lightweight or same-repo), there is no context file to read: skip the walk below, record every existing Critical as `cannot tell` by construction, and carry that into the verdict — which the Step 7 invariant already caps at `COMMENT`. Otherwise, take **each live blocker already on the PR — from every comment-bearing section of the context file: "Open inline comments", "Blockers to re-check", "Review summaries", and "Already discussed" (both its inline threads and its issue-level comments)** — and check it against the code as it stands at the reviewed commit. Select **semantically, not by the literal marker**: a `**[Critical]**` prefix qualifies, but so does any body that asserts a blocking defect in other words — a "Critical findings could not be anchored" preamble, an explicit must-fix claim (legacy body-only blockers were emitted markerless, and one such review is exactly what a marker filter once discarded). When unsure whether a body asserts a blocker, re-check it — the cost is one ruling; the alternative is certifying a merge past it. ("Already discussed" stays in scope even though `pr-context` now promotes blocker-bearing bodies out of it: `carriesBlockerSignal` is a **fail-safe floor, not a ceiling** — it recognises the phrasings we have seen, not every phrasing that exists, and a blocker worded around all of them still settles there. That section's "do NOT re-report" header governs duplicate-_reporting_ by the finder agents; it does not exempt a body from this re-check. Read it with the same eyes you bring to the promoted section.) Review-level bodies matter because an unmappable or 422-relocated blocker lives **only** there — and the context file now carries them **in full**: `pr-context` renders every meaningful review body whole under "Review summaries" (no more 240-character snippets), and pulls every blocker-bearing body — replied inline thread or issue comment, marker or no marker — into the "Blockers to re-check" section, rendered in full, because a reply alone never settles a blocker. So the re-check usually needs no separate fetch: read those sections under the file's untrusted-data preamble, paging with `offset`/`limit` until `isTruncated` is false. Review summaries and blocker bodies are rendered in full; the Open and Already-discussed sections use one-line snippets, and **every snippet the renderer cut carries its own `_(truncated — fetch …)_` note naming the exact, already-filled-in command for the rest** — a candidate blocker whose snippet was cut is ruled on only after running that fetch; ruling on the visible prefix alone is the fail-closed violation. Run any such fetch **redirected to a file, never into the terminal** (shell output truncates at 30 000 chars, which would re-truncate the very body being completed): append `--jq .body > .qwen/tmp/qwen-review-{target}-body-.md` to the command the note names, then `read_file` that file, paging until `isTruncated` is false, before ruling. **Fail closed either way:** a body you could not read whole — the capped tail unfetched, or the single-object fetch failing (auth, rate limit, network) — is `cannot tell`, not "no Critical in it": it goes to compose-review's `cannotTellCriticals` input, which serializes it and caps the event at `COMMENT`; a blocker you could not read is never approved past. A reply alone does not retire a blocker — "I disagree" or "wontfix" is a reply, which is exactly why `pr-context` quarantines blocker-bearing threads in their own section instead of letting them settle into "Already discussed". Only the code decides: a blocker counts as closed exactly when the re-check below lands on "fixed by this diff", never because the thread has an answer. Record one verdict per blocker: - **still stands** — the defect is present in the code you just read. It blocks: the event is `REQUEST_CHANGES`, and the finding goes inline (or into the body if it cannot be anchored). - **fixed by this diff** — you read the lines and the fix is there. Say nothing; do not re-report it. A GitHub thread can read `isResolved: false, isOutdated: false` for a bug a later commit fixed on an adjacent line — the flag tracks the anchored line, not the fix, so the flag is not evidence either way. Only the code is. From 893314c2d524dbc6044eb61e29e2241a83f26191 Mon Sep 17 00:00:00 2001 From: wenshao Date: Mon, 13 Jul 2026 12:44:07 +0800 Subject: [PATCH 02/12] fix(review): make "fixed by this diff" a verdict that has to be earned MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Step 6 re-check has three verdicts, and until now only two of them cost anything: still stands REQUEST_CHANGES — blocks the merge cannot tell serialized into the body, caps the event at COMMENT fixed by this diff nothing. Silent, free, unrecorded. An agent choosing among three answers where one is free and two are not drifts toward the free one — and the free one is the only one that can ship a bug. The bar for it also read "you read the lines and the fix is there", which invites reading the diff's lines. That is precisely the reading that fails: a fix's new lines are always in the diff, but whether they WORK routinely depends on code outside it. #6486 is the case. The author answered a Ctrl+F dual-fire blocker by adding a guard to the toggle handler — visible in the diff, and it reads like a fix. It changed nothing. The second handler is text-buffer.ts:2663, in a file the PR never touches, subscribed independently to a KeypressContext.broadcast() with no stop-propagation; returning from one subscriber does not stop the other. Read the diff and you see a guard and rule "fixed". Read text-buffer.ts:2663 and you cannot. Determinism owns the evidence, judgment owns the ruling: - pr-context extracts the evidence. A blocker's body names the code it is about — #6486's named text-buffer.ts:2663 outright — so every promoted blocker now renders a "Referenced code" list. "Go read the untouched code" stops being a hope the agent might have and becomes a list it is handed. - SKILL.md raises the bar on the ruling: name the mechanism, name what now stops it, and when the stopping condition lives outside the diff, read it there — or the verdict is `cannot tell`. No new compose-review input: `cannot tell` already caps the event. The change is to make wrong "fixed" rulings land there instead of passing silently. --- .../src/commands/review/pr-context.test.ts | 101 ++++++++++++++++++ .../cli/src/commands/review/pr-context.ts | 55 ++++++++++ .../core/src/skills/bundled/review/DESIGN.md | 23 ++++ .../core/src/skills/bundled/review/SKILL.md | 9 +- 4 files changed, 187 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/commands/review/pr-context.test.ts b/packages/cli/src/commands/review/pr-context.test.ts index f428a585065..bd99276bd7e 100644 --- a/packages/cli/src/commands/review/pr-context.test.ts +++ b/packages/cli/src/commands/review/pr-context.test.ts @@ -17,6 +17,7 @@ import { truncatedHeadings, buildMarkdown, carriesBlockerSignal, + extractCodeRefs, classifyInlineThreads, fullBody, fullCommentBody, @@ -457,6 +458,53 @@ describe('buildMarkdown — a markerless maintainer blocker must not render as a ).toBe(true); }); + it('hands the re-check the untouched file the fix turns on', () => { + const md = render(); + // The blocker names `text-buffer.ts:2663` — a file THIS PR NEVER TOUCHES, + // and the reason the author's first fix (a guard, plainly visible in the + // diff) was inert. An agent that rules "fixed" from the diff alone rules + // wrong. Extracting the reference turns "go read the untouched code" from + // a hope into a list the agent is handed. + expect(md).toContain('**Referenced code'); + expect(md).toContain('`text-buffer.ts:2663`'); + }); + + it('puts the blockers where one read_file can see them', () => { + // Found by running it against the live thread, not by any unit test. The + // section was originally written after "Open inline comments"; on #6486 that + // put its heading at char 25 961 and the blocker body at 43 094 — both past + // the 25 000 chars one `read_file` returns. The blocker was in the file and + // nobody could read it, which is strictly no better than not promoting it. + const md = render(); + const section = md.indexOf('## Blockers to re-check'); + const blocker = md.indexOf('dual-fires'); + expect(section).toBeGreaterThanOrEqual(0); + expect(section).toBeLessThan(md.indexOf('## Description')); + expect(blocker).toBeLessThan(25_000); + }); + + it('does not promote the triage bot saying there are NO blockers', () => { + // "No critical blockers." is the triage bot's own template line. A + // whole-body keyword scan fired on it, on every PR it ever commented on — + // and each false promotion spends the read budget the real blocker needs. + const md = buildMarkdown( + '6486', + 'QwenLM/qwen-code', + meta, + [], + [ + { id: 1, user: { login: 'bot' }, body: 'No critical blockers. LGTM.' }, + { + id: 2, + user: { login: 'author' }, + body: '### 🔴 Critical fixes\nAddressed all 3 findings.', + }, + ], + [], + ); + expect(md).not.toContain('## Blockers to re-check'); + }); + it('still lets ordinary chatter settle into Already discussed', () => { const md = render(); const alreadyDiscussed = md.indexOf('## Already discussed'); @@ -468,6 +516,59 @@ describe('buildMarkdown — a markerless maintainer blocker must not render as a }); }); +describe('extractCodeRefs', () => { + it('pulls the locations a blocker points at, with line numbers', () => { + expect( + extractCodeRefs( + "`text-buffer.ts:2663` still binds `Ctrl+F → move('right')`, and the " + + 'handler in `AppContainer.tsx` is an independent subscriber.', + ), + ).toEqual(['text-buffer.ts:2663', 'AppContainer.tsx']); + }); + + it('keeps full paths and line ranges', () => { + expect( + extractCodeRefs('see packages/cli/src/ui/x.ts:10-20 and lib/y.go:3'), + ).toEqual(['packages/cli/src/ui/x.ts:10-20', 'lib/y.go:3']); + }); + + it('dedups repeats and bounds the list', () => { + expect(extractCodeRefs('a.ts:1 a.ts:1 a.ts:1')).toEqual(['a.ts:1']); + const many = Array.from({ length: 30 }, (_, i) => `f${i}.ts`).join(' '); + expect(extractCodeRefs(many)).toHaveLength(12); + }); + + it('collapses a bare filename into the full path naming the same location', () => { + // Reports name a location twice — once bare, once by path. Keep the one + // the reader can actually open. + expect( + extractCodeRefs( + '`text-buffer.ts:2663` still binds it; remove it at ' + + '`packages/cli/src/ui/components/shared/text-buffer.ts:2663`.', + ), + ).toEqual(['packages/cli/src/ui/components/shared/text-buffer.ts:2663']); + // Different lines in the same file are different locations — keep both. + expect(extractCodeRefs('a/b.ts:1 and a/b.ts:2')).toEqual([ + 'a/b.ts:1', + 'a/b.ts:2', + ]); + }); + + it('keeps a scoped or relative path prefix intact', () => { + // `\b` fires on the first word-character transition, so `@scope/…` came back + // as `scope/…` and `../lib/b.ts` as `lib/b.ts` — not the path that was cited. + expect(extractCodeRefs('see @scope/pkg/index.ts:10')).toEqual([ + '@scope/pkg/index.ts:10', + ]); + expect(extractCodeRefs('see ../lib/b.ts')).toEqual(['../lib/b.ts']); + }); + + it('returns nothing for a body that names no code', () => { + expect(extractCodeRefs('LGTM, ship it')).toEqual([]); + expect(extractCodeRefs(undefined)).toEqual([]); + }); +}); + describe('carriesBlockerSignal', () => { it('recognises a blocker that never uses the [Critical] marker', () => { // The real PR #6486 heading. Only /review emits `[Critical]`; a human diff --git a/packages/cli/src/commands/review/pr-context.ts b/packages/cli/src/commands/review/pr-context.ts index 17a0b7f096b..8c935495239 100644 --- a/packages/cli/src/commands/review/pr-context.ts +++ b/packages/cli/src/commands/review/pr-context.ts @@ -168,6 +168,47 @@ export function fullIssueCommentBody( ); } +/** + * Code locations a blocker's body points at, in the order they appear. + * + * The Step 6 re-check rules "fixed by this diff" by reading the code. The trap + * is *which* code: a fix's new lines are in the diff, but whether they actually + * work often turns on a file the diff never touches, and an agent reading only + * the diff sees a plausible-looking fix and rules it good. + * + * PR #6486 again. The author's first fix added a guard to the toggle handler — + * visible in the diff, and it looks like a fix. It changed nothing: `Ctrl+F` + * still dual-fired, because the second handler is `text-buffer.ts:2663`, an + * untouched file, subscribed independently to the same broadcast. The blocker's + * body *names that line*. So the evidence the re-check needs is right there in + * the text — it just has to be pulled out and handed over as a read list, not + * left for an agent to notice inside 6 000 characters of prose. + * + * Deliberately loose: a path-shaped token with a known-ish extension, optional + * `:line` (or `:line-line`). Over-matching costs one file read; under-matching + * costs the ruling. `MAX_CODE_REFS` bounds the render, since a long report can + * name a lot of files. + */ +// The leading boundary is a lookbehind, not `\b`: `\b` fires on the first +// word-character transition, so `@scope/pkg/index.ts` extracted as +// `scope/pkg/index.ts` and `../lib/b.ts` as `lib/b.ts` — a path whose meaning +// is not the path that was cited. +const CODE_REF_RE = + /(? m[0])), + ]; + // A report routinely names the same location twice — once bare and once by + // full path (`text-buffer.ts:2663` and `packages/.../text-buffer.ts:2663`). + // Keep the fuller path: it is the one the reader can open. + const refs = all.filter( + (r) => !all.some((other) => other !== r && other.endsWith(`/${r}`)), + ); + return refs.slice(0, MAX_CODE_REFS); +} + /** * Does this body assert a blocking defect? * @@ -417,6 +458,8 @@ function blockerSection( '## Blockers to re-check — a reply alone does NOT retire a blocker; the re-check must rule on each against the code', '', '> Bodies are rendered in full; a body cut at a cap names its comment id to fetch, and a body read in part is `cannot tell`, never "no blocker in it".', + '>', + '> **Ruling "fixed by this diff" means reading the code the blocker names — including the files this PR never touches.** Each blocker below carries a **Referenced code** list extracted from its own body. A fix whose new lines are in the diff can still be inert because of a file outside it (PR #6486: the added guard looked right; `Ctrl+F` still dual-fired, because the second handler lived in an untouched file). A location you did not read is not evidence of a fix — that ruling is `cannot tell`.', '', ]; @@ -434,6 +477,16 @@ function blockerSection( spent += lines.join('\n').length; return lines; }; + const refsLine = (body: string | undefined): string[] => { + const refs = extractCodeRefs(body); + return refs.length > 0 + ? [ + `**Referenced code — read each at the reviewed commit before ruling:** ${refs.map((r) => `\`${r}\``).join(', ')}`, + '', + ] + : []; + }; + const sortedRoots = [...roots].sort((a, b) => { const p = (a.path ?? '').localeCompare(b.path ?? ''); if (p !== 0) return p; @@ -462,6 +515,7 @@ function blockerSection( ]), ); } + out.push(...charge(refsLine(root.body))); const replies = repliesByRoot.get(root.id) ?? []; if (replies.length > 0) { out.push( @@ -500,6 +554,7 @@ function blockerSection( ]), ); } + out.push(...charge(refsLine(c.body))); } return out; } diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index 8b14df4b046..ecc90969136 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -330,6 +330,29 @@ Three changes, and the ordering one is load-bearing: The lesson generalizes past this file: **"a false positive is cheap" is a claim about a budget, and it has to be measured against the real distribution, not assumed.** Here it was false until the ordering was fixed. +## Why "fixed by this diff" is the verdict that needed a bar + +The re-check has three verdicts, and until PR #6486 only two of them cost anything: + +| verdict | consequence | +| -------------------- | ----------------------------------------------------- | +| `still stands` | `REQUEST_CHANGES` — blocks the merge | +| `cannot tell` | serialized into the body, caps the event at `COMMENT` | +| `fixed by this diff` | **nothing. Silent, free, unrecorded.** | + +An agent under context pressure, choosing among three answers where one is free and two are not, drifts toward the free one — and the free one is the only one that can ship a bug. + +Worse, the bar for it read "you read the lines and the fix is there", which invites reading **the diff's lines**. That is precisely the reading that fails. A fix's new lines are always in the diff; whether they _work_ routinely depends on code outside it. + +PR #6486 is the case. A `Ctrl+F` dual-fire blocker was filed — the hotkey toggled the model _and_ moved the input cursor. The author added a guard to the toggle handler: visible in the diff, and it reads like a fix. It changed nothing. The second handler is `text-buffer.ts:2663`, in a file the PR never touches, subscribed independently to a `KeypressContext.broadcast()` that has no stop-propagation — `return`ing from one subscriber does not stop the other. Read the diff and you see a guard and rule "fixed". Read `text-buffer.ts:2663` and you cannot. + +Two changes, split the way this document keeps arriving at — **determinism owns the evidence, judgment owns the ruling**: + +- **`pr-context` extracts the evidence** (`extractCodeRefs`). A blocker's body names the code it is about — #6486's named `text-buffer.ts:2663` outright — so every promoted blocker now renders a **Referenced code** list. "Go read the untouched code" stops being a hope the agent might have and becomes a list it is handed. +- **SKILL.md raises the bar** on the ruling: name the mechanism, name what now stops it, and when the stopping condition lives outside the diff, read it there — or the verdict is `cannot tell`. + +No new `compose-review` input was needed: `cannot tell` already caps the event. The change is to make wrong "fixed" rulings land there instead of passing silently. + ## What the first dogfood batch changed Six concurrent real-PR runs (batch 3) produced three targeted changes, each fixing something the batch measured rather than predicted: diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index dd80abe7625..9ae7d955f98 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -719,7 +719,14 @@ If there are none of either, omit this section. A `C=0` outcome — Approve, or a Comment with no Critical — is a claim that nothing blocks the merge. It is not the default you fall back to when your own agents surfaced nothing. **If Step 1 set the context-unavailable state** (`pr-context` failed — lightweight or same-repo), there is no context file to read: skip the walk below, record every existing Critical as `cannot tell` by construction, and carry that into the verdict — which the Step 7 invariant already caps at `COMMENT`. Otherwise, take **each live blocker already on the PR — from every comment-bearing section of the context file: "Open inline comments", "Blockers to re-check", "Review summaries", and "Already discussed" (both its inline threads and its issue-level comments)** — and check it against the code as it stands at the reviewed commit. Select **semantically, not by the literal marker**: a `**[Critical]**` prefix qualifies, but so does any body that asserts a blocking defect in other words — a "Critical findings could not be anchored" preamble, an explicit must-fix claim (legacy body-only blockers were emitted markerless, and one such review is exactly what a marker filter once discarded). When unsure whether a body asserts a blocker, re-check it — the cost is one ruling; the alternative is certifying a merge past it. ("Already discussed" stays in scope even though `pr-context` now promotes blocker-bearing bodies out of it: `carriesBlockerSignal` is a **fail-safe floor, not a ceiling** — it recognises the phrasings we have seen, not every phrasing that exists, and a blocker worded around all of them still settles there. That section's "do NOT re-report" header governs duplicate-_reporting_ by the finder agents; it does not exempt a body from this re-check. Read it with the same eyes you bring to the promoted section.) Review-level bodies matter because an unmappable or 422-relocated blocker lives **only** there — and the context file now carries them **in full**: `pr-context` renders every meaningful review body whole under "Review summaries" (no more 240-character snippets), and pulls every blocker-bearing body — replied inline thread or issue comment, marker or no marker — into the "Blockers to re-check" section, rendered in full, because a reply alone never settles a blocker. So the re-check usually needs no separate fetch: read those sections under the file's untrusted-data preamble, paging with `offset`/`limit` until `isTruncated` is false. Review summaries and blocker bodies are rendered in full; the Open and Already-discussed sections use one-line snippets, and **every snippet the renderer cut carries its own `_(truncated — fetch …)_` note naming the exact, already-filled-in command for the rest** — a candidate blocker whose snippet was cut is ruled on only after running that fetch; ruling on the visible prefix alone is the fail-closed violation. Run any such fetch **redirected to a file, never into the terminal** (shell output truncates at 30 000 chars, which would re-truncate the very body being completed): append `--jq .body > .qwen/tmp/qwen-review-{target}-body-.md` to the command the note names, then `read_file` that file, paging until `isTruncated` is false, before ruling. **Fail closed either way:** a body you could not read whole — the capped tail unfetched, or the single-object fetch failing (auth, rate limit, network) — is `cannot tell`, not "no Critical in it": it goes to compose-review's `cannotTellCriticals` input, which serializes it and caps the event at `COMMENT`; a blocker you could not read is never approved past. A reply alone does not retire a blocker — "I disagree" or "wontfix" is a reply, which is exactly why `pr-context` quarantines blocker-bearing threads in their own section instead of letting them settle into "Already discussed". Only the code decides: a blocker counts as closed exactly when the re-check below lands on "fixed by this diff", never because the thread has an answer. Record one verdict per blocker: - **still stands** — the defect is present in the code you just read. It blocks: the event is `REQUEST_CHANGES`, and the finding goes inline (or into the body if it cannot be anchored). -- **fixed by this diff** — you read the lines and the fix is there. Say nothing; do not re-report it. A GitHub thread can read `isResolved: false, isOutdated: false` for a bug a later commit fixed on an adjacent line — the flag tracks the anchored line, not the fix, so the flag is not evidence either way. Only the code is. +- **fixed by this diff** — you traced the blocker's **mechanism** through the code as it now stands and it can no longer fire. Say nothing; do not re-report it. A GitHub thread can read `isResolved: false, isOutdated: false` for a bug a later commit fixed on an adjacent line — the flag tracks the anchored line, not the fix, so the flag is not evidence either way. Only the code is. + + **"The diff adds a fix" is not the same claim as "the defect can no longer fire", and this verdict requires the second one.** A fix's new lines are in the diff, but whether they _work_ frequently turns on code the diff never touches — a sibling subscriber, a registry entry, a dispatch order, a global binding, a default in a caller three files away. Read the diff alone and you see a plausible fix and rule it good. **So: name the mechanism the blocker claims, then name what now stops it. If that stopping condition lives outside the diff, go read it at the reviewed commit — a blocker in "Blockers to re-check" carries a `Referenced code` list extracted from its own body **whenever it names a file**, and the locations on it that the PR does not touch are precisely the ones this rule is about.** If you did not read them, you do not have this verdict; you have `cannot tell`. A blocker that cites no file gets no list, and hands you no shortcut: trace the mechanism through the code yourself, on the same terms. + + This is not a hypothetical. On PR #6486 the author responded to a `Ctrl+F` dual-fire blocker by adding a guard to the toggle handler. The guard is right there in the diff and reads like a fix. It changed nothing — `Ctrl+F` still toggled the model **and** moved the cursor, because the second handler is `text-buffer.ts:2663` in an untouched file, subscribed independently to a `KeypressContext.broadcast()` with no stop-propagation. The blocker's own body named that line. A re-check that read only the diff would rule "fixed" and be wrong; a re-check that read the named line could not. + + **Of the three verdicts, this is the only one with no consequence** — `still stands` blocks the merge, `cannot tell` caps the event at `COMMENT`, and `fixed` is free and silent. That asymmetry is a gradient toward the cheapest answer, and it is exactly the answer that ships the bug. Do not take it without the trace. + - **cannot tell** — you could not reach a verdict from the code (including: its full text could not be fetched). It goes into the review body via compose-review's `cannotTellCriticals` input (Step 7), which survives every downgrade and the 422 recovery — so it does not silently vanish, forbids the "no blockers" opener, and caps a would-be Approve at `COMMENT`. Two failure modes this closes, both observed in this repo's own dogfood: reporting a Critical that cites code **not present** at the reviewed commit (a fabricated blocker), and submitting `C=0` while a **live, already-filed** Critical still stands (a dropped blocker). The event must follow from reading the code, never from the finding count or the thread flags. From 6be68208470d476e30a4b9bb7b234597dad9a79e Mon Sep 17 00:00:00 2001 From: wenshao Date: Mon, 13 Jul 2026 12:44:18 +0800 Subject: [PATCH 03/12] fix(review): a skipped CI check is not a passing CI check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitHub reports a skipped job as `status: completed, conclusion: skipped`. The classifier tested for failure conclusions and for pending statuses, and `skipped` matched neither — so it fell through both branches and landed the run in `all_pass`. A job that never ran was scored as a job that passed. This is load-bearing. /review treats green CI as its licence to approve, and the whole design delegates runtime truth to CI precisely because the LLM pipeline reads code statically. If the delegation returns nothing and returns it wearing a green badge, the delegation is worse than not having it. On #6486 the one job that would have exercised the new hotkey — "Integration Tests (CLI, No Sandbox)" — was skipped, as were the macOS and Windows Test legs. all_pass. "Did it run" is a question about the check NAME, not about any single run: this repo's routing workflows (authorize, review-pr, precheck-pr) routinely emit both a skipped and a successful run of the same name, and reporting those as unrun would bury the one skipped check that matters under a dozen that do not. A name counts as executed if any of its runs reached a real conclusion. Two deliberately different consequences: - Some checks skipped -> a disclosure, not a downgrade. A docs-only PR legitimately skips the test matrix, and auto-downgrading on any skip would downgrade every review in this repo, which is how a gate gets ignored. So presubmit names them and Step 7 rules on them — whether a skipped check would have exercised THIS diff is a question about the diff, which presubmit cannot see and the reviewer can. - Every check skipped -> a downgrade. Checks exist, not one ran: there is no green here to approve on, and no judgment is required to say so. A repo with no CI at all is a different claim (totalChecks === 0) and is not downgraded. The check-run shapes in the tests are the real ones from 6486's head commit. --- .../cli/src/commands/review/presubmit.test.ts | 151 +++++++++++++++++- packages/cli/src/commands/review/presubmit.ts | 70 +++++++- .../core/src/skills/bundled/review/DESIGN.md | 9 ++ .../core/src/skills/bundled/review/SKILL.md | 7 + 4 files changed, 234 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/commands/review/presubmit.test.ts b/packages/cli/src/commands/review/presubmit.test.ts index 1259ad93086..393bd4bccc6 100644 --- a/packages/cli/src/commands/review/presubmit.test.ts +++ b/packages/cli/src/commands/review/presubmit.test.ts @@ -5,7 +5,123 @@ */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { presubmitCommand } from './presubmit.js'; +import { presubmitCommand, classifyCi } from './presubmit.js'; + +// A `skipped` check run arrives as `status: completed` with `conclusion: +// "skipped"`. It used to fall through both branches of the classifier and land +// the run in `all_pass`: a job that never ran, scored as a job that passed. +// +// `/review` treats green CI as its licence to approve, and the design +// explicitly delegates runtime truth to CI ("the LLM pipeline reads code +// statically… CI does not"). On PR #6486 the delegation returned nothing and +// returned it looking like a pass — `Integration Tests (CLI, No Sandbox)` was +// skipped, along with the macOS and Windows `Test` legs. +// +// The shapes below are the real check runs on 6486's head commit `240c08545`. +describe('classifyCi — a skipped check is not a passing check', () => { + const run = (name: string, conclusion: string, status = 'completed') => ({ + name, + status, + conclusion, + }); + + it('names the checks that never executed at this commit', () => { + const got = classifyCi( + [ + run('Test (ubuntu-latest, Node 22.x)', 'success'), + run('Test (macos-latest, Node 22.x)', 'skipped'), + run('Test (windows-latest, Node 22.x)', 'skipped'), + run('Integration Tests (CLI, No Sandbox)', 'skipped'), + ], + [], + ); + expect(got.skippedCheckNames).toEqual([ + 'Integration Tests (CLI, No Sandbox)', + 'Test (macos-latest, Node 22.x)', + 'Test (windows-latest, Node 22.x)', + ]); + // Something real did pass, so the class is still all_pass — the skipped + // names are a DISCLOSURE, not a downgrade. Whether a skipped check would + // have exercised this particular diff is a question about the diff, which + // presubmit cannot see; Step 7 rules on it. + expect(got.class).toBe('all_pass'); + }); + + it('does not report a name that also ran under another check run', () => { + // This repo's routing workflows emit both a skipped and a successful run + // of the same name (`authorize`, `review-pr`, `precheck-pr`). Reporting + // those as unrun would bury the one skipped check that matters under a + // dozen that do not. + const got = classifyCi( + [ + run('authorize', 'skipped'), + run('authorize', 'success'), + run('review-pr', 'skipped'), + run('review-pr', 'success'), + run('Integration Tests (CLI, No Sandbox)', 'skipped'), + ], + [], + ); + expect(got.skippedCheckNames).toEqual([ + 'Integration Tests (CLI, No Sandbox)', + ]); + }); + + it('calls it no_checks when checks exist and NOT ONE of them ran', () => { + // The unambiguous case: there is no green here to approve on. + const got = classifyCi( + [run('Test (ubuntu-latest)', 'skipped'), run('Lint', 'skipped')], + [], + ); + expect(got.class).toBe('no_checks'); + expect(got.totalChecks).toBe(2); + }); + + it('still fails on a real failure and waits on a real pending', () => { + expect( + classifyCi([run('Test', 'failure'), run('Lint', 'skipped')], []).class, + ).toBe('any_failure'); + expect( + classifyCi([run('Test', '', 'in_progress'), run('Lint', 'skipped')], []) + .class, + ).toBe('all_pending'); + }); + + it('treats `neutral` and `stale` as not-run, like `skipped`', () => { + // GitHub's other "completed but nothing happened" conclusions. They arrive + // on the same code path and mean the same thing for a review: no evidence. + // `stale` in particular is a check GitHub superseded — it produced no + // verdict about this commit, and scoring it as executed is the same mistake + // as scoring `skipped` as a pass. + const got = classifyCi( + [ + run('Test', 'success'), + run('Coverage Gate', 'neutral'), + run('Lint', 'stale'), + ], + [], + ); + expect(got.skippedCheckNames).toEqual(['Coverage Gate', 'Lint']); + expect(got.class).toBe('all_pass'); + }); + + it('names a completed check that produced NO conclusion, instead of "skipped ()"', () => { + // A completed run with a null conclusion was invisible to both tallies, so + // the class fell through to `no_checks` while `skippedCheckNames` stayed + // empty — the downgrade then read "every check was skipped ()", naming + // nothing. A run that produced no verdict did not run. + const got = classifyCi([run('Ghost Check', '' as unknown as string)], []); + expect(got.skippedCheckNames).toEqual(['Ghost Check']); + expect(got.class).toBe('no_checks'); + }); + + it('a repo with no CI at all is still no_checks, with nothing to disclose', () => { + const got = classifyCi([], []); + expect(got.class).toBe('no_checks'); + expect(got.totalChecks).toBe(0); + expect(got.skippedCheckNames).toEqual([]); + }); +}); const { ghMock, @@ -82,6 +198,39 @@ describe('presubmitCommand', () => { } }); + it('sets downgradeApprove — not just a reason — when every check was skipped', async () => { + // The bug this guards was found by dogfooding /review on this very change: + // `downgradeReasons` gained a "CI did not run" entry while `downgradeApprove` + // — the boolean compose-review actually acts on — did not. The disclosure was + // written and the downgrade never fired. A reason nobody reads is not a gate, + // so the assertion is on the boolean, through the real command. + ghApiMock.mockImplementation((path: string) => { + if (path.endsWith('/check-runs')) { + return { + check_runs: [ + { name: 'Test', status: 'completed', conclusion: 'skipped' }, + { name: 'Lint', status: 'completed', conclusion: 'skipped' }, + ], + }; + } + if (path.endsWith('/status')) return { statuses: [] }; + return null; + }); + + const handler = presubmitCommand.handler; + if (!handler) throw new Error('presubmit handler missing'); + await handler(baseArgs as Parameters[0]); + + const [, content] = writeFileSyncMock.mock.calls.find( + ([path]) => path === '/tmp/presubmit.json', + ) ?? [null, null]; + const result = JSON.parse(String(content)); + + expect(result.ciStatus.class).toBe('no_checks'); + expect(result.downgradeApprove).toBe(true); + expect(result.downgradeReasons.join(' ')).toContain('CI did not run'); + }); + it('ignores the running Qwen PR review check when deciding whether CI is still pending', async () => { ghApiMock.mockImplementation((path: string) => { if (path.endsWith('/check-runs')) { diff --git a/packages/cli/src/commands/review/presubmit.ts b/packages/cli/src/commands/review/presubmit.ts index eb9a3c5923c..5c17776bcf5 100644 --- a/packages/cli/src/commands/review/presubmit.ts +++ b/packages/cli/src/commands/review/presubmit.ts @@ -66,6 +66,22 @@ const FAIL_CONCLUSIONS = new Set([ const FAIL_STATUS_STATES = new Set(['failure', 'error']); const PENDING_STATES = new Set(['queued', 'in_progress', 'pending']); +/** + * Conclusions that mean the job did not execute. GitHub reports these with + * `status: completed`, so they used to fall through both branches of the + * classifier and land the run in `all_pass` — a job that never ran was scored + * as a job that passed. + * + * This is not a theoretical hole. `/review` treats green CI as its licence to + * approve (see "Why downgrade APPROVE when CI is non-green" in DESIGN.md), and + * the whole design delegates runtime truth to CI because the LLM pipeline reads + * code statically. On PR #6486 the one job that would have exercised the new + * hotkey — `Integration Tests (CLI, No Sandbox)` — was `skipped`, as were the + * macOS and Windows `Test` jobs. The delegation returned nothing, and returned + * it looking like a pass. + */ +const NOT_RUN_CONCLUSIONS = new Set(['skipped', 'neutral', 'stale']); + function isCurrentActionsRunCheck(run: CheckRun): boolean { const runId = process.env['GITHUB_RUN_ID']; if (!runId) return false; @@ -84,13 +100,38 @@ interface PresubmitArgs { 'new-findings'?: string; } -function classifyCi(checkRuns: CheckRun[], statuses: CommitStatus[]) { +export function classifyCi(checkRuns: CheckRun[], statuses: CommitStatus[]) { const failedCheckNames: string[] = []; let hasPending = false; const relevantCheckRuns = checkRuns.filter( (run) => !isCurrentActionsRunCheck(run), ); + // A job that ran and a job that was skipped can share a name — GitHub emits + // one check run per matrix leg and per re-dispatch, and this repo's routing + // workflows (`authorize`, `review-pr`, `precheck-pr`) routinely produce both. + // So "did it run" is a question about the NAME, not about any single run: + // a name counts as executed if ANY of its runs reached a real conclusion. + // Without this, every review would disclose a dozen routing jobs as unrun. + const executedNames = new Set(); + const notRunNames = new Set(); + for (const run of relevantCheckRuns) { + if (run.status !== 'completed') continue; + if (!run.conclusion || NOT_RUN_CONCLUSIONS.has(run.conclusion)) { + // A completed run with NO conclusion produced no verdict about this + // commit, which is the same thing `skipped` means for a review. Leaving it + // invisible to both tallies made the class fall through to `no_checks` + // while `skippedCheckNames` stayed empty — the downgrade then read + // "every check was skipped ()", naming nothing. + notRunNames.add(run.name); + } else { + executedNames.add(run.name); + } + } + const skippedCheckNames = [...notRunNames] + .filter((n) => !executedNames.has(n)) + .sort(); + for (const run of relevantCheckRuns) { if (run.status === 'completed') { if (run.conclusion && FAIL_CONCLUSIONS.has(run.conclusion)) { @@ -115,6 +156,11 @@ function classifyCi(checkRuns: CheckRun[], statuses: CommitStatus[]) { cls = 'no_checks'; } else if (hasPending) { cls = 'all_pending'; + } else if (executedNames.size === 0 && statuses.length === 0) { + // Every check was skipped. Nothing ran, nothing failed — and the old + // classifier called that `all_pass`, licensing an approval on the strength + // of a CI run that did not happen. + cls = 'no_checks'; } else { cls = 'all_pass'; } @@ -122,6 +168,13 @@ function classifyCi(checkRuns: CheckRun[], statuses: CommitStatus[]) { return { class: cls, failedCheckNames, + /** + * Checks that never executed at this commit. NOT a downgrade on its own — + * most are routing jobs, and a docs-only PR legitimately skips the test + * matrix. It is a disclosure: Step 7 rules on whether a skipped check is + * one that would have exercised THIS diff, which presubmit cannot know. + */ + skippedCheckNames, totalChecks: relevantCheckRuns.length + statuses.length, }; } @@ -237,6 +290,14 @@ async function runPresubmit(args: PresubmitArgs): Promise { if (ciStatus.class === 'all_pending') { downgradeReasons.push('CI still running'); } + // Checks exist at this commit and NOT ONE of them executed. There is no + // green to approve on. (A repo with no CI at all is `no_checks` with + // `totalChecks === 0` and is not downgraded — that is a different claim.) + if (ciStatus.class === 'no_checks' && ciStatus.totalChecks > 0) { + downgradeReasons.push( + `CI did not run: every check was skipped (${ciStatus.skippedCheckNames.join(', ')})`, + ); + } const result = { prNumber, @@ -257,10 +318,15 @@ async function runPresubmit(args: PresubmitArgs): Promise { resolved: buckets.resolved, noConflict: buckets.noConflict, }, + // `no_checks` with checks present means not one of them ran — the + // downgradeReasons entry above says so, and this is the boolean that makes + // compose-review act on it. Omitting it made the whole disclosure inert: + // the reason was written and the downgrade never fired. downgradeApprove: isSelfPr || ciStatus.class === 'any_failure' || - ciStatus.class === 'all_pending', + ciStatus.class === 'all_pending' || + (ciStatus.class === 'no_checks' && ciStatus.totalChecks > 0), downgradeRequestChanges: isSelfPr, downgradeReasons, blockOnExistingComments: buckets.overlap.length > 0, diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index ecc90969136..e09abcce977 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -228,6 +228,15 @@ Line-based classification was chosen because it's deterministic, cheap, and catc - Any failure → downgrade `APPROVE` to `COMMENT`, body explains. - All pending → downgrade to `COMMENT` (don't approve before CI decides), body explains. +**The hole under all of this: a check that never ran looked like a check that passed.** GitHub reports a skipped job as `status: completed, conclusion: skipped`. The classifier tested for failure conclusions and for pending statuses, and `skipped` matched neither — so it fell through into `all_pass`. Every word above delegates runtime truth to CI _because_ the LLM pipeline reads code statically. If the delegation returns nothing, and returns it wearing a green badge, the delegation is worse than not having it. + +PR #6486: the one job that would have exercised the new `Ctrl+F` hotkey — `Integration Tests (CLI, No Sandbox)` — was skipped, as were the macOS and Windows `Test` legs. `all_pass`. And even had it run, it would have passed: the test drove a CSI-u sequence into a PTY that never negotiated the kitty protocol, so the keypress was discarded before reaching the handler. A test that cannot fail, in a job that did not run, scored as verification. + +`skipped`/`neutral` are now recognised, with two deliberately different consequences: + +- **Some checks skipped → a disclosure, not a downgrade.** Empirically this repo emits skipped runs constantly — routing jobs (`authorize`, `review-pr`, `precheck-pr`) that also emit a successful run of the same name, which is why "did it run" is a question about the _name_, not about any single run. And a docs-only PR legitimately skips the test matrix. Auto-downgrading on any skip would downgrade every review in the repo, which is how a gate gets ignored. So presubmit _names_ them and Step 7 rules on them — because whether a skipped check would have exercised **this** diff is a question about the diff, which presubmit cannot see and the reviewer can. +- **Every check skipped → a downgrade.** Checks exist, not one ran: there is no green here to approve on, and no judgment is required to say so. (A repo with no CI at all is a different claim — `totalChecks === 0`, not downgraded.) + **Why downgrade rather than block:** the reviewer LLM has done substantive work; throwing the review away because CI is red wastes that. Downgrading to `COMMENT` keeps all inline findings, preserves the static review value, and lets GitHub's check status carry the "do not merge" signal naturally. **Why this stacks with self-PR downgrade:** a self-authored PR with red CI hits **both** downgrade rules. The event is `COMMENT` either way, so stacking is operationally a no-op — but the body should mention both reasons so a future maintainer reading the review knows why an LLM that found no Critical issues did not approve. diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 9ae7d955f98..8362332e65f 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -798,6 +798,7 @@ Read `.qwen/tmp/qwen-review-{target}-presubmit.json`. Schema: ciStatus: { class: 'all_pass' | 'any_failure' | 'all_pending' | 'no_checks'; failedCheckNames: string[]; // failing check names — include in body text + skippedCheckNames: string[]; // checks that NEVER RAN at this commit — see below totalChecks: number; }; existingComments: { @@ -819,6 +820,12 @@ Read `.qwen/tmp/qwen-review-{target}-presubmit.json`. Schema: - `blockOnExistingComments=true` → **an overlap is a duplicate; the disposal is deterministic — do not ask the user.** Drop each finding whose `(path, line)` appears in `existingComments.overlap` from your `comments` array (adjusting the counts you hand to `compose-review`: a dropped Critical was already reported on the PR, so it is neither `criticalsInline` nor `bodyCriticals`; a dropped Suggestion joins neither count), list the dropped findings in the terminal summary as "already reported at :", and submit the remainder without pausing. Dogfooding measured this exact decision point improvised as an interactive question in 2 of 6 runs — which stalls a headless run forever — while the other 4 runs proceeded; the Exclusion Criteria already forbid re-reporting discussed issues, so there is nothing to ask. (If dropping overlaps leaves zero findings, that is still not a question: run `compose-review` with the remaining counts like any other submission.) - `downgradeApprove` / `downgradeRequestChanges` / `downgradeReasons` → **do not apply these by hand.** Copy them into the `presubmit` field of the `compose-review` input (below); the subcommand owns the semantics its tests pin — a downgrade fires only when the verdict it names is the one on the table (a Suggestion-only review is already Comment, so nothing is downgraded and no "Downgraded" sentence is emitted), the downgrade sentence carries the reasons, and a downgraded Request changes keeps its body Criticals after the sentence so the self-PR downgrade never erases the only copy of a blocker. +- `ciStatus.skippedCheckNames` → **a green CI is not evidence about a check that never ran.** These are checks that reached `completed` with `skipped`, `neutral`, `stale`, or **no conclusion at all** at this commit — GitHub reports them alongside the passing ones, and this classifier used to score them as passes. Most are routing jobs and are noise; a docs-only PR legitimately skips the test matrix. But **presubmit cannot know which of them would have exercised _this_ diff, and you can** — you have `files[]`. So rule on the list: for each skipped check, ask whether it is the one that would have run the code this PR changes (a test job whose suite covers the changed package; the integration/E2E job for a feature whose only new test lives there). If one is, then **CI verified nothing about this change**, and the review must say so rather than resting on the green: + - Name the skipped check in the terminal output, always. + - If Agent 7's build/test did not cover that ground either — and it usually does not: a skipped **integration** job is exactly the suite `npm test` excludes — record `build-and-test — was skipped in CI and its suite did not run locally` in `unreviewedDimensions`. That already caps a would-be Approve at `COMMENT`, through machinery that exists. + + This is the hole PR #6486 fell through. The one job that would have exercised the new hotkey, `Integration Tests (CLI, No Sandbox)`, was skipped; so were the macOS and Windows `Test` legs. The classifier called it `all_pass`, and the whole design leans on CI precisely because the LLM pipeline reads code statically (DESIGN.md, "Why downgrade APPROVE when CI is non-green"). The delegation returned nothing, and returned it looking like a pass. **The one case presubmit does decide for you: if checks exist and _not one_ of them ran, `class` is `no_checks` and a downgrade reason is already emitted — there is no green there to approve on.** + - For `stale` / `resolved` / `noConflict` buckets, log to terminal but do not block. **Why these checks block submission:** From 44d5763d6a8831b80eb1c03c82ed9a3a4ec2cd1b Mon Sep 17 00:00:00 2001 From: wenshao Date: Mon, 13 Jul 2026 12:44:32 +0800 Subject: [PATCH 04/12] =?UTF-8?q?feat(review):=20add=20a=20test-efficacy?= =?UTF-8?q?=20probe=20=E2=80=94=20does=20the=20new=20test=20gate=20the=20n?= =?UTF-8?q?ew=20code=3F?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Agent 5 asks whether a test EXISTS and whether its assertions look like they check something. Agent 7 runs the suite and reports that it is GREEN. Neither can see a test that protects nothing, and there are two ways to ship one: - unreachable — the project's test command never collects the file. - inert — it runs, it passes, and it would still pass with the change reverted. #6486 shipped both, in one file. The new test lived in integration-tests/, which is not an npm workspace, so `npm test --workspaces` never collected it; its CI job was skipped, so CI never ran it either. The test executed nowhere — not in CI, not in the review — and nothing in the pipeline noticed. Had it run it would have passed anyway: it drove a kitty CSI-u sequence into a PTY that never negotiated the kitty protocol, so the keypress was discarded before reaching the handler under test. It could only ever have caught a startup crash. Agent 5 saw a test file with plausible assertions and called coverage fine. Both questions are decidable without judgment, which is why they are a subcommand and not a prompt. Unreachability needs no execution at all — a path against the root package.json workspace globs. Inertness needs one run: revert the diff's source files to base, keep its tests, re-run them. The classifier is asymmetric on purpose. Reverting source frequently breaks a test's own compile — it imports a symbol the diff introduced — and the runner exits non-zero having collected nothing. Scoring that as "the test caught the revert" would hand back exactly the false assurance this command exists to remove. `gated` therefore requires a real ASSERTION failure; a bare non-zero exit with nothing collected is `inconclusive`, and `inconclusive` is never reported as a finding. Verdicts are per test FILE, not per run. One `vitest run` covers every probe, and a run-level verdict lets one honest test cover for a useless one: the gating test fails, the run reports failures, and the inert test beside it is scored `gated` too — so every inert test with a working sibling would be invisible, which is the exact defect this command exists to find. Found by running it against a real repo; the unit tests for the run-level classifier all passed. Two other limits are deliberate: a test-only diff is never probed (a new test for old code is SUPPOSED to pass with nothing reverted, and flagging it would be a false blocker on exactly the PRs we want people to write), and findings are Suggestions, not Criticals — a test that does not gate is not itself wrong code; what the finding must name is the behaviour now shipping unprotected. Driven against real PRs: #6433 reports GATED (9 assertions fail on revert, no finding); #6486 reports its integration test unreachable and its two unit tests gated. --- packages/cli/src/commands/review.test.ts | 1 + packages/cli/src/commands/review.ts | 4 +- .../src/commands/review/test-efficacy.test.ts | 247 ++++++++++ .../cli/src/commands/review/test-efficacy.ts | 447 ++++++++++++++++++ .../core/src/skills/bundled/review/DESIGN.md | 18 + .../core/src/skills/bundled/review/SKILL.md | 22 +- 6 files changed, 737 insertions(+), 2 deletions(-) create mode 100644 packages/cli/src/commands/review/test-efficacy.test.ts create mode 100644 packages/cli/src/commands/review/test-efficacy.ts diff --git a/packages/cli/src/commands/review.test.ts b/packages/cli/src/commands/review.test.ts index 53af8950d83..08a63fad742 100644 --- a/packages/cli/src/commands/review.test.ts +++ b/packages/cli/src/commands/review.test.ts @@ -45,6 +45,7 @@ describe('reviewCommand', () => { 'pr-context', 'load-rules', 'presubmit', + 'test-efficacy', 'compose-review', 'cleanup', ]); diff --git a/packages/cli/src/commands/review.ts b/packages/cli/src/commands/review.ts index 731f5de6794..9a6940b0d2c 100644 --- a/packages/cli/src/commands/review.ts +++ b/packages/cli/src/commands/review.ts @@ -16,6 +16,7 @@ import { planDiffCommand } from './review/plan-diff.js'; import { prContextCommand } from './review/pr-context.js'; import { loadRulesCommand } from './review/load-rules.js'; import { presubmitCommand } from './review/presubmit.js'; +import { testEfficacyCommand } from './review/test-efficacy.js'; import { cleanupCommand } from './review/cleanup.js'; export const reviewCommand: CommandModule = { @@ -30,11 +31,12 @@ export const reviewCommand: CommandModule = { .command(prContextCommand) .command(loadRulesCommand) .command(presubmitCommand) + .command(testEfficacyCommand) .command(composeReviewCommand) .command(cleanupCommand) .demandCommand( 1, - 'Specify a subcommand: parse-args, fetch-pr, plan-diff, pr-context, load-rules, presubmit, compose-review, or cleanup.', + 'Specify a subcommand: parse-args, fetch-pr, plan-diff, pr-context, load-rules, presubmit, test-efficacy, compose-review, or cleanup.', ) .version(false), handler: () => { diff --git a/packages/cli/src/commands/review/test-efficacy.test.ts b/packages/cli/src/commands/review/test-efficacy.test.ts new file mode 100644 index 00000000000..69a7fca3807 --- /dev/null +++ b/packages/cli/src/commands/review/test-efficacy.test.ts @@ -0,0 +1,247 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect } from 'vitest'; +import { + isWorkspaceMember, + planTestEfficacy, + classifyProbeRun, +} from './test-efficacy.js'; + +// The real root `package.json` workspace list. +const GLOBS = [ + 'packages/*', + 'packages/channels/base', + 'packages/channels/telegram', + '!packages/desktop', +]; + +describe('isWorkspaceMember', () => { + it('places the integration-tests directory outside every workspace', () => { + // The whole of the PR #6486 unreachability finding, decided without running + // anything: `npm test` is `npm run test --workspaces`, and this path is in + // no workspace, so nothing ever collects it. + expect( + isWorkspaceMember( + 'integration-tests/interactive/model-toggle-hotkey.test.ts', + GLOBS, + ), + ).toBe(false); + }); + + it('places a package test inside one', () => { + expect( + isWorkspaceMember('packages/cli/src/config/keyBindings.test.ts', GLOBS), + ).toBe(true); + expect( + isWorkspaceMember('packages/channels/base/src/x.test.ts', GLOBS), + ).toBe(true); + }); + + it('honours a negated glob', () => { + expect(isWorkspaceMember('packages/desktop/src/a.test.ts', GLOBS)).toBe( + false, + ); + }); + + it('honours workspace-glob ORDER — a positive after a negation re-includes', () => { + // npm evaluates the list in order. Filtering all negations first let a + // negation win wherever it sat, which would file a false `unreachable`. + const globs = ['packages/*', '!packages/desktop', 'packages/desktop']; + expect(isWorkspaceMember('packages/desktop/src/a.test.ts', globs)).toBe( + true, + ); + const reordered = ['packages/*', 'packages/desktop', '!packages/desktop']; + expect(isWorkspaceMember('packages/desktop/src/a.test.ts', reordered)).toBe( + false, + ); + }); + + it('does not match a sibling directory by prefix', () => { + expect(isWorkspaceMember('packages-old/cli/a.test.ts', GLOBS)).toBe(false); + expect(isWorkspaceMember('scripts/a.test.ts', GLOBS)).toBe(false); + }); +}); + +describe('planTestEfficacy', () => { + // PR #6486's real file list: one unreachable integration test, two reachable + // unit tests, and the production files they are supposed to be gating. + const files6486 = [ + { path: 'packages/cli/src/ui/AppContainer.tsx', kind: 'source' }, + { path: 'packages/cli/src/config/keyBindings.ts', kind: 'source' }, + { path: 'packages/cli/src/config/keyBindings.test.ts', kind: 'test' }, + { path: 'packages/cli/src/ui/keyMatchers.test.ts', kind: 'test' }, + { + path: 'integration-tests/interactive/model-toggle-hotkey.test.ts', + kind: 'test', + }, + ]; + + it('reports the unreachable test and probes only the ones that can run', () => { + const plan = planTestEfficacy(files6486, GLOBS); + expect(plan.unreachable).toEqual([ + 'integration-tests/interactive/model-toggle-hotkey.test.ts', + ]); + expect(plan.probes).toEqual([ + 'packages/cli/src/config/keyBindings.test.ts', + 'packages/cli/src/ui/keyMatchers.test.ts', + ]); + expect(plan.revert).toEqual([ + 'packages/cli/src/ui/AppContainer.tsx', + 'packages/cli/src/config/keyBindings.ts', + ]); + }); + + it('probes nothing on a test-only diff', () => { + // A new test for OLD code is supposed to pass with nothing reverted. Probing + // it would report every such PR as "inert" — a false blocker on exactly the + // PRs we want people to write. + const plan = planTestEfficacy( + [{ path: 'packages/cli/src/a.test.ts', kind: 'test' }], + GLOBS, + ); + expect(plan.probes).toEqual([]); + expect(plan.revert).toEqual([]); + }); +}); + +describe('classifyProbeRun', () => { + const json = (o: unknown) => JSON.stringify(o); + const only = (got: T[]): T => got[0]; + + it('calls a test that still passes without the change INERT', () => { + // The finding. The source is reverted and the test is green anyway, so it + // is green whether or not the feature exists. + const got = classifyProbeRun( + 0, + json({ + testResults: [ + { + name: '/w/packages/lib/src/inert.test.ts', + assertionResults: [{ status: 'passed' }, { status: 'passed' }], + }, + ], + }), + ['packages/lib/src/inert.test.ts'], + ); + expect(only(got).verdict).toBe('inert'); + expect(only(got).detail).toContain('does not gate'); + }); + + it('calls a real assertion failure GATED', () => { + const got = classifyProbeRun( + 1, + json({ + testResults: [ + { + name: '/w/a.test.ts', + assertionResults: [{ status: 'failed' }, { status: 'passed' }], + }, + ], + }), + ['a.test.ts'], + ); + expect(only(got).verdict).toBe('gated'); + }); + + it('does not let a gating test cover for an inert one in the same run', () => { + // The bug the LIVE run found and the unit tests did not. One `vitest run` + // covers every probe; a run-level verdict scored BOTH files `gated` because + // the gating test failed — so every inert test with a working sibling was + // invisible, which is the exact defect this command exists to find. + const got = classifyProbeRun( + 1, + json({ + testResults: [ + { + name: '/w/packages/lib/src/inert.test.ts', + assertionResults: [{ status: 'passed' }], + }, + { + name: '/w/packages/lib/src/gating.test.ts', + assertionResults: [{ status: 'failed' }], + }, + ], + }), + ['packages/lib/src/inert.test.ts', 'packages/lib/src/gating.test.ts'], + ); + expect(got.map((r) => [r.file, r.verdict])).toEqual([ + ['packages/lib/src/inert.test.ts', 'inert'], + ['packages/lib/src/gating.test.ts', 'gated'], + ]); + }); + + it('does NOT call a compile error GATED', () => { + // The trap this command would otherwise walk into. Reverting the source + // routinely breaks the test's own imports — it references a symbol the diff + // introduced. The runner exits non-zero and collects nothing. That is not + // the test catching a regression; mistaking it for one would hand back + // exactly the false assurance we are trying to remove. + const got = classifyProbeRun(1, json({ testResults: [] }), ['a.test.ts']); + expect(only(got).verdict).toBe('inconclusive'); + expect(only(got).detail).toContain('not evidence either way'); + }); + + it('is inconclusive on unparseable output, and says why', () => { + const got = only( + classifyProbeRun( + 1, + 'ELIFECYCLE npm ERR!', + ['a.test.ts'], + 'ENOENT: vitest', + ), + ); + expect(got.verdict).toBe('inconclusive'); + // The runner's own error is the only thing that explains this outcome; + // dropping stderr leaves an `inconclusive` nobody can act on. + expect(got.detail).toContain('ENOENT: vitest'); + }); + + it('does not take another file’s verdict by suffix collision', () => { + // `endsWith(file)` alone matches `/w/vendor/other-src/a.test.ts` for the + // probe `src/a.test.ts` — and would then report that file's verdict for + // ours, silently. Match on a path-separator boundary. + const got = only( + classifyProbeRun( + 1, + json({ + testResults: [ + { + name: '/w/vendor/other-src/a.test.ts', + assertionResults: [{ status: 'failed' }], + }, + ], + }), + ['src/a.test.ts'], + ), + ); + // Our file was never collected — that is `inconclusive`, not the neighbour's + // `gated`. + expect(got.verdict).toBe('inconclusive'); + }); + + it('does not call an all-skipped file INERT', () => { + // Nothing failed and nothing passed — every test was skipped. Reporting + // "all 0 test(s) still PASSED" about tests that never executed is the same + // false assurance in a different costume. + const got = only( + classifyProbeRun( + 0, + json({ + testResults: [ + { + name: '/w/a.test.ts', + assertionResults: [{ status: 'skipped' }, { status: 'skipped' }], + }, + ], + }), + ['a.test.ts'], + ), + ); + expect(got.verdict).toBe('inconclusive'); + expect(got.detail).toContain('none executed'); + }); +}); diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts new file mode 100644 index 00000000000..a0631abd53d --- /dev/null +++ b/packages/cli/src/commands/review/test-efficacy.ts @@ -0,0 +1,447 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// `qwen review test-efficacy`: does the diff's new test actually gate the +// diff's new behaviour? +// +// Agent 5 and the test-coverage matrix ask whether a test EXISTS and whether +// its assertions look like they check something. Neither question can catch the +// two ways a test ships without protecting anything: +// +// 1. UNREACHABLE — the project's test command never runs the file. On PR +// #6486 the new test lived in `integration-tests/`, which is not an npm +// workspace, so `npm test --workspaces` never collected it; and its CI job +// (`Integration Tests (CLI, No Sandbox)`) was skipped. The test executed +// nowhere, in CI or in review, and nothing noticed. +// 2. INERT — it runs, it passes, and it would still pass with the source +// change reverted. #6486's did: it drove a kitty CSI-u sequence into a PTY +// that never negotiated the kitty protocol, so the keypress was discarded +// before it could reach the handler under test. The test could only ever +// have caught a startup crash. +// +// Both are decidable without judgment, which is why they live here in TypeScript +// rather than in a review agent's prompt. Findings carry `Source: [test]` and +// are pre-confirmed like Agent 7's — they are the outcome of running commands, +// not of reading code. +// +// The revert probe is the load-bearing half, and its trap is the third outcome: +// reverting the source can make the test fail to COMPILE (it imports a symbol +// the new code introduced). That failure is not evidence the test gates +// anything, and calling it "gated" would be exactly the false assurance this +// command exists to remove. So `gated` requires a real assertion failure, and +// everything else that is not a clean pass is `inconclusive`. + +import type { CommandModule } from 'yargs'; +import { spawnSync } from 'node:child_process'; +import { mkdirSync, writeFileSync, readFileSync, rmSync } from 'node:fs'; +import { dirname, join } from 'node:path'; +import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; + +export type ProbeVerdict = 'gated' | 'inert' | 'inconclusive'; + +export interface FileEntry { + path: string; + kind: string; +} + +/** + * Does `npm test --workspaces` reach this file? + * + * A test outside every workspace glob is collected by nothing. This is the + * whole of the #6486 unreachability finding, and it needs no execution at all — + * just the root `package.json`. + * + * Globs here are npm workspace globs, not full minimatch: a trailing `/*` means + * "one path segment", a leading `!` excludes. Anything fancier is treated as a + * literal prefix, which errs toward calling a file REACHABLE — the safe + * direction, since a false "unreachable" finding would be posted to a PR. + */ +export function isWorkspaceMember( + filePath: string, + workspaceGlobs: string[], +): boolean { + const norm = filePath.replace(/^\.\//, ''); + const matches = (glob: string): boolean => { + const g = glob.replace(/^!/, '').replace(/\/$/, ''); + if (g.endsWith('/*')) { + const base = g.slice(0, -2); + if (!norm.startsWith(`${base}/`)) return false; + // `packages/*` owns `packages/cli/**`, not `packages/channels/base/**` + // — but the latter is listed explicitly, and negation is handled below. + return true; + } + return norm === g || norm.startsWith(`${g}/`); + }; + // npm evaluates the globs IN ORDER, so a positive glob listed after a negation + // re-includes what the negation excluded. Two passes (all negations, then all + // positives) let a negation win regardless of where it sits, which would file + // a false `unreachable` — a finding posted to someone's PR. + let member = false; + for (const g of workspaceGlobs) { + if (!matches(g)) continue; + member = !g.startsWith('!'); + } + return member; +} + +export interface EfficacyPlan { + /** Test files the diff adds or changes that the test command never collects. */ + unreachable: string[]; + /** Test files worth probing — they are reachable, so they can be run. */ + probes: string[]; + /** Production files to revert to base for the probe. */ + revert: string[]; +} + +/** + * Split the diff into what to report and what to run. + * + * A diff with no source changes has nothing to gate, so it gets no probe: a + * test-only PR (a new test for old code) must not be told its tests are inert. + */ +export function planTestEfficacy( + files: FileEntry[], + workspaceGlobs: string[], +): EfficacyPlan { + const tests = files.filter((f) => f.kind === 'test').map((f) => f.path); + const revert = files.filter((f) => f.kind === 'source').map((f) => f.path); + const unreachable = tests.filter( + (t) => !isWorkspaceMember(t, workspaceGlobs), + ); + const reachable = tests.filter((t) => isWorkspaceMember(t, workspaceGlobs)); + return { + unreachable, + probes: revert.length > 0 ? reachable : [], + revert, + }; +} + +interface VitestAssertion { + status?: string; +} +interface VitestFileResult { + /** Absolute path of the test file this result belongs to. */ + name?: string; + assertionResults?: VitestAssertion[]; +} +interface VitestJson { + numPassedTests?: number; + numFailedTests?: number; + testResults?: VitestFileResult[]; +} + +/** + * Rule on the revert probe, **per test file**. + * + * Per-file, not per-run, and that distinction is load-bearing. One `vitest run` + * covers every probe at once, but a run-level verdict lets one honest test cover + * for a useless one: the gating test fails, the run reports failures, and the + * inert test sitting beside it is scored `gated` too. Every inert test with a + * working sibling would be invisible — which is the exact defect this command + * exists to find. (Found by running it, not by unit-testing it. The unit tests + * for the run-level classifier all passed.) `testResults[].name` carries the + * file, so the mapping is available; use it. + * + * The three-way asymmetry is deliberate: + * + * - `inert` — this file's tests PASSED with the source change reverted. They do + * not gate the change. This is a finding. + * - `gated` — at least one ASSERTION in this file failed. It caught the revert; + * it is doing its job. Requires a real assertion failure, never a bare + * non-zero exit: reverting source routinely breaks a test's own compile (it + * imports a symbol the diff introduced), and a compile error proves nothing + * about whether the test would catch a behavioural regression. + * - `inconclusive` — everything else: the file collected nothing, an + * import/type error, unparseable output. Do NOT let this read as `gated`; a + * review that mistakes "it errored" for "it caught the bug" is back where it + * started. + */ +export function classifyProbeRun( + exitCode: number, + stdout: string, + probes: string[], + stderr = '', +): Array<{ file: string; verdict: ProbeVerdict; detail: string }> { + let parsed: VitestJson | undefined; + const start = stdout.indexOf('{'); + if (start >= 0) { + try { + parsed = JSON.parse(stdout.slice(start)) as VitestJson; + } catch { + parsed = undefined; + } + } + if (!parsed) { + // The runner's own error is the only thing that explains this, and dropping + // it leaves an `inconclusive` nobody can act on. + const why = stderr.trim().split('\n').slice(-3).join(' ').slice(0, 300); + return probes.map((file) => ({ + file, + verdict: 'inconclusive' as const, + detail: `runner produced no parseable JSON (exit ${exitCode})${why ? `: ${why}` : ''}`, + })); + } + + const byFile = parsed.testResults ?? []; + return probes.map((file) => { + // `testResults[].name` is absolute; the probe path is repo-relative. Match + // on a path-separator boundary, so `src/a.test.ts` cannot be satisfied by + // `/w/vendor/other-src/a.test.ts` — a bare `endsWith` would take the wrong + // file's verdict and never say so. + const result = byFile.find( + (r) => (r.name ?? '').endsWith(`/${file}`) || r.name === file, + ); + const assertions = result?.assertionResults ?? []; + const failed = assertions.filter((a) => a.status === 'failed').length; + const passed = assertions.filter((a) => a.status === 'passed').length; + + if (!result || assertions.length === 0) { + return { + file, + verdict: 'inconclusive' as const, + detail: `collected no tests with the source reverted (run exit ${exitCode}) — likely a compile or import error, which is not evidence either way`, + }; + } + if (failed > 0) { + return { + file, + verdict: 'gated' as const, + detail: `${failed} assertion(s) failed with the source reverted — this test catches the change`, + }; + } + if (passed === 0) { + // Collected, but nothing failed AND nothing passed — every test skipped + // (`it.skip`, an unmet `describe.runIf`). A file that ran no assertions + // proves nothing; calling that `inert` would report "still passed" about + // tests that never executed. + return { + file, + verdict: 'inconclusive' as const, + detail: `${assertions.length} test(s) collected but none executed with the source reverted (all skipped) — not evidence either way`, + }; + } + return { + file, + verdict: 'inert' as const, + detail: `all ${passed} test(s) still PASSED with the source change reverted — this test does not gate the change`, + }; + }); +} + +interface TestEfficacyArgs { + report: string; + worktree: string; + base: string; + out: string; +} + +function git(cwd: string, ...args: string[]): void { + const r = spawnSync('git', args, { cwd, encoding: 'utf8' }); + // `git` not on PATH leaves `status` null and `stderr` undefined, which the + // status check below would report as `failed: ` — an error message with no + // error in it. The runner spawn already guards this; so does this one now. + if (r.error) throw r.error; + if (r.status !== 0) { + throw new Error(`git ${args.join(' ')} failed: ${r.stderr ?? ''}`); + } +} + +/** Does this path exist at the base commit? */ +function existsAtRev(cwd: string, rev: string, path: string): boolean { + return ( + spawnSync('git', ['cat-file', '-e', `${rev}:${path}`], { cwd }).status === 0 + ); +} +const existsAtBase = (cwd: string, base: string, path: string) => + existsAtRev(cwd, base, path); +/** A file the PR DELETED does not exist at HEAD — restoring it means removing it. */ +const existsAtHead = (cwd: string, path: string) => + existsAtRev(cwd, 'HEAD', path); + +async function runTestEfficacy(args: TestEfficacyArgs): Promise { + const { report, worktree, base, out } = args; + const plan = JSON.parse(readFileSync(report, 'utf8')) as { + files?: FileEntry[]; + }; + const rootPkg = JSON.parse( + readFileSync(`${worktree}/package.json`, 'utf8'), + ) as { workspaces?: string[] }; + const globs = rootPkg.workspaces ?? []; + + const { unreachable, probes, revert } = planTestEfficacy( + plan.files ?? [], + globs, + ); + + const results: Array<{ + file: string; + verdict: ProbeVerdict; + detail: string; + }> = []; + let restoreFailure: string | undefined; + + if (probes.length > 0 && revert.length > 0) { + // "Revert to base" is two operations, not one. A file the PR MODIFIED is + // checked out from base; a file the PR ADDED did not exist at base, and + // `git checkout -- ` does not quietly skip it — it fails + // with `pathspec ... did not match any file(s) known to git`. That throw + // used to escape past `writeFileSync` and discard the whole report, + // `unreachable` findings included, on every PR that adds a source file. + // Which is most of them. + const modified: string[] = []; + const added: string[] = []; + for (const p of revert) { + (existsAtBase(worktree, base, p) ? modified : added).push(p); + } + try { + if (modified.length > 0) { + git(worktree, 'checkout', base, '--', ...modified); + } + // An added file's base state is "absent". Removing it is the honest + // revert; the probe usually then fails to compile, which is + // `inconclusive` — not a verdict, but an honest one. + for (const p of added) rmSync(join(worktree, p), { force: true }); + + const r = spawnSync( + 'npx', + ['vitest', 'run', '--reporter=json', ...probes], + { cwd: worktree, encoding: 'utf8', timeout: 300_000 }, + ); + // `r.error` is set — and `r.status` is null — when the process never ran + // (npx missing) or was killed (the timeout above fires SIGTERM). Ignoring + // it reports those as "the runner produced no parseable JSON", which + // blames the runner's output for a run that produced none. + if (r.error) throw r.error; + if (r.signal) { + throw new Error( + `runner killed by ${r.signal}${r.signal === 'SIGTERM' ? ' (probe timed out after 300s)' : ''}`, + ); + } + results.push( + ...classifyProbeRun( + r.status ?? 1, + `${r.stdout ?? ''}`, + probes, + `${r.stderr ?? ''}`, + ), + ); + } catch (e) { + // The probe could not be set up or run. That is not evidence about any + // test — record it and keep going, so the report (and the unreachable + // findings, which needed no probe at all) still reaches the caller. + const detail = `probe could not run: ${e instanceof Error ? e.message : String(e)}`; + results.push( + ...probes.map((file) => ({ + file, + verdict: 'inconclusive' as const, + detail, + })), + ); + } finally { + // Always put the worktree back — the review's later steps read this tree + // and must see the PR's code, not the base's. This restores deleted files + // too. A restore failure must not mask the probe's own outcome (hence the + // catch), but it must not be swallowed either: the tree is now sitting on + // BASE code, and every agent that reads it afterwards reviews the wrong + // source. That is the loudest thing this command can have to say. + // + // Restore is also two operations, for the mirror-image reason the revert + // was. A file the PR DELETED does not exist at HEAD either, and + // `git checkout HEAD -- ` fails on the bad pathspec and + // restores NOTHING — so one deleted source file used to leave the whole + // revert set sitting on base code, plus a resurrected copy of the file the + // PR removed. Delete what HEAD does not have; check out what it does. + const atHead: string[] = []; + const notAtHead: string[] = []; + for (const p of revert) { + (existsAtHead(worktree, p) ? atHead : notAtHead).push(p); + } + try { + if (atHead.length > 0) { + git(worktree, 'checkout', 'HEAD', '--', ...atHead); + } + if (notAtHead.length > 0) { + // `git checkout -- ` writes the INDEX as well as the + // working tree, so removing the file leaves a staged phantom add + // behind (`AD` in `git status`). Reset those index entries to HEAD — + // where the path does not exist, which is exactly the state we want. + for (const p of notAtHead) rmSync(join(worktree, p), { force: true }); + git(worktree, 'reset', '-q', 'HEAD', '--', ...notAtHead); + } + } catch (e) { + restoreFailure = `WORKTREE NOT RESTORED — it is still on base code for: ${revert.join(', ')}. Every later step of this review reads the wrong source. Run \`git checkout HEAD -- ${atHead.join(' ')}\` in ${worktree} before continuing. (${e instanceof Error ? e.message : String(e)})`; + } + } + } + + const findings = [ + ...unreachable.map((f) => ({ + file: f, + kind: 'unreachable' as const, + message: `\`${f}\` is outside every npm workspace, so the project's test command never collects it. It did not run in this review, and it does not gate this change. Confirm it runs in CI — and check \`ciStatus.skippedCheckNames\`, because the job that would run it is exactly the kind that gets skipped.`, + })), + ...results + .filter((r) => r.verdict === 'inert') + .map((r) => ({ + file: r.file, + kind: 'inert' as const, + message: `\`${r.file}\`: ${r.detail}. It passes whether or not the change is present, so it cannot catch a regression in it.`, + })), + ]; + + const result = { + unreachable, + probed: results, + inconclusive: results.filter((r) => r.verdict === 'inconclusive'), + findings, + restoreFailure, + }; + mkdirSync(dirname(out), { recursive: true }); + writeFileSync(out, JSON.stringify(result, null, 2), 'utf8'); + writeStdoutLine( + `Wrote test-efficacy report to ${out} (${unreachable.length} unreachable, ${results.length} probed, ${findings.length} finding(s))`, + ); + for (const f of findings) { + writeStdoutLine(` [test] ${f.kind}: ${f.file}`); + } + if (restoreFailure) { + // Loud, and on stderr: every subsequent step of the review is now reading + // base code out of this worktree, and a line buried in a JSON field is not + // going to stop it. + writeStderrLine(`ERROR: ${restoreFailure}`); + } +} + +export const testEfficacyCommand: CommandModule = { + command: 'test-efficacy ', + describe: + "Check whether the diff's new tests actually gate its new behaviour (unreachable + revert probe)", + builder: (yargs) => + yargs + .positional('report', { + type: 'string', + demandOption: true, + describe: 'Path to the fetch-pr / plan-diff report JSON', + }) + .option('worktree', { + type: 'string', + demandOption: true, + describe: 'Worktree to probe in', + }) + .option('base', { + type: 'string', + demandOption: true, + describe: 'Base SHA to revert source files to', + }) + .option('out', { + type: 'string', + demandOption: true, + describe: 'Output JSON path', + }), + handler: async (argv) => { + await runTestEfficacy(argv as unknown as TestEfficacyArgs); + }, +}; diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index e09abcce977..f7f9b77e53f 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -339,6 +339,24 @@ Three changes, and the ordering one is load-bearing: The lesson generalizes past this file: **"a false positive is cheap" is a claim about a budget, and it has to be measured against the real distribution, not assumed.** Here it was false until the ordering was fixed. +## Why a test-efficacy probe, when there is already a Test Coverage agent + +Agent 5 asks whether a test **exists** and whether its assertions **look like** they check something. Agent 7 runs the suite and reports that it is **green**. Neither can see a test that protects nothing, and there are two ways to ship one: + +- **Unreachable** — the project's test command never collects the file. +- **Inert** — it runs, it passes, and it would still pass with the change reverted. + +PR #6486 shipped both, in one file. The new test lived in `integration-tests/`, which is not an npm workspace, so `npm test --workspaces` never collected it; its CI job (`Integration Tests (CLI, No Sandbox)`) was skipped, so CI never ran it either. **The test executed nowhere — not in CI, not in the review — and nothing in the pipeline noticed.** And had it run, it would have passed regardless: it drove a kitty CSI-u sequence into a PTY that never negotiated the kitty protocol, so the keypress was discarded before reaching the handler under test. It could only ever have caught a startup crash. Agent 5 saw a test file with plausible assertions and said coverage was fine. + +Both questions are decidable without judgment, which is why they are a subcommand and not a prompt. Unreachability needs no execution at all — it is a path against the root `package.json` workspace globs. Inertness needs one run: revert the diff's **source** files to base, keep its **tests**, re-run them. A test that is still green is green whether or not the feature exists. + +**The trap, and the reason the classifier is asymmetric.** Reverting source frequently breaks the test's own compile — it imports a symbol the diff introduced — and the runner exits non-zero having collected nothing. It is tempting to score that as "the test caught the revert". It is not: a compile error says nothing about whether the test would catch a _behavioural_ regression, and scoring it as `gated` would hand back precisely the false assurance this command exists to remove. So `gated` requires a real **assertion** failure; a bare non-zero exit with nothing collected is `inconclusive`, and `inconclusive` is never reported as a finding. + +Two other deliberate limits: + +- **A test-only diff is never probed.** A new test for old code is _supposed_ to pass with nothing reverted. Probing it would flag every such PR as inert — a false blocker on exactly the PRs we want people to write. +- **Findings are Suggestions, not Criticals.** A test that does not gate is not itself wrong code; nothing is broken today. What the finding must say concretely is which behaviour is now shipping unprotected. + ## Why "fixed by this diff" is the verdict that needed a bar The re-check has three verdicts, and until PR #6486 only two of them cost anything: diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 8362332e65f..b63af4f10c7 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -535,7 +535,27 @@ This agent runs deterministic build and test commands to verify the code compile - **Environment/setup failures** (missing dependencies, tool not installed, virtualenv not activated) → report as informational note, not Critical 5. Output format: same as other agents, but the **Source** field MUST be `[build]` for build failures or `[test]` for test failures (not `[review]`). -**Note**: Build/test results are deterministic facts. Code-caused failures skip Step 4 verification — the `[build]`/`[test]` source tag is how they are recognized as pre-confirmed. Environment/setup failures are informational only and should not affect the verdict. +6. **Run the test-efficacy probe** (same-repo PR reviews, high effort — it needs the worktree and the base SHA). A green suite says the tests pass. It does not say the tests would have failed had the change been wrong, and those are different claims: + + ```bash + qwen review test-efficacy .qwen/tmp/qwen-review-pr--fetch.json \ + --worktree \ + --base \ + --out .qwen/tmp/qwen-review-pr--efficacy.json + ``` + + `` is the base the fetch report resolved. **If it is null** (merge-base unresolvable — the same state that leaves `diffPath` null), skip this probe entirely and say so: there is no base to revert to, and a probe against the wrong base would report every gating test as inert. + + It reverts the diff's **source** files to base, keeps its **tests**, re-runs them, and reports two things no reading of the code can establish: + `findings[]` carries **both** kinds — read it, not the individual arrays: + - **`kind: 'unreachable'`** — a test file the project's test command never collects (outside every npm workspace). It did not run here and it does not run in `npm test`. Cross-check it against `ciStatus.skippedCheckNames` from Step 7's presubmit: a test that runs in neither place gates nothing, anywhere. + - **`kind: 'inert'`** — the test **still passed with the change reverted**. It is green whether or not the feature exists, so it cannot catch a regression in it. + + Report each entry in `findings` as a **Suggestion** with `Source: [test]` (a test that does not gate is not itself broken code — but say plainly, in the failure scenario, which behaviour ships unprotected). Both were true of PR #6486 at once: the new test lived in `integration-tests/` (collected by nothing), its CI job was skipped, and it drove a kitty CSI-u sequence into a PTY that never negotiated the protocol — so the keypress was discarded and the test could only ever have caught a startup crash. It shipped as coverage for a feature it never touched. + + **`inconclusive` is not a finding and must never be reported as one.** Reverting the source often breaks the test's own compile — it imports a symbol the diff introduced — and the runner then errors out having collected nothing. That is not the test catching a regression; the subcommand refuses to call it `gated` for exactly that reason, and you must not either. Note it in the terminal and move on. + +**Note**: Build/test results are deterministic facts. Code-caused failures skip Step 4 verification — the `[build]`/`[test]` source tag is how they are recognized as pre-confirmed. Environment/setup failures are informational only and should not affect the verdict. Test-efficacy findings are deterministic in the same way and are likewise pre-confirmed. ### Agent 8: Diff-specialized finders (0–2 agents, optional; high effort only) From 7a2b48bc918c00aa5c05207b7f983cb954836755 Mon Sep 17 00:00:00 2001 From: wenshao Date: Mon, 13 Jul 2026 15:21:49 +0800 Subject: [PATCH 05/12] fix(review): harden CI classification, path safety, and probe robustness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five correctness fixes surfaced by a Codex $qreview pass on this PR. Each was verified against the real code before applying; the review filed 30 Criticals, of which these are the ones that actually reproduce a wrong result. - presubmit: paginate `check-runs`. The single-page `ghApi` call saw only the first 30 runs — this PR's own head has 508 — so a failing or skipped job past the cut was invisible and could let a review approve past it. New `ghApiAllNested` streams `--paginate --jq '.check_runs[]'` as NDJSON (gh has no `--slurp`; the parse is split into a pure `parseNdjson` for testing). - presubmit: treat `startup_failure` as a failure. It was absent from `FAIL_CONCLUSIONS`, so a workflow that could not start counted as an executed run that added no failed name — an `all_pass` on a commit whose CI never ran. - presubmit: `waiting` and `requested` are active check-run statuses; add them to the pending set so a commit whose only check is waiting is not mislabeled `no_checks`. - pr-context: `extractCodeRefs` rendered path tokens from an untrusted comment body into the trusted "read each at the reviewed commit" directive. A blocker citing `../../../../etc/passwd.sh` or `/root/.ssh/id_rsa.key` entered the read list. Drop any absolute, `~`, or `..`-segment path; a real in-repo reference is repository-relative. - test-efficacy: raise the probe's `spawnSync` maxBuffer to 64 MiB (the ceiling the gh wrapper already uses). Vitest's JSON reporter on a large suite exceeds the 1 MiB default, returns ENOBUFS, and turns every probe `inconclusive`. --- .../cli/src/commands/review/lib/gh.test.ts | 22 ++++++- packages/cli/src/commands/review/lib/gh.ts | 32 ++++++++++ .../src/commands/review/pr-context.test.ts | 18 +++++- .../cli/src/commands/review/pr-context.ts | 16 ++++- .../cli/src/commands/review/presubmit.test.ts | 61 +++++++++++-------- packages/cli/src/commands/review/presubmit.ts | 25 ++++++-- .../cli/src/commands/review/test-efficacy.ts | 10 ++- 7 files changed, 148 insertions(+), 36 deletions(-) diff --git a/packages/cli/src/commands/review/lib/gh.test.ts b/packages/cli/src/commands/review/lib/gh.test.ts index 6d9ee575ad1..46641ddee8d 100644 --- a/packages/cli/src/commands/review/lib/gh.test.ts +++ b/packages/cli/src/commands/review/lib/gh.test.ts @@ -5,7 +5,7 @@ */ import { describe, it, expect, afterEach } from 'vitest'; -import { ghEnv, setGhHost } from './gh.js'; +import { ghEnv, setGhHost, parseNdjson } from './gh.js'; // Host targeting is code, not prose: the subcommands thread `--host` here, // and every gh child gets GH_HOST from ghEnv(). These tests pin the pure @@ -42,3 +42,23 @@ describe('setGhHost / ghEnv', () => { expect(() => setGhHost('https://ghe.internal')).toThrow(/--host/); }); }); + +describe('parseNdjson (the paginated check-runs decode)', () => { + it('parses one JSON value per non-blank line', () => { + // `gh api --paginate --jq '.check_runs[]'` applies the jq per page + // and emits each element on its own line (NDJSON) — NOT one array, and NOT + // the raw `{check_runs:[…]}{check_runs:[…]}` that a plain `--paginate` would + // concatenate and make `JSON.parse` throw on. (`gh api` has no `--slurp`; + // one real head had 508 check runs, so the first-page-only read missed 478.) + expect(parseNdjson('{"name":"a"}\n{"name":"b"}\n{"name":"c"}')).toEqual([ + { name: 'a' }, + { name: 'b' }, + { name: 'c' }, + ]); + }); + + it('returns [] for an empty response and ignores blank lines', () => { + expect(parseNdjson('')).toEqual([]); + expect(parseNdjson('{"name":"a"}\n\n')).toEqual([{ name: 'a' }]); + }); +}); diff --git a/packages/cli/src/commands/review/lib/gh.ts b/packages/cli/src/commands/review/lib/gh.ts index c6282ee4290..5328ea169e0 100644 --- a/packages/cli/src/commands/review/lib/gh.ts +++ b/packages/cli/src/commands/review/lib/gh.ts @@ -97,6 +97,38 @@ export function ghApiAll(path: string): unknown[] { return Array.isArray(parsed) ? parsed : []; } +/** + * Paginate an endpoint whose array is nested under a key, e.g. + * `check-runs` → `{ total_count, check_runs: [...] }`. + * + * A plain `ghApiAll` cannot be used here: `--paginate` alone concatenates the + * raw per-page objects, so `JSON.parse` sees `}{ ` between pages and throws. On + * a commit with more than 30 check runs (a busy CI matrix — one real head had + * 508) the un-paginated call silently saw only the first page, which could hide + * a failing or skipped run behind the cut and let a review approve past it. + * + * `--paginate --jq '.[]'` applies the jq to every page and streams each + * element as a newline-delimited JSON value (NDJSON), so the result is parsed + * line by line rather than as one array. (`gh api` has no `--slurp`.) + */ +export function ghApiAllNested(path: string, key: string): unknown[] { + return parseNdjson(gh('api', '--paginate', path, '--jq', `.${key}[]`)); +} + +/** + * Parse the newline-delimited JSON that `gh --paginate --jq '.x[]'` streams: + * one JSON value per non-blank line. Split out and exported so the parse is + * unit-testable without spawning `gh` (the spawn is covered by the commands' + * own runs, per this module's testing note above). + */ +export function parseNdjson(out: string): unknown[] { + if (!out) return []; + return out + .split('\n') + .filter((line) => line.trim().length > 0) + .map((line) => JSON.parse(line)); +} + /** Login of the currently authenticated GitHub user. */ export function currentUser(): string { return gh('api', 'user', '--jq', '.login'); diff --git a/packages/cli/src/commands/review/pr-context.test.ts b/packages/cli/src/commands/review/pr-context.test.ts index bd99276bd7e..e78986fd694 100644 --- a/packages/cli/src/commands/review/pr-context.test.ts +++ b/packages/cli/src/commands/review/pr-context.test.ts @@ -554,13 +554,25 @@ describe('extractCodeRefs', () => { ]); }); - it('keeps a scoped or relative path prefix intact', () => { + it('drops paths that escape the worktree — the read list is a trusted directive', () => { + // The body is untrusted and this list is rendered as "read each at the + // reviewed commit". A traversal or absolute token must not enter it. + expect( + extractCodeRefs('read `../../../../etc/passwd.sh` and `src/ok.ts:5`'), + ).toEqual(['src/ok.ts:5']); + expect(extractCodeRefs('see `/root/.ssh/id_rsa.key`')).toEqual([]); + expect(extractCodeRefs('see `~/secrets.json`')).toEqual([]); + }); + + it('keeps a scoped in-repo path prefix intact', () => { // `\b` fires on the first word-character transition, so `@scope/…` came back - // as `scope/…` and `../lib/b.ts` as `lib/b.ts` — not the path that was cited. + // as `scope/…` — not the path that was cited. A scoped package path stays in + // the repo, so it is kept; a `../` path escapes it and is dropped by the + // traversal filter above. expect(extractCodeRefs('see @scope/pkg/index.ts:10')).toEqual([ '@scope/pkg/index.ts:10', ]); - expect(extractCodeRefs('see ../lib/b.ts')).toEqual(['../lib/b.ts']); + expect(extractCodeRefs('see ../lib/b.ts')).toEqual([]); }); it('returns nothing for a body that names no code', () => { diff --git a/packages/cli/src/commands/review/pr-context.ts b/packages/cli/src/commands/review/pr-context.ts index 8c935495239..6a4ad8e4ade 100644 --- a/packages/cli/src/commands/review/pr-context.ts +++ b/packages/cli/src/commands/review/pr-context.ts @@ -199,7 +199,21 @@ const MAX_CODE_REFS = 12; export function extractCodeRefs(body: string | undefined): string[] { const all = [ ...new Set([...(body ?? '').matchAll(CODE_REF_RE)].map((m) => m[0])), - ]; + ] + // The body is untrusted, and this list is rendered as a trusted "read each + // at the reviewed commit" directive. A path that escapes the worktree — + // absolute, or containing a `..` segment — must never enter it: a blocker + // citing `../../../../etc/passwd.sh` or `/root/.ssh/id_rsa.key` would + // otherwise land on the read list. Drop them; a real in-repo reference is + // repository-relative. + .filter((r) => { + const path = r.split(':')[0]; + return ( + !path.startsWith('/') && + !path.startsWith('~') && + !path.split('/').includes('..') + ); + }); // A report routinely names the same location twice — once bare and once by // full path (`text-buffer.ts:2663` and `packages/.../text-buffer.ts:2663`). // Keep the fuller path: it is the one the reader can open. diff --git a/packages/cli/src/commands/review/presubmit.test.ts b/packages/cli/src/commands/review/presubmit.test.ts index 393bd4bccc6..cbfbf2716a7 100644 --- a/packages/cli/src/commands/review/presubmit.test.ts +++ b/packages/cli/src/commands/review/presubmit.test.ts @@ -115,6 +115,18 @@ describe('classifyCi — a skipped check is not a passing check', () => { expect(got.class).toBe('no_checks'); }); + it('treats startup_failure as a failure, not a silent pass', () => { + // A workflow that could not start is `completed` with `startup_failure`. It + // used to count as an execution that added no failed name — an all_pass on + // a commit whose CI never ran. + const got = classifyCi( + [run('Test', 'success'), run('E2E', 'startup_failure')], + [], + ); + expect(got.class).toBe('any_failure'); + expect(got.failedCheckNames).toContain('E2E'); + }); + it('a repo with no CI at all is still no_checks, with nothing to disclose', () => { const got = classifyCi([], []); expect(got.class).toBe('no_checks'); @@ -127,6 +139,7 @@ const { ghMock, ghApiMock, ghApiAllMock, + ghApiAllNestedMock, currentUserMock, ensureAuthenticatedMock, setGhHostMock, @@ -137,6 +150,7 @@ const { ghMock: vi.fn(), ghApiMock: vi.fn(), ghApiAllMock: vi.fn(), + ghApiAllNestedMock: vi.fn(), currentUserMock: vi.fn(), ensureAuthenticatedMock: vi.fn(), setGhHostMock: vi.fn(), @@ -149,6 +163,7 @@ vi.mock('./lib/gh.js', () => ({ gh: ghMock, ghApi: ghApiMock, ghApiAll: ghApiAllMock, + ghApiAllNested: ghApiAllNestedMock, currentUser: currentUserMock, ensureAuthenticated: ensureAuthenticatedMock, setGhHost: setGhHostMock, @@ -186,6 +201,7 @@ describe('presubmitCommand', () => { currentUserMock.mockReturnValue('qwen-code-ci-bot'); ghMock.mockReturnValue('contributor'); ghApiAllMock.mockReturnValue([]); + ghApiAllNestedMock.mockReturnValue([]); readFileSyncMock.mockReturnValue('[]'); process.env['GITHUB_RUN_ID'] = '28788268483'; }); @@ -204,15 +220,11 @@ describe('presubmitCommand', () => { // — the boolean compose-review actually acts on — did not. The disclosure was // written and the downgrade never fired. A reason nobody reads is not a gate, // so the assertion is on the boolean, through the real command. + ghApiAllNestedMock.mockReturnValue([ + { name: 'Test', status: 'completed', conclusion: 'skipped' }, + { name: 'Lint', status: 'completed', conclusion: 'skipped' }, + ]); ghApiMock.mockImplementation((path: string) => { - if (path.endsWith('/check-runs')) { - return { - check_runs: [ - { name: 'Test', status: 'completed', conclusion: 'skipped' }, - { name: 'Lint', status: 'completed', conclusion: 'skipped' }, - ], - }; - } if (path.endsWith('/status')) return { statuses: [] }; return null; }); @@ -232,25 +244,21 @@ describe('presubmitCommand', () => { }); it('ignores the running Qwen PR review check when deciding whether CI is still pending', async () => { + ghApiAllNestedMock.mockReturnValue([ + { + name: 'Test (ubuntu-latest, Node 22.x)', + status: 'completed', + conclusion: 'success', + }, + { + name: 'review-pr', + status: 'in_progress', + conclusion: null, + details_url: + 'https://github.com/QwenLM/qwen-code/actions/runs/28788268483/job/85362025778', + }, + ]); ghApiMock.mockImplementation((path: string) => { - if (path.endsWith('/check-runs')) { - return { - check_runs: [ - { - name: 'Test (ubuntu-latest, Node 22.x)', - status: 'completed', - conclusion: 'success', - }, - { - name: 'review-pr', - status: 'in_progress', - conclusion: null, - details_url: - 'https://github.com/QwenLM/qwen-code/actions/runs/28788268483/job/85362025778', - }, - ], - }; - } if (path.endsWith('/status')) { return { statuses: [] }; } @@ -275,6 +283,7 @@ describe('presubmitCommand', () => { it('threads --host to the gh layer before any call (GitHub Enterprise routing is code, not prose)', async () => { ghApiMock.mockReturnValue(null); ghApiAllMock.mockReturnValue([]); + ghApiAllNestedMock.mockReturnValue([]); currentUserMock.mockReturnValue('someone'); ghMock.mockReturnValue('{}'); diff --git a/packages/cli/src/commands/review/presubmit.ts b/packages/cli/src/commands/review/presubmit.ts index 5c17776bcf5..4941814e146 100644 --- a/packages/cli/src/commands/review/presubmit.ts +++ b/packages/cli/src/commands/review/presubmit.ts @@ -17,6 +17,7 @@ import { gh, ghApi, ghApiAll, + ghApiAllNested, currentUser, ensureAuthenticated, setGhHost, @@ -62,9 +63,22 @@ const FAIL_CONCLUSIONS = new Set([ 'cancelled', 'timed_out', 'action_required', + // GitHub reports a workflow that could not start as `startup_failure`. It is + // a failure, and leaving it out let it count as an execution that added no + // failed name — an all_pass on a commit whose CI never ran. + 'startup_failure', ]); const FAIL_STATUS_STATES = new Set(['failure', 'error']); -const PENDING_STATES = new Set(['queued', 'in_progress', 'pending']); +// GitHub check-run statuses that mean "still going". `waiting` and `requested` +// are real active states — omitting them mislabels a commit whose only check is +// waiting as `no_checks` with a spurious "every check was skipped" reason. +const PENDING_STATES = new Set([ + 'queued', + 'in_progress', + 'pending', + 'waiting', + 'requested', +]); /** * Conclusions that mean the job did not execute. GitHub reports these with @@ -241,10 +255,13 @@ async function runPresubmit(args: PresubmitArgs): Promise { const isSelfPr = author.toLowerCase() === me.toLowerCase(); // --- CI status --------------------------------------------------------- - const checkRunsResp = ghApi( + // Paginate: a busy CI matrix produces more than 30 check runs on one commit, + // and the first-page-only call could hide a failing or skipped job behind the + // cut, letting the review approve past it. + const checkRuns = ghApiAllNested( `repos/${owner}/${repo}/commits/${commitSha}/check-runs`, - ) as { check_runs?: CheckRun[] } | null; - const checkRuns = checkRunsResp?.check_runs ?? []; + 'check_runs', + ) as CheckRun[]; const statusResp = ghApi( `repos/${owner}/${repo}/commits/${commitSha}/status`, ) as { statuses?: CommitStatus[] } | null; diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts index a0631abd53d..e45daa8380e 100644 --- a/packages/cli/src/commands/review/test-efficacy.ts +++ b/packages/cli/src/commands/review/test-efficacy.ts @@ -308,7 +308,15 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { const r = spawnSync( 'npx', ['vitest', 'run', '--reporter=json', ...probes], - { cwd: worktree, encoding: 'utf8', timeout: 300_000 }, + { + cwd: worktree, + encoding: 'utf8', + timeout: 300_000, + // Vitest's JSON reporter on a large suite easily exceeds spawnSync's + // 1 MiB default stdout buffer, which returns ENOBUFS and turns every + // probe `inconclusive`. Match the 64 MiB ceiling the gh wrapper uses. + maxBuffer: 64 * 1024 * 1024, + }, ); // `r.error` is set — and `r.status` is null — when the process never ran // (npx missing) or was killed (the timeout above fires SIGTERM). Ignoring From 1006bad8965c73da1becd265c131ef2ce23936ad Mon Sep 17 00:00:00 2001 From: wenshao Date: Mon, 13 Jul 2026 16:06:49 +0800 Subject: [PATCH 06/12] fix(review): comma-clause negation, RegExp flag preservation, stale comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third self-review round. No blockers; these are the substantive suggestions. - pr-context: the negation stop-set gained clause separators last round but not the comma, so "No other concerns, but auth is a blocker" let the negation reach across the comma and suppress a real blocker — a false negative, the costly direction, flagged independently by two reviewers. Adding `,，、` leaves recall 2/2 and false positives 6/36 on the 38-comment corpus, and still negates "No blockers found, ship it". - pr-context: `carriesBlockerSignal` rebuilt each pattern with `new RegExp(re. source, 'g')`, dropping any flags the pattern carried. Harmless today (no pattern has flags) but a latent trap the moment one gains `i`/`u`. Preserve the pattern's flags and dedupe `g`. - test-efficacy: the workspace-glob comment still described the old two-pass filter as a present defect; the code is single-pass ordered evaluation. Fixed the comment to match. --- packages/cli/src/commands/review/pr-context.test.ts | 12 ++++++++++++ packages/cli/src/commands/review/pr-context.ts | 6 ++++-- packages/cli/src/commands/review/presubmit.test.ts | 11 +++++++++++ packages/cli/src/commands/review/test-efficacy.ts | 8 ++++---- 4 files changed, 31 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/commands/review/pr-context.test.ts b/packages/cli/src/commands/review/pr-context.test.ts index e78986fd694..74126d93bb8 100644 --- a/packages/cli/src/commands/review/pr-context.test.ts +++ b/packages/cli/src/commands/review/pr-context.test.ts @@ -674,6 +674,18 @@ describe('carriesBlockerSignal', () => { ).toBe(false); }); + it('does not let a negation reach across a comma either', () => { + // Both Codex and qwen flagged this: the stop-set had `;:—` but not `,`, so + // "No other concerns, but auth is a blocker" suppressed the real assertion + // across the comma — a false negative, the costly direction. Adding `,，、` + // to the stop-set left recall 2/2 and false positives 6/36 on the corpus. + expect( + carriesBlockerSignal('No other concerns, but auth is a blocker'), + ).toBe(true); + // A negation whose clause genuinely covers the signal still negates. + expect(carriesBlockerSignal('No blockers found, ship it')).toBe(false); + expect(carriesBlockerSignal('没有阻塞问题，一切正常')).toBe(false); + }); it('does not let a negation reach across a clause separator', () => { // The negation guard scans back 40 characters for a negation word, and its // stop-set only had `.!?`. "No blockers; the cache path is a (blocker)" diff --git a/packages/cli/src/commands/review/pr-context.ts b/packages/cli/src/commands/review/pr-context.ts index 6a4ad8e4ade..95249e7ff91 100644 --- a/packages/cli/src/commands/review/pr-context.ts +++ b/packages/cli/src/commands/review/pr-context.ts @@ -306,12 +306,14 @@ const BLOCKER_PATTERNS: RegExp[] = [ * (The CJK clause takes no `\b`: there are no word boundaries to anchor to.) */ const NEGATION = - /(?:\b(?:no|not|zero|without|never)\b|没有|不是|无|未发现|不存在)[^.!?。！？;:；：—\n]{0,40}$/; + /(?:\b(?:no|not|zero|without|never)\b|没有|不是|无|未发现|不存在)[^.!?。！？;:；：，,、—\n]{0,40}$/; export function carriesBlockerSignal(body: string | undefined): boolean { const b = (body ?? '').toLowerCase(); return BLOCKER_PATTERNS.some((re) => { - const m = new RegExp(re.source, 'g'); + // Preserve the pattern's own flags (a future `i`/`u` must not be silently + // dropped) and add `g` for the scan; dedupe so `g` is never doubled. + const m = new RegExp(re.source, [...new Set(re.flags + 'g')].join('')); let hit: RegExpExecArray | null; while ((hit = m.exec(b)) !== null) { // Negated occurrences do not count, but a body may both mention "no diff --git a/packages/cli/src/commands/review/presubmit.test.ts b/packages/cli/src/commands/review/presubmit.test.ts index cbfbf2716a7..ef8f4f2051b 100644 --- a/packages/cli/src/commands/review/presubmit.test.ts +++ b/packages/cli/src/commands/review/presubmit.test.ts @@ -127,6 +127,17 @@ describe('classifyCi — a skipped check is not a passing check', () => { expect(got.failedCheckNames).toContain('E2E'); }); + it('treats waiting and requested as pending, not skipped', () => { + // Real active check-run statuses. Omitting them mislabeled a commit whose + // only check is waiting as no_checks with a spurious "skipped" reason. + expect( + classifyCi([run('E2E', null as unknown as string, 'waiting')], []).class, + ).toBe('all_pending'); + expect( + classifyCi([run('Lint', null as unknown as string, 'requested')], []) + .class, + ).toBe('all_pending'); + }); it('a repo with no CI at all is still no_checks, with nothing to disclose', () => { const got = classifyCi([], []); expect(got.class).toBe('no_checks'); diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts index e45daa8380e..f54657da841 100644 --- a/packages/cli/src/commands/review/test-efficacy.ts +++ b/packages/cli/src/commands/review/test-efficacy.ts @@ -75,10 +75,10 @@ export function isWorkspaceMember( } return norm === g || norm.startsWith(`${g}/`); }; - // npm evaluates the globs IN ORDER, so a positive glob listed after a negation - // re-includes what the negation excluded. Two passes (all negations, then all - // positives) let a negation win regardless of where it sits, which would file - // a false `unreachable` — a finding posted to someone's PR. + // npm evaluates the globs IN ORDER — a positive glob listed after a negation + // re-includes what the negation excluded. So walk them in order and let the + // last match win; a two-pass filter (all negations, then all positives) would + // let a negation win wherever it sat and file a false `unreachable`. let member = false; for (const g of workspaceGlobs) { if (!matches(g)) continue; From b49ad6560bc10b621da156d5750cfa6330fe09e9 Mon Sep 17 00:00:00 2001 From: wenshao Date: Mon, 13 Jul 2026 16:44:30 +0800 Subject: [PATCH 07/12] fix(review): don't revert data fixtures; guard dirty worktree; CJK non-blocker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourth review round (Codex $qreview + qwen). No blockers survived verification; these are the confirmed correctness issues. - test-efficacy: `planTestEfficacy` reverted every `kind: source` file, but `classifyPath` labels non-executable data under a src tree `source` too — JSON fixtures, `.md` bodies, snapshots. This PR ships one such fixture that `pr-context.test.ts` loads; reverting it deleted the file and made that probe inconclusive because of the probe itself. Revert now gates on an executable-source extension. - test-efficacy: refuse to run when the `--worktree` has uncommitted changes to a revert-set file. Safe on the pipeline's ephemeral worktree, but this is a public command and the checkout-over-revert would discard a user's staged or unstaged edits with no undo. - pr-context: `非阻塞` / `并非阻塞` is the Chinese "non-blocking" — the CJK twin of the `non-blocking` lookbehind. Without a guard, "非阻塞问题" promoted and consumed the mandatory-review budget. Same class as the bilingual-negation fix two rounds ago; a guard was written for one language and not the other. - DESIGN.md: sync the pattern list (bare `blocking`, not the noun forms) and the Referenced-code claim (only when the blocker names a file) to the code. --- .../src/commands/review/pr-context.test.ts | 9 ++++ .../cli/src/commands/review/pr-context.ts | 4 +- .../src/commands/review/test-efficacy.test.ts | 17 ++++++++ .../cli/src/commands/review/test-efficacy.ts | 43 ++++++++++++++++++- .../core/src/skills/bundled/review/DESIGN.md | 4 +- 5 files changed, 73 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/commands/review/pr-context.test.ts b/packages/cli/src/commands/review/pr-context.test.ts index 74126d93bb8..bf393c3a01a 100644 --- a/packages/cli/src/commands/review/pr-context.test.ts +++ b/packages/cli/src/commands/review/pr-context.test.ts @@ -651,6 +651,15 @@ describe('carriesBlockerSignal', () => { expect(carriesBlockerSignal('非阻塞观察：建议后续跟进')).toBe(false); }); + it('guards the Chinese non-blocking prefix 非/并非', () => { + // `非阻塞` is the Chinese "non-blocking" — the CJK twin of the non-blocking + // lookbehind. Without the guard, "非阻塞问题" promoted and ate the budget. + expect(carriesBlockerSignal('非阻塞问题：建议后续跟进')).toBe(false); + expect(carriesBlockerSignal('这是并非阻塞项的小建议')).toBe(false); + // The bare blocker still promotes. + expect(carriesBlockerSignal('这是阻塞问题，必须修复')).toBe(true); + }); + it('guards the Chinese signal in Chinese, not only in English', () => { // The signal list is bilingual (`阻塞项`); the guard was not. On a repo whose // PR discussion is substantially Chinese, every "没有阻塞项" — the Chinese half diff --git a/packages/cli/src/commands/review/pr-context.ts b/packages/cli/src/commands/review/pr-context.ts index 95249e7ff91..502be19a48b 100644 --- a/packages/cli/src/commands/review/pr-context.ts +++ b/packages/cli/src/commands/review/pr-context.ts @@ -292,7 +292,9 @@ const BLOCKER_PATTERNS: RegExp[] = [ /(? { ]); }); + it('does not revert non-executable source (a .md/.json fixture)', () => { + // classifyPath labels a fixture under a src tree `source`. Reverting it is + // meaningless (no behaviour) and destructive — this PR ships a .md fixture + // that pr-context.test.ts loads; deleting it makes that test fail to load + // and the probe inconclusive because of the probe itself. + const plan = planTestEfficacy( + [ + { path: 'packages/cli/src/x.ts', kind: 'source' }, + { path: 'packages/cli/src/__fixtures__/body.md', kind: 'source' }, + { path: 'packages/cli/src/data.json', kind: 'source' }, + { path: 'packages/cli/src/x.test.ts', kind: 'test' }, + ], + GLOBS, + ); + expect(plan.revert).toEqual(['packages/cli/src/x.ts']); + }); + it('probes nothing on a test-only diff', () => { // A new test for OLD code is supposed to pass with nothing reverted. Probing // it would report every such PR as "inert" — a false blocker on exactly the diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts index f54657da841..761b1da29e3 100644 --- a/packages/cli/src/commands/review/test-efficacy.ts +++ b/packages/cli/src/commands/review/test-efficacy.ts @@ -96,6 +96,19 @@ export interface EfficacyPlan { revert: string[]; } +/** + * Executable source that can carry a behavioural change worth reverting. + * `classifyPath` labels everything under a `src` tree that is not a test, + * doc, or generated file `source` — which includes non-executable data a test + * imports: JSON fixtures, `.md` fixture bodies (this PR ships one: + * `__fixtures__/pr-6486-comment-4942713150.md`), snapshots. Reverting those to + * base deletes a file `pr-context.test.ts` then fails to load — an inconclusive + * probe caused by the probe itself. Only revert code that actually holds the + * behaviour under test. + */ +const REVERTABLE_SOURCE_RE = + /\.(tsx?|jsx?|mjs|cjs|vue|svelte|py|go|rs|java|kt|rb|c|cc|cpp|h|hpp|cs|php|swift|scala)$/; + /** * Split the diff into what to report and what to run. * @@ -107,7 +120,13 @@ export function planTestEfficacy( workspaceGlobs: string[], ): EfficacyPlan { const tests = files.filter((f) => f.kind === 'test').map((f) => f.path); - const revert = files.filter((f) => f.kind === 'source').map((f) => f.path); + // `kind === 'source'` is the diff-plan bucket for "not test/doc/generated", + // which sweeps in data files a test imports. Reverting a fixture is both + // meaningless (it holds no behaviour) and destructive (a test that loads it + // then fails), so gate on an executable-source extension too. + const revert = files + .filter((f) => f.kind === 'source' && REVERTABLE_SOURCE_RE.test(f.path)) + .map((f) => f.path); const unreachable = tests.filter( (t) => !isWorkspaceMember(t, workspaceGlobs), ); @@ -283,6 +302,28 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { }> = []; let restoreFailure: string | undefined; + if (probes.length > 0 && revert.length > 0) { + // The probe checks out base over the revert set and deletes added files, + // then restores HEAD. That is safe on the ephemeral worktree the /review + // pipeline builds, but this is a public command that accepts any + // `--worktree`: on a tree with uncommitted edits to a revert-set file, the + // checkout would discard them with no undo. Refuse a dirty revert set + // rather than eat someone's work. + const dirty = revert.filter((p) => { + const r = spawnSync('git', ['status', '--porcelain', '--', p], { + cwd: worktree, + encoding: 'utf8', + }); + return (r.stdout ?? '').trim().length > 0; + }); + if (dirty.length > 0) { + throw new Error( + `refusing to run: the worktree has uncommitted changes to files this probe would revert (${dirty.join(', ')}). ` + + `Commit or stash them first — the probe checks out base over these files and could not restore your edits.`, + ); + } + } + if (probes.length > 0 && revert.length > 0) { // "Revert to base" is two operations, not one. A file the PR MODIFIED is // checked out from base; a file the PR ADDED did not exist at base, and diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index f7f9b77e53f..7950f5dfedd 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -334,7 +334,7 @@ Two properties are deliberate: Three changes, and the ordering one is load-bearing: - **The section is written FIRST**, ahead of the description and the review history. Nothing in the file outranks the claims a `C=0` verdict may not be reached without ruling on. On the live thread this moved the heading from char 25 961 to **569**, and the blocker body from 43 094 to **4 421**. -- **Recognition matches assertion patterns, not word presence** — `[Critical]`, `(blocker)`, `is a blocker`, `blocking issue/defect/bug`, `must fix`, `still reproducible/repro/broken/fails`, `阻塞项` — with a **bilingual** negation guard, so neither "no blockers" nor "没有阻塞项" ever promotes. Live promotions dropped 8 → 3 (the one real blocker plus two harmless mentions), and the file 59 KB → 40 KB. +- **Recognition matches assertion patterns, not word presence** — `[Critical]`, `(blocker)`, `is a blocker`, a bare `blocking` (with a `non-blocking` / `非阻塞` lookbehind), `must fix`, `still reproducible/repro/broken/fails`, `阻塞项/问题/点` — with a **bilingual** negation guard, so neither "no blockers" nor "没有阻塞项" ever promotes. Live promotions dropped 8 → 3 (the one real blocker plus two harmless mentions), and the file 59 KB → 40 KB. - **The section carries a character budget.** Tight patterns keep promotion rare; the budget keeps a pathological thread from blowing the read window anyway. Bodies past it degrade to snippets **naming their exact fetch**, which the re-check already must run before ruling — not to silence. The lesson generalizes past this file: **"a false positive is cheap" is a claim about a budget, and it has to be measured against the real distribution, not assumed.** Here it was false until the ordering was fixed. @@ -375,7 +375,7 @@ PR #6486 is the case. A `Ctrl+F` dual-fire blocker was filed — the hotkey togg Two changes, split the way this document keeps arriving at — **determinism owns the evidence, judgment owns the ruling**: -- **`pr-context` extracts the evidence** (`extractCodeRefs`). A blocker's body names the code it is about — #6486's named `text-buffer.ts:2663` outright — so every promoted blocker now renders a **Referenced code** list. "Go read the untouched code" stops being a hope the agent might have and becomes a list it is handed. +- **`pr-context` extracts the evidence** (`extractCodeRefs`). A blocker's body names the code it is about — #6486's named `text-buffer.ts:2663` outright — so a promoted blocker that names a file now renders a **Referenced code** list (a blocker citing no path gets none — the reader traces the mechanism themselves). "Go read the untouched code" stops being a hope the agent might have and becomes a list it is handed. - **SKILL.md raises the bar** on the ruling: name the mechanism, name what now stops it, and when the stopping condition lives outside the diff, read it there — or the verdict is `cannot tell`. No new `compose-review` input was needed: `cannot tell` already caps the event. The change is to make wrong "fixed" rulings land there instead of passing silently. From 57927f0c50fb434a3ac52c8080797190d9be7915 Mon Sep 17 00:00:00 2001 From: wenshao Date: Mon, 13 Jul 2026 18:39:23 +0800 Subject: [PATCH 08/12] fix(review): make the dirty-worktree guard fail closed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fifth review round. One Critical and a vacuous test, both real. - test-efficacy: the dirty-worktree guard added last round called `spawnSync` directly and read `(r.stdout ?? '')`, so a spawn failure produced an empty string, read as "clean", and let the probe proceed — the fail-OPEN outcome in a guard whose whole purpose is to prevent data loss. Route it through a `gitOut` helper that throws on `r.error`/non-zero, and apply the same `r.error` check to `existsAtRev`. Verified: a dirty worktree now refuses to run and the uncommitted change survives. - pr-context.test: the comma-negation test's second assertion (`No blockers found, ship it` -> false) was vacuous — the plural `blockers` matches no pattern, so it proved nothing about the negation window. Replaced with `This is not a blocker`, which actually exercises it. - test-efficacy.test: add the source-only case (`probes: []` with a non-empty revert set), the mirror of the existing test-only case. --- .../src/commands/review/pr-context.test.ts | 8 +++-- .../src/commands/review/test-efficacy.test.ts | 11 ++++++ .../cli/src/commands/review/test-efficacy.ts | 36 +++++++++++++------ 3 files changed, 42 insertions(+), 13 deletions(-) diff --git a/packages/cli/src/commands/review/pr-context.test.ts b/packages/cli/src/commands/review/pr-context.test.ts index bf393c3a01a..6cb8fe5949b 100644 --- a/packages/cli/src/commands/review/pr-context.test.ts +++ b/packages/cli/src/commands/review/pr-context.test.ts @@ -691,8 +691,12 @@ describe('carriesBlockerSignal', () => { expect( carriesBlockerSignal('No other concerns, but auth is a blocker'), ).toBe(true); - // A negation whose clause genuinely covers the signal still negates. - expect(carriesBlockerSignal('No blockers found, ship it')).toBe(false); + // A negation whose clause genuinely covers the signal still negates — the + // negation word is in the SAME clause as the matched pattern, no comma + // between them. (`No blockers found` would be a vacuous check here: the + // plural `blockers` matches no pattern at all, so it proves nothing about + // the negation window.) + expect(carriesBlockerSignal('This is not a blocker')).toBe(false); expect(carriesBlockerSignal('没有阻塞问题，一切正常')).toBe(false); }); it('does not let a negation reach across a clause separator', () => { diff --git a/packages/cli/src/commands/review/test-efficacy.test.ts b/packages/cli/src/commands/review/test-efficacy.test.ts index d67d1cc103b..a12dad35d97 100644 --- a/packages/cli/src/commands/review/test-efficacy.test.ts +++ b/packages/cli/src/commands/review/test-efficacy.test.ts @@ -112,6 +112,17 @@ describe('planTestEfficacy', () => { expect(plan.revert).toEqual(['packages/cli/src/x.ts']); }); + it('probes nothing on a source-only diff (no tests to run)', () => { + // Mirror of the test-only case: source changed but no test file to probe + // means nothing to gate. `probes` must be empty even though `revert` is not. + const plan = planTestEfficacy( + [{ path: 'packages/cli/src/a.ts', kind: 'source' }], + GLOBS, + ); + expect(plan.revert).toEqual(['packages/cli/src/a.ts']); + expect(plan.probes).toEqual([]); + }); + it('probes nothing on a test-only diff', () => { // A new test for OLD code is supposed to pass with nothing reverted. Probing // it would report every such PR as "inert" — a false blocker on exactly the diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts index 761b1da29e3..13dbb2cce82 100644 --- a/packages/cli/src/commands/review/test-efficacy.ts +++ b/packages/cli/src/commands/review/test-efficacy.ts @@ -268,11 +268,25 @@ function git(cwd: string, ...args: string[]): void { } } -/** Does this path exist at the base commit? */ +/** Run git and return trimmed stdout; throws on spawn failure or non-zero. */ +function gitOut(cwd: string, ...args: string[]): string { + const r = spawnSync('git', args, { cwd, encoding: 'utf8' }); + if (r.error) throw r.error; + if (r.status !== 0) { + throw new Error(`git ${args.join(' ')} failed: ${r.stderr ?? ''}`); + } + return (r.stdout ?? '').trim(); +} + +/** + * Does this path exist at the given rev? A non-zero exit is a legitimate "no" + * (git prints nothing), but a spawn *failure* (`r.error`, e.g. git missing) is + * not evidence of absence — surface it rather than reading it as "not present". + */ function existsAtRev(cwd: string, rev: string, path: string): boolean { - return ( - spawnSync('git', ['cat-file', '-e', `${rev}:${path}`], { cwd }).status === 0 - ); + const r = spawnSync('git', ['cat-file', '-e', `${rev}:${path}`], { cwd }); + if (r.error) throw r.error; + return r.status === 0; } const existsAtBase = (cwd: string, base: string, path: string) => existsAtRev(cwd, base, path); @@ -309,13 +323,13 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { // `--worktree`: on a tree with uncommitted edits to a revert-set file, the // checkout would discard them with no undo. Refuse a dirty revert set // rather than eat someone's work. - const dirty = revert.filter((p) => { - const r = spawnSync('git', ['status', '--porcelain', '--', p], { - cwd: worktree, - encoding: 'utf8', - }); - return (r.stdout ?? '').trim().length > 0; - }); + // `gitOut` throws on spawn failure (via the `git()` guard), so a `git` + // that could not run fails the probe rather than silently reading as a + // clean tree — the fail-OPEN outcome would defeat the whole guard, which + // exists to prevent data loss. + const dirty = revert.filter( + (p) => gitOut(worktree, 'status', '--porcelain', '--', p).length > 0, + ); if (dirty.length > 0) { throw new Error( `refusing to run: the worktree has uncommitted changes to files this probe would revert (${dirty.join(', ')}). ` + From cb178b611077754bcff17e7743384307ed91f05f Mon Sep 17 00:00:00 2001 From: wenshao Date: Mon, 13 Jul 2026 20:05:08 +0800 Subject: [PATCH 09/12] fix(review): NDJSON per-line tolerance, dedupe failed checks, GFM nesting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sixth review round — three small confirmed issues, two of them my own from earlier rounds. - gh: `parseNdjson` threw the whole page away if any single line failed to parse. `gh` can print an update/deprecation notice to stdout, so parse line-by-line and skip a non-JSON line instead of losing the records already read. - presubmit: `failedCheckNames` used `.push()` and so listed a matrix job once per failing platform ("Test, Test, Test"), while the `skippedCheckNames` I added dedupes via a Set. Dedupe `failedCheckNames` too. - SKILL.md: a nested `**bold**` inside a `**bold**` span (introduced when I qualified the Referenced-code claim two rounds ago) breaks GFM — the inner `**` closes the outer span, mis-rendering an agent-facing instruction. Drop the inner emphasis. --- packages/cli/src/commands/review/lib/gh.test.ts | 8 ++++++++ packages/cli/src/commands/review/lib/gh.ts | 17 +++++++++++++---- .../cli/src/commands/review/presubmit.test.ts | 12 ++++++++++++ packages/cli/src/commands/review/presubmit.ts | 4 +++- .../core/src/skills/bundled/review/SKILL.md | 2 +- 5 files changed, 37 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/commands/review/lib/gh.test.ts b/packages/cli/src/commands/review/lib/gh.test.ts index 46641ddee8d..35e0913ca27 100644 --- a/packages/cli/src/commands/review/lib/gh.test.ts +++ b/packages/cli/src/commands/review/lib/gh.test.ts @@ -57,6 +57,14 @@ describe('parseNdjson (the paginated check-runs decode)', () => { ]); }); + it('skips a non-JSON line instead of losing the whole page', () => { + // gh can print an update/deprecation notice to stdout. One stray line must + // not throw away every record already parsed. + expect( + parseNdjson('{"name":"a"}\ngh version 2.x available\n{"name":"b"}'), + ).toEqual([{ name: 'a' }, { name: 'b' }]); + }); + it('returns [] for an empty response and ignores blank lines', () => { expect(parseNdjson('')).toEqual([]); expect(parseNdjson('{"name":"a"}\n\n')).toEqual([{ name: 'a' }]); diff --git a/packages/cli/src/commands/review/lib/gh.ts b/packages/cli/src/commands/review/lib/gh.ts index 5328ea169e0..6139b56bb66 100644 --- a/packages/cli/src/commands/review/lib/gh.ts +++ b/packages/cli/src/commands/review/lib/gh.ts @@ -123,10 +123,19 @@ export function ghApiAllNested(path: string, key: string): unknown[] { */ export function parseNdjson(out: string): unknown[] { if (!out) return []; - return out - .split('\n') - .filter((line) => line.trim().length > 0) - .map((line) => JSON.parse(line)); + const values: unknown[] = []; + for (const line of out.split('\n')) { + if (line.trim().length === 0) continue; + // Skip a line that is not JSON rather than throwing away every value + // already parsed — `gh` occasionally prints an update/deprecation notice + // to stdout, and one stray line should not lose the whole page. + try { + values.push(JSON.parse(line)); + } catch { + // not a JSON record; ignore + } + } + return values; } /** Login of the currently authenticated GitHub user. */ diff --git a/packages/cli/src/commands/review/presubmit.test.ts b/packages/cli/src/commands/review/presubmit.test.ts index ef8f4f2051b..1cd7cc41d56 100644 --- a/packages/cli/src/commands/review/presubmit.test.ts +++ b/packages/cli/src/commands/review/presubmit.test.ts @@ -138,6 +138,18 @@ describe('classifyCi — a skipped check is not a passing check', () => { .class, ).toBe('all_pending'); }); + + it('dedupes a matrix job that fails on several platforms', () => { + // Three legs of one failing matrix job pushed the name three times, so the + // downgrade message read "Test, Test, Test". + const got = classifyCi( + [run('Test', 'failure'), run('Test', 'failure'), run('Test', 'failure')], + [], + ); + expect(got.failedCheckNames).toEqual(['Test']); + expect(got.class).toBe('any_failure'); + }); + it('a repo with no CI at all is still no_checks, with nothing to disclose', () => { const got = classifyCi([], []); expect(got.class).toBe('no_checks'); diff --git a/packages/cli/src/commands/review/presubmit.ts b/packages/cli/src/commands/review/presubmit.ts index 4941814e146..1a1135f4f9d 100644 --- a/packages/cli/src/commands/review/presubmit.ts +++ b/packages/cli/src/commands/review/presubmit.ts @@ -181,7 +181,9 @@ export function classifyCi(checkRuns: CheckRun[], statuses: CommitStatus[]) { return { class: cls, - failedCheckNames, + // Dedupe: a matrix job failing on N platforms pushes its name N times, + // and `skippedCheckNames` already dedupes — keep the message consistent. + failedCheckNames: [...new Set(failedCheckNames)], /** * Checks that never executed at this commit. NOT a downgrade on its own — * most are routing jobs, and a docs-only PR legitimately skips the test diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index b63af4f10c7..2b1bb409664 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -741,7 +741,7 @@ A `C=0` outcome — Approve, or a Comment with no Critical — is a claim that n - **still stands** — the defect is present in the code you just read. It blocks: the event is `REQUEST_CHANGES`, and the finding goes inline (or into the body if it cannot be anchored). - **fixed by this diff** — you traced the blocker's **mechanism** through the code as it now stands and it can no longer fire. Say nothing; do not re-report it. A GitHub thread can read `isResolved: false, isOutdated: false` for a bug a later commit fixed on an adjacent line — the flag tracks the anchored line, not the fix, so the flag is not evidence either way. Only the code is. - **"The diff adds a fix" is not the same claim as "the defect can no longer fire", and this verdict requires the second one.** A fix's new lines are in the diff, but whether they _work_ frequently turns on code the diff never touches — a sibling subscriber, a registry entry, a dispatch order, a global binding, a default in a caller three files away. Read the diff alone and you see a plausible fix and rule it good. **So: name the mechanism the blocker claims, then name what now stops it. If that stopping condition lives outside the diff, go read it at the reviewed commit — a blocker in "Blockers to re-check" carries a `Referenced code` list extracted from its own body **whenever it names a file**, and the locations on it that the PR does not touch are precisely the ones this rule is about.** If you did not read them, you do not have this verdict; you have `cannot tell`. A blocker that cites no file gets no list, and hands you no shortcut: trace the mechanism through the code yourself, on the same terms. + **"The diff adds a fix" is not the same claim as "the defect can no longer fire", and this verdict requires the second one.** A fix's new lines are in the diff, but whether they _work_ frequently turns on code the diff never touches — a sibling subscriber, a registry entry, a dispatch order, a global binding, a default in a caller three files away. Read the diff alone and you see a plausible fix and rule it good. **So: name the mechanism the blocker claims, then name what now stops it. If that stopping condition lives outside the diff, go read it at the reviewed commit — a blocker in "Blockers to re-check" carries a `Referenced code` list extracted from its own body whenever it names a file, and the locations on it that the PR does not touch are precisely the ones this rule is about.** If you did not read them, you do not have this verdict; you have `cannot tell`. A blocker that cites no file gets no list, and hands you no shortcut: trace the mechanism through the code yourself, on the same terms. This is not a hypothetical. On PR #6486 the author responded to a `Ctrl+F` dual-fire blocker by adding a guard to the toggle handler. The guard is right there in the diff and reads like a fix. It changed nothing — `Ctrl+F` still toggled the model **and** moved the cursor, because the second handler is `text-buffer.ts:2663` in an untouched file, subscribed independently to a `KeypressContext.broadcast()` with no stop-propagation. The blocker's own body named that line. A re-check that read only the diff would rule "fixed" and be wrong; a re-check that read the named line could not. From a9cd9c3969d50cdcdc49e25ab9d5e3bce96d6686 Mon Sep 17 00:00:00 2001 From: wenshao Date: Mon, 13 Jul 2026 20:54:16 +0800 Subject: [PATCH 10/12] fix(review): redesign blocker negation; fail-closed parse; fixture-dir revert MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seventh review round (Codex). Six confirmed issues, four of them regressions from my own earlier fixes — a sign the patch-on-patch approach to natural language and to the probe's file selection had to be replaced, not extended. - pr-context: replace the pile of per-pattern negation lookbehinds with one negation-window model. Each lookbehind fix had opened a hole in the other direction: `(? { ]); }); - it('skips a non-JSON line instead of losing the whole page', () => { - // gh can print an update/deprecation notice to stdout. One stray line must - // not throw away every record already parsed. - expect( + it('is strict by default — a non-JSON line throws rather than fail open', () => { + // A check-runs snapshot feeds CI classification, and silently dropping a + // malformed line could hide a *failing* run — the fail-open the pagination + // fix closed, reintroduced by lenient parsing. So the default throws. + expect(() => parseNdjson('{"name":"a"}\ngh version 2.x available\n{"name":"b"}'), + ).toThrow(); + }); + + it('skips a non-JSON line only when explicitly non-strict', () => { + // The opt-in for a caller that genuinely expects interleaved notices and + // can tolerate a lost record — not the check-runs path. + expect( + parseNdjson('{"name":"a"}\ngh version 2.x available\n{"name":"b"}', { + strict: false, + }), ).toEqual([{ name: 'a' }, { name: 'b' }]); }); diff --git a/packages/cli/src/commands/review/lib/gh.ts b/packages/cli/src/commands/review/lib/gh.ts index 6139b56bb66..937bc1eb4a7 100644 --- a/packages/cli/src/commands/review/lib/gh.ts +++ b/packages/cli/src/commands/review/lib/gh.ts @@ -110,9 +110,16 @@ export function ghApiAll(path: string): unknown[] { * `--paginate --jq '.[]'` applies the jq to every page and streams each * element as a newline-delimited JSON value (NDJSON), so the result is parsed * line by line rather than as one array. (`gh api` has no `--slurp`.) + * + * `strict` parsing here: a check-runs snapshot feeds CI classification, and + * dropping a malformed line could hide a *failing* run — the same fail-open the + * pagination fix closed, reintroduced by lenient parsing. A parse failure + * throws. */ export function ghApiAllNested(path: string, key: string): unknown[] { - return parseNdjson(gh('api', '--paginate', path, '--jq', `.${key}[]`)); + return parseNdjson(gh('api', '--paginate', path, '--jq', `.${key}[]`), { + strict: true, + }); } /** @@ -120,15 +127,25 @@ export function ghApiAllNested(path: string, key: string): unknown[] { * one JSON value per non-blank line. Split out and exported so the parse is * unit-testable without spawning `gh` (the spawn is covered by the commands' * own runs, per this module's testing note above). + * + * `strict` (default) throws on any non-JSON line — correct when a dropped + * record would change a safety-relevant answer (e.g. hiding a failing check + * run). Non-strict skips a stray line, for the rare caller that genuinely + * expects interleaved human-readable notices and can tolerate a lost record. */ -export function parseNdjson(out: string): unknown[] { +export function parseNdjson( + out: string, + opts: { strict?: boolean } = {}, +): unknown[] { + const strict = opts.strict ?? true; if (!out) return []; const values: unknown[] = []; for (const line of out.split('\n')) { if (line.trim().length === 0) continue; - // Skip a line that is not JSON rather than throwing away every value - // already parsed — `gh` occasionally prints an update/deprecation notice - // to stdout, and one stray line should not lose the whole page. + if (strict) { + values.push(JSON.parse(line)); + continue; + } try { values.push(JSON.parse(line)); } catch { diff --git a/packages/cli/src/commands/review/pr-context.test.ts b/packages/cli/src/commands/review/pr-context.test.ts index 6cb8fe5949b..4f86c9d158d 100644 --- a/packages/cli/src/commands/review/pr-context.test.ts +++ b/packages/cli/src/commands/review/pr-context.test.ts @@ -651,11 +651,17 @@ describe('carriesBlockerSignal', () => { expect(carriesBlockerSignal('非阻塞观察：建议后续跟进')).toBe(false); }); - it('guards the Chinese non-blocking prefix 非/并非', () => { - // `非阻塞` is the Chinese "non-blocking" — the CJK twin of the non-blocking - // lookbehind. Without the guard, "非阻塞问题" promoted and ate the budget. + it('guards the Chinese non-blocking forms, adjacent or not', () => { + // `非阻塞` is the Chinese "non-blocking". The first guard was an adjacency + // lookbehind (`(? { ).toBe(false); }); - it('does not let a negation reach across a comma either', () => { - // Both Codex and qwen flagged this: the stop-set had `;:—` but not `,`, so - // "No other concerns, but auth is a blocker" suppressed the real assertion - // across the comma — a false negative, the costly direction. Adding `,，、` - // to the stop-set left recall 2/2 and false positives 6/36 on the corpus. + it('resets a negation at an adversative, but not at a bare comma', () => { + // The distinction a comma-stop-set got backwards. `but`/`但` reverses — the + // clause after it is asserting — so the blocker promotes. A bare comma + // coordinates, so a negated list stays negated. Both directions matter: + // the first was a false negative (real blocker suppressed), the second a + // false positive (a "No X, Y, or Z" list promoted). expect( carriesBlockerSignal('No other concerns, but auth is a blocker'), ).toBe(true); - // A negation whose clause genuinely covers the signal still negates — the - // negation word is in the SAME clause as the matched pattern, no comma - // between them. (`No blockers found` would be a vacuous check here: the - // plural `blockers` matches no pattern at all, so it proves nothing about - // the negation window.) + expect(carriesBlockerSignal('没有其他问题，但这是阻塞问题')).toBe(true); + // Coordinated negated list — the `No` distributes across the commas. + expect( + carriesBlockerSignal('No blocking, must-fix, or critical issues.'), + ).toBe(false); + // Plain same-clause negation still negates. expect(carriesBlockerSignal('This is not a blocker')).toBe(false); expect(carriesBlockerSignal('没有阻塞问题，一切正常')).toBe(false); }); - it('does not let a negation reach across a clause separator', () => { - // The negation guard scans back 40 characters for a negation word, and its - // stop-set only had `.!?`. "No blockers; the cache path is a (blocker)" - // therefore suppressed the real assertion — a false negative, which is the - // costly direction. + it('breaks the negation window at a semicolon or colon (new clause)', () => { + // `;` and `:` start an independent clause, so a negation before one does not + // carry into it — "No blockers; the cache path is a blocker" promotes. This + // is the opposite of a bare comma, which only coordinates a list (see the + // adversative test above). Both are false-negative-avoiding. expect( - carriesBlockerSignal('No blockers; the cache path is a (blocker)'), + carriesBlockerSignal('No blockers; the cache path is a blocker'), ).toBe(true); expect( - carriesBlockerSignal('No blockers: the cache path is a (blocker)'), + carriesBlockerSignal('No blockers: the cache path is a blocker'), ).toBe(true); - // …and the plain negation still negates. + // …and the plain same-clause negation still negates. expect(carriesBlockerSignal('No critical blockers. LGTM.')).toBe(false); + // A CJK negation whose clause ends at `：` before the signal still negates. expect(carriesBlockerSignal('没有阻塞问题：一切正常')).toBe(false); }); diff --git a/packages/cli/src/commands/review/pr-context.ts b/packages/cli/src/commands/review/pr-context.ts index 502be19a48b..64d1330a890 100644 --- a/packages/cli/src/commands/review/pr-context.ts +++ b/packages/cli/src/commands/review/pr-context.ts @@ -287,28 +287,52 @@ const BLOCKER_PATTERNS: RegExp[] = [ // `blocking` on its own, because that is how people actually write it: a // "blocking gap", a "🔴 Blocking:" heading. Naming the nouns (`blocking // issue|defect|bug`) looked precise and missed a real blocker — see below. - // The lookbehind is what keeps "non-blocking" out, which matters: our own - // verification reports file their nits under "🟡 Non-blocking observations". - /(? { ]); }); - it('does not revert non-executable source (a .md/.json fixture)', () => { - // classifyPath labels a fixture under a src tree `source`. Reverting it is - // meaningless (no behaviour) and destructive — this PR ships a .md fixture - // that pr-context.test.ts loads; deleting it makes that test fail to load - // and the probe inconclusive because of the probe itself. + it('excludes fixture-directory data but keeps runtime-loaded source', () => { + // The discriminator is the directory, not the extension. A `.md` fixture + // under `__fixtures__/` is test-support data — reverting it breaks the test + // that loads it. But an executable skill prompt (`SKILL.md`) and a config + // JSON a test validates against are production source that a test can + // genuinely gate, so they stay revertable. const plan = planTestEfficacy( [ { path: 'packages/cli/src/x.ts', kind: 'source' }, { path: 'packages/cli/src/__fixtures__/body.md', kind: 'source' }, - { path: 'packages/cli/src/data.json', kind: 'source' }, + { + path: 'packages/core/src/skills/bundled/review/SKILL.md', + kind: 'source', + }, + { path: 'packages/cli/src/config/schema.json', kind: 'source' }, { path: 'packages/cli/src/x.test.ts', kind: 'test' }, ], GLOBS, ); - expect(plan.revert).toEqual(['packages/cli/src/x.ts']); + expect(plan.revert).toEqual([ + 'packages/cli/src/x.ts', + 'packages/core/src/skills/bundled/review/SKILL.md', + 'packages/cli/src/config/schema.json', + ]); }); it('probes nothing on a source-only diff (no tests to run)', () => { diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts index 13dbb2cce82..66424800d30 100644 --- a/packages/cli/src/commands/review/test-efficacy.ts +++ b/packages/cli/src/commands/review/test-efficacy.ts @@ -97,17 +97,21 @@ export interface EfficacyPlan { } /** - * Executable source that can carry a behavioural change worth reverting. - * `classifyPath` labels everything under a `src` tree that is not a test, - * doc, or generated file `source` — which includes non-executable data a test - * imports: JSON fixtures, `.md` fixture bodies (this PR ships one: - * `__fixtures__/pr-6486-comment-4942713150.md`), snapshots. Reverting those to - * base deletes a file `pr-context.test.ts` then fails to load — an inconclusive - * probe caused by the probe itself. Only revert code that actually holds the - * behaviour under test. + * Test-support data a test file imports: fixtures, mocks, snapshots. Reverting + * one is both meaningless (it holds no behaviour) and destructive — this PR + * ships `__fixtures__/pr-6486-comment-4942713150.md`, and deleting it makes + * `pr-context.test.ts` fail to load, an inconclusive probe caused by the probe + * itself. + * + * The discriminator is the **directory**, not the extension. An earlier cut + * whitelisted executable extensions, which also dropped runtime-loaded sources + * that a test genuinely gates: an executable skill prompt + * (`packages/core/src/skills/**\/SKILL.md`), a settings-schema JSON a test + * validates against. Those are production source and must stay revertable; + * only test-support data under a fixtures/mocks/snapshots path is excluded. */ -const REVERTABLE_SOURCE_RE = - /\.(tsx?|jsx?|mjs|cjs|vue|svelte|py|go|rs|java|kt|rb|c|cc|cpp|h|hpp|cs|php|swift|scala)$/; +const FIXTURE_DIR_RE = + /(^|\/)(__fixtures__|__mocks__|__snapshots__|fixtures)\//; /** * Split the diff into what to report and what to run. @@ -121,11 +125,12 @@ export function planTestEfficacy( ): EfficacyPlan { const tests = files.filter((f) => f.kind === 'test').map((f) => f.path); // `kind === 'source'` is the diff-plan bucket for "not test/doc/generated", - // which sweeps in data files a test imports. Reverting a fixture is both - // meaningless (it holds no behaviour) and destructive (a test that loads it - // then fails), so gate on an executable-source extension too. + // which sweeps in test-support data a test imports. Reverting a fixture is + // meaningless and destructive (a test that loads it then fails), so exclude + // the fixture/mock directories — but keep everything else, including + // runtime-loaded prompts and config a test genuinely gates. const revert = files - .filter((f) => f.kind === 'source' && REVERTABLE_SOURCE_RE.test(f.path)) + .filter((f) => f.kind === 'source' && !FIXTURE_DIR_RE.test(f.path)) .map((f) => f.path); const unreachable = tests.filter( (t) => !isWorkspaceMember(t, workspaceGlobs), @@ -327,8 +332,14 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { // that could not run fails the probe rather than silently reading as a // clean tree — the fail-OPEN outcome would defeat the whole guard, which // exists to prevent data loss. + // `--ignored` too: a revert-set path can be gitignored at HEAD (a generated + // or locally-recreated file), and a plain `status --porcelain` says nothing + // about it — the base checkout would then overwrite a file the user has and + // git will not restore. const dirty = revert.filter( - (p) => gitOut(worktree, 'status', '--porcelain', '--', p).length > 0, + (p) => + gitOut(worktree, 'status', '--porcelain', '--ignored', '--', p).length > + 0, ); if (dirty.length > 0) { throw new Error( From cdec7bb9f8e049f033ac34bef8e4c00b2d7b5475 Mon Sep 17 00:00:00 2001 From: wenshao Date: Mon, 13 Jul 2026 21:59:41 +0800 Subject: [PATCH 11/12] fix(review): ReDoS, un-replied blocker promotion, status pagination, path guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eighth review round (Codex). Several confirmed defects, and one I got wrong last round. - pr-context: **ReDoS in `CODE_REF_RE`** — I rejected this as a hallucination after testing the wrong input shape. Codex's exact shape (`"(blocker)\n" + "a".repeat(n)`) reproduces: the two overlapping greedy quantifiers `[\w./@-]*[\w-]+\.` backtrack catastrophically when `\.ext` fails, ~7s at 80k chars on an untrusted comment body. Replaced with a single bounded class `[\w./@-]{0,200}[\w-]\.` — 0 ms at 80k, same matches. - pr-context: **an un-replied blocker root was never promoted.** Only *replied* roots ran through `carriesBlockerSignal`; a fresh `[Critical]` with no reply went straight into "Open inline comments" as a 240-char snippet — the exact read-window failure this change exists to close, left open for the un-replied half. Open blocker roots now join the re-check section, rendered first and in full. - pr-context: the negation window resets at a space-surrounded hyphen (` - ` / ` -- `), an informal clause separator, without touching `must-fix` / `non-blocking`. - presubmit: paginate the legacy combined-status endpoint (same first-page-only gap as check-runs — a failing status on page 2 was invisible). - test-efficacy: reject a revert path that escapes the worktree (the report JSON is untrusted and these become git pathspecs / fs targets), and exit non-zero on a restore failure so a caller cannot mistake a base-code tree for a clean run. --- .../src/commands/review/pr-context.test.ts | 73 +++++++++++++++++- .../cli/src/commands/review/pr-context.ts | 77 ++++++++++++++----- .../cli/src/commands/review/presubmit.test.ts | 56 +++++++------- packages/cli/src/commands/review/presubmit.ts | 10 ++- .../cli/src/commands/review/test-efficacy.ts | 28 ++++++- 5 files changed, 185 insertions(+), 59 deletions(-) diff --git a/packages/cli/src/commands/review/pr-context.test.ts b/packages/cli/src/commands/review/pr-context.test.ts index 4f86c9d158d..2393a5ef170 100644 --- a/packages/cli/src/commands/review/pr-context.test.ts +++ b/packages/cli/src/commands/review/pr-context.test.ts @@ -318,7 +318,10 @@ describe('buildMarkdown — truncation refs are copy-runnable with real coordina user: { login: 'r' }, path: 'a.ts', line: 1, - body: `Must fix: ${'x'.repeat(400)}`, + // A non-blocker open root (a plain nit) — one carrying a blocker signal + // would now be promoted to the re-check section and rendered in full, + // not left as an open-section snippet. + body: `Please rename this helper: ${'x'.repeat(400)}`, }, ]; const issue = [{ id: 31, user: { login: 'r' }, body: 'y'.repeat(400) }]; @@ -395,7 +398,11 @@ describe('buildMarkdown — truncation refs are copy-runnable with real coordina // re-check section never appears in the body at all — the finding is headed // "🔴 Finding 1". // -// The fixture is the real comment body, byte for byte. +// The fixture is the real #6486 comment body. It DOES contain `[Critical]` +// (inside doudouOUC's quoted text) and is not byte-identical to the live +// thread — the point it proves is that a maintainer's blocker filed as an +// ISSUE comment gets promoted and rendered in full past the 25k cut, which the +// literal-marker gate would have missed. describe('buildMarkdown — a markerless maintainer blocker must not render as an endorsement (PR #6486 regression)', () => { const realBody = readFileSync( join( @@ -707,6 +714,23 @@ describe('carriesBlockerSignal', () => { expect(carriesBlockerSignal('This is not a blocker')).toBe(false); expect(carriesBlockerSignal('没有阻塞问题，一切正常')).toBe(false); }); + + it('resets a negation at a space-surrounded hyphen, not at must-fix', () => { + // ` - ` / ` -- ` is an informal clause separator (like an em dash), so the + // clause after it is asserting. Space-surrounded on purpose: `must-fix` and + // `non-blocking` have no surrounding spaces and are untouched. + expect( + carriesBlockerSignal( + 'No blockers - auth is still broken and is a blocker', + ), + ).toBe(true); + expect(carriesBlockerSignal('No issues -- the cache is a blocker')).toBe( + true, + ); + expect(carriesBlockerSignal('This is a must-fix issue')).toBe(true); + expect(carriesBlockerSignal('🟡 Non-blocking observations')).toBe(false); + }); + it('breaks the negation window at a semicolon or colon (new clause)', () => { // `;` and `:` start an independent clause, so a negation before one does not // carry into it — "No blockers; the cache path is a blocker" promotes. This @@ -886,13 +910,58 @@ describe('classifyInlineThreads', () => { { id: 3, user: { login: 'r' }, body: '**[Suggestion]** nit' }, { id: 4, user: { login: 'a' }, in_reply_to_id: 3, body: 'done' }, { id: 5, user: { login: 'r' }, body: 'open question' }, + // A fresh un-replied blocker: must NOT fall into openRoots. + { id: 6, user: { login: 'r' }, body: '**[Critical]** open blocker' }, ]; const t = classifyInlineThreads(inline); expect(t.repliedBlockerRoots.map((c) => c.id)).toEqual([1]); + expect(t.openBlockerRoots.map((c) => c.id)).toEqual([6]); expect(t.repliedRoots.map((c) => c.id)).toEqual([3]); expect(t.openRoots.map((c) => c.id)).toEqual([5]); expect(t.repliesByRoot.get(1)!.map((c) => c.id)).toEqual([2]); }); + + it('promotes an un-replied blocker root to the re-check section, in full', () => { + // The gap this closes: a fresh `[Critical]` with no reply used to go + // straight into "Open inline comments" as a 240-char snippet, past the read + // window — the exact failure the whole change exists to prevent, left open + // for the un-replied half. + const meta = { + title: 'T', + body: 'D', + author: { login: 'a' }, + baseRefName: 'main', + headRefName: 'b', + headRefOid: 's', + additions: 1, + deletions: 1, + changedFiles: 1, + state: 'OPEN', + } as PrMetadata; + const md = buildMarkdown( + '1', + 'o/r', + meta, + [ + { + id: 1, + user: { login: 'rev' }, + path: 'a.ts', + line: 3, + body: '**[Critical]** the cache is never invalidated', + }, + ], + [], + [], + ); + const section = md.indexOf('## Blockers to re-check'); + const body = md.indexOf('the cache is never invalidated'); + expect(section).toBeGreaterThanOrEqual(0); + expect(body).toBeGreaterThan(section); + // Rendered before any Open/Already-discussed section, i.e. inside the read + // window, not as a trailing snippet. + expect(md).not.toContain('## Open inline comments'); + }); }); describe('prContextCommand builder', () => { diff --git a/packages/cli/src/commands/review/pr-context.ts b/packages/cli/src/commands/review/pr-context.ts index 64d1330a890..5fcccd5c796 100644 --- a/packages/cli/src/commands/review/pr-context.ts +++ b/packages/cli/src/commands/review/pr-context.ts @@ -193,8 +193,15 @@ export function fullIssueCommentBody( // word-character transition, so `@scope/pkg/index.ts` extracted as // `scope/pkg/index.ts` and `../lib/b.ts` as `lib/b.ts` — a path whose meaning // is not the path that was cited. +// The path body is `[\w./@-]{0,200}[\w-]` — a bounded run ending in a name +// char — NOT `[\w./@-]*[\w-]+`. The two overlapping greedy quantifiers in the +// old form backtracked catastrophically when the trailing `\.ext` failed: a +// long extensionless token (`"(blocker)\n" + "a".repeat(n)`) was O(n²), ~7 s at +// 80k chars, a real ReDoS on an untrusted comment body. The single bounded +// class cannot split, and {0,200} caps a real code path well above any genuine +// one while making the scan linear. const CODE_REF_RE = - /(?; @@ -456,25 +469,37 @@ export function classifyInlineThreads(inline: RawComment[]): InlineThreads { const roots = inline.filter( (c) => c.in_reply_to_id === undefined || c.in_reply_to_id === null, ); - const allRepliedRoots = roots.filter((c) => repliesByRoot.has(c.id)); - // A reply alone does not retire a blocker — "I disagree" is a reply. Any - // replied thread whose root asserts a blocking defect is pulled out of - // "Already discussed" into its own mandatory re-check section. Promotion is - // fail-safe in this direction: a third party can only ADD their thread to - // the re-check list, never hide one. + // A root asserting a blocking defect is pulled into the mandatory re-check + // section, rendered first and in full — WHETHER OR NOT it has a reply. An + // earlier cut only promoted *replied* roots, so a fresh un-replied `[Critical]` + // went straight into "Open inline comments" as a 240-char snippet: exactly the + // "blocker past the read window" failure this whole change exists to close, + // left open for the un-replied half. Promotion is fail-safe either way — a + // third party can only ADD a thread to the re-check list, never hide one. // - // This used to key on the literal `[Critical]` marker, which only /review - // emits — so a human blocker phrased any other way settled into the section - // headed "do NOT re-report". `carriesBlockerSignal` is the semantic test. - const repliedBlockerRoots = allRepliedRoots.filter((c) => - carriesBlockerSignal(c.body), + // (This used to key on the literal `[Critical]` marker, which only /review + // emits — a human blocker phrased any other way settled into "do NOT + // re-report". `carriesBlockerSignal` is the semantic test.) + const repliedBlockerRoots = roots.filter( + (c) => repliesByRoot.has(c.id) && carriesBlockerSignal(c.body), ); - const repliedRoots = allRepliedRoots.filter( - (c) => !carriesBlockerSignal(c.body), + const openBlockerRoots = roots.filter( + (c) => !repliesByRoot.has(c.id) && carriesBlockerSignal(c.body), + ); + const repliedRoots = roots.filter( + (c) => repliesByRoot.has(c.id) && !carriesBlockerSignal(c.body), + ); + const openRoots = roots.filter( + (c) => !repliesByRoot.has(c.id) && !carriesBlockerSignal(c.body), ); - const openRoots = roots.filter((c) => !repliesByRoot.has(c.id)); - return { openRoots, repliedBlockerRoots, repliedRoots, repliesByRoot }; + return { + openRoots, + openBlockerRoots, + repliedBlockerRoots, + repliedRoots, + repliesByRoot, + }; } /** @@ -609,8 +634,16 @@ export function buildMarkdown( issue: RawComment[], reviews: RawReview[], ): string { - const { openRoots, repliedBlockerRoots, repliedRoots, repliesByRoot } = - classifyInlineThreads(inline); + const { + openRoots, + openBlockerRoots, + repliedBlockerRoots, + repliedRoots, + repliesByRoot, + } = classifyInlineThreads(inline); + // Both replied and un-replied blocker roots go to the re-check section, + // rendered first and in full. Un-replied ones simply have no reply chain. + const allBlockerRoots = [...repliedBlockerRoots, ...openBlockerRoots]; const ctx: RefContext = { ownerRepo, prNumber }; // Issue-level comments are the channel a maintainer's out-of-band review @@ -654,7 +687,7 @@ export function buildMarkdown( // section existed and nobody could see it, which is the PR #5738 failure this // file already carries a comment about, reintroduced one section further down. parts.push( - ...blockerSection(repliedBlockerRoots, blockerIssue, repliesByRoot, ctx), + ...blockerSection(allBlockerRoots, blockerIssue, repliesByRoot, ctx), ); parts.push('## Description'); @@ -835,8 +868,10 @@ async function runPrContext(args: PrContextArgs): Promise { ).length; // Same walk buildMarkdown just rendered from — never a re-implementation, // so this count cannot silently diverge from the file's contents. + const threads = classifyInlineThreads(inline); const blockerCount = - classifyInlineThreads(inline).repliedBlockerRoots.length + + threads.repliedBlockerRoots.length + + threads.openBlockerRoots.length + issue.filter((c) => carriesBlockerSignal(c.body)).length; writeStdoutLine( `Wrote PR context to ${out} (${inline.length} inline, ${issue.length} issue comments, ${blockerCount} blocker(s) to re-check, ${meaningfulReviewCount}/${reviews.length} review summaries — review bodies and blocker bodies rendered in full)`, diff --git a/packages/cli/src/commands/review/presubmit.test.ts b/packages/cli/src/commands/review/presubmit.test.ts index 1cd7cc41d56..fd2860913d7 100644 --- a/packages/cli/src/commands/review/presubmit.test.ts +++ b/packages/cli/src/commands/review/presubmit.test.ts @@ -243,14 +243,15 @@ describe('presubmitCommand', () => { // — the boolean compose-review actually acts on — did not. The disclosure was // written and the downgrade never fired. A reason nobody reads is not a gate, // so the assertion is on the boolean, through the real command. - ghApiAllNestedMock.mockReturnValue([ - { name: 'Test', status: 'completed', conclusion: 'skipped' }, - { name: 'Lint', status: 'completed', conclusion: 'skipped' }, - ]); - ghApiMock.mockImplementation((path: string) => { - if (path.endsWith('/status')) return { statuses: [] }; - return null; - }); + ghApiAllNestedMock.mockImplementation((path: string) => + path.endsWith('/check-runs') + ? [ + { name: 'Test', status: 'completed', conclusion: 'skipped' }, + { name: 'Lint', status: 'completed', conclusion: 'skipped' }, + ] + : [], + ); + ghApiMock.mockReturnValue(null); const handler = presubmitCommand.handler; if (!handler) throw new Error('presubmit handler missing'); @@ -267,26 +268,25 @@ describe('presubmitCommand', () => { }); it('ignores the running Qwen PR review check when deciding whether CI is still pending', async () => { - ghApiAllNestedMock.mockReturnValue([ - { - name: 'Test (ubuntu-latest, Node 22.x)', - status: 'completed', - conclusion: 'success', - }, - { - name: 'review-pr', - status: 'in_progress', - conclusion: null, - details_url: - 'https://github.com/QwenLM/qwen-code/actions/runs/28788268483/job/85362025778', - }, - ]); - ghApiMock.mockImplementation((path: string) => { - if (path.endsWith('/status')) { - return { statuses: [] }; - } - return null; - }); + ghApiAllNestedMock.mockImplementation((path: string) => + path.endsWith('/check-runs') + ? [ + { + name: 'Test (ubuntu-latest, Node 22.x)', + status: 'completed', + conclusion: 'success', + }, + { + name: 'review-pr', + status: 'in_progress', + conclusion: null, + details_url: + 'https://github.com/QwenLM/qwen-code/actions/runs/28788268483/job/85362025778', + }, + ] + : [], + ); + ghApiMock.mockReturnValue(null); const handler = presubmitCommand.handler; if (!handler) throw new Error('presubmit handler missing'); diff --git a/packages/cli/src/commands/review/presubmit.ts b/packages/cli/src/commands/review/presubmit.ts index 1a1135f4f9d..0644f62d5ac 100644 --- a/packages/cli/src/commands/review/presubmit.ts +++ b/packages/cli/src/commands/review/presubmit.ts @@ -15,7 +15,6 @@ import { writeFileSync, readFileSync } from 'node:fs'; import { writeStdoutLine } from '../../utils/stdioHelpers.js'; import { gh, - ghApi, ghApiAll, ghApiAllNested, currentUser, @@ -264,10 +263,13 @@ async function runPresubmit(args: PresubmitArgs): Promise { `repos/${owner}/${repo}/commits/${commitSha}/check-runs`, 'check_runs', ) as CheckRun[]; - const statusResp = ghApi( + // Paginate the legacy combined-status endpoint too (default 30 per page): + // same first-page-only gap as check-runs — a failing or pending status on + // page 2 would otherwise be invisible and let the review approve past it. + const statuses = ghApiAllNested( `repos/${owner}/${repo}/commits/${commitSha}/status`, - ) as { statuses?: CommitStatus[] } | null; - const statuses = statusResp?.statuses ?? []; + 'statuses', + ) as CommitStatus[]; const ciStatus = classifyCi(checkRuns, statuses); // --- Existing Qwen Code comments -------------------------------------- diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts index 66424800d30..5cf6543c9c2 100644 --- a/packages/cli/src/commands/review/test-efficacy.ts +++ b/packages/cli/src/commands/review/test-efficacy.ts @@ -37,7 +37,7 @@ import type { CommandModule } from 'yargs'; import { spawnSync } from 'node:child_process'; import { mkdirSync, writeFileSync, readFileSync, rmSync } from 'node:fs'; -import { dirname, join } from 'node:path'; +import { dirname, join, isAbsolute, sep } from 'node:path'; import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; export type ProbeVerdict = 'gated' | 'inert' | 'inconclusive'; @@ -314,6 +314,22 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { globs, ); + // The report JSON is untrusted input, and `revert` paths become both git + // pathspecs and `join(worktree, …)` filesystem targets we check out and + // delete. Reject anything that is not a plain repository-relative path — an + // absolute path, or one that normalises outside the worktree (`../`, or a + // `a/../../b` that looks clean per-segment) — before it can point the + // checkout/delete at a file outside the tree. + for (const p of revert) { + const norm = join(worktree, p); + const root = join(worktree, '.'); + if (isAbsolute(p) || (norm !== root && !norm.startsWith(root + sep))) { + throw new Error( + `refusing to run: revert path escapes the worktree: ${JSON.stringify(p)}`, + ); + } + } + const results: Array<{ file: string; verdict: ProbeVerdict; @@ -482,10 +498,14 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { writeStdoutLine(` [test] ${f.kind}: ${f.file}`); } if (restoreFailure) { - // Loud, and on stderr: every subsequent step of the review is now reading - // base code out of this worktree, and a line buried in a JSON field is not - // going to stop it. + // Loud, on stderr, AND a non-zero exit. The worktree is now on base code, + // so every later review step reads the wrong source; a line in a JSON + // field the workflow does not consume would let it proceed anyway (the + // fail-open the whole guard exists to prevent). The report is already + // written, so the caller still has the findings — it just cannot mistake + // this for a clean run. writeStderrLine(`ERROR: ${restoreFailure}`); + process.exitCode = 1; } } From 937bfa6dfe2c67323e692a471f8e22ff796f6786 Mon Sep 17 00:00:00 2001 From: wenshao Date: Mon, 13 Jul 2026 22:55:44 +0800 Subject: [PATCH 12/12] fix(review): don't let the efficacy probe delete through a PR-controlled symlink MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A reviewer reproduced a P0. The efficacy probe reverts the PR's source to base in the shared worktree and restores it afterward, deleting files with `rmSync(join(worktree, p), { force: true })`. `rmSync` follows symlinks in the path prefix, and the revert set is PR-controlled, so: 1. base has a real `dir/victim`; 2. PR head replaces `dir` with a symlink to an outside directory and deletes `dir/victim`; 3. the probe restores HEAD (the `dir` symlink), then deletes the `dir/victim` path — which now resolves through the link and removes the OUTSIDE file. The lexical `escapes the worktree` guard added last round cannot catch this: `dir/victim` is lexically inside the tree; the escape is a runtime symlink traversal. Confirmed by driving the real handler — the outside file was deleted. Both delete sites (reverting an added file, restoring a deleted one) now go through `safeRmWithin`, which walks every path component from the worktree root and refuses when an ANCESTOR is a symlink. The final component being a symlink is still fine — that unlinks the link itself, which is what reverting an added symlink should do. A refusal fails closed: it sets the restore-failure disclosure and the non-zero exit, so the tree is never silently left mutated. Verified: the P0 repro now leaves the outside file intact and exits non-zero; a legit no-symlink PR still restores cleanly (exit 0, worktree back at HEAD). The deeper fix the reviewer suggested — run the probe in a disposable isolated worktree — also addresses the concurrent-read Critical and is tracked as a follow-up; this closes the file-deletion vector now. --- .../src/commands/review/test-efficacy.test.ts | 55 +++++++++++++++++++ .../cli/src/commands/review/test-efficacy.ts | 50 ++++++++++++++++- 2 files changed, 102 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/commands/review/test-efficacy.test.ts b/packages/cli/src/commands/review/test-efficacy.test.ts index 16b916ec8be..b19aac4d8e0 100644 --- a/packages/cli/src/commands/review/test-efficacy.test.ts +++ b/packages/cli/src/commands/review/test-efficacy.test.ts @@ -9,7 +9,18 @@ import { isWorkspaceMember, planTestEfficacy, classifyProbeRun, + safeRmWithin, } from './test-efficacy.js'; +import { + mkdtempSync, + mkdirSync, + writeFileSync, + symlinkSync, + existsSync, + readFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; // The real root `package.json` workspace list. const GLOBS = [ @@ -145,6 +156,50 @@ describe('planTestEfficacy', () => { }); }); +describe('safeRmWithin', () => { + // A reviewer reproduced a P0: the revert set is PR-controlled, and `rmSync` + // follows symlinks in the path prefix, so a PR that turns `dir` into a symlink + // to an outside directory and has the probe delete `dir/victim` deleted the + // OUTSIDE file. These pin the guard that closed it. + const setup = () => { + const root = mkdtempSync(join(tmpdir(), 'saferm-root-')); + const outside = mkdtempSync(join(tmpdir(), 'saferm-outside-')); + writeFileSync(join(outside, 'victim'), 'must survive'); + return { root, outside }; + }; + + it('removes a file reachable through real directories', () => { + const { root } = setup(); + mkdirSync(join(root, 'realdir')); + writeFileSync(join(root, 'realdir', 'f'), 'x'); + safeRmWithin(root, 'realdir/f'); + expect(existsSync(join(root, 'realdir', 'f'))).toBe(false); + }); + + it('refuses to delete through a symlinked ancestor, sparing the outside file', () => { + const { root, outside } = setup(); + // `dir` is a symlink to an outside directory; deleting `dir/victim` must not + // follow it. This is the exact P0 shape. + symlinkSync(outside, join(root, 'dir')); + expect(() => safeRmWithin(root, 'dir/victim')).toThrow(/through a symlink/); + expect(readFileSync(join(outside, 'victim'), 'utf8')).toBe('must survive'); + }); + + it('unlinks a symlink that is itself the target, not what it points at', () => { + const { root, outside } = setup(); + // Reverting an ADDED symlink means removing the link — never its target. + symlinkSync(outside, join(root, 'addedlink')); + safeRmWithin(root, 'addedlink'); + expect(existsSync(join(root, 'addedlink'))).toBe(false); + expect(existsSync(join(outside, 'victim'))).toBe(true); + }); + + it('is a no-op on a missing path (force rm never threw there either)', () => { + const { root } = setup(); + expect(() => safeRmWithin(root, 'nope/gone')).not.toThrow(); + }); +}); + describe('classifyProbeRun', () => { const json = (o: unknown) => JSON.stringify(o); const only = (got: T[]): T => got[0]; diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts index 5cf6543c9c2..90b0d167428 100644 --- a/packages/cli/src/commands/review/test-efficacy.ts +++ b/packages/cli/src/commands/review/test-efficacy.ts @@ -36,7 +36,13 @@ import type { CommandModule } from 'yargs'; import { spawnSync } from 'node:child_process'; -import { mkdirSync, writeFileSync, readFileSync, rmSync } from 'node:fs'; +import { + mkdirSync, + writeFileSync, + readFileSync, + rmSync, + lstatSync, +} from 'node:fs'; import { dirname, join, isAbsolute, sep } from 'node:path'; import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; @@ -293,6 +299,44 @@ function existsAtRev(cwd: string, rev: string, path: string): boolean { if (r.error) throw r.error; return r.status === 0; } +/** + * Remove `join(worktree, relPath)` without following a PR-controlled symlink. + * + * `rmSync` follows symlinks in the path PREFIX, and the revert set is + * PR-controlled: a diff that turns `dir` into a symlink to an outside directory + * and has the probe delete `dir/victim` would make `rmSync` follow `dir` and + * delete the outside file — a real P0 a reviewer reproduced. The lexical + * `escapes the worktree` guard cannot catch it, because `dir/victim` is lexically + * inside the tree; the escape happens at runtime through the link. + * + * So walk every component from the worktree root down and refuse if any + * ANCESTOR is a symlink — the target must be reachable through real directories + * only. The final component being a symlink is fine: `rmSync` unlinks the link + * itself, not what it points at, which is exactly what reverting an added + * symlink should do. A missing component means there is nothing to remove + * (`force` rm is already a no-op there), so return quietly. + */ +export function safeRmWithin(worktree: string, relPath: string): void { + const parts = relPath.split(/[/\\]+/).filter((s) => s && s !== '.'); + let cur = worktree; + for (let i = 0; i < parts.length; i++) { + cur = join(cur, parts[i]); + let st; + try { + st = lstatSync(cur); + } catch { + return; + } + if (st.isSymbolicLink() && i < parts.length - 1) { + throw new Error( + `refusing to delete through a symlink: ${relPath} ` + + `(ancestor ${parts.slice(0, i + 1).join('/')} is a symlink)`, + ); + } + } + rmSync(cur, { force: true }); +} + const existsAtBase = (cwd: string, base: string, path: string) => existsAtRev(cwd, base, path); /** A file the PR DELETED does not exist at HEAD — restoring it means removing it. */ @@ -385,7 +429,7 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { // An added file's base state is "absent". Removing it is the honest // revert; the probe usually then fails to compile, which is // `inconclusive` — not a verdict, but an honest one. - for (const p of added) rmSync(join(worktree, p), { force: true }); + for (const p of added) safeRmWithin(worktree, p); const r = spawnSync( 'npx', @@ -458,7 +502,7 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { // working tree, so removing the file leaves a staged phantom add // behind (`AD` in `git status`). Reset those index entries to HEAD — // where the path does not exist, which is exactly the state we want. - for (const p of notAtHead) rmSync(join(worktree, p), { force: true }); + for (const p of notAtHead) safeRmWithin(worktree, p); git(worktree, 'reset', '-q', 'HEAD', '--', ...notAtHead); } } catch (e) {