fix(orb): repoint the orphaned review_targets reputation signal, sample clock skew in broker mode, and bound the upstream-drift fallback fetch#9230
Conversation
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
❌ 6 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
|
Caution 🛑 LoopOver review result - fixes requiredReview updated: 2026-07-27 08:56:08 UTC
Review summary Nits — 6 non-blocking
CI checks failing
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk 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.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Decision record
🟩 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.
|
a63a1d2 to
bf4a216
Compare
) fetchTrackedSource's raw-GitHub fallback was the only fetch in ruleset.ts without a timeout, so a stalled raw.githubusercontent.com connection hung the whole scheduled refreshUpstreamDrift job. Route it through the same timeoutFetch every other call site in the file already uses, and confirm the Promise.all fan-out degrades per-source rather than all-or-nothing. Sweep the rest of src/ for the same bare-fetch shape: three GitHub calls in github-oauth.ts (device-flow start/poll, web-OAuth code exchange) and one call each in linear-adapter.ts and registry/sync.ts had no timeout and are now bounded the same way.
#9156) recordClockSkewFromResponse's only call site is inside the local GitHub App JWT mint path, which a brokered self-host never reaches (mintInstallationToken returns from the broker branch first) -- so a brokered deployment's clock skew gauge read a hard 0, indistinguishable from "clock is fine". Sample it from fetchBrokeredInstallationToken's own response instead, on both success and failure. 0 is a real, reachable skew value (a perfectly synced clock), so it can't double as "never sampled" -- clockSkewSecondsSample() now returns NaN until the first real sample lands. The companion loopover_clock_skew_sample_age_seconds gauge (added in #7000 precisely so a stale reading is distinguishable from a fresh one) was wired to zero alert rules and zero dashboard panels. Add LoopoverClockSkewSampleStale (fires past 2h, tolerating normal ~hourly mint-timing jitter) and the matching Grafana stat panel.
…w_targets (#9136) getSubmitterReputation, getSubmitterReputationAcrossInstall, and listSubmitterCohortRows read `review_targets` over a 90-day recency window -- but review_targets has had no live writer since the 2026-06-22 convergence cutover, so that window reads a shrinking set that goes permanently empty around 2026-09-20 (following #9015's partial fix and #9179's repoint of the anomaly-alerter's reversal/DLQ signals). Repointed onto the same live ledgers #9179 and getSubmitterCadence (#9015) already read: review_audit's pr_outcome rows (the realized outcome, not just the bot's own prediction) joined to pull_requests for the submitter's login, restricted to each target's LATEST pr_outcome row so a redelivered webhook can't double-count. ams-miner-cohort.ts required no changes -- it consumes listSubmitterCohortRows directly and picks the repoint up for free; its stale doc comments are updated to match. Before restoring the signal, give evaluateVisualVisionGate's and evaluateScreenshotTableVisionGate's `low_reputation` skip a compensating advisory finding: silently skipping the one extra AI check a low-reputation submitter's confirmed visual change would otherwise get reproduces #9015's "suspicion buys less scrutiny" shape. Remaining scope, tracked under #9136: ops.ts's computeAgentHealth (byStatus/byVerdict/failedRows/manualRate/stuckRetryable/failed) and computeCalibration still read review_targets directly. Deferred because review_targets' non-terminal states (queued/reviewing/error/error_retryable) and the attempt-exhausted 'failed' bucket have no live per-target equivalent post-cutover -- gate_decision's own `decision` column only ever records 'merge' | 'close' | 'hold', not the full status enum review_targets tracked. See the PR description for the full reader inventory.
bf4a216 to
f3d151a
Compare
Bundles three related "a placeholder silently satisfies its own health check" observability fixes.
#9165 — raw-GitHub fallback fetch had no timeout
fetchTrackedSource's raw-GitHub fallback (src/upstream/ruleset.ts:520) was the onlyfetchin thefile without a timeout; a stalled
raw.githubusercontent.comconnection hung the whole scheduledrefreshUpstreamDriftjob. It now goes through the sametimeoutFetchevery other call site in thefile already uses. Confirmed the
Promise.allfan-out inrefreshUpstreamSourceSnapshotsdegradesper-source (a test simulates one source timing out while five others complete normally).
Swept
src/for the same bare-fetchshape and found four more real gaps, all fixed the same way:src/auth/github-oauth.ts— device-flow start/poll + web-OAuth code exchange (3 call sites)src/integrations/linear-adapter.ts— the Linear GraphQL callsrc/registry/sync.ts— the registry-probe candidate loopEverything else
grep-matched already carried asignal: AbortSignal.timeout(...)(verified site bysite — qdrant-vectorize.ts, ai.ts, capture.ts, actions-fallback.ts, notify-*.ts, gittensor/api.ts, etc.).
#9156 — clock skew never sampled in broker mode, staleness gauge wired to nothing
recordClockSkewFromResponse's only call site was inside the local GitHub App JWT mint path(
src/github/app.ts:224), which a brokered self-host can never reach —mintInstallationTokenreturns from the broker branch first, so a brokered deployment's clock-skew gauge read a hard
0,indistinguishable from "clock is fine". Now sampled from
fetchBrokeredInstallationToken's ownresponse too (both the success and failure arm — the Date header is there either way).
0is a real, reachable skew value (a perfectly synced clock), so it can't double as "unmeasured" —clockSkewSecondsSample()now returnsNaNuntil the first real sample lands.The companion
loopover_clock_skew_sample_age_secondsgauge (added in #7000 precisely so a stalereading is distinguishable from a fresh one) was referenced by zero alert rules and zero dashboard
panels. Added
LoopoverClockSkewSampleStale(fires past 2h — a ~2x margin over the roughly-hourlytoken-mint cadence) plus the matching Grafana stat panel. Audited the sibling
-1-sentinel gauge(
loopover_orb_relay_drain_seconds_since_last, #9128) — it already has an alert rule from a prior PR,so nothing further needed there.
#9136 — orphaned
review_targets(largest of the three)review_targetshas had no live writer since the 2026-06-22 convergence cutover. A prior PR (#9179)already repointed the anomaly-alerter's reversal and DLQ signals in
ops.tsonto the livereview_auditledger (and fixed thetarget_idnamespace mismatch in that join), landingcheckReviewSourceFreshnessas the generalizable staleness check. That PR explicitly deferred therest of the scope to a follow-up — this PR is that follow-up.
Decision: repoint, not restore a writer — same call #9179 already made for the reversal/DLQ half;
this PR applies it consistently rather than re-litigating.
Full reader inventory (per the issue's request to enumerate every one):
ops.tsreversals + DLQ signals (computeAgentHealth)ops.tscheckReviewSourceFreshnesssubmitter-reputation.ts—getSubmitterReputationsubmitter-reputation.ts—getSubmitterReputationAcrossInstallsubmitter-reputation.ts—listSubmitterCohortRowsams-miner-cohort.tslistSubmitterCohortRowsdirectly, picks up the repoint for free; stale doc comments updatedevaluateVisualVisionGate/evaluateScreenshotTableVisionGatelow_reputationskipops.tscomputeAgentHealth—byStatus/byVerdict/failedRows/manualRate/stuckRetryable/failedreview_targets-sourced — deferred, see belowops.tscomputeCalibration— merged/closes-by-reason/disputedreview_targets-sourced — deferred, see belowpublic-stats.tsWhat's repointed:
getSubmitterReputation,getSubmitterReputationAcrossInstall, andlistSubmitterCohortRowsmoved offreview_targets's 90-dayterminal_atwindow (which was headingto permanently-empty around 2026-09-20) onto
review_audit'spr_outcomerows joined topull_requestsfor the submitter's login — the same live ledgergetSubmitterCadence(#9015) and#9179's reversal/DLQ signals already read. Restricted to each target's latest
pr_outcomerow, sincerecordPrOutcome's webhook path has no existence check (unlike the direct-write path) and aredelivered
closedwebhook can double-insert.avgAttemptCountinlistSubmitterCohortRowsis a documented substitution, not a silent behaviorchange: the pre-repoint
review_targets.attempt_count(the gate's own re-review counter) has no liveequivalent, so it now reads
pull_requests.merge_attempt_count— narrower (only failed merge attempts:permission/check/conflict, reset per head SHA) but still a genuine review-friction signal. Called out
in both
submitter-reputation.tsandams-miner-cohort.ts's doc comments.Compensating hold: restoring the reputation signal without addressing the two vision-gate skips
would reproduce #9015's "suspicion buys less scrutiny" shape — a low-reputation submitter's
low_reputationskip in
evaluateVisualVisionGate/evaluateScreenshotTableVisionGatepreviously vanished with no trace.Both now push a
severity: "warning"advisory finding (never a blocker — both modules stay strictlyadvisory) naming how many real routes/pairs were skipped, wired into
processors.ts'srunVisualVisionForAdvisory/runScreenshotTableVisionForAdvisory. No finding is pushed when there wasnothing to compensate for (no confirmed regression / no real image pairs) — that's a legitimately
different skip reason, not reputation-driven.
Deferred, still on
review_targets:computeAgentHealth'sbyStatus/byVerdict/failedRows/manualRate/stuckRetryable/failedandcomputeCalibration's merged/closes-by-reason/disputedqueries. These aren't a mechanical repoint like the pieces above —
review_targets' non-terminalstates (
queued/reviewing/error/error_retryable) and its attempt-exhaustedfailedbucket haveno live per-target equivalent post-cutover:
gate_decision's owndecisioncolumn(
src/review/parity.ts'sGateAction) only ever records'merge' | 'close' | 'hold', not the fullstatus enum
review_targetstracked. Repointing these needs a product decision on what amanual/error/stuckbucket even means without a live gate-processing-state ledger, which I didn't want toguess at silently in the same PR as the mechanical repoints above.
checkReviewSourceFreshness'sreview_targetscomment is updated to point at these as the actual remaining live readers (itpreviously, now-incorrectly, cited
submitter-reputation.ts).Practical effect: the anomaly alerter's DLQ/reversal signals (fixed in #9179) and the reputation
quality signal + AMS-cohort dashboard (fixed here) are live again.
manualRate/stuckRetryable/failedin the/statusalerter remain silently zero until the deferred piece above is resolved —called out explicitly rather than left to be rediscovered.
Testing
npx tsc --noEmit -p tsconfig.json --incremental false— cleanvitest runacross every touched/consuming test file (clock-skew, orb-broker-client,upstream-ruleset, submitter-reputation, visual-findings, screenshot-table-vision(-wiring)x2,
visual-vision-wiring, ops, ams-miner-cohort, mcp-cli-ams-miner-cohort, registry, linear-adapter,
auth, auth-github-token, alerts-metric-name-references, observability-ci,
selfhost-grafana-dashboard, validate-observability-configs-script,
docs-selfhost-troubleshooting-metric-names) — 549 tests, all green
origin/main(4 commits, no overlap with touched files)Closes #9165
Closes #9156
Advances #9136 (see the deferred-scope note above —
ops.ts's remainingreview_targetsreaders areintentionally out of scope for this PR)