Skip to content

feat: agent background edits on notebooks#586

Open
emrberk wants to merge 30 commits into
feat/notebook-cell-toolbar-redesignfrom
feat/agent-passive-notebook-edits
Open

feat: agent background edits on notebooks#586
emrberk wants to merge 30 commits into
feat/notebook-cell-toolbar-redesignfrom
feat/agent-passive-notebook-edits

Conversation

@emrberk

@emrberk emrberk commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Depends on #581

Tandem PR: questdb/mcp-bridge#1

What

Agents could previously only edit the notebook the user had open, and was stealing focus of the users while editing a notebook. This PR lets the MCP bridge and the built-in assistant work on any notebook in the background.

  • The agent edits a notebook in the background, without stealing the focus
  • The user gets notifications about the agent work, and can navigate to the last edit using the notification popper, or MCP pair popper
  • The agent has activate_notebook tool for navigating the user if requested

How it works

  • Dexie controller (notebookDexieController.ts): every op on an unmounted buffer is a queued read → pure transition → write on the persisted buffer row. No mounting, no hidden React tree.
  • Per-buffer FIFO queue (notebookBufferQueue.ts): all writers (agent ops, mount seed read, unmount flush, debounced persists) are strictly ordered per buffer — no lost updates at the mount/unmount boundaries.
  • Mount-claim protocol: when the user opens a buffer mid-edit, the claim + seed read enqueue atomically; in-flight agent ops fail with a typed mounted_mid_edit error telling the agent to re-sync and retry.
  • Headless run_cell (notebookHeadlessRun.ts): runs cells on unmounted notebooks with a verification outcome machine (notebook/cell deleted, user mounted mid-run, cell changed mid-run, superseded) — results are persisted and reported as unverified with a note when the run can't be attributed cleanly.
  • Freshness gating: mutating tools are rejected with STATE_STALE if the user edited the notebook after the agent's last read (per-buffer action sequence, baseline captured before the read). apply_notebook_state is deliberately not up-front gated — it's a wholesale PUT; it keeps its internal mid-apply staleness re-check.
  • activate_notebook tool: brings a buffer to the foreground and reveals a cell (used on user request; intentionally ungated — accepted risk).
  • Footer notifications: agent edits to non-active buffers surface as an unread-style "(new changes)" state on the MCP pill (cyan accent + blink) with a transient popper and a persistent row in the pair popover; cleared when the popover is opened or the user visits the buffer.

Testing

  • New unit tests: queue ordering, mount-claim races, headless run outcomes, freshness baselines, storage cap (max 10 notebooks' results), NOTICE mapping. Suite: 1,418 passing.
  • New e2e spec agentBackgroundEdits.spec.js + shared mcpFakeWebSocket.js test util.
  • Typecheck, lint, build green.

emrberk and others added 21 commits July 8, 2026 14:17
…CellType fallback ladder

Completes Phase 5's read-method carve-out. createModelToolsClient is the sole
client and always provides getCellBasics, so the per-field getCellSql/getCellType
fallbacks in dispatch and applyNotebookState were unreachable in production.
Removes the interface optionals, both method bodies, the orphaned findCellSoft
helper, and rewrites the tests that relied on the fallback onto getCellBasics.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KQfYWS6dFLNBxZ2ncyTNyW
…e-export cleanup

Un-exports six notebooks/ module symbols referenced only within their own file,
removes the always-false `activate` knob from the create/duplicate options and
both call sites, fixes four import-then-re-export cases by pointing consumers at
each symbol's real source, repairs a leaked comment fragment in bufferOwnership,
and removes the obsolete SIMPLIFY_PLAN.md (UNIFY_PLAN.md already gone).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KQfYWS6dFLNBxZ2ncyTNyW
On a mounted notebook, the live runCell wrapper inferred its result by
object-identity diff against the pre-run result. When a user's manual Run
superseded the agent's in-flight run, the cell then held the user's result,
which the wrapper misattributed to the agent — returning success:false/pending
with no unverified note for a write that had actually committed, so the agent
could re-run it and duplicate the DML.

Thread a `superseded` flag out of the live run (useCellExecution → provider
runCell) as a `CellRunOutcome`, and on supersession have the controller return
the SUPERSEDED_RUN_NOTE the headless path already emits, instead of reading the
newer run's result back. Adds Given/When/Then tests for the live runCell
supersede, normal-record, and no-op cases.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KQfYWS6dFLNBxZ2ncyTNyW
…tch (phase 5)

Lands phase 5 additively on top of the #1 supersede fix and the phase-6 sweep,
reconciled so both survive.

- NotebookController -> { bufferId, kind, mutate, readView, runCell }; drop the
  named ops and makeNotebookControllerOps; dispatch builds a transition and
  calls withBoundNotebook(ctrl.mutate) / readView directly
- strip ModelToolsClient to the quest + workspace surface; delete the
  isLiveController pre-checks (validation already lives in the transitions)
- move read serializers into notebookSnapshot; buildSnapshot reads via readView
- preserve the live runCell CellRunOutcome/superseded reporting from the #1 fix
  in the collapsed wrapper, plus its re-homed unknown_cell guard
- keep the sweep's NotebookToolError source import and the dropped activate knob

Original phase-5 authored on machine A; reconciled onto origin here.

Co-Authored-By: emrberk <emreberk5699@gmail.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KQfYWS6dFLNBxZ2ncyTNyW
list_cells (summarizeCells) omitted the cell `name`, while get_cell and
get_notebook_state return it — an inconsistency a model/caller could trip on
when correlating cells by name. Surface `name` on NotebookCellSummary when set.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KQfYWS6dFLNBxZ2ncyTNyW
…ss the read-only gate

A write the validator rejects (e.g. a type-mismatched INSERT) returns an error
result, so classifyStatements maps it to `ERROR`, not `DDL_DML`. The permission
gates only denied on `DDL_DML`, so an ERROR-classified write PASSED the gate and
was sent to /exec — verified live via MCP: a read-only agent's INSERT reached the
server (blocked only by demo's own 403; a writable instance would have run it).

Fail closed: under read-only permission, only confirmed DQL may execute. Any
statement the validator can't confirm as a read (klass ERROR) is treated as a
possible write — run_query / run_cell deny it, add_cell auto-run skips it. The
validator's error is surfaced in the message so the agent knows why. classifyStatements
now carries the validator error on ERROR statements.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KQfYWS6dFLNBxZ2ncyTNyW
…n't bypass the read-only gate"

This reverts commit 0569fd4.
Comment thread e2e/utils/mcpFakeWebSocket.js
@emrberk

emrberk commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator Author

Review: PR #586 — feat: agent background edits on notebooks

Review level: 3.

The checked-out HEAD is 00cdf7e. The remote PR advanced to def1703 during review; that follow-up was inspected and does not resolve the retained findings. Full quality gates ran on the checked-out HEAD.

Issues

ID Issue Category Severity Location Description / reproduction Suggested fix
#1 Stale rows under new SQL Query execution & data integrity Moderate in-diff Background SQL transitions in src/utils/notebooks/notebookController/notebookTransitions.ts:97 do not invalidate persisted result snapshots, while hydration in src/scenes/Editor/Notebook/NotebookProvider.tsx:230 restores solely by cell ID. Run a SELECT, let the agent change the SQL while the notebook is inactive, then reopen it: rows from the old SQL appear under the new SQL. Clear/delete snapshots whenever source SQL changes and revision-tag snapshots so hydration can reject mismatches.
#2 Globals change during run Query execution & data integrity Moderate in-diff Headless execution in src/utils/notebooks/notebookHeadlessRun.ts:211 captures variables, releases the queue, then commits without comparing current variables. Concurrent bulk apply can change @sym while an old run executes, leaving results produced with old globals under new settings. Include normalized variables in the execution fingerprint checked before commit; return an unverified outcome on mismatch.
#3 Obsolete read seeds reconnect State & context architecture Moderate in-diff One freshness tracker is reset and reused across MCP reconnections. An old unabortable workspace read can finish after reset and record freshness in src/utils/mcp/dispatchMCPTool.ts:177, allowing the new connection to mutate a notebook it never read. Allocate a tracker per connection generation; propagate cancellation/session identity through snapshot reads and reject obsolete recordRead calls.
#4 Cross-tab writes lose edits Persistence & migrations Moderate in-diff Queues and claims in src/utils/notebooks/notebookController/notebookController.ts:228 are tab-local, while both tabs replace the complete shared IndexedDB notebook view without revision checking. A background agent edit in tab A can be silently overwritten by tab B's next persistence write. Add persisted revisions and transactional compare-and-swap or patch-based commits; coordinate mounted ownership across tabs with Web Locks/BroadcastChannel.
#5 Activation ignores cancellation Async, timers & cancellation Moderate in-diff activate_notebook in src/utils/tools/dispatch.ts:449 drops its signal. MCP cancel, deadline, reconnect, or permission revocation can suppress the result while the tab still switches, focus metadata persists, and reveal events fire. Thread AbortSignal through dispatch, client, workspace, queue reads, persistence, and navigation; recheck before every side effect.
#6 Archive races passive commit Persistence & migrations Moderate in-diff commitView in src/utils/notebooks/notebookDexieView.ts:115 updates an archived row because archive bypasses the buffer queue. An update_cell can report success after the user archives the notebook; the hidden edit appears when restored. Queue archive/delete with passive mutations and atomically reject archived/deleted rows inside the commit transaction.
#7 Applied state reported failed Async, timers & cancellation Moderate in-diff apply_notebook_state in src/utils/tools/applyNotebookState.ts:373 can commit durably, then observe cancellation during its post-apply read and return a generic error with no indication the PUT succeeded. A retry is ambiguous. Treat mutation completion as a commit point and return state_applied:true, post_apply_aborted:true rather than a generic failure.
#8 Archived previews no longer open Cross-context caller impact Moderate in-diff Search still temporarily activates archived notebooks, but the new durable mount rejects archived rows at src/scenes/Editor/Notebook/index.tsx:793. Double-click unarchive retains the same key/effect dependencies, so the error remains until switching away and back. Add a read-only archived-preview mount path and explicitly remount/reseed after restoration.
#9 Hide result preserves freshness Cross-context caller impact Moderate in-diff The new hydration-time hide-result handler in src/scenes/Editor/Notebook/cells/Cell.tsx:798 clears model-visible persisted run status without advancing the notebook sequence. A subsequent agent mutation can pass freshness using state read before the clear. Call signalUserEdit(bufferId) before user-origin result clearing; cover clear and cancellation paths with freshness tests.
#10 Headless scripts are unbounded Performance & rendering at scale Moderate in-diff Headless execution in src/utils/notebooks/notebookHeadlessRun.ts:146 immutably clones a q-entry result list q times—Θ(q²)—and caps each statement separately rather than the whole cell. Hundreds of results can consume hundreds of MB before IndexedDB pruning. Accumulate into one private mutable array, commit once, enforce an aggregate result budget, hard-cap oversized rows, and limit statement count.
#11 Background work triggers foreground churn Performance & rendering at scale Moderate in-diff A mounted snapshot performs a redundant full Dexie view clone at src/utils/ai/notebookSnapshot.ts:162, then discards it for live state. Every passive commit also invalidates EditorProvider's whole-table live query, rerendering all useEditor consumers. Fast-path live-controller metadata; separate notebook payloads from globally observed buffer metadata and split/select editor contexts.
#12 Notification state loses events React correctness & hooks Moderate in-diff Three verified paths exist in src/scenes/Footer/MCPBridgeStatus/useAgentChanges.ts:38: dismissing once suppresses all later poppers; failed/rejected View clears or strands the notification; and a passive-effect-synchronized ref lets an older View completion clear a newer edit under React 17 scheduling. Dismiss only the current event, retain state on failed activation, catch rejections, and update the identity ref synchronously or in a layout effect.
#13 Agent deletion is silent State & context architecture Moderate in-diff deleteNotebook in src/scenes/Editor/Notebook/NotebookWorkspaceBridge.tsx:138 correctly suppresses a fake user-action digest but emits no agent activity. An inactive tab disappears into History with no footer notification. Emit a deletion-specific activity containing the label and a Restore/History action; the normal View CTA cannot open archived buffers.
#14 Timed notices hide message Styling & theming Moderate in-diff StatusNotification in src/scenes/Editor/Notebook/result-table/StatusNotification.tsx:107 renders only timings when a DQL result has both notice and timings. The server message remains only in a hidden live region and a non-focusable title. Render notice text visibly alongside the timing summary and add a rendered component test.
#15 Agent-change accessibility breaks Accessibility & UX Moderate in-diff The first populated role=status is appended from a detached portal and may not announce; activation/View and X/Escape remove focused controls without moving focus; one boolean represents hover plus focus, allowing auto-hide while focus remains inside. Keep a persistent empty live announcer, focus the destination/pill after navigation or dismissal, and derive pause from separate hover/focus-within state.
#16 Loading status is empty Accessibility & UX Minor in-diff The delayed notebook loading fallback in src/scenes/Editor/Notebook/index.tsx:835 contains only an aria-hidden spinner inside role=status, exposing no “Loading notebook” text or busy state. Add visible or screen-reader-only loading text and optionally mark a labeled notebook region aria-busy.
#17 Long labels break controls Styling & theming Moderate in-diff Unbounded notebook labels flow into flex text items without min-width:0 or wrapping in AgentChangesPopper.tsx:28 and PairPopover.tsx:524, pushing View/close beyond the card. Add min-width:0, flexible sizing, and overflow-wrap:anywhere to both text elements.
#18 Race coverage gaps Test review & coverage Moderate in-diff The 1,482-test suite does not exercise stale snapshot hydration, reconnect pollution, post-commit abort, archived search preview, failed/newer notification activation, multi-tab conflict, or async ownership races. Add deterministic deferred-promise/IndexedDB integration tests and E2E coverage for these user paths.

False-positives

This section also records real defects excluded from the PR-focused report because they are unchanged from the base.

Category Draft candidate Explanation
Fresh-context adversarial Notebook A's seed leaks into B False positive: production keys <Notebook> by activeBuffer.id, fully remounting state and cancelling A's seed lifecycle.
Code structure & types Missing validator auto-runs sticky draw cells False positive for production: every shipped AI/MCP caller supplies validateSql; the optional direct-call behavior is pre-existing and test-only.
Code structure & types JSON registry cast currently breaks metadata False positive: all 33 definitions are valid. The cast remains a non-blocking validation-hardening opportunity.
Accessibility & UX A→B edits lose A's notification Intended: the PR explicitly promises navigation to the “last edit,” not a notification inbox; A's durable changes remain.
Performance Every bulk event repositions Popper Overstated: 51 events can cause 53 small React renders, but react-popper deep-compares unchanged options and does not repeatedly reposition.
React correctness Notebook grid add-bottom regression Real but pre-existing: switching an unsaved layout to grid and adding a cell at bottom already placed it at the top on the base branch.
Async cancellation Concurrent creates produce tied positions Real but pre-existing; the position computation was already outside the Dexie transaction.
Query integrity Live run A commits under agent-updated SQL B Real but pre-existing; the PR's mitigation remains incomplete, but the same live-route race exists at the merge base.
Persistence Live mutations acknowledge before IndexedDB commit Real but pre-existing; the new passive path improves this, while mounted notebooks retain the old behavior.
Query integrity Async write timeout is treated as verified failure Real and potentially dangerous, but unchanged from base; the new headless runner inherits it.
Query integrity A single huge row bypasses run_query's 1 MB cap Real but identical to base and explicitly reflected by an existing test.
Async cancellation OAuth refresh rejection wedges all queries Real but unchanged in client.ts; the PR only reuses the existing client from headless execution.
Cross-context caller impact Rejected draw/no-op arrow falsely stales state Real but pre-existing; PR narrows freshness to a buffer while broadening which mutations consume it.
Async cancellation delete_notebook ignores cancellation Real but pre-existing. The newly added activate_notebook cancellation defect is retained as issue #5.

Summary

Verdict: approve. The retained findings are edge-case follow-ups rather than merge blockers.

  • 18 findings retained: 17 Moderate and 1 Minor.
  • 14 draft candidates removed or separated as false positives, intentional behavior, or pre-existing defects.
  • Split: 18 in-diff, 0 out-of-diff PR regressions. A mandatory widened caller sweep rechecked all changed export families and unchanged consumers and found no additional broken external contract.
  • Quality gates on checked-out 00cdf7e: typecheck, lint, build, and 1,482 unit tests all passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant