Skip to content

Strict wire validation, safer defaults, and the moderate deep-review fixes - #41

Merged
scosman merged 11 commits into
mainfrom
claude/serene-newton-214g7x
Sep 30, 2026
Merged

Strict wire validation, safer defaults, and the moderate deep-review fixes#41
scosman merged 11 commits into
mainfrom
claude/serene-newton-214g7x

Conversation

@scosman

@scosman scosman commented Sep 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

This PR fixes the moderate findings from the September 2026 full-repo deep review, plus four earlier fixes on the same branch. The review work is planned in specs/projects/cr_moderate_fixes/, where triage.md records the decision on each of the 56 findings.

Behaviour changes

  • seahaven serve binds 127.0.0.1 by default. It used to bind 0.0.0.0. /ws has no authentication, so pass --host 0.0.0.0 to expose the server. The hub Dockerfile already does this, and a test pins it.
  • The OpenEnv idle reaper is off by default. Opt in with --session-timeout N or session_timeout=.
  • Tool arguments are validated as they arrive on the wire. Plain-JSON arguments are validated in strict JSON mode; other arguments stay in strict Python mode. See below.
  • A contributed tool runs the middleware of the route that contributed it. It no longer runs the middleware of the owning node's canonical route. Nothing changes when no node is shared.
  • Startup hooks run hosts first. A node shared by two hosts runs its hooks after both. Nothing changes when no node is shared.

Changes

Tool argument validation (a1680c0)

Each call is validated in one of two strict modes, chosen for the whole call. Arguments that are plain JSON at every depth use strict JSON mode, which accepts the JSON form the schema publishes: tuple, UUID, Decimal, datetime, dataclasses, Enum and so on. Other arguments use strict Python mode. At registration Seahaven now refuses a Literal of a value JSON cannot send. It also refuses a nested type with its own pydantic config that does not set strict.

OpenEnv idle reaper off by default (4b5ddf8)

The reaper could only ever reap a connected client. A harness that only read state lost its instance, because OpenEnv counts only reset and step as activity.

seahaven new scaffold (38c6f3c)

The scaffold now passes its own tests. This adds a dev group with pytest and a fixture snapshot for the recipe test. A world name that clashes with the standard library or seahaven is refused.

ProjectTracker assignee docs (dc29e4b)

The docs no longer promise graders that a closed issue never has an assignee, because update_issue allows one.

Deep-review fixes, phase 1 (1aef546)

  • Instance creation no longer fsyncs (synchronous=OFF before WAL).
  • SeahavenClient.step(ListToolsAction()) returns a ListToolsObservation.
  • An import error in world code is SH501 at the failing file and line, with the fix fix the error at <file:line>.
  • Startup hooks run hosts first (new composition.hosts_first).
  • New tests: call-log argument copies, a typed call through a handle over a diamond, and a check that every relative link and anchor in the docs resolves.

bulk() deadlock (34be711)

A call inside with inst.bulk(): could deadlock against another thread at a gate of 1. gate() is now re-entrant per thread. bulk() marks its thread and takes no slot. Tests use a gate that times out, so a regression fails instead of hanging.

Middleware per contributing route (80d8723)

Contributed carries its own middleware chain, built at the seal once per distinct route. Before this change, adding a second route to a node silently dropped the first route's middleware.

Docs (7db13b7)

  • New reasons for SH103.
  • ProjectTracker's audit-trail and actor_id claims now match what it records.
  • The docs say a composite bulk() is not all-or-nothing when a commit fails, and a new test shows it.
  • History, benchmark figures and spec citations are removed from the bundled docs.

Merge of main (e3f3c96)

There were two conflicts, in state.md and serving_and_openenv.md. In both, main had changed bench/ to tools/bench/ in paragraphs this branch removes, so this branch's side is kept. tests/test_docs.py now link-checks .github/CONTRIBUTING.md, the file's new location on main.

Testing

Every AGENTS.md check passes on the merged head, in both the serve and mcp environments: ruff, the split ty check, the framework, projecttracker and xmlrpc suites, and the licence gates. Each test added in the review phases (1aef546 to 7db13b7) was checked to fail when its fix is reverted. The one intended exception is test_bulk_does_not_wait_for_a_full_gate, a no-change guard.

🤖 Generated with Claude Code

https://claude.ai/code/session_0194LhNcR83VnggdfuMJMpSn

scosman and others added 10 commits September 25, 2026 02:35
@scosman @claude
Stop promising graders that a closed issue never has an assignee
dc29e4b 
update_issue checks only the status it is given, so setting assignee_id
without status assigns an issue that is already done or canceled. The
reference world's README, AGENTS.md, issues.py docstring and the bundled
projecttracker page said that state could not be reached; they now say
what the tools do.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PtthfdcF8q7whsCMK9gKwQ
@scosman @claude
Validate tool arguments as the wire sends them, strictly
a1680c0 
Tool.validate ran pydantic's strict Python mode on every call, so an
argument annotated tuple, UUID, Decimal, set, bytes, Path, ipaddress or
a dataclass registered cleanly, published a normal JSON schema, and then
refused every call arriving as parsed JSON over OpenEnv or MCP, while a
world's typed in-process tests stayed green.

A call is now validated in one of two strict modes, chosen for the
whole call: arguments that are plain JSON at every depth are validated
in strict JSON mode, which accepts the JSON form the schema publishes;
arguments holding any other Python object are validated in strict
Python mode and are never rendered to JSON to fit another annotation.
Plain JSON with no UTF-8 text (a lone surrogate) falls back to Python
mode rather than failing as an internal error.

Registration drops the datetime/date/time/Enum refusal and instead
refuses, at any depth, a Literal of a value JSON cannot send (a plain
Enum member, bytes) and a nested type with its own pydantic config
(BaseModel, pydantic dataclass, @with_config) that does not set
`strict`, so strictness no longer stops at the top level.

Docs (authoring.md, reference/api.md) describe the two modes, union
ambiguity and the nested-strict rule. Tests cover each JSON form, the
refused coercions, unions, the new refusals, and calls by name through
world.instance(...) and SeahavenEnv.step.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PtthfdcF8q7whsCMK9gKwQ
@scosman @claude
Turn the OpenEnv idle reaper off by default
4b5ddf8 
DEFAULT_SESSION_TIMEOUT is now None, so `seahaven serve` and
`seahaven.openenv.app()` start no reaper unless `--session-timeout N`
(or `session_timeout=`) asks for one; `0` still means off.

The old rationale was wrong: OpenEnv's /ws handler destroys a session
when its socket drops, uvicorn's ping closes half-open peers, and
Seahaven refuses openenv/session/create, so the reaper could only ever
reap a client that is still connected. OpenEnv counts only `reset` and
`step` as activity, so a harness only reading `state` lost its instance.

Docs state the new default and that caveat. A new server test checks no
reaper task runs by default and one does on opt-in; the reap test now
names the opt-in it exercises.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PtthfdcF8q7whsCMK9gKwQ
@scosman @claude
Make a plain seahaven new scaffold pass its own tests
38c6f3c 
- Add a `dev` dependency group with `pytest>=8` to the scaffold's
  pyproject, so `uv sync` installs pytest and `uv run pytest` works.
- Snapshot the world's fixtures before the scaffold's recipe test builds,
  and compare against that snapshot, so the test still passes after the
  README's first `build('empty')`.
- Refuse a world name whose package is a standard library module or
  `seahaven`, naming the clash and suggesting a suffix, instead of
  rendering a scaffold that fails its first `seahaven check`.
- Document the name refusal in reference/cli.md.
- Test the dev group, the refusal, and pytest passing after the README
  build; share the subprocess helpers between the scaffold tests.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PtthfdcF8q7whsCMK9gKwQ
@scosman @claude
Start the cr_moderate_fixes spec: overview, triage and functional spec
10fd736 
The triage records the decision on each of the 56 moderate findings from
the full-repo deep review; the 18 marked Fix are this project's scope.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PtthfdcF8q7whsCMK9gKwQ
@scosman @claude
Complete the cr_moderate_fixes spec: architecture, components and plan
ed99ec5 
Four phases for the 17 moderate findings marked Fix: a low-risk batch,
the bulk() deadlock and middleware per contributing route in commits of
their own, then a docs phase. 3.M1 moves to won't-fix. Each component
doc carries a prototyped design with the tests that fail without it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PtthfdcF8q7whsCMK9gKwQ
@scosman @claude
Fix the phase 1 batch of moderate review findings
1aef546 
Eight low-risk fixes from the September 2026 deep review (cr_moderate_fixes
phase 1):

- 11.M1: instance connections set synchronous=OFF before the WAL switch, and
  the blank-instance builder sets it too, so creating an instance no longer
  fsyncs. Instance directories are deleted by the sweep after a crash.
- 7.M2: `seahaven serve` and `seahaven.openenv.serve` bind 127.0.0.1 by
  default; `/ws` has no authentication. The hub Dockerfile already passes
  `--host 0.0.0.0` to uvicorn, and a test pins that. Help text and docs
  updated.
- 5.M3: `SeahavenClient.step(ListToolsAction())` parses as a
  `ListToolsObservation` instead of raising a ValidationError.
- 6.M1: an import that raises in world code is SH501 at the file and line
  that raised it, with the fix "fix the error at <file:line>" and a pointer to
  `python -c 'import <pkg>'`. A missing world module gets a fix naming
  `--world`; the "give the package a world" fix is kept for the no-world case
  only. The pytest plugin prints the same located message.
- 4.M2: startup hooks run hosts first (new `composition.hosts_first`): a node
  shared by two hosts runs after both. Unchanged order when nothing is shared.
- 10.M1, 10.M3: call-log argument-copy tests that fail without the deep copy;
  a typed call by reference through a handle over a diamond.
- 10.M4: tests/test_docs.py checks every relative link and anchor in every
  bundled page, README.md and CONTRIBUTING.md.

Deviations from the spec:
- The spec gave CliError `line` and `fix`; the code also adds `root`, because
  `check` used `error.path` as the project root that paths are shown relative
  to, and `path` now names the failing file.
- The spec said `_finding_for` falls back to the no-world fix text; the code
  falls back to an empty fix, because every SH501 raise now carries its own
  fix and the no-world text should appear in one place only.
- A non-schema SeahavenError at import also gets the location and traceback
  hint appended to its message, not only its path and line, because the
  pytest plugin and `main` print only the message.
- The three find_world error cases are one parametrised test rather than three
  named tests. The optional bench rerun (bench/results/latest.md) was not done.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0194LhNcR83VnggdfuMJMpSn
@scosman @claude
Fix the bulk() deadlock with a per-thread re-entrant gate (2.M1)
34be711 
An inst.call(...) inside `with inst.bulk():` queued for the process-wide
concurrency gate while its thread held the instance lock. A second thread
holding the last slot and waiting for that lock deadlocked both, at a gate of
1 with two threads. Two more hangs had the same cause: a call on another
instance inside bulk(), and a tool calling another instance, which waited for
a second slot while it held the only one.

- instances.gate() is re-entrant per thread through a threading.local flag.
  A ContextVar would not work, because Starlette and asyncio.to_thread copy
  context variables into worker threads. Only the outermost block sets the
  flag, after its acquire returns, and clears it in `finally`. The flag is
  set even when that block takes no slot (bypass, or no gate in place), so a
  mid-block set_concurrency and a control tool calling its own instance do
  not queue. An inner block takes nothing.
- Instance._bulk enters gate(bypass=True) before the instance lock. It marks
  the thread and takes no slot, so HTTP requests, which run inside bulk(),
  stay ungated as http_apis.md states.
- The module docstring's gate rule, the gate() docstring and the gate
  section of serving_and_openenv.md now state the re-entrancy and that
  bulk() bypasses the gate.

Tests (test_instances.py): a TimedGate whose acquire raises after WAIT
instead of hanging. The tests cover:
- a call inside bulk() on the same and on another instance
- a tool calling another instance at a gate of 1
- nested bulk() blocks taking no slot
- a bulk() that raised clearing the flag
- bulk() not waiting for a full gate
- a resize inside bulk()
- a control tool calling its own instance past a full gate

On the old code, 7 of the 8 test cases fail within their timeout. The
exception is the full-gate test, which is the no-change guard. The
component doc's three mutations fail the tests it names.

Deviation: the spec said only test_instances.py changes among the tests;
the code also changes tests/conftest.py, because the session-scoped
`receivers` fixture in test_docs_examples.py holds a bulk() open on the main
thread. That left the main thread marked, so older main-thread gate tests
stopped taking slots and test_a_nested_call_does_not_take_the_gate depended
on test order. The autouse _process_wide_runtime_state fixture now clears
the mark for each test and restores it afterwards.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0194LhNcR83VnggdfuMJMpSn
@scosman @claude
Run each contributed tool's own route's middleware (4.M1)
80d8723 
An agent call to a contributed tool ran the middleware of the owning node's
canonical route. When a node became reachable by a second route, a tool
contributed through the first route silently lost the middleware of the worlds
on that route (shop's middleware stopped running on shop_payments_write once
the company also added payments directly).

The chain now belongs to the entry, not the node:

- `Contributed` gains `chain`: every middleware of every node on the route
  that contributed the entry, outermost first, then `invoke` on the owner.
  Chains are built at the seal, once per distinct route, only for entries
  that reach the root; the cache is seeded with the root's own chain, so every
  root tool's chain is `World.chain`.
- `_Candidate` carries `hosts` (the node keys of the contributing route) in
  place of a prebuilt `Contributed`; `_through` prepends the host key.
- `Node.agent_chain` is removed and `Node.internal_chain` is renamed
  `own_chain`. `_walk` no longer computes `parents`, and `_own_layers` is
  folded into `_route_layers(route)`.
- `Instance._dispatch` runs the chain carried on the resolved target, for calls
  by name and by function reference alike.
- Handle calls (`ctx.worlds.x.call`) are unchanged: they run only the owner's
  own chain, since the host's layers already wrap the calling host tool.

Behaviour is identical when no node is shared. Docs: composition.md's
Middleware section states the per-route rule, and SH207's Why in
reference/lints.md notes that two names of one tool can run different
middleware.

Tests: nine dispatch tests through world.instance(...) (the shop/payments
reproduction, two names by two routes, a diamond, a route filtered out by an
allow list, three deep, an intermediate error handler, a middleware registered
on a live instance, and a guard that a handle call runs only the owner's
layer) and two seal tests on entry chains. All but the guard fail on the
previous code.

No deviations from the spec.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0194LhNcR83VnggdfuMJMpSn
@scosman @claude
Correct the docs for the final code and sweep out history (phase 4)
7db13b7 
The last phase of cr_moderate_fixes: the docs, ProjectTracker's notes and
two code comments now describe the code as phases 1-3 left it.

- 9.M1: SH103's reason is restated. The old one (SQLite writes a second
  timestamp format) is false on Seahaven's own connections. The new
  reasons: CURRENT_DATE, julianday('now') and the like write a date or a
  number; a stamping trigger overwrites fixture history; a connection
  Seahaven did not open reads the host clock; 'now' in a generated
  column, index or CHECK is read once. The rule, and the lint's
  docstring, name every place the regex covers (CHECK constraints and
  index expressions included).
- 9.M7: composition.md's cost note drops the benchmark and design-target
  sentences for a per-node cost statement.
- 9.M8: projecttracker.md runs `seahaven check --world
  projecttracker:world` from the repo root, where the bare command fails
  with SH501.
- 8.M4: README, AGENTS.md, projecttracker.md, _events.py and startup.py
  say the audit trail records issue creation, status, assignee and
  comments, and that the five issue writes (not every write) take
  actor_id. AGENTS.md names what the trail does not record.
  schema/001_core.sql is left alone, since a comment edit changes the
  schema hash and every fixture.
- 2.M2/4.M3: Instance.bulk's docstring, the _bulk comment, composition.md
  and the api.md bulk() row say a composite bulk() rolls every node back
  when the block raises, but a failed commit leaves the nodes committed
  before it committed (the root commits last). New test
  test_a_bulk_commit_that_fails_keeps_the_nodes_committed_before_it pins
  this through world.instance(...) with a deferred foreign key.
- 9.M6: history, bench figures, version stories, maintainer notes and
  spec citations are removed from state.md, serving_and_openenv.md,
  composition.md, reference/api.md, db_schema_and_fixtures.md and
  projecttracker.md (H1-H24). api.md states the two clock-reading edge
  cases itself instead of linking the clock_modes risk report, and lists
  the non-root public names without citing component docs.
- Reviewer wording fixes in api.md, lints.md, composition.md,
  projecttracker.md, instances.py, lint/ddl.py and startup.py.

Deviations from components/phase_4_docs.md:
- The spec's bulk() paragraph for composition.md said the nodes that
  committed "before it" keep their rows; the docs also say "the nodes
  commit one at a time, the root last", because without the order
  "before it" has no meaning to a reader. "Build it again" became "make
  a new instance and load it again".
- The spec's projecttracker.md viewer sentence said "the five issue
  writes the audit trail records"; the docs also name the five tools,
  so a reader need not look them up.
- The spec's note left projecttracker.md:58 ("the whole reason") to the
  writer's judgement; it is reworded to "Nothing else uses those
  imports".

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0194LhNcR83VnggdfuMJMpSn
@scosman scosman changed the title Support JSON mode validation for tool arguments Strict wire validation, safer defaults, and the moderate deep-review fixes Sep 30, 2026
@scosman @claude
Merge main into claude/serene-newton-214g7x
e3f3c96 
Conflicts in state.md and serving_and_openenv.md: main moved bench/ to
tools/bench/ in paragraphs this branch removed in its docs sweep, so this
branch's side is kept. tests/test_docs.py now link-checks
.github/CONTRIBUTING.md, where main moved it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0194LhNcR83VnggdfuMJMpSn
@scosman
scosman merged commit 9cfb4e4 into main Sep 30, 2026
3 checks passed
@scosman
scosman deleted the claude/serene-newton-214g7x branch September 30, 2026 02:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@scosman