feat: extract mintcore shared library with JWKSVerifier and status endpoint by ggallen · Pull Request #1783 · fullsend-ai/fullsend

ggallen · 2026-06-02T14:20:30Z

Summary

Extract shared types and functions from internal/mint/main.go into a new internal/mintcore/ Go module, then refactor the GCP mint to import from it. This PR also introduces new features alongside the refactoring.

This is the first half of #1751, split to isolate the production-risk changes for focused review.

New features

JWKSVerifier: Direct JWKS-based OIDC token verification (for future dev mint and non-GCP deployments)
/v1/status endpoint: Authenticated endpoint returning configured orgs and roles
RolePermissions immutability: RolePermissions is now a function returning a deep copy, preventing accidental mutation of the canonical permission map

What moved to mintcore

OIDCVerifier interface with JWKSVerifier (new) and STSVerifier (extracted from GCP mint)
Claims/Audience types and validation (ValidateOrgAllowed, ValidateWorkflowRef)
GitHub API helpers (GenerateAppJWT, FindInstallation, CreateInstallationToken)
Pattern constants (GitHubOrgPattern, RepoNamePattern, RolePattern)
PEMAccessor interface for pluggable PEM storage
WIF helper (BuildRepoProviderID)

What changed in the GCP mint

main.go now imports from mintcore instead of defining types/functions inline
STSVerifier replaces inline stsTokenValidator (same logic, just relocated)
NewHandler constructs the verifier internally
/v1/status endpoint added for mint health inspection (authenticated)

Intentional behavioral change: 403 → 401 for OIDC auth failures

The /v1/token endpoint now returns HTTP 401 (Unauthorized) for OIDC authentication failures, changed from the previous 403 (Forbidden). This is intentional:

401 is semantically correct per HTTP spec — OIDC verification is authentication, not authorization
/v1/status already returned 401 for the same condition, so both endpoints are now consistent
Authorization failures (e.g., role not allowed) continue to return 403

Clients that distinguish 401 from 403 in error handling may need to update their logic.

Provisioner / Cloud Function deployment

Provisioner bundles mintcore/ source alongside the mint function in the deployment zip
go.mod replace directive rewritten from ../mintcore to ./mintcore during zip creation
9 new .embed files for mintcore (synced from source, including go.sum)
Embed sync test extended to cover mintcore files

Review focus

Is the STSVerifier extraction faithful — same logic, no behavioral changes?
Does bundleFunctionSource correctly nest mintcore in the deployment zip?
Are all embedded files in sync with their source counterparts?
Does JWKSVerifier correctly validate org allowlist and workflow refs?

Test plan

go test ./internal/mintcore/... — all tests pass (including new JWKSVerifier validation tests)
go test ./internal/mint/... — all tests pass (including new STSVerifier integration test)
go test ./internal/dispatch/gcf/... — provisioner tests pass (including embed sync)
go vet ./... — clean

🤖 Generated with Claude Code

github-actions · 2026-06-02T14:22:13Z

Site preview

Preview: https://b92bc573-site.fullsend-ai.workers.dev

Commit: eecac658c9996f7a0408ffb3c680c1d5bb89e7d8

fullsend-ai-review · 2026-06-02T14:34:23Z

Review

Findings

Medium

[behavioral-change] internal/mintcore/handler.go — OIDC authentication failures on /v1/token now return HTTP 401 (Unauthorized) instead of 403 (Forbidden). This is semantically correct per HTTP spec (401 = unauthenticated, 403 = authenticated but unauthorized) and explicitly documented in the PR description. The primary consumer (mint-token action) uses --retry-all-errors and checks for non-2xx broadly, so the practical impact is low. Both /v1/token and /v1/status now consistently use 401 for authentication failures. Clients that distinguish 401 from 403 in error handling or monitoring may need to update their logic. See also: [authn-status-code] from security perspective.

Low

[behavioral-change] internal/mintcore/handler.go — lookupRoleAppID changed from exact-case map lookup to case-insensitive iteration via strings.ToLower. This resolves a prior inconsistency where handleStatus lowercased keys but lookupRoleAppID did not. Covered by TestHandler_StatusEndpoint_MixedCaseRoleAppIDs. Verify that no deployed ROLE_APP_IDS configurations rely on case-sensitive key differentiation.
[race-condition] internal/mintcore/jwks_verifier.go — Between RUnlock and singleflight.Do, the recentKidMiss value can become stale. Benign in practice: worst case is one extra JWKS refresh within minRefreshInterval, and singleflight collapses concurrent refreshes. No data corruption or incorrect verification can result.
[data-exposure] internal/mintcore/handler.go — The /v1/status endpoint exposes role names for the authenticated caller's org. Correctly filters to only the requesting org's roles (not app IDs or other orgs' data), requires OIDC authentication, and test coverage verifies no app ID leakage. Within design intent for diagnostics.
[validation-duplication] internal/mintcore/gcp_pem.go — Org/role validation (pattern match + double-hyphen check) is duplicated between GCPSecretPEMAccessor.AccessPEM and provisioner.StoreAgentPEM. Consider centralizing into helper functions in patterns.go, or documenting this as intentional defense-in-depth.
[error-message-consistency] internal/mintcore/jwks_verifier.go, internal/mintcore/sts_verifier.go — JWKSVerifier says "OIDC audience must be configured" (generic, accurate), while STSVerifier says "OIDC_AUDIENCE must be configured" (references env var). Since STSVerifier is always configured from os.Getenv("OIDC_AUDIENCE"), the message is contextually correct for all current callers. Minor inconsistency.
[constant-naming-consistency] internal/mintcore/patterns.go — Clock skew is defined as package-level maxClockSkew (30s, time.Duration) which is more idiomatic than the prior clockSkewSeconds integer. Consider exporting as MaxClockSkew for consistency with other exported pattern constants.

Info

[test-integrity] — Test coverage verified: deleted internal/mint/main_test.go (1667 lines) is fully replaced by internal/mintcore/handler_test.go (1761 lines) plus claims_test.go, github_test.go, jwks_verifier_test.go, sts_verifier_test.go, and wif_test.go. New tests are strictly stronger — they use real RSA key generation and JWKS-based verification instead of fakeTokenValidator. No test weakening or assertion loosening found.
[faithful-extraction] — STSVerifier extraction faithfully preserves all validation logic. ValidateWorkflowRef is semantically equivalent. lookupRoleAppID case-sensitivity inconsistency from prior reviews is resolved — both lookupRoleAppID and handleStatus now consistently use strings.ToLower.
[defense-in-depth] — JWKSVerifier verification sequence is sound: parse header → parse claims → validate issuer/audience/time → fetch key → verify signature → validate org → validate workflow ref. RSA minimum key size of 2048 bits enforced. JWKS URI origin validated against issuer. Response size capped at 512KB. Stale keys bounded by 24h maxKeysStaleness. STSVerifier verification sequence is equally sound.
[immutability-improvement] — RolePermissions changed from an exported mutable var to unexported canonicalRolePermissions with accessor functions returning deep copies. Validated by TestRolePermissions_DeepCopy. Appropriate for a shared library consumed by multiple mint implementations.
[path-traversal-defense] — addDirToZipRooted correctly implements path traversal protection (absolute path check + separator), symlink skipping, extension allowlisting (.go/.mod/.sum), and test file exclusion. The mintcoreReplaceRe regex correctly rewrites => ../mintcore to => ./mintcore in go.mod for the deployed zip layout.
[injection-defense] — No prompt injection patterns, Unicode steganography, or bidirectional text overrides found in code comments, string literals, or configuration values across all 51 changed files.
[docs-current] — All documentation updated in this PR: CLAUDE.md (mintcore sync instructions), cli-internals.md (mintcore.BuildRepoProviderID), installation.md (two-module description), infrastructure-reference.md (RolePermissions() and Status Endpoint section), mint-administration.md (two-module description). No stale documentation remains.
[scope-verified] — All changes trace to authorized work in issue feat: add mintcore shared library and dev mint for local evaluation #1751, which explicitly lists JWKSVerifier, /v1/status endpoint, and mintcore extraction. PR scope matches the "first half" claim (mintcore extraction without devmint).

Previous run

Review

Findings

Medium

[behavioral-change] internal/mintcore/handler.go — OIDC authentication failures on /v1/token now return HTTP 401 (Unauthorized) instead of 403 (Forbidden). This is semantically correct and explicitly documented in the PR description. The primary consumer (mint-token action) uses --retry-all-errors and checks for non-2xx broadly, so the practical impact is low. Both /v1/token and /v1/status now consistently use 401 for authentication failures, which is an improvement over the prior inconsistency. See also: [http-status-code-change] from cross-repo contracts perspective. (Anchored from prior review — code unchanged.)

Low

[behavioral-change] internal/mintcore/handler.go — lookupRoleAppID changed from exact-case map lookup to case-insensitive iteration via strings.ToLower. This resolves the prior case-sensitivity inconsistency where handleStatus lowercased keys but lookupRoleAppID did not, and is covered by TestHandler_StatusEndpoint_MixedCaseRoleAppIDs. Verify that no deployed ROLE_APP_IDS configurations rely on case-sensitive key differentiation.
[error-message-accuracy] internal/mintcore/jwks_verifier.go — The error message "OIDC audience must be configured" in JWKSVerifier is accurate (audience set via config struct), but STSVerifier still says "OIDC_AUDIENCE must be configured" referencing the env var. Since STSVerifier is always configured from os.Getenv("OIDC_AUDIENCE"), the message is contextually correct for all current callers. (Anchored from prior review — code unchanged.)
[race-condition] internal/mintcore/jwks_verifier.go — Between RUnlock and singleflight.Do, the recentKidMiss value can become stale. Benign in practice: worst case is one extra JWKS refresh within minRefreshInterval, and singleflight collapses concurrent refreshes. (Anchored from prior review — code unchanged.)
[http-status-code-change] internal/mintcore/handler.go — 403→401 status code change from a cross-repo contract perspective. Documented and intentional; no known external consumers affected. See also: [behavioral-change] at this location. (Anchored from prior review.)
[data-exposure] internal/mintcore/handler.go — The /v1/status endpoint exposes role names for the authenticated caller's org. Correctly filters to only the requesting org's roles (not app IDs or other orgs' data), requires OIDC authentication, and test coverage verifies no app ID leakage. Within the stated design intent for diagnostics. (Anchored from prior review — code unchanged.)
[validation-pattern] internal/mintcore/gcp_pem.go — Org/role validation (pattern match + double-hyphen check) is duplicated between GCPSecretPEMAccessor.AccessPEM and provisioner.StoreAgentPEM. Consider centralizing into helper functions in patterns.go.
[constant-naming] internal/mintcore/jwks_verifier.go, internal/mintcore/sts_verifier.go — Clock skew is defined as package-level maxClockSkew (30s) in jwks_verifier.go but as a local clockSkewSeconds inside a function in sts_verifier.go. Consider extracting to a shared package-level constant for consistency.

Info

[test-integrity] — Test coverage verified: deleted internal/mint/main_test.go (1667 lines) is fully replaced by internal/mintcore/handler_test.go (1761 lines) plus claims_test.go, github_test.go, jwks_verifier_test.go, sts_verifier_test.go, and wif_test.go. New tests are strictly stronger — they use real RSA key generation and JWKS-based verification instead of fakeTokenValidator. No test weakening or assertion loosening found.
[faithful-extraction] — STSVerifier extraction faithfully preserves all validation logic. ValidateWorkflowRef is semantically equivalent. lookupRoleAppID case-sensitivity inconsistency from prior review is resolved — both lookupRoleAppID and handleStatus now consistently use strings.ToLower.
[defense-in-depth] — JWKSVerifier verification sequence is sound: parse header → parse claims → validate issuer/audience/time → fetch key → verify signature → validate org → validate workflow ref. RSA minimum key size of 2048 bits is enforced in parseRSAPublicKey. JWKS URI origin validated against issuer. Response size capped at 512KB. Stale keys bounded by 24h maxKeysStaleness.
[immutability-improvement] — RolePermissions changed from an exported mutable var to unexported canonicalRolePermissions with accessor functions returning deep copies. Validated by TestRolePermissions_ReturnsCopy.
[audit-trail-restored] — FindInstallation logs cross-org installation mismatches ("cross-org installation mismatch") before returning the error.
[deployment-bundling] — addDirToZipRooted correctly implements path traversal protection (absolute path check + separator), symlink skipping, extension allowlisting (.go/.mod/.sum), and test file exclusion. The mintcoreReplaceRe regex correctly rewrites => ../mintcore to => ./mintcore in go.mod for the deployed zip layout.
[architectural-alignment] — The mintcore module follows Go module conventions correctly with separate go.mod, clean interface boundaries (OIDCVerifier, PEMAccessor), and supports the multi-implementation mint architecture authorized by feat: add mintcore shared library and dev mint for local evaluation #1751.
[scope-resolved] — Prior scope-creep findings (scaffold template changes to dispatch.yml, repo-maintenance.yml, intent-coherence.md, style-conventions.md) have been removed from this PR revision. The PR scope is now focused on mintcore extraction as authorized by feat: add mintcore shared library and dev mint for local evaluation #1751.
[docs-current] — All documentation updated in this PR: CLAUDE.md (mintcore sync instructions), cli-internals.md (mintcore.BuildRepoProviderID, line counts), installation.md (two-module description), infrastructure-reference.md (RolePermissions() and Status Endpoint section), mint-administration.md (two-module description). No stale documentation remains.
[injection-defense] — Scanned all 51 changed files for prompt injection patterns, Unicode steganography, and bidirectional text overrides. No suspicious patterns found in code comments, string literals, or configuration values.

Previous run (2)

Review

Findings

Medium

[scope-creep] internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml:274 — Removes fix from the code|fix) STAGE_ROLE="coder" case statement, so the fix stage will now use its own fix role (with contents:write, pull_requests:write, issues:write, metadata:read) instead of mapping to coder (which additionally has checks:read). This is a behavioral change for all repos using the scaffold template and is not documented in the PR description, nor covered by the linked issue feat: add mintcore shared library and dev mint for local evaluation #1751. While the permission narrowing is benign (the fix role already exists with appropriate permissions), this should be documented or split into a separate PR.
Remediation: Either document this as an intentional fix in the PR description (explaining why fix should use its own role), or revert the dispatch.yml change and handle it in a separate PR.
[behavioral-change] internal/mintcore/handler.go — OIDC authentication failures on /v1/token now return HTTP 401 (Unauthorized) instead of 403 (Forbidden). This is semantically correct and explicitly documented in the PR description. The primary consumer (mint-token action) uses --retry-all-errors and checks for non-2xx broadly, so the practical impact is low. See also: [http-status-code-change] from cross-repo contracts perspective.

Low

[error-message-accuracy] internal/mintcore/jwks_verifier.go — The error message "OIDC_AUDIENCE must be configured" references the OIDC_AUDIENCE environment variable, which is accurate for STSVerifier but misleading for JWKSVerifier where the audience is set via the JWKSVerifierConfig struct, not an env var. Operators debugging a devmint configuration would see a reference to an irrelevant env var.
Remediation: Use a generic message like "audience must be configured" or give each verifier path its own phrasing.
[race-condition] internal/mintcore/jwks_verifier.go — Between RUnlock and singleflight.Do, the recentKidMiss value can become stale. Benign in practice: worst case is one extra JWKS refresh within minRefreshInterval, and singleflight collapses concurrent refreshes. (Anchored from prior review — code unchanged.)
[http-status-code-change] internal/mintcore/handler.go — 403→401 status code change from a cross-repo contract perspective. Documented and intentional; no known external consumers affected. See also: [behavioral-change] at this location.
[data-exposure] internal/mintcore/handler.go — The /v1/status endpoint exposes role names for the authenticated caller's org. While it correctly filters to only the requesting org's roles (not app IDs or other orgs' data) and requires OIDC authentication, this is a minor increase in information surface. Within the stated design intent for diagnostics.

Info

[test-integrity] — Test coverage verified: the deleted main_test.go (1667 lines) is fully replaced by mintcore/handler_test.go (1761 lines) plus mint/wiring_test.go and mint/pem_test.go. New tests are strictly stronger — they use real RSA key generation and JWKS-based verification instead of fakeTokenValidator. No test weakening or assertion loosening found.
[faithful-extraction] — STSVerifier extraction faithfully preserves all validation logic. ValidateWorkflowRef is semantically equivalent. lookupRoleAppID case-sensitivity inconsistency from prior review is resolved — both lookupRoleAppID and handleStatus now consistently use strings.ToLower.
[defense-in-depth] — JWKSVerifier verification sequence is sound: parse header → parse claims → validate issuer/audience/time → fetch key → verify signature → validate org → validate workflow ref. RSA minimum key size of 2048 bits enforced in parseRSAPublicKey. JWKS URI origin validated against issuer. Response size capped at 512KB. Stale keys bounded by 24h maxKeysStaleness.
[immutability-improvement] — RolePermissions changed from an exported mutable var to unexported canonicalRolePermissions with accessor functions returning deep copies. Validated by TestRolePermissions_ReturnsCopy.
[audit-trail-restored] — FindInstallation now logs cross-org installation mismatches ("cross-org installation mismatch") before returning the error. This resolves the prior low-severity finding about removed audit logging.
[deployment-bundling] — addDirToZipRooted correctly implements path traversal protection (absolute path check + separator), symlink skipping, and extension allowlisting (.go/.mod/.sum). The go.mod replace directive rewriting (../mintcore → ./mintcore) is validated by provisioner tests.
[architectural-alignment] — The mintcore module follows Go module conventions correctly with separate go.mod, clean interface boundaries (OIDCVerifier, PEMAccessor), and supports the multi-implementation mint architecture authorized by feat: add mintcore shared library and dev mint for local evaluation #1751.
[scope-resolved] — Prior scope-creep findings (scaffold template changes to repo-maintenance.yml, intent-coherence.md, style-conventions.md) have been removed from this PR revision. The PR scope is now focused on mintcore extraction as authorized by feat: add mintcore shared library and dev mint for local evaluation #1751.

Previous run (3)

Review

Findings

Medium

[scope-creep] internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml:274 — Removes fix from the code|fix) STAGE_ROLE="coder" case statement, so the fix stage will now use its own fix role (with contents:write, pull_requests:write, issues:write, metadata:read) instead of mapping to coder (which additionally has checks:read). This is a behavioral change for all repos using the scaffold template and is not documented in the PR description, nor covered by the linked issue feat: add mintcore shared library and dev mint for local evaluation #1751. While the permission narrowing is benign (the fix role already exists with appropriate permissions), this should be documented or split into a separate PR.
Remediation: Either document this as an intentional fix in the PR description (explaining why fix should use its own role), or revert the dispatch.yml change and handle it in a separate PR.
[behavioral-change] internal/mintcore/handler.go — OIDC authentication failures on /v1/token now return HTTP 401 (Unauthorized) instead of 403 (Forbidden). This is semantically correct and explicitly documented in the PR description. The primary consumer (mint-token action) uses --retry-all-errors and checks for non-2xx broadly, so the practical impact is low. See also: [http-status-code-change] from cross-repo contracts perspective.

Low

[error-message-accuracy] internal/mintcore/jwks_verifier.go — The error message "OIDC_AUDIENCE must be configured" references the OIDC_AUDIENCE environment variable, which is accurate for STSVerifier but misleading for JWKSVerifier where the audience is set via the JWKSVerifierConfig struct, not an env var. Operators debugging a devmint configuration would see a reference to an irrelevant env var.
Remediation: Use a generic message like "audience must be configured" or give each verifier path its own phrasing.
[race-condition] internal/mintcore/jwks_verifier.go — Between RUnlock and singleflight.Do, the recentKidMiss value can become stale. Benign in practice: worst case is one extra JWKS refresh within minRefreshInterval, and singleflight collapses concurrent refreshes. (Anchored from prior review — code unchanged.)
[http-status-code-change] internal/mintcore/handler.go — 403→401 status code change from a cross-repo contract perspective. Documented and intentional; no known external consumers affected. See also: [behavioral-change] at this location.
[data-exposure] internal/mintcore/handler.go — The /v1/status endpoint exposes role names for the authenticated caller's org. While it correctly filters to only the requesting org's roles (not app IDs or other orgs' data) and requires OIDC authentication, this is a minor increase in information surface. Within the stated design intent for diagnostics.

Info

[test-integrity] — Test coverage verified: the deleted main_test.go (1667 lines) is fully replaced by mintcore/handler_test.go (1761 lines) plus mint/wiring_test.go and mint/pem_test.go. New tests are strictly stronger — they use real RSA key generation and JWKS-based verification instead of fakeTokenValidator. No test weakening or assertion loosening found.
[faithful-extraction] — STSVerifier extraction faithfully preserves all validation logic. ValidateWorkflowRef is semantically equivalent. lookupRoleAppID case-sensitivity inconsistency from prior review is resolved — both lookupRoleAppID and handleStatus now consistently use strings.ToLower.
[defense-in-depth] — JWKSVerifier verification sequence is sound: parse header → parse claims → validate issuer/audience/time → fetch key → verify signature → validate org → validate workflow ref. RSA minimum key size of 2048 bits is enforced in parseRSAPublicKey. JWKS URI origin validated against issuer. Response size capped at 512KB. Stale keys bounded by 24h maxKeysStaleness.
[immutability-improvement] — RolePermissions changed from an exported mutable var to unexported canonicalRolePermissions with accessor functions returning deep copies. Validated by TestRolePermissions_ReturnsCopy.
[audit-trail-restored] — FindInstallation now logs cross-org installation mismatches ("cross-org installation mismatch") before returning the error. This resolves the prior low-severity finding about removed audit logging.
[deployment-bundling] — addDirToZipRooted correctly implements path traversal protection (absolute path check + separator), symlink skipping, and extension allowlisting (.go/.mod/.sum). The go.mod replace directive rewriting (../mintcore → ./mintcore) is validated by provisioner tests.
[architectural-alignment] — The mintcore module follows Go module conventions correctly with separate go.mod, clean interface boundaries (OIDCVerifier, PEMAccessor), and supports the multi-implementation mint architecture authorized by feat: add mintcore shared library and dev mint for local evaluation #1751.
[scope-resolved] — Prior scope-creep findings (scaffold template changes to repo-maintenance.yml, intent-coherence.md, style-conventions.md) have been removed from this PR revision. The PR scope is now focused on mintcore extraction as authorized by feat: add mintcore shared library and dev mint for local evaluation #1751.

Previous run (4)

Review

Findings

Medium

[behavioral-change] internal/mintcore/handler.go:198 — OIDC authentication failures on /v1/token now return HTTP 401 (Unauthorized) instead of 403 (Forbidden). While semantically correct and explicitly documented in the PR description, this is a behavioral change for callers that distinguish 401 from 403 in error handling or monitoring. The primary consumer (mint-token action) uses --retry-all-errors so is unaffected. See also: [breaking-api] at this location.
[scope-creep] internal/scaffold/fullsend-repo/.github/workflows/repo-maintenance.yml — Removes the enabled field check (select(.value.enabled == true)) and changes from graceful skip to hard error when no repos are found. This workflow enrollment change is functionally independent of the mintcore extraction and is not authorized by the linked issue feat: add mintcore shared library and dev mint for local evaluation #1751.
[scope-creep] internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/intent-coherence.md — Removes the "Exploration budget" and "Revert PR authorization" sections (~54 lines), changing how the intent-coherence sub-agent calibrates investigation depth and handles revert PRs across all scaffolded repos. Not related to mintcore extraction.
[scope-creep] internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/style-conventions.md — Lowers the trivial-diff threshold from 20 to 10 lines, removes tool-use restrictions, and simplifies early-exit criteria. Changes agent behavior for all future reviews in scaffolded repos. Not related to mintcore extraction.

Low

[case-sensitivity-inconsistency] internal/mintcore/handler.go:294 — lookupRoleAppID uses a case-sensitive map lookup on roleAppIDs keys, but handleStatus lowercases keys before prefix matching. If ROLE_APP_IDS contains mixed-case org names (e.g. MyOrg/coder), the status endpoint will show the role as available, but token minting will fail to find the app ID.
[audit-trail] internal/mintcore/github.go:171 — The old findInstallation logged cross-org installation mismatches with log.Printf before returning the error. The new FindInstallation returns the error without logging. This removes a security-relevant audit trail entry for cross-org token leakage attempts.
[error-message-divergence] internal/mintcore/sts_verifier.go — Error message for unconfigured audience changed from "OIDC_AUDIENCE must be configured" to "OIDC audience must be configured" (lowercase, no underscore). Could break log-based alerting rules matching the old string.
[race-condition] internal/mintcore/jwks_verifier.go:216 — Between RUnlock and singleflight.Do, the recentKidMiss value can become stale. Benign in practice: worst case is one extra JWKS refresh within minRefreshInterval, and singleflight collapses concurrent refreshes.
[breaking-api] internal/mintcore/handler.go:198 — 403→401 status code change from cross-repo contract perspective. Documented and intentional; no known external consumers affected. See also: [behavioral-change] at this location.

Info

[test-integrity] — Test coverage verified: the deleted main_test.go (1667 lines) is fully replaced by mintcore/handler_test.go (1761 lines) plus mint/wiring_test.go and mint/pem_test.go. New tests are strictly stronger — they include JWKS signature verification via testOIDCEnv. No test weakening or assertion loosening found.
[faithful-extraction] — STSVerifier extraction faithfully preserves all validation logic from the old inline code. ValidateWorkflowRef is semantically equivalent with one structural improvement: derives config repo owner from the repository claim directly.
[defense-in-depth] — JWKSVerifier verification sequence is sound: parse header → parse claims → validate issuer/audience/time → fetch key → verify signature → validate org → validate workflow ref. No authorization decisions before signature verification.
[architectural-alignment] — The mintcore module follows Go module conventions correctly with separate go.mod, replace directive in parent, and clean interface boundaries (OIDCVerifier, PEMAccessor). Aligns with the project's documented architecture for supporting multiple mint implementations.
[immutability-improvement] — RolePermissions changed from an exported mutable var to unexported canonicalRolePermissions with accessor functions returning deep copies. Correctness improvement validated by TestRolePermissions_ReturnsCopy.

Previous run (5)

Review — PR #1783

Head SHA: 7a6a810e6a961f2e4c7884408407907c1e91a465

Summary

Large extraction PR (55 files, ~9700 lines changed) that moves mint HTTP handler logic from internal/mint/main.go into a new internal/mintcore/ package, introduces a JWKSVerifier for direct OIDC token verification, adds a /v1/status diagnostic endpoint, and updates scaffold templates. The core extraction is well-executed: tests are preserved and strengthened (real RSA-signed JWTs instead of mocked validators), the handler API surface is cleaner (error returns instead of log.Fatalf), and RolePermissions gains immutability via deep-copy accessor.

Two high-severity findings block approval — both relate to unauthorized scope changes in scaffold templates that are not covered by the linked issue #1751.

Findings

High

#	Category	File	Description
1	scope-authorization-mismatch	`internal/scaffold/fullsend-repo/scripts/post-fix.sh`	Adds `scripts/` to `PROTECTED_PATHS` in both `post-fix.sh` and `post-review.sh` scaffold templates. This governance change modifies agent autonomy guardrails without authorization from #1751. While it is a security improvement, PROTECTED_PATHS changes define what agents can modify and should be explicitly authorized. Remediation: File a separate issue proposing `scripts/` protection with security rationale, or add explicit justification to the PR description.
2	scope-authorization-mismatch	`internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/correctness.md`	Removes the entire "Technical documentation with correctness surface area" section from the correctness agent mandate (`correctness.md` + `SKILL.md` dispatch table). This narrows review coverage by eliminating correctness checks on algorithm descriptions, pseudocode, data structure definitions, and API behavior claims in design documents. Not authorized by #1751. Remediation: File a separate issue with justification or split into own PR.

Medium

#	Category	File	Line	Description
3	behavioral-change	`internal/mintcore/handler.go`	125	Auth failure HTTP status changed from 403→401. Semantically correct but a breaking behavioral change for clients matching on status codes. (Anchored from prior review.)
4	defense-in-depth	`internal/mintcore/jwks_verifier.go`	495	`parseRSAPublicKey` does not validate minimum RSA key size. A compromised JWKS endpoint could inject a weak key. Add `if n.BitLen() < 2048` check. (See also: security finding #5.)
5	JWKS-key-validation	`internal/dispatch/gcf/mintsrc/mintcore/jwks_verifier.go.embed`	495	Same RSA key size gap in the embedded Cloud Function copy. (Cross-ref: correctness finding #4.)

Low

#	Category	File	Description
6	missing-doc-comment	`internal/mintcore/handler.go`	`mintError` type lacks doc comment; sibling types have them.
7	missing-doc-comment	`internal/mintcore/jwks_verifier.go`	Types `jwtHeader`, `jwksResponse`, `jwkKey`, `discoveryDoc` lack doc comments.
8	stale-doc	`docs/guides/dev/cli-internals.md`	Line counts for `main.go` (~~906→~~936) and `provisioner.go` (~~1350→~~1897) are stale.
9	missing-doc	`docs/guides/dev/cli-internals.md`	New `mintcore` package not documented in Key Source Files Reference.

Info

Extraction faithfulness: STSVerifier, handler, claims, github, patterns, and WIF extraction is faithful to the original — no behavioral regressions detected beyond the intentional 403→401 change.
Test integrity: The deleted 1667-line main_test.go is fully replaced by stronger tests using real RSA-signed JWTs via testOIDCEnv. No test weakening or coverage reduction detected.
RolePermissions immutability: Conversion from exported mutable map to unexported with deep-copy accessor is a correctness improvement during extraction.
JWKS cache design: Permanent cachedJWKSURI with origin validation is a deliberate performance trade-off with appropriate documentation.
Claims-before-signature pattern: JWT claims are parsed before signature verification in JWKSVerifier.Verify — this is standard JWKS practice (kid needed to find key). Existing kid-miss rate limiting mitigates DoS surface.

Positive observations

The mintcore extraction is architecturally sound. The package boundary is clean, the handler returns errors instead of fataling, test coverage is maintained with stronger verification (real crypto instead of mocks), and the embed-sync pattern is correctly followed. The JWKSVerifier implementation is thorough with proper origin validation, key rotation support, and rate limiting.

Recommended resolution

Scaffold governance changes (findings 1-2): Either split into a separate PR with proper authorization, or update the PR description + linked issue to explicitly authorize these changes.
403→401 change (finding 3): Add a note confirming this is intentional and that consumers have been notified.
RSA key size check (findings 4-5): Add minimum 2048-bit validation in parseRSAPublicKey.

Previous run (6)

Review

Findings

Medium

[api-contract] internal/mintcore/handler.go — The /v1/token endpoint changed its HTTP status code for OIDC authentication failures from 403 (Forbidden) to 401 (Unauthorized). In the old code, both prevalidateOIDCToken and tokenValidator.Validate failures returned 403. In the new handler, oidcVerifier.Verify failures return 401. This is a behavioral change for any client that distinguishes auth failure responses by status code. 401 is arguably more correct per HTTP semantics (authentication failure vs. authorization failure), and the /v1/status endpoint also uses 401 — so the API is now internally consistent. If backward compatibility is not a concern, consider documenting this as an intentional change in the PR description.

Low

[test-inadequate] internal/mint/wiring_test.go — The new wiring_test.go covers health, auth rejection, and routing through the composed STSVerifier+Handler, and handler_test.go includes STSVerifier integration tests exercising the full success path. The remaining narrow gap: no integration test in internal/mint/ exercises the full success path through the actual GCP wiring (STSVerifier+GCPSecretPEMAccessor combined), but this would require a GCP environment and is reasonable to omit from unit tests.
[pattern-inconsistency] internal/mintcore/handler.go — The statusResponse type lacks a documentation comment while sibling types mintRequest and mintResponse have doc comments. Minor consistency gap.

Info

[api-contract] internal/mintcore/handler.go — /v1/status and /v1/token now both return HTTP 401 for OIDC auth failures. This is internally consistent — an improvement over the prior state where /v1/token used 403 and /v1/status used 401. See also: medium [api-contract] finding about the 403→401 behavioral change.
[extraction-faithfulness] internal/mintcore/sts_verifier.go — STSVerifier extraction is faithful: all validation steps from the old prevalidateOIDCToken, resolveWIFProvider, and stsTokenValidator.Validate are preserved with identical logic. ValidateWorkflowRef correctly replicates per-repo, .fullsend, and upstream workflow ref validation.
[authentication] internal/mintcore/jwks_verifier.go — JWKS verification is well-implemented: validates RS256 algorithm, kid presence, issuer, audience, expiry/iat with clock skew, cryptographic signature, org allowlist, and workflow ref. JWKS discovery validates origin matches issuer. Rate-limits kid-miss refreshes with singleflight deduplication. maxKeysStaleness (24h) prevents indefinite stale-key usage.
[prior-resolved] Multiple prior findings resolved: handleStatus casing fixed (uses strings.ToLower consistently), NewHandler returns (*Handler, error) instead of calling log.Fatalf(), method checks run before auth for all paths, /v1/status error logging added, stale-doc findings addressed.

Previous run (7)

Review

Findings

Low

[test-inadequate] internal/mint/wiring_test.go — The new wiring_test.go covers health, auth rejection, and routing through the composed STSVerifier+Handler, and handler_test.go includes TestHandler_STSVerifier_Integration and TestHandler_STSVerifier_PerRepoWIF_RestrictedWorkflows which exercise the verifier end-to-end. The remaining narrow gap: no integration test exercises the full success path through STSVerifier.Verify → mintToken → mock GitHub API in the internal/mint/ package. Individual components are thoroughly tested in internal/mintcore/.
[pattern-inconsistency] internal/mintcore/patterns.go — Pattern regexes (GitHubOrgPattern, RepoNamePattern, RolePattern) are exported here but identical copies remain unexported in internal/dispatch/gcf/provisioner.go and internal/cli/admin.go. Future pattern changes must be synchronized across three locations. Consider having the other packages import from mintcore to deduplicate.

Info

[api-contract] internal/mintcore/handler.go — /v1/status returns HTTP 401 for auth failure while /v1/token returns HTTP 403 for the same condition. Both are defensible (401 = not authenticated, 403 = not authorized), but the inconsistency may confuse clients with shared error-handling logic.
[authentication] internal/mintcore/jwks_verifier.go — JWKS verification is well-implemented: validates RS256 algorithm, kid presence, issuer, audience, expiry/iat with clock skew, cryptographic signature, org allowlist, and workflow ref. JWKS discovery validates origin matches issuer. Rate-limits kid-miss refreshes with singleflight deduplication. Added maxKeysStaleness (24h) since prior review — addresses the indefinite stale-key concern.
[extraction-faithfulness] internal/mintcore/sts_verifier.go — STSVerifier extraction is faithful: all validation steps from the old two-phase flow are preserved. GitHub API helpers, RolePermissions deep-copy, path traversal protections are correct.
[prior-resolved] Multiple prior findings resolved in this iteration: (1) handleStatus casing inconsistency fixed — now uses strings.ToLower consistently for both role lookup and response. (2) NewHandler() now returns (*Handler, error) instead of calling log.Fatalf(). (3) Prior behavioral regression (method check vs auth check ordering) fixed — method checks now run before auth for all paths. (4) /v1/status error logging added (log.Printf on OIDC failure). (5) Both stale-doc findings in cli-internals.md and infrastructure-reference.md addressed by PR doc updates.

Previous run (8)

Review

Findings

Low

[test-inadequate] internal/mint/wiring_test.go — The new wiring_test.go covers health, auth rejection, and routing through the composed STSVerifier+Handler, and handler_test.go includes TestHandler_STSVerifier_Integration and TestHandler_STSVerifier_PerRepoWIF_RestrictedWorkflows which exercise the verifier end-to-end. The remaining narrow gap: no integration test exercises the full success path through STSVerifier.Verify → mintToken → mock GitHub API in the internal/mint/ package. Individual components are thoroughly tested in internal/mintcore/.
[pattern-inconsistency] internal/mintcore/patterns.go — Pattern regexes (GitHubOrgPattern, RepoNamePattern, RolePattern) are exported here but identical copies remain unexported in internal/dispatch/gcf/provisioner.go and internal/cli/admin.go. Future pattern changes must be synchronized across three locations. Consider having the other packages import from mintcore to deduplicate.

Info

[api-contract] internal/mintcore/handler.go — /v1/status returns HTTP 401 for auth failure while /v1/token returns HTTP 403 for the same condition. Both are defensible (401 = not authenticated, 403 = not authorized), but the inconsistency may confuse clients with shared error-handling logic.
[authentication] internal/mintcore/jwks_verifier.go — JWKS verification is well-implemented: validates RS256 algorithm, kid presence, issuer, audience, expiry/iat with clock skew, cryptographic signature, org allowlist, and workflow ref. JWKS discovery validates origin matches issuer. Rate-limits kid-miss refreshes with singleflight deduplication. Added maxKeysStaleness (24h) since prior review — addresses the indefinite stale-key concern.
[extraction-faithfulness] internal/mintcore/sts_verifier.go — STSVerifier extraction is faithful: all validation steps from the old two-phase flow are preserved. GitHub API helpers, RolePermissions deep-copy, path traversal protections are correct.
[prior-resolved] Multiple prior findings resolved in this iteration: (1) handleStatus casing inconsistency fixed — now uses strings.ToLower consistently for both role lookup and response. (2) NewHandler() now returns (*Handler, error) instead of calling log.Fatalf(). (3) Prior behavioral regression (method check vs auth check ordering) fixed — method checks now run before auth for all paths. (4) /v1/status error logging added (log.Printf on OIDC failure). (5) Both stale-doc findings in cli-internals.md and infrastructure-reference.md addressed by PR doc updates.

Previous run (9)

Review

Findings

Medium

[test-inadequate] internal/mint/main_test.go — The entire 1667-line integration test file was deleted. While replacement unit tests exist in internal/mintcore/handler_test.go, they use fakeOIDCVerifier rather than testing the composed STSVerifier+Handler path. The old tests (e.g., TestServeHTTP_DotFullsendProvider) verified the full init() → ServeHTTP → prevalidateOIDCToken → resolveWIFProvider → Validate chain. The new pem_test.go in internal/mint/ only tests GCPSecretPEMAccessor input validation — a small fraction of the prior coverage. Consider adding at least one integration-style test that wires STSVerifier+Handler together to prove the init() wiring is correct.
[logic-error] internal/mintcore/handler.go (handleStatus) — The handleStatus method lowercases claims.RepositoryOwner for role lookup but returns the original casing in the Org response field. While not a security issue (the endpoint is read-only and properly authenticated), this is inconsistent with /v1/token which uses strings.ToLower(claims.RepositoryOwner) throughout. Clients comparing the Org field from /v1/status against the org used in token minting would see different casing.

Low

[pattern-violation] internal/mintcore/handler.go — NewHandler() uses log.Fatalf() for env var validation. The original code had this in init(), which only runs in production. Moving it into the constructor makes the function harder to test in isolation (callers must set all env vars or the process exits). Consider returning an error from NewHandler() instead.
[pattern-inconsistency] internal/mintcore/patterns.go — Pattern regexes (GitHubOrgPattern, RepoNamePattern, RolePattern) are exported here but identical copies remain unexported in internal/dispatch/gcf/provisioner.go and internal/cli/admin.go. Future changes to validation patterns will need to be synchronized across three locations.
[stale-doc] docs/guides/dev/cli-internals.md:485 — File size table lists internal/mint/main.go at ~906 lines, but this PR reduces it to ~52 lines after extracting code to mintcore. The table should reflect the new size and add a row for internal/mintcore/.
[stale-doc] docs/guides/infrastructure/infrastructure-reference.md:169 — References BuildRepoProviderID() without the mintcore. package prefix. Other docs in this PR were updated to use mintcore.BuildRepoProviderID() but this occurrence was missed.

Info

[api-contract] internal/mintcore/handler.go — /v1/status returns HTTP 401 for auth failure while /v1/token returns HTTP 403 for the same condition. Both are defensible (401 = not authenticated, 403 = not authorized), but the inconsistency may confuse clients with shared error-handling logic.
[authentication] internal/mintcore/jwks_verifier.go — JWKS verification is well-implemented: validates RS256 algorithm, kid presence, issuer, audience, expiry/iat with clock skew, cryptographic signature, org allowlist, and workflow ref. JWKS discovery validates origin matches issuer. Rate-limits kid-miss refreshes with singleflight deduplication.

Previous run (10)

Review

Findings

Medium

[test-inadequate] internal/mint/main_test.go — The entire 1667-line integration test file was deleted. While replacement unit tests exist in internal/mintcore/handler_test.go, they use fakeOIDCVerifier rather than testing the composed STSVerifier+Handler path. The old tests (e.g., TestServeHTTP_DotFullsendProvider) verified the full init() → ServeHTTP → prevalidateOIDCToken → resolveWIFProvider → Validate chain. The new pem_test.go in internal/mint/ only tests GCPSecretPEMAccessor input validation — a small fraction of the prior coverage. Consider adding at least one integration-style test that wires STSVerifier+Handler together to prove the init() wiring is correct.
[logic-error] internal/mintcore/handler.go (handleStatus) — The handleStatus method lowercases claims.RepositoryOwner for role lookup but returns the original casing in the Org response field. While not a security issue (the endpoint is read-only and properly authenticated), this is inconsistent with /v1/token which uses strings.ToLower(claims.RepositoryOwner) throughout. Clients comparing the Org field from /v1/status against the org used in token minting would see different casing.

Low

[pattern-violation] internal/mintcore/handler.go — NewHandler() uses log.Fatalf() for env var validation. The original code had this in init(), which only runs in production. Moving it into the constructor makes the function harder to test in isolation (callers must set all env vars or the process exits). Consider returning an error from NewHandler() instead.
[pattern-inconsistency] internal/mintcore/patterns.go — Pattern regexes (GitHubOrgPattern, RepoNamePattern, RolePattern) are exported here but identical copies remain unexported in internal/dispatch/gcf/provisioner.go and internal/cli/admin.go. Future changes to validation patterns will need to be synchronized across three locations.
[stale-doc] docs/guides/dev/cli-internals.md:485 — File size table lists internal/mint/main.go at ~906 lines, but this PR reduces it to ~52 lines after extracting code to mintcore. The table should reflect the new size and add a row for internal/mintcore/.
[stale-doc] docs/guides/infrastructure/infrastructure-reference.md:169 — References BuildRepoProviderID() without the mintcore. package prefix. Other docs in this PR were updated to use mintcore.BuildRepoProviderID() but this occurrence was missed.

Info

[api-contract] internal/mintcore/handler.go — /v1/status returns HTTP 401 for auth failure while /v1/token returns HTTP 403 for the same condition. Both are defensible (401 = not authenticated, 403 = not authorized), but the inconsistency may confuse clients with shared error-handling logic.
[authentication] internal/mintcore/jwks_verifier.go — JWKS verification is well-implemented: validates RS256 algorithm, kid presence, issuer, audience, expiry/iat with clock skew, cryptographic signature, org allowlist, and workflow ref. JWKS discovery validates origin matches issuer. Rate-limits kid-miss refreshes with singleflight deduplication.

Previous run (11)

Review

Findings

Low

[edge-case] internal/mintcore/handler.go:1990 — In handleStatus, the role name is extracted using lower[strings.Index(lower, "/")+1:]. Since the HasPrefix(lower, prefix) guard already confirms lower starts with org + "/", the strings.Index call is redundant and could be replaced with strings.TrimPrefix(lower, prefix) for clarity and resilience to hypothetical key format changes.
[data-exposure] internal/mintcore/handler.go:234 — The /v1/status endpoint returns role names for the authenticated caller's org. The endpoint requires full OIDC verification and returns only role names (not app IDs or other orgs' data). The exposure is minimal since role names are documented in the repo.
[jwt-verification] internal/mintcore/jwks_verifier.go:115 — JWKSVerifier falls back to stale cached keys when JWKS fetches fail. The 1-hour jwksCacheTTL provides an implicit staleness bound, but there is no maximum staleness limit — if refreshes keep failing, a compromised key could remain accepted indefinitely. Consider adding a maximum staleness window (e.g., 24h). This only affects the JWKS path (devmint), not the production STS path.

Info

[test-integrity] All 50+ test functions from the deleted internal/mint/main_test.go (1667 lines) have equivalents in the new internal/mintcore/ test files (handler_test.go at 1746 lines, jwks_verifier_test.go at 495 lines, sts_verifier_test.go at 205 lines, plus claims_test.go and github_test.go). Tests are strengthened: fakeTokenValidator replaced by newTestOIDCEnv with real RSA keys, signed JWTs, and a live JWKS endpoint. No assertion loosening detected.
[extraction-faithfulness] STSVerifier extraction is faithful — all validation steps from the old prevalidateOIDCToken + stsTokenValidator.Validate two-phase flow are preserved in STSVerifier.Verify(). GitHub API helpers, RolePermissions deep-copy, bundleFunctionSource go.mod rewrite, and addDirToZipRooted path traversal protections are correct.
[prior-resolved] Prior medium finding (Handler.allowedOrgs dead code) is resolved — the allowedOrgs field has been removed from the Handler struct. Org validation is now delegated entirely to the OIDCVerifier implementations. Prior low finding (fragile pem_test assertion) is also resolved — the test no longer asserts on specific error message strings.

Previous run (12)

Review

Findings

Medium

[logic-error] internal/mintcore/handler.go:45 — Handler.allowedOrgs is dead code: the field is populated from ALLOWED_ORGS in NewHandler() but never read by any Handler method. In the old code, allowedOrgs was used by checkAllowedOrg() in prevalidateOIDCToken(). In the refactored code, org validation is delegated entirely to OIDCVerifier.Verify(), which has its own copy of allowedOrgs. The dead field creates a false impression that org validation happens at the handler level and risks configuration divergence.
Remediation: Remove the allowedOrgs field from Handler and the corresponding os.Getenv("ALLOWED_ORGS") parsing block in NewHandler(). This clarifies that org validation is solely the OIDCVerifier's responsibility. Apply the same change to handler.go.embed.

Low

[missing-test] internal/mint/pem_test.go — TestGCPSecretPEMAccessor_InputValidation's "valid org and role" case asserts the error contains "getting metadata token", which verifies it passed input validation but is fragile: any refactoring that changes the metadata fetch error message would silently break the assertion. Consider asserting err != nil with a specific non-validation error, or documenting that the GCP metadata call is expected to fail in unit tests.
[data-exposure] internal/mintcore/handler.go:234 — The /v1/status endpoint returns role names for the authenticated caller's org. The endpoint requires full OIDC verification and returns only role names (not app IDs or other orgs' data). The exposure is minimal since role names are documented in the repo.
[jwt-verification] internal/mintcore/jwks_verifier.go:115 — JWKSVerifier falls back to stale cached keys when JWKS fetches fail. If a key is rotated or revoked at the issuer, the verifier continues accepting tokens signed with the old key indefinitely as long as refresh attempts fail. Consider adding a maximum staleness window (e.g., 24h) after which stale keys are rejected. This only affects the JWKS path (devmint), not the production STS path.

Info

[test-integrity] All 50+ test functions from the deleted internal/mint/main_test.go (1667 lines) have equivalents in the new internal/mintcore/ test files. No assertion loosening detected. Tests are strengthened: fakeTokenValidator replaced by newTestOIDCEnv with real RSA keys, signed JWTs, and a live JWKS endpoint.
[extraction-faithfulness] STSVerifier extraction is faithful — all validation steps from the old prevalidateOIDCToken + stsTokenValidator.Validate two-phase flow are preserved in STSVerifier.Verify(). GitHub API helpers, RolePermissions deep-copy, bundleFunctionSource go.mod rewrite, and addDirToZipRooted path traversal protections are correct.

Previous run (13)

Review

Findings

Low

[missing-test] internal/mint/pem_test.go — TestSMPEMAccessor_InputValidation's "valid org and role" case does not meaningfully assert the happy path. When wantErr is empty, the test only checks that the error (if any) does not contain "invalid org name" or "invalid role name" — any other error (e.g., network failure reaching GCP metadata) passes silently. The test name correctly scopes itself to input validation, but the "valid" case gives false confidence. Consider asserting err == nil is false with a specific non-validation error message, or documenting that the GCP Secret Manager call is expected to fail in unit tests.
[data-exposure] internal/mintcore/handler.go — The /v1/status endpoint returns role names for the authenticated caller's org. While properly authenticated and the test verifies no app IDs leak, any compromised workflow OIDC token from an allowed org can enumerate available roles. Role names are already documented in the repo, so the exposure is minimal, but consider whether the endpoint should be restricted to specific roles or documented as intentionally public. See also: [scope-creep] finding for this endpoint.
[jwt-verification] internal/mintcore/oidc.go — JWKSVerifier falls back to stale cached keys when JWKS fetches fail (the "stale key is better than no key" path). This improves availability during transient JWKS endpoint failures, but a rotated/revoked key could continue to be accepted indefinitely if fetches keep failing. Consider adding a maximum staleness window (e.g., 24 hours) after which stale keys are rejected.
[scope-creep] internal/mintcore/oidc.go, internal/mintcore/handler.go — JWKSVerifier and /v1/status are net-new features introduced alongside the extraction refactor. Both are authorized by issue feat: add mintcore shared library and dev mint for local evaluation #1751 (which explicitly lists "JWKS-based OIDC verifier" and "/v1/status diagnostic endpoint"), so this is within scope. Noting for visibility since the PR title emphasizes "extract" but includes new functionality.

Info

[test-integrity] All 50+ test functions from the deleted internal/mint/main_test.go (1667 lines) have equivalents in the new internal/mintcore/handler_test.go (1719 lines) and other test files. No assertion loosening detected. Tests are strengthened: fakeTokenValidator replaced by newTestOIDCEnv with real RSA keys, signed JWTs, and a live JWKS endpoint. New coverage added for status endpoint, STS verifier integration, role permissions immutability, and error message leak prevention.
[extraction-faithfulness] STSVerifier extraction is faithful — all validation steps from the old prevalidateOIDCToken + stsTokenValidator.Validate two-phase flow are preserved in STSVerifier.Verify(). GitHub API helpers (GenerateAppJWT, FindInstallation, CreateInstallationToken) and RolePermissions deep-copy are correct. The bundleFunctionSource go.mod rewrite regex and addDirToZip path traversal protections (symlink skip, absRoot prefix check, extension allowlist) are adequate.
[sub-agent-failure] N/A — The style-conventions sub-agent could not access the PR branch files (repo checked out on main). Style review was not completed for this run.

Previous run (14)

Review

Findings

High

[logic-error] internal/mintcore/handler.go:4018 — Behavioral regression in HTTP routing: GET /v1/token without an Authorization header now returns 401 Unauthorized instead of the previous 405 Method Not Allowed. The old ServeHTTP checked the HTTP method before extracting the auth header (method check at line 955, auth at line 960). The new code checks auth (line 4018) before the method check (line 4035) for non-status paths. Additionally, TestHandler_MethodNotAllowed was modified to include req.Header.Set("Authorization", "Bearer dummy-token") — the old test sent no auth header and verified 405. The same pattern applies to TestHandler_RootPathAccepted (GET / without auth now returns 401 instead of 405). This is a test weakening that masks a production behavioral change.
Remediation: Move the POST method check for /v1/token and / paths before the auth header extraction. After the /v1/status method guard (line 4013-4016), add: if r.URL.Path != "/v1/status" && r.Method != http.MethodPost { writeError(w, http.StatusMethodNotAllowed, "method not allowed"); return }. Then restore TestHandler_MethodNotAllowed and TestHandler_RootPathAccepted to not send a Bearer token, matching the original test behavior.

Medium

[missing-test] internal/mint/main.go — The internal/mint/ package has zero test coverage after main_test.go (-1667 lines) and testmain_test.go (-35 lines) were deleted. The remaining smPEMAccessor.AccessPEM method validates org and role inputs using mintcore.GitHubOrgPattern, mintcore.RolePattern, and double-hyphen checks before constructing GCP Secret Manager resource names. These validations are security-relevant (preventing path traversal in Secret Manager URLs) and are not exercised by any test. While the regex patterns are tested in mintcore, the double-hyphen rejection and Secret Manager path construction logic are specific to this file.
Remediation: Add a minimal test file for internal/mint/ covering smPEMAccessor.AccessPEM input validation — at minimum: valid org/role, org with double-hyphen, role with double-hyphen, org failing pattern, and role failing pattern.

Low

[error-handling-idiom] internal/mintcore/handler.go:4027 — The /v1/status endpoint silently discards OIDC verification errors (writeError(w, 401, "authentication failed") with no logging), while the /v1/token endpoint logs "OIDC verification failed: %v" before returning its error. Inconsistent error logging makes debugging authentication failures harder for operators.
[incomplete-doc] docs/guides/infrastructure/mint-administration.md:76 — Documentation describes --source-dir (default internal/mint/) as the mint source directory but does not mention the new internal/mintcore/ module. While the flag behavior is unchanged (the provisioner bundles mintcore automatically), developers reading these docs won't know about the mintcore module's existence or the .embed sync requirement.
[incomplete-doc] docs/guides/getting-started/installation.md:228 — Flag documentation for --mint-source-dir describes it as pointing to internal/mint/ without mentioning that the mint now consists of two modules. Same concern as above — functionally correct but incomplete.
[code-organization] internal/mintcore/handler.go — The statusResponse type is defined after ServeHTTP rather than grouped with other request/response types (mintRequest, mintResponse) near the top of the handler. The original internal/mint/main.go grouped all request/response types together.

waynesun09

Review Squad Report — 7 agents, 11 findings (MEDIUM+)

Agents: 2x claude-coder, 2x claude-researcher, 2x gemini-code-review, 1x cursor-code-review
Total findings: 21 (after dedup and false positive removal) — 3 HIGH, 8 MEDIUM, 7 LOW, 3 INFO
False positives removed: 5

Summary

The STSVerifier extraction is faithful — identical validation logic, error messages, and control flow preserved. The most actionable finding is the JWKSVerifier fail-open design on empty allowlists (3-agent consensus), which creates a security asymmetry with STSVerifier that should be fixed before devmint uses it. The status endpoint App ID exposure (4-agent consensus) and duplicate BuildRepoProviderID are cleanup items to address in this PR. Four deleted test suites covering security-relevant edge cases should be restored in mintcore.

Only MEDIUM+ findings are posted inline. 7 LOW and 3 INFO findings available on request.

ggallen · 2026-06-02T15:58:53Z

All findings from both review sources have been addressed in fc4843d2:

From review #4411014072 (8 inline comments): All fixed and threads resolved.

From issue comment #4603468794:

[auth-bypass] MEDIUM — Fixed: JWKSVerifier now errors on empty audience (fail-closed, matching STSVerifier)
[test-inadequate] MEDIUM — Fixed: Added TestHandler_RestrictedWorkflowFiles integration test
[coverage-reduced] LOW — Fixed: Added whitespace audience edge case tests and TestValidateOrgAllowed_EmptyList
[data-exposure] LOW — Fixed: Removed App IDs from /v1/status response
[pattern-violation] LOW — Fixed: Added symlink check in addDirToZip

ggallen · 2026-06-03T16:09:24Z

Addressing review findings from pullrequestreview-4419453135:

Medium:

[scope-exceeded] internal/cli/run.go — Acknowledged. The URL resource resolution changes (--offline, HasURLReferences, resolve.ResolveHarness) are tightly coupled to the harness loading path that was being refactored. Splitting them out would require reverting and re-applying harness.go changes. Will keep as-is for this PR since the changes are self-contained and well-tested.

Low — fixed:

[edge-case] handleStatus null roles — Fixed. Changed var roles []string to roles := make([]string, 0) so JSON serializes as "roles":[] instead of "roles":null.
[edge-case] handleStatus key slicing — Fixed. Replaced key[len(prefix):] with lower[strings.Index(lower, "/")+1:] for robust role name extraction independent of prefix length assumptions.
[test-adequacy] json.Decode error — Fixed. Added error check on json.NewDecoder(rec.Body).Decode(&resp) in TestHandler_StatusEndpoint.
[test-adequacy] mixed-case test — Fixed. Added TestHandler_StatusEndpoint_MixedCaseRoleAppIDs with mixed-case org names in ROLE_APP_IDS to verify case-insensitive matching.

Low — acknowledged (no change):

[api-contract] go.mod string replacement — The guard (rewritten == original) catches failures. Using golang.org/x/mod/modfile would add a dependency for a single string replacement in deployment tooling. The go.mod.embed format is controlled by us, not external tools.
[dependency-change] golang.org/x/net removal — Intentional. The only consumer was code now in mintcore (which has its own go.mod).

Embed copy synced. All tests pass. Squashed, rebased, and force-pushed.

ggallen · 2026-06-03T18:07:12Z

Low findings from review addressed in e7cda39:

[naming-convention] Unexported InstallationResponse → installationResponse and InstallationTokenResponse → installationTokenResponse in internal/mintcore/github.go — reduces API surface since they're only used within mintcore. Updated all test references. Synced embed copy.
[api-contract] Added comment to internal/dispatch/gcf/provisioner.go clarifying the literal string match for go.mod rewriting and that the guard catches formatting changes.
[test-adequacy] Production mint init() smoke test — acknowledged as low-priority since init() is just functions.HTTP() framework registration. No change.
[code-organization] Acknowledged as good defensive pattern. No change needed.

Scope-creep cleanup: removed 4 scaffold sub-agent model pin changes that were unrelated to mintcore extraction.

fullsend-ai-review

See the review comment for full details.

ifireball

Sorry for still requesting changes, but I want to get this re-factor right. If all goes well, the dev/stand-alone mint PR should end up being very small (A PEMAccessor implementation reading from local files + a tiny main to launch the server in the background)

fullsend-ai-review

See the review comment for full details.

Extract shared mint logic from internal/mint/main.go into internal/mintcore/ package. Handler, OIDC verifiers (STS + JWKS), GitHub API helpers, claims, and GCP PEM accessor are now in the shared library. The Cloud Function entry point retains only GCP-specific wiring (init, env vars). Key changes: - OIDCVerifier is injected into Handler (supports STSVerifier and JWKSVerifier) - PEMAccessor is injected (GCPSecretPEMAccessor exported from mintcore) - BuildRepoProviderID consolidated into mintcore (removed from provisioner) - JWKS verifier uses singleflight to prevent thundering-herd refreshes - RSA key size validation (2048-bit minimum) in parseRSAPublicKey - Issuer validation in OIDC discovery per spec Section 4.3 - Case-insensitive org lookup in lookupRoleAppID - Cross-org installation mismatch audit logging preserved - Shared maxClockSkew constant and ValidateOrgName/ValidateRoleName helpers - go.mod replace directive rewriting validated in bundleFunctionSource - Symlink detection in addDirToZip for Cloud Function packaging - Handler tests use real RSA-signed JWTs instead of mocked validators - All source files synced to .embed copies for Cloud Function deployment Signed-off-by: Greg Allen <gallen@fullsend.ai> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Greg Allen <gallen@redhat.com>

ggallen mentioned this pull request Jun 2, 2026

feat: add development-only local mint server #1784

Open

5 tasks

github-actions Bot deployed to site-preview June 2, 2026 14:22 View deployment

ggallen mentioned this pull request Jun 2, 2026

feat: add mintcore shared library and dev mint for local evaluation #1751

Closed

6 tasks

fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Jun 2, 2026

ggallen changed the title ~~refactor: extract mintcore shared library from GCP mint~~ feat: extract mintcore shared library with JWKSVerifier and status endpoint Jun 2, 2026

ggallen force-pushed the refactor/mintcore branch from 9806077 to e5a32f6 Compare June 2, 2026 14:50

github-actions Bot deployed to site-preview June 2, 2026 14:52 View deployment

ralphbean mentioned this pull request Jun 2, 2026

Review agent: post inline comments for comment-only verdicts #1787

Closed

fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Jun 2, 2026

waynesun09 reviewed Jun 2, 2026

View reviewed changes

ggallen force-pushed the refactor/mintcore branch from e5a32f6 to fc4843d Compare June 2, 2026 15:54

github-actions Bot deployed to site-preview June 2, 2026 15:56 View deployment

fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Jun 2, 2026

ggallen force-pushed the refactor/mintcore branch from fc4843d to 60e46b1 Compare June 2, 2026 16:19

github-actions Bot deployed to site-preview June 2, 2026 16:21 View deployment

ggallen force-pushed the refactor/mintcore branch from 60e46b1 to 9f4015b Compare June 2, 2026 16:22

github-actions Bot deployed to site-preview June 2, 2026 16:26 View deployment

fullsend-ai-review Bot approved these changes Jun 2, 2026

View reviewed changes

fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed requires-manual-review Review requires human judgment labels Jun 2, 2026

ggallen force-pushed the refactor/mintcore branch from 9f4015b to a2ed9bb Compare June 2, 2026 17:34

github-actions Bot deployed to site-preview June 2, 2026 17:36 View deployment

fullsend-ai-review Bot approved these changes Jun 2, 2026

View reviewed changes

fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed ready-for-merge All reviewers approved — ready to merge labels Jun 2, 2026

ggallen force-pushed the refactor/mintcore branch from 4309f74 to 531aefd Compare June 3, 2026 16:08

github-actions Bot deployed to site-preview June 3, 2026 16:10 View deployment

ggallen force-pushed the refactor/mintcore branch from 531aefd to 24d32f1 Compare June 3, 2026 16:10

github-actions Bot deployed to site-preview June 3, 2026 16:12 View deployment

fullsend-ai-review Bot approved these changes Jun 3, 2026

View reviewed changes

fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed requires-manual-review Review requires human judgment labels Jun 3, 2026

ggallen requested a review from ifireball June 3, 2026 16:23

ggallen force-pushed the refactor/mintcore branch from 24d32f1 to e7cda39 Compare June 3, 2026 18:07

github-actions Bot deployed to site-preview June 3, 2026 18:08 View deployment

fullsend-ai-review Bot approved these changes Jun 3, 2026

View reviewed changes

fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed ready-for-merge All reviewers approved — ready to merge labels Jun 3, 2026

ggallen force-pushed the refactor/mintcore branch from e7cda39 to 7237a6b Compare June 3, 2026 19:35

github-actions Bot deployed to site-preview June 3, 2026 19:37 View deployment

fullsend-ai-review Bot approved these changes Jun 3, 2026

View reviewed changes

fullsend-ai-review Bot suggested changes Jun 4, 2026

View reviewed changes

Comment thread internal/mint/main.go Outdated

fullsend-ai-review Bot approved these changes Jun 4, 2026

View reviewed changes

ifireball requested changes Jun 4, 2026

View reviewed changes

Comment thread internal/mint/main.go Outdated

Comment thread internal/mintcore/handler.go Outdated

Comment thread internal/mintcore/sts_verifier.go

ifireball reviewed Jun 4, 2026

View reviewed changes

Comment thread internal/mintcore/jwks_verifier.go

fullsend-ai-review Bot approved these changes Jun 4, 2026

View reviewed changes

Comment thread internal/mintcore/handler.go Outdated

Comment thread internal/mintcore/jwks_verifier.go

fullsend-ai-review Bot approved these changes Jun 4, 2026

View reviewed changes

ifireball approved these changes Jun 4, 2026

View reviewed changes

fullsend-ai-review Bot suggested changes Jun 4, 2026

View reviewed changes

Comment thread internal/scaffold/fullsend-repo/scripts/post-fix.sh Outdated

Comment thread internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/correctness.md

Comment thread internal/mintcore/handler.go

Comment thread docs/guides/dev/cli-internals.md

This was referenced Jun 4, 2026

Review bot should not override human approval with stale findings #1922

Open

Review agent should flag large PRs and recommend splitting #2034

Open

Uh oh!

Conversation

ggallen commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

New features

What moved to mintcore

What changed in the GCP mint

Intentional behavioral change: 403 → 401 for OIDC auth failures

Provisioner / Cloud Function deployment

Review focus

Test plan

Uh oh!

github-actions Bot commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Site preview

Uh oh!

fullsend-ai-review Bot commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review

Findings

Medium

Low

Info

Review

Findings

Medium

Low

Info

Review

Findings

Medium

Low

Info

Review

Findings

Medium

Low

Info

Review

Findings

Medium

Low

Info

Review — PR #1783

Summary

Findings

High

Medium

Low

Info

Positive observations

Recommended resolution

Review

Findings

Medium

Low

Info

Review

Findings

Low

Info

Review

Findings

Low

Info

Review

Findings

Medium

Low

Info

Review

Findings

Medium

Low

Info

Review

Findings

Low

Info

ggallen commented Jun 2, 2026 •

edited

Loading

github-actions Bot commented Jun 2, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 2, 2026 •

edited

Loading