From b845a285e774cb3208cf6bbef03df2c57357709c Mon Sep 17 00:00:00 2001
From: Ashish Huddar <ashish.hudar@gmail.com>
Date: Sat, 9 May 2026 23:41:47 +0530
Subject: [PATCH] fix(canvas): duplicate canvas ids must keep newest payload
 (greptile P1)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

readCanvases sorts file canvases newest-first by updatedAt. The
canvases route then merged them via Map.set in iteration order, which
overwrites earlier (newer) entries with later (older) ones. If an
agent wrote `result-v1.json` and `result-v2.json` with the same canvas
id, the user would see the OLDER payload — the exact opposite of the
intended last-write-wins behavior.

Fix: extract the merge into a pure helper (merge.ts) with explicit
first-write-wins semantics. Synthesized seeds the map first (preserves
the pass-9 reservation that core- prefix wins on collision); file
canvases skip ids already present, so the newest wins.

Adds 5 regression tests covering: empty input, synth + file mix,
duplicate-id newest-wins (the regression), synth-vs-file collision,
and final sort order.

Also adds a "Why no HTML canvas type?" section to the feature writeup
HTML — the most-asked design question, answered honestly so reviewers
don't have to ask. Covers the trust-hierarchy-inversion argument
(agents are lowest-trust + LLMs are prompt-injectable + same-origin
HTML = supervisor JS execution), the leaky-sanitizer argument, and
the legitimate sandboxed-iframe path for v0.5+.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 docs/canvases-feature.html                    | 41 +++++++++++-
 .../[id]/canvases/__tests__/merge.test.ts     | 63 +++++++++++++++++++
 .../app/api/sessions/[id]/canvases/merge.ts   | 30 +++++++++
 .../app/api/sessions/[id]/canvases/route.ts   | 10 +--
 4 files changed, 135 insertions(+), 9 deletions(-)
 create mode 100644 packages/web/src/app/api/sessions/[id]/canvases/__tests__/merge.test.ts
 create mode 100644 packages/web/src/app/api/sessions/[id]/canvases/merge.ts

diff --git a/docs/canvases-feature.html b/docs/canvases-feature.html
index 5ca3aae4d..5334a0eb3 100644
--- a/docs/canvases-feature.html
+++ b/docs/canvases-feature.html
@@ -214,6 +214,7 @@
     <li><a href="#files">File map</a></li>
     <li><a href="#try">Try it locally</a></li>
     <li><a href="#roadmap">Roadmap</a></li>
+    <li><a href="#why-no-html">Why no <code>html</code> canvas type?</a></li>
     <li><a href="#non-goals">Deliberate non-goals</a></li>
   </ol>
 </nav>
@@ -623,7 +624,45 @@ EOF</code></pre>
   </tbody>
 </table>
 
-<h2 id="non-goals">10. Deliberate non-goals</h2>
+<h2 id="why-no-html">10. Why no <code>html</code> canvas type?</h2>
+<p>The most-asked design question — answered honestly here so reviewers don't have to ask.</p>
+<p>Two reasons, both load-bearing.</p>
+
+<h3>Trust hierarchy inversion</h3>
+<p>Agents are the lowest-trust surface in the system. They emit content on every poll cycle, and large parts of that content come from LLMs that can be prompt-injected. If an agent ingests a malicious URL, summarizes a poisoned document, or just hits a jailbreak, its next "render this nice summary" call could include attacker-controlled HTML. Putting that HTML into the same-origin dashboard means the attacker now has:</p>
+<ul>
+  <li>Read access to every same-origin auth token / cookie / <code>localStorage</code> value.</li>
+  <li>Permission to call every API the user can call (kill sessions, restore them, modify config).</li>
+  <li>The ability to inject scripts that survive across navigation.</li>
+  <li>Keylogging on every page the dashboard renders.</li>
+</ul>
+<p>That's the same blast radius v0.4 renderer plugins have — but renderer plugins clear <code>npm install</code> review <em>once</em>. An agent emits a fresh canvas every poll. The trust gap is enormous.</p>
+
+<h3>HTML sanitization is a leaky abstraction</h3>
+<p>DOMPurify, sanitize-html, etc. are honest attempts but every year ships new bypasses — <code>&lt;svg&gt;</code> foreign objects, <code>&lt;math&gt;</code> payloads, mutation-XSS via clipboard, CSS exfiltration via <code>attr()</code>. A sanitizer-based HTML canvas is a 90% solution that fails open on the 10%, and the 10% is "execute arbitrary JS in your supervisor". Even GitHub, GitLab, and Slack treat HTML rendering as a multi-quarter security investment, not a feature you slot in next to <code>markdown</code>.</p>
+
+<h3>What the existing types cover</h3>
+<table>
+  <thead><tr><th>You want to render</th><th>Use</th></tr></thead>
+  <tbody>
+    <tr><td>Formatted text with bold / italic / headings / code / lists</td><td><code>markdown</code> (the v0.1 parser handles all of this safely — no HTML pass-through)</td></tr>
+    <tr><td>Code changes</td><td><code>diff</code></td></tr>
+    <tr><td>Structured rows</td><td><code>table</code></td></tr>
+    <tr><td>Metric cards</td><td><code>stats</code></td></tr>
+    <tr><td>Custom UI (flame graph, Gantt, network diagram)</td><td><code>canvas-renderer</code> plugin (v0.4)</td></tr>
+    <tr><td>Genuinely arbitrary, untrusted markup</td><td><strong>Sandboxed iframe canvas</strong> — credible v0.5+ option, deferred</td></tr>
+  </tbody>
+</table>
+
+<h3>The legitimate "I need raw HTML" path — sandboxed iframe</h3>
+<p>An iframe with <code>sandbox="allow-scripts"</code> (no <code>allow-same-origin</code>) runs in a null origin — it can't reach your auth tokens or call APIs. Add a <code>postMessage</code> protocol for size negotiation and limited event-out, document the trust trade-off explicitly, and you've got a real HTML surface that's safe by construction.</p>
+<p>But — per codex's pass-13 advice — <strong>half-baked iframe support is worse than no sandbox</strong>. Sandbox protocols have to be designed end-to-end (CSP headers, postMessage validation, focus management, accessibility, viewport sizing) before they ship. v0.4 renderer plugins is the bigger unlock; the iframe canvas comes after, only if a real use case shows up.</p>
+
+<div class="callout">
+  <strong>Bottom line</strong> — not because we couldn't, but because direct HTML inverts the trust hierarchy (agents shouldn't be able to JS-execute in the supervisor) and sanitizers don't fix that. The plugin path covers most "I need a custom widget" cases at v0.4. Sandboxed iframe is the answer for genuine arbitrary HTML when we're ready to design the protocol properly.
+</div>
+
+<h2 id="non-goals">11. Deliberate non-goals</h2>
 <ul>
   <li><strong>Remote-loaded JS at runtime.</strong> No URL-fetched code, no agent-emitted React, no on-the-fly module fetching. v0.4 plugins are <em>discovered</em> at AO startup from <code>node_modules</code> (Node loads each plugin's schema entrypoint) and <em>renderers</em> are bundled at web build time via a generated barrel of static imports. That's the OSS extensibility line: trust at install, not at runtime.</li>
   <li><strong>Write APIs from the dashboard back into canvases.</strong> Read-only surface in v0.1. Bidirectional adds a whole new failure mode (auth, ordering, conflict).</li>
diff --git a/packages/web/src/app/api/sessions/[id]/canvases/__tests__/merge.test.ts b/packages/web/src/app/api/sessions/[id]/canvases/__tests__/merge.test.ts
new file mode 100644
index 000000000..5a00f1364
--- /dev/null
+++ b/packages/web/src/app/api/sessions/[id]/canvases/__tests__/merge.test.ts
@@ -0,0 +1,63 @@
+import { describe, it, expect } from "vitest";
+import type { CanvasArtifact } from "@aoagents/ao-core";
+import { mergeCanvases } from "../merge";
+
+function md(id: string, updatedAt: string, body: string): CanvasArtifact {
+  return {
+    version: 1,
+    id,
+    type: "markdown",
+    title: id,
+    createdAt: updatedAt,
+    updatedAt,
+    payload: { markdown: body },
+  };
+}
+
+describe("mergeCanvases", () => {
+  it("returns empty when nothing to merge", () => {
+    expect(mergeCanvases(null, [])).toEqual([]);
+  });
+
+  it("includes synthesized canvas alongside file canvases", () => {
+    const synth = md("core-git-diff", "2026-05-06T10:00:00Z", "diff");
+    const file = md("notes", "2026-05-06T09:00:00Z", "notes");
+    const out = mergeCanvases(synth, [file]);
+    expect(out.map((c) => c.id)).toEqual(["core-git-diff", "notes"]);
+  });
+
+  it("on duplicate ids, keeps the NEWEST file entry (regression: PR #1653 pass-14 P1)", () => {
+    // readCanvases sorts newest-first; merge must respect that.
+    // If two files emit the same id, the user must see the newer payload.
+    const newer = md("test-results", "2026-05-06T10:00:00Z", "v2 (newer)");
+    const older = md("test-results", "2026-05-06T08:00:00Z", "v1 (older)");
+    const out = mergeCanvases(null, [newer, older]); // newest-first input
+    expect(out).toHaveLength(1);
+    expect(out[0]).toMatchObject({ id: "test-results" });
+    if (out[0]?.type === "markdown") {
+      expect(out[0].payload.markdown).toBe("v2 (newer)");
+    } else {
+      throw new Error("expected markdown canvas");
+    }
+  });
+
+  it("synthesized wins on id collision with a file canvas", () => {
+    // Even though `core-` is reserved at the reader, the merge layer also
+    // needs to resolve collisions deterministically in favor of synthesized.
+    const synth = md("core-git-diff", "2026-05-06T08:00:00Z", "synthesized");
+    const fake = md("core-git-diff", "2026-05-06T10:00:00Z", "imposter");
+    const out = mergeCanvases(synth, [fake]);
+    expect(out).toHaveLength(1);
+    if (out[0]?.type === "markdown") {
+      expect(out[0].payload.markdown).toBe("synthesized");
+    }
+  });
+
+  it("output is sorted newest-first by updatedAt", () => {
+    const a = md("a", "2026-05-06T08:00:00Z", "");
+    const b = md("b", "2026-05-06T10:00:00Z", "");
+    const c = md("c", "2026-05-06T09:00:00Z", "");
+    const out = mergeCanvases(null, [b, c, a]); // arbitrary input order
+    expect(out.map((x) => x.id)).toEqual(["b", "c", "a"]);
+  });
+});
diff --git a/packages/web/src/app/api/sessions/[id]/canvases/merge.ts b/packages/web/src/app/api/sessions/[id]/canvases/merge.ts
new file mode 100644
index 000000000..157ea7184
--- /dev/null
+++ b/packages/web/src/app/api/sessions/[id]/canvases/merge.ts
@@ -0,0 +1,30 @@
+import type { CanvasArtifact } from "@aoagents/ao-core";
+
+/**
+ * Merge synthesized + file canvases for the canvases endpoint.
+ *
+ * Semantics:
+ *   1. Synthesized canvas (e.g. `core-git-diff`) seeds the map first. The
+ *      `core-` prefix is reserved at the reader, so a legitimate file canvas
+ *      can't collide; if collision happens anyway the trusted synthesized
+ *      version wins by being inserted first.
+ *   2. `fileCanvases` MUST be sorted newest-first by `updatedAt`. The first
+ *      occurrence of each id is kept; subsequent duplicates are skipped, NOT
+ *      used to overwrite.
+ *
+ * The naive `Map.set` loop did the opposite (older payload won) — see PR #1653
+ * pass-14 review.
+ *
+ * Final output is sorted by `updatedAt` descending (newest first).
+ */
+export function mergeCanvases(
+  synthesized: CanvasArtifact | null,
+  fileCanvases: CanvasArtifact[],
+): CanvasArtifact[] {
+  const merged = new Map<string, CanvasArtifact>();
+  if (synthesized) merged.set(synthesized.id, synthesized);
+  for (const c of fileCanvases) {
+    if (!merged.has(c.id)) merged.set(c.id, c);
+  }
+  return Array.from(merged.values()).sort((a, b) => (a.updatedAt < b.updatedAt ? 1 : -1));
+}
diff --git a/packages/web/src/app/api/sessions/[id]/canvases/route.ts b/packages/web/src/app/api/sessions/[id]/canvases/route.ts
index 035761e7f..0d36521b4 100644
--- a/packages/web/src/app/api/sessions/[id]/canvases/route.ts
+++ b/packages/web/src/app/api/sessions/[id]/canvases/route.ts
@@ -5,7 +5,6 @@ import {
   readCanvases,
   synthesizeGitDiffCanvas,
   SessionNotFoundError,
-  type CanvasArtifact,
 } from "@aoagents/ao-core";
 import {
   getCorrelationId,
@@ -13,6 +12,7 @@ import {
   recordApiObservation,
   resolveProjectIdForSessionId,
 } from "@/lib/observability";
+import { mergeCanvases } from "./merge";
 
 /** GET /api/sessions/:id/canvases — List canvas artifacts for a session */
 export async function GET(_request: NextRequest, { params }: { params: Promise<{ id: string }> }) {
@@ -47,13 +47,7 @@ export async function GET(_request: NextRequest, { params }: { params: Promise<{
         ])
       : [[], null];
 
-    const merged = new Map<string, CanvasArtifact>();
-    if (synthesized) merged.set(synthesized.id, synthesized);
-    for (const c of fileCanvases) merged.set(c.id, c);
-
-    const canvases = Array.from(merged.values()).sort((a, b) =>
-      a.updatedAt < b.updatedAt ? 1 : -1,
-    );
+    const canvases = mergeCanvases(synthesized, fileCanvases);
 
     recordApiObservation({
       config,