refactor(canvas): delegate publish composition to the desktop-fs canvas action

adboio · adboio · commit e9b12cda4015 · 2026-07-20T15:55:22.000-04:00
canvas_publish now calls the desktop-fs canvas action, which owns version composition server-side, passing the checkout's version as expected_current_version_id so a stale publish is rejected atomically (409 version_conflict) instead of guarded by a non-atomic tool-side check-then-PATCH. Deletes the tool-side compose logic and the raw meta PATCH; backends predating the guard field ignore it and publish unguarded, so the tool degrades gracefully. Server side: PostHog/posthog#72365. Generated-By: PostHog Code Task-Id: 2b3d176e-3023-41d2-92e8-81eda7c12a8c
diff --git a/packages/agent/src/adapters/local-tools/tools/canvas.test.ts b/packages/agent/src/adapters/local-tools/tools/canvas.test.ts
@@ -1,10 +1,9 @@
 import { describe, expect, it } from "vitest";
-import {
-  canvasScratchDir,
-  canvasScratchFile,
-  composePublishedMeta,
-} from "./canvas";
+import { canvasScratchDir, canvasScratchFile } from "./canvas";
 
+// Version composition and the stale-base rejection live server-side in the
+// desktop-fs canvas action (and are tested there); the tool's own logic is
+// the scratch-file plumbing.
 describe("canvas scratch paths", () => {
   it("keys the scratch dir and file by canvas id, outside any workspace", () => {
     expect(canvasScratchDir("dash-1")).toBe("/tmp/posthog-canvas/dash-1");
@@ -13,120 +12,3 @@ describe("canvas scratch paths", () => {
     );
   });
 });
-
-describe("composePublishedMeta", () => {
-  const v1 = { id: "v1", code: "one", createdAt: 1 };
-  const v2 = { id: "v2", code: "two", createdAt: 2 };
-  const v3 = { id: "v3", code: "three", createdAt: 3 };
-
-  it("appends a new head version when the base matches", () => {
-    const result = composePublishedMeta({
-      freshMeta: { code: "two", versions: [v1, v2], currentVersionId: "v2" },
-      baseVersionId: "v2",
-      code: "edited",
-      prompt: "tweak the chart",
-      now: 10,
-    });
-    expect(result.ok).toBe(true);
-    if (!result.ok) return;
-    expect(result.meta.code).toBe("edited");
-    expect(result.meta.currentVersionId).toBe(result.versionId);
-    expect(result.meta.versions).toHaveLength(3);
-    expect(result.meta.versions?.at(-1)).toMatchObject({
-      code: "edited",
-      prompt: "tweak the chart",
-      createdAt: 10,
-    });
-    expect(result.meta.updatedAt).toBe(10);
-  });
-
-  it("preserves unrelated meta keys", () => {
-    const result = composePublishedMeta({
-      freshMeta: {
-        code: "one",
-        versions: [v1],
-        currentVersionId: "v1",
-        templateId: "freeform",
-        pinnedAt: 123,
-      },
-      baseVersionId: "v1",
-      code: "edited",
-      now: 10,
-    });
-    expect(result.ok).toBe(true);
-    if (!result.ok) return;
-    expect(result.meta.templateId).toBe("freeform");
-    expect(result.meta.pinnedAt).toBe(123);
-  });
-
-  it("rejects when the canvas moved past the base (concurrent edit)", () => {
-    const result = composePublishedMeta({
-      freshMeta: {
-        code: "three",
-        versions: [v1, v2, v3],
-        currentVersionId: "v3",
-      },
-      baseVersionId: "v2",
-      code: "edited",
-      now: 10,
-    });
-    expect(result.ok).toBe(false);
-  });
-
-  it("rejects a based publish onto a canvas with no versions", () => {
-    const result = composePublishedMeta({
-      freshMeta: {},
-      baseVersionId: "v1",
-      code: "edited",
-      now: 10,
-    });
-    expect(result.ok).toBe(false);
-  });
-
-  it("truncates the redo tail when publishing from an undone version", () => {
-    // The user undid to v1 (pointer mid-history), then the agent edited from
-    // that checkout: the redo tail (v2, v3) is discarded — the same
-    // linear-discard the client's undo/redo uses.
-    const result = composePublishedMeta({
-      freshMeta: {
-        code: "one",
-        versions: [v1, v2, v3],
-        currentVersionId: "v1",
-      },
-      baseVersionId: "v1",
-      code: "edited",
-      now: 10,
-    });
-    expect(result.ok).toBe(true);
-    if (!result.ok) return;
-    expect(result.meta.versions?.map((v) => v.id)).toEqual([
-      "v1",
-      result.versionId,
-    ]);
-  });
-
-  it("seeds an empty canvas as the first version (no base)", () => {
-    const result = composePublishedMeta({
-      freshMeta: { templateId: "freeform" },
-      baseVersionId: undefined,
-      code: "first build",
-      now: 10,
-    });
-    expect(result.ok).toBe(true);
-    if (!result.ok) return;
-    expect(result.meta.versions).toHaveLength(1);
-    expect(result.meta.currentVersionId).toBe(result.versionId);
-    expect(result.meta.code).toBe("first build");
-  });
-
-  it("rejects an un-based publish onto a canvas that gained versions", () => {
-    // Checked out empty, but a concurrent first build published meanwhile.
-    const result = composePublishedMeta({
-      freshMeta: { code: "one", versions: [v1], currentVersionId: "v1" },
-      baseVersionId: undefined,
-      code: "edited",
-      now: 10,
-    });
-    expect(result.ok).toBe(false);
-  });
-});
diff --git a/packages/agent/src/adapters/local-tools/tools/canvas.ts b/packages/agent/src/adapters/local-tools/tools/canvas.ts
@@ -1,8 +1,10 @@
-import { randomUUID } from "node:crypto";
 import { mkdirSync, readFileSync, writeFileSync } from "node:fs";
 import * as path from "node:path";
 import { z } from "zod";
-import { PostHogAPIClient } from "../../../posthog-api";
+import {
+  DesktopCanvasVersionConflictError,
+  PostHogAPIClient,
+} from "../../../posthog-api";
 import { resolveSandboxPosthogApi } from "../../../signed-commit-artefacts";
 import { defineLocalTool, type LocalToolResult } from "../registry";
 
@@ -15,15 +17,11 @@ import { defineLocalTool, type LocalToolResult } from "../registry";
  *   deterministic scratch path tool-side (no model transcription), and stashes
  *   the fetched `currentVersionId` as the publish-time concurrency base.
  * - `canvas_publish` reads the scratch file from disk (again, no
- *   transcription), verifies the canvas hasn't moved past the stashed base
- *   (a concurrent edit or user undo), appends a full-file version snapshot,
- *   and PATCHes the merged meta — mirroring `dashboardsService.saveFreeform`
- *   in `@posthog/core`, including the linear-discard of any redo tail.
- *
- * The check-then-PATCH guard is tool-side and therefore not atomic; it shrinks
- * the clobber window from "the whole generation turn" to milliseconds. A
- * server-side `base_version` check on the desktop-fs PATCH remains the
- * follow-up that closes it completely.
+ *   transcription) and publishes through the desktop-fs canvas action, which
+ *   owns version composition server-side. The stashed base rides along as
+ *   `expected_current_version_id`, so a publish based on a stale read (a
+ *   concurrent edit, or a user's undo) is rejected atomically with a version
+ *   conflict instead of clobbering the newer head.
  *
  * Credentials come from the sandbox environment (see
  * `resolveSandboxPosthogApi`), so this works identically from the Claude
@@ -47,19 +45,9 @@ function baseVersionMarkerFile(canvasId: string): string {
   return path.join(canvasScratchDir(canvasId), ".base-version.json");
 }
 
-interface CanvasVersion {
-  id: string;
-  code: string;
-  context?: string;
-  prompt?: string;
-  createdAt: number;
-}
-
 interface CanvasMeta {
   code?: string;
-  versions?: CanvasVersion[];
   currentVersionId?: string;
-  updatedAt?: number;
   [key: string]: unknown;
 }
 
@@ -104,17 +92,6 @@ async function fetchCanvasEntry(canvasId: string): Promise<CanvasFsEntry> {
   return entry;
 }
 
-async function patchCanvasMeta(
-  canvasId: string,
-  meta: CanvasMeta,
-): Promise<void> {
-  const client = createClient();
-  if (!client) {
-    throw new Error("No PostHog credentials available in this session.");
-  }
-  await client.patchDesktopFsEntryMeta(canvasId, meta);
-}
-
 function readMarker(canvasId: string): BaseVersionMarker | undefined {
   try {
     return JSON.parse(
@@ -130,47 +107,6 @@ function writeMarker(canvasId: string, marker: BaseVersionMarker): void {
   writeFileSync(baseVersionMarkerFile(canvasId), JSON.stringify(marker));
 }
 
-/**
- * Compose the published meta from a freshly-fetched entry: verify the base,
- * truncate any redo tail past the current pointer (linear-discard, matching
- * the client's undo semantics in `freeformSchemas.ts`), and append the new
- * full-file snapshot. Pure — exported for tests.
- */
-export function composePublishedMeta(input: {
-  freshMeta: CanvasMeta;
-  baseVersionId: string | undefined;
-  code: string;
-  prompt?: string;
-  now: number;
-}): { ok: true; meta: CanvasMeta; versionId: string } | { ok: false } {
-  const { freshMeta, baseVersionId, code, prompt, now } = input;
-  if ((freshMeta.currentVersionId ?? undefined) !== baseVersionId) {
-    return { ok: false };
-  }
-  const versions = freshMeta.versions ?? [];
-  const pointer = baseVersionId
-    ? versions.findIndex((v) => v.id === baseVersionId)
-    : -1;
-  const kept = pointer >= 0 ? versions.slice(0, pointer + 1) : [];
-  const version: CanvasVersion = {
-    id: randomUUID(),
-    code,
-    ...(prompt ? { prompt } : {}),
-    createdAt: now,
-  };
-  return {
-    ok: true,
-    versionId: version.id,
-    meta: {
-      ...freshMeta,
-      code,
-      versions: [...kept, version],
-      currentVersionId: version.id,
-      updatedAt: now,
-    },
-  };
-}
-
 function errorResult(text: string): LocalToolResult {
   return { content: [{ type: "text", text }], isError: true };
 }
@@ -221,8 +157,8 @@ export const canvasCheckoutTool = defineLocalTool({
 export const canvasPublishTool = defineLocalTool({
   name: "canvas_publish",
   description:
-    "Publish the checked-out canvas: reads the scratch file written by canvas_checkout, verifies the " +
-    "canvas hasn't changed since checkout, and saves the file as the canvas's new live version. Call " +
+    "Publish the checked-out canvas: reads the scratch file written by canvas_checkout and saves it as " +
+    "the canvas's new live version, guarded against the canvas having changed since checkout. Call " +
     "exactly once when the edit is complete. On a version-conflict error, re-run canvas_checkout, " +
     "re-apply your edits, and publish again.",
   schema: {
@@ -256,36 +192,38 @@ export const canvasPublishTool = defineLocalTool({
         `canvas_publish failed: no checkout record for canvas "${args.id}". Call canvas_checkout first.`,
       );
     }
+    const client = createClient();
+    if (!client) {
+      return errorResult(
+        "canvas_publish failed: no PostHog credentials available in this session.",
+      );
+    }
     try {
-      const fresh = await fetchCanvasEntry(args.id);
-      const composed = composePublishedMeta({
-        freshMeta: fresh.meta ?? {},
-        baseVersionId: marker.versionId,
+      const entry = await client.publishDesktopCanvas<CanvasFsEntry>(args.id, {
         code,
         prompt: args.prompt,
-        now: Date.now(),
+        expectedCurrentVersionId: marker.versionId ?? null,
       });
-      if (!composed.ok) {
-        return errorResult(
-          `canvas_publish failed: ${CONFLICT_MESSAGE(args.id)}`,
-        );
-      }
-      await patchCanvasMeta(args.id, composed.meta);
       // Advance the base so a follow-up publish in the same session works
       // without a re-checkout.
-      writeMarker(args.id, {
-        versionId: composed.versionId,
-        fetchedAt: Date.now(),
-      });
+      const newVersionId = entry.meta?.currentVersionId;
+      writeMarker(args.id, { versionId: newVersionId, fetchedAt: Date.now() });
       return {
         content: [
           {
             type: "text",
-            text: `Published canvas "${args.id}" (new version ${composed.versionId}). The canvas is live; do not paste the code into chat.`,
+            text: `Published canvas "${args.id}"${
+              newVersionId ? ` (new version ${newVersionId})` : ""
+            }. The canvas is live; do not paste the code into chat.`,
           },
         ],
       };
     } catch (err) {
+      if (err instanceof DesktopCanvasVersionConflictError) {
+        return errorResult(
+          `canvas_publish failed: ${CONFLICT_MESSAGE(args.id)}`,
+        );
+      }
       return errorResult(
         `canvas_publish failed: ${err instanceof Error ? err.message : String(err)}`,
       );
diff --git a/packages/agent/src/posthog-api.ts b/packages/agent/src/posthog-api.ts
@@ -63,6 +63,14 @@ export type TaskRunUpdate = Partial<
   state_remove_keys?: string[];
 };
 
+/** A guarded canvas publish was based on a stale version (409 from the API). */
+export class DesktopCanvasVersionConflictError extends Error {
+  constructor(readonly currentVersionId: string | null) {
+    super("Canvas version conflict: the canvas changed since it was read.");
+    this.name = "DesktopCanvasVersionConflictError";
+  }
+}
+
 export class PostHogAPIClient {
   private config: PostHogAPIConfig;
 
@@ -357,22 +365,50 @@ export class PostHogAPIClient {
   }
 
   /**
-   * PATCH a desktop file system entry's meta blob (the endpoint stores the
-   * sent meta verbatim, so callers merge into the previously-fetched meta —
-   * the same read-modify-write contract as `dashboardsService` in core).
+   * Publish a freeform canvas's source via the desktop-fs canvas action. The
+   * server owns version composition (appends the full-file snapshot, moves
+   * `currentVersionId`, truncates any redo tail) and rejects a publish whose
+   * `expectedCurrentVersionId` no longer matches the live head with a 409 —
+   * surfaced as DesktopCanvasVersionConflictError. Backends predating the
+   * guard ignore the field and publish unguarded, so this degrades gracefully.
    */
-  async patchDesktopFsEntryMeta<T>(
+  async publishDesktopCanvas<T>(
     entryId: string,
-    meta: Record<string, unknown>,
+    input: {
+      code: string;
+      prompt?: string;
+      /** The head version the code was based on; null when it was empty. */
+      expectedCurrentVersionId: string | null;
+    },
   ): Promise<T> {
     const teamId = this.getTeamId();
-    return this.apiRequest<T>(
-      `/api/projects/${teamId}/desktop_file_system/${encodeURIComponent(entryId)}/`,
-      {
-        method: "PATCH",
-        body: JSON.stringify({ meta }),
-      },
+    const body: Record<string, unknown> = {
+      code: input.code,
+      expected_current_version_id: input.expectedCurrentVersionId,
+    };
+    if (input.prompt) {
+      body.prompt = input.prompt;
+    }
+    const response = await this.performRequestWithRetry(
+      `/api/projects/${teamId}/desktop_file_system/${encodeURIComponent(entryId)}/canvas/`,
+      { method: "PATCH", body: JSON.stringify(body) },
     );
+    if (response.status === 409) {
+      let currentVersionId: string | null = null;
+      try {
+        const parsed = (await response.json()) as {
+          current_version_id?: string | null;
+        };
+        currentVersionId = parsed.current_version_id ?? null;
+      } catch {
+        // Conflict body unavailable — the status alone carries the signal.
+      }
+      throw new DesktopCanvasVersionConflictError(currentVersionId);
+    }
+    if (!response.ok) {
+      throw new Error(`Failed to publish canvas (${response.status})`);
+    }
+    return response.json() as Promise<T>;
   }
 
   /** Signal reports the given task is associated with (via report task associations). */
diff --git a/packages/ui/src/features/canvas/AGENTS.md b/packages/ui/src/features/canvas/AGENTS.md
