mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-08-24 16:16:13 +00:00
feat(review): adopt a round-aware convergence posture for posted findings (#9118)
* 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 (6118109118) and severity is a field.
This commit is contained in:
Shaojin Wen
GitHub
parent
90f754e73e
commit
162213e9da
18 changed files with 2295 additions and 100 deletions
|
|
@ -109,11 +109,12 @@ Settings are organized into categories. Most settings should be placed within th
|
|||
|
||||
#### review
|
||||
|
||||
| Setting | Type | Description | Default |
|
||||
| -------------------- | ------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------- |
|
||||
| `review.attribution` | boolean | Append the attribution footer naming the model and CLI version (e.g. `_— qwen3-coder via Qwen Code /review (v0.21.2)_`) to review bodies and inline comments posted by `/review`. Disable to post reviews without AI attribution. With the footer off, presubmit duplicate detection still recognizes earlier posts by the same GitHub account, but footer-less posts from other accounts escape it. | `true` |
|
||||
| `review.effort` | enum | Default effort for `/review` when `--effort` is not given: `"low"`, `"medium"`, `"high"`, or `"auto"` (the built-in rule: high for PRs, medium for local changes). An explicit `--effort` wins; an effective `--comment` still forces high and `--fix` still floors at medium. | `"auto"` |
|
||||
| `review.comment` | boolean | Treat every PR `/review` as if `--comment` was passed: findings are posted to the pull request without the flag. The post still binds to the PR named in the invocation. Enable only if you always want reviews published. | `false` |
|
||||
| Setting | Type | Description | Default |
|
||||
| ---------------------- | ------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------- |
|
||||
| `review.attribution` | boolean | Append the attribution footer naming the model and CLI version (e.g. `_— qwen3-coder via Qwen Code /review (v0.21.2)_`) to review bodies and inline comments posted by `/review`. Disable to post reviews without AI attribution. With the footer off, presubmit duplicate detection still recognizes earlier posts by the same GitHub account, but footer-less posts from other accounts escape it. | `true` |
|
||||
| `review.effort` | enum | Default effort for `/review` when `--effort` is not given: `"low"`, `"medium"`, `"high"`, or `"auto"` (the built-in rule: high for PRs, medium for local changes). An explicit `--effort` wins; an effective `--comment` still forces high and `--fix` still floors at medium. | `"auto"` |
|
||||
| `review.comment` | boolean | Treat every PR `/review` as if `--comment` was passed: findings are posted to the pull request without the flag. The post still binds to the PR named in the invocation. Enable only if you always want reviews published. | `false` |
|
||||
| `review.severityFloor` | enum | The lowest severity a PR `/review` posts when `--severity-floor` is not given: `"auto"` (the round-adaptive default — Suggestions post through round 5, only Criticals from round 6, with otherwise-postable high-confidence Suggestions recorded and deferred, and rounds 2–5 deferring new Suggestions on code unchanged since the previous round; low-confidence and Nice-to-have findings stay terminal-only), `"critical"` (that posture from round 1), or `"suggestion"` (Suggestions post at every round; turns the convergence posture off). Non-PR targets have no rounds and ignore this. | `"auto"` |
|
||||
|
||||
These settings are read from operator scopes only (User, System, and SystemDefaults); values in a workspace `.qwen/settings.json` are ignored, so a repository cannot set review policy for its reviewers.
|
||||
|
||||
|
|
|
|||
|
|
@ -14,7 +14,7 @@ import {
|
|||
utimesSync,
|
||||
} from 'node:fs';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { join } from 'node:path';
|
||||
import { dirname, join } from 'node:path';
|
||||
import { createHash } from 'node:crypto';
|
||||
import { promptRecordDir, briefPath } from './lib/prompt-record.js';
|
||||
import { writeBudgetStop, writeRoundCapStop } from './lib/deadline.js';
|
||||
|
|
@ -32,6 +32,7 @@ import {
|
|||
verdictLine,
|
||||
type ComposeReviewInput,
|
||||
type ComposeReviewResult,
|
||||
type DeferredEntry,
|
||||
type PrBodyFetcher,
|
||||
} from './compose-review.js';
|
||||
|
||||
|
|
@ -3061,6 +3062,7 @@ describe('verdictLine — the terminal verdict, and its dangling colon', () => {
|
|||
downgraded: false,
|
||||
downgradedFrom: null,
|
||||
remediation: [],
|
||||
deferredCount: 0,
|
||||
lowSignal: null,
|
||||
...over,
|
||||
});
|
||||
|
|
@ -4413,6 +4415,448 @@ describe('the ledger marker reaches the POSTED body', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('composeReview — convergence-posture deferrals (typed channel; disclosed, never capping)', () => {
|
||||
// The channel is TYPED: `{file, line?, source, severity, title, locations?}`.
|
||||
// Deterministic derives from `source`, relocation from `severity`, and the
|
||||
// rendered `file:line — [source] title` is formatting nothing re-parses —
|
||||
// the class of regex misses four review rounds kept finding is closed by
|
||||
// construction, so no test here probes a spelling.
|
||||
const nit = (over: Partial<DeferredEntry> = {}): DeferredEntry => ({
|
||||
file: 'a.ts',
|
||||
line: 1,
|
||||
source: 'review',
|
||||
severity: 'Suggestion',
|
||||
title: 'nit',
|
||||
...over,
|
||||
});
|
||||
|
||||
it('an APPROVE with deferrals keeps its event, anchor, and honesty', () => {
|
||||
// The posture's whole payoff: a clean late round with only deferrals
|
||||
// composes an APPROVE — the loop's stop signal — while the deferred list
|
||||
// stays on the record and the incremental anchor still rides. And the
|
||||
// opener must not claim "No issues found" over findings the same body
|
||||
// lists two paragraphs down.
|
||||
const planPath = coveredPlan(['verify', 'reverse-audit'], {
|
||||
prNumber: 8255,
|
||||
fetchedSha: 'deadbeef00112233',
|
||||
});
|
||||
writeFileSync(
|
||||
join(dirname(planPath), 'qwen-review-pr-8255-prev-ledger.json'),
|
||||
JSON.stringify({ v: 1, round: 5, findings: [] }),
|
||||
);
|
||||
const r = composeReview({
|
||||
planPath,
|
||||
env: ENV,
|
||||
modelId: MODEL,
|
||||
criticalsInline: 0,
|
||||
suggestionsInline: 0,
|
||||
severityFloor: 'auto',
|
||||
deferredSuggestions: [
|
||||
nit({ file: 'src/a.ts', line: 42, title: 'tighten the retry backoff' }),
|
||||
],
|
||||
});
|
||||
expect(r.event).toBe('APPROVE');
|
||||
expect(r.cappedBy).toEqual([]);
|
||||
expect(r.body).toContain('No blocking issues. LGTM! ✅');
|
||||
expect(r.body).not.toContain('No issues found');
|
||||
expect(r.body).toContain('convergence posture (round 6, not a blocker)');
|
||||
expect(r.body).toContain(
|
||||
'- `src/a.ts:42 — [review] tighten the retry backoff`',
|
||||
);
|
||||
expect(parseLedger(r.body)?.sha).toBe('deadbeef00112233');
|
||||
// The clause and the marker must name the SAME round — mutation-verified
|
||||
// that re-splitting the side-file read ships green without this pin.
|
||||
expect(parseLedger(r.body)?.round).toBe(6);
|
||||
// Pure deferrals stay OUT of the ledger work list — feeding them to
|
||||
// buildLedger re-opens next round exactly what the posture recorded so
|
||||
// nobody would re-rule it.
|
||||
expect(parseLedger(r.body)?.findings).toEqual([]);
|
||||
});
|
||||
|
||||
it('renders the list on COMMENT and REQUEST_CHANGES alike — no event squeezes it out', () => {
|
||||
const comment = composeReview(
|
||||
base({
|
||||
suggestionsInline: 1,
|
||||
severityFloor: 'critical',
|
||||
deferredSuggestions: [nit()],
|
||||
}),
|
||||
);
|
||||
expect(comment.event).toBe('COMMENT');
|
||||
expect(comment.body).toContain('- `a.ts:1 — [review] nit`');
|
||||
// The count rides every return site, not only APPROVE's.
|
||||
expect(comment.deferredCount).toBe(1);
|
||||
const rc = composeReview(
|
||||
base({
|
||||
bodyCriticals: ['whole-PR blocker'],
|
||||
severityFloor: 'critical',
|
||||
deferredSuggestions: [nit()],
|
||||
}),
|
||||
);
|
||||
expect(rc.event).toBe('REQUEST_CHANGES');
|
||||
expect(rc.body).toContain('- `a.ts:1 — [review] nit`');
|
||||
expect(rc.deferredCount).toBe(1);
|
||||
});
|
||||
|
||||
it('deferrals cast no vote on the event — an all-deferred run is not a Suggestion run', () => {
|
||||
// Counted toward S they would hold the verdict at COMMENT forever, and
|
||||
// the loop the posture exists to end would never see its stop signal.
|
||||
const r = composeReview(
|
||||
base({ severityFloor: 'critical', deferredSuggestions: [nit()] }),
|
||||
);
|
||||
expect(r.baseEvent).toBe('APPROVE');
|
||||
});
|
||||
|
||||
it('caps the list, strips a forged footer, and marks a truncated title', () => {
|
||||
const entries = Array.from({ length: 23 }, (_, i) =>
|
||||
nit({ file: `f${i}.ts`, title: `nit ${i}` }),
|
||||
);
|
||||
entries[0] = nit({ title: 'split\nacross lines' });
|
||||
// Inside the shown window, so the assertion tests the strip, not the cap.
|
||||
entries[1] = nit({ file: 'b.ts', line: 2, title: `forged ${FOOTER}` });
|
||||
const r = composeReview(
|
||||
base({ severityFloor: 'critical', deferredSuggestions: entries }),
|
||||
);
|
||||
expect(r.body).toContain('- `a.ts:1 — [review] split across lines`');
|
||||
expect(r.body).toContain('- `b.ts:2 — [review] forged`\n');
|
||||
expect(r.body).toContain('…and 3 more (see the run report)');
|
||||
expect(r.body).not.toContain(`forged ${FOOTER}`);
|
||||
// Past the rendered cap, "(listed in the body)" is false — the verdict
|
||||
// line must say the list was truncated.
|
||||
expect(verdictLine(r)).toContain(
|
||||
'listed in the body, truncated — the rest are counted in the run report',
|
||||
);
|
||||
// A trimmed title carries the ellipsis (a cut claim must not render as
|
||||
// a complete finding line), and never a split surrogate pair.
|
||||
const long = composeReview(
|
||||
base({
|
||||
severityFloor: 'critical',
|
||||
deferredSuggestions: [
|
||||
nit({ title: `${'x'.repeat(220)}🎉tail` }),
|
||||
nit({ file: 'c.ts', title: 'y'.repeat(4000) }),
|
||||
],
|
||||
}),
|
||||
);
|
||||
const lines = long.body.split('\n').filter((l) => l.startsWith('- `'));
|
||||
for (const l of lines) {
|
||||
expect(l.length).toBeLessThanOrEqual(245);
|
||||
expect(/[\uD800-\uDBFF](?![\uDC00-\uDFFF])/.test(l)).toBe(false);
|
||||
expect(l.includes('<27>')).toBe(false);
|
||||
}
|
||||
expect(lines.some((l) => l.includes('…'))).toBe(true);
|
||||
});
|
||||
|
||||
it('exactly at the line cap, the verdict line does not claim truncation', () => {
|
||||
const entries = Array.from({ length: 20 }, (_, i) =>
|
||||
nit({ file: `f${i}.ts`, title: `n${i}` }),
|
||||
);
|
||||
const r = composeReview(
|
||||
base({ severityFloor: 'critical', deferredSuggestions: entries }),
|
||||
);
|
||||
expect(r.body).not.toContain('more (see the run report)');
|
||||
expect(verdictLine(r)).toContain('(listed in the body)');
|
||||
expect(verdictLine(r)).not.toContain('truncated');
|
||||
});
|
||||
|
||||
it('a deferrals-only APPROVE is not low signal, and the verdict line names the deferrals', () => {
|
||||
const r = composeReview(
|
||||
base({
|
||||
planPath: coveredPlan(['verify', 'reverse-audit'], {
|
||||
srcDiffLines: 5000,
|
||||
}),
|
||||
severityFloor: 'critical',
|
||||
deferredSuggestions: [nit()],
|
||||
}),
|
||||
);
|
||||
expect(r.event).toBe('APPROVE');
|
||||
expect(r.lowSignal).toBeNull();
|
||||
expect(r.deferredCount).toBe(1);
|
||||
expect(verdictLine(r)).toBe(
|
||||
'Verdict: Approve — 1 non-Critical finding(s) deferred under the convergence posture (listed in the body)',
|
||||
);
|
||||
});
|
||||
|
||||
it('deferred findings count toward the verifier-delivery floor — deterministic sources excepted', () => {
|
||||
// A deferral publishes its claim in the body, so a deferrals-only run
|
||||
// owes a verifier exactly as a posting run does — unless the source is
|
||||
// deterministic (build/test/probe are pre-confirmed and Step 4 launches
|
||||
// no verifier for them; demanding one would be a permanent self-cap).
|
||||
// NOT base(): its planPath default writes a verify record into the
|
||||
// shared dir, which would satisfy the very floor this proves.
|
||||
const planPath = coveredPlan(['reverse-audit']);
|
||||
const common = {
|
||||
criticalsInline: 0,
|
||||
suggestionsInline: 0,
|
||||
planPath,
|
||||
env: ENV,
|
||||
modelId: MODEL,
|
||||
severityFloor: 'critical' as const,
|
||||
};
|
||||
expect(composeReview(common).cappedBy).toEqual([]);
|
||||
const reviewSourced = composeReview({
|
||||
...common,
|
||||
deferredSuggestions: [nit()],
|
||||
});
|
||||
expect(reviewSourced.cappedBy).toContain('unreviewed-dimension');
|
||||
expect(reviewSourced.event).toBe('COMMENT');
|
||||
for (const source of ['build', 'test', 'probe'] as const) {
|
||||
const det = composeReview({
|
||||
...common,
|
||||
deferredSuggestions: [
|
||||
nit({
|
||||
file: 'packages/core/src/my-file.ts',
|
||||
line: 42,
|
||||
source,
|
||||
title: 'mutation survivor',
|
||||
locations: 2,
|
||||
}),
|
||||
],
|
||||
});
|
||||
expect(det.cappedBy).toEqual([]);
|
||||
expect(det.event).toBe('APPROVE');
|
||||
expect(det.body).toContain(
|
||||
`- \`packages/core/src/my-file.ts:42 (+2 locations) — [${source}] mutation survivor\``,
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
it('relocates a Critical entry into the body Criticals — never a throw, never deferred', () => {
|
||||
// The entry is a Critical by its own field, so it counts toward C, the
|
||||
// event blocks, the round posts, and it rides the machine ledger ("the
|
||||
// findings always ride" includes the mis-routed ones).
|
||||
const planPath = coveredPlan(['verify', 'reverse-audit'], {
|
||||
prNumber: 8255,
|
||||
fetchedSha: 'deadbeef00112233',
|
||||
});
|
||||
const r = composeReview({
|
||||
planPath,
|
||||
env: ENV,
|
||||
modelId: MODEL,
|
||||
criticalsInline: 0,
|
||||
suggestionsInline: 0,
|
||||
deferredSuggestions: [
|
||||
nit({
|
||||
file: 'src/auth.ts',
|
||||
line: 88,
|
||||
severity: 'Critical',
|
||||
title: 'auth bypass',
|
||||
}),
|
||||
],
|
||||
});
|
||||
expect(r.event).toBe('REQUEST_CHANGES');
|
||||
expect(r.deferredCount).toBe(0);
|
||||
expect(r.body).toContain(
|
||||
'**[Critical]** `src/auth.ts:88 — [review] auth bypass` _(relocated from the deferral channel',
|
||||
);
|
||||
expect(parseLedger(r.body)?.findings.some((f) => f.sev === 'C')).toBe(true);
|
||||
// A relocation-only run (no floor echoed) incurs no licence cap — the
|
||||
// licence keys on the post-split deferred list, and salvage is exactly
|
||||
// the run relocation exists for.
|
||||
expect(r.cappedBy).not.toContain('unlicensed-deferral');
|
||||
});
|
||||
|
||||
it('a relocated Critical is classified by its source FIELD, never its title', () => {
|
||||
// `source: 'review'` owes a verifier and caps `criticals-unverified`
|
||||
// when none ran, whatever the title mentions; `source: 'test'` is
|
||||
// pre-confirmed and blocks. Own case: the flagship relocation test's
|
||||
// verify record in the shared dir would satisfy the very floor this
|
||||
// proves.
|
||||
const titled = composeReview({
|
||||
criticalsInline: 0,
|
||||
suggestionsInline: 0,
|
||||
planPath: coveredPlan(['reverse-audit']),
|
||||
env: ENV,
|
||||
modelId: MODEL,
|
||||
deferredSuggestions: [
|
||||
nit({
|
||||
severity: 'Critical',
|
||||
title: 'mishandles [test] configuration files',
|
||||
}),
|
||||
],
|
||||
});
|
||||
expect(titled.cappedBy).toContain('criticals-unverified');
|
||||
expect(titled.event).toBe('COMMENT');
|
||||
const genuine = composeReview({
|
||||
criticalsInline: 0,
|
||||
suggestionsInline: 0,
|
||||
planPath: coveredPlan(['reverse-audit']),
|
||||
env: ENV,
|
||||
modelId: MODEL,
|
||||
deferredSuggestions: [
|
||||
nit({
|
||||
severity: 'Critical',
|
||||
source: 'test',
|
||||
title: 'red on the merge',
|
||||
}),
|
||||
],
|
||||
});
|
||||
expect(genuine.cappedBy).not.toContain('criticals-unverified');
|
||||
expect(genuine.event).toBe('REQUEST_CHANGES');
|
||||
});
|
||||
|
||||
it('a relocated Critical is bounded like its deferred siblings — no unbounded feed into the body', () => {
|
||||
// Round-9 finding: relocation bypassed the per-entry cap, the newline
|
||||
// collapse, the surrogate trim and the Markdown neutralization that the
|
||||
// deferred exit applies; twenty-five 4,000-char relocated titles would
|
||||
// splice ~100 KB into the body and lose the review at GitHub's limit.
|
||||
const r = composeReview(
|
||||
base({
|
||||
deferredSuggestions: [
|
||||
nit({
|
||||
severity: 'Critical',
|
||||
title: `${'x'.repeat(4000)}\nsecond line @mention #123`,
|
||||
}),
|
||||
],
|
||||
}),
|
||||
);
|
||||
const bodyLine = r.body
|
||||
.split('\n')
|
||||
.find((l) => l.startsWith('**[Critical]**'))!;
|
||||
// marker + backticked bounded line + relocation note: well under 4,000.
|
||||
expect(bodyLine.length).toBeLessThan(400);
|
||||
expect(bodyLine).toContain('…');
|
||||
expect(bodyLine).not.toContain('\nsecond');
|
||||
// Neutralized: the title rides inside a code span.
|
||||
expect(bodyLine).toMatch(/\*\*\[Critical\]\*\* `a\.ts:1 — \[review\] x+…`/);
|
||||
});
|
||||
|
||||
it('refuses a malformed entry — the channel that un-posts findings is not guessed at', () => {
|
||||
const cases: Array<[unknown, RegExp]> = [
|
||||
['a.ts:1 — nit', /free-text entry is not accepted/],
|
||||
[
|
||||
{ file: 'a.ts', source: 'review', severity: 'Suggestion' },
|
||||
/non-empty file and title/,
|
||||
],
|
||||
[
|
||||
{ file: 'a.ts', source: 'lint?', severity: 'Suggestion', title: 't' },
|
||||
/source must be one of/,
|
||||
],
|
||||
[
|
||||
{ file: 'a.ts', source: 'review', severity: 'Blocker', title: 't' },
|
||||
/severity must be one of/,
|
||||
],
|
||||
[
|
||||
{
|
||||
file: 'a.ts',
|
||||
source: 'review',
|
||||
severity: 'Nice to have',
|
||||
title: 't',
|
||||
},
|
||||
/terminal-only findings are never deferred/,
|
||||
],
|
||||
[
|
||||
{
|
||||
file: 'a.ts',
|
||||
line: 0,
|
||||
source: 'review',
|
||||
severity: 'Suggestion',
|
||||
title: 't',
|
||||
},
|
||||
/line must be a positive integer/,
|
||||
],
|
||||
];
|
||||
for (const [entry, re] of cases) {
|
||||
expect(() =>
|
||||
composeReview(base({ deferredSuggestions: [entry] as never })),
|
||||
).toThrow(re);
|
||||
}
|
||||
expect(() =>
|
||||
composeReview(base({ deferredSuggestions: 'a.ts' as never })),
|
||||
).toThrow(/deferredSuggestions/);
|
||||
});
|
||||
|
||||
it('caps — never refuses — deferrals the posture does not license', () => {
|
||||
// The channel only ever removes findings from posting, so unlicensed
|
||||
// shapes fail CLOSED but not FATAL: a thrown compose loses the whole
|
||||
// round, Criticals included, and `prevRound` is a best-effort side-file
|
||||
// read whose every failure mode returns 0 — a missing file at a true
|
||||
// round 6 must degrade to a disclosed, capped verdict, never to no
|
||||
// verdict at all. Every shape renders the list, discloses the missing
|
||||
// licence, caps the event, and withholds the anchor.
|
||||
const explicitOff = composeReview(
|
||||
base({ severityFloor: 'suggestion', deferredSuggestions: [nit()] }),
|
||||
);
|
||||
expect(explicitOff.cappedBy).toContain('unlicensed-deferral');
|
||||
expect(explicitOff.event).toBe('COMMENT');
|
||||
expect(explicitOff.body).toContain('without a posture licence');
|
||||
expect(explicitOff.body).toContain('- `a.ts:1 — [review] nit`');
|
||||
// The opener may not certify what the ⚠️ clause retracts.
|
||||
expect(explicitOff.body).not.toContain('no blockers');
|
||||
expect(parseLedger(explicitOff.body)?.sha).toBeUndefined();
|
||||
const round1Auto = composeReview(
|
||||
base({ severityFloor: 'auto', deferredSuggestions: [nit()] }),
|
||||
);
|
||||
expect(round1Auto.cappedBy).toContain('unlicensed-deferral');
|
||||
expect(verdictLine(round1Auto)).toContain(
|
||||
'findings were deferred without a posture licence',
|
||||
);
|
||||
// An ABSENT floor beside a non-empty list is unlicensed too: the field
|
||||
// ships in the same PR as the channel, so omission is fail-closed.
|
||||
const absent = composeReview(base({ deferredSuggestions: [nit()] }));
|
||||
expect(absent.cappedBy).toContain('unlicensed-deferral');
|
||||
expect(absent.body).toContain('carried no recognisable `severityFloor`');
|
||||
// And `auto` in the context-unavailable state: the round is unknowable.
|
||||
const noContext = composeReview(
|
||||
base({
|
||||
severityFloor: 'auto',
|
||||
contextUnavailable: true,
|
||||
deferredSuggestions: [nit()],
|
||||
}),
|
||||
);
|
||||
expect(noContext.cappedBy).toContain('unlicensed-deferral');
|
||||
expect(noContext.body).toContain('context-unavailable');
|
||||
});
|
||||
|
||||
it('an unrecognised severityFloor is unknown — never a throw', () => {
|
||||
// A model-transcribed drift ("Critical", "auto ", "") on an ordinary
|
||||
// zero-deferral round must not lose the WHOLE composed round over a
|
||||
// field that changes no output. Unknown folds into the absent state:
|
||||
// unlicensed (capped, disclosed) with a list, inert without one.
|
||||
// Trimmed/cased spellings of the three legal values still resolve.
|
||||
const withList = composeReview(
|
||||
base({ severityFloor: 'blocker' as never, deferredSuggestions: [nit()] }),
|
||||
);
|
||||
expect(withList.cappedBy).toContain('unlicensed-deferral');
|
||||
const inert = composeReview(base({ severityFloor: 'blocker' as never }));
|
||||
expect(inert.event).toBe('APPROVE');
|
||||
expect(inert.cappedBy).toEqual([]);
|
||||
const cased = composeReview(
|
||||
base({
|
||||
severityFloor: ' Critical ' as never,
|
||||
deferredSuggestions: [nit()],
|
||||
}),
|
||||
);
|
||||
expect(cased.cappedBy).toEqual([]);
|
||||
expect(cased.deferredCount).toBe(1);
|
||||
});
|
||||
|
||||
it('auto with a recovered previous round licenses the age-rule deferral', () => {
|
||||
// The state carries `auto` unresolved and the module licenses it by the
|
||||
// round it derives itself — this pins the legal rounds-2-5 shape end to
|
||||
// end (a round-resolved `suggestion` would have been refused as the
|
||||
// operator's override — the shipped round-5 regression).
|
||||
const planPath = coveredPlan(['verify', 'reverse-audit'], {
|
||||
prNumber: 8255,
|
||||
fetchedSha: 'deadbeef00112233',
|
||||
});
|
||||
writeFileSync(
|
||||
join(dirname(planPath), 'qwen-review-pr-8255-prev-ledger.json'),
|
||||
JSON.stringify({ v: 1, round: 2, findings: [] }),
|
||||
);
|
||||
const r = composeReview({
|
||||
planPath,
|
||||
env: ENV,
|
||||
modelId: MODEL,
|
||||
criticalsInline: 0,
|
||||
suggestionsInline: 0,
|
||||
severityFloor: 'auto',
|
||||
deferredSuggestions: [nit({ title: 'aged-out nit' })],
|
||||
});
|
||||
expect(r.cappedBy).toEqual([]);
|
||||
expect(r.event).toBe('APPROVE');
|
||||
expect(r.body).toContain('convergence posture (round 3, not a blocker)');
|
||||
});
|
||||
});
|
||||
|
||||
describe('composeReview — the findings file tag check', () => {
|
||||
// The pipelined loop's invariant, machine-read. Under the serial loop the
|
||||
// last round's verification completing before Step 6 was structural; the
|
||||
|
|
|
|||
|
|
@ -30,7 +30,13 @@ import {
|
|||
verificationGaps,
|
||||
TranscriptsUnavailableError,
|
||||
} from './lib/coverage.js';
|
||||
import { compressSummary } from './findings.js';
|
||||
import {
|
||||
compressSummary,
|
||||
SEVERITIES,
|
||||
SOURCES,
|
||||
type Severity,
|
||||
type Source,
|
||||
} from './findings.js';
|
||||
import {
|
||||
BUDGET_STOP_PHRASE,
|
||||
ROUND_CAP_PHRASE,
|
||||
|
|
@ -94,6 +100,209 @@ export type ReviewEvent = 'APPROVE' | 'REQUEST_CHANGES' | 'COMMENT';
|
|||
*/
|
||||
export const LOW_SIGNAL_SRC_DIFF_LINES = 100;
|
||||
|
||||
/**
|
||||
* The deferred-suggestions list's rendered bounds. Module-scoped because two
|
||||
* surfaces read the line cap: the body renderer that applies it, and
|
||||
* `verdictLine`, whose "(listed in the body)" claim must turn cap-aware the
|
||||
* moment the list overflows — a verdict that counts 21 over a body that
|
||||
* lists 20 is a false record persisted into the archived report.
|
||||
*/
|
||||
const MAX_DEFERRED_SUGGESTION_LINES = 20;
|
||||
const MAX_DEFERRED_SUGGESTION_CHARS = 240;
|
||||
|
||||
/**
|
||||
* The deterministic source tags, exactly as the body-Critical scan reads
|
||||
* them (~`nonDeterministicBodyCriticals`): a `[build]`/`[test]`/`[probe]`
|
||||
* finding is pre-confirmed and skips Step 4 by design, so it never produces
|
||||
* a verifier delivery — demanding one for it is an unsatisfiable cap.
|
||||
*/
|
||||
const DETERMINISTIC_TAG_RE = /\[(?:build|test|probe)\]/i;
|
||||
|
||||
/**
|
||||
* A deferred finding, TYPED. The convergence posture removes findings from
|
||||
* posting through exactly one channel, and for four review rounds that
|
||||
* 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 every round's probe found the spelling each
|
||||
* regex excluded — kebab paths, the SKILL's own aggregate suffix, an en
|
||||
* dash, `(Critical)`, a title-borne `[test]`. The class closes only by
|
||||
* carrying the fields: the model already holds `file`/`line`/`source`/
|
||||
* `severity`/`title` for every finding in the artifact it wrote in Step 6,
|
||||
* so the entry carries them, `deterministic` derives from `source`, the
|
||||
* relocation from `severity`, and the rendered `file:line — [source] title`
|
||||
* is formatting — nothing downstream ever parses it back.
|
||||
*
|
||||
* Validated at the boundary like every other model-written state field:
|
||||
* a present entry of the wrong shape is refused (a NaN count is refused
|
||||
* the same way), because a channel that un-posts findings must not be
|
||||
* guessed at.
|
||||
*/
|
||||
export interface DeferredEntry {
|
||||
file: string;
|
||||
line?: number;
|
||||
/** The finding's source tag — decides deterministic (`build`/`test`/`probe`). */
|
||||
source: Source;
|
||||
/**
|
||||
* The finding's severity. Only `Suggestion` defers; a `Critical` here is
|
||||
* RELOCATED into the body Criticals (a Critical is never deferred), and a
|
||||
* `Nice to have` is refused (terminal-only, never publishable).
|
||||
*/
|
||||
severity: Severity;
|
||||
/** One-line claim, rendered inside a code span; a location count may be appended. */
|
||||
title: string;
|
||||
/** For a pattern aggregate: how many further locations the finding covers. */
|
||||
locations?: number;
|
||||
}
|
||||
|
||||
const DETERMINISTIC_SOURCES: ReadonlySet<Source> = new Set([
|
||||
'build',
|
||||
'test',
|
||||
'probe',
|
||||
]);
|
||||
|
||||
/** Render one entry as the human line — formatting only, never re-parsed. */
|
||||
export function renderDeferredEntry(entry: DeferredEntry): string {
|
||||
const loc =
|
||||
entry.line !== undefined ? `${entry.file}:${entry.line}` : entry.file;
|
||||
const agg =
|
||||
entry.locations && entry.locations > 0
|
||||
? ` (+${entry.locations} locations)`
|
||||
: '';
|
||||
return `${loc}${agg} — [${entry.source}] ${entry.title}`;
|
||||
}
|
||||
|
||||
/**
|
||||
* The per-entry bound every deferral-channel exit applies — deferred AND
|
||||
* relocated: collapse newlines, cap at MAX_DEFERRED_SUGGESTION_CHARS
|
||||
* without splitting a surrogate pair, mark a trim with an ellipsis. The
|
||||
* relocation exit once bypassed all of it (round-9 finding): twenty-five
|
||||
* relocated 4,000-char titles spliced ~100 KB of unbounded model text into
|
||||
* the body — the whole review lost at GitHub's 65,536 limit, precisely what
|
||||
* the cap on the deferred exit was added to prevent.
|
||||
*/
|
||||
function boundDeferredLine(rendered: string): string {
|
||||
const collapsed = rendered
|
||||
.split('\n')
|
||||
.map((seg) => seg.trim())
|
||||
.filter((seg) => seg !== '')
|
||||
.join(' ');
|
||||
let oneLine = collapsed.slice(0, MAX_DEFERRED_SUGGESTION_CHARS);
|
||||
// The cap slices UTF-16 code units; a cut landing inside a surrogate pair
|
||||
// leaves a lone high surrogate that serializes as U+FFFD into the posted
|
||||
// body — and the zh clause keeps titles untranslated, so astral CJK/emoji
|
||||
// at the boundary are a real input, not a curiosity.
|
||||
if (/[\uD800-\uDBFF]/.test(oneLine.charAt(oneLine.length - 1))) {
|
||||
oneLine = oneLine.slice(0, -1);
|
||||
}
|
||||
// A trimmed entry must say so — a claim cut mid-sentence otherwise renders
|
||||
// as a complete finding line on the PR record.
|
||||
if (oneLine.length < collapsed.length) oneLine += '…';
|
||||
return oneLine;
|
||||
}
|
||||
|
||||
function toDeferredEntries(value: unknown): DeferredEntry[] {
|
||||
if (value === undefined || value === null) return [];
|
||||
if (!Array.isArray(value)) {
|
||||
throw new TypeError(
|
||||
`compose-review: deferredSuggestions must be an array of {file, line?, source, severity, title, locations?} entries, got ${JSON.stringify(value)}`,
|
||||
);
|
||||
}
|
||||
return value.map((raw, i) => {
|
||||
if (typeof raw !== 'object' || raw === null || Array.isArray(raw)) {
|
||||
throw new TypeError(
|
||||
`compose-review: deferredSuggestions[${i}] must be an object {file, line?, source, severity, title, locations?} — a free-text entry is not accepted, the channel is typed`,
|
||||
);
|
||||
}
|
||||
const o = raw as Record<string, unknown>;
|
||||
const file = typeof o['file'] === 'string' ? o['file'].trim() : '';
|
||||
const title =
|
||||
typeof o['title'] === 'string'
|
||||
? stripReviewFooter(o['title']).trim()
|
||||
: '';
|
||||
const source = o['source'];
|
||||
const severity = o['severity'];
|
||||
const line = o['line'];
|
||||
const locations = o['locations'];
|
||||
if (file === '' || title === '') {
|
||||
throw new TypeError(
|
||||
`compose-review: deferredSuggestions[${i}] needs a non-empty file and title`,
|
||||
);
|
||||
}
|
||||
if (typeof source !== 'string' || !SOURCES.includes(source as Source)) {
|
||||
throw new TypeError(
|
||||
`compose-review: deferredSuggestions[${i}].source must be one of ${SOURCES.join('|')}, got ${JSON.stringify(source)}`,
|
||||
);
|
||||
}
|
||||
if (
|
||||
typeof severity !== 'string' ||
|
||||
!SEVERITIES.includes(severity as Severity)
|
||||
) {
|
||||
throw new TypeError(
|
||||
`compose-review: deferredSuggestions[${i}].severity must be one of ${SEVERITIES.join('|')}, got ${JSON.stringify(severity)}`,
|
||||
);
|
||||
}
|
||||
if (severity === 'Nice to have') {
|
||||
throw new TypeError(
|
||||
`compose-review: deferredSuggestions[${i}] is a Nice to have — terminal-only findings are never deferred to the PR; drop it from the state`,
|
||||
);
|
||||
}
|
||||
if (
|
||||
line !== undefined &&
|
||||
line !== null &&
|
||||
(typeof line !== 'number' || !Number.isInteger(line) || line < 1)
|
||||
) {
|
||||
throw new TypeError(
|
||||
`compose-review: deferredSuggestions[${i}].line must be a positive integer when present`,
|
||||
);
|
||||
}
|
||||
if (
|
||||
locations !== undefined &&
|
||||
locations !== null &&
|
||||
(typeof locations !== 'number' ||
|
||||
!Number.isInteger(locations) ||
|
||||
locations < 0)
|
||||
) {
|
||||
throw new TypeError(
|
||||
`compose-review: deferredSuggestions[${i}].locations must be a non-negative integer when present`,
|
||||
);
|
||||
}
|
||||
return {
|
||||
file,
|
||||
...(typeof line === 'number' ? { line } : {}),
|
||||
source: source as Source,
|
||||
severity: severity as Severity,
|
||||
title,
|
||||
...(typeof locations === 'number' && locations > 0 ? { locations } : {}),
|
||||
};
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* The deferral channel's split, shared by the body composer and the ledger
|
||||
* marker: `Critical` entries are RELOCATED into the body Criticals (a
|
||||
* Critical is never deferred — it counts toward `C`, blocks, and rides the
|
||||
* machine ledger), the rest defer. One split, two readers, no parsing.
|
||||
*/
|
||||
function splitDeferralChannel(raw: unknown): {
|
||||
deferred: DeferredEntry[];
|
||||
relocated: string[];
|
||||
/** Relocated entries whose `source` is deterministic — no verifier owed. */
|
||||
relocatedDeterministic: number;
|
||||
} {
|
||||
const entries = toDeferredEntries(raw);
|
||||
const relocatedEntries = entries.filter((e) => e.severity === 'Critical');
|
||||
return {
|
||||
deferred: entries.filter((e) => e.severity !== 'Critical'),
|
||||
relocated: relocatedEntries.map(
|
||||
(e) =>
|
||||
`${mdField(boundDeferredLine(renderDeferredEntry(e)))} _(relocated from the deferral channel — a Critical is never deferred, it posts)_`,
|
||||
),
|
||||
relocatedDeterministic: relocatedEntries.filter((e) =>
|
||||
DETERMINISTIC_SOURCES.has(e.source),
|
||||
).length,
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Reads a PR's description body, given its `owner/repo` and number. The one
|
||||
* production implementation calls `gh pr view`; the bilingual fallback uses it
|
||||
|
|
@ -125,6 +334,40 @@ export interface ComposeReviewInput {
|
|||
bodyCriticals?: string[];
|
||||
/** Suggestions discarded as unanchorable (offline validation or 422). */
|
||||
suggestionsDiscarded?: number;
|
||||
/**
|
||||
* The findings the convergence posture deferred — Step 6's round-aware
|
||||
* posting discipline (from round 6, or under an explicit `--severity-floor
|
||||
* critical`, and the rounds-2-5 code-age rule). TYPED entries — see
|
||||
* `DeferredEntry`: only otherwise-postable high-confidence Suggestions
|
||||
* belong here (a `Critical` is relocated into the body Criticals, a
|
||||
* `Nice to have` is refused; low-confidence findings stay terminal-only and
|
||||
* never enter the state). They are neither drafted inline nor counted
|
||||
* toward `S` — a deferral must not regenerate a review round — but they
|
||||
* must not vanish either: the body renders them as a disclosed,
|
||||
* NON-capping list, so the record survives on the PR while the round
|
||||
* stays convergent. A deferral never withholds the ledger anchor: it is a
|
||||
* posting decision, not unreviewed scope.
|
||||
*/
|
||||
deferredSuggestions?: DeferredEntry[];
|
||||
/**
|
||||
* The UNRESOLVED posting floor from the Step 1 verdict (`critical`,
|
||||
* `suggestion`, or the literal `auto`) — never the level `auto` resolved
|
||||
* to this round: the module resolves `auto` itself from the side-file
|
||||
* round, and a pre-resolved `suggestion` is indistinguishable from the
|
||||
* operator's posture-off override (a shipped regression, closed in round
|
||||
* 5). Carried so the deferral channel's precondition is checkable:
|
||||
* deferrals are legitimate under a
|
||||
* `critical` floor at any round, and under `auto` from round 2 (the
|
||||
* code-age rule) — never under an explicit `suggestion` floor (the
|
||||
* operator turned the posture off), never on round 1 of `auto` (no
|
||||
* posture, no age reference), never under `auto` in the
|
||||
* context-unavailable state (the round is unknowable), and never ABSENT
|
||||
* beside a non-empty deferral list: the field ships in the same PR as the
|
||||
* channel, so omission is fail-closed — a dropped echo must not silently
|
||||
* re-license what an explicit `suggestion` floor forbade. Unlicensed
|
||||
* shapes cap; they never throw.
|
||||
*/
|
||||
severityFloor?: 'critical' | 'suggestion' | 'auto';
|
||||
/**
|
||||
* Existing Criticals already on the PR whose Step 6 re-check landed on
|
||||
* `cannot tell` — one line each (location + what could not be decided).
|
||||
|
|
@ -225,6 +468,14 @@ export interface ComposeReviewResult {
|
|||
* operator which command repairs it. Two registers, two channels.
|
||||
*/
|
||||
remediation: string[];
|
||||
/**
|
||||
* How many non-Critical findings the convergence posture deferred — the
|
||||
* count of `deferredSuggestions` entries that survived validation. On the
|
||||
* verdict surface so `verdictLine` can say a deferrals-only Approve
|
||||
* deferred findings rather than implying none existed: the low-signal
|
||||
* sentence's premise is "zero findings", and a deferral is a finding.
|
||||
*/
|
||||
deferredCount: number;
|
||||
/**
|
||||
* Set on an APPROVE composed from zero findings over a non-trivial source
|
||||
* diff (the plan's `srcDiffLines` above `LOW_SIGNAL_SRC_DIFF_LINES`).
|
||||
|
|
@ -475,16 +726,52 @@ export function composeReview(
|
|||
cliVersion = 'unknown',
|
||||
attribution = true,
|
||||
): ComposeReviewResult {
|
||||
const result = composeReviewBody(input, cliVersion, attribution);
|
||||
// One read, one round: the deferred-suggestions clause and the ledger
|
||||
// marker both name this round, and each reading the side file for itself
|
||||
// would let a mid-compose update publish two different round numbers in
|
||||
// one review.
|
||||
const prevRound = prevRoundFor(input.planPath);
|
||||
const result = composeReviewBody(input, cliVersion, attribution, prevRound);
|
||||
// The ledger marker rides the body THIS function returns, because this — not
|
||||
// the CLI handler — is what `submit` calls and posts. Appending it in the
|
||||
// handler left the feature inert end to end: the marker reached only the
|
||||
// composed JSON on disk, which nothing in the posting path reads, so no
|
||||
// posted review ever carried one and every round recovered `null`.
|
||||
const marker = ledgerMarkerFor(input, result.cappedBy);
|
||||
const marker = ledgerMarkerFor(input, result.cappedBy, prevRound);
|
||||
return marker ? { ...result, body: `${result.body}\n\n${marker}` } : result;
|
||||
}
|
||||
|
||||
/**
|
||||
* The previous posted round's number, recovered from the side file
|
||||
* `pr-context` wrote — never from the model. 0 when the plan names no PR or
|
||||
* no previous round was recovered: this is round 1. Shared by the marker
|
||||
* (which stamps `prevRound + 1`) and the deferred-suggestions clause (which
|
||||
* names the round the posture engaged on), so the two cannot disagree about
|
||||
* which round this is.
|
||||
*/
|
||||
function prevRoundFor(planPath: string | undefined): number {
|
||||
try {
|
||||
if (!planPath) return 0;
|
||||
const plan = JSON.parse(readFileSync(planPath, 'utf8')) as {
|
||||
prNumber?: unknown;
|
||||
};
|
||||
const pr = plan?.prNumber;
|
||||
const isPr =
|
||||
(typeof pr === 'number' && Number.isInteger(pr) && pr > 0) ||
|
||||
(typeof pr === 'string' && /^\d+$/.test(pr));
|
||||
if (!isPr) return 0;
|
||||
const prev = JSON.parse(
|
||||
readFileSync(
|
||||
join(dirname(planPath), `qwen-review-pr-${pr}-prev-ledger.json`),
|
||||
'utf8',
|
||||
),
|
||||
) as Ledger;
|
||||
return Number.isInteger(prev.round) && prev.round > 0 ? prev.round : 0;
|
||||
} catch {
|
||||
return 0;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The next round's marker, or null when this review has no PR to carry one.
|
||||
* Round number comes from the side file `pr-context` wrote from the PREVIOUS
|
||||
|
|
@ -493,6 +780,7 @@ export function composeReview(
|
|||
function ledgerMarkerFor(
|
||||
input: ComposeReviewInput,
|
||||
cappedBy: string[],
|
||||
prevRound: number,
|
||||
): string | null {
|
||||
try {
|
||||
if (!input.planPath) return null;
|
||||
|
|
@ -505,22 +793,6 @@ function ledgerMarkerFor(
|
|||
(typeof pr === 'number' && Number.isInteger(pr) && pr > 0) ||
|
||||
(typeof pr === 'string' && /^\d+$/.test(pr));
|
||||
if (!isPr) return null;
|
||||
let prevRound = 0;
|
||||
try {
|
||||
const prev = JSON.parse(
|
||||
readFileSync(
|
||||
join(
|
||||
dirname(input.planPath),
|
||||
`qwen-review-pr-${pr}-prev-ledger.json`,
|
||||
),
|
||||
'utf8',
|
||||
),
|
||||
) as Ledger;
|
||||
if (Number.isInteger(prev.round) && prev.round > 0)
|
||||
prevRound = prev.round;
|
||||
} catch {
|
||||
// No previous posted round recovered: this is round 1.
|
||||
}
|
||||
// The anchor rides only when this round's scope was clean, and "clean" is
|
||||
// the verdict this module just computed: `cappedBy` aggregates every
|
||||
// fail-closed state — each named input pushes its own cap entry, plus the
|
||||
|
|
@ -551,9 +823,14 @@ function ledgerMarkerFor(
|
|||
line?: unknown;
|
||||
body?: unknown;
|
||||
}>,
|
||||
toStringList(input.bodyCriticals, 'bodyCriticals')
|
||||
.map(stripReviewFooter)
|
||||
.filter((entry) => entry.trim() !== ''),
|
||||
[
|
||||
...toStringList(input.bodyCriticals, 'bodyCriticals')
|
||||
.map(stripReviewFooter)
|
||||
.filter((entry) => entry.trim() !== ''),
|
||||
// The same split the body performed: a relocated Critical is a
|
||||
// posted, counted blocker and must enter the work list.
|
||||
...splitDeferralChannel(input.deferredSuggestions).relocated,
|
||||
],
|
||||
),
|
||||
...(sha ? { sha } : {}),
|
||||
});
|
||||
|
|
@ -567,6 +844,7 @@ function composeReviewBody(
|
|||
input: ComposeReviewInput,
|
||||
cliVersion: string,
|
||||
attribution: boolean,
|
||||
prevRound: number,
|
||||
): ComposeReviewResult {
|
||||
const criticalsInline = toCount(input.criticalsInline, 'criticalsInline');
|
||||
const suggestionsInline = toCount(
|
||||
|
|
@ -585,6 +863,54 @@ function composeReviewBody(
|
|||
input.suggestionsDiscarded,
|
||||
'suggestionsDiscarded',
|
||||
);
|
||||
// A Critical marker in the deferral channel is RELOCATED, never fatal and
|
||||
// never deferred: it counts toward `C`, the event blocks, and the round
|
||||
// posts (a throw would lose the whole round — the round-5 doctrine). The
|
||||
// lookbehind spares hyphenated compounds ("non-Critical findings", the
|
||||
// SKILL's own phrasing); the residual false positive — a Suggestion title
|
||||
// literally opening `critical:` — costs one wrongly-blocking body entry
|
||||
// the next round rules on, not a lost round. The split lives in the
|
||||
// shared helper: the ledger marker performs the same one, so a relocated
|
||||
// blocker also rides the work list.
|
||||
const {
|
||||
deferred: deferredSuggestions,
|
||||
relocated: relocatedCriticals,
|
||||
relocatedDeterministic,
|
||||
} = splitDeferralChannel(input.deferredSuggestions);
|
||||
for (const stray of relocatedCriticals) {
|
||||
bodyCriticals.push(stray);
|
||||
}
|
||||
// The channel's OTHER precondition: deferring is only ever licensed by
|
||||
// the posture — `critical` at any round; `auto` from round 2 (the
|
||||
// code-age rule) and round 6 (the floor); never an explicit `suggestion`
|
||||
// (the operator turned the posture off) and never round 1 of `auto` (no
|
||||
// posture, no age reference). An unlicensed deferral is a model
|
||||
// mis-execution that would silently un-post findings — but the response
|
||||
// is a CAP, not a refusal: a thrown compose loses the WHOLE round,
|
||||
// Criticals included, and `prevRound` is a best-effort side-file read
|
||||
// whose every failure mode returns 0 — a missing file at a true round 6
|
||||
// must degrade to a disclosed, uncertified verdict, never to no verdict
|
||||
// at all. The findings render; the cap keeps anything from certifying
|
||||
// past them; the anchor is withheld with every other cap. The shape check
|
||||
// stays a refusal — a floor that is not one of the three values is a
|
||||
// malformed state file, same as a NaN count.
|
||||
// A floor the module does not recognise — absent, null, or a
|
||||
// model-transcribed spelling drift ("Critical", "auto ", "") — is folded
|
||||
// into ONE state: unknown. It caps as unlicensed when a deferral list
|
||||
// exists (fail-closed, disclosed) and is inert when it does not — a
|
||||
// refusal here would lose the whole round over a field that changes no
|
||||
// output on a zero-deferral run, the exact outcome the licence block is
|
||||
// written to avoid. Model-transcribed prose is not a NaN count.
|
||||
const floorRaw =
|
||||
typeof input.severityFloor === 'string'
|
||||
? input.severityFloor.trim().toLowerCase()
|
||||
: input.severityFloor;
|
||||
const floorKnown =
|
||||
floorRaw === 'critical' || floorRaw === 'suggestion' || floorRaw === 'auto';
|
||||
const floorAbsent = !floorKnown;
|
||||
const severityFloor: 'critical' | 'suggestion' | 'auto' = floorKnown
|
||||
? (floorRaw as 'critical' | 'suggestion' | 'auto')
|
||||
: 'auto';
|
||||
const cannotTell = toStringList(
|
||||
input.cannotTellCriticals,
|
||||
'cannotTellCriticals',
|
||||
|
|
@ -746,9 +1072,20 @@ function composeReviewBody(
|
|||
// pre-confirmed and skip verification. `[lint]` is NOT trusted as a tag — a
|
||||
// model-written string containing it must not launder an unverified claim into a
|
||||
// blocker (that is what the gate's provenance-tracked criticals are for).
|
||||
const nonDeterministicBodyCriticals = modelBodyCriticals.filter(
|
||||
(x) => !/\[(?:build|test|probe)\]/i.test(x),
|
||||
).length;
|
||||
// Relocated entries (the tail of `modelBodyCriticals` — pushed after the
|
||||
// input's own) are classified by the deferral channel's position-anchored
|
||||
// rule, counted in the split, not by the whole-entry tag scan the model's
|
||||
// own body Criticals get: they came in as deferral strings, and a
|
||||
// title-borne `[test]` must not exempt an unverified relocated claim from
|
||||
// the floor.
|
||||
const relocatedCount = relocatedCriticals.length;
|
||||
const ownBodyCriticals = modelBodyCriticals.slice(
|
||||
0,
|
||||
modelBodyCriticals.length - relocatedCount,
|
||||
);
|
||||
const nonDeterministicBodyCriticals =
|
||||
ownBodyCriticals.filter((x) => !DETERMINISTIC_TAG_RE.test(x)).length +
|
||||
(relocatedCount - relocatedDeterministic);
|
||||
const criticalsNeedingVerify =
|
||||
criticalsInline + nonDeterministicBodyCriticals;
|
||||
// Fail closed at every exit: this flag softens a Request changes below, and
|
||||
|
|
@ -941,8 +1278,24 @@ function composeReviewBody(
|
|||
// Its own try, so a read failure here says so rather than wearing the coverage
|
||||
// message, and does not undo a coverage pass a line above it.
|
||||
try {
|
||||
// Deferred findings count toward the delivery floor: they publish in
|
||||
// the body as the deferral list, and an unverified claim published as
|
||||
// "recorded, not requested" is still an unverified claim published — a
|
||||
// deferrals-only APPROVE must not slip past the verifier floor that a
|
||||
// posting run would have met. NON-DETERMINISTIC deferrals only, the
|
||||
// same exclusion the body Criticals get: a `[build]`/`[test]`/`[probe]`
|
||||
// finding is pre-confirmed and Step 4 launches no verifier for it, so
|
||||
// counting it demands a delivery that cannot exist — the cap never
|
||||
// lifts, the anchor is withheld every round, and the full-range
|
||||
// re-review loop the posture exists to end is regenerated by its own
|
||||
// enforcement. (Deferral entries carry their source tag for exactly
|
||||
// this scan — the SKILL's entry format.)
|
||||
const findingsToVerify =
|
||||
criticalsInline + suggestionsInline + nonDeterministicBodyCriticals;
|
||||
criticalsInline +
|
||||
suggestionsInline +
|
||||
nonDeterministicBodyCriticals +
|
||||
deferredSuggestions.filter((e) => !DETERMINISTIC_SOURCES.has(e.source))
|
||||
.length;
|
||||
const verification = verificationGaps(
|
||||
input.planPath,
|
||||
{ postsFindings: findingsToVerify > 0 },
|
||||
|
|
@ -1029,6 +1382,34 @@ function composeReviewBody(
|
|||
input.contextUnavailable,
|
||||
'contextUnavailable',
|
||||
);
|
||||
|
||||
// The deferral licence, decided here because two of its arms need inputs
|
||||
// parsed above: deferring is only ever licensed by the posture —
|
||||
// `critical` at any round; `auto` from round 2 (the code-age rule) and
|
||||
// round 6 (the floor); never an explicit `suggestion` (posture off),
|
||||
// never round 1 of `auto` (no posture, no age reference), never `auto` in
|
||||
// the context-unavailable state (the round is unknowable — SKILL resolves
|
||||
// it as round 1), and never with the field ABSENT beside a non-empty list
|
||||
// (the licence cannot be checked, and the channel ships in the same PR as
|
||||
// the field — omission is fail-closed, not grandfathered). The response
|
||||
// is a CAP, not a refusal: a thrown compose loses the whole round,
|
||||
// Criticals included, and `prevRound` is a best-effort side-file read
|
||||
// whose every failure mode returns 0 — a missing file at a true round 6
|
||||
// must degrade to a disclosed, uncertified verdict, never to no verdict
|
||||
// at all. The findings render; the cap keeps anything from certifying
|
||||
// past them; the anchor is withheld with every other cap.
|
||||
const unlicensedDeferral =
|
||||
deferredSuggestions.length === 0
|
||||
? null
|
||||
: floorAbsent
|
||||
? 'the state carried no recognisable `severityFloor`, so the licence cannot be checked'
|
||||
: severityFloor === 'suggestion'
|
||||
? 'the operator turned the posture off (`--severity-floor suggestion`)'
|
||||
: severityFloor === 'auto' && contextUnavailable
|
||||
? 'the round is unknowable in the context-unavailable state'
|
||||
: severityFloor === 'auto' && prevRound === 0
|
||||
? 'no posture is engaged on round 1 and no age reference exists'
|
||||
: null;
|
||||
const presubmitRaw: unknown = input.presubmit ?? {};
|
||||
if (typeof presubmitRaw !== 'object' || Array.isArray(presubmitRaw)) {
|
||||
throw new TypeError(
|
||||
|
|
@ -1088,6 +1469,7 @@ function composeReviewBody(
|
|||
cappedBy.push('unreviewed-dimension');
|
||||
}
|
||||
if (contextUnavailable) cappedBy.push('context-unavailable');
|
||||
if (unlicensedDeferral !== null) cappedBy.push('unlicensed-deferral');
|
||||
if (criticalsUnverified) cappedBy.push('criticals-unverified');
|
||||
if (findingsUnverifiedAtCompose) {
|
||||
cappedBy.push('findings-unverified-at-compose');
|
||||
|
|
@ -1163,7 +1545,14 @@ function composeReviewBody(
|
|||
// the field the topology is chosen from), so a docs-only or typo-class diff
|
||||
// keeps its bare Approve — there, finding nothing is the expected outcome.
|
||||
let lowSignal: ComposeReviewResult['lowSignal'] = null;
|
||||
if (event === 'APPROVE' && input.planPath) {
|
||||
// A deferrals-only APPROVE is not low signal: the agents DID report
|
||||
// findings — this run recorded them as deferred — and the low-signal
|
||||
// sentence's whole claim is that none reported any.
|
||||
if (
|
||||
event === 'APPROVE' &&
|
||||
input.planPath &&
|
||||
deferredSuggestions.length === 0
|
||||
) {
|
||||
let plan: RosterPlan | undefined;
|
||||
try {
|
||||
plan = JSON.parse(readFileSync(input.planPath, 'utf8')) as RosterPlan;
|
||||
|
|
@ -1545,6 +1934,53 @@ function composeReviewBody(
|
|||
]
|
||||
: [];
|
||||
|
||||
// Non-Critical findings the convergence posture deferred: disclosed on
|
||||
// EVERY event, never capping. The disclosure is the record the round
|
||||
// discipline demands — a deferral silently dropped is a finding lost, and
|
||||
// a deferral that capped would withhold the incremental anchor and
|
||||
// regenerate exactly the full-diff re-review the posture exists to end.
|
||||
// Entries are model-written: newlines collapse the way the cannot-tell
|
||||
// entries collapse, and the list is capped like the budget-gap lines — an
|
||||
// unbounded join would drown the verdict it rides on. The round number is
|
||||
// the same side-file read the ledger marker stamps (one read, passed in),
|
||||
// so the clause and the marker cannot disagree about which round deferred.
|
||||
// Both dimensions are bounded (module-scoped constants — verdictLine reads
|
||||
// the line cap too): entries are model-written with no upstream cap, and
|
||||
// twenty 4,000-char entries would put an ~80 KB block into a body GitHub
|
||||
// rejects outright at 65,536, losing the whole review over its own
|
||||
// footnote. 240 chars holds a `file:line — title` line with room to
|
||||
// spare; the findings artifact keeps every entry whole.
|
||||
const deferredShown = deferredSuggestions
|
||||
.slice(0, MAX_DEFERRED_SUGGESTION_LINES)
|
||||
.map(renderDeferredEntry)
|
||||
.map(boundDeferredLine);
|
||||
const deferredMore = deferredSuggestions.length - deferredShown.length;
|
||||
const deferredRound = deferredSuggestions.length ? prevRound + 1 : 0;
|
||||
// The unlicensed-deferral disclosure precedes the list it disclaims: the
|
||||
// findings stay visible, but nothing may read the paragraph below as a
|
||||
// sanctioned deferral when the posture never licensed one.
|
||||
const unlicensedDeferralBlock: Bi[] =
|
||||
unlicensedDeferral === null
|
||||
? []
|
||||
: [
|
||||
{
|
||||
en: `⚠️ ${deferredSuggestions.length} finding(s) were deferred without a posture licence — ${unlicensedDeferral}. They are listed below, but this verdict is capped: findings may be under-posted this round.`,
|
||||
zh: `⚠️ ${deferredSuggestions.length} 条发现在姿态未授权的情况下被延后——${unlicensedDeferral}。清单见下，但本判定已被限制：本轮发现可能未被完整发布。`,
|
||||
},
|
||||
];
|
||||
const deferredSuggestionsBlock: Bi[] = deferredSuggestions.length
|
||||
? [
|
||||
{
|
||||
en: `Deferred under the convergence posture (round ${deferredRound}, not a blocker) — recorded, not requested in this round:\n\n${deferredShown
|
||||
.map((entry) => `- ${mdField(entry)}`)
|
||||
.join(
|
||||
'\n',
|
||||
)}${deferredMore > 0 ? `\n- …and ${deferredMore} more (see the run report)` : ''}`,
|
||||
zh: `收敛姿态下延后（第 ${deferredRound} 轮，非阻断）——已记录，本轮不要求修改：共 ${deferredSuggestions.length} 条（原文未翻译，列表见上方英文部分）。`,
|
||||
},
|
||||
]
|
||||
: [];
|
||||
|
||||
if (event === 'REQUEST_CHANGES') {
|
||||
// Empty body, except the disclosures: every clause whose state holds
|
||||
// appears on every event — a confirmed blocker must not squeeze out the
|
||||
|
|
@ -1559,6 +1995,8 @@ function composeReviewBody(
|
|||
...deferredBlock,
|
||||
...testPlanBlock,
|
||||
...repositoryContextBlock,
|
||||
...unlicensedDeferralBlock,
|
||||
...deferredSuggestionsBlock,
|
||||
...bodyCriticalBlock,
|
||||
];
|
||||
return {
|
||||
|
|
@ -1569,6 +2007,7 @@ function composeReviewBody(
|
|||
downgraded,
|
||||
downgradedFrom,
|
||||
remediation,
|
||||
deferredCount: deferredSuggestions.length,
|
||||
lowSignal,
|
||||
};
|
||||
}
|
||||
|
|
@ -1580,20 +2019,28 @@ function composeReviewBody(
|
|||
// disclosure, not a defect — hiding "stopped at the tool budget" behind
|
||||
// an unqualified LGTM would break the one promise the disclosure channel
|
||||
// makes, that it reaches the author mechanically.
|
||||
// With posture-deferred Suggestions on record, "No issues found" would be
|
||||
// a lie the deferral list two lines down contradicts: the review DID find
|
||||
// them — it recorded them and chose, per the posture, not to request them.
|
||||
return {
|
||||
event,
|
||||
body: render(
|
||||
[
|
||||
{ en: 'No issues found. LGTM! ✅', zh: '未发现问题。LGTM！✅' },
|
||||
deferredSuggestionsBlock.length
|
||||
? { en: 'No blocking issues. LGTM! ✅', zh: '无阻断问题。LGTM！✅' }
|
||||
: { en: 'No issues found. LGTM! ✅', zh: '未发现问题。LGTM！✅' },
|
||||
...notReviewedParts,
|
||||
...deferredBlock,
|
||||
...testPlanBlock,
|
||||
...repositoryContextBlock,
|
||||
...unlicensedDeferralBlock,
|
||||
...deferredSuggestionsBlock,
|
||||
],
|
||||
notReviewedParts.length ||
|
||||
deferredBlock.length ||
|
||||
testPlanBlock.length ||
|
||||
repositoryContextBlock.length
|
||||
repositoryContextBlock.length ||
|
||||
deferredSuggestionsBlock.length
|
||||
? '\n\n'
|
||||
: ' ',
|
||||
),
|
||||
|
|
@ -1602,6 +2049,7 @@ function composeReviewBody(
|
|||
downgraded,
|
||||
downgradedFrom,
|
||||
remediation,
|
||||
deferredCount: deferredSuggestions.length,
|
||||
lowSignal,
|
||||
};
|
||||
}
|
||||
|
|
@ -1648,6 +2096,10 @@ function composeReviewBody(
|
|||
// shape the comment below forbids. (A gap the caller promoted into
|
||||
// `unreviewedDimensions` already denies certification above.)
|
||||
keptBudgetGaps.length === 0 &&
|
||||
// An unlicensed deferral withdrew findings from posting without a
|
||||
// licence — "no blockers" cannot open a body whose own ⚠️ clause says
|
||||
// findings may be under-posted.
|
||||
unlicensedDeferral === null &&
|
||||
!findingsUnverifiedAtCompose;
|
||||
// The opener may not say "Reviewed." over a disclosure set that denies it.
|
||||
// #7268's posted body opened exactly that way — "Reviewed. Suggestions are
|
||||
|
|
@ -1736,6 +2188,11 @@ function composeReviewBody(
|
|||
// planner recommends disclosing without claiming the code is defective.
|
||||
clauses.push(...repositoryContextBlock);
|
||||
|
||||
// 6e. Convergence-posture deferrals — the licence disclosure (capping)
|
||||
// precedes the list (non-capping).
|
||||
clauses.push(...unlicensedDeferralBlock);
|
||||
clauses.push(...deferredSuggestionsBlock);
|
||||
|
||||
// 7. Body Criticals — on a COMMENT that stands where a REQUEST_CHANGES
|
||||
// would have been: the presubmit carve-out, and the unverified-blockers
|
||||
// cap. Either way the body copy is the ONLY copy of an unanchorable
|
||||
|
|
@ -1764,6 +2221,7 @@ function composeReviewBody(
|
|||
downgraded,
|
||||
downgradedFrom,
|
||||
remediation,
|
||||
deferredCount: deferredSuggestions.length,
|
||||
lowSignal,
|
||||
};
|
||||
}
|
||||
|
|
@ -2487,6 +2945,7 @@ export function verdictLine(r: ComposeReviewResult): string {
|
|||
'uncoverable-chunk': 'part of the diff cannot be read at all',
|
||||
'unreviewed-dimension': 'a dimension nobody reviewed',
|
||||
'context-unavailable': "the PR's existing discussion could not be read",
|
||||
'unlicensed-deferral': 'findings were deferred without a posture licence',
|
||||
'findings-unverified-at-compose':
|
||||
'findings were still unverified when the loop ended',
|
||||
};
|
||||
|
|
@ -2551,5 +3010,19 @@ export function verdictLine(r: ComposeReviewResult): string {
|
|||
`reported a finding on a non-trivial diff ` +
|
||||
`(${r.lowSignal.srcDiffLines} source diff lines)`;
|
||||
}
|
||||
// Deferrals are findings the run stands behind and chose not to request;
|
||||
// a verdict line that omits them reads as "nothing was found" on exactly
|
||||
// the runs the posture targets. `lowSignal` is mutually exclusive with
|
||||
// this by construction — a deferrals-only APPROVE never sets it. The
|
||||
// "(listed in the body)" claim turns cap-aware past the rendered line
|
||||
// cap: a verdict counting 21 over a body listing 20 is a false record,
|
||||
// persisted into the composed JSON and the archived report.
|
||||
if (r.deferredCount > 0) {
|
||||
line += ` — ${r.deferredCount} non-Critical finding(s) deferred under the convergence posture (listed in the body${
|
||||
r.deferredCount > MAX_DEFERRED_SUGGESTION_LINES
|
||||
? ', truncated — the rest are counted in the run report'
|
||||
: ''
|
||||
})`;
|
||||
}
|
||||
return line;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -53,11 +53,17 @@ describe('operatorReviewSettings', () => {
|
|||
});
|
||||
|
||||
it('explicit values pass through unchanged', () => {
|
||||
setReview({ attribution: false, comment: true, effort: 'low' });
|
||||
setReview({
|
||||
attribution: false,
|
||||
comment: true,
|
||||
effort: 'low',
|
||||
severityFloor: 'critical',
|
||||
});
|
||||
expect(operatorReviewSettings()).toEqual({
|
||||
attribution: false,
|
||||
comment: true,
|
||||
effort: 'low',
|
||||
severityFloor: 'critical',
|
||||
});
|
||||
});
|
||||
|
||||
|
|
@ -71,6 +77,13 @@ describe('operatorReviewSettings', () => {
|
|||
expect(operatorReviewSettings().effort).toBeUndefined();
|
||||
});
|
||||
|
||||
it('severityFloor rides the same raw-passthrough contract as effort', () => {
|
||||
setReview({ severityFloor: 'Auto' });
|
||||
expect(operatorReviewSettings().severityFloor).toBe('Auto');
|
||||
setReview({ severityFloor: 42 });
|
||||
expect(operatorReviewSettings().severityFloor).toBeUndefined();
|
||||
});
|
||||
|
||||
it('a hand-edited non-boolean attribution falls back to the schema default', () => {
|
||||
// The quoted-JSON classic: `"attribution": "false"` is a truthy string
|
||||
// — `?? true` once handed it straight to the truthiness checks and the
|
||||
|
|
@ -92,7 +105,7 @@ describe('operatorReviewSettings', () => {
|
|||
});
|
||||
|
||||
describe('review settings in the /settings dialog', () => {
|
||||
it('exposes all three settings for toggling', () => {
|
||||
it('exposes all four settings for toggling', () => {
|
||||
// Maintainer A/B verification of this PR caught the description claiming
|
||||
// dialog membership while the schema shipped showInDialog: false. Pin the
|
||||
// membership so the claim and the schema cannot drift again.
|
||||
|
|
@ -100,5 +113,6 @@ describe('review settings in the /settings dialog', () => {
|
|||
expect(dialogKeys).toContain('review.attribution');
|
||||
expect(dialogKeys).toContain('review.effort');
|
||||
expect(dialogKeys).toContain('review.comment');
|
||||
expect(dialogKeys).toContain('review.severityFloor');
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -15,6 +15,8 @@ export interface OperatorReviewSettings {
|
|||
* at all. Callers normalize.
|
||||
*/
|
||||
effort?: string;
|
||||
/** The raw `review.severityFloor` value when set — same caveats as effort. */
|
||||
severityFloor?: string;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -38,5 +40,9 @@ export function operatorReviewSettings(): OperatorReviewSettings {
|
|||
typeof review?.attribution === 'boolean' ? review.attribution : true,
|
||||
comment: review?.comment === true,
|
||||
effort: typeof review?.effort === 'string' ? review.effort : undefined,
|
||||
severityFloor:
|
||||
typeof review?.severityFloor === 'string'
|
||||
? review.severityFloor
|
||||
: undefined,
|
||||
};
|
||||
}
|
||||
|
|
|
|||
|
|
@ -355,6 +355,270 @@ describe('parseReviewArgs', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('parseReviewArgs — --severity-floor (the convergence posture knob)', () => {
|
||||
it('defaults to auto: the round-adaptive rule is resolved at Step 6, not here', () => {
|
||||
const got = parseReviewArgs('6711');
|
||||
expect(got.severityFloor).toBe('auto');
|
||||
expect(got.severityFloorSource).toBe('default');
|
||||
});
|
||||
|
||||
it('parses both forms case-insensitively; the last valid occurrence wins', () => {
|
||||
expect(parseReviewArgs('6711 --severity-floor critical')).toMatchObject({
|
||||
severityFloor: 'critical',
|
||||
severityFloorSource: 'explicit',
|
||||
});
|
||||
expect(parseReviewArgs('6711 --severity-floor=Suggestion')).toMatchObject({
|
||||
severityFloor: 'suggestion',
|
||||
});
|
||||
expect(
|
||||
parseReviewArgs(
|
||||
'6711 --severity-floor critical --severity-floor suggestion',
|
||||
),
|
||||
).toMatchObject({ severityFloor: 'suggestion' });
|
||||
});
|
||||
|
||||
it('warns and ignores the flag on a non-PR target, exactly as --comment does', () => {
|
||||
const got = parseReviewArgs('src/foo.ts --severity-floor critical');
|
||||
expect(got.target.type).toBe('file');
|
||||
expect(got.severityFloor).toBe('auto');
|
||||
expect(got.severityFloorSource).toBe('default');
|
||||
expect(got.warnings.some((w) => w.includes('--severity-floor'))).toBe(true);
|
||||
});
|
||||
|
||||
it('an invalid value warns naming what is in effect, and never eats the target', () => {
|
||||
// The typo is discarded when another token is the target — without the
|
||||
// disposal rule, `criticl` would classify as a file path and shadow the
|
||||
// real PR target that follows it.
|
||||
const got = parseReviewArgs('--severity-floor criticl 6711');
|
||||
expect(got.target).toEqual({ type: 'pr-number', number: 6711 });
|
||||
expect(got.severityFloor).toBe('auto');
|
||||
expect(
|
||||
got.warnings.some(
|
||||
(w) =>
|
||||
w.includes('Invalid --severity-floor value "criticl"') &&
|
||||
w.includes('round-adaptive default'),
|
||||
),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it('an invalid equals-form value warns instead of vanishing', () => {
|
||||
// Mutation-verified gap: replacing the invalid-eq push with a bare
|
||||
// `continue` left the whole suite green — an operator who typed the flag
|
||||
// believing round 6 went Critical-only would get Suggestions posted with
|
||||
// nothing saying the flag never took effect.
|
||||
const got = parseReviewArgs('6711 --severity-floor=critcl');
|
||||
expect(got.target).toEqual({ type: 'pr-number', number: 6711 });
|
||||
expect(got.severityFloor).toBe('auto');
|
||||
expect(
|
||||
got.warnings.some((w) =>
|
||||
w.includes('Invalid --severity-floor value "critcl"'),
|
||||
),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it('a sole invalid value becomes the target, and the warning says so', () => {
|
||||
const got = parseReviewArgs('--severity-floor criticl');
|
||||
expect(got.target).toEqual({ type: 'file', path: 'criticl' });
|
||||
expect(
|
||||
got.warnings.some(
|
||||
(w) =>
|
||||
w.includes('Invalid --severity-floor value "criticl"') &&
|
||||
w.includes('treating it as the review target'),
|
||||
),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it('an unrelated typo never changes WHICH codebase is reviewed', () => {
|
||||
// `--effort 6711` reviews PR 6711 past a flag mistake; adding a second
|
||||
// malformed flag must not silently retarget the review at the local
|
||||
// diff. PR-shaped values survive disposal; enum-typo-shaped ones do not.
|
||||
const got = parseReviewArgs('--severity-floor blocker --effort 6711');
|
||||
expect(got.target).toEqual({ type: 'pr-number', number: 6711 });
|
||||
expect(
|
||||
got.warnings.some(
|
||||
(w) => w.includes('"blocker"') && w.includes('discarded'),
|
||||
),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it('a typed target outranks a PR-shaped flag value', () => {
|
||||
// With a real positional target present, `--effort 6712` is a typo, not
|
||||
// a second target — keeping it would make the kept-as-target warning
|
||||
// lie about which PR is reviewed.
|
||||
const got = parseReviewArgs('6711 --effort 6712');
|
||||
expect(got.target).toEqual({ type: 'pr-number', number: 6711 });
|
||||
expect(got.extraTokens).toEqual([]);
|
||||
expect(
|
||||
got.warnings.some((w) => w.includes('"6712"') && w.includes('discarded')),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it('two PR-shaped flag values are ambiguous — refused, never first-wins', () => {
|
||||
// `--severity-floor 6711 --effort 6712`: silently reviewing 6711 would
|
||||
// review the wrong PR half the time. Both are discarded with a warning
|
||||
// that names them, and the review falls back to the local diff.
|
||||
const got = parseReviewArgs('--severity-floor 6711 --effort 6712');
|
||||
expect(got.target).toEqual({ type: 'local' });
|
||||
expect(got.extraTokens).toEqual([]);
|
||||
expect(
|
||||
got.warnings.some(
|
||||
(w) =>
|
||||
w.includes('Ambiguous target') &&
|
||||
w.includes('"6711"') &&
|
||||
w.includes('"6712"'),
|
||||
),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it('the ambiguity guard covers the equals form — syntax does not pick a PR', () => {
|
||||
// Round-7 probe: the eq form never enters the disposal pool, so
|
||||
// `--severity-floor=6711 --effort 6712` silently targeted 6712 while
|
||||
// the all-spaced spelling was loudly refused. All four spellings must
|
||||
// land the same place.
|
||||
for (const raw of [
|
||||
'--severity-floor 6711 --effort 6712',
|
||||
'--severity-floor=6711 --effort 6712',
|
||||
'--severity-floor 6711 --effort=6712',
|
||||
'--severity-floor=6711 --effort=6712',
|
||||
]) {
|
||||
const got = parseReviewArgs(raw);
|
||||
expect(got.target).toEqual({ type: 'local' });
|
||||
expect(got.warnings.some((w) => w.includes('Ambiguous target'))).toBe(
|
||||
true,
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
it('the equals form rescues a PR-shaped value exactly as the spaced form does', () => {
|
||||
// Round-8 probe: `--severity-floor=6711` reviewed the LOCAL tree while
|
||||
// `--severity-floor 6711` rescued PR 6711 — the guard's invariant
|
||||
// ("which codebase is reviewed cannot depend on which syntax happened
|
||||
// to be typed") was wired into refusal only. Every spelling converges.
|
||||
for (const raw of [
|
||||
'--severity-floor=6711',
|
||||
'--severity-floor 6711',
|
||||
'--effort=6711',
|
||||
'--severity-floor blocker --effort=6711',
|
||||
'--severity-floor blocker --effort 6711',
|
||||
]) {
|
||||
expect(parseReviewArgs(raw).target).toEqual({
|
||||
type: 'pr-number',
|
||||
number: 6711,
|
||||
});
|
||||
}
|
||||
// Two spellings of the SAME PR are one candidate, not an ambiguity —
|
||||
// and not an extra argument either: the restated spelling must not
|
||||
// surface as `extraTokens` / "Ignoring extra argument(s)" (round-9).
|
||||
const dup = parseReviewArgs('--severity-floor 6711 --effort=6711');
|
||||
expect(dup.target).toEqual({ type: 'pr-number', number: 6711 });
|
||||
expect(dup.warnings.some((w) => w.includes('Ambiguous'))).toBe(false);
|
||||
expect(dup.extraTokens).toEqual([]);
|
||||
expect(dup.warnings.some((w) => w.includes('Ignoring extra'))).toBe(false);
|
||||
// Identity is the resolved TARGET, not the raw string: a bare number and
|
||||
// a same-number URL name one PR (round-9 finding — a raw-token Set read
|
||||
// them as two and fell back to the local tree).
|
||||
const mixed = parseReviewArgs(
|
||||
'--severity-floor 6711 --effort https://github.com/QwenLM/qwen-code/pull/6711',
|
||||
);
|
||||
expect(mixed.target).toMatchObject({ number: 6711 });
|
||||
expect(mixed.warnings.some((w) => w.includes('Ambiguous'))).toBe(false);
|
||||
expect(mixed.extraTokens).toEqual([]);
|
||||
});
|
||||
|
||||
it('an invalid configured floor stays silent on a non-PR target', () => {
|
||||
// Round-8 mutant: deleting the `&& isPr` gate warned every file/local
|
||||
// review about a floor that is inert there by design.
|
||||
const got = parseReviewArgs('src/foo.ts', { severityFloor: 'blocker' });
|
||||
expect(got.severityFloor).toBe('auto');
|
||||
expect(got.warnings).toEqual([]);
|
||||
});
|
||||
|
||||
it('an explicit auto floor is legal and overrides a configured floor for one run', () => {
|
||||
// Mutation-shown gap: dropping 'auto' from SEVERITY_FLOORS shipped
|
||||
// green while the documented one-shot override was rejected and, alone,
|
||||
// promoted to a bogus `auto` file target.
|
||||
expect(
|
||||
parseReviewArgs('6711 --severity-floor auto', {
|
||||
severityFloor: 'critical',
|
||||
}),
|
||||
).toMatchObject({ severityFloor: 'auto', severityFloorSource: 'explicit' });
|
||||
const sole = parseReviewArgs('--severity-floor auto');
|
||||
expect(sole.target).toEqual({ type: 'local' });
|
||||
});
|
||||
|
||||
it('a quoted-empty value is consumed as missing on both flags', () => {
|
||||
// Mutation-shown gap: with the consumption branch deleted, '' survived
|
||||
// as the sole candidate and became an empty-string file target.
|
||||
for (const raw of ['--severity-floor ""', '--effort ""']) {
|
||||
const got = parseReviewArgs(raw);
|
||||
expect(got.target).toEqual({ type: 'local' });
|
||||
expect(got.warnings.some((w) => w.includes('requires a value'))).toBe(
|
||||
true,
|
||||
);
|
||||
}
|
||||
const withTarget = parseReviewArgs('6711 --severity-floor ""');
|
||||
expect(withTarget.target).toEqual({ type: 'pr-number', number: 6711 });
|
||||
expect(
|
||||
withTarget.warnings.some((w) => w.includes('requires a value')),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it('two invalid values are two typos, not a target and a tiebreak', () => {
|
||||
// "Sole target candidate" is literal: with two invalid tokens neither is
|
||||
// sole, so both are discarded and the review falls back to the local
|
||||
// diff — promoting the first to a file target would send the caller off
|
||||
// to stat `blocker`.
|
||||
const got = parseReviewArgs(
|
||||
'--severity-floor blocker --severity-floor warning',
|
||||
);
|
||||
expect(got.target).toEqual({ type: 'local' });
|
||||
expect(got.extraTokens).toEqual([]);
|
||||
expect(got.warnings.filter((w) => w.includes('discarded')).length).toBe(2);
|
||||
});
|
||||
|
||||
it('flag-final or flag-followed is a missing value, never a consumed flag', () => {
|
||||
const got = parseReviewArgs('6711 --severity-floor --comment');
|
||||
expect(got.comment.requested).toBe(true);
|
||||
expect(got.severityFloor).toBe('auto');
|
||||
expect(
|
||||
got.warnings.some((w) => w.includes('--severity-floor requires a value')),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it('applies the configured review.severityFloor; an explicit flag still wins', () => {
|
||||
expect(
|
||||
parseReviewArgs('6711', { severityFloor: 'critical' }),
|
||||
).toMatchObject({
|
||||
severityFloor: 'critical',
|
||||
severityFloorSource: 'configured',
|
||||
});
|
||||
expect(
|
||||
parseReviewArgs('6711 --severity-floor suggestion', {
|
||||
severityFloor: 'critical',
|
||||
}),
|
||||
).toMatchObject({
|
||||
severityFloor: 'suggestion',
|
||||
severityFloorSource: 'explicit',
|
||||
});
|
||||
});
|
||||
|
||||
it('a configured floor is silently inert on a non-PR target', () => {
|
||||
const got = parseReviewArgs('src/foo.ts', { severityFloor: 'critical' });
|
||||
expect(got.severityFloor).toBe('auto');
|
||||
expect(got.warnings).toHaveLength(0);
|
||||
});
|
||||
|
||||
it('an invalid configured floor warns on a PR target instead of dropping silently', () => {
|
||||
const got = parseReviewArgs('6711', { severityFloor: 'blocker' });
|
||||
expect(got.severityFloor).toBe('auto');
|
||||
expect(
|
||||
got.warnings.some((w) =>
|
||||
w.includes('Invalid review.severityFloor value "blocker"'),
|
||||
),
|
||||
).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('parseReviewArgs — settings-provided defaults', () => {
|
||||
it('applies the configured effort when --effort is absent', () => {
|
||||
const got = parseReviewArgs('6711', { effort: 'medium' });
|
||||
|
|
@ -821,6 +1085,38 @@ describe('parseArgsCommand — configured defaults wiring', () => {
|
|||
).toBe(true);
|
||||
});
|
||||
|
||||
it('a configured severityFloor reaches the verdict through the handler', async () => {
|
||||
// Deleting the severityFloor line of reviewDefaultsFromSettings leaves
|
||||
// every pure-parser test green while production silently ignores the
|
||||
// setting — same seam as the effort/comment cases above.
|
||||
reviewSettingsMock.mockReturnValue({ severityFloor: 'critical' });
|
||||
const got = await verdictFor('6711\n');
|
||||
expect(got.severityFloor).toBe('critical');
|
||||
expect(got.severityFloorSource).toBe('configured');
|
||||
});
|
||||
|
||||
it('discards an invalid configured severityFloor, warning instead of dropping it silently', async () => {
|
||||
// Parity with the effort twin: a settings-layer "validation" that
|
||||
// silently dropped non-enum values would leave every pure-parser test
|
||||
// green while the operator's typo takes effect as silence.
|
||||
reviewSettingsMock.mockReturnValue({ severityFloor: 'bogus' });
|
||||
const got = await verdictFor('6711\n');
|
||||
expect(got.severityFloor).toBe('auto');
|
||||
expect(
|
||||
got.warnings.some((w) =>
|
||||
w.includes('Invalid review.severityFloor value "bogus" in settings'),
|
||||
),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it('maps a configured auto severityFloor to the round-adaptive default without warning', async () => {
|
||||
reviewSettingsMock.mockReturnValue({ severityFloor: 'Auto' });
|
||||
const got = await verdictFor('6711\n');
|
||||
expect(got.severityFloor).toBe('auto');
|
||||
expect(got.severityFloorSource).toBe('default');
|
||||
expect(got.warnings).toHaveLength(0);
|
||||
});
|
||||
|
||||
it('ignores workspace settings — the policy keys resolve from operator scopes only', async () => {
|
||||
// The mock answers a flag-less loadSettings call with a workspace-
|
||||
// polluted view (comment on, low effort); the handler's
|
||||
|
|
|
|||
|
|
@ -28,6 +28,15 @@ import { bundleStalenessNotices } from './lib/stale-bundle.js';
|
|||
|
||||
export type ReviewEffort = 'low' | 'medium' | 'high';
|
||||
|
||||
/**
|
||||
* The posting floor for findings on a PR review: `critical` posts only
|
||||
* Critical findings (otherwise-postable high-confidence Suggestions are
|
||||
* recorded and deferred; low-confidence and Nice-to-have stay terminal-only
|
||||
* as ever), `suggestion` posts Criticals and Suggestions — today's behaviour. The floor governs what
|
||||
* the review PUBLISHES, never what it finds or verifies.
|
||||
*/
|
||||
export type ReviewSeverityFloor = 'critical' | 'suggestion';
|
||||
|
||||
export type ReviewTarget =
|
||||
| { type: 'pr-number'; number: number }
|
||||
| {
|
||||
|
|
@ -79,6 +88,17 @@ export interface ParsedReviewArgs {
|
|||
/** `--fix` applies (the target has a durable working tree). */
|
||||
effective: boolean;
|
||||
};
|
||||
/**
|
||||
* The posting floor, or `'auto'` — the round-adaptive default, resolved at
|
||||
* Step 6 where the round is known (`suggestion` through round 5, `critical`
|
||||
* from round 6). The parser cannot resolve `auto` itself: the round comes
|
||||
* from the previous posted round's ledger, which is not fetched yet. An
|
||||
* explicit `--severity-floor` on a non-PR target is ignored with a warning,
|
||||
* exactly as `--comment` is — the floor is a posting rule, and rounds exist
|
||||
* only for PRs.
|
||||
*/
|
||||
severityFloor: ReviewSeverityFloor | 'auto';
|
||||
severityFloorSource: 'explicit' | 'configured' | 'default';
|
||||
/** Non-flag tokens beyond the first target token, reported not guessed. */
|
||||
extraTokens: string[];
|
||||
/** Unrecognized `--flags`, reported not guessed. */
|
||||
|
|
@ -92,6 +112,16 @@ export const EFFORT_LEVELS: ReadonlySet<string> = new Set([
|
|||
'high',
|
||||
]);
|
||||
|
||||
export const SEVERITY_FLOORS: ReadonlySet<string> = new Set([
|
||||
'critical',
|
||||
'suggestion',
|
||||
// `auto` is a legal EXPLICIT value too — it is the schema-enumerated
|
||||
// default's name, so an operator typing `--severity-floor auto` means
|
||||
// "the round-adaptive rule" (overriding a configured floor), not a typo
|
||||
// to reject and then promote into a bogus file target.
|
||||
'auto',
|
||||
]);
|
||||
|
||||
// The verdict's owner/repo/number are interpolated into `gh` commands by the
|
||||
// caller, so they must be established trustworthily, not merely extracted:
|
||||
// the scheme is case-insensitive, the number must END at the path segment
|
||||
|
|
@ -112,6 +142,13 @@ function asEffort(value: string): ReviewEffort | null {
|
|||
return EFFORT_LEVELS.has(lower) ? (lower as ReviewEffort) : null;
|
||||
}
|
||||
|
||||
function asSeverityFloor(value: string): ReviewSeverityFloor | 'auto' | null {
|
||||
const lower = value.toLowerCase();
|
||||
return SEVERITY_FLOORS.has(lower)
|
||||
? (lower as ReviewSeverityFloor | 'auto')
|
||||
: null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Single-dash tokens count as flags too: `-c` is never a plausible review
|
||||
* target, and classifying it as a file path demoted the real target the
|
||||
|
|
@ -125,6 +162,16 @@ function isPureInteger(token: string): boolean {
|
|||
return /^\d+$/.test(token);
|
||||
}
|
||||
|
||||
/** A token that classifies as a PR target (number or PR URL). */
|
||||
function isPrShapedToken(token: string): boolean {
|
||||
const shape = classifyToken(token);
|
||||
return (
|
||||
shape !== null &&
|
||||
shape !== 'invalid-url' &&
|
||||
(shape.type === 'pr-number' || shape.type === 'pr-url')
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Split a raw argument string on whitespace, honouring double- and
|
||||
* single-quoted segments so file paths with spaces survive.
|
||||
|
|
@ -206,6 +253,12 @@ export function parseReviewArgs(
|
|||
* authorises only the PR the arguments name.
|
||||
*/
|
||||
comment?: boolean;
|
||||
/**
|
||||
* The standing `review.severityFloor` setting, raw (`auto` already mapped
|
||||
* to undefined by the caller). Validated exactly like the flag — a typo
|
||||
* warns and falls back to the round-adaptive default.
|
||||
*/
|
||||
severityFloor?: string;
|
||||
} = {},
|
||||
): ParsedReviewArgs {
|
||||
const tokens = tokenizeArgs(raw);
|
||||
|
|
@ -215,6 +268,7 @@ export function parseReviewArgs(
|
|||
let commentRequestedByFlag = false;
|
||||
let fixRequested = false;
|
||||
let explicitEffort: ReviewEffort | null = null;
|
||||
let explicitFloor: ReviewSeverityFloor | 'auto' | null = null;
|
||||
|
||||
// The configured default gets the same validation as an explicit flag:
|
||||
// settings loading performs no enum validation, so a hand-edited typo
|
||||
|
|
@ -231,6 +285,20 @@ export function parseReviewArgs(
|
|||
invalidConfiguredEffort = defaults.effort;
|
||||
}
|
||||
}
|
||||
let configuredFloor: ReviewSeverityFloor | undefined;
|
||||
let invalidConfiguredFloor: string | undefined;
|
||||
if (defaults.severityFloor !== undefined) {
|
||||
const normalized = asSeverityFloor(defaults.severityFloor);
|
||||
if (normalized === 'auto') {
|
||||
// The default's own name: the same round-adaptive rule as an unset
|
||||
// setting. The settings caller pre-maps it, but a direct caller must
|
||||
// get identical semantics.
|
||||
} else if (normalized !== null) {
|
||||
configuredFloor = normalized;
|
||||
} else {
|
||||
invalidConfiguredFloor = defaults.severityFloor;
|
||||
}
|
||||
}
|
||||
|
||||
// Warnings about a rejected `--effort` occurrence must state what effort
|
||||
// is ACTUALLY in effect — which is not known until every occurrence is
|
||||
|
|
@ -244,15 +312,19 @@ export function parseReviewArgs(
|
|||
| { kind: 'discarded'; value: string }
|
||||
| { kind: 'kept-as-target'; value: string };
|
||||
const effortIssues: EffortIssue[] = [];
|
||||
// `--severity-floor` shares the value-token grammar and therefore the same
|
||||
// deferred-warning problem; its issues are a separate list because its
|
||||
// resolution sentence is its own.
|
||||
const floorIssues: EffortIssue[] = [];
|
||||
|
||||
// First pass: pull out flags (and each `--effort`'s value token, when the
|
||||
// spaced form legitimately consumes one). Non-flag tokens are kept in
|
||||
// order; invalid spaced `--effort` values are kept as *candidates* whose
|
||||
// disposal is decided after we know whether any other token is the target.
|
||||
// First pass: pull out flags (and each value-taking flag's value token,
|
||||
// when the spaced form legitimately consumes one). Non-flag tokens are kept
|
||||
// in order; invalid spaced values are kept as *candidates* whose disposal
|
||||
// is decided after we know whether any other token is the target.
|
||||
interface Kept {
|
||||
token: string;
|
||||
/** True when this token arrived as an invalid `--effort` value. */
|
||||
fromInvalidEffortValue: boolean;
|
||||
/** Set when this token arrived as an invalid value of the named flag. */
|
||||
invalidValueOf?: '--effort' | '--severity-floor';
|
||||
}
|
||||
const kept: Kept[] = [];
|
||||
|
||||
|
|
@ -276,6 +348,12 @@ export function parseReviewArgs(
|
|||
const effortValue = asEffort(value);
|
||||
if (effortValue !== null) {
|
||||
explicitEffort = effortValue;
|
||||
} else if (value !== '' && isPrShapedToken(value)) {
|
||||
// A PR-shaped value in either flag syntax is the same typo of the
|
||||
// same intent — it joins the disposal pool exactly as the spaced
|
||||
// form does, so which codebase gets reviewed cannot depend on
|
||||
// which syntax happened to be typed (round-8 review finding).
|
||||
kept.push({ token: value, invalidValueOf: '--effort' });
|
||||
} else {
|
||||
effortIssues.push({ kind: 'invalid-eq', value });
|
||||
}
|
||||
|
|
@ -288,6 +366,14 @@ export function parseReviewArgs(
|
|||
i++;
|
||||
continue;
|
||||
}
|
||||
// A quoted-empty value ('' survives tokenization) is a missing value,
|
||||
// not a candidate — and it is CONSUMED, or the leftover '' token would
|
||||
// classify as an empty-string file target.
|
||||
if (next === '') {
|
||||
effortIssues.push({ kind: 'missing' });
|
||||
i++;
|
||||
continue;
|
||||
}
|
||||
if (next === undefined || isFlag(next)) {
|
||||
// Flag-final, or followed by another flag: the value is simply
|
||||
// missing. Never consume a flag as a value.
|
||||
|
|
@ -297,7 +383,41 @@ export function parseReviewArgs(
|
|||
// Spaced form with an invalid non-flag value. Whether `next` is a
|
||||
// discarded typo or the review target is decided below, once we know
|
||||
// whether any other token can be the target.
|
||||
kept.push({ token: next, fromInvalidEffortValue: true });
|
||||
kept.push({ token: next, invalidValueOf: '--effort' });
|
||||
i++;
|
||||
continue;
|
||||
}
|
||||
|
||||
if (token === '--severity-floor' || token.startsWith('--severity-floor=')) {
|
||||
if (token.includes('=')) {
|
||||
const value = token.slice(token.indexOf('=') + 1);
|
||||
const floorValue = asSeverityFloor(value);
|
||||
if (floorValue !== null) {
|
||||
explicitFloor = floorValue;
|
||||
} else if (value !== '' && isPrShapedToken(value)) {
|
||||
kept.push({ token: value, invalidValueOf: '--severity-floor' });
|
||||
} else {
|
||||
floorIssues.push({ kind: 'invalid-eq', value });
|
||||
}
|
||||
continue;
|
||||
}
|
||||
const next = i + 1 < tokens.length ? tokens[i + 1] : undefined;
|
||||
const nextFloor = next !== undefined ? asSeverityFloor(next) : null;
|
||||
if (nextFloor !== null) {
|
||||
explicitFloor = nextFloor;
|
||||
i++;
|
||||
continue;
|
||||
}
|
||||
if (next === '') {
|
||||
floorIssues.push({ kind: 'missing' });
|
||||
i++;
|
||||
continue;
|
||||
}
|
||||
if (next === undefined || isFlag(next)) {
|
||||
floorIssues.push({ kind: 'missing' });
|
||||
continue;
|
||||
}
|
||||
kept.push({ token: next, invalidValueOf: '--severity-floor' });
|
||||
i++;
|
||||
continue;
|
||||
}
|
||||
|
|
@ -308,21 +428,88 @@ export function parseReviewArgs(
|
|||
continue;
|
||||
}
|
||||
|
||||
kept.push({ token, fromInvalidEffortValue: false });
|
||||
kept.push({ token });
|
||||
}
|
||||
|
||||
// Disposal rule for invalid `--effort` values: a typo is discarded when
|
||||
// any *other* token is the target; it survives only when it is itself the
|
||||
// sole target candidate (`/review --effort 6711`).
|
||||
const hasOtherCandidate = kept.some((k) => !k.fromInvalidEffortValue);
|
||||
// Disposal rule for invalid flag values, by what the token could BE:
|
||||
// - PR-shaped (a pure number, a PR URL) survives as a target candidate —
|
||||
// `/review --effort 6711` is a user reviewing PR 6711 past a flag
|
||||
// mistake, and an unrelated second typo (`--severity-floor blocker
|
||||
// --effort 6711`) must not change WHICH codebase is reviewed.
|
||||
// - File-shaped survives only as the SOLE kept token (`/review
|
||||
// --severity-floor criticl`): beside anything else, `blocker` is an
|
||||
// enum typo, not a path — promoting it would send the caller off to
|
||||
// stat nonsense, and two such typos are two typos, not a target and a
|
||||
// tiebreak.
|
||||
// A token the user typed OUTSIDE any flag always outranks a flag value:
|
||||
// when one exists, every invalid value is a typo beside the real target.
|
||||
// And a PR-shaped rescue must be UNIQUE: two PR-shaped values arriving as
|
||||
// invalid flag values (`--severity-floor 6711 --effort 6712`) are an
|
||||
// ambiguous invocation — silently reviewing the first would review the
|
||||
// wrong PR half the time, so both are refused, loudly, and the review
|
||||
// falls back to the local diff nothing contradicted.
|
||||
const soleCandidate = kept.length === 1;
|
||||
const hasValidCandidate = kept.some((k) => k.invalidValueOf === undefined);
|
||||
const isPrShaped = isPrShapedToken;
|
||||
// The pool counts BOTH spellings: an `=`-form invalid value never enters
|
||||
// `kept` (it was recorded as `invalid-eq` and consumed in place), but it
|
||||
// is the same typed PR number — `--severity-floor=6711 --effort 6712`
|
||||
// must be exactly as ambiguous as the all-spaced spelling, or the guard
|
||||
// is defeated by which syntax happened to be typed.
|
||||
// Distinct TARGETS, not distinct strings: `--severity-floor 6711 --effort
|
||||
// https://github.com/o/r/pull/6711` are two spellings of one PR and are
|
||||
// unambiguous (round-9 finding: a raw-token Set read them as two and
|
||||
// silently fell back to the local tree — the very harm the guard exists
|
||||
// to prevent). The identity is the classified target's number plus, for
|
||||
// a URL, its host/owner/repo. Both flag syntaxes already landed in `kept`
|
||||
// above, so the pool sees every spelling.
|
||||
const targetKey = (token: string): string => {
|
||||
const shape = classifyToken(token);
|
||||
if (shape === null || shape === 'invalid-url') return `raw:${token}`;
|
||||
if (shape.type === 'pr-number') return `pr:${shape.number}`;
|
||||
if (shape.type === 'pr-url')
|
||||
return `pr:${shape.number}@${shape.host}/${shape.owner}/${shape.repo}`;
|
||||
return `raw:${token}`;
|
||||
};
|
||||
const prShapedKeys = [
|
||||
...new Set(
|
||||
kept
|
||||
.filter((k) => k.invalidValueOf !== undefined && isPrShaped(k.token))
|
||||
.map((k) => targetKey(k.token)),
|
||||
),
|
||||
];
|
||||
// A bare number and a same-number URL name one PR when no other repo is
|
||||
// in play: collapse `pr:N` into `pr:N@…` for the count.
|
||||
const distinctPr = new Set(prShapedKeys.map((k) => k.replace(/@.*$/, '')));
|
||||
if (!hasValidCandidate && distinctPr.size > 1) {
|
||||
const shown = kept
|
||||
.filter((k) => k.invalidValueOf !== undefined && isPrShaped(k.token))
|
||||
.map((k) => JSON.stringify(k.token));
|
||||
warnings.push(
|
||||
`Ambiguous target: ${shown.join(' and ')} arrived as invalid flag values and name different PRs; refusing to choose between them.`,
|
||||
);
|
||||
}
|
||||
const targetTokens: string[] = [];
|
||||
// Of several spellings of the SAME rescued PR, exactly one becomes the
|
||||
// target; the rest are the same intent restated, not extra arguments —
|
||||
// pushing them all left the operator told "Ignoring extra argument(s)"
|
||||
// on the very invocation the dedupe blesses (round-9 finding).
|
||||
let rescuedPr = false;
|
||||
for (const k of kept) {
|
||||
if (k.fromInvalidEffortValue && hasOtherCandidate) {
|
||||
effortIssues.push({ kind: 'discarded', value: k.token });
|
||||
continue;
|
||||
}
|
||||
if (k.fromInvalidEffortValue) {
|
||||
effortIssues.push({ kind: 'kept-as-target', value: k.token });
|
||||
const issues = k.invalidValueOf === '--effort' ? effortIssues : floorIssues;
|
||||
if (k.invalidValueOf !== undefined) {
|
||||
const survives = isPrShaped(k.token)
|
||||
? !hasValidCandidate && distinctPr.size === 1
|
||||
: soleCandidate;
|
||||
if (!survives) {
|
||||
issues.push({ kind: 'discarded', value: k.token });
|
||||
continue;
|
||||
}
|
||||
if (isPrShaped(k.token)) {
|
||||
if (rescuedPr) continue; // same PR, restated — not an extra token
|
||||
rescuedPr = true;
|
||||
}
|
||||
issues.push({ kind: 'kept-as-target', value: k.token });
|
||||
}
|
||||
targetTokens.push(k.token);
|
||||
}
|
||||
|
|
@ -464,12 +651,70 @@ export function parseReviewArgs(
|
|||
);
|
||||
}
|
||||
|
||||
// The floor resolves like the effort — explicit flag over configured
|
||||
// setting over the built-in default — except the default is `auto`: the
|
||||
// round-adaptive rule, which only Step 6 can resolve (the round comes from
|
||||
// the previous posted round's ledger, not fetched yet). Non-PR gating
|
||||
// mirrors `--comment`: the floor is a posting rule and rounds exist only
|
||||
// for PRs, so an explicit flag on a local/file target warns and is
|
||||
// ignored, and a configured setting is silently inert there.
|
||||
let severityFloor: ReviewSeverityFloor | 'auto' = 'auto';
|
||||
let severityFloorSource: ParsedReviewArgs['severityFloorSource'] = 'default';
|
||||
if (explicitFloor !== null && !isPr) {
|
||||
warnings.push(
|
||||
'Warning: `--severity-floor` flag is ignored because the review target is not a PR.',
|
||||
);
|
||||
} else if (explicitFloor !== null) {
|
||||
severityFloor = explicitFloor;
|
||||
severityFloorSource = 'explicit';
|
||||
} else if (configuredFloor !== undefined && isPr) {
|
||||
severityFloor = configuredFloor;
|
||||
severityFloorSource = 'configured';
|
||||
}
|
||||
const floorResolution =
|
||||
severityFloorSource === 'explicit'
|
||||
? `--severity-floor ${severityFloor} (the last valid occurrence) is in effect`
|
||||
: severityFloorSource === 'configured'
|
||||
? 'using the configured review.severityFloor'
|
||||
: 'using the round-adaptive default';
|
||||
for (const issue of floorIssues) {
|
||||
switch (issue.kind) {
|
||||
case 'invalid-eq':
|
||||
warnings.push(
|
||||
`Invalid --severity-floor value ${JSON.stringify(issue.value)}; ${floorResolution}.`,
|
||||
);
|
||||
break;
|
||||
case 'missing':
|
||||
warnings.push(`--severity-floor requires a value; ${floorResolution}.`);
|
||||
break;
|
||||
case 'discarded':
|
||||
warnings.push(
|
||||
`Invalid --severity-floor value ${JSON.stringify(issue.value)} discarded; ${floorResolution}.`,
|
||||
);
|
||||
break;
|
||||
case 'kept-as-target':
|
||||
warnings.push(
|
||||
`Invalid --severity-floor value ${JSON.stringify(issue.value)}; treating it as the review target — ${floorResolution}.`,
|
||||
);
|
||||
break;
|
||||
default:
|
||||
break;
|
||||
}
|
||||
}
|
||||
if (invalidConfiguredFloor !== undefined && isPr) {
|
||||
warnings.push(
|
||||
`Invalid review.severityFloor value ${JSON.stringify(invalidConfiguredFloor)} in settings; ${floorResolution}.`,
|
||||
);
|
||||
}
|
||||
|
||||
return {
|
||||
target,
|
||||
effort,
|
||||
effortSource,
|
||||
comment: { requested: commentRequestedByFlag, effective: commentEffective },
|
||||
fix: { requested: fixRequested, effective: fixEffective },
|
||||
severityFloor,
|
||||
severityFloorSource,
|
||||
extraTokens,
|
||||
unknownFlags,
|
||||
warnings,
|
||||
|
|
@ -494,6 +739,7 @@ interface ParseArgsCliArgs {
|
|||
function reviewDefaultsFromSettings(): {
|
||||
effort?: string;
|
||||
comment?: boolean;
|
||||
severityFloor?: string;
|
||||
} {
|
||||
const review = operatorReviewSettings();
|
||||
return {
|
||||
|
|
@ -502,13 +748,18 @@ function reviewDefaultsFromSettings(): {
|
|||
? undefined
|
||||
: review.effort,
|
||||
comment: review.comment,
|
||||
severityFloor:
|
||||
review.severityFloor === undefined ||
|
||||
review.severityFloor.toLowerCase() === 'auto'
|
||||
? undefined
|
||||
: review.severityFloor,
|
||||
};
|
||||
}
|
||||
|
||||
export const parseArgsCommand: CommandModule = {
|
||||
command: 'parse-args [raw]',
|
||||
describe:
|
||||
'Parse the /review skill argument string (--comment, --fix, --effort, target disambiguation) and emit the verdict as JSON; pass the string on stdin via --stdin (a positional that begins with a dash never reaches this handler — yargs rejects it as an unknown flag)',
|
||||
'Parse the /review skill argument string (--comment, --fix, --effort, --severity-floor, target disambiguation) and emit the verdict as JSON; pass the string on stdin via --stdin (a positional that begins with a dash never reaches this handler — yargs rejects it as an unknown flag)',
|
||||
builder: (yargs) =>
|
||||
yargs
|
||||
.positional('raw', {
|
||||
|
|
|
|||
158
packages/cli/src/commands/review/pr-context-persist.test.ts
Normal file
158
packages/cli/src/commands/review/pr-context-persist.test.ts
Normal file
|
|
@ -0,0 +1,158 @@
|
|||
/**
|
||||
* @license
|
||||
* Copyright 2026 Qwen Team
|
||||
* SPDX-License-Identifier: Apache-2.0
|
||||
*/
|
||||
|
||||
// `persistRecoveredLedger` writes REAL files (atomic temp+rename, removal,
|
||||
// in-place strip), so its tests live apart from pr-context.test.ts, which
|
||||
// mocks node:fs writes for the handler tests — under that mock every
|
||||
// assertion here would pass vacuously or fail on a missing file.
|
||||
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import {
|
||||
existsSync,
|
||||
mkdtempSync,
|
||||
readdirSync,
|
||||
readFileSync,
|
||||
rmSync,
|
||||
writeFileSync,
|
||||
} from 'node:fs';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { join } from 'node:path';
|
||||
import { persistRecoveredLedger } from './pr-context.js';
|
||||
import type { Ledger } from './lib/ledger.js';
|
||||
|
||||
describe('persistRecoveredLedger', () => {
|
||||
// The serialization seam the helper tests could not reach before the
|
||||
// extraction: a regression dropping a field here disabled rounds-2-5
|
||||
// code-age behavior while every latestOwnLedger test stayed green. The
|
||||
// fixture carries a `sha` on purpose: the side file's sha is the
|
||||
// incremental anchor for cache-absent machines, and a rewrite that
|
||||
// reconstructed the file from known fields dropped it with the suite
|
||||
// green until the fixture carried one.
|
||||
const ledger: Ledger = {
|
||||
v: 1,
|
||||
round: 3,
|
||||
findings: [{ id: 'R3-1', sev: 'S', file: 'a.ts', title: 't' }],
|
||||
sha: 'deadbeef00112233',
|
||||
};
|
||||
|
||||
it('persists the ledger with its age reference and provenance', () => {
|
||||
const dir = mkdtempSync(join(tmpdir(), 'prev-ledger-'));
|
||||
const side = join(dir, 'nested', 'qwen-review-pr-1-prev-ledger.json');
|
||||
try {
|
||||
persistRecoveredLedger(
|
||||
side,
|
||||
{ ledger, commitId: 'a'.repeat(40), reviewId: 42 },
|
||||
true,
|
||||
);
|
||||
const written = JSON.parse(readFileSync(side, 'utf8'));
|
||||
expect(written).toEqual({
|
||||
...ledger,
|
||||
commitId: 'a'.repeat(40),
|
||||
reviewId: 42,
|
||||
});
|
||||
expect(written.sha).toBe('deadbeef00112233');
|
||||
} finally {
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it('a recovery that THREW strips the age reference but keeps round and sha', () => {
|
||||
// A transient failure must not reset the id space or lose the anchor;
|
||||
// it must also not keep an age reference this run could not re-vouch —
|
||||
// code changed-and-reverted since the true previous round would look
|
||||
// unchanged against the stale head and a first-time finding would be
|
||||
// wrongly deferred (snapshot diffs are not monotonic over intervals).
|
||||
const dir = mkdtempSync(join(tmpdir(), 'prev-ledger-'));
|
||||
const side = join(dir, 'side.json');
|
||||
try {
|
||||
writeFileSync(
|
||||
side,
|
||||
JSON.stringify({ ...ledger, commitId: 'b'.repeat(40), reviewId: 7 }),
|
||||
);
|
||||
persistRecoveredLedger(side, null, false);
|
||||
const written = JSON.parse(readFileSync(side, 'utf8'));
|
||||
expect(written).toEqual(ledger);
|
||||
expect(written.round).toBe(3);
|
||||
expect(written.sha).toBe('deadbeef00112233');
|
||||
} finally {
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it('proven absence REMOVES the stale file whole', () => {
|
||||
// The PR demonstrably holds no prior round for this account (a walked
|
||||
// list with no own submitted review) — another account's round counter
|
||||
// must not stamp this account's first review "round N+1" and engage the
|
||||
// posture on rounds it never ran.
|
||||
const dir = mkdtempSync(join(tmpdir(), 'prev-ledger-'));
|
||||
const side = join(dir, 'side.json');
|
||||
try {
|
||||
writeFileSync(
|
||||
side,
|
||||
JSON.stringify({ ...ledger, commitId: 'b'.repeat(40), reviewId: 7 }),
|
||||
);
|
||||
persistRecoveredLedger(side, null, true);
|
||||
expect(existsSync(side)).toBe(false);
|
||||
} finally {
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it('never lowers the round — a stale walk cannot overwrite a newer side file', () => {
|
||||
// Self-audit finding: a lower-round recovery (a concurrent lane's stale
|
||||
// list, or a paginated fetch that came back short) overwrote round 7
|
||||
// with round 2 and dropped the anchor sha. Compare on round, reviewId
|
||||
// as the tiebreak.
|
||||
const dir = mkdtempSync(join(tmpdir(), 'prev-ledger-'));
|
||||
const side = join(dir, 'side.json');
|
||||
try {
|
||||
const newer = { ...ledger, round: 7, sha: 'ffff1111', reviewId: 70 };
|
||||
writeFileSync(side, JSON.stringify(newer));
|
||||
persistRecoveredLedger(
|
||||
side,
|
||||
{
|
||||
ledger: { ...ledger, round: 2 },
|
||||
commitId: 'a'.repeat(40),
|
||||
reviewId: 20,
|
||||
},
|
||||
false,
|
||||
);
|
||||
expect(JSON.parse(readFileSync(side, 'utf8'))).toEqual(newer);
|
||||
// Same round, older reviewId: also kept.
|
||||
persistRecoveredLedger(
|
||||
side,
|
||||
{ ledger: { ...ledger, round: 7 }, commitId: null, reviewId: 60 },
|
||||
false,
|
||||
);
|
||||
expect(JSON.parse(readFileSync(side, 'utf8'))).toEqual(newer);
|
||||
// A genuinely newer recovery still writes.
|
||||
persistRecoveredLedger(
|
||||
side,
|
||||
{ ledger: { ...ledger, round: 8 }, commitId: null, reviewId: 80 },
|
||||
false,
|
||||
);
|
||||
expect(JSON.parse(readFileSync(side, 'utf8')).round).toBe(8);
|
||||
} finally {
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it('a no-recovery run with no side file writes nothing', () => {
|
||||
const dir = mkdtempSync(join(tmpdir(), 'prev-ledger-'));
|
||||
const side = join(dir, 'side.json');
|
||||
try {
|
||||
persistRecoveredLedger(side, null, false);
|
||||
expect(existsSync(side)).toBe(false);
|
||||
// No debris of any name — the temp is per-process (`.<pid>.tmp`), so
|
||||
// asserting on the directory listing is the only check independent of
|
||||
// the naming scheme (round-9 finding: the old `${side}.tmp` check
|
||||
// named a path no code path ever writes and could never fail).
|
||||
expect(readdirSync(dir)).toEqual([]);
|
||||
} finally {
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
});
|
||||
|
|
@ -65,6 +65,7 @@ import {
|
|||
type RawComment,
|
||||
type RawReview,
|
||||
latestOwnLedger,
|
||||
recoverOwnLedger,
|
||||
renderLedgerSection,
|
||||
} from './pr-context.js';
|
||||
import { serializeLedger, type Ledger } from './lib/ledger.js';
|
||||
|
|
@ -1057,7 +1058,7 @@ describe('latestOwnLedger', () => {
|
|||
});
|
||||
|
||||
it('takes the LATEST marker from the reviewing account only', () => {
|
||||
const ledger = latestOwnLedger(
|
||||
const recovered = latestOwnLedger(
|
||||
[
|
||||
review('bot', '2026-01-01T00:00:00Z', marker(1)),
|
||||
review('bot', '2026-01-03T00:00:00Z', marker(3)),
|
||||
|
|
@ -1067,7 +1068,7 @@ describe('latestOwnLedger', () => {
|
|||
],
|
||||
'bot',
|
||||
);
|
||||
expect(ledger?.round).toBe(3);
|
||||
expect(recovered?.ledger.round).toBe(3);
|
||||
});
|
||||
|
||||
it('breaks a submitted_at tie on the review id, not on array order', () => {
|
||||
|
|
@ -1075,14 +1076,14 @@ describe('latestOwnLedger', () => {
|
|||
// ordered only by id. Keeping the earlier one hands the next round the
|
||||
// older work list — the one failure the whole recovery exists to prevent.
|
||||
const at = '2026-01-01T00:00:00Z';
|
||||
const ledger = latestOwnLedger(
|
||||
const recovered = latestOwnLedger(
|
||||
[
|
||||
{ id: 2, user: { login: 'bot' }, submitted_at: at, body: marker(1) },
|
||||
{ id: 9, user: { login: 'bot' }, submitted_at: at, body: marker(4) },
|
||||
],
|
||||
'bot',
|
||||
);
|
||||
expect(ledger?.round).toBe(4);
|
||||
expect(recovered?.ledger.round).toBe(4);
|
||||
});
|
||||
|
||||
it('carries the anchor sha through the recovery seam intact', () => {
|
||||
|
|
@ -1107,7 +1108,116 @@ describe('latestOwnLedger', () => {
|
|||
],
|
||||
'bot',
|
||||
);
|
||||
expect(recovered).toEqual(anchored);
|
||||
expect(recovered?.ledger).toEqual(anchored);
|
||||
});
|
||||
|
||||
it('recovers the winning review’s own commit_id as the age reference', () => {
|
||||
// The reference must come from the SAME review the ledger came from — a
|
||||
// recovery that took the newest ledger but another review's commit_id
|
||||
// would date old code against the wrong head. The fixture must be able
|
||||
// to refute that mutant: the account's NEWEST review is marker-less with
|
||||
// a different commit_id (the bot's follow-up comment posted against a
|
||||
// later head), so "take commitId from the latest review regardless of
|
||||
// ledger" fails here instead of passing by coincidence. An invalid or
|
||||
// missing commit_id yields null, never a truncated or garbage reference.
|
||||
// 64 hex chars: COMMIT_SHA_RE's deliberate {40,64} breadth exists for
|
||||
// SHA-256 heads, and no fixture pinned it (round-8 finding) — narrowing
|
||||
// to {40} would silently drop the age reference on such repos.
|
||||
const head = 'a'.repeat(64);
|
||||
const recovered = latestOwnLedger(
|
||||
[
|
||||
{
|
||||
...review('bot', '2026-01-01T00:00:00Z', marker(1)),
|
||||
commit_id: 'b'.repeat(40),
|
||||
},
|
||||
{
|
||||
...review('bot', '2026-01-02T00:00:00Z', marker(2)),
|
||||
id: 77,
|
||||
commit_id: head,
|
||||
},
|
||||
{
|
||||
...review('bot', '2026-01-03T00:00:00Z', 'marker-less follow-up'),
|
||||
commit_id: 'c'.repeat(40),
|
||||
},
|
||||
],
|
||||
'bot',
|
||||
);
|
||||
expect(recovered?.ledger.round).toBe(2);
|
||||
expect(recovered?.commitId).toBe(head);
|
||||
// The winning review's own id rides along: Step 6's not-reviewed check
|
||||
// must know WHICH body's disclosures bind the age rule.
|
||||
expect(recovered?.reviewId).toBe(77);
|
||||
const invalid = latestOwnLedger(
|
||||
[
|
||||
{
|
||||
...review('bot', '2026-01-01T00:00:00Z', marker(1)),
|
||||
commit_id: 'abc123',
|
||||
},
|
||||
],
|
||||
'bot',
|
||||
);
|
||||
expect(invalid?.ledger.round).toBe(1);
|
||||
expect(invalid?.commitId).toBeNull();
|
||||
});
|
||||
|
||||
it('distinguishes "no own review" from "own review without a parseable ledger"', () => {
|
||||
// Round-8 finding (two auditors independently): the deletion arm read
|
||||
// "recovery returned null although reviews were read" as proof of no
|
||||
// prior round, but an own review whose marker fails to parse (edited or
|
||||
// damaged bot body, marker-less follow-up) also yields null — a
|
||||
// persistent state, not absence. Deleting the side file there stamped
|
||||
// the next round "round 1" mid-PR and reset the posture clock.
|
||||
const damaged = recoverOwnLedger(
|
||||
[review('bot', '2026-01-01T00:00:00Z', 'edited body, marker gone')],
|
||||
'bot',
|
||||
);
|
||||
expect(damaged.recovered).toBeNull();
|
||||
expect(damaged.sawOwnReview).toBe(true);
|
||||
const absent = recoverOwnLedger(
|
||||
[review('stranger', '2026-01-01T00:00:00Z', marker(3))],
|
||||
'bot',
|
||||
);
|
||||
expect(absent.recovered).toBeNull();
|
||||
expect(absent.sawOwnReview).toBe(false);
|
||||
// Logins compare case-insensitively (GitHub's rule): a case mismatch
|
||||
// would read "own review exists" as "proven absence" and delete the
|
||||
// counter (self-audit finding).
|
||||
const cased = recoverOwnLedger(
|
||||
[review('Bot', '2026-01-01T00:00:00Z', marker(2))],
|
||||
'bot',
|
||||
);
|
||||
expect(cased.sawOwnReview).toBe(true);
|
||||
expect(cased.recovered?.ledger.round).toBe(2);
|
||||
// A PENDING draft is not "seen" either — it is not a submitted review.
|
||||
const draftOnly = recoverOwnLedger(
|
||||
[
|
||||
{
|
||||
...review('bot', '2026-01-01T00:00:00Z', marker(1)),
|
||||
state: 'PENDING',
|
||||
},
|
||||
],
|
||||
'bot',
|
||||
);
|
||||
expect(draftOnly.sawOwnReview).toBe(false);
|
||||
});
|
||||
|
||||
it('never selects a PENDING draft — an unsubmitted review is not a previous round', () => {
|
||||
// The API serves the caller's own drafts in the reviews list; a run that
|
||||
// crashed between creating and submitting one must not hand the next
|
||||
// round a round number, an age reference and a reviewId from state the
|
||||
// PR never showed anyone.
|
||||
const recovered = latestOwnLedger(
|
||||
[
|
||||
review('bot', '2026-01-01T00:00:00Z', marker(1)),
|
||||
{
|
||||
...review('bot', '2026-01-02T00:00:00Z', marker(9)),
|
||||
state: 'PENDING',
|
||||
commit_id: 'd'.repeat(40),
|
||||
},
|
||||
],
|
||||
'bot',
|
||||
);
|
||||
expect(recovered?.ledger.round).toBe(1);
|
||||
});
|
||||
|
||||
it('yields nothing with no login, no marker, or a malformed one', () => {
|
||||
|
|
|
|||
|
|
@ -14,7 +14,13 @@
|
|||
// comments, and issue comments.
|
||||
|
||||
import type { CommandModule } from 'yargs';
|
||||
import { mkdirSync, writeFileSync } from 'node:fs';
|
||||
import {
|
||||
mkdirSync,
|
||||
readFileSync,
|
||||
renameSync,
|
||||
rmSync,
|
||||
writeFileSync,
|
||||
} from 'node:fs';
|
||||
import { dirname, join } from 'node:path';
|
||||
import { DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD } from '@qwen-code/qwen-code-core';
|
||||
import { writeStdoutLine } from '../../utils/stdioHelpers.js';
|
||||
|
|
@ -68,6 +74,8 @@ export interface RawReview {
|
|||
body?: string;
|
||||
state?: string; // APPROVED | CHANGES_REQUESTED | COMMENTED | DISMISSED | PENDING
|
||||
submitted_at?: string;
|
||||
/** The head commit the review was submitted against, per the API. */
|
||||
commit_id?: string;
|
||||
}
|
||||
|
||||
interface PrContextArgs {
|
||||
|
|
@ -660,6 +668,13 @@ function blockerSection(
|
|||
return out;
|
||||
}
|
||||
|
||||
/**
|
||||
* A full object id, as the API serves `commit_id`. Deliberately stricter than
|
||||
* the ledger marker's abbreviated-anchor check: this value comes from the API
|
||||
* response, not from an untrusted body, and a full sha is what it always is.
|
||||
*/
|
||||
const COMMIT_SHA_RE = /^[0-9a-f]{40,64}$/;
|
||||
|
||||
/**
|
||||
* The latest machine ledger the REVIEWING account itself posted, if any.
|
||||
*
|
||||
|
|
@ -672,24 +687,212 @@ function blockerSection(
|
|||
* which is monotonic: keeping the earlier review on a tie would hand the next
|
||||
* round the older work list, the one failure this whole recovery exists to
|
||||
* prevent.
|
||||
*
|
||||
* `commitId` is the review's own `commit_id` — the head the previous round's
|
||||
* review was submitted against, set by GitHub, not by the body. It is the age
|
||||
* reference for Step 6's convergence posture ("was this line changed since
|
||||
* the previous round saw it?") and NEVER an incremental anchor: the ledger's
|
||||
* `sha` certifies "this range was cleanly reviewed" and is withheld on a
|
||||
* fail-closed round on purpose, while `commit_id` exists on every posted
|
||||
* round, fail-closed or not — a posting bar needs a reference point, not a
|
||||
* certification.
|
||||
*/
|
||||
export function latestOwnLedger(
|
||||
reviews: RawReview[],
|
||||
login: string | null,
|
||||
): Ledger | null {
|
||||
if (!login) return null;
|
||||
let best: { at: string; id: number; ledger: Ledger } | null = null;
|
||||
): RecoveredLedger | null {
|
||||
return recoverOwnLedger(reviews, login).recovered;
|
||||
}
|
||||
|
||||
/**
|
||||
* The full recovery outcome — `latestOwnLedger`'s answer plus whether the
|
||||
* walk saw ANY submitted review by this login. Three states drive the side
|
||||
* file: recovered / walked own reviews but none carried a parseable ledger
|
||||
* (an edited or damaged bot body, a marker-less follow-up — a persistent
|
||||
* state, NOT proof of no prior round) / no own review at all. Only the last
|
||||
* licenses deleting the side file; the middle strips conservatively.
|
||||
*/
|
||||
export function recoverOwnLedger(
|
||||
reviews: RawReview[],
|
||||
login: string | null,
|
||||
): { recovered: RecoveredLedger | null; sawOwnReview: boolean } {
|
||||
if (!login) return { recovered: null, sawOwnReview: false };
|
||||
// GitHub logins are case-insensitive; a case mismatch here would turn "own
|
||||
// review exists" into "proven absence" and license deleting the counter.
|
||||
const me = login.toLowerCase();
|
||||
let sawOwnReview = false;
|
||||
let best: {
|
||||
at: string;
|
||||
id: number;
|
||||
ledger: Ledger;
|
||||
commitId: string | null;
|
||||
} | null = null;
|
||||
for (const r of reviews) {
|
||||
if (r.user?.login !== login) continue;
|
||||
if (r.user?.login?.toLowerCase() !== me) continue;
|
||||
// A PENDING review is an unsubmitted draft — the API serves the caller's
|
||||
// own drafts in this list — and a draft is not a previous round: a run
|
||||
// that crashed between creating and submitting one must not hand the
|
||||
// next round a round number, an age reference and a reviewId from state
|
||||
// the PR never showed anyone.
|
||||
if (r.state === 'PENDING') continue;
|
||||
sawOwnReview = true;
|
||||
const ledger = parseLedger(r.body);
|
||||
if (!ledger) continue;
|
||||
const at = r.submitted_at ?? '';
|
||||
const id = typeof r.id === 'number' ? r.id : 0;
|
||||
if (!best || at > best.at || (at === best.at && id > best.id)) {
|
||||
best = { at, id, ledger };
|
||||
best = {
|
||||
at,
|
||||
id,
|
||||
ledger,
|
||||
commitId:
|
||||
typeof r.commit_id === 'string' && COMMIT_SHA_RE.test(r.commit_id)
|
||||
? r.commit_id
|
||||
: null,
|
||||
};
|
||||
}
|
||||
}
|
||||
return best?.ledger ?? null;
|
||||
return {
|
||||
recovered: best
|
||||
? { ledger: best.ledger, commitId: best.commitId, reviewId: best.id }
|
||||
: null,
|
||||
sawOwnReview,
|
||||
};
|
||||
}
|
||||
|
||||
/** What ledger recovery hands the side-file writer. */
|
||||
export interface RecoveredLedger {
|
||||
ledger: Ledger;
|
||||
commitId: string | null;
|
||||
/**
|
||||
* The winning review's own id — persisted so Step 6 can find WHICH body's
|
||||
* not-reviewed disclosures bind the code-age rule: with several summaries
|
||||
* on the PR, "check the previous round's review body" is ambiguous, and
|
||||
* checking the wrong one suppresses a finding on code the true previous
|
||||
* round declared unread.
|
||||
*/
|
||||
reviewId: number;
|
||||
}
|
||||
|
||||
/**
|
||||
* Persist (or degrade) the prev-ledger side file for this run's recovery.
|
||||
* Three outcomes, each honest about what this run learned:
|
||||
*
|
||||
* - Recovered: the ledger's own fields plus `commitId`/`reviewId` — the age
|
||||
* reference and its provenance for Step 6's convergence posture. Readers
|
||||
* of the ledger shape (compose-review's round count, Step 1's
|
||||
* recovered-anchor check) ignore the extra keys.
|
||||
* - Not recovered, absence PROVEN (`noOwnReview` — a non-empty reviews list
|
||||
* was walked and no submitted review by this account exists in it; an
|
||||
* empty list may be an error envelope `ghApiAll` flattens to `[]`, and an
|
||||
* own review whose marker fails to parse is a persistent state — neither
|
||||
* proves absence, both strip): the PR demonstrably
|
||||
* holds no prior round for this account — the file is another account's
|
||||
* or a deleted round's leftovers, and it is REMOVED whole: carrying its
|
||||
* round counter would stamp a first review "round N+1" and engage the
|
||||
* posture on rounds this account never ran.
|
||||
* - Recovery THREW: unknowable, so the stale file keeps its round counter —
|
||||
* a transient failure must not reset the id space — but loses
|
||||
* `commitId`/`reviewId`: an age reference this run could not re-vouch can
|
||||
* suppress a first-time finding on code changed-and-reverted since the
|
||||
* true previous round (snapshot diffs are not monotonic over intervals),
|
||||
* while dropping it merely fails open to full posting.
|
||||
*
|
||||
* Every write is write-temp-then-rename: a failure mid-write must leave the
|
||||
* previous file intact, never a truncated one that parses as no round and
|
||||
* restarts the id space. Best-effort throughout — a side-file hiccup must
|
||||
* never fail the command.
|
||||
*/
|
||||
export function persistRecoveredLedger(
|
||||
sideFilePath: string,
|
||||
recovered: RecoveredLedger | null,
|
||||
noOwnReview: boolean,
|
||||
): void {
|
||||
// Unique per process: two same-PR fetches racing on one fixed `.tmp` can
|
||||
// rename each other's bytes (A renames B's write; B's ENOENT is
|
||||
// swallowed), leaving the side file disagreeing with the context A holds
|
||||
// in memory. Distinct temp names make the rename last-writer-wins on the
|
||||
// FINAL path only, which is the intended semantics. The temp is unlinked
|
||||
// on a failed rename so an aborted write leaves no debris.
|
||||
const writeAtomic = (text: string) => {
|
||||
const tmp = `${sideFilePath}.${process.pid}.tmp`;
|
||||
writeFileSync(tmp, text);
|
||||
try {
|
||||
renameSync(tmp, sideFilePath);
|
||||
} catch (err) {
|
||||
try {
|
||||
rmSync(tmp, { force: true });
|
||||
} catch {
|
||||
/* debris removal is best-effort */
|
||||
}
|
||||
throw err;
|
||||
}
|
||||
};
|
||||
if (recovered) {
|
||||
try {
|
||||
// Never lower the round: a walked review list can be STALE relative
|
||||
// to a side file another run wrote (a concurrent lane, or a paginated
|
||||
// fetch that came back short), and overwriting round 7 with round 2
|
||||
// would drop the anchor sha and rewind the posture clock. Compare on
|
||||
// `round`, `reviewId` as the tiebreak — both already in the file.
|
||||
try {
|
||||
const existing = JSON.parse(readFileSync(sideFilePath, 'utf8')) as {
|
||||
round?: unknown;
|
||||
reviewId?: unknown;
|
||||
};
|
||||
const exRound =
|
||||
typeof existing.round === 'number' ? existing.round : -1;
|
||||
const exId =
|
||||
typeof existing.reviewId === 'number' ? existing.reviewId : -1;
|
||||
if (
|
||||
exRound > recovered.ledger.round ||
|
||||
(exRound === recovered.ledger.round && exId > recovered.reviewId)
|
||||
) {
|
||||
return;
|
||||
}
|
||||
} catch {
|
||||
// No readable existing file: nothing to protect.
|
||||
}
|
||||
mkdirSync(dirname(sideFilePath), { recursive: true });
|
||||
writeAtomic(
|
||||
JSON.stringify(
|
||||
{
|
||||
...recovered.ledger,
|
||||
...(recovered.commitId ? { commitId: recovered.commitId } : {}),
|
||||
reviewId: recovered.reviewId,
|
||||
},
|
||||
null,
|
||||
2,
|
||||
),
|
||||
);
|
||||
} catch {
|
||||
// The previous file (if any) is intact; compose-review reads it or
|
||||
// starts the round count over, nothing else.
|
||||
}
|
||||
return;
|
||||
}
|
||||
if (noOwnReview) {
|
||||
try {
|
||||
rmSync(sideFilePath, { force: true });
|
||||
} catch {
|
||||
// Removal is best-effort; a survivor is the pre-existing stale risk.
|
||||
}
|
||||
return;
|
||||
}
|
||||
try {
|
||||
const stale = JSON.parse(readFileSync(sideFilePath, 'utf8')) as Record<
|
||||
string,
|
||||
unknown
|
||||
>;
|
||||
if ('commitId' in stale || 'reviewId' in stale) {
|
||||
delete stale['commitId'];
|
||||
delete stale['reviewId'];
|
||||
writeAtomic(JSON.stringify(stale, null, 2));
|
||||
}
|
||||
} catch {
|
||||
// No stale side file (the normal case), or an unreadable one — either
|
||||
// way there is nothing age-sensitive to strip.
|
||||
}
|
||||
}
|
||||
|
||||
/** Render the previous round's ledger for the context file. */
|
||||
|
|
@ -982,35 +1185,38 @@ async function runPrContext(args: PrContextArgs): Promise<void> {
|
|||
// posted review, and persist it beside the context file: compose-review reads
|
||||
// the side file for the round number, and Step 6 owes each entry a ruling.
|
||||
// Best-effort — offline/unauthenticated just means no ledger, never a failure.
|
||||
let prevLedger: Ledger | null = null;
|
||||
let prevRecovered: RecoveredLedger | null = null;
|
||||
let recoveryThrew = false;
|
||||
let sawOwnReview = false;
|
||||
try {
|
||||
// `currentUser()` is a network round-trip; with no reviews on the PR there
|
||||
// is nothing for its answer to match against, so it is not made.
|
||||
prevLedger = reviews.length
|
||||
? latestOwnLedger(reviews, currentUser())
|
||||
: null;
|
||||
} catch {
|
||||
prevLedger = null;
|
||||
}
|
||||
if (prevLedger) {
|
||||
// mkdir FIRST and guard: this write precedes the one below that creates the
|
||||
// directory, and an unguarded ENOENT would fail the whole command over a
|
||||
// best-effort carry-forward.
|
||||
try {
|
||||
mkdirSync(dirname(out), { recursive: true });
|
||||
writeFileSync(
|
||||
join(dirname(out), `qwen-review-pr-${prNumber}-prev-ledger.json`),
|
||||
JSON.stringify(prevLedger, null, 2),
|
||||
);
|
||||
} catch {
|
||||
// No side file: compose-review starts the round count over, nothing else.
|
||||
if (reviews.length) {
|
||||
const outcome = recoverOwnLedger(reviews, currentUser());
|
||||
prevRecovered = outcome.recovered;
|
||||
sawOwnReview = outcome.sawOwnReview;
|
||||
}
|
||||
} catch {
|
||||
prevRecovered = null;
|
||||
recoveryThrew = true;
|
||||
}
|
||||
// No `else` clearing a stale side file, deliberately. A recovery that failed
|
||||
// transiently — auth, network, a review page not fetched — would otherwise
|
||||
// reset the round counter to 1 and re-issue ids the PR already carries.
|
||||
// Keeping the stale copy only ever advances the count, which is the safe
|
||||
// direction for an id space.
|
||||
const prevLedger = prevRecovered?.ledger ?? null;
|
||||
// The side file's three outcomes live in the helper: recovered → written
|
||||
// whole; demonstrably no prior round for this account → removed (a stale
|
||||
// counter would stamp someone else's rounds onto this account's first
|
||||
// review); recovery threw → round counter kept, age-sensitive
|
||||
// `commitId`/`reviewId` stripped.
|
||||
persistRecoveredLedger(
|
||||
join(dirname(out), `qwen-review-pr-${prNumber}-prev-ledger.json`),
|
||||
prevRecovered,
|
||||
// Deletion is licensed ONLY by proof of true absence: a non-empty list
|
||||
// this run walked in which NO submitted review by this account exists.
|
||||
// An empty `reviews` may be an error envelope ghApiAll flattened to []
|
||||
// (indistinguishable from a review-less PR), and an own review whose
|
||||
// marker fails to parse is a persistent state, not absence — both take
|
||||
// the conservative strip path.
|
||||
reviews.length > 0 && !recoveryThrew && !sawOwnReview,
|
||||
);
|
||||
|
||||
// The effective host (explicit --host, else an operator-exported
|
||||
// GH_HOST) goes into the emitted refetch commands — but only if it is a
|
||||
|
|
|
|||
|
|
@ -64,6 +64,9 @@ const verdict = {
|
|||
downgraded: true,
|
||||
downgradedFrom: 'Request changes',
|
||||
remediation: ['Run verification again.'],
|
||||
// Non-zero on purpose: the copy test then proves passthrough, not just
|
||||
// the validator's absent-means-zero default.
|
||||
deferredCount: 2,
|
||||
lowSignal: { agents: 4, srcDiffLines: 120 },
|
||||
verdictLine: 'Verdict: Comment — Request changes was downgraded',
|
||||
};
|
||||
|
|
@ -280,6 +283,44 @@ describe('saveReviewArtifact', () => {
|
|||
expect(existsSync(paths.out)).toBe(false);
|
||||
});
|
||||
|
||||
it.each(['two', -1, 1.5] as const)(
|
||||
'refuses a present deferredCount of the wrong shape (%s)',
|
||||
(bad) => {
|
||||
// The absent-means-zero default must not swallow a PRESENT malformed
|
||||
// value: a mutant deleting the refuse arm saved `deferredCount: -1`
|
||||
// into the durable artifact with the suite fully green.
|
||||
const paths = fixture();
|
||||
writeJson(paths.composed, { ...verdict, deferredCount: bad });
|
||||
|
||||
expect(() =>
|
||||
saveReviewArtifact({
|
||||
...paths,
|
||||
target: 'local',
|
||||
effort: 'medium',
|
||||
}),
|
||||
).toThrow(/deferredCount/);
|
||||
expect(existsSync(paths.out)).toBe(false);
|
||||
},
|
||||
);
|
||||
|
||||
it('reads an absent or null deferredCount as zero — a pre-posture composed file must still save', () => {
|
||||
// Null rides the same absence semantics compose-review's own toCount
|
||||
// boundary gives this field's siblings.
|
||||
const paths = fixture();
|
||||
const { deferredCount: _absent, ...prePosture } = verdict;
|
||||
for (const composed of [prePosture, { ...verdict, deferredCount: null }]) {
|
||||
writeJson(paths.composed, composed);
|
||||
saveReviewArtifact({
|
||||
...paths,
|
||||
target: 'local',
|
||||
effort: 'medium',
|
||||
});
|
||||
expect(
|
||||
JSON.parse(readFileSync(paths.out, 'utf8')).verdict.deferredCount,
|
||||
).toBe(0);
|
||||
}
|
||||
});
|
||||
|
||||
it.each(['findings', 'composed', 'report'] as const)(
|
||||
'refuses to overwrite the %s input',
|
||||
(input) => {
|
||||
|
|
|
|||
|
|
@ -223,6 +223,22 @@ function validateVerdict(value: unknown): PersistedVerdict {
|
|||
}
|
||||
}
|
||||
}
|
||||
// Absent or null means zero, not malformed — the same absence semantics
|
||||
// compose-review's own `toCount` boundary applies to this field's siblings:
|
||||
// a composed JSON written by a build predating the convergence posture
|
||||
// carries no deferredCount, and a mid-upgrade save must not fail over a
|
||||
// count that only affects display. A PRESENT value of any other wrong
|
||||
// shape is refused like every other field here.
|
||||
const deferredCount = verdict['deferredCount'] ?? 0;
|
||||
if (
|
||||
typeof deferredCount !== 'number' ||
|
||||
!Number.isInteger(deferredCount) ||
|
||||
deferredCount < 0
|
||||
) {
|
||||
throw new Error(
|
||||
'Composed verdict.deferredCount must be a non-negative integer.',
|
||||
);
|
||||
}
|
||||
return {
|
||||
event: event(verdict['event'], 'Composed verdict.event'),
|
||||
body: string(verdict['body'], 'Composed verdict.body'),
|
||||
|
|
@ -234,6 +250,7 @@ function validateVerdict(value: unknown): PersistedVerdict {
|
|||
verdict['remediation'],
|
||||
'Composed verdict.remediation',
|
||||
),
|
||||
deferredCount,
|
||||
lowSignal:
|
||||
lowSignal === null
|
||||
? null
|
||||
|
|
|
|||
|
|
@ -996,6 +996,42 @@ describe('payload consistency — refuse before GitHub sees it', () => {
|
|||
expect(sent.body).toContain('the inline cache is stale after a rebase');
|
||||
});
|
||||
|
||||
it('carries a posture deferral through the submit seam into the posted body', () => {
|
||||
// submit's compose() destructures-and-drops distrusted state fields
|
||||
// (env, prBodyFetcher, draftedComments); a future hardening that adds
|
||||
// deferredSuggestions to that drop would post every review without its
|
||||
// deferral disclosure — "a deferral silently dropped is a finding lost"
|
||||
// — while the compose-review unit tests, which call the function
|
||||
// directly, stay green. This is the only production path from the
|
||||
// model-written state to a posted body.
|
||||
const review = file('deferral-seam.json', {
|
||||
...REVIEW,
|
||||
state: {
|
||||
...REVIEW.state,
|
||||
planPath: verifiedPlan(),
|
||||
severityFloor: 'critical',
|
||||
deferredSuggestions: [
|
||||
{
|
||||
file: 'a.ts',
|
||||
line: 1,
|
||||
source: 'review',
|
||||
severity: 'Suggestion',
|
||||
title: 'tighten the retry backoff',
|
||||
},
|
||||
],
|
||||
},
|
||||
comments: [],
|
||||
});
|
||||
|
||||
withVerifyEnv(() => runSubmit(authorized({ review })));
|
||||
expect(ghMock).toHaveBeenCalledOnce();
|
||||
const sent = JSON.parse(ghMock.mock.calls[0][0] as string);
|
||||
expect(sent.body).toContain('Deferred under the convergence posture');
|
||||
expect(sent.body).toContain(
|
||||
'- `a.ts:1 — [review] tighten the retry backoff`',
|
||||
);
|
||||
});
|
||||
|
||||
it('rejects a line that is not a positive whole number', () => {
|
||||
// Every one of these 422s, and a 422 discards every blocker in the review.
|
||||
for (const [i, line] of [-1, 0, 2.5, NaN, Infinity].entries()) {
|
||||
|
|
|
|||
|
|
@ -753,6 +753,21 @@ const SETTINGS_SCHEMA = {
|
|||
'Treat every PR /review as if --comment was passed: findings are posted to the pull request without the flag. The post still binds to the PR named in the invocation. Enable only if you always want reviews published. Only honored from User, System, and SystemDefaults settings scopes; values set in Workspace settings are ignored, so a repository cannot set review policy for its reviewers.',
|
||||
showInDialog: true,
|
||||
},
|
||||
severityFloor: {
|
||||
type: 'enum',
|
||||
label: 'Posting floor: review',
|
||||
category: 'General',
|
||||
requiresRestart: false,
|
||||
default: 'auto',
|
||||
description:
|
||||
'The lowest severity a PR /review posts when --severity-floor is not given. "auto" keeps the round-adaptive default: Suggestions post through round 5, and from round 6 only Criticals post while otherwise-postable high-confidence Suggestions are recorded and deferred (low-confidence and Nice-to-have findings stay terminal-only as ever); under "auto", rounds 2-5 additionally defer new Suggestions on code unchanged since the previous round — the same discipline that stops review rounds from ballooning a PR. "critical" applies that posture from round 1; "suggestion" keeps Suggestions posting at every round. Non-PR targets have no rounds and ignore this. Only honored from User, System, and SystemDefaults settings scopes; values set in Workspace settings are ignored, so a repository cannot set review policy for its reviewers.',
|
||||
showInDialog: true,
|
||||
options: [
|
||||
{ value: 'auto', label: 'Auto (Critical-only from round 6)' },
|
||||
{ value: 'critical', label: 'Critical-only (every round)' },
|
||||
{ value: 'suggestion', label: 'Suggestions and Criticals' },
|
||||
],
|
||||
},
|
||||
},
|
||||
},
|
||||
output: {
|
||||
|
|
|
|||
|
|
@ -52,10 +52,7 @@ import { profileCheckpoint } from '../utils/startupProfiler.js';
|
|||
import { writeStderrLine, writeStdoutLine } from '../utils/stdioHelpers.js';
|
||||
import { sanitizeTerminalText } from './utils/textUtils.js';
|
||||
import { startPostRenderPrefetches } from '../startup/startup-prefetch.js';
|
||||
import {
|
||||
computeWindowTitle,
|
||||
writeTerminalTitle,
|
||||
} from './utils/windowTitle.js';
|
||||
import { computeWindowTitle, writeTerminalTitle } from './utils/windowTitle.js';
|
||||
import { getCliVersion } from '../utils/version.js';
|
||||
|
||||
const debugLogger = createDebugLogger('STARTUP');
|
||||
|
|
|
|||
File diff suppressed because one or more lines are too long
|
|
@ -181,6 +181,112 @@ describe('bundled review skill', () => {
|
|||
expect(body).toContain("at the **root's own confidence**");
|
||||
});
|
||||
|
||||
it('pins the convergence posture and its load-bearing clauses', () => {
|
||||
// The posture is the reviewer-side brake on the review→fix→re-review
|
||||
// bloat loop. Each clause below carries a distinct obligation a later
|
||||
// "simplify the prose" edit is most likely to drop: the floor's
|
||||
// round-adaptive default, the never-defer-Criticals rule, the
|
||||
// record-not-request contract, and the age-reference/anchor distinction
|
||||
// (conflating `commitId` with the ledger `sha` would scope an
|
||||
// incremental review past scope a fail-closed round never certified).
|
||||
const body = skillBody();
|
||||
expect(body).toContain('Through round 5 the floor is `suggestion`');
|
||||
expect(body).toContain('**from round 6 it is `critical`**');
|
||||
expect(body).toContain(
|
||||
'A Critical is never deferred — any round, any floor',
|
||||
);
|
||||
expect(body).toContain('an **age reference, never an incremental anchor**');
|
||||
expect(body).toContain('skip the age rule, not the review');
|
||||
// The explicit knob's two directions: `critical` from round 1, and
|
||||
// `suggestion` as the off switch — the operator override the default
|
||||
// must never shadow.
|
||||
expect(body).toContain(
|
||||
'`critical` applies the Critical-only posture from round 1',
|
||||
);
|
||||
expect(body).toContain('`suggestion` turns the posture **off**');
|
||||
// The deferrable set is what the floor takes away — never the
|
||||
// terminal-only tiers: routing low-confidence or Nice-to-have findings
|
||||
// through the deferral list would PUBLISH what the posting path never
|
||||
// would (round-1 review finding).
|
||||
expect(body).toContain(
|
||||
'a non-Critical finding that would otherwise post is recorded, not requested',
|
||||
);
|
||||
expect(body).toContain('stay terminal-only exactly as before');
|
||||
// Deferral publishes, so it owes verification like a posted finding —
|
||||
// a deferrals-only APPROVE must not slip the verifier floor.
|
||||
expect(body).toContain(
|
||||
'an unverified claim does not become publishable by being deferred',
|
||||
);
|
||||
// ...and the entry is TYPED — one object per finding copied from the
|
||||
// artifact's own fields, never a sentence: four review rounds of regex
|
||||
// misses on the free-text form (kebab paths, the aggregate suffix, an
|
||||
// en dash, a title-borne tag) closed only by carrying the fields.
|
||||
expect(body).toContain(
|
||||
"as a **TYPED entry, one object per finding, copied from the artifact's own fields**",
|
||||
);
|
||||
expect(body).toContain('never write that line into the state');
|
||||
// The age command is hostile-input-hardened in both operands (round-1
|
||||
// review findings: shell injection via unquoted PR-controlled filename;
|
||||
// glob pathspec matching a sibling file). A "simplify the command"
|
||||
// edit must fail here.
|
||||
expect(body).toContain(
|
||||
"git --literal-pathspecs diff <commitId>..HEAD --unified=0 -- '<file>'",
|
||||
);
|
||||
expect(body).toContain('neither hardening is optional');
|
||||
// The embedded-apostrophe rule is load-bearing on its own: a legal name
|
||||
// like `it's.ts` breaks the quoted token without it, and deleting only
|
||||
// that clause left every other assertion green (round-5 review finding).
|
||||
expect(body).toContain("a `'` inside the name becomes `'\\''`");
|
||||
// The state carries the verdict's floor UNRESOLVED — a round-resolved
|
||||
// `suggestion` is indistinguishable from the operator's explicit
|
||||
// posture-off override, and passing it turned every legal rounds-2-5
|
||||
// age deferral into an unlicensed one (round-5 review finding).
|
||||
expect(body).toContain(
|
||||
"verdict's `severityFloor` into the compose state UNRESOLVED",
|
||||
);
|
||||
// The age rule's premise needs the previous round to have READ the code
|
||||
// it vouches for: scope that round disclosed as not reviewed gets no
|
||||
// age suppression (round-1 review finding).
|
||||
expect(body).toContain(
|
||||
'a first-time Suggestion in code nobody read must post like any round-1 finding',
|
||||
);
|
||||
// The validation commands are the rebase-skip arm's only detection
|
||||
// mechanism — without these pins, deleting the sentence leaves the
|
||||
// skip-list's "fails the validation" clause dangling (round-7 finding).
|
||||
expect(body).toContain('git cat-file -e <commitId>^{commit}');
|
||||
expect(body).toContain('git merge-base --is-ancestor <commitId> HEAD');
|
||||
// The two diff-output doubt states fail open (round-7 finding): a
|
||||
// non-matching pathspec is about the path, and a zero-hunk non-empty
|
||||
// diff (a PR-controlled .gitattributes binary mark) is a change.
|
||||
expect(body).toContain("git cat-file -e HEAD:'<file>'");
|
||||
expect(body).toContain('zero `@@` hunks');
|
||||
// Multi-location findings have exactly one governing rule under the age
|
||||
// gate (round-7 finding).
|
||||
expect(body).toContain('A pattern aggregate is aged per location');
|
||||
// The posture round's source of truth and the context-unavailable
|
||||
// resolution (round-7 findings): the cache never decides the posture,
|
||||
// and a degraded run fails open to full posting at round 1.
|
||||
expect(body).toContain(
|
||||
'the round that decides the posture is the SIDE FILE',
|
||||
);
|
||||
expect(body).toContain('no recovered ledger → round 1 → no posture');
|
||||
expect(body).toContain('treat `auto` as round 1: no posture, full posting');
|
||||
// The age rule is auto-only: an explicit `suggestion` floor is the
|
||||
// operator saying "post everything", and the age gate deferring under it
|
||||
// would contradict the override (round-2 review finding).
|
||||
expect(body).toContain(
|
||||
'never under an explicit `--severity-floor suggestion`',
|
||||
);
|
||||
// Deferral is a posting decision: the finding stays in the artifact, and
|
||||
// the deferred list must never become ledger work for the next round.
|
||||
expect(body).toContain(
|
||||
'the deferral is a posting decision recorded in the compose state',
|
||||
);
|
||||
expect(body).toContain(
|
||||
'Findings the convergence posture deferred stay out the same way',
|
||||
);
|
||||
});
|
||||
|
||||
it('routes both remote-resolution paths through match-remote', () => {
|
||||
// The pr-url path (Step 1) and the bare-PR-number path both resolve the
|
||||
// remote via the deterministic matcher. A later edit reverting either
|
||||
|
|
|
|||
|
|
@ -245,6 +245,15 @@
|
|||
"description": "Treat every PR /review as if --comment was passed: findings are posted to the pull request without the flag. The post still binds to the PR named in the invocation. Enable only if you always want reviews published. Only honored from User, System, and SystemDefaults settings scopes; values set in Workspace settings are ignored, so a repository cannot set review policy for its reviewers.",
|
||||
"type": "boolean",
|
||||
"default": false
|
||||
},
|
||||
"severityFloor": {
|
||||
"description": "The lowest severity a PR /review posts when --severity-floor is not given. \"auto\" keeps the round-adaptive default: Suggestions post through round 5, and from round 6 only Criticals post while otherwise-postable high-confidence Suggestions are recorded and deferred (low-confidence and Nice-to-have findings stay terminal-only as ever); under \"auto\", rounds 2-5 additionally defer new Suggestions on code unchanged since the previous round — the same discipline that stops review rounds from ballooning a PR. \"critical\" applies that posture from round 1; \"suggestion\" keeps Suggestions posting at every round. Non-PR targets have no rounds and ignore this. Only honored from User, System, and SystemDefaults settings scopes; values set in Workspace settings are ignored, so a repository cannot set review policy for its reviewers. Options: auto, critical, suggestion",
|
||||
"enum": [
|
||||
"auto",
|
||||
"critical",
|
||||
"suggestion"
|
||||
],
|
||||
"default": "auto"
|
||||
}
|
||||
}
|
||||
},
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue