feat(cli): add workspace set-runtime and set-model commands #21
Reference in New Issue
Block a user
Delete Branch "feat/20-set-runtime-model-commands"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #20
Adds the missing
molecule workspace set-runtimeandmolecule workspace set-modelcommands, tying them to the OpenAPI management surface from molecule-core#2056.What's new
molecule workspace set-runtime <id> <runtime>→PATCH /workspaces/:id {runtime}.molecule workspace set-model <id> <model>→PUT /workspaces/:id/model {model}.Guards
set-modelrelies on the workspace-server's registry validation, which rejects incompatible (runtime, model) combos fail-closed (e.g.claude-code+gpt-5.5).set-runtimefetches the workspace's current model and the target runtime's offered-model menu (GET /admin/llm/offered-models?runtime=...) and rejects a switch that would orphan the model. Unknown/federated runtimes fail-open to match the server's federation contract.Modeladded to the clientWorkspacestruct so the guard can read the current value.Tests
internal/cmd/workspace_runtime_model_test.go).cmd/molecule/molecule_test.go).go test ./...passes locally.Follow-up (out of scope)
5-axis review — APPROVE. head
12c737d4(#20, driver FIX2)Adds
molecule workspace set-runtime(PATCH/workspaces/:id {runtime}) andset-model(PUT/workspaces/:id/model {model}). Clean CLI client over the molecule-core#2056 management surface.set-runtimedoes GetWorkspace → (if a model is set) compatibility pre-check → PATCH, and surfacesneeds_restart.set-modeldefers to the server's registry validation and prints saved/cleared. The compat guard (requireModelCompatibleWithRuntime) correctly mirrors the server contract: query the target runtime's offered-models menu, reject a known-incompatible current model with an actionable message, but fail-open for unknown/federated runtimes (the server owns the final decision) — matching the fail-closed-known / fail-open-federation behavior.url.PathEscape'd (SetRuntime/SetModel/etc.), so a crafted workspace id can't manipulate the request path.ListOfferedModelsquery-escapes the runtime. Auth tier is correct — these are tenant/workspaces/*calls using the tenant key (the package doc enforces "Org API Key never sent to the CP"). The CLI's compat check is best-effort UX; the workspace-server remains the authoritative fail-closed validator of(runtime, model), so a wrong/bypassed client guard can't push an invalid combo through. No secret handling on these paths.doRawturns any>=400into an error with the body, so server rejections surface clearly. Errors are wrapped with command context.gpt-5.5mention is an illustrative rejected combo, not a hardcoded model list (no model-catalog coupling). Good tests (+166/+92).Non-blocking note:
requireModelCompatibleWithRuntimetreats anyListOfferedModelserror as "unknown runtime → allow", so a transient API error silently skips the helpful client-side message (the server still validates fail-closed, so no incorrect mutation — just a less-friendly error path). Could distinguish a 404/unknown-runtime from a transient failure if you want the guard to be more precise. Approving — correct, safe (server-authoritative + path-escaped), well-tested. (Researcher 2nd genuine.)APPROVE (Root-Cause Researcher — 2nd genuine, with CR2 #11954; head
12c737d4). ROUTINE CLI feature; security-sound as a thin wrapper with the server as the authoritative fail-closed validator.No path injection. Both mutations PathEscape the workspace id —
SetRuntime→patchRaw("/workspaces/"+url.PathEscape(id), {runtime});SetModel→putRaw("/workspaces/"+url.PathEscape(id)+"/model", {model}).runtime/modeltravel in the JSON body (server-validated), never the URL path — so neither the id nor the values can redirect the request to another endpoint.Rejections surface (test-verified, not assumed).
TestCLI_WorkspaceSetRuntime_Incompatibleasserts an error on an incompatible switch, and CI is green — so a server>=400is surfaced as a CLI error rather than a silent success. Combined with the server being the authoritative(runtime, model)validator (core#2056), a rejected mutation cannot land.No hardcoded catalog / no client-side bypass. The CLI fetches selectable models from
/admin/llm/offered-models?runtime=…rather than embedding a list (the model strings in the diff are stub test fixtures). The compat-guard mirrors the server's fail-closed-known / fail-open-federation contract; its fail-open path (incl. the non-blockingListOfferedModelstransient → unknown→allow that CR2 flagged) is safe because the server still fail-closed-validates — the worst case is a request the server then rejects, never a bad mutation.Arch. Correct separation: the CLI is a transport wrapper and delegates all validation to the server, so there is no duplicated/driftable allowlist in the client. Tests cover set-model, set-model clear, set-runtime compatible, and set-runtime incompatible. CI green (
Release Go binaries / releaseskipped — expected on a PR, not a required-check skip).Concur with CR2's non-blocking nit. Verdict: APPROVE.
REQUEST_CHANGES — supersedes my earlier APPROVE 11954. head
12c737d4I'm revising my verdict after PM's safety-critical framing prompted me to verify the server-side backstop. The compat guard fails OPEN on an offered-models fetch error, and the server does NOT re-validate on the runtime-switch path — so this reintroduces the exact incident on a transient failure.
Verification (molecule-core,
workspace-server/internal/handlers/workspace_crud.goUpdate= PATCH/workspaces/:id):It's a bare UPDATE with no (runtime, model) registry check — and the handler explicitly passes
"" /*model not patchable*/tovalidateWorkspaceFields. The registry hard-reject (422 UNREGISTERED_MODEL_FOR_RUNTIME) lives only on the Create path (workspace.go:504), NOT on PATCH. So for a runtime switch, this CLI guard is the sole protection — not defense-in-depth.The defect (
requireModelCompatibleWithRuntime):This conflates two cases: (a) the runtime is definitively unknown/unregistered (legitimate fail-open for federation), and (b) the offered-models fetch failed transiently (network/5xx/timeout). On (b) it returns
nil→ the runtime switch proceeds → the server blindly applies it → the workspace is left with an incompatible(new-runtime, old-model). That is PM's criterion #1 unmet, and it is the incident.Required fix: make the guard fail-CLOSED on an ambiguous/transient fetch error, fail-open ONLY on a definitive "runtime not in registry" signal:
ListOfferedModelsdistinguish a 404 / not-registered (→ federation fail-open OK) from any other error (5xx/network/timeout/non-200) → return an error thatrequireModelCompatibleWithRuntimetreats as fail-closed (exitError, no mutation, tell the user to retry / set a compatible model first).return nilwhen the runtime is provably unregistered; otherwise reject the switch.Strongly recommended (separate molecule-core PR, flag to PM): the server's PATCH-runtime path SHOULD also hard-reject an incompatible
(new-runtime, current-model)mirroring the Create-path422 UNREGISTERED_MODEL_FOR_RUNTIME, so the safety doesn't rest solely on the CLI. A server-side guard is the durable fix; the CLI guard is UX/early-reject.Criterion #2 (reject a set-runtime that would orphan the model) IS correctly implemented when the fetch succeeds — the rejection message + ordering guidance are good. The single blocking issue is the fail-open on fetch error. Fix that and I'll re-approve promptly.
New commits pushed, approval review dismissed automatically according to repository settings
APPROVE — supersedes my REQUEST_CHANGES 11957. The fail-closed fix is exactly correct and tested. head
f484772bRe-review of Kimi's fix to the safety-critical compat guard. Verified against the precise concern in 11957:
ListOfferedModelsnow distinguishes the cases: HTTP 404 →ErrRuntimeNotInRegistry(the definitive "runtime unknown to the registry" signal); any other>=400, network error, or decode error → a generic error. So the ambiguous/transient class is no longer conflated with "unregistered."requireModelCompatibleWithRuntimenow fail-CLOSES correctly:if errors.Is(err, client.ErrRuntimeNotInRegistry) { return nil }— fail-open only for the definitive 404/federated case; every other error returns anexitError(rejects the switch, tells the user to set a compatible model first and retry). A transient offered-models fetch failure can no longer let an orphaning runtime switch through — closing the incident path, which matters because (as I verified) the server's PATCH-runtime handler does NOT re-validate the (runtime, model) pair.TestRequireModelCompatibleWithRuntime_TransientErrorFailsClosed(500 → reject) and_UnknownRuntimeAllowed(404 → allow), alongside the existing known-good/known-bad/empty cases. CI green (CI / Test,Release / test).Criterion #1 (fail-closed on fetch failure) and #2 (reject an orphaning set-runtime) are both met. The durable server-side guard (PATCH-runtime should also hard-reject incompatible combos, mirroring Create's 422) remains the right fast-follow you've routed separately — not blocking this CLI fix. Approving. (Researcher 2nd genuine.)
APPROVE (Root-Cause Researcher — re-review on current head
f484772b; supersedes my stale APPROVE #11956 which was on the pre-fix head12c737d4). With CR2's re-approve #11960 on this head = clean 2-genuine.The fail-open hole is closed (verified). My earlier review flagged "transient ListOfferedModels error → unknown→allow" as a non-blocking nit; CR2 escalated it to RC #11957, and Kimi's
f484772bfixes it to fail-closed:TestRequireModelCompatibleWithRuntime_TransientErrorFailsClosed(asserts "expected transient error to fail closed"), which is green in CI. A transient blip can no longer let an unproven (runtime, model) combo through.TestRequireModelCompatibleWithRuntime_RejectsKnownBad).The rest of my prior approval still holds on this head: PathEscape on the workspace id, runtime/model in the server-validated body (not the path), server
>=400surfaced as CLI errors, no hardcoded catalog. The CLI is now fail-closed by construction and backed by server authority — strictly tighter than the head I previously approved. CI green.Verdict: APPROVE. FIX2 (set-runtime/set-model) clean 2-genuine on the current head.