From ba24c2963daac866e833de24b7d8749d4188767f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Mon, 11 May 2026 22:55:25 +0100 Subject: [PATCH 1/3] refactor(logger): close rotating sink on shutdown, add config docs (#877) --- apps/backend/cmd/kandev/main.go | 4 +- apps/backend/internal/common/config/config.go | 29 +++- .../internal/common/config/config_test.go | 85 ++++++++++ apps/backend/internal/common/logger/logger.go | 64 +++++--- .../internal/common/logger/logger_test.go | 117 ++++++++++++++ docs/configuration.md | 148 ++++++++++++++++++ docs/db.md | 34 ++-- docs/docker.md | 25 ++- docs/k8s.md | 43 +++-- 9 files changed, 481 insertions(+), 68 deletions(-) create mode 100644 apps/backend/internal/common/logger/logger_test.go create mode 100644 docs/configuration.md diff --git a/apps/backend/cmd/kandev/main.go b/apps/backend/cmd/kandev/main.go index 1cb5d1f736..382f52e540 100644 --- a/apps/backend/cmd/kandev/main.go +++ b/apps/backend/cmd/kandev/main.go @@ -164,7 +164,7 @@ func realMain() int { } defer func() { runCleanups() - _ = log.Sync() + _ = log.Close() }() logger.SetDefault(log) @@ -605,7 +605,7 @@ func awaitShutdown( go func() { second := <-quit log.Warn("Received second shutdown signal, forcing exit", zap.String("signal", second.String())) - _ = log.Sync() + _ = log.Close() os.Exit(1) }() diff --git a/apps/backend/internal/common/config/config.go b/apps/backend/internal/common/config/config.go index c0873b8d2c..8421f3c8af 100644 --- a/apps/backend/internal/common/config/config.go +++ b/apps/backend/internal/common/config/config.go @@ -141,12 +141,16 @@ type LoggingConfig struct { Format string `mapstructure:"format"` OutputPath string `mapstructure:"outputPath"` - // Rotation options — apply only when OutputPath is a file path + // Rotation options - apply only when OutputPath is a file path // (ignored for stdout/stderr). Backed by lumberjack. - MaxSizeMB int `mapstructure:"maxSizeMb"` - MaxBackups int `mapstructure:"maxBackups"` - MaxAgeDays int `mapstructure:"maxAgeDays"` - Compress bool `mapstructure:"compress"` + // + // Note: lumberjack creates the active log file with mode 0600 (owner read/write + // only); the previous os.OpenFile path used 0644. External log shippers or + // sidecars running as a different user will need to run as the same user. + MaxSizeMB int `mapstructure:"maxSizeMb"` // rotate when file exceeds this size; 0 = lumberjack default (100MB) + MaxBackups int `mapstructure:"maxBackups"` // max number of rotated files to retain; 0 = unlimited + MaxAgeDays int `mapstructure:"maxAgeDays"` // max age in days of rotated files; 0 = unlimited + Compress bool `mapstructure:"compress"` // gzip rotated files } // RepositoryDiscoveryConfig holds configuration for local repository scanning. @@ -394,7 +398,14 @@ func validate(cfg *Config) error { errs = append(errs, "server.port must be between 1 and 65535") } - // Database validation + // Database validation. Normalize the driver in place so downstream + // case-sensitive comparisons (and the postgres-only branch below) see + // a canonical value. + cfg.Database.Driver = strings.ToLower(cfg.Database.Driver) + validDrivers := map[string]bool{"sqlite": true, "postgres": true} + if !validDrivers[cfg.Database.Driver] { + errs = append(errs, "database.driver must be one of: sqlite, postgres") + } if cfg.Database.Driver == "postgres" { if cfg.Database.Port <= 0 || cfg.Database.Port > 65535 { errs = append(errs, "database.port must be between 1 and 65535") @@ -405,6 +416,12 @@ func validate(cfg *Config) error { if cfg.Database.DBName == "" { errs = append(errs, "database.dbName is required for postgres driver") } + validSSLModes := map[string]bool{ + "disable": true, "require": true, "verify-ca": true, "verify-full": true, + } + if !validSSLModes[strings.ToLower(cfg.Database.SSLMode)] { + errs = append(errs, "database.sslMode must be one of: disable, require, verify-ca, verify-full") + } } // NATS validation - optional (uses in-memory event bus if not set) diff --git a/apps/backend/internal/common/config/config_test.go b/apps/backend/internal/common/config/config_test.go index 7538606f76..1fc69a543e 100644 --- a/apps/backend/internal/common/config/config_test.go +++ b/apps/backend/internal/common/config/config_test.go @@ -3,9 +3,24 @@ package config import ( "os" "path/filepath" + "strings" "testing" ) +// minimalValidConfig returns a Config that passes validate() out of the box. +// Tests modify a copy to exercise individual validation branches. +func minimalValidConfig() *Config { + return &Config{ + Server: ServerConfig{Port: 38429}, + Database: DatabaseConfig{Driver: "sqlite"}, + Auth: AuthConfig{TokenDuration: 3600}, + Logging: LoggingConfig{Level: "info", Format: "text"}, + RepositoryDiscovery: RepositoryDiscoveryConfig{ + MaxDepth: 5, + }, + } +} + func TestResolvedHomeDir_Default(t *testing.T) { cfg := &Config{} home, err := os.UserHomeDir() @@ -57,3 +72,73 @@ func TestResolvedDataDir_DerivedFromHomeDir(t *testing.T) { t.Errorf("ResolvedDataDir() = %q, want %q", got, want) } } + +func TestValidate_DatabaseDriver(t *testing.T) { + t.Run("sqlite_ok", func(t *testing.T) { + cfg := minimalValidConfig() + if err := validate(cfg); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + + t.Run("mixed_case_postgres_normalized", func(t *testing.T) { + cfg := minimalValidConfig() + cfg.Database.Driver = "Postgres" + cfg.Database.Port = 5432 + cfg.Database.User = "u" + cfg.Database.DBName = "db" + cfg.Database.SSLMode = "disable" + if err := validate(cfg); err != nil { + t.Fatalf("expected mixed-case 'Postgres' to normalize, got %v", err) + } + if cfg.Database.Driver != "postgres" { + t.Errorf("driver not normalized: got %q, want %q", cfg.Database.Driver, "postgres") + } + }) + + t.Run("unknown_driver_rejected", func(t *testing.T) { + cfg := minimalValidConfig() + cfg.Database.Driver = "mysql" + err := validate(cfg) + if err == nil || !strings.Contains(err.Error(), "database.driver") { + t.Fatalf("expected database.driver error, got %v", err) + } + }) +} + +func TestValidate_PostgresSSLMode(t *testing.T) { + for _, mode := range []string{"disable", "require", "verify-ca", "verify-full"} { + t.Run(mode, func(t *testing.T) { + cfg := minimalValidConfig() + cfg.Database.Driver = "postgres" + cfg.Database.Port = 5432 + cfg.Database.User = "u" + cfg.Database.DBName = "db" + cfg.Database.SSLMode = mode + if err := validate(cfg); err != nil && strings.Contains(err.Error(), "sslMode") { + t.Errorf("sslMode %q rejected unexpectedly: %v", mode, err) + } + }) + } + + t.Run("invalid_rejected", func(t *testing.T) { + cfg := minimalValidConfig() + cfg.Database.Driver = "postgres" + cfg.Database.Port = 5432 + cfg.Database.User = "u" + cfg.Database.DBName = "db" + cfg.Database.SSLMode = "bogus" + err := validate(cfg) + if err == nil || !strings.Contains(err.Error(), "sslMode") { + t.Fatalf("expected sslMode error, got %v", err) + } + }) + + t.Run("sqlite_ignores_sslmode", func(t *testing.T) { + cfg := minimalValidConfig() + cfg.Database.SSLMode = "bogus" + if err := validate(cfg); err != nil { + t.Errorf("sqlite should ignore sslMode, got %v", err) + } + }) +} diff --git a/apps/backend/internal/common/logger/logger.go b/apps/backend/internal/common/logger/logger.go index 204f69b4df..eb62a2b06c 100644 --- a/apps/backend/internal/common/logger/logger.go +++ b/apps/backend/internal/common/logger/logger.go @@ -22,24 +22,29 @@ const ( ) // LoggingConfig holds the configuration for the logger. +// +// This struct is constructed in Go (not unmarshaled via viper/mapstructure), +// so it does not carry struct tags. Callers map their own config types +// (e.g. config.LoggingConfig) into these fields explicitly. type LoggingConfig struct { - Level string `mapstructure:"level"` // debug, info, warn, error - Format string `mapstructure:"format"` // json, console - OutputPath string `mapstructure:"output_path"` // stdout, stderr, or file path + Level string // debug, info, warn, error + Format string // json, console + OutputPath string // stdout, stderr, or file path - // Rotation options — apply only when OutputPath is a file path + // Rotation options - apply only when OutputPath is a file path // (ignored for stdout/stderr). Backed by lumberjack. - MaxSizeMB int `mapstructure:"max_size_mb"` // rotate when file exceeds this size; 0 = lumberjack default (100MB) - MaxBackups int `mapstructure:"max_backups"` // max number of rotated files to retain; 0 = unlimited - MaxAgeDays int `mapstructure:"max_age_days"` // max age of rotated files; 0 = unlimited - Compress bool `mapstructure:"compress"` // gzip rotated files + MaxSizeMB int // rotate when file exceeds this size; 0 = lumberjack default (100MB) + MaxBackups int // max number of rotated files to retain; 0 = unlimited + MaxAgeDays int // max age of rotated files; 0 = unlimited + Compress bool // gzip rotated files } // Logger wraps zap.Logger to provide structured logging with helper methods. type Logger struct { - zap *zap.Logger - sugar *zap.SugaredLogger - fields []zap.Field + zap *zap.Logger + sugar *zap.SugaredLogger + fields []zap.Field + rotator *lumberjack.Logger // non-nil only when OutputPath is a file path } var ( @@ -95,20 +100,24 @@ func NewLogger(cfg LoggingConfig) (*Logger, error) { encoder = zapcore.NewJSONEncoder(encoderConfig) } - var writeSyncer zapcore.WriteSyncer + var ( + writeSyncer zapcore.WriteSyncer + rotator *lumberjack.Logger + ) switch cfg.OutputPath { case "", "stdout": writeSyncer = zapcore.AddSync(os.Stdout) case "stderr": writeSyncer = zapcore.AddSync(os.Stderr) default: - writeSyncer = zapcore.AddSync(&lumberjack.Logger{ + rotator = &lumberjack.Logger{ Filename: cfg.OutputPath, MaxSize: cfg.MaxSizeMB, MaxBackups: cfg.MaxBackups, MaxAge: cfg.MaxAgeDays, Compress: cfg.Compress, - }) + } + writeSyncer = zapcore.AddSync(rotator) } outputCore := zapcore.NewCore(encoder, writeSyncer, level) @@ -117,8 +126,9 @@ func NewLogger(cfg LoggingConfig) (*Logger, error) { zapLogger := zap.New(core, zap.AddCaller(), zap.AddStacktrace(zapcore.ErrorLevel)) return &Logger{ - zap: zapLogger, - sugar: zapLogger.Sugar(), + zap: zapLogger, + sugar: zapLogger.Sugar(), + rotator: rotator, }, nil } @@ -151,12 +161,28 @@ func (l *Logger) Sync() error { return l.zap.Sync() } +// Close flushes pending log entries and releases the file handle held by the +// rotating file sink, if any. Safe to call on a stdout/stderr logger (no-op). +// Call before process shutdown to ensure the final batch reaches disk and the +// rotation cycle can complete cleanly. +func (l *Logger) Close() error { + _ = l.zap.Sync() + if l.rotator != nil { + return l.rotator.Close() + } + return nil +} + // WithFields returns a new Logger with the given fields added. +// The rotator reference is propagated so Close() on a derived logger +// still releases the underlying file handle. func (l *Logger) WithFields(fields ...zap.Field) *Logger { + z := l.zap.With(fields...) return &Logger{ - zap: l.zap.With(fields...), - sugar: l.zap.With(fields...).Sugar(), - fields: append(l.fields, fields...), + zap: z, + sugar: z.Sugar(), + fields: append(l.fields, fields...), + rotator: l.rotator, } } diff --git a/apps/backend/internal/common/logger/logger_test.go b/apps/backend/internal/common/logger/logger_test.go new file mode 100644 index 0000000000..33c29816f6 --- /dev/null +++ b/apps/backend/internal/common/logger/logger_test.go @@ -0,0 +1,117 @@ +package logger + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "go.uber.org/zap" +) + +func TestNewLogger_StdoutHasNoRotator(t *testing.T) { + for _, path := range []string{"", "stdout", "stderr"} { + t.Run(path, func(t *testing.T) { + log, err := NewLogger(LoggingConfig{Level: "info", Format: "json", OutputPath: path}) + if err != nil { + t.Fatalf("NewLogger: %v", err) + } + if log.rotator != nil { + t.Fatalf("expected rotator to be nil for %q output, got %#v", path, log.rotator) + } + if err := log.Close(); err != nil { + t.Fatalf("Close on stdout/stderr logger should be a no-op, got %v", err) + } + }) + } +} + +func TestNewLogger_FileOutputUsesRotator(t *testing.T) { + dir := t.TempDir() + logPath := filepath.Join(dir, "kandev.log") + + log, err := NewLogger(LoggingConfig{ + Level: "info", + Format: "json", + OutputPath: logPath, + MaxSizeMB: 10, + MaxBackups: 3, + MaxAgeDays: 7, + Compress: true, + }) + if err != nil { + t.Fatalf("NewLogger: %v", err) + } + if log.rotator == nil { + t.Fatal("expected rotator to be configured for file output") + } + + if log.rotator.Filename != logPath { + t.Errorf("Filename: got %q, want %q", log.rotator.Filename, logPath) + } + if log.rotator.MaxSize != 10 { + t.Errorf("MaxSize: got %d, want 10", log.rotator.MaxSize) + } + if log.rotator.MaxBackups != 3 { + t.Errorf("MaxBackups: got %d, want 3", log.rotator.MaxBackups) + } + if log.rotator.MaxAge != 7 { + t.Errorf("MaxAge: got %d, want 7", log.rotator.MaxAge) + } + if !log.rotator.Compress { + t.Error("Compress: got false, want true") + } + + log.Info("hello", zap.String("k", "v")) + if err := log.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + data, err := os.ReadFile(logPath) + if err != nil { + t.Fatalf("read log file: %v", err) + } + if !strings.Contains(string(data), "hello") { + t.Errorf("log file missing entry; got %q", string(data)) + } +} + +func TestWithFields_PropagatesRotator(t *testing.T) { + dir := t.TempDir() + log, err := NewLogger(LoggingConfig{ + Level: "info", + Format: "json", + OutputPath: filepath.Join(dir, "kandev.log"), + }) + if err != nil { + t.Fatalf("NewLogger: %v", err) + } + + derived := log.WithFields(zap.String("k", "v")) + if derived.rotator != log.rotator { + t.Fatalf("WithFields dropped rotator: got %p, want %p", derived.rotator, log.rotator) + } + + // Derived helpers all funnel through WithFields, so spot-check one. + if log.WithTaskID("t1").rotator != log.rotator { + t.Fatal("WithTaskID dropped rotator") + } +} + +func TestLoggerClose_IsIdempotent(t *testing.T) { + dir := t.TempDir() + log, err := NewLogger(LoggingConfig{ + Level: "info", + Format: "json", + OutputPath: filepath.Join(dir, "kandev.log"), + }) + if err != nil { + t.Fatalf("NewLogger: %v", err) + } + if err := log.Close(); err != nil { + t.Fatalf("first Close: %v", err) + } + if err := log.Close(); err != nil { + t.Fatalf("second Close should be a no-op, got %v", err) + } +} diff --git a/docs/configuration.md b/docs/configuration.md new file mode 100644 index 0000000000..59fec358f6 --- /dev/null +++ b/docs/configuration.md @@ -0,0 +1,148 @@ +# Configuration + +Kandev's backend reads configuration from three sources, in this order of precedence (later sources override earlier ones): + +1. Built-in defaults (`apps/backend/internal/common/config/config.go`). +2. A YAML config file (`config.yaml`). +3. Environment variables (`KANDEV_*`). + +Both the file and env vars are optional; the backend boots with sensible defaults out of the box. See [`docker.md`](./docker.md) and [`k8s.md`](./k8s.md) for deployment-specific tables; this page is the full reference. + +## Config file + +The backend looks for `config.yaml` in, in order: + +- An explicit path passed at startup (used in tests). +- The current working directory. +- `/etc/kandev/`. + +A missing file is not an error - defaults plus env vars take over. + +## Environment variables + +All env vars use the `KANDEV_` prefix. Nested keys map by replacing `.` with `_` and uppercasing; **camelCase becomes one uppercase run** (no underscore inserted), because viper does not synthesize a snake_case form. + +| YAML key | Env var | +|---|---| +| `server.port` | `KANDEV_SERVER_PORT` | +| `server.webInternalUrl` | `KANDEV_SERVER_WEBINTERNALURL` (or alias `KANDEV_WEB_INTERNAL_URL`) | +| `database.dbName` | `KANDEV_DATABASE_DBNAME` | +| `logging.maxSizeMb` | `KANDEV_LOGGING_MAXSIZEMB` | +| `homeDir` | `KANDEV_HOME_DIR` (alias) | +| `logging.level` | `KANDEV_LOG_LEVEL` (alias) | + +The aliases on the right are explicit bindings - see `LoadWithPath` in `config.go` for the full list. New keys should follow the deterministic rule (`KANDEV_
_`) unless there is a reason to add an alias. + +## Full `config.yaml` example + +Every key shown here has a default - copying the whole file changes nothing. Use it as a starting point and delete what you don't need to override. + +```yaml +# Kandev root directory. Empty = ~/.kandev (or KANDEV_HOME_DIR if set). +# All workspace artifacts (data, tasks, worktrees, repos, sessions) live here. +homeDir: "" + +server: + host: "0.0.0.0" + port: 38429 # API + WebSocket + Web UI + readTimeout: 30 # seconds + writeTimeout: 30 # seconds + webInternalUrl: "" # internal URL the backend uses to call the web app + +database: + driver: "sqlite" # "sqlite" or "postgres" + path: "" # sqlite: empty = $homeDir/data/kandev.db + + # postgres-only fields below (ignored when driver=sqlite) + host: "localhost" + port: 5432 + user: "kandev" # required when driver=postgres + password: "" # required when driver=postgres in most setups + dbName: "kandev" # required when driver=postgres + sslMode: "disable" # disable | require | verify-ca | verify-full + maxConns: 25 + minConns: 5 + +nats: + url: "" # empty = use in-memory event bus + clusterId: "kandev-cluster" + clientId: "kandev-client" + maxReconnects: 10 + +events: + namespace: "" # empty = derive from runtime data identity + +docker: + enabled: true # disables Docker-based executors when false + host: "" # empty = platform default (unix:///var/run/docker.sock, etc.) + apiVersion: "" # empty = auto-negotiate + tlsVerify: false + defaultNetwork: "kandev-network" + volumeBasePath: "" # empty = /var/lib/kandev/volumes (Linux/macOS) + +agent: + standaloneHost: "localhost" + standalonePort: 39429 # agentctl control port + +auth: + jwtSecret: "" # empty = auto-generate a random dev secret on boot + tokenDuration: 3600 # seconds + +logging: + level: "info" # debug | info | warn | error + format: "text" # text | json (auto = json when KUBERNETES_SERVICE_HOST is set) + outputPath: "stdout" # stdout | stderr | /path/to/file.log + + # Rotation - only applied when outputPath is a file path. + # Active log files are created with mode 0600 (owner-only). + maxSizeMb: 100 # rotate at this size; 0 = lumberjack default (100MB) + maxBackups: 5 # 0 = unlimited + maxAgeDays: 30 # 0 = unlimited + compress: true # gzip rotated files + +repositoryDiscovery: + roots: [] # absolute paths to scan for local git repos + maxDepth: 5 + +worktree: + enabled: true + defaultBranch: "main" + cleanupOnRemove: true + fetchTimeoutSeconds: 60 + pullTimeoutSeconds: 60 + +repoClone: + basePath: "" # empty = $homeDir/repos + +debug: + pprofEnabled: false # enables /debug/pprof and /api/v1/debug/memory +``` + +## Required vs optional + +Almost every field has a default and is optional. The exceptions: + +| Field | When required | What happens otherwise | +|---|---|---| +| `database.user` | `database.driver=postgres` | Startup fails with `database.user is required for postgres driver` | +| `database.dbName` | `database.driver=postgres` | Startup fails with `database.dbName is required for postgres driver` | +| `database.password` | `database.driver=postgres` in most setups (some Postgres configs allow passwordless local auth) | Connection fails at runtime | +| `auth.jwtSecret` | Never strictly required, but in production set an explicit value | A random secret is generated on boot - tokens become invalid on restart | + +Validated value sets (any other value is a startup error): + +| Field | Allowed values | +|---|---| +| `server.port` | `1`-`65535` | +| `database.driver` | `sqlite`, `postgres` | +| `database.port` | `1`-`65535` (only validated when `driver=postgres`) | +| `database.sslMode` | `disable`, `require`, `verify-ca`, `verify-full` (only validated when `driver=postgres`) | +| `logging.level` | `debug`, `info`, `warn`, `error` | +| `logging.format` | `json`, `text` | + +## Tips + +- **Env vars override the file.** Useful for secrets (`KANDEV_DATABASE_PASSWORD`) and per-environment knobs (`KANDEV_LOG_LEVEL`). +- **K8s / Docker:** prefer env vars for everything; skip the YAML file entirely. +- **Local dev:** drop a `config.yaml` next to where you run the backend; viper picks it up from the current working directory. +- **Format auto-detection** (`logging.format`): set `KANDEV_ENV=production` or run inside K8s and you get JSON logs without changing the config. diff --git a/docs/db.md b/docs/db.md index f8b18b1863..49fbe10606 100644 --- a/docs/db.md +++ b/docs/db.md @@ -2,17 +2,19 @@ Kandev supports two database backends: **SQLite** (default) and **PostgreSQL**. +> See [`configuration.md`](./configuration.md) for the full backend configuration reference (every YAML key + env var). This page focuses on database-specific settings. + ## SQLite (default) SQLite requires no external services and works out of the box. ### Environment variables -| Variable | Default | Description | -|----------|---------|-------------| -| `KANDEV_HOME_DIR` | `~/.kandev` | Kandev root directory — contains `data/` (DB), `tasks/`, `worktrees/`, `repos/`, `sessions/`, and `lsp-servers/` | -| `KANDEV_DATABASE_DRIVER` | `sqlite` | Database driver | -| `KANDEV_DATABASE_PATH` | `$KANDEV_HOME_DIR/data/kandev.db` | Path to the SQLite database file (override) | +| Variable | Required | Default | Description | +|----------|----------|---------|-------------| +| `KANDEV_HOME_DIR` | No | `~/.kandev` | Kandev root directory - contains `data/` (DB), `tasks/`, `worktrees/`, `repos/`, `sessions/`, and `lsp-servers/` | +| `KANDEV_DATABASE_DRIVER` | No | `sqlite` | Database driver | +| `KANDEV_DATABASE_PATH` | No | `$KANDEV_HOME_DIR/data/kandev.db` | Path to the SQLite database file (override) | ### Config file (`config.yaml`) @@ -38,17 +40,17 @@ CREATE DATABASE kandev OWNER kandev; #### Environment variables -| Variable | Default | Description | -|----------|---------|-------------| -| `KANDEV_DATABASE_DRIVER` | `sqlite` | Set to `postgres` | -| `KANDEV_DATABASE_HOST` | `localhost` | PostgreSQL host | -| `KANDEV_DATABASE_PORT` | `5432` | PostgreSQL port | -| `KANDEV_DATABASE_USER` | `kandev` | Database user | -| `KANDEV_DATABASE_PASSWORD` | (empty) | Database password | -| `KANDEV_DATABASE_DBNAME` | `kandev` | Database name | -| `KANDEV_DATABASE_SSLMODE` | `disable` | SSL mode (`disable`, `require`, `verify-ca`, `verify-full`) | -| `KANDEV_DATABASE_MAXCONNS` | `25` | Maximum open connections | -| `KANDEV_DATABASE_MINCONNS` | `5` | Minimum idle connections | +| Variable | Required | Default | Description | +|----------|----------|---------|-------------| +| `KANDEV_DATABASE_DRIVER` | Yes | `sqlite` | Set to `postgres` | +| `KANDEV_DATABASE_HOST` | No | `localhost` | PostgreSQL host | +| `KANDEV_DATABASE_PORT` | No | `5432` | PostgreSQL port | +| `KANDEV_DATABASE_USER` | Yes | `kandev` | Database user | +| `KANDEV_DATABASE_PASSWORD` | Usually | (empty) | Database password - required unless your Postgres allows passwordless auth | +| `KANDEV_DATABASE_DBNAME` | Yes | `kandev` | Database name | +| `KANDEV_DATABASE_SSLMODE` | No | `disable` | SSL mode (`disable`, `require`, `verify-ca`, `verify-full`) | +| `KANDEV_DATABASE_MAXCONNS` | No | `25` | Maximum open connections | +| `KANDEV_DATABASE_MINCONNS` | No | `5` | Minimum idle connections | #### Config file (`config.yaml`) diff --git a/docs/docker.md b/docs/docker.md index de151ad713..e3c83df5d1 100644 --- a/docs/docker.md +++ b/docs/docker.md @@ -61,14 +61,23 @@ docker run -p 38429:38429 \ ### Environment Variables -| Variable | Default | Description | -|----------|---------|-------------| -| `KANDEV_HOME_DIR` | `/data` | Kandev home directory — contains `data/` (DB), `tasks/`, `worktrees/`, `repos/`, `sessions/`, and `lsp-servers/` | -| `KANDEV_DATABASE_DRIVER` | `sqlite` | Database driver (`sqlite` or `postgres`) | -| `KANDEV_DATABASE_PATH` | `$KANDEV_HOME_DIR/data/kandev.db` | SQLite database file path (override) | -| `KANDEV_LOG_LEVEL` | `info` | Log level: `debug`, `info`, `warn`, `error` | -| `KANDEV_LOGGING_FORMAT` | `text` | Log format: `text` or `json` | -| `KANDEV_DOCKER_ENABLED` | `false` | Enable Docker runtime for agents (see below) | +See [`configuration.md`](./configuration.md) for the full reference (including the YAML form and every knob the backend reads). The table below covers the env vars most often set in a Docker deployment. + +| Variable | Required | Default | Description | +|----------|----------|---------|-------------| +| `KANDEV_HOME_DIR` | No | `/data` | Kandev home directory - contains `data/` (DB), `tasks/`, `worktrees/`, `repos/`, `sessions/`, and `lsp-servers/` | +| `KANDEV_DATABASE_DRIVER` | No | `sqlite` | Database driver (`sqlite` or `postgres`) | +| `KANDEV_DATABASE_PATH` | No | `$KANDEV_HOME_DIR/data/kandev.db` | SQLite database file path (override) | +| `KANDEV_LOG_LEVEL` | No | `info` | Log level: `debug`, `info`, `warn`, `error` | +| `KANDEV_LOGGING_FORMAT` | No | `text` | Log format: `text` or `json` | +| `KANDEV_LOGGING_OUTPUTPATH` | No | `stdout` | Log destination: `stdout`, `stderr`, or a file path (rotated when a file) | +| `KANDEV_LOGGING_MAXSIZEMB` | No | `100` | Rotate the log file when it exceeds this size (MB). File output only. | +| `KANDEV_LOGGING_MAXBACKUPS` | No | `5` | Max rotated files to retain (`0` = unlimited). File output only. | +| `KANDEV_LOGGING_MAXAGEDAYS` | No | `30` | Max age of rotated files in days (`0` = unlimited). File output only. | +| `KANDEV_LOGGING_COMPRESS` | No | `true` | Gzip rotated files. File output only. | +| `KANDEV_DOCKER_ENABLED` | No | `false` | Enable Docker runtime for agents (see below) | + +> **File-mode note:** when `KANDEV_LOGGING_OUTPUTPATH` is a file path, the active log file is created with mode `0600` (owner read/write only). Run any log shipper or sidecar as the same user, or use `stdout`/`stderr` and let the container runtime collect logs. > **Upgrading from a pre-`KANDEV_HOME_DIR` image?** The SQLite DB path moved from `/data/kandev.db` to `/data/data/kandev.db`. The backend auto-migrates the legacy `kandev.db` (plus any `-wal`/`-shm` files) on first boot — look for `Migrated SQLite database from pre-KANDEV_HOME_DIR location` in the logs. If you prefer to pin the old location instead, set `-e KANDEV_DATABASE_PATH=/data/kandev.db`. If you previously set `KANDEV_DATA_DIR`, replace it with `KANDEV_HOME_DIR`. diff --git a/docs/k8s.md b/docs/k8s.md index f76b075b40..4759464475 100644 --- a/docs/k8s.md +++ b/docs/k8s.md @@ -96,28 +96,37 @@ Kandev reads configuration via `KANDEV_`-prefixed environment variables (Viper). ### Core Settings -| Env Var | Default | Description | -|---------|---------|-------------| -| `KANDEV_SERVER_PORT` | `38429` | Server port (API + WebSocket + Web UI) | -| `KANDEV_HOME_DIR` | `/data` | Kandev home directory — contains `data/` (DB), `tasks/`, `worktrees/`, `repos/`, `sessions/`, and `lsp-servers/` | -| `KANDEV_DATABASE_DRIVER` | `sqlite` | Database driver (`sqlite` or `postgres`) | -| `KANDEV_DATABASE_PATH` | `$KANDEV_HOME_DIR/data/kandev.db` | SQLite database file path (override) | -| `KANDEV_DOCKER_ENABLED` | `false` | Enable Docker runtime for agents (requires DinD) | -| `KANDEV_LOG_LEVEL` | `info` | Log level: `debug`, `info`, `warn`, `error` | -| `KANDEV_LOGGING_FORMAT` | auto | Log format: `json` (auto-detected in K8s) or `text` | +See [`configuration.md`](./configuration.md) for the full reference (every backend knob and its YAML form). The tables below cover what's most commonly set in K8s manifests. + +| Env Var | Required | Default | Description | +|---------|----------|---------|-------------| +| `KANDEV_SERVER_PORT` | No | `38429` | Server port (API + WebSocket + Web UI) | +| `KANDEV_HOME_DIR` | No | `/data` | Kandev home directory - contains `data/` (DB), `tasks/`, `worktrees/`, `repos/`, `sessions/`, and `lsp-servers/` | +| `KANDEV_DATABASE_DRIVER` | No | `sqlite` | Database driver (`sqlite` or `postgres`) | +| `KANDEV_DATABASE_PATH` | No | `$KANDEV_HOME_DIR/data/kandev.db` | SQLite database file path (override) | +| `KANDEV_DOCKER_ENABLED` | No | `false` | Enable Docker runtime for agents (requires DinD) | +| `KANDEV_LOG_LEVEL` | No | `info` | Log level: `debug`, `info`, `warn`, `error` | +| `KANDEV_LOGGING_FORMAT` | No | auto | Log format: `json` (auto-detected in K8s) or `text` | +| `KANDEV_LOGGING_OUTPUTPATH` | No | `stdout` | Log destination: `stdout`, `stderr`, or a file path (rotated when a file) | +| `KANDEV_LOGGING_MAXSIZEMB` | No | `100` | Rotate the log file when it exceeds this size (MB). File output only. | +| `KANDEV_LOGGING_MAXBACKUPS` | No | `5` | Max rotated files to retain (`0` = unlimited). File output only. | +| `KANDEV_LOGGING_MAXAGEDAYS` | No | `30` | Max age of rotated files in days (`0` = unlimited). File output only. | +| `KANDEV_LOGGING_COMPRESS` | No | `true` | Gzip rotated files. File output only. | + +> **Logging in K8s:** prefer the default `stdout` so kubelet collects logs. If you set `KANDEV_LOGGING_OUTPUTPATH` to a file, the active log is created with mode `0600` (owner read/write only); any sidecar reading it must run as the same user. > **Upgrading from a pre-`KANDEV_HOME_DIR` deployment?** The SQLite DB path moved from `/data/kandev.db` to `/data/data/kandev.db`, and `KANDEV_DATA_DIR` is gone — point `KANDEV_HOME_DIR` at the same volume mount (`/data`) instead. (`KANDEV_WORKTREE_BASEPATH` still works as an explicit override if you want to keep worktrees outside the home dir.) The backend auto-migrates the legacy `kandev.db` (plus any `-wal`/`-shm` files) on first boot — look for `Migrated SQLite database from pre-KANDEV_HOME_DIR location` in the pod logs. If you'd rather pin the old path, set `KANDEV_DATABASE_PATH=/data/kandev.db` in the ConfigMap. ### PostgreSQL Settings (when `KANDEV_DATABASE_DRIVER=postgres`) -| Env Var | Default | Description | -|---------|---------|-------------| -| `KANDEV_DATABASE_HOST` | `localhost` | PostgreSQL host | -| `KANDEV_DATABASE_PORT` | `5432` | PostgreSQL port | -| `KANDEV_DATABASE_USER` | `kandev` | Database user | -| `KANDEV_DATABASE_PASSWORD` | (empty) | Database password | -| `KANDEV_DATABASE_DBNAME` | `kandev` | Database name | -| `KANDEV_DATABASE_SSLMODE` | `disable` | SSL mode | +| Env Var | Required | Default | Description | +|---------|----------|---------|-------------| +| `KANDEV_DATABASE_HOST` | No | `localhost` | PostgreSQL host | +| `KANDEV_DATABASE_PORT` | No | `5432` | PostgreSQL port | +| `KANDEV_DATABASE_USER` | Yes | `kandev` | Database user | +| `KANDEV_DATABASE_PASSWORD` | Usually | (empty) | Database password - required unless your Postgres allows passwordless auth | +| `KANDEV_DATABASE_DBNAME` | Yes | `kandev` | Database name | +| `KANDEV_DATABASE_SSLMODE` | No | `disable` | SSL mode (`disable`, `require`, `verify-ca`, `verify-full`) | ## Database: SQLite vs PostgreSQL From 472ffb874881df9c0d054f5cfffc241360202257 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Mon, 11 May 2026 22:58:54 +0100 Subject: [PATCH 2/3] fix(backend): list remote branches for provider-backed workspace repos (#876) --- apps/backend/cmd/kandev/main.go | 7 + apps/backend/cmd/kandev/services.go | 29 +++ .../task/service/repository_discovery.go | 72 ++++++- .../task/service/repository_discovery_test.go | 177 ++++++++++++++++++ apps/backend/internal/task/service/service.go | 31 +++ .../components/task-create-dialog-pill.tsx | 5 +- ...create-task-url-reopen-no-branches.spec.ts | 122 ++++++++++++ 7 files changed, 437 insertions(+), 6 deletions(-) create mode 100644 apps/web/e2e/tests/task/create-task-url-reopen-no-branches.spec.ts diff --git a/apps/backend/cmd/kandev/main.go b/apps/backend/cmd/kandev/main.go index 382f52e540..e2052a0a9e 100644 --- a/apps/backend/cmd/kandev/main.go +++ b/apps/backend/cmd/kandev/main.go @@ -326,6 +326,13 @@ func startAgentInfrastructure( log.Info("Repository cloner configured", zap.String("base_path", cfg.RepoClone.BasePath)) + // Let the task service treat the cloner's base path as an implicit + // allow-listed root. Without this, deploys that put the clone base + // outside HOME (e.g. KANDEV_REPOCLONE_BASEPATH=/data/repos in a + // container) fail the discoveryRoots() allow-list check and local + // branch listing returns nothing. + services.Task.SetRepoCloneLocation(repoCloner) + // ============================================ // ORCHESTRATOR // ============================================ diff --git a/apps/backend/cmd/kandev/services.go b/apps/backend/cmd/kandev/services.go index b12381a40d..dc1df28070 100644 --- a/apps/backend/cmd/kandev/services.go +++ b/apps/backend/cmd/kandev/services.go @@ -93,6 +93,14 @@ func provideServices(cfg *config.Config, log *logger.Logger, repos *Repositories linearSvc := initLinearService(dbPool, eventBus, repos.Secrets, log) slackSvc := initSlackService(dbPool, repos.Secrets, log) + // Plumb GitHub branch listing into the task service so provider-backed + // ("Remote") repos serve branches from the GitHub API rather than relying + // on a local clone that may not exist yet (or ever - some executors clone + // inside their own container). + if githubSvc != nil { + taskSvc.SetRemoteBranchLister(githubBranchListerAdapter{svc: githubSvc}) + } + return &Services{ Task: taskSvc, User: userSvc, @@ -379,3 +387,24 @@ func buildAgentProfileMatcher(repos *Repositories) wfmodels.AgentProfileMatcher return "" } } + +// githubBranchListerAdapter bridges github.Service to the task service's +// RemoteBranchLister interface. It maps github.RepoBranch into the task +// service's Branch shape with Type="remote" so the dialog renders branches +// the same way URL-mode does - bare names without an "origin/" prefix, since +// there is no checked-out clone whose tracking config could disambiguate. +type githubBranchListerAdapter struct { + svc *github.Service +} + +func (a githubBranchListerAdapter) ListRepoBranches(ctx context.Context, owner, repo string) ([]taskservice.Branch, error) { + remote, err := a.svc.ListRepoBranches(ctx, owner, repo) + if err != nil { + return nil, err + } + out := make([]taskservice.Branch, 0, len(remote)) + for _, b := range remote { + out = append(out, taskservice.Branch{Name: b.Name, Type: "remote"}) + } + return out, nil +} diff --git a/apps/backend/internal/task/service/repository_discovery.go b/apps/backend/internal/task/service/repository_discovery.go index 4a5e8b526e..16191fff92 100644 --- a/apps/backend/internal/task/service/repository_discovery.go +++ b/apps/backend/internal/task/service/repository_discovery.go @@ -58,6 +58,11 @@ var ErrPathNotAllowed = errors.New("path is not within an allowed root") // gitHEAD is the HEAD git ref. const gitHEAD = "HEAD" +// sourceTypeLocal is the Repository.SourceType value for on-machine repos +// (a path the user discovered or added manually). Provider-backed repos use +// other values like "provider". +const sourceTypeLocal = "local" + func (s *Service) DiscoverLocalRepositories(ctx context.Context, root string) (RepositoryDiscoveryResult, error) { roots := s.discoveryRoots() if root != "" { @@ -149,7 +154,16 @@ type BranchListResult struct { // (by id, path resolved from the DB row) or an on-machine folder (by path // directly). Exactly one of `repoID` or `path` should be set; the caller // (the HTTP handler) validates that. +// +// For provider-backed workspace repos (the "Remote" badge in the UI), the +// branches come from the remote API rather than a local clone. This makes +// the picker work the moment a URL is added - before the orchestrator's +// async clone finishes, and even when no clone ever happens because the +// chosen executor runs the agent in a container that clones on its own. func (s *Service) ListBranches(ctx context.Context, repoID, path string) ([]Branch, error) { + if remote, ok, err := s.listRemoteBranchesIfApplicable(ctx, repoID); ok { + return remote, err + } resolved, err := s.resolveBranchListingPath(ctx, repoID, path) if err != nil { return nil, err @@ -160,6 +174,14 @@ func (s *Service) ListBranches(ctx context.Context, repoID, path string) ([]Bran // ListBranchesWithCurrent is ListBranches plus the current-branch readout. // One method so the handler resolves the path once instead of twice. func (s *Service) ListBranchesWithCurrent(ctx context.Context, repoID, path string) (BranchListResult, error) { + if remote, ok, err := s.listRemoteBranchesIfApplicable(ctx, repoID); ok { + if err != nil { + return BranchListResult{}, err + } + // No CurrentBranch for remote: there's no working tree to check. + // The dialog falls back to its preferred-default-branch heuristic. + return BranchListResult{Branches: remote}, nil + } resolved, err := s.resolveBranchListingPath(ctx, repoID, path) if err != nil { return BranchListResult{}, err @@ -176,6 +198,36 @@ func (s *Service) ListBranchesWithCurrent(ctx context.Context, repoID, path stri }, nil } +// listRemoteBranchesIfApplicable returns (branches, true, err) when the repo +// should be answered from a remote provider, and (_, false, _) when the +// caller should fall through to the local-path code path. +// +// "Applicable" means: a repository id is supplied, the repo has a non-local +// source_type (i.e. is provider-backed), and the provider has a registered +// remote lister. The local-path arm of ListBranches stays untouched; this +// only widens the answer for the existing repository-id arm. +// +// Errors from GetRepository propagate with handled=true so callers +// short-circuit instead of falling through to resolveBranchListingPath, +// which would re-issue the same DB lookup. Repo-not-found falls through. +func (s *Service) listRemoteBranchesIfApplicable(ctx context.Context, repoID string) ([]Branch, bool, error) { + if repoID == "" || s.remoteBranchLister == nil { + return nil, false, nil + } + repo, err := s.repoEntities.GetRepository(ctx, repoID) + if err != nil { + return nil, true, err + } + if repo == nil { + return nil, false, nil + } + if repo.SourceType == sourceTypeLocal || repo.ProviderOwner == "" || repo.ProviderName == "" { + return nil, false, nil + } + branches, err := s.remoteBranchLister.ListRepoBranches(ctx, repo.ProviderOwner, repo.ProviderName) + return branches, true, err +} + // resolveBranchListingPath turns the request inputs into a validated // absolute path to list branches in. Both inputs go through the same // allowed-roots / dir / symlink validation via resolveAllowedLocalPath. @@ -292,14 +344,24 @@ func pathWithinRoots(abs string, roots []string) (string, error) { } func (s *Service) discoveryRoots() []string { + var roots []string if len(s.discoveryConfig.Roots) > 0 { - return normalizeRoots(s.discoveryConfig.Roots) + roots = append(roots, s.discoveryConfig.Roots...) + } else if home, err := os.UserHomeDir(); err == nil { + roots = append(roots, home) } - homeDir, err := os.UserHomeDir() - if err != nil { - return []string{} + // The orchestrator clones provider-backed repos into a configurable base + // path. When that base path sits outside HOME (e.g. /data/repos in a + // container deployment) it would otherwise be rejected by the allow-list + // and local branch listing would silently return nothing. Adding it here + // keeps the allow-list narrow while still covering kandev's own clone + // destination. + if s.repoCloneLocation != nil { + if base, err := s.repoCloneLocation.ExpandedBasePath(); err == nil && base != "" { + roots = append(roots, base) + } } - return []string{filepath.Clean(homeDir)} + return normalizeRoots(roots) } func (s *Service) discoveryMaxDepth() int { diff --git a/apps/backend/internal/task/service/repository_discovery_test.go b/apps/backend/internal/task/service/repository_discovery_test.go index 5bd13c8869..641badaccc 100644 --- a/apps/backend/internal/task/service/repository_discovery_test.go +++ b/apps/backend/internal/task/service/repository_discovery_test.go @@ -13,6 +13,7 @@ import ( "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/db" "github.com/kandev/kandev/internal/events/bus" + "github.com/kandev/kandev/internal/task/models" "github.com/kandev/kandev/internal/task/repository" ) @@ -637,3 +638,179 @@ func newDiscoveryService(t *testing.T, root string) *Service { MaxDepth: 6, }) } + +// --- listRemoteBranchesIfApplicable + discoveryRoots routing --- + +// stubRemoteLister captures the call args and returns canned branches/err. +type stubRemoteLister struct { + branches []Branch + err error + calls int +} + +func (s *stubRemoteLister) ListRepoBranches(_ context.Context, _, _ string) ([]Branch, error) { + s.calls++ + return s.branches, s.err +} + +// stubRepoErrors embeds a real RepositoryEntityRepository and overrides +// GetRepository to return a forced error - lets us cover the DB-error +// branch without standing up a broken sqlite. +type stubRepoErrors struct { + repository.RepositoryEntityRepository + getErr error +} + +func (s *stubRepoErrors) GetRepository(_ context.Context, _ string) (*models.Repository, error) { + return nil, s.getErr +} + +func TestListBranches_RoutesProviderRepoToRemoteLister(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + if err := repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "ws"}); err != nil { + t.Fatalf("create workspace: %v", err) + } + mustCreateRepo(t, repo, &models.Repository{ + ID: "remote-1", + WorkspaceID: "ws-1", + Name: "owner/repo", + SourceType: "provider", + Provider: "github", + ProviderOwner: "owner", + ProviderName: "repo", + }) + lister := &stubRemoteLister{branches: []Branch{{Name: "main", Type: "remote"}, {Name: "develop", Type: "remote"}}} + svc.SetRemoteBranchLister(lister) + + got, err := svc.ListBranches(ctx, "remote-1", "") + if err != nil { + t.Fatalf("ListBranches: %v", err) + } + if lister.calls != 1 { + t.Fatalf("remote lister calls = %d, want 1", lister.calls) + } + if len(got) != 2 || got[0].Name != "main" || got[1].Name != "develop" { + t.Fatalf("unexpected branches: %+v", got) + } +} + +func TestListBranches_FallsThroughWhenNoRemoteListerWired(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + if err := repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "ws"}); err != nil { + t.Fatalf("create workspace: %v", err) + } + mustCreateRepo(t, repo, &models.Repository{ + ID: "remote-2", WorkspaceID: "ws-1", Name: "o/r", + SourceType: "provider", Provider: "github", ProviderOwner: "o", ProviderName: "r", + }) + // No SetRemoteBranchLister call. listRemoteBranchesIfApplicable should + // return ok=false and the call falls through to the local-path arm, + // which errors out on empty local_path. + _, err := svc.ListBranches(ctx, "remote-2", "") + if err == nil { + t.Fatal("expected local-path error when remote lister unwired") + } +} + +func TestListBranches_FallsThroughForLocalSourceType(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + if err := repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "ws"}); err != nil { + t.Fatalf("create workspace: %v", err) + } + mustCreateRepo(t, repo, &models.Repository{ + ID: "local-1", WorkspaceID: "ws-1", Name: "local", + SourceType: sourceTypeLocal, Provider: "github", ProviderOwner: "o", ProviderName: "r", + }) + lister := &stubRemoteLister{branches: []Branch{{Name: "main"}}} + svc.SetRemoteBranchLister(lister) + + // Local source-type repos must use the local-path arm even with provider + // info populated. local_path is empty here, so we expect an error - what + // matters for the test is that the remote lister is never consulted. + _, _ = svc.ListBranches(ctx, "local-1", "") + if lister.calls != 0 { + t.Fatalf("remote lister called for source_type=local: calls = %d", lister.calls) + } +} + +func TestListBranches_FallsThroughOnMissingProviderInfo(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + if err := repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "ws"}); err != nil { + t.Fatalf("create workspace: %v", err) + } + mustCreateRepo(t, repo, &models.Repository{ + ID: "remote-3", WorkspaceID: "ws-1", Name: "no-owner", + SourceType: "provider", Provider: "github", ProviderOwner: "", ProviderName: "repo", + }) + lister := &stubRemoteLister{branches: []Branch{{Name: "main"}}} + svc.SetRemoteBranchLister(lister) + + _, _ = svc.ListBranches(ctx, "remote-3", "") + if lister.calls != 0 { + t.Fatalf("remote lister called for repo with no owner: calls = %d", lister.calls) + } +} + +func TestListBranches_PropagatesRemoteListerError(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + if err := repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "ws"}); err != nil { + t.Fatalf("create workspace: %v", err) + } + mustCreateRepo(t, repo, &models.Repository{ + ID: "remote-4", WorkspaceID: "ws-1", Name: "o/r", + SourceType: "provider", Provider: "github", ProviderOwner: "o", ProviderName: "r", + }) + wantErr := errors.New("rate limited") + svc.SetRemoteBranchLister(&stubRemoteLister{err: wantErr}) + + if _, err := svc.ListBranches(ctx, "remote-4", ""); !errors.Is(err, wantErr) { + t.Fatalf("ListBranches err = %v, want %v", err, wantErr) + } +} + +func TestListBranches_PropagatesGetRepositoryError(t *testing.T) { + svc, _, repo := createTestService(t) + wrapped := &stubRepoErrors{RepositoryEntityRepository: repo, getErr: errors.New("db down")} + // Swap repoEntities for the wrapper to force the error path. Direct field + // access stays inside the package, which is fine for a white-box test. + svc.repoEntities = wrapped + svc.SetRemoteBranchLister(&stubRemoteLister{}) + + _, err := svc.ListBranches(context.Background(), "repo-id", "") + if err == nil || err.Error() != "db down" { + t.Fatalf("ListBranches err = %v, want db down", err) + } +} + +// --- discoveryRoots extension --- + +type stubCloneLocation struct{ path string } + +func (s stubCloneLocation) ExpandedBasePath() (string, error) { return s.path, nil } + +func TestDiscoveryRoots_IncludesCloneBasePath(t *testing.T) { + svc, _, _ := createTestService(t) + cloneDir := t.TempDir() + svc.SetRepoCloneLocation(stubCloneLocation{path: cloneDir}) + + roots := svc.discoveryRoots() + normalizedClone := filepath.Clean(cloneDir) + for _, r := range roots { + if r == normalizedClone { + return + } + } + t.Fatalf("clone base path %q missing from discoveryRoots %v", normalizedClone, roots) +} + +func mustCreateRepo(t *testing.T, repo repository.RepositoryEntityRepository, r *models.Repository) { + t.Helper() + if err := repo.CreateRepository(context.Background(), r); err != nil { + t.Fatalf("create repository: %v", err) + } +} diff --git a/apps/backend/internal/task/service/service.go b/apps/backend/internal/task/service/service.go index 322e6f815a..04b321fc46 100644 --- a/apps/backend/internal/task/service/service.go +++ b/apps/backend/internal/task/service/service.go @@ -136,6 +136,8 @@ type Service struct { startStepResolver StartStepResolver quickChatDir string // Directory for quick-chat workspaces (e.g., ~/.kandev/quick-chat) branchFetcher *branchFetcher + remoteBranchLister RemoteBranchLister + repoCloneLocation RepoCloneLocation } // NewService creates a new task service @@ -196,3 +198,32 @@ func (s *Service) SetStartStepResolver(resolver StartStepResolver) { func (s *Service) SetQuickChatDir(dir string) { s.quickChatDir = dir } + +// RemoteBranchLister fetches branches from a provider's remote (e.g. GitHub +// API) without needing a local clone. Used by ListBranches so a repo that is +// registered as remote ("Remote" badge in the UI) can serve branches before +// or even without the orchestrator finishing its clone. +type RemoteBranchLister interface { + ListRepoBranches(ctx context.Context, owner, repo string) ([]Branch, error) +} + +// SetRemoteBranchLister wires the remote branch source. Currently only GitHub +// is plumbed; other providers can be added by extending the adapter. +func (s *Service) SetRemoteBranchLister(lister RemoteBranchLister) { + s.remoteBranchLister = lister +} + +// RepoCloneLocation reports the base path the orchestrator clones repos into +// (e.g. ~/.kandev/repos or KANDEV_REPOCLONE_BASEPATH). Listing local branches +// for a cloned repo requires that path to be allow-listed by +// discoveryRoots(); without this hook clones to a custom basepath silently +// fall outside the allow-list and branch listing returns no results. +type RepoCloneLocation interface { + ExpandedBasePath() (string, error) +} + +// SetRepoCloneLocation wires the cloner so its base path is treated as an +// implicit discovery root. +func (s *Service) SetRepoCloneLocation(loc RepoCloneLocation) { + s.repoCloneLocation = loc +} diff --git a/apps/web/components/task-create-dialog-pill.tsx b/apps/web/components/task-create-dialog-pill.tsx index c4054d7a81..9379a2335f 100644 --- a/apps/web/components/task-create-dialog-pill.tsx +++ b/apps/web/components/task-create-dialog-pill.tsx @@ -282,7 +282,10 @@ export function branchToOption(b: Branch): PillOption { // from local branches with the same short name (e.g. "main" vs "origin/main"). // Without the prefix, the dropdown shows two indistinguishable rows. const display = b.type === "remote" && b.remote ? `${b.remote}/${b.name}` : b.name; - const badge = b.type === "local" ? "local" : (b.remote ?? "remote"); + // `||` (not `??`) so an empty-string `remote` falls back too. Provider-backed + // workspace repos (URL-added) list branches without a tracking remote, so the + // backend sends `remote: ""`; `??` would render an invisible empty badge. + const badge = b.type === "local" ? "local" : b.remote || "remote"; return { value: display, label: display, diff --git a/apps/web/e2e/tests/task/create-task-url-reopen-no-branches.spec.ts b/apps/web/e2e/tests/task/create-task-url-reopen-no-branches.spec.ts new file mode 100644 index 0000000000..6c0b2de76d --- /dev/null +++ b/apps/web/e2e/tests/task/create-task-url-reopen-no-branches.spec.ts @@ -0,0 +1,122 @@ +import { test, expect } from "../../fixtures/test-base"; +import { KanbanPage } from "../../pages/kanban-page"; + +// Regression test for the user-reported bug: +// +// 1. Open the create-task dialog, switch to GitHub URL mode, enter a URL. +// 2. Submit the task - the URL flow registers the repo in the workspace as +// a remote (provider-backed) repository, without a local clone. +// 3. Reopen the dialog - the repo now appears in the workspace dropdown. +// Pre-fix: picking it showed "no branches" with the +// "No branches available for this repository." tooltip, because the +// backend tried to list branches from an empty local_path and the +// frontend cached the failure as a successful (empty) load. +// +// Fix: branch listing for a provider-backed repo now goes through the +// GitHub API (mocked in e2e) instead of the local clone. The local clone is +// only consulted when source_type=local. This test deliberately uses a fake +// owner/repo so the orchestrator's background `gh repo clone` fails - that +// keeps local_path empty, proving the remote-first path is what's serving +// the branches. +test.describe("Create-task URL flow - branches after reopen", () => { + test.describe.configure({ retries: 1 }); + + test("repo added via GitHub URL still lists branches when re-picked from the workspace dropdown", async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(60_000); + + // Fake owner/repo so the background clone in the orchestrator fails with + // 404 and local_path stays empty. The bug only repros when local listing + // can't help - that's the path the fix has to keep working. + const owner = "kandev-e2e-no-such-owner"; + const repo = "kandev-e2e-no-such-repo"; + const repoFullName = `${owner}/${repo}`; + const taskTitle = "URL repo reopen bug"; + + await apiClient.mockGitHubAddBranches(owner, repo, [ + { name: "main" }, + { name: "develop" }, + { name: "feature/test" }, + ]); + + const kanban = new KanbanPage(testPage); + await kanban.goto(); + + // ── First open: enter URL, pick worktree executor, submit task ── + await kanban.createTaskButton.first().click(); + const dialog = testPage.getByTestId("create-task-dialog"); + await expect(dialog).toBeVisible(); + + await testPage.getByTestId("toggle-github-url").click(); + await testPage.getByTestId("github-url-input").fill(`https://github.com/${repoFullName}`); + + await testPage.getByTestId("task-title-input").fill(taskTitle); + await testPage.getByTestId("task-description-input").fill("/e2e:simple-message"); + + await selectWorktreeExecutor(testPage); + + const startBtn = testPage.getByTestId("submit-start-agent"); + await expect(startBtn).toBeEnabled({ timeout: 15_000 }); + await startBtn.click(); + await expect(dialog).not.toBeVisible({ timeout: 10_000 }); + + // Wait for the workspace to surface the URL-added repo so the second + // dialog open sees it in the dropdown. + await expect + .poll( + async () => { + const res = await apiClient.rawRequest( + "GET", + `/api/v1/workspaces/${seedData.workspaceId}/repositories`, + ); + const body = (await res.json()) as { repositories?: Array<{ name: string }> }; + return (body.repositories ?? []).some((r) => r.name === repoFullName); + }, + { timeout: 15_000, intervals: [200, 500, 1000] }, + ) + .toBe(true); + + // ── Second open: pick the same repo from the workspace dropdown ── + await kanban.createTaskButton.first().click(); + await expect(dialog).toBeVisible(); + + await selectWorktreeExecutor(testPage); + + await testPage.getByTestId("repo-chip-trigger").first().click(); + await testPage + .getByRole("option", { name: new RegExp(`^${repoFullName}\\b`) }) + .first() + .click(); + + // The branch chip should become enabled once the remote-branch fetch + // resolves. Pre-fix this never happened: the listing returned an error + // and the chip stayed disabled with the "no branches" tooltip. + const branchChip = testPage.getByTestId("branch-chip-trigger").first(); + await expect(branchChip).toBeEnabled({ timeout: 10_000 }); + + // Open the dropdown and verify every mocked branch is present. Substring + // name matching (string, not regex) tolerates the "remote" badge that the + // pill renders alongside the branch name in the option's accessible name. + await branchChip.click(); + for (const name of ["main", "develop", "feature/test"]) { + await expect(testPage.getByRole("option", { name })).toBeVisible({ timeout: 5_000 }); + } + }); +}); + +// Pin the executor on every dialog open. This test runs in a shard alongside +// create-task-branch-selector specs that leave local-executor profiles +// behind, and a stale local default trips the local-executor's +// currentLocalBranch autoselect race. Worktree is the cleanest neutral +// choice and exercises the same remote-branch-listing path. +async function selectWorktreeExecutor(page: import("@playwright/test").Page): Promise { + const selector = page.getByTestId("executor-profile-selector"); + await selector.click(); + await page + .getByRole("option", { name: /Worktree/i }) + .first() + .click(); +} From ed056a22b52488a744318276eab12228b50caa6e Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 11 May 2026 21:59:51 +0000 Subject: [PATCH 3/3] release: 0.44.0 --- CHANGELOG.md | 15 +++++++++++++++ apps/cli/package.json | 2 +- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ad7f6c726..f4b61a989d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,21 @@ All notable changes to Kandev. +## 0.44.0 - 2026-05-11 + +### Features + +- persist N-entry FIFO message queue ([#864](https://github.com/kdlbs/kandev/pull/864)) +- add size/age/backup rotation for file output ([#874](https://github.com/kdlbs/kandev/pull/874)) + +### Bug Fixes + +- list remote branches for provider-backed workspace repos ([#876](https://github.com/kdlbs/kandev/pull/876)) + +### Refactoring + +- close rotating sink on shutdown, add config docs ([#877](https://github.com/kdlbs/kandev/pull/877)) + ## 0.43.0 - 2026-05-11 ### Bug Fixes diff --git a/apps/cli/package.json b/apps/cli/package.json index 80cc07b9cd..bc66a88900 100644 --- a/apps/cli/package.json +++ b/apps/cli/package.json @@ -1,6 +1,6 @@ { "name": "kandev", - "version": "0.43.0", + "version": "0.44.0", "private": false, "description": "Launcher for Kandev — manage tasks, orchestrate agents, review changes, and ship value", "license": "AGPL-3.0-only",