From d6abc1286fa6377aaac94b2c2a5afa8f11104da4 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Thu, 23 Apr 2026 11:58:04 -0700 Subject: [PATCH] fix(workspace): auto-fill model from template's runtime_config when missing (#1779) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends the existing "read runtime from template config.yaml" preflight to also pre-fill `model` from the template's runtime_config.model (current format) or top-level `model:` (legacy format). Without this, any create path that names a template but doesn't pass an explicit model produced a workspace with empty model — and hermes-agent's compiled-in Anthropic fallback ran with whatever key the user did provide, 401'ing at the first A2A call. Affected paths (all produced broken workspaces before this change): - TemplatePalette "Deploy" button (POSTs only name + template + tier) - Direct API / script callers (MCP, CI scripts) - Anyone copying an existing workspace's template name without model PR #1714 fixed the canvas CreateWorkspaceDialog's hermes branch — when the user typed template="hermes" in the dialog, a provider picker + model auto-fill kicked in. But TemplatePalette and direct API calls bypassed that dialog entirely, so the trap stayed open. Fix is backend-side so it catches every caller at once (defense in depth). The parser is line-based + a minimal state var tracking whether the current line sits under `runtime_config:` — matches the existing fragile-but-safe style used for `runtime:` above. Strings are trimmed of quote wrappers so both `model: x` and `model: "x"` round-trip. Explicit model in the payload still wins — we only pre-fill when payload.Model is empty. Added TestWorkspaceCreate_ CallerModelOverridesTemplateDefault to pin that contract. ## Tests - TestWorkspaceCreate_TemplateDefaultsMissingRuntimeAndModel — the hermes-trap fix: runtime=hermes + model=nousresearch/... inherits from template when payload omits both. - TestWorkspaceCreate_TemplateDefaultsLegacyTopLevelModel — legacy top-level `model:` still fills. - TestWorkspaceCreate_CallerModelOverridesTemplateDefault — explicit payload.model NOT overwritten. - Full suite `go test -race ./...` stays green. ## Complementary work in flight - PR molecule-core#1772 — fixes the E2E Staging SaaS which had the same trap on its own POST body (missing provider prefix). - Canvas TemplatePalette could still surface a richer per-template key picker (deferred; MissingKeysModal already handles keys, and the default model now flows from the template config). Co-authored-by: Hongming Wang Co-authored-by: Claude Opus 4.7 (1M context) Co-authored-by: molecule-ai[bot] <276602405+molecule-ai[bot]@users.noreply.github.com> --- .../internal/handlers/workspace.go | 45 ++++- .../internal/handlers/workspace_test.go | 168 ++++++++++++++++++ 2 files changed, 206 insertions(+), 7 deletions(-) diff --git a/workspace-server/internal/handlers/workspace.go b/workspace-server/internal/handlers/workspace.go index 6af680f1..c55f1543 100644 --- a/workspace-server/internal/handlers/workspace.go +++ b/workspace-server/internal/handlers/workspace.go @@ -95,9 +95,18 @@ func (h *WorkspaceHandler) Create(c *gin.Context) { payload.Tier = 1 } - // Detect runtime from template config.yaml if not specified in request. - // Must happen before DB insert so the correct runtime is persisted. - if payload.Runtime == "" && payload.Template != "" { + // Detect runtime + default model from template config.yaml when the + // caller omitted them. Must happen before DB insert so persisted + // fields match the template's intent. + // + // Model default pre-fills the hermes-trap gap (PR #1714 + TemplatePalette + // patch): any create path (canvas dialog, TemplatePalette, direct API) + // that names a template but forgets a model slug now inherits the + // template's `runtime_config.model` — without it, hermes-agent falls + // 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). + if payload.Template != "" && (payload.Runtime == "" || payload.Model == "") { // #226: payload.Template is attacker-controllable. resolveInsideRoot // rejects absolute paths and any ".." that escapes configsDir so the // provisioner can't be pointed at host directories. @@ -111,10 +120,32 @@ func (h *WorkspaceHandler) Create(c *gin.Context) { if readErr != nil { log.Printf("Create: could not read config.yaml for template %q: %v", payload.Template, readErr) } - for _, line := range strings.Split(string(cfgData), "\n") { - line = strings.TrimSpace(line) - if strings.HasPrefix(line, "runtime:") { - payload.Runtime = strings.TrimSpace(strings.TrimPrefix(line, "runtime:")) + // Two-pass line scanner: the old parser found top-level `runtime:` + // by substring match on trimmed lines. We extend it to also find + // the nested `runtime_config.model:` (new format) and top-level + // `model:` (legacy format). A minimal state var tracks whether + // we're inside the runtime_config block based on indentation. + inRuntimeConfig := false + for _, rawLine := range strings.Split(string(cfgData), "\n") { + // Track indentation to detect block transitions. + trimmed := strings.TrimLeft(rawLine, " \t") + indented := len(rawLine) > len(trimmed) + if !indented { + // Left the runtime_config block (or never entered it). + inRuntimeConfig = strings.HasPrefix(trimmed, "runtime_config:") + } + stripped := strings.TrimSpace(rawLine) + switch { + case payload.Runtime == "" && !indented && strings.HasPrefix(stripped, "runtime:") && !strings.HasPrefix(stripped, "runtime_config"): + payload.Runtime = strings.TrimSpace(strings.TrimPrefix(stripped, "runtime:")) + 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:")), `"'`) + case payload.Model == "" && indented && inRuntimeConfig && strings.HasPrefix(stripped, "model:"): + // Nested `runtime_config.model:` — current format (hermes etc.). + payload.Model = strings.Trim(strings.TrimSpace(strings.TrimPrefix(stripped, "model:")), `"'`) + } + if payload.Runtime != "" && payload.Model != "" { break } } diff --git a/workspace-server/internal/handlers/workspace_test.go b/workspace-server/internal/handlers/workspace_test.go index cc9289b9..b98f42d3 100644 --- a/workspace-server/internal/handlers/workspace_test.go +++ b/workspace-server/internal/handlers/workspace_test.go @@ -6,6 +6,8 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "os" + "path/filepath" "testing" "github.com/DATA-DOG/go-sqlmock" @@ -1215,3 +1217,169 @@ func TestWorkspaceUpdate_BudgetLimitOnly_Ignored(t *testing.T) { t.Errorf("unexpected DB call for budget_limit: %v", err) } } + +// TestWorkspaceCreate_TemplateDefaultsMissingRuntimeAndModel covers the +// hermes-trap case: a caller (TemplatePalette, direct API, script) POSTs +// /workspaces with only a template name + no runtime + no model. The +// handler must read the template's config.yaml and fill in both fields +// BEFORE DB insert — otherwise hermes-agent auto-detects provider +// wrong and 401s downstream (PR #1714 context). +// +// Uses the nested runtime_config.model format current templates use; +// legacy top-level `model:` is covered by the Legacy test below. +func TestWorkspaceCreate_TemplateDefaultsMissingRuntimeAndModel(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + + // Stage a hermes-like template inside the configsDir the handler reads. + configsDir := t.TempDir() + templateDir := filepath.Join(configsDir, "hermes-template") + if err := os.MkdirAll(templateDir, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + cfg := []byte(`name: Hermes Agent +tier: 2 +runtime: hermes +runtime_config: + model: nousresearch/hermes-4-70b +`) + if err := os.WriteFile(filepath.Join(templateDir, "config.yaml"), cfg, 0o644); err != nil { + t.Fatalf("write cfg: %v", err) + } + + handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", configsDir) + + mock.ExpectBegin() + // Request omits runtime + model; handler must fill from the template + // and hand the completed values to the INSERT. + mock.ExpectExec("INSERT INTO workspaces"). + WithArgs( + sqlmock.AnyArg(), "Hermes Agent", nil, 1, "hermes", + sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil)). + 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":"Hermes Agent","template":"hermes-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.StatusCreated { + t.Fatalf("expected 201, got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet expectations: %v", err) + } +} + +// TestWorkspaceCreate_TemplateDefaultsLegacyTopLevelModel covers +// pre-runtime_config templates that declare `model:` at the top level. +// These should still surface the default via the same auto-fill. +func TestWorkspaceCreate_TemplateDefaultsLegacyTopLevelModel(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + + configsDir := t.TempDir() + templateDir := filepath.Join(configsDir, "legacy-template") + if err := os.MkdirAll(templateDir, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + cfg := []byte(`name: Legacy Agent +tier: 1 +runtime: langgraph +model: anthropic:claude-sonnet-4-5 +`) + if err := os.WriteFile(filepath.Join(templateDir, "config.yaml"), cfg, 0o644); err != nil { + t.Fatalf("write cfg: %v", err) + } + + handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", configsDir) + + mock.ExpectBegin() + mock.ExpectExec("INSERT INTO workspaces"). + WithArgs( + sqlmock.AnyArg(), "Legacy Agent", nil, 1, "langgraph", + sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil)). + 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":"Legacy Agent","template":"legacy-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.StatusCreated { + t.Fatalf("expected 201, got %d: %s", w.Code, w.Body.String()) + } +} + +// TestWorkspaceCreate_CallerModelOverridesTemplateDefault asserts that +// when the caller passes an explicit `model`, we DO NOT overwrite it +// with the template's default. The pre-fill only happens on empty. +func TestWorkspaceCreate_CallerModelOverridesTemplateDefault(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + + configsDir := t.TempDir() + templateDir := filepath.Join(configsDir, "hermes-template") + if err := os.MkdirAll(templateDir, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + cfg := []byte(`runtime: hermes +runtime_config: + model: nousresearch/hermes-4-70b +`) + if err := os.WriteFile(filepath.Join(templateDir, "config.yaml"), cfg, 0o644); err != nil { + t.Fatalf("write cfg: %v", err) + } + + handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", configsDir) + + mock.ExpectBegin() + // Caller explicitly chose minimax — template's hermes-4-70b must NOT win. + // The INSERT only passes runtime to the DB (model goes to agent_card / + // downstream config); we verify runtime == "hermes" and rely on the + // absence of a handler error to mean the model passthrough was honored. + mock.ExpectExec("INSERT INTO workspaces"). + WithArgs( + sqlmock.AnyArg(), "Custom Hermes", nil, 1, "hermes", + sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil)). + 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":"Custom Hermes","template":"hermes-template","model":"minimax/MiniMax-M2.7"}` + 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()) + } +}