diff --git a/workspace-server/internal/handlers/workspace_crud.go b/workspace-server/internal/handlers/workspace_crud.go index b22886a2..030519dd 100644 --- a/workspace-server/internal/handlers/workspace_crud.go +++ b/workspace-server/internal/handlers/workspace_crud.go @@ -248,6 +248,36 @@ func (h *WorkspaceHandler) Update(c *gin.Context) { } needsRestart := false if runtime, ok := body["runtime"]; ok { + // (runtime, model) compatibility validation (the new field a PATCH can + // change). model is NOT patchable per the body whitelist above, so the + // post-PATCH model is the workspace's CURRENT model — fetched here + // rather than re-parsed from the body. The (newRuntime, currentModel) + // pair is what the boot path will try to resolve; an unroutable pair + // is rejected at the API boundary instead of wedging the agent at + // boot. validateRegisteredModelForRuntime is the same SSOT the + // create-boundary uses (workspace_crud.go create + llm_billing_mode + // resolver); mirroring it here keeps the PATCH-runtime path consistent + // and catches the drift surface CR2 found on the #21 review. + var currentModel sql.NullString + if err := db.DB.QueryRowContext(ctx, + `SELECT COALESCE(model, '') FROM workspaces WHERE id = $1`, id, + ).Scan(¤tModel); err != nil { + log.Printf("Update runtime validation: read current model for %s: %v", id, err) + c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to read current model for runtime compatibility check"}) + return + } + if ok, reason := validateRegisteredModelForRuntime(runtime.(string), currentModel.String); !ok { + log.Printf("Update: PATCH runtime=%q on %s REJECTED (model=%q is not registered for that runtime): %s", + runtime.(string), id, currentModel.String, reason) + // 422 (Unprocessable Entity) matches the create-boundary's + // validateRegisteredModelForRuntime path (secrets.go:942, 952 + // + workspace_crud.go create) — both reviewers flagged the + // original 400 as inconsistent. 422 = "syntactically valid + // PATCH body, but the (runtime, model) pair is unroutable + // per the registry SSOT", which is the precise semantic. + c.JSON(http.StatusUnprocessableEntity, gin.H{"error": reason}) + return + } if _, err := db.DB.ExecContext(ctx, `UPDATE workspaces SET runtime = $2, updated_at = now() WHERE id = $1`, id, runtime); err != nil { log.Printf("Update runtime error for %s: %v", id, err) c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to save runtime"}) diff --git a/workspace-server/internal/handlers/workspace_crud_test.go b/workspace-server/internal/handlers/workspace_crud_test.go index 7988ac64..af7dd63c 100644 --- a/workspace-server/internal/handlers/workspace_crud_test.go +++ b/workspace-server/internal/handlers/workspace_crud_test.go @@ -584,3 +584,95 @@ func TestCascadeDelete_DescendantRowsError(t *testing.T) { // Note: Full CascadeDelete testing requires mocking StopWorkspace, RemoveVolume, // and provisioner calls — covered in integration tests. Unit tests here focus on // the validation and pre-condition paths. + +// TestUpdate_Runtime_RegisteredModelForRuntime_Passes pins the (runtime, +// model) compatibility check happy path: the workspace's current model IS +// registered for the new runtime, so the validation passes and the +// PATCH-runtime proceeds. Mirrors the create-boundary's use of +// validateRegisteredModelForRuntime (the same SSOT). +func TestUpdate_Runtime_RegisteredModelForRuntime_Passes(t *testing.T) { + wsID := "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" + mock, r := setupWorkspaceCrudTest(t) + h := newWorkspaceCrudHandler(t) + r.PATCH("/workspaces/:id", h.Update) + + mock.ExpectQuery(`SELECT EXISTS\(SELECT 1 FROM workspaces WHERE id = \$1\)`). + WithArgs(wsID). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1)) + // New (newRuntime, currentModel) check — model is read so we can + // pair it with the new runtime. + mock.ExpectQuery(`SELECT COALESCE\(model, ''\) FROM workspaces WHERE id = \$1`). + WithArgs(wsID). + WillReturnRows(sqlmock.NewRows([]string{"model"}).AddRow("moonshot/kimi-k2.6")) + // The validation passes (moonshot/kimi-k2.6 is a registered model + // for claude-code in the harness's provider registry), so the + // UPDATE proceeds. + mock.ExpectExec(`UPDATE workspaces\s+SET runtime = \$2`). + WithArgs(wsID, "claude-code"). + WillReturnResult(sqlmock.NewResult(0, 1)) + + body := map[string]interface{}{"runtime": "claude-code"} + b, _ := json.Marshal(body) + req, _ := http.NewRequest("PATCH", "/workspaces/"+wsID, bytes.NewReader(b)) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet: %v", err) + } +} + +// TestUpdate_Runtime_UnroutableModel_Fails422 pins the (runtime, model) +// compatibility check REJECT path: the new runtime + current model pair +// is unroutable (the model isn't registered for that runtime AND no +// provider prefix-matches a native arm). Rejected with 422 + the +// registry-SSOT reason. The PATCH does NOT update the DB (the UPDATE +// exec is NOT mocked, so unmet-expectations would fire if the UPDATE +// happened — but we only check the 422 response code here). +func TestUpdate_Runtime_UnroutableModel_Fails422(t *testing.T) { + wsID := "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" + mock, r := setupWorkspaceCrudTest(t) + h := newWorkspaceCrudHandler(t) + r.PATCH("/workspaces/:id", h.Update) + + mock.ExpectQuery(`SELECT EXISTS\(SELECT 1 FROM workspaces WHERE id = \$1\)`). + WithArgs(wsID). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1)) + // A model that is NOT registered for any runtime (so + // validateRegisteredModelForRuntime returns false via both the + // exact-membership loop AND the DeriveProvider allow path). + // "unroutable/unknown" has no model-prefix matches in any runtime's + // native provider set, and doesn't appear on any runtime's + // ModelsForRuntime list — both checks fail. + mock.ExpectQuery(`SELECT COALESCE\(model, ''\) FROM workspaces WHERE id = \$1`). + WithArgs(wsID). + WillReturnRows(sqlmock.NewRows([]string{"model"}).AddRow("unroutable/unknown")) + + body := map[string]interface{}{"runtime": "claude-code"} + b, _ := json.Marshal(body) + req, _ := http.NewRequest("PATCH", "/workspaces/"+wsID, bytes.NewReader(b)) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + + // 422 (Unprocessable Entity) matches the create-boundary's + // validateRegisteredModelForRuntime path (secrets.go:942, 952 + + // workspace_crud.go create) — 422-align per CR2 + Researcher's + // review on the 400→422 consistency ask. Precise semantic: the + // PATCH body is syntactically valid, but the (runtime, model) pair + // is unroutable per the registry SSOT. + if w.Code != http.StatusUnprocessableEntity { + t.Errorf("expected 422 (unroutable (runtime, model)), got %d: %s", w.Code, w.Body.String()) + } + // The UPDATE should NOT have fired. ExpectationsWereMet is + // informational (the unmet-expectations log would surface in + // verbose test output); the code status is the load-bearing + // assertion. We DON'T add a strict unmet check here because + // sqlmock's ExpectationsWereMet fires on the failure path too + // (mock has unconsumed expectations). + _ = mock +} diff --git a/workspace-server/internal/handlers/workspace_test.go b/workspace-server/internal/handlers/workspace_test.go index 5022c56b..53e89b00 100644 --- a/workspace-server/internal/handlers/workspace_test.go +++ b/workspace-server/internal/handlers/workspace_test.go @@ -958,6 +958,14 @@ func TestWorkspaceUpdate_RuntimeField(t *testing.T) { mock.ExpectQuery("SELECT EXISTS.*workspaces WHERE id"). WithArgs("cccccccc-0006-0000-0000-000000000000"). WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(true)) + // New (newRuntime, currentModel) compatibility check (#2926): + // the PATCH-runtime path now reads the current model + validates + // (runtime, model) against the provider registry. moonshot/kimi-k2.6 + // is registered for claude-code in the harness's provider registry + // (providers.yaml:919) so the validation passes + the UPDATE proceeds. + mock.ExpectQuery(`SELECT COALESCE\(model, ''\) FROM workspaces WHERE id = \$1`). + WithArgs("cccccccc-0006-0000-0000-000000000000"). + WillReturnRows(sqlmock.NewRows([]string{"model"}).AddRow("moonshot/kimi-k2.6")) mock.ExpectExec("UPDATE workspaces SET runtime"). WithArgs("cccccccc-0006-0000-0000-000000000000", "claude-code"). WillReturnResult(sqlmock.NewResult(0, 1)) @@ -997,6 +1005,14 @@ func TestWorkspaceUpdate_RuntimeField_DBErrorReturnsServerError(t *testing.T) { mock.ExpectQuery("SELECT EXISTS.*workspaces WHERE id"). WithArgs("cccccccc-0006-0000-0000-000000000001"). WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(true)) + // New (newRuntime, currentModel) compatibility check (#2926): + // the SELECT COALESCE must succeed (the UPDATE is the one that + // errors out). moonshot/kimi-k2.6 is registered for hermes + // (providers.yaml:919) so the validation passes + the UPDATE is + // attempted, where the sqlmock is set to return the DB error. + mock.ExpectQuery(`SELECT COALESCE\(model, ''\) FROM workspaces WHERE id = \$1`). + WithArgs("cccccccc-0006-0000-0000-000000000001"). + WillReturnRows(sqlmock.NewRows([]string{"model"}).AddRow("moonshot/kimi-k2.6")) mock.ExpectExec("UPDATE workspaces SET runtime"). WithArgs("cccccccc-0006-0000-0000-000000000001", "hermes"). WillReturnError(errors.New("database unavailable"))