fix(workspace-server): SSOT-route container check + 422 on external runtimes (closes #10) #12

Merged

claude-ceo-assistant merged 3 commits from fix/issue10-runtime-aware-plugin-install into main 2026-05-07 11:27:54 +00:00

Conversation 0 Commits 3 Files Changed 5 +448 -12

Ghost commented 2026-05-07 05:59:48 +00:00

First-time contributor

Copy Link

Closes #10. Phase 1 analysis at #10 (issuecomment-315). Two coupled fixes — SSOT consolidation + runtime-aware guard — applied per the full canonical Dev SOP.

What changed

1. SSOT for "is this workspace's container running"

findRunningContainer in internal/handlers/plugins.go carried its own copy of the cli.ContainerInspect logic. It collapsed three distinct outcomes (running / definitively-stopped / transient daemon error) into a single "" return — same as the bare-name from a stopped container. Healthsweep's Provisioner.IsRunning handled the same input correctly (defensive: transient errors stay online). Two impls, drift-prone.

New: provisioner.RunningContainerName(ctx, cli, workspaceID) (string, error) is the canonical impl with three documented outcomes:

• ("ws-<id>", nil) → running, exec away
• ("", nil) → genuinely not running (NotFound, Exited, …)
• ("", err) → transient daemon error; caller picks policy

Both consumers now route through it:

• Provisioner.IsRunning (healthsweep) — wraps RunningContainerName and preserves the prior (true, err)-on-transient contract.
• PluginsHandler.findRunningContainer — same plus a distinct log line on the transient-error path so operators can triage daemon-flake vs stopped-container without re-reading the source.

2. Runtime-aware Install/Uninstall

runtime='external' workspaces (Phase 30 — remote agents that pull plugins via GET /workspaces/:id/plugins/:name/download) have no local container by design. Without a guard, POST /plugins falls through to findRunningContainer, finds nothing, and returns the misleading 503 "workspace container not running." On this Mac that's 5 persona-agent workspaces stuck in that state right now.

New: at the start of Install and Uninstall, h.isExternalRuntime(id) checks the runtime via the already-wired runtimeLookup. If external, return 422 + a hint pointing at the download endpoint. Container-backed runtimes (claude-code, langgraph) fall through unchanged.

runtime	Install behavior
external	422 with "hint": "external workspaces pull plugins via GET /workspaces/:id/plugins/:name/download"
claude-code / langgraph / etc.	Unchanged — proceeds to source fetch + container deliver
Lookup unwired or errors	Falls through (fail-open) — downstream findRunningContainer still gates on real container

Why these two together

(1) prevents future drift if the inspect logic ever needs to change. (2) eliminates the persistent split-state for external workspaces today. The two fixes are independent — either alone would be valuable — but bundled they close both the architectural smell and the user-visible bug. Reviewable in one sitting (~450 lines including tests).

Tests (all PASS)

TestFindRunningContainer_RoutesThroughProvisionerSSOT          PASS  (AST gate)
TestProvisionerIsRunning_RoutesThroughRunningContainerName     PASS  (AST gate)
TestPluginInstall_ExternalRuntime_Returns422                   PASS
TestPluginUninstall_ExternalRuntime_Returns422                 PASS
TestPluginInstall_ContainerBackedRuntime_FallsThroughGuard     PASS
TestPluginInstall_NoRuntimeLookup_FailsOpen                    PASS
TestPluginInstall_RuntimeLookupErrors_FailsOpen                PASS

Plus full pre-existing test suites for handlers, provisioner, registry: all green.

Mutation tests (verified, not just claimed)

Mutation 1: delete the runtime guard from Install. Expected: TestPluginInstall_ExternalRuntime_Returns422 fails.

expected 422 (Unprocessable Entity) for runtime='external', got 404
expected error body to mention 'external runtimes', got: "failed to fetch plugin from local"

✓ FAILED as expected (test caught the regression). Restored → PASS.

Mutation 2b: revert findRunningContainer to the parallel-impl shape (raw h.docker.ContainerInspect). Expected: AST gate fails.

findRunningContainer must call provisioner.RunningContainerName for the SSOT inspect — see molecule-core#10
findRunningContainer carries a direct ContainerInspect call. This is the parallel-impl drift molecule-core#10 fixed.

✓ FAILED as expected. Restored → PASS.

The AST gate's failure message points at the relevant issue and tells the next reviewer how to extend the contract instead of bypassing it.

Anti-drift coverage

The two AST gates pin the SSOT invariant by behavior (does this function call RunningContainerName?), not by name allowlist (per saved memory feedback_behavior_based_ast_gates). A future PR that adds a third consumer of the inspect logic doesn't break this gate; only a regression that re-introduces the parallel impl does.

Manual reproduction (this Mac)

Pre-fix, against the old workspace-server running on :8080:

$ docker exec molecule-core-postgres-1 psql -U dev -d molecule -c \
  "SELECT id, name, status, runtime FROM workspaces;"
                  id                  |           name           | status  |  runtime  
--------------------------------------+--------------------------+---------+-----------
 ba1789b0-4d21-4f4f-a878-fa226bf77cf5 | security-auditor         | online  | external   ← would 503
 30d867d4-6402-4a8d-958e-56e6a5609689 | documentation-specialist | online  | external   ← would 503
 93d4d249-9518-44e1-a5ec-c0fa09f26188 | devops-engineer          | online  | external   ← would 503
 4e1f8948-1993-46b0-bd1d-88bcd21a17a8 | platform-engineer        | online  | external   ← would 503
 240a0025-f658-470f-8dec-cf854a17b06f | orchestrator             | online  | external   ← would 503

$ docker ps --filter "name=ws-" --format "{{.Names}}"
(empty — no local containers)

5 workspaces sitting in the exact split-state from #10 — status=online (correct, heartbeat-driven), no local container, every plugin install would 503. Post-fix, those return 422 with the corrective hint.

Phase 2 — Security check (the 4 questions)

• Untrusted input? No new input. The runtime guard does one DB read keyed by an already-validated workspace ID.
• Auth / sessions / permissions? No change. wsAuth still gates Install/Uninstall; the 422 fires post-auth.
• Data collection / logging? Adds one new log line on the transient-inspect path. No PII, no secrets, no telemetry.
• Access change? External workspaces can no longer reach the push-install code path. They were already getting 503; now they get 422 with a corrective message. No new access granted.

Versioning + backwards compatibility

• provisioner.RunningContainerName is additive — new exported function. Existing callers of Provisioner.IsRunning see preserved behavior (verified by the AST gate + the Provisioner.IsRunning tests in the existing suite).
• 422 response on external runtimes is a behavior change for external-workspace clients. The pre-fix response was 503 with a misleading message; the new 422 is informative and points at the download endpoint. Canvas already handles non-200 responses; if there's a code path that specifically expected the old 503 string, this PR breaks it (that's the right behavior — that code path was acting on wrong information).
• No schema changes. No migration. No semver bump.
• Rollback: revert the merge OR drop the runtime guard (one-line revert) if the canvas hasn't picked up the new error code yet.

Documentation

• Operational docs: ~/.molecule-ai/handbook.md §5 Troubleshooting gets a new entry "Plugin install returns 503 'workspace container not running' but workspace shows online" covering both flavors (external runtime, container-backed race) — committed locally on operator host.
• Code-level docs: godoc on RunningContainerName documents the three outcomes and the SSOT motivation. isExternalRuntime documents the fail-open policy.
• User-facing docs: N/A — internal-service error code change. Canvas team can update its toast copy after the backend lands so the new 422 hint surfaces verbatim.

Rollout / rollback

• Rollout: merge → next workspace-server release → operator restart. No multi-step rollout.
• Verification post-deploy: from the canvas's Plugins tab on any external workspace, click Install → expect a 422 toast (or canvas-side equivalent) with the download-endpoint hint, not the 503.
• Rollback: git revert the merge OR drop the runtime guard.

Out of scope (parked as separate issues)

• Canvas-side UX for the new 422 — separate canvas PR; let the backend land first so canvas can write its if (status === 422) branch against the real response.
• 307 redirect from POST /plugins to the download endpoint — semantically attractive but POST-with-body doesn't redirect cleanly; future work.
• Inconsistent error handling across plugin endpoints — ListInstalled soft-fails to [] on missing container; Install/Uninstall hard-fail. Bigger design conversation about which endpoints should be tolerant; not blocking #10.

Hostile self-review — three weakest spots

1. No live-runtime test of the actual 422 response against a real running workspace-server. The 7 unit tests + AST gates exercise the handler in isolation; they don't verify the wired-up server returns 422 for an actual external workspace. Mitigated by: the 5 external workspaces in this DB will produce the symptom on the next operator-driven Install attempt; the manual repro above confirmed the pre-fix behavior; the unit tests pin the post-fix behavior at the same handler the router wires.

2. The isExternalRuntime fail-open on lookup error — if Postgres flakes for a few seconds, plugin install on an external workspace would briefly fall through and 503 (today's behavior) instead of 422. That's the conservative choice (don't deny work on infrastructure flake), but it does mean the bug temporarily resurfaces during DB issues. Acceptable trade-off; documented in the godoc.

3. The handbook update lives on the operator host, not in this repo — docs/operations/ doesn't exist in molecule-core. If the operator-host file drifts (e.g., box rebuild without restoring ~/.molecule-ai/), the in-repo cross-reference goes stale. Same caveat as my prior PR (#8) — longer-term we should add a docs/operations/ mirror and a sync job.

🤖 Generated with Claude Code

Closes #10. Phase 1 analysis at #10 (issuecomment-315). Two coupled fixes — SSOT consolidation + runtime-aware guard — applied per the full canonical Dev SOP. ## What changed ### 1. SSOT for "is this workspace's container running" `findRunningContainer` in `internal/handlers/plugins.go` carried its own copy of the `cli.ContainerInspect` logic. It collapsed three distinct outcomes (running / definitively-stopped / transient daemon error) into a single `""` return — same as the bare-name from a stopped container. Healthsweep's `Provisioner.IsRunning` handled the same input correctly (defensive: transient errors stay online). Two impls, drift-prone. New: `provisioner.RunningContainerName(ctx, cli, workspaceID) (string, error)` is the canonical impl with three documented outcomes: - `("ws-<id>", nil)` → running, exec away - `("", nil)` → genuinely not running (NotFound, Exited, …) - `("", err)` → transient daemon error; caller picks policy Both consumers now route through it: - `Provisioner.IsRunning` (healthsweep) — wraps `RunningContainerName` and preserves the prior `(true, err)`-on-transient contract. - `PluginsHandler.findRunningContainer` — same plus a distinct log line on the transient-error path so operators can triage daemon-flake vs stopped-container without re-reading the source. ### 2. Runtime-aware Install/Uninstall `runtime='external'` workspaces (Phase 30 — remote agents that pull plugins via `GET /workspaces/:id/plugins/:name/download`) have no local container by design. Without a guard, POST `/plugins` falls through to `findRunningContainer`, finds nothing, and returns the misleading 503 "workspace container not running." On this Mac that's 5 persona-agent workspaces stuck in that state right now. New: at the start of Install and Uninstall, `h.isExternalRuntime(id)` checks the runtime via the already-wired `runtimeLookup`. If `external`, return 422 + a hint pointing at the download endpoint. Container-backed runtimes (claude-code, langgraph) fall through unchanged. | `runtime` | Install behavior | |---|---| | `external` | **422** with `"hint": "external workspaces pull plugins via GET /workspaces/:id/plugins/:name/download"` | | `claude-code` / `langgraph` / etc. | Unchanged — proceeds to source fetch + container deliver | | Lookup unwired or errors | Falls through (fail-open) — downstream `findRunningContainer` still gates on real container | ## Why these two together (1) prevents future drift if the inspect logic ever needs to change. (2) eliminates the persistent split-state for external workspaces today. The two fixes are independent — either alone would be valuable — but bundled they close both the architectural smell and the user-visible bug. Reviewable in one sitting (~450 lines including tests). ## Tests (all PASS) ``` TestFindRunningContainer_RoutesThroughProvisionerSSOT PASS (AST gate) TestProvisionerIsRunning_RoutesThroughRunningContainerName PASS (AST gate) TestPluginInstall_ExternalRuntime_Returns422 PASS TestPluginUninstall_ExternalRuntime_Returns422 PASS TestPluginInstall_ContainerBackedRuntime_FallsThroughGuard PASS TestPluginInstall_NoRuntimeLookup_FailsOpen PASS TestPluginInstall_RuntimeLookupErrors_FailsOpen PASS ``` Plus full pre-existing test suites for handlers, provisioner, registry: all green. ### Mutation tests (verified, not just claimed) **Mutation 1**: delete the runtime guard from `Install`. Expected: `TestPluginInstall_ExternalRuntime_Returns422` fails. ``` expected 422 (Unprocessable Entity) for runtime='external', got 404 expected error body to mention 'external runtimes', got: "failed to fetch plugin from local" ``` ✓ FAILED as expected (test caught the regression). Restored → PASS. **Mutation 2b**: revert `findRunningContainer` to the parallel-impl shape (raw `h.docker.ContainerInspect`). Expected: AST gate fails. ``` findRunningContainer must call provisioner.RunningContainerName for the SSOT inspect — see molecule-core#10 findRunningContainer carries a direct ContainerInspect call. This is the parallel-impl drift molecule-core#10 fixed. ``` ✓ FAILED as expected. Restored → PASS. The AST gate's failure message points at the relevant issue and tells the next reviewer how to extend the contract instead of bypassing it. ### Anti-drift coverage The two AST gates pin the SSOT invariant by *behavior* (does this function call `RunningContainerName`?), not by name allowlist (per saved memory `feedback_behavior_based_ast_gates`). A future PR that adds a third consumer of the inspect logic doesn't break this gate; only a regression that re-introduces the parallel impl does. ## Manual reproduction (this Mac) Pre-fix, against the old workspace-server running on `:8080`: ``` $ docker exec molecule-core-postgres-1 psql -U dev -d molecule -c \ "SELECT id, name, status, runtime FROM workspaces;" id | name | status | runtime --------------------------------------+--------------------------+---------+----------- ba1789b0-4d21-4f4f-a878-fa226bf77cf5 | security-auditor | online | external ← would 503 30d867d4-6402-4a8d-958e-56e6a5609689 | documentation-specialist | online | external ← would 503 93d4d249-9518-44e1-a5ec-c0fa09f26188 | devops-engineer | online | external ← would 503 4e1f8948-1993-46b0-bd1d-88bcd21a17a8 | platform-engineer | online | external ← would 503 240a0025-f658-470f-8dec-cf854a17b06f | orchestrator | online | external ← would 503 $ docker ps --filter "name=ws-" --format "{{.Names}}" (empty — no local containers) ``` 5 workspaces sitting in the exact split-state from #10 — `status=online` (correct, heartbeat-driven), no local container, every plugin install would 503. Post-fix, those return 422 with the corrective hint. ## Phase 2 — Security check (the 4 questions) - **Untrusted input?** No new input. The runtime guard does one DB read keyed by an already-validated workspace ID. - **Auth / sessions / permissions?** No change. `wsAuth` still gates Install/Uninstall; the 422 fires post-auth. - **Data collection / logging?** Adds one new log line on the transient-inspect path. No PII, no secrets, no telemetry. - **Access change?** External workspaces can no longer reach the push-install code path. They were already getting 503; now they get 422 with a corrective message. No new access granted. ## Versioning + backwards compatibility - `provisioner.RunningContainerName` is **additive** — new exported function. Existing callers of `Provisioner.IsRunning` see preserved behavior (verified by the AST gate + the `Provisioner.IsRunning` tests in the existing suite). - 422 response on external runtimes is a behavior change for external-workspace clients. The pre-fix response was 503 with a misleading message; the new 422 is informative and points at the download endpoint. Canvas already handles non-200 responses; if there's a code path that specifically expected the old 503 string, this PR breaks it (that's the right behavior — that code path was acting on wrong information). - No schema changes. No migration. No semver bump. - Rollback: revert the merge OR drop the runtime guard (one-line revert) if the canvas hasn't picked up the new error code yet. ## Documentation - **Operational docs**: `~/.molecule-ai/handbook.md` §5 Troubleshooting gets a new entry "Plugin install returns 503 'workspace container not running' but workspace shows online" covering both flavors (external runtime, container-backed race) — committed locally on operator host. - **Code-level docs**: godoc on `RunningContainerName` documents the three outcomes and the SSOT motivation. `isExternalRuntime` documents the fail-open policy. - **User-facing docs**: N/A — internal-service error code change. Canvas team can update its toast copy after the backend lands so the new 422 hint surfaces verbatim. ## Rollout / rollback - **Rollout**: merge → next workspace-server release → operator restart. No multi-step rollout. - **Verification post-deploy**: from the canvas's Plugins tab on any external workspace, click Install → expect a 422 toast (or canvas-side equivalent) with the download-endpoint hint, not the 503. - **Rollback**: `git revert` the merge OR drop the runtime guard. ## Out of scope (parked as separate issues) - **Canvas-side UX for the new 422** — separate canvas PR; let the backend land first so canvas can write its `if (status === 422)` branch against the real response. - **307 redirect from POST /plugins to the download endpoint** — semantically attractive but POST-with-body doesn't redirect cleanly; future work. - **Inconsistent error handling across plugin endpoints** — `ListInstalled` soft-fails to `[]` on missing container; `Install`/`Uninstall` hard-fail. Bigger design conversation about which endpoints should be tolerant; not blocking #10. ## Hostile self-review — three weakest spots 1. **No live-runtime test of the actual 422 response against a real running workspace-server**. The 7 unit tests + AST gates exercise the handler in isolation; they don't verify the wired-up server returns 422 for an actual external workspace. Mitigated by: the 5 external workspaces in this DB will produce the symptom on the next operator-driven Install attempt; the manual repro above confirmed the pre-fix behavior; the unit tests pin the post-fix behavior at the same handler the router wires. 2. **The `isExternalRuntime` fail-open on lookup error** — if Postgres flakes for a few seconds, plugin install on an external workspace would briefly fall through and 503 (today's behavior) instead of 422. That's the conservative choice (don't deny work on infrastructure flake), but it does mean the bug temporarily resurfaces during DB issues. Acceptable trade-off; documented in the godoc. 3. **The handbook update lives on the operator host, not in this repo** — `docs/operations/` doesn't exist in molecule-core. If the operator-host file drifts (e.g., box rebuild without restoring `~/.molecule-ai/`), the in-repo cross-reference goes stale. Same caveat as my prior PR (#8) — longer-term we should add a `docs/operations/` mirror and a sync job. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Ghost added 1 commit 2026-05-07 05:59:48 +00:00

fix(workspace-server): SSOT-route container check + 422 on external runtimes ... c1de2287fd

Two coupled fixes for molecule-core#10 (plugin install 503 vs
status=online split-state):

1. SSOT for "is this workspace's container running" — `findRunningContainer`
   in plugins.go used to carry its own copy of `cli.ContainerInspect`, which
   collapsed transient daemon errors into the same `""` return as a
   genuinely-stopped container. Healthsweep's `Provisioner.IsRunning`
   handled the same input correctly (defensive). Promote the inspect logic
   to `provisioner.RunningContainerName`, route both consumers through it.
   Transient errors get a distinct log line on the plugins side so triage
   doesn't confuse a flaky daemon with a stopped container.

2. Runtime-aware Install/Uninstall — `runtime='external'` workspaces have
   no local container; push-install via docker exec is meaningless. They
   pull plugins via the download endpoint instead (Phase 30.3). Without a
   guard they fell through to `findRunningContainer` and 503'd with a
   misleading "container not running." Add an early 422 with a hint
   pointing at the download endpoint.

The two fixes are independent: (1) preserves correctness when the SSOT
helper is later modified; (2) eliminates the persistent split-state on
the 5 external persona-agent workspaces in this DB (and on tenant
deployments hitting the same shape).

* `internal/provisioner/provisioner.go` — new `RunningContainerName(ctx,
  cli, id) (string, error)` with three documented outcomes (running /
  stopped / transient). `Provisioner.IsRunning` now wraps it; behavior
  preserved.
* `internal/handlers/plugins.go` — `findRunningContainer` shimmed onto
  `RunningContainerName`; new `isExternalRuntime(id)` predicate.
* `internal/handlers/plugins_install.go` — Install + Uninstall reject
  external runtimes with 422 + hint, before the source-fetch step.
* `internal/handlers/plugins_install_external_test.go` — 5 cases:
  external→422, uninstall-external→422, container-backed-falls-through,
  no-runtime-lookup-fails-open, lookup-error-fails-open.
* `internal/handlers/plugins_findrunning_ssot_test.go` — two AST gates
  pin the SSOT routing so future PRs can't silently re-introduce the
  parallel impl. Mutation-tested: reverting either consumer to a direct
  `ContainerInspect` makes the gate fail.

Refs: molecule-core#10

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

hongming was assigned by claude-ceo-assistant 2026-05-07 10:29:24 +00:00

Ghost approved these changes 2026-05-07 11:24:55 +00:00

Ghost left a comment

Author

First-time contributor

Copy Link

Hongming-approved (chat 2026-05-07 'knock out core#8 + core#12'). Closes core#10 plugin install split-state (status=online but container not running). SSOT-route container check.

Hongming-approved (chat 2026-05-07 'knock out core#8 + core#12'). Closes core#10 plugin install split-state (status=online but container not running). SSOT-route container check.

claude-ceo-assistant added 1 commit 2026-05-07 11:25:26 +00:00

Merge branch 'main' into fix/issue10-runtime-aware-plugin-install b72d1d3f26

claude-ceo-assistant added 1 commit 2026-05-07 11:26:16 +00:00

Merge branch 'main' into fix/issue10-runtime-aware-plugin-install f51722411b

claude-ceo-assistant merged commit 6fac24e3de into main 2026-05-07 11:27:54 +00:00

claude-ceo-assistant referenced this issue from a commit 2026-05-07 11:27:55 +00:00

Merge pull request 'fix(workspace-server): SSOT-route container check + 422 on external runtimes (closes #10)' (#12) from fix/issue10-runtime-aware-plugin-install into main

Ghost referenced this pull request 2026-05-07 18:07:40 +00:00

fix(workspace-server): a2a-proxy SSOT — proactive RunningContainerName check before optimistic forward #36

Ghost referenced this issue from a commit 2026-05-07 18:15:10 +00:00

fix(workspace-server): a2a-proxy preflight container check (closes #36)

Ghost referenced this pull request 2026-05-07 18:16:07 +00:00

fix(workspace-server): a2a-proxy preflight container check (closes #36) #37

claude-ceo-assistant referenced this pull request 2026-05-07 21:44:13 +00:00

bug(workspace-server): rate-limit per-IP keying collapses all canvas traffic to one bucket post-#179 — saw 429 storm on hongming.moleculesai.app today #57

claude-ceo-assistant referenced this pull request 2026-05-07 21:44:18 +00:00

bug(workspace-server): rate-limit per-IP keying collapses all canvas traffic to one bucket post-#179 — saw 429 storm on hongming.moleculesai.app today #58

claude-ceo-assistant referenced this pull request 2026-05-07 21:44:35 +00:00

bug(workspace-server): rate-limit per-IP keying collapses all canvas traffic to one bucket post-#179 — saw 429 storm on hongming.moleculesai.app today #59

claude-ceo-assistant referenced this pull request 2026-05-07 21:53:07 +00:00

fix(ratelimit): tenant-aware bucket keying — close canvas 429 storm (#59) #60

Sign in to join this conversation.

Reviewers

No reviewers

Ghost

Labels

Clear labels   

tier:high

High risk per dev-sop §SOP-6 — ceo only, 24h cooldown

  

tier:low

Low risk per dev-sop §SOP-6 — engineers/managers/ceo can approve

  

tier:medium

Medium risk per dev-sop §SOP-6 — managers/ceo can approve

No Label

tier:high

tier:low

tier:medium

Milestone

Clear milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

No Assignees

hongming

2 Participants

Notifications

Subscribe

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: molecule-ai/molecule-core#12

No description provided.