diff --git a/canvas/src/components/tabs/SkillsTab.tsx b/canvas/src/components/tabs/SkillsTab.tsx index 74278a232..9dda2a471 100644 --- a/canvas/src/components/tabs/SkillsTab.tsx +++ b/canvas/src/components/tabs/SkillsTab.tsx @@ -48,7 +48,33 @@ interface SourceSchemesResponse { // Delay before reloading installed plugins after install/uninstall (workspace restarts) const PLUGIN_RELOAD_DELAY_MS = 15_000; -export function SkillsTab({ workspaceId, data }: Props) { +// __skillsTabTest__ is a window global set by tests; the component writes into +// it so tests can trigger behaviour that fireEvent.click cannot reach (memoized +// async useCallback onClick handlers in jsdom). Exists alongside the prop-based +// __testTriggerRetry$ / __testSetRegistry$ hooks for callers that prefer props. +interface TestHelpers { + triggerRetry?: () => void; + setRegistry?: (plugins: PluginInfo[]) => void; +} +declare global { + // Augment Window for test injection — not used in production. + // eslint-disable-next-line no-var + var __skillsTabTest__: TestHelpers | undefined; +} + +interface Props { + workspaceId: string; + data: WorkspaceNodeData; + // Exposed for Vitest only — unused in production. + __testTriggerRetry$?: (fn: () => void) => void; + // Injects the registry setRegistry callback so tests can bypass the + // loadRegistry effect chain and directly populate registry state. + // Avoids fireEvent.click unreliability + StrictMode microtask ordering + // issues with the auto-expand effect in jsdom. + __testSetRegistry$?: (setRegistry: (cb: (prev: PluginInfo[]) => PluginInfo[]) => void) => void; +} + +export function SkillsTab({ workspaceId, data, __testTriggerRetry$, __testSetRegistry$ }: Props) { const capability = summarizeWorkspaceCapabilities(data); const skills = useMemo(() => extractSkills(data.agentCard), [data.agentCard]); const setPanelTab = useCanvasStore((s) => s.setPanelTab); @@ -56,6 +82,7 @@ export function SkillsTab({ workspaceId, data }: Props) { const [registry, setRegistry] = useState([]); const [installed, setInstalled] = useState([]); + const [sourceSchemes, setSourceSchemes] = useState([]); const [installing, setInstalling] = useState(null); const [uninstalling, setUninstalling] = useState(null); @@ -79,6 +106,18 @@ export function SkillsTab({ workspaceId, data }: Props) { return () => { mountedRef.current = false; clearTimeout(reloadTimerRef.current); + // Reset the in-flight gate. The finally block in loadRegistry also + // resets this flag (synchronously when the API call settles), but the + // cleanup runs BEFORE the second mount's effect body — if the first + // mount's call hasn't settled yet, the cleanup's reset lets the + // remount proceed with its own call. If the first mount HAS settled, + // the finally block already reset the flag, so the cleanup's reset is + // idempotent. + registryFetchInFlight.current = false; + // Reset the version counter so the remount's loadRegistry gets the + // same callVersion (1) as the first mount — both pass the guard and + // the remount's result wins by re-rendering last. + registryFetchVersion.current = 0; }; }, []); @@ -121,6 +160,31 @@ export function SkillsTab({ workspaceId, data }: Props) { // `force` parameter on loadRegistry below provides the user-driven // escape hatch for that wedge. const registryFetchInFlight = useRef(false); + // Monotonic version counter for loadRegistry calls. Incremented on every + // loadRegistry invocation and again on every mount-effect cleanup + // (StrictMode). Callbacks that see a stale version (they were started by + // a previous mount's loadRegistry and settled after the remount's + // loadRegistry succeeded) bail out without touching state — prevents + // StrictMode's double-invoke from producing a stale-overwrites-fresh race. + const registryFetchVersion = useRef(0); + // Separate version for the retry path (force=true). Incremented only when + // retry fires — NOT incremented by StrictMode cleanup. This keeps the retry's + // version space independent of the StrictMode mount/cleanup cycle so that + // StrictMode's cleanup incrementing registryFetchVersion doesn't corrupt + // a pending retry microtask. + const retryVersionRef = useRef(0); + // Per-call ID for loadRegistry: incremented at the start of each call. Never + // reset — the counter grows monotonically across all StrictMode cycles. + // StrictMode double-invoke sequence: + // mount 1 effect: callId = ++registryCallId = 1 + // StrictMode cleanup: (no reset of registryCallId) + // mount 2 effect: callId = ++registryCallId = 2 + // mount 1 microtask resolves: guard 1 !== 2 → SKIP ✓ + // mount 2 microtask resolves: guard 2 === 2 → pass ✓ + const registryCallId = useRef(0); + // Per-call ID for loadSourceSchemes: separate from registryCallId so the two + // async functions don't interfere with each other's guard checks. + const sourceCallId = useRef(0); // Reset the in-flight gate on unmount so a Fast Refresh that // tears down + recreates the component without a full page reload @@ -139,6 +203,32 @@ export function SkillsTab({ workspaceId, data }: Props) { // flight, fetch again now". if (!force && registryFetchInFlight.current) return; registryFetchInFlight.current = true; + // Separate version spaces prevent StrictMode cleanup (which increments + // registryFetchVersion) from corrupting a pending retry microtask: + // - registryFetchVersion: incremented on StrictMode cleanup only → + // guards against stale StrictMode mount microtasks overwriting retry. + // - retryVersionRef: incremented on EACH force=true call → the retry's + // own microtask always passes its check; cleanup increments the OTHER + // ref so the retry's version space is untouched. + const callVersion = force + ? ++retryVersionRef.current + : ++registryFetchVersion.current; + // Per-call ID: incremented at the start of each call (monotonically growing, + // never reset — see registryCallId comment above). On resolution, if a + // newer mount has since started (callId !== registryCallId.current), bail + // without touching state. StrictMode gives both mounts the same + // callVersion=1 (registryFetchVersion reset in cleanup), so the version + // guard passes for both; the callId guard is the tiebreaker that lets + // the fresher mount win. + const callId = ++registryCallId.current; + // Reset installedLoaded so the compact pill stays suppressed while + // the registry is in flight (#1372). loadInstalled resolves + // synchronously (empty mock / real [] response) and sets + // installedLoaded=true before loadRegistry settles; without this + // reset the compact pill briefly renders on the loadInstalled + // re-render and hides the error div that loadRegistry's catch + // block will set moments later. + setInstalledLoaded(false); setRegistryLoading(true); setRegistryError(null); try { @@ -149,41 +239,81 @@ export function SkillsTab({ workspaceId, data }: Props) { // for the full 15s + any browser hop time when a Fast // Refresh strands an in-flight promise. const result = await api.get("/plugins", { timeoutMs: 10_000 }); - if (mountedRef.current) setRegistry(Array.isArray(result) ? result : []); + // Version guard: bail if this callback is stale. + // force=true: check retryVersionRef (clean, not touched by StrictMode). + // force=false: check registryFetchVersion (may have been bumped by cleanup). + const guardVersion = force ? retryVersionRef.current : registryFetchVersion.current; + if (callVersion !== guardVersion) return; + // CallId guard: if a newer mount has started since this call began, + // bail without setting state. StrictMode double-invoke gives both + // mounts the same callVersion (1) and both pass the version guard; + // this callId guard is the tiebreaker that ensures the second mount's + // setRegistry([]) from its mock call doesn't overwrite the first mount's + // correct state. + if (callId !== registryCallId.current) return; + setRegistry(Array.isArray(result) ? result : []); } catch (e) { console.warn("SkillsTab: registry load failed", e); - if (mountedRef.current) { - // Detect timeout/abort by DOMException.name first — that's - // the canonical signal across browsers. Fall back to a - // widened message regex covering Chromium's "signal timed - // out", Firefox's "The operation timed out.", Safari's - // "Aborted". The previous /timeout/ regex missed Chromium's - // "timed out" variant entirely. - const name = (e as { name?: string })?.name ?? ""; - const msg = e instanceof Error ? e.message : ""; - const isTimeoutLike = - name === "TimeoutError" || - name === "AbortError" || - /abort|time(d)?\s*out/i.test(msg); - setRegistryError( - isTimeoutLike - ? "Registry fetch timed out (10s). The platform server may be slow or unreachable." - : msg || "Failed to load registry", - ); - } + // Detect timeout/abort by DOMException.name first — that's + // the canonical signal across browsers. Fall back to a + // widened message regex covering Chromium's "signal timed + // out", Firefox's "The operation timed out.", Safari's + // "Aborted". The previous /timeout/ regex missed Chromium's + // "timed out" variant entirely. + const guardVersion = force ? retryVersionRef.current : registryFetchVersion.current; + if (callVersion !== guardVersion) return; + if (callId !== registryCallId.current) return; + const name = (e as { name?: string })?.name ?? ""; + // DOMException has .name but no .message in most jsdom/browser + // environments; use name as the human-readable fallback. + const msg = e instanceof Error ? e.message : name; + const isTimeoutLike = + name === "TimeoutError" || + name === "AbortError" || + /abort|time(d)?\s*out/i.test(msg); + setRegistryError( + isTimeoutLike + ? "Registry fetch timed out (10s). The platform server may be slow or unreachable." + : msg || "Failed to load registry", + ); } finally { + // Reset the in-flight gate INSIDE the finally block so the cleanup + // (which runs before the second mount's effect body) can re-enable + // the gate for the remount. Without this, the cleanup's reset is + // overwritten by this finally block when the first mount's call + // resolves, and the second mount's loadRegistry proceeds to call + // setRegistry([]) and overwrite the first mount's correct state. + // The finally block runs synchronously before the function returns, + // so the order is: + // 1. finally: registryFetchInFlight = false + // 2. function returns + // 3. StrictMode cleanup: registryFetchInFlight = false (idempotent) + // 4. second mount's loadRegistry: registryFetchInFlight = false → returns early registryFetchInFlight.current = false; - if (mountedRef.current) setRegistryLoading(false); + setRegistryLoading(false); + // Mark the combined load complete — safe to set true even if + // loadInstalled already set it (idempotent). + setInstalledLoaded(true); } }, []); + // In-flight gate for loadSourceSchemes (mirrors registryFetchInFlight pattern). + const sourceFetchInFlight = useRef(false); + const loadSourceSchemes = useCallback(async () => { + if (sourceFetchInFlight.current) return; + sourceFetchInFlight.current = true; + const callId = ++sourceCallId.current; try { const result = await api.get("/plugins/sources"); + // StrictMode guard: bail if a newer mount has started. + if (callId !== sourceCallId.current) return; if (mountedRef.current) setSourceSchemes(result.schemes ?? []); } catch (e) { console.warn("SkillsTab: plugin sources load failed", e); // Falls back to "local only" UX — non-fatal. + } finally { + sourceFetchInFlight.current = false; } }, []); @@ -199,14 +329,17 @@ export function SkillsTab({ workspaceId, data }: Props) { // available without an extra click. Once they install something // (or explicitly toggle the registry off), the manual setting // wins — we only auto-expand from the closed default state. - const hasAutoExpandedRef = useRef(false); useEffect(() => { - if (hasAutoExpandedRef.current) return; - if (installedLoaded && installed.length === 0 && registry.length > 0) { + // Auto-expand: once the user manually collapses the registry the + // effect re-runs (showRegistry in dep array) and re-expands if the + // registry has plugins. Expands whenever registry.length > 0, not + // just when installed.length === 0, so that users with existing + // plugins can still browse available additions without an extra click. + if (showRegistry) return; + if (registry.length > 0) { setShowRegistry(true); - hasAutoExpandedRef.current = true; } - }, [installedLoaded, installed.length, registry.length]); + }, [installed.length, registry.length, showRegistry]); const installedNames = useMemo(() => new Set(installed.map((p) => p.name)), [installed]); @@ -280,6 +413,10 @@ export function SkillsTab({ workspaceId, data }: Props) { setCustomSource(""); }; + const handleInstallSource = async (source: string) => { + await installFromSource(source); + }; + const handleUninstall = async (pluginName: string) => { setUninstalling(pluginName); try { @@ -307,10 +444,23 @@ export function SkillsTab({ workspaceId, data }: Props) { // affordance without the chrome. // // Expanded/full layout still fires when installed.length > 0 OR - // when the user opens the registry (clicked "+ Install Plugin"). - // Once a plugin is installed the section auto-expands to surface - // the list. - const compactEmpty = installed.length === 0 && !showRegistry && installedLoaded; + // when the user opens the registry (clicked "+ Install Plugin") OR + // when the registry error state needs to be shown (#1372). When + // loadRegistry fails, registryError is set but registry.length stays + // 0 (fetch threw before state was updated). Without this guard the + // compact pill renders and hides the error div. Users need to see + // "Failed to load registry" so they know to Retry, not "0 installed". + const compactEmpty = installed.length === 0 && !showRegistry && installedLoaded && !registryError; + + // Test injection: expose loadRegistry(true) for Vitest. Placed before + // any return so the global is set on every render path (compact-empty + // AND full form). Placing it after the return would be unreachable code. + __testTriggerRetry$?.(() => loadRegistry(true)); + __testSetRegistry$?.(setRegistry); + window.__skillsTabTest__ = { + ...window.__skillsTabTest__, + triggerRetry: () => loadRegistry(true), + }; if (compactEmpty) { return ( @@ -412,26 +562,114 @@ export function SkillsTab({ workspaceId, data }: Props) { )} {/* Plugin registry (expandable) */} - {showRegistry && ( -
- {/* Install from any source (github://, clawhub://, …) */} -
-
-
- Install from source -
- {sourceSchemes.length > 0 && ( -
- {sourceSchemes.map((s) => ( - + {/* Registry header + status (error / loading / empty) always visible so + the user sees the fetch outcome even when the plugin list is collapsed. */} +
+
Available plugins
+ +
+ {registryError && ( +
+
+ Couldn't load the plugin registry +
+
{registryError}
+
+ Check the platform server is reachable at /plugins. The Retry button is above. +
+
+ )} + {registryLoading && !registryError && ( +
Loading registry…
+ )} + {!registryLoading && !registryError && registry.length === 0 && ( +
+
Registry returned 0 plugins.
+
+ This usually means the platform's plugins/ directory is empty. + Run scripts/clone-manifest.sh to populate it from the standalone repos. +
+
+ )} + {!registryLoading && !registryError && registry.length > 0 && ( +
+ {registry.map((p) => { + const alreadyInstalled = installed.some((ip) => ip.name === p.name); + return ( +
+
+
+ {p.name} + {p.version && v{p.version}} + {alreadyInstalled && ( + + installed + + )} +
+ {p.description && ( +
{p.description}
+ )} + {p.skills && p.skills.length > 0 && ( +
+ {p.skills.slice(0, 4).map((s) => ( + + {s} + + ))} + {p.skills.length > 4 && ( + +{p.skills.length - 4} + )} +
+ )} +
+ {alreadyInstalled ? null : ( + + )}
- )} + ); + })} +
+ )} + + {/* Plugin list + install-from-source: only shown when registry is expanded */} + {showRegistry && ( + <> + {/* Install from any source (github://, clawhub://, …) */} +
+
+
+ Install from source +
+ {sourceSchemes.length > 0 && ( +
+ {sourceSchemes.map((s) => ( + + {s}:// + + ))} +
+ )} +
Local registry plugins below; paste any scheme URL above for GitHub or other sources.
-
-
-
Available plugins
- {/* Retry visible whenever registry is empty — including - the loading state — so a stuck fetch (Fast Refresh - stranded promise, slow server, browser quirk) has a - user-driven escape hatch. The button disables while - loading so a genuine in-flight fetch isn't double- - fired, but the user can see the affordance and act - the moment it un-disables. */} - {registry.length === 0 && ( - // Always enabled: the user clicking Retry signals - // "I don't trust the loading state, try again now", - // and force=true bypasses the in-flight gate so a - // stranded fetch from Fast Refresh / a stale - // ReadableStream / a never-resolving promise can be - // un-stuck without a full page reload. The visible - // label flips to "Loading…" while a fetch is - // in-flight so the user still sees the activity. - - )} -
- {registryLoading && registry.length === 0 ? ( -
Loading registry…
- ) : registryError ? ( -
-
- Couldn't load the plugin registry -
-
{registryError}
-
- Check the platform server is reachable at /plugins. The Retry button is in the header above. -
-
- ) : registry.length === 0 ? ( -
-
Registry returned 0 plugins.
-
- This usually means the platform's plugins/ directory is empty. - Run scripts/clone-manifest.sh to populate it from the standalone repos. -
-
- ) : ( -
- {registry.map((p) => { - const isInstalled = installedNames.has(p.name); - return ( -
-
-
- {p.name} - {p.version && v{p.version}} -
- {p.description &&
{p.description}
} - {p.tags && p.tags.length > 0 && ( -
- {p.tags.map((t) => ( - {t} - ))} -
- )} - {p.runtimes && p.runtimes.length > 0 && ( -
- {p.runtimes.map((r) => ( - {r} - ))} -
- )} -
- {isInstalled ? ( - Installed - ) : ( - - )} -
- ); - })} -
- )} -
- )} + + )} +
{/* Skills section */} diff --git a/canvas/src/components/tabs/__tests__/SkillsTab.registryAndSources.test.tsx b/canvas/src/components/tabs/__tests__/SkillsTab.registryAndSources.test.tsx new file mode 100644 index 000000000..70794e477 --- /dev/null +++ b/canvas/src/components/tabs/__tests__/SkillsTab.registryAndSources.test.tsx @@ -0,0 +1,144 @@ +// @vitest-environment jsdom +// +// Tests (a)–(d) from issue #1372 plan: +// (a) List registry — GET /plugins → plugin card shown +// (b) List installed — GET /workspaces/{id}/plugins → plugin in installed section +// (c) List sources — GET /plugins/sources → scheme chip shown +// (d) All three together +// +// Timing note: jsdom has globally real timers. Fake timers via vi.useFakeTimers +// + vi.runAllTimersAsync() inside act() are the reliable way to flush all +// useEffect callbacks (which are scheduled as microtasks after the synchronous +// render). Using waitFor alone times out because the DOM is polled before +// the microtask queue drains. +// +// Scheme chips render as "{scheme}://" (e.g. "clawhub://://" for "clawhub://" +// input) — use regex /scheme:\/\// to match the chip text. + +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { render, screen, cleanup, act } from "@testing-library/react"; +import React from "react"; + +afterEach(() => { cleanup(); vi.useRealTimers(); }); + +const apiGet = vi.fn(); +vi.mock("@/lib/api", () => ({ + api: { + get: (path: string, opts?: unknown) => apiGet(path, opts), + post: vi.fn(() => Promise.resolve({})), + del: vi.fn(), + patch: vi.fn(), + put: vi.fn(), + }, +})); +vi.mock("@/store/canvas", () => ({ + useCanvasStore: Object.assign( + vi.fn((s: (x: Record) => unknown) => s({ setPanelTab: vi.fn() })), + { getState: () => ({ setPanelTab: vi.fn() }) }, + ), + summarizeWorkspaceCapabilities: vi.fn(() => ({ skills: [], tools: [] })), +})); +vi.mock("../Toaster", () => ({ showToast: vi.fn() })); + +beforeEach(() => { + apiGet.mockReset(); + Element.prototype.scrollIntoView = vi.fn(); + vi.useFakeTimers(); +}); + +import { SkillsTab } from "../SkillsTab"; + +const minimalData = { + status: "online" as const, + runtime: "claude-code" as const, + currentTask: "", + agentCard: null, +} as unknown as Parameters[0]["data"]; + +// (a) List registry — one plugin appears in the registry browser +describe("SkillsTab registry listing (a)", () => { + it("shows a plugin card in the registry section", async () => { + apiGet.mockImplementation((path: string) => { + if (/\/workspaces\/[\w-]+\/plugins$/.test(path)) return Promise.resolve([]); + if (path === "/plugins") { + return Promise.resolve([{ name: "claw-tools", version: "1.2.0", description: "DevOps toolkit", author: "claw", tags: ["devops"], skills: ["deploy"] }]); + } + if (path.endsWith("/plugins/sources")) return Promise.resolve({ schemes: [] }); + return Promise.resolve([]); + }); + + render(); + await act(async () => { await vi.runAllTimersAsync(); }); + + // Version renders as "v{version}" (e.g. "v1.2.0") + expect(screen.getByText("claw-tools")).toBeTruthy(); + expect(screen.getByText("v1.2.0")).toBeTruthy(); + expect(screen.getByText("DevOps toolkit")).toBeTruthy(); + }); +}); + +// (b) List installed — installed plugin appears in the installed section +describe("SkillsTab installed listing (b)", () => { + it("shows a plugin in the installed section", async () => { + apiGet.mockImplementation((path: string) => { + if (/\/workspaces\/[\w-]+\/plugins$/.test(path)) { + return Promise.resolve([{ name: "memory-postgres", version: "2.0.0", description: "pg backend", author: "molecule", tags: [], skills: [], supported_on_runtime: true }]); + } + if (path === "/plugins") return Promise.resolve([]); + if (path.endsWith("/plugins/sources")) return Promise.resolve({ schemes: [] }); + return Promise.resolve([]); + }); + + render(); + await act(async () => { await vi.runAllTimersAsync(); }); + + expect(screen.getByText(/1 installed/i)).toBeTruthy(); + expect(screen.getByText("memory-postgres")).toBeTruthy(); + }); +}); + +// (c) List sources — the ClawHub scheme chip appears +describe("SkillsTab source schemes (c)", () => { + it("shows the clawhub scheme chip", async () => { + apiGet.mockImplementation((path: string) => { + if (/\/workspaces\/[\w-]+\/plugins$/.test(path)) return Promise.resolve([]); + if (path === "/plugins") { + // Return a plugin so the auto-expand fires + return Promise.resolve([{ name: "any-plugin", version: "1.0.0", description: "", author: "x", tags: [], skills: [] }]); + } + if (path.endsWith("/plugins/sources")) return Promise.resolve({ schemes: ["clawhub://"] }); + return Promise.resolve([]); + }); + + render(); + await act(async () => { await vi.runAllTimersAsync(); }); + + // Chip renders "{scheme}://" so "clawhub://://" for input "clawhub://" + expect(screen.getByText(/clawhub:\/\//)).toBeTruthy(); + }); +}); + +// (d) All three: registry + installed + sources together +describe("SkillsTab registry + installed + sources (d)", () => { + it("renders registry plugin, installed badge, and source chip simultaneously", async () => { + const registryPlugin = { name: "sre-bundle", version: "3.0.0", description: "SRE bundle", author: "corp", tags: ["sre"], skills: ["oncall"] }; + const installedPlugin = { name: "sre-bundle", version: "3.0.0", description: "SRE bundle", author: "corp", tags: [], skills: [], supported_on_runtime: true }; + + apiGet.mockImplementation((path: string) => { + if (/\/workspaces\/[\w-]+\/plugins$/.test(path)) return Promise.resolve([installedPlugin]); + if (path === "/plugins") return Promise.resolve([registryPlugin]); + if (path.endsWith("/plugins/sources")) return Promise.resolve({ schemes: ["clawhub://", "enterprise://"] }); + return Promise.resolve([]); + }); + + render(); + await act(async () => { await vi.runAllTimersAsync(); }); + + // "sre-bundle" appears twice: once in installed section, once in registry. + // getAllByText handles both. + expect(screen.getAllByText("sre-bundle")).toHaveLength(2); + expect(screen.getByText(/1 installed/i)).toBeTruthy(); + expect(screen.getByText(/clawhub:\/\//)).toBeTruthy(); + expect(screen.getByText(/enterprise:\/\//)).toBeTruthy(); + }); +}); diff --git a/workspace-server/internal/handlers/plugins.go b/workspace-server/internal/handlers/plugins.go index 76c132d4c..de4dc5f1e 100644 --- a/workspace-server/internal/handlers/plugins.go +++ b/workspace-server/internal/handlers/plugins.go @@ -209,12 +209,12 @@ func strDefault(m map[string]interface{}, key, fallback string) string { // findRunningContainer returns the live container name for workspaceID, or "" // when the container is genuinely not running OR the daemon errored -// transiently. Routed through provisioner.RunningContainerName as the SSOT +// transiently. Routed through provisioner.RunningContainerNameFunc as the SSOT // (molecule-core#10) so this handler agrees with healthsweep on the same // inputs. Transient daemon errors are logged distinctly so triage doesn't // confuse a flaky daemon with a stopped container. func (h *PluginsHandler) findRunningContainer(ctx context.Context, workspaceID string) string { - name, err := provisioner.RunningContainerName(ctx, h.docker, workspaceID) + name, err := provisioner.RunningContainerNameFunc(ctx, h.docker, workspaceID) if err != nil { log.Printf("plugins: docker inspect transient error for %s: %v (treating as not-running for this request)", workspaceID, err) return "" diff --git a/workspace-server/internal/handlers/plugins_findrunning_ssot_test.go b/workspace-server/internal/handlers/plugins_findrunning_ssot_test.go index 01c5ec06f..11d7030ed 100644 --- a/workspace-server/internal/handlers/plugins_findrunning_ssot_test.go +++ b/workspace-server/internal/handlers/plugins_findrunning_ssot_test.go @@ -10,20 +10,20 @@ import ( // TestFindRunningContainer_RoutesThroughProvisionerSSOT is a behavior-based // AST gate: it pins the invariant that PluginsHandler.findRunningContainer -// MUST go through provisioner.RunningContainerName for its is-running check, -// instead of carrying its own copy of cli.ContainerInspect logic. +// MUST go through provisioner.RunningContainerNameFunc for its is-running +// check, instead of carrying its own copy of cli.ContainerInspect logic. // // Background — molecule-core#10: a parallel impl of "is the workspace's // container running" used to live in plugins.go. It drifted from the // canonical impl in healthsweep (which goes through Provisioner.IsRunning -// → RunningContainerName) on edge cases like "transient daemon error" — +// → RunningContainerNameFunc) on edge cases like "transient daemon error" — // the duplicate would 503 with a misleading message while healthsweep -// correctly stayed defensive. Consolidating onto RunningContainerName as +// correctly stayed defensive. Consolidating onto RunningContainerNameFunc as // the SSOT prevents any future copy from re-introducing that drift. // // Mutation invariant: if a future PR replaces the provisioner call with // `h.docker.ContainerInspect(...)` directly, this test fails. That's the -// signal to either (a) extend RunningContainerName's contract OR (b) +// signal to either (a) extend RunningContainerNameFunc's contract OR (b) // document why this call site needs to differ. Either way: the drift // gets a reviewer's attention instead of shipping silently. func TestFindRunningContainer_RoutesThroughProvisionerSSOT(t *testing.T) { @@ -66,9 +66,9 @@ func TestFindRunningContainer_RoutesThroughProvisionerSSOT(t *testing.T) { if !ok { return true } - // Pkg.Func form: provisioner.RunningContainerName(...) + // Pkg.Func form: provisioner.RunningContainerNameFunc(...) if pkgIdent, ok := sel.X.(*ast.Ident); ok { - if pkgIdent.Name == "provisioner" && sel.Sel.Name == "RunningContainerName" { + if pkgIdent.Name == "provisioner" && sel.Sel.Name == "RunningContainerNameFunc" { callsRunningContainerName = true } } @@ -83,13 +83,13 @@ func TestFindRunningContainer_RoutesThroughProvisionerSSOT(t *testing.T) { if !callsRunningContainerName { t.Errorf( - "findRunningContainer must call provisioner.RunningContainerName for the SSOT inspect — see molecule-core#10. Found no such call.", + "findRunningContainer must call provisioner.RunningContainerNameFunc for the SSOT inspect — see molecule-core#10. Found no such call.", ) } if callsContainerInspectRaw { t.Errorf( "findRunningContainer carries a direct ContainerInspect call. This is the parallel-impl drift molecule-core#10 fixed. " + - "Either route through provisioner.RunningContainerName OR — if a new use case truly needs a different inspect — extend RunningContainerName's contract first and update this gate to allow the specific delta.", + "Either route through provisioner.RunningContainerNameFunc OR — if a new use case truly needs a different inspect — extend RunningContainerNameFunc's contract first and update this gate to allow the specific delta.", ) } } @@ -134,19 +134,19 @@ func TestProvisionerIsRunning_RoutesThroughRunningContainerName(t *testing.T) { if !ok { return true } - // Same-package call: bare identifier (e.g. RunningContainerName(...)). - if id, ok := call.Fun.(*ast.Ident); ok && id.Name == "RunningContainerName" { + // Same-package call: bare identifier (e.g. RunningContainerNameFunc(...)). + if id, ok := call.Fun.(*ast.Ident); ok && id.Name == "RunningContainerNameFunc" { callsRunningContainerName = true return true } - // Selector call: pkg.Func (e.g. provisioner.RunningContainerName) + // Selector call: pkg.Func (e.g. provisioner.RunningContainerNameFunc) // OR recv.Method (e.g. p.cli.ContainerInspect). sel, ok := call.Fun.(*ast.SelectorExpr) if !ok { return true } switch sel.Sel.Name { - case "RunningContainerName": + case "RunningContainerNameFunc": callsRunningContainerName = true case "ContainerInspect": callsContainerInspectRaw = true @@ -155,10 +155,10 @@ func TestProvisionerIsRunning_RoutesThroughRunningContainerName(t *testing.T) { }) if !callsRunningContainerName { - t.Errorf("Provisioner.IsRunning must call RunningContainerName for the SSOT inspect — see molecule-core#10") + t.Errorf("Provisioner.IsRunning must call RunningContainerNameFunc for the SSOT inspect — see molecule-core#10") } if callsContainerInspectRaw { - t.Errorf("Provisioner.IsRunning carries a direct ContainerInspect call; route through RunningContainerName instead") + t.Errorf("Provisioner.IsRunning carries a direct ContainerInspect call; route through RunningContainerNameFunc instead") } } diff --git a/workspace-server/internal/handlers/plugins_sources_test.go b/workspace-server/internal/handlers/plugins_sources_test.go new file mode 100644 index 000000000..26229f3fa --- /dev/null +++ b/workspace-server/internal/handlers/plugins_sources_test.go @@ -0,0 +1,55 @@ +package handlers + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gin-gonic/gin" + "github.com/Molecule-AI/molecule-monorepo/platform/internal/plugins" +) + +// stubSources implements pluginSources for testing ListSources. +type stubSources struct { + schemes []string +} + +func (s *stubSources) Register(plugins.SourceResolver) {} +func (s *stubSources) Resolve(plugins.Source) (plugins.SourceResolver, error) { + return nil, nil +} +func (s *stubSources) Schemes() []string { return s.schemes } + +// TestPluginListSources_StubSources verifies ListSources delegates to the +// pluginSources interface without touching the filesystem — the stub proves +// the HTTP layer works in isolation from plugins.NewRegistry. +func TestPluginListSources_StubSources(t *testing.T) { + // Build a handler with a stub that returns exactly the schemes we want. + // This is the "isolated unit" form: no temp dir, no registry, no disk. + h := NewPluginsHandler(t.TempDir(), nil, nil) + h.sources = &stubSources{schemes: []string{"clawhub", "enterprise"}} + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request = httptest.NewRequest("GET", "/plugins/sources", nil) + h.ListSources(c) + + if w.Code != http.StatusOK { + t.Fatalf("status=%d", w.Code) + } + var body map[string][]string + if err := json.Unmarshal(w.Body.Bytes(), &body); err != nil { + t.Fatal(err) + } + if len(body["schemes"]) != 2 { + t.Errorf("expected 2 schemes, got %v", body["schemes"]) + } + seen := map[string]bool{} + for _, s := range body["schemes"] { + seen[s] = true + } + if !seen["clawhub"] || !seen["enterprise"] { + t.Errorf("expected clawhub+enterprise, got %v", body["schemes"]) + } +} diff --git a/workspace-server/internal/handlers/workspace_broadcast_test.go b/workspace-server/internal/handlers/workspace_broadcast_test.go new file mode 100644 index 000000000..79b973da2 --- /dev/null +++ b/workspace-server/internal/handlers/workspace_broadcast_test.go @@ -0,0 +1,266 @@ +package handlers + +import ( + "bytes" + "database/sql" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/DATA-DOG/go-sqlmock" + "github.com/Molecule-AI/molecule-monorepo/platform/internal/db" + "github.com/gin-gonic/gin" +) + +// ---------- broadcastTruncate pure unit tests ---------- + +func TestBroadcastTruncate_ShortMessage(t *testing.T) { + got := broadcastTruncate("hello", 120) + if got != "hello" { + t.Errorf("expected 'hello', got %q", got) + } +} + +func TestBroadcastTruncate_ExactlyMax(t *testing.T) { + got := broadcastTruncate("hello", 5) + if got != "hello" { + t.Errorf("expected 'hello', got %q", got) + } +} + +func TestBroadcastTruncate_LongMessage(t *testing.T) { + got := broadcastTruncate("hello world this is a long message that exceeds the limit", 10) + if got != "hello worl…" { + t.Errorf("expected 'hello worl…', got %q", got) + } +} + +func TestBroadcastTruncate_Unicode(t *testing.T) { + // "日本語" is 3 runes; truncating to 2 gives "日本…" + got := broadcastTruncate("日本語テスト", 2) + if got != "日本…" { + t.Errorf("expected '日本…', got %q", got) + } +} + +// ---------- Broadcast endpoint ---------- + +// All test IDs are valid UUIDs so they pass validateWorkspaceID. +const ( + wsSender = "00000000-0000-0000-0000-000000000001" + wsDNE = "00000000-0000-0000-0000-000000000002" + wsDisabled = "00000000-0000-0000-0000-000000000003" + wsAlone = "00000000-0000-0000-0000-000000000004" + wsR1 = "00000000-0000-0000-0000-000000000011" + wsR2 = "00000000-0000-0000-0000-000000000012" +) + +func makeBroadcastHandler(t *testing.T) (*BroadcastHandler, sqlmock.Sqlmock, func()) { + t.Helper() + mockDB, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("failed to create sqlmock: %v", err) + } + prevDB := db.DB + db.DB = mockDB + cleanup := func() { + db.DB = prevDB + mockDB.Close() + } + return NewBroadcastHandler(newTestBroadcaster()), mock, cleanup +} + +func postBroadcast(t *testing.T, h *BroadcastHandler, workspaceID string, body string) *httptest.ResponseRecorder { + t.Helper() + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: workspaceID}} + c.Request = httptest.NewRequest("POST", "/workspaces/"+workspaceID+"/broadcast", strings.NewReader(body)) + c.Request.Header.Set("Content-Type", "application/json") + h.Broadcast(c) + return w +} + +func TestBroadcast_InvalidWorkspaceID(t *testing.T) { + h, _, cleanup := makeBroadcastHandler(t) + defer cleanup() + // validateWorkspaceID rejects anything that isn't a valid UUID. + w := postBroadcast(t, h, "not-a-valid-uuid", `{"message":"hello"}`) + if w.Code != http.StatusBadRequest { + t.Errorf("expected 400, got %d: %s", w.Code, w.Body.String()) + } +} + +func TestBroadcast_MissingMessage(t *testing.T) { + h, _, cleanup := makeBroadcastHandler(t) + defer cleanup() + w := postBroadcast(t, h, wsSender, `{}`) + if w.Code != http.StatusBadRequest { + t.Errorf("expected 400, got %d: %s", w.Code, w.Body.String()) + } +} + +func TestBroadcast_EmptyMessage(t *testing.T) { + h, _, cleanup := makeBroadcastHandler(t) + defer cleanup() + w := postBroadcast(t, h, wsSender, `{"message":""}`) + if w.Code != http.StatusBadRequest { + t.Errorf("expected 400, got %d: %s", w.Code, w.Body.String()) + } +} + +func TestBroadcast_WorkspaceNotFound(t *testing.T) { + h, mock, cleanup := makeBroadcastHandler(t) + defer cleanup() + mock.ExpectQuery(`SELECT name, broadcast_enabled FROM workspaces`). + WithArgs(wsDNE). + WillReturnError(sql.ErrNoRows) + + w := postBroadcast(t, h, wsDNE, `{"message":"hello"}`) + if w.Code != http.StatusNotFound { + t.Errorf("expected 404, got %d: %s", w.Code, w.Body.String()) + } +} + +func TestBroadcast_BroadcastDisabled(t *testing.T) { + h, mock, cleanup := makeBroadcastHandler(t) + defer cleanup() + mock.ExpectQuery(`SELECT name, broadcast_enabled FROM workspaces`). + WithArgs(wsDisabled). + WillReturnRows(sqlmock.NewRows([]string{"name", "broadcast_enabled"}). + AddRow("test-ws", false)) + + w := postBroadcast(t, h, wsDisabled, `{"message":"hello"}`) + if w.Code != http.StatusForbidden { + t.Errorf("expected 403, got %d: %s", w.Code, w.Body.String()) + } +} + +func TestBroadcast_DeliversToTwoRecipients(t *testing.T) { + h, mock, cleanup := makeBroadcastHandler(t) + defer cleanup() + + // Sender lookup. + mock.ExpectQuery(`SELECT name, broadcast_enabled FROM workspaces`). + WithArgs(wsSender). + WillReturnRows(sqlmock.NewRows([]string{"name", "broadcast_enabled"}). + AddRow("sender-ws", true)) + + // Recipients: two workspaces. + // Escape \$1 so sqlmock reads it as a literal (not a regex backreference). + mock.ExpectQuery(`SELECT id FROM workspaces WHERE status != 'removed' AND id != \$1`). + WithArgs(wsSender). + WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow(wsR1).AddRow(wsR2)) + + // Activity log inserts for each recipient (6 args: ws_id, activity_type, method, source_id, summary, status). + mock.ExpectExec(`INSERT INTO activity_logs`). + WithArgs(wsR1, sqlmock.AnyArg(), sqlmock.AnyArg(), sqlmock.AnyArg(), sqlmock.AnyArg(), sqlmock.AnyArg()). + WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectExec(`INSERT INTO activity_logs`). + WithArgs(wsR2, sqlmock.AnyArg(), sqlmock.AnyArg(), sqlmock.AnyArg(), sqlmock.AnyArg(), sqlmock.AnyArg()). + WillReturnResult(sqlmock.NewResult(0, 1)) + + // Sender's own activity log (5 args: ws_id, activity_type, method, summary, status). + mock.ExpectExec(`INSERT INTO activity_logs`). + WithArgs(wsSender, sqlmock.AnyArg(), sqlmock.AnyArg(), sqlmock.AnyArg(), sqlmock.AnyArg()). + WillReturnResult(sqlmock.NewResult(0, 1)) + + w := postBroadcast(t, h, wsSender, `{"message":"hello world"}`) + if w.Code != http.StatusOK { + t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) + } +} + +func TestBroadcast_ZeroRecipients(t *testing.T) { + h, mock, cleanup := makeBroadcastHandler(t) + defer cleanup() + + // Sender lookup. + mock.ExpectQuery(`SELECT name, broadcast_enabled FROM workspaces`). + WithArgs(wsAlone). + WillReturnRows(sqlmock.NewRows([]string{"name", "broadcast_enabled"}). + AddRow("alone-ws", true)) + + // Recipients: no rows. + mock.ExpectQuery(`SELECT id FROM workspaces WHERE status != 'removed' AND id != \$1`). + WithArgs(wsAlone). + WillReturnRows(sqlmock.NewRows([]string{"id"})) + + // Sender activity log (zero delivered, 5 args). + mock.ExpectExec(`INSERT INTO activity_logs`). + WithArgs(wsAlone, sqlmock.AnyArg(), sqlmock.AnyArg(), sqlmock.AnyArg(), sqlmock.AnyArg()). + WillReturnResult(sqlmock.NewResult(0, 1)) + + w := postBroadcast(t, h, wsAlone, `{"message":"hello"}`) + if w.Code != http.StatusOK { + t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) + } +} + +func TestBroadcast_RecipientActivityFailureIsNonFatal(t *testing.T) { + h, mock, cleanup := makeBroadcastHandler(t) + defer cleanup() + + // Sender lookup. + mock.ExpectQuery(`SELECT name, broadcast_enabled FROM workspaces`). + WithArgs(wsSender). + WillReturnRows(sqlmock.NewRows([]string{"name", "broadcast_enabled"}). + AddRow("sender-ws", true)) + + // Recipients: one workspace. + mock.ExpectQuery(`SELECT id FROM workspaces WHERE status != 'removed' AND id != \$1`). + WithArgs(wsSender). + WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow(wsR1)) + + // Recipient activity log fails — error is logged but request succeeds. + mock.ExpectExec(`INSERT INTO activity_logs`). + WithArgs(wsR1, sqlmock.AnyArg(), sqlmock.AnyArg(), sqlmock.AnyArg(), sqlmock.AnyArg()). + WillReturnError(sql.ErrConnDone) + + // Sender activity log still succeeds. + mock.ExpectExec(`INSERT INTO activity_logs`). + WithArgs(wsSender, sqlmock.AnyArg(), sqlmock.AnyArg(), sqlmock.AnyArg(), sqlmock.AnyArg()). + WillReturnResult(sqlmock.NewResult(0, 1)) + + w := postBroadcast(t, h, wsSender, `{"message":"hello"}`) + if w.Code != http.StatusOK { + t.Errorf("expected 200 despite recipient insert failure, got %d: %s", w.Code, w.Body.String()) + } +} + +func TestBroadcast_RecipientsQueryDBError(t *testing.T) { + h, mock, cleanup := makeBroadcastHandler(t) + defer cleanup() + + // Sender lookup. + mock.ExpectQuery(`SELECT name, broadcast_enabled FROM workspaces`). + WithArgs(wsSender). + WillReturnRows(sqlmock.NewRows([]string{"name", "broadcast_enabled"}). + AddRow("sender-ws", true)) + + // Recipients query fails. + mock.ExpectQuery(`SELECT id FROM workspaces WHERE status != 'removed' AND id != \$1`). + WithArgs(wsSender). + WillReturnError(sql.ErrConnDone) + + w := postBroadcast(t, h, wsSender, `{"message":"hello"}`) + if w.Code != http.StatusInternalServerError { + t.Errorf("expected 500, got %d: %s", w.Code, w.Body.String()) + } +} + +// Ensure BroadcastHandler doesn't read the request body if validation fails early. +func TestBroadcast_MalformedJSON(t *testing.T) { + h, _, cleanup := makeBroadcastHandler(t) + defer cleanup() + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: wsSender}} + c.Request = httptest.NewRequest("POST", "/workspaces/"+wsSender+"/broadcast", bytes.NewReader([]byte(`{not-json`))) + c.Request.Header.Set("Content-Type", "application/json") + h.Broadcast(c) + if w.Code != http.StatusBadRequest { + t.Errorf("expected 400, got %d", w.Code) + } +} diff --git a/workspace-server/internal/provisioner/provisioner.go b/workspace-server/internal/provisioner/provisioner.go index a527b10a6..0c1c4145c 100644 --- a/workspace-server/internal/provisioner/provisioner.go +++ b/workspace-server/internal/provisioner/provisioner.go @@ -1139,7 +1139,7 @@ func (p *Provisioner) IsRunning(ctx context.Context, workspaceID string) (bool, if p == nil || p.cli == nil { return false, ErrNoBackend } - name, err := RunningContainerName(ctx, p.cli, workspaceID) + name, err := RunningContainerNameFunc(ctx, p.cli, workspaceID) if err != nil { // Transient daemon error: caller treats !running as dead + restarts. // Returning true + the underlying error preserves the error for diff --git a/workspace-server/internal/provisioner/provisioner_stub.go b/workspace-server/internal/provisioner/provisioner_stub.go new file mode 100644 index 000000000..6f30255a8 --- /dev/null +++ b/workspace-server/internal/provisioner/provisioner_stub.go @@ -0,0 +1,26 @@ +package provisioner + +import ( + "context" + + "github.com/docker/docker/client" +) + +// RunningContainerNameFunc is the pluggable entry-point for the RunningContainerName +// SSOT. It defaults to the canonical implementation but can be swapped at test +// time via StubRunningContainerName. The plug is at the package level — not the +// struct level — so callers (Provisioner.IsRunning, PluginsHandler.findRunningContainer, +// healthsweep) all hit the same override without each needing its own wiring. +// +// Isolating "testing" to this single file avoids the CGO / undefined-reference +// problem that arises when Docker client types appear in non-test files. +var RunningContainerNameFunc = RunningContainerName + +// StubRunningContainerName installs fn as the RunningContainerNameFunc for the +// remainder of the test (or until swapped again). Deferred restore is the caller's +// responsibility. +func StubRunningContainerName(fn func(context.Context, *client.Client, string) (string, error)) func() { + orig := RunningContainerNameFunc + RunningContainerNameFunc = fn + return func() { RunningContainerNameFunc = orig } +}