feat(a2a): stream review findings per-finding at finalize (ADR-0098, #457)

[stephane-segning]

1. Summary

This PR changes:

• A2A review findings now stream incrementally: at finalize_review, one artifactUpdate per confirmed finding (finding-{file}:{line}, ADR-0032 JSON), then a conclusion review artifact — all before the terminal COMPLETED status-update, which stays the sole stream freeze/close.
• Removes the terminal completed_artifact blob emit from append_transition_events (the terminal transition now carries only the status-update).
• Updates the caller-facing streaming contract in docs/a2a-review-skill.md §5 and records the decision in docs/adr/0098-a2a-per-finding-review-streaming.md.

It solves:

• [Ticket]: Stream A2A review findings per-finding at finalize (ADR-0098) #457 — "Stream A2A review findings per-finding at finalize (ADR-0098)".

---

2. Intent

The intent of this PR is:

Decouple how findings reach an A2A consumer from how they reach the PR. The A2A caller is the consumer that feels the "wait for the whole run, then often nothing" pain — today the stream delivers one end-of-run blob and, on the common posting path, closes empty (the reviews row lands async after succeeded), forcing a follow-up GetTask. Streaming per-finding lets a downstream consumer act on finding #1 without waiting for the rest, and carries the findings on every path. This is emit-only: confirmation timing (the end-of-run refute pass) and the batched PR posting are deliberately unchanged. Source of truth: #457, ADR-0098, refining ADR-0077 / RFC-0006.

---

3. Scope

In Scope

• New append_review_stream producer (services/control-plane/src/a2a/events.rs) with the ADR-0077 invariants preserved: gap-free seq, has_final freeze, FOR UPDATE row-lock serialization, and has_artifact_update idempotency for re-finalize.
• Pure finding_artifact / conclusion_artifact builders (mapping.rs) + unit tests.
• One best-effort call in finalize_review (http/internal.rs) after the summary is computed.
• Caller doc + ADR.

Out of Scope

• Posting findings to the PR incrementally — still one grouped review through the outbox.
• Per-finding confirmation (incremental refute) — confirmation stays end-of-run.
• Any change to the review agent loop or the OpenCode host (ADR-0097) — the hook is host-agnostic.

---

4. Verification

I verified this change by:

• Running automated tests
• Running manual tests
• Checking logs
• Checking metrics
• Testing error cases
• Testing permissions/security behavior
• Testing rollback or failure behavior, if relevant

Commands run:

$ cargo test -p control-plane
$ cargo test -p control-plane 'a2a::'
$ cargo check --workspace
$ cargo fmt --all -- --check
$ cargo clippy -p control-plane --all-targets

Results:

cargo test -p control-plane        → ok. 307 passed; 0 failed; 2 ignored
cargo test -p control-plane 'a2a::' → ok. 106 passed; 0 failed; 203 filtered out
cargo check --workspace            → Finished (clean)
cargo fmt --all -- --check         → clean
cargo clippy -p control-plane      → 0 warnings/errors

New tests: per-finding-then-conclusion ordering before the terminal event; idempotent re-finalize (has_artifact_update); non-A2A run no-op; freeze-after-terminal (has_final); updated terminal-replay test (no artifact on the transition); finding_artifact/conclusion_artifact builder unit tests including the streaming⟷polling parity assertion.

Rebased onto current main (b8d3223): the A2A files applied cleanly; internal.rs auto-merged with the Restate-egress removal (no conflict). ADR renumbered 0094→0098 after main claimed 0094–0097.

---

5. Screenshots / Evidence

• Test output: see §4 (fenced results above).
• Decision record: docs/adr/0098-a2a-per-finding-review-streaming.md.
• Caller contract: docs/a2a-review-skill.md §5 (updated stream ordering + permalink note).

---

6. Risk Assessment

Risk level:

• Low
• Medium
• High

Potential risks:

• Stream/poll divergence (a consumer assembling from the stream sees a different set than GetTask).
• Duplicate or late stream events on a re-finalize or after the terminal event.

Mitigation:

• Streamed set == persisted reviews.findings == GetTask — asserted by a parity unit test; data part is byte-identical serde_json::to_value(finding).
• has_artifact_update (idempotent re-finalize) + has_final (freeze) guards, both covered by tests; the append is best-effort so a failure never fails the finalize.

---

7. AI Usage Declaration

AI was used for:

• Understanding existing code
• Generating code
• Refactoring
• Generating tests
• Drafting documentation
• Reviewing the diff
• Not used

Human verification:

• I understand every meaningful change in this PR
• I checked generated code manually
• I checked generated tests manually
• I removed unsupported AI assumptions
• I accept responsibility for this PR

Accountable owner: @leghadjeu-christian (assigned). Code, tests, and docs were AI-generated in-session and the automated verification in §4 was run by the AI agent; the human review + accountability sign-off is completed by the assigned owner.

---

8. Reviewer Focus

Please focus your review on:

• Correctness
• Architecture
• Security
• Performance
• Tests
• Maintainability
• Product intent
• Edge cases

Specifically: the event ordering/freeze invariants in append_review_stream (does it correctly land before the terminal event and never after a freeze?), the streaming⟷polling parity claim, and whether the conclusion-carries-no-findings-blob shape is what A2A consumers want.

[changeset-bot]

⚠️ No Changeset found

Latest commit: db2af56

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[github-actions]

✅ AI Governance check passed

This PR declares AI usage, references a source of truth, and provides verification evidence. Thank you.

[gemini-code-assist]

Code Review

This pull request implements incremental A2A per-finding review streaming as detailed in ADR-0098. Instead of emitting a single large artifact blob at the terminal transition, findings are now streamed individually as they are confirmed during the finalize phase, followed by a conclusion artifact containing the summary and context. The terminal transition now only carries the status update. Corresponding updates have been made to the documentation, event emission logic, mapping helpers, and test suite. I have no feedback to provide as there are no review comments.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[stephane-segning]

@lightbridge-assistant review

[stephane-segning]

@lightbridge-assistant review

[stephane-segning]

@lightbridge-assistant review

[lightbridge-assistant]

Lightbridge review

The change correctly implements per-finding review streaming (ADR-0098): findings and the conclusion emit as artifact-update chunks from finalize_review while the task is still non-terminal, carrying a lower seq than the eventual COMPLETED status-update that closes the stream; polling (GetTask) still rebuilds the combined artifact from the reviews row. Verified load-bearing claims against the code: runner ordering (finalize→succeeded) ensures artifacts precede the terminal event; idempotency is transactionally sound (single-tx all-or-nothing, so has_artifact_update never sees a partial stream); freeze-safety holds via has_final + tasks ... FOR UPDATE. SAST found nothing; no P0/P1 defect reproducible from the diff. One non-blocking P2: the finalize_review→append_review_stream wiring (ReviewContext construction + call placement in internal.rs) is not exercised end-to-end — the four new tests in tests.rs call append_review_stream directly, so a regression there wouldn't be caught by a test. All changed source files reviewed (events.rs, mapping.rs, internal.rs, handler/tests.rs); the two docs (a2a-review-skill.md, adr/0098) carry low signal and were not opened. Prior reviews' "ready to merge" stands; nothing to retract (no prior findings were recorded).

ℹ️ 2 additional changed file(s) carry low review signal and aren't counted above: 2 config/docs.

A custom operator OpenCode config (review.opencode) was active and relaxed the review floor: built-in tool task re-enabled (coverage may go blind). Coverage/read-only guarantees may differ from the default.

---

🤖 AI-generated review — treat it as untrusted, verify before acting; a human owns the final decision (AI governance).

[lightbridge-assistant]

No integration test for finalize→stream wiring

The new append_review_stream invocation here (the ReviewContext constructed from context.owner/name/target_id/base_sha/head_sha, the call placed right after effective_summary) is the one piece of this change not exercised end-to-end: the four new tests in a2a/handler/tests.rs all call append_review_stream directly, so they validate the events.rs layer but not this wiring. A field-mapping regression at this call site (e.g. wrong pr/head_sha, or the call drifting below the buffer-clear so findings is empty) would not be caught. A single finalize_review-level test that seeds an A2A-fronted task, finalizes, and asserts the artifact-update rows appear with the right reviewUrl: null context would close the loop and mirror the repo's "write the regression test rather than trust the change" convention.

Evidence: The added block in internal.rs builds ReviewContext from context and calls append_review_stream(pool, id, &findings, &summary, &review_ctx); the new tests in a2a/handler/tests.rs (e.g. review_stream_emits_per_finding_then_conclusion_before_terminal) invoke crate::a2a::events::append_review_stream directly and never go through finalize_review, so this construction/placement is unverified.

Was this useful? React 👍/👎 to give us feedback