Skip to content

fix(watcher): keep the open-file revert off a running kernel and announce it (#429) - #430

Open
pjdoland wants to merge 2 commits into
plmbr:mainfrom
pjdoland:fix/429-revert-inflight-output
Open

pjdoland wants to merge 2 commits into
plmbr:mainfrom
pjdoland:fix/429-revert-inflight-output

Conversation

@pjdoland

Copy link
Copy Markdown
Collaborator

Summary

The open-file refresh watcher added in #330 reverts an open document whenever the file is newer on disk, which is what makes an agent's edits appear without a manual reload. It is on by default (refresh_open_files_on_disk_change, notebook_intelligence/config.py:248-250). Two gaps, both user-visible:

  1. A revert could land while a cell was still executing, replacing the model under the running execution so the result the user was waiting for went nowhere. No prompt, no way back.
  2. The revert was completely silent. onRevert was declared and invoked but never supplied at the only call site, so the user saw their notebook jump to the top with the cursor moved and no explanation. That reads as data loss rather than as a sync feature.

Solution

The kernel guard. The revert decision takes a new isKernelBusy input and skips while the document's kernel is busy, read from the sessionContext that DocumentRegistry.IContext already exposes, so nothing new is threaded through IRefreshWatcherEnv. The post-decision re-check consults it beside the existing dirty and disposed re-reads, matching that guard's documented defense-in-depth rationale.

Worth being precise about why this is needed, because the obvious reason is wrong. Starting an execution marks the model dirty, so the existing isDirty rule covers a run on its own. The gap is autosave: JupyterLab saves every 120s by default (options.saveInterval || 120 in @jupyterlab/docmanager/lib/savehandler.js), and a save landing mid-execution clears dirty while the kernel keeps working. Any cell running longer than that interval spends the rest of its life clean and busy, with nothing suppressing the revert. An earlier framing of mine, that a computing cell leaves the model clean because no output has landed yet, was disproved in the live session below and is not what this guards.

'busy' is a deliberately conservative proxy: the kernel also reports it for completion and kernel-info requests, not only cell execution. That errs toward skipping a revert, which is the safe direction, and the poll retries seconds later. The optional chain degrades to "not busy" at every level, so a document with no kernel (every plain text file the watcher walks) still refreshes.

The notification. onRevert is now wired to Notification.info naming the reloaded file, informational rather than a warning since reloading is the feature working as intended.

Testing

tsc --noEmit clean; jest 428 passed (27 in the watcher suite, 5 new); eslint, stylelint, prettier clean; pytest 1753 passed (Python untouched here).

New unit tests pin ordering rather than a single call: busy skips, idle proceeds so the guard cannot latch, a document with no kernel still reverts across all three null shapes of the optional chain, and the kernel going busy during the in-flight disk fetch is caught by the post-decision re-check.

Verified in a live JupyterLab against a freshly built bundle, which is load-bearing in this file specifically: #330 shipped the Area = 'down' bug because JupyterLab's TypeScript union lists an area its runtime switch throws on, and this change likewise depends on runtime behavior the types do not describe.

  • A 600s cell was started (kernel busy, model dirty), then saved mid-run to stand in for autosave, reaching the clean-and-busy state (dirty: false with execution_state=busy confirmed from both the REST API and the client).
  • The file was then edited on disk (last_modified strictly newer) and observed for 12 seconds, four poll ticks at the 3s cadence, sampling every 250ms. The document kept its content and no revert fired. That is the window this guard exists for.
  • Separately, with the kernel idle and the document clean, an external edit produced both the reload and the toast Reloaded revert-test.ipynb from disk.
  • Zero console errors throughout.

Risks and follow-ups

  • While a long cell runs, agent edits will not appear until it finishes. That is the intended tradeoff (a stale view beats a destroyed execution), and the guard does not latch: the next poll after the kernel goes idle picks the edit up, which the idle-proceeds test pins.
  • The notification fires once per reverted file per tick, so a multi-file agent run produces one toast per file. Left as-is here to keep the change small; if it proves noisy, a dedupe window or an aggregate count is the cheapest fix.
  • Two open files sharing a basename produce identical messages, since the toast shows PathExt.basename(path) rather than the full path.
  • Review coverage was curtailed. The intended persona review (accessibility/UX, extension architecture, test architecture) was cut short by a session rate limit; one reviewer failed outright and two did not report. The live verification above and a manual pass over the diff carried that weight instead, so a reviewer eye on the notification's UX in particular would be welcome.
  • Adjacent, out of scope: JupyterLab's own "File Changed" conflict dialog defaults focus to Overwrite, so a user pressing Enter discards the agent's on-disk edit.

Closes #429

…unce it (plmbr#429)

The open-file refresh watcher reverts a document whenever the file is
newer on disk, which is what makes an agent's edits appear without a
manual reload. It is on by default. Two gaps, both user-visible.

A revert could land while a cell was still executing. Starting an
execution marks the model dirty, so the existing dirty guard covers a
run by itself; what it does not cover is autosave. JupyterLab saves
every 120s by default, and a save landing mid-execution clears dirty
while the kernel keeps working, so any cell running longer than that
interval spends the rest of its life clean and busy with nothing
suppressing the revert. Reverting there swaps the model out from under
the running execution and the result the user was waiting for lands
nowhere, with no prompt and no way back. The decision now takes an
isKernelBusy input, read from the sessionContext the context already
exposes, and the post-decision re-check consults it alongside the
existing dirty and disposed re-reads. `busy` is a deliberately
conservative proxy, since the kernel also reports it for completion and
kernel-info requests, so this errs toward skipping a revert and the
next poll retries seconds later.

The revert was also completely silent. onRevert was declared and
invoked but never supplied at the only call site, so a user saw their
notebook jump to the top with the cursor moved and no explanation,
which reads as data loss rather than as a sync feature. It now raises
an informational notification naming the file.

Verified in a live JupyterLab against a fresh build rather than argued
from the types, which matters in this file: a 600s cell was started,
saved mid-run to stand in for autosave to reach the clean-and-busy
state, then the file was edited on disk and watched for four poll ticks
at 250ms sampling. The document held its content and no revert fired.
Separately, with the kernel idle and the document clean, an external
edit produced both the reload and the toast "Reloaded
revert-test.ipynb from disk". An earlier premise, that a computing cell
leaves the model clean because no output has landed, was disproved in
that same session and is not what this guards.

Tests pin the ordering rather than a single call: busy skips, idle
proceeds so the guard cannot latch, a document with no kernel still
reverts across all three null shapes of the optional chain, and the
kernel going busy during the in-flight disk fetch is caught by the
post-decision re-check.
)

Review remediation. Four instruments looked at the first version: two
independent UX reviews, a JupyterLab extension architect, and a test
architecture review with measured mutation testing.

The notification fired for every reverted file, including ones the user
cannot see, which contradicts its own justification: the cursor and
scroll position only visibly jump for the document on screen. Worse, all
reverts in a tick run as Promise.all batches with no delay between them,
so an agent rewriting six open files produced six toasts at once, and
JupyterLab renders them with role="alert", an assertive live region, so a
screen-reader user got six interruptions overwriting each other. The
kernel guard added in the previous commit makes that worse rather than
better, because reverts deferred for a busy kernel bunch up and fire
together on the first idle tick. It now notifies only for the active
document; background reverts are silent.

The message carries the full path rather than the basename, matching what
JupyterLab itself does in the closest analogous message: its "File
Changed" dialog interpolates this.path into `"%1" has changed on disk
since the last time it was opened or saved`. Two open files sharing a
basename previously produced identical text. The wording now names effect
and cause while staying agnostic about the writer, since the watcher only
ever sees a newer mtime and a terminal command or git checkout produces
that just as readily as an agent. autoClose moves to 5000, matching
JupyterLab's own toast default and the MCP-save notification above it.

Both behaviors are now pure exported functions rather than an inline
lambda, which is what makes them testable at all; the test review's
suggested fix and the UX fixes wanted the same shape.

Test fixes, all from measured findings rather than reading:

- Mutation testing found that deleting the post-decision kernel re-check
  killed none of the 27 tests. Both kernel reads happen after the disk
  fetch resolves with no await between them, so flipping state before
  release() is already visible to the first read. A fake whose
  sessionContext getter reports idle once and busy afterwards puts the two
  reads on opposite sides of the decision, so the re-check is the only
  thing that can stop that revert. Verified: deleting it now fails exactly
  that test.
- The kernel-less loop became it.each. A failing iteration used to abort
  the test, so the later shapes contributed no coverage and the failure
  never said which shape broke.
- "reverts once the kernel is idle again" is removed. base already carries
  isKernelBusy: false, so it restated a pre-existing test and killed no
  mutant that test did not already kill.
- The in-flight-fetch test's comment no longer claims to cover the
  post-decision re-check, which mutation testing disproved. It now
  discloses that either read satisfies it, as its dirty-flip sibling
  always did.

Not changed, and why: info severity is right (every other call site is a
failure, and a success must not outrank them); kernel.status is the right
signal, since kernelDisplayStatus substitutes connectionStatus when the
socket drops and would mask a real busy execution; silence on a deferred
refresh is correct; and no toast actions, because the pre-revert content
is discarded so there is nothing to diff or undo against.

Tightening the fake's status to the real Kernel.Status union was
considered and dropped: this pipeline runs no type diagnostics on test
files (isolatedModules, and the root tsconfig includes only src), which
the reviewer verified by assigning an invalid literal and seeing it run.

jest 439 passed, tsc clean, eslint/stylelint/prettier clean.
@pjdoland

Copy link
Copy Markdown
Collaborator Author

Pushed a review-remediation commit (6acd044). Recording what changed and why, since the review found a real defect in the first version.

Four instruments looked at it: two independent UX reviews, a JupyterLab extension architect, and a test-architecture review that built the pre-change tree and ran mutation testing.

Fixed

The notification fired for files the user cannot see. That contradicted its own justification, since the cursor and scroll position only visibly jump for the document on screen. It was also worse than one toast per agent run: every revert in a tick runs as Promise.all batches with no delay between them, so six edited files produced six toasts simultaneously, and JupyterLab renders them with role="alert" (an assertive live region), so a screen-reader user got six interruptions overwriting each other. The kernel guard from the first commit aggravates this, because reverts deferred for a busy kernel bunch up and fire together on the first idle tick. Now scoped to the active document; background reverts are silent.

The message showed the basename. JupyterLab's own "File Changed" dialog interpolates the full this.path into the analogous message, and two open files sharing a basename produced identical text. Now the full path, with wording that names effect and cause while staying agnostic about the writer (a terminal command or git checkout produces a newer mtime just as readily as an agent). autoClose moves to 5000 to match JupyterLab's default and the MCP-save site above it.

Both behaviors are now pure exported functions rather than an inline lambda, which is what makes them testable. The test review's suggested fix and the UX fixes wanted the same shape.

Test fixes, from measured findings

  • A coverage gap the mutation run exposed: deleting the post-decision kernel re-check killed none of the 27 tests. Both kernel reads happen after the disk fetch resolves with no await between them, so flipping state before release() is already visible to the first read. A fake whose sessionContext getter reports idle once and busy afterwards puts the two reads on opposite sides of the decision. Verified: deleting the re-check now fails exactly that test and nothing else.
  • The kernel-less loop is now it.each. A failing iteration used to abort the test, so the later sessionContext shapes contributed no coverage and the failure never said which shape broke.
  • reverts once the kernel is idle again is removed: base already carries isKernelBusy: false, so it restated a pre-existing test and killed no mutant that test did not already kill.
  • The in-flight-fetch test no longer claims to cover the post-decision re-check, which mutation testing disproved. It now discloses that either read satisfies it, as its dirty-flip sibling always did.

Considered and deliberately not changed

  • info severity and the toast itself. Every other Notification call site in the repo is a failure; a success must not outrank them. A status-bar item is ambient and the user is not looking at it when their cursor moves, which is the problem being solved.
  • kernel.status over kernelDisplayStatus. The latter substitutes connectionStatus when the socket is not connected, so a websocket blip during a real execution would report connecting and mask exactly the condition this guard exists for.
  • Silence on a deferred refresh, and no toast actions ("Show diff" / "Undo"), because the pre-revert content is discarded so there is nothing to diff or undo against without new plumbing.
  • Typing the fake's status to the real Kernel.Status union. The reviewer verified this pipeline runs no type diagnostics on test files at all (isolatedModules, and the root tsconfig includes only src) by assigning an invalid literal and watching it run. Cosmetic today.

Related, filed separately

#437: terminal-drag.ts:327 and chat-sidebar.tsx:2305 both omit autoClose, which defaults to 0, so two failure notifications never render as toasts at all. Pre-existing and unrelated to this change, but found while reviewing it, and worth knowing that this PR's notification would otherwise have been the only toast in the codebase.

jest 439 passed, tsc clean, eslint/stylelint/prettier clean.

@pjdoland
pjdoland requested a review from mbektas September 14, 2026 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(watcher): open-file revert can discard an in-flight cell execution, and never tells the user

1 participant