Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/one-message-on-render-failure.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@sapiom/harness": patch
"@sapiom/harness-desktop": patch
---

The canvas Render-failed state shows one message instead of two drawn on top of each other. The app's card and the rendered document both painted the failure reason, and the card is a transparent layer over the document, so the short reason and the long one overlapped and neither was readable. The document now stands its prose down while it is embedded, the same way it already hides its title, badge and legend as chrome the app draws instead. Opened on its own, or embedded somewhere that never takes the message over, the document keeps its prose and is still the only message, so a failure never ends as an empty board.
4 changes: 4 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,7 @@ Thumbs.db

# Subagent-driven-development scratch (ledger, briefs, review packages) — never committed
.superpowers/

# Throwaway verification probes, never committed.
packages/harness/.probe*.mjs
packages/harness/.shot.mjs
177 changes: 176 additions & 1 deletion packages/harness/e2e/canvas-render.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
* `file://` — not `setContent` — so the document's own inline theme script runs
* against `location.search`, exactly as the SPA's iframe loads it.
*/
import { test, expect } from "@playwright/test";
import { test, expect, type FrameLocator, type Page } from "@playwright/test";
import * as fs from "node:fs/promises";
import * as os from "node:os";
import * as path from "node:path";
Expand Down Expand Up @@ -128,6 +128,181 @@ test("a workflow with no extractable definition renders an honest error panel, n
expect(errors).toEqual([]);
});

/**
* SAP-3199. Embedded, the SPA draws its own Render-failed card (short claim,
* one-line reason, actions, full reason behind Details) as a TRANSPARENT layer
* over this document. So the composed view has exactly one error message only if
* the document stands its own prose down while it is framed. It used to keep
* painting, and the short reason and the long one drew through each other.
*
* The parent here is a bare file:// shell rather than the real SPA: it loads the
* render the same way (an `allow-scripts`-only sandboxed iframe) and listens on
* the same channel, which is all the document can observe. Asserting against the
* real app is the screenshot pass on the PR, not this spec.
*/
async function embedInParentShell(page: Page, url: string, file: string): Promise<FrameLocator> {
const parentFile = path.join(path.dirname(file), "workbench-shell.html");
await fs.writeFile(
parentFile,
`<!doctype html><html><body style="margin:0">
<script>
window.__posted = [];
addEventListener("message", function (e) {
if (e.data && e.data.type === "sapiom-canvas:error") window.__posted.push(e.data);
});
</script>
<iframe id="board" sandbox="allow-scripts" src="${url}" style="width:900px;height:600px;border:0"></iframe>
</body></html>`,
"utf8",
);
await page.goto(pathToFileURL(parentFile).href);
return page.frameLocator("#board");
}

/** The opening of the failed panel's prose, which is the string that used to
* draw through the app's card. */
const REASON_TEXT = /Could not extract this agent's step graph/;

test("an embedded failed render shows one error message, not two drawn through each other", async ({
page,
}) => {
const errors: string[] = [];
page.on("pageerror", (err) => errors.push(String(err)));

const { url, file } = await renderToFileUrl(NO_DEFINITION, "no-definition");
const frame = await embedInParentShell(page, url, file);

// The reason reaches the workbench exactly once, which is the one message the
// user is shown, and the SPA renders it as the card.
await expect
.poll(() => page.evaluate(() => (window as unknown as { __posted: unknown[] }).__posted.length))
.toBe(1);
const posted = await page.evaluate(
() => (window as unknown as { __posted: { title: string; reason: string }[] }).__posted[0],
);
expect(posted.title).toBe("no-definition");
expect(posted.reason.length).toBeGreaterThan(10);

// …and the document under the card paints no second copy of it. Restore the
// overlap (drop the class or the stylesheet rule) and this is the assertion
// that goes red.
await expect(frame.locator(".canvas-render-error-note")).toBeAttached();
await expect(frame.getByText(REASON_TEXT)).toBeHidden();
// Nothing else in the document paints a message either. Every text-bearing
// element is walked rather than the two we know about, so a THIRD string added
// to the failed panel later cannot quietly reintroduce the overlap.
//
// "Paints" is hit-tested rather than inferred from the box, because the title
// and the "render failed" badge would otherwise count: they live in
// `.canvas-header`, which the template collapses to a clipped 1px box, and the
// children inside it keep their full-size rects even though the parent clips
// every pixel of them away. Asking the document what is actually at the
// element's centre point is the question a reader asks.
const { painted, offscreen } = await frame.locator("body").evaluate((body) => {
const painted: string[] = [];
const offscreen: string[] = [];
for (const el of Array.from(body.querySelectorAll<HTMLElement>("p, h1, span, div"))) {
const own = Array.from(el.childNodes)
.filter((n) => n.nodeType === Node.TEXT_NODE)
.map((n) => (n.textContent || "").trim())
.join(" ")
.trim();
if (!own) continue;
const box = el.getBoundingClientRect();
if (box.width === 0 && box.height === 0) continue; // display:none paints nothing
// A box wholly outside the frame is reported separately rather than
// counted clear: `elementFromPoint` answers null out there, so a second
// message below the fold would otherwise pass as "not painted".
if (box.right <= 0 || box.bottom <= 0 || box.left >= innerWidth || box.top >= innerHeight) {
offscreen.push(own);
continue;
}
// Probe inside the element AND inside the frame, for a box that straddles
// the edge.
const x = Math.min(Math.max(box.left + box.width / 2, 1), innerWidth - 1);
const y = Math.min(Math.max(box.top + box.height / 2, 1), innerHeight - 1);
const hit = document.elementFromPoint(x, y);
if (hit && (hit === el || el.contains(hit))) painted.push(own);
}
return { painted, offscreen };
});
expect(painted).toEqual([]);
expect(offscreen).toEqual([]);
expect(errors).toEqual([]);
});

/**
* The other side of the flag, and the one with no message of its own to lose.
* `EMBED_SCRIPT` ships in every canvas document, not just the failed one, and a
* healthy board never posts an error, so it never marks `data-canvas-error-posted`
* and the head script withdraws `data-canvas-embedded` at load. This asserts what
* that withdrawal may not disturb: the title, badges and legend are app chrome the
* SPA draws in its overview panel, and the board must stay ONLY the graph
* (canvas-template.ts, the rule above `.canvas-header`).
*/
test("an embedded successful render still hides its header and legend", async ({ page }) => {
const errors: string[] = [];
page.on("pageerror", (err) => errors.push(String(err)));

const { url, file } = await renderToFileUrl(ORDER_TRIAGE, "order-triage");
const frame = await embedInParentShell(page, url, file);

// The graph is the board, and it is drawn.
await expect(frame.locator("rect.canvas-node-rect").first()).toBeVisible();

// The chrome is not. These are clipped by an ungated rule, so the flag being
// withdrawn here (nothing posted, nothing to lose) cannot bring them back.
for (const chrome of [".canvas-header", ".canvas-legend"]) {
const el = frame.locator(chrome).first();
await expect(el).toBeAttached();
const box = await el.boundingBox();
expect(box?.width ?? 0).toBeLessThanOrEqual(1);
expect(box?.height ?? 0).toBeLessThanOrEqual(1);
}
// The title text is in the markup and paints nothing.
await expect(frame.locator(".canvas-title")).not.toBeInViewport();

// And the withdrawal did happen, so this is not passing because the flag stuck.
const flag = await frame.locator(":root").getAttribute("data-canvas-embedded");
expect(flag).toBeNull();
expect(errors).toEqual([]);
});

test("an embedded failed render whose payload will not parse keeps its own prose", async ({
page,
}) => {
const { url, file } = await renderToFileUrl(NO_DEFINITION, "no-definition");

// Corrupt the payload the boot script reads. A hand-authored document can
// carry a malformed `#sapiom-render-error` block, and the renderer's own
// classes are a documented contract for those. Nothing posts, so the SPA
// shows no card, so the document's prose has to come back: hiding it here
// would leave an empty board and no message at all, which is worse than the
// overlap this pair of specs exists to prevent.
const html = await fs.readFile(file, "utf8");
const corrupt = html.replace(
/(<script type="application\/json" id="sapiom-render-error">)[^<]*/,
"$1{ not json",
);
expect(corrupt).not.toBe(html);
await fs.writeFile(file, corrupt, "utf8");

const frame = await embedInParentShell(page, url, file);

await expect(frame.getByText(REASON_TEXT)).toBeVisible();
const posted = await page.evaluate(() => (window as unknown as { __posted: unknown[] }).__posted.length);
expect(posted).toBe(0);
});

test("a failed render opened on its own keeps its prose as the only message", async ({ page }) => {
const { url } = await renderToFileUrl(NO_DEFINITION, "no-definition");
await page.goto(url);

// No parent, so no card is coming, and the document must still say what happened.
await expect(page.getByText(REASON_TEXT)).toBeVisible();
expect(await page.getByText(REASON_TEXT).count()).toBe(1);
});

test("launched sub-workflows paint as their own dashed nodes with a launch edge", async ({ page }) => {
const { url } = await renderToFileUrl(HUB, "hub");
await page.goto(url);
Expand Down
11 changes: 9 additions & 2 deletions packages/harness/src/core/canvas-body.ts
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,14 @@ ${renderGraphSvg(graph, enrichment)}

/** A degraded panel for a workflow whose graph couldn't be extracted — never
* a crash, never a silent fallback to the LLM path, just an honest reason
* styled through the same shell. */
* styled through the same shell.
*
* The prose carries `canvas-render-error-note` so the template can hide it
* while the document is embedded: the SPA paints its own Render-failed card
* (short claim, one-line reason, actions, full reason behind Details)
* transparently over this document, and two error strings drawn through each
* other was SAP-3199. Opened standalone there is no card, so the prose stays
* as the only message. */
export function buildErrorPanelHtml(title: string, reason: string): string {
const errorData = JSON.stringify({ title, reason }).replace(/</g, "\\u003c");
return `<section class="canvas-panel">
Expand All @@ -216,7 +223,7 @@ export function buildErrorPanelHtml(title: string, reason: string): string {
</div>
</header>
<div class="canvas-diagram-panel">
<p class="canvas-empty-note">Could not extract this agent's step graph: ${esc(reason)}. Use the workbench actions to ask your coding agent to fix it or retry the deterministic render.</p>
<p class="canvas-empty-note canvas-render-error-note">Could not extract this agent's step graph: ${esc(reason)}. Use the workbench actions to ask your coding agent to fix it or retry the deterministic render.</p>
</div>
<script type="application/json" id="sapiom-render-error">${errorData}</script>
</section>`;
Expand Down
9 changes: 9 additions & 0 deletions packages/harness/src/core/canvas-render.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,15 @@ describe("renderCanvasForSession", () => {
expect(html).toContain("render failed");
expect(html).toContain("Could not extract this agent's step graph");
expect(html).toContain('id="sapiom-render-error"');
// Embedded, the SPA's Render-failed card is the one message; the document's
// prose steps aside instead of drawing through it (SAP-3199).
expect(html).toContain('class="canvas-empty-note canvas-render-error-note"');
expect(html).toContain(":root[data-canvas-embedded] .canvas-render-error-note { display: none; }");
expect(html).toContain('root.setAttribute("data-canvas-embedded", "")');
// The flag is withdrawn again if nothing takes the message over, so a
// document that cannot post is never left blank (SAP-3199 review round 1).
expect(html).toContain('root.removeAttribute("data-canvas-embedded")');
expect(html).toContain('document.documentElement.setAttribute("data-canvas-error-posted", "")');
expect(html).toContain('"title":"broken-flow"');
expect(html).toContain("sapiom-canvas:error");
expect(html).not.toContain('class="canvas-node '); // no diagram — just the note
Expand Down
8 changes: 8 additions & 0 deletions packages/harness/src/core/canvas-run-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,13 @@ export function bootCanvasOverview(): void {
* panel embeds `{ title, reason }` in `#sapiom-render-error`; without this
* bridge the app's actionable error overlay can never appear and the iframe
* is left showing only static prose.
*
* A successful post marks the root `data-canvas-error-posted`. That marker is
* what lets the head script withdraw its optimistic `data-canvas-embedded`
* flag at load time, so the in-document prose only stands down when something
* has actually stood up in its place (SAP-3199). Every path that does NOT post
* leaves the marker unset, which is why the `catch` below can still promise to
* keep the panel visible.
*/
export function bootCanvasError(): void {
function post(): void {
Expand All @@ -408,6 +415,7 @@ export function bootCanvasError(): void {
},
"*",
);
document.documentElement.setAttribute("data-canvas-error-posted", "");
} catch {
/* malformed payload — keep the in-document error panel visible */
}
Expand Down
46 changes: 44 additions & 2 deletions packages/harness/src/core/canvas-template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,14 @@ body {
position: absolute; width: 1px; height: 1px; overflow: hidden;
clip-path: inset(50%); white-space: nowrap; margin: 0; padding: 0;
}
/* Same reason as the header above, for the failed render. The SPA paints its
own Render-failed card (short claim, one-line reason, actions, full reason
behind Details) as a transparent layer directly over this document, so the
document's copy of the reason drew straight through it (SAP-3199). Embedded,
the card is the one message and this prose steps aside; opened standalone,
or embedded in a frame that never took the message over, it stays and is the
only message. See EMBED_SCRIPT for how the flag is withdrawn in that case. */
:root[data-canvas-embedded] .canvas-render-error-note { display: none; }
.canvas-interconnections { display: flex; flex-direction: column; gap: 12px; }
.canvas-panel-title { margin: 0; font-size: 12px; text-transform: uppercase; letter-spacing: 0.06em; color: var(--canvas-text-dim); }
.canvas-interconnection-row { display: grid; grid-template-columns: 12px 1fr auto; column-gap: 8px; row-gap: 2px; align-items: baseline; }
Expand Down Expand Up @@ -260,9 +268,40 @@ template { display: none; }
`.trim();
}

/** Marks the document as embedded so the stylesheet can stand down the chrome
* the SPA already draws around the iframe. In the head, so the flag is on the
* root element before the body paints and the prose never flashes.
*
* The flag is OPTIMISTIC, and withdrawn if it turns out to be wrong. The prose
* may only stand down if something stands up in its place, and the thing that
* stands up is the SPA's card, which only appears if `bootCanvasError` posts
* the reason to the parent. That runs from the much larger run-state script,
* which can fail to post (a hand-authored document with a malformed
* `#sapiom-render-error` payload, or no payload at all) or abort as a whole
* before it is ever called. So `bootCanvasError` marks a successful post, and
* at load, with the DOM and every deferred boot done, an unmarked document
* takes its flag back and shows its own prose. Hiding it in that case would
* leave an empty board and no message anywhere, which is worse than the
* overlap SAP-3199 fixed.
*
* `window.parent` is readable from a sandboxed frame and comparing the two
* references is not a cross-origin access, so this is safe under the
* `allow-scripts`-only sandbox the SPA loads the board with. */
const EMBED_SCRIPT = `
(function () {
if (window.parent === window) return;
var root = document.documentElement;
root.setAttribute("data-canvas-embedded", "");
function withdrawUnlessTaken() {
if (!root.hasAttribute("data-canvas-error-posted")) root.removeAttribute("data-canvas-embedded");
}
if (document.readyState === "complete") withdrawUnlessTaken();
else window.addEventListener("load", withdrawUnlessTaken);
})();
`.trim();

/** Reads the current theme from `?theme=light|dark`, falling back to the
* Studio's light product default when the param is absent — the only script
* in the whole document. */
* Studio's light product default when the param is absent. */
const THEME_SCRIPT = `
(function () {
var params = new URLSearchParams(location.search);
Expand Down Expand Up @@ -400,6 +439,9 @@ export function renderCanvasDocument(bodyHtml: string): string {
<meta name="viewport" content="width=device-width, initial-scale=1" />
<title>Agent Studio canvas</title>
<script>
${EMBED_SCRIPT}
</script>
<script>
${THEME_SCRIPT}
</script>
<script>
Expand Down
9 changes: 6 additions & 3 deletions packages/harness/web/src/styles.css
Original file line number Diff line number Diff line change
Expand Up @@ -6218,9 +6218,12 @@ button.system-graph-node.is-navigable:focus-visible {
box-shadow: var(--shadow-sm);
}

/* Render-failure card, app-side (the document's own error text is hidden by
the board snippet): short claim, one-line reason, direct actions, full
reason folded behind Details. Sits on the board's dotted grid. */
/* Render-failure card, app-side: short claim, one-line reason, direct actions,
full reason folded behind Details. Sits transparently on the board's dotted
grid, so the document underneath must not paint a message of its own. The
canvas template hides the failed panel's prose while it is embedded
(`[data-canvas-embedded] .canvas-render-error-note`, canvas-template.ts).
Without that the two error strings drew through each other (SAP-3199). */
.canvas-render-error {
position: absolute;
inset: 0;
Expand Down
Loading