feat(canvas): edit canvases as scratch files via canvas_checkout / canvas_publish#3594
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
|
👋 Visual changes detected for this PR. Review and approve in PostHog Visual Review If these changes are unexpected, they may be caused by a flaky test or a broken snapshot on master. Don't approve — rerun the job or wait for a fix. |
3c61527 to
40df541
Compare
46f300a to
5815c04
Compare
40df541 to
fefc6d6
Compare
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
fefc6d6 to
525012d
Compare
3b038cd to
f73be5a
Compare
525012d to
061c5de
Compare
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 5 should fix, 2 consider. Published 7 findings (view the review). |
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
There was a problem hiding this comment.
ReviewHog Report
Feature
Issues: 5 issues
Files (6)
packages/agent/src/adapters/local-tools/tools/canvas.tspackages/agent/src/posthog-api.tspackages/agent/src/adapters/claude/permissions/permission-handlers.tspackages/agent/src/adapters/local-tools/index.tspackages/agent/src/adapters/local-tools/registry.tspackages/agent/src/adapters/local-tools/tools/speak.ts
What were the main changes
- New canvas_checkout / canvas_publish local tools: checkout fetches (or creates) a canvas and writes its source to a deterministic tool-side scratch path plus a base-version marker; publish reads the scratch file, calls the desktop-fs canvas action with the recorded expected_current_version_id, and refreshes the marker on success
- PostHogAPIClient gains getDesktopFsEntry, createDesktopCanvas, and publishDesktopCanvas, with a 409 response surfaced as a typed DesktopCanvasVersionConflictError
- New autoApprove flag on LocalTool/LocalToolDef; local-tools/index.ts derives AUTO_APPROVED_LOCAL_TOOL_IDS from it and flags canvas_checkout and speak
- Claude permission handler generalizes the old single-purpose speak-tool bypass into a generic auto-approve check against AUTO_APPROVED_LOCAL_TOOL_IDS (do_not_use block still wins)
Feature
Issues: 2 issues
Files (4)
packages/ui/src/features/canvas/freeformPrompt.tspackages/core/src/canvas/freeformSchemas.tspackages/ui/src/features/canvas/AGENTS.mdpackages/workspace-server/src/services/agent/agent.ts
What were the main changes
- freeformPrompt.ts now routes canvas generation through canvas_checkout \u2192 targeted file edits \u2192 canvas_publish instead of embedding the full current source and publishing via the raw desktop-file-system-canvas-partial-update MCP call
- Starter-scaffold and fresh-build instructions reworked to write/iterate on the checked-out scratch file rather than requiring a full-file rewrite each turn
- workspace-server agent.ts adds a "## Canvases" system-prompt section directing generic coding-agent tasks to use canvas_checkout/canvas_publish instead of the raw PostHog MCP tool
- Docs (AGENTS.md, freeformSchemas.ts comment) updated to describe the new scratch-file workflow and the publish-time version-conflict guard
Other findings (outside the changed lines)
Valid issues on this PR's files that sit on lines GitHub won't let us comment on inline.
New "## Canvases" guardrail is scoped to channelMode even though canvas_checkout/canvas_publish are available in every task
Priority: consider | File: packages/workspace-server/src/services/agent/agent.ts:651-684 | Category: best_practice
Why we think it's a valid issue
- Checked: the
isEnabledgates on the canvas tools vs. the channel-only tools, the placement of the new "## Canvases" block, howchannelModeis derived, and the PR's own comments about intended usage. - Found:
canvasCheckoutTool/canvasPublishToolgate on credentials only —isEnabled: () => resolveSandboxPosthogApi() !== undefined(canvas.ts:190, :238) — with nochannelModecheck, whereasclone_repo(clone-repo.ts:42) andlist_repos(list-repos.ts:47) useisEnabled: (_ctx, meta) => meta?.channelMode === true. registry.ts confirms a single MCP server hosts every tool for all tasks, with per-toolisEnableddeciding visibility, so the canvas tools are indeed visible in non-channel (repo-attached) tasks. - Found: the "## Canvases" guidance (agent.ts:679-683, which tells the agent to never publish canvas source through the raw PostHog MCP) sits entirely inside
if (channelMode)(agent.ts:651).channelModeis true only when the workspace resolves to a scratch path (agent.ts:761). So the scope of the guidance and the availability of the tools genuinely disagree. - Found: the PR itself designs for the non-channel case — canvas.ts:98-100 comments that create-if-missing exists so "an agent working from a normal task (not the channel generate bar) can start a canvas in one call." This makes the guidance gap a real design inconsistency the authors anticipated, not a speculative edge case.
- Impact: in a repo-attached task where the agent does canvas work, it receives no prompt guardrail steering it to
canvas_checkout/canvas_publishand away from the rawdesktop-file-system-canvas-partial-updatepath, so the scratch-file workflow and version-conflict guard this PR establishes aren't reinforced there. The fix is trivial (move the block out of thechannelModebranch, alongside the always-added "## Attribution"/"## Shell efficiency" sections). - Priority: lowering to
consider. The impact is bounded: non-channel canvas work is an uncommon path; the raw-MCP fallback degrades to today's already-shipped behavior (not a regression); and the tool descriptions already soft-nudge toward checkout ("Always start canvas work with this tool", canvas.ts:166-167). Real and worth broadening, but milder than a pre-mergeshould_fix.
Issue description
The new "## Canvases" system-prompt section (lines 677-683) is appended only inside if (channelMode) { ... }, which fires when the task's workspace resolves to a scratch directory (i.e. allowNoRepo/no-repo, "channel" tasks). But canvas_checkout/canvas_publish (packages/agent/src/adapters/local-tools/tools/canvas.ts) are registered unconditionally — their isEnabled check only tests resolveSandboxPosthogApi() !== undefined, with no channelMode/meta.channelMode gate (unlike list_repos/clone_repo, which explicitly check meta?.channelMode === true). So a normal repo-attached coding task (channelMode === false) has full access to both the local canvas tools AND the raw PostHog MCP tool desktop-file-system-canvas-partial-update, but never receives the instruction added here telling it to "never publish canvas source through the raw PostHog MCP." The tools' own descriptions (canvas.ts:161-167, :223-227) explain the scratch-file mechanics and nudge toward using them ("Always start canvas work with this tool"), but neither description names or prohibits the raw MCP tool — that prohibition exists solely in the gated block. If an agent working a repo-attached task decides mid-task to create/edit a canvas (e.g. "also add a dashboard summarizing this"), it has no textual guardrail against falling back to the old raw-MCP full-file-rewrite path this PR is trying to eliminate, defeating the version-conflict guard and scratch-file workflow the rest of the PR establishes.
Suggested fix
Move the "## Canvases" block out of the if (channelMode) branch so it's appended unconditionally (alongside the always-added "## Attribution"/"## Shell efficiency" sections above it), or gate it on the same signal that actually controls tool availability (credential presence) rather than channelMode. If canvas tools are genuinely meant to be channel-task-only, the fix should instead be on the isEnabled checks in canvas.ts to match list_repos/clone_repo's meta?.channelMode === true pattern — but as written, the instruction's scope and the tools' actual availability disagree.
| // Create-if-missing for `canvas_checkout`: an agent working from a normal task | ||
| // (not the channel generate bar) can start a canvas in one call instead of | ||
| // chaining the raw desktop-fs create tool first. | ||
| async function createCanvasEntry( | ||
| name: string | undefined, | ||
| parentPath: string | undefined, | ||
| ): Promise<CanvasFsEntry> { | ||
| if (!name?.trim()) { | ||
| throw new Error( | ||
| "pass `id` to edit an existing canvas, or `name` to create a new one.", | ||
| ); | ||
| } | ||
| const client = createClient(); | ||
| if (!client) { | ||
| throw new Error("No PostHog credentials available in this session."); | ||
| } | ||
| return client.createDesktopCanvas<CanvasFsEntry>({ | ||
| name: name.trim(), | ||
| parentPath, | ||
| }); | ||
| } |
There was a problem hiding this comment.
canvas_checkout's create-if-missing branch has no idempotency or existing-canvas check, so repeated name-based calls silently create duplicate canvases
Why we think it's a valid issue
- Checked: the only in-repo driver of
canvas_checkout(freeformPrompt.tsviauseGenerateFreeformCanvas.ts), the create path (canvas.ts:101-118,191-195),createDesktopCanvas(posthog-api.ts:372-390), and the auto-approve wiring (canvas.test.ts:17-30). - Found:
buildFreeformGenerationPrompt(freeformPrompt.ts:54-57) always emitscanvas_checkoutwithid "${dashboardId}"; the row is created upstream inuseGenerateFreeformCanvas.tsand only itsdashboardIdis threaded into the prompt. So the generate-bar flow — the sole current caller — never reaches thenamebranch. That branch is a forward-looking convenience (commentcanvas.ts:98-100) reachable only when some future agent-initiated task callscanvas_checkout({name})without anid. - Found: the non-idempotency premise holds client-side —
createDesktopCanvas(posthog-api.ts:379-385) POSTs{ path, type: 'dashboard' }with no existing-entry lookup, and the checkout response (canvas.ts:206-211) carries no duplicate warning. Server-side path-uniqueness fordesktop_file_systemcannot be verified from this repo (lives in the Django backend), so 'silently duplicates' is plausible but unconfirmed. Note the branch also makes an auto-approved (autoApprove: true,canvas.ts:189) tool perform an unguarded write, amplifying the lack of a human gate. - Impact: a real idempotency gap in a newly-added write path, but the trigger is speculative (a future flow plus agent context loss re-invoking with the same
name) and the worst outcome is a duplicate canvas row — recoverable, not data loss or corruption of existing content, and not reachable by any current code path. - Priority: the observation is genuine (not overengineering/style/mistaken premise), so it stays on record, but with no current caller, a speculative trigger, and low blast radius it does not meet the
should_fixbar — down-ranked toconsider.
Issue description
createCanvasEntry (canvas.ts:101-118) is invoked whenever canvas_checkout is called without id (canvas.ts:193-195), and it unconditionally calls client.createDesktopCanvas (posthog-api.ts:372-390), which POSTs a brand-new dashboard row with no check for whether a canvas already exists at that computed path/name. The tool's own description says to "Always start canvas work with this tool," and a caller working from a normal (non-generate-bar) task that doesn't persist the returned id between turns — e.g. it drops out of context, or the agent simply calls canvas_checkout({name: ...}) again to "start" canvas work a second time in the same or a later turn — has no way to detect this is a repeat and will silently create a second, unrelated dashboard row with the same name. Nothing in the response (canvas.ts:206-211) warns the agent this could be a duplicate, and there is no server-side or client-side dedup by path.
Suggested fix
Before creating, look up whether an entry already exists at the target parentPath/name (or require the caller to have first attempted a lookup) and reuse it instead of blindly creating; alternatively, have the checkout response for a freshly-created canvas explicitly warn the agent to remember and reuse the returned id for all subsequent canvas_checkout/canvas_publish calls in this task, so it never re-supplies name once an id exists.
Prompt to fix with AI (copy-paste)
## Context
@packages/agent/src/adapters/local-tools/tools/canvas.ts#L98-118
<issue_description>
`createCanvasEntry` (`canvas.ts:101-118`) is invoked whenever `canvas_checkout` is called without `id` (`canvas.ts:193-195`), and it unconditionally calls `client.createDesktopCanvas` (`posthog-api.ts:372-390`), which POSTs a brand-new `dashboard` row with no check for whether a canvas already exists at that computed `path`/`name`. The tool's own description says to "Always start canvas work with this tool," and a caller working from a normal (non-generate-bar) task that doesn't persist the returned `id` between turns — e.g. it drops out of context, or the agent simply calls `canvas_checkout({name: ...})` again to "start" canvas work a second time in the same or a later turn — has no way to detect this is a repeat and will silently create a second, unrelated `dashboard` row with the same name. Nothing in the response (`canvas.ts:206-211`) warns the agent this could be a duplicate, and there is no server-side or client-side dedup by path.
</issue_description>
<issue_validation>
- **Checked:** the only in-repo driver of `canvas_checkout` (`freeformPrompt.ts` via `useGenerateFreeformCanvas.ts`), the create path (`canvas.ts:101-118`, `191-195`), `createDesktopCanvas` (`posthog-api.ts:372-390`), and the auto-approve wiring (`canvas.test.ts:17-30`).
- **Found:** `buildFreeformGenerationPrompt` (`freeformPrompt.ts:54-57`) always emits `canvas_checkout` with `id "${dashboardId}"`; the row is created upstream in `useGenerateFreeformCanvas.ts` and only its `dashboardId` is threaded into the prompt. So the generate-bar flow — the sole current caller — never reaches the `name` branch. That branch is a forward-looking convenience (comment `canvas.ts:98-100`) reachable only when some future agent-initiated task calls `canvas_checkout({name})` without an `id`.
- **Found:** the non-idempotency premise holds client-side — `createDesktopCanvas` (`posthog-api.ts:379-385`) POSTs `{ path, type: 'dashboard' }` with no existing-entry lookup, and the checkout response (`canvas.ts:206-211`) carries no duplicate warning. Server-side path-uniqueness for `desktop_file_system` cannot be verified from this repo (lives in the Django backend), so 'silently duplicates' is plausible but unconfirmed. Note the branch also makes an auto-approved (`autoApprove: true`, `canvas.ts:189`) tool perform an unguarded write, amplifying the lack of a human gate.
- **Impact:** a real idempotency gap in a newly-added write path, but the trigger is speculative (a future flow plus agent context loss re-invoking with the same `name`) and the worst outcome is a duplicate canvas row — recoverable, not data loss or corruption of existing content, and not reachable by any current code path.
- **Priority:** the observation is genuine (not overengineering/style/mistaken premise), so it stays on record, but with no current caller, a speculative trigger, and low blast radius it does not meet the `should_fix` bar — down-ranked to `consider`.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Before creating, look up whether an entry already exists at the target `parentPath`/`name` (or require the caller to have first attempted a lookup) and reuse it instead of blindly creating; alternatively, have the checkout response for a freshly-created canvas explicitly warn the agent to remember and reuse the returned `id` for all subsequent `canvas_checkout`/`canvas_publish` calls in this task, so it never re-supplies `name` once an `id` exists.
</potential_solution>
| schema: { | ||
| id: z.string().describe("The canvas (desktop-fs dashboard row) id."), |
There was a problem hiding this comment.
Unsanitized id in canvas_publish enables filesystem path traversal outside the scratch root
Why we think it's a valid issue
- Checked:
canvas_publishschema/handler (canvas.ts:228-295), the path builderscanvasScratchFile/canvasScratchDir/baseVersionMarkerFile(canvas.ts:38-48), the citedparseGithubUrlprecedent (clone-repo.ts:52-60), andpath.join's normalization behavior. - Found:
id: z.string()(canvas.ts:229) has no format constraint and flows raw intopath.join(CANVAS_SCRATCH_ROOT, args.id, …)viareadFileSync(line 242),readMarker(line 253), andwriteMarker(line 274). I confirmed empirically thatpath.join("/tmp/posthog-canvas", "../../etc")→/etc, so..segments do escape the root.canvas_checkoutis genuinely safe by contrast because its path uses server-returnedentry.id(canvas.ts:196-200), andclone-repo.ts:52-60guards this exact class withparseGithubUrl— so escaping raw path components is a recognized concern in this codebase. - Impact: Real path-traversal weakness with a trivial one-regex fix, so worth keeping. But reachability is narrower than the finding states: the read path always appends
"canvas.tsx"(canvas.ts:42-43), limiting reads to files namedcanvas.tsx(the issue's.ssh/id_rsaexample wouldn't actually read a key); exfiltration additionally requires a co-located.base-version.jsonmarker orreadMarkererrors out first (canvas.ts:253-257); and the only raw-id-keyed write iswriteMarker(line 274) writing benign JSON to.base-version.json, reached only after a full successful publish. There is no arbitrary-content write to an arbitrary path. - Priority: Lowered to
should_fix— the "read arbitrary file and exfiltrate to the backend" framing overstates the live impact (fixedcanvas.tsxfilename, required co-located marker, benign write content), so this is a latent traversal/hygiene gap worth fixing rather than a blocking exfiltration primitive.
Issue description
canvas_publish's schema declares id: z.string() with no format constraint, and this raw, tool-call-controlled string flows unsanitized into canvasScratchFile(args.id) / canvasScratchDir(args.id) (line 242, 253, 274), which build a filesystem path via path.join(CANVAS_SCRATCH_ROOT, canvasId, ...) (canvas.ts:38-44). path.join normalizes .. segments rather than blocking them (path.join("/tmp/posthog-canvas", "../../etc") resolves to /etc), so a crafted id such as "../../../../home/user/.ssh" lets readFileSync (line 242) and mkdirSync/writeFileSync in writeMarker (line 274, called via canvas.ts:145-148) operate outside the intended /tmp/posthog-canvas sandbox. Because args.id here is whatever the LLM agent decides to pass — including under prompt-injection influence from untrusted content it has read — this is a real read/write path-traversal primitive: a matching canvas.tsx/.base-version.json file anywhere on the filesystem can be read and then exfiltrated to the PostHog backend via client.publishDesktopCanvas, or written to, under the guise of a canvas.
By contrast, canvas_checkout's scratch path is safe because it's built from the server-returned entry.id (a backend-issued UUID resolved via fetchCanvasEntry/createCanvasEntry), never from raw args.id directly — canvas_publish has no equivalent indirection or validation. This exact class of risk already has a precedent fix in this codebase: clone-repo.ts calls parseGithubUrl(repo) before its own path.join, with a comment noting it "normalizes away path traversal (a crafted URL can't escape the scratch workspace via the path.join below)" — canvas_publish skips the equivalent step for id.
Suggested fix
Validate args.id against the known canvas id shape before any filesystem use — e.g. add a Zod .regex(/^[0-9a-fA-F-]{36}$/) (matching the backend's UUID id) to the id field in canvas_publish's schema (and ideally canvas_checkout's id field too, defensively), or at minimum reject path separators and .. with .regex(/^[^/\\]+$/). Alternatively, resolve the joined path with path.resolve and assert it still starts with CANVAS_SCRATCH_ROOT before touching the filesystem, mirroring the parseGithubUrl guard in clone-repo.ts.
Prompt to fix with AI (copy-paste)
## Context
@packages/agent/src/adapters/local-tools/tools/canvas.ts#L228-229
@packages/agent/src/adapters/local-tools/tools/canvas.ts#L242
@packages/agent/src/adapters/local-tools/tools/canvas.ts#L253
@packages/agent/src/adapters/local-tools/tools/canvas.ts#L274
<issue_description>
`canvas_publish`'s schema declares `id: z.string()` with no format constraint, and this raw, tool-call-controlled string flows unsanitized into `canvasScratchFile(args.id)` / `canvasScratchDir(args.id)` (line 242, 253, 274), which build a filesystem path via `path.join(CANVAS_SCRATCH_ROOT, canvasId, ...)` (canvas.ts:38-44). `path.join` normalizes `..` segments rather than blocking them (`path.join("/tmp/posthog-canvas", "../../etc")` resolves to `/etc`), so a crafted `id` such as `"../../../../home/user/.ssh"` lets `readFileSync` (line 242) and `mkdirSync`/`writeFileSync` in `writeMarker` (line 274, called via canvas.ts:145-148) operate outside the intended `/tmp/posthog-canvas` sandbox. Because `args.id` here is whatever the LLM agent decides to pass — including under prompt-injection influence from untrusted content it has read — this is a real read/write path-traversal primitive: a matching `canvas.tsx`/`.base-version.json` file anywhere on the filesystem can be read and then exfiltrated to the PostHog backend via `client.publishDesktopCanvas`, or written to, under the guise of a canvas.
By contrast, `canvas_checkout`'s scratch path is safe because it's built from the server-returned `entry.id` (a backend-issued UUID resolved via `fetchCanvasEntry`/`createCanvasEntry`), never from raw `args.id` directly — `canvas_publish` has no equivalent indirection or validation. This exact class of risk already has a precedent fix in this codebase: `clone-repo.ts` calls `parseGithubUrl(repo)` before its own `path.join`, with a comment noting it "normalizes away path traversal (a crafted URL can't escape the scratch workspace via the path.join below)" — `canvas_publish` skips the equivalent step for `id`.
</issue_description>
<issue_validation>
- **Checked:** `canvas_publish` schema/handler (canvas.ts:228-295), the path builders `canvasScratchFile`/`canvasScratchDir`/`baseVersionMarkerFile` (canvas.ts:38-48), the cited `parseGithubUrl` precedent (clone-repo.ts:52-60), and `path.join`'s normalization behavior.
- **Found:** `id: z.string()` (canvas.ts:229) has no format constraint and flows raw into `path.join(CANVAS_SCRATCH_ROOT, args.id, …)` via `readFileSync` (line 242), `readMarker` (line 253), and `writeMarker` (line 274). I confirmed empirically that `path.join("/tmp/posthog-canvas", "../../etc")` → `/etc`, so `..` segments do escape the root. `canvas_checkout` is genuinely safe by contrast because its path uses server-returned `entry.id` (canvas.ts:196-200), and `clone-repo.ts:52-60` guards this exact class with `parseGithubUrl` — so escaping raw path components is a recognized concern in this codebase.
- **Impact:** Real path-traversal weakness with a trivial one-regex fix, so worth keeping. But reachability is narrower than the finding states: the read path always appends `"canvas.tsx"` (canvas.ts:42-43), limiting reads to files named `canvas.tsx` (the issue's `.ssh/id_rsa` example wouldn't actually read a key); exfiltration additionally requires a co-located `.base-version.json` marker or `readMarker` errors out first (canvas.ts:253-257); and the only raw-`id`-keyed write is `writeMarker` (line 274) writing benign JSON to `.base-version.json`, reached only after a full successful publish. There is no arbitrary-content write to an arbitrary path.
- **Priority:** Lowered to `should_fix` — the "read arbitrary file and exfiltrate to the backend" framing overstates the live impact (fixed `canvas.tsx` filename, required co-located marker, benign write content), so this is a latent traversal/hygiene gap worth fixing rather than a blocking exfiltration primitive.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Validate `args.id` against the known canvas id shape before any filesystem use — e.g. add a Zod `.regex(/^[0-9a-fA-F-]{36}$/)` (matching the backend's UUID `id`) to the `id` field in `canvas_publish`'s schema (and ideally `canvas_checkout`'s `id` field too, defensively), or at minimum reject path separators and `..` with `.regex(/^[^/\\]+$/)`. Alternatively, resolve the joined path with `path.resolve` and assert it still starts with `CANVAS_SCRATCH_ROOT` before touching the filesystem, mirroring the `parseGithubUrl` guard in `clone-repo.ts`.
</potential_solution>
| ), | ||
| }, | ||
| alwaysLoad: true, | ||
| autoApprove: true, |
There was a problem hiding this comment.
canvas_checkout's autoApprove:true bypasses all permission modes (including plan mode) for a mutating create-canvas call
Why we think it's a valid issue
- Checked:
canvas_checkout'sautoApproveflag and create path (canvas.ts:189, 193-195, 101-118), the auto-approve set derivation (index.ts:41-45), thecanUseToolordering (permission-handlers.ts:731-834), and the mutation itself (posthog-api.ts:372-390). - Found: The premise is accurate. permission-handlers.ts:764 returns
{behavior: "allow"}unconditionally with nosession.permissionModecheck, inside themcp__block (751-796) that returns before the plan-mode deny gate at line 826 — socanvas_checkoutis allowed in every mode. And itsname-only branch is genuinely mutating:createDesktopCanvasPOSTs to/api/projects/:teamId/desktop_file_system/creating a new dashboard row (posthog-api.ts:379-385), contradicting theautoApprove"read-only or fire-and-forget" contract (registry.ts:57-60). This is newly introduced by generalizing the prior speak-only bypass. - Impact: Real permission/contract gap: a mutating backend create runs with no prompt even in plan mode, whose whole purpose (per the comment at permission-handlers.ts:822-825) is to guarantee no side effects. Worth surfacing. Note
canvas_publish(mutating an existing live version) is correctly NOT auto-approved, so only the create branch leaks. - Priority: Lowered to
should_fix— the consequence is bounded: an empty, reversible canvas created in the user's own project via their own credentials, with no data loss, cross-tenant/IDOR exposure, or privilege escalation. That is below the exploitable/destructive thresholdmust_fiximplies, though the plan-mode contract violation makes it a legitimate fix.
Issue description
canvas_checkout is flagged autoApprove: true (line 189), but its handler is not purely read-only: when args.id is omitted and args.name is supplied, it calls createCanvasEntry (lines 193-195 → canvas.ts:101-118), which issues a POST to /api/projects/:teamId/desktop_file_system/ and creates a new backend dashboard/canvas resource. The autoApprove flag's own contract, documented at registry.ts:56-61 ("Auto-approve this tool's calls without a permission prompt — for read-only or fire-and-forget tools where prompting is pure friction") and restated in the permission handler's comment (permission-handlers.ts:761-763, "read-only or fire-and-forget (canvas checkout, speak narration)"), explicitly frames this bypass as safe only for non-mutating tools — canvas_checkout's create-branch violates that invariant.
In canUseTool (permission-handlers.ts:731-835), the AUTO_APPROVED_LOCAL_TOOL_IDS.has(toolName) check (line 764) returns {behavior: "allow"} unconditionally, with no session.permissionMode check, and runs before the plan-mode deny-by-default gate later in the function and before the explicit session.permissionMode === "auto"-style checks used elsewhere in the same file for other mutating/destructive actions. This means an agent in a restrictive mode (e.g. "plan", where the user expects zero side effects) can still silently create a new canvas via canvas_checkout({name: ...}) with no user confirmation at all — a genuine authorization-gap regression introduced by generalizing the old speak-only bypass to cover this tool.
Suggested fix
Either split checkout into a read-only lookup path (kept autoApprove) and a separate create path that requires normal approval, or drop autoApprove from canvas_checkout entirely so any create-if-missing call goes through the standard permission prompt like canvas_publish does. If the intent is genuinely to allow silent creation, that should be explicitly gated behind session.permissionMode (excluding at least plan/read-only modes) rather than bypassing all modes unconditionally.
Prompt to fix with AI (copy-paste)
## Context
@packages/agent/src/adapters/local-tools/tools/canvas.ts#L189
@packages/agent/src/adapters/local-tools/tools/canvas.ts#L193-195
<issue_description>
`canvas_checkout` is flagged `autoApprove: true` (line 189), but its handler is not purely read-only: when `args.id` is omitted and `args.name` is supplied, it calls `createCanvasEntry` (lines 193-195 → canvas.ts:101-118), which issues a POST to `/api/projects/:teamId/desktop_file_system/` and creates a new backend dashboard/canvas resource. The `autoApprove` flag's own contract, documented at `registry.ts:56-61` ("Auto-approve this tool's calls without a permission prompt — for read-only or fire-and-forget tools where prompting is pure friction") and restated in the permission handler's comment (`permission-handlers.ts:761-763`, "read-only or fire-and-forget (canvas checkout, speak narration)"), explicitly frames this bypass as safe only for non-mutating tools — `canvas_checkout`'s create-branch violates that invariant.
In `canUseTool` (`permission-handlers.ts:731-835`), the `AUTO_APPROVED_LOCAL_TOOL_IDS.has(toolName)` check (line 764) returns `{behavior: "allow"}` unconditionally, with no `session.permissionMode` check, and runs before the plan-mode deny-by-default gate later in the function and before the explicit `session.permissionMode === "auto"`-style checks used elsewhere in the same file for other mutating/destructive actions. This means an agent in a restrictive mode (e.g. "plan", where the user expects zero side effects) can still silently create a new canvas via `canvas_checkout({name: ...})` with no user confirmation at all — a genuine authorization-gap regression introduced by generalizing the old speak-only bypass to cover this tool.
</issue_description>
<issue_validation>
- **Checked:** `canvas_checkout`'s `autoApprove` flag and create path (canvas.ts:189, 193-195, 101-118), the auto-approve set derivation (index.ts:41-45), the `canUseTool` ordering (permission-handlers.ts:731-834), and the mutation itself (posthog-api.ts:372-390).
- **Found:** The premise is accurate. permission-handlers.ts:764 returns `{behavior: "allow"}` unconditionally with no `session.permissionMode` check, inside the `mcp__` block (751-796) that returns before the plan-mode deny gate at line 826 — so `canvas_checkout` is allowed in every mode. And its `name`-only branch is genuinely mutating: `createDesktopCanvas` POSTs to `/api/projects/:teamId/desktop_file_system/` creating a new dashboard row (posthog-api.ts:379-385), contradicting the `autoApprove` "read-only or fire-and-forget" contract (registry.ts:57-60). This is newly introduced by generalizing the prior speak-only bypass.
- **Impact:** Real permission/contract gap: a mutating backend create runs with no prompt even in plan mode, whose whole purpose (per the comment at permission-handlers.ts:822-825) is to guarantee no side effects. Worth surfacing. Note `canvas_publish` (mutating an existing live version) is correctly NOT auto-approved, so only the create branch leaks.
- **Priority:** Lowered to `should_fix` — the consequence is bounded: an empty, reversible canvas created in the user's own project via their own credentials, with no data loss, cross-tenant/IDOR exposure, or privilege escalation. That is below the exploitable/destructive threshold `must_fix` implies, though the plan-mode contract violation makes it a legitimate fix.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Either split checkout into a read-only lookup path (kept `autoApprove`) and a separate create path that requires normal approval, or drop `autoApprove` from `canvas_checkout` entirely so any create-if-missing call goes through the standard permission prompt like `canvas_publish` does. If the intent is genuinely to allow silent creation, that should be explicitly gated behind `session.permissionMode` (excluding at least plan/read-only modes) rather than bypassing all modes unconditionally.
</potential_solution>
| const lines = code ? code.split("\n").length : 0; | ||
| const header = code | ||
| ? `Checked out canvas "${entry.path}" (id ${canvasId}) to ${file} (${lines} lines, base version ${ | ||
| entry.meta?.currentVersionId ?? "none" | ||
| }). Edit that file, then call canvas_publish with id "${canvasId}".` | ||
| : `Canvas "${entry.path}" (id ${canvasId}) is empty — author the complete single-file React app at ${file}, then call canvas_publish with id "${canvasId}".`; | ||
| const text = `${header}\n\n${authoringContract(entry.meta?.templateId, !code)}`; | ||
| return { content: [{ type: "text", text }] }; |
There was a problem hiding this comment.
canvas_checkout hands the agent a contract that contradicts the tool's own workflow
Why we think it's a valid issue
- Checked:
canvas_checkout's returned text (canvas.ts:205-212, 281),authoringContract(canvas.ts:124-133), and the full shared prompt it embeds (canvas-freeform-prompt.ts), including which variants carry the closing line. - Found: The contradiction is real and confirmed.
buildFreeformPromptappends...FREEFORM_STYLEfor every template (canvas-freeform-prompt.ts:70-76, 151-153), whose final line is "Do NOT write files, edit code on disk, or run shell commands. Your entire app is the single fenced tsx block in your reply." (line 64) — directly opposing the checkout header's "Edit that file, then call canvas_publish" (canvas.ts:207-210) and "do not paste the code into chat" (line 281). - Found: It is in fact an internal self-contradiction: FREEFORM_BASE line 26 already instructs "Maintain it as a working file with your file-editing tools ... do not paste the source into chat," so line 64 is a stale leftover from the old full-file-in-chat flow that was not updated in the refactor.
- Impact: An agent obeying the emphatic closing line would emit the whole file in chat and never call
canvas_publish, silently reverting to the exact behavior this PR eliminates — a legitimate logic/prompt-contract defect, cheap to fix by parameterizing/stripping that line for the checkout flow. - Priority: Lowered to
should_fix— the same prompt carries strong, more-specific competing guidance toward the correct flow (line 26 + the checkout header naming the scratch file and publish tool), so the regression is a probabilistic model-behavior risk rather than a guaranteed break; the finding's "always contradicts / never publishes" framing overstates certainty.
Issue description
The checkout response tells the agent to "Edit that file, then call canvas_publish" (line 209/210), then immediately appends the full result of freeformSystemPromptFor() via authoringContract(). That shared prompt's closing STYLE section (packages/shared/src/canvas-freeform-prompt.ts, FREEFORM_STYLE) unconditionally ends with: "Do NOT write files, edit code on disk, or run shell commands. Your entire app is the single fenced tsx block in your reply." Every templateId variant (freeform, dashboard, web-analytics) includes this line, so canvas_checkout's response always contains a direct contradiction: first it instructs the agent to edit a scratch file with file tools and never paste code into chat, then the appended "authoring contract" instructs the opposite (no files, reply with a fenced code block). An agent following the second instruction would revert to the old full-file-in-chat behavior this PR is explicitly trying to eliminate (full-file token cost, drift, stale-read clobbers) and would never call canvas_publish, since the source would go into the chat reply instead of the scratch file.
Suggested fix
Strip or override the file-write prohibition from the authoring contract before returning it from canvas_checkout — e.g. add a scratch-file-specific system message override, or amend freeformSystemPromptFor/FREEFORM_STYLE so the closing instruction is parameterized for the checkout/publish flow instead of unconditionally saying "reply with a fenced block". At minimum, canvas_checkout's returned text should explicitly override that line so the agent isn't given two contradictory instructions in the same tool result.
Prompt to fix with AI (copy-paste)
## Context
@packages/agent/src/adapters/local-tools/tools/canvas.ts#L205-212
@packages/agent/src/adapters/local-tools/tools/canvas.ts#L124-133
<issue_description>
The checkout response tells the agent to "Edit that file, then call canvas_publish" (line 209/210), then immediately appends the full result of `freeformSystemPromptFor()` via `authoringContract()`. That shared prompt's closing STYLE section (packages/shared/src/canvas-freeform-prompt.ts, `FREEFORM_STYLE`) unconditionally ends with: "Do NOT write files, edit code on disk, or run shell commands. Your entire app is the single fenced tsx block in your reply." Every templateId variant (freeform, dashboard, web-analytics) includes this line, so canvas_checkout's response always contains a direct contradiction: first it instructs the agent to edit a scratch file with file tools and never paste code into chat, then the appended "authoring contract" instructs the opposite (no files, reply with a fenced code block). An agent following the second instruction would revert to the old full-file-in-chat behavior this PR is explicitly trying to eliminate (full-file token cost, drift, stale-read clobbers) and would never call canvas_publish, since the source would go into the chat reply instead of the scratch file.
</issue_description>
<issue_validation>
- **Checked:** `canvas_checkout`'s returned text (canvas.ts:205-212, 281), `authoringContract` (canvas.ts:124-133), and the full shared prompt it embeds (canvas-freeform-prompt.ts), including which variants carry the closing line.
- **Found:** The contradiction is real and confirmed. `buildFreeformPrompt` appends `...FREEFORM_STYLE` for every template (canvas-freeform-prompt.ts:70-76, 151-153), whose final line is "Do NOT write files, edit code on disk, or run shell commands. Your entire app is the single fenced tsx block in your reply." (line 64) — directly opposing the checkout header's "Edit that file, then call canvas_publish" (canvas.ts:207-210) and "do not paste the code into chat" (line 281).
- **Found:** It is in fact an internal self-contradiction: FREEFORM_BASE line 26 already instructs "Maintain it as a working file with your file-editing tools ... do not paste the source into chat," so line 64 is a stale leftover from the old full-file-in-chat flow that was not updated in the refactor.
- **Impact:** An agent obeying the emphatic closing line would emit the whole file in chat and never call `canvas_publish`, silently reverting to the exact behavior this PR eliminates — a legitimate logic/prompt-contract defect, cheap to fix by parameterizing/stripping that line for the checkout flow.
- **Priority:** Lowered to `should_fix` — the same prompt carries strong, more-specific competing guidance toward the correct flow (line 26 + the checkout header naming the scratch file and publish tool), so the regression is a probabilistic model-behavior risk rather than a guaranteed break; the finding's "always contradicts / never publishes" framing overstates certainty.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Strip or override the file-write prohibition from the authoring contract before returning it from canvas_checkout — e.g. add a scratch-file-specific system message override, or amend `freeformSystemPromptFor`/`FREEFORM_STYLE` so the closing instruction is parameterized for the checkout/publish flow instead of unconditionally saying "reply with a fenced block". At minimum, canvas_checkout's returned text should explicitly override that line so the agent isn't given two contradictory instructions in the same tool result.
</potential_solution>
| try { | ||
| const entry = await client.publishDesktopCanvas<CanvasFsEntry>(args.id, { | ||
| code, | ||
| prompt: args.prompt, | ||
| expectedCurrentVersionId: marker.versionId ?? null, | ||
| }); | ||
| // Advance the base so a follow-up publish in the same session works | ||
| // without a re-checkout. | ||
| const newVersionId = entry.meta?.currentVersionId; | ||
| writeMarker(args.id, { versionId: newVersionId, fetchedAt: Date.now() }); | ||
| return { | ||
| content: [ | ||
| { | ||
| type: "text", | ||
| text: `Published canvas "${args.id}"${ | ||
| newVersionId ? ` (new version ${newVersionId})` : "" | ||
| }. The canvas is live; do not paste the code into chat.`, | ||
| }, | ||
| ], | ||
| }; |
There was a problem hiding this comment.
A local marker-write failure after a successful remote publish is reported to the agent as a full publish failure
Why we think it's a valid issue
- Checked: the publish handler's try/catch scope (canvas.ts:265-294),
writeMarker's fs calls (canvas.ts:145-148), the existing silent-catch precedent inreadMarker(canvas.ts:135-143), and the checkout recovery path (canvas.ts:196-204) to trace what re-running actually does. - Found:
writeMarker(line 274) executes inside the same try as the already-succeededpublishDesktopCanvas(line 266); an fs throw there falls into the catch and returnscanvas_publish failed:(lines 285-293), since it is not aDesktopCanvasVersionConflictError. Premise confirmed. - Found: the scratch dir + marker were already written at checkout (lines 199-201) and read at publish-start (lines 242, 253) on the same paths, so this requires
/tmpto degrade mid-operation — narrow, but realistic in an ephemeral sandbox tmpfs, and it is the last write in the flow. - Impact: the remote publish is authoritative and succeeded, yet the agent sees a failure. A direct publish retry then hits a real 409 version-conflict (stale marker vs advanced head); a re-checkout instead resets the marker and leads to a redundant duplicate version. Confusing, unnecessary rework — recoverable, no data loss. Legitimate reliability/error-ordering defect (non-authoritative bookkeeping masking an authoritative success) with a trivial best-effort-write fix, matching the 'mishandled error that hides the true outcome' keep category.
Issue description
In canvasPublishTool.handler, writeMarker(args.id, ...) (line 274) runs inside the same try block as the network call to client.publishDesktopCanvas, after that call has already succeeded and mutated server state. If writeMarker throws — e.g. /tmp is full, unwritable, or the process lacks permission to write there — execution falls into the catch block and returns canvas_publish failed: ... to the agent, even though the canvas was actually published successfully. The agent has no way to distinguish 'the publish itself failed' from 'the publish succeeded but bookkeeping failed', and per the tool's own recovery guidance it may re-run canvas_checkout/canvas_publish, which will now trigger a legitimate version-conflict against the version it just itself created (since the marker was never advanced), producing confusing, unnecessary rework.
Suggested fix
Move the success response (and the point where you consider the operation done) ahead of writeMarker, and make the marker update best-effort/non-fatal — wrap it in its own try/catch (or reuse the existing silent-catch pattern from readMarker) so a local disk error after a successful remote publish doesn't get reported as canvas_publish failed.
Prompt to fix with AI (copy-paste)
## Context
@packages/agent/src/adapters/local-tools/tools/canvas.ts#L265-284
<issue_description>
In `canvasPublishTool.handler`, `writeMarker(args.id, ...)` (line 274) runs inside the same `try` block as the network call to `client.publishDesktopCanvas`, after that call has already succeeded and mutated server state. If `writeMarker` throws — e.g. `/tmp` is full, unwritable, or the process lacks permission to write there — execution falls into the `catch` block and returns `canvas_publish failed: ...` to the agent, even though the canvas was actually published successfully. The agent has no way to distinguish 'the publish itself failed' from 'the publish succeeded but bookkeeping failed', and per the tool's own recovery guidance it may re-run `canvas_checkout`/`canvas_publish`, which will now trigger a legitimate `version-conflict` against the version it just itself created (since the marker was never advanced), producing confusing, unnecessary rework.
</issue_description>
<issue_validation>
- **Checked:** the publish handler's try/catch scope (canvas.ts:265-294), `writeMarker`'s fs calls (canvas.ts:145-148), the existing silent-catch precedent in `readMarker` (canvas.ts:135-143), and the checkout recovery path (canvas.ts:196-204) to trace what re-running actually does.
- **Found:** `writeMarker` (line 274) executes inside the same try as the already-succeeded `publishDesktopCanvas` (line 266); an fs throw there falls into the catch and returns `canvas_publish failed:` (lines 285-293), since it is not a `DesktopCanvasVersionConflictError`. Premise confirmed.
- **Found:** the scratch dir + marker were already written at checkout (lines 199-201) and read at publish-start (lines 242, 253) on the same paths, so this requires `/tmp` to degrade mid-operation — narrow, but realistic in an ephemeral sandbox tmpfs, and it is the last write in the flow.
- **Impact:** the remote publish is authoritative and succeeded, yet the agent sees a failure. A direct publish retry then hits a real 409 version-conflict (stale marker vs advanced head); a re-checkout instead resets the marker and leads to a redundant duplicate version. Confusing, unnecessary rework — recoverable, no data loss. Legitimate reliability/error-ordering defect (non-authoritative bookkeeping masking an authoritative success) with a trivial best-effort-write fix, matching the 'mishandled error that hides the true outcome' keep category.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Move the success response (and the point where you consider the operation done) ahead of `writeMarker`, and make the marker update best-effort/non-fatal — wrap it in its own try/catch (or reuse the existing silent-catch pattern from `readMarker`) so a local disk error after a successful remote publish doesn't get reported as `canvas_publish failed`.
</potential_solution>
| const checkoutStep = ` | ||
| [Working copy] — FIRST, check out the canvas: call the \`canvas_checkout\` tool | ||
| (posthog-code-tools) with id "${dashboardId}". It writes the canvas's live | ||
| source to a local scratch file and returns the path.`; |
There was a problem hiding this comment.
Authoring contract and starter scaffold get sent to the model twice per canvas-generation turn
Why we think it's a valid issue
- Checked:
freeformPrompt.ts(contract/starter embedding + checkout ordering),canvas.tscanvas_checkouthandler +authoringContract(), the call siteuseGenerateFreeformCanvas.ts, and whetherextractCanvasInstructionsstrips the block before it reaches the model. - Found: The prompt embeds
${contract}inline (freeformPrompt.ts:104, sourced fromfreeformSystemPromptFor(templateId)at:44) and, for a first build withuseStarter, the fullFREEFORM_STARTER_CODE(:79-81, 6,334 bytes confirmed). It then mandatescanvas_checkoutas the FIRST action (:54-57). The checkout handler independently returns the SAME content:authoringContract(templateId, !code)atcanvas.ts:211re-runsfreeformSystemPromptFor(...)(:128) and re-appendsFREEFORM_STARTER_CODEfor an empty canvas (:129-131). - Found: The duplication genuinely reaches the model —
useGenerateFreeformCanvas.ts:142passes the wholebuildFreeformGenerationPrompt(...)string ascreateTaskcontent;extractCanvasInstructionsis only used in display/recall paths (ChatThread.tsx,UserMessage.tsx,composerPromptRecall.ts), so the<canvas_generation_instructions>wrapper collapses it in the UI but does not remove it from the payload. - Found: The contract is duplicated on EVERY canvas turn (edits included, since checkout returns the contract regardless of emptiness), and the ~6KB starter additionally duplicates on the default first-build path — several thousand tokens of boilerplate per turn.
- Found: The fix is already the author's own pattern in this PR —
agent.ts:683sayscanvas_checkout"returns the authoring contract ... follow it" without inlining it — so mirroring it infreeformPrompt.tsis consistent, not speculative. - Impact: Confirmed, directly PR-introduced (before this PR there was no
canvas_checkout, so the contract shipped once) redundant token cost/latency on the documented common path of a core feature. Concrete trigger and concrete consequence both named; clean, low-risk fix. Meets the performance/efficiency bar; not overengineering or speculation.
Issue description
The task prompt built here embeds the full authoring contract inline (${contract} from freeformSystemPromptFor(templateId), lines 100-104) and, for a first build with the default starter scaffold enabled, the complete FREEFORM_STARTER_CODE (lines 71-82) directly in the initial task message. The very next instruction the prompt gives (checkoutStep, lines 54-57) mandates that the agent's FIRST action be to call the canvas_checkout tool. That tool's handler (packages/agent/src/adapters/local-tools/tools/canvas.ts, authoringContract()) independently calls the same freeformSystemPromptFor(templateId) and, for an empty canvas, appends the same FREEFORM_STARTER_CODE again, returning both as the tool's result text. Because the agent is always instructed to call canvas_checkout as step one, this same content lands in the conversation twice within a single turn: once embedded in the initial prompt, once again in the tool result. The authoring contract is several KB (larger for the dashboard/web-analytics templates, which append the Quill + date-control rule sets on top of the base contract), and the starter scaffold is ~6KB, so a default first-build turn (useStarter=true, the documented default) can duplicate several thousand tokens of pure boilerplate on every single canvas generation/edit call — this is not an edge case, it is the common path introduced by this PR's routing change (before this PR there was no canvas_checkout call, so the contract was only ever sent once).
Suggested fix
Now that canvas_checkout always returns the authoring contract (and, for an empty canvas, the starter scaffold) as its tool result, stop embedding ${contract} and the inline starter-scaffold body in buildFreeformGenerationPrompt and instead point the agent at "the authoring contract canvas_checkout returned." This is exactly the pattern already used elsewhere in this same PR: the ## Canvases section added to packages/workspace-server/src/services/agent/agent.ts (lines 679-683) does not duplicate the contract inline — it only says canvas_checkout "returns the authoring contract ... follow it." Mirror that here to cut the redundant token cost on every canvas-generation turn.
Prompt to fix with AI (copy-paste)
## Context
@packages/ui/src/features/canvas/freeformPrompt.ts#L54-57
@packages/ui/src/features/canvas/freeformPrompt.ts#L71-82
@packages/ui/src/features/canvas/freeformPrompt.ts#L100-104
<issue_description>
The task prompt built here embeds the full authoring contract inline (`${contract}` from `freeformSystemPromptFor(templateId)`, lines 100-104) and, for a first build with the default starter scaffold enabled, the complete `FREEFORM_STARTER_CODE` (lines 71-82) directly in the initial task message. The very next instruction the prompt gives (`checkoutStep`, lines 54-57) mandates that the agent's FIRST action be to call the `canvas_checkout` tool. That tool's handler (`packages/agent/src/adapters/local-tools/tools/canvas.ts`, `authoringContract()`) independently calls the same `freeformSystemPromptFor(templateId)` and, for an empty canvas, appends the same `FREEFORM_STARTER_CODE` again, returning both as the tool's result text. Because the agent is always instructed to call `canvas_checkout` as step one, this same content lands in the conversation twice within a single turn: once embedded in the initial prompt, once again in the tool result. The authoring contract is several KB (larger for the `dashboard`/`web-analytics` templates, which append the Quill + date-control rule sets on top of the base contract), and the starter scaffold is ~6KB, so a default first-build turn (`useStarter=true`, the documented default) can duplicate several thousand tokens of pure boilerplate on every single canvas generation/edit call — this is not an edge case, it is the common path introduced by this PR's routing change (before this PR there was no `canvas_checkout` call, so the contract was only ever sent once).
</issue_description>
<issue_validation>
- **Checked:** `freeformPrompt.ts` (contract/starter embedding + checkout ordering), `canvas.ts` `canvas_checkout` handler + `authoringContract()`, the call site `useGenerateFreeformCanvas.ts`, and whether `extractCanvasInstructions` strips the block before it reaches the model.
- **Found:** The prompt embeds `${contract}` inline (`freeformPrompt.ts:104`, sourced from `freeformSystemPromptFor(templateId)` at `:44`) and, for a first build with `useStarter`, the full `FREEFORM_STARTER_CODE` (`:79-81`, 6,334 bytes confirmed). It then mandates `canvas_checkout` as the FIRST action (`:54-57`). The checkout handler independently returns the SAME content: `authoringContract(templateId, !code)` at `canvas.ts:211` re-runs `freeformSystemPromptFor(...)` (`:128`) and re-appends `FREEFORM_STARTER_CODE` for an empty canvas (`:129-131`).
- **Found:** The duplication genuinely reaches the model — `useGenerateFreeformCanvas.ts:142` passes the whole `buildFreeformGenerationPrompt(...)` string as `createTask` `content`; `extractCanvasInstructions` is only used in display/recall paths (`ChatThread.tsx`, `UserMessage.tsx`, `composerPromptRecall.ts`), so the `<canvas_generation_instructions>` wrapper collapses it in the UI but does not remove it from the payload.
- **Found:** The contract is duplicated on EVERY canvas turn (edits included, since checkout returns the contract regardless of emptiness), and the ~6KB starter additionally duplicates on the default first-build path — several thousand tokens of boilerplate per turn.
- **Found:** The fix is already the author's own pattern in this PR — `agent.ts:683` says `canvas_checkout` "returns the authoring contract ... follow it" without inlining it — so mirroring it in `freeformPrompt.ts` is consistent, not speculative.
- **Impact:** Confirmed, directly PR-introduced (before this PR there was no `canvas_checkout`, so the contract shipped once) redundant token cost/latency on the documented common path of a core feature. Concrete trigger and concrete consequence both named; clean, low-risk fix. Meets the performance/efficiency bar; not overengineering or speculation.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Now that `canvas_checkout` always returns the authoring contract (and, for an empty canvas, the starter scaffold) as its tool result, stop embedding `${contract}` and the inline starter-scaffold body in `buildFreeformGenerationPrompt` and instead point the agent at "the authoring contract `canvas_checkout` returned." This is exactly the pattern already used elsewhere in this same PR: the `## Canvases` section added to `packages/workspace-server/src/services/agent/agent.ts` (lines 679-683) does not duplicate the contract inline — it only says `canvas_checkout` "returns the authoring contract ... follow it." Mirror that here to cut the redundant token cost on every canvas-generation turn.
</potential_solution>
Canvas generation previously required the agent to regenerate and emit the entire React source every turn. Add a canvas_checkout / canvas_publish local tool pair (available to both the Claude and Codex adapters) that moves the file I/O tool-side: checkout writes the live source to a scratch path and records the base version, the agent applies targeted edits with its native file tools, and publish reads the file from disk and appends a version — refusing when the canvas moved past the checkout base (concurrent edit or undo) instead of clobbering it. The generation prompt now routes through checkout → edit → publish and no longer embeds the current source. Storage stays wholesale full-file snapshots; a server-side base_version check on the desktop-fs PATCH remains a follow-up. Generated-By: PostHog Code Task-Id: 2b3d176e-3023-41d2-92e8-81eda7c12a8c
…as action 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
…ools The "do not regenerate wholesale" phrasing in the checkout tool description, its runtime message, and the generation prompt warned against a workflow a fresh agent never sees, and could bias against legitimately large rewrites. The one forward-looking economy nudge (prefer targeted edits over rewriting the whole file for a small change) stays in the authoring contract in canvasTemplates.ts, stated once. Generated-By: PostHog Code Task-Id: 2b3d176e-3023-41d2-92e8-81eda7c12a8c
…ry + allowlist Build on the canvas_checkout/canvas_publish tools so canvas work is reachable and correct from any task, not just the channel generate bar: - canvas_checkout takes an optional id (edit) OR a name (create-if-missing), so the agent starts a canvas in one call instead of chaining desktop-fs create. - checkout returns the freeform authoring contract (from @posthog/shared), so an agent editing a canvas from a normal task authors valid source. - a channelMode-gated "## Canvases" block in the base system prompt names the tools + the create->checkout->publish sequence (fixes ToolSearch flakiness). - add a declarative autoApprove flag to local tools; canvas_checkout (read-only) and speak are auto-approved, canvas_publish (write) still prompts. - createDesktopCanvas API helper + allowlist regression test. Generated-By: PostHog Code Task-Id: 2ed89933-091b-446c-af91-5cc72d939b3b
f73be5a to
72f3e5a
Compare
061c5de to
0aca45b
Compare
The Codex adapter injects the local tools (canvas_checkout/publish,
list_repos, clone_repo, speak, signed-git) as a stdio MCP child resolved via
resolveBundledMcpScript — but nothing ever copied that script into the
Electron build, so resolution threw ("Could not locate ... relative to
.../.vite/build") and Codex silently lost ALL local tools in the desktop app,
dev and packaged alike (cloud harness layouts were unaffected).
- copyLocalToolsMcpServer vite plugin mirrors packages/agent/dist/adapters/
codex-app-server/local-tools-mcp-server.js into .vite/build with its
dist-relative layout intact, so the existing walk-up resolution finds it.
- asarUnpack .vite/build/adapters/** — Codex is an external binary spawning
the child as plain node, which can't read inside app.asar.
- resolveBundledMcpScript returns the app.asar.unpacked mirror when the found
candidate sits inside the archive.
Validated live over CDP: a Codex channel task now logs hasLocalTools: true
and the model lists canvas_checkout/canvas_publish/list_repos/clone_repo.
Generated-By: PostHog Code
Task-Id: 09832237-2f58-4ab7-adf6-231aeb82e1fe
…ically canvas_checkout's create-if-missing asked the model to relay the channel as parentPath from the <channel_context> element — which doesn't exist when the channel has no CONTEXT.md (the model then stalls asking the user), and a mistyped name silently minted a phantom top-level folder because the desktop- fs POST auto-creates missing parents. Canvases landed orphaned at depth 1, invisible in every channel grid. Resolve the destination tool-side instead: the app files every channel task as a desktop-fs task row at <channelFolder>/<title> with ref=<taskId>, so the tool joins ctx.taskId -> filing row -> parent folder. Id-based, so it survives channel renames and duplicate names, and works identically from the Claude in-process server and the Codex stdio child (cloud included — only ctx.taskId and API creds are needed). The created row now carries the same meta the app stamps (channelId/templateId/createdAt/updatedAt), so agent- created canvases list and open exactly like UI-created ones. parentPath remains as an explicit override but must name an EXISTING channel folder — never a guessed one — and the not-in-a-channel error names the real channels to offer the user. Local sessions never actually carried a task id: workspace-server's newSession _meta omitted it (and load/resume only tucked it inside the logUrl-gated persistence blob), so resolveTaskId() came up undefined and the join couldn't run. Pass taskId explicitly on all three session-start paths. Validated E2E over CDP against the local backend: a Sonnet 5 channel task with no CONTEXT.md called canvas_checkout with just a name, the canvas landed at demo-channel/Placement Check with the channel folder id in meta, published on the first try, and renders in the channel grid and canvas view. Generated-By: PostHog Code Task-Id: 09832237-2f58-4ab7-adf6-231aeb82e1fe
1 participant
Problem
Every canvas generation turn is a full-file rewrite: the agent regenerates the entire React source and pastes it into the
desktop-file-system-canvas-partial-updateMCP call, even for a one-line change. That makes small edits pay full-file token cost, lets unrelated regions drift between versions, and a publish based on a stale read can silently clobber a concurrent edit or a user's undo.Why
Groundwork for structured canvas feedback (inline annotations — stacked as #3599) and an eventual "plan mode writes canvases" flow: annotations are surgical, so canvas edits need to be surgical too. Direction discussed in the linked task thread.
Changes
canvas_checkout/canvas_publishlocal tools (packages/agent, registered for both the Claude and Codex adapters). Checkout fetches the canvas and writes the source to a deterministic scratch path tool-side (outside any workspace, so it never pollutes diff panels orgit status) and records the fetchedcurrentVersionId. Publish reads the file from disk and calls the desktop-fs canvas action, which owns version composition server-side; the recorded version rides along asexpected_current_version_id, so a stale publish is rejected atomically with aversion-conflicterror and a re-checkout recovery path. The source never passes through the model in either direction.freeformPrompt.ts) routes builds and edits through checkout → targeted file edits → publish, and no longer embeds the current source (checkout fetches it fresher at turn start).PostHogAPIClientgainsgetDesktopFsEntry/publishDesktopCanvas(409 surfaced as a typed conflict error).Depends on PostHog/posthog#72365 for the guard's server side (
expected_current_version_id+ 409 + redo-tail truncation). Degrades gracefully — a backend without the field ignores it and publishes unguarded (today's behavior) — but the guard is only real once #72365 is deployed to both regions, so merge after it ships. Stacked on top: #3599 (comment-mode annotation overlay).How did you test this code?
vitestunit tests:canvas.test.ts(scratch-path plumbing; version-composition and conflict semantics are tested server-side in feat(file-system): optimistic-concurrency guard on canvas publish posthog#72365) and updatedfreeformPrompt.test.ts— all pass.tsc --noEmiton@posthog/agent,@posthog/ui,@posthog/core;biome checkclean on touched files.signed-commit/signed-merge"no-token" tests fail in any cloud sandbox because the live/tmp/agent-envprovides a real token — unrelated to this change.Automatic notifications
Created with PostHog Code