refactor(#6305): internalize env access via build-tagged mintEnv - #6308
refactor(#6305): internalize env access via build-tagged mintEnv#6308fullsend-ai-coder[bot] wants to merge 2 commits into
Conversation
Replace the injected getEnv func(string) string parameter with a package-internal mintEnv accessor backed by build-tagged files: env.go (!js) delegates to os.Getenv; env_js.go (js) delegates to a JS callback registered once via RegisterEnv during mintcoreInitMint. Remove VerifierFactory; NewHandler now takes a pre-constructed OIDCVerifier. Verifiers (JWKSVerifier, STSVerifier) read OIDC_AUDIENCE internally via mintEnv at construction time, eliminating the Audience field from their config structs. Update all entrypoints (cmd/mint-wasm, cmd/mint, internal/mint) and tests to match the new API. Sync .embed files and register env.go in embeddedMintFiles, env_js.go in gcfSkip. Note: pre-commit could not run (sandbox network policy blocked hook repo fetches). Tests pass: mintcore, internal/mint, cmd/mint, WASM build, embed sync test. Closes #6305
|
🤖 Finished Review · ✅ Success · Started 12:20 AM UTC · Completed 12:37 AM UTC Commit: |
Site previewPreview: https://4c1a05f6-site.fullsend-ai.workers.dev Commit: |
ReviewFindingsLow
Previous runReviewFindingsMedium
Low
Labels: PR modifies mint component Go code (internal/mintcore/, cmd/mint/, cmd/mint-wasm/) |
|
/fs-fix Rework toward issue #6305 (supersedes current PR direction). The WASM bisect shows mintEnv inside heavy constructors regresses to ~11.5 MB raw; niladic factory invoked from NewHandler stays ~7 MB. Target architecture
Verification (must pass before push)
Out of scope (#6306)
|
|
🤖 Finished Fix · ✅ Success · Started 3:18 AM UTC · Completed 3:43 AM UTC Commit: |
Rework the mintEnv PR toward the target architecture from #6305: - Add mintHTTP accessor (http_client.go / http_client_js.go) following the same build-tagged pattern as mintEnv — native returns http.Client, WASM wraps HostFetchDoer via RegisterHTTP - Add niladic VerifierFactory type: func() (OIDCVerifier, error) - Create named wire factories (verifier_jwks_env.go, verifier_sts_env.go) that read OIDC_AUDIENCE via mintEnv and obtain HTTP clients from mintHTTP - Revert WASM-unsafe changes: restore Audience field on JWKSVerifierConfig and STSVerifierConfig so heavy constructors stay pure-data - Simplify NewHandler to (VerifierFactory, PEMAccessor) — handler reads config via mintEnv and gets its HTTP client from mintHTTP internally - Add RegisterEnv validation (fn.Type() == js.TypeFunction) - Simplify all entrypoints to use named factories - Update all tests, embeds, and provisioner embed map Addresses review feedback on #6308
🔧 Fix agent — iteration 1 (human-triggered)Reworked PR toward target architecture from #6305: added mintHTTP build-tagged accessor, niladic VerifierFactory type, named wire factories (NewJWKSVerifierFromEnv, NewSTSVerifierFromEnv), reverted WASM-unsafe mintEnv calls from heavy verifier constructors, simplified NewHandler to (VerifierFactory, PEMAccessor), and updated all entrypoints, tests, and embeds. Fixed (8):
Tests: passedNext steps:
|
|
🤖 Review · ❌ Terminated · Started 3:44 AM UTC · Ended 4:02 AM UTC Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:
internal/mint/main.go(file-level): Line 20 · [low] fail-open
The GCF init() requiredEnvVars list does not include OIDC_AUDIENCE. The system is still fail-closed (the verifier factory rejects empty audience), but the error message changed from the clear startup diagnostic to the wrapped 'creating OIDC verifier: OIDC_AUDIENCE must be configured'. Adding OIDC_AUDIENCE to requiredEnvVars would restore the early, clear diagnostic.
Suggested fix: Add OIDC_AUDIENCE to the requiredEnvVars slice in internal/mint/main.go init().
internal/mintcore/config_test.go:39: [low] test-naming-convention
TestNewHandler_UsesMinHTTP has a typo — 'MinHTTP' should be 'MintHTTP'.
Suggested fix: Rename to TestNewHandler_UsesMintHTTP.
internal/mintcore/http_client.go:18: [low] naming-consistency
mintHTTP() creates a new *http.Client on every call, acting as a factory rather than an accessor like its sibling mintEnv. Consider caching a package-level default client for semantic consistency.
Suggested fix: Initialize a package-level var defaultHTTPClient = &http.Client{Timeout: 30 * time.Second} and have mintHTTP() return that (or the test override).
internal/mintcore/env_js.go(file-level): Line 52 · [low] fail-open
mintEnv in the WASM build silently returns empty string when RegisterEnv has not been called. Defense-in-depth only — the init sequence is correct and the verifier factory fails hard on empty OIDC_AUDIENCE.
internal/mintcore/http_client_js.go:35: [low] fail-open
mintHTTP() in the WASM build returns nil if RegisterHTTP has not been called. The handler path has no nil fallback (would panic on first HTTP request). Defense-in-depth only — the current init sequence calls RegisterHTTP before NewHandler.
internal/mintcore/env.go:11: [low] naming-convention
mintEnv uses a package-name prefix uncommon for unexported functions in internal/mintcore/. The name is intentionally chosen and documented in docs/contributing/go-code.md, distinguishing the build-tagged accessor from the now-deleted getEnv parameter.
|
🤖 Finished Review · ✅ Success · Started 3:44 AM UTC · Completed 4:02 AM UTC Commit: |
|
/fs-fix Rebase onto current main and downscope this PR to static Already on main (do not re-implement)
Current NewHandler(getEnv, pemAccessor, oidcVerifier, httpClient)In scope
Out of scope (revert / do not land)
Verification
|
|
🤖 Finished Fix · ❌ Failure · Started 4:35 PM UTC · Completed 4:57 PM UTC Commit: |
|
The fix agent completed, but the post-fix script failed before finishing. Workflow run: https://github.com/fullsend-ai/.fullsend/actions/runs/32161014462 Details: Please check the workflow logs for full details and retry with |
|
/fs-fix re-try last fix, but this time don't forget to update the embed files as needed |
|
🤖 Finished Fix · ❌ Failure · Started 5:32 PM UTC · Completed 5:58 PM UTC Commit: |
|
The fix agent completed, but the post-fix script failed before finishing. Workflow run: https://github.com/fullsend-ai/.fullsend/actions/runs/32165986067 Details: Please check the workflow logs for full details and retry with |
|
Since fix is constantly failing, will rewrite the issue with what we learned from working on this PR and start from scratch. |
|
🤖 Finished Retro · ❌ Failure · Started 6:15 PM UTC · Completed 6:15 PM UTC Commit: |
Summary
mintEnvaccessor (env.gofor native,env_js.gofor WASM) so any code ininternal/mintcore/can read environment variables without constructor injection offunc(string) stringVerifierFactorytype and thegetEnvparameter fromNewHandler; handler now accepts a pre-constructedOIDCVerifierand reads config viamintEnvNewJWKSVerifierandNewSTSVerifierreadOIDC_AUDIENCEinternally viamintEnv, eliminating theAudiencefield from their config structsmintcore.RegisterEnv()incmd/mint-wasm'sinitMintcmd/mint-wasm,cmd/mint,internal/mint, GCF embed) updated to construct verifiers directly.embedfiles synced;env.goadded toembeddedMintFiles,env_js.goadded togcfSkipTesting
go test -race ./internal/mintcore/...— all tests pass including newenv_test.gogo test -race ./internal/mint/...— wiring test passes with updated APIgo test -race ./cmd/mint/...— standalone mint tests passGOOS=js GOARCH=wasm go build ./cmd/mint-wasm— WASM build succeedsTestEmbeddedMintSource_MatchesOriginal— embed sync test passesmintEnv100%,NewHandler100%,NewJWKSVerifier100%,NewSTSVerifier100%Closes #6305
Post-script verification
agent/6305-internalize-mintenv)c6668c13e7cca8edd7cb8d6ac644f0321ae01403..HEAD)