fix: route the roles the deployment actually calls, and check they answer - #140
Conversation
|
Rebased onto This branch was cut before #137 landed. It kept using the module-level
All four are re-applied on top:
633 tests, ruff and |
111c555 to
e6f656b
Compare
…swer Three defects with one shape: the role map was reported as though it were the map in force, and nothing ever asked whether it worked. **A project's role overrides never reached execution (#110).** `ProjectSpec.roles` is persisted, documented as a per-role override and already consulted by preflight, but `session_executor_factory` injected the one global `ModelClient` into every executor and its `routes_provider` read only the global `role_map` setting. A project could pass preflight on its own reviewer and have the work reviewed by the global model -- or fail with `no route for role reviewer` when the global map had none. Preflight was no better in the other direction: it took `project.roles or stored`, wholesale, so a project overriding only its planner lost the global reviewer it had all along. Now one merge, `effective_routes`, per role, used by everything that reads the map: the executor gets a sibling client whose routes resolve through the project's map at call time (sharing the parks -- a spend cap belongs to the endpoint, not to whichever project hit it first), and preflight, readiness and `/api/roles` resolve through the same function. Live changes still take effect without a restart, for the project row as well as the global map. **The map advertised two routes session mode never calls (#127).** `SessionExecutor` has exactly one `ModelClient` call site, the reviewer; planning and implementation are done by the agent process with its own credentials and endpoint. `planner` and `implementer` were nevertheless advertised, editable, and echoed back by `PUT /api/roles` -- so an operator would look for that spend in an audit log where it can never appear. `/api/roles` now reports `used: false` and what does the work instead, without deleting configuration the non-session `Executor` still needs. The independence warning had the same fault at one remove: it compared the reviewer to the *configured* implementer, which in session mode is a verdict about a pairing that never happens, while `/api/roles` never computed the property at all and defaulted it to true. Both now call one function against the implementer that actually runs, so the two endpoints cannot disagree. **Preflight never asked whether a model answers (#131).** `reviewer: ok=true -- a reviewer role is routed` is true of a model that will never reply. An endpoint can advertise a model in `/models` and serve nothing behind it; the harness then discovered that once per item, after the planner and implementer had been paid for, at the cost of the full retry ladder -- six attempts with escalating backoff, fifteen to twenty minutes of wall clock each, to establish a condition that was true before the run started. `role_reachability_probe` asks one minimal completion per reachable role, in parallel, on daemon threads with a deadline, and names the model and the status: `claude-sonnet-4-6 returned HTTP 504`, not `not ready`. Blocking when a role provably cannot answer. A model that is merely slow gets a warning, not a refusal -- a late answer is still an answer. Only roles the executor calls are probed, the ask is injected like every other probe here, and the answer is remembered for a minute so that polling readiness does not become a load generator against the endpoint. Every fix has a test that fails without it, and no test touches a network.
e6f656b to
450a1d3
Compare
Three defects with one shape: the role map was reported as though it were the map
in force, and nothing ever asked whether it worked.
A project's role overrides never reached execution
ProjectSpec.rolesis persisted, documented as a per-role override and alreadyconsulted by preflight, but
session_executor_factoryinjected the one globalModelClientinto every executor and itsroutes_providerread only the globalrole_mapsetting. A project could pass preflight on its own reviewer and havethe work reviewed by the global model — or fail with
no route for role reviewerwhen the global map had none.
Preflight was no better in the other direction: it took
project.roles or stored,wholesale, so a project overriding only its planner lost the global reviewer it
had all along.
/api/readinessread only the global route, so its top-levelanswer could contradict the project reports underneath it.
One merge now —
effective_routes, per role — used by everything that reads themap. The executor gets a sibling client whose routes resolve through the project's
map at call time, sharing the endpoint parks (a spend cap belongs to the endpoint,
not to whichever project hit it first). Live changes still take effect without a
restart, for the project row as well as the global map.
Closes #110.
The map advertised two routes session mode never calls
SessionExecutorhas exactly oneModelClientcall site, the reviewer; planningand implementation are done by the agent process with its own credentials and
endpoint.
plannerandimplementerwere nevertheless advertised, editable andechoed back by
PUT /api/roles— so an operator would look for that spend in anaudit log where it can never appear.
/api/rolesnow reportsused: falseand what does the work instead, withoutdeleting configuration the non-session
Executorstill needs. The independencewarning had the same fault at one remove: it compared the reviewer to the
configured implementer — in session mode a verdict about a pairing that never
happens — while
/api/rolesnever computed the property at all and defaulted itto
true. Both now call one function against the implementer that actually runs,so the two endpoints cannot disagree.
Closes #127.
Preflight never asked whether a model answers
reviewer: ok=true — a reviewer role is routedis true of a model that will neverreply. An endpoint can advertise a model in
/modelsand serve nothing behind it;the harness then discovered that once per item, after the planner and implementer
had been paid for, at the cost of the full retry ladder.
role_reachability_probeasks one minimal completion per reachable role, inparallel, on daemon threads with a deadline, and names the model and the status —
claude-sonnet-4-6 returned HTTP 504, notnot ready. Blocking when a roleprovably cannot answer; a model that is merely slow gets a warning rather than a
refusal, because a late answer is still an answer. Only roles the executor calls
are probed, the ask is injected like every other probe in
preflight.py, and theanswer is remembered for a minute so polling readiness does not become a load
generator against the endpoint.
Closes #131.
Verification
Every fix has a regression test that was confirmed to fail with the fix
temporarily reverted, and no test makes a network call:
tests/test_serve_fleet.pyfail withassert 'global-model' == 'project-model';tests/test_readiness.pyfails with a spuriousreviewerblocker, and the readiness summary reports no reviewer while every project has
one of its own;
/api/rolestests fail, the independence one withexactly the reported
assert True == False;tests/test_preflight.pyandtests/test_readiness.pyfail when the check is disabled.uv run pytest -q,uv run ruff check .,uv run ruff format --check .anduv run mypy .all pass.docs/DEPLOYMENT.md,docs/USAGE.md,docs/INTERNALS.mdandREADME.mdare updated to match; the deployment-doctests assert the new blocker and warning names against the preflight that
produces them.