diff --git a/workspace-server/internal/handlers/github_token.go b/workspace-server/internal/handlers/github_token.go index ce9492a9d..f00714a96 100644 --- a/workspace-server/internal/handlers/github_token.go +++ b/workspace-server/internal/handlers/github_token.go @@ -159,7 +159,8 @@ func generateAppInstallationToken() (string, time.Time, error) { req, _ := http.NewRequest("POST", fmt.Sprintf("https://api.github.com/app/installations/%d/access_tokens", installID), nil) req.Header.Set("Authorization", "Bearer "+signed) req.Header.Set("Accept", "application/vnd.github+json") - resp, err := http.DefaultClient.Do(req) + client := &http.Client{Timeout: 30 * time.Second} + resp, err := client.Do(req) if err != nil { return "", time.Time{}, err } diff --git a/workspace-server/internal/handlers/handlers_additional_test.go b/workspace-server/internal/handlers/handlers_additional_test.go index 0e13600d5..fc24f3fb6 100644 --- a/workspace-server/internal/handlers/handlers_additional_test.go +++ b/workspace-server/internal/handlers/handlers_additional_test.go @@ -33,7 +33,7 @@ func TestWorkspaceCreate_WithParentID(t *testing.T) { // Default tier is 3 (Privileged) — see workspace.go create-handler comment. // delivery_mode defaults to "push" when payload omits it (#2339). mock.ExpectExec("INSERT INTO workspaces"). - WithArgs(sqlmock.AnyArg(), "Child Agent", nil, 3, "langgraph", sqlmock.AnyArg(), &parentID, nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push"). + WithArgs(sqlmock.AnyArg(), "Child Agent", nil, 3, "langgraph", sqlmock.AnyArg(), &parentID, nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push", (*string)(nil), (*int)(nil)). WillReturnResult(sqlmock.NewResult(0, 1)) mock.ExpectCommit() mock.ExpectExec("INSERT INTO canvas_layouts"). @@ -69,7 +69,7 @@ func TestWorkspaceCreate_ExplicitClaudeCodeRuntime(t *testing.T) { mock.ExpectBegin() // delivery_mode defaults to "push" when payload omits it (#2339). mock.ExpectExec("INSERT INTO workspaces"). - WithArgs(sqlmock.AnyArg(), "CC Agent", nil, 2, "claude-code", sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push"). + WithArgs(sqlmock.AnyArg(), "CC Agent", nil, 2, "claude-code", sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push", (*string)(nil), (*int)(nil)). WillReturnResult(sqlmock.NewResult(0, 1)) mock.ExpectCommit() mock.ExpectExec("INSERT INTO canvas_layouts"). @@ -291,7 +291,7 @@ func TestWorkspaceCreate_MaxConcurrentTasksOverride(t *testing.T) { mock.ExpectBegin() mock.ExpectExec("INSERT INTO workspaces"). - WithArgs(sqlmock.AnyArg(), "Leader Agent", nil, 3, "claude-code", sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), 3, "push"). + WithArgs(sqlmock.AnyArg(), "Leader Agent", nil, 3, "claude-code", sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), 3, "push", (*string)(nil), (*int)(nil)). WillReturnResult(sqlmock.NewResult(0, 1)) mock.ExpectCommit() mock.ExpectExec("INSERT INTO canvas_layouts"). diff --git a/workspace-server/internal/handlers/handlers_test.go b/workspace-server/internal/handlers/handlers_test.go index 7ce01b239..58ee8d942 100644 --- a/workspace-server/internal/handlers/handlers_test.go +++ b/workspace-server/internal/handlers/handlers_test.go @@ -368,7 +368,7 @@ func TestWorkspaceCreate(t *testing.T) { // Default tier is 3 (Privileged) — see workspace.go create-handler comment. // delivery_mode defaults to "push" when payload omits it (#2339). mock.ExpectExec("INSERT INTO workspaces"). - WithArgs(sqlmock.AnyArg(), "Test Agent", nil, 3, "langgraph", sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push"). + WithArgs(sqlmock.AnyArg(), "Test Agent", nil, 3, "langgraph", sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push", (*string)(nil), (*int)(nil)). WillReturnResult(sqlmock.NewResult(0, 1)) // Expect transaction commit (no secrets in this payload) diff --git a/workspace-server/internal/handlers/workspace.go b/workspace-server/internal/handlers/workspace.go index c89622fde..93b918409 100644 --- a/workspace-server/internal/handlers/workspace.go +++ b/workspace-server/internal/handlers/workspace.go @@ -214,6 +214,11 @@ func (h *WorkspaceHandler) Create(c *gin.Context) { c.JSON(http.StatusBadRequest, gin.H{"error": "invalid workspace fields"}) return } + // #1686 Phase 1: validate per-workspace compute overrides. + if err := models.ValidateComputeConfig(payload.Compute); err != nil { + c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) + return + } id := uuid.New().String() awarenessNamespace := workspaceAwarenessNamespace(id) @@ -398,11 +403,22 @@ func (h *WorkspaceHandler) Create(c *gin.Context) { // double-click. Helper retries with " (2)", " (3)", … up to maxNameSuffix, // returns the actually-persisted name (which we MUST thread back into // payload + broadcast so the canvas displays what the DB has). + var computeInstanceType *string + var computeVolumeRootGB *int + if payload.Compute != nil { + if payload.Compute.InstanceType != "" { + computeInstanceType = &payload.Compute.InstanceType + } + if payload.Compute.Volume.RootGB != 0 { + computeVolumeRootGB = &payload.Compute.Volume.RootGB + } + } + const insertWorkspaceSQL = ` - INSERT INTO workspaces (id, name, role, tier, runtime, awareness_namespace, status, parent_id, workspace_dir, workspace_access, budget_limit, max_concurrent_tasks, delivery_mode) - VALUES ($1, $2, $3, $4, $5, $6, 'provisioning', $7, $8, $9, $10, $11, $12) + INSERT INTO workspaces (id, name, role, tier, runtime, awareness_namespace, status, parent_id, workspace_dir, workspace_access, budget_limit, max_concurrent_tasks, delivery_mode, compute_instance_type, compute_volume_root_gb) + VALUES ($1, $2, $3, $4, $5, $6, 'provisioning', $7, $8, $9, $10, $11, $12, $13, $14) ` - insertArgs := []any{id, payload.Name, role, payload.Tier, payload.Runtime, awarenessNamespace, payload.ParentID, workspaceDir, workspaceAccess, payload.BudgetLimit, maxConcurrent, deliveryMode} + insertArgs := []any{id, payload.Name, role, payload.Tier, payload.Runtime, awarenessNamespace, payload.ParentID, workspaceDir, workspaceAccess, payload.BudgetLimit, maxConcurrent, deliveryMode, computeInstanceType, computeVolumeRootGB} persistedName, currentTx, err := insertWorkspaceWithNameRetry( ctx, tx, diff --git a/workspace-server/internal/handlers/workspace_budget_test.go b/workspace-server/internal/handlers/workspace_budget_test.go index 4652e2932..bb1e87715 100644 --- a/workspace-server/internal/handlers/workspace_budget_test.go +++ b/workspace-server/internal/handlers/workspace_budget_test.go @@ -157,6 +157,8 @@ func TestWorkspaceBudget_Create_WithLimit(t *testing.T) { &budgetVal, // budget_limit ($10) models.DefaultMaxConcurrentTasks, // max_concurrent_tasks default "push", // delivery_mode default (#2339) + (*string)(nil), // compute_instance_type default + (*int)(nil), // compute_volume_root_gb default ). WillReturnResult(sqlmock.NewResult(0, 1)) mock.ExpectCommit() diff --git a/workspace-server/internal/handlers/workspace_provision.go b/workspace-server/internal/handlers/workspace_provision.go index 11da5f448..3a5774951 100644 --- a/workspace-server/internal/handlers/workspace_provision.go +++ b/workspace-server/internal/handlers/workspace_provision.go @@ -309,9 +309,31 @@ func (h *WorkspaceHandler) buildProvisionerConfig( // RuntimeImages[Runtime] :latest lookup, which is what the dead // reader's sql.ErrNoRows path was producing already. Image: "", + // Compute overrides (nullable — omitted = platform-managed default). + // Issue #1686 Phase 1. + InstanceType: extractComputeInstanceType(payload.Compute), + VolumeRootGB: extractComputeVolumeRootGB(payload.Compute), } } +// extractComputeInstanceType returns the instance type from a ComputeConfig, +// or nil when cfg is nil or the field is empty. +func extractComputeInstanceType(cfg *models.ComputeConfig) *string { + if cfg != nil && cfg.InstanceType != "" { + return &cfg.InstanceType + } + return nil +} + +// extractComputeVolumeRootGB returns the root volume size from a ComputeConfig, +// or nil when cfg is nil or the field is zero. +func extractComputeVolumeRootGB(cfg *models.ComputeConfig) *int { + if cfg != nil && cfg.Volume.RootGB != 0 { + return &cfg.Volume.RootGB + } + return nil +} + // issueAndInjectToken rotates the workspace auth token and injects the // plaintext into cfg.ConfigFiles[".auth_token"] so it is written into the // /configs volume by WriteFilesToContainer immediately after the container diff --git a/workspace-server/internal/handlers/workspace_provision_test.go b/workspace-server/internal/handlers/workspace_provision_test.go index a5e46d64a..f00983a49 100644 --- a/workspace-server/internal/handlers/workspace_provision_test.go +++ b/workspace-server/internal/handlers/workspace_provision_test.go @@ -779,6 +779,75 @@ func TestBuildProvisionerConfig_WorkspacePathFromEnv(t *testing.T) { } } +// TestBuildProvisionerConfig_ComputeOverrides verifies that #1686 Phase 1 +// compute fields (instance_type + volume.root_gb) are threaded from the +// create payload into the provisioner config. +func TestBuildProvisionerConfig_ComputeOverrides(t *testing.T) { + mock := setupTestDB(t) + mock.ExpectQuery(`SELECT COALESCE\(workspace_dir`). + WithArgs("ws-compute"). + WillReturnRows(sqlmock.NewRows([]string{"workspace_dir", "workspace_access"}).AddRow("", "none")) + + broadcaster := newTestBroadcaster() + handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) + + cfg := handler.buildProvisionerConfig( + context.Background(), + "ws-compute", + "", + nil, + models.CreateWorkspacePayload{ + Tier: 2, + Runtime: "python", + Compute: &models.ComputeConfig{ + InstanceType: "g4dn.xlarge", + Volume: models.ComputeVolume{RootGB: 256}, + }, + }, + nil, + "", + "workspace:ws-compute", + ) + + if cfg.InstanceType == nil || *cfg.InstanceType != "g4dn.xlarge" { + t.Errorf("InstanceType = %v, want g4dn.xlarge", cfg.InstanceType) + } + if cfg.VolumeRootGB == nil || *cfg.VolumeRootGB != 256 { + t.Errorf("VolumeRootGB = %v, want 256", cfg.VolumeRootGB) + } +} + +// TestBuildProvisionerConfig_ComputeNil verifies backward compat: when the +// payload omits compute, the provisioner config fields are nil so the CP +// applies its own defaults. +func TestBuildProvisionerConfig_ComputeNil(t *testing.T) { + mock := setupTestDB(t) + mock.ExpectQuery(`SELECT COALESCE\(workspace_dir`). + WithArgs("ws-no-compute"). + WillReturnRows(sqlmock.NewRows([]string{"workspace_dir", "workspace_access"}).AddRow("", "none")) + + broadcaster := newTestBroadcaster() + handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) + + cfg := handler.buildProvisionerConfig( + context.Background(), + "ws-no-compute", + "", + nil, + models.CreateWorkspacePayload{Tier: 1, Runtime: "python"}, + nil, + "", + "workspace:ws-no-compute", + ) + + if cfg.InstanceType != nil { + t.Errorf("InstanceType = %v, want nil", cfg.InstanceType) + } + if cfg.VolumeRootGB != nil { + t.Errorf("VolumeRootGB = %v, want nil", cfg.VolumeRootGB) + } +} + // ==================== issueAndInjectToken (issue #418) ==================== // TestIssueAndInjectToken_HappyPath verifies that on a normal (re)provision the diff --git a/workspace-server/internal/handlers/workspace_test.go b/workspace-server/internal/handlers/workspace_test.go index 7f329da2e..00b4d44c2 100644 --- a/workspace-server/internal/handlers/workspace_test.go +++ b/workspace-server/internal/handlers/workspace_test.go @@ -8,6 +8,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "strings" "testing" "time" @@ -342,7 +343,7 @@ func TestWorkspaceCreate_DBInsertError(t *testing.T) { // Transaction begins, workspace INSERT fails, transaction is rolled back. mock.ExpectBegin() mock.ExpectExec("INSERT INTO workspaces"). - WithArgs(sqlmock.AnyArg(), "Failing Agent", nil, 3, "langgraph", sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push"). + WithArgs(sqlmock.AnyArg(), "Failing Agent", nil, 3, "langgraph", sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push", (*string)(nil), (*int)(nil)). WillReturnError(sql.ErrConnDone) mock.ExpectRollback() @@ -364,6 +365,94 @@ func TestWorkspaceCreate_DBInsertError(t *testing.T) { } } +// TestWorkspaceCreate_InvalidCompute verifies #1686 Phase 1 create-time +// validation: bad instance_type or volume.root_gb returns 400 before any +// DB call. +func TestWorkspaceCreate_InvalidCompute(t *testing.T) { + broadcaster := newTestBroadcaster() + handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) + + cases := []struct { + name string + body string + want string + }{ + { + name: "instance_type too long", + body: `{"name":"Bad Type","compute":{"instance_type":"` + strings.Repeat("x", 65) + `"}}`, + want: "compute.instance_type too long", + }, + { + name: "root_gb too small", + body: `{"name":"Small Disk","compute":{"volume":{"root_gb":16}}}`, + want: "compute.volume.root_gb must be at least 32", + }, + { + name: "root_gb too large", + body: `{"name":"Big Disk","compute":{"volume":{"root_gb":4096}}}`, + want: "compute.volume.root_gb exceeds maximum 2048", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request = httptest.NewRequest("POST", "/workspaces", bytes.NewBufferString(tc.body)) + c.Request.Header.Set("Content-Type", "application/json") + + handler.Create(c) + if w.Code != http.StatusBadRequest { + t.Errorf("expected 400, got %d: %s", w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), tc.want) { + t.Errorf("body %q should contain %q", w.Body.String(), tc.want) + } + }) + } +} + +// TestWorkspaceCreate_WithComputeOverrides verifies that valid #1686 Phase 1 +// compute fields are persisted into the workspaces table. +func TestWorkspaceCreate_WithComputeOverrides(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) + + mock.ExpectBegin() + instanceType := "g4dn.xlarge" + rootGB := 256 + mock.ExpectExec("INSERT INTO workspaces"). + WithArgs(sqlmock.AnyArg(), "GPU Agent", nil, 3, "python", sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push", &instanceType, &rootGB). + WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectCommit() + + mock.ExpectExec("INSERT INTO canvas_layouts"). + WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectExec("INSERT INTO structure_events"). + WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectExec("INSERT INTO structure_events"). + WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectExec(`UPDATE workspaces SET status =`). + WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectExec("INSERT INTO workspace_config"). + WillReturnResult(sqlmock.NewResult(0, 1)) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + body := `{"name":"GPU Agent","runtime":"python","compute":{"instance_type":"g4dn.xlarge","volume":{"root_gb":256}}}` + c.Request = httptest.NewRequest("POST", "/workspaces", bytes.NewBufferString(body)) + c.Request.Header.Set("Content-Type", "application/json") + + handler.Create(c) + if w.Code != http.StatusCreated { + t.Errorf("expected 201, got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + func TestWorkspaceCreate_DefaultsApplied(t *testing.T) { mock := setupTestDB(t) setupTestRedis(t) @@ -375,7 +464,7 @@ func TestWorkspaceCreate_DefaultsApplied(t *testing.T) { // Expect workspace INSERT with defaulted tier=3 (Privileged — the // handler default in workspace.go), runtime="langgraph" mock.ExpectExec("INSERT INTO workspaces"). - WithArgs(sqlmock.AnyArg(), "Default Agent", nil, 3, "langgraph", sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push"). + WithArgs(sqlmock.AnyArg(), "Default Agent", nil, 3, "langgraph", sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push", (*string)(nil), (*int)(nil)). WillReturnResult(sqlmock.NewResult(0, 1)) mock.ExpectCommit() @@ -423,7 +512,7 @@ func TestWorkspaceCreate_SaaSHardForcesTier4(t *testing.T) { mock.ExpectBegin() mock.ExpectExec("INSERT INTO workspaces"). - WithArgs(sqlmock.AnyArg(), "SaaS External Agent", nil, 4, "external", sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push"). + WithArgs(sqlmock.AnyArg(), "SaaS External Agent", nil, 4, "external", sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push", (*string)(nil), (*int)(nil)). WillReturnResult(sqlmock.NewResult(0, 1)) mock.ExpectCommit() mock.ExpectExec("INSERT INTO canvas_layouts"). @@ -464,7 +553,7 @@ func TestWorkspaceCreate_WithSecrets_Persists(t *testing.T) { mock.ExpectBegin() mock.ExpectExec("INSERT INTO workspaces"). - WithArgs(sqlmock.AnyArg(), "Hermes Agent", nil, 3, "hermes", sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push"). + WithArgs(sqlmock.AnyArg(), "Hermes Agent", nil, 3, "hermes", sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push", (*string)(nil), (*int)(nil)). WillReturnResult(sqlmock.NewResult(0, 1)) // Secret inserted inside the same transaction. mock.ExpectExec("INSERT INTO workspace_secrets"). @@ -576,7 +665,7 @@ func TestWorkspaceCreate_ExternalURL_SSRFSafe(t *testing.T) { mock.ExpectBegin() mock.ExpectExec("INSERT INTO workspaces"). - WithArgs(sqlmock.AnyArg(), "Ext Agent", nil, 3, "external", sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push"). + WithArgs(sqlmock.AnyArg(), "Ext Agent", nil, 3, "external", sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push", (*string)(nil), (*int)(nil)). WillReturnResult(sqlmock.NewResult(0, 1)) mock.ExpectCommit() // External URL update (localhost is explicitly allowed by validateAgentURL). @@ -615,7 +704,7 @@ func TestWorkspaceCreate_KimiRuntime_PreservesLabel(t *testing.T) { mock.ExpectBegin() mock.ExpectExec("INSERT INTO workspaces"). - WithArgs(sqlmock.AnyArg(), "Kimi Agent", nil, 3, "kimi", sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push"). + WithArgs(sqlmock.AnyArg(), "Kimi Agent", nil, 3, "kimi", sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push", (*string)(nil), (*int)(nil)). WillReturnResult(sqlmock.NewResult(0, 1)) mock.ExpectCommit() // Pre-register flow: awaiting_agent + runtime preserved as "kimi" @@ -1639,7 +1728,7 @@ runtime_config: mock.ExpectExec("INSERT INTO workspaces"). WithArgs( sqlmock.AnyArg(), "Hermes Agent", nil, 3, "hermes", - sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push"). + sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push", (*string)(nil), (*int)(nil)). WillReturnResult(sqlmock.NewResult(0, 1)) mock.ExpectCommit() mock.ExpectExec("INSERT INTO canvas_layouts"). @@ -1696,7 +1785,7 @@ model: anthropic:claude-sonnet-4-5 mock.ExpectExec("INSERT INTO workspaces"). WithArgs( sqlmock.AnyArg(), "Legacy Agent", nil, 3, "langgraph", - sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push"). + sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push", (*string)(nil), (*int)(nil)). WillReturnResult(sqlmock.NewResult(0, 1)) mock.ExpectCommit() mock.ExpectExec("INSERT INTO canvas_layouts"). @@ -1749,7 +1838,7 @@ runtime_config: mock.ExpectExec("INSERT INTO workspaces"). WithArgs( sqlmock.AnyArg(), "Custom Hermes", nil, 3, "hermes", - sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push"). + sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push", (*string)(nil), (*int)(nil)). WillReturnResult(sqlmock.NewResult(0, 1)) mock.ExpectCommit() mock.ExpectExec("INSERT INTO canvas_layouts"). @@ -1855,7 +1944,7 @@ func TestWorkspaceCreate_188_NoTemplateNoRuntime_StillDefaultsLanggraph(t *testi mock.ExpectBegin() mock.ExpectExec("INSERT INTO workspaces"). - WithArgs(sqlmock.AnyArg(), "Plain Default", nil, 3, "langgraph", sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push"). + WithArgs(sqlmock.AnyArg(), "Plain Default", nil, 3, "langgraph", sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push", (*string)(nil), (*int)(nil)). WillReturnResult(sqlmock.NewResult(0, 1)) mock.ExpectCommit() mock.ExpectExec("INSERT INTO canvas_layouts"). @@ -1890,7 +1979,7 @@ func TestWorkspaceCreate_188_ExplicitRuntimeNoTemplate_OK(t *testing.T) { mock.ExpectBegin() mock.ExpectExec("INSERT INTO workspaces"). - WithArgs(sqlmock.AnyArg(), "Explicit Codex", nil, 3, "codex", sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push"). + WithArgs(sqlmock.AnyArg(), "Explicit Codex", nil, 3, "codex", sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push", (*string)(nil), (*int)(nil)). WillReturnResult(sqlmock.NewResult(0, 1)) mock.ExpectCommit() mock.ExpectExec("INSERT INTO canvas_layouts"). diff --git a/workspace-server/internal/models/workspace.go b/workspace-server/internal/models/workspace.go index 9139fc5b9..c1f608a5f 100644 --- a/workspace-server/internal/models/workspace.go +++ b/workspace-server/internal/models/workspace.go @@ -3,6 +3,7 @@ package models import ( "database/sql" "encoding/json" + "fmt" "time" ) @@ -45,6 +46,10 @@ type Workspace struct { // forced to route updates through a parent workspace. Default true // (preserves existing behaviour for all workspaces). TalkToUserEnabled bool `json:"talk_to_user_enabled" db:"talk_to_user_enabled"` + // Compute overrides (nullable — omitted = platform-managed default). + // Issue #1686 Phase 1. + ComputeInstanceType *string `json:"compute_instance_type,omitempty" db:"compute_instance_type"` + ComputeVolumeRootGB *int `json:"compute_volume_root_gb,omitempty" db:"compute_volume_root_gb"` // Canvas layout fields (from JOIN) X float64 `json:"x"` Y float64 `json:"y"` @@ -154,6 +159,40 @@ type MemorySeed struct { Scope string `json:"scope" yaml:"scope"` // LOCAL, TEAM, GLOBAL } +// ComputeVolume holds per-workspace disk configuration. +type ComputeVolume struct { + RootGB int `json:"root_gb"` +} + +// ComputeConfig holds per-workspace EC2 compute overrides. +// Omitted at create time means "use platform-managed defaults". +type ComputeConfig struct { + InstanceType string `json:"instance_type"` + Volume ComputeVolume `json:"volume"` +} + +// ValidateComputeConfig performs create-time validation on compute overrides. +// Returns nil when cfg is nil (omitted = platform-managed default). +func ValidateComputeConfig(cfg *ComputeConfig) error { + if cfg == nil { + return nil + } + if cfg.InstanceType != "" { + if len(cfg.InstanceType) > 64 { + return fmt.Errorf("compute.instance_type too long (max 64 chars)") + } + } + if cfg.Volume.RootGB != 0 { + if cfg.Volume.RootGB < 32 { + return fmt.Errorf("compute.volume.root_gb must be at least 32") + } + if cfg.Volume.RootGB > 2048 { + return fmt.Errorf("compute.volume.root_gb exceeds maximum 2048") + } + } + return nil +} + type CreateWorkspacePayload struct { Name string `json:"name" binding:"required"` Role string `json:"role"` @@ -180,6 +219,9 @@ type CreateWorkspacePayload struct { // MaxConcurrentTasks caps parallel A2A + cron dispatch. 0 means use // DefaultMaxConcurrentTasks. Leaders typically set 3. MaxConcurrentTasks int `json:"max_concurrent_tasks"` + // Compute is an optional per-workspace EC2 shape override. + // Omitted = platform-managed default (current behaviour). + Compute *ComputeConfig `json:"compute,omitempty"` Canvas struct { X float64 `json:"x"` Y float64 `json:"y"` diff --git a/workspace-server/internal/models/workspace_compute_test.go b/workspace-server/internal/models/workspace_compute_test.go new file mode 100644 index 000000000..86b4aa5d8 --- /dev/null +++ b/workspace-server/internal/models/workspace_compute_test.go @@ -0,0 +1,90 @@ +package models + +import "testing" + +func TestValidateComputeConfig_NilIsValid(t *testing.T) { + if err := ValidateComputeConfig(nil); err != nil { + t.Errorf("nil compute config should be valid, got: %v", err) + } +} + +func TestValidateComputeConfig_EmptyIsValid(t *testing.T) { + cfg := &ComputeConfig{} + if err := ValidateComputeConfig(cfg); err != nil { + t.Errorf("empty compute config should be valid, got: %v", err) + } +} + +func TestValidateComputeConfig_ValidOverrides(t *testing.T) { + cfg := &ComputeConfig{ + InstanceType: "g4dn.xlarge", + Volume: ComputeVolume{RootGB: 256}, + } + if err := ValidateComputeConfig(cfg); err != nil { + t.Errorf("valid overrides should pass, got: %v", err) + } +} + +func TestValidateComputeConfig_InstanceTypeTooLong(t *testing.T) { + longName := string(make([]byte, 65)) + for i := range longName { + longName = longName[:i] + "x" + longName[i+1:] + } + cfg := &ComputeConfig{InstanceType: longName} + if err := ValidateComputeConfig(cfg); err == nil { + t.Error("expected error for instance_type > 64 chars") + } else if err.Error() != "compute.instance_type too long (max 64 chars)" { + t.Errorf("unexpected error message: %q", err.Error()) + } +} + +func TestValidateComputeConfig_RootGBTooSmall(t *testing.T) { + cfg := &ComputeConfig{Volume: ComputeVolume{RootGB: 31}} + if err := ValidateComputeConfig(cfg); err == nil { + t.Error("expected error for root_gb < 32") + } else if err.Error() != "compute.volume.root_gb must be at least 32" { + t.Errorf("unexpected error message: %q", err.Error()) + } +} + +func TestValidateComputeConfig_RootGBTooLarge(t *testing.T) { + cfg := &ComputeConfig{Volume: ComputeVolume{RootGB: 2049}} + if err := ValidateComputeConfig(cfg); err == nil { + t.Error("expected error for root_gb > 2048") + } else if err.Error() != "compute.volume.root_gb exceeds maximum 2048" { + t.Errorf("unexpected error message: %q", err.Error()) + } +} + +func TestValidateComputeConfig_BoundaryValues(t *testing.T) { + cases := []struct { + name string + cfg ComputeConfig + ok bool + }{ + {"min root_gb", ComputeConfig{Volume: ComputeVolume{RootGB: 32}}, true}, + {"max root_gb", ComputeConfig{Volume: ComputeVolume{RootGB: 2048}}, true}, + {"just under min", ComputeConfig{Volume: ComputeVolume{RootGB: 31}}, false}, + {"just over max", ComputeConfig{Volume: ComputeVolume{RootGB: 2049}}, false}, + {"exactly 64 char type", ComputeConfig{InstanceType: string(make([]byte, 64))}, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + // fill the 64-char case with 'x' + if tc.cfg.InstanceType != "" { + b := make([]byte, len(tc.cfg.InstanceType)) + for i := range b { + b[i] = 'x' + } + tc.cfg.InstanceType = string(b) + } + err := ValidateComputeConfig(&tc.cfg) + if tc.ok && err != nil { + t.Errorf("expected valid, got: %v", err) + } + if !tc.ok && err == nil { + t.Error("expected invalid, got nil") + } + }) + } +} diff --git a/workspace-server/internal/provisioner/cp_provisioner.go b/workspace-server/internal/provisioner/cp_provisioner.go index 8f6f0c557..2c44bcd45 100644 --- a/workspace-server/internal/provisioner/cp_provisioner.go +++ b/workspace-server/internal/provisioner/cp_provisioner.go @@ -163,6 +163,10 @@ type cpProvisionRequest struct { // collectCPConfigFiles which rejects symlinks and non-regular files // before including them. Serialised as base64 to avoid JSON escaping. ConfigFiles map[string]string `json:"config_files,omitempty"` + // Compute overrides (nullable — omitted = platform-managed default). + // Issue #1686 Phase 1. + InstanceType *string `json:"instance_type,omitempty"` + VolumeRootGB *int `json:"volume_root_gb,omitempty"` } type cpProvisionResponse struct { @@ -206,13 +210,15 @@ func (p *CPProvisioner) Start(ctx context.Context, cfg WorkspaceConfig) (string, } req := cpProvisionRequest{ - OrgID: p.orgID, - WorkspaceID: cfg.WorkspaceID, - Runtime: cfg.Runtime, - Tier: cfg.Tier, - PlatformURL: cfg.PlatformURL, - Env: env, - ConfigFiles: configFiles, + OrgID: p.orgID, + WorkspaceID: cfg.WorkspaceID, + Runtime: cfg.Runtime, + Tier: cfg.Tier, + PlatformURL: cfg.PlatformURL, + Env: env, + ConfigFiles: configFiles, + InstanceType: cfg.InstanceType, + VolumeRootGB: cfg.VolumeRootGB, } body, err := json.Marshal(req) diff --git a/workspace-server/internal/provisioner/cp_provisioner_test.go b/workspace-server/internal/provisioner/cp_provisioner_test.go index 6f1ea07e8..6561cc84e 100644 --- a/workspace-server/internal/provisioner/cp_provisioner_test.go +++ b/workspace-server/internal/provisioner/cp_provisioner_test.go @@ -1062,3 +1062,75 @@ func TestCollectCPConfigFiles_RejectsRootSymlink(t *testing.T) { t.Errorf("expected symlink-related error, got: %v", err) } } + +// TestStart_ComputeOverrides — when WorkspaceConfig carries InstanceType and +// VolumeRootGB, they must be forwarded in the cpProvisionRequest body so the +// CP can pass them to EC2 RunInstances. Regression guard for #1686 Phase 1. +func TestStart_ComputeOverrides(t *testing.T) { + var gotBody cpProvisionRequest + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if err := json.NewDecoder(r.Body).Decode(&gotBody); err != nil { + t.Errorf("decode request: %v", err) + } + w.WriteHeader(http.StatusCreated) + _, _ = io.WriteString(w, `{"instance_id":"i-compute","state":"pending"}`) + })) + defer srv.Close() + + p := &CPProvisioner{baseURL: srv.URL, orgID: "org-1", httpClient: srv.Client()} + instanceType := "g4dn.xlarge" + volumeRootGB := 256 + _, err := p.Start(context.Background(), WorkspaceConfig{ + WorkspaceID: "ws-1", + Runtime: "python", + Tier: 2, + PlatformURL: "http://tenant", + InstanceType: &instanceType, + VolumeRootGB: &volumeRootGB, + }) + if err != nil { + t.Fatalf("Start: %v", err) + } + if gotBody.InstanceType == nil || *gotBody.InstanceType != "g4dn.xlarge" { + t.Errorf("instance_type = %v, want g4dn.xlarge", gotBody.InstanceType) + } + if gotBody.VolumeRootGB == nil || *gotBody.VolumeRootGB != 256 { + t.Errorf("volume_root_gb = %v, want 256", gotBody.VolumeRootGB) + } +} + +// TestStart_ComputeOmittedWhenNil — when WorkspaceConfig has no compute +// overrides, the JSON body must omit the keys entirely (omitempty) so CP +// applies its own defaults rather than empty/zero values. +func TestStart_ComputeOmittedWhenNil(t *testing.T) { + var raw json.RawMessage + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if err := json.NewDecoder(r.Body).Decode(&raw); err != nil { + t.Errorf("decode request: %v", err) + } + w.WriteHeader(http.StatusCreated) + _, _ = io.WriteString(w, `{"instance_id":"i-default","state":"pending"}`) + })) + defer srv.Close() + + p := &CPProvisioner{baseURL: srv.URL, orgID: "org-1", httpClient: srv.Client()} + _, err := p.Start(context.Background(), WorkspaceConfig{ + WorkspaceID: "ws-1", + Runtime: "python", + Tier: 1, + PlatformURL: "http://tenant", + }) + if err != nil { + t.Fatalf("Start: %v", err) + } + var decoded map[string]interface{} + if err := json.Unmarshal(raw, &decoded); err != nil { + t.Fatalf("unmarshal raw body: %v", err) + } + if _, ok := decoded["instance_type"]; ok { + t.Errorf("instance_type should be omitted when nil") + } + if _, ok := decoded["volume_root_gb"]; ok { + t.Errorf("volume_root_gb should be omitted when nil") + } +} diff --git a/workspace-server/internal/provisioner/provisioner.go b/workspace-server/internal/provisioner/provisioner.go index 164c951bf..9f71cafe2 100644 --- a/workspace-server/internal/provisioner/provisioner.go +++ b/workspace-server/internal/provisioner/provisioner.go @@ -105,6 +105,11 @@ type WorkspaceConfig struct { WorkspaceAccess string // #65: "none" (default), "read_only", or "read_write" ResetClaudeSession bool // #12: if true, discard the claude-sessions volume before start (fresh session dir) + // Compute overrides (nullable — omitted = platform-managed default). + // Issue #1686 Phase 1. + InstanceType *string `json:"instance_type,omitempty"` + VolumeRootGB *int `json:"volume_root_gb,omitempty"` + // Image, when non-empty, overrides the runtime→image lookup. CP // (molecule-controlplane) is the single SSOT for runtime image digest // pins via its migrations/027_runtime_image_pins table — the pin is @@ -726,6 +731,16 @@ func buildContainerEnv(cfg WorkspaceConfig) []string { } env = append(env, fmt.Sprintf("%s=%s", k, v)) } + // #1687: alias GH_PAT → GH_TOKEN / GITHUB_TOKEN on the READ side + // (container env assembly). gh CLI and git credential helpers look + // for these standard names; by aliasing here we avoid writing the + // forbidden keys into tenant-writer surfaces (workspace_secrets, + // envVars map, etc.). GH_PAT itself is not an SCM-write credential + // and passes through cfg.EnvVars untouched. + if pat, hasPAT := cfg.EnvVars["GH_PAT"]; hasPAT && pat != "" { + env = append(env, fmt.Sprintf("GH_TOKEN=%s", pat)) + env = append(env, fmt.Sprintf("GITHUB_TOKEN=%s", pat)) + } // Inject ADMIN_TOKEN from the platform server's environment so workspace // containers can call /admin/liveness and other admin-gated endpoints // (core#831). cp_provisioner.go handles this separately for SaaS tenants. diff --git a/workspace-server/migrations/20260523010000_workspace_compute.down.sql b/workspace-server/migrations/20260523010000_workspace_compute.down.sql new file mode 100644 index 000000000..4ffa6c4d5 --- /dev/null +++ b/workspace-server/migrations/20260523010000_workspace_compute.down.sql @@ -0,0 +1,5 @@ +ALTER TABLE workspaces + DROP COLUMN IF EXISTS compute_instance_type; + +ALTER TABLE workspaces + DROP COLUMN IF EXISTS compute_volume_root_gb; diff --git a/workspace-server/migrations/20260523010000_workspace_compute.up.sql b/workspace-server/migrations/20260523010000_workspace_compute.up.sql new file mode 100644 index 000000000..28dfef749 --- /dev/null +++ b/workspace-server/migrations/20260523010000_workspace_compute.up.sql @@ -0,0 +1,10 @@ +-- Per-workspace EC2 compute configuration (#1686 Phase 1). +-- Allows callers to override instance_type and root volume size +-- at workspace creation time. Omitted/null values preserve the +-- platform-managed default (current behaviour), so this is fully +-- backwards-compatible. +ALTER TABLE workspaces + ADD COLUMN IF NOT EXISTS compute_instance_type TEXT; + +ALTER TABLE workspaces + ADD COLUMN IF NOT EXISTS compute_volume_root_gb INTEGER;