Merge pull request 'fix(handlers): CWE-78 guard + rows.Err() checks — hotfix for staging regressions' (#1071) from fix/offsec-003-boundary-wrapping into main

.gitea/scripts/sop-checklist.py

@@ -109,58 +109,57 @@ def normalize_slug(raw: str, numeric_aliases: dict[int, str] | None = None) -> s
 # Optional trailing note after the slug for /sop-ack and required reason
 # for /sop-revoke (RFC#351 open question 4 — reason is captured but not
 # yet validated; future iteration may require a min-length).
-#
-# /sop-n/a <gate> [reason] — declares a gate as not-applicable.
-#   <gate> is a canonical gate name (qa-review, security-review).
-#   The declaring user must be in one of the gate's required_teams.
-#   Most-recent per-user declaration wins (revoke semantics mirror ack).
 _DIRECTIVE_RE = re.compile(
     r"^[ \t]*/(sop-ack|sop-revoke)[ \t]+([A-Za-z0-9_\- ]+?)(?:[ \t]+(.*))?[ \t]*$",
     re.MULTILINE,
 )
-_NA_DIRECTIVE_RE = re.compile(
-    r"^[ \t]*/sop-n/?a[ \t]+([A-Za-z0-9_\-]+)(?:[ \t]+(.*))?[ \t]*$",
-    re.MULTILINE,
-)
 
 
 def parse_directives(
     comment_body: str,
     numeric_aliases: dict[int, str],
-) -> tuple[list[tuple[str, str, str]], list[tuple[str, str, str]]]:
-    """Extract /sop-ack, /sop-revoke, and /sop-n/a directives from a comment body.
+) -> list[tuple[str, str, str]]:
+    """Extract /sop-ack and /sop-revoke directives from a comment body.
 
-    Returns a tuple of two lists:
-      0. list of (kind, canonical_slug, note) for sop-ack/sop-revoke
-      1. list of (kind, gate_name, reason) for sop-n/a
-
-    canonical_slug is the normalized form (or "" if unparseable).
-    note/reason is the trailing free-text (may be "").
+    Returns a list of (kind, canonical_slug, note) tuples where:
+      kind is "sop-ack" or "sop-revoke"
+      canonical_slug is the normalized form (or "" if unparseable)
+      note is the trailing free-text (may be "")
     """
     out: list[tuple[str, str, str]] = []
-    na_out: list[tuple[str, str, str]] = []
     if not comment_body:
-        return out, na_out
+        return out
     for m in _DIRECTIVE_RE.finditer(comment_body):
         kind = m.group(1)
         raw_slug = (m.group(2) or "").strip()
+        # If the raw match included trailing words, the regex non-greedy
+        # captured only the first token; strip again for safety.
+        # We split on whitespace to keep the FIRST word as the slug, and
+        # everything after as the note.
         parts = raw_slug.split()
         if not parts:
             continue
         first = parts[0]
+        # If the slug-capture greedily matched multiple words (e.g.
+        # "comprehensive testing"), preserve normalize behavior: join
+        # the WHOLE first-word-token only; trailing words get appended to
+        # the note. The regex limits group(2) to [A-Za-z0-9_\- ] so we
+        # may have multi-word forms here — normalize handles them.
         if len(parts) > 1:
+            # User wrote "/sop-ack comprehensive testing extra-note"
+            # → treat "comprehensive testing" as the slug source if it
+            # normalizes to a known item; otherwise treat "comprehensive"
+            # as slug and "testing extra-note" as note. We defer the
+            # disambiguation to the caller via the returned canonical
+            # slug. For simplicity: try the WHOLE captured string first.
             canonical = normalize_slug(raw_slug, numeric_aliases)
         else:
             canonical = normalize_slug(first, numeric_aliases)
         note_from_group = (m.group(3) or "").strip()
+        # If we collapsed multi-word slug into kebab and there's a
+        # trailing-text group too, append it.
         out.append((kind, canonical, note_from_group))
-
-    for m in _NA_DIRECTIVE_RE.finditer(comment_body):
-        gate = (m.group(1) or "").strip().lower()
-        reason = (m.group(2) or "").strip()
-        na_out.append(("sop-n/a", gate, reason))
-
-    return out, na_out
+    return out
 
 
 # ---------------------------------------------------------------------------
@@ -231,8 +230,9 @@ def compute_ack_state(
        {
          "comprehensive-testing": {
            "ackers": ["bob"],         # non-author, team-verified
-           "rejected": {
+           "rejected_ackers": {        # debugging info
              "self_ack": ["alice"],
+             "unknown_slug": [],
              "not_in_team": ["eve"],
            }
          },
@@ -249,8 +249,7 @@ def compute_ack_state(
         user = (c.get("user") or {}).get("login", "")
         if not user:
             continue
-        directives, _na_directives = parse_directives(body, numeric_aliases)
-        for kind, slug, _note in directives:
+        for kind, slug, _note in parse_directives(body, numeric_aliases):
             if not slug:
                 unparseable_per_user[user] = unparseable_per_user.get(user, 0) + 1
                 continue
@@ -260,19 +259,25 @@ def compute_ack_state(
     # Filter out self-acks and unknown slugs.
     ackers_per_slug: dict[str, list[str]] = {s: [] for s in items_by_slug}
     rejected_self: dict[str, list[str]] = {s: [] for s in items_by_slug}
+    rejected_unknown: dict[str, list[str]] = {s: [] for s in items_by_slug}
     pending_team_check: dict[str, list[str]] = {s: [] for s in items_by_slug}
 
     for (user, slug), kind in latest_directive.items():
         if kind != "sop-ack":
             continue  # revokes leave the (user,slug) state as "no ack"
         if slug not in items_by_slug:
+            # Slug normalized to something not in our config — store
+            # under a synthetic key for diagnostic surfacing. Don't add
+            # to any item.
             continue
         if user == pr_author:
             rejected_self[slug].append(user)
             continue
         pending_team_check[slug].append(user)
 
-    # Step 3: team membership probe per slug.
+    # Step 3: team membership probe per slug (batched per slug to keep
+    # API call count down — same user may ack multiple items but the
+    # required_teams differ per item, so we MUST probe per (user, item)).
     rejected_not_in_team: dict[str, list[str]] = {s: [] for s in items_by_slug}
     for slug, candidates in pending_team_check.items():
         if not candidates:
@@ -281,6 +286,7 @@ def compute_ack_state(
         approved = team_membership_probe(slug, candidates)  # returns subset
         rejected_not_in_team[slug] = [u for u in candidates if u not in approved]
         ackers_per_slug[slug] = approved
+        # Stash required teams for description rendering.
         items_by_slug[slug]["_required_resolved"] = required
 
     return {
@@ -295,113 +301,6 @@ def compute_ack_state(
     }
 
 
-def compute_na_state(
-    comments: list[dict[str, Any]],
-    pr_author: str,
-    na_gates: dict[str, dict[str, Any]],
-    numeric_aliases: dict[int, str],
-    team_membership_probe: "callable[[str, list[str]], list[str]]",
-    client: "GiteaClient",
-    org: str,
-) -> dict[str, dict[str, Any]]:
-    """Compute per-gate N/A declaration state.
-
-    Returns a dict keyed by gate name:
-       {
-         "qa-review": {
-           "declared":  ["alice"],      # non-author, team-verified, not revoked
-           "rejected": ["eve (not-in-team)", "bob (self-decl)"],
-           "reason":   "pure-infra change — no qa surface",
-         },
-         ...
-       }
-    A gate is N/A-satisfied when at least one declaration from a valid
-    team member exists and has not been revoked by the same user.
-    """
-    if not na_gates:
-        return {}
-
-    # Collapse directives per (commenter, gate) — most recent wins.
-    latest_na: dict[tuple[str, str], str] = {}   # (user, gate) → "sop-n/a"
-    latest_na_reason: dict[tuple[str, str], str] = {}  # (user, gate) → reason
-    for c in comments:
-        body = c.get("body", "") or ""
-        user = (c.get("user") or {}).get("login", "")
-        if not user:
-            continue
-        _directives, na_directives = parse_directives(body, numeric_aliases)
-        for _kind, gate, reason in na_directives:
-            if gate not in na_gates:
-                continue
-            latest_na[(user, gate)] = "sop-n/a"
-            latest_na_reason[(user, gate)] = reason
-
-    # Determine candidate declarers per gate.
-    na_state: dict[str, dict[str, Any]] = {
-        gate: {"declared": [], "rejected": [], "reason": ""}
-        for gate in na_gates
-    }
-    pending_per_gate: dict[str, list[str]] = {gate: [] for gate in na_gates}
-
-    for (user, gate), kind in latest_na.items():
-        if kind != "sop-n/a":
-            continue
-        if user == pr_author:
-            na_state[gate]["rejected"].append(f"{user} (self-decl)")
-            continue
-        pending_per_gate[gate].append(user)
-
-    # Probe team membership per gate using that gate's required_teams.
-    for gate, candidates in pending_per_gate.items():
-        if not candidates:
-            continue
-        required_teams = na_gates[gate].get("required_teams", [])
-        # Resolve team names → ids using the client's resolver.
-        team_ids: list[int] = []
-        for tn in required_teams:
-            tid = client.resolve_team_id(org, tn)
-            if tid is not None:
-                team_ids.append(tid)
-        if not team_ids:
-            na_state[gate]["rejected"].extend(
-                f"{u} (no-team-id)" for u in candidates
-            )
-            continue
-        for u in candidates:
-            in_any_team = False
-            for tid in team_ids:
-                result = client.is_team_member(tid, u)
-                if result is True:
-                    in_any_team = True
-                    break
-                if result is None:
-                    # 403 — token owner not in team. Fail-closed.
-                    print(
-                        f"::warning::na: team-probe for {u} in team-id {tid} "
-                        "returned 403 — treating as not-in-team (fail-closed)",
-                        file=sys.stderr,
-                    )
-            if in_any_team:
-                na_state[gate]["declared"].append(u)
-            else:
-                na_state[gate]["rejected"].append(f"{u} (not-in-team)")
-
-    # Build per-gate reason string from declared users.
-    for gate in na_gates:
-        decl = na_state[gate]["declared"]
-        if decl:
-            reasons: list[str] = []
-            for u in decl:
-                r = latest_na_reason.get((u, gate), "")
-                if r:
-                    reasons.append(f"{u}: {r}")
-                else:
-                    reasons.append(u)
-            na_state[gate]["reason"] = "; ".join(reasons)
-
-    return na_state
-
-
 # ---------------------------------------------------------------------------
 # Gitea API client
 # ---------------------------------------------------------------------------
@@ -799,7 +698,6 @@ def main(argv: list[str] | None = None) -> int:
     numeric_aliases = {
         int(it["numeric_alias"]): it["slug"] for it in items if it.get("numeric_alias")
     }
-    na_gates: dict[str, dict[str, Any]] = cfg.get("n/a_gates") or {}
 
     client = GiteaClient(args.gitea_host, token) if token else None
     if not client:
@@ -819,8 +717,6 @@ def main(argv: list[str] | None = None) -> int:
         print("::error::PR payload missing user.login or head.sha", file=sys.stderr)
         return 1
 
-    target_url = f"https://{args.gitea_host}/{args.owner}/{args.repo}/pulls/{args.pr}"
-
     comments = client.get_issue_comments(args.owner, args.repo, args.pr)
 
     # Build team-membership probe closure that caches results per
@@ -878,47 +774,6 @@ def main(argv: list[str] | None = None) -> int:
     ack_state = compute_ack_state(comments, author, items_by_slug, numeric_aliases, probe)
     body_state = {it["slug"]: section_marker_present(body, it["pr_section_marker"]) for it in items}
 
-    # --- N/A gate state (RFC#324 §N/A follow-up) ---
-    na_state: dict[str, dict[str, Any]] = {}
-    if na_gates:
-        na_state = compute_na_state(
-            comments, author, na_gates, numeric_aliases,
-            probe, client, args.owner,
-        )
-        # Post N/A declarations status (read by review-check.sh).
-        na_satisfied = [g for g, s in na_state.items() if s["declared"]]
-        na_missing   = [g for g, s in na_state.items() if not s["declared"]]
-        if na_satisfied:
-            na_desc = f"N/A: {', '.join(na_satisfied)}"
-            na_post_state = "success"
-        elif na_missing:
-            na_desc = f"awaiting /sop-n/a declaration for: {', '.join(na_missing)}"
-            na_post_state = "pending"
-        else:
-            # Configured but no declarations yet.
-            na_desc = "no /sop-n/a declarations yet"
-            na_post_state = "pending"
-        na_context = "sop-checklist / na-declarations (pull_request)"
-        print(f"::notice::na-declarations status: {na_post_state} — {na_desc}")
-        if not args.dry_run:
-            client.post_status(
-                args.owner, args.repo, head_sha,
-                state=na_post_state, context=na_context,
-                description=na_desc,
-                target_url=target_url,
-            )
-            print(f"::notice::na-declarations status posted: {na_context} → {na_post_state}")
-        # Log per-gate diagnostics.
-        for gate in na_gates:
-            s = na_state.get(gate, {})
-            if s.get("declared"):
-                print(f"::notice::  [PASS] gate={gate} — N/A declared by {','.join(s['declared'])}"
-                      + (f" ({s['reason']})" if s.get("reason") else ""))
-            else:
-                extra = f" — rejected: {', '.join(s.get('rejected', []))}" if s.get("rejected") else ""
-                print(f"::notice::  [WAIT] gate={gate} — no valid N/A declaration yet{extra}")
-
-
     state, description = render_status(items, ack_state, body_state)
     mode = get_tier_mode(pr, cfg)
     if mode == "soft":
@@ -953,6 +808,7 @@ def main(argv: list[str] | None = None) -> int:
             return 0 if state in ("success", "pending") else 1
         return 0
 
+    target_url = f"https://{args.gitea_host}/{args.owner}/{args.repo}/pulls/{args.pr}"
     client.post_status(
         args.owner, args.repo, head_sha,
         state=state, context=args.status_context,

canvas/src/components/canvas/__tests__/useOrgDeployState.test.ts

@@ -0,0 +1,311 @@
+/**
+ * Unit tests for buildDeployMap — the pure tree-traversal core of
+ * useOrgDeployState.
+ *
+ * What is tested here:
+ *   - Root / leaf identification via parent-chain walk
+ *   - isDeployingRoot: true when any descendant is "provisioning"
+ *   - isActivelyProvisioning: true only for the node itself in that state
+ *   - isLockedChild: true for non-root nodes in a deploying tree
+ *   - isLockedChild: also true for nodes in deletingIds (even if not deploying)
+ *   - descendantProvisioningCount: non-zero only on root nodes
+ *   - Performance contract: O(n) single-pass walk — tested by verifying
+ *     correctness across 50-node trees (n=50, all cases above)
+ *
+ * What is NOT tested here (hook integration — appropriate for E2E):
+ *   - The useMemo / Zustand subscription wiring
+ *   - React Flow integration (flowToScreenPosition, getInternalNode)
+ *
+ * Issue: #2071 (Canvas test gaps follow-up).
+ */
+import { describe, expect, it } from "vitest";
+import { buildDeployMap, type OrgDeployState } from "../useOrgDeployState";
+
+// ── Helpers ──────────────────────────────────────────────────────────────────
+
+type Projection = { id: string; parentId: string | null; status: string };
+
+function proj(
+  id: string,
+  parentId: string | null,
+  status: string,
+): Projection {
+  return { id, parentId, status };
+}
+
+/** Unchecked cast — test helpers aren't production code paths. */
+function m(
+  ps: Projection[],
+  deletingIds: string[] = [],
+): Map<string, OrgDeployState> {
+  return buildDeployMap(ps, new Set(deletingIds));
+}
+
+function s(
+  map: Map<string, OrgDeployState>,
+  id: string,
+): OrgDeployState {
+  const got = map.get(id);
+  if (!got) throw new Error(`no entry for id=${id}`);
+  return got;
+}
+
+// ── Empty / trivial ───────────────────────────────────────────────────────────
+
+describe("buildDeployMap — empty", () => {
+  it("returns empty map for empty projections", () => {
+    expect(m([]).size).toBe(0);
+  });
+});
+
+// ── Single node ─────────────────────────────────────────────────────────────
+
+describe("buildDeployMap — single node", () => {
+  it("isolated node is its own root and not deploying", () => {
+    const map = m([proj("a", null, "online")]);
+    expect(s(map, "a")).toEqual({
+      isActivelyProvisioning: false,
+      isDeployingRoot: false,
+      isLockedChild: false,
+      descendantProvisioningCount: 0,
+    });
+  });
+
+  it("isolated provisioning node is deploying root", () => {
+    const map = m([proj("a", null, "provisioning")]);
+    expect(s(map, "a")).toEqual({
+      isActivelyProvisioning: true,
+      isDeployingRoot: true,
+      isLockedChild: false,
+      descendantProvisioningCount: 1,
+    });
+  });
+});
+
+// ── Parent / child chains ─────────────────────────────────────────────────────
+
+describe("buildDeployMap — parent / child chains", () => {
+  it("root with online child: root is not deploying, child is not locked", () => {
+    // A ──► B
+    const map = m([
+      proj("A", null, "online"),
+      proj("B", "A", "online"),
+    ]);
+    expect(s(map, "A")).toMatchObject({ isDeployingRoot: false, isLockedChild: false });
+    expect(s(map, "B")).toMatchObject({ isDeployingRoot: false, isLockedChild: false });
+  });
+
+  it("root with provisioning child: root is deploying, child is locked", () => {
+    // A ──► B (B is provisioning)
+    const map = m([
+      proj("A", null, "online"),
+      proj("B", "A", "provisioning"),
+    ]);
+    expect(s(map, "A")).toMatchObject({ isDeployingRoot: true, descendantProvisioningCount: 1 });
+    expect(s(map, "B")).toMatchObject({ isLockedChild: true, isActivelyProvisioning: true });
+  });
+
+  it("provisioning root with online child: root is deploying, child is locked", () => {
+    // A (provisioning) ──► B (online)
+    const map = m([
+      proj("A", null, "provisioning"),
+      proj("B", "A", "online"),
+    ]);
+    expect(s(map, "A")).toMatchObject({ isDeployingRoot: true, isActivelyProvisioning: true });
+    expect(s(map, "B")).toMatchObject({ isLockedChild: true, isActivelyProvisioning: false });
+  });
+
+  it("grandchild inherits deploy lock through intermediate online node", () => {
+    // A ──► B ──► C  (A is provisioning)
+    const map = m([
+      proj("A", null, "provisioning"),
+      proj("B", "A", "online"),
+      proj("C", "B", "online"),
+    ]);
+    // B and C are both non-root descendants of the deploying root
+    expect(s(map, "B")).toMatchObject({ isLockedChild: true });
+    expect(s(map, "C")).toMatchObject({ isLockedChild: true });
+    expect(s(map, "A")).toMatchObject({ isDeployingRoot: true, descendantProvisioningCount: 1 });
+  });
+
+  it("deep chain: only the topmost node with a null parent counts as root", () => {
+    // A ──► B ──► C ──► D  (A is provisioning)
+    const map = m([
+      proj("A", null, "provisioning"),
+      proj("B", "A", "online"),
+      proj("C", "B", "online"),
+      proj("D", "C", "online"),
+    ]);
+    const roots = ["A", "B", "C", "D"].filter((id) => s(map, id).isDeployingRoot);
+    expect(roots).toEqual(["A"]);
+  });
+});
+
+// ── Sibling branching ─────────────────────────────────────────────────────────
+
+describe("buildDeployMap — sibling branching", () => {
+  it("parent with multiple children: deploying root propagates to all children", () => {
+    //         A (provisioning)
+    //        / \
+    //       B   C
+    const map = m([
+      proj("A", null, "provisioning"),
+      proj("B", "A", "online"),
+      proj("C", "A", "online"),
+    ]);
+    expect(s(map, "B")).toMatchObject({ isLockedChild: true });
+    expect(s(map, "C")).toMatchObject({ isLockedChild: true });
+    expect(s(map, "A")).toMatchObject({ descendantProvisioningCount: 1 });
+  });
+
+  it("only one provisioning descendant marks the root as deploying", () => {
+    //           A
+    //         / | \
+    //        B  C  D   (only C is provisioning)
+    const map = m([
+      proj("A", null, "online"),
+      proj("B", "A", "online"),
+      proj("C", "A", "provisioning"),
+      proj("D", "A", "online"),
+    ]);
+    expect(s(map, "A")).toMatchObject({ isDeployingRoot: true, descendantProvisioningCount: 1 });
+    expect(s(map, "B")).toMatchObject({ isLockedChild: true });
+    expect(s(map, "C")).toMatchObject({ isLockedChild: true, isActivelyProvisioning: true });
+    expect(s(map, "D")).toMatchObject({ isLockedChild: true });
+  });
+
+  it("two provisioning siblings: count reflects both", () => {
+    const map = m([
+      proj("A", null, "online"),
+      proj("B", "A", "provisioning"),
+      proj("C", "A", "provisioning"),
+    ]);
+    expect(s(map, "A")).toMatchObject({ descendantProvisioningCount: 2 });
+    expect(s(map, "B")).toMatchObject({ isActivelyProvisioning: true });
+    expect(s(map, "C")).toMatchObject({ isActivelyProvisioning: true });
+  });
+});
+
+// ── Multiple disjoint trees ───────────────────────────────────────────────────
+
+describe("buildDeployMap — multiple disjoint trees", () => {
+  it("each tree has its own root; deploying nodes are independent", () => {
+    // Tree 1: X (provisioning) ──► Y
+    // Tree 2: P ──► Q  (no provisioning)
+    const map = m([
+      proj("X", null, "provisioning"),
+      proj("Y", "X", "online"),
+      proj("P", null, "online"),
+      proj("Q", "P", "online"),
+    ]);
+    expect(s(map, "X")).toMatchObject({ isDeployingRoot: true });
+    expect(s(map, "Y")).toMatchObject({ isLockedChild: true });
+    expect(s(map, "P")).toMatchObject({ isDeployingRoot: false, isLockedChild: false });
+    expect(s(map, "Q")).toMatchObject({ isDeployingRoot: false, isLockedChild: false });
+  });
+});
+
+// ── Deleting nodes ────────────────────────────────────────────────────────────
+
+describe("buildDeployMap — deletingIds", () => {
+  it("node in deletingIds is locked even if tree is not deploying", () => {
+    const map = m(
+      [
+        proj("A", null, "online"),
+        proj("B", "A", "online"),
+      ],
+      ["B"], // B is being deleted
+    );
+    expect(s(map, "A")).toMatchObject({ isLockedChild: false });
+    expect(s(map, "B")).toMatchObject({ isLockedChild: true, isActivelyProvisioning: false });
+  });
+
+  it("node in deletingIds: isLockedChild is true regardless of provisioning", () => {
+    const map = m(
+      [
+        proj("A", null, "provisioning"),
+        proj("B", "A", "online"),
+      ],
+      ["B"],
+    );
+    // B is both a deploying-child AND a deleting node — either alone locks it
+    expect(s(map, "B")).toMatchObject({ isLockedChild: true });
+  });
+
+  it("empty deletingIds set has no effect", () => {
+    const map = m(
+      [
+        proj("A", null, "online"),
+        proj("B", "A", "online"),
+      ],
+      [],
+    );
+    expect(s(map, "B")).toMatchObject({ isLockedChild: false });
+  });
+});
+
+// ── descendantProvisioningCount ───────────────────────────────────────────────
+
+describe("buildDeployMap — descendantProvisioningCount", () => {
+  it("is 0 for non-root nodes", () => {
+    const map = m([
+      proj("A", null, "provisioning"),
+      proj("B", "A", "provisioning"),
+    ]);
+    expect(s(map, "B").descendantProvisioningCount).toBe(0);
+  });
+
+  it("includes the root's own status when provisioning", () => {
+    const map = m([
+      proj("A", null, "provisioning"),
+      proj("B", "A", "online"),
+    ]);
+    // A is both root and provisioning → count includes itself
+    expect(s(map, "A").descendantProvisioningCount).toBe(1);
+  });
+
+  it("accumulates all provisioning descendants (not just immediate children)", () => {
+    const map = m([
+      proj("A", null, "online"),
+      proj("B", "A", "online"),
+      proj("C", "B", "provisioning"),
+    ]);
+    expect(s(map, "A").descendantProvisioningCount).toBe(1);
+  });
+});
+
+// ── O(n) performance ─────────────────────────────────────────────────────────
+
+describe("buildDeployMap — O(n) performance contract", () => {
+  it("handles a 50-node three-level tree without incorrect node assignments", () => {
+    // Level 0: 1 root
+    // Level 1: 7 children
+    // Level 2: 42 leaves
+    // Total: 50 nodes
+    const projections: Projection[] = [];
+    projections.push(proj("root", null, "provisioning"));
+    for (let i = 0; i < 7; i++) {
+      projections.push(proj(`l1-${i}`, "root", "online"));
+    }
+    for (let i = 0; i < 42; i++) {
+      const parent = `l1-${Math.floor(i / 6)}`;
+      projections.push(proj(`l2-${i}`, parent, "online"));
+    }
+    const map = m(projections);
+
+    // Root is the only deploying node
+    expect(s(map, "root")).toMatchObject({
+      isDeployingRoot: true,
+      isLockedChild: false,
+      descendantProvisioningCount: 1,
+    });
+
+    // Every other node is a locked child
+    for (let i = 0; i < 7; i++) {
+      expect(s(map, `l1-${i}`)).toMatchObject({ isLockedChild: true, isDeployingRoot: false });
+    }
+    for (let i = 0; i < 42; i++) {
+      expect(s(map, `l2-${i}`)).toMatchObject({ isLockedChild: true, isDeployingRoot: false });
+    }
+  });
+});

canvas/src/lib/__tests__/palette-context.test.tsx

@@ -0,0 +1,205 @@
+// @vitest-environment jsdom
+"use client";
+/**
+ * Tests for palette-context.tsx — MobileAccentProvider context + usePalette hook.
+ *
+ * Test coverage (9 cases):
+ * 1. MobileAccentProvider renders children
+ * 2. usePalette(false) without provider → MOL_LIGHT
+ * 3. usePalette(true) without provider → MOL_DARK
+ * 4. accent=null returns base palette unchanged
+ * 5. accent=base.accent returns base palette unchanged (identity guard)
+ * 6. accent="#custom" overrides both accent and online
+ * 7. MOL_LIGHT singleton never mutated
+ * 8. MOL_DARK singleton never mutated
+ *
+ * Plus pure-function coverage for normalizeStatus + tierCode.
+ */
+import { describe, expect, it, vi, beforeEach, afterEach } from "vitest";
+import React from "react";
+import { render, screen, cleanup } from "@testing-library/react";
+import {
+  MOL_LIGHT,
+  MOL_DARK,
+  getPalette,
+  normalizeStatus,
+  tierCode,
+  MobileAccentProvider,
+  usePalette,
+} from "../palette-context";
+
+// ─── usePalette test helper ───────────────────────────────────────────────────
+// usePalette reads document.documentElement.dataset.theme internally.
+// We set this before rendering so the hook sees the right value.
+
+function setDataTheme(theme: "light" | "dark") {
+  if (typeof document !== "undefined") {
+    document.documentElement.dataset.theme = theme;
+  }
+}
+
+// ─── Pure function tests ──────────────────────────────────────────────────────
+
+describe("normalizeStatus", () => {
+  it("returns emerald-400 for online status", () => {
+    expect(normalizeStatus("online", false)).toBe("bg-emerald-400");
+    expect(normalizeStatus("online", true)).toBe("bg-emerald-400");
+  });
+
+  it("returns emerald-400 for degraded status", () => {
+    expect(normalizeStatus("degraded", false)).toBe("bg-emerald-400");
+    expect(normalizeStatus("degraded", true)).toBe("bg-emerald-400");
+  });
+
+  it("returns red-400 for failed status", () => {
+    expect(normalizeStatus("failed", false)).toBe("bg-red-400");
+    expect(normalizeStatus("failed", true)).toBe("bg-red-400");
+  });
+
+  it("returns amber-400 for paused status", () => {
+    expect(normalizeStatus("paused", false)).toBe("bg-amber-400");
+    expect(normalizeStatus("paused", true)).toBe("bg-amber-400");
+  });
+
+  it("returns amber-400 for not_configured status", () => {
+    expect(normalizeStatus("not_configured", false)).toBe("bg-amber-400");
+  });
+
+  it("returns zinc-400 for unknown status", () => {
+    expect(normalizeStatus("unknown", false)).toBe("bg-zinc-400");
+    expect(normalizeStatus("", false)).toBe("bg-zinc-400");
+  });
+});
+
+describe("tierCode", () => {
+  it("returns T1 for tier 1", () => {
+    expect(tierCode(1)).toBe("T1");
+  });
+
+  it("returns T2 for tier 2", () => {
+    expect(tierCode(2)).toBe("T2");
+  });
+
+  it("returns T4 for tier 4", () => {
+    expect(tierCode(4)).toBe("T4");
+  });
+
+  it("returns generic T{n} for non-standard tiers", () => {
+    expect(tierCode(99)).toBe("T99");
+  });
+});
+
+// ─── getPalette tests ─────────────────────────────────────────────────────────
+
+describe("getPalette — accent override", () => {
+  it("accent=null returns base palette unchanged (light)", () => {
+    const result = getPalette(null, false);
+    expect(result).toEqual({ ...MOL_LIGHT });
+    expect(result).not.toBe(MOL_LIGHT); // returned object is a copy
+  });
+
+  it("accent=null returns base palette unchanged (dark)", () => {
+    const result = getPalette(null, true);
+    expect(result).toEqual({ ...MOL_DARK });
+    expect(result).not.toBe(MOL_DARK);
+  });
+
+  it("accent=base.accent returns base palette unchanged (identity guard, light)", () => {
+    const result = getPalette(MOL_LIGHT.accent, false);
+    expect(result).toEqual({ ...MOL_LIGHT });
+    expect(result).not.toBe(MOL_LIGHT);
+  });
+
+  it("accent=base.accent returns base palette unchanged (identity guard, dark)", () => {
+    const result = getPalette(MOL_DARK.accent, true);
+    expect(result).toEqual({ ...MOL_DARK });
+    expect(result).not.toBe(MOL_DARK);
+  });
+
+  it("accent='#custom' overrides accent and online (light)", () => {
+    const result = getPalette("#ff0000", false);
+    expect(result.accent).toBe("#ff0000");
+    expect(result.online).toBe("bg-emerald-400"); // normalizeStatus("online", false)
+  });
+
+  it("accent='#custom' overrides accent and online (dark)", () => {
+    const result = getPalette("#00ff00", true);
+    expect(result.accent).toBe("#00ff00");
+    expect(result.online).toBe("bg-emerald-400"); // normalizeStatus("online", true)
+  });
+
+  it("MOL_LIGHT singleton is never mutated", () => {
+    getPalette("#mutate", false);
+    // All fields must still match the original freeze definition
+    expect(MOL_LIGHT.accent).toBe("bg-blue-500");
+    expect(MOL_LIGHT.online).toBe("bg-emerald-400");
+    expect(MOL_LIGHT.surface).toBe("bg-zinc-900");
+    expect(MOL_LIGHT.ink).toBe("text-zinc-100");
+    expect(MOL_LIGHT.line).toBe("border-zinc-700");
+    expect(MOL_LIGHT.bg).toBe("bg-zinc-950");
+  });
+
+  it("MOL_DARK singleton is never mutated", () => {
+    getPalette("#mutate", true);
+    expect(MOL_DARK.accent).toBe("bg-sky-400");
+    expect(MOL_DARK.online).toBe("bg-emerald-400");
+    expect(MOL_DARK.surface).toBe("bg-zinc-800");
+    expect(MOL_DARK.ink).toBe("text-zinc-100");
+    expect(MOL_DARK.line).toBe("border-zinc-700");
+    expect(MOL_DARK.bg).toBe("bg-zinc-950");
+  });
+
+  it("getPalette always returns a new object (no shared mutation risk)", () => {
+    const a = getPalette("#a", false);
+    const b = getPalette("#b", false);
+    expect(a).not.toBe(b);
+    expect(a.accent).not.toBe(b.accent);
+  });
+});
+
+// ─── MobileAccentProvider tests ───────────────────────────────────────────────
+
+describe("MobileAccentProvider", () => {
+  beforeEach(() => {
+    setDataTheme("light");
+  });
+
+  afterEach(() => {
+    cleanup();
+    if (typeof document !== "undefined") {
+      document.documentElement.dataset.theme = "";
+    }
+  });
+
+  it("renders children", () => {
+    render(
+      <MobileAccentProvider accent={null}>
+        <span data-testid="child">Hello</span>
+      </MobileAccentProvider>,
+    );
+    expect(screen.getByTestId("child")).toBeTruthy();
+  });
+
+  // usePalette hook reads data-theme from <html> to determine light/dark.
+  // In the test environment, data-theme is empty, which falls through to
+  // the "light" default in usePalette, giving MOL_LIGHT.
+  it("usePalette(false) without provider → MOL_LIGHT", () => {
+    setDataTheme("light");
+    function ShowPalette() {
+      const p = usePalette(false);
+      return <span data-testid="accent-light">{p.accent}</span>;
+    }
+    render(<ShowPalette />);
+    expect(screen.getByTestId("accent-light").textContent).toBe(MOL_LIGHT.accent);
+  });
+
+  it("usePalette(true) without provider → MOL_DARK when data-theme=dark", () => {
+    setDataTheme("dark");
+    function ShowPalette() {
+      const p = usePalette(true);
+      return <span data-testid="accent-dark">{p.accent}</span>;
+    }
+    render(<ShowPalette />);
+    expect(screen.getByTestId("accent-dark").textContent).toBe(MOL_DARK.accent);
+  });
+});

canvas/src/lib/palette-context.tsx

@@ -0,0 +1,167 @@
+"use client";
+
+/**
+ * palette-context.tsx
+ *
+ * Mobile canvas accent palette system.
+ *
+ * - MOL_LIGHT / MOL_DARK  — immutable base singletons
+ * - getPalette(accent, isDark) — returns base palette or accent-overridden copy
+ * - normalizeStatus(status, isDark) — maps workspace status → online dot color
+ * - tierCode(tier) — maps tier number → display label
+ * - MobileAccentProvider — React context that propagates accent override
+ * - usePalette(allowAccentOverride) — hook; returns the effective palette
+ */
+
+import { createContext, useContext } from "react";
+
+// ─── Types ─────────────────────────────────────────────────────────────────────
+
+export interface Palette {
+  /** Accent colour (CSS colour string). */
+  accent: string;
+  /** Online indicator colour (CSS class string, e.g. "bg-emerald-400"). */
+  online: string;
+  /** Surface background colour class. */
+  surface: string;
+  /** Primary text colour class. */
+  ink: string;
+  /** Border/divider colour class. */
+  line: string;
+  /** Background colour class. */
+  bg: string;
+  /** Tier display code, e.g. "T1". */
+  tier: string;
+}
+
+// ─── Singleton base palettes ────────────────────────────────────────────────────
+
+/** Light-mode base palette — must never be mutated. */
+export const MOL_LIGHT: Readonly<Palette> = Object.freeze({
+  accent: "bg-blue-500",
+  online: "bg-emerald-400",
+  surface: "bg-zinc-900",
+  ink: "text-zinc-100",
+  line: "border-zinc-700",
+  bg: "bg-zinc-950",
+  tier: "T1",
+});
+
+/** Dark-mode base palette — must never be mutated. */
+export const MOL_DARK: Readonly<Palette> = Object.freeze({
+  accent: "bg-sky-400",
+  online: "bg-emerald-400",
+  surface: "bg-zinc-800",
+  ink: "text-zinc-100",
+  line: "border-zinc-700",
+  bg: "bg-zinc-950",
+  tier: "T1",
+});
+
+// ─── Pure helpers ─────────────────────────────────────────────────────────────
+
+/**
+ * Maps workspace status string → online dot colour class.
+ * Returns the appropriate green for light/dark mode.
+ */
+export function normalizeStatus(
+  status: string,
+  _isDark: boolean,
+): string {
+  if (status === "online" || status === "degraded") {
+    return "bg-emerald-400";
+  }
+  if (status === "failed") {
+    return "bg-red-400";
+  }
+  if (status === "paused" || status === "not_configured") {
+    return "bg-amber-400";
+  }
+  return "bg-zinc-400";
+}
+
+/**
+ * Maps tier number → display code.
+ */
+export function tierCode(tier: number): string {
+  return `T${tier}`;
+}
+
+/**
+ * Returns the effective palette.
+ *
+ * - `accent = null` → base palette (light or dark) unchanged
+ * - `accent = basePalette.accent` → base palette unchanged (identity guard)
+ * - `accent = "#custom"` → copy with `accent` and `online` overridden
+ *
+ * Always returns a new object; neither MOL_LIGHT nor MOL_DARK is ever mutated.
+ */
+export function getPalette(
+  accent: string | null,
+  isDark: boolean,
+): Palette {
+  const base: Readonly<Palette> = isDark ? MOL_DARK : MOL_LIGHT;
+
+  // null accent → use base unchanged
+  if (accent === null) return { ...base };
+
+  // identity guard — accent same as base accent → no override needed
+  if (accent === base.accent) return { ...base };
+
+  // Custom accent: override accent + online to keep them in sync
+  return { ...base, accent, online: normalizeStatus("online", isDark) };
+}
+
+// ─── Context ──────────────────────────────────────────────────────────────────
+
+type MobileAccentContextValue = {
+  /** Override accent colour (null = no override, use default). */
+  accent: string | null;
+};
+
+const MobileAccentContext = createContext<MobileAccentContextValue>({
+  accent: null,
+});
+
+export { MobileAccentContext };
+
+/**
+ * Renders children inside the accent override context.
+ */
+export function MobileAccentProvider({
+  accent,
+  children,
+}: {
+  accent: string | null;
+  children: React.ReactNode;
+}) {
+  return (
+    <MobileAccentContext.Provider value={{ accent }}>
+      {children}
+    </MobileAccentContext.Provider>
+  );
+}
+
+// ─── Hook ─────────────────────────────────────────────────────────────────────
+
+/**
+ * Returns the effective `Palette` for the current context.
+ *
+ * @param allowAccentOverride  When false, always returns the base palette
+ *                              even when an override is set (useful for
+ *                              non-accent-aware child components).
+ */
+export function usePalette(allowAccentOverride: boolean): Palette {
+  const { accent } = useContext(MobileAccentContext);
+
+  // Resolved from the OS-level theme preference. In a real app this would
+  // be derived from useTheme().resolvedTheme; for this hook we default
+  // to light (the safe default for SSR / component-library use).
+  // We read data-theme from <html> to stay in sync with the theme system.
+  const isDark =
+    typeof document !== "undefined" &&
+    document.documentElement.dataset.theme === "dark";
+
+  const effectiveAccent = allowAccentOverride ? accent : null;
+  return getPalette(effectiveAccent, isDark);
+}

workspace-server/internal/bundle/exporter_test.go

@@ -0,0 +1,261 @@
+package bundle
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+// ---------------------------------------------------------------------------
+// extractDescription
+// ---------------------------------------------------------------------------
+
+func TestExtractDescription_WithFrontmatter(t *testing.T) {
+	// YAML frontmatter is skipped; first non-comment, non-empty line after
+	// the closing `---` is the description.
+	content := `---
+title: My Workspace
+---
+# This is a comment
+This is the description line.
+Another line.`
+	got := extractDescription(content)
+	if got != "This is the description line." {
+		t.Errorf("got %q, want %q", got, "This is the description line.")
+	}
+}
+
+func TestExtractDescription_NoFrontmatter(t *testing.T) {
+	// No frontmatter: first non-comment, non-empty line is returned.
+	content := `# Copyright header
+My workspace description
+Another line.`
+	got := extractDescription(content)
+	if got != "My workspace description" {
+		t.Errorf("got %q, want %q", got, "My workspace description")
+	}
+}
+
+func TestExtractDescription_CommentOnly(t *testing.T) {
+	// All content is comments or empty → empty string.
+	content := `# comment only
+# another comment
+`
+	got := extractDescription(content)
+	if got != "" {
+		t.Errorf("got %q, want empty string", got)
+	}
+}
+
+func TestExtractDescription_EmptyInput(t *testing.T) {
+	got := extractDescription("")
+	if got != "" {
+		t.Errorf("got %q, want empty string", got)
+	}
+}
+
+func TestExtractDescription_UnclosedFrontmatter(t *testing.T) {
+	// With no closing `---`, inFrontmatter stays true after the opening
+	// delimiter, so all subsequent lines are skipped and "" is returned.
+	// This is the documented behaviour: without a closing delimiter,
+	// all lines are considered frontmatter.
+	content := `---
+title: No closing delimiter
+This is the description.`
+	got := extractDescription(content)
+	if got != "" {
+		t.Errorf("unclosed frontmatter: got %q, want empty string", got)
+	}
+}
+
+func TestExtractDescription_FrontmatterThenCommentThenContent(t *testing.T) {
+	content := `---
+tags: [test]
+---
+# internal comment
+Real description here.
+`
+	got := extractDescription(content)
+	if got != "Real description here." {
+		t.Errorf("got %q, want %q", got, "Real description here.")
+	}
+}
+
+func TestExtractDescription_BlankLinesSkipped(t *testing.T) {
+	// Empty lines (len=0) are skipped; whitespace-only lines (spaces) are NOT
+	// skipped because len(line)>0. First non-comment, non-empty line is returned.
+	content := "\n\n\n\nA. Description\nB. Should not be returned.\n"
+	got := extractDescription(content)
+	if got != "A. Description" {
+		t.Errorf("got %q, want %q", got, "A. Description")
+	}
+}
+
+// ---------------------------------------------------------------------------
+// splitLines
+// ---------------------------------------------------------------------------
+
+func TestSplitLines_Basic(t *testing.T) {
+	got := splitLines("a\nb\nc")
+	want := []string{"a", "b", "c"}
+	if len(got) != len(want) {
+		t.Fatalf("len=%d, want %d", len(got), len(want))
+	}
+	for i := range want {
+		if got[i] != want[i] {
+			t.Errorf("got[%d]=%q, want %q", i, got[i], want[i])
+		}
+	}
+}
+
+func TestSplitLines_TrailingNewline(t *testing.T) {
+	got := splitLines("line1\nline2\n")
+	want := []string{"line1", "line2"}
+	if len(got) != len(want) {
+		t.Errorf("trailing newline: got %v, want %v", got, want)
+	}
+}
+
+func TestSplitLines_NoNewline(t *testing.T) {
+	got := splitLines("no newline")
+	want := []string{"no newline"}
+	if len(got) != 1 || got[0] != want[0] {
+		t.Errorf("got %v, want %v", got, want)
+	}
+}
+
+func TestSplitLines_EmptyString(t *testing.T) {
+	got := splitLines("")
+	if len(got) != 0 {
+		t.Errorf("empty string: got %v, want []", got)
+	}
+}
+
+func TestSplitLines_OnlyNewlines(t *testing.T) {
+	got := splitLines("\n\n\n")
+	// Three consecutive '\n' characters → s[start:i] at each '\n' gives
+	// the empty string between newlines → 3 empty segments.
+	// (No trailing segment because start == len(s) at the end.)
+	if len(got) != 3 {
+		t.Errorf("only newlines: got %v (len=%d), want 3 empty strings", got, len(got))
+	}
+	for i, s := range got {
+		if s != "" {
+			t.Errorf("got[%d]=%q, want empty string", i, s)
+		}
+	}
+}
+
+func TestSplitLines_MultipleConsecutiveNewlines(t *testing.T) {
+	got := splitLines("a\n\n\nb")
+	// a\n\n\nb → ["a", "", "", "b"]
+	if len(got) != 4 {
+		t.Errorf("consecutive newlines: got %v (len=%d)", got, len(got))
+	}
+	if got[0] != "a" || got[3] != "b" {
+		t.Errorf("first/last: got %v, want [a, ..., b]", got)
+	}
+}
+
+// ---------------------------------------------------------------------------
+// findConfigDir
+// ---------------------------------------------------------------------------
+
+func TestFindConfigDir_NameMatch(t *testing.T) {
+	tmp := t.TempDir()
+
+	// Create two sub-dirs; only the one with matching name should be found.
+	mustMkdir(filepath.Join(tmp, "workspace-a"))
+	mustWrite(filepath.Join(tmp, "workspace-a", "config.yaml"),
+		"name: other-workspace\ntier: 1\n")
+
+	mustMkdir(filepath.Join(tmp, "workspace-b"))
+	mustWrite(filepath.Join(tmp, "workspace-b", "config.yaml"),
+		"name: target-workspace\nruntime: claude-code\n")
+
+	got := findConfigDir(tmp, "target-workspace")
+	want := filepath.Join(tmp, "workspace-b")
+	if got != want {
+		t.Errorf("got %q, want %q", got, want)
+	}
+}
+
+func TestFindConfigDir_NoMatch_UsesFallback(t *testing.T) {
+	tmp := t.TempDir()
+
+	mustMkdir(filepath.Join(tmp, "first"))
+	mustWrite(filepath.Join(tmp, "first", "config.yaml"), "name: workspace-a\n")
+
+	mustMkdir(filepath.Join(tmp, "second"))
+	mustWrite(filepath.Join(tmp, "second", "config.yaml"), "name: workspace-b\n")
+
+	// No exact name match → fallback to the first directory with a config.yaml.
+	got := findConfigDir(tmp, "nonexistent")
+	want := filepath.Join(tmp, "first")
+	if got != want {
+		t.Errorf("no match: got %q, want fallback %q", got, want)
+	}
+}
+
+func TestFindConfigDir_MissingDir(t *testing.T) {
+	got := findConfigDir("/nonexistent/path/for/findConfigDir", "any-name")
+	if got != "" {
+		t.Errorf("missing dir: got %q, want empty string", got)
+	}
+}
+
+func TestFindConfigDir_NoSubdirs(t *testing.T) {
+	tmp := t.TempDir()
+	// Empty directory → no matches, no fallback.
+	got := findConfigDir(tmp, "any")
+	if got != "" {
+		t.Errorf("empty dir: got %q, want empty string", got)
+	}
+}
+
+// ---------------------------------------------------------------------------
+// Helpers
+// ---------------------------------------------------------------------------
+
+func mustMkdir(path string) {
+	os.MkdirAll(path, 0o755)
+}
+
+func mustWrite(path, content string) {
+	os.WriteFile(path, []byte(content), 0o644)
+}
+
+// ---------------------------------------------------------------------------
+// findConfigDir
+// ---------------------------------------------------------------------------
+
+func TestFindConfigDir_SubdirWithoutConfig(t *testing.T) {
+	tmp := t.TempDir()
+	mustMkdir(filepath.Join(tmp, "empty-skill"))
+	// Sub-dir without config.yaml → skipped.
+	got := findConfigDir(tmp, "any")
+	if got != "" {
+		t.Errorf("no config.yaml: got %q, want empty string", got)
+	}
+}
+
+func TestFindConfigDir_FirstWithConfigIsFallback(t *testing.T) {
+	// When name doesn't match, fallback is the FIRST dir with config.yaml,
+	// not the last. Confirm ordering by creating three dirs.
+	tmp := t.TempDir()
+
+	mustMkdir(filepath.Join(tmp, "a"))
+	mustWrite(filepath.Join(tmp, "a", "config.yaml"), "name: alpha\n")
+
+	mustMkdir(filepath.Join(tmp, "b"))
+	mustWrite(filepath.Join(tmp, "b", "config.yaml"), "name: beta\n")
+
+	mustMkdir(filepath.Join(tmp, "c"))
+	mustWrite(filepath.Join(tmp, "c", "config.yaml"), "name: gamma\n")
+
+	got := findConfigDir(tmp, "nonexistent")
+	want := filepath.Join(tmp, "a") // first dir with config.yaml
+	if got != want {
+		t.Errorf("fallback order: got %q, want first-with-config %q", got, want)
+	}
+}

workspace-server/internal/bundle/importer_test.go

@@ -0,0 +1,317 @@
+package bundle
+
+import (
+	"testing"
+)
+
+func TestBuildBundleConfigFiles_EmptyBundle(t *testing.T) {
+	b := &Bundle{}
+	files := buildBundleConfigFiles(b)
+	if len(files) != 0 {
+		t.Errorf("empty bundle: want 0 files, got %d", len(files))
+	}
+}
+
+func TestBuildBundleConfigFiles_SystemPromptOnly(t *testing.T) {
+	b := &Bundle{
+		SystemPrompt: "You are a helpful assistant.",
+	}
+	files := buildBundleConfigFiles(b)
+	if n := len(files); n != 1 {
+		t.Fatalf("system-prompt only: want 1 file, got %d", n)
+	}
+	if content, ok := files["system-prompt.md"]; !ok {
+		t.Fatal("missing system-prompt.md")
+	} else if string(content) != "You are a helpful assistant." {
+		t.Errorf("system-prompt content: got %q", string(content))
+	}
+}
+
+func TestBuildBundleConfigFiles_ConfigYamlOnly(t *testing.T) {
+	b := &Bundle{
+		Prompts: map[string]string{
+			"config.yaml": "runtime: langgraph\ntier: 2\n",
+		},
+	}
+	files := buildBundleConfigFiles(b)
+	if n := len(files); n != 1 {
+		t.Fatalf("config.yaml only: want 1 file, got %d", n)
+	}
+	if content, ok := files["config.yaml"]; !ok {
+		t.Fatal("missing config.yaml")
+	} else if string(content) != "runtime: langgraph\ntier: 2\n" {
+		t.Errorf("config.yaml content: got %q", string(content))
+	}
+}
+
+func TestBuildBundleConfigFiles_SystemPromptAndConfigYaml(t *testing.T) {
+	b := &Bundle{
+		SystemPrompt: "Be concise.",
+		Prompts: map[string]string{
+			"config.yaml": "runtime: langgraph\n",
+		},
+	}
+	files := buildBundleConfigFiles(b)
+	if n := len(files); n != 2 {
+		t.Fatalf("system-prompt + config.yaml: want 2 files, got %d", n)
+	}
+	if _, ok := files["system-prompt.md"]; !ok {
+		t.Error("missing system-prompt.md")
+	}
+	if _, ok := files["config.yaml"]; !ok {
+		t.Error("missing config.yaml")
+	}
+}
+
+func TestBuildBundleConfigFiles_Skills(t *testing.T) {
+	b := &Bundle{
+		Skills: []BundleSkill{
+			{
+				ID:   "web-search",
+				Files: map[string]string{"readme.md": "# Web Search\n"},
+			},
+			{
+				ID:   "code-interpreter",
+				Files: map[string]string{"readme.md": "# Code Interpreter\n"},
+			},
+		},
+	}
+	files := buildBundleConfigFiles(b)
+	// 2 skills × 1 file each = 2 files
+	if n := len(files); n != 2 {
+		t.Fatalf("skills: want 2 files, got %d", n)
+	}
+	if _, ok := files["skills/web-search/readme.md"]; !ok {
+		t.Error("missing skills/web-search/readme.md")
+	}
+	if _, ok := files["skills/code-interpreter/readme.md"]; !ok {
+		t.Error("missing skills/code-interpreter/readme.md")
+	}
+}
+
+func TestBuildBundleConfigFiles_SkillSubPaths(t *testing.T) {
+	b := &Bundle{
+		Skills: []BundleSkill{
+			{
+				ID: "multi-file",
+				Files: map[string]string{
+					"readme.md":        "# Multi",
+					"instructions.txt": "Step 1, Step 2",
+				},
+			},
+		},
+	}
+	files := buildBundleConfigFiles(b)
+	if n := len(files); n != 2 {
+		t.Fatalf("skill with sub-paths: want 2 files, got %d", n)
+	}
+	if _, ok := files["skills/multi-file/readme.md"]; !ok {
+		t.Error("missing skills/multi-file/readme.md")
+	}
+	if _, ok := files["skills/multi-file/instructions.txt"]; !ok {
+		t.Error("missing skills/multi-file/instructions.txt")
+	}
+}
+
+func TestBuildBundleConfigFiles_EmptySystemPrompt(t *testing.T) {
+	b := &Bundle{
+		SystemPrompt: "",
+		Prompts: map[string]string{
+			"config.yaml": "runtime: langgraph\n",
+		},
+	}
+	files := buildBundleConfigFiles(b)
+	// Empty system-prompt should not produce a file
+	if n := len(files); n != 1 {
+		t.Errorf("empty system-prompt: want 1 file, got %d", n)
+	}
+}
+
+func TestBuildBundleConfigFiles_EmptyPrompts(t *testing.T) {
+	b := &Bundle{
+		Prompts: map[string]string{},
+	}
+	files := buildBundleConfigFiles(b)
+	if n := len(files); n != 0 {
+		t.Errorf("empty prompts map: want 0 files, got %d", n)
+	}
+}
+
+func TestBuildBundleConfigFiles_emptyBundle(t *testing.T) {
+	b := &Bundle{}
+	files := buildBundleConfigFiles(b)
+	if len(files) != 0 {
+		t.Errorf("expected empty map for empty bundle, got %d entries", len(files))
+	}
+}
+
+func TestBuildBundleConfigFiles_systemPrompt(t *testing.T) {
+	b := &Bundle{SystemPrompt: "You are a helpful assistant."}
+	files := buildBundleConfigFiles(b)
+	if len(files) != 1 {
+		t.Fatalf("expected 1 file, got %d", len(files))
+	}
+	if string(files["system-prompt.md"]) != "You are a helpful assistant." {
+		t.Errorf("unexpected system prompt content: %q", files["system-prompt.md"])
+	}
+}
+
+func TestBuildBundleConfigFiles_configYaml(t *testing.T) {
+	b := &Bundle{Prompts: map[string]string{
+		"config.yaml": "runtime: langgraph\nmodel: claude-sonnet-4-20250514\n",
+	}}
+	files := buildBundleConfigFiles(b)
+	if len(files) != 1 {
+		t.Fatalf("expected 1 file, got %d", len(files))
+	}
+	if string(files["config.yaml"]) != "runtime: langgraph\nmodel: claude-sonnet-4-20250514\n" {
+		t.Errorf("unexpected config.yaml content: %q", files["config.yaml"])
+	}
+}
+
+func TestBuildBundleConfigFiles_systemPromptAndConfigYaml(t *testing.T) {
+	b := &Bundle{
+		SystemPrompt: "# System",
+		Prompts:     map[string]string{"config.yaml": "runtime: langgraph"},
+	}
+	files := buildBundleConfigFiles(b)
+	if len(files) != 2 {
+		t.Fatalf("expected 2 files, got %d", len(files))
+	}
+	if _, ok := files["system-prompt.md"]; !ok {
+		t.Error("missing system-prompt.md")
+	}
+	if _, ok := files["config.yaml"]; !ok {
+		t.Error("missing config.yaml")
+	}
+}
+
+func TestBuildBundleConfigFiles_skills(t *testing.T) {
+	b := &Bundle{
+		Skills: []BundleSkill{
+			{
+				ID:          "web-search",
+				Name:        "Web Search",
+				Description: "Search the web",
+				Files:       map[string]string{"readme.md": "# Web Search"},
+			},
+			{
+				ID:          "code-runner",
+				Name:        "Code Runner",
+				Description: "Execute code",
+				Files:       map[string]string{"handler.py": "print('hello')"},
+			},
+		},
+	}
+	files := buildBundleConfigFiles(b)
+	if len(files) != 2 {
+		t.Fatalf("expected 2 skill files, got %d", len(files))
+	}
+
+	if content, ok := files["skills/web-search/readme.md"]; !ok {
+		t.Error("missing skills/web-search/readme.md")
+	} else if string(content) != "# Web Search" {
+		t.Errorf("unexpected readme.md: %q", content)
+	}
+
+	if _, ok := files["skills/code-runner/handler.py"]; !ok {
+		t.Error("missing skills/code-runner/handler.py")
+	}
+}
+
+func TestBuildBundleConfigFiles_skillsWithSubPaths(t *testing.T) {
+	b := &Bundle{
+		Skills: []BundleSkill{
+			{
+				ID:    "nested-skill",
+				Files: map[string]string{"src/main.py": "def main(): pass", "pyproject.toml": "[tool.foo]"},
+			},
+		},
+	}
+	files := buildBundleConfigFiles(b)
+	if len(files) != 2 {
+		t.Fatalf("expected 2 files, got %d", len(files))
+	}
+	if _, ok := files["skills/nested-skill/src/main.py"]; !ok {
+		t.Error("missing skills/nested-skill/src/main.py")
+	}
+	if _, ok := files["skills/nested-skill/pyproject.toml"]; !ok {
+		t.Error("missing skills/nested-skill/pyproject.toml")
+	}
+}
+
+func TestBuildBundleConfigFiles_skipsEmptyPrompts(t *testing.T) {
+	b := &Bundle{Prompts: map[string]string{}}
+	files := buildBundleConfigFiles(b)
+	if len(files) != 0 {
+		t.Errorf("expected 0 files for empty prompts map, got %d", len(files))
+	}
+}
+
+func TestBuildBundleConfigFiles_skipsMissingConfigYaml(t *testing.T) {
+	b := &Bundle{
+		SystemPrompt: "# My Prompt",
+		Prompts:      map[string]string{"other.yaml": "something: else"},
+	}
+	files := buildBundleConfigFiles(b)
+	if len(files) != 1 {
+		t.Fatalf("expected 1 file (system-prompt only), got %d", len(files))
+	}
+	if _, ok := files["config.yaml"]; ok {
+		t.Error("config.yaml should not be written when not in Prompts")
+	}
+}
+
+func TestNilIfEmpty_emptyString(t *testing.T) {
+	result := nilIfEmpty("")
+	if result != nil {
+		t.Errorf("expected nil for empty string, got %v", result)
+	}
+}
+
+func TestNilIfEmpty_nonEmptyString(t *testing.T) {
+	result := nilIfEmpty("hello")
+	if result == nil {
+		t.Fatal("expected non-nil result for non-empty string")
+	}
+	if result != "hello" {
+		t.Errorf("expected hello, got %q", result)
+	}
+}
+
+func TestNilIfEmpty_whitespaceString(t *testing.T) {
+	// Whitespace is not empty — nilIfEmpty only checks for zero-length
+	result := nilIfEmpty("   ")
+	if result == nil {
+		t.Error("expected non-nil for whitespace string")
+	} else if result != "   " {
+		t.Errorf("expected '   ', got %q", result)
+	}
+}
+
+func TestNilIfEmpty_EmptyString(t *testing.T) {
+	got := nilIfEmpty("")
+	if got != nil {
+		t.Errorf("nilIfEmpty(\"\"): want nil, got %v", got)
+	}
+}
+
+func TestNilIfEmpty_NonEmptyString(t *testing.T) {
+	got := nilIfEmpty("hello")
+	if got == nil {
+		t.Fatal("nilIfEmpty(\"hello\"): want \"hello\", got nil")
+	}
+	if s, ok := got.(string); !ok || s != "hello" {
+		t.Errorf("nilIfEmpty(\"hello\"): got %v (%T)", got, got)
+	}
+}
+
+func TestNilIfEmpty_Whitespace(t *testing.T) {
+	got := nilIfEmpty("   ")
+	if got == nil {
+		t.Fatal("nilIfEmpty(\"   \"): want \"   \", got nil (whitespace is not empty)")
+	}
+	if s, ok := got.(string); !ok || s != "   " {
+		t.Errorf("nilIfEmpty(\"   \"): got %v (%T)", got, got)
+	}
+}

workspace-server/internal/handlers/delegation_extract_response_text_test.go

@@ -0,0 +1,224 @@
+package handlers
+
+import (
+	"encoding/json"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// extractResponseText tests — walks A2A JSON-RPC response bodies and
+// returns the first text part, falling back to raw body on parse failures.
+
+func TestExtractResponseText_PartsWithTextKind(t *testing.T) {
+	resp := map[string]interface{}{
+		"result": map[string]interface{}{
+			"parts": []interface{}{
+				map[string]interface{}{"kind": "text", "text": "hello world"},
+				map[string]interface{}{"kind": "text", "text": "second part"},
+			},
+		},
+	}
+	body, _ := json.Marshal(resp)
+	assert.Equal(t, "hello world", extractResponseText(body))
+}
+
+func TestExtractResponseText_PartNotTextKind(t *testing.T) {
+	resp := map[string]interface{}{
+		"result": map[string]interface{}{
+			"parts": []interface{}{
+				map[string]interface{}{"kind": "image", "data": "base64..."},
+				map[string]interface{}{"kind": "text", "text": "visible"},
+			},
+		},
+	}
+	body, _ := json.Marshal(resp)
+	assert.Equal(t, "visible", extractResponseText(body))
+}
+
+func TestExtractResponseText_PartsEmpty(t *testing.T) {
+	// Empty parts array — falls through to artifacts, then raw body
+	resp := map[string]interface{}{
+		"result": map[string]interface{}{
+			"parts":     []interface{}{},
+			"artifacts": []interface{}{},
+		},
+	}
+	body, _ := json.Marshal(resp)
+	// Falls through to raw body (which is the JSON string)
+	result := extractResponseText(body)
+	assert.NotEmpty(t, result)
+}
+
+func TestExtractResponseText_ArtifactPartsWithText(t *testing.T) {
+	resp := map[string]interface{}{
+		"result": map[string]interface{}{
+			"parts": []interface{}{},
+			"artifacts": []interface{}{
+				map[string]interface{}{
+					"kind": "file",
+					"parts": []interface{}{
+						map[string]interface{}{"kind": "text", "text": "artifact text"},
+					},
+				},
+			},
+		},
+	}
+	body, _ := json.Marshal(resp)
+	assert.Equal(t, "artifact text", extractResponseText(body))
+}
+
+func TestExtractResponseText_ArtifactPartNotTextKind(t *testing.T) {
+	resp := map[string]interface{}{
+		"result": map[string]interface{}{
+			"parts": []interface{}{},
+			"artifacts": []interface{}{
+				map[string]interface{}{
+					"kind": "code",
+					"parts": []interface{}{
+						map[string]interface{}{"kind": "image", "data": "..."},
+						map[string]interface{}{"kind": "text", "text": "code comment"},
+					},
+				},
+			},
+		},
+	}
+	body, _ := json.Marshal(resp)
+	assert.Equal(t, "code comment", extractResponseText(body))
+}
+
+func TestExtractResponseText_ArtifactsEmpty(t *testing.T) {
+	resp := map[string]interface{}{
+		"result": map[string]interface{}{
+			"parts":     []interface{}{},
+			"artifacts": []interface{}{},
+		},
+	}
+	body, _ := json.Marshal(resp)
+	result := extractResponseText(body)
+	// Falls back to raw body
+	assert.Equal(t, string(body), result)
+}
+
+func TestExtractResponseText_NoResult(t *testing.T) {
+	// No "result" key at all — falls back to raw body
+	body := []byte(`{"error": {"code": -32600, "message": "Invalid Request"}}`)
+	result := extractResponseText(body)
+	assert.Equal(t, string(body), result)
+}
+
+func TestExtractResponseText_ResultNotMap(t *testing.T) {
+	// result is a string, not a map — falls back to raw body
+	body := []byte(`{"result": "just a string"}`)
+	result := extractResponseText(body)
+	assert.Equal(t, string(body), result)
+}
+
+func TestExtractResponseText_NonJSONBody(t *testing.T) {
+	// Non-JSON bytes — returns the raw string
+	body := []byte("plain text response, not JSON at all")
+	result := extractResponseText(body)
+	assert.Equal(t, "plain text response, not JSON at all", result)
+}
+
+func TestExtractResponseText_PartWithNilText(t *testing.T) {
+	// Text field is nil — kind is "text" but text is nil, should skip
+	resp := map[string]interface{}{
+		"result": map[string]interface{}{
+			"parts": []interface{}{
+				map[string]interface{}{"kind": "text", "text": nil},
+				map[string]interface{}{"kind": "text", "text": "found"},
+			},
+		},
+	}
+	body, _ := json.Marshal(resp)
+	assert.Equal(t, "found", extractResponseText(body))
+}
+
+func TestExtractResponseText_ArtifactPartWithNilText(t *testing.T) {
+	resp := map[string]interface{}{
+		"result": map[string]interface{}{
+			"parts": []interface{}{},
+			"artifacts": []interface{}{
+				map[string]interface{}{
+					"parts": []interface{}{
+						map[string]interface{}{"kind": "text", "text": nil},
+						map[string]interface{}{"kind": "text", "text": "artifact-found"},
+					},
+				},
+			},
+		},
+	}
+	body, _ := json.Marshal(resp)
+	assert.Equal(t, "artifact-found", extractResponseText(body))
+}
+
+func TestExtractResponseText_PartsWithNonMapElement(t *testing.T) {
+	// parts contains a non-map element — should be skipped gracefully
+	resp := map[string]interface{}{
+		"result": map[string]interface{}{
+			"parts": []interface{}{
+				"not a map",
+				123,
+				nil,
+				map[string]interface{}{"kind": "text", "text": "parsed"},
+			},
+		},
+	}
+	body, _ := json.Marshal(resp)
+	assert.Equal(t, "parsed", extractResponseText(body))
+}
+
+func TestExtractResponseText_ArtifactWithNonMapElement(t *testing.T) {
+	resp := map[string]interface{}{
+		"result": map[string]interface{}{
+			"parts": []interface{}{},
+			"artifacts": []interface{}{
+				"not a map",
+				nil,
+				map[string]interface{}{
+					"parts": []interface{}{
+						"not a map",
+						map[string]interface{}{"kind": "text", "text": "safe"},
+					},
+				},
+			},
+		},
+	}
+	body, _ := json.Marshal(resp)
+	assert.Equal(t, "safe", extractResponseText(body))
+}
+
+func TestExtractResponseText_PartKindNotString(t *testing.T) {
+	// kind is an integer, not a string — should be skipped
+	resp := map[string]interface{}{
+		"result": map[string]interface{}{
+			"parts": []interface{}{
+				map[string]interface{}{"kind": 123, "text": "ignored"},
+				map[string]interface{}{"kind": "text", "text": "found"},
+			},
+		},
+	}
+	body, _ := json.Marshal(resp)
+	assert.Equal(t, "found", extractResponseText(body))
+}
+
+func TestExtractResponseText_EmptyResponse(t *testing.T) {
+	body := []byte("{}")
+	result := extractResponseText(body)
+	// Falls back to raw "{}"
+	assert.Equal(t, "{}", result)
+}
+
+func TestExtractResponseText_NilBody(t *testing.T) {
+	// nil byte slice — string(nil) = ""
+	result := extractResponseText(nil)
+	assert.Equal(t, "", result)
+}
+
+func TestExtractResponseText_WhitespaceBody(t *testing.T) {
+	body := []byte("   \n\t  ")
+	result := extractResponseText(body)
+	// Unmarshals to empty map, no result, returns raw string
+	assert.Equal(t, "   \n\t  ", result)
+}

workspace-server/internal/handlers/discovery_filter_test.go

@@ -0,0 +1,160 @@
+package handlers
+
+import (
+	"testing"
+)
+
+// filterPeersByQuery tests — nil-safe role/name filtering for peer discovery.
+
+func TestFilterPeersByQuery_EmptyQueryNoOp(t *testing.T) {
+	peers := []map[string]interface{}{
+		{"name": "foo", "role": "bar"},
+		{"name": "baz", "role": "qux"},
+	}
+	result := filterPeersByQuery(peers, "")
+	if len(result) != 2 {
+		t.Errorf("empty query: expected 2, got %d", len(result))
+	}
+}
+
+func TestFilterPeersByQuery_WhitespaceQueryNoOp(t *testing.T) {
+	peers := []map[string]interface{}{
+		{"name": "foo", "role": "bar"},
+	}
+	result := filterPeersByQuery(peers, "   ")
+	if len(result) != 1 {
+		t.Errorf("whitespace-only query: expected 1, got %d", len(result))
+	}
+}
+
+func TestFilterPeersByQuery_MatchName(t *testing.T) {
+	peers := []map[string]interface{}{
+		{"name": "backend-agent", "role": "sre"},
+		{"name": "frontend-agent", "role": "ui"},
+	}
+	result := filterPeersByQuery(peers, "backend")
+	if len(result) != 1 || result[0]["name"] != "backend-agent" {
+		t.Errorf("expected backend-agent, got %v", result)
+	}
+}
+
+func TestFilterPeersByQuery_MatchRole(t *testing.T) {
+	peers := []map[string]interface{}{
+		{"name": "agent-alpha", "role": "security engineer"},
+		{"name": "agent-beta", "role": "devops"},
+	}
+	result := filterPeersByQuery(peers, "engineer")
+	if len(result) != 1 || result[0]["name"] != "agent-alpha" {
+		t.Errorf("expected agent-alpha, got %v", result)
+	}
+}
+
+func TestFilterPeersByQuery_CaseInsensitive(t *testing.T) {
+	peers := []map[string]interface{}{
+		{"name": "AgentX", "role": "SRE"},
+	}
+	result := filterPeersByQuery(peers, "AGENTx")
+	if len(result) != 1 {
+		t.Errorf("expected 1 match (case-insensitive), got %d", len(result))
+	}
+}
+
+func TestFilterPeersByQuery_NilRoleNoPanic(t *testing.T) {
+	// This is the regression case for #730: queryPeerMaps explicitly sets
+	// peer["role"] = nil when the DB role is empty string. Before the fix,
+	// p["role"].(string) panics on nil. After the fix, it returns "" and
+	// no match occurs — which is the correct behaviour.
+	defer func() {
+		if r := recover(); r != nil {
+			t.Errorf("filterPeersByQuery panicked on nil role: %v", r)
+		}
+	}()
+	peers := []map[string]interface{}{
+		{"name": "some-agent", "role": nil},
+	}
+	result := filterPeersByQuery(peers, "some-agent")
+	if len(result) != 1 {
+		t.Errorf("expected 1 match by name, got %d", len(result))
+	}
+}
+
+func TestFilterPeersByQuery_NilRoleQueryNoMatch(t *testing.T) {
+	// When role is nil and query does not match name, nothing matches.
+	defer func() {
+		if r := recover(); r != nil {
+			t.Errorf("filterPeersByQuery panicked on nil role: %v", r)
+		}
+	}()
+	peers := []map[string]interface{}{
+		{"name": "agent-alpha", "role": nil},
+	}
+	result := filterPeersByQuery(peers, "no-match")
+	if len(result) != 0 {
+		t.Errorf("expected 0 matches, got %d", len(result))
+	}
+}
+
+func TestFilterPeersByQuery_NilNameNoPanic(t *testing.T) {
+	// Defensive check: name could also theoretically be nil.
+	defer func() {
+		if r := recover(); r != nil {
+			t.Errorf("filterPeersByQuery panicked on nil name: %v", r)
+		}
+	}()
+	peers := []map[string]interface{}{
+		{"name": nil, "role": "sre"},
+	}
+	result := filterPeersByQuery(peers, "sre")
+	if len(result) != 1 {
+		t.Errorf("expected 1 match by role, got %d", len(result))
+	}
+}
+
+func TestFilterPeersByQuery_BothNilNoPanic(t *testing.T) {
+	defer func() {
+		if r := recover(); r != nil {
+			t.Errorf("filterPeersByQuery panicked on nil name+role: %v", r)
+		}
+	}()
+	peers := []map[string]interface{}{
+		{"name": nil, "role": nil},
+	}
+	result := filterPeersByQuery(peers, "")
+	if len(result) != 1 {
+		t.Errorf("empty query with nil name/role: expected 1, got %d", len(result))
+	}
+	result = filterPeersByQuery(peers, "anything")
+	if len(result) != 0 {
+		t.Errorf("non-empty query with nil name/role: expected 0, got %d", len(result))
+	}
+}
+
+func TestFilterPeersByQuery_NoMatches(t *testing.T) {
+	peers := []map[string]interface{}{
+		{"name": "alpha", "role": "beta"},
+		{"name": "gamma", "role": "delta"},
+	}
+	result := filterPeersByQuery(peers, "zzz")
+	if len(result) != 0 {
+		t.Errorf("expected 0, got %d", len(result))
+	}
+}
+
+func TestFilterPeersByQuery_EmptyPeers(t *testing.T) {
+	result := filterPeersByQuery([]map[string]interface{}{}, "query")
+	if len(result) != 0 {
+		t.Errorf("empty peers: expected 0, got %d", len(result))
+	}
+}
+
+func TestFilterPeersByQuery_MultipleMatches(t *testing.T) {
+	peers := []map[string]interface{}{
+		{"name": "backend-alpha", "role": "eng"},
+		{"name": "backend-beta", "role": "eng"},
+		{"name": "frontend", "role": "ui"},
+	}
+	result := filterPeersByQuery(peers, "backend")
+	if len(result) != 2 {
+		t.Errorf("expected 2 backend matches, got %d", len(result))
+	}
+}

workspace-server/internal/handlers/instructions_test.go

@@ -14,22 +14,69 @@ import (
 	"github.com/gin-gonic/gin"
 )
 
-// ── List ─────────────────────────────────────────────────────────────────────────
-
-func TestInstructionsHandler_List_EmptyResult(t *testing.T) {
-	mock := setupTestDB(t)
-	handler := NewInstructionsHandler()
-
-	mock.ExpectQuery("SELECT id, scope, scope_target, title, content, priority, enabled, created_at, updated_at FROM platform_instructions WHERE 1=1 ORDER BY scope, priority DESC, created_at").
-		WillReturnRows(sqlmock.NewRows([]string{
-			"id", "scope", "scope_target", "title", "content", "priority", "enabled", "created_at", "updated_at",
-		}))
+// ─── request helpers ───────────────────────────────────────────────────────────
 
+func newPostRequest(path string, body interface{}) (*httptest.ResponseRecorder, *gin.Context) {
 	w := httptest.NewRecorder()
 	c, _ := gin.CreateTestContext(w)
-	c.Request = httptest.NewRequest("GET", "/instructions", nil)
+	raw, _ := json.Marshal(body)
+	c.Request = httptest.NewRequest(http.MethodPost, path, bytes.NewReader(raw))
+	c.Request.Header.Set("Content-Type", "application/json")
+	return w, c
+}
 
-	handler.List(c)
+func newPutRequest(path string, body interface{}) (*httptest.ResponseRecorder, *gin.Context) {
+	w := httptest.NewRecorder()
+	c, _ := gin.CreateTestContext(w)
+	raw, _ := json.Marshal(body)
+	c.Request = httptest.NewRequest(http.MethodPut, path, bytes.NewReader(raw))
+	c.Request.Header.Set("Content-Type", "application/json")
+	return w, c
+}
+
+func newDeleteRequest(path string) (*httptest.ResponseRecorder, *gin.Context) {
+	w := httptest.NewRecorder()
+	c, _ := gin.CreateTestContext(w)
+	c.Request = httptest.NewRequest(http.MethodDelete, path, nil)
+	return w, c
+}
+
+func newGetRequest(path string) (*httptest.ResponseRecorder, *gin.Context) {
+	w := httptest.NewRecorder()
+	c, _ := gin.CreateTestContext(w)
+	c.Request = httptest.NewRequest(http.MethodGet, path, nil)
+	return w, c
+}
+
+// ─── mock row helpers ─────────────────────────────────────────────────────────
+
+// instructionCols matches the SELECT in List/Resolve.
+var instructionCols = []string{
+	"id", "scope", "scope_target", "title", "content",
+	"priority", "enabled", "created_at", "updated_at",
+}
+
+// resolveCols matches the SELECT in Resolve (scope, title, content).
+var resolveCols = []string{"scope", "title", "content"}
+
+// ─── List ────────────────────────────────────────────────────────────────────
+
+func TestInstructionsList_ByWorkspaceID(t *testing.T) {
+	mock := setupTestDB(t)
+	h := NewInstructionsHandler()
+
+	wsID := "ws-123-abc"
+	w, c := newGetRequest("/instructions?workspace_id=" + wsID)
+	c.Request = httptest.NewRequest(http.MethodGet, "/instructions?workspace_id="+wsID, nil)
+
+	rows := sqlmock.NewRows(instructionCols).
+		AddRow("inst-1", "global", nil, "Be helpful", "Always be helpful.", 10, true, time.Now(), time.Now()).
+		AddRow("inst-2", "workspace", &wsID, "Use Claude", "Use Claude Code.", 5, true, time.Now(), time.Now())
+	mock.ExpectQuery("SELECT id, scope, scope_target, title, content, priority, enabled, created_at, updated_at").
+		WithArgs(wsID).
+		WillReturnRows(rows)
+
+	h.List(c)
 
 	if w.Code != http.StatusOK {
 		t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String())
@@ -164,33 +211,104 @@ func TestInstructionsHandler_Create_Success(t *testing.T) {
 	if w.Code != http.StatusCreated {
 		t.Fatalf("expected 201, got %d: %s", w.Code, w.Body.String())
 	}
-	var resp map[string]string
-	if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil {
-		t.Fatalf("invalid JSON: %v", err)
+	var out map[string]string
+	if err := json.Unmarshal(w.Body.Bytes(), &out); err != nil {
+		t.Fatalf("response not valid JSON: %v", err)
 	}
-	if resp["id"] != "new-inst-id" {
-		t.Errorf("expected id 'new-inst-id', got %q", resp["id"])
+	if out["id"] != "new-inst-1" {
+		t.Errorf("expected id new-inst-1, got %s", out["id"])
 	}
 	if err := mock.ExpectationsWereMet(); err != nil {
-		t.Fatalf("unmet expectations: %v", err)
+		t.Errorf("unmet expectations: %v", err)
 	}
 }
 
-func TestInstructionsHandler_Create_InvalidScope(t *testing.T) {
-	setupTestDB(t)
-	handler := NewInstructionsHandler()
+func TestInstructionsCreate_ValidWorkspace(t *testing.T) {
+	mock := setupTestDB(t)
+	h := NewInstructionsHandler()
+	wsTarget := "ws-xyz-789"
 
-	body, _ := json.Marshal(map[string]interface{}{
-		"scope":   "team",
-		"title":   "Test",
-		"content": "Test content",
+	w, c := newPostRequest("/instructions", map[string]interface{}{
+		"scope":        "workspace",
+		"scope_target": wsTarget,
+		"title":        "Use Claude Code",
+		"content":      "Prefer Claude Code for all tasks.",
+		"priority":     5,
 	})
-	w := httptest.NewRecorder()
-	c, _ := gin.CreateTestContext(w)
-	c.Request = httptest.NewRequest("POST", "/instructions", bytes.NewReader(body))
-	c.Request.Header.Set("Content-Type", "application/json")
 
-	handler.Create(c)
+	mock.ExpectQuery("INSERT INTO platform_instructions").
+		WithArgs("workspace", &wsTarget, "Use Claude Code", "Prefer Claude Code for all tasks.", 5).
+		WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("ws-inst-2"))
+
+	h.Create(c)
+
+	if w.Code != http.StatusCreated {
+		t.Fatalf("expected 201, got %d: %s", w.Code, w.Body.String())
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("unmet expectations: %v", err)
+	}
+}
+
+func TestInstructionsCreate_MissingScope(t *testing.T) {
+	setupTestDB(t)
+	h := NewInstructionsHandler()
+
+	w, c := newPostRequest("/instructions", map[string]interface{}{
+		"title":   "Missing Scope",
+		"content": "This has no scope.",
+	})
+
+	h.Create(c)
+
+	if w.Code != http.StatusBadRequest {
+		t.Fatalf("expected 400, got %d: %s", w.Code, w.Body.String())
+	}
+}
+
+func TestInstructionsCreate_MissingTitle(t *testing.T) {
+	setupTestDB(t)
+	h := NewInstructionsHandler()
+
+	w, c := newPostRequest("/instructions", map[string]interface{}{
+		"scope":   "global",
+		"content": "Has no title.",
+	})
+
+	h.Create(c)
+
+	if w.Code != http.StatusBadRequest {
+		t.Fatalf("expected 400, got %d: %s", w.Code, w.Body.String())
+	}
+}
+
+func TestInstructionsCreate_MissingContent(t *testing.T) {
+	setupTestDB(t)
+	h := NewInstructionsHandler()
+
+	w, c := newPostRequest("/instructions", map[string]interface{}{
+		"scope": "global",
+		"title": "Has no content",
+	})
+
+	h.Create(c)
+
+	if w.Code != http.StatusBadRequest {
+		t.Fatalf("expected 400, got %d: %s", w.Code, w.Body.String())
+	}
+}
+
+func TestInstructionsCreate_InvalidScope(t *testing.T) {
+	setupTestDB(t)
+	h := NewInstructionsHandler()
+
+	w, c := newPostRequest("/instructions", map[string]interface{}{
+		"scope":   "team",
+		"title":   "Bad Scope",
+		"content": "Team scope is not supported yet.",
+	})
+
+	h.Create(c)
 
 	if w.Code != http.StatusBadRequest {
 		t.Fatalf("expected 400, got %d: %s", w.Code, w.Body.String())
@@ -262,55 +380,489 @@ func TestInstructionsHandler_Create_TitleTooLong(t *testing.T) {
 	}
 }
 
-func TestInstructionsHandler_Create_WorkspaceScopeWithScopeTarget(t *testing.T) {
+func TestInstructionsCreate_DBError(t *testing.T) {
 	mock := setupTestDB(t)
-	handler := NewInstructionsHandler()
-	wsID := "ws-abc-123"
+	h := NewInstructionsHandler()
+
+	w, c := newPostRequest("/instructions", map[string]interface{}{
+		"scope":   "global",
+		"title":   "DB Error",
+		"content": "This will fail.",
+	})
 
 	mock.ExpectQuery("INSERT INTO platform_instructions").
-		WithArgs("workspace", &wsID, "WS rule", "Use HTTPS", 10).
-		WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("ws-inst-1"))
+		WillReturnError(errors.New("connection refused"))
 
-	body, _ := json.Marshal(map[string]interface{}{
-		"scope":        "workspace",
-		"scope_target": wsID,
-		"title":        "WS rule",
-		"content":      "Use HTTPS",
-		"priority":     10,
+	h.Create(c)
+
+	if w.Code != http.StatusInternalServerError {
+		t.Fatalf("expected 500, got %d: %s", w.Code, w.Body.String())
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("unmet expectations: %v", err)
+	}
+}
+
+// ─── Update ──────────────────────────────────────────────────────────────────
+
+func TestInstructionsUpdate_ValidPartial(t *testing.T) {
+	mock := setupTestDB(t)
+	h := NewInstructionsHandler()
+
+	instID := "inst-update-1"
+	newTitle := "Updated Title"
+	w, c := newPutRequest("/instructions/"+instID, map[string]interface{}{
+		"title": newTitle,
 	})
-	w := httptest.NewRecorder()
-	c, _ := gin.CreateTestContext(w)
-	c.Request = httptest.NewRequest("POST", "/instructions", bytes.NewReader(body))
-	c.Request.Header.Set("Content-Type", "application/json")
+	c.Params = []gin.Param{{Key: "id", Value: instID}}
 
-	handler.Create(c)
+	mock.ExpectExec("UPDATE platform_instructions SET").
+		WithArgs(instID, &newTitle, sqlmock.AnyArg(), sqlmock.AnyArg(), sqlmock.AnyArg()).
+		WillReturnResult(sqlmock.NewResult(0, 1))
+
+	h.Update(c)
+
+	if w.Code != http.StatusOK {
+		t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String())
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("unmet expectations: %v", err)
+	}
+}
+
+func TestInstructionsUpdate_AllFields(t *testing.T) {
+	mock := setupTestDB(t)
+	h := NewInstructionsHandler()
+
+	instID := "inst-update-2"
+	title := "Full Update"
+	content := "New content body."
+	priority := 20
+	enabled := false
+	w, c := newPutRequest("/instructions/"+instID, map[string]interface{}{
+		"title":    title,
+		"content":  content,
+		"priority": priority,
+		"enabled":  enabled,
+	})
+	c.Params = []gin.Param{{Key: "id", Value: instID}}
+
+	mock.ExpectExec("UPDATE platform_instructions SET").
+		WithArgs(instID, &title, &content, &priority, &enabled).
+		WillReturnResult(sqlmock.NewResult(0, 1))
+
+	h.Update(c)
+
+	if w.Code != http.StatusOK {
+		t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String())
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("unmet expectations: %v", err)
+	}
+}
+
+func TestInstructionsUpdate_ContentTooLong(t *testing.T) {
+	setupTestDB(t)
+	h := NewInstructionsHandler()
+
+	instID := "inst-too-long"
+	longContent := string(make([]byte, maxInstructionContentLen+1))
+	w, c := newPutRequest("/instructions/"+instID, map[string]interface{}{
+		"content": longContent,
+	})
+	c.Params = []gin.Param{{Key: "id", Value: instID}}
+
+	h.Update(c)
+
+	if w.Code != http.StatusBadRequest {
+		t.Fatalf("expected 400, got %d: %s", w.Code, w.Body.String())
+	}
+}
+
+func TestInstructionsUpdate_TitleTooLong(t *testing.T) {
+	setupTestDB(t)
+	h := NewInstructionsHandler()
+
+	instID := "inst-title-long"
+	longTitle := string(make([]byte, 201))
+	w, c := newPutRequest("/instructions/"+instID, map[string]interface{}{
+		"title": longTitle,
+	})
+	c.Params = []gin.Param{{Key: "id", Value: instID}}
+
+	h.Update(c)
+
+	if w.Code != http.StatusBadRequest {
+		t.Fatalf("expected 400, got %d: %s", w.Code, w.Body.String())
+	}
+}
+
+func TestInstructionsUpdate_NotFound(t *testing.T) {
+	mock := setupTestDB(t)
+	h := NewInstructionsHandler()
+
+	instID := "inst-missing"
+	w, c := newPutRequest("/instructions/"+instID, map[string]interface{}{
+		"title": "New Title",
+	})
+	c.Params = []gin.Param{{Key: "id", Value: instID}}
+
+	mock.ExpectExec("UPDATE platform_instructions SET").
+		WillReturnResult(sqlmock.NewResult(0, 0))
+
+	h.Update(c)
+
+	if w.Code != http.StatusNotFound {
+		t.Fatalf("expected 404, got %d: %s", w.Code, w.Body.String())
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("unmet expectations: %v", err)
+	}
+}
+
+func TestInstructionsUpdate_DBError(t *testing.T) {
+	mock := setupTestDB(t)
+	h := NewInstructionsHandler()
+
+	instID := "inst-db-err"
+	w, c := newPutRequest("/instructions/"+instID, map[string]interface{}{
+		"title": "Error Update",
+	})
+	c.Params = []gin.Param{{Key: "id", Value: instID}}
+
+	mock.ExpectExec("UPDATE platform_instructions SET").
+		WillReturnError(errors.New("connection refused"))
+
+	h.Update(c)
+
+	if w.Code != http.StatusInternalServerError {
+		t.Fatalf("expected 500, got %d: %s", w.Code, w.Body.String())
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("unmet expectations: %v", err)
+	}
+}
+
+// ─── Delete ───────────────────────────────────────────────────────────────────
+
+func TestInstructionsDelete_Valid(t *testing.T) {
+	mock := setupTestDB(t)
+	h := NewInstructionsHandler()
+
+	instID := "inst-delete-1"
+	w, c := newDeleteRequest("/instructions/" + instID)
+	c.Params = []gin.Param{{Key: "id", Value: instID}}
+
+	mock.ExpectExec(`DELETE FROM platform_instructions WHERE id = \$1`).
+		WithArgs(instID).
+		WillReturnResult(sqlmock.NewResult(0, 1))
+
+	h.Delete(c)
+
+	if w.Code != http.StatusOK {
+		t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String())
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("unmet expectations: %v", err)
+	}
+}
+
+func TestInstructionsDelete_NotFound(t *testing.T) {
+	mock := setupTestDB(t)
+	h := NewInstructionsHandler()
+
+	instID := "inst-not-there"
+	w, c := newDeleteRequest("/instructions/" + instID)
+	c.Params = []gin.Param{{Key: "id", Value: instID}}
+
+	mock.ExpectExec(`DELETE FROM platform_instructions WHERE id = \$1`).
+		WithArgs(instID).
+		WillReturnResult(sqlmock.NewResult(0, 0))
+
+	h.Delete(c)
+
+	if w.Code != http.StatusNotFound {
+		t.Fatalf("expected 404, got %d: %s", w.Code, w.Body.String())
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("unmet expectations: %v", err)
+	}
+}
+
+func TestInstructionsDelete_DBError(t *testing.T) {
+	mock := setupTestDB(t)
+	h := NewInstructionsHandler()
+
+	instID := "inst-del-err"
+	w, c := newDeleteRequest("/instructions/" + instID)
+	c.Params = []gin.Param{{Key: "id", Value: instID}}
+
+	mock.ExpectExec(`DELETE FROM platform_instructions WHERE id = \$1`).
+		WithArgs(instID).
+		WillReturnError(errors.New("connection refused"))
+
+	h.Delete(c)
+
+	if w.Code != http.StatusInternalServerError {
+		t.Fatalf("expected 500, got %d: %s", w.Code, w.Body.String())
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("unmet expectations: %v", err)
+	}
+}
+
+// ─── Resolve ──────────────────────────────────────────────────────────────────
+
+func TestInstructionsResolve_GlobalThenWorkspace(t *testing.T) {
+	mock := setupTestDB(t)
+	h := NewInstructionsHandler()
+
+	wsID := "ws-resolve-1"
+	w, c := newGetRequest("/workspaces/" + wsID + "/instructions/resolve")
+	c.Params = []gin.Param{{Key: "id", Value: wsID}}
+	c.Request = httptest.NewRequest(http.MethodGet, "/workspaces/"+wsID+"/instructions/resolve", nil)
+
+	rows := sqlmock.NewRows(resolveCols).
+		AddRow("global", "Be Helpful", "Always help the user.").
+		AddRow("global", "Stay on Topic", "Don't diverge.").
+		AddRow("workspace", "Use Claude Code", "Claude Code is the default runtime.")
+	mock.ExpectQuery("SELECT scope, title, content FROM platform_instructions").
+		WithArgs(wsID).
+		WillReturnRows(rows)
+
+	h.Resolve(c)
+
+	if w.Code != http.StatusOK {
+		t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String())
+	}
+	var out struct {
+		WorkspaceID   string `json:"workspace_id"`
+		Instructions string `json:"instructions"`
+	}
+	if err := json.Unmarshal(w.Body.Bytes(), &out); err != nil {
+		t.Fatalf("response not valid JSON: %v", err)
+	}
+	if out.WorkspaceID != wsID {
+		t.Errorf("expected workspace_id %s, got %s", wsID, out.WorkspaceID)
+	}
+	// Global section must come before workspace section.
+	if !bytes.Contains([]byte(out.Instructions), []byte("Platform-Wide Rules")) {
+		t.Error("instructions should contain 'Platform-Wide Rules' section")
+	}
+	if !bytes.Contains([]byte(out.Instructions), []byte("Role-Specific Rules")) {
+		t.Error("instructions should contain 'Role-Specific Rules' section")
+	}
+	// Global instructions must appear before workspace instructions.
+	idxGlobal := bytes.Index([]byte(out.Instructions), []byte("Platform-Wide Rules"))
+	idxWorkspace := bytes.Index([]byte(out.Instructions), []byte("Role-Specific Rules"))
+	if idxGlobal >= idxWorkspace {
+		t.Error("global section should appear before workspace section")
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("unmet expectations: %v", err)
+	}
+}
+
+func TestInstructionsResolve_EmptyWorkspace(t *testing.T) {
+	mock := setupTestDB(t)
+	h := NewInstructionsHandler()
+
+	wsID := "ws-empty"
+	w, c := newGetRequest("/workspaces/" + wsID + "/instructions/resolve")
+	c.Params = []gin.Param{{Key: "id", Value: wsID}}
+	c.Request = httptest.NewRequest(http.MethodGet, "/workspaces/"+wsID+"/instructions/resolve", nil)
+
+	rows := sqlmock.NewRows(resolveCols)
+	mock.ExpectQuery("SELECT scope, title, content FROM platform_instructions").
+		WithArgs(wsID).
+		WillReturnRows(rows)
+
+	h.Resolve(c)
+
+	if w.Code != http.StatusOK {
+		t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String())
+	}
+	var out struct {
+		Instructions string `json:"instructions"`
+	}
+	if err := json.Unmarshal(w.Body.Bytes(), &out); err != nil {
+		t.Fatalf("response not valid JSON: %v", err)
+	}
+	// No rows → builder writes nothing; empty string returned.
+	if out.Instructions != "" {
+		t.Errorf("expected empty instructions for empty workspace, got: %q", out.Instructions)
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("unmet expectations: %v", err)
+	}
+}
+
+func TestInstructionsResolve_DBError(t *testing.T) {
+	mock := setupTestDB(t)
+	h := NewInstructionsHandler()
+
+	wsID := "ws-err"
+	w, c := newGetRequest("/workspaces/" + wsID + "/instructions/resolve")
+	c.Params = []gin.Param{{Key: "id", Value: wsID}}
+	c.Request = httptest.NewRequest(http.MethodGet, "/workspaces/"+wsID+"/instructions/resolve", nil)
+
+	mock.ExpectQuery("SELECT scope, title, content FROM platform_instructions").
+		WithArgs(wsID).
+		WillReturnError(errors.New("connection refused"))
+
+	h.Resolve(c)
+
+	if w.Code != http.StatusInternalServerError {
+		t.Fatalf("expected 500, got %d: %s", w.Code, w.Body.String())
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("unmet expectations: %v", err)
+	}
+}
+
+func TestInstructionsResolve_MissingWorkspaceID(t *testing.T) {
+	setupTestDB(t)
+	h := NewInstructionsHandler()
+
+	w, c := newGetRequest("/workspaces//instructions/resolve")
+	c.Params = []gin.Param{{Key: "id", Value: ""}}
+
+	h.Resolve(c)
+
+	if w.Code != http.StatusBadRequest {
+		t.Fatalf("expected 400, got %d: %s", w.Code, w.Body.String())
+	}
+}
+
+// ─── scanInstructions edge cases ───────────────────────────────────────────────
+
+// NOTE: TestScanInstructions_ScanError was removed — go-sqlmock v1.5.2 does not
+// implement Go 1.25's sql.Rows.Next([]byte) bool method, so *sqlmock.Rows cannot
+// satisfy scanInstructions' interface. The test needs a sqlmock upgrade or a
+// different mocking strategy (tracked: internal issue).
+
+// ─── maxInstructionContentLen boundary ────────────────────────────────────────
+
+func TestInstructionsCreate_ContentExactlyAtLimit(t *testing.T) {
+	mock := setupTestDB(t)
+	h := NewInstructionsHandler()
+
+	exactContent := string(make([]byte, maxInstructionContentLen))
+	w, c := newPostRequest("/instructions", map[string]interface{}{
+		"scope":   "global",
+		"title":   "At Limit",
+		"content": exactContent,
+	})
+
+	mock.ExpectQuery("INSERT INTO platform_instructions").
+		WithArgs("global", nil, "At Limit", exactContent, 0).
+		WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("at-limit-1"))
+
+	h.Create(c)
+
+	// Exactly at limit must succeed (8192 chars is acceptable).
+	if w.Code != http.StatusCreated {
+		t.Fatalf("expected 201 for content at limit, got %d: %s", w.Code, w.Body.String())
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("unmet expectations: %v", err)
+	}
+}
+
+// ─── priority defaults ────────────────────────────────────────────────────────
+
+func TestInstructionsCreate_PriorityDefaultsToZero(t *testing.T) {
+	mock := setupTestDB(t)
+	h := NewInstructionsHandler()
+
+	// Body omits priority — expect it defaults to 0.
+	w, c := newPostRequest("/instructions", map[string]interface{}{
+		"scope":   "global",
+		"title":   "No Priority",
+		"content": "Default priority body.",
+	})
+
+	mock.ExpectQuery("INSERT INTO platform_instructions").
+		WithArgs("global", nil, "No Priority", "Default priority body.", 0).
+		WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("no-prio-1"))
+
+	h.Create(c)
 
 	if w.Code != http.StatusCreated {
 		t.Fatalf("expected 201, got %d: %s", w.Code, w.Body.String())
 	}
 	if err := mock.ExpectationsWereMet(); err != nil {
-		t.Fatalf("unmet expectations: %v", err)
+		t.Errorf("unmet expectations: %v", err)
 	}
 }
 
-// ── Update ────────────────────────────────────────────────────────────────────
+// ─── nil scope_target for global instructions ─────────────────────────────────
 
-func TestInstructionsHandler_Update_Success(t *testing.T) {
+func TestInstructionsCreate_GlobalScopeNilTarget(t *testing.T) {
 	mock := setupTestDB(t)
-	handler := NewInstructionsHandler()
+	h := NewInstructionsHandler()
 
-	mock.ExpectExec(regexp.QuoteMeta("UPDATE platform_instructions SET\n\t\t\t\ttitle = COALESCE($2, title),\n\t\t\t\tcontent = COALESCE($3, content),\n\t\t\t\tpriority = COALESCE($4, priority),\n\t\t\t\tenabled = COALESCE($5, enabled),\n\t\t\t\tupdated_at = NOW()\n\t\t\t\tWHERE id = $1")).
-		WithArgs("inst-1", sqlmock.AnyArg(), nil, nil, nil).
-		WillReturnResult(sqlmock.NewResult(0, 1))
+	w, c := newPostRequest("/instructions", map[string]interface{}{
+		"scope":   "global",
+		"title":   "Global Nil Target",
+		"content": "Global instruction.",
+	})
 
-	body, _ := json.Marshal(map[string]interface{}{"title": "Updated title"})
-	w := httptest.NewRecorder()
-	c, _ := gin.CreateTestContext(w)
-	c.Params = gin.Params{{Key: "id", Value: "inst-1"}}
-	c.Request = httptest.NewRequest("PUT", "/instructions/inst-1", bytes.NewReader(body))
-	c.Request.Header.Set("Content-Type", "application/json")
+	// For global scope, scope_target must be SQL NULL.
+	mock.ExpectQuery("INSERT INTO platform_instructions").
+		WithArgs("global", nil, "Global Nil Target", "Global instruction.", 0).
+		WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("global-nil-1"))
 
-	handler.Update(c)
+	h.Create(c)
+
+	if w.Code != http.StatusCreated {
+		t.Fatalf("expected 201, got %d: %s", w.Code, w.Body.String())
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("unmet expectations: %v", err)
+	}
+}
+
+// ─── workspace scope with empty string target (rejected) ─────────────────────
+
+func TestInstructionsCreate_WorkspaceScopeEmptyStringTarget(t *testing.T) {
+	setupTestDB(t)
+	h := NewInstructionsHandler()
+
+	empty := ""
+	w, c := newPostRequest("/instructions", map[string]interface{}{
+		"scope":        "workspace",
+		"scope_target": empty,
+		"title":        "Empty Target",
+		"content":      "Empty workspace target.",
+	})
+
+	h.Create(c)
+
+	if w.Code != http.StatusBadRequest {
+		t.Fatalf("expected 400 for empty string scope_target, got %d: %s", w.Code, w.Body.String())
+	}
+}
+
+// ─── Resolve: scope label transitions ────────────────────────────────────────
+
+func TestInstructionsResolve_ScopeTransitionOnlyGlobal(t *testing.T) {
+	mock := setupTestDB(t)
+	h := NewInstructionsHandler()
+
+	wsID := "ws-only-global"
+	w, c := newGetRequest("/workspaces/" + wsID + "/instructions/resolve")
+	c.Params = []gin.Param{{Key: "id", Value: wsID}}
+	c.Request = httptest.NewRequest(http.MethodGet, "/workspaces/"+wsID+"/instructions/resolve", nil)
+
+	rows := sqlmock.NewRows(resolveCols).
+		AddRow("global", "Rule One", "First rule.").
+		AddRow("global", "Rule Two", "Second rule.")
+	mock.ExpectQuery("SELECT scope, title, content FROM platform_instructions").
+		WithArgs(wsID).
+		WillReturnRows(rows)
+
+	h.Resolve(c)
 
 	if w.Code != http.StatusOK {
 		t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String())

workspace-server/internal/handlers/org_helpers.go

@@ -132,6 +132,34 @@ func expandWithEnv(s string, env map[string]string) string {
 	return b.String()
 }
 
+
+func isEnvIdentStart(c byte) bool {
+	return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || c == '_'
+}
+
+func isEnvIdentPart(c byte) bool {
+	return isEnvIdentStart(c) || (c >= '0' && c <= '9')
+}
+
+// expandEnvRef resolves a single variable reference extracted from s.
+//
+// Guards:
+//   - Empty key → "$$" escape, return "$"
+//   - key[0] not POSIX ident start → "$" + partial chars, return "$<chars>"
+//   - Key in env map → return the mapped value (template override wins)
+//   - Otherwise → only fall back to os.Getenv if the whole input string IS the
+//     variable reference (ref == whole).
+//
+// Bare $VAR format:
+//   $HOME (alone) → ref==whole → os.Getenv ✓  (host HOME is org-template HOME)
+//   $HOME/path (partial) → ref!=whole → literal "$HOME" ✓  (CWE-78: prevents host leak)
+//
+// Braced ${VAR} format:
+//   ${HOME} (alone) → ref==whole → os.Getenv ✓
+//   ${ROLE}/admin (partial) → ref!=whole → literal ✓
+//   "yes and ${NOT_SET}" (embedded) → ref!=whole → literal ✓
+//
+// This is the CWE-78 fix from commit a3a358f9.
 func expandEnvRef(key, ref, whole string, env map[string]string) string {
 	if key == "" {
 		return "$"
@@ -148,15 +176,8 @@ func expandEnvRef(key, ref, whole string, env map[string]string) string {
 	return ref
 }
 
-func isEnvIdentStart(c byte) bool {
-	return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || c == '_'
-}
 
-func isEnvIdentPart(c byte) bool {
-	return isEnvIdentStart(c) || (c >= '0' && c <= '9')
-}
-
-// loadWorkspaceEnv reads the org root .env and the workspace-specific .env
+// loadWorkspaceEnv reads the org root .env and the workspace-specific .env .env and the workspace-specific .env
 // (workspace overrides org root). Used by both secret injection and channel
 // config expansion.
 //

workspace-server/internal/handlers/org_helpers_loadWorkspaceEnv_test.go

@@ -0,0 +1,126 @@
+package handlers
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// setupOrgEnv creates a temp dir with an optional org .env file and returns the dir.
+func setupOrgEnv(t *testing.T, orgEnvContent string) string {
+	t.Helper()
+	dir := t.TempDir()
+	if orgEnvContent != "" {
+		require.NoError(t, os.WriteFile(filepath.Join(dir, ".env"), []byte(orgEnvContent), 0o600))
+	}
+	return dir
+}
+
+func Test_loadWorkspaceEnv_orgRootOnly(t *testing.T) {
+	org := setupOrgEnv(t, "ORG_VAR=orgval\nORG_DEBUG=true")
+	vars := loadWorkspaceEnv(org, "")
+	assert.Equal(t, "orgval", vars["ORG_VAR"])
+	assert.Equal(t, "true", vars["ORG_DEBUG"])
+}
+
+func Test_loadWorkspaceEnv_orgRootMissing(t *testing.T) {
+	// No .env at org root — should return empty map without error.
+	dir := t.TempDir()
+	vars := loadWorkspaceEnv(dir, "")
+	assertEmpty(t, vars)
+}
+
+func Test_loadWorkspaceEnv_workspaceEnvMerges(t *testing.T) {
+	org := setupOrgEnv(t, "SHARED=sharedval\nORG_ONLY=orgonly")
+	wsDir := filepath.Join(org, "myworkspace")
+	require.NoError(t, os.MkdirAll(wsDir, 0o700))
+	require.NoError(t, os.WriteFile(filepath.Join(wsDir, ".env"), []byte("WS_VAR=wsval\nSHARED=overridden"), 0o600))
+
+	vars := loadWorkspaceEnv(org, "myworkspace")
+	assert.Equal(t, "wsval", vars["WS_VAR"])
+	assert.Equal(t, "overridden", vars["SHARED"]) // workspace overrides org
+	assert.Equal(t, "orgonly", vars["ORG_ONLY"])   // org vars preserved
+}
+
+func Test_loadWorkspaceEnv_emptyFilesDir(t *testing.T) {
+	org := setupOrgEnv(t, "VAR=val")
+	vars := loadWorkspaceEnv(org, "")
+	assert.Equal(t, "val", vars["VAR"])
+}
+
+func Test_loadWorkspaceEnv_traversalRejects(t *testing.T) {
+	// #321 / CWE-22: filesDir "../../../etc" must not escape the org root.
+	// resolveInsideRoot rejects the traversal so workspace .env is skipped;
+	// org root .env is still loaded (it's before the guard).
+	org := setupOrgEnv(t, "INNOCENT=val\nSAFE_WS=wsval")
+	parent := filepath.Dir(org)
+	require.NoError(t, os.WriteFile(filepath.Join(parent, ".env"), []byte("MALICIOUS=evil"), 0o600))
+	// Also create a workspace dir inside org to prove it IS accessible normally.
+	wsDir := filepath.Join(org, "legit-workspace")
+	require.NoError(t, os.MkdirAll(wsDir, 0o700))
+	require.NoError(t, os.WriteFile(filepath.Join(wsDir, ".env"), []byte("WS_SECRET=ssh-key-123"), 0o600))
+
+	// Traversal is blocked.
+	vars := loadWorkspaceEnv(org, "../../../etc")
+	// Org root vars present; workspace vars blocked.
+	assert.Equal(t, "val", vars["INNOCENT"])
+	assert.Equal(t, "wsval", vars["SAFE_WS"]) // from org root .env
+	assert.Empty(t, vars["WS_SECRET"])        // workspace .env blocked by traversal guard
+	_, hasEvil := vars["MALICIOUS"]
+	assert.False(t, hasEvil, "MALICIOUS from escaped path must not appear")
+}
+
+func Test_loadWorkspaceEnv_traversalWithDots(t *testing.T) {
+	// A sibling-traversal attempt: go up one level then into a sibling dir.
+	// The sibling dir is NOT inside org, so it must be rejected.
+	org := setupOrgEnv(t, "INNOCENT=val")
+	parent := filepath.Dir(org)
+	require.NoError(t, os.MkdirAll(filepath.Join(parent, "sibling"), 0o700))
+	require.NoError(t, os.WriteFile(filepath.Join(parent, "sibling/.env"), []byte("LEAKED=secret"), 0o600))
+
+	vars := loadWorkspaceEnv(org, "../sibling")
+	// Org vars loaded; sibling vars blocked.
+	assert.Equal(t, "val", vars["INNOCENT"])
+	assert.Empty(t, vars["LEAKED"], "sibling traversal must be rejected")
+}
+
+func Test_loadWorkspaceEnv_absolutePathRejected(t *testing.T) {
+	// Absolute paths are rejected outright by resolveInsideRoot.
+	org := setupOrgEnv(t, "INNOCENT=val")
+	vars := loadWorkspaceEnv(org, "/etc")
+	assert.Equal(t, "val", vars["INNOCENT"]) // org root still loaded
+	assert.Empty(t, vars["SAFE_WS"])
+}
+
+func Test_loadWorkspaceEnv_dotPathRejected(t *testing.T) {
+	// "." resolves to the org root itself — this is NOT a traversal but
+	// would create org-root/.env which is the org root .env, not a
+	// workspace .env. resolveInsideRoot accepts this; the workspace .env
+	// path is org/.env, which IS the org root .env (already loaded).
+	// So the correct result is the org vars (same as org root, no change).
+	org := setupOrgEnv(t, "INNOCENT=val")
+	vars := loadWorkspaceEnv(org, ".")
+	// "." passes resolveInsideRoot (resolves to org root, which is valid).
+	// But workspace path org/.env is the same as org/.env already loaded.
+	assert.Equal(t, "val", vars["INNOCENT"])
+}
+
+func Test_loadWorkspaceEnv_emptyOrgRootReturnsEmpty(t *testing.T) {
+	vars := loadWorkspaceEnv("", "some/dir")
+	assertEmpty(t, vars)
+}
+
+func Test_loadWorkspaceEnv_missingWorkspaceDir(t *testing.T) {
+	org := setupOrgEnv(t, "ORG=val")
+	// Workspace dir doesn't exist — org vars still loaded.
+	vars := loadWorkspaceEnv(org, "nonexistent")
+	assert.Equal(t, "val", vars["ORG"])
+}
+
+func assertEmpty(t *testing.T, m map[string]string) {
+	t.Helper()
+	assert.Equal(t, 0, len(m), "expected empty map, got %v", m)
+}

workspace-server/internal/handlers/org_helpers_pure_test.go

@@ -287,7 +287,7 @@ func TestRenderCategoryRoutingYAML_StableOrdering(t *testing.T) {
 	if ai <= 0 || zi <= 0 || mi <= 0 {
 		t.Fatalf("could not locate all keys in output: %s", out)
 	}
-	if ai >= mi || mi >= zi {
+	if !(ai < mi && mi < zi) {
 		t.Errorf("keys not sorted: alpha=%d middle=%d zebra=%d, output:\n%s", ai, mi, zi, out)
 	}
 }

workspace-server/internal/handlers/org_helpers_security_test.go

@@ -16,7 +16,7 @@ import (
 func TestResolveInsideRoot_EmptyUserPath(t *testing.T) {
 	_, err := resolveInsideRoot("/safe/root", "")
 	if err == nil {
-		t.Fatal("empty userPath: expected error, got nil")
+		t.Fatalf("empty userPath: expected error, got nil")
 	}
 	if err.Error() != "path is empty" {
 		t.Errorf("empty userPath: got %q, want %q", err.Error(), "path is empty")
@@ -26,7 +26,7 @@ func TestResolveInsideRoot_EmptyUserPath(t *testing.T) {
 func TestResolveInsideRoot_AbsolutePathRejected(t *testing.T) {
 	_, err := resolveInsideRoot("/safe/root", "/etc/passwd")
 	if err == nil {
-		t.Fatal("absolute userPath: expected error, got nil")
+		t.Fatalf("absolute userPath: expected error, got nil")
 	}
 	if err.Error() != "absolute paths are not allowed" {
 		t.Errorf("absolute userPath: got %q, want %q", err.Error(), "absolute paths are not allowed")
@@ -44,6 +44,11 @@ func TestResolveInsideRoot_DotDotTraversal(t *testing.T) {
 	}
 }
 
+// TestResolveInsideRoot_DotDotWithIntermediate verifies that a/b/../../c does NOT
+// escape when root=/safe/root. After normalization: a/b/../.. = ., so a/b/../../c = c,
+// which is a valid descendant of /safe/root. The original test expected an error
+// but resolveInsideRoot correctly returns nil (the path stays within root).
+// The OFFSEC-006 concern is covered by ../../etc/passwd which DOES escape.
 func TestResolveInsideRoot_DotDotWithIntermediate(t *testing.T) {
 	// a/b/../../c normalises to "c" — a valid descendant inside any root.
 	// Must use t.TempDir() for a real filesystem path so filepath.Abs resolves.
@@ -93,14 +98,16 @@ func TestResolveInsideRoot_DotPathComponent(t *testing.T) {
 	if err != nil {
 		t.Fatalf("dot path component: unexpected error: %v", err)
 	}
-	if !strings.HasSuffix(got, "/subdir/file.txt") {
-		t.Errorf("dot path component: got %q, want suffix /subdir/file.txt", got)
+	// Verify the file component is subdir/file.txt regardless of root length.
+	suffix := string(filepath.Separator) + "subdir" + string(filepath.Separator) + "file.txt"
+	if !strings.HasSuffix(got, suffix) {
+		t.Errorf("dot path component: got %q, want suffix %q", got, suffix)
 	}
 }
 
 func TestResolveInsideRoot_NestedDotDotEscapes(t *testing.T) {
 	root := t.TempDir()
-	// a/../../b from /tmp/dirsomething → /tmp/b (escapes temp dir)
+	// a/../../b from /tmp/xyz → /tmp/b (escapes temp dir)
 	got, err := resolveInsideRoot(root, "a/../../b")
 	if err == nil {
 		t.Fatalf("nested dotdot: expected error, got %q", got)
@@ -188,15 +195,17 @@ func TestIsSafeRoleName_SpecialChars(t *testing.T) {
 }
 
 // ── mergeCategoryRouting ──────────────────────────────────────────────────────
+// Duplicate mergeCategoryRouting tests removed to avoid redeclaration with
+// org_helpers_pure_test.go. Only security-specific behaviour lives here.
 
-func TestMergeCategoryRouting_BothNil(t *testing.T) {
+func TestSecureRouting_BothNil(t *testing.T) {
 	got := mergeCategoryRouting(nil, nil)
 	if len(got) != 0 {
 		t.Errorf("both nil: got %v, want empty", got)
 	}
 }
 
-func TestMergeCategoryRouting_DefaultOnly(t *testing.T) {
+func TestSecureRouting_DefaultOnly(t *testing.T) {
 	defaultRouting := map[string][]string{
 		"security": {"Backend Engineer", "DevOps"},
 	}
@@ -209,7 +218,7 @@ func TestMergeCategoryRouting_DefaultOnly(t *testing.T) {
 	}
 }
 
-func TestMergeCategoryRouting_WorkspaceOnly(t *testing.T) {
+func TestSecureRouting_WorkspaceOnly(t *testing.T) {
 	wsRouting := map[string][]string{
 		"ui": {"Frontend Engineer"},
 	}
@@ -222,7 +231,7 @@ func TestMergeCategoryRouting_WorkspaceOnly(t *testing.T) {
 	}
 }
 
-func TestMergeCategoryRouting_MergeNoOverlap(t *testing.T) {
+func TestSecureRouting_MergeNoOverlap(t *testing.T) {
 	defaultRouting := map[string][]string{
 		"security": {"Backend Engineer"},
 	}
@@ -235,7 +244,7 @@ func TestMergeCategoryRouting_MergeNoOverlap(t *testing.T) {
 	}
 }
 
-func TestMergeCategoryRouting_WsOverrideDropsDefault(t *testing.T) {
+func TestSecureRouting_WsOverrideDropsDefault(t *testing.T) {
 	defaultRouting := map[string][]string{
 		"security": {"Backend Engineer", "DevOps"},
 	}
@@ -251,7 +260,34 @@ func TestMergeCategoryRouting_WsOverrideDropsDefault(t *testing.T) {
 	}
 }
 
-func TestMergeCategoryRouting_EmptyRolesInDefaultSkipped(t *testing.T) {
+func TestSecureRouting_EmptyListDropsCategory(t *testing.T) {
+	defaultRouting := map[string][]string{
+		"security": {"Backend Engineer"},
+		"ui":       {"Frontend Engineer"},
+	}
+	wsRouting := map[string][]string{
+		"security": {}, // empty list = opt out
+	}
+	got := mergeCategoryRouting(defaultRouting, wsRouting)
+	if _, exists := got["security"]; exists {
+		t.Error("empty ws list should delete the category from output")
+	}
+	if len(got["ui"]) != 1 {
+		t.Errorf("ui should still exist: got %v", got["ui"])
+	}
+}
+
+func TestSecureRouting_EmptyKeySkipped(t *testing.T) {
+	defaultRouting := map[string][]string{
+		"": {"Backend Engineer"},
+	}
+	got := mergeCategoryRouting(defaultRouting, nil)
+	if _, exists := got[""]; exists {
+		t.Error("empty key should be skipped")
+	}
+}
+
+func TestSecureRouting_EmptyRolesInDefaultSkipped(t *testing.T) {
 	defaultRouting := map[string][]string{
 		"security": {},
 	}
@@ -261,7 +297,7 @@ func TestMergeCategoryRouting_EmptyRolesInDefaultSkipped(t *testing.T) {
 	}
 }
 
-func TestMergeCategoryRouting_OriginalMapsUnmodified(t *testing.T) {
+func TestSecureRouting_OriginalMapsUnmodified(t *testing.T) {
 	defaultRouting := map[string][]string{
 		"security": {"Backend Engineer"},
 	}
@@ -276,3 +312,121 @@ func TestMergeCategoryRouting_OriginalMapsUnmodified(t *testing.T) {
 		t.Error("ws routing should be unmodified after merge")
 	}
 }
+
+// ── expandWithEnv ─────────────────────────────────────────────────────────────
+//
+// CWE-78 regression tests. The original fix (a3a358f9) ensures that partial
+// variable references like $HOME/path are NOT resolved via os.Getenv — the
+// host HOME env var must not leak into org template values. Only whole-string
+// references ($VAR or ${VAR}) may fall back to the host process environment.
+
+func TestExpandWithEnv_PartialRefDollarHomePath(t *testing.T) {
+	// $HOME/path must NOT resolve to the host's HOME env var.
+	// The literal $HOME must be returned as-is.
+	got := expandWithEnv("$HOME/path", nil)
+	if got != "$HOME/path" {
+		t.Errorf("$HOME/path: got %q, want literal $HOME/path", got)
+	}
+}
+
+func TestExpandWithEnv_PartialRefBracedRoleAdmin(t *testing.T) {
+	// ${ROLE}/admin — ROLE is not in env, so expand to the literal ${ROLE}/admin.
+	got := expandWithEnv("${ROLE}/admin", nil)
+	if got != "${ROLE}/admin" {
+		t.Errorf("${ROLE}/admin: got %q, want literal ${ROLE}/admin", got)
+	}
+}
+
+func TestExpandWithEnv_PartialRefMiddleOfString(t *testing.T) {
+	// $ROLE in the middle of a string — literal, not os.Getenv.
+	got := expandWithEnv("prefix/$ROLE/suffix", nil)
+	if got != "prefix/$ROLE/suffix" {
+		t.Errorf("prefix/$ROLE/suffix: got %q, want literal", got)
+	}
+}
+
+func TestExpandWithEnv_WholeVarInEnv(t *testing.T) {
+	// Whole-string $VAR that IS in env — env value wins.
+	env := map[string]string{"FOO": "barvalue"}
+	got := expandWithEnv("$FOO", env)
+	if got != "barvalue" {
+		t.Errorf("$FOO with FOO=barvalue: got %q, want barvalue", got)
+	}
+}
+
+func TestExpandWithEnv_WholeVarBracedInEnv(t *testing.T) {
+	// Whole-string ${VAR} that IS in env — env value wins.
+	env := map[string]string{"FOO": "barvalue"}
+	got := expandWithEnv("${FOO}", env)
+	if got != "barvalue" {
+		t.Errorf("${FOO} with FOO=barvalue: got %q, want barvalue", got)
+	}
+}
+
+func TestExpandWithEnv_WholeVarNotInEnvBare(t *testing.T) {
+	// Whole-string $VAR not in env — falls back to os.Getenv.
+	// If the host has the var, we get the host value. If not, empty.
+	// At minimum, the result must NOT be the literal "$UNDEFINED_VAR_9Z".
+	got := expandWithEnv("$UNDEFINED_VAR_9Z", nil)
+	if got == "$UNDEFINED_VAR_9Z" {
+		t.Errorf("$UNDEFINED_VAR_9Z: should expand (whole-string fallback to os.Getenv), got literal")
+	}
+}
+
+func TestExpandWithEnv_WholeVarNotInEnvBraced(t *testing.T) {
+	// Whole-string ${VAR} not in env — falls back to os.Getenv.
+	got := expandWithEnv("${UNDEFINED_VAR_9Z}", nil)
+	if got == "${UNDEFINED_VAR_9Z}" {
+		t.Errorf("${UNDEFINED_VAR_9Z}: should expand (whole-string fallback to os.Getenv), got literal")
+	}
+}
+
+func TestExpandWithEnv_EmptyString(t *testing.T) {
+	got := expandWithEnv("", map[string]string{"FOO": "bar"})
+	if got != "" {
+		t.Errorf("empty string: got %q, want empty", got)
+	}
+}
+
+func TestExpandWithEnv_NoVarRefs(t *testing.T) {
+	got := expandWithEnv("plain string with no vars", map[string]string{"FOO": "bar"})
+	if got != "plain string with no vars" {
+		t.Errorf("plain string: got %q, want unchanged", got)
+	}
+}
+
+func TestExpandWithEnv_MultipleVarRefs(t *testing.T) {
+	// Two vars, both whole — both expand from env.
+	env := map[string]string{"A": "alpha", "B": "beta"}
+	got := expandWithEnv("$A and $B and more", env)
+	if got != "alpha and beta and more" {
+		t.Errorf("multiple vars: got %q, want alpha and beta and more", got)
+	}
+}
+
+func TestExpandWithEnv_NumericVarRef(t *testing.T) {
+	// $5 — starts with digit, not a valid identifier start.
+	// Must return the literal "$5", not expand via os.Getenv.
+	got := expandWithEnv("$5", map[string]string{"5": "five"})
+	if got != "$5" {
+		t.Errorf("$5: got %q, want literal $5", got)
+	}
+}
+
+func TestExpandWithEnv_DollarEscape(t *testing.T) {
+	// $$ → both $ written literally (each $ is not followed by an identifier char,
+	// so it is written as-is). No special escape sequence for $$.
+	got := expandWithEnv("$$", nil)
+	if got != "$$" {
+		t.Errorf("$$: got %q, want literal $$", got)
+	}
+}
+
+func TestExpandWithEnv_MixedPartialAndWhole(t *testing.T) {
+	// $A is in env (whole), $HOME is partial — only $A expands.
+	env := map[string]string{"A": "alpha"}
+	got := expandWithEnv("$A at $HOME", env)
+	if got != "alpha at $HOME" {
+		t.Errorf("$A at $HOME: got %q, want alpha at $HOME", got)
+	}
+}

workspace-server/internal/handlers/org_helpers_walk_test.go

@@ -0,0 +1,191 @@
+package handlers
+
+import (
+	"errors"
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// walkOrgWorkspaceNames tests — recursive collection of non-empty workspace names.
+
+func TestWalkOrgWorkspaceNames_EmptySlice(t *testing.T) {
+	var names []string
+	walkOrgWorkspaceNames([]OrgWorkspace{}, &names)
+	assert.Empty(t, names)
+}
+
+func TestWalkOrgWorkspaceNames_SingleNode(t *testing.T) {
+	var names []string
+	walkOrgWorkspaceNames([]OrgWorkspace{{Name: "my-workspace"}}, &names)
+	assert.Equal(t, []string{"my-workspace"}, names)
+}
+
+func TestWalkOrgWorkspaceNames_SingleNodeEmptyName(t *testing.T) {
+	var names []string
+	walkOrgWorkspaceNames([]OrgWorkspace{{Name: ""}}, &names)
+	assert.Empty(t, names)
+}
+
+func TestWalkOrgWorkspaceNames_NestedChildren(t *testing.T) {
+	var names []string
+	tree := []OrgWorkspace{
+		{
+			Name: "parent",
+			Children: []OrgWorkspace{
+				{Name: "child-a"},
+				{Name: "child-b"},
+			},
+		},
+	}
+	walkOrgWorkspaceNames(tree, &names)
+	assert.Equal(t, []string{"parent", "child-a", "child-b"}, names)
+}
+
+func TestWalkOrgWorkspaceNames_DeeplyNested(t *testing.T) {
+	var names []string
+	tree := []OrgWorkspace{
+		{
+			Name: "level0",
+			Children: []OrgWorkspace{
+				{
+					Name: "level1",
+					Children: []OrgWorkspace{
+						{
+							Name: "level2",
+							Children: []OrgWorkspace{
+								{Name: "level3"},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+	walkOrgWorkspaceNames(tree, &names)
+	assert.Equal(t, []string{"level0", "level1", "level2", "level3"}, names)
+}
+
+func TestWalkOrgWorkspaceNames_SkipsEmptyNames(t *testing.T) {
+	var names []string
+	tree := []OrgWorkspace{
+		{Name: "a"},
+		{Name: ""},
+		{Name: "b"},
+	}
+	walkOrgWorkspaceNames(tree, &names)
+	assert.Equal(t, []string{"a", "b"}, names)
+}
+
+func TestWalkOrgWorkspaceNames_Siblings(t *testing.T) {
+	var names []string
+	tree := []OrgWorkspace{
+		{Name: "team"},
+		{Name: "alpha"},
+		{Name: "beta"},
+	}
+	walkOrgWorkspaceNames(tree, &names)
+	assert.Equal(t, []string{"team", "alpha", "beta"}, names)
+}
+
+func TestWalkOrgWorkspaceNames_MultipleRoots(t *testing.T) {
+	var names []string
+	tree := []OrgWorkspace{
+		{Name: "root-a", Children: []OrgWorkspace{{Name: "child-a"}}},
+		{Name: "root-b", Children: []OrgWorkspace{{Name: "child-b"}}},
+	}
+	walkOrgWorkspaceNames(tree, &names)
+	assert.Equal(t, []string{"root-a", "child-a", "root-b", "child-b"}, names)
+}
+
+func TestWalkOrgWorkspaceNames_SpawningFalseStillWalks(t *testing.T) {
+	// The comment in the source is explicit: spawning:false subtrees are
+	// still walked. Empty names within those subtrees are still skipped.
+	var names []string
+	yes := true
+	no := false
+	tree := []OrgWorkspace{
+		{
+			Name: "parent",
+			Children: []OrgWorkspace{
+				{Name: "spawning-child", Spawning: &yes},
+				{Name: "non-spawning-child", Spawning: &no},
+				{Name: ""},
+			},
+		},
+	}
+	walkOrgWorkspaceNames(tree, &names)
+	assert.Equal(t, []string{"parent", "spawning-child", "non-spawning-child"}, names)
+}
+
+// resolveProvisionConcurrency tests — env-var parsing with sensible fallback.
+
+func TestResolveProvisionConcurrency_Default(t *testing.T) {
+	os.Unsetenv("MOLECULE_PROVISION_CONCURRENCY")
+	defer os.Unsetenv("MOLECULE_PROVISION_CONCURRENCY")
+	val := resolveProvisionConcurrency()
+	assert.Equal(t, defaultProvisionConcurrency, val)
+}
+
+func TestResolveProvisionConcurrency_ValidPositiveInt(t *testing.T) {
+	os.Setenv("MOLECULE_PROVISION_CONCURRENCY", "5")
+	defer os.Unsetenv("MOLECULE_PROVISION_CONCURRENCY")
+	val := resolveProvisionConcurrency()
+	assert.Equal(t, 5, val)
+}
+
+func TestResolveProvisionConcurrency_ZeroUnlimited(t *testing.T) {
+	os.Setenv("MOLECULE_PROVISION_CONCURRENCY", "0")
+	defer os.Unsetenv("MOLECULE_PROVISION_CONCURRENCY")
+	val := resolveProvisionConcurrency()
+	// Zero is mapped to 1<<20 (unlimited semantics with finite cap)
+	assert.Equal(t, 1<<20, val)
+}
+
+func TestResolveProvisionConcurrency_NegativeFallsBack(t *testing.T) {
+	os.Setenv("MOLECULE_PROVISION_CONCURRENCY", "-1")
+	defer os.Unsetenv("MOLECULE_PROVISION_CONCURRENCY")
+	val := resolveProvisionConcurrency()
+	assert.Equal(t, defaultProvisionConcurrency, val)
+}
+
+func TestResolveProvisionConcurrency_NonIntegerFallsBack(t *testing.T) {
+	os.Setenv("MOLECULE_PROVISION_CONCURRENCY", "not-a-number")
+	defer os.Unsetenv("MOLECULE_PROVISION_CONCURRENCY")
+	val := resolveProvisionConcurrency()
+	assert.Equal(t, defaultProvisionConcurrency, val)
+}
+
+func TestResolveProvisionConcurrency_WhitespaceOnly(t *testing.T) {
+	os.Setenv("MOLECULE_PROVISION_CONCURRENCY", "   ")
+	defer os.Unsetenv("MOLECULE_PROVISION_CONCURRENCY")
+	val := resolveProvisionConcurrency()
+	assert.Equal(t, defaultProvisionConcurrency, val)
+}
+
+func TestResolveProvisionConcurrency_LargeValue(t *testing.T) {
+	os.Setenv("MOLECULE_PROVISION_CONCURRENCY", "10000")
+	defer os.Unsetenv("MOLECULE_PROVISION_CONCURRENCY")
+	val := resolveProvisionConcurrency()
+	assert.Equal(t, 10000, val)
+}
+
+// errString tests — nil-safe error-to-string wrapper.
+
+func TestErrString_NilError(t *testing.T) {
+	result := errString(nil)
+	assert.Equal(t, "", result)
+}
+
+func TestErrString_WithError(t *testing.T) {
+	err := errors.New("something went wrong")
+	result := errString(err)
+	assert.Equal(t, "something went wrong", result)
+}
+
+func TestErrString_EmptyError(t *testing.T) {
+	err := errors.New("")
+	result := errString(err)
+	assert.Equal(t, "", result)
+}

workspace-server/internal/handlers/plugins_helpers_pure_test.go

@@ -0,0 +1,80 @@
+package handlers
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// supportsRuntime tests — plugin runtime compatibility checking.
+
+func TestSupportsRuntime_EmptyRuntimes(t *testing.T) {
+	// Empty runtimes = unspecified, try it → always compatible.
+	info := pluginInfo{Name: "test", Runtimes: nil}
+	assert.True(t, info.supportsRuntime("claude_code"))
+	assert.True(t, info.supportsRuntime("any_runtime"))
+}
+
+func TestSupportsRuntime_ExactMatch(t *testing.T) {
+	info := pluginInfo{Name: "test", Runtimes: []string{"claude_code", "anthropic"}}
+	assert.True(t, info.supportsRuntime("claude_code"))
+	assert.True(t, info.supportsRuntime("anthropic"))
+}
+
+func TestSupportsRuntime_NoMatch(t *testing.T) {
+	info := pluginInfo{Name: "test", Runtimes: []string{"claude_code"}}
+	assert.False(t, info.supportsRuntime("openai"))
+}
+
+func TestSupportsRuntime_HyphenUnderscoreNormalized(t *testing.T) {
+	// "claude-code" and "claude_code" are considered equal.
+	info := pluginInfo{Name: "test", Runtimes: []string{"claude-code"}}
+	assert.True(t, info.supportsRuntime("claude_code"))
+	assert.True(t, info.supportsRuntime("claude-code")) // symmetric hyphen form
+}
+
+func TestSupportsRuntime_HyphenVsUnderscoreReverse(t *testing.T) {
+	// Plugin declares underscore form; runtime uses hyphen.
+	info := pluginInfo{Name: "test", Runtimes: []string{"claude_code"}}
+	assert.True(t, info.supportsRuntime("claude-code"))
+}
+
+func TestSupportsRuntime_EmptyStringRuntime(t *testing.T) {
+	info := pluginInfo{Name: "test", Runtimes: []string{"claude_code"}}
+	// Empty runtime string: should not match any plugin.
+	assert.False(t, info.supportsRuntime(""))
+}
+
+func TestSupportsRuntime_SingleRuntimeMatch(t *testing.T) {
+	// Multiple declared runtimes: only matching one is sufficient.
+	info := pluginInfo{Name: "test", Runtimes: []string{"python", "nodejs", "claude_code"}}
+	assert.True(t, info.supportsRuntime("claude_code"))
+	assert.False(t, info.supportsRuntime("ruby"))
+}
+
+func TestSupportsRuntime_AllHyphenForms(t *testing.T) {
+	// Both plugin and runtime use hyphen form.
+	info := pluginInfo{Name: "test", Runtimes: []string{"claude-code"}}
+	assert.True(t, info.supportsRuntime("claude-code"))
+}
+
+func TestSupportsRuntime_MultipleHyphenNormalization(t *testing.T) {
+	// Mixed hyphen/underscore forms normalize to the same.
+	info := pluginInfo{Name: "test", Runtimes: []string{"some-runtime-name"}}
+	assert.True(t, info.supportsRuntime("some_runtime_name"))
+	assert.True(t, info.supportsRuntime("some-runtime-name"))
+}
+
+func TestSupportsRuntime_EmptyPluginRuntimesWithAnyInput(t *testing.T) {
+	// Empty Runtimes on plugin = try it regardless of runtime.
+	info := pluginInfo{Name: "test", Runtimes: []string{}}
+	assert.True(t, info.supportsRuntime(""))
+	assert.True(t, info.supportsRuntime("any"))
+	assert.True(t, info.supportsRuntime("unknown"))
+}
+
+func TestSupportsRuntime_ZeroLengthRuntimes(t *testing.T) {
+	// Empty slice vs nil: both should be treated as "unspecified".
+	info := pluginInfo{Name: "test"}
+	assert.True(t, info.supportsRuntime("anything"))
+}

workspace-server/internal/handlers/terminal_diagnose_test.go

@@ -24,6 +24,9 @@ import (
 //   - response is HTTP 200 (the endpoint always returns 200; failure is
 //     in the JSON body so callers don't need branch-on-status)
 func TestHandleDiagnose_RoutesToRemote(t *testing.T) {
+	if _, err := exec.LookPath("ssh-keygen"); err != nil {
+		t.Skip("ssh-keygen not available in PATH:", err)
+	}
 	mock := setupTestDB(t)
 	setupTestRedis(t)
 
@@ -167,6 +170,12 @@ func TestHandleDiagnose_KI005_RejectsCrossWorkspace(t *testing.T) {
 // to differentiate "IAM broke" (send-key fails) from "sshd broke" (probe
 // fails) from "SG/network broke" (wait-for-port fails).
 func TestDiagnoseRemote_StopsAtSSHProbe(t *testing.T) {
+	if _, err := exec.LookPath("ssh-keygen"); err != nil {
+		t.Skip("ssh-keygen not available in PATH:", err)
+	}
+	if _, err := exec.LookPath("nc"); err != nil {
+		t.Skip("nc not available in PATH:", err)
+	}
 	mock := setupTestDB(t)
 	setupTestRedis(t)
 

workspace-server/internal/handlers/workspace_crud_helpers_test.go

@@ -0,0 +1,165 @@
+package handlers
+
+// workspace_crud_helpers_test.go — tests for pure-logic helpers in workspace_crud.go.
+//
+// Covered helpers:
+//   validateWorkspaceDir — bind-mount path safety (CWE-22 defence-in-depth)
+
+import "testing"
+
+// ─────────────────────────────────────────────────────────────────────────────
+// validateWorkspaceDir
+// ─────────────────────────────────────────────────────────────────────────────
+
+func TestValidateWorkspaceDir_AcceptsValidAbsolutePath(t *testing.T) {
+	cases := []string{
+		"/home/ubuntu/workspace",
+		"/opt/myapp/data",
+		"/tmp/molecule-workspace",
+		"/Users/admin/workspace",
+		"/workspace",
+		"/mnt/volumes/data",
+		"/srv/molecule",
+		"/nix/store",
+	}
+	for _, dir := range cases {
+		err := validateWorkspaceDir(dir)
+		if err != nil {
+			t.Errorf("validateWorkspaceDir(%q) returned error: %v; want nil", dir, err)
+		}
+	}
+}
+
+func TestValidateWorkspaceDir_RejectsRelativePath(t *testing.T) {
+	cases := []string{
+		"relative/path",
+		"./local",
+		"../sibling",
+		"workspace",
+		"",
+	}
+	for _, dir := range cases {
+		err := validateWorkspaceDir(dir)
+		if err == nil {
+			t.Errorf("validateWorkspaceDir(%q) = nil; want error (relative path)", dir)
+		}
+	}
+}
+
+func TestValidateWorkspaceDir_RejectsTraversalSequence(t *testing.T) {
+	cases := []string{
+		"/etc/../../../etc/passwd",
+		"/home/user/../../root",
+		"/workspace/../../../sibling",
+		"/foo/bar/..%2f..%2fetc",
+		"/valid/../etc/passwd",
+	}
+	for _, dir := range cases {
+		err := validateWorkspaceDir(dir)
+		if err == nil {
+			t.Errorf("validateWorkspaceDir(%q) = nil; want error (traversal)", dir)
+		}
+	}
+}
+
+func TestValidateWorkspaceDir_RejectsSystemPaths(t *testing.T) {
+	// System paths must be rejected outright — a workspace binding /etc or
+	// /proc would let the agent read host secrets or inspect kernel state.
+	systemPaths := []string{
+		"/etc",
+		"/var",
+		"/proc",
+		"/sys",
+		"/dev",
+		"/boot",
+		"/sbin",
+		"/bin",
+		"/usr",
+	}
+	for _, dir := range systemPaths {
+		err := validateWorkspaceDir(dir)
+		if err == nil {
+			t.Errorf("validateWorkspaceDir(%q) = nil; want error (system path)", dir)
+		}
+	}
+}
+
+func TestValidateWorkspaceDir_RejectsDescendantsOfSystemPaths(t *testing.T) {
+	// A descendant of a system path must also be rejected — /etc/shadow,
+	// /proc/1/cmdline, /dev/null all fall in this category.
+	descendants := []string{
+		"/etc/passwd",
+		"/etc/shadow",
+		"/etc/ssh/sshd_config",
+		"/var/log/syslog",
+		"/proc/self/environ",
+		"/sys/kernel/version",
+		"/dev/null",
+		"/boot/grub/grub.cfg",
+		"/sbin/init",
+		"/bin/bash",
+		"/usr/bin/python3",
+	}
+	for _, dir := range descendants {
+		err := validateWorkspaceDir(dir)
+		if err == nil {
+			t.Errorf("validateWorkspaceDir(%q) = nil; want error (descendant of system path)", dir)
+		}
+	}
+}
+
+func TestValidateWorkspaceDir_AcceptsPathsSimilarToSystemPaths(t *testing.T) {
+	// Paths that LOOK like system paths but are NOT exact matches or
+	// descendants should be accepted. These are valid workspace directories.
+	valid := []string{
+		"/etcworkspace",
+		"/varworkspace",
+		"/procworkspace",
+		"/sysworkspace",
+		"/devworkspace",
+		"/bootworkspace",
+		"/sbinworkspace",
+		"/binworkspace",
+		"/usrworkspace",
+		"/etx",    // typo of /etc but a different path
+		"/vartmp",  // /var/tmp is different from /var
+		"/usrr",    // typo of /usr but a different path
+		"/workspace/etc",
+		"/workspace/var",
+		"/home/user/etc",
+		"/opt/etc",
+	}
+	for _, dir := range valid {
+		err := validateWorkspaceDir(dir)
+		if err != nil {
+			t.Errorf("validateWorkspaceDir(%q) returned error: %v; want nil", dir, err)
+		}
+	}
+}
+
+func TestValidateWorkspaceDir_ErrorMessages(t *testing.T) {
+	// Error messages must be descriptive enough for operators to self-diagnose.
+	relErr := validateWorkspaceDir("relative")
+	if relErr == nil {
+		t.Fatal("relative path: want error, got nil")
+	}
+	if relErr.Error() == "" {
+		t.Error("relative path error message is empty")
+	}
+
+	travErr := validateWorkspaceDir("/etc/../../../etc/passwd")
+	if travErr == nil {
+		t.Fatal("traversal: want error, got nil")
+	}
+	if travErr.Error() == "" {
+		t.Error("traversal error message is empty")
+	}
+
+	sysErr := validateWorkspaceDir("/etc")
+	if sysErr == nil {
+		t.Fatal("system path: want error, got nil")
+	}
+	if sysErr.Error() == "" {
+		t.Error("system path error message is empty")
+	}
+}

workspace-server/internal/handlers/workspace_crud_validators_test.go

@@ -0,0 +1,167 @@
+package handlers
+
+import (
+	"testing"
+)
+
+// ── validateWorkspaceDir ───────────────────────────────────────────────────────
+
+func TestValidateWorkspaceDir_RelativeRejected(t *testing.T) {
+	cases := []string{
+		"relative/path",
+		"./myworkspace",
+		"~/workspaces/dev",
+	}
+	for _, dir := range cases {
+		t.Run(dir, func(t *testing.T) {
+			if err := validateWorkspaceDir(dir); err == nil {
+				t.Errorf("validateWorkspaceDir(%q): expected error (relative path), got nil", dir)
+			}
+		})
+	}
+}
+
+func TestValidateWorkspaceDir_TraversalRejected(t *testing.T) {
+	cases := []string{
+		"/opt/molecule/../../../etc",
+		"/workspaces/dev/../../root",
+		"/opt/../opt/../etc",
+	}
+	for _, dir := range cases {
+		t.Run(dir, func(t *testing.T) {
+			if err := validateWorkspaceDir(dir); err == nil {
+				t.Errorf("validateWorkspaceDir(%q): expected error (traversal), got nil", dir)
+			}
+		})
+	}
+}
+
+func TestValidateWorkspaceDir_SystemPathsRejected(t *testing.T) {
+	cases := []string{
+		"/etc",
+		"/etc/molecule",
+		"/var",
+		"/var/log",
+		"/proc",
+		"/proc/self",
+		"/sys",
+		"/sys/kernel",
+		"/dev",
+		"/dev/null",
+		"/boot",
+		"/sbin",
+		"/bin",
+		"/lib",
+		"/usr",
+		"/usr/local",
+	}
+	for _, dir := range cases {
+		t.Run(dir, func(t *testing.T) {
+			if err := validateWorkspaceDir(dir); err == nil {
+				t.Errorf("validateWorkspaceDir(%q): expected error (system path), got nil", dir)
+			}
+		})
+	}
+}
+
+func TestValidateWorkspaceDir_PrefixMatchesBlocked(t *testing.T) {
+	// The blocklist checks prefix so /etc/foo must also be rejected.
+	cases := []string{
+		"/etc/molecule-config",
+		"/var/log/workspace",
+		"/usr/local/bin",
+		"/usr/bin/molecule",
+	}
+	for _, dir := range cases {
+		t.Run(dir, func(t *testing.T) {
+			if err := validateWorkspaceDir(dir); err == nil {
+				t.Errorf("validateWorkspaceDir(%q): expected error (prefix of blocked path), got nil", dir)
+			}
+		})
+	}
+}
+
+// ── validateWorkspaceFields ────────────────────────────────────────────────────
+
+func TestValidateWorkspaceFields_AllEmpty(t *testing.T) {
+	// All empty → valid (creation uses defaults; empty is allowed)
+	if err := validateWorkspaceFields("", "", "", ""); err != nil {
+		t.Errorf("validateWorkspaceFields with all empty: expected nil, got %v", err)
+	}
+}
+
+func TestValidateWorkspaceFields_ModelTooLong(t *testing.T) {
+	longModel := make([]byte, 101)
+	for i := range longModel {
+		longModel[i] = 'x'
+	}
+	if err := validateWorkspaceFields("", "", string(longModel), ""); err == nil {
+		t.Error("model > 100 chars: expected error, got nil")
+	}
+}
+
+func TestValidateWorkspaceFields_RuntimeTooLong(t *testing.T) {
+	longRuntime := make([]byte, 101)
+	for i := range longRuntime {
+		longRuntime[i] = 'x'
+	}
+	if err := validateWorkspaceFields("", "", "", string(longRuntime)); err == nil {
+		t.Error("runtime > 100 chars: expected error, got nil")
+	}
+}
+
+func TestValidateWorkspaceFields_CRLFInRole(t *testing.T) {
+	if err := validateWorkspaceFields("", "Backend\r\nEngineer", "", ""); err == nil {
+		t.Error("role with \\r\\n: expected error, got nil")
+	}
+}
+
+func TestValidateWorkspaceFields_NewlineInModel(t *testing.T) {
+	if err := validateWorkspaceFields("", "", "gpt-\n4o", ""); err == nil {
+		t.Error("model with \\n: expected error, got nil")
+	}
+}
+
+func TestValidateWorkspaceFields_NewlineInRuntime(t *testing.T) {
+	if err := validateWorkspaceFields("", "", "", "lang\rgraph"); err == nil {
+		t.Error("runtime with \\r: expected error, got nil")
+	}
+}
+
+func TestValidateWorkspaceFields_YAMLSpecialChars(t *testing.T) {
+	// yamlSpecialChars = "{}[]|>*&!"
+	// These must be rejected in name and role.
+	dangerous := []string{
+		"Workspace{evil}",
+		"Workspace[evil]",
+		"Workspace]evil[",
+		"Workspace|evil",
+		"Workspace>evil",
+		"Workspace*evil",
+		"Workspace&evil",
+		"Workspace!evil",
+		"Name{}",
+		"Role[]",
+	}
+	for _, v := range dangerous {
+		t.Run(v, func(t *testing.T) {
+			if err := validateWorkspaceFields(v, "", "", ""); err == nil {
+				t.Errorf("name %q: expected error (YAML special char), got nil", v)
+			}
+		})
+	}
+}
+
+func TestValidateWorkspaceFields_YAMLCharsAllowedInModelRuntime(t *testing.T) {
+	// YAML special chars are only blocked in name/role, not model/runtime.
+	if err := validateWorkspaceFields("", "", "model{}[]", "runtime*&!"); err != nil {
+		t.Errorf("model/runtime with YAML chars: expected nil, got %v", err)
+	}
+}
+
+func TestValidateWorkspaceFields_YAMLCharsAllowedInEmptyName(t *testing.T) {
+	// Empty name is fine; YAML char restriction is only on non-empty values.
+	if err := validateWorkspaceFields("", "Backend Engineer", "", ""); err != nil {
+		t.Errorf("empty name with valid role: expected nil, got %v", err)
+	}
+}

workspace-server/internal/handlers/workspace_dispatchers_test.go

@@ -0,0 +1,165 @@
+package handlers
+
+import (
+	"context"
+	"database/sql"
+	"testing"
+
+	"github.com/DATA-DOG/go-sqlmock"
+	"github.com/Molecule-AI/molecule-monorepo/platform/internal/models"
+)
+
+// ==================== resolveDeliveryMode ====================
+// Covers workspace_dispatchers.go / registry.go:resolveDeliveryMode
+
+func TestResolveDeliveryMode_PayloadModeWins(t *testing.T) {
+	mock := setupTestDB(t)
+	setupTestRedis(t)
+	broadcaster := newTestBroadcaster()
+	h := NewRegistryHandler(broadcaster)
+
+	ctx := context.Background()
+	for _, mode := range []string{models.DeliveryModePush, models.DeliveryModePoll} {
+		got, err := h.resolveDeliveryMode(ctx, "ws-any-id", mode)
+		if err != nil {
+			t.Errorf("resolveDeliveryMode(payloadMode=%q) unexpected error: %v", mode, err)
+		}
+		if got != mode {
+			t.Errorf("resolveDeliveryMode(payloadMode=%q) = %q, want %q", mode, got, mode)
+		}
+	}
+
+	// DB must NOT have been queried when payloadMode is set.
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("DB expectations not met: %v", err)
+	}
+}
+
+func TestResolveDeliveryMode_ExistingDeliveryMode(t *testing.T) {
+	mock := setupTestDB(t)
+	setupTestRedis(t)
+	broadcaster := newTestBroadcaster()
+	h := NewRegistryHandler(broadcaster)
+
+	// Workspace row has existing delivery_mode = "poll"
+	mock.ExpectQuery("SELECT delivery_mode, runtime FROM workspaces").
+		WithArgs("ws-poll").
+		WillReturnRows(sqlmock.NewRows([]string{"delivery_mode", "runtime"}).
+			AddRow("poll", "langgraph"))
+
+	ctx := context.Background()
+	got, err := h.resolveDeliveryMode(ctx, "ws-poll", "")
+	if err != nil {
+		t.Errorf("resolveDeliveryMode() unexpected error: %v", err)
+	}
+	if got != models.DeliveryModePoll {
+		t.Errorf("resolveDeliveryMode() = %q, want %q", got, models.DeliveryModePoll)
+	}
+}
+
+func TestResolveDeliveryMode_ExternalRuntime_DefaultsToPoll(t *testing.T) {
+	mock := setupTestDB(t)
+	setupTestRedis(t)
+	broadcaster := newTestBroadcaster()
+	h := NewRegistryHandler(broadcaster)
+
+	// Row exists but delivery_mode is NULL; runtime = "external"
+	mock.ExpectQuery("SELECT delivery_mode, runtime FROM workspaces").
+		WithArgs("ws-external").
+		WillReturnRows(sqlmock.NewRows([]string{"delivery_mode", "runtime"}).
+			AddRow(nil, "external"))
+
+	ctx := context.Background()
+	got, err := h.resolveDeliveryMode(ctx, "ws-external", "")
+	if err != nil {
+		t.Errorf("resolveDeliveryMode() unexpected error: %v", err)
+	}
+	if got != models.DeliveryModePoll {
+		t.Errorf("resolveDeliveryMode() = %q, want %q (external runtime)", got, models.DeliveryModePoll)
+	}
+}
+
+func TestResolveDeliveryMode_SelfHosted_DefaultsToPush(t *testing.T) {
+	mock := setupTestDB(t)
+	setupTestRedis(t)
+	broadcaster := newTestBroadcaster()
+	h := NewRegistryHandler(broadcaster)
+
+	// Row exists; delivery_mode is NULL; runtime = "langgraph"
+	mock.ExpectQuery("SELECT delivery_mode, runtime FROM workspaces").
+		WithArgs("ws-self-hosted").
+		WillReturnRows(sqlmock.NewRows([]string{"delivery_mode", "runtime"}).
+			AddRow(nil, "langgraph"))
+
+	ctx := context.Background()
+	got, err := h.resolveDeliveryMode(ctx, "ws-self-hosted", "")
+	if err != nil {
+		t.Errorf("resolveDeliveryMode() unexpected error: %v", err)
+	}
+	if got != models.DeliveryModePush {
+		t.Errorf("resolveDeliveryMode() = %q, want %q (self-hosted default)", got, models.DeliveryModePush)
+	}
+}
+
+func TestResolveDeliveryMode_NotFound_DefaultsToPush(t *testing.T) {
+	mock := setupTestDB(t)
+	setupTestRedis(t)
+	broadcaster := newTestBroadcaster()
+	h := NewRegistryHandler(broadcaster)
+
+	// Row not found → sql.ErrNoRows → default push
+	mock.ExpectQuery("SELECT delivery_mode, runtime FROM workspaces").
+		WithArgs("ws-nonexistent").
+		WillReturnError(sql.ErrNoRows)
+
+	ctx := context.Background()
+	got, err := h.resolveDeliveryMode(ctx, "ws-nonexistent", "")
+	if err != nil {
+		t.Errorf("resolveDeliveryMode() unexpected error on no-rows: %v", err)
+	}
+	if got != models.DeliveryModePush {
+		t.Errorf("resolveDeliveryMode() = %q, want %q (not-found default)", got, models.DeliveryModePush)
+	}
+}
+
+func TestResolveDeliveryMode_DBError_Propagated(t *testing.T) {
+	mock := setupTestDB(t)
+	setupTestRedis(t)
+	broadcaster := newTestBroadcaster()
+	h := NewRegistryHandler(broadcaster)
+
+	mock.ExpectQuery("SELECT delivery_mode, runtime FROM workspaces").
+		WithArgs("ws-error").
+		WillReturnError(context.DeadlineExceeded)
+
+	ctx := context.Background()
+	_, err := h.resolveDeliveryMode(ctx, "ws-error", "")
+	if err == nil {
+		t.Errorf("resolveDeliveryMode() expected error, got nil")
+	}
+}
+
+func TestResolveDeliveryMode_ExistingDeliveryModeEmptyString(t *testing.T) {
+	// When the DB returns an empty (non-NULL) string for delivery_mode,
+	// it falls through to the runtime check (not the existing.Valid path).
+	mock := setupTestDB(t)
+	setupTestRedis(t)
+	broadcaster := newTestBroadcaster()
+	h := NewRegistryHandler(broadcaster)
+
+	// delivery_mode is explicitly empty string (not NULL), runtime = "langgraph"
+	// → falls through to runtime check → "push" for non-external
+	mock.ExpectQuery("SELECT delivery_mode, runtime FROM workspaces").
+		WithArgs("ws-empty-mode").
+		WillReturnRows(sqlmock.NewRows([]string{"delivery_mode", "runtime"}).
+			AddRow("", "langgraph"))
+
+	ctx := context.Background()
+	got, err := h.resolveDeliveryMode(ctx, "ws-empty-mode", "")
+	if err != nil {
+		t.Errorf("resolveDeliveryMode() unexpected error: %v", err)
+	}
+	if got != models.DeliveryModePush {
+		t.Errorf("resolveDeliveryMode() = %q, want %q", got, models.DeliveryModePush)
+	}
+}

workspace-server/internal/models/workspace_delivery_mode_test.go

@@ -0,0 +1,100 @@
+package models
+
+import "testing"
+
+// ==================== IsValidDeliveryMode ====================
+
+func TestIsValidDeliveryMode_Valid(t *testing.T) {
+	for _, mode := range []string{DeliveryModePush, DeliveryModePoll} {
+		if !IsValidDeliveryMode(mode) {
+			t.Errorf("IsValidDeliveryMode(%q) = false, want true", mode)
+		}
+	}
+}
+
+func TestIsValidDeliveryMode_Invalid(t *testing.T) {
+	cases := []struct {
+		val  string
+		want bool
+	}{
+		{"", false},         // empty string is not valid — callers must resolve the default
+		{"pushx", false},   // typo
+		{"pollx", false},    // typo
+		{"PUSH", false},     // case-sensitive
+		{"PUSH ", false},    // trailing space
+		{"push ", false},    // trailing space
+		{"hybrid", false},   // non-existent mode
+		{"poll ", false},    // trailing space
+	}
+	for _, tc := range cases {
+		got := IsValidDeliveryMode(tc.val)
+		if got != tc.want {
+			t.Errorf("IsValidDeliveryMode(%q) = %v, want %v", tc.val, got, tc.want)
+		}
+	}
+}
+
+// ==================== WorkspaceStatus ====================
+
+func TestWorkspaceStatus_String(t *testing.T) {
+	statuses := []WorkspaceStatus{
+		StatusProvisioning,
+		StatusOnline,
+		StatusOffline,
+		StatusDegraded,
+		StatusFailed,
+		StatusRemoved,
+		StatusPaused,
+		StatusHibernated,
+		StatusHibernating,
+		StatusAwaitingAgent,
+	}
+	for _, s := range statuses {
+		if got := s.String(); got != string(s) {
+			t.Errorf("WorkspaceStatus(%q).String() = %q, want %q", s, got, string(s))
+		}
+	}
+}
+
+func TestAllWorkspaceStatuses_Length(t *testing.T) {
+	// The const block has 10 statuses; AllWorkspaceStatuses must match.
+	if got := len(AllWorkspaceStatuses); got != 10 {
+		t.Errorf("len(AllWorkspaceStatuses) = %d, want 10", got)
+	}
+}
+
+func TestAllWorkspaceStatuses_ContainsAllNamed(t *testing.T) {
+	// Verify every named const appears in AllWorkspaceStatuses exactly once.
+	named := []WorkspaceStatus{
+		StatusProvisioning,
+		StatusOnline,
+		StatusOffline,
+		StatusDegraded,
+		StatusFailed,
+		StatusRemoved,
+		StatusPaused,
+		StatusHibernated,
+		StatusHibernating,
+		StatusAwaitingAgent,
+	}
+	set := make(map[WorkspaceStatus]bool, len(AllWorkspaceStatuses))
+	for _, s := range AllWorkspaceStatuses {
+		set[s] = true
+	}
+	for _, s := range named {
+		if !set[s] {
+			t.Errorf("named status %q missing from AllWorkspaceStatuses", s)
+		}
+	}
+	if len(set) != len(named) {
+		t.Errorf("AllWorkspaceStatuses has %d unique entries, want %d", len(set), len(named))
+	}
+}
+
+func TestAllWorkspaceStatuses_NoEmpty(t *testing.T) {
+	for _, s := range AllWorkspaceStatuses {
+		if s == "" {
+			t.Errorf("AllWorkspaceStatuses contains empty string")
+		}
+	}
+}

workspace-server/internal/ws/hub_test.go

@@ -0,0 +1,386 @@
+package ws
+
+import (
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/Molecule-AI/molecule-monorepo/platform/internal/models"
+)
+
+// ─── helpers ────────────────────────────────────────────────────────────────
+
+// mockClient returns a Client with a buffered send channel of the given size
+// and a nil WebSocket connection. Nil Conn is safe for our tests because we
+// never call WritePump (which uses Conn) — we only test the hub's send channel
+// and broadcast logic.
+func mockClient(workspaceID string, bufSize int) *Client {
+	return &Client{
+		WorkspaceID: workspaceID,
+		Send:        make(chan []byte, bufSize),
+		// Conn is nil — safe: WritePump (which uses Conn) is never called in tests.
+	}
+}
+
+// ─── NewHub ────────────────────────────────────────────────────────────────
+
+func TestNewHub_NilChecker(t *testing.T) {
+	// nil AccessChecker is accepted (hub allows all workspace→workspace broadcasts
+	// when canCommunicate is unset — the gating is purely advisory).
+	h := NewHub(nil)
+	if h == nil {
+		t.Fatal("NewHub(nil) returned nil")
+	}
+	if h.canCommunicate != nil {
+		t.Error("canCommunicate should be nil")
+	}
+}
+
+func TestNewHub_AccessCheckerWired(t *testing.T) {
+	called := false
+	checker := func(callerID, targetID string) bool {
+		called = true
+		return callerID == targetID // only self-communication allowed
+	}
+	h := NewHub(checker)
+	if h.canCommunicate == nil {
+		t.Fatal("canCommunicate not wired")
+	}
+	// Invoke the wired function directly
+	allowed := h.canCommunicate("ws-1", "ws-1")
+	if !called {
+		t.Error("checker was not called")
+	}
+	if !allowed {
+		t.Error("self-communication should be allowed")
+	}
+	if h.canCommunicate("ws-1", "ws-2") {
+		t.Error("cross-workspace communication should be blocked by checker")
+	}
+}
+
+// ─── safeSend ─────────────────────────────────────────────────────────────
+
+func TestSafeSend_OpenChannel_Sends(t *testing.T) {
+	c := mockClient("ws-1", 10)
+	data := []byte(`{"type":"ping"}`)
+	ok := safeSend(c, data)
+	if !ok {
+		t.Error("safeSend should return true for open channel")
+	}
+	select {
+	case got := <-c.Send:
+		if string(got) != string(data) {
+			t.Errorf("got %q, want %q", got, data)
+		}
+	case <-time.After(100 * time.Millisecond):
+		t.Error("no message received on channel")
+	}
+}
+
+func TestSafeSend_ClosedChannel_ReturnsFalse(t *testing.T) {
+	c := mockClient("ws-1", 10)
+	close(c.Send) // close before safeSend
+	ok := safeSend(c, []byte("data"))
+	if ok {
+		t.Error("safeSend should return false for closed channel")
+	}
+}
+
+func TestSafeSend_FullChannel_ReturnsFalse(t *testing.T) {
+	c := mockClient("ws-1", 1) // buffer size 1
+	// Fill the channel
+	c.Send <- []byte("first")
+	// Channel is now full
+	ok := safeSend(c, []byte("second"))
+	if ok {
+		t.Error("safeSend should return false when channel buffer is full")
+	}
+	// Drain to leave clean state
+	<-c.Send
+}
+
+// ─── Broadcast ────────────────────────────────────────────────────────────
+
+func TestBroadcast_CanvasAlwaysReceives(t *testing.T) {
+	h := NewHub(nil) // nil checker: canvas always gets messages
+
+	// Canvas client (no workspaceID) + two workspace clients
+	canvas := mockClient("", 10)
+	ws1 := mockClient("ws-1", 10)
+	ws2 := mockClient("ws-2", 10)
+
+	// Manually register clients into hub state
+	h.mu.Lock()
+	h.clients[canvas] = true
+	h.clients[ws1] = true
+	h.clients[ws2] = true
+	h.mu.Unlock()
+
+	msg := models.WSMessage{Event: "test", Payload: []byte(`"hello"`)}
+	h.Broadcast(msg)
+
+	// Canvas must receive
+	select {
+	case got := <-canvas.Send:
+		t.Logf("canvas received: %s", got)
+	case <-time.After(100 * time.Millisecond):
+		t.Error("canvas client did not receive broadcast")
+	}
+}
+
+func TestBroadcast_WorkspaceCanCommunicateGating(t *testing.T) {
+	// Only ws-1 can receive messages for ws-2
+	checker := func(callerID, targetID string) bool {
+		return callerID == targetID
+	}
+	h := NewHub(checker)
+
+	ws1 := mockClient("ws-1", 10)
+	ws2 := mockClient("ws-2", 10)
+	canvas := mockClient("", 10)
+
+	h.mu.Lock()
+	h.clients[ws1] = true
+	h.clients[ws2] = true
+	h.clients[canvas] = true
+	h.mu.Unlock()
+
+	// Broadcast addressed to ws-2
+	msg := models.WSMessage{Event: "test", WorkspaceID: "ws-2"}
+	h.Broadcast(msg)
+
+	// ws-1 should NOT receive (not the target, checker says no)
+	select {
+	case <-ws1.Send:
+		t.Error("ws-1 should not receive broadcast for ws-2")
+	case <-time.After(50 * time.Millisecond):
+		t.Log("ws-1 correctly blocked — no message")
+	}
+
+	// ws-2 should receive
+	select {
+	case <-ws2.Send:
+		t.Log("ws-2 correctly received broadcast")
+	case <-time.After(100 * time.Millisecond):
+		t.Error("ws-2 did not receive broadcast")
+	}
+
+	// Canvas always receives
+	select {
+	case <-canvas.Send:
+		t.Log("canvas correctly received broadcast")
+	case <-time.After(100 * time.Millisecond):
+		t.Error("canvas did not receive broadcast")
+	}
+}
+
+func TestBroadcast_DropsOnClosedChannel(t *testing.T) {
+	h := NewHub(nil)
+	c := mockClient("", 10)
+	close(c.Send) // pre-close so safeSend returns false
+
+	h.mu.Lock()
+	h.clients[c] = true
+	h.mu.Unlock()
+
+	// Broadcast must not panic; closed client should be dropped silently.
+	msg := models.WSMessage{Event: "ping"}
+	h.Broadcast(msg) // should not panic
+}
+
+func TestBroadcast_DropsOnFullChannel(t *testing.T) {
+	h := NewHub(nil)
+	c := mockClient("", 1)
+	c.Send <- []byte("blocker") // fill buffer
+
+	h.mu.Lock()
+	h.clients[c] = true
+	h.mu.Unlock()
+
+	msg := models.WSMessage{Event: "ping"}
+	h.Broadcast(msg) // safeSend returns false; no panic
+
+	// Drain to leave clean state
+	<-c.Send
+}
+
+func TestBroadcast_EmptyHubNoPanic(t *testing.T) {
+	h := NewHub(nil)
+	msg := models.WSMessage{Event: "ping"}
+	h.Broadcast(msg) // must not panic with no clients
+}
+
+func TestBroadcast_MultiClient(t *testing.T) {
+	h := NewHub(nil)
+	clients := make([]*Client, 5)
+	h.mu.Lock()
+	for i := 0; i < 5; i++ {
+		clients[i] = mockClient("", 10)
+		h.clients[clients[i]] = true
+	}
+	h.mu.Unlock()
+
+	msg := models.WSMessage{Event: "multi", Payload: []byte(`"all receive"`)}
+	h.Broadcast(msg)
+
+	for i, c := range clients {
+		select {
+		case <-c.Send:
+			t.Logf("client %d received", i)
+		case <-time.After(100 * time.Millisecond):
+			t.Errorf("client %d did not receive broadcast", i)
+		}
+	}
+}
+
+func TestBroadcast_CanvasIgnoresChecker(t *testing.T) {
+	// Strict checker that blocks ALL cross-workspace (never returns true for different IDs)
+	strictChecker := func(callerID, targetID string) bool {
+		return callerID == targetID
+	}
+	h := NewHub(strictChecker)
+
+	canvas := mockClient("", 10)
+
+	h.mu.Lock()
+	h.clients[canvas] = true
+	h.mu.Unlock()
+
+	msg := models.WSMessage{Event: "ping", WorkspaceID: "ws-1"}
+	h.Broadcast(msg)
+
+	select {
+	case <-canvas.Send:
+		t.Log("canvas received message even though checker blocks ws-1")
+	case <-time.After(100 * time.Millisecond):
+		t.Error("canvas must always receive — checker should be bypassed")
+	}
+}
+
+// ─── Close ────────────────────────────────────────────────────────────────
+
+func TestClose_DisconnectsAllClients(t *testing.T) {
+	h := NewHub(nil)
+	clients := make([]*Client, 3)
+	h.mu.Lock()
+	for i := 0; i < 3; i++ {
+		clients[i] = mockClient("", 10)
+		h.clients[clients[i]] = true
+	}
+	h.mu.Unlock()
+
+	// Start Run goroutine so Close can drain Unregister channel
+	go h.Run()
+	defer h.Close()
+
+	// Unregister all clients so the mutex is released before Close() tries to lock it
+	for _, c := range clients {
+		h.Unregister <- c
+	}
+	time.Sleep(50 * time.Millisecond)
+
+	// Now close — mutex is free, Close() should succeed
+	h.Close()
+
+	// All client channels should be closed
+	for i, c := range clients {
+		select {
+		case _, ok := <-c.Send:
+			if ok {
+				t.Errorf("client %d channel still open after Close", i)
+			}
+		case <-time.After(100 * time.Millisecond):
+			// Channel drained and closed
+		}
+	}
+}
+
+func TestClose_Idempotent(t *testing.T) {
+	h := NewHub(nil)
+	c := mockClient("", 10)
+	h.mu.Lock()
+	h.clients[c] = true
+	h.mu.Unlock()
+
+	// Close twice — must not panic or deadlock
+	h.Close()
+	h.Close() // second call also fine
+}
+
+func TestClose_ClosesDoneChannel(t *testing.T) {
+	h := NewHub(nil)
+
+	// Start Run goroutine
+	done := make(chan struct{})
+	go func() {
+		h.Run()
+		close(done)
+	}()
+
+	h.Close()
+
+	select {
+	case <-done:
+		t.Log("Run exited after Close")
+	case <-time.After(200 * time.Millisecond):
+		t.Error("Run did not exit after Close")
+	}
+}
+
+// ─── Run goroutine (Unregister) ──────────────────────────────────────────
+
+func TestRun_UnregisterClosesClientSend(t *testing.T) {
+	h := NewHub(nil)
+	c := mockClient("ws-1", 10)
+
+	// Start Run() BEFORE sending to Register — Register is unbuffered,
+	// so Run() must be ready to receive before the send can complete.
+	go h.Run()
+	defer h.Close()
+
+	// Register the client
+	h.Register <- c
+
+	// Give Run a moment to register the client
+	time.Sleep(20 * time.Millisecond)
+
+	// Unregister client
+	h.Unregister <- c
+
+	select {
+	case _, ok := <-c.Send:
+		if ok {
+			t.Error("client send channel should be closed after Unregister")
+		}
+	case <-time.After(500 * time.Millisecond):
+		t.Error("client send channel not closed within timeout")
+	}
+}
+
+// ─── Concurrent access ────────────────────────────────────────────────────
+
+func TestBroadcast_ConcurrentSafe(t *testing.T) {
+	h := NewHub(nil)
+	clients := make([]*Client, 10)
+	h.mu.Lock()
+	for i := 0; i < 10; i++ {
+		clients[i] = mockClient("", 100)
+		h.clients[clients[i]] = true
+	}
+	h.mu.Unlock()
+
+	var wg sync.WaitGroup
+	for i := 0; i < 5; i++ {
+		wg.Add(1)
+		go func(id int) {
+			defer wg.Done()
+			for j := 0; j < 20; j++ {
+				h.Broadcast(models.WSMessage{Event: "ping", Payload: []byte(`"concurrent"`)})
+
+			}
+		}(i)
+	}
+
+	wg.Wait() // should not deadlock or panic
+}