From 34179e64a30d5b58024b6cec9f5c6be8f9427e41 Mon Sep 17 00:00:00 2001 From: hongming Date: Sun, 24 May 2026 19:23:34 -0700 Subject: [PATCH] fix: require workspace name confirmation on delete --- canvas/e2e/filestab-smoke.spec.ts | 10 ++-- canvas/src/components/Canvas.tsx | 5 +- canvas/src/components/ProvisioningTimeout.tsx | 5 +- .../__tests__/OrgCancelButton.test.tsx | 4 +- .../src/components/canvas/OrgCancelButton.tsx | 1 + .../canvas/__tests__/OrgCancelButton.test.tsx | 4 +- canvas/src/components/tabs/DetailsTab.tsx | 4 +- .../tabs/__tests__/DetailsTab.test.tsx | 4 +- canvas/src/lib/api.ts | 2 + canvas/src/store/canvas.ts | 5 +- docs/api-reference.md | 2 +- tests/e2e/_lib.sh | 29 +++++++++-- tests/e2e/test_a2a_e2e.sh | 2 +- tests/e2e/test_activity_e2e.sh | 2 +- tests/e2e/test_api.sh | 10 ++-- tests/e2e/test_chat_upload_e2e.sh | 1 + tests/e2e/test_claude_code_e2e.sh | 6 +-- tests/e2e/test_comprehensive_e2e.sh | 25 +++++---- tests/e2e/test_dev_mode.sh | 2 +- tests/e2e/test_notify_attachments_e2e.sh | 4 +- tests/e2e/test_peer_visibility_mcp_local.sh | 4 +- tests/e2e/test_poll_mode_chat_upload_e2e.sh | 4 +- tests/e2e/test_poll_mode_e2e.sh | 4 +- tests/e2e/test_priority_runtimes_e2e.sh | 4 +- tests/e2e/test_today_pr_coverage_e2e.sh | 8 ++- tests/e2e/test_workspace_abilities_e2e.sh | 8 ++- .../handlers/handlers_extended_test.go | 3 ++ .../internal/handlers/workspace_crud.go | 47 +++++++++++++++++ .../internal/handlers/workspace_crud_test.go | 52 +++++++++++++++++++ .../internal/handlers/workspace_test.go | 18 +++++++ 30 files changed, 232 insertions(+), 47 deletions(-) diff --git a/canvas/e2e/filestab-smoke.spec.ts b/canvas/e2e/filestab-smoke.spec.ts index 05e86610b..1ab32eb20 100644 --- a/canvas/e2e/filestab-smoke.spec.ts +++ b/canvas/e2e/filestab-smoke.spec.ts @@ -15,9 +15,11 @@ test("FilesTab renders after split", async ({ page, request }) => { // Clean slate const { workspaces } = await request .get("http://localhost:8080/workspaces") - .then(async (r) => ({ workspaces: (await r.json()) as Array<{ id: string }> })); + .then(async (r) => ({ workspaces: (await r.json()) as Array<{ id: string; name: string }> })); for (const w of workspaces) { - await request.delete(`http://localhost:8080/workspaces/${w.id}?confirm=true`); + await request.delete(`http://localhost:8080/workspaces/${w.id}?confirm=true`, { + headers: { "X-Confirm-Name": w.name }, + }); } // Create a workspace @@ -80,5 +82,7 @@ test("FilesTab renders after split", async ({ page, request }) => { await expect(editorEmpty.first()).toBeVisible({ timeout: 5_000 }); // Cleanup - await request.delete(`http://localhost:8080/workspaces/${wsId}?confirm=true`); + await request.delete(`http://localhost:8080/workspaces/${wsId}?confirm=true`, { + headers: { "X-Confirm-Name": "FilesTab Smoke" }, + }); }); diff --git a/canvas/src/components/Canvas.tsx b/canvas/src/components/Canvas.tsx index 888343b0e..2bb68e447 100644 --- a/canvas/src/components/Canvas.tsx +++ b/canvas/src/components/Canvas.tsx @@ -232,7 +232,10 @@ function CanvasInner() { } state.beginDelete(subtree); try { - await api.del(`/workspaces/${id}?confirm=true`); + const workspaceName = state.nodes.find((n) => n.id === id)?.data.name ?? ""; + await api.del(`/workspaces/${id}?confirm=true`, { + headers: { "X-Confirm-Name": workspaceName }, + }); // Mirror the server-side cascade locally — drop the parent AND // every descendant in one atomic update. The per-descendant // WORKSPACE_REMOVED WS events still arrive (and are no-ops diff --git a/canvas/src/components/ProvisioningTimeout.tsx b/canvas/src/components/ProvisioningTimeout.tsx index 89b70b101..b21be5a4b 100644 --- a/canvas/src/components/ProvisioningTimeout.tsx +++ b/canvas/src/components/ProvisioningTimeout.tsx @@ -242,10 +242,13 @@ export function ProvisioningTimeout({ const handleCancelConfirm = useCallback(async () => { if (!confirmingCancel) return; const workspaceId = confirmingCancel; + const workspaceName = timedOut.find((e) => e.workspaceId === workspaceId)?.workspaceName ?? ""; setConfirmingCancel(null); setCancelling((prev) => new Set(prev).add(workspaceId)); try { - await api.del(`/workspaces/${workspaceId}`); + await api.del(`/workspaces/${workspaceId}`, { + headers: { "X-Confirm-Name": workspaceName }, + }); setTimedOut((prev) => prev.filter((e) => e.workspaceId !== workspaceId)); trackingRef.current.delete(workspaceId); showToast("Deployment cancelled", "info"); diff --git a/canvas/src/components/__tests__/OrgCancelButton.test.tsx b/canvas/src/components/__tests__/OrgCancelButton.test.tsx index a1268b9f4..a4b6eb46a 100644 --- a/canvas/src/components/__tests__/OrgCancelButton.test.tsx +++ b/canvas/src/components/__tests__/OrgCancelButton.test.tsx @@ -272,7 +272,9 @@ describe("OrgCancelButton — API interactions", () => { fireEvent.click(screen.getByRole("button", { name: /cancel deployment of test org/i })); fireEvent.click(screen.getByRole("button", { name: /yes/i })); await act(async () => { /* flush */ }); - expect(mockApiDel).toHaveBeenCalledWith("/workspaces/root-1?confirm=true"); + expect(mockApiDel).toHaveBeenCalledWith("/workspaces/root-1?confirm=true", { + headers: { "X-Confirm-Name": "Test Org" }, + }); }); it("shows success toast on DELETE success", async () => { diff --git a/canvas/src/components/canvas/OrgCancelButton.tsx b/canvas/src/components/canvas/OrgCancelButton.tsx index 7b3025c75..968b0e7ef 100644 --- a/canvas/src/components/canvas/OrgCancelButton.tsx +++ b/canvas/src/components/canvas/OrgCancelButton.tsx @@ -57,6 +57,7 @@ export function OrgCancelButton({ rootId, rootName, workspaceCount }: Props) { try { await api.del<{ status: string }>( `/workspaces/${rootId}?confirm=true`, + { headers: { "X-Confirm-Name": rootName } }, ); showToast(`Cancelled deployment of "${rootName}"`, "success"); // Optimistic local removal — workspace-server broadcasts diff --git a/canvas/src/components/canvas/__tests__/OrgCancelButton.test.tsx b/canvas/src/components/canvas/__tests__/OrgCancelButton.test.tsx index 527a83e41..334f72e3d 100644 --- a/canvas/src/components/canvas/__tests__/OrgCancelButton.test.tsx +++ b/canvas/src/components/canvas/__tests__/OrgCancelButton.test.tsx @@ -199,7 +199,9 @@ describe("OrgCancelButton — Yes / cascade delete", () => { }); // 1) API call hit the cascade-delete endpoint with confirm=true - expect(mockApiDel).toHaveBeenCalledWith("/workspaces/ws-root?confirm=true"); + expect(mockApiDel).toHaveBeenCalledWith("/workspaces/ws-root?confirm=true", { + headers: { "X-Confirm-Name": "My Org" }, + }); // 2) beginDelete locked the WHOLE subtree (root + 2 children) — NOT the unrelated node expect(mockState.beginDelete).toHaveBeenCalledTimes(1); diff --git a/canvas/src/components/tabs/DetailsTab.tsx b/canvas/src/components/tabs/DetailsTab.tsx index ba781c0fb..197015ecb 100644 --- a/canvas/src/components/tabs/DetailsTab.tsx +++ b/canvas/src/components/tabs/DetailsTab.tsx @@ -93,7 +93,9 @@ export function DetailsTab({ workspaceId, data }: Props) { const handleDelete = async () => { setDeleteError(null); try { - await api.del(`/workspaces/${workspaceId}?confirm=true`); + await api.del(`/workspaces/${workspaceId}?confirm=true`, { + headers: { "X-Confirm-Name": name }, + }); // Mirror the server-side cascade — drop the row + every // descendant locally so the canvas reflects the deletion // immediately, even when the WS is dead and the per-descendant diff --git a/canvas/src/components/tabs/__tests__/DetailsTab.test.tsx b/canvas/src/components/tabs/__tests__/DetailsTab.test.tsx index 7b3bb053c..3c7bb59fd 100644 --- a/canvas/src/components/tabs/__tests__/DetailsTab.test.tsx +++ b/canvas/src/components/tabs/__tests__/DetailsTab.test.tsx @@ -290,7 +290,9 @@ describe("DetailsTab — delete workflow", () => { ) as HTMLButtonElement; fireEvent(confirmBtn, new MouseEvent("click", { bubbles: true })); await flush(); - expect(mockApi.del).toHaveBeenCalledWith("/workspaces/ws-1?confirm=true"); + expect(mockApi.del).toHaveBeenCalledWith("/workspaces/ws-1?confirm=true", { + headers: { "X-Confirm-Name": "Test Workspace" }, + }); expect(mockRemoveSubtree).toHaveBeenCalledWith("ws-1"); expect(mockSelectNode).toHaveBeenCalledWith(null); }); diff --git a/canvas/src/lib/api.ts b/canvas/src/lib/api.ts index 83c6b0651..436a779f8 100644 --- a/canvas/src/lib/api.ts +++ b/canvas/src/lib/api.ts @@ -23,6 +23,7 @@ const DEFAULT_TIMEOUT_MS = 35_000; export interface RequestOptions { timeoutMs?: number; + headers?: Record; } /** @@ -76,6 +77,7 @@ async function request( const headers: Record = { "Content-Type": "application/json", ...platformAuthHeaders(), + ...(options?.headers ?? {}), }; // Re-read slug locally for the 401 handler below — `headers` already // has it, but the 401 branch needs the bare value to gate the diff --git a/canvas/src/store/canvas.ts b/canvas/src/store/canvas.ts index 7cedeaee9..7f90e8937 100644 --- a/canvas/src/store/canvas.ts +++ b/canvas/src/store/canvas.ts @@ -337,8 +337,11 @@ export const useCanvasStore = create((set, get) => ({ }, batchDelete: async () => { const ids = Array.from(get().selectedNodeIds); + const names = new Map(get().nodes.map((node) => [node.id, node.data.name])); const results = await Promise.allSettled( - ids.map((id) => api.del(`/workspaces/${id}`)) + ids.map((id) => api.del(`/workspaces/${id}`, { + headers: { "X-Confirm-Name": names.get(id) ?? "" }, + })) ); const failed: string[] = []; results.forEach((r, i) => { diff --git a/docs/api-reference.md b/docs/api-reference.md index cd43f3937..7a73b112e 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -26,7 +26,7 @@ Full contract: `docs/runbooks/admin-auth.md`. |--------|------|---------| | GET | /health | inline | | GET | /metrics | metrics.Handler() — Prometheus text format; no auth, scrape-safe | -| POST/GET/PATCH/DELETE | /workspaces[/:id] | workspace.go — `GET /workspaces`, `POST /workspaces`, and `DELETE /workspaces/:id` require `AdminAuth`. `PATCH /workspaces/:id` enforces field-level authz: cosmetic fields (name, role, x, y, canvas) pass through; sensitive fields (tier, parent_id, runtime, workspace_dir) require a valid bearer token when any live token exists. | +| POST/GET/PATCH/DELETE | /workspaces[/:id] | workspace.go — `GET /workspaces`, `POST /workspaces`, and `DELETE /workspaces/:id` require `AdminAuth`. `DELETE /workspaces/:id` also requires `X-Confirm-Name: `; cascading deletes still require `?confirm=true`. `PATCH /workspaces/:id` enforces field-level authz: cosmetic fields (name, role, x, y, canvas) pass through; sensitive fields (tier, parent_id, runtime, workspace_dir) require a valid bearer token when any live token exists. | | GET/PATCH | /workspaces/:id/config | workspace.go | | GET/POST | /workspaces/:id/memory | workspace.go | | DELETE | /workspaces/:id/memory/:key | workspace.go | diff --git a/tests/e2e/_lib.sh b/tests/e2e/_lib.sh index 31a19e828..6ade61136 100755 --- a/tests/e2e/_lib.sh +++ b/tests/e2e/_lib.sh @@ -45,12 +45,31 @@ e2e_mint_workspace_token() { printf '%s' "$json" | python3 -c "import json,sys; print(json.load(sys.stdin)['auth_token'])" } -e2e_cleanup_all_workspaces() { - for _wid in $(curl -s "$BASE/workspaces" | python3 -c "import json,sys +e2e_delete_workspace() { + local wid="$1" + local name="${2:-}" + shift 2 || true + local curl_args=("$@") + if [ -z "$wid" ]; then + return 0 + fi + if [ -z "$name" ]; then + name=$(curl -s "$BASE/workspaces/$wid" "${curl_args[@]}" | python3 -c "import json,sys try: - [print(w['id']) for w in json.load(sys.stdin)] + print(json.load(sys.stdin).get('name','')) except Exception: - pass" 2>/dev/null); do - curl -s -X DELETE "$BASE/workspaces/$_wid?confirm=true" > /dev/null || true + pass" 2>/dev/null || true) + fi + curl -s -X DELETE "$BASE/workspaces/$wid?confirm=true" \ + -H "X-Confirm-Name: $name" "${curl_args[@]}" > /dev/null || true +} + +e2e_cleanup_all_workspaces() { + curl -s "$BASE/workspaces" | python3 -c "import json,sys +try: + [print(f\"{w.get('id','')}\\t{w.get('name','')}\") for w in json.load(sys.stdin)] +except Exception: + pass" 2>/dev/null | while IFS=$'\t' read -r _wid _name; do + e2e_delete_workspace "$_wid" "$_name" done } diff --git a/tests/e2e/test_a2a_e2e.sh b/tests/e2e/test_a2a_e2e.sh index 7378a780d..cf6f748d6 100644 --- a/tests/e2e/test_a2a_e2e.sh +++ b/tests/e2e/test_a2a_e2e.sh @@ -137,7 +137,7 @@ R=$(curl -s --max-time 10 -X POST "$BASE/workspaces/$OFFLINE_ID/a2a" \ -d '{"method":"message/send","params":{"message":{"role":"user","parts":[{"type":"text","text":"test"}]}}}') check "Offline workspace returns error" '"error"' "$R" # Clean up -curl -s -X DELETE "$BASE/workspaces/$OFFLINE_ID" >/dev/null +e2e_delete_workspace "$OFFLINE_ID" "Offline Test" echo "" # ======================================== diff --git a/tests/e2e/test_activity_e2e.sh b/tests/e2e/test_activity_e2e.sh index 12c673aa0..b3a6b2b6c 100755 --- a/tests/e2e/test_activity_e2e.sh +++ b/tests/e2e/test_activity_e2e.sh @@ -235,7 +235,7 @@ R=$(curl -s "$BASE/workspaces/$TEMP_ID/activity") check "Activity in correct workspace" 'Temp workspace log' "$R" # Cleanup -curl -s -X DELETE "$BASE/workspaces/$TEMP_ID" > /dev/null +e2e_delete_workspace "$TEMP_ID" "Activity Test Workspace" # ---------- Edge Cases ---------- echo "" diff --git a/tests/e2e/test_api.sh b/tests/e2e/test_api.sh index dcf3c10fb..770e627c8 100644 --- a/tests/e2e/test_api.sh +++ b/tests/e2e/test_api.sh @@ -289,7 +289,9 @@ R=$(curl -s "$BASE/workspaces" -H "Authorization: Bearer $ECHO_TOKEN") check "current_task in list response" '"current_task"' "$R" # Test 21: Delete -R=$(acurl -X DELETE "$BASE/workspaces/$ECHO_ID" -H "Authorization: Bearer $ECHO_TOKEN") +R=$(acurl -X DELETE "$BASE/workspaces/$ECHO_ID?confirm=true" \ + -H "Authorization: Bearer $ECHO_TOKEN" \ + -H "X-Confirm-Name: Echo Agent v2") check "DELETE /workspaces/:id" '"status":"removed"' "$R" R=$(curl -s "$BASE/workspaces" -H "Authorization: Bearer $SUM_TOKEN") @@ -310,7 +312,9 @@ ORIG_TIER=$(echo "$BUNDLE" | python3 -c "import sys,json; print(json.load(sys.st # Delete the workspace — use SUM_TOKEN (per-workspace) for WorkspaceAuth # and ADMIN_TOKEN for the AdminAuth layer. -R=$(curl -s -X DELETE "$BASE/workspaces/$SUM_ID" -H "Authorization: Bearer $SUM_TOKEN") +R=$(curl -s -X DELETE "$BASE/workspaces/$SUM_ID?confirm=true" \ + -H "Authorization: Bearer $SUM_TOKEN" \ + -H "X-Confirm-Name: Summarizer Agent") check "Delete before re-import" '"status":"removed"' "$R" # After deleting both workspaces, all per-workspace tokens are revoked. @@ -381,7 +385,7 @@ REBUNDLE=$(curl -s "$BASE/bundles/export/$NEW_ID" -H "Authorization: Bearer $NEW check "Re-exported bundle has agent_card" '"agent_card"' "$REBUNDLE" # Clean up — use the token just issued to the re-imported workspace -curl -s -X DELETE "$BASE/workspaces/$NEW_ID" -H "Authorization: Bearer $NEW_TOKEN" > /dev/null +e2e_delete_workspace "$NEW_ID" "$ORIG_NAME" -H "Authorization: Bearer $NEW_TOKEN" echo "" echo "=== Results: $PASS passed, $FAIL failed ===" diff --git a/tests/e2e/test_chat_upload_e2e.sh b/tests/e2e/test_chat_upload_e2e.sh index e8dcda201..2ae47ea6a 100755 --- a/tests/e2e/test_chat_upload_e2e.sh +++ b/tests/e2e/test_chat_upload_e2e.sh @@ -39,6 +39,7 @@ cleanup() { set +e if [ -n "$PARENT" ]; then curl -sS -X DELETE "$BASE/workspaces/$PARENT?confirm=true&purge=true" \ + -H "X-Confirm-Name: e2e-chat-upload" \ ${PARENT_TOK:+-H "Authorization: Bearer $PARENT_TOK"} >/dev/null 2>&1 fi exit $rc diff --git a/tests/e2e/test_claude_code_e2e.sh b/tests/e2e/test_claude_code_e2e.sh index 4b6c0d1b4..30ec73ded 100755 --- a/tests/e2e/test_claude_code_e2e.sh +++ b/tests/e2e/test_claude_code_e2e.sh @@ -10,6 +10,8 @@ set -euo pipefail PLATFORM="http://localhost:8080" +export BASE="$PLATFORM" +source "$(dirname "$0")/_lib.sh" PASS=0 FAIL=0 ERRORS="" @@ -38,9 +40,7 @@ else fi # --- Clean existing workspaces --- -for id in $(curl -s $PLATFORM/workspaces | python3 -c "import sys,json; [print(w['id']) for w in json.load(sys.stdin)]" 2>/dev/null); do - curl -s -X DELETE "$PLATFORM/workspaces/$id" > /dev/null -done +e2e_cleanup_all_workspaces # shellcheck disable=SC2046 # Intentional word-split over container IDs docker stop $(docker ps -q --filter "name=ws-") 2>/dev/null || true # shellcheck disable=SC2046 diff --git a/tests/e2e/test_comprehensive_e2e.sh b/tests/e2e/test_comprehensive_e2e.sh index 619c97998..92af762d2 100755 --- a/tests/e2e/test_comprehensive_e2e.sh +++ b/tests/e2e/test_comprehensive_e2e.sh @@ -228,10 +228,12 @@ else fi # Clean up runtime test workspaces -for rt_id in $RT_CC_ID $RT_CX_ID $RT_HM_ID; do - curl -s -X DELETE "$BASE/workspaces/$rt_id?confirm=true" > /dev/null 2>&1 - sleep 0.3 -done +e2e_delete_workspace "$RT_CC_ID" "RT Claude" +sleep 0.3 +e2e_delete_workspace "$RT_CX_ID" "RT Codex" +sleep 0.3 +e2e_delete_workspace "$RT_HM_ID" "RT Hermes" +sleep 0.3 # ============================================================ # Section 3: Registry & Heartbeat @@ -550,16 +552,21 @@ check "Import bundle" '"status"' "$R" echo "" echo "--- Section 14: Cleanup & Delete ---" -# Delete with children — should require confirmation +# Delete without name confirmation should be rejected before cascade. R=$(curl -s -X DELETE "$BASE/workspaces/$PM_ID") -check "Delete PM requires confirmation" '"confirmation_required"' "$R" +check "Delete PM requires name confirmation" '"destructive_action_requires_confirmation"' "$R" + +# Delete with name confirmation but without cascade confirmation should +# still require explicit child confirmation. +R=$(curl -s -X DELETE "$BASE/workspaces/$PM_ID" -H "X-Confirm-Name: Test PM") +check "Delete PM requires cascade confirmation" '"confirmation_required"' "$R" # Delete with confirmation -R=$(curl -s -X DELETE "$BASE/workspaces/$PM_ID?confirm=true") +R=$(curl -s -X DELETE "$BASE/workspaces/$PM_ID?confirm=true" -H "X-Confirm-Name: Test PM") check "Delete PM cascades" '"cascade_deleted"' "$R" # Delete outsider -curl -s -X DELETE "$BASE/workspaces/$OUTSIDER_ID?confirm=true" > /dev/null +e2e_delete_workspace "$OUTSIDER_ID" "Test Outsider" # Clean up remaining workspaces (bundle imports, runtime test workspaces, etc.) sleep 2 @@ -568,7 +575,7 @@ import json, sys, subprocess, time ws = json.load(sys.stdin) for w in ws: time.sleep(0.5) # avoid rate limit - subprocess.run(['curl', '-s', '-X', 'DELETE', '$BASE/workspaces/' + w['id'] + '?confirm=true'], capture_output=True) + subprocess.run(['curl', '-s', '-X', 'DELETE', '$BASE/workspaces/' + w['id'] + '?confirm=true', '-H', 'X-Confirm-Name: ' + w.get('name','')], capture_output=True) " 2>/dev/null # Poll for clean state up to 30s — DB cascade + container stop is async on busy systems diff --git a/tests/e2e/test_dev_mode.sh b/tests/e2e/test_dev_mode.sh index 1e01a94df..8406762bb 100755 --- a/tests/e2e/test_dev_mode.sh +++ b/tests/e2e/test_dev_mode.sh @@ -134,7 +134,7 @@ fi # ---------------------------------------------------------------------- # Cleanup # ---------------------------------------------------------------------- -curl -s -X DELETE "$BASE/workspaces/$WS_ID?confirm=true" > /dev/null || true +e2e_delete_workspace "$WS_ID" "Dev-Mode-Test" echo "" echo "=== Results: $PASS passed, $FAIL failed ===" diff --git a/tests/e2e/test_notify_attachments_e2e.sh b/tests/e2e/test_notify_attachments_e2e.sh index 9c9141e8d..0d92bfe46 100755 --- a/tests/e2e/test_notify_attachments_e2e.sh +++ b/tests/e2e/test_notify_attachments_e2e.sh @@ -32,7 +32,7 @@ cleanup() { # Workspace teardown — best-effort, ignore errors so an unrelated CP # outage doesn't shadow a real test failure. if [ -n "$WSID" ]; then - curl -s -X DELETE "$BASE/workspaces/$WSID?confirm=true" > /dev/null || true + e2e_delete_workspace "$WSID" "Notify E2E" fi # /tmp scratch — pre-fix only ran on success path (the unconditional # rm at the bottom of the script). Trap-based path lets the file leak @@ -89,7 +89,7 @@ except Exception: ') for _wid in $PRIOR; do echo "Sweeping leftover Notify E2E workspace: $_wid" - curl -s -X DELETE "$BASE/workspaces/$_wid?confirm=true" > /dev/null || true + e2e_delete_workspace "$_wid" "Notify E2E" done # model is required at the Create boundary (CTO 2026-05-22 SSOT — see diff --git a/tests/e2e/test_peer_visibility_mcp_local.sh b/tests/e2e/test_peer_visibility_mcp_local.sh index 8390b880a..11cbd18ca 100755 --- a/tests/e2e/test_peer_visibility_mcp_local.sh +++ b/tests/e2e/test_peer_visibility_mcp_local.sh @@ -113,7 +113,7 @@ teardown() { log "[teardown] deleting ${#CREATED_WSIDS[@]} workspace(s) this run created (scoped)" for wid in ${CREATED_WSIDS[@]+"${CREATED_WSIDS[@]}"}; do [ -n "$wid" ] || continue - curl -s -X DELETE "$BASE/workspaces/$wid?confirm=true" ${ADMIN_AUTH[@]+"${ADMIN_AUTH[@]}"} >/dev/null 2>&1 || true + e2e_delete_workspace "$wid" "" ${ADMIN_AUTH[@]+"${ADMIN_AUTH[@]}"} done exit $rc } @@ -131,7 +131,7 @@ except Exception: ' 2>/dev/null) for _wid in $PRIOR; do log "Pre-sweeping prior PV-Local workspace: $_wid" - curl -s -X DELETE "$BASE/workspaces/$_wid?confirm=true" ${ADMIN_AUTH[@]+"${ADMIN_AUTH[@]}"} >/dev/null 2>&1 || true + e2e_delete_workspace "$_wid" "" ${ADMIN_AUTH[@]+"${ADMIN_AUTH[@]}"} done # ─── Local-stack preflight ───────────────────────────────────────────── diff --git a/tests/e2e/test_poll_mode_chat_upload_e2e.sh b/tests/e2e/test_poll_mode_chat_upload_e2e.sh index fbed604f2..c708c3489 100755 --- a/tests/e2e/test_poll_mode_chat_upload_e2e.sh +++ b/tests/e2e/test_poll_mode_chat_upload_e2e.sh @@ -48,8 +48,8 @@ TMPDIR_E2E=$(mktemp -d -t poll-chat-upload-e2e-XXXXXX) cleanup() { local rc=$? - curl -s -X DELETE "$BASE/workspaces/$WS_A?confirm=true" >/dev/null 2>&1 || true - curl -s -X DELETE "$BASE/workspaces/$WS_B?confirm=true" >/dev/null 2>&1 || true + e2e_delete_workspace "$WS_A" "poll-chat-upload-test-a" + e2e_delete_workspace "$WS_B" "poll-chat-upload-test-b" rm -rf "$TMPDIR_E2E" exit $rc } diff --git a/tests/e2e/test_poll_mode_e2e.sh b/tests/e2e/test_poll_mode_e2e.sh index d1ffeea75..46daa8584 100755 --- a/tests/e2e/test_poll_mode_e2e.sh +++ b/tests/e2e/test_poll_mode_e2e.sh @@ -43,8 +43,8 @@ INVALID_PROBE_ID="$(gen_uuid)" cleanup() { local rc=$? # Best-effort delete; non-fatal if the row was never created. - curl -s -X DELETE "$BASE/workspaces/$POLL_WS_ID" >/dev/null || true - curl -s -X DELETE "$BASE/workspaces/$CALLER_WS_ID" >/dev/null || true + e2e_delete_workspace "$POLL_WS_ID" "poll-mode-test" + e2e_delete_workspace "$CALLER_WS_ID" "poll-cross-test" exit $rc } trap cleanup EXIT diff --git a/tests/e2e/test_priority_runtimes_e2e.sh b/tests/e2e/test_priority_runtimes_e2e.sh index 41bf25ee3..7785f87f4 100755 --- a/tests/e2e/test_priority_runtimes_e2e.sh +++ b/tests/e2e/test_priority_runtimes_e2e.sh @@ -53,7 +53,7 @@ cleanup() { # ${VAR[@]+"…"} form expands to nothing when the array is unset/empty # so the loop body is skipped cleanly. Hits the skip-no-keys path. for wid in ${CREATED_WSIDS[@]+"${CREATED_WSIDS[@]}"}; do - [ -n "$wid" ] && curl -s -X DELETE "$BASE/workspaces/$wid?confirm=true" > /dev/null || true + [ -n "$wid" ] && e2e_delete_workspace "$wid" "" done } trap cleanup EXIT @@ -74,7 +74,7 @@ except Exception: ') for _wid in $PRIOR; do echo "Sweeping prior workspace: $_wid" - curl -s -X DELETE "$BASE/workspaces/$_wid?confirm=true" > /dev/null || true + e2e_delete_workspace "$_wid" "" done # Block until $1 reaches one of $2 (space-separated states), or $3 sec elapse. diff --git a/tests/e2e/test_today_pr_coverage_e2e.sh b/tests/e2e/test_today_pr_coverage_e2e.sh index 0ab6c6cb5..fe1c32208 100755 --- a/tests/e2e/test_today_pr_coverage_e2e.sh +++ b/tests/e2e/test_today_pr_coverage_e2e.sh @@ -364,7 +364,13 @@ for wid in "${WS_A_ID:-}" "${WS_B_ID:-}"; do DELETE_AUTH=("${WS_B_AUTH[@]}") fi fi - curl -s -X DELETE "$BASE/workspaces/$wid?confirm=true" "${DELETE_AUTH[@]}" > /dev/null || true + if [ "$wid" = "${WS_A_ID:-}" ]; then + e2e_delete_workspace "$wid" "$WS_A_NAME" "${DELETE_AUTH[@]}" + elif [ "$wid" = "${WS_B_ID:-}" ]; then + e2e_delete_workspace "$wid" "$WS_B_NAME" "${DELETE_AUTH[@]}" + else + e2e_delete_workspace "$wid" "" "${DELETE_AUTH[@]}" + fi echo "deleted $wid" done diff --git a/tests/e2e/test_workspace_abilities_e2e.sh b/tests/e2e/test_workspace_abilities_e2e.sh index 8a0012358..f097c8688 100755 --- a/tests/e2e/test_workspace_abilities_e2e.sh +++ b/tests/e2e/test_workspace_abilities_e2e.sh @@ -31,7 +31,11 @@ RECEIVER_TOKEN="" cleanup() { for wid in "$SENDER_ID" "$RECEIVER_ID"; do if [ -n "$wid" ]; then - curl -s -X DELETE "$BASE/workspaces/$wid?confirm=true" > /dev/null || true + if [ "$wid" = "$SENDER_ID" ]; then + e2e_delete_workspace "$wid" "Abilities Sender" + else + e2e_delete_workspace "$wid" "Abilities Receiver" + fi fi done } @@ -88,7 +92,7 @@ except Exception: ") for _wid in $PRIOR; do echo "Sweeping leftover '$NAME' workspace: $_wid" - curl -s -X DELETE "$BASE/workspaces/$_wid?confirm=true" > /dev/null || true + e2e_delete_workspace "$_wid" "$NAME" done done diff --git a/workspace-server/internal/handlers/handlers_extended_test.go b/workspace-server/internal/handlers/handlers_extended_test.go index b4250e6b8..25fe4a096 100644 --- a/workspace-server/internal/handlers/handlers_extended_test.go +++ b/workspace-server/internal/handlers/handlers_extended_test.go @@ -21,6 +21,8 @@ func TestExtended_WorkspaceDelete(t *testing.T) { broadcaster := newTestBroadcaster() handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", "/tmp/configs") + expectWorkspaceDeleteLookup(mock, wsDelID, "Delete Me", 0, "running") + // Expect children query — no children mock.ExpectQuery("SELECT id, name FROM workspaces WHERE parent_id"). WithArgs(wsDelID). @@ -59,6 +61,7 @@ func TestExtended_WorkspaceDelete(t *testing.T) { c, _ := gin.CreateTestContext(w) c.Params = gin.Params{{Key: "id", Value: wsDelID}} c.Request = httptest.NewRequest("DELETE", "/workspaces/"+wsDelID+"?confirm=true", nil) + c.Request.Header.Set("X-Confirm-Name", "Delete Me") handler.Delete(c) diff --git a/workspace-server/internal/handlers/workspace_crud.go b/workspace-server/internal/handlers/workspace_crud.go index 9ba810b21..23f489db6 100644 --- a/workspace-server/internal/handlers/workspace_crud.go +++ b/workspace-server/internal/handlers/workspace_crud.go @@ -325,6 +325,37 @@ func (h *WorkspaceHandler) Delete(c *gin.Context) { return } + var workspaceName, workspaceStatus string + var activeTasks int + if err := db.DB.QueryRowContext(ctx, + `SELECT name, COALESCE(active_tasks, 0), status FROM workspaces WHERE id = $1`, id, + ).Scan(&workspaceName, &activeTasks, &workspaceStatus); err != nil { + if err == sql.ErrNoRows { + c.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"}) + return + } + log.Printf("Delete: workspace lookup failed for %s: %v", id, err) + c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to check workspace"}) + return + } + if workspaceStatus == string(models.StatusRemoved) { + c.JSON(http.StatusGone, gin.H{"error": "workspace removed", "id": id}) + return + } + + if c.GetHeader("X-Confirm-Name") != workspaceName { + childCount, scheduleCount := destructiveDeleteCounts(ctx, id) + c.JSON(http.StatusBadRequest, gin.H{ + "error": "destructive_action_requires_confirmation", + "hint": "Re-send the same request with header X-Confirm-Name: " + workspaceName, + "workspace_name": workspaceName, + "active_tasks": activeTasks, + "child_count": childCount, + "schedule_count": scheduleCount, + }) + return + } + // Check for children rows, err := db.DB.QueryContext(ctx, `SELECT id, name FROM workspaces WHERE parent_id = $1 AND status != 'removed'`, id) @@ -450,6 +481,22 @@ func (h *WorkspaceHandler) Delete(c *gin.Context) { c.JSON(http.StatusOK, gin.H{"status": "removed", "cascade_deleted": len(descendantIDs)}) } +func destructiveDeleteCounts(ctx context.Context, id string) (childCount int, scheduleCount int) { + if err := db.DB.QueryRowContext(ctx, + `SELECT COUNT(*) FROM workspaces WHERE parent_id = $1 AND status != 'removed'`, id, + ).Scan(&childCount); err != nil { + log.Printf("Delete: child count failed for %s: %v", id, err) + childCount = 0 + } + if err := db.DB.QueryRowContext(ctx, + `SELECT COUNT(*) FROM workspace_schedules WHERE workspace_id = $1 AND enabled = true`, id, + ).Scan(&scheduleCount); err != nil { + log.Printf("Delete: schedule count failed for %s: %v", id, err) + scheduleCount = 0 + } + return childCount, scheduleCount +} + // CascadeDelete performs the cascade-removal sequence used by the HTTP // DELETE handler and by OrgImport's reconcile mode: walk descendants, mark // self+descendants 'removed' first (#73 race guard), stop containers / EC2s, diff --git a/workspace-server/internal/handlers/workspace_crud_test.go b/workspace-server/internal/handlers/workspace_crud_test.go index 624447752..8010e7c00 100644 --- a/workspace-server/internal/handlers/workspace_crud_test.go +++ b/workspace-server/internal/handlers/workspace_crud_test.go @@ -44,6 +44,13 @@ func expectWorkspaceLiveTokenCount(mock sqlmock.Sqlmock, count int) { WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(count)) } +func expectWorkspaceDeleteLookup(mock sqlmock.Sqlmock, id, name string, activeTasks int, status string) { + mock.ExpectQuery(`SELECT name, COALESCE\(active_tasks, 0\), status FROM workspaces WHERE id = \$1`). + WithArgs(id). + WillReturnRows(sqlmock.NewRows([]string{"name", "active_tasks", "status"}). + AddRow(name, activeTasks, status)) +} + // ---------- State ---------- func TestState_LegacyWorkspaceNoLiveToken(t *testing.T) { @@ -304,12 +311,15 @@ func TestDelete_HasChildrenWithoutConfirm(t *testing.T) { h := newWorkspaceCrudHandler(t) r.DELETE("/workspaces/:id", h.Delete) + expectWorkspaceDeleteLookup(mock, wsID, "Parent Workspace", 0, "running") + mock.ExpectQuery(`SELECT id, name FROM workspaces WHERE parent_id = \$1 AND status != 'removed'`). WithArgs(wsID). WillReturnRows(sqlmock.NewRows([]string{"id", "name"}). AddRow("child-1", "Child Workspace")) req, _ := http.NewRequest("DELETE", "/workspaces/"+wsID, nil) + req.Header.Set("X-Confirm-Name", "Parent Workspace") // No ?confirm=true w := httptest.NewRecorder() r.ServeHTTP(w, req) @@ -330,17 +340,59 @@ func TestDelete_HasChildrenWithoutConfirm(t *testing.T) { } } +func TestDelete_LeafWithoutConfirmName(t *testing.T) { + wsID := "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" + mock, r := setupWorkspaceCrudTest(t) + h := newWorkspaceCrudHandler(t) + r.DELETE("/workspaces/:id", h.Delete) + + expectWorkspaceDeleteLookup(mock, wsID, "SEO Agent", 3, "running") + mock.ExpectQuery(`SELECT COUNT\(\*\) FROM workspaces WHERE parent_id = \$1 AND status != 'removed'`). + WithArgs(wsID). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(0)) + mock.ExpectQuery(`SELECT COUNT\(\*\) FROM workspace_schedules WHERE workspace_id = \$1 AND enabled = true`). + WithArgs(wsID). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(11)) + + req, _ := http.NewRequest("DELETE", "/workspaces/"+wsID, nil) + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d: %s", w.Code, w.Body.String()) + } + + var resp map[string]interface{} + if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { + t.Fatalf("failed to unmarshal: %v", err) + } + if resp["error"] != "destructive_action_requires_confirmation" { + t.Errorf("error should require destructive confirmation, got %v", resp["error"]) + } + if resp["workspace_name"] != "SEO Agent" { + t.Errorf("workspace_name should be surfaced for confirmation") + } + if resp["active_tasks"] != float64(3) { + t.Errorf("active_tasks should be 3, got %v", resp["active_tasks"]) + } + if resp["schedule_count"] != float64(11) { + t.Errorf("schedule_count should be 11, got %v", resp["schedule_count"]) + } +} + func TestDelete_ChildrenCheckQueryError(t *testing.T) { wsID := "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" mock, r := setupWorkspaceCrudTest(t) h := newWorkspaceCrudHandler(t) r.DELETE("/workspaces/:id", h.Delete) + expectWorkspaceDeleteLookup(mock, wsID, "Workspace", 0, "running") mock.ExpectQuery(`SELECT id, name FROM workspaces WHERE parent_id = \$1 AND status != 'removed'`). WithArgs(wsID). WillReturnError(sql.ErrConnDone) req, _ := http.NewRequest("DELETE", "/workspaces/"+wsID, nil) + req.Header.Set("X-Confirm-Name", "Workspace") w := httptest.NewRecorder() r.ServeHTTP(w, req) diff --git a/workspace-server/internal/handlers/workspace_test.go b/workspace-server/internal/handlers/workspace_test.go index 30d7fcd76..4e5c320e7 100644 --- a/workspace-server/internal/handlers/workspace_test.go +++ b/workspace-server/internal/handlers/workspace_test.go @@ -919,6 +919,8 @@ func TestWorkspaceDelete_ConfirmationRequired(t *testing.T) { broadcaster := newTestBroadcaster() handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) + expectWorkspaceDeleteLookup(mock, "cccccccc-0007-0000-0000-000000000000", "Parent Workspace", 0, "running") + // Children query returns 2 children mock.ExpectQuery("SELECT id, name FROM workspaces WHERE parent_id"). WithArgs("cccccccc-0007-0000-0000-000000000000"). @@ -931,6 +933,7 @@ func TestWorkspaceDelete_ConfirmationRequired(t *testing.T) { c.Params = gin.Params{{Key: "id", Value: "cccccccc-0007-0000-0000-000000000000"}} // No ?confirm=true c.Request = httptest.NewRequest("DELETE", "/workspaces/ws-parent", nil) + c.Request.Header.Set("X-Confirm-Name", "Parent Workspace") handler.Delete(c) @@ -964,6 +967,8 @@ func TestWorkspaceDelete_CascadeWithChildren(t *testing.T) { broadcaster := newTestBroadcaster() handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) + expectWorkspaceDeleteLookup(mock, "cccccccc-000a-0000-0000-000000000000", "Parent Delete", 0, "running") + // Children query returns 1 child mock.ExpectQuery("SELECT id, name FROM workspaces WHERE parent_id"). WithArgs("cccccccc-000a-0000-0000-000000000000"). @@ -999,6 +1004,7 @@ func TestWorkspaceDelete_CascadeWithChildren(t *testing.T) { c, _ := gin.CreateTestContext(w) c.Params = gin.Params{{Key: "id", Value: "cccccccc-000a-0000-0000-000000000000"}} c.Request = httptest.NewRequest("DELETE", "/workspaces/ws-parent-del?confirm=true", nil) + c.Request.Header.Set("X-Confirm-Name", "Parent Delete") handler.Delete(c) @@ -1034,6 +1040,8 @@ func TestWorkspaceDelete_DisablesSchedules(t *testing.T) { wsID := "dddddddd-0001-0000-0000-000000000000" + expectWorkspaceDeleteLookup(mock, wsID, "Scheduled Workspace", 0, "running") + // No children mock.ExpectQuery("SELECT id, name FROM workspaces WHERE parent_id"). WithArgs(wsID). @@ -1065,6 +1073,7 @@ func TestWorkspaceDelete_DisablesSchedules(t *testing.T) { c, _ := gin.CreateTestContext(w) c.Params = gin.Params{{Key: "id", Value: wsID}} c.Request = httptest.NewRequest("DELETE", "/workspaces/"+wsID+"?confirm=true", nil) + c.Request.Header.Set("X-Confirm-Name", "Scheduled Workspace") handler.Delete(c) @@ -1090,6 +1099,8 @@ func TestWorkspaceDelete_CascadeDisablesDescendantSchedules(t *testing.T) { childID := "dddddddd-0003-0000-0000-000000000000" grandchildID := "dddddddd-0004-0000-0000-000000000000" + expectWorkspaceDeleteLookup(mock, parentID, "Parent Scheduled Workspace", 0, "running") + // Children query returns 1 direct child mock.ExpectQuery("SELECT id, name FROM workspaces WHERE parent_id"). WithArgs(parentID). @@ -1129,6 +1140,7 @@ func TestWorkspaceDelete_CascadeDisablesDescendantSchedules(t *testing.T) { c, _ := gin.CreateTestContext(w) c.Params = gin.Params{{Key: "id", Value: parentID}} c.Request = httptest.NewRequest("DELETE", "/workspaces/"+parentID+"?confirm=true", nil) + c.Request.Header.Set("X-Confirm-Name", "Parent Scheduled Workspace") handler.Delete(c) @@ -1162,6 +1174,8 @@ func TestWorkspaceDelete_ScheduleDisableOnlyTargetsDeletedWorkspace(t *testing.T wsA := "dddddddd-0005-0000-0000-000000000000" // wsB is "dddddddd-0006-0000-0000-000000000000" — NOT part of the delete + expectWorkspaceDeleteLookup(mock, wsA, "Workspace A", 0, "running") + // No children for workspace A mock.ExpectQuery("SELECT id, name FROM workspaces WHERE parent_id"). WithArgs(wsA). @@ -1192,6 +1206,7 @@ func TestWorkspaceDelete_ScheduleDisableOnlyTargetsDeletedWorkspace(t *testing.T c, _ := gin.CreateTestContext(w) c.Params = gin.Params{{Key: "id", Value: wsA}} c.Request = httptest.NewRequest("DELETE", "/workspaces/"+wsA+"?confirm=true", nil) + c.Request.Header.Set("X-Confirm-Name", "Workspace A") handler.Delete(c) @@ -1214,6 +1229,8 @@ func TestWorkspaceDelete_ChildrenQueryError(t *testing.T) { broadcaster := newTestBroadcaster() handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) + expectWorkspaceDeleteLookup(mock, "cccccccc-000c-0000-0000-000000000000", "Error Workspace", 0, "running") + mock.ExpectQuery("SELECT id, name FROM workspaces WHERE parent_id"). WithArgs("cccccccc-000c-0000-0000-000000000000"). WillReturnError(sql.ErrConnDone) @@ -1222,6 +1239,7 @@ func TestWorkspaceDelete_ChildrenQueryError(t *testing.T) { c, _ := gin.CreateTestContext(w) c.Params = gin.Params{{Key: "id", Value: "cccccccc-000c-0000-0000-000000000000"}} c.Request = httptest.NewRequest("DELETE", "/workspaces/ws-err-del?confirm=true", nil) + c.Request.Header.Set("X-Confirm-Name", "Error Workspace") handler.Delete(c) -- 2.52.0