Skip to content

refactor(rees): migrate three analyzers off hand-rolled bounded-fetch onto boundedFetchText - #4821

Merged
loopover-orb[bot] merged 1 commit into
mainfrom
refactor/4759-rees-boundedfetchtext-migration
Jul 11, 2026
Merged

refactor(rees): migrate three analyzers off hand-rolled bounded-fetch onto boundedFetchText#4821
loopover-orb[bot] merged 1 commit into
mainfrom
refactor/4759-rees-boundedfetchtext-migration

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • doc-comment-drift.ts, exhaustiveness-drift.ts, and complexity-delta.ts each hand-rolled their own private "fetch a repo file's content at a ref, bounded/streamed read capped at 1MB" helper. duplication-delta.ts (Real duplication-delta analyzer (before/after comparison) #4741/feat(rees): detect resolved duplication via before/after comparison #4760) already migrated onto the more mature boundedFetchText (external-fetch.ts — typed ok/failure result, per-endpoint-category circuit breaker, configurable timeout, byte-size capping), so this migrates all three onto that existing utility instead, deleting each file's own readBoundedText/bounded-fetch logic entirely. No new shared module is introduced.
  • doc-comment-drift.ts also switches to the shared githubHeaders() helper instead of hand-building its own auth headers inline (the other two analyzers already used it).
  • Each migrated fetchFileAtHead/fetchFileAtHeadSha helper mirrors duplication-delta.ts's own reference implementation exactly, including the options.analysis?.fetchText(...) vs. direct boundedFetchText(...) fallback and the endpointCategory/phase/subcall field conventions.

Verification

  • All three analyzers' existing test suites (doc-comment-drift.test.ts, exhaustiveness-drift.test.ts, complexity-delta.test.ts) pass unchanged with zero logic edits — confirmed byte-faithful, including the oversized-response, streamed-cancel, and abort-timing edge cases already covered there.
  • Added one new test per file exercising the new options.analysis branch (mirroring duplication-delta.test.ts's own equivalent test), so that branch has real coverage rather than shipping untested.
  • Two comments in complexity-delta.test.ts that named the now-deleted readBoundedText's internal abort-polling as the mechanism under test were reworded to describe the actual post-migration mechanism (the analyzer's own outer post-fetch abort check) — no assertions changed.
  • No analyzer-metadata.json drift (pure fetch-mechanism refactor, no descriptor-shape change).
  • Full local gate green: npm run rees:test and the complete npm run test:ci (all steps, including the engine-parity/coverage steps called out as a specific risk for this change) both pass.

Closes #4759

… onto boundedFetchText (#4759)

doc-comment-drift.ts, exhaustiveness-drift.ts, and complexity-delta.ts each
carried their own private "fetch a file at headSha, bounded/streamed read
capped at 1MB" helper -- near-byte-identical copies of the same logic, none
with a timeout or circuit breaker. duplication-delta.ts (#4741/#4760) already
migrated onto the more mature boundedFetchText (external-fetch.ts): a typed
ok/failure result, a per-endpoint-category circuit breaker, a configurable
timeout, and byte-size capping. Mirror that same fetchFileAtHead call
pattern -- including the options.analysis.fetchText fallback for when an
AnalysisContext is available -- in all three instead of leaving three more
hand-rolled copies to drift further.

doc-comment-drift.ts also switches to the shared githubHeaders() helper in
place of its own inline auth headers, closing the one pre-existing
inconsistency among the three (the other two already used it).

Pure fetch-mechanism migration -- verified byte-faithful via each analyzer's
existing test suite passing unchanged, plus one new test per file exercising
the added options.analysis branch (mirroring duplication-delta.test.ts's own
coverage of that path).
@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 11, 2026
@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 11, 2026
@loopover-orb

loopover-orb Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ Gittensory review result - approve/merge recommended

Review updated: 2026-07-11 01:32:32 UTC

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

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This is a clean, mechanical migration of doc-comment-drift.ts, exhaustiveness-drift.ts, and complexity-delta.ts off their hand-rolled readBoundedText/fetch logic onto the shared boundedFetchText helper, mirroring the already-proven duplication-delta.ts pattern byte-for-byte (same endpointCategory/phase/subcall conventions, same options.analysis?.fetchText fallback). All call sites are updated consistently with the new ScanOptions-based signatures, the existing test suites pass unchanged (confirming byte-faithful behavior including oversized/streamed-cancel/abort-timing edge cases), and one new test per file exercises the previously-uncovered options.analysis branch. The only substantive behavioral nuance — per the test comments, boundedFetchText no longer polls the abort signal inside its own read loop, relying solely on each analyzer's outer post-fetch check — is explicitly called out and tested, so correctness is preserved even though a mid-stream abort no longer short-circuits the read early.

Nits — 6 non-blocking
  • The abort-cancellation timing changed (boundedFetchText reads a mocked stream to completion instead of polling the signal mid-read the way each analyzer's own old readBoundedText did) — behavior is proven correct by the updated tests, but worth confirming external-fetch.ts's real (non-mocked) stream reader doesn't waste meaningful bytes/CPU on a large real response before the outer check discards it.
  • GITHUB_API and the raw `https:​//github.com/ghapi` string are now duplicated across four analyzer files (three touched here plus duplication-delta.ts) with no shared constant — acceptable per the PR's explicit 'no new shared module' scope, but worth a follow-up extraction issue.
  • The PR description's Summary doesn't explicitly say 'part of Extract shared GitHub-contents-at-ref fetch helper in REES #4759' / 'closes Extract shared GitHub-contents-at-ref fetch helper in REES #4759' even though the migrated code comments cite it repeatedly — worth adding an explicit issue link line to the PR body itself for traceability.
  • maxCallsPerCategory is now enforced twice in exhaustiveness-drift.ts (the analyzer's own `fetches`/MAX_FETCHES counter plus boundedFetchText's own maxCallsPerCategory: MAX_FETCHES) — harmless redundancy, but confirm the two caps are intended to stay in lockstep rather than just being copy-paste leftover from duplication-delta.ts.
  • Consider a short follow-up issue to hoist GITHUB_API and MAX_FETCH_BYTES into a small shared const module now that four analyzers duplicate them, without going as far as sharing the fetch wrapper itself.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #4759
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: 48 registered-repo PR(s), 40 merged, 275 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 48 PR(s), 275 issue(s).
Gate result ✅ Passing No configured blocker found.
Linked issue satisfaction

Addressed
All three analyzers (doc-comment-drift.ts, exhaustiveness-drift.ts, complexity-delta.ts) have their private readBoundedText helpers deleted and now route through the existing boundedFetchText (via options.analysis.fetchText when available, falling back to boundedFetchText directly), matching duplication-delta.ts's reference pattern with no new shared module introduced; doc-comment-drift.ts also sw

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: not available
  • Official Gittensor activity: 48 PR(s), 275 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
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

@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.

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

@loopover-orb
loopover-orb Bot merged commit 0cad5ec into main Jul 11, 2026
11 checks passed
@loopover-orb
loopover-orb Bot deleted the refactor/4759-rees-boundedfetchtext-migration branch July 11, 2026 01:32
loopover-orb Bot pushed a commit that referenced this pull request Jul 11, 2026
…fetch onto boundedFetchText (#4947)

undocumented-export.ts, unused-export.ts, and caller-impact.ts each still hand-rolled their own
private readBoundedText + fetchFileAtHead wrapper for fetching a changed file's content at headSha,
the same pattern doc-comment-drift.ts, exhaustiveness-drift.ts, and complexity-delta.ts had before
#4759/PR #4821 migrated them onto the shared boundedFetchText helper. undocumented-export.ts also
hand-built its own GitHub auth headers inline instead of the shared githubHeaders() helper, the same
inconsistency doc-comment-drift.ts had before that PR.

Deletes each file's private readBoundedText outright (never relocated), replaces the fetch path with
boundedFetchText / options.analysis.fetchText (mirroring duplication-delta.ts's own fetchFileAtHead),
and switches undocumented-export.ts onto githubHeaders(). unused-export.ts and caller-impact.ts
already used githubHeaders() and only needed the boundedFetchText migration. No new shared module.

Closes #4824
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.

Extract shared GitHub-contents-at-ref fetch helper in REES

1 participant