fix: always emit execution:end, honor tool:pre rewrites, and break identical failure loops by michaeljabbour · Pull Request #41 · microsoft/amplifier-module-loop-streaming

Michael J. Jabbour (michaeljabbour) · 2026-08-19T04:57:08Z

Summary

Four fixes to the streaming orchestrator, all found by forensic analysis of a real
21-hour session (eec9ae98) that destroyed itself.

The headline defect: execution:end was emitted on the method's last line, so any
early return skipped it. The incident shows 27 execution:start against 15
execution:end — and the 12 missing ends match the 12 cancellations exactly.

Commits

905ccfc — add amplifier-foundation to the dev group so the suite can collect
On a clean checkout the suite could not even collect:
ModuleNotFoundError: amplifier_foundation. Adding the missing dev dependency took the
repo from uncollectable to 173 passing. This lands first because nothing below is
verifiable without it.

c0c95af — always emit execution:end, including when a turn is cancelled
Four early-return paths bypassed the emit. Now a try/finally inside the async
generator, which additionally covers GeneratorExit — the case where a consumer stops
reading mid-stream and the generator is closed from the outside.

Reviewers: read this one with git show -w.
The diff reads as ~1,244 lines because wrapping the body in try/finally required
re-indenting ~728 lines. With whitespace ignored it collapses to
46 insertions / 28 deletions.

d4ce28b — honor tool:pre hook rewrites of tool arguments
Hook rewrites were silently discarded. Worth flagging for the kernel folks: the kernel
normalizes modify away — emit() returns action="continue" with the payload
moved into data. So the pattern documented at ORCHESTRATOR_CONTRACT.md:259-261
(checking if result.action == "modify") is unreachable code. This fix reads the
payload from where the kernel actually puts it; the contract doc is worth a follow-up.

13f7fc5 — trip a circuit breaker when a tool call keeps failing identically
The incident issued the same failing read_file 13 times. Keyed on
(tool, arguments, error) so that legitimate repetition — polling, git status loops,
retrying with changed arguments — never trips it. Only a call that is identical in all
three is treated as a loop.

Test plan

ruff check — clean on changed files
ruff format --check — clean on changed files
pytest — 192 passed (baseline: 108 partial / full suite uncollectable)
execution:end verified on all four early-return paths plus GeneratorExit
Circuit breaker verified not to trip on repeated-but-legitimate calls
(polling and git status loops covered)

microsoft/amplifier-module-context-simple fix/compaction-token-estimate — the compaction loop that destroyed the session
fix: session state that outlives the turn that created it michaeljabbour/amplifier-runtime#10 — session state that outlives its turn
fix: client-side follow-through for runtime resume/transcript repair (requires runtime#10) michaeljabbour/amplifier-app-tui#259 — client-side follow-through

Generated with Amplifier

On a clean checkout `uv run pytest` did not run at all -- it errored at COLLECTION with `ModuleNotFoundError: No module named 'amplifier_foundation'`. tests/test_goal_loop.py:27 imports ProviderPreference from amplifier-foundation, but the `dev` dependency group listed only amplifier-core, pytest and pytest-asyncio. A collection error takes down the entire suite rather than failing one file, so the whole run was lost to one missing dependency. Adds `amplifier-foundation` to the `dev` group plus the matching `[tool.uv.sources]` git entry. Before: uncollectable -- zero tests ran. After: 173 passed. A partial run with test_goal_loop.py deselected was 108, so the missing dependency was hiding 65 tests. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>

REVIEWERS: read this diff with `git show -w`. The `__init__.py` diff looks enormous (~1,244 lines) because wrapping the body in try/finally required re-indenting ~728 lines by four spaces. `git diff -w` / `git show -w` collapses it to the real change: 46 insertions, 28 deletions. The problem ----------- `execution:end` was not emitted when a turn exited early. In a real incident (session eec9ae98) there were 27 `execution:start` events and only 15 `execution:end` -- and the 12 missing ends matched the 12 cancellations exactly, on both the kernel event log and the UI event stream. The turn state machine was left stuck in "executing" and never unwound. Cause ----- `_execute_stream` emitted the end event on its final line, and several paths returned before ever reaching it: graceful cancellation, immediate cancellation between chunks, a denied `provider:request`, and "no providers available". A consumer that broke out of its `async for` skipped it too. Fix --- Wrap the body from `execution:start` onward in try/finally and emit from the `finally`. This is deliberately NOT an emit bolted onto each early return. That shape is fragile by construction -- the next early return anyone adds silently reintroduces the bug. It also cannot cover the consumer-stops-reading case: `_execute_stream` is an async generator, so Python raises GeneratorExit at the suspended yield and the `finally` still runs, while no per-return patch would ever fire. Tests ----- Three new tests in tests/test_execution_end_invariant.py: - the no-provider early return - the consumer-stops-reading case, verified non-vacuous: the generator is suspended at the yield with zero end events, and the event fires only on `aclose()` via GeneratorExit - a start/end balance invariant across five turns Verification ------------ 176 passed (173 + 3 new). Both changed files pass `ruff check` and `ruff format --check`. Out of scope: two pre-existing F401 lint errors and formatting drift in tests/test_error_propagation.py and tests/test_goal_loop.py. Confirmed present on the pristine branch by stashing, and deliberately left alone. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>

Both dispatch paths -- `_execute_tool_only` (parallel) and `_execute_tool_with_result` (sequential) -- emitted `tool:pre`, passed the result through `coordinator.process_hook_result`, branched on `action == "deny"`, and then executed the ORIGINAL arguments regardless. Every hook that rewrites input (argument normalization, path jailing, secret scrubbing) was a silent no-op: it ran, returned its correction, and was discarded without a word. The subtlety that made this easy to get wrong: the kernel NORMALIZES `modify` away. `emit()` returns `action="continue"` with the modified payload in `data` (amplifier-core `hooks.rs`), so the pattern documented in ORCHESTRATOR_CONTRACT.md:259-261 -- `if result.action == "modify"` -- is unreachable code that can never fire. Reading `data` unconditionally is the only consumption that works. `tool:post` already honored modifications; this makes `tool:pre` symmetric. Two guards, both deliberate: - Adopt only when `data` is a real `dict`. A partial or mocked result can carry a non-dict `data`, and reading it loosely would replace real arguments with nonsense -- a worse failure than the no-op being fixed. - Treat equal content as unmodified. The kernel round-trips the payload through serde, so `data` is always a NEW object; comparing by identity would make every call look rewritten. This is mechanism, not policy: it honors whatever a hook decided without deciding anything itself. It is the precondition for shipping argument normalization as an opt-in `tool:pre` hook rather than baking a stripping policy into a swappable orchestrator that every bundle inherits. Eight new tests: adoption works on both dispatch paths, an equal-content payload is not treated as a rewrite, and five parametrized partial-result shapes (`None`, `{}`, dict without `tool_input`, a MagicMock, a bare string) leave the arguments untouched. Verified: 184 passed (was 176). Both changed files pass `ruff check` and `ruff format --check`. Two pre-existing F401 errors and formatting drift in tests/test_error_propagation.py and tests/test_goal_loop.py were confirmed present before this work and are left alone as out of scope. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>

In session eec9ae98 the SAME failing read_file call was issued 13 times with nothing intervening. 37 distinct tool inputs were issued more than once; 73 calls were redundant. Whitespace-malformed arguments fail deterministically -- same input, same error, forever -- so an agent that cannot notice the repetition burns tokens and wall-clock producing nothing. Add _apply_failure_breaker, applied at both dispatch sites: _execute_tool_only (parallel) and _execute_tool_with_result (sequential). Keyed on (tool, arguments, error), deliberately NOT on arguments alone. Legitimate repeats exist -- polling a file being written, git status in a loop, retrying after fixing something externally -- so only a call that fails the SAME way counts toward the trip. The trip is surfaced to the model, never silently dropped. A silent breaker is the same class of bug as a silent argument rewrite. The tool's real error is preserved verbatim and a note is appended telling the model this exact call has failed N times and to change approach. Threshold is 3, so 13 identical failures become 3 plus an actionable message. Eight new tests: failures below the threshold pass through untouched; the same failure repeated trips and preserves the original error; a different error for the same input does not count; different arguments do not count toward each other; the same error from a different tool does not count; success never trips; non-JSON-serialisable arguments do not break dispatch; and an end-to-end run through real parallel dispatch proves the note reaches the model's tool content. Verified: 192 passed (was 184). Both changed files pass ruff check and ruff format --check. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>

Michael J. Jabbour (michaeljabbour) · 2026-08-20T10:30:07Z

Withdrawing this. It was pushed as a branch directly into this repo; it should
have gone to a fork, and it is now preserved at
michaeljabbour/amplifier-module-loop-streaming on branch
fix/emit-execution-end-on-cancel (13f7fc598204) if any of it is useful.
Closing rather than leaving it open so it does not sit in the queue ahead of
work that was here first.

Same reason: this rewrites amplifier_module_loop_streaming/__init__.py, and
#9 and #10 both touch it — and tests/test_hook_modify.py, which is directly
the subject of one of these commits (a tool:pre hook rewrite being silently
discarded). Worth checking whether that is already in hand there. This branch
also carries a large whitespace-only re-indent from wrapping a body in
try/finally, which would conflict badly with anything else on that file.

Michael J. Jabbour (michaeljabbour) and others added 4 commits August 18, 2026 16:07

Michael J. Jabbour (michaeljabbour) closed this Aug 20, 2026

Michael J. Jabbour (michaeljabbour) deleted the fix/emit-execution-end-on-cancel branch August 20, 2026 10:30

Navigation Menu

fix: always emit execution:end, honor tool:pre rewrites, and break identical failure loops - #41

fix: always emit execution:end, honor tool:pre rewrites, and break identical failure loops#41
Michael J. Jabbour (michaeljabbour) wants to merge 4 commits into
mainfrom
fix/emit-execution-end-on-cancel

Michael J. Jabbour (michaeljabbour) commented Aug 19, 2026

Uh oh!

Michael J. Jabbour (michaeljabbour) commented Aug 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

Michael J. Jabbour (michaeljabbour) commented Aug 19, 2026

Summary

Commits

Test plan

Related

Uh oh!

Michael J. Jabbour (michaeljabbour) commented Aug 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant