Simplify install enrollment UX to all-or-none choice by ggallen · Pull Request #698 · fullsend-ai/fullsend

ggallen · 2026-05-06T17:46:29Z

Summary

Implements #495 by removing the --repo flag from fullsend admin install and replacing it with an interactive prompt that gives users a simple choice: enroll all repositories or none.

Changes

Removed --repo flag: No longer need to specify repos upfront during installation
Interactive enrollment prompt: Users choose between:
- [a] Enroll all repositories (excluding .fullsend)
- [n] Enroll no repositories (configure later)
Automatic .fullsend exclusion: Config repo is never enrolled, even with "all" option
Helpful guidance: Users who choose "none" are shown how to enable repos later via fullsend admin repos enable
Updated tests: Verify --repo flag removal and proper command structure

Benefits

Simpler onboarding: No need to enumerate repositories during install
Better UX for large orgs: Organizations with many repos no longer face unwieldy command lines
Clear default paths: All-or-none choice is straightforward, with easy post-install adjustment via the enable/disable commands

Test plan

Unit tests verify --repo flag no longer exists
All existing CLI tests pass
Code compiles successfully
Enrollment prompt function handles user input correctly

Review: #698

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

Summary

This PR correctly implements issue #495 by removing the --repo flag from fullsend admin install and replacing it with an interactive all-or-none enrollment prompt, plus --enroll-all / --enroll-none flags for non-interactive use. The change is well-scoped, the code follows existing patterns, tests are thorough, and documentation is updated. Two minor observations are noted below but neither is blocking.

Findings

Medium / Low / Info

[correctness] internal/cli/admin.go:promptEnrollment — The promptEnrollment function does not check term.IsTerminal() before reading from stdin. If the install command is run non-interactively (e.g., in a CI script) without --enroll-all or --enroll-none, the process will hang waiting for input. The codebase has precedent for this check in runDisableRepos (line 1256). That said, the install command is inherently interactive (opens browser windows for app setup), so this is low-risk in practice — the --enroll-all/--enroll-none flags exist for automation. Severity: low.
Remediation: Consider adding a term.IsTerminal check with a helpful error message suggesting --enroll-all or --enroll-none, consistent with runDisableRepos.
[style] docs/guides/admin/installation.md — The new bullet references fullsend admin enable repos without the <org> argument, while the CLI prompt in the code correctly shows the full invocation with org. This is fine as a brief pointer rather than a full usage reference, but could be slightly confusing for users who copy-paste. Severity: info.

Footer

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

Previous run

Review: #698

Head SHA: 4686d41
Timestamp: 2026-05-08T13:09:21Z
Outcome: approve

Summary

This PR cleanly replaces the --repo flag on fullsend admin install with an interactive all-or-none enrollment prompt, matching the scope authorized by issue #495 (section 1 only, with sections 2 and 3 already split to separate issues). The implementation is correct, well-structured, and thoroughly tested. The promptEnrollment function accepts an io.Reader for testability, the --enroll-all/--enroll-none flags provide non-interactive escape hatches, the discovered repo list is threaded through to runDryRun/runInstall to avoid redundant API calls, and the docs are updated to reflect the new flow. No security, injection, or correctness concerns were found.

Findings

Critical

None

High

None

Medium

None

Low

[correctness] internal/cli/admin.go — No test covers the mutual-exclusivity check for --enroll-all and --enroll-none (the if enrollAllFlag && enrollNoneFlag guard). While the logic is trivial, a test would prevent future regressions if the flag handling changes.
Remediation: Add a test that sets both flags and asserts the expected error message.

Info

[style/conventions] docs/guides/admin/installation.md:133 — The enrollment option description says "enroll repositories later using fullsend admin enable repos" without showing the required <org> argument. The full syntax is documented in section 4, so this is not misleading, but including the org placeholder would be more helpful inline.
Remediation: Consider updating to fullsend admin enable repos <org> ....
[correctness] internal/cli/admin.go — The promptEnrollment function does not check term.IsTerminal(os.Stdin.Fd()) before prompting, unlike runDisableRepos. This is mitigated by the --enroll-all/--enroll-none flags for non-interactive use, but could produce a confusing hang if a user pipes input without realizing a prompt is expected.
Remediation: Consider adding a terminal check with a hint to use --enroll-all or --enroll-none.

Footer

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

Previous run (2)

Review: #698

Head SHA: ce80dfc
Timestamp: 2026-05-08T00:00:00Z
Outcome: request-changes

Summary

The PR correctly removes the --repo flag and adds an interactive all-or-none enrollment prompt, matching the intent of #495. The implementation is clean, well-tested for the new promptEnrollment function, and the documentation is updated consistently. However, the removal of --repo with no non-interactive substitute (e.g. --enroll-all / --enroll-none flags) breaks scriptability of the install command, which is a significant regression given the CLI already has patterns for non-interactive overrides (--yolo, --skip-app-setup).

Findings

High

[Correctness] internal/cli/admin.go — No non-interactive override for the enrollment prompt. The --repo flag is removed and replaced with an interactive-only promptEnrollment() call that reads from os.Stdin. There is no flag-based alternative (e.g. --enroll-all or --enroll-none) to allow scripted or CI-driven installations. This is inconsistent with existing patterns in the same CLI: --yolo skips the uninstall confirmation, and --skip-app-setup bypasses interactive app creation. The dry-run path also now requires interactive input since the prompt executes before the dryRun check.
Remediation: Add --enroll-all and/or --enroll-none flags that bypass the interactive prompt. When either flag is set, skip promptEnrollment() and use the flag value directly. This preserves the improved interactive UX while maintaining scriptability.

Medium

(none)

Low

[Style] internal/cli/admin.go — Two uses of fmt.Sprintf without format verbs where a plain string would suffice: printer.StepInfo(fmt.Sprintf("To enroll repositories later, use:")) and the two lines below it. These should be printer.StepInfo("...") directly.
Remediation: Remove the unnecessary fmt.Sprintf wrappers.

Info

[Correctness] internal/cli/admin.go — When enrollAll is false, allRepos remains nil (zero-value []forge.Repository). The runDryRun and runInstall functions check discoveredRepos != nil to decide whether to call ListOrgRepos. This works correctly but means the "enroll none" path always re-fetches the repo list inside those functions. Not a bug, but a redundant API call that could be avoided by always populating allRepos from the prompt step.

Footer

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

Previous run (3)

Review: #698

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

Summary

Clean, well-scoped change that replaces the --repo flag with an interactive all-or-none enrollment prompt during install, matching the authorization in #495. The implementation correctly avoids duplicate ListOrgRepos API calls by caching discovered repos when "all" is chosen and falling back to a fresh call when "none" is chosen. The promptEnrollment function follows the same testability pattern as the existing promptDispatchToken (accepting io.Reader), and the test suite is thorough with table-driven tests covering valid inputs, retries, EOF, and read errors. Two minor style nits noted below.

Findings

Medium / Low / Info

[style] internal/cli/admin.go:397 — Stale comment still references --repo flag: "Validate that every --repo value matches a discovered repo." The equivalent comment in runInstall (line 559) and the validateEnabledRepos docstring (line 506) were correctly updated to say "enabled repository," but this one in runDryRun was missed.
Remediation: Update to "Validate that every enabled repository matches a discovered repo."
[style] internal/cli/admin.go:212 — The guidance message for the "none" path reads fullsend admin enable repos %s <repo-name> or --all, where "or --all" looks like a literal argument rather than an alternative. Consider splitting into two lines or using parentheses: fullsend admin enable repos <org> <repo-name> and fullsend admin enable repos <org> --all.

Footer

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

Previous run (4)

Review: #698

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

Summary

Clean, well-scoped change that replaces the --repo flag with an interactive all-or-none enrollment prompt, aligning with issue #495. The implementation correctly threads discovered repos through to avoid redundant API calls, handles edge cases (EOF, invalid input, retries), and has thorough test coverage. No blocking issues found. The main concern is a dependency ordering risk: this PR references fullsend admin enable repos in user-facing output, but the PR that adds those commands (#697) is still open.

Findings

Medium

[correctness] internal/cli/admin.go — The CLI guidance message ("To enroll repositories later, use: fullsend admin enable repos ...") and docs/guides/admin/installation.md both reference fullsend admin enable repos, but PR Add CLI commands for repository enrollment management #697 (which adds the enable/disable commands) is still open and unmerged. If this PR merges first, users who choose "enroll none" will be directed to a command that does not exist.
Remediation: Ensure Add CLI commands for repository enrollment management #697 merges before this PR, or gate this PR on Add CLI commands for repository enrollment management #697 via a merge queue dependency.

Low

[style] internal/cli/admin.go:211 — The guidance message "To enroll repositories later, use: fullsend admin enable repos %s <repo-name> or --all" embeds or --all inline, which reads ambiguously — it could be mistaken for literal command text. Consider separating alternatives more clearly, e.g., two separate lines or using parentheses: fullsend admin enable repos <org> <repo-name> and fullsend admin enable repos <org> --all.
[style] docs/guides/admin/installation.md:133 — The doc text says fullsend admin enable repos without showing the required org argument. Including it (e.g., fullsend admin enable repos <org>) would make the guidance copy-paste-friendly.

Info

[correctness] internal/cli/admin.go — When enrollAll is true, the validateEnabledRepos call inside runInstall/runDryRun is always a no-op because enabledRepos is derived from the same discoveredRepos list being validated against. This is harmless (defense in depth) but worth noting for future maintainers.

Footer

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

Previous run (5)

Review: #698

Head SHA: 1aa02f6
Timestamp: 2026-05-08T00:00:00Z
Outcome: approve

Summary

Clean, well-scoped implementation of issue #495 section 1. The --repo flag is correctly removed from fullsend admin install and replaced with an interactive all-or-none enrollment prompt. The promptEnrollment function follows existing patterns (accepts io.Reader for testability), the .fullsend exclusion logic is correct, and the runInstall signature change avoids a redundant ListOrgRepos call on the enroll-all path. Tests are thorough — covering valid inputs, retries on invalid input, EOF, and read errors. No security, injection, or intent-alignment concerns.

Findings

Low

[inefficiency] internal/cli/admin.go — When enrollAll is true and --dry-run is set, ListOrgRepos is called twice: once during the enrollment prompt and once inside runDryRun. Could pass discovered repos through like runInstall does, but this is a cold path with no correctness impact.

Info

[docs-consistency] docs/guides/admin/installation.md — The doc guidance says fullsend admin enable repos while the code's help message includes the full syntax with org argument: fullsend admin enable repos <org> <repo-name> or --all. Minor phrasing difference; the doc is a shorthand reference.

Footer

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

Previous run (6)

Review: #698

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

Summary

This PR cleanly implements the install UX simplification described in #495 — removing the --repo flag and replacing it with an interactive all-or-none enrollment prompt. The promptEnrollment function is well-structured with proper testability (accepts io.Reader), and test coverage is thorough across valid/invalid/EOF/error cases. The documentation updates are consistent with the code changes. Two minor efficiency/UX observations are noted below; neither is blocking.

Findings

Medium

[correctness] internal/cli/admin.go — Duplicate ListOrgRepos API call when enrolling all repos. When the user chooses "enroll all," RunE calls client.ListOrgRepos to discover and filter repos, then passes them to runInstall, which calls client.ListOrgRepos again (line 500 in base). For organizations with many repositories, this doubles the API cost of installation. Consider passing the already-discovered repo list into runInstall or extracting the discovery step so it runs once.
Remediation: Refactor so runInstall accepts a pre-discovered repo list, or move the enrollment prompt inside runInstall after the existing discovery call.

Low

[correctness] internal/cli/admin.go — Dry-run path bypasses enrollment prompt. The dry-run branch (line ~186) passes nil for repos and returns early before the enrollment prompt is reached. This means fullsend admin install --dry-run cannot preview the "enroll all" scenario. This may be intentional (dry-run is for previewing infrastructure changes, not enrollment), but it's a behavioral change from the prior version where --repo values were passed through to dry-run.
Remediation: If intentional, consider adding a note in the dry-run output that enrollment choices are not previewed. If not intentional, move the enrollment prompt before the dry-run branch.

Footer

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

Previous run (7)

Review: #698

Head SHA: 144e178
Timestamp: 2026-05-07T00:00:00Z
Outcome: request-changes

Summary

The change correctly removes the --repo flag and introduces an interactive all-or-none enrollment prompt, which aligns well with issue #495. The implementation is clean, follows existing patterns, and has thorough test coverage for the new promptEnrollment function. However, there is one high-severity issue: the documentation tells users the wrong command for post-install enrollment (fullsend admin enable repos instead of fullsend admin repos enable per PR #697), which will cause a command-not-found error for users who follow the docs. There is also a medium-severity redundant API call that fetches org repos twice when enrolling all.

Findings

High

[Correctness] docs/guides/admin/installation.md:133 — Documentation references wrong command fullsend admin enable repos but PR Add CLI commands for repository enrollment management #697 implements fullsend admin repos enable <org>. Users choosing "enroll no repositories" during install will be directed to a non-existent command.
Remediation: Change fullsend admin enable repos to fullsend admin repos enable in the doc.

Medium

[Correctness] internal/cli/admin.go:~206 — When the user chooses "enroll all", client.ListOrgRepos is called in newInstallCmd to build the repos list, then runInstall calls ListOrgRepos again at line 500. This is a redundant API call that doubles the GitHub API cost for the "all" path. Not a bug, but wasteful for large orgs.
Remediation: Consider passing the already-discovered repos into runInstall or restructuring so the list is fetched once.
[Correctness] internal/cli/admin.go:185 — Dry-run mode now passes nil for repos, meaning --dry-run can no longer preview enrollment of specific repos. Before this change, --dry-run would show what would happen for repos passed via --repo. This behavioral change is not documented and may surprise users who use dry-run to validate before a real install.
Remediation: Consider adding the enrollment prompt to the dry-run path as well, or document that dry-run no longer previews enrollment.

Info

[Style] The promptEnrollment function cleanly accepts io.Reader for testability, consistent with the project's existing pattern. Test coverage is thorough, including EOF, error, retry, and case-insensitive input scenarios.

Footer

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

Previous run (8)

Review: #698

Head SHA: 0f513de
Timestamp: 2026-05-07T00:00:00Z
Outcome: approve

Summary

Clean, well-scoped PR that implements issue #495 by replacing the --repo flag with an interactive all-or-none enrollment prompt. The change is correctly implemented: promptEnrollment handles input parsing, retry on invalid input, and EOF/read errors properly; the .fullsend config repo is correctly excluded from enrollment; tests cover all input variants and edge cases. No security, injection, or correctness concerns.

Findings

Info

[Style] internal/cli/admin.go — When the user chooses "enroll all," ListOrgRepos is called to discover repos for enrollment, then runInstall calls ListOrgRepos again at its start. This is a redundant API call. Consider passing the already-discovered repos into runInstall or refactoring to avoid the double fetch. Not blocking — it's a minor efficiency issue.
[Style] internal/cli/admin.go — The dry-run path now always passes nil for repos (line 185 in the diff), meaning dry-run can never preview enrollment. This is a reasonable consequence of moving enrollment to an interactive prompt (dry-run returns before the prompt), but worth noting as a behavioral change from the previous --repo-based flow.
[Style] internal/cli/admin.go — Minor inconsistency: the "none" guidance message uses fullsend admin enable repos <org> <repo-name> or --all while installation.md uses fullsend admin enable repos without the full syntax. Aligning these would improve the user experience.

Footer

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

Previous run (9)

Review: #698

Head SHA: 21b42ff
Timestamp: 2026-05-06T00:00:00Z
Outcome: approve

Summary

Clean, well-scoped PR that correctly implements issue #495 section 1: removes the --repo flag from fullsend admin install and replaces it with an interactive all-or-none enrollment prompt. The implementation follows existing code patterns (compare with promptDispatchToken), properly excludes .fullsend from enrollment, and has thorough test coverage including edge cases (invalid input retry, EOF, read errors). Documentation is updated consistently. No security, injection, or correctness concerns.

Findings

Low

[Correctness] internal/cli/admin.go — When the user chooses "enroll all", ListOrgRepos is called in the enrollment block (to build the repo list) and then again inside runInstall (which re-discovers repos). This is a minor inefficiency — the second call also serves as validation, so it is not incorrect, but a future optimization could pass the already-discovered repos through.

Info

[Correctness] internal/cli/admin.go — The dry-run path now always passes nil for repos, meaning --dry-run can no longer preview enrollment with specific repos. This is an intentional consequence of removing --repo and is consistent with the new UX.
[Style] internal/cli/admin.go — promptEnrollment correctly accepts io.Reader for testability, matching the codebase pattern of injectable I/O. Good practice.

Footer

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

Previous run (10)

Review: #698

Head SHA: ac91cbe
Timestamp: 2026-05-06T21:00:00Z
Outcome: request-changes

Summary

This PR removes the --repo flag from fullsend admin install and adds an interactive all-or-none enrollment prompt, which aligns with issue #495. However, the PR silently breaks the --dry-run flag by removing the runDryRun() call path without providing a replacement — --dry-run now executes a real install including GitHub App creation and repo enrollment. This is a data-destructive regression that must be fixed before merge. Additionally, the promptEnrollment function reads directly from os.Stdin, making it untestable, and no unit tests cover the new interactive logic.

Findings

Critical

[Correctness] internal/cli/admin.go:~194-229 — --dry-run is broken: performs a real install instead of a dry run. The PR removes the if dryRun { return runDryRun(...) } guard (previously at line 185) but never calls runDryRun in the new code path. When --dry-run is passed: (1) the enrollment prompt is skipped (the if !dryRun guard works), but (2) runInstall() is called instead of runDryRun(), and (3) app setup (runAppSetup) also runs, creating real GitHub Apps. A user expecting a safe preview will instead mutate their GitHub organization. runDryRun becomes dead code.
Remediation: Restore the dry-run early return before app setup, or add a new dry-run path after enrollment that calls runDryRun with the resolved repos list. The dry-run guard must come before both runAppSetup and runInstall.

High

[Correctness] internal/cli/admin.go:906-933 — promptEnrollment is untestable due to hard-coded os.Stdin. The function creates bufio.NewReader(os.Stdin) directly, making it impossible to unit test without monkey-patching global state. Other interactive prompts in this file may share the pattern, but new code should follow Go best practices.
Remediation: Accept an io.Reader parameter (or use cmd.InOrStdin() from cobra) so tests can inject a strings.Reader. Add unit tests covering the "a", "n", and invalid-input paths.

Medium

[Correctness] internal/cli/admin_test.go:56-58 — No tests for the new enrollment behavior. The test changes only verify the --repo flag is absent. There are no tests for promptEnrollment, the enrollment flow integration, or the interaction between --dry-run and the new enrollment logic.
Remediation: Add table-driven tests for promptEnrollment (requires the io.Reader refactor above). Add an integration-level test verifying that --dry-run does not call runInstall.
[Correctness] internal/cli/admin.go:928-929 — Invalid input causes a hard error instead of re-prompting. A single typo (e.g., typing "y" instead of "a") terminates the entire install process, forcing the user to restart from scratch — potentially after a lengthy app setup phase.
Remediation: Wrap the prompt in a retry loop (e.g., up to 3 attempts) before returning an error.

Low

[Style] internal/cli/admin.go:916 — fmt.Print used directly instead of going through the printer abstraction. All other user-facing output in this function uses printer.StepInfo or printer.Header, but the "Enter choice" prompt bypasses it. This inconsistency may cause issues if output is redirected or if the printer is extended with formatting.
Remediation: Add a prompt method to the printer, or at minimum use printer.StepInfo consistently.

Footer

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

fullsend-ai-review

See the review comment for full details.

Fixes five critical and high-priority issues identified in the review: 1. **Restore --dry-run functionality** (CRITICAL): Re-added the early return guard that prevents app setup from running in dry-run mode. Previously removed code caused --dry-run to execute real GitHub App creation instead of previewing changes. Now passes nil repos to runDryRun as a sensible default when --repo flag is unavailable. 2. **Make promptEnrollment testable**: Refactored to accept io.Reader parameter instead of hard-coding os.Stdin. This enables dependency injection for unit testing without monkey-patching. 3. **Add retry logic for invalid input**: Changed error handling to re-prompt on invalid choices instead of terminating the install. Users can now recover from typos (e.g., "y" instead of "a") without restarting the entire installation process. 4. **Standardize output through printer**: Replaced fmt.Print with printer.StepInfo() for consistency with the rest of the codebase. 5. **Add comprehensive test coverage**: Created 6 test cases covering: - Valid inputs (a/all, n/none) with case variations - Retry logic for single and multiple invalid attempts - Error handling for EOF and read failures - Custom errorReader test helper All new tests pass. The refactoring preserves existing behavior while fixing the data-destructive dry-run bug and improving testability. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Updates installation.md to reflect changes from PR fullsend-ai#698: - Remove --repo flag from install command examples - Document new interactive enrollment prompt (all/none choice) - Add new section 4 "Managing repository enrollment" - Document `fullsend admin repos enable` command - Document `fullsend admin repos disable` command - Explain workflow for post-install enrollment changes The guide now accurately reflects the simplified all-or-none install flow and the new dedicated commands for managing enrollment after installation completes. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Updates installation.md to reflect PR fullsend-ai#698 changes: - Document new interactive enrollment prompt (all/none choice) - Remove --repo flag from all install command examples - Update "Merge enrollment PRs" section to reflect conditional behavior The guide now accurately reflects the simplified all-or-none install flow introduced in this PR. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Fixes critical, medium, and low priority issues from the review: **Critical - Test/Code Mismatch:** - Remove test assertion for --repo flag removal (belongs in PR fullsend-ai#698, not this PR) - This PR only adds repos enable/disable commands, doesn't touch install command **Medium - Intent Alignment:** - Change --all behavior to ignore positional repo arguments instead of rejecting them - Update validation: when --all is set, positional args are silently ignored - Update tests to verify --all ignores positional args rather than erroring **Medium - Code Duplication:** - Extract shared cobra setup into newReposSubcommand() helper - Define reposRunFunc type for enable/disable operation signatures - Reduces duplicate code from ~160 lines to ~30 lines - Enable and disable commands now use the same setup logic **Low - Deterministic Output:** - Sort repository lists when using --all to ensure deterministic commit diffs - Add sort.Strings() calls in both enable and disable operations - Import "sort" package **Low - Test Brittleness:** - Change TestReposCommand_HasSubcommands to match command names instead of full Use strings - Use sub.Name() instead of sub.Use to avoid fragility from usage text changes All tests pass. The refactoring preserves behavior while addressing reviewer concerns. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Fixes five critical and high-priority issues identified in the review: 1. **Restore --dry-run functionality** (CRITICAL): Re-added the early return guard that prevents app setup from running in dry-run mode. Previously removed code caused --dry-run to execute real GitHub App creation instead of previewing changes. Now passes nil repos to runDryRun as a sensible default when --repo flag is unavailable. 2. **Make promptEnrollment testable**: Refactored to accept io.Reader parameter instead of hard-coding os.Stdin. This enables dependency injection for unit testing without monkey-patching. 3. **Add retry logic for invalid input**: Changed error handling to re-prompt on invalid choices instead of terminating the install. Users can now recover from typos (e.g., "y" instead of "a") without restarting the entire installation process. 4. **Standardize output through printer**: Replaced fmt.Print with printer.StepInfo() for consistency with the rest of the codebase. 5. **Add comprehensive test coverage**: Created 6 test cases covering: - Valid inputs (a/all, n/none) with case variations - Retry logic for single and multiple invalid attempts - Error handling for EOF and read failures - Custom errorReader test helper All new tests pass. The refactoring preserves existing behavior while fixing the data-destructive dry-run bug and improving testability. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Updates installation.md to reflect PR fullsend-ai#698 changes: - Document new interactive enrollment prompt (all/none choice) - Remove --repo flag from all install command examples - Update "Merge enrollment PRs" section to reflect conditional behavior The guide now accurately reflects the simplified all-or-none install flow introduced in this PR. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

github-actions · 2026-05-07T16:12:48Z

Site preview

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

Commit: 48b002f146b2ebbe839704e33679bd2d8c80e249

ralphbean

Clean, well-scoped PR that correctly implements issue #495 section 1. The core promptEnrollment logic is solid, testable, and well-tested. A couple of items to address before merge:

Command syntax inconsistency — the guidance message shown after choosing "none" uses enable repos but the prompt option text (and PR #697) uses repos enable. These should be consistent and match PR #697's actual command structure.
Stale --repo references in comments — since the --repo flag no longer exists, comments in runInstall (around the validateEnabledRepos call, ~line 511) and the validateEnabledRepos docstring still reference --repo. These should be updated to reflect that repos now come from the enrollment prompt.

Also noting (not blocking): the "enroll all" path calls ListOrgRepos in the RunE closure, then runInstall calls it again — a minor inefficiency for large orgs that could be cleaned up in a follow-up.

Fixes critical, medium, and low priority issues from the review: **Critical - Test/Code Mismatch:** - Remove test assertion for --repo flag removal (belongs in PR fullsend-ai#698, not this PR) - This PR only adds repos enable/disable commands, doesn't touch install command **Medium - Intent Alignment:** - Change --all behavior to ignore positional repo arguments instead of rejecting them - Update validation: when --all is set, positional args are silently ignored - Update tests to verify --all ignores positional args rather than erroring **Medium - Code Duplication:** - Extract shared cobra setup into newReposSubcommand() helper - Define reposRunFunc type for enable/disable operation signatures - Reduces duplicate code from ~160 lines to ~30 lines - Enable and disable commands now use the same setup logic **Low - Deterministic Output:** - Sort repository lists when using --all to ensure deterministic commit diffs - Add sort.Strings() calls in both enable and disable operations - Import "sort" package **Low - Test Brittleness:** - Change TestReposCommand_HasSubcommands to match command names instead of full Use strings - Use sub.Name() instead of sub.Use to avoid fragility from usage text changes All tests pass. The refactoring preserves behavior while addressing reviewer concerns. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Address review feedback from PR fullsend-ai#698: - Fix command syntax in enrollment guidance message (enable repos -> repos enable) - Update validateEnabledRepos docstring to remove --repo reference - Update runInstall comment to remove --repo reference These changes ensure consistency with PR fullsend-ai#697's command structure (fullsend admin repos enable/disable) and reflect that repos now come from the enrollment prompt rather than a --repo flag. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

ggallen

Re: Command syntax inconsistency (3203705056): Fixed in 144e178. Changed the command syntax from fullsend admin enable repos to fullsend admin repos enable to match PR #697's implementation.

github-actions · 2026-05-07T23:20:56Z

fullsend review is working on this — view logs

fullsend-ai-review

See the review comment for full details.

github-actions · 2026-05-07T23:56:52Z

fullsend review is working on this — view logs

- Move enrollment prompt before dry-run check so dry-run can preview enrollment - Pass discovered repos to runInstall to avoid duplicate ListOrgRepos call - Add discoveredRepos parameter to runInstall that accepts pre-discovered repos - When discoveredRepos is provided, skip API call and use the provided list Addresses review feedback in fullsend-ai#698 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

github-actions · 2026-05-08T00:24:11Z

fullsend review is working on this — view logs

ggallen · 2026-05-08T00:24:16Z

Both review issues have been addressed:

Duplicate API call: Moved enrollment prompt before dry-run check and refactored runInstall to accept pre-discovered repos, avoiding the redundant ListOrgRepos call when user chooses to enroll all repositories.
Dry-run bypass: The enrollment prompt now runs before the dry-run early return, so --dry-run can preview enrollment scenarios.

@ralphbean Ready for another review when you have a moment.

ggallen · 2026-05-08T00:26:02Z

/review

- Document new interactive enrollment prompt (all/none choice) - Remove --repo flag from all install command examples - Update "Merge enrollment PRs" section to reflect conditional behavior The guide now accurately reflects the simplified all-or-none install flow introduced. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Corrected the command syntax for enrolling repositories after install: - Changed "fullsend admin repos enable" to "fullsend admin enable repos" in both the CLI output message and installation guide - This aligns with the actual command structure Also clarified in the installation guide that repositories can be enrolled later using the enable repos command. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

- Fix command syntax in enrollment guidance message (enable repos -> repos enable) - Update validateEnabledRepos docstring to remove --repo reference - Update runInstall comment to remove --repo reference These changes ensure consistency with the enable/disable repos command structure and reflect that repos now come from the enrollment prompt rather than a --repo flag. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Changed all references from "fullsend admin repos enable" to "fullsend admin enable repos" to match the actual command structure. Fixed in two locations: - Line 223: CLI output message when no repos enrolled during install - Line 921: promptEnrollment function help text Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

- Move enrollment prompt before dry-run check so dry-run can preview enrollment - Pass discovered repos to runInstall to avoid duplicate ListOrgRepos call - Add discoveredRepos parameter to runInstall that accepts pre-discovered repos - When discoveredRepos is provided, skip API call and use the provided list Addresses review feedback in fullsend-ai#698 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

When enrollAll is true and --dry-run is set, ListOrgRepos was being called twice: once during enrollment discovery and again inside runDryRun. This fix passes the already-discovered repos to runDryRun to avoid the redundant API call. Modified runDryRun to accept an optional discoveredRepos parameter similar to runInstall. When repos are provided, they are used directly; otherwise, the function calls ListOrgRepos as before. Addresses low-priority efficiency issue in review fullsend-ai#698 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

github-actions · 2026-05-08T11:37:49Z

fullsend review is working on this — view logs

Address two remaining style issues from review: 1. Line 399: Update stale comment that referenced removed --repo flag - Changed "Validate that every --repo value matches a discovered repo." - To "Validate that every enabled repository matches a discovered repo." 2. Lines 212-214: Clarify enrollment guidance message format - Split ambiguous "fullsend admin enable repos %s <repo-name> or --all" - Into two clearly separated command examples showing both options: - "fullsend admin enable repos <org> <repo-name> [repo-name...]" - "fullsend admin enable repos <org> --all" The improved formatting makes it clear these are alternative command forms rather than literal command syntax. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

github-actions · 2026-05-08T11:48:21Z

fullsend review is working on this — view logs

fullsend-ai-review

See the review comment for full details.

ralphbean

Nice, clean PR — the interactive prompt is a solid UX improvement and the implementation is well-tested. One blocking concern around scriptability, plus two small nits.

Blocking: The install command now unconditionally requires interactive input with no flag-based bypass. Every other interactive prompt in this CLI has a non-interactive escape hatch (--yolo on uninstall/disable, --skip-app-setup on install). This needs parity — something like --enroll-all / --enroll-none, or reusing --yolo to default to a sensible choice.

Non-blocking nits noted inline.

Addresses review feedback from ralphbean and fullsend-ai-review bot: Blocking (High): - Add --enroll-all and --enroll-none flags to bypass the interactive enrollment prompt, maintaining scriptability for CI/cron environments. When either flag is set, skip promptEnrollment() and use the flag value directly. Flags are mutually exclusive with validation. Non-blocking (Low): - Remove unnecessary fmt.Sprintf calls with no format verbs (lines 228) - Update stale comment in validateEnabledRepos from "user targets a fork" to "enabled repo is a fork" to reflect new all-or-none enrollment model Follows existing CLI patterns: --yolo skips uninstall confirmation, --skip-app-setup bypasses app creation, now --enroll-all/--enroll-none bypass enrollment prompt. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

github-actions · 2026-05-08T13:05:14Z

fullsend review is working on this — view logs

Addresses Info-level issue from bot review: when enrollAll is false, allRepos remained nil, causing runDryRun and runInstall to call ListOrgRepos again. This resulted in a redundant API call. Fix: Always fetch org repos upfront, regardless of enrollment choice. Both enroll-all and enroll-none paths now use the same discovered repos, eliminating the duplicate API call. - Moved ListOrgRepos call before the enrollment branch - Both paths now pass populated allRepos to runDryRun/runInstall - No change in behavior, just eliminates redundant API call Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

github-actions · 2026-05-08T13:28:37Z

fullsend review is working on this — view logs

ralphbean

Thanks!

fullsend-ai-review Bot suggested changes May 6, 2026

View reviewed changes

ggallen force-pushed the fix/495-repo-enrollment branch from 6547a51 to 21b42ff Compare May 6, 2026 18:03

fullsend-ai-review Bot approved these changes May 6, 2026

View reviewed changes

ggallen force-pushed the fix/495-repo-enrollment branch from 21b42ff to 0f513de Compare May 7, 2026 16:11

github-actions Bot deployed to site-preview May 7, 2026 16:12 View deployment

fullsend-ai-review Bot approved these changes May 7, 2026

View reviewed changes

ralphbean requested changes May 7, 2026

View reviewed changes

Comment thread internal/cli/admin.go Outdated

Comment thread internal/cli/admin.go Outdated

ggallen commented May 7, 2026

View reviewed changes

github-actions Bot deployed to site-preview May 7, 2026 23:21 View deployment

fullsend-ai-review Bot suggested changes May 7, 2026

View reviewed changes

github-actions Bot deployed to site-preview May 7, 2026 23:58 View deployment

ggallen requested a review from ralphbean May 8, 2026 00:24

github-actions Bot deployed to site-preview May 8, 2026 00:25 View deployment

ggallen and others added 6 commits May 8, 2026 07:36

ggallen force-pushed the fix/495-repo-enrollment branch from ec1426c to c7165c5 Compare May 8, 2026 11:37

github-actions Bot deployed to site-preview May 8, 2026 11:39 View deployment

fullsend-ai-review Bot approved these changes May 8, 2026

View reviewed changes

github-actions Bot deployed to site-preview May 8, 2026 11:49 View deployment

fullsend-ai-review Bot suggested changes May 8, 2026

View reviewed changes

ralphbean requested changes May 8, 2026

View reviewed changes

Comment thread internal/cli/admin.go

Comment thread internal/cli/admin.go

Comment thread internal/cli/admin.go

github-actions Bot deployed to site-preview May 8, 2026 13:06 View deployment

fullsend-ai-review Bot approved these changes May 8, 2026

View reviewed changes

github-actions Bot deployed to site-preview May 8, 2026 13:29 View deployment

ralphbean approved these changes May 8, 2026

View reviewed changes

ralphbean added this pull request to the merge queue May 8, 2026

Merged via the queue into fullsend-ai:main with commit 28f225a May 8, 2026
24 checks passed

This was referenced Jul 6, 2026

feat(repos): add repos init command for manifest bootstrapping #3032

Merged

feat(repos): add repos install command for bulk per-repo installation #3033

Merged

fullsend-ai-triage Bot mentioned this pull request Sep 8, 2026

triage agent: add 'already-fixed' action to close issues resolved by existing commits #281

Open

fullsend-ai-retro Bot mentioned this pull request Sep 11, 2026

Standardize programming language choices for fullsend tooling #66

Closed

Uh oh!

Conversation

ggallen commented May 6, 2026

Summary

Changes

Benefits

Test plan

Related

Uh oh!

fullsend-ai-review Bot commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review: #698

Summary

Findings

Medium / Low / Info

Footer

Review: #698

Summary

Findings

Critical

High

Medium

Low

Info

Footer

Review: #698

Summary

Findings

High

Medium

Low

Info

Footer

Review: #698

Summary

Findings

Medium / Low / Info

Footer

Review: #698

Summary

Findings

Medium

Low

Info

Footer

Review: #698

Summary

Findings

Low

Info

Footer

Review: #698

Summary

Findings

Medium

Low

Footer

Review: #698

Summary

Findings

High

Medium

Info

Footer

Review: #698

Summary

Findings

Info

Footer

Review: #698

Summary

Findings

Low

Info

Footer

Review: #698

Summary

Findings

Critical

High

fullsend-ai-review Bot commented May 6, 2026 •

edited

Loading

github-actions Bot commented May 7, 2026 •

edited

Loading