Skip to content

feat: validate mint infrastructure during per-repo install#904

Merged
waynesun09 merged 7 commits into
mainfrom
mint-url-validate
May 14, 2026
Merged

feat: validate mint infrastructure during per-repo install#904
waynesun09 merged 7 commits into
mainfrom
mint-url-validate

Conversation

@waynesun09
Copy link
Copy Markdown
Contributor

Summary

  • Adds a mint validation step to runPerRepoInstall that verifies the mint Cloud Function exists at the expected URL, checks if the org is already registered in ALLOWED_ORGS and ROLE_APP_IDS, and updates the function's env vars if the org is missing
  • Adds UpdateFunctionEnvVars to the GCF client for env-var-only PATCH (avoids corrupting buildConfig.source)
  • Adds EnsureOrgInMint provisioner method with URL verification, org coverage check, and additive env var update
  • Adds resolveSharedRoleAppIDs helper to discover app IDs for the new org by matching installed apps against existing mint entries
  • Copies shared PEM secrets for the new org with deterministic source selection
  • Makes --mint-region available in per-repo mode (removed from perOrgOnlyFlags)

Test plan

  • go test ./internal/dispatch/gcf/... — 11 new tests covering UpdateFunctionEnvVars, EnsureOrgInMint (org covered, adds new org, function not found, nil return, URL mismatch, partial coverage, update fails, wait fails, empty/malformed ROLE_APP_IDS, value mismatch trigger)
  • go test ./internal/cli/... — 6 new tests covering --mint-region acceptance in per-repo mode and resolveSharedRoleAppIDs (matches installed apps, error when not installed, error when no existing IDs, skips same org, uses own entry)
  • go vet ./internal/... — clean
  • E2E test with a real per-repo install against a live mint (manual)

@github-actions
Copy link
Copy Markdown

fullsend review is working on this — view logs

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 14, 2026

Site preview

Preview: https://53418a8f-site.fullsend-ai.workers.dev

Commit: ce0ca7e93fd1a9735ab3ee24f2ad798e83a3d452

@fullsend-ai-review
Copy link
Copy Markdown

fullsend-ai-review Bot commented May 14, 2026

Review: #904

Head SHA: ce0ca7e
Timestamp: 2026-05-14T00:00:00Z
Outcome: approve

Summary

This PR adds mint infrastructure validation during per-repo install, ensuring the Cloud Function exists, URLs match, and the org is registered in ALLOWED_ORGS and ROLE_APP_IDS before proceeding. It also adds per-repo WIF provider resolution in the mint handler (routing .fullsend repos to the default provider and per-repo repos to dynamically constructed provider IDs), a new UpdateFunctionEnvVars API for env-var-only PATCH updates, and proper error propagation in CopyAgentPEM. The code is well-structured, follows existing patterns, and has comprehensive test coverage across all new paths. No critical or high findings were identified.

Findings

Medium

  • [Correctness] internal/mint/main.go:buildRepoProviderID — Function is duplicated from internal/dispatch/gcf/provisioner.go (exported BuildRepoProviderID). The SYNC comment in tests is good practice, but if the two copies diverge, per-repo WIF validation will silently fail (mint resolves a different provider ID than the provisioner created). Consider extracting to a shared internal package if the Cloud Function build constraints allow it.
    Remediation: Move to a shared internal/wifid package, or add a CI check that compares the two function bodies.

Low

  • [Correctness] internal/dispatch/gcf/provisioner.go:EnsureOrgInMint — Read-modify-write on function env vars is not atomic. Concurrent per-repo installs sharing a mint could overwrite each other's org registrations. The doc comment notes "not safe for concurrent calls," but nothing enforces sequential execution.
    Remediation: Consider adding advisory locking (e.g., GCS lock file) or documenting the serialization requirement in the admin guide.

  • [Style] internal/cli/admin.go--mint-project was also removed from perOrgOnlyFlags (now accepted in per-repo mode and falls back to --inference-project), but the PR description only mentions --mint-region. Minor documentation gap.

Info

  • [Platform security] internal/dispatch/gcf/provisioner.go:EnsureOrgInMintALLOWED_WORKFLOW_FILES defaults to "*" when empty, permitting any workflow file to request tokens. This matches the existing provisionSelfManaged behavior and is appropriate for initial setup, but operators should be aware of the permissive default.

  • [Correctness] internal/mint/main.go:resolveWIFProvider — Uses the repository claim from the OIDC token (pre-STS-validation) to select the WIF provider. This is safe because a wrong provider selection causes STS validation to fail, but the design relies on the trust boundary being at STS, not at provider selection. This is correctly implemented.

Footer

Outcome: approve
This review applies to SHA ce0ca7e93fd1a9735ab3ee24f2ad798e83a3d452. Any push to the PR head clears this review and requires a new evaluation.

Previous run

Review: #904

Head SHA: 11f0d9a
Timestamp: 2026-05-14T00:00:00Z
Outcome: comment-only

Summary

This PR adds mint infrastructure validation during per-repo install, including org registration in ALLOWED_ORGS/ROLE_APP_IDS, shared PEM copying, per-repo WIF provider resolution, and an env-var-only Cloud Function update API. The changes are well-structured, well-tested (17+ new tests), and follow existing patterns. No critical or high findings. A few medium items worth noting around code duplication risk and a default value that could override an operator's intent.

Findings

Medium

  • [Correctness] internal/mint/main.gobuildRepoProviderID is duplicated from internal/dispatch/gcf/provisioner.go (BuildRepoProviderID). While the SYNC test comment documents the constraint (Cloud Function cannot import the provisioner package), any future divergence between these implementations would cause per-repo WIF resolution to silently fail — the provisioner would create a provider with one ID and the mint would look up a different one. Consider extracting this into a shared leaf package (e.g., internal/wifid/) that both the Cloud Function and provisioner can import, or at minimum add a build-time check beyond the comment.
    Remediation: Extract to a shared package or add a go generate step that verifies the two implementations are identical.

  • [Platform security] internal/dispatch/gcf/provisioner.go:~line 345 (EnsureOrgInMint) — When updating env vars, if ALLOWED_WORKFLOW_FILES is empty, it is set to "*". The prevalidateOIDCToken comment says "An empty or unset value denies all requests. Set to * to allow any workflow file." If an operator intentionally emptied this value as a kill-switch (deny all token minting), adding a new org via EnsureOrgInMint would silently re-enable all workflows. This also applies to the same logic added in provisionSelfManaged.
    Remediation: Consider only defaulting to "*" on initial deployment (when the function is first created), not on updates to existing functions. Or document that ALLOWED_WORKFLOW_FILES must be set explicitly if the operator wants deny-all.

Low

  • [Correctness] internal/dispatch/gcf/provisioner.go (EnsureOrgInMint) — The method is a read-modify-write on function env vars without locking. The doc comment correctly documents this ("Not safe for concurrent calls"), but the caller in runPerRepoInstall doesn't enforce serialization. If two per-repo installs targeting the same mint run simultaneously, the second write could overwrite the first's additions.
    Remediation: Acceptable for now given the documented limitation. Consider adding an advisory lock or compare-and-swap (e.g., check source hash before writing) in a follow-up if concurrent per-repo installs become common.

  • [Style] internal/cli/admin.go — The PEM-copying loop (lines ~500-530 in the diff) duplicates the sorted-keys + match pattern from resolveSharedRoleAppIDs. Both iterate sorted existingIDs keys, split on /, filter by role and skip same-org. Minor duplication but the two serve different purposes (one resolves app IDs, the other copies secrets).

Info

  • [Correctness] internal/mint/main.go — The TokenValidator interface signature changed from Validate(ctx, oidcToken) to Validate(ctx, oidcToken, providerName). This is a breaking interface change. The only implementations (stsTokenValidator and fakeTokenValidator) are both updated in this PR, and the interface is internal, so this is safe. Noted for awareness.

  • [Correctness] internal/config/config.goPerRepoDefaultRoles() excludes "fullsend" from the default roles. The comment explains this is because per-repo mode uses the target repo's shim workflow. This is a deliberate design choice, not a bug.

Footer

Outcome: comment-only
This review applies to SHA 11f0d9a3159a2b0ebf7900d7348c086050954f06. Any push to the PR head clears this review and requires a new evaluation.

Previous run (2)

Review: #904

Head SHA: 046e3b5
Timestamp: 2026-05-14T00:00:00Z
Outcome: comment-only

Summary

This PR adds mint infrastructure validation during per-repo install, including org registration in ALLOWED_ORGS/ROLE_APP_IDS, a targeted UpdateFunctionEnvVars API to avoid corrupting buildConfig.source, WIF provider routing per repository, and shared PEM secret copying. The implementation is well-structured with good test coverage across success and error paths. No critical or high findings; the main concern is a duplicated buildRepoProviderID function across two packages that could drift silently.

Findings

Medium

  • [Correctness] internal/mint/main.go:631-651 / internal/dispatch/gcf/provisioner.go:639-655buildRepoProviderID is duplicated between the mint Cloud Function and the provisioner package (unexported in mint, exported in provisioner). The code comment acknowledges this ("Duplicated from internal/dispatch/gcf/provisioner.go to avoid importing the provisioner package into the Cloud Function"), but there is no mechanism (test, build constraint, linter) to detect drift between the two copies. If they diverge, per-repo WIF authentication silently fails — the provisioner creates a provider with one ID and the mint validates against a different one.
    Remediation: Add a cross-package test that asserts buildRepoProviderID(owner, repo) == gcf.BuildRepoProviderID(owner, repo) for a set of representative inputs, or extract the function into a shared leaf package with no heavy dependencies.

Low

  • [Correctness] internal/dispatch/gcf/provisioner.go:208-210CopyAgentPEM has a behavior change: previously it returned nil immediately when the destination secret existed; now it calls ensureSecretIAM to re-grant SA access. This is a beneficial fix for older installs but affects all callers of CopyAgentPEM, not just the new per-repo flow. The PR description doesn't mention this change, making it easy to overlook during review.
    Remediation: Document this behavior change in the PR description or commit message.

  • [Correctness] internal/dispatch/gcf/provisioner.go:225-239GetExistingRoleAppIDs returns (nil, nil) on GCF API errors, nil function responses, and JSON parse failures. The new runPerRepoInstall caller only checks err != nil, so a transient API error yields a misleading "mint has no existing ROLE_APP_IDS" error from resolveSharedRoleAppIDs instead of surfacing the real failure. This is fail-safe (won't proceed with bad data) but diagnostically confusing.
    Remediation: Consider returning the underlying error from GetFunction failures so callers can distinguish "no function" from "API error."

Info

  • [Style/conventions] internal/dispatch/gcf/provisioner.go:257EnsureOrgInMint documents "Not safe for concurrent calls" which is an honest limitation. Future work should consider a distributed lock or compare-and-swap pattern if multiple per-repo installs target the same mint concurrently.

  • [Correctness] internal/dispatch/gcf/provisioner.go:589-593provisionSelfManaged now preserves existing ALLOWED_WORKFLOW_FILES and defaults to "*" (allow all). This is the most permissive default but is gated by the existing fail-closed check in prevalidateOIDCToken, so it's a configuration convenience, not a security gap.

  • [Correctness] Test coverage is thorough: 11 new provisioner tests covering UpdateFunctionEnvVars and EnsureOrgInMint (org covered, adds new org, function not found, nil return, URL mismatch, partial coverage, update fails, wait fails, empty/malformed ROLE_APP_IDS, value mismatch); 6 new CLI tests covering --mint-region acceptance and resolveSharedRoleAppIDs; mint tests covering resolveWIFProvider, buildRepoProviderID, missing repository claim, and per-repo vs .fullsend provider routing.

  • [Injection defense] PR body, commit messages, and code comments were inspected for prompt injection patterns and non-rendering Unicode. None found.

Footer

Outcome: comment-only
This review applies to SHA 046e3b557154a39097391ebf2a1b42db9722e713. Any push to the PR head clears this review and requires a new evaluation.

Previous run (3)

Review: #904

Head SHA: f32697e
Timestamp: 2026-05-14T02:06:41Z
Outcome: comment-only

Summary

This PR adds mint infrastructure validation to per-repo installs, including a new EnsureOrgInMint provisioner method, dynamic WIF provider resolution in the mint handler, and shared PEM secret copying. The code is well-structured with thorough test coverage (17+ new tests across four files). Two medium-severity correctness findings exist around a duplicated buildRepoProviderID function that could silently diverge, and a case-sensitivity inconsistency between org presence checking (case-insensitive) and org merging (case-sensitive). Neither is blocking, but both represent maintenance risks worth addressing. No security, injection, or intent-alignment concerns were identified.

Findings

Medium

  • [correctness] internal/mint/main.go:638buildRepoProviderID is duplicated from BuildRepoProviderID in internal/dispatch/gcf/provisioner.go. The implementations are identical today, but if either changes independently, per-repo WIF provider resolution will silently diverge, causing hard-to-debug authentication failures.
    Remediation: Extract to a shared package (e.g. internal/wifid) or add a cross-package test asserting identical output.

  • [correctness] internal/dispatch/gcf/provisioner.go:268EnsureOrgInMint checks ALLOWED_ORGS with strings.EqualFold (case-insensitive) but mergeAllowedOrgs deduplicates with exact string matching. If existing ALLOWED_ORGS has "Acme-Corp" and the install passes "acme-corp", a ROLE_APP_IDS-triggered update will create duplicate ALLOWED_ORGS entries and duplicate ROLE_APP_IDS keys with different casing.
    Remediation: Normalize org names to lowercase before comparison and merge, consistent with ServeHTTP's strings.ToLower.

Low

  • [correctness] internal/cli/admin.go:509 — PEM copy selects the first alphabetically-sorted org as the source. If that org has a stale PEM, the copy will propagate stale credentials.
    Remediation: Document the selection strategy or allow the user to specify a source org.

Info

  • [style/conventions] internal/mint/main.go:635 — Duplication comment references the source file but not the exported function name (BuildRepoProviderID), making discovery via grep harder.
    Remediation: Update comment to reference the exported name.

Footer

Outcome: comment-only
This review applies to SHA f32697e97d7bdce3d8d1b39d09a9968f8a670d0e. Any push to the PR head clears this review and requires a new evaluation.

Previous run (4)

Review: #904

Head SHA: ac741a7
Timestamp: 2026-05-14T00:00:00Z
Outcome: comment-only

Summary

This PR adds mint infrastructure validation during per-repo install — verifying the Cloud Function exists, registering the org in ALLOWED_ORGS and ROLE_APP_IDS if missing, copying shared PEM secrets, and routing per-repo OIDC tokens to dynamically-constructed WIF providers. The implementation is well-structured with thorough test coverage (17 new tests). No critical or high findings; two medium items around case-sensitivity consistency and code duplication divergence risk are worth noting.

Findings

Medium

  • [Correctness] internal/dispatch/gcf/provisioner.go:EnsureOrgInMint — Case-sensitivity mismatch between org presence check and merge. EnsureOrgInMint checks ALLOWED_ORGS using strings.EqualFold (case-insensitive), but mergeAllowedOrgs deduplicates using exact string comparison (seen[org]). If an existing function has "Acme-Corp" in ALLOWED_ORGS and the new install passes "acme-corp", the EqualFold check marks it present (no update needed for that reason alone). However, if a ROLE_APP_IDS change triggers an update, mergeAllowedOrgs will add "acme-corp" alongside "Acme-Corp" since they differ case-sensitively, producing a duplicate. GitHub org names are case-insensitive, so this could cause confusion downstream.
    Remediation: Normalize org names to lowercase before the mergeAllowedOrgs call, or make mergeAllowedOrgs case-insensitive.

  • [Style/conventions] internal/mint/main.go:buildRepoProviderID — Duplicated function with divergence risk. buildRepoProviderID in mint/main.go is an exact copy of BuildRepoProviderID in dispatch/gcf/provisioner.go. The comment explains the rationale (avoiding import of provisioner into the Cloud Function), which is reasonable. However, if either copy is updated independently, the mint will construct different provider IDs than the provisioner, causing authentication failures that would be hard to diagnose.
    Remediation: Consider adding a cross-reference test that asserts both functions produce identical output for a set of inputs, or extract the function into a tiny shared package (e.g., internal/wifid/) that both can import without pulling in the full provisioner.

Low

  • [Correctness] internal/cli/admin.go:~line 510 — PEM copy source selection is deterministic but arbitrary. The sorted-key iteration picks the lexicographically first org that shares the role as the PEM source. This works but could surprise operators if the "wrong" org is chosen. The warning when no source is found is good. Consider logging which source org was selected.

Info

  • [Correctness] Test coverage is solid: 11 new provisioner tests cover happy path, error paths (function not found, nil return, URL mismatch, partial coverage, update/wait failures, malformed JSON, value mismatch), 6 CLI tests cover flag acceptance and resolveSharedRoleAppIDs edge cases, and the mint tests cover WIF provider resolution and the new repository claim validation.

  • [Platform security] The TokenValidator.Validate interface change from storing wifProviderName on the struct to passing providerName per-call is a clean way to support per-repo WIF providers without breaking the org-level flow (.fullsend repos fall back to defaultWIFProvider).

Footer

Outcome: comment-only
This review applies to SHA ac741a7a7ccab8577f27be96173cd0a828b5c9ef. Any push to the PR head clears this review and requires a new evaluation.

Previous run (5)

Review: #904

Head SHA: 7e5a7b6
Timestamp: 2026-05-14T00:00:00Z
Outcome: comment-only

Summary

This PR adds mint validation to per-repo install — verifying the Cloud Function exists, its URL matches, and the org is registered in ALLOWED_ORGS and ROLE_APP_IDS. It also refactors the mint's WIF provider resolution to be per-request (supporting per-repo WIF providers) rather than a single static value. The code is well-structured with solid test coverage (17+ new tests). Two non-blocking findings relate to code duplication risk and a minor case-sensitivity inconsistency.

Findings

Medium

  • [Correctness] internal/mint/main.go:618-651buildRepoProviderID is duplicated from internal/dispatch/gcf/provisioner.go:641-655 (BuildRepoProviderID). The comment acknowledges this is intentional to avoid importing the provisioner package into the Cloud Function, but there is no test asserting both implementations produce identical output. If either copy drifts, the mint will resolve a different WIF provider name than the one the provisioner created, causing silent authentication failures for per-repo installs.
    Remediation: Add a test in a shared test package (or a build-time check) that asserts BuildRepoProviderID and buildRepoProviderID produce the same output for a set of representative inputs. Alternatively, extract the function into a small shared package with no heavy dependencies.

Low

  • [Correctness] internal/dispatch/gcf/provisioner.go:261-270EnsureOrgInMint checks org presence in ALLOWED_ORGS case-insensitively (strings.EqualFold) but mergeAllowedOrgs deduplicates case-sensitively. If the existing value has "Acme-Corp" and the new org is "acme-corp", the check correctly finds the org (no update needed), so this doesn't cause a bug today. However, if both checks trigger an update in a future code path, a case-variant duplicate could be inserted.
    Remediation: Consider normalizing org names to lowercase before storage (consistent with Provision() which lowercases orgs at line 267).

Info

  • [Style] internal/dispatch/gcf/provisioner.go:243EnsureOrgInMint doc comment correctly warns it is not safe for concurrent calls. Good documentation of a known limitation.

  • [Correctness] internal/cli/admin.go:479-509 — PEM copy loop uses deterministic sorted-key iteration for source selection, which is good for reproducibility. The warning when no shared PEM source is found ("manual PEM setup required") is appropriate — it doesn't fail the install, letting the operator handle it.

  • [Correctness] internal/mint/main.go:563-565 — Adding a "missing repository claim" check in prevalidateOIDCToken is a good defensive addition. The repository field is needed by resolveWIFProvider and was previously unchecked.

Footer

Outcome: comment-only
This review applies to SHA 7e5a7b649cfdf7fc23a53e0cacbea5ac78c4a402. Any push to the PR head clears this review and requires a new evaluation.

Previous run (6)

Review: #904

Head SHA: a07e933
Timestamp: 2026-05-14T00:00:00Z
Outcome: comment-only

Summary

The change is well-structured and adds a coherent mint validation step to per-repo install, plus per-repo WIF provider resolution in the mint handler. The security design is sound — using the repository claim to select a WIF provider before STS validation is analogous to using a JWT kid to select a verification key. Test coverage is thorough across all new code paths. Three medium-severity observations are worth noting but do not block the change.

Findings

Medium

  • [Correctness] internal/cli/admin.go:~481GetExistingRoleAppIDs swallows all errors (returns nil, nil on GetFunction failure, nil EnvVars, or unmarshal error). When the mint function doesn't exist or a network error occurs, execution falls through to resolveSharedRoleAppIDs which produces a misleading error: "mint has no existing ROLE_APP_IDS" instead of the real cause. Consider either propagating the error from GetExistingRoleAppIDs or checking function existence before calling it.
    Remediation: Have GetExistingRoleAppIDs distinguish between "function has no ROLE_APP_IDS" (return nil, nil) and "could not reach function" (return nil, err).

  • [Correctness] internal/mint/main.go:~633-649buildRepoProviderID is explicitly duplicated from internal/dispatch/gcf/provisioner.go:BuildRepoProviderID. The implementations are identical today, but if either drifts, the mint would validate tokens against the wrong WIF provider — a silent authentication failure. The comment documents the rationale (avoid importing the provisioner package into the Cloud Function), which is reasonable.
    Remediation: Consider extracting the function into a small shared package (e.g., internal/wifid/) that both the provisioner and the Cloud Function can import, or add a test that asserts both implementations produce identical output for a set of inputs.

  • [Correctness] internal/cli/admin.go:~760-768resolveSharedRoleAppIDs iterates over a map[string]string to find matching roles from other orgs. Go map iteration is non-deterministic, so if multiple orgs have different app IDs for the same role and different installed-app sets, the selected app ID could vary between runs. In practice this is unlikely (shared apps have the same ID), but it's worth noting.
    Remediation: Sort the map keys before iterating, consistent with the deterministic pattern used in the PEM-copy loop below.

Info

  • [Correctness] internal/dispatch/gcf/provisioner.go:~307 — The newRoleAppIDs variable from json.Marshal(roleAppIDs) discards the error (newRoleAppIDs, _ := json.Marshal(...)). While json.Marshal on a map[string]string cannot fail in practice, the idiomatic Go pattern is to check the error.

  • [Correctness] internal/dispatch/gcf/provisioner.go:~244EnsureOrgInMint is documented as not safe for concurrent calls, which is accurate — it has a TOCTOU window between reading function state and updating. The existing per-repo install flow is sequential, so this is informational.

Footer

Outcome: comment-only
This review applies to SHA a07e93308126d649b6b49ce652dd723c092512d2. Any push to the PR head clears this review and requires a new evaluation.

Previous run (7)

Review: #904

Head SHA: 21e4b31
Timestamp: 2026-05-14T00:00:00Z
Outcome: comment-only

Summary

The PR adds mint infrastructure validation to per-repo install, including a new EnsureOrgInMint provisioner method, an env-var-only UpdateFunctionEnvVars GCF client method, a resolveSharedRoleAppIDs helper, and PEM secret copying for shared apps. The implementation is solid with good test coverage (17 new tests). Two non-blocking issues are worth addressing: non-deterministic map iteration in resolveSharedRoleAppIDs and a redundant GCP API call.

Findings

Medium

  • [Correctness] internal/cli/admin.go:810resolveSharedRoleAppIDs iterates over the existingIDs map in non-deterministic order when searching for a shared app matching a role. If multiple orgs share the same role with the same app installed, the selected source org varies between runs. The PEM copy loop later in the same function (line 507) sorts existingIDs keys first for deterministic source selection — this function should do the same for consistency.
    Remediation: Sort the keys of existingIDs and iterate the sorted slice instead of ranging over the map directly, matching the pattern used in the PEM copy block.

Low

  • [Correctness] internal/cli/admin.go:487-498runPerRepoInstall calls GetExistingRoleAppIDs (which internally calls GetFunction) and then passes the result to EnsureOrgInMint (which also calls GetFunction). This makes two identical GCP API calls where one would suffice. Consider passing the function info through or restructuring to avoid the redundant call.
    Remediation: Have GetExistingRoleAppIDs return the FunctionInfo alongside the parsed map, or refactor EnsureOrgInMint to accept a pre-fetched FunctionInfo.

Info

  • [Correctness] internal/dispatch/gcf/provisioner.go:227 — Pre-existing: GetExistingRoleAppIDs silently swallows GetFunction errors (returns nil, nil). The new code path then fails in resolveSharedRoleAppIDs with "no existing ROLE_APP_IDS" instead of surfacing the real error (e.g., permission denied). Not introduced by this PR but the new dependency makes the error masking more consequential. Worth a follow-up.

Footer

Outcome: comment-only
This review applies to SHA 21e4b31ccaad61d09b4f6ecaf1aa14dab56c20d3. Any push to the PR head clears this review and requires a new evaluation.

@github-actions
Copy link
Copy Markdown

fullsend review is working on this — view logs

@github-actions
Copy link
Copy Markdown

fullsend review is working on this — view logs

Add mint validation step to runPerRepoInstall that verifies the mint
function exists at the expected URL, checks org coverage in ALLOWED_ORGS
and ROLE_APP_IDS, and updates env vars if the org is missing.

- Add UpdateFunctionEnvVars to GCFClient for env-var-only PATCH
- Add EnsureOrgInMint provisioner method with URL and org validation
- Add resolveSharedRoleAppIDs to discover app IDs from existing mint
- Wire mint validation into per-repo install before WIF provisioning
- Copy shared PEM secrets for new org with deterministic source selection
- Make --mint-region available in per-repo mode

Signed-off-by: Wayne Sun <gsun@redhat.com>
The mint was hardcoded to validate all OIDC tokens against a single WIF
provider (github-oidc), which only accepts tokens from .fullsend repos.
Per-repo installs create repo-scoped providers (gh-{owner}-{repo}) that
the mint didn't know about, causing STS validation failures.

Now the mint extracts the repository claim from the OIDC token and
dynamically resolves the correct WIF provider: .fullsend repos use the
default provider, per-repo repos use the repo-scoped provider ID
constructed with the same naming convention as the CLI.

Signed-off-by: Wayne Sun <gsun@redhat.com>
- Sort existingIDs keys in resolveSharedRoleAppIDs for deterministic
  app selection when multiple orgs share the same role
- Use strings.EqualFold in EnsureOrgInMint to match the mint handler's
  case-insensitive org check and prevent duplicate ALLOWED_ORGS entries
- Warn when no shared PEM source is found during per-repo install
  instead of silently continuing
- Check json.Marshal error in EnsureOrgInMint instead of discarding it

Signed-off-by: Wayne Sun <gsun@redhat.com>
@waynesun09 waynesun09 force-pushed the mint-url-validate branch from 7e5a7b6 to ac741a7 Compare May 14, 2026 01:24
@github-actions
Copy link
Copy Markdown

fullsend review is working on this — view logs

- Remove mint-project from perOrgOnlyFlags so per-repo install can
  specify a separate GCP project for the mint Cloud Function
- Default perRepoMintProject to inferenceProject when not explicitly set
- Add PerRepoDefaultRoles() excluding the fullsend dispatch role, which
  is not needed in per-repo mode (target repo's shim handles dispatch)
- Update runPerRepoInstall to accept mintProject separately from
  inferenceProject

Signed-off-by: Wayne Sun <gsun@redhat.com>
@github-actions
Copy link
Copy Markdown

fullsend review is working on this — view logs

…RKFLOW_FILES

CopyAgentPEM now ensures the mint service account has secretAccessor
on existing secrets, fixing 403 when secrets were created by an older
install that granted a different SA.

Mint deployment now sets ALLOWED_WORKFLOW_FILES (default '*') and
preserves the existing value across redeployments, preventing workflow
file validation from failing after a per-org install redeploys the mint.

Signed-off-by: Wayne Sun <gsun@redhat.com>
@github-actions
Copy link
Copy Markdown

fullsend review is working on this — view logs

- CopyAgentPEM now distinguishes ErrSecretNotFound from other GetSecret
  errors instead of falling through to the copy path on any error
- EnsureOrgInMint normalizes org to lowercase before adding to
  ALLOWED_ORGS, matching the per-org install path behavior
- EnsureOrgInMint defaults ALLOWED_WORKFLOW_FILES to "*" when missing,
  preventing fail-closed denial on older mint upgrades
- Update TestCopyAgentPEM to verify ensureSecretIAM is called when
  destination secret exists
- Add cross-validation test cases for buildRepoProviderID matching
  both mint and provisioner implementations

Signed-off-by: Wayne Sun <gsun@redhat.com>
@github-actions
Copy link
Copy Markdown

fullsend review is working on this — view logs

Copy link
Copy Markdown
Contributor

@rh-hemartin rh-hemartin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need this because of the WORKFLOW_FILES thing, now when I install from main it breaks because it empties the WORKFLOW_FILES variable on the mint.

@github-actions
Copy link
Copy Markdown

fullsend review is working on this — view logs

Clarify that the mint URL is stable across redeploys within the same
project and region, and only changes when --mint-region is changed.

Signed-off-by: Wayne Sun <gsun@redhat.com>
@waynesun09 waynesun09 force-pushed the mint-url-validate branch from 93775b3 to ce0ca7e Compare May 14, 2026 14:18
@github-actions
Copy link
Copy Markdown

fullsend review is working on this — view logs

@waynesun09 waynesun09 added this pull request to the merge queue May 14, 2026
Merged via the queue into main with commit a64089b May 14, 2026
25 checks passed
@waynesun09 waynesun09 deleted the mint-url-validate branch May 14, 2026 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants