fix: address self-review findings on sandbox stop/start + draft approve

- Fix false-positive success toasts: list-page Stop/Start now report
  success/failure from the mutation's actual outcome, not immediately
  on click. Detail-page Stop/Start gained the error feedback they were
  missing entirely.
- Extract canStopSandbox/canStartSandbox so the list row and detail page
  page can't disagree on which phases allow which action.
- Add STOPPED/STOPPING/STARTING colors to StatusDot (PhaseLabel already
  had them).
- Drop the delete-confirmation copy claiming deletion is "the only way
  to stop a running sandbox" — no longer true now that Stop exists.
- ApproveDraftChunk: accept an optional client-supplied reviewToken
  (the frontend already has it from GetDraftPolicy) and only fall back
  to the extra GetDraft round-trip when the client didn't send one.
- Add handler tests for StopSandbox/StartSandbox (previously untested;
  the SDK mock's Stop/Start literally panicked) and for both
  ApproveDraftChunk token paths.
- Correct .claude/rules/openshell-api.md and CLAUDE.md: backend/proto/
  and backend/internal/gateway/ were removed by the SDK migration (#43)
  and are stale/unused — the vendored Go SDK is the source of truth now,
  not a local proto copy. Also documented ApproveDraftChunk's new
  review_token under the optimistic-concurrency hard fact.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Gage Krumbach <gkrumbac@redhat.com>

Author: Gkrumbach07

.claude/rules/openshell-api.md

@@ -1,19 +1,19 @@
 ---
-description: OpenShell gRPC API reference for dashboard development — proto is source of truth
-globs: "backend/internal/gateway/**,backend/proto/**,frontend/src/api/**,frontend/src/types/**"
+description: OpenShell gRPC API reference for dashboard development — the vendored Go SDK is source of truth
+globs: "backend/internal/sdkclient/**,backend/internal/api/**,frontend/src/api/**,frontend/src/types/**"
 alwaysApply: false
 ---
 
 # OpenShell API Reference
 
-**THE PROTO FILES IN `backend/proto/` ARE THE SOURCE OF TRUTH.** Before writing any gateway wrapper method, REST handler, or TypeScript type, read the actual RPC and message definitions in the proto. Never invent RPCs, fields, or lifecycle states. If a UI idea has no backing RPC, flag it — do not fabricate an endpoint.
+**THE VENDORED GO SDK IS THE SOURCE OF TRUTH.** Since the SDK migration (PR #43), the BFF calls `github.com/NVIDIA/OpenShell/sdk/go` directly — there is no local `backend/proto/` copy or `backend/gen/` anymore (both were removed; `backend/proto/*.proto` predates the migration and is stale/unused — do not edit or regenerate from it). Before writing any handler or TypeScript type, read the actual RPC/message definitions in `$(go env GOMODCACHE)/github.com/!n!v!i!d!i!a/!open!shell/sdk/go@<version>/openshell/v1/` (hand-written client wrappers with doc comments; `types/*.go` for message shapes) or via `go doc`. Never invent RPCs, fields, or lifecycle states. If a UI idea has no backing RPC, flag it — do not fabricate an endpoint. Track upstream via `go get github.com/NVIDIA/OpenShell/sdk/go@latest`.
 
 ## Services
 
-| Service | Proto | What we use |
-|---------|-------|-------------|
-| `openshell.v1.OpenShell` (68 RPCs) | openshell.proto | Sandbox (incl. stop/start), workspace, member, provider, profile, policy, draft policy, logs, SSH, services |
-| `openshell.inference.v1.Inference` (4 RPCs) | inference.proto | Inference route CRUD |
+| Service | SDK package | What we use |
+|---------|-------------|-------------|
+| `openshell.v1.OpenShell` | `openshell/v1` (`Client.Sandboxes()`, `.Workspaces()`, `.Providers()`, `.Policy()`, ...) | Sandbox (incl. stop/start), workspace, member, provider, profile, policy, draft policy, logs, SSH, services |
+| `openshell.inference.v1.Inference` | `openshell/v1` (`Client.Inference()`) | Inference route CRUD |
 
 Skip `GatewayInterceptor`, `SupervisorMiddleware`, `ComputeDriver` — internal/operator only.
 
@@ -29,7 +29,7 @@ Skip `GatewayInterceptor`, `SupervisorMiddleware`, `ComputeDriver` — internal/
 8. **Provider model** (`datamodel.v1.Provider`): metadata, type (profile slug like "claude"/"gitlab"), credentials (map, `secret` option — strip before returning to browser), config (map), credential_expires_at_ms, profile_workspace. No endpoint-URL/status/model fields. Valid types come from `ListProviderProfiles`.
 9. **No list-images API.** Sandbox images are free-text OCI refs; community images by convention `ghcr.io/nvidia/openshell-community/sandboxes/<name>`.
 10. **`GetGatewayInfo` returns only** status, gateway_version, compute_drivers[]. No uptime/db/TLS/auth-mode fields.
-11. **Optimistic concurrency:** `AttachSandboxProvider`, `DetachSandboxProvider`, `UpdateConfig`, `UpdateProviderProfiles` accept `expected_resource_version` — pass the ObjectMeta.resource_version from the last read.
+11. **Optimistic concurrency:** `AttachSandboxProvider`, `DetachSandboxProvider`, `UpdateConfig`, `UpdateProviderProfiles` accept `expected_resource_version` — pass the ObjectMeta.resource_version from the last read. `ApproveDraftChunk` similarly takes a `review_token` (from the chunk's last `GetDraft` read) pinning the exact evaluated candidate; the BFF falls back to resolving it server-side if the client doesn't send one.
 12. **Workspace scoping:** most requests carry a `workspace` field (empty = "default"); list RPCs offer `all_workspaces`. Workspaces themselves are top-level.
 13. **Secret fields** are annotated `[(openshell.options.v1.secret) = true]` in proto — grep for `secret` when adding a wrapper and never serialize those fields to the frontend.
 

CLAUDE.md

@@ -53,7 +53,7 @@ Each rule has a corresponding Architecture Decision Record in [`docs/adrs/`](doc
 
 ## OpenShell API reference
 
-The gateway exposes 72 gRPC RPCs across 2 services (68 in `OpenShell`, 4 in `Inference`). Proto files are in `backend/proto/`.
+The gateway exposes RPCs across 2 services (`OpenShell`, `Inference`), consumed via the vendored `github.com/NVIDIA/OpenShell/sdk/go` (see `.claude/rules/openshell-api.md`) — not a local proto copy.
 
 ## Personas
 

backend/internal/api/drafts_handler.go

@@ -45,22 +45,38 @@ func (app *App) GetDraftPolicy(w http.ResponseWriter, r *http.Request) {
 	writeJSON(w, http.StatusOK, models.FromSDKDraftPolicy(draft))
 }
 
+// ApproveDraftChunkRequest optionally carries the chunk's review token, which
+// the frontend already has from its last GetDraftPolicy fetch. Absent for
+// older clients or the approve-from-notification path.
+type ApproveDraftChunkRequest struct {
+	ReviewToken string `json:"reviewToken,omitempty"`
+}
+
 // ApproveDraftChunk merges one proposed rule into the active policy.
 func (app *App) ApproveDraftChunk(w http.ResponseWriter, r *http.Request) {
 	workspace := chi.URLParam(r, "workspace")
 	name := chi.URLParam(r, "name")
 	chunkID := chi.URLParam(r, "chunk")
 
-	// The gateway binds each approval to a review_token that pins the exact
-	// evaluated candidate (optimistic concurrency). Resolve the current token
-	// for this chunk from the draft before approving; older gateways return an
-	// empty token, which the RPC accepts.
-	reviewToken, err := app.resolveDraftReviewToken(r.Context(), workspace, name, chunkID)
-	if err != nil {
-		writeSDKError(w, err)
+	var body ApproveDraftChunkRequest
+	if r.ContentLength > 0 && !decodeBody(w, r, &body) {
 		return
 	}
 
+	// The gateway binds each approval to a review_token that pins the exact
+	// evaluated candidate (optimistic concurrency). Use the token the client
+	// already has; only fall back to a GetDraft round-trip when it didn't send
+	// one. Older gateways return an empty token, which the RPC accepts.
+	reviewToken := body.ReviewToken
+	if reviewToken == "" {
+		var err error
+		reviewToken, err = app.resolveDraftReviewToken(r.Context(), workspace, name, chunkID)
+		if err != nil {
+			writeSDKError(w, err)
+			return
+		}
+	}
+
 	result, err := app.sdk.Policy().ApproveDraftChunk(r.Context(), workspace, name, chunkID, reviewToken)
 	if err != nil {
 		writeSDKError(w, err)

backend/internal/api/mock_sdk_test.go

@@ -46,6 +46,8 @@ type mockSDKSandboxes struct {
 	detachFn        func(ctx context.Context, workspace, sandboxName, providerName string, expectedResourceVersion uint64) (*openshell.DetachProviderResult, error)
 	listProvidersFn func(ctx context.Context, workspace, sandboxName string) ([]*openshell.Provider, error)
 	getLogsFn       func(ctx context.Context, workspace, name string, opts ...openshell.LogOption) (*openshell.LogResult, error)
+	stopFn          func(ctx context.Context, workspace, name string) (*openshell.Sandbox, error)
+	startFn         func(ctx context.Context, workspace, name string) (*openshell.Sandbox, error)
 }
 
 func (m *mockSDKSandboxes) List(ctx context.Context, workspace string, opts ...openshell.ListOptions) ([]*openshell.Sandbox, error) {
@@ -112,12 +114,18 @@ func (m *mockSDKSandboxes) GetLogs(ctx context.Context, workspace, name string,
 	return &openshell.LogResult{}, nil
 }
 
-func (m *mockSDKSandboxes) Start(_ context.Context, _, _ string) (*openshell.Sandbox, error) {
-	panic("not implemented")
+func (m *mockSDKSandboxes) Start(ctx context.Context, workspace, name string) (*openshell.Sandbox, error) {
+	if m.startFn != nil {
+		return m.startFn(ctx, workspace, name)
+	}
+	return &openshell.Sandbox{Name: name}, nil
 }
 
-func (m *mockSDKSandboxes) Stop(_ context.Context, _, _ string) (*openshell.Sandbox, error) {
-	panic("not implemented")
+func (m *mockSDKSandboxes) Stop(ctx context.Context, workspace, name string) (*openshell.Sandbox, error) {
+	if m.stopFn != nil {
+		return m.stopFn(ctx, workspace, name)
+	}
+	return &openshell.Sandbox{Name: name}, nil
 }
 
 func (m *mockSDKSandboxes) WaitStopped(_ context.Context, _, _ string, _ ...openshell.WaitOptions) (*openshell.Sandbox, error) {

backend/internal/api/policies_handler_test.go

@@ -210,15 +210,63 @@ func TestGetDraftPolicy(t *testing.T) {
 }
 
 func TestApproveDraftChunk(t *testing.T) {
-	app := newTestAppWithSDK(&mockSDK{})
-	r := chi.NewRouter()
-	r.Post("/workspaces/{workspace}/sandboxes/{name}/drafts/{chunk}/approve", app.ApproveDraftChunk)
-	req := httptest.NewRequest(http.MethodPost, "/workspaces/default/sandboxes/my-sandbox/drafts/c1/approve", nil)
-	w := httptest.NewRecorder()
-	r.ServeHTTP(w, req)
-	if w.Code != http.StatusOK {
-		t.Fatalf("status = %d", w.Code)
-	}
+	t.Run("uses client-supplied review token without resolving the draft", func(t *testing.T) {
+		sdk := &mockSDK{}
+		sdk.policy.getDraftFn = func(_ context.Context, _, _ string, _ ...openshell.GetDraftOption) (*openshell.DraftPolicy, error) {
+			t.Fatal("GetDraft should not be called when the client supplies a reviewToken")
+			return nil, nil
+		}
+		var gotToken string
+		sdk.policy.approveFn = func(_ context.Context, _, _, _, reviewToken string) (*openshell.ApproveResult, error) {
+			gotToken = reviewToken
+			return &openshell.ApproveResult{PolicyVersion: 2}, nil
+		}
+		app := newTestAppWithSDK(sdk)
+		r := chi.NewRouter()
+		r.Post("/workspaces/{workspace}/sandboxes/{name}/drafts/{chunk}/approve", app.ApproveDraftChunk)
+		req := httptest.NewRequest(
+			http.MethodPost,
+			"/workspaces/default/sandboxes/my-sandbox/drafts/c1/approve",
+			strings.NewReader(`{"reviewToken":"client-token"}`),
+		)
+		req.Header.Set("Content-Type", "application/json")
+		w := httptest.NewRecorder()
+		r.ServeHTTP(w, req)
+		if w.Code != http.StatusOK {
+			t.Fatalf("status = %d; body: %s", w.Code, w.Body.String())
+		}
+		if gotToken != "client-token" {
+			t.Errorf("reviewToken passed to ApproveDraftChunk = %q, want %q", gotToken, "client-token")
+		}
+	})
+
+	t.Run("resolves the review token from the draft when the client sends none", func(t *testing.T) {
+		sdk := &mockSDK{}
+		sdk.policy.getDraftFn = func(_ context.Context, _, _ string, _ ...openshell.GetDraftOption) (*openshell.DraftPolicy, error) {
+			return &openshell.DraftPolicy{
+				Chunks: []openshell.PolicyChunk{
+					{ID: "c1", ReviewToken: "resolved-token"},
+				},
+			}, nil
+		}
+		var gotToken string
+		sdk.policy.approveFn = func(_ context.Context, _, _, _, reviewToken string) (*openshell.ApproveResult, error) {
+			gotToken = reviewToken
+			return &openshell.ApproveResult{PolicyVersion: 2}, nil
+		}
+		app := newTestAppWithSDK(sdk)
+		r := chi.NewRouter()
+		r.Post("/workspaces/{workspace}/sandboxes/{name}/drafts/{chunk}/approve", app.ApproveDraftChunk)
+		req := httptest.NewRequest(http.MethodPost, "/workspaces/default/sandboxes/my-sandbox/drafts/c1/approve", nil)
+		w := httptest.NewRecorder()
+		r.ServeHTTP(w, req)
+		if w.Code != http.StatusOK {
+			t.Fatalf("status = %d; body: %s", w.Code, w.Body.String())
+		}
+		if gotToken != "resolved-token" {
+			t.Errorf("reviewToken passed to ApproveDraftChunk = %q, want %q", gotToken, "resolved-token")
+		}
+	})
 }
 
 func TestRejectDraftChunk(t *testing.T) {

backend/internal/api/sandboxes_handler_test.go

@@ -235,6 +235,93 @@ func TestGetSandbox(t *testing.T) {
 	}
 }
 
+func TestStopSandbox(t *testing.T) {
+	tests := []struct {
+		stopFn     func(ctx context.Context, workspace, name string) (*openshell.Sandbox, error)
+		name       string
+		wantStatus int
+	}{
+		{
+			name: "success",
+			stopFn: func(_ context.Context, _, name string) (*openshell.Sandbox, error) {
+				return &openshell.Sandbox{ID: "id-1", Name: name, Status: openshell.SandboxStatus{Phase: openshell.SandboxStopping}}, nil
+			},
+			wantStatus: http.StatusOK,
+		},
+		{
+			name: "not found",
+			stopFn: func(_ context.Context, _, _ string) (*openshell.Sandbox, error) {
+				return nil, &openshell.StatusError{Code: openshell.ErrorNotFound, Message: "sandbox not found"}
+			},
+			wantStatus: http.StatusNotFound,
+		},
+		{
+			name: "already stopped conflicts",
+			stopFn: func(_ context.Context, _, _ string) (*openshell.Sandbox, error) {
+				return nil, &openshell.StatusError{Code: openshell.ErrorConflict, Message: "sandbox is not running"}
+			},
+			wantStatus: http.StatusConflict,
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			sdk := &mockSDK{}
+			sdk.sandboxes.stopFn = tc.stopFn
+			app := newTestAppWithSDK(sdk)
+			r := chi.NewRouter()
+			r.Post("/workspaces/{workspace}/sandboxes/{name}/stop", app.StopSandbox)
+
+			req := httptest.NewRequest(http.MethodPost, "/workspaces/default/sandboxes/my-sandbox/stop", nil)
+			w := httptest.NewRecorder()
+			r.ServeHTTP(w, req)
+
+			if w.Code != tc.wantStatus {
+				t.Errorf("status = %d, want %d; body: %s", w.Code, tc.wantStatus, w.Body.String())
+			}
+		})
+	}
+}
+
+func TestStartSandbox(t *testing.T) {
+	tests := []struct {
+		startFn    func(ctx context.Context, workspace, name string) (*openshell.Sandbox, error)
+		name       string
+		wantStatus int
+	}{
+		{
+			name: "success",
+			startFn: func(_ context.Context, _, name string) (*openshell.Sandbox, error) {
+				return &openshell.Sandbox{ID: "id-1", Name: name, Status: openshell.SandboxStatus{Phase: openshell.SandboxStarting}}, nil
+			},
+			wantStatus: http.StatusOK,
+		},
+		{
+			name: "not found",
+			startFn: func(_ context.Context, _, _ string) (*openshell.Sandbox, error) {
+				return nil, &openshell.StatusError{Code: openshell.ErrorNotFound, Message: "sandbox not found"}
+			},
+			wantStatus: http.StatusNotFound,
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			sdk := &mockSDK{}
+			sdk.sandboxes.startFn = tc.startFn
+			app := newTestAppWithSDK(sdk)
+			r := chi.NewRouter()
+			r.Post("/workspaces/{workspace}/sandboxes/{name}/start", app.StartSandbox)
+
+			req := httptest.NewRequest(http.MethodPost, "/workspaces/default/sandboxes/my-sandbox/start", nil)
+			w := httptest.NewRecorder()
+			r.ServeHTTP(w, req)
+
+			if w.Code != tc.wantStatus {
+				t.Errorf("status = %d, want %d; body: %s", w.Code, tc.wantStatus, w.Body.String())
+			}
+		})
+	}
+}
+
 func TestDeleteSandbox(t *testing.T) {
 	tests := []struct {
 		deleteFn   func(ctx context.Context, workspace, name string) error

frontend/src/api/policy.ts

@@ -63,10 +63,11 @@ export const approveDraftChunk = (
   workspace: string,
   name: string,
   chunkId: string,
+  reviewToken?: string,
 ): Promise<PolicyUpdateResult> =>
   post<PolicyUpdateResult>(
     `${sandboxBase(workspace, name)}/drafts/${encodeURIComponent(chunkId)}/approve`,
-    {},
+    reviewToken ? { reviewToken } : {},
   );
 
 export const rejectDraftChunk = (
@@ -190,8 +191,11 @@ const useDraftMutation = <TArgs, TResult>(
 };
 
 export const useApproveDraftChunk = (workspace: string, name: string) =>
-  useDraftMutation(workspace, name, (chunkId: string) =>
-    approveDraftChunk(workspace, name, chunkId),
+  useDraftMutation(
+    workspace,
+    name,
+    (args: { chunkId: string; reviewToken?: string }) =>
+      approveDraftChunk(workspace, name, args.chunkId, args.reviewToken),
   );
 
 export const useRejectDraftChunk = (workspace: string, name: string) =>

frontend/src/components/SandboxDraftsTab/DraftChunkRow.tsx

@@ -114,7 +114,12 @@ const DraftChunkRow: React.FC<DraftChunkRowProps> = ({
                     <Button
                       variant="primary"
                       size="sm"
-                      onClick={() => approve.mutate(chunk.id)}
+                      onClick={() =>
+                        approve.mutate({
+                          chunkId: chunk.id,
+                          reviewToken: chunk.reviewToken,
+                        })
+                      }
                       isDisabled={approve.isPending}
                       data-testid={`approve-chunk-${chunk.id}`}
                     >

frontend/src/components/StatusDot.tsx

@@ -4,7 +4,10 @@ const STATUS_DOT_COLORS: Partial<Record<SandboxPhase, string>> = {
   READY: 'var(--pf-t--global--color--status--success--default)',
   ERROR: 'var(--pf-t--global--color--status--danger--default)',
   PROVISIONING: 'var(--pf-t--global--color--status--info--default)',
+  STARTING: 'var(--pf-t--global--color--status--info--default)',
+  STOPPING: 'var(--pf-t--global--color--status--info--default)',
   DELETING: 'var(--pf-t--global--color--status--warning--default)',
+  STOPPED: 'var(--pf-t--global--icon--color--disabled)',
 };
 
 export const getStatusDotColor = (phase: SandboxPhase): string =>

frontend/src/components/sandbox/SandboxTableRow.tsx

@@ -16,6 +16,7 @@ import LabelsList from '../LabelsList';
 import { getPolicySummary } from './SandboxEgressSummary';
 import StatusDot from '../StatusDot';
 import { formatAge } from '../../utils/formatters';
+import { canStartSandbox, canStopSandbox } from '../../utils/sandboxLifecycle';
 import type { Sandbox, SandboxPolicyView } from '../../types';
 
 type PolicySummary = ReturnType<typeof getPolicySummary>;
@@ -67,10 +68,10 @@ const SandboxTableRow: React.FC<SandboxTableRowProps> = ({
   const actionItems = [
     ...(onOpenTerminal ? [{ title: 'Terminal', onClick: onOpenTerminal }] : []),
     { title: 'Logs', onClick: onViewLogs },
-    ...(onStop && sandbox.status.phase === 'READY'
+    ...(onStop && canStopSandbox(sandbox.status.phase)
       ? [{ title: 'Stop', onClick: onStop }]
       : []),
-    ...(onStart && sandbox.status.phase === 'STOPPED'
+    ...(onStart && canStartSandbox(sandbox.status.phase)
       ? [{ title: 'Start', onClick: onStart }]
       : []),
     { title: 'Delete', onClick: onDelete },