diff --git a/specs/projects/cr_moderate_fixes/architecture.md b/specs/projects/cr_moderate_fixes/architecture.md new file mode 100644 index 0000000..c685482 --- /dev/null +++ b/specs/projects/cr_moderate_fixes/architecture.md @@ -0,0 +1,109 @@ +--- +status: complete +--- + +# Architecture: CR Moderate Fixes + +This project is 17 independent fixes to an existing codebase. There is no new component or data +model, so this document gives the shape of the work, the rules every phase follows and how the +phases depend on each other. The design for each fix — files, functions, code sketches, test names +and what each test asserts — is in the component doc for its phase: + +| Phase | Component doc | Items | +|---|---|---| +| 1 | [components/phase_1_batch.md](components/phase_1_batch.md) | 11.M1, 7.M2, 5.M3, 6.M1, 4.M2, 10.M1, 10.M3, 10.M4 | +| 2 | [components/phase_2_bulk_gate.md](components/phase_2_bulk_gate.md) | 2.M1 | +| 3 | [components/phase_3_route_middleware.md](components/phase_3_route_middleware.md) | 4.M1 | +| 4 | [components/phase_4_docs.md](components/phase_4_docs.md) | 9.M1, 9.M7, 9.M8, 8.M4, 2.M2/4.M3, 9.M6 | + +Every design was prototyped on a scratch copy of the tree at `38c6f3c`, with the framework, +projecttracker and xmlrpc suites passing apart from the test changes each doc names. Line numbers +in the component docs are at that commit; later phases move some of them, so the coding agent +finds the quoted code rather than trusting a line number. + +## 1. Rules for every phase + +- **Commit per phase.** Phases 2 and 3 in particular must land as one commit each, so each can be + reviewed and reverted on its own. +- **No ergonomic change for default callers.** No public signature changes. The only default that + changes on purpose is `seahaven serve`'s bind address (7.M2). +- **Test through the real entry point.** Each behaviour change gets a test through + `world.instance(...)`, the served client, `seahaven check` or pytest, as the component doc names, + as well as any unit test (AGENTS.md). +- **Tests must fail without the fix.** Each component doc names the tests that fail on today's + code, or the mutation that makes them fail. The coding agent confirms this before it attests. +- **Docs follow the code in the same phase.** Any bundled-docs sentence a phase makes false is + fixed in that phase (each component doc lists them). Phase 4 then sweeps the docs once more. +- **Checks.** Every phase runs the full AGENTS.md "Automated checks" list in both environments + (`serve` and `mcp`), and ends synced on `serve`. + +## 2. Phase order and dependencies + +``` +Phase 1 (batch) ──► Phase 2 (gate) ──► Phase 3 (route middleware) ──► Phase 4 (docs) +``` + +- **Phase 1 before phase 3.** Phase 1's `hosts_first` (4.M2) walks `Node.added`. Phase 3 removes + the `parents` map from `composition._walk` and replaces `Node.agent_chain` with a chain per + contributed entry. `hosts_first` does not read either, so the two do not conflict, but phase 3's + seal tests then run against the new hook order. +- **Phase 1's link check (10.M4) before phase 4.** The docs rewrite is then guarded by a test that + checks every relative link and anchor in every bundled page, README and CONTRIBUTING. +- **Phase 2 is independent** of phases 1 and 3 in code (it touches `instances.gate`, `_bulk` and + the module docstring). It sits between them so that the two riskier changes are reviewed one at a + time. +- **Phase 4 last**, so that SH103's restated rationale, the `bulk()` atomicity text and the + history sweep describe the final code. + +## 3. Files touched + +| Phase | Source | Tests | Docs | +|---|---|---|---| +| 1 | `db.py`, `openenv/serve.py`, `cli/serve.py`, `openenv/client.py`, `cli/__init__.py`, `cli/check.py`, `pytest_plugin.py` (comment), `composition.py` (`hosts_first`), `instances.py` (`_run_startup_hooks`) | `test_db.py`, `test_instances.py`, `test_serve.py`, `test_cli_new.py`, `test_client.py`, `test_find_world.py`, `test_cli_check.py`, `test_pytest_plugin.py`, `test_composite_instance.py`, `test_composition.py`, `test_call_log.py`, `test_typed_call.py`, `test_docs.py` | `serving_and_openenv.md`, `reference/cli.md`, `reference/lints.md` (SH501), `reference/api.md` (optional client line), `composition.md` (hook order) | +| 2 | `instances.py` (`gate`, `_bulk`, module docstring) | `test_instances.py` | `serving_and_openenv.md` (the gate section) | +| 3 | `composition.py`, `instances.py`, `world.py`, `handles.py` | `test_composite_dispatch.py`, `test_composition.py` | `composition.md` (Middleware), `reference/lints.md` (SH207) | +| 4 | `instances.py` (bulk docstring and comment only), `worlds/projecttracker/src/projecttracker/tools/_events.py` (docstring only) | — | `db_schema_and_fixtures.md`, `reference/lints.md`, `composition.md`, `projecttracker.md`, `state.md`, `serving_and_openenv.md`, `reference/api.md`, `worlds/projecttracker/README.md`, `worlds/projecttracker/AGENTS.md` | + +Out of bounds for every phase: `src/seahaven/http/` (its behaviour must not change; phase 2's +design keeps HTTP requests ungated), `ui/`, `specs/` (history), and +`worlds/projecttracker/src/projecttracker/schema/*.sql` (a comment edit changes the schema hash +and breaks every fixture). + +## 4. Key technical decisions + +- **11.M1:** `PRAGMA synchronous=OFF` before `journal_mode=WAL` on instance connections and on the + blank-instance builder. Instance directories are deleted by the sweep after a crash, so they need + no durability. `VACUUM INTO` (freeze) is not fsynced at any setting, so fixture durability does + not change. +- **6.M1:** `CliError` gains `line` and `fix`. A helper finds the innermost frame in the world's own + code (the `SyntaxError`'s own filename and line for a syntax error), skipping Seahaven, the + standard library and installed packages. +- **4.M2:** hook order is a depth-first walk in `add_world` order that emits a node only once all of + its distinct hosts have been emitted. It equals today's canonical preorder when nothing is shared + (proof in the component doc). +- **2.M1:** `gate()` is re-entrant per thread through a `threading.local` flag (not a `ContextVar`: + Starlette and `asyncio.to_thread` copy context variables into worker threads). `_bulk` enters + `gate(bypass=True)` before the instance lock: it marks the thread and takes no slot, because + taking a slot would gate HTTP requests and make an open `bulk()` hold a slot. +- **4.M1:** `Contributed` carries the chain of the route that contributed it; chains are built once + per distinct route at the seal. `Node.agent_chain` is removed, and `Node.internal_chain` becomes + `own_chain`, used by handle calls and the root's own tools. Host code through a handle keeps + running only the owner's own chain. + +## 5. Error handling + +No new error types. 6.M1 changes the message, location and fix text of SH501 and the pytest +plugin's import failure. 2.M1's new tests use a gate whose `acquire` raises after a timeout, so a +regression fails the test instead of hanging pytest in `destroy()` at teardown. + +## 6. Testing strategy + +Each component doc lists its tests with names, files and assertions. Across the project: + +- Concurrency tests (phase 2) run their bodies on a fresh thread, because the session-scoped + `receivers` fixture in `tests/test_docs_examples.py` leaves the main thread inside a `bulk()`. +- Tests that need a broken world (6.M1) write it into `tmp_path`: a committed file with a syntax + error would fail `ruff` and `ty`. +- One known environment-only failure, `test_typed_call.py::test_ty_resolves_the_result_type_of_a_ + call_by_reference`, appeared on the scratch copies (ty could not resolve the copy's + environment). It must pass on the real tree. diff --git a/specs/projects/cr_moderate_fixes/components/phase_1_batch.md b/specs/projects/cr_moderate_fixes/components/phase_1_batch.md new file mode 100644 index 0000000..f7d2bc1 --- /dev/null +++ b/specs/projects/cr_moderate_fixes/components/phase_1_batch.md @@ -0,0 +1,350 @@ +# Component: Phase 1, low-risk code and test batch + +Eight items: 11.M1, 7.M2, 5.M3, 6.M1, 4.M2, 10.M1, 10.M3, 10.M4. Each was prototyped on a scratch +copy of the tree at `38c6f3c`; the framework, projecttracker and xmlrpc suites pass with the +changes below apart from the test changes each section names. Line numbers are at that commit. + +## 1. 11.M1 — `synchronous=OFF` before the WAL switch + +**Change.** `src/seahaven/db.py::open_instance` (l.262-267): + +```py +conn = apsw.Connection(str(path)) +_harden(conn) +# An instance directory does not survive a crash: the sweep deletes it. So its +# files need no durability, and at FULL the WAL conversion of a copied fixture +# fsyncs on every creation. +conn.pragma("synchronous", "OFF") +conn.pragma("journal_mode", "WAL") +``` + +Order matters: OFF must precede `journal_mode=WAL`. Measured: WAL switch 1.25 ms -> 0.11 ms on a +bare copy; `world.instance("agency")` median 5.5 ms -> 2.6 ms. A counting VFS +shows 4 `xSync` calls on open+one write at the shipped order, 0 with the new one. + +**Other opens, checked:** +- `open_inspection` — read-only (`mode=ro`), never converts or writes; unaffected, leave it. +- `fixtures.py::_vacuum_into` — runs on the instance connection; SQLite never fsyncs `VACUUM INTO` + output whatever the setting, so freeze durability is unchanged (it was never fsynced). +- `_build`/`_plain_connection` (`build_blank`) — for a *blank* instance this writes the instance's + own throwaway file at default FULL with a rollback journal. **Decision: set `synchronous=OFF` on + the builder too** (same reasoning; a further ~0.8 ms per blank instance, 5.3 -> 4.6 ms). The same + builder also serves `:memory:` builds (`World.__init__`, lint, conformance), where the pragma is a + no-op. Add a blank-instance case to the no-fsync test. +- composite nodes: each node goes through `_open_node` -> `open_instance`, so all get it. +- `bench/results/latest.md` §7 ("about 3 ms per node") will be stale; rerun is optional. + +**Tests.** +- `tests/test_db.py::test_an_instance_connection_is_hardened`: `synchronous` expectation `1` -> `0` + (this is the only existing test that breaks; confirmed on the copy). +- New `tests/test_db.py::test_opening_an_instance_file_does_not_fsync` — register a counting + `apsw.VFS` as default (base `apsw.vfs_names()[0]`, `makedefault=True`, `unregister()` in a + fixture finaliser; `xOpen` returns an `apsw.VFSFile` subclass that counts `xSync`), build a + rollback-journal file with a plain connection, then `open_instance` it and run one committed + `INSERT`. Assert zero syncs. Fails on the shipped order (4 syncs). +- New `tests/test_instances.py::test_every_node_of_a_fixture_instance_opens_without_durability` + (real entry point): freeze a fixture of a composite (`rooted` + one `add_world`), then + `with world.instance(fixture) as live, live.bulk() as ctx:` assert + `ctx.db.conn.pragma("synchronous") == 0` and the same for `ctx.worlds..db.conn`, and + `journal_mode == "wal"` for both. + +## 2. 7.M2 — `seahaven serve` binds loopback by default + +**Change.** +- `src/seahaven/openenv/serve.py`: `DEFAULT_HOST = "127.0.0.1"`; replace the comment with the + constraint: `/ws` has no authentication, so a server is reachable from the network only when + the operator says so (`--host 0.0.0.0`); the scaffolded container passes it itself. +- `src/seahaven/cli/serve.py:29`: help text `(default 127.0.0.1)`. +- `console_url` unchanged (still maps `0.0.0.0`/`::` to loopback). + +**Dockerfile.** `src/seahaven/cli/templates/hub/Dockerfile.tmpl:13` does not run `seahaven serve` +at all: `CMD ["uv", "run", "uvicorn", "$package.openenv_app:app", "--host", "0.0.0.0", ...]`, so it +is unaffected. It is the only Dockerfile/template in the repo. + +**Every place that states the default** (all to change): +- `src/seahaven/docs/serving_and_openenv.md:79` — row becomes `127.0.0.1` | "the address to bind. + Pass `--host 0.0.0.0` to serve on every interface, as a container does". Line 74's example + `seahaven serve --host 127.0.0.1 --port 9000` should become `--host 0.0.0.0 --port 9000`. +- `src/seahaven/docs/reference/cli.md:177` (`0.0.0.0` -> `127.0.0.1`) and the example at l.187 + (same swap as above). +- `tests/test_serve.py:84-85` — the literal pair becomes `("127.0.0.1", 8000)`; l.154 docstring + ("The default bind is `0.0.0.0` ... names loopback") reword: the default is loopback, and + `console_url` owns the `0.0.0.0` translation. Add to that test a `serve(world, host="0.0.0.0")` + case asserting the printed URL is still `http://127.0.0.1:8000/console`. +- `tests/test_cli_serve.py:94` compares with `serving.DEFAULT_HOST` — no change needed. +- `docs/http_apis.md:196` already says `127.0.0.1` (now consistent). + +**New test.** `tests/test_cli_new.py` (next to l.255): the scaffolded hub `Dockerfile` CMD contains +`"--host", "0.0.0.0"` — the container's reachability no longer rides on the default, so pin it. + +## 3. 5.M3 — `step(ListToolsAction())` parses as `ListToolsObservation` + +**Change.** `src/seahaven/openenv/client.py::_parse_result`: + +```py +observation = payload.get("observation", {}) +# A tool list is the one reply with a `tools` field; a reset carries its tool +# count in `metadata`, never at the top level. +model = ListToolsObservation if "tools" in observation else SeahavenObservation +return StepResult(observation=model.model_validate(observation), ...) +``` + +Import `ListToolsObservation` from `openenv.core.env_server.mcp_types`. Rewrite the docstring's last +paragraph ("does not come through here") and trim `list_tools`'s docstring (it still reads the frame +directly and answers `list[dict]`; that stays). + +**Types.** No annotation changes. The client is `EnvClient[..., Observation, SeahavenState]`, +`_parse_result -> StepResult[Observation]`, and `ListToolsObservation` subclasses `Observation`, so +`step()` already types as `StepResult[Observation]`; a caller narrows with `isinstance`. Overloading +`step` per action type is possible but the base class's dual-mode `step` makes it awkward; not +proposed. Confirmed live: today `ValidationError`; patched, `ListToolsObservation` with +`Tool` models, and a following `CallToolAction` step still parses as `SeahavenObservation`. + +**Tests** (`tests/test_client.py`): +- Rewrite `test_the_observation_model_refuses_a_frame_it_does_not_know`: it pins the bug with + `{"observation": {"tools": []}}`. Use an unknown key instead (`{"observation": {"bogus": 1}}`, + `match="bogus"`). +- New unit `test_parse_result_answers_a_tool_list_as_a_list_tools_observation`: frame + `{"observation": {"tools": [{"name": "ping", "description": "d", "input_schema": {}}]}}` -> + `isinstance(..., ListToolsObservation)`, `tools[0].name == "ping"`. +- New live `test_stepping_a_list_tools_action_answers_the_tool_list(world)`: `serving(world)`, + `env.reset()`, `result = env.step(ListToolsAction())`, assert `ListToolsObservation`, `"rows"` in + names and `controller_run_sql` not; then `env.call("rows", sql="SELECT 1 AS n").result` still + works on the same session. +- Optional doc line in `docs/reference/api.md` near l.611: `step(ListToolsAction())` answers a + `ListToolsObservation`. + +## 4. 6.M1 — an import error names its file, line and the right fix + +**Change** in `src/seahaven/cli/__init__.py`: +- `CliError.__init__` gains `line: int | None = None` and `fix: str | None = None` (stored). +- `_import` `except Exception` branch -> `raise _import_failure(module_name, error, root) from error`. + The `SeahavenError` non-DDL SH501 branch also gets a location (same helper) and its own fix. +- `_no_world` raise passes `fix="give the package a world, or point at the one it has"` — the only + place that text remains. +- New helpers: + +```py +def _import_failure(module_name, error, root) -> CliError: + summary = _last_traceback_line(error) # "SyntaxError: invalid syntax" + if isinstance(error, ModuleNotFoundError) and _is_the_world_module(module_name, error.name): + return CliError(f"cannot import {module_name!r}: {summary}", code="SH501", path=root, + fix=f"make {module_name!r} importable (the package the [project] name names, under " + "src/), or point at the world with --world module:attr") + place = _world_frame(error, root) + command = f"python -c 'import {module_name}'" + if place is None: + return CliError(f"cannot import {module_name!r}: {summary}; {command} prints the " + "whole traceback", code="SH501", path=root, fix=f"fix the error {command} ends in") + file, line = place + where = f"{_shown(file, root)}:{line}" + return CliError(f"cannot import {module_name!r}: {summary}, at {where}; {command} prints the " + "whole traceback", code="SH501", path=file, line=line, fix=f"fix the error at {where}") + +def _world_frame(error, root) -> tuple[Path, int] | None: + if isinstance(error, SyntaxError) and error.filename and error.lineno \ + and Path(error.filename).is_file(): + return Path(error.filename).resolve(), error.lineno # not in the tb frames + frames = [f for f in reversed(traceback.extract_tb(error.__traceback__)) if _authored(f)] + under_root = [f for f in frames if Path(f.filename).resolve().is_relative_to(root)] + chosen = (under_root or frames or [None])[0] + return None if chosen is None else (Path(chosen.filename).resolve(), chosen.lineno) +``` + +`_authored(frame)`: the file exists (drops ``), and is not under the `seahaven` +package directory, `sys.prefix` or `sys.base_prefix` (drops a project-local `.venv`, the stdlib and +installed libraries). `_is_the_world_module`: `name == module_name or +module_name.startswith(name + ".")`. `_shown` is the path relative to `root` when under it. + +`src/seahaven/cli/check.py::_finding_for`: `Finding(..., path=path, line=error.line, +fix=error.fix or )` and delete the comment about the shared fix. +`src/seahaven/pytest_plugin.py`: no code change — it prints `str(error)`, which now carries the +location and the traceback hint; update its comment. Same for `main()` (serve, fixture, mcp). + +Prototype on a copy of `tests/worlds/tidy`: a `def broken(:` in `tools/notes.py` gives +`src/tidy/tools/notes.py:23`; a bare `undefined_name` gives the same; a bad `World(...)` call in +`__init__.py` gives `src/tidy/__init__.py:9`; a missing top-level module gives no frame. + +**Tests.** The broken worlds are written into `tmp_path` by the test (a committed file with a +syntax error would break `ruff format --check`, `ruff check` and `ty`): copy `WORLDS / "tidy"` with +`shutil.copytree(..., ignore=ignore_patterns("__pycache__"))` and append the bad line. Modules use +`isolated_imports`, which already purges worlds under a temp dir. +- `tests/test_find_world.py::test_a_syntax_error_in_world_code_names_its_file_and_line` — `discover` + raises `CliError` with `code == "SH501"`, `path == .../src/tidy/tools/notes.py`, + `line == `, `"SyntaxError" in message`, `"src/tidy/tools/notes.py:" in message`, + `"world = seahaven.World" not in (fix + message)`. +- `...::test_a_name_error_in_world_code_names_its_file_and_line` — same for `NameError`. +- `...::test_a_missing_dependency_is_placed_at_the_import_that_needs_it` — `import not_installed_x` + in `tools/notes.py`: location is that line (distinguishes it from the missing-world case). +- Keep `test_a_world_option_naming_a_module_that_does_not_import_is_sh501`; add + `assert "--world" in raised.value.fix`. +- `tests/test_cli_check.py::test_an_import_error_is_a_finding_at_the_line_that_raised` — via + `check(tmp_world, monkeypatch, capsys)` (the real `seahaven check` path): exit 1, one line + matching `FINDING_LINE`, starting `SH501 error src/tidy/tools/notes.py:`, `fix: fix the error + at src/tidy/tools/notes.py:`. Rename/extend `test_an_import_error_is_sh501_with_the_last_ + traceback_line` (still asserts no `\n`). +- `tests/test_cli_check.py::test_a_package_with_no_world_is_sh501` — add + `"fix: give the package a world" in line` (pins that the old fix stays there only). +- `tests/test_pytest_plugin.py::test_a_world_that_does_not_import_fails_with_its_file_and_line` — + `write_world(pytester)`, append `"\nundefined_name\n"` to `src/tinyworld/__init__.py`, a test + using `instance` with a marker; `runpytest()` -> `assert_outcomes(errors=1)`, and + `result.stdout.fnmatch_lines(["*NameError*src/tinyworld/__init__.py:*"])`. +- Docs: `docs/reference/lints.md:357-358` — an import that fails for another reason is reported with + the exception's type and message at the file and line in the world's code that raised it, and the + fix names that place; `python -c "import "` prints the whole traceback. + +## 5. 4.M2 — startup hooks in a hosts-first order + +**Algorithm** (new `composition.py::hosts_first(composition) -> list[Node]`, exported; replaces +`canonical_tree` in `instances.py::_run_startup_hooks`): + +```py +remaining = {node: 0 for node in composition.nodes} +for node in composition.nodes: + for child in dict.fromkeys(node.added.values()): # distinct children, add_world order + remaining[child] += 1 # = number of distinct hosts +order = [] +def visit(node): + order.append(node) + for child in dict.fromkeys(node.added.values()): + remaining[child] -= 1 + if remaining[child] == 0: # its last host has just run + visit(child) +visit(composition.root) +``` + +A depth-first preorder from the root in `add_world` order, where a node is emitted only when the +last of its hosts has been emitted (Kahn's algorithm with a DFS stack). `node.added` holds every +edge, alias edges included. `dict.fromkeys` dedups a host that adds one node twice under two names. + +**Correctness.** (a) Every node is emitted: the graph is a DAG, and a node whose hosts are all +emitted reaches `remaining == 0` during the last host's visit. (b) Hosts first: a node is emitted +only after its last host's `visit` began, and each host was emitted when it began. (c) Once each: a +node is visited only on the transition to 0. (d) **Same as today without sharing:** with no node +reached by two distinct hosts, every non-root node has `remaining == 1`, so `visit(child)` runs as +soon as its only host reaches that edge, in `add_world` order — exactly `canonical_tree`, which in +that case descends every edge because every route equals the child's path. By induction on depth, +the two sequences are identical. A node added twice by *one* host keeps today's order too, because +hosts are counted distinctly. Add an assert `len(order) == len(composition.nodes)`. + +Examples: company adds payments, shop; shop adds payments -> `company, shop, payments` (today +`company, payments, shop`). `test_a_node_two_hosts_reach_runs_its_hooks_once` (host adds middle, +shared; middle adds shared) stays `host, middle, shared`. + +Keep `canonical_tree` (still exported; only this call site used it). Update `_run_startup_hooks`'s +docstring and `docs/composition.md:401-405`: "Hooks run once per node, from the root: a node's own +hooks, then its added worlds in `add_world` order, and a world that more than one world adds runs +after every world that adds it." + +**Tests** (`tests/test_composite_instance.py`; the prototype passes these and all suites): +- `test_a_shared_node_runs_after_every_world_that_adds_it` — the company/payments/shop shape above + with `recording`, `tool_prefix` on one route; assert `["company", "shop", "payments"]`. Fails on + today's code (verified). +- `test_a_host_that_seeds_a_shared_node_seeds_it_composed_as_alone` — shop's hook writes + `ctx.worlds.payments.state["plan"] = "merchant"`; payments' hook does + `ctx.state.setdefault("plan", "free")` and records it; assert `"merchant"` both for + `shop.instance(None)` and for `company.instance(None)`. +- Unit, `tests/test_composition.py::test_hosts_first_is_the_canonical_preorder_without_sharing`: + for a three-level tree with no sharing, `hosts_first(resolve(root)) == + list(canonical_tree(resolve(root).root))`. + +## 6. 10.M1 — the call-log copy test that can fail + +Replace `tests/test_call_log.py::test_a_tool_that_mutates_its_arguments_does_not_alter_the_record` +with two tests, both through `world.instance(None)`: + +```python +def test_a_caller_that_changes_its_list_after_the_call_does_not_alter_the_record(tmp_path): + world = build_world(tmp_path) + @world.tool + def count(ctx: Ctx, tags: list[str]) -> int: + """Count the tags.""" + return len(tags) + with world.instance(None) as instance: + given = ["a", "b"] + assert instance.call("count", tags=given) == 2 + given.append("later") + (record,) = instance.call_log() + assert record.arguments == {"tags": ["a", "b"]} + +def test_a_middleware_that_changes_an_argument_in_place_does_not_alter_the_record(tmp_path): + ... # same tool, plus: + @world.middleware + def extend(ctx: Ctx, call: Call, next_: Handler) -> object: + call.arguments["tags"].append("added") # the caller's list, before validation + return next_(ctx, call) + # instance.call("count", tags=["a", "b"]) == 3; record.arguments == {"tags": ["a", "b"]} +``` + +Verified on the copy: both pass as shipped; with `given = arguments` at `instances.py:767` both fail +(and every existing test in the module still passes, confirming the finding). A shallow +`dict(arguments)` also fails both, so they pin the deep copy. + +## 7. 10.M3 — typed call by reference through a handle, diamond + +Add to `tests/test_typed_call.py` after `test_a_handle_reaches_a_function_of_a_world_in_its_subtree`: + +```python +def test_a_handle_reaches_a_function_of_a_node_two_routes_beneath_it_reach(tmp_path): + """One store reached twice under one scope is one account: the reference is not ambiguous.""" + host, _w, _r = rooted("host", tmp_path) + middle, _mw, _mr = rowed("middle") + left, _lw, _lr = rowed("left") + right, _rw, _rr = rowed("right") + leaf, leaf_write, leaf_read = rowed("leaf") + left.add_world(leaf, name="leaf") + right.add_world(leaf, name="leaf", tool_prefix="r_") # two routes, one contribution each + middle.add_world(left, name="left") + middle.add_world(right, name="right") + host.add_world(middle, name="middle") + + @host.tool + def reach(ctx: Ctx, value: str) -> list[str]: + """Reach the shared grandchild's store through the child, by reference.""" + ctx.worlds.middle.call(leaf_write, value=value) + return ctx.worlds.middle.call(leaf_read) + + with host.instance(None) as live: + assert live.call("reach", value="once") == ["once"] + assert live.call("leaf_read") == ["once"] + assert live.call("r_leaf_read") == ["once"] # both routes read one store +``` + +The diamond has to sit *under* the handle (`middle`), since `_subtree` starts at the handle's node. +Verified: passes as shipped; with the `seen` check removed it raises `names more than one tool ... +(leaf_write at middle/left/leaf, leaf_write at middle/left/leaf)`. + +## 8. 10.M4 — every relative link in the docs resolves + +In `tests/test_docs.py`, replace `test_every_page_index_md_links_to_is_a_page_of_the_layout` with a +parametrised check over `PAGES` plus `README.md` and `CONTRIBUTING.md` (repo root via +`Path(__file__).resolve().parents[1]`; the suite always runs from a checkout, so no skip is needed). + +- Strip fenced blocks (`^(```|~~~).*?^\1`, `re.S|re.M`) and inline code spans before matching, so + code is never read as a link. Link pattern `\]\(([^)\s]+)(?:\s+"[^"]*")?\)`; skip `scheme:` + targets (`https:`, `mailto:`). +- Split `path#fragment`. Empty path means the page itself. Resolve `(page.parent / path).resolve()`; + assert it exists. For a bundled page, also assert the target is inside `DOCS` (the docs ship in + the wheel, so a link out of `docs/` is dead there) and, for `.md`, is in `PAGES`. +- A fragment must be an anchor of the target: the GitHub slug of each heading outside fences + (strip backticks and link markup, lowercase, drop characters that are not word, space or hyphen, + spaces to hyphens, `-1`, `-2` suffixes for duplicates), plus any ``. +- Report every broken link in one assertion message (`page: target (missing|no anchor)`). +- Keep `test_a_link_out_of_the_docs_is_not_read_as_a_page_of_the_layout` against the new pattern + and add a sample-based unit test for the slug (`"SH103 — a wall-clock ..."` -> + `"sh103--a-wall-clock-..."`, a duplicate heading gets `-1`), so a slug bug cannot pass silently. +- Update the module docstring and the comment above `_PAGE_LINK` (it says in-page anchors are not + handled). + +Prototype: 249 relative links (140 in-page anchors, 88 page links, 21 page#anchor) and +15 external across the 15 pages + README + CONTRIBUTING; all resolve today, and no bundled page links +out of `docs/`. + +## Decisions (settled during planning) + +- **11.M1:** the blank-instance builder also gets `synchronous=OFF` (see §1). +- **6.M1:** a non-schema `SeahavenError` raised at import (for example a duplicate tool name) gets + the same file:line lookup, and the fix text `fix the error at `. The traceback hint + stays `python -c 'import '`. +- **7.M2:** no WebSocket `Origin` check. It is not part of the approved fix. +- **3.M1** (clock functions registered deterministic) is won't-fix; it is not in this phase. diff --git a/specs/projects/cr_moderate_fixes/components/phase_2_bulk_gate.md b/specs/projects/cr_moderate_fixes/components/phase_2_bulk_gate.md new file mode 100644 index 0000000..8353bca --- /dev/null +++ b/specs/projects/cr_moderate_fixes/components/phase_2_bulk_gate.md @@ -0,0 +1,179 @@ +# Component: Phase 2, the `bulk()` deadlock (2.M1) + +A per-thread re-entrant gate. Prototyped on a scratch copy of the tree at `38c6f3c`, with scripts +that reproduce each hang below; line numbers are at that commit. + +## 1. Defect, and one more case the fix covers + +Rule broken: a thread that holds an instance lock queues for the gate. There are three confirmed +ways to do this on current `main`, and all three hang for good at `set_concurrency(1)`: + +| Case | Current code | Prototype | +|---|---|---| +| A in `X.bulk()`; B calls `X.call` (gets the slot, then waits for X's lock); A calls `X.call` | hangs | ok | +| The same, but A calls `Y.call` inside `X.bulk()` | hangs | ok | +| **New:** a tool on X calls `Y.call(...)` on an instance it captured. **One thread** holds the slot and X's lock, then queues for a second slot. | hangs | ok | + +(`World.instance` is refused inside a call; calling an existing instance is not.) + +## 2. Change 1: `gate()` becomes re-entrant per thread (instances.py, replaces lines 213-230) + +```py +# Whether this thread is inside a `gate()` block, whether or not that block took +# a slot. Per thread, not a ContextVar: Starlette's `run_in_threadpool` and +# `asyncio.to_thread` copy contextvars into the worker thread, and that worker +# holds none of the locks the flag stands for. +_gated = threading.local() + +@contextmanager +def gate(*, bypass: bool = False) -> Iterator[None]: + """...(existing text)... Re-entrant per thread: a block opened inside another + takes nothing, because by then the thread may hold an instance lock.""" + if getattr(_gated, "active", False): + yield + return + semaphore = None if bypass else _gate # read once, as now + if semaphore is not None: + semaphore.acquire() + _gated.active = True # set only after the acquire returns + try: + yield + finally: + _gated.active = False + if semaphore is not None: + semaphore.release() +``` + +Decisions: +- **A bool, not a depth counter or the semaphore.** Only the outermost block writes it, so it + resets to `False` without a saved value. The acquired semaphore stays in the outer generator's + local, as now. A mid-block `set_concurrency` therefore works: nested calls bypass whichever gate + is in place, and the outer block releases the one it took. +- **Set even when no slot is taken** (`bypass=True`, or `_gate is None`). This covers (a) a + `set_concurrency(1)` inside a block that started with no gate (test 6), and (b) a control tool + (given the `Instance`) calling `live.call(...)` under the lock. Neither changes un-nested + behaviour. +- An exception inside the block reaches the `finally`, and an interrupted `acquire()` leaves the + flag unset. +- **Nothing moves a held slot to another thread.** Every `gate()` is a `with` inside one function + (`_dispatch`, `_bulk`), so it is entered and exited on the same thread. OpenEnv runs + `instance.call` inside one executor job for each session (`openenv/env.py:743`). The HTTP server + runs `dispatch` inside one `run_in_threadpool` job (`http/server.py:182`, `http/runtime.py:279`). + A `bulk()` entered on one thread and exited on another already fails at the RLock release. + +`_dispatch` and `handles.py` do not change (a handle's activation is on the thread that opened it). + +## 3. Change 2: `_bulk` enters the gate before the lock + +```py +with gate(bypass=True), self._held() as frame, self._recording(None), ExitStack() as stack: +``` + +`_refuse_if_formatting()` stays first. In a nested `bulk()`, or `Y.bulk()` inside `X.bulk()`, the +inner `gate()` returns early and `_held()` takes the lock. + +**Decision: `bypass=True` (variant B, "mark but do not take"), not a slot-taking `gate()` +(variant A).** Variant A was the first proposal; B is what the maintainer's requirement of no change +for default callers requires. The code shape is the one decided: the gate before the lock, +a thread-local, and `gate()` bypassing on a marked thread. The difference is one keyword. Both +variants remove all three hangs , and both pass every existing test +on 4 CPUs. Variant A fails the hard requirement "same behaviour when not nested": + +1. **HTTP requests would become gated.** `http/runtime.dispatch` runs every request inside + `instance.bulk()`. `docs/http_apis.md:264` says the concurrency gate does not apply there, and + `:277` says requests to different instances run in parallel. `seahaven http` has no + `--concurrency` flag and never calls `set_concurrency`. Under A, HTTP throughput would be + limited to `min(cpus, 16)` on every `seahaven http` server, and it would also get the gate's + documented starvation defect. +2. **A `bulk()` that stays open holds a slot while it is open.** The repo's own + `tests/test_docs_examples.py::receivers` (session scope) yields from inside `inst.bulk()` on the + main thread. On 1 CPU (`taskset -c 0`, default gate 1), A made `test_http_server.py:: + test_an_id_of_64_characters_is_taken_and_65_is_not` time out after test_docs_examples.py (it + passes alone). B passes the same run (480 passed). 1-CPU CI and eval containers are common. +3. A harness loading data with `bulk()` while other threads call would queue behind them, unfairly. + +What B does not do: it does not count bulk work, or calls made inside `bulk()`, against the bound. +Today the `bulk()` block is already outside the bound. Only the calls nested in it took slots, and +that is the source of the deadlock. The rule "the gate before the lock" does not require `bulk()` to +take a slot. The rule is that a thread holding a lock never queues, and B keeps it. A `destroy` or a +`freeze` still never waits on a queued call, because under B a `bulk()` never queues. + + +## 4. Other paths that hold the instance lock + +| Path | Takes the gate? | Can it reach `gate()` under the lock? | Action | +|---|---|---|---| +| `_dispatch` (call) | yes, first | nested `inst.call` | covered by the flag | +| control tool | `bypass=True`, first | a control fn calling `live.call` | covered (flag set on bypass) | +| `_bulk` | now yes (marks) | `inst.call` inside the block | covered | +| `handle.call` | no | only inside an activation that is already marked, or inside creation | none | +| creation `_held` (startup hooks, `instances.py:1162`) | no | a hook calling some `Y.call` | none: the new instance is not published yet, so no other thread can wait on its lock | +| `state()` formatter | no | a formatter calling **another** instance's `call` (own instance is refused) | none: formatters are documented as read-only (see Decisions) | +| `freeze`, `change_log`, `call_log`, `inspect`, `_control_db`, `destroy` | no | no: no user code runs under the lock | none | + +## 5. Text changes + +- **Module docstring, rule 2**, rewritten: "*The gate before the lock, and never under it.* The + concurrency gate bounds how many tool calls run at once across the process. A call takes it + before the instance lock, so a call queued behind it holds nothing and can never delay a + `destroy` or a `freeze`. The gate is re-entrant per thread: once a thread is inside a call or a + `bulk()` block, nothing more it does takes the gate. That covers a nested call through a handle, + an `inst.call(...)` inside `bulk()`, and a call on another instance from inside either, because + by then the thread may hold an instance lock that a queued caller is waiting for. `bulk()`, + instance creation and the control tool take no slot." +- The `gate()` docstring gets the last sentence from §2. `_bulk` gets a one-line comment: it marks + the thread and takes no slot, because HTTP requests run in it and the gate does not apply to them + (`http_apis.md`). +- `docs/serving_and_openenv.md` §"The concurrency gate", line 492-493: add "`inst.bulk()` bypasses + it too, and so does any call made inside a call or a `bulk()` block on the same thread." + `composition.md:320-321` and `http_apis.md` stay correct under B. + +## 6. Tests (in `tests/test_instances.py`, after `test_the_gate_is_released_after_a_call_that_raised`) + +Helper `TimedGate(instances._Gate)`: `acquire` waits at most `WAIT` and **raises** `AssertionError` +("queued for the gate while holding an instance lock") when the wait runs out. Its `acquire` also +counts `taken` and sets `contender_has_slot` when a thread named `contender` gets a slot. Install it +with `monkeypatch.setattr`. The timeout is required: on current code a deadlocked test blocks the +`with world.instance()` teardown in `destroy()` for ever, and hung pytest in the first prototype. +The autouse `_process_wide_runtime_state` already restores `_gate`. + +**Run the body of every flag-checking test on a fresh `Caller` thread.** The `receivers` fixture +leaves the main thread marked; the first prototype passed alone and failed in the full suite. + +1. `test_a_call_inside_bulk_does_not_queue_behind_a_caller_waiting_for_the_lock[same|other]`: the + author thread enters `x.bulk()`. The `contender` thread calls `x.call` and gets the only slot. + The author then calls `x.call` or `y.call`. Both `finish()`, and both rows are present. +2. `test_a_tool_calling_another_instance_does_not_wait_for_its_own_slot`: at gate 1, a tool on X + calls captured `y.call`. +3. `test_nested_bulk_blocks_take_no_slot`: `x.bulk()` containing `x.bulk(), y.bulk()` with a call + on each. `taken == 0`, and a later call gives `taken == 1`. +4. `test_bulk_that_raised_leaves_the_thread_gated_again`: raise inside `x.bulk()` after a call. The + row is rolled back, `taken` stays 0, and the next call takes a slot (`taken == 1`), so the flag + was cleared. +5. `test_bulk_does_not_wait_for_a_full_gate`: the no-change guard. A slot holder on X at gate 1. + `y.bulk()` on another thread finishes within `WAIT/5`. +6. `test_a_resize_inside_bulk_does_not_make_its_calls_queue`: `set_concurrency(0)`, enter + `x.bulk()`, install `TimedGate(1)`, occupy it from a thread calling Y, then `x.call` returns. +7. Optional: `test_a_control_tool_calling_its_instance_passes_a_full_gate`, which extends the + `peek` control tool in `test_a_control_tool_passes_an_exhausted_gate`. + +Results: the prototype passes all of these. On current code 6 of the 7 fail, each within its +timeout. Test 5 passes on current code on purpose. Mutation checks: +- without the flag check in `gate()`, tests 1, 2, 3, 4 and 6 fail; +- without `gate()` in `_bulk`, tests 1, 3, 4 and 6 fail; +- without the reset in `finally`, tests 3 and 4 fail. + +Full framework suite with the patch: 2393 passed, 1 failed (`test_typed_call::test_ty_resolves...`, +which also fails unpatched in the scratch copy). The reference world and xmlrpc pass on 1 CPU. + +## 7. Decisions and known limits + +- **`state()` does not enter the gate.** A formatter that calls another instance could still queue + while it holds X's lock, but formatters are documented as read-only. Not changed. +- **The `receivers` fixture** in `tests/test_docs_examples.py` keeps a `bulk()` open on the main + thread for the session, as it does today. Under variant B this only means main-thread tests are + not gated; tests that check the flag run their bodies on a fresh thread (§6). +- **Out of scope:** two instances can still deadlock on lock order (A in `X.bulk()` calls Y while B + in `Y.bulk()` calls X). This exists today and does not involve the gate. +- **HTTP** (`src/seahaven/http/`) is unchanged: requests run inside `bulk()`, which marks the thread + and takes no slot, exactly as the gate does not apply to them today. diff --git a/specs/projects/cr_moderate_fixes/components/phase_3_route_middleware.md b/specs/projects/cr_moderate_fixes/components/phase_3_route_middleware.md new file mode 100644 index 0000000..d6d033c --- /dev/null +++ b/specs/projects/cr_moderate_fixes/components/phase_3_route_middleware.md @@ -0,0 +1,200 @@ +# Component: Phase 3, middleware per contributing route (4.M1) + +Prototyped on a scratch copy of the tree at `38c6f3c` (4 files, +63/-58 in `src/`); line numbers +are at that commit. Full framework suite, projecttracker and xmlrpc suites, ruff and ty (serve side) +are green on the prototype. Ten scratch tests: seven fail +on today's code and pass on the prototype. The other three guard behaviour that must not change, +and pass on both. + +## 1. Today + +- `_walk` records `parents[child]` (the first edge that reaches it). `_nodes` builds, per node, + `agent_chain = build_route_chain(_route_layers(key, parents), key)` (the canonical route) and + `internal_chain = build_route_chain(_own_layers(key), key)`. +- `_contribute` folds `_Candidate(entry=Contributed(name, tool, node), route, prefixes, shadowed)` + bottom-up. `_Candidate.route` is the `/`-joined edge *names* (diagnostics only). It holds no node + keys, so the chain cannot be read from it. +- Consumers of the chains (all of them): + - `instances.Instance._dispatch` (agent calls, by name and by function) runs + `target.node.agent_chain`. + - `world.World.chain` runs `root.agent_chain`. It is used by `tests/test_world.py` 390/734/743. + - `handles.WorldHandle.call` (host code, `ctx.worlds.x.call`) runs `owner.internal_chain`. + - Control tools go to `control.dispatch` and never touch a chain. Startup hooks run no + middleware. openenv, mcp and lint read `composition.tools` for names, tools and nodes only. + No chain is read there. + +## 2. Change + +### Data model (`src/seahaven/composition.py`) + +- `Contributed` gains `chain: Handler`. It is what an agent's call to *this entry* descends: every + middleware of every node on the route that contributed it, outermost first, then `invoke` on + `node`. The docstring says the chain belongs to the entry because one node reached by two routes + contributes an entry through each. +- `Node.agent_chain` is **removed**. Keeping a canonical-route chain invites the same bug back. + `Node.internal_chain` is **renamed `own_chain`** (decided): this node's middlewares, then `invoke` on this + node. It serves two callers: a handle call, and an agent call to one of the root's own tools + (for the root, the canonical route is the root alone)." It stops `World.chain` reading + `root.internal_chain`. `Node` is not exported from `seahaven`. +- `_Candidate.entry` is replaced by `name: str, tool: Tool, node: Node, hosts: tuple[NodeKey, ...]`. + `hosts` holds the node keys on the contributing route, from the node whose list this is down to + the owner (`hosts[-1] == node.key`). A `Contributed` is built only at the end, because it now + needs a chain, and building a chain for every candidate at every level would be wasted work. +- `_walk` drops `parents` (it now has no reader). It returns four values instead of five. + +### Where it is computed (the seal) + +- `_contribute`: a node's own tool starts with `hosts=(key,)`. `_through(host, added, inner)` takes + the host key from `contribution(key)` and sets `hosts=(host, *inner.hosts)`. `_fold` keys on + `candidate.name`. The standing candidate keeps its own `hosts`, and a repeat that survives to the + root is a collision anyway (`_raise_clashes`), so every served entry has exactly one route. +- `resolve`: `tools = _entries(root_node, surface.candidates)` in place of + `{name: c.entry ...}`. Chains are built only for entries that reach the root, one per distinct + route: + +```py +def _route_layers(route: Sequence[NodeKey]) -> list[tuple[NodeKey, Middleware]]: + return [(key, middleware) for key in route for middleware in key[0].middlewares] + + +def _entries(root: Node, candidates: Mapping[str, _Candidate]) -> dict[str, Contributed]: + chains: dict[tuple[NodeKey, ...], Handler] = {(root.key,): root.own_chain} + tools: dict[str, Contributed] = {} + for name, candidate in candidates.items(): + route = candidate.hosts + if route not in chains: + chains[route] = build_route_chain(_route_layers(route), route[-1]) + tools[name] = Contributed(name, candidate.tool, candidate.node, chains[route]) + return tools +``` + +- `_nodes`: `own_chain=build_route_chain(_route_layers((key,)), key)`. `_own_layers` goes away. +- The cache is keyed by node keys, not edge names. Two edges from one host to one node + (`name="a"` and `name="b"`, same store) are the same nodes, so they give the same chain. That is + correct: middleware belongs to worlds, not to edge names. Seeding `(root.key,)` makes every root + tool's `chain is world.chain`, so a leaf world still has exactly one chain object. +- Cost: `hosts` prepends O(depth) per `_through`. The seal stays O(nodes x children x tools x + depth), with depth at most the attach bound. The diamond tests take 0.02s both before and after. + +### Dispatch (`src/seahaven/instances.py`) + +- `_Target` gains `chain: Handler`. `_target` fills it from `entry.chain` on both the by-name and + the by-function branch. `entry_for` returns the same `Contributed`, so a typed call runs the + same chain as the name. The control-tool branch passes `composition.root.own_chain` (unused, + because `control.dispatch` returns first). +- `_dispatch` runs `target.chain(ctx, call)`. Reword the comment: the chain comes from this + call's resolution, which reseals after a registration, so a middleware registered after the + instance was made still applies. +- Add `Handler` to the `seahaven.call` import. +- `world.py`: `World.chain` returns `self.composition().root.own_chain`. The docstring is still + true. `handles.py`: `owner.own_chain(...)`. + +### Host code through handles: unchanged, deliberately + +`ctx.worlds.x.call` runs the owning node's own chain today and after the change. A handle names a +store, not a route, and the host's chain (and the chain of every world above it on *its* route) is +already wrapped around the host tool making the call. So a nested call runs exactly the layers of +the route by which the agent reached the host, plus the owner's own layers. Using the handle's +route would run the host's layers twice. + +### Behaviour is identical when no node is shared + +For an entry, the contributing route is a path of edges from the root to the owner, and the +canonical route is the `parents` path. If every node on the contributing route (the root +excepted) is reached by exactly one edge, then that node's only incoming edge is its `parents` +edge. So the two paths are equal, `_route_layers` returns the same list and `build_route_chain` +builds the same chain. The root's own tools have route `(root,)`, which is the old +`root.agent_chain`. Scoped second accounts are separate nodes, so the graph is still a tree. +Behaviour changes **only** for an entry whose contributing route crosses an alias edge. Among +the committed worlds, only emporium's `shop/payments` crosses one, and it contributes nothing +(`tool_allow_list=[]`). No world there has middleware, and the suites pass unchanged. + +### Cases + +- **Two names, two routes** (`m_leaf_write` through `middle`, `l_leaf_write` direct, one store): + host+middle+leaf versus host+leaf. `call.node` is the canonical path (`leaf`) for both, and + `ctx` for each layer is its own node's. +- **Diamond** (host→left→leaf and host→right→leaf, prefixes `l_`/`r_`): each side runs its own + middle. Today `r_` runs `left`'s. +- **Allow/block lists**: a route filtered out never reaches the root, so it cannot choose the + chain. With `host.add_world(leaf, tool_allow_list=[])` first (canonical) and `middle` adding + `leaf` second, `leaf_write` runs host+middle+leaf. Today it runs host+leaf. +- **Three deep** (host→a→b→leaf, plus host→leaf `x_`): `leaf_write` runs host, a, b, leaf. +- **Intermediate error handler**: `middle`'s handler maps the leaf's error on `m_refuse`, and + `l_refuse` raises the leaf's own error. +- **Composite fixtures**: chains are not frozen. Sidecar, paths, file and schema names, change-log + `world`, `NodeReport` and edges are unchanged, because paths still come from `_walk`. +- **SH207** (one tool under two names): still a warning. The two names can now also differ in + middleware; `reference/lints.md` says so (§3). + +## 3. Docs + +`src/seahaven/docs/composition.md`, the first sentence of `## Middleware` (line 378). Replace +"along the canonical route from the root to the owning node" so it reads: + +> An agent call to a contributed tool descends the middleware of every world on the route that +> contributed that tool, from the root to the owning world, outermost first: the host's, then any +> world in between, then the owning world's, and then the tool. When a shared node's tool reaches +> the surface under two names by two routes, each name runs its own route's middleware. + +Nothing else on the page names the canonical route for middleware. + +`src/seahaven/docs/reference/lints.md`, SH207 **Why**: add one sentence, "Each name also runs the +middleware of its own route." + "Paths and aliases" (paths, +files, ids, change log) stays correct. The `authoring.md:509` note stays correct. + +Code comments: update the module docstring line 7-8 ("the chains their calls descend" becomes +"the chain each tool on the surface descends") and the `_route_layers` docstring (a route, not +the canonical route). Update the `Node` field comment and the `_Candidate` docstring to describe +`hosts`. + +## 4. Tests (all through `world.instance(...)` unless marked seal) + +`tests/test_composite_dispatch.py`, in the "the chain" section. They reuse `rooted`, `traced` and +`composable_world`. Add a helper `shared_under_a_middle(tmp_path, trace)`: host adds traced +`middle` (which adds traced `leaf`) with `m_`, then adds `leaf` with `l_`. + +1. Rename `test_the_agent_chain_is_the_whole_canonical_route` to + `test_the_agent_chain_is_the_whole_contributing_route`. The body is unchanged. This is the only + existing test that changes. +2. `test_sharing_a_node_keeps_the_middle_worlds_middleware_on_what_it_contributes`: the reviewer's + reproduction. `company` adds `shop` (`shop_`, with a recording middleware, adds `payments`). + Call `shop_payments_write`. Then `company.add_world(payments, tool_prefix="pay_")`, make a new + instance and call both names. Shop's middleware ran for `shop_payments_write` both times and + never for `pay_payments_write`. +3. `test_each_name_of_a_shared_tool_runs_its_own_routes_middleware`: in `shared_under_a_middle`, + the trace for `m_leaf_write` is host/middle/leaf and for `l_leaf_write` is host/leaf. Both reads + return both rows, which proves it is one store. +4. `test_both_sides_of_a_diamond_run_their_own_middle`: `r_leaf_write` gives + `[host, right, leaf]` and `l_leaf_write` gives `[host, left, leaf]`. +5. `test_a_route_the_lists_filter_out_does_not_choose_the_chain`: the canonical edge has + `tool_allow_list=[]` and `middle`'s route contributes, so `leaf_write` runs host/middle/leaf. + The same holds by function reference: `live.call(leaf.tools["leaf_write"].fn, ...)`. +6. `test_a_shared_leaf_three_deep_keeps_every_world_between`: host→a→b→leaf plus host→leaf `x_`. + `leaf_write` gives `[host, a, b, leaf]` and `x_leaf_write` gives `[host, leaf]`. +7. `test_a_middle_worlds_error_handler_shapes_the_errors_it_contributes`: `leaf.refuse` raises + `Boom`, and `middle`'s middleware re-raises it as its own `ToolError` subclass. `m_refuse` + raises middle's error and `l_refuse` raises `Boom`. +8. `test_a_middleware_registered_later_reaches_a_route_through_a_shared_node`: on a live instance, + register a middleware on `middle`. It runs on `m_leaf_write` and not on `l_leaf_write`. +9. Guard: `test_a_nested_call_into_a_shared_node_runs_only_the_owners_layer`. A host tool calls + `ctx.worlds.middle.worlds.leaf.call("leaf_write")`, and the trace is `[host, leaf]`. It passes + today too, and pins the rule that a handle call does not take a route. + +`tests/test_composition.py`, in the "contribution" section (seal, no instance): + +10. `test_each_entry_carries_the_chain_of_the_route_that_contributed_it`: `m_leaf_write` and + `l_leaf_write` have the same `.node` but different `.chain`. +11. `test_entries_of_one_route_share_one_chain`: `m_leaf_write.chain is m_leaf_read.chain`, and a + root tool's `.chain is host.chain`. + +Each of tests 2-8 and 10-11 fails on today's code (confirmed on the prototype's scratch copies). + +## 5. Decisions + +- `internal_chain` is renamed `own_chain`. +- SH207's **Why** in `reference/lints.md` gains the middleware sentence (§3). +- This design removes `parents` from `_walk`. Phase 1's hook order (4.M2, `hosts_first`) walks + `Node.added` and does not read `parents`, so the two changes do not conflict; phase 1 lands + first. diff --git a/specs/projects/cr_moderate_fixes/components/phase_4_docs.md b/specs/projects/cr_moderate_fixes/components/phase_4_docs.md new file mode 100644 index 0000000..f7d83f6 --- /dev/null +++ b/specs/projects/cr_moderate_fixes/components/phase_4_docs.md @@ -0,0 +1,214 @@ +# Component: Phase 4, docs (9.M1, 9.M7, 9.M8, 8.M4, 2.M2/4.M3, 9.M6) + +Line numbers are at `38c6f3c`; phases 1-3 move some of them, so find the quoted text. Docs paths are under `src/seahaven/docs/`. Wrap all prose at 100 +columns when applying. Nothing below renames a heading, so no anchor or `tests/test_docs.py` change. + +## 1. 9.M1: SH103's stated reason + +**Verified** on a fixed clock at `2026-06-01T09:00:00.000Z`, with the clock functions registered +as they are (`SQLITE_DETERMINISTIC | SQLITE_INNOCUOUS`; 3.M1, removing the flag, is won't-fix): +- `DEFAULT CURRENT_TIMESTAMP`, `DEFAULT (datetime('now'))`, a trigger's `datetime('now')` and a + view's `datetime('now')` all give `2026-06-01T09:00:00.000Z`: canonical text. +- `DEFAULT CURRENT_DATE` gives `2026-06-01`; `DEFAULT (julianday('now'))` gives `2461192.875`. + Not a timestamp in the canonical format. +- `'now'` in a generated column, an index expression or a partial-index `WHERE` is accepted, + because the overrides are registered deterministic. The stored or indexed value is computed once + and goes stale as the clock moves. +- A plain SQLite connection (Python `sqlite3`) on the same schema writes host wall-clock time in + SQLite's format (`2026-09-29 19:44:50`). The overrides exist only on connections Seahaven opens + (`db.open_instance`, `open_inspection`, `_build`). + +**What SH103 is for:** +(a) `CURRENT_DATE`, `CURRENT_TIME`, `date/time/julianday/unixepoch('now')` write a date, a time or a +number. (b) A trigger that stamps a time overwrites the timestamps a fixture generator writes in +`bulk()` with the instance's clock, so loaded history is lost. (c) `'now'` in a generated column, +an index or a `CHECK` is evaluated once and not again as the clock moves. (d) A default, trigger or +view is part of the file, and any connection Seahaven did not open (a grader using `sqlite3`, the +`sqlite3` shell) evaluates it with the host's wall clock in SQLite's format. (e) One rule: world +code owns every timestamp. + +**Edit 1a. `db_schema_and_fixtures.md:79-82`.** Current: "one built from `CURRENT_TIMESTAMP` reads +the instance's clock. The wall-clock rule below is unchanged and still refuses `CURRENT_TIMESTAMP` +inside a `CREATE` — a column default, a trigger body — so that one timestamp format is written +everywhere; what is sanctioned here is the `INSERT` the file runs itself." Replace from "The +wall-clock rule" with (drops "unchanged and still", a history phrase, and the false reason): +> The wall-clock rule below covers the objects a schema file creates, such as a column default or a +> trigger body. It does not cover the `INSERT` statements the file runs. + +(True: SH103 reads `sqlite_master` only, `lint/ddl.py:166-198`.) + +**Edit 1b. `db_schema_and_fixtures.md:98-103`.** Current: "…The problem is the text as much as the +instant: SQLite writes `2026-06-01 09:00:00`, and the rest of the world writes +`2026-06-01T09:00:00.000Z`. Two formats in one column make a comparison fail for a reason nobody +finds quickly. Declare the column…". Replace the three sentences from "The problem is" with: +> The tool that makes a row decides its time. A trigger that stamps `updated_at` overwrites the +> history a fixture generator writes, and `CURRENT_DATE` or `julianday('now')` writes a date or a +> number where every other timestamp is `2026-06-01T09:00:00.000Z`. + +**Edit 1c. `reference/lints.md:94-98` (the SH103 "Why").** Current: "The problem is the text rather +than the instant. An instance's clock overrides make a default in the schema read the instance's +clock anyway, but what SQLite *writes* is `2026-06-01 09:00:00`, and every other door of a world +writes `2026-06-01T09:00:00.000Z`. One format across every door is the rule, and two formats in one +column is a comparison that fails for a reason nobody will find quickly." Replace with: +> **Why.** On Seahaven's own connections, `CURRENT_TIMESTAMP` and `datetime('now')` read the +> instance's clock and write canonical text. The rest of the family does not: `CURRENT_DATE`, +> `CURRENT_TIME`, `date('now')`, `julianday('now')` and `unixepoch('now')` write a date, a time or a +> number. A trigger that stamps a time also overwrites the timestamps a fixture generator writes in +> `inst.bulk()`. And a schema object is part of the database file: a connection Seahaven did not +> open, such as a grader reading `state.sqlite` with Python's `sqlite3`, evaluates it with SQLite's +> own functions, which read the host's wall clock and write `2026-06-01 09:00:00`. In a generated +> column, an index or a `CHECK`, the time is read once and the stored value does not move with the +> clock. A timestamp that world code writes from `ctx.clock.iso()` has none of these problems. + +Keep the "Rule", "Fix" and false-positive paragraphs (`:90-92`, `:100-106`) unchanged. The Fix +text is the lint's own message (`lint/ddl.py:193-196`), so do not reword it here. + +**Edit 1d. `reference/lints.md:90-92` (the SH103 "Rule").** The regex already searches the whole +schema; name what it covers: "…in a column default, a trigger body, a view, a generated column, a +`CHECK` constraint, an index expression or a partial index." + +## 2. 9.M7: `composition.md:717-721`, the benchmark sentence + +Current (`:716-721`): "**An idle composite instance is one file and one connection per node.** A +session is opened per node per call and closed with the call, so an idle instance holds none. Cost +scales with the tree, and the design target — hundreds of concurrent instances with minute-long +lifetimes — holds for a small number of nodes. The benchmark measures one node per instance and +reads as a per-node floor." + +What `bench/` measures now: `uv run python -m bench composite` (`bench/composite.py`) stands up the +1-, 2- and 4-node trees of `tests/worlds/` and reports open/destroy time, disk per node, and +one-row read and write per call at 1 and 4 nodes (`bench/results/latest.md` §7: about 3 ms and +44 KiB per added node, call overhead 1-5% for 3 added nodes). Bench only runs over the repo's own +worlds, so it is no help to an author, and figures are out under 9.M6. Replace the last two +sentences with: +> Cost scales with the number of nodes: each node adds a file and a connection when an instance +> is made, and a session on every call. + +(Verified: `NodeRuntime` per node, `state.md:563` says a session per node per call.) + +## 3. 9.M8: `projecttracker.md:208-211` + +Current fence: `uv run pytest worlds/projecttracker` / `uv run seahaven check`. From the repo root +`uv run seahaven check` exits 1 with `SH501 ... seahaven has a module and not a World for its +'world'` (reproduced). Replacement, both run from the repo root and verified clean (pytest exit 0, +check exit 0; `cd worlds/projecttracker && uv run seahaven check` is also clean): +```sh +uv run pytest worlds/projecttracker +uv run seahaven check --world projecttracker:world +``` +Optionally add above the fence: "Run both from the root of the Seahaven repository." + +## 4. 8.M4: audit trail and `actor_id` claims (ProjectTracker) + +**Facts (`worlds/projecttracker/src/projecttracker/tools/`).** 13 write tools. Five take `actor_id` +and resolve an actor (`_rows.resolve_actor`): `create_issue`, `update_issue`, `assign_issue`, +`transition_issue`, `add_comment`. Eight take none: `create_user`, `create_team`, +`add_team_member`, `create_project`, `update_project`, `create_label`, `set_issue_labels`, +`archive_issue`. Event kinds are `created`, `status`, `assignee`, `comment` (`_types.py:57`). +Rows are written by `create_issue` (`created`), `add_comment` (`comment`), and `_apply` for +`update_issue`/`assign_issue`/`transition_issue` (`status` and `assignee` changes only). No event +for: `title`, `description`, `priority`, `due_at` changes, `set_issue_labels`, `archive_issue`. + +| Where | Current | Replacement | +|---|---|---| +| `worlds/projecttracker/README.md:6-7` | "issues carry labels and comments, and every write to an issue appends to an audit trail." | "issues carry labels and comments, and an audit trail records who created each issue, changed its status or assignee, or commented on it." | +| `src/seahaven/docs/projecttracker.md:63-64` | same sentence as the README | same replacement | +| `worlds/projecttracker/AGENTS.md:37-39` | "Every write takes an optional `actor_id`, which wins, and a write with neither is `InvalidInput("actor_id", "no actor")` — which is…" | "The five issue writes the trail records (`create_issue`, `update_issue`, `assign_issue`, `transition_issue` and `add_comment`) take an optional `actor_id`, which wins, and one of them with neither is `InvalidInput("actor_id", "no actor")` — which is…" (keep the rest) | +| `src/seahaven/docs/projecttracker.md:173-175` | "Every write takes an optional `actor_id` which wins, so one run can have two people writing without two instances. A write with neither, …, is `INVALID_INPUT`." | "The five issue writes the audit trail records take an optional `actor_id`, which wins, so one run can have two people writing without two instances. One of them with neither, …, is `INVALID_INPUT`." | +| `worlds/projecttracker/src/projecttracker/tools/_events.py:3-4` | "Every write to an issue leaves a row here -- created, status changed, assignee changed, comment added -- and every one of them goes through `record_event`." | "Creating an issue, changing its status or its assignee, and adding a comment each leave a row here, and every one of them goes through `record_event`. Other field changes, labels and archiving leave none." | + +Add to AGENTS.md (the world's product-rules list, next to the viewer bullet) one line an eval +author needs: "The trail does not record `title`, `description`, `priority` or `due_at` changes, +labels, or archiving; grade those on the row." Do not touch the closed-issue wording (`dc29e4b`). + +**Leave `schema/001_core.sql:105`** ("The audit trail every write to an issue appends to") as it is: +`world._schema_hash` collapses whitespace only, so a comment edit changes `schema_hash` and every +fixture then fails SH403 (decided; no fixture regeneration in this project). + +## 5. 2.M2 / 4.M3: `bulk()` is not atomic across nodes when a commit fails (accepted) + +**Real behaviour (reproduced: root 0 rows, child row kept and in the change log, `DbError` +raised).** `_bulk` enters one transaction per node +in `_runtime` order (root first) on an `ExitStack`, so they exit in reverse: added nodes commit +first, the root last. If the block raises, every node rolls back. If a commit fails (a +`DEFERRABLE INITIALLY DEFERRED` foreign key, a full disk), `bulk()` raises; that node and the nodes +not yet committed roll back; nodes already committed keep their rows. + +Every place that makes or implies the claim: +- **`src/seahaven/instances.py:598-600`** (`bulk()` docstring): "Every node's transaction is + committed on the way out, and all of them are rolled back together if the block raised." This + sentence is true as written (raising rolls back all), but reads as all-or-nothing. Replace with: + "If the block raises, every node's transaction is rolled back. Otherwise they commit one at a + time, the root last, and a commit that fails leaves the nodes committed before it committed." +- **`src/seahaven/instances.py:921-923`** (comment in `_bulk`): "Every node's transaction open + before the block runs and committed in sequence on the way out, so a bulk write that reaches two + stores through `ctx.worlds` either lands in both or in neither." False. Replace with a constraint + comment: "One transaction per node, because each node is its own file and SQLite cannot commit + two files atomically. The stack commits them in reverse, the root last: a block that raises rolls + every node back, but a commit that fails leaves the nodes before it committed." +- **`composition.md:489-490`**: "`inst.bulk()` yields the root's context with a live `ctx.worlds`, + and opens one transaction per node and commits them in sequence, so a fixture generator fills + every store in one block." Keep, then add: + > If the block raises, every node rolls back. If a node's commit fails, for example on a deferred + > foreign key, `bulk()` raises and the nodes that committed before it keep their rows. Do not + > freeze an instance after `bulk()` raised; build it again. +- **`reference/api.md:198`**: "a context manager yielding the instance's own `Ctx`, in one + transaction, for loading rows fast". Wrong for a composite. Replace: "a context manager yielding + the root node's `Ctx`, with one transaction per node, for loading rows fast. See + [composition.md](../composition.md#one-instance-many-stores) for a composite". +- `db_schema_and_fixtures.md:204-205` ("in one transaction per node") and `composition.md:323-326` + (per-call "no cross-world atomicity") are accurate; no change. + +## 6. 9.M6: history, bench figures, version stories, spec citations + +Scope: every `*.md` under `src/seahaven/docs/` except `http_apis.md`. Grep terms from the brief plus +`§`, `as before`, `always`, `carried`, `meanwhile`, `design target`, `rc2`, `sign-off`. Hits for +"was"/"were" and `version` in examples were read and are ordinary present-tense uses; not listed. + +| # | file:line | Quote | Action | +|---|---|---|---| +| H1 | state.md:563-566 | "The Seahaven repository carries a probe that measures what that costs, under `bench/`. Run the probe with `uv run python -m bench recording ...`" | Delete the bench sentences; keep the first sentence ("Recording is not free … when the call commits.") | +| H2 | state.md:568-569 | "The figures below are approximate. … Read the figures as the size of the cost, not as the cost." | Delete | +| H3 | state.md:571-577 | "costs about 47% more than the single long-lived session per node that Seahaven kept before this release … used to do once an episode … between about 5% and 18%" | Delete. Keep one present fact: "A read-only call records nothing, so it costs less than a write." | +| H4 | state.md:579-582 | "The four-node `emporium` world … 45% to 67% … does not grow in proportion to the node count" | Delete (node cost is covered by §2 above) | +| H5 | serving_and_openenv.md:91-95 | "If the command fails on a release candidate of CPython 3.14 … 3.14.0rc2 … pydantic … `beartype` … 3.14.0 final" | Delete; `:44-45` already says "a final CPython 3.14 or newer, not a release candidate" | +| H6 | serving_and_openenv.md:502-507 | "In the framework's own benchmark, with five threads … 12,874 at a gate of 1, and 5,351 and 4,011 at 2 and 4 … a server with 500 sessions and a gate of 16 is the ordinary case" | Rewrite as fact: "Under load, one session can be served once while another is served thousands of times. Every gate size that binds behaves this way." | +| H7 | serving_and_openenv.md:511-512 | "The fix is a gate that hands slots out in arrival order, not a different number." | Delete (roadmap) | +| H8 | serving_and_openenv.md:514-520 | "What to do meanwhile … `--concurrency 0` was the one setting measured … The measurements … are in `bench/results/latest.md` … no number from them should be quoted" | Rewrite: "If a saturated workload cares about its slowest session, run with `--concurrency 0`. Without the gate every session gets an even share, and median and 95th-percentile latency go up." (`--concurrency` at `cli/serve.py:40`) | +| H9 | serving_and_openenv.md:547-549 | "Seahaven carried a client-side close handshake and an ASGI middleware to get that under openenv 0.4.2, and carries neither now, because openenv 0.5 logs no error for a normal close." | Delete. `:7` already pins OpenEnv v0.5.x | +| H10 | serving_and_openenv.md:594-595 | "Seahaven's own reference world is not published anywhere. That step is gated on a maintainer's sign-off and has not happened." | Delete (maintainer note; AGENTS.md already holds the rule) | +| H11 | composition.md:91-92 | "gains a node dimension it did not have before." | Rewrite: "gains a node dimension." | +| H12 | composition.md:470 | "The root's file is `state.sqlite`, as it has always been, and" | Delete ", as it has always been" | +| H13 | composition.md:482-483 | "The root's stream is the instance seed untouched, which is why a world that adds nothing mints exactly what it always did." | Delete ", which is why … always did"; keep "The root's stream is the instance seed untouched." | +| H14 | composition.md:495 | "It carries the version-1 fields describing the root exactly as before, plus a `nodes` list" | Rewrite: "It carries the version-1 fields, which describe the root, and a `nodes` list" | +| H15 | composition.md:521-523 | "so its fixture directory is byte for byte what it was before composition existed, and every fixture already on disk keeps loading." | Rewrite: "A world that adds nothing writes `format_version: 1` with no `nodes` key, and Seahaven reads both versions." (`fixtures.py:153` `Literal[1, 2]`) | +| H16 | composition.md:589 | "is the read-only connection it always was, with every added node attached" | Rewrite: "is the eval's read-only connection, with every added node attached" | +| H17 | composition.md:592 | "Writes, `ATTACH` and `DETACH` are denied on it, as they always were." | Delete ", as they always were" | +| H18 | composition.md:718-721 | "the design target — hundreds of concurrent instances … The benchmark measures one node per instance" | See §2 | +| H19 | reference/api.md:29-44 | "Ten names … `components/world_and_dispatch.md` §1 … `architecture.md` §1 … declares them public on its own authority … That goes further than the specification" | Rewrite, no specs: "Some names on this page are not exported from `seahaven/__init__.py`. They are public all the same: `seahaven.world.Handler`, `Middleware` and `StartupHook`; `seahaven.fixtures.load`, `load_all`, `verify` and `freeze`; and the concurrency gate, `seahaven.instances.default_concurrency`, `concurrency` and `set_concurrency`." Drop the count "Ten": 9.Mild notes `NodeReport`, `Exec` and the `seahaven.openenv` names are also on the page and not root exports, so say "for example" or list them too | +| H20 | reference/api.md:318-319 | "Two rare cases are exceptions: see the [clock modes risk report](…/specs/projects/clock_modes/risk_report.md)." | Rewrite with the cases (risk report R4, R6; strict xfails `tests/test_clock.py:642,654`): "Two rare cases take another statement's reading: a statement whose text starts with a `-- ` comment, and a statement whose rows are read while another statement runs on the same connection. Neither happens under `fixed`, and under `tick` only on `inst.inspect()` and the control tool." | +| H21 | db_schema_and_fixtures.md:190-191 | "A fixture frozen under an id this rule now refuses still appears in `world.fixtures()` but no longer opens." | Rewrite present tense: "A fixture directory whose id breaks this rule appears in `world.fixtures()` and does not open." | +| H22 | db_schema_and_fixtures.md:326-327 | "Nothing migrates a fixture that has already shipped. Version 1 of Seahaven has no migration path at all, which is why the generator is committed." | Rewrite: "Nothing migrates a fixture, which is why the generator is committed." | +| H23 | db_schema_and_fixtures.md:80-81 | "The wall-clock rule below is unchanged and still refuses" | See §1a | +| H24 | projecttracker.md:198 | "`agency` is also the fixture the framework's benchmark runs against." | Delete (repo-internal; nothing for an author) | + +Kept on purpose (current fact, not history): the PyPI placeholder warnings (`authoring.md:52`, +`serving_and_openenv.md:47-51`, `reference/cli.md:77-81`), "does not build a working image today" +(`serving_and_openenv.md:573-577`), OpenEnv "(v0.5.x)" and "needs openenv 0.5 or newer" +(`serving_and_openenv.md:7`, `:468`), "`seahaven_version": "0.0.1"` (`state.md:47`, an example), +"none is planned" (`state.md:547`), "Deliberately absent, and not planned" (`composition.md:730`). + +**AGENTS.md banned words.** A grep over the same pages for "load bearing"/"load-bearing", "seam(s)", +"the whole point", "surface" as a verb (surfaces/surfaced/surfacing/to surface) and "lives in" finds +**no hits**. "surface" appears only as a noun (`composition.md:124`, `reference/api.md:138`). Near +misses in the same register, for the writer's judgement: `composition.md:90` "That example is the +whole feature." (aphoristic opener; delete), `serving_and_openenv.md:599-600` "which is the whole +integration" (rewrite: "Point Kiln at a served world and it drives the sessions."), +`extensions.md:126` "whole contract", `projecttracker.md:58` "That is the whole reason". + +## Decisions + +- 3.M1 is won't-fix, so SH103's rationale is written for the clock functions as they are (§1), and + the rule list names every place the regex already covers (Edit 1d). +- `schema/001_core.sql` is not edited (§4). +- H19: the concurrency gate stays on the public API page; only the spec citations go. diff --git a/specs/projects/cr_moderate_fixes/functional_spec.md b/specs/projects/cr_moderate_fixes/functional_spec.md new file mode 100644 index 0000000..c7cd7f0 --- /dev/null +++ b/specs/projects/cr_moderate_fixes/functional_spec.md @@ -0,0 +1,117 @@ +--- +status: complete +--- + +# Functional Spec: CR Moderate Fixes + +This project fixes the 17 moderate findings that [triage.md](triage.md) marks "Fix". Each +section below states the defect, the behaviour after the fix, and what an author or operator sees. +IDs are the review's `phase.M#` IDs, as used in the triage. + +Guiding rules for every item: + +- **No ergonomic change for default callers** unless this spec names it. Public signatures stay + the same. +- **The real entry point is tested.** Every behaviour change has a test through + `world.instance(...)` (or the server/CLI a caller uses), as well as any unit test (AGENTS.md). +- **Bundled docs stay current.** Any docs text a change makes false is fixed in the same phase; + the docs phase then does one sweep over everything. + +## 1. Engine and serving fixes + +### 11.M1: faster instance creation +Creating an instance (a fork from a fixture, or a blank instance) no longer fsyncs when it +builds its database file or switches it to WAL. Instance databases are throwaway, so they give up +durability against an OS crash or power loss. Nothing else changes: fixture files written by +`freeze` keep their current durability. Expected effect: fork time roughly halves on `agency`. + +### 7.M2: `seahaven serve` listens on localhost by default +`seahaven serve` and `seahaven.openenv.serve(...)` bind `127.0.0.1` by default, not `0.0.0.0`. +An operator who wants the server reachable from other hosts passes `--host 0.0.0.0`. The Docker +image a hub scaffold builds keeps working: its Dockerfile runs uvicorn directly with +`--host 0.0.0.0`, and a test pins that. +The CLI help and the docs state the new default. + +### 5.M3: `SeahavenClient.step(ListToolsAction())` works +The typed client already accepts a `ListToolsAction`. Today the reply fails to parse and raises a +pydantic `ValidationError` after the server ran the step. After the fix, the client returns a +`StepResult` whose observation is a `ListToolsObservation` carrying the tools. + +### 6.M1: import errors in world code are reported where they happen +When importing a world's package raises (a `SyntaxError`, a `NameError`, an `ImportError` from +the world's own code, or a Seahaven error such as a duplicate tool name), `seahaven check` (rule +SH501) and the pytest plugin report the exception type, its message, and the file and line in the +world's code where it was raised, with the fix "fix the error at ". They do not tell +the author to "export world = ...": that fix text is shown only when the package imports cleanly +and has no world. A world module that does not exist at all gets a fix that names `--world`. + +### 4.M2: startup hooks run hosts first, even when a node is shared +Instance startup hooks run in an order where every world runs before every world it adds. When +nothing is shared, the order is identical to today's (root first, then each added world in +`add_world` order, depth first). When a node is added by two hosts, it runs after both. A host's +hook that seeds a shared node therefore works the same composed as alone. Example: +`company` adds `payments` and then `shop`, and `shop` adds `payments`; the order becomes +`company`, `shop`, `payments` (today: `company`, `payments`, `shop`). + +## 2. The `bulk()` deadlock (2.M1) + +Today, an `inst.call(...)` made inside `with inst.bulk():` waits for a slot of the process-wide +concurrency gate while its thread holds the instance lock. Another thread can hold the last slot +while it waits for that lock, and both threads then wait for ever. With a gate of 1 (the default +on a one-CPU host), two threads are enough. + +After the fix: + +- The gate is re-entrant per thread. Once a thread is inside a call or a `bulk()` block, nothing + more it does waits at the gate: a call inside `bulk()` (on the same instance or another), a + nested call, and a tool that calls another instance all run without queueing. +- `bulk()` marks the thread before it takes the instance lock, and takes no gate slot. The gate + does not apply to `bulk()` today, and it still does not. +- Nothing changes for a caller that does not nest: same signatures, same results, same limits. + The HTTP server, whose requests run inside `bulk()`, stays ungated. +- This also fixes a third hang: with a gate of 1, a tool that calls another instance waited for a + second slot while it held the only one. + +## 3. Middleware on a shared node (4.M1) + +Today, the middleware that runs on an agent-facing tool is chosen from the node's canonical route. +When a node becomes reachable by a second route, a tool contributed through the first route can +silently lose the middleware of the worlds on that route. + +After the fix, the middleware that runs on an agent-facing tool is the middleware of the worlds on +the route through which that tool was contributed. When a tool reaches the surface under two +names through two routes, each name runs its own route's middleware. When nothing is shared, +behaviour is unchanged. Host code that calls through a handle (`ctx.worlds.x.call`) is also +unchanged: it runs only the owning world's own middleware, because the host's layers already wrap +the host tool making the call. `composition.md` states the rule, and SH207's entry in +`reference/lints.md` notes that two names of one tool can run different middleware. + +## 4. Docs + +The docs phase fixes every docs-only item and does one sweep: + +- **9.M1:** restate what lint SH103 is for. The current reason is false: the SQL clock already + writes canonical time text. The rule text names every place the lint already searches. +- **9.M7:** correct `composition.md`'s statement of what the benchmark measures. +- **9.M8:** correct the `projecttracker.md` block that runs `uv run seahaven check` from where it + fails. +- **8.M4:** correct projecttracker's README, AGENTS.md and `_events.py` claims about which writes + are recorded in the audit trail and which tools take `actor_id`. +- **2.M2 / 4.M3:** composite `bulk()` is not all-or-nothing when a later node's commit fails. + This behaviour is accepted. Correct the code comments and docstrings that claim otherwise, and + document the real behaviour in `composition.md`. +- **9.M6:** remove history, benchmark figures, version stories and spec citations from the bundled + docs, per the AGENTS.md docs style. + +## 5. Tests-only items + +- **10.M1:** the call-log argument-copy test fails when the copy it guards is deleted. +- **10.M3:** a typed call through a handle in a diamond composition is tested. +- **10.M4:** `tests/test_docs.py` checks the relative links of every bundled page, not only + `index.md`. + +## Out of scope + +- The 29 won't-fix and 9 pre-publish items in [triage.md](triage.md). +- The `seahaven new --hub` root `__init__.py` fix (another branch). +- The review's mild findings. diff --git a/specs/projects/cr_moderate_fixes/implementation_plan.md b/specs/projects/cr_moderate_fixes/implementation_plan.md new file mode 100644 index 0000000..25dc943 --- /dev/null +++ b/specs/projects/cr_moderate_fixes/implementation_plan.md @@ -0,0 +1,25 @@ +--- +status: complete +--- + +# Implementation Plan: CR Moderate Fixes + +One commit per phase. Details are in [architecture.md](architecture.md) and the component doc each +phase names. + +## Phases + +- [x] **Phase 1: Low-risk code and test batch.** 11.M1 (no fsync on instance creation), 7.M2 + (`serve` binds `127.0.0.1`), 5.M3 (`step(ListToolsAction())`), 6.M1 (import errors name their + file and line), 4.M2 (hosts-first startup-hook order), 10.M1, 10.M3 and 10.M4 (tests, including + the link check over every docs page). See + [components/phase_1_batch.md](components/phase_1_batch.md). +- [x] **Phase 2: The `bulk()` deadlock (2.M1).** A per-thread re-entrant gate; `bulk()` marks the + thread and takes no slot. Its own commit. See + [components/phase_2_bulk_gate.md](components/phase_2_bulk_gate.md). +- [x] **Phase 3: Middleware per contributing route (4.M1).** A chain per contributed entry, built + from the route that contributed it. Its own commit. See + [components/phase_3_route_middleware.md](components/phase_3_route_middleware.md). +- [x] **Phase 4: Docs.** 9.M1 (SH103's rationale), 9.M7, 9.M8, 8.M4 (projecttracker audit-trail + claims), 2.M2/4.M3 (composite `bulk()` is not all-or-nothing when a commit fails), and 9.M6 (the + history and spec-citation sweep). See [components/phase_4_docs.md](components/phase_4_docs.md). diff --git a/specs/projects/cr_moderate_fixes/phase_plans/phase_1.md b/specs/projects/cr_moderate_fixes/phase_plans/phase_1.md new file mode 100644 index 0000000..cf1adc6 --- /dev/null +++ b/specs/projects/cr_moderate_fixes/phase_plans/phase_1.md @@ -0,0 +1,79 @@ +--- +status: complete +--- + +# Phase 1: Low-risk code and test batch + +## Overview + +Eight independent, low-risk fixes from the September 2026 deep review, each designed and +prototyped in [components/phase_1_batch.md](../components/phase_1_batch.md): faster instance +creation (11.M1), a loopback default for `seahaven serve` (7.M2), a typed client that parses a +`ListToolsAction` step (5.M3), import errors placed at their file and line (6.M1), hosts-first +startup-hook order (4.M2), and three test-only items (10.M1, 10.M3, 10.M4). The component doc is the +design; this plan lists the concrete steps. + +## Steps + +1. **11.M1** `src/seahaven/db.py`: in `open_instance`, set `synchronous=OFF` before + `journal_mode=WAL` (drop the `NORMAL` line), with a comment giving the constraint (the sweep + deletes an instance directory after a crash). In `_build`, set `synchronous=OFF` on the + builder before the DDL runs. +2. **7.M2** `src/seahaven/openenv/serve.py`: `DEFAULT_HOST = "127.0.0.1"`, comment says `/ws` has + no authentication, so network reach is the operator's choice (`--host 0.0.0.0`); the scaffolded + container passes it itself. `src/seahaven/cli/serve.py`: help text `(default 127.0.0.1)`. + Docs: `serving_and_openenv.md` option row and example, `reference/cli.md` row and example. +3. **5.M3** `src/seahaven/openenv/client.py::_parse_result`: pick `ListToolsObservation` when the + observation has a `tools` field, else `SeahavenObservation`. Update its docstring and + `list_tools`'s. One line in `reference/api.md` near the client entry. +4. **6.M1** `src/seahaven/cli/__init__.py`: + - `CliError.__init__(message, *, code=None, path=None, line=None, fix=None, root=None)`; + `root` is the project the path is shown relative to (defaults to `path`), because `path` now + names a file for an import failure and `check` renders findings relative to the project. + - `_import`: the generic `except Exception` branch raises `_import_failure(module_name, error, + root)`. The non-DDL `SeahavenError` branch also gets a location from `_world_frame` and the fix + `fix the error at `. + - `_no_world` raise passes `fix="give the package a world, or point at the one it has"`. + - New helpers `_import_failure`, `_world_frame`, `_authored`, `_is_the_world_module`, `_shown` + per the component doc. + - `src/seahaven/cli/check.py`: `collect` uses `error.root`; `_finding_for` passes + `line=error.line` and `fix=error.fix or `; delete the shared-fix comment. + - `src/seahaven/pytest_plugin.py`: comment only. + - `docs/reference/lints.md` SH501 entry: an import that fails for another reason is reported + at the file and line that raised it. +5. **4.M2** `src/seahaven/composition.py`: new exported `hosts_first(composition) -> list[Node]` + (DFS from the root in `add_world` order, emitting a node once its last distinct host has been + emitted; assert every node is emitted). `instances.py::_run_startup_hooks` iterates it; update + its docstring. `docs/composition.md` hook-order sentence. +6. **10.M1** `tests/test_call_log.py`: replace the argument-mutation test with the two tests of + the component doc. +7. **10.M3** `tests/test_typed_call.py`: add the diamond-under-a-handle test. +8. **10.M4** `tests/test_docs.py`: replace the index-only link test with a parametrised check + over every page, `README.md` and `CONTRIBUTING.md` that resolves every relative link and + every fragment against GitHub heading slugs and explicit anchors; keep the out-of-docs test + against the new pattern; add a slug unit test. + +## Tests + +- `test_db.py::test_an_instance_connection_is_hardened`: `synchronous` is `0`. +- `test_db.py::test_opening_an_instance_file_does_not_fsync` (parametrised over opening a copied + file and building a blank one): a counting VFS sees no `xSync`. +- `test_instances.py::test_every_node_of_a_fixture_instance_opens_without_durability`: both nodes + of a composite fixture instance are `synchronous=0`, `journal_mode=wal`. +- `test_serve.py`: default bind pair is `("127.0.0.1", 8000)`; `host="0.0.0.0"` still prints the + loopback console URL. +- `test_cli_new.py`: the hub `Dockerfile` CMD passes `"--host", "0.0.0.0"`. +- `test_client.py`: unknown-key refusal rewritten; `_parse_result` answers a + `ListToolsObservation` for a tool list; live `step(ListToolsAction())` answers the tool list + and a later call still works. +- `test_find_world.py`: syntax error, name error and missing dependency in world code are placed + at their file and line; `--world` naming a missing module has a fix naming `--world`. +- `test_cli_check.py`: an import error is a finding at the line that raised it (through + `seahaven check`); the no-world finding keeps its fix. +- `test_pytest_plugin.py::test_a_world_that_does_not_import_fails_with_its_file_and_line`. +- `test_composite_instance.py`: a shared node runs after every host; a host seeding a shared node + seeds it the same composed as alone. +- `test_composition.py::test_hosts_first_is_the_canonical_preorder_without_sharing`. +- `test_call_log.py`: the two argument-copy tests. +- `test_typed_call.py::test_a_handle_reaches_a_function_of_a_node_two_routes_beneath_it_reach`. +- `test_docs.py`: every relative link and anchor resolves, per page; slug unit test. diff --git a/specs/projects/cr_moderate_fixes/phase_plans/phase_2.md b/specs/projects/cr_moderate_fixes/phase_plans/phase_2.md new file mode 100644 index 0000000..6dc09e6 --- /dev/null +++ b/specs/projects/cr_moderate_fixes/phase_plans/phase_2.md @@ -0,0 +1,67 @@ +--- +status: complete +--- + +# Phase 2: The `bulk()` deadlock (2.M1) + +## Overview + +An `inst.call(...)` made inside `with inst.bulk():` queues for the process-wide concurrency gate +while its thread holds the instance lock, so a second thread holding the last slot and waiting for +that lock deadlocks both. A tool that calls another instance at a gate of 1 hangs one thread the +same way. This phase makes the gate re-entrant per thread and has `bulk()` mark the thread without +taking a slot, per [components/phase_2_bulk_gate.md](../components/phase_2_bulk_gate.md) (variant +B). No public signature changes, and HTTP requests (which run inside `bulk()`) stay ungated. + +## Steps + +1. `src/seahaven/instances.py`, beside `gate()`: a module-level `_gated = threading.local()`, with + a comment giving the external constraint (Starlette's `run_in_threadpool` and + `asyncio.to_thread` copy context variables into worker threads, so a `ContextVar` would mark a + thread that holds none of the locks). +2. Rewrite `gate(*, bypass: bool = False)`: + - return straight through when `_gated.active` is already set on this thread; + - otherwise read `_gate` once (`None` when `bypass`), acquire it if there is one, then set + `_gated.active = True`; in `finally` clear the flag, then release the semaphore it took. + - Docstring gains: "Re-entrant per thread: a block opened inside another takes nothing, because + by then the thread may hold an instance lock." +3. `Instance._bulk`: after `_refuse_if_formatting()`, enter `gate(bypass=True)` before + `self._held()` in the same `with`. A one-line comment: it marks the thread and takes no slot, + because HTTP requests run inside `bulk()` and the gate does not apply to them + (`docs/http_apis.md`). +4. Module docstring, rule 2 ("The gate before the lock"): rewrite to the text in component doc §5. +5. `src/seahaven/docs/serving_and_openenv.md`, "The concurrency gate": after "Instance creation, + tool listing and the control tool bypass it entirely." add "`inst.bulk()` bypasses it too, and + so does any call made inside a call or a `bulk()` block on the same thread." + +## Tests + +All in `tests/test_instances.py`, after `test_the_gate_is_released_after_a_call_that_raised`. A +helper `TimedGate(instances._Gate)` whose `acquire` waits at most `WAIT` and raises +`AssertionError` when it runs out, counts `taken`, and sets a `contender_has_slot` event when a +thread named `contender` takes a slot; installed with `monkeypatch.setattr(instances, "_gate", +...)`. Each flag-checking body runs on a fresh `Caller` thread, because the docs-examples +`receivers` fixture leaves the main thread inside a `bulk()`. + +- `test_a_call_inside_bulk_does_not_queue_behind_a_caller_waiting_for_the_lock[same|other]`: the + author thread enters `x.bulk()`; a `contender` thread calls `x.call` and takes the only slot, + then waits for X's lock; the author calls `x.call` (or `y.call`). Both threads finish, and both + rows are present. +- `test_a_tool_calling_another_instance_does_not_wait_for_its_own_slot`: at gate 1, a tool on X + calls a captured `y.call`; the call returns and Y has the row. +- `test_nested_bulk_blocks_take_no_slot`: `x.bulk()` containing `x.bulk()` and `y.bulk()` with a + call in each: `taken == 0`; a later call gives `taken == 1`. +- `test_bulk_that_raised_leaves_the_thread_gated_again`: a call then a raise inside `x.bulk()`: the + row is rolled back, `taken == 0`, and the next call gives `taken == 1`. +- `test_bulk_does_not_wait_for_a_full_gate`: a slot holder on X at gate 1; `y.bulk()` with a write + on another thread finishes within `WAIT / 5` (passes on today's code on purpose: the no-change + guard). +- `test_a_resize_inside_bulk_does_not_make_its_calls_queue`: `set_concurrency(0)`, enter + `x.bulk()`, install `TimedGate(1)`, occupy its slot from a thread calling Y, then `x.call` + returns. +- `test_a_control_tool_calling_its_instance_passes_a_full_gate`: a control tool calls + `live.call(...)` on its own instance while another thread holds the only slot; it returns + within `WAIT / 5`. + +Confirm the component doc's mutations: without the flag check in `gate()`, without `gate()` in +`_bulk`, and without the reset in `finally`, the named tests fail. diff --git a/specs/projects/cr_moderate_fixes/phase_plans/phase_3.md b/specs/projects/cr_moderate_fixes/phase_plans/phase_3.md new file mode 100644 index 0000000..dff47db --- /dev/null +++ b/specs/projects/cr_moderate_fixes/phase_plans/phase_3.md @@ -0,0 +1,81 @@ +--- +status: complete +--- + +# Phase 3: Middleware per contributing route (4.M1) + +## Overview + +An agent call to a contributed tool runs the middleware of the node's canonical route today. When +a node becomes reachable by a second route, a tool contributed through the first route silently +loses the middleware of the worlds on that route. This phase moves the chain from the node to the +contributed entry: each `Contributed` carries the chain of the route that contributed it, built +once per distinct route at the seal. Per +[components/phase_3_route_middleware.md](../components/phase_3_route_middleware.md). Behaviour is +unchanged when no node is shared, and handle calls still run only the owner's own chain. + +## Steps + +1. `src/seahaven/composition.py`, data model: + - `Contributed` gains `chain: Handler`, with a docstring saying the chain belongs to the entry + because one node reached by two routes contributes an entry through each. + - `Node.agent_chain` is removed; `Node.internal_chain` is renamed `own_chain`, and the field + comment says it serves a handle call and an agent call to one of the root's own tools. + - `_Candidate.entry` becomes `name: str`, `tool: Tool`, `node: Node`, + `hosts: tuple[NodeKey, ...]` (the node keys on the contributing route, from the node whose + list this is down to the owner). The docstring describes `hosts`. + - Module docstring: "the chains their calls descend" becomes "the chain each tool on the + surface descends". +2. `_walk` drops `parents` and returns four values; `resolve` and `_nodes` follow. +3. `_route_layers(route: Sequence[NodeKey])` returns every middleware of every node on a route, + outermost first; `_own_layers` is removed. `_nodes` builds + `own_chain=build_route_chain(_route_layers((key,)), key)`. +4. `_contribute`: a node's own tool starts with `hosts=(key,)`. `_through(host, added, inner)` + prepends `host`. `_fold` keys on `candidate.name`. +5. `resolve`: `tools = _entries(nodes[order[0]], surface.candidates)`, where `_entries` builds one + chain per distinct `hosts` route, seeded with `{(root.key,): root.own_chain}`. +6. `src/seahaven/instances.py`: `_Target` gains `chain: Handler`; `_target` fills it from + `entry.chain` on the by-name and by-function branches, and from `composition.root.own_chain` + for a control tool. `_dispatch` runs `target.chain(ctx, call)`, with the comment reworded (the + chain comes from this call's resolution, which reseals after a registration). Import `Handler`. +7. `src/seahaven/world.py`: `World.chain` returns `root.own_chain`; the docstring's "on the node + that owns it" becomes "on its entry in the tool list". `src/seahaven/handles.py`: + `owner.own_chain(...)`. +8. Docs: `src/seahaven/docs/composition.md` `## Middleware` first sentence replaced per the + component doc §3; `src/seahaven/docs/reference/lints.md` SH207 **Why** gains "Each name also + runs the middleware of its own route." + +## Tests + +`tests/test_composite_dispatch.py`, "the chain" section, with a helper +`shared_under_a_middle(tmp_path, trace)` (host adds traced `middle`, which adds traced `leaf`, +with `m_`; host then adds `leaf` with `l_`): + +- `test_the_agent_chain_is_the_whole_contributing_route`: renamed from `..._canonical_route`, + body unchanged. +- `test_sharing_a_node_keeps_the_middle_worlds_middleware_on_what_it_contributes`: company adds + shop (`shop_`, recording middleware, adds payments); shop's middleware runs on + `shop_payments_write` before and after `company.add_world(payments, tool_prefix="pay_")`, and + never on `pay_payments_write`. +- `test_each_name_of_a_shared_tool_runs_its_own_routes_middleware`: `m_leaf_write` traces + host/middle/leaf, `l_leaf_write` host/leaf; both reads see both rows. +- `test_both_sides_of_a_diamond_run_their_own_middle`: `l_` gives host/left/leaf, `r_` gives + host/right/leaf. +- `test_a_route_the_lists_filter_out_does_not_choose_the_chain`: canonical edge with + `tool_allow_list=[]`, middle's route contributes; `leaf_write` runs host/middle/leaf, by name + and by function reference. +- `test_a_shared_leaf_three_deep_keeps_every_world_between`: `leaf_write` gives host/a/b/leaf, + `x_leaf_write` gives host/leaf. +- `test_a_middle_worlds_error_handler_shapes_the_errors_it_contributes`: `m_refuse` raises + middle's error, `l_refuse` raises `Boom`. +- `test_a_middleware_registered_later_reaches_a_route_through_a_shared_node`: on a live instance, + a middleware registered on `middle` runs on `m_leaf_write`, not on `l_leaf_write`. +- `test_a_nested_call_into_a_shared_node_runs_only_the_owners_layer` (guard): a host tool calling + `ctx.worlds.middle.worlds.leaf.call("leaf_write")` traces host/leaf. + +`tests/test_composition.py`, "contribution" section (seal only): + +- `test_each_entry_carries_the_chain_of_the_route_that_contributed_it`: `m_leaf_write` and + `l_leaf_write` share `.node` and differ in `.chain`. +- `test_entries_of_one_route_share_one_chain`: `m_leaf_write.chain is m_leaf_read.chain`, and a + root tool's `.chain is host.chain`. diff --git a/specs/projects/cr_moderate_fixes/phase_plans/phase_4.md b/specs/projects/cr_moderate_fixes/phase_plans/phase_4.md new file mode 100644 index 0000000..cd3495a --- /dev/null +++ b/specs/projects/cr_moderate_fixes/phase_plans/phase_4.md @@ -0,0 +1,64 @@ +--- +status: complete +--- + +# Phase 4: Docs + +## Overview + +The last phase corrects what the bundled docs, ProjectTracker's notes and two code comments say, +now that phases 1-3 have settled the code. It restates why lint SH103 exists (9.M1), fixes the +composite cost note (9.M7) and ProjectTracker's check command (9.M8), narrows the audit-trail and +`actor_id` claims to the writes that make them true (8.M4), documents that a composite `bulk()` is +not all-or-nothing when a commit fails (2.M2/4.M3), and removes history, benchmark figures, +version stories and spec citations from the bundled docs (9.M6). Per +[components/phase_4_docs.md](../components/phase_4_docs.md). No heading is renamed, so no anchor +or `tests/test_docs.py` change. + +## Steps + +1. **SH103 (9.M1).** + - `db_schema_and_fixtures.md`, the seeded-rows paragraph: replace from "The wall-clock rule + below is unchanged…" to the end of that sentence with Edit 1a. + - `db_schema_and_fixtures.md`, "No expression reads the wall clock": replace the three + sentences from "The problem is" with Edit 1b. + - `reference/lints.md`, SH103: the "Rule" names every place the regex covers (Edit 1d); the + "Why" becomes Edit 1c. "Fix" and the false-positive paragraph are unchanged. +2. **Composite cost (9.M7, H18).** `composition.md`, the "idle composite instance" bullet: replace + the design-target and benchmark sentences with the per-node cost sentence. +3. **ProjectTracker check (9.M8).** `projecttracker.md`, "Its tests": the fence runs + `uv run seahaven check --world projecttracker:world`, with "Run both from the root of the + Seahaven repository." above it. +4. **Audit trail and `actor_id` (8.M4).** Apply the component's table to + `worlds/projecttracker/README.md`, `projecttracker.md` (schema intro and viewer paragraph), + `worlds/projecttracker/AGENTS.md` (viewer bullet, plus one new bullet naming what the trail does + not record), and the `_events.py` module docstring. `schema/001_core.sql` is not edited. +5. **Composite `bulk()` atomicity (2.M2/4.M3).** + - `instances.py` `Instance.bulk` docstring: the last two sentences say what happens when the + block raises and when a commit fails. + - `instances.py` `_bulk`: the comment above the transaction loop becomes the constraint comment + (one file per node, SQLite cannot commit two files atomically, reverse commit order). + - `composition.md` "One instance, many stores": after the `inst.bulk()` sentence, add the + three-sentence paragraph on a raise, a failed commit, and not freezing after one. + - `reference/api.md`, the `inst.bulk()` row: the root node's `Ctx`, one transaction per node, + linked to `composition.md#one-instance-many-stores`. +6. **History sweep (9.M6).** Apply H1-H24 as the component table gives them: + - `state.md`: drop the bench probe and its figures (H1-H4), keeping "Recording is not free…" + and "A read-only call records nothing, so it costs less than a write." + - `serving_and_openenv.md`: delete H5, H7, H9, H10; rewrite H6 and H8 as present-tense fact; + rewrite "which is the whole integration" in the Kiln section. + - `composition.md`: H11-H17 as listed; delete "That example is the whole feature." + - `reference/api.md`: H19 without spec citations or a count; H20 names the two cases. + - `db_schema_and_fixtures.md`: H21, H22. + - `projecttracker.md`: delete H24. +7. Wrap every changed paragraph at 100 columns; run `ruff format` and the docs tests. + +## Tests + +- `tests/test_composite_instance.py::test_a_bulk_commit_that_fails_keeps_the_nodes_committed_before_it`: + a two-node instance whose root schema has a `DEFERRABLE INITIALLY DEFERRED` foreign key; a + `bulk()` block writes a child row and a root row that breaks the key. `bulk()` raises `DbError`, + the root has no row, and the child keeps its row and its change-log record. Drives it through + `world.instance(...)`, and pins the behaviour the new docs describe. +- Existing guards cover the rest: `tests/test_docs.py` (page list, every relative link and anchor) + and `tests/test_docs_examples.py` (every example still runs). diff --git a/specs/projects/cr_moderate_fixes/project_overview.md b/specs/projects/cr_moderate_fixes/project_overview.md new file mode 100644 index 0000000..35b8927 --- /dev/null +++ b/specs/projects/cr_moderate_fixes/project_overview.md @@ -0,0 +1,22 @@ +--- +status: complete +--- + +# CR Moderate Fixes + +Fix the moderate findings from the full-repo deep code review (September 2026) that the maintainer +decided to fix. The review's 56 moderate findings were triaged one by one: 29 are won't-fix, 9 are +pre-publish decisions, 1 was already fixed, and 17 are fixed by this project. The triage, with the +reason for every decision, is in [triage.md](triage.md). + +Not a ton of phases. A good clustering of related items, with the low-risk ones grouped together: + +- A batch of low-risk code and test fixes. +- The `bulk()` deadlock fix (2.M1) in its own phase and commit, so it can be reviewed and rolled + back on its own. No ergonomic change for default callers. +- Middleware per contributing route (4.M1) in its own phase, so its complexity and tests can be + judged on their own. +- A docs phase with every docs-only fix, including the history and spec-citation cleanup (9.M6). + +Out of scope: the pre-publish items, the `seahaven new --hub` fix (on another branch), and the +review's mild findings. diff --git a/specs/projects/cr_moderate_fixes/triage.md b/specs/projects/cr_moderate_fixes/triage.md new file mode 100644 index 0000000..3b7414d --- /dev/null +++ b/specs/projects/cr_moderate_fixes/triage.md @@ -0,0 +1,148 @@ +# Moderate Findings: Triage Decisions + +Decisions on the 56 moderate findings from the full-repo deep code review of `0360978` +(September 2026; the review files are not in the repository). IDs are +`phase.M#`: the Nth item under "Moderate" in `phase__feedback.md`. Two IDs on one row are the +same issue, found by two phases. + +Status: **decided.** Every moderate is classified. The "Fix" items are the scope of this project; +the "Pre-publish" items are considered before release. + +## Summary + +| Bucket | Count | IDs | +|---|---:|---| +| Already fixed | 1 | 1.M2 (in `a1680c0`, the tool-argument validation fix) | +| Won't fix (permanent) | 29 | See [Won't fix](#wont-fix-permanent) | +| Fix | 17 | See [Fix](#fix): 13 easy fixes and 4 larger items | +| Pre-publish: consider before release | 9 | See [Pre-publish](#pre-publish-consider-before-release) | + +--- + +## Won't fix (permanent) + +Grouped by reason. + +### A1. Goes away once published + +| ID | Issue | Why | +|---|---|---| +| 12.M2 | README says `uv add seahaven` / `pip install seahaven`, which installs the PyPI placeholder | Correct as written once the real package is published | +| 6.M7 | `seahaven new` prints `uv sync` as a next step, which pulls the placeholder | Same: correct once published | + +### B. Follows from the critical decisions (the world owns SQL limits) + +| ID | Issue | Why | +|---|---|---| +| 7.M1 | `run_sql` has no default cap on result size | Same stance as critical #2: the world sets `max_rows` / `max_bytes` | +| 10.M2 | No test runs a recursive CTE through the sandbox | Callers are not expected to write recursive queries | + +### D. Rare author-side edge cases that fail loudly or never come up + +| ID | Issue | Why | +|---|---|---| +| 1.M1 | A parameter named `_x` or `model_config` is silently dropped | Odd names; every call then fails, so it is caught on first use | +| 1.M3 | A result reshaped by middleware is not checked before commit | Fails loudly on the wire; middleware rarely reshapes results | +| 1.M4 | `serialise` checks a model's raw attributes, not what it renders | Exotic model serializers only | +| 1.M5 | `ToolError.details` is not checked the way results are | Needs sets or bytes in error details | +| 2.M3 | Two concurrent freezes of the same fixture id can corrupt it | An authoring-time action by one person | +| 2.M4, 6.M2 | Fixture id vs directory name, and duplicate ids, are not checked | Only after hand-renaming a fixture directory; the runtime error is loud | +| 6.M3 | A non-UTF-8 fixture file crashes `seahaven check` | We write these files ourselves | +| 6.M4 | Lint SH103 misses date functions called with no argument | Lints are an advisory safety net | +| 6.M5 | Lint SH201 misses `datetime.today()` and `time.time_ns()` | Same | +| 6.M6 | The missing-extra message suggests an extra that conflicts with the installed one | The user sees a clear conflict error next | +| 3.M2 | FTS3/FTS4 and R*Tree internal tables fill the change log | No world uses them (projecttracker uses FTS5) | +| 5.M4 | The published reset schema and the server disagree on edge fields | Only a startup hook with required keywords or very long `episode_id`s | +| 9.M5 | The "what registration refuses" list is in a guide page, not `reference/` | Placement only; the content is correct | +| 8.M3 | projecttracker's fixture history does not show closing an issue dropping its assignee | Fixture realism only; the tools behave correctly | + +### E. Performance: wait for a real workload + +| ID | Issue | Why | +|---|---|---| +| 11.M4 | The result-safety check is 20% of a one-row read, and runs twice when served | Fine at current scale | +| 11.M5 | 500 sessions need more open files than the common 1024 soft limit | Operators raise `ulimit`; a docs line later | +| 11.M6 | The default gate can starve waiting callers | Already known and documented in the code | +| 8.M1 | projecttracker's index comments are wrong about sort order | Results are still correct; fixtures are small | +| 8.M2 | No `issue_id` index on comments or issue events | Small fixture data | + +### F. Decided in the easy-fix round + +| ID | Issue | Why | +|---|---|---| +| 5.M5 | After a client-side timeout, the next call silently runs on a new, blank session | Maintainer decision | +| 5.M2 | `reset` and `state` failures are not scrubbed the way `step`'s are | By design: `reset` and `state` answer the orchestrator, not the agent. Scrubbing is not wanted there | + +### G. Decided in the final round + +| ID | Issue | Why | +|---|---|---| +| 5.M1, 11.M3 | `state` is built on OpenEnv's event loop; the default format re-renders the whole log per read | We own the state code and it is fast enough | +| 3.M1 | Clock SQL functions are registered deterministic, so `'now'` can get into a generated column, an index or a partial index | Removing the flag also refuses deterministic uses such as an index on `date(created_at)`, and slows scans that call the clock. Lint SH103 already flags `'now'` anywhere in the schema | + +--- + +## Fix + +### Easy fixes + +Clear what to do, a small change, low risk. + +#### Docs only + +| ID | Issue | Cost | +|---|---|---| +| 9.M1 | The docs' reason for SH103 is false: the SQL clock already writes canonical text | Rewrite one paragraph each in `db_schema_and_fixtures.md` and `reference/lints.md` | +| 9.M7 | `composition.md` says the benchmark measures one node per instance; it now measures composite trees | Fix one sentence | +| 9.M8 | `projecttracker.md` runs `uv run seahaven check` from the repo root, where it fails | Fix one code block | +| 8.M4 | projecttracker README and AGENTS.md overstate the audit trail and `actor_id` | Reword a few lines in 3 files | +| 2.M2, 4.M3 | Composite `bulk()` claims all-or-nothing, but is not when a later commit fails | Accepted as behaviour: fix the code comment, add 2-3 lines to `composition.md` | + +#### Tests only + +| ID | Issue | Cost | +|---|---|---| +| 10.M1 | The argument-copy test still passes with the copy deleted | Rewrite one test so the caller mutates its own list after the call | +| 10.M3 | A typed call through a handle in a diamond composition is untested | Add one test | +| 10.M4 | `test_docs.py` checks links only in `index.md` | Widen the check to every page | + +#### Small code changes + +| ID | Issue | Cost | +|---|---|---| +| 11.M1 | Switching each new instance to WAL fsyncs: more than half of fork time | `synchronous=OFF` before the WAL switch in `db.py` | +| 7.M2 | `seahaven serve` binds `0.0.0.0` with no auth | Default to `127.0.0.1`, plus a docs row; the Dockerfile template already passes `--host 0.0.0.0` | +| 5.M3 | `client.step(ListToolsAction())` crashes with a pydantic error | Parse a reply that carries `tools` as `ListToolsObservation`, plus a test | +| 6.M1 | An import error in world code shows no file or line, and gives the wrong fix text | Report the exception and its last frame; drop the wrong fix text (CLI and pytest plugin) | + +--- + +### Larger fixes + +| ID | Issue | Decision | +|---|---|---| +| 9.M6 | History, bench figures and spec citations in the bundled docs | Clean up, per the AGENTS.md docs style | +| 4.M2 | Startup hooks can run in the wrong order when a node is shared | Run hooks in topological order over the node graph: every host before every node it adds. Order is unchanged wherever nothing is shared. Add a test | +| 2.M1 | `inst.call(...)` inside `bulk()` takes the gate while holding the instance lock, so it can deadlock (reproduced with a gate of 1) | Option (a): make the gate re-entrant per thread. `bulk()` takes a gate slot and records it in a thread-local; a thread already holding a slot skips the gate (covers `Y.call()` inside X's `bulk()`). **No ergonomic change for default callers.** Add a two-thread test with a timeout | +| 4.M1 | A middle world's middleware stops running on its tools once a node it adds is shared | Option (a): build the middleware chain for each contributed tool from its contributing route, not the node's canonical route. Add tests for the shared-node case | + +### Plan notes for the spec + +- **Docs phase:** all the docs-only easy fixes, plus 9.M6. +- **2.M1 gets its own phase and commit**, so it can be reviewed and rolled back on its own. +- **4.M1 gets its own phase and commit**, so its complexity and tests can be judged on their own. +- The other easy fixes (tests only, small code changes) and 4.M2 can be batched. + +## Pre-publish: consider before release + +These only matter once Seahaven is public. Consider them before release. + +| ID | Issue | To decide | +|---|---|---| +| 12.M1 | Scaffold pins `seahaven~=X.Y`, which only pins the major version | Pin style that follows the minor version | +| 12.M4, 9.M4 | MIT `LICENSE` exists, but `pyproject.toml` declares no licence; CONTRIBUTING and AGENTS.md say there is no LICENSE | Licence metadata, and aligning AGENTS.md and CONTRIBUTING | +| 12.M3 | The licence gate reads only package metadata; numpy's wheel bundles libgfortran (GPL-3 with the GCC runtime exception) and libquadmath (LGPL-2.1) | Record a legal decision; allow-list them, or teach the gate about bundled libraries | +| 12.M5 | `projecttracker` is a third party's PyPI name; `seahaven-xmlrpc` is unclaimed; docstrings say `pip install "projecttracker[serve]"` | Rename or claim the names; fix the docstrings | +| 9.M3 | README promises Hugging Face publishing, which does not work yet | Lands with the `--hub` branch, or soften the README | +| 5.M6 | Hub-pushed worlds share one global instance on `/web/*` | `--hub` branch: disable it, or document it as a single-user demo | +| 9.M2, 11.M2 | README capacity and speed claims are not measured by `bench/` | Soften the claims, or extend the benchmark | diff --git a/src/seahaven/cli/__init__.py b/src/seahaven/cli/__init__.py index f49cdea..3682de0 100644 --- a/src/seahaven/cli/__init__.py +++ b/src/seahaven/cli/__init__.py @@ -50,6 +50,9 @@ _OVERRIDE = "or pass --world module:attr" +# Seahaven's own package: its frames are never where a world's import failed. +_SEAHAVEN_DIR = Path(__file__).resolve().parents[1] + class CliError(Exception): """A failure with a fix in it, printed as one line and never as a traceback. @@ -59,13 +62,29 @@ class CliError(Exception): SH501, and one that failed on the world's DDL is SH104. A `CliError` without a code -- no `pyproject.toml`, a malformed `--world` -- is the user's to fix before any lint can run at all. + + `path` and `line` place a finding, `fix` is the edit `check` prints beside + it, and `root` is the project `check` shows the path relative to: the same + as `path` unless `path` names a file inside it. """ - def __init__(self, message: str, *, code: str | None = None, path: Path | None = None) -> None: + def __init__( + self, + message: str, + *, + code: str | None = None, + path: Path | None = None, + line: int | None = None, + fix: str | None = None, + root: Path | None = None, + ) -> None: super().__init__(message) self.message = message self.code = code self.path = path + self.line = line + self.fix = fix + self.root = root or path @dataclass(frozen=True) @@ -199,21 +218,23 @@ def _import(module_name: str, attribute: str, *, root: Path) -> Discovery: module = import_module(module_name) except SeahavenError as error: # A world that refused to be constructed. Its message is written for its - # author and is the whole of what `check` should print: SH104 when the - # DDL is what SQLite refused, SH501 for everything else a `World` raises - # at import -- a duplicate tool name, a middleware of the wrong shape. - code = "SH104" if DDL_DOES_NOT_EXECUTE in str(error) else "SH501" - raise CliError(str(error), code=code, path=root) from error + # author: SH104 when the DDL is what SQLite refused, SH501, placed at the + # line of world code that made the call, for everything else a `World` + # raises at import -- a duplicate tool name, a middleware of the wrong shape. + if DDL_DOES_NOT_EXECUTE in str(error): + raise CliError(str(error), code="SH104", path=root) from error + raise _placed(str(error), module_name, error, root) from error except Exception as error: - raise CliError( - f"cannot import {module_name!r}: {_last_traceback_line(error)}", - code="SH501", - path=root, - ) from error + raise _import_failure(module_name, error, root) from error imported = frozenset(sys.modules) world = getattr(module, attribute, None) if not isinstance(world, World): - raise CliError(_no_world(module_name, attribute, world), code="SH501", path=root) + raise CliError( + _no_world(module_name, attribute, world), + code="SH501", + path=root, + fix="give the package a world, or point at the one it has", + ) package = _package_of(module) if package is None: # Functional spec §2.1: a world is a package, never a single module and @@ -254,6 +275,87 @@ def _no_world(module_name: str, attribute: str, found: object) -> str: ) +def _import_failure(module_name: str, error: Exception, root: Path) -> CliError: + """SH501 for an import that raised, placed at the line of the world's code that raised it.""" + summary = _last_traceback_line(error) + if isinstance(error, ModuleNotFoundError) and _is_the_world_module(module_name, error.name): + return CliError( + f"cannot import {module_name!r}: {summary}", + code="SH501", + path=root, + fix=( + f"make {module_name!r} importable (the package the [project] name names, under " + f"{SOURCE_DIRNAME}/), or point at the world with --world module:attr" + ), + ) + return _placed(f"cannot import {module_name!r}: {summary}", module_name, error, root) + + +def _placed(message: str, module_name: str, error: Exception, root: Path) -> CliError: + """SH501 at the innermost frame of the world's own code, or with the command that finds it.""" + command = f"python -c 'import {module_name}'" + place = _world_frame(error, root) + if place is None: + return CliError( + f"{message}; {command} prints the whole traceback", + code="SH501", + path=root, + fix=f"fix the error {command} ends in", + ) + file, line = place + where = f"{_shown(file, root)}:{line}" + return CliError( + f"{message}, at {where}; {command} prints the whole traceback", + code="SH501", + path=file, + line=line, + fix=f"fix the error at {where}", + root=root, + ) + + +def _world_frame(error: Exception, root: Path) -> tuple[Path, int] | None: + """The file and line in the world's own code that raised, preferring the project's own files. + + A `SyntaxError` is raised by the compiler, not by a frame of the file it is + in, so its place comes off the exception. + """ + if isinstance(error, SyntaxError) and error.filename and error.lineno: + path = Path(error.filename) + if path.is_file(): + return path.resolve(), error.lineno + places = [ + (Path(frame.filename).resolve(), frame.lineno) + for frame in reversed(traceback.extract_tb(error.__traceback__)) + if frame.lineno is not None and _authored(Path(frame.filename)) + ] + in_project = [place for place in places if place[0].is_relative_to(root)] + return next(iter(in_project or places), None) + + +def _authored(path: Path) -> bool: + """Whether a traceback frame is in code an author wrote, not in Python, a library or Seahaven. + + `sys.prefix` holds a project's own virtual environment, and `sys.base_prefix` + the standard library. An `importlib` frame names a file that does not exist. + """ + if not path.is_file(): + return False + resolved = path.resolve() + outside = (_SEAHAVEN_DIR, Path(sys.prefix).resolve(), Path(sys.base_prefix).resolve()) + return not any(resolved.is_relative_to(directory) for directory in outside) + + +def _is_the_world_module(module_name: str, missing: str | None) -> bool: + """Whether the module that was not found is the one named, or a package above it.""" + return missing is not None and (missing == module_name or module_name.startswith(f"{missing}.")) + + +def _shown(path: Path, root: Path) -> str: + """`path` relative to the project when it is inside it, as `check` prints paths.""" + return str(path.relative_to(root)) if path.is_relative_to(root) else str(path) + + def _last_traceback_line(error: BaseException) -> str: """The exception as Python would print it, without the frames above it.""" return traceback.format_exception_only(error)[-1].strip() diff --git a/src/seahaven/cli/check.py b/src/seahaven/cli/check.py index cf0b04b..645c72e 100644 --- a/src/seahaven/cli/check.py +++ b/src/seahaven/cli/check.py @@ -69,7 +69,7 @@ def collect(explicit: str | None, start: Path) -> Report: except CliError as error: if error.code is None: raise - return Report(root=error.path or start, findings=[_finding_for(error, start)]) + return Report(root=error.root or start, findings=[_finding_for(error, start)]) return Report( root=found.root, findings=lint.run_all( @@ -86,9 +86,7 @@ def _finding_for(error: CliError, start: Path) -> Finding: code="SH501", severity="error", path=path, + line=error.line, message=error.message, - # The message already names the export and the override, because it is - # also what `find_world` raises outside `check`. Repeating it here would - # print one sentence twice on one line. - fix="give the package a world, or point at the one it has", + fix=error.fix or "", ) diff --git a/src/seahaven/cli/new.py b/src/seahaven/cli/new.py index 17ce578..9dc920d 100644 --- a/src/seahaven/cli/new.py +++ b/src/seahaven/cli/new.py @@ -21,6 +21,7 @@ import keyword import re import shutil +import sys from collections.abc import Iterator from importlib import resources from importlib.resources.abc import Traversable @@ -99,6 +100,14 @@ def render(name: str, target: Path, *, hub: bool = False) -> Path: f"{name!r} does not make a package name: {package!r} is not a Python identifier; " f"use letters, digits and underscores, starting with a letter" ) + # A world's package is imported by name, so one that shares a name with a + # module already on the path loses to it, and the scaffold fails its first + # `seahaven check` with an error about a module the author never wrote. + if package in sys.stdlib_module_names or package == "seahaven": + raise CliError( + f"{name!r} makes the package {package!r}, which is already the name of a module " + f"Python imports first; add a suffix, such as {package + '_world'!r}" + ) # The name reaches the rendered `world.py` verbatim, and `World(name=...)` # holds it to `names.why_not_a_name`. Refused here rather than there: a # scaffold that cannot be imported is met at the author's first `pytest`, in diff --git a/src/seahaven/cli/serve.py b/src/seahaven/cli/serve.py index 4fc9bed..750bf0d 100644 --- a/src/seahaven/cli/serve.py +++ b/src/seahaven/cli/serve.py @@ -26,7 +26,7 @@ def add_parser(subcommands: argparse._SubParsersAction[argparse.ArgumentParser]) description="Run this world's OpenEnv server: one world, many sessions.", ) add_world_option(parser) - parser.add_argument("--host", default=None, help="the address to bind (default 0.0.0.0)") + parser.add_argument("--host", default=None, help="the address to bind (default 127.0.0.1)") parser.add_argument("--port", type=int, default=None, help="the port to bind (default 8000)") # Underscores, not hyphens: this is OpenEnv's own option name, and a second # spelling of it here would be one more thing to translate. @@ -46,7 +46,7 @@ def add_parser(subcommands: argparse._SubParsersAction[argparse.ArgumentParser]) "--session-timeout", type=float, default=None, - help="seconds of idleness before a session is reaped; 0 disables the reaper (default 3600)", + help="seconds of idleness before a session is reaped; 0 turns the reaper off (default off)", ) parser.add_argument( "--include-control-tools", @@ -97,7 +97,7 @@ def run(args: argparse.Namespace) -> int: return 0 -def _session_timeout(given: float | None, default: float) -> float | None: +def _session_timeout(given: float | None, default: float | None) -> float | None: """The reaper's idleness budget: the default, the value, or off.""" if given is None: return default diff --git a/src/seahaven/cli/templates/base/pyproject.toml.tmpl b/src/seahaven/cli/templates/base/pyproject.toml.tmpl index f16f9c5..8a95176 100644 --- a/src/seahaven/cli/templates/base/pyproject.toml.tmpl +++ b/src/seahaven/cli/templates/base/pyproject.toml.tmpl @@ -25,6 +25,11 @@ dependencies = ["$seahaven_requirement"] serve = ["seahaven[serve]"] mcp = ["seahaven[mcp]"] +# `uv sync` installs the `dev` group by default, and pytest is not a dependency +# of seahaven itself: without this group `uv run pytest` has nothing to run. +[dependency-groups] +dev = ["pytest>=8"] + # Saying the two extras conflict is what keeps `uv lock` solvable: without this # table uv has to satisfy both at once, which no version of openenv and the MCP # SDK allows, and the lock fails rather than the install. diff --git a/src/seahaven/cli/templates/base/tests/test_fixtures.py.tmpl b/src/seahaven/cli/templates/base/tests/test_fixtures.py.tmpl index 0f09cff..502180a 100644 --- a/src/seahaven/cli/templates/base/tests/test_fixtures.py.tmpl +++ b/src/seahaven/cli/templates/base/tests/test_fixtures.py.tmpl @@ -1,7 +1,7 @@ -"""The recipe in `fixtures_src/generate.py`, exercised before it has made anything. +"""The recipe in `fixtures_src/generate.py`, exercised without touching what it has made. A fixture is bytes in the repository, and bytes whose recipe no longer runs are -bytes nobody can change. This world has no fixture frozen yet -- that is its +bytes nobody can change. A new world has no fixture frozen yet -- that is its author's first `build` -- so what is asserted here is the recipe itself: that `build` runs end to end, and that it freezes into the world it is handed. @@ -46,6 +46,7 @@ def test_the_recipe_freezes_the_empty_fixture( generate: Any, world: seahaven.World, tmp_path: Path ) -> None: """`build` into a temporary directory: the whole recipe, and nothing committed.""" + before = world.fixtures() elsewhere = copy.copy(world) elsewhere.fixtures_dir = tmp_path / "fixtures" elsewhere.fixtures_dir.mkdir() @@ -58,7 +59,7 @@ def test_the_recipe_freezes_the_empty_fixture( assert fixture.dir == tmp_path / "fixtures" / "empty" assert [frozen.id for frozen in elsewhere.fixtures()] == ["empty"] # The copy is what took the write: the imported world is where it was. - assert world.fixtures() == [] + assert world.fixtures() == before def test_the_recipe_defaults_to_this_packages_world(generate: Any, world: seahaven.World) -> None: diff --git a/src/seahaven/composition.py b/src/seahaven/composition.py index b29aa13..e8db47f 100644 --- a/src/seahaven/composition.py +++ b/src/seahaven/composition.py @@ -4,8 +4,9 @@ *graph* -- the same world reached by two routes is one store, a world added under a named scope is another. This module turns that graph into the one thing the rest of the framework reads: a `Composition`, the sealed tree of `Node`s with -their canonical paths, their files, their bound startup keywords, the chains -their calls descend, and the flat, insertion-ordered tool list an agent sees. +their canonical paths, their files and their bound startup keywords, and the +flat, insertion-ordered tool list an agent sees, with the chain each tool on the +surface descends. A leaf world is a composition of exactly one node, with path `main`, file `state.sqlite` and its own tools in registration order, so nothing downstream @@ -54,6 +55,7 @@ "bump", "canonical_tree", "epoch", + "hosts_first", "resolve", ] @@ -163,12 +165,11 @@ class Node: file_name: str schema_name: str bound_startup: Mapping[str, Any] - # What an agent-initiated call to a tool this node owns descends -- every - # node's middlewares along the canonical route, outermost first -- and what a - # call from host code through a handle descends, which is this node's own - # middlewares and nothing above them. Both end in `invoke` on this node. - agent_chain: Handler - internal_chain: Handler + # This node's own middlewares and nothing above them, then `invoke` on this + # node: what a call from host code through a handle descends, and what an + # agent call to one of the root's own tools descends. An agent call to any + # other tool descends its entry's chain (`Contributed.chain`). + own_chain: Handler def __repr__(self) -> str: return f"" @@ -211,11 +212,19 @@ def of(cls, node: Node, frozen_world_version: str | None = None) -> NodeReport: @dataclass(frozen=True, eq=False) class Contributed: - """One entry of the composite tool list: the name the agent sees, and who owns it.""" + """One entry of the composite tool list: the name the agent sees, who owns it, and its chain. + + `chain` is what an agent call to this entry descends: the middlewares of every + node on the route that contributed it, outermost first, then `invoke` on + `node`. It belongs to the entry rather than to the node, because one node + reached by two routes contributes an entry through each, and each entry runs + the middleware of its own route. + """ name: str tool: Tool node: Node + chain: Handler @dataclass(frozen=True, eq=False) @@ -274,10 +283,10 @@ def resolve(root: World) -> Composition: in that table's order, with a message naming the path and the `add_world` behind it. """ - order, children, paths, depths, parents = _walk(root) + order, children, paths, depths = _walk(root) edges, aliases = _edges(children, paths) bound, conflicts = _bound_startup(children, paths) - nodes = _nodes(order, paths, depths, parents, children, aliases, bound) + nodes = _nodes(order, paths, depths, children, aliases, bound) surface = _contribute(order[0], nodes, children) _raise_unknown_list_names(surface.unknown) @@ -286,7 +295,7 @@ def resolve(root: World) -> Composition: _raise_conflicts(conflicts) _raise_over_the_attach_limit(nodes) - tools = {name: candidate.entry for name, candidate in surface.candidates.items()} + tools = _entries(nodes[order[0]], surface.candidates) return Composition( root=nodes[order[0]], nodes=tuple(nodes[key] for key in order), @@ -302,12 +311,8 @@ def resolve(root: World) -> Composition: type _Children = Mapping[NodeKey, tuple[tuple[str, AddedWorld, NodeKey], ...]] -def _walk( - root: World, -) -> tuple[ - list[NodeKey], _Children, dict[NodeKey, str], dict[NodeKey, int], dict[NodeKey, NodeKey] -]: - """One breadth-first walk: the nodes in order, their edges, paths, depths and parents. +def _walk(root: World) -> tuple[list[NodeKey], _Children, dict[NodeKey, str], dict[NodeKey, int]]: + """One breadth-first walk: the nodes in order, their edges, paths and depths. A node is `(world object, scope)`. An edge with `store=None` passes the adder's scope through and an edge with `store="eu"` opens that scope for the @@ -331,9 +336,6 @@ def _walk( children: dict[NodeKey, tuple[tuple[str, AddedWorld, NodeKey], ...]] = {} paths = {root_key: ROOT_PATH} depths = {root_key: 0} - # The node each one was first reached from: its parent on the canonical - # route, and what the chain of a node is read off. - parents: dict[NodeKey, NodeKey] = {} pending: deque[NodeKey] = deque([root_key]) while pending: key = pending.popleft() @@ -347,11 +349,10 @@ def _walk( if child not in paths: paths[child] = _route(paths[key], added.name) depths[child] = depths[key] + 1 - parents[child] = key order.append(child) pending.append(child) children[key] = tuple(edges) - return order, children, paths, depths, parents + return order, children, paths, depths def canonical_tree(root: Node) -> Iterator[Node]: @@ -359,8 +360,7 @@ def canonical_tree(root: Node) -> Iterator[Node]: The tree, not the graph: an edge whose route is an alias is not descended, because the node it reaches is visited under the parent its path names. So a - node shared by two hosts is walked once, which is what "each node's startup - hooks run once, however many routes reach it" asks for. + node shared by two hosts is walked once. """ yield root for name, child in root.added.items(): @@ -368,6 +368,31 @@ def canonical_tree(root: Node) -> Iterator[Node]: yield from canonical_tree(child) +def hosts_first(composition: Composition) -> list[Node]: + """Every node once, each after every world that adds it: the order startup hooks run in. + + A depth-first walk from the root in `add_world` order that reaches a node + only from the last of its distinct hosts to be walked. With nothing shared, + every node has one host, and this is `canonical_tree`'s order exactly. + """ + waiting = dict.fromkeys(composition.nodes, 0) + for node in composition.nodes: + for child in dict.fromkeys(node.added.values()): + waiting[child] += 1 + order: list[Node] = [] + + def visit(node: Node) -> None: + order.append(node) + for child in dict.fromkeys(node.added.values()): + waiting[child] -= 1 + if waiting[child] == 0: + visit(child) + + visit(composition.root) + assert len(order) == len(composition.nodes), "a node no walk from the root reaches" + return order + + def _route(parent_path: str, name: str) -> str: """The path an edge spells: a child of the root is bare, everything deeper is qualified.""" return name if parent_path == ROOT_PATH else f"{parent_path}/{name}" @@ -431,7 +456,6 @@ def _nodes( order: Sequence[NodeKey], paths: Mapping[NodeKey, str], depths: Mapping[NodeKey, int], - parents: Mapping[NodeKey, NodeKey], children: _Children, aliases: Mapping[NodeKey, tuple[str, ...]], bound: Mapping[NodeKey, Mapping[str, Any]], @@ -450,8 +474,7 @@ def _nodes( file_name=_file_name(paths[key]), schema_name=_schema_name(paths[key]), bound_startup=MappingProxyType(dict(bound[key])), - agent_chain=build_route_chain(_route_layers(key, parents), key), - internal_chain=build_route_chain(_own_layers(key), key), + own_chain=build_route_chain(_route_layers((key,)), key), ) for key in order } @@ -508,34 +531,35 @@ def call_innermost(ctx: Ctx[Any], call: Call) -> Any: return call_innermost -def _route_layers( - key: NodeKey, parents: Mapping[NodeKey, NodeKey] -) -> list[tuple[NodeKey, Middleware]]: - """Every middleware on the canonical route to a node, outermost first. +def _route_layers(route: Sequence[NodeKey]) -> list[tuple[NodeKey, Middleware]]: + """Every middleware of every node on a route, outermost first. The whole route rather than the host's chain and the owner's: applied recursively through the nesting a composite is, that is what "the host's chain outermost, then the owning world's" says, and it is the reading under which an intermediate composite's error handler still shapes the errors of the tools it - contributes. + contributes. A route of one node is that node's own middlewares, which is all + a call from host code runs: the host's are already wrapped around the host tool + making the call. """ - route = [key] - while route[-1] in parents: - route.append(parents[route[-1]]) - return [ - (node_key, middleware) - for node_key in reversed(route) - for middleware in node_key[0].middlewares - ] + return [(key, middleware) for key in route for middleware in key[0].middlewares] -def _own_layers(key: NodeKey) -> list[tuple[NodeKey, Middleware]]: - """A node's own middlewares, for a call host code makes into it. +def _entries(root: Node, candidates: Mapping[str, _Candidate]) -> dict[str, Contributed]: + """The served tool list, each entry with the chain of the route that contributed it. - Only the owning world's chain runs: the host's is already wrapped around the - host tool making the call. + One chain per distinct route, and only for what reached the root. Seeded with + the root's own chain, so every tool of the root runs `root.own_chain`, which is + also `World.chain`. """ - return [(key, middleware) for middleware in key[0].middlewares] + chains: dict[tuple[NodeKey, ...], Handler] = {(root.key,): root.own_chain} + tools: dict[str, Contributed] = {} + for name, candidate in candidates.items(): + route = candidate.hosts + if route not in chains: + chains[route] = build_route_chain(_route_layers(route), route[-1]) + tools[name] = Contributed(name, candidate.tool, candidate.node, chains[route]) + return tools def _schema_name(path: str) -> str: @@ -566,8 +590,12 @@ class _UnknownListName: class _Candidate: """One contributed tool on its way up the tree, and where it was declared. - The route -- the `/`-joined `add_world` names from the node whose list this - is -- and not the owning node's path, is what a diagnostic has to name: two + `hosts` is the node keys on the route that contributed it, from the node whose + list this is down to the owner (`hosts[-1] == node.key`): what its chain is + built from once it reaches the root. `route` spells the same route as the + `/`-joined `add_world` names, for diagnostics. + + The route, and not the owning node's path, is what a diagnostic has to name: two views of one node share a path, and the author's fix is on one of the two `add_world` lines. `prefixes` is every `tool_prefix` applied on that route, outermost first, each paired with the route of the `add_world` that carries @@ -578,7 +606,10 @@ class _Candidate: being reported at once, because the pair may yet be filtered out above. """ - entry: Contributed + name: str + tool: Tool + node: Node + hosts: tuple[NodeKey, ...] route: str prefixes: tuple[tuple[str, str], ...] shadowed: str | None @@ -638,19 +669,19 @@ def contribution(key: NodeKey) -> dict[str, _Candidate]: candidates: dict[str, _Candidate] = {} for tool in node.world.tools.values(): if not tool.control: - _fold(candidates, _Candidate(Contributed(tool.name, tool, node), "", (), None)) + _fold(candidates, _Candidate(tool.name, tool, node, (key,), "", (), None)) for _name, added, child in children[key]: # A new candidate per edge, so the route and the prefixes below are # this edge's own even when the child's list came out of the memo. for inner in _filtered(added, contribution(child), unknown): - _fold(candidates, _through(added, inner)) + _fold(candidates, _through(key, added, inner)) memo[key] = candidates return memo[key] return _Surface(candidates=contribution(root_key), unknown=unknown) -def _through(added: AddedWorld, inner: _Candidate) -> _Candidate: +def _through(host: NodeKey, added: AddedWorld, inner: _Candidate) -> _Candidate: """One candidate as its host contributes it: renamed, and one route deeper.""" prefix = added.tool_prefix @@ -659,9 +690,10 @@ def outward(route: str) -> str: carried = tuple((applied, outward(where)) for applied, where in inner.prefixes) return _Candidate( - entry=Contributed( - name=_prefixed(prefix, inner.entry.name), tool=inner.entry.tool, node=inner.entry.node - ), + name=_prefixed(prefix, inner.name), + tool=inner.tool, + node=inner.node, + hosts=(host, *inner.hosts), route=outward(inner.route), prefixes=carried if prefix is None else ((prefix, added.name), *carried), shadowed=None if inner.shadowed is None else outward(inner.shadowed), @@ -670,14 +702,14 @@ def outward(route: str) -> str: def _fold(candidates: dict[str, _Candidate], candidate: _Candidate) -> None: """Add a candidate, or record it as the second declaration of a name already there.""" - standing = candidates.get(candidate.entry.name) + standing = candidates.get(candidate.name) if standing is None: - candidates[candidate.entry.name] = candidate + candidates[candidate.name] = candidate return if standing.shadowed is None: # The first repeat is the one reported, as the first of anything else the # seal finds is: a second is one more line about the same mistake. - candidates[candidate.entry.name] = replace(standing, shadowed=candidate.declared_at) + candidates[candidate.name] = replace(standing, shadowed=candidate.declared_at) def _filtered( diff --git a/src/seahaven/db.py b/src/seahaven/db.py index aa4a414..d136d18 100644 --- a/src/seahaven/db.py +++ b/src/seahaven/db.py @@ -263,8 +263,11 @@ def open_instance(path: Path, clock: Clock, seed: bytes) -> Db: """Open the writable connection an instance serves its tool calls from.""" conn = apsw.Connection(str(path)) _harden(conn) + # An instance directory does not survive a crash: the sweep deletes it. So + # its files need no durability, and set before the WAL switch, because at + # FULL the conversion of a copied fixture fsyncs on every creation. + conn.pragma("synchronous", "OFF") conn.pragma("journal_mode", "WAL") - conn.pragma("synchronous", "NORMAL") # One writer per instance by construction, so waiting on a lock can only # mean a bug. Fail loudly instead of hanging. Said rather than left to the # default: APSW opens a connection with no busy handler at all, so deleting @@ -382,6 +385,9 @@ def _build( try: register_random_functions(builder, seed, BUILD_STREAM) builder.pragma("foreign_keys", "ON") + # The file is a blank instance's own, deleted by the sweep after a + # crash like the rest of its directory, so it needs no fsync either. + builder.pragma("synchronous", "OFF") with builder: # Iterated, not just executed: APSW runs a multi-statement string # lazily, stopping at the first statement that returns a row, so a diff --git a/src/seahaven/docs/authoring.md b/src/seahaven/docs/authoring.md index c7e7145..5e7180c 100644 --- a/src/seahaven/docs/authoring.md +++ b/src/seahaven/docs/authoring.md @@ -206,25 +206,41 @@ JSON schema is what the tool list publishes. The model is **strict**: - every violation is reported at once, as a `seahaven.ArgumentError` carrying `violations`, so an agent can fix all of them in one turn. -Argument types are the JSON types, `Literal`, and nested pydantic models. Two kinds are refused at -registration, because strict validation could never satisfy them from a wire format: `datetime`, -`date` and `time`, and any `Enum` subclass, anywhere inside the annotation. Use `str` with a pattern -for a timestamp, and `Literal` for a closed set: +An argument may be almost any type pydantic gives a JSON schema: the JSON types, `Literal`, an +`Enum`, `datetime`, `UUID`, `Decimal`, `tuple`, `set`, a dataclass, a `TypedDict` or a nested +pydantic model. [What registration refuses](#what-registration-refuses) lists the exceptions. An +agent sends the JSON form that the schema publishes, such as a string for a `datetime` and an array +for a `tuple`, and the tool receives the Python object. The strict rules above apply at every depth, +except inside a type with a pydantic config of its own, described below. + +A call is validated in one of two modes. When every argument is plain JSON at every depth (`dict`, +`list`, `str`, `int`, `float`, `bool` and `None`), the call is validated as JSON, which is the form +the schema publishes. When any argument holds another Python object, the whole call is validated as +Python, and each argument must then be the annotated type's own object. A `datetime` passed for a +`str` is refused, and so is a list for a `tuple` beside a `UUID` object. + +A union whose members accept the same JSON string is ambiguous, and pydantic picks the member: +`datetime | str` receives a timestamp as a `datetime`, and `str | datetime` receives it as a `str`. + +A nested pydantic model validates under its own config, not the tool's. So does a pydantic +dataclass, and a type given `@with_config`. Set `strict=True` in it, or `strict=False` to accept lax +input on purpose. Registration refuses such a type when it sets neither. ```py +from datetime import datetime from typing import Annotated, Literal -from pydantic import Field +from pydantic import BaseModel, ConfigDict, Field Status = Literal["backlog", "todo", "done"] -Timestamp = Annotated[ - str, - Field( - pattern=r"^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z$", - description="A UTC timestamp with milliseconds, e.g. 2026-06-01T09:00:00.000Z.", - ), -] Limit = Annotated[int, Field(ge=1, le=250, description="How many rows to return, 1 to 250.")] + + +class Window(BaseModel): + model_config = ConfigDict(strict=True) + + start: datetime + end: datetime ``` Declare these once in a module of their own and import them everywhere, so that five tools that take @@ -308,7 +324,9 @@ when it: - has no first parameter, or one that is not positional and either annotated `seahaven.Ctx` or left unannotated; - has an argument with no type annotation, a positional-only argument, `*args` or `**kwargs`; -- has an argument annotated `datetime`, `date`, `time` or an `Enum` subclass, at any depth; +- has an argument that holds, at any depth, a nested pydantic model, pydantic dataclass or + `@with_config` type that does not set `strict`, or a `Literal` of a value JSON cannot send, such + as a plain `Enum` member; - has an argument whose annotation exists only under `TYPE_CHECKING`, in which case the message names the symbol; - has an argument with a mutable default (`[]`, `{}`, `set()`); diff --git a/src/seahaven/docs/composition.md b/src/seahaven/docs/composition.md index 1c2d241..b6763ee 100644 --- a/src/seahaven/docs/composition.md +++ b/src/seahaven/docs/composition.md @@ -87,9 +87,8 @@ with company.instance(now="2026-06-01T09:00:00.000Z") as inst: } ``` -That example is the whole feature. There is one new registration verb, one new handle on `ctx`, and -everything else — the tool list, the change log, the inspection connection, the fixture — gains a -node dimension it did not have before. +There is one new registration verb, one new handle on `ctx`, and everything else — the tool list, +the change log, the inspection connection, the fixture — gains a node dimension. ## Declaring @@ -375,12 +374,14 @@ for what no tool covers. Outside a `db.transaction()` block, statements there au ## Middleware -An agent call to a contributed tool descends every world's middleware along the canonical route from -the root to the owning node, outermost first: the host's, then any world in between, then the owning -world's, and then the tool. The host can wrap every tool it serves, with an access-control layer -gating the whole surface or a trace, and the added world's own error handler still shapes its errors -as its product would. The scaffolded handler passes `ToolError` through, so a host's outermost -handler does not re-wrap an added world's product errors. +An agent call to a contributed tool descends the middleware of every world on the route that +contributed that tool, from the root to the owning world, outermost first: the host's, then any +world in between, then the owning world's, and then the tool. When a shared node's tool reaches the +surface under two names by two routes, each name runs its own route's middleware. The host can wrap +every tool it serves, with an access-control layer gating the whole surface or a trace, and the +added world's own error handler still shapes its errors as its product would. The scaffolded handler +passes `ToolError` through, so a host's outermost handler does not re-wrap an added world's product +errors. **Each layer runs with its own world's context.** A host middleware sees the host's `db`, `state`, `ids` and `worlds`. The owning world's middleware and the tool see the owning node's. Without that @@ -399,11 +400,12 @@ behalf. ## Startup hooks Hooks run for every world in the tree, depth-first from the root: each node's own hooks, then its -added worlds in `add_world` order, each with its own node's context. Each node's hooks run **once**, -however many routes reach it. Every node's transaction is open before the first hook runs, which is -what lets a root hook write into a child's store — through `ctx.worlds..db` and -`ctx.worlds..state` — before that child's own hooks run. A hook that raises rolls all of them -back, and no instance is left behind. +added worlds in `add_world` order, each with its own node's context. A world that more than one +world adds runs after every world that adds it. Each node's hooks run **once**, however many routes +reach it. Every node's transaction is open before the first hook runs, which is what lets a host's +hook write into a child's store — through `ctx.worlds..db` and `ctx.worlds..state` — +before that child's own hooks run. A hook that raises rolls all of them back, and no instance is +left behind. The startup keywords a caller passes — `world.instance(startup={...})`, or the same `startup` on `reset()` — are **broadcast**: every hook in the tree that names a keyword receives it, and an @@ -467,10 +469,9 @@ keyword names. ## One instance, many stores An instance of a composite is one SQLite file and one connection per node, in one instance -directory. The root's file is `state.sqlite`, as it has always been, and an added node's is -`state..sqlite` with `/` replaced by `__`: `state.payments.sqlite`, -`state.payments__tax.sqlite`. Every file carries its own world's schema and gets every per-file rule -Seahaven has. +directory. The root's file is `state.sqlite`, and an added node's is `state..sqlite` with `/` +replaced by `__`: `state.payments.sqlite`, `state.payments__tax.sqlite`. Every file carries its own +world's schema and gets every per-file rule Seahaven has. - **One clock** for the whole instance, in the root world's clock mode unless the caller names one. Every connection gets the same overrides, so every tool in every added world reads the same @@ -479,21 +480,25 @@ Seahaven has. or removing a node never perturbs another node's ids, and the same fixture and seed reproduce every node. SQL's `random()` and `randomblob()` are seeded per node from the same salt, so a `DEFAULT (randomblob(8))` in one added world's schema replays and is not the stream any other - node, or any `ctx.ids`, draws from. The root's stream is the instance seed untouched, which is why - a world that adds nothing mints exactly what it always did. + node, or any `ctx.ids`, draws from. The root's stream is the instance seed untouched. - **`inst.db`, `inst.state_path` and the `ctx` a tool of the root receives are the root's.** An added node's store is reached through `ctx.worlds`, never from the instance. - **Isolation between nodes is structural.** A world's `ctx.db` is one file, so a world's own `run_sql` helper cannot see a sibling's tables. There is no allowlist to get wrong. `inst.bulk()` yields the root's context with a live `ctx.worlds`, and opens one transaction per node -and commits them in sequence, so a fixture generator fills every store in one block. +and commits them one at a time, the root last, so a fixture generator fills every store in one +block. + +If the block raises, every node rolls back. If a node's commit fails, for example on a deferred +foreign key, `bulk()` raises and the nodes that committed before it keep their rows. Do not freeze +an instance after `bulk()` raised; make a new instance and load it again. ## Fixtures A composite fixture is one directory holding one frozen SQLite file per node and one `fixture.yaml` -at `format_version: 2`. It carries the version-1 fields describing the root exactly as before, plus -a `nodes` list with one entry per added node. +at `format_version: 2`. It carries the version-1 fields, which describe the root, and a `nodes` list +with one entry per added node. ```yaml format_version: 2 @@ -518,13 +523,12 @@ nodes: - shop/payments ``` -A world that adds nothing keeps writing `format_version: 1` with no `nodes` key at all, so its -fixture directory is byte for byte what it was before composition existed, and every fixture already -on disk keeps loading. There is exactly one `now`, because no store has a clock of its own. A node's -`file` is a plain file name, held to the same rule as a world's name and a fixture id but with no -length limit, since Seahaven mints it from the node's path rather than accepting it. A sidecar that -spells anything else — a separator, a `..`, a drive letter, a character outside the name charset — -is refused when it is read, because both readers join it onto the fixture directory and follow the +A world that adds nothing writes `format_version: 1` with no `nodes` key, and Seahaven reads both +versions. There is exactly one `now`, because no store has a clock of its own. A node's `file` is a +plain file name, held to the same rule as a world's name and a fixture id but with no length limit, +since Seahaven mints it from the node's path rather than accepting it. A sidecar that spells +anything else — a separator, a `..`, a drive letter, a character outside the name charset — is +refused when it is read, because both readers join it onto the fixture directory and follow the result, and the sidecar supplies the hash too. - **Genesis** yields a blank file per node, each with its own schema, on one clock. Fill them @@ -586,10 +590,10 @@ with company.instance("acme") as inst: Nothing agent-facing says a world is composed. Everything eval-facing does. -- **`inst.inspect()`** is the read-only connection it always was, with every added node attached +- **`inst.inspect()`** is the eval's read-only connection, with every added node attached read-only under a schema named by its path, with `/` replaced by `__`: `payments.charges`, `payments__tax.rates`. "Was the invoice created and was the charge taken" is one statement. - Writes, `ATTACH` and `DETACH` are denied on it, as they always were. + Writes, `ATTACH` and `DETACH` are denied on it. - **`inst.change_log()`** is one list covering every node, in call order. Each record carries `world`, the owning node's path, which is `main` for the root. That is what tells two tables of the same name in two stores apart; the records of one call are sorted by that path, then the table, @@ -716,9 +720,8 @@ Eight codes, all of them for mistakes a composite makes silently. `inst.composition()`. - **An idle composite instance is one file and one connection per node.** A session is opened per node per call and closed with the call, so an idle instance holds none. Cost scales with the - tree, and the design target — hundreds of concurrent instances with minute-long lifetimes — - holds for a small number of nodes. The benchmark measures one node per instance and reads as a - per-node floor. + number of nodes: each node adds a file and a connection when an instance is made, and a session + on every call. - **Sealing costs a walk of the tree,** on the first use after any registration anywhere in the process. The invalidation is global, because a world cannot be told what added it. Steady state is one integer compare per call. Registration is expected to finish at import, so this is a cost you diff --git a/src/seahaven/docs/db_schema_and_fixtures.md b/src/seahaven/docs/db_schema_and_fixtures.md index 26b29af..42353b7 100644 --- a/src/seahaven/docs/db_schema_and_fixtures.md +++ b/src/seahaven/docs/db_schema_and_fixtures.md @@ -77,10 +77,9 @@ A schema file may also seed static reference rows — currencies, plans, a looku ships with — and they are part of the schema in the sense that matters: every instance of the world starts with them. The connection those `INSERT`s run on carries the instance's clock and its seed, so a seeded row built from `randomblob()` replays like anything else a world writes, and one built -from `CURRENT_TIMESTAMP` reads the instance's clock. The wall-clock rule below is unchanged and -still refuses `CURRENT_TIMESTAMP` inside a `CREATE` — a column default, a trigger body — so that one -timestamp format is written everywhere; what is sanctioned here is the `INSERT` the file runs -itself. Rows that belong to one *scenario* are a fixture's job, not the schema's. +from `CURRENT_TIMESTAMP` reads the instance's clock. The wall-clock rule below covers the objects a +schema file creates, such as a column default or a trigger body. It does not cover the `INSERT` +statements the file runs. Rows that belong to one *scenario* are a fixture's job, not the schema's. ### Three rules @@ -96,11 +95,11 @@ key cannot be identified in a change-log record, so writes to a table without on the eval grading the run. A join table takes a composite key: `PRIMARY KEY (issue_id, label_id)`. **No expression reads the wall clock.** No `CURRENT_TIMESTAMP` default, no `datetime('now')` in a -trigger, and nothing like them in a view, a generated column or a partial index. The problem is the -text as much as the instant: SQLite writes `2026-06-01 09:00:00`, and the rest of the world writes -`2026-06-01T09:00:00.000Z`. Two formats in one column make a comparison fail for a reason nobody -finds quickly. Declare the column `NOT NULL` with no default, and pass the value in from -`ctx.clock.iso()`. +trigger, and nothing like them in a view, a generated column or a partial index. The tool that +makes a row decides its time. A trigger that stamps `updated_at` overwrites the history a fixture +generator writes, and `CURRENT_DATE` or `julianday('now')` writes a date or a number where every +other timestamp is `2026-06-01T09:00:00.000Z`. Declare the column `NOT NULL` with no default, and +pass the value in from `ctx.clock.iso()`. FTS5 virtual tables and their shadow tables are exempt from the first two rules, because a virtual table cannot be `STRICT` and does not declare a primary key. [authoring.md](authoring.md) covers @@ -187,9 +186,9 @@ change it, and freeze the result under a new id. `@pytest.mark.seahaven(fixture="agency")`, `seahaven fixture fork agency ...`. The id is also the directory name, and it travels with the fixture, so it follows the same rule a world's name does: 1 to 128 characters of letters, digits, space, `.`, `-` and `_`, with no leading or trailing space or -dot, and no Windows device name. Anything else is refused with `not a fixture id`. A fixture frozen -under an id this rule now refuses still appears in `world.fixtures()` but no longer opens. Rename -its directory and its sidecar's `id` to reach it again. +dot, and no Windows device name. Anything else is refused with `not a fixture id`. A fixture +directory whose id breaks this rule appears in `world.fixtures()` and does not open. Rename its +directory and its sidecar's `id` to reach it again. ## Building a fixture @@ -323,8 +322,8 @@ loaded world fails with a message saying the fixture needs regenerating, and `se reports the same thing as `SH403` before you get that far. So: change the schema, delete every fixture directory, re-run the generator for each one in parent -order, and commit the new bytes. Nothing migrates a fixture that has already shipped. Version 1 of -Seahaven has no migration path at all, which is why the generator is committed. +order, and commit the new bytes. Nothing migrates a fixture, which is why the generator is +committed. An extension that brings its own `CREATE TABLE` text (see [extensions.md](extensions.md)) changes the schema hash too. That is a real cost of using one, and the extensions page says so. diff --git a/src/seahaven/docs/projecttracker.md b/src/seahaven/docs/projecttracker.md index 8c4efe6..3ab78c9 100644 --- a/src/seahaven/docs/projecttracker.md +++ b/src/seahaven/docs/projecttracker.md @@ -55,13 +55,14 @@ worlds/projecttracker/ ``` `__init__.py` is four lines and no cleverness: build the world, then import the modules whose import -registers things. That is the whole reason those imports exist, and `seahaven check` (`SH301`) fails -if a module under `tools/` or `middleware/` is missing from them. +registers things. The imports are there only to register, and `seahaven check` (`SH301`) fails if a +module under `tools/` or `middleware/` is missing from them. ## The schema Nine tables and one FTS5 index. Users belong to teams, teams own projects, projects hold issues, -issues carry labels and comments, and every write to an issue appends to an audit trail. +issues carry labels and comments, and an audit trail records who created each issue, changed its +status or assignee, or commented on it. | Table | Notes | |---|---| @@ -157,9 +158,10 @@ team `ENG` ever had, whichever of its projects it is in. The counter is bumped b Anything that writes issues without that statement, such as a bulk load in a fixture generator, has to leave the counter where the tools would have. -**A closed issue has no assignee.** Moving an issue to `done` or `canceled` drops its assignee and -records the drop, and assigning a closed issue is a `CONFLICT`. So `status IN ('done','canceled') -AND assignee_id IS NOT NULL` is a state this world cannot reach, and a grader may rely on it. +**Closing an issue drops its assignee.** Moving an issue to `done` or `canceled` drops its assignee +and records the drop, and `assign_issue` on a closed issue is a `CONFLICT`. `update_issue` checks +only the `status` it is given, so a call that sets `assignee_id` without `status` can assign a +closed issue. Do not grade on a closed issue never having an assignee. **An archived issue keeps its fields and leaves the lists.** `archive_issue` stamps `archived_at` and takes the issue out of `list_issues`. `get_issue`, `search_issues` and `run_sql` still see it, @@ -169,11 +171,12 @@ and the world's own notes avoid it. **The viewer.** `world.instance(fixture, startup={"user_id": ...})`, and the same `startup` on `reset` over a server, names who the session is driving the tracker as. `startup.py` puts it in -`ctx.state["viewer_id"]`, falling back to the workspace's first admin. Every write takes an optional -`actor_id` which wins, so one run can have two people writing without two instances. A write with -neither, in a workspace with no admin such as a blank instance or `empty`, is `INVALID_INPUT`. A -`user_id` naming nobody is a `WorldBug` rather than a product error, because it comes from the -eval's `reset` and not from the agent. +`ctx.state["viewer_id"]`, falling back to the workspace's first admin. The five issue writes the +audit trail records (`create_issue`, `update_issue`, `assign_issue`, `transition_issue` and +`add_comment`) take an optional `actor_id`, which wins, so one run can have two people writing +without two instances. One of them with neither, in a workspace with no admin such as a blank +instance or `empty`, is `INVALID_INPUT`. A `user_id` naming nobody is a `WorldBug` rather than a +product error, because it comes from the eval's `reset` and not from the agent. **Order within one run.** Rows one run writes can share a `created_at`: every row under the `fixed` clock, the rows of one call under `tick`, and rows written in the same millisecond under the @@ -194,8 +197,6 @@ All three are frozen at `2026-06-01T09:00:00.000Z`, and all three are built by | `small_startup` | one engineering team, three people, two projects, forty issues over two months, sixty comments, six labels | | `agency` | three teams, twelve people, nine projects including a finished one, six hundred issues over six months, fifteen hundred comments, thirty labels, and an audit trail with reassignments and transitions in it | -`agency` is also the fixture the framework's benchmark runs against. - To rebuild one, delete its directory and re-run the generator. Never edit one in place. ## Its tests @@ -205,9 +206,11 @@ real `instance.call`, the fixtures' invariants, the declared errors by code, the exceptions, pagination's cursor rules, and determinism from the same seed. Nothing builds an instance in a `conftest.py`, and nothing calls a tool function directly. +Run the tests and `seahaven check` from the root of the Seahaven repository: + ```sh uv run pytest worlds/projecttracker -uv run seahaven check +uv run seahaven check --world projecttracker:world ``` ## What to copy, and what not to diff --git a/src/seahaven/docs/reference/api.md b/src/seahaven/docs/reference/api.md index 33166d7..59bb444 100644 --- a/src/seahaven/docs/reference/api.md +++ b/src/seahaven/docs/reference/api.md @@ -26,22 +26,10 @@ internal and may change without notice. | [The concurrency gate](#the-concurrency-gate) | `set_concurrency` and friends | | [Middleware and hook types](#middleware-and-hook-types) | `Handler`, `Middleware` and `StartupHook` | -Ten names on this page are not exported from `seahaven/__init__.py`, and there are two reasons for -that: - -- Seven of them are public by the repository's own rule. `seahaven.world.Handler`, `Middleware` and - `StartupHook` are the type aliases the scaffolded error handler imports, and - `seahaven.fixtures.load`, `load_all`, `verify` and `freeze` read a fixture directory. Both sets - are part of their module's stated interface, in `components/world_and_dispatch.md` §1 and - `components/fixtures_instances.md` §1. `architecture.md` §1 makes that the rule: a name the - component document lists as part of a module's interface is public, and `seahaven/__init__.py` - re-exports only the subset worth a short import. -- The other three are `seahaven.instances.default_concurrency`, `concurrency` and `set_concurrency`, - the concurrency gate. No component document lists them, so **this page declares them public on its - own authority**. The gate is on in every process, and `serve --concurrency` is otherwise the only - documented way to change it, which leaves an in-process harness with a real control and no name - for it. That goes further than the specification, and this page says so rather than implying the - rule above covers it. +Some names on this page are not exported from `seahaven/__init__.py`. They are public all the same, +and are imported from their own module. They include `seahaven.world.Handler`, `Middleware` and +`StartupHook`; `seahaven.fixtures.load`, `load_all`, `verify` and `freeze`; and the concurrency +gate, `seahaven.instances.default_concurrency`, `concurrency` and `set_concurrency`. ```py import seahaven @@ -195,7 +183,7 @@ Made by `world.instance(...)`, never by hand. A context manager; leaving the blo | `inst.state(format=None)` | the state document as a plain dict: the envelope, and `state` from this instance's format. `format` answers in another of the world's formats instead. Refused inside `bulk()` or a tool call | | `inst.composition()` | what this instance is running against: one `NodeReport` per store, root first | | `inst.freeze(id, description)` | mint a fixture from the current state; returns the `Fixture`. Refuses inside `bulk()` | -| `inst.bulk()` | a context manager yielding the instance's own `Ctx`, in one transaction, for loading rows fast | +| `inst.bulk()` | a context manager yielding the root node's `Ctx`, with one transaction per node, for loading rows fast. See [composition.md](../composition.md#one-instance-many-stores) for a composite | | `inst.destroy()` | close everything and remove the working directory. Idempotent, and waits for a call in flight | | `inst.id`, `inst.fixture`, `inst.seed` | the instance id, the fixture id (or `None`), the derived seed bytes | | `inst.state_format` | the format `inst.state()` answers in, fixed for the instance's life | @@ -315,10 +303,12 @@ identity. Every connection overrides SQLite's `current_timestamp`, `current_date`, `current_time` and the `'now'` argument of `datetime`, `date`, `time`, `strftime`, `julianday`, `unixepoch` and `timediff` to return the clock's reading, so SQL sees the same time world code does. One SQL statement takes -one reading, and a trigger it fires shares that reading. Two rare cases are exceptions: see the -[clock modes risk report](https://github.com/Kiln-AI/Seahaven/blob/main/specs/projects/clock_modes/risk_report.md). -The overrides are registered as innocuous, so schema objects may reference them. What they return -compares and sorts correctly against the canonical text a world stores. +one reading, and a trigger it fires shares that reading. Two rare cases take another statement's +reading: a statement whose text starts with a `-- ` comment, and a statement whose rows are read +while another statement runs on the same connection. Neither happens under `fixed`, and under `tick` +only on `inst.inspect()` and the control tool. Under `running` and `wall`, either can happen on any +connection. The overrides are registered as innocuous, so schema objects may reference them. What +they return compares and sorts correctly against the canonical text a world stores. ## `Ids` @@ -373,6 +363,11 @@ class Tool: Seahaven's helpers or an extension's. `control` is Seahaven's own flag for its control tool and cannot be set through it. +`validate` is strict in one of two modes, chosen for the whole call. Arguments that are plain JSON +at every depth are validated as JSON, the form the schema publishes. Arguments that hold any other +Python object are validated as Python, and each must be the annotated type's own object. It returns +the Python objects. + ## `LogRecord` ```py @@ -590,7 +585,7 @@ def app( *, include_control_tools=False, max_concurrent_envs=500, - session_timeout=3600.0, + session_timeout=None, console=True, ) -> FastAPI: ... @@ -607,6 +602,8 @@ class SeahavenClient: # .reset(...), .call(tool, /, **arguments), .list_tools() `SeahavenClient.call(...)` answers a `SeahavenObservation`. On a failed call its `error` is OpenEnv's own `{error_type, message}` model and its `seahaven_error` is the world's `{code, message, details}` triple, read from `metadata["seahaven_error"]`. +`SeahavenClient.step(ListToolsAction())` answers a `ListToolsObservation`, whose `tools` are OpenEnv +`Tool` models. `SeahavenClient.state()` answers a `SeahavenState`: the state document, plus OpenEnv's `step_count`. Every envelope field of the document is a typed field on it, with `world` a diff --git a/src/seahaven/docs/reference/cli.md b/src/seahaven/docs/reference/cli.md index 771837b..c2040a4 100644 --- a/src/seahaven/docs/reference/cli.md +++ b/src/seahaven/docs/reference/cli.md @@ -70,7 +70,8 @@ seahaven new notes --dir ~/projects --hub The command refuses an existing directory rather than merging into one. It also refuses a name that does not make a Python package name, and a name `World(name=...)` would not accept — the name reaches the scaffolded `world.py` verbatim, so a world it cannot import is refused before the -directory is written. +directory is written. A name whose package has the same name as a standard library module or +`seahaven` is refused too, because Python imports that module instead of the world. The command finishes by printing what to do next. While Seahaven is unpublished, the `uv sync` in that list is a trap: the scaffold's `seahaven~=0.0` resolves to a placeholder release that contains @@ -173,17 +174,17 @@ Runs the OpenEnv server for this world: one world, many sessions, one worker pro | Option | Default | What it does | |---|---|---| -| `--host HOST` | `0.0.0.0` | the address to bind | +| `--host HOST` | `127.0.0.1` | the address to bind; `0.0.0.0` serves on every interface | | `--port PORT` | `8000` | the port to bind | | `--max_concurrent_envs N` | `500` | how many sessions may be open at once | | `--concurrency N` | `min(cpus, 16)` | how many tool calls run at once; `0` for no gate | -| `--session-timeout SECONDS` | `3600` | seconds of idleness before a session is reaped; `0` disables the reaper | +| `--session-timeout SECONDS` | off | seconds of idleness before a session is reaped; `0` turns the reaper off | | `--include-control-tools` | off | make each session's instance one that can call `controller_run_sql`; without the flag the name is an unknown tool. It is never listed either way, and it is deprecated in favour of the state document -- run Python with `-W default::DeprecationWarning` to see the warning | | `--no-console` | off | do not serve the web console at `/console`; the address is otherwise printed when the server starts | ```sh seahaven serve -seahaven serve --host 127.0.0.1 --port 9000 +seahaven serve --host 0.0.0.0 --port 9000 seahaven serve --include-control-tools --concurrency 0 ``` diff --git a/src/seahaven/docs/reference/lints.md b/src/seahaven/docs/reference/lints.md index b518f9d..738bc51 100644 --- a/src/seahaven/docs/reference/lints.md +++ b/src/seahaven/docs/reference/lints.md @@ -88,14 +88,20 @@ composite key: `PRIMARY KEY (issue_id, label_id)`. ## SH103 — a wall-clock expression anywhere in the schema **Rule.** No `CURRENT_TIMESTAMP`, `CURRENT_DATE`, `CURRENT_TIME`, `datetime('now')` or any relative -of them, in a column default, a trigger body, a view, a generated column or a partial index. The -whole schema is searched through `sqlite_master`, with comments and string literals excluded. - -**Why.** The problem is the text rather than the instant. An instance's clock overrides make a -default in the schema read the instance's clock anyway, but what SQLite *writes* is `2026-06-01 -09:00:00`, and every other door of a world writes `2026-06-01T09:00:00.000Z`. One format across -every door is the rule, and two formats in one column is a comparison that fails for a reason nobody -will find quickly. +of them, in a column default, a trigger body, a view, a generated column, a `CHECK` constraint, an +index expression or a partial index. The whole schema is searched through `sqlite_master`, with +comments and string literals excluded. + +**Why.** On Seahaven's own connections, `CURRENT_TIMESTAMP` and `datetime('now')` read the +instance's clock and write canonical text. The rest of the family does not: `CURRENT_DATE`, +`CURRENT_TIME`, `date('now')`, `julianday('now')` and `unixepoch('now')` write a date, a time or a +number. A trigger that stamps a time also overwrites the timestamps a fixture generator writes in +`inst.bulk()`. A schema object is part of the database file, so a connection Seahaven did not open, +such as a grader reading `state.sqlite` with Python's `sqlite3`, evaluates it with SQLite's own +functions. Those read the host's wall clock and write `2026-06-01 09:00:00`. In a generated column +or an index, the time is read when the row is written, and the stored value does not move with the +clock. A `CHECK` is tested only when the row is written. A timestamp that world code writes from +`ctx.clock.iso()` has none of these problems. **Fix.** `write the timestamp from world code with ctx.clock.iso(), so every door of the world uses one format`. Make the column `NOT NULL` with no default and pass the value in. @@ -196,7 +202,8 @@ and move on. **Why.** Two routes to one shared store each carry their own prefix and lists, which is two declarations rather than a collision. The result is an agent that sees `stripe_create_charge` and `billing_create_charge`, both writing to one account, with nothing in either listing saying so. -Sometimes that is the client's real surface, which is why it is a warning. +Each name also runs the middleware of its own route. Sometimes that is the client's real surface, +which is why it is a warning. **Fix.** `give every route but one tool_allow_list=[] or a tool_block_list naming it, or accept that the agent sees one account twice`. @@ -354,8 +361,9 @@ all find a world this way. A package that does not follow it works in process an the attribute it looked for, and `--world module:attr` is the override for a layout the convention misses. -An import that fails for another reason is reported here too, with the exception's own last line. -`check` never answers with a traceback. +An import that fails for another reason is reported here too. The finding gives the exception's +type and message at the file and line in the world's code that raised it, and the fix names that +place. `python -c "import "` prints the whole traceback; `check` never prints one. ## SH502 — a `Worlds` subclass annotates a name no `add_world` registered diff --git a/src/seahaven/docs/serving_and_openenv.md b/src/seahaven/docs/serving_and_openenv.md index 7f8e973..e0113af 100644 --- a/src/seahaven/docs/serving_and_openenv.md +++ b/src/seahaven/docs/serving_and_openenv.md @@ -71,16 +71,16 @@ the extra. A world used in process — in pytest, in a script, in a notebook — ```sh seahaven serve -seahaven serve --host 127.0.0.1 --port 9000 +seahaven serve --host 0.0.0.0 --port 9000 ``` | Option | Default | What it does | |---|---|---| -| `--host` | `0.0.0.0` | the address to bind. A container serves on the network it was given; pass `--host 127.0.0.1` for loopback | +| `--host` | `127.0.0.1` | the address to bind. Pass `--host 0.0.0.0` to serve on every interface, as a container does | | `--port` | `8000` | the port to bind | | `--max_concurrent_envs` | `500` | how many sessions may be open at once. Over capacity, OpenEnv answers `CAPACITY_REACHED` and closes the connection | | `--concurrency` | `min(cpus, 16)` | how many tool calls run at once; `0` for no gate | -| `--session-timeout` | `3600` | seconds of idleness before a session is reaped; `0` disables the reaper | +| `--session-timeout` | off | seconds of idleness before a session is reaped; `0` turns the reaper off. See [the idle reaper](#running-it-in-production) | | `--include-control-tools` | off | make each session's instance one that can call the deprecated control tool; without the flag the name is an unknown tool. [reference/cli.md](reference/cli.md) names it | | `--no-console` | off | do not serve the web console at `/console` | | `--world module:attr` | the convention | which world to serve | @@ -88,12 +88,6 @@ seahaven serve --host 127.0.0.1 --port 9000 `--max_concurrent_envs` is spelled with underscores because that is OpenEnv's own option name, and a second spelling here would be one more thing to translate. -If the command fails on a release candidate of CPython 3.14, that is expected and there are two -separate breakages under it. On 3.14.0rc2 Seahaven does not import at all, because pydantic cannot -evaluate its forward references there. The extra would also meet a second one on its own: 3.14.0rc2 -has no `collections.abc.ByteString`, and `beartype` asks for that name unguarded. Both are gone on -3.14.0 final, where the extra installs and works unpatched. - One process serves one world, and `serve` always runs a single worker. A session's instance, connections and working directory are in-process state, so a second worker would answer a session's second frame with an environment that has never seen its first. @@ -112,8 +106,6 @@ Open that in a browser and you can drive the world by hand: open instances, call the state document, without writing a client. `seahaven serve --no-console` leaves the console out, for a server that should answer the protocol and nothing else. -https://github.com/user-attachments/assets/33759ce9-8e01-4f8c-8d79-f04883848027 - ## Sessions and instances A WebSocket connection is one session, and one session holds one instance. @@ -127,8 +119,7 @@ A WebSocket connection is one session, and one session holds one instance. - **A second `reset`** destroys the current instance before making the new one, so a session never holds two. If creation then fails, the session is left exactly as a fresh one — no instance, no episode, no steps — and is open for another `reset`. -- **Closing the connection** destroys the instance. A dropped client costs nothing once it is - reaped. +- **Closing the connection**, or losing it, destroys the instance. `reset` takes what `world.instance(...)` takes, because it *is* `world.instance(...)`: @@ -494,6 +485,8 @@ observation too, and raises only on a framework or protocol failure. The gate bounds how many tool calls run at once. It never bounds admission: calls queue, and nothing is rejected. A call takes the gate before the instance lock, so a queued call cannot block a `destroy` or a `freeze`. Instance creation, tool listing and the control tool bypass it entirely. +`inst.bulk()` bypasses it too, and so does any call made inside a call or a `bulk()` block on the +same thread. Its default follows the process's CPU affinity, so it respects a container's limit rather than the host's core count. @@ -501,27 +494,16 @@ host's core count. **The gate is unfair whenever it binds, and the default is not exempt.** It is a `threading.BoundedSemaphore`, and a semaphore is not a queue. A thread that releases a slot and immediately asks for another usually wins the race against the waiter that was just woken, because -the waiter needs the GIL to make progress and the barging thread already has it. In the framework's -own benchmark, with five threads calling and the gate at 1, 2 or 4, one three-second window served -its worst-served session **once** while another session in that same window was served thousands of -times: 12,874 at a gate of 1, and 5,351 and 4,011 at 2 and 4. At a gate size above the number of -threads offered, so that the gate never binds, every session got an even share. Every gate size that -binds behaves this way, and a server with 500 sessions and a gate of 16 is the ordinary case rather -than an edge one. +the waiter needs the GIL to make progress and the barging thread already has it. Under load, one +session can be served once while another is served thousands of times. Every gate size that binds +behaves this way. Nothing is dropped, so the promise that calls queue is kept to the letter. But a call that queues for seconds behind a thread barging in front of it is not the service that promise implies, and a -run whose session is the unlucky one will time out. The fix is a gate that hands slots out in -arrival order, not a different number. - -What to do meanwhile, for a workload that is saturated and cares about the slowest session: -`--concurrency 0` was the one setting measured that served every session evenly, and at 32 sessions -it matched or beat the default on throughput while cutting the worst observed wait by an order of -magnitude. It pays for that in median and 95th-percentile latency. The measurements, with the -caveats they need — one machine, one afternoon, a closed loop with no think time — are in -`tools/bench/results/latest.md` in the Seahaven repository. They are not a service-level objective, -they are not a capacity model, and no number from them should be quoted as a property of the -framework. +run whose session is the unlucky one will time out. + +If a saturated workload cares about its slowest session, run with `--concurrency 0`. Without the +gate every session gets an even share, and median and 95th-percentile latency go up. **The gate is not a serving feature.** It is process-wide and on by default in *any* process that calls a tool, including an in-process eval harness driving instances on threads. `serve` only gives @@ -539,15 +521,16 @@ affinity, which is the operator's business. One world is not one *package*, thou adds other worlds serves their tools as part of its own surface, so a composite world is still one environment on the wire ([composition.md](composition.md)). -**The idle reaper matters.** A held session costs its fixture copy on disk and about a megabyte of -memory, and a client that drops without closing holds one for ever. An hour is long enough that no -live eval is reaped and short enough that a crashed harness does not accumulate instances. +**The idle reaper is off by default.** A session holds its fixture copy on disk and about a megabyte +of memory until its connection ends. A dropped connection ends the session too, so a client that +crashes leaves no instance behind. `--session-timeout N` turns the reaper on: it destroys the +instance of a session that has been idle for `N` seconds. The reaper counts only `reset` and `step` +as activity, so it can reap a client that is still connected, for example one that only reads +`state`. **A disconnect is not an error in the log.** A session that ends normally leaves nothing on `uvicorn.error`, whatever client ended it: `SeahavenClient`, a stock `GenericEnvClient`, a raw -socket, or a harness that simply dies. Seahaven carried a client-side close handshake and an ASGI -middleware to get that under openenv 0.4.2, and carries neither now, because openenv 0.5 logs no -error for a normal close. +socket, or a harness that simply dies. **A dropped connection is a lost episode, so `SeahavenClient` waits longer before calling one dead.** A session is one connection holding one instance, and there is no resume: any disconnect @@ -592,13 +575,10 @@ is empty. The one-line description is unaffected, since that is `World(descripti with the world rather than with the directory. Put the world's `README.md` beside the directory you named, or leave `fixtures_dir` alone and ship `fixtures/` where it was. -Seahaven's own reference world is not published anywhere. That step is gated on a maintainer's -sign-off and has not happened. - ## Evaluating with Kiln -[Kiln](https://kiln.tech) connects to a Seahaven world as an OpenEnv client, which is the whole -integration: point it at a served world and it drives the sessions. +[Kiln](https://kiln.tech) connects to a Seahaven world as an OpenEnv client. Point Kiln at a served +world and it drives the sessions. Kiln is where the goal that Seahaven deliberately does not hold gets written down. Define a scenario against a world and a fixture, run it as an [eval](https://kiln.tech/features/evals) and grade the diff --git a/src/seahaven/docs/state.md b/src/seahaven/docs/state.md index 38303fc..b971a4b 100644 --- a/src/seahaven/docs/state.md +++ b/src/seahaven/docs/state.md @@ -561,22 +561,5 @@ needs from them, and the framework does not guess which. That is also why the document costs no database work to produce, however long the episode ran. Recording is not free at the other end: a session is opened on each node for each call, and what it -recorded is rendered when the call commits. The Seahaven repository carries a probe that measures -what that costs, under `tools/bench/`. Run the probe with -`uv run python -m tools.bench recording --calls 1000 --repeats 9`. - -The figures below are approximate. They come from a shared virtual machine, and the probe prints -the machine it ran on. Read the figures as the size of the cost, not as the cost. - -A write-heavy call on ProjectTracker's `agency` fixture costs about 47% more than the single -long-lived session per node that Seahaven kept before this release. Reading the changeset and -rendering the records is about 33 of those 47 percentage points, and that work is what an eval's -own read of the changeset used to do once an episode. A read-only call records nothing, so it adds -less than a write call does. A read-only call is also the harder figure to pin down: repeats of the -same command put it between about 5% and 18% above the same long-lived shape, on a call of -under 0.1 ms. - -A composite instance opens a session on every node of the tree for every call. The four-node -`emporium` world used in the framework's own tests came out at 45% to 67% on its write calls, -which overlaps the one-node figure above, so the share does not grow in proportion to the node -count even though the work does. +recorded is rendered when the call commits. A read-only call records nothing, so it costs less than +a write. diff --git a/src/seahaven/handles.py b/src/seahaven/handles.py index b246e88..0211936 100644 --- a/src/seahaven/handles.py +++ b/src/seahaven/handles.py @@ -199,7 +199,7 @@ def call(self, tool: str | Callable[..., Any], /, *args: Any, **arguments: Any) raise WorldBug(_STALE) given = arguments_of(registered, tool, args, arguments) call = Call(name, given, registered, node=owner.path) - result = owner.internal_chain(frame.ctx(owner.key, call), call) + result = owner.own_chain(frame.ctx(owner.key, call), call) except BaseException as error: instance._log_failure(name, started, error, owner.path, internal=True) raise diff --git a/src/seahaven/instances.py b/src/seahaven/instances.py index 325bbab..767ea62 100644 --- a/src/seahaven/instances.py +++ b/src/seahaven/instances.py @@ -26,12 +26,15 @@ already held and then asks the instance for something -- its control handle -- that takes it again on the same thread. -*The gate before the lock.* The concurrency gate bounds how many tool calls run -at once across the process. It is taken before the instance lock, so a call -queued behind it holds nothing and can never delay a `destroy` or a `freeze`. A -call host code makes into an added world does not take it at all: the outermost -call is already holding it, and one instance runs one call at a time however many -nodes that call touches. +*The gate before the lock, and never under it.* The concurrency gate bounds how +many tool calls run at once across the process. A call takes it before the +instance lock, so a call queued behind it holds nothing and can never delay a +`destroy` or a `freeze`. The gate is re-entrant per thread: once a thread is +inside a call or a `bulk()` block, nothing more it does takes the gate. That +covers a nested call through a handle, an `inst.call(...)` inside `bulk()`, and a +call on another instance from inside either, because by then the thread may hold +an instance lock that a queued caller is waiting for. `bulk()`, instance creation +and the control tool take no slot. *The manager's lock is never held while an instance lock is.* The registry is touched only in short moments that take nothing else. @@ -55,7 +58,7 @@ from pathlib import Path from typing import TYPE_CHECKING, Any, Concatenate, Self, overload -from seahaven.call import Call, arguments_of, name_of, serialise +from seahaven.call import Call, Handler, arguments_of, name_of, serialise from seahaven.changes import ( CallRecord, LogRecord, @@ -72,7 +75,7 @@ Node, NodeKey, NodeReport, - canonical_tree, + hosts_first, ) from seahaven.ctx import Ctx, InstanceInfo from seahaven.db import Db, build_blank, open_inspection, open_instance @@ -210,24 +213,36 @@ def concurrency() -> int: return current.size if current is not None else 0 +# Whether this thread is inside a `gate()` block, whether or not that block took +# a slot. Per thread, not a ContextVar: Starlette's `run_in_threadpool` and +# `asyncio.to_thread` copy context variables into the worker thread, and that +# worker holds none of the locks the flag stands for. +_gated = threading.local() + + @contextmanager def gate(*, bypass: bool = False) -> Iterator[None]: """Hold one of the gate's slots for the block, queueing for it if need be. Nothing is ever rejected: the gate bounds how many calls execute at once, not - how many are admitted. + how many are admitted. Re-entrant per thread: a block opened inside another + takes nothing, because by then the thread may hold an instance lock. """ - # Read once: a `set_concurrency` between the acquire and the release must not - # let this call release a slot on a semaphore it never took. - semaphore = _gate - if bypass or semaphore is None: + if getattr(_gated, "active", False): yield return - semaphore.acquire() + # Read once: a `set_concurrency` between the acquire and the release must not + # let this call release a slot on a semaphore it never took. + semaphore = None if bypass else _gate + if semaphore is not None: + semaphore.acquire() + _gated.active = True try: yield finally: - semaphore.release() + _gated.active = False + if semaphore is not None: + semaphore.release() @dataclass @@ -261,11 +276,12 @@ class NodeRuntime: @dataclass(frozen=True) class _Target: - """What a call resolved to: the name as it was asked for, and who owns it.""" + """What a call resolved to: the name as it was asked for, who owns it, and its chain.""" name: str tool: Tool node: Node + chain: Handler class Instance: @@ -595,9 +611,11 @@ def bulk(self) -> AbstractContextManager[Ctx[Any]]: validation and transaction boundaries for tens of thousands of rows. What is yielded is the root node's context, with no call attached and a live `ctx.worlds`, so an added world's store is reached through - `ctx.worlds..db`; nothing is disabled and nothing is wrapped. Every - node's transaction is committed on the way out, and all of them are rolled - back together if the block raised. Startup hooks do not run again. + `ctx.worlds..db`; nothing is disabled and nothing is wrapped. If the + block raises, every node's transaction is rolled back. Otherwise they + commit one at a time, the root last, and a commit that fails leaves the + nodes that committed before it with their rows. Startup hooks do not run + again. """ return self._bulk() @@ -632,11 +650,11 @@ def _target(self, tool: str | Callable[..., Any]) -> _Target: # A function names the entry, and the entry names the node: the two # ways in meet here, and everything below this line is one path. entry = composition.entry_for(tool) - return _Target(entry.name, entry.tool, entry.node) + return _Target(entry.name, entry.tool, entry.node, entry.chain) name = tool entry = composition.tools.get(name) if entry is not None: - return _Target(name, entry.tool, entry.node) + return _Target(name, entry.tool, entry.node, entry.chain) # Control tools are never contributed -- they are the framework's, not a # world's surface -- so the composite list cannot hold them and the root's # own registry is where they are. Every other name the root registers is @@ -649,7 +667,8 @@ def _target(self, tool: str | Callable[..., Any]) -> _Target: # words: whether the name exists at all is not something a caller # gets to learn by calling it. raise UnknownTool(name) - return _Target(name, registered, composition.root) + # `control.dispatch` answers a control tool before any chain would run. + return _Target(name, registered, composition.root, composition.root.own_chain) def _dispatch(self, target: _Target, arguments: Mapping[str, Any]) -> Any: tool = target.tool @@ -668,15 +687,16 @@ def _dispatch(self, target: _Target, arguments: Mapping[str, Any]) -> Any: # and a middleware short-circuit included: the harness's Nth call is # this instance's Nth (functional_spec.md §7). i = self._next_ordinal() - # The chain is read from the node here rather than held, so a - # middleware registered after this instance was made applies to it. + # The chain comes from this call's resolution, which reseals after a + # registration, so a middleware registered after this instance was + # made applies to it. with ( self._recording(i), self._logging_call(target.name, arguments), self._logging_escape(target.name, target.node.path), in_call(), ): - return target.node.agent_chain(ctx, call) + return target.chain(ctx, call) def _next_ordinal(self) -> int: """The ordinal of the call about to run, counting from 0. The lock is held. @@ -917,10 +937,18 @@ def _bulk(self) -> Iterator[Ctx[Any]]: # `_recording(None)`: authoring writes happen after creation and count, # but there is no call in flight for them to belong to. It is outside the # transactions, so it reads each changeset after they have all settled. - with self._held() as frame, self._recording(None), ExitStack() as stack: - # Every node's transaction open before the block runs and committed in - # sequence on the way out, so a bulk write that reaches two stores - # through `ctx.worlds` either lands in both or in neither. + # `gate(bypass=True)` marks the thread and takes no slot: HTTP requests run + # in here, and the gate does not apply to them (`docs/http_apis.md`). + with ( + gate(bypass=True), + self._held() as frame, + self._recording(None), + ExitStack() as stack, + ): + # One transaction per node, because each node is its own file and SQLite + # cannot commit two files atomically. The stack commits them in reverse, + # the root last: a block that raises rolls every node back, but a commit + # that fails leaves the nodes before it committed. for runtime in self._runtime.values(): stack.enter_context(runtime.db.transaction()) yield frame.ctx(self._root_key, None) @@ -1564,14 +1592,15 @@ def _run_startup_hooks(composition: Composition, frame: Frame, keywords: Mapping is already open. A hook that raises rolls every one of them back, and creation removes the instance entirely. - Depth-first preorder over the canonical tree, so a node whose hooks seed a - child runs before it, and a node reached by two routes runs once. + Hosts first (`hosts_first`), so a world whose hooks seed a world it adds + runs before it -- every host of a shared node included -- and a node + reached by two routes runs once. """ runtimes = frame.instance._runtime with ExitStack() as stack: for node in composition.nodes: stack.enter_context(runtimes[node.key].db.transaction()) - for node in canonical_tree(composition.root): + for node in hosts_first(composition): ctx = frame.ctx(node.key, None) for hook in node.world.startup_hooks: hook(ctx, **_hook_arguments(hook, node, keywords)) diff --git a/src/seahaven/lint/ddl.py b/src/seahaven/lint/ddl.py index acda922..0daabf0 100644 --- a/src/seahaven/lint/ddl.py +++ b/src/seahaven/lint/ddl.py @@ -173,7 +173,7 @@ def _wall_clock_findings( `sqlite_master` rather than the files, so a wall clock reaches the finding wherever it is spelled: a column default, a trigger body, a view, a generated - column or a partial index. + column, a `CHECK` constraint, an index expression or a partial index. """ findings: list[Finding] = [] for name, table, sql in _schema_objects(conn): diff --git a/src/seahaven/openenv/__init__.py b/src/seahaven/openenv/__init__.py index 4d90f04..681650a 100644 --- a/src/seahaven/openenv/__init__.py +++ b/src/seahaven/openenv/__init__.py @@ -71,11 +71,14 @@ # connection, and `seahaven serve --max_concurrent_envs` moves the number. DEFAULT_MAX_CONCURRENT_ENVS = 500 -# A held session costs its fixture copy on disk and about a megabyte of memory, -# and a client that drops without closing holds one for ever. An hour idle is -# long enough that no live eval is reaped and short enough that a crashed -# harness does not accumulate; `--session-timeout 0` turns the reaper off. -DEFAULT_SESSION_TIMEOUT = 3600.0 +# No reaper unless the operator asks for one with `--session-timeout N`. A +# dropped client already leaves nothing behind: OpenEnv's `/ws` handler destroys +# the session in its `finally` when the socket goes, uvicorn's ping closes a +# half-open peer, and Seahaven refuses `openenv/session/create`, the only way to +# hold a session without a socket. So every session the reaper could find has a +# client still connected, and OpenEnv counts only `reset` and `step` frames as +# activity -- a harness that is only reading `state` would lose its instance. +DEFAULT_SESSION_TIMEOUT: float | None = None # OpenEnv's three plain-HTTP episode-control routes. Every one of their handlers diff --git a/src/seahaven/openenv/client.py b/src/seahaven/openenv/client.py index f1b2925..7b27147 100644 --- a/src/seahaven/openenv/client.py +++ b/src/seahaven/openenv/client.py @@ -18,7 +18,11 @@ from openenv.core.client_types import StepResult from openenv.core.env_client import EnvClient -from openenv.core.env_server.mcp_types import CallToolAction, ListToolsAction +from openenv.core.env_server.mcp_types import ( + CallToolAction, + ListToolsAction, + ListToolsObservation, +) from openenv.core.env_server.types import Observation from seahaven.openenv.env import SeahavenObservation, SeahavenState @@ -117,27 +121,32 @@ def _step_payload(self, action: CallToolAction | ListToolsAction) -> dict[str, A return action.model_dump() def _parse_result(self, payload: dict[str, Any]) -> StepResult[Observation]: - """A reply frame as a `StepResult` carrying a `SeahavenObservation`. + """A reply frame as a `StepResult` carrying a `SeahavenObservation`, or a tool list. The base client parses the reply to a `step` and the reply to a `reset` - through this one hook, so both arrive as a `SeahavenObservation`. A tool - call fills `tool_name` and one of `result` and `error`, and an error also - fills `metadata["seahaven_error"]`, which `obs.seahaven_error` reads. A - reset fills only the inherited `metadata`, because the server answers a - reset with a plain `Observation` and this model's own three fields all - have defaults. + through this one hook, so both arrive as a `SeahavenObservation`, unless + the step was a `ListToolsAction`. A tool call fills `tool_name` and one + of `result` and `error`, and an error also fills + `metadata["seahaven_error"]`, which `obs.seahaven_error` reads. A reset + fills only the inherited `metadata`, because the server answers a reset + with a plain `Observation` and this model's own three fields all have + defaults. Reading `result` off a reset is therefore `None` at runtime rather than an error, which is why this client is parameterised on `Observation` instead: `reset(...).observation.result` does not type-check, and `metadata` is how a reset is read. - A `ListToolsAction` answers a `ListToolsObservation`, which has a - `tools` list and no `result`; it does not come through here, and - `list_tools` below says why. + `step(ListToolsAction())` answers a `ListToolsObservation`, whose `tools` + are OpenEnv's `Tool` models. A caller narrows the observation with + `isinstance`. """ + observation = payload.get("observation", {}) + # A tool list is the one reply with a `tools` field; a reset carries its + # tool count in `metadata`, never at the top level. + model = ListToolsObservation if "tools" in observation else SeahavenObservation return StepResult( - observation=SeahavenObservation.model_validate(payload.get("observation", {})), + observation=model.model_validate(observation), reward=payload.get("reward"), done=payload.get("done", False), metadata=payload.get("metadata"), @@ -167,10 +176,8 @@ def list_tools(self) -> Any: Each entry is `{"name", "description", "input_schema"}`. Control tools are never listed, whatever the server was started with. - This is the one verb that reads the frame itself instead of parsing it - into `SeahavenObservation`: that model forbids extra fields and has no - `tools`, because it is the shape of a tool *call*. The list is read - straight off the step payload as `list[dict]`. + The list is read straight off the step payload as `list[dict]`; + `step(ListToolsAction())` answers the same list as `Tool` models. """ return self._dispatch(self._list_tools_async) diff --git a/src/seahaven/openenv/serve.py b/src/seahaven/openenv/serve.py index e8c73af..52d661f 100644 --- a/src/seahaven/openenv/serve.py +++ b/src/seahaven/openenv/serve.py @@ -26,9 +26,10 @@ __all__ = ["DEFAULT_HOST", "DEFAULT_PORT", "console_url", "serve"] -# A container serves on the network it was given, not on loopback; an -# operator who wants loopback says so with `--host 127.0.0.1`. -DEFAULT_HOST = "0.0.0.0" +# `/ws` has no authentication, so a server is reachable from the network only +# when the operator says so with `--host 0.0.0.0`. The scaffolded container +# passes that itself. +DEFAULT_HOST = "127.0.0.1" DEFAULT_PORT = 8000 LOG_LEVEL = "info" diff --git a/src/seahaven/pytest_plugin.py b/src/seahaven/pytest_plugin.py index 31e18d7..c1fe752 100644 --- a/src/seahaven/pytest_plugin.py +++ b/src/seahaven/pytest_plugin.py @@ -93,9 +93,10 @@ def world(request: pytest.FixtureRequest) -> World: try: return cli.find_world(request.config.getoption(WORLD_OPTION), start=request.config.rootpath) except cli.CliError as error: - # Every `CliError` is one line with the fix in it. Raising it would print - # that line under a traceback through `import_module`, which says only - # that Python was involved. + # Every `CliError` is one line with the fix in it; an import that raised + # names the file and line of the world's code that raised it. Raising it + # would print that line under a traceback through `import_module`, which + # says only that Python was involved. pytest.fail(str(error), pytrace=False) diff --git a/src/seahaven/tool.py b/src/seahaven/tool.py index 2b82cb7..541e1d5 100644 --- a/src/seahaven/tool.py +++ b/src/seahaven/tool.py @@ -11,14 +11,22 @@ `world.tool(...)`. """ +import dataclasses import inspect from collections.abc import Callable, Iterator, Mapping, Sequence from dataclasses import dataclass -from datetime import date, time -from enum import Enum -from typing import Any, Concatenate, get_args, get_origin +from typing import ( + Any, + Concatenate, + Literal, + get_args, + get_origin, + get_type_hints, + is_typeddict, +) import pydantic +import pydantic_core from pydantic import ConfigDict, create_model from seahaven.ctx import Ctx @@ -29,17 +37,17 @@ # Strict, so the tool list is the contract: `"5"` is not an int and `1` is not a # bool. A world mimicking a product that coerces relaxes one argument with # `Annotated[int, Field(strict=False)]`, which works because strictness lives in -# the config rather than in the call to `model_validate`. +# the config: a `strict=` passed to `model_validate` or `model_validate_json` +# would override every field's own setting. _ARGUMENT_CONFIG = ConfigDict(extra="forbid", strict=True) _POSITIONAL = (inspect.Parameter.POSITIONAL_ONLY, inspect.Parameter.POSITIONAL_OR_KEYWORD) _VARIADIC = (inspect.Parameter.VAR_POSITIONAL, inspect.Parameter.VAR_KEYWORD) -# Types no argument may be annotated with, anywhere in its annotation. Under -# strict validation each is unsatisfiable from JSON: pydantic would demand a real -# `datetime` or a real enum member, which no wire format carries. `datetime` is a -# `date`, and `Enum` covers `StrEnum` and `IntEnum` too. -_UNREACHABLE_UNDER_STRICT = (date, time, Enum) +# The scalars JSON carries. A `Literal` value must be one for JSON to send it: a +# `StrEnum` or `IntEnum` member is, and equal to its own value, so pydantic +# accepts that value. +_JSON_SCALARS = (str, int, float, bool, type(None)) # Defaults Python itself warns about: a shared mutable bound to the function. # Refused here for a second reason, that the schema publishes `default` and a @@ -117,23 +125,27 @@ def from_function[**Q, S]( def validate(self, arguments: Mapping[str, Any]) -> dict[str, Any]: """The arguments as the function's parameters, or `ArgumentError`. + Strict either way, in one of two modes chosen for the whole call. A call + made of plain JSON values at every depth -- which is all the wire can hand + over -- is validated in pydantic's JSON mode, which accepts the JSON form + of every type the published schema describes: an array for a `tuple`, a + string for a `UUID` or a `datetime`. Strict *Python* mode would demand the + real `tuple` or `UUID`, which parsed JSON never holds. A call holding any + other Python object is validated in strict Python mode, so a `datetime` + handed to a `str` parameter is refused rather than rendered to text. + Every violation is reported, not just the first: an agent that has to make one round trip per mistake makes several. """ + given = dict(arguments) + text = _json_text(given) try: - model = self.params.model_validate(dict(arguments)) + if text is None: + model = self.params.model_validate(given) + else: + model = self.params.model_validate_json(text) except pydantic.ValidationError as error: - raise ArgumentError( - tool=self.name, - violations=[ - { - "path": ".".join(str(part) for part in violation["loc"]), - "message": violation["msg"], - "type": violation["type"], - } - for violation in error.errors(include_url=False) - ], - ) from error + raise _argument_error(self.name, error) from error # By field name, not by alias: the wire carries the alias and the # function is called with the name Python can spell. return {name: getattr(model, name) for name in type(model).model_fields} @@ -198,6 +210,55 @@ def listing(self) -> dict[str, Any]: return {"name": self.name, "description": self.description, "input_schema": self.schema} +def _json_text(arguments: dict[str, Any]) -> bytes | None: + """The arguments as JSON text, or `None` when they are not plain JSON. + + Plain JSON can still have no UTF-8 text: `json.loads` keeps a lone surrogate + such as `"\\ud800"`, which an HTTP body can carry. Strict Python validation + takes a `str` as it is, so that call is validated as Python. + """ + if not _is_json(arguments): + return None + try: + return pydantic_core.to_json(arguments) + except pydantic_core.PydanticSerializationError: + return None + + +def _is_json(value: Any) -> bool: + """Whether a value is plain parsed JSON at every depth. + + By exact type: a `StrEnum` member is a `str` and a `NamedTuple` a `tuple`, and + each is a Python object its caller chose over the JSON form. + """ + pending = [value] + while pending: + item = pending.pop() + if type(item) is list: + pending.extend(item) + elif type(item) is dict: + if not all(type(key) is str for key in item): + return False + pending.extend(item.values()) + elif type(item) not in _JSON_SCALARS: + return False + return True + + +def _argument_error(tool_name: str, error: pydantic.ValidationError) -> ArgumentError: + return ArgumentError( + tool=tool_name, + violations=[ + { + "path": ".".join(str(part) for part in violation["loc"]), + "message": violation["msg"], + "type": violation["type"], + } + for violation in error.errors(include_url=False) + ], + ) + + def _refuse_unsupported_callable(tool_name: str, fn: Callable[..., Any]) -> None: if ( inspect.iscoroutinefunction(fn) @@ -257,7 +318,7 @@ def _field(tool_name: str, parameter: inspect.Parameter) -> tuple[Any, Any]: ) if parameter.annotation is inspect.Parameter.empty: raise WorldBug(f"tool {tool_name!r}: argument {parameter.name!r} needs a type annotation") - _refuse_unreachable_annotation(tool_name, parameter) + _refuse_unfaithful_annotation(tool_name, parameter) if isinstance(parameter.default, _MUTABLE_DEFAULTS): raise WorldBug( f"tool {tool_name!r}: argument {parameter.name!r} has a mutable default; use `None` " @@ -267,41 +328,85 @@ def _field(tool_name: str, parameter: inspect.Parameter) -> tuple[Any, Any]: return (parameter.annotation, default) -def _refuse_unreachable_annotation(tool_name: str, parameter: inspect.Parameter) -> None: - for atom in _atoms(parameter.annotation): - # A value, not a class: `Literal[Colour.RED]` carries the member itself, - # and strict validation would then accept only that member object, which - # no wire format carries -- while the schema says `"const": "red"`. - if isinstance(atom, _UNREACHABLE_UNDER_STRICT): - raise _unreachable(tool_name, parameter, type(atom).__name__) - # Most atoms are not classes either: a `Literal`'s strings, a `FieldInfo` - # out of `Annotated`, a parameterised generic. `issubclass` refuses those - # rather than answering, so they are skipped before it is asked. +def _refuse_unfaithful_annotation(tool_name: str, parameter: inspect.Parameter) -> None: + """Refuse an annotation whose validation would not match the schema it publishes. + + Two shapes do, anywhere in the annotation or in the fields of a type it + reaches. A `Literal` of a value JSON cannot spell -- a plain `Enum` member, or + `bytes` -- publishes `"const": "red"` and then accepts only the member, which + is less than the schema says. + + And a type with a pydantic config of its own, which can accept more: a + `BaseModel`, a pydantic dataclass, or anything given `@with_config`. The + argument model's strict config reaches a nested stdlib dataclass or + `TypedDict`, but such a type validates under its own config, which is lax + unless it says otherwise, so `"5"` would be an `int` one level down. It must + say `strict=` itself, `True` to follow the tool list or `False` to relax + deliberately. + """ + for atom in _reachable(parameter.annotation): + if get_origin(atom) is Literal: + for value in get_args(atom): + if not isinstance(value, _JSON_SCALARS): + raise WorldBug( + f"tool {tool_name!r}: argument {parameter.name!r} is a `Literal` of " + f"{value!r}, which JSON cannot send; use a `Literal` of strings or " + f"numbers, or the `Enum` class itself" + ) if not isinstance(atom, type): continue - if issubclass(atom, _UNREACHABLE_UNDER_STRICT): - raise _unreachable(tool_name, parameter, atom.__name__) + config = _own_config(atom) + if config is not None and "strict" not in config: + raise WorldBug( + f"tool {tool_name!r}: argument {parameter.name!r} uses {atom.__name__}, which " + f"has a pydantic config of its own that does not set `strict`; add " + f"`strict=True` to it, or `strict=False` to accept lax input on purpose" + ) -def _unreachable(tool_name: str, parameter: inspect.Parameter, annotated: str) -> WorldBug: - return WorldBug( - f"tool {tool_name!r}: parameter {parameter.name!r} is annotated " - f"{annotated}; use `str` with a pattern or `Literal`" - ) +def _own_config(atom: type) -> Mapping[str, Any] | None: + if issubclass(atom, pydantic.BaseModel): + return atom.model_config + return getattr(atom, "__pydantic_config__", None) + + +def _reachable(annotation: Any) -> Iterator[Any]: + """Every atom of an annotation, and of each model, dataclass and `TypedDict` it reaches.""" + seen: set[int] = set() + # `seen` holds ids, and an id is unique only while its object is alive. + walked: list[Any] = [] + pending = [annotation] + while pending: + walked.append(pending.pop()) + for atom in _atoms(walked[-1], seen): + yield atom + if isinstance(atom, type): + pending.extend(_field_annotations(atom)) + + +def _field_annotations(atom: type) -> list[Any]: + if issubclass(atom, pydantic.BaseModel): + return [field.annotation for field in atom.model_fields.values()] + if not (dataclasses.is_dataclass(atom) or is_typeddict(atom)): + return [] + try: + return list(get_type_hints(atom).values()) + except Exception: + # pydantic resolved these names from the frame the type was defined in, + # which `get_type_hints` cannot see; the type is not looked into. + return [] def _atoms(annotation: Any, seen: set[int] | None = None) -> Iterator[Any]: """The annotation and everything inside it. - `Annotated[datetime, Field(...)]`, `datetime | None` and `list[datetime]` are - all as unusable as a bare `datetime`, so the refusal looks through unions, - metadata and containers rather than only at the whole annotation. A name for - a type is looked through too: `type Stamp = datetime` (PEP 695) and - `NewType("Stamp", datetime)` both carry the annotation they stand for, and - both publish a schema a strict `datetime` will then refuse. + Looks through unions, `Annotated` metadata and containers, so `Point | None` + and `list[Point]` reach `Point`. A name for a type is looked through too: + `type Where = Point` (PEP 695) and `NewType("Where", Point)` both carry the + annotation they stand for. - `seen` guards the one shape that could otherwise not end: an alias defined in - terms of itself. + `seen` guards the shapes that could otherwise not end: an alias, or a model + walked by `_reachable`, defined in terms of itself. """ seen = set() if seen is None else seen if id(annotation) in seen: diff --git a/src/seahaven/world.py b/src/seahaven/world.py index c2475fb..851db02 100644 --- a/src/seahaven/world.py +++ b/src/seahaven/world.py @@ -256,10 +256,10 @@ def chain(self) -> Handler: The root node's chain, which for a world that adds nothing is this world's middlewares and `invoke` -- the chain it has always been. A contributed - tool has its own, on the node that owns it, because the route to it runs - through more worlds than this one. + tool has its own, on its entry in the tool list, because the route to it + runs through more worlds than this one. """ - return self.composition().root.agent_chain + return self.composition().root.own_chain @property def added_worlds(self) -> Sequence[AddedWorld]: diff --git a/tests/conftest.py b/tests/conftest.py index b257065..f9ca7c3 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -9,6 +9,7 @@ """ import copy +import shutil import sys import tempfile import threading @@ -177,9 +178,17 @@ def _process_wide_runtime_state() -> Iterator[None]: The concurrency gate and the once-per-process sweep are module-level by design (both are about the process, not about one world), so a test that changes either would otherwise change the next one. + + The gate's per-thread mark is cleared for the test and put back after it: + the session-scoped `receivers` fixture in `test_docs_examples.py` holds a + `bulk()` open on the main thread, which would otherwise let every later + main-thread call past the gate without taking a slot. """ gate = instances._gate + marked = getattr(instances._gated, "active", False) + instances._gated.active = False yield + instances._gated.active = marked instances._gate = gate instances._swept = False @@ -409,3 +418,22 @@ def run_cli(capsys: pytest.CaptureFixture[str], *argv: str) -> CliResult: code = main(list(argv)) captured = capsys.readouterr() return CliResult(code=code, out=captured.out, err=captured.err) + + +# Where `broken_tidy` appends its line, relative to the project it copies. +BROKEN_MODULE = Path("src/tidy/tools/notes.py") + + +def broken_tidy(tmp_path: Path, statement: str) -> tuple[Path, int]: + """A copy of `tidy` whose tool module ends in `statement`, and the line it is on. + + Written per test rather than committed: a file with a syntax error in it + would fail `ruff` and `ty` over the tree. + """ + root = tmp_path / "tidy" + shutil.copytree(WORLDS / "tidy", root, ignore=shutil.ignore_patterns("__pycache__")) + module = root / BROKEN_MODULE + source = module.read_text(encoding="utf-8") + line = source.count("\n") + 2 + module.write_text(f"{source}\n{statement}\n", encoding="utf-8") + return root, line diff --git a/tests/test_call.py b/tests/test_call.py index 345e714..2671cf2 100644 --- a/tests/test_call.py +++ b/tests/test_call.py @@ -146,6 +146,8 @@ def test_a_tool_is_called_with_exactly_the_validated_arguments(ctx: Ctx) -> None """ class Point(BaseModel): + model_config = ConfigDict(strict=True) + x: int y: int diff --git a/tests/test_call_log.py b/tests/test_call_log.py index e873a0f..016e498 100644 --- a/tests/test_call_log.py +++ b/tests/test_call_log.py @@ -211,19 +211,43 @@ def counted(ctx: Ctx, n: int) -> int: assert len(instance.call_log()) == instance.call_count == 1 -def test_a_tool_that_mutates_its_arguments_does_not_alter_the_record(tmp_path: Path) -> None: +def counting_world(tmp_path: Path) -> World: world = build_world(tmp_path) @world.tool - def consume(ctx: Ctx, tags: list[str]) -> int: - """Empty the list it was handed.""" - count = len(tags) - tags.clear() - return count + def count(ctx: Ctx, tags: list[str]) -> int: + """Count the tags.""" + return len(tags) - with world.instance(None) as instance: + return world + + +def test_a_caller_that_changes_its_list_after_the_call_does_not_alter_the_record( + tmp_path: Path, +) -> None: + with counting_world(tmp_path).instance(None) as instance: given = ["a", "b"] - instance.call("consume", tags=given) + assert instance.call("count", tags=given) == 2 + given.append("later") + + (record,) = instance.call_log() + + assert record.arguments == {"tags": ["a", "b"]} + + +def test_a_middleware_that_changes_an_argument_in_place_does_not_alter_the_record( + tmp_path: Path, +) -> None: + """The caller's list, changed before validation copies it: the record is the call as made.""" + world = counting_world(tmp_path) + + @world.middleware + def extend(ctx: Ctx, call: Call, next_: Handler) -> object: + call.arguments["tags"].append("added") + return next_(ctx, call) + + with world.instance(None) as instance: + assert instance.call("count", tags=["a", "b"]) == 3 (record,) = instance.call_log() diff --git a/tests/test_cli_check.py b/tests/test_cli_check.py index da105cf..1a2b42e 100644 --- a/tests/test_cli_check.py +++ b/tests/test_cli_check.py @@ -15,7 +15,7 @@ from seahaven.cli import CliError from seahaven.cli.check import collect from seahaven.cli.new import render -from tests.conftest import WORLDS, run_cli +from tests.conftest import BROKEN_MODULE, WORLDS, broken_tidy, run_cli pytestmark = pytest.mark.usefixtures("isolated_imports") @@ -141,13 +141,27 @@ def test_a_package_with_no_world_is_sh501( assert code == 1 assert line.startswith("SH501 error") assert "world = seahaven.World(...)" in line + assert "fix: give the package a world" in line -def test_an_import_error_is_sh501_with_the_last_traceback_line() -> None: +def test_a_missing_world_module_is_sh501_with_the_last_traceback_line() -> None: (finding,) = collect("no_such_module_at_all:world", WORLDS / "tidy").findings assert finding.code == "SH501" assert "ModuleNotFoundError" in finding.message assert "\n" not in finding.message + assert "--world" in finding.fix + + +def test_an_import_error_is_a_finding_at_the_line_that_raised( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + root, line = broken_tidy(tmp_path, "undefined_name") + code, (printed,) = check(root, monkeypatch, capsys) + assert code == 1 + assert FINDING_LINE.match(printed), printed + assert printed.startswith(f"SH501 error {BROKEN_MODULE}:{line} ") + assert "NameError" in printed + assert printed.endswith(f"fix: fix the error at {BROKEN_MODULE}:{line}") def test_a_user_error_is_not_a_finding() -> None: diff --git a/tests/test_cli_new.py b/tests/test_cli_new.py index 590b60e..1cb3d77 100644 --- a/tests/test_cli_new.py +++ b/tests/test_cli_new.py @@ -15,6 +15,7 @@ from pathlib import Path import pytest +from packaging.requirements import Requirement from seahaven.cli import CliError from seahaven.cli.new import HUB_FILES, render, seahaven_requirement @@ -115,6 +116,17 @@ def test_the_scaffold_declares_the_conflict_between_serve_and_mcp(scaffold: Path ) +def test_the_scaffold_installs_pytest_with_uv_sync(scaffold: Path) -> None: + """The next steps say `uv run pytest`, and seahaven does not depend on pytest. + + `uv sync` installs the `dev` group by default, so that group is where the + scaffold has to ask for it. + """ + declared = tomllib.loads((scaffold / "pyproject.toml").read_text(encoding="utf-8")) + dev = declared["dependency-groups"]["dev"] + assert [Requirement(entry).name for entry in dev] == ["pytest"] + + def test_the_scaffold_pins_the_installed_minor_version(scaffold: Path) -> None: requirement = seahaven_requirement() assert requirement.startswith("seahaven~=") @@ -130,10 +142,14 @@ def test_check_passes_on_a_fresh_scaffold( assert result.code == 0 -def test_pytest_passes_on_a_fresh_scaffold(scaffold: Path) -> None: - """A subprocess, because a second pytest cannot run inside this one.""" - finished = subprocess.run( - [sys.executable, "-m", "pytest", "-q"], +def python_in(scaffold: Path, *args: str) -> subprocess.CompletedProcess[str]: + """This interpreter, run in the scaffold with its package on `PYTHONPATH`. + + Standing in for `uv run python`: the scaffold's own environment would install + seahaven from an index it has not been published to. + """ + return subprocess.run( + [sys.executable, *args], cwd=scaffold, env={**os.environ, "PYTHONPATH": str(scaffold / "src")}, capture_output=True, @@ -141,33 +157,37 @@ def test_pytest_passes_on_a_fresh_scaffold(scaffold: Path) -> None: timeout=300, check=False, ) - assert finished.returncode == 0, finished.stdout + finished.stderr -def test_the_readme_builds_the_first_fixture(scaffold: Path) -> None: - """The command under the README's "Making a fixture", run as written. - - `uv run python` is this interpreter with the package on `PYTHONPATH`, as in - the pytest test above: the scaffold's own environment would install seahaven - from an index it has not been published to. - """ +def build_as_the_readme_says(scaffold: Path) -> None: + """The command under the README's "Making a fixture", run as written.""" readme = (scaffold / "README.md").read_text(encoding="utf-8") section = readme.split("## Making a fixture\n", 1)[1] command = section.split("```sh\n", 1)[1].split("\n```", 1)[0] launcher = "uv run python " assert command.startswith(launcher) - finished = subprocess.run( - [sys.executable, *shlex.split(command.removeprefix(launcher))], - cwd=scaffold, - env={**os.environ, "PYTHONPATH": str(scaffold / "src")}, - capture_output=True, - text=True, - timeout=120, - check=False, - ) + finished = python_in(scaffold, *shlex.split(command.removeprefix(launcher))) + assert finished.returncode == 0, finished.stdout + finished.stderr + +def test_pytest_passes_on_a_fresh_scaffold(scaffold: Path) -> None: + """A subprocess, because a second pytest cannot run inside this one.""" + finished = python_in(scaffold, "-m", "pytest", "-q") + assert finished.returncode == 0, finished.stdout + finished.stderr + + +def test_pytest_passes_after_the_readme_builds_the_first_fixture(scaffold: Path) -> None: + """The README's first step is a `build`, and the tests have to survive it.""" + build_as_the_readme_says(scaffold) + + finished = python_in(scaffold, "-m", "pytest", "-q") assert finished.returncode == 0, finished.stdout + finished.stderr + + +def test_the_readme_builds_the_first_fixture(scaffold: Path) -> None: + build_as_the_readme_says(scaffold) + generator = (scaffold / "fixtures_src" / "generate.py").read_text(encoding="utf-8") fixture = load(scaffold / "fixtures" / "empty") assert fixture.id == "empty" @@ -194,6 +214,16 @@ def test_a_name_that_is_not_a_package_name_is_refused(tmp_path: Path, name: str) assert not (tmp_path / "world").exists() +@pytest.mark.parametrize("name", ["email", "json", "Queue", "seahaven"]) +def test_a_name_that_shadows_an_existing_module_is_refused(tmp_path: Path, name: str) -> None: + """Each is a legal package name that loses to a module already on the path.""" + with pytest.raises(CliError) as raised: + render(name, tmp_path / "world") + assert f"add a suffix, such as '{name.lower()}_world'" in str(raised.value) + assert f"the package {name.lower()!r}" in str(raised.value) + assert not (tmp_path / "world").exists() + + @pytest.mark.parametrize("name", ["café", "con", "trailing.", "x" * 129]) def test_a_name_that_is_not_a_world_name_is_refused(tmp_path: Path, name: str) -> None: """All four make a legal package name, and none makes a legal `World(name=...)`. @@ -226,6 +256,17 @@ def test_the_hub_files_re_export_the_framework(tmp_path: Path) -> None: assert "name: hubbed" in (hubbed / "openenv.yaml").read_text(encoding="utf-8") +def test_the_hub_container_serves_on_every_interface(tmp_path: Path) -> None: + """The server binds loopback by default, so the container has to say otherwise itself.""" + hubbed = render("hubbed", tmp_path / "hub" / "hubbed", hub=True) + command = next( + line + for line in (hubbed / "Dockerfile").read_text(encoding="utf-8").splitlines() + if line.startswith("CMD ") + ) + assert '"--host", "0.0.0.0"' in command + + def test_new_prints_the_next_steps( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] ) -> None: diff --git a/tests/test_client.py b/tests/test_client.py index 342bede..b8183be 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -25,7 +25,11 @@ "seahaven.openenv", exc_type=ImportError, reason="the serve extra does not import here" ) -from openenv.core.env_server.mcp_types import CallToolAction, ListToolsAction +from openenv.core.env_server.mcp_types import ( + CallToolAction, + ListToolsAction, + ListToolsObservation, +) from seahaven.openenv import SeahavenClient, SeahavenObservation, SeahavenState from tests.serving import serving @@ -257,9 +261,19 @@ def test_a_reset_frame_parses_through_the_step_hook_with_only_its_metadata( def test_the_observation_model_refuses_a_frame_it_does_not_know(client: SeahavenClient) -> None: - """`SeahavenObservation` forbids extras, which is why `list_tools` does not use it.""" - with pytest.raises(ValueError, match="tools"): - client._parse_result({"observation": {"tools": []}}) + """`SeahavenObservation` forbids extras: a field nothing sends is an error, not dropped.""" + with pytest.raises(ValueError, match="bogus"): + client._parse_result({"observation": {"bogus": 1}}) + + +def test_parse_result_answers_a_tool_list_as_a_list_tools_observation( + client: SeahavenClient, +) -> None: + result = client._parse_result( + {"observation": {"tools": [{"name": "ping", "description": "d", "input_schema": {}}]}} + ) + assert isinstance(result.observation, ListToolsObservation) + assert result.observation.tools[0].name == "ping" # --- the client, on a server ----------------------------------------------- @@ -286,6 +300,16 @@ def test_the_client_drives_a_world_synchronously(world: World) -> None: assert state.state["db"]["log"][0]["key"] == {"id": "n1"} +def test_stepping_a_list_tools_action_answers_the_tool_list(world: World) -> None: + with serving(world) as url, SeahavenClient(base_url=url) as env: + env.reset() + result = env.step(ListToolsAction()) + assert isinstance(result.observation, ListToolsObservation) + names = [tool.name for tool in result.observation.tools] + assert "rows" in names and "controller_run_sql" not in names + assert env.call("rows", sql="SELECT 1 AS n").result == [{"n": 1}] + + def test_the_client_drives_a_world_asynchronously(world: World) -> None: async def drive(url: str) -> None: async with SeahavenClient(base_url=url) as env: diff --git a/tests/test_composite_dispatch.py b/tests/test_composite_dispatch.py index aeffbbd..4f3f5cb 100644 --- a/tests/test_composite_dispatch.py +++ b/tests/test_composite_dispatch.py @@ -19,7 +19,7 @@ from seahaven import instances from seahaven.call import Call, Handler from seahaven.ctx import Ctx -from seahaven.errors import ArgumentError, UnknownTool, WorldBug +from seahaven.errors import ArgumentError, ToolError, UnknownTool, WorldBug from seahaven.handles import WorldHandle from seahaven.instances import Instance from seahaven.world import World @@ -86,7 +86,7 @@ def test_two_accounts_of_one_world_do_not_share_rows(tmp_path: Path) -> None: # ----------------------------------------------------------------- the chain -def test_the_agent_chain_is_the_whole_canonical_route(tmp_path: Path) -> None: +def test_the_agent_chain_is_the_whole_contributing_route(tmp_path: Path) -> None: trace: list[str] = [] with three_levels(tmp_path, trace).instance(None) as live: live.call("leaf_write", value="x") @@ -240,6 +240,173 @@ def from_the_middle(ctx: Ctx, call: Call, next_: Handler) -> Any: assert seen[:2] == ["host: has middle, no leaf", "middle: has leaf, no middle"] +# ------------------------------------------------------ the chain, shared + + +def shared_under_a_middle(tmp_path: Path, trace: list[str]) -> tuple[World, World, World]: + """`leaf` twice: through `middle` as `m_`, and straight from the host as `l_`. + + The canonical route to `leaf` is the host's direct edge, so only the `m_` + names cross a world the canonical route does not. + """ + host = traced(rooted("host", tmp_path), trace) + middle = traced(composable_world("middle"), trace) + leaf = traced(composable_world("leaf"), trace) + middle.add_world(leaf, name="leaf") + host.add_world(middle, name="middle", tool_prefix="m_") + host.add_world(leaf, name="leaf", tool_prefix="l_") + return host, middle, leaf + + +def recording_names(world: World, seen: list[str]) -> None: + @world.middleware + def record_name(ctx: Ctx, call: Call, next_: Handler) -> Any: + seen.append(call.name) + return next_(ctx, call) + + +def test_sharing_a_node_keeps_the_middle_worlds_middleware_on_what_it_contributes( + tmp_path: Path, +) -> None: + seen: list[str] = [] + company = rooted("company", tmp_path) + shop = composable_world("shop") + payments = composable_world("payments") + recording_names(shop, seen) + shop.add_world(payments, name="payments") + company.add_world(shop, name="shop", tool_prefix="shop_") + with company.instance(None) as live: + live.call("shop_payments_write", value="before") + assert seen == ["shop_payments_write"] + + company.add_world(payments, name="payments", tool_prefix="pay_") + with company.instance(None) as live: + live.call("shop_payments_write", value="through the shop") + live.call("pay_payments_write", value="direct") + assert sorted(live.call("pay_payments_read")) == ["direct", "through the shop"] + assert seen == ["shop_payments_write", "shop_payments_write"] + + +def test_each_name_of_a_shared_tool_runs_its_own_routes_middleware(tmp_path: Path) -> None: + trace: list[str] = [] + host, _middle, _leaf = shared_under_a_middle(tmp_path, trace) + with host.instance(None) as live: + live.call("m_leaf_write", value="through the middle") + assert trace == ["host sees host", "middle sees middle", "leaf sees leaf"] + trace.clear() + live.call("l_leaf_write", value="direct") + assert trace == ["host sees host", "leaf sees leaf"] + both = ["direct", "through the middle"] + assert sorted(live.call("m_leaf_read")) == both + assert sorted(live.call("l_leaf_read")) == both + + +def test_both_sides_of_a_diamond_run_their_own_middle(tmp_path: Path) -> None: + trace: list[str] = [] + host = traced(rooted("host", tmp_path), trace) + leaf = traced(composable_world("leaf"), trace) + for side in ("left", "right"): + middle = traced(composable_world(side), trace) + middle.add_world(leaf, name="leaf") + host.add_world(middle, name=side, tool_prefix=f"{side[0]}_") + with host.instance(None) as live: + live.call("r_leaf_write", value="x") + assert trace == ["host sees host", "right sees right", "leaf sees leaf"] + trace.clear() + live.call("l_leaf_write", value="x") + assert trace == ["host sees host", "left sees left", "leaf sees leaf"] + + +def test_a_route_the_lists_filter_out_does_not_choose_the_chain(tmp_path: Path) -> None: + trace: list[str] = [] + host = traced(rooted("host", tmp_path), trace) + middle = traced(composable_world("middle"), trace) + leaf = traced(composable_world("leaf"), trace) + host.add_world(leaf, name="leaf", tool_allow_list=[]) + middle.add_world(leaf, name="leaf") + host.add_world(middle, name="middle") + route = ["host sees host", "middle sees middle", "leaf sees leaf"] + with host.instance(None) as live: + live.call("leaf_write", value="by name") + assert trace == route + trace.clear() + live.call(leaf.tools["leaf_write"].fn, value="by function") + assert trace == route + + +def test_a_shared_leaf_three_deep_keeps_every_world_between(tmp_path: Path) -> None: + trace: list[str] = [] + host = traced(rooted("host", tmp_path), trace) + a, b = traced(composable_world("a"), trace), traced(composable_world("b"), trace) + leaf = traced(composable_world("leaf"), trace) + b.add_world(leaf, name="leaf") + a.add_world(b, name="b") + host.add_world(a, name="a") + host.add_world(leaf, name="leaf", tool_prefix="x_") + with host.instance(None) as live: + live.call("leaf_write", value="deep") + assert trace == ["host sees host", "a sees a", "b sees b", "leaf sees leaf"] + trace.clear() + live.call("x_leaf_write", value="shallow") + assert trace == ["host sees host", "leaf sees leaf"] + + +class MiddleError(ToolError): + """The middle world's own error, which its handler turns every leaf error into.""" + + def __init__(self, message: str) -> None: + super().__init__("middle", message) + + +def test_a_middle_worlds_error_handler_shapes_the_errors_it_contributes(tmp_path: Path) -> None: + host, middle, leaf = shared_under_a_middle(tmp_path, []) + + @leaf.tool + def refuse(ctx: Ctx) -> None: + """Fail the way the leaf's own world fails.""" + raise Boom("the leaf said no") + + @middle.middleware + def as_the_middles(ctx: Ctx, call: Call, next_: Handler) -> Any: + try: + return next_(ctx, call) + except Boom as error: + raise MiddleError(str(error)) from error + + with host.instance(None) as live: + with pytest.raises(MiddleError): + live.call("m_refuse") + with pytest.raises(Boom): + live.call("l_refuse") + + +def test_a_middleware_registered_later_reaches_a_route_through_a_shared_node( + tmp_path: Path, +) -> None: + seen: list[str] = [] + host, middle, _leaf = shared_under_a_middle(tmp_path, []) + with host.instance(None) as live: + live.call("m_leaf_write", value="before") + recording_names(middle, seen) + live.call("m_leaf_write", value="after") + live.call("l_leaf_write", value="after") + assert seen == ["m_leaf_write"] + + +def test_a_nested_call_into_a_shared_node_runs_only_the_owners_layer(tmp_path: Path) -> None: + trace: list[str] = [] + host, _middle, _leaf = shared_under_a_middle(tmp_path, trace) + + @host.tool + def reach(ctx: Ctx) -> None: + """Write into the leaf through the middle's handle.""" + ctx.worlds.middle.worlds.leaf.call("leaf_write", value="nested") + + with host.instance(None) as live: + live.call("reach") + assert trace == ["host sees host", "leaf sees leaf"] + + # ------------------------------------------------------------- nested calls diff --git a/tests/test_composite_instance.py b/tests/test_composite_instance.py index 7810bc1..20814dd 100644 --- a/tests/test_composite_instance.py +++ b/tests/test_composite_instance.py @@ -13,13 +13,14 @@ from pathlib import Path from typing import Any +import apsw import emporium import pytest from seahaven.clock import Clock from seahaven.ctx import Ctx from seahaven.db import open_instance -from seahaven.errors import WorldBug +from seahaven.errors import DbError, WorldBug from seahaven.ids import BUILD_STREAM, CONTROL_STREAM, INSPECTION_STREAM, INSTANCE_STREAM, Ids from seahaven.instances import node_seed from seahaven.world import World @@ -287,6 +288,49 @@ def test_a_node_two_hosts_reach_runs_its_hooks_once(tmp_path: Path) -> None: assert ran == ["host", "middle", "shared"] +def test_a_shared_node_runs_after_every_world_that_adds_it(tmp_path: Path) -> None: + ran: list[str] = [] + company = rooted("company", tmp_path) + payments, shop = composable_world("payments"), composable_world("shop") + shop.add_world(payments, name="payments") + company.add_world(payments, name="payments") + company.add_world(shop, name="shop", tool_prefix="s_") + for world in (company, payments, shop): + recording(world, ran) + + with company.instance(None): + pass + + assert ran == ["company", "shop", "payments"] + + +def test_a_host_that_seeds_a_shared_node_seeds_it_composed_as_alone(tmp_path: Path) -> None: + """A shop's hook seeds its payments account the same whoever else adds that account.""" + plans: list[str] = [] + company = rooted("company", tmp_path) + shop = rooted("shop", tmp_path / "shop") + payments = composable_world("payments") + + @shop.instance_startup + def open_account(ctx: Ctx) -> None: + ctx.worlds.payments.state["plan"] = "merchant" + + @payments.instance_startup + def default_plan(ctx: Ctx) -> None: + plans.append(ctx.state.setdefault("plan", "free")) + + shop.add_world(payments, name="payments") + company.add_world(payments, name="payments") + company.add_world(shop, name="shop", tool_prefix="s_") + + with shop.instance(None): + pass + with company.instance(None): + pass + + assert plans == ["merchant", "merchant"] + + def regional(world: World, seen: dict[str, str]) -> None: @world.instance_startup def note(ctx: Ctx, *, region: str = "us") -> None: @@ -481,6 +525,30 @@ def test_a_bulk_block_that_raises_rolls_every_node_back(tmp_path: Path) -> None: assert (live.call("host_read"), live.call("child_read")) == ([], []) +def test_a_bulk_commit_that_fails_keeps_the_nodes_committed_before_it(tmp_path: Path) -> None: + """SQLite cannot commit two files atomically: the child commits, then the root refuses.""" + host = composable_world( + "host", + extra_schema="CREATE TABLE host_refs (" + " id TEXT PRIMARY KEY," + " row_id TEXT NOT NULL REFERENCES host_rows(id) DEFERRABLE INITIALLY DEFERRED" + ") STRICT;", + fixtures_dir=tmp_path / "fixtures", + work_dir=tmp_path / "work", + ) + host.add_world(composable_world("child"), name="child") + with host.instance(None) as live: + with pytest.raises(DbError) as raised, live.bulk() as ctx: + ctx.worlds.child.db.execute("INSERT INTO child_rows VALUES ('a', 'child')") + ctx.db.execute("INSERT INTO host_refs VALUES ('r', 'nobody')") + assert raised.value.sqlite_code == apsw.SQLITE_CONSTRAINT_FOREIGNKEY + assert live.inspect().rows("SELECT id FROM host_refs") == [] + assert live.call("child_read") == ["child"] + assert [(record.world, record.table) for record in live.change_log()] == [ + ("child", "child_rows") + ] + + # ---------------------------------------------------------- the committed tree diff --git a/tests/test_composition.py b/tests/test_composition.py index 3eca910..4012335 100644 --- a/tests/test_composition.py +++ b/tests/test_composition.py @@ -19,7 +19,7 @@ import pytest import shop -from seahaven.composition import attached_limit +from seahaven.composition import attached_limit, canonical_tree, hosts_first from seahaven.ctx import Ctx from seahaven.errors import WorldBug from seahaven.world import World @@ -106,6 +106,32 @@ def test_depth_beats_registration_order() -> None: assert host.composition().by_key[leaf, None].aliases == ("deep/under_deep/under_middle",) +def test_hosts_first_is_the_canonical_preorder_without_sharing() -> None: + host, left, right, deep, deeper = ( + make_world(n) for n in ("host", "left", "right", "deep", "deeper") + ) + deep.add_world(deeper, name="deeper") + left.add_world(deep, name="deep") + host.add_world(left, name="left") + host.add_world(right, name="right") + composition = host.composition() + order = [node.path for node in hosts_first(composition)] + assert order == ["main", "left", "left/deep", "left/deep/deeper", "right"] + assert order == [node.path for node in canonical_tree(composition.root)] + + +def test_hosts_first_walks_a_shared_node_after_its_last_host() -> None: + company, payments, shop = make_world("company"), make_world("payments"), make_world("shop") + shop.add_world(payments, name="payments") + company.add_world(payments, name="payments") + company.add_world(shop, name="shop") + assert [node.world.name for node in hosts_first(company.composition())] == [ + "company", + "shop", + "payments", + ] + + def deep_diamond(depth: int, *tools: str) -> list[World]: """A spine whose every level is reached by two branches: 2**depth routes to the last. @@ -329,6 +355,29 @@ def test_by_fn_carries_every_entry_a_function_reaches() -> None: assert [entry.node.path for entry in entries] == ["ours", "theirs"] +def shared_under_a_middle() -> World: + """`leaf` twice: through `middle` as `m_`, and straight from the host as `l_`.""" + host, middle = make_world("host", "own"), make_world("middle") + leaf = make_world("leaf", "leaf_write", "leaf_read") + middle.add_world(leaf, name="leaf") + host.add_world(middle, name="middle", tool_prefix="m_") + host.add_world(leaf, name="leaf", tool_prefix="l_") + return host + + +def test_each_entry_carries_the_chain_of_the_route_that_contributed_it() -> None: + tools = shared_under_a_middle().composition().tools + assert tools["m_leaf_write"].node is tools["l_leaf_write"].node + assert tools["m_leaf_write"].chain is not tools["l_leaf_write"].chain + + +def test_entries_of_one_route_share_one_chain() -> None: + host = shared_under_a_middle() + tools = host.composition().tools + assert tools["m_leaf_write"].chain is tools["m_leaf_read"].chain + assert tools["own"].chain is host.chain + + # ------------------------------------------------------------- bound startup diff --git a/tests/test_db.py b/tests/test_db.py index da73e5d..96f3632 100644 --- a/tests/test_db.py +++ b/tests/test_db.py @@ -3,6 +3,7 @@ import gc import time import weakref +from collections.abc import Iterator from pathlib import Path from typing import Any, Literal, cast @@ -199,12 +200,70 @@ def test_the_raw_connection_is_the_live_one(notes: Db) -> None: def test_an_instance_connection_is_hardened(db: Db) -> None: assert db.conn.pragma("journal_mode") == "wal" - assert db.conn.pragma("synchronous") == 1 + assert db.conn.pragma("synchronous") == 0 assert db.conn.pragma("foreign_keys") == 1 assert db.conn.config(apsw.SQLITE_DBCONFIG_DEFENSIVE, -1) == 1 assert db.conn.config(apsw.SQLITE_DBCONFIG_TRUSTED_SCHEMA, -1) == 0 +class _CountingVfs(apsw.VFS): + """The default VFS, counting every `xSync` any file it opens makes.""" + + def __init__(self) -> None: + self.base = apsw.vfs_names()[0] + self.syncs = 0 + super().__init__("counting", self.base, makedefault=True) + + def xOpen(self, name: str | apsw.URIFilename | None, flags: list[int]) -> apsw.VFSFile: + return _CountingFile(self, name, flags) + + +class _CountingFile(apsw.VFSFile): + def __init__( + self, vfs: _CountingVfs, name: str | apsw.URIFilename | None, flags: list[int] + ) -> None: + self._vfs = vfs + super().__init__(vfs.base, name, flags) + + def xSync(self, flags: int) -> None: + self._vfs.syncs += 1 + super().xSync(flags) + + +@pytest.fixture +def counting_vfs() -> Iterator[_CountingVfs]: + vfs = _CountingVfs() + try: + yield vfs + finally: + vfs.unregister() + + +@pytest.mark.parametrize("how", ["copied", "blank"]) +def test_opening_an_instance_file_does_not_fsync( + how: str, db_path: Path, clock: Clock, seed: bytes, counting_vfs: _CountingVfs +) -> None: + """Neither building, converting to WAL nor writing an instance's file waits on the disk. + + "copied" is a file in the rollback journal a fixture copy arrives in, opened + and converted by `open_instance`; "blank" is one `build_blank` writes first. + """ + if how == "copied": + plain = apsw.Connection(str(db_path)) + plain.execute(NOTES) + plain.close() + counting_vfs.syncs = 0 # the fixture's own write, which is not the instance's + else: + build_blank(db_path, NOTES, clock=clock, seed=seed).close() + db = open_instance(db_path, clock, seed) + try: + with db.transaction(): + db.execute("INSERT INTO notes (id, body) VALUES ('n1', 'x')") + finally: + db.close() + assert counting_vfs.syncs == 0 + + def test_world_code_keeps_sqlites_own_limits(db: Db) -> None: plain = apsw.Connection(":memory:") limits = ( diff --git a/tests/test_docs.py b/tests/test_docs.py index 8fe3737..f1f2fd4 100644 --- a/tests/test_docs.py +++ b/tests/test_docs.py @@ -1,17 +1,18 @@ -"""The bundled docs: the layout exists, and the lint reference is not stale. +"""The bundled docs: the layout exists, every link resolves, and the lint reference is not stale. The pages are hand-written and nothing generates them, so there is no drift test -to write for their prose -- `components/pytest_and_docs.md` §2 says so. What can -go wrong silently is checked here: a page named by the layout (and by `index.md`, -and by a scaffolded world's `AGENTS.md`) that is not there; the lint reference -falling behind the rules, which is the one page whose content is a list of things -that exist elsewhere in the code; and a page still naming either of the two -surfaces the state-format project removed from the docs, which no fence check -catches in prose. +to write for their prose. What can go wrong silently is checked here: a page +named by the layout (and by `index.md`, and by a scaffolded world's `AGENTS.md`) +that is not there; a relative link or anchor in a page, `README.md` or +`CONTRIBUTING.md` that goes nowhere; the lint reference falling behind the rules, +which is the one page whose content is a list of things that exist elsewhere in +the code; and a page still naming either of the two surfaces the state-format +project removed from the docs, which no fence check catches in prose. """ import re from pathlib import Path +from urllib.parse import unquote import pytest @@ -55,16 +56,28 @@ # exactly the staleness these tests exist to catch. _DOCUMENTED = re.compile(r"^\|\s*(SH\d+)\s*\|", re.MULTILINE) -# Every markdown link target except an `http`/`https` URL, which is a link *out* -# of the docs: the test below is about which pages exist, and matching one would -# fail the layout test over a working link. Nothing else is excluded -- a -# `mailto:`, a protocol-relative `//host/x` or an in-page `#anchor` would be read -# as a page and fail. None exists in the docs, and this is a layout test rather -# than a link checker, so the pattern is left as narrow as what it is asked. -_PAGE_LINK = re.compile(r"\]\((?!https?://)([^)]+)\)") +# A markdown link's target, with an optional title after it. Read from text with +# its code removed, so that code is never taken for a link. +_LINK = re.compile(r"\]\(([^)\s]+)(?:\s+\"[^\"]*\")?\)") +# A target with a scheme (`https:`, `mailto:`) or no scheme but a host is a link +# out, which this suite does not fetch. +_EXTERNAL = re.compile(r"^([a-zA-Z][a-zA-Z0-9+.-]*:|//)") +_FENCE = re.compile(r"^[ \t]*(`{3,}|~{3,}).*?^[ \t]*\1", re.MULTILINE | re.DOTALL) +_CODE_SPAN = re.compile(r"(`+).+?\1") +_HEADING = re.compile(r"^#{1,6}[ \t]+(.+?)[ \t#]*$", re.MULTILINE) +_EXPLICIT_ANCHOR = re.compile(r"]+>") DOCS = Path(docs_path()) SOURCE = Path(seahaven.__file__).resolve().parent +# The suite always runs from a checkout, where the repository's own pages are. +REPO = Path(__file__).resolve().parents[1] +LINKED_PAGES = ( + *(DOCS / page for page in PAGES), + REPO / "README.md", + REPO / ".github" / "CONTRIBUTING.md", +) def registered_codes() -> set[str]: @@ -111,21 +124,86 @@ def test_the_docs_are_where_seahaven_docs_says_they_are() -> None: assert found == sorted(PAGES) -def test_every_page_index_md_links_to_is_a_page_of_the_layout() -> None: - """`index.md` is the reading order, and it ships: a dead link in it ships too.""" - index = (DOCS / "index.md").read_text(encoding="utf-8") - linked = set(_PAGE_LINK.findall(index)) - assert linked <= set(PAGES), f"{sorted(linked - set(PAGES))} is linked from index.md" +def without_fences(text: str) -> str: + return _FENCE.sub("", text) + + +def relative_links(text: str) -> list[str]: + """Every link target in `text` that points into the repository, outside code.""" + prose = _CODE_SPAN.sub("", without_fences(text)) + return [target for target in _LINK.findall(prose) if not _EXTERNAL.match(target)] + + +def slug(heading: str) -> str: + """A heading's anchor as GitHub spells it, before a duplicate's `-1` suffix.""" + text = _LINK_MARKUP.sub(r"\1", heading) + # Code keeps its text, `` included; a tag outside code is dropped. + text = _HTML_TAG.sub("", _CODE_SPAN.sub(lambda span: re.sub(r"[`<>]", "", span[0]), text)) + return re.sub(r"[^\w\- ]", "", text.lower()).replace(" ", "-") + + +def anchors(text: str) -> set[str]: + """Every fragment a link into `text` may name: its headings' slugs and explicit anchors.""" + found: set[str] = set() + seen: dict[str, int] = {} + for heading in _HEADING.findall(without_fences(text)): + base = slug(heading) + count = seen.get(base, 0) + seen[base] = count + 1 + found.add(base if count == 0 else f"{base}-{count}") + return found | set(_EXPLICIT_ANCHOR.findall(text)) + + +def broken_links(page: Path) -> list[str]: + """Each relative link in `page` whose file, or whose anchor in that file, is not there.""" + bundled = page.is_relative_to(DOCS) + broken = [] + for target in relative_links(page.read_text(encoding="utf-8")): + path, _, fragment = target.partition("#") + resolved = (page.parent / unquote(path)).resolve() if path else page + if not resolved.exists(): + broken.append(f"{target} (missing)") + elif bundled and not resolved.is_relative_to(DOCS): + # The docs ship in the wheel without the rest of the repository. + broken.append(f"{target} (outside the bundled docs)") + elif ( + bundled + and resolved.suffix == ".md" + and resolved.relative_to(DOCS).as_posix() not in PAGES + ): + broken.append(f"{target} (not a page of the layout)") + elif ( + fragment + and resolved.suffix == ".md" + and fragment not in anchors(resolved.read_text(encoding="utf-8")) + ): + broken.append(f"{target} (no anchor)") + return broken + + +@pytest.mark.parametrize("page", LINKED_PAGES, ids=lambda page: page.name) +def test_every_relative_link_resolves(page: Path) -> None: + """Pages, files and anchors, for every page that ships and the two a contributor reads first.""" + broken = broken_links(page) + assert not broken, f"{page.relative_to(REPO)}: {', '.join(broken)}" def test_a_link_out_of_the_docs_is_not_read_as_a_page_of_the_layout() -> None: - """`index.md` has no external link today, and the first one must not fail the test above. + """Asserted on a sample, because a page with no such link cannot show this.""" + sample = ( + "[the OpenEnv spec](https://example.invalid/spec), [mail](mailto:a@example.invalid), " + '[concepts](concepts.md "Concepts"), [here](#setup) and `[code](not-a-link.md)`\n' + "```md\n[fenced](not-a-link-either.md)\n```\n" + ) + assert relative_links(sample) == ["concepts.md", "#setup"] - Asserted on a sample rather than on the page, because the page that has no - such link is exactly the page that cannot show this. - """ - sample = "[the OpenEnv spec](https://example.invalid/spec) and [concepts](concepts.md)\n" - assert set(_PAGE_LINK.findall(sample)) == {"concepts.md"} + +def test_a_heading_anchor_is_the_one_github_makes() -> None: + assert slug("SH103 — a wall-clock `DEFAULT`") == "sh103--a-wall-clock-default" + assert slug("[Composition](composition.md) and `ctx.worlds`") == "composition-and-ctxworlds" + assert slug("`seahaven new `") == "seahaven-new-name" + text = '# Setup\n\n## Setup\n\n```sh\n# not a heading\n```\n\n' + assert anchors(text) == {"setup", "setup-1", "kept"} def test_every_registered_lint_code_is_documented() -> None: diff --git a/tests/test_env.py b/tests/test_env.py index d343af0..80f4a91 100644 --- a/tests/test_env.py +++ b/tests/test_env.py @@ -11,6 +11,7 @@ import json import logging import re +import uuid from datetime import UTC, datetime from pathlib import Path from typing import Any @@ -22,7 +23,7 @@ from seahaven.ctx import Ctx from seahaven.errors import SeahavenError, ToolError, WorldBug from seahaven.world import World -from tests.conftest import INSTANT_ISO, build_world +from tests.conftest import INSTANT, INSTANT_ISO, build_world # The subpackage and not `openenv`: what this module imports is # `seahaven.openenv`, so that is what has to import for the tests below to mean @@ -55,6 +56,7 @@ pytestmark = pytest.mark.filterwarnings("ignore:controller_run_sql is deprecated") CONTROL_SQL = "SELECT count(*) AS n FROM notes" +UID = uuid.UUID(int=5) def wire_error(observation: SeahavenObservation) -> tuple[str, str] | None: @@ -511,6 +513,26 @@ def test_bad_arguments_are_rendered_as_invalid_arguments(env: SeahavenEnv) -> No assert observation.error.error_type.value == "invalid_args" +def test_an_argument_arrives_as_the_json_spelling_its_schema_publishes(world: World) -> None: + """A `tuple`, a `UUID` and a `datetime` are an array and two strings on the wire.""" + + @world.tool + def book(ctx: Ctx, span: tuple[int, int], id: uuid.UUID, at: datetime) -> dict[str, str]: + """Book.""" + return {"span": repr(span), "id": id.hex, "at": at.isoformat()} + + env = SeahavenEnv(world, include_control_tools=False) + env.reset() + arguments = json.loads(json.dumps({"span": [1, 2], "id": str(UID), "at": INSTANT_ISO})) + + assert call(env, "book", **arguments).result == { + "span": "(1, 2)", + "id": UID.hex, + "at": INSTANT.isoformat(), + } + assert wire_error(call(env, "book", **{**arguments, "span": ["1", "2"]})) is not None + + # --- step: control tools --------------------------------------------------- diff --git a/tests/test_find_world.py b/tests/test_find_world.py index d99dffe..714214e 100644 --- a/tests/test_find_world.py +++ b/tests/test_find_world.py @@ -14,7 +14,7 @@ from seahaven.cli import CliError, discover, find_world, package_name from seahaven.world import World -from tests.conftest import WORLDS +from tests.conftest import BROKEN_MODULE, WORLDS, broken_tidy pytestmark = pytest.mark.usefixtures("isolated_imports") @@ -74,6 +74,48 @@ def test_a_world_option_naming_a_module_that_does_not_import_is_sh501() -> None: assert raised.value.code == "SH501" assert "not_a_module_anyone_has" in str(raised.value) assert "Traceback" not in str(raised.value) + assert raised.value.fix is not None and "--world" in raised.value.fix + + +@pytest.mark.parametrize( + ("statement", "raised_as"), + [ + ("def broken(:", "SyntaxError"), + ("undefined_name", "NameError"), + ("import not_installed_anywhere_x", "ModuleNotFoundError"), + ], + ids=["syntax error", "name error", "missing dependency"], +) +def test_an_error_in_world_code_names_its_file_and_line( + tmp_path: Path, statement: str, raised_as: str +) -> None: + """The place the import failed, not the export a package with no world is missing. + + A missing dependency is a `ModuleNotFoundError` too, and is placed at the + import that needs it rather than read as the world module being missing. + """ + root, line = broken_tidy(tmp_path, statement) + with pytest.raises(CliError) as raised: + discover(None, root) + error = raised.value + assert error.code == "SH501" + assert (error.path, error.line) == ((root / BROKEN_MODULE).resolve(), line) + assert raised_as in error.message + assert f"{BROKEN_MODULE}:{line}" in error.message + assert error.fix == f"fix the error at {BROKEN_MODULE}:{line}" + assert "world = seahaven.World" not in f"{error.message} {error.fix}" + + +def test_a_world_that_refuses_a_tool_at_import_names_the_line(tmp_path: Path) -> None: + """A `SeahavenError` out of world code is placed there, not in Seahaven's frames.""" + root, line = broken_tidy(tmp_path, "world.tool(write_note)") + with pytest.raises(CliError) as raised: + discover(None, root) + error = raised.value + assert error.code == "SH501" + assert "write_note" in error.message + assert (error.path, error.line) == ((root / BROKEN_MODULE).resolve(), line) + assert error.fix == f"fix the error at {BROKEN_MODULE}:{line}" def test_a_package_with_no_world_attribute_names_the_fix() -> None: diff --git a/tests/test_instances.py b/tests/test_instances.py index 01af632..29dd01b 100644 --- a/tests/test_instances.py +++ b/tests/test_instances.py @@ -233,6 +233,19 @@ def test_an_instance_from_a_fixture_is_a_writable_copy_at_the_fixtures_clock(wor assert (world.fixtures_dir / fixture / STATE_NAME).stat().st_mode & 0o222 == 0 +def test_every_node_of_a_fixture_instance_opens_without_durability(tmp_path: Path) -> None: + """A fixture's copies are the instance's throwaway files: WAL, and never fsynced.""" + host = composable_world("host", fixtures_dir=tmp_path / "fixtures", work_dir=tmp_path / "work") + host.add_world(composable_world("child"), name="child") + with host.instance(None, now=INSTANT_ISO, clock_mode="fixed") as origin: + origin.freeze("start", "Empty.") + + with host.instance("start") as live, live.bulk() as ctx: + for db in (ctx.db, ctx.worlds.child.db): + assert db.conn.pragma("synchronous") == 0 + assert db.conn.pragma("journal_mode") == "wal" + + @pytest.mark.parametrize("bad", ["", ".", "..", "sub/start", "/start", ".hidden"]) def test_a_fixture_id_that_is_not_a_directory_name_is_refused(world: World, bad: str) -> None: """The id rule is applied before the filesystem is touched: an id cannot become a path.""" @@ -1086,6 +1099,228 @@ def test_the_gate_is_released_after_a_call_that_raised(tmp_path: Path) -> None: assert instance.call("now")["python"] == instance.clock.iso() +class TimedGate(instances._Gate): + """A gate that fails the test rather than hang it when a thread queues too long. + + A thread queued for the gate while it holds an instance lock waits for ever, + and so does the `destroy()` at the end of the test's `with world.instance()`. + """ + + def __init__(self, size: int = 1) -> None: + super().__init__(size) + self.taken = 0 + self.contender_has_slot = threading.Event() + + def acquire(self, blocking: bool = True, timeout: float | None = None) -> bool: + if not super().acquire(timeout=WAIT): + raise AssertionError("queued for the gate while holding an instance lock") + self.taken += 1 + if threading.current_thread().name == "contender": + self.contender_has_slot.set() + return True + + +def timed_gate(monkeypatch: pytest.MonkeyPatch, size: int = 1) -> TimedGate: + installed = TimedGate(size) + monkeypatch.setattr(instances, "_gate", installed) + return installed + + +def on_a_fresh_thread(body: Callable[[], Any], name: str | None = None) -> Caller: + """Start `body` on another thread, whose failure is the test's failure.""" + caller = Caller(body) + if name is not None: + caller.name = name + caller.start() + return caller + + +def note_ids(instance: Instance) -> list[str]: + return [row["id"] for row in instance.inspect().rows("SELECT id FROM notes ORDER BY id")] + + +def register_holder(world: World) -> tuple[threading.Event, threading.Event]: + """A `hold` tool that keeps its gate slot until the test lets go.""" + inside = threading.Event() + release = threading.Event() + + @world.tool + def hold(ctx: Ctx) -> dict[str, bool]: + """Hold a slot of the gate until the test lets go.""" + inside.set() + assert release.wait(WAIT) + return {"done": True} + + return inside, release + + +@pytest.mark.parametrize("target", ["same", "other"]) +def test_a_call_inside_bulk_does_not_queue_behind_a_caller_waiting_for_the_lock( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, target: str +) -> None: + world = build_world(tmp_path) + gate = timed_gate(monkeypatch) + + with world.instance(None) as x, world.instance(None) as y: + written = x if target == "same" else y + + def author() -> None: + with x.bulk(): + contender = on_a_fresh_thread(lambda: add(x, "contender"), name="contender") + # The contender holds the only slot and waits for the lock this block holds. + assert gate.contender_has_slot.wait(WAIT) + add(written, "author") + contender.finish() + + on_a_fresh_thread(author).finish(2 * WAIT) + + assert "contender" in note_ids(x) + assert "author" in note_ids(written) + + +def test_a_tool_calling_another_instance_does_not_wait_for_its_own_slot( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + world = build_world(tmp_path) + captured: dict[str, Instance] = {} + + @world.tool + def relay(ctx: Ctx) -> dict[str, bool]: + """Write to another instance from inside a call.""" + add(captured["y"], "relayed") + return {"done": True} + + timed_gate(monkeypatch) + with world.instance(None) as x, world.instance(None) as y: + captured["y"] = y + result: dict[str, Any] = {} + on_a_fresh_thread(lambda: result.update(x.call("relay"))).finish(2 * WAIT) + + assert result == {"done": True} + assert note_ids(y) == ["relayed"] + + +def test_nested_bulk_blocks_take_no_slot(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + world = build_world(tmp_path) + gate = timed_gate(monkeypatch) + + with world.instance(None) as x, world.instance(None) as y: + + def author() -> None: + with x.bulk(): + add(x, "outer") + with x.bulk(): + add(x, "inner") + with y.bulk(): + add(y, "other") + assert gate.taken == 0 + add(x, "after") + assert gate.taken == 1 + + on_a_fresh_thread(author).finish(2 * WAIT) + + assert note_ids(x) == ["after", "inner", "outer"] + assert note_ids(y) == ["other"] + + +def test_bulk_that_raised_leaves_the_thread_gated_again( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + world = build_world(tmp_path) + gate = timed_gate(monkeypatch) + + with world.instance(None) as x: + + def author() -> None: + with pytest.raises(Boom), x.bulk(): + add(x, "rolled back") + raise Boom("the block failed") + assert gate.taken == 0 + add(x, "after") + assert gate.taken == 1 + + on_a_fresh_thread(author).finish(2 * WAIT) + + assert note_ids(x) == ["after"] + + +def test_bulk_does_not_wait_for_a_full_gate( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + world = build_world(tmp_path) + inside, release = register_holder(world) + timed_gate(monkeypatch) + + with world.instance(None) as x, world.instance(None) as y: + holder = on_a_fresh_thread(lambda: x.call("hold")) + assert inside.wait(WAIT) + + def author() -> None: + with y.bulk() as ctx: + ctx.db.execute("INSERT INTO notes VALUES ('bulk', 'a body', 0)") + + started = time.perf_counter() + on_a_fresh_thread(author).finish() + assert time.perf_counter() - started < WAIT / 5 + + release.set() + holder.finish() + assert note_ids(y) == ["bulk"] + + +def test_a_resize_inside_bulk_does_not_make_its_calls_queue( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + world = build_world(tmp_path) + inside, release = register_holder(world) + set_concurrency(0) + + with world.instance(None) as x, world.instance(None) as y: + + def author() -> None: + with x.bulk(): + # A gate put in place after the block opened, and full. + timed_gate(monkeypatch) + holder = on_a_fresh_thread(lambda: y.call("hold")) + assert inside.wait(WAIT) + add(x, "author") + release.set() + holder.finish() + + on_a_fresh_thread(author).finish(2 * WAIT) + + assert note_ids(x) == ["author"] + + +def test_a_control_tool_calling_its_instance_passes_a_full_gate( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + world = build_world(tmp_path) + inside, release = register_holder(world) + + def stamp(live: Instance, ctx: Ctx) -> dict[str, bool]: + """Write through the instance's own tools.""" + add(live, "stamped") + return {"stamped": True} + + world.tool(control_tool(stamp)) + timed_gate(monkeypatch) + + with world.instance(None) as x, world.instance(None, control_tools=True) as y: + holder = on_a_fresh_thread(lambda: x.call("hold")) + assert inside.wait(WAIT) + + result: dict[str, Any] = {} + started = time.perf_counter() + on_a_fresh_thread(lambda: result.update(y.call("stamp"))).finish(2 * WAIT) + assert time.perf_counter() - started < WAIT / 5 + + release.set() + holder.finish() + assert result == {"stamped": True} + assert note_ids(y) == ["stamped"] + + def test_a_negative_concurrency_is_refused() -> None: with pytest.raises(WorldBug, match="must not be negative"): set_concurrency(-1) diff --git a/tests/test_pytest_plugin.py b/tests/test_pytest_plugin.py index d0c7d6d..c3b68ee 100644 --- a/tests/test_pytest_plugin.py +++ b/tests/test_pytest_plugin.py @@ -590,6 +590,27 @@ def test_it(world): assert "Traceback" not in result.stdout.str() +def test_a_world_that_does_not_import_fails_with_its_file_and_line( + pytester: pytest.Pytester, +) -> None: + root = write_world(pytester) + module = root / "src" / "tinyworld" / "__init__.py" + module.write_text(module.read_text(encoding="utf-8") + "\nundefined_name\n", encoding="utf-8") + pytester.makepyfile( + """ + import pytest + + @pytest.mark.seahaven(fixture=None) + def test_it(instance): + assert instance is not None + """ + ) + result = pytester.runpytest() + result.assert_outcomes(errors=1) + result.stdout.fnmatch_lines(["*NameError*src/tinyworld/__init__.py:*"]) + assert "must export" not in result.stdout.str() + + def test_the_marker_is_registered(pytester: pytest.Pytester) -> None: """`--strict-markers` is what a world's CI runs; an unregistered marker fails it.""" write_world(pytester) diff --git a/tests/test_serve.py b/tests/test_serve.py index 14dd2ec..c58cccd 100644 --- a/tests/test_serve.py +++ b/tests/test_serve.py @@ -81,16 +81,16 @@ def test_serve_runs_one_worker_on_an_app_object(world: World, served: dict[str, assert callable(served["app"]) # The literals, not the constants: comparing a default against itself would # pass whatever the default became, and "which interface" is the decision. - assert (served["kwargs"]["host"], served["kwargs"]["port"]) == ("0.0.0.0", 8000) - assert (DEFAULT_HOST, DEFAULT_PORT) == ("0.0.0.0", 8000) + assert (served["kwargs"]["host"], served["kwargs"]["port"]) == ("127.0.0.1", 8000) + assert (DEFAULT_HOST, DEFAULT_PORT) == ("127.0.0.1", 8000) # An operator watching a training run reads uvicorn's request log; a quieter # default would be a decision, and it is not the one that was made. assert served["kwargs"]["log_level"] == "info" def test_serve_binds_where_it_is_told(world: World, served: dict[str, Any]) -> None: - serve(world, host="127.0.0.1", port=9123) - assert (served["kwargs"]["host"], served["kwargs"]["port"]) == ("127.0.0.1", 9123) + serve(world, host="0.0.0.0", port=9123) + assert (served["kwargs"]["host"], served["kwargs"]["port"]) == ("0.0.0.0", 9123) def test_serve_sizes_the_gate_before_the_server_starts( @@ -122,9 +122,9 @@ def test_serve_passes_the_operators_session_numbers_to_the_app( world: World, served: dict[str, Any] ) -> None: """The app is built from `serve`'s arguments, not from the module defaults.""" - serve(world, max_concurrent_envs=7, session_timeout=None) + serve(world, max_concurrent_envs=7, session_timeout=60.0) assert served["options"]["max_concurrent_envs"] == 7 - assert served["options"]["session_timeout"] is None + assert served["options"]["session_timeout"] == 60.0 def test_serve_passes_the_module_defaults_when_it_is_told_nothing( @@ -134,10 +134,10 @@ def test_serve_passes_the_module_defaults_when_it_is_told_nothing( assert served["options"] == { "include_control_tools": False, "max_concurrent_envs": 500, - "session_timeout": 3600.0, + "session_timeout": None, "console": True, } - assert (DEFAULT_MAX_CONCURRENT_ENVS, DEFAULT_SESSION_TIMEOUT) == (500, 3600.0) + assert (DEFAULT_MAX_CONCURRENT_ENVS, DEFAULT_SESSION_TIMEOUT) == (500, None) def test_serve_can_expose_the_control_tools(world: World, served: dict[str, Any]) -> None: @@ -151,9 +151,9 @@ def test_serve_announces_the_console_at_an_address_a_browser_can_open( ) -> None: """The line an operator reads after `seahaven serve`. - The default bind is `0.0.0.0`, which is not an address, so the message names - loopback. `console_url` owns that translation and is tested against every - spelling in `test_server.py`. + The default bind is loopback. A bind to `0.0.0.0` is not an address a browser + can open, so the message names loopback for it too: `console_url` owns that + translation and is tested against every spelling in `test_server.py`. `capsys` and not `caplog`: the line is a `print`, because until `uvicorn.run` configures logging the root logger has no handler and sits at WARNING, and a @@ -165,6 +165,11 @@ def test_serve_announces_the_console_at_an_address_a_browser_can_open( capsys.readouterr().out == "Starting. Web console will be available at http://127.0.0.1:8000/console\n" ) + serve(world, host="0.0.0.0") + assert ( + capsys.readouterr().out + == "Starting. Web console will be available at http://127.0.0.1:8000/console\n" + ) serve(world, host="127.0.0.1", port=8001) assert ( capsys.readouterr().out diff --git a/tests/test_server.py b/tests/test_server.py index 342d183..609368c 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -48,6 +48,7 @@ from fastapi import WebSocket from fastapi.routing import APIWebSocketRoute from openenv import GenericEnvClient +from openenv.core.env_server.http_server import HTTPEnvServer from openenv.core.env_server.mcp_types import ( CallToolAction, CallToolObservation, @@ -1362,14 +1363,46 @@ def test_the_announced_console_address_is_one_a_browser_can_open(host: str, expe # --- the idle reaper ------------------------------------------------------- -def test_an_idle_session_is_reaped_and_its_instance_destroyed(world: World, tmp_path: Path) -> None: - """`session_timeout` was pinned as a number passed through, never as behaviour. +@pytest.mark.parametrize( + ("options", "reaps"), + [({}, False), ({"session_timeout": 60.0}, True)], + ids=["default", "opt-in"], +) +def test_the_reaper_runs_only_when_the_operator_asks_for_one( + world: World, monkeypatch: pytest.MonkeyPatch, options: dict[str, Any], reaps: bool +) -> None: + """No `session_timeout` means no reaper task in the running server. + + The reaper would destroy the instance of a client that is still connected + and only reading `state`, because OpenEnv counts only `reset` and `step` as + activity, so it is off unless asked for. What is read is the task OpenEnv + starts at lifespan startup, not the number passed to it; the opt-in case + proves the probe sees a reaper when there is one. + """ + servers: list[HTTPEnvServer] = [] + start = HTTPEnvServer._start_reaper + + def recording(server: HTTPEnvServer) -> None: + servers.append(server) + start(server) + + monkeypatch.setattr(HTTPEnvServer, "_start_reaper", recording) + with serving(world, **options) as url, SeahavenClient(base_url=url) as env: + env.reset() + (server,) = servers + assert (server._reaper_task is not None) is reaps + + +def test_an_opted_in_reaper_destroys_a_connected_idle_sessions_instance( + world: World, tmp_path: Path +) -> None: + """`--session-timeout N` reaps, and the instance really goes. - It is the only thing standing between a long-lived server and a disk full of - abandoned instances, and OpenEnv's cleanup swallows every exception - `env.close()` raises -- so an `Instance.destroy()` that started failing would - leak a fixture copy and an APSW connection per session with nothing red - anywhere. This test asks the filesystem instead. + OpenEnv's cleanup swallows every exception `env.close()` raises -- so an + `Instance.destroy()` that started failing would leak a fixture copy and an + APSW connection per reaped session with nothing red anywhere. This test asks + the filesystem instead. The client stays connected throughout: the reaper + does not wait for a disconnect, only for `reset` and `step` to stop. The reaper wakes every `max(timeout / 4, 5.0)` seconds, so the deadline is generous and the assertion is only that the directory goes. diff --git a/tests/test_tool.py b/tests/test_tool.py index cde4451..075106b 100644 --- a/tests/test_tool.py +++ b/tests/test_tool.py @@ -1,27 +1,56 @@ """Building a tool from a function: what is accepted, what is refused, and what reaches the wire.""" +import decimal +import json +import threading +import uuid from dataclasses import FrozenInstanceError, dataclass, replace -from datetime import date, datetime, time -from enum import Enum +from datetime import UTC, date, datetime, time +from enum import Enum, StrEnum +from ipaddress import IPv4Address +from pathlib import Path from typing import TYPE_CHECKING, Annotated, Any, Literal, NewType, TypedDict +import pydantic import pytest -from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel, ConfigDict, Field, with_config from seahaven.ctx import Ctx from seahaven.errors import ArgumentError, WorldBug from seahaven.handles import WorldHandle, Worlds from seahaven.tool import Tool, _blame, _build_schema +from tests.conftest import build_world if TYPE_CHECKING: # deliberately not importable at runtime: see the refusal below from decimal import Decimal class Point(BaseModel): + model_config = ConfigDict(strict=True) + x: int y: int +class Lax(BaseModel): + n: int + + +Loose = NewType("Loose", Lax) + + +class Relaxed(BaseModel): + model_config = ConfigDict(strict=False) + + n: int + + +@dataclass +class Span: + start: int + end: int + + class Window(TypedDict): start: str end: str @@ -31,6 +60,50 @@ class Colour(Enum): RED = "red" +class Shade(StrEnum): + DARK = "dark" + + +class Palette(TypedDict): + colour: Literal[Colour.RED] + + +class Booking(TypedDict): + room: str + at: datetime + + +@dataclass +class Holder: + inner: Lax + + +class Holding(TypedDict): + inner: Lax + + +@pydantic.dataclasses.dataclass +class LaxPydanticDataclass: + n: int + + +@with_config(ConfigDict(str_strip_whitespace=True)) +class ConfiguredTypedDict(TypedDict): + n: int + + +class Wrapper(BaseModel): + model_config = ConfigDict(strict=True) + + inner: Lax + + +class Branch(BaseModel): + model_config = ConfigDict(strict=True) + + children: list[Branch] + + # Two names for a type that carry the type they stand for, and publish its schema. type Timestamp = datetime Stamp = NewType("Stamp", datetime) @@ -104,46 +177,316 @@ def page(ctx: Ctx, limit: Annotated[int, Field(ge=1, le=50, description="how man tool.validate({"limit": 99}) +STAMP = "2026-06-01T09:00:00Z" +AT = datetime(2026, 6, 1, 9, tzinfo=UTC) +UID = uuid.UUID(int=5) + +# Each annotation, a value as parsed JSON spells it, and what the tool receives. +JSON_SPELLINGS: list[tuple[Any, Any, Any]] = [ + (tuple[int, int], [1, 2], (1, 2)), + (uuid.UUID, str(UID), UID), + (decimal.Decimal, "1.50", decimal.Decimal("1.50")), + (decimal.Decimal, 1.5, decimal.Decimal("1.5")), + (set[int], [1, 2], {1, 2}), + (frozenset[str], ["a"], frozenset({"a"})), + (bytes, "abc", b"abc"), + (Path, "/srv/data", Path("/srv/data")), + (IPv4Address, "10.0.0.1", IPv4Address("10.0.0.1")), + (Span, {"start": 1, "end": 2}, Span(1, 2)), + (datetime, STAMP, AT), + (date, "2026-06-01", date(2026, 6, 1)), + (time, "09:00:00", time(9)), + (Colour, "red", Colour.RED), + (Shade, "dark", Shade.DARK), + (Literal[Shade.DARK], "dark", Shade.DARK), + (Annotated[datetime, Field()], STAMP, AT), + (datetime | None, None, None), + (Timestamp, STAMP, AT), + (list[Stamp], [STAMP], [AT]), + (Booking, {"room": "r1", "at": STAMP}, {"room": "r1", "at": AT}), +] + + +@pytest.mark.parametrize(("annotation", "sent", "received"), JSON_SPELLINGS) +def test_an_argument_is_accepted_as_parsed_json_spells_it( + annotation: Any, sent: Any, received: Any +) -> None: + """What the wire hands over is parsed JSON, and the published schema describes that.""" + + def at(ctx: Ctx, value: annotation) -> dict: + """At.""" + return {} + + tool = build(at) + + assert tool.validate(json.loads(json.dumps({"value": sent}))) == {"value": received} + + +@pytest.mark.parametrize(("annotation", "sent", "received"), JSON_SPELLINGS) +def test_an_in_process_caller_may_hand_over_the_python_object( + annotation: Any, sent: Any, received: Any +) -> None: + def at(ctx: Ctx, value: annotation) -> dict: + """At.""" + return {} + + assert build(at).validate({"value": received}) == {"value": received} + + +def test_an_in_process_object_keeps_its_identity() -> None: + """A value strict Python validation accepts is handed over as it was, not rebuilt.""" + + def place(ctx: Ctx, origin: Point, raw: str | bytes) -> dict: + """Place.""" + return {} + + origin = Point(x=1, y=2) + + validated = build(place).validate({"origin": origin, "raw": b"\x00\xff"}) + + assert validated["origin"] is origin + assert validated["raw"] == b"\x00\xff" + + +def test_a_call_holding_a_python_object_is_validated_as_python_throughout() -> None: + """The mode is the whole call's: beside a real `UUID`, a list is not a `tuple`.""" + + def book(ctx: Ctx, id: uuid.UUID, span: tuple[int, int]) -> dict: + """Book.""" + return {} + + tool = build(book) + + assert tool.validate({"id": UID, "span": (1, 2)}) == {"id": UID, "span": (1, 2)} + with pytest.raises(ArgumentError) as raised: + tool.validate({"id": UID, "span": [1, 2]}) + + assert [violation["path"] for violation in raised.value.violations] == ["span"] + + +@pytest.mark.parametrize( + ("annotation", "sent"), + [ + (str, AT), + (str, UID), + (str, decimal.Decimal("1.5")), + (str, b"abc"), + (str, Colour.RED), + (dict[str, int], Point(x=1, y=2)), + (list[int], (1, 2)), + ], +) +def test_a_python_object_is_not_rendered_to_fit_another_annotation( + annotation: Any, sent: Any +) -> None: + """Its JSON form would fit, and strict Python validation refuses it all the same.""" + + def at(ctx: Ctx, value: annotation) -> dict: + """At.""" + return {} + + with pytest.raises(ArgumentError): + build(at).validate({"value": sent}) + + +@pytest.mark.parametrize( + ("annotation", "sent", "received"), + [ + (datetime | str, STAMP, AT), + (str | datetime, STAMP, STAMP), + (uuid.UUID | str, str(UID), UID), + ], +) +def test_a_union_of_types_one_json_string_fits_takes_the_member_pydantic_picks( + annotation: Any, sent: Any, received: Any +) -> None: + """Pinned rather than chosen: pydantic's smart union decides, and order can matter.""" + + def at(ctx: Ctx, value: annotation) -> dict: + """At.""" + return {} + + assert build(at).validate({"value": sent}) == {"value": received} + + +@pytest.mark.parametrize( + ("annotation", "sent"), + [ + (int, "5"), + (bool, "true"), + (float, "1.5"), + (tuple[int, int], ["1", "2"]), + (uuid.UUID, 5), + (datetime, 1_700_000_000), + (datetime, "2026-06-01"), + (Colour, "RED"), + (set[int], ["1"]), + (Span, {"start": "1", "end": 2}), + (Booking, {"room": 1, "at": STAMP}), + (Point, {"x": "1", "y": 2}), + ], +) +def test_the_json_spelling_is_held_strictly_at_every_depth(annotation: Any, sent: Any) -> None: + def at(ctx: Ctx, value: annotation) -> dict: + """At.""" + return {} + + with pytest.raises(ArgumentError) as raised: + build(at).validate({"value": sent}) + + assert raised.value.violations[0]["path"].startswith("value") + + +def test_a_value_with_no_json_form_is_refused_as_an_argument() -> None: + """Only an in-process caller can send one, and the refusal is Python validation's.""" + + def count(ctx: Ctx, n: int) -> dict: + """Count.""" + return {} + + with pytest.raises(ArgumentError) as raised: + build(count).validate({"n": threading.Lock()}) + + assert [violation["path"] for violation in raised.value.violations] == ["n"] + + @pytest.mark.parametrize( "annotation", [ - datetime, - date, - time, - Colour, - Annotated[datetime, Field()], - datetime | None, - list[date], - # The member, not the class: a `Literal` of enum members publishes - # `"const": "red"` and then accepts only `Colour.RED`. - Literal[Colour.RED], - Timestamp, - Stamp, - list[Stamp], + Lax, + Lax | None, + list[Lax], + Annotated[Lax, Field(description="lax")], + Loose, + Wrapper, + Holder, + Holding, + LaxPydanticDataclass, + ConfiguredTypedDict, ], ) -def test_timestamps_and_enums_are_refused_wherever_they_appear(annotation: Any) -> None: - def at(ctx: Ctx, when: annotation) -> dict: +def test_a_nested_type_with_its_own_lax_config_is_refused(annotation: Any) -> None: + """A type with a pydantic config of its own validates under it, not the tool's.""" + + def at(ctx: Ctx, value: annotation) -> dict: """At.""" return {} message = refusal(at) - assert "when" in message - assert "use `str` with a pattern or `Literal`" in message + assert message.startswith("tool 'at': argument 'value' uses ") + assert "`strict=True`" in message -def test_the_refusal_stops_at_the_tools_own_parameters() -> None: - """A nested model is the world's own declaration, and pydantic answers for it.""" +@pytest.mark.parametrize( + "annotation", [Literal[Colour.RED], Literal[b"red"], list[Literal["a", Colour.RED]], Palette] +) +def test_a_literal_json_cannot_send_is_refused(annotation: Any) -> None: + """The schema says `"const": "red"`, and validation would accept only the member.""" - class Moment(BaseModel): - at: datetime + def at(ctx: Ctx, value: annotation) -> dict: + """At.""" + return {} + + message = refusal(at) + + assert message.startswith("tool 'at': argument 'value' is a `Literal` of ") + assert "JSON cannot send" in message + + +def test_a_nested_model_that_chose_lax_input_gets_it() -> None: + def at(ctx: Ctx, value: Relaxed) -> dict: + """At.""" + return {} + + assert build(at).validate({"value": {"n": "5"}}) == {"value": Relaxed(n=5)} - def schedule(ctx: Ctx, moment: Moment) -> dict: - """Schedule.""" + +def test_a_model_defined_in_terms_of_itself_still_registers() -> None: + def grow(ctx: Ctx, root: Branch) -> dict: + """Grow.""" return {} - assert "moment" in build(schedule).schema["properties"] + tool = build(grow) + + assert tool.validate({"root": {"children": [{"children": []}]}}) == { + "root": Branch(children=[Branch(children=[])]) + } + + +def test_json_spellings_reach_a_tool_called_by_name(tmp_path: Path) -> None: + """The real entry point: the arguments an agent sends, through `world.instance(...)`.""" + world = build_world(tmp_path) + + received: list[tuple[Any, ...]] = [] + + @world.tool + def reserve( + ctx: Ctx, + span: tuple[int, int], + id: uuid.UUID, + amount: decimal.Decimal, + tags: set[str], + where: Span, + at: datetime, + colour: Colour, + ) -> dict[str, bool]: + """Reserve.""" + received.append((span, id, amount, tags, where, at, colour)) + return {"reserved": True} + + sent = json.loads( + json.dumps( + { + "span": [1, 2], + "id": str(UID), + "amount": "9.99", + "tags": ["a"], + "where": {"start": 1, "end": 2}, + "at": STAMP, + "colour": "red", + } + ) + ) + + with world.instance(None) as instance: + assert instance.call("reserve", **sent) == {"reserved": True} + with pytest.raises(ArgumentError) as raised: + instance.call("reserve", **{**sent, "span": ["1", "2"], "colour": "RED"}) + + assert received == [((1, 2), UID, decimal.Decimal("9.99"), {"a"}, Span(1, 2), AT, Colour.RED)] + assert {violation["path"] for violation in raised.value.violations} == { + "span.0", + "span.1", + "colour", + } + + +def test_a_lone_surrogate_json_can_carry_reaches_a_tool_called_by_name(tmp_path: Path) -> None: + """Parsed JSON with no UTF-8 text: validated as the `str` it is, not an internal error.""" + world = build_world(tmp_path) + + @world.tool + def echo(ctx: Ctx, s: str) -> dict[str, str]: + """Echo.""" + return {"s": s} + + with world.instance(None) as instance: + assert instance.call("echo", **json.loads('{"s": "a\\ud800"}')) == {"s": "a\ud800"} + with pytest.raises(ArgumentError): + instance.call("echo", **json.loads('{"s": ["a\\ud800"]}')) + + +def test_deeply_nested_json_reaches_a_tool_called_by_name(tmp_path: Path) -> None: + world = build_world(tmp_path) + + @world.tool + def keep(ctx: Ctx, value: Any) -> dict[str, bool]: + """Keep.""" + return {"kept": True} + + with world.instance(None) as instance: + assert instance.call("keep", value=json.loads("[" * 600 + "]" * 600)) == {"kept": True} def test_defaults_are_published_and_required_arguments_are_not() -> None: diff --git a/tests/test_typed_call.py b/tests/test_typed_call.py index 18a67ea..49dafe5 100644 --- a/tests/test_typed_call.py +++ b/tests/test_typed_call.py @@ -327,6 +327,33 @@ def reach(ctx: Ctx, value: str) -> list[str]: assert live.call("leaf_read") == ["deep"] +def test_a_handle_reaches_a_function_of_a_node_two_routes_beneath_it_reach( + tmp_path: Path, +) -> None: + """One store reached twice under one handle is one tool: the reference is not ambiguous.""" + host, _write, _read = rooted("host", tmp_path) + middle, _middle_write, _middle_read = rowed("middle") + left, _left_write, _left_read = rowed("left") + right, _right_write, _right_read = rowed("right") + leaf, leaf_write, leaf_read = rowed("leaf") + left.add_world(leaf, name="leaf") + right.add_world(leaf, name="leaf", tool_prefix="r_") + middle.add_world(left, name="left") + middle.add_world(right, name="right") + host.add_world(middle, name="middle") + + @host.tool + def reach(ctx: Ctx, value: str) -> list[str]: + """Reach the shared grandchild's store through the child, by reference.""" + ctx.worlds.middle.call(leaf_write, value=value) + return ctx.worlds.middle.call(leaf_read) + + with host.instance(None) as live: + assert live.call("reach", value="once") == ["once"] + assert live.call("leaf_read") == ["once"] + assert live.call("r_leaf_read") == ["once"] + + def test_a_handle_refuses_a_function_no_world_beneath_it_owns(tmp_path: Path) -> None: host, _write, _read = rooted("host", tmp_path) leaf, _leaf_write, _leaf_read = rowed("leaf") diff --git a/worlds/projecttracker/AGENTS.md b/worlds/projecttracker/AGENTS.md index dc30d0e..fcea0df 100644 --- a/worlds/projecttracker/AGENTS.md +++ b/worlds/projecttracker/AGENTS.md @@ -34,10 +34,13 @@ applies here too. tools would have. - **The viewer.** `world.instance(fixture, startup={"user_id": ...})`, and the same `startup=` on `reset` over OpenEnv, names who the session is driving the tracker as; `startup.py` puts it in - `ctx.state["viewer_id"]` and falls back to the workspace's first admin. Every write takes an - optional `actor_id`, which wins, and a write with neither is + `ctx.state["viewer_id"]` and falls back to the workspace's first admin. The five issue writes the + trail records (`create_issue`, `update_issue`, `assign_issue`, `transition_issue` and + `add_comment`) take an optional `actor_id`, which wins, and one of them with neither is `InvalidInput("actor_id", "no actor")` — which is what a blank instance and `empty` do, having no admin to fall back to. +- **The audit trail** does not record `title`, `description`, `priority` or `due_at` changes, + labels, or archiving; grade those on the row. - **Order within one episode.** Rows a single episode writes can share a `created_at`: every row on a `fixed` clock, the rows of one call on `tick`, and rows written in the same millisecond on the default `running` clock. The timestamp is not a complete order. Two places feel it. Reading @@ -48,10 +51,10 @@ applies here too. so it orders by `(created_at, id)` and says so: oldest first, id order among comments that share an instant. Do not write an eval that grades on the order of comments an agent added; grade on the rows. -- **The closed-issue rule.** A `done` or `canceled` issue has no assignee: transitioning to one - drops the assignee and records the drop, and assigning a closed issue is a `CONFLICT`. An eval may - rely on `status IN ('done','canceled') AND assignee_id IS NOT NULL` being a state this world - cannot reach. +- **The closed-issue rule.** Transitioning an issue to `done` or `canceled` drops its assignee and + records the drop, and `assign_issue` on a closed issue is a `CONFLICT`. `update_issue` checks only + the `status` it is given: a call that sets `assignee_id` and leaves `status` out will assign a + closed issue. Do not write an eval that relies on a closed issue never having an assignee. - **Archiving** stamps `issues.archived_at` and takes the issue out of `list_issues`. It stays in the database and inside `get_issue` and `search_issues`, and no field of it changes again: all four tools that write to an issue — `update_issue`, `assign_issue`, `transition_issue` and diff --git a/worlds/projecttracker/README.md b/worlds/projecttracker/README.md index e1aa71e..e8b8d32 100644 --- a/worlds/projecttracker/README.md +++ b/worlds/projecttracker/README.md @@ -4,8 +4,8 @@ A Seahaven world: a fictional issue tracker for a fictional company, and the ref framework is developed against. Nothing here mimics a real product's names, schema or error text. Users belong to teams, teams own projects, projects hold issues, issues carry labels and comments, -and every write to an issue appends to an audit trail. Keys are product-shaped (`ENG-41`), lists are -cursor-paginated, and search is SQLite's FTS5. +and an audit trail records who created each issue, changed its status or assignee, or commented on +it. Keys are product-shaped (`ENG-41`), lists are cursor-paginated, and search is SQLite's FTS5. ## What it has @@ -71,8 +71,9 @@ Tests: `uv run pytest worlds/projecttracker` from the repository root, or `uv ru (`2026-06-01T09:00:00.000Z`), written from `ctx.clock.iso()` and never from the wall clock. - **Issue keys** are `TEAMKEY-n`, minted from a counter on the team: `ENG-41` is the forty-first issue team `ENG` ever had, in whichever of its projects. -- **A closed issue has no assignee.** Moving an issue to `done` or `canceled` drops its assignee, - and assigning a closed issue is refused. +- **Closing an issue drops its assignee.** Moving an issue to `done` or `canceled` drops its + assignee, and `assign_issue` refuses a closed issue. `update_issue` with an `assignee_id` and no + `status` does not check the current status, so it can assign a closed issue. - **An archived issue keeps its fields and leaves `list_issues`** — no tool will change one again, though `get_issue` and `search_issues` still find it. It can still be commented on: archiving freezes the issue, not the conversation about it. diff --git a/worlds/projecttracker/src/projecttracker/startup.py b/worlds/projecttracker/src/projecttracker/startup.py index ffd30fd..0a94c80 100644 --- a/worlds/projecttracker/src/projecttracker/startup.py +++ b/worlds/projecttracker/src/projecttracker/startup.py @@ -4,8 +4,8 @@ attributed to that person without the caller saying so. This world spells that as a startup hook: `world.instance("agency", startup={"user_id": ...})` -- or `reset(startup={"user_id": ...})` over OpenEnv -- names the person the session is -driving the tracker as, and every tool that writes falls back to them when the -call passed no `actor_id`. +driving the tracker as, and each issue write that takes an `actor_id` falls back +to them when the call passed none. `actor_id` on the call is still there, and wins, because an eval that wants two people writing in one episode should not need two instances to do it. @@ -14,9 +14,9 @@ "the viewer is the admin" true of the fixtures that have one (`components/projecttracker.md` §4) without an eval having to look a generated id up first. A workspace with no admin -- `empty`, or a fresh blank instance -- has no -viewer, and a write there must name its `actor_id` or be refused. That refusal is -`InvalidInput("actor_id", "no actor")`, raised in `tools/_rows.py` where the -fallback is read. +viewer, and one of those writes there must name its `actor_id` or be refused. That +refusal is `InvalidInput("actor_id", "no actor")`, raised in `tools/_rows.py` where +the fallback is read. """ import seahaven diff --git a/worlds/projecttracker/src/projecttracker/tools/_events.py b/worlds/projecttracker/src/projecttracker/tools/_events.py index 170bebc..cbe5b29 100644 --- a/worlds/projecttracker/src/projecttracker/tools/_events.py +++ b/worlds/projecttracker/src/projecttracker/tools/_events.py @@ -1,7 +1,8 @@ """`issue_events`: the audit trail, and the one function that appends to it. -Every write to an issue leaves a row here -- created, status changed, assignee -changed, comment added -- and every one of them goes through `record_event`. +Creating an issue, changing its status or its assignee, and adding a comment +each leave a row here, and every one of them goes through `record_event`. Other +field changes, labels and archiving leave none. One function rather than an `INSERT` in each tool, because the trail is what an eval grades an agent's work on: a tool that wrote its own row would eventually write a payload shaped differently from the rest, and the grader would have to diff --git a/worlds/projecttracker/src/projecttracker/tools/issues.py b/worlds/projecttracker/src/projecttracker/tools/issues.py index 9a53437..fce2f82 100644 --- a/worlds/projecttracker/src/projecttracker/tools/issues.py +++ b/worlds/projecttracker/src/projecttracker/tools/issues.py @@ -9,11 +9,12 @@ forty-first issue team `ENG` ever had, whichever project it is in and whatever became of the forty before it. -**A closed issue has no assignee.** Moving an issue to `done` or `canceled` drops -its assignee, and assigning one is refused. The product's reasoning is that an -assignee means "this person is doing this", which a finished issue has nobody -doing; the eval-facing consequence is that `assignee_id IS NOT NULL` and -`status = 'done'` is a state this world cannot be in, and a grader may rely on it. +**Closing an issue drops its assignee.** Moving an issue to `done` or `canceled` +drops its assignee, and `assign_issue` refuses a closed issue. The product's +reasoning is that an assignee means "this person is doing this", which a finished +issue has nobody doing. `update_issue` checks only the `status` it is given, so a +call that sets `assignee_id` without `status` can assign a closed issue: a grader +must not rely on a closed issue never having an assignee. **An archived issue keeps its fields.** `archive_issue` stamps `archived_at` and takes the issue out of `list_issues`. It is still there -- `get_issue` answers