Skip to content

feat(cli): add github sub-command for standalone org/repo setup - #1259

Merged
waynesun09 merged 4 commits into
mainfrom
github-subcommand
May 27, 2026
Merged

feat(cli): add github sub-command for standalone org/repo setup#1259
waynesun09 merged 4 commits into
mainfrom
github-subcommand

Conversation

@waynesun09

Copy link
Copy Markdown
Member

Summary

  • Adds fullsend github sub-command tree with 7 commands: setup, enroll, unenroll, set, status, uninstall, sync-scaffold
  • Extracts all GitHub-side operations from admin install so GitHub org admins can manage repos using pre-provisioned mint URL and WIF values without GCP credentials
  • setup handles both per-org and per-repo installation using the existing layer stack with skipMintDispatcher
  • set unifies secret/variable updates behind an internal key mapping, enabling transparent migration from secrets to variables
  • enroll/unenroll delegate to existing enable/disable logic for lightweight config.yaml toggles

Test plan

  • go test ./internal/cli/... passes (39 new tests)
  • go vet ./internal/cli/... clean
  • Verify fullsend github --help shows all 7 sub-commands
  • Verify fullsend github setup --help shows all flags
  • Verify fullsend github set --help shows usage
  • Verify no GCP credentials are required for any command
  • E2E: fullsend github setup <org> --mint-url=<url> --skip-app-setup runs layer stack without GCP calls
  • E2E: fullsend github set <org> FULLSEND_MINT_URL <url> sets org variable
  • E2E: fullsend github sync-scaffold <org> updates workflows only

@github-actions

github-actions Bot commented May 20, 2026

Copy link
Copy Markdown

Site preview

Preview: https://0682b760-site.fullsend-ai.workers.dev

Commit: 12b54d0f9481845543e9e794f611e4a830bbff5e

@fullsend-ai-review

fullsend-ai-review Bot commented May 20, 2026

Copy link
Copy Markdown

Review

Findings

Low

  • [docs-currency] docs/guides/dev/cli-internals.md:8 — The CLI command tree diagram does not include the new github sub-command tree (setup, enroll, unenroll, set, status, uninstall, sync-scaffold). After this PR merges, the diagram will be incomplete.
    Remediation: Add the github sub-command tree to the command tree diagram.

  • [correctness] internal/cli/github.go:382 — When enrollAll is true but every eligible repo is skipped due to per-repo guard (not errors), enabledRepos remains nil and silently falls through to loadExistingEnabledRepos. The "Enrolling 0 repositories" message is printed but the user gets no indication that their --enroll-all request was effectively a no-op and existing enrollment was preserved. Consider adding a warning when enrollAll was requested but zero repos were newly enrolled.
    Remediation: Add a printer.StepWarn message when enrollAll is true and len(enabledRepos) == 0 before the nil check at line 382.

Info

  • [style] internal/cli/github.go:72 — Extra blank line between the githubSetupConfig struct and newGitHubSetupCmd. Other files in the package use single blank lines between declarations.
Previous run

Review

Findings

Low

  • [docs-currency] docs/guides/dev/cli-internals.md:8 — The CLI command tree diagram does not include the new github sub-command tree (setup, enroll, unenroll, set, status, uninstall, sync-scaffold). After this PR merges, the diagram will be incomplete.
    Remediation: Add the github sub-command tree to the command tree diagram.

  • [correctness] internal/cli/github.go:382 — When enrollAll is true but every eligible repo is skipped due to per-repo guard (not errors), enabledRepos remains nil and silently falls through to loadExistingEnabledRepos. The "Enrolling 0 repositories" message is printed but the user gets no indication that their --enroll-all request was effectively a no-op and existing enrollment was preserved. Consider adding a warning when enrollAll was requested but zero repos were newly enrolled.
    Remediation: Add a printer.StepWarn message when enrollAll is true and len(enabledRepos) == 0 before the nil check at line 382.

Info

  • [style] internal/cli/github.go:72 — Extra blank line between the githubSetupConfig struct and newGitHubSetupCmd. Other files in the package use single blank lines between declarations.
Previous run (2)

Review

Findings

Critical

  • [correctness] internal/cli/github.go:178printer.Banner() is called without the required version string argument. The ui.Printer.Banner method signature is Banner(version string) (see internal/ui/ui.go:35). This call appears in 5 places: lines 178, 314, 671, 797, and 947. The code will not compile.
    Remediation: Pass Version() to all Banner calls, e.g. printer.Banner(Version()), matching the pattern used in admin.go.

High

  • [correctness] internal/cli/github_test.go:1078TestGitHubSetupCmd_PerRepoRejectsPerOrgFlags asserts that --vendor-fullsend-binary produces an "only valid for per-org" error, but "vendor-fullsend-binary" is not in the perOrgOnlyFlags slice (admin.go:105). The flag is silently accepted in per-repo mode, so the test assertion will fail.
    Remediation: Either add "vendor-fullsend-binary" to perOrgOnlyFlags in admin.go, or create a local extended slice in newGitHubSetupCmd that appends it.

Low

  • [documentation-currency] docs/guides/dev/cli-internals.md — This file documents the complete CLI command tree (admin, run, scan, post-review, post-comment) but does not include the new github subcommand. The command tree listing is now stale.
Previous run (3)

Review

Findings

Medium

  • [documentation-currency] docs/guides/dev/cli-internals.md:5 — The CLI Command Tree diagram (lines 7–25) will be incomplete after this PR. It enumerates every top-level sub-command but will not include the new github sub-command tree with its 7 children (setup, enroll, unenroll, set, status, uninstall, sync-scaffold). Developers consulting this reference will miss the new command surface.
    Remediation: Add the github sub-tree to the CLI Command Tree diagram in cli-internals.md.

Low

  • [documentation-currency] docs/guides/admin/installation.md:45 — The installation guide references fullsend admin enable repos for post-install enrollment but does not mention fullsend github enroll as an alternative that works without GCP credentials. Users following this guide in a GitHub-only setup scenario will not discover the new command.
    Remediation: Add a cross-reference noting fullsend github enroll as an alternative for environments without GCP credentials.

  • [style] internal/cli/github.go:386 — The nil check if enabledRepos == nil relies on the distinction between a nil slice and an empty slice. When enrollAll is true but every repo is skipped (per-repo guard), enabledRepos remains nil and falls through to loadExistingEnabledRepos. This is correct but fragile — appending zero elements to a nil slice in a future refactor would change the behavior. A comment explaining the nil-vs-empty invariant would prevent accidental breakage.
    Remediation: Add an inline comment documenting that nil means "no enrollment decision was made" while empty means "explicitly enrolled zero repos".

Previous run (4)

Review

Findings

Medium

  • [documentation-currency] docs/guides/dev/cli-internals.md:7-25 — The CLI Command Tree diagram does not include the new fullsend github sub-command tree (setup, enroll, unenroll, set, status, uninstall, sync-scaffold). This diagram is the canonical reference for CLI structure and is now stale.
    Remediation: Add the github sub-command and its children to the command tree between admin and run.

  • [documentation-currency] docs/guides/admin/installation.md — The installation guide documents only fullsend admin install for org/repo setup. Now that fullsend github setup provides a GCP-credential-free alternative for the same GitHub-side operations, the guide should mention when to use each command (e.g., admin install for full infrastructure including GCP, github setup when GCP resources are already provisioned).
    Remediation: Add a section or callout explaining the github setup alternative and when it applies.

Low

  • [style] internal/cli/github.go:73-75githubSetupPerOrgOnlyFlags duplicates the identical perOrgOnlyFlags list defined in admin.go:105-107. Both contain ["vendor-fullsend-binary", "enroll-all", "enroll-none"].
    Remediation: Consider reusing perOrgOnlyFlags directly, or extracting a shared constant, to avoid the two lists drifting apart.
Previous run (5)

Review

Findings

Medium

  • [documentation-currency] docs/guides/dev/cli-internals.md:7-25 — The CLI command tree diagram does not include the new github sub-command tree (setup, enroll, unenroll, set, status, uninstall, sync-scaffold). This documentation is the primary reference for CLI structure and will be stale once this PR merges.
    Remediation: Add the github sub-command tree to the command tree diagram, showing all 7 sub-commands with brief descriptions.

  • [documentation-currency] docs/guides/admin/installation.md — The installation guide describes onboarding exclusively via fullsend admin install. The new fullsend github setup command provides an alternative GCP-free path that is not mentioned. Users may not discover the new command.
    Remediation: Add a section or cross-reference explaining when to use fullsend github setup vs fullsend admin install (i.e., when GCP credentials are pre-provisioned and only GitHub-side setup is needed).

Low

  • [correctness] internal/cli/github.go:551-565 — The set command accepts secret values (e.g., FULLSEND_GCP_PROJECT_ID, FULLSEND_GCP_WIF_PROVIDER) as positional CLI arguments. These values will be visible in shell history (~/.bash_history) and process listings (/proc/<pid>/cmdline). This is a common pattern in CLI tools (including gh secret set with --body), but worth noting for security-conscious deployments.
    Remediation: Consider supporting --from-stdin or reading from a file for secret values in a follow-up. Not blocking.

  • [correctness] internal/cli/github_test.go — There is no unit test for runGitHubSetupPerOrg with a mock client (unlike runGitHubSetupPerRepo which has TestRunGitHubSetupPerRepo). The org-level setup path is the more complex code path (enrollment logic, layer stack construction, app setup branching) and would benefit from mock-based coverage.
    Remediation: Add a test that exercises runGitHubSetupPerOrg with --skip-app-setup --enroll-none and a fake client to verify the layer stack is built and installed correctly.

  • [style] internal/cli/github.go:398-409 — The variable is named dummyAgents which is accurate but may be confusing in maintenance. These are real agent entries used when --skip-app-setup is set; the "dummy" refers to missing credentials, not to the agents themselves. Consider placeholderAgents or credlessAgents for clarity.

Previous run (6)

Review

Findings

Low

  • [style] internal/cli/github_test.go:1293 — Test name TestGitHubSetCmd_RejectsOrgScopeKeyWithRepoTarget is misleading — the test actually validates that setting an org-scope key with a repo target works (routes to org variable), not that it is rejected. The test body and comment confirm this. Consider renaming to TestGitHubSetCmd_OrgScopeKeyWithRepoTargetSetsOrgVariable.

  • [docs-currency] docs/guides/dev/cli-internals.md — The CLI command tree documented in this file does not include the new github subcommand or its 7 sub-commands. This will be stale once the PR merges. The installation guide (docs/guides/admin/installation.md) references fullsend admin install extensively but remains accurate since github setup is a parallel path, not a replacement.

Previous run (7)

Review

Findings

Medium

  • [correctness] internal/cli/github.go:549 — The set command (runGitHubSet) applies no value validation, allowing security controls enforced by setup to be bypassed. Specifically: FULLSEND_MINT_URL can be set to a non-HTTPS URL (bypassing validateMintURLHTTPS), and FULLSEND_GCP_WIF_PROVIDER can be set to a malformed value (bypassing validateWIFProvider). An admin using fullsend github set to update the mint URL could inadvertently configure plaintext HTTP token exchange.
    Remediation: Add the same validation calls used in setup — call validateMintURLHTTPS when key is FULLSEND_MINT_URL and validateWIFProvider when key is FULLSEND_GCP_WIF_PROVIDER.

  • [documentation-currency] docs/guides/dev/cli-internals.md:7 — The CLI command tree diagram lists every top-level command and their children but does not include the new github sub-command tree (setup, enroll, unenroll, set, status, uninstall, sync-scaffold). The diagram will be stale as soon as this PR merges.
    Remediation: Add the github sub-command and its 7 children to the ASCII tree diagram.

Low

  • [documentation-currency] docs/guides/admin/installation.md:45 — The installation guide tells users to "enroll repositories later using fullsend admin enable repos" but the new fullsend github enroll command is the intended GitHub-only equivalent. Users following the guide won't discover the new command path.
    Remediation: Add a note or section mentioning fullsend github enroll as an alternative for environments where GCP credentials are not available.

  • [style] internal/cli/github_test.go:1207 — Test name TestGitHubSetCmd_RejectsOrgScopeKeyWithRepoTarget is misleading — the test body and comments confirm the operation succeeds (org-scope key set via repo target routes to org variable). The name implies rejection.
    Remediation: Rename to TestGitHubSetCmd_OrgScopeKeyWithRepoTargetSetsOrgVariable or similar.

  • [correctness] internal/cli/github.go:376runGitHubSetupPerOrg builds orgCfg with nil agents at line 376, then rebuilds it with real agents at lines 417-424 when !cfg.skipAppSetup. The first orgCfg and first buildLayerStack call are discarded. Not a bug, but wasted work and potentially confusing for future readers.
    Remediation: Consider deferring the first orgCfg/buildLayerStack construction to after the app setup branch, or documenting why the double-build is intentional.

@fullsend-ai-review fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label May 20, 2026
@waynesun09
waynesun09 force-pushed the github-subcommand branch from 288c917 to b62db5a Compare May 20, 2026 19:11
@waynesun09
waynesun09 force-pushed the github-subcommand branch from b62db5a to 4c8af2e Compare May 20, 2026 19:25
@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels May 20, 2026
@waynesun09
waynesun09 force-pushed the github-subcommand branch from 4c8af2e to 9cc3f5a Compare May 20, 2026 19:33
@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels May 20, 2026
@waynesun09
waynesun09 force-pushed the github-subcommand branch from 9cc3f5a to 28bd34f Compare May 20, 2026 19:39
@waynesun09
waynesun09 marked this pull request as ready for review May 20, 2026 19:39
@waynesun09
waynesun09 requested a review from ggallen May 20, 2026 19:40
@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels May 20, 2026

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the review comment for full details.

Comment thread internal/cli/github.go
printer.Blank()

perRepoCfg := config.NewPerRepoConfig(roles)
if err := perRepoCfg.Validate(); err != nil {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[critical] correctness

printer.Banner() is called without the required version string argument. The ui.Printer.Banner method signature is Banner(version string). This call appears in 5 places: lines 178, 314, 671, 797, and 947. The code will not compile.

Suggested fix: Pass Version() to all Banner calls, e.g. printer.Banner(Version()), matching the pattern used in admin.go.

Comment thread internal/cli/github.go
if err := validateEnabledRepos(enabledRepos, repoNames); err != nil {
return err
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] correctness

When enrollAll is true but every eligible repo is skipped due to per-repo guard, enabledRepos remains nil and silently falls through to loadExistingEnabledRepos. The user gets no indication their --enroll-all request was a no-op.

Suggested fix: Add a printer.StepWarn message when enrollAll is true and len(enabledRepos) == 0 before the nil check at line 382.

Comment thread internal/cli/github.go

cmd := &cobra.Command{
Use: "setup <org|owner/repo>",
Short: "Configure fullsend for a GitHub org or repo",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[info] style

Extra blank line between githubSetupConfig struct and newGitHubSetupCmd function. Other files in the package use single blank lines between declarations.

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label May 27, 2026
Extract all GitHub-side operations from the monolithic admin install
command into a standalone sub-command that only requires GitHub access
(no GCP credentials needed).

New commands:
- github setup <org|owner/repo>: configure GitHub org or repo
- github enroll <org> [repo...]: enable repos in config.yaml
- github unenroll <org> [repo...]: disable repos in config.yaml
- github set <org|owner/repo> <key> <value>: update config values
- github status <org>: analyze GitHub-side installation
- github uninstall <org>: remove .fullsend repo + org variables
- github sync-scaffold <org>: update workflow templates

The setup command uses skipMintDispatcher for DispatchTokenLayer,
accepting --mint-url as a flag instead of provisioning GCP resources.
The set command maintains an internal mapping of key -> storage type
(secret vs variable) so users don't need to know the storage backend.

Enroll/unenroll delegate to the existing runEnableRepos/runDisableRepos
functions for lightweight config.yaml toggles without setting secrets.

Signed-off-by: Wayne Sun <gsun@redhat.com>
…dering

Remove githubSetupPerOrgOnlyFlags in github.go — it duplicated the
existing perOrgOnlyFlags from admin.go (same package, same values).

In newGitHubUninstallCmd, move resolveToken() and printer creation
before the confirmation prompt so users get a fast failure on missing
tokens before typing confirmation. This matches the pattern in
admin.go's newUninstallCmd and eliminates a duplicate printer
declaration.

Signed-off-by: Wayne Sun <gsun@redhat.com>
…n github sub-command

When ListOrgInstallations fails in runGitHubUninstall, the app cleanup
section was silently skipped with no warning. Add a fallback matching
admin.go's runUninstall pattern: warn the user and show all agentSlugs
as potential app URLs for manual cleanup.

Also update all printer.Banner() calls to pass Version() — the Banner
signature was updated upstream to accept a version string. Remove
vendor-fullsend-binary from the per-org-only test since the upstream
vendor binary feature now supports per-repo mode.

Signed-off-by: Wayne Sun <gsun@redhat.com>
Add tests for set command org-target path, ListOrgInstallations error
fallback, parseTarget multi-slash behavior, and secret location
assertions. Log when installed apps are not found during uninstall
app filtering.

Signed-off-by: Wayne Sun <gsun@redhat.com>
Comment thread internal/cli/github.go
if err := validateEnabledRepos(enabledRepos, repoNames); err != nil {
return err
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] correctness

When enrollAll is true but every eligible repo is skipped due to per-repo guard (not errors), enabledRepos remains nil and silently falls through to loadExistingEnabledRepos. The user gets no indication that their --enroll-all request was effectively a no-op.

Suggested fix: Add a printer.StepWarn message when enrollAll is true and len(enabledRepos) == 0 before the nil check at line 382.

Comment thread internal/cli/github.go

cmd := &cobra.Command{
Use: "setup <org|owner/repo>",
Short: "Configure fullsend for a GitHub org or repo",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[info] style

Extra blank line between the githubSetupConfig struct and newGitHubSetupCmd. Other files in the package use single blank lines between declarations.

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed ready-for-merge All reviewers approved — ready to merge labels May 27, 2026
@waynesun09
waynesun09 added this pull request to the merge queue May 27, 2026
Merged via the queue into main with commit 22b09b0 May 27, 2026
9 of 10 checks passed
@waynesun09
waynesun09 deleted the github-subcommand branch May 27, 2026 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-merge All reviewers approved — ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants