Skip to content

perf(test): memoize the shared GitHub App test keypair and cap rag-index's fixture size#8550

Merged
JSONbored merged 1 commit into
mainfrom
perf/test-hotspots
Jul 24, 2026
Merged

perf(test): memoize the shared GitHub App test keypair and cap rag-index's fixture size#8550
JSONbored merged 1 commit into
mainfrom
perf/test-hotspots

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

Second follow-up in the CI-speed sequence (after the TestD1Database template-clone fix and the github/client↔db/repositories import-cycle cut). Profiling the suite's remaining per-test outliers found two concentrated, fixable hotspots — neither requires sharding, and neither touches test semantics:

  • 774 fresh RSA-2048 GitHub App keypairs. ~19 test files each declared their own generatePrivateKeyPem() and called it per test — a real WebCrypto keygen (prime search) whose signature no fetch stub ever verifies; any single valid key works for every fixture. New test/helpers/github-app-key.ts memoizes one keypair per worker process on globalThis (not module scope — vitest's per-file module isolation would otherwise rebuild it once per file, the same lesson test/helpers/d1.ts's template memo already learned). github-app.test.ts's key-rotation tests genuinely need distinct keys per call and keep their own local, non-memoized generator — left untouched.
  • rag-index.test.ts's cap tests built up to 4,000 real chunk rows to prove MAX_CHUNKS_PER_REPO-capping behavior, not to validate the number 4000 itself. src/review/rag.ts gains maxChunksPerRepo() / setMaxChunksPerRepoForTest() — the same ...ForTest override convention as clearInstallationTokenCacheForTest / clearGitHubResponseCacheForTest — and rag-index.ts's three cap-read sites switch from the raw constant to the getter. Production is unaffected (the override is null unless a test arms it). The test file runs under a 24-row cap; no other fixture in it indexes anywhere near that many files, so unrelated test semantics are unchanged. Added one direct test asserting the getter falls back to the real 4000 when unarmed.

Measured: rag-index.test.ts 46s → 1.6s (28×). The keygen fix's effect is spread thin across ~19 files rather than concentrated in one, but removes real, pointless CPU work (prime search) at ~774 call sites.

Validation

  • npm run typecheck clean.
  • npm run test:coverage (full, unsharded): 21,483 passed / 0 failed.
  • npx vitest run test/unit/rag-index.test.ts — 52 passed, including the new default-cap assertion.

…dex's fixture size

19 test files each generated a fresh RSA-2048 GitHub App keypair per call
(~774 sites) for JWTs no fetch stub ever verifies -- any single valid key
works for every fixture. test/helpers/github-app-key.ts memoizes one keypair
per worker process on globalThis (not module scope -- vitest's per-file
module isolation would rebuild it once per file otherwise, the same lesson
test/helpers/d1.ts's template memo already learned). github-app.test.ts's
key-rotation tests genuinely need distinct keys and keep their own local
generator.

rag-index.test.ts's cap tests built up to 4000 real chunk rows to prove
MAX_CHUNKS_PER_REPO-capping BEHAVIOR, not to validate the number itself.
rag.ts gains maxChunksPerRepo()/setMaxChunksPerRepoForTest() (the same
...ForTest override convention as clearInstallationTokenCacheForTest); the
test file runs under a 24-row cap. rag-index.test.ts: 46s -> 1.6s (28x).
@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 24, 2026
@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.50%. Comparing base (61f6ec6) to head (838fe90).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8550      +/-   ##
==========================================
+ Coverage   89.58%   92.50%   +2.92%     
==========================================
  Files          97      793     +696     
  Lines       22706    79549   +56843     
  Branches     3872    24036   +20164     
==========================================
+ Hits        20341    73587   +53246     
- Misses       2187     4800    +2613     
- Partials      178     1162     +984     
Flag Coverage Δ
backend 93.67% <100.00%> (?)

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

Files with missing lines Coverage Δ
src/review/rag-index.ts 93.86% <100.00%> (ø)
src/review/rag.ts 99.61% <100.00%> (ø)

... and 694 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 24, 2026
@loopover-orb

loopover-orb Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Warning

⏸️ LoopOver review result - manual review recommended

Review updated: 2026-07-24 20:14:04 UTC

22 files · 1 AI reviewer · 3 blockers · CI green · clean

⏸️ Suggested Action - Manual Review

Review summary
This is a mechanical, low-risk test-infra perf PR: it memoizes one throwaway RSA-2048 keypair per worker process on globalThis (test/helpers/github-app-key.ts) and replaces 19 duplicated ad-hoc generatePrivateKeyPem() implementations with imports of it, and it adds a null-defaulting test-only override (maxChunksPerRepo()/setMaxChunksPerRepoForTest) to src/review/rag.ts so rag-index.test.ts's cap tests run against 24 rows instead of 4000. Production code paths in rag-index.ts correctly switch from the raw MAX_CHUNKS_PER_REPO constant to the maxChunksPerRepo() getter at all three cap-read sites, and the getter falls back to the real 4000 constant when the override is unarmed (verified by the added direct test) — the override is genuinely inert unless a test arms it. The key-rotation tests in github-app.test.ts are explicitly left with their own non-memoized generator per the description, which is consistent with needing distinct keys per call.

Nits — 5 non-blocking
  • The external secrets scanner flagged test/helpers/github-app-key.ts:8-9, but these are just the PEM_HEADER/PEM_FOOTER string label constants, not an actual key value — false positive, no action needed.
  • src/review/rag-index.ts now sits at ~401 lines against a 400-line file-length threshold purely from the mechanical MAX_CHUNKS_PER_REPO→maxChunksPerRepo() import/rename churn; not actionable but worth knowing if the repo enforces that threshold.
  • upsertChunksCapped's loop in rag-index.ts calls maxChunksPerRepo() twice per iteration (loop condition + remaining calc) — trivial, but a single local const per iteration would avoid the redundant call.
  • In test/unit/rag-index.test.ts, shadowing the production constant's name with a local `const MAX_CHUNKS_PER_REPO = 24` (per the stated convention of keeping existing test bodies textually unchanged) is a little surprising on first read; a code comment explaining that intent is present, which mitigates this.
  • Consider a lint rule or comment enforcing that new github-app tests import generatePrivateKeyPem from test/helpers/github-app-key.ts rather than re-declaring a local generator, to prevent the same duplication from creeping back in.

Concerns raised — review before merging

  • 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.
  • Possible leaked secret in the diff (private_key_block): The PR diff matches secret pattern(s): private_key_block. Found at: test/helpers/github-app-key.ts:8. A committed credential must be rotated and removed from the change before merge. — Remove the secret from the diff, rotate the exposed credential, then re-run the gate.
📋 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.

3. Possible leaked secret in the diff (private_key_block): The PR diff matches secret pattern(s): private_key_block. Found at: test/helpers/github-app-key.ts:8. A committed credential must be rotated and removed from the change before merge. — Remove the secret from the diff, rotate the exposed credential, then re-run the gate.

Decision drivers

  • ❌ Code review — 3 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: 13 registered-repo PR(s), 13 merged, 272 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 13 PR(s), 272 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
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, MDX, Shell, Solidity, JavaScript
  • Official Gittensor activity: 13 PR(s), 272 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 24, 2026
@JSONbored
JSONbored merged commit 798f32e into main Jul 24, 2026
7 checks passed
@JSONbored
JSONbored deleted the perf/test-hotspots branch July 24, 2026 20:17
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