-
Notifications
You must be signed in to change notification settings - Fork 3.1k
feat(review): engage the severity floor early on a sustained convergence signal (#9903) #9938
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,158 @@ | ||||||||||||||
| # Signal-driven early severity floor (#9903) | ||||||||||||||
|
|
||||||||||||||
| Date: 2026-08-24 | ||||||||||||||
| Issue: #9903 — act on the convergence/root-cause-clustering signal earlier | ||||||||||||||
| (auto-drop to `--severity-floor critical`) instead of only printing it. | ||||||||||||||
|
|
||||||||||||||
| ## Problem | ||||||||||||||
|
|
||||||||||||||
| The convergence diagnosis (`packages/cli/src/commands/review/lib/convergence.ts`) | ||||||||||||||
| fires from round 3 when the same files keep producing first-time findings and | ||||||||||||||
| the new-finding rate is not falling, and its own `stem-surface` recommendation | ||||||||||||||
| already names the remedy: drop this PR's reviews to `--severity-floor | ||||||||||||||
| critical`. But nothing consumes that signal. Under `severityFloor: "auto"` | ||||||||||||||
| the floor resolves to critical on a fixed schedule — round 6 | ||||||||||||||
| (`floorResolvesCritical` in `compose-review.ts`) — so rounds 3–5 keep posting | ||||||||||||||
| Suggestions inline at full volume while the body prints the advice. Each of | ||||||||||||||
| those rounds costs a full multi-hour review leg to re-derive the same | ||||||||||||||
| sibling set. | ||||||||||||||
|
|
||||||||||||||
| The issue's triage verdict fixed the shape: implement the **early-floor | ||||||||||||||
| variant** (the hard-gate variant is declined — the diagnosis keeps advisory | ||||||||||||||
| power), as a signal-driven early trigger on the existing `auto` posture | ||||||||||||||
| transition, at the same code site as the round-6 check, with a conservative | ||||||||||||||
| streak threshold and the drop disclosed in the round's post. | ||||||||||||||
|
|
||||||||||||||
| ## Design | ||||||||||||||
|
|
||||||||||||||
| ### The trigger signal | ||||||||||||||
|
|
||||||||||||||
| `volumeNotShrinking` — exactly the signal the `stem-surface` (floor-drop) | ||||||||||||||
| recommendation is matched to. The tool acts on the recommendation it already | ||||||||||||||
| prints, only when it prints it. A clusters-only recurrence with a falling | ||||||||||||||
| new-finding rate is a converging loop and never advances the trigger. | ||||||||||||||
|
|
||||||||||||||
| The measurement is the one the module already makes: `diagnoseConvergence` | ||||||||||||||
| reads FRESH drafts only, so carried-id re-posts of unfixed Criticals (steady | ||||||||||||||
| state) never count — the triage's first correctness constraint is inherited, | ||||||||||||||
| not re-implemented. | ||||||||||||||
|
|
||||||||||||||
| ### The streak | ||||||||||||||
|
|
||||||||||||||
| A new ledger marker field, `flatRounds`: how many consecutive rounds the | ||||||||||||||
| first-time-finding rate did not fall. It advances on a firing round and | ||||||||||||||
| **resets on any non-firing round**. This is deliberately stricter than | ||||||||||||||
| `churnRounds`' carry-on-unmeasured: `churnRounds` arms a blocking Critical | ||||||||||||||
| where late filing loses the mechanism on exactly the churning PRs, while | ||||||||||||||
| `flatRounds` engages a disclosed, non-capping deferral posture where a false | ||||||||||||||
| engagement silently defers real Suggestions (the triage's second | ||||||||||||||
| constraint). A wiped streak costs one delayed engagement — the fail-open | ||||||||||||||
| direction every other input in this module family takes. | ||||||||||||||
|
|
||||||||||||||
| The engagement bar is **2 consecutive rounds** (`FLAT_STREAK_TO_ENGAGE`), | ||||||||||||||
| matching `CHURN_STREAK_TO_FILE`'s stated argument: two counted rounds is the | ||||||||||||||
| shortest window in which "not falling" is an observation rather than a | ||||||||||||||
| single step. In the issue's scenario (signal visible from round 3) the floor | ||||||||||||||
| engages at round 4 instead of round 6. | ||||||||||||||
|
|
||||||||||||||
| ### Engagement and latch | ||||||||||||||
|
|
||||||||||||||
| The floor engages **on the round the streak reaches the bar** — `prevStreak | ||||||||||||||
| === 1` and this round's signal fires — not the round after. Once the recorded | ||||||||||||||
| streak is at the bar it **latches**: later rounds engage on the recorded | ||||||||||||||
| streak alone, and the streak is pinned rather than re-measured. | ||||||||||||||
|
|
||||||||||||||
| The latch is forced by two failure modes of the alternative (re-measure every | ||||||||||||||
| round and stay engaged only while the streak holds): | ||||||||||||||
|
|
||||||||||||||
| - **The measurement dies under the floor.** Enforcement moves fresh | ||||||||||||||
| Suggestions into the deferral channel, so the posted-set trend the signal | ||||||||||||||
| reads goes quiet — not because the loop settled, but because the floor is | ||||||||||||||
| working. Re-measuring would release the floor the round after it engaged. | ||||||||||||||
| - **Circularity through the `floorChanged` guard.** The volume signal | ||||||||||||||
| refuses to compare two rounds that posted under different floors, and the | ||||||||||||||
| floor this round posts under is the thing the trigger is deciding. | ||||||||||||||
| Re-measuring against a pre-trigger floor assumption makes engagement flap | ||||||||||||||
| at period two. | ||||||||||||||
|
|
||||||||||||||
| A latched round measures nothing, so neither failure mode is reachable. The | ||||||||||||||
| latch spans only rounds 4–5 in the typical case — round 6 engages | ||||||||||||||
| unconditionally — and the operator's explicit `--severity-floor suggestion` | ||||||||||||||
| still turns the posture off at any round (the trigger lives only in the | ||||||||||||||
| `auto` arm). | ||||||||||||||
|
|
||||||||||||||
| ### Trust boundaries (mirroring `churnRounds`) | ||||||||||||||
|
|
||||||||||||||
| The marker rides a public, writable review body, so the recovered streak is | ||||||||||||||
| hardened exactly like the churn streak: | ||||||||||||||
|
|
||||||||||||||
| - clamped to the marker's own `round` at parse (`parseLedger`) and at | ||||||||||||||
| side-file read (`prevLedgerFacts`) — a planted `flatRounds: 9999` cannot | ||||||||||||||
| reach the bar off one honest round; | ||||||||||||||
| - a **foreign winner's streak is stripped** at the `pr-context` recovery | ||||||||||||||
| seam — another account's marker cannot engage this account's floor; | ||||||||||||||
| - the side-file carry rules for anonymous recovery mirror the churn group's, | ||||||||||||||
| so the latch does not drop when this account's own marker leaves the walk. | ||||||||||||||
|
|
||||||||||||||
| Worst case for a forged own-account-looking streak: Suggestions move into a | ||||||||||||||
| **disclosed** deferral list for the rest of the PR. Nothing is withheld from | ||||||||||||||
| the record, no verdict is capped, and the operator override disengages. | ||||||||||||||
|
|
||||||||||||||
| ### Disclosure | ||||||||||||||
|
|
||||||||||||||
| A new `CriticalFloorKind`, `'auto-signaled'`, names the early engagement | ||||||||||||||
| everywhere the kind is read back, so the round where Suggestions move at | ||||||||||||||
| round 4 says why instead of presenting an unexplained posture change: | ||||||||||||||
|
|
||||||||||||||
| - the deferral header / floor-enforcement note states the trigger ("the | ||||||||||||||
| first-time-finding rate has not fallen for N consecutive rounds"); | ||||||||||||||
| - the convergence rendering's "already at the floor" wording covers the new | ||||||||||||||
| kind. | ||||||||||||||
|
|
||||||||||||||
| ### Model-side contract (SKILL.md) | ||||||||||||||
|
|
||||||||||||||
| The model cannot evaluate the deterministic trend itself — that is why the | ||||||||||||||
| module exists — so its Step 6 routing follows the **marker**: a recovered | ||||||||||||||
| `flatRounds` at the bar means the floor is critical and Suggestions route to | ||||||||||||||
| the deferral channel. On the round the streak first reaches the bar the | ||||||||||||||
| model still drafts under the old posture and the code backstop | ||||||||||||||
| (`floorEnforcedReroute`) moves the drafted Suggestions — precisely the | ||||||||||||||
| backstop's stated job — and the posted body discloses the move. | ||||||||||||||
|
|
||||||||||||||
| ## Implementation outline | ||||||||||||||
|
|
||||||||||||||
| 1. `lib/ledger.ts`: `Ledger.flatRounds`, serialized beside `churnRounds` | ||||||||||||||
| (above the shed cascade, omitted at zero), parsed with the same | ||||||||||||||
| clamp-to-round. | ||||||||||||||
| 2. `pr-context.ts`: persist/recover `flatRounds` with the churn group's | ||||||||||||||
| seam rules (foreign strip, anonymous carry). | ||||||||||||||
| 3. `compose-review.ts`: | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R1-9: The implementation outline enumerates the per-file edits but never mentions the co-shipped behavior change to the #9410 residual-risk advisory: its floor-engagement conjunct now reads the caller's signal-inclusive enforcement state (compose-review.ts:3514–3520 replaces the old schedule-only re-derivation), and the CLI-glue test 'fires the persistently-critical advisory under a SIGNAL-engaged floor too (#9903)' exists solely for that change. With the signal arm the advisory first fires at round 5 on a signal-engaged PR; the doc's schedule-only model implies it cannot appear before round 7 (round-6 engagement plus the 'previous round posted under the same engaged floor' conjunct), so a maintainer reading this doc derives a first-appearance round two rounds later than shipped code, and any follow-up editing the signal arm or the advisory conjunct has no record that the two are coupled. Add a bullet under the compose-review.ts item recording that the advisory surfaces from the signal-engaged round, not the round the schedule first proves. 中文说明[Suggestion] 实现大纲逐文件列出了改动,却从未提及同船交付的 #9410 残余风险建议的行为变化:其 floor 激活合取项现在读取调用方包含信号的执行状态(compose-review.ts:3514–3520 替换了旧的仅时间表重推导),而 CLI 胶水测试 'fires the persistently-critical advisory under a SIGNAL-engaged floor too (#9903)' 正是为该变化而存在。有了信号臂,该建议在信号激活的 PR 上第 5 轮即可首次触发;而文档的仅时间表模型暗示它不可能早于第 7 轮出现(第 6 轮激活加上"上一轮在同一已激活 floor 下发布过"的合取项),读者按本文档推导出的首次出现轮次比实际代码晚两轮,后续任何修改信号臂或建议合取项的跟进都没有记录表明二者耦合。建议在 compose-review.ts 条目下补一条:该建议自信号激活轮起即可出现,而非时间表首次证明的轮次。 — qwen3.8-max via Qwen Code /review (v0.22.0) |
||||||||||||||
| - `prevLedgerFacts` recovers `flatRounds` (clamped, travels with round); | ||||||||||||||
| - `composeReview` measures this round's `volumeNotShrinking` via the one | ||||||||||||||
| `diagnoseConvergence` statement (pre-reroute — identical to post-reroute | ||||||||||||||
| while the floor is not engaged, since no reroute is in flight), advances | ||||||||||||||
| or resets the streak, pins it when latched; | ||||||||||||||
| - `floorResolvesCritical` gains the signal arm: `auto` resolves critical | ||||||||||||||
| when the streak is at/past the bar — as the new | ||||||||||||||
| `CriticalFloorKind 'auto-signaled'`; | ||||||||||||||
| - the #9410 residual-risk advisory's floor-engagement conjunct reads the | ||||||||||||||
| caller's signal-inclusive enforcement state (`floorEnforcementEngaged`), | ||||||||||||||
| not a schedule-only re-derivation: it surfaces from the signal-engaged | ||||||||||||||
| round, not the round the schedule first proves (round 5, not round 7, | ||||||||||||||
|
Comment on lines
+139
to
+141
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] This bullet says the residual-risk advisory "surfaces from the signal-engaged round", but its own parenthetical — round 5 — is one round after the signal-engaged round. The "Engagement and latch" section above fixes engagement at round 4 in this scenario, and
Suggested change
中文说明[建议] 该条目称残余风险建议项"从信号激活轮起出现"(surfaces from the signal-engaged round),但其自身括号里的数字——第 5 轮——是信号激活轮之后的一轮。上文"Engagement and latch"一节把本场景的激活定在第 4 轮,而 — qwen3.8-max via Qwen Code /review (v0.22.0) |
||||||||||||||
| in the scenario above); | ||||||||||||||
| - the marker stamps `flatRounds` beside `churnRounds`. | ||||||||||||||
| 4. `lib/convergence.ts`: `CriticalFloorKind` gains `'auto-signaled'` with | ||||||||||||||
| rendering wording. | ||||||||||||||
| 5. `SKILL.md`: `auto` floor rule gains the signal-driven trigger; Step 6 | ||||||||||||||
| routes on the marker's `flatRounds`; the deferral disclosure names it. | ||||||||||||||
|
|
||||||||||||||
| ## Explicitly not done | ||||||||||||||
|
|
||||||||||||||
| - **No hard gate.** The "root cause not triaged / consider splitting the PR" | ||||||||||||||
| blocker is the triage-declined variant; the existing churn-census | ||||||||||||||
| non-convergence Critical already owns the blocking lane. | ||||||||||||||
| - **No release-on-settle.** Once latched, the floor stays engaged for the PR | ||||||||||||||
| (round 6 would engage anyway). A self-releasing floor re-opens the | ||||||||||||||
| oscillation and circularity the latch exists to avoid. | ||||||||||||||
| - **No new threshold vocabulary.** Bar = 2, the codebase's one existing | ||||||||||||||
| streak constant's argument, restated — not a tuned number. | ||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] The implementation outline enumerates the per-file edits but never mentions the co-shipped behavior change to the #9410 residual-risk advisory: its floor-engagement conjunct now reads the caller's signal-inclusive enforcement state (compose-review.ts:3514–3520 replaces the old schedule-only re-derivation), and the CLI-glue test 'fires the persistently-critical advisory under a SIGNAL-engaged floor too (#9903)' exists solely for that change. With the signal arm the advisory first fires at round 5 on a signal-engaged PR; the doc's schedule-only model implies it cannot appear before round 7 (round-6 engagement plus the 'previous round posted under the same engaged floor' conjunct), so a maintainer reading this doc derives a first-appearance round two rounds later than shipped code, and any follow-up editing the signal arm or the advisory conjunct has no record that the two are coupled. Add a bullet under the compose-review.ts item recording that the advisory surfaces from the signal-engaged round, not the round the schedule first proves.
中文说明
[Suggestion] 实现大纲逐文件列出了改动,却从未提及同船交付的 #9410 残余风险建议的行为变化:其 floor 激活合取项现在读取调用方包含信号的执行状态(compose-review.ts:3514–3520 替换了旧的仅时间表重推导),而 CLI 胶水测试 'fires the persistently-critical advisory under a SIGNAL-engaged floor too (#9903)' 正是为该变化而存在。有了信号臂,该建议在信号激活的 PR 上第 5 轮即可首次触发;而文档的仅时间表模型暗示它不可能早于第 7 轮出现(第 6 轮激活加上"上一轮在同一已激活 floor 下发布过"的合取项),读者按本文档推导出的首次出现轮次比实际代码晚两轮,后续任何修改信号臂或建议合取项的跟进都没有记录表明二者耦合。建议在 compose-review.ts 条目下补一条:该建议自信号激活轮起即可出现,而非时间表首次证明的轮次。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deferred to the next round under this round's batch bound — accepted as stated, docs-only, and it couples to nothing this round changed, so the wait costs nothing: the implementation outline will gain the bullet recording that the #9410 residual-risk advisory's floor-engagement conjunct reads the signal-inclusive enforcement state and therefore first fires on a signal-engaged round, not the round the schedule first proves.
中文说明
在本轮批量上限下延后到下一轮——按原文接受,纯文档改动,且与本轮改动无任何耦合,等待没有成本:实现大纲将补上这条记录——#9410 残余风险建议的 floor 激活合取项读取的是包含信号的执行状态,因此它在信号激活轮即可首次触发,而非时间表首次证明的轮次。