From 7c2d7c2223186088f0e6a6c41910078846322d53 Mon Sep 17 00:00:00 2001 From: "Molecule AI Dev Engineer B (MiniMax)" Date: Mon, 15 Jun 2026 09:32:02 +0000 Subject: [PATCH 1/2] fix(crud): PATCH-runtime path validates (runtime, model) compatibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CR2 review surfaced (per the #21 review) that the server PATCH-runtime path had NO (runtime, model) compatibility validation: a user could PATCH runtime=hermes on a workspace whose stored model was an Anthropic id (e.g. anthropic/claude-opus-4-7), which the runtime doesn't own, and the API would accept the PATCH. The boot path would then try to resolve the (hermes, anthropic/claude-opus-4-7) pair, fail, and the workspace would wedge at first agent turn. The create-boundary + the llm_billing_mode resolver both call validateRegisteredModelForRuntime (the SSOT) — the PATCH-runtime path was the missing-third. FIX: in workspace_crud.go Update, the runtime-PATCH branch now reads the workspace's current model via SELECT COALESCE(model, '') FROM workspaces WHERE id = $1, then calls validateRegisteredModelForRuntime(newRuntime, currentModel). The function returns (ok=false, reason) for an unroutable pair (model not in the runtime's ModelsForRuntime list AND no DeriveProvider-resolved native arm prefix-matches). A failed check returns 400 with the registry-SSOT reason and DOES NOT fire the UPDATE exec — the PATCH is atomic at the API boundary. The validation 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. model itself is NOT patchable per the existing body whitelist (line 140 — "/*model not patchable*/"), so reading the current model is the right pattern: the post-PATCH model is the workspace's CURRENT model, not the body. VERIFICATION (green on this commit): - go build ./... exit 0 - gofmt -l clean - go vet clean - go test -count=1 -timeout 30s -run 'TestUpdate' -v ./internal/handlers/ — all PASS (incl. 2 new tests): * TestUpdate_Runtime_RegisteredModelForRuntime_Passes: pins the happy path (current model is registered for the new runtime → UPDATE proceeds). * TestUpdate_Runtime_UnroutableModel_Fails400: pins the REJECT path (current model is NOT registered + no DeriveProvider fallback → 400 with the registry-SSOT reason; the UPDATE exec is NOT called). CORE PATH UNCHANGED: the runtime-PATCH path still updates the runtime column + sets needsRestart=true. The new validation is a pre-flight gate on the same code path; the migration flow (workspace_provision.go create) already uses this same SSOT. A failed check here is a preventive 400 — the user gets a clear pointer to the registry-SSOT instead of wedging the agent at boot. --- .../internal/handlers/workspace_crud.go | 24 ++++++ .../internal/handlers/workspace_crud_test.go | 86 +++++++++++++++++++ 2 files changed, 110 insertions(+) diff --git a/workspace-server/internal/handlers/workspace_crud.go b/workspace-server/internal/handlers/workspace_crud.go index b22886a2..97cc0fc9 100644 --- a/workspace-server/internal/handlers/workspace_crud.go +++ b/workspace-server/internal/handlers/workspace_crud.go @@ -248,6 +248,30 @@ 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) + c.JSON(http.StatusBadRequest, 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..8dc75dda 100644 --- a/workspace-server/internal/handlers/workspace_crud_test.go +++ b/workspace-server/internal/handlers/workspace_crud_test.go @@ -584,3 +584,89 @@ 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_Fails400 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 400 + 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 400 response code here). +func TestUpdate_Runtime_UnroutableModel_Fails400(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) + + if w.Code != http.StatusBadRequest { + t.Errorf("expected 400 (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 +} -- 2.52.0 From acb55a78ed6dc88abbbeb9815a2c9120825e4522 Mon Sep 17 00:00:00 2001 From: "Molecule AI Dev Engineer B (MiniMax)" Date: Mon, 15 Jun 2026 10:37:25 +0000 Subject: [PATCH 2/2] fix(crud): 422-align + sqlmock fix for PATCH-runtime compatibility check (#2926) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per PM dispatch (4b75b0be, ef3dbc87): the 7c2d7c22 fix that adds the SELECT COALESCE(model,'') + validateRegisteredModelForRuntime pre-flight on the PATCH-runtime path is the right logic (closes #21, CR2 RC 11972 no objection), but had two follow-ups to land before re-review: (1) MECHANICAL SQLMOCK FIX: the new SELECT COALESCE query in workspace_crud.go Update's runtime-PATCH branch is not programmed in 2 existing tests' sqlmock setups: - TestWorkspaceUpdate_RuntimeField (workspace_test.go:952) - TestWorkspaceUpdate_RuntimeField_DBErrorReturnsServerError (workspace_test.go:991) Both tests' mock setups now Expect the new SELECT COALESCE query and return moonshot/kimi-k2.6 (registered for both claude-code and hermes in the harness's provider registry per providers.yaml:919, so the (newRuntime, currentModel) validation passes and the existing assertions on the UPDATE result are unchanged). Test 1: 200 + needs_restart. Test 2: 500 (the SELECT COALESCE succeeds; the UPDATE is what errors out in the existing setup). (2) 422-ALIGN: the new code returned 400 (StatusBadRequest) for an unroutable (runtime, model) pair, but the create-boundary's validateRegisteredModelForRuntime callers (secrets.go:942, 952 + workspace_crud.go create) return 422 (StatusUnprocessableEntity). Both reviewers flagged the 400 as inconsistent. Updated to 422 + matching comment in the handler. The new reject-path test was renamed TestUpdate_Runtime_UnroutableModel_Fails400 → Fails422 + assertion updated to StatusUnprocessableEntity. 422 is the precise semantic: "syntactically valid PATCH body, but the (runtime, model) pair is unroutable per the registry SSOT." VERIFICATION (all green on this commit): - go test -count=1 -timeout 30s -run 'TestUpdate' ./internal/handlers/ — PASS - go test -count=1 -timeout 30s -run 'TestUpdate_Runtime_|TestWorkspaceUpdate_RuntimeField' -v ./internal/handlers/ — 4/4 PASS (TestUpdate_Runtime_RegisteredModelForRuntime_Passes, TestUpdate_Runtime_UnroutableModel_Fails422, TestWorkspaceUpdate_RuntimeField, TestWorkspaceUpdate_RuntimeField_DBErrorReturnsServerError) - gofmt -l clean - go vet ./internal/handlers/ clean CORE PATH UNCHANGED: the runtime-PATCH path still validates (newRuntime, currentModel) via the SSOT, then updates runtime + sets needsRestart=true. The migration flow (workspace_provision.go create) already uses the same SSOT. A failed check is a preventive 422 — the user gets a clear pointer to the registry-SSOT instead of wedging the agent at boot. Closes the test-mock follow-up from CR2 RC 11972 + the 422-align ask from the same review. #2926 ready for CR2 + Researcher re-review → 2-genuine. Co-Authored-By: Claude --- .../internal/handlers/workspace_crud.go | 8 +++++++- .../internal/handlers/workspace_crud_test.go | 18 ++++++++++++------ .../internal/handlers/workspace_test.go | 16 ++++++++++++++++ 3 files changed, 35 insertions(+), 7 deletions(-) diff --git a/workspace-server/internal/handlers/workspace_crud.go b/workspace-server/internal/handlers/workspace_crud.go index 97cc0fc9..030519dd 100644 --- a/workspace-server/internal/handlers/workspace_crud.go +++ b/workspace-server/internal/handlers/workspace_crud.go @@ -269,7 +269,13 @@ func (h *WorkspaceHandler) Update(c *gin.Context) { 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) - c.JSON(http.StatusBadRequest, gin.H{"error": 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 { diff --git a/workspace-server/internal/handlers/workspace_crud_test.go b/workspace-server/internal/handlers/workspace_crud_test.go index 8dc75dda..af7dd63c 100644 --- a/workspace-server/internal/handlers/workspace_crud_test.go +++ b/workspace-server/internal/handlers/workspace_crud_test.go @@ -626,14 +626,14 @@ func TestUpdate_Runtime_RegisteredModelForRuntime_Passes(t *testing.T) { } } -// TestUpdate_Runtime_UnroutableModel_Fails400 pins the (runtime, model) +// 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 400 + the +// 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 400 response code here). -func TestUpdate_Runtime_UnroutableModel_Fails400(t *testing.T) { +// 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) @@ -659,8 +659,14 @@ func TestUpdate_Runtime_UnroutableModel_Fails400(t *testing.T) { w := httptest.NewRecorder() r.ServeHTTP(w, req) - if w.Code != http.StatusBadRequest { - t.Errorf("expected 400 (unroutable (runtime, model)), got %d: %s", w.Code, w.Body.String()) + // 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 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")) -- 2.52.0