mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-08-21 06:35:07 +00:00
fix(review): cross-check the churn census and harden the streak's edges
Round-1 review findings on the non-convergence mechanism: - Refuse a census whose fresh count exceeds everything the round reports (drafted comments, body Criticals, deferrals). The census is the model-written half of the trigger; this one-sided bound is the cross-check it gets before it can arm the streak, so a round that reported nothing can no longer file the blocker on the model's say-so alone. - persistRecoveredLedger's anonymous-advance branch now drops churnRounds/fresh/induced with the other round-specific facts: a streak re-dated across a round this account never ran would arm the blocker one round early and discard the foreign winner's own streak state. The plain recovery path round-trips them, and both seams are now pinned. - SKILL Step 6's fix-induced rule caps re-reports at one per original id per round — two same-id entries are a duplicate id, and the artifact validator refuses the round's findings whole. - Reword the posted blocker and its docblocks to what the arithmetic actually does: the bar is half-or-more (not "most"), it keys on the attributed count (not findings on new lines), and the streak counts rounds against the bar — rounds that could not measure carry the count — rather than calendar-consecutive rounds. - parseLedger clamps a recovered streak to the marker's own round: the streak counts rounds inside the round it rides, and an unclamped forged streak inflates the posted ordinal past everything the pull request ever ran. - Witness pins for the gaps the reviewers probed: the >= filing condition at streak 3, ordinalSuffix past 2 (rd/teen-th/st), CHURN_MIN_FRESH from both sides, and the full corrected blocker text.
This commit is contained in:
wenshao
parent
e631d95d9b
commit
78ae615677
8 changed files with 251 additions and 55 deletions
|
|
@ -40,6 +40,7 @@ import {
|
|||
CHURN_STREAK_TO_FILE,
|
||||
churnCensusOf,
|
||||
composeReview,
|
||||
nonConvergenceCritical,
|
||||
isNonDiffDimensionGap,
|
||||
buildLedger,
|
||||
repositoryContextGate,
|
||||
|
|
@ -8975,18 +8976,22 @@ describe('convergence telemetry — volume, carried in the marker', () => {
|
|||
describe('the convergence census and the non-convergence finding', () => {
|
||||
// The reviewer-side half of #9578. The loop's largest single source of its
|
||||
// own next round is the fix round before it; this is the machinery that
|
||||
// MEASURES that and, after two consecutive rounds of it, says so as a
|
||||
// blocker instead of filing a third round of derived findings.
|
||||
// MEASURES that and, after two rounds counted against the bar, says so as
|
||||
// a blocker instead of filing a third round of derived findings.
|
||||
const prevLedger = (over: Record<string, unknown>) =>
|
||||
writeFileSync(
|
||||
join(dir, 'qwen-review-pr-8255-prev-ledger.json'),
|
||||
JSON.stringify({ v: 1, findings: [], ...over }),
|
||||
);
|
||||
// The census's denominator is cross-checked against everything the round
|
||||
// reports, so a fixture claiming `fresh` findings must REPORT them: one
|
||||
// drafted comment per first-appearing finding.
|
||||
const round = (
|
||||
convergence: unknown,
|
||||
planOpts: Record<string, unknown> = {},
|
||||
) =>
|
||||
composeReview({
|
||||
) => {
|
||||
const fresh = (convergence as { fresh?: number } | undefined)?.fresh;
|
||||
return composeReview({
|
||||
planPath: coveredPlan(['verify', 'reverse-audit'], {
|
||||
prNumber: 8255,
|
||||
...planOpts,
|
||||
|
|
@ -8998,10 +9003,16 @@ describe('the convergence census and the non-convergence finding', () => {
|
|||
...(convergence === undefined
|
||||
? {}
|
||||
: { convergence: convergence as { fresh: number; induced: number } }),
|
||||
draftedComments: [
|
||||
{ path: 'src/a.ts', line: 3, body: '**[Suggestion]** one' },
|
||||
],
|
||||
draftedComments: Array.from(
|
||||
{ length: Math.max(fresh ?? 1, 1) },
|
||||
(_, i) => ({
|
||||
path: 'src/a.ts',
|
||||
line: i + 1,
|
||||
body: `**[Suggestion]** finding ${i + 1}`,
|
||||
}),
|
||||
),
|
||||
});
|
||||
};
|
||||
|
||||
it('reads a census only when it can be one', () => {
|
||||
expect(churnCensusOf({ fresh: 10, induced: 5 })).toEqual({
|
||||
|
|
@ -9025,15 +9036,20 @@ describe('the convergence census and the non-convergence finding', () => {
|
|||
expect(churnCensusOf(undefined)).toBeNull();
|
||||
});
|
||||
|
||||
it('sets the bar at a majority, over a round big enough to have one', () => {
|
||||
it('sets the bar at half or more, over a round big enough to have one', () => {
|
||||
// A ratio over two or three findings is rounding, not a trend: 2/2 is
|
||||
// 100% and says nothing, which is what the minimum exists to refuse.
|
||||
expect(aboveChurnBar({ fresh: CHURN_MIN_FRESH - 1, induced: 3 })).toBe(
|
||||
false,
|
||||
);
|
||||
// And the bar itself is a MAJORITY, not the measured baseline: roughly a
|
||||
// third of an ordinary re-review's findings are fix-induced, so a bar set
|
||||
// there would fire on every pull request that ever gets a second round.
|
||||
// Exactly the minimum is the weakest statement that is still a statement
|
||||
// — pinned from BOTH sides, or a raised constant silently disarms the
|
||||
// streak at exactly four first-appearing findings.
|
||||
expect(aboveChurnBar({ fresh: CHURN_MIN_FRESH, induced: 2 })).toBe(true);
|
||||
// And the bar itself is half or more, not the measured baseline: roughly
|
||||
// a third of an ordinary re-review's findings are fix-induced, so a bar
|
||||
// set there would fire on every pull request that ever gets a second
|
||||
// round.
|
||||
expect(aboveChurnBar({ fresh: 12, induced: 4 })).toBe(false);
|
||||
expect(aboveChurnBar({ fresh: 10, induced: 4 })).toBe(false);
|
||||
expect(aboveChurnBar({ fresh: 10, induced: 5 })).toBe(true);
|
||||
|
|
@ -9053,14 +9069,21 @@ describe('the convergence census and the non-convergence finding', () => {
|
|||
expect(r.body).not.toContain('is not converging');
|
||||
});
|
||||
|
||||
it('files the blocker on the second consecutive round above the bar', () => {
|
||||
it('files the blocker on the second round counted against the bar', () => {
|
||||
prevLedger({ round: 3, churnRounds: 1 });
|
||||
const r = round({ fresh: 11, induced: 7 });
|
||||
expect(parseLedger(r.body)!.churnRounds).toBe(CHURN_STREAK_TO_FILE);
|
||||
// Pins the WHOLE corrected claim: the counted-rounds phrasing (a
|
||||
// reversion to "consecutive" reds) and the half-or-more premise (a
|
||||
// reversion to "most" reds at the even-fresh boundary the bar allows).
|
||||
expect(r.body).toContain(
|
||||
'This pull request is not converging. Of the 11 findings first filed ' +
|
||||
"in round 4, 7 were introduced by the previous round's fixes for " +
|
||||
"this review's own findings — the 2nd consecutive round",
|
||||
"this review's own findings — the 2nd round counted against the " +
|
||||
'churn bar (rounds that could not measure carry the count rather ' +
|
||||
'than reset it), and in every counted round at least half of its ' +
|
||||
'first-appearing findings were introduced by the previous ' +
|
||||
"round's fixes.",
|
||||
);
|
||||
// It blocks. A claim that the loop cannot close itself is worth nothing
|
||||
// if the review then approves the pull request anyway.
|
||||
|
|
@ -9100,6 +9123,13 @@ describe('the convergence census and the non-convergence finding', () => {
|
|||
criticalsInline: 0,
|
||||
suggestionsInline: 0,
|
||||
convergence: { fresh: 11, induced: 7 },
|
||||
// The census's denominator is cross-checked against the round's own
|
||||
// reports, so the fixture must report what its census claims.
|
||||
draftedComments: Array.from({ length: 11 }, (_, i) => ({
|
||||
path: 'src/a.ts',
|
||||
line: i + 1,
|
||||
body: `**[Suggestion]** finding ${i + 1}`,
|
||||
})),
|
||||
});
|
||||
expect(filed.body).toContain('is not converging');
|
||||
expect(filed.cappedBy).not.toContain('criticals-unverified');
|
||||
|
|
@ -9170,4 +9200,58 @@ describe('the convergence census and the non-convergence finding', () => {
|
|||
expect(l.churnRounds).toBe(1);
|
||||
expect(r.body).not.toContain('is not converging');
|
||||
});
|
||||
|
||||
it('refuses a census that out-counts the round’s own reports', () => {
|
||||
// The census is model-written, and the module holds the cross-check
|
||||
// that needs no verifier: a FRESH finding only exists as something the
|
||||
// round reports — inline, body or deferred — so a denominator past all
|
||||
// three channels combined cannot be describing this round. Without the
|
||||
// bound, a round that reports nothing files the blocker on the model's
|
||||
// say-so alone.
|
||||
prevLedger({ round: 3, churnRounds: 1 });
|
||||
const r = composeReview({
|
||||
planPath: coveredPlan(['verify', 'reverse-audit'], { prNumber: 8255 }),
|
||||
env: ENV,
|
||||
modelId: MODEL,
|
||||
criticalsInline: 0,
|
||||
suggestionsInline: 0,
|
||||
convergence: { fresh: 11, induced: 7 },
|
||||
});
|
||||
expect(r.event).toBe('APPROVE');
|
||||
expect(r.body).not.toContain('is not converging');
|
||||
const l = parseLedger(r.body)!;
|
||||
// The refused census writes nothing; the streak carries, exactly as an
|
||||
// absent census does.
|
||||
expect(l.fresh).toBeUndefined();
|
||||
expect(l.induced).toBeUndefined();
|
||||
expect(l.churnRounds).toBe(1);
|
||||
});
|
||||
|
||||
it('keeps filing on every counted round past the streak bar', () => {
|
||||
// Pins the `>=` in the filing condition: every other filing test lands
|
||||
// the streak at exactly the bar, so mutating `>=` to `===` keeps them
|
||||
// green while a genuinely churning pull request — blocker already
|
||||
// filed, next round above the bar again — silently never receives it
|
||||
// again. The ordinal assertion doubles as the `rd` pin for
|
||||
// `ordinalSuffix`.
|
||||
prevLedger({ round: 4, churnRounds: CHURN_STREAK_TO_FILE });
|
||||
const r = round({ fresh: 12, induced: 8 });
|
||||
expect(r.body).toContain('the 3rd round counted against the churn bar');
|
||||
expect(parseLedger(r.body)!.churnRounds).toBe(3);
|
||||
expect(r.event).toBe('REQUEST_CHANGES');
|
||||
});
|
||||
|
||||
it('renders the ordinal past the filing bar — rd, teen th, and st', () => {
|
||||
// `ordinalSuffix` is exercised at streak 2 only by the filing tests;
|
||||
// the `rd` branch, the teens guard and the `st` branch are reachable on
|
||||
// a genuinely churning pull request (the streak caps at
|
||||
// LEDGER_MAX_ROUND), and a "11st" inside the blocker must not ship with
|
||||
// the suite green.
|
||||
const body = (streak: number) =>
|
||||
nonConvergenceCritical({ fresh: 11, induced: 7 }, streak, streak + 2);
|
||||
expect(body(3)).toContain('the 3rd round counted');
|
||||
expect(body(11)).toContain('the 11th round counted');
|
||||
expect(body(12)).toContain('the 12th round counted');
|
||||
expect(body(21)).toContain('the 21st round counted');
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -1191,7 +1191,25 @@ export function composeReview(
|
|||
// predating the field, an age reference the round could not validate — and
|
||||
// reading it as "converging" would let one unmeasurable round wipe a
|
||||
// standing claim about the pull request.
|
||||
const churnCensus = churnCensusOf(input.convergence);
|
||||
// The census is the model-written half of this trigger, so it gets the
|
||||
// module's one-sided cross-check before it can arm anything: a FRESH
|
||||
// finding only exists as something this round REPORTS — an inline draft, a
|
||||
// body Critical, a deferral — so a denominator past everything reported,
|
||||
// all three channels counted together, is a census this round cannot have
|
||||
// measured. Refused as no census at all — the streak carries, exactly as
|
||||
// absence does — or a round that reported nothing could file the
|
||||
// non-convergence blocker on the model's say-so alone.
|
||||
const reportedThisRound =
|
||||
(Array.isArray(input.draftedComments) ? input.draftedComments.length : 0) +
|
||||
(Array.isArray(input.bodyCriticals) ? input.bodyCriticals.length : 0) +
|
||||
(Array.isArray(input.deferredSuggestions)
|
||||
? input.deferredSuggestions.length
|
||||
: 0);
|
||||
const readCensus = churnCensusOf(input.convergence);
|
||||
const churnCensus =
|
||||
readCensus !== null && readCensus.fresh > reportedThisRound
|
||||
? null
|
||||
: readCensus;
|
||||
const churnAbove = aboveChurnBar(churnCensus);
|
||||
const churnRounds = churnAbove
|
||||
? Math.min(prevFacts.churnRounds + 1, LEDGER_MAX_ROUND)
|
||||
|
|
@ -1331,15 +1349,15 @@ export function composeReview(
|
|||
export const CHURN_MIN_FRESH = 4;
|
||||
|
||||
/**
|
||||
* How many consecutive rounds above the bar are needed before the finding is
|
||||
* filed.
|
||||
* How many rounds counted against the churn bar are needed before the finding
|
||||
* is filed.
|
||||
*
|
||||
* One round above the bar is an ordinary re-review: the fix round touched the
|
||||
* code, so of course this round's findings are on it, and the measured
|
||||
* baseline for that is roughly a third. Two consecutive rounds is the
|
||||
* shortest window in which "each round is reviewing the last round's answer
|
||||
* to it" is an observation rather than a single step — the same argument
|
||||
* `prevPosted` makes for the volume trend.
|
||||
* baseline for that is roughly a third. Two counted rounds is the shortest
|
||||
* window in which "each round is reviewing the last round's answer to it" is
|
||||
* an observation rather than a single step — the same argument `prevPosted`
|
||||
* makes for the volume trend.
|
||||
*/
|
||||
export const CHURN_STREAK_TO_FILE = 2;
|
||||
|
||||
|
|
@ -1364,13 +1382,15 @@ export function churnCensusOf(
|
|||
|
||||
/**
|
||||
* Is this round above the churn bar? Half or more of its first-appearing
|
||||
* findings anchored on lines pushed since the previous review.
|
||||
* findings attributed by the fix-induced rule to the previous round's fixes
|
||||
* — the ATTRIBUTED count, not findings on newly pushed lines.
|
||||
*
|
||||
* Half, not the measured third: the third IS the baseline — the rate an
|
||||
* ordinary, healthy re-review runs at — and a bar set at the baseline fires
|
||||
* on every pull request that ever gets a second round. The claim this arms is
|
||||
* that the round did MORE work on the previous round's answer than on the
|
||||
* change itself, and that claim needs a majority to be worth making.
|
||||
* on every pull request that ever gets a second round. The claim this arms
|
||||
* is that at least half the round's first-appearing findings were work the
|
||||
* previous round created — below that, the round is still mostly reviewing
|
||||
* the change itself.
|
||||
*
|
||||
* Integer arithmetic on purpose (`induced * 2 >= fresh`): a float ratio
|
||||
* compared against 0.5 puts the bar's behaviour at 5/10 at the mercy of
|
||||
|
|
@ -1400,12 +1420,15 @@ export function nonConvergenceCritical(
|
|||
`This pull request is not converging. Of the ${census.fresh} findings ` +
|
||||
`first filed in round ${thisRound}, ${census.induced} were introduced by ` +
|
||||
`the previous round's fixes for this review's own findings — the ` +
|
||||
`${streak}${ordinalSuffix(streak)} ` +
|
||||
`consecutive round in which most of the review's new work was work the ` +
|
||||
`previous round created. Filing more findings will not close this: split ` +
|
||||
`the change into separately reviewable pieces, or reconsider the approach ` +
|
||||
`under review, and re-request review after. (Counted by the review from ` +
|
||||
`its own ledger and diff; it blocks so the decision is a person's.)`
|
||||
`${streak}${ordinalSuffix(streak)} round counted against the churn bar ` +
|
||||
`(rounds that could not measure carry the count rather than reset it), ` +
|
||||
`and in every counted round at least half of its first-appearing ` +
|
||||
`findings were introduced by the previous round's fixes. Filing more ` +
|
||||
`findings will not close this: split the change into separately ` +
|
||||
`reviewable pieces, or ` +
|
||||
`reconsider the approach under review, and re-request review after. ` +
|
||||
`(Counted by the review from its own ledger and diff; it blocks so the ` +
|
||||
`decision is a person's.)`
|
||||
);
|
||||
}
|
||||
|
||||
|
|
@ -2076,12 +2099,14 @@ function composeReviewBody(
|
|||
unreviewed.push(...layerAuditGate(input.planPath, input.env).unreviewed);
|
||||
}
|
||||
// The non-convergence finding rides the SAME channel as the gates above,
|
||||
// and for the same reason: it is deterministic by provenance. This module
|
||||
// counted it, from its own marker and the census the orchestrator measured
|
||||
// — there is no verifier for it and there never will be, so routing it
|
||||
// through `modelBodyCriticals` would demand one and cap the verdict on a
|
||||
// gap no repair can close. It is pushed AFTER that capture on purpose;
|
||||
// moving this line above it silently converts the finding into an
|
||||
// and for the same reason: it is deterministic by provenance — this module
|
||||
// counted the streak from its own marker and side file, and the census
|
||||
// beside it is the orchestrator-supplied half, checked in `composeReview`
|
||||
// for shape and against everything this round reports before it could arm
|
||||
// the streak. There is no verifier for it and there never will be, so
|
||||
// routing it through `modelBodyCriticals` would demand one and cap the
|
||||
// verdict on a gap no repair can close. It is pushed AFTER that capture on
|
||||
// purpose; moving this line above it silently converts the finding into an
|
||||
// unsatisfiable cap.
|
||||
if (nonConvergence) bodyCriticals.push(nonConvergence);
|
||||
|
||||
|
|
|
|||
|
|
@ -822,8 +822,29 @@ describe('the convergence census and the churn streak', () => {
|
|||
const written = serializeLedger({ ...base, churnRounds: over });
|
||||
expect(written).toContain(`"churnRounds":${LEDGER_MAX_ROUND}`);
|
||||
expect(written).not.toContain(String(over));
|
||||
expect(parseLedger(handCrafted({ churnRounds: over }))?.churnRounds).toBe(
|
||||
LEDGER_MAX_ROUND,
|
||||
);
|
||||
expect(
|
||||
parseLedger(handCrafted({ round: LEDGER_MAX_ROUND, churnRounds: over }))
|
||||
?.churnRounds,
|
||||
).toBe(LEDGER_MAX_ROUND);
|
||||
});
|
||||
|
||||
it('clamps a recovered streak to the marker’s own round', () => {
|
||||
// The streak counts rounds INSIDE the round it rides, so a legitimate
|
||||
// marker can never carry more counted rounds than rounds it claims.
|
||||
// The marker body is any GitHub user's writable surface: unclamped,
|
||||
// `{round: 2, churnRounds: 9999}` beside one honest above-bar round
|
||||
// posts "the 10000th round…" on a pull request in its third. Same
|
||||
// invariant the finding-id squat filter enforces — a claim about rounds
|
||||
// that did not exist is not read.
|
||||
const forged = `<!-- qwen-review-ledger ${JSON.stringify({
|
||||
v: 1,
|
||||
round: 2,
|
||||
findings: [{ id: 'R2-1', sev: 'S', file: 'a.ts', title: 'x' }],
|
||||
churnRounds: 9999,
|
||||
})} -->`;
|
||||
expect(parseLedger(forged)?.churnRounds).toBe(2);
|
||||
// A streak AT the round rides untouched — the clamp strips nothing a
|
||||
// legitimate marker can carry (round 5 is the handCrafted default).
|
||||
expect(parseLedger(handCrafted({ churnRounds: 5 }))?.churnRounds).toBe(5);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -125,11 +125,13 @@ export interface Ledger {
|
|||
fresh?: number;
|
||||
induced?: number;
|
||||
/**
|
||||
* How many consecutive rounds — this one included — have come in above the
|
||||
* churn bar. This is the ONE field here that is neither telemetry nor a
|
||||
* gate on scope: it is the review's own standing claim about the pull
|
||||
* request, carried exactly the way a finding id is, and `compose-review`
|
||||
* reads it back to decide whether to file the non-convergence finding.
|
||||
* How many rounds — this one included — have been counted against the
|
||||
* churn bar since the last round measured converging; a round that could
|
||||
* not measure carries the count without adding to it. This is the ONE
|
||||
* field here that is neither telemetry nor a gate on scope: it is the
|
||||
* review's own standing claim about the pull request, carried exactly the
|
||||
* way a finding id is, and `compose-review` reads it back to decide
|
||||
* whether to file the non-convergence finding.
|
||||
*
|
||||
* So it does NOT ride in the volume tier that sheds first. It is a single
|
||||
* small integer, and the pull request most likely to be churning is also
|
||||
|
|
@ -538,8 +540,15 @@ export function parseLedger(body: string | undefined): Ledger | null {
|
|||
// when the census beside it did not, because it is the field the
|
||||
// non-convergence rule reads and the census is only what the body
|
||||
// quotes. Clamped on read as on write; a shape the serializer would not
|
||||
// have written does not survive.
|
||||
const churnRounds = streakOf(raw.churnRounds);
|
||||
// have written does not survive. Clamped to the marker's own ROUND too:
|
||||
// the streak counts rounds INSIDE the round it rides, and the pipeline's
|
||||
// own writes advance it at most once per round, so a legitimate marker
|
||||
// can never carry more counted rounds than rounds it claims. The marker
|
||||
// body is any GitHub user's writable surface, and an unclamped streak
|
||||
// inflates the posted ordinal ("the 10000th round…") past everything the
|
||||
// pull request ever ran. Same invariant the finding-id filter enforces
|
||||
// above: a claim about rounds that did not exist is not read.
|
||||
const churnRounds = Math.min(streakOf(raw.churnRounds) ?? 0, raw.round);
|
||||
return {
|
||||
v: 1,
|
||||
round: raw.round,
|
||||
|
|
@ -550,9 +559,7 @@ export function parseLedger(body: string | undefined): Ledger | null {
|
|||
...(posted === undefined ? {} : { posted }),
|
||||
...(prevPosted === undefined ? {} : { prevPosted }),
|
||||
...(census ? { fresh, induced } : {}),
|
||||
...(churnRounds === undefined || churnRounds === 0
|
||||
? {}
|
||||
: { churnRounds }),
|
||||
...(churnRounds === 0 ? {} : { churnRounds }),
|
||||
};
|
||||
} catch {
|
||||
return null;
|
||||
|
|
|
|||
|
|
@ -59,6 +59,39 @@ describe('persistRecoveredLedger', () => {
|
|||
}
|
||||
});
|
||||
|
||||
it('round-trips the churn fields on the plain recovery path', () => {
|
||||
// The identity-known write keeps the recovered ledger WHOLE: the streak
|
||||
// and its census are this account's own certified state for the round
|
||||
// it recovered, and `compose-review` reads the streak back out of this
|
||||
// file to decide whether the non-convergence finding files. A future
|
||||
// edit field-picking this write the way the anonymous branch does must
|
||||
// red here first.
|
||||
const dir = mkdtempSync(join(tmpdir(), 'prev-ledger-'));
|
||||
const side = join(dir, 'side.json');
|
||||
try {
|
||||
persistRecoveredLedger(
|
||||
side,
|
||||
{
|
||||
ledger: { ...ledger, churnRounds: 2, fresh: 10, induced: 6 },
|
||||
commitId: 'a'.repeat(40),
|
||||
reviewId: 43,
|
||||
},
|
||||
{ noOwnReview: false, identityKnown: true },
|
||||
);
|
||||
const written = JSON.parse(readFileSync(side, 'utf8'));
|
||||
expect(written).toEqual({
|
||||
...ledger,
|
||||
churnRounds: 2,
|
||||
fresh: 10,
|
||||
induced: 6,
|
||||
commitId: 'a'.repeat(40),
|
||||
reviewId: 43,
|
||||
});
|
||||
} 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 —
|
||||
|
|
@ -230,9 +263,15 @@ describe('persistRecoveredLedger', () => {
|
|||
// past it, so they must go the way the anchor and the age
|
||||
// reference go — kept, they would attribute this account's round-7
|
||||
// posting count to the foreign round that won recovery, and the
|
||||
// next compose would stamp it as `prevPosted`.
|
||||
// next compose would stamp it as `prevPosted`. The churn fields
|
||||
// are the same class of round-specific fact — kept, they would
|
||||
// re-date this account's streak across the foreign round and
|
||||
// discard the foreign winner's own streak state.
|
||||
posted: 4,
|
||||
prevPosted: 2,
|
||||
churnRounds: 2,
|
||||
fresh: 10,
|
||||
induced: 6,
|
||||
}),
|
||||
);
|
||||
persistRecoveredLedger(
|
||||
|
|
@ -258,6 +297,12 @@ describe('persistRecoveredLedger', () => {
|
|||
});
|
||||
expect(written.sha).toBeUndefined();
|
||||
expect(written.commitId).toBeUndefined();
|
||||
// The drop witness: the fixture carries a streak and a census, and
|
||||
// the written file must not — keeping them arms the blocker one
|
||||
// round early across a round this account never ran.
|
||||
expect(written.churnRounds).toBeUndefined();
|
||||
expect(written.fresh).toBeUndefined();
|
||||
expect(written.induced).toBeUndefined();
|
||||
} finally {
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1155,12 +1155,21 @@ export function persistRecoveredLedger(
|
|||
// foreign round 5 that won recovery — one fabricated point on a trend
|
||||
// whose whole value is that its points are real. Absence is already
|
||||
// the "not recorded" reading downstream, so dropping them degrades
|
||||
// exactly as a pre-telemetry predecessor does.
|
||||
// exactly as a pre-telemetry predecessor does. The churn fields are
|
||||
// the same class: the census describes the round being advanced past,
|
||||
// and a streak re-dated across a round this account never ran would
|
||||
// arm the non-convergence blocker one round early — and silently
|
||||
// discard the foreign winner's own streak state, a below-bar reset
|
||||
// included. Dropped, the streak re-arms from scratch: a round of
|
||||
// lateness on a genuinely churning pull request, never earliness.
|
||||
const {
|
||||
sha: _droppedSha,
|
||||
commitId: _droppedCommitId,
|
||||
posted: _droppedPosted,
|
||||
prevPosted: _droppedPrevPosted,
|
||||
fresh: _droppedFresh,
|
||||
induced: _droppedInduced,
|
||||
churnRounds: _droppedChurnRounds,
|
||||
...kept
|
||||
} = existing;
|
||||
mkdirSync(dirname(sideFilePath), { recursive: true });
|
||||
|
|
|
|||
|
|
@ -804,13 +804,13 @@ The ledger has two sources, in priority order: **the PR itself** — `pr-context
|
|||
|
||||
**The test is mechanical on both operands, and both must hold.** (1) The finding's anchor falls inside a hunk **changed since the age reference** — the side file's `commitId`, validated and diffed exactly as the code-age rule below prescribes (`git --literal-pathspecs diff <commitId>..HEAD --unified=0 -- '<file>'`, same quoting, same pathspec proof, same two doubt states); code that predates the previous round cannot have been introduced by its fix. (2) A **previous-round ledger entry named that site** — the same file, and a line inside or adjacent to the hunk that answered it — and you can state the causal link in one clause: what the fix changed, and how that change produced this defect. A traced link, not an adjacency: two unrelated defects in one busy file are two findings.
|
||||
|
||||
**Three guardrails, and none of them is optional.** Attribution is a **bookkeeping** decision and never a posting one: a fix-induced finding posts, inline, at its own severity, exactly as it would under a fresh id — if you ever find yourself reaching for it to avoid reporting something, you have the rule backwards. It applies **only when the new defect is at least as severe and as confident as the entry it carries** — the same guard supersession carries, and for the same reason: a Critical id that quietly becomes a Suggestion retires a blocker nobody ruled on, so when the new defect is weaker, rule the entry `fixed` and file the new defect under its own fresh id. And when either operand is missing — no `commitId`, no worktree, the **context-unavailable** state, a previous entry you cannot identify, a causal link you cannot trace — **mint the fresh id**: unattributed is the safe direction, it is what every round did before this rule existed, and a wrong attribution is worse than none because it welds two claims to one id that later rounds cannot separate.
|
||||
**Four guardrails, and none of them is optional.** Attribution is a **bookkeeping** decision and never a posting one: a fix-induced finding posts, inline, at its own severity, exactly as it would under a fresh id — if you ever find yourself reaching for it to avoid reporting something, you have the rule backwards. It applies **only when the new defect is at least as severe and as confident as the entry it carries** — the same guard supersession carries, and for the same reason: a Critical id that quietly becomes a Suggestion retires a blocker nobody ruled on, so when the new defect is weaker, rule the entry `fixed` and file the new defect under its own fresh id. And when either operand is missing — no `commitId`, no worktree, the **context-unavailable** state, a previous entry you cannot identify, a causal link you cannot trace — **mint the fresh id**: unattributed is the safe direction, it is what every round did before this rule existed, and a wrong attribution is worse than none because it welds two claims to one id that later rounds cannot separate. And **one re-report per original id per round**: when two distinct new defects trace to the same previous entry, the first takes the id and the second takes a fresh `R<round>-<n>` — two entries under one id are a duplicate id, and the artifact validator refuses the round's findings whole. Count the second in `fresh` but not `induced`: it is a new defect, but attribution keys on the id, and the id is spent.
|
||||
|
||||
**What it buys.** The ledger stops spending one id per round on a single churning site, so the marker's fifty-entry work list holds more distinct claims; the author reads one thread per site instead of a new one each round; and the count this produces — how many of the round's findings were fix-induced — is what the non-convergence rule below reads. That count is the honest measure of a loop's productivity, and it is not available to a review that renumbers everything every round.
|
||||
|
||||
**Count the round as you rule it, and hand the two numbers over.** While you walk the findings above, keep a running census of exactly two numbers. **`fresh`** — how many findings FIRST APPEAR this round: the ones that took a new `R<round>-<n>` id, plus the fix-induced ones that took a previous id (they are new defects; the id is bookkeeping), and NOT the entries you ruled `still stands`, `fixed`, `cannot tell` or `superseded`. **`induced`** — how many of those `fresh` findings the fix-induced rule above **attributed**: the ones you traced to the change that answered a previous entry. `induced` is a SUBSET of `fresh` and can never exceed it. **It is the attributed count, not the count of findings on new lines**, and the difference is the whole precision of the mechanism: a pull request whose author pushed a new feature between rounds has most of its new findings on new lines and has NOT created them out of the review — there is no previous entry to trace them to, so they are `fresh` and not `induced`. A bar built on the looser number would block a pull request for growing. Carry the pair into the compose state as `convergence: {"fresh": N, "induced": M}` — one object, two integers, no prose. Omit the field entirely when you could not measure it: no `commitId`, no worktree, the **context-unavailable** state, or an age reference that failed validation. **Omitting is not the same as zero**, and the difference is load-bearing: `compose-review` reads a measured-and-low census as "this round converged" and resets the streak, and an absent one as "not measured" and carries the streak untouched. Writing `{"fresh": 0, "induced": 0}` for a round you did not measure erases a standing claim about the pull request.
|
||||
|
||||
**You count; the module rules.** `compose-review` owns the threshold, the streak and the finding — do not compute a verdict from these numbers yourself, do not mention convergence in your Summary on the strength of them, and do not adjust what you post because of them. When two consecutive rounds come in above the bar, the module appends its own body Critical (`This pull request is not converging…`) with the counts, and the event becomes `REQUEST_CHANGES`. **That finding is the module's, and the narrated-away-cap rule covers it exactly**: it is not yours to soften, re-word, delete from the body, or explain away in the Summary, any more than a cap is — if you believe it is wrong, the answer is a corrected census, never a corrected verdict. It is deterministic by provenance (this module counted it from its own marker and your census), so no verifier is owed and none will ever exist for it; it carries no anchor because the claim is about the pull request, not a line.
|
||||
**You count; the module rules.** `compose-review` owns the threshold, the streak and the finding — do not compute a verdict from these numbers yourself, do not mention convergence in your Summary on the strength of them, and do not adjust what you post because of them. When two rounds come in counted against the bar, the module appends its own body Critical (`This pull request is not converging…`) with the counts, and the event becomes `REQUEST_CHANGES`. **That finding is the module's, and the narrated-away-cap rule covers it exactly**: it is not yours to soften, re-word, delete from the body, or explain away in the Summary, any more than a cap is — if you believe it is wrong, the answer is a corrected census, never a corrected verdict. It is deterministic by provenance (this module counted it from its own marker and your census), so no verifier is owed and none will ever exist for it; it carries no anchor because the claim is about the pull request, not a line.
|
||||
|
||||
Render the rulings as a short table at the top of the Findings section — id, one-line title, this round's status — so the report reads as a continuation, the way a human reviewer's round-2 comment opens with "M1 is fixed". The incremental scope rule does not conflict with this: the _diff_ reviewed is `lastCommitSha..HEAD`, but a ledger ruling reads the code at HEAD, which every agent already has.
|
||||
|
||||
|
|
@ -1215,7 +1215,7 @@ Then reference each finding's `assets` URLs in its inline comment body as `![evi
|
|||
- `suggestionsDroppedAsDuplicates` — one entry per **confirmed** Suggestion you did not re-post because it is already reported on the PR (a prior round, a concurrent reviewer, an overlap drop), each naming the finding and where it already lives — never the finding's own text, which the never-in-body rule above keeps out of the body (its carve-out for this account is exactly that name + location), e.g. `R1-2 loose review-config pins — already reported (comment 3788857379)`. Use this INSTEAD of bumping `suggestionsDiscarded` for duplicate drops: the two render different sentences, and the discarded one asserts an anchor failure that never happened. They still count toward `S`.
|
||||
- `cannotTellCriticals` — one line per existing PR Critical whose Step 6 re-check landed on `cannot tell` (location + what could not be determined).
|
||||
- `deferredSuggestions` — the findings the convergence posture deferred, as **typed entries** `{file, line?, source, severity, title, locations?}` copied from the findings artifact (Step 6's posture section — **high-confidence Suggestions that would otherwise post**, never low-confidence or Nice-to-have entries, which stay terminal-only; a `Critical` entry is relocated into the body Criticals, a malformed or free-text entry is refused). Deferred findings are **not** drafted into `comments` and are **not** counted toward `S` — the body renders them as a disclosed, non-capping list (up to 20 entries × 240 chars, overflow counted; the full set lives in the findings artifact), so the deferral is on the PR record without regenerating a review round. Non-deterministic entries **do** count toward the verifier-delivery floor — a deferred claim still publishes — while `source: build|test|probe` entries are excluded by that field exactly as body Criticals are by their tag: they are pre-confirmed, no verifier ever exists for them, and demanding one would cap the verdict with a gap no repair can close. A deferral never withholds the ledger anchor.
|
||||
- `convergence` — this round's census from Step 6's fix-induced rule, as `{"fresh": N, "induced": M}`: how many findings first appeared this round, and how many of those the fix-induced rule attributed to a previous entry's fix (the ATTRIBUTED count, not the count of findings on newly pushed lines). Two integers, `induced <= fresh`; a malformed pair, a float, a negative, or a numerator larger than its denominator is read as no census at all. **Omit the field when the round could not measure it** — absence carries the churn streak forward, a measured low census resets it, and zeros written for an unmeasured round are the one input that silently retires a standing non-convergence claim. `compose-review` owns everything downstream: the bar (half or more of `fresh`, and at least 4 `fresh`), the streak it stamps into the marker as `churnRounds`, and the body Critical it files itself on the second consecutive round above the bar.
|
||||
- `convergence` — this round's census from Step 6's fix-induced rule, as `{"fresh": N, "induced": M}`: how many findings first appeared this round, and how many of those the fix-induced rule attributed to a previous entry's fix (the ATTRIBUTED count, not the count of findings on newly pushed lines). Two integers, `induced <= fresh`; a malformed pair, a float, a negative, or a numerator larger than its denominator is read as no census at all. **Omit the field when the round could not measure it** — absence carries the churn streak forward, a measured low census resets it, and zeros written for an unmeasured round are the one input that silently retires a standing non-convergence claim. `compose-review` owns everything downstream: the bar (half or more of `fresh`, and at least 4 `fresh`), the streak it stamps into the marker as `churnRounds`, and the body Critical it files itself on the second round counted against the bar.
|
||||
- `severityFloor` — the Step 1 verdict's floor, carried UNRESOLVED (`critical`, `suggestion`, or the literal `auto` — never `auto`'s per-round resolution, which would masquerade as the operator's explicit override). This is the deferral channel's licence check: a non-empty `deferredSuggestions` under an explicit `suggestion` floor (posture off) or on round 1 under `auto` (no posture, no age reference) is an unlicensed deferral — `compose-review` renders the list but CAPS the verdict and says so, the same fail-closed treatment as unreviewed scope: the findings stay visible, nothing certifies past them, and the round is never lost to a refusal.
|
||||
- `planPath` — the plan report from Step 1. **Coverage is not an input.** `submit` recomputes it from the harness's transcripts, because a `coverage` object you typed is a document you write — and the last time this skill trusted one, it was fabricated.
|
||||
- `findingsPath` — the cumulative reverse-audit findings file at loop end (high effort only): the same file every round's `--findings` received, after the final merge. `compose-review` reads it for surviving `— [unverified]` tags — a tag at compose time is an entry no verifier ruled on, and it caps the verdict at Comment, disclosed in the body. Omit at medium and low; they run no Step 5.
|
||||
|
|
|
|||
|
|
@ -686,7 +686,7 @@ describe('bundled review skill', () => {
|
|||
);
|
||||
expect(body).toContain('changed since the age reference');
|
||||
expect(body).toContain('you can state the causal link in one clause');
|
||||
// The three guardrails. The first keeps attribution from becoming a way
|
||||
// The first three guardrails. The first keeps attribution from becoming a way
|
||||
// to not report something, the second keeps a Critical id from quietly
|
||||
// becoming a Suggestion, and the third fixes the fail direction at
|
||||
// "mint a new id" — the behaviour every round had before the rule.
|
||||
|
|
@ -697,6 +697,11 @@ describe('bundled review skill', () => {
|
|||
'only when the new defect is at least as severe and as confident as the entry it carries',
|
||||
);
|
||||
expect(body).toContain('**mint the fresh id**');
|
||||
// The fourth guardrail: two distinct new defects tracing to the same
|
||||
// previous entry cannot both take its id — the artifact validator
|
||||
// refuses a duplicate id and with it the whole round's findings.
|
||||
expect(body).toContain('**one re-report per original id per round**');
|
||||
expect(body).toContain('Count the second in `fresh` but not `induced`');
|
||||
});
|
||||
|
||||
it('pins the census contract and the module-owns-the-verdict split', () => {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue