Runner.resume: the except ImportError is dead because sinks/__init__.py rebinds the names to None

[pedapudi]

Type: bug. Severity: Runner.resume raises a confusing TypeError instead of the clear RuntimeError the code was written to raise.

Summary

Runner.resume guards its optional-dependency import with try/except ImportError, but the except clause is dead: goldfive/sinks/__init__.py binds the same names to None on import failure, so the import always succeeds and the guard never fires.

goldfive/sinks/__init__.py:41-47:

    from goldfive.sinks.persistence import (
        JSONLPersistenceSink,
        reconstruct_session,
        replay_from_jsonl,
    )
except ImportError:
    JSONLPersistenceSink = None  # type: ignore[assignment]
    reconstruct_session = None   # type: ignore[assignment]
    replay_from_jsonl = None     # type: ignore[assignment]

goldfive/runner.py:1395-1403:

    from goldfive.sinks import reconstruct_session, replay_from_jsonl
except ImportError as exc:  # pragma: no cover
    raise RuntimeError(
        "goldfive.sinks.reconstruct_session is not available; "
        ...
    ) from exc

events = replay_from_jsonl(persistence_path)
session = reconstruct_session(events)

Because the names exist (bound to None), the from ... import succeeds. Control reaches line 1402 and the user gets:

TypeError: 'NoneType' object is not callable

instead of the actionable message the author wrote, which names the missing extra. The # pragma: no cover on the except is consistent with it never having been exercised.

Repro

Install without the persistence extra so goldfive.sinks.persistence fails to import, then call Runner.resume(...). Expected: RuntimeError explaining the missing dependency. Actual: TypeError: 'NoneType' object is not callable.

Proposed fix

Import from the submodule directly, so a genuinely missing dependency raises ImportError and the existing handler works as intended:

    from goldfive.sinks.persistence import reconstruct_session, replay_from_jsonl
except ImportError as exc:
    raise RuntimeError(...) from exc

This restores the error path already written; no behaviour change on the happy path.

Wider point

The = None shim is a reasonable optional-dependency pattern, but it converts every downstream except ImportError into dead code and defers the failure to an unhelpful call-site TypeError. The same rebinding exists for JSONLPersistenceSink, SQLitePersistenceSink, list_runs, replay_from_sqlite, LoggingSink and GRPCSink, so any other module guarding those with except ImportError has the same latent problem.

Two options, either is fine:

• Keep the shim but have consumers import from the submodule (the fix above), and add a note in sinks/__init__.py saying the names may be None so except ImportError will not fire.
• Replace the None shims with a small placeholder that raises the actionable RuntimeError on call, which makes every consumer correct by default without each one having to know.

The second is more invasive but removes a whole class of "why is this None" bugs. Typing the shims Callable[..., X] | None would additionally let a type checker flag call sites that do not guard.