Detect stale GitHub App permissions during admin install

[rh-hemartin]

Summary

• Parse permissions from the /orgs/{org}/installations API response (already returned, was being ignored)
• Compare each app's installed permissions against AgentAppConfig() expectations during admin install
• Check all roles before exiting so the user sees every mismatch at once, with direct links to each app's permissions page

Fixes #319

Test plan

• make go-test — new and existing tests pass
• admin install on an org with stale app permissions — exits with error listing all apps and missing permissions
• admin install on an org with correct permissions — proceeds normally

🤖 Generated with Claude Code

[github-actions]

Site preview

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

Commit: d033833e31d067cdd048b029c19801af3f68f5a5

[ralphbean]

Three items to address — overall the approach is solid.

[ralphbean]

Review: #320

Head SHA: d033833
Timestamp: 2026-04-22T00:00:00Z
Outcome: request-changes

Summary

This PR adds detection of stale GitHub App permissions during admin install by comparing installed permissions against manifest expectations. The implementation has a critical flaw in the checkPermissions method that uses json.Marshal/json.Unmarshal to convert a struct to map[string]string, which fails to account for struct field naming vs. JSON tag naming. The implementation also silently swallows marshaling errors, which could mask bugs. Additionally, there is a potential issue with state accumulation across multiple Run() calls on the same Setup instance.

Findings

Critical

• [naming-mismatch] internal/appsetup/appsetup.go:280-282 — The checkPermissions method uses json.Marshal then json.Unmarshal to convert AppPermissions to map[string]string. This breaks because the JSON tags use snake_case (e.g., pull_requests) while the API response uses the same snake_case, BUT the struct field names are PascalCase. When the code does for perm, level := range want, it's iterating over keys like "pull_requests", but then checks inst.Permissions[perm] which also has "pull_requests". This actually works correctly because BOTH sides use the JSON tag names. However, the approach is fragile and non-obvious.

  The real problem is that this relies on JSON serialization round-tripping, which is indirect and error-prone. If someone changes the struct tags or adds a field without the correct tag, the check will silently break.

  Remediation: Replace the marshal/unmarshal dance with direct field access. Example:

  expected := ghTypes.AgentAppConfig(org, role).Permissions
  checks := []struct{ field, level string }{
      {"issues", expected.Issues},
      {"pull_requests", expected.PullRequests},
      {"checks", expected.Checks},
      {"contents", expected.Contents},
      {"workflows", expected.Workflows},
      {"administration", expected.Administration},
      {"members", expected.Members},
  }
  var missing []string
  for _, c := range checks {
      if c.level == "" {
          continue
      }
      if inst.Permissions[c.field] != c.level {
          missing = append(missing, fmt.Sprintf("%s:%s", c.field, c.level))
      }
  }

• [error-suppression] internal/appsetup/appsetup.go:280-282 — Both json.Marshal and json.Unmarshal errors are silently ignored with _ =. If marshaling fails, want will be empty and no permissions will be checked. If unmarshaling fails, want will also be empty. This makes the check silently ineffective if the struct cannot be serialized for any reason.

  Remediation: Check both errors and log or fail explicitly if they occur. Since this is unexpected (the struct should always be marshalable), it's reasonable to either panic or log an error and skip the check with a clear warning.

High

• [state-accumulation] internal/appsetup/appsetup.go:106 — The permErrors field on Setup accumulates state across multiple Run() calls. If the same Setup instance is reused (as it is in admin.go lines 378-383), subsequent Run() calls will append to the same permErrors slice. This is by design per the test TestSetup_StalePermissions_AllRolesChecked which relies on this behavior. However, the field is not documented, and there's no Reset() method. If a caller creates a Setup instance and calls Run() multiple times in different contexts (not just for multiple roles in one install), the errors will accumulate incorrectly.

  Remediation: Document the stateful behavior clearly in the Setup struct comment. Consider adding a Reset() method or making PermissionErrors() clear the errors after returning them to prevent accidental reuse. Alternatively, make PermissionErrors() consume the errors (move them out of the struct).

• [flow-ordering] internal/cli/admin.go:395-397 — The call to setup.PermissionErrors() happens AFTER all roles have been processed (lines 378-393). This is correct per the design (accumulate all mismatches, then exit once). However, there is a subtle issue: if setup.Run() returns an error for any role (e.g., network failure, client ID lookup failure), the loop short-circuits and PermissionErrors() is never called. This means that if role 1 has stale permissions but role 2 fails to set up due to an unrelated error, the user will only see the role 2 error, not the stale permissions warning for role 1.

  Remediation: Consider checking permissions before other error-prone operations in handleExistingApp, or accumulate permission errors even if later roles fail. One option is to continue the loop even on error (collect all role setup errors), then report both setup failures and permission mismatches at the end.

Medium

• [test-coverage] internal/appsetup/appsetup_test.go:308-352 — The tests do not cover the case where inst.Permissions is nil. The code handles this (line 275-277), but there's no test verifying that the warning is printed and the check is skipped. Add a test case for this scenario.

• [test-coverage] internal/appsetup/appsetup_test.go:354-383 — The tests do not cover the case where marshaling or unmarshaling fails (since this is currently silently ignored). Once error handling is added per the critical finding, add a test that verifies the error is logged/handled correctly.

Low

None.

Info

• [convention] internal/appsetup/appsetup.go:274 — The method checkPermissions is unexported but PermissionErrors() is exported. This is intentional (the check happens internally, the caller only consumes the result), but the naming asymmetry (checkPermissions vs PermissionErrors) is slightly awkward. Consider renaming PermissionErrors() to PermissionCheckErrors() or GetPermissionErrors() for clarity.

Footer

Outcome: request-changes

This review applies to SHA d033833e31d067cdd048b029c19801af3f68f5a5. Any push to the PR head clears this review and requires a new evaluation.

[ralphbean]

LGTM. Clean implementation — parses permissions from the installations API, compares against expected manifest values, and accumulates mismatches across all roles before exiting. Tests cover both stale and correct permission scenarios.