diff --git a/AGENTS.md b/AGENTS.md index d64107b47..d56aad495 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -179,14 +179,16 @@ you are running inside of — NOT the automation backend. - **State isolation**: `OH_CANVAS_SAFE_STATE_DIR=.tmp/mock-llm-state` isolates test state from the user's real `~/.openhands/agent-canvas/` directory. Both `STATE_DIR` (`.tmp/mock-llm-state`) and the automation DB dir (`.tmp/automation/`) are cleaned before each test run — the automation DB now lives outside STATE_DIR at `dirname(STATE_DIR)/automation/automations.db`, mirroring Docker's `~/.openhands/automation/automations.db`. - **Session API key**: A random key is generated per test run and passed to the stack via `SESSION_API_KEY` / `OH_SESSION_API_KEYS_0` / `VITE_SESSION_API_KEY`. The static server injects it into `index.html` at serve time so the frontend authenticates automatically. - **Mock LLM server** (`tests/e2e/mock-llm/scripts/mock-llm-server.py`): Python HTTP server using openhands-sdk's `TestLLM` to return scripted tool-call + text trajectories. Supports admin API endpoints for dynamic trajectory management: - - `POST /admin/reset` — reset to the default trajectory (terminal printf + text reply) + - `POST /admin/reset` — reset to the default trajectory (terminal printf + text reply); also clears the stored completion-request history - `POST /admin/trajectory/register` — register a named trajectory (JSON body: `{name, turns}` where each turn is `{tool_call: {name, arguments}}` or `{text: "..."}`) - `POST /admin/trajectory/activate` — activate a previously registered trajectory + - `GET /admin/requests` — return the list of all `/v1/chat/completions` request bodies captured since the last reset (used by the image-upload test to verify the image was forwarded to the LLM) - **Real automation backend**: The automation test uses the production automation backend (started by `bin/agent-canvas.mjs`), NOT a mock server. Terminal `curl` commands from the agent hit the automation API through the ingress proxy at the test's `BACKEND_URL` (default `http://localhost:18300`). Auth uses the `X-Session-API-Key` header matching the stack's session key. The `mock-automation-server.py` file still exists as a reference but is not used by current tests. -- **Test helpers** (`tests/e2e/mock-llm/utils/mock-llm-helpers.ts`): Exports `registerTrajectory()`, `activateTrajectory()`, `resetMockLLM()`, `ensureMockLLMProfile()`, etc. +- **Test helpers** (`tests/e2e/mock-llm/utils/mock-llm-helpers.ts`): Exports `registerTrajectory()`, `activateTrajectory()`, `resetMockLLM()`, `ensureMockLLMProfile()`, `getMockLLMRequests()` (fetches captured completion bodies from `GET /admin/requests`), `IMAGE_REPLY_TOKEN` + `MINIMAL_PNG_BASE64` (constants for the image-upload spec), and more. - **Padding response for internal LLM call**: The agent-server makes an internal LLM call (condenser/skill-analysis) before the agent's main loop starts when skills are activated. This consumes one trajectory response. Automation tests prepend a throwaway `{ text: "" }` response as padding. The conversation test does NOT need this because its user message doesn't trigger skill activation. - **Test specs**: - `mock-llm-conversation.spec.ts` — Creates LLM profile via UI, runs a conversation with a terminal tool call, verifies bash execution and agent reply. + - `mock-llm-image-upload.spec.ts` — Attaches a 1×1 PNG via the hidden file input, sends "What is in this image?", verifies the agent replies, that the user message event stores image_urls, and that the mock LLM received an image_url content block with a base64 data: URL in at least one completion call. - `mock-llm-automation.spec.ts` — Full automation lifecycle: registers a trajectory (7 responses total — 4 for the main conversation + 3 for the automation run's spawned conversation) where the LLM creates a cron automation and dispatches a run via terminal `curl` commands to the real automation backend. Verifies: automation created with correct schedule, run reaches COMPLETED status with a conversation_id, automation appears on the `/automations` list page, detail page shows COMPLETED badge (`data-testid="run-status-icon-completed"`), and clicking the run's conversation link navigates to the correct `/conversations/{id}` page. - `mock-llm-partial-stack.spec.ts` — Partial stack mode tests. Unlike other specs, these spawn their own `bin/agent-canvas.mjs` child processes instead of relying on the config's webServer entries. Three describe blocks: (1) `--frontend-only` verifies static frontend is served (200 on `/`), backend routes return 503 (`/server_info`, `/api/settings`, `/api/automation/v1`), and the browser shows the manage-backends modal; (2) `--backend-only` verifies `/server_info` returns 200, `/api/settings` is reachable, automation endpoint works, and root/asset requests return 503; (3) port conflict verifies the process exits non-zero with a clear error message when the ingress port is occupied, then starts successfully on a free port. Each test uses isolated state dirs and high port numbers (18310+ range) to avoid collisions with the main full-stack instance. - Tests run serially (`workers: 1`, `mode: "serial"` per describe block). Files are discovered alphabetically so automation tests run before conversation tests; each spec is self-contained (automation test configures its own LLM profile via the settings API). The `afterEach` hook resets the mock LLM to its default trajectory so subsequent specs start fresh even when a preceding test fails. diff --git a/__tests__/api/agent-server-conversation-service.test.ts b/__tests__/api/agent-server-conversation-service.test.ts index 854da8636..6f72989b9 100644 --- a/__tests__/api/agent-server-conversation-service.test.ts +++ b/__tests__/api/agent-server-conversation-service.test.ts @@ -146,6 +146,9 @@ describe("AgentServerConversationService", () => { ); return response.data; }, + // @spec WUP-001 — createConversation resolves relative working dirs + // via FileClient.getHome before sending the conversation-start payload. + getHome: async () => ({ home: "/Users/agent" }), }); mockSettingsClient.mockReturnValue({ listSecrets: vi.fn().mockResolvedValue({ secrets: [] }), @@ -284,6 +287,85 @@ describe("AgentServerConversationService", () => { `/state/workspaces/${secondHex}`, ); }); + + // @spec WUP-001 — When the default working_dir is relative, the + // conversation-start payload must be anchored against the agent-server + // home dir so the worktree and later file uploads agree on a writable + // absolute path. + it("resolves relative default working dirs against /api/file/home", async () => { + const { buildConversationWorkingDir: mockedBuilder } = + await import("#/api/agent-server-config"); + vi.mocked(mockedBuilder).mockImplementationOnce( + (id: string) => `workspace/project/${id.replace(/-/g, "")}`, + ); + const { clearAgentServerHomeDirCache } = + await import("#/api/agent-server-home"); + clearAgentServerHomeDirCache(); + + mockGetSettings.mockResolvedValue({ + agent_settings: { llm: { model: "gpt-4o" } }, + conversation_settings: {}, + }); + mockGetSettingsForConversation.mockResolvedValue({ + agentSettings: { llm: { model: "gpt-4o" } }, + conversationSettings: {}, + secretsEncrypted: true, + }); + mockHttpPost.mockResolvedValue({ + data: { + id: "ignored-server-id", + created_at: "2024-01-01", + updated_at: "2024-01-01", + }, + }); + + await AgentServerConversationService.createConversation(); + + const [payloadCall] = mockHttpPost.mock.calls; + const payload = payloadCall[1] as { + conversation_id: string; + workspace: { working_dir: string }; + }; + const hex = payload.conversation_id.replace(/-/g, ""); + expect(payload.workspace.working_dir).toBe( + `/Users/agent/workspace/project/${hex}`, + ); + }); + + // @spec WUP-001 — User-supplied workspace overrides are already absolute + // (they come from `search_subdirs`), so they must pass through verbatim. + it("leaves an absolute workingDirOverride untouched", async () => { + mockGetSettings.mockResolvedValue({ + agent_settings: { llm: { model: "gpt-4o" } }, + conversation_settings: {}, + }); + mockGetSettingsForConversation.mockResolvedValue({ + agentSettings: { llm: { model: "gpt-4o" } }, + conversationSettings: {}, + secretsEncrypted: true, + }); + mockHttpPost.mockResolvedValue({ + data: { + id: "ignored-server-id", + created_at: "2024-01-01", + updated_at: "2024-01-01", + }, + }); + + await AgentServerConversationService.createConversation( + undefined, + undefined, + undefined, + undefined, + "/Users/jane/projects/foo", + ); + + const [payloadCall] = mockHttpPost.mock.calls; + const payload = payloadCall[1] as { + workspace: { working_dir: string }; + }; + expect(payload.workspace.working_dir).toBe("/Users/jane/projects/foo"); + }); }); describe("downloadConversation local branch", () => { diff --git a/__tests__/api/conversation-file-upload.test.ts b/__tests__/api/conversation-file-upload.test.ts index 5ff8d0817..be83cd27c 100644 --- a/__tests__/api/conversation-file-upload.test.ts +++ b/__tests__/api/conversation-file-upload.test.ts @@ -7,8 +7,10 @@ import { } from "#/api/backend-registry/active-store"; import type { Backend } from "#/api/backend-registry/types"; import { uploadFilesToConversation } from "#/api/conversation-file-upload.api"; +import { clearAgentServerHomeDirCache } from "#/api/agent-server-home"; const fileUploadMock = vi.fn(); +const getHomeMock = vi.fn(); vi.mock("@openhands/typescript-client/workspace/remote-workspace", () => ({ RemoteWorkspace: vi.fn(function RemoteWorkspaceMock() { @@ -16,6 +18,12 @@ vi.mock("@openhands/typescript-client/workspace/remote-workspace", () => ({ }), })); +vi.mock("@openhands/typescript-client/clients", () => ({ + FileClient: vi.fn(function FileClientMock() { + return { getHome: getHomeMock }; + }), +})); + const batchGetCloudConversations = vi.fn(); vi.mock("#/api/cloud/conversation-service.api", () => ({ @@ -40,11 +48,16 @@ describe("uploadFilesToConversation", () => { vi.clearAllMocks(); window.localStorage.clear(); __resetActiveStoreForTests(); + clearAgentServerHomeDirCache(); fileUploadMock.mockResolvedValue(undefined); batchGetCloudConversations.mockReset(); + getHomeMock.mockReset(); + getHomeMock.mockResolvedValue({ home: "/Users/test" }); }); - it("uploads local conversations through the bundled agent-server host", async () => { + // @spec WUP-001 — Default-fallback relative working dirs are resolved + // against /api/file/home, not the filesystem root. + it("uploads local conversations under the agent-server home dir when working dir is relative", async () => { setRegisteredBackends([ { id: "local-1", @@ -62,10 +75,42 @@ describe("uploadFilesToConversation", () => { expect(fileUploadMock).toHaveBeenCalledWith( expect.objectContaining({ name: "a.txt" }), - "/workspace/project/a.txt", + "/Users/test/workspace/project/a.txt", ); expect(result.uploaded_files).toEqual(["a.txt"]); expect(batchGetCloudConversations).not.toHaveBeenCalled(); + expect(getHomeMock).toHaveBeenCalled(); + }); + + // @spec WUP-001 — Absolute working dirs (e.g. the conversation's own + // `workspace.working_dir`) pass through without a /api/file/home round-trip. + it("respects an absolute conversation working_dir verbatim", async () => { + setRegisteredBackends([ + { + id: "local-1", + name: "Local", + host: "http://127.0.0.1:18000", + apiKey: "local-key", + kind: "local", + }, + ]); + setActiveSelection({ backendId: "local-1" }); + + const result = await uploadFilesToConversation( + "conv-1", + [makeFile("notes.md")], + { + id: "conv-1", + workspace: { working_dir: "/Users/test/projects/foo" }, + } as never, + ); + + expect(fileUploadMock).toHaveBeenCalledWith( + expect.objectContaining({ name: "notes.md" }), + "/Users/test/projects/foo/notes.md", + ); + expect(result.uploaded_files).toEqual(["notes.md"]); + expect(getHomeMock).not.toHaveBeenCalled(); }); it("uploads cloud conversations against the provisioned runtime URL", async () => { diff --git a/__tests__/api/conversation-service.test.ts b/__tests__/api/conversation-service.test.ts index 246325538..30e99a155 100644 --- a/__tests__/api/conversation-service.test.ts +++ b/__tests__/api/conversation-service.test.ts @@ -2,8 +2,10 @@ import { describe, expect, it, vi, beforeEach } from "vitest"; import { RemoteWorkspace } from "@openhands/typescript-client/workspace/remote-workspace"; import ConversationService from "#/api/conversation-service/conversation-service.api"; +import { clearAgentServerHomeDirCache } from "#/api/agent-server-home"; const fileUploadMock = vi.fn(); +const getHomeMock = vi.fn(); vi.mock("@openhands/typescript-client/workspace/remote-workspace", () => ({ RemoteWorkspace: vi.fn(function RemoteWorkspaceMock() { @@ -11,6 +13,12 @@ vi.mock("@openhands/typescript-client/workspace/remote-workspace", () => ({ }), })); +vi.mock("@openhands/typescript-client/clients", () => ({ + FileClient: vi.fn(function FileClientMock() { + return { getHome: getHomeMock }; + }), +})); + function makeFile(name: string) { return new File(["content"], name, { type: "text/plain" }); } @@ -19,9 +27,14 @@ describe("ConversationService", () => { beforeEach(() => { vi.clearAllMocks(); ConversationService.setCurrentConversation(null); + clearAgentServerHomeDirCache(); + getHomeMock.mockResolvedValue({ home: "/Users/agent" }); }); describe("uploadFiles", () => { + // @spec WUP-001 — The default fallback working dir is relative + // (`workspace/project`); the upload path is resolved against the + // agent-server's home directory via /api/file/home. it("uploads files through RemoteWorkspace and reports successes", async () => { fileUploadMock.mockResolvedValue(undefined); @@ -38,11 +51,11 @@ describe("ConversationService", () => { ); expect(fileUploadMock).toHaveBeenCalledWith( expect.objectContaining({ name: "a.txt" }), - "/workspace/project/a.txt", + "/Users/agent/workspace/project/a.txt", ); expect(fileUploadMock).toHaveBeenCalledWith( expect.objectContaining({ name: "b.txt" }), - "/workspace/project/b.txt", + "/Users/agent/workspace/project/b.txt", ); expect(result).toEqual({ uploaded_files: ["a.txt", "b.txt"], @@ -59,7 +72,7 @@ describe("ConversationService", () => { expect(fileUploadMock).toHaveBeenCalledWith( expect.objectContaining({ name: "../../evil.txt" }), - "/workspace/project/evil.txt", + "/Users/agent/workspace/project/evil.txt", ); expect(result).toEqual({ uploaded_files: ["evil.txt"], diff --git a/__tests__/api/workspace-upload-path.test.ts b/__tests__/api/workspace-upload-path.test.ts index 004af6556..76cebd2d6 100644 --- a/__tests__/api/workspace-upload-path.test.ts +++ b/__tests__/api/workspace-upload-path.test.ts @@ -1,34 +1,131 @@ -import { describe, expect, it, vi } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; import { buildWorkspaceUploadPath, getSafeUploadFileName, + resolveAbsoluteWorkspacePath, resolveConversationUploadWorkingDir, toAbsoluteWorkspacePath, } from "#/api/workspace-upload-path"; +import { clearAgentServerHomeDirCache } from "#/api/agent-server-home"; -vi.mock("#/api/conversation-service/agent-server-conversation-service.api", () => ({ - default: { - resolveConversationWorkingDir: vi.fn( - async (id: string) => `/workspace/project/${id.replace(/-/g, "")}`, - ), - }, +const mockGetHome = vi.fn(); + +vi.mock("@openhands/typescript-client/clients", () => ({ + FileClient: vi.fn(function FileClientMock() { + return { getHome: mockGetHome }; + }), +})); + +vi.mock("#/api/agent-server-client-options", () => ({ + getAgentServerClientOptions: vi.fn(() => ({ + host: "http://localhost:8000", + apiKey: "test-key", + workingDir: "workspace/project", + })), })); +vi.mock( + "#/api/conversation-service/agent-server-conversation-service.api", + () => ({ + default: { + resolveConversationWorkingDir: vi.fn( + async (id: string) => `/workspace/project/${id.replace(/-/g, "")}`, + ), + }, + }), +); + +beforeEach(() => { + clearAgentServerHomeDirCache(); + mockGetHome.mockReset(); + mockGetHome.mockResolvedValue({ home: "/Users/test" }); +}); + describe("workspace-upload-path", () => { - it("normalizes relative working dirs to absolute paths", () => { + // @spec WUP-001 — legacy helper kept for cosmetic uses only. + it("toAbsoluteWorkspacePath is a naive `/`-prefix and stays sync", () => { expect(toAbsoluteWorkspacePath("workspace/project")).toBe( "/workspace/project", ); - expect(buildWorkspaceUploadPath("a.txt", "workspace/project")).toBe( - "/workspace/project/a.txt", + expect(toAbsoluteWorkspacePath("/already/absolute")).toBe( + "/already/absolute", ); }); + // @spec WUP-001 — resolver anchors relative paths against /api/file/home. + it("resolveAbsoluteWorkspacePath joins relative dirs to the agent-server home", async () => { + const resolved = await resolveAbsoluteWorkspacePath("workspace/project"); + expect(resolved).toBe("/Users/test/workspace/project"); + expect(mockGetHome).toHaveBeenCalledTimes(1); + }); + + // @spec WUP-001 — absolute inputs pass through, no /file/home round-trip. + it("resolveAbsoluteWorkspacePath leaves absolute paths alone", async () => { + const resolved = await resolveAbsoluteWorkspacePath( + "/workspace/project/custom", + ); + expect(resolved).toBe("/workspace/project/custom"); + expect(mockGetHome).not.toHaveBeenCalled(); + }); + + // @spec WUP-001 — Windows-style absolute paths are also pass-through. + it("resolveAbsoluteWorkspacePath treats Windows drive-letter paths as absolute", async () => { + const resolved = await resolveAbsoluteWorkspacePath("C:\\foo\\bar"); + expect(resolved).toBe("C:\\foo\\bar"); + expect(mockGetHome).not.toHaveBeenCalled(); + }); + + // @spec WUP-001 — the home dir is cached so concurrent uploads share one round-trip. + it("caches the home directory across calls", async () => { + await resolveAbsoluteWorkspacePath("workspace/project"); + await resolveAbsoluteWorkspacePath("other/relative"); + expect(mockGetHome).toHaveBeenCalledTimes(1); + }); + + // @spec WUP-001 — failures are not cached so a later call retries fresh. + it("does not cache failed lookups", async () => { + mockGetHome.mockRejectedValueOnce(new Error("boom")); + await expect( + resolveAbsoluteWorkspacePath("workspace/project"), + ).rejects.toThrow("boom"); + + mockGetHome.mockResolvedValueOnce({ home: "/Users/test" }); + const resolved = await resolveAbsoluteWorkspacePath("workspace/project"); + expect(resolved).toBe("/Users/test/workspace/project"); + expect(mockGetHome).toHaveBeenCalledTimes(2); + }); + + // @spec WUP-001 — empty or whitespace working dirs collapse to the home dir. + it("resolves an empty working dir to the home directory itself", async () => { + expect(await resolveAbsoluteWorkspacePath("")).toBe("/Users/test"); + expect(await resolveAbsoluteWorkspacePath("/")).toBe("/Users/test"); + }); + + // @spec WUP-001 — the safe-name helper still strips traversal segments. it("strips path segments from file names", () => { expect(getSafeUploadFileName("../../evil.txt")).toBe("evil.txt"); - expect(buildWorkspaceUploadPath("../../evil.txt", "/workspace/project")).toBe( - "/workspace/project/evil.txt", + }); + + // @spec WUP-001 — buildWorkspaceUploadPath uses the resolver and a safe leaf. + it("builds an absolute upload path from a relative working dir", async () => { + const upload = await buildWorkspaceUploadPath("a.txt", "workspace/project"); + expect(upload).toBe("/Users/test/workspace/project/a.txt"); + }); + + it("rejects file names that escape the destination via path traversal", async () => { + const upload = await buildWorkspaceUploadPath( + "../../evil.txt", + "/workspace/project", ); + expect(upload).toBe("/workspace/project/evil.txt"); + }); + + it("collapses trailing slashes on the working dir", async () => { + const upload = await buildWorkspaceUploadPath( + "doc.md", + "/workspace/project/", + ); + expect(upload).toBe("/workspace/project/doc.md"); }); it("prefers the active conversation workspace when ids match", async () => { @@ -46,8 +143,6 @@ describe("workspace-upload-path", () => { null, ); - expect(dir).toBe( - "/workspace/project/550e8400e29b41d4a716446655440000", - ); + expect(dir).toBe("/workspace/project/550e8400e29b41d4a716446655440000"); }); }); diff --git a/specs/workspace-upload-path.md b/specs/workspace-upload-path.md new file mode 100644 index 000000000..37b46d762 --- /dev/null +++ b/specs/workspace-upload-path.md @@ -0,0 +1,16 @@ +# Workspace Upload Path Specs + +--- + +### WUP-001: Relative working dirs are resolved against `/api/file/home`, not the filesystem root +- [x] When the frontend creates a conversation and the resolved working dir is **relative** (e.g. the `DEFAULT_WORKING_DIR = "workspace/project"` fallback), `AgentServerConversationService.createConversation` shall resolve it to an **absolute** path before sending `workspace.working_dir` to the agent-server, by prefixing the agent-server's home directory as returned by `GET /api/file/home`. +- [x] When the frontend uploads a file, `buildWorkspaceUploadPath` shall resolve the conversation's working dir through the same home-directory anchor, so the upload destination always matches the conversation's worktree location. +- [x] When the working dir is already absolute (e.g. POSIX `/foo`, Windows `C:\foo`, or the explicit selection from `search_subdirs`), the resolver shall pass it through unchanged. +- [x] The home-directory lookup shall be cached per backend host so concurrent uploads share a single in-flight `/api/file/home` request, and a cached value is reused for subsequent uploads. +- [x] A failed lookup shall not be cached so the next call retries fresh. +- [x] The legacy `toAbsoluteWorkspacePath` helper that naively prepends `/` shall remain only for cosmetic/log use; it shall not be used to construct upload paths. + +### Why this exists +- The agent-server's `/api/file/upload` endpoint requires an absolute path and `mkdir -p`s the parent of the destination. Naively prepending `/` to the default `workspace/project/` produces `/workspace/project/`. On macOS and on fresh Docker images that mount only `/home/` as writable, the filesystem root is read-only, so the upload fails with `OSError: [Errno 30] Read-only file system: '/workspace'`. +- The agent-server otherwise interprets a relative `workspace.working_dir` against its process CWD (which is whichever directory the launcher used), so absent this resolver, the conversation's worktree lands in one place and the upload tries to land in a totally different place. +- `/api/file/home` is the most reliable absolute, writable anchor the agent-server API currently exposes; `/server_info` does not include the CWD. diff --git a/src/api/agent-server-home.ts b/src/api/agent-server-home.ts new file mode 100644 index 000000000..5372cdc4e --- /dev/null +++ b/src/api/agent-server-home.ts @@ -0,0 +1,114 @@ +// @spec WUP-001 — Resolve relative working dirs against /api/file/home +import { FileClient } from "@openhands/typescript-client/clients"; +import { + getAgentServerClientOptions, + type AgentServerClientOverrides, +} from "./agent-server-client-options"; + +/** + * Cache the agent-server's home directory per host so we only round-trip + * `/api/file/home` once per backend. The home dir is effectively static for + * the lifetime of a running agent-server (it's `Path.home()` on the host), + * so caching is safe and avoids hammering the endpoint on every upload. + * + * The cache holds `Promise` rather than `string` so concurrent + * callers share a single in-flight request. + */ +const homeDirCache = new Map>(); + +function isAbsolutePath(path: string): boolean { + // Treat POSIX-style and Windows-style absolute paths as absolute. + // The agent-server itself is POSIX on Linux/macOS and uses `\\` style + // on Windows; we just need to know whether `Path(path).is_absolute()` + // would return true on the server. + // + // Patterns covered: + // POSIX absolute: /foo/bar + // Windows drive: C:\foo or C:/foo + // Windows UNC: \\server\share (starts with `\`, matches `[/\\]`) + return /^([/\\]|[a-zA-Z]:[/\\])/.test(path); +} + +/** + * Join a parent directory and a relative child segment with a forward slash, + * collapsing any duplicate separators that result from the join. + */ +function joinPath(parent: string, child: string): string { + const left = parent.replace(/[/\\]+$/, ""); + const right = child.replace(/^[/\\]+/, ""); + return `${left}/${right}`; +} + +/** + * Fetch and cache the agent-server's home directory via `GET /api/file/home`. + * + * The result is the absolute path returned by `Path.home()` on the + * agent-server host (e.g. `/Users/foo`, `/root`, or `C:\\Users\\Foo`). This + * is the most reliable absolute, writable anchor the agent-server API + * currently exposes — `/server_info` doesn't include the process CWD. + * + * @param overrides Same shape as `getAgentServerClientOptions` — lets cloud + * sandboxes pass a `conversationUrl` + `sessionApiKey` so the lookup goes + * to the per-conversation runtime rather than the bundled local backend. + */ +export async function getAgentServerHomeDir( + overrides: AgentServerClientOverrides = {}, +): Promise { + const options = getAgentServerClientOptions(overrides); + const cacheKey = options.host; + const cached = homeDirCache.get(cacheKey); + if (cached) return cached; + + const lookup = (async () => { + const { home } = await new FileClient(options).getHome(); + if (!home || typeof home !== "string") { + throw new Error("Agent server returned an empty home directory"); + } + return home.replace(/[/\\]+$/, ""); + })(); + + homeDirCache.set(cacheKey, lookup); + try { + return await lookup; + } catch (error) { + // Don't cache failures — let the next call retry. + homeDirCache.delete(cacheKey); + throw error; + } +} + +/** Test-only helper. */ +export function clearAgentServerHomeDirCache(): void { + homeDirCache.clear(); +} + +/** + * Resolve a (possibly relative) working dir to an absolute path the + * agent-server's file APIs will accept. + * + * - If `workingDir` is already absolute, returns it unchanged. + * - Otherwise prepends the agent-server's home dir (looked up via + * `/api/file/home` and cached). This matches how the published binary + * and Docker entrypoint expect to anchor relative working dirs: under + * `~/workspace/project` rather than the filesystem root. + * + * Why this matters: the agent-server's `/api/file/upload` endpoint requires + * an absolute path and `mkdir -p`s the parent. Naively prepending `/` to a + * relative dir like `workspace/project/` produces `/workspace/...`, + * which on macOS lives under the SIP-protected read-only root and fails + * with `Errno 30: Read-only file system: '/workspace'`. Resolving against + * `Path.home()` instead puts the path somewhere reliably writable. + */ +export async function resolveAbsoluteAgentServerPath( + workingDir: string, + overrides: AgentServerClientOverrides = {}, +): Promise { + const trimmed = workingDir.replace(/[/\\]+$/, ""); + if (!trimmed) { + return getAgentServerHomeDir(overrides); + } + if (isAbsolutePath(trimmed)) return trimmed; + + const home = await getAgentServerHomeDir(overrides); + return joinPath(home, trimmed); +} diff --git a/src/api/conversation-file-upload.api.ts b/src/api/conversation-file-upload.api.ts index b83f0ab15..d07e54db9 100644 --- a/src/api/conversation-file-upload.api.ts +++ b/src/api/conversation-file-upload.api.ts @@ -132,7 +132,16 @@ async function uploadFilesToRuntime(options: { const uploadFile = async (file: File) => { try { const safeName = getSafeUploadFileName(file.name); - const uploadPath = buildWorkspaceUploadPath(file.name, workingDir); + // @spec WUP-001 — Build an absolute upload path that's anchored against + // the agent-server's home dir (when `workingDir` is relative) instead + // of the filesystem root. Without this, default conversations whose + // working_dir is `workspace/project/` (relative) land at + // `/workspace/project//...` on the agent-server, which on macOS + // and fresh containers is a read-only mount. + const uploadPath = await buildWorkspaceUploadPath(file.name, workingDir, { + conversationUrl, + sessionApiKey, + }); await workspace.fileUpload(file, uploadPath); return { uploadedFile: safeName, skippedFile: null }; } catch (error) { diff --git a/src/api/conversation-service/agent-server-conversation-service.api.ts b/src/api/conversation-service/agent-server-conversation-service.api.ts index 09b217986..bb2f57b1c 100644 --- a/src/api/conversation-service/agent-server-conversation-service.api.ts +++ b/src/api/conversation-service/agent-server-conversation-service.api.ts @@ -16,6 +16,7 @@ import { buildConversationWorkingDir, getAgentServerWorkingDir, } from "../agent-server-config"; +import { resolveAbsoluteAgentServerPath } from "../agent-server-home"; import { getActiveBackend, getEffectiveLocalBackend, @@ -375,8 +376,15 @@ class AgentServerConversationService { const settings = await SettingsService.getSettings(); const conversationId = uuidv4(); - const workingDir = - workingDirOverride ?? buildConversationWorkingDir(conversationId); + // @spec WUP-001 — Send an absolute working_dir to the agent-server. + // The default is `workspace/project/` (relative); without + // resolving it here, `/api/file/upload` later prepends `/` and writes + // to `/workspace/...` (read-only on macOS and fresh containers). When + // the user picks an explicit workspace, `workingDirOverride` is + // already absolute (it comes from `search_subdirs`). + const workingDir = await resolveAbsoluteAgentServerPath( + workingDirOverride ?? buildConversationWorkingDir(conversationId), + ); // Use encrypted settings to avoid exposing secrets in the browser const payload = await buildStartConversationRequestWithEncryptedSettings({ diff --git a/src/api/workspace-upload-path.ts b/src/api/workspace-upload-path.ts index a8026890c..4548b734d 100644 --- a/src/api/workspace-upload-path.ts +++ b/src/api/workspace-upload-path.ts @@ -1,5 +1,11 @@ +// @spec WUP-001 — Resolve relative working dirs against /api/file/home import AgentServerConversationService from "#/api/conversation-service/agent-server-conversation-service.api"; import { getAgentServerWorkingDir } from "#/api/agent-server-config"; +import { + type AgentServerClientOverrides, + getAgentServerClientOptions, +} from "#/api/agent-server-client-options"; +import { resolveAbsoluteAgentServerPath } from "#/api/agent-server-home"; import { getStoredConversationMetadata } from "#/api/conversation-metadata-store"; import type { AppConversation } from "#/api/conversation-service/agent-server-conversation-service.types"; @@ -17,20 +23,66 @@ export function getSafeUploadFileName(fileName: string): string { return safeName; } -/** Normalize agent-server working_dir values to absolute sandbox paths. */ +/** + * @deprecated The old `prepend `/` if not already absolute` behaviour is the + * root cause of #XXXX (relative `workspace/project` → `/workspace/project`, + * which is on a read-only mount on macOS / fresh containers). Callers that + * need an actual absolute path must use {@link resolveAbsoluteWorkspacePath} + * so the relative leg is anchored against `/api/file/home` instead. + * + * Kept for the rare callers that genuinely just want a leading-slash + * normaliser on a value already known to be agent-server-rooted (e.g. + * cosmetic display, log strings). + */ export function toAbsoluteWorkspacePath(path: string): string { return path.startsWith("/") ? path : `/${path}`; } -export function buildWorkspaceUploadPath( +/** + * Resolve `workingDir` to an absolute path the agent-server's file APIs + * accept. Relative paths are joined against `/api/file/home` (cached per + * backend); absolute paths pass through. + */ +export async function resolveAbsoluteWorkspacePath( + workingDir: string, + overrides: AgentServerClientOverrides = {}, +): Promise { + return resolveAbsoluteAgentServerPath(workingDir, overrides); +} + +/** + * Build the absolute destination path for a file upload, resolving the + * working-dir leg via {@link resolveAbsoluteWorkspacePath}. + */ +export async function buildWorkspaceUploadPath( fileName: string, workingDir: string, -): string { + overrides: AgentServerClientOverrides = {}, +): Promise { const safeName = getSafeUploadFileName(fileName); - const base = toAbsoluteWorkspacePath(workingDir.replace(/\/+$/, "")); - return `${base}/${safeName}`; + const absoluteDir = await resolveAbsoluteWorkspacePath(workingDir, overrides); + return `${absoluteDir.replace(/[/\\]+$/, "")}/${safeName}`; } +/** + * Resolve the working directory for a file upload into a conversation's + * workspace. + * + * **Returns the raw working dir string** — which may be relative (e.g. + * `workspace/project/`) when the default `DEFAULT_WORKING_DIR` is in + * use. Callers that need an actual filesystem-absolute path (e.g. to pass to + * the agent-server's `/api/file/upload` endpoint) **must** funnel this result + * through {@link buildWorkspaceUploadPath}, which calls + * {@link resolveAbsoluteWorkspacePath} to anchor any relative segment against + * the agent-server's home directory. + * + * Why not resolve here? Because this function is also called by cloud-runtime + * upload paths where the overrides (conversationUrl, sessionApiKey) aren't + * available until {@link uploadFilesToConversation} assembles them. Keeping + * the resolution step in {@link buildWorkspaceUploadPath} means both the + * local and cloud legs share a single resolution point with the correct + * override context. + */ export async function resolveConversationUploadWorkingDir( conversationId: string, currentConversation?: AppConversation | null, @@ -55,3 +107,8 @@ export async function resolveConversationUploadWorkingDir( return getAgentServerWorkingDir(); } + +// Re-export so callers can construct overrides matching what +// {@link buildWorkspaceUploadPath} expects without importing two modules. +export type { AgentServerClientOverrides }; +export { getAgentServerClientOptions }; diff --git a/src/hooks/mutation/use-conversation-upload-files.ts b/src/hooks/mutation/use-conversation-upload-files.ts index 4e6cf669d..59df9d062 100644 --- a/src/hooks/mutation/use-conversation-upload-files.ts +++ b/src/hooks/mutation/use-conversation-upload-files.ts @@ -29,9 +29,25 @@ export const useConversationUploadFiles = () => const { conversationUrl, sessionApiKey, workingDir, files } = variables; const uploadPromises = files.map(async (file) => { + // @spec WUP-001 — Resolve once so both the success and failure + // branches report the same absolute path. + let filePath: string; + try { + filePath = await buildWorkspaceUploadPath(file.name, workingDir, { + conversationUrl, + sessionApiKey, + }); + } catch (error) { + return { + success: false as const, + fileName: file.name, + filePath: file.name, + error: error instanceof Error ? error.message : "Unknown error", + }; + } + try { const safeName = getSafeUploadFileName(file.name); - const filePath = buildWorkspaceUploadPath(file.name, workingDir); await new RemoteWorkspace( getAgentServerClientOptions({ conversationUrl, @@ -44,7 +60,7 @@ export const useConversationUploadFiles = () => return { success: false as const, fileName: file.name, - filePath: buildWorkspaceUploadPath(file.name, workingDir), + filePath, error: error instanceof Error ? error.message : "Unknown error", }; } diff --git a/tests/e2e/mock-llm/mock-llm-image-upload.spec.ts b/tests/e2e/mock-llm/mock-llm-image-upload.spec.ts new file mode 100644 index 000000000..a4b71f568 --- /dev/null +++ b/tests/e2e/mock-llm/mock-llm-image-upload.spec.ts @@ -0,0 +1,279 @@ +/** + * Mock-LLM E2E: image attachment is embedded as base64 and forwarded to LLM. + * + * Covers the fix from PR #1106: relative working-dir paths no longer resolve + * to a read-only root, so file uploads (and image attachments processed via + * the same pipeline) land in the correct writable location. + * + * Flow: + * 1. Configure mock LLM profile via the settings API (skips UI setup). + * 2. Register a scripted trajectory: one text reply with IMAGE_REPLY_TOKEN. + * 3. Attach a minimal 1×1 PNG to the home-page chat input via the hidden + * file input (`data-testid="upload-image-input"`). + * 4. Wait for the submit button to become enabled (image processed into + * the Zustand store → canSubmit flips to true). + * 5. Type "What is in this image?" and submit. + * 6. After the conversation is created and the agent replies: + * a. Verify IMAGE_REPLY_TOKEN appears in the chat UI. + * b. Verify the user's conversation event has image_urls set. + * c. Verify at least one LLM completion call to the mock server + * included an image_url content block (base64 data: URL). + */ + +import { test, expect } from "@playwright/test"; +import { + IMAGE_REPLY_TOKEN, + MINIMAL_PNG_BASE64, + waitForAgentMessageContaining, + waitForNonUserMessageText, + getMockLLMRequests, + BACKEND_URL, + SESSION_API_KEY, + seedLocalStorage, + routeSessionApiKey, + dismissAnalyticsModal, + waitForTestId, + waitForPath, + getConversationIdFromURL, + deleteConversation, + resetMockLLM, + registerTrajectory, + activateTrajectory, + ensureMockLLMProfile, + setChatInput, +} from "./utils/mock-llm-helpers"; + +// ── Constants ──────────────────────────────────────────────────────────────── + +const USER_MESSAGE = "What is in this image?"; +const TRAJECTORY_NAME = "image-query"; + +// ── Test suite ──────────────────────────────────────────────────────────────── + +test.describe.configure({ mode: "serial" }); + +test.describe("mock-LLM image upload", () => { + let conversationId: string | null = null; + + test.beforeEach(async ({ page }) => { + await seedLocalStorage(page); + }); + + test.afterEach(async ({ request }) => { + if (conversationId) { + try { + await deleteConversation(request, conversationId); + } catch { + // best-effort cleanup + } + conversationId = null; + } + await resetMockLLM(request); + }); + + // ── Main test ────────────────────────────────────────────────────────────── + + test("attaching an image embeds it as base64 in the LLM completion call", async ({ + page, + request, + }) => { + // ── 1. Configure mock LLM via API (avoids repeating the UI profile steps) ── + // Use a vision-capable model name so litellm does not strip image_url + // content blocks when constructing the completion request. The base_url + // still points at the local mock server; the model name is purely a hint + // to litellm about what content types the model accepts. + + await ensureMockLLMProfile(request, "openai/gpt-4o"); + + // ── 2. Register and activate the trajectory ── + // The mock LLM ignores the request body, so we don't need the agent to + // "understand" the image — we just want a reply that proves the LLM was + // called and the conversation completed successfully. + // + // ⚠️ Padding note (mirrors the automation test's pattern): + // When public skills are loaded (VITE_LOAD_PUBLIC_SKILLS !== "false"), + // the agent-server may make one internal LLM call for skill-analysis + // before the agent loop starts, consuming one trajectory slot. + // Turn 0 is a throwaway empty response that absorbs this internal call. + // Turn 1 is the agent's actual reply (IMAGE_REPLY_TOKEN). + // Turn 2 is a safety buffer in case a follow-up internal call is made. + + await resetMockLLM(request); // clears request history too + await registerTrajectory(request, TRAJECTORY_NAME, [ + { text: "" }, // 0: padding — absorbs any internal skill-activation call + { text: IMAGE_REPLY_TOKEN }, // 1: agent's actual reply + { text: "" }, // 2: safety buffer for any follow-up internal call + ]); + await activateTrajectory(request, TRAJECTORY_NAME); + + // ── 3. Navigate to the home page ── + + await routeSessionApiKey(page); + await page.goto("/", { waitUntil: "domcontentloaded" }); + await dismissAnalyticsModal(page); + await waitForTestId(page, "home-chat-launcher"); + + // ── 4. Attach the test image via the hidden file input ── + // Playwright's setInputFiles works on hidden inputs without needing a + // visible click target. The onChange handler (handleFileInputChange → + // handleUpload → processImages → addImages) runs asynchronously in + // React, so we wait for the submit button to become enabled afterwards. + + await page.locator('[data-testid="upload-image-input"]').setInputFiles({ + name: "test-image.png", + mimeType: "image/png", + buffer: Buffer.from(MINIMAL_PNG_BASE64, "base64"), + }); + + // Wait for the image to be processed (FileReader async + Zustand store + // update). Once images.length > 0 the submit button stops being disabled. + await expect(page.getByTestId("submit-button")).not.toBeDisabled({ + timeout: 10_000, + }); + + // ── 5. Type the user message and submit ── + // setChatInput sets innerText directly so the chat doesn't need visible + // typing; the InputEvent dispatch keeps canSubmit in sync. + + await setChatInput(page, USER_MESSAGE); + + // Confirm submit button is still enabled after text is added + await expect(page.getByTestId("submit-button")).not.toBeDisabled({ + timeout: 5_000, + }); + + await page.getByTestId("submit-button").click(); + + // ── 6a. Wait for the conversation page ── + + await waitForPath(page, /\/conversations\/.+/, 30_000); + conversationId = getConversationIdFromURL(page); + + // ── 6b. Verify agent reply appears in the chat UI ── + + await test.step("agent reply token appears in chat UI", async () => { + await waitForNonUserMessageText(page, IMAGE_REPLY_TOKEN, 30_000); + }); + + // ── 6c. Verify agent reply captured in conversation events API ── + + await test.step("agent reply captured in conversation events", async () => { + await waitForAgentMessageContaining( + request, + conversationId!, + IMAGE_REPLY_TOKEN, + 15_000, + ); + }); + + // ── 6d. Verify user message includes image_urls in conversation events ── + // The agent-server stores the user's message as a MessageEvent whose + // content array contains an "image" block with base64 data URLs. + + await test.step("user message event includes image_urls", async () => { + let lastDiag = "no polls yet"; + await expect + .poll( + async () => { + const resp = await request.get( + `${BACKEND_URL}/api/conversations/${encodeURIComponent(conversationId!)}/events/search`, + { + headers: { "X-Session-API-Key": SESSION_API_KEY }, + params: { limit: "50", sort_order: "TIMESTAMP_DESC" }, + }, + ); + if (!resp.ok()) { + lastDiag = `events API: ${resp.status()}`; + return false; + } + const body = (await resp.json()) as { items?: unknown[] }; + const items = body.items ?? []; + lastDiag = `${items.length} events`; + + return items.some((e: any) => { + if (e.source !== "user") return false; + // Check llm_message.content for image blocks (REST send path) + if (Array.isArray(e.llm_message?.content)) { + if ( + e.llm_message.content.some( + (c: any) => + c.type === "image" && + Array.isArray(c.image_urls) && + c.image_urls.length > 0, + ) + ) + return true; + } + // Fall back: args.image_urls (WebSocket send path) + const imageUrls = e.args?.image_urls; + return Array.isArray(imageUrls) && imageUrls.length > 0; + }); + }, + { timeout: 15_000 }, + ) + .toBe(true) + .catch((err) => { + throw new Error( + `User message event should have image_urls after 15s.\n${lastDiag}`, + { cause: err }, + ); + }); + }); + + // ── 6e. Verify the LLM completion call included image content ── + // The mock server stores every /v1/chat/completions request body since + // the last reset. At least one should contain an image_url content + // block with a base64 data: URL, confirming the frontend embedded the + // image rather than dropping it. + + await test.step("LLM completion call included image_url content", async () => { + const llmRequests = await getMockLLMRequests(request); + + expect( + llmRequests.length, + "mock LLM should have received at least one completion request", + ).toBeGreaterThan(0); + + // Helper: recursively walk any JSON value looking for a base64 image URL. + function containsImageUrl(value: unknown): boolean { + if (typeof value === "string") { + return value.startsWith("data:image/"); + } + if (Array.isArray(value)) { + return value.some(containsImageUrl); + } + if (value !== null && typeof value === "object") { + return Object.values(value as Record).some( + containsImageUrl, + ); + } + return false; + } + + const anyRequestHadImage = llmRequests.some(containsImageUrl); + expect( + anyRequestHadImage, + `At least one LLM completion call should include a base64 image data: URL.\n` + + `Received ${llmRequests.length} request(s).\n` + + llmRequests + .map( + (req, i) => + `Request ${i} messages:\n` + + JSON.stringify((req as any)?.messages ?? [], null, 2).slice( + 0, + 800, + ), + ) + .join("\n---\n"), + ).toBe(true); + }); + + // ── 6f. Verify no error banners appeared ── + + await test.step("no error banners", async () => { + await expect(page.getByTestId("error-message-banner")).not.toBeVisible({ + timeout: 2_000, + }); + }); + }); +}); diff --git a/tests/e2e/mock-llm/scripts/mock-llm-server.py b/tests/e2e/mock-llm/scripts/mock-llm-server.py index c54917e83..f58c0141d 100644 --- a/tests/e2e/mock-llm/scripts/mock-llm-server.py +++ b/tests/e2e/mock-llm/scripts/mock-llm-server.py @@ -77,10 +77,22 @@ class MockLLMHandler(BaseHTTPRequestHandler): # Named trajectories that tests can register via the admin API and then # activate with POST /admin/trajectory/activate. _named_trajectories: dict[str, list[Message | Exception]] = {} + # All completion request bodies since the last /admin/reset. + # Tests read them via GET /admin/requests to verify image / content details. + # Stored as a list so assertions survive even when the agent-server makes + # multiple LLM calls (e.g., internal condenser calls after the main turn). + _completion_requests: list = [] _lock = threading.Lock() def do_GET(self): - """Health check — Playwright's webServer probes GET / to detect readiness.""" + """Health check and admin read endpoints.""" + path = self.path.rstrip("/").split("?")[0] + if path == "/admin/requests": + with self._lock: + payload = list(MockLLMHandler._completion_requests) + self._send_json(200, {"requests": payload}) + return + # Default: health check — Playwright's webServer probes GET / to detect readiness. self._send_json(200, {"status": "ok", "server": "mock-llm"}) def do_POST(self): @@ -91,6 +103,7 @@ def do_POST(self): with self._lock: MockLLMHandler.test_llm = TestLLM.from_messages(build_trajectory()) MockLLMHandler._named_trajectories.clear() + MockLLMHandler._completion_requests.clear() remaining = MockLLMHandler.test_llm.remaining_responses self._send_json(200, { "status": "reset", @@ -149,6 +162,11 @@ def do_POST(self): length = int(self.headers.get("Content-Length", 0)) body = json.loads(self.rfile.read(length)) if length else {} + # Append to request history for test verification. + # Tests can GET /admin/requests to confirm image content was included. + with self._lock: + MockLLMHandler._completion_requests.append(body) + try: response = self.test_llm.completion([]) except TestLLMExhaustedError: diff --git a/tests/e2e/mock-llm/utils/mock-llm-helpers.ts b/tests/e2e/mock-llm/utils/mock-llm-helpers.ts index 28073137f..b557a6aff 100644 --- a/tests/e2e/mock-llm/utils/mock-llm-helpers.ts +++ b/tests/e2e/mock-llm/utils/mock-llm-helpers.ts @@ -12,6 +12,18 @@ export const BASH_TOKEN = "MOCK_LLM_E2E_BASH_OK"; export const REPLY_TOKEN = "MOCK_LLM_E2E_REPLY_OK"; export const BASH_COMMAND = `printf '${BASH_TOKEN}\\n'`; +/** Reply token used by the image-upload test trajectory. */ +export const IMAGE_REPLY_TOKEN = "MOCK_LLM_IMAGE_OK"; + +/** + * A minimal valid 1×1 white pixel PNG, base64-encoded. + * Used as a lightweight test fixture for image-upload E2E tests — small + * enough to keep request bodies manageable while still being a real PNG that + * the browser's FileReader can process. + */ +export const MINIMAL_PNG_BASE64 = + "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAAC0lEQVQI12NgAAIABQAABjE+ibYAAAAASUVORK5CYII="; + // Ports / URLs — set via env or defaults matching playwright.mock-llm.config.ts. // The agent-canvas binary exposes a single ingress port; API calls are proxied // through it, so BACKEND_URL = ingress URL (no separate backend port). @@ -403,12 +415,29 @@ export async function activateTrajectory( /** * Reset the mock LLM server to its default trajectory. + * Also clears the stored completion-request history. */ export async function resetMockLLM(request: APIRequestContext) { const resp = await request.post(`${MOCK_LLM_BASE_URL}/admin/reset`); expect(resp.ok(), `Reset mock LLM: ${resp.status()}`).toBe(true); } +/** + * Fetch all chat-completion request bodies captured by the mock LLM server + * since the last /admin/reset. + * + * The server stores every POST to /v1/chat/completions, so callers can assert + * that at least one request contained image content (or any other field). + */ +export async function getMockLLMRequests( + request: APIRequestContext, +): Promise[]> { + const resp = await request.get(`${MOCK_LLM_BASE_URL}/admin/requests`); + expect(resp.ok(), `GET /admin/requests: ${resp.status()}`).toBe(true); + const body = await resp.json(); + return (body.requests as Record[]) ?? []; +} + /** * Set contentEditable chat input text and dispatch an input event. *