From 7c9c8867d638150f9ee70f5850bc99a9b3dafdf5 Mon Sep 17 00:00:00 2001 From: fullsend-code Date: Wed, 13 May 2026 09:57:47 +0000 Subject: [PATCH] fix(#860): sync org variable visibility when enabling repos When `fullsend admin enable repos` enrolls new repositories, the FULLSEND_MINT_URL org variable's "selected" visibility is not updated to include the new repos. This causes the shim workflow in newly enrolled repos to fail because the variable is empty. Add SetOrgVariableRepos and GetOrgVariableRepos to the forge client interface (paralleling existing SetOrgSecretRepos). After saving the updated config in runEnableRepos, call a new syncOrgVariableVisibility helper that collects all enrolled repo IDs (plus the config repo) and updates the variable's repository access list. The sync is best-effort: failures are logged as warnings but do not block the enable command, since repo-maintenance can reconcile later. Closes #860 --- internal/cli/admin.go | 71 +++++++++++++++++++++++++++-- internal/cli/admin_test.go | 79 +++++++++++++++++++++++++++++++++ internal/forge/fake.go | 39 +++++++++++++++- internal/forge/fake_test.go | 9 ++++ internal/forge/forge.go | 4 ++ internal/forge/github/github.go | 41 +++++++++++++++++ 6 files changed, 238 insertions(+), 5 deletions(-) diff --git a/internal/cli/admin.go b/internal/cli/admin.go index 74211a081..74e357e4b 100644 --- a/internal/cli/admin.go +++ b/internal/cli/admin.go @@ -1277,7 +1277,10 @@ func runEnableRepos(ctx context.Context, client forge.Client, printer *ui.Printe } // Determine which repos to enable. + // We always need the full org repo list (for validation or discovery), + // so fetch it once and reuse for org variable visibility sync later. var reposToEnable []string + var allOrgRepos []forge.Repository if all { // Get all org repos by calling ListOrgRepos. // Note: disable --all iterates cfg.Repos instead of calling ListOrgRepos. @@ -1285,13 +1288,13 @@ func runEnableRepos(ctx context.Context, client forge.Client, printer *ui.Printe // while disable --all operates on previously configured repos (which may have // been deleted from the org but still need unenrollment PRs for cleanup). printer.StepStart("Discovering all organization repositories") - allRepos, err := client.ListOrgRepos(ctx, org) + allOrgRepos, err = client.ListOrgRepos(ctx, org) if err != nil { printer.StepFail("Failed to list organization repositories") printer.StepInfo("Hint: verify your token has 'repo' scope with: gh auth refresh -s repo") return fmt.Errorf("listing org repos: %w", err) } - for _, r := range allRepos { + for _, r := range allOrgRepos { if r.Name != forge.ConfigRepoName { reposToEnable = append(reposToEnable, r.Name) } @@ -1304,7 +1307,7 @@ func runEnableRepos(ctx context.Context, client forge.Client, printer *ui.Printe // one API call per repo (O(n) → O(1) API calls). printer.StepStart("Validating repository names") - allOrgRepos, err := client.ListOrgRepos(ctx, org) + allOrgRepos, err = client.ListOrgRepos(ctx, org) if err != nil { printer.StepFail("Failed to list organization repositories") printer.StepInfo("Hint: verify your token has 'repo' scope with: gh auth refresh -s repo") @@ -1366,6 +1369,14 @@ func runEnableRepos(ctx context.Context, client forge.Client, printer *ui.Printe return err } + // Sync org variable visibility so newly enrolled repos can read + // dispatch variables like FULLSEND_MINT_URL. Without this, the + // shim workflow in the new repo fails because the org variable + // has "selected" visibility that doesn't include the new repo. + if cfg.Dispatch.Mode == "oidc-mint" { + syncOrgVariableVisibility(ctx, client, printer, org, cfg, allOrgRepos) + } + printer.Blank() printer.Summary("Repositories enabled", []string{ fmt.Sprintf("Organization: %s", org), @@ -1376,6 +1387,60 @@ func runEnableRepos(ctx context.Context, client forge.Client, printer *ui.Printe return nil } +// dispatchOrgVariableNames returns the org-level variable names managed by the +// dispatch layer. This is kept in sync with the gcf.Provisioner.OrgVariableNames() +// method. We avoid importing the gcf package here to keep the CLI layer thin; +// adding a variable to the dispatcher requires updating this list. +var dispatchOrgVariableNames = []string{"FULLSEND_MINT_URL"} + +// syncOrgVariableVisibility updates the "selected" repository list for each +// dispatch org variable so that all currently enrolled repos (plus the config +// repo) can read them. This is best-effort: failures are logged as warnings +// but do not fail the enable command, because the repo-maintenance workflow +// can reconcile this later. +func syncOrgVariableVisibility(ctx context.Context, client forge.Client, printer *ui.Printer, org string, cfg *config.OrgConfig, allOrgRepos []forge.Repository) { + // Build a name→ID lookup from the discovered org repos. + repoIDByName := make(map[string]int64, len(allOrgRepos)) + for _, r := range allOrgRepos { + repoIDByName[r.Name] = r.ID + } + + // Collect IDs for all enabled repos. + enrolledRepoIDs := collectEnrolledRepoIDs(allOrgRepos, cfg.EnabledRepos()) + + // Ensure the config repo (.fullsend) is included — it needs access + // to dispatch variables for its own workflows. + configRepo, err := client.GetRepo(ctx, org, forge.ConfigRepoName) + if err == nil && configRepo != nil { + seen := make(map[int64]bool, len(enrolledRepoIDs)) + for _, id := range enrolledRepoIDs { + seen[id] = true + } + if !seen[configRepo.ID] { + enrolledRepoIDs = append(enrolledRepoIDs, configRepo.ID) + } + } + + for _, varName := range dispatchOrgVariableNames { + exists, checkErr := client.OrgVariableExists(ctx, org, varName) + if checkErr != nil { + printer.StepWarn(fmt.Sprintf("could not check org variable %s: %v", varName, checkErr)) + continue + } + if !exists { + // Variable not yet created (e.g. mint not provisioned yet). + continue + } + + printer.StepStart(fmt.Sprintf("Updating %s visibility for enrolled repos", varName)) + if setErr := client.SetOrgVariableRepos(ctx, org, varName, enrolledRepoIDs); setErr != nil { + printer.StepWarn(fmt.Sprintf("failed to update %s visibility: %v", varName, setErr)) + } else { + printer.StepDone(fmt.Sprintf("Updated %s visibility (%d repos)", varName, len(enrolledRepoIDs))) + } + } +} + // runDisableRepos disables the specified repositories from fullsend enrollment. func runDisableRepos(ctx context.Context, client forge.Client, printer *ui.Printer, org string, repos []string, all bool, yolo bool) error { printer.Banner() diff --git a/internal/cli/admin_test.go b/internal/cli/admin_test.go index c18841562..b379f8409 100644 --- a/internal/cli/admin_test.go +++ b/internal/cli/admin_test.go @@ -512,6 +512,85 @@ func TestRunEnableRepos_CommitMessageFormat(t *testing.T) { assert.Contains(t, client.CreatedFiles[0].Message, "chore: enable 2 repositories") } +func TestRunEnableRepos_UpdatesOrgVariableVisibility(t *testing.T) { + // Setup: initial config with repo A enabled, repo B disabled. + // Dispatch mode is oidc-mint. Org variable FULLSEND_MINT_URL exists. + cfg := setupTestConfig(map[string]bool{ + "web-app": true, + "api": false, + }) + cfg.Dispatch.Mode = "oidc-mint" + + client := setupTestClient("testorg", cfg, []string{"web-app", "api"}) + // Assign repo IDs so we can verify they appear in the variable visibility. + for i := range client.Repos { + switch client.Repos[i].Name { + case ".fullsend": + client.Repos[i].ID = 100 + case "web-app": + client.Repos[i].ID = 200 + case "api": + client.Repos[i].ID = 300 + } + } + // Pre-populate the org variable so it "exists". + client.OrgVariables = map[string]bool{"testorg/FULLSEND_MINT_URL": true} + client.OrgVariableValues = map[string]string{"testorg/FULLSEND_MINT_URL": "https://mint.example.com"} + + printer := ui.New(&discardWriter{}) + + // Action: enable repo "api". + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"api"}, false, true) + require.NoError(t, err) + + // Assert: SetOrgVariableRepos was called with both enrolled repo IDs + // plus the config repo (.fullsend). + require.Contains(t, client.OrgVariableRepoIDs, "testorg/FULLSEND_MINT_URL") + repoIDs := client.OrgVariableRepoIDs["testorg/FULLSEND_MINT_URL"] + assert.Contains(t, repoIDs, int64(100), "config repo ID should be included") + assert.Contains(t, repoIDs, int64(200), "web-app repo ID should be included") + assert.Contains(t, repoIDs, int64(300), "api repo ID should be included") +} + +func TestRunEnableRepos_SkipsVariableSyncWhenNotOIDCMint(t *testing.T) { + // When dispatch mode is not oidc-mint, variable sync should be skipped. + cfg := setupTestConfig(map[string]bool{ + "web-app": false, + }) + // No dispatch mode set (empty string). + + client := setupTestClient("testorg", cfg, []string{"web-app"}) + client.OrgVariables = map[string]bool{"testorg/FULLSEND_MINT_URL": true} + + printer := ui.New(&discardWriter{}) + + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true) + require.NoError(t, err) + + // SetOrgVariableRepos should not have been called. + assert.Nil(t, client.OrgVariableRepoIDs) +} + +func TestRunEnableRepos_SkipsVariableSyncWhenVariableNotExists(t *testing.T) { + // When the org variable doesn't exist yet (mint not provisioned), + // sync should skip gracefully. + cfg := setupTestConfig(map[string]bool{ + "web-app": false, + }) + cfg.Dispatch.Mode = "oidc-mint" + + client := setupTestClient("testorg", cfg, []string{"web-app"}) + // No OrgVariables set — FULLSEND_MINT_URL doesn't exist. + + printer := ui.New(&discardWriter{}) + + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true) + require.NoError(t, err) + + // SetOrgVariableRepos should not have been called. + assert.Nil(t, client.OrgVariableRepoIDs) +} + // Business logic tests for runDisableRepos func TestRunDisableRepos_DisableSingleRepo(t *testing.T) { diff --git a/internal/forge/fake.go b/internal/forge/fake.go index a754f1cf6..6cb38e2a7 100644 --- a/internal/forge/fake.go +++ b/internal/forge/fake.go @@ -109,8 +109,9 @@ type FakeClient struct { OrgSecretRepoIDs map[string][]int64 // key: "org/name" → repo IDs // Org-level variable state - OrgVariables map[string]bool // key: "org/name" - OrgVariableValues map[string]string // key: "org/name" → value + OrgVariables map[string]bool // key: "org/name" + OrgVariableValues map[string]string // key: "org/name" → value + OrgVariableRepoIDs map[string][]int64 // key: "org/name" → repo IDs // Error injection: key is method name, value is error to return. Errors map[string]error @@ -936,6 +937,11 @@ func (f *FakeClient) CreateOrUpdateOrgVariable(_ context.Context, org, name, val f.OrgVariableValues = make(map[string]string) } f.OrgVariableValues[org+"/"+name] = value + + if f.OrgVariableRepoIDs == nil { + f.OrgVariableRepoIDs = make(map[string][]int64) + } + f.OrgVariableRepoIDs[org+"/"+name] = selectedRepoIDs return nil } @@ -953,6 +959,35 @@ func (f *FakeClient) OrgVariableExists(_ context.Context, org, name string) (boo return f.OrgVariables[org+"/"+name], nil } +func (f *FakeClient) SetOrgVariableRepos(_ context.Context, org, name string, repoIDs []int64) error { + f.mu.Lock() + defer f.mu.Unlock() + + if e := f.err("SetOrgVariableRepos"); e != nil { + return e + } + + if f.OrgVariableRepoIDs == nil { + f.OrgVariableRepoIDs = make(map[string][]int64) + } + f.OrgVariableRepoIDs[org+"/"+name] = repoIDs + return nil +} + +func (f *FakeClient) GetOrgVariableRepos(_ context.Context, org, name string) ([]int64, error) { + f.mu.Lock() + defer f.mu.Unlock() + + if e := f.err("GetOrgVariableRepos"); e != nil { + return nil, e + } + + if f.OrgVariableRepoIDs == nil { + return nil, nil + } + return f.OrgVariableRepoIDs[org+"/"+name], nil +} + func (f *FakeClient) DeleteOrgVariable(_ context.Context, org, name string) error { f.mu.Lock() defer f.mu.Unlock() diff --git a/internal/forge/fake_test.go b/internal/forge/fake_test.go index 6ea573120..8da76a826 100644 --- a/internal/forge/fake_test.go +++ b/internal/forge/fake_test.go @@ -457,6 +457,13 @@ func TestFakeClient_ErrorInjection(t *testing.T) { {"DeleteOrgVariable", func(fc *FakeClient) error { return fc.DeleteOrgVariable(ctx, "o", "n") }}, + {"SetOrgVariableRepos", func(fc *FakeClient) error { + return fc.SetOrgVariableRepos(ctx, "o", "n", nil) + }}, + {"GetOrgVariableRepos", func(fc *FakeClient) error { + _, err := fc.GetOrgVariableRepos(ctx, "o", "n") + return err + }}, } for _, m := range methods { @@ -522,6 +529,8 @@ func TestFakeClient_ThreadSafety(t *testing.T) { _ = fc.CreateOrUpdateOrgVariable(ctx, "o", "n", "v", []int64{1}) _, _ = fc.OrgVariableExists(ctx, "o", "var") _ = fc.DeleteOrgVariable(ctx, "o", "n") + _ = fc.SetOrgVariableRepos(ctx, "o", "n", []int64{1, 2}) + _, _ = fc.GetOrgVariableRepos(ctx, "o", "n") }(i) } diff --git a/internal/forge/forge.go b/internal/forge/forge.go index f2290ad45..ba80582a7 100644 --- a/internal/forge/forge.go +++ b/internal/forge/forge.go @@ -178,6 +178,10 @@ type Client interface { CreateOrUpdateOrgVariable(ctx context.Context, org, name, value string, selectedRepoIDs []int64) error OrgVariableExists(ctx context.Context, org, name string) (bool, error) DeleteOrgVariable(ctx context.Context, org, name string) error + SetOrgVariableRepos(ctx context.Context, org, name string, repoIDs []int64) error + // GetOrgVariableRepos returns the list of repository IDs that have access + // to the given org-level variable. + GetOrgVariableRepos(ctx context.Context, org, name string) ([]int64, error) // CI/Workflow operations GetLatestWorkflowRun(ctx context.Context, owner, repo, workflowFile string) (*WorkflowRun, error) diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index 58d0a25bd..6fd29b88f 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -1756,6 +1756,47 @@ func (c *LiveClient) DeleteOrgVariable(ctx context.Context, org, name string) er return &APIError{StatusCode: resp.StatusCode, Message: "unexpected status deleting org variable"} } +// SetOrgVariableRepos sets the list of repositories that can access an org variable. +func (c *LiveClient) SetOrgVariableRepos(ctx context.Context, org, name string, repoIDs []int64) error { + if repoIDs == nil { + repoIDs = []int64{} + } + payload := map[string]any{ + "selected_repository_ids": repoIDs, + } + + resp, err := c.put(ctx, fmt.Sprintf("/orgs/%s/actions/variables/%s/repositories", org, name), payload) + if err != nil { + return fmt.Errorf("set org variable repos for %s: %w", name, err) + } + resp.Body.Close() + return nil +} + +// GetOrgVariableRepos returns the repository IDs that have access to an org variable. +func (c *LiveClient) GetOrgVariableRepos(ctx context.Context, org, name string) ([]int64, error) { + resp, err := c.get(ctx, fmt.Sprintf("/orgs/%s/actions/variables/%s/repositories", org, name)) + if err != nil { + return nil, fmt.Errorf("get org variable repos for %s: %w", name, err) + } + defer resp.Body.Close() + + var result struct { + Repositories []struct { + ID int64 `json:"id"` + } `json:"repositories"` + } + if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { + return nil, fmt.Errorf("decode org variable repos for %s: %w", name, err) + } + + ids := make([]int64, len(result.Repositories)) + for i, r := range result.Repositories { + ids[i] = r.ID + } + return ids, nil +} + // isNotFound checks whether an error is a 404 API error. func isNotFound(err error) bool { var apiErr *APIError