fix: reapply mint.fullsend.sh as default hosted mint URL - #6290
Conversation
|
🤖 Review · Commit: |
PR Summary by QodoReapply mint.fullsend.sh as default hosted mint URL
AI Description
Diagram
High-Level Assessment
Files changed (12)
|
Code Review by Qodo
1. Hardcoded Cloud Run mint URL
|
a93dfac to
6afd368
Compare
|
🤖 Finished Review · ✅ Success · Started 1:39 PM UTC · Completed 1:59 PM UTC Commit: |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsLow
Previous run (3)ReviewFindingsHigh
Low
Next steps:
Previous run (4)ReviewFindingsMedium
Low
Labels: PR modifies mint URL defaults, validation, and documentation across cli, config, e2e, and docs packages |
waynesun09
left a comment
There was a problem hiding this comment.
Additional findings on files not touched by this PR's diff (GitHub won't allow inline comments on unchanged lines), posted here instead:
[HIGH] internal/dispatch/gcf/provisioner.go:670 — validateMintURL now accepts mint.fullsend.sh but GCF provisioner still rejects it
Confirmed on HEAD (6afd368): internal/cli/admin.go's validateMintURL (admin.go:217-225) now accepts host mint.fullsend.sh via strings.EqualFold, matching this PR's intent. However internal/dispatch/gcf/provisioner.go — not touched by this PR — still hard-requires the mint host to end in .run.app or .cloudfunctions.net in two separate places (provisionWithExistingMint at line 670: "MintURL %q must be a valid Cloud Run URL (.run.app or .cloudfunctions.net)", and again at line 1009: "function URL %q is not a valid Cloud Run URL"). Since DefaultMintURL is now https://mint.fullsend.sh, any code path that reaches provisionWithExistingMint with the new default (e.g. fullsend admin install without --skip-mint-check) will pass CLI-level validation and then fail deep inside the provisioner with a confusing, unrelated error message.
Suggestion: Add a mint.fullsend.sh exception to both provisioner.go host checks (ideally via cli.IsHostedMintURL so the allowlists can't diverge again), or explicitly document that installs against the hosted community mint require --skip-mint-check.
[MEDIUM] docs/guides/getting-started/configuring-github.md:11 — Getting-started docs edit leaves a stale "step 1" cross-reference in an untouched doc
Confirmed via diff against origin/main: this PR removes the numbered "Mint enrollment" step 1 from docs/guides/getting-started/README.md, renumbering "Getting Inference" as step 1. docs/guides/getting-started/configuring-github.md (not touched by this PR) still says on line 11: "Your org or repo is enrolled in a fullsend token mint service (see Getting Started step 1)." Step 1 is now "Getting Inference", which has nothing to do with mint enrollment, so this cross-reference now points to the wrong content and undermines the PR's own stated goal of fixing the getting-started user journey.
Suggestion: Update configuring-github.md's prerequisite bullet to remove the stale "see Getting Started step 1" reference or rewrite it to match the no-enrollment-required hosted community mint flow this PR introduces.
|
/fs-plan-tests |
|
🤖 Finished Qualityflow · ✅ Success · Started 7:18 AM UTC · Completed 7:27 AM UTC Commit: |
Generated 21 passing tests across 3 packages: - internal/cli: 15 tests (validateMintURL, IsHostedMintURL, DefaultMintURL) - internal/config: 1 test (DefaultPerRepoMintURL) - pkg/e2etest: 5 tests (MintEnrollProjectID, DefaultPoolOrgInstallMintURL)
|
🤖 Review · Commit: |
4db5569 to
6afd368
Compare
|
🤖 Review · Commit: |
6afd368 to
d557b83
Compare
|
🤖 Review · Commit: |
d557b83 to
2400e8e
Compare
|
🤖 Review · ❌ Terminated · Started 7:58 AM UTC · Ended 8:16 AM UTC Commit: |
Site previewPreview: https://836905ba-site.fullsend-ai.workers.dev Commit: |
|
🤖 Finished Review · ✅ Success · Started 7:58 AM UTC · Completed 8:16 AM UTC Commit: |
2400e8e to
3cf82e1
Compare
|
🤖 Review · ❌ Terminated · Started 9:11 AM UTC · Ended 9:27 AM UTC Commit: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Superseded by updated review
|
🤖 Finished Review · ✅ Success · Started 9:11 AM UTC · Completed 9:27 AM UTC Commit: |
3cf82e1 to
e6a7569
Compare
|
🤖 Finished Review · ✅ Success · Started 10:26 AM UTC · Completed 10:44 AM UTC Commit: |
Reapply changes from PR #6261 that were silently reverted by merge df491ae (#6257): update DefaultMintURL and DefaultPerRepoMintURL to https://mint.fullsend.sh, add IsHostedMintURL() helper, update validateMintURL() and provisioner host checks to accept the new hostname, add DefaultPoolOrgInstallMintURL for admin e2e tests, and replace stale Cloud Run URL references in docs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Hector Martinez <hemartin@redhat.com>
e6a7569 to
4ee7cec
Compare
|
🤖 Finished Review · ✅ Success · Started 11:41 AM UTC · Completed 12:00 PM UTC Commit: |
| (!strings.EqualFold(host, "mint.fullsend.sh") && | ||
| !strings.HasSuffix(host, ".run.app") && | ||
| !strings.HasSuffix(host, ".cloudfunctions.net")) { | ||
| return nil, fmt.Errorf("function URL %q is not a valid Cloud Run URL", mintURL) |
There was a problem hiding this comment.
[low] error-message-inconsistency
Error messages in provisionSelfManaged say 'function URL is not a valid Cloud Run URL' but validation now accepts mint.fullsend.sh. The provisionWithExistingMint block was updated but provisionSelfManaged retains the old wording in both error branches (lines 1012 and 1019).
Suggested fix: Update both error format strings to match the other validation sites.
| if !strings.HasSuffix(host, ".run.app") && | ||
| !strings.HasSuffix(host, ".cloudfunctions.net") { | ||
| return fmt.Errorf("--mint-url must be a Cloud Run URL (.run.app or .cloudfunctions.net), got host %q", host) | ||
| if strings.EqualFold(host, "mint.fullsend.sh") || |
There was a problem hiding this comment.
[low] DRY-internal-reuse
validateMintURL duplicates the mint.fullsend.sh hostname check that IsHostedMintURL already encapsulates.
Suggested fix: Consider extracting the hostname comparison into a helper that takes a pre-parsed hostname.
| mintURL = cli.DefaultMintURL | ||
| } | ||
| if mintURL == cli.DefaultMintURL { | ||
| if isPoolOrgMintURL(mintURL) || cli.IsHostedMintURL(mintURL) { |
There was a problem hiding this comment.
[low] authorization-scope
MintEnrollProjectID() broadened detection from exact string equality to matching either legacy Cloud Run URL or mint.fullsend.sh hostname. Impact limited to e2e test infrastructure.
|
🤖 Finished Retro · ✅ Success · Started 12:03 PM UTC · Completed 12:12 PM UTC Commit: |
Retro: PR #6290 — reapply mint.fullsend.sh as default hosted mint URLWhat happenedTwo agent-created PRs touched overlapping files and merged within 2 hours of each other, causing a silent revert:
Review qualityThe review agent correctly identified several issues: the Evidence for existing issues
Proposals filed
|
Summary
DefaultMintURLandDefaultPerRepoMintURLtohttps://mint.fullsend.sh, addIsHostedMintURL()helper, updatevalidateMintURL()to accept the new hostname, and addDefaultPoolOrgInstallMintURLfor admin e2e testsmint.fullsend.shand remove enrollment-required language for the hosted community mintmint.fullsend.sh, which may still gate on enrollmentTest plan
go test ./internal/cli/ ./internal/config/ ./pkg/e2etest/all passTestValidateMintURL_Accepts*,TestDefaultMintURL_IsHostedCommunity,TestIsHostedMintURL(including port and case edge cases)fullsend-mint-gljhbkcloqreferences outsideDefaultPoolOrgInstallMintURL🤖 Generated with Claude Code