Skip to content

fix(selfhost): stop delayed webhooks from resurrecting closed PRs' review tracking - #7839

Merged
JSONbored merged 3 commits into
mainfrom
claude/orb-review-performance-analysis-a4152b
Jul 21, 2026
Merged

fix(selfhost): stop delayed webhooks from resurrecting closed PRs' review tracking#7839
JSONbored merged 3 commits into
mainfrom
claude/orb-review-performance-analysis-a4152b

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • upsertPullRequestFromGitHub now compares GitHub's own updated_at (new pull_requests.github_updated_at column) against what's already stored before applying state/headSha/mergedAt, so a webhook job delayed by queue backpressure can no longer dequeue after a newer event for the same PR and silently regress its state back to "open" — this is the exact mechanism that left active_review_tracking stuck showing an already-closed PR as still under active review.
  • The function's own returned record is corrected to match what was actually persisted (not the rejected raw payload), since callers like handlePullRequestWebhookEvent reason from that return value, not a fresh DB read.
  • Adds a flag/config-as-code-gated reconcile-active-review-tracking sweep (activeReviewReconciliation: in .loopover.yml, falling back to LOOPOVER_ACTIVE_REVIEW_RECONCILIATION, default OFF) that re-checks stale active rows against live GitHub state and terminalizes the ones confirmed closed — a self-heal backstop for this failure mode regardless of cause, matching the config-as-code pattern already used by prReconciliation/sweepWatchdog/ops.
  • Fixes a follow-on bug found while completing the above: lastSeenOpenAt, isReadyForReview, and the review-latency clock (headShaObservedAt) were still derived from the raw incoming payload rather than the staleness-resolved values, so a rejected-as-stale payload could still corrupt those derived fields even though state/headSha/mergedAt themselves were protected.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format.
  • This PR is focused on one concern (the out-of-order-webhook race and its config-as-code wiring) and does not mix in unrelated backend, UI, MCP, docs, dependency, or deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • Linked issue — none. This was diagnosed directly as a maintainer investigation into a live incident (a specific PR showing as stuck in review tracking ~44s after it actually closed on GitHub), not filed as a separate contributor-facing issue.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally — full unsharded run, 19,861 tests, 0 failures; every line/branch touched by this diff verified hit via the generated lcov.info (spot-checked src/index.ts, src/db/repositories.ts, src/queue/job-dispatch.ts, src/selfhost/queue-common.ts, src/review/active-review-reconciliation.ts — the only uncovered lines/branches in those files are pre-existing, unrelated to this diff)
  • npm run test:workers
  • npm run build:mcp (build:mcp:check in the full gate)
  • npm run test:mcp-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:build
  • npm audit --audit-level=moderate — 0 vulnerabilities
  • New/changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries — see Notes for the full list of added/changed test files

Ran the complete npm run test:ci gate three times end to end as this PR was iterated (each time after a fix), all green; final run: 1049 test files, 19,861 tests, 0 failures, 17 skipped (pre-existing, unrelated).

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics. (N/A — no public-facing GitHub text changed.)
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests — the new sweep mints a GitHub App installation token per stale row; tests cover a repo with no installation, a failed/inconclusive live-state check, a per-row error, and a top-level scan failure, all failing safe (row left untouched, no throw).
  • API/OpenAPI/MCP behavior is updated and tested where needed. (N/A — no API/OpenAPI/MCP surface changed.)
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. (N/A — no UI changes.)
  • Visible UI changes include a UI Evidence section. (N/A — no UI changes.)
  • Public docs/changelogs are updated where needed. config/examples/loopover.full.yml and .loopover.yml.example (self-host operator-facing config reference) are updated to document the new activeReviewReconciliation: block; CHANGELOG.md itself is intentionally untouched (not a release-prep PR).

UI Evidence

N/A — no UI, frontend, docs-site, or extension changes.

Notes

  • Migration 0172_pull_requests_github_updated_at.sql adds a single nullable TEXT column (github_updated_at); purely additive, no backfill needed — the staleness guard fails open (applies the write, exactly like before this column existed) whenever it's NULL on either side of the comparison.
  • New/changed test files: test/unit/active-review-reconciliation.test.ts (new, 12 tests), test/unit/db-parsers.test.ts (+8 tests: the out-of-order-webhook guard and its two review-latency-clock regressions), test/unit/db-persistence.test.ts (+4 tests: listStaleActiveReviewTracking), test/unit/focus-manifest.test.ts (+8 tests: activeReviewReconciliation: parsing/round-trip), test/unit/focus-manifest-validation.test.ts (+2 tests), test/unit/selfhost-config-lint.test.ts (+1 test), test/unit/index.test.ts (+2 tests: cron wiring), test/unit/queue-5.test.ts (+2 tests: job dispatch), test/unit/selfhost-queue-common.test.ts (+1 test: GitHub-budget registration).

…view tracking

A webhook job delayed by queue backpressure could dequeue AFTER a newer event
for the same PR (e.g. closed) already landed, and its stale embedded snapshot
would silently regress the locally stored state/headSha/mergedAt back to what
GitHub reported minutes earlier -- restarting active_review_tracking for a PR
that had already closed, with nothing left to terminalize it again.

Guard upsertPullRequestFromGitHub with GitHub's own updated_at (new
pull_requests.github_updated_at column) so an out-of-order payload can no
longer clobber newer state, and correct this call's own returned record to
match what was actually persisted. Add a flag-gated
reconcile-active-review-tracking sweep (LOOPOVER_ACTIVE_REVIEW_RECONCILIATION)
that re-checks stale active rows against live GitHub state as a self-heal
backstop.
…iation sweep

The reconciliation sweep added in the prior commit was env-var-only
(LOOPOVER_ACTIVE_REVIEW_RECONCILIATION), unlike every sibling self-heal flag
(prReconciliation, sweepWatchdog, ops), which all resolve a top-level
.loopover.yml manifest block first and fall back to the env var. Add the same
activeReviewReconciliation: manifest-override layer end to end: engine-package
parsing/serialization/lint recognition, the app-level cached resolver, and the
cron/dispatch call sites -- config-as-code is the established control surface
for these fleet-wide flags, not .env.

Also fixes a follow-on bug in the prior commit's out-of-order-webhook guard:
lastSeenOpenAt, isReadyForReview, and the review-latency clock
(headShaObservedAt) were still derived from the raw incoming payload instead
of the staleness-resolved state/headSha, so a rejected-as-stale payload could
still corrupt those derived fields even though the state/headSha/mergedAt
columns themselves were protected.
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 21, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
loopover-ui 98fa4db Commit Preview URL

Branch Preview URL
Jul 21 2026, 02:46 PM

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@JSONbored JSONbored self-assigned this Jul 21, 2026
@codecov

codecov Bot commented Jul 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.59155% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 91.38%. Comparing base (17ae3e6) to head (98fa4db).
⚠️ Report is 4 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/db/repositories.ts 91.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7839   +/-   ##
=======================================
  Coverage   91.37%   91.38%           
=======================================
  Files         729      730    +1     
  Lines       74688    74751   +63     
  Branches    22793    22809   +16     
=======================================
+ Hits        68246    68308   +62     
  Misses       5396     5396           
- Partials     1046     1047    +1     
Flag Coverage Δ
shard-1 56.25% <83.09%> (+1.75%) ⬆️
shard-2 51.06% <54.92%> (-4.14%) ⬇️
shard-3 53.85% <29.57%> (+2.53%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/loopover-engine/src/config-lint.ts 100.00% <ø> (ø)
...s/loopover-engine/src/focus-manifest-validation.ts 100.00% <100.00%> (ø)
packages/loopover-engine/src/focus-manifest.ts 98.43% <100.00%> (+0.01%) ⬆️
src/db/schema.ts 72.97% <ø> (ø)
src/index.ts 96.11% <100.00%> (+0.15%) ⬆️
src/queue/job-dispatch.ts 100.00% <100.00%> (ø)
src/review/active-review-reconciliation.ts 100.00% <100.00%> (ø)
src/selfhost/metrics.ts 100.00% <ø> (ø)
src/selfhost/queue-common.ts 98.65% <ø> (ø)
src/signals/focus-manifest-loader.ts 95.79% <ø> (ø)
... and 3 more

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 21, 2026
@loopover-orb

loopover-orb Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Caution

🛑 LoopOver review result - fixes required

Review updated: 2026-07-21 15:02:29 UTC

29 files · 1 AI reviewer · 2 blockers · CI failing · unstable

🛑 Suggested Action - Manual Review

Review summary
This closes the write-side race (compare GitHub's own `updated_at` before applying state/headSha/mergedAt, per the new `github_updated_at` column and matching migration) and adds a read-side self-heal sweep gated by config-as-code + env flag, following the existing prReconciliation pattern exactly (manifest field, TTL cache, cron enqueue, queue dispatch, GitHub-budget registration, metrics). The staleness guard correctly fails open when either side lacks a timestamp, and the derived fields (lastSeenOpenAt, isReadyForReview, headShaObservedAt) are now computed from the resolved rather than raw values, which is the real fix for the second bug described. The returned record from `upsertPullRequestFromGitHub` is also corrected to reflect what was persisted, matching caller expectations in `handlePullRequestWebhookEvent`.

Nits — 5 non-blocking
  • src/review/active-review-reconciliation.ts: fetchLivePullRequestState's undefined/'open'/'closed' contract is inferred from context but not visible in this diff — worth double-checking it never throws instead of returning undefined on a transient GitHub error, since the catch is only around token creation, not the fetch itself.
  • No index on `active_review_tracking(status, started_at)` per the comment in `listStaleActiveReviewTracking` — acceptable at current scale per the author's own note, but worth revisiting if row counts grow.
  • The per-row live GitHub call in `runActiveReviewReconciliation` is unbounded by a page/limit on `listStaleActiveReviewTracking` — fine today but could become a large fan-out if many rows go stale simultaneously (e.g., after an outage).
  • Consider capping `listStaleActiveReviewTracking`'s result set (e.g., LIMIT N per tick) to bound the number of live GitHub calls issued in one reconciliation pass.
  • Confirm `fetchLivePullRequestState`'s failure mode (network error, 404, rate limit) all cleanly resolve to something other than 'closed' so a transient error can never be mistaken for a confirmed close.

Why this is blocked

  • No linked issue detected: No closing reference or linked issue number was found in the PR metadata/body. — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue: This repo's maintainer focus manifest requires every PR to reference a tracked issue. — Link the relevant issue (for example Closes #123) before opening the PR.
📋 Copy for AI agents — paste into your coding agent
Fix the following blocker(s) from this PR review:

1. No linked issue detected: No closing reference or linked issue number was found in the PR metadata/body. — If this PR is intended to solve an issue, link it explicitly in the PR body.

2. Maintainer requires a linked issue: This repo's maintainer focus manifest requires every PR to reference a tracked issue. — Link the relevant issue (for example `Closes #123`) before opening the PR.

CI checks failing

  • codecov/patch — 98.59% of diff hit (target 99.00%)

Decision drivers

  • ❌ Code review — 2 blockers (1 reviewer)
  • ❌ Gate result — Blocking (Repo-configured hard blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 21 registered-repo PR(s), 14 merged, 347 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 21 PR(s), 347 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: significant
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, TypeScript, Ruby, Go, JavaScript, MDX, Shell, Solidity
  • Official Gittensor activity: 21 PR(s), 347 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Then work through the remaining 2 steps in the Signals table above.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 21, 2026
@JSONbored
JSONbored merged commit 6ea4e28 into main Jul 21, 2026
15 of 16 checks passed
@JSONbored
JSONbored deleted the claude/orb-review-performance-analysis-a4152b branch July 21, 2026 15:06
This was referenced Jul 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant