diff --git a/cmd/gohomed/main.go b/cmd/gohomed/main.go index 5f3595a..2f86504 100644 --- a/cmd/gohomed/main.go +++ b/cmd/gohomed/main.go @@ -27,7 +27,6 @@ func run() int { adminPort = flag.Int("admin-port", 9190, "HTTP admin port for /metrics and /health") snapshotEveryEvt = flag.Int("snapshot-every-events", 10_000, "snapshot cadence: events since last") snapshotEveryDur = flag.Duration("snapshot-every-period", time.Hour, "snapshot cadence: wall-clock period") - driversTOML = flag.String("drivers-toml", "", "path to drivers.toml (default /drivers.toml)") configDir = flag.String("config-dir", "", "config directory with main.pkl (default /config)") ) flag.Parse() @@ -56,7 +55,6 @@ func run() int { AdminPort: *adminPort, SnapshotEveryEvents: *snapshotEveryEvt, SnapshotEveryPeriod: *snapshotEveryDur, - DriversTOMLPath: *driversTOML, ConfigDir: *configDir, } d := daemon.New(cfg, logger, metrics) diff --git a/docs/design/plans/2026-05-01-dynamic-carport-management.md b/docs/design/plans/2026-05-01-dynamic-carport-management.md new file mode 100644 index 0000000..4e669d4 --- /dev/null +++ b/docs/design/plans/2026-05-01-dynamic-carport-management.md @@ -0,0 +1,891 @@ +# Dynamic Carport Management Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Implement `RegisterInstance`/`UnregisterInstance` on `carport.Host` and wire it as the real `CarportManager` in the daemon, so driver instances declared in `main.pkl` are spawned and supervised at runtime. + +**Architecture:** Add a `binary` field to the Pkl `DriverInstance` base class and `DriverInstanceConfig` proto so the executable path flows through the evaluation pipeline. Implement `RegisterInstance`/`UnregisterInstance` on `carport.Host` using the existing `launchLifecycle`/`shutdownInstance` machinery. Replace the `nopCarportManager` stub in `daemon.go` with the real `d.carport`. Drivers from `drivers.toml` and `main.pkl` coexist; IDs must not overlap — `RegisterInstance` returns an error on duplicate. + +**Tech Stack:** Go, `internal/carport`, `internal/config`, `proto/gohome/config/v1`; `task proto` for codegen; `task build` / `task test` throughout. + +--- + +## Codebase orientation + +Read these files before starting: + +| File | Why | +|---|---| +| `internal/carport/carport.go` | `Host` struct + `Start`/`Stop` — new methods go here | +| `internal/carport/supervisor.go` | `launchLifecycle(ctx, m)` and `shutdownInstance(ctx, m)` — reused unchanged | +| `internal/carport/config.go` | `Instance` struct and lifecycle defaults — extract `defaultLifecycleConfig()` here | +| `internal/config/manager.go` | `CarportManager` interface — adding `binary` param to `RegisterInstance` | +| `internal/config/evaluator.go` | JSON parsing of driver instances — add `binary` extraction | +| `internal/daemon/daemon.go` | Phase 4.5 (carport) then 4.6 (config) — swap nop for real carport at line ~220 | + +--- + +## File map + +### Modified files + +| File | Change | +|---|---| +| `internal/config/pkl/gohome/carport.pkl` | Add `binary: String(!isEmpty)` to `DriverInstance` | +| `proto/gohome/config/v1/snapshot.proto` | Add `string binary = 5` to `DriverInstanceConfig` | +| `internal/config/evaluator.go` | Extract `binary` from driver instance JSON; populate proto field | +| `internal/config/manager.go` | Add `binary string` to `CarportManager.RegisterInstance`; pass it in `Apply` | +| `internal/config/manager_test.go` | Update `fakeCarport.RegisterInstance` signature | +| `internal/carport/config.go` | Add `defaultLifecycleConfig()` helper | +| `internal/carport/carport.go` | Add `ctx` field; implement `RegisterInstance` + `UnregisterInstance` | +| `internal/daemon/daemon.go` | Pass `d.carport` to `config.NewManager`; remove `nopCarportManager`; add compile-time assertion | +| `internal/cli/config.go` | Update `nopCarportManager.RegisterInstance` signature | + +### New files + +| File | Responsibility | +|---|---| +| `internal/carport/dynamic_test.go` | Unit tests for `RegisterInstance`/`UnregisterInstance` error cases | + +--- + +## Task 1: Add `binary` to Pkl schema, proto, and evaluator + +**Files:** +- Modify: `internal/config/pkl/gohome/carport.pkl` +- Modify: `proto/gohome/config/v1/snapshot.proto` +- Modify: `internal/config/evaluator.go` + +- [ ] **Step 1: Add `binary` to `DriverInstance` in carport.pkl** + +Full file content for `internal/config/pkl/gohome/carport.pkl`: + +```pkl +module gohome.carport + +// DriverInstance is the base class for all driver instance configs. +// Driver authors extend this with their own typed fields. +abstract class DriverInstance { + id: String(!isEmpty) + driverName: String(!isEmpty) + binary: String(!isEmpty) +} +``` + +- [ ] **Step 2: Add `binary` to `DriverInstanceConfig` proto** + +In `proto/gohome/config/v1/snapshot.proto`, update `DriverInstanceConfig`: + +```protobuf +message DriverInstanceConfig { + string id = 1; + string driver_name = 2; + bytes config_hash = 3; + bytes params = 4; + string binary = 5; +} +``` + +- [ ] **Step 3: Regenerate proto** + +```bash +task proto +``` + +Expected: `gen/gohome/config/v1/snapshot.pb.go` updated, `GetBinary()` accessor present. + +- [ ] **Step 4: Extract `binary` in evaluator.go** + +In `internal/config/evaluator.go`, find the anonymous struct inside `parseConfigJSON` that extracts per-instance fields: + +```go +var base struct { + ID string `json:"id"` + DriverName string `json:"driverName"` +} +``` + +Replace with: + +```go +var base struct { + ID string `json:"id"` + DriverName string `json:"driverName"` + Binary string `json:"binary"` +} +``` + +In the same block where the proto struct is constructed, add `Binary`: + +```go +snap.DriverInstances = append(snap.DriverInstances, &configpb.DriverInstanceConfig{ + Id: base.ID, + DriverName: base.DriverName, + Binary: base.Binary, + ConfigHash: h[:], + Params: rawInst, +}) +``` + +- [ ] **Step 5: Build and test** + +```bash +task build && task test +``` + +Expected: compiles; all tests pass. No behaviour change yet. + +- [ ] **Step 6: Commit** + +```bash +git add internal/config/pkl/gohome/carport.pkl proto/gohome/config/v1/snapshot.proto gen/ internal/config/evaluator.go +git commit -m "feat(carport,config): add binary field to DriverInstance and DriverInstanceConfig proto" +``` + +--- + +## Task 2: Update CarportManager interface and all call sites + +**Files:** +- Modify: `internal/config/manager.go` +- Modify: `internal/config/manager_test.go` +- Modify: `internal/daemon/daemon.go` +- Modify: `internal/cli/config.go` + +- [ ] **Step 1: Update the interface in manager.go** + +In `internal/config/manager.go`, change `CarportManager`: + +```go +type CarportManager interface { + RegisterInstance(ctx context.Context, id, driverName, binary string, params []byte) error + UnregisterInstance(ctx context.Context, id string) error +} +``` + +- [ ] **Step 2: Pass binary in Apply** + +Still in `manager.go`, in the `Apply` method update both `RegisterInstance` calls: + +```go +// In DriverInstancesAdded loop: +if err := m.carportMgr.RegisterInstance(ctx, di.GetId(), di.GetDriverName(), di.GetBinary(), di.GetParams()); err != nil { + return fmt.Errorf("register %q: %w", id, err) +} + +// In DriverInstancesChanged loop (re-register after unregister): +if err := m.carportMgr.RegisterInstance(ctx, di.GetId(), di.GetDriverName(), di.GetBinary(), di.GetParams()); err != nil { + return fmt.Errorf("re-register changed %q: %w", id, err) +} +``` + +- [ ] **Step 3: Update fakeCarport in manager_test.go** + +In `internal/config/manager_test.go`, update `fakeCarport`: + +```go +type fakeCarport struct { + registered []string + unregistered []string +} + +func (f *fakeCarport) RegisterInstance(_ context.Context, id, _, _ string, _ []byte) error { + f.registered = append(f.registered, id) + return nil +} + +func (f *fakeCarport) UnregisterInstance(_ context.Context, id string) error { + f.unregistered = append(f.unregistered, id) + return nil +} +``` + +- [ ] **Step 4: Update nopCarportManager in daemon.go** + +In `internal/daemon/daemon.go`, update the method signature: + +```go +func (n *nopCarportManager) RegisterInstance(_ context.Context, _, _, _ string, _ []byte) error { + return nil +} +``` + +- [ ] **Step 5: Update nopCarportManager in cli/config.go** + +In `internal/cli/config.go`: + +```go +func (n *nopCarportManager) RegisterInstance(_ context.Context, _, _, _ string, _ []byte) error { + return nil +} +``` + +- [ ] **Step 6: Build and test** + +```bash +task build && task test +``` + +Expected: compiles; all tests pass. + +- [ ] **Step 7: Commit** + +```bash +git add internal/config/manager.go internal/config/manager_test.go internal/daemon/daemon.go internal/cli/config.go +git commit -m "feat(config): add binary param to CarportManager.RegisterInstance" +``` + +--- + +## Task 3: Add defaultLifecycleConfig helper to carport/config.go + +**Files:** +- Modify: `internal/carport/config.go` + +- [ ] **Step 1: Add the helper** + +In `internal/carport/config.go`, add `defaultLifecycleConfig()` after the `intd` helper at the bottom of the file: + +```go +// defaultLifecycleConfig returns the defaults used for dynamically registered +// instances (those coming from main.pkl rather than drivers.toml). +func defaultLifecycleConfig() LifecycleConfig { + return LifecycleConfig{ + HandshakeDeadline: 5 * time.Second, + HealthProbeInterval: 15 * time.Second, + HealthProbeTimeout: 3 * time.Second, + HealthFailuresToRestart: 3, + ShutdownGrace: 10 * time.Second, + RestartBackoffInitial: time.Second, + RestartBackoffMax: 60 * time.Second, + RestartBudgetWindow: 10 * time.Minute, + RestartBudgetMax: 10, + } +} +``` + +- [ ] **Step 2: Build and test** + +```bash +task build && task test +``` + +Expected: passes. `LoadConfig` behaviour is unchanged. + +- [ ] **Step 3: Commit** + +```bash +git add internal/carport/config.go +git commit -m "refactor(carport): extract defaultLifecycleConfig helper" +``` + +--- + +## Task 4: Implement RegisterInstance and UnregisterInstance on carport.Host + +**Files:** +- Modify: `internal/carport/carport.go` +- Create: `internal/carport/dynamic_test.go` + +- [ ] **Step 1: Write failing tests** + +Create `internal/carport/dynamic_test.go`: + +```go +package carport + +import ( + "context" + "testing" +) + +func TestRegisterInstance_HostNotStarted(t *testing.T) { + h := &Host{ + instances: map[string]*managedInstance{}, + stopped: make(chan struct{}), + // ctx is nil — host not started + } + err := h.RegisterInstance(context.Background(), "new", "fake", "/bin/fake", nil) + if err == nil { + t.Fatal("expected error when host not started (ctx nil)") + } +} + +func TestRegisterInstance_DuplicateID(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + h := &Host{ + instances: map[string]*managedInstance{ + "existing": {cfg: Instance{ID: "existing"}}, + }, + stopped: make(chan struct{}), + ctx: ctx, + } + err := h.RegisterInstance(context.Background(), "existing", "fake", "/bin/fake", nil) + if err == nil { + t.Fatal("expected error for duplicate instance ID") + } +} + +func TestRegisterInstance_HostStopped(t *testing.T) { + stopped := make(chan struct{}) + close(stopped) // host is already stopped + h := &Host{ + instances: map[string]*managedInstance{}, + stopped: stopped, + ctx: context.Background(), + } + err := h.RegisterInstance(context.Background(), "new", "fake", "/bin/fake", nil) + if err == nil { + t.Fatal("expected error when host is stopped") + } +} + +func TestUnregisterInstance_NotFound(t *testing.T) { + h := &Host{ + instances: map[string]*managedInstance{}, + stopped: make(chan struct{}), + } + err := h.UnregisterInstance(context.Background(), "missing") + if err == nil { + t.Fatal("expected error for missing instance") + } +} +``` + +- [ ] **Step 2: Run tests to verify they fail** + +```bash +go test ./internal/carport/... -run "TestRegisterInstance|TestUnregisterInstance" -v +``` + +Expected: FAIL — `RegisterInstance` and `UnregisterInstance` undefined; `ctx` field missing. + +- [ ] **Step 3: Add ctx field to Host** + +In `internal/carport/carport.go`, add `ctx` to the `Host` struct (after `metrics`): + +```go +type Host struct { + cfg HostConfig + cfgData *Config + + db *sql.DB + store *eventstore.Store + router *Router + logger *slog.Logger + metrics *observability.Metrics + + ctx context.Context // root context for lifecycle goroutines; set by Start + + mu sync.RWMutex + instances map[string]*managedInstance + + stopOnce sync.Once + stopped chan struct{} +} +``` + +- [ ] **Step 4: Store ctx in Start** + +In `carport.go`, update `Start` to store the context: + +```go +func (h *Host) Start(ctx context.Context) error { + h.ctx = ctx + for _, inst := range h.cfgData.Instances { + if !inst.Enabled { + continue + } + m := &managedInstance{cfg: inst, state: StateDeclared} + h.mu.Lock() + h.instances[inst.ID] = m + h.mu.Unlock() + h.launchLifecycle(ctx, m) + } + return nil +} +``` + +- [ ] **Step 5: Implement RegisterInstance and UnregisterInstance** + +In `internal/carport/carport.go`, add these methods after `Stop` (and before the `// launchLifecycle and shutdownInstance are implemented in supervisor.go` comment): + +```go +// RegisterInstance adds a new driver instance and begins its lifecycle goroutine. +// Returns an error if an instance with that ID is already registered, if the host +// has not been started, or if the host has been stopped. +// IDs must not conflict with instances already running from drivers.toml. +func (h *Host) RegisterInstance(_ context.Context, id, driverName, binary string, params []byte) error { + if h.ctx == nil { + return fmt.Errorf("carport host not started") + } + select { + case <-h.stopped: + return fmt.Errorf("carport host is stopped") + default: + } + h.mu.Lock() + if _, exists := h.instances[id]; exists { + h.mu.Unlock() + return fmt.Errorf("instance %q already registered", id) + } + inst := Instance{ + ID: id, + Binary: binary, + Enabled: true, + ConfigJSON: params, + Lifecycle: defaultLifecycleConfig(), + } + m := &managedInstance{cfg: inst, state: StateDeclared} + h.instances[id] = m + h.mu.Unlock() + h.launchLifecycle(h.ctx, m) + return nil +} + +// UnregisterInstance stops and removes a driver instance by ID. +// Returns an error if the instance is not found. +func (h *Host) UnregisterInstance(_ context.Context, id string) error { + h.mu.Lock() + m, exists := h.instances[id] + if !exists { + h.mu.Unlock() + return fmt.Errorf("instance %q not found", id) + } + delete(h.instances, id) + h.mu.Unlock() + // Use Background context so shutdown isn't cut short by the caller's deadline. + // Shutdown duration is bounded by m.cfg.Lifecycle.ShutdownGrace. + h.shutdownInstance(context.Background(), m) + return nil +} +``` + +`fmt` is already imported in `carport.go`. `context` is already imported. + +- [ ] **Step 6: Run tests to verify they pass** + +```bash +go test ./internal/carport/... -run "TestRegisterInstance|TestUnregisterInstance" -v +``` + +Expected: all four tests PASS. + +- [ ] **Step 7: Full build and test** + +```bash +task build && task test +``` + +Expected: all pass. + +- [ ] **Step 8: Commit** + +```bash +git add internal/carport/carport.go internal/carport/dynamic_test.go +git commit -m "feat(carport): implement RegisterInstance and UnregisterInstance on Host" +``` + +--- + +## Task 5: Wire real carport into daemon + +**Files:** +- Modify: `internal/daemon/daemon.go` + +- [ ] **Step 1: Replace nopCarportManager with d.carport** + +In `internal/daemon/daemon.go`, find the config manager construction at phase 4.6 (search for `nopCarportManager`): + +```go +cfgMgr, err := config.NewManager(ctx, configDir, d.store, &nopCarportManager{}) +``` + +Replace with: + +```go +cfgMgr, err := config.NewManager(ctx, configDir, d.store, d.carport) +``` + +- [ ] **Step 2: Remove the nopCarportManager type** + +Delete the entire `nopCarportManager` block (the comment, struct, and two methods): + +```go +// DELETE this entire block: +// nopCarportManager satisfies config.CarportManager until carport.Host gains +// RegisterInstance/UnregisterInstance methods (C5+). +type nopCarportManager struct{} + +func (n *nopCarportManager) RegisterInstance(_ context.Context, _, _, _ string, _ []byte) error { + return nil +} +func (n *nopCarportManager) UnregisterInstance(_ context.Context, _ string) error { + return nil +} +``` + +- [ ] **Step 3: Add compile-time interface assertion** + +In `internal/daemon/daemon.go`, add after the `import` block: + +```go +// Compile-time assertion: *carport.Host must satisfy config.CarportManager. +var _ config.CarportManager = (*carport.Host)(nil) +``` + +- [ ] **Step 4: Build** + +```bash +task build +``` + +Expected: compiles. The assertion confirms the interface is satisfied. If it fails, the missing method will be named in the error. + +- [ ] **Step 5: Full test suite with race detector** + +```bash +task test && task test:race +``` + +Expected: all pass, no races. + +- [ ] **Step 6: Commit** + +```bash +git add internal/daemon/daemon.go +git commit -m "feat(daemon): wire carport.Host as real CarportManager, remove nop stub" +``` + +--- + +## Task 6: Remove drivers.toml support + +Now that `main.pkl` is the canonical driver source, remove the static TOML loading path entirely. + +**Files:** +- Modify: `internal/carport/config.go` — delete `Config` struct and `LoadConfig()` +- Delete: `internal/carport/config_test.go` — only tested `LoadConfig` +- Modify: `internal/carport/carport.go` — remove `cfgData` field and TOML loop from `Start`; remove `DriversTOMLPath` from `HostConfig` +- Modify: `internal/daemon/config.go` — remove `DriversTOMLPath` field and its default +- Modify: `internal/daemon/daemon.go` — remove `driversTOML` resolution block and flag passthrough +- Modify: `cmd/gohomed/main.go` — remove `--drivers-toml` flag + +- [ ] **Step 1: Prune config.go — delete LoadConfig and Config** + +In `internal/carport/config.go`, delete the `Config` struct and `LoadConfig` function. Keep `LifecycleConfig`, `Instance`, `defaultLifecycleConfig`, `dur`, and `intd`. The file should look like this after the edit: + +```go +package carport + +import ( + "time" +) + +// LifecycleConfig tunes per-instance timing and restart policy. +// Zero values get replaced by defaults during load. +type LifecycleConfig struct { + HandshakeDeadline time.Duration + HealthProbeInterval time.Duration + HealthProbeTimeout time.Duration + HealthFailuresToRestart int + ShutdownGrace time.Duration + RestartBackoffInitial time.Duration + RestartBackoffMax time.Duration + RestartBudgetWindow time.Duration + RestartBudgetMax int +} + +// Instance is a single driver instance declaration. +type Instance struct { + ID string + Binary string + Enabled bool + ConfigJSON []byte + Lifecycle LifecycleConfig +} + +// defaultLifecycleConfig returns the defaults used for dynamically registered +// instances (those coming from main.pkl rather than drivers.toml). +func defaultLifecycleConfig() LifecycleConfig { + return LifecycleConfig{ + HandshakeDeadline: 5 * time.Second, + HealthProbeInterval: 15 * time.Second, + HealthProbeTimeout: 3 * time.Second, + HealthFailuresToRestart: 3, + ShutdownGrace: 10 * time.Second, + RestartBackoffInitial: time.Second, + RestartBackoffMax: 60 * time.Second, + RestartBudgetWindow: 10 * time.Minute, + RestartBudgetMax: 10, + } +} + +func dur(ms int, def time.Duration) time.Duration { + if ms <= 0 { + return def + } + return time.Duration(ms) * time.Millisecond +} + +func intd(v, def int) int { + if v <= 0 { + return def + } + return v +} +``` + +- [ ] **Step 2: Delete config_test.go** + +```bash +rm internal/carport/config_test.go +``` + +- [ ] **Step 3: Update HostConfig and Host in carport.go** + +In `internal/carport/carport.go`, remove `DriversTOMLPath` from `HostConfig`: + +```go +type HostConfig struct { + // SocketDir is where per-instance UDS files are created. Defaults to + // /carport/ in the daemon wiring; required for any instance to be spawned. + SocketDir string +} +``` + +Remove the `cfgData *Config` field from `Host`: + +```go +type Host struct { + cfg HostConfig + + db *sql.DB + store *eventstore.Store + router *Router + logger *slog.Logger + metrics *observability.Metrics + + ctx context.Context + + mu sync.RWMutex + instances map[string]*managedInstance + + stopOnce sync.Once + stopped chan struct{} +} +``` + +Simplify `New` — no more TOML loading: + +```go +func New(cfg HostConfig, db *sql.DB, store *eventstore.Store, reg *registry.Registry, logger *slog.Logger, metrics *observability.Metrics) (*Host, error) { + return &Host{ + cfg: cfg, + db: db, + store: store, + router: NewRouter(reg), + logger: logger.With("subsystem", "carport"), + metrics: metrics, + instances: map[string]*managedInstance{}, + stopped: make(chan struct{}), + }, nil +} +``` + +Simplify `Start` — remove the TOML instance loop (keep the `h.ctx = ctx` line added in Task 4): + +```go +func (h *Host) Start(ctx context.Context) error { + h.ctx = ctx + return nil +} +``` + +- [ ] **Step 4: Build to catch compile errors** + +```bash +task build +``` + +Expected: compile errors pointing to the `DriversTOMLPath` references in daemon. Fix them in the next steps. + +- [ ] **Step 5: Remove DriversTOMLPath from daemon/config.go** + +In `internal/daemon/config.go`, remove the `DriversTOMLPath` field and its default: + +```go +type Config struct { + DataDir string + LogLevel slog.Level + LogFormat string + AdminPort int + SocketPath string + SnapshotEveryEvents int + SnapshotEveryPeriod time.Duration + CarportSocketDir string + ConfigDir string +} + +func (c *Config) WithDefaults() { + if c.DataDir == "" { + c.DataDir = "~/.local/share/gohome" + } + if c.LogFormat == "" { + c.LogFormat = "auto" + } + if c.AdminPort == 0 { + c.AdminPort = 9190 + } + if c.SocketPath == "" { + c.SocketPath = "gohomed.sock" + } + if c.SnapshotEveryEvents == 0 { + c.SnapshotEveryEvents = 10_000 + } + if c.SnapshotEveryPeriod == 0 { + c.SnapshotEveryPeriod = time.Hour + } + if c.CarportSocketDir == "" { + c.CarportSocketDir = "@data/carport" + } + if c.ConfigDir == "" { + c.ConfigDir = "@data/config" + } +} +``` + +- [ ] **Step 6: Remove driversTOML resolution from daemon.go** + +In `internal/daemon/daemon.go`, find the phase 4.5 carport block (around line 186) and remove the `driversTOML` resolution. The block should become: + +```go +// Phase 4.5: carport — driver supervisor +socketDir := d.cfg.CarportSocketDir +if socketDir == "@data/carport" { + socketDir = filepath.Join(dataDir, "carport") +} +cport, err := carport.New(carport.HostConfig{ + SocketDir: socketDir, +}, d.db, d.store, d.registry, d.logger, d.metrics) +if err != nil { + return fmt.Errorf("carport: %w", err) +} +d.carport = cport +if err := d.carport.Start(ctx); err != nil { + return fmt.Errorf("carport start: %w", err) +} +``` + +- [ ] **Step 7: Remove --drivers-toml flag from cmd/gohomed/main.go** + +In `cmd/gohomed/main.go`, delete the flag declaration: + +```go +// DELETE: +driversTOML = flag.String("drivers-toml", "", "path to drivers.toml (default /drivers.toml)") +``` + +And remove `DriversTOMLPath` from the `daemon.Config` construction: + +```go +cfg := daemon.Config{ + DataDir: *dataDir, + LogLevel: level, + LogFormat: *logFormat, + AdminPort: *adminPort, + SnapshotEveryEvents: *snapshotEveryEvt, + SnapshotEveryPeriod: *snapshotEveryDur, + ConfigDir: *configDir, +} +``` + +- [ ] **Step 8: Build and full test suite** + +```bash +task build && task test +``` + +Expected: compiles and all tests pass. + +- [ ] **Step 9: Commit** + +```bash +git add internal/carport/config.go internal/carport/carport.go internal/daemon/config.go internal/daemon/daemon.go cmd/gohomed/main.go +git rm internal/carport/config_test.go +git commit -m "feat(carport): remove drivers.toml — main.pkl is now the sole driver config source" +``` + +--- + +## Task 7: Definition of done + +- [ ] **Step 1: Build** + +```bash +task build +``` + +Expected: no errors. + +- [ ] **Step 2: All tests** + +```bash +task test && task test:race +``` + +Expected: PASS, no races. + +- [ ] **Step 3: Integration tests** + +```bash +task test:integration +``` + +Expected: PASS (requires `pkl` on PATH). + +- [ ] **Step 4: Smoke test — driver from main.pkl launches** + +```bash +mkdir -p /tmp/gohome-smoke/config +cat > /tmp/gohome-smoke/config/main.pkl << 'EOF' +amends "gohome:config" + +import "gohome:carport" as cp + +local class SmokeDriver extends cp.DriverInstance {} + +driverInstances { + new SmokeDriver { + id = "smoke-1" + driverName = "fake" + binary = "/nonexistent-binary" + } +} +EOF +./dist/gohomed --data-dir /tmp/gohome-smoke --log-level debug 2>&1 | head -40 +``` + +Expected log lines (in order): +1. `config applied` — confirms `Apply` ran and called `RegisterInstance` +2. `spawn_error` or similar for `smoke-1` — confirms `launchLifecycle` ran (binary doesn't exist) + +- [ ] **Step 5: Verify --drivers-toml is gone** + +```bash +./dist/gohomed --help 2>&1 | grep drivers-toml +``` + +Expected: no output. + +- [ ] **Step 6: Verify duplicate ID error** + +Declare the same `id` twice in `main.pkl` and confirm the daemon exits with a clear error during `Apply`. + +--- + +## Known limitations (follow-up work) + +- **No lifecycle tuning from Pkl**: dynamically registered instances always use `defaultLifecycleConfig()`. Add lifecycle fields to `DriverInstance` in a follow-up. +- **Migration**: existing `drivers.toml` entries must be moved to `main.pkl` before upgrading. The `--drivers-toml` flag is gone. diff --git a/gen/gohome/config/v1/snapshot.pb.go b/gen/gohome/config/v1/snapshot.pb.go index b2ead41..5afad30 100644 --- a/gen/gohome/config/v1/snapshot.pb.go +++ b/gen/gohome/config/v1/snapshot.pb.go @@ -285,6 +285,7 @@ type DriverInstanceConfig struct { DriverName string `protobuf:"bytes,2,opt,name=driver_name,json=driverName,proto3" json:"driver_name,omitempty"` ConfigHash []byte `protobuf:"bytes,3,opt,name=config_hash,json=configHash,proto3" json:"config_hash,omitempty"` Params []byte `protobuf:"bytes,4,opt,name=params,proto3" json:"params,omitempty"` + Binary string `protobuf:"bytes,5,opt,name=binary,proto3" json:"binary,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache } @@ -347,6 +348,13 @@ func (x *DriverInstanceConfig) GetParams() []byte { return nil } +func (x *DriverInstanceConfig) GetBinary() string { + if x != nil { + return x.Binary + } + return "" +} + type EntityConfig struct { state protoimpl.MessageState `protogen:"open.v1"` Id string `protobuf:"bytes,1,opt,name=id,proto3" json:"id,omitempty"` @@ -3029,14 +3037,15 @@ const file_gohome_config_v1_snapshot_proto_rawDesc = "" + "\ascripts\x18\x11 \x03(\v2\x1e.gohome.config.v1.ScriptConfigR\ascripts\x12<\n" + "\blistener\x18\x14 \x01(\v2 .gohome.config.v1.ListenerConfigR\blistener\x12-\n" + "\x03mcp\x18\x15 \x01(\v2\x1b.gohome.config.v1.MCPConfigR\x03mcp\x12I\n" + - "\rauth_settings\x18\x16 \x01(\v2$.gohome.config.v1.AuthSettingsConfigR\fauthSettings\"\x80\x01\n" + + "\rauth_settings\x18\x16 \x01(\v2$.gohome.config.v1.AuthSettingsConfigR\fauthSettings\"\x98\x01\n" + "\x14DriverInstanceConfig\x12\x0e\n" + "\x02id\x18\x01 \x01(\tR\x02id\x12\x1f\n" + "\vdriver_name\x18\x02 \x01(\tR\n" + "driverName\x12\x1f\n" + "\vconfig_hash\x18\x03 \x01(\fR\n" + "configHash\x12\x16\n" + - "\x06params\x18\x04 \x01(\fR\x06params\"x\n" + + "\x06params\x18\x04 \x01(\fR\x06params\x12\x16\n" + + "\x06binary\x18\x05 \x01(\tR\x06binary\"x\n" + "\fEntityConfig\x12\x0e\n" + "\x02id\x18\x01 \x01(\tR\x02id\x12#\n" + "\rfriendly_name\x18\x02 \x01(\tR\ffriendlyName\x12\x1f\n" + diff --git a/go.mod b/go.mod index f0ad370..f8402b4 100644 --- a/go.mod +++ b/go.mod @@ -4,7 +4,6 @@ go 1.25.9 require ( connectrpc.com/connect v1.19.2 - github.com/BurntSushi/toml v1.6.0 github.com/apple/pkl-go v0.13.2 github.com/benbjohnson/immutable v0.4.3 github.com/charmbracelet/lipgloss v1.1.0 diff --git a/go.sum b/go.sum index 2d8269c..c5d9602 100644 --- a/go.sum +++ b/go.sum @@ -1,7 +1,5 @@ connectrpc.com/connect v1.19.2 h1:McQ83FGdzL+t60peksi0gXC7MQ/iLKgLduAnThbM0mo= connectrpc.com/connect v1.19.2/go.mod h1:tN20fjdGlewnSFeZxLKb0xwIZ6ozc3OQs2hTXy4du9w= -github.com/BurntSushi/toml v1.6.0 h1:dRaEfpa2VI55EwlIW72hMRHdWouJeRF7TPYhI+AUQjk= -github.com/BurntSushi/toml v1.6.0/go.mod h1:ukJfTF/6rtPPRCnwkur4qwRxa8vTRFBF0uk2lLoLwho= github.com/apple/pkl-go v0.13.2 h1:UCug0neH9Ufw0Loujosv5/UUXEXzY76HXbjqYqlPSGI= github.com/apple/pkl-go v0.13.2/go.mod h1:Ko3AgXOKd/vVYtsRZgoCZhymymz9RxqCIcfdZhOX85I= github.com/aymanbagabas/go-osc52/v2 v2.0.1 h1:HwpRHbFMcZLEVr42D4p7XBqjyuxQH5SMiErDT4WkJ2k= diff --git a/internal/carport/carport.go b/internal/carport/carport.go index ebdd910..6b22be2 100644 --- a/internal/carport/carport.go +++ b/internal/carport/carport.go @@ -1,6 +1,6 @@ -// Package carport hosts the driver-supervisor subsystem: drivers.toml -// configuration, per-instance subprocess lifecycle, command dispatch, and -// event ingest from drivers over the Carport gRPC protocol (v1alpha1). +// Package carport hosts the driver-supervisor subsystem: per-instance subprocess +// lifecycle, command dispatch, and event ingest from drivers over the Carport +// gRPC protocol (v1alpha1). Driver instances are declared via main.pkl. // // See docs/superpowers/specs/2026-04-21-c2-carport-protocol-design.md. package carport @@ -8,6 +8,7 @@ package carport import ( "context" "database/sql" + "fmt" "log/slog" "sync" @@ -18,21 +19,14 @@ import ( // HostConfig is the daemon-level configuration handed to New. type HostConfig struct { - // DriversTOMLPath is the absolute path to drivers.toml. Empty or missing - // file → zero configured instances (host is a no-op but still starts). - DriversTOMLPath string - - // SocketDir is where per-instance UDS files are created. Defaults to - // /carport/ in the daemon wiring (T18); required here if - // any instance is to be spawned. + // SocketDir is where per-instance UDS files are created. SocketDir string } // Host is the public face of the carport subsystem. Instances are spawned // and supervised on Start; Stop shuts them down gracefully. type Host struct { - cfg HostConfig - cfgData *Config + cfg HostConfig db *sql.DB store *eventstore.Store @@ -40,6 +34,8 @@ type Host struct { logger *slog.Logger metrics *observability.Metrics + ctx context.Context // root context for lifecycle goroutines; set by Start + mu sync.RWMutex instances map[string]*managedInstance // keyed by Instance.ID @@ -75,20 +71,9 @@ type managedInstance struct { } // New constructs a Host. The host is inert until Start is called. -// Passing an empty DriversTOMLPath OR a path that does not exist yields a -// Host with zero instances; Start is then a no-op. func New(cfg HostConfig, db *sql.DB, store *eventstore.Store, reg *registry.Registry, logger *slog.Logger, metrics *observability.Metrics) (*Host, error) { - cfgData := &Config{} - if cfg.DriversTOMLPath != "" { - loaded, err := LoadConfig(cfg.DriversTOMLPath) - if err != nil { - return nil, err - } - cfgData = loaded - } return &Host{ cfg: cfg, - cfgData: cfgData, db: db, store: store, router: NewRouter(reg), @@ -99,21 +84,11 @@ func New(cfg HostConfig, db *sql.DB, store *eventstore.Store, reg *registry.Regi }, nil } -// Start launches lifecycle goroutines for each enabled instance. Non-blocking. -// After Start returns, supervision goroutines are running; errors during -// spawn/handshake are reported through DriverEvents in the event log, not -// returned from this call. +// Start initialises the host's root context. Non-blocking. +// Driver instances are added dynamically via RegisterInstance (driven by +// main.pkl evaluation in the config manager). func (h *Host) Start(ctx context.Context) error { - for _, inst := range h.cfgData.Instances { - if !inst.Enabled { - continue - } - m := &managedInstance{cfg: inst, state: StateDeclared} - h.mu.Lock() - h.instances[inst.ID] = m - h.mu.Unlock() - h.launchLifecycle(ctx, m) - } + h.ctx = ctx return nil } @@ -135,4 +110,83 @@ func (h *Host) Stop(ctx context.Context) { }) } +// RegisterInstance adds a new driver instance and begins its lifecycle goroutine. +// Returns an error if an instance with that ID is already registered, if the host +// has not been started, or if the host has been stopped. +func (h *Host) RegisterInstance(_ context.Context, id, driverName, binary string, params []byte) error { + if h.ctx == nil { + return fmt.Errorf("carport host not started") + } + select { + case <-h.stopped: + return fmt.Errorf("carport host is stopped") + default: + } + h.mu.Lock() + if _, exists := h.instances[id]; exists { + h.mu.Unlock() + return fmt.Errorf("instance %q already registered", id) + } + inst := Instance{ + ID: id, + Binary: binary, + Enabled: true, + ConfigJSON: params, + Lifecycle: defaultLifecycleConfig(), + } + m := &managedInstance{cfg: inst, state: StateDeclared} + h.instances[id] = m + h.mu.Unlock() + h.launchLifecycle(h.ctx, m) //nolint:contextcheck // lifecycle goroutine must outlive the caller's context + return nil +} + +// RegisterInstanceWithLifecycle is like RegisterInstance but accepts an explicit +// LifecycleConfig, allowing callers (e.g. integration tests) to override timing. +// The CarportManager interface uses RegisterInstance which applies defaultLifecycleConfig. +func (h *Host) RegisterInstanceWithLifecycle(_ context.Context, id, _, binary string, params []byte, lc LifecycleConfig) error { + if h.ctx == nil { + return fmt.Errorf("carport host not started") + } + select { + case <-h.stopped: + return fmt.Errorf("carport host is stopped") + default: + } + h.mu.Lock() + if _, exists := h.instances[id]; exists { + h.mu.Unlock() + return fmt.Errorf("instance %q already registered", id) + } + inst := Instance{ + ID: id, + Binary: binary, + Enabled: true, + ConfigJSON: params, + Lifecycle: lc, + } + m := &managedInstance{cfg: inst, state: StateDeclared} + h.instances[id] = m + h.mu.Unlock() + h.launchLifecycle(h.ctx, m) //nolint:contextcheck // lifecycle goroutine must outlive the caller's context + return nil +} + +// UnregisterInstance stops and removes a driver instance by ID. +// Returns an error if the instance is not found. +func (h *Host) UnregisterInstance(_ context.Context, id string) error { + h.mu.Lock() + m, exists := h.instances[id] + if !exists { + h.mu.Unlock() + return fmt.Errorf("instance %q not found", id) + } + delete(h.instances, id) + h.mu.Unlock() + // Use Background context so shutdown isn't cut short by the caller's deadline. + // Shutdown duration is bounded by m.cfg.Lifecycle.ShutdownGrace. + h.shutdownInstance(context.Background(), m) //nolint:contextcheck + return nil +} + // launchLifecycle and shutdownInstance are implemented in supervisor.go. diff --git a/internal/carport/config.go b/internal/carport/config.go index 31f23be..aaed7ad 100644 --- a/internal/carport/config.go +++ b/internal/carport/config.go @@ -1,13 +1,7 @@ package carport import ( - "errors" - "fmt" - "os" - "regexp" "time" - - "github.com/BurntSushi/toml" ) // LifecycleConfig tunes per-instance timing and restart policy. @@ -24,7 +18,7 @@ type LifecycleConfig struct { RestartBudgetMax int } -// Instance is one entry in drivers.toml. +// Instance is a single driver instance declaration. type Instance struct { ID string Binary string @@ -33,104 +27,18 @@ type Instance struct { Lifecycle LifecycleConfig } -// Config is the parsed drivers.toml. -type Config struct { - Instances []Instance -} - -var idRE = regexp.MustCompile(`^[a-z0-9_\-]{1,64}$`) - -// LoadConfig reads drivers.toml at path. A missing file yields an empty Config. -// An unreadable/invalid file is an error. Every entry is validated per §5.2 of -// the C2 design doc. -func LoadConfig(path string) (*Config, error) { - data, err := os.ReadFile(path) - if err != nil { - if errors.Is(err, os.ErrNotExist) { - return &Config{}, nil - } - return nil, fmt.Errorf("read drivers.toml: %w", err) - } - - var raw struct { - Instance []struct { - ID string `toml:"id"` - Binary string `toml:"binary"` - Enabled *bool `toml:"enabled"` - ConfigJSON string `toml:"config_json"` - Lifecycle struct { - HandshakeDeadlineMs int `toml:"handshake_deadline_ms"` - HealthProbeIntervalMs int `toml:"health_probe_interval_ms"` - HealthProbeTimeoutMs int `toml:"health_probe_timeout_ms"` - HealthFailuresToRestart int `toml:"health_failures_to_restart"` - ShutdownGraceMs int `toml:"shutdown_grace_ms"` - RestartBackoffInitialMs int `toml:"restart_backoff_initial_ms"` - RestartBackoffMaxMs int `toml:"restart_backoff_max_ms"` - RestartBudgetWindowMin int `toml:"restart_budget_window_minutes"` - RestartBudgetMax int `toml:"restart_budget_max"` - } `toml:"lifecycle"` - } `toml:"instance"` - } - if err := toml.Unmarshal(data, &raw); err != nil { - return nil, fmt.Errorf("parse drivers.toml: %w", err) - } - - seen := map[string]bool{} - cfg := &Config{} - for i, r := range raw.Instance { - if !idRE.MatchString(r.ID) { - return nil, fmt.Errorf("instance[%d]: invalid id %q (want [a-z0-9_-]{1,64})", i, r.ID) - } - if seen[r.ID] { - return nil, fmt.Errorf("duplicate instance id %q", r.ID) - } - seen[r.ID] = true - if r.Binary == "" { - return nil, fmt.Errorf("instance %q: binary is required", r.ID) - } - info, err := os.Stat(r.Binary) - if err != nil { - return nil, fmt.Errorf("instance %q: binary %q: %w", r.ID, r.Binary, err) - } - if info.Mode()&0o111 == 0 { - return nil, fmt.Errorf("instance %q: binary %q is not executable", r.ID, r.Binary) - } - enabled := true - if r.Enabled != nil { - enabled = *r.Enabled - } - lc := LifecycleConfig{ - HandshakeDeadline: dur(r.Lifecycle.HandshakeDeadlineMs, 5*time.Second), - HealthProbeInterval: dur(r.Lifecycle.HealthProbeIntervalMs, 15*time.Second), - HealthProbeTimeout: dur(r.Lifecycle.HealthProbeTimeoutMs, 3*time.Second), - HealthFailuresToRestart: intd(r.Lifecycle.HealthFailuresToRestart, 3), - ShutdownGrace: dur(r.Lifecycle.ShutdownGraceMs, 10*time.Second), - RestartBackoffInitial: dur(r.Lifecycle.RestartBackoffInitialMs, 1*time.Second), - RestartBackoffMax: dur(r.Lifecycle.RestartBackoffMaxMs, 60*time.Second), - RestartBudgetWindow: time.Duration(intd(r.Lifecycle.RestartBudgetWindowMin, 10)) * time.Minute, - RestartBudgetMax: intd(r.Lifecycle.RestartBudgetMax, 10), - } - cfg.Instances = append(cfg.Instances, Instance{ - ID: r.ID, - Binary: r.Binary, - Enabled: enabled, - ConfigJSON: []byte(r.ConfigJSON), - Lifecycle: lc, - }) - } - return cfg, nil -} - -func dur(ms int, def time.Duration) time.Duration { - if ms <= 0 { - return def - } - return time.Duration(ms) * time.Millisecond -} - -func intd(v, def int) int { - if v <= 0 { - return def +// defaultLifecycleConfig returns the defaults used for dynamically registered +// instances (those coming from main.pkl rather than drivers.toml). +func defaultLifecycleConfig() LifecycleConfig { + return LifecycleConfig{ + HandshakeDeadline: 5 * time.Second, + HealthProbeInterval: 15 * time.Second, + HealthProbeTimeout: 3 * time.Second, + HealthFailuresToRestart: 3, + ShutdownGrace: 10 * time.Second, + RestartBackoffInitial: time.Second, + RestartBackoffMax: 60 * time.Second, + RestartBudgetWindow: 10 * time.Minute, + RestartBudgetMax: 10, } - return v } diff --git a/internal/carport/config_test.go b/internal/carport/config_test.go deleted file mode 100644 index 4917bc6..0000000 --- a/internal/carport/config_test.go +++ /dev/null @@ -1,154 +0,0 @@ -package carport_test - -import ( - "os" - "path/filepath" - "testing" - "time" - - "github.com/fdatoo/gohome/internal/carport" -) - -func writeTOML(t *testing.T, content string) string { - t.Helper() - dir := t.TempDir() - p := filepath.Join(dir, "drivers.toml") - if err := os.WriteFile(p, []byte(content), 0o644); err != nil { - t.Fatal(err) - } - return p -} - -func TestLoadConfig_EmptyFileIsValid(t *testing.T) { - p := writeTOML(t, "") - cfg, err := carport.LoadConfig(p) - if err != nil { - t.Fatalf("LoadConfig: %v", err) - } - if len(cfg.Instances) != 0 { - t.Fatalf("want 0 instances, got %d", len(cfg.Instances)) - } -} - -func TestLoadConfig_HappyPath(t *testing.T) { - bin := filepath.Join(t.TempDir(), "fake") - if err := os.WriteFile(bin, []byte("#!/bin/sh\nexit 0"), 0o755); err != nil { - t.Fatal(err) - } - p := writeTOML(t, ` -[[instance]] -id = "hue_main" -binary = "`+bin+`" -enabled = true -config_json = "{\"x\": 1}" -`) - cfg, err := carport.LoadConfig(p) - if err != nil { - t.Fatalf("LoadConfig: %v", err) - } - if len(cfg.Instances) != 1 { - t.Fatalf("want 1 instance, got %d", len(cfg.Instances)) - } - got := cfg.Instances[0] - if got.ID != "hue_main" { - t.Errorf("ID = %q", got.ID) - } - if got.Binary != bin { - t.Errorf("Binary = %q", got.Binary) - } - if !got.Enabled { - t.Error("Enabled = false, want true") - } - if string(got.ConfigJSON) != `{"x": 1}` { - t.Errorf("ConfigJSON = %q", got.ConfigJSON) - } - if got.Lifecycle.HealthProbeInterval != 15*time.Second { - t.Errorf("HealthProbeInterval default = %v", got.Lifecycle.HealthProbeInterval) - } -} - -func TestLoadConfig_RejectsDuplicateID(t *testing.T) { - bin := filepath.Join(t.TempDir(), "fake") - _ = os.WriteFile(bin, []byte("x"), 0o755) - p := writeTOML(t, ` -[[instance]] -id = "a" -binary = "`+bin+`" - -[[instance]] -id = "a" -binary = "`+bin+`" -`) - _, err := carport.LoadConfig(p) - if err == nil { - t.Fatal("expected error for duplicate id") - } -} - -func TestLoadConfig_RejectsInvalidID(t *testing.T) { - bin := filepath.Join(t.TempDir(), "fake") - _ = os.WriteFile(bin, []byte("x"), 0o755) - longID := "a" + string(make([]byte, 80)) // > 64 chars; fails regex - for _, id := range []string{"", "UPPER", "with space", longID} { - p := writeTOML(t, ` -[[instance]] -id = "`+id+`" -binary = "`+bin+`" -`) - if _, err := carport.LoadConfig(p); err == nil { - t.Errorf("expected error for id %q", id) - } - } -} - -func TestLoadConfig_RejectsMissingBinary(t *testing.T) { - p := writeTOML(t, ` -[[instance]] -id = "x" -binary = "/no/such/file/exists/here" -`) - _, err := carport.LoadConfig(p) - if err == nil { - t.Fatal("expected error for missing binary") - } -} - -func TestLoadConfig_AcceptsLifecycleOverrides(t *testing.T) { - bin := filepath.Join(t.TempDir(), "fake") - _ = os.WriteFile(bin, []byte("x"), 0o755) - p := writeTOML(t, ` -[[instance]] -id = "x" -binary = "`+bin+`" -[instance.lifecycle] -health_probe_interval_ms = 5000 -health_failures_to_restart = 5 -shutdown_grace_ms = 20000 -restart_budget_window_minutes = 30 -restart_budget_max = 3 -`) - cfg, err := carport.LoadConfig(p) - if err != nil { - t.Fatal(err) - } - inst := cfg.Instances[0] - if inst.Lifecycle.HealthProbeInterval != 5*time.Second { - t.Errorf("HealthProbeInterval = %v", inst.Lifecycle.HealthProbeInterval) - } - if inst.Lifecycle.HealthFailuresToRestart != 5 { - t.Errorf("HealthFailuresToRestart = %d", inst.Lifecycle.HealthFailuresToRestart) - } - if inst.Lifecycle.RestartBudgetWindow != 30*time.Minute { - t.Errorf("RestartBudgetWindow = %v", inst.Lifecycle.RestartBudgetWindow) - } -} - -func TestLoadConfig_MissingFileReturnsEmptyConfig(t *testing.T) { - cfg, err := carport.LoadConfig(filepath.Join(t.TempDir(), "nope.toml")) - if err != nil { - t.Fatalf("LoadConfig missing: %v", err) - } - if len(cfg.Instances) != 0 { - t.Fatalf("want 0 instances, got %d", len(cfg.Instances)) - } -} diff --git a/internal/carport/coverage_test.go b/internal/carport/coverage_test.go index 1bbb187..51b75eb 100644 --- a/internal/carport/coverage_test.go +++ b/internal/carport/coverage_test.go @@ -4,8 +4,6 @@ import ( "bytes" "context" "log/slog" - "os" - "path/filepath" "strings" "testing" "time" @@ -192,128 +190,16 @@ func TestIngestMessage_DriverEventFillsInstanceID(t *testing.T) { } } -// ---- LoadConfig error paths ---- - -func TestLoadConfig_MissingFileReturnsEmpty(t *testing.T) { - cfg, err := carport.LoadConfig(filepath.Join(t.TempDir(), "absent.toml")) - if err != nil { - t.Fatalf("missing file should be empty config, got err: %v", err) - } - if len(cfg.Instances) != 0 { - t.Errorf("Instances = %d, want 0", len(cfg.Instances)) - } -} - -func TestLoadConfig_BadTOML(t *testing.T) { - p := filepath.Join(t.TempDir(), "bad.toml") - _ = os.WriteFile(p, []byte("][]not valid toml"), 0o644) - if _, err := carport.LoadConfig(p); err == nil { - t.Fatal("expected parse error") - } -} - -func TestLoadConfig_InvalidID(t *testing.T) { - p := filepath.Join(t.TempDir(), "x.toml") - _ = os.WriteFile(p, []byte(`[[instance]] -id = "BAD ID!" -binary = "/bin/ls" -`), 0o644) - if _, err := carport.LoadConfig(p); err == nil || !strings.Contains(err.Error(), "invalid id") { - t.Errorf("err = %v, want 'invalid id'", err) - } -} - -func TestLoadConfig_DuplicateID(t *testing.T) { - p := filepath.Join(t.TempDir(), "x.toml") - _ = os.WriteFile(p, []byte(`[[instance]] -id = "a" -binary = "/bin/ls" -[[instance]] -id = "a" -binary = "/bin/ls" -`), 0o644) - if _, err := carport.LoadConfig(p); err == nil || !strings.Contains(err.Error(), "duplicate") { - t.Errorf("err = %v, want 'duplicate'", err) - } -} - -func TestLoadConfig_BinaryRequired(t *testing.T) { - p := filepath.Join(t.TempDir(), "x.toml") - _ = os.WriteFile(p, []byte(`[[instance]] -id = "a" -`), 0o644) - if _, err := carport.LoadConfig(p); err == nil || !strings.Contains(err.Error(), "binary is required") { - t.Errorf("err = %v, want 'binary is required'", err) - } -} - -func TestLoadConfig_BinaryNotFound(t *testing.T) { - p := filepath.Join(t.TempDir(), "x.toml") - _ = os.WriteFile(p, []byte(`[[instance]] -id = "a" -binary = "/no/such/path/binary" -`), 0o644) - if _, err := carport.LoadConfig(p); err == nil { - t.Fatal("expected error for missing binary") - } -} - -func TestLoadConfig_BinaryNotExecutable(t *testing.T) { - dir := t.TempDir() - bin := filepath.Join(dir, "notexec") - _ = os.WriteFile(bin, []byte("data"), 0o644) // no exec bit - p := filepath.Join(dir, "x.toml") - _ = os.WriteFile(p, []byte(`[[instance]] -id = "a" -binary = "`+bin+`" -`), 0o644) - if _, err := carport.LoadConfig(p); err == nil || !strings.Contains(err.Error(), "not executable") { - t.Errorf("err = %v, want 'not executable'", err) - } -} - -func TestLoadConfig_DefaultsApplied(t *testing.T) { - dir := t.TempDir() - bin := filepath.Join(dir, "ok") - _ = os.WriteFile(bin, []byte("#!/bin/sh\nexit 0\n"), 0o755) - p := filepath.Join(dir, "x.toml") - _ = os.WriteFile(p, []byte(`[[instance]] -id = "a" -binary = "`+bin+`" -`), 0o644) - cfg, err := carport.LoadConfig(p) - if err != nil { - t.Fatalf("LoadConfig: %v", err) - } - if len(cfg.Instances) != 1 { - t.Fatalf("got %d instances", len(cfg.Instances)) - } - inst := cfg.Instances[0] - if !inst.Enabled { - t.Error("default Enabled = false; want true") - } - // Defaults applied: - if inst.Lifecycle.HandshakeDeadline != 5*time.Second { - t.Errorf("HandshakeDeadline = %v", inst.Lifecycle.HandshakeDeadline) - } - if inst.Lifecycle.HealthFailuresToRestart != 3 { - t.Errorf("HealthFailuresToRestart = %d", inst.Lifecycle.HealthFailuresToRestart) - } - if inst.Lifecycle.RestartBudgetMax != 10 { - t.Errorf("RestartBudgetMax = %d", inst.Lifecycle.RestartBudgetMax) - } -} - // ---- New / Stop / InstanceState edges ---- -func TestNew_EmptyDriversTOMLPath(t *testing.T) { +func TestNew_NoInstances(t *testing.T) { f := newStoreFixtureForTest(t) logger := newQuietLogger() metrics := newQuietMetrics() - h, err := carport.New(carport.HostConfig{DriversTOMLPath: "", SocketDir: t.TempDir()}, + h, err := carport.New(carport.HostConfig{SocketDir: t.TempDir()}, f.db, f.store, nil, logger, metrics) if err != nil { - t.Fatalf("New empty path should succeed: %v", err) + t.Fatalf("New should succeed: %v", err) } if h == nil { t.Fatal("Host nil") @@ -327,9 +213,7 @@ func TestStartStop_NoInstances(t *testing.T) { f := newStoreFixtureForTest(t) logger := newQuietLogger() metrics := newQuietMetrics() - cfgPath := filepath.Join(t.TempDir(), "drivers.toml") - _ = os.WriteFile(cfgPath, []byte(""), 0o644) - h, err := carport.New(carport.HostConfig{DriversTOMLPath: cfgPath, SocketDir: t.TempDir()}, + h, err := carport.New(carport.HostConfig{SocketDir: t.TempDir()}, f.db, f.store, nil, logger, metrics) if err != nil { t.Fatal(err) @@ -343,33 +227,3 @@ func TestStartStop_NoInstances(t *testing.T) { h.Stop(context.Background()) h.Stop(context.Background()) } - -func TestStart_DisabledInstanceSkipped(t *testing.T) { - dir := t.TempDir() - bin := filepath.Join(dir, "ok") - _ = os.WriteFile(bin, []byte("#!/bin/sh\nexit 0\n"), 0o755) - cfgPath := filepath.Join(dir, "drivers.toml") - _ = os.WriteFile(cfgPath, []byte(`[[instance]] -id = "off" -binary = "`+bin+`" -enabled = false -`), 0o644) - - f := newStoreFixtureForTest(t) - h, err := carport.New(carport.HostConfig{DriversTOMLPath: cfgPath, SocketDir: t.TempDir()}, - f.db, f.store, nil, newQuietLogger(), newQuietMetrics()) - if err != nil { - t.Fatal(err) - } - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - if err := h.Start(ctx); err != nil { - t.Fatal(err) - } - defer h.Stop(context.Background()) - // Disabled instance is not entered into h.instances → InstanceState returns StateDeclared - // (the “unknown” fallthrough). - if state := h.InstanceState("off"); state != carport.StateDeclared { - t.Errorf("disabled instance state = %s, want StateDeclared (unknown)", state) - } -} diff --git a/internal/carport/dynamic_test.go b/internal/carport/dynamic_test.go new file mode 100644 index 0000000..ee71bcf --- /dev/null +++ b/internal/carport/dynamic_test.go @@ -0,0 +1,59 @@ +package carport + +import ( + "context" + "testing" +) + +func TestRegisterInstance_HostNotStarted(t *testing.T) { + h := &Host{ + instances: map[string]*managedInstance{}, + stopped: make(chan struct{}), + // ctx is nil — host not started + } + err := h.RegisterInstance(context.Background(), "new", "fake", "/bin/fake", nil) + if err == nil { + t.Fatal("expected error when host not started (ctx nil)") + } +} + +func TestRegisterInstance_DuplicateID(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + h := &Host{ + instances: map[string]*managedInstance{ + "existing": {cfg: Instance{ID: "existing"}}, + }, + stopped: make(chan struct{}), + ctx: ctx, + } + err := h.RegisterInstance(context.Background(), "existing", "fake", "/bin/fake", nil) + if err == nil { + t.Fatal("expected error for duplicate instance ID") + } +} + +func TestRegisterInstance_HostStopped(t *testing.T) { + stopped := make(chan struct{}) + close(stopped) // host is already stopped + h := &Host{ + instances: map[string]*managedInstance{}, + stopped: stopped, + ctx: context.Background(), + } + err := h.RegisterInstance(context.Background(), "new", "fake", "/bin/fake", nil) + if err == nil { + t.Fatal("expected error when host is stopped") + } +} + +func TestUnregisterInstance_NotFound(t *testing.T) { + h := &Host{ + instances: map[string]*managedInstance{}, + stopped: make(chan struct{}), + } + err := h.UnregisterInstance(context.Background(), "missing") + if err == nil { + t.Fatal("expected error for missing instance") + } +} diff --git a/internal/carport/errors.go b/internal/carport/errors.go index ab8893f..0b2c08c 100644 --- a/internal/carport/errors.go +++ b/internal/carport/errors.go @@ -1,6 +1,7 @@ -// Package carport hosts the driver-supervisor subsystem: drivers.toml -// configuration, per-instance subprocess lifecycle, command dispatch, and -// event ingest from drivers over the Carport gRPC protocol (v1alpha1). +// Package carport hosts the driver-supervisor subsystem: per-instance subprocess +// lifecycle, command dispatch, and event ingest from drivers over the Carport +// gRPC protocol (v1alpha1). Driver instances are registered dynamically via +// RegisterInstance. // // See docs/superpowers/specs/2026-04-21-c2-carport-protocol-design.md. package carport diff --git a/internal/carport/fsm.go b/internal/carport/fsm.go index ca718e6..e1825c8 100644 --- a/internal/carport/fsm.go +++ b/internal/carport/fsm.go @@ -6,7 +6,7 @@ import "fmt" type State int const ( - StateDeclared State = iota // row exists in drivers.toml; nothing running yet + StateDeclared State = iota // instance registered via RegisterInstance; nothing running yet StateSpawning StateAwaitingHandshake StateRunning diff --git a/internal/carport/integration_test.go b/internal/carport/integration_test.go index cdbe60a..dc7d0d0 100644 --- a/internal/carport/integration_test.go +++ b/internal/carport/integration_test.go @@ -74,34 +74,12 @@ func runScenario(t *testing.T, mode string, until func(*carport.Host) bool) *car t.Fatal(err) } - cfgDir := t.TempDir() - tomlPath := filepath.Join(cfgDir, "drivers.toml") - // Short timeouts to keep the test fast. - content := "[[instance]]\n" + - "id = \"test_one\"\n" + - "binary = \"" + bin + "\"\n" + - "enabled = true\n" + - "config_json = \"{\\\"TESTDRIVER_MODE\\\":\\\"" + mode + "\\\"}\"\n" + - "[instance.lifecycle]\n" + - "handshake_deadline_ms = 2000\n" + - "health_probe_interval_ms = 500\n" + - "health_probe_timeout_ms = 300\n" + - "health_failures_to_restart = 2\n" + - "restart_backoff_initial_ms = 100\n" + - "restart_backoff_max_ms = 500\n" + - "restart_budget_window_minutes = 1\n" + - "restart_budget_max = 3\n" + - "shutdown_grace_ms = 1000\n" - if err := os.WriteFile(tomlPath, []byte(content), 0o644); err != nil { - t.Fatal(err) - } - sockDir, err := os.MkdirTemp("", "ghsd") if err != nil { t.Fatal(err) } t.Cleanup(func() { _ = os.RemoveAll(sockDir) }) - h, err := carport.New(carport.HostConfig{DriversTOMLPath: tomlPath, SocketDir: sockDir}, + h, err := carport.New(carport.HostConfig{SocketDir: sockDir}, f.db, f.store, reg, newTestLogger(), newTestMetrics()) if err != nil { t.Fatal(err) @@ -114,6 +92,24 @@ func runScenario(t *testing.T, mode string, until func(*carport.Host) bool) *car if err := h.Start(ctx); err != nil { t.Fatal(err) } + params := []byte(`{"TESTDRIVER_MODE":"` + mode + `"}`) + // Use short timeouts so scenarios resolve within the 10 s test window. + // The default lifecycle has 15 s health probes which would cause crash-detection + // tests to time out before the supervisor transitions out of StateRunning. + lc := carport.LifecycleConfig{ + HandshakeDeadline: 2 * time.Second, + HealthProbeInterval: 500 * time.Millisecond, + HealthProbeTimeout: 300 * time.Millisecond, + HealthFailuresToRestart: 2, + RestartBackoffInitial: 100 * time.Millisecond, + RestartBackoffMax: 500 * time.Millisecond, + RestartBudgetWindow: time.Minute, + RestartBudgetMax: 3, + ShutdownGrace: time.Second, + } + if err := h.RegisterInstanceWithLifecycle(ctx, "test_one", "testdriver", bin, params, lc); err != nil { + t.Fatal(err) + } if !waitFor(10*time.Second, func() bool { return until(h) }) { t.Fatalf("scenario %s: never reached expected state; current=%s", mode, h.InstanceState("test_one")) } diff --git a/internal/carport/test_helpers_test.go b/internal/carport/test_helpers_test.go index 9cf87ae..4b6e0a4 100644 --- a/internal/carport/test_helpers_test.go +++ b/internal/carport/test_helpers_test.go @@ -5,8 +5,6 @@ import ( "context" "database/sql" "log/slog" - "os" - "path/filepath" "testing" carportpb "github.com/fdatoo/gohome/gen/gohome/carport/v1alpha1" @@ -62,11 +60,9 @@ func newHostForDispatch(t *testing.T, f *storeFixture, seed func(*storeFixture)) if seed != nil { seed(f) } - cfgPath := filepath.Join(t.TempDir(), "drivers.toml") - _ = os.WriteFile(cfgPath, []byte(""), 0o644) logger := observability.Init(observability.LogConfig{Level: slog.LevelWarn, Format: "json", Output: &bytes.Buffer{}}) metrics := observability.NewMetrics() - h, err := carport.New(carport.HostConfig{DriversTOMLPath: cfgPath, SocketDir: t.TempDir()}, + h, err := carport.New(carport.HostConfig{SocketDir: t.TempDir()}, f.db, f.store, reg, logger, metrics) if err != nil { t.Fatal(err) diff --git a/internal/config/evaluator.go b/internal/config/evaluator.go index 5eb6d06..d3dd1b6 100644 --- a/internal/config/evaluator.go +++ b/internal/config/evaluator.go @@ -310,6 +310,7 @@ func parseConfigJSON(text, configDir string) (*configpb.ConfigSnapshot, error) { var base struct { ID string `json:"id"` DriverName string `json:"driverName"` + Binary string `json:"binary"` } if err := json.Unmarshal(rawInst, &base); err != nil { return nil, fmt.Errorf("parse driver instance: %w", err) @@ -318,6 +319,7 @@ func parseConfigJSON(text, configDir string) (*configpb.ConfigSnapshot, error) { snap.DriverInstances = append(snap.DriverInstances, &configpb.DriverInstanceConfig{ Id: base.ID, DriverName: base.DriverName, + Binary: base.Binary, ConfigHash: h[:], Params: rawInst, }) diff --git a/internal/config/manager.go b/internal/config/manager.go index ce82d86..0be6aef 100644 --- a/internal/config/manager.go +++ b/internal/config/manager.go @@ -15,7 +15,7 @@ import ( // For C4, the daemon passes a no-op implementation; dynamic carport management // will be wired when carport.Host gains RegisterInstance/UnregisterInstance methods. type CarportManager interface { - RegisterInstance(ctx context.Context, id, driverName string, params []byte) error + RegisterInstance(ctx context.Context, id, driverName, binary string, params []byte) error UnregisterInstance(ctx context.Context, id string) error } @@ -104,7 +104,7 @@ func (m *Manager) Apply(ctx context.Context, dryRun bool) error { } for _, id := range diff.DriverInstancesAdded { di := findInstance(snap, id) - if err := m.carportMgr.RegisterInstance(ctx, di.GetId(), di.GetDriverName(), di.GetParams()); err != nil { + if err := m.carportMgr.RegisterInstance(ctx, di.GetId(), di.GetDriverName(), di.GetBinary(), di.GetParams()); err != nil { return fmt.Errorf("register %q: %w", id, err) } } @@ -113,7 +113,7 @@ func (m *Manager) Apply(ctx context.Context, dryRun bool) error { if err := m.carportMgr.UnregisterInstance(ctx, id); err != nil { return fmt.Errorf("unregister changed %q: %w", id, err) } - if err := m.carportMgr.RegisterInstance(ctx, di.GetId(), di.GetDriverName(), di.GetParams()); err != nil { + if err := m.carportMgr.RegisterInstance(ctx, di.GetId(), di.GetDriverName(), di.GetBinary(), di.GetParams()); err != nil { return fmt.Errorf("re-register changed %q: %w", id, err) } } diff --git a/internal/config/manager_test.go b/internal/config/manager_test.go index 5bd6bb8..2fd1f13 100644 --- a/internal/config/manager_test.go +++ b/internal/config/manager_test.go @@ -14,7 +14,7 @@ type fakeCarport struct { unregistered []string } -func (f *fakeCarport) RegisterInstance(_ context.Context, id, _ string, _ []byte) error { +func (f *fakeCarport) RegisterInstance(_ context.Context, id, _, _ string, _ []byte) error { f.registered = append(f.registered, id) return nil } diff --git a/internal/config/pkl/gohome/carport.pkl b/internal/config/pkl/gohome/carport.pkl index 4102cd5..3f105a5 100644 --- a/internal/config/pkl/gohome/carport.pkl +++ b/internal/config/pkl/gohome/carport.pkl @@ -2,8 +2,8 @@ module gohome.carport // DriverInstance is the base class for all driver instance configs. // Driver authors extend this with their own typed fields. -// The pkl_module field in DriverManifest is reserved for C5+ (DR-7). abstract class DriverInstance { id: String(!isEmpty) driverName: String(!isEmpty) + binary: String(!isEmpty) } diff --git a/internal/config/testdata/valid/main.pkl b/internal/config/testdata/valid/main.pkl index 7faa3ed..aed3ed9 100644 --- a/internal/config/testdata/valid/main.pkl +++ b/internal/config/testdata/valid/main.pkl @@ -9,6 +9,7 @@ driverInstances { new FakeDriverInstance { id = "fake-main" driverName = "fake" + binary = "/usr/local/bin/fake-driver" } } diff --git a/internal/daemon/config.go b/internal/daemon/config.go index 41280cd..1244d32 100644 --- a/internal/daemon/config.go +++ b/internal/daemon/config.go @@ -15,7 +15,6 @@ type Config struct { SocketPath string // UNIX socket for CLI mutative ops SnapshotEveryEvents int SnapshotEveryPeriod time.Duration - DriversTOMLPath string // resolved against DataDir in Run; "@data/drivers.toml" is the sentinel CarportSocketDir string // resolved against DataDir in Run; "@data/carport" is the sentinel ConfigDir string // resolved against DataDir in Run; "@data/config" is the sentinel } @@ -39,9 +38,6 @@ func (c *Config) WithDefaults() { if c.SnapshotEveryPeriod == 0 { c.SnapshotEveryPeriod = time.Hour } - if c.DriversTOMLPath == "" { - c.DriversTOMLPath = "@data/drivers.toml" - } if c.CarportSocketDir == "" { c.CarportSocketDir = "@data/carport" } diff --git a/internal/daemon/daemon.go b/internal/daemon/daemon.go index 35daa96..2242614 100644 --- a/internal/daemon/daemon.go +++ b/internal/daemon/daemon.go @@ -45,6 +45,9 @@ import ( "github.com/fdatoo/gohome/internal/web" ) +// Compile-time assertion: *carport.Host must satisfy config.CarportManager. +var _ config.CarportManager = (*carport.Host)(nil) + type Daemon struct { cfg Config logger *slog.Logger @@ -183,17 +186,12 @@ func (d *Daemon) Run(ctx context.Context) error { } // Phase 4.5: carport — driver supervisor - driversTOML := d.cfg.DriversTOMLPath - if driversTOML == "@data/drivers.toml" { - driversTOML = filepath.Join(dataDir, "drivers.toml") - } socketDir := d.cfg.CarportSocketDir if socketDir == "@data/carport" { socketDir = filepath.Join(dataDir, "carport") } cport, err := carport.New(carport.HostConfig{ - DriversTOMLPath: driversTOML, - SocketDir: socketDir, + SocketDir: socketDir, }, d.db, d.store, d.registry, d.logger, d.metrics) if err != nil { return fmt.Errorf("carport: %w", err) @@ -217,7 +215,7 @@ func (d *Daemon) Run(ctx context.Context) error { mainPkl := filepath.Join(configDir, "main.pkl") switch _, statErr := os.Stat(mainPkl); { case statErr == nil: - cfgMgr, err := config.NewManager(ctx, configDir, d.store, &nopCarportManager{}) + cfgMgr, err := config.NewManager(ctx, configDir, d.store, d.carport) if err != nil { return fmt.Errorf("config manager: %w", err) } @@ -599,14 +597,3 @@ func (a *carportAdapter) Dispatch(ctx context.Context, entityID, capability stri } return &starlark.DispatchResult{Ok: res.GetOk(), Error: res.GetErrorMessage()}, nil } - -// nopCarportManager satisfies config.CarportManager until carport.Host gains -// RegisterInstance/UnregisterInstance methods (C5+). -type nopCarportManager struct{} - -func (n *nopCarportManager) RegisterInstance(_ context.Context, _, _ string, _ []byte) error { - return nil -} -func (n *nopCarportManager) UnregisterInstance(_ context.Context, _ string) error { - return nil -} diff --git a/proto/gohome/config/v1/snapshot.proto b/proto/gohome/config/v1/snapshot.proto index ea7954e..df82862 100644 --- a/proto/gohome/config/v1/snapshot.proto +++ b/proto/gohome/config/v1/snapshot.proto @@ -30,6 +30,7 @@ message DriverInstanceConfig { string driver_name = 2; bytes config_hash = 3; bytes params = 4; + string binary = 5; } message EntityConfig {