fix: detect stale PEM when GitHub App is deleted and recreated

[waynesun09]

Summary

• When a GitHub App is deleted and recreated with the same name (after GitHub's ~90-day cooldown), the CLI silently reuses the old PEM from Secret Manager because handleExistingApp matches by slug and sees the PEM exists. The stale PEM causes auth failures at runtime since the new app has a different app ID and private key.
• Adds isAppIDStale() to compare the live installation's app_id against the stored ROLE_APP_IDS value. When they differ, falls through to the existing recoverPEM flow instead of silently reusing stale credentials.
• Backwards compatible: when no stored app IDs are available (e.g., first install, --skip-mint-check), the check is a no-op and existing behavior is preserved.

Changes

• internal/appsetup/appsetup.go: Added storedAppIDs field, WithStoredAppIDs builder, isAppIDStale method, and staleness check in handleExistingApp
• internal/cli/admin.go: Pass stored ROLE_APP_IDS to runAppSetup from both per-org and per-repo install paths
• internal/appsetup/appsetup_test.go: Three new test cases covering stale ID detection, matching ID reuse, and no-stored-IDs backwards compatibility

Test plan

• go test ./internal/appsetup/... — all tests pass including 3 new tests
• go test ./internal/cli/... — all tests pass
• go vet ./internal/appsetup/... ./internal/cli/... — clean
• Manual: install with existing matching apps → reuses silently (no regression)
• Manual: simulate stale scenario by changing ROLE_APP_IDS to a different ID → triggers PEM recovery prompt

[github-actions]

fullsend review is working on this — view logs

[github-actions]

Site preview

Preview: https://840c640d-site.fullsend-ai.workers.dev

Commit: e41e8a91a280790431760639d6850984ff8972ec

[fullsend-ai-review]

Review: #1019

Head SHA: e41e8a9
Timestamp: 2026-05-15T00:00:00Z
Outcome: approve

Summary

Clean, well-scoped fix for a real security/reliability issue: when a GitHub App is deleted and recreated with the same slug, the CLI now detects the stale credentials instead of silently reusing an invalid PEM. The implementation is backwards-compatible (nil storedAppIDs map makes the check a no-op), correctly integrated at both the per-org and per-repo install paths, and thoroughly tested with five new test scenarios covering stale detection, matching ID reuse, backwards compatibility, user-decline flow, and unit-level isAppIDStale edge cases.

Findings

Info

• [style] internal/cli/admin.go — The per-org and per-repo install paths handle storedIDs/existingIDs with slightly different patterns (per-org introduces a new perOrgStoredIDs variable; per-repo conditionally updates the pre-existing existingIDs). Both are correct, but the asymmetry is worth noting for future maintainers. Not blocking since the per-org path has no earlier discovery to fall back on, making the patterns appropriately different.

• [correctness] internal/cli/admin.go:copySharedAppPEMs — Nice improvement separating the error case (now logs a warning) from the empty-result case, rather than silently swallowing both with if err != nil || len(...) == 0. The ListOrgInstallations error path now returns existingIDs so the staleness check still works downstream even when installation listing fails.

Footer

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

Previous run

Review: #1019

Head SHA: 3e8f4a5
Timestamp: 2026-05-15T00:00:00Z
Outcome: comment-only

Summary

This PR adds stale-PEM detection when a GitHub App is deleted and recreated with the same slug. The core logic is correct and backwards-compatible: isAppIDStale compares the live installation's app ID against stored ROLE_APP_IDS values, and when they differ, the existing reuse path is bypassed in favor of PEM recovery. When no stored IDs are available, the check is a no-op. The three new test cases cover the key scenarios well. A few medium/low findings below are worth addressing.

Findings

Medium

• [correctness] internal/cli/admin.go (per-org install path, around line 497 in diff) — The error from prov.GetExistingRoleAppIDs(ctx) is silently discarded (perOrgStoredIDs, _ = prov.GetExistingRoleAppIDs(ctx)). While this degrades gracefully (nil map → no staleness check → backward-compatible behavior), silently swallowing GCP API errors can mask connectivity or permission issues that the operator should know about. Consider logging a warning when the error is non-nil, similar to how other discovery failures are handled in the per-repo path (e.g., printer.StepWarn).

  Remediation: Replace perOrgStoredIDs, _ = prov.GetExistingRoleAppIDs(ctx) with:

  perOrgStoredIDs, idsErr := prov.GetExistingRoleAppIDs(ctx)
  if idsErr != nil {
      printer.StepWarn(fmt.Sprintf("Could not fetch stored app IDs: %v (stale PEM detection disabled)", idsErr))
  }

• [correctness] internal/cli/admin.go (per-repo install path, line 842 in base) — The per-repo path passes existingIDs to runAppSetup, but existingIDs is only populated when mintProject != "" and the mint is discoverable. When mintProject is empty or mint discovery fails with ErrFunctionNotFound, existingIDs remains nil and the stale-PEM check is silently disabled. This is technically backwards-compatible, but the asymmetry with the per-org path (which creates a dedicated provisioner to fetch IDs) means per-repo installs get weaker protection. Consider documenting this gap or aligning the behavior.

Low

• [style/conventions] internal/appsetup/appsetup.go — The isAppIDStale function is inserted in the middle of the multi-paragraph doc comment for handleExistingApp, splitting it. The first part of the original comment ("GitHub App PEM private keys are only available at creation time...") becomes a free-floating comment above isAppIDStale rather than part of handleExistingApp's godoc. Consider moving isAppIDStale above or below the handleExistingApp comment block to keep the doc comment intact.

• [style/conventions] internal/appsetup/appsetup.go — The // Empty PEM signals reuse of existing credentials. comment was removed from the reuse return path. This was a useful documentation hint explaining why PEM is intentionally left empty. Consider preserving it.

Info

• [correctness] Test coverage is good: TestSetup_ExistingApp_StaleAppID_TriggersRecovery verifies that a mismatched app ID triggers PEM recovery and re-storage; TestSetup_ExistingApp_MatchingAppID_Reuses verifies matching IDs preserve reuse; TestSetup_ExistingApp_NoStoredIDs_Reuses confirms backward compatibility. No negative test for isAppIDStale returning false on non-existent key, but that case is covered implicitly by the no-stored-IDs test.

Footer

Outcome: comment-only
This review applies to SHA 3e8f4a57d56bc0a086c734132e5d827051f0afbf. 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

[ralphbean]

LGTM!