feat(scoring): commit successful completion with outbox atomically - #69
seonghobae wants to merge 15 commits into
Conversation
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head43845df9266024348bfebd54e2e98d277c3db070. -
Head SHA:
43845df9266024348bfebd54e2e98d277c3db070 -
Workflow run: 31851486961
-
Workflow attempt: 1
Coverage evidence
Coverage Decision
- Result: FAIL
- Test evidence: not proven passing
- Docstring evidence: not proven passing when configured
- Failure count: 1
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (2 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (2 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Test (2 files)"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test (2 files)"]
R2 --> V2["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage Decision
Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (2 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (2 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Test (2 files)"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test (2 files)"]
R2 --> V2["targeted test run"]
|
Cover InvalidCompletionEnvelope on Linux llvm-cov by constructing the variant and asserting its exact Display text and absent source, plus the exact Completion/Outbox wrapper messages.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Add real PostgreSQL cases for the crash-between-old-APIs path this composition exists to repair, and record the envelope's actual source/job/time gate so callers keep event identity stable. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
There was a problem hiding this comment.
Review (HEAD cda680c)
This head closes the crash window it claims: fenced successful completion and the bound outbox insert share one caller-owned READ COMMITTED transaction. Exact replay and both mixed legacy dispositions now have real PostgreSQL proof. A late outbox ConflictingReplay rolls the job back to leased. A missing job never reaches enqueue. Foreign source, subject, or completion time is rejected before any write.
OpenCode REQUEST_CHANGES on 43845df is stale coverage-evidence, not a current-head defect.
Do not enlarge this PR
- Do not bind
ResultSnapshotor invent an envelope field forscoring_result_ref. The job row already storesresult_ref. Result publication stays a later worker/schema slice (UML persists that snapshot after narrative provenance). - Do not add a
completed_atmigration here. Duplicate completion still cannot verify historical time; that is pre-existing onscoring_job_state.
Land this head when
- Runtime CI, exact statement/branch coverage, docs, and security checks are terminal-success on this exact SHA.
- An independent non-author review approves this exact SHA.
Next product slice after merge
Wire the scoring worker to this helper with a stable event_ref (job + result identity). A retry that mints a new event_ref after an accepted completion will insert a second outbox row. Permanent failure still has no atomic outbox composition and is not exact-replay idempotent; that is the next reliability hole, not a reason to block this PR.
Sent by Cursor Automation: Fix Issues
| ) -> Result<(), ScoringCompletionOutboxError> { | ||
| if completion_event.source() != SOURCE_REF | ||
| || completion_event.subject_ref() != scoring_job_ref | ||
| || completion_event.occurred_at_unix_ms() != completed_at_unix_ms |
There was a problem hiding this comment.
The bind set is source + job + occurred_at / completed_at. Leave ResultSnapshot and scoring_result_ref to the worker/schema PR. Keep event_ref stable across retries: a new identity after Duplicate completion inserts a second outbox row rather than reconciling.
Point the active-work slot at the completion-plus-failure successor so reviewers do not treat #69 as a separate landable head. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>


Why
Protected main persists fenced scoring-job completion and transactional outbox evidence, but those APIs can still be called in separate commits. A crash between accepting an immutable result and enqueuing its integration evidence can therefore leave downstream consumers permanently unaware of a valid completed result.
What
postgres_scoring_completioncomposition that executes the existing fenced completion transition and existing outbox insert in the same caller-ownedREAD COMMITTEDtransaction.Verification intent
cargo test --test postgres_scoring_completion_outboxBase at branch creation:
cc5850a0d1eacbbf16d03075534fce460a8286e6.