Publish review readiness ratings #4

lambadalambda · 2026-08-05T13:19:11Z

lambadalambda commented

2026-08-05 13:19:11 +00:00

Summary

add a strict protocol-v2 merge-readiness rating from 0/5 through 5/5 to review reports and render it in compact, full, and fallback output
keep the summary target near 500 bytes while raising the hard validation and persistence limit to 2000 bytes
publish new pull reviews atomically with inline comments so Forgejo renders one timeline review, while retaining bounded recovery for persisted pending and ambiguous outcomes
preserve complete summaries in fallback publication and report bounded validation reasons from the latest report-shaped JSON candidate

Verification

go test ./...
go test -race ./...
go vet ./...
sh scripts/adversarial-check.sh
independent code review with no remaining findings

Deployment

Review protocol v2 requires an atomic dispatcher and worker/launcher rollout. There were no running or publishing jobs when this branch was created.

## Summary - add a strict protocol-v2 merge-readiness rating from 0/5 through 5/5 to review reports and render it in compact, full, and fallback output - keep the summary target near 500 bytes while raising the hard validation and persistence limit to 2000 bytes - publish new pull reviews atomically with inline comments so Forgejo renders one timeline review, while retaining bounded recovery for persisted pending and ambiguous outcomes - preserve complete summaries in fallback publication and report bounded validation reasons from the latest report-shaped JSON candidate ## Verification - `go test ./...` - `go test -race ./...` - `go vet ./...` - `sh scripts/adversarial-check.sh` - independent code review with no remaining findings ## Deployment Review protocol v2 requires an atomic dispatcher and worker/launcher rollout. There were no running or publishing jobs when this branch was created.

lambadalambda added 1 commit

2026-08-05 13:19:11 +00:00

feat: publish review readiness ratings 71cf62a0db

lambadalambda added 1 commit

2026-08-05 17:55:11 +00:00

fix: publish pull reviews atomically

opencode/review OpenCode execution completed

Details

bc901866b2

lambadalambda commented

2026-08-05 17:55:52 +00:00

/oc review

opencode-bot reviewed

2026-08-05 18:06:52 +00:00

opencode-bot left a comment

OpenCode Review

Publishes a required integer merge-readiness rating (0-5) for review jobs and makes inline review publication atomic. Bumps ReviewReportVersion to 2; review reports now require rating while summary reports reject it. Validation, JSON strict-field checks, omitempty/zero-rating pointer handling, prompt text (launcher + review.json), README, and the SQLite summary trim limit (500->2000, target ~500) are all updated consistently. The headline behavior change switches new pull reviews to a single CreatePullReview with event=COMMENT and inline comments, eliminating the staged PENDING-then-submit window. Definitive 4xx rejections fall back to the canonical comment; ambiguous (5xx/network) outcomes are polled for a bounded 30s and, if still unknowable, fail terminally without publishing a potentially duplicate fallback. Interrupted PENDING reviews from the prior design are still completed for backward compatibility. Test coverage is comprehensive (atomic create, pending recovery, definitive rejection, ambiguous reconciliation, zero rating, summary above target).

Rating

5/5

2 finding(s) are attached to changed lines.

Overall

Correct, secure, and well-engineered. The atomic-publish refactor carefully preserves the no-duplicate guarantee: POST is single-attempt, ambiguous outcomes are recovered by the unique publication marker and only published after per-comment verification, while terminal ambiguity refuses to emit any visible output. Rating validation is sound across decode and normalize paths (null, fractional, missing, out-of-range, zero all handled), and pointer-with-omitempty correctly serializes rating:0. The 2000-byte summary ceiling is enforced identically in control, store, and prompt. Backward compatibility for older staged-PENDING reviews is retained and tested. All targeted and full-suite tests pass; gofmt and go vet are clean. The two findings below are low-severity, optional hardening/efficiency notes on newly added code rather than merge blockers; I would merge as-is.

## OpenCode Review Publishes a required integer merge-readiness rating (0-5) for review jobs and makes inline review publication atomic. Bumps ReviewReportVersion to 2; review reports now require rating while summary reports reject it. Validation, JSON strict-field checks, omitempty/zero-rating pointer handling, prompt text (launcher + review.json), README, and the SQLite summary trim limit (500->2000, target ~500) are all updated consistently. The headline behavior change switches new pull reviews to a single CreatePullReview with event=COMMENT and inline comments, eliminating the staged PENDING-then-submit window. Definitive 4xx rejections fall back to the canonical comment; ambiguous (5xx/network) outcomes are polled for a bounded 30s and, if still unknowable, fail terminally without publishing a potentially duplicate fallback. Interrupted PENDING reviews from the prior design are still completed for backward compatibility. Test coverage is comprehensive (atomic create, pending recovery, definitive rejection, ambiguous reconciliation, zero rating, summary above target). ## Rating **5/5** _2 finding(s) are attached to changed lines._ ## Overall Correct, secure, and well-engineered. The atomic-publish refactor carefully preserves the no-duplicate guarantee: POST is single-attempt, ambiguous outcomes are recovered by the unique publication marker and only published after per-comment verification, while terminal ambiguity refuses to emit any visible output. Rating validation is sound across decode and normalize paths (null, fractional, missing, out-of-range, zero all handled), and pointer-with-omitempty correctly serializes rating:0. The 2000-byte summary ceiling is enforced identically in control, store, and prompt. Backward compatibility for older staged-PENDING reviews is retained and tested. All targeted and full-suite tests pass; gofmt and go vet are clean. The two findings below are low-severity, optional hardening/efficiency notes on newly added code rather than merge blockers; I would merge as-is.

internal/app/reviews.go

					
				@ -62,2 +78,3 @@

							}

							reconcileCtx, cancel := context.WithTimeout(context.Background(), compensationTimeout)

							review, err = a.reconcilePullReview(reconcileCtx, job, marker)

							review, err = a.reconcilePullReviewUntilFound(reconcileCtx, job, marker)

[LOW] Ambiguous reconciliation holds the publication lock for up to 30s

On the rare ambiguous-create path (network/5xx error on CreatePullReview), publishPullReview now calls reconcilePullReviewUntilFound, which polls ListPullReviews with exponential backoff for the full compensationTimeout (30s). This runs while handleResult holds the global publication lock (a.lockPublication at app.go:351), so a single ambiguous publication serially blocks every other result publication for up to 30s. The bound is firm and the path is uncommon, but for repositories with many reviews each poll is a paginated list (up to 20 pages), amplifying lock hold time and API load on the error path.

Suggested replacement (manual):

Consider polling a cheaper signal first (e.g., a single GetPullReview once a candidate ID is learned) or accepting a shorter compensation window for the lock-held phase while moving extended polling to the lease-retry path.

**[LOW] Ambiguous reconciliation holds the publication lock for up to 30s** On the rare ambiguous-create path (network/5xx error on CreatePullReview), publishPullReview now calls reconcilePullReviewUntilFound, which polls ListPullReviews with exponential backoff for the full compensationTimeout (30s). This runs while handleResult holds the global publication lock (a.lockPublication at app.go:351), so a single ambiguous publication serially blocks every other result publication for up to 30s. The bound is firm and the path is uncommon, but for repositories with many reviews each poll is a paginated list (up to 20 pages), amplifying lock hold time and API load on the error path. **Suggested replacement (manual):** Consider polling a cheaper signal first (e.g., a single GetPullReview once a candidate ID is learned) or accepting a shorter compensation window for the lock-held phase while moving extended polling to the lease-retry path.

internal/app/reviews.go

					
				@ -77,0 +91,4 @@

						if err := a.store.SavePullReviewID(ctx, job.ID, job.LeaseToken, job.HeadSHA, review.ID); err != nil {

							return nil, a.failStagedPullReview(ctx, job, review.ID, err)

						}

						if review.CommentsCount != len(comments) {

[LOW] Happy-path atomic create verifies comment count but not comment bodies

When CreatePullReview returns a clean 2xx, the code validates state, persists the ID, and checks review.CommentsCount == len(comments), then returns. It only re-lists and matches each expected comment body in the ambiguous branch (createErr != nil). Relying on the count is reasonable for a successful response, but it means a server that reported the expected count while storing different/rewritten bodies would not be caught on the happy path, even though the ambiguous path explicitly defends against this. Impact is theoretical since Forgejo stores comment bodies verbatim.

Suggested replacement (manual):

For consistency with the ambiguous-path integrity check, optionally re-list and verify each inline body on the success path too, or document the trust assumption explicitly.

**[LOW] Happy-path atomic create verifies comment count but not comment bodies** When CreatePullReview returns a clean 2xx, the code validates state, persists the ID, and checks review.CommentsCount == len(comments), then returns. It only re-lists and matches each expected comment body in the ambiguous branch (createErr != nil). Relying on the count is reasonable for a successful response, but it means a server that reported the expected count while storing different/rewritten bodies would not be caught on the happy path, even though the ambiguous path explicitly defends against this. Impact is theoretical since Forgejo stores comment bodies verbatim. **Suggested replacement (manual):** For consistency with the ambiguous-path integrity check, optionally re-list and verify each inline body on the success path too, or document the trust assumption explicitly.

opencode-bot reviewed

2026-08-05 18:06:53 +00:00

opencode-bot left a comment

OpenCode Review

Publishes a required integer merge-readiness rating (0-5) for review jobs and makes inline review publication atomic. Bumps ReviewReportVersion to 2; review reports now require rating while summary reports reject it. Validation, JSON strict-field checks, omitempty/zero-rating pointer handling, prompt text (launcher + review.json), README, and the SQLite summary trim limit (500->2000, target ~500) are all updated consistently. The headline behavior change switches new pull reviews to a single CreatePullReview with event=COMMENT and inline comments, eliminating the staged PENDING-then-submit window. Definitive 4xx rejections fall back to the canonical comment; ambiguous (5xx/network) outcomes are polled for a bounded 30s and, if still unknowable, fail terminally without publishing a potentially duplicate fallback. Interrupted PENDING reviews from the prior design are still completed for backward compatibility. Test coverage is comprehensive (atomic create, pending recovery, definitive rejection, ambiguous reconciliation, zero rating, summary above target).

Rating

5/5

2 finding(s) are attached to changed lines.

Overall

Correct, secure, and well-engineered. The atomic-publish refactor carefully preserves the no-duplicate guarantee: POST is single-attempt, ambiguous outcomes are recovered by the unique publication marker and only published after per-comment verification, while terminal ambiguity refuses to emit any visible output. Rating validation is sound across decode and normalize paths (null, fractional, missing, out-of-range, zero all handled), and pointer-with-omitempty correctly serializes rating:0. The 2000-byte summary ceiling is enforced identically in control, store, and prompt. Backward compatibility for older staged-PENDING reviews is retained and tested. All targeted and full-suite tests pass; gofmt and go vet are clean. The two findings below are low-severity, optional hardening/efficiency notes on newly added code rather than merge blockers; I would merge as-is.

## OpenCode Review Publishes a required integer merge-readiness rating (0-5) for review jobs and makes inline review publication atomic. Bumps ReviewReportVersion to 2; review reports now require rating while summary reports reject it. Validation, JSON strict-field checks, omitempty/zero-rating pointer handling, prompt text (launcher + review.json), README, and the SQLite summary trim limit (500->2000, target ~500) are all updated consistently. The headline behavior change switches new pull reviews to a single CreatePullReview with event=COMMENT and inline comments, eliminating the staged PENDING-then-submit window. Definitive 4xx rejections fall back to the canonical comment; ambiguous (5xx/network) outcomes are polled for a bounded 30s and, if still unknowable, fail terminally without publishing a potentially duplicate fallback. Interrupted PENDING reviews from the prior design are still completed for backward compatibility. Test coverage is comprehensive (atomic create, pending recovery, definitive rejection, ambiguous reconciliation, zero rating, summary above target). ## Rating **5/5** _2 finding(s) are attached to changed lines._ ## Overall Correct, secure, and well-engineered. The atomic-publish refactor carefully preserves the no-duplicate guarantee: POST is single-attempt, ambiguous outcomes are recovered by the unique publication marker and only published after per-comment verification, while terminal ambiguity refuses to emit any visible output. Rating validation is sound across decode and normalize paths (null, fractional, missing, out-of-range, zero all handled), and pointer-with-omitempty correctly serializes rating:0. The 2000-byte summary ceiling is enforced identically in control, store, and prompt. Backward compatibility for older staged-PENDING reviews is retained and tested. All targeted and full-suite tests pass; gofmt and go vet are clean. The two findings below are low-severity, optional hardening/efficiency notes on newly added code rather than merge blockers; I would merge as-is.

internal/app/reviews.go

					
				@ -62,2 +78,3 @@

							}

							reconcileCtx, cancel := context.WithTimeout(context.Background(), compensationTimeout)

							review, err = a.reconcilePullReview(reconcileCtx, job, marker)

							review, err = a.reconcilePullReviewUntilFound(reconcileCtx, job, marker)

[LOW] Ambiguous reconciliation holds the publication lock for up to 30s

On the rare ambiguous-create path (network/5xx error on CreatePullReview), publishPullReview now calls reconcilePullReviewUntilFound, which polls ListPullReviews with exponential backoff for the full compensationTimeout (30s). This runs while handleResult holds the global publication lock (a.lockPublication at app.go:351), so a single ambiguous publication serially blocks every other result publication for up to 30s. The bound is firm and the path is uncommon, but for repositories with many reviews each poll is a paginated list (up to 20 pages), amplifying lock hold time and API load on the error path.

Suggested replacement (manual):

Consider polling a cheaper signal first (e.g., a single GetPullReview once a candidate ID is learned) or accepting a shorter compensation window for the lock-held phase while moving extended polling to the lease-retry path.

**[LOW] Ambiguous reconciliation holds the publication lock for up to 30s** On the rare ambiguous-create path (network/5xx error on CreatePullReview), publishPullReview now calls reconcilePullReviewUntilFound, which polls ListPullReviews with exponential backoff for the full compensationTimeout (30s). This runs while handleResult holds the global publication lock (a.lockPublication at app.go:351), so a single ambiguous publication serially blocks every other result publication for up to 30s. The bound is firm and the path is uncommon, but for repositories with many reviews each poll is a paginated list (up to 20 pages), amplifying lock hold time and API load on the error path. **Suggested replacement (manual):** Consider polling a cheaper signal first (e.g., a single GetPullReview once a candidate ID is learned) or accepting a shorter compensation window for the lock-held phase while moving extended polling to the lease-retry path.

internal/app/reviews.go

					
				@ -77,0 +91,4 @@

						if err := a.store.SavePullReviewID(ctx, job.ID, job.LeaseToken, job.HeadSHA, review.ID); err != nil {

							return nil, a.failStagedPullReview(ctx, job, review.ID, err)

						}

						if review.CommentsCount != len(comments) {

[LOW] Happy-path atomic create verifies comment count but not comment bodies

When CreatePullReview returns a clean 2xx, the code validates state, persists the ID, and checks review.CommentsCount == len(comments), then returns. It only re-lists and matches each expected comment body in the ambiguous branch (createErr != nil). Relying on the count is reasonable for a successful response, but it means a server that reported the expected count while storing different/rewritten bodies would not be caught on the happy path, even though the ambiguous path explicitly defends against this. Impact is theoretical since Forgejo stores comment bodies verbatim.

Suggested replacement (manual):

For consistency with the ambiguous-path integrity check, optionally re-list and verify each inline body on the success path too, or document the trust assumption explicitly.

**[LOW] Happy-path atomic create verifies comment count but not comment bodies** When CreatePullReview returns a clean 2xx, the code validates state, persists the ID, and checks review.CommentsCount == len(comments), then returns. It only re-lists and matches each expected comment body in the ambiguous branch (createErr != nil). Relying on the count is reasonable for a successful response, but it means a server that reported the expected count while storing different/rewritten bodies would not be caught on the happy path, even though the ambiguous path explicitly defends against this. Impact is theoretical since Forgejo stores comment bodies verbatim. **Suggested replacement (manual):** For consistency with the ambiguous-path integrity check, optionally re-list and verify each inline body on the success path too, or document the trust assumption explicitly.

opencode-bot commented

2026-08-05 18:06:53 +00:00

OpenCode Review

Execution completed for lambadalambda/opencode-forgejo-bot at bc901866b217 (job #32).

Rating: 5/5

Publishes a required integer merge-readiness rating (0-5) for review jobs and makes inline review publication atomic. Bumps ReviewReportVersion to 2; review reports now require rating while summary reports reject it. Validation, JSON strict-field checks, omitempty/zero-rating pointer handling, prompt text (launcher + review.json), README, and the SQLite summary trim limit (500->2000, target ~500) are all updated consistently. The headline behavior change switches new pull reviews to a single CreatePullReview with event=COMMENT and inline comments, eliminating the staged PENDING-then-submit window. Definitive 4xx rejections fall back to the canonical comment; ambiguous (5xx/network) outcomes are polled for a bounded 30s and, if still unknowable, fail terminally without publishing a potentially duplicate fallback. Interrupted PENDING reviews from the prior design are still completed for backward compatibility. Test coverage is comprehensive (atomic create, pending recovery, definitive rejection, ambiguous reconciliation, zero rating, summary above target).

View the pull review with 2 finding(s).

## OpenCode Review _Execution completed for `lambadalambda/opencode-forgejo-bot` at `bc901866b217` (job #32)._ Rating: **5/5** Publishes a required integer merge-readiness rating (0-5) for review jobs and makes inline review publication atomic. Bumps ReviewReportVersion to 2; review reports now require rating while summary reports reject it. Validation, JSON strict-field checks, omitempty/zero-rating pointer handling, prompt text (launcher + review.json), README, and the SQLite summary trim limit (500->2000, target ~500) are all updated consistently. The headline behavior change switches new pull reviews to a single CreatePullReview with event=COMMENT and inline comments, eliminating the staged PENDING-then-submit window. Definitive 4xx rejections fall back to the canonical comment; ambiguous (5xx/network) outcomes are polled for a bounded 30s and, if still unknowable, fail terminally without publishing a potentially duplicate fallback. Interrupted PENDING reviews from the prior design are still completed for backward compatibility. Test coverage is comprehensive (atomic create, pending recovery, definitive rejection, ambiguous reconciliation, zero rating, summary above target). [View the pull review with 2 finding(s)](https://git.pleroma.social/lambadalambda/opencode-forgejo-bot/pulls/4#issuecomment-117665).

lambadalambda merged commit 43a3a3c0e0 into main

2026-08-05 18:09:39 +00:00

lambadalambda referenced this pull request from a commit

2026-08-05 18:09:39 +00:00

Merge pull request 'Publish review readiness ratings' (#4) from feat/review-rating into main

Sign in to join this conversation.

No reviewers

No labels

No milestone

No project

No assignees

2 participants

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

lambadalambda/opencode-forgejo-bot!4

Rows
Columns