fix(wif): preserve GitHub canonical case in attributeCondition and IAM principals

[waynesun09]

Summary

Fixes #3678 — WIF attributeCondition forces lowercase org/repo, rejecting auth for mixed-case GitHub orgs.

The WIF provisioner lowercased org/repo names before substituting them into CEL attributeCondition literals and IAM principalSet paths. Since GitHub's OIDC token preserves canonical case and Google STS compares claims byte-for-byte, any org/repo with uppercase letters (e.g. RHEcosystemAppEng/sdlc-plugins) permanently failed authentication with unauthorized_client: The given credential is rejected by the attribute condition.

Changes

• parseConditionOrgs — preserves case from existing CEL conditions instead of lowercasing
• ensureWIFPoolAndProvider — case-insensitive dedup using lowered map keys, preserving canonical-case values; installing orgs take precedence when casing differs
• ProvisionWIF repo path — split into parts (original case for CEL/IAM) and partsLower (for BuildRepoProviderID and validation)
• ProvisionWIF org path — preserves original case in orgs slice while deduping case-insensitively
• EnsureOrgInWIFCondition — case-insensitive dedup preserving canonical case
• RemoveOrgFromWIFCondition — uses strings.EqualFold for case-insensitive matching
• IAM grant helpers — removed defensive strings.ToLower since attribute.repository maps directly from the assertion without normalization
• Provision — stopped mutating p.cfg.GitHubOrgs entries to lowercase

What stays lowered (correct behavior)

• BuildRepoProviderID — GCP resource IDs require lowercase
• EnsureOrgInMint / RegisterPerRepoWIF — env var bookkeeping (ALLOWED_ORGS, PER_REPO_WIF_REPOS)
• RemoveOrgFromMint / RemoveRepoFromMint — env var bookkeeping

Test plan

• Updated TestProvisionWIF_PreservesOrgCase — condition preserves original case
• Updated TestProvisionWIF_RepoScoped_PreservesRepoCase — condition and IAM binding preserve case, provider ID still lowered
• Updated TestParseConditionOrgs — mixed case preserved in parsed output
• Updated TestEnsureOrgInWIFCondition_AddsOrgAndStripsPlaceholder — preserves case
• Added TestProvisionWIF_OrgScoped_PreservesOrgCase — mixed-case org in condition and IAM
• Added TestProvisionWIF_OrgScoped_MergeDedupsCase — installing org case wins on merge
• Added TestRemoveOrgFromWIFCondition_CaseInsensitiveMatch — removes by case-insensitive match
• Full go test ./internal/dispatch/gcf/ passes
• go vet clean

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:23 PM UTC · Completed 5:38 PM UTC
Commit: 0438979 · View workflow run →

[qodo-code-review]

PR Summary by Qodo

Fix WIF: preserve GitHub canonical case in CEL conditions and IAM principals

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Preserve mixed-case GitHub org/repo strings in WIF CEL attributeCondition literals.
• Keep provider ID generation/validation lowercase while deduping orgs case-insensitively.
• Update and add tests to prevent regressions for org- and repo-scoped WIF.

Diagram

graph TD
  GH{{"GitHub OIDC token"}} --> P["Provisioner"] --> CEL["CEL attributeCondition"] --> GCP[("GCP WIF Provider")]
  P --> IAM["IAM principalSet member"] --> GCP
  P --> DEDUP["Case-insensitive dedup"] --> CEL
  subgraph Legend
    direction LR
    _ext{{"External"}} ~~~ _svc["Service"] ~~~ _plat[("GCP resource")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Make CEL checks case-insensitive (lower(assertion.*) comparisons)

• ➕ Users can enter org/repo in any case and still authenticate
• ➕ Avoids relying on stored canonical casing in conditions
• ➖ Depends on available CEL string functions and their exact semantics in Google STS
• ➖ Harder to read/debug conditions; more complex expressions
• ➖ May create subtle mismatches if only one side is normalized

2. Resolve canonical casing via GitHub API before writing conditions

• ➕ Conditions always use GitHub’s canonical org/repo casing regardless of user input
• ➖ Adds network dependency and failure modes to provisioning
• ➖ Requires auth/rate-limit handling; more operational complexity

Recommendation: Keep the PR’s approach: preserve the exact canonical case in security-critical CEL literals and IAM principalSet paths, while doing internal bookkeeping (dedup/provider IDs) case-insensitively. This aligns with Google STS byte-for-byte comparisons and avoids introducing CEL-function or external-API dependencies.

Files changed (2) +91 / -50

Bug fix (1) +33 / -43

provisioner.goPreserve org/repo casing for WIF CEL conditions and IAM principalSet members +33/-43

Preserve org/repo casing for WIF CEL conditions and IAM principalSet members

• Stops lowercasing GitHub org/repo names when constructing WIF provider attributeCondition strings and IAM principalSet member paths. Introduces case-insensitive dedup/merge using lowercased map keys while preserving canonical-case values, and keeps repo provider ID generation lowercased via separate lowercase parts.

internal/dispatch/gcf/provisioner.go

Tests (1) +58 / -7

provisioner_test.goAdd/adjust tests for canonical-case preservation and case-insensitive org removal +58/-7

Add/adjust tests for canonical-case preservation and case-insensitive org removal

• Renames and updates existing tests to assert that org/repo casing is preserved in attributeCondition and IAM bindings while provider IDs remain lowercased. Adds new coverage for org-scoped case preservation, merge dedup precedence when casing differs, and case-insensitive org removal from existing conditions.

internal/dispatch/gcf/provisioner_test.go

[github-actions]

Site preview

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

Commit: 27398bb3717bffe7d56950c413eff3cbe8675294

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 WIF attributeCondition forces lowercase org/repo, rejecting auth for mixed-case GitHub orgs

✅ Compliance rules (platform): 54 rules

✅ Skills: writing-user-docs, writing-adrs

1. EnsureOrg flips existing case 🐞 Bug ≡ Correctness

Description

EnsureOrgInWIFCondition/ensureWIFPoolAndProvider case-insensitively deduplicates orgs by lowercasing
the key but still overwrites the stored org string when the caller/installer provides a
different-case variant, which rewrites the emitted CEL condition literals. Because the provider’s
attributeMapping is a direct pass-through and CEL string comparisons are case-sensitive, this casing
rewrite changes which assertions match and can unintentionally revoke access that previously matched
the original casing.

Code

internal/dispatch/gcf/provisioner.go[R1281-1293]

+	merged := make(map[string]string)
	for _, o := range existingOrgs {
		if o != PlaceholderOrg {
-			merged[o] = true
+			merged[strings.ToLower(o)] = o
		}
	}
-	merged[org] = true
+	merged[strings.ToLower(org)] = org

	allOrgs := make([]string, 0, len(merged))
-	for o := range merged {
+	for _, o := range merged {
		allOrgs = append(allOrgs, o)
	}
	sort.Strings(allOrgs)

Relevance

⭐ Low

PR #1113 normalized org casing (lowercased) in WIF condition parsing/merge; team didn’t treat casing
changes as breakage.

PR-#1113

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The cited logic constructs a merged map keyed by strings.ToLower(org) and then unconditionally
assigns merged[key] = org, so if an org already exists in the provider condition and a later call
provides the same org with different casing, the map value (the casing that will be emitted) is
rewritten. The rebuilt condition is produced via buildAttributeCondition, which emits
case-sensitive CEL string equality / membership literals, while the WIF provider attributeMapping
passes GitHub assertion fields through without normalizing case (no .lower()), so changing the
literal casing directly changes which assertion strings satisfy the condition.

internal/dispatch/gcf/provisioner.go[1280-1304]
internal/dispatch/gcf/provisioner.go[1099-1112]
internal/dispatch/gcf/gcp.go[239-260]
internal/dispatch/gcf/provisioner.go[1194-1220]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`EnsureOrgInWIFCondition` / `ensureWIFPoolAndProvider` is intended to be an idempotent “ensure/merge” operation, but its case-insensitive dedup currently still rewrites an existing org’s casing when the caller supplies a different-case string. Because the resulting `AttributeCondition` uses case-sensitive CEL string literals and the provider `attributeMapping` does not normalize assertion values, this implicit casing rewrite changes condition semantics and can unintentionally revoke access.

## Issue Context
This code path reads an existing WIF provider, parses orgs out of the existing condition, merges in installing/caller orgs, and rebuilds the provider’s `AttributeCondition`. The merge uses a lowercased key for deduplication, but currently overwrites the stored value for that key, allowing later inputs to change the canonical casing used in the emitted CEL condition.

## Fix Focus Areas
- internal/dispatch/gcf/provisioner.go[1194-1220]
- internal/dispatch/gcf/provisioner.go[1280-1304]

## Suggested change
- Use the lowercased key only for presence detection, but do not overwrite an existing value for that key (preserve the first-seen value, typically the existing provider’s casing): e.g., `k := strings.ToLower(org); if _, ok := merged[k]; !ok { merged[k] = org }`.
- If intentionally changing casing is a requirement, introduce an explicit override mode/flag or a separate operation (e.g., `SetOrgCasingInWIFCondition`) rather than mutating casing implicitly during ensure/merge.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[fullsend-ai-review]

Review

Outcome: Approve

Re-review: Head advanced from e4eab89c5c → 27398bb371. Prior review was approve (provenance: app-verified). The incremental change is a single commit extending the WIF case-preservation fix to the repo-enroll CLI path and fixing dry-run previews.

Summary

This PR fixes a genuine authentication bug (#3678) where the WIF provisioner lowercased org/repo names before substituting them into CEL attributeCondition literals and IAM principalSet paths. Since GitHub's OIDC token preserves canonical case and Google STS compares claims byte-for-byte, any org/repo with uppercase letters (e.g., RHEcosystemAppEng/sdlc-plugins) permanently failed authentication.

The fix correctly separates two concerns:

• CEL conditions and IAM principals — now preserve the original case, matching GitHub's OIDC token claims
• Internal bookkeeping (GCP resource IDs, env vars, dedup) — remains lowercase where required

Incremental Change (commit 4)

Commit 27398bb371 (fix(mint): preserve case in repo-enroll WIF condition and dry-run previews) completes the end-to-end fix by extending case preservation to the repo-enroll CLI path and fixing misleading dry-run output:

• runMintEnrollRepo — saves originalCaseRepo before lowering; passes it to Config.Repo so ProvisionWIF receives original case for the CEL condition and IAM principal, while the lowered repoFullName continues to be used for PER_REPO_WIF_REPOS env var bookkeeping
• runMintEnrollOrg dry-run — the "Would add X to WIF provider condition" message now uses originalCaseOrg instead of the lowered org, matching what the actual provisioning call would produce
• runInferenceProvisionDryRun — removes strings.ToLower() from both repo-scoped and org-scoped condition previews, so dry-run output matches the actual provisioning behavior
• LastWIFProviderCondition — new exported test helper in fakeclient.go that extracts the attribute condition from a fake client's lastWIFProviderConfig for cross-package assertions, replacing the UpdateWIFProvider state-update approach from commit 3
• fakeGCFClient.UpdateWIFProvider — reverts the state update added in commit 3 (setting f.wifProvider) since LastWIFProviderCondition provides a cleaner cross-package assertion path; no existing tests relied on read-after-write via GetWIFProvider
• 3 new tests — TestRunMintEnrollOrg_DryRunPreservesCaseInPreview, TestRunMintEnrollOrg_PreservesCaseInWIFCondition (refactored), TestRunMintEnrollRepo_PreservesCaseInWIFCondition

Reviewed Dimensions

Dimension	Result
Correctness	✅ originalCaseRepo pattern is correct — lowered repoFullName is validated before originalCaseRepo is used. ProvisionWIF re-validates via partsLower (commits 1-3). RegisterPerRepoWIF independently lowercases its argument. No test relies on GetWIFProvider returning state after UpdateWIFProvider — removal is safe. runInferenceProvisionDryRun receives original-case values from parseOrgOrRepo which does not lowercase.
Security	✅ No injection risk — validateOrgName restricts to [a-zA-Z0-9-], ValidateRepoSlug restricts to [a-zA-Z0-9._-]. originalCaseOrg/originalCaseRepo can only differ from validated lowered values in letter casing. CEL metacharacters (single quotes, backslashes, parentheses) are impossible. Validation ordering is safe: early return on failure prevents use of unvalidated input. ProvisionWIF applies defense-in-depth validation via partsLower. No fail-open paths.
Intent & coherence	✅ Within scope of #3678 (type/bug, priority/high). Commit fixes the repo-enroll CLI path and dry-run previews — both directly affected by the original bug. Not a breaking change — all-lowercase repos are unaffected; mixed-case repos go from broken to working.
Style & conventions	✅ originalCaseRepo naming follows the original<Descriptor> pattern established by originalCaseOrg. LastWIFProviderCondition follows Go export conventions with a doc comment explaining purpose, return behavior, and edge cases. Test names follow Test<FunctionName>_<Scenario> convention.
Documentation	✅ No documentation references the old lowercasing behavior for WIF conditions. Docs use shell variable placeholders ($ORG_NAME) rather than concrete lowercased examples, so they remain correct.
Cross-repo contracts	✅ All changes within internal/ packages — Go's internal package restriction prevents external imports.

Previous run

Review

Outcome: Approve

Re-review: Head advanced from 9a935bdc0a → e4eab89c5c. Prior review was approve (provenance: app-verified). The incremental change is a single commit extending the WIF case-preservation fix from the provisioner layer to the CLI layer.

Summary

This PR fixes a genuine authentication bug (#3678) where the WIF provisioner lowercased org/repo names before substituting them into CEL attributeCondition literals and IAM principalSet paths. Since GitHub's OIDC token preserves canonical case and Google STS compares claims byte-for-byte, any org/repo with uppercase letters (e.g., RHEcosystemAppEng/sdlc-plugins) permanently failed authentication.

The fix correctly separates two concerns:

• CEL conditions and IAM principals — now preserve the original case, matching GitHub's OIDC token claims
• Internal bookkeeping (GCP resource IDs, env vars, dedup) — remains lowercase where required

Incremental Change (commit 3)

Commit e4eab89c5c (fix(mint): preserve org case for WIF condition on CLI enroll/unenroll) completes the end-to-end fix by extending case preservation to the CLI layer:

• runMintEnrollOrg — saves originalCaseOrg before lowering; passes it to EnsureOrgInWIFCondition while continuing to use the lowered org for validation, env var operations, and display
• runMintUnenrollOrg — same pattern; passes originalCaseOrg to RemoveOrgFromWIFCondition
• fakeGCFClient.UpdateWIFProvider — now updates f.wifProvider state so GetWIFProvider returns the updated condition (enabling read-after-write test assertions)
• TestRunMintEnrollOrg_PreservesCaseInWIFCondition — verifies the full enroll flow preserves mixed-case org ("AcmeCorp") in the WIF condition

Reviewed Dimensions

Dimension	Result
Correctness	✅ originalCaseOrg pattern is correct — all lowered-org uses (validation, placeholder check, env var ops, display) remain on org; only WIF condition calls receive original case. Fakeclient change is additive and does not affect existing tests. No assertion weakening detected.
Security	✅ No injection risk — originalCaseOrg differs from the validated lowered copy only in letter casing within the [a-zA-Z0-9-] alphabet enforced by GitHubOrgPattern. CEL metacharacters are impossible. Validation ordering is safe (early return on failure prevents use of unvalidated input). No fail-open paths introduced.
Intent & coherence	✅ Within scope of #3678 (type/bug, priority/high). Commit prefix fix(mint): correctly identifies the CLI mint subsystem. No scope creep — all changes serve the single purpose of case preservation.
Style & conventions	✅ originalCaseOrg naming follows original<Descriptor> pattern. Test name follows Test<FunctionName>_<Scenario> convention. Fakeclient state update is consistent with read-after-write test patterns.
Documentation	✅ No documentation references the old lowercasing behavior for WIF conditions. No staleness found.
Cross-repo contracts	✅ All changes within internal/ packages — Go's internal package restriction prevents external imports.

Previous run

Review

Outcome: Approve

Re-review: Head advanced from 043897993b → 9a935bdc0a. Prior review was approve (provenance: app-verified). The incremental change is a single test-only commit addressing the prior review's low finding.

Summary

This PR fixes a genuine authentication bug (#3678) where the WIF provisioner lowercased org/repo names before substituting them into CEL attributeCondition literals and IAM principalSet paths. Since GitHub's OIDC token preserves canonical case and Google STS compares claims byte-for-byte, any org/repo with uppercase letters (e.g., RHEcosystemAppEng/sdlc-plugins) permanently failed authentication.

The fix correctly separates two concerns:

• CEL conditions and IAM principals — now preserve the original case, matching GitHub's OIDC token claims
• Internal bookkeeping (GCP resource IDs, env vars, dedup) — remains lowercase where required

Incremental Change

The new commit (9a935bdc0a) adds TestEnsureOrgInWIFCondition_ReEnrollmentInstallingCaseWins, which directly addresses the prior review's low finding about merge precedence during re-enrollment. The test verifies that when an org re-enrolls with different casing ("ACME" over existing 'acme'), the installing org's case wins — documenting the intended "installing orgs take precedence" behavior.

Reviewed Dimensions

Dimension	Result
Correctness	✅ Production code unchanged from prior approved review. New test correctly verifies re-enrollment case precedence. No test weakening detected — all assertion changes reflect the intentional behavior change (case preservation), not assertion loosening.
Security	✅ No injection risk — GitHubOrgPattern (^[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,37}[a-zA-Z0-9])?$) prevents CEL metacharacters. IAM principals correctly use canonical case to match GitHub OIDC token claims. Case-insensitive dedup via lowered map keys prevents bypass. RemoveOrgFromWIFCondition uses strings.EqualFold — no fail-open.
Intent & coherence	✅ Directly addresses #3678 (type/bug, priority/high). New commit stays within authorized scope — test addition per review feedback. fix(wif): prefix correct. Not a breaking change.
Style & conventions	✅ New test follows existing Test<FunctionName>_<ScenarioDescription> naming pattern, uses the standard NewFakeGCFClient(WithFakeWIFProvider(...)) setup, and matches assertion style of sibling tests.
Documentation	✅ Test-only incremental change — no documentation staleness.
Cross-repo contracts	✅ All changes within internal/dispatch/gcf/ — Go's internal package restriction prevents external imports.

Test Assessment

The prior review's low finding has been fully addressed. Test coverage now includes: case preservation for org-scoped and repo-scoped provisioning, case-insensitive merge dedup, case-insensitive removal, provider ID lowering, and the re-enrollment precedence scenario. The test suite exercises the critical invariants that the provider ID remains lowered (GCP requirement) while CEL conditions and IAM bindings preserve canonical case (GitHub OIDC requirement).

Previous run

Review

Outcome: Approve

Summary

This PR fixes a genuine authentication bug (#3678) where the WIF provisioner lowercased org/repo names before substituting them into CEL attributeCondition literals and IAM principalSet paths. Since GitHub's OIDC token preserves canonical case and Google STS compares claims byte-for-byte, any org/repo with uppercase letters (e.g., RHEcosystemAppEng/sdlc-plugins) permanently failed authentication.

The fix correctly separates two concerns:

• CEL conditions and IAM principals — now preserve the original case, matching GitHub's OIDC token claims
• Internal bookkeeping (GCP resource IDs, env vars, dedup) — remains lowercase where required

Reviewed Dimensions

Dimension	Result
Correctness	✅ Logic is sound. Merge dedup uses lowered map keys with case-preserved values. Validation checks correctly use partsLower for pattern matching while parts preserves original case for CEL/IAM. BuildRepoProviderID still receives lowercase (GCP resource IDs require it).
Security	✅ No injection risk — GitHubOrgPattern and githubRepoSlugPattern prevent CEL metacharacters. No fail-open — PlaceholderOrg fallback preserved. IAM principal paths use validated values only.
Intent & coherence	✅ Directly addresses #3678 (type/bug, priority/high). fix(wif): prefix is correct per COMMITS.md. Not a breaking change — all-lowercase orgs are unaffected; mixed-case orgs go from broken to working.
Style & conventions	✅ map[string]string for case-insensitive dedup, strings.EqualFold for matching, copy() for array duplication — all idiomatic Go and consistent with existing codebase patterns.
Documentation	✅ No stale documentation references found.
Cross-repo contracts	✅ All changes within internal/dispatch/gcf/ — Go's internal package restriction prevents external imports.

Findings

[low] Merge precedence during re-enrollment could cause condition case change — In EnsureOrgInWIFCondition and ensureWIFPoolAndProvider, the installing org's case always overwrites the existing condition's case (by design: "Installing orgs take precedence"). This enables fixing broken conditions via re-provisioning, but also means re-enrollment with wrong case (e.g., user types acmecorp when canonical name is AcmeCorp) will change a working condition. Consider adding a test for this scenario (e.g., EnsureOrgInWIFCondition("ACME") when 'acme' already exists in the condition) to document the intended behavior. (internal/dispatch/gcf/provisioner.go, EnsureOrgInWIFCondition merge logic)

Test Assessment

Test coverage is strong: 7 tests updated or added covering case preservation for org-scoped, repo-scoped, merge dedup, case-insensitive removal, and provider ID lowering. The test TestProvisionWIF_RepoScoped_PreservesRepoCase correctly verifies the critical invariant that the provider ID is lowered while the condition and IAM binding preserve original case.

---

Labels: PR modifies WIF provisioning in the dispatch component

---

Labels: PR modifies WIF provisioning across dispatch, CLI mint, and CLI inference components

---

Labels: PR modifies CLI mint enrollment and inference dry-run paths alongside dispatch WIF provisioning

Previous run

Review

Outcome: Approve

Re-review: Head advanced from 9a935bdc0a → e4eab89c5c. Prior review was approve (provenance: app-verified). The incremental change is a single commit extending the WIF case-preservation fix from the provisioner layer to the CLI layer.

Summary

This PR fixes a genuine authentication bug (#3678) where the WIF provisioner lowercased org/repo names before substituting them into CEL attributeCondition literals and IAM principalSet paths. Since GitHub's OIDC token preserves canonical case and Google STS compares claims byte-for-byte, any org/repo with uppercase letters (e.g., RHEcosystemAppEng/sdlc-plugins) permanently failed authentication.

The fix correctly separates two concerns:

• CEL conditions and IAM principals — now preserve the original case, matching GitHub's OIDC token claims
• Internal bookkeeping (GCP resource IDs, env vars, dedup) — remains lowercase where required

Incremental Change (commit 3)

Commit e4eab89c5c (fix(mint): preserve org case for WIF condition on CLI enroll/unenroll) completes the end-to-end fix by extending case preservation to the CLI layer:

• runMintEnrollOrg — saves originalCaseOrg before lowering; passes it to EnsureOrgInWIFCondition while continuing to use the lowered org for validation, env var operations, and display
• runMintUnenrollOrg — same pattern; passes originalCaseOrg to RemoveOrgFromWIFCondition
• fakeGCFClient.UpdateWIFProvider — now updates f.wifProvider state so GetWIFProvider returns the updated condition (enabling read-after-write test assertions)
• TestRunMintEnrollOrg_PreservesCaseInWIFCondition — verifies the full enroll flow preserves mixed-case org ("AcmeCorp") in the WIF condition

Reviewed Dimensions

Dimension	Result
Correctness	✅ originalCaseOrg pattern is correct — all lowered-org uses (validation, placeholder check, env var ops, display) remain on org; only WIF condition calls receive original case. Fakeclient change is additive and does not affect existing tests. No assertion weakening detected.
Security	✅ No injection risk — originalCaseOrg differs from the validated lowered copy only in letter casing within the [a-zA-Z0-9-] alphabet enforced by GitHubOrgPattern. CEL metacharacters are impossible. Validation ordering is safe (early return on failure prevents use of unvalidated input). No fail-open paths introduced.
Intent & coherence	✅ Within scope of #3678 (type/bug, priority/high). Commit prefix fix(mint): correctly identifies the CLI mint subsystem. No scope creep — all changes serve the single purpose of case preservation.
Style & conventions	✅ originalCaseOrg naming follows original<Descriptor> pattern. Test name follows Test<FunctionName>_<Scenario> convention. Fakeclient state update is consistent with read-after-write test patterns.
Documentation	✅ No documentation references the old lowercasing behavior for WIF conditions. No staleness found.
Cross-repo contracts	✅ All changes within internal/ packages — Go's internal package restriction prevents external imports.

---

Labels: PR modifies WIF provisioning across dispatch and CLI mint components

Previous run (2)

Review

Outcome: Approve

Re-review: Head advanced from 043897993b → 9a935bdc0a. Prior review was approve (provenance: app-verified). The incremental change is a single test-only commit addressing the prior review's low finding.

Summary

This PR fixes a genuine authentication bug (#3678) where the WIF provisioner lowercased org/repo names before substituting them into CEL attributeCondition literals and IAM principalSet paths. Since GitHub's OIDC token preserves canonical case and Google STS compares claims byte-for-byte, any org/repo with uppercase letters (e.g., RHEcosystemAppEng/sdlc-plugins) permanently failed authentication.

The fix correctly separates two concerns:

• CEL conditions and IAM principals — now preserve the original case, matching GitHub's OIDC token claims
• Internal bookkeeping (GCP resource IDs, env vars, dedup) — remains lowercase where required

Incremental Change

The new commit (9a935bdc0a) adds TestEnsureOrgInWIFCondition_ReEnrollmentInstallingCaseWins, which directly addresses the prior review's low finding about merge precedence during re-enrollment. The test verifies that when an org re-enrolls with different casing ("ACME" over existing 'acme'), the installing org's case wins — documenting the intended "installing orgs take precedence" behavior.

Reviewed Dimensions

Dimension	Result
Correctness	✅ Production code unchanged from prior approved review. New test correctly verifies re-enrollment case precedence. No test weakening detected — all assertion changes reflect the intentional behavior change (case preservation), not assertion loosening.
Security	✅ No injection risk — GitHubOrgPattern (^[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,37}[a-zA-Z0-9])?$) prevents CEL metacharacters. IAM principals correctly use canonical case to match GitHub OIDC token claims. Case-insensitive dedup via lowered map keys prevents bypass. RemoveOrgFromWIFCondition uses strings.EqualFold — no fail-open.
Intent & coherence	✅ Directly addresses #3678 (type/bug, priority/high). New commit stays within authorized scope — test addition per review feedback. fix(wif): prefix correct. Not a breaking change.
Style & conventions	✅ New test follows existing Test<FunctionName>_<ScenarioDescription> naming pattern, uses the standard NewFakeGCFClient(WithFakeWIFProvider(...)) setup, and matches assertion style of sibling tests.
Documentation	✅ Test-only incremental change — no documentation staleness.
Cross-repo contracts	✅ All changes within internal/dispatch/gcf/ — Go's internal package restriction prevents external imports.

Test Assessment

The prior review's low finding has been fully addressed. Test coverage now includes: case preservation for org-scoped and repo-scoped provisioning, case-insensitive merge dedup, case-insensitive removal, provider ID lowering, and the re-enrollment precedence scenario. The test suite exercises the critical invariants that the provider ID remains lowered (GCP requirement) while CEL conditions and IAM bindings preserve canonical case (GitHub OIDC requirement).

Previous run

Review

Outcome: Approve

Summary

This PR fixes a genuine authentication bug (#3678) where the WIF provisioner lowercased org/repo names before substituting them into CEL attributeCondition literals and IAM principalSet paths. Since GitHub's OIDC token preserves canonical case and Google STS compares claims byte-for-byte, any org/repo with uppercase letters (e.g., RHEcosystemAppEng/sdlc-plugins) permanently failed authentication.

The fix correctly separates two concerns:

• CEL conditions and IAM principals — now preserve the original case, matching GitHub's OIDC token claims
• Internal bookkeeping (GCP resource IDs, env vars, dedup) — remains lowercase where required

Reviewed Dimensions

Dimension	Result
Correctness	✅ Logic is sound. Merge dedup uses lowered map keys with case-preserved values. Validation checks correctly use partsLower for pattern matching while parts preserves original case for CEL/IAM. BuildRepoProviderID still receives lowercase (GCP resource IDs require it).
Security	✅ No injection risk — GitHubOrgPattern and githubRepoSlugPattern prevent CEL metacharacters. No fail-open — PlaceholderOrg fallback preserved. IAM principal paths use validated values only.
Intent & coherence	✅ Directly addresses #3678 (type/bug, priority/high). fix(wif): prefix is correct per COMMITS.md. Not a breaking change — all-lowercase orgs are unaffected; mixed-case orgs go from broken to working.
Style & conventions	✅ map[string]string for case-insensitive dedup, strings.EqualFold for matching, copy() for array duplication — all idiomatic Go and consistent with existing codebase patterns.
Documentation	✅ No stale documentation references found.
Cross-repo contracts	✅ All changes within internal/dispatch/gcf/ — Go's internal package restriction prevents external imports.

Findings

[low] Merge precedence during re-enrollment could cause condition case change — In EnsureOrgInWIFCondition and ensureWIFPoolAndProvider, the installing org's case always overwrites the existing condition's case (by design: "Installing orgs take precedence"). This enables fixing broken conditions via re-provisioning, but also means re-enrollment with wrong case (e.g., user types acmecorp when canonical name is AcmeCorp) will change a working condition. Consider adding a test for this scenario (e.g., EnsureOrgInWIFCondition("ACME") when 'acme' already exists in the condition) to document the intended behavior. (internal/dispatch/gcf/provisioner.go, EnsureOrgInWIFCondition merge logic)

Test Assessment

Test coverage is strong: 7 tests updated or added covering case preservation for org-scoped, repo-scoped, merge dedup, case-insensitive removal, and provider ID lowering. The test TestProvisionWIF_RepoScoped_PreservesRepoCase correctly verifies the critical invariant that the provider ID is lowered while the condition and IAM binding preserve original case.

---

Labels: PR modifies WIF provisioning in the dispatch component

Previous run (3)

Review

Outcome: Approve

Re-review: Head advanced from 043897993b → 9a935bdc0a. Prior review was approve (provenance: app-verified). The incremental change is a single test-only commit addressing the prior review's low finding.

Summary

This PR fixes a genuine authentication bug (#3678) where the WIF provisioner lowercased org/repo names before substituting them into CEL attributeCondition literals and IAM principalSet paths. Since GitHub's OIDC token preserves canonical case and Google STS compares claims byte-for-byte, any org/repo with uppercase letters (e.g., RHEcosystemAppEng/sdlc-plugins) permanently failed authentication.

The fix correctly separates two concerns:

• CEL conditions and IAM principals — now preserve the original case, matching GitHub's OIDC token claims
• Internal bookkeeping (GCP resource IDs, env vars, dedup) — remains lowercase where required

Incremental Change

The new commit (9a935bdc0a) adds TestEnsureOrgInWIFCondition_ReEnrollmentInstallingCaseWins, which directly addresses the prior review's low finding about merge precedence during re-enrollment. The test verifies that when an org re-enrolls with different casing ("ACME" over existing 'acme'), the installing org's case wins — documenting the intended "installing orgs take precedence" behavior.

Reviewed Dimensions

Dimension	Result
Correctness	✅ Production code unchanged from prior approved review. New test correctly verifies re-enrollment case precedence. No test weakening detected — all assertion changes reflect the intentional behavior change (case preservation), not assertion loosening.
Security	✅ No injection risk — GitHubOrgPattern (^[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,37}[a-zA-Z0-9])?$) prevents CEL metacharacters. IAM principals correctly use canonical case to match GitHub OIDC token claims. Case-insensitive dedup via lowered map keys prevents bypass. RemoveOrgFromWIFCondition uses strings.EqualFold — no fail-open.
Intent & coherence	✅ Directly addresses #3678 (type/bug, priority/high). New commit stays within authorized scope — test addition per review feedback. fix(wif): prefix correct. Not a breaking change.
Style & conventions	✅ New test follows existing Test<FunctionName>_<ScenarioDescription> naming pattern, uses the standard NewFakeGCFClient(WithFakeWIFProvider(...)) setup, and matches assertion style of sibling tests.
Documentation	✅ Test-only incremental change — no documentation staleness.
Cross-repo contracts	✅ All changes within internal/dispatch/gcf/ — Go's internal package restriction prevents external imports.

Test Assessment

The prior review's low finding has been fully addressed. Test coverage now includes: case preservation for org-scoped and repo-scoped provisioning, case-insensitive merge dedup, case-insensitive removal, provider ID lowering, and the re-enrollment precedence scenario. The test suite exercises the critical invariants that the provider ID remains lowered (GCP requirement) while CEL conditions and IAM bindings preserve canonical case (GitHub OIDC requirement).

Previous run (4)

Review

Outcome: Approve

Summary

This PR fixes a genuine authentication bug (#3678) where the WIF provisioner lowercased org/repo names before substituting them into CEL attributeCondition literals and IAM principalSet paths. Since GitHub's OIDC token preserves canonical case and Google STS compares claims byte-for-byte, any org/repo with uppercase letters (e.g., RHEcosystemAppEng/sdlc-plugins) permanently failed authentication.

The fix correctly separates two concerns:

• CEL conditions and IAM principals — now preserve the original case, matching GitHub's OIDC token claims
• Internal bookkeeping (GCP resource IDs, env vars, dedup) — remains lowercase where required

Reviewed Dimensions

Dimension	Result
Correctness	✅ Logic is sound. Merge dedup uses lowered map keys with case-preserved values. Validation checks correctly use partsLower for pattern matching while parts preserves original case for CEL/IAM. BuildRepoProviderID still receives lowercase (GCP resource IDs require it).
Security	✅ No injection risk — GitHubOrgPattern and githubRepoSlugPattern prevent CEL metacharacters. No fail-open — PlaceholderOrg fallback preserved. IAM principal paths use validated values only.
Intent & coherence	✅ Directly addresses #3678 (type/bug, priority/high). fix(wif): prefix is correct per COMMITS.md. Not a breaking change — all-lowercase orgs are unaffected; mixed-case orgs go from broken to working.
Style & conventions	✅ map[string]string for case-insensitive dedup, strings.EqualFold for matching, copy() for array duplication — all idiomatic Go and consistent with existing codebase patterns.
Documentation	✅ No stale documentation references found.
Cross-repo contracts	✅ All changes within internal/dispatch/gcf/ — Go's internal package restriction prevents external imports.

Findings

[low] Merge precedence during re-enrollment could cause condition case change — In EnsureOrgInWIFCondition and ensureWIFPoolAndProvider, the installing org's case always overwrites the existing condition's case (by design: "Installing orgs take precedence"). This enables fixing broken conditions via re-provisioning, but also means re-enrollment with wrong case (e.g., user types acmecorp when canonical name is AcmeCorp) will change a working condition. Consider adding a test for this scenario (e.g., EnsureOrgInWIFCondition("ACME") when 'acme' already exists in the condition) to document the intended behavior. (internal/dispatch/gcf/provisioner.go, EnsureOrgInWIFCondition merge logic)

Test Assessment

Test coverage is strong: 7 tests updated or added covering case preservation for org-scoped, repo-scoped, merge dedup, case-insensitive removal, and provider ID lowering. The test TestProvisionWIF_RepoScoped_PreservesRepoCase correctly verifies the critical invariant that the provider ID is lowered while the condition and IAM binding preserve original case.

---

Labels: PR modifies WIF provisioning in the dispatch component

[codecov]

Codecov Report

❌ Patch coverage is 88.57143% with 4 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/cli/mint.go	71.42%	0 Missing and 2 partials ⚠️
internal/dispatch/gcf/provisioner.go	90.47%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:50 PM UTC · Completed 7:57 PM UTC
Commit: 9a935bd · View workflow run →

[ralphbean]

LGTM. One non-blocking note inline.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:34 PM UTC · Completed 1:45 PM UTC
Commit: e4eab89 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:11 PM UTC · Completed 2:23 PM UTC
Commit: 27398bb · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 2:29 PM UTC · Completed 2:36 PM UTC
Commit: 27398bb · View workflow run →

[fullsend-ai-retro]

PR #3692 fixed a WIF authentication bug where case-preserving org/repo names in CEL conditions were defeated by pre-lowercasing in CLI callers. The review agent approved the initial fix but missed that CLI callers (runMintEnrollOrg, runMintUnenrollOrg in mint.go) still lowercased the org before calling the fixed provisioner functions. Human reviewer ralphbean caught this gap, prompting 2 additional fix commits and 2 additional review runs. The review agent's [low] finding about re-enrollment precedence testing was valuable and acted upon immediately. One proposal filed as evidence for #2915 (call-chain tracing for bug-fix PRs).

Proposals filed

• Evidence for #2915: review agent missed caller-side normalization defeating WIF case-preservation fix (in fullsend-ai/fullsend)