From 9b930d8e39e537c3651f90cd3fabd5628d136ba6 Mon Sep 17 00:00:00 2001 From: hongming-pc2 Date: Sun, 10 May 2026 03:11:41 -0700 Subject: [PATCH] fix(provisioner): export MOLECULE_MODEL (canonical model env) + read it first; drop stray brace in delegation_test.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit internal#226 follow-up #1. `molecule_runtime.config` resolves the picked model as `MOLECULE_MODEL` > `MODEL` > (legacy) `MODEL_PROVIDER` (#280) — this side of the boundary now matches: - applyRuntimeModelEnv reads `MOLECULE_MODEL` ahead of `MODEL` / `MODEL_PROVIDER`, and exports BOTH `MOLECULE_MODEL` and `MODEL` (the latter kept for back-compat with everything that already reads `os.environ["MODEL"]`). So a workspace whose secrets carry `MOLECULE_MODEL` (the unambiguous name) is honoured, and the `MODEL_PROVIDER` misnomer — which got set to provider slugs ("minimax") and even runtime names ("claude-code") — is the lowest- priority fallback, exactly as on the runtime side. - the resolution-order comment is updated to flag MODEL_PROVIDER as the legacy-and-misleadingly-named var. Also drops a stray trailing `}` in delegation_test.go (committed in 97768272 "test(delegation): add isDeliveryConfirmedSuccess helper") that made `internal/handlers` fail to parse — one of the things keeping the package from compiling for tests. Tests: TestApplyRuntimeModelEnv_SetsUniversalMODELForAllRuntimes extended to assert MOLECULE_MODEL mirrors MODEL on every case, plus two new cases (MOLECULE_MODEL env fallback; MOLECULE_MODEL beats MODEL_PROVIDER). Could not run `go test ./internal/handlers/` locally — the package is still blocked behind `internal/plugins` `SourceResolver` redeclaration (the #248 plugin-router/resolver refactor, Core-BE's lane); CI validates once that lands. The applyRuntimeModelEnv change is mechanical (same shape as the existing `MODEL` handling) — reviewer please eyeball. Companion: molecule-core#280 (runtime config.py side), molecule-ai-workspace-template-claude-code#14 (CLI-stream-error surfacing). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../internal/handlers/delegation_test.go | 1 - .../internal/handlers/workspace_provision.go | 40 +++++++++++-------- .../workspace_provision_shared_test.go | 35 +++++++++++++--- 3 files changed, 53 insertions(+), 23 deletions(-) diff --git a/workspace-server/internal/handlers/delegation_test.go b/workspace-server/internal/handlers/delegation_test.go index 427e71b2..38c63206 100644 --- a/workspace-server/internal/handlers/delegation_test.go +++ b/workspace-server/internal/handlers/delegation_test.go @@ -1262,4 +1262,3 @@ func TestExecuteDelegation_CleanProxyResponse_Unchanged(t *testing.T) { t.Errorf("unmet sqlmock expectations: %v", err) } } -} diff --git a/workspace-server/internal/handlers/workspace_provision.go b/workspace-server/internal/handlers/workspace_provision.go index 57d6c5a6..bf910ff4 100644 --- a/workspace-server/internal/handlers/workspace_provision.go +++ b/workspace-server/internal/handlers/workspace_provision.go @@ -717,13 +717,16 @@ func deriveProviderFromModelSlug(model string) string { func applyRuntimeModelEnv(envVars map[string]string, runtime, model string) { // Resolution order (priority high → low): // 1. payload.Model (caller passed the canvas-picked model id verbatim) - // 2. envVars["MODEL"] (workspace_secret persisted by /org/import via + // 2. envVars["MOLECULE_MODEL"] (the canonical, unambiguous name) + // 3. envVars["MODEL"] (workspace_secret persisted by /org/import via // the persona env file — MODEL=MiniMax-M2.7-highspeed etc.) - // 3. envVars["MODEL_PROVIDER"] (legacy: this secret was historically a - // *model id* set by canvas Save+Restart's PUT /model; on the - // post-2026-05-08 persona-env convention it's a *provider slug* - // (e.g. "minimax") which is NOT a valid model id, so this fallback - // only fires when MODEL is absent.) + // 4. envVars["MODEL_PROVIDER"] (legacy + misleadingly named: it carries + // a *model id*, never the provider — that's LLM_PROVIDER. Historically + // set by canvas Save+Restart's PUT /model; the post-2026-05-08 + // persona-env convention sometimes (mis)set it to a provider slug + // ("minimax") or a runtime name ("claude-code"), neither a valid + // model id — see internal#226. Only fires when the better-named + // vars are absent.) // // Pre-fix bug: this function unconditionally OVERWROTE envVars["MODEL"] // with the MODEL_PROVIDER slug (when payload.Model was empty), wiping @@ -736,6 +739,9 @@ func applyRuntimeModelEnv(envVars map[string]string, runtime, model string) { // and the workspace template's adapter routed to providers[0] // (anthropic-oauth) and wedged at SDK initialize. Caught 2026-05-08 // during Phase 4 verification of template-claude-code PR #9. + if model == "" { + model = envVars["MOLECULE_MODEL"] + } if model == "" { model = envVars["MODEL"] } @@ -746,16 +752,18 @@ func applyRuntimeModelEnv(envVars map[string]string, runtime, model string) { return } - // Universal MODEL env var — every adapter that wants to honour the - // canvas-picked model (instead of its template's default) reads this. - // molecule-runtime's workspace/config.py already falls back to MODEL - // for runtime_config.model (#194). Without this line, the user's - // canvas selection is silently dropped on every templated provision — - // confirmed via crash-loop diagnosis on 2026-05-02 where MiniMax - // picks booted with model=sonnet (template default) and demanded - // CLAUDE_CODE_OAUTH_TOKEN. Set it FIRST so the per-runtime branches - // below can still layer on additional vendor-specific names without - // fighting over the canonical one. + // Canonical model env vars — molecule-runtime's workspace/config.py + // resolves the picked model as MOLECULE_MODEL > MODEL > (legacy) + // MODEL_PROVIDER (#280). Export both new names so adapters can read + // either; MODEL stays for backwards compat with everything that + // already reads os.environ["MODEL"] (the claude-code adapter does, + // since #194). Without this, the user's canvas selection is silently + // dropped on every templated provision — confirmed via crash-loop + // diagnosis on 2026-05-02 where MiniMax picks booted with model=sonnet + // (template default) and demanded CLAUDE_CODE_OAUTH_TOKEN. Set these + // FIRST so the per-runtime branches below can layer on additional + // vendor-specific names without fighting over the canonical one. + envVars["MOLECULE_MODEL"] = model envVars["MODEL"] = model switch runtime { diff --git a/workspace-server/internal/handlers/workspace_provision_shared_test.go b/workspace-server/internal/handlers/workspace_provision_shared_test.go index 7a85d118..01101ddf 100644 --- a/workspace-server/internal/handlers/workspace_provision_shared_test.go +++ b/workspace-server/internal/handlers/workspace_provision_shared_test.go @@ -665,46 +665,62 @@ func TestApplyRuntimeModelEnv_SetsUniversalMODELForAllRuntimes(t *testing.T) { runtime string model string modelProviderEnv string + moleculeModelEnv string wantMODEL string wantHermesDefault string // empty string = must be unset }{ { - name: "claude-code: picked model populates MODEL", + name: "claude-code: picked model populates MODEL + MOLECULE_MODEL", runtime: "claude-code", model: "MiniMax-M2", wantMODEL: "MiniMax-M2", }, { - name: "hermes: picked model populates BOTH MODEL and HERMES_DEFAULT_MODEL", + name: "hermes: picked model populates MODEL, MOLECULE_MODEL, HERMES_DEFAULT_MODEL", runtime: "hermes", model: "minimax/MiniMax-M2.7", wantMODEL: "minimax/MiniMax-M2.7", wantHermesDefault: "minimax/MiniMax-M2.7", }, { - name: "langgraph: picked model populates MODEL (no vendor-specific name)", + name: "langgraph: picked model populates MODEL + MOLECULE_MODEL (no vendor-specific name)", runtime: "langgraph", model: "anthropic:claude-opus-4-7", wantMODEL: "anthropic:claude-opus-4-7", }, { - name: "crewai: picked model populates MODEL (no vendor-specific name)", + name: "crewai: picked model populates MODEL + MOLECULE_MODEL (no vendor-specific name)", runtime: "crewai", model: "openai:gpt-4o", wantMODEL: "openai:gpt-4o", }, { - name: "empty model + empty MODEL_PROVIDER fallback: nothing set", + name: "empty model + no env fallback: nothing set", runtime: "claude-code", model: "", }, { - name: "empty model + MODEL_PROVIDER fallback hits: MODEL set from secret", + name: "empty model + MODEL_PROVIDER fallback hits: MODEL/MOLECULE_MODEL set from secret", runtime: "claude-code", model: "", modelProviderEnv: "MiniMax-M2", wantMODEL: "MiniMax-M2", }, + { + name: "empty model + MOLECULE_MODEL env fallback hits (canonical name)", + runtime: "claude-code", + model: "", + moleculeModelEnv: "opus", + wantMODEL: "opus", + }, + { + name: "MOLECULE_MODEL beats MODEL_PROVIDER when both set (misnomer guard, internal#226)", + runtime: "claude-code", + model: "", + moleculeModelEnv: "opus", + modelProviderEnv: "claude-code", + wantMODEL: "opus", + }, } for _, tc := range cases { @@ -713,11 +729,18 @@ func TestApplyRuntimeModelEnv_SetsUniversalMODELForAllRuntimes(t *testing.T) { if tc.modelProviderEnv != "" { envVars["MODEL_PROVIDER"] = tc.modelProviderEnv } + if tc.moleculeModelEnv != "" { + envVars["MOLECULE_MODEL"] = tc.moleculeModelEnv + } applyRuntimeModelEnv(envVars, tc.runtime, tc.model) if got := envVars["MODEL"]; got != tc.wantMODEL { t.Errorf("MODEL = %q, want %q", got, tc.wantMODEL) } + // MOLECULE_MODEL (the canonical name) must mirror MODEL exactly. + if got := envVars["MOLECULE_MODEL"]; got != tc.wantMODEL { + t.Errorf("MOLECULE_MODEL = %q, want %q", got, tc.wantMODEL) + } if got := envVars["HERMES_DEFAULT_MODEL"]; got != tc.wantHermesDefault { t.Errorf("HERMES_DEFAULT_MODEL = %q, want %q", got, tc.wantHermesDefault) } -- 2.45.2