diff --git a/apps/backend/internal/backendapp/adapters_test.go b/apps/backend/internal/backendapp/adapters_test.go index 7b48559b32..066dd7efee 100644 --- a/apps/backend/internal/backendapp/adapters_test.go +++ b/apps/backend/internal/backendapp/adapters_test.go @@ -535,6 +535,67 @@ func TestResolveForReviewRedetectsStoredMasterAfterClonePath(t *testing.T) { } } +func TestRepositoryResolverAdapterUsesCurrentGitProtocol(t *testing.T) { + harness := newBootStateTestHarness(t) + ctx := context.Background() + workspace, err := harness.taskSvc.CreateWorkspace(ctx, &taskservice.CreateWorkspaceRequest{ + Name: "Workspace", + }) + if err != nil { + t.Fatalf("CreateWorkspace: %v", err) + } + paths := []string{filepath.Join(t.TempDir(), "one"), filepath.Join(t.TempDir(), "two")} + for _, path := range paths { + runGit(t, "", "init", "--quiet", path) + } + cloner := &recordingReviewRepositoryCloner{protocol: repoclone.ProtocolSSH, paths: paths} + adapter := &repositoryResolverAdapter{ + cloner: cloner, + taskSvc: harness.taskSvc, + logger: newTestLogger(), + } + + if _, _, err := adapter.ResolveForReview(ctx, workspace.ID, "github", "owner", "one", ""); err != nil { + t.Fatalf("first ResolveForReview: %v", err) + } + cloner.protocol = repoclone.ProtocolHTTPS + if _, _, err := adapter.ResolveForReview(ctx, workspace.ID, "github", "owner", "two", ""); err != nil { + t.Fatalf("second ResolveForReview: %v", err) + } + + want := []string{ + "git@github.com:owner/one.git", + "https://github.com/owner/two.git", + } + if !reflect.DeepEqual(cloner.cloneURLs, want) { + t.Fatalf("clone URLs = %v, want %v", cloner.cloneURLs, want) + } +} + +type recordingReviewRepositoryCloner struct { + protocol string + paths []string + cloneURLs []string +} + +func (c *recordingReviewRepositoryCloner) EnsureWorkspaceCloned( + _ context.Context, _, _, _, _, name string, +) (string, error) { + path := c.paths[0] + c.paths = c.paths[1:] + return path, nil +} + +func (c *recordingReviewRepositoryCloner) BuildCloneURLWithHost( + _ context.Context, provider, host, owner, name string, +) (string, error) { + cloneURL, err := repoclone.CloneURLWithHost(provider, host, owner, name, c.protocol) + if err == nil { + c.cloneURLs = append(c.cloneURLs, cloneURL) + } + return cloneURL, err +} + type staticRepoCloneCredential struct{} func (staticRepoCloneCredential) ResolveGitCredential( diff --git a/apps/backend/internal/backendapp/main.go b/apps/backend/internal/backendapp/main.go index f76647a30b..62c2355a03 100644 --- a/apps/backend/internal/backendapp/main.go +++ b/apps/backend/internal/backendapp/main.go @@ -580,9 +580,9 @@ func startAgentInfrastructure( // ============================================ // REPO CLONER // ============================================ - repoCloner := repoclone.NewCloner(repoclone.Config{ + repoCloner := repoclone.NewClonerWithProtocolResolver(repoclone.Config{ BasePath: cfg.RepoClone.BasePath, - }, repoclone.DetectGitProtocol(), cfg.ResolvedHomeDir(), log) + }, repoclone.NewGitProtocolResolver(), cfg.ResolvedHomeDir(), log) if services.GitHub != nil || services.Plugins != nil { repoCloner.SetGitCredentialProvider( newRepositoryCloneCredentialProvider(services.GitHub, services.Plugins), diff --git a/apps/backend/internal/backendapp/orchestrator.go b/apps/backend/internal/backendapp/orchestrator.go index a12d6d1987..8fdf03f08e 100644 --- a/apps/backend/internal/backendapp/orchestrator.go +++ b/apps/backend/internal/backendapp/orchestrator.go @@ -231,10 +231,9 @@ func provideOrchestrator( // Wire repository resolver for auto-cloning repos during review task creation if repoCloner != nil { orchestratorSvc.SetRepositoryResolver(&repositoryResolverAdapter{ - cloner: repoCloner, - protocol: repoclone.DetectGitProtocol(), - taskSvc: taskSvc, - logger: log, + cloner: repoCloner, + taskSvc: taskSvc, + logger: log, }) // Wire repo cloner into executor for provider-backed repos with no local path @@ -971,10 +970,14 @@ func (u *repoLocalPathUpdater) UpdateTaskRepositoryBaseBranch(ctx context.Contex // repositoryResolverAdapter resolves GitHub repos by cloning + finding/creating DB records. type repositoryResolverAdapter struct { - cloner *repoclone.Cloner - protocol string - taskSvc *taskservice.Service - logger *logger.Logger + cloner reviewRepositoryCloner + taskSvc *taskservice.Service + logger *logger.Logger +} + +type reviewRepositoryCloner interface { + EnsureWorkspaceCloned(context.Context, string, string, string, string, string) (string, error) + BuildCloneURLWithHost(context.Context, string, string, string, string) (string, error) } // ResolveForReview implements orchestrator.RepositoryResolver. @@ -1000,7 +1003,7 @@ func (a *repositoryResolverAdapter) ResolveForReview( return existing.ID, baseBranch, nil } - cloneURL, err := repoclone.CloneURL(provider, owner, name, a.protocol) + cloneURL, err := a.cloner.BuildCloneURLWithHost(ctx, provider, providerHost, owner, name) if err != nil { return "", "", fmt.Errorf("unsupported provider: %w", err) } diff --git a/apps/backend/internal/orchestrator/executor/executor.go b/apps/backend/internal/orchestrator/executor/executor.go index b8a9a17072..92e4721950 100644 --- a/apps/backend/internal/orchestrator/executor/executor.go +++ b/apps/backend/internal/orchestrator/executor/executor.go @@ -876,7 +876,7 @@ type RepoCloner interface { // SetOriginURL updates a Kandev-managed checkout remote without exposing credentials. SetOriginURL(ctx context.Context, repositoryPath, originURL string) error // BuildCloneURL constructs a protocol-aware clone URL for the given provider/owner/name. - BuildCloneURLWithHost(provider, host, owner, name string) (string, error) + BuildCloneURLWithHost(ctx context.Context, provider, host, owner, name string) (string, error) } type authenticatedRepoCloner interface { diff --git a/apps/backend/internal/orchestrator/executor/executor_resume.go b/apps/backend/internal/orchestrator/executor/executor_resume.go index 12e0c96615..0341f15e65 100644 --- a/apps/backend/internal/orchestrator/executor/executor_resume.go +++ b/apps/backend/internal/orchestrator/executor/executor_resume.go @@ -304,7 +304,7 @@ func (e *Executor) refreshManagedRepositoryForSession( if cloneURL == "" { var err error cloneURL, err = e.repoCloner.BuildCloneURLWithHost( - repo.Provider, repo.ProviderHost, repo.ProviderOwner, repo.ProviderName, + ctx, repo.Provider, repo.ProviderHost, repo.ProviderOwner, repo.ProviderName, ) if err != nil || cloneURL == "" { return ErrNoCloneURL @@ -416,7 +416,7 @@ func (e *Executor) ensureRepoClonedForSession( if cloneURL == "" { var urlErr error cloneURL, urlErr = e.repoCloner.BuildCloneURLWithHost( - repo.Provider, repo.ProviderHost, repo.ProviderOwner, repo.ProviderName, + ctx, repo.Provider, repo.ProviderHost, repo.ProviderOwner, repo.ProviderName, ) if urlErr != nil || cloneURL == "" { return "", ErrNoCloneURL @@ -474,7 +474,7 @@ func (e *Executor) reconcileGitHubCheckoutOrigin( } policy = resolved } - originURL, err := gitHubCheckoutOriginURL(repo, policy, e.repoCloner) + originURL, err := gitHubCheckoutOriginURL(ctx, repo, policy, e.repoCloner) if err != nil { return err } @@ -494,11 +494,11 @@ func isGitHubRepository(repo *models.Repository) bool { } func gitHubCheckoutOriginURL( - repo *models.Repository, policy TaskGitCredentialPolicy, cloner RepoCloner, + ctx context.Context, repo *models.Repository, policy TaskGitCredentialPolicy, cloner RepoCloner, ) (string, error) { if policy.Mode == taskGitCredentialsModeExecutor { originURL, err := cloner.BuildCloneURLWithHost( - repo.Provider, repo.ProviderHost, repo.ProviderOwner, repo.ProviderName, + ctx, repo.Provider, repo.ProviderHost, repo.ProviderOwner, repo.ProviderName, ) if err != nil { return "", fmt.Errorf("build executor GitHub checkout origin: %w", err) diff --git a/apps/backend/internal/orchestrator/executor/executor_resume_clone_default_branch_test.go b/apps/backend/internal/orchestrator/executor/executor_resume_clone_default_branch_test.go index 4d403124d4..0daac5e10b 100644 --- a/apps/backend/internal/orchestrator/executor/executor_resume_clone_default_branch_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_resume_clone_default_branch_test.go @@ -408,7 +408,7 @@ func (f *fakeRepoCloner) ShouldRecloneForWorkspace(_, _ string) bool { return fa func (f *fakeRepoCloner) SetOriginURL(context.Context, string, string) error { return nil } -func (f *fakeRepoCloner) BuildCloneURLWithHost(_, _, owner, name string) (string, error) { +func (f *fakeRepoCloner) BuildCloneURLWithHost(_ context.Context, _, _, owner, name string) (string, error) { return "https://github.com/" + owner + "/" + name + ".git", nil } diff --git a/apps/backend/internal/orchestrator/executor/executor_resume_clone_transport_test.go b/apps/backend/internal/orchestrator/executor/executor_resume_clone_transport_test.go index 404b251d52..ac4deeee25 100644 --- a/apps/backend/internal/orchestrator/executor/executor_resume_clone_transport_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_resume_clone_transport_test.go @@ -65,6 +65,46 @@ func TestEnsureRepoLocalPath_ReconcilesGitHubOriginForCredentialPolicy(t *testin } } +func TestEnsureRepoLocalPathReevaluatesGitHubProtocol(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + + repoPath := initGitRepoWithOrigin(t, "https://github.com/acme/widgets.git") + resolver := &mutableExecutorGitProtocolResolver{protocol: repoclone.ProtocolSSH} + cloner := repoclone.NewClonerWithProtocolResolver( + repoclone.Config{BasePath: t.TempDir()}, resolver, "", nil, + ) + executor := newTestExecutor(t, &mockAgentManager{}, newMockRepository()) + executor.SetTaskGitCredentialPolicyResolver(fakeTaskGitCredentialPolicyResolver{ + policy: TaskGitCredentialPolicy{Mode: taskGitCredentialsModeExecutor}, + }) + executor.SetRepoCloner(cloner, nil) + repository := &models.Repository{ + WorkspaceID: "workspace-1", + SourceType: "provider", + Provider: "github", + ProviderOwner: "acme", + ProviderName: "widgets", + LocalPath: repoPath, + } + + if err := executor.ensureRepoLocalPath(context.Background(), repository); err != nil { + t.Fatalf("first ensureRepoLocalPath() error = %v", err) + } + if got := gitOriginURL(t, repoPath); got != "git@github.com:acme/widgets.git" { + t.Fatalf("first origin = %q, want SSH URL", got) + } + + resolver.protocol = repoclone.ProtocolHTTPS + if err := executor.ensureRepoLocalPath(context.Background(), repository); err != nil { + t.Fatalf("second ensureRepoLocalPath() error = %v", err) + } + if got := gitOriginURL(t, repoPath); got != "https://github.com/acme/widgets.git" { + t.Fatalf("second origin = %q, want HTTPS URL", got) + } +} + func TestEnsureRepoLocalPath_DoesNotRewriteUserManagedOrigin(t *testing.T) { if _, err := exec.LookPath("git"); err != nil { t.Skip("git not available") @@ -507,6 +547,14 @@ type cloneTransportTestCloner struct { setOriginPaths []string } +type mutableExecutorGitProtocolResolver struct { + protocol string +} + +func (r *mutableExecutorGitProtocolResolver) ResolveGitProtocol(context.Context, string) string { + return r.protocol +} + var _ authenticatedRepoCloner = (*cloneTransportTestCloner)(nil) func (c *cloneTransportTestCloner) EnsureWorkspaceClonedWithCredentialRequest( @@ -557,7 +605,7 @@ func (c *cloneTransportTestCloner) SetOriginURL(ctx context.Context, repositoryP return nil } -func (c *cloneTransportTestCloner) BuildCloneURLWithHost(string, string, string, string) (string, error) { +func (c *cloneTransportTestCloner) BuildCloneURLWithHost(context.Context, string, string, string, string) (string, error) { return c.cloneURL, nil } diff --git a/apps/backend/internal/orchestrator/executor/executor_test.go b/apps/backend/internal/orchestrator/executor/executor_test.go index 88a954451a..8140849122 100644 --- a/apps/backend/internal/orchestrator/executor/executor_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_test.go @@ -2948,7 +2948,7 @@ func (c *recordingAuthenticatedCloner) SetOriginURL(context.Context, string, str return nil } -func (c *recordingAuthenticatedCloner) BuildCloneURLWithHost(_, _, _, _ string) (string, error) { +func (c *recordingAuthenticatedCloner) BuildCloneURLWithHost(_ context.Context, _, _, _, _ string) (string, error) { return "", nil } diff --git a/apps/backend/internal/repoclone/clone.go b/apps/backend/internal/repoclone/clone.go index e6608eccac..8f3cf6709f 100644 --- a/apps/backend/internal/repoclone/clone.go +++ b/apps/backend/internal/repoclone/clone.go @@ -55,11 +55,11 @@ type Config struct { // Cloner handles git clone and fetch operations. type Cloner struct { - config Config - protocol string - logger *logger.Logger - credentials GitCredentialProvider - repoMus sync.Map + config Config + protocolResolver GitProtocolResolver + logger *logger.Logger + credentials GitCredentialProvider + repoMus sync.Map } // GitCredentialProvider resolves the workspace automation identity selected @@ -88,12 +88,33 @@ type GitCredentialRequest struct { PRNumber int } +type staticGitProtocolResolver string + +func (r staticGitProtocolResolver) ResolveGitProtocol(context.Context, string) string { + return string(r) +} + // NewCloner creates a new Cloner with the given configuration. func NewCloner(cfg Config, protocol string, dataDir string, log *logger.Logger) *Cloner { + return newCloner(cfg, staticGitProtocolResolver(protocol), dataDir, log) +} + +// NewClonerWithProtocolResolver creates a Cloner that resolves Git protocols +// when protocol-aware clone URLs are built. +func NewClonerWithProtocolResolver( + cfg Config, resolver GitProtocolResolver, dataDir string, log *logger.Logger, +) *Cloner { + if resolver == nil { + resolver = NewGitProtocolResolver() + } + return newCloner(cfg, resolver, dataDir, log) +} + +func newCloner(cfg Config, resolver GitProtocolResolver, dataDir string, log *logger.Logger) *Cloner { if cfg.BasePath == "" && dataDir != "" { cfg.BasePath = filepath.Join(dataDir, "repos") } - return &Cloner{config: cfg, protocol: protocol, logger: log} + return &Cloner{config: cfg, protocolResolver: resolver, logger: log} } // SetGitCredentialProvider configures workspace-scoped Git transport auth. @@ -120,13 +141,40 @@ func (c *Cloner) ExpandedBasePath() (string, error) { } // BuildCloneURL constructs a protocol-aware clone URL for a provider repository. -func (c *Cloner) BuildCloneURL(provider, owner, name string) (string, error) { - return CloneURL(provider, owner, name, c.protocol) +func (c *Cloner) BuildCloneURL(ctx context.Context, provider, owner, name string) (string, error) { + host, err := providerHost(provider) + if err != nil { + return "", err + } + return CloneURL(provider, owner, name, c.resolveProtocol(ctx, host)) } // BuildCloneURLWithHost constructs a clone URL using a persisted provider origin. -func (c *Cloner) BuildCloneURLWithHost(provider, host, owner, name string) (string, error) { - return CloneURLWithHost(provider, host, owner, name, c.protocol) +func (c *Cloner) BuildCloneURLWithHost( + ctx context.Context, provider, host, owner, name string, +) (string, error) { + resolvedHost, _, err := normalizeGitProviderHost(host) + if err != nil { + return "", err + } + if resolvedHost == "" { + resolvedHost, err = providerHost(provider) + if err != nil { + return "", err + } + } + return CloneURLWithHost(provider, host, owner, name, c.resolveProtocol(ctx, resolvedHost)) +} + +func (c *Cloner) resolveProtocol(ctx context.Context, host string) string { + if c.protocolResolver == nil { + return ProtocolSSH + } + protocol := strings.ToLower(strings.TrimSpace(c.protocolResolver.ResolveGitProtocol(ctx, host))) + if protocol == ProtocolHTTPS { + return ProtocolHTTPS + } + return ProtocolSSH } // RepoPath returns the legacy owner/name clone path. diff --git a/apps/backend/internal/repoclone/clone_test.go b/apps/backend/internal/repoclone/clone_test.go index 883a8f1b59..349958cf90 100644 --- a/apps/backend/internal/repoclone/clone_test.go +++ b/apps/backend/internal/repoclone/clone_test.go @@ -43,6 +43,42 @@ func TestClonerExposesExactScopeWorkspaceClone(t *testing.T) { } } +func TestClonerBuildCloneURLUsesCurrentProtocol(t *testing.T) { + resolver := &mutableGitProtocolResolver{protocol: ProtocolHTTPS} + cloner := NewClonerWithProtocolResolver( + Config{BasePath: t.TempDir()}, resolver, "", logger.Default(), + ) + + got, err := cloner.BuildCloneURLWithHost( + context.Background(), "github", "https://github.com", "acme", "widgets", + ) + if err != nil { + t.Fatalf("BuildCloneURLWithHost() initial error = %v", err) + } + if got != "https://github.com/acme/widgets.git" { + t.Fatalf("initial clone URL = %q, want HTTPS", got) + } + + resolver.protocol = ProtocolSSH + got, err = cloner.BuildCloneURLWithHost( + context.Background(), "github", "https://github.com", "acme", "widgets", + ) + if err != nil { + t.Fatalf("BuildCloneURLWithHost() updated error = %v", err) + } + if got != "git@github.com:acme/widgets.git" { + t.Fatalf("updated clone URL = %q, want SSH", got) + } +} + +type mutableGitProtocolResolver struct { + protocol string +} + +func (r *mutableGitProtocolResolver) ResolveGitProtocol(context.Context, string) string { + return r.protocol +} + func TestProviderRepoPathSeparatesProviderHosts(t *testing.T) { t.Parallel() diff --git a/apps/backend/internal/repoclone/protocol.go b/apps/backend/internal/repoclone/protocol.go index e37ccfd34f..a032d9f380 100644 --- a/apps/backend/internal/repoclone/protocol.go +++ b/apps/backend/internal/repoclone/protocol.go @@ -17,21 +17,64 @@ const ProtocolSSH = "ssh" // ProtocolHTTPS is the HTTPS git protocol. const ProtocolHTTPS = "https" -// DetectGitProtocol returns the user's preferred git clone protocol. -// It checks the gh CLI config (`gh config get git_protocol`). If gh reports -// "https", it returns ProtocolHTTPS. Otherwise it defaults to ProtocolSSH. -func DetectGitProtocol() string { - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) +const gitProtocolLookupTimeout = 5 * time.Second + +// GitProtocolResolver resolves the clone protocol for a provider host. +type GitProtocolResolver interface { + ResolveGitProtocol(context.Context, string) string +} + +type gitProtocolCommandRunner func(context.Context, ...string) ([]byte, error) + +type gitProtocolResolver struct { + run gitProtocolCommandRunner +} + +// NewGitProtocolResolver creates a resolver backed by the host gh CLI. +func NewGitProtocolResolver() GitProtocolResolver { + return &gitProtocolResolver{run: runGHConfigCommand} +} + +func newGitProtocolResolver(run gitProtocolCommandRunner) GitProtocolResolver { + if run == nil { + run = runGHConfigCommand + } + return &gitProtocolResolver{run: run} +} + +func runGHConfigCommand(ctx context.Context, args ...string) ([]byte, error) { + return subproc.RunGHOutput(ctx, exec.CommandContext(ctx, "gh", args...)) +} + +func (r *gitProtocolResolver) ResolveGitProtocol(ctx context.Context, providerHost string) string { + lookupCtx, cancel := context.WithTimeout(ctx, gitProtocolLookupTimeout) defer cancel() - out, err := subproc.RunGHOutput(ctx, exec.CommandContext(ctx, "gh", "config", "get", "git_protocol")) - if err == nil { - if strings.TrimSpace(string(out)) == ProtocolHTTPS { - return ProtocolHTTPS + host, _, err := normalizeGitProviderHost(providerHost) + if err == nil && host != "" { + if protocol := r.lookup(lookupCtx, "-h", host, "git_protocol"); protocol != "" { + return protocol + } + } + if lookupCtx.Err() == nil { + if protocol := r.lookup(lookupCtx, "git_protocol"); protocol != "" { + return protocol } } return ProtocolSSH } +func (r *gitProtocolResolver) lookup(ctx context.Context, args ...string) string { + out, err := r.run(ctx, append([]string{"config", "get"}, args...)...) + if err != nil || ctx.Err() != nil { + return "" + } + protocol := strings.TrimSpace(string(out)) + if protocol == ProtocolSSH || protocol == ProtocolHTTPS { + return protocol + } + return "" +} + // CloneURL builds a clone URL for the given provider, owner, name, and protocol. // For SSH: git@github.com:{owner}/{name}.git // For HTTPS: https://github.com/{owner}/{name}.git @@ -45,18 +88,11 @@ func CloneURL(provider, owner, name, protocol string) (string, error) { // provider's default — used for self-managed GitLab. When host is empty, // behaves exactly like CloneURL. func CloneURLWithHost(provider, host, owner, name, protocol string) (string, error) { - resolvedHost := strings.TrimSpace(strings.TrimRight(host, "/")) - httpsScheme := "https" - if strings.Contains(resolvedHost, "://") { - parsed, err := url.Parse(resolvedHost) - if err != nil || parsed.Host == "" || parsed.User != nil || (parsed.Scheme != "http" && parsed.Scheme != "https") || (parsed.Path != "" && parsed.Path != "/") { - return "", fmt.Errorf("invalid git provider host: %q", host) - } - resolvedHost = parsed.Host - httpsScheme = parsed.Scheme + resolvedHost, httpsScheme, err := normalizeGitProviderHost(host) + if err != nil { + return "", err } if resolvedHost == "" { - var err error resolvedHost, err = providerHost(provider) if err != nil { return "", err @@ -74,6 +110,22 @@ func CloneURLWithHost(provider, host, owner, name, protocol string) (string, err return fmt.Sprintf("%s://%s/%s/%s.git", httpsScheme, resolvedHost, owner, name), nil } +func normalizeGitProviderHost(host string) (string, string, error) { + resolvedHost := strings.TrimSpace(strings.TrimRight(host, "/")) + httpsScheme := "https" + if strings.Contains(resolvedHost, "://") { + parsed, err := url.Parse(resolvedHost) + if err != nil || parsed.Host == "" || parsed.User != nil || + (parsed.Scheme != "http" && parsed.Scheme != "https") || + (parsed.Path != "" && parsed.Path != "/") { + return "", "", fmt.Errorf("invalid git provider host: %q", host) + } + resolvedHost = parsed.Host + httpsScheme = parsed.Scheme + } + return resolvedHost, httpsScheme, nil +} + // providerHost maps a provider name to its git host. func providerHost(provider string) (string, error) { switch strings.ToLower(provider) { diff --git a/apps/backend/internal/repoclone/protocol_test.go b/apps/backend/internal/repoclone/protocol_test.go index d532d10c14..e18494746b 100644 --- a/apps/backend/internal/repoclone/protocol_test.go +++ b/apps/backend/internal/repoclone/protocol_test.go @@ -1,6 +1,15 @@ package repoclone -import "testing" +import ( + "context" + "errors" + "os" + "path/filepath" + "reflect" + "strings" + "testing" + "time" +) func TestCloneURL(t *testing.T) { tests := []struct { @@ -142,9 +151,122 @@ func TestProviderHost(t *testing.T) { } } -func TestDetectGitProtocol(t *testing.T) { - got := DetectGitProtocol() - if got != ProtocolSSH && got != ProtocolHTTPS { - t.Errorf("expected %q or %q, got %q", ProtocolSSH, ProtocolHTTPS, got) +func TestDetectGitProtocolPrefersHostConfiguration(t *testing.T) { + ghPath := filepath.Join(t.TempDir(), "gh") + script := "#!/bin/sh\n" + + "if [ \"$3\" = \"-h\" ]; then printf 'ssh\\n'; else printf 'https\\n'; fi\n" + if err := os.WriteFile(ghPath, []byte(script), 0o755); err != nil { + t.Fatalf("write fake gh: %v", err) + } + t.Setenv("PATH", filepath.Dir(ghPath)) + + resolver := NewGitProtocolResolver() + if got := resolver.ResolveGitProtocol(context.Background(), "https://github.com/"); got != ProtocolSSH { + t.Fatalf("ResolveGitProtocol() = %q, want host-specific %q", got, ProtocolSSH) + } +} + +func TestDetectGitProtocolFallbacks(t *testing.T) { + tests := []struct { + name string + hostOutput string + hostErr error + globalOut string + globalErr error + want string + wantCalls [][]string + }{ + { + name: "host-specific value wins", + hostOutput: ProtocolSSH, + globalOut: ProtocolHTTPS, + want: ProtocolSSH, + wantCalls: [][]string{{"config", "get", "-h", "github.com", "git_protocol"}}, + }, + { + name: "global value is fallback", + hostErr: errors.New("host config unavailable"), + globalOut: ProtocolHTTPS, + want: ProtocolHTTPS, + wantCalls: [][]string{ + {"config", "get", "-h", "github.com", "git_protocol"}, + {"config", "get", "git_protocol"}, + }, + }, + { + name: "unsupported host value falls back", + hostOutput: "ssh+git", + globalOut: ProtocolHTTPS, + want: ProtocolHTTPS, + wantCalls: [][]string{ + {"config", "get", "-h", "github.com", "git_protocol"}, + {"config", "get", "git_protocol"}, + }, + }, + { + name: "unsupported values default to ssh", + hostOutput: "git", + globalOut: "http", + want: ProtocolSSH, + wantCalls: [][]string{ + {"config", "get", "-h", "github.com", "git_protocol"}, + {"config", "get", "git_protocol"}, + }, + }, + { + name: "command failures default to ssh", + hostErr: errors.New("host lookup failed"), + globalErr: errors.New("global lookup failed"), + want: ProtocolSSH, + wantCalls: [][]string{ + {"config", "get", "-h", "github.com", "git_protocol"}, + {"config", "get", "git_protocol"}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var calls [][]string + runner := func(_ context.Context, args ...string) ([]byte, error) { + calls = append(calls, append([]string(nil), args...)) + if len(calls) == 1 { + return []byte(tt.hostOutput), tt.hostErr + } + return []byte(tt.globalOut), tt.globalErr + } + resolver := newGitProtocolResolver(runner) + + got := resolver.ResolveGitProtocol(context.Background(), "https://github.com/") + if got != tt.want { + t.Fatalf("ResolveGitProtocol() = %q, want %q", got, tt.want) + } + if !reflect.DeepEqual(calls, tt.wantCalls) { + t.Fatalf("gh calls = %v, want %v", calls, tt.wantCalls) + } + if len(calls) > 0 && strings.Contains(strings.Join(calls[0], " "), "https://") { + t.Fatalf("host-specific gh lookup received URL instead of hostname: %v", calls[0]) + } + }) + } +} + +func TestDetectGitProtocolStopsFallbackAfterContextDeadline(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Millisecond) + defer cancel() + + calls := 0 + runner := func(ctx context.Context, _ ...string) ([]byte, error) { + calls++ + <-ctx.Done() + return nil, ctx.Err() + } + resolver := newGitProtocolResolver(runner) + + if got := resolver.ResolveGitProtocol(ctx, "github.com"); got != ProtocolSSH { + t.Fatalf("ResolveGitProtocol() = %q, want SSH default", got) + } + if calls != 1 { + t.Fatalf("gh lookup calls = %d, want one lookup after deadline", calls) } } diff --git a/docs/plans/github-host-protocol-resolution/plan.md b/docs/plans/github-host-protocol-resolution/plan.md new file mode 100644 index 0000000000..4c164ba979 --- /dev/null +++ b/docs/plans/github-host-protocol-resolution/plan.md @@ -0,0 +1,129 @@ +--- +created: 2026-08-27 +status: complete +requirements: + - REQ-INTEGRATIONS-GITHUB-AUTHENTICATION-001 +system_design: + - ../../specs/integrations/system-design/github-authentication-01.md + - ../../specs/integrations/system-design/github-authentication-02.md + - ../../specs/integrations/system-design/github-authentication-03.md +legacy_specs: [] +--- + +# Fix Plan: Resolve GitHub Clone Protocol Per Host At Use Time + +## Overview + +Replace the backend-startup Git protocol snapshots with one host-aware resolver. Evaluate this +resolver when Kandev constructs a clone URL or an executor-inherited managed-checkout origin. Then +route the long-lived cloner and review-repository adapter through it. The tests prove that all paths +observe configuration changes without a restart. + +## Confirmed root cause + +`repoclone.DetectGitProtocol` runs only `gh config get git_protocol`. Thus, a global `https` value +wins when `gh config get -h github.com git_protocol` reports the effective `ssh` preference. +`backendapp/main.go` passes that incorrect startup result into one long-lived `repoclone.Cloner`. +`backendapp/orchestrator.go` stores a second startup result on `repositoryResolverAdapter`. +Executor-inherited origin reconciliation selects the wrong transport. It cannot observe later `gh` +configuration changes until the backend restarts. + +The smallest reproduction uses an isolated `GH_CONFIG_DIR`. Set global `git_protocol=https` and +host-specific `github.com` `git_protocol=ssh`. The current detector reads `https`. +`gh config get -h github.com git_protocol` reads `ssh`. + +## Scope + +### In scope + +- Resolve the `github.com` clone protocol from host-specific configuration first. Use global + configuration second. Use SSH by default when neither scope yields `ssh` or `https`. +- Resolve the protocol at clone URL construction and Local/Worktree launch or resume origin + reconciliation, not backend startup. +- Remove the cached protocol field from `repositoryResolverAdapter` and route it through the same + live resolver as `repoclone.Cloner`. +- Preserve canonical HTTPS for managed task credentials and preserve user-managed local origins. +- Add deterministic tests with injected command and protocol resolvers. + +### Out of scope + +- Executor-mode credential-helper or token bridging tracked by #3072. +- Discovering or validating the actor behind inherited SSH agents or credential managers. +- GitHub Enterprise Server or hosts other than the existing `github.com` contract. +- Frontend, API, persistence, schema, or workspace-policy changes. + +## Technical approach + +### Host-aware protocol resolver + +- In `apps/backend/internal/repoclone/protocol.go`, make protocol resolution accept a context and a + provider host. Query `gh config get -h git_protocol` first. Then query + `gh config get git_protocol`. Accept only `ssh` and `https`. Otherwise, return the existing SSH + default. Bound the complete host-plus-global resolution operation with one five-second command + context. +- Isolate command execution behind an injected runner. Unit tests control the results and assert + command order without reading the developer's `gh` configuration. Keep one five-second command + timeout for the complete resolution operation. + +### Use-boundary resolution + +- In `apps/backend/internal/repoclone/clone.go`, store an injected protocol resolver, not a resolved + protocol string. Make protocol-aware URL construction resolve the normalized provider host on + every call. +- In `apps/backend/internal/backendapp/main.go`, construct the cloner with the live resolver and + remove the startup call to `DetectGitProtocol`. +- In `apps/backend/internal/orchestrator/executor/executor.go` and + `executor_resume.go`, thread the operation context through the protocol-aware `RepoCloner` URL + builder. The shared launch and resume path continues to reconcile only Kandev-managed GitHub + checkouts. Managed mode remains explicitly HTTPS. +- In `apps/backend/internal/backendapp/orchestrator.go`, replace the adapter's concrete cached + protocol field with a narrow cloner interface that builds the URL at `ResolveForReview` use time. + Production wiring supplies the same `repoclone.Cloner`. Tests supply a deterministic fake. + +## Tests + +- `TestDetectGitProtocolPrefersHostConfiguration` in + `apps/backend/internal/repoclone/protocol_test.go` is the primary regression. Before the + correction, the command runner observes no host-specific lookup. The result is HTTPS. + Companion cases cover global fallback, unsupported values, command failures, and SSH default. +- `TestClonerBuildCloneURLUsesCurrentProtocol` in + `apps/backend/internal/repoclone/clone_test.go` mutates an injected resolver between calls and + proves one long-lived cloner does not cache a result. +- `TestEnsureRepoLocalPathReevaluatesGitHubProtocol` in + `apps/backend/internal/orchestrator/executor/executor_resume_clone_transport_test.go` performs two + preparation passes with different resolver results. It proves that the managed origin changes + without executor recreation. Existing managed-HTTPS and user-managed-local cases cover the + exclusions. +- `TestRepositoryResolverAdapterUsesCurrentGitProtocol` in + `apps/backend/internal/backendapp/adapters_test.go` performs two resolutions through an injected + fake and proves the adapter has no independent startup cache. + +## Execution checklist + +- [x] Audit the current plan, specifications, work order, and working tree. +- [x] Implement the live host-aware protocol resolver and use-time URL construction. +- [x] Update executor origin reconciliation and review-repository wiring. +- [x] Add and pass the specified regression tests. +- [x] Update plan/work-order verification results. +- [x] Commit with hooks, push the branch, and create a ready-for-review PR. +- [x] Wait 15 minutes, run PR fixup, and report the final PR state. + +## Work orders + +- [x] [Task 01: Resolve GitHub clone protocol at use time](task-01-resolve-github-clone-protocol.md) + +## Verification results + +- `rtk go test -race ./internal/repoclone ./internal/orchestrator/executor ./internal/backendapp -run 'Test(DetectGitProtocol|ClonerBuildCloneURLUsesCurrentProtocol|EnsureRepoLocalPathReevaluatesGitHubProtocol|RepositoryResolverAdapterUsesCurrentGitProtocol)' -count=1` passed with 11 tests in 3 packages, including the total-timeout regression. +- `rtk go test -race ./internal/repoclone ./internal/orchestrator/executor ./internal/backendapp` passed with 1,316 tests in 3 packages. +- PR 3078 at `662d3c98d3ddd560ff46cab83e2d8cf965571f65` completed with 44 checks passed, 0 failed, 0 pending, and 0 unresolved review threads. GitHub reports `MERGEABLE / CLEAN`. + +## Risks + +- Adding context to protocol-aware URL construction touches executor test fakes. All implementations + must preserve the same host and operation context. +- A `gh` lookup now occurs on each relevant URL construction. One five-second bound covers the + complete host-plus-global resolution, and the shared `gh` subprocess throttle limits the cost. + Correctness requires no process cache. +- Provider-host normalization must pass `github.com`, not an HTTPS URL, to `gh --host`. Clone URL + construction must retain the canonical persisted provider origin. diff --git a/docs/plans/github-host-protocol-resolution/task-01-resolve-github-clone-protocol.md b/docs/plans/github-host-protocol-resolution/task-01-resolve-github-clone-protocol.md new file mode 100644 index 0000000000..1db139d017 --- /dev/null +++ b/docs/plans/github-host-protocol-resolution/task-01-resolve-github-clone-protocol.md @@ -0,0 +1,110 @@ +--- +id: "01-resolve-github-clone-protocol" +title: "Resolve GitHub clone protocol at use time" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-INTEGRATIONS-GITHUB-AUTHENTICATION-001 +acceptance_criteria: + - AC-INTEGRATIONS-GITHUB-AUTHENTICATION-001.9 + - AC-INTEGRATIONS-GITHUB-AUTHENTICATION-001.10 +system_design: + - ../../specs/integrations/system-design/github-authentication-01.md + - ../../specs/integrations/system-design/github-authentication-02.md + - ../../specs/integrations/system-design/github-authentication-03.md +--- + +# Task 01: Resolve GitHub Clone Protocol At Use Time + +## Summary + +Implement one host-aware, live Git protocol resolver. Use it for executor-inherited checkout origins +and review-repository clone URLs. Remove both backend-startup result caches. Prove that the same +long-lived runtime observes host configuration changes. + +## In scope + +- Add the per-host, global-fallback, default-SSH resolver with an injected command runner. +- Inject the resolver into `repoclone.Cloner` and evaluate it for each protocol-aware URL build. +- Thread context through executor URL construction and preserve the launch/resume reconciliation + boundary. +- Remove `repositoryResolverAdapter.protocol` and resolve through a narrow injected cloner seam. +- Add deterministic regression coverage in `repoclone`, `executor`, and `backendapp`. + +## Out of scope + +- Credential-helper bridging or any #3072 behavior. +- Credential probing, new providers or hosts, UI/API changes, persistence, or migrations. +- Rewriting user-managed local checkout origins or changing managed-mode HTTPS. + +## Acceptance + +- Host-specific `github.com` `ssh` overrides global `https`. +- A supported global value applies when no supported host-specific value is available. +- SSH is the final default. +- A running backend observes a protocol change on the next Local/Worktree launch or resume and + reconciles the Kandev-managed checkout without restart. +- `repositoryResolverAdapter` uses the same live resolution path and stores no protocol result. + +## Verification + +```bash +# From apps/backend: +rtk go test -race ./internal/repoclone ./internal/orchestrator/executor ./internal/backendapp -run 'Test(DetectGitProtocol|ClonerBuildCloneURLUsesCurrentProtocol|EnsureRepoLocalPathReevaluatesGitHubProtocol|RepositoryResolverAdapterUsesCurrentGitProtocol)' -count=1 +``` + +The targeted command passed with 11 tests in 3 packages, including the total-timeout regression. + +## Files likely touched + +- `apps/backend/internal/repoclone/protocol.go` +- `apps/backend/internal/repoclone/protocol_test.go` +- `apps/backend/internal/repoclone/clone.go` +- `apps/backend/internal/repoclone/clone_test.go` +- `apps/backend/internal/backendapp/main.go` +- `apps/backend/internal/backendapp/orchestrator.go` +- `apps/backend/internal/backendapp/adapters_test.go` +- `apps/backend/internal/orchestrator/executor/executor.go` +- `apps/backend/internal/orchestrator/executor/executor_resume.go` +- `apps/backend/internal/orchestrator/executor/executor_resume_clone_transport_test.go` +- Executor test files whose `RepoCloner` fakes implement the context-aware URL builder. + +## Dependencies + +None. + +## Risks + +- Incorrect host normalization can query a non-existent `gh` host scope or construct a different + clone URL. Test the command arguments and final canonical URLs separately. +- A constructor string or adapter field can reintroduce restart-only behavior. Mutable-resolver + tests at both long-lived consumers prevent this error. + +## Parallelism + +`sequential` + +## Inputs + +- `REQ-INTEGRATIONS-GITHUB-AUTHENTICATION-001`, especially + `AC-INTEGRATIONS-GITHUB-AUTHENTICATION-001.9` and `.10`. +- The three Workspace GitHub Authentication system-design parts referenced above. +- `ADR-2026-07-27-task-git-credential-policy`. +- Issue #3069 and the confirmed isolated `GH_CONFIG_DIR` reproduction in `plan.md`. +- Existing origin-reconciliation coverage in + `executor_resume_clone_transport_test.go` and review-repository adapter coverage in + `adapters_test.go`. + +## Results + +- Replaced startup Git protocol snapshots with host-aware, context-bounded resolution. Host-specific + `gh` configuration takes precedence over global configuration, with SSH as the final default. One + five-second context bounds the complete host-plus-global lookup. +- Injected the resolver into the long-lived repository cloner and review-repository adapter. Executor + origin reconciliation now evaluates it on every applicable launch or resume. +- Passed the targeted regression command with 11 tests in 3 packages and the full race-enabled + affected-package suites with 1,316 tests in 3 packages. +- PR 3078 completed its fixup with 44 checks passed, no failures or pending checks, and no unresolved + review threads. diff --git a/docs/specs/integrations/requirements/github-authentication.md b/docs/specs/integrations/requirements/github-authentication.md index b032633172..f95726c0c5 100644 --- a/docs/specs/integrations/requirements/github-authentication.md +++ b/docs/specs/integrations/requirements/github-authentication.md @@ -27,6 +27,8 @@ GitHub credentials must not silently cross workspace boundaries. A local workspa - **AC-INTEGRATIONS-GITHUB-AUTHENTICATION-001.6:** Reusing one registration shares its App private key, client secret, webhook secret, permission policy, and bot identity. Installation tokens, workspace repository scope, connection generation, broker leases, health, and personal OAuth tokens remain workspace isolated. - **AC-INTEGRATIONS-GITHUB-AUTHENTICATION-001.7:** A newly created App defaults to private, meaning GitHub permits installation only on the account that owns it. The user may explicitly choose public when the same App must be installable on other GitHub accounts or organizations. Public does not list the App in Marketplace, reveal secrets, or grant repository access without installation approval. - **AC-INTEGRATIONS-GITHUB-AUTHENTICATION-001.8:** PAT and named CLI automation act as the verified human account. A separate `My GitHub` connection is only offered when workspace automation uses a GitHub App, because App installations are not people and cannot provide authenticated-viewer semantics. +- **AC-INTEGRATIONS-GITHUB-AUTHENTICATION-001.9:** When a Local or Worktree task inherits executor Git credentials, every Kandev-managed GitHub checkout shall use the current host `gh` clone protocol for `github.com`. Host-specific configuration takes precedence over global configuration, and SSH is used when neither scope provides a supported protocol. +- **AC-INTEGRATIONS-GITHUB-AUTHENTICATION-001.10:** Every Local or Worktree launch and resume shall re-evaluate the inherited clone protocol. The system shall reconcile the Kandev-managed checkout before task preparation continues. This behavior shall not require a backend restart. Kandev shall not rewrite the origin of a user-managed local checkout. ## System design diff --git a/docs/specs/integrations/system-design/github-authentication-01.md b/docs/specs/integrations/system-design/github-authentication-01.md index 7f92de7261..391203a38b 100644 --- a/docs/specs/integrations/system-design/github-authentication-01.md +++ b/docs/specs/integrations/system-design/github-authentication-01.md @@ -95,14 +95,19 @@ automation under different GitHub Apps without operating separate Kandev deploym actions because they leave Kandev for GitHub rather than saving a local connection draft. - For Kandev-managed GitHub checkouts used by Local and Worktree tasks, the selected task policy also controls the persisted `origin` transport. Managed routing uses canonical GitHub HTTPS. - Executor inheritance uses the host's detected `gh` clone protocol, including SSH, and reconciles - an existing managed checkout when the policy changes. This makes Git conditional includes based - on `remote.*.url` observe the same transport the task uses. Kandev never rewrites the remote of a - repository registered as a user-managed local checkout. + Executor inheritance resolves the current `github.com` protocol from host `gh` configuration at + the clone or origin-reconciliation boundary. Resolution accepts the host-specific value first, + then the global value, and defaults to SSH when neither scope supplies `ssh` or `https`. Launch + and resume do not use a backend-startup snapshot, so a later host configuration change applies + without restart. This makes Git conditional includes based on `remote.*.url` observe the same + transport the task uses. Kandev never rewrites the remote of a repository registered as a + user-managed local checkout. - Repository preparation resolves each attached repository once per launch or resume and reuses that result for primary-repository configuration, multi-repository configuration, and credential - routing. Origin reconciliation is serialized per managed checkout, compares the current and - desired canonical URLs, and performs no write when they already match. + routing. The clone-protocol resolver itself is stateless and injected at the repository URL + construction boundary. The long-lived cloner and review-repository adapter do not store a + resolved protocol. Origin reconciliation is serialized per managed checkout. It compares the + current and desired canonical URLs, and performs no write when they already match. - Repository preparation validates any contribution destination before issuing credentials, adds a collision-resistant dedicated fork remote, and reconstructs it on launch and resume. It never accepts a fork inferred from the checkout's current remotes or a caller-provided repository name. diff --git a/docs/specs/integrations/system-design/github-authentication-02.md b/docs/specs/integrations/system-design/github-authentication-02.md index 1585e98cf2..64444c3555 100644 --- a/docs/specs/integrations/system-design/github-authentication-02.md +++ b/docs/specs/integrations/system-design/github-authentication-02.md @@ -71,6 +71,10 @@ post-signature processing failures produce `failing`; a later valid successful d - Each attached repository is resolved once for an individual launch or resume. The resolved repository set is shared by task configuration and credential-snapshot construction rather than triggering repeated materialization or origin mutation. +- Executor-inherited Local and Worktree preparation resolves the clone protocol while it constructs + the desired `github.com` origin. It queries host-specific `gh` configuration before global + configuration and defaults to SSH when neither query returns a supported protocol. The resolver + is called again for every launch and resume. Backend startup stores no protocol result. - Broker issuance and redemption use the same persisted repository identity fields. A local Git checkout and its `origin` are materialization data, not authorization data. - A public `github.com` remote can supply an in-memory origin when persisted provider-host metadata @@ -151,6 +155,9 @@ post-signature processing failures produce `failing`; a later valid successful d truncating, partially merging, or executing a different block. - If executor inheritance is selected but no usable credential exists in that executor, Git/SSH reports its normal authentication failure. Kandev does not probe or guess the actor. +- If host-specific `gh` clone-protocol configuration is unavailable or unsupported, resolution + tries the global value. If neither scope returns `ssh` or `https`, executor inheritance uses SSH. + A configuration lookup failure does not reuse an earlier process-cached result. - If host `gh` is absent, unauthenticated, or fails active-account validation while a workspace is created, the workspace remains disconnected for automation, retains executor task access, and creation succeeds. diff --git a/docs/specs/integrations/system-design/github-authentication-03.md b/docs/specs/integrations/system-design/github-authentication-03.md index bb80743fa1..3c06ea44d0 100644 --- a/docs/specs/integrations/system-design/github-authentication-03.md +++ b/docs/specs/integrations/system-design/github-authentication-03.md @@ -106,10 +106,19 @@ This design preserves the technical source detail for `REQ-INTEGRATIONS-GITHUB-A **THEN** it explains that Git calls Kandev's `agentctl` credential helper for attached GitHub HTTPS repositories, the helper redeems a scoped broker lease on demand, `gh` uses a broker-aware shim, and credentials are not written into the repository. -- **GIVEN** the host `gh` clone protocol is SSH and a Kandev-managed GitHub checkout currently has - an HTTPS `origin`, **WHEN** the workspace selects executor inheritance and launches a Local or - Worktree task, **THEN** Kandev changes that managed checkout's `origin` to the canonical SSH URL - before task preparation so matching Git conditional includes apply. +- **GIVEN** host-specific `github.com` `gh` configuration selects SSH while global configuration + selects HTTPS. The Kandev-managed GitHub checkout has an HTTPS `origin`. **WHEN** the workspace + uses executor inheritance and launches or resumes a Local or Worktree task. **THEN** Kandev uses + the host-specific value. Kandev changes the managed checkout to canonical SSH before task + preparation, so matching Git conditional includes apply. +- **GIVEN** host-specific `github.com` `gh` clone-protocol configuration is unavailable. **WHEN** + Local or Worktree preparation constructs an executor-inherited managed-checkout origin. **THEN** + Kandev uses the supported global protocol. Kandev defaults to SSH when neither scope returns + `ssh` or `https`. +- **GIVEN** a Local or Worktree task previously launched with one host `gh` clone protocol. + **WHEN** the host-specific or global protocol changes and the task launches or resumes without a + Kandev restart. **THEN** origin reconciliation uses the new effective protocol, not the earlier + process value. - **GIVEN** a Kandev-managed GitHub checkout currently has an SSH `origin`, **WHEN** the workspace selects managed credentials and launches a Local or Worktree task, **THEN** Kandev changes that managed checkout's `origin` to canonical HTTPS before task preparation.