Merge pull request #2390 from Molecule-AI/auto/local-provisioner-api-interface

refactor(handlers): widen WorkspaceHandler.provisioner to LocalProvisionerAPI interface (#2369)

workspace-server/internal/handlers/team.go

@@ -25,20 +25,28 @@ import (
 // NULL auth_token — same drift class as the SaaS bug fixed in #2366.
 type TeamHandler struct {
 	broadcaster *events.Broadcaster
-	provisioner *provisioner.Provisioner
+	// provisioner is interface-typed (#2369) for the same reason as
+	// WorkspaceHandler.provisioner — Stop is the only call site here
+	// and it's on the LocalProvisionerAPI surface, so widening is free
+	// and symmetric with WorkspaceHandler.
+	provisioner provisioner.LocalProvisionerAPI
 	wh          *WorkspaceHandler
 	platformURL string
 	configsDir  string
 }
 
 func NewTeamHandler(b *events.Broadcaster, p *provisioner.Provisioner, wh *WorkspaceHandler, platformURL, configsDir string) *TeamHandler {
-	return &TeamHandler{
+	h := &TeamHandler{
 		broadcaster: b,
-		provisioner: p,
 		wh:          wh,
 		platformURL: platformURL,
 		configsDir:  configsDir,
 	}
+	// Avoid the typed-nil interface trap (see NewWorkspaceHandler note).
+	if p != nil {
+		h.provisioner = p
+	}
+	return h
 }
 
 // Expand handles POST /workspaces/:id/expand

workspace-server/internal/handlers/workspace.go

@@ -34,7 +34,15 @@ type WorkspaceHandler struct {
 	// satisfies the interface — see the compile-time assertion in
 	// internal/events/broadcaster.go.
 	broadcaster events.EventEmitter
-	provisioner *provisioner.Provisioner
+	// provisioner narrowed from `*provisioner.Provisioner` to the
+	// provisioner.LocalProvisionerAPI interface (#2369) so tests can
+	// substitute a stub without standing up the real Docker daemon.
+	// Production callers still pass *provisioner.Provisioner via
+	// NewWorkspaceHandler, which satisfies the interface — see the
+	// compile-time assertion in internal/provisioner/local_provisioner_api.go.
+	// Mirrors cpProv's interface-typed field for symmetry across both
+	// backends.
+	provisioner provisioner.LocalProvisionerAPI
 	// cpProv narrowed from `*provisioner.CPProvisioner` to the
 	// provisioner.CPProvisionerAPI interface (#1814) so tests can
 	// substitute a stub without standing up the real CP HTTP client +
@@ -60,12 +68,22 @@ type WorkspaceHandler struct {
 }
 
 func NewWorkspaceHandler(b events.EventEmitter, p *provisioner.Provisioner, platformURL, configsDir string) *WorkspaceHandler {
-	return &WorkspaceHandler{
+	h := &WorkspaceHandler{
 		broadcaster: b,
-		provisioner: p,
 		platformURL: platformURL,
 		configsDir:  configsDir,
 	}
+	// Only assign p when the concrete pointer is non-nil. Without this
+	// guard, a `NewWorkspaceHandler(..., nil, ...)` call (which all the
+	// non-Docker test fixtures use) yields a typed-nil interface — the
+	// `if h.provisioner != nil` checks scattered through the SaaS-vs-
+	// Docker fork would incorrectly evaluate as non-nil and route into
+	// the Docker path. Mirrors the pattern documented in the Go FAQ
+	// "Why is my nil error value not equal to nil?".
+	if p != nil {
+		h.provisioner = p
+	}
+	return h
 }
 
 // SetCPProvisioner wires the control plane provisioner for SaaS tenants.

workspace-server/internal/provisioner/backend_contract_test.go

@@ -51,6 +51,14 @@ import (
 // Kept minimal on purpose; expand only when a new cross-backend
 // behavior needs a contract test. Implementation-private methods stay
 // off this interface.
+//
+// NOTE: this is the SHARED-CONTRACT subset — the methods both backends
+// must implement. The handler-facing surface is wider (LocalProvisionerAPI
+// includes ExecRead / RemoveVolume / VolumeHasFile / WriteAuthTokenToVolume,
+// which are Docker-specific because the CP backend delegates volume
+// management to EC2 boot scripts). Don't widen Backend with Docker-only
+// methods — split into per-backend interfaces if a method only one side
+// implements.
 type Backend interface {
 	Start(ctx context.Context, cfg WorkspaceConfig) (string, error)
 	Stop(ctx context.Context, workspaceID string) error
@@ -225,4 +233,40 @@ func TestZeroValuedBackends_NoPanic(t *testing.T) {
 			t.Errorf("nil *CPProvisioner.Stop: got %v, want ErrNoBackend", err)
 		}
 	})
+
+	// Methods extended to the LocalProvisionerAPI interface in #2369 —
+	// every one needs the same nil-receiver guard so the typed-nil
+	// interface case (handler tests passing nil concrete *Provisioner
+	// to NewWorkspaceHandler, which the constructor now elides into a
+	// real nil interface) doesn't regress in the future.
+	t.Run("nil_Provisioner.Start", func(t *testing.T) {
+		var p *Provisioner
+		if _, err := p.Start(context.Background(), WorkspaceConfig{}); !errors.Is(err, ErrNoBackend) {
+			t.Errorf("nil *Provisioner.Start: got %v, want ErrNoBackend", err)
+		}
+	})
+	t.Run("nil_Provisioner.ExecRead", func(t *testing.T) {
+		var p *Provisioner
+		if _, err := p.ExecRead(context.Background(), "any", "any"); !errors.Is(err, ErrNoBackend) {
+			t.Errorf("nil *Provisioner.ExecRead: got %v, want ErrNoBackend", err)
+		}
+	})
+	t.Run("nil_Provisioner.RemoveVolume", func(t *testing.T) {
+		var p *Provisioner
+		if err := p.RemoveVolume(context.Background(), "any"); !errors.Is(err, ErrNoBackend) {
+			t.Errorf("nil *Provisioner.RemoveVolume: got %v, want ErrNoBackend", err)
+		}
+	})
+	t.Run("nil_Provisioner.VolumeHasFile", func(t *testing.T) {
+		var p *Provisioner
+		if _, err := p.VolumeHasFile(context.Background(), "any", "config.yaml"); !errors.Is(err, ErrNoBackend) {
+			t.Errorf("nil *Provisioner.VolumeHasFile: got %v, want ErrNoBackend", err)
+		}
+	})
+	t.Run("nil_Provisioner.WriteAuthTokenToVolume", func(t *testing.T) {
+		var p *Provisioner
+		if err := p.WriteAuthTokenToVolume(context.Background(), "any", "tok"); !errors.Is(err, ErrNoBackend) {
+			t.Errorf("nil *Provisioner.WriteAuthTokenToVolume: got %v, want ErrNoBackend", err)
+		}
+	})
 }

workspace-server/internal/provisioner/local_provisioner_api.go

@@ -0,0 +1,36 @@
+package provisioner
+
+import "context"
+
+// LocalProvisionerAPI is the contract WorkspaceHandler uses to talk to the
+// local Docker provisioner. Mirrors CPProvisionerAPI so the handler types
+// both backends symmetrically — no more "interface for SaaS, concrete for
+// Docker" asymmetry that blocked easy mocking and made future
+// pluggable-backend work harder than it needed to be (issue #2369).
+//
+// Method set is the union of methods WorkspaceHandler actually calls
+// today. Adding a new handler call site that reaches into Provisioner
+// means widening this interface explicitly — the same review-surfacing
+// contract CPProvisionerAPI already enforces.
+//
+// Keep narrow: the *Provisioner type also exposes ListManagedContainerIDPrefixes,
+// CopyTemplateToContainer, DockerClient, etc. Those are consumed by
+// non-handler call sites (orphan sweeper, registry health watcher) which
+// type their dependency as `*Provisioner` directly. Pulling them onto
+// this interface would force every handler test to stub method bodies
+// they never exercise.
+type LocalProvisionerAPI interface {
+	Start(ctx context.Context, cfg WorkspaceConfig) (string, error)
+	Stop(ctx context.Context, workspaceID string) error
+	IsRunning(ctx context.Context, workspaceID string) (bool, error)
+	ExecRead(ctx context.Context, containerName, filePath string) ([]byte, error)
+	RemoveVolume(ctx context.Context, workspaceID string) error
+	VolumeHasFile(ctx context.Context, workspaceID, relPath string) (bool, error)
+	WriteAuthTokenToVolume(ctx context.Context, workspaceID, token string) error
+}
+
+// Compile-time assertion: *Provisioner satisfies LocalProvisionerAPI.
+// Catches a future method-signature drift at build time instead of at
+// the NewWorkspaceHandler call site. Mirror of the assertion in
+// cp_provisioner.go for CPProvisionerAPI.
+var _ LocalProvisionerAPI = (*Provisioner)(nil)

workspace-server/internal/provisioner/provisioner.go

@@ -272,6 +272,9 @@ func InternalURL(workspaceID string) string {
 
 // Start provisions and starts a workspace container.
 func (p *Provisioner) Start(ctx context.Context, cfg WorkspaceConfig) (string, error) {
+	if p == nil || p.cli == nil {
+		return "", ErrNoBackend
+	}
 	name := ContainerName(cfg.WorkspaceID)
 	configVolume := ConfigVolumeName(cfg.WorkspaceID)
 
@@ -807,6 +810,9 @@ func (p *Provisioner) CopyToContainer(ctx context.Context, containerID, dstPath
 // ExecRead runs "cat <filePath>" in an existing container and returns the output.
 // Used to read config files from a running container before stopping it.
 func (p *Provisioner) ExecRead(ctx context.Context, containerName, filePath string) ([]byte, error) {
+	if p == nil || p.cli == nil {
+		return nil, ErrNoBackend
+	}
 	exec, err := p.cli.ContainerExecCreate(ctx, containerName, container.ExecOptions{
 		Cmd:          []string{"cat", filePath},
 		AttachStdout: true,
@@ -891,6 +897,9 @@ func (p *Provisioner) ReadFromVolume(ctx context.Context, volumeName, filePath s
 // Uses a throwaway alpine container to write directly to the named volume,
 // bypassing the container lifecycle entirely.
 func (p *Provisioner) WriteAuthTokenToVolume(ctx context.Context, workspaceID, token string) error {
+	if p == nil || p.cli == nil {
+		return ErrNoBackend
+	}
 	volName := ConfigVolumeName(workspaceID)
 	resp, err := p.cli.ContainerCreate(ctx, &container.Config{
 		Image: "alpine",
@@ -936,6 +945,9 @@ func (p *Provisioner) execInContainer(ctx context.Context, containerID string, c
 // Also removes the claude-sessions volume (best-effort, may not exist
 // for non claude-code runtimes). Issue #12.
 func (p *Provisioner) RemoveVolume(ctx context.Context, workspaceID string) error {
+	if p == nil || p.cli == nil {
+		return ErrNoBackend
+	}
 	volName := ConfigVolumeName(workspaceID)
 	if err := p.cli.VolumeRemove(ctx, volName, true); err != nil {
 		return fmt.Errorf("failed to remove volume %s: %w", volName, err)
@@ -1139,6 +1151,9 @@ var ErrNoConfigSource = fmt.Errorf("no config.yaml source: template path missing
 // volume read-only. Returns (false, nil) if the file is absent and
 // (false, err) only on Docker-level failures.
 func (p *Provisioner) VolumeHasFile(ctx context.Context, workspaceID, relPath string) (bool, error) {
+	if p == nil || p.cli == nil {
+		return false, ErrNoBackend
+	}
 	volName := ConfigVolumeName(workspaceID)
 	// Confirm the volume exists first — Docker auto-creates on bind otherwise.
 	if _, err := p.cli.VolumeInspect(ctx, volName); err != nil {