Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -630,9 +630,9 @@ When adding code that needs a new string, decide up front which rule it falls un

- Worktree policy (this conversation): commits are made on the worktree branch and the user expects the worktree to stay attached to that branch. Do NOT run `git switch --detach` in the worktree and reattach the branch to the main workspace after each commit — only do that when the user explicitly asks. See `~/.openhands/skills/worktree-switch/SKILL.md` for the manual procedure the user invokes.

- Files tab diff-view default logic: keyed off `useHasAttachedSource()` (`src/hooks/use-has-attached-source.ts`), which is true when the user explicitly attached _either_ a repo (`conversation.selected_repository`) _or_ a local workspace (`getStoredConversationMetadata(id).selected_workspace`, persisted by `createConversation` when `workingDirOverride` is supplied). The agent-server pre-initialises every conversation workspace as a git worktree for its own change tracking, so do NOT use a filesystem probe (`git status` / `useUnifiedGetGitChanges`) as the attachment signal — that was tried in earlier iterations and made every fresh no-attachment conversation incorrectly default to diff view. The companion `useHasGitCommits` probe (`src/hooks/query/use-has-git-commits.ts`) then suppresses diff view for attached-but-empty cases (unborn HEAD, non-git workspace).
- Files tab diff-view default logic: keyed off `useHasAttachedSource()` (`src/hooks/use-has-attached-source.ts`), which is true when the user explicitly attached _either_ a repo (`conversation.selected_repository`) _or_ a local workspace (`conversation.selected_workspace`). Both fields are populated by `toAppConversation` from the agent-server's per-conversation `tags` map; `createConversation` writes the `selected_workspace` tag when `workingDirOverride` is supplied. The agent-server pre-initialises every conversation workspace as a git worktree for its own change tracking, so do NOT use a filesystem probe (`git status` / `useUnifiedGetGitChanges`) as the attachment signal — that was tried in earlier iterations and made every fresh no-attachment conversation incorrectly default to diff view. The companion `useHasGitCommits` probe (`src/hooks/query/use-has-git-commits.ts`) then suppresses diff view for attached-but-empty cases (unborn HEAD, non-git workspace).

- Files tab diff-view default logic: keyed off `useHasAttachedSource()` (`src/hooks/use-has-attached-source.ts`), which is true when the user explicitly attached _either_ a repo (`conversation.selected_repository`) _or_ a local workspace (`getStoredConversationMetadata(id).selected_workspace`, persisted by `createConversation` when `workingDirOverride` is supplied). The agent-server pre-initialises every conversation workspace as a git worktree for its own change tracking, so do NOT use a filesystem probe (`git status` / `useUnifiedGetGitChanges`) as the attachment signal — that was tried in earlier iterations and made every fresh no-attachment conversation incorrectly default to diff view. The companion `useHasGitCommits` probe (`src/hooks/query/use-has-git-commits.ts`) then suppresses diff view for attached-but-empty cases (unborn HEAD, non-git workspace).
- Per-conversation metadata (`selected_repository`, `selected_branch`, `git_provider`, `selected_workspace`, `active_profile`) now lives in agent-server `tags` instead of the legacy `openhands-agent-server-conversation-metadata` localStorage blob. The five tag keys are declared in `AGENT_CANVAS_METADATA_TAG_KEYS` (`src/api/agent-server-adapter.ts`); the agent-server validator constrains tag keys to `^[a-z0-9_]+$` (SDK PR #3621 relaxed the previous `^[a-z0-9]+$` to allow underscores). Writers: `createConversation` stamps the full set on `POST /api/conversations` via `mergeMetadataIntoTags`; `updateConversationRepository` and `updateConversationActiveProfile` PATCH single fields and preserve unrelated tags (e.g. `acpserver`). Readers: `toAppConversation` reads server tags first and falls back to the legacy localStorage entry for un-migrated conversations. The legacy store (`src/api/conversation-metadata-store.ts`) is `@deprecated` and only kept alive by the fallback + a one-shot lazy migration (`src/api/conversation-metadata-migration.ts`) wired into `usePaginatedConversations`'s `queryFn`: for each listed conversation it PATCHes legacy values onto missing server tags, then clears the localStorage entry. Once one upgrade cycle has passed, the store, the fallback branches, and the migration helper can all be deleted.

- Collapsible thinking: `ThinkAction` events and LLM extended reasoning (`reasoning_content` / `thinking_blocks` on `ActionEvent`) are rendered as collapsible sections via `CollapsibleThinking` (`src/components/conversation-events/chat/event-message-components/collapsible-thinking.tsx`). Collapsed by default to keep the chat compact — the thinking is often in English regardless of the user's conversation language. The `getReasoningContent()` helper in `event-thought-helpers.ts` extracts the content, preferring `reasoning_content` (plain string) and falling back to Anthropic `thinking_blocks`. i18n keys: `THINKING$TITLE`, `THINKING$EXPAND`, `THINKING$COLLAPSE`. Tests: `__tests__/components/conversation-events/chat/event-message-think-action.test.tsx`.

Expand Down
155 changes: 155 additions & 0 deletions __tests__/api/conversation-metadata-migration.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
import { ConversationClient } from "@openhands/typescript-client/clients";
import { beforeEach, describe, expect, it, vi } from "vitest";
import { migrateLegacyConversationMetadata } from "#/api/conversation-metadata-migration";
import {
getStoredConversationMetadata,
setStoredConversationMetadata,
} from "#/api/conversation-metadata-store";
import type { AppConversation } from "#/api/conversation-service/agent-server-conversation-service.types";

const updateConversation = vi.fn();

vi.mock("@openhands/typescript-client/clients", async () => {
const actual = await vi.importActual<
typeof import("@openhands/typescript-client/clients")
>("@openhands/typescript-client/clients");
return {
...actual,
ConversationClient: vi.fn(function ConversationClientMock() {
return { updateConversation };
}),
};
});

vi.mock("#/api/agent-server-client-options", () => ({
getAgentServerClientOptions: vi.fn(() => ({
host: "http://localhost:18000",
apiKey: "test-key",
workingDir: "workspace/project",
})),
}));

vi.mock("#/api/backend-registry/active-store", () => ({
getActiveBackend: vi.fn(() => ({
backend: { kind: "local", id: "default-local" },
})),
}));

const makeConversation = (
id: string,
tags: Record<string, string> | null = null,
): AppConversation =>
({
id,
title: "x",
createdAt: "2024-01-01T00:00:00Z",
updatedAt: "2024-01-01T00:00:00Z",
selected_repository: null,
selected_branch: null,
git_provider: null,
selected_workspace: null,
active_profile: null,
tags,
}) as unknown as AppConversation;

describe("migrateLegacyConversationMetadata", () => {
beforeEach(() => {
vi.clearAllMocks();
window.localStorage.clear();
updateConversation.mockResolvedValue(undefined);
vi.mocked(ConversationClient).mockClear();
});

it("PATCHes legacy localStorage entries onto server tags and clears the entry", async () => {
setStoredConversationMetadata("conv-1", {
selected_repository: "octocat/hello-world",
selected_branch: "main",
git_provider: "github",
selected_workspace: null,
active_profile: "team-default",
});

// Server has no agent-canvas tags yet — pure legacy state.
await migrateLegacyConversationMetadata([makeConversation("conv-1", null)]);

expect(updateConversation).toHaveBeenCalledTimes(1);
expect(updateConversation).toHaveBeenCalledWith("conv-1", {
tags: {
selected_repository: "octocat/hello-world",
selected_branch: "main",
git_provider: "github",
active_profile: "team-default",
},
});
expect(getStoredConversationMetadata("conv-1")).toBeNull();
});

it("skips fields the server already has and clears the localStorage entry without PATCHing when fully mirrored", async () => {
setStoredConversationMetadata("conv-2", {
selected_repository: "octocat/hello-world",
selected_branch: "main",
git_provider: "github",
});

// The server already mirrors every field — nothing to migrate, just
// drop the stale legacy entry.
await migrateLegacyConversationMetadata([
makeConversation("conv-2", {
selected_repository: "octocat/hello-world",
selected_branch: "main",
git_provider: "github",
}),
]);

expect(updateConversation).not.toHaveBeenCalled();
expect(getStoredConversationMetadata("conv-2")).toBeNull();
});

it("preserves unrelated server tags (e.g. acpserver) when merging in legacy values", async () => {
setStoredConversationMetadata("conv-3", {
selected_repository: "octocat/hello-world",
selected_branch: null,
git_provider: "github",
});

await migrateLegacyConversationMetadata([
makeConversation("conv-3", { acpserver: "claude-code" }),
]);

expect(updateConversation).toHaveBeenCalledTimes(1);
const [, body] = updateConversation.mock.calls[0];
expect(body.tags).toEqual({
acpserver: "claude-code",
selected_repository: "octocat/hello-world",
git_provider: "github",
});
});

it("leaves the legacy entry in place if the PATCH fails so the next refresh retries", async () => {
setStoredConversationMetadata("conv-4", {
selected_repository: "octocat/hello-world",
selected_branch: null,
git_provider: "github",
});
updateConversation.mockRejectedValueOnce(new Error("boom"));

await migrateLegacyConversationMetadata([makeConversation("conv-4", null)]);

expect(getStoredConversationMetadata("conv-4")).not.toBeNull();
});

it("is a no-op when no conversations are passed", async () => {
setStoredConversationMetadata("conv-5", {
selected_repository: "octocat/hello-world",
selected_branch: null,
git_provider: "github",
});

await migrateLegacyConversationMetadata([]);

expect(updateConversation).not.toHaveBeenCalled();
// Untouched — the entry will be migrated next time the list query
// returns conv-5.
expect(getStoredConversationMetadata("conv-5")).not.toBeNull();
});
});
50 changes: 31 additions & 19 deletions __tests__/api/use-create-conversation-metadata.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,25 @@ import { renderHook, waitFor } from "@testing-library/react";
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
import React from "react";
import { useCreateConversation } from "#/hooks/mutation/use-create-conversation";
import { getStoredConversationMetadata } from "#/api/conversation-metadata-store";

/**
* The metadata that USED to live in `conversation-metadata-store` localStorage
* (and was tested against that store here) now rides on the
* `POST /api/conversations` payload's `tags` field. These tests assert that
* wire shape directly — the localStorage shim is deprecated, and the
* agent-server is the canonical source. See `agent-server-adapter.ts`
* (AGENT_CANVAS_METADATA_TAG_KEYS) and SDK PR #3621.
*/
const lastCreatePayload = (): Record<string, unknown> => {
const calls = mockHttpPost.mock.calls;
expect(calls.length).toBeGreaterThan(0);
return calls[calls.length - 1][1] as Record<string, unknown>;
};

const tagsFromLastCreate = (): Record<string, string> | undefined => {
const payload = lastCreatePayload();
return payload.tags as Record<string, string> | undefined;
};

const {
mockHttpPost,
Expand Down Expand Up @@ -123,7 +141,7 @@ describe("useCreateConversation persists selected repository metadata", () => {
window.localStorage.clear();
});

it("stores the selected repo/branch/provider in the metadata store after a successful create", async () => {
it("sends selected repo/branch/provider tags on POST /api/conversations", async () => {
const { result } = renderHook(() => useCreateConversation(), { wrapper });

result.current.mutate({
Expand All @@ -137,15 +155,14 @@ describe("useCreateConversation persists selected repository metadata", () => {

await waitFor(() => expect(result.current.isSuccess).toBe(true));

expect(getStoredConversationMetadata("conv-new")).toEqual({
expect(tagsFromLastCreate()).toEqual({
selected_repository: "octocat/hello-world",
selected_branch: "main",
git_provider: "github",
selected_workspace: null,
});
});

it("stores the selected workspace path when only a workspace (no repo) is attached", async () => {
it("sends the selected_workspace tag when only a workspace (no repo) is attached", async () => {
const { result } = renderHook(() => useCreateConversation(), { wrapper });

result.current.mutate({
Expand All @@ -155,27 +172,26 @@ describe("useCreateConversation persists selected repository metadata", () => {

await waitFor(() => expect(result.current.isSuccess).toBe(true));

// We persist the workspace path so `useHasAttachedSource` can default
// the Files tab to diff view even when no repo was picked.
expect(getStoredConversationMetadata("conv-new")).toEqual({
selected_repository: null,
selected_branch: null,
git_provider: null,
// `useHasAttachedSource` reads `selected_workspace` from server tags
// to default the Files tab to diff view even when no repo was picked.
expect(tagsFromLastCreate()).toEqual({
selected_workspace: "/home/me/code/some-project",
});
});

it("does not write metadata when neither a repository nor a workspace is attached", async () => {
it("omits the tags field entirely when neither a repository nor a workspace is attached", async () => {
const { result } = renderHook(() => useCreateConversation(), { wrapper });

result.current.mutate({ query: "scratch session" });

await waitFor(() => expect(result.current.isSuccess).toBe(true));

expect(getStoredConversationMetadata("conv-new")).toBeNull();
// No metadata + no ACP server → buildStartConversationRequest leaves
// `tags` off the wire entirely (same shape as the original behaviour).
expect(lastCreatePayload().tags).toBeUndefined();
});

it("stamps the active LLM profile even when no repo or workspace is attached (#1082)", async () => {
it("stamps the active_profile tag even when no repo or workspace is attached (#1082)", async () => {
mockUseLlmProfiles.mockReturnValue({
data: { active_profile: "team-default" },
});
Expand All @@ -186,11 +202,7 @@ describe("useCreateConversation persists selected repository metadata", () => {

await waitFor(() => expect(result.current.isSuccess).toBe(true));

expect(getStoredConversationMetadata("conv-new")).toEqual({
selected_repository: null,
selected_branch: null,
git_provider: null,
selected_workspace: null,
expect(tagsFromLastCreate()).toEqual({
active_profile: "team-default",
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ describe("NewConversationButton (cloud)", () => {
selected_repository: "octo/dog",
selected_branch: "trunk",
git_provider: "github",
active_profile: null,
},
undefined,
undefined,
Expand Down Expand Up @@ -197,6 +198,7 @@ describe("NewConversationButton (cloud)", () => {
selected_repository: "octo/no-main",
selected_branch: "main",
git_provider: "github",
active_profile: null,
},
undefined,
undefined,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,7 @@ describe("HomeChatLauncher", () => {
selected_repository: "org/repo",
selected_branch: "main",
git_provider: "github",
active_profile: null,
},
undefined,
undefined,
Expand Down
1 change: 1 addition & 0 deletions __tests__/components/features/home/task-card.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ describe("TaskCard", () => {
selected_repository: MOCK_TASK_1.repo,
selected_branch: null,
git_provider: MOCK_TASK_1.git_provider,
active_profile: null,
},
undefined,
undefined,
Expand Down
34 changes: 27 additions & 7 deletions __tests__/contexts/conversation-websocket-context.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,25 @@ import { useOptimisticUserMessageStore } from "#/stores/optimistic-user-message-
import { useBrowserStore } from "#/stores/browser-store";
import { useUserConversation } from "#/hooks/query/use-user-conversation";
import EventService from "#/api/event-service/event-service.api";
import { getStoredConversationMetadata } from "#/api/conversation-metadata-store";
import type { MessageEvent } from "#/types/agent-server/core";

// The websocket context now PATCHes the `active_profile` server tag through
// the conversation service when a model switch arrives over the socket.
// Mock the static method so this test asserts the call shape without
// touching the agent-server client. The mock object must be created via
// `vi.hoisted` because `vi.mock` is hoisted above plain top-level decls.
const { updateActiveProfileMock } = vi.hoisted(() => ({
updateActiveProfileMock: vi.fn().mockResolvedValue(undefined),
}));
vi.mock(
"#/api/conversation-service/agent-server-conversation-service.api",
() => ({
default: {
updateConversationActiveProfile: updateActiveProfileMock,
},
}),
);

// Captures the main socket's `onMessage` (`handleMainMessage`) so tests can
// drive the live message path without a real WebSocket. Only the main socket
// gets a non-empty url (planning stays ""), so url presence discriminates it.
Expand Down Expand Up @@ -123,7 +139,7 @@ describe("ConversationWebSocketProvider — conversation-scoped event store", ()
},
});

it("stamps active_profile on a successful agent-triggered model switch so it survives reload", async () => {
it("PATCHes active_profile on a successful agent-triggered model switch so it survives reload", async () => {
// Arrange: open a conversation with a real ws url so the main socket's
// onMessage (handleMainMessage) is wired and captured.
render(
Expand All @@ -145,11 +161,15 @@ describe("ConversationWebSocketProvider — conversation-scoped event store", ()
});
});

// Assert: the profile identity is persisted to stored metadata — the same
// field the chat-header switcher reads after a reload (#1082). Without the
// stamp this stays null and the header falls back to ambiguous matching.
expect(getStoredConversationMetadata("conv-switch")?.active_profile).toBe(
"fast-opus",
// Assert: the profile identity is PATCHed onto the conversation's
// `active_profile` server tag — the same value the chat-header
// switcher reads after a reload (#1082). Without this the header
// falls back to ambiguous matching on `llm.model`.
await waitFor(() =>
expect(updateActiveProfileMock).toHaveBeenCalledWith(
"conv-switch",
"fast-opus",
),
);
});

Expand Down
8 changes: 6 additions & 2 deletions __tests__/hooks/mutation/use-create-conversation.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -82,15 +82,19 @@ describe("useCreateConversation", () => {
});

await waitFor(() => {
// The metadata object now also carries `active_profile` (null when
// no profile is active) — see `useCreateConversation` and the
// deprecation of `conversation-metadata-store`. The mocked
// `useLlmProfiles` in this test returns no data, so the field is null.
expect(createConversationSpy).toHaveBeenCalledWith(
"Please address the comments",
"Focus on review comments",
undefined,
{
expect.objectContaining({
selected_repository: "owner/repo",
selected_branch: "main",
git_provider: "github",
},
}),
undefined,
undefined,
undefined,
Expand Down
Loading
Loading