Worlds fixes (realized after Seahaven was built) - #1773
Merged
sfierro merged 8 commits intoSep 18, 2026
Merged
Conversation
World Builder phase 0 needs a runtime before it can have a world. Seahaven is specified but unwritten, so `kiln_ai.worlds.shim` copies its authoring surface closely enough that a world moves to Seahaven by changing one dependency. It is deliberately throwaway, and it is self-contained: nothing outside the package changed, and Kiln's session manager stays a client of the wire, never an import. What a world gets: - `World`, `@world.tool`, `@world.middleware`, `@world.instance_startup`, `Ctx`, and `Tool.from_function`, which refuses at registration everything it can see going wrong later: an `async def`, a missing annotation, a `datetime` argument, a mutable default, a name that only exists under `TYPE_CHECKING`. - A per-instance SQLite runtime on the stdlib module: a clock frozen at the fixture's timestamp that SQLite's own date functions read, seeded ids, nesting transactions, and a read-only inspection connection behind a write-denying authorizer. - Fixtures that freeze through `VACUUM INTO` into a pending directory and one rename, verify their own bytes, seal build artifacts beside themselves, and fork. - One instance per session, and a net-changes diff plus a state digest computed by comparing the instance against a read-only baseline, which is what a grader reads after an episode. - The three control tools, the OpenEnv wire server Kiln's `OpenEnvSessionManager` talks to, content-composed versions, and a three-verb CLI. Tests: one module per runtime module, 281 in all, including determinism, 25 concurrent sessions with no cross-talk, and a conformance test that drives the shim's app through Kiln's real session manager end to end and asserts the whole `final_state` shape — the contract the wire fixes in phase 2 build on. Deviations from the component spec, accepted in review: - The spec puts the session-capacity counter at module level; the code keeps one per app. A module-level counter is only correct when a process serves one world, and the fault runs and the tests both serve several, where one world's sessions would otherwise eat another's budget. - The spec lists `fastapi` and `uvicorn` as dependencies of `openenv.py` without adding them to `kiln-ai`'s own dependency list, and the code does the same: both are already in the workspace and already imported by `worlds/testing.py`, and `__init__.py` imports neither `openenv.py` nor `cli.py`, so importing `kiln_ai.worlds.shim` pulls in neither. Serving a world is an explicit opt-in, so it is the right place for the cost. Five checks go beyond the spec, each closing a hole review found, and `components/shim.md` has been updated to describe them: - Forbidden argument annotations are matched anywhere inside the annotation, not only at its top level: with `strict=True`, `Optional[datetime]` builds a model no wire value satisfies, so accepting one would hand the agent a tool it can never call. - A tracked table's primary key columns must be unable to hold NULL. The diff joins on the key with `IS`, so two NULL-keyed rows would match each other and `changes()` would report edits nobody made. - Every `untracked_tables` name must be a real table, so a typo cannot silently leave a table tracked. - The clock's SQL overrides refuse the `'localtime'` and `'utc'` modifiers, which read the host machine's timezone and would make a world answer differently on every machine. - The wire serialises with `ensure_ascii=False`, and a failed reset clears the destroyed episode's id and step count. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QZFUkuNKL5kaBWVz9Sk6sE
…client
Five changes to the OpenEnv session manager and the world tool proxy, so a world
that models a real product can be driven and graded without Kiln learning
anything about that product. Every one is backwards compatible with the
reference server in `worlds/testing.py`: an environment that knows nothing of
control tools or coded errors ends its episodes exactly as before.
1. Error code and details reach the model. `ToolCallOutcome` carries
`error_code` and `error_details`, read from a dict error's `code`, `message`
and `details` only. The proxy renders the error through the new
`render_tool_error` -- `{"error": {code, message, details}}`, all three keys
always present -- and splits on the code: `WORLD_FAILURE_CODES` (`internal`,
`unknown_tool`, `invalid_arguments`, `world_gap`) keep `is_error=True`, while
any other code is the modelled product answering with an error of its own and
is delivered as an ordinary result, which is what the real system's tool would
return for the same refusal. The reference server's uncoded `type` key is
deliberately not mapped to a code, so no existing assertion moves.
2. Reset facts merge `observation.result`, `observation.metadata` and top-level
`metadata`, lowest to highest precedence, rather than reading one or the
other. Seahaven-shaped environments report their facts in the result.
3. `end_episode` settles. After `state` and before `close`, under the popped
session's lock, it steps each of `DEFAULT_SETTLE_CALLS` and writes
`final_state["changes"]` and `final_state["state_digest"]`. After, not
before, so `step_count` stays the agent's own on an environment that counts
tool calls. An `unknown_tool` or uncoded error means the environment has no
such snapshot and the key is simply absent; any other coded error is recorded
as `final_state["settle_error"] = {tool, code, message}`, settling stops
there, and the episode still ends so the generation is saved with the
evidence in it rather than scoring as "the agent wrote nothing".
`settle_calls=()` disables it.
4. `call_control_tool(episode, tool_name, arguments)` on the session manager and
its Protocol. The wire step is factored into `_step_call_tool(..., record=)`;
control calls pass `record=False`, so Kiln's own probes never count as the
agent's reward or end its episode.
5. `render_tool_result(None)` renders `null`, matching what a tool that
serializes its own empty body shows.
`worlds/testing.py` gains `ControlledCounterEnv` -- coded errors and unlisted
control tools -- and an `env_factory` on `create_app`/`serve_in_thread`,
defaulting to the plain `CounterEnv`.
Deviations from the spec (components/kiln_wire_fixes.md):
- The spec said the shim's conformance test belongs to phase 1 and is untouched
here; the code edits it, because phase 1's `serve_in_thread` defaults to
`include_control_tools=True`, so the shim's episodes now settle for real and
the test's exact-equality assertion on `final_state` no longer held. The edit
tightens rather than loosens it: `changes` and `state_digest` are popped (so
their absence is a `KeyError`), exact equality holds on the remaining five
keys, and both popped values are asserted on content -- the one insert the
episode made, and a lowercase-hex digest.
- The spec put the reversed-settle-order case inside
`test_settle_records_a_coded_failure`; the code splits it into
`test_settle_stops_at_the_first_failure`, because two setups and two teardowns
in one test blur which arrangement failed.
- The spec said the judge's `KeyError` on a missing `changes` is "recorded as
the eval item's error"; the code asserts the runner's last `Progress` is
`(complete=0, errors=1)` alongside no persisted score, because the runner
keeps no per-item error record, and the score assertion alone would also pass
if the item had been silently skipped or the failure swallowed -- which is the
failure mode that test exists to rule out.
Spec error corrected: the spec's `ControlledCounterEnv` reports `"tools": 4` in
its reset result, but the env lists five tools; the code reports
`len(self.TOOLS)` and the tests assert against it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QZFUkuNKL5kaBWVz9Sk6sE
`kiln_ai.worlds.validity`: the measurement that answers phase 0's question — does a hand-written world reproduce, per input and per configuration, the eval outcome the system it replicates produces, and does it rank agent configurations the same way. Nine modules. `scrub` (noise rules, declared-divergence expansion, JSON comparison), `idmap` (created ids by creation order), `traces` (recorded episodes out of a Kiln project, tool-step extraction, the symmetric invalidity rule), `replay` (tool-level replay against an in-process shim instance), `decision` (teacher-forced decision replay against a control arm), `metrics` (pairing, bootstraps, the paired and rank statistics, the gates), `faults` (world-server lifecycle, trigger counting, the fault gate), `cost` (cost model and budget meter), `report` (`validity.json` and `validity.md`). The package holds nothing about any particular world. Scrub rules, create tools, served tools, declared divergences, the world-server command, the fault table and pricing all arrive as arguments from a driver that knows its own world; phase 8 supplies them. The boundary is enforced rather than promised: `test_package` greps every source for the driver's vocabulary — the product, its fixture, its config file — and a second grep bans model ids and providers in the package's own modules, but not in tests, since a Kiln run config has to name a real `ModelProviderName`. That grep found a real leak: `cost.py` defaulted `pricing_source` to the driver's own filename. Three defects worth recording, all of which made a gate easier to pass. The cluster bootstrap did not resample clusters. `_clustered` drew with replacement, but both statistics it feeds re-group their input by the clustering key — `delta_value` by (configuration, input) and `_difference` through a dict keyed by (run_id, decision_index) — so a cluster drawn twice collapsed back into one, or overwrote itself. What was computed was a ~63% unweighted subset of the clusters, under-dispersed: on a realistic 6x20x3 paired set the interval came out about 28% too narrow. Both gates that read an interval are one-sided in the permissive direction (`paired` tests `interval.high`, `decision` tests `interval.low`), so every instance made a gate easier to pass. Each draw is now relabelled into its own cluster identity. The first regression tests for this were wrong: they exercised the statistics on hand-relabelled input and passed against the reintroduced bug. They now count groups and pairs inside the statistic, so they reach `_clustered`'s `relabel` argument, and were mutation-verified against a reverted fix. The replay gate passed on zero replayed steps. `replay_steps` was an optional keyword, so a clean mismatch list with no step count read as "no undeclared classes". It is now required and `not replay_steps` covers both None and 0 — the spec's rule is that nothing raises on emptiness and nothing passes on it. Four of the six seeded faults could never report a moved gate. A paired-targeted fault run is ten input pairs x six configurations x two repeats: 20 valid episodes per configuration, judged against a `min_valid_per_config` of 40. The paired and rank gates therefore came back `inconclusive`, `moved_gates` counts only pass-to-fail, and the report's `moved` column would have read empty however broken the faulted world was — which a reader parses as "the gate did not move" when it means "the gate could not move". `fault_thresholds` rescales the clean thresholds to the fault run's own volume, and `detect` now takes those thresholds and an `evaluable` check that marks the fault inconclusive *with a reason* when no gate could have moved. An inconclusive fault fails the fault gate's zero-inconclusive clause exactly like an untriggered one. Anything the report did not verify, it says so. A step count nobody supplied stays null rather than becoming `len(mismatches)`; `build_report` cross-checks its gates against `executed_steps` and against the replay count the gate was judged on, because it is the only place both halves are visible; a world server that publishes no `max_concurrent_envs` becomes a caveat; a model the pricing table does not name raises `UnpricedModel` whenever a budget is set, because a budget that cannot bind is not a budget. Deviations from components/validity_harness.md: - The spec puts the world-server lifecycle in `faults.py` and defines `cost.py`; the implementation plan named a `server.py` and omitted `cost.py`. Followed the component spec, which is authoritative, and corrected the plan line. - The spec has `start_world_server` refuse a server whose `max_concurrent_envs` is below the requested concurrency; the shim's `/metadata` reported no such key, so the check could never fire. Added it to the shim (one line, plus an assertion in the existing test) and kept the skip-with-caveat path for any other OpenEnv server. - The spec's `build_prefix` distinguishes a skipped step by `results[i] is None`; the code takes an explicit `raw_steps`, because a world result that is genuinely null — a delete — is not the same thing as a step the world was never asked to answer. - `ReplayedEpisode` gains `records`, because `replay.jsonl` needs an "ok" line per matching step and a matching step leaves no `Mismatch` to build one from. - `decision._substituted` sets `is_error` only for `WORLD_FAILURE_CODES`, where the spec's prose says every error envelope, because it reuses `world_tool.py`'s own constant so the world arm sees exactly what the world lane would have shown the model. A coded product error is an ordinary result on both arms; the spec text is wrong here. - Several signatures gained arguments the spec left no room for: `known_ids` on the replay entry points, `reset_kwargs`/`world_version` on `replay_decision` (the spec's example hardcodes a fixture name the generic half cannot know), `replay_steps` and the bootstrap seed on the gates, `scores` on `Cell` so the per-configuration component rates are computable at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QZFUkuNKL5kaBWVz9Sk6sE
Three client fixes found after Seahaven shipped, all in the session manager's wire handling and the episode model it fills. websockets keepalive now states ping_interval=20 / ping_timeout=120. The library's default 120s -> 20s timeout loses every episode on the box when a world server stalls at the event loop doing synchronous work: it cannot answer a ping, and the keepalive closes the session out from under a run. Seahaven ships its own client at 120s and this matches it. Both values are stated rather than defaulted, so a library default that moves cannot silently change what a run measured. STEP_TIMEOUT_S gains a comment recording the consequence: the keepalive, not the 600s step timeout, is the real ceiling for a stalled event loop. _settle() tolerates "tool_not_found" beside None and "unknown_tool". MCPErrorType.TOOL_NOT_FOUND is a real OpenEnv constant; nothing maps error_type onto the code today, so this is defensive rather than currently reachable, but if anyone later does, the tolerance already covers it. WorldEpisode.reset_metadata is renamed to reset_facts and its description corrected. Both were fossils: the name and the description claimed the facts came from the observation's metadata, but start_episode reads observation.result first and Seahaven reports there exclusively. No alias and no migration -- nothing has shipped and the model is not on main. The generated api_schema.d.ts was regenerated with generate_schema.sh and is verified by check_schema.sh inside the check suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QZFUkuNKL5kaBWVz9Sk6sE
…und one environment
The transport layer should tolerate what the OpenEnv protocol actually permits
and nothing that was fitted to a particular world. Reviewing the settle and
reset paths against openenv 0.4.2 on the wire, one entry in each was shaped
around environments we wrote ourselves; both are gone, and each thing that
stayed is justified here as generic rather than accommodation.
Removed: `"tool_not_found"` from `_settle`'s tolerated codes. It was
unreachable for every OpenEnv environment. `ToolError` is `extra="forbid"`
with only `error_type` and `message`, so no environment can send a `code` at
all, and `tool_not_found` is a value of `error_type`, which Kiln never reads.
The only thing the entry could ever have matched is a world that deliberately
repeated the literal as a code. It also never belonged: `functional_spec.md:259`
and `[A-13]` both state the rule as "`unknown_tool` or an error with no code at
all", so this restores the code to the spec rather than changing it.
Removed: the top-level `data["metadata"]` reset-facts source, dead by
construction. OpenEnv's `serialization.py` copies `observation.metadata` to the
top level as a sibling, so that value is always either absent or an exact copy
and can never say anything the observation did not. Pinned by a new test that
fails if it is re-added.
Kept, both observation slots. This is generic rather than accommodation: base
`Observation` is `extra="forbid"` and `metadata` is its only free-form field,
so a stock observation has nowhere else to put facts, while `result` belongs to
`CallToolObservation` and is reachable only by subclassing. Reading only
`result` would mean Kiln takes facts solely from environments shaped like one
framework. A real environment proves it -- the UpKeep mock returns a bare
`Observation(metadata={...})` and would have gone factless.
Kept, `None` as the generic settle arm, whose value is narrower than it looks:
for an environment serving no control tools `changes` is absent either way, so
what the arm buys is the difference between an absent optional key and a
recorded fault. It keeps `validity/traces.py` from stamping
`invalid="settle_error"` on every such episode and inflating the
`settle_errors` count, and it keeps worlds that serve only some settle tools
working.
Kept, non-dict `error` handling, as type discipline on a field declared
`str | None` rather than accommodation of any one environment: it stops a
foreign server's list or number reaching the tool proxy as an "error string".
Its test is now parametrized over a string and a list, so the coercion itself
is pinned and not only the tolerance.
Kept, `"unknown_tool"`, with the tension documented rather than resolved. It is
the same class of thing `tool_not_found` was -- one framework's vocabulary, not
a protocol constant -- and survives only because it is reachable and
load-bearing: a Seahaven-shaped world started without `include_control_tools`
answers exactly that, and dropping it would write `settle_error` into every one
of its episodes. Resolving it properly means a design decision (a
`settle_tolerated_codes` argument beside `settle_calls`, or moving the
vocabulary to where `WORLD_FAILURE_CODES` lives) and belongs with the Seahaven
swap, so `_settle`'s docstring now says so in code rather than only in the spec
repo.
The UpKeep verifier was run before and after and is byte-identical in
behaviour; both reset slots and non-dict tolerance are preserved, so nothing
another engineer depends on moved.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QZFUkuNKL5kaBWVz9Sk6sE
Kiln's job with worlds is to call them. It speaks OpenEnv over a socket and stays a black box about what serves the world, so the machinery for *building* one does not belong here. `kiln_ai.worlds.shim` and `kiln_ai.worlds.validity` move to the integration-tests repo as two standalone distributions, `world-shim` and `world-validity`, under `world-builder/resources/`. What stays is the client and its proof: `session_manager.py`, `datamodel/world.py`, `tools/world_tool.py`, and `testing.py` — a reference OpenEnv server that exists to show Kiln can drive a world, which is the client under test, not world-building. Nothing in Kiln imported either package: no pyproject changes, no rewritten imports, and a grep for both module paths across the tree comes back empty. The shim was under-declared here anyway — it imports fastapi and uvicorn, which `libs/core/pyproject.toml` never listed, so they only ever arrived through kiln-server inside the workspace and a plain `pip install kiln-ai` could not have served a world. `world-shim` declares them. Git history does not follow: this is a `git rm` here and fresh files there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`DEFAULT_SETTLE_CALLS` carried a `("state_digest", "controller_digest")` pair.
No world framework we point Kiln at serves that tool — Seahaven's control set is
`controller_changes` and `controller_run_sql`, and nothing else — so against a
Seahaven world the call missed on every single episode.
It missed quietly, which is why it went unnoticed: `_settle` reads `unknown_tool`
as "this environment does not serve the tool", logs at debug and leaves the key
absent. So the cost was a wasted round trip per episode writing a key nothing
opened. A grep across `libs/` and `app/` found one writer — the entry itself —
and no reader outside tests: no scorer, judge, API route or serializer.
The docstring now says what settle asks for and why, and makes the important
distinction explicit: what makes an absent control tool safe is `_settle`'s
tolerance, not the contents of this tuple. That tolerance is untouched, so a
deployment whose world does serve a digest tool adds the pair back and it
settles like any other entry.
Tests narrowed rather than deleted. Two that used `controller_digest` through
`call_control_tool` rather than through settle are deliberately left: they are
now what pins the tolerance this docstring claims.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📊 Coverage ReportOverall Coverage: 93% Diff: origin/sfierro/synthetic-worlds-eb...HEAD
Summary
|
Seahaven now answers `reset` with its facts in the observation's `metadata`,
publishes the change log through the ordinary `state` call, and sends an error
as OpenEnv's own `{error_type, message}` with its `{code, message, details}`
triple moved to `metadata["seahaven_error"]`. A `WorldBug` raises and stops the
session rather than arriving as data. Kiln is a plain OpenEnv client again, and
three things it grew to cope with the old shapes have nothing left to do.
The settle machinery goes. `call_control_tool`, `DEFAULT_SETTLE_CALLS` and
`_settle` existed to reach a control tool for the record of what an episode
changed. That record is in the state document now, and `end_episode` already
copies every key of `state` into `final_state`, so the replacement is no code at
all. `_live_session` and `_step_call_tool` were split out only to share a step
with the control path and fold back into `call_tool`.
The classification goes with it. `WORLD_FAILURE_CODES` decided whether an error
was the world failing or the product answering by matching a code against a
hardcoded set -- a denylist of one framework's spelling, wrong for `db_error`,
and needing a patch for every new backend. No code arrives any more, and a
broken world stops being data, so the question it answered is not Kiln's. Every
error on an observation is a failed call: `error_type`'s whole vocabulary,
`execution_error` included, means the call did not work, and an environment that
wants an error read as an ordinary answer returns it as a result.
`reset_facts` reverts to `reset_metadata`, reading the field the protocol
declares and nothing else.
What stays, because none of it was about Seahaven. The keepalive: a world server
doing synchronous work cannot answer a ping, and the library's 20s default drops
the session long before a long tool call finishes or Kiln's own 600s ceiling is
near. The error read, repointed at `error_type` and pulled out as
`read_observation_error` so its tolerance is testable on its own -- a code is
still read for an environment that sends one, and no shape is rejected.
`render_tool_result(None)` answering `"null"` rather than an empty string.
The test environment sent `{"type": ...}` on an error, which is not a shape any
OpenEnv environment sends: `ToolError` declares `error_type`. Conformed, so the
fake is worth testing against.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sfierro
changed the title
sfierro
marked this pull request as ready for review
September 18, 2026 18:33
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft, for reading the diff. Based on
sfierro/synthetic-worlds-eb(itself draft PR #1761).Net effect on Kiln is much smaller than the commit list suggests. Two of the seven commits on this branch add the runtime shim and the validity harness; a later commit removes both. Read as a unit, what actually lands in Kiln is the wire fixes plus one settle change.
What survives
90d9aa8ee,a415afce2,f2c5dfe8b— the OpenEnv client fixes a shim-shaped world needs: error code and details to the model, reset facts fromobservation.result, settle afterstate, keepalive, and generic protocol tolerance.119452d8e—DEFAULT_SETTLE_CALLSdrops the("state_digest", "controller_digest")pair.What lands and leaves again
466072e0eaddskiln_ai.worlds.shim,cabc3a57baddskiln_ai.worlds.validity.1b4b59ddaremoves both. They now live in the integration-tests repo as theworld-shimandworld-validitydistributions.The reasoning: Kiln's job with worlds is to call them. It speaks OpenEnv over a socket and stays a black box about what serves the world, so the machinery for building one does not belong here. What stays is the client and its proof —
session_manager.py,datamodel/world.py,tools/world_tool.py, andtesting.py, a reference OpenEnv server that exists to show Kiln can drive a world at all.Nothing in Kiln imported either package: no pyproject changes, no rewritten imports, and a grep for both module paths across the tree comes back empty.
On the settle change
controller_digestis served by no world framework Kiln is pointed at — Seahaven's control set iscontroller_changesandcontroller_run_sql, and nothing else — so against a Seahaven world the call missed on every episode. It missed quietly, because_settlereadsunknown_toolas "this environment does not serve the tool". A grep acrosslibs/andapp/found one writer (the entry itself) and no reader outside tests.What makes an absent control tool safe is
_settle's tolerance, not the contents of that tuple. That tolerance is untouched, so a deployment whose world does serve a digest tool adds the pair back.Checks
uv run ./checks.sh --agent-modegreen.Worth deciding before review
If this is read commit-by-commit, a reviewer spends most of it on ~6,300 lines of shim that leave again. Squashing or reordering before review would make the actual Kiln change legible. Left as-is because the history is honest and that is your call.
🤖 Generated with Claude Code