fix: check token scopes before interactive app setup

[waynesun09]

Summary

• Add early token scope check (repo, workflow, admin:org) before GitHub App creation in fullsend admin install
• Previously, the install command created all 5 GitHub Apps (each requiring interactive browser flow) before checking scopes — if scopes were missing, the user wasted ~5 minutes on browser interactions before the install failed
• The existing runPreflight inside runInstall remains as defense-in-depth

Test plan

• go vet ./... passes
• go test -race ./internal/cli/ passes
• Manual: run fullsend admin install with a token missing workflow scope — should fail immediately with scope error before any browser windows open

[github-actions]

fullsend review is working on this — view logs

[github-actions]

Site preview

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

Commit: 88fb974df38ada11e295371c25bd04bd17ca7a08

[fullsend-ai-review]

Review: #760

Head SHA: 88fb974
Timestamp: 2026-05-08T00:00:00Z
Outcome: approve

Summary

This PR adds an early token scope check (repo, workflow, admin:org) before the interactive GitHub App creation flow in fullsend admin install. Previously, missing scopes were only detected after the user completed up to 5 browser-based app creation flows (~5 minutes of interactive work). The new checkInstallScopes function mirrors the existing runPreflight logic and runs before app setup, failing fast with a clear error. The existing runPreflight inside runInstall is retained as defense-in-depth. The implementation is correct, well-tested, and appropriately scoped.

Findings

Info

• [correctness] internal/cli/admin.go:825 — The hardcoded installRequiredScopes variable duplicates information derivable at runtime from Stack.CollectRequiredScopes(OpInstall). The TestCheckInstallScopes_SyncWithLayers test mitigates drift risk by asserting parity, which is a sound approach. No action needed — just noting the design trade-off between startup speed (no stack construction needed) and DRY.

• [style] internal/cli/admin_test.go:729-745 — The new tests use direct struct literal initialization (&forge.FakeClient{...}) while the rest of the file uses forge.NewFakeClient(). Both are safe (the err() method nil-checks the Errors map, and sync.Mutex zero-value is valid), but the inconsistency is worth noting. Not blocking.

Footer

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

Previous run

Review: #760

Head SHA: d6d198f
Timestamp: 2026-05-08T00:00:00Z
Outcome: comment-only

Summary

This PR adds a useful early scope check before the interactive GitHub App setup flow, preventing users from wasting time on browser-based app creation when their token is missing required scopes. The change is correctly placed after the dry-run check and before runAppSetup, and the existing runPreflight inside runInstall is preserved as defense-in-depth. Two findings are worth noting: the hardcoded scope list could drift from the layer system's canonical scope definitions, and the new function has no unit tests.

Findings

Medium

• [Correctness] internal/cli/admin.go:784 — The required scopes are hardcoded as ["repo", "workflow", "admin:org"] rather than derived from the layer system's CollectRequiredScopes(OpInstall). If a layer is added or modified to require a new scope (e.g., delete_repo is already required by ConfigRepoLayer for uninstall), this list will silently drift. The defense-in-depth runPreflight inside runInstall catches drift at runtime, but the early check would then give a misleading "permissions verified" message before failing later.
  Remediation: Consider extracting the scope list from the layer system. One approach: build a minimal stack (with placeholder credentials) solely for CollectRequiredScopes, or define the install-required scopes as a package-level constant in layers/ that both checkInstallScopes and the layer implementations reference.

Low

• [Correctness] internal/cli/admin.go:764 — No unit tests for checkInstallScopes. The function has three code paths (error from GetTokenScopes, fine-grained token nil return, and missing scopes) that should each be tested. The existing runPreflight has coverage via preflight_test.go but the new function does not.
  Remediation: Add table-driven tests covering: (1) GetTokenScopes returns error, (2) GetTokenScopes returns nil (fine-grained token skip), (3) all scopes present, (4) one or more scopes missing.

Footer

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