From 97ebd1910a7d3adfa43a4f91e9edcfd1207eb42d Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Sat, 2 May 2026 21:36:24 -0700 Subject: [PATCH] fix(runtime): canvas-picked model wins universally + per-model required_env MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two surgical edits to the molecule-runtime workspace package that fix Bug B (canvas-picked model silently dropped for templated workspaces) and Bug D (preflight rejects valid auth for non-default models), universally for every adapter. Bug B — canvas-picked model dropped (config.py) ================================================ Before: load_config resolved runtime_config.model as runtime_raw.get("model") or model which means a template's `runtime_config.model: sonnet` always wins over the canvas-picked MODEL_PROVIDER env var. Surfaced 2026-05-02 during MiniMax E2E — picking MiniMax-M2.7 in canvas, server plumbed MODEL_PROVIDER=MiniMax-M2.7 correctly, but the workspace booted with sonnet because the template's verbatim config.yaml won. After: os.environ.get("MODEL_PROVIDER") or runtime_raw.get("model") or model Centralising in load_config means EVERY adapter (claude-code, hermes, codex, langgraph, future ones) gets canvas-picked-model passthrough for free — no per-adapter env-reading code required. Bug D — preflight per-model required_env (preflight.py) ======================================================== Before: preflight read the top-level required_env list, which declares the auth needed by the *default* model. A template like claude-code-default declares CLAUDE_CODE_OAUTH_TOKEN at the top level. When a user picked MiniMax instead and only set MINIMAX_API_KEY, preflight rejected the workspace with "missing CLAUDE_CODE_OAUTH_TOKEN" and the workspace crash-looped despite the user having satisfied the picked model's actual auth. After: when runtime_config.models[] declares per-entry required_env, preflight matches the picked model id (case-insensitive) and uses that entry's required_env outright instead of the top-level list. REPLACE semantics, not union — different models have *different* auth paths (OAuth vs API key vs third-party provider key); unioning would re-introduce the very crash-loop this fix closes. Surface enabling both fixes (config.py) ======================================== RuntimeConfig now carries `models: list[dict]` so the canvas Model dropdown source flows through to preflight without forcing the parser schema to grow. Malformed entries are silently dropped to match the rest of the lenient parser. Tests ===== - workspace/tests/test_preflight.py: 9 new tests covering the per-model lookup (case-insensitive, REPLACE not union, fallback to top-level when no models[] or no match, multi-entry, malformed entries dropped, etc.) - workspace/tests/test_config.py: existing 48 pass; field initialisation already covered by parser tests. - All 75 targeted tests pass locally; CI runs the full suite including coverage gate. Closes part of #246. Sibling PR opens against molecule-ai-workspace-template-claude-code for per-template defensive fixes + boot debug logging. Co-Authored-By: Claude Opus 4.7 (1M context) --- workspace/config.py | 47 ++++++++--- workspace/preflight.py | 38 +++++++++ workspace/tests/test_preflight.py | 134 ++++++++++++++++++++++++++++++ 3 files changed, 206 insertions(+), 13 deletions(-) diff --git a/workspace/config.py b/workspace/config.py index 4e199c57..6a256579 100644 --- a/workspace/config.py +++ b/workspace/config.py @@ -100,6 +100,16 @@ class RuntimeConfig: # "minimax"). Falls back to the top-level resolved # provider when empty. Adapters (hermes, claude-code, # codex) prefer this over slug-parsing the model name. + # Per-model entries surfaced in the canvas Model dropdown. Each entry is a + # raw dict with at least ``id``; ``required_env`` is the per-model auth + # list (e.g. ``{"id": "MiniMax-M2.7", "required_env": ["MINIMAX_API_KEY"]}``). + # Preflight prefers an entry's ``required_env`` over the top-level + # ``required_env`` when the picked ``model`` matches an entry's ``id`` + # (case-insensitive). The top-level list remains the fallback so single- + # model templates need not migrate. Surfaced 2026-05-02 after a user + # picked MiniMax in canvas, set MINIMAX_API_KEY, and still got booted + # into a CLAUDE_CODE_OAUTH_TOKEN preflight failure. + models: list[dict] = field(default_factory=list) # Deprecated — use required_env + secrets API instead. Kept for backward compat. auth_token_env: str = "" auth_token_file: str = "" @@ -426,25 +436,36 @@ def load_config(config_path: Optional[str] = None) -> WorkspaceConfig: args=runtime_raw.get("args", []), required_env=runtime_raw.get("required_env", []), timeout=runtime_raw.get("timeout", 0), - # Fall back to top-level resolved `model` (which already honors - # MODEL_PROVIDER env override, line 277) when YAML doesn't carry - # runtime_config.model. Without this fallback, SaaS workspaces - # silently boot with the adapter's hard-coded default — - # claude-code-default reads `runtime_config.model or "sonnet"`, - # so a user who picks Opus in the canvas Config tab gets Sonnet - # on the next CP-driven restart. Root cause: the CP user-data - # script regenerates /configs/config.yaml at every boot with - # only `name`, `runtime`, `a2a` keys (intentionally minimal so - # it doesn't carry stale state), losing runtime_config.model. - # MODEL_PROVIDER is plumbed as an env var, so picking it up via - # the top-level resolved model keeps the selection sticky. - model=runtime_raw.get("model") or model, + # Picked-model precedence (priority order): + # 1. MODEL_PROVIDER env var — canvas-picked model, plumbed via + # workspace-server's secret-mint path or the universal + # MODEL/MODEL_PROVIDER env from applyRuntimeModelEnv. The + # operator's canvas selection MUST win over the template's + # baked-in default; previously the template's + # `runtime_config.model: sonnet` always won and the picked + # MiniMax/GLM/etc model was silently dropped (Bug B, + # surfaced 2026-05-02 during E2E). + # 2. runtime_raw.model — explicit YAML override in the + # template's runtime_config. + # 3. top-level `model` — already honors MODEL_PROVIDER (line + # 359) but only when YAML lacks a top-level `model:`. This + # is the SaaS restart case (CP regenerates a minimal + # config.yaml on every boot, dropping runtime_config.model). + # Centralising here means EVERY adapter gets the override for + # free — no per-adapter env-reading code required. + model=os.environ.get("MODEL_PROVIDER") or runtime_raw.get("model") or model, # Same fallback shape as ``model`` above: an explicit # ``runtime_config.provider`` wins; otherwise inherit the # top-level resolved provider so adapters see a single # consistent choice without each one re-implementing # env/YAML/slug-prefix resolution. provider=runtime_raw.get("provider") or provider, + # Per-model entries (canvas Model dropdown source). Pass through + # raw dicts so the schema can grow without a parser change. Only + # entries that are dicts are kept — a malformed YAML element + # (string, list, None) is silently dropped rather than raising, + # matching the rest of this parser's lenient defaults. + models=[m for m in (runtime_raw.get("models") or []) if isinstance(m, dict)], # Deprecated fields — kept for backward compat auth_token_env=runtime_raw.get("auth_token_env", ""), auth_token_file=runtime_raw.get("auth_token_file", ""), diff --git a/workspace/preflight.py b/workspace/preflight.py index 1e6aaad2..d6a5f0a3 100644 --- a/workspace/preflight.py +++ b/workspace/preflight.py @@ -140,6 +140,44 @@ def run_preflight(config: WorkspaceConfig, config_path: str) -> PreflightReport: # Check required environment variables (e.g. CLAUDE_CODE_OAUTH_TOKEN, OPENAI_API_KEY). # These are declared per-runtime in config.yaml and injected via the secrets API. required_env = getattr(config.runtime_config, "required_env", []) or [] + + # Per-model override path. When the template's runtime_config declares + # `models[]` (canvas Model dropdown), prefer the picked model's own + # `required_env` over the top-level fallback. The picked model is + # `runtime_config.model` (which already honors the MODEL_PROVIDER env + # override at parse time — see config.py:RuntimeConfig.model resolution). + # Match on `entry["id"]` case-insensitively because canvas-side ids + # ("MiniMax-M2.7") and adapter-side normalization ("minimax-m2.7") drift + # by case across registries. + # + # Bug surfaced 2026-05-02: claude-code-default top-level required_env + # demands CLAUDE_CODE_OAUTH_TOKEN, but the user picked MiniMax and only + # set MINIMAX_API_KEY. Without this lookup, preflight failed and the + # workspace crash-looped despite the user having satisfied the picked + # model's actual auth requirement. + models = getattr(config.runtime_config, "models", None) or [] + picked_model = (getattr(config.runtime_config, "model", "") or "").strip() + if models and picked_model: + picked_lower = picked_model.lower() + for entry in models: + if not isinstance(entry, dict): + continue + entry_id = str(entry.get("id", "")).strip() + if not entry_id: + continue + if entry_id.lower() != picked_lower: + continue + per_model_env = entry.get("required_env") + if per_model_env: + # Per-model required_env wins outright — do NOT union with the + # top-level list. Templates use per-model entries precisely + # to express that different models have *different* auth + # paths (OAuth token vs API key vs third-party provider key); + # unioning would re-introduce the very crash-loop this fix + # closes. + required_env = list(per_model_env) + break + for env_var in required_env: if not os.environ.get(env_var): report.failures.append( diff --git a/workspace/tests/test_preflight.py b/workspace/tests/test_preflight.py index 3bb4a793..d56e02db 100644 --- a/workspace/tests/test_preflight.py +++ b/workspace/tests/test_preflight.py @@ -286,6 +286,140 @@ def test_required_env_empty_list_passes(tmp_path): assert report.ok is True +# ---------- Per-model required_env (models[] override) ---------- + + +def test_per_model_required_env_wins_over_top_level(tmp_path, monkeypatch): + """When `runtime_config.models[]` declares per-model `required_env` and + the picked `model` matches an entry id, the entry's required_env wins + over the top-level fallback. The 2026-05-02 MiniMax-on-claude-code bug: + user picks MiniMax + sets MINIMAX_API_KEY, top-level demands + CLAUDE_CODE_OAUTH_TOKEN — without this override path the workspace + crash-loops on a stale top-level requirement.""" + monkeypatch.setenv("MINIMAX_API_KEY", "mx-test") + monkeypatch.delenv("CLAUDE_CODE_OAUTH_TOKEN", raising=False) + + config = make_config( + runtime="claude-code", + runtime_config=RuntimeConfig( + model="MiniMax-M2.7", + required_env=["CLAUDE_CODE_OAUTH_TOKEN"], # top-level fallback + models=[ + {"id": "sonnet", "required_env": ["CLAUDE_CODE_OAUTH_TOKEN"]}, + {"id": "MiniMax-M2.7", "required_env": ["MINIMAX_API_KEY"]}, + ], + ), + ) + + report = run_preflight(config, str(tmp_path)) + + assert report.ok is True + assert not any(issue.title == "Required env" for issue in report.failures) + + +def test_top_level_required_env_used_when_no_models_declared(tmp_path, monkeypatch): + """No `models[]` field → preserve the existing top-level behavior. This + is the single-model template path — claude-code-default before it grew + a Model dropdown, codex-default today, etc.""" + monkeypatch.delenv("CLAUDE_CODE_OAUTH_TOKEN", raising=False) + + config = make_config( + runtime="claude-code", + runtime_config=RuntimeConfig( + model="sonnet", + required_env=["CLAUDE_CODE_OAUTH_TOKEN"], + models=[], + ), + ) + + report = run_preflight(config, str(tmp_path)) + + assert report.ok is False + assert any( + issue.title == "Required env" and "CLAUDE_CODE_OAUTH_TOKEN" in issue.detail + for issue in report.failures + ) + + +def test_top_level_used_when_picked_model_not_in_models_list(tmp_path, monkeypatch): + """`models[]` declared but the picked `model` isn't listed → fall back + to the top-level required_env. Defensive: protects against typos / + template drift / a CP override that names a model the template doesn't + enumerate. Never silently accept zero-auth in that case.""" + monkeypatch.delenv("CLAUDE_CODE_OAUTH_TOKEN", raising=False) + + config = make_config( + runtime="claude-code", + runtime_config=RuntimeConfig( + model="some-unknown-model", + required_env=["CLAUDE_CODE_OAUTH_TOKEN"], + models=[ + {"id": "sonnet", "required_env": ["CLAUDE_CODE_OAUTH_TOKEN"]}, + {"id": "MiniMax-M2.7", "required_env": ["MINIMAX_API_KEY"]}, + ], + ), + ) + + report = run_preflight(config, str(tmp_path)) + + assert report.ok is False + assert any( + issue.title == "Required env" and "CLAUDE_CODE_OAUTH_TOKEN" in issue.detail + for issue in report.failures + ) + + +def test_per_model_match_is_case_insensitive(tmp_path, monkeypatch): + """Match `entry["id"]` against `runtime_config.model` case-insensitively + — canvas surfaces `MiniMax-M2.7`, registries normalise to lowercase + `minimax-m2.7`, MODEL_PROVIDER env may carry either. The match must + not be brittle to that drift or templates ship preflight failures + on a working auth setup.""" + monkeypatch.setenv("MINIMAX_API_KEY", "mx-test") + monkeypatch.delenv("CLAUDE_CODE_OAUTH_TOKEN", raising=False) + + config = make_config( + runtime="claude-code", + runtime_config=RuntimeConfig( + model="minimax-m2.7", # lowercase + required_env=["CLAUDE_CODE_OAUTH_TOKEN"], + models=[ + {"id": "MiniMax-M2.7", "required_env": ["MINIMAX_API_KEY"]}, # mixed case + ], + ), + ) + + report = run_preflight(config, str(tmp_path)) + + assert report.ok is True + assert not any(issue.title == "Required env" for issue in report.failures) + + +def test_per_model_match_with_no_required_env_falls_back_to_top_level(tmp_path, monkeypatch): + """An entry that matches the picked model but has no `required_env` + (or an empty one) falls back to the top-level list. This protects + against partially-specified template entries — many templates list + a `name`/`description` per model without enumerating env vars when + the auth is identical across the family.""" + monkeypatch.setenv("CLAUDE_CODE_OAUTH_TOKEN", "sk-test") + + config = make_config( + runtime="claude-code", + runtime_config=RuntimeConfig( + model="sonnet", + required_env=["CLAUDE_CODE_OAUTH_TOKEN"], + models=[ + {"id": "sonnet", "name": "Claude Sonnet"}, # no required_env + ], + ), + ) + + report = run_preflight(config, str(tmp_path)) + + assert report.ok is True + assert not any(issue.title == "Required env" for issue in report.failures) + + # ---------- Legacy auth_token_file backward compat ----------