Skip to content

fix(review): stop re-reviewing a PR whose CI is permanently stuck - #3755

Merged
JSONbored merged 3 commits into
mainfrom
fix/orb-ci-stuck-finalize-repeat-review
Jul 6, 2026
Merged

fix(review): stop re-reviewing a PR whose CI is permanently stuck#3755
JSONbored merged 3 commits into
mainfrom
fix/orb-ci-stuck-finalize-repeat-review

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • prReadyForReview finalizes (runs a full paid AI review) once a PR's CI has been pending past its staleness cap, so it's never silently deferred forever -- correct in isolation, but it had no memory of having already done so for the exact same head SHA.
  • Confirmed live in production (audit_events, direct query): 3 PRs whose CI never settles (JSONbored/metagraphed#3546, JSONbored/metagraphed#3594, JSONbored/awesome-claude#4398) were each re-reviewed 200-300+ times over more than 20 hours, continuing right up through this investigation -- entirely separate from the sweep outage-repair cap fixed in fix(review): stop the medium-effort retry storm burning tokens on zero output #3747, since ordinary (non-priority) sweep candidacy and live webhook re-evaluations both still reach this exact function every few minutes.
  • Fix: cap it at one finalize per head SHA via a SHA-scoped audit event (reusing the existing audit_events ledger, no new table/migration). Once already finalized for the current head SHA, defer again instead of paying for another review. A new commit changes the head SHA, which resets the guard and lets a still-stuck PR finalize fresh.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • No issue linked -- discovered via a live production audit (direct query against audit_events), not a filed issue; the summary above is the evidence.

Validation

  • git diff --check
  • npm run actionlint -- not run (no workflow files touched)
  • npm run typecheck -- clean
  • npm run test:coverage (full/unsharded) -- not run locally; ran the full affected file directly instead (test/unit/queue.test.ts, 587/587 passed, including the new regression test)
  • npm run test:workers / npm run build:mcp / 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 -- not run locally (no wrangler/MCP/UI/dependency surface touched); left to CI.
  • New behavior has a regression test: runs the same stuck PR through prReadyForReview twice at the same head SHA and asserts the first evaluation finalizes (posts a real check-run/gate write) while the second defers with zero additional writes.

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 auth/cookie/CORS/GitHub App/Cloudflare/session changes.
  • N/A -- no API/OpenAPI/MCP surface changed.
  • N/A -- no UI changes.
  • N/A -- no docs/changelog changes needed.

Notes

@superagent-security

superagent-security Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Superagent did not find any vulnerabilities or security issues in this PR.

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

loopover-orb Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Caution

🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥

🛑 Gittensory review result - fixes required

Review updated: 2026-07-06 08:03:01 UTC

2 files · 1 AI reviewer · 2 blockers · readiness 93/100 · CI failing · unstable

🛑 Suggested Action - Manual Review

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.

Review summary
This PR closes a real, confirmed-in-production gap: prReadyForReview could finalize a paid AI review on a permanently CI-stuck PR every time it was re-evaluated (every few minutes), because there was no record that it had already finalized for that exact head SHA. The fix adds a SHA-scoped audit-event guard (reusing audit_events, no schema change) that caps finalization to once per head SHA and defers on repeat evaluations, resetting naturally when a new commit changes the SHA. The new regression test exercises both the first-finalize and second-defer paths and asserts the guard's audit event, the deferred event, and the Sentry-visible log line, which is solid coverage of the core logic.

Nits — 5 non-blocking
  • src/queue/processors.ts: the guard's own recordAuditEvent call is wrapped in `.catch(() => undefined)` (consistent with the rest of the file), so a transient DB write failure on the guard write would silently make every future evaluation re-finalize again — worth a comment acknowledging this is an accepted trade-off rather than an oversight.
  • test/unit/queue.test.ts: no test covers the described reset behavior (a new commit / different head SHA lets a still-stuck PR finalize again) — only same-SHA repeat is tested, so the 'resets on new commit' claim in the PR description is unverified by the test suite.
  • src/queue/processors.ts: `countRecentAuditEventsForActorAndTarget`'s parameter order/signature isn't visible in this diff — worth double-checking the call matches its definition, particularly that `CI_STUCK_FINALIZE_GUARD_EVENT_TYPE`/`guardTargetKey`/lookback timestamp land in the right positional slots.
  • The flagged 'generic_secret_assignment' at test/unit/queue.test.ts around line 1487 is a cache-key string (`ci-pending-first-seen:...`), not a real secret — false positive, no action needed.
  • Consider adding a short test that changes `head.sha` after a first finalize and asserts the guard does not suppress finalization for the new SHA, to lock in the reset behavior described in the PR summary.

Why this is blocked

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.

CI checks failing

  • codecov/patch — 83.33% of diff hit (target 99.00%)
Signal Result Evidence
Code review ❌ 2 blockers 1 reviewer
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: 54 registered-repo PR(s), 46 merged, 413 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 54 PR(s), 413 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 54 PR(s), 413 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
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.

🟩 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 Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.32%. Comparing base (8398615) to head (963b654).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3755      +/-   ##
==========================================
+ Coverage   93.30%   93.32%   +0.02%     
==========================================
  Files         317      317              
  Lines       32384    32396      +12     
  Branches    11876    11877       +1     
==========================================
+ Hits        30215    30234      +19     
+ Misses       1537     1530       -7     
  Partials      632      632              
Files with missing lines Coverage Δ
src/queue/processors.ts 94.40% <100.00%> (+0.27%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

JSONbored added 3 commits July 6, 2026 02:38
prReadyForReview finalizes (runs a full paid AI review) once CI has been
pending past its staleness cap, so a PR is never silently deferred forever.
But it had no memory of having already done so: a CI context that will never
settle (an orphaned required context, a fork check with no webhook to ever
fire) hits this exact branch again on every later evaluation and re-spends
another review for a disposition already established.

Confirmed live: 3 PRs whose CI never settled were each re-reviewed 200-300+
times over more than 20 hours this way -- independent of the sweep's own
outage-repair attempt cap (#3747), since ordinary (non-priority) sweep
candidacy and live webhook re-evaluations both still reach this function.

Caps it at one finalize per head SHA via a SHA-scoped audit event (reusing
the existing ledger, no new table): once already finalized for the exact
current head SHA, defer again instead of paying for another review. A new
commit changes the head SHA, resetting the guard so a still-stuck PR
finalizes fresh.
The DB-only audit_events write from the previous commit has no operator
visibility until someone queries the ledger directly, the way this bug was
originally found. Add a matching structured console.error at the exact
moment the guard suppresses a repeat review, so it flows through the
existing forwardStructuredLogToSentry pipeline (level:"error" is
deliberate -- the line's own existence IS the anomaly signal, same
convention as selfhost_ai_provider_failed / selfhost_ai_providers_exhausted).
…ze guard

Both new recordAuditEvent calls in the #orb-ci-stuck-repeat guard
(review_finalized_ci_stuck_guard on first finalize, review_deferred_ci_pending
on a guarded repeat) swallow their own write failures via .catch(() =>
undefined), but neither failure path had a test -- codecov flagged both lines
as uncovered patch. Add two regression tests forcing the audit_events insert
to throw in each case and asserting the surrounding finalize/defer decision
still completes correctly despite the swallowed write failure.
@JSONbored
JSONbored force-pushed the fix/orb-ci-stuck-finalize-repeat-review branch from 4604fd7 to 963b654 Compare July 6, 2026 09:41
@JSONbored
JSONbored merged commit 9b1cfc9 into main Jul 6, 2026
7 checks passed
@JSONbored
JSONbored deleted the fix/orb-ci-stuck-finalize-repeat-review branch July 6, 2026 09:45
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

Development

Successfully merging this pull request may close these issues.

1 participant