feat: parameterize app set prefix for GitHub App selection

[waynesun09]

Summary

• Introduce configurable app set prefix (default: fullsend) for GitHub App naming, changing from hardcoded fullsend-{role} to {appSet}-{role} (e.g., fullsend-ai-fullsend, fullsend-ai-coder)
• Add --app-set flag to fullsend admin install and fullsend admin uninstall for custom prefixes in multi-org SaaS environments
• Fixes incorrect app selection when a shared mint project has multiple GitHub Apps registered
• Add input validation for app set slugs (lowercase alphanumeric, hyphens, max 23 characters)
• Document custom app sets in admin installation guide

What does NOT change

• GCP secret naming (fullsend-{org}--{role}-app-pem) — independent of app set
• Mint ROLE_APP_IDS keys ({org}/{role}) — independent of app set
• Default behavior — DefaultAppSet = "fullsend" preserves backward compatibility with existing installations

Test plan

• go build ./... compiles cleanly
• go test ./internal/appsetup/ ./internal/forge/github/ — all pass
• go vet ./... — no issues
• make lint — passes
• Verify --app-set fullsend-ai creates apps named fullsend-ai-fullsend, fullsend-ai-coder, etc.
• Verify default behavior uses fullsend prefix without explicit flag
• Verify --app-set custom creates apps named custom-fullsend, custom-coder, etc.
• Verify uninstall fallback uses fullsend prefix for default app slug generation

[github-actions]

fullsend review is working on this — view logs

[github-actions]

Site preview

Preview: https://5feb84fa-site.fullsend-ai.workers.dev

Commit: 847cc9edf740b0ae1a7c4d02f0e9c79d779797b0

[fullsend-ai-review]

Review: #994

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

Summary

This PR introduces a configurable --app-set prefix for GitHub App naming, replacing the hardcoded fullsend- prefix with {appSet}-{role}. The implementation is clean and well-structured: validation is performed early in both install and uninstall command handlers, the Setup struct defaults to DefaultAppSet ("fullsend") for backward compatibility, and all call sites (Run, checkPermissions, runManifestFlow, runUninstall fallback) are updated consistently. The AgentAppConfig function in types.go is updated to accept the appSet parameter. Tests are thorough — including validation table tests, a non-default app set flow-through test, a default app set test, and updated existing tests. Documentation in the admin installation guide covers creation, multi-org usage, uninstall, and constraints. No critical or high findings.

Findings

Medium

• [correctness] internal/appsetup/appsetup.go:WithAppSet — WithAppSet does not validate its input, relying on callers to call ValidateAppSet first (as documented). Consider adding a defensive check or returning an error to prevent silent misuse by future callers.
  Remediation: Either add if err := ValidateAppSet(appSet); err != nil { panic(...) } or change the signature to return (*Setup, error).

• [correctness] e2e/admin/cleanup.go:legacySlug block — With the current DefaultAppSet = "fullsend", newSlug and legacySlug will always be equal ("fullsend-" + role), making the legacy cleanup block dead code. This is harmless future-proofing but may confuse readers.
  Remediation: Add a comment explaining this is intentional for forward compatibility if DefaultAppSet changes.

Footer

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

Previous run

Review: #994

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

Summary

This PR introduces a configurable --app-set prefix for GitHub App naming, cleanly parameterizing what was previously a hardcoded fullsend- prefix. The change is well-structured: validation is thorough (regex + length), all call sites are updated consistently, test coverage is good (including validation edge cases and a flow-through integration test), and the default (fullsend) preserves backward compatibility. Two low/info findings are noted below but neither blocks the change.

Findings

Medium / Low / Info

• [Correctness — Low] e2e/admin/cleanup.go:55-63 — Dead code: legacySlug always equals newSlug. Since DefaultAppSet = "fullsend" and AppSlug(DefaultAppSet, role) = "fullsend-" + role, the added legacySlug := "fullsend-" + role will always equal newSlug := appsetup.AppSlug(appsetup.DefaultAppSet, role). The guard legacySlug != newSlug is always false, so the legacy cleanup block never executes. This is harmless (e2e cleanup code) but adds confusion — consider removing it or making it meaningful (e.g., a truly different hardcoded legacy prefix).
  Remediation: Remove the legacySlug block or differentiate it from newSlug if a distinct legacy convention exists.

• [Style — Info] internal/appsetup/appsetup.go — WithAppSet does not call ValidateAppSet. Validation relies entirely on the CLI entry points. This is consistent with the existing builder pattern (e.g., WithPublicApps doesn't validate either), but a defensive ValidateAppSet call inside WithAppSet (or at least in Run) would prevent misuse from non-CLI callers.
  Remediation: Consider adding validation in WithAppSet or documenting that callers must validate first.

Footer

Outcome: comment-only
This review applies to SHA 5157626077f610b194bcaf02092bd00a4fde0ec1. Any push to the PR head clears this review and requires a new evaluation.

Previous run (2)

Review: automated review

Outcome: failure
Reason: stale-head

The review agent reviewed commit 03972c9e2e07c5405405d17161a6f2222ed69503 but the PR HEAD is now 4d6795b90eff98baf43fbb37ca1b80898b315953. This review was discarded to avoid approving unreviewed code.

Previous run (3)

Review: #994

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

Summary

This PR cleanly parameterizes the GitHub App naming convention from a hardcoded fullsend-{role} prefix to a configurable {appSet}-{role} pattern, with fullsend-ai as the new default. The change is well-scoped: it threads the appSet parameter through the CLI flags, Setup builder, AppSlug, and AgentAppConfig, with proper input validation via ValidateAppSet. The default change from fullsend- to fullsend-ai- is the most significant behavioral shift, but existing installations are handled via the knownSlugs mechanism (loaded from config.yaml), and the e2e cleanup code properly handles all three naming conventions (org-based, current fullsend-ai-*, and legacy fullsend-*). Test coverage is thorough — validation edge cases, flow-through with a non-default app set, and default behavior are all covered.

Findings

Medium

• [correctness] internal/appsetup/appsetup.go:WithAppSet — WithAppSet does not validate its input, unlike the CLI entry points which call ValidateAppSet first. If the Go API is used directly (e.g., by future internal callers), an invalid app set could produce broken GitHub App slugs. This is consistent with the existing builder pattern (WithPublicApps doesn't validate either), so it's not blocking, but adding validation in WithAppSet or at the start of Run would be defensive.
  Remediation: Consider calling ValidateAppSet inside WithAppSet (returning (*Setup, error)) or at the top of Run.

Info

• [correctness] internal/appsetup/appsetup_test.go:TestSetup_CorrectPermissions_NoError — This test adds "actions_variables": "read" to the expected permissions map. This is a correct fix for a pre-existing test gap (the Variables field was already set in AgentAppConfig but the test didn't assert it), though it's unrelated to the app set feature. Minor scope creep but harmless.

• [style] internal/forge/github/types.go:AgentAppConfig — The appSet parameter is added as the third positional argument. Since org, role, and appSet are all strings, callers must remember the order. An options struct would be more self-documenting at scale, but for three parameters this is acceptable.

Footer

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

Previous run (4)

Review: #994

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

Summary

Well-structured change that parameterizes the GitHub App naming prefix, enabling multi-org SaaS deployments where multiple app sets coexist in the same mint project. The implementation follows existing patterns (builder methods, CLI flag plumbing), includes thorough validation with ValidateAppSet, and has good test coverage including boundary cases. Two medium findings around API-level validation gaps and combined slug length limits are worth addressing but non-blocking.

Findings

Medium

• [Correctness] internal/appsetup/appsetup.go:196 — WithAppSet accepts any string without calling ValidateAppSet. CLI callers validate first, but programmatic callers of the Setup API could pass invalid values (empty string, uppercase, special characters) that would fail at GitHub API call time with an unhelpful error.
  Remediation: Either have WithAppSet return an error and call ValidateAppSet internally, or add a doc comment stating the precondition that ValidateAppSet must be called first.

• [Correctness] internal/appsetup/appsetup.go:178 — ValidateAppSet allows appSet up to 39 characters, but the final slug is appSet + "-" + role. The longest default role is "prioritize" (10 chars), yielding a max slug of 50 characters. GitHub App names/slugs have length limits (~34-39 chars depending on context), so a near-max appSet combined with a long role name would be rejected by GitHub at creation time with a non-obvious error.
  Remediation: Either lower the appSet max length to account for the role suffix (e.g., 27 chars to leave room for - + longest role), or validate the combined slug length.

Low

• [Correctness] internal/appsetup/appsetup.go:789 — The default prefix changes from fullsend- to fullsend-ai-. Existing installations with apps named fullsend-triage, fullsend-coder, etc. won't be found by the new default slug lookup. This is mitigated by the knownSlugs mechanism (loaded from config repo) and the --app-set flag, but users upgrading without a config repo could hit a confusing "no existing app found" state.
  Remediation: Consider documenting the migration path or adding a fallback lookup for the legacy fullsend-{role} pattern in findExistingInstallation.

Info

• [Style] Good use of regexp.MustCompile at package level and builder pattern consistent with WithPublicApps/WithKnownSlugs. Test coverage is thorough — ValidateAppSet has a comprehensive table-driven test, and TestSetup_NonDefaultAppSet_FlowsThrough verifies end-to-end plumbing.

Footer

Outcome: comment-only
This review applies to SHA fe134f2dc35a9006d3978d470f108948f2b3a68b. Any push to the PR head clears this review and requires a new evaluation.

Previous run (5)

Review: #994

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

Summary

This PR is a well-structured, mechanically sound change that parameterizes the GitHub App naming prefix from a hardcoded fullsend-{role} to a configurable {appSet}-{role} pattern. All call sites for AppSlug and AgentAppConfig are updated, test coverage is comprehensive (including validation, default behavior, and non-default flow-through), and the e2e cleanup code correctly handles the legacy naming convention. The one item worth noting is a behavioral change in the default prefix that affects existing installations.

Findings

Medium

• [Correctness] internal/appsetup/appsetup.go:789 — The default app set prefix changes from fullsend to fullsend-ai, which silently changes the default slug pattern (e.g., fullsend-fullsend → fullsend-ai-fullsend). Existing installations that used the previous default will not be found on re-run of install, and uninstall will generate incorrect fallback slugs unless --app-set fullsend is explicitly passed. The e2e cleanup code handles both conventions (line 55 of cleanup.go), but the production runUninstall fallback path does not attempt the legacy prefix.
  Remediation: Consider adding a legacy-slug fallback in runUninstall (similar to e2e cleanup) or document the migration path for users who installed with the old default. At minimum, the PR description should call out that this is a breaking change to the default naming convention.

Info

• [Style/conventions] internal/appsetup/appsetup.go:193 — WithAppSet does not validate its input; validation is only performed at the CLI boundary via ValidateAppSet. This is a standard Go builder-pattern trade-off and acceptable, but callers using the library directly (e.g., tests, future automation) could pass invalid values and get confusing downstream errors. Consider noting this in the doc comment or validating lazily in Run().

Footer

Outcome: comment-only
This review applies to SHA eb85575fd1d2959ece47e38d3e3cb3fadb53fa38. Any push to the PR head clears this review and requires a new evaluation.

Previous run (6)

Review: #994

Head SHA: 8fac023
Timestamp: 2026-05-15T00:00:00Z
Outcome: comment-only

Summary

Clean, well-scoped change that parameterizes the GitHub App naming prefix from a hardcoded fullsend- to a configurable {appSet}- pattern. All callers of the modified functions (AppSlug, AgentAppConfig, runUninstall, runAppSetup) are updated, the default preserves backward compatibility via DefaultAppSet = "fullsend-ai", and test coverage is thorough. Two minor gaps worth noting: WithAppSet doesn't validate its input (relying solely on CLI-boundary validation), and there's no length guard to prevent GitHub App name limit violations.

Findings

Medium

• [Correctness] internal/appsetup/appsetup.go:WithAppSet — The builder method WithAppSet accepts any string without calling ValidateAppSet. Validation is only performed at the CLI boundary (newInstallCmd, newUninstallCmd). A programmatic caller (e.g., future internal code, tests, or integrations) could set an invalid appSet (empty string, uppercase, special characters) that would propagate to GitHub API calls and fail with confusing errors.
  Remediation: Either validate inside WithAppSet (returning an error or using a separate constructor) or document that callers must validate first. The simplest fix is to add validation in WithAppSet and return (*Setup, error), though this breaks the builder chain pattern — alternatively, validate lazily at the start of Run.

Low

• [Correctness] internal/appsetup/appsetup.go:ValidateAppSet — No maximum length check on the appSet prefix. GitHub App names have a character limit (~34 chars). A long prefix like my-very-long-organization-prefix combined with a role name could exceed this limit, causing a GitHub API rejection with a non-obvious error message.
  Remediation: Add a length constraint to ValidateAppSet (e.g., max 20 characters for the prefix, leaving room for - plus the role name).

Footer

Outcome: comment-only
This review applies to SHA 8fac023a5b7004c13682494d9079aabf1f5fe0a9. Any push to the PR head clears this review and requires a new evaluation.

Previous run (7)

Review: #994

Head SHA: cd03dab
Timestamp: 2026-05-15T00:00:00Z
Outcome: request-changes

Summary

The PR changes AppSlug from a 1-argument to a 2-argument function but misses a caller in e2e/admin/admin_test.go, which will break compilation. Beyond the build breakage, the uninstall command has no --app-set flag, so apps installed with a custom prefix cannot be uninstalled via the default path, and the appSet parameter lacks input validation.

Findings

Critical

• [Correctness] e2e/admin/admin_test.go:236 — appsetup.AppSlug(role) still uses the old 1-argument signature. The PR changes AppSlug to require (appSet, role) but does not update this caller. This will cause a compilation failure across the entire module.
  Remediation: Update to appsetup.AppSlug(appsetup.DefaultAppSet, role) or pass the appropriate app set value.

Medium

• [Correctness] internal/cli/admin.go (uninstall command) — The uninstall fallback at line 1438 hardcodes appsetup.DefaultAppSet when config is unavailable, but the uninstall command has no --app-set flag. If an org was installed with --app-set custom, uninstall will look for fullsend-ai-{role} apps instead of custom-{role} apps and fail to clean up.
  Remediation: Add --app-set flag to the uninstall command, or document that custom app-set uninstall requires the config repo to be present.

• [Correctness] internal/appsetup/appsetup.go (WithAppSet) — No input validation on the appSet parameter. An empty string produces slugs like -coder (leading hyphen). Strings with spaces or special characters may violate GitHub App slug constraints.
  Remediation: Validate that appSet is non-empty and contains only lowercase alphanumeric characters and hyphens.

Low

• [Correctness] internal/appsetup/appsetup_test.go — All tests set .WithAppSet("fullsend") which reproduces the old naming convention. No test exercises Run() with a non-default app set (e.g., "fullsend-ai") to verify the custom prefix flows through AgentAppConfig and app creation.
  Remediation: Add at least one test that uses a non-default app set prefix and asserts the resulting slug and app config name.

Footer

Outcome: request-changes
This review applies to SHA cd03dabbb849bf245997495d8bf1d582ff174c46. Any push to the PR head clears this review and requires a new evaluation.

[fullsend-ai-review]

See the review comment for full details.

[github-actions]

fullsend fix is working on this — view logs

[fullsend-ai-coder]

🔧 Fix agent — iteration 1 (bot-triggered)

Addressed all 4 review findings: fixed e2e compilation error, added --app-set flag to uninstall command, added ValidateAppSet() input validation, and added tests for non-default app set prefix.

Fixed (4):

1. e2e/admin/admin_test.go uses old 1-arg AppSlug signature (e2e/admin/admin_test.go): Updated AppSlug(role) to AppSlug(appsetup.DefaultAppSet, role) at line 236 to match the new 2-argument signature, fixing the compilation error.
2. Uninstall command has no --app-set flag (internal/cli/admin.go): Added --app-set flag to the uninstall command with default value of appsetup.DefaultAppSet. The flag is passed through to runUninstall() which uses it in the fallback slug generation when config is unavailable. Also added ValidateAppSet() call to reject invalid values.
3. No input validation on appSet parameter (internal/appsetup/appsetup.go): Added ValidateAppSet() function that checks appSet is non-empty and matches ^[a-z0-9]+(-[a-z0-9]+)*$ (lowercase alphanumeric with hyphens, no leading/trailing hyphens). Validation is called at the CLI flag level in both install and uninstall commands.
4. No test exercises Run() with non-default app set (internal/appsetup/appsetup_test.go): Added TestSetup_NonDefaultAppSet_FlowsThrough which uses WithAppSet("custom-prefix") and asserts the resulting slug is "custom-prefix-fullsend". Also added TestValidateAppSet with positive and negative cases.

Tests: passed

Decision points

• Chose to add validation as a standalone ValidateAppSet() function called at CLI flag level rather than returning an error from WithAppSet() (alternatives: Make WithAppSet return error, Validate inside Run(); rationale: WithAppSet follows the builder pattern (returns Setup) used by all other With methods in the package. Moving validation to the CLI flag level catches invalid input early and matches the existing validateOrgName pattern.)

_{Updated by fullsend fix agent}

[github-actions]

fullsend review is working on this — view logs

[github-actions]

fullsend review is working on this — view logs

[github-actions]

fullsend review is working on this — view logs

[github-actions]

fullsend review is working on this — view logs

[github-actions]

fullsend review is working on this — view logs

[github-actions]

fullsend review is working on this — view logs

[github-actions]

fullsend review is working on this — view logs

[github-actions]

fullsend review is working on this — view logs

[github-actions]

fullsend review is working on this — view logs