mirror of
https://github.com/OpenHands/OpenHands.git
synced 2026-10-07 13:38:55 +08:00
fix(upload): resolve relative working dirs against /api/file/home (#1106)
* fix(upload): resolve relative working dirs against /api/file/home The agent-server's /api/file/upload endpoint requires an absolute path and `mkdir -p`'s the parent. The frontend's `toAbsoluteWorkspacePath` was naively prepending `/` to the default relative `workspace/project` working dir, producing `/workspace/project/<hex>/...`. On macOS and fresh Docker images that path lives under a read-only filesystem root, so uploads failed with `OSError: [Errno 30] Read-only file system: '/workspace'`. Conversations themselves kept working because the agent-server resolves relative `workspace.working_dir` against its own process CWD (which is writable in dev), so the worktree landed elsewhere and only uploads mistargeted the read-only root. Fix: introduce `getAgentServerHomeDir` (cached per backend, backed by `FileClient.getHome` → `GET /api/file/home`) and a `resolveAbsoluteAgentServerPath` helper. Both the conversation-start payload and the file-upload destination now go through this resolver, so they always agree on a single absolute path anchored at the agent-server's home directory (e.g. `~/workspace/project/<hex>`). Absolute paths pass through unchanged, so explicit workspace selections and `VITE_WORKING_DIR` overrides are unaffected. Spec: WUP-001 in specs/workspace-upload-path.md. Co-authored-by: openhands <openhands@all-hands.dev> * docs: clarify resolveConversationUploadWorkingDir returns raw (possibly-relative) working dir Add JSDoc explaining that callers must funnel the result through buildWorkspaceUploadPath (which calls resolveAbsoluteWorkspacePath) to get an absolute path for the upload endpoint. Also document that the UNC path case is already covered by the existing isAbsolutePath regex. Addresses review comment on PR #1106. Co-authored-by: openhands <openhands@all-hands.dev> * test(e2e): add mock-llm image-upload test Adds an end-to-end mock-LLM test that exercises the full image-attachment pipeline: 1. Attaches a minimal 1×1 PNG to the home-page chat via the hidden file input (data-testid="upload-image-input") using Playwright's setInputFiles. 2. Submits "What is in this image?" — creating a conversation and sending the message via sendMessageWithAttachments. 3. Verifies the agent replies with IMAGE_REPLY_TOKEN in the chat UI. 4. Verifies the user MessageEvent in the conversation events API has image_urls populated (base64 data: URL). 5. Verifies at least one /v1/chat/completions call to the mock server contained an image_url content block — confirming the image was forwarded to the LLM as expected. Supporting changes: - mock-llm-server.py: add GET /admin/requests endpoint that exposes all captured completion request bodies since the last reset; reset also clears the history. - mock-llm-helpers.ts: add getMockLLMRequests(), IMAGE_REPLY_TOKEN, and MINIMAL_PNG_BASE64 exports. - AGENTS.md: document the new endpoint and test spec. Co-authored-by: openhands <openhands@all-hands.dev> * test(e2e): add padding response to image-upload trajectory The agent-server makes one internal LLM call for skill-analysis before the main agent loop starts. The original 1-response trajectory was consumed by that internal call, leaving the agent with a 500 error and retry storm. Add 1 padding response (turn 0: empty text) + 1 safety buffer (turn 2) following the same pattern as mock-llm-automation.spec.ts. Co-authored-by: openhands <openhands@all-hands.dev> * test(e2e): use gpt-4o model name for vision-capable LLM requests litellm strips image_url content blocks for unknown model names like 'openai/mock-test-model'. Switch to 'openai/gpt-4o' so litellm knows the model accepts vision content and includes base64 image_url blocks in the completion request body. The base_url still points at the local mock server; the model name is only a hint to litellm's request formatter. Also improve assertion diagnostics to print all captured LLM requests (not just the first) when the assertion fails. Co-authored-by: openhands <openhands@all-hands.dev> --------- Co-authored-by: openhands <openhands@all-hands.dev>
This commit is contained in:
committed by
GitHub
co-authored by
openhands
parent
30cd9e3d88
commit
2ed2c571e6
@@ -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.
|
||||
|
||||
@@ -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", () => {
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
@@ -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"],
|
||||
|
||||
@@ -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");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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/<hex>` produces `/workspace/project/<hex>`. On macOS and on fresh Docker images that mount only `/home/<user>` 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.
|
||||
@@ -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<string>` rather than `string` so concurrent
|
||||
* callers share a single in-flight request.
|
||||
*/
|
||||
const homeDirCache = new Map<string, Promise<string>>();
|
||||
|
||||
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<string> {
|
||||
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/<hex>` 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<string> {
|
||||
const trimmed = workingDir.replace(/[/\\]+$/, "");
|
||||
if (!trimmed) {
|
||||
return getAgentServerHomeDir(overrides);
|
||||
}
|
||||
if (isAbsolutePath(trimmed)) return trimmed;
|
||||
|
||||
const home = await getAgentServerHomeDir(overrides);
|
||||
return joinPath(home, trimmed);
|
||||
}
|
||||
@@ -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/<hex>` (relative) land at
|
||||
// `/workspace/project/<hex>/...` 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) {
|
||||
|
||||
@@ -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/<hex>` (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({
|
||||
|
||||
@@ -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(
|
||||
fileName: string,
|
||||
/**
|
||||
* 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,
|
||||
): string {
|
||||
const safeName = getSafeUploadFileName(fileName);
|
||||
const base = toAbsoluteWorkspacePath(workingDir.replace(/\/+$/, ""));
|
||||
return `${base}/${safeName}`;
|
||||
overrides: AgentServerClientOverrides = {},
|
||||
): Promise<string> {
|
||||
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,
|
||||
overrides: AgentServerClientOverrides = {},
|
||||
): Promise<string> {
|
||||
const safeName = getSafeUploadFileName(fileName);
|
||||
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/<hex>`) 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 };
|
||||
|
||||
@@ -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",
|
||||
};
|
||||
}
|
||||
|
||||
@@ -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<string, unknown>).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,
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -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 @@ class MockLLMHandler(BaseHTTPRequestHandler):
|
||||
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 @@ class MockLLMHandler(BaseHTTPRequestHandler):
|
||||
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:
|
||||
|
||||
@@ -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<Record<string, unknown>[]> {
|
||||
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<string, unknown>[]) ?? [];
|
||||
}
|
||||
|
||||
/**
|
||||
* Set contentEditable chat input text and dispatch an input event.
|
||||
*
|
||||
|
||||
Reference in New Issue
Block a user