feat(launcher): drop dead frontmatter model + validate model name (never-break)
G1: remove the dead `model:` line from all 6 .openclaw/agents/*.md prompts — launcher never read it; config (agent_model_*) is the single source of truth. G2: add is_valid_model helper (format check ^claude-…$) applied inside resolve_agent_model's resolution cascade and at the inline --fallback-model read in _spawn. An invalid name is logged and skipped to the next valid level (in the limit: no --model flag), never passed to the CLI, never raises. Format check chosen over an allowlist for forward-compatibility (ADR-001). G3 (routing) and G4 (fallback) intentionally NOT enabled — all agents stay on claude-opus-4-8; agent_fallback_model stays "". Docs (golden source) updated in the same change: README model/effort table + validation, CLAUDE.md, .env.example (ORCH_AGENT_MODEL_*/EFFORT_*/FALLBACK_MODEL), CHANGELOG. Tests: test_agent_frontmatter_no_model.py (G1), extended test_resolve_agent_model.py (G2 never-break). Refs: ORCH-074 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
@@ -2,6 +2,7 @@ import subprocess
|
||||
import os
|
||||
import json
|
||||
import logging
|
||||
import re
|
||||
import threading
|
||||
import signal
|
||||
import time
|
||||
@@ -20,6 +21,36 @@ logger = logging.getLogger("orchestrator.launcher")
|
||||
# never passed through to the CLI.
|
||||
VALID_EFFORTS = frozenset({"low", "medium", "high", "xhigh", "max"})
|
||||
|
||||
# ORCH-074 (G2): structural validity check for a Claude CLI model name. We use a
|
||||
# FORMAT check (^claude-…$), not a static allowlist, on purpose: an allowlist
|
||||
# recreates the exact rot we kill in G1 — it silently drops a CORRECT newer model
|
||||
# (e.g. claude-opus-4-9) the day Anthropic ships it (never-break working against
|
||||
# the operator). The final authority on whether a model exists is the Claude CLI
|
||||
# itself, not our code; a format check is forward-compatible (new versions pass
|
||||
# without code edits) while still catching the real failure classes: another
|
||||
# provider (gpt-4), empty/whitespace, garbage chars, wrong prefix (claud-opus-typo).
|
||||
# The claude- prefix is hardcoded here because the orchestrator is bound to the
|
||||
# Claude CLI (CLAUDE_BIN); the canonical model VERSION lives ONLY in
|
||||
# settings.agent_model_default, never here. See ADR-001 (ORCH-074).
|
||||
_MODEL_NAME_RE = re.compile(r"^claude-[a-z0-9.-]+$")
|
||||
|
||||
|
||||
def is_valid_model(name: str) -> bool:
|
||||
"""ORCH-074 (G2): True iff ``name`` is a structurally valid Claude model name.
|
||||
|
||||
A valid name, after ``strip()``, is non-empty, starts with ``claude-`` and
|
||||
contains only lowercase letters, digits, dots and dashes. Anything else
|
||||
(empty/whitespace, another provider like ``gpt-4``, a wrong prefix, illegal
|
||||
characters) is invalid. This is the single predicate used by BOTH
|
||||
``resolve_agent_model`` and the inline ``--fallback-model`` read in ``_spawn``
|
||||
so a typo can never reach the CLI (never-break). It is a structural guard, not
|
||||
a registry of existing models — a structurally valid typo (``claude-opus-typo``)
|
||||
is left for the CLI to reject. Never raises.
|
||||
"""
|
||||
if not name:
|
||||
return False
|
||||
return bool(_MODEL_NAME_RE.match(name.strip()))
|
||||
|
||||
# ORCH-061: action stages whose success is an ACTION (restart/retag), not a src
|
||||
# edit — so "no changes to commit" is EXPECTED there, not under-delivery (FR-3).
|
||||
_ACTION_STAGES = frozenset({"deploy-staging", "deploy"})
|
||||
@@ -83,18 +114,48 @@ def _resolve_agent_attr(agent, project_id, project_map_attr, env_attr_prefix,
|
||||
return ""
|
||||
|
||||
|
||||
def _agent_model_candidates(agent: str, project_id: str = None):
|
||||
"""Yield non-empty model candidates in ORCH-41 priority order.
|
||||
|
||||
Same priority as _resolve_agent_attr (project-override > per-agent env >
|
||||
global default), but as a generator so resolve_agent_model can validate each
|
||||
level and SKIP an invalid one (ORCH-074 G2) instead of returning the first
|
||||
non-empty value blindly. Empty levels are simply not yielded.
|
||||
"""
|
||||
if project_id:
|
||||
from ..projects import get_project_by_plane_id
|
||||
proj = get_project_by_plane_id(project_id)
|
||||
if proj is not None:
|
||||
override = getattr(proj, "agent_models", {}).get(agent)
|
||||
if override:
|
||||
yield override
|
||||
per_agent = getattr(settings, f"agent_model_{agent}", "")
|
||||
if per_agent:
|
||||
yield per_agent
|
||||
default = getattr(settings, "agent_model_default", "")
|
||||
if default:
|
||||
yield default
|
||||
|
||||
|
||||
def resolve_agent_model(agent: str, project_id: str = None) -> str:
|
||||
"""ORCH-41: resolve the LLM model for an agent (optionally per-project).
|
||||
|
||||
Returns "" when no model is configured at any level -> caller omits --model
|
||||
and the CLI default applies. See _resolve_agent_attr for the priority order.
|
||||
ORCH-074 (G2): the resolved name is validated with is_valid_model BEFORE it is
|
||||
returned. An invalid (structurally garbage) value at any level is logged and
|
||||
SKIPPED — resolution falls through to the next valid level (project-override
|
||||
invalid -> per-agent env -> default); if no level yields a valid name the
|
||||
function returns "" so the caller omits --model and the CLI default applies.
|
||||
The ORCH-41 priority order and signature are unchanged; validation is layered
|
||||
on top. Never raises and never returns garbage that could reach --model.
|
||||
"""
|
||||
return _resolve_agent_attr(
|
||||
agent, project_id,
|
||||
project_map_attr="agent_models",
|
||||
env_attr_prefix="agent_model_",
|
||||
default_attr="agent_model_default",
|
||||
)
|
||||
for value in _agent_model_candidates(agent, project_id):
|
||||
if is_valid_model(value):
|
||||
return value
|
||||
logger.warning(
|
||||
f"Invalid model name '{value}' for agent '{agent}' "
|
||||
f"(expected '^claude-…'); skipping to next resolution level / CLI default"
|
||||
)
|
||||
return ""
|
||||
|
||||
|
||||
def resolve_agent_effort(agent: str, project_id: str = None) -> str:
|
||||
@@ -371,7 +432,17 @@ class AgentLauncher:
|
||||
effort = resolve_agent_effort(agent, project_id)
|
||||
model_flag = f"--model {model} " if model else ""
|
||||
effort_flag = f"--effort {effort} " if effort else ""
|
||||
# ORCH-074 (G2): agent_fallback_model is read directly here, bypassing
|
||||
# resolve_agent_model, so the same validator must guard this point too —
|
||||
# otherwise a typo in ORCH_AGENT_FALLBACK_MODEL would slip into
|
||||
# --fallback-model (never-break violation). Empty value -> no flag, exactly
|
||||
# as before (is_valid_model("") is False but the `if fb` short-circuits).
|
||||
fb = settings.agent_fallback_model
|
||||
if fb and not is_valid_model(fb):
|
||||
logger.warning(
|
||||
f"Invalid fallback model '{fb}'; dropping --fallback-model"
|
||||
)
|
||||
fb = ""
|
||||
fb_flag = f"--fallback-model {fb} " if fb else ""
|
||||
|
||||
# No git fetch/checkout here: ensure_worktree() already put the worktree on
|
||||
|
||||
Reference in New Issue
Block a user