fix: merge existing orgs in ProvisionWIF to prevent WIF condition clobber

[waynesun09]

Summary

• ProvisionWIF() overwrote the WIF provider's attributeCondition with only the installing org, removing all other orgs. This caused STS 400 → mint 403 for every org not in the latest install run.
• Root cause: ProvisionWIF() (used by inference WIF and per-repo install paths) lacked the merge logic that Provision() already had.
• Adds the same read-existing → parse → union → write-merged pattern from Provision() into ProvisionWIF() via shared ensureWIFPoolAndProvider helper.
• Hardens repo name validation with allowlist regex, adds defensive lowercasing in the shared helper, and stops mutating p.cfg.WIFProvider in the repo-scoped path.
• Changes GetWIFProvider error handling from silent continue to fail-fast in both Provision() and ProvisionWIF() paths to prevent condition clobber on transient errors.

Incident: fullsend-ai/fullsend dispatch failed on PR #1111 (run 26062774013) because an earlier fullsend install for konflux-ci overwrote the WIF condition to only allow konflux-ci, blocking all other orgs (STS returned 400, mint returned 403). The WIF provider has been manually corrected.

Test plan

• All existing TestProvisionWIF_* tests pass
• New TestProvisionWIF_OrgScoped_MergesExistingOrgs verifies merge behavior
• New TestProvisionWIF_OrgScoped_GetProviderError_FailsToPreventClobber verifies fail-fast on transient errors
• New TestProvisioner_Provision_GetWIFProviderError_FailsFast verifies fail-fast in Provision() path
• New TestProvisionWIF_RepoScoped_RejectsInvalidRepo verifies allowlist validation (quotes, backslash, spaces)
• New TestParseConditionOrgs verifies case normalization and format parsing
• Full go test ./... passes

[github-actions]

Site preview

Preview: https://5abc07a1-site.fullsend-ai.workers.dev

Commit: 6fc2271c336d727ae0c5a4148a554ab1fd4a9248

[fullsend-ai-review]

Review

Findings

Low

• [correctness] internal/dispatch/gcf/provisioner.go:1179 — The strings.Contains(org, "--") check in ProvisionWIF() org validation rejects GitHub org/user names with consecutive hyphens (e.g., my--org), which GitHub allows. The existing githubOrgPattern regex already constrains the character set. This is consistent with the same check in Provision() (line 482) but could cause confusing rejections for valid org names.
  Remediation: Remove the -- check from both Provision() and ProvisionWIF(), or document the GCP-side constraint that motivates it.

• [documentation-currency] docs/guides/admin/infrastructure-reference.md:219 — The "GCF Provisioner: Provision() Flow" diagram shows the old step ordering (GetProjectNumber as the first step, before CreateServiceAccount) and does not reflect the new ensureWIFPoolAndProvider helper or its read-merge-write behavior. The diagram also says "(skip if exists)" for the WIF Provider step, which is now inaccurate — the code reads the existing provider, merges org lists, and writes the updated condition.
  Remediation: Update the diagram to reflect the current flow: CreateServiceAccount → ensureWIFPoolAndProvider (GetProjectNumber → CreateWIFPool → GetWIFProvider → merge orgs → CreateWIFProvider) → Grant Vertex AI access → Store PEMs → Deploy.

Previous run

Review

Findings

Low

• [correctness] internal/dispatch/gcf/provisioner.go:1180 — The strings.Contains(org, "--") check added to ProvisionWIF() org validation rejects GitHub org/user names with consecutive hyphens (e.g., my--org), which GitHub allows. The existing githubOrgPattern regex already constrains the character set. This is unlikely to affect real users but could cause a confusing rejection.
  Remediation: Remove the -- check, or document why consecutive hyphens are intentionally rejected if there is a GCP-side constraint.

• [documentation-currency] docs/guides/admin/infrastructure-reference.md:219 — The "GCF Provisioner: Provision() Flow" diagram shows the old step ordering (GetProjectNumber as the first step, before CreateServiceAccount) and does not reflect the new ensureWIFPoolAndProvider helper or its merge-with-existing-orgs behavior. The diagram also says "(skip if exists)" for the WIF Provider step, which is now inaccurate — the code reads the existing provider, merges org lists, and writes the updated condition.
  Remediation: Update the diagram to reflect the current flow: CreateServiceAccount → ensureWIFPoolAndProvider (GetProjectNumber → CreateWIFPool → GetWIFProvider → merge orgs → CreateWIFProvider) → Grant Vertex AI access → Store PEMs → Deploy.

Previous run (2)

Review

Findings

No findings.

Previous run (3)

Review

Findings

No findings.

Previous run (4)

Review

Findings

No findings.

Previous run (5)

Review

Findings

No findings.

The fix correctly addresses the WIF condition clobber bug by extracting ensureWIFPoolAndProvider as a shared helper that implements read-existing → parse → union → write-merged for the org-scoped WIF provider. Key observations:

• Fail-fast on GetWIFProvider errors in both Provision() and ProvisionWIF() paths prevents silent condition overwrite on transient failures — this is the right trade-off (fail the install rather than risk clobbering existing orgs).
• Case normalization in parseConditionOrgs (new strings.ToLower) and the shared helper prevents duplicate orgs from coexisting due to case differences.
• Repo-scoped path separation in ProvisionWIF correctly avoids the shared helper since each repo gets a unique provider ID via BuildRepoProviderID — no merge needed.
• p.cfg.WIFProvider mutation eliminated in the repo-scoped path (uses local providerID instead) — cleaner and avoids side effects.
• Input validation hardened: githubRepoSlugPattern for repo names, -- check added to ProvisionWIF org validation to match Provision().
• Test coverage is thorough: merge behavior, fail-fast on errors, invalid repo rejection, case normalization parsing, and the Provision() path fail-fast are all covered.

Previous run (6)

Review

Findings

No findings.

Previous run (7)

Review

Findings

No findings.

Previous run (8)

Review

Findings

No findings.

Previous run (9)

Review

Findings

No findings.

The fix correctly addresses the WIF condition clobber bug by extracting the merge logic into a shared ensureWIFPoolAndProvider helper, wiring it into both provisionSelfManaged and ProvisionWIF's org-scoped path. Key observations:

• Shared helper is correct: ensureWIFPoolAndProvider faithfully implements the read-existing → parse → union → sort → write-merged pattern, with the improvement of logging a warning on GetWIFProvider failure (previously silently skipped in provisionSelfManaged).
• Case normalization is consistent: Both callers (Provision() at line 487, ProvisionWIF explicitly) pre-lowercase org names before passing them to the helper. The parseConditionOrgs lowercasing ensures existing condition orgs are also lowered, making deduplication case-insensitive as intended.
• Repo-scoped path correctly excluded from merge: Per-repo providers have a dedicated assertion.repository == '...' condition — merge logic would be wrong here. The inline pool+provider creation with added input validation (format check, quote injection prevention) is appropriate.
• Validation hardening: The strings.Contains(org, "--") check and repo format/quote validation prevent potential GCP resource ID confusion and CEL injection.
• IAM bindings remain scoped to installing orgs only: Both paths correctly bind only the current run's orgs, not the merged set.
• Test coverage is adequate: New tests cover merge-with-existing, graceful-degradation-on-error, and parseConditionOrgs edge cases including case normalization.

Previous run (10)

Review

Findings

No findings.

The fix correctly addresses the WIF condition clobber bug by extracting the existing merge logic from provisionSelfManaged into a shared ensureWIFPoolAndProvider helper and wiring it into ProvisionWIF's org-scoped path. Key observations:

• Merge logic is faithful: The extracted helper preserves the same read-existing → parse → union → write-merged pattern, with the improvement of logging a warning on GetWIFProvider failure (previously silently skipped).
• Case normalization fix in parseConditionOrgs: Lowercasing extracted org names prevents duplicate entries when existing conditions contain mixed-case org names — correct and necessary for the dedup map to work.
• Repo-scoped path correctly excluded from merge: Per-repo providers have a dedicated assertion.repository == '...' condition and don't need org merging.
• IAM bindings scoped to installing orgs only: Both provisionSelfManaged and ProvisionWIF correctly apply IAM bindings to orgs/installingOrgs (not allOrgs), avoiding redundant binding operations for previously provisioned orgs.
• Validation consistency: The strings.Contains(org, "--") check added to ProvisionWIF matches existing validation on other entry paths.
• Test coverage is adequate: The two new tests cover the merge-with-existing and graceful-degradation-on-error scenarios. The test assertion on IAM bindings verifies the installing-org-only scoping.

Previous run (11)

Review

Findings

Low

• [style] internal/dispatch/gcf/provisioner.go — The read-existing → parse → union → sort merge pattern is now duplicated between Provision() (lines 676–693) and ProvisionWIF() (lines 1170–1191). Consider extracting a shared helper (e.g., mergeWIFOrgs(existing, new []string) []string) to reduce duplication and keep future changes in sync.
  Remediation: Extract the org-merge block into a helper function called from both sites.

Previous run (12)

Review

Findings

Low

• [style] internal/dispatch/gcf/provisioner.go — The read-existing → parse → union → sort merge pattern is now duplicated between Provision() (lines 676–693) and ProvisionWIF() (lines 1170–1185). Consider extracting a shared helper (e.g., mergeWIFOrgs(existing, new []string) []string) to reduce duplication and keep future changes in sync.
  Remediation: Extract the org-merge block into a helper function called from both sites.

[fullsend-ai-review]

Review follow-ups

Created follow-up issues for actionable non-blocking review findings:

• #1131 — The strings.Contains(org, "--") check in ProvisionWIF() org validation rejects GitHub org/user names with consecutive hyphens (e.g., my--org), which GitHub allows. The existing githubOrgPattern regex already constrains the character set. This is consistent with the same check in Provision() (line 482) but could cause confusing rejections for valid org names.
• #1132 — The GCF Provisioner: Provision() Flow diagram shows the old step ordering (GetProjectNumber as the first step) and does not reflect the new ensureWIFPoolAndProvider helper or its read-merge-write behavior. The diagram says (skip if exists) for the WIF Provider step, which is now inaccurate.

Previous run

Review follow-ups

Created follow-up issues for actionable non-blocking review findings:

• #1129 — strings.Contains(org, "--") check rejects valid GitHub org names with consecutive hyphens. GitHub allows names like my--org. The existing githubOrgPattern regex already constrains the character set.
• #1130 — The GCF Provisioner Provision() Flow diagram shows old step ordering and does not reflect the ensureWIFPoolAndProvider helper or its merge-with-existing-orgs behavior.

Previous run (2)

Review follow-ups

Created follow-up issues for actionable non-blocking review findings:

• #1118 — The read-existing, parse, union, sort merge pattern is now duplicated between Provision() (lines 676-693) and ProvisionWIF() (lines 1170-1191). Consider extracting a shared helper (e.g., mergeWIFOrgs(existing, new []string) []string) to reduce duplication and keep future changes in sync.

Previous run (3)

Review follow-ups

Created follow-up issues for actionable non-blocking review findings:

• #1116 — The read-existing / parse / union / sort merge pattern is duplicated between Provision() and ProvisionWIF(). Consider extracting a shared helper to reduce duplication.

[fullsend-ai-review]

[low] correctness

strings.Contains(org, "--") check rejects valid GitHub org names with consecutive hyphens. GitHub allows names like my--org. The existing githubOrgPattern regex already constrains the character set.

Suggested fix: Remove the -- check, or document why consecutive hyphens are intentionally rejected if there is a GCP-side constraint.

[fullsend-ai-review]

[low] correctness

The strings.Contains(org, "--") check in ProvisionWIF() org validation rejects GitHub org/user names with consecutive hyphens (e.g., my--org), which GitHub allows. The existing githubOrgPattern regex already constrains the character set. This is consistent with the same check in Provision() (line 482) but could cause confusing rejections for valid org names.

Suggested fix: Remove the -- check from both Provision() and ProvisionWIF(), or document the GCP-side constraint that motivates it.