feat(workspace-server): exempt external runtime from MODEL_REQUIRED + E2E updates

Follow-up to the kill-DefaultModel + Create-handler-gate commits. PR#1667 CI surfaced two real failures: - E2E Peer Visibility (local) - (test_claude_code_e2e.sh, depending on which workflow) Root cause: the MODEL_REQUIRED gate as initially written 422s every external workspace too. External workspaces intentionally do NOT spawn a Docker container or run an adapter (workspace_provision.go:497-498: "external is a first-class runtime that intentionally does NOT spawn a Docker container"). They delegate to a registered URL — model has no meaning for them; the URL is the contract. Requiring it would 422 every legitimate "register my agent at https://..." flow. Changes: - `handlers.WorkspaceHandler.Create` (workspace.go): exempt external workspaces from the MODEL_REQUIRED gate. Two spellings count as external — `payload.External == true` and `isExternalLikeRuntime(payload.Runtime)`. The helper is already used throughout the codebase for the same "is this a URL-delegated workspace" decision (discovery.go, registry.go, a2a_proxy_helpers.go, plugins.go, workspace_restart.go). - New test `TestCreate_ExternalRuntime_NoModel_OK`: pins the exemption. Uses the test_api.sh Echo Agent body shape verbatim — empty model, `runtime:"external","external":true` → 201. - Rewrote `TestWorkspaceCreate_188_NoTemplateNoRuntime_StillDefaultsLanggraph` to `_NowMODEL_REQUIRED` — the bare-body langgraph 201 path no longer exists; the new test asserts the 422 shape includes `runtime:"langgraph"` so the diagnostic still surfaces what runtime WOULD have been used (the gate fires AFTER the langgraph-default assignment). Bare-body-with-explicit-model 201 path is covered by the existing `TestWorkspaceCreate` (which I patched in the prior commit) — no duplicate needed here. - `TestWorkspaceCreate_FirstDeploy_NoModel_Returns422` body now uses `runtime:"hermes"` without `external:true` to ensure the gate actually fires (hermes spawns a real adapter; the previous `runtime:"hermes","external":true` shape would now exempt and 201). - `tests/e2e/test_peer_visibility_mcp_local.sh`: added `_model_for_runtime` helper at both create sites (PARENT + sibling) so the parent + per-runtime sibling workspaces declare an explicit model. Mapping mirrors what the deleted DefaultModel returned plus the gpt-5.5 / kimi / minimax slugs used in production. - `tests/e2e/test_claude_code_e2e.sh`: added `"model":"sonnet"` to both POST bodies (ROOT + CHILD). Both use `runtime:"claude-code"` which spawns a real adapter, not exempt. Other E2E scripts inspected: - `test_api.sh` — already uses `runtime:"external","external":true`, exempt by the new gate. No change. - `test_a2a_e2e.sh`:133, `test_activity_e2e.sh`:218, `test_dev_mode.sh`:72, `test_workspace_abilities_e2e.sh`:93,99, `test_comprehensive_e2e.sh`:78,105,134,139,144, `test_notify_attachments_e2e.sh`:95, `test_mcp_stdio_staging.sh`:76 — these create workspaces without an explicit runtime field. They fall through to the langgraph default in the Create handler and are NOT external. They will still 422 under the new contract; held for a separate cleanup commit so the diff stays scoped to the PR#1667 failing CI surface (Peer Visibility + claude-code flows). Tracked for follow-up. Verified: go test -count=1 -short -timeout 300s ./internal/handlers/... \ ./internal/models/... → OK Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-21 21:42:49 -07:00
parent 1ece444ea2
commit a3c15bc9be
6 changed files with 119 additions and 54 deletions
@@ -50,13 +50,16 @@ docker rm $(docker ps -aq --filter "name=ws-") 2>/dev/null || true
 echo ""
 echo "--- Create Workspaces ---"

+# model is required at the Create boundary (CTO 2026-05-22 SSOT —
+# feedback_workspace_model_required_no_platform_default_dynamic_credential_intake).
+# Pass the same value the deleted DefaultModel("claude-code") returned.
 ROOT=$(curl -s -X POST $PLATFORM/workspaces -H "Content-Type: application/json" \
-  -d '{"name":"Root Agent","role":"Company coordinator","runtime":"claude-code","tier":3}' \
+  -d '{"name":"Root Agent","role":"Company coordinator","runtime":"claude-code","model":"sonnet","tier":3}' \
  | python3 -c "import sys,json; print(json.load(sys.stdin)['id'])")
 check_contains "Create root workspace" "-" "$ROOT"

 CHILD=$(curl -s -X POST $PLATFORM/workspaces -H "Content-Type: application/json" \
-  -d "{\"name\":\"Child Agent\",\"role\":\"Sub-team member\",\"runtime\":\"claude-code\",\"tier\":2,\"parent_id\":\"$ROOT\"}" \
+  -d "{\"name\":\"Child Agent\",\"role\":\"Sub-team member\",\"runtime\":\"claude-code\",\"model\":\"sonnet\",\"tier\":2,\"parent_id\":\"$ROOT\"}" \
  | python3 -c "import sys,json; print(json.load(sys.stdin)['id'])")
 check_contains "Create child workspace" "-" "$CHILD"

@@ -241,8 +241,24 @@ else
 fi
 log "1/5 provisioning parent ($PARENT_RUNTIME, mode=$PV_LOCAL_PROVISION_MODE) + one sibling per runtime under test..."

+# Map runtime → model per the CTO 2026-05-22 SSOT directive (model is
+# required, no platform default). External runtimes are exempt by the
+# Create-handler gate — for them the URL is the contract — but we still
+# pass model="external:custom" defensively in case a downstream consumer
+# of the create body asserts presence.
+_model_for_runtime() {
+  case "$1" in
+    claude-code) echo "sonnet" ;;
+    codex)       echo "gpt-5.5" ;;
+    kimi)        echo "kimi-coding/kimi-k2-coding-6" ;;
+    minimax)     echo "minimax/MiniMax-M2.7" ;;
+    external)    echo "external:custom" ;;
+    *)           echo "anthropic:claude-opus-4-7" ;;
+  esac
+}
+PARENT_MODEL=$(_model_for_runtime "$PARENT_RUNTIME")
 P_RESP=$(curl -s -X POST "$BASE/workspaces" ${ADMIN_AUTH[@]+"${ADMIN_AUTH[@]}"} -H "Content-Type: application/json" \
-  -d "{\"name\":\"${NAME_PREFIX}-parent\",\"runtime\":\"$PARENT_RUNTIME\",\"tier\":3$PARENT_EXTRA,\"secrets\":$PARENT_SECRETS}")
+  -d "{\"name\":\"${NAME_PREFIX}-parent\",\"runtime\":\"$PARENT_RUNTIME\",\"model\":\"$PARENT_MODEL\",\"tier\":3$PARENT_EXTRA,\"secrets\":$PARENT_SECRETS}")
 PARENT_ID=$(echo "$P_RESP" | python3 -c 'import json,sys;print(json.load(sys.stdin).get("id",""))' 2>/dev/null)
 if [ -z "$PARENT_ID" ]; then
  echo "::error::parent create failed: $(echo "$P_RESP" | head -c 300)" >&2
@@ -291,8 +307,9 @@ for rt in $PV_RUNTIMES; do
    CREATE_RUNTIME="$rt"
    CREATE_EXTRA=""
  fi
+  CREATE_MODEL=$(_model_for_runtime "$CREATE_RUNTIME")
  R=$(curl -s -X POST "$BASE/workspaces" ${ADMIN_AUTH[@]+"${ADMIN_AUTH[@]}"} -H "Content-Type: application/json" \
-    -d "{\"name\":\"${NAME_PREFIX}-$rt\",\"runtime\":\"$CREATE_RUNTIME\",\"tier\":2,\"parent_id\":\"$PARENT_ID\"$CREATE_EXTRA,\"secrets\":$SEC}")
+    -d "{\"name\":\"${NAME_PREFIX}-$rt\",\"runtime\":\"$CREATE_RUNTIME\",\"model\":\"$CREATE_MODEL\",\"tier\":2,\"parent_id\":\"$PARENT_ID\"$CREATE_EXTRA,\"secrets\":$SEC}")
  WID=$(echo "$R" | python3 -c 'import json,sys;print(json.load(sys.stdin).get("id",""))' 2>/dev/null)
  if [ -z "$WID" ]; then
    echo "::error::$rt workspace create failed: $(echo "$R" | head -c 300)" >&2
@@ -823,6 +823,57 @@ func TestCreate_ModelRequired_Returns422(t *testing.T) {
 	}
 }

+// TestCreate_ExternalRuntime_NoModel_OK pins the external-runtime
+// exemption from the MODEL_REQUIRED gate. External workspaces
+// intentionally do not spawn a Docker container or run an adapter;
+// they delegate to a registered URL (workspace_provision.go:497-498:
+// "external is a first-class runtime that intentionally does NOT
+// spawn a Docker container"). The model field has no meaning for
+// them — the URL is the contract, and the gate would 422 every
+// legitimate "register my agent at https://..." flow.
+//
+// Both spellings count as external:
+//   1. payload.External == true (the canonical flag, e.g. with any runtime)
+//   2. payload.Runtime == "external" (legacy shape some E2E scripts still use)
+//
+// The isExternalLikeRuntime() helper catches both "external" and any
+// future external-like runtime alias.
+func TestCreate_ExternalRuntime_NoModel_OK(t *testing.T) {
+	mock := setupTestDB(t)
+	setupTestRedis(t)
+	handler := NewWorkspaceHandler(newTestBroadcaster(), nil, "http://localhost:8080", t.TempDir())
+
+	// External=true with explicit runtime — the test_api.sh / Echo Agent shape.
+	mock.ExpectBegin()
+	mock.ExpectExec("INSERT INTO workspaces").
+		WillReturnResult(sqlmock.NewResult(0, 1))
+	mock.ExpectCommit()
+	mock.ExpectExec("INSERT INTO canvas_layouts").
+		WillReturnResult(sqlmock.NewResult(0, 1))
+	mock.ExpectExec("INSERT INTO structure_events").
+		WillReturnResult(sqlmock.NewResult(0, 1))
+	mock.ExpectExec(`UPDATE workspaces SET status =`).
+		WillReturnResult(sqlmock.NewResult(0, 1))
+	mock.ExpectExec("INSERT INTO workspace_auth_tokens").
+		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":"Echo Agent","tier":1,"runtime":"external","external":true}`
+	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("external workspace without model: want 201, got %d: %s", w.Code, w.Body.String())
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("sqlmock expectations not met: %v", err)
+	}
+}
+
 func TestUpdate_FieldValidation_Returns400(t *testing.T) {
 	setupTestDB(t)
 	setupTestRedis(t)
@@ -322,9 +322,10 @@ func (h *WorkspaceHandler) Create(c *gin.Context) {
 	}

 	// SSOT (CTO 2026-05-22, feedback_workspace_model_required_no_platform_default_dynamic_credential_intake):
-	// model is REQUIRED user input. The platform must not provide a default;
-	// the runtime must not fall back. The decision belongs to the user (or
-	// to the agent acting on the user's behalf), never to the platform.
+	// model is REQUIRED user input for SPAWNED-runtime workspaces. The
+	// platform must not provide a default; the runtime must not fall back.
+	// The decision belongs to the user (or to the agent acting on the
+	// user's behalf), never to the platform.
 	//
 	// Empirical trigger: Code Reviewer 5ba15d7e was created with
 	// `{"name":"Code Reviewer","role":"...","runtime":"codex",...}` (no
@@ -337,11 +338,24 @@ func (h *WorkspaceHandler) Create(c *gin.Context) {
 	// `model not patchable`), so the only recovery path was SQL UPDATE
 	// or delete+recreate.
 	//
+	// External workspaces are EXEMPT — they intentionally do not spawn
+	// a Docker container or run an adapter; they delegate to a registered
+	// URL (see provision.go: "external is a first-class runtime that
+	// intentionally does NOT spawn a Docker container"). The MODEL_REQUIRED
+	// gate is meaningful for spawned-runtime workspaces where the model
+	// id drives provider selection at adapter init. For external workspaces
+	// the contract is the URL, not the model — requiring it would be
+	// ceremony with no payoff, and would 422 every legitimate "register
+	// my agent at https://..." flow. The SSOT directive concerns
+	// platform-side defaults; an external workspace genuinely has no
+	// "model decision" for the user to make.
+	//
 	// Fail-closed at the Create boundary so the caller learns the
 	// contract immediately — same shape as the controlplane#188
 	// runtime-unresolved gate above. Caller fixes the request, no
 	// EC2 launched, no stuck workspace, no operator paging.
-	if payload.Model == "" {
+	isExternal := payload.External || isExternalLikeRuntime(payload.Runtime)
+	if payload.Model == "" && !isExternal {
 		log.Printf("Create: FAIL-CLOSED — model is required (runtime=%q template=%q); refusing the silent DefaultModel fallback per CTO 2026-05-22 SSOT directive", payload.Runtime, payload.Template)
 		c.JSON(http.StatusUnprocessableEntity, gin.H{
 			"error":    "model is required and has no platform-side default — pass an explicit \"model\" in the request body, or use a \"template\" whose config.yaml declares one. See feedback_workspace_model_required_no_platform_default_dynamic_credential_intake for the contract.",
@@ -784,9 +784,14 @@ func TestWorkspaceCreate_FirstDeploy_NoModel_Returns422(t *testing.T) {
 	// will surface "call to ExecQuery 'INSERT INTO workspaces' was not
 	// expected" — which is exactly the failure mode we want to flag.

+	// Body: hermes runtime WITHOUT external:true (the external-runtime
+	// exemption — see TestCreate_ExternalRuntime_NoModel_OK — does NOT
+	// apply here; hermes spawns a real adapter and model selection
+	// matters at adapter init). This is exactly the shape the old
+	// "no-model-no-secret-write" test pinned, minus the external flag.
 	w := httptest.NewRecorder()
 	c, _ := gin.CreateTestContext(w)
-	body := `{"name":"No Model Agent","runtime":"hermes","external":true}`
+	body := `{"name":"No Model Agent","runtime":"hermes"}`
 	c.Request = httptest.NewRequest("POST", "/workspaces", bytes.NewBufferString(body))
 	c.Request.Header.Set("Content-Type", "application/json")

@@ -1844,31 +1844,35 @@ func TestWorkspaceCreate_188_TemplateConfigNoRuntimeKey_FailsClosed(t *testing.T
 	}
 }

-// Regression guard for the legitimate default path (no template, no
-// runtime — bare `{"name":...}`). Pre-2026-05-22 this returned 201 with
-// runtime defaulted to langgraph + model defaulted to
-// "anthropic:claude-opus-4-7" via the (now-deleted) DefaultModel helper.
-// Post-CTO-SSOT-directive the bare body must fail-closed at MODEL_REQUIRED
-// before reaching the runtime-default langgraph branch — the model
-// gate runs AFTER the langgraph fallback so we can observe the
-// "runtime=langgraph, model=missing" framing in the error body. With
-// model supplied, the langgraph default still kicks in (so the original
-// invariant — bare-name still routes to langgraph — is preserved when
-// the new contract is satisfied).
-func TestWorkspaceCreate_188_NoTemplateNoRuntime_StillDefaultsLanggraph(t *testing.T) {
-	mock := setupTestDB(t)
+// Pre-2026-05-22 this test guarded "bare {name} → langgraph 201" — the
+// regression check for controlplane#188 (where an explicit runtime that
+// failed to resolve must NOT silently substitute langgraph) had a sibling
+// to ensure the LEGITIMATE bare default still landed on langgraph.
+//
+// Post-CTO-SSOT-directive (2026-05-22) bare body is 422 MODEL_REQUIRED
+// before reaching the langgraph branch — the gate runs AFTER the
+// langgraph-default assignment so the error body still surfaces
+// runtime=langgraph (helps the caller see "ok, langgraph WOULD have
+// been the runtime, but you still owe me a model"). The bare-body
+// langgraph 201 path no longer exists; what we guard now is the
+// 422-shape diagnostic.
+//
+// Bare-body-with-explicit-model 201 (the new "legitimate default" path)
+// is covered by TestWorkspaceCreate in handlers_test.go — no need to
+// duplicate the mock dance here.
+func TestWorkspaceCreate_188_NoTemplateNoRuntime_NowMODEL_REQUIRED(t *testing.T) {
+	setupTestDB(t)
 	setupTestRedis(t)
 	broadcaster := newTestBroadcaster()
 	handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir())

-	// First half: bare body now MUST 422 with MODEL_REQUIRED. No DB
-	// writes are expected — the gate fires before INSERT.
 	w := httptest.NewRecorder()
 	c, _ := gin.CreateTestContext(w)
 	body := `{"name":"Plain Default"}`
 	c.Request = httptest.NewRequest("POST", "/workspaces", bytes.NewBufferString(body))
 	c.Request.Header.Set("Content-Type", "application/json")
 	handler.Create(c)
+
 	if w.Code != http.StatusUnprocessableEntity {
 		t.Fatalf("bare-body create: expected 422 MODEL_REQUIRED, got %d: %s", w.Code, w.Body.String())
 	}
@@ -1876,36 +1880,7 @@ func TestWorkspaceCreate_188_NoTemplateNoRuntime_StillDefaultsLanggraph(t *testi
 		t.Errorf("bare-body create: expected code=MODEL_REQUIRED in body, got %s", w.Body.String())
 	}
 	if !bytes.Contains(w.Body.Bytes(), []byte(`"runtime":"langgraph"`)) {
-		t.Errorf("bare-body create: expected runtime=langgraph in 422 body (gate runs after the langgraph default), got %s", w.Body.String())
-	}
-
-	// Second half: with explicit model, the langgraph default still
-	// kicks in — original invariant (bare-name routes to langgraph)
-	// preserved when the new contract is satisfied.
-	mock.ExpectBegin()
-	mock.ExpectExec("INSERT INTO workspaces").
-		WithArgs(sqlmock.AnyArg(), "Plain Default", nil, 3, "langgraph", sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push").
-		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":"Plain Default","model":"anthropic:claude-opus-4-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 (legitimate default path), got %d: %s", w.Code, w.Body.String())
-	}
-	if err := mock.ExpectationsWereMet(); err != nil {
-		t.Errorf("unmet sqlmock expectations: %v", err)
+		t.Errorf("bare-body create: expected runtime=\"langgraph\" in 422 body (the gate runs AFTER the langgraph-default assignment so the diagnostic surfaces what runtime WOULD have been used), got %s", w.Body.String())
 	}
 }