fix: resolve SourceResolver naming conflict, SSRF guard placement, and multiple test regressions

- plugins/drift_sweeper.go: rename SourceResolver→PluginResolver to avoid
  redeclaring the interface already defined in source.go (core#228)

- handlers/workspace.go: move SSRF guard before BeginTx so URL rejection
  never touches the DB (core#212 fix — same pattern as registry.go:324)

- handlers/restart_signals.go: convert rewriteForDocker standalone function
  to a method on *WorkspaceHandler; fix two call sites to use h.rewriteForDocker

- handlers/plugins.go: change Sources() return type from plugins.SourceResolver
  to pluginSources (the narrow interface satisfied by *Registry)

- handlers/admin_plugin_drift.go: remove unused "context" import

- handlers/delegation_test.go: remove stray closing brace

- handlers/restart_signals_test.go: rewrite with correct miniredis v2 API
  (mr.Get takes context, mr.Set requires TTL), resolveURLTestWrapper embedding
  pattern, and corrected Redis key handling

- handlers/workspace_test.go: use http://localhost:8000 for SSRF-safe test
  (no DNS required); remove spurious mock.ExpectExec for Redis CacheURL call

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

.gitea/scripts/sop-tier-check.sh

@@ -285,12 +285,26 @@ _passed_clauses=""
 _failed_clauses=""
 
 for _raw_clause in $EXPR; do
-  # Normalise: strip parens, split on comma, trim whitespace.
-  _clause=$(echo "$_raw_clause" | tr -d '()' | tr ',' '\n' | tr -d '[:space:]' | grep -v '^$')
+  # Normalise: strip parens, replace commas with spaces so bash word-split
+  # can iterate the OR-set members. The previous form
+  #   _clause=$(echo ... | tr ',' '\n' | tr -d '[:space:]' | grep -v '^$')
+  # collapsed every member into one concatenated token because
+  # `tr -d '[:space:]'` strips the very newlines that just separated them
+  # ("engineers,managers,ceo" -> "engineersmanagersceo"), so the OR-clause
+  # only ever evaluated as a single nonsense team name and never matched
+  # APPROVER_TEAMS. Fixed in #229: leave the comma-separated members as
+  # space-separated tokens for `for _t in $_clause`.
+  _no_parens=${_raw_clause//[()]/}
+  _clause=${_no_parens//,/ }
   _clause_passed="no"
   _clause_names=""
   for _t in $_clause; do
-    _clause_names="${_clause_names:+, }${_t}"
+    # Append (don't overwrite) team name to the human-readable accumulator.
+    # The previous form `_clause_names="${_clause_names:+, }${_t}"`
+    # rewrote the variable on every iteration, so the FAIL message only
+    # ever showed the LAST team. Fixed: prepend prior value before the
+    # comma-separator, then append the new team name.
+    _clause_names="${_clause_names}${_clause_names:+, }${_t}"
     # Skip teams not yet in Gitea (qa??? / security??? placeholders).
     [[ "$_t" == *"???" ]] && debug "clause \"$_t\": skipped (team pending creation)" && continue
     [ -z "${TEAM_ID[$_t]:-}" ] && debug "clause \"$_t\": no ID resolved, skipping" && continue
@@ -311,11 +325,12 @@ for _raw_clause in $EXPR; do
   _label=$(echo "$_raw_clause" | tr -d '()' | tr ',' '/' | tr -d '[:space:]' | sed 's/???//g')
 
   if [ "$_clause_passed" = "yes" ]; then
-    _passed_clauses="${_passed_clauses:+, }$_label"
+    # Append (don't overwrite) — same accumulator bug as _clause_names above.
+    _passed_clauses="${_passed_clauses}${_passed_clauses:+, }$_label"
     echo "::notice::clause [$_label]: PASS — satisfied by approving reviewer(s)"
   else
-    _failed_clauses="${_failed_clauses:+, }$_label"
-    echo "::error::clause [$_label]: FAIL — no approving reviewer belongs to any of these teams${_clause_names}. Set SOP_DEBUG=1 to see per-team probe results."
+    _failed_clauses="${_failed_clauses}${_failed_clauses:+, }$_label"
+    echo "::error::clause [$_label]: FAIL — no approving reviewer belongs to any of these teams (${_clause_names}). Set SOP_DEBUG=1 to see per-team probe results."
   fi
 done
 

.gitea/scripts/tests/test_sop_tier_check_clause_split.sh

@@ -0,0 +1,101 @@
+#!/usr/bin/env bash
+# Regression test for #229 — sop-tier-check tier:low OR-clause splitter.
+#
+# Bug (PR #225 → still broken after PR #231):
+#   Line ~289 of sop-tier-check.sh used:
+#     _clause=$(echo "$_raw_clause" | tr -d '()' | tr ',' '\n' | tr -d '[:space:]' | grep -v '^$')
+#   `tr -d '[:space:]'` strips the newlines that `tr ',' '\n'` just
+#   inserted, collapsing "engineers,managers,ceo" into a single token
+#   "engineersmanagersceo". The for-loop then iterates ONCE on a name
+#   that matches no team, so every tier:low PR fails:
+#     ::error::clause [engineers/managers/ceo]: FAIL — no approving
+#     reviewer belongs to any of these teamsengineersmanagersceo
+#   (note also: missing separators in the error string is bug #2 —
+#    `_clause_names` used "${var:+, }$x" which OVERWRITES per iteration).
+#
+# Fix shape (this PR):
+#   _no_parens=${_raw_clause//[()]/}
+#   _clause=${_no_parens//,/ }    # comma -> space, bash word-split iterates
+#   _clause_names="${_clause_names}${_clause_names:+, }${_t}"  # APPEND, not overwrite
+#
+# This test extracts the splitter logic and asserts it produces the right
+# token list for each of the three tier expressions live in the script.
+
+set -euo pipefail
+
+PASS=0
+FAIL=0
+
+assert_eq() {
+  local label="$1"
+  local expected="$2"
+  local got="$3"
+  if [ "$expected" = "$got" ]; then
+    echo "  PASS  $label"
+    PASS=$((PASS + 1))
+  else
+    echo "  FAIL  $label"
+    echo "        expected: <$expected>"
+    echo "        got:      <$got>"
+    FAIL=$((FAIL + 1))
+  fi
+}
+
+# ----- Splitter under test (mirrors the fixed sop-tier-check.sh block) -----
+split_clause() {
+  local raw="$1"
+  local no_parens=${raw//[()]/}
+  local clause=${no_parens//,/ }
+  local out=""
+  for _t in $clause; do
+    out="${out}${out:+|}$_t"
+  done
+  echo "$out"
+}
+
+echo "test: tier:low OR-clause splits to 3 tokens"
+assert_eq "tier:low" "engineers|managers|ceo" "$(split_clause "engineers,managers,ceo")"
+
+echo "test: tier:medium AND-expression — bash word-split on \$EXPR yields 5 tokens"
+EXPR="managers AND engineers AND qa???,security???"
+out=""
+for _raw in $EXPR; do
+  out="${out}${out:+ ; }$(split_clause "$_raw")"
+done
+assert_eq "tier:medium" "managers ; AND ; engineers ; AND ; qa???|security???" "$out"
+
+echo "test: tier:high single-team OR-clause"
+assert_eq "tier:high" "ceo" "$(split_clause "ceo")"
+
+echo "test: paren-wrapped OR-set unwraps + splits"
+assert_eq "paren OR" "managers|ceo" "$(split_clause "(managers,ceo)")"
+
+# ----- _clause_names accumulator (was overwriting per iteration) -----
+acc=""
+for t in engineers managers ceo; do
+  acc="${acc}${acc:+, }${t}"
+done
+assert_eq "_clause_names append" "engineers, managers, ceo" "$acc"
+
+# ----- _failed_clauses / _passed_clauses accumulator across raw clauses -----
+acc=""
+for c in clauseA clauseB clauseC; do
+  acc="${acc}${acc:+, }${c}"
+done
+assert_eq "_failed_clauses append" "clauseA, clauseB, clauseC" "$acc"
+
+# ----- End-to-end OR-gate: simulate APPROVER_TEAMS[core-lead]=' managers ' -----
+# The script's case pattern is *${_t}* with a space-padded value.
+APPROVER_TEAMS_VAL=" managers "
+matched=""
+for _t in $(split_clause "engineers,managers,ceo" | tr '|' ' '); do
+  case "$APPROVER_TEAMS_VAL" in
+    *${_t}*) matched="$_t"; break ;;
+  esac
+done
+assert_eq "OR-gate matches managers" "managers" "$matched"
+
+echo
+echo "------"
+echo "PASS=$PASS FAIL=$FAIL"
+[ "$FAIL" -eq 0 ]

canvas/src/components/Toolbar.tsx

@@ -317,7 +317,7 @@ export function Toolbar() {
           onClick={() => setHelpOpen((open) => !open)}
           className="flex items-center justify-center w-7 h-7 bg-surface-card hover:bg-surface-card/70 border border-line rounded-lg transition-colors text-ink-mid hover:text-ink focus:outline-none focus-visible:ring-2 focus-visible:ring-accent/40"
           aria-expanded={helpOpen}
-          aria-label="Open quick help"
+          aria-label="Open shortcuts and tips"
           title="Help — shortcuts & quick start"
         >
           <svg width="14" height="14" viewBox="0 0 16 16" fill="none" aria-hidden="true">
@@ -327,24 +327,35 @@ export function Toolbar() {
         </button>
 
         {helpOpen && (
-          <div className="absolute right-0 top-full mt-2 w-72 rounded-xl border border-line/60 bg-surface/95 p-3 shadow-2xl shadow-black/50 backdrop-blur-md">
-            <div className="mb-2 flex items-center justify-between">
-              <span className="text-[10px] font-semibold uppercase tracking-[0.24em] text-ink-mid">Quick start</span>
+          <div
+            role="dialog"
+            aria-label="Shortcuts and tips"
+            aria-modal="false"
+            className="absolute right-0 top-full mt-2 w-80 rounded-xl border border-line/60 bg-surface/95 p-3 shadow-2xl shadow-black/50 backdrop-blur-md z-50"
+          >
+            <div className="mb-3 flex items-center justify-between">
+              <span className="text-[10px] font-semibold uppercase tracking-[0.24em] text-ink-mid">Shortcuts & tips</span>
               <button
                 type="button"
                 onClick={() => setHelpOpen(false)}
+                aria-label="Close help dialog"
                 className="text-[10px] text-ink-mid hover:text-ink transition-colors focus:outline-none focus-visible:underline"
               >
                 Close
               </button>
             </div>
-            <div className="space-y-2">
+            <div className="space-y-1.5">
               <HelpRow shortcut="⌘K" text="Search workspaces and jump straight into Details or Chat." />
+              <HelpRow shortcut="Esc" text="Clear selection, close menus, dismiss dialogs." />
+              <HelpRow shortcut="Enter" text="Zoom into selected team and select its first child node." />
+              <HelpRow shortcut="Shift+Enter" text="Select the parent of the selected node." />
+              <HelpRow shortcut="⌘]" text="Bring selected node forward in the z-order." />
+              <HelpRow shortcut="⌘[" text="Send selected node backward in the z-order." />
+              <HelpRow shortcut="Z" text="Zoom canvas to fit a team node and all its sub-workspaces." />
               <HelpRow shortcut="Palette" text="Open the template palette to deploy a new workspace." />
               <HelpRow shortcut="Right-click" text="Use node actions for duplicate, export, restart, or delete." />
-              <HelpRow shortcut="Chat" text="If a task is still running, the chat tab resumes that session automatically." />
-              <HelpRow shortcut="Config" text="Use the Config tab for skills, model, secrets, and runtime settings." />
-              <HelpRow shortcut="Dbl-click / Z" text="Zoom canvas to fit a team node and all its sub-workspaces." />
+              <HelpRow shortcut="Dbl-click" text="On a team node: expand and zoom to show all sub-workspaces." />
+              <HelpRow shortcut="Shift+click" text="Multi-select: add or remove a node from the batch selection." />
             </div>
             {/* Link to the full keyboard shortcuts dialog */}
             <button

canvas/src/lib/__tests__/statusDotClass.test.ts

@@ -4,7 +4,7 @@
  * CSS tailwind class used on the status indicator dot.
  */
 import { describe, it, expect } from "vitest";
-import { statusDotClass } from "../design-tokens";
+import { statusDotClass, TIER_CONFIG, COMM_TYPE_LABELS } from "../design-tokens";
 
 describe("statusDotClass", () => {
   it('returns "bg-emerald-400" for "online"', () => {
@@ -50,3 +50,57 @@ describe("statusDotClass", () => {
     }
   });
 });
+
+// ── TIER_CONFIG ────────────────────────────────────────────────────────────────
+
+describe("TIER_CONFIG", () => {
+  it("has entries for all four tier levels", () => {
+    expect(TIER_CONFIG).toHaveProperty(1);
+    expect(TIER_CONFIG).toHaveProperty(2);
+    expect(TIER_CONFIG).toHaveProperty(3);
+    expect(TIER_CONFIG).toHaveProperty(4);
+  });
+
+  it("each tier has label, color, and border fields", () => {
+    for (const tier of [1, 2, 3, 4]) {
+      expect(TIER_CONFIG[tier]).toHaveProperty("label");
+      expect(TIER_CONFIG[tier]).toHaveProperty("color");
+      expect(TIER_CONFIG[tier]).toHaveProperty("border");
+    }
+  });
+
+  it("tier labels match expected values", () => {
+    expect(TIER_CONFIG[1].label).toBe("T1");
+    expect(TIER_CONFIG[2].label).toBe("T2");
+    expect(TIER_CONFIG[3].label).toBe("T3");
+    expect(TIER_CONFIG[4].label).toBe("T4");
+  });
+
+  it("is immutable at runtime — same key always returns same shape", () => {
+    const result = TIER_CONFIG[2];
+    expect(TIER_CONFIG[2]).toBe(result);
+  });
+});
+
+// ── COMM_TYPE_LABELS ────────────────────────────────────────────────────────
+
+describe("COMM_TYPE_LABELS", () => {
+  it("has labels for all known communication types", () => {
+    expect(COMM_TYPE_LABELS).toHaveProperty("a2a_send");
+    expect(COMM_TYPE_LABELS).toHaveProperty("a2a_receive");
+    expect(COMM_TYPE_LABELS).toHaveProperty("task_update");
+  });
+
+  it("labels are non-empty strings", () => {
+    for (const key of Object.keys(COMM_TYPE_LABELS)) {
+      expect(typeof COMM_TYPE_LABELS[key]).toBe("string");
+      expect(COMM_TYPE_LABELS[key].length).toBeGreaterThan(0);
+    }
+  });
+
+  it("is a static map — same key always returns same label", () => {
+    expect(COMM_TYPE_LABELS["a2a_send"]).toBe("sent");
+    expect(COMM_TYPE_LABELS["a2a_receive"]).toBe("received");
+    expect(COMM_TYPE_LABELS["task_update"]).toBe("task update");
+  });
+});

docs/design-system/canvas-audit-items.md

@@ -88,6 +88,7 @@ PR: `fix/ink-soft-wcag-contrast`.
 - Arrow keys move selected node 10px (50px with Shift) — keyboard node drag (PR #182) ✅
 - `Cmd/Ctrl+Arrow` resize selected node (↑↓ height, ←→ width, 10px, Shift 2px) ✅
 - Hierarchy navigation (Enter/Shift+Enter), z-order (Cmd+]/[), zoom-to-team (Z) ✅
+- Toolbar help dialog ("Shortcuts & tips") documents all shortcuts + mouse interactions ✅
 
 ### Focus Management ✅ (strong)
 - Skip link → `#canvas-main` ✅

workspace-server/internal/handlers/admin_plugin_drift.go

@@ -8,7 +8,6 @@ package handlers
 //   POST /admin/plugin-updates/:id/apply — apply a queued drift update
 
 import (
-	"context"
 	"database/sql"
 	"errors"
 	"fmt"

workspace-server/internal/handlers/delegation_test.go

@@ -1262,4 +1262,3 @@ func TestExecuteDelegation_CleanProxyResponse_Unchanged(t *testing.T) {
 		t.Errorf("unmet sqlmock expectations: %v", err)
 	}
 }
-}

workspace-server/internal/handlers/plugins.go

@@ -112,7 +112,10 @@ func (h *PluginsHandler) WithInstanceIDLookup(lookup InstanceIDLookup) *PluginsH
 
 // Sources returns the underlying plugin source registry. Used by main.go to
 // pass the same registry to the drift sweeper so both share resolver state.
-func (h *PluginsHandler) Sources() plugins.SourceResolver {
+// Returns the narrow pluginSources interface so callers receive only the
+// methods they need (Register, Resolve, Schemes), not the full SourceResolver
+// contract with Fetch.
+func (h *PluginsHandler) Sources() pluginSources {
 	return h.sources
 }
 

workspace-server/internal/handlers/restart_signals.go

@@ -120,7 +120,7 @@ func (h *WorkspaceHandler) resolveAgentURLForRestartSignal(ctx context.Context,
 	// Try Redis cache first.
 	agentURL, err := db.GetCachedURL(ctx, workspaceID)
 	if err == nil && agentURL != "" {
-		return rewriteForDocker(agentURL, workspaceID), nil
+		return h.rewriteForDocker(agentURL, workspaceID), nil
 	}
 
 	// Cache miss — fall back to DB.
@@ -136,13 +136,13 @@ func (h *WorkspaceHandler) resolveAgentURLForRestartSignal(ctx context.Context,
 	}
 	agentURL = *urlNullable
 	_ = db.CacheURL(ctx, workspaceID, agentURL)
-	return rewriteForDocker(agentURL, workspaceID), nil
+	return h.rewriteForDocker(agentURL, workspaceID), nil
 }
 
 // rewriteForDocker rewrites a 127.0.0.1 agent URL to the Docker-DNS form
 // when the platform is running inside a Docker container. When platform is
 // on the host (non-Docker), 127.0.0.1 IS the host and the original URL works.
-func rewriteForDocker(agentURL, workspaceID string) string {
+func (h *WorkspaceHandler) rewriteForDocker(agentURL, workspaceID string) string {
 	if platformInDocker && h.provisioner != nil {
 		// Only rewrite if the URL points to localhost (the ephemeral port
 		// binding the container published to the host). Internal Docker

workspace-server/internal/handlers/restart_signals_test.go

@@ -97,10 +97,10 @@ func TestRewriteForDocker_LocalhostUrlRewritten(t *testing.T) {
 // TestResolveAgentURLForRestartSignal_CacheHit verifies that a Redis-cached
 // URL is returned without hitting the DB.
 func TestResolveAgentURLForRestartSignal_CacheHit(t *testing.T) {
-	mockDB, mock := setupTestDB(t) // must come before setupTestRedisWithURL so db.DB is correct
+	_ = setupTestDB(t) // db.DB must be set before setupTestRedisWithURL
 	_ = setupTestRedisWithURL(t, "http://cached.internal:9000/agent")
 
-	h := newHandlerWithTestDepsWithDB(t, mockDB)
+	h := newHandlerWithTestDeps(t)
 
 	// Redis cache hit → DB should NOT be queried
 	url, err := h.resolveAgentURLForRestartSignal(context.Background(), "ws-cache-hit-123")
@@ -110,19 +110,18 @@ func TestResolveAgentURLForRestartSignal_CacheHit(t *testing.T) {
 	if url == "" {
 		t.Fatal("expected non-empty URL from cache")
 	}
-	// DB should not be queried (no rows returned to sqlmock)
-	if err := mock.ExpectationsWereMet(); err != nil {
-		t.Errorf("unfulfilled DB expectations: %v", err)
+	if url != "http://cached.internal:9000/agent" {
+		t.Errorf("expected cached URL, got %q", url)
 	}
 }
 
 // TestResolveAgentURLForRestartSignal_DBError verifies that a DB error is
 // returned and propagated when neither Redis cache nor DB lookup succeeds.
 func TestResolveAgentURLForRestartSignal_DBError(t *testing.T) {
-	mockDB, mock := setupTestDB(t) // must come before setupTestRedis so db.DB is correct
-	_ = setupTestRedis(t)         // empty → cache miss
+	mock := setupTestDB(t) // must come before setupTestRedis so db.DB is correct
+	_ = setupTestRedis(t) // empty → cache miss
 
-	h := newHandlerWithTestDepsWithDB(t, mockDB)
+	h := newHandlerWithTestDeps(t)
 
 	mock.ExpectQuery(`SELECT url FROM workspaces WHERE id =`).
 		WithArgs("ws-db-err-789").
@@ -141,10 +140,10 @@ func TestResolveAgentURLForRestartSignal_DBError(t *testing.T) {
 // TestResolveAgentURLForRestartSignal_CacheMiss verifies that on Redis miss,
 // the URL is fetched from the DB and cached.
 func TestResolveAgentURLForRestartSignal_CacheMiss(t *testing.T) {
-	mockDB, mock := setupTestDB(t) // must come before setupTestRedis so db.DB is correct
-	mr := setupTestRedis(t)         // empty → cache miss
+	mock := setupTestDB(t) // must come before setupTestRedis so db.DB is correct
+	_ = setupTestRedis(t)  // empty → cache miss
 
-	h := newHandlerWithTestDepsWithDB(t, mockDB)
+	h := newHandlerWithTestDeps(t)
 
 	mock.ExpectQuery(`SELECT url FROM workspaces WHERE id =`).
 		WithArgs("ws-cache-miss-456").
@@ -159,10 +158,12 @@ func TestResolveAgentURLForRestartSignal_CacheMiss(t *testing.T) {
 		t.Errorf("expected DB URL, got %q", url)
 	}
 
-	// Verify the URL was cached in Redis
-	cached, err := mr.Get(context.Background(), "ws:ws-cache-miss-456:url").Result()
+	// Verify the URL was cached in Redis via db.GetCachedURL.
+	// GetCachedURL takes workspaceID and builds the key internally, so
+	// pass "ws-cache-miss-456" (not the full "ws:ws-cache-miss-456:url").
+	cached, err := db.GetCachedURL(context.Background(), "ws-cache-miss-456")
 	if err != nil {
-		t.Fatalf("URL was not cached in Redis: %v", err)
+		t.Fatalf("URL cache read failed: %v", err)
 	}
 	if cached != "http://db.internal:8000/agent" {
 		t.Errorf("expected cached URL %q, got %q", "http://db.internal:8000/agent", cached)
@@ -175,9 +176,7 @@ func TestResolveAgentURLForRestartSignal_CacheMiss(t *testing.T) {
 // TestGracefulPreRestart_Success verifies that when the workspace returns 200,
 // the signal is logged as acknowledged without error.
 func TestGracefulPreRestart_Success(t *testing.T) {
-	_ = setupTestDB(t) // must come before setupTestRedisWithURL so db.DB is correct
-
-	mr := setupTestRedisWithURL(t, "http://localhost:18000/agent")
+	_ = setupTestDB(t)
 
 	// httptest server simulating the workspace container's /signals/restart_pending
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -206,44 +205,40 @@ func TestGracefulPreRestart_Success(t *testing.T) {
 		})
 	}))
 	defer srv.Close()
-	mr.Set("ws:ws-ack-789:url", srv.URL, 5*time.Minute)
 
-	// Patch the handler's resolveAgentURLForRestartSignal to return the test server URL
-	// (avoids needing a real provisioner for this test)
-	h := newHandlerWithTestDeps(t)
-	origResolve := h.resolveAgentURLForRestartSignal
-	h.resolveAgentURLForRestartSignal = func(ctx context.Context, wsID string) (string, error) {
-		return srv.URL + "/agent", nil
+	// Pre-populate Redis cache with the test server URL
+	_ = setupTestRedisWithURL(t, srv.URL)
+
+	// Use an embedded struct to override resolveAgentURLForRestartSignal.
+	hWrapper := &resolveURLTestWrapper{
+		WorkspaceHandler: newHandlerWithTestDeps(t),
+		testURL:         srv.URL + "/agent",
 	}
-	defer func() { h.resolveAgentURLForRestartSignal = origResolve }()
 
 	// gracefulPreRestart runs in a goroutine with its own timeout.
 	// We give it time to complete before the test ends.
-	h.gracefulPreRestart(context.Background(), "ws-ack-789")
+	hWrapper.gracefulPreRestart(context.Background(), "ws-ack-789")
 	time.Sleep(200 * time.Millisecond)
 }
 
 // TestGracefulPreRestart_NotImplemented verifies that when the workspace returns
 // 404 (old SDK version), the platform proceeds gracefully (log + no error).
 func TestGracefulPreRestart_NotImplemented(t *testing.T) {
-	_ = setupTestDB(t) // must come before setupTestRedisWithURL so db.DB is correct
-
-	mr := setupTestRedisWithURL(t, "http://localhost:18001/agent")
+	_ = setupTestDB(t)
 
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(http.StatusNotFound)
 	}))
 	defer srv.Close()
-	mr.Set("ws:ws-noimpl-999:url", srv.URL, 5*time.Minute)
 
-	h := newHandlerWithTestDeps(t)
-	origResolve := h.resolveAgentURLForRestartSignal
-	h.resolveAgentURLForRestartSignal = func(ctx context.Context, wsID string) (string, error) {
-		return srv.URL + "/agent", nil
+	_ = setupTestRedisWithURL(t, srv.URL)
+
+	hWrapper := &resolveURLTestWrapper{
+		WorkspaceHandler: newHandlerWithTestDeps(t),
+		testURL:         srv.URL + "/agent",
 	}
-	defer func() { h.resolveAgentURLForRestartSignal = origResolve }()
 
-	h.gracefulPreRestart(context.Background(), "ws-noimpl-999")
+	hWrapper.gracefulPreRestart(context.Background(), "ws-noimpl-999")
 	time.Sleep(200 * time.Millisecond)
 	// No panic or error expected — graceful degradation
 }
@@ -251,19 +246,17 @@ func TestGracefulPreRestart_NotImplemented(t *testing.T) {
 // TestGracefulPreRestart_ConnectionRefused verifies that when the workspace
 // is unreachable, the platform proceeds gracefully without error.
 func TestGracefulPreRestart_ConnectionRefused(t *testing.T) {
-	_ = setupTestDB(t) // must come before setupTestRedisWithURL so db.DB is correct
+	_ = setupTestDB(t)
 
 	mr := setupTestRedisWithURL(t, "http://localhost:19999/agent") // nothing listening on 19999
-	mr.Set("ws:ws-unreachable-000:url", "http://localhost:19999/agent", 5*time.Minute)
+	_ = mr
 
-	h := newHandlerWithTestDeps(t)
-	origResolve := h.resolveAgentURLForRestartSignal
-	h.resolveAgentURLForRestartSignal = func(ctx context.Context, wsID string) (string, error) {
-		return "http://localhost:19999/agent", nil
+	hWrapper := &resolveURLTestWrapper{
+		WorkspaceHandler: newHandlerWithTestDeps(t),
+		testURL:         "http://localhost:19999/agent",
 	}
-	defer func() { h.resolveAgentURLForRestartSignal = origResolve }()
 
-	h.gracefulPreRestart(context.Background(), "ws-unreachable-000")
+	hWrapper.gracefulPreRestart(context.Background(), "ws-unreachable-000")
 	time.Sleep(200 * time.Millisecond)
 	// No panic or error expected — proceeds with stop as documented
 }
@@ -274,36 +267,35 @@ func TestGracefulPreRestart_URLResolutionError(t *testing.T) {
 	_ = setupTestDB(t)
 	_ = setupTestRedis(t) // empty → URL resolution will fail in resolveAgentURLForRestartSignal
 
-	h := newHandlerWithTestDeps(t)
-
-	// Override resolveAgentURLForRestartSignal to return an error
-	origResolve := h.resolveAgentURLForRestartSignal
-	h.resolveAgentURLForRestartSignal = func(ctx context.Context, wsID string) (string, error) {
-		return "", context.DeadlineExceeded
+	hWrapper := &resolveURLTestWrapper{
+		WorkspaceHandler: newHandlerWithTestDeps(t),
+		errToReturn:     context.DeadlineExceeded,
 	}
-	defer func() { h.resolveAgentURLForRestartSignal = origResolve }()
 
-	h.gracefulPreRestart(context.Background(), "ws-url-err-111")
+	hWrapper.gracefulPreRestart(context.Background(), "ws-url-err-111")
 	time.Sleep(200 * time.Millisecond)
 	// No panic or error expected — proceeds with stop as documented
 }
 
 // ─── helpers ─────────────────────────────────────────────────────────────────
 
-// newHandlerWithTestDeps creates a WorkspaceHandler with test stubs.
-// provisioner is nil so rewriteForDocker returns URL unchanged.
-func newHandlerWithTestDeps(t *testing.T) *WorkspaceHandler {
-	return NewWorkspaceHandler(newTestBroadcaster(), nil, "http://localhost:8080", t.TempDir())
+// resolveURLTestWrapper embeds *WorkspaceHandler and overrides
+// resolveAgentURLForRestartSignal so tests can inject a fixed URL or error.
+type resolveURLTestWrapper struct {
+	*WorkspaceHandler
+	testURL     string
+	errToReturn error
 }
 
-// newHandlerWithTestDepsWithDB creates a WorkspaceHandler with a specific mock DB.
-// Use this when you need to control the DB mock expectations.
-func newHandlerWithTestDepsWithDB(t *testing.T, mockDB *sql.DB) *WorkspaceHandler {
-	// We need to temporarily replace db.DB with our mock
-	origDB := db.DB
-	db.DB = mockDB
-	t.Cleanup(func() { db.DB = origDB })
+func (w *resolveURLTestWrapper) resolveAgentURLForRestartSignal(ctx context.Context, workspaceID string) (string, error) {
+	if w.errToReturn != nil {
+		return "", w.errToReturn
+	}
+	return w.testURL, nil
+}
 
+// newHandlerWithTestDeps creates a WorkspaceHandler with test stubs.
+func newHandlerWithTestDeps(t *testing.T) *WorkspaceHandler {
 	return NewWorkspaceHandler(newTestBroadcaster(), nil, "http://localhost:8080", t.TempDir())
 }
 
@@ -314,7 +306,6 @@ func setupTestRedisWithURL(t *testing.T, url string) *miniredis.Miniredis {
 		t.Fatalf("failed to start miniredis: %v", err)
 	}
 	db.RDB = redis.NewClient(&redis.Options{Addr: mr.Addr()})
-	// Pre-populate a URL for the test workspace IDs used in these tests
 	for _, wsID := range []string{"ws-cache-hit-123", "ws-cache-miss-456", "ws-ack-789", "ws-noimpl-999", "ws-unreachable-000"} {
 		if err := db.CacheURL(context.Background(), wsID, url); err != nil {
 			t.Fatalf("failed to cache URL for %s: %v", wsID, err)
@@ -322,9 +313,4 @@ func setupTestRedisWithURL(t *testing.T, url string) *miniredis.Miniredis {
 	}
 	t.Cleanup(func() { mr.Close() })
 	return mr
-}
-
-// rewriteForDocker is exported from restart_signals.go so it can be tested here.
-func (h *WorkspaceHandler) rewriteForDocker(agentURL, workspaceID string) string {
-	return rewriteForDocker(agentURL, workspaceID)
-}
+}

workspace-server/internal/handlers/workspace.go

@@ -248,6 +248,19 @@ func (h *WorkspaceHandler) Create(c *gin.Context) {
 	// Begin a transaction so the workspace row and any initial secrets are
 	// committed atomically.  A secret-encrypt or DB error rolls back the
 	// workspace insert so we never leave a workspace row with missing secrets.
+
+	// SSRF guard: validate workspace URL before starting any DB transaction.
+	// registry.go:324 calls this same guard for agent self-registration;
+	// the admin-create path must be covered too (core#212).
+	// Must stay above BeginTx so the rejection path never touches the DB.
+	if payload.URL != "" {
+		if err := validateAgentURL(payload.URL); err != nil {
+			log.Printf("Create: workspace URL rejected: %v", err)
+			c.JSON(http.StatusBadRequest, gin.H{"error": "unsafe workspace URL: " + err.Error()})
+			return
+		}
+	}
+
 	tx, txErr := db.DB.BeginTx(ctx, nil)
 	if txErr != nil {
 		log.Printf("Create workspace: begin tx error: %v", txErr)
@@ -383,16 +396,9 @@ func (h *WorkspaceHandler) Create(c *gin.Context) {
 	if payload.External || payload.Runtime == "external" {
 		var connectionToken string
 		if payload.URL != "" {
-			// SSRF guard (issue #212): validateAgentURL blocks cloud metadata
-			// IPs (169.254/16), loopback, link-local, and RFC-1918 in
-			// strict/self-hosted mode. AdminAuth is required here, but the
-			// admin token could be leaked or a compromised insider — defence
-			// in depth. Compare: registry.go:324 (heartbeat path) also
-			// calls validateAgentURL; external_rotate.go should too.
-			if err := validateAgentURL(payload.URL); err != nil {
-				c.JSON(http.StatusBadRequest, gin.H{"error": "unsafe workspace URL: " + err.Error()})
-				return
-			}
+			// URL already validated by validateAgentURL above (before BeginTx).
+			// Now persist it: the external URL is set after the workspace row
+			// commits so that a failed URL UPDATE doesn't roll back the row.
 			db.DB.ExecContext(ctx, `UPDATE workspaces SET url = $1, status = $2, runtime = 'external', updated_at = now() WHERE id = $3`, payload.URL, models.StatusOnline, id)
 			if err := db.CacheURL(ctx, id, payload.URL); err != nil {
 				log.Printf("External workspace: failed to cache URL for %s: %v", id, err)

workspace-server/internal/handlers/workspace_test.go

@@ -537,17 +537,15 @@ func TestWorkspaceCreate_ExternalURL_SSRFSafe(t *testing.T) {
 		WithArgs(sqlmock.AnyArg(), "Ext Agent", nil, 3, "external", sqlmock.AnyArg(), (*string)(nil), nil, "none", (*int64)(nil), models.DefaultMaxConcurrentTasks, "push").
 		WillReturnResult(sqlmock.NewResult(0, 1))
 	mock.ExpectCommit()
-	// External URL update (SSRF-safe public URL passes validateAgentURL).
+	// External URL update (localhost is explicitly allowed by validateAgentURL).
 	mock.ExpectExec("UPDATE workspaces SET url").
 		WillReturnResult(sqlmock.NewResult(0, 1))
-	// CacheURL is non-fatal but still called.
-	mock.ExpectExec("SELECT").
-		WillReturnRows(sqlmock.NewRows([]string{"ok"}).AddRow("ok"))
+	// CacheURL is non-fatal — uses Redis (db.RDB, set by setupTestRedis), not the DB.
 
 	w := httptest.NewRecorder()
 	c, _ := gin.CreateTestContext(w)
 
-	body := `{"name":"Ext Agent","runtime":"external","external":true,"url":"https://agent.example.com/a2a"}`
+	body := `{"name":"Ext Agent","runtime":"external","external":true,"url":"http://localhost:8000"}`
 	c.Request = httptest.NewRequest("POST", "/workspaces", bytes.NewBufferString(body))
 	c.Request.Header.Set("Content-Type", "application/json")
 

workspace-server/internal/plugins/drift_sweeper.go

@@ -9,7 +9,7 @@ package plugins
 //   1. SELECTs workspace_plugins rows where tracked_ref != 'none'
 //      AND installed_sha IS NOT NULL (skip pre-migration rows with NULL SHA).
 //   2. For each row, resolves the tracked ref to its current upstream SHA
-//      using the appropriate SourceResolver.
+//      using the appropriate PluginResolver.
 //   3. If the resolved SHA differs from installed_sha → drift detected.
 //   4. On drift, INSERT INTO plugin_update_queue (ON CONFLICT DO NOTHING so
 //      a re-drift while a row is still pending is a no-op).
@@ -61,10 +61,12 @@ const DriftSweepInterval = 1 * time.Hour
 // that handles Gitea instances on high-latency links.
 const ResolveRefDeadline = 60 * time.Second
 
-// SourceResolver resolves plugin sources to installable directories.
+// PluginResolver resolves plugin sources to installable directories.
 // Satisfied by *Registry (which wraps GithubResolver + LocalResolver).
-type SourceResolver interface {
-	Resolve(source Source) (SourceResolver, error)
+// Named PluginResolver (not SourceResolver) to avoid redeclaring the
+// SourceResolver interface defined in source.go (core#228 fix).
+type PluginResolver interface {
+	Resolve(source Source) (PluginResolver, error)
 	Schemes() []string
 }
 
@@ -74,7 +76,7 @@ type SourceResolver interface {
 //
 // Registers itself via atexits in cmd/server/main.go so the process
 // shuts down cleanly on SIGTERM.
-func StartPluginDriftSweeper(ctx context.Context, resolver SourceResolver) {
+func StartPluginDriftSweeper(ctx context.Context, resolver PluginResolver) {
 	if resolver == nil {
 		log.Println("Plugin drift sweeper: resolver is nil — sweeper disabled")
 		return
@@ -107,7 +109,7 @@ func StartPluginDriftSweeper(ctx context.Context, resolver SourceResolver) {
 // sweepDriftOnce runs one full drift-detection cycle.
 // Errors are non-fatal — each row is handled independently so a single
 // slow row doesn't block the rest of the sweep.
-func sweepDriftOnce(parent context.Context, resolver SourceResolver) {
+func sweepDriftOnce(parent context.Context, resolver PluginResolver) {
 	ctx, cancel := context.WithTimeout(parent, 10*time.Minute)
 	defer cancel()
 
@@ -170,7 +172,7 @@ func sweepDriftOnce(parent context.Context, resolver SourceResolver) {
 // resolveLatestSHA resolves the tracked ref to its current upstream SHA.
 // Handles both github:// and local:// sources; local sources are skipped
 // (no meaningful upstream to drift against).
-func resolveLatestSHA(ctx context.Context, resolver SourceResolver, sourceRaw, trackedRef string) (string, error) {
+func resolveLatestSHA(ctx context.Context, resolver PluginResolver, sourceRaw, trackedRef string) (string, error) {
 	// Strip the scheme prefix to get the raw spec.
 	// sourceRaw is stored as the full string, e.g. "github://owner/repo#tag:v1.0.0"
 	spec := sourceRaw
@@ -231,7 +233,7 @@ func queueDriftEntry(ctx context.Context, workspaceID, pluginName, trackedRef, c
 // ─────────────────────────────────────────────────────────────────────────────
 
 // SweepDriftOnceForTest exposes sweepDriftOnce for package-level testing.
-func SweepDriftOnceForTest(parent context.Context, resolver SourceResolver) {
+func SweepDriftOnceForTest(parent context.Context, resolver PluginResolver) {
 	sweepDriftOnce(parent, resolver)
 }