Skip to content

Give the Review stage the producer it never had - #7539

Closed
gunbai-bot[bot] wants to merge 2 commits into
mainfrom
session/proud-swift-104-reviewtally
Closed

gunbai-bot[bot] wants to merge 2 commits into
mainfrom
session/proud-swift-104-reviewtally

Conversation

@gunbai-bot

@gunbai-bot gunbai-bot Bot commented Aug 1, 2026 •

Copy link
Copy Markdown
Contributor

The Review stage has a complete, fail-closed judgment that nothing in production can ask a question of. This gives it a producer.

The gap

gunbc.code_change_workflow's decide_code_change_transition adjudicates a MergeReadinessTally across twelve typed refusal arms — CodeChangeHeadShaMismatch, MissingReviewTally, TallyRequestChanges, TallyBlockingFindings, NotMergeableClean, and the rest. Verified against the whole tree:

  • MergeReadinessTally is constructed only inside witnesses.
  • decide_code_change_transition is called only from its own witness.
  • ReviewVerdictParseRequest, ReviewVerdictParseResult, ParsedReport, ParseDeferred, expected_head_sha — zero consumers outside their declaring module.
  • Checked the Rust side too: no producer there either.

That is the same state Publication was in before #7498 gave it one, and the same sentence roadmap_publish's own end_to_end_note used about itself — "the adjudicator was a judgment nothing could ask."

The shape follows the landed precedent

This is the pure half, exactly as roadmap_publish / roadmap_publish_observe split: it takes reports a caller already holds and decides everything from them, so every arm is reachable in a hermetic witness with no dashboard, no network and no token. The transport half — fetching artifacts and parsing bodies into verdicts — is deliberately not here. It rides the process-observation substrate (#7525) and is a separate lane.

The tally is wrapped, not extended

Excluded stale reviews have to be counted — §5 says a degradation is typed, located and countable, never absorbed — and MergeReadinessTally has no field for them. Extending it would edit a carrier whose consumer is a live twelve-arm adjudicator. Wrapping it does not: ReviewTallyObservation carries the tally beside the exclusions, and decide_code_change_transition keeps taking exactly the MergeReadinessTally it takes today. That is the shape RemoteBranchObservation already uses one module over, so it is precedent rather than invention.

Three decisions, each with a claim

Approvals count distinct reviewers, not artifacts. One provider re-reviewing the same head produces two approving artifacts. Counting artifacts would let a single reviewer satisfy a two-approval bar by reviewing twice — the bar defeated arithmetically rather than by any decision anyone made. Request-changes counts the same way. Blocking findings count per finding, because two reviewers finding one defect is still one defect, but three defects from one reviewer is three.

A stale review is excluded and named, never dropped. It was read perfectly well; it is simply about another commit — the ordinary state after a worker pushes while a review is in flight. Dropping it would zero the exclusion's frequency by construction, so nobody would ever learn that reviews are going stale faster than they land.

A report about a different PR refuses. A stale review is evidence about this PR at another commit, so excluding and counting it is honest. A foreign report is not evidence about this PR at all, and silently filtering it would hand back a confident tally assembled from whatever happened to match — a wrong answer that looks like a right one.

Verification

12 claims green by execution via claim_batch — not merely compiled.

The RED control is proven, not asserted. Replacing count(distinct_reviewer_keys(...)) with count(approving) fails exactly one claim — one_reviewer_approving_twice_counts_once — and leaves the other 11 green. Executed, then reverted. That discrimination is the whole point: every other case in the file already has distinct reviewers, so only the intended claim can detect the defect.

gunbc compile --target dag: 0 blocking, advisories pre-existing in untouched files.

Scope

No existing carrier is modified — this adds one module and one witness file. MergeReadinessTally, ReviewVerdictReport and decide_code_change_transition are untouched.

Not included, and stated so the bound is visible: nothing here fetches a review artifact or parses a comment body into a verdict. A change that broke the fetch while leaving review_tally_of intact would keep these claims green.

gunbc-ci-auto-heal and others added 2 commits August 1, 2026 02:38
gunbc.code_change_workflow's decide_code_change_transition already adjudicates a
MergeReadinessTally with twelve typed refusal arms -- stale head, missing tally,
request-changes, blocking findings, not-mergeable-clean and the rest. Nothing in
the corpus BUILDS one. A whole-tree search finds MergeReadinessTally constructed
only inside witnesses, decide_code_change_transition called only from its own,
and ReviewVerdictParseRequest / ReviewVerdictParseResult / ParsedReport /
ParseDeferred / expected_head_sha with zero consumers outside their declaring
module. Checked on the Rust side too: no producer there either.

So the Review stage is a complete fail-closed judgment that nothing in production
can ask a question of -- the same state Publication was in until its observer
landed, and the same sentence roadmap_publish's own note used about itself.

This is the PURE half, following that precedent exactly: it takes reports a
caller already holds and decides everything from them, so every arm is reachable
in a hermetic witness with no dashboard, no network and no token. The transport
half -- fetching artifacts and parsing bodies into verdicts -- is deliberately
not here; it rides the process-observation substrate and is a separate lane.

THE TALLY IS WRAPPED, NOT EXTENDED. Excluded stale reviews must be counted, and
MergeReadinessTally has no field for them. Extending it would edit a carrier
whose consumer is a live twelve-arm adjudicator; wrapping it does not.
ReviewTallyObservation carries the tally beside the exclusions and
decide_code_change_transition keeps taking exactly what it takes today. That is
the shape RemoteBranchObservation already uses one module over.

Three decisions worth naming, each with a claim:

Approvals count DISTINCT REVIEWERS, not artifacts. One provider re-reviewing the
same head produces two approving artifacts; counting artifacts would let a single
reviewer satisfy a two-approval bar by reviewing twice -- the bar defeated
arithmetically rather than by any decision.

A stale review is excluded AND named, never dropped. It was read fine, it is just
about another commit. Dropping it would zero the exclusion's frequency by
construction so nobody learns reviews are going stale faster than they land.

A report about a different PR REFUSES rather than being filtered. A stale review
is evidence about this PR at another commit; a foreign report is not evidence
about this PR at all, and silently dropping it hands back a confident tally built
from whatever happened to match.

12 claims green by execution. The RED control is proven rather than asserted:
replacing the distinct-reviewer count with an artifact count fails exactly one
claim -- one_reviewer_approving_twice_counts_once -- and leaves the other 11
green.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gunbai-bot gunbai-bot Bot changed the title Roadmap Give the Review stage the producer it never had Aug 1, 2026
@gunbai-bot
gunbai-bot Bot marked this pull request as ready for review August 1, 2026 02:57
@gunbai-bot

gunbai-bot Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Closing per portfolio verdict (operator, 2026-08-01). This is the wrong Review phase and I accept that.

The ratified Review stage is trigger a review worker -> exact pushed head -> typed findings -> feedback to the implementation worker -> bounded re-review loop. This PR built passive tallying over ReviewVerdictReport values that no roadmap review worker produces. Building a producer for artifacts nothing generates is the same accumulation pattern the verdict names.

Two concrete defects, both verified rather than conceded on assertion:

  1. decide_code_change_transition never reads approval_count — grep returns nothing. So the field this PR worked hardest on, with its discriminating RED control, is consumed by nothing. I spent today deleting write-only fields from other modules and then wrote one.
  2. The note says two reviewers finding the same defect is one defect, but blocking_finding_count_of sums every actionable finding from every report with no finding identity or dedup. Prose and code contradict.

Not merging. The first Review PR should prove one actual round, not tally artifacts.

— sent from proud-swift-104

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants