PR Webhook E2E Test — Real Pipeline #3
No reviewers
Labels
No labels
Compat/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
starsetbyte/Carin!3
Loading…
Reference in a new issue
No description provided.
Delete branch "test/pr-e2e"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Real PR pipeline test. Carin should:
f22961b2ac856bfde62e6086558c789e4497262aCarin Code Review — VERDICT: APPROVE
Review: PR #3 — Durable Review Loop + PWA Shell
Branch:
test/pr-e2e→mainAuthor: starsetbyte
Date: 2026-07-12
Summary
This PR implements the core of Carin's Phase 0–1 review-loop hardening plus Phase 3.1 PWA shell, as described in the companion plan (
docs/plans/2026-07-12-review-loop-hooks-pwa.md). The headline changes are:processedStatusRuns/postedPRCommentRunsmaps with a persistent SQLite delivery record with lease tokens, atomic claiming, exponential backoff, and retry with a finite cap.ReviewOriginreplaces pipe-delimitedcontext_summaryparsing as the primary contract for Forgejo/GitHub metadata, with lineage resolution throughparent_run_id+ recursive CTE.pull_requestevents) that creates immediately-queued reviewer roots with typed origins, including support foropened/synchronize(d)/reopenedactions and audit-only logging for other actions.run.queued,run.started,review.verdict,review.delivery.succeeded/failed, etc.) persisted alongside mutations, with an opt-in observer webhook subscription model.vite-plugin-pwa: manifest, icons, service worker, offline detection, and aNetworkFirstcaching strategy for safe read-only API routes.Test Results
go build ./cmd/caringo vet ./...go test ./... -count=1npm run check(Svelte)What's Good
1. In-memory map removal is correct and complete
The two process-memory maps (
processedStatusRuns,postedPRCommentRuns) that caused duplicate comments and stale pending statuses on restart are fully removed. The new outbox persists delivery state, lease tokens, and retry attempts in SQLite — restarting the server replays only incomplete deliveries. The E2E test (TestInboundPRE2EForgeOutboxIsDurableAndDeduplicated) proves this with a fake Forgejo server: two dispatcher instances against the same DB produce exactly two status changes (pending → terminal) and one PR comment, with no duplicates.2. Lease-based claiming prevents duplicate sends across concurrent dispatchers
ClaimDueReviewDeliveries(reviews.go:235–275) atomically upgrades due rows toin_flightwith a random lease token, and reportsRowsAffectedto skip any row another worker already claimed.MarkReviewDeliveryDeliveredthen double-checksstate = in_flight AND lease_token = ?before committing. The testTestConcurrentDispatchersClaimDeliveryOnlyOnceproves this: two goroutines racing ondispatchReviewOutboxresult in exactly one Forgejo request.3. Typed origin with lineage resolution
ReviewOriginForRunuses a recursive CTE to walkparent_run_idup to the inbound root, then joinsreview_origins. This means every handoff descendant can resolve the original repo/SHA/PR identity without copying webhook text intocontext_summary. ThematerializeReviewEffectspath usesTerminalReviewerRunsWithoutDeliverywhich also walks lineage, ensuring the final reviewer (potentially several handoff hops deep) still maps back to the correct root origin.4. Artifact selection is deterministic and safe
selectReviewArtifact(review_outbox.go:216–243) has clear precedence:review.md>summary.md> first safe text artifact. It excludespi-session,dispatch, raw-output, diffs, and empty artifacts. The PR comment body formats with## Carin Code Review — VERDICT: APPROVEheader and a[View review run]link only when a configured base URL is valid. This directly addresses the "hash comments" finding from the ground-truth audit.5. SPA routing middleware is minimal and correct
spaDocumentOrAPI(main.go:296–304) serves the Svelte workspace only for browser document GET requests (Accept: text/html), keeping API routes intact for JSON consumers. TheGET /runs/{id}route is specifically wrapped for deep-linking — reloading a run page now serves the SPA and restores the correct run, rather than returning raw JSON. The testTestDirectRunDeepLinksverifies this in Playwright.6. Domain events are transactional
Domain events are inserted in the same transaction as their triggering mutation (
insertDomainEventreceives*sql.Tx). If a status transition is illegal (e.g., double-complete), the domain event is never persisted —TestDomainEventPersistsOnlyAfterSuccessfulRunTransitionverifies this. Theafter_idcursor forDomainEventshandles same-timestamp events correctly via a three-way comparison (occurred_at > COALESCE(…) OR (occurred_at = … AND id > …)).7. Webhook provider discrimination
webhookProvider(inbound.go:67–75) correctly prioritises Forgejo identity: ifX-Forgejo-EventorX-Forgejo-Deliveryis present, the payload is treated as Forgejo regardless of any GitHub compatibility headers. The signature verification follows the same priority. The testTestForgejoCompatibilityHeadersAcceptQueuedReviewWithTypedOriginverifies that a payload carrying bothX-Forgejo-*andX-Hub-Signature-256is classified as Forgejo.8. PWA integration is clean
The service worker, manifest, and registration are minimal and standard. Offline detection flows through the existing workspace store via
setNetworkState(), which setsofflineandstaleflags. The topbar renders a visible "OFFLINE · cached reads may be stale" banner.NetworkFirstis used only for a whitelist of safe read-only API routes — never for mutations.Observations (Non-Blocking)
1. Unused
projectsandknowledgeprops in Overview.svelteThe refactored
Overview.svelte(the new table/health-bar layout) declaresprojectsandknowledgeas export props but never uses them in the template. They're passed fromApp.sveltebut the component doesn't reference them. This triggers the 2 Svelte warnings. These props should either be removed from the export declaration or the component should use them.Severity: Cosmetic. No runtime impact, no test failures.
Suggested fix: Remove
export let projectsandexport let knowledgefromOverview.sveltesince they're no longer used there.2.
itoahelper in review_e2e_test.go duplicates stdlibThe test file defines
func itoa(v int64) string { return strconv.FormatInt(v, 10) }. This is a thin wrapper that could be replaced by usingstrconv.FormatIntdirectly orfmt.Sprint. It works correctly but adds an unnecessary helper.Severity: Trivial, test-only.
3.
TerminalReviewerRunsWithoutDeliveryhas no row limitThe SQL query scans ALL terminal reviewer runs without a
LIMIT. While this is intentional (as documented: "It intentionally has no recent row cap: an old missing external effect must not be stranded by newer runs"), the query could theoretically scan a large number of rows on a server with thousands of runs. ThematerializeReviewEffectscaller wraps this in a per-tick call with anORDER BY id DESC, so earlier rows are processed first, and theUNIQUE(root_run_id, action)constraint onreview_deliveriesensuresEnqueueReviewDeliveryis a no-op for already-enqueued deliveries.Severity: Acceptable for current scale; may warrant a periodic backfill strategy if the run table grows very large.
4. Legacy
postReviewStatusesandpostReviewPRCommentsare still called fromtick()The deprecated migration paths in
dispatch.go(postReviewStatusesandpostReviewPRComments) are still invoked fromtick()alongside the new outbox path (materializeReviewEffects+dispatchReviewOutbox). This is intentional as a migration path — runs created before this PR won't have typed origins or outbox records. The plan's Phase 0 tasks mention a feature flag that should gate this; that flag hasn't been implemented yet. Until it is, both paths run, but the durable outbox's uniqueness constraint prevents duplicate effects.Severity: Low. The outbox's
UNIQUE(root_run_id, action)constraint prevents duplicates even when both paths run. The legacy path doesn't attempt to post if the outbox already recorded the effect.Verdict
All tests pass. The durable review outbox replaces the core reliability gap (in-memory maps). The typed
ReviewOriginwith lineage resolution makes the pipeline self-documenting rather than string-parsing dependent. The E2E test against a fake Forgejo API validates the entire webhook→queued→review→outbox→delivered chain. The PWA integration adds installability and offline resilience without breaking existing API routes.No logic errors, no regressions, no security concerns.
VERDICT: approve
View review run
Generalizes the review delivery outbox pattern to all webhook targets. New model/types: - model/webhook.go: WebhookDelivery, WebhookDeliveryParams, state constants Store layer (lease-based, crash-safe): - webhook_deliveries table with lease tokens, retry timing, dedup index - EnqueueWebhookDelivery, ClaimDueWebhookDeliveries (CAS lease) - MarkWebhookDeliveryDelivered, RetryWebhookDelivery (exp backoff) - ListWebhookDeliveries, WebhookDeliveryByID - Tests: enqueue, claim idempotency, lease expiry reclaim, retry cap at max_attempts, delivered marking, state filter API visibility: - GET /webhook-deliveries (list with ?state=&limit=) - GET /webhook-deliveries/{id} Webhook dispatcher changes: - DeliveryStore interface for durable outbox - enqueueReliable(): durable delivery for reliable:true targets - dispatchOutbox(): claim → send → mark/retry loop - StartOutboxWorker(): periodic ticker goroutine - reliable:true is now accepted — rejection removed from New() and ConfigFromFile - Falls back to inline fire when store is nil - Tests: reliable enqueue, observer path, HTTP roundtrip, error retry, non-reliable inline fire, config accepts reliable, nil-store no-panic Wiring: - cmd/carin/main.go: outbox worker started, routes registered - internal/api/api.go: webhook delivery routes mounted All 13 packages pass (go test, go vet clean).Carin Code Review — VERDICT: APPROVE
$(cat /tmp/review.md)
View review run
Carin Code Review — VERDICT: APPROVE
Code Review: PR #3 — Durable Review Pipeline & PWA
Branch:
test/pr-e2e→mainAuthor: starsetbyte
Reviewer: pi-reviewer (run #281)
Verdict: APPROVE ✅
This is a well-executed PR that replaces fragile in-memory state with a durable outbox pattern for Forgejo review deliveries, fixes bug-prone webhook metadata handling, and adds PWA installability to the existing SPA. All tests pass and
go vetis clean.Summary of Changes
The PR addresses the seven findings documented in the plan's "Ground Truth" table:
pendingqueued; typedreview_originstrack the repo/SHAReviewOriginForRunresolves through parent lineage via recursive CTEsafeReviewArtifactexcludespi-session,dispatch, raw output, diff artifactspostedPRCommentRuns/processedStatusRunsreplaced with durablereview_deliveriestablewebhookProviderchecksX-Forgejo-Event/X-Forgejo-DeliveryfirstdraftAcceptReviewatomically creates queued root + typed origin + pending deliveryvite-plugin-pwa;Todaymobile viewArchitecture Assessment
Durable Outbox (
internal/dispatch/review_outbox.go)The outbox pattern is correctly implemented:
ClaimDueReviewDeliveriesuses atomic UPDATE with state filters)(root_run_id, action)deduplicates at the schema levelreview.md>summary.md> first safe artifactTyped Review Origins (
internal/store/reviews.go)context_summaryAcceptReviewcreates run + origin + pending delivery in one transactionEnsureReviewOriginprovides idempotent migration path for legacy runsWebhook Handler Fixes (
internal/webhook/inbound.go)webhookProvider()checks Forgejo headers before falling back to GitHubhandlePREventprocessespull_requestevents separately from push eventsopened,synchronize(d),reopenedcreate review runs; others are auditedinbound_audits) records non-spawning events for observabilityTesting Coverage
New Tests Added:
TestInboundPRE2EForgeOutboxIsDurableAndDeduplicated— full E2E: webhook → queued run → typed origin → outbox delivery → dedup on restartTestReviewOutboxRetriesAndDoesNotRedeliverAfterDispatcherRestart— verifies retry + restart safetyTestReviewArtifactPrecedenceAndSafety— verifiesreview.mdwins, session/raw excludedTestReviewCommentExcludesAllOutputArtifactsUsesSummaryAndBoundsUTF8— comment body correctnessTestConcurrentDispatchersClaimDeliveryOnlyOnce— lease isolationTestReviewOutboxPublishesOnlyFinalReviewerLeafAfterReviseFixApprove— revise → fix → approve pipelineTestReviewOutboxMaterializesOldMissingTerminalBeyondRecentHundred— no stranding of old runsTestForgejoCompatibilityHeadersAcceptQueuedReviewWithTypedOrigin— Forgejo priority detectionTestForgejoPushAcceptsQueuedReviewerRootAndPendingDelivery— push events use same durable pathTestPRPayloadParsing,TestPRWebhookCreatesReviewRun,TestPRWebhookSkipsClosedAction, etc. — PR event parsingTestDomainEventPersistsOnlyAfterSuccessfulRunTransition,TestTerminalTypedReviewerPersistsVerdictExactlyOnceTestReliableTargetEnqueuesViaStore,TestDispatchOutboxSendsHTTPAndMarksDelivered, etc.Minor Observations (Non-Blocking)
1. Dead code from old status/comment posting
The old
postReviewStatuses()andpostReviewPRComments()methods remain indispatch.gobut are no longer called fromtick(). They can be removed in a follow-up cleanup.2.
promoteLegacyReviewOriginunusedThe legacy migration function is defined but does not appear to have a call site. This is acceptable as future-proofing for pre-migration runs.
3. Large package-lock.json diff
The
webapp/package-lock.jsondiff is ~7K lines due tovite-plugin-pwaand its transitive dependencies (workbox, babel, rollup plugins). This is expected and unavoidable for the PWA feature.4. Hardcoded base URL remnants in deprecated code
The old
postForgejoCommitStatusstill referenceshttp://astraea1:8787/ui/runs/%das a fallback URL. Since this function is no longer called by the dispatcher tick, this is not an active bug, but it should be cleaned up.Security Review
webhookProvider()correctly distinguishes Forgejo from GitHub (Forgejo-first priority)crypto/randspaDocumentOrAPIonly interceptstext/htmldocument GETs; all API paths remain JSONNetworkFirststrategy; mutations are never queued offlineView review run