diff --git a/backend/internal/adapters/scm/github/observer_provider.go b/backend/internal/adapters/scm/github/observer_provider.go index fb9aebb10b..f65efa357f 100644 --- a/backend/internal/adapters/scm/github/observer_provider.go +++ b/backend/internal/adapters/scm/github/observer_provider.go @@ -19,6 +19,7 @@ import ( "github.com/aoagents/agent-orchestrator/backend/internal/domain" "github.com/aoagents/agent-orchestrator/backend/internal/ports" + aoprocess "github.com/aoagents/agent-orchestrator/backend/internal/process" ) const scmBatchCheckContextLimit = 20 @@ -39,9 +40,10 @@ const ( // ParseRepository normalizes a GitHub remote/origin URL into a provider-neutral // repository key. It accepts https://github.com/owner/repo(.git), -// git@github.com:owner/repo(.git), and path-only owner/repo inputs used by tests. +// git@github.com:owner/repo(.git), SSH aliases whose effective hostname is a +// GitHub host, and path-only owner/repo inputs used by tests. func (p *Provider) ParseRepository(remote string) (ports.SCMRepo, bool) { - repo, ok := parseGitHubRepo(remote) + repo, ok := parseGitHubRepo(remote, p.resolveSSHHost) return repo, ok } @@ -620,7 +622,7 @@ func scmThreadFromGraphQL(th map[string]any) ports.SCMReviewThreadObservation { return out } -func parseGitHubRepo(remote string) (ports.SCMRepo, bool) { +func parseGitHubRepo(remote string, resolveSSHHost func(string) (string, bool)) (ports.SCMRepo, bool) { raw := strings.TrimSpace(remote) if raw == "" { return ports.SCMRepo{}, false @@ -631,7 +633,15 @@ func parseGitHubRepo(remote string) (ports.SCMRepo, bool) { if len(parts) != 2 { return ports.SCMRepo{}, false } - host := strings.ToLower(parts[0]) + // Preserve alias casing for ssh -G / Host matching; lowercase only for + // GitHub-host comparison and the normalized repo key. + hostAlias := strings.TrimSpace(parts[0]) + host := strings.ToLower(hostAlias) + if !isGitHubHost(host) && resolveSSHHost != nil { + if resolved, ok := resolveSSHHost(hostAlias); ok { + host = strings.ToLower(resolved) + } + } owner, name, ok := splitOwnerRepo(parts[1]) return makeGitHubRepo(host, owner, name), ok && isGitHubHost(host) } @@ -643,11 +653,78 @@ func parseGitHubRepo(remote string) (ports.SCMRepo, bool) { if err != nil { return ports.SCMRepo{}, false } - host := strings.ToLower(u.Host) + hostAlias := strings.TrimSpace(u.Hostname()) + host := strings.ToLower(hostAlias) + if strings.EqualFold(u.Scheme, "ssh") && !isGitHubHost(host) && resolveSSHHost != nil { + if resolved, ok := resolveSSHHost(hostAlias); ok { + host = strings.ToLower(resolved) + } + } owner, name, ok := splitOwnerRepo(u.Path) return makeGitHubRepo(host, owner, name), ok && isGitHubHost(host) } +func (p *Provider) resolveSSHHost(host string) (string, bool) { + // Keep original casing: OpenSSH Host patterns are case-sensitive. + host = strings.TrimSpace(host) + if host == "" || strings.HasPrefix(host, "-") || p == nil || p.sshHostResolver == nil { + return "", false + } + p.sshHostMu.Lock() + if resolved, ok := p.sshHosts[host]; ok { + p.sshHostMu.Unlock() + return resolved, true + } + p.sshHostMu.Unlock() + + resolved, ok := p.sshHostResolver(host) + resolved = strings.ToLower(strings.TrimSpace(resolved)) + if !ok || resolved == "" { + // Do not cache failures: transient ssh/timeout errors should recover on + // the next observer poll without a daemon restart. + return "", false + } + + p.sshHostMu.Lock() + if p.sshHosts == nil { + p.sshHosts = make(map[string]string) + } + if cached, ok := p.sshHosts[host]; ok { + p.sshHostMu.Unlock() + return cached, true + } + p.sshHosts[host] = resolved + p.sshHostMu.Unlock() + return resolved, true +} + +func resolveSSHConfigHost(host string) (string, bool) { + host = strings.TrimSpace(host) + if host == "" || strings.HasPrefix(host, "-") { + return "", false + } + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + // "--" keeps hosts that start with "-" from being parsed as ssh flags + // (those are also rejected above; this is defense in depth). + out, err := aoprocess.CommandContext(ctx, "ssh", "-G", "--", host).Output() + if err != nil { + return "", false + } + return sshConfigHostname(string(out)) +} + +func sshConfigHostname(config string) (string, bool) { + for _, line := range strings.Split(config, "\n") { + fields := strings.Fields(line) + if len(fields) >= 2 && strings.EqualFold(fields[0], "hostname") { + resolved := strings.TrimSpace(fields[1]) + return resolved, resolved != "" + } + } + return "", false +} + func splitOwnerRepo(p string) (string, string, bool) { parts := strings.Split(strings.Trim(p, "/"), "/") if len(parts) < 2 { diff --git a/backend/internal/adapters/scm/github/provider.go b/backend/internal/adapters/scm/github/provider.go index a125c4887d..0aebe8f8de 100644 --- a/backend/internal/adapters/scm/github/provider.go +++ b/backend/internal/adapters/scm/github/provider.go @@ -11,6 +11,7 @@ import ( "path" "strconv" "strings" + "sync" "github.com/aoagents/agent-orchestrator/backend/internal/domain" "github.com/aoagents/agent-orchestrator/backend/internal/ports" @@ -27,6 +28,9 @@ type ProviderOptions struct { Client *Client HTTPClient *http.Client Token TokenSource + // SSHHostResolver resolves an SSH config alias to its effective hostname. + // Production uses `ssh -G`; tests inject a resolver to avoid local config. + SSHHostResolver func(host string) (string, bool) // SkipTokenPreflight defers token validation until the first provider call. // Daemon wiring uses this so gh-token shell-out never blocks readiness. SkipTokenPreflight bool @@ -41,8 +45,14 @@ type ProviderOptions struct { // loop in v1 — the loop is a follow-up PR (#35); this adapter is the // observation primitive that loop will call. type Provider struct { - client *Client - logger *slog.Logger + client *Client + logger *slog.Logger + sshHostResolver func(host string) (string, bool) + sshHostMu sync.Mutex + // sshHosts caches successful alias → resolved hostname lookups only. + // Failures are not cached so a later poll can recover without restart. + // Keys keep the original alias casing so OpenSSH Host matches stay case-sensitive. + sshHosts map[string]string } // NewProvider returns a Provider. If opts.Client is supplied it is used @@ -71,7 +81,16 @@ func NewProvider(opts ProviderOptions) (*Provider, error) { if logger == nil { logger = slog.Default() } - return &Provider{client: c, logger: logger}, nil + sshHostResolver := opts.SSHHostResolver + if sshHostResolver == nil { + sshHostResolver = resolveSSHConfigHost + } + return &Provider{ + client: c, + logger: logger, + sshHostResolver: sshHostResolver, + sshHosts: make(map[string]string), + }, nil } // SCMCredentialsAvailable checks whether this provider can obtain a token. The diff --git a/backend/internal/adapters/scm/github/repository_test.go b/backend/internal/adapters/scm/github/repository_test.go new file mode 100644 index 0000000000..418de76d43 --- /dev/null +++ b/backend/internal/adapters/scm/github/repository_test.go @@ -0,0 +1,239 @@ +package github + +import "testing" + +func TestParseRepository(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + remote string + resolve func(string) (string, bool) + wantHost string + wantRepo string + wantParsed bool + }{ + { + name: "https github", + remote: "https://github.com/acme/demo.git", + wantHost: "github.com", + wantRepo: "acme/demo", + wantParsed: true, + }, + { + name: "scp github", + remote: "git@github.com:acme/demo.git", + wantHost: "github.com", + wantRepo: "acme/demo", + wantParsed: true, + }, + { + name: "scp ssh alias", + remote: "git@github-work:acme/demo.git", + resolve: func(string) (string, bool) { return "github.com", true }, + wantHost: "github.com", + wantRepo: "acme/demo", + wantParsed: true, + }, + { + name: "case sensitive ssh alias", + remote: "git@GitHub-Work:acme/demo.git", + resolve: func(host string) (string, bool) { + // OpenSSH Host patterns are case-sensitive; only the exact alias matches. + if host == "GitHub-Work" { + return "github.com", true + } + return "", false + }, + wantHost: "github.com", + wantRepo: "acme/demo", + wantParsed: true, + }, + { + name: "ssh url alias", + remote: "ssh://git@github-work/acme/demo.git", + resolve: func(string) (string, bool) { + return "github.com", true + }, + wantHost: "github.com", + wantRepo: "acme/demo", + wantParsed: true, + }, + { + name: "ssh url case sensitive alias", + remote: "ssh://git@GitHub-Work/acme/demo.git", + resolve: func(host string) (string, bool) { + if host == "GitHub-Work" { + return "github.com", true + } + return "", false + }, + wantHost: "github.com", + wantRepo: "acme/demo", + wantParsed: true, + }, + { + name: "non github ssh alias", + remote: "git@gitlab-work:acme/demo.git", + resolve: func(string) (string, bool) { + return "gitlab.com", true + }, + wantParsed: false, + }, + { + name: "unresolved ssh alias", + remote: "git@code-host:acme/demo.git", + resolve: func(string) (string, bool) { return "", false }, + wantParsed: false, + }, + { + name: "leading dash host rejected", + remote: "git@-oProxyCommand=evil:acme/demo.git", + resolve: func(string) (string, bool) { + return "github.com", true + }, + wantParsed: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + provider, err := NewProvider(ProviderOptions{ + Client: NewClient(ClientOptions{}), + SSHHostResolver: tt.resolve, + }) + if err != nil { + t.Fatalf("NewProvider: %v", err) + } + repo, ok := provider.ParseRepository(tt.remote) + if ok != tt.wantParsed { + t.Fatalf("ParseRepository(%q) ok = %v, want %v; repo=%+v", tt.remote, ok, tt.wantParsed, repo) + } + if !tt.wantParsed { + return + } + if repo.Provider != "github" || repo.Host != tt.wantHost || repo.Repo != tt.wantRepo { + t.Fatalf("ParseRepository(%q) = %+v, want host=%q repo=%q", tt.remote, repo, tt.wantHost, tt.wantRepo) + } + }) + } +} + +func TestParseRepositoryCachesSSHHostResolution(t *testing.T) { + t.Parallel() + + calls := 0 + provider, err := NewProvider(ProviderOptions{ + Client: NewClient(ClientOptions{}), + SSHHostResolver: func(host string) (string, bool) { + calls++ + return "github.com", host == "github-work" + }, + }) + if err != nil { + t.Fatalf("NewProvider: %v", err) + } + + for range 2 { + if _, ok := provider.ParseRepository("git@github-work:acme/demo.git"); !ok { + t.Fatal("ParseRepository returned ok=false") + } + } + if calls != 1 { + t.Fatalf("resolver calls = %d, want 1", calls) + } +} + +func TestParseRepositoryDoesNotCacheSSHHostFailures(t *testing.T) { + t.Parallel() + + calls := 0 + provider, err := NewProvider(ProviderOptions{ + Client: NewClient(ClientOptions{}), + SSHHostResolver: func(string) (string, bool) { + calls++ + if calls == 1 { + return "", false + } + return "github.com", true + }, + }) + if err != nil { + t.Fatalf("NewProvider: %v", err) + } + + if _, ok := provider.ParseRepository("git@github-work:acme/demo.git"); ok { + t.Fatal("first ParseRepository should fail when resolver fails") + } + if _, ok := provider.ParseRepository("git@github-work:acme/demo.git"); !ok { + t.Fatal("second ParseRepository should succeed after resolver recovers") + } + if calls != 2 { + t.Fatalf("resolver calls = %d, want 2 (failure must not be cached)", calls) + } +} + +func TestParseRepositoryPreservesSSHAliasCaseForResolver(t *testing.T) { + t.Parallel() + + var seen []string + provider, err := NewProvider(ProviderOptions{ + Client: NewClient(ClientOptions{}), + SSHHostResolver: func(host string) (string, bool) { + seen = append(seen, host) + if host == "GitHub-Work" { + return "github.com", true + } + return "", false + }, + }) + if err != nil { + t.Fatalf("NewProvider: %v", err) + } + + repo, ok := provider.ParseRepository("git@GitHub-Work:acme/demo.git") + if !ok { + t.Fatalf("ParseRepository ok=false; seen=%v", seen) + } + if repo.Host != "github.com" || repo.Repo != "acme/demo" { + t.Fatalf("repo=%+v", repo) + } + if len(seen) != 1 || seen[0] != "GitHub-Work" { + t.Fatalf("resolver hosts = %v, want exact [GitHub-Work]", seen) + } +} + +func TestSSHConfigHostname(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + config string + want string + ok bool + }{ + {name: "hostname", config: "user git\nhostname github.com\nport 22\n", want: "github.com", ok: true}, + {name: "case insensitive", config: "HostName github.com\n", want: "github.com", ok: true}, + {name: "missing", config: "user git\nport 22\n"}, + {name: "blank", config: "hostname \n"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, ok := sshConfigHostname(tt.config) + if got != tt.want || ok != tt.ok { + t.Fatalf("sshConfigHostname() = %q, %v; want %q, %v", got, ok, tt.want, tt.ok) + } + }) + } +} + +func TestResolveSSHConfigHostRejectsLeadingDash(t *testing.T) { + t.Parallel() + + if host, ok := resolveSSHConfigHost("-oProxyCommand=true"); ok || host != "" { + t.Fatalf("resolveSSHConfigHost(leading-dash) = %q, %v; want empty, false", host, ok) + } +}