Skip to content

test(github): verify installation-token isolation under multi-tenant concurrency - #6277

Merged
loopover-orb[bot] merged 1 commit into
mainfrom
review/rent-a-loop-credential-isolation-tests
Jul 16, 2026
Merged

test(github): verify installation-token isolation under multi-tenant concurrency#6277
loopover-orb[bot] merged 1 commit into
mainfrom
review/rent-a-loop-credential-isolation-tests

Conversation

@JSONbored

@JSONbored JSONbored commented Jul 16, 2026

Copy link
Copy Markdown
Owner

Summary

Rent-a-Loop's #4794 ("Scoped credential handling") asked for verified, documented behavior that one tenant's credentials can never be used against another tenant's repo. Read the existing installation-token cache/mint layer (src/github/app.ts) end to end before writing anything:

  • installationTokenCache (in-isolate Map) and the external InstallationTokenStore interface (Redis, self-host fleet) are both keyed correctly by installationId throughout.
  • inFlightMints (the single-flight coalescing map for cold-cache concurrent mints) is also keyed by installationId — no shared key collision risk.
  • Every queue-processor call site (src/queue/processors.ts) threads installationId as a local variable or job-payload field, never through module-level/shared mutable state (grepped every non-cache let/var at module scope across src/github/*.ts, src/queue/*.ts, src/review/*.ts — the only two found were an unrelated store-reference pointer and a process-wide REES-auth flag, neither tenant-scoped).

No code changes were needed — the mechanism was already sound. Added the adversarial tests that actually prove it, since none of the existing token tests exercised more than one installation concurrently:

  • Two tenants racing on a cold in-isolate cache each single-flight to exactly one mint of their own installation, zero cross-contamination, warm-cache re-reads confirmed isolated too.
  • Same proof against the external (Redis self-host fleet) token store, since that's the shared-across-replicas path where a real key-collision bug would matter most.

Also fixes a stale addBlockedBy mutation field name in the contributor-gardening skill's reference doc (blockedByIdblockingIssueId), caught while wiring #5669's relationship graph in an earlier session pass — never committed until now.

Closes #4794 — this PR is exactly its "Verified, documented behavior (with tests)" deliverable.

Test plan

  • npx tsc --noEmit clean
  • npm run test:coverage (unsharded): 894 files / 17152 tests passed, 0 failures
  • npm audit --audit-level=moderate: 0 vulnerabilities
  • npm run test:ci (full local gate incl. drift checks, engine parity, MCP/miner packs, UI build): green, verified via literal captured exit code, not a piped one
  • Rebased onto latest origin/main (picked up the concurrent fumadocs-mdx docs migration cleanly, npm ci + fumadocs-mdx source.config.ts codegen re-run), no conflicts

…enant concurrency (#4794)

Adds adversarial tests proving the credential-scoping mechanism Rent-a-Loop's
#4794 asked to verify already holds: two tenants racing on a cold cache each
single-flight to exactly one mint of their own installation, with zero
cross-contamination, in both the in-isolate Map and the external (Redis
self-host fleet) token store. Confirmed no code changes were needed -- the
cache/mint layer and every queue-processor call site already threads
installationId per-job, never through shared/ambient state.

Also fixes a stale addBlockedBy mutation field name in the contributor
gardening skill's reference doc (blockedById -> blockingIssueId), caught
while wiring #5669's relationship graph.
@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 16, 2026
@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

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

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6277   +/-   ##
=======================================
  Coverage   95.35%   95.35%           
=======================================
  Files         598      598           
  Lines       47127    47127           
  Branches    14994    14994           
=======================================
  Hits        44940    44940           
  Misses       1463     1463           
  Partials      724      724           
Flag Coverage Δ
shard-1 43.97% <ø> (-0.08%) ⬇️
shard-2 36.78% <ø> (+0.19%) ⬆️
shard-3 32.35% <ø> (+0.01%) ⬆️
shard-4 33.76% <ø> (-0.61%) ⬇️
shard-5 31.61% <ø> (-0.19%) ⬇️
shard-6 45.21% <ø> (+0.30%) ⬆️

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

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

loopover-orb Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-16 00:27:55 UTC

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

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This is a pure test-addition PR that adds two adversarial concurrency tests proving the existing installationId-keyed cache/mint layer never cross-contaminates two tenants' tokens, both for the in-isolate cache and the external Redis-backed store, plus a one-line doc fix for a stale GraphQL field name. The tests exercise a genuine concurrent race (Promise.all across two tenants with 5 interleaved callers each) against a fetch mock keyed by installationId, and the assertions (single mint per tenant, no cross-tenant token strings, warm-cache re-reads) actually validate the isolation claim rather than fabricating an unreachable scenario. No production code is touched, matching the PR's claim that no fix was needed — this is documentation-via-tests for #4794.

Nits — 5 non-blocking
  • The second test (test/unit/github-app.test.ts) doesn't assert on mint counts to confirm single-flight coalescing under concurrency the way the first test does — consider adding a mintsByInstallation-style counter there too for symmetry.
  • The inline comments in the new test blocks are fairly long for the stated no-comments convention seen elsewhere in the file; consider trimming to the non-obvious parts only.
  • Consider adding a negative-path assertion that TENANT_A's token never equals any TENANT_B token value (not just prefix-based), to guard against a future format change silently breaking the prefix check.
  • Since this PR closes Scoped credential handling #4794, link the issue number directly in the test description strings is already done well — keep this pattern for future adversarial-proof PRs.
  • 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.

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 #4794
Related work ⚠️ 1 scoped overlap Top overlaps are listed below; lower-confidence bulk is hidden.
Change scope ❌ 8/20 High 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: 42 registered-repo PR(s), 34 merged, 401 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 42 PR(s), 401 issue(s).
Improvement ℹ️ Insufficient signal risk: clean · value: insufficient-signal · LLM: moderate
Linked issue satisfaction

Partially addressed
The PR adds solid concurrency tests proving the installation-token cache/mint layer never leaks tenant A's token to tenant B's calls, which supports the issue's deliverable, but the acceptance criterion specifically asks for a test where tenant A's execution context attempts to act on tenant B's repo and fails — these tests verify token issuance isolation rather than an actual attempted cross-tena

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: 42 PR(s), 401 issue(s).
  • Related work: Titles/paths share 4 meaningful terms. (issue #4794, issue #4791)
Contributor next steps
  • Start here: Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Then work through the remaining 3 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://gittensory.aethereal.dev/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 6db3683 into main Jul 16, 2026
17 checks passed
@loopover-orb
loopover-orb Bot deleted the review/rent-a-loop-credential-isolation-tests branch July 16, 2026 00:28
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.

Scoped credential handling

1 participant