From 507696d88a3c423b63255beafdf2e53b3d56029b Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Thu, 23 Apr 2026 20:29:44 -0700 Subject: [PATCH] fix(canvas,server): address review findings on 3f11df03 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five review findings from the 3f11df03 six-bug commit: 1. Add TestPeers_DevModeFailOpen_{Allows,ClosedWhenAdminTokenSet, ClosedInProduction} covering all three gating states for the security-sensitive dev-mode hatch the prior commit added to /registry/:id/peers. Previously shipped untested — a future refactor could have silently inverted polarity or removed the gate. New tests pin the contract: * MOLECULE_ENV=development + ADMIN_TOKEN="" → allow bearerless * MOLECULE_ENV=development + ADMIN_TOKEN set → require token * MOLECULE_ENV=production → require token 2. ConfigTab handleSave diffs against the RAW parsed YAML / form config instead of the DEFAULT_CONFIG-merged shape. The previous code would silently PATCH tier=1 to the DB when a user deleted the `tier:` line in raw mode (the default-merge substituted 1). Now: only fields the user actually typed participate in the diff. Type guards (typeof === "number" / "string") prevent coercion surprises on malformed YAML. 3. ConfigTab model-save failure no longer lies "Saved". The /workspaces/:id/model PATCH can reject when the runtime doesn't support the chosen model; previously we caught + console.warn'd + showed green Saved, and the user watched the model revert on next reload with no explanation. Now the save path collects a `modelSaveError` and surfaces it via setError with a partial- success message ("Other fields saved, but model update failed: …") so the user sees why. 4. ChannelsTab now surfaces BOTH channels-fetch and adapters-fetch failures, distinguishing them in the error text ("Failed to load connected channels and platforms — try refreshing"). Previously only an adapters failure was visible; a channels failure left the user with an apparently-empty list and no indication the API was unreachable. 5. ChatTab panels drop the redundant aria-hidden attribute. The `hidden`/`flex` Tailwind class already sets display:none, which removes the node from the accessibility tree on its own; the extra aria-hidden invited WAI-ARIA lint warnings if a focusable descendant ever landed inside an inactive panel. Tests: 923 canvas + full Go handler suite pass. 3 new Go tests. No behaviour change on the five prior fixes — this commit tightens their edges per the independent review. Co-Authored-By: Claude Opus 4.7 (1M context) --- canvas/src/components/tabs/ChannelsTab.tsx | 11 +- canvas/src/components/tabs/ChatTab.tsx | 2 - canvas/src/components/tabs/ConfigTab.tsx | 55 ++++++--- .../internal/handlers/discovery_test.go | 106 ++++++++++++++++++ 4 files changed, 153 insertions(+), 21 deletions(-) diff --git a/canvas/src/components/tabs/ChannelsTab.tsx b/canvas/src/components/tabs/ChannelsTab.tsx index ed0b4241..b7e93ea4 100644 --- a/canvas/src/components/tabs/ChannelsTab.tsx +++ b/canvas/src/components/tabs/ChannelsTab.tsx @@ -69,16 +69,25 @@ export function ChannelsTab({ workspaceId }: Props) { api.get(`/workspaces/${workspaceId}/channels`), api.get(`/channels/adapters`), ]); + const errors: string[] = []; if (chResult.status === "fulfilled") { setChannels(Array.isArray(chResult.value) ? chResult.value : []); } else { console.warn("ChannelsTab: channels load failed", chResult.reason); + errors.push("connected channels"); } if (adResult.status === "fulfilled") { setAdapters(Array.isArray(adResult.value) ? adResult.value : []); } else { console.warn("ChannelsTab: adapters load failed", adResult.reason); - setError("Failed to load channel platforms — try refreshing"); + errors.push("platforms"); + } + // Surface BOTH failure modes so the user can distinguish + // "no channels configured" from "API unreachable". + if (errors.length > 0) { + setError(`Failed to load ${errors.join(" and ")} — try refreshing`); + } else { + setError(""); } setLoading(false); }, [workspaceId]); diff --git a/canvas/src/components/tabs/ChatTab.tsx b/canvas/src/components/tabs/ChatTab.tsx index 95f6ad23..3762ffdc 100644 --- a/canvas/src/components/tabs/ChatTab.tsx +++ b/canvas/src/components/tabs/ChatTab.tsx @@ -152,7 +152,6 @@ export function ChatTab({ workspaceId, data }: Props) { id="chat-panel-my-chat" role="tabpanel" aria-labelledby="chat-tab-my-chat" - aria-hidden={subTab !== "my-chat"} className={`flex-1 overflow-hidden flex-col ${ subTab === "my-chat" ? "flex" : "hidden" }`} @@ -163,7 +162,6 @@ export function ChatTab({ workspaceId, data }: Props) { id="chat-panel-agent-comms" role="tabpanel" aria-labelledby="chat-tab-agent-comms" - aria-hidden={subTab !== "agent-comms"} className={`flex-1 overflow-hidden flex-col ${ subTab === "agent-comms" ? "flex" : "hidden" }`} diff --git a/canvas/src/components/tabs/ConfigTab.tsx b/canvas/src/components/tabs/ConfigTab.tsx index ed9b82d3..09742689 100644 --- a/canvas/src/components/tabs/ConfigTab.tsx +++ b/canvas/src/components/tabs/ConfigTab.tsx @@ -257,21 +257,26 @@ export function ConfigTab({ workspaceId }: Props) { // the form, hits Save, sees the request succeed, then watches the // values snap back on the next reload because the workspace row // never heard about the change. + // + // Diff against the RAW parsed YAML (or the form `config` in non- + // raw mode) rather than the DEFAULT_CONFIG-merged shape — if the + // user deleted a field in raw mode the merge would substitute the + // default (e.g. tier=1) and we'd silently PATCH that down from + // the stored value. Only fields the user actually typed get sent. const oldParsed = parseYaml(originalYaml); - const nextParsed = rawMode ? parseYaml(rawDraft) : null; - const effective = nextParsed - ? { ...DEFAULT_CONFIG, ...nextParsed } as ConfigData - : config; + const nextSource = rawMode + ? (parseYaml(rawDraft) as Record) + : (config as unknown as Record); const dbPatch: Record = {}; - if (effective.name && effective.name !== oldParsed.name) { - dbPatch.name = effective.name; + if (typeof nextSource.name === "string" && nextSource.name && nextSource.name !== oldParsed.name) { + dbPatch.name = nextSource.name; } - if (effective.tier && effective.tier !== (oldParsed.tier ?? null)) { - dbPatch.tier = effective.tier; + if (typeof nextSource.tier === "number" && nextSource.tier !== (oldParsed.tier ?? null)) { + dbPatch.tier = nextSource.tier; } const oldRuntime = (oldParsed.runtime as string) || ""; - if (effective.runtime && effective.runtime !== oldRuntime) { - dbPatch.runtime = effective.runtime; + if (typeof nextSource.runtime === "string" && nextSource.runtime && nextSource.runtime !== oldRuntime) { + dbPatch.runtime = nextSource.runtime; } if (Object.keys(dbPatch).length > 0) { await api.patch(`/workspaces/${workspaceId}`, dbPatch); @@ -279,14 +284,21 @@ export function ConfigTab({ workspaceId }: Props) { // Model has its own endpoint (separate from the general workspace // PATCH) because the runtime may need to validate it against the - // template's supported models list. + // template's supported models list. A model rejection is a + // partial-save state — we report it as a user-visible warning + // rather than lying "Saved" and letting the user discover the + // revert on next reload. const oldModel = (oldParsed.model as string) || ""; - if (effective.model && effective.model !== oldModel) { + let modelSaveError: string | null = null; + if ( + typeof nextSource.model === "string" && + nextSource.model && + nextSource.model !== oldModel + ) { try { - await api.put(`/workspaces/${workspaceId}/model`, { model: effective.model }); + await api.put(`/workspaces/${workspaceId}/model`, { model: nextSource.model }); } catch (e) { - // Non-fatal — log and continue so the rest of the save commits. - console.warn("ConfigTab: model PATCH failed", e); + modelSaveError = e instanceof Error ? e.message : "Model update was rejected"; } } @@ -302,9 +314,16 @@ export function ConfigTab({ workspaceId }: Props) { } else { useCanvasStore.getState().updateNodeData(workspaceId, { needsRestart: true }); } - setSuccess(true); - clearTimeout(successTimerRef.current); - successTimerRef.current = setTimeout(() => setSuccess(false), 2000); + if (modelSaveError) { + // Partial-save UX: surface the model rejection instead of + // showing "Saved" — the user would otherwise watch the model + // field revert on next reload with no explanation. + setError(`Other fields saved, but model update failed: ${modelSaveError}`); + } else { + setSuccess(true); + clearTimeout(successTimerRef.current); + successTimerRef.current = setTimeout(() => setSuccess(false), 2000); + } } catch (e) { setError(e instanceof Error ? e.message : "Failed to save"); } finally { diff --git a/workspace-server/internal/handlers/discovery_test.go b/workspace-server/internal/handlers/discovery_test.go index 5b16738a..7c90005e 100644 --- a/workspace-server/internal/handlers/discovery_test.go +++ b/workspace-server/internal/handlers/discovery_test.go @@ -618,3 +618,109 @@ func TestDiscoverHostPeer_Smoke_Success(t *testing.T) { t.Errorf("expected 200, got %d", w.Code) } } + +// ==================== Peers auth — dev-mode fail-open gate ==================== +// +// validateDiscoveryCaller applies a Tier-1b dev-mode hatch so the canvas +// user session (which holds no workspace-scoped bearer) can still load +// the Details → PEERS list on a local Docker setup. The gate must pass +// ONLY when MOLECULE_ENV is development AND ADMIN_TOKEN is empty. +// These tests pin that contract against accidental polarity flips. + +// peersAuthFixtureHasLiveToken seeds the mock rows required for the +// Peers handler to reach the auth branch: HasAnyLiveToken → true (a +// non-zero count so validateDiscoveryCaller has to make the dev-mode +// decision instead of grandfathering the request). +func peersAuthFixtureHasLiveToken(mock sqlmock.Sqlmock, workspaceID string) { + // HasAnyLiveToken issues `SELECT COUNT(*) FROM workspace_auth_tokens ...` + mock.ExpectQuery("SELECT COUNT.+workspace_auth_tokens"). + WithArgs(workspaceID). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1)) +} + +func TestPeers_DevModeFailOpen_AllowsBearerlessRequest(t *testing.T) { + // Dev mode: MOLECULE_ENV=development AND ADMIN_TOKEN empty. Canvas + // sends no bearer token; validateDiscoveryCaller must return nil + // (allow) and the handler must proceed to return the peer list. + t.Setenv("MOLECULE_ENV", "development") + t.Setenv("ADMIN_TOKEN", "") + + mock := setupTestDB(t) + setupTestRedis(t) + handler := NewDiscoveryHandler() + + peersAuthFixtureHasLiveToken(mock, "ws-dev") + + // Root workspace → children+parent queries still fire but the + // parent_id lookup comes first. + mock.ExpectQuery("SELECT parent_id FROM workspaces WHERE id ="). + WithArgs("ws-dev"). + WillReturnRows(sqlmock.NewRows([]string{"parent_id"}).AddRow(nil)) + peerCols := []string{"id", "name", "role", "tier", "status", "agent_card", "url", "parent_id", "active_tasks"} + mock.ExpectQuery("SELECT w.id.+WHERE w.parent_id IS NULL AND w.id"). + WithArgs("ws-dev"). + WillReturnRows(sqlmock.NewRows(peerCols)) + mock.ExpectQuery("SELECT w.id.+WHERE w.parent_id = \\$1 AND w.status"). + WithArgs("ws-dev"). + WillReturnRows(sqlmock.NewRows(peerCols)) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-dev"}} + c.Request = httptest.NewRequest("GET", "/registry/ws-dev/peers", nil) + + handler.Peers(c) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200 under dev-mode hatch, got %d: %s", w.Code, w.Body.String()) + } +} + +func TestPeers_DevModeFailOpen_ClosedWhenAdminTokenSet(t *testing.T) { + // An operator with ADMIN_TOKEN set has explicitly opted into #684 + // closure; dev-mode hatch must NOT open even when MOLECULE_ENV is + // "development". This is the SaaS guarantee. + t.Setenv("MOLECULE_ENV", "development") + t.Setenv("ADMIN_TOKEN", "seven-admin-token") + + mock := setupTestDB(t) + setupTestRedis(t) + handler := NewDiscoveryHandler() + + peersAuthFixtureHasLiveToken(mock, "ws-prod") + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-prod"}} + c.Request = httptest.NewRequest("GET", "/registry/ws-prod/peers", nil) + + handler.Peers(c) + + if w.Code != http.StatusUnauthorized { + t.Fatalf("expected 401 with ADMIN_TOKEN set, got %d: %s", w.Code, w.Body.String()) + } +} + +func TestPeers_DevModeFailOpen_ClosedInProduction(t *testing.T) { + // Production MOLECULE_ENV — hatch must stay closed regardless of + // ADMIN_TOKEN state. SaaS production rejects the bearerless call. + t.Setenv("MOLECULE_ENV", "production") + t.Setenv("ADMIN_TOKEN", "") + + mock := setupTestDB(t) + setupTestRedis(t) + handler := NewDiscoveryHandler() + + peersAuthFixtureHasLiveToken(mock, "ws-prod") + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-prod"}} + c.Request = httptest.NewRequest("GET", "/registry/ws-prod/peers", nil) + + handler.Peers(c) + + if w.Code != http.StatusUnauthorized { + t.Fatalf("expected 401 in production, got %d: %s", w.Code, w.Body.String()) + } +}