code-review

Reviewing a change to Jupyter MCP Server

This server is what an agent talks to when it reads and runs somebody's notebooks. Most of what goes wrong here is quiet: a test that passes without exercising its subject, a tool that answers an empty list instead of an error, a page that documents a flag nothing reads. None of it fails loudly, and all of it has happened.

Review for those first. Ordinary bugs are usually caught by the suite.

Start here: does the test exercise its subject?

The most common defect in this repository is not broken code — it is a test that passes for a reason other than the one it claims. Two real examples:

• test_an_expired_task_is_gone… called store.get() before store.list(). get() deletes the expired record itself, so by the time list() ran there was nothing left to sweep and the sweep never executed. list() was iterating a dictionary while popping from it — a guaranteed RuntimeError — and the test named after that path was green.
• tests/test_mcpb_version_sync.py never ran at all. It imports scripts.sync_mcpb_version, scripts/ had no __init__.py, and the module failed at collection. The suite was green and the version drift it guards was unguarded.

So for every new or changed test, ask:

• Would it fail if the behaviour were removed? If you cannot say yes, delete the assertion or strengthen it. Say so in the review.
• Does an earlier line already do the work? A setup call that cleans up, caches, or short-circuits leaves the code under test unreached.
• Does it assert a cell, or a substring of everything? assert "no" in output passes on almost any English. Pull the row or the field apart.
• Does it signal by raising inside a try? Several functions here catch Exception deliberately — a test that raises to signal gets swallowed and asserts nothing. Record into a list and assert the list.
• Does it set an environment variable that was already read at import? config-style modules read once. Patch the attribute, not the environment.