feat: rename GCP CLI flags to provider-agnostic inference flags

[waynesun09]

Summary

• Rename --gcp-project, --gcp-region, --gcp-wif-provider to --inference-project, --inference-region, --inference-wif-provider
• Default --inference-region to global so only --inference-project is required to enable inference
• Auto-discover or create WIF pool+provider when --inference-wif-provider is omitted in per-org mode

Test plan

• go build ./... compiles cleanly
• go vet ./... passes
• go test ./internal/cli/ — all install command tests pass
• Tests verify old flag names (--gcp-project etc.) are no longer accepted
• Tests verify --inference-region defaults to global

Closes #884

[github-actions]

fullsend review is working on this — view logs

[github-actions]

Site preview

Preview: https://3dbf63a5-site.fullsend-ai.workers.dev

Commit: 20b21930a933a39c4251a321a1c9d9d0d6c4f387

[fullsend-ai-review]

Review: #905

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

Summary

This PR correctly renames --gcp-project, --gcp-region, and --gcp-wif-provider to provider-agnostic --inference-* equivalents, defaults --inference-region to global, and adds WIF auto-provisioning for per-org mode. The code changes are mechanically sound: flag renames are consistent across declaration, validation, and call sites; the new default region correctly uses cmd.Flags().Changed() to distinguish explicit overrides from the default; and the auto-provisioning reuses the existing gcf.Provisioner.ProvisionWIF() with the correct org-scoped config (no Repo field, matching the org-level attribute condition path). The documentation restructuring is well done — manual WIF setup moves to an "Advanced" appendix, and the happy-path install example is simplified. Tests verify old flags are removed and new flags have correct defaults. No security, injection, or correctness issues found.

Findings

Medium

None

Low

• [style] internal/cli/admin_test.go — The comment // --gcp-wif-sa-email removed (direct WIF, no intermediate SA) on what will be line ~74 (after the new assertions) still references old gcp- naming convention. Consider updating the comment to reference the current flag namespace for consistency, though this is cosmetic.

Info

• [correctness] internal/cli/admin.go — The repository variable FULLSEND_GCP_REGION and secrets FULLSEND_GCP_PROJECT_ID / FULLSEND_GCP_WIF_PROVIDER retain their GCP-prefixed names. This is the right call — renaming them would break deployed workflows — but a follow-up issue to rename these variables (with a migration path) would complete the provider-agnostic rename.

• [correctness] docs/ADRs/0033-per-repo-installation-mode.md:261 — Line 261 (unchanged) still references FULLSEND_GCP_REGION and FULLSEND_GCP_AUTH_MODE in the step-5 description. Not introduced by this PR, but worth noting as adjacent stale documentation.

• [intent-alignment] The issue's proposed change docs: Add codebase context problem document and trim CLAUDE.md #3 mentions auto-discover (list existing WIF pools matching the org's attribute condition) before creating. The implementation skips discovery and always calls ProvisionWIF(), which is idempotent (pool/provider creation is no-op if they exist). This is a valid simplification — idempotent creation is functionally equivalent to discover-then-create — but diverges slightly from the issue spec.

Footer

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

Previous run

Review: #905

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

Summary

Clean, well-scoped rename of --gcp-project, --gcp-region, and --gcp-wif-provider to --inference-project, --inference-region, and --inference-wif-provider. The PR correctly defaults --inference-region to global, removes the now-unnecessary requirement for --gcp-wif-provider by adding auto-provisioning in per-org mode (reusing existing gcf.NewProvisioner infrastructure), and updates documentation, ADR, and plan docs consistently. Tests verify the renamed flags, confirm old flags are removed, and validate the new default value. The change matches the scope authorized by issue #884. No critical, high, or blocking findings.

Findings

Medium

• [style/conventions] internal/cli/admin.go — Repository variables and secrets (FULLSEND_GCP_REGION, FULLSEND_GCP_PROJECT_ID, FULLSEND_GCP_WIF_PROVIDER) retain GCP-prefixed names while CLI flags were renamed to provider-agnostic inference-* names. This creates a naming inconsistency between the user-facing CLI and the stored configuration. Likely intentional for backward compatibility with existing installations and downstream workflows, but worth tracking as follow-up work if the goal is full provider-agnostic naming.
  Remediation: Consider a follow-up issue to rename repository variables with a migration path for existing installations.

• [correctness] internal/cli/admin.go:~230-243 — The new WIF auto-provisioning path in per-org mode (inferenceProject != "" && inferenceWIFProvider == "") has no unit test coverage. The code instantiates gcf.NewLiveGCFClient() directly, making it difficult to unit test without dependency injection. The same pattern exists in per-repo mode, so this is consistent but compounds untested surface area.
  Remediation: Consider extracting a gcpClientFactory parameter or interface to enable testing the provisioning decision logic without live GCP calls.

Info

• [correctness] docs/ADRs/0033-per-repo-installation-mode.md — Step 7 was simplified from "Auto-provisions WIF pool/provider/service account" to "Auto-provisions WIF pool/provider". The removal of "service account" is consistent with the broader direction (direct WIF, no intermediate SA), but the ADR should ideally note this as a behavioral change rather than a silent edit.

Footer

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

Previous run (2)

Review: #905

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

Summary

The PR correctly renames the three --gcp-* CLI flags to --inference-* equivalents, defaults --inference-region to global, and adds WIF auto-discovery/creation in the per-org install path. The flag rename is mechanically sound, validation logic is correctly updated (using cmd.Flags().Changed() to handle the new default), and tests verify the new flag names and assert the old names are removed. Two findings are worth noting: the new auto-discovery/creation logic in the per-org path lacks unit test coverage, and the flag removal has no deprecation period.

Findings

Medium

• [Correctness / test adequacy] internal/cli/admin.go:~230-260 — The new WIF auto-discovery logic (GetProjectNumber → GetWIFProvider → conditional ProvisionWIF) added to the per-org install path has no unit tests. The linked issue's triage summary specifically recommended mock-based tests for the discovery-hit and discovery-miss paths, but neither is present. The per-repo path's existing WIF provisioning was already tested indirectly via integration tests, but the per-org discovery path introduces new branching (existing provider found vs. provision new) that should have targeted unit coverage.
  Remediation: Add unit tests with a fake GCF client (the fakeGCFClient in provisioner_test.go already implements the interface) that exercise: (1) existing WIF provider found → uses it, (2) no provider found → provisions one, (3) GetProjectNumber fails → returns error, (4) GetWIFProvider fails → returns error.

Low

• [Correctness / breaking change] internal/cli/admin.go:357-359 — The old --gcp-project, --gcp-region, and --gcp-wif-provider flags are removed with no deprecation aliases or warning. Existing user scripts and documentation referencing these flags will break immediately with "unknown flag" errors. The tests explicitly assert the old flags are gone, so this is intentional, but a deprecation period (e.g., hidden aliases that print a warning) would be more user-friendly.
  Remediation: Consider adding cmd.Flags().MarkDeprecated() aliases for one release cycle, or document the breaking change in release notes.

Info

• [Style/conventions] internal/cli/admin_test.go:~115-125 — TestInstallCmd_PerRepoRequiresInferenceProject and TestInstallCmd_PerRepoRequiresInferenceProjectExplicit test essentially the same condition (missing --inference-project), with the only difference being that one also omits --inference-region. Since --inference-region now has a default, both tests exercise the same code path. Not harmful, but could be consolidated.

Footer

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

[github-actions]

fullsend review is working on this — view logs

[github-actions]

fullsend review is working on this — view logs

[github-actions]

fullsend review is working on this — view logs