fix(review): stop dropping live blockers, and probe whether new tests actually gate new code - #6790
Merged
Merged
Conversation
Collaborator
|
Thanks for the PR! (re-run) Template: PR body doesn't follow the exact template headings — uses Problem: Observed, not theoretical. PR #6486's dropped Ctrl+F dual-fire blocker is documented with timestamps, character positions, and the exact chain of structural failures. All four holes have concrete evidence — the skipped CI classification, the inert integration test, the 240-char snippet burying a maintainer's blocker. This is not "could theoretically happen"; it did happen, on a real PR, and was fixed that evening. Direction: Aligned. Size: 1,355 production lines (additions + deletions, excluding 1,168 test lines and 114 fixture lines). Touches Approach: The scope feels right — each fix is the minimum change that closes its specific hole. The Moving on to code review. 🔍 中文说明感谢贡献！（re-run） 模板： PR 正文未使用模板要求的标题（用了 问题： 已观测，非理论性。PR #6486 上丢弃的 Ctrl+F 双重触发阻塞项有时间戳、字符位置和完整失败链。四个洞都有具体证据——不是"理论上可能发生"，而是在真实 PR 上真实发生了。 方向： 对齐。 规模： 1,355 生产行（排除 1,168 测试行和 114 fixture 行）。触及 方案： 范围合理——每个修复都是关闭对应洞的最小改动。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
Code ReviewIndependent proposal (before reading the diff): Given the PR's description — four holes in Comparison: The PR's implementation is thorough and correct. No correctness bugs, security holes, or regressions found. Key strengths:
Reuse check: No critical blockers. VerificationUnit TestsTypeCheck & LintTmux Real-Scenario TestingBefore/after is not applicable in the traditional sense — this PR changes internal review pipeline subcommands, not user-facing CLI behavior. Verified the new test-efficacy --help (new subcommand): presubmit --help (existing subcommand, unchanged interface): CI StatusGitHub Actions on this PR: 中文说明代码审查独立方案（读 diff 前）： 根据 PR 描述—— 对比： 实现完整正确。无正确性 bug、安全漏洞或回归。 无关键阻塞项。 验证
— Qwen Code · qwen3.7-max |
Collaborator
|
This PR fixes four real, observed holes in Reflection: My independent proposal matched the PR's approach, and the implementation exceeded it in several dimensions: the bilingual negation window, the per-file probe classification, the symlink-safe The PR is large (1,355 production lines) but the scope is justified — each fix is the minimum change that closes its specific hole. The four-commit structure (222 → 230 → 237 → 250 tests, each independently green) makes The author has already filed follow-up issues (#6828 for minor cleanup, #6832 for an architectural improvement to the worktree-safety approach), which shows good hygiene in separating what must land now from what can iterate. CI is green (ubuntu tests pass, typecheck clean, eslint clean). The tmux verification confirms both subcommands are accessible and their interfaces are correct. Verdict: LGTM. The implementation is thorough, well-tested (101 unit tests), and solves a real problem that directly undermines the trustworthiness of automated review. Approving. 中文说明本 PR 修复了 反思： 我的独立方案与 PR 方案一致，实现在多处超越预期：双语否定窗口、按文件 probe 分类、符号链接安全的 PR 较大（1,355 生产行）但范围合理——每个修复都是关闭对应洞的最小改动。四 commit 结构（222 → 230 → 237 → 250 测试）保证 bisect 安全。 作者已提交后续 issue（#6828 小清理、#6832 架构改进），体现了良好的工程素养。 CI 绿色（ubuntu 测试通过，typecheck clean，eslint clean）。tmux 验证确认两个子命令可访问且接口正确。 结论： LGTM，批准合并。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
previously approved these changes
Jul 13, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
wenshao
force-pushed
the
feat/review-blocker-and-test-efficacy
branch
from
July 13, 2026 04:34
eb3514d to
8fb9084
Compare
Contributor
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
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.
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.
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.
…new code? 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.
wenshao
force-pushed
the
feat/review-blocker-and-test-efficacy
branch
from
July 13, 2026 04:45
8fb9084 to
44d5763
Compare
wenshao
commented
Jul 13, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
— Codex $qreview via Qwen Code /review
| ]), | ||
| ); | ||
| const body = fullCommentBody(root.body, root.id, ctx); | ||
| if (spent + body.length <= BLOCKER_SECTION_BUDGET) { |
Collaborator
Author
There was a problem hiding this comment.
[Critical] This is not a rendered-output ceiling: the admission check uses raw body length, quote prefixes add per-line overhead, the preamble is uncharged, and headings/references/replies continue after exhaustion. A reproduced accepted body put its tail beyond character 25,000. Budget the exact rendered strings and enforce a hard cap with bounded overflow fetch notices.
— Codex $qreview via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Partially real, but the headline mechanism is contradicted by the code. The admission gate is on the rendered quoted string (pr-context.ts:505-506), not raw body length; > prefixes are inside quoted; and the preamble is charged (let spent = out.join('\n').length at line 475, the very line this anchors to). I could not reproduce "accepted body past character 25,000" — full-rendered bodies top out ~11.5k and the spent + quoted.length <= 16000 gate provably bounds the tail to ~16.6k. The genuine narrow gap — replies are appended after the gate without their own budget — needs an exceptional shape (6 roots × 20 replies) to push a blocker announcement past 25k; at realistic 1-3 blocker threads it stays well under. Tracking as a Suggestion, not Critical.
| * these patterns are tight rather than generous. | ||
| */ | ||
| const BLOCKER_PATTERNS: RegExp[] = [ | ||
| /\[critical\]/, // the marker /review itself emits |
Collaborator
Author
There was a problem hiding this comment.
[Suggestion] The Critical marker matches inside Markdown quotations or code, so “Fixed in abc123” followed by a quoted old Critical is promoted and counted as live. Ignore markers inside quotes/code/fences, or require the marker to begin a top-level finding line.
— Codex $qreview via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Real but narrow: a quoted old [Critical] inside a "Fixed in abc123" reply can promote. The mitigation is the same family as the negation guard — ignore markers inside > quotes / code fences. Worth a follow-up; the promoted thread still goes through the Step 6 re-check which rules on the code, so a stale quote surfaces as fixed by this diff rather than a false blocker.
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`.
…omment 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.
wenshao
commented
Jul 13, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
Comment on lines
+520
to
+521
| if (spent + quoted.length <= BLOCKER_SECTION_BUDGET) { | ||
| out.push(...charge([quoted, ''])); |
Collaborator
Author
There was a problem hiding this comment.
[Suggestion] Budget admission gate undercounts by 1 per admission. The gate checks spent + quoted.length <= BLOCKER_SECTION_BUDGET, but the actual charge is [quoted, ''].join('\n').length = quoted.length + 1 (the empty string adds one \n separator). Each admitted body overshoots by 1 char.
Concrete cost: With ~4 charge calls per promoted root, the drift is ~4 chars per root. On a 16000-char budget with 3 promoted roots, total drift is ~12 chars (0.075%). Practically nil, but the comment says "Everything this section emits counts against the budget" — the accounting overstates what it delivers.
Suggested fix: Tighten the gate to match:
if (spent + quoted.length + 1 <= BLOCKER_SECTION_BUDGET) {— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Real but a 1-character accounting slack per admission (the gate checks quoted.length, the emit is quoted.length + 1 for the trailing newline). The budget is 16 000 against a 25 000 read window, and last round's measurement put every accepted body's tail at ~16.6k — the off-by-one does not move that. Leaving as-is rather than complicating the arithmetic for one char; noted.
| // overhead the `charge()` comment above exists to close. | ||
| let spent = out.join('\n').length; | ||
| const charge = (lines: string[]): string[] => { | ||
| spent += lines.join('\n').length; |
Collaborator
Author
There was a problem hiding this comment.
[Suggestion] charge() systematically undercounts by 1 per call (inter-group separator). spent += lines.join('\n').length counts intra-group separators, but when lines are spread into out via push(...lines), the \n between the previous out tail and the first new element is never charged. Each non-empty charge() call undercounts by exactly 1 char.
Concrete cost: On the live #6486 thread with 3 promoted roots and ~4 charge calls each, total drift is ~15-18 chars on a 16000-char budget (0.1%). Noise, not a bug — but paired with the budget-gate undercount above, the accounting drift compounds.
Suggested fix:
const charge = (lines: string[]): string[] => {
if (lines.length > 0) spent += 1 + lines.join('\n').length;
return lines;
};— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Same 1-char inter-group separator slack as above — the section stays well inside the read window. Noted as a precision nit, not fixing for one character.
|
|
||
| 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. |
Collaborator
Author
There was a problem hiding this comment.
[Suggestion] Agent 7's brief includes a forward reference to Step 7's presubmit report, which doesn't exist when Agent 7 runs in Step 3. The instruction says: "Cross-check it against ciStatus.skippedCheckNames from Step 7's presubmit: a test that runs in neither place gates nothing, anywhere." Agent 7 runs during Step 3A/3B; the presubmit report is generated in Step 7.
Concrete cost: The Agent 7 subagent either silently skips the cross-check (losing the synthesis) or attempts to run presubmit out of order (producing stale results). The cross-check already happens correctly at the orchestrator level in Step 7 (line 843-849 of SKILL.md), making this Agent 7 instruction redundant and misleading.
Suggested fix: Remove the forward cross-check from Agent 7's brief. Replace with: "The orchestrator cross-checks unreachable findings against ciStatus.skippedCheckNames in Step 7."
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fair structural note — Agent 7 runs in Step 3 and the presubmit report is a Step 7 artifact. The cross-check is aspirational ("confirm it runs in CI"), not a hard data dependency, but the wording should not imply the file exists yet. Noted.
wenshao
commented
Jul 13, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
— Codex $qreview via Qwen Code /review
| (r) => (r.name ?? '').endsWith(`/${file}`) || r.name === file, | ||
| ); | ||
| const assertions = result?.assertionResults ?? []; | ||
| const failed = assertions.filter((a) => a.status === 'failed').length; |
Collaborator
Author
There was a problem hiding this comment.
[Critical] assertionResults[].status === 'failed' is not proof of an assertion mismatch: Vitest also marks tasks failed for hook errors, timeouts, and runtime exceptions. A revert that breaks setup can therefore label an inert test gated and suppress the finding. Inspect failure details and return inconclusive unless the failure is demonstrably an assertion failure.
— Codex $qreview via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Real but narrow, and the direction is fail-safe — tracking as a follow-up, not fixing here. You're right that vitest marks a task failed for hook/timeout/runtime errors too. But that pushes toward gated, which is the conservative verdict: it can only suppress an inert Suggestion (never a blocker), so the failure mode is under-reporting cry-wolf noise, not missing a bug. Inspecting failure details to demote hook-only failures to inconclusive is a good refinement; noted.
| // — but the latter is listed explicitly, and negation is handled below. | ||
| return true; | ||
| } | ||
| return norm === g || norm.startsWith(`${g}/`); |
Collaborator
Author
There was a problem hiding this comment.
[Critical] Valid npm workspace patterns beyond trailing /*—for example packages/**, brace globs, or extglobs—are treated as literal prefixes. Tests inside workspaces npm actually collects are then emitted as pre-confirmed unreachable findings, contrary to the documented conservative policy. Use npm-compatible glob matching or treat unsupported positive patterns as reachable.
— Codex $qreview via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Real scope limit, low severity — follow-up. isWorkspaceMember handles trailing /*, explicit paths, and ! negation (npm's common forms) but treats packages/**, brace, and extglob patterns as literal prefixes. A test in such a workspace could be mislabeled — but the finding is a [test] Suggestion, never a blocker, and the code errs toward REACHABLE. Full npm-glob semantics is a worthwhile follow-up; noted.
| 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 `<commit>`"-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 `<commit>`"-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. |
Collaborator
Author
There was a problem hiding this comment.
[Suggestion] This promises that every blocker body, whatever its wording, appears here in full. In practice recognition is a finite floor, open inline and review-summary blockers stay elsewhere, and bodies can degrade to fetchable snippets under caps. Describe the exact promoted sources and tell reviewers to follow emitted fetch notices.
— Codex $qreview via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fair — the SKILL.md line overpromises ('every blocker body appears here in full'). Recognition is a finite floor, and open-inline / review-summary blockers live in their own sections. Will qualify the claim.
| }); | ||
| }); | ||
|
|
||
| describe('parseNdjson (the paginated check-runs decode)', () => { |
Collaborator
Author
There was a problem hiding this comment.
[Suggestion] These tests exercise only the NDJSON decoder. They remain green if ghApiAllNested drops --paginate or uses the wrong jq key—the exact integration that prevents first-page-only CI classification. Add a test that captures and asserts the spawned gh api --paginate ... --jq '.check_runs[]' arguments.
— Codex $qreview via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Correct — the parseNdjson tests cover the decoder but not that ghApiAllNested actually passes --paginate and the right jq key. That integration (the thing that prevents the first-page-only bug) is exactly what my earlier attempt got wrong with a non-existent --slurp. An args-assertion test would lock it; noted as a follow-up (the real gh spawn is otherwise covered by the command's own runs).
…n-blocker 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.
…r revert
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: `(?<!非)` suppressed `除非` ("unless", a real blocking condition);
the adjacency-only guard missed `并非一个阻塞项`; the comma I added to the
stop-set broke the coordinated list "No blocking, must-fix, or critical". The
window now scans a negation word within ~40 clause chars, RESETS at an
adversative (`but`/`但`) but not a bare comma, and breaks at `;`/`:`. Verified
on an 11-case matrix and the 38-comment corpus: recall 2/2, false positives
5/36 (down from 6). Patterns are now bare, negation is one mechanism.
- gh: `parseNdjson` is strict by default and `ghApiAllNested` uses strict. Last
round I made it lenient to tolerate a `gh` update notice, but silently
dropping a malformed check-runs line could hide a *failing* run — the
fail-open the pagination fix closed. Leniency is now an explicit opt-in.
- test-efficacy: the revert set excludes fixture DIRECTORIES, not non-code
extensions. The extension whitelist also dropped runtime-loaded sources a
test gates — an executable `SKILL.md`, a settings-schema JSON — so a
skill-only change produced no probe. Directory is the right discriminator.
- test-efficacy: the dirty-worktree guard adds `--ignored`, so a gitignored
revert-set path recreated locally is not read as clean and overwritten.
Collaborator
Author
Follow-ups tracked in #6828 — ready to landThanks for the thorough multi-round review (qwen + Codex
Two findings were verified false and rejected: a claimed ReDoS that measured 0–2 ms, and a dedup-conflation that could not be reproduced. The 15 review threads still open are deliberate follow-ups — real-but-narrow robustness gaps, test-hardening, and doc-accuracy items, none a blocker. I've collected them with per-thread links, severity, and a fix sketch in #6828 so they have a home and this PR can close rather than keep growing (the last rounds were increasingly spent reviewing code the PR had just added). Each open thread maps to a bullet there. |
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
previously approved these changes
Jul 13, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
commented
Jul 13, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Reviewed — no new blockers. Suggestions are inline.
Existing Criticals re-checked against this commit: 2 still stand (blocker promotion applies only to replied roots — open Critical threads remain as 240-char snippets; classifyProbeRun ignores Vitest's file-level status, so an afterAll failure with passing assertions classifies as inert). 5 fixed by this diff (negation stop-set expanded, budget now charges overhead, check-run name dedup via executedNames, glob ordering uses single-pass last-match-wins, skipped-CI assessment documented in Step 7). 1 not applicable (probe runs after Step 3 agents). 2 cannot tell (neutral in NOT_RUN_CONCLUSIONS — the test suite pins this but the rationale is unclear; worktree-relative paths in the test-efficacy command example depend on orchestrator cwd).
— qwen3.7-max via Qwen Code /review
| const existsAtHead = (cwd: string, path: string) => | ||
| existsAtRev(cwd, 'HEAD', path); | ||
|
|
||
| async function runTestEfficacy(args: TestEfficacyArgs): Promise<void> { |
Collaborator
Author
There was a problem hiding this comment.
[Suggestion] runTestEfficacy is declared async but every I/O operation inside — readFileSync, writeFileSync, spawnSync, git(), gitOut(), rmSync — is synchronous. Zero await expressions in the function body. The async keyword wraps the return in a needless Promise and misleads callers into expecting async I/O.
Failure scenario: no runtime failure, but the signature creates false reassurance for future contributors who may add genuinely async work assuming the function was designed for it.
Suggested change
| async function runTestEfficacy(args: TestEfficacyArgs): Promise<void> { | |
| function runTestEfficacy(args: TestEfficacyArgs): void { |
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fair — runTestEfficacy is async with no await (every op is sync). Harmless (the handler awaits it and the signature stays uniform with the other subcommands), so I've left it; happy to drop the keyword if you'd prefer. Noted.
yiliang114
previously requested changes
Jul 13, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
The main direction looks right, but the current head still has a CI-status pagination hole that can turn a failing commit into all_pass. I left the new findings inline and followed up on three existing threads with concrete reproductions; the status pagination issue is a must-fix before merge.
yiliang114
dismissed
their stale review
July 13, 2026 13:46
Severity recalibrated after checking repository usage: the legacy-status pagination issue is real but pre-existing and not reachable in current Qwen Code CI, so it is non-blocking rather than P1.
…path guard
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.
yiliang114
requested changes
Jul 13, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the latest head. I found one new blocker in the test-efficacy restore path: cleanup can delete files outside the review worktree through PR-controlled symlinks. Details inline.
…led symlink
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.
6 tasks
Collaborator
Author
Architectural follow-up filed: #6832The two worktree-safety findings on this PR share one root cause — the efficacy probe reverts source in place in the shared review worktree and restores it afterward — and one refactor retires both:
#6832 proposes running the probe in its own disposable The point-fixes on this PR make every known P0/Critical-severity defect safe today; #6832 is the design-level fix, better suited to its own PR than to more rounds here. |
yiliang114
reviewed
Jul 13, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
[Suggestion, non-blocking] classifyProbeRun only inspects assertionResults[] — it never reads the file-level status field. When an afterAll hook fails, Vitest sets status: "failed" at the file level but produces no individual failed assertion. The classifier sees failed === 0 and returns inert — which is the wrong verdict for a real teardown failure. Adding a check like if (result.status === 'failed' && failed === 0) → 'gated' would close the gap.
yiliang114
approved these changes
Jul 13, 2026
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jul 13, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
pull Bot
pushed a commit
to edisplay/qwen-code
that referenced
this pull request
Jul 13, 2026
QwenLM#6836) The probe used to revert the PR's source to base IN the shared review worktree and restore it in a `finally`. That shared tree is the one every Step 3 review agent reads, and the in-place mutate/restore was the root of two findings on QwenLM#6790: - a concurrent reader could observe the tree half-reverted to base for the probe's whole duration (Critical), and the later restore cannot un-produce a finding written from the wrong source; - the restore's in-place delete followed a PR-controlled symlink out of the tree and removed an outside file (P0, band-aided with `safeRmWithin`). Both share one cause — mutating a live, shared tree — and one fix retires both. The probe now runs in its OWN disposable worktree, checked out at the PR head as a sibling of the shared one (`.qwen/tmp/review-pr-<n>-probe`) and removed wholesale when it finishes: - the shared tree is never touched, so no reader can see a reverted state; - there is no in-place restore, so the delete that followed a symlink is gone with it — `safeRmWithin` stays only as belt-and-suspenders on the revert-phase delete of added files; - `node_modules` needs no per-tree install: the probe tree is nested under the repo, so `npx vitest` resolves upward to the repo-root `node_modules`, exactly as the shared worktree already does. (Confirmed empirically before relying on it — this is what had the refactor deferred.) Because the shared tree is no longer mutated, the dirty-worktree guard is gone (nothing the caller has uncommitted is ever discarded), and the loud `restoreFailure` / non-zero exit becomes a soft `cleanupFailure` warning: a leftover probe worktree does not corrupt anything and is swept at the next run's `worktree add` and by `cleanup.ts`. Verified by driving the real handler (new `test-efficacy.integration.test.ts`, real git worktrees, a stub vitest bin): verdicts are unchanged (gated/inert), the shared tree is byte-identical before and after, the probe tree is always discarded, and the symlink P0 repro leaves the outside file intact WITHOUT `safeRmWithin` having to refuse — isolation alone protects it. Closes QwenLM#6832.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
/reviewsubmitted "Reviewed — no blockers" on PR #6486 at commit5ede0f3a2, three hours after a maintainer had filed a liveCtrl+Fdual-fire blocker on that same PR. The fix did not land until that evening. This is precisely the "dropped blocker" failure the Step 6 re-check exists to prevent — and it was structural, not a lapse of judgment.Digging into why produced four separate holes, each fixed here in its own commit.
1. A maintainer's blocker rendered as an endorsement
pr-contextpromoted a thread into the mandatory re-check section only if its body contained the literal string[Critical]— a marker only/reviewitself emits. A human types whatever they type.The 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:That reads as an endorsement, filed under a heading that says not to re-report it. The blocker itself began 1 143 characters past the cut.
Recognition is now semantic (
carriesBlockerSignal) and matches assertion patterns, not word presence, with a bilingual 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. Run against 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 fixesheading, 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 43 094 — past the 25 000 oneread_filereturns. The section held the right blocker and no agent could read it: PR #5738's failure, reintroduced one section further down by the fix for it.So the section is now written first, ahead of the description and the review history — nothing in that file outranks the claims a
C=0verdict may not be reached without ruling on. On the live thread that moved the heading from character 25 961 to 569, and the blocker body from 43 094 to 4 421. A character budget bounds the section even so; bodies past it degrade to snippets naming their exact fetch, not to silence.2. "fixed by this diff" was the verdict with no cost
The re-check has three verdicts, and only two of them cost anything:
still standsREQUEST_CHANGES— blocks the mergecannot tellCOMMENTfixed by this diffAn agent choosing among three answers where one is free drifts toward the free one — and the free one is the only one that can ship a bug.
Its bar also read "you read the lines and the fix is there", which invites reading the diff's lines. That is the reading that fails. On #6486 the author answered the 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 aKeypressContext.broadcast()with no stop-propagation — returning from one subscriber does not stop the other.Determinism owns the evidence, judgment owns the ruling:
pr-contextextracts the evidence. A blocker's body names the code it is about; every promoted blocker now renders a Referenced code list. "Go read the untouched code" stops being a hope and becomes a list the agent is handed.cannot tell, which already caps the event.3. A skipped CI check was scored as a passing check
GitHub reports a skipped job as
status: completed, conclusion: skipped. The classifier tested for failure conclusions and pending statuses;skippedmatched neither and fell through intoall_pass.This is load-bearing:
/reviewtreats green CI as its licence to approve, and the design delegates runtime truth to CI because the LLM pipeline reads code statically. On #6486 the one job that would have exercised the new hotkey —Integration Tests (CLI, No Sandbox)— was skipped, as were the macOS and WindowsTestlegs.all_pass."Did it run" is a question about the check name, not any single run: this repo's routing workflows 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.
Two deliberately different consequences:
4.
qwen review test-efficacy— does the new test gate the new code?Agent 5 asks whether a test exists. Agent 7 reports that the suite is green. Neither can see a test that protects nothing, and there are two ways to ship one:
#6486 shipped both, in one file. The new test lived in
integration-tests/, which is not an npm workspace, sonpm test --workspacesnever 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 noticed. 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.Both questions are decidable without judgment, so they are a subcommand and not a prompt. Unreachability needs no execution at all. Inertness needs one run: revert the diff's source 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.
gatedrequires a real assertion failure; anything else that is not a clean pass isinconclusive, andinconclusiveis never reported as a finding.Verification
Every claim above was measured, not assumed.
Driven against real PRs with two built bundles (base vs this branch), same live GitHub data:
qwenOAuth2.ts+ its test) —GATED: 9 assertions fail with the source reverted. Zero findings. The negative control: the tool does not cry wolf.GATED.pr-contexton the live feat(cli): Add model toggle hotkey (Ctrl+F) #6486 thread — base arm reports0 replied Critical(s)and the stringdual-firesappears zero times in the context file; this branch promotes 3 and puts the blocker at character 4 421.presubmiton feat(cli): Add model toggle hotkey (Ctrl+F) #6486's head — base arm has noskippedCheckNamesfield at all and silently reportsall_pass; this branch names the three checks that never ran.Dogfooded with
/review --effort highon this very changeset, twice. The first run returned Request changes (2 Critical, 8 Suggestion) — and it was right:test-efficacycrashed and discarded its entire report on any PR that adds a source file (git checkout <base> -- <newfile>fails withpathspec did not match). Never fired on fix(core): emit auth URL as OSC 8 hyperlink instead of hard-wrapping #6433/feat(cli): Add model toggle hotkey (Ctrl+F) #6486 because those only modify existing files.downgradeApproveomitted theno_checkscase, so the "CI did not run" reason was written and the downgrade never fired — a fix that is in the diff, looks right, and does nothing. Which is the bug class this PR is about.review.test.tswere red and had never been run, because that file lives outside the directory the suite was being invoked on.The second run, after fixing those, returned Comment — no blockers, with none of the first round's findings recurring. Its 13 suggestions are all addressed here, including a real hole it found in the fix from round one: the signal list is bilingual (
阻塞项) but the negation guard was English-only, so没有阻塞项— the Chinese half of the triage bot's own template — promoted on every PR, on a repo whose discussion is substantially Chinese.Gates: 250 tests across 15 files; each of the four commits is independently green (222 / 230 / 237 / 250), so
bisectcannot land on a red tree.eslint,prettier,tsc --noEmitclean. Key guards are mutation-tested — raising the section budget, dropping the CJK negation clause, or removingneutralfrom the not-run set each turns its test red.🇨🇳 中文说明
/review在 PR #6486 的5ede0f3a2上发出了 "Reviewed — no blockers",而三小时前维护者刚在同一个 PR 上贴出一个活着的Ctrl+F双触发阻塞项(修复当晚才落地)。这正是 Step 6 复核门禁存在的唯一理由,而它没兜住 —— 原因是结构性的,不是判断失误。追下去发现四个独立的洞,每个一个 commit:
维护者的阻塞项被渲染成了背书。
pr-context只认字面量[Critical](只有/review自己会发这个标记)。那条评论是 issue 评论,被压成 240 字符片段沉进"已讨论·不要重复报告"区 —— 而那 240 个字符正好是报告开头"我从源码构建并驱动了真实 CLI 做合并前验证",读起来像背书;阻塞项正文在 1143 字符之外。现在改为语义识别(断言模式,不是词出现)+ 双语否定守卫,阻塞项全文渲染并放在文件最前面。fixed by this diff是三个裁决里唯一没有代价的那个 —— 静默、免费、不留痕。而它的门槛写成"读到那几行、修复在那儿",引导人去读 diff 里的行。feat(cli): Add model toggle hotkey (Ctrl+F) #6486 的作者加了个 guard,在 diff 里、看着对、完全无效(第二个 handler 在 diff 从没碰过的text-buffer.ts:2663)。现在要求:说出机制、说出现在是什么挡住了它;挡住的条件在 diff 之外就必须去读,否则是cannot tell。被 skip 的 CI check 被当成通过的 check。
skipped直接落进all_pass。feat(cli): Add model toggle hotkey (Ctrl+F) #6486 上唯一能跑到这个功能的 job 正是被 skip 的。新增
test-efficacy子命令 —— 判定新测试是否真的守护了新代码:unreachable(测试命令根本收集不到)和 inert(把源码回滚后测试照样绿)。feat(cli): Add model toggle hotkey (Ctrl+F) #6486 一个文件同时占了两条。验证: 在真实 PR 上双跑(base vs 本分支);用
/review --effort high对本改动自审两轮 —— 第一轮报出 2 个 Critical 且都属实(其中一个正是"降级理由写了、降级布尔量没设"的哑弹,恰是本 PR 主题的翻版),修复后第二轮 0 阻塞项。250 个测试,四个 commit 各自独立可测。