Skip to content

fix(review): paginate preview-url.ts's PR-comment and check-run GitHub reads - #7469

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
xfodev:fix/preview-url-paginate-7450
Jul 20, 2026
Merged

fix(review): paginate preview-url.ts's PR-comment and check-run GitHub reads#7469
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
xfodev:fix/preview-url-paginate-7450

Conversation

@xfodev

@xfodev xfodev commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #7450: src/review/visual/preview-url.ts's findPreviewUrlFromPrComments and getPreviewBuildState each fetched exactly one per_page=100 page (no page param, no Link header check) of /issues/{prNumber}/comments and /commits/{sha}/check-runs. On a PR with >100 discussion comments or a commit with >100 check-runs, the Cloudflare Workers Builds bot's preview-URL comment / check-run can land on page 2+, and either function then silently returned null / "absent" as if it genuinely didn't exist — a truncated page-1 response is indistinguishable from an empty one.

Both now walk the response's Link: rel="next" header, bounded to PREVIEW_LIST_MAX_PAGES = 10 — mirroring this repo's existing precedents (src/github/backfill.ts's githubPaginatedList/PR_DETAIL_MAX_PAGES and src/github/app.ts's workflow-run listing / MAX_WORKFLOW_RUN_LIST_PAGES, both bounded to 10) so a pathological PR/commit can't turn one read into an unbounded fetch loop.

Implementation

  • Refactored the module's bespoke githubJson into githubJsonWithLink (returns { payload, link }) with githubJson kept as a thin wrapper, so existing callers are unchanged — this file uses its own timeoutFetch helper rather than Octokit, so backfill.ts's Octokit-based githubPaginatedList isn't directly importable; a local walker mirroring its shape is the sanctioned option per the issue.
  • Added findAcrossPages(firstPageUrl, init, selectItems, probe): probes each page's items as it arrives and returns the first match, stopping early once found, when the Link header stops advertising a next page, or at the 10-page bound.
  • Fail-safe preserved exactly (the file header's "Every helper degrades to null/absent on failure — preview discovery must NEVER sink a review"): each function keeps its try/catch returning null/"absent", and a mid-pagination failure falls back to what earlier pages already yielded (each page is probed as it arrives, so a later-page failure never drops a successful first page — mirroring githubPaginatedList's own contract). Page 1 stays the bare per_page=100 request (the &page=N cursor is added only for page 2+), so the single-page path is byte-identical to before.

Tests

test/unit/preview-url.test.ts adds regression tests exercising the real functions against mocked multi-page fetch responses with realistic Link headers: bot comment / Workers-Builds check found on page 2; early-exit once found (no further pages fetched); the PREVIEW_LIST_MAX_PAGES bound against a pathological always-rel="next" mock (asserts exactly 10 fetches, no unbounded loop); mid-pagination fetch-failure degrading to null/"absent" without throwing; plus the user-less / link-less-comment and nameless-check-run edge branches. Every changed line and branch is covered (verified via --coverage), so codecov/patch holds.

Validation

  • git diff --check
  • npm run typecheck (root tsc --noEmit) green
  • npm run test:coverage on the affected suite: 100% of changed lines + branches covered (codecov/patch); existing preview-url / visual-capture tests pass unmodified
  • Scope is src/review/visual/preview-url.ts + its unit test only — no API/OpenAPI/MCP/UI/DB/wrangler surface, so no generated artifact needs regeneration

Safety

  • No secrets, wallets, hotkeys/coldkeys, PATs, private keys, raw trust scores, private rankings, or maintainer evidence.
  • The bot-login restriction (cloudflare-workers-and-pages[bot], unspoofable [bot] suffix) is unchanged — pagination doesn't widen which comments are trusted for a server-rendered *.workers.dev URL.
  • Auth/CORS/session: N/A. API/OpenAPI/MCP: N/A. UI: N/A. No changelog edit; no site//CNAME/lovable changes.

Closes #7450

…b reads

findPreviewUrlFromPrComments and getPreviewBuildState each read only one
per_page=100 page, so on a PR with >100 comments or a commit with >100 check-runs
the Cloudflare Workers Builds bot's preview comment/check-run could land on page
2+ and be silently missed (null/absent as if it did not exist). Both now walk the
Link: rel="next" header, bounded to PREVIEW_LIST_MAX_PAGES=10, mirroring the
existing githubPaginatedList (backfill.ts) and workflow-run listing (app.ts)
precedents. The fail-safe contract is preserved: each function still degrades to
null/absent on any failure and a mid-pagination failure falls back to earlier
pages, never throwing. Adds regression tests for the page-2 case, early-exit, the
page bound against a pathological always-next mock, and mid-page failure.

Closes JSONbored#7450
@xfodev
xfodev requested a review from JSONbored as a code owner July 20, 2026 11:09
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.35%. Comparing base (25decd9) to head (7b8beb3).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7469      +/-   ##
==========================================
- Coverage   91.35%   91.35%   -0.01%     
==========================================
  Files         716      716              
  Lines       72990    73004      +14     
  Branches    21628    21629       +1     
==========================================
+ Hits        66678    66690      +12     
  Misses       5272     5272              
- Partials     1040     1042       +2     
Flag Coverage Δ
shard-1 32.67% <100.00%> (-0.01%) ⬇️
shard-2 36.12% <3.33%> (+<0.01%) ⬆️
shard-3 34.52% <3.33%> (-0.01%) ⬇️
shard-4 42.86% <3.33%> (-0.03%) ⬇️
shard-5 36.96% <16.66%> (-0.01%) ⬇️
shard-6 33.47% <53.33%> (-0.08%) ⬇️

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

Files with missing lines Coverage Δ
src/review/visual/preview-url.ts 72.51% <100.00%> (+10.98%) ⬆️

... and 2 files with indirect coverage changes

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

loopover-orb Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-20 11:22:16 UTC

2 files · 1 AI reviewer · no blockers · readiness 100/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR fixes a genuine truncation bug: findPreviewUrlFromPrComments and getPreviewBuildState each read only one per_page=100 page and would silently miss the bot's comment/check-run on a PR with >100 comments or a commit with >100 check-runs. The fix walks the Link: rel=next header via a new findAcrossPages helper bounded to 10 pages, mirroring the repo's existing pagination precedents in backfill.ts and app.ts, and preserves the fail-safe null/absent degrade-on-error contract. The change is well-scoped, the page-1 URL stays byte-identical to the pre-pagination request, and the test suite covers early-exit, non-array payloads, mid-pagination failure, and the page-bound cap.

Nits — 5 non-blocking
  • src/review/visual/preview-url.ts:80 hasNextPage uses a naive `/rel="next"/` regex split on commas — a Link header value containing a comma inside a URL (unlikely but technically legal) could misparse; acceptable given GitHub's actual format but worth a one-line comment on the assumption.
  • The 404 in getLatestDeploymentStatus (src/review/visual/preview-url.ts:~140) is unrelated pre-existing code, not part of this diff's magic-number surface — the external brief's flag on line 45 doesn't correspond to a new literal introduced by this change.
  • test/unit/preview-url.test.ts's NEXT_LINK constant hardcodes `page=99` for both rel="next" and rel="last", which is harmless for these tests but slightly confusing since findAcrossPages ignores the URL in the Link header and always appends its own `&page=N`.
  • Consider extracting the `/rel="next"/` regex in hasNextPage (preview-url.ts:80) into a named constant/comment noting it assumes GitHub's exact Link header format.
  • The PR description states this closes fix(review): paginate preview-url.ts's PR-comment and check-run GitHub reads #7450 — confirm that issue is the correct eligible link since the external brief marks issue coverage as 'partial'.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #7450
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 (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 53 registered-repo PR(s), 29 merged, 0 issue(s).
Contributor context ✅ Confirmed Gittensor contributor xfodev; Gittensor profile; 53 PR(s), 0 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The diff adds a bounded, Link-header-driven pagination helper (findAcrossPages, PREVIEW_LIST_MAX_PAGES=10) and applies it to both findPreviewUrlFromPrComments and getPreviewBuildState, preserving the null/absent fail-safe contract including mid-pagination failures. It includes regression tests for multi-page discovery, early-exit on found match, mid-page fetch failure fallback, and the max-page bo

Review context
  • Author: xfodev
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: JavaScript, TypeScript
  • Official Gittensor activity: 53 PR(s), 0 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
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 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoopOver approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit fbd1cba into JSONbored:main Jul 20, 2026
15 checks passed
shin-core added a commit to shin-core/loopover that referenced this pull request Jul 21, 2026
…tuses reads

getLatestDeploymentStatus fetched only page 1 of a head SHA/ref's Deployments
list (per_page=10) and page 1 of each deployment's statuses -- never following
GitHub's Link: rel="next", unlike this file's own findPreviewUrlFromPrComments
and getPreviewBuildState. A ref with more than 10 deployments (repeated CI
re-runs, multiple environments, a long-lived branch) could carry the deployment
with the real environment_url outside page 1, so the function under-reported a
missing preview exactly as if none existed -- the same false-negative class the
file's own header warns about, and the class JSONbored#7469 already fixed for the
comment/check-run reads.

Reuse the existing findAcrossPages helper for both reads, the way the sibling
functions do: walk deployment pages, and per deployment walk its status pages,
returning the first usable environment_url. findAcrossPages now awaits its probe
so the outer deployments scan can fetch each deployment's statuses (a sync probe
is unaffected). The sawFailure/sawPending bookkeeping and DeploymentLookup return
contract are preserved.

Closes JSONbored#7805
JSONbored added a commit that referenced this pull request Jul 22, 2026
…as secret leaks (#7994)

* fix(review): stop bare hotkey/coldkey mentions from false-positiving as secret leaks

containsSecretLikeText matched the bare words "hotkey"/"coldkey" anywhere in a
registry document, unlike "wallet" (already scoped to "wallet path"). A Bittensor
hotkey is the standard PUBLIC miner identifier, not secret material, and appears
routinely in ordinary registry content -- API paths, field names, even notes
explicitly denying any such data ("No wallet/hotkey data"). Confirmed root cause
of 4 mis-closed metagraphed PRs in one day (#7469, #7589, #7591, #7594).

Scope hot/coldkey the same way wallet already is: require adjacency to something
that actually indicates key material (a keystore path, a private-key/password/
mnemonic/seed qualifier) rather than a bare word match.

Also fixes a companion bug: the privacy scrub that redacts private-rubric terms
from dynamically assembled/AI-generated public text was being applied to the
reviewer's own static, hardcoded secret-detection message too, rendering
"...secret, wallet, PAT..." as the confusing "...secret, [context], PAT...".
AdvisoryFinding gains an alreadyPublicSafe flag a producer can set when its
detail/publicText has no interpolated contributor or AI content, so a fixed
message an engineer already wrote and reviewed renders verbatim.

Closes #7981

* fix(engine): bump loopover-engine patch version for gate-decision twin parity

The #7981 fix touched src/rules/advisory.ts (formatCheckRunOutput) without a
matching edit to its gate-decision twin (packages/loopover-engine/src/advisory/gate-advisory.ts),
which has no such function. check-engine-parity.ts requires a version bump in
that case; expected-engine.version must stay in sync with it too.

* fix(scripts): resolve baseEngineVersion via git by default in engine-parity check

runEngineParityChecks aliased an un-overridden baseEngineVersion straight to
headEngineVersion, while its changedFiles default already resolved via a real
git diff against origin/main. That asymmetry meant a genuine version bump
could never be detected unless the caller passed baseEngineVersion explicitly
(only runEngineParityMain did), so any branch with a single-sided
gate-decision edit plus a correct version bump still failed this check.
Discovered while landing the #7981 hotkey/coldkey fix, which needed exactly
that bump for src/rules/advisory.ts's formatCheckRunOutput.

* fix(release): sync release-please manifest with the engine version bump

.release-please-manifest.json tracks packages/loopover-engine's version
independently of package.json; the 3.4.0 -> 3.4.1 twin-pair parity bump
needed a matching entry here too, caught by release-manifest:sync:check.

---------

Co-authored-by: loopover-orb[bot] <296761690+loopover-orb[bot]@users.noreply.github.com>
JSONbored added a commit that referenced this pull request Jul 22, 2026
…8074)

Closes #7985.

A bare owner reopen of a bot-closed PR stayed excluded from
reversalRate (still ambiguous — could be an administrative re-queue),
but an owner reopen followed by an approve/merge within 6h is
unambiguous: the owner looked again and decided the bot was wrong.
Every one of the 2026-07-21/22 metagraphed false-positive incidents
(#7469/#7589/#7591/#7594) was exactly this pattern, and the old
unconditional owner-reopen exclusion recorded nothing for any of them
— part of why the accuracy metric stayed misleadingly high that day.

Record a time-bounded owner_reopen_pending_reversal marker on reopen,
then promote it to a real reversal_reopened event if a merge follows
within OWNER_REOPEN_MERGE_WINDOW_MS. A bot reopening itself still
never counts.
JSONbored added a commit that referenced this pull request Jul 22, 2026
…us action (#8092)

Closes #7983.

The existing self-correction system only detects a systematically-
wrong rule via precision-over-time (auto-tune.ts), which needs a
real, DECIDED sample (>= AUTOTUNE_MIN_DECIDED) accumulated over
however long that takes -- too slow for a bug that can mis-close 4
PRs within hours, as the 2026-07-21/22 metagraphed incident did. A
much cheaper, ground-truth-free signal already exists: the SAME
deterministic rule/blocker code rejecting several DIFFERENT PRs in a
short window in the same repo is itself a strong "something's
broken" signal, independent of whether any of those rejections is
ever confirmed or reversed by a human.

New packages/loopover-engine/src/calibration/signal-tracking.ts:
evaluateRuleRepeatAlarm(ruleId, fired, threshold) — pure, no ground
truth needed, mirrors src/orb/analytics.ts's gamingPatternFlags
precedent ("Detection only — never an automatic action").

New src/review/rule-repeat-alarm-wire.ts wires this into ORB for
real: every gate block now records a #7982 rule-fired signal per
blocker code (nothing called the ORB adapter until now), scoped
per-(repo, code) so an unrelated repo or code never contributes to
another's count, and checks the repeat alarm inline, immediately
after each block — not on a later cron tick, matching the "hours,
not days" urgency the incident exposed. A triggered alarm logs a
structured console.error (forwarded to Sentry, the same "detected an
anomaly" channel src/review/ops-wire.ts's own runOpsAlerts already
uses) and writes a cooldown marker so an ongoing incident doesn't
re-alert on every subsequent PR.

Note on the issue's own cited alert channel: notify-discord.ts/
notify-slack turned out to be the wrong fit on inspection — that's a
per-REPO, community-facing channel for PR action notifications, not
an operator-facing "an ORB rule may be systematically broken" signal
that can span any repo the instance reviews. Sentry (via the
existing structured-log forwarder) is the channel actually already
used for this class of alert.

Validated against a replay of the exact #7469/#7589/#7591/#7594
incident shape: triggers on the 3rd distinct PR, matching the
issue's own "should have alerted after the 2nd or 3rd occurrence"
bar.
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(review): paginate preview-url.ts's PR-comment and check-run GitHub reads

1 participant