Add CLI commands for repository enrollment management

[ggallen]

Summary

Implements #695 by adding dedicated CLI commands to manage repository enrollment state in the .fullsend config repository.

Changes

• New command structure: fullsend admin repos enable <org> [repo...] and fullsend admin repos disable <org> [repo...]
• --all flag: Enable/disable all discovered repositories at once
• Flag behavior: When --all is set, positional repository arguments are silently ignored
• Validation: Verifies repository existence before modifying config.yaml
• Workflow trigger: Automatically triggers repo-maintenance.yml after config updates

Test plan

• Unit tests verify command structure and flag handling
• Flag behavior tested: --all ignores positional arguments (both enable and disable)
• Error handling tested for missing org, conflicting flags, and invalid inputs
• Integration with forge.Client abstraction layer verified

Related

Closes #695

🤖 Generated with Claude Code

[fullsend-ai-review]

Review: #697

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

Summary

This PR adds fullsend admin enable repos and fullsend admin disable repos CLI commands for managing repository enrollment, implementing #695. The code is well-structured, follows existing CLI patterns (Banner/StepStart/StepDone), uses a shared newReposSubcommand factory to reduce duplication, and includes thorough unit tests covering all major paths. The enable/disable asymmetry (enable discovers from GitHub API, disable operates on existing config) is intentional and well-documented in code comments. Error handling is consistent, with helpful hints for token scope issues. The loadRepoConfig/saveRepoConfig helpers are cleanly extracted for reuse. No security, correctness, or injection concerns found.

Findings

Critical

None.

High

None.

Medium

None.

Low

• [intent-alignment] Command hierarchy — The issue specifies fullsend admin repos enable/disable <org> (resource-first), but the implementation uses fullsend admin enable/disable repos <org> (action-first). Both achieve the stated extensibility goal. The implementation's approach (admin enable auto-enrollment) reads naturally. The docs and tests are internally consistent with the implemented hierarchy. Not blocking, but worth confirming this is the intended divergence from the issue spec.

Info

• [correctness] internal/cli/admin.go — The read-modify-write pattern in loadRepoConfig/saveRepoConfig has no optimistic concurrency control. The code documents this trade-off explicitly (lines 407-414), noting it's acceptable for an admin CLI with rare concurrent usage. This is a reasonable decision for the current scope.

• [style] internal/cli/admin.go — The yolo parameter in runEnableRepos is accepted but unused (for signature compatibility with reposRunFunc). This is documented in the function comment. An alternative would be separate function signatures, but the current approach keeps the shared subcommand factory simple.

Footer

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

Previous run

Review: #697

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

Summary

This PR adds fullsend admin enable repos and fullsend admin disable repos CLI commands for managing repository enrollment state, closing #695. The implementation is well-structured with shared command scaffolding, proper validation, confirmation prompts for destructive operations, and comprehensive unit tests. The code correctly uses existing forge.Client abstractions and follows established patterns in the codebase. A few medium/low findings are noted below — none are blocking.

Findings

Medium

• [Correctness] internal/cli/admin.go:1148-1164 — When enabling specific repos (non---all path), the .fullsend check and repo-existence validation iterate sequentially with one API call per repo. For large lists this is O(n) serial API calls. Not a correctness bug, but worth noting for future optimization if users pass many repos at once.
  Remediation: Consider batching validation against ListOrgRepos results when the repo count exceeds a threshold.

• [Correctness] internal/cli/admin.go:1111 — The yolo parameter is accepted by runEnableRepos but never used. This is dead code carried over from the shared reposRunFunc signature. Harmless since enable doesn't register --yolo as a flag, but the unused parameter could confuse future maintainers.
  Remediation: Consider splitting the function signature or adding a comment explaining why yolo is accepted but unused in enable.

Low

• [Intent alignment] docs/guides/admin/installation.md — The issue's code examples show fullsend admin repos enable/disable but the implementation uses fullsend admin enable/disable repos. The issue's own design note ("use a repos subcommand to allow future extensibility") supports the implemented structure. The docs match the implementation. This is fine — just noting the discrepancy with the issue's example syntax.

• [Style/conventions] internal/cli/admin.go:1316-1387 — The loadRepoConfig and saveRepoConfig helper functions are well-documented with a race-condition note. The concurrency limitation (last-write-wins) is explicitly called out in comments, which is good practice for an admin CLI.

• [Correctness] internal/cli/admin_test.go:327-336 — setupTestConfig iterates a map, which has non-deterministic order in Go. The config returned may have repos in different orders across runs. This doesn't affect current test correctness since tests check individual repo states, but could cause flaky tests if order-dependent assertions are added later.

Info

• [Injection defense] PR body and commit messages inspected — no non-rendering Unicode, bidirectional overrides, or prompt injection patterns detected.
• [Platform security] The commands use the same token-resolution chain as existing admin commands. No new authentication or authorization surface introduced. The .fullsend self-enrollment guard is present in both enable and disable paths.
• [Content security] No user content handling or sandboxing concerns — these commands modify an admin config file via authenticated API calls.

Footer

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

Previous run (2)

Review: #697

Head SHA: ef04484
Timestamp: 2026-05-07T00:00:00Z
Outcome: comment-only

Summary

This PR cleanly implements the enable/disable repository enrollment commands as specified in #695. The code follows existing patterns in admin.go, uses the shared newReposSubcommand factory to reduce duplication, and includes comprehensive test coverage. The intentional asymmetry between enable (discovers from org) and disable (operates on config) is well-documented. A few minor findings are worth noting but none block merging.

Findings

Medium

• [Correctness] internal/cli/admin.go:109 — The yolo parameter is accepted by runEnableRepos via the shared reposRunFunc signature but is never used, since enable has no confirmation prompt. The --yolo flag is silently accepted on the enable repos command with no effect, which may confuse users. Consider either (a) documenting that --yolo is a no-op for enable, (b) removing it from the enable subcommand by not using the shared factory, or (c) adding the flag only to the disable subcommand.
  Remediation: Suppress the --yolo flag on the enable subcommand, or document it as a no-op.

Low

• [Style/conventions] docs/guides/admin/installation.md:47 — The docs describe a --yolo flag to skip the confirmation prompt for disable --all, but there is no corresponding documentation for what the confirmation prompt actually looks like or what input it expects (typing the org name). Adding a brief note would improve the user experience.
  Remediation: Add a sentence explaining the prompt expects the user to type the organization name.

• [Correctness] internal/cli/admin.go:313 — fmt.Scanln(&confirmation) reads from stdin. If stdin is not a terminal (e.g., piped input), this will read whatever is in the pipe or return EOF. This is standard CLI behavior and not a bug, but the error message "reading confirmation" is generic. Consider checking if stdin is a terminal and providing a clearer error if it's not (suggesting --yolo).
  Remediation: Optional improvement — detect non-TTY stdin and suggest --yolo.

Info

• [Intent alignment] The issue's code examples show fullsend admin repos enable/disable (repos as parent, enable/disable as subcommands) but the implementation uses fullsend admin enable/disable repos (enable/disable as parent, repos as subcommand). The issue's own "Note" section explains this structure is intentional for extensibility (fullsend admin enable auto-enrollment). The docs match the implementation. No action needed.

• [Correctness] The read-modify-write pattern in loadRepoConfig/saveRepoConfig has no optimistic concurrency control, as explicitly documented in the code comment. Acceptable for an admin CLI with rare concurrent usage.

• [Injection defense] No prompt injection patterns, non-rendering Unicode, or bidirectional overrides detected in the PR body, commit messages, or code strings.

• [Platform security] No privilege escalation, RBAC bypass, or data exposure concerns. Token resolution follows the same pattern as existing commands. Org name input is validated via validateOrgName.

Footer

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

Previous run (3)

Review: #697

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

Summary

This PR adds fullsend admin enable repos and fullsend admin disable repos commands as requested in #695, with well-structured shared scaffolding (newReposSubcommand), proper input validation, config read-modify-write with workflow dispatch, and comprehensive test coverage. The code follows existing patterns in admin.go, correctly excludes .fullsend from enrollment, and handles the intentional asymmetry between enable (discovers from GitHub) and disable (operates on config for cleanup of deleted repos). The race condition in the read-modify-write pattern is documented and acceptable for an admin CLI. No blocking findings.

Findings

Critical

None.

High

None.

Medium

None.

Low

• [style] PR description — The PR body describes the command structure as fullsend admin repos enable <org> but the code and installation.md docs correctly implement fullsend admin enable repos <org>. The PR description should match the actual command hierarchy to avoid confusion for reviewers and users reading the PR later.
  Remediation: Update the PR body's "New command structure" bullet to show fullsend admin enable repos <org> [repo...] and fullsend admin disable repos <org> [repo...].

Info

• [correctness] internal/cli/admin.go:331-332 — In the disable path (non---all), repos not found in config trigger a warning saying "skipping" but are still appended to reposToDisable at line 335. This is functionally harmless because the update loop at lines 348-355 checks exists before modifying, but the log message is slightly misleading. Consider either actually skipping the repo or rewording the message.

• [correctness] internal/cli/admin_test.go — All disable --all tests pass yolo: true, so the interactive confirmation flow (fmt.Scanln) has no test coverage. This is understandable since stdin mocking is non-trivial, but worth noting for future consideration — extracting the confirmation prompt into an injectable reader would enable testing.

• [intent-alignment] Issue Add dedicated enable/disable commands for repository enrollment #695 shows the command syntax as fullsend admin repos enable <org> (repos before enable) while the implementation note in the same issue describes the extensible admin enable repos / admin disable repos pattern. The PR follows the extensible pattern from the note, which is the better design for future commands like admin enable auto-enrollment. No action needed — just noting the issue description was ambiguous.

Footer

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

Previous run (4)

Review: #697

Head SHA: 6be0cd0
Timestamp: 2026-05-07T00:00:00Z
Outcome: comment-only

Summary

This PR adds well-structured enable and disable subcommands for managing repository enrollment, closely following existing patterns in admin.go. The code has good error handling, appropriate validation (org name, repo existence, .fullsend exclusion), intentional asymmetry between enable/disable discovery, and comprehensive test coverage. No critical or high findings — the items below are observations for consideration.

Findings

Medium

• [Correctness] internal/cli/admin.go — No preflight token scope verification for the new commands. The existing install, uninstall, and analyze commands all call runPreflight() to verify the token has required scopes before proceeding, giving users a clear remediation message. The new enable/disable commands skip this, so users with insufficient permissions will see raw API errors (e.g., from CreateOrUpdateFile or DispatchWorkflow) instead of a structured "missing scopes" diagnostic.
  Remediation: Consider adding a lightweight scope check, or at minimum wrapping API errors with a hint to check token permissions. This can also be deferred since the individual API errors are still informative.

Low

• [Style] internal/cli/admin.go:109 — The shared reposRunFunc signature includes a yolo bool parameter that runEnableRepos accepts but never uses. Only runDisableRepos uses it for the destructive-action confirmation prompt. This is a natural consequence of the shared command builder pattern and not a bug, but a future reader may wonder whether enable should also confirm when --all is used.

Info

• [Intent alignment] Command hierarchy differs from the issue specification. Issue Add dedicated enable/disable commands for repository enrollment #695 specifies fullsend admin repos enable <org> (verb under noun), but the implementation uses fullsend admin enable repos <org> (noun under verb). The implementation's choice actually aligns better with the issue's own extensibility note ("e.g., fullsend admin enable auto-enrollment") and the documentation is consistent with the code. No action needed — just noting the divergence for traceability.

• [Correctness] internal/cli/admin.go:329 — When disabling specific repos, names not found in config are warned about but still added to reposToDisable. The update loop at line 346 safely skips them (no config entry to modify), so this is harmless, but the warning could be confusing since the repo still appears to be "processed."

• [Correctness] Test coverage does not verify that DispatchWorkflow is called after config updates. The FakeClient doesn't record dispatched workflows, so this gap is infrastructure-driven rather than an oversight. The workflow trigger is a best-effort operation (failure is logged as a warning, not an error), which limits the risk.

Footer

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

Previous run (5)

Review: #697

Head SHA: 5e84e9f
Timestamp: 2026-05-07T00:00:00Z
Outcome: comment-only

Summary

This PR adds well-structured enable/disable subcommands for repository enrollment management with comprehensive test coverage and good error handling. The implementation correctly follows existing CLI patterns and the shared newReposSubcommand factory keeps the code DRY. The intentional asymmetry between enable-all (discovers from org) and disable-all (uses existing config) is well-documented. A few minor items are worth noting but none block merge.

Findings

Medium

• [Correctness] internal/cli/admin.go:175 — runEnableRepos accepts a yolo parameter via the shared reposRunFunc signature but never uses it. The enable repos --all command has no confirmation prompt, so --yolo is silently ignored. Consider either: (a) adding a confirmation prompt for enable --all (consistent with disable --all), or (b) documenting that --yolo is a no-op for enable. Enabling all repos triggers the repo-maintenance workflow across the org, which could be surprising without confirmation.
  Remediation: Add a confirmation prompt to runEnableRepos when all is true and !yolo, mirroring the pattern in runDisableRepos.

Low

• [Intent alignment] docs/guides/admin/installation.md — The PR description says the command structure is fullsend admin repos enable/disable but the actual implementation (and docs) use fullsend admin enable/disable repos. The code is internally consistent and the issue's extensibility note supports this pattern, but the PR description could confuse future readers.
  Remediation: Update the PR description to match the implemented command structure (fullsend admin enable repos / fullsend admin disable repos).

• [Correctness] internal/cli/admin.go:377-383 — The race condition in loadRepoConfig/saveRepoConfig is documented thoroughly (no optimistic concurrency control), which is good. For an admin CLI with rare concurrent usage this is acceptable, but worth keeping in mind if this pattern is reused for higher-concurrency paths.

Info

• [Style] The discardWriter type in admin_test.go (line 175) is already defined in the file before this PR's changes. The new tests correctly reuse it — good pattern adherence.

• [Correctness] The disable --all path iterates cfg.Repos (map) which has non-deterministic order, but the results are sorted before processing. The enable --all path similarly sorts after discovery. Both are correct.

Footer

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

Previous run (6)

Review: #697

Head SHA: df788c3
Timestamp: 2026-05-07T00:00:00Z
Outcome: comment-only

Summary

Solid implementation of CLI commands for repository enrollment management. The code follows existing patterns in admin.go well, uses a shared factory (newReposSubcommand) to reduce duplication, includes comprehensive test coverage for both enable and disable paths, and updates user-facing documentation. No critical or high severity issues found. A few medium and low findings around documentation consistency, unused parameters, and test coverage gaps are noted below.

Findings

Medium

• [intent-alignment] PR body — The PR description states the command structure is fullsend admin repos enable <org> [repo...] but the actual implementation creates fullsend admin enable repos <org> [repo...] (enable/disable are subcommands of admin, with repos underneath). The installation.md docs correctly show the implemented structure. The PR body should be corrected to avoid confusing reviewers.
  Remediation: Update the PR body to reflect the actual command hierarchy: fullsend admin enable repos and fullsend admin disable repos.

Low

• [correctness] internal/cli/admin.go:175 — The yolo parameter in runEnableRepos is accepted but never used. It's inherited from the shared reposRunFunc type signature, but unlike runDisableRepos (which uses it to skip the confirmation prompt on --all), runEnableRepos has no confirmation step. The --yolo flag is still exposed to users on the enable command where it silently does nothing.
  Remediation: Either add a confirmation prompt for enable --all (for consistency with disable) or document that --yolo is a no-op for enable. Alternatively, accept the asymmetry since enabling is less destructive than disabling.

• [correctness] internal/cli/admin_test.go:15 — TestAdminCommand_HasSubcommands was not updated to verify the new enable and disable subcommands were registered on the admin command. This test explicitly checks for install, uninstall, and analyze but omits the two new subcommands.
  Remediation: Add assertions for the enable and disable subcommands in TestAdminCommand_HasSubcommands.

• [correctness] internal/cli/admin.go:316 — When disabling specific repos (not --all), the code validates that the repo exists in the org via GetRepo. If a repo has been deleted from the org but still has an entry in config.yaml, the user cannot disable it individually — only disable --all works because it iterates config entries rather than calling GetRepo. This edge case could strand config entries for deleted repos.
  Remediation: Consider falling back to config-entry lookup when GetRepo returns not-found in the disable path, or document this limitation.

Info

• [style] internal/cli/admin.go:190 — The asymmetry between enable --all (discovers repos via ListOrgRepos) and disable --all (iterates cfg.Repos) is intentional and well-documented in the inline comment. Good design choice — enable discovers current state while disable cleans up historical state.

• [correctness] internal/cli/admin.go:377 — The read-modify-write race condition is documented in the loadRepoConfig comment. Acceptable for an admin CLI with rare concurrent usage.

Footer

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

Previous run (7)

Review: #697

Head SHA: a091b4f
Timestamp: 2026-05-06T18:30:00Z
Outcome: comment-only

Summary

This is a well-structured PR that adds dedicated repos enable and repos disable CLI subcommands, matching the specification in #695. The code is cleanly factored with a shared newReposSubcommand builder, properly separated loadRepoConfig/saveRepoConfig helpers, and comprehensive test coverage. The intentional asymmetry between enable --all (discovers org repos) and disable --all (uses config repos) is documented with clear comments. Two non-blocking findings are noted below — one test regression and one UX consideration.

Findings

Medium

• [Correctness / Test integrity] internal/cli/admin_test.go:37-38 — The --repo flag assertion was removed from TestInstallCmd_Flags, but the --repo flag still exists on the install command (line 206 of admin.go). This removes valid test coverage for an existing feature and appears unintentional — it is not mentioned in the PR description or linked issue.
  Remediation: Restore the removed lines:

  repoFlag := cmd.Flags().Lookup("repo")
  require.NotNil(t, repoFlag, "expected --repo flag")

Low

• [Correctness / UX] internal/cli/admin.go (runReposDisable) — disable --all has no confirmation prompt, unlike uninstall which requires typing the org name (or --yolo). Accidentally running disable --all against a production org would disable all enrolled repositories. While recoverable (re-run enable --all), a confirmation step would be consistent with the existing destructive-operation pattern.
  Remediation: Consider adding a confirmation prompt or --yes/--yolo flag for --all operations, consistent with the uninstall command pattern. This is a UX suggestion, not a blocker.

Footer

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

Previous run (8)

Review: #697

Head SHA: 7ae576c
Timestamp: 2026-05-06T00:00:00Z
Outcome: comment-only

Summary

This PR adds well-structured repos enable and repos disable CLI subcommands for managing repository enrollment, matching the intent of #695. The implementation follows existing codebase patterns (shared newReposSubcommand factory, forge.Client abstraction, ui.Printer output), includes thorough unit tests for both happy paths and error cases, and adds appropriate user documentation. Two medium-severity findings are noted below — neither is blocking, but both warrant attention.

Findings

Medium

• [Correctness] internal/cli/admin_test.go:37 — Test assertion for install command's --repo flag was removed, but the flag itself still exists on the install command (line 206 of admin.go). This reduces test coverage for an existing feature with no justification. If the flag is intentionally being deprecated in favor of the new repos enable command, that removal should happen in the same PR. Otherwise, restore the test assertion.
  Remediation: Restore the removed lines (repoFlag := cmd.Flags().Lookup("repo") and require.NotNil(t, repoFlag, ...)), or if the --repo flag on install is being deprecated, remove the flag itself and document the deprecation.

• [Intent alignment] PR description — The PR description claims "Mutual exclusivity: Enforces that --all and explicit repository names cannot be used together," but the implementation silently ignores positional args when --all is set (matching the issue spec's stated behavior: "If --all is specified, positional repository arguments are ignored"). The description should accurately reflect the implemented behavior to avoid confusion for reviewers and future maintainers.
  Remediation: Update the PR description to say "When --all is set, positional repository arguments are silently ignored" instead of claiming mutual exclusivity enforcement.

Low

• [Correctness] internal/cli/admin.go (loadRepoConfig/saveRepoConfig) — The read-modify-write pattern on config.yaml has no optimistic concurrency control at the application layer. If two admins run repos enable concurrently, one write could silently overwrite the other. The CreateOrUpdateFile method may handle SHA-based conflict detection internally (depends on the GitHub API implementation), but the CLI does not surface or retry on conflict. Acceptable for admin CLI usage but worth documenting.

Info

• [Style/conventions] internal/cli/admin.go — The disable --all path iterates cfg.Repos (repos already in config) while enable --all discovers from ListOrgRepos (all org repos). This asymmetry is intentionally correct — you wouldn't add config entries just to disable repos not yet tracked — but a brief code comment explaining the design choice would help future readers.

• [Correctness] Tests are comprehensive: 20+ test cases covering single/multi/all repos, no-op idempotency, error paths (missing .fullsend repo, missing config, repo not found, .fullsend self-reference), commit message format, and --all ignoring positional args. Good coverage.

Footer

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

Previous run (9)

Review: #697

Head SHA: 98e6c0e
Timestamp: 2026-05-06T00:00:00Z
Outcome: request-changes

Summary

The new repos enable / repos disable commands are well-structured and follow existing CLI patterns. Test coverage for the new business logic is thorough. However, a test modification unrelated to this feature breaks an existing test: TestInstallCmd_Flags now asserts the --repo flag was removed, but admin.go still registers it on the install command. This will cause CI failures.

Findings

High

• [Correctness] internal/cli/admin_test.go:57-58 — TestInstallCmd_Flags changed to assert repoFlag is nil ("--repo flag should have been removed"), but admin.go:206 still registers cmd.Flags().StringSliceVar(&repos, "repo", nil, ...). This test will fail. Either remove the --repo flag from the install command (if issue Improve enrollment UX with all-or-none install and dedicated enable/disable commands #495 requires it) or revert this test change.
  Remediation: If --repo removal is intended as part of this PR, also remove the flag registration at admin.go:206 and any code that reads repos in newInstallCmd. If not intended, revert the test assertion back to require.NotNil.

Medium

• [Style/conventions] internal/cli/admin.go:1030-1100 — newReposEnableCmd and newReposDisableCmd share ~20 lines of identical validation logic (org validation, mutual exclusivity check, token resolution, client construction). Consider extracting a shared helper (e.g., parseReposArgs(cmd, args, all) (org string, repos []string, client forge.Client, err error)) to reduce duplication.
  Remediation: Extract common argument parsing and validation into a helper function.

Low

• [Correctness] internal/cli/admin.go:1172 — When enable --all discovers repos not yet in cfg.Repos, new entries are created with RepoConfig{Enabled: true} and empty Roles. This works because Defaults.Roles applies, but there is no comment explaining this design choice. A brief comment would prevent future confusion.
  Remediation: Add a comment noting that per-repo Roles are intentionally empty because Defaults.Roles applies.

Info

• [Intent alignment] The linked issue Add dedicated enable/disable commands for repository enrollment #695 specifies "if --all is specified, positional repository arguments are ignored," but the implementation correctly rejects the combination with an explicit error. This is safer and more user-friendly than silent ignoring — no action needed, just noting the intentional deviation.

Footer

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

Previous run (10)

Review: #697

Head SHA: 18f8cf8
Timestamp: 2026-05-06T18:30:00Z
Outcome: request-changes

Summary

The PR adds fullsend admin repos enable/disable commands for managing repository enrollment, which aligns with issue #695. The core business logic is well-structured with proper validation, error handling, and good test coverage for the new commands. However, the PR includes an unrelated test change that will cause TestInstallCmd_Flags to fail: the test now asserts the --repo flag on the install command was removed, but the flag is still registered in the production code at admin.go:206. This is a blocking correctness issue. Additionally, the issue specification says --all should cause positional repo arguments to be silently ignored, but the implementation rejects combining them — this is arguably a better UX choice but deviates from the spec and should be called out.

Findings

Critical

• [Correctness] internal/cli/admin_test.go:57-58 — Test asserts --repo flag was removed from newInstallCmd() (assert.Nil(t, repoFlag)), but the flag is still registered at internal/cli/admin.go:206 (cmd.Flags().StringSliceVar(&repos, "repo", ...)). This test will fail. Either remove the --repo flag from the install command (if that's intended — reference to issue Improve enrollment UX with all-or-none install and dedicated enable/disable commands #495 suggests it is) or revert this test change. As written, this PR breaks TestInstallCmd_Flags.
  Remediation: If removing --repo from install is intended (per Improve enrollment UX with all-or-none install and dedicated enable/disable commands #495), also remove the flag registration at admin.go:206, the repos variable at line 84, and update runInstall to no longer accept a repos parameter. If this removal is out of scope, revert the test change.

Medium

• [Intent alignment] internal/cli/admin.go:1042,1093 — The issue spec states "If --all is specified, positional repository arguments are ignored." The implementation instead rejects combining --all with repo names (cannot specify both --all and repository names). This is arguably better UX (fail-fast vs. silent ignore), but deviates from the issue spec. Confirm the intended behavior with the issue author or update the issue description.

• [Style/conventions] internal/cli/admin.go:1021-1102 — The newReposEnableCmd and newReposDisableCmd functions contain ~80 lines of nearly identical cobra setup code (argument parsing, validation, token resolution, client creation). Consider extracting the shared boilerplate into a helper to reduce duplication, consistent with how other commands in admin.go share helpers like resolveToken and validateOrgName.

• [Correctness] internal/cli/admin.go:1158 — In runReposEnable with --all, newly discovered repos not in config are added with only Enabled: true and zero-value Roles. This means they get no agent roles assigned. Existing repos added via install have roles populated from defaults. Verify whether omitting roles is intentional or if new repos should inherit cfg.Defaults.Roles.

Low

• [Style/conventions] internal/cli/admin_test.go:222-228 — TestReposCommand_HasSubcommands checks subcommand existence by matching the full Use string ("enable <org> [repo...]") which is brittle — if the usage text changes, the test breaks. Consider checking cmd.Name() (which returns just the command name) or using assert.NotNil(t, cmd.Commands()) with name matching.

• [Correctness] internal/cli/admin.go:1209 — runReposDisable with --all iterates cfg.Repos map keys but doesn't sort them, so the order of operations is non-deterministic. While functionally correct (the end result is the same), it produces non-deterministic commit diffs. runReposEnable with --all has the same characteristic via ListOrgRepos ordering.

Info

• [Injection defense] PR body and commit messages contain no prompt injection patterns or non-rendering Unicode. No issues found.
• [Platform security] The new commands properly use resolveToken() for authentication and validate org names. saveRepoConfig commits directly to the .fullsend config repo's default branch — this is consistent with the existing install command's behavior and is appropriate for admin-level operations.
• [Content security] No user content handling concerns. Repository names flow through validateOrgName and GetRepo existence checks before use.

Footer

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

Previous run (11)

Review: #697

Head SHA: f866608
Timestamp: 2026-05-06T00:00:00Z
Outcome: request-changes

Summary

The PR adds fullsend admin repos enable/disable commands that read and update config.yaml in the .fullsend repository. The implementation correctly uses existing abstractions (forge.Client, config.OrgConfig, validateOrgName, resolveToken) and follows established CLI patterns. However, there are no unit tests for the core business logic (runReposEnable/runReposDisable) despite the existing test file demonstrating forge.FakeClient usage for exactly this purpose, and the two functions contain substantial duplication that should be factored out. Additionally, the disable path has an asymmetric validation gap — it does not check whether explicitly-named repos exist in the org, unlike enable.

Findings

High

• [Correctness] internal/cli/admin_test.go — No unit tests for runReposEnable or runReposDisable business logic. The tests only cover command wiring (subcommand presence, flag existence, argument validation). The actual workflows — reading config, updating repo entries, committing, triggering workflows, and all error branches within those functions — are completely untested. The existing test file already uses forge.NewFakeClient() for similar business-logic tests (e.g., TestEnsureConfigRepoExists), so the pattern is established. These are 150+ line functions with multiple branches and error paths that need coverage.
  Remediation: Add tests using forge.FakeClient that exercise: (1) enabling repos that are already enabled (no-op path), (2) enabling new repos, (3) enabling with --all, (4) disabling repos, (5) disabling repos not in config (warning path), (6) error cases for GetFileContent, CreateOrUpdateFile, and DispatchWorkflow failures.

Medium

• [Style/conventions] internal/cli/admin.go:1028-1367 — Substantial code duplication between runReposEnable and runReposDisable. Both functions share ~70% identical structure: verify .fullsend repo exists, read and parse config.yaml, marshal updated config, commit changes, trigger repo-maintenance.yml workflow, and print summary. The command definitions (newReposEnableCmd/newReposDisableCmd) are also nearly identical. This should be factored into a shared helper that accepts a mode/direction parameter.
  Remediation: Extract common logic into a helper like runReposUpdate(ctx, client, printer, org, repos, all, enable bool) or use a struct to encapsulate the shared workflow, with the enable/disable difference isolated to the config-update step.

• [Correctness] internal/cli/admin.go (runReposDisable, repo validation block) — When explicit repo names are passed to disable, the code warns if a repo is not in config.yaml but still adds it to reposToDisable. The subsequent update loop safely handles this (checks exists && rc.Enabled), so it won't crash, but the asymmetry with enable (which validates repos exist in the org) is surprising. More importantly, disable does not validate that the named repos actually exist in the GitHub org, which means typos pass silently — the user gets a warning about config but no error about the repo not existing.
  Remediation: Either add org-level existence validation to disable (matching enable's behavior) or document the intentional asymmetry with a code comment explaining why disable skips org validation (e.g., allowing cleanup of repos that have been deleted).

Low

• [Style/conventions] internal/cli/admin.go:1028-1100 — The newReposEnableCmd and newReposDisableCmd RunE closures are identical except for calling runReposEnable vs runReposDisable. Consider extracting the shared argument-parsing and validation logic into a helper function.

Footer

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

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

See the review comment for full details.

[rh-hemartin]

Weren't we going to use fullsend admin enable repos?

#495 (comment)

[ggallen]

Weren't we going to use fullsend admin enable repos?

Yeah, I think this is backward.

[ggallen]

@rh-hemartin Your review feedback has been addressed:

1. ✅ Command structure corrected: Changed from fullsend admin repos enable/disable to fullsend admin enable/disable repos as specified in issue Improve enrollment UX with all-or-none install and dedicated enable/disable commands #495. This provides better extensibility for future commands like fullsend admin enable auto-enrollment.

2. ✅ Method naming updated: Renamed runReposEnable → runEnableRepos and runReposDisable → runDisableRepos for consistency with the command structure.

3. ✅ Additional improvements (from bot review):

   • Restored --repo flag test assertion in TestInstallCmd_Flags
   • Added confirmation prompt for disable --all operations with --yolo flag to skip (consistent with uninstall command)

All tests pass. Ready for re-review.

[github-actions]

Site preview

Preview: https://1094c1ca-site.fullsend-ai.workers.dev

Commit: e129f33dd6f2b1fe864b62249f60d57b794ee760

[ralphbean]

Good implementation overall — the shared newReposSubcommand factory, extracted loadRepoConfig/saveRepoConfig helpers, and comprehensive test suite are well done. The intentional asymmetry between enable --all (discovers org repos) and disable --all (iterates config entries) is a smart design choice.

Two changes requested and two notes for consideration.

---

[moderate] TestAdminCommand_HasSubcommands not updated for new subcommands

TestAdminCommand_HasSubcommands (admin_test.go:17-25) asserts that admin has install, uninstall, and analyze subcommands, but does not check for enable and disable. If someone removes the cmd.AddCommand(newEnableCmd()) line, no test catches it. Please add:

assert.True(t, names["enable"], "expected enable subcommand")
assert.True(t, names["disable"], "expected disable subcommand")

[ggallen]

Thread 1 (disable validation): Fixed in 6be0cd0. The disable operation now validates against config instead of GitHub, allowing cleanup of repos that have been deleted from the org. Updated test to verify this behavior.

Thread 2 (unused yolo parameter): Acknowledged as minor/deferred. The yolo flag remains in the shared function signature for consistency, even though enable doesn't use it. This could be refactored in a future PR if needed.

Thread 3 (documentation): Fixed in 6be0cd0. Added documentation for the confirmation prompt and the flag to skip it for scripted usage.

[ggallen]

Thread 1 (disable validation): Fixed in 6be0cd0. The disable operation now validates against config instead of GitHub, allowing cleanup of repos that have been deleted from the org. Updated test TestRunDisableRepos_AllowsRepoNotInConfig to verify this behavior.

Thread 2 (unused yolo parameter): Acknowledged as minor/deferred. The yolo flag remains in the shared function signature for consistency, even though enable does not use it. This could be refactored in a future PR if needed.

Thread 3 (documentation): Fixed in 6be0cd0. Added documentation for the --all confirmation prompt and the --yolo flag to skip it for scripted usage.

[github-actions]

fullsend review is working on this — view logs

[ggallen]

Review feedback addressed in ef04484:

1. Disable repos skipping behavior: Fixed the misleading "skipping" message. The code now actually skips repos not in config by adding a continue statement and only appending repos that exist in config to reposToDisable.

2. Documentation clarity: Changed the wording from "validates repository names against the config" to "warns (but does not reject) repository names not found in the config" to accurately describe the behavior.

The changes ensure that when disabling repos, the command correctly handles repos that have been deleted from GitHub while still being present in config (cleanup scenario), and repos that never existed in config are properly skipped with a warning.

[ggallen]

/review

[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