From 60bc93f0e944584d7da27863278969380743827d Mon Sep 17 00:00:00 2001 From: Rohit Malhotra Date: Thu, 21 May 2026 18:08:53 -0400 Subject: [PATCH] Add backend management specs and @spec annotations (#727) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add backend management specs and @spec annotations - Curate specs/backend-management.md with 4 behavioral specs (BM-001–BM-004) - Add @spec comments to source and test files for traceability - Add spec file convention note to AGENTS.md Co-authored-by: openhands * Remove duplicate BM-002 test (non-conversation redirect) The 'stays on settings' case was covered by two tests; keep the one with the clearer assertion and drop the redundant copy. Co-authored-by: openhands * Parameterize BM-002 tests with it.each Collapse 3 identical-structure redirect tests into a single parameterized it.each covering conversation detail, automation detail, and non-ID routes. Co-authored-by: openhands * Document @spec tagging convention in AGENTS.md Co-authored-by: openhands --------- Co-authored-by: openhands --- AGENTS.md | 2 + .../backends/backend-selector.test.tsx | 170 ++++-------------- .../contexts/active-backend-context.test.tsx | 2 + specs/backend-management.md | 10 +- src/api/backend-registry/active-store.ts | 1 + .../features/backends/backend-selector.tsx | 1 + 6 files changed, 54 insertions(+), 132 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index bf01707b4a..bede18a9b5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -512,4 +512,6 @@ When adding code that needs a new string, decide up front which rule it falls un - To bump a version, edit `config/defaults.json` only — the JS scripts, Docker build, and CI workflow all derive their values from it. - Docker all-in-one image: `.github/workflows/docker.yml` builds and publishes `ghcr.io/openhands/agent-canvas` — a combined image that bundles the agent-server (from `ghcr.io/openhands/agent-server`), the automation server (`openhands-automation` via pip), and the agent-canvas frontend (static build). The Dockerfile lives at `docker/Dockerfile`, the entrypoint at `docker/entrypoint.sh`. The workflow structure mirrors the SDK repo's `server.yml`: a `build-and-push-image` matrix job (2 × arch: amd64 on `ubuntu-24.04`, arm64 on `ubuntu-24.04-arm`) pushes arch-suffixed tags, then `merge-manifests` creates multi-arch manifests via `docker buildx imagetools create`, then `consolidate-build-info` aggregates artifacts, and `update-pr-description` updates the PR body (using `` / `` markers). The workflow triggers on push to main, `v*` tags (releases), PRs, and `workflow_dispatch`. On release tags it also pushes semver tags (e.g. `1.2.3`, `1.2`, `1`, `latest`). Fork PRs are skipped (no GHCR auth). The image exposes port 8000 as a unified entry point: `/api/automation/*` → automation (:18001), `/api/*` → agent-server (:18000), `/*` → static frontend. The Dockerfile accepts a `VITE_APP_ENV` build arg (default empty → staging PostHog key); the CI workflow passes `VITE_APP_ENV=production` only for tagged releases (`refs/tags/v*`), so PR and main-branch images use the staging key while release images use the production key, matching the `build:lib` npm path. The entrypoint auto-generates **both** the session API key and `OH_SECRET_KEY` (persisted to `~/.openhands/agent-canvas/session-api-key.txt` and `secret-key.txt` respectively) when none is provided, so the image runs secure by default. Users can override either via env var (`OH_SECRET_KEY`, `SESSION_API_KEY` / `OH_SESSION_API_KEYS_0`). Unlike `scripts/dev-safe.mjs` (which uses a static default secret key for local dev convenience), the Docker entrypoint never falls back to a known default. +- Spec files live under `specs/`. Spec IDs are stable — never renumber. Mark deprecated specs with ~~strikethrough~~. Tag implementation code and tests with `// @spec BM-002 — Short title` comments so specs are grep-able across the codebase (`grep -rn '@spec BM-' src/ __tests__/`). Place the comment on the line immediately above the relevant code block or test. When multiple tests cover the same spec, use `it.each` if the test structure is identical. + - Cloud conversation resume gating: when a cloud conversation is closed from the UI (`pauseCloudSandbox` is called), the conversation's `conversation_url` is NOT cleared -- it still points to the old sandbox host. `WebSocketProviderWrapper` must suppress the URL (pass `null` to `ConversationWebSocketProvider`) while `sandbox_status === "PAUSED"`, otherwise the WebSocket immediately tries the stale URL before the sandbox wakes. Symmetrically, `useActiveConversation`'s refetch interval must fast-poll (3 s) on both `!conversation_url` AND `sandbox_status === "PAUSED"` -- checking only the missing URL would leave the hook on the 30 s interval while the sandbox is resuming. The resume sequence: navigate -> sandbox PAUSED detected -> `resumeCloudSandbox` called (in `conversation.tsx`) -> fast-poll detects RUNNING -> `conversationUrl` unblocked -> WebSocket connects. diff --git a/__tests__/components/backends/backend-selector.test.tsx b/__tests__/components/backends/backend-selector.test.tsx index a455af97bf..b53085e0ce 100644 --- a/__tests__/components/backends/backend-selector.test.tsx +++ b/__tests__/components/backends/backend-selector.test.tsx @@ -385,24 +385,48 @@ describe("BackendSelector", () => { expect(input.value).toBe("Local"); }); - it("redirects to home when switching backends from a conversation route", async () => { - function ConversationRoute() { + // @spec BM-002 — Switching backends keeps the user on the same page + it.each([ + { + name: "conversation detail → conversations list", + startPath: "/conversations/abc", + startRoute: "/conversations/:conversationId", + landingRoute: "/conversations", + expectRedirect: true, + }, + { + name: "automation detail → automations list", + startPath: "/automations/abc-123", + startRoute: "/automations/:automationId", + landingRoute: "/automations", + expectRedirect: true, + }, + { + name: "settings → stays on settings", + startPath: "/settings", + startRoute: "/settings", + landingRoute: "/conversations", + expectRedirect: false, + }, + ])("$name", async ({ startPath, startRoute, landingRoute, expectRedirect }) => { + function StartRoute() { return ( { ctx.addBackend(SEED_LOCAL_1); }} > +
); } - function HomeRoute() { - return
; + function LandingRoute() { + return
; } const RouterStub = createRoutesStub([ - { path: "/conversations/:conversationId", Component: ConversationRoute }, - { path: "/conversations", Component: HomeRoute }, + { path: startRoute, Component: StartRoute }, + { path: landingRoute, Component: LandingRoute }, ]); const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } }, @@ -410,59 +434,22 @@ describe("BackendSelector", () => { render( - + , ); - // Auto-switch lands on "Local 1"; click the seeded default to switch. const user = await openDropdown(); await user.click(screen.getByText("Local")); - expect(await screen.findByTestId("home")).toBeInTheDocument(); - }); - - it("stays on the current page when switching backends from a non-conversation route", async () => { - function SettingsRoute() { - return ( - { - ctx.addBackend(SEED_LOCAL_1); - }} - > -
- - - ); + if (expectRedirect) { + expect(await screen.findByTestId("landing-route")).toBeInTheDocument(); + } else { + await waitFor(() => { + expect(screen.getByTestId("start-route")).toBeInTheDocument(); + }); + expect(screen.queryByTestId("landing-route")).not.toBeInTheDocument(); } - function HomeRoute() { - return
; - } - const RouterStub = createRoutesStub([ - { path: "/settings", Component: SettingsRoute }, - { path: "/conversations", Component: HomeRoute }, - ]); - const queryClient = new QueryClient({ - defaultOptions: { queries: { retry: false } }, - }); - render( - - - - - , - ); - - // Auto-switch lands on "Local 1"; click the seeded default to switch. - const user = await openDropdown(); - await user.click(screen.getByText("Local")); - - // The settings route is still in the DOM after the switch — no - // redirect to /conversations or anywhere else. - await waitFor(() => { - expect(screen.getByTestId("settings-route")).toBeInTheDocument(); - }); - expect(screen.queryByTestId("home")).not.toBeInTheDocument(); }); it("keeps the environment-switch overlay visible even after the selector unmounts mid-switch", async () => { @@ -587,6 +574,7 @@ describe("BackendSelector", () => { expect(remaining.map((b: { name: string }) => b.name)).toEqual(["Local"]); }); + // @spec BM-003 — Fallback on active backend removal it("falls back to the seeded default backend when removing the active backend from manage backends", async () => { // Pre-seed the registry and active selection in localStorage so the // initial render already reflects `Local 1` as active. Seeding via @@ -644,86 +632,6 @@ describe("BackendSelector", () => { expect(input.value).toBe("Local"); }); - it("redirects to the automations list when switching backends from an automation detail route", async () => { - function AutomationDetailRoute() { - return ( - { - ctx.addBackend(SEED_LOCAL_1); - }} - > - - - ); - } - function AutomationsListRoute() { - return
; - } - const RouterStub = createRoutesStub([ - { path: "/automations/:automationId", Component: AutomationDetailRoute }, - { path: "/automations", Component: AutomationsListRoute }, - ]); - const queryClient = new QueryClient({ - defaultOptions: { queries: { retry: false } }, - }); - render( - - - - - , - ); - - // Auto-switch lands on "Local 1"; click the seeded default to switch. - const user = await openDropdown(); - await user.click(screen.getByText("Local")); - - expect(await screen.findByTestId("automations-list")).toBeInTheDocument(); - }); - - it("does not redirect when switching backends from a non-conversation route", async () => { - function SettingsRoute() { - return ( - { - ctx.addBackend(SEED_LOCAL_1); - }} - > - - - ); - } - function HomeRoute() { - return
; - } - const RouterStub = createRoutesStub([ - { path: "/settings", Component: SettingsRoute }, - { path: "/", Component: HomeRoute }, - ]); - const queryClient = new QueryClient({ - defaultOptions: { queries: { retry: false } }, - }); - render( - - - - - , - ); - - const settingsButton = screen.getByTestId("backend-selector-settings-link"); - expect(settingsButton).toHaveAttribute("data-active", "true"); - - // Auto-switch lands on "Local 1"; click the seeded default to switch. - const user = await openDropdown(); - await user.click(screen.getByText("Local")); - - const wrapper = screen.getByTestId("backend-selector"); - const input = wrapper.querySelector("input") as HTMLInputElement; - expect(input.value).toBe("Local"); - expect(screen.queryByTestId("home")).not.toBeInTheDocument(); - }); - describe("connection indicator", () => { it("renders one status dot per option, green when the probe succeeds", async () => { vi.mocked(ServerClient).mockImplementation(function ServerClientMock() { diff --git a/__tests__/contexts/active-backend-context.test.tsx b/__tests__/contexts/active-backend-context.test.tsx index 2b93e5232d..1be8d33e6a 100644 --- a/__tests__/contexts/active-backend-context.test.tsx +++ b/__tests__/contexts/active-backend-context.test.tsx @@ -131,6 +131,7 @@ describe("ActiveBackendProvider", () => { expect(queryClient.getQueryData(["dummy"])).toEqual({ value: 1 }); }); + // @spec BM-003 — Fallback on active backend removal it("removeBackend falls back to the seeded default when the active backend is removed", () => { const { result } = renderHook(() => useActiveBackendContext(), { wrapper: makeWrapper(), @@ -159,6 +160,7 @@ describe("ActiveBackendProvider", () => { expect(result.current.backends[0].id).toBe(DEFAULT_LOCAL_BACKEND_ID); }); + // @spec BM-003 — Fallback on active backend removal it("removeBackend allows removing the seeded default and falls back to a synthesized env-derived backend", () => { const { result } = renderHook(() => useActiveBackendContext(), { wrapper: makeWrapper(), diff --git a/specs/backend-management.md b/specs/backend-management.md index 98e41b079b..53df60a0bb 100644 --- a/specs/backend-management.md +++ b/specs/backend-management.md @@ -1,4 +1,12 @@ # Backend Management Specs -## BM-001: Auto-switch on connect +--- + +### BM-001: Auto-switch on connect - [x] Adding a backend shall automatically switch the active selection to it. + +### BM-002: Switching backends keeps the user on the same page +- [x] Switching backends shall redirect to the same section but on the new backend. The user shall never see stale data from the previous backend. + +### BM-003: Fallback on active backend removal +- [x] Removing the currently active backend shall fall back to a remaining local backend. The user shall never be left without an active backend. \ No newline at end of file diff --git a/src/api/backend-registry/active-store.ts b/src/api/backend-registry/active-store.ts index d5b9d544da..78d0c5999b 100644 --- a/src/api/backend-registry/active-store.ts +++ b/src/api/backend-registry/active-store.ts @@ -46,6 +46,7 @@ function computeSnapshot( // makes sense in the context of a specific cloud backend. } + // @spec BM-003 — Fallback on active backend removal if (!activeBackend) { activeBackend = pickLocalBackend(backends); activeOrgId = null; diff --git a/src/components/features/backends/backend-selector.tsx b/src/components/features/backends/backend-selector.tsx index 0318624377..6d97a0e413 100644 --- a/src/components/features/backends/backend-selector.tsx +++ b/src/components/features/backends/backend-selector.tsx @@ -279,6 +279,7 @@ export function BackendSelector({ setTimeout(resolve, ENVIRONMENT_SWITCH_SETACTIVE_DELAY_MS); }); + // @spec BM-002 — Switching backends keeps the user on the same page if (conversationMatch) navigate("/conversations"); else if (automationDetailMatch) navigate("/automations");