fix(#5912): GitLab WIF install/uninstall cleanup and edge cases

[fullsend-ai-coder]

Summary

Address four unresolved review findings from PR #5898 for the GitLab WIF install/uninstall flow.

Changes

• Secret-lifecycle on uninstall: Add cleanupGitLabBotTokenSecret to delete the bot token Secret Manager secret during GitLab uninstall. Before teardown, the FULLSEND_SA variable is read to derive the GCP project ID for best-effort cleanup.
• Secret-ID dot-vs-hyphen collision: Map dots to _dot_ in botTokenSecretID so my.group and my-group produce distinct secret IDs. Slashes continue to map to double underscores.
• Partial failure cleanup in WIF path: If ReplaceSecretIAMBinding or CreateProtectedCIVariable fails after storeSecretManagerToken succeeds, best-effort DeleteSecret cleans up the orphaned secret.
• Stale IAM binding on re-install: Add ReplaceSecretIAMBinding to the GCFClient interface with replace semantics (all existing members for the role are replaced with the specified member). The bot token path uses this method so re-install with a different service account revokes the old binding. The provisioner's agent PEM path retains the additive SetSecretIAMBinding.

Testing

• Unit tests for botTokenSecretID dot-vs-hyphen collision (owner and repo)
• Unit tests for projectIDFromSAEmail parsing
• Unit tests for cleanupGitLabBotTokenSecret success and failure paths
• Updated WIF mode tests verify ReplaceSecretIAMBinding is called
• Updated IAM binding failure test verifies DeleteSecret cleanup
• Updated CreateProtectedCIVariable failure test verifies DeleteSecret cleanup
• All existing SetSecretIAMBinding tests pass (additive behavior preserved)
• go vet passes
• Pre-commit could not run (sandbox network restriction); post-script runs authoritatively

---

Closes #5912

Post-script verification

• Branch is not main/master (agent/5912-gitlab-wif-cleanup)
• Secret scan passed (gitleaks — 9df3b1398df9f29db3eada0607b4c9f06535bcb0..HEAD)
• PR body secret scan passed (gitleaks — no-git)

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:04 PM UTC · Completed 9:22 PM UTC
Commit: ba48028 · View workflow run →

[codecov]

Codecov Report

❌ Patch coverage is 80.00000% with 18 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/cli/repos.go	70.37%	3 Missing and 5 partials ⚠️
internal/cli/repos_gitlab.go	82.92%	4 Missing and 3 partials ⚠️
internal/dispatch/gcf/fakeclient.go	71.42%	1 Missing and 1 partial ⚠️
internal/dispatch/gcf/gcp.go	93.33%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Review

Findings

Low

• [edge-case] internal/cli/repos_gitlab.go:57 — legacyBotTokenSecretID does not enforce the 255-character Secret Manager ID limit that botTokenSecretID does. For repos with very long subgroup paths, the generated legacy ID could exceed the GCP limit. Since this is only used for best-effort cleanup where the API error would be silently ignored, the practical impact is minimal.

• [code-duplication] internal/cli/repos_gitlab.go:143 — The cleanup block on IAM binding failure and the cleanup block on CreateProtectedCIVariable failure are identical copy-pasted code: delete the orphaned secret and revoke the PAT. Extracting a helper like cleanupFailedWIFInstall would eliminate the duplication and reduce the risk of the two blocks diverging.

• [privilege-escalation] internal/dispatch/gcf/gcp.go:729 — ReplaceSecretIAMBinding replaces ALL existing members for the matched role binding with a single member. If another service account legitimately has roles/secretmanager.secretAccessor on the same secret, its access is silently revoked. The risk is bounded: these are fullsend-managed bot token secrets (fullsend-bot-token-*) with a single expected accessor, and the destructive semantics are documented in the method's godoc.

• [authorization] internal/cli/repos_gitlab.go:337 — projectIDFromSAEmail trusts the FULLSEND_SA CI/CD variable (user-controllable by project maintainers) to derive the GCP project for secret deletion. A tampered variable could redirect the DeleteSecret call, but GCP IAM is the actual authorization boundary — the CLI's Application Default Credentials must have secretmanager.secrets.delete permission on the target project.

• [data-exposure] internal/cli/repos_gitlab.go:44 — The _dot_ mapping in botTokenSecretID introduces a theoretical collision: a GitLab group with literal _dot_ in its name would produce the same secret ID as a group with a dot. This is acknowledged in the code comment as extremely unlikely. During cleanup, both current and legacy-format secrets are deleted, so a collision could cause deletion of another repo's secret within the same GCP project.

• [architectural-coherence] internal/dispatch/gcf/gcp.go:729 — ReplaceSecretIAMBinding and SetSecretIAMBinding have similar names but critically different semantics (replace vs. additive). The naming follows established conventions (Replace strongly signals destructive behavior) and the interface godoc clearly documents the distinction, but callers must read the documentation to avoid misuse.

Previous run

Review

Findings

Low

• [edge-case] internal/cli/repos_gitlab.go:58 — legacyBotTokenSecretID does not enforce the 255-character Secret Manager ID limit that botTokenSecretID does. For repos with very long subgroup paths, the generated legacy ID could exceed the GCP limit. Since this is only used for best-effort cleanup where the API error would be silently ignored, the practical impact is minimal.

• [privilege-escalation] internal/dispatch/gcf/gcp.go:856 — ReplaceSecretIAMBinding replaces ALL existing members for the matched role binding with a single member. If another service account legitimately has roles/secretmanager.secretAccessor on the same secret, its access is silently revoked. The risk is bounded: these are fullsend-managed bot token secrets (fullsend-bot-token-*) with a single expected accessor, and the destructive semantics are documented in the method's godoc.

• [authorization] internal/cli/repos_gitlab.go:339 — projectIDFromSAEmail trusts the FULLSEND_SA CI/CD variable (user-controllable by project maintainers) to derive the GCP project for secret deletion. A tampered variable could redirect the DeleteSecret call, but GCP IAM is the actual authorization boundary — the CLI's Application Default Credentials must have secretmanager.secrets.delete permission on the target project.

• [data-exposure] internal/cli/repos_gitlab.go:45 — The _dot_ mapping in botTokenSecretID introduces a theoretical collision: a GitLab group with literal _dot_ in its name would produce the same secret ID as a group with a dot. This is acknowledged in the code comment as extremely unlikely. During cleanup, both current and legacy-format secrets are deleted, so a collision could cause deletion of another repo's secret within the same GCP project.

• [edge-case] internal/cli/repos_gitlab.go:320 — cleanupGitLabBotTokenSecret logs a warning when DeleteSecret fails for the current-style secret ID. In production, LiveGCFClient.DeleteSecret returns nil for StatusNotFound, so spurious warnings are not a concern.

• [code-organization] internal/cli/repos_gitlab.go:339 — projectIDFromSAEmail is a general-purpose GCP utility (parsing a service account email to extract the project ID) placed in the GitLab CLI file. The codebase has internal/gcp/ and internal/dispatch/gcf/ for GCP helpers. Consider relocating for reusability.

Previous run (2)

Review

Findings

Low

• [edge-case] internal/cli/repos_gitlab.go:43 — The backward-incompatible secret naming change (dot to _dot_) is now mitigated for uninstall by the legacyBotTokenSecretID dual-delete in cleanupGitLabBotTokenSecret. However, during re-install of a previously-installed repo whose name contains dots, the install path creates a new secret with the _dot_ naming scheme but does not clean up the old-style secret (dot mapped to hyphen). The old secret remains orphaned until the next uninstall. This is a minor resource leak, not a functional bug, since uninstall handles both naming schemes.

• [privilege-escalation] internal/dispatch/gcf/gcp.go:856 — ReplaceSecretIAMBinding replaces ALL existing members for the matched role binding with a single member. If another service account legitimately has roles/secretmanager.secretAccessor on the same secret, its access is silently revoked. The risk is bounded: these are fullsend-managed bot token secrets (fullsend-bot-token-*) with a single expected accessor, and the destructive semantics are documented in the method's godoc. The additive SetSecretIAMBinding is preserved for callers that need it.

• [authorization] internal/cli/repos_gitlab.go:329 — projectIDFromSAEmail trusts the FULLSEND_SA CI/CD variable (user-controllable by project maintainers) to derive the GCP project for secret deletion. A tampered variable could redirect the DeleteSecret call, but GCP IAM is the actual authorization boundary — the call uses the operator's Application Default Credentials, limiting exploitability.

• [data-exposure] internal/cli/repos_gitlab.go:45 — The _dot_ mapping in botTokenSecretID introduces a theoretical collision: a GitLab group with literal _dot_ in its name would produce the same secret ID as a group with a dot. During cleanup, both current and legacy-format secrets are deleted, so a collision could cause deletion of another repo's secret. The risk is extremely low and documented in code comments.

• [edge-case] internal/cli/repos_gitlab.go:312 — cleanupGitLabBotTokenSecret logs a warning when DeleteSecret fails for the current-style secret ID. In production, LiveGCFClient.DeleteSecret returns nil for StatusNotFound, so spurious warnings are not a concern. A different GCFClient implementation could surface not-found as an error, but this is speculative.

Previous run (3)

Review

Findings

Medium

• [edge-case] internal/cli/repos_gitlab.go:43 — The botTokenSecretID change from dot-to-hyphen (via sanitizer) to dot-to-_dot_ is backward-incompatible: secrets created under the old naming scheme (e.g., fullsend-bot-token-my-group--repo) will not be found by the new naming scheme (e.g., fullsend-bot-token-my_dot_group--repo). During uninstall, cleanupGitLabBotTokenSecret will attempt to delete a secret with the new-style name, miss the old-style secret, and silently succeed (GCP DeleteSecret returns success for non-existent secrets). During re-install, a new secret with the new name is created while the old one remains orphaned. This affects any repo whose owner or name contains a dot that was installed before this change. The orphaned secret contains a revoked PAT, so the security impact is minimal — this is a resource leak, not an access control gap.

Low

• [privilege-escalation] internal/dispatch/gcf/gcp.go:856 — ReplaceSecretIAMBinding replaces ALL existing members for the matched role binding with a single member. If another service account legitimately has roles/secretmanager.secretAccessor on the same secret, its access is silently revoked. The risk is bounded: these are fullsend-managed bot token secrets (fullsend-bot-token-*) with a single expected accessor, and the destructive semantics are documented in the method's godoc. The additive SetSecretIAMBinding is preserved for callers that need it.

• [authorization] internal/cli/repos_gitlab.go:309 — projectIDFromSAEmail trusts the FULLSEND_SA CI/CD variable (user-controllable by project maintainers) to derive the GCP project for secret deletion. A tampered variable could redirect the DeleteSecret call, but GCP IAM is the actual authorization boundary — the call uses the operator's Application Default Credentials, limiting exploitability.

Previous run (4)

Review

Findings

Medium

• [test-adequacy] internal/cli/repos_test.go:1683 — TestRunReposUninstall_GitLabWIFSecretPreRead does not verify that Secret Manager secret deletion was actually attempted. The code at repos.go:1096 creates a real NewLiveGCFClient(projectID) (not a fake), so the DeleteSecret call silently fails in the test environment and cleanupGitLabBotTokenSecret swallows the error. The test passes without exercising the cleanup wiring at the integration level. The cleanupGitLabBotTokenSecret function itself is properly unit-tested via TestCleanupGitLabBotTokenSecret with a fake client — this gap is at the integration level only.

Low

• [privilege-escalation] internal/dispatch/gcf/gcp.go:856 — ReplaceSecretIAMBinding replaces ALL existing members for the matched role binding with a single member. If another service account legitimately has roles/secretmanager.secretAccessor on the same secret, its access is silently revoked. The risk is bounded: these are fullsend-managed bot token secrets (fullsend-bot-token-*) with a single expected accessor, and the destructive semantics are documented in the method's godoc. The additive SetSecretIAMBinding is preserved for callers that need it.

• [authorization] internal/cli/repos_gitlab.go:309 — projectIDFromSAEmail trusts the FULLSEND_SA CI/CD variable (user-controllable) to derive the GCP project for secret deletion. A tampered variable could redirect the DeleteSecret call, but GCP IAM is the actual authorization boundary — the call uses the operator's Application Default Credentials, limiting exploitability.

• [edge-case] internal/cli/repos.go:1096 — cleanupGitLabBotTokenSecret is called with a NewLiveGCFClient instantiated per-repo during the post-uninstall loop. During bulk uninstall of many WIF-mode GitLab repos, this creates a new HTTP client and ADC token source per repo, which is wasteful but not a correctness bug.

Previous run (5)

Review

Findings

Low

• [privilege-escalation] internal/dispatch/gcf/gcp.go — ReplaceSecretIAMBinding replaces ALL existing members for the matched role binding with a single member. If another service account legitimately has roles/secretmanager.secretAccessor on the same secret, its access is silently revoked. The risk is bounded: these are fullsend-managed bot token secrets (fullsend-bot-token-*) with a single expected accessor, and the destructive semantics are documented in the method's godoc. The additive SetSecretIAMBinding is preserved for callers that need it.

• [authorization] internal/cli/repos.go — projectIDFromSAEmail trusts the FULLSEND_SA CI/CD variable (user-controllable) to derive the GCP project for secret deletion. A tampered variable could redirect the DeleteSecret call, but GCP IAM is the actual authorization boundary — the call uses the operator's Application Default Credentials, limiting exploitability.

• [architectural-coherence] internal/cli/repos.go — The pre-uninstall variable read creates a subtle dependency on FULLSEND_SA existing and being readable before teardown. If a previous partial uninstall removed this variable, Secret Manager cleanup is silently skipped (best-effort).

• [incomplete-lifecycle-documentation] docs/guides/infrastructure/infrastructure-reference.md — The Secrets Layer Behavior section does not document the new uninstall cleanup behavior for bot token secrets. The PR already updates docs/cli/repos.md and docs/guides/getting-started/operations.md with this information; a brief note in the reference doc would complete the coverage.

Previous run (6)

Review

Findings

Low

• [privilege escalation / IAM policy clobber] internal/dispatch/gcf/gcp.go — ReplaceSecretIAMBinding replaces ALL existing members for the matched role binding with a single member. If another service account legitimately has roles/secretmanager.secretAccessor on the same secret, its access is silently revoked. The risk is bounded: these are fullsend-managed bot token secrets (fullsend-bot-token-*) with a single expected accessor, and the destructive semantics are documented in the method's godoc. The additive SetSecretIAMBinding is preserved for callers that need it.

• [edge-case] internal/cli/repos.go — Secret Manager cleanup (cleanupGitLabBotTokenSecret) is placed after the *gl.LiveClient type assertion in the post-uninstall loop. If the type assertion fails, the continue skips Secret Manager cleanup even though it only needs a GCFClient (constructed independently via gcf.NewLiveGCFClient). In practice the type assertion succeeds on all production paths, but logically the cleanup is independent.

• [secrets cleanup / incomplete revocation] internal/cli/repos_gitlab.go — When setupGitLabBotToken fails at ReplaceSecretIAMBinding or CreateProtectedCIVariable, the best-effort cleanup deletes the orphaned Secret Manager secret but does not revoke the bot PAT that was created earlier in the function. The PAT remains active with api scope and Maintainer access for up to 1 year. Risk is low because the PAT is not written to any CI/CD variable on failure and the function returns an error.

Previous run (7)

Review

Findings

Medium

• [privilege escalation / IAM policy clobber] internal/dispatch/gcf/gcp.go — ReplaceSecretIAMBinding replaces ALL existing members for the matched role binding with a single member. If another service account legitimately has secretmanager.secretAccessor on the same secret, its access is silently revoked. This is a design trade-off: the replace semantics are correct for preventing stale bindings on re-install (issue fix: GitLab WIF install/uninstall cleanup and edge cases #5912 item 4), but a targeted swap (remove the specific old SA, add the new one) would preserve third-party bindings. Given these are fullsend-managed bot token secrets unlikely to have other bindings, the risk is bounded — worth documenting the destructive semantics.

Low

• [documentation comment convention] internal/dispatch/gcf/gcp.go — setSecretIAMBindingWithMode lacks a doc comment; every other unexported method in this file has one.
• [interface consistency] internal/dispatch/gcf/gcp.go — GCFClient now has Set+Replace for secrets but only Set for projects. A clarifying comment on SetProjectIAMBinding noting it is intentionally additive-only would prevent confusion.
• [stale-uninstall-steps] docs/guides/getting-started/operations.md:79 — Manual GitLab uninstall steps omit Secret Manager secret cleanup for WIF-mode repos. Users following these steps would leave an orphaned secret.
• [secret-ID collision] internal/cli/repos_gitlab.go:43 — The _dot_ mapping is not injective: a literal _dot_ in a GitLab name collides with a dot-mapped name. Extremely unlikely in practice.
• [authorization / implicit trust] internal/cli/repos.go — projectIDFromSAEmail trusts the FULLSEND_SA CI/CD variable (user-controllable) to derive the GCP project for secret deletion. GCP IAM is the actual authorization boundary, bounding exploitability.
• [error message format] internal/cli/repos_gitlab.go — cleanupGitLabBotTokenSecret uses "Could not delete" where existing code uses "Failed to delete" for deletion errors.
• [naming convention] internal/dispatch/gcf/gcp.go — Bare bool parameter on trySetIAMBinding and setSecretIAMBindingWithMode is opaque at call sites.
• [incomplete-behavior-description] docs/cli/repos.md:227 — States WIF cleanup is handled via inference deprovision, but repos uninstall now also performs best-effort bot token Secret Manager secret deletion.
• [missing-test] internal/dispatch/gcf/gcp_test.go — No integration-level test exercises the replace=true branch of trySetIAMBinding.

---

Labels: PR modifies GitLab WIF install/uninstall flow and GCP dispatch client

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 9:24 PM UTC · Ended 9:31 PM UTC
Commit: 7dda7d3 · View workflow run →

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 9:32 PM UTC · Ended 9:49 PM UTC
Commit: 7b721fb · View workflow run →

[github-actions]

Site preview

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

Commit: 67b45cbf18994a6062d08f007766d4a64c2988aa

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:32 PM UTC · Completed 9:49 PM UTC
Commit: 7b721fb · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:54 PM UTC · Completed 10:11 PM UTC
Commit: 5661e80 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:10 PM UTC · Completed 11:26 PM UTC
Commit: 319e96a · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 11:44 PM UTC · Ended 11:56 PM UTC
Commit: fd534a9 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:57 PM UTC · Completed 12:13 AM UTC
Commit: 2f16bf4 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:17 AM UTC · Completed 12:34 AM UTC
Commit: b318b47 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:41 AM UTC · Completed 1:02 AM UTC
Commit: 688bd64 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:49 AM UTC · Completed 3:05 AM UTC
Commit: 67b45cb · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 3:58 AM UTC · Completed 4:11 AM UTC
Commit: 67b45cb · View workflow run →

[fullsend-ai-retro]

Retro: PR #5914 — GitLab WIF install/uninstall cleanup

What happened

Issue #5912 was filed by ggallen with 4 specific tasks: orphaned secret cleanup on uninstall, secret-ID dot-vs-hyphen collision, partial failure cleanup, and stale IAM binding on re-install.

Code agent (run 30948690170) produced a solid initial implementation in ~25 minutes: +519/-30 lines across 10 files with unit tests covering all 4 tasks. PR #5914 was opened at 21:03 UTC.

Review agent ran 11 times (8 completed, 3 cancelled by superseding pushes) between 21:04 and 03:05 UTC. It caught 4 legitimate bugs that led to code improvements:

1. Backward-incompatible secret naming change (dot-to-_dot_ breaks cleanup of previously-installed repos) — escalated from low to medium severity across iterations, fixed with legacy dual-delete.
2. Secret Manager cleanup placed after type assertion (skipped on failure) — fixed by reordering.
3. PAT not revoked on partial failure in setupGitLabBotToken — fixed by adding revocation to cleanup paths.
4. Documentation gaps in manual uninstall steps — updated.

Human rework loop: ggallen manually fixed all review findings with 10 force pushes over ~6 hours. The fix agent was not involved — the review agent never suggested /fs-fix, and all commits were human-authored. Each force push triggered a new review run.

Merge: PR merged at 03:56 UTC with human approval from ggallen.

What went well

• Code agent quality: Addressed all 4 issue tasks in a single pass with comprehensive tests. The initial implementation was structurally sound.
• Review agent detection quality: Caught 4 real bugs the code agent missed, including a subtle backward-compatibility issue that was appropriately escalated from low to medium severity. The severity calibration was well-judged.
• Final outcome: All actionable review findings were addressed before merge.

What could go better

Review agent redundancy was the dominant issue. Of 29 inline review comments posted by the bot, approximately 18 (62%) were duplicates of findings already raised in prior iterations. The ReplaceSecretIAMBinding privilege-escalation finding was raised in all 8 completed iterations with nearly identical text. The projectIDFromSAEmail authorization finding was raised in 7 of 8. These were acknowledged as accepted-risk from the first iteration but re-raised every time. The actionable signal-to-noise ratio was approximately 4/29 (14%).

No fix agent involvement. The human iterated manually 10 times rather than using /fs-fix. The review agent did not suggest auto-fix for any of its findings. Some fixes (cleanup reordering, PAT revocation) were mechanical enough for the fix agent to handle.

Label churn. The PR oscillated between requires-manual-review and ready-for-merge four times as new medium-severity findings emerged on code changed in response to previous findings.

Existing issue coverage

All identified improvement areas are well-covered by existing open issues. This PR provides additional quantitative evidence:

• Finding dedup across iterations: Review agent should deduplicate findings across re-review iterations on the same PR #2959, Deduplicate inline review comments across force-push re-reviews #5007 — this PR shows 18/29 duplicate inline comments across 8 review iterations on a single PR.
• Suppressing acknowledged/dismissed findings: Review agent should suppress findings already dismissed by the PR author #4682, Review re-review anchoring should incorporate author response context to suppress explained-away findings #5265, Review agent should not re-raise findings that a human has explicitly resolved #3515 — the privilege-escalation finding was acknowledged as accepted-risk in iteration 1 but re-raised 7 more times.
• Debouncing rapid force-pushes: Debounce review agent runs on rapid force-pushes to reduce token waste #4960 — 10 force pushes triggered 11 review runs; debouncing could have reduced this to ~4-5.
• Suggesting /fs-fix for auto-fixable findings: Review agent should suggest /fs-fix for auto-fixable low-severity findings to prevent manual rework loops #5001 — the review agent never suggested /fs-fix despite mechanical fixes being available.
• Diminishing-value findings after approval: Review agent posts diminishing-value findings after approval #3025 — later iterations continued posting low-value findings alongside approval verdicts.
• Label management churn: Make review post-script label management idempotent to prevent label churn on re-reviews #4282 — ready-for-merge was added and removed twice.

Autonomy signal

The review agent demonstrated strong correctness detection for Go/GCP/IAM changes — it caught backward-compatibility, resource lifecycle ordering, and credential cleanup gaps that the code agent missed. If the redundancy issues tracked above were resolved (reducing noise from 62% to near-zero), the review agent's signal quality would strongly support increased autonomy for this class of change.