fix(workspace): auto-fill model from template's runtime_config when missing (#1779)
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 <hongmingwang.rabbit@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: molecule-ai[bot] <276602405+molecule-ai[bot]@users.noreply.github.com>
This commit is contained in:
Hongming Wang
GitHub
parent
a5ca587516
commit
d6abc1286f
@ -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
|
||||
}
|
||||
}
|
||||
|
||||
@ -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())
|
||||
}
|
||||
}
|
||||
|
||||
Loading…
Reference in New Issue
Block a user