diff --git a/bin/check_core_floor.py b/bin/check_core_floor.py index a218843..9d03053 100755 --- a/bin/check_core_floor.py +++ b/bin/check_core_floor.py @@ -101,25 +101,71 @@ # (a signature change evaluated at module scope, say), and this would miss it — # but a check that cries wolf is worth less than one with a known blind spot, # and the three escapes it exists to catch were all plain ImportErrors. +# +# ★ THE NON-FLOOR FAILURES SPLIT IN TWO, and the split is the difference +# between a report and a line people learn to ignore. `witan.server` raising +# `omnigraph binary not found` is not a defect awaiting a fix; it is a design +# decision, taken deliberately and load-bearing for something else (see +# EXPECTED_IMPORT_FAILURES). Printing it under the same heading as a genuinely +# unexplained failure asks the reader to re-derive that on every run, and the +# second or third time they do, they stop reading the section. So a known case +# is named as known, with its reason, and only the rest is left open. +# +# The entries are matched on module + exception type + a substring of the +# message, all three. Matching on the module alone would let some future +# unrelated RuntimeError from `witan.server` inherit an explanation that does +# not apply to it, which is the failure mode an allowlist like this has. +EXPECTED_IMPORT_FAILURES = [ + ( + "witan.server", + "RuntimeError", + "omnigraph binary not found", + "witan.server bootstraps the local store at module scope " + "(`_ensure_graph`), which needs the omnigraph binary; `witan setup` " + "installs it. Deliberate: the CLI's local-dispatch guard holds this " + "import unevaluated precisely BECAUSE importing is what touches the " + "store, and that ordering is the agent-kit#261 fix. See " + "witan/server.py::_ensure_graph.", + ), +] + IMPORT_ALL = """ -import importlib, pkgutil, sys, traceback +import importlib, json, pkgutil, sys, textwrap, traceback pkg = importlib.import_module(sys.argv[1]) +expected_spec = json.loads(sys.argv[2]) names = [pkg.__name__] + [ m.name for m in pkgutil.walk_packages(pkg.__path__, pkg.__name__ + ".") ] -floor, other = [], [] +floor, expected, other = [], [], [] for name in names: try: importlib.import_module(name) except Exception as exc: trace = "".join(traceback.format_exception(exc)) - line = f"{name}: {type(exc).__name__}: {exc}" + kind = type(exc).__name__ + line = f"{name}: {kind}: {exc}" missing_name = isinstance(exc, ImportError | AttributeError) - (floor if missing_name and "witan_core" in trace else other).append(line) -ok = len(names) - len(floor) - len(other) + if missing_name and "witan_core" in trace: + floor.append(line) + continue + reason = next( + ( + r + for mod, exc_type, needle, r in expected_spec + if mod == name and exc_type == kind and needle in str(exc) + ), + None, + ) + (expected if reason else other).append( + (line, reason) if reason else line + ) +ok = len(names) - len(floor) - len(expected) - len(other) print(f"imported {ok}/{len(names)} modules") for line in floor: print("FAIL " + line) +for line, reason in expected: + print("EXPECTED (by design; does not fail this check) " + line) + print(textwrap.fill(reason, 74, initial_indent=" > ", subsequent_indent=" ")) for line in other: print("UNRELATED (not a witan_core import; does not fail this check) " + line) sys.exit(1 if floor else 0) @@ -269,7 +315,14 @@ def check(root: Path, floor: Floor, workdir: Path) -> str | None: ) imported = run( - [str(venv / "bin" / "python"), "-c", IMPORT_ALL, floor.package], cwd=workdir + [ + str(venv / "bin" / "python"), + "-c", + IMPORT_ALL, + floor.package, + json.dumps(EXPECTED_IMPORT_FAILURES), + ], + cwd=workdir, ) if imported.returncode != 0: return ( diff --git a/mcp/servers/witan/CHANGELOG.md b/mcp/servers/witan/CHANGELOG.md index 1a596af..2cc9983 100644 --- a/mcp/servers/witan/CHANGELOG.md +++ b/mcp/servers/witan/CHANGELOG.md @@ -6,6 +6,31 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/) (pre-1.0: a MINOR bump may include breaking changes). +## [Unreleased] + +### Changed + +- **`import witan.server` needing the omnigraph binary is now a decision on + record, not an open question.** On a fresh install the module raises + `RuntimeError: omnigraph binary not found`, because `_ensure_graph` creates + the local store at module scope. `bin/check_core_floor.py` printed that on + every run under `UNRELATED`, alongside genuinely unexplained failures, which + is how a report becomes a line people learn to ignore. + + Deferring the bootstrap to first use was weighed and declined. Importing this + module IS a write and the CLI depends on it: `_srv` diagnoses routing before + importing and hands the guard the import unevaluated, so a refused write never + reaches the store — the #261 fix, for a `task close` that reported success + against a graph nobody was reading for nine days. That invariant is checkable + in one line (`"witan.server" not in sys.modules`); "touched on first use" + would not be. + + `_ensure_graph`'s docstring now carries the reasoning, and the floor check + reports this case as EXPECTED with its reason rather than as an anomaly. + Entries are matched on module, exception type and a message substring + together, so an unrelated `RuntimeError` from the same module cannot inherit + an explanation that does not apply to it. No behaviour change. + ## [0.29.1] - 2026-08-24 ### Fixed diff --git a/mcp/servers/witan/witan/server.py b/mcp/servers/witan/witan/server.py index c47f527..428d939 100644 --- a/mcp/servers/witan/witan/server.py +++ b/mcp/servers/witan/witan/server.py @@ -102,6 +102,30 @@ def _ensure_graph(graph_uri: str) -> None: Creation keeps ``check=True``, and lets a missing binary raise — a store that does not exist yet has nothing to degrade to. + + ★ SO ``import witan.server`` REQUIRES THE OMNIGRAPH BINARY ON A FRESH + INSTALL, and that is a decision rather than an oversight. It means a + caller that only wants the module object — a plugin loader, a docs + generator, ``pkgutil.walk_packages``, ``bin/check_core_floor.py`` — gets a + ``RuntimeError`` about a missing binary when it asked about nothing of the + kind. The alternative, deferring the bootstrap to first use, was weighed + and declined: + + Importing this module IS a write, and the CLI depends on that being true. + ``witan.cli._common._srv`` diagnoses local-vs-deployed routing BEFORE + importing, and hands ``local_dispatch.local_server`` the import as an + unevaluated callable, so a refused write never reaches the store it is + refusing to touch. That ordering is the agent-kit#261 fix — a ``witan task + close`` that printed success, exited 0, and wrote to a local store while + the deployed graph still showed the task open nine days later. The + invariant is checkable in one line (``"witan.server" not in sys.modules``, + asserted in ``test_local_dispatch.py``); "the store is touched on first + use" would not be, because several legitimate paths reach the client + without passing the guard. A bright line around a failure mode that hid + for nine days is worth more than the import ergonomics it costs. + + ``bin/check_core_floor.py`` reports this case as EXPECTED, naming this + function, so its output does not re-ask a settled question on every run. """ if graph_uri.startswith(("http://", "https://", "s3://")): return