mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-08-21 22:55:16 +00:00
2610f6ed6d
* feat(review): let an operator lower the reverse-audit round cap The round cap is chosen from the diff topology, which is right for the cost question it answers but leaves no way for someone paying for reviews to say "spend less on this loop". `review.reverseAuditRounds` is that knob: a standing operator policy that lowers whichever tier applies. It can only lower, and the asymmetry is the point rather than caution about configuration. A single operator-chosen count is exactly what tiering removed — a round is one agent on a small diff and about ninety minutes on a huge one, so one number is wrong for at least one topology, and most wrong for the one whose cap exists to stop six-hour reviews that post nothing. Lowering carries no matching hazard: it can only end the loop sooner. The floor stays at the huge tier's three, for the reason the plan reader already refuses one and two — a cap below that pre-empts the two-consecutive-dry rule and buys a capped verdict rather than a cheaper review. The two things an operator means by "let it run longer" both have direct expressions that a round count only approximates: a ceiling longer than the huge tier assumes is a deadline, which the admission gate already prices a round against, and "keep going while it is still finding real defects" is a property of the findings rather than of a number chosen before the review starts. It is a setting, not a flag, and it resolves in the capture command rather than at the gate — so it lands in the plan and every reader sees one number without learning a setting was involved. That satisfies the module's standing rule that a budget the caller passes is a budget the caller can inflate, rather than making an exception to it. The reader needs no new code: a lowered value is inside the tier's band, which the existing clamp already honours. Operator scopes only, like the other review policy settings — a repository must not choose how deeply the pipeline verifies it, which the new tests pin by putting the setting in a workspace file and asserting it does nothing. Two things fall out of adding a fourth caller, both fixed here: - The plan builder now takes the ceiling as a REQUIRED parameter. Three capture commands build a plan, an optional one is a parameter a call site can quietly omit, and a setting that applies to two of the three review entry points is worse than one that applies to none. Passing undefined is how a caller says "no ceiling", visibly. - Reading the settings can throw: loading raises a fatal error when any settings file fails to parse, and this is now read while the diff is being captured — the review's first step. A stray comma in a file none of these settings had to come from would have ended the whole review. It degrades to the defaults and discloses instead, and every default is the conservative side: attribution on, no auto-posting, no effort or round-cap override. * chore(review): regenerate the settings JSON Schema for the new review setting The schema file is generated from the settings schema and checked in; adding a setting without regenerating it fails the CI check that keeps the two in step. No behaviour change — the file is derived output. * fix(review): stop the settings degrade from being killed by its own announcement The try/catch added here exists so a corrupt or unreadable settings file cannot end a review: loading throws a fatal error when any settings file fails to parse, and this is read while the diff is being captured, the review's first step. It then announced the degrade through the *throwing* stderr helper, so the announcement could end the review the degrade was written to save. Both halves are reachable together, and the second is ordinary rather than exotic: `process.stderr.write` throws on EPIPE or a closed fd — whenever the reader goes away (`qwen … | head`) or a daemon redirects its stderr — which is why the safe sibling exists and says so in its own docstring. With a broken settings file and no reader, the throw propagates out of the catch and all three capture commands crash before writing a plan. Switched to the safe writer, with the reason recorded at the call site. The test that covers it needed the mock repaired first, because the mock was hiding the bug: it mapped both writers to one non-throwing spy, which makes the throwing and safe helpers interchangeable and mocks away the entire distinction the degrade depends on. The safe one is now a spy that swallows what the underlying write throws, matching the real contract — so a test can make the write fail and see which helper the code chose. Reverting to the throwing helper turns the new test red; before this, it turned nothing red. * fix(review): correct two rationales that contradict the code they sit beside Both are claims about mechanism, both false, and both contradicted by documentation already in the same file — which is what makes them worth fixing past the round where only correctness fixes land: in this codebase the comment is the design record, and a wrong one outlives the round that shipped it. **Why a cap of one or two is refused** was stated in four places as "it forces a non-converged stop where two-consecutive-dry would have converged on its own". That is false for two: the convergence check runs before the cap gate, so an all-dry loop reaches CONVERGED under any cap of two or more — which the huge-tier constant's own docstring, two functions away, already said. The reasons are real but different for each value. One refuses the convergence pair's second member, so the loop can never produce the two dry audits convergence is defined by. Two lets an all-dry loop converge but leaves no round for a loop that reports anything, so the first finding makes the stop non-converged. Both end in a capped verdict rather than a cheaper review; only the mechanism was wrong. **When loading settings throws** was described as "any settings file fails to read or parse", with a stray comma as the example. Malformed JSON is the one case that does not throw: it is copied aside and recovered, under a comment that says "Never crash due to a corrupted settings file". The throw comes from a file that cannot be read, which is enough on its own to justify the degrade — this is read while the plan is being captured — so the correction narrows the claim without weakening the reason for the guard. * feat(review): apply the huge round reduction only when the run has a clock Three is the one tier lower than the topology beneath it, and read as a statement about auditing it is backwards. A huge diff has more defects and more territory than a chunked one, converges later, and on recall deserves more rounds rather than fewer — the standing counterexample is a 5,801-line PR that took eight review rounds and was still surfacing Criticals in code present since its first commit. It was never a statement about auditing. It is a statement about a wall: a reverse-audit round on a 4,000-line PR is about ninety minutes, five of them are 450, and a six-hour CI ceiling does not hold that plus the fan-out and the tail. The survey behind it measured absent reviews rather than slow ones — twenty-six timed-out jobs in one window, about 122 hours of compute, nothing posted. Three rounds reported beat five rounds lost. That argument is sound exactly where the wall is. A local run exports no review deadline, nothing kills it at six hours, and the reduction there trades recall away to fit a ceiling that does not exist — on the tier where recall matters most, and by a number calibrated against somebody else's CI. So the reduction now applies only when the run has a deadline at all. With a clock a huge diff caps at three, as before; without one it is simply a large chunked diff and caps at five. The clock is read where the settings ceiling is read — in the capture command, passed into the budget, recorded in the plan — so the two facts the cap depends on arrive the same way and the budget module keeps its property of touching neither the environment nor the settings. Both now travel as one context object rather than a growing parameter list, still required at the plan builder so a capture command cannot silently omit them. The admission gate asks the same question through the same parse the deadline gates already use, so "has a deadline" and "a deadline will be enforced" cannot come apart. All four capture/gate clock combinations are safe and covered: a plan captured without a clock records five and is honoured at five; read later under a clock its band closes to three and it is cut to three, which is the conservative direction when a wall turns out to exist after all. Two things this does not pretend to fix, both recorded in the design note. The deadline gate falls back to a flat thirty-minute round estimate until it has measured one, so on exactly the runs that time out it under-prices the first two rounds threefold and cannot refuse them — that, not the round count, is why a static reduction was needed on top of a working gate, and a size-aware first estimate is what would retire the reduction entirely. And chunk retirement can only begin at round three, so under a three-round cap only one round can ever shrink: the arithmetic that justifies the cap is an arithmetic the cap guarantees stays true. * fix(review): correct the claims this stack got wrong, and cover its untested seams Round-4 review of the three stacked changes, all of it comment-and-coverage rather than behaviour. Grouped by what was actually wrong. **A doc comment detached from what it documents.** The context interface was inserted between `reviewBudget`'s doc block and the function, so the whole block — including the input-laundering contract that opens it — attached to the interface and the function was left undocumented. The interface moves up beside its sibling with its own doc; the function keeps its contract. Its new paragraph also called both context fields environment values, when only one is — the other is resolved from settings. **Four test comments that argued for their assertions with false reasons.** The assertions were right and the rationales were not, which in a codebase whose comments are its design documentation is the more durable error. The integer guard is tested before the floor, not after, so the fractional value cited says nothing about what the floor would have caught alone. Not every value in the coercible-garbage list becomes zero — a negative stays negative and two strings become real counts, one of them large enough to land on the huge tier rather than the fallback. A single global bound of ten honours only two of the three clamp assertions, not all three; the third is an edge case, not a discriminator. And one comment justified its cases by naming a writer that does not exist on that branch. **A guarantee that is not guaranteed.** The settings fallback was documented as always degrading toward more work; that holds for three of the four fields and not for effort, where losing an operator's `high` returns the built-in rule and a local review drops to medium — less work, not more. Named as the exception it is, which is also why the fallback discloses on stderr. **Five prose sites that never learned the cap is conditional.** The step's stop rule, the clock module's own header, the large-diff cost model, the setting's schema description and the user-facing feature doc all still enumerated the cap unconditionally, contradicting the change one file away. The schema description additionally advised setting a deadline "to let a productive loop run longer", which is backwards: on a huge diff a deadline lowers the cap from five to three. **Two prose sites that overstated scope.** The setting is honoured from three operator scopes, not user only, and a value below the floor is ignored rather than clamped to it; and it cuts the cap for high-effort reviews only, since medium skips the reverse audit and low runs none. A cross-reference named a heading that does not exist. **The two seams nothing covered.** The environment boundary — whether this run has a deadline — was never exercised with the variable actually set: the budget tests pass the flag as a literal and the gate tests delete it. The operator ceiling's write path was likewise untested end to end; every builder call in the unit tests passes it as absent. Both now run through the real capture handler against a real environment and a mocked settings source, and both were mutation-checked: inverting the environment predicate and ignoring the ceiling each turn the new tests red. * fix(review): finish propagating the clock, and close the seams round 5 found Round-5 review of the stack. One real defect, one unsafe writer, and the rest is narration that stopped short plus coverage that could not see the change it was meant to cover. **A mock that leaked into every test after it.** The handler test added last round set an operator ceiling on a module-level settings mock and never restored it, so every later test in that file — including the whole trailing block — ran the real handler with an undeclared ceiling of twenty in play. Inert today because nothing downstream asserts on it, which is exactly how it would have survived to matter. Reset in the file's `beforeEach`. **The degrade announced itself with a writer that can destroy the degrade.** The settings fallback wrote its NOTE through the throwing stderr helper, whose safe sibling exists for precisely this case and says so in its own docstring: the write is incidental to the work in hand, and failing it would take down the fallback the guard exists to provide. Switched, with the reason recorded. **Five narration sites still stated the huge reduction unconditionally** — the field doc, the cap reader's own bullet, the enforcement-site comment above the round gate, the skill's version-skew paragraph, and the design note's "what an operator means by let it run longer". The last one was the most wrong: it named a deadline as the way to say "my ceiling is larger than six hours", but the check is for a deadline's *presence*, so any deadline however generous reads as three, and the cap is evaluated before the deadline arithmetic — setting one lowers the cap rather than raising it. Saying that would need the tier to read the deadline's size, which is now written down as the missing capability rather than implied to exist. **The setting was documented everywhere except the reference table.** The hand-maintained settings reference lists the review section exhaustively and did not list this one; the generator only writes the JSON schema, so it does not self-heal. Its schema description also never mentioned that only whole numbers are honoured, which matters because JSON Schema has no integer type here — a fraction validates in an editor and is discarded at runtime. **Two coverage gaps where the change was invisible to mutation.** Every gate test used the unsized fixture, whose tier is the fallback whatever the clock says, or forced a cap by storing one — so hardcoding the clock argument at all four call sites left the suite green. A sized huge plan is the only shape where the flag decides anything, and it now drives the gate on both sides. And two of the three capture commands had no budget assertion at all, so dropping either context field from their call sites compiled clean; the local one now asserts both. Both new tests were mutation-checked against exactly those edits. |
||
|---|---|---|
| .. | ||
| _meta.ts | ||
| auth.md | ||
| model-providers.md | ||
| qwen-ignore.md | ||
| settings.md | ||
| themes.md | ||
| trusted-folders.md | ||