Skip to content

fix: close 11 correctness findings in the merged runtime - #16

Merged
zhanghanduo merged 6 commits into
mainfrom
fix/review-findings-main
Sep 2, 2026
Merged

zhanghanduo merged 6 commits into
mainfrom
fix/review-findings-main

Conversation

@zhanghanduo

Copy link
Copy Markdown
Collaborator

Summary

Eleven correctness defects surfaced by a high-effort review of the extracted runtime. All of them are already on main (they predate #15, which the review over-scoped into); none were caught by the suite. They share one shape: each is invisible in the default configuration and only surfaces once a second feature is switched on — a middleware chain gets registered, a ContextPolicy gets an include_fields list, an operator points the loader at an explicit config path, a stream retries.

Five commits, grouped by subsystem so each is reviewable on its own.

Token accounting — e5a71e4

BudgetObserver read ctx.usage["input_tokens"] / ["output_tokens"], but the loop normalises every provider payload to prompt_tokens / completion_tokens. tokens_used stayed 0 for the whole run, so neither the budget_exhausted intervention nor the warn_ratio warning could ever fire. The alias-only tests passed because they fed raw input_tokens.

ContextSizeGuard carries a comment describing having fixed exactly this for itself, and InputTokenGauge open-codes the same two-spelling read — three consumers, three chances to get it wrong. Added usage_input_tokens / usage_output_tokens beside extract_usage and routed all three through them.

LLM middleware — 871fff3

  • SummarizationMiddleware bounded its orphan-avoidance loop at len(rest) - 1, one element short. With keep_recent=3 over a tail of [assistant(tool_calls), tool, tool, tool] the kept window was [tool] — precisely the orphan the loop exists to prevent, and the Azure 400 No tool call found for function call output it was written to stop.
  • LLMProxy.stream emitted run_after from a finally nested inside the retry while True. A stream failing before its first chunk called every middleware's after_llm with an empty LLMResponse and then again with the real one — cost/usage accounting and history-appending middleware double-counted.
  • LLMProxy._make_ctx wrote its per-call step_id / prompt_id back into the shared ExecutionScope.metadata. Two concurrent chat / stream calls on one scope (which _next_call_index documents as supported) clobbered each other, so _log_llm_exception attributed failures to the other call's step. Only session_id is scope-stable, so only it is published back.

DAG node wrappers — 7eb7aa2

  • metadata["max_retries"] never retried. The condition was err_result is None and attempt < max_retries, making retry depend on a middleware suppressing the error. With the default ExecutionMiddleware.on_error (which returns the error) a node declaring retries raised on attempt 0 and was never retried. The suppression branch keeps its documented behaviour but now logs the swallowed exception, so a genuine crash is no longer traceless.
  • A None delta crashed the wrapper. MiniDAGRunner explicitly accepts None as "no state change", but the middleware wrapper passed it into run_after_phase and then result.get(...). A node that worked with no middleware registered raised AttributeError the moment a chain was added — and the surrounding except misreported the contract violation as a middleware error.
  • ctx.task_id was empty under a ContextPolicy. The NodeContext wrapper was applied inside the context filter, so task_id_getter read the already-filtered state. Any node whose include_fields omitted task_id — the normal use of the policy — saw ctx.task_id == "", and every task-scoped lookup or telemetry emission from it was attributed to the empty task id.

Skills loader — e4fa196

The auto-reload and reload() both called ExtensionsConfig.from_file() with no argument, discarding the path the config came from and restarting the cwd/env search. For a config loaded from an explicit path outside those locations the search finds nothing and returns empty defaults — and since is_skill_enabled defaults to True for anything unlisted, an operator disabling one skill got every skill re-enabled: the decision inverted, not merely lost. The empty config also carries no _file_path, so has_changed returned False from then on and the state could never recover. ExtensionsConfig now exposes source_path; an unreadable config keeps the previous state rather than failing open.

Separately, toggle_skill was the only accessor that did not discover lazily, so on a fresh loader it was a silent no-op. The existing test called discover() first, which hid it.

Observers — 58de97c

  • The landing turn could be replayed with tools. LeakedToolCallRetryObserver returns continue_to_next_turn=True to replay a leaked turn, but LastTurnForcer strips tools via a one-shot _llm_strip_tools that _prepare_llm_request has already consumed — so the replay re-bound tools on the one turn the policy guaranteed would have none. The existing blocked_tool_calls guard cannot see this: with no tools bound nothing was parsed, while a prose answer merely mentioning finalize_answer(...) still trips _looks_like_leak.
  • TrajectoryFileObserver read a coalescing bound of 0 as "never due" rather than "bound disabled", contradicting its own module header. With SWARM_TRAJECTORY_COALESCE_N=0 and the default COALESCE_MS=0 the JSON snapshot was written once and frozen until on_loop_end forced a flush — so a process killed before that (OOM, hard timeout) left only the start state, the exact case the snapshot exists for.

Tests

37 new tests across 6 files. Every one was verified to fail against the pre-fix code (by stashing each fix and re-running), so they are regression tests rather than restatements of current behaviour. Two guard the fixes from over-correcting: a healthy non-tool tail must still be kept intact by summarization, and a genuine leak on a normal turn must still retry.

Validation

  • uv run ruff check agent_core tests
  • uv run pyright agent_core — 0 errors, 0 warnings
  • uv run pytest -q — 770 passed (was 711)
  • uv build, plus a smoke run of all 11 paths against the built wheel

Note on review scope

The review that produced these ran against a stale local main, so it reported them alongside #15's own findings. #15's four (SpawnGuard reservation leaks, the AgentComm ordinal contract, the EventSink append mismatch, and wall_deadline_s going unenforced) are fixed on that branch; these eleven are independent of it and land here.

zhanghanduo and others added 6 commits September 2, 2026 13:38
`BudgetObserver` read `ctx.usage["input_tokens"]` / `["output_tokens"]`, but
the loop normalises every provider payload to `prompt_tokens` /
`completion_tokens` (`extract_usage`). `tokens_used` therefore stayed 0 for
the whole run and neither the `budget_exhausted` intervention nor the
`warn_ratio` warning could ever fire. The alias-only tests passed because
they fed raw `input_tokens`.

`ContextSizeGuard` carries a comment describing having fixed exactly this for
itself, and `InputTokenGauge` open-codes the same two-spelling read — three
consumers, three chances to get it wrong. Add `usage_input_tokens` /
`usage_output_tokens` beside `extract_usage` and route all three through
them, so the next consumer inherits the knowledge instead of rediscovering
it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…pe trace ids

Three defects in the LLM middleware layer.

`SummarizationMiddleware` advanced the split point past leading tool
messages to avoid an orphan tool result, but bounded the loop at
`len(rest) - 1` and so stopped one element short. With `keep_recent=3` over
a tail of `[assistant(tool_calls), tool, tool, tool]` the kept window was
`[tool]` — precisely the orphan the loop exists to prevent, and the Azure
`400 No tool call found for function call output` it was written to stop.
Running off the end is the safe outcome: everything is summarised and the
kept window is empty.

`LLMProxy.stream` emitted `run_after` from a `finally` nested inside the
retry `while True`, so a stream failing before its first chunk called every
middleware's `after_llm` with an empty `LLMResponse` and then again with the
real one. Cost/usage accounting and history-appending middleware
double-counted, and the phantom response read as a genuine empty answer.
Hoist the block so it runs once per call.

`LLMProxy._make_ctx` wrote its freshly minted `step_id` / `prompt_id` back
into the shared `ExecutionScope.metadata`. Two concurrent `chat` / `stream`
calls on one scope — which `_next_call_index` documents as supported — then
clobbered each other and `_log_llm_exception` attributed a failure to the
other call's step. Only `session_id` is scope-stable, so only it is
published back.

Each fix has a regression test verified to fail against the previous code.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…the filter

Three defects in the node-wrapper stack `DynamicGraphBuilder.build` assembles.
Each is invisible in the default configuration and only surfaces once a second
feature is switched on, which is why the suite missed all three.

`metadata["max_retries"]` was gated on `err_result is None`, i.e. on a
middleware *suppressing* the error. With the default
`ExecutionMiddleware.on_error` — which returns the error — a node declaring
retries raised on attempt 0 and was never retried. Retry now depends on
attempts remaining, which is what the field means. The suppression branch
keeps its documented behaviour (advance on a synthetic delta) but logs the
swallowed exception, so a genuine crash is no longer traceless.

`MiniDAGRunner` explicitly accepts a `None` delta as "no state change", but
the middleware wrapper passed it straight into `run_after_phase` and then
`result.get(...)`. A node that worked with no middleware registered raised
`AttributeError: 'NoneType' has no attribute 'get'` the moment a chain was
added — and the surrounding `except` reported the contract violation as a
middleware error. Normalise `None` to `{}` before the hook.

The NodeContext wrapper was applied *inside* the context filter, so
`task_id_getter` read the already-filtered state. Any node whose
`ContextPolicy.include_fields` omitted `task_id` — the normal use of the
policy — saw `ctx.task_id == ""`, and every task-scoped lookup or telemetry
emission from it was attributed to the empty task id. The filter now sits
innermost; it forwards extra args so the context wrapper can sit outside it,
and `build` decides the arity question from the original node signature
rather than from a wrapper's.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`get_enabled_skills` auto-reloads when the extensions config changes on disk,
and `reload()` does the same on demand. Both called
`ExtensionsConfig.from_file()` with no argument, discarding the path the
config was loaded from and restarting the cwd/env search.

For a loader built with a config loaded from an explicit path outside those
locations the search finds nothing and returns empty defaults. Since
`is_skill_enabled` defaults to True for anything unlisted, an operator
disabling one skill got every skill re-enabled — the decision inverted, not
merely lost. The empty config also carries no `_file_path`, so `has_changed`
returned False from then on and the state could never recover.

`ExtensionsConfig` now exposes `source_path`, and the loader re-reads from it.
A config that has become unreadable keeps the previously loaded state rather
than failing open to "everything enabled", which is the wrong direction for a
gate.

Separately, `toggle_skill` was the only accessor that did not discover
lazily: on a freshly built loader it read an empty `_skills` and returned
False for a skill that exists on disk. The existing test called `discover()`
first, which hid it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…cing

`LeakedToolCallRetryObserver` returns `continue_to_next_turn=True` to replay a
turn whose tool call leaked into prose. On the landing turn that is wrong:
`LastTurnForcer` strips tools by planting `_llm_strip_tools`, which
`_prepare_llm_request` consumes one-shot, so the replayed turn is re-bound
*with* tools — the one turn the policy guaranteed would have none. The
existing `blocked_tool_calls` guard cannot see this, because with no tools
bound nothing was parsed and that list is empty, while a prose answer merely
mentioning `finalize_answer(...)` still trips `_looks_like_leak`.
`_prepare_llm_request` now records `_llm_tools_stripped` for the turn and the
observer declines to replay.

`TrajectoryFileObserver` read a coalescing bound of 0 as "never due" rather
than "bound disabled", contradicting the module header. With
`SWARM_TRAJECTORY_COALESCE_N=0` and the default `COALESCE_MS=0` the JSON
snapshot was written once and then frozen until `on_loop_end` forced a final
flush — so a process killed before that (OOM, hard timeout) left a file
holding only the `start` state, the exact case the snapshot exists for. With
every bound disabled there is nothing to coalesce, so every event flushes.

Both regression tests verified to fail against the previous code.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@zhanghanduo
zhanghanduo merged commit aa7df4d into main Sep 2, 2026
1 check passed
@zhanghanduo
zhanghanduo deleted the fix/review-findings-main branch September 2, 2026 06:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant