feat(sync): remote heads tracking for confirmed cell mutations

[rgbkrk]

Adds sync_to_daemon_confirmed that checks peer_state.shared_heads includes our local heads after a sync round-trip. Cell mutations stay fire-and-forget with sync_to_daemon — confirmation is opt-in at the execution boundary.

Python bindings call confirm_sync() before sending ExecuteCell, fixing the create_cell → execute_cell race. WASM edits are unaffected.

Phase A of the sync improvements plan.

PR submitted by @rgbkrk's agent Quill, via Zed

[Copilot]

Pull request overview

This PR strengthens notebook sync correctness by making cell mutation APIs wait for daemon-side confirmation that local Automerge heads have been merged, addressing a create_cell → execute_cell race in the document-first architecture.

Changes:

• Switched key cell mutation methods (add_cell, add_cell_with_source, delete_cell, move_cell, update_source, append_source) to use a new confirmed sync helper.
• Added sync_to_daemon_confirmed(), which performs additional sync rounds until peer_state.shared_heads includes the local heads (bounded retries), then logs if confirmation isn’t reached.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

sync_to_daemon_confirmed returns Ok(()) even when the daemon never confirms our heads (after retries it only logs). That undermines the method’s stated goal and can still allow the create_cell → execute_cell race when confirmation doesn’t arrive in time. Consider returning an error (e.g., NotebookSyncError::Timeout/SyncError) or changing the API to return a boolean so callers can decide whether to proceed with execute requests when confirmation fails.

Suggested change

	// Best-effort: even if not fully confirmed after retries, the
	// changes are very likely applied. Log and continue rather than
	// failing the mutation — a hard error here would be worse than
	// the original race.
	log::debug!(
	"[notebook-sync-client] sync_to_daemon_confirmed: heads not fully confirmed after retries (our_heads={}, shared_heads={})",
	our_heads.len(),
	self.peer_state.shared_heads.len()
	);
	Ok(())
	// We failed to observe the daemon confirming all of our heads within
	// the bounded number of sync rounds. This means we cannot uphold the
	// contract that this method only returns `Ok(())` once our changes are
	// confirmed, so surface this as an error to the caller.
	log::debug!(
	"[notebook-sync-client] sync_to_daemon_confirmed: heads not fully confirmed after retries (our_heads={}, shared_heads={})",
	our_heads.len(),
	self.peer_state.shared_heads.len()
	);
	Err(NotebookSyncError::SyncError(
	"sync_to_daemon_confirmed: heads not fully confirmed after retries".into(),
	))

[Copilot]

The retry loop checks shared_heads before doing the next sync round. If confirmation only becomes true on the last sync_to_daemon() call inside the loop, it won’t be re-checked and the function will fall through to the "not fully confirmed" path. Consider restructuring so each retry performs a sync and then re-checks (or add a final check after the loop).

Suggested change

	// Final check after the last sync attempt in case confirmation
	// only became true on the final round.
	let shared = &self.peer_state.shared_heads;
	if our_heads.iter().all(|h| shared.contains(h)) {
	return Ok(()); // Daemon has confirmed all our changes
	}

[Copilot]

This new confirmation behavior is core to correctness (prevents create_cell → execute_cell races), but it isn’t covered by tests in this file. Adding a unit/integration test that simulates multiple sync rounds and asserts the method blocks until the expected heads are present in peer_state.shared_heads would help prevent regressions.

[Copilot]

The confirmation retry count (0..5) is a magic number here. Consider extracting it (and possibly a total confirmation timeout) into a named constant/config so it’s easier to reason about and tune alongside sync_to_daemon’s internal 500ms deadline.