From 98121e3a9c267615e4a0db082a963d13da590348 Mon Sep 17 00:00:00 2001 From: "Michael J. Jabbour" Date: Wed, 19 Aug 2026 01:14:39 -0400 Subject: [PATCH] docs: remove unreachable `action == "modify"` branch from orchestrator contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The orchestrator contract told authors to consume hook modifications with `if pre_result.action == "modify"`. That branch can never execute -- `emit()` normalizes the aggregate result to `continue` on every path. Verified against crates/amplifier-core/src/hooks.rs: - no handlers registered (:170-174) -> Continue, with `data` - no matching entries (:180-184) -> Continue, with `data` - normal path (:269-274) -> Continue, with `value_to_map(¤t_data)`, under the comment "Return final result with potentially modified data" `modify` is a handler-to-handler chaining semantic *inside* the dispatch loop; the payload reaches the caller in `data`, never via the action. Replaces the dead branch with the consumption that actually works -- read `data` unconditionally, guarded on it being a dict -- and adds a "Consuming modifications" section explaining why. Also documents the interaction at hooks.rs:249-267, currently written down nowhere: if any handler on the same event returns `inject_context` or `ask_user`, that handler's result is returned instead of the accumulated payload, so an earlier handler's modification is silently discarded, and on the approval path the returned result carries no `data` at all. Docs-only. No code or behavior change. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- docs/contracts/ORCHESTRATOR_CONTRACT.md | 34 ++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/docs/contracts/ORCHESTRATOR_CONTRACT.md b/docs/contracts/ORCHESTRATOR_CONTRACT.md index c7e8d300..29f08453 100644 --- a/docs/contracts/ORCHESTRATOR_CONTRACT.md +++ b/docs/contracts/ORCHESTRATOR_CONTRACT.md @@ -256,10 +256,6 @@ if pre_result.action == "deny": # Don't execute tool return ToolResult(is_error=True, output=pre_result.reason) -if pre_result.action == "modify": - # Use modified data - data = pre_result.data - if pre_result.action == "inject_context": # Add feedback to context await context.add_message({ @@ -272,8 +268,38 @@ if pre_result.action == "ask_user": approved = await request_approval(pre_result) if not approved: return ToolResult(is_error=True, output="User denied") + +# Adopt handler modifications. `emit()` ALWAYS populates `data`, so read it +# unconditionally -- see "Consuming modifications" below. +if isinstance(pre_result.data, dict): + data = pre_result.data ``` +#### Consuming modifications + +**Do not branch on `action == "modify"`.** `emit()` never returns it. + +`modify` is a handler-to-handler chaining semantic *inside* the dispatch loop: +each handler that returns `modify` updates the payload passed to the next one. +The aggregate result handed back to the caller is always `action="continue"`, +with the possibly-modified payload in `data` +(`crates/amplifier-core/src/hooks.rs`, "Return final result with potentially +modified data"). `data` is populated on every path -- no handlers registered, +no matching entries, and the normal path alike. + +An orchestrator that checks `action == "modify"` therefore contains a branch +that can never execute, and every handler which rewrites event data becomes a +silent no-op: the handler runs, returns its correction, and the original data is +used anyway, with no error and no log. That is a real failure mode -- argument +normalization, path jailing, and secret scrubbing all depend on this path. + +**One interaction is easy to miss.** If any handler on the same event returns +`inject_context` or `ask_user`, that handler's result is returned *instead of* +the accumulated payload, so modifications made by earlier handlers are +discarded. On the approval path the returned result carries no `data` at all. +A modifying handler is therefore only effective when no other handler on that +event injects context or requests approval. + ### Context Management Manage conversation state: