mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-08-25 00:26:31 +00:00
162213e9da
* feat(review): adopt a round-aware convergence posture for posted findings
Re-reviews of the same PR regenerate non-Critical feedback at zero marginal
cost after every push: the review files findings on code the previous round
just added, the next push implements them, and the diff widens — which
allocates more agents, which file more findings. Measured from the outside:
one managed PR reached +13k lines across 8 review rounds with its per-round
Critical count flat and was closed unmerged; on two others 78-86% of the
growth was test lines. Every existing brake lives in the calling workflow
and covers only bot-managed PRs; a human contributor answering a
push-triggered review round by round rides the same loop with no brake at
all. Convergence therefore has to be a property of /review itself, not of
any orchestration around it.
Teach the reviewer to raise its POSTING bar as rounds accumulate — finding,
verification, the terminal report and the findings artifact are untouched:
- The convergence posture (Step 6): with no flag, Suggestions post through
round 5; from round 6 only Criticals post, and non-Critical findings are
recorded, not requested — one line each into the new `deferredSuggestions`
compose state, which compose-review renders as a disclosed, non-capping
list on every event (nothing is silently dropped, and a deferral never
withholds the incremental anchor). A Critical is never deferred, at any
round under any floor. An APPROVE over a non-empty deferral list opens
"No blocking issues" instead of "No issues found", and the round number
in the clause comes from the same side-file read the ledger marker
stamps, so the two cannot disagree.
- The code-age rule (rounds 2-5): a NEW non-Critical finding anchored on
code unchanged since the previous round's reviewed head defers the same
way — that code was read last round and not flagged, so filing a nit on
it now is re-derivation churn. The age reference is the previous review's
own commit_id, which pr-context now recovers into the prev-ledger side
file: unlike the ledger sha (a range certification, withheld on
fail-closed rounds on purpose) it exists on every posted round, so the
fail-closed full-range re-review — the common case in a bot loop — can
still apply the rule. Validation and the newly-reachable exception mirror
the anchor check; an absent or invalid reference skips the rule, never
the review.
- `--severity-floor <critical|suggestion>` and `review.severityFloor` make
the posture an explicit knob in both directions: `critical` applies it
from round 1, `suggestion` turns it off, and `auto` — the default — is
the round-adaptive rule. Grammar, deferred warnings, non-PR gating and
operator-scope resolution all mirror `--effort`/`--comment`.
* chore(review): regenerate settings.schema.json for review.severityFloor
* fix(review): close the round-1 review findings on the convergence posture
Five verified findings from the PR's own reviews, each with its mechanism:
- The verdict surface now carries the deferrals. A deferrals-only APPROVE
set lowSignal, so verdictLine printed "none of the N review agents
reported a finding" while the same body listed the findings two
paragraphs down — on exactly the posture's canonical end state. lowSignal
is skipped when deferrals exist, ComposeReviewResult gains deferredCount,
and the verdict line names the deferrals instead.
- Deferred findings count toward the verifier-delivery floor. They publish
in the body as the deferral list, and an unverified claim does not become
publishable by being deferred — a deferrals-only run owes a verifier
exactly as a posting run does.
- The deferrable set is narrowed to high-confidence Suggestions that would
otherwise post. Low-confidence and Nice-to-have findings stay
terminal-only: routing them through the deferral list would publish what
the review contract keeps off the PR.
- The code-age rule is hardened on both operands and both premises: the
documented command quotes the PR-controlled path and passes
--literal-pathspecs (an unquoted name executes shell; a glob name ages
the finding against a sibling file's hunks), and age suppression is
skipped for findings in scope the previous round disclosed as not
reviewed — "the previous round saw this code" is false there.
- One side-file read per compose: the deferral clause and the ledger marker
take the same prevRound value, so a mid-compose update can no longer
publish two different round numbers in one review. The deferral list also
caps each entry at 240 chars (twenty 4,000-char entries would push the
body past GitHub's 65,536 rejection line), and the disclosed 20×240 cap
is now stated in the prose the list's survival promise lives in.
Smaller closures from the same rounds: two invalid flag values are two
typos, not a target and a tiebreak (neither becomes a file target); the
--severity-floor warning arms invalid-eq and kept-as-target gain the tests
whose absence a mutation probe demonstrated; the commitId fixture gains a
newer marker-less review so a latest-review-wins mutant fails; the
severityFloor setting joins the settings-dialog membership assertion and
the configured-floor wiring tests; and the /review argument-hint now
advertises the flag.
* fix(review): carry deferredCount through the persisted verdict
The save-artifact validator constructs the persisted verdict from the
composed JSON and gained the field with absent-means-zero semantics: a
composed file written by a build predating the posture must not fail a
save over a count that only affects display, while a present value of
the wrong shape is refused like every other field.
* test(review): pin deferredCount passthrough and the pre-posture default in the saved artifact
* fix(review): close the round-2 findings — deterministic deferrals and the truthful verdict line
- The verifier-delivery floor now excludes deterministic [build]/[test]/
[probe] deferrals by their source tag, the same exclusion body Criticals
get: a pre-confirmed finding never produces a verifier delivery, so
counting it demanded a delivery that cannot exist — the cap never lifted,
the anchor was withheld every round, and the posture's own enforcement
regenerated the full-range re-review loop it exists to end. The deferral
entry format now carries the source tag (SKILL prose + a pin), and the
determinism regex is one shared constant for both scans.
- verdictLine's "(listed in the body)" turns cap-aware past the 20-line
render cap, so a verdict counting 21 no longer certifies a body listing
20; the line caps are module-scoped for the two readers.
- The mutation-demonstrated assertion gaps are closed: deferredCount is
asserted on the REQUEST_CHANGES and COMMENT return sites, and
save-artifact's refuse arm has its wrong-shape cases ('two', -1, 1.5).
* fix(review): close the round-3 findings — source-position tags, a Critical tripwire, and honest age-reference lifecycle
- Deterministic classification of a deferral reads the SOURCE position only
(tag immediately after the entry's first em-dash): a whole-entry scan
classified a finding deterministic off a title that mentioned [test] and
skipped the verifier floor for an unverified claim (probe-confirmed).
- A deferral carrying a Critical marker is refused outright: the channel is
model-written free text that casts no vote on C, and a Critical routed
there composed an APPROVE over a blocker in a probe. Marker forms only,
so prose like "critical-path" passes; artifact-level reconciliation stays
a structural follow-up.
- The prev-ledger side file now carries the winning review's own id, and a
run that recovers no ledger strips a stale file's commitId/reviewId while
keeping the round counter: an age reference the PR's current reviews no
longer vouch for can wrongly defer a finding on code changed-and-reverted
since the true previous round — snapshot diffs are not monotonic over
intervals, which refutes the earlier keep-it-it's-conservative ruling.
The write/strip logic is an extracted, filesystem-tested helper, closing
the serialization blind spot two rounds of review asked about.
- Prose repairs: the code-age rule is auto-only (an explicit suggestion
floor turns it off, as the resolve-floor paragraph already promised); the
fallback names the commitId, not the ledger sha; the not-reviewed check
binds to the review the side file's reviewId names; and the
context-unavailable state skips the age rule (the side file may be a
previous run's). save-artifact's null-deferredCount reads as zero — the
same absence semantics compose-review's toCount gives the field's
siblings — now stated and tested rather than accidental.
* fix(review): close the round-4 findings — licence the deferral channel, harden the side file, age aggregates per location
Code:
- The deferral channel gains its licence check: the resolved severityFloor
rides the compose state, and a non-empty deferral under an explicit
suggestion floor (posture off) or on round 1 of auto (no posture, no age
reference) is refused — the one channel that removes findings from
posting now gets the same deterministic treatment as the counts. Rounds
2-5 under auto stay licensed: the code-age rule defers there, which the
reviewer's proposed round<6 condition would have wrongly outlawed.
- Ledger recovery skips PENDING drafts (the API serves the caller's own
unsubmitted reviews; a crashed run's draft is not a previous round), and
the side file's lifecycle is three-way honest: recovered → written whole
(atomically, temp+rename — a mid-write failure must never leave a
truncated file that parses as no round); reviews read but no ledger for
this account → file REMOVED (another account's round counter must not
stamp this account's first review round N+1); recovery threw → round
counter kept, age-sensitive commitId/reviewId stripped.
- Disposal rule v3: an invalid flag value survives by what it could BE — a
PR-shaped token survives unless a typed target exists (an unrelated typo
must not change WHICH codebase is reviewed), a file-shaped token only as
the sole kept token.
- The per-entry cap backs off a split surrogate pair (zh titles ride the
deferral list untranslated; unit 240 on a high surrogate shipped U+FFFD).
Prose (SKILL):
- Pattern aggregates age per location: ANY changed location posts the
aggregate; deferral only when every location is unchanged and covered.
- The reviewId body is consulted only after fetching a truncated tail (the
8,000-char render cap can hide the very "Not reviewed" disclosure the
age rule depends on); an unreadable body skips the rule.
- context-unavailable resolves the auto floor as round 1: no posture, full
posting, said in the terminal — a posting bar in doubt fails open.
- Deferred findings number under D<round>-<n>, never consuming an R id the
ledger's buildLedger would reassign to a posted sibling.
- The Step 8 record sentence now states exactly what survives where, and
the state contract documents severityFloor.
Tests pin all of it, including the mutation-shown gaps: the marker/clause
round agreement, the exactly-20 verdict-line boundary, the submit-seam
deferral passthrough, and sha survival through the side-file rewrites.
Structural remainders are filed instead of grown: #9176 (typed deferral
channel derived from the findings artifact), #9177 (whole-body byte budget).
* fix(review): close the round-5 findings — carry the floor unresolved, cap unlicensed deferrals, guard the counter
The round-5 reviews caught a shipped design contradiction: the SKILL told
Step 6 to carry the RESOLVED floor into the compose state, so a legal
rounds-2-5 age-rule deferral arrived as the string 'suggestion' — which
the licence check reads as the operator's explicit posture-off override —
and the compose threw, losing the entire round, Criticals included. Two
fixes, one per side of the contract:
- The state carries the verdict's floor UNRESOLVED: auto stays the literal
'auto', and the module licenses it by the round it derives itself. A
SKILL pin makes the sentence load-bearing, and an end-to-end test pins
the legal round-3 shape (side file at round 2, auto floor, deferral →
clean APPROVE naming round 3).
- Unlicensed deferrals CAP instead of throwing: prevRound is a best-effort
side-file read whose every failure mode returns 0, so a missing file at
a true round 6 must degrade to a disclosed, capped, anchor-withheld
verdict — never to no verdict at all. The findings render under a
warning clause; the Critical-marker tripwire stays a refusal (a Critical
rendered as "recorded, not requested" would be the worse outcome).
Adjacent round-5 closures: the tripwire is separator-agnostic (an ASCII
hyphen where the format prescribes an em dash was the cheapest real miss);
side-file removal is gated on a POSITIVELY read non-empty reviews list
(ghApiAll flattens error envelopes to [], and an empty list must not
delete a live round counter and its anchor); the atomic temp name is
per-process so concurrent same-PR fetches cannot rename each other's
bytes, with debris unlinked on a failed rename; two PR-shaped invalid flag
values are refused as ambiguous instead of first-wins; the apostrophe
escaping clause and the unresolved-carry sentence gain SKILL pins; and
review.severityFloor joins the user-facing settings reference.
Declined, recorded on the PR: requiring per-file coverage evidence for the
age rule — the anchor chain already encodes it (an incremental round's
anchor certifies the preceding full coverage; broken links are fail-closed
rounds whose disclosures the rule consults).
* fix(review): close the round-6 Criticals — relocate stray Criticals, license by evidence at hand, one separator grammar
Round 6 tripled the finding count and aimed almost entirely at rounds 4-5's
hardening code — the expansion signal this PR's own posture exists to
answer — so this round lands only the confirmed Criticals and defers the
Suggestion tail on the record:
- A Critical-marked deferral is RELOCATED into the body Criticals instead
of thrown: it counts toward C, the event blocks, and the round posts —
the same doctrine the round-5 fix applied to the licence check, closing
the last channel where one bad entry could cost a composed round (with
real drafted Criticals attached). A lookbehind spares hyphenated
compounds — the SKILL's own "non-Critical findings" phrasing was a
realistic false positive that would have blocked over a nit.
- The two deferral regexes share one separator grammar: the deterministic
classifier now accepts hyphen/en/em like the tripwire, so two spellings
of one [test] finding no longer produce opposite verdicts (the unmatched
form demanded a verifier that cannot exist — the self-inflicted cap).
- The licence closes its evidence gaps: an ABSENT severityFloor beside a
non-empty deferral list is unlicensed (the field ships with the channel;
omission must not silently re-license what an explicit suggestion floor
forbade), auto in the context-unavailable state is unlicensed (the round
is unknowable), and the unlicensed cap now joins the certification
ladder so "Reviewed — no blockers." cannot open a body whose own warning
says findings may be under-posted.
- Deferred entries render Markdown-neutralized (mdField, the budget-gap
rule — model text reaching a public body), carry a truncation ellipsis
when the per-entry cap cut them, and --severity-floor accepts the
documented `auto` spelling while quoted-empty flag values are consumed
as missing instead of becoming empty file targets.
- Prose: the age rule's deferrable set names the same high-confidence
otherwise-postable Suggestions as the floor paragraph (never
low-confidence/NTH); a reviewId body absent because it matched the
canonical LGTM filter is disclosure-free by definition, not unreadable;
and the posture round is the side file's — the cache scopes the diff but
never decides the posture.
The Suggestion tail and the two structural families are recorded, not
grown: #9176 gains the age-evidence and id-timing bullets; #9177 already
carries the whole-body budget. Declines stand where argued (R2-2/R2-15
anchor-chain; sole-file-shaped promotion keeps the forgot-the-level use).
* fix(review): close the round-7 findings — kebab paths, the equals-form ambiguity hole, and the relocated blocker's ledger seat
Round 7 caught a regression the round-6 fix itself introduced, plus one
genuine hole in the round-5/6 ambiguity guard:
- The deterministic classifier anchors on the first WHITESPACE-FLANKED
separator after the leading file:line token. The round-6 negated-class
walk stopped at the first hyphen INSIDE a kebab-case path — this repo's
enforced .ts convention — so the common spelling of a [test] deferral was
misclassified non-deterministic and demanded a verifier that cannot
exist: the permanent self-cap loop, on exactly the entries the exclusion
exists for. Kebab, [build] and [probe] cases pin all three tags on both
separators.
- The PR-target ambiguity pool counts BOTH flag spellings: an equals-form
invalid value never enters the disposal set, so which of two PR numbers
got reviewed depended on which syntax was typed. All four spelling
combinations now land on the same loud refusal, pinned.
- A relocated Critical rides the machine ledger: the split moved into a
shared helper that the body composer and the marker builder both call,
so a mis-routed blocker keeps its id continuity ("the findings always
ride" includes the mis-routed ones).
- The age rule's two diff-output doubt states fail open like every other
arm: a non-matching pathspec (prove it with tree-relative cat-file before
reading the diff's silence) is about the path, not the code, and a
zero-hunk non-empty diff — a PR-controlled .gitattributes binary mark —
is a file-level change; both post. Commands pinned to the worktree root.
- The mutation-shown pin and test gaps are closed: explicit auto override,
quoted-empty consumption on both flags, the invalid-configured-floor
handler seam, the SKILL's validation commands, round-source and
context-unavailable clauses, and the aggregate age clause.
The four re-asserted body Criticals remain tracked (#9176/#9177 and the
two serialization-ordering threads) — their standing disposition is the
round-6 batch record, and their final resolution is the maintainer's
merge decision.
* fix(review): close the round-8 Criticals — one entry grammar, one rescue rule, deletion only on proven absence
Round 8 landed three genuine Criticals, all on rounds 6-7's hardening code,
plus a fourth from a local lane sharing the first's mechanism:
- The deterministic classifier's entry grammar tolerates the shapes the
SKILL itself prescribes: the aggregate `(+N locations)` suffix between
the anchor and the separator, a leading space (entries are trimmed before
the scan; the filter trimmed only for emptiness), and an en dash. Every
one classified a pre-confirmed [test] deferral non-deterministic and
demanded a verifier that cannot exist — the permanent self-cap on the
posture's own stop signal, probe-demonstrated three ways.
- A relocated Critical is classified by that same position-anchored rule,
not the whole-entry tag scan the model's own body Criticals get: a
title-borne [test] in a relocated unverified claim exempted it from the
floor and posted it as a blocking Request changes with no verifier. The
split helper reports the deterministic count and the body composer keeps
the two provenances apart.
- The equals-form PR-shaped invalid value joins the disposal pool exactly
as the spaced form does — the round-7 fix wired it into refusal only, so
`--severity-floor=6711` reviewed the local tree while `--severity-floor
6711` rescued PR 6711. Every spelling converges; the ambiguity pool counts
distinct values, so two spellings of one PR are one candidate.
- Side-file deletion requires PROVEN absence — a walked review list with no
submitted review by this account — never "recovery returned null": an own
review whose marker fails to parse (edited or damaged bot body, marker-
less follow-up) is a persistent state, and deleting there stamped the next
round "round 1" mid-PR and reset the posture clock. Recovery is
three-valued; the middle state strips conservatively.
Adjacent closures: an unrecognised severityFloor (model-transcribed drift
like "Critical" or "auto ") is the unknown state — unlicensed with a list,
inert without one — instead of a refusal that lost zero-deferral rounds
over a field that changed no output; the interface doc says UNRESOLVED
where it said RESOLVED (the round-5 regression's own wording); the
deferrable-set description at four sites names the real set
(otherwise-postable high-confidence Suggestions; low-confidence and NTH
stay terminal-only) and the rounds-2-5 age deferrals; and the mutation-
shown gaps are pinned — pure deferrals stay out of the ledger, a
relocation-only run incurs no licence cap, an invalid configured floor is
silent on non-PR targets, and a 64-hex commit id survives recovery.
* feat(review): type the deferral channel — carry the fields, stop re-parsing prose
Every genuine Critical in review rounds 5-8 lived in one place: the
deferral channel was free text re-parsed for provenance it did not carry.
A separator regex classified deterministic source, a marker regex caught
mis-routed Criticals, and each round's probe found the spelling the last
fix excluded — kebab paths, the SKILL's own aggregate suffix, an en dash, a
title-borne [test], (Critical), a fullwidth colon. A whole-module
self-audit (with reproduced witnesses) found eight more shapes and named
the class: the fourth, fifth and sixth rounds of the same enumeration
trap this repo's own review doctrine (#9095) says to close structurally.
So the entry is typed. `deferredSuggestions` is an array of
`{file, line?, source, severity, title, locations?}` copied from the
findings artifact the model already wrote in Step 6: deterministic derives
from `source` (build/test/probe), relocation from `severity === Critical`
(counts toward C, blocks, rides the ledger, classified by its FIELD — a
title mentioning [test] no longer exempts an unverified claim), a
`Nice to have` or malformed or free-text entry is refused at the boundary
like a NaN count (the channel that un-posts findings is not guessed at),
and the human line `file:line — [source] title (+N locations)` is RENDERED
by compose-review — nothing downstream parses it back. Both regexes and
the split helper's string grammar are gone; the SKILL contract, the state
bullet, the submit seam test and the whole deferral describe block are
rewritten for the typed shape (no test probes a spelling any more).
The self-audit's side-file findings land in the same commit: the recovered
write never lowers the round (a stale walked list — a concurrent lane, or a
paginated fetch that came back short — overwrote round 7 with round 2 and
dropped the anchor sha; compare on round, reviewId as tiebreak), and login
comparison is case-insensitive per GitHub (a case mismatch read "own
review exists" as proven absence and deleted the counter). Both pinned.
This pulls the structural half of #9176 into the PR; the issue keeps its
remaining bullets (structured `deferred` artifact marker, age-evidence arm,
R/D-id timing).
* fix(review): bound the relocation exit and dedupe rescued targets by identity
Round-9 findings on the pre-typed head, the halves that survive typing:
- The relocation exit applies the same per-entry bound as the deferred
exit — newline collapse, the 240-char cap without splitting a surrogate
pair, the ellipsis on a trim, and Markdown neutralization — through one
shared helper. Relocated titles were spliced into the body verbatim, an
unbounded feed the deferred exit's cap was added precisely to prevent.
- Rescued PR-shaped flag values dedupe by RESOLVED TARGET (number, plus
host/owner/repo for a URL), not by raw string: a bare number and a
same-number URL are one PR, and a raw-token Set read them as two and
silently fell back to the local tree. Of several spellings of one rescued
PR exactly one becomes the target; the restatements no longer surface as
"Ignoring extra argument(s)" on the invocation the dedupe blesses.
- The persist test's debris check asserts on the directory listing rather
than a temp name no code path writes (the temp is per-process).
The round's other two findings — the whole-entry Critical tripwire and its
title-borne false positives — no longer have an input form: the channel is
typed (
|
||
|---|---|---|
| .. | ||
| scripts | ||
| src | ||
| vendor | ||
| index.ts | ||
| package.json | ||
| test-setup.ts | ||
| tsconfig.json | ||
| vitest.config.ts | ||