diff --git a/docs/ADRs/0044-deprecate-per-org-installation-mode.md b/docs/ADRs/0044-deprecate-per-org-installation-mode.md index 025acb94d..983cd66d0 100644 --- a/docs/ADRs/0044-deprecate-per-org-installation-mode.md +++ b/docs/ADRs/0044-deprecate-per-org-installation-mode.md @@ -333,8 +333,8 @@ ADR 0045 is accepted and its implementation (Phases 1–4) is complete. `OrgVariableExists`, `DeleteOrgVariable`, `SetOrgVariableRepos`, `GetOrgVariableRepos`, `CreateOrgSecret`, `OrgSecretExists`, `DeleteOrgSecret`, `SetOrgSecretRepos`, `GetOrgSecretRepos`, - `ListOrgRepos`). Keep `ListOrgInstallations` (per-repo app discovery) - and `GetOrgPlan` (plan detection). + `ListOrgRepos`). Keep `ListOrgInstallations` (now on `GitHubExtensions`, + per-repo app discovery) and `GetOrgPlan` (plan detection). - Remove the `FULLSEND_PER_REPO_INSTALL` guard variable and all guard-checking logic (no longer needed when per-org enrollment does not exist). @@ -374,7 +374,8 @@ release checklist. The summary below captures the high-level ordering. - PR 9: Remove per-org scaffold templates; rename per-repo scaffolds. - PR 10: Remove org-level forge methods (10 secret/variable methods, `ListOrgRepos`); remove `PerRepoGuardVar`; keep - `ListOrgInstallations` (per-repo app discovery) and `GetOrgPlan`. + `ListOrgInstallations` (now on `GitHubExtensions`, per-repo app + discovery) and `GetOrgPlan`. - PR 11: Simplify dispatch provisioner; retain `Config.GitHubOrgs` (needed by `EnsureOrgInMint`). - PR 12: Simplify appsetup; remove per-org branching. diff --git a/docs/ADRs/0067-gitlab-cron-polling-event-dispatch.md b/docs/ADRs/0067-gitlab-cron-polling-event-dispatch.md index 797b76edc..457925f72 100644 --- a/docs/ADRs/0067-gitlab-cron-polling-event-dispatch.md +++ b/docs/ADRs/0067-gitlab-cron-polling-event-dispatch.md @@ -398,7 +398,7 @@ secret storage. protected branches API - `CreatePipelineSchedule` / `DeletePipelineSchedule` — GitLab-native; GitHub returns `ErrNotSupported` -- `UpdateVariable` — for poll watermark management +- `UpdateCIVariable` — for poll watermark management A new `ErrNotSupported` sentinel (complementing the existing forge sentinel errors) allows forge diff --git a/docs/plans/deprecate-per-org-install.md b/docs/plans/deprecate-per-org-install.md index aba10d361..6d13d1328 100644 --- a/docs/plans/deprecate-per-org-install.md +++ b/docs/plans/deprecate-per-org-install.md @@ -33,7 +33,7 @@ The per-org model touches these subsystems: | Layer stack | `internal/layers/configrepo.go`, `dispatch.go`, `enrollment.go`, `secrets.go`, `inference.go`, `vendorbinary.go` | `ConfigRepoLayer`, `DispatchTokenLayer`, `EnrollmentLayer`, `buildLayerStack()` | | Config | `internal/config/config.go` | `OrgConfig`, `NewOrgConfig`, `ParseOrgConfig`, `EnabledRepos`, `DisabledRepos`, `DefaultAgentRoles` | | Scaffold | `internal/scaffold/fullsend-repo/` | `dispatch.yml`, thin callers, `shim-workflow-call.yaml`, `repo-maintenance.yml`, `CustomizedDirs()` | -| Forge interface | `internal/forge/forge.go`, `github/github.go`, `fake.go` | 11 org-level methods to remove (5 secret, 5 variable, `ListOrgRepos`), 2 retained (`ListOrgInstallations`, `GetOrgPlan`), `PerRepoGuardVar` | +| Forge interface | `internal/forge/forge.go`, `github/github.go`, `fake.go` | 11 org-level methods to remove (5 secret, 5 variable, `ListOrgRepos`), 2 retained (`ListOrgInstallations` via `GitHubExtensions`, `GetOrgPlan`), `PerRepoGuardVar` | | Dispatch | `internal/dispatch/dispatch.go`, `gcf/provisioner.go` | `OrgSecretNames`, `OrgVariableNames`, PEM-sharing logic, org-wide WIF | | Appsetup | `internal/appsetup/appsetup.go` | `findExistingInstallation` (org-level app discovery) | | Mint | `internal/mintcore/claims.go` | `.fullsend` workflow ref pattern, `ValidateOrgAllowed` | @@ -641,9 +641,9 @@ SetOrgVariableRepos(ctx context.Context, org, name string, repoIDs []int64) erro GetOrgVariableRepos(ctx context.Context, org, name string) ([]int64, error) // Org installations — DO NOT remove: -// ListOrgInstallations is also used by per-repo install path -// (detectSharedApps in admin.go, findExistingInstallation and -// ensureInstalled in appsetup.go). +// ListOrgInstallations (now on GitHubExtensions) is also used by per-repo +// install path (detectSharedApps in admin.go, findExistingInstallation +// and ensureInstalled in appsetup.go). ``` Remove `PerRepoGuardVar` constant and all its callers (these are @@ -667,8 +667,9 @@ The guard variable is no longer needed when per-org enrollment does not exist. All callers are explicitly covered: per-repo paths in this PR, `enrollment.go` in PR 7, and `reconcile-repos.sh` in PR 9. -Keep `ListOrgInstallations()` — used by per-repo app discovery -(`detectSharedApps`, `findExistingInstallation`, `ensureInstalled`). +Keep `ListOrgInstallations()` (now on `GitHubExtensions`) — used by +per-repo app discovery (`detectSharedApps`, `findExistingInstallation`, +`ensureInstalled`). Remove `ListOrgRepos()` — all callers (`admin.go` enrollment discovery, `github.go` org setup, `dispatch.go` layer) are per-org code paths removed in earlier PRs; no per-repo callers exist. @@ -683,7 +684,7 @@ Remove implementations of the 10 removed interface methods: - `CreateOrUpdateOrgVariable`, `OrgVariableExists`, `DeleteOrgVariable`, `SetOrgVariableRepos`, `GetOrgVariableRepos` (~200 lines) -Keep `ListOrgInstallations` implementation (retained in interface). +Keep `ListOrgInstallations` implementation (retained on `GitHubExtensions`). **`internal/forge/fake.go`:** @@ -701,8 +702,8 @@ Remove: - Any test files that use fake org methods: update to remove. **After merge:** Forge interface has no org-level secret/variable methods. -`ListOrgInstallations` retained for per-repo app discovery. ~400 lines -removed from GitHub implementation. +`ListOrgInstallations` retained (on `GitHubExtensions`) for per-repo app +discovery. ~400 lines removed from GitHub implementation. --- @@ -753,8 +754,9 @@ Keep `Provision()`, `StoreAgentPEM()`, `Name()`. **`internal/appsetup/appsetup.go`:** - Keep `findExistingInstallation()` and `ensureInstalled()` — these use - `ListOrgInstallations()` which is retained (app installation discovery - is an org-level GitHub API operation regardless of fullsend install mode). + `ListOrgInstallations()` via `GitHubExtensions` type assertion (app + installation discovery is an org-level GitHub API operation regardless + of fullsend install mode). - Simplify `Run()`: remove any per-org-specific branching (e.g. org-level app creation paths that differ from per-repo). The shared app discovery flow (`detectSharedApps` → `findExistingInstallation`) works for both @@ -771,7 +773,7 @@ Keep `Provision()`, `StoreAgentPEM()`, `Name()`. cases. **After merge:** Appsetup retains shared app discovery -(`findExistingInstallation`, `ensureInstalled` via `ListOrgInstallations`) +(`findExistingInstallation`, `ensureInstalled` via `GitHubExtensions.ListOrgInstallations`) while per-org-specific branching is removed. --- diff --git a/docs/plans/gitlab-cron-polling-implementation.md b/docs/plans/gitlab-cron-polling-implementation.md index 036a92538..12983bc01 100644 --- a/docs/plans/gitlab-cron-polling-implementation.md +++ b/docs/plans/gitlab-cron-polling-implementation.md @@ -36,14 +36,14 @@ Add to `internal/forge/forge.go`: ```go IsProtectedBranch(ctx context.Context, owner, repo, branch string) (bool, error) -CreatePipelineSchedule(ctx context.Context, owner, repo, ref, description, cron string, variables map[string]string) (scheduleID string, err error) -DeletePipelineSchedule(ctx context.Context, owner, repo, scheduleID string) error +CreatePipelineSchedule(ctx context.Context, owner, repo, ref, description, cron string, variables map[string]string) (int64, error) +DeletePipelineSchedule(ctx context.Context, owner, repo string, scheduleID int64) error ListPipelineSchedules(ctx context.Context, owner, repo string) ([]PipelineSchedule, error) -UpdateVariable(ctx context.Context, owner, repo, key, value string) error -CreateProtectedVariable(ctx context.Context, owner, repo, key, value string) error +UpdateCIVariable(ctx context.Context, owner, repo, name, value string, protected bool) error +CreateProtectedCIVariable(ctx context.Context, owner, repo, name, value string) error ``` -These methods are forge-neutral by design. `IsProtectedBranch` maps to GitHub's branch protection API and GitLab's protected branches API. `CreatePipelineSchedule` and `DeletePipelineSchedule` are GitLab-native; the GitHub implementation returns `ErrNotSupported`. `UpdateVariable` maps to GitLab's CI/CD variable API. `CreateProtectedVariable` creates a CI/CD variable with `Protected: true, Masked: false` — used for poll state variables (watermark, label state) that must not be accessible on non-protected branches but whose values are not secrets. +These methods are forge-neutral by design. `IsProtectedBranch` maps to GitHub's branch protection API and GitLab's protected branches API. `CreatePipelineSchedule` and `DeletePipelineSchedule` are GitLab-native; the GitHub implementation returns `ErrNotSupported`. `UpdateCIVariable` maps to GitLab's CI/CD variable API. `CreateProtectedCIVariable` creates a CI/CD variable with `Protected: true, Masked: false` — used for poll state variables (watermark, label state) that must not be accessible on non-protected branches but whose values are not secrets. The "CI" prefix distinguishes these from the existing `RepoVariable` methods which model GitHub Actions variables. ### New sentinel error @@ -206,8 +206,8 @@ Single-token constructor for the bot project access token. The token is used for | `CreatePipelineSchedule` | `PipelineSchedules.CreatePipelineSchedule` | GitLab-specific | | `DeletePipelineSchedule` | `PipelineSchedules.DeletePipelineSchedule` | GitLab-specific | | `ListPipelineSchedules` | `PipelineSchedules.ListProjectPipelineSchedules` | For uninstall cleanup | -| `UpdateVariable` | `ProjectVariables.UpdateVariable` | For poll watermark | -| `CreateProtectedVariable` | `ProjectVariables.CreateVariable` | With `Protected: true`, `Masked: false` — for poll state | +| `UpdateCIVariable` | `ProjectVariables.UpdateVariable` | For poll watermark | +| `CreateProtectedCIVariable` | `ProjectVariables.CreateVariable` | With `Protected: true`, `Masked: false` — for poll state | | `DispatchWorkflow` | → `ErrNotSupported` | GitHub-only | | `ListOrgInstallations` | → `GitHubExtensions` (not on base interface) | GitHub-only | | `GetAppClientID` | → `GitHubExtensions` (not on base interface) | GitHub-only | @@ -1061,7 +1061,7 @@ func (p *Poller) persistLabelState(ctx context.Context, owner, repo string, stat if err != nil { return fmt.Errorf("marshal label state: %w", err) } - if err := p.client.UpdateVariable(ctx, owner, repo, "FULLSEND_LABEL_STATE", string(stateBytes)); err != nil { + if err := p.client.UpdateCIVariable(ctx, owner, repo, "FULLSEND_LABEL_STATE", string(stateBytes), true); err != nil { return fmt.Errorf("persist label state: %w", err) } return nil @@ -1093,7 +1093,7 @@ func (p *Poller) watermarkVarName() string { } func (p *Poller) updateWatermark(ctx context.Context, owner, repo string, t time.Time) error { - return p.client.UpdateVariable(ctx, owner, repo, p.watermarkVarName(), t.Format(time.RFC3339)) + return p.client.UpdateCIVariable(ctx, owner, repo, p.watermarkVarName(), t.Format(time.RFC3339), true) } ``` @@ -1673,19 +1673,19 @@ func runGitLabPerRepoInstall(ctx context.Context, target string, opts installOpt "chore: add fullsend CI/CD pipeline", scaffoldFiles) // 12. Set protected CI/CD variables. - // Use CreateProtectedVariable (Protected: true, Masked: false) for + // Use CreateProtectedCIVariable (Protected: true, Masked: false) for // configuration identifiers — CreateRepoSecret (Protected + Masked) // requires values >= 8 characters (e.g. "wif" would fail) and masks // GCP resource names in logs, hindering debugging. - client.CreateProtectedVariable(ctx, owner, repo, "FULLSEND_CREDENTIAL_MODE", credentialMode) + client.CreateProtectedCIVariable(ctx, owner, repo, "FULLSEND_CREDENTIAL_MODE", credentialMode) if credentialMode == "wif" { - client.CreateProtectedVariable(ctx, owner, repo, "FULLSEND_WIF_PROVIDER", wifProviderResourceName) - client.CreateProtectedVariable(ctx, owner, repo, "FULLSEND_SA", serviceAccountEmail) - client.CreateProtectedVariable(ctx, owner, repo, "FULLSEND_BOT_TOKEN_SECRET", secretManagerSecretName) - client.CreateProtectedVariable(ctx, owner, repo, "FULLSEND_GCP_PROJECT_ID", opts.gcpProject) + client.CreateProtectedCIVariable(ctx, owner, repo, "FULLSEND_WIF_PROVIDER", wifProviderResourceName) + client.CreateProtectedCIVariable(ctx, owner, repo, "FULLSEND_SA", serviceAccountEmail) + client.CreateProtectedCIVariable(ctx, owner, repo, "FULLSEND_BOT_TOKEN_SECRET", secretManagerSecretName) + client.CreateProtectedCIVariable(ctx, owner, repo, "FULLSEND_GCP_PROJECT_ID", opts.gcpProject) } - client.CreateProtectedVariable(ctx, owner, repo, "FULLSEND_FORGE", "gitlab") - client.CreateProtectedVariable(ctx, owner, repo, "FULLSEND_PER_REPO_INSTALL", "true") + client.CreateProtectedCIVariable(ctx, owner, repo, "FULLSEND_FORGE", "gitlab") + client.CreateProtectedCIVariable(ctx, owner, repo, "FULLSEND_PER_REPO_INSTALL", "true") // 13. Initialize poll watermarks (protected — must not be accessible // to pipelines on non-protected branches to prevent tampering). @@ -1696,9 +1696,9 @@ func runGitLabPerRepoInstall(ctx context.Context, target string, opts installOpt // full-poll re-discovers it) is harmless — resource_group serialization // ensures at most one pipeline at a time; see ADR 0067. initTime := time.Now().Format(time.RFC3339) - client.CreateProtectedVariable(ctx, owner, repo, "FULLSEND_LAST_POLL_AT_FAST", initTime) - client.CreateProtectedVariable(ctx, owner, repo, "FULLSEND_LAST_POLL_AT_FULL", initTime) - client.CreateProtectedVariable(ctx, owner, repo, "FULLSEND_LABEL_STATE", "{}") + client.CreateProtectedCIVariable(ctx, owner, repo, "FULLSEND_LAST_POLL_AT_FAST", initTime) + client.CreateProtectedCIVariable(ctx, owner, repo, "FULLSEND_LAST_POLL_AT_FULL", initTime) + client.CreateProtectedCIVariable(ctx, owner, repo, "FULLSEND_LABEL_STATE", "{}") // 14. Set up inference WIF (if --inference-project provided) @@ -1816,7 +1816,7 @@ Add implementations to `internal/forge/fake.go` for: - `IsProtectedBranch` — configurable return value - `CreatePipelineSchedule` — record call, return fake schedule ID - `DeletePipelineSchedule` — record call -- `UpdateVariable` — record call +- `UpdateCIVariable` — record call ## Security-Critical Code Paths diff --git a/docs/problems/gitlab-implementation.md b/docs/problems/gitlab-implementation.md index 271362fef..516920d42 100644 --- a/docs/problems/gitlab-implementation.md +++ b/docs/problems/gitlab-implementation.md @@ -379,7 +379,7 @@ This keeps the dispatch scanning logic identical across GitHub and GitLab. ## Forge Interface Evolution -**Challenge**: ADR-0005 promises "Adding a new forge requires implementing `forge.Client` — no changes to layers, CLI, or app setup code." However, the current `forge.Client` interface contains GitHub-specific methods (`ListOrgInstallations`, `GetAppClientID`) and operations (`DispatchWorkflow`) that don't map directly to GitLab. +**Challenge**: ADR-0005 promises "Adding a new forge requires implementing `forge.Client` — no changes to layers, CLI, or app setup code." However, the `forge.Client` interface previously contained GitHub-specific methods (`ListOrgInstallations`, `GetAppClientID`) that don't map to GitLab. These have since been moved to the `forge.GitHubExtensions` interface, and callers use type assertions. Operations like `DispatchWorkflow` remain on the base interface. ### Proposed Forge-Neutral Interface Additions @@ -418,23 +418,16 @@ DeleteWebhook(ctx context.Context, owner, repo, webhookID string) error ### Existing GitHub-Specific Methods -- `ListOrgInstallations(ctx, org) ([]Installation, error)` — GitHub App-specific. GitLab equivalent would list Project Access Tokens, but tokens are scoped per-project not org-wide. This method may need to become forge-specific or return an empty list for non-GitHub forges. -- `GetAppClientID(ctx, slug) (string, error)` — GitHub App-specific. No GitLab equivalent. This should be deprecated or moved to a GitHub-specific extension interface. +- `ListOrgInstallations(ctx, org) ([]Installation, error)` — GitHub App-specific. **Now moved to `forge.GitHubExtensions`**. GitLab equivalent would list Project Access Tokens, but tokens are scoped per-project not org-wide. Callers type-assert to `GitHubExtensions` and handle the fallback. +- `GetAppClientID(ctx, slug) (string, error)` — GitHub App-specific. No GitLab equivalent. **Now moved to `forge.GitHubExtensions`**. Callers type-assert and fall back gracefully. - `DispatchWorkflow(ctx, owner, repo, workflowFile, ref, inputs)` — GitHub Actions-specific (targets a specific workflow file). Replaced by forge-neutral `TriggerPipeline` above. ### Backward Compatibility and Migration Strategy To prevent interface bloat while maintaining backward compatibility: -1. **Deprecation phase**: Mark GitHub-specific methods with deprecation comments and update callers to use forge-neutral equivalents (`TriggerPipeline` instead of `DispatchWorkflow`, etc.). This phase allows gradual migration without breaking existing code. -2. **Extension interfaces**: Move forge-specific methods that have no neutral equivalent (e.g., `GetAppClientID`) to optional extension interfaces: - ```go - type GitHubForgeClient interface { - Client - GetAppClientID(ctx context.Context, slug string) (string, error) - } - ``` - Callers that need GitHub-specific behavior can type-assert to the extension interface. +1. **Extension interfaces (implemented)**: GitHub-specific methods (`ListOrgInstallations`, `GetAppClientID`) have been moved to the `forge.GitHubExtensions` optional extension interface. Callers type-assert and handle non-GitHub forges gracefully. +2. **Deprecation phase**: Mark remaining GitHub-specific methods with deprecation comments and update callers to use forge-neutral equivalents (`TriggerPipeline` instead of `DispatchWorkflow`, etc.). This phase allows gradual migration without breaking existing code. 3. **Breaking change timeline**: After all internal callers migrate to forge-neutral methods, remove deprecated methods in a major version bump. Document this timeline in the interface godoc (e.g., "deprecated: use TriggerPipeline, will be removed in v2.0.0"). This strategy limits interface growth to forge-neutral primitives while preserving GitHub-specific functionality via opt-in extension interfaces. diff --git a/internal/appsetup/appsetup.go b/internal/appsetup/appsetup.go index 87543d184..5836d80a9 100644 --- a/internal/appsetup/appsetup.go +++ b/internal/appsetup/appsetup.go @@ -263,7 +263,16 @@ func (s *Setup) Run(ctx context.Context, org, role string) (*AppCredentials, err // just need to be installed — not re-created via the manifest flow. // When --public was passed, install without prompting. Otherwise, ask // the user to confirm so they don't accidentally create a duplicate. - clientID, lookupErr := s.client.GetAppClientID(ctx, slug) + var clientID string + var lookupErr error + if ghExt, ok := s.client.(forge.GitHubExtensions); ok { + clientID, lookupErr = ghExt.GetAppClientID(ctx, slug) + } else { + lookupErr = forge.ErrNotSupported + } + if forge.IsNotSupported(lookupErr) { + return nil, fmt.Errorf("app setup requires GitHub: %w", forge.ErrNotSupported) + } if lookupErr != nil && !forge.IsNotFound(lookupErr) { return nil, fmt.Errorf("checking existing app %s: %w", slug, lookupErr) } @@ -343,8 +352,13 @@ func (s *Setup) recoverCreatedApp(ctx context.Context, org, role, slug string) ( } } + ghExt, ok := s.client.(forge.GitHubExtensions) + if !ok { + s.ui.StepInfo("Skipping app recovery: forge does not support GitHub App lookups") + return nil, nil + } for _, candidate := range candidates { - clientID, err := s.client.GetAppClientID(ctx, candidate) + clientID, err := ghExt.GetAppClientID(ctx, candidate) if err != nil { continue } @@ -371,7 +385,11 @@ func (s *Setup) recoverCreatedApp(ctx context.Context, org, role, slug string) ( func (s *Setup) findExistingInstallation( ctx context.Context, org, role, expectedSlug string, ) (*forge.Installation, bool, error) { - installations, err := s.client.ListOrgInstallations(ctx, org) + ghExt, ok := s.client.(forge.GitHubExtensions) + if !ok { + return nil, false, nil + } + installations, err := ghExt.ListOrgInstallations(ctx, org) if err != nil { return nil, false, err } @@ -532,7 +550,11 @@ func (s *Setup) isAppIDStale(org, role string, liveID int) bool { func (s *Setup) handleExistingApp(ctx context.Context, inst *forge.Installation, org, role string) (*AppCredentials, error) { s.ui.StepDone(fmt.Sprintf("Found existing app: %s (ID: %d)", inst.AppSlug, inst.AppID)) - clientID, err := s.client.GetAppClientID(ctx, inst.AppSlug) + ghExt, ok := s.client.(forge.GitHubExtensions) + if !ok { + return nil, fmt.Errorf("looking up client ID for %s: %w", inst.AppSlug, forge.ErrNotSupported) + } + clientID, err := ghExt.GetAppClientID(ctx, inst.AppSlug) if err != nil { return nil, fmt.Errorf("looking up client ID for %s: %w", inst.AppSlug, err) } @@ -851,7 +873,12 @@ const installPollInterval = 2 * time.Second const installPollTimeout = 5 * time.Minute func (s *Setup) ensureInstalled(ctx context.Context, org, slug string) error { - installations, err := s.client.ListOrgInstallations(ctx, org) + ghExt, ok := s.client.(forge.GitHubExtensions) + if !ok { + return fmt.Errorf("listing installations: %w", forge.ErrNotSupported) + } + + installations, err := ghExt.ListOrgInstallations(ctx, org) if err != nil { return fmt.Errorf("listing installations: %w", err) } @@ -884,7 +911,7 @@ func (s *Setup) ensureInstalled(ctx context.Context, org, slug string) error { case <-pollCtx.Done(): return fmt.Errorf("timed out waiting for app %s to be installed on %s", slug, org) case <-time.After(installPollInterval): - installations, err := s.client.ListOrgInstallations(pollCtx, org) + installations, err := ghExt.ListOrgInstallations(pollCtx, org) if err != nil { continue // transient errors — keep polling } diff --git a/internal/appsetup/appsetup_test.go b/internal/appsetup/appsetup_test.go index 3e01678e6..a72098391 100644 --- a/internal/appsetup/appsetup_test.go +++ b/internal/appsetup/appsetup_test.go @@ -1152,6 +1152,81 @@ func TestValidateRSAPEM_NonRSAKey(t *testing.T) { assert.Contains(t, err.Error(), "expected RSA") } +// nonGitHubClient wraps a forge.Client without implementing +// forge.GitHubExtensions, so type assertions to that interface fail. +type nonGitHubClient struct { + forge.Client +} + +func TestSetup_Run_NonGitHub_ReturnsErrNotSupported(t *testing.T) { + client := &nonGitHubClient{Client: forge.NewFakeClient()} + prompter := &fakePrompter{} + browser := newFakeBrowser() + printer := ui.New(&discardWriter{}) + + s := NewSetup(client, prompter, browser, printer) + _, err := s.Run(context.Background(), "myorg", "fullsend") + require.Error(t, err) + assert.Contains(t, err.Error(), "app setup requires GitHub") + assert.True(t, forge.IsNotSupported(err)) +} + +func TestSetup_RecoverCreatedApp_NonGitHub_ReturnsNil(t *testing.T) { + client := &nonGitHubClient{Client: forge.NewFakeClient()} + printer := ui.New(&discardWriter{}) + + s := &Setup{ + client: client, + ui: printer, + secretExists: func(string) (bool, error) { return true, nil }, + } + creds, err := s.recoverCreatedApp(context.Background(), "myorg", "fullsend", "fullsend-ai-fullsend") + require.NoError(t, err) + assert.Nil(t, creds) +} + +func TestSetup_HandleExistingApp_NonGitHub_ReturnsError(t *testing.T) { + client := &nonGitHubClient{Client: forge.NewFakeClient()} + printer := ui.New(&discardWriter{}) + + s := &Setup{ + client: client, + ui: printer, + } + inst := &forge.Installation{AppSlug: "test-app", AppID: 42} + _, err := s.handleExistingApp(context.Background(), inst, "myorg", "fullsend") + require.Error(t, err) + assert.True(t, forge.IsNotSupported(err)) + assert.Contains(t, err.Error(), "looking up client ID") +} + +func TestSetup_EnsureInstalled_NonGitHub_ReturnsError(t *testing.T) { + client := &nonGitHubClient{Client: forge.NewFakeClient()} + printer := ui.New(&discardWriter{}) + + s := &Setup{ + client: client, + ui: printer, + } + err := s.ensureInstalled(context.Background(), "myorg", "test-app") + require.Error(t, err) + assert.True(t, forge.IsNotSupported(err)) +} + +func TestSetup_FindExistingInstallation_NonGitHub_ReturnsNil(t *testing.T) { + client := &nonGitHubClient{Client: forge.NewFakeClient()} + printer := ui.New(&discardWriter{}) + + s := &Setup{ + client: client, + ui: printer, + } + inst, found, err := s.findExistingInstallation(context.Background(), "myorg", "fullsend", "fullsend-ai-fullsend") + require.NoError(t, err) + assert.False(t, found) + assert.Nil(t, inst) +} + // discardWriter implements io.Writer, discarding all output. type discardWriter struct{} diff --git a/internal/cli/admin.go b/internal/cli/admin.go index 220673436..9d894d358 100644 --- a/internal/cli/admin.go +++ b/internal/cli/admin.go @@ -1400,7 +1400,11 @@ func resolveSharedRoleAppIDs(ctx context.Context, client forge.Client, existingI return nil, fmt.Errorf("mint has no existing ROLE_APP_IDS — cannot determine app IDs for %s", owner) } - installations, err := client.ListOrgInstallations(ctx, owner) + ghExt, ok := client.(forge.GitHubExtensions) + if !ok { + return nil, fmt.Errorf("listing installations for %s: %w", owner, forge.ErrNotSupported) + } + installations, err := ghExt.ListOrgInstallations(ctx, owner) if err != nil { return nil, fmt.Errorf("listing installations for %s: %w", owner, err) } @@ -1452,7 +1456,11 @@ func detectSharedApps(ctx context.Context, client forge.Client, printer *ui.Prin } roleOnly := mintcore.RoleOnlyAppIDs(existingIDs) - installations, err := client.ListOrgInstallations(ctx, org) + ghExt, ok := client.(forge.GitHubExtensions) + if !ok { + return nil, roleOnly, nil + } + installations, err := ghExt.ListOrgInstallations(ctx, org) if err != nil { return nil, roleOnly, nil } @@ -1837,8 +1845,17 @@ func runUninstall(ctx context.Context, client forge.Client, printer *ui.Printer, // Find which slugs correspond to real installed apps. var existingSlugs []string appIDs := make(map[string]int) - appInstallations, listErr := client.ListOrgInstallations(ctx, org) - if listErr == nil { + ghExt, ghOK := client.(forge.GitHubExtensions) + var appInstallations []forge.Installation + var listErr error + if ghOK { + appInstallations, listErr = ghExt.ListOrgInstallations(ctx, org) + } else { + listErr = forge.ErrNotSupported + } + if forge.IsNotSupported(listErr) { + printer.StepInfo("App uninstall is not available on this forge — skipping") + } else if listErr == nil { for _, inst := range appInstallations { appIDs[inst.AppSlug] = inst.ID } @@ -1881,7 +1898,7 @@ func runUninstall(ctx context.Context, client forge.Client, printer *ui.Printer, printer.Blank() printer.StepStart("Verifying if apps were removed") - freshInstalls, verifyErr := client.ListOrgInstallations(ctx, org) + freshInstalls, verifyErr := ghExt.ListOrgInstallations(ctx, org) if verifyErr != nil { printer.StepWarn(fmt.Sprintf("Could not get installations for org %s: %v", org, verifyErr)) } else { diff --git a/internal/cli/admin_test.go b/internal/cli/admin_test.go index e8c5b407f..fcd1adf86 100644 --- a/internal/cli/admin_test.go +++ b/internal/cli/admin_test.go @@ -3289,6 +3289,54 @@ func TestGCFWIFAdapter_DeletePerRepoWIF_Success(t *testing.T) { require.NoError(t, err) } +// nonGitHubClient wraps forge.Client without GitHubExtensions. +type nonGitHubClient struct { + forge.Client +} + +func TestRunUninstall_NonGitHub_SkipsAppUninstall(t *testing.T) { + inner := forge.NewFakeClient() + inner.TokenScopes = []string{"admin:org", "repo", "delete_repo"} + inner.Errors["GetFileContent"] = errors.New("not found") + client := &nonGitHubClient{Client: inner} + + var buf strings.Builder + printer := ui.New(&buf) + + err := runUninstall(context.Background(), client, printer, "test-org", "fullsend-ai", appsetup.NopBrowser{}, strings.NewReader("")) + require.NoError(t, err) + assert.Contains(t, buf.String(), "App uninstall is not available on this forge") +} + +func TestResolveSharedRoleAppIDs_NonGitHub_ReturnsError(t *testing.T) { + inner := forge.NewFakeClient() + client := &nonGitHubClient{Client: inner} + + existingIDs := map[string]string{"coder": "100"} + _, err := resolveSharedRoleAppIDs(context.Background(), client, existingIDs, "acme", []string{"coder"}) + require.Error(t, err) + assert.True(t, forge.IsNotSupported(err)) +} + +func TestDetectSharedApps_NonGitHub_ReturnsRoleOnlyIDs(t *testing.T) { + old := detectSharedAppsGCFClientFactory + detectSharedAppsGCFClientFactory = func(string) gcf.GCFClient { + return gcf.NewFakeGCFClient(gcf.WithFakeFunctionInfo(&gcf.FunctionInfo{ + URI: "https://mint.example.com", + EnvVars: map[string]string{"ROLE_APP_IDS": `{"coder":"100"}`}, + })) + } + t.Cleanup(func() { detectSharedAppsGCFClientFactory = old }) + + inner := forge.NewFakeClient() + client := &nonGitHubClient{Client: inner} + + slugs, roleIDs, err := detectSharedApps(context.Background(), client, ui.New(&strings.Builder{}), "acme", []string{"coder"}, "mint-project", "us-central1") + require.NoError(t, err) + assert.Nil(t, slugs) + assert.Equal(t, map[string]string{"coder": "100"}, roleIDs) +} + func TestToAgentCredentials(t *testing.T) { ac := &appsetup.AppCredentials{ AppID: 42, diff --git a/internal/cli/agent.go b/internal/cli/agent.go index 8359c7e9c..86393dcd3 100644 --- a/internal/cli/agent.go +++ b/internal/cli/agent.go @@ -537,6 +537,9 @@ func parseAgentSourceURL(source string) (*forge.ForgeURLInfo, error) { } info, err = forge.ParseForgeURL(cleanSource) if err == nil { + if info.Forge != "github" { + return nil, fmt.Errorf("forge %q is recognized but fetch support has not landed yet", info.Forge) + } return info, nil } return parseGenericURL(cleanSource) diff --git a/internal/cli/agent_test.go b/internal/cli/agent_test.go index c0f54ef5b..8e062b477 100644 --- a/internal/cli/agent_test.go +++ b/internal/cli/agent_test.go @@ -632,6 +632,13 @@ func TestParseAgentSourceURL_GitHubBlobToRawConversion(t *testing.T) { assert.Equal(t, "https://raw.githubusercontent.com/my-org/agents/"+testCommitSHA+"/harness/triage.yaml", rawURL) } +func TestParseAgentSourceURL_GitLabURLRejected(t *testing.T) { + gitlabURL := "https://gitlab.com/my-org/agents/-/blob/" + testCommitSHA + "/harness/triage.yaml" + _, err := parseAgentSourceURL(gitlabURL) + require.Error(t, err) + assert.Contains(t, err.Error(), "fetch support has not landed yet") +} + func TestRunAgentAdd_NonGitHubUpdateRequiresExplicitSHA(t *testing.T) { dir := t.TempDir() hash := "7777777777777777777777777777777777777777777777777777777777777777" diff --git a/internal/cli/github.go b/internal/cli/github.go index d15db6390..7e440b243 100644 --- a/internal/cli/github.go +++ b/internal/cli/github.go @@ -906,9 +906,17 @@ func runGitHubUninstall(ctx context.Context, client forge.Client, printer *ui.Pr printer.StepDone("Deleted org secret " + name) } - installations, listErr := client.ListOrgInstallations(ctx, org) + var installations []forge.Installation + var listErr error + if ghExt, ok := client.(forge.GitHubExtensions); ok { + installations, listErr = ghExt.ListOrgInstallations(ctx, org) + } else { + listErr = forge.ErrNotSupported + } var existingSlugs []string - if listErr == nil { + if forge.IsNotSupported(listErr) { + printer.StepInfo("App uninstall is not available on this forge — skipping") + } else if listErr == nil { installedSet := make(map[string]bool, len(installations)) for _, inst := range installations { installedSet[inst.AppSlug] = true @@ -921,7 +929,6 @@ func runGitHubUninstall(ctx context.Context, client forge.Client, printer *ui.Pr } } } else { - // Can't check — fall back to showing all of them. printer.StepWarn("Could not verify which apps exist; showing all") existingSlugs = agentSlugs } diff --git a/internal/cli/github_test.go b/internal/cli/github_test.go index 3fbdd3233..e2275e7bf 100644 --- a/internal/cli/github_test.go +++ b/internal/cli/github_test.go @@ -445,6 +445,17 @@ func TestGitHubUninstallCmd_HasFlags(t *testing.T) { require.NotNil(t, appSetFlag, "expected --app-set flag") } +func TestRunGitHubUninstall_NonGitHub_SkipsAppUninstall(t *testing.T) { + inner := forge.NewFakeClient() + client := &nonGitHubClient{Client: inner} + var buf strings.Builder + printer := ui.New(&buf) + + err := runGitHubUninstall(context.Background(), client, printer, "acme", "fullsend-ai") + require.NoError(t, err) + assert.Contains(t, buf.String(), "App uninstall is not available on this forge") +} + func TestRunGitHubUninstall_DeletesResources(t *testing.T) { client := forge.NewFakeClient() client.Repos = []forge.Repository{ diff --git a/internal/fetchsvc/service.go b/internal/fetchsvc/service.go index 78ca88f1a..0b30fea71 100644 --- a/internal/fetchsvc/service.go +++ b/internal/fetchsvc/service.go @@ -130,6 +130,9 @@ func (s *Service) HandleFetch(ctx context.Context, req FetchRequest) (FetchRespo if err != nil { return FetchResponse{}, &fetchError{"skill URLs must be hosted on a supported forge", http.StatusBadRequest} } + if forgeInfo.Forge != "github" { + return FetchResponse{}, &fetchError{fmt.Sprintf("forge %q is recognized but fetch support has not landed yet", forgeInfo.Forge), http.StatusBadRequest} + } if forgeInfo.Path == "" { return FetchResponse{}, &fetchError{"skill URL must include a path to a directory", http.StatusBadRequest} diff --git a/internal/fetchsvc/service_test.go b/internal/fetchsvc/service_test.go index 8f605ee6a..ee83faff3 100644 --- a/internal/fetchsvc/service_test.go +++ b/internal/fetchsvc/service_test.go @@ -295,6 +295,25 @@ func TestHandleFetch_NonForgeURL(t *testing.T) { } } +func TestHandleFetch_GitLabURLRejected(t *testing.T) { + svc := New(ServiceConfig{ + Harness: testHarness("https://gitlab.com/org/repo/"), + WorkspaceRoot: t.TempDir(), + MaxFetches: 10, + }) + + _, err := svc.HandleFetch(context.Background(), FetchRequest{ + URL: "https://gitlab.com/org/repo/-/tree/main/skills/foo#sha256=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + }) + + if err == nil { + t.Fatal("expected error for GitLab URL") + } + if !strings.Contains(err.Error(), "fetch support has not landed yet") { + t.Fatalf("error should mention fetch support: %v", err) + } +} + func TestHandleFetch_OfflineMode(t *testing.T) { svc := New(ServiceConfig{ Harness: testHarness("https://github.com/org/repo/"), diff --git a/internal/forge/detect.go b/internal/forge/detect.go index 5bce359fd..33bdc8dfb 100644 --- a/internal/forge/detect.go +++ b/internal/forge/detect.go @@ -11,8 +11,9 @@ import ( // with a suggestion to use the --forge flag. // // Note: this detects the forge from a remote URL, which is distinct from -// IsSupportedForge (used by ParseForgeURL for full URL parsing support). -// A forge may be detectable here before full URL parsing is implemented. +// IsSupportedForge (which gates fetch support in harness validation). +// URL parsing uses isRecognizedForge; a forge may be parseable before +// fetch support is implemented. func DetectForge(remoteURL string) (string, error) { host := extractHost(remoteURL) if host == "" { diff --git a/internal/forge/detect_test.go b/internal/forge/detect_test.go index 905aa81f4..39d1dd111 100644 --- a/internal/forge/detect_test.go +++ b/internal/forge/detect_test.go @@ -231,5 +231,5 @@ func TestDetectForgeDistinctFromIsSupportedForge(t *testing.T) { assert.Equal(t, "gitlab", forge) assert.False(t, IsSupportedForge("gitlab.com"), - "IsSupportedForge gates ParseForgeURL support, not detection") + "IsSupportedForge gates fetch support (harness validation), not URL parsing or detection") } diff --git a/internal/forge/fake.go b/internal/forge/fake.go index ada52cd6e..96e28874a 100644 --- a/internal/forge/fake.go +++ b/internal/forge/fake.go @@ -7,22 +7,25 @@ import ( "sync" ) -// Compile-time check that FakeClient implements Client. +// Compile-time interface checks. var _ Client = (*FakeClient)(nil) +var _ GitHubExtensions = (*FakeClient)(nil) // NewFakeClient returns a FakeClient with all maps initialised. func NewFakeClient() *FakeClient { return &FakeClient{ - FileContents: make(map[string][]byte), - WorkflowRuns: make(map[string]*WorkflowRun), - Secrets: make(map[string]bool), - VariablesExist: make(map[string]bool), - VariableValues: make(map[string]string), - Errors: make(map[string]error), - DirContents: make(map[string][]DirectoryEntry), - FileContentsRef: make(map[string][]byte), - BranchRefs: make(map[string]string), - Refs: make(map[string]string), + FileContents: make(map[string][]byte), + WorkflowRuns: make(map[string]*WorkflowRun), + Secrets: make(map[string]bool), + VariablesExist: make(map[string]bool), + VariableValues: make(map[string]string), + Errors: make(map[string]error), + DirContents: make(map[string][]DirectoryEntry), + FileContentsRef: make(map[string][]byte), + BranchRefs: make(map[string]string), + Refs: make(map[string]string), + ProtectedBranches: make(map[string]bool), + PipelineSchedules: make(map[string][]PipelineSchedule), } } @@ -52,6 +55,7 @@ type OrgVariableRecord struct { // VariableRecord records a variable creation/update call. type VariableRecord struct { Owner, Repo, Name, Value string + Protected bool } // UpdatedCommentRecord records an issue comment update call. @@ -152,6 +156,12 @@ type FakeClient struct { OrgVariableValues map[string]string // key: "org/name" → value OrgVariableRepoIDs map[string][]int64 // key: "org/name" → repo IDs + // Protected branches for IsProtectedBranch. + ProtectedBranches map[string]bool // key: "owner/repo/branch" + + // Pipeline schedules for List/Create/DeletePipelineSchedule. + PipelineSchedules map[string][]PipelineSchedule // key: "owner/repo" + // Directory listings for ListDirectoryContents. DirContents map[string][]DirectoryEntry // key: "owner/repo/path@ref" @@ -225,6 +235,10 @@ type FakeClient struct { CommittedFilesToBranch []CommitFilesToBranchRecord CreatedForks []string // "owner/repo" DeletedComments []int // comment IDs + CreatedSchedules []PipelineSchedule + DeletedScheduleIDs []int64 + UpdatedVariables []VariableRecord + CreatedProtectedVars []VariableRecord // internal counters proposalCounter int @@ -1677,3 +1691,109 @@ func (f *FakeClient) DeleteOrgVariable(_ context.Context, org, name string) erro delete(f.OrgVariables, org+"/"+name) return nil } + +func (f *FakeClient) IsProtectedBranch(_ context.Context, owner, repo, branch string) (bool, error) { + f.mu.Lock() + defer f.mu.Unlock() + + if e := f.err("IsProtectedBranch"); e != nil { + return false, e + } + + key := owner + "/" + repo + "/" + branch + return f.ProtectedBranches[key], nil +} + +func (f *FakeClient) CreatePipelineSchedule(_ context.Context, owner, repo, ref, description, cron string, _ map[string]string) (int64, error) { + f.mu.Lock() + defer f.mu.Unlock() + + if e := f.err("CreatePipelineSchedule"); e != nil { + return 0, e + } + + s := PipelineSchedule{ + ID: int64(len(f.CreatedSchedules) + 1), + Description: description, + Ref: ref, + Cron: cron, + Active: true, + } + f.CreatedSchedules = append(f.CreatedSchedules, s) + key := owner + "/" + repo + if f.PipelineSchedules == nil { + f.PipelineSchedules = make(map[string][]PipelineSchedule) + } + f.PipelineSchedules[key] = append(f.PipelineSchedules[key], s) + return s.ID, nil +} + +func (f *FakeClient) DeletePipelineSchedule(_ context.Context, owner, repo string, scheduleID int64) error { + f.mu.Lock() + defer f.mu.Unlock() + + if e := f.err("DeletePipelineSchedule"); e != nil { + return e + } + + f.DeletedScheduleIDs = append(f.DeletedScheduleIDs, scheduleID) + key := owner + "/" + repo + if f.PipelineSchedules != nil { + schedules := f.PipelineSchedules[key] + filtered := schedules[:0] + for _, s := range schedules { + if s.ID != scheduleID { + filtered = append(filtered, s) + } + } + f.PipelineSchedules[key] = filtered + } + return nil +} + +func (f *FakeClient) ListPipelineSchedules(_ context.Context, owner, repo string) ([]PipelineSchedule, error) { + f.mu.Lock() + defer f.mu.Unlock() + + if e := f.err("ListPipelineSchedules"); e != nil { + return nil, e + } + + return f.PipelineSchedules[owner+"/"+repo], nil +} + +func (f *FakeClient) UpdateCIVariable(_ context.Context, owner, repo, name, value string, protected bool) error { + f.mu.Lock() + defer f.mu.Unlock() + + if e := f.err("UpdateCIVariable"); e != nil { + return e + } + + f.UpdatedVariables = append(f.UpdatedVariables, VariableRecord{ + Owner: owner, + Repo: repo, + Name: name, + Value: value, + Protected: protected, + }) + return nil +} + +func (f *FakeClient) CreateProtectedCIVariable(_ context.Context, owner, repo, name, value string) error { + f.mu.Lock() + defer f.mu.Unlock() + + if e := f.err("CreateProtectedCIVariable"); e != nil { + return e + } + + f.CreatedProtectedVars = append(f.CreatedProtectedVars, VariableRecord{ + Owner: owner, + Repo: repo, + Name: name, + Value: value, + Protected: true, + }) + return nil +} diff --git a/internal/forge/fake_test.go b/internal/forge/fake_test.go index a18b56adc..d661a4f9c 100644 --- a/internal/forge/fake_test.go +++ b/internal/forge/fake_test.go @@ -3,6 +3,7 @@ package forge import ( "context" "errors" + "fmt" "sync" "testing" @@ -1073,6 +1074,87 @@ func TestFakeClient_AddIssueLabels_Idempotent(t *testing.T) { assert.Equal(t, []string{"ready-for-triage"}, got.Labels) } +func TestIsNotSupported(t *testing.T) { + assert.True(t, IsNotSupported(ErrNotSupported)) + assert.True(t, IsNotSupported(fmt.Errorf("wrap: %w", ErrNotSupported))) + assert.False(t, IsNotSupported(ErrNotFound)) + assert.False(t, IsNotSupported(nil)) +} + +func TestNewFakeClient_MapsInitialized(t *testing.T) { + fc := NewFakeClient() + assert.NotNil(t, fc.ProtectedBranches) + assert.NotNil(t, fc.PipelineSchedules) +} + +func TestFakeClient_PipelineScheduleRoundTrip(t *testing.T) { + ctx := context.Background() + fc := NewFakeClient() + + id, err := fc.CreatePipelineSchedule(ctx, "org", "repo", "main", "nightly", "0 0 * * *", nil) + require.NoError(t, err) + assert.Equal(t, int64(1), id) + + schedules, err := fc.ListPipelineSchedules(ctx, "org", "repo") + require.NoError(t, err) + require.Len(t, schedules, 1) + assert.Equal(t, "nightly", schedules[0].Description) + assert.Equal(t, "main", schedules[0].Ref) + assert.Equal(t, "0 0 * * *", schedules[0].Cron) + assert.True(t, schedules[0].Active) + + err = fc.DeletePipelineSchedule(ctx, "org", "repo", id) + require.NoError(t, err) + + schedules, err = fc.ListPipelineSchedules(ctx, "org", "repo") + require.NoError(t, err) + assert.Empty(t, schedules) + + assert.Equal(t, []int64{id}, fc.DeletedScheduleIDs) +} + +func TestFakeClient_UpdateCIVariable_RecordsProtected(t *testing.T) { + ctx := context.Background() + fc := NewFakeClient() + + err := fc.UpdateCIVariable(ctx, "org", "repo", "KEY", "val", true) + require.NoError(t, err) + require.Len(t, fc.UpdatedVariables, 1) + assert.True(t, fc.UpdatedVariables[0].Protected) + assert.Equal(t, "KEY", fc.UpdatedVariables[0].Name) + + err = fc.UpdateCIVariable(ctx, "org", "repo", "KEY2", "val2", false) + require.NoError(t, err) + require.Len(t, fc.UpdatedVariables, 2) + assert.False(t, fc.UpdatedVariables[1].Protected) +} + +func TestFakeClient_IsProtectedBranch(t *testing.T) { + ctx := context.Background() + fc := NewFakeClient() + fc.ProtectedBranches["org/repo/main"] = true + + protected, err := fc.IsProtectedBranch(ctx, "org", "repo", "main") + require.NoError(t, err) + assert.True(t, protected) + + protected, err = fc.IsProtectedBranch(ctx, "org", "repo", "dev") + require.NoError(t, err) + assert.False(t, protected) +} + +func TestFakeClient_CreateProtectedCIVariable(t *testing.T) { + ctx := context.Background() + fc := NewFakeClient() + + err := fc.CreateProtectedCIVariable(ctx, "org", "repo", "SECRET_KEY", "secret-val") + require.NoError(t, err) + require.Len(t, fc.CreatedProtectedVars, 1) + assert.Equal(t, "SECRET_KEY", fc.CreatedProtectedVars[0].Name) + assert.Equal(t, "secret-val", fc.CreatedProtectedVars[0].Value) + assert.True(t, fc.CreatedProtectedVars[0].Protected) +} + func TestFakeClient_CommitFilesErrSeq(t *testing.T) { ctx := context.Background() files := []TreeFile{{Path: "f.txt", Content: []byte("x"), Mode: "100644"}} diff --git a/internal/forge/forge.go b/internal/forge/forge.go index e9ac3af94..9b7c7796d 100644 --- a/internal/forge/forge.go +++ b/internal/forge/forge.go @@ -78,6 +78,15 @@ func IsNoChanges(err error) bool { return errors.Is(err, ErrNoChanges) } +// ErrNotSupported indicates that the forge implementation does not +// support the requested operation. +var ErrNotSupported = errors.New("operation not supported by this forge") + +// IsNotSupported reports whether err indicates an unsupported operation. +func IsNotSupported(err error) bool { + return errors.Is(err, ErrNotSupported) +} + // Repository represents a repository on a git forge. type Repository struct { ID int64 @@ -477,9 +486,49 @@ type Client interface { // etc.) from all jobs in a workflow run. GetWorkflowRunAnnotations(ctx context.Context, owner, repo string, runID int) ([]Annotation, error) - // App installation operations - ListOrgInstallations(ctx context.Context, org string) ([]Installation, error) + // Branch protection + // IsProtectedBranch returns true if the given branch has protection + // rules enabled. Returns ErrNotSupported if the forge does not + // expose branch-protection queries. + IsProtectedBranch(ctx context.Context, owner, repo, branch string) (bool, error) + + // Pipeline schedules and branch-restricted CI variables live on + // the base Client because both GitHub Actions and GitLab CI support + // timed triggers. However, the branch-restricted/protected variable + // semantics and pipeline schedule APIs have no GitHub Actions + // analogue, so GitHub stubs return ErrNotSupported. + // GitHubExtensions is reserved for operations with no cross-forge + // analogue (App installations, OAuth client IDs). + // The existing RepoVariable methods model GitHub Actions variables; + // the CIVariable methods below model GitLab CI protected variables + // (branch-restricted, unmasked). + CreatePipelineSchedule(ctx context.Context, owner, repo, ref, description, cron string, variables map[string]string) (int64, error) + DeletePipelineSchedule(ctx context.Context, owner, repo string, scheduleID int64) error + ListPipelineSchedules(ctx context.Context, owner, repo string) ([]PipelineSchedule, error) + + // CI/CD branch-restricted variables (distinct from RepoVariable methods). + UpdateCIVariable(ctx context.Context, owner, repo, name, value string, protected bool) error + // CreateProtectedCIVariable creates a branch-restricted, unmasked CI/CD variable. + // Values are visible in pipeline logs; use CreateRepoSecret for credentials. + CreateProtectedCIVariable(ctx context.Context, owner, repo, name, value string) error +} + +// PipelineSchedule represents a scheduled pipeline trigger. +type PipelineSchedule struct { + ID int64 + Description string + Ref string + Cron string + CronTimezone string + Active bool +} - // GetAppClientID returns the Client ID for a GitHub App identified by slug. +// GitHubExtensions provides GitHub-specific operations that are not +// part of the cross-forge Client interface. Callers should type-assert +// to this interface when they need GitHub App installation features. +type GitHubExtensions interface { + // ListOrgInstallations returns all GitHub App installations for the org. + ListOrgInstallations(ctx context.Context, org string) ([]Installation, error) + // GetAppClientID returns the OAuth client ID for the named GitHub App. GetAppClientID(ctx context.Context, slug string) (string, error) } diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index d2a3f0c70..12acb7654 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -31,8 +31,9 @@ type LiveClient struct { baseURL string } -// Compile-time interface check. +// Compile-time interface checks. var _ forge.Client = (*LiveClient)(nil) +var _ forge.GitHubExtensions = (*LiveClient)(nil) // New creates a new GitHub client with the given personal access token. func New(token string) *LiveClient { @@ -3290,6 +3291,53 @@ func (c *LiveClient) GetOrgVariableRepos(ctx context.Context, org, name string) return ids, nil } +// IsProtectedBranch checks whether the given branch has protection rules +// enabled on GitHub by querying the branch protection API endpoint. +// GitHub returns 404 both when a branch exists but is not protected and +// when the branch/repo does not exist. We distinguish the two by +// inspecting the API error message: "Branch not protected" means the +// branch exists but has no protection rules. +func (c *LiveClient) IsProtectedBranch(ctx context.Context, owner, repo, branch string) (bool, error) { + resp, err := c.get(ctx, fmt.Sprintf("/repos/%s/%s/branches/%s/protection", url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(branch))) + if err != nil { + var apiErr *APIError + if errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusNotFound { + if strings.EqualFold(apiErr.Message, "Branch not protected") { + return false, nil + } + return false, fmt.Errorf("check branch protection: %w", err) + } + return false, fmt.Errorf("check branch protection: %w", err) + } + resp.Body.Close() + return true, nil +} + +// CreatePipelineSchedule is not supported on GitHub. +func (c *LiveClient) CreatePipelineSchedule(_ context.Context, owner, repo, ref, description, cron string, _ map[string]string) (int64, error) { + return 0, forge.ErrNotSupported +} + +// DeletePipelineSchedule is not supported on GitHub. +func (c *LiveClient) DeletePipelineSchedule(_ context.Context, owner, repo string, scheduleID int64) error { + return forge.ErrNotSupported +} + +// ListPipelineSchedules is not supported on GitHub. +func (c *LiveClient) ListPipelineSchedules(_ context.Context, owner, repo string) ([]forge.PipelineSchedule, error) { + return nil, forge.ErrNotSupported +} + +// UpdateCIVariable is not supported on GitHub. +func (c *LiveClient) UpdateCIVariable(_ context.Context, _, _, _, _ string, _ bool) error { + return forge.ErrNotSupported +} + +// CreateProtectedCIVariable is not supported on GitHub. +func (c *LiveClient) CreateProtectedCIVariable(_ context.Context, _, _, _, _ string) error { + return forge.ErrNotSupported +} + // isNotFound checks whether an error is a 404 API error. func isNotFound(err error) bool { var apiErr *APIError diff --git a/internal/forge/github/github_test.go b/internal/forge/github/github_test.go index c0fc3bc8b..0a8b14c44 100644 --- a/internal/forge/github/github_test.go +++ b/internal/forge/github/github_test.go @@ -3220,3 +3220,87 @@ func TestDeleteRepoSecret(t *testing.T) { assert.Contains(t, err.Error(), "unexpected status") }) } + +func TestIsProtectedBranch(t *testing.T) { + t.Run("protected", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "GET", r.Method) + assert.Equal(t, "/repos/owner/repo/branches/main/protection", r.URL.Path) + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, `{"url":"https://api.github.com/repos/owner/repo/branches/main/protection"}`) + })) + defer srv.Close() + + client := newTestClient(t, srv) + protected, err := client.IsProtectedBranch(context.Background(), "owner", "repo", "main") + require.NoError(t, err) + assert.True(t, protected) + }) + + t.Run("not protected", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + fmt.Fprint(w, `{"message":"Branch not protected"}`) + })) + defer srv.Close() + + client := newTestClient(t, srv) + protected, err := client.IsProtectedBranch(context.Background(), "owner", "repo", "dev") + require.NoError(t, err) + assert.False(t, protected) + }) +} + +func TestIsProtectedBranch_NotFound(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + fmt.Fprint(w, `{"message":"Not Found"}`) + })) + defer srv.Close() + + client := newTestClient(t, srv) + _, err := client.IsProtectedBranch(context.Background(), "owner", "repo", "nonexistent") + require.Error(t, err) + assert.Contains(t, err.Error(), "check branch protection") +} + +func TestIsProtectedBranch_SlashInBranch(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "GET", r.Method) + assert.Equal(t, "/repos/owner/repo/branches/release%2F1.2/protection", r.URL.RawPath) + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, `{"url":"https://api.github.com/repos/owner/repo/branches/release%2F1.2/protection"}`) + })) + defer srv.Close() + + client := newTestClient(t, srv) + protected, err := client.IsProtectedBranch(context.Background(), "owner", "repo", "release/1.2") + require.NoError(t, err) + assert.True(t, protected) +} + +func TestUnsupportedMethods(t *testing.T) { + client := New("test-token") + ctx := context.Background() + + t.Run("CreatePipelineSchedule", func(t *testing.T) { + _, err := client.CreatePipelineSchedule(ctx, "o", "r", "main", "desc", "0 * * * *", nil) + assert.ErrorIs(t, err, forge.ErrNotSupported) + }) + t.Run("DeletePipelineSchedule", func(t *testing.T) { + err := client.DeletePipelineSchedule(ctx, "o", "r", 1) + assert.ErrorIs(t, err, forge.ErrNotSupported) + }) + t.Run("ListPipelineSchedules", func(t *testing.T) { + _, err := client.ListPipelineSchedules(ctx, "o", "r") + assert.ErrorIs(t, err, forge.ErrNotSupported) + }) + t.Run("UpdateCIVariable", func(t *testing.T) { + err := client.UpdateCIVariable(ctx, "o", "r", "KEY", "val", false) + assert.ErrorIs(t, err, forge.ErrNotSupported) + }) + t.Run("CreateProtectedCIVariable", func(t *testing.T) { + err := client.CreateProtectedCIVariable(ctx, "o", "r", "KEY", "val") + assert.ErrorIs(t, err, forge.ErrNotSupported) + }) +} diff --git a/internal/forge/url.go b/internal/forge/url.go index 3dd4131c8..b718a8b13 100644 --- a/internal/forge/url.go +++ b/internal/forge/url.go @@ -26,6 +26,11 @@ type ForgeURLInfo struct { // // https://github.com/{owner}/{repo}/tree/{ref}/{path} (directory) // https://github.com/{owner}/{repo}/blob/{ref}/{path} (file) +// +// Accepted GitLab formats (supports nested groups): +// +// https://gitlab.com/{group}[/subgroup...]/{repo}/-/tree/{ref}/{path} +// https://gitlab.com/{group}[/subgroup...]/{repo}/-/blob/{ref}/{path} func ParseForgeURL(rawURL string) (*ForgeURLInfo, error) { // Strip fragment (including #sha256=... integrity hashes) before parsing. if idx := strings.LastIndex(rawURL, "#"); idx != -1 { @@ -41,10 +46,13 @@ func ParseForgeURL(rawURL string) (*ForgeURLInfo, error) { } hostname := u.Hostname() - if !IsSupportedForge(hostname) { + if !isRecognizedForge(hostname) { return nil, fmt.Errorf("unsupported forge host %q", hostname) } + // Determine forge name from hostname. + forgeName := hostnameToForge(hostname) + // Split the path into segments, filtering out empty strings from leading/trailing slashes. var segments []string for _, s := range strings.Split(u.Path, "/") { @@ -53,6 +61,11 @@ func ParseForgeURL(rawURL string) (*ForgeURLInfo, error) { } } + if forgeName == "gitlab" { + return parseGitLabURL(segments) + } + + // GitHub format: /{owner}/{repo}/{tree|blob}/{ref}/{path...} // Need at least 4 segments: owner, repo, type (tree/blob), ref. if len(segments) < 4 { return nil, fmt.Errorf("URL path too short: need at least /{owner}/{repo}/{tree|blob}/{ref}") @@ -84,7 +97,7 @@ func ParseForgeURL(rawURL string) (*ForgeURLInfo, error) { } return &ForgeURLInfo{ - Forge: "github", + Forge: forgeName, Owner: owner, Repo: repo, Path: repoPath, @@ -92,6 +105,70 @@ func ParseForgeURL(rawURL string) (*ForgeURLInfo, error) { }, nil } +// parseGitLabURL parses a GitLab URL path (segments after the host). +// GitLab uses "/-/" as a separator between the project path and +// resource type. Nested groups are supported (e.g., group/subgroup/repo). +// +// Format: {group}[/subgroup...]/{repo}/-/{tree|blob}/{ref}/{path...} +func parseGitLabURL(segments []string) (*ForgeURLInfo, error) { + // Find the "/-/" separator (represented as "-" in segments). + dashIdx := -1 + for i, s := range segments { + if s == "-" { + dashIdx = i + break + } + } + + if dashIdx < 0 { + return nil, fmt.Errorf("URL path too short: need at least /{group}/{repo}/-/{tree|blob}/{ref}") + } + + // Need at least 2 segments before dash (group + repo) and 2 after (type + ref). + if dashIdx < 2 { + return nil, fmt.Errorf("URL path too short: need at least /{group}/{repo}/-/{tree|blob}/{ref}") + } + if len(segments) < dashIdx+3 { + return nil, fmt.Errorf("URL path too short: need at least /{group}/{repo}/-/{tree|blob}/{ref}") + } + + // Everything before dash except the last segment is the owner (group/subgroups). + // The last segment before dash is the repo. + owner := strings.Join(segments[:dashIdx-1], "/") + repo := segments[dashIdx-1] + pathType := segments[dashIdx+1] + ref := segments[dashIdx+2] + + if pathType != "tree" && pathType != "blob" { + return nil, fmt.Errorf("unsupported path type %q: expected \"tree\" or \"blob\"", pathType) + } + + var repoPath string + if len(segments) > dashIdx+3 { + repoPath = strings.Join(segments[dashIdx+3:], "/") + } + + return &ForgeURLInfo{ + Forge: "gitlab", + Owner: owner, + Repo: repo, + Path: repoPath, + Ref: ref, + }, nil +} + +// hostnameToForge maps a forge hostname to its short name. +func hostnameToForge(hostname string) string { + switch hostname { + case "github.com": + return "github" + case "gitlab.com": + return "gitlab" + default: + return hostname + } +} + // ParseRawContentURL extracts forge, owner, repo, path, and ref from a // raw.githubusercontent.com URL. // @@ -173,7 +250,16 @@ func forgeHost(forge string) string { } } -// IsSupportedForge returns true if the hostname belongs to a recognized forge. +// IsSupportedForge returns true if the hostname belongs to a forge with +// full fetch/clone support. Use this for validating user-facing URLs +// (e.g., skill references) where the system must actually be able to +// retrieve content. func IsSupportedForge(hostname string) bool { return hostname == "github.com" } + +// isRecognizedForge returns true for any forge whose URL format we can parse, +// even if fetch support has not landed yet. +func isRecognizedForge(hostname string) bool { + return hostname == "github.com" || hostname == "gitlab.com" +} diff --git a/internal/forge/url_test.go b/internal/forge/url_test.go index 65b71291e..306aef8a8 100644 --- a/internal/forge/url_test.go +++ b/internal/forge/url_test.go @@ -100,6 +100,55 @@ func TestParseForgeURL(t *testing.T) { Path: "skills/foo", }, }, + { + name: "GitLab tree URL", + input: "https://gitlab.com/my-group/my-repo/-/tree/main/src/lib", + want: &ForgeURLInfo{ + Forge: "gitlab", + Owner: "my-group", + Repo: "my-repo", + Ref: "main", + Path: "src/lib", + }, + }, + { + name: "GitLab blob URL", + input: "https://gitlab.com/my-group/my-repo/-/blob/abc123/README.md", + want: &ForgeURLInfo{ + Forge: "gitlab", + Owner: "my-group", + Repo: "my-repo", + Ref: "abc123", + Path: "README.md", + }, + }, + { + name: "GitLab nested groups", + input: "https://gitlab.com/top/sub/deep/my-repo/-/tree/v1.0/docs", + want: &ForgeURLInfo{ + Forge: "gitlab", + Owner: "top/sub/deep", + Repo: "my-repo", + Ref: "v1.0", + Path: "docs", + }, + }, + { + name: "GitLab root path no trailing path", + input: "https://gitlab.com/group/repo/-/tree/main", + want: &ForgeURLInfo{ + Forge: "gitlab", + Owner: "group", + Repo: "repo", + Ref: "main", + Path: "", + }, + }, + { + name: "GitLab URL missing dash separator", + input: "https://gitlab.com/group/repo", + wantErr: "URL path too short", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -237,7 +286,7 @@ func TestIsSupportedForge(t *testing.T) { want bool }{ {"github.com", "github.com", true}, - {"gitlab.com not yet supported", "gitlab.com", false}, + {"gitlab.com — recognized but no fetch support yet", "gitlab.com", false}, {"example.com", "example.com", false}, {"empty string", "", false}, } diff --git a/internal/harness/harness.go b/internal/harness/harness.go index dbca839ff..6c6d0cd51 100644 --- a/internal/harness/harness.go +++ b/internal/harness/harness.go @@ -806,9 +806,13 @@ func (h *Harness) ValidateResourceTypes() error { return fmt.Errorf("skills[%d] URL must include #sha256=... integrity hash", i) } cleanURL, _, _ := ParseIntegrityHash(s) - if _, err := forge.ParseForgeURL(cleanURL); err != nil { + info, err := forge.ParseForgeURL(cleanURL) + if err != nil { return fmt.Errorf("skills[%d] URL must be hosted on a supported forge (github.com): %w", i, err) } + if info.Forge != "github" { + return fmt.Errorf("skills[%d] forge %q is recognized but fetch support has not landed yet", i, info.Forge) + } } } diff --git a/internal/resolve/resolve.go b/internal/resolve/resolve.go index 9393d79ea..353b2cdca 100644 --- a/internal/resolve/resolve.go +++ b/internal/resolve/resolve.go @@ -314,6 +314,9 @@ func resolveSkillDirURL(ctx context.Context, field, rawURL string, h *harness.Ha if err != nil { return Dependency{}, "", fmt.Errorf("%s: skill URLs must be hosted on a supported forge: %w", field, err) } + if forgeInfo.Forge != "github" { + return Dependency{}, "", fmt.Errorf("%s: forge %q is recognized but fetch support has not landed yet", field, forgeInfo.Forge) + } treePath, dirEntry, err := fetch.CacheGetDir(opts.WorkspaceRoot, expectedHash) if err != nil { diff --git a/internal/resolve/resolve_test.go b/internal/resolve/resolve_test.go index 8373813d0..519bd9bb9 100644 --- a/internal/resolve/resolve_test.go +++ b/internal/resolve/resolve_test.go @@ -294,6 +294,21 @@ func TestResolveHarness_SkillNonForgeURLRejected(t *testing.T) { assert.Contains(t, err.Error(), "supported forge") } +func TestResolveHarness_GitLabURLRejected(t *testing.T) { + fakeHash := strings.Repeat("a", 64) + h := &harness.Harness{ + Skills: []string{fmt.Sprintf("https://gitlab.com/org/repo/-/tree/main/skills/review#sha256=%s", fakeHash)}, + AllowedRemoteResources: []string{"https://gitlab.com/org/repo/"}, + } + + _, err := ResolveHarness(context.Background(), h, ResolveOpts{ + WorkspaceRoot: t.TempDir(), + FetchPolicy: fetch.FetchPolicy{}, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "fetch support has not landed yet") +} + func TestResolveHarness_DiamondDependency(t *testing.T) { reg := newSkillRegistry()