Repository navigation
fix(review): post findings when the rollup conflicts with the review policy - #707
Merged
Merged
Conversation
…policy The rollup prompt did not state the review policy, so the model proposed request_changes for a major finding under major_event: comment and the decoder rejected it. A response that also omitted schema_version reported only the version on the first attempt, so the retry failed on the event with no attempts left and the run posted nothing. The prompt now states the policy. The decoder accepts an absent schema_version and no longer judges the event; reviewplan decides the posted event from severities and policy, and a major finding never approves. Rollup output that stays invalid after the retry falls back to posting every finding without deduplication. Twapp-Session: 59d912f3-982f-402e-8bd8-3e31269a28b0
piekstra-dev
previously approved these changes
Nov 3, 2026
piekstra-dev
left a comment
piekstra-dev
left a comment
Collaborator
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 968c5a72e6dc
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| documentation:drift | 0 |
| privacy:captured-data | 0 |
| go:implementation-tests | 1 |
| documentation:docs | 0 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
| go:implementation | 0 |
| design:interface | 0 |
go:implementation-tests (1 finding)
Minor - internal/reviewplan/effective_event_test.go:6
reviewplan.buildReview now owns validation of the proposed rollup event: a non-empty proposal that is not valid returns 'reviewplan: invalid rollup review event'. Before this PR the check ran on the resolved event, and the decoder also rejected unknown values. A grep of internal/ finds no test that reaches this error. The rewritten TestEffectiveReviewEvent only calls effectiveReviewEvent, which never validates. If the guard is dropped, an unknown proposal such as 'merge' passes through silently, because effectiveReviewEvent ignores anything but comment. Add a small Build-level test in internal/reviewplan, for example with summaryRequest(), that sets Rollup.ReviewEvent to an unknown value and asserts the error, plus an empty proposal that builds.
Reviewer Coverage
Complete coverage details are retained in the local coverage.json artifact.
documentation:drift— complete (constrained); scope: 5 files; examples:internal/llm/contracts.go,internal/pipeline/pipeline.go,internal/pipeline/prompts.go,internal/review/review.go,internal/reviewplan/reviewplan.go; body-inspected: 5 files; examples:internal/llm/contracts.go,internal/pipeline/pipeline.go,internal/pipeline/prompts.go,internal/review/review.go,internal/reviewplan/reviewplan.go; skipped: none; constraints: Docs comparison was by grep of markdown files for review_event, major_event, rollup, schema_version and failed_blocking terms; README.md and docs/llm-task-artifacts.md were already updated in this PR and match the code.privacy:captured-data— complete (constrained); scope: 14 files; examples:README.md,docs/llm-task-artifacts.md,internal/llm/contracts.go,internal/llm/contracts_test.go,internal/pipeline/pipeline.go; 9 omitted; body-inspected: 14 files; examples:README.md,docs/llm-task-artifacts.md,internal/llm/contracts.go,internal/llm/contracts_test.go,internal/pipeline/pipeline.go; 9 omitted; skipped: none; constraints: Review relied on commit message, PR intent, and a pattern scan of added lines (long digit runs, emails, currency, cent-precise figures, provenance words) over the diff; no hits found. Files were not each read line by line.go:implementation-tests— complete (broad); scope: 12 files; examples:internal/llm/contracts.go,internal/llm/contracts_test.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go,internal/pipeline/prompts.go; 7 omitted; body-inspected: 12 files; examples:internal/llm/contracts.go,internal/llm/contracts_test.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go,internal/pipeline/prompts.go; 7 omitted; skipped: none; constraints: Reviewed the non-test source diffs in full. Test files were read mainly for pipeline_test.go; the other test files were checked via the diff stat and the cases the PR describes. Tests were not executed. Tests were not executed; review is static, from the diff and a grep of the head checkout.documentation:docs— complete (broad); scope: 2 files; examples:README.md,docs/llm-task-artifacts.md; body-inspected: 2 files; examples:README.md,docs/llm-task-artifacts.md; skipped: none; constraints: Checked the fallback rollup claims only against internal/pipeline/pipeline.go and internal/reviewplan/reviewplan.go by grep; did not trace every code path for the event table.policies:conventions— complete (broad); scope: 2 files; examples:README.md,docs/llm-task-artifacts.md; body-inspected: 2 files; examples:README.md,docs/llm-task-artifacts.md; skipped: none; constraints: Reviewed only the README.md and docs/llm-task-artifacts.md diffs; code behavior was not verified against the docs.structure:repo-health— complete (broad); scope: 4 files; examples:internal/llm/contracts.go,internal/pipeline/pipeline.go,internal/pipeline/prompts.go,internal/reviewplan/reviewplan.go; body-inspected: 4 files; examples:internal/llm/contracts.go,internal/pipeline/pipeline.go,internal/pipeline/prompts.go,internal/reviewplan/reviewplan.go; skipped: none; constraints: Reviewed the diff of the four assigned files plus the adjacent tests and docs in the diff stat; did not run the test suite.go:implementation— complete (broad); scope: 5 files; examples:internal/llm/contracts.go,internal/pipeline/pipeline.go,internal/pipeline/prompts.go,internal/review/review.go,internal/reviewplan/reviewplan.go; body-inspected: 5 files; examples:internal/llm/contracts.go,internal/pipeline/pipeline.go,internal/pipeline/prompts.go,internal/review/review.go,internal/reviewplan/reviewplan.go; skipped: none; constraints: Reviewed the production diffs only; test files were not read in detail and nothing was executed.design:interface— complete (broad); scope: 5 files; examples:docs/llm-task-artifacts.md,internal/llm/contracts.go,internal/pipeline/pipeline.go,internal/pipeline/prompts.go,internal/reviewplan/reviewplan.go; body-inspected: 5 files; examples:docs/llm-task-artifacts.md,internal/llm/contracts.go,internal/pipeline/pipeline.go,internal/pipeline/prompts.go,internal/reviewplan/reviewplan.go; skipped: none; constraints: Reviewed the diff only; did not run tests or a live review. Consumer-visible surfaces judged: rollup prompt contract, DecodeRollup acceptance, posted review event, review body note, and docs.
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 1m 35s | $1.10 | claude-sonnet-5-5 | cr 0.10.363
| Field | Value |
|---|---|
| Model | claude-sonnet-5-5 |
| Reviewers | documentation:drift, privacy:captured-data, go:implementation-tests, documentation:docs, policies:conventions, structure:repo-health, go:implementation, design:interface |
| Engine | claude_cli · claude-sonnet-5-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 1m 35s wall · 3m 35s compute |
| Cost | $1.10 |
| Tokens | 106 in / 16.1k out |
Per-workstream usage
orchestrator-selection— claude-sonnet-5-5- In: 6
- Out: 2.2k
- Cache read: 20.8k
- Cache create: 13.8k
- Cost: $0.08
- Duration: 21s
documentation:drift— claude-sonnet-5-5- In: 12
- Out: 1.8k
- Cache read: 85.6k
- Cache create: 20.0k
- Cost: $0.11
- Duration: 27s
privacy:captured-data— claude-sonnet-5-5- In: 8
- Out: 1.3k
- Cache read: 38.4k
- Cache create: 11.5k
- Cost: $0.06
- Duration: 15s
go:implementation-tests— claude-sonnet-5-5- In: 24
- Out: 4.2k
- Cache read: 284.8k
- Cache create: 41.2k
- Cost: $0.36
- Duration: 56s
documentation:docs— claude-sonnet-5-5- In: 12
- Out: 1.5k
- Cache read: 63.8k
- Cache create: 11.0k
- Cost: $0.07
- Duration: 20s
policies:conventions— claude-sonnet-5-5- In: 8
- Out: 956
- Cache read: 33.0k
- Cache create: 9.1k
- Cost: $0.05
- Duration: 12s
structure:repo-health— claude-sonnet-5-5- In: 10
- Out: 1.3k
- Cache read: 67.0k
- Cache create: 21.0k
- Cost: $0.10
- Duration: 23s
go:implementation— claude-sonnet-5-5- In: 10
- Out: 1.3k
- Cache read: 61.0k
- Cache create: 16.6k
- Cost: $0.09
- Duration: 16s
design:interface— claude-sonnet-5-5- In: 10
- Out: 1.3k
- Cache read: 62.0k
- Cache create: 17.0k
- Cost: $0.09
- Duration: 15s
orchestrator-rollup— claude-sonnet-5-5- In: 6
- Out: 336
- Cache read: 53.0k
- Cache create: 3.3k
- Cost: $0.10
- Duration: 5s
Twapp-Session: 59d912f3-982f-402e-8bd8-3e31269a28b0
piekstra-dev
approved these changes
Nov 3, 2026
piekstra-dev
left a comment
piekstra-dev
left a comment
Collaborator
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: e075c50208d4
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| documentation:drift | 0 |
| privacy:captured-data | 0 |
| go:implementation-tests | 0 |
| documentation:docs | 0 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
| go:implementation | 0 |
| design:interface | 0 |
Reviewer Coverage
Complete coverage details are retained in the local coverage.json artifact.
documentation:drift— complete (constrained); scope: 5 files; examples:internal/llm/contracts.go,internal/pipeline/pipeline.go,internal/pipeline/prompts.go,internal/review/review.go,internal/reviewplan/reviewplan.go; body-inspected: 5 files; examples:internal/llm/contracts.go,internal/pipeline/pipeline.go,internal/pipeline/prompts.go,internal/review/review.go,internal/reviewplan/reviewplan.go; skipped: none; constraints: Only a test file changed since the previously reviewed head; non-test source diff and README.md and docs/llm-task-artifacts.md were compared by grep and match the code.privacy:captured-data— complete (constrained); scope: 14 files; examples:README.md,docs/llm-task-artifacts.md,internal/llm/contracts.go,internal/llm/contracts_test.go,internal/pipeline/pipeline.go; 9 omitted; body-inspected: 14 files; examples:README.md,docs/llm-task-artifacts.md,internal/llm/contracts.go,internal/llm/contracts_test.go,internal/pipeline/pipeline.go; 9 omitted; skipped: none; constraints: Review relied on commit messages, PR intent, and a pattern scan of added lines (long digit runs, emails, currency, cent-precise figures, provenance words) over the diff; no hits found. Files were not each read line by line.go:implementation-tests— complete (constrained); scope: 12 files; examples:internal/llm/contracts.go,internal/llm/contracts_test.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go,internal/pipeline/prompts.go; 7 omitted; body-inspected: 12 files; examples:internal/llm/contracts.go,internal/llm/contracts_test.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go,internal/pipeline/prompts.go; 7 omitted; skipped: none; constraints: Tests were not executed; review is static. The only change since the earlier reviewed head (968c5a7) is the new TestBuildValidatesProposedRollupEvent in internal/reviewplan/effective_event_test.go; all other files were read at the earlier head and are unchanged.documentation:docs— complete (constrained); scope: 2 files; examples:README.md,docs/llm-task-artifacts.md; body-inspected: 2 files; examples:README.md,docs/llm-task-artifacts.md; skipped: none; constraints: Docs diff is identical to the previously reviewed head; fallback behavior was spot-checked against pipeline and reviewplan code by grep only.policies:conventions— complete (constrained); scope: 2 files; examples:README.md,docs/llm-task-artifacts.md; body-inspected: 2 files; examples:README.md,docs/llm-task-artifacts.md; skipped: none; constraints: Reviewed only the README.md and docs/llm-task-artifacts.md diffs; code behavior was not verified against the docs.structure:repo-health— complete (constrained); scope: 4 files; examples:internal/llm/contracts.go,internal/pipeline/pipeline.go,internal/pipeline/prompts.go,internal/reviewplan/reviewplan.go; body-inspected: 4 files; examples:internal/llm/contracts.go,internal/pipeline/pipeline.go,internal/pipeline/prompts.go,internal/reviewplan/reviewplan.go; skipped: none; constraints: Manifest line counts for the four assigned files match the previously reviewed head, so I relied on that earlier read of their bodies; the new head adds a test for proposed-event validation. I did not run the test suite.go:implementation— complete (constrained); scope: 5 files; examples:internal/llm/contracts.go,internal/pipeline/pipeline.go,internal/pipeline/prompts.go,internal/review/review.go,internal/reviewplan/reviewplan.go; body-inspected: 5 files; examples:internal/llm/contracts.go,internal/pipeline/pipeline.go,internal/pipeline/prompts.go,internal/review/review.go,internal/reviewplan/reviewplan.go; skipped: none; constraints: Production code is unchanged from the previously reviewed head; the only delta is an added reviewplan test. Nothing was executed.design:interface— complete (constrained); scope: 5 files; examples:docs/llm-task-artifacts.md,internal/llm/contracts.go,internal/pipeline/pipeline.go,internal/pipeline/prompts.go,internal/reviewplan/reviewplan.go; body-inspected: 5 files; examples:docs/llm-task-artifacts.md,internal/llm/contracts.go,internal/pipeline/pipeline.go,internal/pipeline/prompts.go,internal/reviewplan/reviewplan.go; skipped: none; constraints: Reviewed the diff only; did not run tests or a live review. Consumer-visible surfaces judged: rollup prompt contract, DecodeRollup acceptance, posted review event, review body note, and docs.
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 56s | $1.40 | claude-sonnet-5-5 | cr 0.10.363
| Field | Value |
|---|---|
| Model | claude-sonnet-5-5 |
| Reviewers | documentation:drift, privacy:captured-data, go:implementation-tests, documentation:docs, policies:conventions, structure:repo-health, go:implementation, design:interface |
| Engine | claude_cli · claude-sonnet-5-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 56s wall · 2m 08s compute |
| Cost | $1.40 |
| Tokens | 68 in / 8.5k out |
Per-workstream usage
documentation:drift— claude-sonnet-5-5- In: 8
- Out: 1.1k
- Cache read: 114.9k
- Cache create: 9.5k
- Cost: $0.17
- Duration: 21s
privacy:captured-data— claude-sonnet-5-5- In: 8
- Out: 1.2k
- Cache read: 84.2k
- Cache create: 11.5k
- Cost: $0.13
- Duration: 13s
go:implementation-tests— claude-sonnet-5-5- In: 8
- Out: 1.4k
- Cache read: 200.1k
- Cache create: 10.2k
- Cost: $0.31
- Duration: 16s
documentation:docs— claude-sonnet-5-5- In: 8
- Out: 954
- Cache read: 78.7k
- Cache create: 9.6k
- Cost: $0.12
- Duration: 14s
policies:conventions— claude-sonnet-5-5- In: 8
- Out: 861
- Cache read: 69.0k
- Cache create: 8.9k
- Cost: $0.10
- Duration: 11s
structure:repo-health— claude-sonnet-5-5- In: 6
- Out: 817
- Cache read: 83.8k
- Cache create: 8.0k
- Cost: $0.15
- Duration: 12s
go:implementation— claude-sonnet-5-5- In: 8
- Out: 892
- Cache read: 100.8k
- Cache create: 9.3k
- Cost: $0.14
- Duration: 17s
design:interface— claude-sonnet-5-5- In: 8
- Out: 973
- Cache read: 106.1k
- Cache create: 12.8k
- Cost: $0.16
- Duration: 14s
orchestrator-rollup— claude-sonnet-5-5- In: 6
- Out: 313
- Cache read: 62.6k
- Cache create: 2.9k
- Cost: $0.12
- Duration: 6s
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A run whose reviewers returned a major finding under
review_policy.major_event: commentcould fail withstructured output invalid after retryand post nothing, so the findings never reached the pull request.Two independent causes combined:
approve, comment, or request_changeswith no rule, so the model proposedrequest_changesfor a major finding, and the decoder rejected that.schema_version, although the prompt asks for it three times. The decoder reports one error per attempt, so the first attempt reported only the version, the retry fixed the version, and the retry then failed on the event with no attempts left.Change
major_eventpolicy.DecodeRollupschema_versionis accepted, as thread analysis already does. An explicit wrong version is still rejected. The event must still be a known value, but the decoder no longer judges it against the policy.reviewplanfailed_blockingwith both attempts recorded. Provider failures stay blocking and resumable.Posted event for each proposal:
request_changescommentcommentrequest_changesrequest_changes,approve, or nonerequest_changesrequest_changescommentcommentapprove, orcommentwhen self-approval is off or a reviewer failedA major finding never approves.
The fallback leaves two existing tests with a new trigger: they exercised resume by feeding invalid rollup output, and now use a provider failure, which is what stays blocking.
Verification
make tidy fmt lint buildgo test -tags=keyring_nopassage -count=1 ./...TestPiRPCReviewerImageProcessingBounds, a parent and itsoutputsubtest under full-suite load. The package passes alone on this branch, and on cleanmain.Not verified: a live review of a pull request with a real major finding through this binary. The tests drive the pipeline with a scripted adapter.