Skip to content

docs(miner): AMS auth/identity research — hosted login-layer options - #5763

Closed
lourincedaging0-commits wants to merge 2 commits into
JSONbored:mainfrom
lourincedaging0-commits:docs-ams-auth-research
Closed

docs(miner): AMS auth/identity research — hosted login-layer options#5763
lourincedaging0-commits wants to merge 2 commits into
JSONbored:mainfrom
lourincedaging0-commits:docs-ams-auth-research

Conversation

@lourincedaging0-commits

Copy link
Copy Markdown
Contributor

Closes #5217.

What

A research-spike comparison document (packages/loopover-miner/docs/ams-auth-identity-research.md) surveying auth/identity approaches for a hosted, multi-tenant AMS — evaluated against the GitHub App / installation-token patterns gittensory already operates, per the issue.

Grounding (existing precedent, cited not changed)

  • src/orb/broker.ts (Orb: fleet calibration collector + analytics (retire per-instance App; feed from review_audit) #1255) — the das-github-mirror token-broker: enrollment-secret → short-lived GitHub installation tokens, installation_id bound server-side at issue time (never from the request).
  • src/orb/oauth.ts — maintainer-OAuth self-enrollment proving installation-account admin server-side.
  • src/orb/broker-client.ts, src/github/app.ts (token cache), src/auth/security (opaque-token/hash primitives).
  • packages/loopover-miner — the current GITHUB_TOKEN-env, no-login baseline.

Comparison (2-3 approaches, with the required broker-interaction notes)

Option Broker interaction
1. GitHub OAuth + existing self-enrollment (recommended) reuse unchanged — GitHub is already AMS's authority
2. Managed IdP (Auth0/Clerk/WorkOS) alongside reuse (fed by an identity↔installation linking step); earns its keep only for non-GitHub/SSO tenants
3. Custom JWT/OIDC reuse (run alongside); most to build + secure — not recommended

In every option the broker's server-side installation_id binding is reused, not replaced (the token path is orthogonal to tenant login).

Scope

Docs-only, per the issue boundaries — no auth flow/token-exchange/provider integration implemented or decided; no change to broker.ts; recommendation is a non-binding input to the maintainer-owned auth design issue.

…parison

Research spike (JSONbored#5216): compares managed Postgres, Cloudflare D1, and a KV/document store as
replacements for the miner local-store's node:sqlite DatabaseSync, evaluated against AMS's ACTUAL
access patterns (interactive batchClaim transactions, single-statement atomic claims, append-only
ledgers, PRAGMA user_version migrations, the api_base_url composite-key tenant seam, lease
liveness). Recommends reusing ORB's existing SqliteDriver adapter (src/selfhost/d1-adapter.ts +
pg-adapter.ts) with a Postgres lead and D1 alternative; KV excluded as a relational-atomicity
mismatch. Research/writeup only, non-binding on the maintainer-owned storage-design issue.

Closes JSONbored#5216
Research spike (JSONbored#5217): surveys auth/identity approaches for a hosted, multi-tenant AMS, evaluated
against the installation-token / GitHub App patterns gittensory already operates (ORB's broker,
src/orb/broker.ts's das-github-mirror token exchange + src/orb/oauth.ts maintainer-OAuth
self-enrollment). Compares (1) GitHub OAuth reusing the existing self-enrollment + broker, (2) a
managed identity provider alongside the broker, (3) a custom JWT/OIDC scheme, each with security +
integration tradeoffs and explicit reuse/replace/alongside notes for the broker's token exchange.
Recommends GitHub-OAuth-as-identity reusing the broker, IdP only for non-GitHub tenants, custom
JWT/OIDC avoided. Research/writeup only, non-binding, no auth code changed.

Closes JSONbored#5217
@superagent-security

Copy link
Copy Markdown
Contributor

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

@loopover-orb loopover-orb Bot added the gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier. label Jul 14, 2026
@loopover-orb

loopover-orb Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Caution

🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥

🛑 LoopOver review result - reject/close recommended

Review updated: 2026-07-14 12:49:15 UTC

2 files · 1 AI reviewer · 1 blocker · readiness 95/100 · CI green · clean

🛑 Suggested Action - Reject/Close

Review summary
This is a docs-only research-spike PR, but the diff actually adds two separate documents — `ams-auth-identity-research.md` (auth/identity options, tied to #5217 as the description states) and `ams-storage-abstraction-research.md` (storage-backend options, explicitly scoped to a different issue, #5216, per its own header). The description and title only describe and link the auth/identity research and #5217; the storage-abstraction doc is undisclosed scope-creep bundled into the same PR. Both documents are well-grounded, cite existing ORB precedent (`broker.ts`, `oauth.ts`, `d1-adapter.ts`, `pg-adapter.ts`) accurately per the provided reference code, and are explicit that they're non-binding research with no code/schema/auth changes.

Blockers

Nits — 5 non-blocking
  • Both docs cite specific line numbers (e.g. `broker.ts:8`, `:30`, `pg-adapter.ts:56`) for precedent code — worth double-checking these stay accurate as those files evolve, since a stale line-anchor in a research doc that later informs an implementation issue could mislead.
  • The auth doc's Option 2 (managed IdP) and storage doc's Option A (Postgres) both note nontrivial new operational/linking-table cost — consider a short 'open questions for the design issue' bullet list at the end of each doc to make the maintainer hand-off actionable rather than purely descriptive.
  • Either split the storage-abstraction doc into its own PR that explicitly links Research spike: storage-abstraction candidates to replace AMS's node:sqlite local-store #5216, or update this PR's description to state both issues it's addressing and why they're combined.
  • If combining research spikes for related issues is intentional going forward, consider a convention (e.g. `docs(miner): AMS research spikes — auth/identity (Research spike: auth/identity provider comparison for hosted AMS #5217) + storage (Research spike: storage-abstraction candidates to replace AMS's node:sqlite local-store #5216)`) so the title/description always matches the full diff.
  • Diff looks like trivial or whitespace-only churn — Reduce whitespace-only or formatting-only churn and keep the diff focused on substantive changes.

Why this is blocked

📋 Copy for AI agents — paste into your coding agent
Fix the following blocker(s) from this PR review:

1. The diff bundles a second research document \(\`ams-storage-abstraction-research.md\`, scoped to \#5216\) that is not mentioned anywhere in the PR title or description, which only claims to close \#5217 — per repo convention every PR must close/link an eligible issue for everything it contains, and this one is silent on the \#5216 half.
Signal Result Evidence
Code review ❌ 1 blocker 1 reviewer
Linked issue ✅ Linked #5217
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 ⚠️ 20/25 Preflight is ready, but the PR body does not name the validation run.
Contributor workload ✅ 10/10 Author activity: 0 registered-repo PR(s), 0 merged, 0 issue(s).
Contributor context ❌ No public Gittensor match lourincedaging0-commits; not a blocker.
Gate result ❌ Blocking Repo-configured hard blocker found.
Improvement ⚠️ ℹ️ Insufficient signal risk: low · value: insufficient-signal — Nothing measurable for the structural-improvement analyzers on this PR (e.g. no code files changed). LLM value judgment: moderate — Both documents are substantive, well-grounded research spikes that correctly trace existing ORB precedent and give the maintainer-owned design issues a concrete, non-binding starting point, though the undisclosed bundling of two issues' worth of research under one PR undercuts the otherwise clean scope.
Review context
  • Author: lourincedaging0-commits
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: HTML
  • Contributor context: Public profile only; not a blocker.
  • PR-specific overlap: none found.
Contributor next steps
  • Add validation command/output.
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.
[BETA] Chat with Gittensory

Ask Gittensory a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @gittensory ask <question> answers contribution-quality Q&A with source citations and freshness.
  • @gittensory chat <question> answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @gittensory 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/gittensory-commands

🟩 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 commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Gittensory is closing this pull request on the maintainer's behalf (AI reviewers agree on a likely critical defect: The diff bundles a second research document (`ams-storage-abstraction-research.md`, scoped to #5216) that is not mentioned anywhere in the PR title or description, which only claims to close #5217 — per repo convention every PR must close/link an eligible issue for everything it contains, and this one is silent on the #5216 half.). This is an automated maintenance action — to pursue this change, please open a new pull request with the issues resolved. Closed PRs may be analyzed later to improve review accuracy, but they are not automatically reopened or re-reviewed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Research spike: auth/identity provider comparison for hosted AMS

1 participant