Protect Canvas saves with revision preconditions

[wesbillman]

Summary

Adopt Canvas compare-and-swap from block/buzz#6780, without history/restore UI or a new delivery owner.

• Editor/Todos saves carry their loaded revision; template seeds carry expected-revision=none. Writer-authoritative reads guard editing/setup; template copies explicitly remain replica-eligible.
• Proven Canvas refusals retain drafts and dismiss only the definitively rejected operation. Unknown delivery and refused retries after a lost ACK remain journaled and block replacement. Seed refusal stops agent additions without fencing later manual edits.
• Native/development admission validates the precondition while allowing legacy untagged retries. Existing timestamp waits, exact signed-byte retries and conservative post-save head checks remain.

Compatibility: atomic protection requires a relay implementing #6780. Older relays may ignore the tag; this PR does not upgrade them.

Latest repair: 70a71183

Merged main 75568535 in e907e2ba; subsequent commits dcbe566d and 70a71183 change only documentation. No force push. The latest correction addresses review 5395157022: standalone recipes share writer-backed exact-ID confirmation, while recipe head/catalog reads remain replica-eligible. No runtime behavior changed.

• Resolved all five conflicts while retaining main’s header-menu/template actions and removal of the Channel Settings Canvas preview. The entire src/bundled/channels/ directory matches that main revision.
• Kept the strong-by-default Canvas read contract, main’s writer-backed setup delivery/exact confirmation, and the template-copy opt-out. The imported stale-replica assertion now explicitly requests an ordinary read. Removed the obsolete lost-seed-ACK replica-read limitation from the documentation and linked the consistency contract from relay-queries.md.
• All four existing review threads are addressed. The earlier shared definitive-conflict predicate and lost-ACK regressions remain; the CI workflow still matches main, with two JavaScript shards at 10 minutes. Independent source re-review found no blocker in this merge or adjacent auto-merges.

A local trial merge against fetched main f432088a is conflict-free. The newer main/toolchain state is left to hosted CI, not claimed by the local results below.

Validation

Local behavioral checks cover the integrated source tree committed as e907e2ba (tree d5c2080c). Both commits after e907e2ba, through final head 70a71183, change only Markdown. No browser cases were added or removed in this repair.

Check	Result
Full Vitest	499 files / 6,804 tests passed, 66.27s wall, 504.78s summed tests
Affected browser files, Chromium and WebKit	18 passed, 36.9s: channel-header-menu.spec.mjs, channel-modal-dismissal.spec.mjs, and todos.spec.mjs; fixture-backed header actions, Canvas draft recovery and Todos, not live-relay/native acceptance
Mandatory push gates at final head 70a71183	TypeScript, 199 files / 3,398 related tests (50.16s test run), design types/guards and whole-workspace Clippy passed. Custom hooks preserved; the repository gate explicitly received the actual ref update.
Independent merge review	Clear at e907e2ba; source review only, not a second test run
Hosted CI	CI at dcbe566d passed. At documentation-only head 70a71183, DCO passed and fresh CI is running.

Earlier evidence, not rerun at the latest head

Automated review 5394829289, published via wpfleger96 at 602f25ef, reports a two-session stale-save race against a fresh local block/buzz relay supporting #6780. It exercised this PR’s Canvas capability/outbox via signed HTTP requests, for both an existing Canvas and first write: B saved first, A received 409, B remained head, A’s event was absent from relay storage, and only A’s rejected outbox entry was dismissed. It also reports predicate mutation checks. This is credited reviewer evidence, not a new run by this repair; it did not exercise the browser editor, native WebSocket transport or GUI.

At 602f25ef, the full native package passed with --test-threads=2: 209 unit + 1 integration passed, 7 ignored, doctests passed. An earlier default-concurrency run had 205 passed / 4 failed / 7 ignored, in unchanged host_command process-start/deadline tests while full JS ran concurrently. The bounded rerun changed no assertions, deadlines or test code. This does not establish that default-concurrency failures were fixed. The full native package was not rerun for the latest merge; current local native validation is Clippy only.

Remaining merge/acceptance gates

• Fresh CI required and required approving/code-owner review. GitHub reports no merge conflicts, and DCO Check passed at 70a71183. The PR is currently non-draft; this documentation-only update preserved its existing state. Not merged; no human acceptance is asserted here.
• Native-app/WebSocket exercise and explicit human acceptance remain open. Current-head end-to-end relay race acceptance has not been repeated. This repair did not launch/restart the running desktop or write to a live channel.

Human acceptance

On an isolated test channel with a supporting relay, open Channel actions → View canvas in two sessions. Save one, then save the stale draft in the other: the newer document must survive and the stale draft must remain editable. Reload/review and save again; it must succeed without manual outbox dismissal. Repeat an initial-seed race: the competing document survives and setup adds no agents after rejection. Check Todos uses the same conflict behavior and Save as template still works from the header menu. Include a native Desktop session and report explicit acceptance after testing.

Originating conversation: buzz://message?channel=8e06aee5-0a0b-4b04-ad07-c29125101cb6&id=b52a2f327106b932d3bfc8c3e2763d29432336b43cfa5603c4db3cf3cff48cac

[wesbillman]

On Wes’s behalf — Brain: Independent Brain/Pinky source reviews agree: no new blocking defect in the changed CAS paths at 45fdbdaf5b82cda0065951685537a56b2297f5be. Two optional improvements are inline.

This is not readiness clearance: the disclosed FOUNDATION session.ts seed-confirmation gap (also tracked in #501’s review) still needs human-authorized resolution. Supporting-relay deployment, real two-session Canvas/Todos/seed-race exercise, native exercise, explicit human acceptance and required approving review remain unverified.

CI 36892211891, attempt 1, is not green: JavaScript exceeded its 10-minute job limit during Vitest, without a completed suite summary; Rust/tools remain running at inspection. All twelve browser shards passed (1,078 executions), plus 9+1 measurements, on synthetic merge ee6acae7dcadd3198d609141fb1a9e53bbf71bcb. DCO/security pass; Windows skipped. The timeout mechanism is verified, not its root cause; no timeout increase or speculative code change recommended.

Read-only immutable source and hosted logs; neither maintenance reviewer ran local suites or live/native flows. Exact refusal/rollback semantics were compared against the actual #6780 relay code, not inferred solely from its description. Its merge does not prove deployment.

[wpfleger96]

🤖 Thanks for this. No blocking findings at 45fdbdaf5b82cda0065951685537a56b2297f5be.

Both Canvas writers (ChannelCanvasDialog and TodosPanel) pass the head they loaded into canvas.save, an explicit reload replaces content and base together, and the shared writer sends expected-revision=<loaded id> or none. The template seed sends none. I didn't find any other kind-40100 publisher in src/ or dev/.

The cleanup matches what the relay actually does. In block/buzz#6780 (b6a26556), the three Canvas conflict: reasons only come from the compare-and-swap branch in ingest.rs, and insert_channel_head_checked rolls back before inserting. Resending the bytes that are already the live head comes back as Duplicate. The case that matters is a stored save whose ACK was lost, followed by someone else's save and then a retry that gets refused with conflict:. That refusal doesn't prove the original never landed, but the outbox already keeps a retry after an unknown outcome as unknown with Retry blocked: prepended. The new dismiss in capability.ts and setup.ts only fires on delivery === "failed" plus a conflict: error, so that save stays in the outbox and the pending-save gate still blocks a replacement. Native goes through the 409 mapping in acceptPublish, the dev WebSocket path through the prefix rule in socket-requests.ts, and the broker only forwards a proven conflict as 409. Native valid_canvas and dev validCanvas now agree: no revision tag, none, or 64 lowercase hex, and at most one. Native is stricter than before (40100 used to be allowlisted with no tag check), but it matches the broker and keeps client-id and untagged legacy retries.

Beyond the source read, at this head the full Vitest suite passed locally with two workers (481 files / 6,139 tests, about 8m17s), and the native relay tests passed (61, 2 ignored). Each of these deliberate code breaks made at least one test fail:

• dropping the editor revision tag (5 failures in capability.test.ts)
• classifying a Canvas refusal as unknown (the 40100 case in dev/relay-broker-live.test.mjs)
• continuing to add agents after a refused seed (setup.test.ts; it really did enqueue an agent add)
• rejecting untagged native saves (canvas_signing_bounds_revision_preconditions_and_allows_exact_legacy_retries)

The full native package wasn't reliably green on this machine because of intermittent failures in unchanged host_command and Pi cancellation tests. None of this is a two-session race against a relay with #6780, a native GUI run, or human acceptance.

Optional notes:

• +1 to Wes's read-load point: every canvas.read() is now strong, including the Settings preview and "Save as template". Splitting a display read from the editor base, preflight, and confirmation reads would keep the writer load where it's needed.
• +1 to the lost-ACK test: a Canvas-level case covering a lost ACK, a newer head, and then a refused exact retry would pin the retained uncertainty and the blocked replacement. The generic outbox test covers the mechanism today.

Still open outside the code: the setup confirmation read in session.ts you disclosed (replica lag can stop a successful setup but can't authorize an overwrite), the JavaScript job hitting its 10 minute limit, so CI required is red, and the description's "Keep draft" line, since the PR is no longer a draft.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Pushed 8a35727f to address the CI timeout and the separately authorized setup confirmation read:

• JavaScript now has 15 minutes of job budget. The prior job passed all 6,329 tests before the 10-minute cap; no test deadlines, workers or coverage were changed.
• The setup exact-ID read is strong after delivery settles; full local Vitest (6,143 tests), push gates and independent re-review passed. Lost seed ACKs can still fail closed at the earlier replica read; the description and architecture doc now disclose that limitation.
• DCO is green at the new head. Hosted CI is running; this is not yet a hosted-green claim. PR is draft pending the documented real-relay/native and human acceptance gates.

Timing comparisons and exact validation scope are in the updated description.

[wpfleger96]

🤖 Thanks for the follow-ups. No blocking findings at 8a35727fecd23c7c7888f59b062dcf2e1aa74092.

Both notes from the last review are handled. canvas.read() in capability.ts defaults to a strong read and drops consistency only on an explicit { strong: false }. The only callers that opt out are the Settings preview (ChannelSettingsPanel.tsx:60) and "Save as template" (TemplateSettings.tsx:103), and neither passes its result into a save. The editor (ChannelCanvasDialog.tsx:57), Todos (TodosPanel.tsx:137), the save preflight and post-save selection (capability.ts:286, :312), and setup's Canvas head check (session.ts:1196) all keep the strong default. The new real-outbox case in capability.test.ts covers a lost ACK, then a newer head, then a refused exact retry. The save stays unknown, nothing gets dismissed, and a replacement stays blocked.

The session.ts change only touches setup's separate exact-ID confirmation read, after delivery has settled. workSessions.delivered(id, undefined, false, false) is unchanged, and work-sessions.ts returns or throws before source.retry() when that flag is false, so this can't turn into a republish. The limit in the docs is real: a seed whose ACK was lost can still stop at the earlier replica-eligible delivery check. That leaves setup incomplete, not replayed, and it isn't new here.

I checked that the tests catch regressions. Reverting the strong confirmation read makes both new setup.test.ts cases fail. Removing the opt-out from either display caller fails that caller's component test. These prove the callers are wired correctly, not that reads actually get routed to a replica. Both seed publishes in the setup tests are acknowledged, so flipping the retry flag to true wouldn't make them fail. It's worth adding if you want that flag pinned.

The JavaScript job going from 10 to 15 minutes is fine as a separate change. Workers, per-test deadlines, and coverage are unchanged, and the job took 7m57s on this head.

On Kalvin's shared-predicate note, I'd leave it as is. Both sites require delivery === "failed" plus an error starting with conflict:. capability.ts also checks kind 40100 because its confirmer handles recipes, and setup.ts already holds the Canvas operation ID. Neither treats Retry blocked: conflict: or an unknown outcome as definitive. It's reasonable if you want one helper, but the two checks are clear and equivalent today.

None of this is a two-client race against a relay with block/buzz#6780, a native GUI run, or human acceptance. The description still says "Returned to draft", "new CI is running, not yet claimed green", and "Keep draft", which no longer match the PR.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Pushed 602f25e: merged main, resolved both conflicts, and addressed the remaining predicate suggestion. All three review threads are resolved; GitHub reports MERGEABLE and DCO passed. Full local JavaScript, bounded-concurrency native package, push gates and independent re-review passed; the initial native timing failures are disclosed in the updated description.

New CI is running: https://github.com/block/buzz-app/actions/runs/37028975497 . Required approval and documented live/human acceptance remain outstanding; this is not merge authorization.

[wpfleger96]

🤖 Thanks for the repair. No blocking findings at 602f25ef9cd3cc8aa43eb74a1cdbd56d52ed9584.

isDefinitiveCanvasConflict matches the checks it replaced: kind 40100, delivery === "failed", and an error that starts with conflict:. An unknown outcome or a Retry blocked: conflict: error still doesn't count, so a save whose ACK was lost stays in the outbox and keeps blocking a replacement. Setup's check is now slightly stricter, since it didn't look at the kind before, but that changes nothing in practice because the seed is always created or reused as kind 40100. In relay.rs the merge keeps main's 30177 in the allowlist and the dedicated Canvas validator. Compared with 8a35727f, adding 30177 is the only change in that file. The CI workflow now matches main exactly, and I didn't find anything in the merged main changes that conflicts with the Canvas paths.

Beyond the source read, at this head the full Vitest suite passed (496 files / 6,641 tests), and so did the native package (209 passed, 7 ignored, plus the integration test). Each of these deliberate breaks in the new helper made at least one test fail:

• dropping the kind check (1 failure)
• treating unknown delivery as definitive (3 failures, including Canvas cleanup and seed recovery)
• accepting the Retry blocked: conflict: prefix (1 failure)

I also ran a two-session stale-save race against a fresh local block/buzz relay with #6780, using the PR's Canvas capability and outbox code through signed HTTP requests. In both cases, an existing Canvas and no Canvas yet, session A held an old preflight read, B saved first, and A's save came back 409. B's document stayed the head, A's event never reached relay storage, and only A's rejected outbox entry was dismissed. This didn't go through the browser editor, the native WebSocket transport, or a GUI, so the human acceptance steps in the description are still open.

Small nonblocking note: the Hosted CI row in the description still says the new run is in progress. It's green now.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Pushed dcbe566d: all five merge conflicts resolved, main’s newer UI and the Canvas safety contract preserved, and misleading consistency documentation corrected. All three existing review threads are resolved; independent merge re-review is clear.

The integrated source passed 6,804 JS tests and 18 Chromium/WebKit journeys; final-head push gates passed. GitHub reports no conflicts; DCO is green. The description now credits the earlier reviewer’s local-relay race evidence at its actual head and removes the obsolete setup replica-read limitation.

Still draft: fresh CI, native/human acceptance, and required approval remain. Exact acceptance steps and validation boundaries are in the updated description. Not merged.

[wpfleger96]

🤖 Thanks for this. No blocking findings at 70a711835eaa169d73d37fe30a35b11da252d44d. This review covers the exact head that merged.

I mostly looked at the five files the main merge (e907e2ba) resolved by hand:

• capability.ts is byte-identical to 602f25ef, so Canvas reads still default to strong. No caller at head still passes main's old { consistency } option.
• session.ts drops main's explicit { consistency: "strong" } at the setup canvasHead read. Behavior doesn't change, because strong is now the default. The setup exact-ID confirmation still reads strong.
• TemplateSettings.tsx puts { strong: false } on the shared copy() handler that main added, so both the button and the header-menu Save as template still read from replicas.
• ChannelSettingsPanel.tsx and its test match main exactly. Main removed the Canvas preview, so dropping the PR's replica preview read is correct.

The recipe paragraph in docs/relay-queries.md matches the code. confirm() does strong exact-ID reads, while the recipe head and catalog reads leave consistency unset. That covers Kalvin's note in r4168479720.

Beyond the source read, the full Vitest suite passed at this head (499 files / 6,804 tests). Reverting each hand resolution toward one side failed tests for three of the four source files. Removing the template-copy opt-out failed 1 test, restoring main's Canvas read API failed 7, and restoring the PR's old channel-ID row failed 5. Re-adding main's { consistency: "strong" } at canvasHead passed, but that change does nothing at runtime. Actually weakening that read to { strong: false } failed 2 seed-completion tests. This was fixture-backed Vitest only, not a live relay or the native app.

Small nonblocking note: the comment above the { strong: false } read in agent-selection.test.tsx still says "Ordinary browsing still uses the lagging replica," but ordinary Canvas reads are strong by default now, and template copy is the only replica read left.