Skip to content

Add a test that marimo notebooks have no unbound names (and fix the one it finds) #500

Description

@JackKelly

Written by Claude Code, reviewed by Jack.

Why

Nothing in the test suite touches the marimo notebooks, and ruff/ty/pytest all pass on a
notebook that is broken at runtime. This is not hypothetical: in
#494 ruff's UP017 autofix
inserted from datetime import UTC at module top level in packages/notebooks/plot_nwp_map.py
and packages/notebooks/plot_missing_NWP_data.py, while datetime stayed in with app.setup:.
Marimo does not execute module-level statements — it reconstructs the notebook from app.setup
plus the @app.cell functions — so UTC was never bound in the cell namespace and both notebooks
would have raised NameError on open. All four gates passed. It was caught by a reviewer reading
the diff, which is not a control we should rely on.

CLAUDE.md already states the rule this violated ("Put every import in the with app.setup: block,
never at module top level"), so the gap is enforcement, not documentation.

Part 1 — add the check

Marimo exposes cell.refs and cell.defs, so a test can assert that every name a cell references
is defined by some other cell or by app.setup. Prototyped and validated: it flags UTC in cells
3 and 4 of the reconstructed broken plot_nwp_map.py, and passes on the fixed file.

import builtins
import pathlib

from marimo._ast.load import load_app


def unbound_names(path: pathlib.Path) -> dict[int, list[str]]:
    """Cell index → names the cell references that nothing in the notebook defines."""
    app = load_app(str(path))
    cells = [(i, c.cell) for i, c in enumerate(app._cell_manager.cell_data()) if c.cell]
    defined = set(dir(builtins))
    for _, cell in cells:
        defined |= set(cell.defs)
    return {i: sorted(set(cell.refs) - defined) for i, cell in cells if set(cell.refs) - defined}

Parametrise over packages/notebooks/*.py and packages/dashboard/*.py (the two dashboard apps
are marimo notebooks too; packages/dashboard/src/ is ordinary library code and must not be
swept in). Root tests/ is the natural home, since the check spans two packages.

Two things to be aware of when implementing:

  • Key cells by index, not by name. Almost every cell is named _, so a {cell.name: cell}
    dict collapses them — which produces both false positives and false negatives. This bit the
    prototype.

  • marimo._ast.load and cell.refs/cell.defs are private API. A marimo upgrade could break
    the test itself. That is an acceptable price here, but the test should fail loudly rather than
    silently degrade to a no-op if the API moves — e.g. assert that a known-good notebook parses to
    a non-empty cell list before checking anything.

Part 2 — fix the pre-existing bug it finds

Once the check exists it goes red immediately on one notebook:
packages/notebooks/plot_missing_NWP_data.py:42
declares def _(df, nwp_vars), but df is defined nowhere in the notebook, so the notebook cannot
run standalone. Confirmed present on main, so it predates #494 — presumably a scratch cell that
defined df was deleted at some point. nwp_vars is fine; df is the only dangling reference.
Deciding what that cell should read is a judgement call about what the notebook is meant to plot,
which is why it is bundled here rather than fixed blind.

The other five notebooks are clean, so the suite goes green once this one is dealt with.

Scope note

This is a static name-binding check, not execution. It catches misplaced imports and dangling cell
inputs; it will not catch a notebook that binds every name and still fails inside a Polars or
Altair call. Executing the notebooks in CI is not viable — they read real Delta tables and S3.

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions