From 991e0e212b02e946f6bb6bbb8b06b0a41584b335 Mon Sep 17 00:00:00 2001 From: core-devops Date: Sun, 21 Jun 2026 04:51:40 -0700 Subject: [PATCH] fix(plugins): ListInstalled reads installed plugins on SaaS (EIC), not just Docker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GET /workspaces/:id/plugins (ListInstalled) only ls'd a LOCAL Docker container. On SaaS (EC2-per-workspace) there is no local container, so findRunningContainer returned "" and the handler returned [] — an installed plugin read back as not-installed on EVERY SaaS tenant, even though it was on the box and working (the "[] readback after a successful install" symptom). Fix: add a SaaS dispatch branch mirroring Install/Uninstall (lookupSaaSDispatch). New listPluginsViaEIC lists the directory names under the workspace EC2's /plugins/ over the existing EIC SSH tunnel; each manifest is read via the existing readPluginManifestViaEIC. Same validation (validatePluginName drops traversal names) and fail-soft posture as the Docker path (an unreachable box returns [] + 200, never a 5xx). Runtime- support annotation is extracted into annotateRuntimeSupport and shared by both branches. - plugins_install_eic.go: add buildPluginsListShell + listPluginsViaEIC (var for test stubbing, mirrors installPluginViaEIC). - plugins_listing.go: ListInstalled dispatches Docker → SaaS → empty; extract annotateRuntimeSupport. - plugins_listing_saas_test.go: cover SaaS list (with/without manifest), list error fail-soft, and traversal-name rejection. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../internal/handlers/plugins_install_eic.go | 66 +++++++++ .../internal/handlers/plugins_listing.go | 113 ++++++++++----- .../handlers/plugins_listing_saas_test.go | 130 ++++++++++++++++++ 3 files changed, 271 insertions(+), 38 deletions(-) create mode 100644 workspace-server/internal/handlers/plugins_listing_saas_test.go diff --git a/workspace-server/internal/handlers/plugins_install_eic.go b/workspace-server/internal/handlers/plugins_install_eic.go index 03a9ac687..a5ca37b40 100644 --- a/workspace-server/internal/handlers/plugins_install_eic.go +++ b/workspace-server/internal/handlers/plugins_install_eic.go @@ -247,3 +247,69 @@ func realReadPluginManifestViaEIC(ctx context.Context, instanceID, runtime, plug } return out, nil } + +// buildPluginsListShell returns the remote command that lists the immediate +// child directory names under /plugins/. One name per +// line; missing-dir lands as empty stdout (the `2>/dev/null || true` mirrors +// the local-Docker ListInstalled `ls ... || true`). `-mindepth 1 -maxdepth 1 +// -type d -printf '%f\n'` returns bare names (not paths) so the caller can +// validate + read each plugin.yaml without string-stripping. +func buildPluginsListShell(hostPluginsDir string) string { + q := shellQuote(hostPluginsDir) + return fmt.Sprintf( + "sudo -n find %s -mindepth 1 -maxdepth 1 -type d -printf '%%f\\n' 2>/dev/null || true", + q, + ) +} + +// listPluginsViaEIC returns the directory names under the SaaS workspace EC2's +// /plugins/ — the EIC sibling of the local-Docker +// `ls /configs/plugins/` in ListInstalled. Without it, ListInstalled returns +// [] for EVERY SaaS tenant (findRunningContainer finds no LOCAL container), so +// an installed plugin reads back as not-installed even though it is on the box. +// +// Best-effort: a missing plugins dir returns an empty slice (not an error), +// matching the local path's `|| true`. Tunnel/ssh failures DO return an error +// so the caller can distinguish "no plugins" from "couldn't reach the box". +var listPluginsViaEIC = realListPluginsViaEIC + +func realListPluginsViaEIC(ctx context.Context, instanceID, runtime string) ([]string, error) { + if instanceID == "" { + return nil, fmt.Errorf("listPluginsViaEIC: empty instance_id") + } + + hostPluginsDir := filepath.Join(resolveWorkspaceRootPath(runtime, "/configs"), "plugins") + cmd := buildPluginsListShell(hostPluginsDir) + + ctx, cancel := context.WithTimeout(ctx, eicPluginOpTimeout) + defer cancel() + + var stdout bytes.Buffer + runErr := withEICTunnel(ctx, instanceID, func(s eicSSHSession) error { + sshCmd := exec.CommandContext(ctx, "ssh", s.sshArgs(cmd)...) + sshCmd.Env = os.Environ() + var stderr bytes.Buffer + sshCmd.Stdout = &stdout + sshCmd.Stderr = &stderr + if err := sshCmd.Run(); err != nil { + return fmt.Errorf( + "ssh list: %w (instance=%s runtime=%s stderr=%s)", + err, instanceID, runtime, strings.TrimSpace(stderr.String()), + ) + } + return nil + }) + if runErr != nil { + return nil, runErr + } + + var names []string + for _, line := range strings.Split(stdout.String(), "\n") { + name := strings.TrimSpace(line) + if name == "" { + continue + } + names = append(names, name) + } + return names, nil +} diff --git a/workspace-server/internal/handlers/plugins_listing.go b/workspace-server/internal/handlers/plugins_listing.go index 32e8ae37d..2deed2eb0 100644 --- a/workspace-server/internal/handlers/plugins_listing.go +++ b/workspace-server/internal/handlers/plugins_listing.go @@ -2,6 +2,7 @@ package handlers import ( "fmt" + "log" "net/http" "os" "path/filepath" @@ -61,53 +62,89 @@ func (h *PluginsHandler) ListInstalled(c *gin.Context) { ctx := c.Request.Context() plugins := []pluginInfo{} - containerName := h.findRunningContainer(ctx, workspaceID) - if containerName == "" { - c.JSON(http.StatusOK, plugins) - return - } - - // List directories in /configs/plugins/ - output, err := h.execInContainer(ctx, containerName, []string{ - "sh", "-c", "ls -1 /configs/plugins/ 2>/dev/null || true", - }) - if err != nil { - c.JSON(http.StatusOK, plugins) - return - } - - for _, name := range strings.Split(output, "\n") { - name = strings.TrimSpace(name) - if name == "" || validatePluginName(name) != nil { - continue - } - // Try to read manifest from container (safe: name is validated) - manifestOutput, err := h.execInContainer(ctx, containerName, []string{ - "cat", fmt.Sprintf("/configs/plugins/%s/plugin.yaml", name), + // Dispatch order mirrors Install/Uninstall (plugins_install.go): a local + // Docker container wins; otherwise fall back to the SaaS EIC path. Without + // the SaaS branch ListInstalled returned [] for EVERY SaaS tenant (no LOCAL + // container), so an installed plugin read back as not-installed even though + // it was on the box — the "[] readback after a successful install" bug. + if containerName := h.findRunningContainer(ctx, workspaceID); containerName != "" { + // List directories in /configs/plugins/ + output, err := h.execInContainer(ctx, containerName, []string{ + "sh", "-c", "ls -1 /configs/plugins/ 2>/dev/null || true", }) - if err != nil || manifestOutput == "" { - plugins = append(plugins, pluginInfo{Name: name}) - continue + if err != nil { + c.JSON(http.StatusOK, plugins) + return } - info := parseManifestYAML(name, []byte(manifestOutput)) - plugins = append(plugins, info) - } - - // Annotate each installed plugin with whether it still supports the - // workspace's current runtime. Lets the canvas grey out plugins that - // went inert after a runtime change. - if h.runtimeLookup != nil { - if runtime, err := h.runtimeLookup(workspaceID); err == nil && runtime != "" { - for i := range plugins { - ok := plugins[i].supportsRuntime(runtime) - plugins[i].SupportedOnRuntime = &ok + for _, name := range strings.Split(output, "\n") { + name = strings.TrimSpace(name) + if name == "" || validatePluginName(name) != nil { + continue } + // Try to read manifest from container (safe: name is validated) + manifestOutput, err := h.execInContainer(ctx, containerName, []string{ + "cat", fmt.Sprintf("/configs/plugins/%s/plugin.yaml", name), + }) + if err != nil || manifestOutput == "" { + plugins = append(plugins, pluginInfo{Name: name}) + continue + } + info := parseManifestYAML(name, []byte(manifestOutput)) + plugins = append(plugins, info) } + h.annotateRuntimeSupport(workspaceID, plugins) + c.JSON(http.StatusOK, plugins) + return } + // SaaS path: list + read manifests over the EIC SSH tunnel. + if instanceID, runtime := h.lookupSaaSDispatch(workspaceID); instanceID != "" { + names, err := listPluginsViaEIC(ctx, instanceID, runtime) + if err != nil { + // Couldn't reach the box — return [] (not a 5xx) to match the + // local path's fail-soft posture; the canvas treats an empty list + // as "none installed / try again", never a hard error. + log.Printf("ListInstalled: EIC list failed for %s: %v", workspaceID, err) + c.JSON(http.StatusOK, plugins) + return + } + for _, name := range names { + if validatePluginName(name) != nil { + continue + } + manifest, mErr := readPluginManifestViaEIC(ctx, instanceID, runtime, name) + if mErr != nil || len(manifest) == 0 { + plugins = append(plugins, pluginInfo{Name: name}) + continue + } + plugins = append(plugins, parseManifestYAML(name, manifest)) + } + h.annotateRuntimeSupport(workspaceID, plugins) + c.JSON(http.StatusOK, plugins) + return + } + + // Neither backend reachable — empty list (fail-soft, same as before). c.JSON(http.StatusOK, plugins) } +// annotateRuntimeSupport stamps each plugin with whether it still supports the +// workspace's current runtime. Lets the canvas grey out plugins that went inert +// after a runtime change. Shared by the Docker and SaaS ListInstalled branches. +func (h *PluginsHandler) annotateRuntimeSupport(workspaceID string, plugins []pluginInfo) { + if h.runtimeLookup == nil { + return + } + runtime, err := h.runtimeLookup(workspaceID) + if err != nil || runtime == "" { + return + } + for i := range plugins { + ok := plugins[i].supportsRuntime(runtime) + plugins[i].SupportedOnRuntime = &ok + } +} + // CheckRuntimeCompatibility handles GET /workspaces/:id/plugins/compatibility?runtime= // — preflight for runtime changes. Reports which installed plugins would // become inert if the workspace switched to . Canvas uses this diff --git a/workspace-server/internal/handlers/plugins_listing_saas_test.go b/workspace-server/internal/handlers/plugins_listing_saas_test.go new file mode 100644 index 000000000..846c9f2f8 --- /dev/null +++ b/workspace-server/internal/handlers/plugins_listing_saas_test.go @@ -0,0 +1,130 @@ +package handlers + +// plugins_listing_saas_test.go — ListInstalled SaaS (EIC) path. +// +// Regression: ListInstalled only ls'd a LOCAL Docker container, so every SaaS +// tenant (no local container) read back [] for GET /workspaces/:id/plugins even +// when plugins were installed on its EC2 — the "[] readback after a successful +// install" bug. These tests pin the EIC dispatch + manifest read + fail-soft. + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gin-gonic/gin" +) + +// stubListPluginsViaEIC swaps the package-level listPluginsViaEIC for the test. +func stubListPluginsViaEIC(t *testing.T, fn func(ctx context.Context, instanceID, runtime string) ([]string, error)) { + t.Helper() + prev := listPluginsViaEIC + listPluginsViaEIC = fn + t.Cleanup(func() { listPluginsViaEIC = prev }) +} + +func newSaaSListHandler() *PluginsHandler { + // docker == nil → findRunningContainer returns "" → SaaS branch is taken. + return NewPluginsHandler(t_TempDirNoop(), nil, nil). + WithRuntimeLookup(func(string) (string, error) { return "claude-code", nil }). + WithInstanceIDLookup(func(string) (string, error) { return "i-saaslist", nil }) +} + +// t_TempDirNoop avoids importing testing into the helper signature above; the +// registry path is unused on the list path. +func t_TempDirNoop() string { return "/tmp" } + +func callListInstalled(t *testing.T, h *PluginsHandler) (int, []pluginInfo) { + t.Helper() + gin.SetMode(gin.TestMode) + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "c7244ed9-f623-4cba-8873-020e5c9fe104"}} + c.Request = httptest.NewRequest(http.MethodGet, "/workspaces/x/plugins", nil) + h.ListInstalled(c) + var out []pluginInfo + _ = json.Unmarshal(w.Body.Bytes(), &out) + return w.Code, out +} + +func TestListInstalled_SaaS_ReturnsInstalledPlugins(t *testing.T) { + h := newSaaSListHandler() + stubListPluginsViaEIC(t, func(_ context.Context, instanceID, runtime string) ([]string, error) { + if instanceID != "i-saaslist" || runtime != "claude-code" { + t.Fatalf("unexpected dispatch instance=%q runtime=%q", instanceID, runtime) + } + return []string{"molecule-ai-plugin-image-gen", "molecule-ai-plugin-molecule-platform-mcp"}, nil + }) + stubReadPluginManifestViaEIC(t, func(_ context.Context, _, _, name string) ([]byte, error) { + return []byte("name: " + name + "\nversion: 0.1.0\n"), nil + }) + + code, plugins := callListInstalled(t, h) + if code != http.StatusOK { + t.Fatalf("status = %d, want 200", code) + } + if len(plugins) != 2 { + t.Fatalf("got %d plugins, want 2: %+v", len(plugins), plugins) + } + names := map[string]bool{} + for _, p := range plugins { + names[p.Name] = true + } + if !names["molecule-ai-plugin-image-gen"] { + t.Errorf("user plugin image-gen missing from listing: %+v", plugins) + } +} + +func TestListInstalled_SaaS_MissingManifestStillListsName(t *testing.T) { + h := newSaaSListHandler() + stubListPluginsViaEIC(t, func(_ context.Context, _, _ string) ([]string, error) { + return []string{"bare-plugin"}, nil + }) + stubReadPluginManifestViaEIC(t, func(_ context.Context, _, _, _ string) ([]byte, error) { + return nil, nil // no manifest + }) + + code, plugins := callListInstalled(t, h) + if code != http.StatusOK { + t.Fatalf("status = %d, want 200", code) + } + if len(plugins) != 1 || plugins[0].Name != "bare-plugin" { + t.Fatalf("got %+v, want single bare-plugin entry", plugins) + } +} + +func TestListInstalled_SaaS_ListErrorFailsSoftEmpty(t *testing.T) { + h := newSaaSListHandler() + stubListPluginsViaEIC(t, func(_ context.Context, _, _ string) ([]string, error) { + return nil, errors.New("tunnel down") + }) + + code, plugins := callListInstalled(t, h) + if code != http.StatusOK { + t.Fatalf("status = %d, want 200 (fail-soft)", code) + } + if len(plugins) != 0 { + t.Fatalf("got %d plugins, want 0 on list error", len(plugins)) + } +} + +func TestListInstalled_SaaS_RejectsInvalidPluginName(t *testing.T) { + h := newSaaSListHandler() + stubListPluginsViaEIC(t, func(_ context.Context, _, _ string) ([]string, error) { + return []string{"../etc", "good-plugin"}, nil + }) + stubReadPluginManifestViaEIC(t, func(_ context.Context, _, _, _ string) ([]byte, error) { + return nil, nil + }) + + code, plugins := callListInstalled(t, h) + if code != http.StatusOK { + t.Fatalf("status = %d, want 200", code) + } + if len(plugins) != 1 || plugins[0].Name != "good-plugin" { + t.Fatalf("traversal name should be dropped; got %+v", plugins) + } +} -- 2.52.0