feat(review): redefine medium effort as a balanced verified pass - #7733
Merged
Conversation
The old medium was a thin inline Step 3C walk: no subagents, no build/test, no verification, no comment-status — cheaper than high but structurally unable to catch a whole class of bugs. Dogfooding measured it missing real Criticals that high caught, including a compile error CI had already flagged red (medium never built, so it could not see it). Redefine medium as a balanced, verified pass — the high pipeline with its most expensive passes removed: - Runs the Step 3A/3B fan-out over a reduced dimension set — issue fidelity (Agent 0), correctness (1a/1b/1c), security (Agent 2), quality (Agent 3), performance (Agent 4), test coverage (Agent 5), and build & test (Agent 7). Skips the adversarial personas (6a/6b/6c) and the Agent 8 diff-specialists. - Loads project rules (Step 2) and runs comment-status like high. - One verification pass (Step 4). Skips the reverse audit (Step 5), the incremental cache, and PR posting (--comment still forces high). - On a large diff, caps the 3B territory fan-out by re-running plan-diff with a coarser --max-chunk-lines. Measured against high on the same PR it lands at roughly one-third to one-half the time and tokens. It reliably catches mechanical defects (compile errors, failing tests — build-test is deterministic) and obvious correctness bugs, but is not an exhaustive correctness audit: a subtle Critical that only the reverse audit or the adversarial personas would surface can slip, so security-sensitive and pre-release reviews should still use --effort high. Because it runs no reverse audit, compose-review caps a clean medium verdict at Comment; a verified Critical still yields Request changes. low (thin inline pass) and high (full pipeline) are unchanged, as are the effort defaults (high for PRs, medium for local) and parse-args. SKILL.md only.
wenshao
requested review from
LaZzyMan,
tanzhenxin and
yiliang114
as code owners
July 26, 2026 00:42
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
Collaborator
|
Re-running on the updated branch — now five commits: the four substantive ones plus a Template: the body carries everything the template exists to elicit — a clear "what", a measured "why" (the #7709 / #7632 comparison table), and a testing section — but under its own headings ( Problem: real and evidenced. Old medium was a thin inline pass structurally unable to catch a whole class of bugs — it missed the #7709 TS2345 compile error entirely because it had no build step. This isn't theoretical hardening; the PR demonstrates the gap on real PRs. Direction: aligned. A medium tier that actually catches bugs at a fraction of high's cost is a genuine improvement to the review tooling, and it stays honest about what it can't do (no reverse audit → never certifies Approve). No auth / sandbox / model-selection / telemetry / release surface is touched. Size: not a core-infrastructure change in the gating sense. The only Approach: the design is the right one, and simpler than the alternative. Recording Moving on to code review. 🔍 中文说明在更新后的分支上重跑——现在五个 commit：四个实质 commit 外加一次 模板： 正文包含了模板想要的全部内容——清晰的"做什么"、有实测支撑的"为什么"（#7709 / #7632 对比表）、以及测试章节——但用的是自己的标题（ 问题： 真实且有证据。旧 medium 是一次很薄的 inline 走查，结构上抓不到一整类 bug——它完全漏掉 #7709 的 TS2345 编译错误，因为没有 build 步骤。这不是理论性加固；PR 在真实 PR 上展示了这个缺口。 方向： 对齐。一个真能抓 bug、成本只有 high 一小部分的 medium 档，是对审查工具的实质改进，且对自己做不到的事保持诚实（不跑 reverse audit → 永不认证 Approve）。未触及 auth / sandbox / model-selection / telemetry / release 面。 规模： 按门禁意义不算核心基础设施改动。唯一的 方案： 设计正确，且比替代方案更简。把 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
Code ReviewReviewed at Prior finding #1 —
I traced the wiring end to end: Prior finding #2 — the 3B coarsening instruction clobbered the plan — fixed. SKILL.md no longer tells a medium review to re-run No new blockers. The Non-blocking nits: the PR body still heads the code section "What changed (SKILL.md only)" though 13 TypeScript files change (carried over from the prior pass); and the body uses non-template headings (see Stage 1). Files changed (14 of 14)
Test evidence — the PR's own CI⛔ Per the static-review rule I did not run any PR code; this is the CI signal read via the API. The head moved to On the prior head That is registry-side infra noise — the npm audit endpoint is being retired and returned a 400. This PR changes no dependency files (no not verified: the unit tests have not produced a green run for this PR's code on either head — on Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 中文说明代码审查在 PR head 的独立 worktree 中、于 上一轮发现 #1—— 上一轮发现 #2——3B 粗化指令把 plan 改坏——修好了。 SKILL.md 不再让 medium 审查重跑 无新阻塞。 非阻塞小问题：PR 正文仍把代码章节标题写作 "What changed (SKILL.md only)"，尽管改了 13 个 TypeScript 文件（上一轮就提过）；正文用了非模板标题（见 Stage 1）。 测试证据——PR 自己的 CI⛔ 按静态审查规则，我没有运行任何 PR 代码；以上是通过 API 读到的 CI 信号。 本轮开始后，head 移动到了 在先前的 head 未验证：本 PR 的代码在两个 head 上都没有跑出过绿色运行—— — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
|
Confidence: 4/5 — both prior blockers are fixed at the root, the design is cleaner than the patch the last pass asked for, and the tests guard the wiring in both directions; the only thing standing between this and a ship is a green CI run, which the PR's own code hasn't produced yet (the prior head failed on an unrelated npm-audit gate; the current head's suite is still running). Stepping back: the independent proposal I'd have written for "make medium a verified tier without forking the roster logic" is exactly what landed — put the effort in the plan and let every reader derive from it, so "the roster you launched" and "the gate that checks it" can't disagree. The PR matches that and goes a bit further (the by-design Comment-cap disclosure is a nicer touch than just silencing the gap). The honesty is what sells it: the SKILL.md is explicit that medium is sampling, not an exhaustive audit, that it never certifies Approve, and that security-sensitive review still wants high. That's the right altitude for a cheaper tier. What keeps this at 4 rather than 5 is not the code — it's that I can't point to a green test run. On the prior head the ubuntu suite failed before executing, on the retired npm-audit endpoint (infra noise; the PR touches no dependencies); on the current head Verdict: approve, deferred until CI lands green on 中文说明置信度：4/5 —— 两个先前阻塞都从根上修好了，设计比上一轮要求的补丁更干净，测试双向守护了接线；唯一横在它与发布之间的，是一次绿色 CI 运行——而 PR 自己的代码还没跑出过（先前的 head 在一个无关的 npm-audit 门上失败；当前 head 的套件仍在运行）。 退一步看：如果让我独立为"把 medium 变成已验证档、又不分叉 roster 逻辑"写方案，落地的正是我会写的——把 effort 放进 plan，让每个读者从它派生，于是"你启动的 roster"与"检查它的门"不可能分歧。PR 与此一致并更进一步（"设计如此"的 Comment 封顶披露比单纯沉默掉缺口更漂亮）。真正打动我的是它的诚实：SKILL.md 明确说 medium 是抽样、不是穷尽审计，永不认证 Approve，安全敏感审查仍应走 high。对一个更便宜的档来说，这是恰当的高度。 停在 4 而非 5 的不是代码——而是我指不出一套绿色的测试运行。在先前的 head 上，ubuntu 套件在退役的 npm-audit 端点上、在执行前就失败了（基础设施噪声；PR 不动任何依赖）；在当前 head 判定：批准，延迟到 CI 在 — Qwen Code · qwen3.8-max-preview Reviewed at |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
approved these changes
Jul 26, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
requested changes
Jul 26, 2026
The medium (balanced) tier deliberately skips the three adversarial personas (6a/6b/6c), but `requiredAgents()` added them unconditionally in the Step 3A (small-diff) roster. So `check-coverage` found no transcripts for the un-launched personas, flagged them missing, and exited 3 — and the skill says a non-zero exit halts before Step 4. A medium review of any small diff (the common case) would stall at Step 3D unless the model improvised past the gate. The SKILL-only redefinition of medium could not work without this. Thread the effort level through the roster/coverage machinery: - `requiredAgents(plan, effort?)` — at `medium`, do not require 6a/6b/6c in 3A. High (and the default) still require them; 3B is unchanged (personas are folded into the chunk agents there, never required as standalone roles). - `agent-prompt --roster --effort <level>` — builds the roster from the same effort-aware `requiredAgents`, so a medium roster prints only the 9 agents it launches instead of the full 12-then-skip. - `check-coverage --effort <level>` — holds the run to the same reduced set, so a balanced review is not flagged for personas it deliberately did not run. SKILL.md Step 3A/3D now pass `--effort medium` to both commands. Adds a roster test for the medium 3A drop. No behaviour change at high effort.
qwen-code-ci-bot
requested changes
Jul 26, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
[Critical] compose-review.ts not effort-aware: SKILL.md documents compose-review runs at medium effort, but ComposeReviewInput has no effort field, no --effort CLI option exists, coverageFromTranscripts is called without effort (line 293), and verificationGaps always requires reverse audit (line ~448). At medium, the body text falsely claims agents 6a/6b/6c were unreviewed and emits FIX: lines telling the orchestrator to relaunch the personas and reverse audit — the exact work medium was designed to skip. The verdict cap (Approve→Comment) happens to be correct by accident, but the body and FIX output are wrong. Additionally, when this is fixed, the medium verdict cap at Comment will break because it currently relies on the false coverage gaps — an explicit effort-dependent cap is needed.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Triage re-run completed without a new review. The stage comments above were updated with the latest result. View workflow run. |
Collaborator
Author
Review —
|
doudouOUC
previously approved these changes
Jul 26, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
wenshao
commented
Jul 26, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
— Codex GPT-5 via Qwen Code /review
…p medium escalating itself The medium tier threaded `--effort` as a flag to `check-coverage` and `agent-prompt --roster`, but `compose-review` recomputes coverage itself and was never given it — so on every medium run its body disclosed the three personas it deliberately skipped as missing coverage, and its stderr FIX lines told the orchestrator to rebuild the full roster and run a reverse audit, turning the one mandated repair round into a silent escalation back to high. A caller-supplied `--effort` was also a roster the caller could shrink, against the invariant `roster.ts` opens with. Record the effort in the plan at capture time instead. `fetch-pr`, `capture-local` and `plan-diff` take `--effort` and write `plan.effort`; `requiredAgents` reads it from there and the `--effort` flag is gone from `agent-prompt`/`check-coverage`. The roster, `check-coverage` and `compose-review`'s recomputation now read one field and cannot disagree — fixing the personas half for free. For the reverse-audit half, `verificationGaps` reads `plan.effort`: at medium the absent reverse audit is a by-design omission that caps a clean verdict at Comment with an honest disclosure and no FIX line, not a repairable gap. Verify (Step 4) still runs and is still enforced at medium. Also: drop the medium 3B coarsening that re-ran `plan-diff` — on a same-repo PR it fed the diff back through the lightweight path, producing a plan with no worktree metadata that dropped Agents 7/1c and could clobber the fetch report Steps 3D/6/7 read. And correct the prose that still said compose-review runs only at high and that medium findings are unverified. Tests: roster reads plan.effort; plan-diff records it; agent-prompt --roster on a medium plan builds the reduced set (the command-boundary the pure-function test could not reach); compose-review caps a medium verdict at Comment with no reverse-audit FIX line while still requiring the verifier.
…ium coverage path Two review suggestions on the effort work: - The `'low' | 'medium' | 'high'` union was inlined in the capture commands where `parse-args` already exports `ReviewEffort`; import and reuse it so a fourth level cannot drift one copy out of sync. - Add a check-coverage test that reads the effort from the plan: a medium plan whose reduced roster was launched has no missing roles, while the same records under a high plan flag the personas — the integration point the roster unit test cannot reach, and the one whose regression would exit 3 on every medium review.
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. |
qwen-code-ci-bot
approved these changes
Jul 26, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
yiliang114
reviewed
Jul 26, 2026
yiliang114
reviewed
Jul 26, 2026
yiliang114
reviewed
Jul 26, 2026
yiliang114
reviewed
Jul 26, 2026
yiliang114
reviewed
Jul 26, 2026
yiliang114
reviewed
Jul 26, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Review Summary
This PR redefines --effort medium from a cheap inline pass to a balanced verified pipeline — a well-motivated design change. The old medium caught little that low didn't (no security, no test-coverage, no adversarial personas), so upgrading it to "high minus the expensive parts" is the right call.
What's done well
-
Design coherence: The effort is recorded in the plan once (
plan.effort) and read by every downstream consumer (roster, check-coverage, compose-review). No caller flag can shrink the roster — the fail-safe is correct (no effort → full roster). -
Clean subset: Medium is a proper subset of high — it drops exactly three things (adversarial personas 6a/6b/6c, Agent 8 diff-specialists, reverse audit) and keeps everything else (security, test-coverage, build-test, verification). The Comment verdict cap is honest: medium doesn't certify Approve because it never ran the second look.
-
Test coverage: 6 test files with A/B comparisons that prove the mechanism — medium satisfies the reduced roster while the same records under high fail for the missing personas. The compose-review tests verify the verdict cap and the absence of reverse-audit remediation.
-
SKILL.md documentation: Exceptionally thorough — every section that mentions effort is updated, with clear guidance on when to use medium vs high.
Concerns (all P2, non-blocking)
5 inline comments posted, all non-blocking:
- Type safety:
balancedMediumuses a type-unsafe cast to readplan.effort;roster.tshas the same field on a proper interface - Backward compatibility: This is a fundamental semantic change to
--effort mediumthat deserves a release note - Cross-file contract: The mechanism that caps medium at Comment (gap always pushed →
ok: false) is spread acrosscoverage.tsandcompose-review.tswithout a linking comment - Defensive comments: The
loweffort value is silently handled by the!== 'medium'condition — worth a comment - Test gap: No test for unrecognized effort values (would catch accidental widening of the medium condition)
Verdict
The design is correct, the implementation is clean, and the test coverage is thorough. All findings are P2 (non-blocking suggestions). Approve.
yiliang114
approved these changes
Jul 26, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Approve ✅
Well-designed redefinition of medium effort. The key insight is correct: medium as a proper subset of high (minus adversarial personas, diff-specialists, reverse audit) with a hard ceiling at Comment verdict (never Approve). Effort stored in the plan ensures no caller can shrink the roster by omitting a flag.
5 P2 inline comments posted (type-unsafe cast, defensive comment for low effort, release note for semantic change, cross-file contract, missing defensive test). All non-blocking.
Collaborator
Author
|
Thanks @yiliang114 — appreciate the thorough read. On the P2s:
Folding the two small code tidies into a follow-up so this doesn't churn your approval. |
wenshao
pushed a commit
to wenshao/qwen-code
that referenced
this pull request
Jul 26, 2026
Resolve compose-review.test.ts by keeping both the deterministic script-lint gate tests and main's bilingual-recovery block. compose-review.ts / roster.ts / SKILL.md auto-merged (main's QwenLM#7733/QwenLM#7736/QwenLM#7739 work + this branch's gate).
Collaborator
|
Released in v0.21.1. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--effort mediumwas a thin inline pass (Step 3C): no subagents, no build/test, no verification, nocomment-status. Cheaper than high, but structurally unable to catch a whole class of bugs — so its practical value was low. This redefines medium as a balanced, verified pass: the high pipeline with its most expensive passes removed. The goal is a real bug-catching review at a fraction of high's cost, for when high is too slow/expensive — high stays the default for PRs and the tier for exhaustive review.What changed (SKILL.md only)
Medium now:
comment-statuslike high.--commentstill forces high).plan-diffwith a coarser--max-chunk-lines.low(thin inline pass) andhigh(full pipeline) are unchanged, as are the effort defaults (high for PRs, medium for local) andparse-args.Why — measured, not assumed
Driven headlessly against real PRs (
qwen3.8-max-preview), comparing tiers on the same PR:build-test(tsc --noEmit)build-testis deterministic) and obvious correctness bugs. Old medium could not see the feat(web-shell): persist terminal history pagination errors #7709 compile error at all; the new tier catches it mechanically.--effort high.compose-reviewcaps a clean medium verdict atComment; a verified Critical still yieldsRequest changes, so medium flags real blockers — it just never certifiesApprove(only high does).Testing
bundled-skills.integration.test.tsgreen (SKILL.md parses);prettierclean.comment-status+build-test, and produces a verified verdict — none of which the old inline medium did.中文说明
概述
--effort medium原本是一次很薄的 inline 走查(Step 3C):无子 agent、无 build/test、无 verification、无comment-status。比 high 便宜,但结构上抓不到一整类 bug,实际价值很低。本 PR 把 medium 重定义为"平衡、已验证"的一档:即 high 流水线去掉最贵的几遍。目标是在 high 太慢/太贵时,用它做一次真能抓 bug、成本只有 high 一小部分的审查——PR 默认仍是 high,穷尽审查也仍用 high。改动内容(仅 SKILL.md)
medium 现在:
comment-status。--comment仍强制 high)。plan-diff --max-chunk-lines(更粗)给领域扇出封顶。low(薄 inline)与high(完整流水线)不变,effort 默认(PR=high、本地=medium)与parse-args也不变。依据——实测,非假设
用
qwen3.8-max-previewheadless 跑真实 PR,同一 PR 对比各档:build-test(tsc --noEmit)抓到build-test是确定性的)与明显 correctness bug。旧 medium 根本看不到 feat(web-shell): persist terminal history pagination errors #7709 那个编译错误;新档能机械命中。--effort high。compose-review会把干净的 medium 判定封顶在Comment;验证过的 Critical 仍产出Request changes——所以 medium 会标出真 blocker,只是永不认证Approve(只有 high 能)。测试
bundled-skills.integration.test.ts通过(SKILL.md 可解析);prettier干净。comment-status+build-test、并产出已验证的判定——这些旧 inline medium 都没有。