diff --git a/apps/backend/internal/agent/runtime/routingerr/sanitize.go b/apps/backend/internal/agent/runtime/routingerr/sanitize.go index 58f018e260..2daa9daec1 100644 --- a/apps/backend/internal/agent/runtime/routingerr/sanitize.go +++ b/apps/backend/internal/agent/runtime/routingerr/sanitize.go @@ -41,3 +41,24 @@ func Sanitize(s string) string { } return s } + +type sanitizedError struct { + cause error + message string +} + +func (e *sanitizedError) Error() string { return e.message } +func (e *sanitizedError) Unwrap() error { return e.cause } + +// SanitizeError redacts an error message while preserving errors.Is/errors.As +// access to the original cause for control flow. +func SanitizeError(err error) error { + if err == nil { + return nil + } + message := Sanitize(err.Error()) + if message == err.Error() { + return err + } + return &sanitizedError{cause: err, message: message} +} diff --git a/apps/backend/internal/agent/runtime/routingerr/sanitize_test.go b/apps/backend/internal/agent/runtime/routingerr/sanitize_test.go index edb204ba08..261ff6624c 100644 --- a/apps/backend/internal/agent/runtime/routingerr/sanitize_test.go +++ b/apps/backend/internal/agent/runtime/routingerr/sanitize_test.go @@ -1,6 +1,8 @@ package routingerr import ( + "errors" + "fmt" "strings" "testing" ) @@ -103,6 +105,23 @@ func TestSanitize_Idempotent(t *testing.T) { } } +func TestSanitizeErrorRedactsMessageAndPreservesCause(t *testing.T) { + sentinel := errors.New("credential rejected") + raw := fmt.Errorf("token=ghp_abcdefghijklmnopqrstuvwxyz1234567890AB: %w", sentinel) + + got := SanitizeError(raw) + + if strings.Contains(got.Error(), "abcdefghijklmnopqrstuvwxyz") { + t.Fatalf("SanitizeError() exposed credential: %q", got) + } + if !errors.Is(got, sentinel) { + t.Fatal("SanitizeError() did not preserve wrapped cause") + } + if SanitizeError(got) != got { + t.Fatal("SanitizeError() is not idempotent") + } +} + func TestSanitize_TruncationBoundary(t *testing.T) { long := strings.Repeat("a", MaxRawExcerptBytes+500) got := Sanitize(long) diff --git a/apps/backend/internal/backendapp/adapters.go b/apps/backend/internal/backendapp/adapters.go index 5fad4244e1..bbbb45e29c 100644 --- a/apps/backend/internal/backendapp/adapters.go +++ b/apps/backend/internal/backendapp/adapters.go @@ -76,6 +76,17 @@ func (a *taskRepositoryAdapter) UpdateTaskStateIfSessionState( return a.svc.UpdateTaskStateIfSessionState(ctx, taskID, sessionID, expectedSessionState, state) } +func (a *taskRepositoryAdapter) UpdateTaskStateIfPrimarySessionState( + ctx context.Context, + taskID, sessionID string, + expectedSessionState models.TaskSessionState, + state v1.TaskState, +) (bool, error) { + return a.svc.UpdateTaskStateIfPrimarySessionState( + ctx, taskID, sessionID, expectedSessionState, state, + ) +} + // lifecycleAdapter adapts the lifecycle manager as an AgentManagerClient type lifecycleAdapter struct { mgr *lifecycle.Manager diff --git a/apps/backend/internal/github/auth_resolver.go b/apps/backend/internal/github/auth_resolver.go index ee531400ce..8040cfe897 100644 --- a/apps/backend/internal/github/auth_resolver.go +++ b/apps/backend/internal/github/auth_resolver.go @@ -52,6 +52,7 @@ type automationCredentialProvider interface { } type legacyCredentialFactory func(ctx context.Context) (Client, string, error) +type legacyTransportCredentialFactory func(ctx context.Context) (Client, string, string, error) type ghAccountTokenResolver func(ctx context.Context, host, login string) (string, error) type credentialCacheKey struct { @@ -80,6 +81,7 @@ type CredentialResolver struct { users userCredentialProvider automation automationCredentialProvider legacy legacyCredentialFactory + legacyGit legacyTransportCredentialFactory ghToken ghAccountTokenResolver now func() time.Time @@ -96,6 +98,9 @@ func NewCredentialResolver(connections workspaceConnectionReader, secrets authSe legacy: func(ctx context.Context) (Client, string, error) { return nil, AuthMethodNone, ErrGitHubNotConfigured }, + legacyGit: func(ctx context.Context) (Client, string, string, error) { + return nil, AuthMethodNone, "", ErrGitHubNotConfigured + }, ghToken: ResolveGHAccountToken, now: time.Now, cache: make(map[credentialCacheKey]*ResolvedCredential), @@ -109,6 +114,12 @@ func (r *CredentialResolver) SetLegacyFactory(factory legacyCredentialFactory) { } } +func (r *CredentialResolver) SetLegacyTransportFactory(factory legacyTransportCredentialFactory) { + if factory != nil { + r.legacyGit = factory + } +} + func (r *CredentialResolver) SetInstallationProvider(provider installationCredentialProvider) { r.app = provider } @@ -335,7 +346,7 @@ func (r *CredentialResolver) resolveAutomationSource( } return r.app.ResolveInstallation(ctx, connection, req) case ConnectionSourceLegacyShared: - return r.resolveLegacy(ctx, connection) + return r.resolveLegacy(ctx, connection, req.Purpose) default: return nil, fmt.Errorf("unsupported GitHub connection source %q", connection.Source) } @@ -392,18 +403,34 @@ func (r *CredentialResolver) resolveGHCLI(ctx context.Context, connection *Works }, nil } -func (r *CredentialResolver) resolveLegacy(ctx context.Context, connection *WorkspaceConnection) (*ResolvedCredential, error) { - client, method, err := r.legacy(ctx) +func (r *CredentialResolver) resolveLegacy( + ctx context.Context, + connection *WorkspaceConnection, + purpose CredentialPurpose, +) (*ResolvedCredential, error) { + var ( + client Client + method string + credential string + err error + ) + if purpose == CredentialPurposeGitTransport { + client, method, credential, err = r.legacyGit(ctx) + } else { + client, method, err = r.legacy(ctx) + } if err != nil { return nil, err } - if client == nil || method == AuthMethodNone { + if client == nil || method == AuthMethodNone || + (purpose == CredentialPurposeGitTransport && strings.TrimSpace(credential) == "") { return nil, ErrGitHubNotConfigured } login, _ := client.GetAuthenticatedUser(ctx) return &ResolvedCredential{ Client: client, Capabilities: allTokenCapabilities(), + credential: credential, Principal: AuthPrincipal{ Kind: AuthPrincipalHuman, Source: ConnectionSourceLegacyShared, diff --git a/apps/backend/internal/github/factory.go b/apps/backend/internal/github/factory.go index 8f8eb692c7..b4a30b4f19 100644 --- a/apps/backend/internal/github/factory.go +++ b/apps/backend/internal/github/factory.go @@ -23,34 +23,63 @@ type SecretListItem struct { HasValue bool `json:"has_value"` } +const legacyMockAuthMethod = "mock" + // NewClient creates a GitHub client using the best available auth method. // It tries the gh CLI first, then falls back to a PAT from the secret store. func NewClient(ctx context.Context, secrets SecretProvider, log *logger.Logger) (Client, string, error) { + client, method, _, err := newLegacyCredential(ctx, secrets, log, false) + return client, method, err +} + +// newLegacyGitTransportCredential resolves the same ambient credential used by +// migrated legacy_shared API access and additionally returns its raw token for +// in-memory Git transport use. The token is never exposed through Client. +func newLegacyGitTransportCredential( + ctx context.Context, + secrets SecretProvider, + log *logger.Logger, +) (Client, string, string, error) { + return newLegacyCredential(ctx, secrets, log, true) +} + +func newLegacyCredential( + ctx context.Context, + secrets SecretProvider, + log *logger.Logger, + includeTransportCredential bool, +) (Client, string, string, error) { // Mock client for E2E testing if os.Getenv("KANDEV_MOCK_GITHUB") == "true" { log.Info("using mock client for GitHub integration") - return NewMockClient(), "mock", nil + credential := "" + if includeTransportCredential { + credential = legacyMockAuthMethod + } + return NewMockClient(), legacyMockAuthMethod, credential, nil } - // Try gh CLI first - if GHAvailable() { - ghClient := NewGHClient() - ok, err := ghClient.IsAuthenticated(ctx) - if err == nil && ok { - log.Info("using gh CLI for GitHub integration") - return ghClient, "gh_cli", nil + // gh inherits GH_TOKEN/GITHUB_TOKEN for API calls, while exact-account + // token resolution deliberately strips them. For Git transport, resolve + // an ambient token directly so the API and clone identities cannot diverge. + // GH_TOKEN follows gh's documented precedence over GITHUB_TOKEN. + if includeTransportCredential { + if token := legacyEnvironmentToken(); token != "" { + log.Info("using environment PAT for legacy GitHub Git transport") + return NewPATClient(token), AuthMethodPAT, token, nil } - log.Debug("gh CLI available but not authenticated", zap.Error(err)) + } + + if client, method, credential, found, err := resolveLegacyGHCredential( + ctx, log, includeTransportCredential, + ); found || err != nil { + return client, method, credential, err } // Fall back to PAT from environment variable - if token := os.Getenv("GITHUB_TOKEN"); token != "" { - log.Info("using GITHUB_TOKEN from environment for GitHub integration") - return NewPATClient(token), AuthMethodPAT, nil - } - if token := os.Getenv("GH_TOKEN"); token != "" { - log.Info("using GH_TOKEN from environment for GitHub integration") - return NewPATClient(token), AuthMethodPAT, nil + if token := legacyEnvironmentToken(); token != "" { + log.Info("using environment PAT for GitHub integration") + return NewPATClient(token), AuthMethodPAT, transportCredential(token, includeTransportCredential), nil } // Fall back to PAT from secrets store @@ -58,14 +87,59 @@ func NewClient(ctx context.Context, secrets SecretProvider, log *logger.Logger) token, err := findGitHubPAT(ctx, secrets) if err == nil && token != "" { log.Info("using PAT from secrets store for GitHub integration") - return NewPATClient(token), AuthMethodPAT, nil + return NewPATClient(token), AuthMethodPAT, transportCredential(token, includeTransportCredential), nil } if err != nil { log.Debug("failed to find GitHub PAT in secrets", zap.Error(err)) } } - return &NoopClient{}, AuthMethodNone, nil + return &NoopClient{}, AuthMethodNone, "", nil +} + +func resolveLegacyGHCredential( + ctx context.Context, + log *logger.Logger, + includeTransportCredential bool, +) (Client, string, string, bool, error) { + if !GHAvailable() { + return nil, "", "", false, nil + } + + ghClient := NewGHClient() + ok, err := ghClient.IsAuthenticated(ctx) + if err != nil || !ok { + log.Debug("gh CLI available but not authenticated", zap.Error(err)) + return nil, "", "", false, nil + } + + log.Info("using gh CLI for GitHub integration") + if !includeTransportCredential { + return ghClient, string(ConnectionSourceGHCLI), "", true, nil + } + login, err := ghClient.GetAuthenticatedUser(ctx) + if err != nil { + return nil, AuthMethodNone, "", false, err + } + token, err := ResolveGHAccountToken(ctx, defaultGitHubHost, login) + if err != nil { + return nil, AuthMethodNone, "", false, err + } + return ghClient, string(ConnectionSourceGHCLI), token, true, nil +} + +func legacyEnvironmentToken() string { + if token := os.Getenv("GH_TOKEN"); token != "" { + return token + } + return os.Getenv("GITHUB_TOKEN") +} + +func transportCredential(token string, include bool) string { + if include { + return token + } + return "" } // findGitHubPAT looks for a secret named "GITHUB_TOKEN" or "github_token". diff --git a/apps/backend/internal/github/factory_test.go b/apps/backend/internal/github/factory_test.go index 9367f71d56..2e700cd652 100644 --- a/apps/backend/internal/github/factory_test.go +++ b/apps/backend/internal/github/factory_test.go @@ -2,6 +2,10 @@ package github import ( "context" + "errors" + "os" + "path/filepath" + "runtime" "testing" "github.com/kandev/kandev/internal/common/logger" @@ -49,3 +53,54 @@ func TestNewClient_NoAuth_ReturnsNoop(t *testing.T) { t.Errorf("expected *NoopClient, got %T", client) } } + +func TestNewLegacyGitTransportCredentialReturnsPAT(t *testing.T) { + t.Setenv("KANDEV_MOCK_GITHUB", "") + t.Setenv("PATH", t.TempDir()) + t.Setenv("GITHUB_TOKEN", "legacy-workspace-token") + t.Setenv("GH_TOKEN", "") + + client, method, credential, err := newLegacyGitTransportCredential( + context.Background(), nil, newTestLogger(), + ) + if err != nil { + t.Fatalf("newLegacyGitTransportCredential(): %v", err) + } + if method != AuthMethodPAT || credential != "legacy-workspace-token" { + t.Fatalf("method/credential = %q/%q, want PAT legacy-workspace-token", method, credential) + } + if _, ok := client.(*PATClient); !ok { + t.Fatalf("client = %T, want *PATClient", client) + } +} + +func TestNewLegacyGitTransportCredentialPrefersAmbientTokenOverStoredGH(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("test uses a POSIX gh fixture") + } + binDir := t.TempDir() + marker := filepath.Join(t.TempDir(), "gh-invoked") + ghPath := filepath.Join(binDir, "gh") + script := "#!/bin/sh\n: > \"$KANDEV_TEST_GH_MARKER\"\nexit 0\n" + if err := os.WriteFile(ghPath, []byte(script), 0o755); err != nil { + t.Fatalf("write fake gh: %v", err) + } + t.Setenv("KANDEV_MOCK_GITHUB", "") + t.Setenv("PATH", binDir) + t.Setenv("KANDEV_TEST_GH_MARKER", marker) + t.Setenv("GITHUB_TOKEN", "lower-priority-ambient-token") + t.Setenv("GH_TOKEN", "ambient-legacy-token") + + _, method, credential, err := newLegacyGitTransportCredential( + context.Background(), nil, newTestLogger(), + ) + if err != nil { + t.Fatalf("newLegacyGitTransportCredential(): %v", err) + } + if method != AuthMethodPAT || credential != "ambient-legacy-token" { + t.Fatalf("method/credential = %q/%q, want PAT ambient-legacy-token", method, credential) + } + if _, err := os.Stat(marker); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("stored gh account was consulted, stat error = %v", err) + } +} diff --git a/apps/backend/internal/github/gh_accounts.go b/apps/backend/internal/github/gh_accounts.go index 6f08df29ab..25801a2c61 100644 --- a/apps/backend/internal/github/gh_accounts.go +++ b/apps/backend/internal/github/gh_accounts.go @@ -9,6 +9,7 @@ import ( "os" "os/exec" "strings" + "sync" "github.com/kandev/kandev/internal/common/subproc" ) @@ -71,6 +72,16 @@ type ghAuthStatus struct { } `json:"hosts"` } +var cachedGHTokenUserSupport = sync.OnceValues(func() (bool, error) { + help, err := (systemGHAccountRunner{}).Run( + context.Background(), "auth", "token", "--help", + ) + if err != nil { + return false, err + } + return strings.Contains(help, "--user"), nil +}) + // ListGHAccounts returns every account known to gh without changing its // active account. Ambient token variables are stripped so they cannot hide // the accounts stored in gh's config. @@ -81,7 +92,15 @@ func ListGHAccounts(ctx context.Context) ([]GHAccount, error) { func listGHAccounts(ctx context.Context, runner ghAccountCommandRunner) ([]GHAccount, error) { raw, err := runner.Run(ctx, "auth", "status", "--json", "hosts") if err != nil { - return nil, fmt.Errorf("list gh accounts: %w", err) + raw, err = runner.Run(ctx, "auth", "status") + if err != nil { + return nil, fmt.Errorf("list gh accounts: %w", err) + } + accounts := parseLegacyGHAuthStatus(raw) + if len(accounts) == 0 { + return nil, errors.New("list gh accounts: unsupported gh auth status output") + } + return accounts, nil } var status ghAuthStatus if err := json.Unmarshal([]byte(raw), &status); err != nil { @@ -108,6 +127,52 @@ func listGHAccounts(ctx context.Context, runner ghAccountCommandRunner) ([]GHAcc return accounts, nil } +func parseLegacyGHAuthStatus(raw string) []GHAccount { + accounts := make([]GHAccount, 0) + host := "" + currentAccount := -1 + for _, line := range strings.Split(raw, "\n") { + trimmed := strings.TrimSpace(line) + if trimmed == line && trimmed != "" && !strings.ContainsAny(trimmed, " \t") { + host = trimmed + currentAccount = -1 + continue + } + if login := legacyStatusLogin(trimmed, host); login != "" { + accounts = append(accounts, GHAccount{ + Host: host, Login: login, Active: true, State: "success", + }) + currentAccount = len(accounts) - 1 + continue + } + if currentAccount >= 0 && strings.HasPrefix(trimmed, "- Active account:") { + accounts[currentAccount].Active = strings.EqualFold( + strings.TrimSpace(strings.TrimPrefix(trimmed, "- Active account:")), + "true", + ) + } + } + return accounts +} + +func legacyStatusLogin(line, host string) string { + if host == "" { + return "" + } + for _, marker := range []string{ + "Logged in to " + host + " account ", + "Logged in to " + host + " as ", + } { + if index := strings.Index(line, marker); index >= 0 { + fields := strings.Fields(line[index+len(marker):]) + if len(fields) > 0 { + return fields[0] + } + } + } + return "" +} + // ResolveGHAccountToken returns the token for an exact host/login pair. It // never changes gh's active account and never persists the returned token. func ResolveGHAccountToken(ctx context.Context, host, login string) (string, error) { @@ -126,7 +191,19 @@ func resolveGHAccountToken(ctx context.Context, runner ghAccountCommandRunner, h if login == "" { return "", fmt.Errorf("GitHub login is required") } - raw, err := runner.Run(ctx, "auth", "token", "--hostname", host, "--user", login) + supportsUser, helpErr := ghTokenUserSelectionSupported(ctx, runner) + if helpErr != nil { + return "", fmt.Errorf("inspect gh account-token support: %w", helpErr) + } + tokenArgs := []string{"auth", "token", "--hostname", host} + if supportsUser { + tokenArgs = append(tokenArgs, "--user", login) + } else { + if err := requireActiveLegacyGHAccount(ctx, runner, host, login); err != nil { + return "", err + } + } + raw, err := runner.Run(ctx, tokenArgs...) if err != nil { return "", fmt.Errorf("resolve gh account %s@%s: %w", login, host, err) } @@ -136,3 +213,37 @@ func resolveGHAccountToken(ctx context.Context, runner ghAccountCommandRunner, h } return token, nil } + +func ghTokenUserSelectionSupported(ctx context.Context, runner ghAccountCommandRunner) (bool, error) { + if _, ok := runner.(systemGHAccountRunner); ok { + return cachedGHTokenUserSupport() + } + help, err := runner.Run(ctx, "auth", "token", "--help") + if err != nil { + return false, err + } + return strings.Contains(help, "--user"), nil +} + +func requireActiveLegacyGHAccount( + ctx context.Context, + runner ghAccountCommandRunner, + host, login string, +) error { + accounts, err := listGHAccounts(ctx, runner) + if err != nil { + return fmt.Errorf("verify active gh account %s@%s: %w", login, host, err) + } + for _, account := range accounts { + if strings.EqualFold(account.Host, host) && + account.Login == login && + account.Active { + return nil + } + } + return fmt.Errorf( + "gh CLI cannot select account %s@%s; make it active or upgrade gh", + login, + host, + ) +} diff --git a/apps/backend/internal/github/gh_accounts_test.go b/apps/backend/internal/github/gh_accounts_test.go index bf6f22e62c..bbe88fab0d 100644 --- a/apps/backend/internal/github/gh_accounts_test.go +++ b/apps/backend/internal/github/gh_accounts_test.go @@ -2,6 +2,7 @@ package github import ( "context" + "errors" "reflect" "strings" "testing" @@ -37,9 +38,103 @@ func TestListGHAccountsParsesAllStoredLogins(t *testing.T) { } } -func TestResolveGHAccountTokenSelectsExactAccount(t *testing.T) { - runner := &fakeGHAccountRunner{output: "secret-token\n"} +type legacyGHAccountRunner struct { + output string + args [][]string +} +func (r *legacyGHAccountRunner) Run(_ context.Context, args ...string) (string, error) { + r.args = append(r.args, append([]string(nil), args...)) + if len(args) == 4 && args[2] == "--json" { + return "", errors.New("unknown flag: --json") + } + return r.output, nil +} + +func TestListGHAccountsSupportsLegacyStatusOutput(t *testing.T) { + tests := []struct { + name string + output string + want []GHAccount + }{ + { + name: "multi-account CLI without JSON output", + output: `github.com + ✓ Logged in to github.com account alice (/home/alice/.config/gh/hosts.yml) + - Active account: true + - Git operations protocol: ssh + + ✓ Logged in to github.com account work-bot (/home/alice/.config/gh/hosts.yml) + - Active account: false + - Git operations protocol: https +`, + want: []GHAccount{ + {Host: "github.com", Login: "alice", Active: true, State: "success"}, + {Host: "github.com", Login: "work-bot", State: "success"}, + }, + }, + { + name: "single-account CLI before account switching", + output: `github.com + ✓ Logged in to github.com as alice (/home/alice/.config/gh/hosts.yml) + ✓ Git operations for github.com configured to use ssh protocol. +`, + want: []GHAccount{ + {Host: "github.com", Login: "alice", Active: true, State: "success"}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + runner := &legacyGHAccountRunner{output: tt.output} + got, err := listGHAccounts(context.Background(), runner) + if err != nil { + t.Fatalf("listGHAccounts: %v", err) + } + if !reflect.DeepEqual(got, tt.want) { + t.Fatalf("accounts = %#v, want %#v", got, tt.want) + } + wantCalls := [][]string{ + {"auth", "status", "--json", "hosts"}, + {"auth", "status"}, + } + if !reflect.DeepEqual(runner.args, wantCalls) { + t.Fatalf("args = %#v, want %#v", runner.args, wantCalls) + } + }) + } +} + +func TestListGHAccountsRejectsUnrecognizedLegacyStatusOutput(t *testing.T) { + runner := &legacyGHAccountRunner{output: "authentication state unavailable"} + accounts, err := listGHAccounts(context.Background(), runner) + if err == nil { + t.Fatalf("listGHAccounts() = %#v, want parse error", accounts) + } + if strings.Contains(err.Error(), runner.output) { + t.Fatalf("error exposed raw gh auth output: %v", err) + } +} + +type modernGHTokenRunner struct { + tokenErr error + args [][]string +} + +func (r *modernGHTokenRunner) Run(_ context.Context, args ...string) (string, error) { + r.args = append(r.args, append([]string(nil), args...)) + if len(args) == 3 && args[2] == "--help" { + return " -u, --user string The account to output the token for\n", nil + } + if r.tokenErr != nil { + return "", r.tokenErr + } + return "secret-token\n", nil +} + +func TestResolveGHAccountTokenSelectsExactAccount(t *testing.T) { + runner := &modernGHTokenRunner{} token, err := resolveGHAccountToken(context.Background(), runner, "github.com", "work-bot") if err != nil { t.Fatalf("resolveGHAccountToken: %v", err) @@ -47,9 +142,106 @@ func TestResolveGHAccountTokenSelectsExactAccount(t *testing.T) { if token != "secret-token" { t.Fatalf("token = %q", token) } - wantArgs := []string{"auth", "token", "--hostname", "github.com", "--user", "work-bot"} - if !reflect.DeepEqual(runner.args[0], wantArgs) { - t.Fatalf("args = %#v, want %#v", runner.args[0], wantArgs) + wantCalls := [][]string{ + {"auth", "token", "--help"}, + {"auth", "token", "--hostname", "github.com", "--user", "work-bot"}, + } + if !reflect.DeepEqual(runner.args, wantCalls) { + t.Fatalf("args = %#v, want %#v", runner.args, wantCalls) + } +} + +type legacyGHTokenRunner struct { + args [][]string +} + +func (r *legacyGHTokenRunner) Run(_ context.Context, args ...string) (string, error) { + r.args = append(r.args, append([]string(nil), args...)) + if len(args) == 3 && args[2] == "--help" { + return " -h, --hostname string The hostname to output the token for\n", nil + } + if len(args) == 4 && args[2] == "--json" { + return "", errors.New("unknown flag: --json") + } + if len(args) == 2 && args[0] == "auth" && args[1] == "status" { + return `github.com + ✓ Logged in to github.com as alice (/home/alice/.config/gh/hosts.yml) +`, nil + } + if len(args) == 6 && args[4] == "--user" { + return "", errors.New("legacy CLI must not receive --user") + } + return "legacy-secret-token\n", nil +} + +func TestResolveGHAccountTokenSupportsLegacySingleAccountCLI(t *testing.T) { + runner := &legacyGHTokenRunner{} + token, err := resolveGHAccountToken(context.Background(), runner, "github.com", "alice") + if err != nil { + t.Fatalf("resolveGHAccountToken: %v", err) + } + if token != "legacy-secret-token" { + t.Fatalf("token = %q", token) + } + wantCalls := [][]string{ + {"auth", "token", "--help"}, + {"auth", "status", "--json", "hosts"}, + {"auth", "status"}, + {"auth", "token", "--hostname", "github.com"}, + } + if !reflect.DeepEqual(runner.args, wantCalls) { + t.Fatalf("args = %#v, want %#v", runner.args, wantCalls) + } +} + +func TestResolveGHAccountTokenRejectsInactiveAccountOnLegacyCLI(t *testing.T) { + runner := &legacyInactiveGHTokenRunner{} + _, err := resolveGHAccountToken(context.Background(), runner, "github.com", "work-bot") + if err == nil || !strings.Contains(err.Error(), "make it active or upgrade gh") { + t.Fatalf("resolveGHAccountToken() error = %v", err) + } + for _, args := range runner.args { + if reflect.DeepEqual(args, []string{"auth", "token", "--hostname", "github.com"}) { + t.Fatal("legacy CLI token requested for an inactive account") + } + } +} + +type legacyInactiveGHTokenRunner struct { + args [][]string +} + +func (r *legacyInactiveGHTokenRunner) Run(_ context.Context, args ...string) (string, error) { + r.args = append(r.args, append([]string(nil), args...)) + switch { + case len(args) == 3 && args[2] == "--help": + return " -h, --hostname string\n", nil + case len(args) == 4 && args[2] == "--json": + return "", errors.New("unknown flag: --json") + case len(args) == 2 && args[0] == "auth" && args[1] == "status": + return `github.com + ✓ Logged in to github.com account alice (/home/alice/.config/gh/hosts.yml) + - Active account: true + + ✓ Logged in to github.com account work-bot (/home/alice/.config/gh/hosts.yml) + - Active account: false +`, nil + default: + return "must-not-return-token", nil + } +} + +func TestResolveGHAccountTokenDoesNotFallBackAfterNamedAccountFailure(t *testing.T) { + runner := &modernGHTokenRunner{tokenErr: errors.New("account token unavailable")} + if _, err := resolveGHAccountToken(context.Background(), runner, "github.com", "work-bot"); err == nil { + t.Fatal("expected named account token failure") + } + wantCalls := [][]string{ + {"auth", "token", "--help"}, + {"auth", "token", "--hostname", "github.com", "--user", "work-bot"}, + } + if !reflect.DeepEqual(runner.args, wantCalls) { + t.Fatalf("args = %#v, want %#v", runner.args, wantCalls) } } diff --git a/apps/backend/internal/github/service.go b/apps/backend/internal/github/service.go index df364c16ce..34dc0f230c 100644 --- a/apps/backend/internal/github/service.go +++ b/apps/backend/internal/github/service.go @@ -209,6 +209,9 @@ func NewService(client Client, authMethod string, secrets SecretProvider, store service.resolver.SetLegacyFactory(func(ctx context.Context) (Client, string, error) { return NewClient(ctx, secrets, log) }) + service.resolver.SetLegacyTransportFactory(func(ctx context.Context) (Client, string, string, error) { + return newLegacyGitTransportCredential(ctx, secrets, log) + }) } return service } diff --git a/apps/backend/internal/github/service_git_transport_test.go b/apps/backend/internal/github/service_git_transport_test.go index 85712900e0..1481a16733 100644 --- a/apps/backend/internal/github/service_git_transport_test.go +++ b/apps/backend/internal/github/service_git_transport_test.go @@ -31,6 +31,124 @@ func TestResolveGitCredentialUsesWorkspacePAT(t *testing.T) { } } +func TestResolveGitCredentialUsesSelectedGHCLIAccount(t *testing.T) { + t.Parallel() + + connections := &fakeConnectionReader{workspaces: map[string]*WorkspaceConnection{ + "workspace-a": { + WorkspaceID: "workspace-a", Source: ConnectionSourceGHCLI, + GitHubHost: "github.com", Login: "automation-user", + Status: ConnectionStatusActive, CredentialGeneration: 1, + }, + }} + resolver := NewCredentialResolver(connections, nil) + resolver.ghToken = func(_ context.Context, host, login string) (string, error) { + if host != "github.com" || login != "automation-user" { + t.Fatalf("selected account = %s@%s", login, host) + } + return "selected-cli-token", nil + } + service := &Service{resolver: resolver} + + _, password, err := service.ResolveGitCredential( + context.Background(), "workspace-a", "github", "acme", "private", + ) + if err != nil { + t.Fatalf("ResolveGitCredential(): %v", err) + } + if password != "selected-cli-token" { + t.Fatalf("password = %q, want selected CLI credential", password) + } +} + +func TestResolveGitCredentialUsesMigratedLegacyCredential(t *testing.T) { + t.Parallel() + + connections := &fakeConnectionReader{workspaces: map[string]*WorkspaceConnection{ + "workspace-a": { + WorkspaceID: "workspace-a", Source: ConnectionSourceLegacyShared, + Status: ConnectionStatusActive, CredentialGeneration: 7, + }, + }} + resolver := NewCredentialResolver(connections, nil) + legacy := NewMockClient() + legacy.SetUser("legacy-user") + resolver.SetLegacyTransportFactory(func(context.Context) (Client, string, string, error) { + return legacy, AuthMethodPAT, "legacy-transport-token", nil + }) + service := &Service{resolver: resolver} + + username, password, err := service.ResolveGitCredential( + context.Background(), "workspace-a", "github", "acme", "private", + ) + if err != nil { + t.Fatalf("ResolveGitCredential(): %v", err) + } + if username != "x-access-token" || password != "legacy-transport-token" { + t.Fatalf("credential = %q/%q, want x-access-token/legacy-transport-token", username, password) + } +} + +func TestResolveGitCredentialRejectsMissingMigratedLegacyCredential(t *testing.T) { + t.Parallel() + + connections := &fakeConnectionReader{workspaces: map[string]*WorkspaceConnection{ + "workspace-a": { + WorkspaceID: "workspace-a", Source: ConnectionSourceLegacyShared, + Status: ConnectionStatusActive, CredentialGeneration: 7, + }, + }} + resolver := NewCredentialResolver(connections, nil) + resolver.SetLegacyTransportFactory(func(context.Context) (Client, string, string, error) { + return &NoopClient{}, AuthMethodNone, "", nil + }) + service := &Service{resolver: resolver} + + _, _, err := service.ResolveGitCredential( + context.Background(), "workspace-a", "github", "acme", "private", + ) + if !errors.Is(err, ErrGitHubNotConfigured) { + t.Fatalf("ResolveGitCredential() error = %v, want ErrGitHubNotConfigured", err) + } +} + +func TestResolveGitCredentialNeverUsesPersonalConnection(t *testing.T) { + t.Parallel() + + connections := &fakeConnectionReader{ + workspaces: map[string]*WorkspaceConnection{ + "workspace-a": { + WorkspaceID: "workspace-a", Source: ConnectionSourceLegacyShared, + Status: ConnectionStatusActive, CredentialGeneration: 7, + }, + }, + users: map[string]*UserConnection{ + "workspace-a:user-a": { + WorkspaceID: "workspace-a", UserID: "user-a", + Status: ConnectionStatusActive, + }, + }, + } + resolver := NewCredentialResolver(connections, nil) + resolver.SetUserProvider(failingUserCredentialProvider{t: t}) + legacy := NewMockClient() + legacy.SetUser("legacy-user") + resolver.SetLegacyTransportFactory(func(context.Context) (Client, string, string, error) { + return legacy, AuthMethodPAT, "automation-token", nil + }) + service := &Service{resolver: resolver} + + _, password, err := service.ResolveGitCredential( + context.Background(), "workspace-a", "github", "acme", "private", + ) + if err != nil { + t.Fatalf("ResolveGitCredential(): %v", err) + } + if password != "automation-token" { + t.Fatalf("password = %q, want automation credential", password) + } +} + func TestResolveGitCredentialRejectsAppWithoutGitRead(t *testing.T) { t.Parallel() @@ -63,6 +181,56 @@ func TestResolveGitCredentialRejectsAppWithoutGitRead(t *testing.T) { } } +func TestResolveGitCredentialUsesAppWithGitRead(t *testing.T) { + t.Parallel() + + installationID := int64(42) + connections := &fakeConnectionReader{workspaces: map[string]*WorkspaceConnection{ + "workspace-a": { + WorkspaceID: "workspace-a", Source: ConnectionSourceGitHubAppInstallation, + InstallationID: &installationID, AppRegistrationID: "registration-test", + Status: ConnectionStatusActive, CredentialGeneration: 1, + }, + }} + resolver := NewCredentialResolver(connections, nil) + resolver.SetAutomationProvider(staticTransportCredentialProvider{resolved: &ResolvedCredential{ + Client: NewMockClient(), + credential: "app-token", + Principal: AuthPrincipal{ + Kind: AuthPrincipalApp, Source: ConnectionSourceGitHubAppInstallation, + AppRegistrationID: "registration-test", + }, + AppRegistrationID: "registration-test", + Capabilities: map[GitHubAppCapability]bool{ + CapabilityGitRead: true, + }, + }}) + service := &Service{resolver: resolver} + + _, password, err := service.ResolveGitCredential( + context.Background(), "workspace-a", "github", "acme", "private", + ) + if err != nil { + t.Fatalf("ResolveGitCredential(): %v", err) + } + if password != "app-token" { + t.Fatalf("password = %q, want app token", password) + } +} + +type failingUserCredentialProvider struct { + t *testing.T +} + +func (p failingUserCredentialProvider) ResolveUser( + context.Context, + *UserConnection, + ResolveCredentialRequest, +) (*ResolvedCredential, error) { + p.t.Fatal("personal credential provider called for Git transport") + return nil, nil +} + type staticTransportCredentialProvider struct { resolved *ResolvedCredential } diff --git a/apps/backend/internal/orchestrator/event_handlers_runtime_state_race_test.go b/apps/backend/internal/orchestrator/event_handlers_runtime_state_race_test.go index 448b42b55d..995f5d200c 100644 --- a/apps/backend/internal/orchestrator/event_handlers_runtime_state_race_test.go +++ b/apps/backend/internal/orchestrator/event_handlers_runtime_state_race_test.go @@ -64,3 +64,43 @@ func TestReconcileTaskStateForRuntime_RejectsClarificationRace(t *testing.T) { require.NoError(t, err) require.Equal(t, models.TaskSessionStateWaitingForInput, session.State) } + +func TestReconcileTaskStateForRuntime_AllowsWorkingNonPrimarySession(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "task-runtime-secondary", "session-runtime-secondary", "") + require.NoError(t, repo.UpdateTaskState( + ctx, "task-runtime-secondary", v1.TaskStateScheduling, + )) + require.NoError(t, repo.UpdateTaskSessionState( + ctx, "session-runtime-secondary", models.TaskSessionStateStarting, "", + )) + session, err := repo.GetTaskSession(ctx, "session-runtime-secondary") + require.NoError(t, err) + require.False(t, session.IsPrimary) + + taskRepo := newMockTaskRepo() + taskRepo.updateIfSessionState = func( + ctx context.Context, + taskID, sessionID string, + expectedSessionState models.TaskSessionState, + state v1.TaskState, + ) (bool, error) { + _, updated, updateErr := repo.UpdateTaskStateIfSessionState( + ctx, taskID, sessionID, expectedSessionState, state, + ) + return updated, updateErr + } + svc := createTestService(repo, newMockStepGetter(), taskRepo) + + require.NoError(t, svc.reconcileTaskStateForRuntime( + ctx, + "task-runtime-secondary", + "session-runtime-secondary", + v1.TaskStateInProgress, + )) + + task, err := repo.GetTask(ctx, "task-runtime-secondary") + require.NoError(t, err) + require.Equal(t, v1.TaskStateInProgress, task.State) +} diff --git a/apps/backend/internal/orchestrator/executor/executor.go b/apps/backend/internal/orchestrator/executor/executor.go index f79580bd0a..399c9a0254 100644 --- a/apps/backend/internal/orchestrator/executor/executor.go +++ b/apps/backend/internal/orchestrator/executor/executor.go @@ -85,6 +85,16 @@ type executorStore interface { GetTaskPlan(ctx context.Context, taskID string) (*models.TaskPlan, error) } +type primarySessionTaskStateStore interface { + UpdateTaskStateIfPrimarySessionState( + context.Context, + string, + string, + models.TaskSessionState, + v1.TaskState, + ) (v1.TaskState, bool, error) +} + // Common errors const defaultBaseBranch = "main" @@ -641,6 +651,9 @@ type Executor struct { // Session-aware task state callback used after agent-process start settles. onTaskRuntimeStateReconcile TaskRuntimeStateReconcileFunc + // Primary-session-aware task state callback used for failures before + // AgentManager.LaunchAgent owns failure bookkeeping. + onEarlyLaunchTaskStateReconcile TaskRuntimeStateReconcileFunc // Callback for session state changes that need event publishing. // Set by the orchestrator to route through updateTaskSessionState which @@ -799,6 +812,12 @@ func (e *Executor) SetOnTaskRuntimeStateReconcile(fn TaskRuntimeStateReconcileFu e.onTaskRuntimeStateReconcile = fn } +// SetOnEarlyLaunchTaskStateReconcile sets the primary-session-aware task-state +// callback for environment-preparation failures. +func (e *Executor) SetOnEarlyLaunchTaskStateReconcile(fn TaskRuntimeStateReconcileFunc) { + e.onEarlyLaunchTaskStateReconcile = fn +} + // SetOnSessionStateChange sets a callback for session state changes. // This allows the orchestrator to route state changes through updateTaskSessionState // which updates the DB and publishes WebSocket events to the frontend. diff --git a/apps/backend/internal/orchestrator/executor/executor_execute.go b/apps/backend/internal/orchestrator/executor/executor_execute.go index 33e1780f70..5185130227 100644 --- a/apps/backend/internal/orchestrator/executor/executor_execute.go +++ b/apps/backend/internal/orchestrator/executor/executor_execute.go @@ -13,6 +13,7 @@ import ( "github.com/google/uuid" "github.com/kandev/kandev/internal/agent/runtime/lifecycle" + "github.com/kandev/kandev/internal/agent/runtime/routingerr" "github.com/kandev/kandev/internal/orchestrator/sessionstate" "github.com/kandev/kandev/internal/repoclone" "github.com/kandev/kandev/internal/sysprompt" @@ -582,14 +583,22 @@ func (e *Executor) ExecuteWithFullProfile(ctx context.Context, task *v1.Task, ag return nil, err } - // Launch the agent for the prepared session - return e.LaunchPreparedSession(ctx, task, sessionID, LaunchOptions{ + // Launch the agent for the prepared session. + execution, err := e.LaunchPreparedSession(ctx, task, sessionID, LaunchOptions{ AgentProfileID: agentProfileID, ExecutorID: executorID, Prompt: prompt, WorkflowStepID: workflowStepID, StartAgent: true, }) + if err != nil { + // LaunchAgent errors are already persisted by LaunchPreparedSession. + // Calling the session-aware recorder again is a no-op for those errors + // and covers failures from earlier environment preparation without + // letting a superseded session fail the task. + return nil, e.handleEarlyLaunchFailure(ctx, task.ID, sessionID, "", err) + } + return execution, nil } // PrepareSession creates a session entry in the database without launching the agent. @@ -979,28 +988,15 @@ func resumeTokenForExecutionProfile(running *models.ExecutorRunning, profileID s return running.ResumeToken } -// handleLaunchFailure marks the session and task as FAILED and returns the original error. +// handleLaunchFailure marks the session and task as FAILED and returns a +// sanitized error that preserves the original cause for errors.Is/errors.As. func (e *Executor) handleLaunchFailure(ctx context.Context, taskID, sessionID, repositoryID string, launchErr error) error { // Detach from caller context so failure bookkeeping completes even if the // original request context was cancelled. failCtx := context.WithoutCancel(ctx) - e.logger.Error("failed to launch agent", - zap.String("task_id", taskID), - zap.Error(launchErr)) - var onChanged func() - if e.onLaunchFailed != nil { - onChanged = func() { - e.onLaunchFailed(failCtx, taskID, sessionID, repositoryID, launchErr) - } - } - changed, _, updateErr := e.transitionSessionStateWithHook( - failCtx, taskID, sessionID, models.TaskSessionStateFailed, launchErr.Error(), onChanged, + safeErr, changed := e.transitionLaunchFailure( + failCtx, taskID, sessionID, repositoryID, launchErr, ) - if updateErr != nil { - e.logger.Warn("failed to mark session as failed after launch error", - zap.String("session_id", sessionID), - zap.Error(updateErr)) - } if changed { if updateErr := e.updateTaskState(failCtx, taskID, v1.TaskStateFailed); updateErr != nil { e.logger.Warn("failed to mark task as failed after launch error", @@ -1008,7 +1004,86 @@ func (e *Executor) handleLaunchFailure(ctx context.Context, taskID, sessionID, r zap.Error(updateErr)) } } - return launchErr + return safeErr +} + +func (e *Executor) handleEarlyLaunchFailure( + ctx context.Context, + taskID, sessionID, repositoryID string, + launchErr error, +) error { + failCtx := context.WithoutCancel(ctx) + safeErr, changed := e.transitionLaunchFailure( + failCtx, taskID, sessionID, repositoryID, launchErr, + ) + if !changed { + return safeErr + } + if e.onEarlyLaunchTaskStateReconcile != nil { + if err := e.onEarlyLaunchTaskStateReconcile( + failCtx, taskID, sessionID, v1.TaskStateFailed, + ); err != nil { + e.logger.Warn("failed to reconcile task after early launch error", + zap.String("task_id", taskID), + zap.String("session_id", sessionID), + zap.Error(err)) + } + return safeErr + } + updater, ok := e.repo.(primarySessionTaskStateStore) + if !ok { + // Older repository adapters may not expose the primary-aware extension. + // Preserve terminal failure behavior while those adapters migrate; the + // production adapter takes the guarded path above. + if _, _, err := e.repo.UpdateTaskStateIfNotArchived( + failCtx, taskID, v1.TaskStateFailed, + ); err != nil { + e.logger.Warn("failed to mark task after early launch error", + zap.String("task_id", taskID), + zap.String("session_id", sessionID), + zap.Error(err)) + } + return safeErr + } + if _, _, err := updater.UpdateTaskStateIfPrimarySessionState( + failCtx, + taskID, + sessionID, + models.TaskSessionStateFailed, + v1.TaskStateFailed, + ); err != nil { + e.logger.Warn("failed to mark task as failed after early launch error", + zap.String("task_id", taskID), + zap.String("session_id", sessionID), + zap.Error(err)) + } + return safeErr +} + +func (e *Executor) transitionLaunchFailure( + failCtx context.Context, + taskID, sessionID, repositoryID string, + launchErr error, +) (error, bool) { + safeErr := routingerr.SanitizeError(launchErr) + e.logger.Error("failed to launch agent", + zap.String("task_id", taskID), + zap.Error(safeErr)) + var onChanged func() + if e.onLaunchFailed != nil { + onChanged = func() { + e.onLaunchFailed(failCtx, taskID, sessionID, repositoryID, safeErr) + } + } + changed, _, updateErr := e.transitionSessionStateWithHook( + failCtx, taskID, sessionID, models.TaskSessionStateFailed, safeErr.Error(), onChanged, + ) + if updateErr != nil { + e.logger.Warn("failed to mark session as failed after launch error", + zap.String("session_id", sessionID), + zap.Error(updateErr)) + } + return safeErr, changed } // finalizeLaunch persists launch state and returns the resulting TaskExecution. diff --git a/apps/backend/internal/orchestrator/executor/executor_mocks_test.go b/apps/backend/internal/orchestrator/executor/executor_mocks_test.go index 31c312d1a7..423a097b3c 100644 --- a/apps/backend/internal/orchestrator/executor/executor_mocks_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_mocks_test.go @@ -265,6 +265,7 @@ type mockRepository struct { // Optional hook to inject behavior into GetTaskSession (e.g. simulate a // transient DB error); if nil, the default map lookup is used. getTaskSessionFunc func(ctx context.Context, id string) (*models.TaskSession, error) + getTaskEnvironmentByTaskIDFunc func(ctx context.Context, taskID string) (*models.TaskEnvironment, error) createTaskSessionFunc func(ctx context.Context, session *models.TaskSession) error updateTaskSessionStateFunc func(ctx context.Context, sessionID string, state models.TaskSessionState, errorMessage string) error listActiveTaskSessionsByTaskIDFunc func(ctx context.Context, taskID string) ([]*models.TaskSession, error) @@ -483,9 +484,34 @@ func (m *mockRepository) SetSessionPrimary(ctx context.Context, sessionID string m.mu.Lock() defer m.mu.Unlock() m.setSessionPrimaryCalls = append(m.setSessionPrimaryCalls, sessionID) + for _, session := range m.sessions { + session.IsPrimary = session.ID == sessionID + } return nil } +func (m *mockRepository) UpdateTaskStateIfPrimarySessionState( + _ context.Context, + taskID, sessionID string, + expectedSessionState models.TaskSessionState, + state v1.TaskState, +) (v1.TaskState, bool, error) { + m.mu.Lock() + defer m.mu.Unlock() + task := m.tasks[taskID] + session := m.sessions[sessionID] + if task == nil || session == nil { + return "", false, nil + } + oldState := task.State + if task.ArchivedAt != nil || session.TaskID != taskID || + session.State != expectedSessionState || !session.IsPrimary { + return oldState, false, nil + } + task.State = state + return oldState, true, nil +} + func (m *mockRepository) UpdateTaskSessionState(ctx context.Context, sessionID string, state models.TaskSessionState, errorMessage string) error { m.mu.Lock() fn := m.updateTaskSessionStateFunc @@ -1042,7 +1068,10 @@ func (m *mockRepository) GetTaskEnvironment(_ context.Context, id string) (*mode } return nil, nil } -func (m *mockRepository) GetTaskEnvironmentByTaskID(_ context.Context, taskID string) (*models.TaskEnvironment, error) { +func (m *mockRepository) GetTaskEnvironmentByTaskID(ctx context.Context, taskID string) (*models.TaskEnvironment, error) { + if m.getTaskEnvironmentByTaskIDFunc != nil { + return m.getTaskEnvironmentByTaskIDFunc(ctx, taskID) + } for _, env := range m.taskEnvironments { if env.TaskID == taskID { return env, nil diff --git a/apps/backend/internal/orchestrator/executor/executor_test.go b/apps/backend/internal/orchestrator/executor/executor_test.go index f6cba060a2..2b5f46b021 100644 --- a/apps/backend/internal/orchestrator/executor/executor_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "os/exec" + "strings" "sync" "sync/atomic" "testing" @@ -1264,6 +1265,95 @@ func TestExecuteWithProfile_UsesPrepareThenLaunch(t *testing.T) { } } +func TestExecuteWithProfile_PersistsEarlyLaunchFailure(t *testing.T) { + repo := newMockRepository() + sentinel := errors.New("GitHub is not configured") + const secret = "ghp_abcdefghijklmnopqrstuvwxyz1234567890AB" + repo.getTaskEnvironmentByTaskIDFunc = func( + context.Context, + string, + ) (*models.TaskEnvironment, error) { + return nil, fmt.Errorf("resolve workspace Git credential: token=%s: %w", secret, sentinel) + } + agentManager := &mockAgentManager{} + executor := newTestExecutor(t, agentManager, repo) + task := &v1.Task{ + ID: "task-early-failure", WorkspaceID: "workspace-123", + Title: "Test Task", Description: "Test description", + } + repo.tasks[task.ID] = &models.Task{ + ID: task.ID, WorkspaceID: task.WorkspaceID, State: v1.TaskStateScheduling, + } + + _, err := executor.ExecuteWithProfile( + context.Background(), task, "profile-123", "", "test prompt", "", + ) + if err == nil || !errors.Is(err, sentinel) { + t.Fatalf("ExecuteWithProfile() error = %v, want GitHub credential failure", err) + } + if strings.Contains(err.Error(), secret) { + t.Fatalf("ExecuteWithProfile() returned raw credential: %v", err) + } + if len(repo.createTaskSessionCalls) != 1 { + t.Fatalf("created sessions = %d, want 1", len(repo.createTaskSessionCalls)) + } + sessionID := repo.createTaskSessionCalls[0].ID + session := repo.sessions[sessionID] + if session.State != models.TaskSessionStateFailed { + t.Fatalf("session state = %s, want FAILED", session.State) + } + if strings.Contains(session.ErrorMessage, secret) { + t.Fatalf("persisted session error contains raw credential: %q", session.ErrorMessage) + } + if repo.tasks[task.ID].State != v1.TaskStateFailed { + t.Fatalf("task state = %s, want FAILED", repo.tasks[task.ID].State) + } +} + +func TestExecuteWithProfile_EarlyFailureDoesNotFailSupersededPrimary(t *testing.T) { + repo := newMockRepository() + repo.getTaskEnvironmentByTaskIDFunc = func( + context.Context, + string, + ) (*models.TaskEnvironment, error) { + repo.mu.Lock() + repo.sessions["session-new"] = &models.TaskSession{ + ID: "session-new", TaskID: "task-superseded", + State: models.TaskSessionStateRunning, + } + repo.mu.Unlock() + if err := repo.SetSessionPrimary(context.Background(), "session-new"); err != nil { + t.Fatalf("SetSessionPrimary: %v", err) + } + return nil, errors.New("resolve workspace Git credential: GitHub is not configured") + } + executor := newTestExecutor(t, &mockAgentManager{}, repo) + task := &v1.Task{ + ID: "task-superseded", WorkspaceID: "workspace-123", + Title: "Test Task", Description: "Test description", + } + repo.tasks[task.ID] = &models.Task{ + ID: task.ID, WorkspaceID: task.WorkspaceID, State: v1.TaskStateScheduling, + } + + _, err := executor.ExecuteWithProfile( + context.Background(), task, "profile-123", "", "test prompt", "", + ) + if err == nil { + t.Fatal("ExecuteWithProfile() succeeded, want early launch failure") + } + oldSessionID := repo.createTaskSessionCalls[0].ID + if repo.sessions[oldSessionID].State != models.TaskSessionStateFailed { + t.Fatalf("old session state = %s, want FAILED", repo.sessions[oldSessionID].State) + } + if repo.sessions["session-new"].State != models.TaskSessionStateRunning { + t.Fatalf("new primary state = %s, want RUNNING", repo.sessions["session-new"].State) + } + if repo.tasks[task.ID].State != v1.TaskStateScheduling { + t.Fatalf("task state = %s, want SCHEDULING", repo.tasks[task.ID].State) + } +} + func TestShouldUseWorktree(t *testing.T) { tests := []struct { executorType string diff --git a/apps/backend/internal/orchestrator/scheduler/scheduler.go b/apps/backend/internal/orchestrator/scheduler/scheduler.go index 0fd8f8a0d3..161fb3fdac 100644 --- a/apps/backend/internal/orchestrator/scheduler/scheduler.go +++ b/apps/backend/internal/orchestrator/scheduler/scheduler.go @@ -55,7 +55,7 @@ type TaskRepository interface { // need the archived_at IS NULL guarantee. Returns whether a row was // modified. UpdateTaskStateIfNotArchived(ctx context.Context, taskID string, state v1.TaskState) (bool, error) - // UpdateTaskStateIfSessionState additionally pins the owning session's + // UpdateTaskStateIfSessionState additionally pins the named session's // current state, closing races with clarification and terminal transitions. UpdateTaskStateIfSessionState( ctx context.Context, diff --git a/apps/backend/internal/orchestrator/service.go b/apps/backend/internal/orchestrator/service.go index 3ca66e7f58..fea0d78c7b 100644 --- a/apps/backend/internal/orchestrator/service.go +++ b/apps/backend/internal/orchestrator/service.go @@ -15,12 +15,14 @@ import ( "errors" "fmt" "regexp" + "strconv" "strings" "sync" "time" "go.uber.org/zap" + "github.com/kandev/kandev/internal/agent/runtime/routingerr" "github.com/kandev/kandev/internal/agentctl/types/streams" "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/events" @@ -633,6 +635,7 @@ func NewService( return nil }) exec.SetOnTaskRuntimeStateReconcile(s.reconcileTaskStateForRuntime) + exec.SetOnEarlyLaunchTaskStateReconcile(s.reconcileTaskStateForEarlyLaunchFailure) exec.SetOnSessionStateChange(func(ctx context.Context, taskID, sessionID string, state models.TaskSessionState, errorMessage string) error { s.updateTaskSessionState(ctx, taskID, sessionID, state, errorMessage, true) return nil @@ -1691,10 +1694,16 @@ func canResumeRunning(running *models.ExecutorRunning) bool { } func isMissingBranchError(err error) bool { - if err == nil { - return false + for current := err; current != nil; current = errors.Unwrap(current) { + if isMissingBranchMessage(current.Error()) { + return true + } } - msg := strings.ToLower(err.Error()) + return false +} + +func isMissingBranchMessage(message string) bool { + msg := strings.ToLower(message) if strings.Contains(msg, "couldn't find remote ref") { return true } @@ -1724,20 +1733,22 @@ var ( const launchFailurePRLookupTimeout = time.Second func extractMissingBranchName(err error) string { - if err == nil { - return "" + branch := "" + for current := err; current != nil; current = errors.Unwrap(current) { + msg := current.Error() + if match := quotedBranchPattern.FindStringSubmatch(msg); len(match) == 2 { + branch = strings.TrimSpace(match[1]) + continue + } + if match := remoteRefPattern.FindStringSubmatch(msg); len(match) == 2 { + branch = strings.TrimSpace(match[1]) + continue + } + if match := pathspecBranchPattern.FindStringSubmatch(msg); len(match) == 2 { + branch = strings.TrimSpace(match[1]) + } } - msg := err.Error() - if match := quotedBranchPattern.FindStringSubmatch(msg); len(match) == 2 { - return strings.TrimSpace(match[1]) - } - if match := remoteRefPattern.FindStringSubmatch(msg); len(match) == 2 { - return strings.TrimSpace(match[1]) - } - if match := pathspecBranchPattern.FindStringSubmatch(msg); len(match) == 2 { - return strings.TrimSpace(match[1]) - } - return "" + return branch } const missingPRBranchRecoveryClaimKey = "missing_pr_branch_recovery_claimed" @@ -1821,6 +1832,85 @@ func (s *Service) hasOnlyTaskRepository(ctx context.Context, taskID, repositoryI strings.TrimSpace(repositories[0].RepositoryID) == repositoryID } +// repositoryForMissingBranchFailure derives the failed repository from the +// task's checkout configuration when environment preparation failed before the +// executor constructed its repository-scoped launch request. It deliberately +// returns no match for an empty or ambiguous branch so destructive recovery +// actions remain scoped to the repository that actually failed. +func (s *Service) repositoryForMissingBranchFailure(ctx context.Context, taskID, branch string) (string, string) { + branch = strings.TrimSpace(branch) + if branch == "" { + return "", branch + } + store, ok := s.repo.(interface { + ListTaskRepositories(context.Context, string) ([]*models.TaskRepository, error) + }) + if !ok { + return "", branch + } + repositories, err := store.ListTaskRepositories(ctx, taskID) + if err != nil { + s.logger.Debug("failed to load task repositories for missing-branch guidance", + zap.String("task_id", taskID), zap.Error(err)) + return "", branch + } + + prNumber := missingBranchPRNumber(branch) + var repositoryID string + resolvedBranch := branch + for _, repository := range repositories { + if repository == nil || !taskRepositoryMatchesMissingBranch(repository, branch, prNumber) { + continue + } + if repositoryID != "" { + return "", branch + } + repositoryID = strings.TrimSpace(repository.RepositoryID) + if checkoutBranch := strings.TrimSpace(repository.CheckoutBranch); checkoutBranch != "" { + resolvedBranch = checkoutBranch + } + } + return repositoryID, resolvedBranch +} + +var missingBranchPRRefPattern = regexp.MustCompile(`^pull/(\d+)/head$`) + +func missingBranchPRNumber(branch string) int { + match := missingBranchPRRefPattern.FindStringSubmatch(strings.TrimSpace(branch)) + if len(match) != 2 { + return 0 + } + number, err := strconv.Atoi(match[1]) + if err != nil || number <= 0 { + return 0 + } + return number +} + +func taskRepositoryMatchesMissingBranch(repository *models.TaskRepository, branch string, prNumber int) bool { + if strings.TrimSpace(repository.CheckoutBranch) == branch { + return true + } + return prNumber > 0 && taskRepositoryPRNumber(repository.Metadata) == prNumber +} + +func taskRepositoryPRNumber(metadata map[string]interface{}) int { + if metadata == nil { + return 0 + } + switch value := metadata["pr_number"].(type) { + case int: + return value + case int64: + return int(value) + case float64: + if value > 0 && value == float64(int(value)) { + return int(value) + } + } + return 0 +} + // handleSessionLaunchFailed creates branch guidance only after the caller has // persisted FAILED. The claim is stored on the session because prepare, start, // and resume may race. @@ -1829,8 +1919,17 @@ func (s *Service) handleSessionLaunchFailed(ctx context.Context, taskID, session return } recoveryCtx := context.WithoutCancel(ctx) - branch := extractMissingBranchName(launchErr) - prState := s.matchingTaskPRState(recoveryCtx, taskID, repositoryID, branch) + rawBranch := extractMissingBranchName(launchErr) + displayBranch := routingerr.Sanitize(rawBranch) + resolvedRepositoryID, resolvedBranch := s.repositoryForMissingBranchFailure(recoveryCtx, taskID, rawBranch) + if repositoryID == "" { + repositoryID = resolvedRepositoryID + } + if repositoryID != "" && repositoryID == resolvedRepositoryID { + rawBranch = resolvedBranch + displayBranch = routingerr.Sanitize(resolvedBranch) + } + prState := s.matchingTaskPRState(recoveryCtx, taskID, repositoryID, rawBranch) if prState == githubPRStateOpen { return } @@ -1851,7 +1950,9 @@ func (s *Service) handleSessionLaunchFailed(ctx context.Context, taskID, session if !claimed { return } - if err := s.createMissingPRBranchRecoveryMessage(recoveryCtx, taskID, sessionID, branch, prState); err != nil { + if err := s.createMissingPRBranchRecoveryMessage( + recoveryCtx, taskID, sessionID, displayBranch, prState, + ); err != nil { releaser, ok := s.repo.(failedSessionMetadataClaimReleaser) if !ok { s.logger.Warn("session repository cannot release missing-branch recovery claim after message failure", diff --git a/apps/backend/internal/orchestrator/task_launch_failure_test.go b/apps/backend/internal/orchestrator/task_launch_failure_test.go index d44dc8a8bc..5ca1072975 100644 --- a/apps/backend/internal/orchestrator/task_launch_failure_test.go +++ b/apps/backend/internal/orchestrator/task_launch_failure_test.go @@ -315,6 +315,62 @@ func TestHandleSessionLaunchFailed_ClosedOrMergedMatchingPRCreatesMissingBranchG } } +func TestHandleSessionLaunchFailure_ResolvesRepositoryForEarlyMissingBranchFailure(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedTaskAndSession(t, repo, "task1", "session1", models.TaskSessionStateCreated) + if err := repo.CreateRepository(ctx, &models.Repository{ + ID: "repo-a", WorkspaceID: "ws1", Name: "repo-a", SourceType: "local", + }); err != nil { + t.Fatalf("create repository: %v", err) + } + if err := repo.CreateTaskRepository(ctx, &models.TaskRepository{ + ID: "task-repo-a", + TaskID: "task1", + RepositoryID: "repo-a", + CheckoutBranch: "feature/already-merged-and-deleted", + Position: 0, + Metadata: map[string]interface{}{"pr_number": 999}, + }); err != nil { + t.Fatalf("create task repository: %v", err) + } + + messages := &mockMessageCreator{} + svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) + svc.messageCreator = messages + svc.SetGitHubService(&mockGitHubService{taskPRs: []*github.TaskPR{ + {TaskID: "task1", RepositoryID: "repo-a", HeadBranch: "feature/already-merged-and-deleted", State: githubPRStateClosed}, + }}) + + _ = svc.handleSessionLaunchFailure( + ctx, + "task1", + "session1", + errors.New("environment preparation failed: branch \"feature/already-merged-and-deleted\" not found locally or on remote: fatal: couldn't find remote ref pull/999/head"), + ) + + session, err := repo.GetTaskSession(ctx, "session1") + if err != nil { + t.Fatalf("get failed session: %v", err) + } + if session.State != models.TaskSessionStateFailed { + t.Fatalf("session state = %q, want FAILED", session.State) + } + if strings.Contains(session.ErrorMessage, "feature/already-merged-and-deleted") { + t.Fatalf("persisted error exposed the raw branch: %q", session.ErrorMessage) + } + + if len(messages.sessionMessages) != 1 { + t.Fatalf("expected one missing-branch recovery message, got %d", len(messages.sessionMessages)) + } + if kind := messages.sessionMessages[0].metadata["failure_kind"]; kind != "missing_pr_branch" { + t.Fatalf("failure_kind = %#v, want missing_pr_branch", kind) + } + if branch := messages.sessionMessages[0].metadata["missing_branch"]; branch != "***" { + t.Fatalf("missing_branch = %#v, want sanitized branch", branch) + } +} + func TestHandleSessionLaunchFailed_UnrelatedOpenPRDoesNotSuppressNeutralGuidance(t *testing.T) { ctx := context.Background() repo := setupTestRepo(t) diff --git a/apps/backend/internal/orchestrator/task_operations.go b/apps/backend/internal/orchestrator/task_operations.go index 07ba2723c6..c73294b94d 100644 --- a/apps/backend/internal/orchestrator/task_operations.go +++ b/apps/backend/internal/orchestrator/task_operations.go @@ -63,6 +63,15 @@ var ErrAgentPromptInProgress = errors.New("agent is currently processing a promp var ErrAgentNotReadyForPrompt = errors.New("agent not ready for prompt") var ErrSessionResetInProgress = errors.New("session reset in progress") +type primarySessionTaskStateUpdater interface { + UpdateTaskStateIfPrimarySessionState( + ctx context.Context, + taskID, sessionID string, + expectedSessionState models.TaskSessionState, + state v1.TaskState, + ) (bool, error) +} + // ErrSessionNotPromptable is returned when a session cannot accept a prompt // because of its lifecycle state (STARTING, CREATED, FAILED, CANCELLED). // Distinct from ErrAgentPromptInProgress, which is RUNNING-only — confusing @@ -261,7 +270,7 @@ func (s *Service) PrepareTaskSession(ctx context.Context, taskID string, agentPr // LaunchAgent failures persist FAILED in the executor. Earlier // workspace failures return here and are recorded through the same // session-level recovery claim. - s.handleEarlyMissingPRBranchLaunchFailure(bgCtx, taskID, sessionID, launchErr) + launchErr = s.handleSessionLaunchFailure(bgCtx, taskID, sessionID, launchErr) s.logger.Warn("failed to launch workspace for prepared session (file browsing may be unavailable)", zap.String("task_id", taskID), zap.String("session_id", sessionID), @@ -483,8 +492,7 @@ func (s *Service) StartCreatedSession( if err != nil { // The executor persists LaunchAgent failures. Cover earlier prepared-session // failures here; the session-level claim makes either completion order safe. - s.handleEarlyMissingPRBranchLaunchFailure(ctx, taskID, sessionID, err) - return nil, err + return nil, s.handleSessionLaunchFailure(ctx, taskID, sessionID, err) } // Record the initial user message and set plan mode metadata after launch. @@ -500,19 +508,21 @@ func (s *Service) StartCreatedSession( return execution, nil } -// handleEarlyMissingPRBranchLaunchFailure covers failures that occur before -// Executor reaches AgentManager.LaunchAgent. Those failures do not run the -// executor's launch-failure bookkeeping, so this fallback owns the terminal -// transition and lets the shared persisted claim create guidance once. -func (s *Service) handleEarlyMissingPRBranchLaunchFailure( +// handleSessionLaunchFailure covers launch and resume failures that have not +// already won terminal bookkeeping. The state CAS makes it safe as an +// idempotent fallback when executor bookkeeping did run. +func (s *Service) handleSessionLaunchFailure( ctx context.Context, taskID, sessionID string, launchErr error, -) { - if !isMissingBranchError(launchErr) { - return - } - s.recordSessionLaunchFailure(ctx, taskID, sessionID, launchErr) + preloadedSession ...*models.TaskSession, +) error { + failureCtx := context.WithoutCancel(ctx) + safeErr := routingerr.SanitizeError(launchErr) + s.recordSessionLaunchFailure( + failureCtx, taskID, sessionID, safeErr, preloadedSession..., + ) + return safeErr } // recordSessionLaunchFailure transitions a still-active session to FAILED, @@ -534,9 +544,7 @@ func (s *Service) recordSessionLaunchFailure(ctx context.Context, taskID, sessio } return } - updated, err := s.taskRepo.UpdateTaskStateIfSessionState( - ctx, taskID, sessionID, models.TaskSessionStateFailed, v1.TaskStateFailed, - ) + updated, err := s.updateTaskStateForEarlyLaunchFailure(ctx, taskID, sessionID) if err != nil { s.logger.Warn("failed to update task state to FAILED after early launch error", zap.String("task_id", taskID), @@ -550,6 +558,37 @@ func (s *Service) recordSessionLaunchFailure(ctx context.Context, taskID, sessio s.processParentChildrenCompletedForTaskState(ctx, taskID, v1.TaskStateFailed) } +func (s *Service) updateTaskStateForEarlyLaunchFailure( + ctx context.Context, + taskID, sessionID string, +) (bool, error) { + if updater, ok := s.taskRepo.(primarySessionTaskStateUpdater); ok { + return updater.UpdateTaskStateIfPrimarySessionState( + ctx, taskID, sessionID, models.TaskSessionStateFailed, v1.TaskStateFailed, + ) + } + // Lightweight test repositories predate the primary-aware extension. + return s.taskRepo.UpdateTaskStateIfSessionState( + ctx, taskID, sessionID, models.TaskSessionStateFailed, v1.TaskStateFailed, + ) +} + +func (s *Service) reconcileTaskStateForEarlyLaunchFailure( + ctx context.Context, + taskID, sessionID string, + state v1.TaskState, +) error { + if state != v1.TaskStateFailed { + return fmt.Errorf("unsupported early launch task state %q", state) + } + updated, err := s.updateTaskStateForEarlyLaunchFailure(ctx, taskID, sessionID) + if err != nil || !updated { + return err + } + s.processParentChildrenCompletedForTaskState(ctx, taskID, state) + return nil +} + func (s *Service) scheduleTaskForSession(ctx context.Context, taskID, sessionID string) error { s.taskRuntimeStateMu.Lock() defer s.taskRuntimeStateMu.Unlock() @@ -838,7 +877,7 @@ func (s *Service) startTask(ctx context.Context, taskID string, agentProfileID s RouteOverride: route, }) if err != nil { - return nil, err + return nil, s.handleSessionLaunchFailure(ctx, taskID, sessionID, err) } s.postLaunchStart(ctx, taskID, execution, effectivePrompt, planModeActive || configMode, planModeActive, autoStart, attachments) @@ -1266,8 +1305,9 @@ func (s *Service) ResumeTaskSession(ctx context.Context, taskID, sessionID strin // ResumeSession launches the workspace directly rather than through // LaunchPreparedSession. Record this failure with the same state CAS, // persisted recovery claim, and archive-safe task CAS as early launch. - s.recordSessionLaunchFailure(resumeCtx, taskID, sessionID, err, session) - return nil, err + return nil, s.handleSessionLaunchFailure( + resumeCtx, taskID, sessionID, err, session, + ) } } if readySession == nil { @@ -1529,16 +1569,13 @@ func (s *Service) ensureSessionRunning(ctx context.Context, sessionID string, se } return nil } - s.updateTaskSessionState(resumeCtx, session.TaskID, sessionID, models.TaskSessionStateFailed, err.Error(), false, session) - if stateErr := s.taskRepo.UpdateTaskState(resumeCtx, session.TaskID, v1.TaskStateFailed); stateErr != nil { - s.logger.Warn("failed to update task state to FAILED after session ensure resume error", - zap.String("task_id", session.TaskID), - zap.String("session_id", sessionID), - zap.Error(stateErr)) - } else { - s.processParentChildrenCompletedForTaskState(resumeCtx, session.TaskID, v1.TaskStateFailed) - } - return fmt.Errorf("failed to resume session: %w", err) + return s.handleSessionLaunchFailure( + resumeCtx, + session.TaskID, + sessionID, + fmt.Errorf("failed to resume session: %w", err), + session, + ) } // ResumeSession launches the agent asynchronously. Wait for it to finish @@ -1685,7 +1722,8 @@ func (s *Service) startAgentOnPreparedWorkspace(ctx context.Context, sessionID s ExecutorID: session.ExecutorID, StartAgent: true, }); err != nil { - return fmt.Errorf("failed to start agent on prepared workspace: %w", err) + launchErr := fmt.Errorf("failed to start agent on prepared workspace: %w", err) + return s.handleSessionLaunchFailure(launchCtx, session.TaskID, sessionID, launchErr) } // Same reasoning as ensureSessionRunning's resume path: use launchCtx diff --git a/apps/backend/internal/orchestrator/task_operations_test.go b/apps/backend/internal/orchestrator/task_operations_test.go index 1473845cf0..976d8e4465 100644 --- a/apps/backend/internal/orchestrator/task_operations_test.go +++ b/apps/backend/internal/orchestrator/task_operations_test.go @@ -2454,7 +2454,8 @@ func TestPrepareTaskSession_WorkspaceLaunchFailureRecovery(t *testing.T) { messages := &mockMessageCreator{sessionMessageDone: make(chan struct{})} svc.messageCreator = messages - if _, err := svc.PrepareTaskSession(context.Background(), taskID, "profile1", "", "", "", true); err != nil { + sessionID, err := svc.PrepareTaskSession(context.Background(), taskID, "profile1", "", "", "", true) + if err != nil { t.Fatalf("PrepareTaskSession: %v", err) } select { @@ -2467,6 +2468,17 @@ func TestPrepareTaskSession_WorkspaceLaunchFailureRecovery(t *testing.T) { t.Fatal("transport failure created missing-branch recovery message") case <-time.After(100 * time.Millisecond): } + require.Eventually(t, func() bool { + session, getErr := baseRepo.GetTaskSession(context.Background(), sessionID) + return getErr == nil && + session.State == models.TaskSessionStateFailed && + strings.Contains(session.ErrorMessage, "Could not resolve host") + }, time.Second, 10*time.Millisecond, "expected transport failure to mark the session FAILED") + require.Eventually(t, func() bool { + taskRepo.mu.Lock() + defer taskRepo.mu.Unlock() + return taskRepo.updatedStates[taskID] == v1.TaskStateFailed + }, time.Second, 10*time.Millisecond, "expected transport failure to mark the task FAILED") }) t.Run("executor callback recovery is not duplicated", func(t *testing.T) { @@ -2509,7 +2521,7 @@ func TestPrepareTaskSession_WorkspaceLaunchFailureRecovery(t *testing.T) { }) } -func TestHandleEarlyMissingPRBranchLaunchFailure_SkipsTaskFailureWhenSessionTransitionLoses(t *testing.T) { +func TestHandleSessionLaunchFailure_SkipsTaskFailureWhenSessionTransitionLoses(t *testing.T) { repo := setupTestRepo(t) seedTaskAndSession(t, repo, "task1", "session1", models.TaskSessionStateCancelled) taskRepo := newMockTaskRepo() @@ -2519,10 +2531,10 @@ func TestHandleEarlyMissingPRBranchLaunchFailure_SkipsTaskFailureWhenSessionTran svc.messageCreator = messages svc.eventBus = bus.NewMemoryEventBus(testLogger()) - svc.handleEarlyMissingPRBranchLaunchFailure( + require.Error(t, svc.handleSessionLaunchFailure( context.Background(), "task1", "session1", errors.New("fatal: couldn't find remote ref feature/deleted-pr"), - ) + )) if len(messages.sessionMessages) != 0 { t.Fatalf("terminal-race loser created %d recovery messages", len(messages.sessionMessages)) @@ -2537,7 +2549,183 @@ func TestHandleEarlyMissingPRBranchLaunchFailure_SkipsTaskFailureWhenSessionTran } } -func TestHandleEarlyMissingPRBranchLaunchFailure_ArchiveSafeTaskWrite(t *testing.T) { +func TestHandleSessionLaunchFailure_PersistsGenericCredentialFailure(t *testing.T) { + repo := setupTestRepo(t) + seedTaskAndSession(t, repo, "task1", "session1", models.TaskSessionStateCreated) + if err := repo.SetSessionPrimary(context.Background(), "session1"); err != nil { + t.Fatalf("SetSessionPrimary: %v", err) + } + taskRepo := newMockTaskRepo() + taskRepo.tasks["task1"] = &v1.Task{ID: "task1", State: v1.TaskStateScheduling} + svc := createTestService(repo, newMockStepGetter(), taskRepo) + messages := &mockMessageCreator{} + svc.messageCreator = messages + eventBus := &recordingEventBus{} + svc.eventBus = eventBus + + const secret = "ghp_abcdefghijklmnopqrstuvwxyz1234567890AB" + returnedErr := svc.handleSessionLaunchFailure( + context.Background(), "task1", "session1", + errors.New("resolve workspace Git credential: token="+secret), + ) + require.Error(t, returnedErr) + require.NotContains(t, returnedErr.Error(), secret) + + failedSession, err := repo.GetTaskSession(context.Background(), "session1") + if err != nil { + t.Fatalf("GetTaskSession: %v", err) + } + if failedSession.State != models.TaskSessionStateFailed { + t.Fatalf("session state = %s, want FAILED", failedSession.State) + } + if strings.Contains(failedSession.ErrorMessage, secret) { + t.Fatalf("session error contains raw credential: %q", failedSession.ErrorMessage) + } + if !strings.Contains(failedSession.ErrorMessage, "resolve workspace Git credential") { + t.Fatalf("session error is not actionable: %q", failedSession.ErrorMessage) + } + if len(messages.sessionMessages) != 0 { + t.Fatalf("generic credential failure created %d missing-branch messages", len(messages.sessionMessages)) + } + if len(eventBus.events) != 1 || eventBus.events[0].subject != events.TaskSessionStateChanged { + t.Fatalf("session lifecycle events = %#v, want one %s event", eventBus.events, events.TaskSessionStateChanged) + } + taskRepo.mu.Lock() + defer taskRepo.mu.Unlock() + if got := taskRepo.tasks["task1"].State; got != v1.TaskStateFailed { + t.Fatalf("task state = %s, want FAILED", got) + } +} + +func TestHandleSessionLaunchFailure_DoesNotOverwriteTerminalSession(t *testing.T) { + for _, state := range []models.TaskSessionState{ + models.TaskSessionStateCompleted, + models.TaskSessionStateCancelled, + } { + t.Run(string(state), func(t *testing.T) { + repo := setupTestRepo(t) + seedTaskAndSession(t, repo, "task1", "session1", state) + taskRepo := newMockTaskRepo() + taskRepo.tasks["task1"] = &v1.Task{ID: "task1", State: v1.TaskStateInProgress} + svc := createTestService(repo, newMockStepGetter(), taskRepo) + svc.eventBus = bus.NewMemoryEventBus(testLogger()) + + require.Error(t, svc.handleSessionLaunchFailure( + context.Background(), "task1", "session1", + errors.New("resolve workspace Git credential: GitHub is not configured"), + )) + + session, err := repo.GetTaskSession(context.Background(), "session1") + if err != nil { + t.Fatalf("GetTaskSession: %v", err) + } + if session.State != state { + t.Fatalf("session state = %s, want %s", session.State, state) + } + taskRepo.mu.Lock() + defer taskRepo.mu.Unlock() + if taskRepo.stateWrites["task1"] != 0 { + t.Fatalf("terminal session caused %d task writes", taskRepo.stateWrites["task1"]) + } + }) + } +} + +func TestStartCreatedSession_PersistsEarlyCredentialFailure(t *testing.T) { + baseRepo := setupTestRepo(t) + seedTaskAndSession(t, baseRepo, "task1", "session1", models.TaskSessionStateCreated) + if err := baseRepo.SetSessionPrimary(context.Background(), "session1"); err != nil { + t.Fatalf("SetSessionPrimary: %v", err) + } + failureRepo := &taskEnvironmentFailureRepo{ + Repository: baseRepo, + err: errors.New("resolve workspace Git credential: GitHub is not configured"), + called: make(chan struct{}), + } + taskRepo := newMockTaskRepo() + taskRepo.tasks["task1"] = &v1.Task{ + ID: "task1", WorkspaceID: "ws1", WorkflowID: "wf1", + Title: "Test Task", Description: "start", State: v1.TaskStateScheduling, + } + agentMgr := &mockAgentManager{} + exec := executor.NewExecutor(agentMgr, failureRepo, testLogger(), executor.ExecutorConfig{}) + svc := &Service{ + logger: testLogger(), + repo: failureRepo, + workflowStepGetter: newMockStepGetter(), + taskRepo: taskRepo, + agentManager: agentMgr, + executor: exec, + messageQueue: messagequeue.NewServiceMemory(testLogger()), + scheduler: scheduler.NewScheduler( + queue.NewTaskQueue(1), exec, taskRepo, testLogger(), scheduler.SchedulerConfig{}, + ), + eventBus: &recordingEventBus{}, + } + messages := &mockMessageCreator{} + svc.messageCreator = messages + + _, err := svc.StartCreatedSession( + context.Background(), "task1", "session1", "profile1", "start", + true, false, true, nil, nil, + ) + if err == nil || !strings.Contains(err.Error(), "GitHub is not configured") { + t.Fatalf("StartCreatedSession error = %v, want GitHub credential failure", err) + } + session, getErr := baseRepo.GetTaskSession(context.Background(), "session1") + if getErr != nil { + t.Fatalf("GetTaskSession: %v", getErr) + } + if session.State != models.TaskSessionStateFailed { + t.Fatalf("session state = %s, want FAILED", session.State) + } + if !strings.Contains(session.ErrorMessage, "resolve workspace Git credential: GitHub is not configured") { + t.Fatalf("session error = %q, want actionable credential failure", session.ErrorMessage) + } + taskRepo.mu.Lock() + taskState := taskRepo.tasks["task1"].State + taskRepo.mu.Unlock() + if taskState != v1.TaskStateFailed { + t.Fatalf("task state = %s, want FAILED", taskState) + } + if len(messages.sessionMessages) != 0 { + t.Fatalf("credential failure created %d missing-branch guidance messages", len(messages.sessionMessages)) + } +} + +func TestHandleSessionLaunchFailure_GenericFailureSurvivesCancelledContext(t *testing.T) { + repo := setupTestRepo(t) + seedTaskAndSession(t, repo, "task1", "session1", models.TaskSessionStateCreated) + if err := repo.SetSessionPrimary(context.Background(), "session1"); err != nil { + t.Fatalf("SetSessionPrimary: %v", err) + } + taskRepo := &ctxAwareTaskRepo{inner: newMockTaskRepo()} + taskRepo.inner.tasks["task1"] = &v1.Task{ID: "task1", State: v1.TaskStateScheduling} + svc := createTestService(repo, newMockStepGetter(), taskRepo.inner) + svc.taskRepo = taskRepo + svc.eventBus = bus.NewMemoryEventBus(testLogger()) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + require.Error(t, svc.handleSessionLaunchFailure( + ctx, "task1", "session1", errors.New("resolve workspace Git credential: GitHub is not configured"), + )) + + failedSession, err := repo.GetTaskSession(context.Background(), "session1") + if err != nil { + t.Fatalf("GetTaskSession: %v", err) + } + if failedSession.State != models.TaskSessionStateFailed { + t.Fatalf("session state = %s, want FAILED", failedSession.State) + } + taskRepo.inner.mu.Lock() + defer taskRepo.inner.mu.Unlock() + if got := taskRepo.inner.tasks["task1"].State; got != v1.TaskStateFailed { + t.Fatalf("task state = %s, want FAILED", got) + } +} + +func TestHandleSessionLaunchFailure_ArchiveSafeTaskWrite(t *testing.T) { repo := setupTestRepo(t) seedTaskAndSession(t, repo, "task1", "session1", models.TaskSessionStateCreated) taskRepo := newMockTaskRepo() @@ -2552,10 +2740,10 @@ func TestHandleEarlyMissingPRBranchLaunchFailure_ArchiveSafeTaskWrite(t *testing svc.messageCreator = messages svc.eventBus = bus.NewMemoryEventBus(testLogger()) - svc.handleEarlyMissingPRBranchLaunchFailure( + require.Error(t, svc.handleSessionLaunchFailure( context.Background(), "task1", "session1", errors.New("fatal: couldn't find remote ref feature/deleted-pr"), - ) + )) failedSession, err := repo.GetTaskSession(context.Background(), "session1") if err != nil { @@ -2574,7 +2762,7 @@ func TestHandleEarlyMissingPRBranchLaunchFailure_ArchiveSafeTaskWrite(t *testing } } -func TestHandleEarlyMissingPRBranchLaunchFailure_ConcurrentLaunchesCreateOneMessage(t *testing.T) { +func TestHandleSessionLaunchFailure_ConcurrentLaunchesCreateOneMessage(t *testing.T) { repo := setupTestRepo(t) seedTaskAndSession(t, repo, "task1", "session1", models.TaskSessionStateCreated) taskRepo := newMockTaskRepo() @@ -2595,7 +2783,7 @@ func TestHandleEarlyMissingPRBranchLaunchFailure_ConcurrentLaunchesCreateOneMess defer done.Done() ready.Done() <-start - svc.handleEarlyMissingPRBranchLaunchFailure(context.Background(), "task1", "session1", launchErr) + _ = svc.handleSessionLaunchFailure(context.Background(), "task1", "session1", launchErr) }() } ready.Wait() @@ -3831,7 +4019,9 @@ func TestResumeTaskSession_FailureTaskWriteIsConditionalOnFailedSession(t *testi taskRepo.updateIfSessionState = func(context.Context, string, string, models.TaskSessionState, v1.TaskState) (bool, error) { return false, nil } - launchErr := errors.New("resume workspace failed") + sentinel := errors.New("resume workspace failed") + const secret = "ghp_abcdefghijklmnopqrstuvwxyz1234567890AB" + launchErr := fmt.Errorf("token=%s: %w", secret, sentinel) agentMgr := &mockAgentManager{ launchAgentFunc: func(context.Context, *executor.LaunchAgentRequest) (*executor.LaunchAgentResponse, error) { return nil, launchErr @@ -3856,8 +4046,11 @@ func TestResumeTaskSession_FailureTaskWriteIsConditionalOnFailedSession(t *testi } _, err = svc.ResumeTaskSession(ctx, "task1", "session1") - if !errors.Is(err, launchErr) { - t.Fatalf("ResumeTaskSession error = %v, want %v", err, launchErr) + if !errors.Is(err, sentinel) { + t.Fatalf("ResumeTaskSession error = %v, want wrapped sentinel", err) + } + if strings.Contains(err.Error(), secret) { + t.Fatalf("ResumeTaskSession returned raw credential: %v", err) } failed, err := repo.GetTaskSession(ctx, "session1") if err != nil { @@ -3866,6 +4059,9 @@ func TestResumeTaskSession_FailureTaskWriteIsConditionalOnFailedSession(t *testi if failed.State != models.TaskSessionStateFailed { t.Fatalf("session state = %s, want FAILED", failed.State) } + if strings.Contains(failed.ErrorMessage, secret) { + t.Fatalf("session error contains raw credential: %q", failed.ErrorMessage) + } taskRepo.mu.Lock() defer taskRepo.mu.Unlock() if taskRepo.stateWrites["task1"] != 0 || taskRepo.unconditionalWrites["task1"] != 0 { diff --git a/apps/backend/internal/task/repository/interface.go b/apps/backend/internal/task/repository/interface.go index de04b58200..4bf6bb6ca2 100644 --- a/apps/backend/internal/task/repository/interface.go +++ b/apps/backend/internal/task/repository/interface.go @@ -71,8 +71,8 @@ type TaskRepository interface { CountOpenWatcherCreatedTasks(ctx context.Context, metadataKey, watchID string) (int, error) UpdateTaskState(ctx context.Context, id string, state v1.TaskState) error // UpdateTaskStateIfSessionState atomically transitions task state only while - // the named owning session remains in expectedSessionState and the task is - // not archived. Returns the pre-update task state and whether a row changed. + // the named session remains in expectedSessionState and the task is not + // archived. Returns the pre-update state and whether a row changed. UpdateTaskStateIfSessionState( ctx context.Context, taskID, sessionID string, diff --git a/apps/backend/internal/task/repository/sqlite/task.go b/apps/backend/internal/task/repository/sqlite/task.go index fa8c5afe41..461299f020 100644 --- a/apps/backend/internal/task/repository/sqlite/task.go +++ b/apps/backend/internal/task/repository/sqlite/task.go @@ -1268,6 +1268,31 @@ func (r *Repository) UpdateTaskStateIfSessionState( taskID, sessionID string, expectedSessionState models.TaskSessionState, state v1.TaskState, +) (v1.TaskState, bool, error) { + return r.updateTaskStateIfSessionState( + ctx, taskID, sessionID, expectedSessionState, state, false, + ) +} + +// UpdateTaskStateIfPrimarySessionState additionally requires the named +// session to remain primary. +func (r *Repository) UpdateTaskStateIfPrimarySessionState( + ctx context.Context, + taskID, sessionID string, + expectedSessionState models.TaskSessionState, + state v1.TaskState, +) (v1.TaskState, bool, error) { + return r.updateTaskStateIfSessionState( + ctx, taskID, sessionID, expectedSessionState, state, true, + ) +} + +func (r *Repository) updateTaskStateIfSessionState( + ctx context.Context, + taskID, sessionID string, + expectedSessionState models.TaskSessionState, + state v1.TaskState, + requirePrimary bool, ) (v1.TaskState, bool, error) { for attempt := range updateTaskStateIfNotArchivedMaxAttempts { if attempt > 0 { @@ -1278,7 +1303,7 @@ func (r *Repository) UpdateTaskStateIfSessionState( } } oldState, updated, retry, err := r.tryUpdateTaskStateIfSessionState( - ctx, taskID, sessionID, expectedSessionState, state, + ctx, taskID, sessionID, expectedSessionState, state, requirePrimary, ) if err != nil || !retry { return oldState, updated, err @@ -1293,6 +1318,7 @@ func (r *Repository) tryUpdateTaskStateIfSessionState( taskID, sessionID string, expectedSessionState models.TaskSessionState, state v1.TaskState, + requirePrimary bool, ) (oldState v1.TaskState, updated, retry bool, err error) { tx, err := r.db.BeginTx(ctx, nil) if err != nil { @@ -1302,21 +1328,27 @@ func (r *Repository) tryUpdateTaskStateIfSessionState( var archivedAt sql.NullTime var currentSessionState models.TaskSessionState + var currentSessionIsPrimary bool err = tx.QueryRowContext(ctx, r.db.Rebind(` - SELECT tasks.state, tasks.archived_at, task_sessions.state + SELECT tasks.state, tasks.archived_at, task_sessions.state, task_sessions.is_primary FROM tasks JOIN task_sessions ON task_sessions.task_id = tasks.id WHERE tasks.id = ? AND task_sessions.id = ? - `), taskID, sessionID).Scan(&oldState, &archivedAt, ¤tSessionState) + `), taskID, sessionID).Scan(&oldState, &archivedAt, ¤tSessionState, ¤tSessionIsPrimary) if errors.Is(err, sql.ErrNoRows) { return "", false, false, nil } if err != nil { return "", false, false, err } - if archivedAt.Valid || currentSessionState != expectedSessionState { + if archivedAt.Valid || currentSessionState != expectedSessionState || + (requirePrimary && !currentSessionIsPrimary) { return oldState, false, false, nil } + requirePrimaryValue := 0 + if requirePrimary { + requirePrimaryValue = 1 + } result, err := tx.ExecContext(ctx, r.db.Rebind(` UPDATE tasks @@ -1330,8 +1362,9 @@ func (r *Repository) tryUpdateTaskStateIfSessionState( WHERE task_sessions.id = ? AND task_sessions.task_id = tasks.id AND task_sessions.state = ? + AND (? = 0 OR task_sessions.is_primary = 1) ) - `), state, time.Now().UTC(), taskID, oldState, sessionID, expectedSessionState) + `), state, time.Now().UTC(), taskID, oldState, sessionID, expectedSessionState, requirePrimaryValue) if err != nil { if isRetryableStateRaceError(err) { return oldState, false, true, nil diff --git a/apps/backend/internal/task/repository/sqlite/task_state_cas_test.go b/apps/backend/internal/task/repository/sqlite/task_state_cas_test.go index fb2de2ec9e..8739d55328 100644 --- a/apps/backend/internal/task/repository/sqlite/task_state_cas_test.go +++ b/apps/backend/internal/task/repository/sqlite/task_state_cas_test.go @@ -77,7 +77,7 @@ func TestUpdateTaskStateIfSessionState_SkipsArchivedTask(t *testing.T) { } } -func TestUpdateTaskStateIfSessionState_UpdatesMatchingSession(t *testing.T) { +func TestUpdateTaskStateIfSessionState_UpdatesMatchingNonPrimarySession(t *testing.T) { repo := newRepoForHealTests(t) ctx := context.Background() insertTask(t, repo.db, "task-session-match") @@ -111,6 +111,44 @@ func TestUpdateTaskStateIfSessionState_UpdatesMatchingSession(t *testing.T) { } } +func TestUpdateTaskStateIfPrimarySessionState_RejectsSupersededPrimarySession(t *testing.T) { + repo := newRepoForHealTests(t) + ctx := context.Background() + insertTask(t, repo.db, "task-session-superseded") + insertSession(t, repo, "session-old", "task-session-superseded", string(models.TaskSessionStateFailed)) + insertSession(t, repo, "session-new", "task-session-superseded", string(models.TaskSessionStateRunning)) + if err := repo.SetSessionPrimary(ctx, "session-new"); err != nil { + t.Fatalf("set newer primary: %v", err) + } + if err := repo.UpdateTaskState(ctx, "task-session-superseded", v1.TaskStateInProgress); err != nil { + t.Fatalf("seed IN_PROGRESS: %v", err) + } + + oldState, updated, err := repo.UpdateTaskStateIfPrimarySessionState( + ctx, + "task-session-superseded", + "session-old", + models.TaskSessionStateFailed, + v1.TaskStateFailed, + ) + if err != nil { + t.Fatalf("UpdateTaskStateIfSessionState: %v", err) + } + if updated { + t.Fatal("superseded primary session changed task state") + } + if oldState != v1.TaskStateInProgress { + t.Fatalf("old state = %q, want %q", oldState, v1.TaskStateInProgress) + } + task, err := repo.GetTask(ctx, "task-session-superseded") + if err != nil { + t.Fatalf("get task: %v", err) + } + if task.State != v1.TaskStateInProgress { + t.Fatalf("task state = %q, want %q", task.State, v1.TaskStateInProgress) + } +} + func TestRestoreTaskMessageRollbackIfSessionState_RejectionDoesNotMutateCandidate(t *testing.T) { repo := newRepoForHealTests(t) ctx := context.Background() diff --git a/apps/backend/internal/task/service/service_workflow.go b/apps/backend/internal/task/service/service_workflow.go index 130674ed3b..cdf36e527b 100644 --- a/apps/backend/internal/task/service/service_workflow.go +++ b/apps/backend/internal/task/service/service_workflow.go @@ -2,6 +2,7 @@ package service import ( "context" + "errors" "fmt" "time" @@ -21,6 +22,16 @@ type ApproveSessionResult struct { WorkflowStep *wfmodels.WorkflowStep } +type primarySessionTaskStateRepository interface { + UpdateTaskStateIfPrimarySessionState( + context.Context, + string, + string, + models.TaskSessionState, + v1.TaskState, + ) (v1.TaskState, bool, error) +} + // ApproveSession approves a session's current step and moves it to the next step. // It reads the step's on_turn_complete actions to determine where to transition. // If no transition actions are configured, it falls back to the next step by position. @@ -272,9 +283,49 @@ func (s *Service) UpdateTaskStateIfSessionState( expectedSessionState models.TaskSessionState, state v1.TaskState, ) (bool, error) { - oldState, updated, err := s.tasks.UpdateTaskStateIfSessionState( - ctx, taskID, sessionID, expectedSessionState, state, + return s.updateTaskStateIfSessionState( + ctx, taskID, sessionID, expectedSessionState, state, false, ) +} + +// UpdateTaskStateIfPrimarySessionState also requires the named session to +// remain primary. +func (s *Service) UpdateTaskStateIfPrimarySessionState( + ctx context.Context, + taskID, sessionID string, + expectedSessionState models.TaskSessionState, + state v1.TaskState, +) (bool, error) { + return s.updateTaskStateIfSessionState( + ctx, taskID, sessionID, expectedSessionState, state, true, + ) +} + +func (s *Service) updateTaskStateIfSessionState( + ctx context.Context, + taskID, sessionID string, + expectedSessionState models.TaskSessionState, + state v1.TaskState, + requirePrimary bool, +) (bool, error) { + var ( + oldState v1.TaskState + updated bool + err error + ) + if requirePrimary { + updater, ok := s.tasks.(primarySessionTaskStateRepository) + if !ok { + return false, errors.New("primary-session task state update is not supported") + } + oldState, updated, err = updater.UpdateTaskStateIfPrimarySessionState( + ctx, taskID, sessionID, expectedSessionState, state, + ) + } else { + oldState, updated, err = s.tasks.UpdateTaskStateIfSessionState( + ctx, taskID, sessionID, expectedSessionState, state, + ) + } if err != nil || !updated { return false, err } diff --git a/apps/backend/internal/task/service/service_workflow_test.go b/apps/backend/internal/task/service/service_workflow_test.go index 47d7bdcded..e3d35f789a 100644 --- a/apps/backend/internal/task/service/service_workflow_test.go +++ b/apps/backend/internal/task/service/service_workflow_test.go @@ -87,6 +87,32 @@ func TestService_SetWorkflowHidden_HealsStaleRecord(t *testing.T) { } } +func TestService_UpdateTaskStateIfPrimarySessionStatePublishesLifecycleEvent(t *testing.T) { + svc, eventBus, repo := createTestService(t) + ctx := context.Background() + createTaskWithoutRepositories(t, ctx, repo) + createRunningSession(t, ctx, repo, "session-1", "task-1", models.TaskSessionStateFailed) + if err := svc.SetPrimarySession(ctx, "session-1"); err != nil { + t.Fatalf("SetPrimarySession: %v", err) + } + eventBus.ClearEvents() + + updated, err := svc.UpdateTaskStateIfPrimarySessionState( + ctx, + "task-1", + "session-1", + models.TaskSessionStateFailed, + v1.TaskStateFailed, + ) + if err != nil { + t.Fatalf("UpdateTaskStateIfSessionState: %v", err) + } + if !updated { + t.Fatal("expected task state transition") + } + findPublishedEvent(t, eventBus.GetPublishedEvents(), events.TaskStateChanged) +} + func TestService_MoveTaskRejectsInvalidWorkflowTargets(t *testing.T) { svc, _, repo := createTestService(t) ctx := context.Background() diff --git a/apps/web/components/github/github-cli-form.test.tsx b/apps/web/components/github/github-cli-form.test.tsx new file mode 100644 index 0000000000..3a129930a8 --- /dev/null +++ b/apps/web/components/github/github-cli-form.test.tsx @@ -0,0 +1,51 @@ +import { cleanup, render, screen } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { ToastProvider } from "@/components/toast-provider"; +import { GitHubCLIForm } from "./github-cli-form"; + +const mocks = vi.hoisted(() => ({ + fetchAccounts: vi.fn(), + setConnection: vi.fn(), +})); + +vi.mock("@/lib/api/domains/github-api", () => ({ + fetchGitHubCLIAccounts: mocks.fetchAccounts, + setGitHubWorkspaceConnection: mocks.setConnection, +})); + +beforeEach(() => { + mocks.fetchAccounts.mockReset(); + mocks.setConnection.mockReset(); +}); + +afterEach(() => cleanup()); + +describe("GitHubCLIForm", () => { + it("shows account discovery failures instead of an empty-account message", async () => { + mocks.fetchAccounts.mockRejectedValue(new Error("Request failed (500): github_internal_error")); + + render( + + + , + ); + + expect((await screen.findByRole("alert")).textContent).toBe( + "Could not load GitHub CLI accounts. Request failed (500): github_internal_error", + ); + expect(screen.queryByText(/Sign in with/)).toBeNull(); + }); + + it("keeps sign-in guidance for a successful empty account list", async () => { + mocks.fetchAccounts.mockResolvedValue([]); + + render( + + + , + ); + + expect(await screen.findByText(/Sign in with/)).toBeTruthy(); + expect(screen.queryByRole("alert")).toBeNull(); + }); +}); diff --git a/apps/web/components/github/github-cli-form.tsx b/apps/web/components/github/github-cli-form.tsx index d586ae580c..a9e7fc0eb4 100644 --- a/apps/web/components/github/github-cli-form.tsx +++ b/apps/web/components/github/github-cli-form.tsx @@ -9,6 +9,36 @@ import { useToast } from "@/components/toast-provider"; import { fetchGitHubCLIAccounts, setGitHubWorkspaceConnection } from "@/lib/api/domains/github-api"; import type { GitHubCLIAccount } from "@/lib/types/github"; +function GitHubCLIAccountNotice({ + loadError, + loading, + hasAccounts, +}: { + loadError: string | null; + loading: boolean; + hasAccounts: boolean; +}) { + if (loading || hasAccounts) return null; + if (loadError) { + return ( +

+ Could not load GitHub CLI accounts. {loadError} +

+ ); + } + return ( +

+ Sign in with gh auth login, then reopen this dialog. +

+ ); +} + +function accountPlaceholder(loading: boolean, loadError: string | null) { + if (loading) return "Loading accounts..."; + if (loadError) return "Accounts unavailable"; + return "No gh accounts found"; +} + export function GitHubCLIForm({ workspaceId, onSaved, @@ -18,6 +48,7 @@ export function GitHubCLIForm({ }) { const [accounts, setAccounts] = useState([]); const [selected, setSelected] = useState(""); + const [loadError, setLoadError] = useState(null); const [loading, setLoading] = useState(true); const [saving, setSaving] = useState(false); const { toast } = useToast(); @@ -26,6 +57,7 @@ export function GitHubCLIForm({ let current = true; setAccounts([]); setSelected(""); + setLoadError(null); setLoading(true); setSaving(false); fetchGitHubCLIAccounts(workspaceId, { cache: "no-store" }) @@ -36,7 +68,13 @@ export function GitHubCLIForm({ items.find((item) => item.selected) ?? items.find((item) => item.active) ?? items[0]; setSelected(preferred ? `${preferred.host}\n${preferred.login}` : ""); }) - .catch(() => current && setAccounts([])) + .catch((error) => { + if (!current) return; + setAccounts([]); + setLoadError( + error instanceof Error ? error.message : "Unexpected response from the server", + ); + }) .finally(() => current && setLoading(false)); return () => { current = false; @@ -80,7 +118,7 @@ export function GitHubCLIForm({