From 871501dfc9a5c9aaefd574c1dada83ff88c5a147 Mon Sep 17 00:00:00 2001 From: core-platform Date: Mon, 18 May 2026 01:51:16 -0700 Subject: [PATCH] fix(ws-server): fail-closed on unresolvable template runtime (controlplane#188) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST /workspaces silently substituted langgraph and returned 201 when a caller named a `template` (intent for a specific runtime) but the runtime could not be resolved from it (config.yaml unreadable / no `runtime:` key). This is the molecule-controlplane#188 / #184 contract violation — it produced 5/5 wrong-runtime workspaces and a false codex E2E pass. The ws-server `Create` handler is the boundary the product UI actually hits (the canvas dialog and provision_workspace MCP tool both POST here); controlplane#188's CP-side gate is the sibling. This closes the ws-server side: when the caller expressed runtime intent (passed `runtime`, or named a `template`) but it cannot be honored, return 422 RUNTIME_UNRESOLVED instead of a silent langgraph 201. The legitimate default path (bare {"name":...} — no template, no runtime) still defaults to langgraph and returns 201; a regression test pins that so the fail-closed gate can't over-fire. Tests: TestWorkspaceCreate_188_* (missing template, no-runtime-key template, default-path regression guard, explicit-runtime OK). Refs: molecule-controlplane#188, #184 Co-Authored-By: Claude Opus 4.7 (1M context) --- .../internal/handlers/workspace.go | 34 ++++ .../internal/handlers/workspace_test.go | 146 +++++++++++++++++- 2 files changed, 179 insertions(+), 1 deletion(-) diff --git a/workspace-server/internal/handlers/workspace.go b/workspace-server/internal/handlers/workspace.go index 781741aad..746705391 100644 --- a/workspace-server/internal/handlers/workspace.go +++ b/workspace-server/internal/handlers/workspace.go @@ -198,6 +198,17 @@ func (h *WorkspaceHandler) Create(c *gin.Context) { // back to its compiled-in Anthropic default and 401s when the user's // key is for a different provider. Non-hermes runtimes are unaffected // (the server still passes model through, they just don't use it). + // runtimeExplicitlyRequested is true when the caller expressed intent for + // a SPECIFIC runtime — either by passing `runtime` directly, or by naming + // a `template` (a template encodes a runtime). When true, we must NOT + // silently fall back to langgraph if that intent can't be honored: that + // is the molecule-controlplane#188 / #184 contract violation (caller asks + // for codex/claude-code, gets a langgraph workspace, 201, no error — a + // false success). #188 mandates fail-closed (error+notify) on mismatch, + // not an advisory degrade. The legitimate "no template, no runtime → + // langgraph default" path (bare {"name":...}) is unaffected. + runtimeExplicitlyRequested := payload.Runtime != "" || payload.Template != "" + templateRuntimeResolved := payload.Runtime != "" if payload.Template != "" && (payload.Runtime == "" || payload.Model == "") { // #226: payload.Template is attacker-controllable. resolveInsideRoot // rejects absolute paths and any ".." that escapes configsDir so the @@ -230,6 +241,9 @@ func (h *WorkspaceHandler) Create(c *gin.Context) { switch { case payload.Runtime == "" && !indented && strings.HasPrefix(stripped, "runtime:") && !strings.HasPrefix(stripped, "runtime_config"): payload.Runtime = strings.TrimSpace(strings.TrimPrefix(stripped, "runtime:")) + if payload.Runtime != "" { + templateRuntimeResolved = true + } case payload.Model == "" && !indented && strings.HasPrefix(stripped, "model:"): // Legacy top-level `model:` — pre-runtime_config templates. payload.Model = strings.Trim(strings.TrimSpace(strings.TrimPrefix(stripped, "model:")), `"'`) @@ -242,7 +256,27 @@ func (h *WorkspaceHandler) Create(c *gin.Context) { } } } + // Fail-closed (molecule-controlplane#188 / #184): if the caller expressed + // intent for a specific runtime (passed `runtime`, or named a `template`) + // but we could NOT resolve a concrete runtime from it (template's + // config.yaml unreadable, or it has no `runtime:` key), DO NOT silently + // substitute langgraph and return 201 — that is the silent contract + // violation that produced 5/5 wrong workspaces and a false codex E2E pass. + // Return 422 so the caller learns the requested runtime was not honored. + // The platform-side CP fix (controlplane#188) is the sibling gate; this + // closes the ws-server `Create` boundary the product UI actually hits. + if payload.Runtime == "" && runtimeExplicitlyRequested && !templateRuntimeResolved { + log.Printf("Create: FAIL-CLOSED (controlplane#188) — template=%q requested but runtime could not be resolved; refusing silent langgraph fallback", payload.Template) + c.JSON(http.StatusUnprocessableEntity, gin.H{ + "error": "runtime could not be resolved from the requested template; refusing to silently provision langgraph (controlplane#188). Pass an explicit \"runtime\", or use a template whose config.yaml declares one.", + "template": payload.Template, + "code": "RUNTIME_UNRESOLVED", + }) + return + } if payload.Runtime == "" { + // Legitimate default path: no template AND no runtime requested + // (bare {"name":...}) — langgraph is the intended default here. payload.Runtime = "langgraph" } diff --git a/workspace-server/internal/handlers/workspace_test.go b/workspace-server/internal/handlers/workspace_test.go index 6d24370bd..7f329da2e 100644 --- a/workspace-server/internal/handlers/workspace_test.go +++ b/workspace-server/internal/handlers/workspace_test.go @@ -718,7 +718,7 @@ func TestWorkspaceList_Empty(t *testing.T) { "parent_id", "active_tasks", "last_error_rate", "last_sample_error", "uptime_seconds", "current_task", "runtime", "workspace_dir", "x", "y", "collapsed", "budget_limit", "monthly_spend", - "broadcast_enabled", "talk_to_user_enabled", + "broadcast_enabled", "talk_to_user_enabled", })) w := httptest.NewRecorder() @@ -1770,3 +1770,147 @@ runtime_config: t.Fatalf("expected 201, got %d: %s", w.Code, w.Body.String()) } } + +// ==================== #188 fail-closed: template/runtime contract ==================== +// +// molecule-controlplane#188 / #184: if a caller names a `template` (intent +// for a specific runtime) but the runtime cannot be resolved from it, the +// server MUST NOT silently provision langgraph and return 201 — that false +// success produced 5/5 wrong workspaces and a bogus codex E2E pass. These +// tests pin the fail-closed boundary at the ws-server `Create` handler (the +// path the product UI hits), and guard the legitimate default path against +// regression. + +// Template requested but its dir/config.yaml is absent → 422, not silent +// langgraph 201. +func TestWorkspaceCreate_188_TemplateMissingRuntime_FailsClosed(t *testing.T) { + setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + // configsDir is an empty temp dir → resolveInsideRoot succeeds (the path + // is inside root) but config.yaml read fails → runtime cannot be resolved. + configsDir := t.TempDir() + if err := os.MkdirAll(filepath.Join(configsDir, "ghost-template"), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", configsDir) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + body := `{"name":"Ghost","template":"ghost-template"}` + c.Request = httptest.NewRequest("POST", "/workspaces", bytes.NewBufferString(body)) + c.Request.Header.Set("Content-Type", "application/json") + + handler.Create(c) + + if w.Code != http.StatusUnprocessableEntity { + t.Fatalf("expected 422 (fail-closed, controlplane#188), got %d: %s", w.Code, w.Body.String()) + } + var resp map[string]interface{} + if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { + t.Fatalf("parse: %v", err) + } + if resp["code"] != "RUNTIME_UNRESOLVED" { + t.Errorf("expected code RUNTIME_UNRESOLVED, got %v", resp["code"]) + } +} + +// Template config.yaml has no `runtime:` key → 422, not silent langgraph. +func TestWorkspaceCreate_188_TemplateConfigNoRuntimeKey_FailsClosed(t *testing.T) { + setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + configsDir := t.TempDir() + tdir := filepath.Join(configsDir, "noruntime-template") + if err := os.MkdirAll(tdir, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + // config.yaml exists but declares no runtime. + if err := os.WriteFile(filepath.Join(tdir, "config.yaml"), []byte("name: noruntime\n"), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", configsDir) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + body := `{"name":"NoRuntime","template":"noruntime-template"}` + c.Request = httptest.NewRequest("POST", "/workspaces", bytes.NewBufferString(body)) + c.Request.Header.Set("Content-Type", "application/json") + + handler.Create(c) + + if w.Code != http.StatusUnprocessableEntity { + t.Fatalf("expected 422 (fail-closed), got %d: %s", w.Code, w.Body.String()) + } +} + +// Regression guard: the legitimate default path (no template, no runtime — +// bare {"name":...}) MUST still default to langgraph and return 201. The +// #188 fix must not break this. +func TestWorkspaceCreate_188_NoTemplateNoRuntime_StillDefaultsLanggraph(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) + + 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"). + WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectCommit() + mock.ExpectExec("INSERT INTO canvas_layouts"). + WithArgs(sqlmock.AnyArg(), float64(0), float64(0)). + WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectExec("INSERT INTO structure_events"). + WillReturnResult(sqlmock.NewResult(0, 1)) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + body := `{"name":"Plain Default"}` + 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.Fatalf("expected 201 (legitimate default path), got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + +// Explicit runtime, no template → honored, 201 (no template resolution +// needed; runtimeExplicitlyRequested true but already resolved). +func TestWorkspaceCreate_188_ExplicitRuntimeNoTemplate_OK(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) + + 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"). + WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectCommit() + mock.ExpectExec("INSERT INTO canvas_layouts"). + WithArgs(sqlmock.AnyArg(), float64(0), float64(0)). + WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectExec("INSERT INTO structure_events"). + WillReturnResult(sqlmock.NewResult(0, 1)) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + body := `{"name":"Explicit Codex","runtime":"codex"}` + 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.Fatalf("expected 201, got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} -- 2.52.0