docs: public community mint ADR and hosting spike (#915) - #1413
Conversation
Site previewPreview: https://686b6a9e-site.fullsend-ai.workers.dev Commit: |
ADR 0040 records deployment, security, monitoring, scaling, and operations for the community mint profile; the spike holds platform trade-offs from issue fullsend-ai#915. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Barak Korren <bkorren@redhat.com>
Focus Decision on steady-state Cloudflare Workers, defer automated PEM rotation to a future ADR, and align options with ADR 0029 goal. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Barak Korren <bkorren@redhat.com>
42c3fe7 to
5e9cca5
Compare
ReviewDocs-only PR adding ADR 0068 (public community mint architecture) and a hosting comparison spike for #915. The ADR is technically sound — all cross-referenced ADRs (0029, 0044, 0054, 0059, 0060) exist and claims about No security findings. No correctness issues with the technical content. FindingsMedium
Low
Previous runReviewDocs-only PR adding ADR 0068 (public community mint architecture) and a hosting comparison spike for #915. The ADR is technically sound — all cross-referenced ADRs (0029, 0044, 0054, 0059, 0060) exist and claims about No security findings. No correctness issues with the technical content. FindingsMedium
Low
Previous runReviewDocs-only PR adding ADR 0068 (public community mint architecture) and a hosting comparison spike for #915. The ADR is technically sound — all cross-referenced ADRs (0029, 0044, 0054, 0059, 0060) exist and claims about No security findings. No correctness issues with the technical content. FindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsHigh
Medium
Low
Labels: PR modifies mint infrastructure code and adds ADR/spike documentation Previous run (5)ReviewFindingsMedium
Low
Previous run (6)Looks good to me Previous run (7)ReviewFindingsCritical
Medium
Previous run (8)ReviewFindingsMedium
Labels: PR adds ADR and spike documentation for the token mint component Previous run (9)ReviewFindingsLow
Info
Previous run (10)ReviewFindingsCritical
Medium
Previous run (11)ReviewFindingsMedium
Labels: PR adds ADR and spike documentation for the token mint component Previous run (12)ReviewFindingsLow
Info
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
ready-for-merge
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review: ADR 0040 + Community Mint Hosting Spike
The hosting analysis and ADR structure are solid. The phasing (GCP interim → Workers steady-state), cost analysis, and single-console ops rationale are well-argued. Four implementation-facing gaps should be addressed before acceptance.
1. job_workflow_ref validation strategy when WIF is gone (High)
The ADR's OIDC validation section (Security §2) lists job_workflow_ref as a fail-closed check, carried over from today. Today's prevalidateOIDCToken() in internal/mint/main.go (lines 597–610) accepts three job_workflow_ref patterns:
{org}/.fullsend/...— org config repofullsend-ai/fullsend/...— upstream reusable workflows{owner}/{repo}/...— only if repo is inPER_REPO_WIF_REPOS
PER_REPO_WIF_REPOS today serves dual duty: WIF provider routing AND job_workflow_ref validation. When WIF goes away in the Workers port, the WIF routing purpose disappears — but the ref validation role remains. The ADR should clarify:
- Does
PER_REPO_WIF_REPOS(or an equivalent) survive as a per-repo allowlist for custom workflows? - If yes, it's still a per-repo enrollment list that requires mint-side updates — contradicting the "opaque URL" goal for adopters.
- If no (org-level trust: any repo in an enrolled org can mint if the workflow filename is in
ALLOWED_WORKFLOW_FILES), the security implications should be documented.
This matters for custom agent workflows — users bringing their own workflow files (not calling upstream reusable workflows) will hit Case 3 and need explicit registration unless the validation is relaxed.
2. Enrollment simplification for shared apps (Medium)
The ADR inherits ALLOWED_ORGS, per-org ROLE_APP_IDS entries, and per-org PEM copies from the current architecture. For shared public apps:
ROLE_APP_IDS: The App ID is identical across all orgs for the same shared app. The current{org/role: appId}keying could simplify to{role: appId}— static config, no per-org updates.- PEM secrets: Same PEM key copied under per-org naming (
fullsend-{org}--{role}-app-pem). Could be one secret per role. ALLOWED_ORGS: Could be replaced by the natural enrollment gate —findInstallation()(line 715) returns 404 if the app isn't installed on the repo. Installing the shared GitHub App IS enrollment.
The ADR doesn't need to mandate this simplification, but should acknowledge it as a consequence of the shared app model. It affects the Worker secrets layout (fewer secrets) and whether #914 enrollment requires mint-operator involvement at all.
3. Per-repo trust routing without WIF (Low)
The ADR mentions "per-repo policy routing (parity with today's GCF behavior)" in Security §2. Today, per-repo routing means resolveWIFProvider() picks a dedicated WIF provider for repos in PER_REPO_WIF_REPOS. In the JWKS world, there's no WIF provider to route to. What's the equivalent? Is it just the job_workflow_ref Case 3 check, or is there additional per-repo config?
4. Spike: clarify OIDC audience handling in JWKS path (Low)
The spike's STS→JWKS section explains signature verification clearly but doesn't mention aud (audience) validation. Today the OIDC_AUDIENCE check is in application code (line 556), not in WIF — so it carries over unchanged. Worth a sentence confirming this so readers don't assume audience validation was a WIF-side responsibility.
What's well-covered
- Trust model aligned with ADR 0029
- JWKS parity requirement (prove STS ≡ JWKS in CI)
- Isolation from inference/internal infrastructure
- GitOps deployment requirement
- PEM rotation deferral to future ADR (appropriate scoping)
- Blast radius analysis
- Cost at ~$0 community budget
- Single-console ops rationale for preferring Scenario 1 or 3 over 2
Recommendation
The hosting decisions and architecture are sound. The gaps above are about documenting the JWKS-world implications for mint-side enrollment config and per-repo custom workflow support. Items 1–2 should be addressed in the ADR text (even as acknowledged open questions or deferred to #914 implementation). Items 3–4 are minor clarifications.
|
How is this going @ifireball @waynesun09 ? Do recent changes impact this? |
Signed-off-by: Barak Korren <bkorren@redhat.com>
Renumber to ADR 0061 (0040 taken by org-pool ADR), merge upstream/main, cross-reference ADR 0059/0060/0044, and fold in trust-policy decisions that supersede the original OIDC/enrollment wording. Update spike, README, and architecture.md; annotate ADR 0059 deferred items now covered here. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Rebased on
|
| Original ADR content | Status after main |
|---|---|
Security §2: three-way job_workflow_ref + PER_REPO_WIF_REPOS for custom workflows |
Superseded by ADR 0059 — public hosted mint is upstream-only (fullsend-ai/fullsend/.github/workflows/); PER_REPO_WIF_REPOS is tight-mode only |
| "Org + per-repo install modes" on same public mint | Revised — per-org .fullsend deprecated (ADR 0044); public profile targets per-repo installs + upstream reusables |
Per-org ROLE_APP_IDS / per-org PEM copies |
Removed — already decided in ADR 0059 §8 (global per-role credentials) |
ALLOWED_ORGS as enrollment gate |
Replaced — hosted mint uses ALLOWED_ORGS=*; installing shared Apps is enrollment (#1145) |
internal/mint as sole code reference |
Updated — internal/mintcore/ is the shared library; GCF (internal/mint/), standalone JWKS (cmd/mint/), future Workers all use it |
../guides/admin/infrastructure-reference.md |
Fixed path — moved to docs/guides/infrastructure/ |
| ADR scope: trust policy + infrastructure mixed | Split — ADR 0059 = trust policy; ADR 0061 = deployment/ops/edge (fulfills items deferred from 0059) |
Still valid unchanged: GCP interim → Workers steady-state phasing, ~$0 cost analysis, isolation from inference, GitOps (#1263), PEM rotation deferred to future ADR, Bootstrap SLO ownership.
Also added docs/spikes/ to README index (review bot note).
@waynesun09 — addressing your review items:
-
job_workflow_ref/PER_REPO_WIF_REPOSwhen WIF is gone — Resolved by cross-referencing ADR 0059: public mode does not keep per-repo custom workflow allowlists; provenance is upstream-only.PER_REPO_WIF_REPOSremains for tight/self-managed mints only. Documented explicitly in ADR 0061 "Trust and enrollment" section. -
Enrollment simplification for shared apps — Resolved in ADR 0059 (global
ROLE_APP_IDS, global PEMs,ALLOWED_ORGS=*, App install = enrollment). ADR 0061 now states hosted mint will use public mode and links feat: default --mint-url to fullsend-ai hosted mint and skip provisioning by default #1145. -
Per-repo trust routing without WIF — In public mode there is no per-repo routing equivalent; JWKS path uses the same
mintcoreclaim checks with emptyPER_REPO_WIF_REPOS. Clarified in ADR 0061 + spike. -
audin JWKS path — Added to spike OIDC section:OIDC_AUDIENCEis validated inmintcoreon both STS and JWKS verifiers, not by WIF.
@rh-hemartin — yes, recent main changes materially affected this PR; see table above. ADR 0059/0060/0044 and mintcore refactor are the big ones.
Signed-off-by: Barak Korren <bkorren@redhat.com>
ADR 0061 was taken by harness-cel-dispatch on main. Merge latest upstream/main and renumber the community mint architecture ADR. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · ❌ Terminated · Started 5:41 AM UTC · Ended 5:52 AM UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
fullsend-ai-review
Bot
added
requires-manual-review
Revert frozen ADR 0059 sections to main. Document Workers PEM secret size (~1,675-byte PEMs fit the 5 KB limit), link automated rotation to fullsend-ai#4175, and warn in mintcore when PEMs exceed the Workers limit. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 6:56 AM UTC · Completed 7:15 AM UTC |
fullsend-ai-review
Bot
added
requires-manual-review
Renumber community mint ADR 0063→0068 (0063 taken by polling-based work discovery on main). Resolve VitePress sidebar conflict keeping Spikes. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · ❌ Terminated · Started 5:49 AM UTC · Ended 6:06 AM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
removed
the
requires-manual-review
Revert mintcore/cmd/mint PEM size warnings added in ccdb32a. Workers 5 KB limit and current ~1,675-byte PEMs stay documented in ADR 0068. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 7:47 AM UTC · Completed 8:03 AM UTC |
Superseded by updated review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
requires-manual-review
Signed-off-by: Barak Korren <bkorren@redhat.com>
Babysit update (Jul 14)Merged latest May 26 review (@waynesun09) — addressed in current ADR/spike
Jul 7 Review Squad — all threads resolved
CI green on |
|
🤖 Finished Review · ✅ Success · Started 5:55 AM UTC · Completed 6:07 AM UTC |
fullsend-ai-review
Bot
added
requires-manual-review
Resolve README conflict by taking main's simplified docs pointer. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 9:28 PM UTC · Completed 9:39 PM UTC |
Stale on 5e9cca5; addressed in ADR 0068.
fullsend-ai-review
Bot
added
requires-manual-review
4 participants
Summary
Test plan
Closes #915 (spike deliverable). Supports #914 / #912.
Made with Cursor