Skip to content

Commit b3fdd60

Browse files
committed
Replace elicitation origin ContextVar with an explicit parameter
The ContextVar-based originating_client() let the connection an ER should ask questions of leak into ambient state, which is invisible at call sites and silently defaults to "nobody" if a hop forgets to set it — exactly the failure the two progressToken handlers in _streaming.py had (a connected client whose run could never be asked). Passing RunDispatchOrigin explicitly, with no default at any hop, turns a dropped connection into a signature the type checker can catch instead of a question nobody receives (ADR-0082). - elicitation_bridge.py: RunDispatchOrigin(connection) replaces the originating_client() context manager; bind_run() now takes it as a required argument instead of reading the ContextVar. - er_dispatch.py, in_flight_runs.py, proxy_utils.py, matrix_streaming.py, project_executor.py, execution_scopes.py, workspace_executor.py, partial_results_service.py: thread `origin` explicitly from each request handler down through every dispatch seam to the choke point (in_flight_runs.track) that binds it. - _actions.py, _streaming.py, cli_app/utils.py, prepare_envs_service.py: each call site states its origin explicitly, including the plain request/response and internal dispatch paths that have none. - developing-finecode.md: documents the "ambient state" rule this refactor follows (when a ContextVar is justified vs. when a parameter is required with no default), plus the boolean-flags, comments, knowledge-package-split and local-observability-stack sections accumulated in this file since the last commit touching it.
1 parent 4962590 commit b3fdd60

23 files changed

Lines changed: 642 additions & 186 deletions

docs/guides/developing-finecode.md

Lines changed: 174 additions & 0 deletions
Large diffs are not rendered by default.

src/finecode/cli_app/utils.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,9 @@ async def run_actions_in_projects_and_concat_results(
124124
run_trigger=run_trigger,
125125
dev_env=dev_env,
126126
payload_overrides_by_project=payload_overrides_by_project or {},
127+
# The CLI drives the WM in-process; there is no client connection an ER
128+
# could be pointed at.
129+
origin=None,
127130
)
128131

129132
result_output: str = ""

src/finecode/wm_server/_api_handlers/_actions.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,10 @@ async def _handle_run_action(
7575
result_formats=parsed.result_formats,
7676
initialize_all_handlers=True,
7777
selected_interpreters=selected_interpreters,
78+
# Plain request/response: this handler never receives the
79+
# caller's writer, so there is no connection to put a question
80+
# to. Elicitation is available on the streamed paths only.
81+
origin=None,
7882
)
7983
return {
8084
"resultByFormat": result.result_by_format,
@@ -240,6 +244,8 @@ async def _handle_run_batch(
240244
concurrently=parsed.concurrently,
241245
result_formats=parsed.result_formats,
242246
payload_overrides_by_project=parsed.params_by_project,
247+
# No writer here either — see `_handle_run_action` above.
248+
origin=None,
243249
)
244250

245251
results, overall_return_code = _build_batch_result(

src/finecode/wm_server/_api_handlers/_streaming.py

Lines changed: 100 additions & 92 deletions
Original file line numberDiff line numberDiff line change
@@ -123,96 +123,97 @@ async def _handle_run_action_with_partial_results(
123123

124124
# From here to the final result, this connection is the origin of the
125125
# run: an ER that asks a question mid-run is asking the client that
126-
# started it and nobody else (ADR-0082 rule 1). Set on the streamed
127-
# paths, which are the ones that still hold their caller's connection;
128-
# the dispatch below binds it to the run id it mints, which is what the
129-
# ER names when it asks.
130-
with elicitation_bridge.originating_client(writer):
131-
stream = await partial_results_service.run_action_with_partial_results(
132-
action_name=action_name,
133-
project_path=project_path,
134-
params=params.get("params", {}),
135-
partial_result_token=token,
136-
run_trigger=trigger,
137-
dev_env=dev_env,
138-
ws_context=ws_context,
139-
result_formats=result_formats,
140-
progress_token=progress_token,
141-
selected_interpreters=selected_interpreters,
142-
)
143-
144-
# Opt-in (collect-style callers like MCP): accumulate the `json` format of
145-
# each partial per project so it can be type-safely merged into the response.
146-
merge_results_enabled = options.get("mergeResults", False)
147-
json_by_project: dict[str, list[dict]] = {}
148-
149-
async def _forward_partials() -> int:
150-
count = 0
151-
async for value in stream:
152-
count += 1
153-
if merge_results_enabled and isinstance(value, dict):
154-
project_str = value.get("project", "")
155-
result_by_format = value.get("resultByFormat") or {}
156-
json_by_project.setdefault(project_str, []).append(
157-
result_by_format.get("json")
158-
)
159-
logger.trace(
160-
f"run+partialResults: sending partial #{count} for token={token}, keys={list(value.keys()) if isinstance(value, dict) else type(value)}"
161-
)
162-
_notify_client(
163-
writer,
164-
"actions/partialResult",
165-
{"token": token, "value": value},
126+
# started it and nobody else (ADR-0082 rule 1). Constructed on the
127+
# streamed paths, which are the ones that still hold their caller's
128+
# connection; the dispatch below binds it to the run id it mints,
129+
# which is what the ER names when it asks.
130+
origin = elicitation_bridge.RunDispatchOrigin(connection=writer)
131+
stream = await partial_results_service.run_action_with_partial_results(
132+
action_name=action_name,
133+
project_path=project_path,
134+
params=params.get("params", {}),
135+
partial_result_token=token,
136+
run_trigger=trigger,
137+
dev_env=dev_env,
138+
ws_context=ws_context,
139+
result_formats=result_formats,
140+
progress_token=progress_token,
141+
selected_interpreters=selected_interpreters,
142+
origin=origin,
143+
)
144+
145+
# Opt-in (collect-style callers like MCP): accumulate the `json` format of
146+
# each partial per project so it can be type-safely merged into the response.
147+
merge_results_enabled = options.get("mergeResults", False)
148+
json_by_project: dict[str, list[dict]] = {}
149+
150+
async def _forward_partials() -> int:
151+
count = 0
152+
async for value in stream:
153+
count += 1
154+
if merge_results_enabled and isinstance(value, dict):
155+
project_str = value.get("project", "")
156+
result_by_format = value.get("resultByFormat") or {}
157+
json_by_project.setdefault(project_str, []).append(
158+
result_by_format.get("json")
166159
)
167-
await writer.drain()
168-
return count
160+
logger.trace(
161+
f"run+partialResults: sending partial #{count} for token={token}, keys={list(value.keys()) if isinstance(value, dict) else type(value)}"
162+
)
163+
_notify_client(
164+
writer,
165+
"actions/partialResult",
166+
{"token": token, "value": value},
167+
)
168+
await writer.drain()
169+
return count
169170

170-
async def _forward_progress() -> None:
171-
if stream.progress_stream is None or progress_token is None:
172-
return
173-
async for value in stream.progress_stream:
174-
logger.trace(
175-
f"run+partialResults: sending progress type={value.get('type')} for token={progress_token}"
176-
)
177-
_notify_client(
178-
writer,
179-
"actions/progress",
180-
{"token": progress_token, "value": value},
181-
)
182-
await writer.drain()
171+
async def _forward_progress() -> None:
172+
if stream.progress_stream is None or progress_token is None:
173+
return
174+
async for value in stream.progress_stream:
175+
logger.trace(
176+
f"run+partialResults: sending progress type={value.get('type')} for token={progress_token}"
177+
)
178+
_notify_client(
179+
writer,
180+
"actions/progress",
181+
{"token": progress_token, "value": value},
182+
)
183+
await writer.drain()
183184

184-
partial_count = 0
185-
async with asyncio.TaskGroup() as forward_tg:
186-
partials_task = forward_tg.create_task(_forward_partials())
187-
forward_tg.create_task(_forward_progress())
188-
partial_count = partials_task.result()
189-
190-
final = await stream.final_result()
191-
192-
if merge_results_enabled and json_by_project:
193-
return_code = final.get("returnCode", 0) if isinstance(final, dict) else 0
194-
results: dict[str, dict] = {}
195-
for project_str, payloads in json_by_project.items():
196-
merged_json = await merge_partial_results_for_action(
197-
project_path=pathlib.Path(project_str),
198-
action_name=action_name,
199-
json_payloads=payloads,
200-
ws_context=ws_context,
201-
)
202-
if merged_json is not None:
203-
results[project_str] = {
204-
action_source: {
205-
"resultByFormat": {"json": merged_json},
206-
"returnCode": return_code,
207-
}
185+
partial_count = 0
186+
async with asyncio.TaskGroup() as forward_tg:
187+
partials_task = forward_tg.create_task(_forward_partials())
188+
forward_tg.create_task(_forward_progress())
189+
partial_count = partials_task.result()
190+
191+
final = await stream.final_result()
192+
193+
if merge_results_enabled and json_by_project:
194+
return_code = final.get("returnCode", 0) if isinstance(final, dict) else 0
195+
results: dict[str, dict] = {}
196+
for project_str, payloads in json_by_project.items():
197+
merged_json = await merge_partial_results_for_action(
198+
project_path=pathlib.Path(project_str),
199+
action_name=action_name,
200+
json_payloads=payloads,
201+
ws_context=ws_context,
202+
)
203+
if merged_json is not None:
204+
results[project_str] = {
205+
action_source: {
206+
"resultByFormat": {"json": merged_json},
207+
"returnCode": return_code,
208208
}
209-
if results:
210-
final = {**final, "results": results}
209+
}
210+
if results:
211+
final = {**final, "results": results}
211212

212-
logger.trace(
213-
f"run+partialResults: done, sent {partial_count} partials, final keys={list(final.keys()) if isinstance(final, dict) else type(final)}"
214-
)
215-
return final
213+
logger.trace(
214+
f"run+partialResults: done, sent {partial_count} partials, final keys={list(final.keys()) if isinstance(final, dict) else type(final)}"
215+
)
216+
return final
216217

217218

218219
async def _handle_run_action_with_partial_results_task(
@@ -301,13 +302,11 @@ async def _handle_run_batch_with_partial_results(
301302

302303
params = params or {}
303304
# The connection that asked for the batch is the origin of every run in it,
304-
# including the ones the per-project tasks below dispatch: a task copies the
305-
# context it is created in, so setting this before they exist is what makes
306-
# it reach them (ADR-0082 rule 1).
307-
with (
308-
telemetry.attach_incoming_traceparent(params),
309-
elicitation_bridge.originating_client(writer),
310-
):
305+
# including the ones the per-project tasks below dispatch: `origin` is
306+
# captured in `_stream_action`'s closure, so a task created from it carries
307+
# the same descriptor to every dispatch it makes (ADR-0082 rule 1).
308+
origin = elicitation_bridge.RunDispatchOrigin(connection=writer)
309+
with telemetry.attach_incoming_traceparent(params):
311310
parsed = _parse_run_batch_params(params)
312311
token = params["partialResultToken"]
313312

@@ -438,6 +437,7 @@ async def _on_partial(
438437
merge_results=parsed.merge_results,
439438
on_partial=_on_partial,
440439
selected_interpreters=selected_interpreters,
440+
origin=origin,
441441
)
442442
if parsed.merge_results:
443443
merged_results.setdefault(str(project_path), {})[action_source] = {
@@ -461,6 +461,7 @@ async def _on_partial(
461461
ws_context=ws_context,
462462
initialize_all_handlers=True,
463463
result_formats=parsed.result_formats,
464+
origin=origin,
464465
) as ctx:
465466
async for value in ctx:
466467
partial_count += 1
@@ -727,6 +728,10 @@ async def _forward_progress() -> None:
727728
result_formats=parsed.result_formats,
728729
initialize_all_handlers=True,
729730
progress_token=progress_token,
731+
# Progress is forwarded to this connection for the whole run, so
732+
# it is just as much the run's origin as on the partial-results
733+
# paths: an ER that elicits mid-run has a client to ask.
734+
origin=elicitation_bridge.RunDispatchOrigin(connection=writer),
730735
)
731736
return {
732737
"resultByFormat": result.result_by_format,
@@ -906,6 +911,9 @@ async def _forward_to_client() -> None:
906911
result_formats=parsed.result_formats,
907912
payload_overrides_by_project=parsed.params_by_project or None,
908913
progress_token_by_project=progress_token_by_project,
914+
# Aggregated progress goes to this connection until the batch
915+
# ends, so it is the origin of every run in the batch.
916+
origin=elicitation_bridge.RunDispatchOrigin(connection=writer),
909917
)
910918
finally:
911919
# Cancel get_progress tasks first, then drain slot lists through aggregator,

src/finecode/wm_server/runner/elicitation_bridge.py

Lines changed: 41 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -28,34 +28,50 @@
2828
implementation notes park exactly this until such an identifier exists — it does
2929
now, independently of the WAL, since ADR-0079 keys in-flight runs by it).
3030
31-
The two halves are separate on purpose: :func:`originating_client` marks *this
32-
task* as belonging to a connection, and :func:`bind_run` records the run id the
33-
dispatch minted under that connection. The first is a
34-
:class:`contextvars.ContextVar`, so it reaches the dispatch through call chains
35-
and into the tasks it spawns without every layer in between having to carry it;
36-
the second is what turns it into a lookup an ER on another connection can
37-
resolve.
31+
:class:`RunDispatchOrigin` carries the originating connection down to
32+
:func:`bind_run`, the one choke point every dispatch passes through
33+
(``in_flight_runs.track``): the request handler that still holds its caller's
34+
connection constructs it, threads it explicitly through the dispatch, and
35+
:func:`bind_run` records the run id the dispatch minted under it. Passed
36+
explicitly rather than read from ambient state, per "Ambient state: when a
37+
``ContextVar`` is allowed" in developing-finecode.md — a value many
38+
intermediate signatures carry is a cost, not a structural blocker.
3839
"""
3940

4041
from __future__ import annotations
4142

4243
import collections.abc
4344
import contextlib
44-
import contextvars
45+
import dataclasses
4546
import typing
4647

4748
__all__ = [
4849
"ElicitationBridge",
50+
"RunDispatchOrigin",
4951
"bind_run",
5052
"handlers",
5153
"install",
52-
"originating_client",
5354
"originating_client_for_run",
5455
"reset",
5556
"reset_origins",
5657
]
5758

5859

60+
@dataclasses.dataclass(frozen=True, slots=True)
61+
class RunDispatchOrigin:
62+
"""Which client connection, if any, is asking for a run.
63+
64+
Constructed at the request handler that still holds its caller's
65+
connection — or, for a nested ER→WM→ER dispatch, derived from the calling
66+
run's connection via :func:`originating_client_for_run` — and threaded
67+
explicitly down to :func:`bind_run`. ``connection=None`` is the honest
68+
answer for anything the WM started on its own behalf, or a dispatch that
69+
never had a client to begin with, and binds nothing.
70+
"""
71+
72+
connection: object | None
73+
74+
5975
class ElicitationBridge(typing.Protocol):
6076
"""What the runner needs from whoever owns client connections."""
6177

@@ -113,49 +129,33 @@ def handlers() -> ElicitationBridge | None:
113129
# Addressing: which connection started a given run
114130
# ---------------------------------------------------------------------------
115131

116-
# The connection whose request this task is executing. A ContextVar rather than
117-
# a parameter because the dispatch that mints a run id sits many layers below
118-
# the handler that knows the connection, and every layer in between would
119-
# otherwise have to carry something it has no use for. Tasks copy the context
120-
# they are created in, so a fan-out inherits it without being told.
121-
_origin: contextvars.ContextVar[object | None] = contextvars.ContextVar(
122-
"finecode_elicitation_origin", default=None
123-
)
124-
125132
# Run id → the connection that started it. Written for the life of the dispatch
126133
# and read by an ER on a different connection entirely, which is why this is a
127-
# registry and not just the ContextVar above.
134+
# registry rather than a value carried on the run's own call stack.
128135
_runs: dict[str, object] = {}
129136

130137

131138
@contextlib.contextmanager
132-
def originating_client(connection: object | None) -> collections.abc.Iterator[None]:
133-
"""Mark this task, and what it starts, as running for *connection*.
134-
135-
Entered by the request handlers that still hold their caller's connection,
136-
and again by nested dispatch on behalf of the run that asked for it. Passing
137-
``None`` is meaningful: it states that the work in the block has no
138-
identifiable origin, which is the honest answer for anything the WM started
139-
on its own behalf.
140-
"""
141-
token = _origin.set(connection)
142-
try:
143-
yield
144-
finally:
145-
_origin.reset(token)
146-
147-
148-
@contextlib.contextmanager
149-
def bind_run(run_id: str) -> collections.abc.Iterator[None]:
150-
"""Record *run_id* as belonging to the connection this task is running for.
139+
def bind_run(
140+
run_id: str, origin: RunDispatchOrigin | None
141+
) -> collections.abc.Iterator[None]:
142+
"""Record *run_id* as belonging to *origin*'s connection.
151143
152144
Entered where the run identifier is minted, so the binding lasts exactly as
153145
long as the run does: a question can only be addressed while the run that
154146
asks it is in flight, and an entry outliving its run would address a later
155-
question to a client that has moved on. A dispatch with no originating
156-
connection records nothing rather than an empty entry.
147+
question to a client that has moved on. ``origin=None``, or an origin whose
148+
``connection`` is ``None``, records nothing rather than an empty entry —
149+
that is the honest state for a dispatch with no identifiable origin.
150+
151+
*origin* has no default, here and at every hop that forwards it. Passing an
152+
explicit ``None`` is cheap and states a real fact; a default would let the
153+
argument be dropped silently at one hop of a long chain, and the result —
154+
a connected client that is never asked — looks exactly like a run that
155+
genuinely had nobody to ask. That is the failure this parameter replaced a
156+
``ContextVar`` to avoid, so it must not be reintroduced as a default.
157157
"""
158-
connection = _origin.get()
158+
connection = origin.connection if origin is not None else None
159159
if connection is None:
160160
yield
161161
return

0 commit comments

Comments
 (0)