fix(review): stop dropping live blockers, and probe whether new tests actually gate new code

packages/cli/src/commands/review.test.ts

@@ -45,6 +45,7 @@ describe('reviewCommand', () => {
       'pr-context',
       'load-rules',
       'presubmit',
+      'test-efficacy',
       'compose-review',
       'cleanup',
     ]);

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: () => {

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. 👍
+
+<details>
+<summary>🇨🇳 中文说明（点击展开）</summary>
+
+## 🔬 维护者本地构建与真机验证
+
+我从源码构建了此 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 是很小的打磨。改完这两处即可合并。👍
+
+</details>

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,42 @@ 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 <path> --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('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' }]);
+  });
+
+  it('returns [] for an empty response and ignores blank lines', () => {
+    expect(parseNdjson('')).toEqual([]);
+    expect(parseNdjson('{"name":"a"}\n\n')).toEqual([{ name: 'a' }]);
+  });
+});

packages/cli/src/commands/review/lib/gh.ts

@@ -97,6 +97,64 @@ 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 '.<key>[]'` 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}[]`), {
+    strict: true,
+  });
+}
+
+/**
+ * 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).
+ *
+ * `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,
+  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;
+    if (strict) {
+      values.push(JSON.parse(line));
+      continue;
+    }
+    try {
+      values.push(JSON.parse(line));
+    } catch {
+      // not a JSON record; ignore
+    }
+  }
+  return values;
+}
+
 /** Login of the currently authenticated GitHub user. */
 export function currentUser(): string {
   return gh('api', 'user', '--jq', '.login');