Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion packages/cli/src/commands/review/compose-review.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,11 @@ import {
} from './lib/deadline.js';
import { getGhHost, setGhHost } from './lib/gh.js';
import { BRIEFS } from './lib/agent-briefs.js';
import { LEDGER_MAX_ROUND, parseLedger } from './lib/ledger.js';
import {
LEDGER_MAX_ROUND,
LEDGER_MAX_VOLUME,
parseLedger,
} from './lib/ledger.js';
import { countInlineFindings } from './lib/inline-counts.js';
import {
composeReview,
Expand Down Expand Up @@ -8750,6 +8754,22 @@ describe('convergence telemetry — volume, carried in the marker', () => {
expect(parseLedger(r.body)?.posted).toBe(3);
});

it('clamps the count at its origin, so every surface agrees', () => {
// The terminal line, the marker and the artifact must never disagree
// about one round's count. This is the defensive over-cap case: the
// reader is applied where the number is derived, not only where it is
// written, so no surface can see the raw value.
const r = composeReview({
planPath: plan(),
modelId: 'm',
criticalsInline: 0,
suggestionsInline: LEDGER_MAX_VOLUME + 5,
draftedComments: drafts(LEDGER_MAX_VOLUME + 5),
});
expect(r.postedInline).toBe(LEDGER_MAX_VOLUME);
expect(parseLedger(r.body)?.posted).toBe(LEDGER_MAX_VOLUME);
});

it('counts the POST-enforcement set — what submit actually sends', () => {
// The floor moves two of the three out of the posting set, so the
// recorded volume is one. Counting the drafts would record a number no
Expand Down
10 changes: 8 additions & 2 deletions packages/cli/src/commands/review/compose-review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1543,8 +1543,14 @@ function composeReviewBody(
): ComposeReviewResult {
// The posting set this body describes — `input` here is already the
// post-enforcement one, so the count needs no second derivation and
// cannot disagree with the marker's.
const postedInline = (input.draftedComments ?? []).length;
// cannot disagree with the marker's. Clamped AT THE ORIGIN through the
// shared reader: every other site that reads a volume applies it, and the
// one that did not was this count on its way to the terminal line, which
// in the defensive over-cap case would have printed the raw number beside
// a marker recording the capped one — the two-outputs-disagree failure
// the shared reader's own docstring exists to prevent. `?? 0` is
// unreachable for an array length; it keeps the type honest.
const postedInline = volumeOf((input.draftedComments ?? []).length) ?? 0;
const criticalsInline = toCount(input.criticalsInline, 'criticalsInline');
const suggestionsInline = toCount(
input.suggestionsInline,
Expand Down
12 changes: 10 additions & 2 deletions packages/cli/src/commands/review/save-artifact.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,17 @@ const verdict = {
deferredCount: 2,
// Non-empty for the same reason — absent defaults to [].
floorEnforced: [1],
// Non-zero for the same reason — absent defaults to 0.
// Non-zero on purpose too, but for the OPPOSITE reason to its siblings:
// this field's absence is preserved, not defaulted, so the fixture value
Comment thread
wenshao marked this conversation as resolved.
// proves passthrough against a validator that would otherwise omit the
// field entirely.
postedInline: 3,
// Also non-default on purpose, for the same reason.
// Also non-default on purpose — and on the DEFAULTING side, with
// `deferredCount` and `floorEnforced`: an absent `bodyTrim` reads as an
// untrimmed one and the field is always emitted. Spelled out rather than
// said as "the same reason", which would now resolve against the
// preserved-absence block above it and teach the opposite of what
// `save-artifact.ts` does.
bodyTrim: { sections: 2, deferralList: true, fold: true, truncated: true },
lowSignal: { agents: 4, srcDiffLines: 120 },
verdictLine: 'Verdict: Comment — Request changes was downgraded',
Expand Down
Loading