From fc6cf5e6907dd9fde31fcb454002cffcf5d465d4 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 26 Aug 2026 17:20:39 -0700 Subject: [PATCH] fix(ui): keep agent file saves intact during refresh (#130468) * fix(ui): keep agent file saves intact during refresh * test(ui): hold catalog requests through recovery startup --- CHANGELOG.md | 2 + ui/src/e2e/agent-file-lifecycle.e2e.test.ts | 61 ++++- ...w-session-page.device-dispatch.e2e.test.ts | 27 +- ui/src/lib/agents/index.ts | 27 ++ .../pages/agents/agent-file-lifecycle.test.ts | 238 ++++++++++++++++-- ui/src/pages/agents/agents-page.test.ts | 1 + ui/src/pages/agents/agents-page.ts | 22 +- ui/src/pages/agents/files.test.ts | 106 +++++++- ui/src/pages/agents/files.ts | 177 ++++++------- .../control-ui-e2e.deferral.test.ts | 110 ++++++++ ui/src/test-helpers/control-ui-e2e.ts | 96 ++++--- 11 files changed, 686 insertions(+), 181 deletions(-) create mode 100644 ui/src/test-helpers/control-ui-e2e.deferral.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 31301292c8e4..7888d59eb7c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,6 +69,8 @@ Docs: https://docs.openclaw.ai ### Fixes +- **Control UI agent files:** keep confirmed saves and file metadata intact when older reads or list refreshes finish later, preserve newer drafts, and rebuild invalidated file lists without losing the open editor. + - **Control UI terminal transcript settlement:** retire live commentary, tool, and streamed reply projections atomically when the matching durable terminal message arrives, preventing duplicated final responses and transient row overlap after steering. Fixes #127209. Thanks @shakkernerd. - **Control UI Codex compaction history:** preserve successful native context compactions as durable, model-excluded activity inside completed work traces after the composer status clears or the session reloads. Fixes #127206. Thanks @shakkernerd. - **Control UI Codex steering:** preserve pre-steer commentary and tool activity in durable transcript order, keep it visible while active, and collapse it before the steering message after completion. Fixes #126938. Thanks @shakkernerd. diff --git a/ui/src/e2e/agent-file-lifecycle.e2e.test.ts b/ui/src/e2e/agent-file-lifecycle.e2e.test.ts index 46166b235701..a14589890f7d 100644 --- a/ui/src/e2e/agent-file-lifecycle.e2e.test.ts +++ b/ui/src/e2e/agent-file-lifecycle.e2e.test.ts @@ -128,7 +128,7 @@ suite.define(() => { "agents.list": { defaultId: "main", mainKey: "main", - scope: "agent", + scope: "per-sender", agents: [ { id: "main", name: "Main" }, { id: "writer", name: "Writer" }, @@ -212,12 +212,15 @@ suite.define(() => { ); }); - it("refreshes the active file while preserving a dirty draft", async () => { + it("preserves drafts and confirmed saves across overlapping refreshes", async () => { await suite.withPage( { locale: "en-US", serviceWorkers: "block", viewport: { height: 900, width: 1440 }, + ...(captureUiProof + ? { recordVideo: { dir: proofDir, size: { height: 900, width: 1440 } } } + : {}), }, async ({ page }) => { const gateway = await installMockGateway(page, { @@ -231,7 +234,7 @@ suite.define(() => { "agents.list": { defaultId: "main", mainKey: "main", - scope: "agent", + scope: "per-sender", agents: [{ id: "main", name: "Main" }], }, "agents.files.get": fileGetResponses("server revision 1"), @@ -274,8 +277,60 @@ suite.define(() => { await reset.click(); await expect.poll(() => editor.inputValue()).toBe("server revision 3"); await expect.poll(() => reset.isDisabled()).toBe(true); + await expect.poll(() => save.isDisabled()).toBe(true); await capture(page, "06-reset-uses-refreshed-authoritative-content.png"); + + await gateway.deferNext("agents.files.get", { agentId: "main", name: "AGENTS.md" }); + await refresh.click(); + await expect + .poll(async () => (await gateway.getRequests("agents.files.get")).length) + .toBe(4); + await editor.fill("Saved latest instructions"); + await gateway.setMethodResponse("agents.files.set", { + ok: true, + ...fileGet("main", "Saved latest instructions"), + }); + await gateway.setMethodResponse( + "agents.files.get", + fileGetResponses("Saved latest instructions"), + ); + await save.click(); + await expect.poll(() => save.isDisabled()).toBe(true); + await gateway.resolveDeferred("agents.files.get", fileGet("main", "server revision 3")); + await expect.poll(() => refresh.isEnabled()).toBe(true); + await capture(page, "08-save-survives-older-refresh.png"); + await expect.poll(() => editor.inputValue()).toBe("Saved latest instructions"); + await expect.poll(() => reset.isDisabled()).toBe(true); + + const missingFile = { ...fileList("main").files[0], missing: true, content: "" }; + const missingList = { ...fileList("main"), files: [missingFile] }; + await gateway.setMethodResponse("agents.files.list", missingList); + await gateway.setMethodResponse("agents.files.get", { ...missingList, file: missingFile }); + await refresh.click(); + await expect.poll(() => editor.inputValue()).toBe(""); + const missingHint = page.locator("#agent-file-panel .callout.info"); + await expect.poll(() => missingHint.isVisible()).toBe(true); + await gateway.deferNext("agents.files.list", { agentId: "main" }); + const listsBeforeSave = (await gateway.getRequests("agents.files.list")).length; + await refresh.click(); + await expect + .poll(async () => (await gateway.getRequests("agents.files.list")).length) + .toBe(listsBeforeSave + 1); + await editor.fill("Saved latest instructions"); + await gateway.setMethodResponse( + "agents.files.get", + fileGetResponses("Saved latest instructions"), + ); + await save.click(); + await expect.poll(() => missingHint.count()).toBe(0); + await gateway.resolveDeferred("agents.files.list", missingList); + await expect.poll(() => refresh.isEnabled()).toBe(true); + await page.locator(".agents-refresh-btn").click(); + await expect.poll(() => page.locator(".agents-refresh-btn").isEnabled()).toBe(true); + await expect.poll(() => editor.inputValue()).toBe("Saved latest instructions"); + expect(await missingHint.count()).toBe(0); + await capture(page, "09-created-file-survives-stale-list.png"); }, ); }); diff --git a/ui/src/e2e/new-session-page.device-dispatch.e2e.test.ts b/ui/src/e2e/new-session-page.device-dispatch.e2e.test.ts index 1c22bbf45567..d5c72c363540 100644 --- a/ui/src/e2e/new-session-page.device-dispatch.e2e.test.ts +++ b/ui/src/e2e/new-session-page.device-dispatch.e2e.test.ts @@ -232,6 +232,22 @@ suite.define(() => { async ({ preference, attribute, value, target }) => { const context = await suite.browser.newContext({ locale: "en-US", serviceWorkers: "block" }); const page = await context.newPage(); + if (preference.kind === "cloud") { + // Settle recovery scope after discovery starts: both the retired and + // replacement catalog request must stay held until restoration resumes. + await page.addInitScript(() => { + const originalDigest = crypto.subtle.digest.bind(crypto.subtle); + const delayed = new Promise((resolve) => { + window.addEventListener("test-release-recovery-scope", () => resolve(), { once: true }); + }); + crypto.subtle.digest = async (algorithm, data) => { + if (new TextDecoder().decode(data) === "e2e-device-token") { + await delayed; + } + return originalDigest(algorithm, data); + }; + }); + } const appUrl = new URL(suite.server.baseUrl); const gatewayUrl = `${appUrl.protocol === "https:" ? "wss:" : "ws:"}//${appUrl.host}`; const storageKey = `openclaw.new-session.preferences.v1:${gatewayOriginScope(gatewayUrl)}`; @@ -261,11 +277,12 @@ suite.define(() => { { key: storageKey, workspace: WORKSPACE, where: preference }, ); const gateway = await installMockGateway(page, { - deferredMethods: ["environments.list"], + heldMethods: ["environments.list"], operatorScopes: ["operator.read", "operator.write", "operator.admin"], workspace: WORKSPACE, workspaceGit: true, methodResponses: { + "environments.list": catalog, "worktrees.branches": { branches: [{ kind: "local", name: "main" }], defaultBranch: "main", @@ -284,6 +301,12 @@ suite.define(() => { await expect .poll(() => page.locator("#new-session-detail-trigger").getAttribute("data-worktree")) .toBe("true"); + if (preference.kind === "cloud") { + await page.evaluate(() => { + window.dispatchEvent(new Event("test-release-recovery-scope")); + }); + await gateway.waitForRequest("environments.list", { after: 1 }); + } await page.locator(".new-session-page__message").fill("keep my chosen remote destination"); const start = page.getByRole("button", { name: "Start session" }); await expect.poll(() => start.isDisabled()).toBe(true); @@ -292,7 +315,7 @@ suite.define(() => { .toContain("Restoring your last session setup"); expect(await gateway.getRequests("sessions.create")).toHaveLength(0); - await gateway.resolveDeferred("environments.list", catalog); + await gateway.resolveDeferred("environments.list"); const where = page.locator("#new-session-where-trigger"); await expect.poll(() => where.getAttribute(attribute)).toBe(value); await expect.poll(() => start.isEnabled()).toBe(true); diff --git a/ui/src/lib/agents/index.ts b/ui/src/lib/agents/index.ts index f3551abf8af5..ad45a0b903b0 100644 --- a/ui/src/lib/agents/index.ts +++ b/ui/src/lib/agents/index.ts @@ -1,5 +1,6 @@ import type { GatewayBrowserClient } from "../../api/gateway.ts"; import type { + AgentsFilesGetResult, AgentsFilesListResult, AgentsListResult, ModelCatalogEntry, @@ -87,6 +88,7 @@ export type AgentCapability = { invalidateFiles: (agentIds: readonly (string | null | undefined)[]) => void; ensureFiles: (agentId: string) => Promise; refreshFiles: (agentId: string) => Promise; + recordFile: (result: AgentsFilesGetResult) => void; subscribe: (listener: (state: AgentCapabilityState) => void) => () => void; dispose: () => void; }; @@ -420,6 +422,31 @@ export function createAgentCapability(gateway: AgentGateway): AgentCapability { }, ensureFiles: (agentId) => loadFiles(agentId, false), refreshFiles: (agentId) => loadFiles(agentId, true), + recordFile({ agentId, file }) { + const status = fileStatus(agentId); + if (!status.list) { + // Reconnect/config invalidation can clear the list while an editor + // remains open. Rebuild the full list after the confirmed operation. + void loadFiles(agentId, true); + return; + } + // A confirmed file result supersedes lists already in flight. Retain the + // full canonical list so their awaiting callers can still read it. + fileRequests.delete(agentId); + fileRequestOwners.delete(agentId); + const entry = { ...file }; + delete entry.content; + const entries = status.list.files; + status.list = { + ...status.list, + files: entries.some((existing) => existing.name === entry.name) + ? entries.map((existing) => (existing.name === entry.name ? entry : existing)) + : [...entries, entry], + }; + status.loading = false; + status.error = null; + publish(); + }, subscribe(listener) { listeners.add(listener); return () => listeners.delete(listener); diff --git a/ui/src/pages/agents/agent-file-lifecycle.test.ts b/ui/src/pages/agents/agent-file-lifecycle.test.ts index 7b1dde9ceee7..a1271a6cf946 100644 --- a/ui/src/pages/agents/agent-file-lifecycle.test.ts +++ b/ui/src/pages/agents/agent-file-lifecycle.test.ts @@ -1,35 +1,43 @@ /* @vitest-environment jsdom */ +import { render, type TemplateResult } from "lit"; import { describe, expect, it, vi } from "vitest"; +import { createDeferred } from "../../../../test/helpers/promise.js"; import type { GatewayBrowserClient } from "../../api/gateway.ts"; -import type { AgentsFilesListResult } from "../../api/types.ts"; +import type { + AgentsFilesGetResult, + AgentsFilesListResult, + AgentsListResult, +} from "../../api/types.ts"; import type { ApplicationContext, ApplicationGatewaySnapshot } from "../../app/context.ts"; +import { createAgentCapability } from "../../lib/agents/index.ts"; import { gatewayHelloForMethods } from "../../test-helpers/gateway-methods.ts"; +import { loadAgentFileContent } from "./files.ts"; import type { AgentsRouteData } from "./route.ts"; import "./agents-page.ts"; const AGENT_FILE_GATEWAY_HELLO = gatewayHelloForMethods(["agents.files.set"]); -type TestAgentsPage = HTMLElement & { - context: ApplicationContext; - routeData?: AgentsRouteData; - agentsSelectedId: string | null; - agentFilesLoading: boolean; - agentFilesError: string | null; - agentFileActive: string | null; - agentFileContents: Record; - agentFileDrafts: Record; - gateway: { - applySnapshot: ( - snapshot: ApplicationGatewaySnapshot, - binding: { initial: boolean; sourceChanged: boolean }, - ) => void; +type TestAgentsPage = HTMLElement & + Parameters[0] & { + context: ApplicationContext; + routeData?: AgentsRouteData; + agentsList: AgentsListResult | null; + agentsSelectedId: string | null; + agentFilesList: AgentsFilesListResult | null; + agentFileActive: string | null; + gateway: { + applySnapshot: ( + snapshot: ApplicationGatewaySnapshot, + binding: { initial: boolean; sourceChanged: boolean }, + ) => void; + }; + selectDefaultAgentFile: (agentId: string) => Promise; + syncCurrentAgentFiles: (agents?: ApplicationContext["agents"]) => void; + loadAgentFiles: (agentId: string, force?: boolean) => Promise; + saveSelectedAgentFile: (agentId: string, name: string, content: string) => void; + render: () => TemplateResult; }; - selectDefaultAgentFile: (agentId: string) => Promise; - syncCurrentAgentFiles: (agents?: ApplicationContext["agents"]) => void; - loadAgentFiles: (agentId: string, force?: boolean) => Promise; - saveSelectedAgentFile: (agentId: string, name: string, content: string) => void; -}; function snapshot(client: GatewayBrowserClient): ApplicationGatewaySnapshot { return { @@ -74,6 +82,7 @@ describe("agent file lifecycle", () => { const client = { request } as unknown as GatewayBrowserClient; const agents = { files: () => ({ list, loading: false, error: null }), + recordFile: () => list, } as unknown as ApplicationContext["agents"]; const page = document.createElement("openclaw-agents-page") as TestAgentsPage; page.context = { gateway: gateway(snapshot(client)), agents } as unknown as ApplicationContext; @@ -110,6 +119,7 @@ describe("agent file lifecycle", () => { files: () => ({ list: null, loading: false, error: null }), ensureFiles: vi.fn(async () => list), refreshFiles, + recordFile: () => list, }, } as unknown as ApplicationContext; setPageGateway(page, client); @@ -148,4 +158,192 @@ describe("agent file lifecycle", () => { await vi.waitFor(() => expect(page.agentFilesError).toBe("workspace write failed")); expect(refreshFiles).not.toHaveBeenCalled(); }); + + it.each(["content read", "file list", "invalidated cache"] as const)( + "keeps saved file metadata through a pending %s and unrelated agent publication", + async (pendingRequest) => { + const missingFile = { name: "AGENTS.md", path: "/tmp/workspace/AGENTS.md", missing: true }; + const missingList = { ...fileList(), files: [missingFile] }; + const savedFile = { ...missingFile, missing: false, content: "saved content", size: 13 }; + const savedList = { ...missingList, files: [savedFile] }; + const savedResult = { agentId: "main", workspace: missingList.workspace, file: savedFile }; + const contentRead = createDeferred(); + const listRead = createDeferred(); + let listCalls = 0; + let contentCalls = 0; + const request = vi.fn(async (method: string) => { + if (method === "agents.files.list") { + listCalls += 1; + if (listCalls === 1) { + return missingList; + } + return pendingRequest === "file list" && listCalls === 2 + ? await listRead.promise + : savedList; + } + if (method === "agents.files.get") { + contentCalls += 1; + return pendingRequest === "content read" && contentCalls === 1 + ? await contentRead.promise + : savedResult; + } + if (method === "agents.files.set") { + return { ok: true, ...savedResult }; + } + if (method === "agents.list") { + return { defaultId: "main", agents: [{ id: "main" }] }; + } + throw new Error(`Unexpected request: ${method}`); + }); + const client = { request } as unknown as GatewayBrowserClient; + const currentGateway = gateway(snapshot(client)); + const agents = createAgentCapability(currentGateway); + await agents.ensureFiles("main"); + const page = document.createElement("openclaw-agents-page") as TestAgentsPage; + page.context = { gateway: currentGateway, agents } as unknown as ApplicationContext; + setPageGateway(page, client); + page.agentsSelectedId = "main"; + page.routeData = { panel: "files" } as AgentsRouteData; + page.agentFilesList = missingList; + page.agentFileActive = missingFile.name; + page.agentFileContents = { [missingFile.name]: "" }; + page.agentFileDrafts = { [missingFile.name]: savedFile.content }; + const unsubscribe = agents.subscribe(() => page.syncCurrentAgentFiles(agents)); + const refresh = + pendingRequest !== "file list" + ? loadAgentFileContent(page, "main", missingFile.name, { force: true }) + : page.loadAgentFiles("main", true); + + try { + expect(page.agentFilesLoading).toBe(true); + if (pendingRequest === "invalidated cache") { + agents.invalidateFiles(["main"]); + } + page.saveSelectedAgentFile("main", missingFile.name, savedFile.content); + await vi.waitFor(() => { + expect(page.agentFileSaving).toBe(false); + expect(page.agentFileContents[missingFile.name]).toBe(savedFile.content); + }); + expect(page.agentFilesList?.files).toEqual([ + expect.objectContaining({ name: missingFile.name, missing: false, size: 13 }), + ]); + + contentRead.resolve(savedResult); + listRead.resolve(missingList); + await refresh; + expect(page.agentFilesList?.files).toEqual([ + expect.objectContaining({ name: missingFile.name, missing: false, size: 13 }), + ]); + expect(page.agentFileActive).toBe(missingFile.name); + + await agents.refreshList(); + + expect(page.agentFilesList?.files).toEqual([ + expect.objectContaining({ name: missingFile.name, missing: false, size: 13 }), + ]); + expect(page.agentFileContents[missingFile.name]).toBe(savedFile.content); + expect(page.agentFileDrafts[missingFile.name]).toBe(savedFile.content); + expect(page.agentFilesError).toBeNull(); + } finally { + unsubscribe(); + contentRead.resolve(savedResult); + listRead.resolve(missingList); + await refresh; + agents.dispose(); + } + }, + ); + + it.each([false, true])( + "renders a failed cache rebuild without replacing a newer save error: %s", + async (newerSaveFails) => { + const list = fileList(); + const savedContent = "saved content"; + const rebuildError = "workspace metadata refresh failed"; + const writeError = "newer workspace write failed"; + const rebuild = createDeferred(); + let listCalls = 0; + let writeCalls = 0; + const request = vi.fn(async (method: string) => { + if (method === "agents.files.list") { + listCalls += 1; + return listCalls === 1 ? list : await rebuild.promise; + } + if (method === "agents.files.set") { + writeCalls += 1; + if (writeCalls > 1) { + throw new Error(writeError); + } + return { + ok: true, + agentId: "main", + workspace: list.workspace, + file: { ...list.files[0], content: savedContent }, + }; + } + throw new Error(`Unexpected request: ${method}`); + }); + const client = { request } as unknown as GatewayBrowserClient; + const currentGateway = gateway(snapshot(client)); + const agents = createAgentCapability(currentGateway); + await agents.ensureFiles("main"); + const page = document.createElement("openclaw-agents-page") as TestAgentsPage; + page.context = { + basePath: "", + gateway: { ...currentGateway, connection: { password: "" } }, + agents, + agentIdentity: { entries: () => [] }, + channels: { state: {} }, + runtimeConfig: { state: { configForm: {} } }, + navigation: { snapshot: { pinnedAgentIds: [] } }, + } as unknown as ApplicationContext; + setPageGateway(page, client); + page.agentsList = { + defaultId: "main", + mainKey: "main", + scope: "per-sender", + agents: [{ id: "main", name: "Main" }], + }; + page.agentsSelectedId = "main"; + page.routeData = { panel: "files" } as AgentsRouteData; + page.agentFilesList = list; + page.agentFileActive = "AGENTS.md"; + page.agentFileContents = { "AGENTS.md": "original" }; + page.agentFileDrafts = { "AGENTS.md": savedContent }; + const unsubscribe = agents.subscribe(() => page.syncCurrentAgentFiles(agents)); + const container = document.createElement("div"); + + try { + agents.invalidateFiles(["main"]); + page.saveSelectedAgentFile("main", "AGENTS.md", savedContent); + await vi.waitFor(() => { + expect(page.agentFileSaving).toBe(false); + expect(page.agentFileContents["AGENTS.md"]).toBe(savedContent); + expect(listCalls).toBe(2); + }); + if (newerSaveFails) { + page.agentFileDrafts = { "AGENTS.md": "newer draft" }; + page.saveSelectedAgentFile("main", "AGENTS.md", "newer draft"); + await vi.waitFor(() => expect(page.agentFilesError).toBe(writeError)); + } + rebuild.reject(new Error(rebuildError)); + await vi.waitFor(() => expect(agents.files("main").error).toBe(rebuildError)); + + render(page.render(), container); + + expect(container.querySelector(".callout.danger")?.textContent).toBe( + newerSaveFails ? writeError : rebuildError, + ); + expect(container.querySelector(".agent-file-textarea")?.value).toBe( + newerSaveFails ? "newer draft" : savedContent, + ); + expect(page.agentFileContents["AGENTS.md"]).toBe(savedContent); + } finally { + unsubscribe(); + rebuild.resolve(list); + agents.dispose(); + render(null, container); + } + }, + ); }); diff --git a/ui/src/pages/agents/agents-page.test.ts b/ui/src/pages/agents/agents-page.test.ts index 6e0cf6fdd7a8..c28f3f5bce51 100644 --- a/ui/src/pages/agents/agents-page.test.ts +++ b/ui/src/pages/agents/agents-page.test.ts @@ -904,6 +904,7 @@ describe("AgentsPage gateway lifecycle", () => { files: () => ({ list: null, loading: false, error: null }), ensureFiles: vi.fn(async () => fileList), refreshFiles: vi.fn(async () => fileList), + recordFile: vi.fn(), }, } as unknown as ApplicationContext; diff --git a/ui/src/pages/agents/agents-page.ts b/ui/src/pages/agents/agents-page.ts index 9aa30dd755db..69cd4f760450 100644 --- a/ui/src/pages/agents/agents-page.ts +++ b/ui/src/pages/agents/agents-page.ts @@ -105,6 +105,7 @@ class AgentsPage @state() agentFileDrafts: Record = {}; @state() agentFileActive: string | null = null; @state() agentFileSaving = false; + readonly agentFileWriteRevisions = new Map(); @state() agentIdentityLoading = false; @state() agentIdentityError: string | null = null; @state() identityDraft: AgentIdentityDraft = { name: null, emoji: null, avatar: null }; @@ -251,6 +252,10 @@ class AgentsPage return this.context.sessions; } + get agents() { + return this.context.agents; + } + get client() { return this.gateway.client; } @@ -332,7 +337,6 @@ class AgentsPage return; } this.agentFilesList = status.list; - this.agentFilesError = status.error; void this.selectDefaultAgentFile(agentId); } @@ -671,7 +675,6 @@ class AgentsPage return; } this.agentFilesList = list ?? agents.files(agentId).list; - this.agentFilesError = agents.files(agentId).error; } finally { if (this.isCurrentRequest(client, generation, agentId, { agents })) { this.agentFilesLoading = false; @@ -745,6 +748,7 @@ class AgentsPage this.agentFileActive = null; this.agentFileContents = {}; this.agentFileDrafts = {}; + this.agentFileWriteRevisions.clear(); this.agentFilesLoading = false; this.agentFileSaving = false; this.agentSkillsReport = null; @@ -858,17 +862,7 @@ class AgentsPage if (!this.canCall("agents.files.set", "operator.admin")) { return; } - const client = this.client; - const generation = this.requestGeneration; - const agents = this.context.agents; - if (!client) { - return; - } - void saveAgentFile(this, agentId, name, content).then((saved) => { - if (saved && this.isCurrentRequest(client, generation, agentId, { agents })) { - void this.loadAgentFiles(agentId, true); - } - }); + void saveAgentFile(this, agentId, name, content); } private reloadConfig() { @@ -974,7 +968,7 @@ class AgentsPage agentFiles: { list: this.agentFilesList, loading: this.agentFilesLoading, - error: this.agentFilesError, + error: this.agentFilesError ?? this.context.agents.files(selectedAgentId).error, active: this.agentFileActive, contents: this.agentFileContents, drafts: this.agentFileDrafts, diff --git a/ui/src/pages/agents/files.test.ts b/ui/src/pages/agents/files.test.ts index adecf4fb22ef..3607250d9386 100644 --- a/ui/src/pages/agents/files.test.ts +++ b/ui/src/pages/agents/files.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it, vi } from "vitest"; +import { createDeferred } from "../../../../test/helpers/promise.js"; import type { GatewayBrowserClient } from "../../api/gateway.ts"; import type { AgentsFilesGetResult, AgentsFilesSetResult } from "../../api/types.ts"; import { loadAgentFileContent, saveAgentFile } from "./files.ts"; @@ -10,13 +11,13 @@ function createState(client: GatewayBrowserClient): FilesState { client, connected: true, requestGeneration: 0, + agents: { recordFile: vi.fn(() => null) }, agentFilesLoading: false, agentFilesError: null, - agentFilesList: { agentId: "main", workspace: "workspace", files: [] }, agentFileContents: {}, agentFileDrafts: {}, - agentFileActive: null, agentFileSaving: false, + agentFileWriteRevisions: new Map(), }; } @@ -29,6 +30,96 @@ function fileResult(content: string): AgentsFilesGetResult { } describe("agent file requests", () => { + it.each(["read result", "read error"])( + "retires an older %s after saving the same file", + async (completion) => { + const read = createDeferred(); + const client = { + request: vi.fn((method: string) => + method === "agents.files.get" + ? read.promise + : Promise.resolve({ ok: true, ...fileResult("saved") }), + ), + } as unknown as GatewayBrowserClient; + const state = createState(client); + state.agentFileContents = { "AGENTS.md": "original" }; + state.agentFileDrafts = { "AGENTS.md": "original" }; + + const load = loadAgentFileContent(state, "main", "AGENTS.md", { force: true }); + state.agentFileDrafts = { "AGENTS.md": "saved" }; + expect(await saveAgentFile(state, "main", "AGENTS.md", "saved")).toBe(true); + if (completion === "read error") { + read.reject(new Error("obsolete read failed")); + } else { + read.resolve(fileResult("original")); + } + expect(await load).toBe(false); + expect(state.agentFileContents).toEqual({ "AGENTS.md": "saved" }); + expect(state.agentFileDrafts).toEqual({ "AGENTS.md": "saved" }); + expect(state.agentFilesError).toBeNull(); + expect(state.agentFilesLoading).toBe(false); + }, + ); + + it("retires a read started during a write before publishing its result", async () => { + const read = createDeferred(); + const write = createDeferred(); + const client = { + request: vi.fn((method: string) => + method === "agents.files.get" ? read.promise : write.promise, + ), + } as unknown as GatewayBrowserClient; + const state = createState(client); + state.agentFileContents = { "AGENTS.md": "original" }; + state.agentFileDrafts = { "AGENTS.md": "saved" }; + + const save = saveAgentFile(state, "main", "AGENTS.md", "saved"); + const load = loadAgentFileContent(state, "main", "AGENTS.md", { force: true }); + write.resolve({ ok: true, ...fileResult("saved") }); + expect(await save).toBe(true); + read.resolve(fileResult("original")); + expect(await load).toBe(false); + expect(state.agentFileContents).toEqual({ "AGENTS.md": "saved" }); + expect(state.agentFileDrafts).toEqual({ "AGENTS.md": "saved" }); + expect(state.agentFilesLoading).toBe(false); + }); + + it("allows another file's read and a fresh post-save refresh", async () => { + const read = createDeferred(); + const request = vi + .fn() + .mockReturnValueOnce(read.promise) + .mockResolvedValueOnce({ ok: true, ...fileResult("saved") }) + .mockResolvedValueOnce(fileResult("external update")); + const state = createState({ request } as unknown as GatewayBrowserClient); + const load = loadAgentFileContent(state, "main", "SOUL.md"); + await saveAgentFile(state, "main", "AGENTS.md", "saved"); + read.resolve({ ...fileResult("soul"), file: { ...fileResult("soul").file, name: "SOUL.md" } }); + expect(await load).toBe(true); + expect(state.agentFileDrafts).toEqual({ "AGENTS.md": "saved", "SOUL.md": "soul" }); + + expect(await loadAgentFileContent(state, "main", "AGENTS.md", { force: true })).toBe(true); + expect(state.agentFileDrafts["AGENTS.md"]).toBe("external update"); + }); + + it("keeps a failed save and its dirty draft visible after an older read settles", async () => { + const read = createDeferred(); + const request = vi + .fn() + .mockReturnValueOnce(read.promise) + .mockRejectedValueOnce(new Error("workspace write failed")); + const state = createState({ request } as unknown as GatewayBrowserClient); + state.agentFileContents = { "AGENTS.md": "original" }; + state.agentFileDrafts = { "AGENTS.md": "unsaved" }; + const load = loadAgentFileContent(state, "main", "AGENTS.md", { force: true }); + expect(await saveAgentFile(state, "main", "AGENTS.md", "unsaved")).toBe(false); + read.resolve(fileResult("old")); + expect(await load).toBe(false); + expect(state.agentFileContents["AGENTS.md"]).toBe("original"); + expect(state.agentFileDrafts["AGENTS.md"]).toBe("unsaved"); + expect(state.agentFilesError).toBe("workspace write failed"); + }); + it("does not let an old-client read overwrite or finish a replacement read", async () => { let resolveOld!: (value: AgentsFilesGetResult) => void; let resolveNext!: (value: AgentsFilesGetResult) => void; @@ -67,7 +158,7 @@ describe("agent file requests", () => { expect(state.agentFilesLoading).toBe(false); }); - it("ignores an old-client save completion", async () => { + it.each(["client", "capability"] as const)("ignores an old-%s save completion", async (owner) => { let resolveSave!: (value: AgentsFilesSetResult) => void; const oldClient = { request: vi.fn( @@ -80,14 +171,19 @@ describe("agent file requests", () => { const state = createState(oldClient); const save = saveAgentFile(state, "main", "AGENTS.md", "old"); - state.client = { request: vi.fn() } as unknown as GatewayBrowserClient; - state.requestGeneration += 1; + if (owner === "client") { + state.client = { request: vi.fn() } as unknown as GatewayBrowserClient; + state.requestGeneration += 1; + } else { + state.agents = { recordFile: vi.fn() }; + } state.agentFileSaving = false; resolveSave({ ok: true, ...fileResult("old") }); await save; expect(state.agentFileContents).toEqual({}); expect(state.agentFileSaving).toBe(false); + expect(state.agents.recordFile).not.toHaveBeenCalled(); }); it("commits the submitted draft when it stays current", async () => { diff --git a/ui/src/pages/agents/files.ts b/ui/src/pages/agents/files.ts index 0bd91e52cfff..11ee9d73d87f 100644 --- a/ui/src/pages/agents/files.ts +++ b/ui/src/pages/agents/files.ts @@ -1,127 +1,112 @@ // Control UI controller manages agent files gateway state. import type { GatewayBrowserClient } from "../../api/gateway.ts"; -import type { - AgentFileEntry, - AgentsFilesGetResult, - AgentsFilesListResult, - AgentsFilesSetResult, -} from "../../api/types.ts"; +import type { AgentsFilesGetResult, AgentsFilesSetResult } from "../../api/types.ts"; +import type { AgentCapability } from "../../lib/agents/index.ts"; import { formatUiError } from "../../lib/format-error.ts"; type AgentFilesState = { client: GatewayBrowserClient | null; connected: boolean; requestGeneration: number; + agents: Pick; agentFilesLoading: boolean; agentFilesError: string | null; - agentFilesList: AgentsFilesListResult | null; agentFileContents: Record; agentFileDrafts: Record; - agentFileActive: string | null; agentFileSaving: boolean; + agentFileWriteRevisions: Map; }; -function mergeFileEntry( - list: AgentsFilesListResult | null, - entry: AgentFileEntry, -): AgentsFilesListResult | null { - if (!list) { - return list; +async function requestAgentFile( + state: AgentFilesState, + agentId: string, + name: string, + operation: { kind: "read"; force?: boolean } | { kind: "write"; content: string }, +): Promise { + const saving = operation.kind === "write"; + const busy = saving ? "agentFileSaving" : "agentFilesLoading"; + const client = state.client; + const agents = state.agents; + if (!client || !state.connected || state[busy]) { + return false; } - const hasEntry = list.files.some((file) => file.name === entry.name); - const nextFiles = hasEntry - ? list.files.map((file) => (file.name === entry.name ? entry : file)) - : [...list.files, entry]; - return { ...list, files: nextFiles }; + if ( + operation.kind === "read" && + !operation.force && + Object.hasOwn(state.agentFileContents, name) + ) { + return true; + } + const generation = state.requestGeneration; + const isConnected = () => + state.client === client && + state.agents === agents && + state.connected && + state.requestGeneration === generation; + const advanceWriteRevision = () => { + state.agentFileWriteRevisions.set(name, (state.agentFileWriteRevisions.get(name) ?? 0) + 1); + }; + // Retire reads admitted before a write, and again on settlement for reads + // admitted during it: a later read request can still return pre-write bytes. + if (saving) { + advanceWriteRevision(); + } + const revision = state.agentFileWriteRevisions.get(name); + const isCurrent = () => + isConnected() && (saving || state.agentFileWriteRevisions.get(name) === revision); + state[busy] = true; + state.agentFilesError = null; + try { + const res = await client.request( + saving ? "agents.files.set" : "agents.files.get", + { agentId, name, ...(operation.kind === "write" ? { content: operation.content } : {}) }, + ); + if (res?.file && isCurrent()) { + const content = operation.kind === "write" ? operation.content : (res.file.content ?? ""); + const previousBase = state.agentFileContents[name] ?? ""; + const currentDraft = state.agentFileDrafts[name]; + state.agentFileContents = { ...state.agentFileContents, [name]: content }; + // Reads rebase clean drafts; writes preserve edits made after submission. + if ( + !Object.hasOwn(state.agentFileDrafts, name) || + currentDraft === (saving ? content : previousBase) + ) { + state.agentFileDrafts = { ...state.agentFileDrafts, [name]: content }; + } + state.agentFilesError = null; + agents.recordFile(res); + return true; + } + } catch (err) { + if (isCurrent()) { + state.agentFilesError = formatUiError(err); + } + return false; + } finally { + if (isConnected()) { + if (saving) { + advanceWriteRevision(); + } + state[busy] = false; + } + } + return false; } -export async function loadAgentFileContent( +export function loadAgentFileContent( state: AgentFilesState, agentId: string, name: string, opts?: { force?: boolean }, ): Promise { - const client = state.client; - if (!client || !state.connected || state.agentFilesLoading) { - return false; - } - if (!opts?.force && Object.hasOwn(state.agentFileContents, name)) { - return true; - } - const generation = state.requestGeneration; - const isCurrent = () => - state.client === client && state.connected && state.requestGeneration === generation; - state.agentFilesLoading = true; - state.agentFilesError = null; - try { - const res = await client.request("agents.files.get", { - agentId, - name, - }); - if (res?.file && isCurrent()) { - const content = res.file.content ?? ""; - const previousBase = state.agentFileContents[name] ?? ""; - const currentDraft = state.agentFileDrafts[name]; - state.agentFilesList = mergeFileEntry(state.agentFilesList, res.file); - state.agentFileContents = { ...state.agentFileContents, [name]: content }; - if (!Object.hasOwn(state.agentFileDrafts, name) || currentDraft === previousBase) { - state.agentFileDrafts = { ...state.agentFileDrafts, [name]: content }; - } - return true; - } - } catch (err) { - if (isCurrent()) { - state.agentFilesError = formatUiError(err); - } - return false; - } finally { - if (isCurrent()) { - state.agentFilesLoading = false; - } - } - return false; + return requestAgentFile(state, agentId, name, { kind: "read", force: opts?.force }); } -export async function saveAgentFile( +export function saveAgentFile( state: AgentFilesState, agentId: string, name: string, content: string, ): Promise { - const client = state.client; - if (!client || !state.connected || state.agentFileSaving) { - return false; - } - const generation = state.requestGeneration; - const isCurrent = () => - state.client === client && state.connected && state.requestGeneration === generation; - state.agentFileSaving = true; - state.agentFilesError = null; - try { - const res = await client.request("agents.files.set", { - agentId, - name, - content, - }); - if (res?.file && isCurrent()) { - state.agentFilesList = mergeFileEntry(state.agentFilesList, res.file); - state.agentFileContents = { ...state.agentFileContents, [name]: content }; - // The response establishes the saved base, but must not discard text - // entered after this save started. - if (!Object.hasOwn(state.agentFileDrafts, name) || state.agentFileDrafts[name] === content) { - state.agentFileDrafts = { ...state.agentFileDrafts, [name]: content }; - } - return true; - } - } catch (err) { - if (isCurrent()) { - state.agentFilesError = formatUiError(err); - } - return false; - } finally { - if (isCurrent()) { - state.agentFileSaving = false; - } - } - return false; + return requestAgentFile(state, agentId, name, { kind: "write", content }); } diff --git a/ui/src/test-helpers/control-ui-e2e.deferral.test.ts b/ui/src/test-helpers/control-ui-e2e.deferral.test.ts new file mode 100644 index 000000000000..a3db8a412c80 --- /dev/null +++ b/ui/src/test-helpers/control-ui-e2e.deferral.test.ts @@ -0,0 +1,110 @@ +/* @vitest-environment jsdom */ +import { expect, it } from "vitest"; +import { createControlUiMockGatewayInitScript } from "./control-ui-e2e.ts"; + +type ResponseFrame = { + type: string; + id: string; + ok: boolean; + payload?: unknown; + error?: { message: string }; +}; + +it.each(["resolve", "reject"] as const)( + "%ss every held request without changing one-shot deferrals", + async (outcome) => { + const catalog = { environments: [], profiles: [{ id: "aws" }] }; + const scenario = { + heldMethods: ["environments.list"], + deferredMethods: ["models.list"], + methodResponses: { "environments.list": catalog }, + }; + const priorWebSocket = window.WebSocket; + window.sessionStorage.clear(); + // oxlint-disable-next-line typescript/no-implied-eval -- Exercise the serialized browser transport, including its closure boundary. + new Function(createControlUiMockGatewayInitScript(scenario))(); + const sockets = [ + new WebSocket("ws://mock-gateway/first"), + new WebSocket("ws://mock-gateway/second"), + ] as const; + const frames: ResponseFrame[] = []; + for (const socket of sockets) { + socket.addEventListener("message", (event) => { + const frame = JSON.parse(String((event as MessageEvent).data)) as ResponseFrame; + if (frame.type === "res") { + frames.push(frame); + } + }); + } + const flush = () => + new Promise((resolve) => { + setTimeout(resolve, 0); + }); + const send = (socket: WebSocket, id: string, method: string) => + socket.send(JSON.stringify({ type: "req", id, method })); + const gateway = ( + window as Window & { + openclawControlUiE2eGateway?: { + resolveDeferred: (method: string) => void; + rejectDeferred: (method: string, error: { message: string }) => void; + deferNext: (method: string) => void; + }; + } + ).openclawControlUiE2eGateway; + try { + if (!gateway) { + throw new Error("Mock Gateway was not installed"); + } + await flush(); + send(sockets[0], "catalog-first", "environments.list"); + send(sockets[1], "catalog-replacement", "environments.list"); + send(sockets[0], "model-held", "models.list"); + send(sockets[0], "model-next", "models.list"); + await flush(); + expect(frames.map((frame) => frame.id)).toEqual(["model-next"]); + + if (outcome === "resolve") { + gateway.resolveDeferred("environments.list"); + } else { + gateway.rejectDeferred("environments.list", { message: "catalog unavailable" }); + } + expect(frames.filter((frame) => frame.id.startsWith("catalog-"))).toEqual( + ["catalog-first", "catalog-replacement"].map((id) => + expect.objectContaining( + outcome === "resolve" + ? { id, ok: true, payload: catalog } + : { + id, + ok: false, + error: expect.objectContaining({ message: "catalog unavailable" }), + }, + ), + ), + ); + send(sockets[1], "catalog-after-release", "environments.list"); + await flush(); + expect(frames.at(-1)).toMatchObject({ + id: "catalog-after-release", + ok: true, + payload: catalog, + }); + gateway.resolveDeferred("models.list"); + expect(frames.at(-1)).toMatchObject({ id: "model-held", ok: true }); + + gateway.deferNext("environments.list"); + send(sockets[0], "catalog-one-shot", "environments.list"); + send(sockets[0], "catalog-unblocked", "environments.list"); + await flush(); + expect(frames.at(-1)).toMatchObject({ id: "catalog-unblocked", ok: true }); + expect(frames.some((frame) => frame.id === "catalog-one-shot")).toBe(false); + gateway.resolveDeferred("environments.list"); + expect(frames.at(-1)).toMatchObject({ id: "catalog-one-shot", ok: true }); + } finally { + for (const socket of sockets) { + socket.close(); + } + window.WebSocket = priorWebSocket; + window.sessionStorage.clear(); + } + }, +); diff --git a/ui/src/test-helpers/control-ui-e2e.ts b/ui/src/test-helpers/control-ui-e2e.ts index 2615abd45363..3933d76b54dd 100644 --- a/ui/src/test-helpers/control-ui-e2e.ts +++ b/ui/src/test-helpers/control-ui-e2e.ts @@ -275,6 +275,8 @@ export type ControlUiMockGatewayScenario = { featureCapabilities?: string[]; defaultAgentId?: string; deferredMethods?: string[]; + /** Hold every request until resolveDeferred/rejectDeferred releases the method. */ + heldMethods?: string[]; /** Non-release gateway checkout branch surfaced in the sidebar footer. */ devGitBranch?: string; /** Exact immutable Control UI artifact served by the mocked Gateway. */ @@ -884,6 +886,7 @@ function normalizeScenario( featureCapabilities: scenario.featureCapabilities ?? [], defaultAgentId, deferredMethods: scenario.deferredMethods ?? [], + heldMethods: scenario.heldMethods ?? [], devGitBranch: scenario.devGitBranch?.trim() || "", serverBuildId: scenario.serverBuildId?.trim() || "e2e", gatewayBootId: scenario.gatewayBootId?.trim() || "e2e-gateway-boot", @@ -1059,6 +1062,7 @@ function installControlUiMockGateway( // Opaque initial documents may not expose storage; the target page will. } const deferredMethods: DeferredMethod[] = scenario.deferredMethods.map((method) => ({ method })); + const heldMethods = new Set(scenario.heldMethods); const deferredResponses: DeferredResponse[] = []; const requests: BrowserRequest[] = []; const methodResponseSequenceIndexes = new Map(); @@ -2069,6 +2073,9 @@ function installControlUiMockGateway( } function shouldDefer(method: string, params: unknown): boolean { + if (heldMethods.has(method)) { + return true; + } const index = deferredMethods.findIndex( (candidate) => candidate.method === method && paramsMatch(params, candidate.match), ); @@ -2079,6 +2086,25 @@ function installControlUiMockGateway( return true; } + function takeDeferredResponses(method: string): DeferredResponse[] { + const index = deferredResponses.findIndex((response) => response.method === method); + if (index < 0) { + throw new Error(`No deferred mock Gateway response for ${method}`); + } + if (!heldMethods.delete(method)) { + return deferredResponses.splice(index, 1); + } + // Startup can replace a request when connection scope settles. A held + // catalog releases every admitted request, not only its retired predecessor. + const responses = deferredResponses.filter((response) => response.method === method); + for (let i = deferredResponses.length - 1; i >= 0; i -= 1) { + if (deferredResponses[i]?.method === method) { + deferredResponses.splice(i, 1); + } + } + return responses; + } + function parseFrame(raw: string | ArrayBufferLike | Blob | ArrayBufferView): BrowserFrame | null { if (typeof raw !== "string") { return null; @@ -2246,52 +2272,40 @@ function installControlUiMockGateway( return method ? requests.filter((request) => request.method === method) : [...requests]; }, rejectDeferred(method, error) { - const index = deferredResponses.findIndex((response) => response.method === method); - if (index < 0) { - throw new Error(`No deferred mock Gateway response for ${method}`); + for (const response of takeDeferredResponses(method)) { + response.socket.deliver({ + error: { + code: error?.code ?? "INVALID_REQUEST", + message: error?.message ?? "mock Gateway rejected request", + ...(error?.details ? { details: error.details } : {}), + ...(error?.retryable ? { retryable: true } : {}), + }, + id: response.id, + ok: false, + type: "res", + }); } - const [response] = deferredResponses.splice(index, 1); - if (!response) { - throw new Error(`Deferred mock Gateway response disappeared for ${method}`); - } - response.socket.deliver({ - error: { - code: error?.code ?? "INVALID_REQUEST", - message: error?.message ?? "mock Gateway rejected request", - ...(error?.details ? { details: error.details } : {}), - ...(error?.retryable ? { retryable: true } : {}), - }, - id: response.id, - ok: false, - type: "res", - }); }, requests, resolveDeferred(method, payload) { - const index = deferredResponses.findIndex((response) => response.method === method); - if (index < 0) { - throw new Error(`No deferred mock Gateway response for ${method}`); + for (const response of takeDeferredResponses(method)) { + const resolvedPayload = applyScenarioAgentModel( + response.method, + payload ?? buildResponse(response.method, response.params), + ); + if ( + response.method === "sessions.create" || + response.method === "sessions.catalog.continue" + ) { + recordMaterializedSession(response.params, resolvedPayload); + } + response.socket.deliver({ + id: response.id, + ok: true, + payload: resolvedPayload, + type: "res", + }); } - const [response] = deferredResponses.splice(index, 1); - if (!response) { - throw new Error(`Deferred mock Gateway response disappeared for ${method}`); - } - const resolvedPayload = applyScenarioAgentModel( - response.method, - payload ?? buildResponse(response.method, response.params), - ); - if ( - response.method === "sessions.create" || - response.method === "sessions.catalog.continue" - ) { - recordMaterializedSession(response.params, resolvedPayload); - } - response.socket.deliver({ - id: response.id, - ok: true, - payload: resolvedPayload, - type: "res", - }); }, setOnline(nextOnline) { online = nextOnline;