Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 82 additions & 5 deletions backend/internal/adapters/scm/github/observer_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}

Expand Down Expand Up @@ -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
Expand All @@ -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)
}
Expand All @@ -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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The not-caching-failures behavior is correct for transient errors. One consequence to be aware of: a host that genuinely errors on every ssh -G (for example ssh binary missing) will re-shell-out with the 1s timeout on every observer poll for that remote, since nothing is cached. In practice ssh -G alias succeeds even with no matching Host block (it echoes the alias back as hostname), so that success path does get cached and the repeated shell-out only affects true ssh failures. Acceptable tradeoff, just worth a comment so it is not mistaken for a leak.

// 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()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The -- plus leading-dash rejection is the right hardening. Note a shared limitation with #2957/#2965 (not introduced here): only the git@-prefixed scp form and ssh:// URL form are resolved. An scp remote without the git@ user (for example work:owner/repo, valid when ssh_config sets User) skips the git@ branch and is not alias-resolved. Fine to leave for a follow-up, but flagging in case such remotes are expected.

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 {
Expand Down
25 changes: 22 additions & 3 deletions backend/internal/adapters/scm/github/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
Loading